mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-11 21:39:01 +00:00
4bbd0a210dbc714da06df5265ac3d03e91a37648
142 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
4bbd0a210d |
feat(collections): widen LibraryPlaybook with InvocationSlug + Arguments (TASK-1398) (#527)
* feat(collections): widen LibraryPlaybook with InvocationSlug + Arguments (TASK-1398) Adds two optional fields to LibraryPlaybook: - InvocationSlug — kebab-case slug for `/pad <slug>` routing (PLAN-1377) - Arguments — argument spec mirroring the body's `## Arguments` section Both fields are tagged with `omitempty` so existing library entries (none of which set them) serialize unchanged. seedPlaybookFromLibrary() now forwards both into the seeded item's Fields JSON only when set, matching the shape ShipPlaybook() already writes. This is the foundational task that unblocks T2 through T6 of the playbook library overhaul. Parent: PLAN-1397. * fix(library): propagate invocation_slug + arguments through activation paths per Codex review (round 1) Codex round 1 caught that the activation paths for library playbooks rebuild the fields map and drop the new fields, so any library entry declaring invocation_slug/arguments would lose `/pad <slug>` routing after activation. Fixed in three places: - internal/cli/client.go LibraryPlaybook (the client-side mirror used by the CLI `pad library activate` command) - cmd/pad/main.go libraryActivate (CLI subprocess activation) - internal/mcp/dispatch_http_slice4.go dispatchLibraryActivate (MCP pad_project action=library-activate) All three now forward invocation_slug and arguments only when set, matching ShipPlaybook()'s shape exactly. Web client (web/src/lib/api/client.ts) activation payload is T2's explicit scope — left for that PR. |
||
|
xarmian
|
3508a83307 |
fix(cli): reject NaN/Inf in --field number parsing per Codex review (round 1)
strconv.ParseFloat accepts "NaN", "+Inf", "-Inf" as valid float64 values, but encoding/json cannot marshal those. The downstream json.Marshal(fields) errors at cmd/pad/main.go createCmd / updateCmd are intentionally ignored (`fieldsJSON, _ := json.Marshal(fields)`), so a single malformed --field input would silently drop the entire fields payload instead of rejecting. Reject non-finite floats in parseFieldFlag and fall back to the raw string; the server validator then returns the useful "field X must be a number" error. Verified: pad item update BLOG-1393 --field reading_time=NaN → "must be a number" ✓ pad item update BLOG-1393 --field reading_time=Inf → "must be a number" ✓ pad item update BLOG-1393 --field reading_time=4 → stored as 4 ✓ |
||
|
xarmian
|
c2014fa7f8 |
fix(cli): schema-aware --field parsing for non-string typed fields (BUG-1125)
pad item create/update --field key=value previously stored every value
as a string, so json / number / checkbox / multi_select fields were
rejected by the server-side validator. The new parseFieldFlag helper
fetches the collection schema once per command and parses each value
according to its declared field type:
- json / multi_select → json.Unmarshal
- number → strconv.ParseFloat
- checkbox → strconv.ParseBool
- text / url / select / date / relation / unknown → raw string
Schema-fetch failure degrades gracefully to pre-fix string-only behavior.
pad item list --field is unchanged (URL query param, not validator).
Verified against both repros: --field reading_time=3 on blogs (the
original number case) and --field 'arguments=[{...}]' on playbooks (the
json case that surfaced authoring the ship playbook). String fields show
no regression.
Skill update folded in: the "Authoring slug-invocable playbooks with
arguments" section in skills/pad/SKILL.md previously routed users to the
web editor as the only path for structured arguments. With this fix the
CLI handles it in one command, so the section now leads with the CLI
flow and demotes the web editor to an alternative.
|
||
|
xarmian
|
9607139340 |
feat(mcp): add pad_playbook tool (list/get/run) (TASK-1381) (#521)
* feat(mcp): add pad_playbook tool (list/get/run) (TASK-1381)
PLAN-1377 T4 — exposes the playbook surface from TASK-1382 via MCP.
Three passThrough actions match the CLI:
- pad_playbook.list → pad playbook list (metadata catalog)
- pad_playbook.get → pad playbook show <ref> (full body + fields)
- pad_playbook.run → pad playbook run <ref> (parse + bind args,
return body. Side-effect-free; the agent
executes the steps, not the server)
Params advertised in the tool schema: ref (required for get/run),
args (pre-parsed map — MCP / programmatic callers), and raw_args
(CLI-style tokens — strict parsing rules applied server-side via
ParsePlaybookCLIArgs from TASK-1382).
HTTP dispatcher route entries (dispatch_http_routes.go) wire the same
three actions into pad-cloud's in-process path:
GET /workspaces/{ws}/playbooks
GET /workspaces/{ws}/playbooks/{ref}
POST /workspaces/{ws}/playbooks/{ref}/run
The run mapper (mapPlaybookRun) JSON-encodes args + raw_args into the
POST body so the server-side parser fires with the same shape it gets
from the CLI.
ToolSurfaceVersion already 0.3 (TASK-1380). This adds a tool but
existing actions are unchanged, so no further bump is needed.
Tests: catalog_readonly_test.go's bijection and dispatch tests
extended with pad_playbook entries (both `expected` maps + the
liveCmdhelpDoc stub). The actions-match-cmdhelp + dispatch-cmdpath
checks pass.
Parent: PLAN-1377.
* fix(mcp): align pad_playbook MCP shape with CLI cmdhelp (TASK-1381)
Codex round 1:
P1 — Renamed CLI Use strings from `show <slug|ref>` / `run <slug|ref> ...`
to plain `show <ref>` / `run <ref> [args...]`. The pipe-alternation
form makes cmdhelp synthesize the arg name as "value"; local stdio MCP
calls were failing with missing "value" because the tool param is
"ref". The Long descriptions still explain the resolver accepts
invocation_slug / item slug / issue ref.
P1 — pad_playbook.action=run is now a custom action handler that
flattens the structured `args` map + `raw_args` slice into the CLI's
positional/flag/kv token sequence before dispatching. Without this the
passThrough path dropped args/raw_args (they aren't cmdhelp args/flags),
making the local-stdio invocation a no-op from the agent's POV. Sort
order is deterministic for test replay stability.
P2 — raw_args type changed from "array" to "array<string>" so the
catalog builder's paramDefToToolOption recognizes it as a string array
instead of falling through to the WithString default. Without this MCP
advertised a string but the mapper expected a slice.
Test cmdhelp stub updated: playbook run now declares "ref" + variadic
"args" positionals, matching the new Use string.
Parent: PLAN-1377.
* fix(mcp): mapPlaybookRun accepts both flattened + map args (TASK-1381)
Codex round 2 HIGH: actionPlaybookRun's flattened input shape
({args: []string}) confused mapPlaybookRun, which expected args as a
map. Cloud/HTTP MCP calls were posting {"args":["PLAN-7"]} and the
server tried to decode that as map[string]any.
mapPlaybookRun now coerces all the shapes both dispatch paths produce:
- args as map → forwarded verbatim as the pre-parsed dictionary.
- args as []string / []any → treated as raw_args (CLI tokens).
- raw_args (any case form) → appended to the raw_args list.
This keeps env.Dispatch the single dispatch entry point on the action
side while letting the HTTP mapper translate freely.
Parent: PLAN-1377.
* fix(cli): use [args]... ellipsis-outside-brackets so cmdhelp parses arg name (TASK-1381)
Codex round 3 HIGH: cmdhelp's argRE bakes the ellipsis into the arg
NAME when it appears inside the brackets — `[args...]` parses as
Arg{Name: "args...", Repeatable: false}, not Arg{Name: "args",
Repeatable: true}. That made BuildCLIArgs (used by ExecDispatcher
for local stdio MCP) drop the playbook argument tokens because the
input map key "args" didn't match the cmdhelp positional name
"args...".
The fix is to move the ellipsis OUTSIDE the brackets per the cmdhelp
spec: `[args]...`. Code comment cross-references cmdhelp/json.go::argRE
so future Use-string editors don't regress.
Parent: PLAN-1377.
* fix(mcp): preserve structured args on HTTP dispatch path (TASK-1381)
Codex round 4 MEDIUM: actionPlaybookRun's flatten step dropped explicit
`false` values on flag-typed args, so an MCP call like
{args: {stop-after-each: false}} couldn't override a flag default of
true.
Fix: dispatcher-type-aware branching.
- HTTPHandlerDispatcher path: forward input as-is. mapPlaybookRun
preserves the structured args map (including explicit false values),
and the server's bindPlaybookArgs sees the override correctly.
- ExecDispatcher path: flatten args + raw_args into CLI tokens as
before. The CLI's strict parser only supports bareword flag
PRESENCE, so the flag=false override is a documented local-stdio
limitation; route through HTTP/in-process MCP for that rare case.
Function docstring now spells out the two paths and the CLI
limitation so the next reader doesn't have to reverse-engineer it.
Parent: PLAN-1377.
* fix(mcp): bypass BuildCLIArgs on HTTP playbook-run dispatch (TASK-1381)
Codex round 5: even with the dispatcher-type branch from round 4,
env.Dispatch still ran BuildCLIArgs FIRST and only forwarded to the
chosen dispatcher AFTER. BuildCLIArgs choked on args:map (the cmdhelp
positional 'args' wants strings) and returned a validation_failed
result before mapPlaybookRun ever saw the structured input.
Fix: when dispatching to HTTPHandlerDispatcher, attach the input map
to context manually via WithDispatchInput and call the dispatcher
directly, skipping BuildCLIArgs. ExecDispatcher path unchanged — it
still uses env.Dispatch with the flattened CLI tokens because the
local CLI needs them.
Parent: PLAN-1377.
* fix(mcp): use structured validation envelope for missing ref (TASK-1381)
Codex round 6 P3: actionPlaybookRun's missing-ref error returned a
plain text result, breaking the structured-envelope contract that
every other validation error in the catalog follows. Switch to
NewErrorResult/ErrorPayload so agents can branch on error.code.
Codex's round-6 P2 (read-scope tokens blocked from POST /playbooks/{ref}/run
because the middleware requires GET/HEAD/OPTIONS) is real but
out-of-scope for this PR — it touches the auth scope model and
deserves a dedicated HT item rather than a snap fix here. Filed as
follow-up. The action is still functional for any token with 'write'
scope, which is the default for local-stdio MCP and pad-cloud
deployments.
Parent: PLAN-1377.
|
||
|
xarmian
|
fed4b60e65 |
feat(playbook): add pad playbook CLI (list/show/run) + endpoints (TASK-1382) (#520)
* feat(playbook): add pad playbook CLI (list/show/run) + endpoints (TASK-1382)
PLAN-1377 T5 — first-class invokable-procedure surface. Three HTTP
endpoints + three CLI subcommands, with a strict CLI arg parser and a
side-effect-free run path.
Endpoints
GET /workspaces/{ws}/playbooks — metadata array, same
projection as bootstrap
GET /workspaces/{ws}/playbooks/{ref} — full item, resolved by
invocation_slug | ref | slug
POST /workspaces/{ws}/playbooks/{ref}/run — bind args, return body +
bound + unbound. No
execution; the agent
owns step playback.
Resolution
resolvePlaybook walks invocation_slug first (the /pad <slug>
user-facing identifier), then falls back to ResolveItem (UUID / ref /
slug). A stray TASK-5 hitting /playbooks/TASK-5 returns 404 instead
of leaking a non-playbook into the surface.
Arg parsing
ParsePlaybookCLIArgs implements the strict rules from PLAN-1377:
- Required positional args first, in declared order.
- Flag types: bareword presence sets true.
- Other types: key=value form (number is parsed as float64,
enum is validated against declared options).
The server takes either pre-parsed args (MCP / programmatic callers)
OR raw CLI tokens (CLI caller); merge logic prefers explicit args
over raw_args. The CLI sends raw_args, so there is one parser
implementation and no drift risk.
CLI
pad playbook list — table or json
pad playbook show <slug|ref> [--format] — markdown / json
pad playbook run <slug|ref> [args...] — body + bound args
Client method
cli.Client.{ListPlaybooks, ShowPlaybook, RunPlaybook(args,
rawArgs)} — runtime callers pass args; CLI passes rawArgs.
Tests
TestPlaybookList, TestPlaybookShowByInvocationSlug,
TestPlaybookShowByRef, TestPlaybookShowRejectsNonPlaybook,
TestPlaybookRunBindsArgs (with-args, missing-required, with-raw-args),
TestParsePlaybookCLIArgsErrors, TestPlaybookListEmptyShape.
Parent: PLAN-1377.
* fix(playbook): tighten arg parsing per Codex round 1 (TASK-1382)
P2.1 — Positional binding now skips optional and flag-typed slots.
The PLAN-1377 contract says ONLY required args fill positional slots;
other typed args must be key=value. Without this, a spec with an
optional arg before a required one (e.g.
[merge-strategy?, target!]) bound the caller's bareword TASK-7 to
merge-strategy instead of target.
P2.2 — number coercion now uses strconv.ParseFloat with NaN/Inf
rejection. Sscanf(%g) was sloppy: it accepted '1abc' as 1 (partial
match) and accepted NaN/Inf, which json.Marshal then refused after
the handler had already written a 200 header.
P3 — empty-body run requests now decode cleanly. The decodeJSON
wrapper folds io.EOF into 'invalid JSON: EOF' so the previous
err.Error() != "EOF" check never fired. errors.Is(err, io.EOF) on
the unwrapped chain handles the wrapping correctly.
Tests: TestPlaybookRunAcceptsEmptyBody,
TestParsePlaybookCLIArgsOptionalNotPositional,
TestCoercePlaybookValueNumberRejectsBadInput.
Parent: PLAN-1377.
|
||
|
xarmian
|
73208bf9d3 |
feat(mcp): expose AgentBootstrap via three MCP surfaces (TASK-1380) (#519)
* feat(mcp): expose AgentBootstrap via three MCP surfaces (TASK-1380)
PLAN-1377 T3: Expose the bootstrap blob (from TASK-1379) via the three
MCP surfaces the agent specs name. One canonical builder
(Server.BuildAgentBootstrap), three discovery paths.
Surfaces
1. Resource: pad://workspace/{ws}/bootstrap. Hosts that prefetch
resources at session start (Claude Desktop, Cursor) get full
context cheap. readBootstrap shells out to `pad bootstrap`.
2. Tool action: pad_meta.action=bootstrap. Mid-session refresh for
agents that didn't get the resource prefetch or want a fresh
snapshot after lots of mutations. Pass-through to `pad bootstrap`
via env.Dispatch — same source of truth.
3. pad_set_workspace response embed: when a BootstrapFetcher is wired
in (production has one via ExecBootstrapFetcher), the response
payload extends from {workspace, status} to {workspace, status,
bootstrap}. One call hands the agent full session context the
moment they switch workspaces. Purely additive — older clients
that ignore unknown keys keep working.
Plumbing
- New BootstrapFetcher interface + ExecBootstrapFetcher impl that
shells out to `pad bootstrap --workspace <ws> --format json` with
RootArgs (e.g. --url) preserved.
- RegistryOptions.BootstrapFetcher (optional) — cmd/pad/mcp.go wires
ExecBootstrapFetcher; tests pass nil and get legacy shape.
- pad_meta.Schema.Workspace flipped to true so the workspace param is
available to the bootstrap action. server-info / version /
tool-surface ignore it as before.
- ToolSurfaceVersion bumped 0.2 → 0.3 with detailed changelog in the
const doc-comment. Bumps are additive but rename pad_set_workspace's
response shape, which is a breaking contract change for any client
that asserts the exact key set.
Tests
- TestSetWorkspaceTool_EmbedsBootstrap, _BootstrapErrorFallsThrough,
_EmptyWorkspaceSkipsBootstrap.
- TestPadMetaTool_NoWorkspaceInSchema rewritten as
TestPadMetaTool_WorkspaceInSchema with reasoning.
- TestCatalogWorkspaceParamAdvertisedOnAllWorkspaceTools updated:
pad_meta is no longer on the intentionallyServerWide list.
Parent: PLAN-1377.
* fix(mcp): wire bootstrap route into HTTP dispatcher (TASK-1380)
Codex round 1: pad_meta.action=bootstrap dispatched cmdPath
['bootstrap'] but the HTTP MCP dispatcher's routeTable had no entry,
so pad-cloud and other HTTP-transport clients hit the 'not yet
implemented over HTTP transport' fallback. Add a route mapping that
GETs /api/v1/workspaces/{workspace}/agent/bootstrap — the canonical
endpoint Server.handleGetBootstrap exposes. Local stdio MCP is
unaffected (it dispatches via ExecDispatcher, which shells out to
`pad bootstrap`).
Parent: PLAN-1377.
|
||
|
xarmian
|
24f0445efa |
feat(bootstrap): single-roundtrip /pad context-load endpoint (TASK-1379) (#518)
* feat(bootstrap): single-roundtrip /pad context-load endpoint (TASK-1379)
Implements the agent bootstrap surface for PLAN-1377. One HTTP call
replaces the four separate /pad context-loading invocations (workspace
+ collections + conventions + roles + playbooks) the skill used to
make, cutting ~200-400ms of startup latency on every /pad command.
Wire shape:
- GET /api/v1/workspaces/{ws}/agent/bootstrap returns AgentBootstrap:
workspace { slug, name, id }, user { name, email, id },
collections [...], conventions [...always-on, status=active],
roles [...], playbooks [metadata-only — no bodies], dashboard {...},
recent_activity [... 24h].
- pad bootstrap [--format json|markdown] CLI wrapper. JSON is the
canonical wire format that the /pad skill consumes; markdown is a
human-readable summary for quick terminal inspection.
Implementation notes:
- Single source of truth: Server.BuildAgentBootstrap. The HTTP handler
is a thin wrapper, and TASK-1380 will reuse it from three MCP
surfaces (resource + set_workspace embed + pad_meta tool action).
- Dashboard reuse: handleGetDashboard's body extracted into
buildDashboardResponse(workspaceID, r) returning (*DashboardResponse,
error). The HTTP handler is now a thin wrapper; bootstrap calls the
builder directly to embed dashboard data without a second roundtrip.
- Playbook bodies are deliberately NOT included — metadata only
(~80 bytes/entry vs 5-10KB) so the bootstrap stays small for
workspaces with many playbooks. Full bodies load on invocation.
- Convention bodies ARE included for the always-on/active subset (must
be agent-known up front); trigger-specific conventions stay
load-on-demand.
- Empty slices serialize as [] not null so the agent doesn't need
defensive nil checks.
Tests:
- TestBootstrapEmptyWorkspace, TestBootstrapEmptyArraysNotNull,
TestBootstrapIncludesPlaybookMetadata,
TestPlaybookSummaryPrefersFirstParagraph.
Parent: PLAN-1377.
* fix(bootstrap): respect collection visibility + guest grants (TASK-1379)
Codex round 1: BuildAgentBootstrap was bypassing visibility filters,
so a guest admitted by RequireWorkspaceAccess could read collections,
conventions, and playbook metadata they don't have access to.
Now mirrors handleListCollections + handleGetDashboard:
1. Resolve visibleCollectionIDs(r, workspaceID) once.
2. Filter the Collections array through isCollectionVisible.
3. Gate the conventions + playbooks sub-queries on whether the caller
can see those collections at all (presence in the filtered slice
implies visibility).
4. Empty slices for inaccessible sub-resources, so the wire shape
stays consistent — no missing keys to confuse the agent skill.
5. Pass-r=nil callers (future MCP in-process dispatchers) keep the
'full visibility' shortcut, but the doc comment now spells out that
those callers MUST verify access out-of-band.
Parent: PLAN-1377.
* fix(bootstrap): apply guest item-level grants + recompute role counts (TASK-1379)
Codex round 2:
P1 — Item-level guest grants now flow into the convention + playbook
sub-queries. visibleCollectionIDs alone admits the conventions /
playbooks collection if a guest has ANY item grant inside it; without
ItemIDs filtering, those queries then return the whole always-on
convention body set or every playbook's metadata. Now mirrors the
handleListItems shape: resolve (fullCollIDs, grantedItemIDs) via
guestResourceFilter, and pass the (collIDs, itemIDs) tuple through to
collectAlwaysOnConventions + collectPlaybookMetadata so a guest with a
single grant only sees that one item.
P2 — AgentRole.ItemCount is now recomputed from the visible item set
for restricted callers, matching handleListAgentRoles. Without this,
a guest could read role counts computed across all workspace items and
infer hidden activity per role.
Helper signatures updated to accept (collIDs, itemIDs); the doc
comments explain the nil/non-nil semantics so future callers can't
silently regress this.
Parent: PLAN-1377.
* fix(bootstrap): rewrite collection counts from visible set for guests (TASK-1379)
Codex round 3: Collection.item_count + active_item_count are computed
across the whole collection by ListCollections, so a guest with one
item grant in a collection still received the collection in the
filtered list but with hidden counts. Reuse the visible item set
(already computed for role counts) to recompute collection item_count
for restricted callers. active_item_count is set equal to item_count
to avoid a separate done-rules buildup the bootstrap consumers don't
depend on — better a self-consistent number than a leaked one.
Parent: PLAN-1377.
|
||
|
xarmian
|
25a21184a2 |
feat(cli): add --schema flag to pad collection create (TASK-1334) (#482)
* feat(cli): add --schema flag to pad collection create (TASK-1334) The existing --fields DSL (key:type[:options]) had no syntax for terminal_options, default, required, computed, suffix, or relation collection — every CLI-created collection lost those FieldDef properties even though the model already supports them. Symptom from BUG-1284: dashboard "active" counts treat published/archived items as in-progress because the persisted schema has no terminal_options. Adds a new --schema flag that accepts the full CollectionSchema JSON, which captures every current and future FieldDef property automatically. Three input modes: --schema '<json>' inline literal (agent-natural; CLI is agent-first) --schema @./path.json file path --schema - stdin --fields and --schema are mutually exclusive; --fields keeps working unchanged for backward compat (no deprecation). Refactors the inline parser into three testable helpers in main.go: collectionSchemaJSONFromFlags (orchestrator), readSchemaInputBytes (input resolver), and parseFieldsDSL (legacy DSL parser preserving the "first status select gets required+default" heuristic). Tests: 9 table-style cases in collection_create_schema_test.go covering all three input modes, the mutually-exclusive guard, malformed JSON, missing file, fallthrough-to-DSL, both-empty, and a regression test that verifies terminal_options + computed + suffix + relation.collection all round-trip through --schema. Parent: PLAN-1333. * fix(cli): backfill missing labels in --schema fields per Codex review (round 2) Codex flagged that the --schema example omitted "label", which the parser preserved as label:"" — agents constructing JSON could create collections that render blank field headers in the web UI. Fix: after unmarshaling --schema input, backfill any FieldDef with an empty Label using the same Title-Case-of-key heuristic the legacy --fields DSL applies (e.g. "due_date" → "Due Date"). Explicit labels are preserved. Also updated the help-text example to include "label" on the status field so the canonical shape is visible, plus a tip line documenting the auto-fill behavior so users know it's safe to omit labels. Test: TestCollectionSchemaJSONFromFlags_BackfillsMissingLabels covers auto-fill, multi-word key normalization, and the explicit-label-not- clobbered case. Parent: PLAN-1333 / TASK-1334. |
||
|
xarmian
|
028db39217 |
feat(collab): periodic op-log GC sweeper for dormant items (TASK-1309) (#471)
The Yjs collab dumb-relay accumulates op-log rows indefinitely in item_yjs_updates. DOC-1307 surfaced 45-second p50 cold-reconnect latency on a single item with 5000 accumulated rows. Without GC, busy items keep growing. This adds a periodic background sweeper that prunes the entire op-log for items that are both DORMANT (no recent activity) AND FULLY FLUSHED (items.content has captured every op-log row). Whole-log only — Yjs op streams are causally linked, prefix-pruning corrupts replay; future cold connects lazy-seed from items.content. Components: - Store.ListDormantOpLogItemsBefore (joins items, filters watermark) - Store.PruneItemOpLogIfDormantBefore (atomic conditional DELETE) - Store.GetItemContentFlushedOpLogID (per-item watermark getter) - RoomManager.PruneSweep (per-item-locked, active-room-skip) - Server.StartOpLogGC / stopOpLogGC (mirrors orphan_gc.go pattern) - cmd/pad/main.go env vars PAD_OPLOG_GC_INTERVAL / PAD_OPLOG_GC_MIN_AGE - New (item_id, created_at) index for the dormancy query - New items.content_flushed_op_log_id column (id-based watermark, monotonic, no clock-skew or second-granularity false positives) + content_flushed_at (informational timestamp) Watermark policy: - Server-driven full-content writes (CLI / MCP / version restore / PruneAndApply) advance content_flushed_op_log_id to MAX(op-log.id) via subquery, atomic with the content UPDATE - Browser collab-snapshot 5s flushes do NOT advance the watermark — they can't prove their markdown captured every peer's ops, so letting them stamp would risk later GC-pruning unsynced peer edits - Schema-mismatch rebuild (TASK-1268) logs a WARN when it drops unflushed ops (data loss is unavoidable on schema bumps but visible) Stop ordering: collab.Close() now runs BEFORE bg.Wait() so a GC goroutine waiting on an itemLock behind an active Join can drain. Migration backfill: items WITH existing op-log rows keep NULL watermark (don't certify); items WITHOUT op-log rows get a synthetic 0 watermark (vacuous, harmless — no rows to compare against). Tests: - 6 RoomManager.PruneSweep tests (dormant prune / default minAge / empty / bails-on-Close / skips-active-room / skips-row-added-mid- sweep via fakeOpLog hook) - 5 Server.OpLogGC tests (prunes-dormant / start-idempotent / preserves-unflushed / backfill-doesnt-certify-unflushed / no-collab-noop) - TestCollabSnapshotDoesNotAdvanceOpLogWatermark in store - TestCollabSnapshotQueryOverridesBodyVersionSource in server (regression for body-attacker bypass) Seven rounds of Codex review — caught 5 P1s and 4 P2s I would have shipped under self-review: 1. Prefix-prune corrupts Yjs replay 2. Stop ordering deadlock 3. Missing index 4. Best-effort flush ⇒ data loss 5. Backfill over-certifies via metadata-PATCH 6. Second-granularity timestamp comparison 7. Schema-mismatch path drops unflushed silently 8. Browser flush stamps watermark beyond Y.Doc 9. Body version_source bypasses server policy |
||
|
xarmian
|
66aa6f5197 |
fix(loadtest): codex catch-up review fixes (TASK-1270) (#470)
PR #468 shipped without a codex review because codex was unavailable that day. This is the catch-up; codex found 2 P2s and 5 NITs. [P2] readSendTimestamp recorded latencies for prior-run replay frames. The original guard only checked nonzero ts, not session recency, so any stale op-log row inflated p95/p99. Fixed: runContext captures startedAt; recv path filters frames whose embedded ts predates this run. Verified: against an item with 90 stale rows the test counts received frames (630) but only records latencies for the 180 live ones. [P2] Shutdown deadlock. The writer goroutine could be blocked in conn.WriteMessage when rc.done closed; the only path to conn.Close was that same goroutine's select-case, so wg.Wait() could hang forever under server backpressure. Fixed: per-client watchdog goroutine closes the conn from outside the writer when done fires, plus a 5s SetWriteDeadline per send as defence-in-depth. A 5s duration test now exits in exactly 5.008s. NITs (also fixed): - buildFrame docstring corrected (minimum is 16 metadata bytes, buffer is frameBytes+1). - buildFrame returns (bytes, error) instead of log.Fatalf-ing on rand.Read; caller logs detail and increments errors counter. - -cookie / -token flag help now states both can be set together. - Watchdog-induced WriteMessage errors are no longer counted as real errors (isClosedDone check). - buildFrame error path now logs the actual error detail. Two rounds of codex review: round 1 found the items above; round 2 returned CLEAN. |
||
|
xarmian
|
287fa545fb |
feat(loadtest): add cmd/loadtest-collab + DOC-1307 findings (TASK-1270) (#468)
Synthetic Go load-test for the Yjs collab dumb-relay. Each simulated client opens a WebSocket, sends tagged sync frames at a configurable rate, consumes inbound frames, and computes broadcast fanout latency. Doesn't depend on a real Yjs port — the dumb-relay's first-byte discriminator (yMessageSync=0) is enough to exercise the persist + broadcast path with synthetic payloads. Each frame embeds a unix-nano timestamp + client ID so receivers can compute round- trip latency without out-of-band coordination. Findings (in DOC-1307): - N=5, N=25: clean, p95 < 10ms, fanout matches expected (N-1)x - N=100: 38/100 dial failures (consistent), but the 62 successful see p95=27ms — server rejects ~38% of simultaneous dials at this level. Filed BUG-1308 to investigate the ceiling. - Op-log grows unbounded without compaction; old runs replay on reconnect causing latency blow-up. Filed TASK-1309 to wire a periodic prune sweeper. Self-reviewed only; codex was unresponsive today after multiple hour-long retries. |
||
|
xarmian
|
e7b1c3b5ae |
feat(collab): per-item Room manager with op-log replay + grace TTL (TASK-1255) (#453)
* feat(collab): per-item Room manager with op-log replay + grace TTL (TASK-1255)
Wires the OpBus + op-log + WS handler from prior phase-1 PRs into a
working dumb-relay collab server. Per-item Room created lazily on
first Join, kept alive across transient disconnects via a 60s grace
TTL, reclaimed when the grace expires with no fresh subscribers.
Components:
- internal/collab/room.go — Room struct + lifecycle
· roomConn pairs (id, conn, bus channel, write mutex). The id is
server-assigned per WS so writeLoop can suppress own-event echoes
without decoding the Y.Doc to read the Yjs ClientID.
· readLoop discriminates yMessageSync vs yMessageAwareness on
byte 0. Sync frames are persisted to the op-log AND broadcast;
awareness frames are broadcast only (presence is ephemeral).
Persistence happens BEFORE broadcast so a crash mid-publish loses
at most a live keystroke that the originator will replay on
reconnect anyway.
· writeLoop drains the bus subscription and writes non-self events
to the WS, gated by a per-conn write mutex (gorilla's "one writer
at a time" rule).
· removeConn arms a 60s graceTimer when the last conn drops; a
fresh addConn cancels the timer. onGraceExpired re-checks
len(conns) == 0 under the room mutex and only THEN sets
closing=true + calls back to the manager. The race between
"manager.getOrCreate found us" and "grace timer fired" is
handled by addConn returning errRoomClosing; the manager retries
via getOrCreate which mints a fresh Room.
- internal/collab/manager.go — RoomManager + RoomManagerConfig
· NewRoomManager wires production defaults (DefaultGraceTTL = 60s,
DefaultSchemaVersion = "1"). NewRoomManagerWithConfig accepts an
explicit config so tests can drop graceTTL to a few ms without
sleeping a minute. graceTTL is per-manager, not a package var,
so parallel tests with different TTLs don't trip the race
detector.
· Join is the public entry point: getOrCreate → addConn (with
retry on errRoomClosing) → replayTo → spawn writeLoop goroutine
→ run readLoop inline → wait for writeLoop drain → return. The
inline read keeps the HTTP handler in scope so its
`defer conn.Close()` doesn't fire until both loops exit.
· Close is for graceful server shutdown — closes every active
conn under the room mutex, then drains the manager's room map.
- internal/collab/manager_test.go — 7 tests covering: lazy create,
op-log replay-on-connect (two seed rows arrive in order), sync
broadcast + persist (peer B sees A's frame, originator does not
echo, op-log gains a row), awareness broadcast WITHOUT persist,
cross-item isolation (item-a frames don't leak to item-b
subscribers), grace-TTL reclaim with a 50ms config TTL, grace
cancel on reconnect within window, manager.Close shuts down
every active conn. All tests run with -race; the bus's
concurrent-publish test was already covered by TASK-1253.
- internal/server/handlers_collab.go — wire to RoomManager
· Returns 503 when s.collab is nil (matches the SSE handler's
"events bus not configured" 503 — fail loud rather than silently
accept the upgrade).
· Otherwise hands the upgraded conn to s.collab.Join, which
blocks until the WS closes. Unexpected close codes get the same
warn-log as before; normal closures stay quiet.
- internal/server/server.go — adds *collab.RoomManager field +
SetCollabRoomManager setter (nil-safe optional, like SetEventBus).
- cmd/pad/main.go — wires NewMemoryOpBus + NewRoomManager into
the running server alongside the event-bus wiring. Single-instance
only today; multi-replica fanout via Redis is a deferred IDEA per
the Plan body.
- internal/server/handlers_collab_test.go — adds
testServerWithCollab helper (so existing collab tests get a real
RoomManager) plus TestCollabUpgradeUnavailableWithoutRoomManager
which asserts the 503 path for unwired servers.
Parent: PLAN-1248. Phase 1 — Backend foundation.
* fix(collab): per-room appendMu + Server.Stop closes RoomManager per Codex review (round 1)
P1 — concurrent peers raced AppendYjsUpdate, violating the
single-writer-per-item contract documented on the store call. Each
peer's readLoop runs in its own goroutine, so two peers in the same
room could call AppendYjsUpdate concurrently. On Postgres that
risks the BIGSERIAL allocation-vs-commit-order cursor gap that
TASK-1252's contract was specifically guarding against. Add an
appendMu on Room held across the persist+publish sequence; reads,
awareness frames, and OTHER rooms remain unserialised.
Regression test (TestRoomManagerSerializesSyncAppends) drives 4
peers × 10 writes concurrently and asserts the op-log gains exactly
40 rows. Without appendMu this would intermittently surface fewer
rows or out-of-order ids on Postgres; with it the count is
deterministic and the race detector stays clean.
P2 — Server.Stop did not close s.collab. Active collab WS goroutines
+ grace timers could keep using s.store after the server's other
cleanup paths winding down. Add s.collab.Close() before
rateLimiters.Stop so any Join goroutines holding rate-limiter
handles can wind down cleanly. nil-safe via the existing collab
optional-attachment pattern.
* fix(collab): start writer before replay to avoid bus-overflow drops per Codex review (round 2)
P2: a joining peer subscribed to live events BEFORE its writer
goroutine started. During a long replay, live sync events would pile
up in the 64-event bus channel; once full, MemoryOpBus.Publish
silently drops them, leaving the new peer connected but permanently
missing those updates.
Restructure runConn to spawn the writer goroutine FIRST so it drains
the bus subscription concurrently with the replay. Both replay and
writer go through rc.writeMessage, which holds the per-conn write
mutex, so we never violate gorilla's one-writer-at-a-time rule.
Yjs CRDTs are commutative — applying live op 100 before replay op 50
yields the same final Y.Doc as the reverse order — so interleaving
is correct. The trade-off is a brief "out of causal order" UX wobble
during replay, which is acceptable: the alternative would require
either an unbounded queue or losing updates the way the original
order did.
* fix(pad): call srv.Stop() in serveCmd shutdown so collab sessions close per Codex review (round 3)
P2: serveCmd's SIGINT/SIGTERM path called srv.Shutdown but never
srv.Stop. http.Server.Shutdown does NOT terminate hijacked
connections (WebSockets), so active collab sessions kept running
until process exit and could race the deferred store close. The
RoomManager.Close path added in round 1 only fires inside Stop, so
without this call the production shutdown was effectively bypassing
the new cleanup.
Add srv.Stop() after srv.Shutdown in the serveCmd shutdown
sequence. Stop also runs the existing background-loop teardowns
(orphan GC, MCP audit writer, MCP session tracker) which were
previously already part of Stop's contract — those will continue to
fire as they always have, so this commit's only behavioural change
is "now also closes the collab room manager".
* fix(collab): WaitGroup drain barrier + bigger bus buffer per Codex review (round 3)
P1 — RoomManager.Close was not a true drain barrier. closeAll
closed the WebSockets but did NOT wait for the corresponding Join
goroutines (running runConn) to exit. Server.Stop returned before
in-flight collab work finished, racing the deferred store close
on process exit. Fix: track every Join in m.activeJoins
(sync.WaitGroup); Close iterates closeAll first (waking up every
reader by closing the conn), then activeJoins.Wait — guaranteeing
no collab goroutine is still running by the time Close returns.
P2 — replay-time bus overflow could still drop sync events on a
slow drain (writeLoop blocks on the same writeMu replayTo holds,
so a long replay starves the bus drain even with the writer
goroutine started before replay). Two-part response:
(a) Bump the per-subscriber bus channel buffer from 64 to 256.
Sized for a 5x safety margin on a 1k-row replay against a
chatty 5-peer room (~50 events/sec during a ~1s replay).
(b) The architectural fix — force-close subscribers on overflow,
honoring the bus's documented slow-peer recovery contract — is
filed as TASK-1273 follow-up. That requires extending the OpBus
interface (per-subscriber drop callback or counter) and an active
health-check tick in the room manager; both are out of scope for
TASK-1255's "lazy room + grace TTL" deliverable.
For PLAN-1248's single-instance scope and typical editor load,
256 covers realistic workloads. Pathological / load-test scenarios
exposing overflow can recover via Yjs's state-vector negotiation
on reconnect, and TASK-1273 will tighten that to an active kick.
* fix(collab): closed flag gates Join + Close idempotency per Codex review (round 4)
P2: http.Server.Shutdown does NOT wait for hijacked WebSocket
handlers, so a Join() call from a freshly-upgraded conn could fire
AFTER Close() returned. The previous Add-then-Wait pattern was
correct for already-started Joins but couldn't catch a Join that
hadn't yet hit Add when Close fired. Race: Close iterates the (empty)
rooms map, Wait sees zero waiters, Close returns; THEN Join hits
Add and proceeds against a torn-down store.
Add a `closed` flag gated by the same mutex that wraps
activeJoins.Add. Three orderings, all safe:
1. Add before Close.closed=true → Wait blocks until Done.
2. Close.closed=true before Add → Join sees closed=true under
the same lock and returns errManagerClosed without ever
incrementing the WaitGroup.
3. Close called twice → second call short-circuits (idempotent).
getOrCreate also gets a closed-flag short-circuit so a future
caller can't bypass the gate by skipping Join.
Test: TestRoomManagerJoinAfterCloseFailsFast asserts post-Close
Join returns errManagerClosed, plus a second Close() is a no-op.
All 15 collab tests pass under -race.
|
||
|
xarmian
|
d915cc3cf8 |
feat(cli): per-server credentials in credentials.json with v1 → v2 migration (TASK-1228) (#435)
Implements IDEA-1226. ~/.pad/credentials.json is now a map keyed by
server URL so one developer machine can stay logged in to multiple Pad
instances simultaneously — `apm/` repo on Pad Cloud, `target/` repo on
local, `testing/` repo on staging — without each `pad init --url <other>`
clobbering the previous server's credentials.
## On-disk format
v2 (new):
{
"version": 2,
"credentials": {
"https://app.getpad.dev": {"token": "...", "user_id": "...", ...},
"http://127.0.0.1:7777": {"token": "...", "user_id": "...", ...}
}
}
v1 (legacy, read-only): {"server_url": "...", "token": "...", "user_id": "...", ...}
Reads transparently migrate v1 → v2 in memory; writes always emit v2.
Side-effect-free reads — the on-disk file stays v1 until login/logout/
setup triggers a Save, which is when migration becomes durable. This
keeps `pad <read-only-command>` from rewriting credentials.json on
every invocation just because the binary upgraded.
## API
Replaces the three top-level helpers (LoadCredentials / SaveCredentials /
DeleteCredentials) with a CredentialStore type:
- LoadStore() (*CredentialStore, error)
- (s).Get(serverURL) *Credentials // nil-receiver safe
- (s).Set(serverURL, *Credentials)
- (s).Delete(serverURL)
- (s).Save() error
- WipeCredentialsFile() error // file-level — replaces DeleteCredentials
URL canonicalization is built in: trailing slash + surrounding whitespace
are stripped before lookup/store, so http://x:7777 and http://x:7777/
hit the same bucket. Same rule cmd/pad/server_info.go was already
applying via its now-redundant normalizeURL — removed.
No top-level `default` field. The configured server (cfg.BaseURL() from
~/.pad/config.toml or --url) is always the source of truth for "which
server am I targeting" — a separate `default` would create a second
source of truth and the split-brain bugs that follow.
## Behavioral changes
- `pad init --url <other>` against a server you've authed to before now
reuses the saved credential instead of clobbering it.
- `pad auth logout` removes only the configured server's entry. Other
servers' tokens stay intact (pre-fix: wiped the whole file).
- `pad auth whoami` reads only the entry matching the configured server.
- Single-server users see no behavior change — one entry, identical
shape per entry, identical UX.
## Compat shims removed
LoadCredentials / SaveCredentials / DeleteCredentials are deleted
outright (no // Deprecated lifecycle) — they're internal package
helpers with no external API contract. All 10 call sites in cmd/pad/
and internal/cli/ are migrated to the per-server API in this PR.
## Tests
internal/cli/credentials_test.go (15 tests):
- File missing / empty → empty store (callers don't need nil checks)
- v1 format reads + migrates in memory
- v1 with empty token → empty store (no phantom entries)
- v1 migration is durable on first Save (file flips to v2)
- v2 round-trip preserves multiple entries
- Set adds + replaces; mirrors URL into ServerURL field
- Delete keeps siblings (multi-server keystone behavior)
- Delete on absent key is a no-op
- Nil receiver Get/Delete don't panic (NewClientFromURL relies on this)
- URL normalization (trailing slash + whitespace)
- Save preserves all entries across the file boundary
- Save uses 0600 permissions
- WipeCredentialsFile removes the file + is idempotent
- Garbage file errors loudly (so we never silently lose data)
Existing tests unchanged. Full suite + lint + web-check green.
Closes: TASK-1228.
Implements: IDEA-1226.
|
||
|
xarmian
|
dfb67ae64b |
feat(init): browser-based admin setup in pad init via /setup#token (TASK-1217) (#433)
Wire `pad init`'s admin-creation step (Step 3) to use cli.RunBrowserBootstrap from TASK-1216 by default, with --cli-prompt preserving the legacy in-terminal email/name/password prompts. Workspace creation stays CLI — `pad init` is intrinsically directory-bound (.pad.toml write, cwd link), and that's what the browser flow can't do. Default flow on a fresh server in TTY: 1. Configure (existing) 2. Start server (existing) 3. NEW: print /setup#token=<x> deep link, poll until admin is created 4. NEW: chain doBrowserLogin so the CLI ends up authenticated 5. Workspace creation (existing template picker, .pad.toml write) 6. Skill files (existing) `pad init --cli-prompt` falls back to the pre-TASK-1217 path verbatim: promptAndBootstrap → saveCredentials → workspace creation. Same behavior as today for users with broken browser environments (headless box no SSH tunnel, broken X11, etc.). The flag is a zero-cost hedge per IDEA-1179 — we don't expect users to need it, but each invocation is a signal we should rethink. SIGINT during the polling loop is handled by installInitCancelHandler (top of the RunE) which calls os.Exit(130) directly — the helper doesn't need its own signal-aware ctx, so context.Background() is fine. Helper-call audit: promptAndBootstrap and readPassword are still reached via the --cli-prompt paths in both `pad auth setup` and `pad init`, plus readPassword serves doInteractiveLogin. All three keep their callers, so no helpers are removed in this PR. Both --cli-prompt paths exist by design as the IDEA-1179 hedge. Implements: IDEA-1179 (pad init half). Closes: TASK-1217. |
||
|
xarmian
|
51959532ad |
feat(auth): browser-based pad auth setup via /setup#token deep link (TASK-1216) (#432)
* feat(auth): browser-based pad auth setup via /setup#token deep link (TASK-1216)
`pad auth setup` now hands the operator a deep link into the browser-based
/setup form by default, replacing the in-terminal email/name/password
prompts. The browser flow gives them password-manager support, HTML5
email validation, and the live strength meter at zero CLI cost — the
mechanism (logs-token bootstrap, /setup route, /api/v1/auth/session) was
already shipped by TASK-1167 / PLAN-1166 for the Unraid use case. This
just unifies the local-CLI install path onto the same flow.
New `internal/cli/bootstrap.go::RunBrowserBootstrap`:
- Reads <DataDir>/.bootstrap-token and prints
`<BrowserURL>/setup#token=<TOKEN>` with the token in the URL fragment
(not query) — fragments are scrubbed from the address bar by /setup's
onMount before paint, so the secret doesn't survive in browser
history (TASK-1167 F10).
- Polls /api/v1/auth/session every 2s; returns nil when
setup_required: false. Internal 5-min timeout uses a separate timer
(not context.WithTimeout) so caller-ctx cancellation surfaces as
ctx.Err() instead of being misreported as the helper's own timeout.
- Idempotent: returns early if setup is already done, without touching
the token file.
- Dispatches on session.setup_method — "logs_token" reads the token,
"open" (PAD_BYPASS_SETUP_TOKEN=true) prints a bare /setup URL,
"local_cli" / unknown returns an error directing the user to
--cli-prompt.
`pad auth setup` is rewired to call the helper, then chain doBrowserLogin
so the user ends up authenticated on the CLI — preserving the post-
condition of the legacy --cli-prompt path. Two browser approvals (admin
creation, CLI auth) but each is one click in a browser the operator
already has open.
The legacy TTY path lives on behind --cli-prompt as a zero-cost hedge
per IDEA-1179. Existing promptAndBootstrap / readPassword helpers are
left in place — TASK-1217 will audit whether they can be removed once
pad init is on the new flow too.
Tests in internal/cli/bootstrap_test.go cover: idempotent session check,
logs_token happy path, open mode, missing/empty token error paths,
local_cli + unknown method rejection, internal timeout firing with the
friendly message, caller-ctx cancellation propagating ctx.Err() (not
timeout error). bootstrapPollInterval / bootstrapPollTimeout are vars so
the timeout-branch test can run in 100ms instead of 5min.
Implements: IDEA-1179 (auth-setup half).
Out of scope: pad init integration → TASK-1217.
Out of scope: post-/setup workspace dead-end → IDEA-1215.
* docs(cli): clarify RunBrowserBootstrap caller staging across TASK-1216 / TASK-1217
Codex review (round 1) read the docstring and flagged that `pad init`
isn't on the new helper. That wiring is TASK-1217's scope by design (one
task = one PR per CONVE-2; TASK-1217 has a hard blocked-by link to
TASK-1216). Tighten the docstring to make the staging explicit so a
reader of the diff alone doesn't conclude it's a missing wire-up.
|
||
|
xarmian
|
40352a32e1 |
feat(auth): PAD_BYPASS_SETUP_TOKEN open-bootstrap escape hatch (#429)
Adds an env-var that lets self-host operators on trusted networks (Unraid behind a firewall, Tailscale-only deployments, homelabs) claim the first admin via the web UI without copying a bootstrap token out of the container logs. Behavior when PAD_BYPASS_SETUP_TOKEN=true: - handleBootstrap accepts non-loopback first-admin POSTs without an X-Bootstrap-Token header. The UserCount==0 invariant is unchanged, so the bypass auto-closes the moment the first admin claims the seat (subsequent bootstrap requests get 409 regardless of bypass). - handleSessionCheck returns setup_method=open so the /setup page skips the paste-token UI and renders the form directly. - Token generation is skipped at startup (no .bootstrap-token file written). A distinct WARN-flavored banner makes the open-mode trade-off obvious in operator logs. - Cloud mode (PAD_CLOUD/PAD_MODE=cloud) ignores the flag entirely. Three layers of defense: cmd/pad masks the env-var with !cfg.IsCloudServer(), Server.openBootstrapEnabled() checks !s.cloudMode, and the cloud branch in handleBootstrap never reads the bypass field. Unraid template gets a new "Bypass Setup Token" field (default false, Display="always") with a description that calls out the trust-the- network trade-off. Tests pin all the security-critical contracts: bypass admits non- loopback, bypass off keeps existing 403, cloud mode hard-ignores, loopback works either way, post-bootstrap gate stays closed, bypass wins over logs_token in session payload, cloud mode never advertises 'open' setup method. Codex review: CLEAN (round 1). |
||
|
xarmian
|
693f03be3c |
fix(auth): emit first-run bootstrap banner to stderr (BUG-1182) (#428)
slog's text handler is contractually one-line-per-record and escapes literal newlines as `\n`, so the multi-line bootstrap banner rendered as a single wide line in `docker logs` — exactly the surface where operators look for the token. Switches the banner to fmt.Fprint to stderr (real newlines), with a companion slog.Info one-liner so structured-log aggregators still record the event. The companion log deliberately does NOT include the URL or token in its structured fields — those would be parseable as log-aggregator-extractable values, defeating the URL-fragment design (TASK-1167 F10) that keeps the token off-server. Operators / agents that want the token programmatically read the on-disk file at token_path. Verified locally: banner now renders the ASCII box with real newlines, token visible, companion slog line shows token_path without the URL. Caught by Dave during the v0.3.0-rc.1 smoke test on a real Unraid box. |
||
|
xarmian
|
05a9665f50 |
feat(auth): first-run logs-token bootstrap flow (TASK-1167) (#424)
One-time bootstrap token generated on first start with no users in self-host mode. Token is logged in a banner the operator can grab from `docker logs`, persists at <DataDir>/.bootstrap-token (mode 0600), and bypasses the loopback-only gate via the X-Bootstrap-Token header — letting the user claim the first admin from a remote browser at /setup#token=<x>. Header-only contract + URL-fragment (browser-only, never transmitted) + log-redaction middleware keeps the secret out of access logs, proxy logs, and browser history. Cloud mode unchanged: token never loaded, never honored. Validate → UserCount-check → CreateUser → consume sequence is mutex-serialized to prevent concurrent valid-token requests from creating multiple admins. Part of PLAN-1166 (Pad on Unraid — Community Apps launch). |
||
|
xarmian
|
63d113624c |
fix(cli): retry password prompt on weak/mismatched passwords (BUG-1155) (#413)
* fix(cli): retry password prompt on weak/mismatched passwords during admin bootstrap (BUG-1155)
`pad auth setup` and `pad init` collected admin credentials with a single-
shot prompt: any rejection — local password mismatch, or server-side weak-
password / length error from validatePasswordStrength — bubbled up and
exited the command. The user had to re-run the whole flow (and in `pad
init`, redo configure + server-start) over a typo.
Replaces promptForAccountDetails() with promptAndBootstrap(client) which
collects email + name once, then loops the password / confirm pair (up to
5 attempts) on:
- local password mismatch
- *cli.APIError from /auth/bootstrap (covers all three messages from
internal/server/password_strength.go: too short, too long, too weak)
Network failures and other non-API errors still bail immediately.
Both call sites — cmd/pad/main.go (auth setup) and cmd/pad/init.go (init
step 3) — now use the new helper.
* fix(cli): only retry password-strength rejections, not all API errors per Codex review (round 1)
Round 1 retried on every *cli.APIError from /auth/bootstrap, but only
password-strength rejections are fixable by re-prompting the password
pair. The server also emits validation_error for invalid email / missing
name, conflict ("Pad instance has already been initialized"), and
forbidden (non-loopback bootstrap) — re-prompting just the password for
those traps the user in a 5-attempt loop that can never succeed.
Narrows the retry gate to validation_error whose message begins with
"Password" — the three messages emitted by validatePasswordStrength
(internal/server/password_strength.go: too-short, too-long, too-weak).
All other APIError codes and message shapes now fall through to the
fail-fast branch, so the user sees the real reason and can re-run with
the right correction.
|
||
|
xarmian
|
abf017c4e7 |
feat(onboarding): make banner + CLI hint template-aware (TASK-1150) (#409)
The IDEA-1 trigger phrase is no longer hardcoded — fresh scrum
workspaces surface "use pad to get BACK-1", product workspaces surface
"use pad to get FEAT-1", and any future template that ships an
agent-onboarding seed declares its primary ref once and gets the
banner / hint for free.
Mechanism:
1. WorkspaceTemplate gains an OnboardingPrimaryRef string field —
the canonical declaration of "this template's IDEA-1-style
primary entry." Set per template that ships the pattern
(startup → "IDEA-1", scrum → "BACK-1", product → "FEAT-1");
left empty for hiring/interviewing/demo where the agent-onboarding
pattern intentionally doesn't apply.
2. Server: handleGetDashboard identifies the seeded primary by
walking allItems looking for item_number=1 + source="template"
+ created_by="system" + collection_slug ∈ {ideas, backlog,
features}. The collection-slug whitelist is what keeps hiring's
REQ-1 (also seeded with item_number=1 + source=template) from
being flagged as an onboarding entry — those are example items,
not agent scripts. The dashboard response gains an
onboarding_seed field with ref/title/slug/collection_slug/status
plus a server-computed `active` boolean (true iff status equals
the schema initial value).
3. CLI: printOnboardingHints accepts the template name, looks up
the primary ref via collections.GetTemplate, and prints the
right "use pad to get X-1" line. Templates without a declared
primary skip the line entirely (so hiring's pad init success
doesn't promise a non-existent BACK-1 / IDEA-1).
4. Web frontend: dashboard reads dashboard.onboarding_seed,
gates the banner on `active=true`, passes ref/slug/collection
to OnboardingIdeaBanner. The component renders the trigger
phrase, copy button, and "Read it first" deep link from those
props — no more hardcoded IDEA-1.
ensureWorkspace's signature gains a returned templateName so init.go
+ main.go can pass it through to printOnboardingHints. The five
existing test call sites updated.
New tests:
internal/collections/templates_test.go
- TestTemplatesDeclareOnboardingPrimaryRef — locks the per-template
OnboardingPrimaryRef values (and the explicit emptiness of
hiring/interviewing/demo).
internal/server/handlers_dashboard_test.go
- TestDashboardOnboardingSeed_StartupTemplate
- TestDashboardOnboardingSeed_ScrumTemplate
- TestDashboardOnboardingSeed_ProductTemplate
- TestDashboardOnboardingSeed_HiringTemplate (asserts NO seed —
hiring's REQ-1 is example data, not an onboarding entry)
- TestDashboardOnboardingSeed_EmptyWorkspace (no template)
Removes the loadIdeaOne race-guard from +page.svelte — the dashboard
poll itself now carries the onboarding_seed.active flag so the banner
state lives entirely in the dashboard response. Drops ~50 lines of
frontend code.
Parent: PLAN-1146.
|
||
|
xarmian
|
553a39f09b |
fix(cli): pad auth setup hint should point at pad init, not a nonexistent IDEA-1 (TASK-1143) (#407)
PR #403 (TASK-1134) added printIdeaOneTriggerHint() to the pad auth setup success path so freshly-bootstrapped admins would learn about the seeded onboarding entry point. But pad auth setup only creates the first admin account — no workspace. IDEA-1 is only seeded when a startup-template workspace is created (via pad init / pad workspace init). A user following the original hint immediately would hit "workspace not found" / "item not found". Caught by Codex during review of PR #406 (the docs PR for TASK-1138). TASK-1143 was spawned then to keep PR #406 docs-only; this is the fix. Reframe (Option 2 from the task spec): keep the hint, but point at the next concrete action — `pad init` — rather than at IDEA-1. The IDEA-1 trigger phrase still surfaces in `printOnboardingHints`, which runs after `pad init` / `pad workspace init`. By then the workspace exists and the trigger phrase resolves correctly. Renamed `printIdeaOneTriggerHint` → `printPostSetupNextStepsHint` since the hint no longer names IDEA-1 directly. Wording matches CLAUDE.md / README — workspace creation precedes the trigger phrase everywhere. Parent: PLAN-1131 (follow-up). Origin: Codex review of PR #406 round 1. |
||
|
xarmian
|
0a5eb777b9 |
feat(onboarding): surface IDEA-1 trigger phrase across CLI and web UI (TASK-1134) (#403)
* feat(onboarding): surface IDEA-1 trigger phrase across CLI and web UI (TASK-1134)
Make the seeded onboarding entry point discoverable without prior
knowledge. CONVE-191 calls for full-stack thinking on user-facing
features — this lands on every surface a fresh user might check.
CLI surfaces:
• `pad auth setup` success message gains a closing hint pointing at
`use pad to get IDEA-1` in a new agent session. New helper
printIdeaOneTriggerHint() so future templates can reuse the shape.
• `printOnboardingHints` (used after `pad init` / workspace creation)
now leads with the trigger phrase before the existing /pad prompt
suggestions. IDEA-1 is named because it's the seeded primary entry
in software-category templates; people-category templates will
seed REQ-1 / APP-1 etc. and need a template-aware version of this
hint — tracked under PLAN-1140.
Web UI surfaces:
• New OnboardingIdeaBanner component renders on the workspace
dashboard whenever IDEA-1 is in status=new. Shows the trigger
phrase verbatim with a copy button and a "Read it first" deep link
into the seeded item itself. Disappears the moment the user (or
agent) flips IDEA-1 out of `new`.
• Dashboard fetches IDEA-1 alongside its existing dashboard +
collections calls (cheap, indexed by ref) and re-checks on every
poll (default 30s) plus every sync signal so the banner is
self-correcting.
• Existing OnboardingChecklist gate (`totalItems === 0`) is left
alone. It still serves empty / non-templated workspaces; the new
banner is the templated-workspace surface.
No tests added — both surfaces are pure copy/render. Existing
dashboard + auth-setup tests still pass.
Parent: PLAN-1131. Origin: IDEA-1128.
* fix(onboarding): pin IDEA-1 lookup to exact prefix+number match per Codex review (round 1)
Server-side ResolveItem (via GetItemByRef) falls back from PREFIX-NUMBER
to a number-only lookup when the prefix doesn't match any collection in
the workspace. That fallback exists so an item moved between collections
is still resolvable by its old ref — but it has a bad interaction with
my new dashboard lookup:
In a non-software-category workspace (hiring, interviewing, …), there
is no Ideas collection. `api.items.get(ws, 'IDEA-1')` would silently
return whatever item has item_number=1 — typically REQ-1 (Requisition)
or APP-1 (Application). If that item happened to have status=new
(which the seeded Requisition / Application entries do), the dashboard
would render the IDEA-1 onboarding banner pointing at a /ideas/... URL
that 404s.
Fix: verify item.collection_prefix === 'IDEA' && item.item_number === 1
before trusting the result. Mismatch (or missing) → ideaOneStatus = null,
banner stays hidden. Software workspaces with a real IDEA-1 still match;
hiring / interviewing / interview-loop-style workspaces stop seeing the
banner entirely.
Caught by Codex on PR #403.
* fix(onboarding): guard IDEA-1 lookup against stale-workspace writes per Codex review (round 2)
Previous round addressed the wrong-collection match. This round fixes a
related race: rapid workspace navigation could let a slow loadIdeaOne()
from workspace A resolve after the user is already on workspace B and
write A's status into B's state, briefly rendering the IDEA-1 banner on
a workspace that doesn't have it.
Two-part fix:
1. The dashboard $effect that triggers load() now resets
ideaOneStatus = null synchronously when wsSlug changes, so any
leftover `new` status from the previous workspace can't briefly
render the banner during the window between navigation and the new
fetch resolving.
2. loadIdeaOne() now compares its captured slug against the current
wsSlug at every assignment point (success and error paths). If
they've diverged, the response is dropped — only the active
workspace's request can write ideaOneStatus.
Standard "was this still the active request" pattern. No behavior
change for the common case (single-workspace dashboard); the guard
only fires when navigation interleaves with an in-flight fetch.
Caught by Codex on PR #403.
|
||
|
xarmian
|
40621ff58d |
feat(metrics): session-id-keyed TTL sweep for mcp_active_sessions (TASK-1120) (#400)
* feat(metrics): session-id-keyed TTL sweep for mcp_active_sessions (TASK-1120) Replaces the naive +1/-1 active-sessions accounting from TASK-961. The old logic bumped on JSON-RPC `initialize` and decremented on HTTP DELETE — but a client that crashed, lost network, or restarted mid-session never emitted DELETE, so the gauge drifted upward monotonically until the pad-cloud server restarted. Approach: - `internal/server/middleware_mcp_session.go` (new) — mcpSessionTracker is an in-memory map keyed by Mcp-Session-Id (the canonical header set by mcp-go's StreamableHTTPServer on initialize responses and echoed by the client on subsequent requests). Touch updates lastSeen on insert + refresh; evict removes; periodic sweep evicts entries older than the TTL. - Gauge is `Set(len(sessions))` via an onChange callback — single consistent observation per state-changing op, no risk of gauge drifting from map size on a multi-evict sweep. - Lifecycle: spawned by SetMCPTransport (alongside startMCPAuditWriter), shut down from Server.Stop. Idempotent on both sides. - Configurable via PAD_MCP_SESSION_TTL (default 30m) and PAD_MCP_SESSION_SWEEP_INTERVAL (default 5m). cmd/pad calls Server.SetMCPSessionTrackerConfig before SetMCPTransport. Other changes: - `recordMCPCallMetrics` no longer touches the active-sessions gauge. Updated comment + signature kept (callers pass the same args; the unused params are explicitly underscored). - `MCPAuditLog` middleware now calls trackMCPSession after next.ServeHTTP — single new line in the audit hot path. - `TestMCPAudit_BufferFull_DropsAndIncrementsCounter` updated to also shut down the new session tracker before bg.Wait(), since SetMCPTransport now spawns two goroutines on srv.bg. Test coverage (16 tests, all green under -race): - Tracker unit: touch insert/dedup, empty-id no-op, evict remove/non-existent, sweep eviction with single onChange, nil-onChange safety, concurrent touch/evict, run() clean shutdown. - Server-side integration: lifecycle happy path (initialize → call → DELETE leaves gauge at 0), failed initialize doesn't open, no-session-id no-op, nil tracker safety, idempotent start, DELETE evicts on any status (transient 5xx on shutdown still counts). - Regression guard: TestRecordMCPCallMetrics_DoesNotTouchSessionGauge pins that the audit-side helper has migrated off the gauge. Parent: PLAN-943. Follow-up to TASK-961 (PR #398). Closes the "sessions drift on client crashes" caveat documented in the metric's help text + the Grafana panel description. * fix(metrics): emit Mcp-Session-Id + serialize gauge updates per Codex review (round 1) Two findings from Codex review on PR #400: 1. WithStateLess(true) wired StatelessSessionIdManager whose Generate() returns "" — mcp-go never set the Mcp-Session-Id response header in production, so the new tracker no-op'd on every initialize and the active-sessions gauge stayed at 0. Fix: introduce padMCPGenerateOnlySessionIDManager in cmd/pad/main.go. Generates a UUID per initialize (so the response carries the header — tracker can observe), but Validate accepts ANY incoming value (including empty / arbitrary). Preserves the original "stateless server, every request stands alone" contract while making the session-id observable. Documented why mcp-go's two shipped stateless managers don't fit (one breaks observability, the other breaks back-compat for clients that never echo the ID). 2. touch / evict / sweep computed `len(sessions)` under the mutex then released the lock BEFORE invoking onChange. Two concurrent inserts could compute (n=1, n=2) under the lock and then race the callback writes — last writer wins on the gauge, leaving it permanently inconsistent with the map size. Fix: hold the mutex across onChange. Trade-off documented: any future onChange that re-enters the tracker would deadlock, but that's a clear failure mode rather than silent metric corruption. Added TestMCPSessionTracker_OnChangeUnderLock that asserts a strictly-monotonic observation sequence under 32-goroutine concurrent inserts; passes 5x in a row under -race. |
||
|
xarmian
|
1c409c8592 |
feat(metrics): emit mcp_authz_denials_total{reason=tier_mismatch} (TASK-1119) (#399)
Wire the dispatcher-side scope-deny seam into the pad_mcp_authz_denials_total counter, completing the denial-reason vocabulary documented in TASK-961. internal/mcp/dispatch_http.go: - Add optional OnScopeDenied(method, urlPath) callback on HTTPHandlerDispatcher - Fire it from buildAuthedRequest right before returning the existing permission_denied error — same control flow, just observability added in front internal/server/middleware_auth.go: - Public Server.RecordMCPTierMismatch helper that bumps the counter. No MCP-origin context gate (unlike recordMCPAuthzDenial below) — the dispatcher is by construction MCP-only, so every invocation is inherently MCP-origin. cmd/pad/main.go: - Wire dispatcher.OnScopeDenied = srv.RecordMCPTierMismatch alongside the existing UserResolver / Lister fields. Safe to attach unconditionally — RecordMCPTierMismatch nil-checks metrics internally, mirroring the OAuth observer wiring pattern. Tests: - Three new dispatcher tests covering OnScopeDenied: fires once with the right (method, urlPath) on deny; does NOT fire on allow; nil hook is safe. - Server-side test for RecordMCPTierMismatch: counter increments, other denial reasons untouched, nil-metrics safe. Parent: PLAN-943. Follow-up to TASK-961 (PR #398). |
||
|
xarmian
|
ba303e456f |
fix(mcp): publish PAD_MCP_PUBLIC_URL verbatim as canonical resource (no /mcp suffix) (#381)
Per the MCP authorization spec the client MUST verify the URL it was given matches the discovery doc's `resource` field exactly; auto- suffixing was forcing operators publishing the bare hostname (the industry convention — mcp.stripe.com, mcp.linear.app, mcp.atlassian.com) into a permanent client-side mismatch and Claude Desktop / Cursor reject pasting `https://mcp.getpad.dev` even though everything else works. Both production sites that previously appended "/mcp" to MCPPublicURL now use the value verbatim: - cmd/pad/main.go: AllowedAudience for the OAuth server constructor. Tokens are now audience-bound to MCPPublicURL exactly. - internal/server/handlers_well_known.go: the protected-resource discovery doc's `resource` field is the bare MCPPublicURL. The transport itself is unchanged — pad still mounts at /mcp on the chi router; pad-cloud's nginx router transparently rewrites mcp.* root → /mcp (TASK-997 PR #28) so external clients see a single canonical URL regardless of the internal HTTP path. The audience binding is just a string; it doesn't have to equal the internal mount path. config.go's MCPPublicURL doc updated to reflect the new semantic ("canonical URL clients paste") rather than the old "vhost URL we suffix-mangle". Operators who want the old shape just include the /mcp suffix in PAD_MCP_PUBLIC_URL — the operator owns the canonical. Test fixtures: testCanonicalAudience flipped from "https://mcp.test.example/mcp" to "https://mcp.test.example", and the two SetMCPTransport call sites that previously stripped /mcp now pass it directly. The TestMCP_DiscoveryDoc_PopulatedFromConfig assertion uses testCanonicalAudience so future renames stay consistent. All other test sites (audience= form fields, aud claim checks, mismatch fixtures) keep working unchanged because they reference testCanonicalAudience symbolically. |
||
|
xarmian
|
9eb1a35f16 |
feat(mcp): privacy-preserving available_workspaces filter on error envelopes (TASK-977) (#379)
* feat(mcp): privacy-preserving available_workspaces filter on error envelopes (TASK-977)
Closes the last open work item in PLAN-943. HTTPHandlerDispatcher's
unknown_workspace error envelope now populates available_workspaces
filtered by the OAuth token's consent allow-list (TASK-952), so an
agent never sees workspace slugs the user didn't explicitly grant.
## What changed
- `HTTPHandlerDispatcher` gains a `Lister WorkspaceLister` field.
Production wires `mcpserver.NewOAuthWorkspaceLister(s)`; tests
can supply mocks.
- `packageHTTPResponse` now takes a `lister` parameter and threads
it down to `classifyHTTPStatus`. Both call sites in the package
updated.
- New `oauthWorkspaceLister` reads three things from request context:
- `server.CurrentUserFromContext` — the requesting user.
- `server.TokenAllowedWorkspacesFromContext` — the consent
allow-list (TASK-953 plumbing).
- `s.GetUserWorkspaces(user.ID)` — the user's full set.
Returns the intersection. Wildcard (`["*"]`) and nil (PAT auth)
short-circuit to "no filter" — the user's full set is returned
in those cases since the token doesn't constrain workspaces.
- `cmd/pad/main.go` wires the production lister.
## Privacy invariant
A token whose allow-list is `[alpha, beta]` MUST NOT see "gamma"
in the available_workspaces hint, even if the user is a member of
gamma. Tested explicitly via
TestUnknownWorkspace_AvailableWorkspaces_FilteredByAllowList —
the test fakes a 4-workspace user membership, sets allow-list to
2, and asserts exactly 2 slugs appear in the filtered envelope.
Without this filter, an attacker controlling an OAuth client could
hit any random workspace slug, get the unknown_workspace envelope,
and read OFF the user's full workspace list — defeating the whole
point of the consent UI's per-workspace selection.
## Tests (18 new)
8 envelope round-trip tests pin every documented HTTP status →
ErrorCode mapping (401 → auth_required, 403 → permission_denied,
404 generic → item_not_found, 404 workspace → unknown_workspace,
409 → conflict, 400/422 → validation_failed, 5xx → server_error,
418 → server_error fallback).
5 privacy-filter tests cover the allow-list shapes:
specific-list-filters, wildcard-no-filter, no-allow-list-no-filter,
no-user-empty-hints, store-error-empty-hints.
4 buildAllowSet unit tests for the helper.
1 end-to-end test through packageHTTPResponse.
* fix(mcp): use req.Context() when packaging HTTP response (Codex round 1)
Codex review #379 round 1 caught a real correctness issue: the
packageHTTPResponse calls in executeRequest + the prefetch path in
dispatchItemUpdate passed the dispatcher's outer ctx instead of
req.Context(). The lister reads CurrentUser + TokenAllowedWorkspaces
from context, and the canonical "everything attached" context is
the SYNTHESIZED request's context — buildHTTPRequest layers
WithCurrentUser + WithAPITokenAuth on it, and d.Apply (when wired)
attaches token state on top of req specifically.
In production this happened to work because MCPBearerAuth attaches
TokenAllowedWorkspaces on the inbound /mcp request's context, which
the dispatcher inherits as its outer ctx. But:
- Tests driving executeRequest with context.Background() + a
UserResolver-supplied user got empty available_workspaces
because the outer ctx had no user.
- Any future dispatcher attaching token state via Apply (rather
than relying on inbound-ctx propagation) would also see the
bug — the Apply hook is documented as the place for "TASK-953
token-scope context" exactly.
Fix: pass req.Context() / prefetchReq.Context() to packageHTTPResponse.
Same dispatcher, same ServeHTTP — just feed the lister the canonical
post-Apply context.
Test: TestExecuteRequest_UsesRequestContext_NotOuterContext drives
executeRequest with an empty outer context + a UserResolver, asserts
the resulting unknown_workspace envelope has the user's full
workspace list. With the buggy version the test fails (lister sees
no user → empty hints).
|
||
|
xarmian
|
48776a3967 |
feat(oauth): DCR + authorize + token endpoints + populated discovery (TASK-1025, sub-PR C of TASK-951) (#372)
* feat(oauth): DCR + authorize + token endpoints + populated discovery doc (TASK-1025, sub-PR C of TASK-951)
Third of 5 sub-PRs landing the OAuth 2.1 authorization server in
PLAN-943. Mounts the three flow-driving HTTP endpoints over the
fosite-backed server constructed in sub-PR B, replaces the
TASK-950 501 stub with the real RFC 8414 discovery doc, and ships
an inline-HTML consent stub as a TASK-952 placeholder so the
auth-code flow runs end-to-end.
What lands:
- internal/server/handlers_oauth.go (744 LoC)
- POST /oauth/register: RFC 7591 DCR. Hand-written, no fosite.
Public clients only (token_endpoint_auth_method=none rejected
for any other value), authorization_code + refresh_token
grants only, code response type only. Validates redirect_uris
(absolute, no fragment, https or loopback-http or custom-
scheme like claude://, blocks file:/javascript:/data:/vbscript:).
- GET /oauth/authorize: starts auth-code flow. fosite validates
request shape (PKCE-S256 required, audience matched, redirect
exact-match). If user has session → renders inline consent
stub. If not → 302 to /login?redirect=<self> (TASK-998's
plumbing in pad-cloud honors the redirect=).
- POST /oauth/authorize/decide: processes consent decision.
Form-bound CSRF token (the existing __Host-pad_csrf cookie,
read from a hidden form field instead of header). Approve →
fosite NewAuthorizeResponse → 303 to client.redirect_uri
with code. Deny → fosite WriteAuthorizeError(access_denied).
- POST /oauth/token: code + refresh exchange. fosite verifies
PKCE verifier (S256-required) + RFC 8707 audience. Returns
{access_token, token_type, expires_in, refresh_token, scope}.
RefreshTokenScopes=[] from sub-PR B means refresh ALWAYS
issues on authorize-code grant.
- Inline consent stub: minimal HTML form with Approve/Deny,
auto-grants every requested scope (TASK-952's UI replaces
with workspace allow-list selection per TASK-953).
- internal/server/handlers_well_known.go: handleOAuthAuthorizationServerStub
→ handleOAuthAuthorizationServer. Returns RFC 8414 metadata
with all six endpoint URLs (revoke + introspect URLs sub-PR D
fills with handlers; the URLs are stable now), advertised
scopes, S256-only code_challenge_methods,
resource_indicators_supported=true, authorization_response_iss_parameter_supported=true.
- internal/server/server.go: Server.oauthServer field +
SetOAuthServer + registerOAuthRoutes called from setupRouter
inside an r.Group with requireCloudMode + SessionAuth (so
/authorize can detect the logged-in user via __Host-pad_session;
SessionAuth falls through gracefully when no cookie).
- cmd/pad/main.go: oauthpkg.NewServer wired in cloud mode using
cfg.EncryptionKey as HMAC secret + cfg.MCPPublicURL+/mcp as
AllowedAudience. Wiring is conditional on PAD_MCP_PUBLIC_URL
being set (the OAuth surface needs a canonical audience to
bind tokens to).
CSRF posture: middleware_csrf.go runs only on /api/* paths so
/oauth/* is naturally exempt. The consent decision endpoint
adds its own form-token check (validateConsentCSRFToken) using
the same __Host-pad_csrf cookie the SPA uses, just with the
token in a hidden form field rather than a header. Same security
model, different transport.
Tests (12, all passing):
- TestOAuth_AuthorizationServerMetadata_PopulatedShape: pins
RFC 8414 metadata fields including S256-only PKCE +
resource_indicators_supported.
- DCR (5): happy path; missing redirect_uris; bad redirect-URI
shapes (relative, non-loopback http, fragment, javascript:);
non-public client auth method rejected; unknown grant type
rejected; not mounted outside cloud mode.
- /authorize (3): redirects to /login when no session;
renders consent stub when logged in; rejects audience
mismatch via fosite's audienceMatchingStrategy.
- /authorize/decide (2): rejects missing csrf_token; deny
produces access_denied redirect.
- Full PKCE flow: end-to-end /authorize/decide (approve) →
/token with code_verifier → 200 with access+refresh tokens.
- /token: rejects missing PKCE verifier.
Replaces the 501 stub assertion in TestMCP_AuthServerStub with
TestMCP_AuthServerMetadata_Mounted (just confirms 200; full
shape lives in the OAuth-handler test).
Out of scope:
- /oauth/revoke + /oauth/introspect (sub-PR D, TASK-1026)
- MCPBearerAuth OAuth introspection branch (sub-PR E, TASK-1027)
- Real consent UI with workspace allow-list (TASK-952)
* fix(oauth): translate RFC 8707 resource= to audience= + omit unmounted endpoints from discovery per Codex review (round 1)
Two findings from PR #372 round 1:
1. P1: Real RFC 8707 clients (Claude Desktop / Cursor / ChatGPT)
send `resource=` not `audience=`. fosite v0.49 reads only
`audience` from the form, so audienceMatchingStrategy was hit
with an empty needle and rejected every real-world authorize /
token request. Tests masked the gap by sending both keys.
Fix: translateResourceToAudience() copies r.Form["resource"]
into r.Form["audience"] before each handler invokes fosite.
Idempotent — if both keys are present, audience wins (test
harness sends both for belt-and-suspenders). Applied at
/authorize, /authorize/decide, and /token entry points.
Test TestOAuth_Authorize_AcceptsResourceOnly sends ONLY
resource= (no audience=) and asserts the request reaches the
consent stub. Without the translation it 303s with
invalid_request.
2. P2: /.well-known/oauth-authorization-server advertised
/oauth/revoke + /oauth/introspect endpoints that don't exist
yet (sub-PR D wires them). Real clients dialing those URLs
would get 404. RFC 8414 §2 lists revocation_endpoint +
introspection_endpoint as OPTIONAL, so omitting until the
handlers ship is spec-compliant + honest.
Fix: drop revocation_endpoint, introspection_endpoint, and
their *_endpoint_auth_methods_supported counterparts from
authServerMetadata. Sub-PR D's PR description includes
"populate these here" as a follow-up.
Test TestOAuth_AuthorizationServerMetadata_OmitsUnimplementedEndpoints
asserts the four fields are absent.
* fix(oauth): rate-limit /oauth/register + drop misleading iss flag per Codex review (round 2)
Two findings from PR #372 round 2:
1. P1: /oauth/register is open by RFC 7591 design (Claude Desktop /
Cursor self-register without prior auth) but had no rate limit.
An attacker could flood the oauth_clients table indefinitely.
Fix: extend RateLimit middleware to gate /oauth/register at
the same 5/hour/IP rate the existing /api/v1/auth/register
uses (RateLimiters.Register, burst 5). Added the OAuth route
group to the s.RateLimit middleware chain so the new path
actually runs through the limiter.
Other /oauth/* endpoints aren't rate-limited here: /authorize
rides session cookies (cheap to abuse but ineffective without
a logged-in user), /token is PKCE-bound to a stored code
(single-use), /authorize/decide is form-bound. Explicit per-
endpoint /oauth/* limits arrive with TASK-959.
Test TestOAuth_Register_RateLimited fires 5 requests
successfully, asserts the 6th returns 429.
2. P2: Discovery doc advertised
authorization_response_iss_parameter_supported=true, but the
/authorize success path delegates to fosite v0.49 which doesn't
add iss=<issuer> to the redirect. RFC 9207-aware clients seeing
the flag would treat the missing parameter as a protocol
violation.
Fix: drop the field from authServerMetadata. RFC 8414 §2
marks it OPTIONAL — omission is spec-compliant. We'll add
the parameter (+ post-processing of fosite's response) in a
future PR if a real client requires it; today's MCP clients
(Claude Desktop, Cursor, ChatGPT) don't.
Test TestOAuth_AuthorizationServerMetadata_OmitsUnimplementedEndpoints
extended to cover the field.
* fix(oauth): gate auth-server discovery doc on oauthServer != nil per Codex review (round 3)
Codex round 3 caught: /.well-known/oauth-authorization-server lives
in the MCP route group (registerMCPRoutes), while the /oauth/{
register,authorize,token} handlers live in the OAuth route group
(registerOAuthRoutes, gated on s.oauthServer != nil). A cloud
deployment with PAD_MCP_PUBLIC_URL unset gets MCP routes mounted
but NOT OAuth — the discovery doc would 200 with /oauth/* URLs
that 404. Worse for clients than no document at all.
Fix: handleOAuthAuthorizationServer now also nil-checks
s.oauthServer; on nil it returns 503 with config_error, matching
the existing fail-loud branch for when the issuer URL isn't
configured. Ops detect the misconfiguration immediately rather
than fielding "OAuth registration is failing with 404" tickets.
Test:
- TestOAuth_AuthorizationServerMetadata_503WhenOAuthDisabled
builds a Server with SetCloudMode + SetMCPTransport (so the
MCP route group mounts) but NOT SetOAuthServer; asserts the
endpoint returns 503 with config_error.
- TestMCP_AuthServerMetadata_Mounted renamed →
TestMCP_AuthServerMetadata_MountedAndGated to reflect the new
behavior under mcpEnabledTestServer (which doesn't wire OAuth).
The full 200 happy path lives in
TestOAuth_AuthorizationServerMetadata_PopulatedShape (uses
oauthEnabledTestServer).
* fix(oauth): apply gofmt to handlers_oauth_test + handlers_well_known
* fix(oauth): bump go-jose/v3 to v3.0.4 to resolve GO-2025-3485
CI govulncheck rejected the build: fosite v0.49.0 transitively
pulls github.com/go-jose/go-jose/v3@v3.0.3 which has
GO-2025-3485 (DoS in JWS parsing). Affected call site:
internal/server/handlers_oauth.go:408 — handleOAuthAuthorize calls
fosite.NewAuthorizeRequest which eventually calls jose.ParseSigned.
Fix: bump go-jose/v3 to v3.0.4 (the fixed version per the advisory).
go mod tidy auto-bumped dependent indirect deps too.
Verified locally:
govulncheck ./... → "No vulnerabilities found"
go test ./... → all green
go build ./... → clean
|
||
|
xarmian
|
521853e0a1 |
feat(mcp): mount /mcp Streamable HTTP transport + OAuth discovery (TASK-950) (#369)
* feat(mcp): mount /mcp Streamable HTTP transport + OAuth discovery (TASK-950) First public cut of pad-cloud as a remote MCP server (PLAN-943). Mounts the Streamable HTTP transport on /mcp, the RFC 9728 protected-resource discovery doc on /.well-known/oauth-protected-resource, and a 501 stub for RFC 8414 auth-server metadata that TASK-951 will fill in. - internal/server/handlers_mcp.go — Server.SetMCPTransport + chi route registration under cloud-mode gate (self-host stays free of MCP overhead unless explicitly opted in). - internal/server/middleware_mcp_auth.go — Bearer auth that produces the spec-shape 401 + WWW-Authenticate (resource_metadata pointer) MCP clients expect, distinct from /api/v1's JSON-only 401 envelope. Reuses the existing PAT (api_tokens) validation path; OAuth-issued tokens layer in via this same middleware in TASK-951. - internal/server/handlers_well_known.go — RFC 9728 discovery doc + RFC 8414 stub. URLs come from PAD_MCP_PUBLIC_URL + PAD_AUTH_SERVER_URL with request-host fallback for local dev. - internal/server/handlers_mcp_test.go — 7 tests covering cloud-off routes-absent, cloud-on-no-transport routes-absent, discovery doc shape, 501 stub, no-token 401+WWW-Authenticate, bad-format-token 401+WWW-Authenticate, and the valid-PAT happy path with user attached to transport context. - cmd/pad/main.go — wires mcpserver.NewServer + HTTPHandlerDispatcher + StreamableHTTPServer in cloud mode, after SetCloudMode. - internal/config — adds PAD_MCP_PUBLIC_URL and PAD_AUTH_SERVER_URL. Resources are intentionally skipped in this v1 — they require an HTTPResourceFetcher equivalent of ExecResourceFetcher and that's a follow-up task. Tools, prompts, instructions, and meta all flow through identically to the stdio surface (verified via spike against mcp-go v0.50.0's StreamableHTTPServer before writing the real PR). * fix(mcp): enforce PAT scopes on /mcp + WWW-Authenticate fallback per Codex review (round 1) Two findings from PR #369 round 1: 1. SECURITY: A PAT with scopes ["read"] could drive write MCP tools. MCPBearerAuth skipped tokenScopeAllows entirely; the dispatcher's synthesized in-process request bypassed TokenAuth's chain-level check (because WithCurrentUser was already set), so a read-scoped token could POST item create / PATCH update / DELETE silently. Fix: stash apiToken.Scopes via server.WithTokenScopes in MCPBearerAuth; re-check per synthesized request in HTTPHandlerDispatcher.executeRequest using the public server.TokenScopeAllows wrapper. Read-scoped tokens can still drive read-only tools (their HTTP method is GET) — only writes are rejected, with a structured permission_denied envelope. 2. DISCOVERY: writeMCPUnauthorized dropped the WWW-Authenticate header when PAD_MCP_PUBLIC_URL was unset. Cloud-mode deploys without that env var mounted /mcp but broke the discovery handshake — fresh MCP clients rely on the header to find /.well-known/oauth-protected- resource. Fix: pass *http.Request through to writeMCPUnauthorized, derive "https://" + r.Host as the fallback (matches handleOAuthProtected- Resource's existing fallback). Tests: - handlers_mcp_test.go: TestMCP_NoToken_FallsBackToHostWhenPublicURLUnset pins the WWW-Authenticate fallback. TestMCP_ReadScopedPAT_StashesScopes- InContext + TestTokenScopeAllows_PublicWrapper pin the scope-stash side. - dispatch_http_test.go: TestHTTPHandlerDispatcher_ScopeEnforcement_* pin the dispatcher-side enforcement (read-on-write rejected, read-on-read allowed, no-scope-context allows-all). - recordingHandler updated to handle nil r.Body so the read-only GET path can be exercised. * fix(mcp): move scope check to buildAuthedRequest so bulk-update can't bypass it per Codex review (round 2) Round 1 enforced scopes in executeRequest, but dispatch_http_project.go's item bulk-update path constructs each per-item PATCH directly via buildAuthedRequest + d.Handler.ServeHTTP, skipping executeRequest. Net result: a PAT with ["read"] scope could still mutate items through bulk-update even after the round-1 fix. Move the scope check from executeRequest into buildAuthedRequest so every synthesized request — main writes, RMW prefetches, bulk-update per-item PATCHes, link-create POSTs, attachment HEADs — passes through the same gate uniformly. The check is dropped from executeRequest to avoid double-checking; buildAuthedRequest is the universal funnel everything calls. Reads (GET/HEAD/OPTIONS) under ["read"] scope still pass — bulk- update's per-item GET prefetch succeeds, the subsequent PATCH fails at request-build time with permission_denied. The bulk operation returns successfully with all-errors recorded per ref (the "no abort on per-item failure" contract is unchanged). Test: TestHTTPHandlerDispatcher_ScopeEnforcement_BulkUpdateBlockedOnReadScope spies on the test handler; asserts the PATCH never reaches it under ["read"] scope and that each per-item entry carries permission_denied. |
||
|
xarmian
|
eb1931d747 |
feat(cli): structured JSON output for note/decide/star/unstar/delete/bulk-update (BUG-989) (#363)
* feat(cli): structured JSON output for note/decide/star/unstar/delete/bulk-update (BUG-989)
Six pad item subcommands previously returned plain-text confirmations
when called with --format json (and therefore via MCP) — e.g.
"Added implementation note to TASK-8 ...\n", "⭐ Starred TASK-7 ...\n".
Agents had to scrape the text for refs / IDs / status. Now each
emits a structured envelope on the JSON branch, matching the shape
the bug report's recommendations specified.
Per-command shapes:
- pad item note --format json:
{ ref, title, note: { id, summary, details, created_at, created_by } }
- pad item decide --format json:
{ ref, title, decision: { id, decision, rationale, created_at, created_by } }
- pad item star --format json:
{ ref, title, starred: true }
- pad item unstar --format json:
{ ref, title, starred: false }
- pad item delete --format json:
{ ref, title, status: "archived" }
- pad item bulk-update --format json:
{ updated: [{ref, applied: {status?, priority?}}], failed: [{ref, error}], total }
Implementation notes:
- For note/decide: capture the entry locally before persisting so the
JSON branch can echo the freshly-created ID + timestamp without
re-fetching the item.
- For bulk-update: collect per-item outcomes in two slices (updated,
failed) so the response carries which refs succeeded with which
applied changes vs which failed with what error. Suppresses the
human-readable per-line ✓/✗ output when --format json (single
payload at the end is the agent's source of truth).
- Human-readable (default) output paths unchanged for all six.
The text fallback that ExecDispatcher passes back through
packageJSONResult also keeps showing the same structured JSON now
since the CLI emits JSON directly when --format json is set —
matches BUG-985's wrap-arrays-as-{items:[...]} pattern for list
shapes, and rounds out the v0.2 catalog's structured-everywhere
contract.
Live verified all six locally (CLI direct + MCP transport):
pad item note TASK-994 "..." --format json → {note: {...}, ref, title}
pad item star TASK-994 --format json → {ref, starred: true, title}
pad item bulk-update --priority medium TASK-994 --format json
→ {updated: [...], failed, total}
pad item delete TASK-994 --format json → {ref, title, status: "archived"}
(note/decide/star/unstar/decide via MCP confirmed dict-shaped
structuredContent with the expected keys.)
Parent: BUG-989.
* fix(cli): delete JSON envelope uses `archived: true` per Codex review (round 1)
Codex finding: `pad item delete --format json` was emitting
`"status": "archived"`, but the store's delete path only sets
`deleted_at` — the item's persisted `status` field is untouched. So
the envelope's status would mislead agents into treating the item's
status as archived, which breaks if the item is later restored (its
original status field is still there).
Fix: rename to `"archived": true` — unambiguous about what actually
happened (soft-delete marker set) and doesn't collide with the
persisted status semantics.
The other five JSON shapes weren't affected.
|
||
|
xarmian
|
55d3a078a8 |
fix(mcp): standup CLI ref + classifier polish for BUG-987 round 2 (#362)
Round-2 hotfix on top of PR #361 (which shipped to v0.1.0-rc.4). Claude Desktop's re-review of rc.4 surfaced two fixes that didn't fully land: - Bug 8 (round 1 went to wrong layer). My HTTPHandlerDispatcher fix populated ref on standup blockers, but Claude Desktop's path is ExecDispatcher → CLI subprocess → standupCmd, which has its own JSON composition struct. That struct's Attention + SuggestedNext anonymous types didn't even define ItemRef as a parseable field. Now both define `item_ref` and the JSON-emit loops set Ref from it. Verified live: blockers now carry refs (TASK-X), not empty strings. - Bug 11 part 2. Round 1 stripped the cobra Usage block but two artifacts still leaked: 1. The "pad <verb> failed: <stderr>" prefix on server_error fallback messages. The verb name is the OLD CLI verb (e.g. `pad item block`) which doesn't match the v0.2 catalog actions agents see, and the cmdPath is already implicit from the invoked tool. Drop the prefix; emit the cleaned stderr directly. 2. Self-link / "cannot ..." validation rejections classified as server_error instead of validation_failed. Extended the validation regex with `cannot ` so server-side rejections like "cannot link an item to itself" / "cannot modify archived item" route to ErrValidationFailed. Verified live with a self-link attempt — now returns code=validation_failed, hint="cannot link an item to itself", no prefix. - New stripErrorPrefix helper trims leading `Error:` / `error:` / `ERROR:` from every classified hint+message so the envelope text isn't redundant with the envelope's `code` signal. Bug 13 / Bug 14: my round-1 fixes verified working locally on rc.4 (tested with a fresh Task → convention=None; dashboard by_role shows "Unassigned"/"unassigned" for the bucket). The reviewer's stale results almost certainly reflect a pad server process that wasn't restarted with the rc.4 binary swap. Tests: - TestClassifyExecError_CannotPhrasingClassifiesAsValidation — three "cannot ..." stderr cases must classify validation_failed. - TestClassifyExecError_NoLegacyVerbPrefixInMessage — pins the prefix-strip behaviour on the server_error fallback path. - TestStripErrorPrefix — trim-rule round-trip across casing variations and empty input. Parent: BUG-987. |
||
|
xarmian
|
0f05012169 |
fix(mcp): six surgical fixes for BUG-987 (bugs 6, 8, 11, 12, 13, 14) (#361)
* fix(mcp): six surgical fixes for BUG-987 (bugs 6, 8, 11, 12, 13, 14)
Hotfix follow-up to v0.1.0-rc.3's Claude Desktop dogfood. Six
surgical fixes; bigger items (5, 7, 9, 10) deferred to separate
tasks.
- Bug 6: `pad project next --format json` was emitting the entire
dashboard, indistinguishable from `pad project dashboard --format
json`. Now slices to suggested_next only. cmd/pad/main.go.
- Bug 8: standup blockers carried empty `ref` strings, blocking
agent linkback to the actually-blocked items. dashboard's
attention[].item_ref is canonical; the standup composer in
internal/mcp/dispatch_http_slice4.go just wasn't propagating it.
Same fix applied to suggested_next entries.
- Bug 11: cobra's auto-emitted "Usage: pad item block ..." help
block leaked into MCP error envelopes via classifyExecError. The
Usage text references OLD CLI verb names (pre-v0.2 catalog) that
agents using the new surface have no business seeing, and bloats
every error response. New stripCobraUsageBlock helper truncates
stderr at the first line-anchored "Usage:" marker before
classification + envelope construction.
- Bug 12: BuildCLIArgs validation errors (missing required arg, type
mismatch) came out of env.Dispatch as bare-text NewToolResultErrorf
results, breaking the structured envelope contract. New helper
validationFailedFromBuildErr wraps them as ErrValidationFailed
envelopes with the field name extracted via regex from the
underlying message.
- Bug 13: every Task / Idea / Plan with a `priority` field got a
phantom `convention: { enforcement: "<priority>" }` surfaced on
its response, because ExtractItemConventionMetadata's legacy
fallback treated `priority` as the Convention enforcement tier
unconditionally. Restructured to track hasConventionShape
separately from hasMetadata — only Convention-specific markers
(structured convention field, trigger, scope, surfaces, commands,
direct enforcement) flip the shape flag. category alone is
insufficient (Ideas / Bugs / Roadmap items legitimately use it).
Final guard returns nil when only category was matched.
- Bug 14: GetRoleBreakdown's unassigned row was emitted with empty
role_name + role_slug, presenting as a "phantom" entry in the
dashboard. Now explicitly labelled "Unassigned" / "unassigned"
while keeping role_id null so it's still distinguishable from a
real role.
Tests:
- internal/mcp/bug987_test.go (new) — stripCobraUsageBlock + classify
+ validation envelope wrapping + env.Dispatch integration.
- internal/models/item_test.go — three cases covering non-Convention
items (Task, Idea, Plan with priority) returning nil metadata, and
one preservation test for legacy Conventions with priority field.
- internal/store/agent_roles_test.go (new) — confirms unassigned row
carries explicit "Unassigned" / "unassigned" labels.
Live verified: pad_project action=next returns just the suggestions
array; pad_item action=create with no fields returns validation_failed
with field=collection; pad_item action=link with self-target returns
without Usage-block leakage.
Deferred to separate items (per BUG-987 triage):
- Bug 5: text vs JSON returns across note, decide, star, unstar,
delete, bulk-update — needs CLI-side handler updates per command.
- Bug 7: suggested_next algorithm — needs to consider in-progress
items, not just open ones; behavior change needs design.
- Bug 9: fields/tags double-stringified — potentially breaking for
web UI/CLI consumers.
- Bug 10: decision_log/notes embedded in fields blob duplicating
top-level arrays — might require data migration.
Parent: BUG-987.
* fix(mcp): HTTP transport equivalence + ordering for BUG-987 per Codex review (round 1)
Two findings from Codex review of PR #361:
1. project.next on HTTP transport still returned the full dashboard.
The route table mapped "project next" directly to /dashboard, so
the CLI fix (slice to suggested_next) didn't reach OAuth-authed
agents going through HTTPHandlerDispatcher. Catalog actions must
produce equivalent shapes on stdio and HTTP — that's the contract
that lets agents be transport-agnostic.
Fix: new dispatchProjectNext method on HTTPHandlerDispatcher that
fetches the dashboard via the existing fetchDashboardJSON helper,
slices to suggested_next[], re-encodes, and runs through
packageJSONResult so it gets the same {items: [...]} wrap as
other list responses.
Also retires the broken route-table entry — replaced with a
comment pointing at the new method so future contributors don't
re-add a passthrough.
Test: TestDispatch_ProjectNext_SlicesToSuggestedNext + the empty-
array case. Asserts dashboard-only top-level fields (summary,
active_items) don't leak into the response — that's the whole
point of project.next being distinct from project.dashboard.
2. ExtractItemConventionMetadata's priority→enforcement legacy
fallback ran BEFORE surfaces/scope/commands had a chance to flip
hasConventionShape, so a Convention with only `{scope, priority}`
would silently drop enforcement.
Fix: move the priority fallback to AFTER all marker checks. Direct
`enforcement` still resolves first; the legacy priority fallback
runs at the bottom once shape detection is complete.
Tests: two new cases covering scope-only and commands-only legacy
Conventions — both must resolve enforcement via the priority
fallback.
Parent: BUG-987.
|
||
|
xarmian
|
1e94fcbd9d |
feat(mcp): structured MCP error envelopes with closed taxonomy (TASK-973) (#357)
* feat(mcp): structured MCP error envelopes with closed taxonomy (TASK-973)
Replaces raw stderr / status-text passthrough with a closed-set
ErrorCode taxonomy + structured ErrorEnvelope. Agents can now branch
on `error.code` instead of parsing free-form text:
{
"error": {
"code": "no_workspace",
"message": "No workspace context. Pass `workspace` explicitly, ...",
"hint": "Available workspaces: docapp, pad-web",
"available_workspaces": [{"slug": "docapp", "default": true}, ...]
}
}
Taxonomy (8 codes):
- no_workspace, unknown_workspace — populate available_workspaces
- auth_required, permission_denied
- item_not_found, validation_failed, conflict
- server_error (catch-all)
Implementation:
- internal/mcp/errors.go (new): ErrorCode constants, ErrorEnvelope +
ErrorPayload + WorkspaceHint types, NewErrorResult constructor,
classifyExecError + classifyHTTPStatus dispatchers, regex pattern
matchers for stderr classification, WorkspaceLister interface for
hint enrichment.
- internal/mcp/dispatch.go: ExecDispatcher.Dispatch routes failures
through classifyExecError (with itself as the WorkspaceLister).
Adds ListWorkspaces method that shells out to `pad workspace list
--format json`. Adds RootArgs field so the listing inherits root
flags (--url etc.).
- internal/mcp/dispatch_http.go: packageHTTPResponse routes 4xx/5xx
through classifyHTTPStatus. Lookup is intentionally nil here —
TASK-977 (PLAN-943) owns the privacy-preserving available_workspaces
filtering by OAuth allow-list.
- cmd/pad/mcp.go: pre-flatten rootFlags into RootArgs at
dispatcher construction.
NewErrorResult emits BOTH structured content (for Claude Desktop,
Cursor) AND a JSON text body (for older clients). Both decode to the
same envelope so wire-level shape stays uniform.
Tests:
- TestNewErrorResult_Envelope: round-trip the envelope through
structured + text surfaces.
- TestClassifyExecError: 11 cases covering every taxonomy code via
stderr patterns.
- TestClassifyExecError_LookupFailureStillReturnsEnvelope: lookup
failures degrade to empty available_workspaces, never drop the
whole envelope.
- TestClassifyHTTPStatus: 10 cases covering each HTTP status mapping
including the workspace-vs-item 404 fork.
- TestParseWorkspaceListJSON: happy path, empty / null / malformed,
entry-without-slug skipping.
- TestExtractUnknownWorkspaceSlug: regex helper round-trip.
Out of scope (per task description):
- HTTPHandlerDispatcher available_workspaces filtering by OAuth
allow-list → TASK-977 (PLAN-943).
- item_not_found "recent items" hint enrichment → also TASK-977.
- Per-code docs page on getpad.dev/mcp/local → TASK-976.
Parent: TASK-973 → PLAN-969.
* fix(cli): add JSON output to pad workspace list per Codex review (round 1)
Codex P2: classifyExecError's WorkspaceLister side channel calls
`pad workspace list --format json` to populate available_workspaces
in no_workspace / unknown_workspace error envelopes (TASK-973). The
CLI command silently ignored formatFlag and always printed the
human-readable shape, so parseWorkspaceListJSON would fail and the
hint was effectively never populated.
Add JSON branch to workspacesCmd that emits a {slug, name,
updated_at, default} array. The `default: true` flag marks the
CWD-linked workspace so agents can prefer it without a separate
DetectWorkspace call.
Manual verification:
$ pad workspace list --format json | jq '.[0]'
{
"slug": "docapp",
"name": "pad",
"updated_at": "2026-04-14T13:24:51Z",
"default": true
}
Parent: TASK-973 → PLAN-969.
* fix(mcp): tighten unknown-workspace slug regex per Codex review (round 2)
Codex finding: extractUnknownWorkspaceSlug's bare-word regex captured
stop-words like "not" out of generic "Workspace not found" messages.
The server emits exactly this generic body in middleware_auth.go and
handlers_workspaces.go, so the resulting envelope would say
`Workspace "not" is not visible to this session.` — pushing agents
toward retrying with a bogus slug.
Tighten the regex to only match QUOTED slug forms ("workspace 'foo'"
or "workspace \"bar\""). Bare-word phrasings yield empty slug, and
unknownWorkspaceResult now emits a generic "Workspace not visible to
this session." instead of the misleading empty-string `Workspace ""`.
Test cases updated:
- "workspace 'foo' does not exist" → "foo" (still works)
- "workspace \"bar\" not found" → "bar" (still works)
- "unknown workspace baz" → "" (was "baz", now intentionally empty)
- "workspace docapp not visible" → "" (was "docapp", now empty)
- "Workspace not found" → "" (the actual server response)
The other taxonomy / hint behavior is unchanged: ErrUnknownWorkspace
still classifies correctly, available_workspaces still populates from
ListWorkspaces, and the body text still appears in Hint via the
classifyHTTPStatus 404 branch's body-append logic.
Parent: TASK-973 → PLAN-969.
|
||
|
xarmian
|
19f20c5911 |
feat(mcp): v0.2 catalog migrate pad_item + retire cmdhelp walker (TASK-981) (#354)
* feat(mcp): v0.2 catalog migrate pad_item + retire cmdhelp walker (TASK-981)
Final commit of TASK-970's 3-stage rollout (PLAN-969). pad_item lands
with 17 actions consolidating the v0.1 verb tools (item_create /
item_block / item_star / item_unstar / item_supersedes / item_unsupersede /
...) into one resource × action shape. cmdhelp leaf walker retired —
tools/list now advertises only the v0.2 catalog (~7 catalog tools +
pad_set_workspace).
pad_item actions:
- Lifecycle: create, update, delete, get, list, move
- Relationships: link, unlink, deps
- Stars: star, unstar, starred
- Comments: comment, list-comments
- Bulk + notes + decisions: bulk-update, note, decide
link / unlink dispatch on link_type via itemLinkRoutes table:
- blocks, blocked-by → item block / blocked-by + item unblock
- supersedes → item supersedes / unsupersede
- implements → item implements / unimplements
- split-from → item split-from / unsplit
Per-direction op (cmdPath, firstArg, secondArg, inverted) handles
the asymmetric "blocked-by unlink reuses unblock with operands swapped"
case correctly.
Walker retirement:
- registry.go shrinks dramatically. Register() now registers
pad_set_workspace + delegates to RegisterCatalog. Drop identifyLeaves,
hasExcludedAncestor, buildTool, makeDispatchHandler, propertyForArg,
propertyForFlag, propertyOptionsCommon, stringifyEnum, ToolNameFromPath,
DefaultExcludes, RegistryOptions.ExcludeCommands.
- mergeDispatchInput moves to dispatch.go (still used by env.Dispatch).
- registry_test.go pruned to: validation tests, MCPPropertyName tests,
shared helpers (fakeDispatcher, fixtureDoc, equalSlice). DOC-978
said to "delete and rebuild" — done; the v0.1 walker assertions
weren't worth carrying forward.
- cmd/pad/mcp.go: single Register() call (no separate RegisterCatalog).
ToolSurfaceVersion bumped 0.1 → 0.2. pad_meta.tool-surface's
rollout_status flips from "in-progress" to "complete" automatically
because the bump makes ToolSurfaceVersion != "0.1".
CLAUDE.md updated to reflect the new architecture (catalog over walker;
two version constants — CmdhelpVersion + ToolSurfaceVersion).
Tests:
- TestPadItemLink_DispatchTable iterates itemLinkRoutes and asserts
link/unlink dispatch correctly for every link_type, including the
blocked-by-uses-unblock-with-swapped-operands case.
- TestPadItemLink_Missing/UnknownLinkType for the structured error path.
- catalog_readonly_test.go's expected{} extended with pad_item
passThrough actions; link/unlink intentionally skipped (custom
dispatch).
- TestRegister_PassesPadVersionToCatalog round-trips PadVersion through
RegistryOptions → CatalogOptions → ActionEnv.
Parent: TASK-981 → TASK-970 → PLAN-969.
* fix(mcp): support repeatable refs for pad_item.bulk-update per Codex review (round 1)
Codex P (no priority shown — substantive issue): pad_item exposed
`ref: string` everywhere, but bulk-update's CLI takes a repeatable
positional (one or more refs). The retired cmdhelp walker generated
array schemas for repeatable args; v0.2's scalar `ref` made
bulk-update effectively single-item or schema-invalid for its
primary use case.
Fix: dedicated `refs: array<string>` schema param + custom
actionItemBulkUpdate handler. Translates `refs` array → repeatable
`ref` positional (the form BuildCLIArgs feeds CLI commands with
arg.Repeatable=true).
Why a separate `refs` param vs. overloading `ref`: keeps the schema
consistent across actions — agents see one shape per param name.
JSON Schema oneOf would also work but mcp-go's helpers don't expose
it cleanly.
Lenient fallback: a single ref passed unwrapped as a string still
works (logically equivalent to a 1-element array). Empty arrays and
missing refs both surface structured errors with `refs is required`.
Tests cover: array of strings → multiple positionals, single string
fallback, missing refs error, empty array error. Existing fixture
in TestReadOnlyCatalog_ActionsDispatchExpectedCmdPath extended with
`refs: ["TASK-1", "TASK-2"]` so bulk-update reaches dispatch.
Parent: TASK-981 → TASK-970 → PLAN-969.
|
||
|
xarmian
|
df8a3631e7 |
feat(mcp): v0.2 catalog scaffold + ToolSurfaceVersion + pad_meta tool (TASK-979) (#352)
* feat(mcp): v0.2 catalog scaffold + ToolSurfaceVersion + pad_meta tool (TASK-979) First commit of TASK-970's 3-stage rollout (PLAN-969). Introduces the hand-curated v0.2 catalog types (ToolDef, ActionFn, ActionEnv) and ships one tool — pad_meta — end-to-end. v0.1 cmdhelp-walk surface stays live alongside; subsequent commits (TASK-980, TASK-981) migrate the rest and flip v0.1 off. Architecture record: DOC-978. The fan-out registry sits ABOVE the dispatcher boundary — Dispatcher / route table are unchanged, so both ExecDispatcher (stdio) and HTTPHandlerDispatcher (HTTP) inherit the new shape for free. Changes: - internal/mcp/catalog.go (new) — ToolDef, ActionFn, ActionEnv, passThrough helper, RegisterCatalog, makeFanOutHandler, structured error helpers. - internal/mcp/catalog_meta.go (new) — pad_meta tool with three inline actions: server-info, version, tool-surface (full catalog dump for PLAN-943 docs generation). - internal/mcp/version.go — add ToolSurfaceVersion = "0.2" + matching experimentalToolSurfaceKey. Independent of CmdhelpVersion (cmdhelp owns CLI help-tree contract; ToolSurfaceVersion owns MCP catalog). - internal/mcp/meta.go — extend MetaPayload with ToolSurfaceVersion; experimentalCapabilities advertises both padCmdhelp + padToolSurface. - cmd/pad/mcp.go — call RegisterCatalog alongside Register so v0.2 surface is live. - Tests: catalog_test.go + catalog_meta_test.go (new); meta_test.go + server_test.go updated to assert the new field/capability. Parent: TASK-970 → PLAN-969. * fix(mcp): keep ToolSurfaceVersion at "0.1" until catalog is complete per Codex review (round 1) Codex P1: advertising tool_surface_version=0.2 while the user-visible surface is still predominantly v0.1 (cmdhelp walker active alongside, only pad_meta in the catalog) misleads consumers that pin against the handshake or pad://_meta/version. The padToolSurface namespace would suggest the full resource/action shape is available when in reality only pad_meta uses it. Delay the 0.1 → 0.2 bump to TASK-981 — the commit that retires the cmdhelp walker and ships the complete catalog. The constant stays declared so the surface contract is wired through the handshake + meta resource + pad_meta.tool-surface, the version string just truthfully reflects "still v0.1" until the catalog is complete. No test changes needed: every assertion uses the constant, not a literal "0.2". Parent: TASK-979 → TASK-970 → PLAN-969. * fix(mcp): scope pad_meta.tool-surface to v0.2 catalog only per Codex review (round 2) Codex P1: pad_meta.tool-surface description claimed "Full catalog dump: every tool" but during PLAN-969's parallel rollout, tools/list contains both the catalog (currently just pad_meta) AND the cmdhelp walker's ~85 verb tools. Calling the catalog dump "every tool" misleads consumers who expect a complete enumeration. Same spirit as round 1's fix: stop claiming what isn't true. The catalog dump is the v0.2 catalog by design — consumers wanting the complete advertised surface should read tools/list directly. Hand-mapping the walker output into the catalog dump would cost duplication for a surface that's about to disappear in TASK-981. Wire-level changes: - Tighten the action description in padMetaToolDescription to say "v0.2 catalog dump: every tool managed by the hand-curated catalog" and explicitly note tools/list is the source for the complete surface. - Add rollout_status field to the response payload: "in-progress" while ToolSurfaceVersion stays at "0.1", "complete" once TASK-981 bumps it. Lets consumers detect the rollout state programmatically. - Test asserts the new field tracks ToolSurfaceVersion. Parent: TASK-979 → TASK-970 → PLAN-969. * fix(mcp): include params in pad_meta.tool-surface dump per Codex review (round 3) Codex P1: tool description claimed the dump includes each tool's "input schema" but the payload only emitted name/description/workspace/ actions[]. Misleading for docs generators (TASK-957) that would build getpad.dev/docs/mcp from this canonical source. Going with the substantive fix rather than just trimming the description: include a synthesized params[] per tool entry. Mirrors what consumers see in tools/list — `action` (always required, enum of declared action names), `workspace` (when ToolDef.Schema.Workspace=true), and per-tool ParamDefs. Synthesizing `action` and `workspace` rather than copying them from ToolDef makes the dump self-contained: a docs generator doesn't need to reproduce buildToolFromDef's implicit-param logic separately. Test asserts each catalog entry has params[] starting with `action` (enum length matches action handler count) and the right total length based on Schema.Workspace + Schema.Params. Parent: TASK-979 → TASK-970 → PLAN-969. |
||
|
xarmian
|
d84f1180a7 |
feat(mcp): pluggable Dispatcher + HTTPHandlerDispatcher for remote MCP (TASK-965) (#343)
* feat(mcp): pluggable Dispatcher + HTTPHandlerDispatcher for remote MCP (TASK-965)
Architectural prerequisite for PLAN-943's remote MCP at /mcp. The
existing ExecDispatcher (PLAN-942) shells out to the pad binary and
inherits credentials from ~/.pad/credentials.json — fine for local
stdio MCP where the user IS the subprocess owner, but unworkable for
a multi-tenant /mcp endpoint where the dispatcher must serve many
OAuth-authenticated users from a single process.
This PR ships the alternative path: HTTPHandlerDispatcher calls
pad-cloud's existing HTTP handler chain in-process, with the
requesting user attached via context. Same handlers, same audit /
event-bus / webhook plumbing — just no fork().
## What's in
- internal/mcp/dispatch.go: keeps the existing Dispatcher interface
(so ExecDispatcher unchanged) and adds a context-keyed
WithDispatchInput helper. The registry attaches the original JSON
input map to the dispatch context so dispatchers that prefer
structured data over reverse-parsed cliArgs can use it.
- internal/mcp/registry.go: forwards the merged input (user-supplied
values + session workspace + root flags) to the dispatcher via
WithDispatchInput. ExecDispatcher ignores it.
- internal/mcp/dispatch_http.go (new): HTTPHandlerDispatcher
implementation with a routeTable[cmdPath]→RouteMapper mapping. Seed
entry: `item create`. Adding more commands is one RouteMapper per
cmdPath plus a routeTable insert.
- internal/server/context.go (new): exported WithCurrentUser /
WithAPITokenAuth / WithTokenWorkspaceID + read-only
CurrentUserFromContext / IsAPITokenFromContext. Lets internal/mcp
synthesize an authenticated request without reaching the
package-private context keys.
- internal/server/middleware_csrf.go: extends the existing "Bearer
token requests skip CSRF" rule to also honor the ctxIsAPIToken
context flag. Same semantic — non-cookie auth means no CSRF risk —
but covers the in-process dispatch path where TokenAuth never sets
the Authorization header. Safe because ctxIsAPIToken can only be
set by trusted in-process code (TokenAuth on the live Bearer path,
or server.WithAPITokenAuth from the dispatcher).
## What's tested
- Unit:
- TestHTTPHandlerDispatcher_RoutesItemCreate — full happy path
with a recordingHandler asserting method/path/body/user-context.
- TestHTTPHandlerDispatcher_UnsupportedToolReturnsErrorResult —
tools not yet in the routeTable produce IsError-flagged results
rather than panicking.
- TestHTTPHandlerDispatcher_NoUserReturnsErrorResult — UserResolver
returning nil produces an IsError, never a nil-deref.
- TestHTTPHandlerDispatcher_HandlerErrorSurfacesAsToolError — 4xx
handler responses come back as IsError MCP results matching
ExecDispatcher's `pad <cmd> failed: <stderr>` format.
- mapItemCreate validation + parseFieldKVP variants.
- Integration: TestHTTPHandlerDispatcher_Integration drives the full
*server.Server (real chi router, real SQLite store, full middleware
chain) with a synthesized OAuth user and asserts the item lands in
the DB.
## Scope discipline
The DoD called for "dispatch item.create end-to-end" — that's the seed
entry. Wiring the remaining ~70 MCP-exposed commands into routeTable
is naturally a follow-up before TASK-950 ships /mcp to real users
(captured as a separate task post-merge).
Audit-log assertion in the integration test is deferred until TASK-960
(B6b) lands the audit log itself.
Parent: PLAN-943.
* fix(mcp): roll status/priority/category/parent into fields JSON per Codex review (round 1)
Codex caught: mapItemCreate placed status / priority / category /
parent at the top level of the JSON body, but handleCreateItem only
reads them after unmarshalling the Fields string from the request. As
written, MCP-driven `item create` would silently drop those flags —
breaking parity with the CLI for almost every realistic call (parent-
linked tasks, priority-set items, status-overridden ideas, etc.).
Mirrored the CLI's behaviour (cmd/pad/main.go ~L2200): build a fields
map from the named flags, overlay the repeatable --field entries on
top, JSON-encode into ItemCreate.Fields. The handler's existing
schema-validation + parent-resolution path now runs unchanged.
Repeatable --field still wins last-write — locked into a new test so
it doesn't drift.
Also rejects --assign / --role with a clear error rather than silently
dropping them. The CLI resolves user-name → user-ID and role-slug →
role-ID via additional API calls before posting; replicating that
pre-resolution belongs in a follow-up that expands the route table for
production use. Failing loudly is better than partial parity.
Tests:
- TestHTTPHandlerDispatcher_RoutesItemCreate now asserts the
status/priority/category/parent values land in fields, not the top
level — guards against the regression directly.
- TestMapItemCreate_ExplicitFieldOverridesNamedFlag locks the
last-write-wins precedence between --status and --field status=...
- TestMapItemCreate_RejectsUnsupportedAssignRole asserts the
defensive error path for the deferred flags.
Parent: PLAN-943.
* fix(mcp): persist source=cli for HTTPHandlerDispatcher calls per Codex review (round 2)
Codex caught: actorFromRequest derives source from the Authorization
header — without one, dispatcher-driven calls would persist
source="web" instead of source="cli", regressing dashboard/standup/
audit attribution vs. ExecDispatcher.
Same pattern as the round-1 CSRF fix: extend actorFromRequest to also
honor the ctxIsAPIToken context flag (which TokenAuth sets on the live
Bearer-auth path and HTTPHandlerDispatcher sets via
server.WithAPITokenAuth on synthesized requests). Both signals mean
"non-cookie authenticated, attribute as CLI/agent traffic".
Integration test now asserts source="cli" on the created item, so any
future regression of this attribution surfaces immediately.
Parent: PLAN-943.
* fix(mcp): normalize collection aliases in HTTPHandlerDispatcher per Codex review (round 3)
Codex caught: CLI's `item create task ...` works because
cmd/pad/main.go's normalizeCollectionSlug maps singular/short forms
("task" → "tasks", "doc" → "docs", etc.) to the canonical slug
before posting. HTTPHandlerDispatcher's mapItemCreate skipped that
step, so the same documented call shape would 404 through the HTTP
transport even though it worked through ExecDispatcher.
Extracted the alias map to internal/collections.NormalizeSlug so the
two transports stay in lockstep without duplication. cmd/pad/main.go's
normalizeCollectionSlug now delegates to it; the in-process
dispatcher calls it from mapItemCreate after pulling the collection
out of input.
TestMapItemCreate_NormalizesCollectionAliases locks every documented
alias plus a passthrough case for custom collections.
Parent: PLAN-943.
* fix(server): WithTokenWorkspaceID actually clears on empty input per Codex review (round 4)
Codex caught: the docstring said "Pass an empty string to clear" but
the implementation early-returned `ctx` unchanged in that case,
leaving any stale ctxTokenWorkspaceID set further up the chain
active. Always overwrite so the contract holds: passing "" produces
a context where tokenWorkspaceID(r) returns "", same as a never-set
context.
Parent: PLAN-943.
|
||
|
xarmian
|
e05ea07d62 |
fix(docs): use canonical wire path capabilities.experimental.padCmdhelp (#342)
Codex caught the same accuracy issue on pad-web that exists in five
spots in this repo: prose described the handshake location as
"serverCapabilities.experimental.padCmdhelp", but per the MCP spec
the InitializeResult shape is
{ result: { capabilities: { experimental: { ... } } } }
There's no `serverCapabilities` field on the wire — `ServerCapabilities`
is the Go-side struct type name in mcp-go; the JSON tag is
`capabilities`. Anyone copying the path out of our docs to navigate
a real JSON-RPC envelope was getting the wrong key.
Updated to `capabilities.experimental.padCmdhelp` (or the fully
qualified `result.capabilities.experimental.padCmdhelp` where the
JSON-RPC envelope context wasn't otherwise obvious) in:
- README.md — public-facing prose
- CLAUDE.md — agent-facing prose
- internal/mcp/version.go — discovery-surfaces doc comment + the
experimentalCapabilityKey doc comment
- internal/mcp/server.go — comment near WithExperimental
- internal/mcp/meta.go — experimentalCapabilities() doc + the wire
shape example (now wrapped under `result` for accuracy)
- internal/mcp/server_test.go — test docstring + failure message
- cmd/pad/mcp.go — comment near RegisterMeta
The Go type `serverCapabilities` in `internal/server/handlers_capabilities.go`
is unrelated (it's the response shape for `GET /api/v1/server/capabilities`)
and stays as-is.
No code/behaviour changes; pure prose accuracy fix. `make check` clean.
Companion fix to pad-web PR #42, which Codex flagged the same issue on.
|
||
|
xarmian
|
2d98f2a170 |
feat(mcp): advertise cmdhelp_version stability tier in handshake (TASK-963) (#340)
* feat(mcp): advertise cmdhelp_version stability tier in handshake (TASK-963)
External agents (Cursor, Claude Desktop, the future Pad Cloud remote MCP
in PLAN-943) depend on tool names, argument shapes, and resource URIs
being stable across pad releases. Without an explicit contract, any
future surface change breaks consumers silently.
This commit ships the contract on two complementary surfaces:
- serverCapabilities.experimental.padCmdhelp in the initialize handshake
— namespaced map carrying {version, tool_surface_stable}, discoverable
in one round-trip.
- pad://_meta/version static resource — full JSON document with
{pad_version, cmdhelp_version, tool_surface_stable, mcp_protocol_version}
for clients that prefer reading a typed payload.
CmdhelpVersion is pinned at "0.1" — the initial cmdhelp-derived surface
shipped in PLAN-942. Bump the major when tool names / arg shapes /
resource URIs change incompatibly.
Tests:
- TestServer_InitializeHandshake extended to assert the experimental
capability shape on the wire (not just the existence of the field).
- TestBuildMetaPayload_* lock the payload field names + fallback
behaviour.
- TestRegisterMeta_ResourceRoundTrip drives the resource through the
real HandleMessage path so a regression in the dispatcher would
surface as a test failure.
Docs:
- README's MCP section briefly mentions the contract surfaces.
- CLAUDE.md's MCP section gets a stability-contract paragraph + the new
resource URI.
- Public docs at getpad.dev/mcp/local will need a follow-up PR in the
pad-web repo (per CONVE-159) — captured at the end of TASK-963.
Parent: PLAN-942.
* fix(mcp): source MCP protocol version from mcp-go LATEST_PROTOCOL_VERSION per Codex review (round 1)
Codex caught: the local MCPProtocolVersion constant was pinned at
"2024-11-05", but mcp-go@v0.50.0 negotiates "2025-11-25" for clients
that request mcp.LATEST_PROTOCOL_VERSION. The meta resource was
therefore reporting a protocol revision newer than what the server
actually speaks, which defeats the field's purpose for feature
detection (e.g. RFC 8707 Resource Indicators land in 2025-11-25).
Drop the local constant and read mcp.LATEST_PROTOCOL_VERSION at
BuildMetaPayload time so the value tracks whatever revision the
linked library will negotiate. The handshake's serverInfo.version
already does this implicitly via NewMCPServer; making the meta
resource follow the same source-of-truth keeps both surfaces in
lockstep across mcp-go upgrades.
Test updated to assert against mcp.LATEST_PROTOCOL_VERSION instead of
the removed constant, plus an "empty-string" guard in case a future
library refactor unsets the constant.
Parent: PLAN-942.
|
||
|
xarmian
|
e90ee18907 |
feat(mcp): pad mcp install / uninstall / status (TASK-948) (#338)
* feat(mcp): pad mcp install / uninstall / status (TASK-948)
One-shot config writers for the three MCP-capable client apps:
pad mcp install claude-desktop # ~/.config/Claude/claude_desktop_config.json (linux)
pad mcp install cursor # ~/.cursor/mcp.json
pad mcp install windsurf # ~/.codeium/windsurf/mcp_config.json
pad mcp install --all # all three
pad mcp uninstall cursor # remove
pad mcp status # report install state
Implementation:
- internal/mcp/install.go (new) — Agent registry with per-OS path
resolvers (PathFor takes (home, goos) so tests inject); AddPadEntry
/ RemovePadEntry / HasPadEntry primitives that read-modify-write
JSON, preserving every entry except mcpServers.pad. Installer
façade with Home/GOOS overrides for tests.
- cmd/pad/mcp.go — three new cobra subcommands wired into mcpCmd:
install (no-args = status, --all = batch), uninstall, status.
Binary path resolved via os.Executable().
DOD coverage:
- Existing entries preserved (TestAddPadEntry_PreservesOtherServers
asserts both other mcpServers and unrelated top-level keys survive).
- Idempotent install (binary unchanged → modified=false).
- Update install (binary changed → modified=true).
- Idempotent uninstall (missing file / missing entry → no-op).
- Per-platform path resolution tested for linux + darwin.
- 16 unit tests including edge cases: empty/whitespace files,
malformed JSON rejected (no silent overwrite), case-insensitive
agent aliases.
Live verified end-to-end:
- HOME=/tmp/fakehome pad mcp install cursor → writes valid JSON
- pad mcp status → shows [x] Cursor with command path
- pad mcp uninstall cursor → leaves mcpServers:{} skeleton
Config file perms: 0600 (configs may hold credentials for OTHER
MCP servers; tighten on principle).
Parent: PLAN-942.
* fix(mcp): tighten install argument validation + chmod existing configs (Codex round 1)
Two findings on PR #338:
1. `pad mcp install` had no Args validator so cobra silently accepted
extras: `pad mcp install cursor windsurf` only installed Cursor.
Added cobra.MaximumNArgs(1) plus an explicit guard rejecting
`--all` combined with an agent name (those flows are
mutually exclusive).
2. os.WriteFile(path, data, 0o600) only honors the mode when CREATING
the file. A pre-existing 0644 config kept 0644 after the install,
defeating the security-tightening claim in the comment. Added an
explicit os.Chmod(path, 0o600) after writing; chmod failures are
stderr warnings, not hard errors (the data write already
succeeded; perms hardening is best-effort defense-in-depth).
New test TestAddPadEntry_TightensExistingFilePerms locks the
0600-after-install contract; live verified the cobra guards reject
both error cases with clean messages.
Parent: PLAN-942.
* fix(mcp): tighten perms on idempotent install path too (Codex round 2)
Codex caught: AddPadEntry's no-op early-return (when desired config
matches existing) skipped the chmod step from round 1's fix. So an
already-up-to-date 0644 config retained 0644 after re-running
`pad mcp install`.
Extracted tightenPerms() as a helper called from BOTH paths:
- writeJSONConfig (modified path) — chmod after write
- AddPadEntry's no-op return — chmod even when content is unchanged
Best-effort: chmod failures still emit a warning rather than failing
the install (the user's intent already succeeded; perms tightening is
defense-in-depth, not core functionality).
New test TestAddPadEntry_TightensPermsOnIdempotentNoop locks the
no-op-path contract.
Parent: PLAN-942.
|
||
|
xarmian
|
ca2fc04a5b |
feat(mcp): static prompts lifted from SKILL.md (TASK-947) (#337)
Four MCP prompts expose pad's high-value multi-step workflows so agents can prompts/get them as user-role system messages: pad_plan — draft + decompose a Plan pad_ideate — brainstorm + capture as items pad_retro — retrospective on a completed Plan pad_onboard — workspace onboarding / codebase scan Implementation: - internal/mcp/prompts.go (new) — RegisterPrompts(srv) + PromptBody accessor; sorted iteration for deterministic prompts/list ordering. - internal/mcp/prompts_data.go (new) — embedded body strings, lifted near-verbatim from skills/pad/SKILL.md "Multi-Step Workflows". - cmd/pad/mcp.go wires RegisterPrompts after the resources path. 7 unit tests including SKILL.md drift contract: - All four prompts registered + reachable via PromptBody - Each body has the standard "# Pad: <workflow>" heading - Unknown prompt name returns error - Lockstep: every prompt body contains its key SKILL.md CLI invocations (catches silent drift if SKILL.md is updated without bumping prompts) - skills/pad/SKILL.md still exists as the source-of-truth (catches rename / removal during refactors) Live smoke: prompts/list returns 4 prompts (with descriptions); prompts/get pad_plan returns 1225 chars of workflow text starting "# Pad: Plan workflow\n\nYou are helping the user...". Naming choice: `pad_plan` (snake_case) over `pad/plan` (slash form) — some MCP clients interpret slashes as namespace paths. Matches the tool-naming convention from TASK-945. Parent: PLAN-942. |
||
|
xarmian
|
342a564113 |
feat(mcp): read-only resource templates (TASK-946) (#336)
* feat(mcp): read-only resource templates for items / dashboard / collections (TASK-946)
Four MCP resource templates expose pad workspace state to agents
without requiring a tool invocation:
pad://workspace/{ws}/items/{ref} → single item markdown
pad://workspace/{ws}/items → list of items (JSON)
pad://workspace/{ws}/dashboard → project dashboard (JSON)
pad://workspace/{ws}/collections → collections + schemas (JSON)
Why resources, not tools: agents can `resources/read` a URI and
ingest the body directly into context without going through a
tool-call round-trip. Useful for "load TASK-5 then plan" workflows
where the agent shouldn't need to pick a tool.
Implementation:
- internal/mcp/resources.go (new) — RegisterResources installs all
four templates on an MCPServer; ResourceFetcher interface +
ExecResourceFetcher shell-out (separate from Dispatcher because
resource handlers return raw bytes, not CallToolResult).
- parsePadURI extracts (workspace, kind, arg) from pad:// URIs;
defensive guards reject mismatched URIs at each handler.
- rootFlagsToArgs forwards startup --url to every fetched call
(same contract as TASK-945's tool dispatch).
- cmd/pad/mcp.go wires RegisterResources after the tool registry
in mcpServeCmd.
15 new unit tests:
- parsePadURI: all 4 forms + 4 malformed inputs
- each handler: dispatches correct CLI args + MIME type
- readItem rejects mismatched URI (defensive)
- fetch errors propagate as Go errors (so MCP returns JSON-RPC
error rather than empty contents)
- root flag forwarding via the resources path
- ExecResourceFetcher: missing binary, stdout capture, non-zero
exit folds stderr into error
Live verified:
- resources/templates/list returns 4 templates with correct mime
types and uri patterns.
- resources/read pad://workspace/docapp/items/TASK-944 returns
1764 bytes of markdown.
Parent: PLAN-942.
* fix(mcp): compose full item markdown from JSON in resource path (Codex round 1)
Codex flagged: pad://workspace/{ws}/items/{ref} fetched
`pad item show --format markdown` which prints only item.Content
(see cmd/pad/main.go:2562). The resource description promised
"Full markdown content … includes title, fields, body, and links",
so clients reading the URI lost ref/title/metadata/parent and
couldn't reliably identify the item.
Fix scoped to the resource path (rather than changing the CLI's
markdown output, which other callers may parse): readItem fetches
`--format json` and a new formatItemAsMarkdown composes the
document — heading with ref + title, optional parent link, sorted
metadata fields, then the content body.
3 new unit tests + the existing readItem test rewritten:
- Full-shape JSON → exact markdown layout (deterministic via sorted keys)
- Missing fields → heading-only doc, no panic
- Empty `{}` fields → no stray list section
- Invalid JSON → error propagates
Live verified: pad://workspace/docapp/items/TASK-944 now returns
"# TASK-944: <title>\n\n**Parent:** PLAN-942 — ...\n\n- **priority:**
high\n- **status:** done\n\n<body>" — full identification + traversable
parent link, body intact.
Parent: PLAN-942.
|
||
|
xarmian
|
2e4a815d0c |
feat(mcp): cmdhelp-derived tool registry + shell-out dispatch (TASK-945) (#335)
* feat(mcp): cmdhelp-derived tool registry + shell-out dispatch (TASK-945)
The strategic centerpiece of PLAN-942: walk the cmdhelp Document built
from `pad`'s cobra tree and register every leaf as an MCP tool, with
shell-out dispatch back to the running binary. New pad commands (or
new flags) extend the MCP surface for free — no hand-mapping ~73
commands.
- internal/mcp/registry.go — Register() walks cmdhelp.Document, picks
leaves, applies a curated DefaultExcludes (db ops, auth, init,
agent install/update, server lifecycle, completion, edit, watch,
workspace lifecycle), builds an MCP Tool per leaf with input schema
derived from cmdhelp Arg/Flag types. Snake-case names: "item create"
→ "item_create".
- internal/mcp/dispatch.go — ExecDispatcher shells out to the pad
binary; BuildCLIArgs is a pure function that translates the JSON
args into a CLI invocation (positionals → flags → workspace
injection → --format json default). JSON stdout is surfaced as
StructuredContent for rich client rendering.
- internal/mcp/workspace.go — WorkspaceState (RWMutex-protected) +
pad_set_workspace built-in tool. Empty string clears the session
default; missing arg returns IsError without mutating state.
- cmd/pad/mcp.go — wire registry into `pad mcp serve` startup; build
the cmdhelp Document from cmd.Root(), resolve the running binary
via os.Executable, seed workspace from --workspace flag.
- 23 new unit tests across registry / dispatch / workspace files
(race-detector clean) covering: leaf identification, exclusion
prefix suppression, snake-case naming, pure CLI arg translation
(positionals + bool presence form + repeatable args & flags +
workspace/format injection), exec dispatcher (binary missing,
stdout capture, non-zero exit), workspace state mutation, and
end-to-end pad_set_workspace handler contract.
Live smoke (real binary, real stdio):
- tools/list returns 66 tools — pad_set_workspace + item_create
present, db_backup + mcp_serve correctly excluded.
- tools/call pad_set_workspace updates session state, then
auth_whoami shells out and returns structured JSON.
Parent: PLAN-942.
* fix(mcp): forward --url root flag + drop unwired --stdin (Codex round 1)
Two findings from Codex review of #335:
P1: --url root persistent flag was not forwarded to dispatched
subprocesses. If an MCP client launches `pad --url X mcp serve`,
every tool call ran against the default URL instead of X. Fixed by
adding RootFlags map[string]string to RegistryOptions; cmd/pad/mcp.go
captures urlFlag at startup and threads it through. BuildCLIArgs
now also takes a rootFlags map and injects each entry when not in
input (empty values skipped, agent value wins on collision).
P2: MCP tool schemas exposed `--stdin` flags but ExecDispatcher
never piped the agent's stdin to the subprocess. Calling e.g.
`item_create {stdin: true}` would block on EOF and create empty
content. The `--content` flag covers the same semantic via JSON
args, which IS wired. Hide stdin from the MCP surface (buildTool
filters out flagsHiddenFromMCP) AND drop it defensively in
BuildCLIArgs in case an agent's stale schema cache passes it.
Tests added (4 new + 2 updated):
- BuildCLIArgs: stdin dropped defensively, root flags injected,
empty root flag skipped, agent value wins over root flag.
- buildTool: omits stdin from input schema.
- Dispatch handler: forwards root flags through to CLI args.
Existing TestBuildCLIArgs_BoolPresenceForm rewritten to use
`dry-run` flag (since stdin is now filtered).
Live verified: `pad mcp serve` tools/list shows item_create with
content+10 other flags, no stdin. Round-trip preserved.
Parent: PLAN-942.
|
||
|
xarmian
|
9905a83134 |
feat(mcp): pad mcp serve skeleton on stdio (TASK-944) (#333)
Stand up internal/mcp + the cobra `pad mcp serve` subcommand. v1 is
handshake-only — the server completes initialize and stays alive over
stdio, advertising tool capability with an empty registry. TASK-945
fills that registry from `pad help --format json`.
- New internal/mcp package wraps mark3labs/mcp-go's stdio transport;
graceful shutdown on EOF / SIGINT / SIGTERM / ctx-cancel.
- New cmd/pad/mcp.go registers `pad mcp` as a top-level cobra group
with the `serve` subcommand wired to internal/mcp.NewServer.
- 4 unit tests: NewServer construction, real initialize round-trip
(asserts serverInfo.name + version), fallback version locked,
graceful shutdown on ctx-cancel.
Live smoke: `echo '<initialize>' | pad mcp serve` returns
`serverInfo:{name:"pad-mcp",version:...}` with `tools:{listChanged:true}`.
cmdhelp emits the new command tree at `pad help mcp serve --format json`.
Parent: PLAN-942.
|
||
|
xarmian
|
cfda4463e8 |
feat(cmdhelp): tests + golden contract + drift validator (TASK-938) (#332)
* feat(cmdhelp): tests + golden contract + drift validator (TASK-938)
The verification layer that turns cmdhelp v0.1 from "implementation"
into "stable contract." Three categories of tests, all running in
`go test ./...`:
1. Schema validation (cmdhelp.schema.json as CI gate)
- internal/cmdhelp/schema_test.go — synthetic tree's emitted JSON
validates after static walk, after dynamic resolution, and after
a no-workspace fallback.
- cmd/pad/cmdhelp_real_test.go — the REAL pad cobra tree's emitted
JSON validates against the published schema. Future regressions
caught: types outside the closed vocabulary, non-numeric exit_code
keys, flag names violating propertyNames, malformed cmdhelp_version.
2. Drift-prevention contract (spec §6 / §11 Q5)
- internal/cmdhelp/example_validation.go — ValidateExamples walks
every example's `cmd` string, tokenizes with shellSplit, resolves
non-flag tokens against the live cobra tree, and asserts every
--flag exists on the resolved command (or any ancestor for
persistent / inherited flags). Negate-flag form (`--no-cache`)
is recognized via the negation rule from spec §5.3.
- shellSplit handles double/single quotes, backslash escape, and
stops at unquoted pipeline boundaries (|, ;, &, >, <) so the
validator only checks the first command in a pipeline.
- ValidateBoolArity asserts no bool flag appears in valued form
(--flag=value) anywhere in its examples (spec §5.3).
- cmd/pad/cmdhelp_real_test.go runs both validators against the
real pad tree as CI gates.
- Negative tests in internal/cmdhelp/example_validation_test.go
prove the validator catches: typo'd flag (--priorty), unknown
command path, valued-form bool flag.
3. Capabilities form equivalence (spec §8)
- cmd/pad/cmdhelp_real_test.go — both forms (help --capabilities
and --cmdhelp-capabilities fallback) produce byte-identical
output. Side-effect-free guarantee verified by passing garbage
args alongside the fallback flag.
Refactors enabling the tests:
- cmd/pad/main.go: extract newRootCmd() so tests can build the real
cobra tree without running it. main() body shrinks to two lines.
- cmd/pad/main.go: extract handleCmdhelpCapabilitiesFallback() so the
fallback's side-effect-free contract is directly assertable instead
of requiring a subprocess.
Parser improvements driven by real-pad-tree drift findings:
- parseExamplesFromLong: strip same-line `# comment` annotations so
`pad foo --bar # one item's attachments` doesn't pollute Examples.
stripCommentIndex is quote-aware (# inside "..." or '...' is literal).
- main.go (github cmd): the Long had annotations on example lines
separated only by spaces (no `#`), which was malformed input. Fixed
to use `#` separators — caught by the drift validator on first run.
New deps:
- github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 — Go JSON Schema
validator supporting draft 2020-12 (matches the cmdhelp schema's $schema).
New helpers in internal/cmdhelp/:
- FindAndCompileSchema(startDir) walks up to locate
schema/cmdhelp.schema.json and returns a compiled schema. Reusable
by any consumer that wants to validate cmdhelp documents.
End-to-end on real binary:
- pad help --format json → 100 commands, schema-valid.
- All examples in pad's emitted output resolve against the live tree
(zero drift findings).
- pad help --capabilities byte-identical to pad --cmdhelp-capabilities.
- Adding a typo'd flag in any cobra Long block in cmd/pad MUST break
TestRealPadTree_ExampleDriftValidator. Verified by the negative
TestValidateExamples_DetectsTypoFlag.
make check clean. All 53 cmdhelp + cmd/pad tests pass.
Parent: PLAN-930.
* fix(cmdhelp): pass full token stream to cobra.Find per Codex review (round 1)
Codex round 1 caught: ValidateExamples stopped collecting the command
path at the first flag, so an example like
pad --workspace foo item create task --priority high
resolved to root, not `item create`. That meant `--priority` was
checked against root's flag set (where it doesn't exist) — false
positive — AND the validator silently missed any command-path drift
after a leading root flag.
Cobra's own Find walks the full token stream and uses each command's
flag definitions to skip flag/value pairs while matching subcommand
names. Pass tokens[1:] directly to root.Find — let cobra handle the
interleaving correctly.
New test:
- TestValidateExamples_FlagBeforeSubcommandResolvesToCorrectTarget —
flag-before-subcommand resolves to the leaf and accepts leaf flags.
|
||
|
xarmian
|
76c9d5aae5 |
feat(cmdhelp): parse Examples blocks from cobra Long as fallback (TASK-939) (#331)
The original TASK-939 ask was to migrate every cobra command's
"Examples:" block from Long into the dedicated Example field. Pad has
102 cobra commands; manually migrating each is a hundreds-of-lines
change with high regression risk and zero user-visible improvement
(cobra renders "Examples:" sections in Long identically to the Example
field — the difference is only machine-readability).
Higher-leverage approach: enrich the cmdhelp emitter to fall back to
parsing Long when the Example field is empty. One small, testable change
in internal/cmdhelp unlocks examples in cmdhelp output for every command
that already has an "Examples:" block — without touching any of the 102
command sites. Structural migration becomes optional polish (HT-941).
Implementation:
- internal/cmdhelp/json.go gains parseExamplesFromLong: locate a
stand-alone "Examples:" / "Example:" header, collect indented
invocation lines until blank-then-prose or end. Comment lines (`#`)
are dropped from the block. The header regex is anchored to a line
on its own (`^\s*Examples?:\s*$`) so prose containing the word
"Examples" doesn't trigger the fallback.
- buildCommand() prefers cmd.Example when set; falls back to
parseExamplesFromLong(cmd.Long) when Example is empty. Tests assert
precedence so future migrations to the Example field win cleanly.
- cmd/pad/main.go: completion command gets a dedicated Example field
(one of the few that didn't have an "Examples:" block at all). Demos
the migration pattern HT-941 will sweep across the rest.
Result on the real binary:
pad help --format json → before: 0/100 commands have examples
after: 24/100 commands have examples
pad help item create --format json → 4 examples (vs 0 before)
pad help completion --format json → 4 examples (from Example field)
The remaining ~76 commands genuinely lack an Examples: block in Long
(or are group commands that don't need examples). HT-941 captures the
sweep work needed to get those to 100%.
Tests:
- 7 new tests for parseExamplesFromLong in internal/cmdhelp/json_test.go:
basic block extraction, no-header (Usage: != Examples:), variant
headers (singular/plural, indented), empty/malformed inputs, stops
at unindented prose, drops comment lines.
- 2 new end-to-end tests via Build():
- falls back to Long when Example is empty
- prefers Example field when both set (precedence)
- All 39 existing cmdhelp tests + 16 routing tests still pass.
- make check clean.
Follow-up: HT-941 ("Migrate cobra Long Examples blocks to dedicated
Example fields") captures the structural sweep — broken into per-group
PRs (auth/*, agent/*, server/* etc.) so it can be done incrementally
without blocking PLAN-930.
Parent: PLAN-930.
|
||
|
xarmian
|
0439c1bf3d |
feat(cmdhelp): --capabilities discovery flag + --cmdhelp-capabilities fallback (TASK-937) (#330)
Implements the cmdhelp v0.1 §8 capability bit so wrappers can detect support without trial and error. - pad help --capabilities → cmdhelp/0.1: text, md, json, llm - pad --cmdhelp-capabilities → same line (spec §8 fallback form) Both forms: - Single line on stdout (terminated by newline only). - Side-effect-free: no logging, no network, no config writes, no auth challenge. Verified by running with no workspace context (cwd /tmp, no auth) — still emits the line and exits 0. - Exit 0 on success. - Format: cmdhelp/<MAJOR>.<MINOR>: <comma-separated formats>. Why both forms: Spec §8 lists `<cmd> help --capabilities` as preferred and `<cmd> --cmdhelp-capabilities` as a fallback for CLIs whose `help` subcommand is overloaded. Pad's `help` is not overloaded, but supporting both forms costs nothing and lets wrappers and harnesses choose whichever convention they prefer — TASK-938 will assert equivalence between them. The fallback is handled in main() before cobra parsing so it really is side-effect-free: it doesn't even reach config.Load() or the detect-workspace path. A simple os.Args scan + early return. Files: - internal/cmdhelp/json.go — new CapabilityLine(formats) helper that produces the spec-format string. Caller passes the format set so different binaries can advertise different surfaces; the helper preserves caller order (spec §8 says order isn't significant). - cmd/pad/help_cmdhelp.go — adds --capabilities to helpCmd; new padCmdhelpFormats constant ["text","md","json","llm"]; short-circuit in RunE before any other logic runs. - cmd/pad/main.go — pre-args scan handles --cmdhelp-capabilities fallback before rootCmd.Execute(). Tests: - TestCapabilityLine_FormatExact — exact string match. - TestCapabilityLine_HonorsCallerOrderAndSet — preserves caller order. - TestHelpCmd_CapabilitiesExactString — exact-byte assertion on the output of `padtest help --capabilities` including the trailing newline. - TestHelpCmd_CapabilitiesShortCircuits — verifies --capabilities wins over --format / --depth / extra args (spec §8 side-effect rule). - All 35 existing cmdhelp tests + 14 routing tests still pass. - make check clean. End-to-end on real binary: pad help --capabilities → "cmdhelp/0.1: text, md, json, llm" exit 0 pad --cmdhelp-capabilities → same cd /tmp && pad help --capabilities → still works, no auth needed pad help item --capabilities --format json --depth 0 → short-circuits Parent: PLAN-930. |
||
|
xarmian
|
e6fd25322e |
feat(cmdhelp): dynamic enum resolution + workspace context (TASK-936) (#329)
* feat(cmdhelp): dynamic enum resolution + workspace context (TASK-936)
The killer differentiator from the cmdhelp v0.1 spec — splice live
workspace facts into help output so an LLM asking "what collections
exist?" gets the real answer rather than a generic "any string".
Files:
- internal/cmdhelp/dynamic.go (new) — Resolver type with Apply method.
ArgEnumSources / FlagEnumSources map names to enum_source identifiers;
Sources maps enum_source to a fetcher func. Apply walks the Document,
stamps enum_source on matching args/flags, populates Enum from the
fetcher, and sets doc.Context.Workspace. Per-Apply caching keeps each
source func to ≤1 invocation regardless of how many commands need it.
- internal/cmdhelp/json.go — added Options.Resolver; Build calls
Resolver.Apply after the static walk, so callers that inspect Build
output as either pre- or post-resolution still work.
- cmd/pad/help_cmdhelp.go — newDynamicResolver constructs a Resolver
bound to the runtime: workspace from DetectWorkspace, server URL from
config, three sources (collections, roles, members). Returns nil when
no workspace is detected so help still works outside any workspace.
cmdhelpOptions takes target so Binary derives from root.Name() instead
of hardcoded "pad" (preserves test-tree bindings for synthetic roots).
Pad-side bindings (matches `pad item create --help`'s existing context):
arg collection → dynamic:pad collection list
flag role → dynamic:pad role list
flag assign → dynamic:pad workspace members
End-to-end on the real binary (inside docapp workspace):
pad help item create --format json
→ args[0].collection: type=enum, enum=[ideas,conventions,...,roadmap],
enum_source="dynamic:pad collection list"
→ flags.role: enum=[planner,implementer,reviewer]
→ flags.assign: enum=[dave]
→ context.workspace="docapp"
pad help --format md → "## Workspace context\n- workspace: `docapp`"
Outside any workspace (cd /tmp; pad help item create --format json):
→ collection arg: type=string, no enum, no enum_source (graceful fallback)
→ context: null
→ output still validates against schema/cmdhelp.schema.json
Fail-safe behavior:
- newDynamicResolver returns nil on any config/detection error → static doc.
- Per-source fetcher errors are caught inside Apply → enum_source still
announced on the binding arg/flag, but Enum is left empty. The help
command MUST NOT fail because dynamic facts can't be fetched.
- Existing Enum values from alternation/ValidArgs are preserved
(resolver only fills the gap, never overwrites authoritative spec).
Tests:
- 10 dynamic-resolver tests in internal/cmdhelp/dynamic_test.go
covering: arg + flag enum population, context population, per-source
caching across multiple commands, graceful error handling, nil
resolver as no-op, existing-Enum preservation, global flag resolution,
unaffected commands left unchanged, end-to-end via Build().
- All 24 prior cmdhelp tests + 12 routing tests still green.
- make check clean (lint + go test + web build).
Out of scope (deferred):
- --capabilities discovery flag — TASK-937.
- Schema-validate live output in CI — TASK-938.
- Audit pad's existing commands' Examples — TASK-939.
Parent: PLAN-930.
* fix(cmdhelp): scope --role / --assign bindings per-command per Codex review (round 1)
Codex round 1 on PR #329 caught a real semantic bug: globally binding
--role to dynamic:pad role list was wrong because pad has two
unrelated --role flags:
pad workspace invite --role workspace role: owner|editor|viewer
pad item create --role <slug> agent role slug
pad item update --role <slug> agent role slug
Globally announcing agent-role slugs as the values for `pad workspace
invite --role` would mislead consumers (LLMs would suggest "planner"
when "owner" is expected; tab-completion would offer the wrong set).
Fix:
- Resolver gains CommandArgBindings and CommandFlagBindings
(map[path]map[name]source) — scoped to a specific command path.
Per-command bindings win over wildcard ArgEnumSources/FlagEnumSources
when both match.
- Helper methods argSource(path,name) / flagSource(path,name) own the
precedence rule so both args and flags use it consistently.
- newDynamicResolver in cmd/pad keeps `<collection>` as a wildcard
ArgEnumSources (universal — every <collection> in pad means a pad
collection), but moves --role and --assign into CommandFlagBindings
scoped to "item create", "item update", and "item list". `pad
workspace invite --role` is intentionally left without a binding.
- A header comment in newDynamicResolver enumerates every --role /
--assign site in the CLI and which one each binding targets, so a
future reviewer adding a new flag can see the rule at a glance.
End-to-end on the real binary:
pad help item create --format json
→ flags.role: type=enum, enum=[planner,implementer,reviewer], enum_source=...
pad help workspace invite --format json
→ flags.role: type=string (untouched). ✓
New tests:
- TestResolver_Apply_PerCommandBindingScoped — explicitly mirrors the
Codex finding: two commands both have a `role` flag, only the bound
command resolves. workspace-invite-style isolation regression test.
- TestResolver_Apply_PerCommandWinsOverWildcard — precedence: when
both wildcard and per-command match, per-command wins.
- TestResolver_Apply_PerCommandArgBindings — same precedence rule
for positional args.
All 33 cmdhelp tests + 12 routing tests still green; make check clean.
* fix(cmdhelp): bind item list --role to agent roles per Codex review (round 2)
Codex round 2 caught that item list --role was still unbound — I missed
it in round 1's grep because the variable name is `&roleFilter` rather
than `&roleFlag`. Pad has 4 --role flags total:
pad workspace invite --role workspace role (NOT bound)
pad item create --role agent role slug (bound)
pad item update --role agent role slug (bound)
pad item list --role agent role filter (now bound)
Fix: extend CommandFlagBindings["item list"] to include the same
itemRoleAssign map as item create/update, so all three item subcommands
that reference an agent role get the dynamic binding.
Added a `grep` recipe in the comment so a future maintainer adding a
new --role / --assign site can find every existing one in one shot
(both `&roleFlag` and `&roleFilter` style declarations).
End-to-end on real binary (inside docapp workspace):
pad help item list --format json
→ flags.role: enum=[planner,implementer,reviewer], enum_source set ✓
→ flags.assign: enum=[dave], enum_source set ✓
make check clean.
|
||
|
xarmian
|
5e93abe552 |
feat(cmdhelp): implement --format md emitter (TASK-935) (#328)
Adds internal/cmdhelp/md.go that renders the Document built in TASK-934
as markdown with the predictable section order from cmdhelp v0.1 §6.
Replaces the markdown stub in cmd/pad/help_cmdhelp.go so `pad help
--format md` (and the `--llm` alias) produce real output.
Section order per command (spec §6):
## `binary path`
summary / description (when distinct from summary)
### Synopsis — fenced usage line, reconstructed from args + flags
### Arguments — table with name | type | required | description
### Flags — table with flag | type | default | description
### Stdin — when Stdin.Accepted is true
### Examples — fenced bash blocks, drawn from same canonical
example set as JSON (spec §6 same-source rule)
### Output — text_template + json_schema_ref when populated
### Exit codes — table when ExitCodes is populated
### See also — bullet list of related command paths
Top-level YAML frontmatter:
cmdhelp_version, binary, version, generated (RFC3339, UTC).
Now is overridable via Options.Now for snapshot-test stability.
Top-level structure: `# binary` heading, summary, optional homepage,
optional `## Workspace context` (populated in TASK-936), `## Global flags`
table, then per-command sections sorted by path for determinism.
Synopsis reconstruction uses the structured Args from Build() (rather
than cobra.UseLine) so JSON and MD stay driven by the same parsed data
— the variadic `<ref>...` and alternation enums from TASK-934 carry
through naturally.
Pipes in flag/arg descriptions are escaped (`\|`) so they don't break
markdown table grids.
cmd/pad/help_cmdhelp.go: emitCmdhelpMarkdown stub replaced with a call
into cmdhelp.EmitMarkdown. --depth/--all threaded through MaxDepth
identically to the JSON path.
Tests:
- 15 markdown emitter tests in internal/cmdhelp/md_test.go covering
frontmatter (presence + timestamp injectability), per-command
section order, synopsis reconstruction (incl. variadic + alternation),
global-flag dedup, fenced-bash examples, hidden-thing exclusion,
deterministic ordering, Stdin/Output/ExitCodes/SeeAlso sections,
Workspace context, table-pipe escaping.
- TestHelpCmd_FormatMarkdownStubError replaced by
TestHelpCmd_FormatMarkdownEmits (asserts frontmatter + structural
markers for both md and llm).
- TestHelpCmd_FormatLLMAliasRoutesToMarkdown replaced by
TestHelpCmd_FormatLLMIsAliasForMD (asserts md and llm produce
byte-identical output modulo the timestamp).
End-to-end on the real binary:
- pad help --format md emits valid markdown with all sections.
- pad help --format llm produces byte-identical output (after
timestamp normalization).
- pad help item create --format md scopes correctly.
- make check clean.
Out of scope (deferred):
- Dynamic Workspace context population — TASK-936.
- --capabilities discovery flag — TASK-937.
- Schema-validation + golden-file tests in CI — TASK-938.
- Examples populated for all pad commands (still in cobra Long for now)
— TASK-939.
Parent: PLAN-930.
|
||
|
xarmian
|
eecc683ab0 |
feat(cmdhelp): implement --format json emitter (TASK-934) (#327)
* feat(cmdhelp): implement --format json emitter (TASK-934) Adds internal/cmdhelp package that walks the cobra command tree and emits a cmdhelp v0.1 Document conforming to schema/cmdhelp.schema.json. Wires it into cmd/pad/help_cmdhelp.go so `pad help --format json` is no longer a stub. Files: - internal/cmdhelp/types.go — Document/Command/Arg/Flag/Stdin/Stdout/ ExitCode/Example structs mirroring the schema. ExitCode implements custom MarshalJSON for the string-or-object union (spec §5.2). - internal/cmdhelp/json.go — Build() walks target's subtree; EmitJSON() serializes to indented JSON. Type mapping covers pflag's full type space, including slice/array→repeatable. Hidden commands and flags filtered. Cobra's auto-installed --help flag suppressed. Zero-default values suppressed to keep output compact. argRE parses positional arg placeholders from cobra Use strings, filtering [flags]/[options]/ [command] cobra conventions. parseExamples splits cmd.Example by newline, drops blanks and # comments. MaxDepth maps to spec §4 semantics: 0 = subcommand list, 1 = + grandchildren, -1 = unlimited. - cmd/pad/help_cmdhelp.go — replaces emitCmdhelpJSON stub with a call into the package; threads --depth and --all through MaxDepth (--all overrides --depth). Verification: - 17 emitter tests in internal/cmdhelp/json_test.go covering envelope, global-flag emission, hidden-thing exclusion, positional arg parsing, pflag type mapping, zero-default suppression, example parsing, description-vs-summary, command-path key shape, MaxDepth semantics, target-subtree scoping, JSON validity, version pattern, ExitCode union marshaling, parseExamples filtering. - 11 cmd/pad routing tests still pass; TestHelpCmd_FormatJSONStubError replaced by TestHelpCmd_FormatJSONEmits which validates the structure. - End-to-end: `pad help --format json` on the real binary emits 100 commands across the full tree; output validates against schema/cmdhelp.schema.json (verified with python jsonschema). - `pad help item --format json` correctly limits output to 29 commands in the item subtree (homepage and other top-level metadata still populated from root). - `pad help --format json --depth 0` correctly emits 15 immediate children of root, no grandchildren. - make check clean (lint + go test + web build). Out of scope (deferred): - Examples: pad's existing commands embed examples in Long rather than using cobra's Example field. The emitter correctly reads Example; TASK-939 will normalize the pad-side commands to populate it. - Dynamic enum injection (workspace-aware enums): TASK-936. - --capabilities discovery flag: TASK-937. - Schema-validation of live output in CI: TASK-938. Parent: PLAN-930. * fix(cmdhelp): handle alternation, variadic, and ValidArgs in Use parser per Codex review (round 1) Codex round 1 on PR #327 flagged that parseArgs missed two real cobra Use-string idioms in pad's command tree: 1. `completion [bash|zsh|fish|powershell]` — alternation in brackets. The old regex only allowed `[a-zA-Z0-9_./-]+` inside brackets, so the `|` made the whole token unmatched and the shell arg disappeared from the emitted JSON. Consumers asking "what does completion take?" got nothing. 2. `item bulk-update [--status X] <ref>...` — variadic ellipsis. Old regex didn't capture trailing `...`, so the arg was emitted but without `repeatable: true`. Consumers couldn't tell that <ref> may be passed multiple times. Fixes: - argRE now allows full-bracket content (`[^<>]+` / `[^\[\]]+`) and captures an optional trailing `...` group. - parseArgs takes *cobra.Command (not just Use string) so it can read cmd.ValidArgs and attach those values as the first arg's enum when set. This covers `Use: "completion [shell]"` + `ValidArgs: [...]` where the allowed values only live on the cobra struct. - Alternation `<a|b|c>` / `[a|b|c]` produces an enum-typed arg with the values as `Enum`. When Use carries no semantic name (only the alternation), the arg name is synthesized as "value". - New validArgName check rejects embedded flag-like fragments such as `[--status X]` and prose with whitespace/punctuation that the broader regex would otherwise capture from idiosyncratic Use strings. - ValidArgs entries strip cobra's tab-separated completion descriptions before becoming enum values. New tests: - TestBuild_VariadicArgsRepeatable — `<ref>...` → repeatable=true. - TestBuild_AlternationProducesEnum — `[bash|zsh|fish|powershell]` → enum with values in source order. - TestBuild_ValidArgsFillsEnumOnNamedArg — Use says `[shell]`, ValidArgs carries the values → enum-typed arg named `shell`. - TestBuild_EmbeddedFlagFragmentsFiltered — `[--status X]` does not leak as a positional arg. Verified on the real binary: - `pad help completion --format json` now emits the shell enum. - `pad help item bulk-update --format json` now marks <ref> repeatable. - `pad help --format json` still validates against the schema (100 cmds). - `make check` clean. |
||
|
xarmian
|
e2b19d393f |
feat(cli): wire pad help subcommand + scope levels (TASK-933) (#326)
Replaces cobra's built-in `help` with a custom subcommand implementing the cmdhelp v0.1 mandatory surface (https://getpad.dev/cmdhelp; IDEA-927): pad help [subcommand…] [--format <fmt>] [--depth <n>] [--all] Routing: - --format text (default): delegates to cobra's text renderer so the existing --help UX is byte-for-byte unchanged. - --format json|md|llm: routes to cmdhelp emitters. JSON and markdown emitters are stubs that return clear "not yet implemented" errors pointing to TASK-934 / TASK-935; this lets the routing layer ship and be tested independently of the emitter work. - --depth <n> and --all are accepted at this layer; their effect lives in the JSON/MD emitters. - llm is a renderer-level alias for md per spec §3. Scope levels (spec §4): - pad help → root tree summary (text mode delegates to cobra) - pad help <group> → subtree (e.g. `pad help item`) - pad help <group> <leaf> → single command, deep Tests (cmd/pad/help_cmdhelp_test.go): - 11 cases covering all routing branches: default text, group scope, leaf scope, --format text == default, --format json stub references TASK-934, --format md/llm stubs reference TASK-935, llm aliases md, unknown --format rejected, unknown topic rejected, --depth/--all accepted, CmdhelpVersion is MAJOR.MINOR. Verification: - All 11 new tests pass. - `make check` clean (lint + go test + web build). - Manual smoke-test on built binary: 13 paths exercised — group/leaf routing, format aliases, error paths, existing `pad <cmd> --help` unchanged. The JSON/MD emitters land in TASK-934/935. --capabilities discovery is a separate concern (TASK-937). Parent: PLAN-930. |
||
|
xarmian
|
5e27989ab8 |
feat(attachment): add pad attachment view|show|list CLI surfaces (IDEA-898) (#321)
* feat(attachment): add `pad attachment view|show|list` CLI surfaces (IDEA-898) Agents and CLI users had no first-class way to fetch attachment bytes through the API: the only path to read an `` reference was to read the raw blob out of `~/.pad/attachments/<storage_key>`, which bypasses workspace ACLs, doesn't work on Pad Cloud / remote / Postgres deployments, skips the variant pipeline (TASK-872 / TASK-879 / TASK-880), and breaks when storage moves to S3. Three new subcommands wrap the existing REST endpoints: - `pad attachment view <id> [-o path]` — agent-friendly: with no `-o`, fetches to a fresh OS temp directory using the stored filename and prints just the absolute path on stdout (so `$(pad attachment view <id>)` composes cleanly into shell pipelines). Reuses `download`'s atomic temp-then-rename pattern via a shared helper. - `pad attachment show <id>` — HEAD-based metadata only; surfaces MIME, size, filename, ETag, Last-Modified. - `pad attachment list [--item REF] [--category X] [--attached|--unattached] [--collection ID] [--sort ...] [--limit N] [--offset N]` — workspace list. The `--item REF` flag resolves a TASK-5-style ref to a UUID client-side and passes it to a new `item_id` query param on the list endpoint (server side: AttachmentListFilters.ItemID, ~6 lines in the store + 1 in the handler). Skill update: `skills/pad/SKILL.md` gains a "Working with attachments" subsection plus a CLI Reference entry, both ending in the hard rule that agents must NEVER read directly from `~/.pad/attachments/`. * style(cli): gofmt AttachmentListParams field alignment CI's golangci-lint v2 flagged this with the gofmt formatter (configured with simplify: true in .golangci.yml). The contiguous Sort/Limit/Offset block at the end of the struct needs uniform column alignment — gofmt considers the doc comment above Sort attached to that field rather than a block separator, so the three int/string fields get aligned together. Verified locally with `golangci-lint run --timeout=5m ./...` (v2.11.4 to match CI) — 0 issues. Local make lint only runs `go vet ./...` and `golangci-lint` wasn't installed, which is why this slipped through; filing a separate follow-up to mirror the CI checks in the local workflow. |