mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-18 08:35:22 +00:00
v0.3.0
127 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
40352a32e1 |
feat(auth): PAD_BYPASS_SETUP_TOKEN open-bootstrap escape hatch (#429)
Adds an env-var that lets self-host operators on trusted networks (Unraid behind a firewall, Tailscale-only deployments, homelabs) claim the first admin via the web UI without copying a bootstrap token out of the container logs. Behavior when PAD_BYPASS_SETUP_TOKEN=true: - handleBootstrap accepts non-loopback first-admin POSTs without an X-Bootstrap-Token header. The UserCount==0 invariant is unchanged, so the bypass auto-closes the moment the first admin claims the seat (subsequent bootstrap requests get 409 regardless of bypass). - handleSessionCheck returns setup_method=open so the /setup page skips the paste-token UI and renders the form directly. - Token generation is skipped at startup (no .bootstrap-token file written). A distinct WARN-flavored banner makes the open-mode trade-off obvious in operator logs. - Cloud mode (PAD_CLOUD/PAD_MODE=cloud) ignores the flag entirely. Three layers of defense: cmd/pad masks the env-var with !cfg.IsCloudServer(), Server.openBootstrapEnabled() checks !s.cloudMode, and the cloud branch in handleBootstrap never reads the bypass field. Unraid template gets a new "Bypass Setup Token" field (default false, Display="always") with a description that calls out the trust-the- network trade-off. Tests pin all the security-critical contracts: bypass admits non- loopback, bypass off keeps existing 403, cloud mode hard-ignores, loopback works either way, post-bootstrap gate stays closed, bypass wins over logs_token in session payload, cloud mode never advertises 'open' setup method. Codex review: CLEAN (round 1). |
||
|
xarmian
|
693f03be3c |
fix(auth): emit first-run bootstrap banner to stderr (BUG-1182) (#428)
slog's text handler is contractually one-line-per-record and escapes literal newlines as `\n`, so the multi-line bootstrap banner rendered as a single wide line in `docker logs` — exactly the surface where operators look for the token. Switches the banner to fmt.Fprint to stderr (real newlines), with a companion slog.Info one-liner so structured-log aggregators still record the event. The companion log deliberately does NOT include the URL or token in its structured fields — those would be parseable as log-aggregator-extractable values, defeating the URL-fragment design (TASK-1167 F10) that keeps the token off-server. Operators / agents that want the token programmatically read the on-disk file at token_path. Verified locally: banner now renders the ASCII box with real newlines, token visible, companion slog line shows token_path without the URL. Caught by Dave during the v0.3.0-rc.1 smoke test on a real Unraid box. |
||
|
xarmian
|
05a9665f50 |
feat(auth): first-run logs-token bootstrap flow (TASK-1167) (#424)
One-time bootstrap token generated on first start with no users in self-host mode. Token is logged in a banner the operator can grab from `docker logs`, persists at <DataDir>/.bootstrap-token (mode 0600), and bypasses the loopback-only gate via the X-Bootstrap-Token header — letting the user claim the first admin from a remote browser at /setup#token=<x>. Header-only contract + URL-fragment (browser-only, never transmitted) + log-redaction middleware keeps the secret out of access logs, proxy logs, and browser history. Cloud mode unchanged: token never loaded, never honored. Validate → UserCount-check → CreateUser → consume sequence is mutex-serialized to prevent concurrent valid-token requests from creating multiple admins. Part of PLAN-1166 (Pad on Unraid — Community Apps launch). |
||
|
xarmian
|
63d113624c |
fix(cli): retry password prompt on weak/mismatched passwords (BUG-1155) (#413)
* fix(cli): retry password prompt on weak/mismatched passwords during admin bootstrap (BUG-1155)
`pad auth setup` and `pad init` collected admin credentials with a single-
shot prompt: any rejection — local password mismatch, or server-side weak-
password / length error from validatePasswordStrength — bubbled up and
exited the command. The user had to re-run the whole flow (and in `pad
init`, redo configure + server-start) over a typo.
Replaces promptForAccountDetails() with promptAndBootstrap(client) which
collects email + name once, then loops the password / confirm pair (up to
5 attempts) on:
- local password mismatch
- *cli.APIError from /auth/bootstrap (covers all three messages from
internal/server/password_strength.go: too short, too long, too weak)
Network failures and other non-API errors still bail immediately.
Both call sites — cmd/pad/main.go (auth setup) and cmd/pad/init.go (init
step 3) — now use the new helper.
* fix(cli): only retry password-strength rejections, not all API errors per Codex review (round 1)
Round 1 retried on every *cli.APIError from /auth/bootstrap, but only
password-strength rejections are fixable by re-prompting the password
pair. The server also emits validation_error for invalid email / missing
name, conflict ("Pad instance has already been initialized"), and
forbidden (non-loopback bootstrap) — re-prompting just the password for
those traps the user in a 5-attempt loop that can never succeed.
Narrows the retry gate to validation_error whose message begins with
"Password" — the three messages emitted by validatePasswordStrength
(internal/server/password_strength.go: too-short, too-long, too-weak).
All other APIError codes and message shapes now fall through to the
fail-fast branch, so the user sees the real reason and can re-run with
the right correction.
|
||
|
xarmian
|
abf017c4e7 |
feat(onboarding): make banner + CLI hint template-aware (TASK-1150) (#409)
The IDEA-1 trigger phrase is no longer hardcoded — fresh scrum
workspaces surface "use pad to get BACK-1", product workspaces surface
"use pad to get FEAT-1", and any future template that ships an
agent-onboarding seed declares its primary ref once and gets the
banner / hint for free.
Mechanism:
1. WorkspaceTemplate gains an OnboardingPrimaryRef string field —
the canonical declaration of "this template's IDEA-1-style
primary entry." Set per template that ships the pattern
(startup → "IDEA-1", scrum → "BACK-1", product → "FEAT-1");
left empty for hiring/interviewing/demo where the agent-onboarding
pattern intentionally doesn't apply.
2. Server: handleGetDashboard identifies the seeded primary by
walking allItems looking for item_number=1 + source="template"
+ created_by="system" + collection_slug ∈ {ideas, backlog,
features}. The collection-slug whitelist is what keeps hiring's
REQ-1 (also seeded with item_number=1 + source=template) from
being flagged as an onboarding entry — those are example items,
not agent scripts. The dashboard response gains an
onboarding_seed field with ref/title/slug/collection_slug/status
plus a server-computed `active` boolean (true iff status equals
the schema initial value).
3. CLI: printOnboardingHints accepts the template name, looks up
the primary ref via collections.GetTemplate, and prints the
right "use pad to get X-1" line. Templates without a declared
primary skip the line entirely (so hiring's pad init success
doesn't promise a non-existent BACK-1 / IDEA-1).
4. Web frontend: dashboard reads dashboard.onboarding_seed,
gates the banner on `active=true`, passes ref/slug/collection
to OnboardingIdeaBanner. The component renders the trigger
phrase, copy button, and "Read it first" deep link from those
props — no more hardcoded IDEA-1.
ensureWorkspace's signature gains a returned templateName so init.go
+ main.go can pass it through to printOnboardingHints. The five
existing test call sites updated.
New tests:
internal/collections/templates_test.go
- TestTemplatesDeclareOnboardingPrimaryRef — locks the per-template
OnboardingPrimaryRef values (and the explicit emptiness of
hiring/interviewing/demo).
internal/server/handlers_dashboard_test.go
- TestDashboardOnboardingSeed_StartupTemplate
- TestDashboardOnboardingSeed_ScrumTemplate
- TestDashboardOnboardingSeed_ProductTemplate
- TestDashboardOnboardingSeed_HiringTemplate (asserts NO seed —
hiring's REQ-1 is example data, not an onboarding entry)
- TestDashboardOnboardingSeed_EmptyWorkspace (no template)
Removes the loadIdeaOne race-guard from +page.svelte — the dashboard
poll itself now carries the onboarding_seed.active flag so the banner
state lives entirely in the dashboard response. Drops ~50 lines of
frontend code.
Parent: PLAN-1146.
|
||
|
xarmian
|
553a39f09b |
fix(cli): pad auth setup hint should point at pad init, not a nonexistent IDEA-1 (TASK-1143) (#407)
PR #403 (TASK-1134) added printIdeaOneTriggerHint() to the pad auth setup success path so freshly-bootstrapped admins would learn about the seeded onboarding entry point. But pad auth setup only creates the first admin account — no workspace. IDEA-1 is only seeded when a startup-template workspace is created (via pad init / pad workspace init). A user following the original hint immediately would hit "workspace not found" / "item not found". Caught by Codex during review of PR #406 (the docs PR for TASK-1138). TASK-1143 was spawned then to keep PR #406 docs-only; this is the fix. Reframe (Option 2 from the task spec): keep the hint, but point at the next concrete action — `pad init` — rather than at IDEA-1. The IDEA-1 trigger phrase still surfaces in `printOnboardingHints`, which runs after `pad init` / `pad workspace init`. By then the workspace exists and the trigger phrase resolves correctly. Renamed `printIdeaOneTriggerHint` → `printPostSetupNextStepsHint` since the hint no longer names IDEA-1 directly. Wording matches CLAUDE.md / README — workspace creation precedes the trigger phrase everywhere. Parent: PLAN-1131 (follow-up). Origin: Codex review of PR #406 round 1. |
||
|
xarmian
|
0a5eb777b9 |
feat(onboarding): surface IDEA-1 trigger phrase across CLI and web UI (TASK-1134) (#403)
* feat(onboarding): surface IDEA-1 trigger phrase across CLI and web UI (TASK-1134)
Make the seeded onboarding entry point discoverable without prior
knowledge. CONVE-191 calls for full-stack thinking on user-facing
features — this lands on every surface a fresh user might check.
CLI surfaces:
• `pad auth setup` success message gains a closing hint pointing at
`use pad to get IDEA-1` in a new agent session. New helper
printIdeaOneTriggerHint() so future templates can reuse the shape.
• `printOnboardingHints` (used after `pad init` / workspace creation)
now leads with the trigger phrase before the existing /pad prompt
suggestions. IDEA-1 is named because it's the seeded primary entry
in software-category templates; people-category templates will
seed REQ-1 / APP-1 etc. and need a template-aware version of this
hint — tracked under PLAN-1140.
Web UI surfaces:
• New OnboardingIdeaBanner component renders on the workspace
dashboard whenever IDEA-1 is in status=new. Shows the trigger
phrase verbatim with a copy button and a "Read it first" deep link
into the seeded item itself. Disappears the moment the user (or
agent) flips IDEA-1 out of `new`.
• Dashboard fetches IDEA-1 alongside its existing dashboard +
collections calls (cheap, indexed by ref) and re-checks on every
poll (default 30s) plus every sync signal so the banner is
self-correcting.
• Existing OnboardingChecklist gate (`totalItems === 0`) is left
alone. It still serves empty / non-templated workspaces; the new
banner is the templated-workspace surface.
No tests added — both surfaces are pure copy/render. Existing
dashboard + auth-setup tests still pass.
Parent: PLAN-1131. Origin: IDEA-1128.
* fix(onboarding): pin IDEA-1 lookup to exact prefix+number match per Codex review (round 1)
Server-side ResolveItem (via GetItemByRef) falls back from PREFIX-NUMBER
to a number-only lookup when the prefix doesn't match any collection in
the workspace. That fallback exists so an item moved between collections
is still resolvable by its old ref — but it has a bad interaction with
my new dashboard lookup:
In a non-software-category workspace (hiring, interviewing, …), there
is no Ideas collection. `api.items.get(ws, 'IDEA-1')` would silently
return whatever item has item_number=1 — typically REQ-1 (Requisition)
or APP-1 (Application). If that item happened to have status=new
(which the seeded Requisition / Application entries do), the dashboard
would render the IDEA-1 onboarding banner pointing at a /ideas/... URL
that 404s.
Fix: verify item.collection_prefix === 'IDEA' && item.item_number === 1
before trusting the result. Mismatch (or missing) → ideaOneStatus = null,
banner stays hidden. Software workspaces with a real IDEA-1 still match;
hiring / interviewing / interview-loop-style workspaces stop seeing the
banner entirely.
Caught by Codex on PR #403.
* fix(onboarding): guard IDEA-1 lookup against stale-workspace writes per Codex review (round 2)
Previous round addressed the wrong-collection match. This round fixes a
related race: rapid workspace navigation could let a slow loadIdeaOne()
from workspace A resolve after the user is already on workspace B and
write A's status into B's state, briefly rendering the IDEA-1 banner on
a workspace that doesn't have it.
Two-part fix:
1. The dashboard $effect that triggers load() now resets
ideaOneStatus = null synchronously when wsSlug changes, so any
leftover `new` status from the previous workspace can't briefly
render the banner during the window between navigation and the new
fetch resolving.
2. loadIdeaOne() now compares its captured slug against the current
wsSlug at every assignment point (success and error paths). If
they've diverged, the response is dropped — only the active
workspace's request can write ideaOneStatus.
Standard "was this still the active request" pattern. No behavior
change for the common case (single-workspace dashboard); the guard
only fires when navigation interleaves with an in-flight fetch.
Caught by Codex on PR #403.
|
||
|
xarmian
|
40621ff58d |
feat(metrics): session-id-keyed TTL sweep for mcp_active_sessions (TASK-1120) (#400)
* feat(metrics): session-id-keyed TTL sweep for mcp_active_sessions (TASK-1120) Replaces the naive +1/-1 active-sessions accounting from TASK-961. The old logic bumped on JSON-RPC `initialize` and decremented on HTTP DELETE — but a client that crashed, lost network, or restarted mid-session never emitted DELETE, so the gauge drifted upward monotonically until the pad-cloud server restarted. Approach: - `internal/server/middleware_mcp_session.go` (new) — mcpSessionTracker is an in-memory map keyed by Mcp-Session-Id (the canonical header set by mcp-go's StreamableHTTPServer on initialize responses and echoed by the client on subsequent requests). Touch updates lastSeen on insert + refresh; evict removes; periodic sweep evicts entries older than the TTL. - Gauge is `Set(len(sessions))` via an onChange callback — single consistent observation per state-changing op, no risk of gauge drifting from map size on a multi-evict sweep. - Lifecycle: spawned by SetMCPTransport (alongside startMCPAuditWriter), shut down from Server.Stop. Idempotent on both sides. - Configurable via PAD_MCP_SESSION_TTL (default 30m) and PAD_MCP_SESSION_SWEEP_INTERVAL (default 5m). cmd/pad calls Server.SetMCPSessionTrackerConfig before SetMCPTransport. Other changes: - `recordMCPCallMetrics` no longer touches the active-sessions gauge. Updated comment + signature kept (callers pass the same args; the unused params are explicitly underscored). - `MCPAuditLog` middleware now calls trackMCPSession after next.ServeHTTP — single new line in the audit hot path. - `TestMCPAudit_BufferFull_DropsAndIncrementsCounter` updated to also shut down the new session tracker before bg.Wait(), since SetMCPTransport now spawns two goroutines on srv.bg. Test coverage (16 tests, all green under -race): - Tracker unit: touch insert/dedup, empty-id no-op, evict remove/non-existent, sweep eviction with single onChange, nil-onChange safety, concurrent touch/evict, run() clean shutdown. - Server-side integration: lifecycle happy path (initialize → call → DELETE leaves gauge at 0), failed initialize doesn't open, no-session-id no-op, nil tracker safety, idempotent start, DELETE evicts on any status (transient 5xx on shutdown still counts). - Regression guard: TestRecordMCPCallMetrics_DoesNotTouchSessionGauge pins that the audit-side helper has migrated off the gauge. Parent: PLAN-943. Follow-up to TASK-961 (PR #398). Closes the "sessions drift on client crashes" caveat documented in the metric's help text + the Grafana panel description. * fix(metrics): emit Mcp-Session-Id + serialize gauge updates per Codex review (round 1) Two findings from Codex review on PR #400: 1. WithStateLess(true) wired StatelessSessionIdManager whose Generate() returns "" — mcp-go never set the Mcp-Session-Id response header in production, so the new tracker no-op'd on every initialize and the active-sessions gauge stayed at 0. Fix: introduce padMCPGenerateOnlySessionIDManager in cmd/pad/main.go. Generates a UUID per initialize (so the response carries the header — tracker can observe), but Validate accepts ANY incoming value (including empty / arbitrary). Preserves the original "stateless server, every request stands alone" contract while making the session-id observable. Documented why mcp-go's two shipped stateless managers don't fit (one breaks observability, the other breaks back-compat for clients that never echo the ID). 2. touch / evict / sweep computed `len(sessions)` under the mutex then released the lock BEFORE invoking onChange. Two concurrent inserts could compute (n=1, n=2) under the lock and then race the callback writes — last writer wins on the gauge, leaving it permanently inconsistent with the map size. Fix: hold the mutex across onChange. Trade-off documented: any future onChange that re-enters the tracker would deadlock, but that's a clear failure mode rather than silent metric corruption. Added TestMCPSessionTracker_OnChangeUnderLock that asserts a strictly-monotonic observation sequence under 32-goroutine concurrent inserts; passes 5x in a row under -race. |
||
|
xarmian
|
1c409c8592 |
feat(metrics): emit mcp_authz_denials_total{reason=tier_mismatch} (TASK-1119) (#399)
Wire the dispatcher-side scope-deny seam into the pad_mcp_authz_denials_total counter, completing the denial-reason vocabulary documented in TASK-961. internal/mcp/dispatch_http.go: - Add optional OnScopeDenied(method, urlPath) callback on HTTPHandlerDispatcher - Fire it from buildAuthedRequest right before returning the existing permission_denied error — same control flow, just observability added in front internal/server/middleware_auth.go: - Public Server.RecordMCPTierMismatch helper that bumps the counter. No MCP-origin context gate (unlike recordMCPAuthzDenial below) — the dispatcher is by construction MCP-only, so every invocation is inherently MCP-origin. cmd/pad/main.go: - Wire dispatcher.OnScopeDenied = srv.RecordMCPTierMismatch alongside the existing UserResolver / Lister fields. Safe to attach unconditionally — RecordMCPTierMismatch nil-checks metrics internally, mirroring the OAuth observer wiring pattern. Tests: - Three new dispatcher tests covering OnScopeDenied: fires once with the right (method, urlPath) on deny; does NOT fire on allow; nil hook is safe. - Server-side test for RecordMCPTierMismatch: counter increments, other denial reasons untouched, nil-metrics safe. Parent: PLAN-943. Follow-up to TASK-961 (PR #398). |
||
|
xarmian
|
ba303e456f |
fix(mcp): publish PAD_MCP_PUBLIC_URL verbatim as canonical resource (no /mcp suffix) (#381)
Per the MCP authorization spec the client MUST verify the URL it was given matches the discovery doc's `resource` field exactly; auto- suffixing was forcing operators publishing the bare hostname (the industry convention — mcp.stripe.com, mcp.linear.app, mcp.atlassian.com) into a permanent client-side mismatch and Claude Desktop / Cursor reject pasting `https://mcp.getpad.dev` even though everything else works. Both production sites that previously appended "/mcp" to MCPPublicURL now use the value verbatim: - cmd/pad/main.go: AllowedAudience for the OAuth server constructor. Tokens are now audience-bound to MCPPublicURL exactly. - internal/server/handlers_well_known.go: the protected-resource discovery doc's `resource` field is the bare MCPPublicURL. The transport itself is unchanged — pad still mounts at /mcp on the chi router; pad-cloud's nginx router transparently rewrites mcp.* root → /mcp (TASK-997 PR #28) so external clients see a single canonical URL regardless of the internal HTTP path. The audience binding is just a string; it doesn't have to equal the internal mount path. config.go's MCPPublicURL doc updated to reflect the new semantic ("canonical URL clients paste") rather than the old "vhost URL we suffix-mangle". Operators who want the old shape just include the /mcp suffix in PAD_MCP_PUBLIC_URL — the operator owns the canonical. Test fixtures: testCanonicalAudience flipped from "https://mcp.test.example/mcp" to "https://mcp.test.example", and the two SetMCPTransport call sites that previously stripped /mcp now pass it directly. The TestMCP_DiscoveryDoc_PopulatedFromConfig assertion uses testCanonicalAudience so future renames stay consistent. All other test sites (audience= form fields, aud claim checks, mismatch fixtures) keep working unchanged because they reference testCanonicalAudience symbolically. |
||
|
xarmian
|
9eb1a35f16 |
feat(mcp): privacy-preserving available_workspaces filter on error envelopes (TASK-977) (#379)
* feat(mcp): privacy-preserving available_workspaces filter on error envelopes (TASK-977)
Closes the last open work item in PLAN-943. HTTPHandlerDispatcher's
unknown_workspace error envelope now populates available_workspaces
filtered by the OAuth token's consent allow-list (TASK-952), so an
agent never sees workspace slugs the user didn't explicitly grant.
## What changed
- `HTTPHandlerDispatcher` gains a `Lister WorkspaceLister` field.
Production wires `mcpserver.NewOAuthWorkspaceLister(s)`; tests
can supply mocks.
- `packageHTTPResponse` now takes a `lister` parameter and threads
it down to `classifyHTTPStatus`. Both call sites in the package
updated.
- New `oauthWorkspaceLister` reads three things from request context:
- `server.CurrentUserFromContext` — the requesting user.
- `server.TokenAllowedWorkspacesFromContext` — the consent
allow-list (TASK-953 plumbing).
- `s.GetUserWorkspaces(user.ID)` — the user's full set.
Returns the intersection. Wildcard (`["*"]`) and nil (PAT auth)
short-circuit to "no filter" — the user's full set is returned
in those cases since the token doesn't constrain workspaces.
- `cmd/pad/main.go` wires the production lister.
## Privacy invariant
A token whose allow-list is `[alpha, beta]` MUST NOT see "gamma"
in the available_workspaces hint, even if the user is a member of
gamma. Tested explicitly via
TestUnknownWorkspace_AvailableWorkspaces_FilteredByAllowList —
the test fakes a 4-workspace user membership, sets allow-list to
2, and asserts exactly 2 slugs appear in the filtered envelope.
Without this filter, an attacker controlling an OAuth client could
hit any random workspace slug, get the unknown_workspace envelope,
and read OFF the user's full workspace list — defeating the whole
point of the consent UI's per-workspace selection.
## Tests (18 new)
8 envelope round-trip tests pin every documented HTTP status →
ErrorCode mapping (401 → auth_required, 403 → permission_denied,
404 generic → item_not_found, 404 workspace → unknown_workspace,
409 → conflict, 400/422 → validation_failed, 5xx → server_error,
418 → server_error fallback).
5 privacy-filter tests cover the allow-list shapes:
specific-list-filters, wildcard-no-filter, no-allow-list-no-filter,
no-user-empty-hints, store-error-empty-hints.
4 buildAllowSet unit tests for the helper.
1 end-to-end test through packageHTTPResponse.
* fix(mcp): use req.Context() when packaging HTTP response (Codex round 1)
Codex review #379 round 1 caught a real correctness issue: the
packageHTTPResponse calls in executeRequest + the prefetch path in
dispatchItemUpdate passed the dispatcher's outer ctx instead of
req.Context(). The lister reads CurrentUser + TokenAllowedWorkspaces
from context, and the canonical "everything attached" context is
the SYNTHESIZED request's context — buildHTTPRequest layers
WithCurrentUser + WithAPITokenAuth on it, and d.Apply (when wired)
attaches token state on top of req specifically.
In production this happened to work because MCPBearerAuth attaches
TokenAllowedWorkspaces on the inbound /mcp request's context, which
the dispatcher inherits as its outer ctx. But:
- Tests driving executeRequest with context.Background() + a
UserResolver-supplied user got empty available_workspaces
because the outer ctx had no user.
- Any future dispatcher attaching token state via Apply (rather
than relying on inbound-ctx propagation) would also see the
bug — the Apply hook is documented as the place for "TASK-953
token-scope context" exactly.
Fix: pass req.Context() / prefetchReq.Context() to packageHTTPResponse.
Same dispatcher, same ServeHTTP — just feed the lister the canonical
post-Apply context.
Test: TestExecuteRequest_UsesRequestContext_NotOuterContext drives
executeRequest with an empty outer context + a UserResolver, asserts
the resulting unknown_workspace envelope has the user's full
workspace list. With the buggy version the test fails (lister sees
no user → empty hints).
|
||
|
xarmian
|
48776a3967 |
feat(oauth): DCR + authorize + token endpoints + populated discovery (TASK-1025, sub-PR C of TASK-951) (#372)
* feat(oauth): DCR + authorize + token endpoints + populated discovery doc (TASK-1025, sub-PR C of TASK-951)
Third of 5 sub-PRs landing the OAuth 2.1 authorization server in
PLAN-943. Mounts the three flow-driving HTTP endpoints over the
fosite-backed server constructed in sub-PR B, replaces the
TASK-950 501 stub with the real RFC 8414 discovery doc, and ships
an inline-HTML consent stub as a TASK-952 placeholder so the
auth-code flow runs end-to-end.
What lands:
- internal/server/handlers_oauth.go (744 LoC)
- POST /oauth/register: RFC 7591 DCR. Hand-written, no fosite.
Public clients only (token_endpoint_auth_method=none rejected
for any other value), authorization_code + refresh_token
grants only, code response type only. Validates redirect_uris
(absolute, no fragment, https or loopback-http or custom-
scheme like claude://, blocks file:/javascript:/data:/vbscript:).
- GET /oauth/authorize: starts auth-code flow. fosite validates
request shape (PKCE-S256 required, audience matched, redirect
exact-match). If user has session → renders inline consent
stub. If not → 302 to /login?redirect=<self> (TASK-998's
plumbing in pad-cloud honors the redirect=).
- POST /oauth/authorize/decide: processes consent decision.
Form-bound CSRF token (the existing __Host-pad_csrf cookie,
read from a hidden form field instead of header). Approve →
fosite NewAuthorizeResponse → 303 to client.redirect_uri
with code. Deny → fosite WriteAuthorizeError(access_denied).
- POST /oauth/token: code + refresh exchange. fosite verifies
PKCE verifier (S256-required) + RFC 8707 audience. Returns
{access_token, token_type, expires_in, refresh_token, scope}.
RefreshTokenScopes=[] from sub-PR B means refresh ALWAYS
issues on authorize-code grant.
- Inline consent stub: minimal HTML form with Approve/Deny,
auto-grants every requested scope (TASK-952's UI replaces
with workspace allow-list selection per TASK-953).
- internal/server/handlers_well_known.go: handleOAuthAuthorizationServerStub
→ handleOAuthAuthorizationServer. Returns RFC 8414 metadata
with all six endpoint URLs (revoke + introspect URLs sub-PR D
fills with handlers; the URLs are stable now), advertised
scopes, S256-only code_challenge_methods,
resource_indicators_supported=true, authorization_response_iss_parameter_supported=true.
- internal/server/server.go: Server.oauthServer field +
SetOAuthServer + registerOAuthRoutes called from setupRouter
inside an r.Group with requireCloudMode + SessionAuth (so
/authorize can detect the logged-in user via __Host-pad_session;
SessionAuth falls through gracefully when no cookie).
- cmd/pad/main.go: oauthpkg.NewServer wired in cloud mode using
cfg.EncryptionKey as HMAC secret + cfg.MCPPublicURL+/mcp as
AllowedAudience. Wiring is conditional on PAD_MCP_PUBLIC_URL
being set (the OAuth surface needs a canonical audience to
bind tokens to).
CSRF posture: middleware_csrf.go runs only on /api/* paths so
/oauth/* is naturally exempt. The consent decision endpoint
adds its own form-token check (validateConsentCSRFToken) using
the same __Host-pad_csrf cookie the SPA uses, just with the
token in a hidden form field rather than a header. Same security
model, different transport.
Tests (12, all passing):
- TestOAuth_AuthorizationServerMetadata_PopulatedShape: pins
RFC 8414 metadata fields including S256-only PKCE +
resource_indicators_supported.
- DCR (5): happy path; missing redirect_uris; bad redirect-URI
shapes (relative, non-loopback http, fragment, javascript:);
non-public client auth method rejected; unknown grant type
rejected; not mounted outside cloud mode.
- /authorize (3): redirects to /login when no session;
renders consent stub when logged in; rejects audience
mismatch via fosite's audienceMatchingStrategy.
- /authorize/decide (2): rejects missing csrf_token; deny
produces access_denied redirect.
- Full PKCE flow: end-to-end /authorize/decide (approve) →
/token with code_verifier → 200 with access+refresh tokens.
- /token: rejects missing PKCE verifier.
Replaces the 501 stub assertion in TestMCP_AuthServerStub with
TestMCP_AuthServerMetadata_Mounted (just confirms 200; full
shape lives in the OAuth-handler test).
Out of scope:
- /oauth/revoke + /oauth/introspect (sub-PR D, TASK-1026)
- MCPBearerAuth OAuth introspection branch (sub-PR E, TASK-1027)
- Real consent UI with workspace allow-list (TASK-952)
* fix(oauth): translate RFC 8707 resource= to audience= + omit unmounted endpoints from discovery per Codex review (round 1)
Two findings from PR #372 round 1:
1. P1: Real RFC 8707 clients (Claude Desktop / Cursor / ChatGPT)
send `resource=` not `audience=`. fosite v0.49 reads only
`audience` from the form, so audienceMatchingStrategy was hit
with an empty needle and rejected every real-world authorize /
token request. Tests masked the gap by sending both keys.
Fix: translateResourceToAudience() copies r.Form["resource"]
into r.Form["audience"] before each handler invokes fosite.
Idempotent — if both keys are present, audience wins (test
harness sends both for belt-and-suspenders). Applied at
/authorize, /authorize/decide, and /token entry points.
Test TestOAuth_Authorize_AcceptsResourceOnly sends ONLY
resource= (no audience=) and asserts the request reaches the
consent stub. Without the translation it 303s with
invalid_request.
2. P2: /.well-known/oauth-authorization-server advertised
/oauth/revoke + /oauth/introspect endpoints that don't exist
yet (sub-PR D wires them). Real clients dialing those URLs
would get 404. RFC 8414 §2 lists revocation_endpoint +
introspection_endpoint as OPTIONAL, so omitting until the
handlers ship is spec-compliant + honest.
Fix: drop revocation_endpoint, introspection_endpoint, and
their *_endpoint_auth_methods_supported counterparts from
authServerMetadata. Sub-PR D's PR description includes
"populate these here" as a follow-up.
Test TestOAuth_AuthorizationServerMetadata_OmitsUnimplementedEndpoints
asserts the four fields are absent.
* fix(oauth): rate-limit /oauth/register + drop misleading iss flag per Codex review (round 2)
Two findings from PR #372 round 2:
1. P1: /oauth/register is open by RFC 7591 design (Claude Desktop /
Cursor self-register without prior auth) but had no rate limit.
An attacker could flood the oauth_clients table indefinitely.
Fix: extend RateLimit middleware to gate /oauth/register at
the same 5/hour/IP rate the existing /api/v1/auth/register
uses (RateLimiters.Register, burst 5). Added the OAuth route
group to the s.RateLimit middleware chain so the new path
actually runs through the limiter.
Other /oauth/* endpoints aren't rate-limited here: /authorize
rides session cookies (cheap to abuse but ineffective without
a logged-in user), /token is PKCE-bound to a stored code
(single-use), /authorize/decide is form-bound. Explicit per-
endpoint /oauth/* limits arrive with TASK-959.
Test TestOAuth_Register_RateLimited fires 5 requests
successfully, asserts the 6th returns 429.
2. P2: Discovery doc advertised
authorization_response_iss_parameter_supported=true, but the
/authorize success path delegates to fosite v0.49 which doesn't
add iss=<issuer> to the redirect. RFC 9207-aware clients seeing
the flag would treat the missing parameter as a protocol
violation.
Fix: drop the field from authServerMetadata. RFC 8414 §2
marks it OPTIONAL — omission is spec-compliant. We'll add
the parameter (+ post-processing of fosite's response) in a
future PR if a real client requires it; today's MCP clients
(Claude Desktop, Cursor, ChatGPT) don't.
Test TestOAuth_AuthorizationServerMetadata_OmitsUnimplementedEndpoints
extended to cover the field.
* fix(oauth): gate auth-server discovery doc on oauthServer != nil per Codex review (round 3)
Codex round 3 caught: /.well-known/oauth-authorization-server lives
in the MCP route group (registerMCPRoutes), while the /oauth/{
register,authorize,token} handlers live in the OAuth route group
(registerOAuthRoutes, gated on s.oauthServer != nil). A cloud
deployment with PAD_MCP_PUBLIC_URL unset gets MCP routes mounted
but NOT OAuth — the discovery doc would 200 with /oauth/* URLs
that 404. Worse for clients than no document at all.
Fix: handleOAuthAuthorizationServer now also nil-checks
s.oauthServer; on nil it returns 503 with config_error, matching
the existing fail-loud branch for when the issuer URL isn't
configured. Ops detect the misconfiguration immediately rather
than fielding "OAuth registration is failing with 404" tickets.
Test:
- TestOAuth_AuthorizationServerMetadata_503WhenOAuthDisabled
builds a Server with SetCloudMode + SetMCPTransport (so the
MCP route group mounts) but NOT SetOAuthServer; asserts the
endpoint returns 503 with config_error.
- TestMCP_AuthServerMetadata_Mounted renamed →
TestMCP_AuthServerMetadata_MountedAndGated to reflect the new
behavior under mcpEnabledTestServer (which doesn't wire OAuth).
The full 200 happy path lives in
TestOAuth_AuthorizationServerMetadata_PopulatedShape (uses
oauthEnabledTestServer).
* fix(oauth): apply gofmt to handlers_oauth_test + handlers_well_known
* fix(oauth): bump go-jose/v3 to v3.0.4 to resolve GO-2025-3485
CI govulncheck rejected the build: fosite v0.49.0 transitively
pulls github.com/go-jose/go-jose/v3@v3.0.3 which has
GO-2025-3485 (DoS in JWS parsing). Affected call site:
internal/server/handlers_oauth.go:408 — handleOAuthAuthorize calls
fosite.NewAuthorizeRequest which eventually calls jose.ParseSigned.
Fix: bump go-jose/v3 to v3.0.4 (the fixed version per the advisory).
go mod tidy auto-bumped dependent indirect deps too.
Verified locally:
govulncheck ./... → "No vulnerabilities found"
go test ./... → all green
go build ./... → clean
|
||
|
xarmian
|
521853e0a1 |
feat(mcp): mount /mcp Streamable HTTP transport + OAuth discovery (TASK-950) (#369)
* feat(mcp): mount /mcp Streamable HTTP transport + OAuth discovery (TASK-950) First public cut of pad-cloud as a remote MCP server (PLAN-943). Mounts the Streamable HTTP transport on /mcp, the RFC 9728 protected-resource discovery doc on /.well-known/oauth-protected-resource, and a 501 stub for RFC 8414 auth-server metadata that TASK-951 will fill in. - internal/server/handlers_mcp.go — Server.SetMCPTransport + chi route registration under cloud-mode gate (self-host stays free of MCP overhead unless explicitly opted in). - internal/server/middleware_mcp_auth.go — Bearer auth that produces the spec-shape 401 + WWW-Authenticate (resource_metadata pointer) MCP clients expect, distinct from /api/v1's JSON-only 401 envelope. Reuses the existing PAT (api_tokens) validation path; OAuth-issued tokens layer in via this same middleware in TASK-951. - internal/server/handlers_well_known.go — RFC 9728 discovery doc + RFC 8414 stub. URLs come from PAD_MCP_PUBLIC_URL + PAD_AUTH_SERVER_URL with request-host fallback for local dev. - internal/server/handlers_mcp_test.go — 7 tests covering cloud-off routes-absent, cloud-on-no-transport routes-absent, discovery doc shape, 501 stub, no-token 401+WWW-Authenticate, bad-format-token 401+WWW-Authenticate, and the valid-PAT happy path with user attached to transport context. - cmd/pad/main.go — wires mcpserver.NewServer + HTTPHandlerDispatcher + StreamableHTTPServer in cloud mode, after SetCloudMode. - internal/config — adds PAD_MCP_PUBLIC_URL and PAD_AUTH_SERVER_URL. Resources are intentionally skipped in this v1 — they require an HTTPResourceFetcher equivalent of ExecResourceFetcher and that's a follow-up task. Tools, prompts, instructions, and meta all flow through identically to the stdio surface (verified via spike against mcp-go v0.50.0's StreamableHTTPServer before writing the real PR). * fix(mcp): enforce PAT scopes on /mcp + WWW-Authenticate fallback per Codex review (round 1) Two findings from PR #369 round 1: 1. SECURITY: A PAT with scopes ["read"] could drive write MCP tools. MCPBearerAuth skipped tokenScopeAllows entirely; the dispatcher's synthesized in-process request bypassed TokenAuth's chain-level check (because WithCurrentUser was already set), so a read-scoped token could POST item create / PATCH update / DELETE silently. Fix: stash apiToken.Scopes via server.WithTokenScopes in MCPBearerAuth; re-check per synthesized request in HTTPHandlerDispatcher.executeRequest using the public server.TokenScopeAllows wrapper. Read-scoped tokens can still drive read-only tools (their HTTP method is GET) — only writes are rejected, with a structured permission_denied envelope. 2. DISCOVERY: writeMCPUnauthorized dropped the WWW-Authenticate header when PAD_MCP_PUBLIC_URL was unset. Cloud-mode deploys without that env var mounted /mcp but broke the discovery handshake — fresh MCP clients rely on the header to find /.well-known/oauth-protected- resource. Fix: pass *http.Request through to writeMCPUnauthorized, derive "https://" + r.Host as the fallback (matches handleOAuthProtected- Resource's existing fallback). Tests: - handlers_mcp_test.go: TestMCP_NoToken_FallsBackToHostWhenPublicURLUnset pins the WWW-Authenticate fallback. TestMCP_ReadScopedPAT_StashesScopes- InContext + TestTokenScopeAllows_PublicWrapper pin the scope-stash side. - dispatch_http_test.go: TestHTTPHandlerDispatcher_ScopeEnforcement_* pin the dispatcher-side enforcement (read-on-write rejected, read-on-read allowed, no-scope-context allows-all). - recordingHandler updated to handle nil r.Body so the read-only GET path can be exercised. * fix(mcp): move scope check to buildAuthedRequest so bulk-update can't bypass it per Codex review (round 2) Round 1 enforced scopes in executeRequest, but dispatch_http_project.go's item bulk-update path constructs each per-item PATCH directly via buildAuthedRequest + d.Handler.ServeHTTP, skipping executeRequest. Net result: a PAT with ["read"] scope could still mutate items through bulk-update even after the round-1 fix. Move the scope check from executeRequest into buildAuthedRequest so every synthesized request — main writes, RMW prefetches, bulk-update per-item PATCHes, link-create POSTs, attachment HEADs — passes through the same gate uniformly. The check is dropped from executeRequest to avoid double-checking; buildAuthedRequest is the universal funnel everything calls. Reads (GET/HEAD/OPTIONS) under ["read"] scope still pass — bulk- update's per-item GET prefetch succeeds, the subsequent PATCH fails at request-build time with permission_denied. The bulk operation returns successfully with all-errors recorded per ref (the "no abort on per-item failure" contract is unchanged). Test: TestHTTPHandlerDispatcher_ScopeEnforcement_BulkUpdateBlockedOnReadScope spies on the test handler; asserts the PATCH never reaches it under ["read"] scope and that each per-item entry carries permission_denied. |
||
|
xarmian
|
eb1931d747 |
feat(cli): structured JSON output for note/decide/star/unstar/delete/bulk-update (BUG-989) (#363)
* feat(cli): structured JSON output for note/decide/star/unstar/delete/bulk-update (BUG-989)
Six pad item subcommands previously returned plain-text confirmations
when called with --format json (and therefore via MCP) — e.g.
"Added implementation note to TASK-8 ...\n", "⭐ Starred TASK-7 ...\n".
Agents had to scrape the text for refs / IDs / status. Now each
emits a structured envelope on the JSON branch, matching the shape
the bug report's recommendations specified.
Per-command shapes:
- pad item note --format json:
{ ref, title, note: { id, summary, details, created_at, created_by } }
- pad item decide --format json:
{ ref, title, decision: { id, decision, rationale, created_at, created_by } }
- pad item star --format json:
{ ref, title, starred: true }
- pad item unstar --format json:
{ ref, title, starred: false }
- pad item delete --format json:
{ ref, title, status: "archived" }
- pad item bulk-update --format json:
{ updated: [{ref, applied: {status?, priority?}}], failed: [{ref, error}], total }
Implementation notes:
- For note/decide: capture the entry locally before persisting so the
JSON branch can echo the freshly-created ID + timestamp without
re-fetching the item.
- For bulk-update: collect per-item outcomes in two slices (updated,
failed) so the response carries which refs succeeded with which
applied changes vs which failed with what error. Suppresses the
human-readable per-line ✓/✗ output when --format json (single
payload at the end is the agent's source of truth).
- Human-readable (default) output paths unchanged for all six.
The text fallback that ExecDispatcher passes back through
packageJSONResult also keeps showing the same structured JSON now
since the CLI emits JSON directly when --format json is set —
matches BUG-985's wrap-arrays-as-{items:[...]} pattern for list
shapes, and rounds out the v0.2 catalog's structured-everywhere
contract.
Live verified all six locally (CLI direct + MCP transport):
pad item note TASK-994 "..." --format json → {note: {...}, ref, title}
pad item star TASK-994 --format json → {ref, starred: true, title}
pad item bulk-update --priority medium TASK-994 --format json
→ {updated: [...], failed, total}
pad item delete TASK-994 --format json → {ref, title, status: "archived"}
(note/decide/star/unstar/decide via MCP confirmed dict-shaped
structuredContent with the expected keys.)
Parent: BUG-989.
* fix(cli): delete JSON envelope uses `archived: true` per Codex review (round 1)
Codex finding: `pad item delete --format json` was emitting
`"status": "archived"`, but the store's delete path only sets
`deleted_at` — the item's persisted `status` field is untouched. So
the envelope's status would mislead agents into treating the item's
status as archived, which breaks if the item is later restored (its
original status field is still there).
Fix: rename to `"archived": true` — unambiguous about what actually
happened (soft-delete marker set) and doesn't collide with the
persisted status semantics.
The other five JSON shapes weren't affected.
|
||
|
xarmian
|
55d3a078a8 |
fix(mcp): standup CLI ref + classifier polish for BUG-987 round 2 (#362)
Round-2 hotfix on top of PR #361 (which shipped to v0.1.0-rc.4). Claude Desktop's re-review of rc.4 surfaced two fixes that didn't fully land: - Bug 8 (round 1 went to wrong layer). My HTTPHandlerDispatcher fix populated ref on standup blockers, but Claude Desktop's path is ExecDispatcher → CLI subprocess → standupCmd, which has its own JSON composition struct. That struct's Attention + SuggestedNext anonymous types didn't even define ItemRef as a parseable field. Now both define `item_ref` and the JSON-emit loops set Ref from it. Verified live: blockers now carry refs (TASK-X), not empty strings. - Bug 11 part 2. Round 1 stripped the cobra Usage block but two artifacts still leaked: 1. The "pad <verb> failed: <stderr>" prefix on server_error fallback messages. The verb name is the OLD CLI verb (e.g. `pad item block`) which doesn't match the v0.2 catalog actions agents see, and the cmdPath is already implicit from the invoked tool. Drop the prefix; emit the cleaned stderr directly. 2. Self-link / "cannot ..." validation rejections classified as server_error instead of validation_failed. Extended the validation regex with `cannot ` so server-side rejections like "cannot link an item to itself" / "cannot modify archived item" route to ErrValidationFailed. Verified live with a self-link attempt — now returns code=validation_failed, hint="cannot link an item to itself", no prefix. - New stripErrorPrefix helper trims leading `Error:` / `error:` / `ERROR:` from every classified hint+message so the envelope text isn't redundant with the envelope's `code` signal. Bug 13 / Bug 14: my round-1 fixes verified working locally on rc.4 (tested with a fresh Task → convention=None; dashboard by_role shows "Unassigned"/"unassigned" for the bucket). The reviewer's stale results almost certainly reflect a pad server process that wasn't restarted with the rc.4 binary swap. Tests: - TestClassifyExecError_CannotPhrasingClassifiesAsValidation — three "cannot ..." stderr cases must classify validation_failed. - TestClassifyExecError_NoLegacyVerbPrefixInMessage — pins the prefix-strip behaviour on the server_error fallback path. - TestStripErrorPrefix — trim-rule round-trip across casing variations and empty input. Parent: BUG-987. |
||
|
xarmian
|
0f05012169 |
fix(mcp): six surgical fixes for BUG-987 (bugs 6, 8, 11, 12, 13, 14) (#361)
* fix(mcp): six surgical fixes for BUG-987 (bugs 6, 8, 11, 12, 13, 14)
Hotfix follow-up to v0.1.0-rc.3's Claude Desktop dogfood. Six
surgical fixes; bigger items (5, 7, 9, 10) deferred to separate
tasks.
- Bug 6: `pad project next --format json` was emitting the entire
dashboard, indistinguishable from `pad project dashboard --format
json`. Now slices to suggested_next only. cmd/pad/main.go.
- Bug 8: standup blockers carried empty `ref` strings, blocking
agent linkback to the actually-blocked items. dashboard's
attention[].item_ref is canonical; the standup composer in
internal/mcp/dispatch_http_slice4.go just wasn't propagating it.
Same fix applied to suggested_next entries.
- Bug 11: cobra's auto-emitted "Usage: pad item block ..." help
block leaked into MCP error envelopes via classifyExecError. The
Usage text references OLD CLI verb names (pre-v0.2 catalog) that
agents using the new surface have no business seeing, and bloats
every error response. New stripCobraUsageBlock helper truncates
stderr at the first line-anchored "Usage:" marker before
classification + envelope construction.
- Bug 12: BuildCLIArgs validation errors (missing required arg, type
mismatch) came out of env.Dispatch as bare-text NewToolResultErrorf
results, breaking the structured envelope contract. New helper
validationFailedFromBuildErr wraps them as ErrValidationFailed
envelopes with the field name extracted via regex from the
underlying message.
- Bug 13: every Task / Idea / Plan with a `priority` field got a
phantom `convention: { enforcement: "<priority>" }` surfaced on
its response, because ExtractItemConventionMetadata's legacy
fallback treated `priority` as the Convention enforcement tier
unconditionally. Restructured to track hasConventionShape
separately from hasMetadata — only Convention-specific markers
(structured convention field, trigger, scope, surfaces, commands,
direct enforcement) flip the shape flag. category alone is
insufficient (Ideas / Bugs / Roadmap items legitimately use it).
Final guard returns nil when only category was matched.
- Bug 14: GetRoleBreakdown's unassigned row was emitted with empty
role_name + role_slug, presenting as a "phantom" entry in the
dashboard. Now explicitly labelled "Unassigned" / "unassigned"
while keeping role_id null so it's still distinguishable from a
real role.
Tests:
- internal/mcp/bug987_test.go (new) — stripCobraUsageBlock + classify
+ validation envelope wrapping + env.Dispatch integration.
- internal/models/item_test.go — three cases covering non-Convention
items (Task, Idea, Plan with priority) returning nil metadata, and
one preservation test for legacy Conventions with priority field.
- internal/store/agent_roles_test.go (new) — confirms unassigned row
carries explicit "Unassigned" / "unassigned" labels.
Live verified: pad_project action=next returns just the suggestions
array; pad_item action=create with no fields returns validation_failed
with field=collection; pad_item action=link with self-target returns
without Usage-block leakage.
Deferred to separate items (per BUG-987 triage):
- Bug 5: text vs JSON returns across note, decide, star, unstar,
delete, bulk-update — needs CLI-side handler updates per command.
- Bug 7: suggested_next algorithm — needs to consider in-progress
items, not just open ones; behavior change needs design.
- Bug 9: fields/tags double-stringified — potentially breaking for
web UI/CLI consumers.
- Bug 10: decision_log/notes embedded in fields blob duplicating
top-level arrays — might require data migration.
Parent: BUG-987.
* fix(mcp): HTTP transport equivalence + ordering for BUG-987 per Codex review (round 1)
Two findings from Codex review of PR #361:
1. project.next on HTTP transport still returned the full dashboard.
The route table mapped "project next" directly to /dashboard, so
the CLI fix (slice to suggested_next) didn't reach OAuth-authed
agents going through HTTPHandlerDispatcher. Catalog actions must
produce equivalent shapes on stdio and HTTP — that's the contract
that lets agents be transport-agnostic.
Fix: new dispatchProjectNext method on HTTPHandlerDispatcher that
fetches the dashboard via the existing fetchDashboardJSON helper,
slices to suggested_next[], re-encodes, and runs through
packageJSONResult so it gets the same {items: [...]} wrap as
other list responses.
Also retires the broken route-table entry — replaced with a
comment pointing at the new method so future contributors don't
re-add a passthrough.
Test: TestDispatch_ProjectNext_SlicesToSuggestedNext + the empty-
array case. Asserts dashboard-only top-level fields (summary,
active_items) don't leak into the response — that's the whole
point of project.next being distinct from project.dashboard.
2. ExtractItemConventionMetadata's priority→enforcement legacy
fallback ran BEFORE surfaces/scope/commands had a chance to flip
hasConventionShape, so a Convention with only `{scope, priority}`
would silently drop enforcement.
Fix: move the priority fallback to AFTER all marker checks. Direct
`enforcement` still resolves first; the legacy priority fallback
runs at the bottom once shape detection is complete.
Tests: two new cases covering scope-only and commands-only legacy
Conventions — both must resolve enforcement via the priority
fallback.
Parent: BUG-987.
|
||
|
xarmian
|
1e94fcbd9d |
feat(mcp): structured MCP error envelopes with closed taxonomy (TASK-973) (#357)
* feat(mcp): structured MCP error envelopes with closed taxonomy (TASK-973)
Replaces raw stderr / status-text passthrough with a closed-set
ErrorCode taxonomy + structured ErrorEnvelope. Agents can now branch
on `error.code` instead of parsing free-form text:
{
"error": {
"code": "no_workspace",
"message": "No workspace context. Pass `workspace` explicitly, ...",
"hint": "Available workspaces: docapp, pad-web",
"available_workspaces": [{"slug": "docapp", "default": true}, ...]
}
}
Taxonomy (8 codes):
- no_workspace, unknown_workspace — populate available_workspaces
- auth_required, permission_denied
- item_not_found, validation_failed, conflict
- server_error (catch-all)
Implementation:
- internal/mcp/errors.go (new): ErrorCode constants, ErrorEnvelope +
ErrorPayload + WorkspaceHint types, NewErrorResult constructor,
classifyExecError + classifyHTTPStatus dispatchers, regex pattern
matchers for stderr classification, WorkspaceLister interface for
hint enrichment.
- internal/mcp/dispatch.go: ExecDispatcher.Dispatch routes failures
through classifyExecError (with itself as the WorkspaceLister).
Adds ListWorkspaces method that shells out to `pad workspace list
--format json`. Adds RootArgs field so the listing inherits root
flags (--url etc.).
- internal/mcp/dispatch_http.go: packageHTTPResponse routes 4xx/5xx
through classifyHTTPStatus. Lookup is intentionally nil here —
TASK-977 (PLAN-943) owns the privacy-preserving available_workspaces
filtering by OAuth allow-list.
- cmd/pad/mcp.go: pre-flatten rootFlags into RootArgs at
dispatcher construction.
NewErrorResult emits BOTH structured content (for Claude Desktop,
Cursor) AND a JSON text body (for older clients). Both decode to the
same envelope so wire-level shape stays uniform.
Tests:
- TestNewErrorResult_Envelope: round-trip the envelope through
structured + text surfaces.
- TestClassifyExecError: 11 cases covering every taxonomy code via
stderr patterns.
- TestClassifyExecError_LookupFailureStillReturnsEnvelope: lookup
failures degrade to empty available_workspaces, never drop the
whole envelope.
- TestClassifyHTTPStatus: 10 cases covering each HTTP status mapping
including the workspace-vs-item 404 fork.
- TestParseWorkspaceListJSON: happy path, empty / null / malformed,
entry-without-slug skipping.
- TestExtractUnknownWorkspaceSlug: regex helper round-trip.
Out of scope (per task description):
- HTTPHandlerDispatcher available_workspaces filtering by OAuth
allow-list → TASK-977 (PLAN-943).
- item_not_found "recent items" hint enrichment → also TASK-977.
- Per-code docs page on getpad.dev/mcp/local → TASK-976.
Parent: TASK-973 → PLAN-969.
* fix(cli): add JSON output to pad workspace list per Codex review (round 1)
Codex P2: classifyExecError's WorkspaceLister side channel calls
`pad workspace list --format json` to populate available_workspaces
in no_workspace / unknown_workspace error envelopes (TASK-973). The
CLI command silently ignored formatFlag and always printed the
human-readable shape, so parseWorkspaceListJSON would fail and the
hint was effectively never populated.
Add JSON branch to workspacesCmd that emits a {slug, name,
updated_at, default} array. The `default: true` flag marks the
CWD-linked workspace so agents can prefer it without a separate
DetectWorkspace call.
Manual verification:
$ pad workspace list --format json | jq '.[0]'
{
"slug": "docapp",
"name": "pad",
"updated_at": "2026-04-14T13:24:51Z",
"default": true
}
Parent: TASK-973 → PLAN-969.
* fix(mcp): tighten unknown-workspace slug regex per Codex review (round 2)
Codex finding: extractUnknownWorkspaceSlug's bare-word regex captured
stop-words like "not" out of generic "Workspace not found" messages.
The server emits exactly this generic body in middleware_auth.go and
handlers_workspaces.go, so the resulting envelope would say
`Workspace "not" is not visible to this session.` — pushing agents
toward retrying with a bogus slug.
Tighten the regex to only match QUOTED slug forms ("workspace 'foo'"
or "workspace \"bar\""). Bare-word phrasings yield empty slug, and
unknownWorkspaceResult now emits a generic "Workspace not visible to
this session." instead of the misleading empty-string `Workspace ""`.
Test cases updated:
- "workspace 'foo' does not exist" → "foo" (still works)
- "workspace \"bar\" not found" → "bar" (still works)
- "unknown workspace baz" → "" (was "baz", now intentionally empty)
- "workspace docapp not visible" → "" (was "docapp", now empty)
- "Workspace not found" → "" (the actual server response)
The other taxonomy / hint behavior is unchanged: ErrUnknownWorkspace
still classifies correctly, available_workspaces still populates from
ListWorkspaces, and the body text still appears in Hint via the
classifyHTTPStatus 404 branch's body-append logic.
Parent: TASK-973 → PLAN-969.
|
||
|
xarmian
|
19f20c5911 |
feat(mcp): v0.2 catalog migrate pad_item + retire cmdhelp walker (TASK-981) (#354)
* feat(mcp): v0.2 catalog migrate pad_item + retire cmdhelp walker (TASK-981)
Final commit of TASK-970's 3-stage rollout (PLAN-969). pad_item lands
with 17 actions consolidating the v0.1 verb tools (item_create /
item_block / item_star / item_unstar / item_supersedes / item_unsupersede /
...) into one resource × action shape. cmdhelp leaf walker retired —
tools/list now advertises only the v0.2 catalog (~7 catalog tools +
pad_set_workspace).
pad_item actions:
- Lifecycle: create, update, delete, get, list, move
- Relationships: link, unlink, deps
- Stars: star, unstar, starred
- Comments: comment, list-comments
- Bulk + notes + decisions: bulk-update, note, decide
link / unlink dispatch on link_type via itemLinkRoutes table:
- blocks, blocked-by → item block / blocked-by + item unblock
- supersedes → item supersedes / unsupersede
- implements → item implements / unimplements
- split-from → item split-from / unsplit
Per-direction op (cmdPath, firstArg, secondArg, inverted) handles
the asymmetric "blocked-by unlink reuses unblock with operands swapped"
case correctly.
Walker retirement:
- registry.go shrinks dramatically. Register() now registers
pad_set_workspace + delegates to RegisterCatalog. Drop identifyLeaves,
hasExcludedAncestor, buildTool, makeDispatchHandler, propertyForArg,
propertyForFlag, propertyOptionsCommon, stringifyEnum, ToolNameFromPath,
DefaultExcludes, RegistryOptions.ExcludeCommands.
- mergeDispatchInput moves to dispatch.go (still used by env.Dispatch).
- registry_test.go pruned to: validation tests, MCPPropertyName tests,
shared helpers (fakeDispatcher, fixtureDoc, equalSlice). DOC-978
said to "delete and rebuild" — done; the v0.1 walker assertions
weren't worth carrying forward.
- cmd/pad/mcp.go: single Register() call (no separate RegisterCatalog).
ToolSurfaceVersion bumped 0.1 → 0.2. pad_meta.tool-surface's
rollout_status flips from "in-progress" to "complete" automatically
because the bump makes ToolSurfaceVersion != "0.1".
CLAUDE.md updated to reflect the new architecture (catalog over walker;
two version constants — CmdhelpVersion + ToolSurfaceVersion).
Tests:
- TestPadItemLink_DispatchTable iterates itemLinkRoutes and asserts
link/unlink dispatch correctly for every link_type, including the
blocked-by-uses-unblock-with-swapped-operands case.
- TestPadItemLink_Missing/UnknownLinkType for the structured error path.
- catalog_readonly_test.go's expected{} extended with pad_item
passThrough actions; link/unlink intentionally skipped (custom
dispatch).
- TestRegister_PassesPadVersionToCatalog round-trips PadVersion through
RegistryOptions → CatalogOptions → ActionEnv.
Parent: TASK-981 → TASK-970 → PLAN-969.
* fix(mcp): support repeatable refs for pad_item.bulk-update per Codex review (round 1)
Codex P (no priority shown — substantive issue): pad_item exposed
`ref: string` everywhere, but bulk-update's CLI takes a repeatable
positional (one or more refs). The retired cmdhelp walker generated
array schemas for repeatable args; v0.2's scalar `ref` made
bulk-update effectively single-item or schema-invalid for its
primary use case.
Fix: dedicated `refs: array<string>` schema param + custom
actionItemBulkUpdate handler. Translates `refs` array → repeatable
`ref` positional (the form BuildCLIArgs feeds CLI commands with
arg.Repeatable=true).
Why a separate `refs` param vs. overloading `ref`: keeps the schema
consistent across actions — agents see one shape per param name.
JSON Schema oneOf would also work but mcp-go's helpers don't expose
it cleanly.
Lenient fallback: a single ref passed unwrapped as a string still
works (logically equivalent to a 1-element array). Empty arrays and
missing refs both surface structured errors with `refs is required`.
Tests cover: array of strings → multiple positionals, single string
fallback, missing refs error, empty array error. Existing fixture
in TestReadOnlyCatalog_ActionsDispatchExpectedCmdPath extended with
`refs: ["TASK-1", "TASK-2"]` so bulk-update reaches dispatch.
Parent: TASK-981 → TASK-970 → PLAN-969.
|
||
|
xarmian
|
df8a3631e7 |
feat(mcp): v0.2 catalog scaffold + ToolSurfaceVersion + pad_meta tool (TASK-979) (#352)
* feat(mcp): v0.2 catalog scaffold + ToolSurfaceVersion + pad_meta tool (TASK-979) First commit of TASK-970's 3-stage rollout (PLAN-969). Introduces the hand-curated v0.2 catalog types (ToolDef, ActionFn, ActionEnv) and ships one tool — pad_meta — end-to-end. v0.1 cmdhelp-walk surface stays live alongside; subsequent commits (TASK-980, TASK-981) migrate the rest and flip v0.1 off. Architecture record: DOC-978. The fan-out registry sits ABOVE the dispatcher boundary — Dispatcher / route table are unchanged, so both ExecDispatcher (stdio) and HTTPHandlerDispatcher (HTTP) inherit the new shape for free. Changes: - internal/mcp/catalog.go (new) — ToolDef, ActionFn, ActionEnv, passThrough helper, RegisterCatalog, makeFanOutHandler, structured error helpers. - internal/mcp/catalog_meta.go (new) — pad_meta tool with three inline actions: server-info, version, tool-surface (full catalog dump for PLAN-943 docs generation). - internal/mcp/version.go — add ToolSurfaceVersion = "0.2" + matching experimentalToolSurfaceKey. Independent of CmdhelpVersion (cmdhelp owns CLI help-tree contract; ToolSurfaceVersion owns MCP catalog). - internal/mcp/meta.go — extend MetaPayload with ToolSurfaceVersion; experimentalCapabilities advertises both padCmdhelp + padToolSurface. - cmd/pad/mcp.go — call RegisterCatalog alongside Register so v0.2 surface is live. - Tests: catalog_test.go + catalog_meta_test.go (new); meta_test.go + server_test.go updated to assert the new field/capability. Parent: TASK-970 → PLAN-969. * fix(mcp): keep ToolSurfaceVersion at "0.1" until catalog is complete per Codex review (round 1) Codex P1: advertising tool_surface_version=0.2 while the user-visible surface is still predominantly v0.1 (cmdhelp walker active alongside, only pad_meta in the catalog) misleads consumers that pin against the handshake or pad://_meta/version. The padToolSurface namespace would suggest the full resource/action shape is available when in reality only pad_meta uses it. Delay the 0.1 → 0.2 bump to TASK-981 — the commit that retires the cmdhelp walker and ships the complete catalog. The constant stays declared so the surface contract is wired through the handshake + meta resource + pad_meta.tool-surface, the version string just truthfully reflects "still v0.1" until the catalog is complete. No test changes needed: every assertion uses the constant, not a literal "0.2". Parent: TASK-979 → TASK-970 → PLAN-969. * fix(mcp): scope pad_meta.tool-surface to v0.2 catalog only per Codex review (round 2) Codex P1: pad_meta.tool-surface description claimed "Full catalog dump: every tool" but during PLAN-969's parallel rollout, tools/list contains both the catalog (currently just pad_meta) AND the cmdhelp walker's ~85 verb tools. Calling the catalog dump "every tool" misleads consumers who expect a complete enumeration. Same spirit as round 1's fix: stop claiming what isn't true. The catalog dump is the v0.2 catalog by design — consumers wanting the complete advertised surface should read tools/list directly. Hand-mapping the walker output into the catalog dump would cost duplication for a surface that's about to disappear in TASK-981. Wire-level changes: - Tighten the action description in padMetaToolDescription to say "v0.2 catalog dump: every tool managed by the hand-curated catalog" and explicitly note tools/list is the source for the complete surface. - Add rollout_status field to the response payload: "in-progress" while ToolSurfaceVersion stays at "0.1", "complete" once TASK-981 bumps it. Lets consumers detect the rollout state programmatically. - Test asserts the new field tracks ToolSurfaceVersion. Parent: TASK-979 → TASK-970 → PLAN-969. * fix(mcp): include params in pad_meta.tool-surface dump per Codex review (round 3) Codex P1: tool description claimed the dump includes each tool's "input schema" but the payload only emitted name/description/workspace/ actions[]. Misleading for docs generators (TASK-957) that would build getpad.dev/docs/mcp from this canonical source. Going with the substantive fix rather than just trimming the description: include a synthesized params[] per tool entry. Mirrors what consumers see in tools/list — `action` (always required, enum of declared action names), `workspace` (when ToolDef.Schema.Workspace=true), and per-tool ParamDefs. Synthesizing `action` and `workspace` rather than copying them from ToolDef makes the dump self-contained: a docs generator doesn't need to reproduce buildToolFromDef's implicit-param logic separately. Test asserts each catalog entry has params[] starting with `action` (enum length matches action handler count) and the right total length based on Schema.Workspace + Schema.Params. Parent: TASK-979 → TASK-970 → PLAN-969. |
||
|
xarmian
|
d84f1180a7 |
feat(mcp): pluggable Dispatcher + HTTPHandlerDispatcher for remote MCP (TASK-965) (#343)
* feat(mcp): pluggable Dispatcher + HTTPHandlerDispatcher for remote MCP (TASK-965)
Architectural prerequisite for PLAN-943's remote MCP at /mcp. The
existing ExecDispatcher (PLAN-942) shells out to the pad binary and
inherits credentials from ~/.pad/credentials.json — fine for local
stdio MCP where the user IS the subprocess owner, but unworkable for
a multi-tenant /mcp endpoint where the dispatcher must serve many
OAuth-authenticated users from a single process.
This PR ships the alternative path: HTTPHandlerDispatcher calls
pad-cloud's existing HTTP handler chain in-process, with the
requesting user attached via context. Same handlers, same audit /
event-bus / webhook plumbing — just no fork().
## What's in
- internal/mcp/dispatch.go: keeps the existing Dispatcher interface
(so ExecDispatcher unchanged) and adds a context-keyed
WithDispatchInput helper. The registry attaches the original JSON
input map to the dispatch context so dispatchers that prefer
structured data over reverse-parsed cliArgs can use it.
- internal/mcp/registry.go: forwards the merged input (user-supplied
values + session workspace + root flags) to the dispatcher via
WithDispatchInput. ExecDispatcher ignores it.
- internal/mcp/dispatch_http.go (new): HTTPHandlerDispatcher
implementation with a routeTable[cmdPath]→RouteMapper mapping. Seed
entry: `item create`. Adding more commands is one RouteMapper per
cmdPath plus a routeTable insert.
- internal/server/context.go (new): exported WithCurrentUser /
WithAPITokenAuth / WithTokenWorkspaceID + read-only
CurrentUserFromContext / IsAPITokenFromContext. Lets internal/mcp
synthesize an authenticated request without reaching the
package-private context keys.
- internal/server/middleware_csrf.go: extends the existing "Bearer
token requests skip CSRF" rule to also honor the ctxIsAPIToken
context flag. Same semantic — non-cookie auth means no CSRF risk —
but covers the in-process dispatch path where TokenAuth never sets
the Authorization header. Safe because ctxIsAPIToken can only be
set by trusted in-process code (TokenAuth on the live Bearer path,
or server.WithAPITokenAuth from the dispatcher).
## What's tested
- Unit:
- TestHTTPHandlerDispatcher_RoutesItemCreate — full happy path
with a recordingHandler asserting method/path/body/user-context.
- TestHTTPHandlerDispatcher_UnsupportedToolReturnsErrorResult —
tools not yet in the routeTable produce IsError-flagged results
rather than panicking.
- TestHTTPHandlerDispatcher_NoUserReturnsErrorResult — UserResolver
returning nil produces an IsError, never a nil-deref.
- TestHTTPHandlerDispatcher_HandlerErrorSurfacesAsToolError — 4xx
handler responses come back as IsError MCP results matching
ExecDispatcher's `pad <cmd> failed: <stderr>` format.
- mapItemCreate validation + parseFieldKVP variants.
- Integration: TestHTTPHandlerDispatcher_Integration drives the full
*server.Server (real chi router, real SQLite store, full middleware
chain) with a synthesized OAuth user and asserts the item lands in
the DB.
## Scope discipline
The DoD called for "dispatch item.create end-to-end" — that's the seed
entry. Wiring the remaining ~70 MCP-exposed commands into routeTable
is naturally a follow-up before TASK-950 ships /mcp to real users
(captured as a separate task post-merge).
Audit-log assertion in the integration test is deferred until TASK-960
(B6b) lands the audit log itself.
Parent: PLAN-943.
* fix(mcp): roll status/priority/category/parent into fields JSON per Codex review (round 1)
Codex caught: mapItemCreate placed status / priority / category /
parent at the top level of the JSON body, but handleCreateItem only
reads them after unmarshalling the Fields string from the request. As
written, MCP-driven `item create` would silently drop those flags —
breaking parity with the CLI for almost every realistic call (parent-
linked tasks, priority-set items, status-overridden ideas, etc.).
Mirrored the CLI's behaviour (cmd/pad/main.go ~L2200): build a fields
map from the named flags, overlay the repeatable --field entries on
top, JSON-encode into ItemCreate.Fields. The handler's existing
schema-validation + parent-resolution path now runs unchanged.
Repeatable --field still wins last-write — locked into a new test so
it doesn't drift.
Also rejects --assign / --role with a clear error rather than silently
dropping them. The CLI resolves user-name → user-ID and role-slug →
role-ID via additional API calls before posting; replicating that
pre-resolution belongs in a follow-up that expands the route table for
production use. Failing loudly is better than partial parity.
Tests:
- TestHTTPHandlerDispatcher_RoutesItemCreate now asserts the
status/priority/category/parent values land in fields, not the top
level — guards against the regression directly.
- TestMapItemCreate_ExplicitFieldOverridesNamedFlag locks the
last-write-wins precedence between --status and --field status=...
- TestMapItemCreate_RejectsUnsupportedAssignRole asserts the
defensive error path for the deferred flags.
Parent: PLAN-943.
* fix(mcp): persist source=cli for HTTPHandlerDispatcher calls per Codex review (round 2)
Codex caught: actorFromRequest derives source from the Authorization
header — without one, dispatcher-driven calls would persist
source="web" instead of source="cli", regressing dashboard/standup/
audit attribution vs. ExecDispatcher.
Same pattern as the round-1 CSRF fix: extend actorFromRequest to also
honor the ctxIsAPIToken context flag (which TokenAuth sets on the live
Bearer-auth path and HTTPHandlerDispatcher sets via
server.WithAPITokenAuth on synthesized requests). Both signals mean
"non-cookie authenticated, attribute as CLI/agent traffic".
Integration test now asserts source="cli" on the created item, so any
future regression of this attribution surfaces immediately.
Parent: PLAN-943.
* fix(mcp): normalize collection aliases in HTTPHandlerDispatcher per Codex review (round 3)
Codex caught: CLI's `item create task ...` works because
cmd/pad/main.go's normalizeCollectionSlug maps singular/short forms
("task" → "tasks", "doc" → "docs", etc.) to the canonical slug
before posting. HTTPHandlerDispatcher's mapItemCreate skipped that
step, so the same documented call shape would 404 through the HTTP
transport even though it worked through ExecDispatcher.
Extracted the alias map to internal/collections.NormalizeSlug so the
two transports stay in lockstep without duplication. cmd/pad/main.go's
normalizeCollectionSlug now delegates to it; the in-process
dispatcher calls it from mapItemCreate after pulling the collection
out of input.
TestMapItemCreate_NormalizesCollectionAliases locks every documented
alias plus a passthrough case for custom collections.
Parent: PLAN-943.
* fix(server): WithTokenWorkspaceID actually clears on empty input per Codex review (round 4)
Codex caught: the docstring said "Pass an empty string to clear" but
the implementation early-returned `ctx` unchanged in that case,
leaving any stale ctxTokenWorkspaceID set further up the chain
active. Always overwrite so the contract holds: passing "" produces
a context where tokenWorkspaceID(r) returns "", same as a never-set
context.
Parent: PLAN-943.
|
||
|
xarmian
|
e05ea07d62 |
fix(docs): use canonical wire path capabilities.experimental.padCmdhelp (#342)
Codex caught the same accuracy issue on pad-web that exists in five
spots in this repo: prose described the handshake location as
"serverCapabilities.experimental.padCmdhelp", but per the MCP spec
the InitializeResult shape is
{ result: { capabilities: { experimental: { ... } } } }
There's no `serverCapabilities` field on the wire — `ServerCapabilities`
is the Go-side struct type name in mcp-go; the JSON tag is
`capabilities`. Anyone copying the path out of our docs to navigate
a real JSON-RPC envelope was getting the wrong key.
Updated to `capabilities.experimental.padCmdhelp` (or the fully
qualified `result.capabilities.experimental.padCmdhelp` where the
JSON-RPC envelope context wasn't otherwise obvious) in:
- README.md — public-facing prose
- CLAUDE.md — agent-facing prose
- internal/mcp/version.go — discovery-surfaces doc comment + the
experimentalCapabilityKey doc comment
- internal/mcp/server.go — comment near WithExperimental
- internal/mcp/meta.go — experimentalCapabilities() doc + the wire
shape example (now wrapped under `result` for accuracy)
- internal/mcp/server_test.go — test docstring + failure message
- cmd/pad/mcp.go — comment near RegisterMeta
The Go type `serverCapabilities` in `internal/server/handlers_capabilities.go`
is unrelated (it's the response shape for `GET /api/v1/server/capabilities`)
and stays as-is.
No code/behaviour changes; pure prose accuracy fix. `make check` clean.
Companion fix to pad-web PR #42, which Codex flagged the same issue on.
|
||
|
xarmian
|
2d98f2a170 |
feat(mcp): advertise cmdhelp_version stability tier in handshake (TASK-963) (#340)
* feat(mcp): advertise cmdhelp_version stability tier in handshake (TASK-963)
External agents (Cursor, Claude Desktop, the future Pad Cloud remote MCP
in PLAN-943) depend on tool names, argument shapes, and resource URIs
being stable across pad releases. Without an explicit contract, any
future surface change breaks consumers silently.
This commit ships the contract on two complementary surfaces:
- serverCapabilities.experimental.padCmdhelp in the initialize handshake
— namespaced map carrying {version, tool_surface_stable}, discoverable
in one round-trip.
- pad://_meta/version static resource — full JSON document with
{pad_version, cmdhelp_version, tool_surface_stable, mcp_protocol_version}
for clients that prefer reading a typed payload.
CmdhelpVersion is pinned at "0.1" — the initial cmdhelp-derived surface
shipped in PLAN-942. Bump the major when tool names / arg shapes /
resource URIs change incompatibly.
Tests:
- TestServer_InitializeHandshake extended to assert the experimental
capability shape on the wire (not just the existence of the field).
- TestBuildMetaPayload_* lock the payload field names + fallback
behaviour.
- TestRegisterMeta_ResourceRoundTrip drives the resource through the
real HandleMessage path so a regression in the dispatcher would
surface as a test failure.
Docs:
- README's MCP section briefly mentions the contract surfaces.
- CLAUDE.md's MCP section gets a stability-contract paragraph + the new
resource URI.
- Public docs at getpad.dev/mcp/local will need a follow-up PR in the
pad-web repo (per CONVE-159) — captured at the end of TASK-963.
Parent: PLAN-942.
* fix(mcp): source MCP protocol version from mcp-go LATEST_PROTOCOL_VERSION per Codex review (round 1)
Codex caught: the local MCPProtocolVersion constant was pinned at
"2024-11-05", but mcp-go@v0.50.0 negotiates "2025-11-25" for clients
that request mcp.LATEST_PROTOCOL_VERSION. The meta resource was
therefore reporting a protocol revision newer than what the server
actually speaks, which defeats the field's purpose for feature
detection (e.g. RFC 8707 Resource Indicators land in 2025-11-25).
Drop the local constant and read mcp.LATEST_PROTOCOL_VERSION at
BuildMetaPayload time so the value tracks whatever revision the
linked library will negotiate. The handshake's serverInfo.version
already does this implicitly via NewMCPServer; making the meta
resource follow the same source-of-truth keeps both surfaces in
lockstep across mcp-go upgrades.
Test updated to assert against mcp.LATEST_PROTOCOL_VERSION instead of
the removed constant, plus an "empty-string" guard in case a future
library refactor unsets the constant.
Parent: PLAN-942.
|
||
|
xarmian
|
e90ee18907 |
feat(mcp): pad mcp install / uninstall / status (TASK-948) (#338)
* feat(mcp): pad mcp install / uninstall / status (TASK-948)
One-shot config writers for the three MCP-capable client apps:
pad mcp install claude-desktop # ~/.config/Claude/claude_desktop_config.json (linux)
pad mcp install cursor # ~/.cursor/mcp.json
pad mcp install windsurf # ~/.codeium/windsurf/mcp_config.json
pad mcp install --all # all three
pad mcp uninstall cursor # remove
pad mcp status # report install state
Implementation:
- internal/mcp/install.go (new) — Agent registry with per-OS path
resolvers (PathFor takes (home, goos) so tests inject); AddPadEntry
/ RemovePadEntry / HasPadEntry primitives that read-modify-write
JSON, preserving every entry except mcpServers.pad. Installer
façade with Home/GOOS overrides for tests.
- cmd/pad/mcp.go — three new cobra subcommands wired into mcpCmd:
install (no-args = status, --all = batch), uninstall, status.
Binary path resolved via os.Executable().
DOD coverage:
- Existing entries preserved (TestAddPadEntry_PreservesOtherServers
asserts both other mcpServers and unrelated top-level keys survive).
- Idempotent install (binary unchanged → modified=false).
- Update install (binary changed → modified=true).
- Idempotent uninstall (missing file / missing entry → no-op).
- Per-platform path resolution tested for linux + darwin.
- 16 unit tests including edge cases: empty/whitespace files,
malformed JSON rejected (no silent overwrite), case-insensitive
agent aliases.
Live verified end-to-end:
- HOME=/tmp/fakehome pad mcp install cursor → writes valid JSON
- pad mcp status → shows [x] Cursor with command path
- pad mcp uninstall cursor → leaves mcpServers:{} skeleton
Config file perms: 0600 (configs may hold credentials for OTHER
MCP servers; tighten on principle).
Parent: PLAN-942.
* fix(mcp): tighten install argument validation + chmod existing configs (Codex round 1)
Two findings on PR #338:
1. `pad mcp install` had no Args validator so cobra silently accepted
extras: `pad mcp install cursor windsurf` only installed Cursor.
Added cobra.MaximumNArgs(1) plus an explicit guard rejecting
`--all` combined with an agent name (those flows are
mutually exclusive).
2. os.WriteFile(path, data, 0o600) only honors the mode when CREATING
the file. A pre-existing 0644 config kept 0644 after the install,
defeating the security-tightening claim in the comment. Added an
explicit os.Chmod(path, 0o600) after writing; chmod failures are
stderr warnings, not hard errors (the data write already
succeeded; perms hardening is best-effort defense-in-depth).
New test TestAddPadEntry_TightensExistingFilePerms locks the
0600-after-install contract; live verified the cobra guards reject
both error cases with clean messages.
Parent: PLAN-942.
* fix(mcp): tighten perms on idempotent install path too (Codex round 2)
Codex caught: AddPadEntry's no-op early-return (when desired config
matches existing) skipped the chmod step from round 1's fix. So an
already-up-to-date 0644 config retained 0644 after re-running
`pad mcp install`.
Extracted tightenPerms() as a helper called from BOTH paths:
- writeJSONConfig (modified path) — chmod after write
- AddPadEntry's no-op return — chmod even when content is unchanged
Best-effort: chmod failures still emit a warning rather than failing
the install (the user's intent already succeeded; perms tightening is
defense-in-depth, not core functionality).
New test TestAddPadEntry_TightensPermsOnIdempotentNoop locks the
no-op-path contract.
Parent: PLAN-942.
|
||
|
xarmian
|
ca2fc04a5b |
feat(mcp): static prompts lifted from SKILL.md (TASK-947) (#337)
Four MCP prompts expose pad's high-value multi-step workflows so agents can prompts/get them as user-role system messages: pad_plan — draft + decompose a Plan pad_ideate — brainstorm + capture as items pad_retro — retrospective on a completed Plan pad_onboard — workspace onboarding / codebase scan Implementation: - internal/mcp/prompts.go (new) — RegisterPrompts(srv) + PromptBody accessor; sorted iteration for deterministic prompts/list ordering. - internal/mcp/prompts_data.go (new) — embedded body strings, lifted near-verbatim from skills/pad/SKILL.md "Multi-Step Workflows". - cmd/pad/mcp.go wires RegisterPrompts after the resources path. 7 unit tests including SKILL.md drift contract: - All four prompts registered + reachable via PromptBody - Each body has the standard "# Pad: <workflow>" heading - Unknown prompt name returns error - Lockstep: every prompt body contains its key SKILL.md CLI invocations (catches silent drift if SKILL.md is updated without bumping prompts) - skills/pad/SKILL.md still exists as the source-of-truth (catches rename / removal during refactors) Live smoke: prompts/list returns 4 prompts (with descriptions); prompts/get pad_plan returns 1225 chars of workflow text starting "# Pad: Plan workflow\n\nYou are helping the user...". Naming choice: `pad_plan` (snake_case) over `pad/plan` (slash form) — some MCP clients interpret slashes as namespace paths. Matches the tool-naming convention from TASK-945. Parent: PLAN-942. |
||
|
xarmian
|
342a564113 |
feat(mcp): read-only resource templates (TASK-946) (#336)
* feat(mcp): read-only resource templates for items / dashboard / collections (TASK-946)
Four MCP resource templates expose pad workspace state to agents
without requiring a tool invocation:
pad://workspace/{ws}/items/{ref} → single item markdown
pad://workspace/{ws}/items → list of items (JSON)
pad://workspace/{ws}/dashboard → project dashboard (JSON)
pad://workspace/{ws}/collections → collections + schemas (JSON)
Why resources, not tools: agents can `resources/read` a URI and
ingest the body directly into context without going through a
tool-call round-trip. Useful for "load TASK-5 then plan" workflows
where the agent shouldn't need to pick a tool.
Implementation:
- internal/mcp/resources.go (new) — RegisterResources installs all
four templates on an MCPServer; ResourceFetcher interface +
ExecResourceFetcher shell-out (separate from Dispatcher because
resource handlers return raw bytes, not CallToolResult).
- parsePadURI extracts (workspace, kind, arg) from pad:// URIs;
defensive guards reject mismatched URIs at each handler.
- rootFlagsToArgs forwards startup --url to every fetched call
(same contract as TASK-945's tool dispatch).
- cmd/pad/mcp.go wires RegisterResources after the tool registry
in mcpServeCmd.
15 new unit tests:
- parsePadURI: all 4 forms + 4 malformed inputs
- each handler: dispatches correct CLI args + MIME type
- readItem rejects mismatched URI (defensive)
- fetch errors propagate as Go errors (so MCP returns JSON-RPC
error rather than empty contents)
- root flag forwarding via the resources path
- ExecResourceFetcher: missing binary, stdout capture, non-zero
exit folds stderr into error
Live verified:
- resources/templates/list returns 4 templates with correct mime
types and uri patterns.
- resources/read pad://workspace/docapp/items/TASK-944 returns
1764 bytes of markdown.
Parent: PLAN-942.
* fix(mcp): compose full item markdown from JSON in resource path (Codex round 1)
Codex flagged: pad://workspace/{ws}/items/{ref} fetched
`pad item show --format markdown` which prints only item.Content
(see cmd/pad/main.go:2562). The resource description promised
"Full markdown content … includes title, fields, body, and links",
so clients reading the URI lost ref/title/metadata/parent and
couldn't reliably identify the item.
Fix scoped to the resource path (rather than changing the CLI's
markdown output, which other callers may parse): readItem fetches
`--format json` and a new formatItemAsMarkdown composes the
document — heading with ref + title, optional parent link, sorted
metadata fields, then the content body.
3 new unit tests + the existing readItem test rewritten:
- Full-shape JSON → exact markdown layout (deterministic via sorted keys)
- Missing fields → heading-only doc, no panic
- Empty `{}` fields → no stray list section
- Invalid JSON → error propagates
Live verified: pad://workspace/docapp/items/TASK-944 now returns
"# TASK-944: <title>\n\n**Parent:** PLAN-942 — ...\n\n- **priority:**
high\n- **status:** done\n\n<body>" — full identification + traversable
parent link, body intact.
Parent: PLAN-942.
|
||
|
xarmian
|
2e4a815d0c |
feat(mcp): cmdhelp-derived tool registry + shell-out dispatch (TASK-945) (#335)
* feat(mcp): cmdhelp-derived tool registry + shell-out dispatch (TASK-945)
The strategic centerpiece of PLAN-942: walk the cmdhelp Document built
from `pad`'s cobra tree and register every leaf as an MCP tool, with
shell-out dispatch back to the running binary. New pad commands (or
new flags) extend the MCP surface for free — no hand-mapping ~73
commands.
- internal/mcp/registry.go — Register() walks cmdhelp.Document, picks
leaves, applies a curated DefaultExcludes (db ops, auth, init,
agent install/update, server lifecycle, completion, edit, watch,
workspace lifecycle), builds an MCP Tool per leaf with input schema
derived from cmdhelp Arg/Flag types. Snake-case names: "item create"
→ "item_create".
- internal/mcp/dispatch.go — ExecDispatcher shells out to the pad
binary; BuildCLIArgs is a pure function that translates the JSON
args into a CLI invocation (positionals → flags → workspace
injection → --format json default). JSON stdout is surfaced as
StructuredContent for rich client rendering.
- internal/mcp/workspace.go — WorkspaceState (RWMutex-protected) +
pad_set_workspace built-in tool. Empty string clears the session
default; missing arg returns IsError without mutating state.
- cmd/pad/mcp.go — wire registry into `pad mcp serve` startup; build
the cmdhelp Document from cmd.Root(), resolve the running binary
via os.Executable, seed workspace from --workspace flag.
- 23 new unit tests across registry / dispatch / workspace files
(race-detector clean) covering: leaf identification, exclusion
prefix suppression, snake-case naming, pure CLI arg translation
(positionals + bool presence form + repeatable args & flags +
workspace/format injection), exec dispatcher (binary missing,
stdout capture, non-zero exit), workspace state mutation, and
end-to-end pad_set_workspace handler contract.
Live smoke (real binary, real stdio):
- tools/list returns 66 tools — pad_set_workspace + item_create
present, db_backup + mcp_serve correctly excluded.
- tools/call pad_set_workspace updates session state, then
auth_whoami shells out and returns structured JSON.
Parent: PLAN-942.
* fix(mcp): forward --url root flag + drop unwired --stdin (Codex round 1)
Two findings from Codex review of #335:
P1: --url root persistent flag was not forwarded to dispatched
subprocesses. If an MCP client launches `pad --url X mcp serve`,
every tool call ran against the default URL instead of X. Fixed by
adding RootFlags map[string]string to RegistryOptions; cmd/pad/mcp.go
captures urlFlag at startup and threads it through. BuildCLIArgs
now also takes a rootFlags map and injects each entry when not in
input (empty values skipped, agent value wins on collision).
P2: MCP tool schemas exposed `--stdin` flags but ExecDispatcher
never piped the agent's stdin to the subprocess. Calling e.g.
`item_create {stdin: true}` would block on EOF and create empty
content. The `--content` flag covers the same semantic via JSON
args, which IS wired. Hide stdin from the MCP surface (buildTool
filters out flagsHiddenFromMCP) AND drop it defensively in
BuildCLIArgs in case an agent's stale schema cache passes it.
Tests added (4 new + 2 updated):
- BuildCLIArgs: stdin dropped defensively, root flags injected,
empty root flag skipped, agent value wins over root flag.
- buildTool: omits stdin from input schema.
- Dispatch handler: forwards root flags through to CLI args.
Existing TestBuildCLIArgs_BoolPresenceForm rewritten to use
`dry-run` flag (since stdin is now filtered).
Live verified: `pad mcp serve` tools/list shows item_create with
content+10 other flags, no stdin. Round-trip preserved.
Parent: PLAN-942.
|
||
|
xarmian
|
9905a83134 |
feat(mcp): pad mcp serve skeleton on stdio (TASK-944) (#333)
Stand up internal/mcp + the cobra `pad mcp serve` subcommand. v1 is
handshake-only — the server completes initialize and stays alive over
stdio, advertising tool capability with an empty registry. TASK-945
fills that registry from `pad help --format json`.
- New internal/mcp package wraps mark3labs/mcp-go's stdio transport;
graceful shutdown on EOF / SIGINT / SIGTERM / ctx-cancel.
- New cmd/pad/mcp.go registers `pad mcp` as a top-level cobra group
with the `serve` subcommand wired to internal/mcp.NewServer.
- 4 unit tests: NewServer construction, real initialize round-trip
(asserts serverInfo.name + version), fallback version locked,
graceful shutdown on ctx-cancel.
Live smoke: `echo '<initialize>' | pad mcp serve` returns
`serverInfo:{name:"pad-mcp",version:...}` with `tools:{listChanged:true}`.
cmdhelp emits the new command tree at `pad help mcp serve --format json`.
Parent: PLAN-942.
|
||
|
xarmian
|
cfda4463e8 |
feat(cmdhelp): tests + golden contract + drift validator (TASK-938) (#332)
* feat(cmdhelp): tests + golden contract + drift validator (TASK-938)
The verification layer that turns cmdhelp v0.1 from "implementation"
into "stable contract." Three categories of tests, all running in
`go test ./...`:
1. Schema validation (cmdhelp.schema.json as CI gate)
- internal/cmdhelp/schema_test.go — synthetic tree's emitted JSON
validates after static walk, after dynamic resolution, and after
a no-workspace fallback.
- cmd/pad/cmdhelp_real_test.go — the REAL pad cobra tree's emitted
JSON validates against the published schema. Future regressions
caught: types outside the closed vocabulary, non-numeric exit_code
keys, flag names violating propertyNames, malformed cmdhelp_version.
2. Drift-prevention contract (spec §6 / §11 Q5)
- internal/cmdhelp/example_validation.go — ValidateExamples walks
every example's `cmd` string, tokenizes with shellSplit, resolves
non-flag tokens against the live cobra tree, and asserts every
--flag exists on the resolved command (or any ancestor for
persistent / inherited flags). Negate-flag form (`--no-cache`)
is recognized via the negation rule from spec §5.3.
- shellSplit handles double/single quotes, backslash escape, and
stops at unquoted pipeline boundaries (|, ;, &, >, <) so the
validator only checks the first command in a pipeline.
- ValidateBoolArity asserts no bool flag appears in valued form
(--flag=value) anywhere in its examples (spec §5.3).
- cmd/pad/cmdhelp_real_test.go runs both validators against the
real pad tree as CI gates.
- Negative tests in internal/cmdhelp/example_validation_test.go
prove the validator catches: typo'd flag (--priorty), unknown
command path, valued-form bool flag.
3. Capabilities form equivalence (spec §8)
- cmd/pad/cmdhelp_real_test.go — both forms (help --capabilities
and --cmdhelp-capabilities fallback) produce byte-identical
output. Side-effect-free guarantee verified by passing garbage
args alongside the fallback flag.
Refactors enabling the tests:
- cmd/pad/main.go: extract newRootCmd() so tests can build the real
cobra tree without running it. main() body shrinks to two lines.
- cmd/pad/main.go: extract handleCmdhelpCapabilitiesFallback() so the
fallback's side-effect-free contract is directly assertable instead
of requiring a subprocess.
Parser improvements driven by real-pad-tree drift findings:
- parseExamplesFromLong: strip same-line `# comment` annotations so
`pad foo --bar # one item's attachments` doesn't pollute Examples.
stripCommentIndex is quote-aware (# inside "..." or '...' is literal).
- main.go (github cmd): the Long had annotations on example lines
separated only by spaces (no `#`), which was malformed input. Fixed
to use `#` separators — caught by the drift validator on first run.
New deps:
- github.com/santhosh-tekuri/jsonschema/v6 v6.0.2 — Go JSON Schema
validator supporting draft 2020-12 (matches the cmdhelp schema's $schema).
New helpers in internal/cmdhelp/:
- FindAndCompileSchema(startDir) walks up to locate
schema/cmdhelp.schema.json and returns a compiled schema. Reusable
by any consumer that wants to validate cmdhelp documents.
End-to-end on real binary:
- pad help --format json → 100 commands, schema-valid.
- All examples in pad's emitted output resolve against the live tree
(zero drift findings).
- pad help --capabilities byte-identical to pad --cmdhelp-capabilities.
- Adding a typo'd flag in any cobra Long block in cmd/pad MUST break
TestRealPadTree_ExampleDriftValidator. Verified by the negative
TestValidateExamples_DetectsTypoFlag.
make check clean. All 53 cmdhelp + cmd/pad tests pass.
Parent: PLAN-930.
* fix(cmdhelp): pass full token stream to cobra.Find per Codex review (round 1)
Codex round 1 caught: ValidateExamples stopped collecting the command
path at the first flag, so an example like
pad --workspace foo item create task --priority high
resolved to root, not `item create`. That meant `--priority` was
checked against root's flag set (where it doesn't exist) — false
positive — AND the validator silently missed any command-path drift
after a leading root flag.
Cobra's own Find walks the full token stream and uses each command's
flag definitions to skip flag/value pairs while matching subcommand
names. Pass tokens[1:] directly to root.Find — let cobra handle the
interleaving correctly.
New test:
- TestValidateExamples_FlagBeforeSubcommandResolvesToCorrectTarget —
flag-before-subcommand resolves to the leaf and accepts leaf flags.
|
||
|
xarmian
|
76c9d5aae5 |
feat(cmdhelp): parse Examples blocks from cobra Long as fallback (TASK-939) (#331)
The original TASK-939 ask was to migrate every cobra command's
"Examples:" block from Long into the dedicated Example field. Pad has
102 cobra commands; manually migrating each is a hundreds-of-lines
change with high regression risk and zero user-visible improvement
(cobra renders "Examples:" sections in Long identically to the Example
field — the difference is only machine-readability).
Higher-leverage approach: enrich the cmdhelp emitter to fall back to
parsing Long when the Example field is empty. One small, testable change
in internal/cmdhelp unlocks examples in cmdhelp output for every command
that already has an "Examples:" block — without touching any of the 102
command sites. Structural migration becomes optional polish (HT-941).
Implementation:
- internal/cmdhelp/json.go gains parseExamplesFromLong: locate a
stand-alone "Examples:" / "Example:" header, collect indented
invocation lines until blank-then-prose or end. Comment lines (`#`)
are dropped from the block. The header regex is anchored to a line
on its own (`^\s*Examples?:\s*$`) so prose containing the word
"Examples" doesn't trigger the fallback.
- buildCommand() prefers cmd.Example when set; falls back to
parseExamplesFromLong(cmd.Long) when Example is empty. Tests assert
precedence so future migrations to the Example field win cleanly.
- cmd/pad/main.go: completion command gets a dedicated Example field
(one of the few that didn't have an "Examples:" block at all). Demos
the migration pattern HT-941 will sweep across the rest.
Result on the real binary:
pad help --format json → before: 0/100 commands have examples
after: 24/100 commands have examples
pad help item create --format json → 4 examples (vs 0 before)
pad help completion --format json → 4 examples (from Example field)
The remaining ~76 commands genuinely lack an Examples: block in Long
(or are group commands that don't need examples). HT-941 captures the
sweep work needed to get those to 100%.
Tests:
- 7 new tests for parseExamplesFromLong in internal/cmdhelp/json_test.go:
basic block extraction, no-header (Usage: != Examples:), variant
headers (singular/plural, indented), empty/malformed inputs, stops
at unindented prose, drops comment lines.
- 2 new end-to-end tests via Build():
- falls back to Long when Example is empty
- prefers Example field when both set (precedence)
- All 39 existing cmdhelp tests + 16 routing tests still pass.
- make check clean.
Follow-up: HT-941 ("Migrate cobra Long Examples blocks to dedicated
Example fields") captures the structural sweep — broken into per-group
PRs (auth/*, agent/*, server/* etc.) so it can be done incrementally
without blocking PLAN-930.
Parent: PLAN-930.
|
||
|
xarmian
|
0439c1bf3d |
feat(cmdhelp): --capabilities discovery flag + --cmdhelp-capabilities fallback (TASK-937) (#330)
Implements the cmdhelp v0.1 §8 capability bit so wrappers can detect support without trial and error. - pad help --capabilities → cmdhelp/0.1: text, md, json, llm - pad --cmdhelp-capabilities → same line (spec §8 fallback form) Both forms: - Single line on stdout (terminated by newline only). - Side-effect-free: no logging, no network, no config writes, no auth challenge. Verified by running with no workspace context (cwd /tmp, no auth) — still emits the line and exits 0. - Exit 0 on success. - Format: cmdhelp/<MAJOR>.<MINOR>: <comma-separated formats>. Why both forms: Spec §8 lists `<cmd> help --capabilities` as preferred and `<cmd> --cmdhelp-capabilities` as a fallback for CLIs whose `help` subcommand is overloaded. Pad's `help` is not overloaded, but supporting both forms costs nothing and lets wrappers and harnesses choose whichever convention they prefer — TASK-938 will assert equivalence between them. The fallback is handled in main() before cobra parsing so it really is side-effect-free: it doesn't even reach config.Load() or the detect-workspace path. A simple os.Args scan + early return. Files: - internal/cmdhelp/json.go — new CapabilityLine(formats) helper that produces the spec-format string. Caller passes the format set so different binaries can advertise different surfaces; the helper preserves caller order (spec §8 says order isn't significant). - cmd/pad/help_cmdhelp.go — adds --capabilities to helpCmd; new padCmdhelpFormats constant ["text","md","json","llm"]; short-circuit in RunE before any other logic runs. - cmd/pad/main.go — pre-args scan handles --cmdhelp-capabilities fallback before rootCmd.Execute(). Tests: - TestCapabilityLine_FormatExact — exact string match. - TestCapabilityLine_HonorsCallerOrderAndSet — preserves caller order. - TestHelpCmd_CapabilitiesExactString — exact-byte assertion on the output of `padtest help --capabilities` including the trailing newline. - TestHelpCmd_CapabilitiesShortCircuits — verifies --capabilities wins over --format / --depth / extra args (spec §8 side-effect rule). - All 35 existing cmdhelp tests + 14 routing tests still pass. - make check clean. End-to-end on real binary: pad help --capabilities → "cmdhelp/0.1: text, md, json, llm" exit 0 pad --cmdhelp-capabilities → same cd /tmp && pad help --capabilities → still works, no auth needed pad help item --capabilities --format json --depth 0 → short-circuits Parent: PLAN-930. |
||
|
xarmian
|
e6fd25322e |
feat(cmdhelp): dynamic enum resolution + workspace context (TASK-936) (#329)
* feat(cmdhelp): dynamic enum resolution + workspace context (TASK-936)
The killer differentiator from the cmdhelp v0.1 spec — splice live
workspace facts into help output so an LLM asking "what collections
exist?" gets the real answer rather than a generic "any string".
Files:
- internal/cmdhelp/dynamic.go (new) — Resolver type with Apply method.
ArgEnumSources / FlagEnumSources map names to enum_source identifiers;
Sources maps enum_source to a fetcher func. Apply walks the Document,
stamps enum_source on matching args/flags, populates Enum from the
fetcher, and sets doc.Context.Workspace. Per-Apply caching keeps each
source func to ≤1 invocation regardless of how many commands need it.
- internal/cmdhelp/json.go — added Options.Resolver; Build calls
Resolver.Apply after the static walk, so callers that inspect Build
output as either pre- or post-resolution still work.
- cmd/pad/help_cmdhelp.go — newDynamicResolver constructs a Resolver
bound to the runtime: workspace from DetectWorkspace, server URL from
config, three sources (collections, roles, members). Returns nil when
no workspace is detected so help still works outside any workspace.
cmdhelpOptions takes target so Binary derives from root.Name() instead
of hardcoded "pad" (preserves test-tree bindings for synthetic roots).
Pad-side bindings (matches `pad item create --help`'s existing context):
arg collection → dynamic:pad collection list
flag role → dynamic:pad role list
flag assign → dynamic:pad workspace members
End-to-end on the real binary (inside docapp workspace):
pad help item create --format json
→ args[0].collection: type=enum, enum=[ideas,conventions,...,roadmap],
enum_source="dynamic:pad collection list"
→ flags.role: enum=[planner,implementer,reviewer]
→ flags.assign: enum=[dave]
→ context.workspace="docapp"
pad help --format md → "## Workspace context\n- workspace: `docapp`"
Outside any workspace (cd /tmp; pad help item create --format json):
→ collection arg: type=string, no enum, no enum_source (graceful fallback)
→ context: null
→ output still validates against schema/cmdhelp.schema.json
Fail-safe behavior:
- newDynamicResolver returns nil on any config/detection error → static doc.
- Per-source fetcher errors are caught inside Apply → enum_source still
announced on the binding arg/flag, but Enum is left empty. The help
command MUST NOT fail because dynamic facts can't be fetched.
- Existing Enum values from alternation/ValidArgs are preserved
(resolver only fills the gap, never overwrites authoritative spec).
Tests:
- 10 dynamic-resolver tests in internal/cmdhelp/dynamic_test.go
covering: arg + flag enum population, context population, per-source
caching across multiple commands, graceful error handling, nil
resolver as no-op, existing-Enum preservation, global flag resolution,
unaffected commands left unchanged, end-to-end via Build().
- All 24 prior cmdhelp tests + 12 routing tests still green.
- make check clean (lint + go test + web build).
Out of scope (deferred):
- --capabilities discovery flag — TASK-937.
- Schema-validate live output in CI — TASK-938.
- Audit pad's existing commands' Examples — TASK-939.
Parent: PLAN-930.
* fix(cmdhelp): scope --role / --assign bindings per-command per Codex review (round 1)
Codex round 1 on PR #329 caught a real semantic bug: globally binding
--role to dynamic:pad role list was wrong because pad has two
unrelated --role flags:
pad workspace invite --role workspace role: owner|editor|viewer
pad item create --role <slug> agent role slug
pad item update --role <slug> agent role slug
Globally announcing agent-role slugs as the values for `pad workspace
invite --role` would mislead consumers (LLMs would suggest "planner"
when "owner" is expected; tab-completion would offer the wrong set).
Fix:
- Resolver gains CommandArgBindings and CommandFlagBindings
(map[path]map[name]source) — scoped to a specific command path.
Per-command bindings win over wildcard ArgEnumSources/FlagEnumSources
when both match.
- Helper methods argSource(path,name) / flagSource(path,name) own the
precedence rule so both args and flags use it consistently.
- newDynamicResolver in cmd/pad keeps `<collection>` as a wildcard
ArgEnumSources (universal — every <collection> in pad means a pad
collection), but moves --role and --assign into CommandFlagBindings
scoped to "item create", "item update", and "item list". `pad
workspace invite --role` is intentionally left without a binding.
- A header comment in newDynamicResolver enumerates every --role /
--assign site in the CLI and which one each binding targets, so a
future reviewer adding a new flag can see the rule at a glance.
End-to-end on the real binary:
pad help item create --format json
→ flags.role: type=enum, enum=[planner,implementer,reviewer], enum_source=...
pad help workspace invite --format json
→ flags.role: type=string (untouched). ✓
New tests:
- TestResolver_Apply_PerCommandBindingScoped — explicitly mirrors the
Codex finding: two commands both have a `role` flag, only the bound
command resolves. workspace-invite-style isolation regression test.
- TestResolver_Apply_PerCommandWinsOverWildcard — precedence: when
both wildcard and per-command match, per-command wins.
- TestResolver_Apply_PerCommandArgBindings — same precedence rule
for positional args.
All 33 cmdhelp tests + 12 routing tests still green; make check clean.
* fix(cmdhelp): bind item list --role to agent roles per Codex review (round 2)
Codex round 2 caught that item list --role was still unbound — I missed
it in round 1's grep because the variable name is `&roleFilter` rather
than `&roleFlag`. Pad has 4 --role flags total:
pad workspace invite --role workspace role (NOT bound)
pad item create --role agent role slug (bound)
pad item update --role agent role slug (bound)
pad item list --role agent role filter (now bound)
Fix: extend CommandFlagBindings["item list"] to include the same
itemRoleAssign map as item create/update, so all three item subcommands
that reference an agent role get the dynamic binding.
Added a `grep` recipe in the comment so a future maintainer adding a
new --role / --assign site can find every existing one in one shot
(both `&roleFlag` and `&roleFilter` style declarations).
End-to-end on real binary (inside docapp workspace):
pad help item list --format json
→ flags.role: enum=[planner,implementer,reviewer], enum_source set ✓
→ flags.assign: enum=[dave], enum_source set ✓
make check clean.
|
||
|
xarmian
|
5e93abe552 |
feat(cmdhelp): implement --format md emitter (TASK-935) (#328)
Adds internal/cmdhelp/md.go that renders the Document built in TASK-934
as markdown with the predictable section order from cmdhelp v0.1 §6.
Replaces the markdown stub in cmd/pad/help_cmdhelp.go so `pad help
--format md` (and the `--llm` alias) produce real output.
Section order per command (spec §6):
## `binary path`
summary / description (when distinct from summary)
### Synopsis — fenced usage line, reconstructed from args + flags
### Arguments — table with name | type | required | description
### Flags — table with flag | type | default | description
### Stdin — when Stdin.Accepted is true
### Examples — fenced bash blocks, drawn from same canonical
example set as JSON (spec §6 same-source rule)
### Output — text_template + json_schema_ref when populated
### Exit codes — table when ExitCodes is populated
### See also — bullet list of related command paths
Top-level YAML frontmatter:
cmdhelp_version, binary, version, generated (RFC3339, UTC).
Now is overridable via Options.Now for snapshot-test stability.
Top-level structure: `# binary` heading, summary, optional homepage,
optional `## Workspace context` (populated in TASK-936), `## Global flags`
table, then per-command sections sorted by path for determinism.
Synopsis reconstruction uses the structured Args from Build() (rather
than cobra.UseLine) so JSON and MD stay driven by the same parsed data
— the variadic `<ref>...` and alternation enums from TASK-934 carry
through naturally.
Pipes in flag/arg descriptions are escaped (`\|`) so they don't break
markdown table grids.
cmd/pad/help_cmdhelp.go: emitCmdhelpMarkdown stub replaced with a call
into cmdhelp.EmitMarkdown. --depth/--all threaded through MaxDepth
identically to the JSON path.
Tests:
- 15 markdown emitter tests in internal/cmdhelp/md_test.go covering
frontmatter (presence + timestamp injectability), per-command
section order, synopsis reconstruction (incl. variadic + alternation),
global-flag dedup, fenced-bash examples, hidden-thing exclusion,
deterministic ordering, Stdin/Output/ExitCodes/SeeAlso sections,
Workspace context, table-pipe escaping.
- TestHelpCmd_FormatMarkdownStubError replaced by
TestHelpCmd_FormatMarkdownEmits (asserts frontmatter + structural
markers for both md and llm).
- TestHelpCmd_FormatLLMAliasRoutesToMarkdown replaced by
TestHelpCmd_FormatLLMIsAliasForMD (asserts md and llm produce
byte-identical output modulo the timestamp).
End-to-end on the real binary:
- pad help --format md emits valid markdown with all sections.
- pad help --format llm produces byte-identical output (after
timestamp normalization).
- pad help item create --format md scopes correctly.
- make check clean.
Out of scope (deferred):
- Dynamic Workspace context population — TASK-936.
- --capabilities discovery flag — TASK-937.
- Schema-validation + golden-file tests in CI — TASK-938.
- Examples populated for all pad commands (still in cobra Long for now)
— TASK-939.
Parent: PLAN-930.
|
||
|
xarmian
|
eecc683ab0 |
feat(cmdhelp): implement --format json emitter (TASK-934) (#327)
* feat(cmdhelp): implement --format json emitter (TASK-934) Adds internal/cmdhelp package that walks the cobra command tree and emits a cmdhelp v0.1 Document conforming to schema/cmdhelp.schema.json. Wires it into cmd/pad/help_cmdhelp.go so `pad help --format json` is no longer a stub. Files: - internal/cmdhelp/types.go — Document/Command/Arg/Flag/Stdin/Stdout/ ExitCode/Example structs mirroring the schema. ExitCode implements custom MarshalJSON for the string-or-object union (spec §5.2). - internal/cmdhelp/json.go — Build() walks target's subtree; EmitJSON() serializes to indented JSON. Type mapping covers pflag's full type space, including slice/array→repeatable. Hidden commands and flags filtered. Cobra's auto-installed --help flag suppressed. Zero-default values suppressed to keep output compact. argRE parses positional arg placeholders from cobra Use strings, filtering [flags]/[options]/ [command] cobra conventions. parseExamples splits cmd.Example by newline, drops blanks and # comments. MaxDepth maps to spec §4 semantics: 0 = subcommand list, 1 = + grandchildren, -1 = unlimited. - cmd/pad/help_cmdhelp.go — replaces emitCmdhelpJSON stub with a call into the package; threads --depth and --all through MaxDepth (--all overrides --depth). Verification: - 17 emitter tests in internal/cmdhelp/json_test.go covering envelope, global-flag emission, hidden-thing exclusion, positional arg parsing, pflag type mapping, zero-default suppression, example parsing, description-vs-summary, command-path key shape, MaxDepth semantics, target-subtree scoping, JSON validity, version pattern, ExitCode union marshaling, parseExamples filtering. - 11 cmd/pad routing tests still pass; TestHelpCmd_FormatJSONStubError replaced by TestHelpCmd_FormatJSONEmits which validates the structure. - End-to-end: `pad help --format json` on the real binary emits 100 commands across the full tree; output validates against schema/cmdhelp.schema.json (verified with python jsonschema). - `pad help item --format json` correctly limits output to 29 commands in the item subtree (homepage and other top-level metadata still populated from root). - `pad help --format json --depth 0` correctly emits 15 immediate children of root, no grandchildren. - make check clean (lint + go test + web build). Out of scope (deferred): - Examples: pad's existing commands embed examples in Long rather than using cobra's Example field. The emitter correctly reads Example; TASK-939 will normalize the pad-side commands to populate it. - Dynamic enum injection (workspace-aware enums): TASK-936. - --capabilities discovery flag: TASK-937. - Schema-validation of live output in CI: TASK-938. Parent: PLAN-930. * fix(cmdhelp): handle alternation, variadic, and ValidArgs in Use parser per Codex review (round 1) Codex round 1 on PR #327 flagged that parseArgs missed two real cobra Use-string idioms in pad's command tree: 1. `completion [bash|zsh|fish|powershell]` — alternation in brackets. The old regex only allowed `[a-zA-Z0-9_./-]+` inside brackets, so the `|` made the whole token unmatched and the shell arg disappeared from the emitted JSON. Consumers asking "what does completion take?" got nothing. 2. `item bulk-update [--status X] <ref>...` — variadic ellipsis. Old regex didn't capture trailing `...`, so the arg was emitted but without `repeatable: true`. Consumers couldn't tell that <ref> may be passed multiple times. Fixes: - argRE now allows full-bracket content (`[^<>]+` / `[^\[\]]+`) and captures an optional trailing `...` group. - parseArgs takes *cobra.Command (not just Use string) so it can read cmd.ValidArgs and attach those values as the first arg's enum when set. This covers `Use: "completion [shell]"` + `ValidArgs: [...]` where the allowed values only live on the cobra struct. - Alternation `<a|b|c>` / `[a|b|c]` produces an enum-typed arg with the values as `Enum`. When Use carries no semantic name (only the alternation), the arg name is synthesized as "value". - New validArgName check rejects embedded flag-like fragments such as `[--status X]` and prose with whitespace/punctuation that the broader regex would otherwise capture from idiosyncratic Use strings. - ValidArgs entries strip cobra's tab-separated completion descriptions before becoming enum values. New tests: - TestBuild_VariadicArgsRepeatable — `<ref>...` → repeatable=true. - TestBuild_AlternationProducesEnum — `[bash|zsh|fish|powershell]` → enum with values in source order. - TestBuild_ValidArgsFillsEnumOnNamedArg — Use says `[shell]`, ValidArgs carries the values → enum-typed arg named `shell`. - TestBuild_EmbeddedFlagFragmentsFiltered — `[--status X]` does not leak as a positional arg. Verified on the real binary: - `pad help completion --format json` now emits the shell enum. - `pad help item bulk-update --format json` now marks <ref> repeatable. - `pad help --format json` still validates against the schema (100 cmds). - `make check` clean. |
||
|
xarmian
|
e2b19d393f |
feat(cli): wire pad help subcommand + scope levels (TASK-933) (#326)
Replaces cobra's built-in `help` with a custom subcommand implementing the cmdhelp v0.1 mandatory surface (https://getpad.dev/cmdhelp; IDEA-927): pad help [subcommand…] [--format <fmt>] [--depth <n>] [--all] Routing: - --format text (default): delegates to cobra's text renderer so the existing --help UX is byte-for-byte unchanged. - --format json|md|llm: routes to cmdhelp emitters. JSON and markdown emitters are stubs that return clear "not yet implemented" errors pointing to TASK-934 / TASK-935; this lets the routing layer ship and be tested independently of the emitter work. - --depth <n> and --all are accepted at this layer; their effect lives in the JSON/MD emitters. - llm is a renderer-level alias for md per spec §3. Scope levels (spec §4): - pad help → root tree summary (text mode delegates to cobra) - pad help <group> → subtree (e.g. `pad help item`) - pad help <group> <leaf> → single command, deep Tests (cmd/pad/help_cmdhelp_test.go): - 11 cases covering all routing branches: default text, group scope, leaf scope, --format text == default, --format json stub references TASK-934, --format md/llm stubs reference TASK-935, llm aliases md, unknown --format rejected, unknown topic rejected, --depth/--all accepted, CmdhelpVersion is MAJOR.MINOR. Verification: - All 11 new tests pass. - `make check` clean (lint + go test + web build). - Manual smoke-test on built binary: 13 paths exercised — group/leaf routing, format aliases, error paths, existing `pad <cmd> --help` unchanged. The JSON/MD emitters land in TASK-934/935. --capabilities discovery is a separate concern (TASK-937). Parent: PLAN-930. |
||
|
xarmian
|
5e27989ab8 |
feat(attachment): add pad attachment view|show|list CLI surfaces (IDEA-898) (#321)
* feat(attachment): add `pad attachment view|show|list` CLI surfaces (IDEA-898) Agents and CLI users had no first-class way to fetch attachment bytes through the API: the only path to read an `` reference was to read the raw blob out of `~/.pad/attachments/<storage_key>`, which bypasses workspace ACLs, doesn't work on Pad Cloud / remote / Postgres deployments, skips the variant pipeline (TASK-872 / TASK-879 / TASK-880), and breaks when storage moves to S3. Three new subcommands wrap the existing REST endpoints: - `pad attachment view <id> [-o path]` — agent-friendly: with no `-o`, fetches to a fresh OS temp directory using the stored filename and prints just the absolute path on stdout (so `$(pad attachment view <id>)` composes cleanly into shell pipelines). Reuses `download`'s atomic temp-then-rename pattern via a shared helper. - `pad attachment show <id>` — HEAD-based metadata only; surfaces MIME, size, filename, ETag, Last-Modified. - `pad attachment list [--item REF] [--category X] [--attached|--unattached] [--collection ID] [--sort ...] [--limit N] [--offset N]` — workspace list. The `--item REF` flag resolves a TASK-5-style ref to a UUID client-side and passes it to a new `item_id` query param on the list endpoint (server side: AttachmentListFilters.ItemID, ~6 lines in the store + 1 in the handler). Skill update: `skills/pad/SKILL.md` gains a "Working with attachments" subsection plus a CLI Reference entry, both ending in the hard rule that agents must NEVER read directly from `~/.pad/attachments/`. * style(cli): gofmt AttachmentListParams field alignment CI's golangci-lint v2 flagged this with the gofmt formatter (configured with simplify: true in .golangci.yml). The contiguous Sort/Limit/Offset block at the end of the struct needs uniform column alignment — gofmt considers the doc comment above Sort attached to that field rather than a block separator, so the three int/string fields get aligned together. Verified locally with `golangci-lint run --timeout=5m ./...` (v2.11.4 to match CI) — 0 issues. Local make lint only runs `go vet ./...` and `golangci-lint` wasn't installed, which is why this slipped through; filing a separate follow-up to mirror the CI checks in the local workflow. |
||
|
xarmian
|
10309fc599 |
fix(config): read PUBLIC_URL for emailed link generation (BUG-899) (#318)
* fix(config): read PUBLIC_URL for emailed link generation (BUG-899) The Pad Cloud deployment binds pad to 0.0.0.0 (Dockerfile, k8s configmap, pad-cloud's docker-compose) and never set PAD_URL on the pad service, so cfg.BaseURL() fell through to "http://0.0.0.0:7777" — that string ended up in password-reset (and invite + share-link + admin-invitation) emails and was unreachable to recipients. Adds a PUBLIC_URL env var read by the server only (does not flip CLI to remote mode the way PAD_URL does — PUBLIC_URL is a generic env var name commonly set in unrelated deployment contexts). Stored in a separate Config.PublicURL field consulted by BaseURL() as a fallback after URL. Resolution order in BaseURL(): PAD_URL > PUBLIC_URL > host:port. Also logs a WARN at server startup if the resolved base URL has an unspecified bind-all host (0.0.0.0, ::, [::]) — a backstop that would have caught BUG-899 the first time email went out. Tests cover the precedence ladder, mode-not-flipping, PAD_URL-beats- PUBLIC_URL, and the BUG-899 repro shape (Host=0.0.0.0 with no URL set yields the broken http://0.0.0.0 URL). Companion change in pad-cloud/docker-compose.yml passes PUBLIC_URL through to the pad service so the Cloud deployment stops shipping broken email links. Parent: BUG-899 (TASK-908). * fix(config): keep PUBLIC_URL out of IsConfigured() per Codex review (round 2) PUBLIC_URL was setting LoadedFromEnv = true, which IsConfigured() consults to decide whether the CLI has explicit configuration. A generic PUBLIC_URL in the environment (very common name) would have made any host appear "configured" to the CLI and skipped the not-configured / setup branch — the exact footgun the separate-field design was supposed to avoid. PUBLIC_URL is purely a server-side fact; LoadedFromEnv is purely a CLI affordance. Stop conflating them. Adds a focused regression test pinning the IsConfigured() invariant. * fix(config): split PublicLinkBaseURL from BaseURL per Codex review (round 3) Round 2's BaseURL fall-through to PublicURL leaked PUBLIC_URL into ~20 CLI-client call sites (cli.NewClientFromURL(cfg.BaseURL()) patterns across cmd/pad/main.go, init.go, server_info.go, configure.go) — same footgun the separate-field design was meant to avoid: a developer with a host-level PUBLIC_URL set for unrelated reasons would have their CLI silently route requests to that URL instead of the local server. Restore BaseURL() to its original CLI-only contract (URL > host:port). Add PublicLinkBaseURL() with the URL > PublicURL > host:port ladder that's used at exactly the two server-side call sites that build emailed-link targets: - cmd/pad/main.go:279 srv.SetBaseURL(cfg.PublicLinkBaseURL()) - cmd/pad/main.go:464 email.NewSender(..., cfg.PublicLinkBaseURL()) Tests pin both contracts: BaseURL() ignores PublicURL even when set; PublicLinkBaseURL() honors the precedence ladder. PAD_URL still wins in both, preserving back-compat. * fix(config): drop public_url toml tag to prevent CLI persistence per Codex review (round 4) Round 3 left PublicURL serializable to ~/.pad/config.toml via toml: "public_url". A CLI user who runs `pad init` or `pad configure` on a host where PUBLIC_URL is set for unrelated reasons would end up with that URL persisted into their config file, surviving any later unset of the env var and contaminating server-side emailed link generation indefinitely (server reads ~/.pad/config.toml on the next boot). Switch the field to toml:"-". PUBLIC_URL is a deployment-time fact (env var / docker-compose / k8s); operators who want a config-file equivalent already have `url` (the PAD_URL path), which serializes properly. Adds a regression test pinning that Save() never writes PublicURL to the file. |
||
|
xarmian
|
9c5f4d5165 |
fix(cli): construct auth login URL on CLI side to avoid 0.0.0.0 leak (TASK-839) (#311)
* fix(cli): construct auth login URL on CLI side to avoid 0.0.0.0 leak (TASK-839) The server builds the CLI auth-approval URL from r.Host, which echoes back whatever Host header the CLI sent. When the local pad server is bound to a bind-all address (e.g. --host 0.0.0.0), the CLI's own config points at that address, so the URL printed by `pad auth login` ends up as http://0.0.0.0:7777/auth/cli/{code} — a bind address, not a usable browser destination. Construct the URL on the CLI instead, using cfg.BrowserURL() (which already rewrites 0.0.0.0 / :: / empty to 127.0.0.1, and returns the explicit URL verbatim for Remote/Cloud). The server-issued auth_url field is now ignored; session_code is what we actually need and is already returned separately. Extracts a small cliAuthBrowserURL helper so the wiring is unit-testable and adds regression coverage for IPv4 bind-all, IPv6 bind-all, empty host, explicit loopback, explicit Remote URL, and trailing-slash trim. * chore(lint): remove unused readBundleAsBytes test helper golangci-lint v2.11.4 (CI) flags this as unused — it was added in the import-bundle test scaffolding (TASK-885 / TASK-891 era) but no caller ever picked it up. Removing it unblocks the lint gate on main. Reviewable in isolation; pure deletion, no behavior change. |
||
|
xarmian
|
2bb7ac35e4 |
feat(attachments): orphan GC sweep with periodic scheduler (TASK-886) (#307)
* feat(attachments): orphan GC sweep with periodic scheduler (TASK-886)
Background job that reclaims attachments past the grace period. Two
qualification criteria, both with a 30-day default grace:
- item_id IS NULL AND deleted_at IS NULL AND created_at < cutoff
(never-attached uploads — editor uploaded then tab-closed before
attaching to an item)
- deleted_at IS NOT NULL AND deleted_at < cutoff
(soft-deleted via the Settings → Storage delete button or the
DELETE /attachments/{id} endpoint)
Reclamation is dedupe-aware: content-addressed storage means the same
hash can be referenced by multiple rows, so the on-disk blob is only
removed when the GC'd row is the LAST live reference to its
content_hash. Otherwise the row drops and the blob stays for the
remaining references. CountLiveAttachmentsForHash is the predicate.
Per-row failures (resolve backend, blob delete, hard-delete) are
logged and skipped; the sweep keeps making progress. Catastrophic
errors (DB failure) return up to the loop, which logs and waits for
the next tick rather than crashing the server.
Lifecycle:
- SetOrphanGCConfig overrides the default 24h interval / 30-day
grace. cmd/pad reads PAD_ORPHAN_GC_INTERVAL / PAD_ORPHAN_GC_GRACE
(Go duration syntax — 1m, 24h, 720h) so operators can tune
without recompiling and tests can crank the interval down to 1ms
to see sweeps land in CI.
- StartOrphanGC kicks the loop. Idempotent — second call is a
no-op so a misconfigured caller can't double-spawn.
- Server.Stop() now signals the loop via stopOrphanGC() before
s.bg.Wait(), so process shutdown drains the goroutine cleanly
(BUG-842 invariant).
- Each tick wraps the sweep in a 30m context timeout so a slow
scan can't pin the goroutine across multiple intervals.
Tests:
- TestOrphanGC_ReclaimsSoftDeleted: upload → soft-delete → sweep
with future cutoff → DB row gone + blob gone from FSStore.
- TestOrphanGC_ReclaimsLongOrphans: upload → backdate created_at
31d → sweep with 30d grace cutoff → row reclaimed.
- TestOrphanGC_KeepsRecentRows: upload → soft-delete → sweep with
past cutoff → row stays. Catches a typo in the WHERE clause that
would silently destroy live attachments.
- TestOrphanGC_PreservesSharedBlob: two uploads with identical
bytes (same hash, same blob), soft-delete only one → sweep →
one row reclaimed BUT BlobsReclaimed=0 because the other row
still references the blob. Pin for content-addressed dedupe.
- TestOrphanGC_StartStop: loop spins up at 1ms interval, second
StartOrphanGC is a no-op, Stop drains via testServer's cleanup.
Parent: PLAN-866. Closes the phase 1 plan with full export →
import → orphan-cleanup round-trip.
* fix(attachments): protect referenced/in-flight blobs from orphan GC per Codex (round 1)
Two real correctness issues Codex caught on PR #307:
P1. The editor's normal upload flow leaves attachments.item_id NULL.
The canonical association lives in markdown content (the editor
PATCHes "pad-attachment:UUID" into the item) — but the GC's
"never-attached past 30d" predicate only checked item_id. So a
legitimate inline image could be hard-deleted 30 days after upload
even though item content still references it.
Added store.AttachmentReferencedInItems(workspaceID, attachmentID)
that scans items.content + items.fields for "pad-attachment:UUID".
The GC sweep now runs this check before reclaiming any
never-attached row; if any live item references the attachment,
the row is left alone (and re-checked next sweep).
P2. Race between concurrent upload and GC. Upload calls
AttachmentStore.Put (blob lands on disk) → THEN inserts the DB row.
Between those two steps an orphan-GC sweep could count zero live
refs for the hash, delete the blob, and the upload's row insert
would then point at a missing blob.
Added Server.inFlightUploadHashes (sync.Map of *atomic.Int64
counters) with markUploadInFlight / uploadInFlight helpers. Every
Put + CreateAttachment site fences itself via markUploadInFlight:
the upload handler, the transform handler, the thumbnail
derivation pipeline, and the bundle-import rehydrate path. The GC
sweep treats an in-flight hash as "another live ref" so it leaves
the blob alone.
Tests:
- TestOrphanGC_KeepsReferencedNeverAttachedRows: upload (item_id
NULL) → create item with pad-attachment: ref → backdate 31d →
sweep with 30d cutoff → row stays.
- TestOrphanGC_RespectsInFlightUploads: upload → soft-delete →
register an in-flight upload at the same hash → sweep → DB row
goes (it's tombstoned past grace) but blob stays so the
in-flight upload can complete cleanly.
The DB row still gets reclaimed in the in-flight case because the
soft-deleted row is independently past grace; only the blob delete
is fenced. That's correct: the blob remains usable for the
incoming upload and the new upload will register its own
attachments row.
* fix(attachments): mutex-protect in-flight tracker + portable JSONB scan per Codex (round 2)
Two fixes for the round-2 findings on PR #307:
P1. Same-hash race in the in-flight upload tracker. The sync.Map +
*atomic.Int64 design split increment from LoadOrStore-then-add and
release-decrement from delete, so a release could see "0" and start
deleting while another upload concurrently reloaded the same map
entry and incremented to "1" — the second upload's signal then
lived in a doomed map slot, invisible to subsequent uploadInFlight
calls.
Replaced with a plain map[string]int64 + sync.Mutex. Inc, dec,
delete-when-zero all run under one critical section, so any
inspection sees a consistent snapshot. Net cost is one mutex per
mark/release; uncontended this is ~10ns and the upload path is
already doing far more expensive work (Put + DB insert).
Stress test: 20 goroutines × 500 iterations of mark→check→release
on a shared hash. Every check must observe in-flight=true while
the calling goroutine holds the mark. Final state must be empty.
Runs cleanly under -race -count=3.
P2. Postgres JSONB compatibility. items.fields is TEXT on SQLite
but JSONB on PostgreSQL (per pgmigrations/001_initial.sql). LIKE
on JSONB fails with a type error, so the orphan GC's reference
scan would error on Postgres and skip every never-attached row —
breaking orphan reclamation for those rows entirely.
Cast fields::text in the Postgres dialect path:
fieldsExpr := "fields"
if s.dialect.Driver() == DriverPostgres {
fieldsExpr = "fields::text"
}
Same approach used elsewhere in the store for dialect-sensitive
text searches.
* fix(attachments): close GC/upload TOCTOU + protect in-grace peers per Codex (round 3)
P1 round 3: TOCTOU race between uploadInFlight check and store.Delete.
The mutex protected the in-flight counter but not the GC's
check-and-delete sequence. A new upload could call markUploadInFlight
between our check and our blob delete, then run Put after the blob
was gone — its CreateAttachment would insert a live row pointing at
the missing hash.
Fixed by holding inFlightHashesMu across the check + FS Delete:
s.inFlightHashesMu.Lock()
inFlight := s.inFlightHashes[hash] > 0
if !inFlight && others == 0 {
store.Delete(ctx, key)
}
s.inFlightHashesMu.Unlock()
A concurrent markUploadInFlight blocks until either we skip (because
we observed in-flight) or finish deleting. Lock window is ms-class
on FSStore; a per-hash lock can replace this server-wide mutex when
S3 lands in Phase 2.
P2 round 3: CountLiveAttachmentsForHash counted only live rows, so
GC could reclaim the blob from row A (soft-deleted 31d ago) even
when row B was also soft-deleted but only 1 day old — within
grace, so its blob must stay reachable until its own grace lapses.
Replaced with CountProtectingAttachmentsForHash which counts rows
where deleted_at IS NULL OR deleted_at >= graceCutoff. The blob is
preserved until every soft-deleted peer has aged past its own
grace window.
Tests:
- TestOrphanGC_RespectsSoftDeletedInGracePeer: two rows sharing a
hash, soft-delete both, backdate only one past 30d → sweep with
30d cutoff → older row reclaimed but blob stays for the still-in-
grace peer.
- existing TestOrphanGC_RespectsInFlightUploads still passes
(still uses the in-flight signal correctly).
* fix(attachments): dedupe blob-reclaim metric across same-hash peers per Codex (round 4)
Codex round 4 noted that when multiple soft-deleted peers share a
content_hash and all are past grace, the GC sweep would inflate
BlobsReclaimed and BytesReclaimed: AttachmentStore.Delete treats a
missing key as success, so the second peer's idempotent no-op
delete still bumped the counter.
Functional cleanup was correct (the blob really was gone after the
first peer); only the metric / log line was wrong, which makes
operator dashboards report fictitious bytes-reclaimed values.
Track per-sweep reclaimed hashes in a map and skip the Delete call
+ counter increment for repeats. The DB row still gets hard-deleted
on each peer.
Test: TestOrphanGC_DedupesBlobReclaimMetric uploads twice with
identical bytes (single shared blob), soft-deletes both, backdates
deleted_at past grace → sweep deletes 2 rows and reports
BlobsReclaimed=1 / BytesReclaimed=blobLen rather than 2 / 2*blobLen.
|
||
|
xarmian
|
134f55045d |
feat(attachments): import workspace bundle with rehydrate + UUID remap (TASK-885) (#306)
* feat(attachments): import workspace bundle with attachment rehydrate + UUID remap (TASK-885) POST /workspaces/import now accepts a tar.gz bundle (Content-Type: application/gzip) and rebuilds the workspace + attachments + items in one round trip. JSON imports still work — content-type dispatch in handleImportWorkspace routes the request. Three-phase flow: 1. Walk the tar, capture pad-export.json + manifest.json + every attachment blob into memory. 2. Run the existing ImportWorkspace path to create the workspace + collections + items + comments + links + versions. New IDs are generated; item.slug is preserved (the existing remap path doesn't re-slugify). 3. For each manifest entry, rehydrate the blob through the storage backend (re-validate MIME + re-hash defensively, don't trust the manifest), insert a fresh attachments row. Build an oldID→newID map keyed on attachment uuid. 4. Walk every imported item's content + fields, replace "pad-attachment:OLD" with "pad-attachment:NEW" in one transactional pass. Refresh FTS afterward (direct UPDATE bypasses triggers). Phase 2 errors per-attachment are logged and skipped — the workspace keeps importing rather than rolling back. The import handler returns the new workspace and the operator can inspect logs for any attachment that didn't make it. CLI: - pad workspace export now defaults to --bundle (.tar.gz) since pad import handles bundles. --json reverts to legacy items-only. - pad import auto-detects format by file extension (.tar.gz / .tgz → application/gzip). Other extensions go through the legacy JSON path. - New Client.PostRawWithContentType for explicit-content-type POSTs. Tests: - TestImportBundle_RoundTrip: upload → embed in markdown → export source → import to FRESH server → verify attachment list has 1 row with new UUID → item content rewritten to new UUID and old UUID is gone → download new blob matches original bytes. - TestImportBundle_LegacyJSONStillWorks: JSON content-type still hits the legacy path. - TestImportBundle_RejectsBadGzip: garbage gzip body returns 400. Parent: PLAN-866. With TASK-884 + TASK-885 merged, the round-trip acceptance criterion (export → import → images intact) is met. * fix(attachments): stream import end-to-end per Codex (round 1) Two memory regressions Codex caught on PR #306: P1 (server). importBundle was buffering every blob into a map[string][]byte during a first pass, then iterating the manifest on a second pass. A 2 GiB bundle full of 25 MiB attachments would pin ~2 GiB of heap. Reworked to single-pass streaming: pad-export.json → import workspace + build slug→id map attachments/manifest.json → index entries by tar path attachments/<uuid>.<ext> → look up entry, rehydrate now The export bundler always writes pad-export.json + manifest.json BEFORE any blob (deterministic order from handlers_export_bundle.go), so this works without buffering. Bundles that violate the ordering — a third-party tool that writes blobs first — return 400 with a clear error. Memory footprint now bounded by the largest single blob (≤25 MiB) regardless of bundle size. Stale blobs without a manifest entry are skipped (their bytes io.Copy'd to io.Discard so the tar reader stays in sync). Unknown top-level entries (forward-compat for future bundle additions) are also consumed and ignored rather than left dangling. P2 (CLI). pad import used os.ReadFile, buffering the entire bundle client-side before posting. Switched to os.Open + a new Client.PostStreamWithContentType helper that streams the body directly into the request — together with the server-side fix, import is end-to-end streaming. Tests: - TestImportBundle_RejectsOutOfOrderTar: hand-crafted bundle with a blob before pad-export.json returns 400 with "ordering" in the message. - existing TestImportBundle_RoundTrip / LegacyJSONStillWorks / RejectsBadGzip continue to pass under the new streaming flow. * fix(cli): give streaming endpoints a 1h timeout per Codex (round 2) Codex P1 round 2: PostStreamWithContentType + RawStream were both using the shared 10s-timeout httpClient. The default works fine for normal API calls but kills a multi-GiB bundle import or export over anything slower than a local network — Client.Timeout fires mid-stream with "Client.Timeout exceeded". Added a dedicated streamClient on Client with a 1h timeout, used by both RawStream (export bundle download) and PostStreamWithContentType (import bundle upload). 1h is generous enough for ~100 MB/s uplinks shipping a 350 GiB bundle and still caps a hung connection eventually. The 10s default stays in place for every other call — short timeouts are the right SLA for normal API requests and protect the CLI from hanging on a wedged server. * fix(attachments): make import bundle cap configurable per Codex (round 3) Codex P1: the 2 GiB import cap was hard-coded with a comment promising operator override "later" — but no setter existed, so workspaces over 2 GiB stream out fine on export and fail on re-import. Added Server.SetImportBundleMaxBytes wired from the PAD_IMPORT_BUNDLE_MAX_BYTES env var in cmd/pad/main.go. Mirrors the existing PAD_ATTACHMENT_MAX_BYTES pattern. Default stays at 2 GiB so the typical workspace works without configuration; operators with larger exports can raise it without recompiling. The per-blob cap (importBlobMaxBytes = 25 MiB) is intentionally kept constant — it bounds in-flight memory regardless of total bundle size, and a 25 MiB-per-blob ceiling matches the upload handler's default, so a bundle can never smuggle larger blobs than the upload endpoint accepts. * fix(attachments): scale per-blob import cap with PAD_ATTACHMENT_MAX_BYTES per Codex (round 4) Codex P1 round 4: importBlobMaxBytes was hard-coded at 25 MiB but the upload handler's per-file cap is configurable via PAD_ATTACHMENT_MAX_BYTES. An operator who raised the upload cap to allow 50 MiB attachments could export a workspace successfully (WorkspaceAttachmentsForExport doesn't gate on size) but the re-import would reject every blob over 25 MiB. Replaced the const with effectiveBlobMaxBytes() which reads s.attachmentMaxBytes (or falls back to defaultAttachmentMaxBytes). The pad-export.json cap also scales with this value (4×) so a content-heavy workspace doesn't trip its own JSON ceiling on a server with raised attachment limits. Error message on a too-large blob now points the operator at PAD_ATTACHMENT_MAX_BYTES so they know which knob to turn rather than digging through code to find the cap. * fix(attachments): independent metadata cap for bundle import per Codex (round 5) Codex P2 round 5: tying pad-export.json + manifest.json caps to PAD_ATTACHMENT_MAX_BYTES regressed deployments that LOWER the attachment cap. A 1 MiB attachment cap would force metadata to fit in 4 MiB / 1 MiB respectively — but metadata size scales with workspace item count, not attachment blob sizes, so a tight upload limit shouldn't gate it. Added importMetadataMaxBytes = 100 MiB constant for both metadata files. effectiveBlobMaxBytes() still drives the per-blob cap which genuinely tracks attachment-upload policy. |
||
|
xarmian
|
a0336e0248 |
feat(attachments): bundle attachments + manifest in workspace export (TASK-884) (#305)
* feat(attachments): bundle attachments + manifest in workspace export (TASK-884)
GET /workspaces/{ws}/export?format=tar streams a gzip'd tar bundle:
pad-export.json # the existing WorkspaceExport JSON
attachments/manifest.json # uuid → {filename, mime, size, hash, ...}
attachments/<uuid>.<ext> # original blobs only — no thumbnails
Default (no ?format) keeps returning JSON so existing automation
hitting the endpoint without a query param continues to work
unchanged. The CLI's pad workspace export now opts into the bundle
by default; pass --json for the legacy items-only output.
Implementation:
- store.WorkspaceAttachmentsForExport returns originals only
(parent_id IS NULL); thumbnails are re-derived on import via the
existing pipeline so shipping them would double the bundle size.
- handleExportWorkspaceBundle streams chunks straight into the
response writer rather than buffering — a workspace with multi-
GB of attachments would otherwise pin that much memory.
- AttachmentManifest is versioned (separate from WorkspaceExport
version) so the bundle layout can evolve independently.
- bundleAttachmentPath is exported (lowercase package fn) so the
import path in TASK-885 can resolve manifest entries to tar
entries without duplicating the filename logic.
- CLI gates against writing binary tar.gz to a TTY and appends the
conventional extension when -o is passed without one.
Tests:
- TestExportBundle_RoundTrip: two uploads → bundle contains
pad-export.json + manifest + 2 blobs whose bytes match the
uploads + manifest decodes cleanly + WorkspaceExport decodes.
- TestExportBundle_HidesThumbnails: synthetic thumbnail row, the
manifest excludes it.
- TestExportBundle_LegacyJSONStillWorks: no ?format param returns
application/json with a decodable WorkspaceExport (backward
compat regression guard).
Parent: PLAN-866. TASK-885 (import path + UUID remap) consumes the
manifest produced here.
* fix(attachments): stream export bundle + revert default to JSON per Codex (round 1)
Two findings from Codex on PR #305:
1. CLI buffered the entire response in memory via RawGet → io.ReadAll,
defeating the server-side streaming design and risking OOM on a
multi-GB bundle. Added Client.RawStream which copies the response
body straight into an io.Writer; export now opens the target file
and streams directly into it.
2. Default tar.gz output broke `pad export → pad import` round trip
because the import handler still only accepts JSON. Reverted the
CLI default to JSON; bundle is now opt-in via --bundle. The flag
docstring notes that TASK-885 will flip the default once import
handles bundles.
* fix(attachments): surface tar/gzip close errors and truncation per Codex (round 2)
Codex round 2 finding: deferred tw.Close() / gzw.Close() ignored
errors. If a backend returned fewer bytes than size_bytes claimed,
io.Copy returned nil, the tar writer's "missed N bytes" trip fired
at Close, and the handler still completed a 200 OK with a corrupt
bundle that gunzip would later refuse to decompress — silently from
the operator's perspective.
Two changes:
1. The deferred close now logs both tw.Close() and gzw.Close()
errors with structured context, so a corruption-on-finalize
trip shows up in the operator log.
2. streamAttachmentToTar checks the bytes-copied count against
a.SizeBytes after io.Copy and returns a per-attachment error
when they disagree. The error is logged with attachment_id +
storage_key so an operator can correlate the corruption with
the row to investigate.
Regression test: TestExportBundle_TruncatedBlobLogsError forces a
size_bytes/blob desync via direct UPDATE and asserts the resulting
bundle bytes don't decode cleanly. (HTTP status stays 200 because
headers are already on the wire by the time we detect the desync;
that's an inherent limitation of mid-stream errors, but the new
logs + close-error surfacing make the failure observable.)
* fix(attachments): X-Bundle-Status trailer for export-stream success per Codex (round 3)
Codex P1 round 3: even with the per-blob truncation log + tar/gzip
close-error logs, mid-stream failures looked successful to clients.
The CLI's RawStream finished without a transport error, the file
landed on disk, and "Exported workspace" printed regardless of
whether the bundle was actually complete.
Two complementary signals now mark a clean stream:
1. HTTP trailer X-Bundle-Status. The handler declares the trailer
in the initial Trailer header and sets it to "ok" only after
tw.Close() and gzw.Close() both return without error. CLI checks
the trailer after streaming and discards the file + returns
error if it's absent or non-"ok".
2. The handler skips the deferred clean close on the error path,
leaving the gzip footer unwritten. A client that ignores the
trailer (curl, third-party tooling) still sees a corrupt gzip
stream that gunzip refuses to decompress.
CLI: pad workspace export --bundle now removes any partial output
file on failure rather than leaving a corrupt one behind.
Client.RawStream signature changed to return (bytes, *http.Response,
error) so callers can inspect resp.Trailer; the only caller is the
export command.
Tests: TestExportBundle_TruncatedBlobAbortsStream now asserts both
signals (trailer absent + gzip/tar can't fully decode), and
TestExportBundle_SuccessTrailer pins the happy-path trailer.
|
||
|
xarmian
|
335762c2bf |
feat(attachments): storage usage API + effective-limit computation (TASK-881) (#302)
* feat(attachments): storage usage API + effective-limit computation (TASK-881)
Adds GET /api/v1/workspaces/{ws}/storage/usage returning
{used_bytes, limit_bytes, plan, override_active}. Resolves the effective
limit through the existing three-tier chain (per-user override → platform
setting → hardcoded plan default) and surfaces the override flag for the
upcoming Settings → Storage and admin user-detail UIs.
Implementation:
- store.WorkspaceStorageInfo consolidates SUM(size_bytes) + owner-plan
resolution in one call; WorkspaceStorageLimit is now a thin wrapper so
the upload-time quota check and the API path stay consistent.
- Server.storageInfoCache is a 30s TTL memoizer to absorb repeated
Settings → Storage page loads. Invalidation hooks fire on upload,
thumbnail derivation, and transform — the ~30s eventual-consistency
window is bounded by TTL only when invalidation isn't reachable.
- Defensive copy on cache read so a caller mutating the returned struct
can't poison subsequent reads.
- New CLI command `pad workspace storage` prints "X used of Y (Z%)" with
IEC units (humanBytes helper) and surfaces the override flag.
- TS api.attachments.storageUsage() + WorkspaceStorageInfo type ready
for TASK-882's Settings → Storage page consumer.
Tests:
- Store-level: no-owner fallback, free-plan resolution chain, override
flip, pro-plan override-active visibility, soft-delete exclusion.
- Server-level: empty-workspace happy path, two uploads with cache
invalidation between, dedicated cache TTL/invalidate/copy-safety test.
Parent: PLAN-866.
* fix(attachments): gate storage usage on viewer+ per Codex review (round 1)
Codex correctly flagged that the storage/usage handler relied solely on
RequireWorkspaceAccess, which admits item-grant guests with
workspaceRole=="guest". Workspace-wide quota numbers (used_bytes, plan,
override status) shouldn't surface to guests — every other workspace-
level read handler uses requireMinRole("viewer") for exactly this case.
Adds the explicit gate + a regression test that calls the handler with
a guest-role context and asserts 403.
|
||
|
xarmian
|
02be33902f |
feat(attachments): ImageProcessor interface + pure-Go impl + thumbnail pipeline (TASK-878) (#295)
* feat(attachments): ImageProcessor interface + pure-Go impl + thumbnail pipeline (TASK-878)
Adds the abstraction Phase 1 needs to derive thumbnail variants on
upload, with a pure-Go default implementation that keeps Pad's
single-binary distribution intact (no cgo). The libvips-tagged
build (Phase 2 / Pad Cloud Docker) will replace processor_purego.go
with a vips-backed implementation behind the same Processor
interface — see DOC-865.
internal/attachments/processor.go:
Processor interface — Decode(io.Reader)→(image.Image, format),
Resize(img, maxLong), Rotate(img, deg), Crop(img, rect),
Encode(img, format, w), Capabilities().
Capabilities struct (image_formats, can_transcode, max_pixels)
surfaces what the editor needs to gate per-format rotate/crop UI
on (TASK-879/880). ErrUnsupportedFormat + ErrImageTooLarge are
separate sentinels so callers can distinguish "format not
supported" from "image dimensions too big".
internal/attachments/processor_purego.go (//go:build !libvips):
Uses github.com/disintegration/imaging plus the stdlib decoders.
Supports PNG/JPEG/GIF/BMP/TIFF for all ops. WebP/AVIF/HEIC
reach Decode and bounce out via ErrUnsupportedFormat — uploads
still succeed (the MIME allowlist is the upload gate), but
thumbnails skip and the editor disables rotate/crop UI per
Capabilities.
Memory ceiling: Decode peeks via image.DecodeConfig (header only)
before allocating any pixel buffer and rejects images whose
width*height exceeds MaxPixelsDefault (8000² = 64MP). At 4 bytes
per pixel that caps the decode buffer at ~256 MiB and prevents an
attacker uploading a forged 100kx100k claim from OOMing the
server. The forged-CRC test exercises this gate.
internal/server/handlers_attachments_thumbnails.go:
deriveThumbnails(parentID) runs in goAsync after every image
upload. Generates thumb-sm (256px long edge) + thumb-md (1024px),
each as its own attachments row with parent_id pointing at the
original. Server.Stop() drains the goroutine before SQLite
closes, so tests can assert post-conditions deterministically.
Skip cases: parent deleted (race), source format not supported
(logged at debug), source already smaller than the variant's
bound, variant already exists (idempotent reruns). Variants
count toward workspace storage usage — DOC-865 is explicit about
this and TestThumbnails_CountsTowardWorkspaceUsage proves it.
Output format policy: PNG inputs stay PNG to preserve transparency;
everything else encodes as JPEG q=85.
internal/server/handlers_capabilities.go:
GET /api/v1/server/capabilities returns the Processor's static
capability profile under {image: {...}}. Public route — the
editor needs it before login (e.g. shared-item preview surfaces).
Reports an empty image-formats list when no processor is wired,
signalling the editor to disable rotate/crop UI rather than
500-ing the editor mount.
cmd/pad/main.go: wires SetImageProcessor(NewProcessor()) alongside
SetAttachments at startup; logs the supported formats so operators
know whether they're on the pure-Go or libvips build.
Tests:
- processor_test.go: 12 unit tests covering capability profile,
decode round-trip for PNG/JPEG/GIF, rejection of unsupported
formats and oversized images (forged-CRC PNG), resize aspect
preservation + pass-through for already-small inputs, rotate
multiples-of-90 + negative + 360-modulo handling, crop with
bounds clipping + empty-intersection rejection, encode round-
trip for PNG/JPEG, ThumbnailFormat/Mime/Ext policy.
- handlers_attachments_thumbnails_test.go: 5 integration tests
covering thumb-sm + thumb-md generation on PNG/JPEG uploads,
skip-when-source-already-small, ?variant=thumb-md serving via
the existing GET handler, workspace usage accounting.
- handlers_capabilities tests cover the happy path + the
no-processor degraded path.
Parent: PLAN-866. Closes the thumbnail-fallback gap that TASK-874 /
TASK-876 left open (thumb-md URLs were falling back to original
because no thumbnails existed). Unblocks TASK-879 (rotation tool)
and TASK-880 (crop tool) — both will reuse Processor.Rotate /
Processor.Crop with the same Capabilities-driven UI gating.
* fix(attachments): make /server/capabilities public per Codex review (round 1)
Codex flagged that GET /api/v1/server/capabilities was registered
inside the auth-gated API group but missing from isPublicAPIPath,
so once any user existed the editor's pre-login fetch would 401 —
contradicting the route's "public" register-time intent and breaking
the share-preview surface.
Fix: add the path to isPublicAPIPath. The handler is read-only,
returns a static profile, and has no per-user state, so making it
public has no security implication. Added
TestServerCapabilities_PublicAfterBootstrap as a regression guard:
it bootstraps an admin (so RequireAuth is active) and then fetches
the endpoint with no auth cookie, asserting 200.
* fix(attachments): make -tags libvips compile per Codex review (round 2)
Codex flagged that build tag !libvips on processor_purego.go meant
NewProcessor + the Thumbnail* helpers were absent under
\`go build -tags libvips\`, so cmd/pad/main.go and the thumbnail
handler — which call them unconditionally — broke that build.
Two minimal fixes preserving the documented Phase 2 split:
1. Move ThumbnailFormat / ThumbnailMime / ThumbnailExt out of the
tagged file and into processor.go (untagged). They're pure
format-name policy, not implementation specifics, so both
backends share the same definitions.
2. Add processor_libvips.go (//go:build libvips) with a stub
NewProcessor that panics at runtime with a clear
"Phase 2 hasn't shipped libvips yet" message. The libvips
build now compiles; anyone actually instantiating the
processor under that tag gets a loud failure rather than a
silent degradation. Phase 2 will replace the body with the
real govips-v2-backed implementation.
Verified: \`go build ./...\` and \`go build -tags libvips ./...\` both
clean. Existing tests still pass on the default tag.
* fix(attachments): make tests compile under -tags libvips per Codex review (round 3)
Codex flagged that running \`go test -tags libvips ./internal/attachments\`
or \`./internal/server\` panicked through the libvips NewProcessor
stub: processor_test.go and the thumbnail/capability server tests
all called NewProcessor() unconditionally, even though the libvips
build's stub is intentionally panicking until Phase 2 ships the
real implementation.
Three minimal fixes:
1. Tag processor_test.go !libvips. It tests the pure-Go
implementation specifically — there's no value in running it
under libvips, and the stub processor would explode the moment
NewProcessor() ran.
2. Tag handlers_attachments_thumbnails_test.go !libvips. Same
reasoning — these integration tests assert thumbnail
derivation against a working processor.
3. Split testServerWithAttachments's processor wiring into two
build-tagged helper files:
* testimageprocessor_purego_test.go (//go:build !libvips)
wires the real pure-Go processor.
* testimageprocessor_libvips_test.go (//go:build libvips)
is a no-op so the rest of the server test surface
(uploads, downloads, auth, etc.) compiles + runs cleanly
under -tags libvips.
Verification:
go build ./... — OK
go build -tags libvips ./... — OK
go test ./internal/attachments ./internal/server (default) — pass
go test -tags libvips ./internal/server -run "TestUpload|TestDownload" — pass
Phase 2 will introduce a real libvips test backend and drop the
!libvips tags on the thumbnail tests.
* fix(attachments): libvips binary boots cleanly per Codex review (round 4)
Codex flagged that the libvips build still crashed at \`pad serve\`
startup: cmd/pad/main.go calls attachments.NewProcessor()
unconditionally, and the libvips stub was panicking — so any
operator who built with -tags libvips today (Phase 2 isn't shipped
yet) lost the entire server, not just image processing.
Two minimal changes:
1. processor_libvips.go: stop panicking. Return nil + slog.Warn
instead. Every call site already nil-checks the processor (the
upload handler skips thumbnail derivation, the capabilities
endpoint reports a degraded empty formats list), so the
libvips-tagged binary now has the same runtime profile as a
self-host build that opted out of image processing entirely
— uploads succeed, originals display, only derived
transformations are unavailable. The slog.Warn keeps the
"this build doesn't have it yet" signal loud.
2. cmd/pad/main.go: skip srv.SetImageProcessor when NewProcessor
returns nil, and log a "not wired" message in that branch.
Distinguishes the wired vs. unwired states cleanly in the
boot log.
Phase 2 will replace processor_libvips.go's body with the real
govips-v2-backed implementation; main.go's wiring is already shape-
correct for that transition.
Verification:
go build ./... — OK
go build -tags libvips ./... — OK
go test ./... — pass (74s server tests included)
go test -tags libvips ./internal/server -run "TestUpload|TestDownload|TestServerCapabilities_Public" — pass
|
||
|
xarmian
|
fc1c47f124 |
feat(attachments): CLI + TypeScript clients + types (TASK-873) (#290)
* feat(attachments): CLI + TypeScript clients + types (TASK-873)
Rounds out the API surface with Go and TS client methods + a
\`pad attachment\` Cobra subcommand for ops debugging.
internal/cli/client.go
AttachmentUploadResult struct mirrors POST /attachments JSON.
UploadAttachment streams a multipart file part via io.Pipe — never
buffers the upload in memory. itemRef is optional. Uses a fresh
http.Client with a 5-minute timeout per request so a 25 MiB upload
over a constrained link doesn't trip the package-shared 10s default.
DownloadAttachment streams the bytes into the caller's writer,
returning Content-Type + total bytes copied. Optional ?variant=
parameter for thumbnails (server falls back to original silently
per TASK-872).
cmd/pad/main.go
pad attachment upload <item-ref|-> <path> [--filename NAME]
pad attachment download <id> <out|-> [--variant thumb-sm|thumb-md]
Item arg accepts an issue ref (TASK-5) or slug; "-" means no parent.
Out arg "-" streams to stdout (with status messages on stderr) so
callers can pipe into image viewers etc. Resolves the item via
GetItem first so a typo'd ref fails fast with a useful error.
List + delete subcommands intentionally omitted — those endpoints
ship with TASK-881 (storage usage) and the future GC task. Adding
client methods that hit 404s would mislead callers; same logic kept
the upload response's "url" out of TASK-871 until TASK-872 wired GET.
web/src/lib/types/index.ts
Attachment interface mirroring the Go model (pointer types → optional).
AttachmentUploadResult interface for the upload response shape.
web/src/lib/api/client.ts
api.attachments.upload(workspaceSlug, file, itemId?) — multipart
POST via direct fetch (skips shared request() because that helper
hard-codes Content-Type: application/json). Carries CSRF, cookies,
and the same 401 → /login redirect.
api.attachments.downloadUrl(workspaceSlug, attachmentId, variant?)
is a pure URL builder so callers can wire <img src> directly without
going through fetch.
End-to-end smoke verified:
pad attachment upload TASK-869 /tmp/tiny.png # uploads PNG
pad attachment download <id> /tmp/dl.png # bytes are identical
cmp /tmp/tiny.png /tmp/dl.png # PASS
Verification
go build ./... — clean
go vet ./... — clean
go test ./... — all packages pass
cd web && npm run build — clean
make install — server restarts on the new binary
Parent: PLAN-866.
* fix(cli): atomic download — write to temp + rename so a failed download doesn't truncate the destination per Codex review (round 1)
P2: pad attachment download <bad-id> /existing/file used to wipe the
existing file on auth/network/404 errors because os.Create truncated
before the request was even attempted. The bytes were never written
because DownloadAttachment errored out, but the destination was
already 0 bytes — a footgun for anyone running the CLI in scripts.
Fix: for the file-path case, write to a sibling .tmp via os.CreateTemp
in the destination directory, fsync, close, then os.Rename only on
success. Same atomic-write pattern as FSStore.Put. The defer cleans
up the .tmp on any error path.
The stdout case (outPath == "-") is unchanged — bytes already
streamed to stdout can't be rolled back, so any partial write is
just visible to the caller as a short payload.
Verified end-to-end:
echo X > /tmp/existing.png
pad attachment download not-a-real-id /tmp/existing.png # errors
cat /tmp/existing.png # still "X" — file untouched
* docs(cli): clarify os.Rename atomic-replace behavior on Windows (Codex round 2 disagreement)
Round 2 flagged this as P2: "os.Rename does not replace an existing
destination on Windows." That is technically incorrect for modern Go.
Verified directly against the Go stdlib source:
src/internal/syscall/windows/syscall_windows.go:
func Rename(oldpath, newpath string) error {
...
return MoveFileEx(from, to, MOVEFILE_REPLACE_EXISTING)
}
MoveFileEx with MOVEFILE_REPLACE_EXISTING atomically replaces an
existing destination on Windows. This has been the behavior since
Go 1.5 (2015), so every version of Go this codebase supports already
gets the desired replace-on-rename semantics on every platform.
Added an inline code comment so future readers don't worry about the
same false alarm. No code-path change.
|
||
|
xarmian
|
48b9e18d34 |
feat(attachments): upload API with MIME sniff + dedupe + quota tracking (TASK-871) (#288)
* feat(attachments): upload API with MIME sniff + dedupe + quota tracking (TASK-871)
Wires the upload endpoint that turns a multipart POST into an
attachments row plus a stored blob. Auth-gated (editor+), per-file
size cap, hash-streaming, MIME allowlist with extension blocklist,
fire-and-forget quota warning.
POST /api/v1/workspaces/{slug}/attachments
Multipart "file" field. Optional ?item_id=… or form item_id to
associate at upload time. Returns
{id, url, mime, size, width?, height?, filename, category, render_mode}.
Errors: 400 bad multipart, 400 empty file, 401 unauthorized, 403
insufficient role, 413 over per-file cap, 415 MIME or extension
rejection, 503 attachments not configured.
internal/attachments/mime.go
MIMEEntry + RenderMode + Category typed allowlist mirroring DOC-865.
Default-deny. SniffMIME wraps http.DetectContentType. ValidateUpload
cross-checks the sniff result against the filename extension and:
(a) rejects when the extension maps to a *blocked* MIME — covers
.svg (sniffs as text/xml; .svg ext makes the browser run embedded
<script>) and .exe family (sniffs vary; extension is unambiguous);
(b) rejects when the extension maps to an allowed MIME but the
sniff's category disagrees — the "exe pretending to be png" case.
Tests cover normalize/lookup/sniff plus happy path, exe-as-png,
extension mismatch, SVG, .exe-by-extension-alone, text/plain accept,
HTML force-download.
internal/store/attachments.go
CreateAttachment / GetAttachment / WorkspaceStorageUsage. Pointer
scan for nullables; SUM(size_bytes) excludes soft-deleted rows but
includes derived blobs (thumbnails are real bytes on disk).
internal/server/handlers_attachments.go
Body capped via http.MaxBytesReader BEFORE ParseMultipartForm spools
any of it. Streams "file" part into an os.CreateTemp file, sha256ing
in one io.MultiWriter pass — multi-GB POST never reaches RAM. Sniff
on first 512 bytes; image dimension probe via stdlib image.DecodeConfig
(PNG/JPEG/GIF). WebP/AVIF/HEIC accepted but width/height nil — matches
the "pure-Go gracefully degrades" decision in DOC-865. Calls
AttachmentStore.Put (which hash-verifies via the dedup fast path) and
inserts the row. Quota check (CheckLimit + WorkspaceStorageUsage) runs
in a goroutine — Phase 1 logs only; Phase 2 will enforce.
Anonymous uploads on a fresh install (RequireWorkspaceAccess grants
implicit owner without a current user) get uploaded_by="system".
internal/server/server.go
Server.attachments + attachmentMaxBytes fields and SetAttachments
setter. Route POST /workspaces/{slug}/attachments wired inside the
authenticated workspace block.
cmd/pad/main.go
Boot wiring: NewFSStore(<DataDir>/attachments) → Registry registered
under "fs" → SetAttachments. PAD_ATTACHMENT_MAX_BYTES env override
for the per-file cap.
Tests
internal/server/handlers_attachments_test.go covers:
happy path PNG (1x1, dimensions resolve to 1×1)
exe bytes with .png filename → 415
PNG bytes with .pdf filename → 415 (extension mismatch)
empty body → 400
missing file part → 400
over the size cap → 413
same content uploaded twice → two rows, same content_hash + storage_key,
WorkspaceStorageUsage = 2 × bytes (dedupe is at the blob layer,
not the row layer)
8 concurrent uploads of identical bytes → all 201, no corruption
no registry wired → 503
Verification
go build ./... — clean
go vet ./... — clean
go test ./... — all packages pass
make install — server restarts on the new binary
Parent: PLAN-866.
* fix(attachments): three Codex round-1 findings — drop premature url, accept Office docs, real quota probe
1. Upload response no longer returns "url". TASK-872 wires GET so any
URL we return today is a 404 — pulling it out keeps clients from
baking in the broken endpoint.
2. Office Open XML docs (.docx/.xlsx/.pptx) and OpenDocument formats
(.odt/.ods/.odp) are zipped XML — http.DetectContentType correctly
sniffs them as application/zip. Previously the validator's
extension-vs-sniff category check rejected them as
"mime_extension_mismatch" (archive vs document). Now: when the
sniffed type is exactly application/zip and the extension maps to
a document MIME, trust the extension and route to the document
entry. Plain .zip with the same bytes still routes to archive.
Test covers all six office/odf extensions plus the plain-zip case.
3. CheckLimit("storage_bytes") returned "unknown workspace feature"
because featureCount only knows row-counted features (items,
members, webhooks). The warning path silently dropped every probe.
Added Store.WorkspaceStorageLimit which does the same three-tier
resolution (user override → platform setting → hardcoded fallback)
but returns the limit only — usage is computed separately via the
existing WorkspaceStorageUsage. Self-hosted/pro plans return -1
(unlimited). Workspaces without an owner_id (fresh installs and
legacy rows) also return -1, so a fresh-install upload no longer
logs "owner not found". Switched maybeWarnStorageQuota to use
WorkspaceStorageLimit + WorkspaceStorageUsage directly. Now also
spawned via Server.goAsync so Stop() drains it (BUG-842 hygiene).
Tests
- TestValidateUpload_AcceptsOfficeOpenXMLAsZipBytes covers all six
extensions + plain .zip
- TestUpload_QuotaCheckResolves regression-tests finding 3: both
storage helpers return non-error after a real upload
- TestUpload_HappyPathPNG asserts the response no longer carries url
Verification
go build ./... — clean
go vet ./... — clean
go test ./... — all packages pass
* fix(attachments): trim trailing blank line at EOF in mime.go per Codex review (round 2)
Round 2 LOW: git diff --check flagged a "new blank line at EOF" on
internal/attachments/mime.go. Cosmetic but addressed because the
ship-tasks workflow requires zero findings (HIGH/MEDIUM/LOW alike) —
leaving LOWs unfixed compounds across PRs and prevents the loop from
ever converging clean on later work.
* fix(attachments): alias stdlib MIME-sniff quirks per Codex review (round 3)
http.DetectContentType returns names that don't match modern IANA
conventions for two formats on the allowlist:
audio/wave → audio/wav (.wav uploads)
application/x-gzip → application/gzip (.gz uploads)
Without aliasing, valid uploads of either format hit "mime_not_allowed"
because the allowlist uses canonical names. Added a sniffAliases map
applied inside SniffMIME so allowlist lookups always see the canonical
form. Allowlist stays single-sourced; the fix is one map entry per
quirk we discover.
Tests:
- TestSniffMIME_AliasesStdlibQuirks pins both aliases at the sniff layer
- TestValidateUpload_AcceptsWAV / TestValidateUpload_AcceptsGzip verify
the end-to-end accept path with real WAV (RIFF/WAVE) and gzip headers
|
||
|
xarmian
|
a03c96f9b0 |
feat(cli): pad init --url X --workspace <slug> as web-first cold-start (TASK-860) (#282)
Make `pad init --url <server> --workspace <slug>` a reliable non-interactive cold-start so the web UI can hand users a single copy-paste command to connect a workspace they created on the web to their local project. Keystone CLI work for the web-first onboarding on-ramp under PLAN-859 (driven by IDEA-750). Changes: - `ensureWorkspace` gains a `wsSlug` parameter. When set, it ONLY attaches by slug — looks up the workspace via GetWorkspace, links the CWD if found, and surfaces a clear "not found on <server>" error otherwise. Critically, it never silently falls through to creating a new workspace named after the slug. - Refuses to clobber a CWD that's already linked to a different workspace; idempotent re-run when the existing link matches. - `pad init --url X` on a fresh machine (no config.toml on disk) now persists the config so subsequent commands don't need --url. - When both a positional name and --workspace are supplied, the slug wins and we print a Note: line so the override is visible. - Same wiring applied to `pad workspace init` for consistency. Tests: 5 new unit tests in cmd/pad/init_test.go cover slug-attach, not-found error, clobber refusal, idempotent re-run, and that the legacy name-driven path still works. Smoke-tested end-to-end against the local server: happy path links, missing slug errors cleanly with no `.pad.toml` written, clobber blocked, idempotent re-run silent. |
||
|
xarmian
|
0fd5d0cdfb |
fix: green up Go (PostgreSQL) CI (BUG-842) (#275)
* fix(store): swap plainto_tsquery → websearch_to_tsquery for PG FTS (BUG-842)
`TestListItems_FTS_HyphenatedSearchTerm/task-five` has been failing on
every Go (PostgreSQL) CI run because `plainto_tsquery('english',
'task-five')` doesn't match the asciihword lexeme(s) the english parser
produces for an indexed `task-five-distinctive`. The result is that
every PG full-text search for hyphenated terms returns zero rows.
`websearch_to_tsquery` (Postgres 11+) is purpose-built for arbitrary
user input and tokenizes hyphenated terms the same way `to_tsvector`
does for the indexed document, so the query intersects the index
correctly. Swapped in three spots in the postgres dialect — FTSMatch,
FTSSnippet, FTSRank — and updated the caller-side comments that
referenced plainto_tsquery. SQLite path is unchanged: it goes through
items_fts MATCH with sanitizeFTSQuery, never through these methods.
* fix(server): drain background goroutines on Stop() (BUG-842)
`TestAdminBillingStats_SidecarSidecarError_DegradesToLocalOnly` (and
other server tests) have been flaking on the Go (PostgreSQL) CI runner
with `TempDir RemoveAll cleanup: directory not empty`. Root cause:
several request handlers spawned bare `go func() { ... }()` goroutines
that touched the SQLite WAL DB after the test function returned.
testServer's t.Cleanup closed the store but had no way to drain those
goroutines first, so a fire-and-forget WAL write could re-create the
`-wal`/`-shm` files between Close() and t.TempDir's RemoveAll.
Add a Server.bg sync.WaitGroup, a Server.goAsync helper that wraps a
WaitGroup-tracked goroutine, and a Server.Stop() that blocks until
every goAsync closure has finished. Convert the four known
fire-and-forget sites to goAsync:
- middleware_auth.go (TouchUserActivity)
- handlers_auth.go (password reset email)
- handlers_cloud.go (stripe_processed_events pruning)
- handlers_members.go (workspace invitation email)
Wire `srv.Stop()` into both testServer (server_test.go) and
newMetricsTestServer (metrics_auth_test.go) so cleanup order is
Stop → Close → TempDir RemoveAll. Add
TestServer_Stop_DrainsBackgroundGoroutines to pin the contract: a
goAsync goroutine must block Stop until it returns.
* fix(store): correct PG FTS hyphenation via OR-combined plainto_tsquery (BUG-842)
The previous attempt swapped plainto_tsquery → websearch_to_tsquery,
which was wrong: websearch_to_tsquery treats `-` as a NEGATION operator
(Google-style), so `task-five` becomes `task & !five` and the search
returns 0 rows for the same reason as before. This commit reverts the
swap and applies the actual fix.
PG's english parser indexes `task-five-distinctive` as
`{task-five-distinct, task, five, distinct}` — the asciihword AND its
parts. plainto_tsquery applied to the partial query `task-five`
produces `task-fiv & task & five`: the stemmed asciihword for the
PARTIAL query (`task-fiv`) is NOT in the vector, so the AND fails.
Replacing the hyphen with a space makes plainto emit `task & five`,
which DOES match — but doing that unconditionally breaks `BUG-842`-
style queries: PG indexes the `-842` suffix as a negative-number
lexeme, so `plainto_tsquery('BUG-842')` matches via `-842`, while
`plainto_tsquery('BUG 842')` searches for `842` and misses.
The fix ORs the two query variants together so the search vector is
matched against either the raw user query OR its hyphen-as-space form.
Both `task-five` (against `task-five-distinctive`) and `BUG-842`
(against `BUG-842 fix the cleanup race`) hit. Verified locally against
postgres:17-alpine via PAD_TEST_POSTGRES_URL — both 10x stress and
race-detector runs are green.
Surfaces:
- dialect.go: FTSMatch / FTSSnippet / FTSRank now consume TWO
placeholders each in the PG dialect.
- items.go: listItemsFTS PG branch + SearchItems PG branch update
args to pass (raw, sanitized) for every PG `?` placeholder.
- search.go: SearchItems main / count / facets PG branches updated
likewise. New sanitizePGFTSQuery helper alongside sanitizeFTSQuery.
- documents.go: ListDocuments PG branch updated.
Tests:
- TestListItems_FTS_HyphenatedSearchTerm extended with a `BUG-842`
case to pin the OR-combined logic — naive hyphen-stripping would
silently regress this.
- New TestSanitizePGFTSQuery unit test.
* chore: gofmt 11 files with import-order issues (BUG-842 PR cleanup)
The Go (SQLite) CI job has been failing on `main` (and every PR built
against it) because golangci-lint flags 11 files whose third-party
imports are intermixed with internal imports — the import-grouping
rule that gofmt enforces. None of these were introduced by the
BUG-842 PR; they're pre-existing on main. The PR can't go green
without this cleanup, though, so it's bundled here.
Pure mechanical change — `gofmt -w <files>` only re-orders import
groups; no logic changes. Files touched:
cmd/pad/configure.go
cmd/pad/main.go
internal/cli/format.go
internal/server/handlers_admin_invitations.go
internal/server/handlers_admin_users.go
internal/server/handlers_grants.go
internal/server/handlers_share_links.go
internal/server/handlers_stars.go
internal/server/middleware_auth.go
internal/store/store.go
internal/store/store_test.go
After this commit `gofmt -l ./cmd ./internal` returns clean.
|
||
|
xarmian
|
7cda0d7896 |
feat: rebrand to Perpetual Software + new tagline (IDEA-832) (#273)
Migrates from xarmian/pad to PerpetualSoftware/pad across the entire
repo and updates the product subtitle to "Collaborate with your AI
agents".
Go module rename
- go.mod: github.com/xarmian/pad → github.com/PerpetualSoftware/pad
- All Go imports updated across cmd/pad, internal/{cli,server,store,
models,collections,items,events,metrics,webhooks} (~130 files)
- Test fixtures with the literal repo slug ("xarmian/pad" in JSON
shapes, SSH/HTTPS git URL strings, workspace_context fixtures)
also updated, including the secondary repo entry
(xarmian/pad-web → PerpetualSoftware/pad-web — pad-web was also
moved to the org per branch context)
Docs / config
- README badges, install instructions, brew tap, Docker image, source
build path, sponsor link (sponsor link kept as personal @xarmian)
- Subtitle: "Project management for developers and AI agents." →
"Collaborate with your AI agents." (README, manifests, web layout
meta, .goreleaser homebrew description)
- CONTRIBUTING.md, SECURITY.md, skills/INSTALL.md
- .goreleaser.yaml: homebrew_casks owner, GHCR image, release github
owner, cosign cert-identity regex, comments
- .github/workflows/release.yml: tap/release comments
- deploy/k8s/deployment.yaml: container image
- docs/deployment.md: clone URL
- web/static/{site.webmanifest,manifest.json}: description
- web/src/routes/+layout.svelte: meta description + og:description
Brew tap path is PerpetualSoftware/tap/pad (CamelCase, matches
GitHub user case). GHCR image is ghcr.io/perpetualsoftware/pad
(lowercased per GHCR's URL normalization). CODEOWNERS @xarmian and
FUNDING.yml github: xarmian intentionally retained — those are the
personal maintainer / sponsor account, separate from the org repo.
Verification: go build ./..., go test ./... (all pkgs pass), web
build, and make install all clean (TASK-844, TASK-845).
|
||
|
xarmian
|
afe721d202 |
feat(cli): add Cloud mode to pad init, drop Docker option (TASK-837, TASK-838) (#272)
Merging despite Go (PostgreSQL) red — those failures (TestListItems_FTS_HyphenatedSearchTerm/task-five + TestAdminBillingStats_SidecarSidecarError_DegradesToLocalOnly TempDir cleanup race) are pre-existing on main and tracked in BUG-842. Codex reviewed in 3 rounds (round 1 clean → round 2 found a real semantic bug → fix → round 3 clean). Tests, vet, and lint all green; remaining check failures are documented pre-existing. |
||
|
xarmian
|
afd3b3c5ee |
feat(cli): explicit cancel + clean SIGINT for pad init prompts (TASK-835) (#270)
* feat(cli): explicit cancel + clean SIGINT for pad init prompts (TASK-835) The interactive prompts in 'pad init' / 'pad workspace init' previously relied on Go's default Ctrl+C behavior (terminate with no message) and offered no in-prompt way to back out. A user who realized mid-init that they were in the wrong directory had no clean exit and risked partial state. This change: - Adds cmd/pad/cancel.go with errCancelled (sentinel), cancelInit() (the canonical "Cancelled." + os.Exit(130) path), and an installable SIGINT/SIGTERM handler. - Template picker accepts c/q/cancel/quit (case-insensitive) and returns errCancelled. Prompt text now mentions the cancel option. - Mode picker (pad configure) gains the same cancel keywords + prompt hint. - Both pad init and pad workspace init RunEs install the signal handler and convert any propagated errCancelled into the same cancelInit() exit, using a named-return + LIFO defer so the existing body is unchanged. - SilenceErrors + SilenceUsage are set on both init commands so cobra doesn't render an "Error: cancelled by user" line on top of our friendly message. State on cancel: the template picker is invoked AFTER step 1 (configure) but BEFORE the workspace is created on the server and BEFORE .pad.toml is written, so an abort at that prompt leaves no half-created workspace, no orphan .pad.toml, and no stale credentials. Tests cover all cancel keyword variants (both via the picker and via errors.Is on wrapped errors), and verify the prompt surface mentions the cancel option. Parent: PLAN-833. Source: IDEA-831 issue #4. * fix(cli): wire cancellation into all init paths per Codex review Round 1 findings: - HIGH: getConfiguredConfig() called os.Exit(1) on errCancelled, bypassing the canonical "Cancelled." + 130 exit. It now recognizes the sentinel and routes through cancelInit() before falling through to its generic Error path. - HIGH: SilenceErrors+SilenceUsage on the init commands silenced every error, hiding real failures (e.g. server connection problems). Removed both — cancelInit() never returns to cobra, so the cancellation case doesn't need silencing, and real errors print normally again. - MEDIUM: doBrowserLogin returned `fmt.Errorf("login cancelled")` on context cancel, which is not errCancelled. If its inner signal listener won the race against the outer init handler on Ctrl+C, the propagated error didn't match isCancellation and the command exited 1 with a generic message. doBrowserLogin now returns errCancelled directly so whichever goroutine wins the race, the exit converges on 130. - MEDIUM: cancel.go cleanup race — if a signal arrived between init completion and the goroutine returning, both sigCh and done could be ready and select could pick sigCh, turning a successful run into a spurious 130 exit. Added a re-check on done inside the sigCh branch so late signals are suppressed once cleanup has run. Also reordered cleanup to call signal.Stop before close(done) so no new signals enter the buffer during shutdown. - MEDIUM: promptForValue (the URL prompt for remote/docker mode) still treated 'c' as URL input and failed validation. It now recognizes c/q/cancel/quit and returns errCancelled, matching the picker and mode-prompt behavior. LOW finding (account-setup prompts) intentionally not addressed: Ctrl+C already covers them via the outer handler, and explicit keyword recognition on the password prompt would risk collision with real passwords. doInteractiveLogin is not on an init path. Parent: PLAN-833. * fix(cli): cancel sentinel handling for pad auth configure / pad auth login Codex round-2 findings: - MEDIUM: pad auth configure RunE returned errCancelled directly to cobra. Now wraps the body with the same isCancellation -> cancelInit() deferred check used in pad init, so 'c' at the mode/URL prompt exits with the canonical "Cancelled." + 130. - LOW: pad auth login RunE called doBrowserLogin (which now returns errCancelled on signal cancellation). The sentinel was leaking to cobra. Added the same deferred check so SIGINT during browser login exits 130 with a friendly message regardless of which goroutine wins the cancellation race. Neither command installs the outer SIGINT handler — pad auth login relies on doBrowserLogin's existing inner listener (avoiding the double-listener race) and pad auth configure's prompts are short enough that Go's default Ctrl+C handling for those is acceptable. The new deferred checks just plug the sentinel-leak holes. Parent: PLAN-833. |
||
|
xarmian
|
189b22825e |
fix(cli): use cfg.BaseURL()/BrowserURL() in pad init success message + pad open (TASK-834) (#269)
* fix(cli): use cfg.BaseURL() in pad init success message (TASK-834) The "Or open the web UI at http://localhost:7777" line in printOnboardingHints was hardcoded, which is wrong for any non-local connection mode (Remote, Docker, eventual Cloud). The CLI already knows the configured base URL — it just used it to talk to the server. Same hardcoded URL existed in the workspace-onboard skip path ("You can activate conventions from the library: ..."). Both call sites now use cfg.BaseURL(), which yields the correct URL for every mode: - Local: http://127.0.0.1:7777 (default host:port) - Remote/Docker/Cloud: the configured URL (e.g. https://app.getpad.dev) printOnboardingHints now takes a *config.Config; both call sites already had cfg in scope. Parent: PLAN-833 (pad init UX gaps + Pad Cloud onboarding fixes). Source: IDEA-831 issue #5. * fix(config): add BrowserURL() that normalizes 0.0.0.0 to 127.0.0.1 Per Codex review (round 1): when local mode runs with --host 0.0.0.0 (bind-all), cfg.BaseURL() returned "http://0.0.0.0:7777" — a bind address that browsers don't reliably accept. BrowserURL() behaves like BaseURL() except that when constructing from host:port, an unspecified bind-all host (empty, "0.0.0.0", "::", "[::]") is rewritten to "127.0.0.1". Explicit URL configurations (Remote/Docker/Cloud) are returned unchanged. The two onboarding-hint call sites updated in the previous commit now use BrowserURL() so the success message and skip-path show a clickable URL in every supported configuration. Tests cover loopback, named hosts, empty/0.0.0.0/::/[::] normalization, and explicit-URL precedence. Parent: PLAN-833. * fix(cli): use BrowserURL() in pad open for bind-all safety Per Codex review (round 2): the 'pad open' command prints and opens cfg.BaseURL(), which produces 'http://0.0.0.0:7777' when the local server is bound bind-all. Same class of bug as the onboarding hint fix in this PR — switch to cfg.BrowserURL() so the URL is a usable browser destination. A second related issue Codex flagged — the server-issued CLI auth URL in doBrowserLogin (which goes through internal/server/handlers_cli_auth.go using r.Host) — is a different surface with multiple possible fix strategies and overlaps with the post-v0.1.0 OAuth-architecture work. Deferred to TASK-839 with a written-up runbook so it isn't lost. Parent: PLAN-833. |