mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-25 03:42:06 +00:00
dfd3811eee5083995823dde03b40cd618a540baa
263 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
dfd3811eee |
feat(api): bulk-mutation endpoint + single SSE batch event (TASK-1668) (#669)
* feat(api): bulk-mutation endpoint + single SSE batch event (TASK-1668)
Add POST /workspaces/{ws}/items/bulk accepting item IDs + a verb
(archive, move, tag, untag, set-priority, assign). The lane-header
bulk actions operate on a whole filtered lane, so the endpoint emits
ONE items_bulk_updated SSE event and ONE item.bulk_updated webhook for
the batch instead of per-item fan-out.
Reuses the existing store paths (UpdateItemWithPreCheck / MoveItem /
DeleteItem) rather than re-implementing writes; the open-children
guard runs per status-bearing move exactly as the single PATCH path
does (force-overridable). Per-row failures are collected into the
response envelope (updated/failed/total) rather than aborting the
batch. Editor/owner gated.
Frontend client + TS types follow in TASK-1669; UI wiring in TASK-1672.
Parent: PLAN-1667.
* fix(api): per-item visibility + collection-move guard on bulk endpoint per Codex review (round 1)
- Enforce per-item collection visibility (checkItemVisible) in the bulk
loop so a member with collection_access="specific" can't bulk-mutate
items in hidden collections by guessing refs; report invisible rows as
not-found. Also gate the move target collection on visibility.
- Route bulk collection moves through MoveItemWithPreCheck with the
open-children guard (destination schema), closing the bypass where a
collection move + terminal status could mark a parent terminal with
open children. Status-only moves already ran the guard.
- Tests: status-move + collection-move guard coverage (reject + force
override + mutation-safety).
* fix(web): consume items_bulk_updated SSE event per Codex review (round 2)
The bulk endpoint emits one items_bulk_updated event, but the SSE
service only listened for the fixed ITEM_EVENTS list — so a bulk
mutation left other tabs/sessions stale until an unrelated sync fired.
Route the batch event through the existing sync_required path: it
carries item_ids + a max seq but no per-item field payload, so an
incremental /items-changes delta reconciles every affected row by seq.
Broadcast so peer tabs reconcile too.
* fix(api): scope bulk SSE event per-collection, drop item_ids per Codex review (round 3)
The batch event published with an empty Collection, which the SSE
filter treats as workspace-level: restricted members received bulk
events for hidden collections (leaking item_ids/op/count) while guests
with grants were dropped entirely and stayed stale.
Emit one items_bulk_updated event per affected collection with
Collection set, so the existing visibility filter routes it like any
collection-scoped event. Drop per-item IDs from the SSE payload — a
batch can't be item-grant-filtered for guests on a broadcast bus, so
IDs would leak; recipients reconcile via the /items-changes delta,
which is visibility-filtered server-side (Seq carries the cursor). The
webhook (a trusted workspace integration) keeps the full id list.
Test asserts the event is collection-scoped and carries no item_ids.
* fix(api): bulk collection move notifies both source and target scopes per Codex review (round 4)
A cross-collection move only emitted a batch event for the target
collection, so a restricted member watching the source lane wouldn't
reconcile the item leaving it. Notify both the source and target
collection scopes for moves (still no per-item IDs). Test asserts both
events fire.
* fix(api): suppress itemless batch SSE events for item-grant-only subscribers per Codex review (round 5)
A guest/restricted member with only item-level grants in a collection
could still receive the collection-scoped items_bulk_updated event
(itemless), learning op/count/timing for items they can't see. Extract
the SSE visibility filter into sseEventVisibleFor and add a rule:
itemless collection-scoped events go only to subscribers with FULL
collection access; item-grant-only subscribers reconcile their granted
items via the next resume/reconnect /items-changes sync instead.
Adds a unit test covering the visibility matrix.
* fix(api): validate status override against target schema on bulk collection move per Codex review (round 6)
A status override on a collection move was applied after MigrateFields
but never validated against the target schema, so an out-of-options
value (e.g. status=bogus) could be written. Run ValidateFields on the
final field map before the move. Test asserts the invalid value is
rejected per-row and the item stays put.
|
||
|
xarmian
|
076fb9b2e7 |
feat(comments): comment editing backend — user_id, UpdateComment, PATCH, SSE (TASK-1663) (#665)
* feat(comments): comment editing backend — user_id, UpdateComment, PATCH, SSE (TASK-1663)
Foundation for comment editing (PLAN-1662). No migration — comments.user_id
already exists (012_users.sql) but was never written or exposed.
- Populate user_id on create/reply: CreateComment takes an explicit userID
param (passed from currentUserID by the handlers, not via the request body
so it can't be spoofed). Expose user_id on models.Comment + all comment
SELECTs/scans. The workspace export path is left as-is — imported comments
keep NULL user_id (admin-only edit), matching the pre-identity fallback.
- Store.UpdateComment(id, body): replaces body + bumps updated_at; the
comments_fts_update trigger re-indexes.
- PATCH /workspaces/{ws}/comments/{commentID}: author-or-admin only
(canEditComment), rejects empty body. Editing is an authorship op, distinct
from delete (item editors). NULL user_id → admin-only.
- comment_updated SSE event: broadcast from the handler; added to the web
sse allowlist + ItemTimeline refresh set.
- web: api.comments.update(), Comment.user_id type.
Tests: author edits own (200), non-author non-admin (403), admin edits
anyone (200), empty body (400), NULL-user_id comment is admin-only.
Parent: PLAN-1662.
* fix(account): detach authored comments on account deletion per Codex review (round 1)
Now that TASK-1663 populates comments.user_id (FK to users.id),
DeleteAccountAtomic would fail on the FK for any user who authored a
comment. Null comments.user_id for the user before deleting the row —
comments live on in soft-deleted/other workspaces; the display-name
author is preserved and the comment just becomes admin-only to edit.
Regression test added.
|
||
|
xarmian
|
e179c595e4 |
feat(comments): paste/drop image attachments in comments + inline render (IDEA-1650) (#663)
* feat(comments): paste/drop image attachments in comments + inline render (IDEA-1650) Comment composers were plain textareas with no upload path, and comment bodies rendered markdown without an attachment resolver — so a `pad-attachment:UUID` reference would never display. This wires both halves end to end: - Compose: paste or drop files into the comment composer (ItemTimeline) and the reply box (TimelineCommentCard). A shared helper (commentAttachments.ts) splices an "Uploading…" placeholder at the caret, uploads concurrently via the existing attachment API, and swaps each placeholder for its `pad-attachment:UUID` markdown ref (image syntax for image MIMEs, link/chip syntax otherwise — mirrors the editor's split). Submit is gated while uploads are in flight. - Display: ItemTimeline lazily HEAD-probes each referenced UUID (reusing the editor's fetchAttachmentMetadata cache), builds a reactive resolver, and threads it into renderMarkdown for comments and replies so refs render as inline images / file chips. - Orphan GC: comment uploads leave attachments.item_id NULL (like the editor), but the GC reference scan only checked items.content/fields. Renamed AttachmentReferencedInItems -> AttachmentReferenced and extended it to scan comments.body, so a screenshot referenced only from a comment isn't reclaimed after the grace period. Added TestOrphanGC_KeepsAttachmentReferencedFromComment. Follow-up refinement (thumbnails + click-to-expand lightbox) captured as IDEA-1660. * fix(comments): escape markdown-significant chars in attachment filenames per Codex review (round 1) Filenames containing [ ] or backslash could break the generated  markdown. P2 (grant-aware upload auth) is a pre-existing endpoint-wide gap shared with the rich editor — tracked as BUG-1661, not fixed here to keep the PR focused. * fix(comments): preventDefault on dragover for file drops per Codex review (round 2) Browsers only deliver a file drop to a custom target if its dragover cancels the default; without it the page navigates to the file. Gated on isFileDrag so in-textarea text drag-drop is unaffected. |
||
|
xarmian
|
1b1068537c |
feat(tags): workspace tag enumeration endpoint + cross-collection filter (TASK-1653) (#658)
* feat(tags): workspace tag enumeration endpoint + cross-collection filter (TASK-1653)
Foundation for the tags feature (PLAN-1652 / IDEA-1649). The write path and
per-collection ?tag= filter already existed; this adds tag enumeration and a
verified cross-collection read so a single tag can group items of any type.
- store: dialect.JSONArrayElements unnests a JSON text-array column
(json_each on SQLite, jsonb_array_elements_text on Postgres);
Store.ListWorkspaceTags returns distinct tags + item counts, ordered by
count desc then tag asc, with the same collection/item ACL filters as
ListItems so counts never leak hidden items.
- server: GET /workspaces/{ws}/tags (handleListTags), respecting collection
visibility + guest item grants.
- models: TagCount{tag,count}.
- cli: client.ListTags + `pad tag list`.
- web: api.tags.list + TagCount type (items.list already forwards `tag`).
- tests: store-level (cross-collection aggregation, collection scoping,
non-nil-empty = empty, archived excluded) and handler-level (a Task + an
Idea sharing one tag; GET /tags counts + ordering).
Parent: PLAN-1652.
* fix(tags): count distinct items per tag, not tag occurrences per Codex review (round 1)
COUNT(DISTINCT i.id) so an item with duplicate tags (e.g. ["ux","ux"]) is
counted once — the write path doesn't enforce per-item tag uniqueness.
Adds a regression test.
|
||
|
xarmian
|
b68164a714 |
feat(report): opt-in 'what shipped' completed-items list (TASK-1641) (#651)
* feat(report): opt-in 'what shipped' completed-items list (TASK-1641)
Add ?include_items=true → completed_items[{ref,title,collection,completed_at}]
on the report: items that reached a positive terminal in the window, deduped
by item (newest completion first), capped at 500 with
completed_items_overflow_count. Same positive-terminal source as
totals.completed (joins live items, deleted_at IS NULL), so the list reconciles
with the count. Opt-in so the interactive dashboard stays count-only; the
print/export report (TASK-1642) requests it.
Web ReportData gains completed_items + the api.report.get includeItems flag.
Parent: PLAN-1628.
* fix(report): scope completed-items list to the item's current visible collection per Codex review (round 1)
The list scoped transitions by st.collection_id (visible at completion) but
returned the item's CURRENT title/ref/collection — so an item completed while
visible then moved to a hidden collection could leak its hidden collection
slug/prefix + current title to a restricted caller. Require i.collection_id to
be in the resolved (scoped) collection set on both the count and list queries.
Adds a move-to-hidden-collection visibility test.
|
||
|
xarmian
|
0d0c660565 |
feat(insights): navigate to past periods (offset + prev/next) (TASK-1639) (#649)
Add an `offset` to the report (periods back; 0 = current, clamped >= 0): window becomes [now - (offset+1)*lookback, now - offset*lookback]. Throughput and cycle-time shift automatically; response echoes `offset` + shifted range. Backend: ReportOptions.Offset + ReportData.offset; handler parses ?offset=. Web: api.report.get passes offset; ReportData.offset typed. Insights page: ◀ Previous / Next ▶ controls (Next disabled at offset 0) + a period label; offset is session-only (not persisted to the layout); resets on window or workspace change. Interim: WIP + status-distribution are hidden when viewing a past period (they're as-of-now) with a note — TASK-1640 reconstructs them historically. Parent: PLAN-1628. |
||
|
xarmian
|
eeff78118b |
feat(insights): per-user layout customization + persistence (TASK-1634) (#645)
* feat(insights): per-user layout customization + persistence (TASK-1634)
Let users personalize the Insights surface, persisted per-user per-workspace:
toggle which metric cards show, and remember the window + collection filter.
Backend:
- migrations 064/043: user_report_layouts (user_id, workspace_id, config JSON,
PK(user_id,workspace_id), ON DELETE CASCADE) — dual-dialect.
- models.ReportLayout (hidden_cards/default_window/default_collections) +
ReportCardIDs/ValidReportWindow validation.
- store.GetReportLayout / SaveReportLayout (ON CONFLICT upsert, both dialects).
- GET/PUT /workspaces/{ws}/report/layout — per-user; PUT sanitizes window +
filters hidden_cards to the known card set. web client + TS type.
Frontend (Insights page):
- loads the saved layout, hydrates window/collections/hidden cards
- a "Customize" panel toggles each card (SvelteSet-backed); each section gated
on !hiddenCards.has(id); Totals always shown
- debounced auto-save, gated on a per-workspace `hydrated` flag so it never
saves during load or stomps another workspace's layout on switch
Single config per user (no named/multiple layouts — deliberate v1 scope).
Parent: PLAN-1628.
* fix(insights): save layout only on explicit user changes, not on load per Codex review (round 1)
The auto-save $effect ran once after hydration (loadLayout assigns reactive
state, then flips hydrated=true), firing a PUT /report/layout on mere page
view — which 401s on no-user/legacy-token sessions and bounces the user to
/login. Replace the effect with a scheduleSave() called only from explicit
handlers (toggleCard, selectWindow, toggleCollection, clearCollectionFilter);
hydration never saves. Also capture wsSlug at schedule time and drop the
pending save if the workspace changes mid-debounce, so A's edit can't land
on B.
|
||
|
xarmian
|
a1d09c90df |
feat(report): windowed project report endpoint + DateBucket dialect (TASK-1630) (#638)
* feat(report): windowed project report endpoint + DateBucket dialect (TASK-1630)
GET /workspaces/{ws}/report?window=week&collections=tasks,bugs returns a
time-bucketed report: created-vs-completed throughput, net flow,
completed-by-collection, and a current status-distribution snapshot.
- Dialect.DateBucket(column, granularity) — day/hour bucketing via fixed-width
substring on the UTC RFC3339 TEXT (identical + exact on SQLite + Postgres;
avoids SQLite 'Z'-parsing fragility). Routes all report date math through it.
- store.GetReport: resolves per-collection done field + positive terminals
(terminal options minus rejected/cancelled/etc.), counts completions from
status_transitions and created from items.created_at, zero-fills buckets.
- HTTP handler + route; web ReportData type + api.report.get client.
- Tests: throughput/totals, negative-terminal exclusion, status distribution,
collection filter, non-status done-field, out-of-window exclusion, hourly
day-window, DateBucket per granularity. Dual-dialect via testStore.
Fixes the response contract that TASK-1632/1633/1635 consume (noted on them).
Parent: PLAN-1628.
* fix(report): scope report to caller's visible collections per Codex review (round 1)
The endpoint sits under RequireWorkspaceAccess (members, restricted members,
guests), but GetReport resolved ALL workspace collections — letting a caller
with access to one collection infer hidden collections' slugs, created/
completed counts, and status distribution. Mirror the dashboard: the handler
computes visibleCollectionIDs() and GetReport restricts to that set
(ScopeToVisible). Empty visible set → empty report. Aggregate reports are a
full-collection-visibility feature; item-level grants aren't surfaced in
workspace-wide counts.
* fix(report): correct visibility scoping for all-access + item-grant callers per Codex review (round 2)
Round 1's scoping had two bugs in how it read visibleCollectionIDs:
1. nil means "all-access" (admin / collection_access=all), but the handler
treated nil as an empty visible set → all-access users got an EMPTY report.
Now nil → ScopeToVisible stays false (full workspace report).
2. For guests, visibleCollectionIDs includes collections visible only via
item-level grants; passing those to the aggregate report leaked the whole
collection's counts. Now mirror the dashboard: when item-level grants are
present, scope to fullCollIDs (full-access collections only).
Adds report handler tests (owner full report + default window) alongside the
store-level scoping test.
* fix(report): bearer-aware admin visibility scoping per Codex review (round 3)
visibleCollectionIDs grants ANY platform admin an unrestricted (nil) view, but
RequireWorkspaceAccess suppresses the platform-admin bypass for bearer auth and
falls through to membership (BUG-1616/1617). So a bearer admin (PAT/CLI/OAuth)
who is only a restricted workspace member could read the full workspace report.
Extract reportVisibleCollections(): gate the admin bypass on cookie auth; for
everyone else resolve actual member/guest visibility, and when item-level
grants exist scope to the full-access collection set only. Adds a cookie-vs-
bearer scoping test (cookie admin unrestricted, bearer restricted-member scoped
to the granted collection, end-to-end through GetReport).
* fix(report): exclude soft-deleted items from completion counts per Codex review (round 4)
status_transitions rows survive a soft delete (only a HARD delete cascades
them), so a completed-then-soft-deleted item still counted toward completed /
completed_by_collection while created and status_distribution (which filter
deleted_at IS NULL) excluded it — inconsistent totals. Join live items in both
completed queries. Adds a regression test.
|
||
|
xarmian
|
08f76f3486 |
fix(deps): bump go-jose/v3 to v3.0.5 to clear GO-2026-4945 (BUG-1619) (#634)
* fix(deps): bump go-jose/v3 to v3.0.5 to clear GO-2026-4945 (BUG-1619)
GO-2026-4945 — Go JOSE panics in JWE decryption. Reachable via
github.com/ory/fosite v0.49.0 from internal/server/handlers_oauth.go's
handleOAuthAuthorize (govulncheck call chain into jose.ParseSigned /
JSONWebSignature.Verify / etc).
Drop-in dependency bump, no API changes. Post-bump govulncheck reports
0 reachable vulnerabilities.
Discovered during v0.6.0 release pre-flight (PLAYB-1160 step 1) — CI
run 26518095093 failed govulncheck after the most recent main push,
even though nothing about that change touched OAuth. Same shape as
TASK-1583 (the golang.org/x/net bump earlier this cycle).
* test(open-children-guard): filter readParentRef by SourceRef to fix race flake (BUG-1621)
readParentRef was matching the first LinkType=="parent" link in
GET /items/{ref}/links's response — but GetItemLinks returns links
in BOTH directions (WHERE source_id = ? OR target_id = ?). After
TestOpenChildrenGuard_PatchAtomicRejectionPreservesParentLink attaches
a child task to its target plan, the response contains two parent
links: target→oldParent AND child→target. ORDER BY created_at DESC
ties at sub-microsecond resolution; SQLite tiebreaks on rowid; under
race-scheduler CI load the child→target link sometimes lands first,
the helper returns the target's OWN ref as the "parent", and the
setup-sanity assertion fails with "parent should start as PLAN-1,
got PLAN-3" (where PLAN-3 = target itself).
Filter on SourceRef == itemRef so only the item's OUTGOING parent link
matches. Test-file-only change.
Latent since PR #571 (IDEA-1494,
|
||
|
xarmian
|
83716a65fc |
fix(backlinks): scope cross-workspace admin enumeration to membership for bearer auth (BUG-1617) (#633)
Companion to BUG-1616. The admin platform role granted unrestricted
cross-workspace visibility at the STORE layer too: `GetCrossWorkspaceBacklinks`
ran `ListWorkspaces()` for any user with `Role=admin`, and
`ResolveBacklinksVisibility` short-circuited to `(nil, nil)` for the
same role check. Both fired BEFORE the BUG-1616 middleware gate could
deny the request, so a bearer-borne admin (CLI / PAT / MCP) could
enumerate cross-workspace backlinks from every workspace on the server.
Policy: bearer-borne admin gets STRICT membership enumeration — no
guest-grants fallback. Matches RequireWorkspaceAccess's membership-only
stance from BUG-1616. Cookie-session admin keeps the global view
(preserved web-UI affordance).
Threads `authIsBearer bool` from the HTTP boundary (via the new
isBearerAuth helper) into the store layer:
- `Store.ResolveBacklinksVisibility` — admin bypass now gated on
`!authIsBearer`; bearer-admin falls through to the regular
member/grants pipeline. Also tightens the "no visibility" return
shape from `(nil, nil)` to non-nil empty slices so callers can
distinguish "unrestricted" from "explicit empty" — closes a
latent ambiguity that doesn't fire in current callers but would
if any future caller bypassed the upstream membership filter.
- `Store.GetCrossWorkspaceBacklinks` — new switch:
- cookie admin → ListWorkspaces (unchanged)
- bearer admin → GetUserMemberWorkspaces (NEW, strict
membership; no grants fallback)
- non-admin → GetUserWorkspaces (unchanged; memberships
∪ guest-grant workspaces)
- `Server.guestResourceFilterCore` — admin short-circuit now gated
on `!isBearerAuth(r)`; bearer-admin delegates to the store-side
helper with the bearer signal threaded through.
- `handlers_backlinks.go` — pass `isBearerAuth(r)` to
`GetCrossWorkspaceBacklinks`.
New `Store.GetUserMemberWorkspaces` helper — the first half of
`GetUserWorkspaces` without the UNION-with-grants block. Used by the
bearer-admin path; existing callers continue to use `GetUserWorkspaces`
unchanged.
Tests:
- `wiki_links_xws_test.go`:
- Updated `TestWikiLinks_CrossWorkspaceAdminSeesAllWorkspaces` to
cover both `authIsBearer=false` (cookie, sees all) and
`authIsBearer=true` (bearer, sees none) cases.
- NEW `TestWikiLinks_CrossWorkspaceBearerAdminGrantOnlyWorkspaceFiltered`
— bearer-admin with a guest grant on workspace C still gets ZERO
cross-ws rows from C (Codex round-2 finding).
- NEW `TestWikiLinks_CrossWorkspaceBearerAdminSeesMemberWorkspaces`
— positive control: bearer-admin who IS a member sees the row.
- Extended `TestResolveBacklinksVisibility_RoleMatrix` with two
bearer-admin subtests (non-member workspace → empty; member
workspace → unrestricted).
- NEW `handlers_backlinks_admin_bearer_test.go::TestCrossWorkspaceBacklinks_AdminBearer_OnlySeesMembershipWorkspaces`
— full HTTP integration test, both cookie and PAT-bearer subtests.
- All existing callers updated to pass `false` for `authIsBearer`
(preserves current cookie-session / non-admin behavior).
Verification: full `go test ./...` green; `make lint` clean;
Codex round 2 review CLEAN.
🤖 BUG-1617
|
||
|
xarmian
|
f48c99e421 |
fix(auth): scope admin platform role to cookie session auth (BUG-1616) (#632)
The admin platform role granted owner-level access to every workspace on every surface, including bearer-borne callers (PATs on /api/v1, CLI session bearers, PATs/OAuth on /mcp). A user with "All current workspaces" consent on an MCP client — or a leaked admin token of any kind — could reach data the admin never joined. Policy: the admin global bypass now fires only for cookie session auth (web UI / SPA / /console/admin). Bearer-borne callers fall back to a strict workspace_members check (membership-only; no guest-grants fallback either). Gated four sites in lockstep: - RequireWorkspaceAccess (internal/server/middleware_auth.go) — covers /api/v1/* routes; emits the existing not_a_member MCP authz denial metric on bearer-admin denials. - handleSSE entry (internal/server/handlers_events.go) — adds an explicit GetWorkspaceMember check for bearer-borne admin after resolveWorkspace's global slug lookup. - sseSubscriberStillHasAccess (internal/server/handlers_events.go) — per-tick revalidation now matches entry-time policy. - computeSSEVisibility (internal/server/handlers_events.go) — bearer-admin gets a real VisibleCollectionIDs filter instead of "no filtering"; a bearer-admin who's a member with collection_access=specific is now correctly scoped. - authorizeCollabAccess (internal/server/handlers_collab.go) — WebSocket collab upgrade gate; same membership-only stance. New shared helper isBearerAuth(r) folds two signals (Authorization: Bearer header OR ctxIsAPIToken stash) so MCP-dispatcher synthesized requests and CLI session bearers are both covered. Mirrors the dual check middleware_csrf.go already uses. Tests (9 total): - middleware_auth_admin_token_gate_test.go (5) — PAT denied/allowed permutations, CLI session-bearer denial, cookie-session bypass preserved. - handlers_admin_bearer_gate_test.go (4) — SSE revalidation, visibility filter, collab WebSocket auth. Companion BUG-1617 (store-layer admin bypass in backlinks visibility) tracked separately. |
||
|
xarmian
|
225fb4a53f |
Wire upgrade CTAs with Stripe-ready billing flow (TASK-800) (#629)
* feat(billing): add billing_available session flag gated on PAD_BILLING_AVAILABLE (TASK-800)
Add Server.billingAvailable field set by SetBillingAvailable(), called from
cmd/pad/main.go when PAD_BILLING_AVAILABLE=true|1. Expose the flag as
billing_available in both the setup-state and authenticated session payloads
(value: cloudMode && billingAvailable) so the web UI can gate Stripe CTAs
without a code change at deploy time. False by default.
* feat(billing): wire upgrade CTAs, checkout POST flow, plan section, clickable limit toasts (TASK-800)
Frontend prep work gated on authStore.billingAvailable (from billing_available
session field). When false, upgrade buttons remain hidden and the "coming soon"
note stays in place — flip PAD_BILLING_AVAILABLE=true at deploy time.
Changes:
- client.ts: add billing_available to AuthSession; add api.billing.createCheckoutSession()
(POST /billing/checkout → parse {url} → caller does window.location.href)
- auth.svelte.ts: billingAvailable getter
- console/billing: replace STRIPE_AVAILABLE=false with $derived(authStore.billingAvailable);
fix GET→POST on upgrade buttons; add ?checkout=cancelled banner; add cancelled style
- console/settings: new cloud-mode-gated "Plan" section with current plan + upgrade/manage link
- All 11 limit-hit sites: replace plain-text '/console/billing' appendage with
toastStore.show(msg, 'error', 6000, '/console/billing') so the toast is clickable
* docs(billing): document pad-cloud CSRF and error-envelope contract divergences in createCheckoutSession (TASK-800)
|
||
|
xarmian
|
342679a364 |
Standardize plan-limit error envelope across HTTP/MCP/CLI/UI (TASK-788) (#628)
* fix: limit-hit responses were actively broken — garbled toasts, no upgrade signal
The limit enforcement responses (plan_limit_exceeded on 403) used a flat
body shape {"error": "plan_limit_exceeded", ...} that is incompatible with
every consumer: the frontend PadApiError parser, the CLI parseError path,
and the MCP classifyHTTPStatusKind all expect {"error": {"code": ...,
"message": ...}}. As a result, hitting any of the 5 plan limits (items,
members, workspaces, api_tokens, webhooks) produced garbled toasts with
undefined message text and zero upgrade signal.
Fix:
- writePlanLimitError now emits the standard nested error envelope with a
human-readable message sentence and limit details in error.details.
- CLI parseError now correctly surfacing the message (net positive, no
code change needed).
- MCP classifyHTTPStatusKind: adds ErrPlanLimitExceeded to the taxonomy
and the allowedStructuredErrorCodes whitelist so 403 plan-limit errors
pass through with code + details rather than collapsing to
ErrPermissionDenied (TASK-788).
- Frontend: exports isPlanLimitError() type-guard and planLimitMessage()
formatter from client.ts; all 4 limit-hit write call sites (item create,
member invite, workspace create, token create) now branch on the code and
show an upgrade-signal message pointing at /console/billing.
- Test: updates handlers_workspace_cap_test.go to the new body shape; adds
TestPlanLimitError_ResponseShape covering members_per_workspace limit hit.
TASK-788
* fix(R1): cover MCP stdio path, 5 more item-create sites, polish message wording
Finding A — MCP stdio transport was missing plan-limit coverage:
- cli/client.go: add PlanLimitDetails struct, AsPlanLimit() helper, and
WritePlanLimitError() that emits the pad-structured-error/v1 marker so
the MCP stdio classifier can lift code + details instead of falling
through to ErrServerError.
- cmd/pad/main.go: wire the WritePlanLimitError branch into all three
CreateItem call sites (item create, convention activate, playbook activate).
- internal/mcp: add TestClassifyHTTPStatus_PlanLimitPreservesCodeAndDetails,
TestClassifyHTTPStatus_Generic403FallsToPermissionDenied,
TestClassifyExecError_PlanLimitMarkerLiftsStructuredPayload, and
TestClassifyExecError_PlanLimitWithoutMarkerFallsThrough.
Note: extractUpstreamErrorEnvelope already parses details (json.RawMessage
field) — the codex concern about it being silently empty was a false alarm;
no fix needed there.
Finding B — 5 more item-create entry points were unguarded:
- EditorBubbleMenu.svelte (inline wiki-link capture)
- Sidebar.svelte (quick-add)
- roles/+page.svelte (board new-item, was console.error only; adds toastStore)
- conventions/+page.svelte
- playbooks/+page.svelte (both create and duplicate paths)
B1/B2 polish — server message is now statement-of-fact only, no doubled
upgrade verb. planLimitMessage() drops "Upgrade to Pro to add more." (each
surface appends its own CTA). limitStr uses hyphenated adjective form
"3-member" / "10-item" (compound modifier before "limit").
TASK-788
* feat(task-788): extend MCP-stdio plan-limit coverage to workspace, invite, webhook
Wire WritePlanLimitError into three additional CLI command error paths so
the MCP stdio classifier surfaces ErrPlanLimitExceeded with details instead
of falling through to ErrServerError:
- workspaceCreateCmd: check before fmt.Errorf wraps the APIError
- inviteCmd: check before returning the raw error
- webhooksCreateCmd: check before returning the raw error
Add TestClassifyExecError_PlanLimitWorkspaceCreate to exercise the full
workspace-create stdio round-trip through classifyExecError, asserting
ErrPlanLimitExceeded code, feature="workspaces", limit, and upgrade_url.
Token create intentionally left bare (agents don't drive token creation).
|
||
|
xarmian
|
a7fb14ee14 |
Reduce free-tier workspace cap from 5 to 3 (TASK-1609) (#627)
Lower DefaultFreeLimits.Workspaces from 5 to 3 to sharpen the price-discrimination delta between Free and Pro ahead of Stripe Live (PLAN-1570 Phase a). Add IDEA-1611 comment on the soft-deleted workspace count behavior. Add store-level and handler-level tests covering the new boundary, pro-tier bypass, override path, and self-hosted no-limit path. |
||
|
xarmian
|
3a04c06684 |
fix(backlinks): suppression must query item_links, not items.parent_id (TASK-1607 followup) (#626)
* fix(backlinks): suppression must query item_links, not items.parent_id (TASK-1607 followup)
The initial TASK-1607 fix routed the parent↔child "Mentioned in"
suppression through `items.parent_id`. That column is empty in
production — 0 of 3,923 items in a live workspace had it set —
because the API path (handlers_items.go::handleCreateItem) writes
parent relationships via Store.SetParentLink, which only touches
the `item_links` table with link_type='parent' (source=child,
target=parent — see migration 023). The deprecated parent_id
column is vestigial. Result: the filter was a no-op everywhere
the user actually encountered the duplication.
Caught when testing on fhir-core/TASK-397 — a task with ~20
children all wiki-linking it, none filtered. Direct sqlite
inspection confirmed: parent_id NULL on every row, but 2,022
'parent' rows in item_links workspace-wide.
The fix:
- Replace the SQL predicate in GetBacklinks and CountBacklinks
with two NOT EXISTS subqueries against item_links — one for
"source is not a child of target", one for "source is not the
target's parent". Both use idx_links_source / idx_links_target
for O(1) lookups per candidate row.
- Drop the TargetParentID field from BacklinksVisibility. The
store no longer needs the handler to plumb it through — the
parent relationship is queried from item_links directly. Same
pattern as children-suppression: both are unconditional and
self-contained.
- Update handlers_backlinks.go to remove the dead plumbing.
- Rewrite createChildItem test helper to use SetParentLink (the
production path) instead of CreateItem with ParentID (the dead
column path). The original test helper made all the
suppression tests pass falsely against the wrong storage.
- Add TestWikiLinks_SuppressionUsesItemLinksNotParentIDColumn:
sets up the parent ONLY via SetParentLink, asserts parent_id
column stays NULL, asserts suppression still works. Regression
guard against routing the filter back through the dead column.
The existing 4 suppression tests still pass against the corrected
mechanism (they now exercise SetParentLink under the hood).
Cross-workspace path remains untouched — item_links is workspace-
scoped same as parent_id was, so the doc comment on
GetCrossWorkspaceBacklinks stays valid.
Refs IDEA-1601.
* fix(backlinks): consult items.parent_id alongside item_links for suppression (Codex P2)
Codex review of the prior commit raised a P2: ItemCreate.ParentID
and ItemUpdate.ParentID still write to items.parent_id directly
without creating an item_links row (handlers_items.go only calls
SetParentLink for the `parent` field path, not for direct
ParentID JSON). The HTTP/CLI path is unaffected — empirically
zero items in production have parent_id set — but a direct
store-API caller (test, future import path, or any code that
bypasses the handler) could leave the suppression invisible
to one of the two storage shapes.
Belt-and-suspenders: the relClause now AND's two pairs of
predicates so suppression triggers whichever shape the parent
relationship lives in.
- Children-suppression: NOT EXISTS item_links AND items.parent_id
comparison
- Parent-suppression: NOT EXISTS item_links AND a correlated
subquery against items (NOT EXISTS items t WHERE t.id = ?
AND t.parent_id = s.id) so we don't have to plumb target's
parent_id through
New regression test (TestWikiLinks_SuppressionFallsBackToItemsParentIDColumn)
exercises the column-only path: CreateItem with ParentID, NO
SetParentLink, asserts items.parent_id IS set and item_links is
empty, then asserts suppression still works in both directions.
CountBacklinks updated identically to stay in lockstep with
GetBacklinks pagination math.
* fix(backlinks): suppression must cover childLinkTypes ('parent' AND 'implements') (Codex round 2)
Codex round 2 caught: the Child Items panel and GetChildItems
both inflate `childLinkTypes = {"parent", "implements"}` (see
items.go:18). My filter only suppressed link_type='parent'. An
'implements' child therefore still appeared in Mentioned in
even though it was visually duplicated in the Children section
above.
Use childLinkTypeSQL() in both NOT EXISTS subqueries (children
direction and parent direction) so the filter stays in lockstep
with the canonical inclusion rule. If a future link type is
added to childLinkTypes, the suppression picks it up automatically.
New regression test
TestWikiLinks_SuppressionCoversImplementsChildLinkType wires a
child via CreateItemLink(link_type=implements) and asserts the
mention is suppressed — pins the lockstep with childLinkTypes.
This was the actual mechanism for the original fhir-core/TASK-397
report: TASK-441 et al. carry "**Implements:** [[TASK-397]]" in
their bodies AND are linked as 'parent' (Pad uses 'parent' for the
explicit hierarchy in that workspace), but a sibling case where
only 'implements' was set would have leaked through round 1's fix.
|
||
|
xarmian
|
4aff8c70a0 |
feat(backlinks): suppress parent↔child mentions from "Mentioned in" panel (TASK-1607) (#625)
On a parent's page (typically a PLAN with many child TASKs), the "Mentioned in" panel was dominated by child tasks that wiki-link back to their parent. Those children are already listed in the Children section directly above the panel, so the same items appeared twice on screen and buried genuine cross-references (sibling plans, retro docs, etc.). Symmetric problem on a child's page: the parent shows up in "Mentioned in" even though it's already in the "Parent: …" header. Suppression is server-side in two parts, modeled on the existing self-link filter in GetBacklinks (s.id != targetItemID): 1. Children-suppression — always-on. Source rows where s.parent_id = targetItemID are dropped. Uses the existing targetItemID parameter; no API change. NULL-safe form so orphan items still surface (raw `s.parent_id != ?` would silently drop NULL parent_id rows under SQL three-valued logic). 2. Parent-suppression — opt-in. New TargetParentID *string field on BacklinksVisibility (zero value = nil = no parent suppression, keeping the ~50 existing test callsites valid). When set, the source row whose id == TargetParentID is dropped. The handler passes item.ParentID from the resolved target. Both apply to CountBacklinks identically so the handlers_backlinks.go same-ws/cross-ws pagination math (which depends on count-vs-fetch agreeing) stays correct. GetCrossWorkspaceBacklinks is unaffected — parent_id is workspace-scoped, so a cross-ws source can't be the target's parent or child by construction. Doc comment added noting this. Tests: - ChildMentionOfParentSuppressed: headline case - ParentMentionOnChildPageSuppressed: symmetric case with/without TargetParentID - SiblingMentionsNotSuppressed: control — siblings of a shared parent that wiki-link each other still surface - OrphanBacklinksUnaffected: regression for the NULL-safe form - PaginationStableAfterSuppression: page 1 + page 2 hit all filtered rows exactly once with no duplicates Promoted from IDEA-1601. Follow-up to [[PLAN-1593]] (which shipped the original wiki-link backlinks reverse index). |
||
|
xarmian
|
c1c9f2ef97 |
fix(share): guard nil grants/share-link slices to prevent ShareDialog TypeError (BUG-1598) (#624)
The Share dialog threw "Cannot read properties of null (reading 'length')"
on any item/collection with zero grants. Store.ListCollectionGrants /
ListItemGrants return a nil slice when there are no rows, which writeJSON
encoded as JSON null; the client then hit grants.length on null.
Server: nil-to-[]{} guard in handleListCollectionGrants and
handleListItemGrants, mirroring the existing pattern in
handlers_share_links.go.
Client: defensive ?? [] in ShareDialog.svelte's loadGrants and
loadShareLinks so a null body can't reach .length checks.
|
||
|
xarmian
|
905876af04 |
feat(backlinks): cross-workspace wiki-links + request-independent ACL (Phase 2b) (#622)
* feat(backlinks): cross-workspace wiki-links + request-independent ACL (Phase 2b) Phase 2b of PLAN-1593 (TASK-1597). Completes the wiki-link reverse index by indexing and surfacing `[[workspace::REF]]` cross-workspace references. Builds on Phase 2a's title work (PR #621). Phase 3 (TASK-1596) owns the UI/MCP/CLI rendering changes. What changed - internal/store/backlinks_visibility.go (new): request-independent ACL helper `Store.ResolveBacklinksVisibility(userID, workspaceID, includeDeletedItems)`. Mirrors the role-determination + collection- merge logic from server.guestResourceFilterCore but doesn't depend on a request context, so cross-ws traversal can compute per-source- workspace ACLs without a `workspaceRole(r)` lookup. The Codex planning-round review caught the prior plan reusing the request- scoped helper as a hidden architectural cost; this is the resolution. - internal/server/server.go: guestResourceFilterCore refactored to delegate to the new store helper. Keeps the request-scoped wrapper signature stable for all existing handler call sites; only the internals move. - internal/links/extract.go: lift the Phase-2a workspace_ref emit gate. WikiLinkKindWorkspaceRef now flows through ExtractWikiLinks alongside ref and title kinds. parseBody recognition was already in place from earlier rounds. - internal/store/wiki_links.go: WikiLinkKindWorkspaceRef branch in replaceWikiLinks stores (target_workspace_id, target_ref) verbatim, resolving the slug→ID via new resolveWorkspaceSlugTx (with per-call cache so repeated `[[ws::X]]` in one body don't re-query). Unknown slugs persist with target_workspace_id=NULL — broken-link semantics, identical to existing ref/title patterns. - internal/store/wiki_links.go: new `Store.GetCrossWorkspaceBacklinks` enumerates accessible workspaces via Store.GetUserWorkspaces (which includes guest-only access — broader than membership query), then per-workspace computes visibility via ResolveBacklinksVisibility and runs the SQL backlinks query with the per-ws (FullCollectionIDs, GrantedItemIDs) predicate inline. Results sorted by updated_at DESC in Go, paginated globally. Per-workspace safety cap (offset+limit) prevents one workspace from dominating the global slice. - internal/store/wiki_links.go: new `Store.CountBacklinks` for same-ws pagination boundary detection. Needed so the handler knows where the cross-ws tier begins for pages 2+. - internal/models/backlink.go: new `SourceWorkspaceSlug string` (omitempty) field. Populated only by cross-ws rows; same-ws rows leave it empty so the existing wire shape is preserved. - internal/server/handlers_backlinks.go: union pagination across same-ws and cross-ws tiers. Same-ws first (matches the renderer's UI mental model — your own workspace's links at the top of the panel). Count-based slice math handles pages 2+ correctly when same-ws is exhausted. Tests - internal/links/extract_test.go: workspace_ref forms emit correctly (bare, display alias, mixed case, invalid-slug fallback to title). - internal/store/wiki_links_xws_test.go (new): six cross-ws scenarios plus a role-matrix test: - end-to-end cross-ws index + query - non-member sees nothing - guest with collection grant sees only that collection - guest with item grant sees only the granted item - unknown workspace slug → broken row, no query results - same-ws rows leave SourceWorkspaceSlug empty - ResolveBacklinksVisibility role matrix (admin/full member/guest with grants/non-member non-grant) Out of scope (Phase 3 / TASK-1596) UI rendering of cross-ws backlinks (workspace badge + workspace- prefixed ref), MCP `pad_item.action: backlinks` cross-ws fields, CLI display tweaks. PLAN-1593 / TASK-1597. * fix(backlinks): admin enumeration + cross-prefix ref fallback + unbounded perWsCap (Codex round 1) Three P2 findings from Codex round 1 against PR #622: Finding 1 — admin users miss cross-ws backlinks. `GetUserWorkspaces` returns only memberships + grant-only guest workspaces, but RequireWorkspaceAccess (middleware_auth.go:481) gives admins implicit access to every workspace. An admin querying for backlinks would silently miss links from workspaces they're not explicitly a member of. Fix: in GetCrossWorkspaceBacklinks, branch on user.Role: - admin → s.ListWorkspaces() (every non-deleted workspace) - non-admin → s.GetUserWorkspaces (memberships + grants) Stale user IDs return empty result rather than erroring. Finding 2 — cross-ws ref matching doesn't handle cross-prefix moves. Same-ws is immune because target_item_id is resolved at parse time and survives renames/moves; cross-ws resolves at query time, so a `[[other-ws::OLD-42]]` row written before the target moved from OLD→NEW collection wouldn't match a query under the NEW ref. Fix: in queryCrossWorkspaceBacklinksForWorkspace, dual ref-match clause: exact `LOWER(wl.target_ref) = LOWER(?)` OR `LOWER(wl.target_ref) LIKE LOWER('%-N')` where N is the item_number from the target ref. Pad prefixes are alphanumeric with no internal `-`, so trailing `-N` uniquely identifies the number suffix — no false positives like "TASK-142" matching "%-42" (LIKE anchors to the trailing literal). Finding 3 — per-workspace cap of 1000 silently broke pagination beyond offset>=1000. The 1000 ceiling was defensive paranoia; the correct math is offset+limit per workspace (worst case all rows come from one workspace and the global slice still needs that many). Fix: drop the 1000 ceiling. perWsCap = offset+limit unconditionally. For runaway offsets the per-workspace transfer cost is proportional; documented as a known characteristic (callers shouldn't be paging past offset=10000 anyway). Regression tests: - TestWikiLinks_CrossWorkspaceAdminSeesAllWorkspaces: admin sees cross-ws backlink without being a workspace member. - TestWikiLinks_CrossWorkspaceRefNumberFallback: move target to new collection, query under new ref, old-ref-stored row still surfaces. PLAN-1593 / TASK-1597. * fix(backlinks): honor OAuth/MCP token workspace allow-list (Codex round 2) Codex round 2 P1: cross-workspace backlinks bypassed the OAuth/MCP token's workspace allow-list (TASK-952). A token consented for workspace A but with the underlying user having access to B would still surface source rows from B via the cross-ws query — leaking data outside the token's consent scope. Fix: thread `allowedWorkspaceSlugs []string` through GetCrossWorkspaceBacklinks. Handler populates it from TokenAllowedWorkspacesFromContext(r.Context()): - nil → no token gate (PAT or pre-TASK-952 token, allow all) - "*" wildcard → allow all - explicit list → strict slug membership Workspace enumeration skips any source workspace whose slug isn't in the allowlist. The same-ws path is unchanged because RequireWorkspaceAccess already gated the target workspace against the allow-list (so we only reach this handler when the target IS in the list). Regression test in wiki_links_xws_test.go covers four shapes: nil, wildcard, target-only (blocks cross-ws), explicit source-workspace (allows cross-ws). PLAN-1593 / TASK-1597. * fix(backlinks): normalize limit at handler boundary (Codex round 3) Codex round 3 P2: the backlinks handler parsed ?limit=N but didn't normalize it before computing the same-ws/cross-ws pagination split. GetBacklinks and GetCrossWorkspaceBacklinks each clamp >300 internally, but the handler's 'remaining := limit - len(sameWs)' used the original (potentially huge) value. With ?limit=301 and more than 50 same-ws backlinks, the first page would mix cross-ws in before same-ws was exhausted, violating the documented tier order. Fix: clamp 'limit' to <=300 at the handler boundary, before any pagination math runs. PLAN-1593 / TASK-1597. * fix(backlinks): normalize same-workspace [[ws::REF]] to ref-kind (Codex round 4) Codex round 4 P2: `[[<current-ws>::TASK-1]]` was being indexed as a workspace_ref row with target_workspace_id = current workspace. But the same-ws GetBacklinks query requires target_item_id (workspace_ref rows leave it NULL), AND GetCrossWorkspaceBacklinks explicitly skips the target workspace — so the link rendered and navigated correctly in the UI but no backlink ever surfaced. The renderer's L307 short-circuits same-workspace fully-qualified form to behave identically to `[[REF]]`; the index must follow. Fix: in replaceWikiLinks, normalize a workspace_ref link to ref-kind when its slug resolves to the current workspace. The promotion canonicalizes the ref (via new links.CanonicalizeRef exported alias) so `[[ws::task-5]]` stores the same canonical shape as `[[TASK-5]]`. Tests: - TestWikiLinks_CrossWorkspaceSameWorkspaceQualifiedNormalized: same-ws fully-qualified `[[ws::REF]]` surfaces in same-ws backlinks and is absent from cross-ws backlinks. PLAN-1593 / TASK-1597. * fix(backlinks): same-ws qualified ref miss doesn't title-fallback (Codex round 5) Codex round 5 P2: my round-4 normalization was too aggressive. It promoted `[[<current-ws>::REF]]` to ref-kind and let the regular ref branch handle it — including the title-fallback path that runs on ref miss. But the renderer's same-ws qualified branch (markdown.ts:472-481) does NOT title-fallback: a ref miss in that path returns the wiki-link verbatim (broken). Only the bare `[[REF]]` path (markdown.ts:513) falls through to title lookup. So my normalization could create ghost backlinks for source bodies like `[[ws::ISO-9001]]` when an item titled "ISO-9001" exists but no ISO collection — the renderer renders broken text, but the index would point at the title-matching item. Fix: handle same-ws qualified refs inline at the top of the loop, BEFORE the switch dispatches. Insert as ref-kind row (resolved or NULL) and `continue` past the switch. Bypasses the title-fallback path entirely, mirroring the renderer's behavior. Regression test in wiki_links_xws_test.go pairs same-ws qualified miss (must NOT title-fallback) with bare ref miss (SHOULD title-fallback) to lock the asymmetry in. PLAN-1593 / TASK-1597. |
||
|
xarmian
|
8e7d4040fd |
feat(backlinks): server-side reverse index for [[...]] (Phase 1) (#620)
* feat(backlinks): server-side reverse index for [[...]] wiki-links (Phase 1)
First phase of PLAN-1593. Today [[REF]] is parsed only at render time
on the client and there's no way to ask "who links to TASK-5?" without
a full-text scan. This change adds a materialized reverse index
(item_wiki_links) that's written every time an item's content changes
and exposes it via REST + CLI.
Phase 1 covers ref-form links only (`[[TASK-5]]` / `[[TASK-5|Display]]`).
Phase 2 (TASK-1595) will extend to titles + cross-workspace; Phase 3
(TASK-1596) adds the web UI panel + MCP action.
What lands here:
* Migrations 061 (SQLite) and 040 (Postgres) create item_wiki_links
with partial indexes on target_item_id, (target_workspace_id, target_ref),
and target_title — the schema accommodates all 5 wiki-link forms
up-front so Phase 2 doesn't ALTER.
* internal/links/extract.go is the canonical parser. It strips fenced
and inline code regions before extracting [[...]] occurrences, so
example refs in docs / code blocks don't pollute the index. Phase 1
emits only WikiLinkKindRef rows; title and workspace_ref kinds parse
successfully but are gated out until Phase 2.
* internal/store/wiki_links.go (replaceWikiLinks + GetBacklinks +
helpers) handles write-time bookkeeping and the read query. Resolution
to target_item_id happens at parse time inside the same transaction
as the items INSERT/UPDATE, so partial state never lands. Broken refs
(target_item_id IS NULL) intentionally persist — they feed a future
broken-links report.
* internal/store/wiki_links_backfill.go + cmd/pad/main.go hook the
idempotent backfill into server startup. Existing items get indexed
on first boot after the migration; subsequent boots are near-no-ops
via an EXISTS short-circuit.
* internal/store/items.go is amended in two places: tryCreateItem
always calls replaceWikiLinks (empty content → no-op DELETE), and
UpdateItemWithPreCheck re-parses whenever input.Content was supplied.
* internal/server/handlers_backlinks.go serves
`GET /api/v1/workspaces/{ws}/items/{itemSlug}/backlinks` with
visibility + guest-grant filtering on the source items.
* internal/cli/client.go adds GetBacklinks; cmd/pad/main.go adds the
`pad item backlinks <ref>` command (registered in groups.go).
Behavior decisions (per PLAN-1593):
- code blocks excluded (fenced + inline)
- self-links filtered at query time (kept in storage)
- repeated mentions stored as separate rows by position
- ordering: source updated_at DESC, position ASC
Tests:
- internal/links/extract_test.go: 26 sub-cases covering ref/title/
workspace-ref discrimination, code-block exclusion (fenced + inline +
unclosed fence), position-is-byte-offset (UTF-8 safety), and edge
inputs.
- internal/store/wiki_links_test.go: 8 integration tests covering the
create/update/delete/self-link/broken-ref/repeated/code-block
scenarios plus backfill idempotence.
All pass. `make check` clean (lint + go test + web build).
Refs: TASK-1594, PLAN-1593, IDEA-1577
* fix(backlinks): visibility-aware pagination + case-insensitive refs per Codex review (round 1)
Two fixes from Codex code review:
P1 — GetBacklinks now takes a visibleCollectionIDs []string argument
that's applied INSIDE the SQL WHERE clause. Previously the handler
fetched LIMIT raw rows and filtered visible ones in Go, so a
restricted user asking for limit=50 could receive an empty page even
when later visible backlinks existed. Pushing visibility into SQL
makes LIMIT/OFFSET count visible rows.
nil → no restriction (owners, editors, root tokens)
[] → see nothing (returns early, no SQL)
[..] → AND s.collection_id IN (?, ?, ...)
Item-level guest grants still apply post-fetch — they're rare enough
that the residual page shrink is acceptable and pushing them into SQL
would balloon the query.
P2 — refPattern now accepts mixed/lowercase refs and parseBody
canonicalizes the prefix to uppercase at the single chokepoint.
Previously the renderer accepted `[[task-5]]` as a real link (its
REF_PATTERN is case-insensitive) but the indexer's ^[A-Z]... pattern
silently dropped it — divergent parsing on the same input. Storage
shape is canonical uppercase so the (workspace, prefix, number)
lookup against collections.prefix (also uppercase) has one shape.
New helper: canonicalizeRef("task-5") → "TASK-5".
Regressions:
internal/links/extract_test.go
+ TestCanonicalizeRef — helper unit tests
+ TestExtractWikiLinks_RefVsTitleFallback updated to assert
mixed/lowercase parses-as-ref-and-uppercases
+ edge-case test renamed from "lowercase ref" to "number-led
not a ref" (lowercase IS a ref now per Codex P2)
internal/store/wiki_links_test.go
+ TestWikiLinks_MixedCaseRefIndexed — `[[task-5]]` produces a
backlink row whose target_ref is "TASK-5"
+ TestWikiLinks_VisibilityAwarePagination — three sub-cases:
nil → all 3, visible-only limit=2 → 2 visible rows (not 1 with
hidden one consuming a slot), empty → 0
All call sites updated (8 in tests + 1 in handler).
`make check` clean (lint + tests + web build).
Refs: TASK-1594, PLAN-1593
* fix(backlinks): SQL-level item-grant filter per Codex review (round 2)
Round 1 fixed pagination for collection-level visibility but Codex
round 2 correctly flagged the same class of bug at the item-grant
layer: `visibleCollectionIDs` returns the UNION (full grants ∪
collections containing granted items), and the handler then
filtered each row's item-level visibility in Go AFTER fetching —
letting hidden rows in a granted-item's collection consume LIMIT
slots.
The refactor moves the precise predicate into SQL. New shape:
type BacklinksVisibility struct {
Unrestricted bool // admin / full-access member
FullCollectionIDs []string // direct collection grants
GrantedItemIDs []string // item-level grants
}
// SQL predicate when Unrestricted=false:
// AND (s.collection_id IN (?...) OR s.id IN (?...))
This matches `guestResourceFilter` (which returns the precise
primitives), so the handler now passes them straight through and
drops the post-fetch filter loop entirely. Pagination is correct
for guests, restricted members, and unrestricted users alike.
New test:
TestWikiLinks_ItemGrantPagination — guest with item-grant on ONE
item in an otherwise-hidden collection sees exactly that one item;
hidden siblings in the same collection do NOT leak in, and limit=2
returns 1 row (not silently shrunken).
Other call sites updated:
- TestWikiLinks_VisibilityAwarePagination → uses
BacklinksVisibility{FullCollectionIDs: ...} and
BacklinksVisibility{} for the no-access case.
- 8 existing tests → BacklinksVisibility{Unrestricted: true}.
- handlers_backlinks.go → no longer calls visibleCollectionIDs;
uses guestResourceFilter exclusively and skips the Go-side filter.
Verification:
- make check clean
- All TestWikiLinks_* pass
Refs: TASK-1594, PLAN-1593
* fix(backlinks): scan EXISTS into bool not int for Postgres parity (Codex round 3)
`SELECT EXISTS(...)` returns boolean on Postgres but integer 0/1 on
SQLite. Scanning into `int` happened to work on SQLite (the modernc.org
driver coerces) but would fail on Postgres — silently disabling the
backfill short-circuit there and meaning upgraded Postgres installs
wouldn't populate backlinks for pre-existing content until each item
got edited.
Fix: scan into bool. Both database/sql drivers in use (modernc.org/
sqlite and lib/pq) coerce their native representation into Go's bool,
so this single shape works on both engines.
make check clean.
Refs: TASK-1594, PLAN-1593
* fix(backlinks): allow CommonMark 0-3 space indented fences per Codex (round 5)
Round 5 flagged two edge cases in the code-stripping pass:
1. Multi-backtick inline code (``see [[X]]``) — traced through the
parser; my permissive close-on-next-backtick logic already covers
it correctly (range = [opener-start, after-closer-run]). Added
a regression test to lock this in:
TestExtractWikiLinks_CodeBlocksExcluded /
"multi-backtick inline code excludes ref"
2. Indented fenced blocks — CommonMark allows 0-3 leading spaces of
indentation before a fence opener (4+ spaces makes it an indented
code block, a different construct). My fencedCodeRanges only
matched fences at column 0, so ` ```\n[[X]]\n```` ` would
render as code in the UI but leak a false backlink. Fixed both
fencedCodeRanges (opener) and findFenceCloser (closer) to skip
up to 3 leading spaces, with a hard cap at 4 (which would be
indented-code, not a fence). Regression test:
TestExtractWikiLinks_CodeBlocksExcluded /
"indented fenced block (CommonMark 0-3 spaces)"
Not addressed:
- Round-4 escape-body parity finding. extract.go mirrors
renderMarkdown's regex (web/src/lib/utils/markdown.ts:300), which
is the actual render-time link parser; wikiLinksToMarkdown's more
permissive escape grammar is editor-serializer-side and the
renderer can't even consume its escaped output. Indexing what the
user actually sees as a link is the correct invariant.
make check clean.
Refs: TASK-1594, PLAN-1593
* fix(backlinks): tilde fences + strict closer lines per Codex (round 6)
Two CommonMark conformance gaps in the code-block stripping pass:
1. Tilde-fenced code blocks (~~~) were ignored. marked() treats them
the same as backtick fences, so a [[REF]] inside a tilde block
would render as code in the UI but leak as a false backlink.
Fixed by parameterizing fenceChar across fencedCodeRanges and
findFenceCloser, with separate handling for the backtick-specific
"no backtick in info string" rule (CommonMark §4.5).
2. Closer-line strictness — CommonMark requires the closing fence
line to contain only the fence + optional trailing spaces. The
previous accept-any-fence-prefixed-line check would terminate
a still-open fence prematurely on a line like ```not-closed,
leaking later refs in the still-rendered code block.
Refs reside in 4 new sub-tests under TestExtractWikiLinks_CodeBlocksExcluded:
- tilde fence excludes refs inside
- tilde fence with language tag
- mixed fence types don't pair
- closer-line strictness — backticks plus other text is not a closer
- closer-line strictness — trailing spaces OK
make check clean.
Refs: TASK-1594, PLAN-1593
* fix(backlinks): inline code closer must match opener length per Codex (round 7)
CommonMark §6.1 requires an inline-code span opened with N backticks
to close on a run of EXACTLY N backticks. The previous "close on next
backtick run of any length" logic would prematurely end the excluded
range on a stray single backtick inside a ``...`` span, leaking any
[[REF]] in the latter half of the code text as a false backlink.
Concrete failure case:
``has ` inside [[X-1]] and more``
→ old: range [0, 7], [[X-1]] indexed (bug)
→ new: range [0, end-of-closer], [[X-1]] excluded (correct)
Fix: track the opener-run length and scan only for matching-length
closer runs. Wrong-length runs in between are code text.
Two new regression sub-cases under TestExtractWikiLinks_CodeBlocksExcluded:
- inline code closer matches opener length — the main case
- single-backtick span unaffected by adjacent multi-backtick run —
asserts the opposite direction (opener=1 doesn't close on ``)
Not addressed:
- Re-flagged round-4/round-7 escape-body parity finding. extract.go
intentionally mirrors renderMarkdown's regex (markdown.ts:300), not
wikiLinksToMarkdown's more permissive escape grammar (markdown.ts:461).
renderMarkdown is the actual link parser at display time; its regex
rejects escaped-`]` bodies, so any link with an escaped `]` in its
body is NOT shown as a clickable link in the UI. Indexing it would
produce phantom backlinks the user can't see. The wikiLinksToMarkdown
permissive grammar is paranoid serialization that the renderer can't
consume — that's a pre-existing inconsistency in the editor pipeline,
not a backlinks bug.
make check clean (lint + tests + web build).
Refs: TASK-1594, PLAN-1593
* fix(backlinks): rune-align snippet end-edge to keep UTF-8 valid (Codex round 8)
The previous snippetAround() trimmed `start` to a rune boundary (so
the leading edge of the snippet was always at a valid codepoint) but
left `end` as a raw +40-byte clamp. When that landed in the middle of
a multi-byte rune — common around emoji or accented text — the
resulting slice was invalid UTF-8 and the JSON encoder would emit
replacement characters in backlink snippets.
Fix: same forward-advance pattern at the end as at the start.
Continuation bytes (10xxxxxx) get skipped until we land on a leading
byte. Going forward keeps the snippet anchored slightly past the
match rather than slightly before it, which is a small UX win
(emoji or accented text right after the link survives intact).
Regression test:
TestWikiLinks_SnippetIsValidUTF8 — pads body with enough 4-byte
emoji on each side that the ±40-byte window cuts through one;
asserts utf8.ValidString on the resulting snippet.
make check clean (lint + tests + web build).
Refs: TASK-1594, PLAN-1593
* fix(backlinks): inline code spans cross newlines, break on blank lines (Codex round 9)
CommonMark §6.1: an inline-code span can cross single newlines but
terminates at a blank line (a line containing no chars or only
whitespace, which ends the enclosing paragraph). My previous scanner
broke at every newline, so multi-line spans like
`pre
[[INSIDE-1]]
post`
would treat the opener as unclosed and leak [[INSIDE-1]] as a false
backlink. Fixed by:
1. The newline branch in the closer scan now peeks ahead via the
new isBlankLineAt() helper. Same-paragraph newlines are
traversed; blank-line breaks terminate the span unmatched.
2. isBlankLineAt() treats any line with only space/tab as blank
(mirroring CommonMark's blank-line definition).
Three new regression sub-cases under TestExtractWikiLinks_CodeBlocksExcluded:
- inline code spans single newline (CommonMark §6.1)
- inline code breaks at blank line (paragraph boundary)
- inline code breaks at whitespace-only blank line
Trade-off: a truly-unclosed inline backtick now consumes from the
opener up to the next blank line instead of just the rest of the
line. False-positive on wiki-links in that span, but the surface
area is small (unclosed backticks are rare in published prose) and
matches the renderer's behavior.
make check clean.
Refs: TASK-1594, PLAN-1593
* fix(backlinks): accept escaped wiki-link bodies per editor grammar (Codex round 10)
After 3 rounds of disagreement, capitulating on the escape-body parity
finding. My position was technically correct for the CURRENT
renderMarkdown behavior (which uses [^\]]+ and can't parse escaped-
bracket bodies), but the editor's wikiLinksToMarkdown grammar at
markdown.ts:461 explicitly produces such bodies — making the
renderer's regex the inconsistent half of the pipeline, not mine.
Mirroring the editor's grammar in the extractor makes the index
forward-compatible: when the renderer eventually gets fixed, no
change here is needed. The cost is a few "phantom" rows in the
interim (indexed links the renderer doesn't currently display as
clickable), but those are harmless and aligned with author intent.
Changes:
- wikiLinkPattern now uses `\[\[((?:\\.|[^\]\\])+)\]\]` — mirrors
markdown.ts:461 verbatim.
- New splitOnUnescapedPipe() helper — scans for the first `|`
that isn't preceded by `\`. Mirrors splitWikiBody at
markdown.ts:664.
- New unescapeWikiBody() helper — undoes `\]`, `\|`, `\\` escapes
in display text and key. Mirrors unescapeWikiBody at markdown.ts:657.
- parseBody() now uses both helpers — split on unescaped `|`,
unescape both sides.
Regression coverage:
- TestExtractWikiLinks_EscapedBodyChars (5 sub-cases): escaped `]`,
escaped `|`, escaped `\`, non-escape backslash passes through,
Position still points at opening `[[` despite escapes.
- TestSplitOnUnescapedPipe + TestUnescapeWikiBody: direct unit
tests for the helpers (round-trip safety vs the editor's
escape/unescape pair).
make check clean.
Refs: TASK-1594, PLAN-1593
* fix(backlinks): preserve display text verbatim per Codex round 11 P3
The previous parseBody trimmed the display side of [[X|Display]] but
the WikiLinkRef.Display contract promises verbatim storage and the
renderer at markdown.ts doesn't trim either. Trimming would silently
diverge on padded display text like [[TASK-1| spaces ]] (renderer
keeps the spaces, extractor stripped them).
Fix: drop TrimSpace from the suffix half of the split. Keep trimming
the key/ref side because refPattern is anchored — a leading or
trailing space in the key would force the body to fall through to
the title kind even though the renderer resolves it as a ref.
Regression test:
TestExtractWikiLinks_EscapedBodyChars / "display text preserved
verbatim (no TrimSpace)"
make check clean.
Refs: TASK-1594, PLAN-1593
* fix(backlinks): distinguish empty display override from no-pipe per Codex round 12
[[REF|]] (explicit empty display) and [[REF]] (no display) are distinct
shapes in the editor: splitWikiBody returns displayOverride="" for the
former, null for the latter, and the renderer uses `displayOverride ??
title` (nullish coalescing, NOT empty-string fallback) so "" is
preserved. The previous extractor collapsed both into display_text=NULL,
violating verbatim-display preservation for the empty-string edge case.
Fix:
- WikiLinkRef gains a HasDisplay bool. parseBody sets HasDisplay=true
iff splitOnUnescapedPipe found a pipe; downstream uses HasDisplay
(not Display!="") to decide whether to persist the override.
- replaceWikiLinks in store: NullString.Valid is keyed off HasDisplay.
display_text='' for explicit empty, NULL for no override.
Regression coverage:
- internal/links/extract_test.go:
"explicit empty display override is distinguished from no pipe"
- internal/store/wiki_links_test.go:
TestWikiLinks_EmptyDisplayDistinct (two-source assert: NOT NULL
for [[REF|]], NULL for [[REF]])
make check clean.
Refs: TASK-1594, PLAN-1593
* fix(backlinks): pointer-typed DisplayText to preserve empty distinction over JSON (Codex round 13)
Round 12 added HasDisplay on the parser side and made the store
preserve display_text='' vs NULL on the DB row, but the wire model
collapsed the distinction at JSON-serialization time:
DisplayText string `json:"display_text,omitempty"`
`omitempty` drops empty strings, so [[REF|]] (empty override) and
[[REF]] (no override) serialized identically on the API and CLI JSON
output. The end-to-end goal of round 12 wasn't reached.
Fix: change DisplayText to *string. nil → no override (field omitted
from JSON via omitempty), pointer to "" → explicit empty override
(field present with empty value). The store's NullString.Valid drives
the assignment, so the SQL round-trip matches the JSON shape.
Knock-on: the CLI's `pad item backlinks` now dereferences the pointer
and prints both populated and empty overrides ("displayed as: ").
Regression coverage:
- TestWikiLinks_EmptyDisplayDistinct extended to assert
withBL.DisplayText is non-nil-pointing-at-"" and noBL.DisplayText
is nil after a GetBacklinks round-trip.
make check clean.
Refs: TASK-1594, PLAN-1593
|
||
|
xarmian
|
de1beb47a9 |
feat(cli): pad library get + list --full + server-side category filter (TASK-1562) (#613)
CLI layer for PLAN-1560 (`pad_library` MCP tool + matching CLI surface).
Wires the HTTP work landed in TASK-1561 through to the `pad library`
subcommands.
## `pad library list` changes
- `--category` is now a server-side filter (the old client-side
display-only skip-loop is dead and removed).
- New `--full` flag. Default JSON output for playbooks now returns the
`summary` field (first non-heading paragraph, ~240 char cap) instead
of the full `content`; `--full` opts back into full bodies for
callers that want to pipe everything.
- Table output gains a summary hint line under each playbook and a
`/pad <slug>` chip when an invocation slug is declared, so the
library becomes self-documenting as a discovery surface.
- `--type` now validates explicitly instead of silently producing an
empty list for unknown values.
## NEW `pad library get <title>`
Calls `GET /api/v1/library/entry?title=X` and renders either a
conventions card (title, category, trigger, surfaces, enforcement,
commands, body) or a playbooks card (title, category, trigger, scope,
invocation slug, argument count, body). Conventions-first precedence
matches `pad library activate`.
JSON output returns the full envelope.
404 errors return a clean `not found in library: "<title>"` message
with exit code 1.
## CLI client
- `GetConventionLibrary(category)` — pass category as a server-side
query param.
- `GetPlaybookLibrary(category, summary)` — same plus the summary
toggle; `summary=true` strips Content and returns Summary instead.
- NEW `GetLibraryEntry(title)` returning `*LibraryEntryResponse`.
- `LibraryPlaybook` gained an omitempty `Summary` field so a single
type round-trips both the legacy and summary shapes.
## Drive-by
Switched `/library/entry` 400/404 from a flat `{error: "..."}` body to
the canonical `writeError(code, message)` envelope used by the rest of
the API. The CLI's `parseError` now hands back a typed `APIError` that
`pad library get` pattern-matches on `Code=="not_found"` for the clean
404 message. Updated `TestLibraryEntry_MissingTitle` and `_NotFound`
to assert the new envelope.
## Verification
go build / go vet / go test ./... all green. golangci-lint clean on
cmd/pad/..., internal/cli/..., internal/server/.... End-to-end smoke
tests via the installed binary confirmed: list summary mode, list
--full, list --category filter, get convention card, get playbook
envelope, get 404 exit-1, --type validation.
Parent: PLAN-1560. Unblocks TASK-1563 (MCP catalog wiring).
|
||
|
xarmian
|
2df6edeaab |
feat(server): library endpoints gain ?category, ?summary, /library/entry (TASK-1561) (#612)
Extends the convention + playbook library HTTP layer to support the shape
the upcoming `pad_library` MCP tool and the updated `pad library` CLI need:
- `GET /api/v1/convention-library?category=X` — server-side filter,
case-sensitive exact match. Unknown categories return an empty slice,
not 404.
- `GET /api/v1/playbook-library?category=X&summary=true` — same filter
plus a new summary mode that strips Content and injects Summary
(first non-heading paragraph, ~240 char cap). Web UI and existing
consumers omit the flag and see the legacy full-body shape. Summary
mode deep-copies category slices so a request never mutates the
package-level library data — TestPlaybookLibrary_SummaryDoesNotMutate
Global pins this.
- `GET /api/v1/library/entry?title=X` — NEW. Returns one matched entry
in a `{type, convention|playbook}` envelope. Conventions-first
precedence mirrors the dispatcher's `library activate` so a title
resolves to the same kind in both surfaces. 400 on missing title,
404 on no match.
Hoisted `playbookSummary` to `collections.PlaybookSummary` so the
bootstrap handler and the new library endpoints share one algorithm.
Bootstrap continues to call it for every playbook entry it returns.
Adds 12 handler tests + the existing bootstrap-summary test stays
green after the move. Lint clean on touched packages; `make check`
gate is blocked by a pre-existing gofmt issue in
internal/store/workspace_members.go captured as BUG-1565.
Parent: PLAN-1560. Unblocks TASK-1562 (CLI) and TASK-1563 (MCP catalog).
|
||
|
xarmian
|
8a85eca713 |
feat(web): admin user table — cheap aggregation columns (TASK-1548) (#603)
* feat(web): add cheap aggregation columns to admin user table (TASK-1548)
Surfaces the per-user aggregations T1544 added to GET /admin/users:
- Workspaces (numeric, after Role)
- Storage (used bytes, formatted via the existing formatStorageBytes
helper — same units the storage-override field accepts)
- Last Write (relative time, color-coded by writeRecency: green <7d,
yellow <30d, red ≥30d, gray italic when never)
- Status pill (replaces the standalone "disabled" badge; renders for
disabled / no-workspace / inactive; suppressed for "active" to keep
the table calm)
Implementation:
- AdminUser interface in admin.svelte.ts gains last_write_at,
workspace_count, storage_bytes, status — matching the server-side
JSON shape from T1544.
- writeRecency() helper in +page.svelte buckets the timestamp into a
CSS class. Visual half of the API's status pill; same age windows.
- .num-cell utility: right-aligned, tabular-nums so digits line up
across rows (workspace_count and storage cells share it).
- edit-row colspan updated to 9 (cloud_mode) / 8 (self-hosted) to span
the new columns.
Frontend purely additive — no API changes, no Go changes. T1549 wires
up pagination + sort + filter; T1550-T1555 build the modal.
Part of PLAN-1542.
* fix: address Codex review on TASK-1548
1. handleAdminGetUser response now includes last_write_at, storage_bytes,
and status — matching handleAdminListUsers' shape. Without these,
the row-merge after PATCH (role change, disable, plan edit) kept
stale values; e.g. disabling an active user wouldn't show the new
"disabled" status pill until the full list reloaded.
- Store.UserStorageUsage: new helper that sums attachments across
all workspaces owned by the user. Mirrors WorkspaceStorageUsage's
definition.
- Store.ComputeAdminUserStatusValue: exported wrapper around the
existing private helper so handler code can compute the pill
value without round-tripping through SearchUsers.
2. writeRecency threshold: 30d is now "stale" (inclusive) rather than
"cold". Matches server-side computeAdminUserStatus which only flips
to "inactive" on > 30 days. Eliminates a 1-day boundary mismatch
between the recency color and the status pill.
3. A11y: write-recency cell now carries aria-label with the bucket
name ("Last write: 12d ago (stale)"), so screen-reader users get
the same meaning the color conveys.
|
||
|
xarmian
|
48323e229e |
feat(admin): GET /admin/users/{id}/metrics windowed engagement metrics (TASK-1547) (#602)
Final backend task for PLAN-1542. Returns three engagement signals that
power the metric tiles on the admin user modal's Overview tab (T1553):
- days_since_write: derived from users.last_write_at (T1543). nil when
the user has never had a write recorded.
- writes_7d: COUNT of write-class activities (created/updated/archived/
restored/moved/commented) authored in the last 7 days.
- collections_touched_30d: COUNT(DISTINCT collection_id) of items the
user has authored writes for in the last 30 days. Goes through
activities.user_id (not items.last_modified_by, which is an attribution
string — see T1543's architecture note).
api_requests_7d is intentionally NOT included; no per-request log exists.
Filed as a follow-up (IDEA-1556) that will add this as an additive,
non-breaking field once the request-log table lands.
Implementation:
- Store.GetUserMetrics in users.go runs three small queries: scalar
SELECT for last_write_at, one COUNT(*) over activities, and a
JOIN(activities, items) for the DISTINCT collection count. All
three are index-backed (idx_activities_user from migration 022).
- No caching layer in this PR. The queries are cheap, and a per-user
short cache fits more naturally at the handler boundary if needed —
premature here.
- Handler handleAdminGetUserMetrics wired at GET /admin/users/{userID}/metrics.
requireAdmin gate; 404 on missing user.
Tests: TestGetUserMetrics seeds a workspace with two collections, six
activities (five inside 7d, one ancient outside both windows), verifies
all three metrics. TestGetUserMetricsEmptyUser covers the no-activity
case (nil days_since_write, zero counts, no error).
|
||
|
xarmian
|
a04e5217c6 |
feat(admin): GET /admin/users/{id}/activity paginated feed (TASK-1546) (#601)
* feat(admin): GET /admin/users/{id}/activity paginated feed (TASK-1546)
New endpoint returns activities originated by the user — item writes,
comments, account-level actions the user took themselves — in reverse-
chronological order with offset pagination.
Scope decision: feed shows activities where activities.user_id = userID
(events the user authored). Admin actions targeting this user as a
subject (role_changed where target_user_id is in metadata) are NOT
included; that "received" sub-feed needs a JSON predicate and is filed
as a follow-up. T1554 (modal Activity tab) consumes the current shape.
Implementation:
- Store.ListUserActivity mirrors the existing ListWorkspaceActivity /
ListDocumentActivity helpers in activities.go. Same column projection,
same LEFT JOIN users u for actor name. Hard cap at limit=50.
- Handler asks for limit+1 rows so it can flag "next_offset" without a
separate COUNT query — trims the extra before responding. Returns
next_offset=null when the page is the last.
- Route wired at GET /api/v1/admin/users/{userID}/activity. 404 on
missing user; requireAdmin gate.
- Pagination is offset-based (matching the sibling endpoints) rather
than cursor-based as the task body suggested. For per-user feeds the
dataset is bounded and between-page drift is acceptable for an admin
tool. Cursor can be added later if needed; the response shape is
forward-compatible (next_offset → next_cursor would just rename).
Test: TestListUserActivity covers cross-user isolation, action filter,
offset pagination across pages without overlap, and the 50-row hard cap.
Part of PLAN-1542.
* fix: address Codex review on TASK-1546
Lift the store-side cap on ListUserActivity from 50 to 100. The handler
caps the public per-page at 50 and asks the store for limit+1 (51) to
flag "more available" without a separate COUNT. Previous store-side
cap of 50 silently truncated that probe, so next_offset would be null
even when row 51 existed — clients iterating at page-size=50 would stop
one page short of the actual end.
The HTTP layer remains the source of truth for the per-page maximum;
the inner cap is now just protection against pathological internal
callers. Regression test seeds 51 activities and verifies the store
returns all 51 when asked.
|
||
|
xarmian
|
eff0824238 |
feat(admin): GET /admin/users/{id}/detail per-workspace breakdown (TASK-1545) (#600)
* feat(admin): GET /admin/users/{id}/detail per-workspace breakdown (TASK-1545)
New endpoint that returns the user vitals plus a per-workspace breakdown
enriched with the aggregations the admin user modal's Workspaces tab
needs: collections_count (excluding system collections — playbooks,
conventions, anything else is_system=1), items_open (status NOT IN a
hardcoded terminal set), items_total, members_count, storage_bytes
(matches WorkspaceStorageUsage's definition), and last_activity_at
(MAX items.updated_at across non-deleted items).
Implementation:
- Store.GetUserWorkspacesDetailed in workspace_members.go uses correlated
subqueries rather than a wide JOIN+GROUP BY — a single user belongs to
at most tens of workspaces in practice, so the readability wins over
micro-optimizing. Caps at 50 rows (frontend caps at 20 in T1552).
- AdminUserWorkspaceDetail embeds the existing AdminUserWorkspace to keep
the JSON shape backward-compatible with /workspaces consumers.
- adminOpenItemTerminalStatuses is a hardcoded list (done/completed/
rejected/archived/implemented/cancelled). A schema-aware terminal_options
check is a separate follow-up — flagged in the struct doc.
- Handler handleAdminGetUserDetail wired at GET /admin/users/{userID}/detail.
Returns 404 on missing user; defensive []AdminUserWorkspaceDetail{}
serialization so JSON consumers see [] rather than null.
Test: TestGetUserWorkspacesDetailed seeds a workspace with two user-facing
collections, one system collection, three items (two open, one terminal),
two members, and one attachment; verifies all six aggregations.
Part of PLAN-1542. T1552 (modal Workspaces tab) consumes this.
* fix: address Codex review on TASK-1545
Three real issues in the items_open count clause, all in the same SQL
fragment:
- Use s.dialect.JSONExtractText("i.fields", "status") instead of the
SQLite-only JSON_EXTRACT(i.fields, '$.status'). The endpoint would
have failed on Postgres deployments.
- Wrap the extracted value in LOWER(COALESCE(..., '')) so items with
NULL/missing status fields still register as "open" (NULL NOT IN
(...) is not TRUE in SQL, which would have undercounted), and so
case-variant statuses match. Matches the interpretation used in
search.go and items.go.
- Source the terminal list from models.DefaultTerminalStatuses rather
than a private list — previous local list was missing 'resolved',
'wontfix', 'fixed', 'disabled', 'deprecated' (overcounting open
items in workspaces that use those statuses).
adminOpenItemsCountClause is now a Store method (was a package-level
fn) because it needs the dialect.
|
||
|
xarmian
|
0c5ec04fac |
feat(admin): extend user list with aggregations + sort/filter (TASK-1544) (#599)
* feat(admin): extend user list with aggregations + sort/filter (TASK-1544)
GET /admin/users now returns per-user workspace_count, storage_bytes,
last_write_at, and a computed status pill (disabled / no-workspace /
inactive / active, with documented precedence). Adds sort and filter
knobs so the table can scale beyond the existing fixed offset/limit.
Store layer:
- AdminUserSearchParams gains Role, Sort, Order, ActiveWithinDays,
HasWorkspaces, Disabled. Pointer types where tri-state ("no filter"
vs. "filter to false") matters.
- AdminUserListEntry wraps models.User with WorkspaceCount, StorageBytes,
Status — returned in AdminUserSearchResult.Users.
- SearchUsers SQL rewritten: LEFT JOIN against grouped subqueries so
one user owning N workspaces with M attachments each still produces
exactly one row (no aggregation explosion). Both subqueries filter
deleted_at IS NULL to match WorkspaceStorageUsage's existing
definition. Allow-listed sort clause prevents injection.
- computeAdminUserStatus exported for unit tests; precedence locked in
by TestComputeAdminUserStatus.
- TestSearchUsersAggregations covers workspace_count + storage_bytes +
status across a three-user fixture and each new filter/sort knob.
Model + scanner:
- models.User gains LastWriteAt. userColumns + scanUser updated; the
legacy callers (GetUser, ListUsers, etc.) inherit the new field for
free via the shared scanner.
Handler:
- handleAdminListUsers accepts the new params: role, disabled,
has_workspaces, active_within_days, sort, order. Tri-state bools
only fire when the query param is present. Response now embeds
workspace_count / storage_bytes / last_write_at / status.
Part of PLAN-1542. Frontend consumption lands in T1548 (cheap columns)
and T1549 (sort/filter UI).
* fix: address Codex review on TASK-1544
- Tri-state bool parsing in handler now uses strconv.ParseBool — accepts
the canonical truthy/falsy variants ("True"/"TRUE"/"t"/"1" and the
parallel falses), and silently ignores garbage values rather than
treating them as false. Closes the "disabled=TRUE silently means
enabled-only" surprise.
- SearchUsers count query no longer joins the storage aggregation when
HasWorkspaces isn't an active filter. The page query still needs both
joins (the row carries the data), but a typical "give me a count"
call no longer scans every live attachment. The workspace_count join
remains conditional on HasWorkspaces filtering.
Status threshold (>30d vs >=30d): the documented spec and impl both say
">30d" — no change.
|
||
|
xarmian
|
0a09c1dca7 |
feat(store): add users.last_write_at column + write-path hook (TASK-1543) (#598)
* feat(store): add users.last_write_at column + write-path hook (TASK-1543)
Engagement metrics need a "last write" signal distinct from last_active_at
(which is bumped on any authenticated request, so it includes reads). Adds:
- Migration 060: users.last_write_at + index. Backfills from activities
table (the canonical record of who-did-what) using the action set
that handlers_items.go / handlers_comments.go actually emit:
created/updated/archived/restored/moved/commented.
- Store.TouchUserWrite(ctx, userID): mirrors TouchUserActivity. Same
5-minute throttle to avoid write-amplification, silent no-op on
empty userID so callers don't have to guard.
- Hook in logActivityWithMetaReturningID (handlers_documents.go) — every
item-write action funnels through this single helper, so one TouchUserWrite
call covers item create/update/archive/restore/move and comment authoring.
- Explicit hook in handlers_attachments.go after CreateAttachment, since
uploads don't go through logActivity.
Test: TestTouchUserWrite covers empty-userID no-op, first-write set,
in-throttle suppression, and out-of-throttle advance.
Architecture note: items.last_modified_by / comments.created_by are
attribution strings ("user"/"agent"/"cli"), not user IDs. The user
identity lives in activities.user_id, populated at the handler layer
where currentUser is in scope. That's why the hook lives in handlers,
not the store layer — and why the backfill reads from activities.
Part of PLAN-1542 (admin user management enhancements).
* fix: address Codex review on TASK-1543
- Add Postgres migration 039 (pgmigrations counterpart to 060). Same
ALTER + index + activities-backfill, using TEXT to match the existing
last_active_at / disabled_at column types in pgmigrations/020-021.
Without this, Postgres deployments silently no-op TouchUserWrite
because the column doesn't exist (and the call's UPDATE error is
swallowed by design).
- Hook TouchUserWrite in handleCreateCommentReply. The reply handler
doesn't go through logActivity (no "commented" activity emitted for
replies — verified by grep), so the activity-helper hook misses it.
Explicit call after a successful CreateComment.
|
||
|
xarmian
|
e27f805ffc |
feat(web): unify dashboard onboarding banners around needs_onboarding signal (TASK-1530) (#594)
IDEA-1516 Phase 3. The pre-IDEA-1516 design split workspace onboarding
guidance across two banners — OnboardingIdeaBanner (gated on the
retired IDEA-1 / BACK-1 / FEAT-1 seed-item pattern from PLAN-1496) and
OnboardingChecklist (gated on a totalItems === 0 heuristic that
predates the canonical needs_onboarding flag from TASK-1504). Both
fired competing CTAs on the same screen; neither read the canonical
signal.
Backend (internal/server/handlers_dashboard.go):
- Add `NeedsOnboarding bool json:"needs_onboarding"` to
DashboardResponse, populated via the existing
Store.WorkspaceHasUserCreatedItems EXISTS query (same predicate
AgentBootstrap.NeedsOnboarding uses). Web reads it from the
dashboard fetch the page already does — no second round-trip
against the heavier bootstrap endpoint.
Frontend:
- Add `needs_onboarding: boolean` to TS DashboardResponse type
- Delete OnboardingIdeaBanner.svelte entirely (signal retired,
no remaining consumers); the back-end onboarding_seed field
stays for now per spec — separate cleanup
- Delete OnboardingChecklist.svelte; replace with
OnboardingNudgeBanner.svelte — single message + "Connect agent →"
CTA that opens the workspace's already-mounted
ConnectWorkspaceModal. Dismissible, preserves the existing
`pad-onboarding-dismissed-{wsSlug}` localStorage key so users who
dismissed the old checklist don't get re-prompted
- Workspace +page.svelte: collapse the two banner blocks into one
gated on `needsOnboarding && !onboardingDismissed`; reshow button
follows the same signal. Drop the now-orphaned `.connect-card`
CSS — its function is subsumed by the banner's CTA. Drop the
unused OnboardingIdeaBanner / OnboardingChecklist imports and the
`onboardingSeed` derived state
Smart-suppression deferred to a follow-up. The existing
api.workspaces.claimCode endpoint returns suppression info but
generates a real claim code as a side effect in the not-suppressed
case — calling it on every workspace page-load with needs_onboarding=true
is awkward. The CTA still opens the modal, which renders its own
suppression state correctly; users get the right experience with one
extra click on the rare suppressed case. A dedicated read-only
GET /workspaces/{ws}/connect-status endpoint is a separate piece
of work.
|
||
|
xarmian
|
8041b46e36 |
fix(sse): surface write errors and link keepalive to IdleTimeout (BUG-1532) (#590)
Two SSE-handler tidy-ups flagged during the BUG-1531 investigation. 1. writeSSEEvent now returns the underlying fmt.Fprintf error. Previously every event/keepalive write swallowed any error from the response writer — when the client TCP went away the handler kept looping, pulling events off the bus channel, and discarding them while waiting for the ctx.Done() cancellation to propagate. Now any write failure exits the handler immediately so the bus subscription is released and we stop fanning broadcast traffic into a dead socket. Marshal errors stay local (don't tear down a healthy stream over one un-marshalable payload). All five callsites + the keepalive Fprintf are updated to log at DEBUG (broken-pipe on client disconnect is normal traffic, not an error worth WARN-level noise) and return. 2. The 30s keepalive interval and the 120s IdleTimeout now live in named constants (sseKeepaliveInterval, httpIdleTimeout) with an init() guard that panics if `3 × keepalive >= IdleTimeout`. Used to be magic numbers in two files; bumping one without the other in lockstep silently created a window where idle SSE streams would get TCP-reset by the http.Server's idle deadline. The init guard fires at process start so a misconfigured constant is visible immediately, not three months from now when someone notices intermittent reconnect storms. Three new tests pin the contracts: - TestWriteSSEEvent_SurfacesWriteErrors - TestWriteSSEEvent_MarshalErrorIsLocal - TestSSEKeepaliveIdleTimeoutInvariant Closes BUG-1532. Full ./internal/server suite passes (~78s). |
||
|
xarmian
|
b801867053 |
chore(web): npm audit fix — svelte 5.55.8, devalue 5.8.1, mermaid 11.15.0 (#589)
* chore(web): npm audit fix — patch-bump svelte, devalue, mermaid
Resolves three advisories surfaced by the Web CI job (and visible on
recent main commits before this):
- svelte: 5.55.5 → 5.55.8 (moderate × 4: SSR XSS via spread,
hydratable Promise XSS, DOM clobbering, ReDoS in
<svelte:element>)
- devalue: 5.6.x → 5.8.1 (high: DoS via sparse array deser)
- mermaid: 11.14 → 11.15.0 (moderate × 4: Gantt infinite-loop DoS,
classDef CSS/HTML injection, config CSS injection)
All three are in-range patch bumps — package.json untouched, only the
lockfile changes. No major bumps, no API churn.
Tiptap deliberately untouched: @tiptap/core, @tiptap/extension-
collaboration, @tiptap/y-tiptap stay at 3.22.5 — the CLAUDE.md
lockstep rule (coordinated bumps + Y.Doc schema-version bump) only
applies when those three move together, and nothing here does.
Verified:
- npm audit: 0 vulnerabilities
- npm run check: 0 errors (existing 6 warnings unchanged)
- make build + make install: clean, server boots and serves the new
bundle.
* test(oauth): TestConsent_ApproveWithSpecificWorkspaces locates connection by shape
The test asserts the persisted oauth_connections row from a specific-
workspaces consent (allowed_workspaces=[alpha,beta]) has
AllCurrentWorkspaces=false and the right slug list. It indexed via
conns[0], which only worked when the approve flow was the most-recent
connection — but the test also calls runAuthCodeFlow above the
assertion to mint a bearer for the introspect call, and that helper
posts allowed_workspaces=["*"] → wildcard connection. With
ListUserOAuthConnections ordered ConnectedAt DESC, conns[0] is now the
wildcard bearer row, not the alpha+beta row this test is verifying.
We can't move the bearer-mint below the assertion (the introspect call
needs the bearer first), and the auth-code response doesn't surface the
request_id we'd need to look up the right connection directly. The
specific-workspaces flow is the only one in this test with
AllCurrentWorkspaces=false, so locate by that shape — surgical fix that
matches the test's actual intent.
Verified with `go test -count=3 -run TestConsent_ApproveWithSpecificWorkspaces`
(deflakes across orderings) and the full ./internal/server suite.
|
||
|
xarmian
|
bfce069f28 |
fix(web,build): search palette hang on numeric query + graceful SSE shutdown (BUG-1531) (#588)
* fix(web,build): search palette hang on numeric query + graceful SSE shutdown (BUG-1531) CommandPalette's reactive `$effect` subscribed to every workspace's `localSearch.epoch` + `localIndex.bootstrapStateFor`. Bare-digit queries short-circuit to `exactItemNumberLookup` (synchronous, very fast) and stacked re-fires of the effect inside one microtask tick whenever an SSE delta arrived — Svelte tripped `effect_update_depth_exceeded` and the palette froze. Treat bare-digit queries the same as `body:` queries (skip the subscription reads) and wrap `doSearch()` in `untrack()` so its internal reactive reads can't smuggle hidden dependencies into the effect. The SSE churn that fanned the loop was rooted in `make install` using `killall -9` — SIGKILL drops every open SSE stream mid-chunk so every browser tab logs `ERR_INCOMPLETE_CHUNKED_ENCODING` and reconnects. Switch to SIGTERM + 5s wait + SIGKILL fallback so the server's existing graceful-shutdown path (cmd/pad/main.go:811-857) actually runs and the http.Server writes a final 0-chunk on each open stream. Follow-up tidy-ups (unchecked write errors in writeSSEEvent, link the 30s keepalive to the 120s IdleTimeout in code) tracked in BUG-1532. * fix(web): track workspace slug in palette $effect per Codex review (round 1) After wrapping doSearch() in untrack(), the workspaceStore.current?.slug read that doSearch performs at line 209 no longer registered as a tracked dep of the search effect. The non-body / non-bare-digit branch still reads the slug via localIndex.bootstrapStateFor(...), so workspace switches re-fire the effect for that branch — but body: and bare-digit queries skip that block entirely. Without an explicit slug subscription they wouldn't re-dispatch on workspace switch; an in-flight server response would land stale, get discarded by isSameDispatch(), and loading could stick true. Hoist `void workspaceStore.current?.slug` into the unconditional void block so all four query shapes re-fire on workspace switch. Refs BUG-1531. * chore: gofmt handlers_claim_code_test.go Drive-by formatting fix to unblock CI on this PR. The file landed slightly unaligned in #586 (TASK-1525) — gofmt straightens the struct tag column on claimCodeResponse. |
||
|
xarmian
|
fc6afd01be |
feat(connect): unified Connect-to-agent modal + claim-code endpoint (TASK-1525) (#586)
* feat(connect): unified Connect-to-agent modal + claim-code endpoint (TASK-1525)
Phase E of PLAN-1519. Repurposes the avatar-menu "Connect a project…"
modal as a one-stop hub where users can connect ANY agent surface
(claim-code → existing OAuth grant, fresh MCP OAuth, or local CLI)
to the current workspace.
Backend
- GET /api/v1/workspaces/{slug}/claim-code — generates a stateless
6-digit HMAC claim code (re-uses the verifier's secret + bucket
math) for the calling member, OR reports `suppressed: true` when
smart-suppression detects the workspace is already covered by one
of the user's active OAuth connections (wildcard OR explicit
allow-list rows). Returns `expires_at` at the current bucket
boundary so the UI can drive a countdown.
- store.IsWorkspaceCoveredForUser — single indexed query against
oauth_connections + oauth_*_tokens; filters by ACTIVE tokens so a
dangling revoked connection row doesn't suppress fresh modals.
- Tests cover 412 (disabled), 404 (non-member), 200 + matching code,
wildcard suppression, explicit-allow-list suppression, and the
revoked-connection-doesn't-suppress invariant.
Frontend
- ConnectWorkspaceModal rewritten as a tabbed unified modal:
- Agent (claim code) — fetches on activate, live countdown,
auto-refetches at bucket roll-over, renders smart-suppression
panel that links to /console/connected-apps, and renders the
locked prompt block
`Authorize the pad workspace '<slug>' with claim code <code>.`
per IDEA-1517 §4.
- MCP setup — subsumed from the now-deleted ConnectMCPModal: URL
block + client-card grid linking to per-client docs.
- CLI — existing install + `pad init` flow, unchanged.
- Default tab: Agent when mcpPublicUrl is set; CLI when not. MCP tab
hidden entirely on self-host without a public MCP URL.
- ConnectBanner simplified: single modal state, generic "Connect an
AI agent to this workspace" copy, no MCP/CLI dual-modal branching.
- ConnectMCPModal.svelte deleted (fully subsumed).
- API client gets `workspaces.claimCode(slug)` + `ClaimCodeResponse`
TypeScript type.
- TopBar + workspace home callsites pass `mcpPublicUrl` from
authStore so the unified modal can pick the right default tab.
Verification
- go build ./... clean
- go test ./... — all packages green (server + store)
- cd web && npm run build — clean
Parent: PLAN-1519. Phases A-D already shipped (oauth_connections
schema, MCP claim action, /authorize redesign, connections-page
mutation UI); this lands Phase E. Phase F (TASK-1526) will wire
post-create auto-open from IDEA-1516's new-workspace modal; Phase G
(TASK-1527) is cross-agent paste validation of the locked prompt
string.
* fix(connect): require membership at claim-code generation; guard modal against stale-response races per Codex review (round 1)
1. Guest-grant generation gap. RequireWorkspaceAccess admits item-grant
guests who aren't workspace members; claim-code REDEMPTION requires
full membership. Generating without the same check handed guests a
valid-looking code + prompt that the claim endpoint always 404s.
Add an explicit GetWorkspaceMember check after getWorkspace and
return 403 not_a_member to fail closed on the same response shape
the redemption path would have used.
2. Stale-response race in the modal. ConnectWorkspaceModal stays
mounted across workspace switches (TopBar reuses the same
instance), so an older claimCode fetch can resolve AFTER a newer
one and stomp claimState with suppression or a code for the wrong
workspace. Add a monotonic seq + captured-slug guard mirroring the
refreshHasAgentActivity pattern already in ConnectBanner.
Test additions:
- TestHandleWorkspaceClaimCode_GrantOnlyGuest_403 asserts a non-member
authenticated caller never gets a 200 + code from the generation
endpoint.
|
||
|
xarmian
|
26aa800b3b |
feat(oauth): connected-apps mutation endpoints + edit UI (TASK-1524) (#585)
* feat(oauth): connected-apps mutation endpoints + edit UI (TASK-1524)
Phase D for PLAN-1519. Adds four mutation endpoints under
/api/v1/connected-apps/{id}/... and extends the console page with
an inline Edit panel per connection card.
Backend (internal/server/handlers_connected_apps.go + server.go)
- PATCH .../name — rename, trim + cap at 120 chars
- PATCH .../flags — atomic set of may_create / all_current /
include_future (rejects toggling
all_current=false when the join table
is empty — empty-allow-list invariant
from IDEA-1517 §3 Acceptance)
- POST .../workspaces — add workspace; membership-checked, 404
uniform when the user isn't a member
(no enumeration leak)
- DELETE .../workspaces/{slug} — remove; idempotent for missing
slugs; rejects last-workspace removal
when all_current=false (same orphan
guard as the flags handler)
All four route through requireConnectionOwner which returns the
same 404 envelope for non-owned connections as the existing
Revoke endpoint. Each handler echoes the updated DTO so the page
can re-render in place; respondWithConnection handles both
active-token chains (via ListUserOAuthConnections) and connections
without token rows (direct fetch of oauth_connections + the access
projection).
DTO (connectedAppDTO) gains name + the three scope flags. Model
already carried the fields (TASK-1522).
Frontend (web/...)
- ConnectedApp TS type extended; api.connectedApps gains
rename/updateFlags/addWorkspace/removeWorkspace methods.
- Connections page: Edit button per card opens an inline panel
with: connection name input (debounced save), three scope-flag
toggles (auto-save), allow-list chips with X-to-remove plus a
workspace picker that lists memberships not already in the list.
- Last-workspace removal disabled at the UI level (chip-remove
disabled when list length <= 1); API enforces the same invariant
if a tampered call slips through.
- Workspaces fetched lazily on first Edit open (cached for the
page lifetime).
Tests
- 7 handler tests cover happy paths + edge cases:
- rename trims/caps, non-owner 404
- flags happy path + empty-allow-list block
- add workspace happy path + non-member 404
- remove workspace happy path + last-blocked + idempotent missing
- loginTestUserAs helper to seed a second user for the non-owner
case (the existing loginTestUser hardcodes a single email).
Parent: PLAN-1519.
* fix(oauth): wildcard→specific toggle now works after pre-stage per Codex review (round 1)
PR #585 round 1 caught that switching all_current_workspaces from
true to false was effectively impossible: the flags handler's
empty-allow-list guard called GetOAuthConnectionAccess, which
intentionally short-circuits on wildcard and reports zero slugs.
Even with join rows present, the guard always tripped. The UI
compounded the issue by hiding the allow-list editor while in
wildcard mode, so users had no way to pre-stage workspaces.
Backend fix: new Store.ConnectionWorkspaceCount(requestID) returns
the raw join-row count regardless of the parent's wildcard flag.
The flags handler now uses this; the remove-workspace handler's
"orphan guard" also routes through the new count (plus an
IsConnectionWorkspaceAllowed probe so a no-op removal of a slug
that isn't even in the list doesn't trip the guard).
Frontend fix: the allow-list editor renders unconditionally inside
the Edit panel. While wildcard is on, an "Inert while wildcard is
on" badge clarifies that staged workspaces don't take effect until
the user flips the flag. Chip-remove disabled only when actively
in specific mode AND about to drop to zero — wildcard-mode removes
are always allowed.
Added TestHandleUpdateConnectedAppFlags_WildcardToSpecific_AfterPrestage
as the regression guard: seeds a wildcard connection, adds one
workspace via the API, then asserts the flag flip succeeds and
the resulting DTO carries the pre-staged slug.
Parent: PLAN-1519.
* fix(oauth): staged-while-wildcard rows now visible per Codex review (round 2)
PR #585 round 2 caught that the round-1 fix (always-rendered
allow-list editor + Backend ConnectionWorkspaceCount) was
incomplete: pre-staging a workspace while wildcard=true succeeded
on the server but the response DTO still suppressed the staged
slugs (ListUserOAuthConnections + respondWithConnection both set
AllowedWorkspaces=nil when AllCurrentWorkspaces=true). The UI
rendered "No workspaces staged" even after a successful add, so
users had no way to see or remove a mistaken staged row.
Backend fix: new Store.ListConnectionWorkspaceSlugs returns the
join table's slugs regardless of the wildcard flag.
ListUserOAuthConnections + respondWithConnection both route
through it now. The hot-path GetOAuthConnectionAccess still
short-circuits on wildcard (correct for the introspection path —
when wildcard is on, slugs are irrelevant for gating); the read-
for-display path needs to surface them.
Frontend fix: isAnyWorkspace now reads the all_current_workspaces
flag directly (drives the "Any workspace" badge), independent of
the slug list. The slug list drives the edit panel chips. Legacy
fallback for missing-flag wire shapes preserved.
Added TestHandleAddConnectedAppWorkspace_VisibleUnderWildcard as
the regression guard: seeds a wildcard connection, adds a
workspace, asserts the slug appears in DTO.AllowedWorkspaces
while AllCurrentWorkspaces stays true.
Parent: PLAN-1519.
|
||
|
xarmian
|
93b9590bf9 |
feat(oauth): consent screen rewrite + new-tables write path (TASK-1523) (#584)
Phase C2 for PLAN-1519. Rewrites the /oauth/authorize consent flow
per IDEA-1517 §2a: collects a display name, three scope flags
(may_create_workspaces, all_current_workspaces, include_future_workspaces),
and the per-workspace allow-list — then writes oauth_connections +
oauth_connection_workspaces instead of session.Extra.
Backend (internal/server/handlers_oauth.go)
- parseConsentPayload returns a structured consentDecision carrying
name, three flags, and resolved workspace IDs. Two-radio +
checkbox UI maps to flags per §2a's table; backward-compat shim
still accepts the legacy allowed_workspaces=["*"] / explicit-list
shape so pre-TASK-1523 clients and fixtures keep working.
- handleOAuthAuthorizeDecide INSERTs oauth_connections (parent) +
oauth_connection_workspaces (children) BEFORE NewAuthorizeResponse.
Ordering matters: an INSERT failure prevents code minting, so
the failure mode is "user retries" rather than "token issued
with no connection-level state" (which the dual-read gate would
silently broaden to no allow-list). A successful INSERT followed
by NewAuthorizeResponse failure leaves an orphan oauth_connections
row — harmless, no token references it. Per-slug INSERT failures
roll back via DeleteOAuthConnection (FK CASCADE clears the join).
- session.SetAllowedWorkspaces call retired; the dual-read
introspection gate from Phase A still consults legacy session.Extra
for pre-TASK-1523 tokens during the soak period.
- /oauth/authorize accepts ?suggested_name= URL param, trimmed +
120-char-capped, threaded through to the template for prefill.
Frontend (consentTmpl)
- Per §2a mockup: identity header, name input (with prefill),
data-permissions block (capability_tier radios — unchanged from
pre-TASK-1523), workspace-access radio ("All my workspaces"
default vs "Only specific workspaces"), conditional picker that
slides in for the specific mode, may_create_workspaces checkbox
(default-on), footer disclosure, and Authorize/Deny buttons.
- Picker shows a search input when the user has >10 workspaces;
per-row role label.
- Mobile layout (max-width: 480px) stacks the actions vertically
with Authorize above Deny.
- "Pick at least one workspace" helper text appears when the
user is in specific mode with zero workspaces selected.
- Empty-workspace case still renders the "create or join one
first" empty state; server validates regardless of JS.
Tests
- 2 new tests: TestConsent_ApproveWithNewShape (end-to-end with
connection_name + workspace_access + may_create_workspaces +
allowed_workspaces — asserts the values land on the
oauth_connections row and join table); TestConsent_SuggestedNamePrefill
(?suggested_name= URL param prefills the form input).
- Existing approve-flow tests updated: the introspection response
no longer carries allowed_workspaces (we stopped writing
session.Extra), so assertions now read the connection row +
join table via ListUserOAuthConnections — same guarantee, new
shape. TestOAuth_Authorize_RendersConsentWhenLoggedIn seeds a
workspace so the consent form renders its allow-list section
(the new template hides it when the user has zero memberships).
Parent: PLAN-1519.
|
||
|
xarmian
|
905baaa010 |
feat(oauth): backfill session.Extra into oauth_connections + switch read path (TASK-1522) (#583)
* feat(oauth): backfill session.Extra into oauth_connections + switch read path (TASK-1522) Phase C1 for PLAN-1519. Seeds existing OAuth grant chains into the new connection tables (Phase A) and switches /console/connected-apps to read from them, retiring the session.Extra parse on the read path. Backfill (internal/store/oauth_connections_backfill.go) - Walks oauth_access_tokens + oauth_refresh_tokens to find every distinct request_id chain (including refresh-only chains). - Picks the newest token row per chain — its session.Extra drives the seeded shape, so a chain whose user re-scoped recently reflects the latest decision. - Maps session.Extra shapes to the new tables per IDEA-1517 §2: no key → all_current=1; ["*"] → all_current=1; explicit slugs → all_current=0 + one join row per slug (added_by='user'). - Resolves slugs → workspace IDs; unresolved slugs (deleted / renamed workspace) are counted + logged at WARN, not fatal. - Idempotent on every INSERT (OR IGNORE / ON CONFLICT DO NOTHING) so re-running on every startup is a cheap no-op once stable. - Returns a BackfillOAuthConnectionsResult so the startup log reports chains_seen / connections_created / workspaces_added / unresolved_slugs — operators see fresh work and notice drift. Read-path rewrite (internal/store/connected_apps.go) - ListUserOAuthConnections projects AllowedWorkspaces from GetOAuthConnectionAccess (oauth_connection_workspaces JOIN workspaces) instead of parsing session.Extra strings. - Hydrates Name + MayCreate + AllCurrent + IncludeFuture from oauth_connections so Phase D's mutation UI has them. - Defensive fallback for chains without an oauth_connections row (any leftover the backfill missed): treats as legacy "any workspace, default-on flags" so the connection still renders. Backfill at startup keeps this branch unreachable in production. - Retires parseAllowedWorkspacesFromSession; the new extractAllowedWorkspacesFromSessionExtra helper in oauth_connections_backfill.go is the only consumer of the session.Extra shape on the store side. Model (internal/models/connected_apps.go) - Adds Name / MayCreateWorkspaces / AllCurrentWorkspaces / IncludeFutureWorkspaces. AllowedWorkspaces semantics stay stable (nil = "any"; explicit slugs = chip list) so the existing DTO + frontend continue working unchanged. Phase D exposes the new fields on the wire. Startup wiring (cmd/pad/main.go) - After srv.SetOAuthServer / SetClaimSecret, run the backfill once. Non-fatal on error (partial state is consistent and the next run completes). Quiet at the Debug level on steady-state re-runs; INFO when fresh work landed. Tests - 8 BackfillOAuthConnections cases: empty DB, pre-TASK-952 (no key), wildcard, explicit list, mixed resolvable/unresolved slugs, multi-row chain newest-row-wins, refresh-only chain, idempotent re-run (verified via post-run row count). - TestExtractAllowedWorkspacesFromSessionExtra replaces the retired parseAllowedWorkspacesFromSession test — covers all three IDEA-1517 §2 input shapes + malformed/non-array defensive cases. - TestListUserOAuthConnections_DeduplicatesChain + TestHandleListConnectedApps_DTOShapeAndAuditEnrichment updated to call BackfillOAuthConnections (the production startup hook) before asserting on AllowedWorkspaces — mirrors the real-world flow now that the read path no longer parses session.Extra inline. Parent: PLAN-1519. * fix(oauth): backfill counters reflect actual new rows per Codex review (round 1) PR #583 Codex review round 1 flagged that the backfill counters over-report on steady-state restarts: - wasFreshlyInserted compared updated_at vs created_at — true for every untouched existing row, so every restart counted every pre-existing connection as "created." - slugsAdded++ ran after AddConnectionWorkspace regardless of whether the INSERT OR IGNORE / ON CONFLICT DO NOTHING hit an existing row. Net effect: startup logs "backfill complete" with non-zero counts on every restart instead of the intended quiet "no-op" path — making real fresh work indistinguishable from steady-state. Fix: probe existence BEFORE the insert on both sides. - backfillOneChain reads GetOAuthConnection first; only sets created=true and runs insertOAuthConnectionIfAbsent on a miss. - Per-slug: IsConnectionWorkspaceAllowed pre-check; skip + don't increment when the row already exists. Two cheap PK / indexed lookups per chain. Pre-Phase-C deployments have small chain counts so the added cost is well below the scan already running. Removed the now-unused wasFreshlyInserted helper. Added an assertion in TestBackfillOAuthConnections_Idempotent that both ConnectionsCreated and WorkspacesAdded report 0 on the second run — the regression guard for this exact finding. Parent: PLAN-1519. * fix(oauth): backfill skips slug re-seed on existing rows per Codex review (round 2) PR #583 round 2 caught that the round-1 fix protected the parent oauth_connections row from re-seed but left the join table mutable from stale session.Extra: When a user removes a workspace from their connection's allow-list via Phase D's mutation UI (RemoveConnectionWorkspace), the next server restart would re-run the backfill, find the parent row intact, and re-INSERT the removed slug from the original session.Extra. The user's removal would silently revert every restart. Fix: backfill is a one-shot seed. Once the parent row exists, the new tables are authoritative — legacy session.Extra is frozen reference data, not a reconciliation source. The slug loop only runs when we just inserted a fresh parent row. Added TestBackfillOAuthConnections_DoesNotResurrectRemovedWorkspace as the regression guard: seeds two slugs, removes one, runs backfill again, asserts the removed slug stays gone and the kept slug is untouched. Parent: PLAN-1519. * fix(oauth): atomic per-chain backfill transaction per Codex review (round 3) PR #583 round 3 caught that round 2's "only seed slugs on fresh parent" gate introduced a permanent-partial-state risk: if the process crashes (or AddConnectionWorkspace errors) between inserting the parent row and finishing the slug loop, the next backfill sees created=false, short-circuits the slug seeding, and leaves the connection permanently scoped to a partial allow-list. Fix: per-chain transaction. Parent insert + every slug insert land in one BEGIN/COMMIT pair; any mid-loop failure rolls everything back. The next backfill then sees the chain as un-seeded and retries from scratch — preserving both round 2's "no-resurrection of user-removed slugs" (existence probe inside the tx) and round 3's "no permanent partial seed" (atomic commit). Scope: per-chain (small tx), not whole-backfill. The original no-transaction rationale was about lock-hold duration across thousands of chains; that doesn't apply at chain granularity (one parent + a handful of join rows = sub-millisecond hold). Removed the now-unused insertOAuthConnectionIfAbsent helper; the INSERTs live inline within the transaction. Added TestBackfillOAuthConnections_AtomicOnMidLoopFailure as the regression guard: forces a mid-loop INSERT failure via a duplicate slug in session.Extra (which violates the join table's PK on the second insert), asserts the parent row rolled back, then runs a clean retry and verifies full seed completion. Parent: PLAN-1519. * fix(oauth): surface store errors from backfill + list path per Codex review (round 4) PR #583 round 4 caught two silent-fallthrough paths that could leak partial/incorrect state instead of failing loudly: 1. Backfill slug loop: GetWorkspaceBySlug errors were treated the same as "workspace not found" — both incremented slugsMissed and continued. A real I/O error mid-loop would commit a partial allow-list, and the next backfill's parent-exists short-circuit would make that partial scope permanent. Fix: distinguish (nil, nil) "not found" from (nil, err) "real failure" — return the error so the per-chain transaction rolls back and the next run retries cleanly. 2. ListUserOAuthConnections hydration: GetOAuthConnectionAccess and GetOAuthConnection errors collapsed into the "no oauth_connections row" defensive-fallback branch, returning the legacy "any workspace, default-on flags" shape. On a real store failure that silently broadens a user's scope — e.g. a connection the user explicitly removed a slug from would render as "Any workspace" until the store recovered. Fix: surface store errors from both calls; the defensive fallback path is now exclusively for HasConnection=false, not for error masking. Both findings tighten the failure mode from "silently emit broadened/partial state" to "surface the error so retries happen against accurate data." Existing tests cover the happy paths; the failure paths are exercised by I/O errors against the same store interfaces (no new test added — the change is "return err instead of swallow it" and the assertion of NOT swallowing is the diff itself). Parent: PLAN-1519. |
||
|
xarmian
|
aec67e202e |
feat(mcp): workspace.create + workspace.claim actions + claim-code mechanics (TASK-1521) (#582)
Phase B for PLAN-1519. Adds two MCP actions so agents can bring a workspace into an OAuth connection without re-auth — the agent-first onboarding story IDEA-1517 §1 set out to fix. pad_workspace.action: create - New action on the shared catalog (stdio + cloud both pick it up). - POSTs to /api/v1/workspaces; handler auto-adds the new workspace to the calling OAuth connection's allow-list (added_by='agent-create') when the grant carries may_create_workspaces=true. Phase A wired the oauth_connection_workspaces table this writes to. PAT / CLI-session callers fall through silently (no request_id → no side effect). - Backed by a new non-interactive `pad workspace create <name>` Cobra command for the stdio MCP shell-out path. `pad workspace init` remains the guided human flow. pad_workspace.action: claim - New POST /api/v1/oauth/claim endpoint redeems a 6-digit stateless HMAC code minted from (user_id, workspace_id, 5-min time bucket) with a sliding 5–10 minute lifetime. Constant-time compare. Code format derived per IDEA-1517 §4. - Verifies workspace membership before code (privilege-escalation guard); uniform 404 envelope so the endpoint can't be used to probe existence vs. membership. - Side effect inserts a row in oauth_connection_workspaces with added_by='claim'. Idempotent — re-claiming returns 200 with already_added=true. - 412 connection_not_persisted when the OAuth grant predates Phase C (no oauth_connections row); 412 claim_disabled when the deployment hasn't wired SetClaimSecret. - `pad workspace claim <code> --workspace <slug>` Cobra command backs the stdio MCP shell-out. MCP server instructions - Appended IDEA-1517 §5 paragraph teaching agents the claim flow as a peer top-level section. Same string lands universally on every MCP handshake response (stdio + cloud both read instructions.md). Tests - 10 claim-code unit tests: determinism, zero-pad, length-prefix collision guard, current/previous bucket accept, aged-out reject, wrong-everything rejects, short-secret fails closed. - 7 handler tests: 412 when secret disabled, 400/404/401 vocabulary, PAT caller note path, 412 connection_not_persisted, idempotent insert. - 5 MCP-catalog tests: actions registered, schema params advertised, description mentions both actions, route mappers produce correct HTTP shape, routeTable carries the entries. - Bumped the existing read-only catalog bijection + fixture-input fixtures so the new actions resolve cleanly. Parent: PLAN-1519. |
||
|
xarmian
|
3f6bcc0ff2 |
feat(oauth): per-connection state tables + dual-read introspection gate (TASK-1520) (#581)
* feat(oauth): per-connection state tables + dual-read introspection gate (TASK-1520) Phase A foundation for PLAN-1519 / IDEA-1517's per-OAuth-connection state overhaul. Promotes the consent-time workspace allow-list out of session.Extra (per-token, re-minted on every refresh-token rotation) into dedicated tables keyed by request_id (the grant chain identifier preserved across rotations). Schema (SQLite migration 059 + Postgres migration 038): - oauth_connections: one row per grant chain with name + three scope flags (may_create_workspaces, all_current_workspaces, include_future_workspaces). - oauth_connection_workspaces: mutable allow-list join table; PK on (request_id, workspace_id); FK ON DELETE CASCADE; added_by audit column. Store (internal/store/oauth_connections.go): Create/Get/Rename/SetScopeFlags/ Add+Remove+IsAllowed/Delete CRUD. GetOAuthConnectionAccess is the hot-path projection — one PK lookup + one indexed join when the wildcard flag is off, nothing else when it's on. Dual-read gate (internal/server/middleware_mcp_auth.go): OR-merges the legacy session.Extra allow-list with the new-table projection. A workspace is allowed iff either source allows it; either source's wildcard makes the gate unrestricted. New tables stay empty until Phase C writes the consent screen, so the dual-read is a no-op until then — and existing OAuth grants keep working unchanged through the Extra path. I/O errors on the new path fall back to the Extra path so a transient outage of the new tables can't regress existing connections. Tests: - 10 store tests cover CRUD, FK cascade, wildcard short-circuit, sorted slug projection, idempotent add/remove, ErrOAuthConnectionNotFound on missing rows. - 11-case table-driven test on mergeAllowedWorkspaces directly verifies PLAN-1519's acceptance criterion: "token with allow-list in session.Extra still passes; token with empty session.Extra but row in oauth_connection_workspaces also passes." Plus wildcard precedence, union dedup, and fail-closed-on-empty-scope. - BenchmarkMergeAllowedWorkspaces measures policy-function overhead on the hot path (the store-side lookup is the other half of the dual-read cost). Parent: PLAN-1519. * fix(oauth): fail-closed on connection lookup error per Codex review (round 1) PR #581 Codex review round 1 caught two issues: 1. middleware_mcp_auth.go: GetOAuthConnectionAccess errors fell through to "no connection" + nil allow-list = unrestricted. Post-Phase-C (when the new tables are authoritative and session.Extra is empty), a DB read error on a scoped token would silently grant access to every workspace the user belongs to. Now fails closed with a 401 matching the IntrospectToken storage-error policy, increments MCPAuthzDenialsTotal{connection_lookup_error} for ops visibility. 2. oauth_connections.go: CreateOAuthConnection's docstring claimed "scope flags default ON if not supplied," but the method writes the three Go bools verbatim — and Go zero-values for bool are false, not true. The schema-level DEFAULT TRUE is unreachable through this path. Docstring updated to clarify that defaults live at the form-rendering layer; the store is a faithful pass-through. Parent: PLAN-1519. * test(oauth): add store-side bench for GetOAuthConnectionAccess per Codex review (round 2) PR #581 Codex review round 2 flagged the docstring references to bench_oauth_connections_test.go pointing at a file that didn't exist — only the in-memory mergeAllowedWorkspaces bench was wired. Add the store-side bench so the documented file is real and PLAN-1519 Phase A's "Hot-path benchmark: dual-read overhead measured and documented" acceptance bullet is satisfied end-to-end. Three shapes covered: Wildcard (PK lookup, join short-circuited), Explicit (PK + indexed scan + small workspaces join), and NoRow (the dominant Phase-A path until Phase C wires the write path). Local numbers (Ryzen 3 5300U, SQLite WAL): 12µs/12µs/37µs respectively — all comfortably sub-millisecond. Parent: PLAN-1519. |
||
|
xarmian
|
2a1a00e385 |
test,docs: blank+onboard+needs_onboarding integration test + CLAUDE.md update (TASK-1507,1508) (#580)
PLAN-1496's final consolidation pair, shipping together because both are small cleanup passes that close the plan out. TASK-1507 (tests): Most of the test coverage required by this task was already added incrementally in the prior PRs that built each surface: - Blank template (4 focused tests, PR #575): TestSeedFromBlankTemplate, TestBlankTemplateShape, TestBlankTemplateExcludesSoftwareCollections, TestBlankTemplateUsesMinimalVocabularies, TestBlankTemplateAppearsInPicker - Onboard auto-seed (PR #576): TestSeedFromTemplateAlwaysIncludesOnboardPlaybook (walks all six templates), TestSeedWithEmptyTemplateNameSkipsOnboard (locks the empty-templateName escape-hatch invariant), TestOnboardPlaybook_Contract (invocation_slug, trigger, mode-enum, ADAPT-DON'T-CURATE rule in the body) - needs_onboarding (PR #578): TestBootstrapNeedsOnboardingFlag (lifecycle: fresh → user item → flag flips), TestBootstrapNeedsOnboardingIgnoresTemplateSeeds (template seeds don't count) - Retired-pattern updates (PR #577): TestSoftwareTemplatesShipNoSeedItems inverse invariant; TestDashboardOnboardingSeed_NilForAllTemplates collapsed from three IDEA-1/BACK-1/FEAT-1 tests. This commit adds ONE integration smoke test that ties the three subsystems together at the bootstrap layer: - TestBootstrapBlankWorkspaceOnboardReady creates a blank-template workspace, fetches bootstrap, and asserts: needs_onboarding=true (nudge fires) AND the onboard playbook is in bootstrap.playbooks AND its status is "active" AND its trigger is "manual" (in the blank template's seeded vocabulary). If any one of the three pieces regresses silently, the integration breaks and this test catches it before /pad onboard stops dispatching on day one. TASK-1508 (docs): - CLAUDE.md "Data Model / Templates" section: added Blank under a new "Custom" category bullet pointing at the new Onboarding section; called out the PLAN-1496 retirement of the IDEA-1 / BACK-1 / FEAT-1 first-person seed pattern; updated design history reference to include PLAN-1496. - CLAUDE.md "API" section: added the /api/v1/workspaces/{ws}/agent/bootstrap endpoint with a note about the needs_onboarding flag (was previously documented only inline in the Playbooks section). - CLAUDE.md: new top-level "Onboarding" section between Playbooks and Testing. Covers: auto-seeded everywhere, surface-agnostic body, adaptation posture (library entries are starting points), the three TASK-1510/1511/1512 mutation primitives, the needs_onboarding bootstrap flag + skill nudge, the four retired surfaces (pad onboard cobra, OnboardingPrimaryRef, *OnboardingItems generators, standalone skill workflow section), and a code map. Verification: - go test ./...: clean (full suite passes including the new integration test) - make lint: 0 issues Parent: PLAN-1496. |
||
|
xarmian
|
96a32aa39d |
feat(bootstrap,skill): add needs_onboarding flag + retire legacy Onboarding workflow (TASK-1504,1505) (#578)
PLAN-1496's bootstrap-signal + skill-cleanup pair, shipped together
because TASK-1505's nudge rendering depends on TASK-1504's bootstrap
field.
TASK-1504 (bootstrap: needs_onboarding):
- internal/store/items.go: new WorkspaceHasUserCreatedItems(workspaceID)
store method. Backed by SELECT EXISTS with the predicate
`source != 'template'` — defined as the inverse of template seeding
rather than enumerating user-side source values, so new attribution
surfaces (mcp, api, future) count automatically.
- internal/server/handlers_bootstrap.go: AgentBootstrap struct gets
the NeedsOnboarding bool field (always emitted — not omitempty,
since the agent reads it on every /pad invocation). BuildAgentBootstrap
computes it via the new store method. On query error the flag falls
back to false (safe default: don't nag).
- Visibility filtering deliberately omitted — needs_onboarding is a
workspace-level state signal, not a per-user view. Two members
reading bootstrap concurrently should see the same answer.
- Two focused tests:
- TestBootstrapNeedsOnboardingFlag walks the lifecycle (fresh
workspace → create user item → flag flips).
- TestBootstrapNeedsOnboardingIgnoresTemplateSeeds locks the
template-seeds-don't-count invariant on the startup template,
which ships seeded conventions, playbooks, and the onboard
playbook itself.
TASK-1505 (skill update):
- skills/pad/SKILL.md:
- Context Loading section: new bullet documenting needs_onboarding
with the exact nudge wording the agent should render when true,
plus the "don't nag past first user item" + "respect prior
decline" rules.
- Onboarding workflow section: deleted (~30 lines). Replaced with
a one-paragraph pointer at the /pad onboard playbook. The skill
is the dispatcher; the playbook body is the script.
- Routing entry under "set up my workspace": simplified from the
bloated PR #577 round-3/4 text into a clean two-bullet form
(canonical phrasing + legacy IDEA-1 phrasing both → /pad onboard).
- internal/mcp/prompts_data.go: the pad_onboard MCP prompt body
was duplicating the same step-by-step script the SKILL.md section
carried. Replaced with the same dispatch-to-playbook pointer.
internal/mcp/prompts_test.go: TestPromptsLockstep_CoreCommands
fragments updated to assert the new dispatch fragments
(`pad playbook list`, `pad playbook show onboard`).
Parent: PLAN-1496.
|
||
|
xarmian
|
0930743304 |
feat: retire IDEA-1 seed pattern + 'pad onboard' cobra + surface blank in init (TASK-1501/1502/1503) (#577)
* feat: retire IDEA-1 seed pattern + 'pad onboard' cobra + surface blank in init (TASK-1501,1502,1503)
PLAN-1496's legacy-onboarding teardown:
TASK-1501 (remove seed items + update banner):
- internal/collections/templates_onboarding.go (and the _product/_scrum
siblings) deleted — these generated the IDEA-1/PLAN-2/TASK-3/DOC-4 +
BACK-1/SPRINT-2/BUG-3/DOC-4 + FEAT-1/FB-2/ROAD-3/DOC-4 first-person
seeds. The /pad onboard playbook (TASK-1499 / TASK-1500) is the
replacement.
- startup/scrum/product templates: SeedItems lines removed.
- post-init banner in printOnboardingHints: now points at "/pad onboard"
in one line, then web UI link, then dashboard hint. The "use pad to
get IDEA-1 / BACK-1 / FEAT-1" branch is gone.
TASK-1502 (retire cobra + OnboardingPrimaryRef plumbing):
- OnboardingPrimaryRef struct field on WorkspaceTemplate removed. The
dashboard's banner auto-discovers seeds via item_number=1 +
source="template" + created_by="system", so the field was redundant
even before retirement.
- onboardingPrimaryRef() helper in cmd/pad/main.go removed.
- 'pad onboard' Cobra subcommand removed (~160 lines). It scanned the
project directory for build/test/CI markers and seeded library
conventions — useful behavior but CLI-only, unreachable from
MCP-only agents. The /pad onboard PLAYBOOK now covers it.
- internal/cli/detect.go and workspace_context_detect.go stay; still
used by the web-side workspace-context save path.
TASK-1503 (Blank in interactive picker):
- The picker already surfaces Blank because templates_picker.go iterates
GroupTemplatesByCategory, and the IDEA-1479 Blank template entry lives
in CategoryCustom. Verified the output renders correctly with the
TASK-1498 description + icon update.
- 'pad workspace init --help' Long now mentions Blank explicitly +
points users at /pad onboard. Helps discoverability without restructuring
the picker.
Test changes (delete or rewrite tests that exercised the retired pattern):
- internal/collections/templates_test.go: six tests deleted (StartupOnboardingItemsOrderAndShape,
ScrumOnboardingItemsOrderAndShape, ProductOnboardingItemsOrderAndShape,
Startup/ScrumProduct/TemplatesDeclareOnboardingPrimaryRef). New
TestSoftwareTemplatesShipNoSeedItems replaces them with the inverse
invariant: software templates ship zero seed items.
- internal/server/handlers_dashboard_test.go: three IDEA-1/BACK-1/FEAT-1
expectation tests collapsed into TestDashboardOnboardingSeed_NilForAllTemplates,
which asserts the auto-discovery finds no seed because seeds no longer
ship. (Hiring + EmptyWorkspace tests untouched — they already expect
nil for unrelated reasons.)
- internal/store/items_test.go: TestSeedCollectionsFromTemplate{Startup,Scrum,Product}RefSequence
and TestOnboardingFlow_FullWalkthrough_{Startup,Scrum,Product} deleted;
these locked the IDEA-1 ref-sequence + walkthrough behavior. Unused
helpers (findItemByTitle, extractStatus, safeFields, setItemStatus,
countItemsInCollection) deleted alongside them.
- internal/mcp/resources_test.go: TestReadItem_PreservesIDEAOneOnboardingBodyVerbatim
→ TestReadItem_PreservesBodyVerbatim. Property is the same (resource
pipeline doesn't mangle markdown), but the fixture is now synthetic
markdown instead of the IDEA-1 seed.
Note: handlers_dashboard.go still has the auto-discovery code path
(onboardingPrimaryCollectionSlugs map + the loop that probes for
item_number=1 + source="template"). It's now dead code — no item
will ever match the criteria after this PR. Left in place for a
follow-up cleanup pass to keep this PR focused.
Parent: PLAN-1496.
* docs: replace 'pad workspace onboard' references with /pad onboard (Codex round 1)
P2 finding on PR #577: README + CLAUDE.md still advertise the
'pad workspace onboard' subcommand in four places (README §Onboard
agents to a new codebase, README §3 Teach your agents the rules,
README CLI Reference, CLAUDE.md CLI). After this branch lands, those
instructions return "unknown command."
Replaced each with guidance pointing at /pad onboard (the playbook,
auto-seeded into every workspace). The library-list commands still
work and stay where they are.
Parent: PLAN-1496.
* docs: replace 'use pad to get IDEA-1' guidance with /pad onboard (Codex round 2)
P1 finding on PR #577: README.md:33-39 and CLAUDE.md:111-117 still
told users to 'use pad to get IDEA-1' after the post-init banner.
Since this branch deletes templates_onboarding.go and stops seeding
IDEA-1/PLAN-2/TASK-3/DOC-4, the quickstart instructions in both
top-level docs pointed at items that no longer exist.
Replaced each with /pad onboard guidance (the playbook is auto-seeded
into every new workspace by TASK-1500). CLAUDE.md's CLI reference
gets a one-line historical note explaining the pre-PLAN-1496 IDEA-1
pattern so readers reviewing older code/blame have context.
Parent: PLAN-1496.
* docs(skill): retire 'use pad to get IDEA-1' guidance in agent skill (Codex round 3)
P1 finding on PR #577: skills/pad/SKILL.md:175 still taught agents
that '"use pad to get IDEA-1"' should dispatch to 'pad item show IDEA-1'.
This branch deletes the seed items, so any agent following the
shipped skill in a fresh workspace would try to fetch a missing ref
instead of running /pad onboard.
Updated the routing entry to dispatch the legacy phrasing (kept as a
recognized intent so older docs/conversations still work) to the
/pad onboard playbook. Explicit "do NOT try to fetch IDEA-1
directly" to short-circuit the previously-trained behavior.
A broader skill cleanup — removing the standalone Onboarding
workflow section and adding the bootstrap nudge rendering — is
TASK-1505's scope. This PR's update is the minimal change needed to
unbreak the agent-facing routing.
Parent: PLAN-1496.
* docs(skill): add library-activation caveat to onboard routing entry (round 4)
P2 finding on PR #577: the routing entry said /pad onboard is
'always invokable because every workspace auto-seeds it.' True for
newly-created workspaces, but pre-existing workspaces (created before
PLAN-1496 lands) won't have it. Auto-upgrade is intentionally not
wired into SeedCollectionsFromTemplate for empty-template-name paths.
Mirrored the same activation-fallback caveat /pad plan and
/pad decompose carry: 'activate via library if the bootstrap's
playbooks array lacks invocation_slug=onboard, status=active.'
Parent: PLAN-1496.
|
||
|
xarmian
|
e59d3904c9 |
feat(server): refuse to mark item terminal while it has open children (IDEA-1494) (#571)
* feat(server): refuse to mark item terminal while it has open children (IDEA-1494)
Server-side guard inside handleUpdateItem that rejects a non-terminal →
terminal done-field transition when the item still has at least one
non-terminal child. Returns HTTP 409 with code=open_children plus a
structured details payload listing each blocking child's
{ref, title, status, collection_slug} so MCP-driven agents can
self-recover (ship the listed children, then retry) and the CLI can
render the same list verbatim.
Escape hatch: `--force` on `pad item update` / `pad item bulk-update`
and `force: true` on the MCP pad_item.action: update / bulk-update
inputs both forward into the same ItemUpdate.Force transport field
the handler consumes before any store mutation.
Trigger conditions are tight: the PATCH must change the done-field key
(resolved via TerminalValuesForDoneField against the parent's schema +
settings) AND the new value must be terminal AND the current value
must NOT already be terminal. Terminal → terminal and no-op terminal
transitions bypass the guard; only entering the terminal set is gated.
Per-child evaluation uses the child's own collection schema so
hierarchical workspaces with custom typed collections work without
extra plumbing.
Tests cover: rejection with one open child (with mutation-safety
assertion on the parent), no children, all-terminal children, --force
override, no-op terminal → terminal, terminal → terminal,
non-terminal → non-terminal, custom collection terminal_options
honored, and a parent task (not a plan) — IDEA-1494 optional extra #3.
MCP coverage asserts --force round-trips through both ExecDispatcher
and HTTPHandlerDispatcher and is omitted when force=false.
* fix(server): open-children guard round 2 — visibility, MCP pass-through, TOCTOU (IDEA-1494)
Three Codex round-1 issues, each fixed with the recommended shape:
P1 — visibility leak. The 409 response previously listed every blocking
child by ref/title/status, including children in collections the caller
couldn't see. The INVARIANT still evaluates against ALL children (it's a
data-integrity gate — a restricted user must not be able to close a
parent whose blockers they can't see), but the response payload now
filters to caller-visible children only. Hidden blockers surface as a
new `details.hidden_blocker_count` field plus an alternate human message
when every blocker is hidden ("blocked by N open children you don't
have access to"). Mirrors the visibility helpers (`visibleCollectionIDs`
+ `isItemVisibleToGuest`) used by the per-parent progress endpoint so
the two paths can't drift.
P2 — MCP code/details pass-through. The HTTP classifier was collapsing
409 into the generic `conflict` code and dropping `details`; the stdio
classifier was matching the human "cannot " message against the
validation regex and surfacing `validation_failed`. Both now surface
`open_children` with the structured details intact:
- HTTP: classifyHTTPStatusKind's 409 branch extracts the upstream
code; any non-empty, non-"conflict" code is passed through with
its `details` RawMessage. Generalizes beyond open_children — any
future structured 409 from a handler gets the same treatment.
- Stdio: the CLI writes a `pad-error: {json}\n` marker line on
stderr before the human-readable block (single source of truth for
both views), and classifyExecError detects the marker and lifts
the envelope verbatim. Marker is duplicated as a const between
internal/cli and internal/mcp to avoid pulling the cli package
into the classifier just for one string.
A new ErrOpenChildren error code constant + `Details json.RawMessage`
field on ErrorPayload back the wire shape.
P2 — TOCTOU. The guard previously ran in the handler before the store
transaction began; a concurrent child insert / child status flip could
slip between the children-list read and the parent's UPDATE. Fix:
- New `Store.UpdateItemWithPreCheck(id, input, precheck)` runs the
caller's invariant check inside the same tx, after acquiring the
workspace seq lock AND a new parent-children advisory lock keyed
on the parent ID. UpdateItem is now a thin wrapper passing nil.
- Every UpdateItem unconditionally acquires the parent-children
advisory lock for its own parent (if any) AND for itself-as-parent,
in a fixed order (parent first) so two updaters touching the same
parent always grab that key before the more-specific one — no
AB/BA deadlock.
- New `GetChildItemsTx` reads via the caller's tx; on Postgres the
advisory lock provides the snapshot guarantee (DISTINCT precludes
`FOR UPDATE`), on SQLite the global BEGIN IMMEDIATE write lock
serializes all writers.
- Handler now passes a precheck closure into UpdateItemWithPreCheck
at all three call sites (collab-snapshot path, applier-direct-write
path, main path). The guard's openChildrenGuardError sentinel is
unwrapped after each call so the 409 surfaces cleanly.
Tests:
- TestOpenChildrenGuard_VisibilitySanitizesPayload — restricted
editor sees parent + visible child, hidden child contributes to
hidden_blocker_count, no leak of ref/title/slug.
- TestOpenChildrenGuard_AllBlockersHiddenSurfaceGenericMessage —
open_children=[], hidden_blocker_count>0, message mentions "you
don't have access to."
- TestOpenChildrenGuard_TOCTOURace — 8 iterations of a child-flip
racing a parent-terminal update; asserts the forbidden outcome
(parent=completed AND child=open) never occurs.
- TestClassifyHTTPStatus_OpenChildrenPreservesCodeAndDetails +
inverse generic-409 test.
- TestClassifyExecError_OpenChildrenMarkerLiftsStructuredPayload +
no-marker-falls-through inverse.
* fix(server): open-children guard round 3 — 7 Codex findings closed (IDEA-1494)
P1 — visibility fail-closed. The handler was swallowing
visibleCollectionIDs errors, leaving visIDs==nil which the guard
treats as unrestricted, leaking hidden-child metadata. Now surfaces
the error as 500 BEFORE installing the precheck. Test:
TestOpenChildrenGuard_VisibilityLookupErrorFailsClosed closes the
store DB and asserts no 409+children leak.
P1 — link mutations acquire the advisory lock. SetParentLink,
ClearParentLink, CreateItemLink (when link_type ∈ childLinkTypes via
new isChildLinkType helper), DeleteItemLink (same condition), and
RestoreItem now take `pad:parent-children:<id>` in canonical sorted
order via new AcquireParentChildrenLocks helper. SetParentLink locks
BOTH old and new parents (re-parenting case). Race test
TestOpenChildrenGuard_LinkMutationRace asserts the forbidden
"link-committed-before-parent-flip AND parent flip succeeded" never
occurs by comparing link.created_at to parent.updated_at. Documented
semantics: status-wins + link-after-commit is legal under the
invariant "no open children EXIST AT THE MOMENT of transition" —
the post-condition variant ("no open child may EVER attach to a
terminal parent") is intentionally deferred.
P1 — MoveItem bypass closed. New MoveItemWithPreCheck mirrors
UpdateItemWithPreCheck — acquires workspace seq lock + parent-children
locks, re-reads in tx, runs caller precheck. handleMoveItem builds
the same guard closure using the DESTINATION schema for done-field
resolution (conservative — honors the schema the item moves INTO).
CLI gains `pad item move --force`, client gains MoveItemWithForce
that appends `?force=true` to the move endpoint. MCP catalog +
mapItemMove forward `force` through the route mapper. Tests:
TestOpenChildrenGuard_MoveItem_RejectsTerminalWithOpenChildren and
…_ForceOverrides.
P2 — pre-tx field-read TOCTOU. UpdateItemWithPreCheck and
MoveItemWithPreCheck now re-read the item via new getItemTx INSIDE
the tx (after locks) and pass that fresh snapshot to the precheck
closure; the precheck classifies the transition against the in-tx
view, not the handler-side pre-tx capture. Handler precheck closure
swaps `currentFieldsJS` from the in-tx snapshot. Test:
TestOpenChildrenGuard_PrecheckReadsInTxSnapshot stages a between-load
status mutation and asserts the precheck observes the post-mutation
fields.
P2 — bulk-update carries structured errors. cmd/pad/main.go's
updateFailure struct extended with Code + Details
(json.RawMessage). When client.UpdateItem returns *cli.APIError, the
row preserves the structured envelope. Human-text output also
renders the open-children list inline. Chose JSON-envelope route
over per-row stderr markers because bulk-update already produces a
structured envelope and ExecDispatcher returns stdout verbatim on
exit-0 — no classifier change needed. Test:
TestBulkUpdateStructuredFailuresCarryOpenChildrenDetails confirms
the wire shape the CLI lifts.
P3 — marker hardening. Marker bumped to versioned form
`pad-structured-error/v1:` (was `pad-error:`). cli.StructuredErrorMarker
+ mcp.structuredErrorMarker kept in lockstep with cross-references.
mcp.allowedStructuredErrorCodes whitelists known codes (currently
just open_children); unknown codes fall back to regex classification.
Marker must start the line after whitespace trim (embedded markers
ignored). Last-marker-wins to defeat pre-emption attacks. Tests:
TestClassifyExecError_{UnknownStructuredCode,OldMarkerVersion,
MarkerEmbeddedMidLine,LastMarker}.
P3 — soft-deleted collection schemas honored. New GetCollectionAnyState
mirrors childrenDoneFiltersForParent's inclusion rule; guard uses it
so a child still attached to a soft-deleted collection is evaluated
against ITS schema (custom terminal_options) instead of the default-
status fallback (which would mis-classify and false-block). Test:
TestOpenChildrenGuard_SoftDeletedCollectionSchemaHonored seeds a
custom collection, soft-deletes it while a child remains, and
asserts the terminal status is correctly recognized.
Comprehensive store-mutation audit results recorded in the PR
description (every method touching items.fields / items.collection_id
or item_links).
* fix(server): open-children guard round 4 — multi-parent locks, enum parity, PATCH atomicity (IDEA-1494)
Four Codex round-3 (blast-radius lens) findings, each fixed with the
recommended shape.
P1 — multi-parent lock set. acquireParentChildrenLocksForUpdate and
RestoreItem previously used `LIMIT 1` against item_links, so a child
with BOTH a `parent` link to P1 AND an `implements` link to P2 only
locked one of them. The other parent's open-children precheck could
race against the child's status flip and miss it.
Fix: new listParentChildLockKeys helper runs the same query
GetChildItems' inclusion rule uses (childLinkTypes), returns ALL
distinct parent target_ids, and feeds them into the canonical
multi-lock helper. Both UpdateItemWithPreCheck and RestoreItem now
acquire locks on {self} ∪ {all-parents-via-childLinkTypes}. Test:
TestOpenChildrenGuard_MultiParentChildLocksAll races a child status
flip against terminal-updates on both parents simultaneously.
P2 — lock-order asymmetry. The pre-fix codebase had multiple lock-
acquisition shapes: parent-then-self in acquireParentChildrenLocksForUpdate,
single-key in RestoreItem / CreateItemLink / DeleteItemLink /
ClearParentLink, and a sorted multi-key in SetParentLink. Two
concurrent callers using different ad-hoc orderings could AB/BA
deadlock.
Fix: removed the per-call-site AcquireParentChildrenLock helper
entirely. Every site now goes through AcquireParentChildrenLocks
(the canonical sorted multi-lock helper) — including ones that need
only one ID (the variadic call still sorts a one-element slice).
The helper's doc comment explicitly states the contract: "Ad-hoc
single-key acquisition outside this helper is FORBIDDEN — two call
sites taking distinct keys in different orders WILL deadlock."
Test: TestOpenChildrenGuard_NoDeadlockUnderReverseOrderConcurrency
runs reverse-order re-parents with a 5-second timeout; assertion
fails on hang.
P2 — HTTP/stdio code-surface parity. Round 2's HTTP pass-through
("any non-conflict upstream code") silently widened the ErrorCode
enum beyond stdio's allow-list (`open_children` only). Agents saw
different code surfaces depending on which dispatcher delivered
the response.
Fix: HTTP 409 branch in classifyHTTPStatusKind now consults the
same allowedStructuredErrorCodes whitelist stdio does. Codes
outside the set collapse to ErrConflict (no details), matching
what stdio does for an unknown-code structured marker. Doc on
allowedStructuredErrorCodes updated to make the dual-consumer
contract explicit: "Adding a new structured code is a TWO-WAY
change." Tests:
TestClassifyHTTPStatus_UnknownConflictCodeFallsBackToErrConflict
and TestStructuredErrorCodeParityAcrossTransports.
P3 — PATCH atomicity. A combined PATCH with `parent` + `status=terminal`
on an item with open children used to commit the parent-link change
INLINE (before the guard ran) and then reject the field write.
Caller saw 409 but the parent had already moved.
Fix: parent-link mutation is now DEFERRED — captured into outer-
scope vars during fields validation, executed AFTER
UpdateItemWithPreCheck succeeds. A guard rejection returns before
the link write block, so on rejection the link is untouched.
Documented choice: "reorder, don't tx-wrap" — wrapping SetParentLink
into the same store tx would require threading a *sql.Tx through
the SetParentLink API (which is also called from the
handler_item_links path); reordering is the smaller surgery and
gives the correct outcome on the failure direction. A residual
window remains in the OTHER direction (field write commits, link
write fails) — not made worse by the reorder, and called out
inline for a future tx-wrap pass.
Test: TestOpenChildrenGuard_PatchAtomicRejectionPreservesParentLink
sets up target → oldParent → openChild, sends PATCH {parent=newParent,
status=completed}, asserts 409 AND target.parent_link still points
at oldParent.
* fix(server): open-children guard — emit details.open_children as [] not null on hidden-only rejection (IDEA-1494)
|
||
|
xarmian
|
8094883869 |
feat(links): cross-workspace wiki-link resolution (IDEA-1492) (#568)
* feat(links): cross-workspace wiki-link resolution (IDEA-1492)
Adds [[workspace::REF]] and [[workspace::REF|Display]] wiki-link syntax
that resolves cross-workspace, plus a Go route GET
/{username}/{workspace}/ref/{REF} that 302-redirects to the canonical
item URL. 404 leaks no info about workspace existence — malformed refs
short-circuit before the workspace lookup, and access-denied returns 404
not 403.
Frontend (web/src/lib/utils/markdown.ts):
- renderMarkdown recognizes the workspace-prefix form and emits
cross-workspace anchors (class doc-link cross-workspace) pointing at
the resolver route. Same-workspace prefix is stripped and behaves
identically to the legacy [[REF]] form.
- wikiLinksToMarkdown emits the resolver URL for cross-workspace storage
and same-workspace items resolve through the in-memory list.
- markdownToWikiLinks rolls /<user?>/<ws>/ref/<REF> URLs back to
[[workspace::REF]] (or |Display when display text differs from the
ref). Legacy same-workspace round-trip is preserved.
Backend (internal/server/handlers_ref_resolver.go):
- Validates the ref shape before the DB hit (no oracle).
- Reuses resolveWorkspace + GetItemByRef for ACL + lookup.
- refResolverItemVisible mirrors requireItemVisible without depending
on RequireWorkspaceAccess middleware (this route is reachable
outside the workspace-scoped route group).
- Redirect target matches itemUrlId() so the post-redirect URL is
indistinguishable from a direct in-app navigation.
Tests: 302 success, 404 unknown-workspace, 404 unknown-ref, 404 on a
matrix of malformed refs (including url-encoded traversal). The
no-access matrix is partially covered — the existing test surface
doesn't compose a multi-user ACL fixture, so production-grade
"member of A probes B" is gated by the real auth middleware stack and
documented in TestRefResolver_NoAccess_DocumentsPreSetupBypass.
* fix(links): codex round-1 fixes for cross-workspace resolver (IDEA-1492)
P1.1 — Reserve "ref" as a collection slug. A collection slug of "ref"
would shadow every item URL under the resolver's /{u}/{ws}/ref/...
route. Added to reservedCollectionSlugs in internal/store/collections.go
so it auto-suffixes to "ref-collection", matching the existing
treatment of settings/activity/roles/etc.
P1.2 — Extract checkItemVisible as a context-free helper. The previous
refResolverItemVisible silently diverged from requireItemVisible by
ignoring direct collection grants and member_collection_access for
restricted members — a member with "specific" access on collection A
plus a direct grant on collection B would 404 through the resolver
even though they could see the item via the API. checkItemVisible now
replays the same rules requireItemVisible inlined; requireItemVisible
is now a thin wrapper, and the resolver derives its workspace role via
resolverWorkspaceRole and delegates to checkItemVisible. Drift between
the two paths is structurally impossible.
P2.1 — Cross-workspace round-trip preserves explicit display overrides.
The strip condition was `displayText === ref`, which would drop the
override on [[other::TASK-1|TASK-1]] — then re-rendering would emit
the default `other::TASK-1` and silently change visible link text.
Fixed: only strip when displayText matches the actual render default
`${ws}::${ref}`.
P2.2 — Two-segment route /{workspace}/ref/{REF}. TimelineCommentCard
and CommentThread call renderMarkdown without a username, so links in
timeline comments emit the two-segment href shape. Registered the
shorter route against the same handler; when the URL-path username is
absent the handler falls back to the workspace owner's username via a
new resolverOwnerUsername helper.
Sanity sweep:
- Dropped refItoa wrapper; use strconv.Itoa directly. The non-test
`itoa` collision was a test-only `itoa` in handlers_admin_users_test.go,
not a real symbol in non-test builds.
- Removed encodeURIComponent on workspace + ref in renderMarkdown.
parseCrossWorkspaceBody validates both against URL-safe regexes, and
wikiLinksToMarkdown doesn't encode — both functions now emit
identical bytes.
Tests:
- TestRefResolver_RejectsRefAsCollectionSlug — pins the reservation.
- TestRefResolver_TwoSegmentRouteResolves — both URL shapes resolve;
two-seg synthesizes the owner username.
- TestRefResolver_RestrictedMemberWithCollectionGrant — the
codex-flagged ACL case (restricted member + collection grant on a
different collection) now resolves to 302, not 404. Frontend test
for the round-trip override fix is documented as a gap (no vitest
infra in the repo).
* fix(links): codex round-2 fixes for cross-workspace resolver (IDEA-1492)
P1.1 — Tokenized roles bypass user-nil check. Pre-fix, checkItemVisible
rejected (nil user, "editor") tuples — exactly what RequireWorkspaceAccess
synthesizes for legacy workspace-scoped API tokens — false-404'ing every
requireItemVisible-gated handler hit by those tokens. Reordered the
checkItemVisible rules so any tokenized role (owner / editor) bypasses
the user-nil guard. checkItemVisible regression test (no HTTP layer)
pins the bypass.
P1.2 — System collections folded into item-grants branch. The pre-round-1
guestResourceFilterCore unioned ListSystemCollectionIDs into the
fullCollIDs set; the round-1 refactor dropped that union, so a
restricted member with conventions/playbooks (system collection) access
plus an unrelated item grant could LIST system items but 404 on
detail-fetch / ref-resolve. Restored the union inside checkItemVisible's
item-grants branch (non-guest path only — matches the original
guestResourceFilterCore semantics).
P1.3 — Empty owner-username 404s instead of emitting broken redirect.
When the workspace owner has no username on file (pre-setup ownerless
workspaces, legacy accounts), the synthesized redirect target became
`"/" + "" + "/" + slug + ...` → `//slug/...` — a protocol-relative URL
browsers interpret as a network-path reference. Now 404s via
refResolverNotFound rather than emitting the malformed Location header.
P1.4 — URL shape changed to /-/r/{workspace}/{ref} (Option B). Pre-fix,
the resolver lived at /{username}/{workspace}/ref/{ref}, which would
intercept item URLs in workspaces with pre-existing `ref`-slugged
collections (upgraded data; the round-1 reservation only blocks NEW
creates). Picked Option B over a migration because the feature is
unshipped, the new shape is more defensive (no future risk under any
collection slug), and the only cost is the frontend emit-shape change.
The leading `/-/r/` prefix can never collide with a user-namespace URL
because username + slug grammar both require letter-led. Frontend
renderMarkdown, wikiLinksToMarkdown, and markdownToWikiLinks all emit
and parse the new shape; the round-1 collection-slug reservation stays
as defense in depth.
Tests:
- TestCheckItemVisible_TokenizedRoleAllowsNilUser — P1.1 regression.
- TestRefResolver_RestrictedMemberWithSystemCollection — P1.2 regression.
- TestRefResolver_PreSetupBypass — P1.3 (ownerless workspace returns
404, not a broken `//slug/...` redirect).
- TestRefResolver_URLShapeNonOverlap — P1.4 (resolver doesn't intercept
`/{user}/{ws}/ref/{slug}` URLs).
- Existing TestRefResolver_* updated to the /-/r/ shape; the previous
two-segment fallback test is removed (the new URL shape has no
username component, so there's no two-segment vs three-segment
distinction to test).
* fix(links): scope round-2 bypass to nil user (Codex round-3 P1)
Round-2's checkItemVisible bypass for role in {"owner", "editor"} fired
unconditionally, including for real authenticated members. Result: a
member with workspace role "editor" and collection_access="specific"
short-circuited the per-collection filter — they could GET/PATCH/DELETE
items in collections their member_collection_access list excluded.
Scoped the bypass to the tokenized-nil-user case only:
if user == nil && (role == "owner" || role == "editor")
This is the exact set the bypass was supposed to address — fresh-install
mode (UserCount==0, role="owner") and legacy workspace-scoped API
tokens (tokenWorkspaceID matches, role="editor"). Both paths set
currentUser to nil; both are authorized by RequireWorkspaceAccess
before checkItemVisible runs.
Real authenticated members with the same roles now correctly fall
through to the existing per-collection visibility filter. Workspace
owners with default access still pass via the rule-4 "all access"
short-circuit (member.CollectionAccess == "all"); restricted editors
are now gated as intended.
Updated the rule-1 doc comment to make the scope-to-nil-user discipline
explicit — the prior wording conflated the tokenized and authenticated
paths, which is what led to the over-broad bypass.
Test: TestCheckItemVisible_AuthenticatedEditorWithRestrictedAccess
seeds a real editor with collection_access="specific" granting only
collection A, asserts visibility on a collection-B item returns false,
and adds a sanity assertion that the same editor sees collection-A
items. The existing TestCheckItemVisible_TokenizedRoleAllowsNilUser
still passes — it covers the (nil, "editor") tuple the corrected
bypass still allows.
Direct callers of checkItemVisible (grep): only requireItemVisible
(server.go) and resolverItemVisible (handlers_ref_resolver.go). Both
pass real (user, role) from request context, so the narrower scope
doesn't break any prior-green path.
* fix(links): allow digit-leading workspace slugs in xw wiki-links
Frontend WORKSPACE_SLUG_PATTERN was tighter than store.slugify (the
canonical rule): slugify keeps digit-leading inputs (e.g. "2026
Roadmap" → "2026-roadmap") but the frontend regex rejected them. Effect:
`[[2026-roadmap::TASK-1]]` fell through as a legacy title link, and
`/-/r/2026-roadmap/TASK-1` URLs didn't round-trip back to wiki syntax.
Two regex hunks, no behavior change beyond accepting the digit-led
case:
- WORKSPACE_SLUG_PATTERN: ^[a-z][a-z0-9-]*$ → ^[a-z0-9][a-z0-9-]*$
- markdownToWikiLinks reverse-regex workspace class: same widening
Stale doc-comment citing the old pattern updated to match.
Collection-slug grammar stays letter-led (the upstream rule differs;
only workspace slugs accept digit-led). Only functional consumer of
WORKSPACE_SLUG_PATTERN is parseCrossWorkspaceBody, which uses the
match boolean — no other downstream code relied on the leading-letter
constraint (Codex round-4).
|
||
|
xarmian
|
ec71903be7 |
feat(store): JSONB NOT NULL hardening on items/views + handler shape validation (IDEA-1486+1488) (#566)
* feat(store): NOT NULL hardening on items.fields/tags + views.config (IDEA-1486) Paired ship of IDEA-1486 (sibling-table JSONB NOT NULL hardening) and IDEA-1488 (handler-layer shape validation for ViewUpdate/CollectionUpdate). Generalizes the IDEA-1484 / collections.settings precedent (PR #562) to the remaining nullable JSON columns and closes the shape-validation gap that NOT NULL alone doesn't cover. Schema layer (IDEA-1486 floor): - migrations/056_items_jsonb_not_null.sql: rebuild items with fields TEXT NOT NULL DEFAULT '{}' and tags TEXT NOT NULL DEFAULT '[]', preserving all 7 indexes, recreating the 3 items_fts triggers, and rebuilding the FTS5 index. Foreign-keys-off / on bookends are lifted outside the IDEA-1485 atomic-tx wrapper. - migrations/057_views_config_not_null.sql: rebuild views with config TEXT NOT NULL DEFAULT '{}'. - pgmigrations/035 + 036: SET NOT NULL + SET DEFAULT on the three JSONB columns. Split per-table to mirror the SQLite per-table file granularity. Store layer (IDEA-1486 floor): - items.go UpdateItem and views.go UpdateView normalize "" -> "{}" / "[]" before writing. Same boundary pattern as CreateItem and the IDEA-1484 precedent at collections.go:248. - export.go ImportWorkspace coerces empty-string AND malformed JSON at import time on items.fields, items.tags, and collections.settings. Malformed input is coerce-and-log via slog.Warn (length only, never raw value) so legacy bundles don't fail-stop on one bad row. - remapFieldIDs early-returns "{}" on empty input so the second-pass UPDATE can't write "" verbatim. - Migrated the existing fmt.Printf at export.go:329 to slog.Warn for consistency. Handler layer (IDEA-1488 ceiling): - ViewCreate / ViewUpdate UnmarshalJSON via flexJSONToString with new ErrInvalidConfigType sentinel. - CollectionCreate / CollectionUpdate UnmarshalJSON with new ErrInvalidSettingsType sentinel. - handlers_views.go and handlers_collections.go surface both sentinels as 400 with the domain-level message (mirrors the BUG-1144 precedent at handlers_items.go:641). Tests: - internal/store/items_views_jsonb_test.go: store-coercion + import coercion + log-and-coerce-on-malformed + SQLite schema introspection (7 indexes + 3 FTS triggers + items_fts virtual table survival) + Postgres NOT NULL enforcement + migration re-apply idempotency + item_links round-trip after rebuild. - internal/server/handlers_views_collections_jsonb_test.go: PATCH/POST flexible-shape coverage for views.config and collections.settings, including domain-level 400 message assertions that the response does not leak Go unmarshal internals. Refs: IDEA-1486, IDEA-1488, IDEA-1484 (precedent), IDEA-1485 (substrate). * fix(store,models): codex R1 follow-ups for IDEA-1486 / IDEA-1488 Three concrete defects surfaced by codex R1 against the initial paired ship. All three close holes that defeated parts of the original contract. P1.1: migration 056 missed the playbook invocation_slug unique index. - migrations/056_items_jsonb_not_null.sql: recreate the partial UNIQUE index idx_items_invocation_slug_per_collection from migration 054 verbatim after the other 7 indexes. Without it, the application-layer pre-check in handlers_items.go:checkUniqueFields would be a TOCTOU race with no DB-level guard — the original index that 054 explicitly added as the actual uniqueness backstop would be silently dropped during the items rebuild. - items_views_jsonb_test.go: the schema-introspection test now asserts 8 indexes, not 7. Verified via `grep -rn "ON items(" migrations/` that no other items-touching indexes were missed. P1.2: flexJSONToString didn't validate inner content of JSON-encoded strings. Pre-fix, `{"config": "[]"}` / `{"settings": "not json"}` / `{"fields": "[]"}` / `{"tags": "{}"}` slipped past the shape validators because the `case '"'` branch unmarshalled the envelope and returned the inner string verbatim — bypassing the whole point of IDEA-1488. - models/item.go: after unmarshalling the JSON-encoded string, validate that the trimmed inner content's first byte matches expectedStart ('{' / '[') AND parses as JSON. Empty inner strings still pass through to the store-layer empty-string coercion (IDEA-1486 floor), so legacy "" → default normalization is preserved. - The pre-existing ItemUpdate fields/tags path inherits the same tightening because it routes through this helper — covered by new test file handlers_items_jsonb_inner_shape_test.go. - Parallel handler tests for views.config and collections.settings added to handlers_views_collections_jsonb_test.go. P2: coerceJSONForImport accepted JSON null as well-formed. - store/export.go: json.Unmarshal("null", &m) returns err=nil with m staying nil; the prior code returned the raw "null" string verbatim, which lands as JSONB null on Postgres (satisfies NOT NULL since SQL NULL ≠ JSONB null) or text "null" on SQLite. The non-nil check on the unmarshalled value routes JSON null to the existing log-and-coerce path with the rest of the malformed shapes. - items_views_jsonb_test.go: extended import test with an item carrying fields=null / tags=null; expects both coerced to "{}" / "[]" and the structured slog.Warn emitted. Verified: make test (SQLite) and the full ./... suite against the existing port-5445 Postgres container both pass cleanly. Refs: IDEA-1486, IDEA-1488, codex R1 review. * fix(store,server): codex R2 follow-ups for IDEA-1486 / IDEA-1488 Two defects surfaced by codex R2. P1 is a real ship-breaker; P2 closes a parity gap that R1 missed. P1: migration backfill normalized only SQL NULL, not malformed/wrong- shape JSON. The four new migrations originally wrote `WHERE x IS NULL`. Rows with fields = '' / 'null' / '[]' / 'not json' all survived the filter, then violated the post-migration NOT NULL+shape contract. Concrete ship- breaker on SQLite: 056 recreates the partial UNIQUE index on json_extract(fields, '$.invocation_slug') from migration 054, and json_extract errors on rows whose fields fails json_valid — a single bad row breaks CREATE INDEX mid-migration. Toggle-verified: with the NULL-only WHERE, the new SQLite test fails at exactly that CREATE INDEX with "SQL logic error: malformed JSON (1)". Widened the backfill clauses: - migrations/056: UPDATE items WHERE fields IS NULL OR json_valid(fields)=0 OR json_type(fields)!='object' (same trio for tags with 'array'). - migrations/057: same trio for views.config. - pgmigrations/035: WHERE fields IS NULL OR jsonb_typeof(fields)!='object'. JSONB rejects invalid JSON on write so the json_valid leg isn't needed on Postgres; only the shape check matters. - pgmigrations/036: same shape check on views.config. Regression tests in internal/store/items_views_jsonb_test.go: - TestItemsViewsJSONB_SQLiteBackfillRepairsMalformedShapes: applies migrations through 053 (skipping 054 which would itself error on malformed rows), seeds every observable shape pathology — SQL NULL, empty string, JSON null literal, wrong-shape JSON, non-JSON garbage — then applies 055/056/057. Asserts every malformed row is repaired AND the partial UNIQUE index actually fires on duplicate invocation_slug post-rebuild (proving the CREATE INDEX path executed end-to-end). - TestItemsViewsJSONB_PostgresBackfillRepairsMalformedShapes: parallel Postgres coverage; seeds JSONB null / array / primitive via direct ::jsonb cast and asserts the widened WHERE clause repairs each. P2: handleCreateItem didn't unwrap ErrInvalidFieldsType/ErrInvalidTagsType. R1's flexJSONToString tightening propagated the sentinels through every UnmarshalJSON path, but handleCreateItem (POST /items) still returned 'invalid JSON: <wrapped>' from decodeJSON. PATCH and the view/collection POST/PATCH handlers already unwrapped — POST was the outlier. - internal/server/handlers_items.go: mirror the PATCH-side errors.Is handling at the POST path. Brief, three-line diff. - handlers_items_jsonb_inner_shape_test.go: new TestCreateItem_ JSONEncodedStringInnerShapeValidated covers POST with fields=`[]`, fields=42, tags=`{}`, tags={"x":1}, plus a valid positive control. Asserts no "invalid JSON:" wrapper and presence of the sentinel message verbatim. Backfill-pattern audit (codex R2's grep prompt): only 055 / pg-034 (collections.settings, already shipped) exhibits the same NULL-only WHERE gap. Per the brief: NOT touched — retroactive repair belongs to a separate IDEA. Other NULL-only backfills (043/pg-023's oauth_providers, 044/pg-024's expires_at) handle their respective shapes correctly or aren't JSON columns. Verified: make test (SQLite) clean. Full ./... suite against the existing port-5445 Postgres container clean (one unrelated flake in internal/collab passed on rerun). Refs: IDEA-1486, IDEA-1488, codex R2 review. |
||
|
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
|
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
|
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
|
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). |