mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-23 02:53:31 +00:00
6433cc51ea1dd68b81f7b287c30385ad4400528f
386 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
6433cc51ea |
feat(mcp): pad_library catalog tool + ToolSurfaceVersion 0.5 (TASK-1563) (#615)
* feat(mcp): pad_library catalog tool + ToolSurfaceVersion 0.5 (TASK-1563)
MCP catalog wiring for PLAN-1560 (`pad_library` MCP tool + matching CLI
surface). Closes IDEA-1514 — pure-MCP agents (notably the /pad onboard
playbook from PLAN-1496) can now browse and activate library entries
without shelling out.
## New tool
`pad_library` joins the v0.5 catalog as the ninth resource × action tool.
Three actions, all passThrough to the `pad library` CLI:
- `list` — Browse conventions + playbooks. Defaults to summary mode
for playbooks (compact bodies via the ?summary=true
endpoint flag); conventions always carry full content.
Optional type / category / full inputs.
- `get` — Full body of one entry by exact title. Conventions-first
precedence mirrors `activate`.
- `activate` — Create a workspace item from a library entry by title.
`Workspace: true` on the tool — list/get ignore it; activate validates
and uses it. The schema-level declaration gives activate automatic
pad_set_workspace session-default resolution (same precedent as
pad_meta's mixed-workspace actions).
## Dispatcher extensions
- `dispatchLibraryList` forwards `category` to BOTH endpoints and
passes `summary=true` to the playbook endpoint by default (unless
input.full=true). MCP-default summary mode keeps agent context
budgets tight; CLI default already aligned in TASK-1562.
- `library get` added to the routeTable as a clean GET to
/api/v1/library/entry with `title` mapped to the query string.
Cleaner than another explicit dispatcher case — matches
playbook list / playbook show shape.
## Version bump
ToolSurfaceVersion bumped from 0.4 → 0.5. Pure addition; no existing
tool/action/param/bootstrap shapes changed. Backwards-compatible for
any v0.4 consumer that doesn't enumerate the new tool. Documented in
version.go with the same comment-block structure as prior bumps.
## Test coverage
- catalog_readonly_test.go — pad_library added to the want{} map; three
library action → cmdPath entries in expected{}; library list / get /
activate added to liveCmdhelpDoc stubs.
- dispatch_http_project_test.go — 4-case table test (defaults, category,
full=true, category+full) pins category/summary query-param forwarding;
library get routing test confirms the routeTable entry resolves.
## Live MCP verification
- `initialize` handshake advertises padToolSurface.version=0.5.
- `pad_meta version` returns tool_surface_version=0.5.
- `pad_library list type=playbooks category=agent-workflows` returns
4 playbooks in summary mode (content stripped, summary populated,
invocation_slug + arguments present).
- `pad_library get title='Ship tasks'` returns
{type: playbook, playbook: {…, content (9512 chars), invocation_slug: ship}}.
Parent: PLAN-1560. Unblocks TASK-1564 (cleanups).
* fix(onboard): update playbook body to use pad_library MCP tool per Codex review (round 1)
Codex P2 on PR #615: the /pad onboard playbook body in
internal/collections/playbook_library_onboard.go still told MCP-only
agents that the library catalog was "not yet exposed as an MCP tool"
and to work from memory — directly contradicting the pad_library tool
this PR just landed and breaking the main advertised consumer of the
new surface.
Updated step B3 (conventions) to mention both surfaces side-by-side
(`pad library list --type conventions` / `pad_library` with
`action: list, type: conventions`), and rewrote step B5 (playbooks)
the same way so the activate path doesn't drift either.
Pre-PLAN-1560 IDEA-1514 reference removed from the body — the idea
is now closed.
No test pins the playbook body content; `make check` passes; the
playbook seed still validates against the playbooks collection schema
since trigger/scope/invocation_slug/arguments are unchanged.
Closes the onboard-side scope of TASK-1564 (stale dispatch_http_slice4
hint + CHANGELOG still pending there).
|
||
|
xarmian
|
9a47c36ea6 |
chore(store): rephrase comment to unblock gofmt (BUG-1565) (#614)
Go 1.26's gofmt rewrites paired ASCII apostrophes (''), used here as a
literal SQL empty string, to a typographic right-curly double quote
(U+201D). The rewrite is applied even inside markdown backticks, so
escaping the SQL fragment in code-span syntax doesn't help.
Rephrase the comment to describe the COALESCE/LOWER pattern in words
instead of embedding the SQL literal, preserving the original meaning
while sidestepping the heuristic. The behavior of
adminOpenItemsCountClause is unchanged — comment-only edit.
Unblocks `make check` for local pre-commit and CI gates. Surfaced
during PLAN-1560's TASK-1561 ship loop.
|
||
|
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
|
db87b47754 |
fix(cli): pin server URL in .pad.toml for remote workspaces (BUG-1535) (#595)
* fix(cli): pin server URL in .pad.toml for remote workspaces (BUG-1535) Two fixes: 1. Replace stale api.getpad.dev references with app.getpad.dev in the --url flag help, NewClientFromURL doc, and Config.URL doc. Also fix internal/mcp/dispatch_http.go's comment to use the canonical mcp.getpad.dev/mcp URL. 2. Persist the server URL into .pad.toml when linking a directory to a non-local workspace. WriteWorkspaceLink now takes a serverURL arg; pad init / workspace link / workspace switch pass cfg.BaseURL() when Mode != local. getConfig() reads .pad.toml's URL as an override above ~/.pad/config.toml and below the --url flag, so commands like `pad collection list` from a remote-linked directory hit the right server without --url on every call. Passing --url explicitly also promotes local → remote so the directory pin is written even when the existing global config has mode=local. * fix(cli): scope .pad.toml URL override to client paths per Codex review (round 1) Round 1 review flagged that applying the .pad.toml URL override inside getConfig() contaminates server/admin commands: pad server start would advertise the wrong PublicLinkBaseURL, and pad auth setup would refuse to run locally because Mode flipped to remote. Extract the override into applyPadTomlOverride() and call it only from client-API entry points — getConfiguredConfig() and the pad init client phase. Server/admin commands (pad server start/stop, pad auth setup, pad auth configure) keep using raw getConfig() and are unaffected. Also skip the override when --url was explicitly passed (LoadedFromFlags), so the flag retains unambiguous priority. * fix(cli): preserve .pad.toml URL on workspace link/switch per Codex review (round 2) Round 2 review noted workspace link / workspace switch reached the server via getClient() (override applied) but then wrote the new .pad.toml URL using a raw getConfig() — which would drop or miswrite the url field when relinking inside a remote-pinned directory whose global config is local. Reuse the cfg returned by getClient() for padTomlURLFor so the write matches the API client. |
||
|
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
|
507793e565 |
feat(playbooks): author canonical /pad onboard library playbook (TASK-1499) (#576)
* feat(playbooks): author canonical /pad onboard library playbook (TASK-1499)
The fourth invokable library playbook (alongside ship/plan/decompose).
This is the workspace bootstrap interview the agent runs to turn a
freshly-created workspace into one whose collections, conventions,
playbooks, and roles actually match the user's project.
Files:
- internal/collections/playbook_library_onboard.go (new):
- onboardPlaybookBody — surface-agnostic instruction set teaching
the agent to ADAPT seeded artifacts, not curate from the library.
Mode-aware: build (blank workspace), audit (templated workspace),
revisit (already-onboarded), defaults (escape hatch). The body
explicitly tells the agent to use pad_item/pad_collection/pad_role
MCP actions OR pad CLI — never assumes a shell. Lean on the
TASK-1510/1511/1512 mutation primitives shipped earlier in
PLAN-1496.
- onboardPlaybookArguments — mode (enum), defaults (flag),
skip-codebase (flag). Mirrors the body's ## Arguments section
for the strict CLI parser.
- OnboardPlaybook() — LibraryPlaybook constructor.
- internal/collections/playbook_library.go: register OnboardPlaybook()
in the agent-workflows category alongside ship/plan/decompose.
- internal/collections/playbook_library_test.go:
- TestPlaybookLibrary_InvokableEntriesPresent now expects 4
invokable entries (was 3) and includes onboard in wantSlugs.
- New TestOnboardPlaybook_Contract locks the design contract:
invocation_slug=onboard, trigger=manual (compatible with the
blank template's minimal vocab), mode/defaults/skip-codebase
argument shape, and presence of the "ADAPT, DON'T CURATE"
posture in the body.
Design notes captured at top of playbook_library_onboard.go:
1. Adapt, don't curate — library entries are starting points,
rewrite using the project's actual commands.
2. Surface-agnostic — describe intent, not specific CLI commands;
pure MCP users must follow the same flow.
3. Mode-aware — blank/audit/revisit/defaults paths.
4. Confirmation before mutation.
5. Self-removing nudge — the playbook produces user-created items
which clear the bootstrap onboarding flag (TASK-1504, separate).
Parent: PLAN-1496. Unblocked by TASK-1497 + TASK-1510/1511/1512.
* fix: auto-seed onboard playbook + correct CLI form in body (Codex round 1)
Addresses two PR #576 findings:
1. P1 — folding TASK-1500 into this PR: without auto-seed, the
library entry alone makes /pad onboard manually-activatable but
not invokable on day one. Codex correctly flagged that the PR as
originally drafted shipped a half-feature.
Wiring (PLAN-1496 / TASK-1500):
- OnboardSeedPlaybook() in playbook_library_onboard.go returns
the playbook as a SeedPlaybook with status=active,
trigger=manual, scope=all, invocation_slug=onboard,
arguments=onboardPlaybookArguments. Body + args are shared
with the library entry (same pattern ShipPlaybook uses for
ship) so they cannot drift.
- SeedCollectionsFromTemplate appends OnboardSeedPlaybook to
EVERY workspace created with a non-empty templateName —
blank, startup, scrum, product, hiring, interviewing, demo.
The empty-templateName path is preserved as the explicit
backward-compat escape hatch (tests + direct API callers
that want a bare workspace with zero items). cmd/pad/init.go
always supplies a non-empty template (interactive picker or
defaultTemplateName), so real user-facing workspace creation
always lands in the seeded branch.
Tests:
- TestSeedFromTemplateAlwaysIncludesOnboardPlaybook walks all
six real templates and confirms the onboard playbook is
seeded into each.
- TestSeedWithEmptyTemplateNameSkipsOnboard locks the
escape-hatch invariant.
- TestSeedFromBlankTemplate updated: blank workspace now ships
exactly one item (the onboard playbook) instead of zero,
because that's TASK-1500's whole point.
2. P2 — the body referenced 'pad library list-conventions', which
doesn't exist. Corrected to 'pad library list --type conventions'
(the actual CLI form), with a parenthetical pointing MCP users
at pad_meta.action: bootstrap for the same data.
This PR now covers both TASK-1499 (author playbook) and TASK-1500
(auto-seed) — combining them because Codex's P1 made it clear they
ship together or not at all.
* docs: correct MCP library-browse fallback in onboard body (Codex round 2)
P2 finding on PR #576: the body told MCP-only users to read the
convention library via 'pad_meta.action: bootstrap'. Bootstrap
returns workspace STATE (collections, conventions, playbooks
actually present in the workspace), not the global library
catalog. So MCP users following that instruction would see only
what's already activated, not what they could activate.
The honest answer is that there is no MCP library-browse surface
today. Updated the body to say so explicitly: if the agent has a
shell, use 'pad library list'; if not, work from domain knowledge
and have the user paste any library bodies they want as starting
text.
Captured the underlying gap as IDEA-1514 (Expose library catalog
via MCP) and linked from the playbook body. Three options outlined
there: new pad_library tool, pad_meta.action: library, or embed in
bootstrap.
Parent: PLAN-1496.
|
||
|
xarmian
|
cc0b1c0bf3 |
feat(templates): finalize 'blank' template with minimal-vocab seeds for /pad onboard (TASK-1498) (#575)
* feat(templates): finalize 'blank' template for /pad onboard flow (TASK-1498)
A blank template entry was already present in templates.go (drafted
for IDEA-1479) but its seeded trigger/scope vocabularies leaked the
software domain — on-commit, on-pr-create, on-implement, etc., baked
into a template whose whole point is being domain-agnostic. The
/pad onboard playbook (PLAN-1496 / TASK-1499) needs a true blank
starting point so the interview can broaden vocabulary to match the
project's actual domain, whatever it is.
This commit:
- Replaces the software-flavored seed with minimal vocab: trigger=
always for conventions, trigger=manual for playbooks, scope=all
on both. The constants live in templates_blank.go so future tweaks
to the seed surface have a focused diff. The agent broadens via
pad collection update (TASK-1510) during onboarding.
- Updates the template's description and icon to point at the
onboard flow ("Empty workspace — run /pad onboard to build it out",
sparkles instead of memo).
- Adds an in-place comment explaining the design choice so the next
reader doesn't re-leak software triggers into the seed.
- New test: TestBlankTemplateUsesMinimalVocabularies locks the
minimal-seed posture; any regression that adds domain-flavored
triggers fails this test and triggers a fresh design conversation.
Pre-existing IDEA-1479 tests (Shape, ExcludesSoftwareCollections,
AppearsInPicker) still pass — the contract they describe is
preserved (2 system collections only, no user-facing leaks, Custom
group placement).
Parent: PLAN-1496.
* fix(test): blank-vocab assertions use literal slices, not the vars they came from (round 1)
P3 finding on PR #575: TestBlankTemplateUsesMinimalVocabularies
compared template output to BlankConventionTriggers /
BlankPlaybookTriggers — the same vars used to build the template.
Widening either var would silently widen the "minimal" definition
and the test would still pass, defeating the drift-guard intent.
Switched to literal expected slices. Now any change to the var that
adds a domain trigger fails the test loudly.
|
||
|
xarmian
|
8c9974f6fb |
feat(cli,mcp): expose 'role update' via CLI and MCP catalog (TASK-1512) (#574)
* feat(cli,mcp): expose 'role update' via CLI and MCP catalog (TASK-1512) Third of three TASK-1497 capability-spike follow-ups (after #572 and #573). The handlers_agent_roles.go::handleUpdateAgentRole PATCH handler and the internal/cli/client.go::UpdateAgentRole HTTP client method already existed. Only the agent-facing surfaces were missing. - cmd/pad: new 'pad role update <slug-or-uuid>' Cobra subcommand with --name / --slug / --description / --icon / --tools / --sort-order flags. Uses cmd.Flags().Changed for omit-if-unset. Positional arg = lookup ref; --slug = new slug value (rename). Empty-string clears for description and icon (the store treats *string("") as "clear", matching collection update semantics). - internal/mcp/catalog_role: new 'update' action + supporting params (new_slug, sort_order). The catalog disambiguates lookup-slug (in path) from rename-target (in body) with the new_slug input, avoiding the conflated-semantics footgun. - internal/mcp/dispatch_http_routes: new mapRoleUpdate mapper. Path uses input.slug for the lookup; body's "slug" key is sourced from input.new_slug. String fields use key-presence semantics so empty-string clears round-trip to the store. - Tests cover canonical body (with AgentRoleUpdate round-trip), new_slug-to-body-slug mapping, empty-string clearing, and required-arg validation. - README.md + internal/mcp/instructions.md pad_role action lists updated to include "update". Pairs with TASK-1510 + TASK-1511 to complete the workspace-mutation trio the /pad onboard playbook (TASK-1499) needs to adapt seeded roles, collections, and schemas to each project's actual shape. Parent: PLAN-1496. * fix(cli,mcp): rename role-update flag --slug → --new-slug (Codex round 1) P1 finding on PR #574: pad_role.update via local stdio MCP was silently broken. BuildCLIArgs translates MCP property "slug" to the CLI's positional <slug> AND to the --slug flag (same key reused), so: pad_role.update slug=<uuid> → pad role update <uuid> --slug <uuid> → tries to rename the role's slug to the literal UUID. BAD. pad_role.update slug=implementer new_slug=engineer → pad role update implementer --slug implementer → new_slug ignored entirely, no rename. The HTTP dispatcher had the disambiguation right (mapRoleUpdate already mapped MCP new_slug → body slug). The CLI flag name was the problem. Renamed --slug to --new-slug. Now MCP "slug" maps to the positional only (lookup), and MCP "new_slug" maps to --new-slug (rename target). Both transports symmetric. Updated example in --help, the liveCmdhelpDoc fake, and the change-detect block. Parent: PLAN-1496, Codex round 1 on PR #574 / TASK-1512. |
||
|
xarmian
|
f76520f6e7 |
feat(cli,mcp): expose 'collection delete' via CLI and MCP catalog (TASK-1511) (#573)
* feat(cli,mcp): expose 'collection delete' via CLI and MCP catalog (TASK-1511)
Mirrors TASK-1510 (collection update). The HTTP handler at
handlers_collections.go::handleDeleteCollection already supported
DELETE on a collection (owner-only, soft-deletes the collection and
every item in it). Wires both agent-facing surfaces:
- internal/cli/client.go: new DeleteCollection client method.
- cmd/pad: new 'pad collection delete <slug>' Cobra subcommand
(no --force; the help text is the confirmation contract).
- internal/mcp/catalog_collection: 'delete' action passes through
to the CLI; tool description updated.
- internal/mcp/dispatch_http_routes: simple routeSpec entry for
DELETE /api/v1/workspaces/{workspace}/collections/{slug}. No
custom mapper needed — no body, no field coercion.
Pairs with TASK-1510 as the second adaptation primitive for the
/pad onboard playbook (TASK-1499): when the onboard interview
discovers a seeded collection that doesn't fit the project, the
agent now has a way to remove it before creating the right one.
Tests:
- TestRouteTable_CollectionDelete (route substitutes correctly)
- catalog_readonly bijection + liveCmdhelpDoc fake updated.
Parent: PLAN-1496.
* docs: correct collection delete contract per Codex review (round 1)
Two findings on PR #573 — both documentation, no code behavior change:
1. CLI Long help / Short blurb / MCP description claimed delete
"removes seeded collections" and the onboard use case targets
template-seeded collections. But store.DeleteCollection refuses
any collection where is_default=true, and every template seed is
is_default=true. The advertised use case wouldn't actually work.
Updated docs to clarify: delete is for USER-CREATED collections;
template seeds must be adapted via 'pad collection update'.
2. Both CLI help and MCP description claimed "AND every item in it"
gets archived. The store delete path only sets collections.deleted_at
and never touches items. The web UI hides them via the join, but
raw API queries still surface them. Updated docs to be honest:
items are NOT cascaded.
Captured the underlying behavior limitation as a follow-up: IDEA-1513
("Lift is_default restriction on collection delete or add a
cascade-items option") — surfaces options 1-4 for lifting the guard
plus the items-orphan issue.
Parent: PLAN-1496, addressing Codex round 1 on PR #573 / TASK-1511.
* docs: tighten collection delete contract per Codex review (round 2)
Three P3 documentation-drift findings:
1. internal/cli/client.go::DeleteCollection Go doc still said "and
all items in it" — missed it in round 1. Updated to describe the
actual behavior (collections.deleted_at only; items orphaned with
soft-deleted collection_id; is_default rejected).
2. CLI Long help and MCP description claimed restore is available
"via the API," but there is no restore endpoint and no
RestoreCollection client method. Recovery is database-backup only.
Both docs updated.
3. catalog_collection.go:33 slug ParamDef only mentioned action=update;
action=delete needs it too. And the headline description still
said "list, create, and update" — three actions when there are
now four. Both fixed.
Parent: PLAN-1496, addressing Codex round 2 on PR #573 / TASK-1511.
* docs: update pad_collection action lists in instructions.md + README (round 3)
Codex round 3 finding: two top-level reference docs still advertised
pad_collection as list/create only. internal/mcp/instructions.md is
embedded into the MCP initialize() handshake instructions — stale
guidance there means MCP clients miss update/delete entirely. README's
catalog table had the same drift.
Parent: PLAN-1496, Codex round 3 on PR #573 / TASK-1511.
|
||
|
xarmian
|
f5579300fb |
feat(cli,mcp): expose 'collection update' via CLI and MCP catalog (TASK-1510) (#572)
* feat(cli,mcp): expose 'collection update' via CLI and MCP catalog (TASK-1510) The HTTP handler at handlers_collections.go::handleUpdateCollection already supported PATCHing a collection's name, icon, description, prefix, schema, settings, and sort_order (plus field-value migrations). The CLI and MCP surfaces never exposed it, so agents couldn't rename collections, swap icons, or reshape schemas — a hard blocker for the adaptive /pad onboard playbook (TASK-1499) which needs to rewrite seeded collections to match each project's actual vocabulary. This wires both agent-facing surfaces to the existing handler: - cmd/pad: new 'pad collection update <slug>' Cobra subcommand with --name / --icon / --description / --prefix / --schema / --fields / --sort-order flags. Only flags explicitly set are sent (uses cmd.Flags().Changed); --schema and --fields reuse the existing collectionSchemaJSONFromFlags helper so DSL parity stays. - internal/mcp/catalog_collection: add 'update' action plus the slug, prefix, and sort_order params on padCollectionTool. - internal/mcp/dispatch_http_routes: new mapCollectionUpdate handles the schema-object-vs-string coercion. The catalog declares schema as a JSON object for MCP ergonomics, but models.CollectionUpdate.Schema is *string — and its UnmarshalJSON only flexes settings, not schema. The mapper re-marshals object input to its JSON-string form before sending, symmetric to what the CLI does via collectionSchemaJSONFromFlags. Tests cover canonical body, schema-object-to-string coercion (round-trip through CollectionUpdate.UnmarshalJSON), schema-string pass-through, empty-field omission, and required-arg validation. catalog_readonly_test bijection + liveCmdhelpDoc fake updated. Parent: PLAN-1496. * fix(mcp): collection update — clear-on-empty + fields DSL parity per Codex review (round 1) Addresses two P2 findings on PR #572: 1. The catalog advertises `icon=""` / `description=""` / `prefix=""` as clear-the-field, and the CLI flag help says the same, but the HTTP mapper filtered empty strings via `v != ""` — leaving MCP HTTP callers unable to clear fields the CLI can. Switched to key-presence semantics for the four string fields so explicit empty strings round-trip to the store (which honors *string("") as "clear"). 2. The catalog advertises `fields OR schema` as mutually exclusive (mirroring `pad collection create`), but the mapper only consumed `schema`. An MCP HTTP request with `fields=...` produced a `{}` PATCH body silently. Extracted the DSL parser to a shared package (internal/collections/dsl.go::ParseFieldsDSL + FieldsDSLToSchemaJSON) so the CLI and the mapper share one parser; mapper now resolves fields-or-schema with the same mutual-exclusion guard the CLI has. Tests added in dispatch_http_routes_extras_test.go: - TestMapCollectionUpdate_EmptyStringClearsField - TestMapCollectionUpdate_AcceptsFieldsDSL (round-trips through models.CollectionSchema to confirm the parsed shape) - TestMapCollectionUpdate_RejectsFieldsAndSchemaTogether cmd/pad/main.go's parseFieldsDSL becomes a one-line alias for collections.ParseFieldsDSL so the CLI's behavior stays identical. Parent: PLAN-1496, fixing PR #572 / TASK-1510. * fix(mcp): collection update — use encodeSchemaForBody + normalize empty schema (round 2) Addresses two more findings from Codex round 2 on PR #572: 1. P2: mapCollectionUpdate bypassed encodeSchemaForBody, so structured schemas didn't get label backfill and string schemas weren't validated before PATCH — diverged from collection create + CLI. Now reuses encodeSchemaForBody (the same encoder collection create uses at dispatch_http_routes.go:418), getting label-backfill via the Title-Case-of-key heuristic and shape validation for free. 2. P3: schema=null or schema="" plus a real fields=... update tripped the mutual-exclusion check. Now normalizes empty inputs as absent BEFORE checking exclusivity, matching the relaxed handling collection create has for optional empty params. Tests: - Renamed TestMapCollectionUpdate_PassesSchemaStringVerbatim to TestMapCollectionUpdate_AcceptsSchemaString — the new property is round-trip parity + label backfill, not verbatim pass-through. - New TestMapCollectionUpdate_EmptySchemaDoesNotBlockFields covers both nil and empty-string schema combined with a real fields value. Parent: PLAN-1496, addressing Codex round 2 on PR #572 / TASK-1510. |
||
|
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
|
9ebdfb503e |
Revert "feat(cli): pad session shape — Claude Code context-window telemetry (IDEA-1491) (#569)" (#570)
This reverts commit
|
||
|
xarmian
|
351f83af3f |
feat(cli): pad session shape — Claude Code context-window telemetry (IDEA-1491) (#569)
* feat(cli): pad session shape — Claude Code context-window telemetry (IDEA-1491)
New `pad session shape [--session <id|path>] [--format json|table|markdown]`
command that reads the active Claude Code session JSONL and reports tokens,
context_pct (vs hardcoded per-agent-version budget), message counts, and
elapsed time. Default format is JSON because agents are the primary caller.
- internal/cli/claudecode.go: project-slug derivation, JSONL streaming
parser, env/cwd/autodetect cascade resolver, per-version budget table
(seeded with 2.1.* → 1M tokens per the IDEA-1491 recon update).
- cmd/pad/session.go: top-level `session` group + `shape` subcommand,
three output formats, registered via cmd/pad/main.go's rootCmd.
- Tests cover slug derivation (live ~/.claude/projects/ verified cases),
JSONL parse (normal / no-usage / sidechain fixtures), budget lookup,
context-class bucketing, and the resolver cascade in t.TempDir().
Sidechain/sub-agent JSONL summing and the IDEA-body Pad-invocation-count
fallback are intentionally deferred to v2 (TODOs in session.go).
* fix(cli): session shape — TotalPrompt as context numerator + count parallel tool_use (IDEA-1491)
Codex R1 review findings:
P1 — context_pct numerator was CacheRead, which is a steady-state proxy
that under-counts at turn boundaries when fresh content sits in
cache_creation/input before being folded into the cached prefix. The
correct denominator-of-budget is the full prompt footprint sent this
turn: cache_read + cache_creation + input = TotalPrompt. Markdown
renderer's context-tokens line follows suit; the explicit per-component
breakdown rows keep CacheRead so the components remain visible.
P2 — ToolInvocations was counting assistant-turns-with-any-tool-use, not
tool invocations. A single assistant turn can emit multiple parallel
tool_use blocks in message.content[]; the field name promises a count
of invocations. Drop the early break.
Test fixture normal.jsonl gains a 3-parallel-tool-use final turn; assert
ToolInvocations==4 (up from 2) to pin the new behavior.
* fix(cli): session shape — path-traversal, oversized-line, content-variant, explicit-flag errors (IDEA-1491)
Codex R2 review findings:
P1 — session-ID inputs (--session flag-id branch AND
$CLAUDE_CODE_SESSION_ID env var) are now validated before they become
filename fragments under ~/.claude/projects/<slug>/. Reject path
separators, '..' segments, and anything that doesn't match a UUID-ish
shape (hex+dashes, 8+ chars). New ErrInvalidSessionID wraps a
descriptive message. Without this, `--session ../../../../../tmp/foo`
or a poisoned env var could escape the projects dir on the candidate
os.Stat. End-to-end smoke confirms `pad session shape --session
'../../../etc/passwd'` now errors with "invalid session id: ...
contains a path separator" and exits non-zero.
P2a — Switch the JSONL line reader from bufio.Scanner (8 MiB cap, hard
fail on overflow with "token too long") to bufio.Reader.ReadBytes('\n')
(no cap). encoding/json has no size limit either, so oversized records
— e.g. inline file attachments — parse cleanly. Both ParseSessionJSONL
and tailLineCWD updated for parity.
P2b — jsonlLine.Message.Content was []json.RawMessage at the outer
decode, which made a schema variant where content is a string or
object fail the WHOLE line's decode, losing type/timestamp/version/
usage data. Now Content is a raw json.RawMessage; the tool-use scan
re-decodes it as []json.RawMessage on a best-effort basis and skips
tool-counting when that fails, while preserving the rest of the line.
P3 — `pad session shape --session <id|path>` no longer silently falls
back when the resolver errors. An explicit flag means the caller has a
specific session in mind; a typo or wrong UUID should fail loudly so
automation surfaces the bug instead of emitting `agent: "unknown"`.
The implicit (no-flag) path still falls back for non-Claude-Code
harnesses.
Tests added/extended:
- TestResolveSessionLog_RejectsPathTraversal — flag-id AND env-id
branches, full bad-input matrix.
- TestParseSessionJSONL_OversizedLine — 10 MiB single record.
- TestParseSessionJSONL_NonArrayContent — content-as-string variant
must still contribute timestamp/version/usage.
- TestParseSessionJSONL_ParallelToolUse — tight hermetic check of the
R1 P2 multi-tool-per-turn fix.
- TestBuildSessionShape_ExplicitFlagErrors — verify --session errors
propagate.
- TestBuildSessionShape_ImplicitFallback — verify implicit path
still falls back.
|
||
|
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
|
713421670a |
feat(store): corrective backfill for collections.settings shape violations (IDEA-1489) (#567)
* feat(store): corrective backfill for collections.settings shape violations (IDEA-1489) Migration 055 / pgmigrations/034 (PR #562) hardened collections.settings to NOT NULL DEFAULT '{}' but its backfill only matched `settings IS NULL`. Pre-existing rows with wrong-shape settings (empty string, JSON "null" literal, "[]" array, non-JSON garbage on SQLite; JSONB null, arrays, primitives on Postgres) survived the filter — NOT NULL satisfied but the IDEA-1484 contract (settings is always a JSON object) violated. PR #566 established the per-driver `json_valid()` / `jsonb_typeof()` widening pattern for its own four migrations (056, 057, pg/035, pg/036) but was scope-bounded out of retroactively repairing 055 / pg/034. This commit closes that gap. - migrations/058_collections_settings_shape_repair.sql: UPDATE collections SET settings = '{}' WHERE settings IS NULL OR json_valid(settings) = 0 OR json_type(settings) != 'object'; - pgmigrations/037_collections_settings_shape_repair.sql: UPDATE collections SET settings = '{}'::jsonb WHERE settings IS NULL OR jsonb_typeof(settings) != 'object'; - internal/store/collections_settings_shape_repair_test.go: TestCollectionsSettingsShapeRepair_SQLite seeds rows with every observable pre-055 pathology, applies 055-058, asserts each is repaired to '{}' and valid rows are preserved. TestCollectionsSettingsShapeRepair_Postgres seeds JSONB-valid-but- wrong-shape rows (NULL is unseedable post-pg/034) and asserts the pg/037 UPDATE clause repairs them. Toggle-verified: with the WHERE predicate reverted to `IS NULL` only, both tests fail on the non-NULL malformed seeds; with the full predicate both pass. Full ./... suite green on SQLite and Postgres. * chore: gofmt comment in shape repair test |
||
|
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
|
853a7cd453 |
feat(store): wrap migrations in atomic transactions (IDEA-1485) (#565)
* fix(store): wrap each migration in a transaction (IDEA-1485) Pad's SQLite migration runner exec'd statements one-by-one on the raw *sql.DB, then INSERT'd into schema_migrations afterwards. A crash between statement N and the bookkeeping INSERT left the schema in an intermediate state, and the next startup re-ran the migration against the already-mutated database — permanent data loss for the table-rebuild migrations 022 / 055. Wrap each migration body + the schema_migrations INSERT in a single BEGIN/COMMIT so they commit (or roll back) atomically. Same pattern for Postgres migrations. PRAGMA-foreign-keys handling for SQLite is load-bearing: the existing 022 / 055 migrations rely on `PRAGMA foreign_keys = OFF` around their DROP TABLE, and that PRAGMA is a no-op inside a SQLite transaction. The new runner lifts PRAGMA statements out of the migration body, pins the migration to a single connection via db.Conn() (foreign_keys is per- connection, so the PRAGMA must hit the same conn that opens BEGIN), and classifies pragmas: foreign_keys=OFF runs BEFORE BEGIN, foreign_keys=ON runs AFTER COMMIT. If a migration disables FKs but never re-enables them, the runner emits PRAGMA foreign_keys = ON itself so the pool conn doesn't leak foreign_keys=OFF. Regression coverage in migration_atomicity_test.go: a deliberately- failing multi-statement migration must NOT record a schema_migrations row AND its partial DDL must roll back, on both SQLite and Postgres. A separate test exercises the PRAGMA-lift path with a real FK rebuild. * fix(store): restore PRAGMA foreign_keys=ON on every exit path (IDEA-1485 P2) Codex R1 caught a P2 in applySQLiteMigration: once `PRAGMA foreign_keys = OFF` exec'd successfully on the pinned conn, any subsequent failure (BeginTx, execMulti, INSERT INTO schema_migrations, Commit, or a post-tx PRAGMA error) returned early before the success-path FK-restore block could run. The conn went back to the pool with foreign_keys=OFF, silently bypassing enforcement for whichever caller next checked it out. Replace the post-success restore block with an inline `defer` registered the moment a before-tx `foreign_keys=OFF` lands on the connection. The defer fires on every return path. The restore is best-effort: a failure is logged via slog.Error (so a real leak isn't silent) but does NOT override the migration's primary error. The migration's own `PRAGMA foreign_keys = ON` (if present) still runs via afterTx on the success path; the deferred re-set is then a harmless idempotent no-op. Drop the now-redundant `disabledFKs` / `reEnabledFKs` tracking and the unused `isForeignKeysOn` helper. Regression test `TestMigrationAtomicity_FailedSQLite_RestoresForeignKeysOnError` pins the pool to a single connection via SetMaxOpenConns(1), runs a migration that disables FKs and then fails on a duplicate PK, and asserts that on a follow-up checkout `PRAGMA foreign_keys` returns 1. Verified the test FAILS when the defer is removed. |
||
|
xarmian
|
7e37dfc34e |
refactor(store): collections.settings boundary normalization + scan revert (IDEA-1484 follow-up) (#563)
* refactor(store): drop defensive sql.NullString scans on collections.settings (IDEA-1484 follow-up) PR #562 (squash |
||
|
xarmian
|
0766d7ecf1 |
feat(store): enforce NOT NULL on collections.settings (IDEA-1484) (#562)
* feat(store): enforce NOT NULL on collections.settings (IDEA-1484)
Adds migration 055 (SQLite) and pg 034 (Postgres) to backfill any NULL
collections.settings rows to '{}' and then enforce NOT NULL DEFAULT '{}'
at the column level. Eliminates the bug class that BUG-1482 / PR #561
plugged defensively in the four reader sites.
SQLite uses the standard table-rebuild recipe (PRAGMA foreign_keys=OFF,
copy via COALESCE, RENAME, recreate the single dependent index from
032_permission_indexes.sql). Same PK values are preserved so FKs in
items, views, collection_access, and grants remain valid.
Postgres uses the simple in-place ALTER TABLE; SET DEFAULT is a no-op
belt-and-braces since 001_initial.sql:115 already had DEFAULT '{}'.
The defensive sql.NullString scans in collections.go / export.go and
the import-side ""→"{}" coercion in export.go remain in place — they
revert in a separate follow-up PR after this migration ships
everywhere.
Removes the four BUG-1482 NULL-only regression tests from
collections_test.go (their `UPDATE collections SET settings = NULL`
setup is now a hard write error against the new constraint and the
NULL-scan branch they guarded is no longer reachable). Reworks
TestExportImportRoundTripWithNullSettings into
TestExportImportRoundTripWithEmptyStringSettings — it now mutates
the exported bundle in-memory to carry the "" sentinel rather than
forcing a NULL row, still exercising the import-side coercion path
that survives this PR.
* test(store): cover collections.settings NOT NULL outcome (IDEA-1484)
Addresses Codex R1 P2: the migration test surface lacked direct
constraint-check coverage. Adds two focused outcome tests against the
post-migration schema:
- TestCollectionsSettingsNotNullEnforced — raw INSERT with settings=NULL
must fail. Error shape differs across SQLite (NOT NULL constraint
failed) and Postgres (SQLSTATE 23502); we only assert err != nil.
- TestCollectionsSettingsDefaultsToEmptyObject — raw INSERT omitting the
settings column entirely must materialize the column DEFAULT as the
Go string "{}" when read back via GetCollection. Same assertion on
both drivers; the defensive sql.NullString scan + Postgres JSONB
normalization both surface "{}".
Both tests reuse createTestWorkspace + the testStore harness, so they
run automatically on whichever driver the test invocation selects.
R1 P1 (migration runner atomicity) is out of scope per established
codebase precedent (022, 025 use the same pattern); will be filed as
a follow-up IDEA.
|
||
|
xarmian
|
714da48442 |
fix(store): handle nullable collections.settings end-to-end (BUG-1482) (#561)
* fix(store): make ListCollectionsMinimal Postgres-safe (BUG-1482)
`COALESCE(settings, '')` failed at planner time on Postgres because
`collections.settings` is JSONB and `''` is not valid JSON
(SQLSTATE 22P02). The query failed regardless of row contents; SQLite
is type-loose and accepted it, leaving the bug latent in the two
production callers (`handlers_dashboard.go`, `handlers_items.go`).
Switch the query to a plain `SELECT ... settings ...` and scan into
`sql.NullString`, materializing NULL as the empty-string sentinel.
This preserves the existing contract that downstream consumers
(`buildDoneContextMap`, `ListCollections`'s own scan loop) gate on
via `if c.Settings != ""`, so no caller-side changes are needed.
Adds two regression tests in `collections_test.go` that exercise the
NULL-settings case (the planner-time failure mode) and the happy-path
JSON round-trip. Both run against SQLite and Postgres via the existing
PAD_TEST_POSTGRES_URL switch in `testStore`.
* test(store): tighten ListCollectionsMinimal happy-path assertion (BUG-1482)
Codex review round 1 flagged TestListCollectionsMinimalReturnsSettingsJSON
as too permissive: `Settings != ""` would pass for `{}` or any wrong JSON
payload. Postgres JSONB also normalizes formatting/key order, so a string
compare against the input literal would be brittle across drivers.
Switch to a semantic compare: unmarshal both sides into map[string]any
and reflect.DeepEqual. This actually verifies the JSON round-trips
through the (now fixed) ListCollectionsMinimal path on both drivers.
* fix(store): NULL-safe settings scan in GetCollection / ListCollections / ExportWorkspace (BUG-1482)
Round-2 extension of the same fix shape. Direct `Scan(... &c.Settings ...)`
into a Go string fails on Postgres for any row holding a real NULL with
"Scan error: converting NULL to string is unsupported". The column is
nullable on both drivers (TEXT DEFAULT '{}' / JSONB DEFAULT '{}'), so
legacy or manually-poisoned rows can 500 every handler that goes through
these readers — `GetCollection` is the hot reader on every item handler,
`ListCollections` powers dashboard + sidebar, `ExportWorkspace` crashes
the export pipeline before any data is emitted.
Same fix as ListCollectionsMinimal: scan into sql.NullString, materialize
NULL as "" to preserve the existing sentinel contract that downstream
consumers gate on via `if c.Settings != ""` (handlers_dashboard.go:247,
handlers_items.go:1626, collections.go:196 in ListCollections's own
post-scan loop). Audited; no caller depends on a non-empty default.
Adds TestGetCollectionHandlesNullSettings, TestListCollectionsHandlesNullSettings,
and TestExportWorkspaceHandlesNullSettings — each forces a NULL via direct
UPDATE (bypassing CreateCollection's empty→`{}` coercion) and asserts the
function returns without error and surfaces "" downstream. All pass on
SQLite and Postgres.
* fix(store): coerce empty-string settings to {} on workspace import (BUG-1482)
The earlier commits in this PR made ExportWorkspace, GetCollection, and
ListCollections all return `""` for a NULL `collections.settings` row,
preserving the in-process sentinel contract that downstream consumers
(buildDoneContextMap and friends) already gate on via `c.Settings != ""`.
That fix surfaced a paired contract gap: ImportWorkspace previously
inserted `c.Settings` verbatim into the collections table. After the
reader fixes, an exported NULL-settings row materializes as `""` in the
bundle, which Postgres's JSONB column rejects at INSERT time. Without
this commit, exporting a workspace with any NULL-settings row and
re-importing it would have crashed on Postgres — turning one half of a
symmetric contract green while leaving the other half broken.
Mirror the same coercion CreateCollection applies on the normal create
path: when the bundle's settings field is the empty-string sentinel,
write `"{}"` instead. Add a round-trip regression test
(TestExportImportRoundTripWithNullSettings) that NULL-poisons a workspace's
settings, exports, re-imports, and asserts the re-imported collections
hold valid JSON. Verified on both drivers.
* style(store): rewrite doc comment to avoid gofmt apostrophe-pair rewrite
Go 1.19+ gofmt's doc-comment formatter collapses `''` (two ASCII
apostrophes) inside backtick code spans into a single `”` (U+201D right
double quotation mark) — a typographic-pair heuristic that doesn't quite
fit when the literal pair is the load-bearing thing being described
(here: SQL's empty-string literal in COALESCE).
CI's golangci-lint flagged the file as gofmt-dirty for this reason.
Rewrite the prose to describe the bug without using `''` literally:
"coalesced settings against an empty SQL string literal" reads more
clearly than `COALESCE(settings, '')` becoming `COALESCE(settings, ”)`
after gofmt normalization. Functionally identical comment; lint-clean.
|
||
|
xarmian
|
7c663a3d3f |
feat(collections): add blank workspace template + retire auto-upgrade hook (IDEA-1479) (#560)
* feat(collections): add blank workspace template (IDEA-1479)
Introduces a `blank` workspace template that seeds only the two system
collections (Conventions, Playbooks) — no Tasks/Ideas/Plans/Docs, no
seeded items, no starter conventions or playbooks. Solves the
agent-self / non-template-fit use case where the existing software
templates leave undeletable ghost collections in the workspace.
Adds a new `CategoryCustom` ("Custom") top-level category so the blank
template doesn't mis-group with `startup` / `scrum` / `product`.
Category is appended last in `CategoryOrder` so it doesn't displace
recommended-path templates in the picker.
Tests:
- TestBlankTemplateShape — exactly 2 system collections, no seeds.
- TestBlankTemplateExcludesSoftwareCollections — no tasks/ideas/plans/docs.
- TestBlankTemplateAppearsInPicker — surfaces under a Custom group.
- TestSeedFromBlankTemplate — bootstrapping produces 2 collections, 0 items.
* fix: address codex review for blank template (IDEA-1479)
- CreateWorkspaceModal: remove hard-coded 'blank' picker entry that
silently fell through to collections.Defaults(). The API-driven blank
template (under the Custom category) is now the canonical surface.
- Dashboard: gate '+ New Task' button on tasks collection existence so
blank workspaces don't render a button that targets a missing
collection.
- OnboardingChecklist: accept collectionSlugs prop and filter steps
whose target collection (plans/tasks/docs) is absent. Conventions
step remains unconditional since the conventions collection ships
with every template, including blank. Empty-steps guard added to
progressPct to avoid NaN.
- web/src/lib/utils/templates.ts: add 'custom' -> 'Custom' to mirror
the Go CategoryOrder + categoryLabels updates.
- cmd/pad/templates_picker_test.go: extend the visible-template
assertion list to include 'blank' and assert the Custom category
header renders.
* fix(store): gate SeedDefaultCollections on zero-collection workspaces (IDEA-1479)
The server's startup auto-upgrade hook (cmd/pad/main.go) called
SeedDefaultCollections against every workspace at boot. That hook
dates to the initial release — long before workspace templates
existed — and was written as a backfill for workspaces created
before tasks/ideas/plans/docs landed in Defaults().
Post-templates, the hook unconditionally re-materialized the
Software-template collections into any workspace missing them —
including blank-template workspaces (IDEA-1479), which ship only
Conventions + Playbooks by design. Result: every restart silently
regrew the ghost user-facing collections the blank template was
explicitly built to avoid.
Fix: SeedDefaultCollections now returns nil immediately when the
workspace has any existing collection (system or user-facing). The
rescue path still triggers for genuinely-empty workspaces, preserving
the original backfill intent.
Tests:
- TestBlankWorkspaceSurvivesSeedDefaultCollections — blank workspace
remains 2 collections after auto-upgrade (and after a second pass).
- TestEmptyWorkspaceStillGetsDefaults — zero-collection workspace
still gets the full Software default set.
* refactor(server): remove SeedDefaultCollections auto-upgrade at startup (IDEA-1479)
The startup auto-upgrade hook in cmd/pad/main.go dated to the initial
release, predating workspace templates entirely. Its original intent
was per-collection backfill — workspaces created before a new entry
landed in Defaults() would acquire it on next boot. Post-templates,
that semantic is incompatible with templates that legitimately
diverge from Defaults() (e.g. `blank`, which ships only Conventions
+ Playbooks by design).
Round-2 of the IDEA-1479 review attempted to keep the hook by adding
a "zero collections" guard, but Dave (after codex round 3) decided
the cleanest fix is removing the hook entirely. The codebase has
proper migration infrastructure now; any future "add a default
collection" work should land as an explicit migration where the
author chooses which workspaces to backfill.
SeedDefaultCollections itself is preserved (with the round-2 guard)
as a building block for any future explicit rescue command or
migration. Its doc comment is updated to note it's no longer
auto-invoked at startup. The round-2 regression tests
(TestBlankWorkspaceSurvivesSeedDefaultCollections,
TestEmptyWorkspaceStillGetsDefaults) still apply and pass unchanged.
* fix(store): rescue gate uses COUNT(*), not ListCollectionsMinimal (IDEA-1479)
Postgres CI on PR #560 caught a regression introduced in commit
|
||
|
xarmian
|
be68292e03 |
fix(store): workspace list freshness reflects item activity (BUG-1481) (#559)
`pad workspace list` was showing `workspaces.updated_at`, which only moves on workspace-row mutations (rename, settings, members). After this fix the effective UpdatedAt surfaces item activity inside the workspace — answering "where is work happening?" instead of "when was this row last UPDATEd?". Implementation: scalar `MAX(items.updated_at)` subquery in `ListWorkspaces` and `GetUserWorkspaces`, then `effectiveWorkspaceUpdatedAt` picks the later of the two timestamps (portable across SQLite + Postgres, no GREATEST). Read-time approach per the bug's design notes. Visibility-aware: codex review surfaced that a naive MAX leaks activity timing for items the caller can't see. The member subquery mirrors `VisibleCollectionIDs` (`collection_access='all'` short-circuits; for `'specific'` members, system collections + `member_collection_access` + `collection_grants` + `item_grants` gate visibility). The guest subquery limits MAX to items reachable via `collection_grants` / `item_grants`. All grant lookups are workspace-scoped for defense-in-depth. Four regression tests cover: admin `ListWorkspaces`, member all-access, member specific-access leak guard, and guest leak guard. |
||
|
xarmian
|
e621eacb9b |
feat(server): POST /api/v1/import/url endpoint + integration tests (TASK-1472) (#556)
Wires internal/urlimport into the API: Fetcher → Detect → converters.
Side-effect-free; the editor's "Insert from URL" modal owns any item
mutation (TASK-1474).
Endpoint:
- POST /api/v1/import/url, body {"url"}, response {markdown,
detected_type, title?, source_url, fetched_at, content_type}.
- Status mapping: 400 invalid URL / SSRF / malformed body; 502 upstream
failure (incl. size cap); 504 fetch timeout; 422 conversion failure.
- Swagger 2.0 fallback: detected as "openapi" by the sniffer but
rejected by ConvertOpenAPI → falls through to ConvertGeneric and
re-classifies the response as "generic" so the UI shows the right
affordance.
- 30s wall-clock budget on the whole pipeline via context.WithTimeout
(Fetcher's own timeout is the HTTP-level cap).
Package-level Fetcher is memoized via the existing sync.Once-cached
safe transport (shared keep-alive pool, no per-request leak). The
handler skips the pre-flight ValidateURL when the package fetcher
has AllowLocal=true so tests can swap in a loopback-friendly fetcher
without bypassing the production guard.
Integration tests (handlers_import_test.go):
- HTML happy path (httptest upstream, asserts detected_type/title/
source_url/fetched_at/content_type all wired up).
- OpenAPI 3.x happy path (inline YAML upstream, asserts ConvertOpenAPI
was invoked and detected_type=openapi).
- Swagger 2.0 fallback (asserts detected_type re-classified to
generic, markdown non-empty).
- SSRF rejection (default fetcher, loopback URL → 400 mentioning
"private"/"reserved").
- file:// scheme rejected (400).
- Missing/malformed body rejected (400).
- Upstream 5xx surfaces as 502.
- Size cap exceeded surfaces as 502.
- No-side-effects check: item count unchanged before/after import.
Parent: PLAN-1467.
|
||
|
xarmian
|
8771f95ab2 |
feat(urlimport): OpenAPI 3.x → Markdown converter (TASK-1471) (#555)
* feat(urlimport): OpenAPI 3.x → Markdown converter (TASK-1471)
Adds ConvertOpenAPI to internal/urlimport — the "openapi" branch of
the v1 importer. Built on pb33f/libopenapi.
Layout:
- H1 with the API title + version + description
- Contact + License lines
- Servers list
- Endpoints section grouped by primary tag (or "Other" for the
untagged). Per operation: `METHOD /path` heading, summary,
description, deprecation marker, operation ID, parameter table,
request-body summary (with media-type fences and YAML-rendered
example), and response code table.
- Schemas section with component schemas as Property/Type/Required/
Description tables, schema names sorted for stable output.
Scope:
- OpenAPI 3.x only. Swagger 2.0 detection returns an explicit
"only 3.x" error so the import endpoint (TASK-1472) can fall
through to the generic converter.
- Recoverable libopenapi build errors (unresolved refs, etc.) are
swallowed when the model is still produced — partial spec >
no output.
Tests:
- testdata/petstore-openapi.yaml — full v3 fixture: tags, params,
requestBody example, deprecated op, ref-typed schema array, two
component schemas with required-field markers.
- TestConvertOpenAPI_Petstore — 30+ markdown-substring assertions
on the rendered output.
- TestConvertOpenAPI_RejectsSwagger2 — explicit v2 error.
- TestConvertOpenAPI_RejectsGarbage — non-spec input.
- TestConvertOpenAPI_MinimalSpec — empty paths short-circuits.
- Helpers: schemaTypeBrief(nil), escapeTableCell, singleLine.
Dependency: github.com/pb33f/libopenapi v0.36.3 (MIT-licensed).
Parent: PLAN-1467.
* fix(urlimport): merge path-level + operation-level parameters per Codex review (round 1)
MEDIUM: OpenAPI path-item-level parameters apply to every operation
on the path. Previously only slot.op.Parameters was rendered, so
common specs that hoist a shared {id} parameter to the path-item
level emitted operations with the path parameter missing from the
docs.
Now opSlot carries item.Parameters as pathParams, and a new
mergeParameters helper produces the spec-conformant union:
- Path-level parameters first, in declared order.
- Operation-level parameters with matching (name, in) override the
path-level entry in place.
- Operation-only parameters appended after.
Tests:
- TestConvertOpenAPI_PathLevelParametersMerged — inline fixture
with a path-level widgetId + trace and an operation-level trace
override + fields op-only param. Asserts widgetId survives, trace
shows op-level (required=yes), no duplicate path-level trace row,
fields appears.
- TestMergeParameters_EmptyInputs — nil/nil short-circuit.
* fix(urlimport): no double-backticks on array-of-ref schema types per Codex review (round 2)
MEDIUM: schemaTypeBrief() previously wrapped refs in inline backticks
("`Pet`"). For array-of-ref schemas the brief became "array of `Pet`",
and the table-cell call site (codeOrBlank) then wrapped the entire
value in another pair, producing broken markdown like
"`array of `Pet``". Schema properties whose type is an array of a
component schema are a normal OpenAPI shape — `Litter.pets: array of
Pet` — so this would have hit real specs immediately.
Fixes:
- schemaTypeBrief now returns plain text — ref names without
surrounding backticks. Docstring updated to make the contract
explicit ("never contains backticks; caller wraps").
- codeOrBlank strips any stray backticks from input before wrapping
so the resulting cell always carries exactly one balanced pair.
Defensive: the contract from schemaTypeBrief is plain text now,
but stray backticks from any future caller can't corrupt the
table.
Tests added:
- TestConvertOpenAPI_ArrayOfRefTypeCell — inline spec with a
`Litter.pets: array of Pet` property. Asserts the type cell is
exactly `` `array of Pet` `` and no malformed variants leak.
- TestCodeOrBlank — 7-case table covering empty, plain, whitespace,
pre-backticked, embedded-backtick, and backtick-only inputs.
|
||
|
xarmian
|
d1560606cb |
feat(urlimport): generic HTML→Markdown converter (TASK-1470) (#553)
* feat(urlimport): generic HTML→Markdown converter (TASK-1470)
Adds ConvertGeneric to internal/urlimport — the v1 catch-all converter
for "non-OpenAPI" URLs. Pipeline:
1. go-shiori/go-readability strips chrome/nav/ads/scripts and returns
the page's primary article.
2. JohannesKaufmann/html-to-markdown/v2 converts the cleaned HTML to
markdown.
3. cleanupMarkdown normalizes line endings, trims trailing whitespace,
collapses blank-line runs, and ensures a single trailing newline.
Fallback path: when Readability cannot identify an article (directory
listings, single paragraphs, pages with no clear content container),
the converter falls back to a whole-body conversion so callers still
get usable markdown.
Dependencies (license-checked):
- github.com/JohannesKaufmann/html-to-markdown/v2 v2.5.1 (MIT)
- github.com/go-shiori/go-readability (Apache-2.0)
Fixtures + tests:
- testdata/availity-shape.html — Availity-style div soup with heavy
chrome (nav, ads, sidebar, footer, analytics script). Asserts the
article content survives and the chrome is stripped.
- testdata/mdn-shape.html — MDN-style semantic HTML (article/main +
proper heading levels + code fences). Asserts structure preserved.
- TestConvertGeneric_EmptyBody — empty-input rejection.
- TestConvertGeneric_PlainTextFallback — Readability-can't-find-article
fallback path.
- TestCleanupMarkdown — 5-case table-driven cleanup verification.
Parent: PLAN-1467.
* fix(urlimport): preserve hard-line-break markers + apply WithDomain on fallback per Codex review (round 1)
- MEDIUM: cleanupMarkdown was stripping the markdown two-trailing-
spaces hard-line-break idiom. html-to-markdown emits <br> as
" \n" — bulk-stripping trailing whitespace was demoting hard
breaks to soft wraps. Now the cleanup steps line-by-line, keeps
exactly-two trailing spaces (no tab), strips 1/3+/tab-mixed runs.
- MEDIUM: The raw-HTML fallback path (used when Readability cannot
identify an article) now passes converter.WithDomain(pageURL) so
relative links/images resolve against the source URL rather than
the Pad host where they'd 404.
Tests added:
- cleanupMarkdown: hard-line-break preserved, single/triple trailing
spaces stripped, tab-mixed spaces stripped, blank-line-with-spaces
collapsed (5 new cases).
- TestConvertGeneric_RelativeURLsResolvedOnFallback: relative href
in non-article HTML resolves to absolute URL via pageURL.
|
||
|
xarmian
|
0aa3988319 |
feat(urlimport): URL fetcher with SSRF guard + content-type detection (TASK-1469) (#552)
* feat(urlimport): URL fetcher with SSRF guard + content-type detection (TASK-1469) First slice of PLAN-1467's "Insert from URL" feature. Adds the internal/urlimport package with: - fetch.go: SSRF-guarded HTTP GET (10s timeout, 5 MB body cap, redirect re-validation, redacted-error formatting). Blocks loopback, RFC1918, CGNAT, IPv4/IPv6 link-local (incl. 169.254.169.254 cloud-metadata), IPv6 unique-local, and the unspecified address. Hostnames are resolved and every returned IP is checked. - detect.go: Content-type + body-prefix sniff returning "openapi" (JSON or YAML, OpenAPI 3.x or Swagger 2.0) or "generic". Inspects at most 64 KiB. - fetch_test.go: Table-driven SSRF tests covering 24 cases plus happy-path, size-cap, timeout, non-2xx, context-cancel, and a stubbed-transport redirect re-validation. - detect_test.go: 20 detection cases including OpenAPI JSON, Swagger YAML, vendor media types, leading comments, indented-key negatives, and charset-parameter normalization. Package name is urlimport (not "import" — reserved word). No callers yet; the endpoint that consumes Fetcher + Detect lands in TASK-1472. Parent: PLAN-1467. * fix(urlimport): close DNS-rebinding gap + handle >64 KiB OpenAPI JSON per Codex review (round 1) - HIGH: Add safe dialer transport (newSafeTransport). ValidateURL no longer does DNS — the dialer resolves once and validates the resolved IP at dial time, then dials that exact IP. DNS rebinding can no longer slip a public-IP validation past a loopback fetch. ValidateURL becomes a pre-flight (scheme/credentials/IP-literal only) with the canonical guarantee now at the transport layer. - MEDIUM: For JSON bodies over the 64 KiB sniff cap, switch from full Unmarshal (which fails on a truncated tail) to a streaming json.Decoder scan that walks top-level keys and short-circuits as soon as `openapi` or `swagger` is seen. Real-world specs over 64 KiB are now classified correctly, including the case where the `openapi` key is not the first top-level entry. Tests added: - TestFetch_DialerBlocksLoopbackHostname (dial-time rebinding guard) - TestDetect_LargeOpenAPIJSON (>200 KiB OpenAPI body, key first) - TestDetect_LargeOpenAPIJSON_KeyNotFirst (openapi key after huge info) - TestDetect_LargeJSONNotOpenAPI (huge non-OpenAPI stays generic) Removed the DNS-resolution case from TestValidateURL's notes and added a positive case proving hostnames pass the pre-flight (the dial-time check is now the canonical guard). * fix(urlimport): disable env proxy and reuse safe transport per Codex review (round 2) - HIGH: Set Proxy=nil on the safe transport. ProxyFromEnvironment would route via HTTP_PROXY/HTTPS_PROXY, where the dialer connects to the proxy host instead of the target — silently bypassing the hostname-resolution SSRF check inside DialContext. Operators who need an outbound proxy can wire their own trusted transport into Fetcher.Transport. - MEDIUM: Memoize the default safe transport per Fetcher via sync.Once. Previously each Fetch built a fresh *http.Transport whose keep-alive idle-pool stayed in scope until GC, leaking FDs under repeated imports. Now one transport is shared by all Fetch calls on a Fetcher; AllowLocal is captured at first use. |
||
|
xarmian
|
38aa872864 |
fix(fields,activity): debounce typed-input field saves + collapse same-field activity runs (BUG-1466) (#549)
* fix(web): debounce typed-input field saves to stop per-keystroke activity rows (BUG-1466)
Text / number / URL fields in FieldEditor wired oninput directly to
onchange, so every keystroke became an item PATCH and an activity row.
Typing `ui/editor/tiptap` into a `component` field produced a 30-step
keystroke chain in the audit metadata (visible on BUG-1419's timeline).
Wrap the typed-input branches in a 500ms idle debounce, flush on blur
so tabbing away commits immediately, and flush on unmount so navigation
never drops a pending value. Discrete inputs (select / date / checkbox,
number ±1 buttons) keep firing on the user action — they aren't typing.
Mirrors the markdown content debounce pattern in the detail page.
* fix(activity): collapse same-field runs in merged changes metadata + unify diff separator (BUG-1466)
Follow-up to the web-side typing debounce. Two related changes:
1) collapseChanges() walks the merged "; "-delimited changes string and
collapses runs of consecutive same-field entries into a single
"field: first-old → last-new". Drops net no-ops (typed then backspaced).
When the web-side debounce in FieldEditor still produces multiple
PATCHes within the 5-minute coalesce window — or for older rows that
pre-date the debounce — the timeline now reads as one transition
instead of a chain. Run-based (not global) collapse so interleaved
edits on different fields keep their chronology.
2) diffFields now joins entries with "; " instead of ", " so the joiner
is consistent with mergeActivityMeta and TimelineActivityCard.svelte's
split delimiter. Multi-field PATCHes previously rendered as a single
unparseable blob in the web timeline because the parser only split on
";" — fixed as a side-effect.
Adds TestCollapseChanges (10 cases including the BUG-1419 repro) and
TestMergeActivityMeta_CollapsesSameFieldRun. Updates the existing
TestDiffFieldsPrimitives expectation to match the new joiner.
* fix(fields,activity): two follow-ups per Codex review (round 1)
[P1] FieldEditor.svelte::handleNumberStep
The ±1 buttons computed `(Number(value) || 0) + delta` AFTER calling
flushPendingSave(). But `value` is the parent prop — flushing fires
onchange asynchronously, so at the moment of the step computation
the prop still holds the pre-typed value. Typing 10 over 5 and
clicking + would flush 10 then send 6, overwriting the typed value.
Compute `base` from `pendingValue` (if hasPending) BEFORE clearing
the timer state, then send `base + delta` in one onchange call.
[P2] activities.go::collapseChanges
The drop-net-no-op step removed entries where `from == to`. But
diffFields intentionally emits same-display entries for
same-cardinality structured-field replacements like
`implementation_notes: (1 note) → (1 note)` (see
TestDiffFieldsSameCardinalityArrayChangeStillReported) — the labels
match because formatChangeValue summarizes by count, not content,
but the underlying data did change. Dropping them silently hides
real updates from the activity feed.
Track `mergedCount` per entry: increment when collapsing a run,
initialize to 1 on parse. Only drop when `mergedCount > 1 && from == to`
— i.e. only when the no-op resulted from collapsing multiple input
segments (the typed-then-backspaced case).
Tests: 2 new TestCollapseChanges cases (single structured-field
preserved, interleaved structured-field around a typed run).
* fix(fields,activity): two follow-ups per Codex review (round 2)
[P1] FieldEditor.svelte number stepper focus race
The number ±1 buttons race with the input's onblur handler: blur
fires before click in the natural focus-transfer flow, so
flushPendingSave clears hasPending → handleNumberStep reads stale
`value` from the parent prop → typing 25 over 10 and clicking +
sends 25 then 11, losing the typed value.
Add onmousedown={preventDefault} on both ±1 buttons. Mousedown
precedes blur, and preventDefault on mousedown suppresses the
natural focus transfer — the input keeps focus through the click,
so hasPending survives until handleNumberStep reads it.
[P2] collapseChanges still drops repeated same-display structured runs
Round 1's mergedCount>1 rule still dropped runs like
`implementation_notes: (1 note) → (1 note); implementation_notes: (1 note) → (1 note)`
— two real updates whose display strings happen to match because
formatChangeValue summarises array-valued fields by count. Each
PATCH represented a different underlying note (diffFields uses
reflect.DeepEqual to detect that), but the merged display showed
no transition.
Track `hadTransition` per entry: true iff the run had a display-
level transition (initial from != to, or a subsequent entry's `to`
differed from the anchored from). Drop only when
mergedCount > 1 && from == to && hadTransition — i.e. only true
net-cancellations (typed-then-backspaced). Same-display structured
repeats stay; real `foo → bar → foo` swings still drop.
Tests: 2 new TestCollapseChanges cases (repeated same-display preserved,
real foo→bar→foo swing still dropped).
* fix(fields,activity): two follow-ups per Codex review (round 3)
[P1] FieldEditor cross-item leak via debounce timer
When the parent reuses a FieldEditor instance across an item swap
(same schema, same field.key, different item — common when
navigating between items in the same collection), the parent's
`updateField` closure reads `item.id` at CALL time. A pending
timer set while item A was active would fire after item B mounted,
patching B with A's typed value.
Two-pronged fix in FieldEditor:
- Add a $effect that tracks the `value` prop and drops any pending
save the moment the parent re-props us. The user actively typed
for a now-stale context; aborting is safer than silently writing
to the new context. Also covers external collab/SSE rebases of
the same field on the same item.
- Switch the unmount cleanup from flush → drop. When the parent
navigates to an item whose schema lacks this field, the
FieldEditor unmounts AFTER the parent's `item` has already been
replaced, so a final onchange call would route through
updateField → wrong item. Blur is the supported commit gesture
(clicking elsewhere within the page, ±1 buttons, tab-out — all
flush eagerly); unmount-without-blur is treated as "user
abandoned the edit."
[P2] collapseChanges still dropped structured count-return swings
Round 2's hadTransition rule still dropped a run like
`implementation_notes: (1 note) → (2 notes); implementation_notes: (2 notes) → (1 note)`
— the user added a note then removed the original, ending with a
different single note. The merged display reads `(1 note) → (1 note)`
with hadTransition=true, indistinguishable from a typed-then-
backspaced cancellation. But formatChangeValue summaries are
LOSSY: same display label can wrap entirely different raw values.
Add a `hasLossySummary` flag per entry — true iff either `from`
or `to` matches the `(text)` format formatChangeValue emits for
structured fields. Propagate stickily across the run (any lossy
entry pins the whole run). Skip the drop step when the run
carries lossy summaries — we can't recover the raw delta from the
merged string, so we preserve the entry rather than risk hiding
a real change.
Tests: 2 new TestCollapseChanges cases (count-return swing
preserved, lossy-on-one-side still pins the run).
* fix(fields): demote hasPending from \$state to plain let — \$effect was cancelling every keystroke (round 4 [P1])
Critical regression in round 3: the value-track \$effect read
\`hasPending\` inside its body, which Svelte 5 promotes to a reactive
dependency. scheduleSave() setting \`hasPending = true\` retriggered
the same \$effect, whose body cleared the typing timer + pendingValue
+ hasPending before the debounce could fire. Net effect: typing into
any text / number / URL field was silently dropped — onchange never
ran, the field never saved.
hasPending is only read from imperative handlers (scheduleSave,
flushPendingSave, handleNumberStep, the value-track \$effect, the
unmount cleanup) — never from a template or other reactive context.
Demoting it to a plain \`let\` removes the unwanted subscription while
preserving the round-3 behaviour: external value-prop changes still
trigger the \$effect (it tracks \`value\`), the body reads hasPending
imperatively to decide whether to clear pending state.
No tests added — this is a Svelte reactivity edge case that can't
be unit-tested without a DOM. svelte-autofixer's pre-existing
"variable assigned inside \$effect" suggestion previously flagged the
hasPending mutation; that signal is gone now.
Per Codex review round 4.
|
||
|
xarmian
|
438cb6180a |
fix(mcp): flexible JSON shapes on item create + clearer field surface (BUG-1431, BUG-1432) (#547)
* fix(mcp): flexible JSON shapes on item create + clearer field surface (BUG-1431, BUG-1432)
BUG-1432 root cause (real): models.ItemCreate.Tags is a Go string, so
the default unmarshaler rejected the natural JSON-array shape every
agent sends (`tags: ["foo","bar"]` → "cannot unmarshal array into Go
struct field ItemCreate.tags of type string", HTTP 400). On Postgres
the alternative — passing `tags: "foo,bar"` per the catalog's old
"Comma-separated tags" description — landed as a non-JSON value in
the JSONB column and surfaced as a generic HTTP 500. SQLite's TEXT
column silently accepted the corrupt value, which is why local repros
didn't show it.
Codex's independent investigation called out the asymmetry: ItemUpdate
already had a flexible UnmarshalJSON for `fields`/`tags` per BUG-1144,
but ItemCreate didn't. This PR mirrors that flexibility on the create
path and aligns the MCP surface description with reality.
BUG-1431 root cause (real, not the misdiagnosis the agent reported):
the dispatcher's `parseFieldKVP` only accepted the CLI-style array-of-
"key=value" shape, rejecting the JSON-native `field: {key: value}` map
shape with "expected array or string, got map[string]interface {}".
Agents naturally try the map shape and got a non-actionable error;
that drove the BUG-1409 agent to mis-blame status placement. Empirical
repro confirmed that `status` actually works in both top-level AND
inside-fields positions today (Tests 1, 4 in the investigation); the
real surface problem was the missing map shape on `field`.
Changes:
- internal/models/item.go: add UnmarshalJSON to ItemCreate mirroring
ItemUpdate's BUG-1144 pattern. Accepts `fields` as object or
JSON-encoded string; `tags` as array or JSON-encoded string; either
field absent / null leaves Go zero value. Wrong shapes surface
ErrInvalidFieldsType / ErrInvalidTagsType (existing sentinels) so
agents see clean domain errors instead of "Go struct field" leaks.
- internal/mcp/dispatch_http.go: parseFieldKVP now accepts
map[string]any in addition to the existing array/string shapes. Map
shape preserves non-string values verbatim (e.g. number from a typed
flag), matching the array path's existing pass-through for non-string
entries.
- internal/mcp/catalog_item.go: update `tags` description from
"Comma-separated tags" (wrong on both SQLite and Postgres) to
"Tags as a JSON array of strings, e.g. [\"v1\",\"frontend\"]". Update
`field` description to clarify it's the escape hatch for
SCHEMA-DECLARED custom fields, name the dedicated top-level params
agents should reach for instead (status/priority/category/parent/
role/assign/tags), and note the new map-shape acceptance. Tool-level
prose updated to match.
Tests:
- TestItemCreateUnmarshalFlexFields (mirror of
TestItemUpdateUnmarshalFlexFields): 9 cases covering array/string/
null/absent/wrong-shape tags + object/string/array fields, plus a
smoke test that other fields decode normally alongside the new
flex paths.
- TestParseFieldKVP_Variants: extended with 3 new map-shape cases
(basic map, empty-key-skipped, non-string-value preserved).
End-to-end verification: 5 input shapes via curl against the live
handler. Pre-fix `tags: ["foo","bar"]` returned HTTP 400; post-fix
returns HTTP 201 with `tags="[\"foo\",\"bar\"]"` in the column.
`tags: {x:1}` (wrong shape) now returns a clean
domain-level 400 instead of leaked Go internals. Existing back-compat
paths (JSON-encoded string forms) preserved.
Related: PR #546 (BUG-1430 rate limit) addressed the original 500
cascade that drove the agent's specific misdiagnoses in BUG-1409.
* fix(mcp): forward tags array on update + drop unsupported map-shape doc per Codex review (round 1)
Codex round 1 caught two issues:
[P1] dispatch_http_advanced.go's PATCH builder filtered on `string`
only when forwarding `tags`, so a schema-conforming
`pad_item.update tags: ["a","b"]` was silently dropped. Now forwards
verbatim like mapItemCreate does — the handler's ItemUpdate
flex-unmarshaler (BUG-1144) normalizes any shape downstream.
Regression test added.
[P2] The `field` description claimed `{key: value}` map shape was
accepted, but the schema Type stays `array<string>` so schema-following
clients won't send the map shape. parseFieldKVP's map-shape handling
(added in the previous commit) stays as defensive parsing for clients
that ignore the schema, but the description no longer promises a shape
the published schema doesn't advertise. Tool-level prose updated to
match.
* fix(mcp): revert speculative parseFieldKVP map-shape support per Codex review (round 2)
Codex round 2 [P2] pointed out the map-shape parseFieldKVP support
added in the first commit is dead code in practice:
1. The advertised schema for `field` is `array<string>` — no
schema-conforming client sends a map.
2. `BuildCLIArgs` rejects map-shaped repeatable flags before they
reach the HTTP dispatcher.
3. Even if a map did reach the dispatcher, `hasFieldChanges`
doesn't recognize map shapes as field changes — `pad_item.update
field: {effort: "l"}` would skip the merge and PATCH without
`fields`.
Either completing the support (fix hasFieldChanges + BuildCLIArgs +
ItemUpdate Unmarshal) OR reverting was the right call. Reverting
keeps the surface consistent with the schema and removes the
unreachable code; future agents who want to override fields can use
the documented `["key=value"]` array shape.
BUG-1431's functional fix lands as the catalog description tightening
(the empirical repro confirmed `status` placement already works in
both forms; the agent's misdiagnosis was rooted in unclear docs, not
broken code). BUG-1432's flexible JSON unmarshal on ItemCreate stays
— that's the real fix verified by the live-handler repro.
* fix(mcp): preserve empty-string tags no-op + table-driven test per Codex review (round 3)
Codex round 3 [P2] caught a regression introduced in round 1's fix: by
switching the tags forwarding guard from \`v.(string) && v != ""\` to
\`v != nil\` to support array shapes, the empty-string filter for tags
on update was lost. \`pad_item.update tags: ""\` would now forward an
empty string to ItemUpdate, which treats it as an explicit
empty-string write — corrupting the JSON/JSONB tags column (500 on
Postgres).
Fix: type-switch on tags. Empty string skips (matches pre-fix
behaviour); arrays (including empty array \`[]\`, the legitimate
"clear tags" case) and non-empty strings forward.
Tests: the single-shape array test is replaced with a table-driven
TestDispatchItemUpdate_TagsForwarding covering array, empty array,
empty string (no-op), and comma-separated back-compat. Each case
asserts the tags key's presence/absence and shape in the PATCH body.
|
||
|
xarmian
|
088ba2f839 |
fix(mcp): raise MCP per-token burst + classify 429 as ErrRateLimited (BUG-1430) (#546)
* fix(mcp): raise MCP per-token burst + classify 429 as ErrRateLimited (BUG-1430)
BUG-1409 reported an agent hitting "Pad backend 500s on parallel writes"
during workspace onboarding via remote MCP on Pad Cloud. Triage split
that umbrella into three children; this PR addresses BUG-1430 (the
parallel-writes symptom).
Root cause investigation showed the underlying write path is fine —
local SQLite handled 24 parallel item-create POSTs cleanly (busy_timeout
+ BEGIN IMMEDIATE + WAL serialize writers without errors). The most
plausible cause of the agent's "500 on parallel writes" report is the
MCP per-token rate limiter (burst 20, 60/min) rejecting requests 21-24
of an onboarding burst with HTTP 429, which the dispatcher's classifier
then collapsed into a generic ErrServerError envelope.
Changes:
- middleware_ratelimit.go: MCPPerToken burst 20 → 60. Sustained rate
unchanged at 60/min/token. Matches the general API limiter's burst-60
per-user cap so the MCP path no longer imposes a tighter ceiling than
the equivalent /api/v1 path. Comment expanded to record the rationale.
- internal/mcp/errors.go: add ErrRateLimited error code and an explicit
case http.StatusTooManyRequests in classifyHTTPStatusKind. 429s now
surface as a first-class rate-limited envelope with an actionable hint
pointing at Retry-After and the per-token cap, instead of landing in
the generic ErrServerError "other 4xx" bucket. Agents implementing
exponential backoff can switch on code without parsing free-form text.
- handlers_cloud.go: add slog.Error instrumentation to enforcePlanLimit
and enforceUserPlanLimit error paths. These are cloud-mode-only 500
candidates we couldn't exercise locally (local dev runs cloudMode=false);
the structured logs give operators a grep-able tag the next time the
symptom surfaces on real Pad Cloud, so we can rule the path in or out
empirically without another investigation pass.
- tests: bump iteration counts past the new burst (20 → 60), add 429
case to classifyHTTPStatus code-mapping table + envelope hint-shape
table.
Investigation context (full triage in BUG-1430):
- ../pad-cloud sidecar is NOT in the /api/v1 or /mcp request path
(nginx-router proxies those directly to pad backend).
- featureCount + advisory-lock contention on Postgres remain plausible
500 candidates under heavy bursts; the new logging is intended to
catch those if they fire.
Siblings BUG-1431 (status field placement) and BUG-1432 (tags field)
are tracked separately and not addressed here.
* fix(mcp): drop hardcoded cap from rate-limit hint per Codex review (round 1)
Codex round 1 [P2] caught that rateLimitHintFor's "the per-token cap is
60 req/min with a burst of 60" text was misleading: classifyHTTPStatusKind
handles 429s from the dispatcher's SYNTHESIZED /api/v1/... requests, which
come from the general API limiter (600/min, burst 60), the Search limiter
(30/min, burst 10), and potentially others — NOT the MCP per-token
limiter (which fires before the dispatcher runs and so never lands in
this classifier path).
Generalize the hint: point at Retry-After (which carries the correct
limiter-specific wait) and drop the cap from prose. Update the matching
test assertion to assert the generic shape ("burst-heavy" instead of
"60 req/min").
|
||
|
xarmian
|
de8679f535 |
chore(mcp): bump ToolSurfaceVersion 0.3 → 0.4; document v0.4 envelope (TASK-1418) (#544)
* chore(mcp): bump ToolSurfaceVersion 0.3 → 0.4; document v0.4 envelope (TASK-1418)
Final PR of PLAN-1410. The contractual announcement that the v0.4
bootstrap shape is stable.
## What
1. internal/mcp/version.go — ToolSurfaceVersion: "0.3" → "0.4".
The godoc on the constant gains a full v0.4 changelog entry
enumerating each shape change shipped by PLAN-1410's six
bootstrap PRs:
- BootstrapCollection projection (TASK-1412): drops id,
workspace_id, created_at, updated_at, settings; schema as
a nested JSON object.
- BootstrapRole projection (TASK-1423): drops id,
workspace_id, tools, created_at, updated_at.
- Convention slug dropped (TASK-1413).
- Top-level recent_activity duplicate removed (TASK-1413).
- BootstrapDashboard wrapper caps five sub-arrays (TASK-1413
+ TASK-1422): attention, recent_activity, active_items,
active_plans, by_role at 5 entries each, parallel
*_overflow_count fields. suggested_next deliberately
excluded — already capped to 3 upstream.
- Schema label omitted when label == TitleCase(key) (TASK-1424).
Plus an explicit compatibility note: all v0.4 changes are
additive or subtractive (no field renames); clients that read
the preserved field names keep working unchanged.
2. CLAUDE.md updates:
- "## MCP server" header: v0.3 catalog → v0.4 catalog, with a
one-paragraph summary of what v0.4 shipped.
- "Surface:" Tools bullet: v0.3 → v0.4, with a note that the
tool/action surface is unchanged — only the bootstrap JSON
these tools return has been trimmed.
- "Stability contract": ToolSurfaceVersion (currently "0.4"),
comprehensive single-paragraph description of the v0.4
envelope, cumulative size reduction (40% live / 54% fixture),
and explicit additive/subtractive note.
## Why the strategy worked
PLAN-1410's "version bump last" strategy paid off:
- Each individual shape PR (TASK-1412/1413/1422/1423/1424) was
reviewable in isolation against a stable v0.3 contract.
- The six skill-side PRs (TASK-1414/1415/1416) had no MCP-shape
impact and didn't need any version bump consideration.
- v0.4 is now announced as a single comprehensive contract change,
not five separate version bumps — easier for downstream MCP
consumers (Claude Desktop, Cursor, future Pad Cloud remote MCP)
to reason about.
## Verification
- `make check` — golangci-lint 0 issues, all Go tests pass
(including the version-tracking tests in catalog_meta_test.go
that auto-pin to whatever ToolSurfaceVersion is set to),
govulncheck clean, web build clean.
- MCP handshake (verified via `pad mcp serve` + an initialize
JSON-RPC request) advertises
capabilities.experimental.padToolSurface.version = "0.4".
padCmdhelp.version stays at "0.1" as expected.
## Post-merge follow-ups
After this lands:
- Update PLAN-1410's Result section with a "v0.4 announced" line
and the final post-everything measurement (taken against
docapp after `make install`).
- Flip PLAN-1410 status from `active` → `completed`.
These are pad-item operations, not git changes.
Parent: PLAN-1410. Closes the plan.
* fix(mcp): update stale v0.3 references after ToolSurfaceVersion bump (TASK-1418 follow-up)
Address Codex P2 + P3 findings on PR #544: bumping
ToolSurfaceVersion in version.go left four runtime/user-facing
docs still claiming v0.3:
P2 — runtime MCP docs:
- internal/mcp/instructions.md "## Tool surface (v0.3)" → v0.4
- internal/mcp/catalog_meta.go "v0.3 server-introspection tool" → "(v0.4 catalog)"
- internal/mcp/catalog_meta.go padMetaToolDescription twice:
* "the v0.3 tool catalog" → "the v0.4 tool catalog"
* "v0.3 catalog dump" → "v0.4 catalog dump"
- internal/mcp/catalog_meta.go actionMetaToolSurface godoc:
"v0.3 catalog" → "catalog" (de-versioned; the comment is
about scope, not version)
P3 — public README:
- README.md "Tool catalog (v0.3)" → "Tool catalog (v0.4)"
- README.md "tool_surface_version: '0.3'" → "'0.4'" with a
pointer to PLAN-1410's bootstrap-trim summary and
version.go's full v0.4 changelog.
Without these, agents reading the initialize-instructions blob or
pad_meta's tool description (both of which are part of the
runtime MCP surface, not just internal docs) would see v0.3 while
the handshake / pad_meta.action: version returned v0.4 — the
exact "contradictory metadata depending on what you read" failure
mode Codex flagged.
Same skill-↔-code sync pattern that has been a running theme
through PLAN-1410's review loops. The cluster of stale references
is a classic side effect of a version bump landing late in a
plan — the version constant is one string, but downstream prose
that names it lives in multiple places.
Verified no remaining "v0.3" claims that imply currency — `grep -rn
"v0\.3\|tool_surface_version" --include="*.{go,md}"` returns only
historical-context mentions in changelog godocs (correct) and the
runtime constant readback (correctly returns "0.4" now).
Parent: PLAN-1410 / TASK-1418.
* fix(mcp): correct schema-type-change disclosure + stale cmdhelp-walker description (TASK-1418 follow-up)
Address Codex round 2 P3 findings on PR #544:
## P3 — `cmd/pad/mcp.go` still described the retired leaf walker
The `pad mcp serve` command's Long description said "every leaf
command becomes an MCP tool, except the curated allow-list
exclusions" — that was true under v0.1 but the cmdhelp leaf
walker was retired in TASK-981 (PLAN-969's v0.2 rollout). The
v0.2/v0.3/v0.4 surface has always been the hand-curated catalog
of eight resource × action tools + pad_set_workspace.
Updated the Long description to:
- Name the v0.4 catalog explicitly.
- List the eight resource × action tools.
- Note that cmdhelp v0.1 still drives per-command arg schemas
at dispatch time (so it's not gone, just no longer drives
tool naming/count).
- Reference TASK-981 for the cutover.
## P3 — "additive/subtractive only" was misleading
The compatibility note in `version.go` and `CLAUDE.md` claimed
all v0.4 changes were additive or subtractive. That glossed over
one breaking change in TASK-1412: `collections[].schema` went
from a JSON-encoded string ("schema":"{\"fields\":...}") to a
nested JSON object ("schema":{"fields":...}). For any v0.3
consumer that read schema as a string and JSON.parse()'d it
themselves, that's a TYPE change, not a no-op.
Updated both godoc and CLAUDE.md to explicitly call this out
as the one breaking change, separately from the additive/
subtractive bucket. Better for downstream MCP consumers to see
the truth than to discover it via runtime failure.
The remaining v0.4 changes ARE additive (overflow counts on
BootstrapDashboard) or subtractive (dropped fields with named
canonical alternatives) — those parts of the original note
are accurate and kept.
Honesty about compatibility is more valuable than a tidy
narrative. Surfaced explicitly in the godoc + the public
contract doc; PLAN-1410's Result section was already honest
about the field-level deltas.
Parent: PLAN-1410 / TASK-1418.
|
||
|
xarmian
|
d18a5d8140 |
refactor(bootstrap): omit redundant schema labels + omitempty sort_order (TASK-1424) (#543)
Two small additive trims to the BootstrapCollection projection
introduced in TASK-1412.
## 1. Omit redundant schema `label` when label == TitleCase(key)
A schema field's `label` is auto-fillable from its `key` (the CLI
and the MCP-side CreateCollection helper both apply
TitleCase(key) when label is empty — see titleCaseLabel in
internal/mcp/dispatch_http_routes.go). When the persisted label
matches that rule, it's redundant — the agent can reconstruct it
from the key. Examples on docapp:
- {"key":"status", "label":"Status"} ← redundant
- {"key":"due_date", "label":"Due Date"} ← redundant
- {"key":"trigger", "label":"When"} ← CUSTOM, preserved
Implementation: projectBootstrapCollection now passes the schema
bytes through trimRedundantSchemaLabels, which:
1. Unmarshals into a parallel bootstrapSchema/bootstrapFieldDef
struct purpose-built for the bootstrap shape.
2. Walks fields, clears any Label that equals TitleCase(Key).
3. Re-marshals — `omitempty` on bootstrapFieldDef.Label drops
the empty-string labels from the output.
Field ordering is preserved by struct-based marshalling.
## 2. Omitempty on BootstrapCollection.SortOrder
`sort_order` defaults to 0. Most collections never get an explicit
non-zero sort_order, so the field carried "sort_order":0 per entry
needlessly. Added ,omitempty to the struct tag.
## Drift detection
bootstrapFieldDef mirrors models.FieldDef field-for-field with two
deliberate differences: `Label` is omitempty, and `Default` is
json.RawMessage (so any default value round-trips verbatim
without re-parsing). The risk: if models.FieldDef gains a new
field, bootstrapFieldDef silently loses it from the bootstrap
schema response.
TestBootstrapFieldDefMirrorsModelsFieldDef catches this via
reflection — compares NumField + per-field JSON tags + has an
explicit allow-list for the Label tag delta. A new field added to
models.FieldDef without mirroring here fails the test with a
field-name-pointed error message.
## Test coverage
- TestBootstrapFieldDefMirrorsModelsFieldDef — drift detector.
- TestTrimRedundantSchemaLabels (4 subtests):
* drops-redundant-labels
* preserves-custom-labels (key="trigger", label="When" stays)
* multi-word-keys-titlecase-correctly (due_date → Due Date)
* malformed-schema-returns-raw (defensive: never block on parse error)
- Existing TestBootstrapSizeBudget shows fixture collections
section drop: 3,979 → 3,532 bytes (-11.2%).
## Measurements
Fixture (TestBootstrapSizeBudget): 7,823 → 7,376 bytes (-447 b / -5.7%).
Collections section alone: 3,979 → 3,532 bytes (-447 b / -11.2%) —
all of the win is concentrated in schema bytes via the label trim.
Live docapp expected savings: 9,384 → ~8,400 bytes (~10% drop on
collections), totaling roughly 600-900 b additional reduction on
the bootstrap response.
## Out of scope
- ToolSurfaceVersion 0.3 → 0.4 bump — TASK-1418 (the FINAL PR in
PLAN-1410). This is the last shape PR; TASK-1418 is now unblocked.
Parent: PLAN-1410. With this merged, PLAN-1410's bootstrap-shape
work is complete; only the contractual v0.4 announcement remains.
|