mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-11 21:39:01 +00:00
9ebdfb503e12ca34fc148c120fd45e657f7d7f65
147 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
9ebdfb503e |
Revert "feat(cli): pad session shape — Claude Code context-window telemetry (IDEA-1491) (#569)" (#570)
This reverts commit
|
||
|
xarmian
|
351f83af3f |
feat(cli): pad session shape — Claude Code context-window telemetry (IDEA-1491) (#569)
* feat(cli): pad session shape — Claude Code context-window telemetry (IDEA-1491)
New `pad session shape [--session <id|path>] [--format json|table|markdown]`
command that reads the active Claude Code session JSONL and reports tokens,
context_pct (vs hardcoded per-agent-version budget), message counts, and
elapsed time. Default format is JSON because agents are the primary caller.
- internal/cli/claudecode.go: project-slug derivation, JSONL streaming
parser, env/cwd/autodetect cascade resolver, per-version budget table
(seeded with 2.1.* → 1M tokens per the IDEA-1491 recon update).
- cmd/pad/session.go: top-level `session` group + `shape` subcommand,
three output formats, registered via cmd/pad/main.go's rootCmd.
- Tests cover slug derivation (live ~/.claude/projects/ verified cases),
JSONL parse (normal / no-usage / sidechain fixtures), budget lookup,
context-class bucketing, and the resolver cascade in t.TempDir().
Sidechain/sub-agent JSONL summing and the IDEA-body Pad-invocation-count
fallback are intentionally deferred to v2 (TODOs in session.go).
* fix(cli): session shape — TotalPrompt as context numerator + count parallel tool_use (IDEA-1491)
Codex R1 review findings:
P1 — context_pct numerator was CacheRead, which is a steady-state proxy
that under-counts at turn boundaries when fresh content sits in
cache_creation/input before being folded into the cached prefix. The
correct denominator-of-budget is the full prompt footprint sent this
turn: cache_read + cache_creation + input = TotalPrompt. Markdown
renderer's context-tokens line follows suit; the explicit per-component
breakdown rows keep CacheRead so the components remain visible.
P2 — ToolInvocations was counting assistant-turns-with-any-tool-use, not
tool invocations. A single assistant turn can emit multiple parallel
tool_use blocks in message.content[]; the field name promises a count
of invocations. Drop the early break.
Test fixture normal.jsonl gains a 3-parallel-tool-use final turn; assert
ToolInvocations==4 (up from 2) to pin the new behavior.
* fix(cli): session shape — path-traversal, oversized-line, content-variant, explicit-flag errors (IDEA-1491)
Codex R2 review findings:
P1 — session-ID inputs (--session flag-id branch AND
$CLAUDE_CODE_SESSION_ID env var) are now validated before they become
filename fragments under ~/.claude/projects/<slug>/. Reject path
separators, '..' segments, and anything that doesn't match a UUID-ish
shape (hex+dashes, 8+ chars). New ErrInvalidSessionID wraps a
descriptive message. Without this, `--session ../../../../../tmp/foo`
or a poisoned env var could escape the projects dir on the candidate
os.Stat. End-to-end smoke confirms `pad session shape --session
'../../../etc/passwd'` now errors with "invalid session id: ...
contains a path separator" and exits non-zero.
P2a — Switch the JSONL line reader from bufio.Scanner (8 MiB cap, hard
fail on overflow with "token too long") to bufio.Reader.ReadBytes('\n')
(no cap). encoding/json has no size limit either, so oversized records
— e.g. inline file attachments — parse cleanly. Both ParseSessionJSONL
and tailLineCWD updated for parity.
P2b — jsonlLine.Message.Content was []json.RawMessage at the outer
decode, which made a schema variant where content is a string or
object fail the WHOLE line's decode, losing type/timestamp/version/
usage data. Now Content is a raw json.RawMessage; the tool-use scan
re-decodes it as []json.RawMessage on a best-effort basis and skips
tool-counting when that fails, while preserving the rest of the line.
P3 — `pad session shape --session <id|path>` no longer silently falls
back when the resolver errors. An explicit flag means the caller has a
specific session in mind; a typo or wrong UUID should fail loudly so
automation surfaces the bug instead of emitting `agent: "unknown"`.
The implicit (no-flag) path still falls back for non-Claude-Code
harnesses.
Tests added/extended:
- TestResolveSessionLog_RejectsPathTraversal — flag-id AND env-id
branches, full bad-input matrix.
- TestParseSessionJSONL_OversizedLine — 10 MiB single record.
- TestParseSessionJSONL_NonArrayContent — content-as-string variant
must still contribute timestamp/version/usage.
- TestParseSessionJSONL_ParallelToolUse — tight hermetic check of the
R1 P2 multi-tool-per-turn fix.
- TestBuildSessionShape_ExplicitFlagErrors — verify --session errors
propagate.
- TestBuildSessionShape_ImplicitFallback — verify implicit path
still falls back.
|
||
|
xarmian
|
7c663a3d3f |
feat(collections): add blank workspace template + retire auto-upgrade hook (IDEA-1479) (#560)
* feat(collections): add blank workspace template (IDEA-1479)
Introduces a `blank` workspace template that seeds only the two system
collections (Conventions, Playbooks) — no Tasks/Ideas/Plans/Docs, no
seeded items, no starter conventions or playbooks. Solves the
agent-self / non-template-fit use case where the existing software
templates leave undeletable ghost collections in the workspace.
Adds a new `CategoryCustom` ("Custom") top-level category so the blank
template doesn't mis-group with `startup` / `scrum` / `product`.
Category is appended last in `CategoryOrder` so it doesn't displace
recommended-path templates in the picker.
Tests:
- TestBlankTemplateShape — exactly 2 system collections, no seeds.
- TestBlankTemplateExcludesSoftwareCollections — no tasks/ideas/plans/docs.
- TestBlankTemplateAppearsInPicker — surfaces under a Custom group.
- TestSeedFromBlankTemplate — bootstrapping produces 2 collections, 0 items.
* fix: address codex review for blank template (IDEA-1479)
- CreateWorkspaceModal: remove hard-coded 'blank' picker entry that
silently fell through to collections.Defaults(). The API-driven blank
template (under the Custom category) is now the canonical surface.
- Dashboard: gate '+ New Task' button on tasks collection existence so
blank workspaces don't render a button that targets a missing
collection.
- OnboardingChecklist: accept collectionSlugs prop and filter steps
whose target collection (plans/tasks/docs) is absent. Conventions
step remains unconditional since the conventions collection ships
with every template, including blank. Empty-steps guard added to
progressPct to avoid NaN.
- web/src/lib/utils/templates.ts: add 'custom' -> 'Custom' to mirror
the Go CategoryOrder + categoryLabels updates.
- cmd/pad/templates_picker_test.go: extend the visible-template
assertion list to include 'blank' and assert the Custom category
header renders.
* fix(store): gate SeedDefaultCollections on zero-collection workspaces (IDEA-1479)
The server's startup auto-upgrade hook (cmd/pad/main.go) called
SeedDefaultCollections against every workspace at boot. That hook
dates to the initial release — long before workspace templates
existed — and was written as a backfill for workspaces created
before tasks/ideas/plans/docs landed in Defaults().
Post-templates, the hook unconditionally re-materialized the
Software-template collections into any workspace missing them —
including blank-template workspaces (IDEA-1479), which ship only
Conventions + Playbooks by design. Result: every restart silently
regrew the ghost user-facing collections the blank template was
explicitly built to avoid.
Fix: SeedDefaultCollections now returns nil immediately when the
workspace has any existing collection (system or user-facing). The
rescue path still triggers for genuinely-empty workspaces, preserving
the original backfill intent.
Tests:
- TestBlankWorkspaceSurvivesSeedDefaultCollections — blank workspace
remains 2 collections after auto-upgrade (and after a second pass).
- TestEmptyWorkspaceStillGetsDefaults — zero-collection workspace
still gets the full Software default set.
* refactor(server): remove SeedDefaultCollections auto-upgrade at startup (IDEA-1479)
The startup auto-upgrade hook in cmd/pad/main.go dated to the initial
release, predating workspace templates entirely. Its original intent
was per-collection backfill — workspaces created before a new entry
landed in Defaults() would acquire it on next boot. Post-templates,
that semantic is incompatible with templates that legitimately
diverge from Defaults() (e.g. `blank`, which ships only Conventions
+ Playbooks by design).
Round-2 of the IDEA-1479 review attempted to keep the hook by adding
a "zero collections" guard, but Dave (after codex round 3) decided
the cleanest fix is removing the hook entirely. The codebase has
proper migration infrastructure now; any future "add a default
collection" work should land as an explicit migration where the
author chooses which workspaces to backfill.
SeedDefaultCollections itself is preserved (with the round-2 guard)
as a building block for any future explicit rescue command or
migration. Its doc comment is updated to note it's no longer
auto-invoked at startup. The round-2 regression tests
(TestBlankWorkspaceSurvivesSeedDefaultCollections,
TestEmptyWorkspaceStillGetsDefaults) still apply and pass unchanged.
* fix(store): rescue gate uses COUNT(*), not ListCollectionsMinimal (IDEA-1479)
Postgres CI on PR #560 caught a regression introduced in commit
|
||
|
xarmian
|
de8679f535 |
chore(mcp): bump ToolSurfaceVersion 0.3 → 0.4; document v0.4 envelope (TASK-1418) (#544)
* chore(mcp): bump ToolSurfaceVersion 0.3 → 0.4; document v0.4 envelope (TASK-1418)
Final PR of PLAN-1410. The contractual announcement that the v0.4
bootstrap shape is stable.
## What
1. internal/mcp/version.go — ToolSurfaceVersion: "0.3" → "0.4".
The godoc on the constant gains a full v0.4 changelog entry
enumerating each shape change shipped by PLAN-1410's six
bootstrap PRs:
- BootstrapCollection projection (TASK-1412): drops id,
workspace_id, created_at, updated_at, settings; schema as
a nested JSON object.
- BootstrapRole projection (TASK-1423): drops id,
workspace_id, tools, created_at, updated_at.
- Convention slug dropped (TASK-1413).
- Top-level recent_activity duplicate removed (TASK-1413).
- BootstrapDashboard wrapper caps five sub-arrays (TASK-1413
+ TASK-1422): attention, recent_activity, active_items,
active_plans, by_role at 5 entries each, parallel
*_overflow_count fields. suggested_next deliberately
excluded — already capped to 3 upstream.
- Schema label omitted when label == TitleCase(key) (TASK-1424).
Plus an explicit compatibility note: all v0.4 changes are
additive or subtractive (no field renames); clients that read
the preserved field names keep working unchanged.
2. CLAUDE.md updates:
- "## MCP server" header: v0.3 catalog → v0.4 catalog, with a
one-paragraph summary of what v0.4 shipped.
- "Surface:" Tools bullet: v0.3 → v0.4, with a note that the
tool/action surface is unchanged — only the bootstrap JSON
these tools return has been trimmed.
- "Stability contract": ToolSurfaceVersion (currently "0.4"),
comprehensive single-paragraph description of the v0.4
envelope, cumulative size reduction (40% live / 54% fixture),
and explicit additive/subtractive note.
## Why the strategy worked
PLAN-1410's "version bump last" strategy paid off:
- Each individual shape PR (TASK-1412/1413/1422/1423/1424) was
reviewable in isolation against a stable v0.3 contract.
- The six skill-side PRs (TASK-1414/1415/1416) had no MCP-shape
impact and didn't need any version bump consideration.
- v0.4 is now announced as a single comprehensive contract change,
not five separate version bumps — easier for downstream MCP
consumers (Claude Desktop, Cursor, future Pad Cloud remote MCP)
to reason about.
## Verification
- `make check` — golangci-lint 0 issues, all Go tests pass
(including the version-tracking tests in catalog_meta_test.go
that auto-pin to whatever ToolSurfaceVersion is set to),
govulncheck clean, web build clean.
- MCP handshake (verified via `pad mcp serve` + an initialize
JSON-RPC request) advertises
capabilities.experimental.padToolSurface.version = "0.4".
padCmdhelp.version stays at "0.1" as expected.
## Post-merge follow-ups
After this lands:
- Update PLAN-1410's Result section with a "v0.4 announced" line
and the final post-everything measurement (taken against
docapp after `make install`).
- Flip PLAN-1410 status from `active` → `completed`.
These are pad-item operations, not git changes.
Parent: PLAN-1410. Closes the plan.
* fix(mcp): update stale v0.3 references after ToolSurfaceVersion bump (TASK-1418 follow-up)
Address Codex P2 + P3 findings on PR #544: bumping
ToolSurfaceVersion in version.go left four runtime/user-facing
docs still claiming v0.3:
P2 — runtime MCP docs:
- internal/mcp/instructions.md "## Tool surface (v0.3)" → v0.4
- internal/mcp/catalog_meta.go "v0.3 server-introspection tool" → "(v0.4 catalog)"
- internal/mcp/catalog_meta.go padMetaToolDescription twice:
* "the v0.3 tool catalog" → "the v0.4 tool catalog"
* "v0.3 catalog dump" → "v0.4 catalog dump"
- internal/mcp/catalog_meta.go actionMetaToolSurface godoc:
"v0.3 catalog" → "catalog" (de-versioned; the comment is
about scope, not version)
P3 — public README:
- README.md "Tool catalog (v0.3)" → "Tool catalog (v0.4)"
- README.md "tool_surface_version: '0.3'" → "'0.4'" with a
pointer to PLAN-1410's bootstrap-trim summary and
version.go's full v0.4 changelog.
Without these, agents reading the initialize-instructions blob or
pad_meta's tool description (both of which are part of the
runtime MCP surface, not just internal docs) would see v0.3 while
the handshake / pad_meta.action: version returned v0.4 — the
exact "contradictory metadata depending on what you read" failure
mode Codex flagged.
Same skill-↔-code sync pattern that has been a running theme
through PLAN-1410's review loops. The cluster of stale references
is a classic side effect of a version bump landing late in a
plan — the version constant is one string, but downstream prose
that names it lives in multiple places.
Verified no remaining "v0.3" claims that imply currency — `grep -rn
"v0\.3\|tool_surface_version" --include="*.{go,md}"` returns only
historical-context mentions in changelog godocs (correct) and the
runtime constant readback (correctly returns "0.4" now).
Parent: PLAN-1410 / TASK-1418.
* fix(mcp): correct schema-type-change disclosure + stale cmdhelp-walker description (TASK-1418 follow-up)
Address Codex round 2 P3 findings on PR #544:
## P3 — `cmd/pad/mcp.go` still described the retired leaf walker
The `pad mcp serve` command's Long description said "every leaf
command becomes an MCP tool, except the curated allow-list
exclusions" — that was true under v0.1 but the cmdhelp leaf
walker was retired in TASK-981 (PLAN-969's v0.2 rollout). The
v0.2/v0.3/v0.4 surface has always been the hand-curated catalog
of eight resource × action tools + pad_set_workspace.
Updated the Long description to:
- Name the v0.4 catalog explicitly.
- List the eight resource × action tools.
- Note that cmdhelp v0.1 still drives per-command arg schemas
at dispatch time (so it's not gone, just no longer drives
tool naming/count).
- Reference TASK-981 for the cutover.
## P3 — "additive/subtractive only" was misleading
The compatibility note in `version.go` and `CLAUDE.md` claimed
all v0.4 changes were additive or subtractive. That glossed over
one breaking change in TASK-1412: `collections[].schema` went
from a JSON-encoded string ("schema":"{\"fields\":...}") to a
nested JSON object ("schema":{"fields":...}). For any v0.3
consumer that read schema as a string and JSON.parse()'d it
themselves, that's a TYPE change, not a no-op.
Updated both godoc and CLAUDE.md to explicitly call this out
as the one breaking change, separately from the additive/
subtractive bucket. Better for downstream MCP consumers to see
the truth than to discover it via runtime failure.
The remaining v0.4 changes ARE additive (overflow counts on
BootstrapDashboard) or subtractive (dropped fields with named
canonical alternatives) — those parts of the original note
are accurate and kept.
Honesty about compatibility is more valuable than a tidy
narrative. Surfaced explicitly in the godoc + the public
contract doc; PLAN-1410's Result section was already honest
about the field-level deltas.
Parent: PLAN-1410 / TASK-1418.
|
||
|
xarmian
|
8fa1dd36f9 |
refactor(library): archive 9 pre-PLAN-1377 playbook bodies, rebuild library as invokable-first (TASK-1403) (#532)
Retires the legacy trigger-only library entries from the public
surface and replaces the 4-category structure with a single
`agent-workflows` category housing the three invokable workflow
playbooks (ship, plan, decompose).
## Changes
- New file `internal/collections/playbook_library_archive.go` —
holds all 9 retired bodies in package-private `archivedPlaybooks()`.
Bodies stay compiled so they're greppable and refactor-safe;
per-entry "convert to invokable" / "promote to convention" /
"retire" decisions are tracked in IDEA-1396. `var _ = archivedPlaybooks`
keeps the symbol referenced for unused-symbol linters.
- `internal/collections/playbook_library.go::PlaybookLibrary()` —
removed the 4 categories (workflow, planning, quality, operations)
and the 9 bodies inline. Replaced with a single `agent-workflows`
category containing ship + plan + decompose (the invokable trio
landed in T3/T4/T5). All three carry InvocationSlug and Arguments
so the library teaches the PLAN-1377 invocation model from the
first card.
- `internal/collections/playbook_library_plan.go` /
`playbook_library_decompose.go` — bump each helper's `Category`
field from `workflow` to `agent-workflows` to match the new
registry grouping.
- `internal/collections/templates.go::softwareStarterPlaybookTitles` —
updated from the retired pair ("Implementation Workflow", "Code
Review Process") to the new invokable pair ("Plan a new
initiative", "Decompose a plan into tasks"). `startup` template
separately prepends `ship` (templates.go:~441), so every software
workspace now seeds the full invokable trio from day one.
- `internal/mcp/dispatch_http_slice4_test.go` — the activate-by-title
fixture used "Implementation Workflow"; switched to "Ship tasks"
(still a real library entry with trigger+scope in its activation
payload). T6's verify section flagged this fixture; addressing it
here keeps the build green on this branch rather than deferring
the breakage to T7.
- `cmd/pad/main.go` — `pad library activate --help` example used
"Implementation Workflow" as a sample title; switched to "Ship
tasks" so the example still resolves.
## Verify
- `go build ./...` clean (no dangling references to the 9 titles in
production code paths).
- `go vet ./...` clean.
- `go test ./...` — all packages green.
- `grep -r "Implementation Workflow" --include="*.go" --include="*.ts"
--include="*.svelte"` returns only:
- `playbook_library_archive.go` (expected — the archive)
- `playbook_library_plan.go` (historical comment, intentional)
- `templates.go:868` (demo-workspace seed item content — unrelated
to library lookup; literal item title in a template, would not
benefit from being retitled in this PR's scope)
Pre-existing workspaces' already-activated copies of the 9 entries
keep working — they live in workspace data, not library code. Only
future activations are affected (the legacy titles no longer resolve
via `pad library activate` or the web Library UI).
Parent: PLAN-1397. Depends on T3/T4/T5 — the library is never empty
because ship, plan, and decompose are already in place.
|
||
|
xarmian
|
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. |