mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-24 19:32:10 +00:00
7e20e297542705acdeebf3df85effaae80fe211d
51 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
ea139272ce |
fix(server,watchevents): shared session presence + honest push acceptance (BUG-2698, BUG-2699) (#1175)
Two coupled defects in the push path, fixed as one unit because 2699's honest-acceptance signature is the substrate 2698's fix reports through. BUG-2699 — Bus.Publish reports acceptance. The endpoint returned 200 pushed:true for a publish that was dropped, because Publish returned nothing and swallowed every failure. An error is two outcomes and they are kept apart: ErrBusClosed proves nothing was published (503 unavailable, safe to resend), while any other error means UNCONFIRMED — go-redis retries a command whose reply was lost, which is why the publish script already carries a dedupe token — and gets 502 push_unconfirmed, deliberately off the web client's safe-to-resend list. MemoryBus was the worse case, not the exempt one: neither implementation checked `closed`, and the in-process one dropped silently with no log at all. Seven production call sites, not the six the item named; the six best-effort producers discard through one named helper, and an AST-based test fails when a new producer publishes directly. BUG-2698 — RedisSessionPresence. A session-targeted push was resolved against the answering replica's presence registry, and the handler skips the publish when the target is absent, so a POST landing on A for a session held on B dropped the instruction and answered delivered_sessions:0. Fixed at the REGISTRY rather than the gate: a shared registry makes the snapshot right, which makes the picker complete and restores the gate's original premise, so the existing skip becomes correct for the reason it was written. Entry and index are written atomically under a TTL renewed by a goroutine that lives exactly as long as the connection; a crashed process stops renewing and Redis clears it. Staleness is unchanged and now stated in full: ~30s for a dropped client, ~90s for a dead instance. delivered_sessions becomes nullable — null means published-but-uncountable, never zero — documented as three states at every consumer. 35 Codex review rounds. Notable: a per-user registry cap was added and then removed after three consecutive rounds found defects inside it and a fourth was asked whether it belonged in this PR at all; a context bound was documented, disproved by its own test (go-redis does not apply a command context to connection establishment — 5.0s measured against a 150ms ctx), and rewritten to say what is true. Every fix was mutation-checked; one instrument was deleted for passing on broken code and one for not asserting its own premise. Filed rather than folded in: BUG-2724 (Redis keyspace namespacing + Cluster), BUG-2725 (delivered_sessions is an estimate with error in both directions), BUG-2726 (no concurrent-connection limit on the watch stream), BUG-2727 (Redis absent from readiness/metrics; silent subscriber loss), BUG-2728 (epoch-reset resume lead). Gates: build · make lint 0 issues · go test ./... (25 pkgs) · svelte-check 0 errors · vitest 1738 passed · CI 7/7 including Go (PostgreSQL) and Nix. |
||
|
David Barkhausen
|
5784d907c0 |
feat(cli): PAD_TOKEN environment override for stored credentials (#879) (#1160)
* feat(cli): PAD_TOKEN environment override for stored credentials (#879) Layer 1 of #879: if PAD_TOKEN is set, the CLI uses it as the bearer token and skips the credential-store lookup — gh's GH_TOKEN convention. Reads never write credentials.json, so a read-only override sidesteps the multi-agent identity contention completely; the store is never touched under the override. Per the acceptance grounding notes: - NewClientFromURL resolves PAD_TOKEN before the per-server store lookup (the single token-attachment chokepoint). - whoami no longer lies under the override: it skips the store short-circuit and reports the effective identity via a real /me fetch, with an 'Auth: PAD_TOKEN environment override' line. - auth login/logout print a gh-style stderr notice when the override is active. logout additionally pins its server-side session invalidation to the STORED token — an unpinned Logout() after the constructor change would have invalidated the env token's session — and skips the server call when there is no stored session. - pad init's status line and server info's report disclose the override (env_token_override field; the auth probe uses the token every other command would use). Zero behaviour change when PAD_TOKEN is unset. Token minting stays web-only; a minimal 'pad token' CLI is offered as a follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(cli): review round 1 — init fails on a rejected PAD_TOKEN; login shortcut skipped under the override; logout asymmetry documented Per the PR #1160 round-1 review: - Bug 1: pad init's auth step no longer falls back to stored credentials when a set PAD_TOKEN is rejected — it fails with the distinct rejected-token message (mirroring whoami), which also makes the status line's override disclosure truthful. Test drives the real padInitCmd flow and asserts the stored identity is never consulted. - Bug 2: login's 'Already logged in as <stored user>' shortcut is skipped when the override is active — it reads the store, and firing it right after envTokenNotice contradicted the notice. A second test pins the unchanged no-override shortcut behaviour. - Doc ask: the deliberate logout asymmetry (the env token's own session is never invalidated; its lifecycle belongs to the minter, GH_TOKEN posture) is now stated in env_token.go's doc comment and the README PAD_TOKEN section. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> |
||
|
xarmian
|
449ac109e9 |
fix(server,mcp,cli): refuse system-metadata keys in fields_patch + retry-hostile error code (BUG-2627 part 2, BUG-2675) (#1166)
* fix(server,mcp,cli): refuse system-metadata keys in fields_patch + retry-hostile error code (BUG-2627 part 2, BUG-2675) Part 2 of BUG-2627 closes the door that mints the defect parts 1 and 3 dealt with: `--field implementation_notes=<json>` stored the entries as a JSON-ENCODED STRING, which is invisible to every reader and — since part 3's guard — disables `pad item note` on that item until the row is repaired. Refused SERVER-SIDE in `fields_patch`, not at the CLI as the item's scope line proposed. The deviation is deliberate and recorded on the trail: the CLI is one of three clients, and all three lower a user field-setter into the same key (`pad item update --field` at cmd_item.go, the MCP `field` param via dispatch_http_advanced.go on remote, and stdio by shelling out to that CLI). One gate closes all three; a CLI-only refusal would have left remote MCP writing the key. Both call sites were read, and the CLI's lowering is now pinned by a test rather than left as an assumption. Scope, stated because it is deliberate: this closes UPDATE only. The full `fields` blob stays open because that door is SHARED — `pad item note` / `decide` / `github link` send one, and so does convention activation via BuildConventionItemFields -> ItemCreate. Closing it would break the system writers the gate exists to protect. Item create therefore remains a mint site, tracked with the rest of that surface in BUG-2685. The refusal message is per-key: implementation_notes -> `pad item note`, decision_log -> `pad item decide`, github_pr -> the GitHub link flow, and `convention` refuses WITHOUT naming a command, because none writes it. PATTE-135 wants a remedy that works in the failing state; a single "use pad item note" line would have been wrong for three of the four keys. BUG-2675 rides along on one ToolSurfaceVersion bump, as ruled. The append refusal from part 3 reached MCP agents as `server_error` — not our fault, and not transient, so agents could reasonably retry a failure that is deterministic forever. New closed-set code `stored_state_unreadable`, emitted on BOTH transports: HTTP classifies the sentinel error directly, stdio via a `pad-structured-error/v1:` marker the CLI now writes for its own local refusal (the first marker generated without an upstream APIError). v0.16-then-v0.17 is what a one-transport fix costs. Also here: - items.ReservedOverrideKeys -> ReservedFieldKeysIn. The second caller passes a patch, not an override map, and the old doc comment said fields_patch was an open exposure — true until this commit. - `Extract* returns nil for THREE reasons` -> FOUR. The comment listed four; the count was corrected everywhere except the code. - Consumer-read artifacts updated where the claim is ACTED on, not only where it is documented: instructions.md (incl. a "do not retry this code" section), the catalog `field` param description, `pad item update --help`, README. Gates: build · make lint · go test ./... · make test-pg · Codex. Eleven-mutation matrix run against the new tests; every one killed by an assertion (two were rewritten after killing by compile error / surviving, which proves nothing). Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(server,mcp): honest remedy when the stored value is already unreadable; name the MCP-facing code (Codex round 1) Three findings from the pre-push review, all real: P2 — the refusal named `pad item note` unconditionally, but on an item whose stored value is ALREADY undecodable that command refuses too (part 3's guard). The caller was routed in a circle: field write refused -> run the note -> refused -> back again. That is exactly the failure PATTE-135 exists to prevent, and my own trail had reasoned the remedy was safe on the strength of the HEALTHY case only. The message now inspects the item's stored value and, when the key is unparseable, says so and points at the one action that works in that state (inspection), noting that the repair needs a full `fields` write no CLI flag exposes. P2 — two doc claims were false where an actor reads them. The catalog said reserved keys are refused "on every action that accepts field", which includes CREATE, and create is deliberately NOT gated; and both the catalog and instructions.md named `validation_error` (the HTTP code) where an MCP client actually receives `validation_failed`. Both corrected, and the create exception is now stated rather than implied by omission — an agent that reads only "refused on update" will otherwise assume create is fine, which is how a hole gets used. nit — the destructive-downstream sentence claimed every reserved key becomes unreadable and trips an append guard. True only for the two append-backed keys; github_pr and convention are simply overwritten. The clause is now per-key, because a confident wrong explanation is worse than a vague right one. Two more mutations run against the new branch: always-readable (the circular remedy returns) and never-readable (the working remedy disappears) — both killed by assertions. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(models,mcp,cli): one appendability predicate, per-key docs, stdio hint parity (Codex round 2) Five findings, all real. P2 — the message's readability check and the guard it describes were two different decodes. Mine unmarshalled into []json.RawMessage; the guard uses []ItemImplementationNote. A stored `[1]` passed mine and fails the guard, so the message would again have prescribed a command that refuses — the same circularity round 1 caught, through a narrower door. Replaced with models.StructuredFieldIsAppendable, which ASKS the guard rather than re-deriving it, plus an agreement test over 12 shapes x 2 keys that compares the predicate against the real Append* helpers. Verified by restoring the RawMessage version: the table catches it on `[1]`. P2 — stdio lost the new code's hint. Remote MCP told the agent retrying is pointless and how to inspect; stdio got the code with an empty hint, because the CLI's marker envelope carried none and the classifier parsed none. Both fixed, with the hint hoisted into paired constants (the same duplication StructuredErrorMarker already uses) and the test comparing the two TRANSPORTS' envelopes rather than either against a literal. P2 — doc text was still false for `convention`: the catalog, the instructions and `--help` all said reserved keys are maintained by note/decide/the GitHub flow, which is true of three of the four. Each key now names its own writer, and `convention` names library activation. Also dropped the `malformed_override` advertisement — that is the SERVER's code; an MCP client sees validation_failed for both refusals. nit — the classification test called structuredAppendErrorResult directly, so deleting either dispatcher call site left it green. Added dispatcher-level tests driving the real server + store, asserting the code, the hint, and that the item's stored fields are byte-identical afterwards. Mutation-verified by reverting the note call site. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(items,models,mcp): github_pr stays writable through fields_patch; no nil-map panic (Codex round 3) P1 — the gate refused `github_pr`, and that was wrong. My model was "system writers use the full fields blob, user setters use fields_patch", which holds for three of the four reserved keys and fails for this one: `pad github link` needs a local git checkout and the `gh` CLI, so it is excluded from remote MCP BY NAME, and internal/mcp/dispatch_http.go's noRemoteEquivalent map tells remote agents in so many words to use `item update --field github_pr=...` instead. For that audience the patch door is not a bypass of the writer — it IS the writer. So the refusal deleted a documented capability from remote agents, and answered with a message naming a command they cannot run: the same circular remedy round 1 caught, aimed this time at the people the gate was meant to help. items.PatchRefusedFieldKeysIn now exempts the key and records the rule being applied — refuse a raw write where a real writer exists — rather than the list it produces. Whether remote agents should get a proper PR-link action, so the key can be closed too, is a product question and is left as one. P2 — the hint told agents to read the bad value with `pad_item action=get`. They cannot: stripDuplicatedFieldsKeys removes implementation_notes and decision_log from every MCP response's fields blob, and the top-level arrays come from the extractor, which returns nil for exactly this shape. The value is invisible on the whole surface. The hint now says so and routes to a human, who can read it with `pad item show --format json`. P2 — `fields` holding a literal `null` unmarshals into a NIL map with no error, and both Append* helpers assign into what they get back, so `pad item note` PANICKED ("assignment to entry in nil map") instead of appending. Reproduced, fixed in parseMutableItemFields, and pinned by a test that fails on a panic rather than taking the process down. An absent blob and a null blob mean the same thing to every caller. Pre-existing, but it sits in the function family this bug is about and the message was about to recommend the command that panics. nit — README claimed a "closed eight-code taxonomy" (17 codes, and I had just added one) and read as if create lowers into fields_patch. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(models,mcp): predicate matches the append on malformed blobs; stop promising a broken workaround (Codex round 4) P1 — round 3 exempted `github_pr` from the update gate on the strength of noRemoteEquivalent's documented workaround. That workaround does not work: ingestFieldKVP (remote) and parseFieldFlag (CLI, and so stdio) both store a `field` value as a STRING, so the PR data lands double-encoded and no link appears — the BUG-2627 shape one key over. Filed as BUG-2696 with the three candidate fixes; NOT folded in, because the narrowest of them changes how every field value is typed. The exemption stands regardless: refusing would leave remote agents with strictly less than a broken door. What changes is what we may PROMISE. The catalog, instructions.md, version.go and README said "this is how you link a PR"; they now say the door is open and broken, and to hand PR linking to a human. Advertising a capability that isn't there is the failure mode this whole unit keeps circling. P2 — StructuredFieldIsAppendable returned TRUE when the whole fields blob was unparseable, on the reasoning that a broken outer blob is a different problem. True of the cause, irrelevant to the caller: the Append* helpers bail on that same parse, so the message again named a command that fails. It now returns false, which is simply the honest answer to the question asked, and the agreement table grew a malformed-outer-blob leg — the gap that let the disagreement through. P2 — the message claimed a raw field write always stores something Pad cannot read back. That holds for the CLI and MCP (a `--field` value is typed by schema lookup and these keys are in no schema) but not for a direct REST caller sending a valid array, who is refused for ownership reasons alone. Reworded to say both parts. nit — a misplaced parenthetical in the README read as if item CREATE lowers into fields_patch. It does not; it sends the full blob. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(mcp,models): stop the remote hint advertising the broken PR workaround; classify an unparseable blob as retry-hostile (Codex round 5) P1 — I corrected four artifacts that pointed agents at the github_pr field write and missed the fifth: noRemoteEquivalent's own text, which IS the message a remote agent receives when it calls `github link`, and which Codex had quoted at me in round 3 to establish the workaround existed. The nearest artifact to the actor was the one I did not open. Both entries now say there is no working remote path and name BUG-2696, with a test pinning the negative so a future edit cannot quietly reinstate the advice while the write is still broken. P2 — a fields blob that will not parse at all produced a bare parse error, so `note` / `decide` reached agents as `server_error`: transient- looking, and therefore retried, for a failure that is as deterministic as the per-key one BUG-2675 exists for. Both Append* helpers now wrap that parse failure in ErrStructuredFieldUnreadable, which both transports already classify, and the malformed-blob test asserts the sentinel rather than just an error. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(mcp,cli): qualify what an agent can actually see when the state is unreadable (Codex round 6 nit) Round 5 widened stored_state_unreadable to cover a fields blob that fails to parse outright, which made half of its own hint false: MCP's normalization strips a broken structured KEY (so `get` hides it), but leaves an unparseable BLOB as a raw string (so `get` shows it). The hint and instructions.md asserted the first case for both. Now stated per layer, in the two paired constants and the instructions. The reason it is worth the words rather than being cut: an agent told 'you cannot see this' does not look, and would have missed a value that was in fact right there in the response it already had. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(mcp): classify the move/copy reserved-key refusal as validation on stdio too (Codex round 7) P2 — carried over from v0.22, surfaced because THIS bump documents the two reserved-key refusals as agreeing across transports. The move/copy message ("Field(s) reserved for system metadata and not settable here") matched none of the stdio validation patterns, so the same deterministic 400 arrived as validation_failed on remote and server_error on stdio — and server_error reads as transient, so an agent retries a refusal that can never pass. One pattern added, plus a test that drives both real classifiers with the real server message text for both refusals, so a reworded message that stops matching fails here rather than in the field. nit — the github_pr exemption is UPDATE-only; move and copy still refuse it, because there the argument is BUG-2674's (an override reintroduces the key the migration just dropped), not this one's. The catalog and instructions said "not refused" without that qualifier. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(mcp): cover the copy path's own refusal wording in the stdio classifier (Codex round 8) P2 — round 7 fixed the MOVE wording; the copy path words the same class of refusal differently ("Destination collection has no field(s): ..."), so it kept arriving as server_error on stdio and validation_failed on remote. Third message in one family, and the round-7 test used the move text for every case, which is why it missed this. The parity table now carries all three real messages plus a control leg using one the pattern list already covered — without it the table could pass by matching everything. Recorded in the pattern list's comment rather than left implicit: matching prose is a stopgap, the structural fix is the pad-structured-error/v1 marker that carries the code instead of inferring it, and until a refusal emits one, this test is where a new wording has to be added. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * test(mcp): use the real upstream codes in the parity fixtures (Codex round 9 nit) The copy legs carried `validation_error` where the handlers actually emit `malformed_override` and `invalid_override`. The 400 branch ignores the body code today, so the test passed either way — which is exactly why the fixture mattered: it was quietly recording a wrong contract, and a future code-aware classifier would regress against a table that agrees with it. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(mcp): the upstream code is not forwarded to MCP clients (Codex round 10 nit) The catalog said the server's own code (validation_error / malformed_override) appears in the MCP message. It does not: the 400 branch emits code=validation_failed with a fixed "Validation failed." message and the server's text in the HINT, discarding the finer-grained code. Reworded to say what an agent actually receives, and to say that telling the two refusals apart means reading the message. Also carried the update-only qualifier on the github_pr exemption into the README, matching the catalog and instructions. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(items): state the exemption predicate, not the exemption list (lead ruling) The lead's ruling on the github_pr reversal: make the REASON what the code says, so the next key added to reserved metadata is evaluated against 'does this audience have a real writer?' rather than pattern-matched onto a list that happened to be wrong for one key. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V |
||
|
xarmian
|
bc68b84848 |
fix(cli,mcp): send raw collection slug so an alias can't shadow a real collection (BUG-2630) (#1162)
* fix(cli,mcp): send raw collection slug so an alias can't shadow a real collection (BUG-2630)
The client-side alias map (collections.NormalizeSlug) rewrote seven hardcoded
singulars ("task", "plan", …) to their plurals BEFORE the request. In a
workspace whose collection slug IS one of those singulars, the user's exact
name was rewritten away and their create/list/move landed in a DIFFERENT
collection — silently, with a success message naming the wrong one.
Fix, per lead ruling on the BUG-2630 trail, split by transport:
CLI (real HTTP, may hit a pre-resolver server) — Option 2, one shared helper
cli.WithCollectionAliasFallback: send the RAW slug first (the server's
exact-match-first resolver from BUG-2578 wins, so an exact name is never
shadowed), and retry with the alias ONLY on a collection-not-found error, only
when the alias differs. Keying on collection-not-found is load-bearing: a
request to a collection that exists but fails for another reason is never
retried into the alias (that would recreate the bug). Both the schema fetch and
the create funnel through the helper so typed --field values parse against — and
the item lands in — one collection. On a genuine double-miss the error names the
RAW slug the user typed (collection "widget" not found), not the alias.
MCP remote transport (in-process ServeHTTP against the SAME binary, which always
carries the resolver — no version skew) — drop client-side normalization
entirely and send raw. Also removed the dormant expandPath collection
normalization: no routeSpec uses a {collection}/{target_collection} path
placeholder, so the branch was dead code in the area this fixes.
Search is deliberately out of scope (filed BUG-2659): its collection is a global
c.slug=? FILTER, not a path — a miss returns 200 + zero results, not
collection-not-found, so the retry can't key on it; and handleSearch is
cross-workspace, so the per-workspace resolver has no single workspace to run
against. Cross-workspace copy is excluded too (DR-13 forbids auto-retrying the
copy mutation).
Verified live against a real server: create/list/move into a singular collection
that collides with its plural now land in the named singular; shorthand still
resolves; genuine misses error naming the raw slug. New MCP integration test
reproduces the original shadow (item → PLANS-1) when normalization is restored.
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
* fix(server,cli): own collection resolution server-side + capability-gate the CLI retry (BUG-2630 Codex r1)
Addresses all three Codex round-1 findings, via the lead's ruling that
dissolves the earlier "retry vs archived-protection" tension by making the
server the sole owner of resolution semantics.
Finding #2 (MCP lost the legacy abbreviations t/i/p/d and phase/phases -> plans,
which the server's ±s resolver did not cover): fold the legacy alias map into
collectionSlugCandidates as a LAST-resort candidate. Exact-match-first and the
archived-claims refusal run for the input and every structural candidate before
the alias is reached, so it never shadows or redirects around a real/archived
collection. Now every client can send the raw slug — including the MCP transport
that can't retry — and lose nothing.
Finding #1 (the client retry re-opened the archived/hidden redirect the server
deliberately refused, because not_found can't be told from absent): add a
collection_resolution capability flag to GET /server/capabilities and gate the
CLI retry on it. Happy path unchanged (raw slug, one request). On
collection-not-found ONLY, the client probes capabilities once (cached): if the
server advertises resolution, its not-found is authoritative — the slug is
absent, archived, or hidden — so the client does NOT retry. Only an older server
that lacks the flag (or 404s the endpoint) triggers the legacy alias retry,
which is non-regressive there since old servers never had the protection. The
probe fails safe toward retry. This makes the follow-up distinct-error-code bug
unnecessary.
Finding #3 (double-fail masked a substantive alias error as "collection not
found"): the helper now surfaces a substantive alias-attempt error verbatim, and
only collapses to the raw-named not-found when the alias ALSO 404s.
Verified live against a resolving server: create/list/move into a singular that
collides with its plural land in the named singular; the abbreviation `i`
resolves to `ideas`; and after archiving `plan`, `create plan` honestly fails
("collection \"plan\" not found") instead of being retried into a live `plans`.
Gates: make lint 0 issues; go test ./... green; make test-pg green.
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
* fix(cli): fail-closed capability probe + always-retry the schema lookup (BUG-2630 Codex r2)
P1: the capability probe cached ANY failure as "no resolver", so a single
transient blip (timeout/5xx) permanently re-enabled the alias retry and could
bypass the archived/hidden protection on a resolving server. Now the probe
distinguishes a DEFINITIVE verdict (HTTP 200 with the flag, or a clean 404 =
legacy build) from an INDETERMINATE one (transport error / 5xx): only definitive
verdicts are cached, and an indeterminate probe fails CLOSED (trusts the
not-found, no retry) without caching, so the next call re-probes. A genuine old
server still returns a clean 404, so its retry is unaffected. Renamed the
predicate to CollectionNotFoundIsAuthoritative to name what it actually decides.
P2: the create schema lookup hits exact-match-only GetCollection, which does NOT
resolve slugs server-side, so capability-gating it made `create task
--field amount=3` 404 the schema fetch, skip the retry, and send amount as the
string "3". The schema lookup now always retries the alias (nil gate),
restoring typed-field parsing against an aliased collection's schema. Best-effort
as before: a genuine miss still degrades to string fields.
New client test covers the probe: definitive verdicts cache (one probe), and a
transient failure fails closed AND re-probes on the next call (mutation-verified).
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
* docs(cli): note fail-closed-on-indeterminate as a deliberate safety asymmetry (BUG-2630)
Per lead review: make explicit in CollectionNotFoundIsAuthoritative's doc that
failing closed on an indeterminate capability probe is deliberate — a recoverable
alias-shorthand miss is the safer side of the trade vs a retry doing an
un-undoable wrong-write. Comment-only.
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
|
||
|
xarmian
|
e40df6b31c |
feat(cli): pad session arm/disarm/status + consent config resolution (PLAN-2613 S2, TASK-2617) (#1149)
* feat(cli): pad session arm/disarm/status + consent config resolution (PLAN-2613 S2, TASK-2617) The S2 CLI contract S3's plugin skills and S4's web composer build against. S1 gated push delivery on a server-side armed bit declared at stream connect; nothing decided WHETHER to arm or sent the declaration. S2 adds both, defaulting off everywhere. - ResolveAutoArm (internal/cli/arm_consent.go): pure consent resolver. .pad.toml [push] auto_arm is the only per-repo enabler (D4); a per-user config auto_arm=false vetoes it (deny-wins); default off. Config surfaces: PadToml.Push.AutoArm + config.Config.Push.AutoArm (*bool, unset != false), both nil-safe. - Wire contract: StreamSessionIdentity.Armed sends ?armed=true on the event stream — S1's server gate finally has a sender. The monitor announces armed = live local arm OR resolved auto_arm, so a repo opt-in works end to end with a safe default-off skew. - Verbs pad session arm/disarm/status: arm/disarm manage a per-session local arm-state file; status reports the resolved local/auto decision plus the server's own armed/connected counts (new Client.ListSessions), degrading gracefully when padd is unreachable. - Arm-state file (session_arm_state.go): keyed per session by CLAUDE_CODE_MESSAGING_SOCKET (cwd fallback for headless, secondary to auto_arm). Mandatory liveness — a dead-owner file (socket vanished / pid gone) reads as disarmed and is reaped, so a crashed session can never arm a future monitor. Local client state only; the server's armed bit stays the sole delivery authority. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(cli): address Codex R1 on push-consent (fail-closed config, owner-identity liveness) - HIGH-1: user config.toml read now fails CLOSED. config.LoadPushConfigAutoArm reads the [push] auto_arm value strictly — absent → no opinion, but present-but-unparseable → error — and ResolveAutoArmFromDisk refuses to auto-arm when it can't confirm the user's veto (was: swallowed by the lenient config.Load and treated as no-opinion). - HIGH-2: arm-state liveness now checks owner IDENTITY, not just presence. Socket-keyed files record the socket's mtime and require an exact match, so a reused socket path can't revive a stale file. Headless files record a Linux /proc start-time token (portable fallback documented) to reject a reused pid. - MED-1: arm-state writes are atomic (temp + rename) and reaping is non-destructive (re-checks staleness before removing) — a concurrent re-arm is never clobbered. - MED-2: pad session status applies the .pad.toml URL override, so it queries the same server the monitor connects to. - LOW: malformed arm-state files are now reaped (safe now that writes are atomic — a corrupt file can't be a torn in-progress write). Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(cli): address Codex R2 on push-consent (atomic config write, stronger owner identity) - HIGH-1: Config.Save() is now atomic (temp + rename), so a monitor reconnecting while `pad configure` rewrites config.toml can't read a truncated/partial file, miss a [push] auto_arm=false veto, and arm. - finding 2: socket owner identity now uses inode+device (unix) as the primary signal, with mtime as the non-unix fallback — a rebound socket or a lingering stale node at the same path gets a new inode and is rejected, closing the mtime-collision / reused-node gaps. - finding 3: headless liveness fails closed when a proc-start token was recorded but can't be re-verified (was: fell back to bare pid-liveness, which a reused pid passes); zombies (state 'Z') now report not-alive. - finding 5: `pad session status` applies an explicit --url override too, not just the .pad.toml one. - finding 4 (connect-time TOCTOU): documented as an accepted, bounded residual — a disarm racing an in-flight connect is corrected on the next reconnect; fully closing it needs S3's server-side disarm-on-open signal. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V |
||
|
xarmian
|
625cab9984 |
fix: bound item history and stop resolving bodies nobody reads (BUG-2608) (#1147)
* fix: bound item history and stop resolving bodies nobody reads (BUG-2608)
Item history was unbounded on every surface, and summary mode paid for what it
discarded: the endpoint resolved EVERY version by walking the item's whole
reverse-patch chain, and both the CLI and the MCP dispatcher then projected
that away to metadata. An item edited under collab records a version every few
seconds while someone types, so this is routinely hundreds of full-content
reconstructions per history call, for output that shows none of them.
Two independent fixes, because they address different costs.
SUMMARY SKIPS THE WALK. `?summary=true` returns metadata from the raw rows and
never resolves a patch. That is the dominant win: the resolution was pure waste
for every caller except --full. Content and is_diff are cleared TOGETHER — an
empty body still claiming to be a reverse patch would tell a consumer to
resolve something that is not there.
LIMIT BOUNDS THE WINDOW, newest-first. That direction is not a preference: with
reverse patches, reconstructing any version means walking back from current
content, so a newest-end window is the cheap prefix of that walk while an older
one still pays for everything above it. That is also why there is deliberately
no offset — it would advertise a pagination whose later pages cost the same as
no bound at all.
Absent limit stays UNBOUNDED on the endpoint, following the item-list
precedent (maxItemListQueryLimit: "a zero/absent limit is left unbounded — this
only clamps an explicit oversized request"). The defaults live on the CLIENTS,
where a token budget is actually known: `pad item history` defaults to 50 with
--limit to change it, and the MCP catalog action injects 50 (max 300, the same
pair list and backlinks already use). A server that truncates a request nobody
bounded is a silent-truncation trap for third-party API consumers.
The MCP default goes in the CATALOG action rather than either dispatcher, so it
reaches BOTH transports — HTTP reads it off the input, and stdio receives it as
the CLI's new --limit through BuildCLIArgs. ToolSurfaceVersion 0.20 -> 0.21
with a changelog entry, plus instructions.md and README, per the 2304-family
contract discipline. Additive param bump: `limit` already existed, nothing
changed shape, and a v0.20 consumer that sends no limit now gets the newest 50
instead of all — which is the fix, not a break in it.
The restore and single-version-expand paths still resolve the FULL chain, and a
test pins that: bounding their walk would strand exactly the old versions those
paths exist to reach.
Eight mutations, each failing only the leg it targets. Three fixture problems
surfaced that way and are worth naming, because each made a test that could not
fail:
- force_version in a PATCH body does nothing (`json:"-"` on ItemUpdate), so
the throttle collapsed six edits into one version; varying the source per
edit is what actually records them.
- an 8-byte body is cheaper stored whole than as a patch, so no version was
ever is_diff=true and the is_diff assertion was inert. The fixture now uses
a body large enough that the store really stores patches.
- the cmdhelp test fixture lacked the new --limit flag, so BuildCLIArgs
silently dropped it. Verified against the REAL cmdhelp tree that the flag
is present and typed int, so the fixture mirrors the CLI rather than
flattering it.
* docs: bring CLAUDE.md to v0.21 and name why the two result caps differ (BUG-2608)
Codex round 1, both findings.
CLAUDE.md still described the MCP surface as v0.20 — stale because of my own
bump, in the document every agent working this repo reads first. README and
instructions.md are held to the version by a test; CLAUDE.md is not, which is
exactly why it drifts.
The cap "mismatch" (MCP max 300, endpoint clamp 500) is deliberate layering,
not an oversight — item lists have the identical split (300 in the catalog,
1000 at the endpoint) because the two answer different questions: an agent
token budget is only knowable in the catalog, while the endpoint's clamp is a
server-resource ceiling on what any caller may ask for. But nothing said so
anywhere, so a reader comparing the two numbers had no way to tell design from
accident — which is precisely the report Codex filed. Now stated at the
constant and in CLAUDE.md, including why the versions ceiling is LOWER than the
list one (resolving a version can cost a patch application per row, not just a
row read) and why an absent limit is left unbounded at the endpoint.
* fix+test: honest truncation notice, armed fixtures, and the residual named (BUG-2608)
Codex round 2, both findings, and the second is the more useful one.
CLI TRUNCATION NOTICE was wrong in both directions: it compared the response
length against the requested limit, so an item with exactly N versions was
reported as truncated, and a --limit above the server's ceiling was clamped
there and reported as complete. It now asks for ONE MORE row than it shows and
reports truncation only when that extra row comes back. The one case this still
cannot detect — an ask above the server's own ceiling, where the probe row is
clamped away with everything else — is stated in the code rather than papered
over by hardcoding the server's constant in the CLI.
UNDER-ARMED FIXTURES. The unbounded test seeded 5 versions, so a server quietly
defaulting to 50 would have passed the assertion that denies exactly that; it
now seeds 60. The clamp test seeded 2 and could not observe a clamp at all;
the clamp is now asserted directly against a parseItemVersionsLimit function
extracted for the purpose, over the inputs a URL can really carry (absent, 0,
negative, unparseable, either side of the ceiling).
That extraction replaced my own first attempt, which was worse than no test: it
re-implemented the clamp arithmetic in the test body and asserted the result
against itself. It could not have failed.
THE RESIDUAL, NAMED RATHER THAN IMPLIED. Codex's sharpest point is that the
summary tests cannot detect "resolve everything, then clear the fields" —
verified by mutation: pointing the summary branch at the resolving reader
leaves every handler test green, because the response is byte-identical either
way. So the performance claim does not rest on them. It rests on the handler's
summary branch calling ListItemVersionsPage (one reviewable line) plus a new
store test proving that reader really returns unresolved rows rather than
quietly resolving them — mutation-verified from the other side by making the
resolver a passthrough. The test file says all of this, including that an
end-to-end assertion would need a patch-application counter in the production
path, and why that is not worth it when the cost of being wrong is performance
rather than correctness.
* fix(cli): don't resolve for table output, guard the probe overflow, finish the CLAUDE.md bump (BUG-2608)
Codex round 3, four findings.
--full was treated as "content needed" regardless of output format, but the
table path prints no bodies at any setting — so `pad item history --full`
without --format json made the server walk the entire patch chain to build
content the CLI then dropped. That is the exact waste this bug is about,
reintroduced through the flag meant to opt into it. Content is now resolved
only when it will actually be shown.
The limit+1 probe overflowed at MaxInt: it wrapped negative, the client omitted
the parameter, and a request the user bounded came back unbounded — the
opposite of the ask. Guarded.
The truncation notice's documented blind spot was understated: it is AT the
server ceiling as well as above it, since the probe row is clamped away with
everything else. Wording corrected rather than resolved — the CLI still does
not duplicate the server's constant, because a copied ceiling goes stale
silently and asking for hundreds of versions is already opting out of a bound.
Two more CLAUDE.md sites still called v0.19 current; I fixed only the first on
the previous pass. That document describes the contract in three places and I
updated one, which is its own small lesson about grepping for every instance of
a claim rather than the first.
Live-verified against a real server: --limit 2 truncates and says so, --limit 4
on a 4-version item says nothing (the false positive Codex found), --limit 0
returns everything, --full --format json carries bodies, and the default JSON
shape carries metadata only.
* fix(server): clamp an out-of-range limit instead of treating it as unbounded (BUG-2608)
Codex round 4. `?limit=9223372036854775808` made strconv.Atoi return ErrRange,
which my parser lumped in with 'unparseable' and mapped to 0 — unbounded. So an
absurdly large number defeated the very ceiling the function exists to impose,
which is worse than no clamp at all: the bound looked enforced and was not.
Atoi hands back the saturated value alongside ErrRange, so a range-positive
input is now clamped to the ceiling — it is an oversized ASK, not a malformed
one. Range-negative still falls through to unbounded, matching a plain
negative.
Regression cases cover both overflow directions, and the mutation that
reinstates the old behaviour fails on the exact input.
This is the input-domain lesson again: I enumerated absent, zero, negative and
non-numeric, and stopped at what a person would plausibly type rather than at
what the TYPE admits.
* docs(cli): say in --help that a large --limit is capped server-side (BUG-2608)
Codex round 6, and the third time this ceiling came up — which is the tell that
the disposition was in the wrong place. I had documented it twice in CODE
comments, where the person affected never looks. `--help` says what the flag
does, and it said 'show only the newest N versions (0 = all)', which is false
for an N above the server's cap.
Now stated in both the flag help and the command's long description, including
that the truncation notice cannot detect that case and that --limit 0 is the
way to ask for a complete history.
Deliberately not naming the number: a constant duplicated into help text goes
stale silently, and 'capped server-side' is the part a user needs in order to
act. Behaviour unchanged — this is the artifact catching up with it, which is
the same correction the BUG-2301 sentinel comment and the instructions.md
overclaim both came down to.
* test: seed versions through the store so the fixture stops tripping the rate limiter (BUG-2608)
CI caught this and it is my defect, not a flake. Arming the unbounded test
above the plausible default meant seeding 60 versions, and the fixture did that
with 60 HTTP PATCHes in a burst — which trips the server's rate limiter. It
passed locally and in the Go job and failed under Nix, which is the signature
of a test that was always going to fail intermittently rather than one that
broke.
Seeding now goes through the store. That is not a weakening: versions are
recorded by the store on either path, and the endpoint under test is the READ
side, so seeding underneath the transport costs the assertions nothing while
removing a burst the server is entitled to refuse.
The three things that make this fixture work are now stated where someone would
otherwise undo them by accident — the large body (a small one is stored whole,
so no version is ever is_diff and every diff assertion goes vacuous), the
rotating source (the throttle collapses same-(actor, source) bursts into one
version), and the store-not-HTTP seeding with the rate-limit reason attached.
Re-verified after the change: the fixture still records more than 50 versions
and still produces reverse-patch rows, and the default-cap mutation now uses
the REALISTIC default of 50 rather than the 3 I first tested with — the old
5-version fixture could only have caught an implausibly small cap.
|
||
|
xarmian
|
c84cf7437c |
feat(sessions): announce session identity on the event stream (PLAN-2558 S2, TASK-2560) (#1094)
* feat(sessions): announce session identity on the event stream (TASK-2560) PLAN-2558 S2. S1 gave the presence registry a count of anonymous uuids; this makes each row nameable, which is what S3 needs for an honest empty state and S5 needs for a target picker. A monitor now announces itself when it opens the stream: X-Pad-Session-Label (the working directory's basename) and X-Pad-Session-Pid. The server sanitizes both and stores them on the LiveSession; GET /api/v1/sessions returns them. TRANSPORT. The task body sketched "the stream connect carries it" without picking a mechanism and explicitly left the call open. Headers, because a query param would put the label and pid into every access-log line (this server logs path= for each request) and any proxy log in front of it — which is the same "don't let local detail travel further than it needs to" the privacy line below is about — and a separate registration POST would need its own correlation to the connection it describes, plus a matching lifecycle, when the registry entry already lives and dies with the stream. Headers ride the request that exists and sit alongside Last-Event-ID, already doing this job on this endpoint. Cost, written into the code rather than discovered later: a browser EventSource cannot set headers, so a future web-tab consumer needs a deliberate query-param fallback or a fetch-based SSE reader. PRIVACY. The basename crosses, never the full cwd — "/home/dave/Dev/ docapp" additionally hands over a home directory and usually an account name for no gain — and messaging_socket_path never leaves the machine. Pinned by a test rather than by the implementation being one line. WHAT THIS DELIBERATELY DOESN'T DO: read ~/.pad/sessions/. The task framed S2 as giving `pad session register` its first consumer, and the monitor cannot honestly be one. Registry entries are written by whatever process ran that command — a different pid — and the only matchable fields are pid and cwd, so two agent sessions in one checkout are indistinguishable and "pick the newest" is a coin flip that would put a confident wrong name in the S5 picker. Process ancestry settles it exactly and is platform-specific (this binary ships for macOS and Windows). The monitor's own cwd basename and pid are never wrong and answer the question the label exists to answer; correlating a stream to the agent session that spawned it needs an identifier the harness passes down, which is worth doing when something needs it and worth not faking until then. Also moves S1's STALENESS doc block, which sat above LiveSession.Label where it read as documenting the name rather than the whole entry. Tests: sanitizer units (whitespace collapse, control-char stripping, rune-not-byte truncation), header wiring, the end-to-end labelled session, the unannounced-client compatibility leg (a pre-S2 monitor must still register and still stream), a hostile-input leg over the wire, the client's omit-when-unset behaviour, and the basename promise. Measured rather than assumed: Go's server answers 400 to a header value containing a control byte before any handler runs (verified with a raw socket, since Go's own client refuses to send one and the two refusals are indistinguishable from a normal client test). So that arm of the sanitizer is unreachable over HTTP; it stays as defence in depth for the next caller in, and both the comment and the wire test say so instead of the test quietly passing because the transport refused the input. Mutation-tested four ways, each revert grep-verified: handler ignoring the parsed identity, monitor sending the full cwd, dropping the truncation, and the client always setting the headers. Refs TASK-2560, PLAN-2558 * fix(cli): sanitize the session label client-side per Codex review (round 1) Codex round 1's only finding, and it is a bigger deal than a missing label. Unix directory names may contain control bytes — "doc\napp" is a legal directory — and Go's http.Client REFUSES to send a request whose header value holds one: Do returns "invalid header field value" and nothing is transmitted. In the monitor that is indistinguishable from an unreachable padd, so the retry loop backs off and tries again, forever, printing nothing by contract. A user who named a directory that way would simply stop receiving notifications, with no signal anywhere. The server cannot defend against a request that never arrives. Reproduced before fixing, with a real directory and a real client, rather than reasoned about from the error message. Sanitizing in NewWatchEventsStreamRequest rather than in monitorSessionIdentity: the invariant is "this function never builds an unsendable request", which belongs at the point where a value becomes a header, not at one caller. The client's cap (256 runes) is deliberately looser than and independent of the server's (64): the server decides what a label should look like, the client only has to keep the request sane, and neither has to track the other to stay correct. The regression test does the ROUND TRIP instead of inspecting the header, because the header contents were never the bug — http.Header.Set stores anything, so an assertion on the value passes against the broken version too. Only attempting the request tells the two apart. Mutation-verified: reverting the sanitizer fails the test with exactly the "invalid header field value" error from the field report. |
||
|
xarmian
|
da6ce642da |
feat(push): pad push — user-authored instruction dispatch to agent sessions (IDEA-2544 Phase 1) (#1090)
* feat(push): add pad push <ref> -m vertical (IDEA-2544 Phase 1)
Self-addressed, human-to-harness dispatch over the existing watch-events
bus/stream: CLI -> POST .../items/{itemSlug}/push -> a new KindPush
Notification (carrying the generalized TargetUserID addressed-to field
KindAsk will later share) -> watchNotificationVisible delivers it back
to the pushing user's own connected monitor sessions. Transient,
fire-and-forget by design (no migration, no durable inbox) since
assignment already covers the durable-notification case and this is
meant to be the explicit, no-inference dispatch verb instead.
* docs(plugin): document the push notification contract (IDEA-2544 Phase 1)
Push is the one notification kind that IS an instruction rather than a
passive fact, so it gets its own lead bullet in the plugin skill's
notification-etiquette section (ahead of the read-only/park default,
which it explicitly lifts) and a mention in the monitor's description.
The embed-source skills/pad/SKILL.md has no notification section to
mirror this into (the two files diverge by design) and is left
untouched.
* fix(push): reject over-long push messages instead of unbounded Summary
Comments truncate their notification Summary to a preview (the full
body is still fetchable), but a push message IS the payload — silently
truncating it would corrupt the instruction with nothing to recover it
from. Add maxPushMessageLen (4096, measured post-collapse) and reject
anything over it with a 400 rather than truncating; state the same
bound in `pad push --help` so it's discoverable before a 400, not only
from one.
* fix(push): close the watch-fallthrough leak, disambiguate SKILL.md exceptions
Codex round 1 P1: watchNotificationVisible's push branch only returned
early on a MATCH — a non-target caller fell through to the watch-map
check below it, so anyone holding an unconditional (or predicated)
watch on the item received every push addressed to every OTHER user,
instruction text included. Push is addressed private dispatch, not an
item-level fact watchers have a legitimate claim on (unlike assignment,
which watchers are expected to see per `pad watch --help`) — the branch
now returns unconditionally for KindPush, gating strictly on
TargetUserID and never reaching the watch-map fallback either way.
Pinned explicitly since Phase 4's session targeting is expected to
inherit this same exclusivity.
Also (codex P2): reworded the SKILL.md notification-etiquette bullets —
the new push exception and the pre-existing assignment/ask exception
literally contradicted each other ("the ONE narrow exception" claimed
singularity after push had already claimed exception status). Now
explicitly enumerated as the first and second exceptions to the
never-write rule.
* test(cli): pin that PushItem inherits X-Pad-Agent (BUG-2542 rebase)
Verified, not assumed: PushItem builds its request via c.post ->
c.newRequest like every other mutating client method (CreateWatch
included), so the attribution fix's client.agentName wiring covers it
for free with zero code changes needed on this branch. Adds a live
httptest assertion rather than trusting the code-path read alone —
the same shape as TestClientSendsResolvedAgentHeader, scoped to
PushItem specifically since that's the one method this PR added.
* fix(push): disambiguate workspace in the monitor line and skill contract
Codex round 2 P1: the watch-events stream is user-scoped ACROSS every
workspace a caller has watches in, but formatMonitorLine printed only
ItemRef/Kind/Actor/Summary and dropped the Workspace field the wire
payload already carried — a session linked to workspace A receiving a
notification for workspace B would resolve the wrong item (or 404) with
no signal in the line that anything was off.
Fixed universally, not push-only: grepped plugin/ and skills/ for
anything parsing "PAD ..." lines and found none — the Claude Code
plugin host ingests the stdout line as free-text notification prose,
formatMonitorLine's only real consumer is its own fmt.Println, so there
is no wire-format consumer a workspace prefix could break. The
ambiguity predates push (any watched item across workspaces already had
it); push just makes the consequence sharper because it carries an
instruction rather than a passive fact.
SKILL.md's push bullet now tells the agent to resolve with
`pad --workspace <workspace> item show <ref>` using the slug read off
the notification line, not a bare `pad item show <ref>`.
* fix(push): respect --format json instead of hardcoding plain text
Codex round 2 P2: pushCmd's RunE ignored the global format flag and
always printed "Pushed <ref>", silently discarding --format json.
- server.pushResponse replaces the bare map the handler wrote before —
a typed {ref, workspace, pushed, message} shape, with workspace
resolved to the CANONICAL slug via s.getWorkspace (not merely echoed
from whatever the URL contained), matching the same disambiguation
need the round-2 P1 fix addressed for the monitor line.
- cli.PushItem now returns (*PushResult, error) instead of discarding
the response body.
- pushCmd checks formatFlag == "json" and calls cli.PrintJSON, mirroring
runCreateWatch's existing pattern.
internal/cli/agent_identity_test.go's TestPushItemSendsResolvedAgentHeader
needed a one-line update for PushItem's new two-value return — caught by
`go vet ./...`, not `go build ./...` (which doesn't compile test files);
folding vet into my own pre-flight going forward.
|
||
|
xarmian
|
212d59e7c6 |
fix(cli,server): make agent attribution actually happen (BUG-2542) (#1088)
* fix(cli,server): make agent attribution actually happen (BUG-2542)
Agent CLI writes were recorded as the human whose credentials they used.
Three independent defects, each verified by reading the path AND by
probing a live instance — the item deliberately held the mechanism open,
so none of this is inherited.
1. THE HEADER WAS NEVER SENT. actorFromRequest sets actor="agent" on one
signal: the X-Pad-Agent header. The only code that sets it took the
value from `agent_name` in .pad.toml and nowhere else — no
environment detection, no session detection. This repo's .pad.toml
has only `workspace`, so the header has never been sent from here and
every agent write has looked human. ResolveAgentName now resolves
.pad.toml → $PAD_AGENT → detected runtime.
2. ITEM CREATE DISCARDED THE ACTOR. createItemChecked called
actorFromRequest and kept only the source (`_, src :=`), never
setting input.CreatedBy, so store.CreateItem fell through to its
"user" default — even for an agent that DID send the header.
Comments have always stamped it correctly; item creation silently did
not, which made the skill's own contract false on its own terms.
3. SINGLE-ITEM PATCH NEVER STAMPED LastModifiedBy. Bulk ops do
(handlers_items_bulk.go); the single-item path did not, so an item
edited only by agents read as human-edited.
Only entries VERIFIED against a live session belong in the runtime
detection table, so it has exactly one: Claude Code exports CLAUDECODE=1
to child processes, confirmed by reading a pad subprocess's environment
inside one. Guessing at Cursor/Windsurf/Aider variable names would put
unverified claims in a shipped binary and misattribute silently when
wrong; those set $PAD_AGENT until someone confirms a signature.
WHAT THIS DOES NOT DO, stated in the code and the skill rather than left
for someone to assume: the header is client-supplied and self-declared.
An agent that omits it is indistinguishable from the human it borrows
credentials from, and a human running `! pad ...` inside an agent's
terminal inherits that environment and is attributed to the agent. This
makes the trail HONEST, not VERIFIED — it is not a basis for
machine-verifiable human-approval provenance, which needs a channel the
agent cannot author at all. The incident behind this item is exactly
that distinction: an agent's relay of a human's words was recorded
indistinguishably from the human typing them.
Contract corrected in both skill copies, since the item's first question
was which of contract and behavior was wrong. It was the contract: it
promised automatic agent attribution that only ever applied to
workspaces that had opted in.
Tests, each mutation-tested against its own defect reverted alone:
- TestResolveAgentName — precedence plus the negative that makes it mean
something: a plain human shell must still resolve to "". Fails 2/5
reverted.
- TestItemAttribution_AgentVsHuman — agent and human legs for create,
update and the create-stamp-survives-edit invariant. Fails on the
create stamp reverted; fails 2/2 on the update stamp reverted.
The update leg deliberately uses the OTHER writer: insertItemTx seeds
last_modified_by FROM created_by, so a same-writer edit passes whether
or not the PATCH stamps anything — the first version of this test did
exactly that and passed its own counterfactual. Caught only because
each fix was reverted separately.
- TestItemAttribution_ExplicitBodyValueWins — an explicit body value
still beats the header.
End-to-end on a live instance through the real CLI, no .pad.toml opt-in:
agent session → created_by/last_modified_by/comment all `agent`; same
binary with CLAUDECODE stripped → all `user`.
Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag
* fix(server): artifact import wrote a UUID into created_by (BUG-2542)
Found by Codex while reviewing the attribution fix. handleImportArtifact
set `input.CreatedBy = u.ID`, which is the wrong DOMAIN for the field
rather than merely the wrong value: created_by holds the role — "user"
or "agent" — and consumers compare it against those literals
(CommentThread.svelte, TimelineVersionCard.svelte). An imported item
matched neither and rendered as neither.
It also would have defeated the fix in the parent commit at this path: a
non-empty CreatedBy suppresses the actor stamp, so imports would have
kept a UUID while every other create path started recording the actor.
The line contradicted the comment directly above it, which said Source
was being left blank precisely so createItemChecked could stamp it "like
every other create path". Now both fields are left blank and stamped
together.
The user's identity has its own home — the items.created_by_user_id
column — which no create path currently populates. That is a separate
gap and is not widened into this change.
Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag
* fix: close the remaining attribution bypasses Codex found (BUG-2542)
Review found no P1s and three P2 families beyond the artifact-import bug
already fixed in
|
||
|
xarmian
|
ec7fd027fc |
feat(server,cli): watches, user-scoped event stream, plugin monitor command — PLAN-2469 Phase 1 (TASK-2533) (#1082)
* feat(store): race-free status/assignment mutation signal (TASK-2533)
Adds models.Item.LastMutation (ItemMutationSignal), populated inside the
SAME transaction that already writes status_transitions / assigned_user_id
in UpdateItemWithParentLink and MoveItemWithPreCheck. This is the
foundation for TASK-2533's watch-notification pipeline: a before/after
snapshot taken in the HTTP handler layer would race concurrent writers of
the same item, so the signal is computed where the authoritative diff
already happens, in-transaction.
* feat(store): watches table migration, both drivers (TASK-2533)
watches(id, workspace_id, user_id, item_id, predicate, created_at) per
DOC-2479's subscription-table design: durable, server-side subscriptions
that survive both the plugin-monitor process and a padd restart.
uq_watches_user_item makes `pad watch <ref>` idempotent (re-watching
upserts the predicate). Wires watches into the workspace-purge child-delete
list, mirroring item_stars.
* feat(watchevents): add in-process notification bus (TASK-2533)
New package: a global (not per-workspace) in-process pub/sub bus carrying
watch-worthy Notifications (status-change / assignment / comment; ask
reserved in the enum with no producer yet — see the follow-up server
commit). Bus is an interface specifically so a Redis-backed implementation
can slot in later without touching any caller; only MemoryBus exists today.
Package doc comment states the single-process/multi-instance limitation
explicitly, mirroring internal/events' shape.
* feat(store): watches CRUD (TASK-2533)
models.Watch + Store.CreateWatch (upsert on user+item)/GetWatchByUserItem/
ListWatchesForUser (unscoped by workspace — a watch is personal, and the
event-stream handler needs every watch a caller holds across all their
workspaces)/DeleteWatch.
* feat(server): watch/nudge event stream + CRUD endpoints (TASK-2533)
GET /api/v1/events/stream (DOC-2479): a user-scoped, cross-workspace SSE
stream, filtered server-side to the caller's watches (with optional
--until field=value predicate) plus "addressed to you" — narrowed to
assignment-to-you only for Phase 1, confirmed with the dispatcher: this
codebase has neither a Collection.Kind field nor any user->active-role
binding to ground DOC-2479's "human-gate-shaped collection targets your
active role" half mechanically. watchevents.KindAsk stays in the wire
enum with no producer. `pad session register` is the natural future hook
for a session-carried role identity.
POST/DELETE .../items/{slug}/watch, GET /api/v1/watches (unscoped,
mirrors /auth/tokens' shape for a personal, not workspace, resource).
Producer wiring (TASK-2533 audit) publishes from every live mutation path
that can produce a LastMutation signal or a new comment: handleUpdateItem
(incl. its collab sub-paths and the comment-attached-to-update path, which
bypasses handleCreateComment entirely), handleMoveItem, handleCreateComment,
item creation with an initial assignee, and the bulk-items loop (covers
archive/restore/move/set-priority/tag/untag/assign uniformly via one call
site). Named, not silent, bypasses: import bundle, status_transitions
backfill, workspace restore/purge — none are live human-facing mutations.
Known Phase-1 tradeoff, flagged not fixed: bulk mutations are NOT batched
into one notification the way the existing SSE/webhook bulk path is — a
bulk-assign of N items surfaces N individual notifications. Each is still
correctly scoped by the recipient's own watches/addressed-to-you filter
(a narrower audience than the workspace-wide SSE firehose the existing
batching protects), so this is a noise-discipline tradeoff, not a leak.
* feat(cli): pad watch + pad session register (TASK-2533)
pad watch <ref> [--until field=value] creates/upserts a durable watch;
pad watch list / pad watch remove <ref> are the hygiene companions the
dispatcher asked to be included explicitly rather than silently added.
pad watch --stream --for-session is the plugin-monitor command: one
stdout line per matching event ("PAD TASK-214 -> kind (actor): summary"),
silent on startup with no .pad.toml (hourly retry) or an unreachable padd
(backoff retry) per DOC-2479's noise-discipline contract. The retry/
backoff math and line formatting are pure, unit-tested functions; the
actual sleep loop is not (per the dispatcher's ask).
pad session register writes ~/.pad/sessions/<pid>.json (pid, cwd,
CLAUDE_CODE_MESSAGING_SOCKET when set) -- forward-looking infra for
Phase 3's live-sessions/presence surface; nothing consumes it yet in
Phase 1/2.
* fix(server): comment replies never published a watch notification (TASK-2533)
Codex round 1 finding 2 (verified real, not a false positive):
handleCreateReply is a SEPARATE code path from handleCreateComment — it
calls store.CreateComment directly via POST .../comments/{id}/replies,
not POST .../comments — and was missing the watch-notification hook
entirely. A reply to a comment on a watched item produced zero
notification. Same kind=comment publish as the top-level path, plus a
regression test covering the reply route specifically.
* fix(server): re-check current access before serving/delivering watches (TASK-2533)
Codex round 1 finding 1: ListWatchesForUser filtered only by user_id — a
watch row survives a revoked workspace membership or grant (nothing
deletes it), so GET /api/v1/watches and the event-stream's notification
filter could keep leaking item title/ref, workspace slug, actor, and
summary for access the caller no longer has.
Adds Store.ListWatchesForUser's ItemCollectionID column (needed for the
visibility check) and server.filterWatchesByCurrentAccess, which mirrors
computeSSEVisibility's RBAC resolution (handlers_events.go) — admin
bypass, VisibleCollectionIDs for member/guest full-collection access,
GuestVisibleResources for item-level grants — grouped by workspace since
a caller's watches can span many, unlike a single SSE connection scoped
to one. Fails closed on any lookup error.
Wired into handleListWatches here; the event-stream's loadWatchPredicates
call site picks up the same filter in the next commit, which also
restructures that function's Subscribe/replay sequence and therefore
touches the same lines.
* fix(watchevents): atomic ID assignment + subscribe-and-replay (TASK-2533)
Codex round 1, findings 3 and 4 (same subsystem, fixed together):
Finding 4 — sequence assignment and replay-buffer insertion happened
under SEPARATE locks in MemoryBus.Publish. Two concurrent Publish calls
could append to the ring buffer out of ID order, corrupting since()'s
ordering assumptions (it walks the ring oldest→newest assuming monotonic
IDs). Fixed by unifying seq assignment, buffer append, and the
subscriber-list snapshot under one lock; the (already non-blocking)
fan-out send still happens after releasing it.
Finding 3 — GET /api/v1/events/stream called Subscribe() and, later
(when resuming via Last-Event-ID), EventsSince() as two separate calls.
A Notification published in the window between them landed in BOTH the
replay result and the live channel, double-delivering it. Bus gains
SubscribeAndReplaySince(sinceID), which atomically subscribes and reads
the replay buffer under the SAME lock; the stream handler now uses it
whenever a Last-Event-ID is present (this commit carries that call-site
change, plus the finding-1 loadWatchPredicates filter wiring from the
previous commit — both land in the same lines of this function).
Adds a concurrent-publish ID-ordering test and a subscribe-then-
concurrent-publish no-duplicate test, both run with -race.
* fix(cli): monitor silent-start ordering + sync_required handling (TASK-2533)
Codex round 1, findings 5 and 6:
Finding 5 (P1) — runWatchMonitor called getClient() once, before the
loop and before the .pad.toml check. getClient() -> getConfiguredConfig()
os.Exit(1)s when unconfigured with no TTY, or launches an INTERACTIVE
configuration wizard when one is attached — either way a direct violation
of DOC-2479's silent-start contract, which requires "not ready yet" to be
a silent retry, never a crash or a prompt. Adds monitorClient(), which
builds the client the same way but returns a plain error instead of
exiting or prompting; client construction now happens INSIDE the loop,
after the .pad.toml gate, on every iteration, and its failure folds into
the existing padd-unreachable backoff path.
Finding 6 (P2) — streamWatchEvents ignored "sync_required" (the server's
signal that the requested Last-Event-ID was evicted from its replay
buffer), so a stale cursor got resent on every reconnect forever. Now
clears the cursor on sync_required so the next reconnect is a fresh,
non-resuming subscription instead.
Both covered by tests that assert the goroutine returns promptly on
context cancellation (proving no os.Exit / no blocking prompt was hit,
since the test process itself is still running to observe the return)
and that streamWatchEvents clears/re-tracks the cursor correctly around
sync_required.
* fix(server): uniform current-access gate for watch AND addressed-to-you delivery (TASK-2533)
Codex round 2, findings 1 and 2 — same subsystem (watch/nudge delivery
access control), fixed together; finding 2 explicitly falsifies finding
1's fix's own admin-bypass argument, so this replaces that reasoning
rather than patching around it.
Finding 1 (confirmed real): VisibleCollectionIDs / GuestVisibleCollectionIDs
deliberately over-widen for navigation — a collection ID is included if the
caller has an item grant on ANY item inside it, explicitly leaving
item-level narrowing to the caller (their own doc comments say so).
computeWatchAccessVisibility used that over-wide set directly as the
"fully visible" gate, so a guest granted item A was treated as having full
access to A's WHOLE collection, including an ungranted sibling item B.
Fixed by building the "genuinely full access" set from
GuestVisibleResources' fullCollectionIDs (populated only from direct
collection_grants, never widened by an item grant) + GetMemberCollectionAccess
/ ListSystemCollectionIDs for an actual member — exactly computeSSEVisibility's
own fullCollSet construction, not an approximation of it.
Finding 2 (confirmed real): the addressed-to-you (KindAssignment) branch in
watchNotificationVisible returned true unconditionally, with NO access
check. validateAssignmentScope (internal/store/items.go) only checks
WORKSPACE membership, never collection access, so an item can be assigned
to a "specific"-access member whose granted collections don't include it
at all — an ordinary assignment, no revocation timing required. Fixed by
gating EVERY notification kind — watch-matched and addressed-to-you alike —
through the SAME watchAccessVisibility check before either branch runs.
watchevents.Notification gains CollectionID so the check has what it needs
without a second lookup; the stream handler resolves it lazily per
workspace via a small connection-scoped cache (workspaces aren't known in
advance for addressed-to-you the way watch workspaces are), cleared on the
same reval tick that reloads the watches map.
This also required replacing computeWatchAccessVisibility's admin-bypass
argument, not just its code: "every call site filters the caller's OWN
watches" stopped being a sufficient justification once addressed-to-you
(which is fundamentally about *this* caller's own assignment activity
across every workspace) shares the same gate — a bearer-borne admin token
unconditionally trusted for that is exactly BUG-1616's blast radius. Now
mirrors computeSSEVisibility's cookie-vs-bearer distinction exactly.
Tests: guest-with-item-grant no longer sees a sibling item's watch or
stream notification (filter-level and HTTP/SSE-level); an assignment
outside a restricted member's granted collections is denied at both
levels; addressed-to-you is proven still gated (denied with no access,
visible once granted) as a pure unit test.
* fix(store): always re-read existing under lock, not just for precheck/patch updates (TASK-2533)
Codex round 2 finding 4, verified real: updateItemWithParentLinkOnce's
`existing` snapshot was only refreshed under the write lock when precheck
!= nil, ExpectedUpdatedAt != "", or FieldsPatch != nil — any update
touching none of those (e.g. a plain title-only PATCH) kept the STALE
pre-tx `existing` for the rest of the function, including the
LastMutation assignment-delta comparison added in TASK-2533's first
round. A concurrent OTHER transaction's assignment change landing between
this transaction's pre-tx read and its lock acquisition would get
misattributed to THIS transaction: a title-only update could report a
spurious, wrongly-attributed AssignmentChanged for a transition it never
made, duplicating the one the other transaction already reported
correctly (or missing a real one, depending on interleaving).
The status-transition capture already defended against exactly this with
its own separate conditional re-read; the assignment-delta capture added
later did not replicate that guard. Fixed by making the re-read
unconditional — once, right after the locks are held, before any SET-
clause building or the UPDATE itself — so every existing.* comparison in
this function is race-free by construction, not by each caller
remembering to guard itself. Also removes the now-redundant duplicate
re-read the status code had of its own.
Reproduces the exact race deterministically using UpdateItemWithPreCheck's
precheck hook as a synchronization point (TX2's assignment change blocks
mid-transaction while TX1's title-only update races its own pre-tx read
against it) — the new test fails reliably against the pre-fix code and
passes reliably (including under -race, and in Postgres mode) against
the fix.
* fix(watchevents): send under the same lock Unsubscribe/Close use (TASK-2533)
Codex round 2 finding 3, confirmed real and high-severity: Publish
snapshotted subscriber channels under the lock, released it, and only
then sent to them. A concurrent Unsubscribe or Close could close one of
those channels in the window between the snapshot and the send — a send
on a closed channel PANICS in Go, which crashes the whole padd process,
not just one subscriber's connection. The reasoning for releasing the
lock before sending ("a slow subscriber would stall everyone else") didn't
hold up: the send is already non-blocking (select/default — a full
channel is dropped-and-logged, never awaited), so holding the lock
through it costs nothing and closes the window structurally.
Adds a hammer test (many iterations of concurrent Publish / Subscribe /
Unsubscribe / Close, short-lived churned channels, recover()-wrapped so a
regression fails cleanly instead of crashing the whole `go test` run) that
reproduces "send on closed channel" dozens of times per run against the
pre-fix code (plus an independent -race detection) and passes cleanly,
repeatedly, against the fix.
* fix(server): re-fetch the user, not just the vis map, on each reval tick (TASK-2533)
Codex round 3, confirmed real: watchVisCache captured *models.User ONCE
at connect time (newWatchVisCache) and never re-fetched it; reset()
cleared only the per-workspace visibility map. computeSSEVisibility's own
doc comment explains why it re-fetches the user fresh on every call —
"so mid-stream role changes (admin demotion, user.disabled flips) take
effect on the next tick" — and the round-2 commit claimed to mirror that
"exactly," but only carried over the collection/bearer logic, not the
re-fetch itself. Net effect: a demoted or disabled admin kept fullAccess
on an open stream (both watch-matched and addressed-to-you delivery,
since both go through this same cache) until reconnect.
Adds watchVisCache.refreshUser, called by both the constructor and
reset() so the cadence matches computeSSEVisibility's actual cadence in
handlers_events.go (that function is invoked once at connect and again
only on each membershipCheck tick — never per event — so "per cache
reset" here is the same cadence, not a narrower one). Deliberately fails
CLOSED (not open-to-stale like computeSSEVisibility's own transient-error
fallback) on a fetch error, a deleted user, or a disabled user — a nudge
stream's wrong failure mode is delivering a fact to someone who
shouldn't see it, not a dropped UI update, so this trades
computeSSEVisibility's availability-leaning fallback for a stricter one
and says so in the comment rather than repeating the "mirrors exactly"
claim the fix falsified.
Tests: a unit-level pair (mirroring handlers_events_revalidation_test.go's
existing admin-demotion/disable coverage of the analogous SSE gap
exactly) proves an admin loses fullAccess after a demotion + reset(),
and a disabled user is denied outright; an HTTP/SSE-level test proves a
live stream stops delivering entirely once its connected user is
disabled and a reval tick passes. All three reproduce the bug reliably
against the pre-fix code and pass cleanly against the fix.
The HTTP-level test deliberately runs serially (not t.Parallel()): it
mutates the package-level watchListRevalInterval var, which every other
parallel watch-stream test in this package also reads via its own
ticker — writing to it from a t.Parallel() test raced against those
reads under -race (misattributed by the race detector to a whole
cluster of unrelated concurrently-running tests before this was
diagnosed). Full server package -race pass is clean after the fix.
* fix(server): decouple vis-cache reset from watch-list reload success (TASK-2533)
Codex round 4, confirmed real: on a reval tick, if ListWatchesForUser
errored, the handler's `continue` skipped visCache.reset() entirely —
the two were coupled, with reset() only reachable on the reload's
success path. A demoted or disabled user's stale identity/visibility
(round 3's fix) stayed live for exactly as long as that UNRELATED query
kept failing, so the round-3 leak reopens for the duration of any
watch-list reload error.
Fixed by running visCache.reset() first, unconditionally, before
attempting the watch-list reload. On a reload failure, the stale watch
list is kept (its own staleness is already bounded by
watchListRevalInterval's "eventually consistent" contract) but is now
gated by the FRESH visCache regardless — a demoted/disabled user is
denied via visCache even while the watch list itself lags a tick.
Chose this over dropping all delivery for the tick (the other option the
finding offered) because tying stream availability to an unrelated
query's transient health seemed like the wrong tradeoff; the comment at
the call site states this choice explicitly.
Adds a watchPredicatesLoadFault test seam on *Server (mirrors the
existing restoreAckFault pattern) so the reload failure can be forced
deterministically without breaking the DB connection for the whole test.
Reproduces the exact bug: forces the reload to fail on every tick while
concurrently disabling the connected user, and asserts addressed-to-you
delivery (which depends only on visCache, never the watch list) is
denied anyway. Fails reliably against a reverted (pre-fix, coupled)
version of the reval branch and passes cleanly against the fix.
Full server package -race pass, full suite (SQLite + Postgres) pass,
lint clean — this is the pre-PR verification matrix; round 5 will be a
narrow re-verify of this fix only.
* fix(server): bound stale watch set under persistent reload failure; atomic test seam (TASK-2533)
Codex round 5, two P2s, both confirmed real:
Finding 1 — `watches = fresh` only ran on the reload's success path, so
under a PERSISTENT (not single-tick) reload failure the watch set stayed
live indefinitely: a dead watch (removed, item deleted) kept matching
forever, and a watch created during the outage was silently missed
forever — visCache (round 4) gates current ACCESS, not whether a watch
still legitimately exists, so it couldn't catch this on its own. Fixed
by tracking consecutive reload failures and clearing the watch set once
maxConsecutiveWatchReloadFailures (3 ticks) is crossed, failing closed
on watch-matched delivery specifically while addressed-to-you delivery
(visCache-only, unaffected either way) continues throughout. Updated the
tradeoff comment at the call site so the "eventually-consistent" claim
now matches the bounded, not unbounded, behavior it actually describes.
Finding 2 — the watchPredicatesLoadFault test seam was a plain `func()
error` field, written by a test AFTER the SSE stream's background
goroutine was already running and reading it on every reval tick:
genuinely racy, unlike restoreAckFault's own use of the identical field
shape, which is set once, synchronously, before the single HTTP request
that reads it — goroutine creation's happens-before edge makes THAT
usage safe without any extra synchronization. Verified restoreAckFault
does not share the flaw and left it untouched. Fixed the watch seam with
atomic.Pointer[func() error] instead.
Test for finding 1: forces maxConsecutiveWatchReloadFailures+1
consecutive reload failures via the (now-atomic) fault seam and asserts
watch-matched delivery is suppressed once the bound is crossed while
addressed-to-you keeps delivering, then clears the fault and confirms
watch-matched delivery resumes on the next successful reload — a bounded
outage response, not a one-way ratchet. Fails reliably against the
bound disabled, passes cleanly restored.
This is the (re-run) pre-PR verification matrix per the dispatcher:
SQLite + Postgres + full-suite -race + lint + gofmt, all clean. Round 6
is a narrow re-verify of these two fixes only.
* test(store): bound the concurrent mutation-signal test's wait (TASK-2533)
CI-triage follow-up: PR #1082's plain Postgres step hit go test's default
10-minute per-binary timeout. Investigated whether any store test added by
this branch scales with runner slowness (lock-wait defaults, sleep-based
polling, transaction-hold durations):
- Watches CRUD tests (8): 0.63-0.80s each under Postgres, isolated and in
the full 741-test package run.
- Mutation-signal tests (6), including the precheck-hook two-transaction
race test: 0.48-0.80s each; the race test held at 0.48-0.51s across 10
consecutive runs (no variance) and across the full-package run.
- Full store package under Postgres: 279.17s and 277.12s across two runs
on this branch, matching the ~275s/297s baseline team-lead measured
locally and on PR #1081 — no reproducible slowdown from anything this
branch adds.
No pathological test found locally. The one test with genuine
cross-goroutine DB lock contention (TestLastMutation_AssignmentDelta_
NotMisattributedUnderConcurrentWrite) had an unbounded wg.Wait() as its
only unbounded wait — TX2's release was already unconditional (fixed 50ms
sleep, not gated on TX1's progress), so there's no deadlock risk, but
there was no ceiling on how long legitimate lock contention could
stretch it under a slow/shared runner. Replaced with a bounded 10s wait
that fails fast with a diagnostic instead of silently consuming
test-binary budget if it's ever exceeded. Verified the regression test
still fails reliably (5/5) against a revert of the round-2 fix it guards.
Could not reproduce the CI timeout locally; likely the pre-existing
~297s CI baseline (already noted as close to the 10-minute ceiling)
plus environmental variance on the shared runner, not a specific test
this branch adds.
|
||
|
Dan Finn
|
20b061902b |
fix(cli): surface actionable errors for cloud-mode setup failures
When a user picks Cloud mode during `pad init` but doesn't have a Pad Cloud account, the CLI hits the cloud server and surfaces raw server errors like "Missing CSRF token" — which is an implementation detail that gives no indication of what went wrong or how to fix it. This patch: - Splits the ModeCloud and ModeRemote branches in printSetupRequiredHint so cloud users see "sign up or switch to local" instead of the generic "run pad auth setup on the server" message. - Returns a cloud-specific error from pad init when setup_required is true in cloud mode. - Intercepts csrf_error responses in the CLI HTTP client and replaces the raw server message with an actionable "run pad auth login" message, since the CLI never sends CSRF cookies and this error always indicates a stale or mismatched session. |
||
|
xarmian
|
1e48a7a1dd |
feat(cli): add pad item copy for cross-workspace copy and move (TASK-2366)
Wraps PLAN-2357's two endpoints behind one command:
pad item copy <ref> --to-workspace <slug> --collection <slug>
[--dry-run] [--archive-source] [--field key=value ...]
--dry-run renders the preflight's three contract buckets (carried /
dropped / needs_value) and DR-15's full warning set. Every bucket header
and every warning line prints unconditionally, zeros and empties
included: omitting a zero would make "no attachments" indistinguishable
from "this CLI does not report attachments", and DR-17's whole point is
that none of it is silent. Schema-supplied strings are escaped and list
members quoted, so a comma or newline in an option value cannot forge an
entry or a row.
--format json emits the endpoint's own response. json.Indent is a lexical
transform, so key order, unmodelled fields and int64 precision all
survive; the bytes are never round-tripped through a Go value.
DR-13, the no-retry obligation. There is no idempotency key, so a blind
re-run duplicates the item. Four mechanisms, each with a test:
1. the mutating copy runs on its own *http.Client AND its own
transport. The transport half is the one that matters: retry in Go
is almost always a RoundTripper wrapper, which a merely-dedicated
http.Client would inherit. A plain *http.Transport is cloned so
proxy/TLS config carries; a wrapper is not used at all;
2. its body is hidden behind an opaque reader, leaving Request.GetBody
nil so net/http's own nothing-written replay cannot fire;
3. redirects are refused rather than followed with the POST body;
4. failures are classified into three exclusive outcomes, because each
licenses a different thing to say. UNKNOWN (transport failure, 500
copy_failed) sends the user to check the destination and never
suggests a retry. COMMITTED-BUT-UNREPORTED (a 2xx whose body could
not be read or decoded) exits ZERO -- a non-zero exit would tell a
script the copy did not happen, which is the DR-13 duplicate
arrived at through the reporting layer. A 4xx is a refusal made
before any write and passes through plainly.
The same asymmetry governs stdout: a write failure on the dry run is an
error (nothing happened), while a write failure after the copy committed
goes to stderr and leaves the exit code at 0.
Refuse to guess. The preflight always runs first (it is read-only), and a
non-empty needs_value refuses before any mutating request, naming each
field and the exact --field flags to add. Mirrors the web dialog's
disabled confirm rather than round-tripping the user into an error they
could have been shown.
--field values are typed against the DESTINATION collection's schema, so
a number lands as a number. A malformed --field is a hard error here
rather than the silent skip `pad item create` does: this command's
contract is "you were told what to supply", and dropping a supplied value
would make the refusal a lie.
The response types in internal/cli mirror internal/server's. That is a
layering choice, not a cycle -- nothing in server imports cli, and the
mirror test imports server freely. It follows the posture already
recorded in internal/cli/bootstrap.go: this package is the HTTP client
and does not depend on the server package. An external cli_test package
walks both response shapes and fails on any JSON contract drift.
MCP is deliberately untouched: no pad_item.action: copy, and
ToolSurfaceVersion stays 0.15.
|
||
|
Yui(ゆい)
|
9f4704a31f | fix(cli): wrap comment not-found errors (#910) | ||
|
xarmian
|
c62658a11b |
fix(cli): root usage/error/format hygiene (TASK-2031, BUG-2032) (#893)
* fix(cli): silence usage on runtime errors, echo not-found input, validate --format TASK-2031 + BUG-2032 (PLAN-1985). SilenceUsage + FlagErrorFunc keeps flag-error help; GetItem/UpdateItem/DeleteItem wrap not_found with ref+workspace; PersistentPreRunE rejects invalid --format; honest markdown advertising. * fix(cli): return enriched *APIError for not_found to preserve concrete type Team-lead P2: itemNotFoundError changed the concrete error type, so direct err.(*cli.APIError) assertions (which do not unwrap) stopped matching not_found — notably bulk-update's per-row code capture (cmd_item.go:2043), dropping code:"not_found" from the JSON envelope. Return a fresh *APIError (same Code/Details, enriched Message) instead of a wrapper type; APIError.Error() returns Message so the clean one-line message is unchanged. Both err.(*APIError) and errors.As now match. Test adds a direct-assertion + Details-passthrough lock-in. |
||
|
xarmian
|
bed933d7fd |
feat(items): field-level PATCH + conflict envelope + read-only version history (TASK-2022) (#876)
* feat(items): field-level PATCH + conflict envelope + version history Adds three related item-update primitives (TASK-2022 / IDEA-1480): - Field-level merge: PATCH `fields_patch` shallow-merges onto the item's current fields INSIDE the write transaction (null deletes a key), so concurrent single-field updates no longer clobber each other via the full-blob read-modify-write. `pad item update` and the MCP `pad_item.update` action now send only the changed keys. - Optimistic concurrency: optional `expected_updated_at` on update; on mismatch the store returns *UpdateConflictError and the handler emits the pad-structured-error/v1 conflict envelope (HTTP 409, code=update_conflict). Surfaced on CLI (`--expected-updated-at`) and MCP (`expected_updated_at`). - Read-only version history: `pad item history <ref>` (alias `versions`) and MCP `pad_item.history`, reusing the existing item_versions store + versions endpoint (no new store, no schema change). MCP ToolSurfaceVersion bumped 0.9 -> 1.0 (new action + param; update behavior change). No migration required. Claude-Session: https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS * fix(items): address Codex review — dispatcher fields_patch, OCC ordering, date/required guards Round 1+2 review fixes for TASK-2022: - HTTP MCP dispatcher (dispatch_http_advanced.go) now sends fields_patch (only changed keys) instead of a client-side merged full fields blob, and forwards expected_updated_at — remote MCP callers get the same race-free merge + optimistic concurrency the CLI/HTTP paths do. - ValidatePartialFields rejects null-deleting a schema-declared REQUIRED field (would otherwise persist a blob the full-update validator rejects). - Open-children guard on the fields_patch path merges the patch onto the IN-TX locked row inside the precheck (not a stale pre-lock preview), so a priority-only patch can't false-fire the guard. - Optimistic-concurrency check now runs BEFORE the open-children precheck in the store, so a stale expected_updated_at yields update_conflict (not open_children) — single in-tx re-read shared by both. - Date auto-population on the patch path only fills an EMPTY current date; an existing end_date the caller isn't touching is preserved. Tests added for each fix. Claude-Session: https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS |
||
|
xarmian
|
c127a5f965 |
fix(playbooks): enforce draft gate server-side + expose status (BUG-2020) (#874)
* fix(playbooks): enforce draft gate server-side + expose status (BUG-2020)
pad_playbook run / POST /playbooks/{ref}/run now refuse a playbook whose
status isn't "active" with a structured playbook_not_active error. Adds
an allow_draft escape hatch across all surfaces: JSON body field, CLI
--allow-draft flag, MCP boolean param, and an "allow-draft" bareword in
raw_args (stripped before strict parsing). status is now echoed on both
the run and get responses.
Bumps ToolSurfaceVersion 0.9 -> 0.10 and updates the drift-guarded docs
(instructions.md, README.md) plus CLAUDE.md.
Claude-Session: https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS
* fix(playbooks): forward allow_draft on WebMCP + refresh mcp serve help
Codex review follow-up for BUG-2020:
- WebMCP dispatcher + api client now forward allow_draft so the browser
surface can use the draft-gate escape hatch the catalog advertises.
- `pad mcp serve --help` refreshed from the stale "v0.4 / eight tools"
text to the current v0.10 / nine-tool surface (incl. pad_library).
Claude-Session: https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS
* fix(playbooks): type playbooks.get() with top-level status (BUG-2020)
Codex P3 follow-up: the get response now returns Item & { status }.
Claude-Session: https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS
|
||
|
xarmian
|
f5b437a65f |
feat(server): workspace restore + deleted-list endpoints (TASK-1970) (#827)
Foundation for PLAN-1969 (user-recoverable workspace soft-delete). A
workspace delete only stamps workspaces.deleted_at; items/collections/
members are untouched, hidden transitively. Restore clears deleted_at so
everything re-surfaces intact.
Store (internal/store/workspaces.go):
- RestoreWorkspace(slug): UPDATE ... SET deleted_at = NULL WHERE slug=?
AND deleted_at IS NOT NULL. Returns sql.ErrNoRows (-> 404) when no
soft-deleted row matched (already live or purged).
- ListDeletedWorkspaces(userID, cutoff): owner-scoped, deleted_at within
the window, ordered deleted_at DESC. Account-deleted workspaces have no
live owner, so they never leak.
- GetDeletedWorkspaceBySlug(slug): resolves a soft-deleted row (the normal
resolvers filter deleted_at IS NULL) so the handler can tell 403 from 404.
- Dual-dialect via s.q/s.dialect; no migration (deleted_at already exists).
Handlers (internal/server/handlers_workspaces.go):
- POST /api/v1/workspaces/{slug}/restore: owner-only; 404 not-restorable,
403 non-owner, 200 + restored workspace; logs a "restored" activity.
- GET /api/v1/workspaces/deleted: owner-scoped list with per-entry
purge_at + days_left, both derived from workspacePurgeRetention so
restore and the purge sweeper share ONE 30-day window (no drift).
- Both routed outside the /{slug} RequireWorkspaceAccess subrouter (which
resolves only live workspaces); restore enforces owner authz inline.
CLI client (internal/cli/client.go): RestoreWorkspace + ListDeletedWorkspaces.
TS type (web/src/lib/types/index.ts): Workspace.deleted_at + DeletedWorkspace.
Tests: store (resurface-intact; double-restore/live -> ErrNoRows; window
boundary 29d IN / 31d OUT + owner-scoping) and handler (owner-only 403,
404 live/unknown, 200 restore, owner-scoped deleted-list). Green on
SQLite and Postgres (make test-pg); golangci-lint clean.
Closes TASK-1970
Claude-Session: https://claude.ai/code/session_01HxBkAMiFBtCRJ2tKSCt3ST
|
||
|
xarmian
|
665f1918a7 |
feat(cli): headless admin bootstrap via --email/--name/--password (BUG-988) (#761)
* feat(cli): headless admin bootstrap via --email/--name/--password (BUG-988) Adds non-interactive flags to `pad auth setup` and `pad init` so agents running inside Claude Code or other non-TTY environments can bootstrap a fresh Pad instance without hitting interactive prompts that block forever. - New flags --email, --name, --password on both commands; all three must be supplied together when any one is present (clear error naming the missing flag otherwise). Checked before the remote-mode guard so the headless path works on any server host — the loopback gate is enforced server-side. - runHeadlessSetup() drives the existing POST /api/v1/auth/bootstrap endpoint directly, saves credentials, and respects --format json (emits a LoginResponse-shaped object with user + token). Already-initialized conflict produces a structured JSON error object under --format json. - `pad init` slots headless bootstrap into the bootstrap step only; the rest of init (config, workspace creation, skill install) continues. - Hardens readPassword() with an early non-TTY guard (generic message). - Hardens promptAndBootstrap() with a bootstrap-specific non-TTY guard (points at --email/--name/--password flags) so --cli-prompt on a pipe exits immediately rather than blocking. - Extends `pad init` non-TTY error message to mention the new flags. - Five new tests in cmd/pad/setup_headless_test.go covering success, missing-flag validation, non-TTY guard, already-initialized conflict, and the full init flow including workspace creation. NOTE: --password is visible in process listings (inherent to flag-based injection). Env-var bootstrap (PAD_ADMIN_*) is the tracked follow-up. Claude-Session: https://claude.ai/code/session_01WK9cUjxniBBAihGD5ygjDr * fix(cli): thread bootstrap token, factor shared core, restore readPassword fallback Round-1 codex findings: 1. Bootstrap token not sent on headless path (BLOCKER) Add BootstrapWithToken(email, name, password, token) to cli.Client that sets X-Bootstrap-Token when token is non-empty. Export ReadBootstrapToken from internal/cli/bootstrap.go (was readBootstrapToken) so cmd/pad can call it. Extract doHeadlessBootstrap(cfg, client, email, name, password) as the shared core for both setupCmd and padInitCmd: reads the on-disk token best-effort (absent → empty → loopback gate still covers that case), calls BootstrapWithToken, saves credentials, sets auth token on client. Both headless paths now go through this single function — no divergence. 2. readPassword bufio fallback removed by accident (REGRESSION) Restore the pre-round-1 bufio fallback in readPassword so piped-password flows (e.g. pad auth login --interactive in CI) keep working. The bootstrap wedge is already prevented by the top-of-promptAndBootstrap TTY guard; the generic readPassword fallback is only reached by non-bootstrap callers. 3. Shared core (CLEANUP) padInitCmd now calls doHeadlessBootstrap instead of duplicating Bootstrap + saveCredentials + SetAuthToken. The --format json asymmetry (init vs setup) is resolved by design: pad init is a multi-step flow; for machine-readable bootstrap output agents should use `pad auth setup --email … --format json`. Documented in the inline comment on the headless branch in padInitCmd. Tests added: TestHeadlessSetupSendsBootstrapToken, TestHeadlessSetupNoTokenFileOK, TestReadPasswordFallback. Update internal/cli/bootstrap_test.go for the rename. Claude-Session: https://claude.ai/code/session_01WK9cUjxniBBAihGD5ygjDr * BUG-988 round-2: surface token-read errors, wrap 403 with hint, rescope readPassword test doHeadlessBootstrap: distinguish os.ErrNotExist (absent token → best-effort empty, proceed without header) from other read errors (permissions, etc. → surface with the file path so operators can diagnose rather than silently hitting a confusing 403). Wrap 403/forbidden from BootstrapWithToken with an actionable multi-bullet hint covering loopback gate, token-file path, and PAD_BYPASS_SETUP_TOKEN. ReadBootstrapToken (internal/cli/bootstrap.go): add %w to the ErrNotExist branch so errors.Is(err, os.ErrNotExist) propagates to callers; existing tests and --cli-prompt hint text preserved. TestReadPasswordFallback → TestReadPasswordBufioFallback: rescoped to assert readPassword isolation only; added comment citing BUG-1886 (pre-existing doInteractiveLogin double-bufio.Reader bug). BUG-1886 filed in docapp. |
||
|
xarmian
|
285a58e40e |
feat(artifact): pad item export/import CLI commands (#756)
* feat(artifact): pad item export/import CLI commands Phase 3 of PLAN-1867. - pad item export <ref> [-o file] — writes a playbook/convention as a portable <slug>.pad.md artifact (or stdout via -o -). - pad item import <file> — POSTs the artifact (or stdin via -), prints the new draft ref + slug and any server warnings (coerced fields, renamed slug). Adds ExportItemArtifact/ImportArtifact client methods. Implements TASK-1876, TASK-1877. Claude-Session: https://claude.ai/code/session_01KmxkPxLksjf1pmrZDpsnTJ * fix(artifact): harden CLI export file write Addresses Codex Phase-3 review: - filenameFromContentDisposition reduces to filepath.Base with safe fallbacks — a hostile Content-Disposition can't traverse/abs-write. - export writes atomically (temp + Sync + Rename) like attachment download, so a failed write can't truncate/leave a partial artifact. Claude-Session: https://claude.ai/code/session_01KmxkPxLksjf1pmrZDpsnTJ |
||
|
xarmian
|
248f7c5ede |
feat(items): expose item restore via CLI + MCP (TASK-1828) (#734)
Adds the agent-facing restore surface so an archived item discovered via
`pad item list --all` can be recovered without dropping to the web UI. The
server already had restore end-to-end (Store.RestoreItem + handleRestoreItem
at POST /items/{ref}/restore, used by the web UI and bulk ops); this wires
the two missing surfaces:
- CLI: `pad item restore <ref>` (cli.Client.RestoreItem → the existing
endpoint, which resolves the ref include-deleted server-side). Mirrors
`pad item delete`'s structured JSON envelope: {ref, title, restored: true}.
- MCP: pad_item action=restore via passThrough(["item","restore"]). Restore
is non-destructive, so it's safe to expose. The action auto-joins the
schema's action enum (derived from the Actions map) and is documented in
the tool description.
Conflict case (slug/invocation_slug reclaimed while archived) is already
handled by handleRestoreItem (409) and surfaced by the client's
handleResponse.
Tests: restore endpoint already covered (handlers_items_test.go); restore
added to the MCP catalog<->cmdhelp bijection + dispatch tests. Child of
BUG-1791 (TASK-1827 shipped in #733).
|
||
|
xarmian
|
1b1068537c |
feat(tags): workspace tag enumeration endpoint + cross-collection filter (TASK-1653) (#658)
* feat(tags): workspace tag enumeration endpoint + cross-collection filter (TASK-1653)
Foundation for the tags feature (PLAN-1652 / IDEA-1649). The write path and
per-collection ?tag= filter already existed; this adds tag enumeration and a
verified cross-collection read so a single tag can group items of any type.
- store: dialect.JSONArrayElements unnests a JSON text-array column
(json_each on SQLite, jsonb_array_elements_text on Postgres);
Store.ListWorkspaceTags returns distinct tags + item counts, ordered by
count desc then tag asc, with the same collection/item ACL filters as
ListItems so counts never leak hidden items.
- server: GET /workspaces/{ws}/tags (handleListTags), respecting collection
visibility + guest item grants.
- models: TagCount{tag,count}.
- cli: client.ListTags + `pad tag list`.
- web: api.tags.list + TagCount type (items.list already forwards `tag`).
- tests: store-level (cross-collection aggregation, collection scoping,
non-nil-empty = empty, archived excluded) and handler-level (a Task + an
Idea sharing one tag; GET /tags counts + ordering).
Parent: PLAN-1652.
* fix(tags): count distinct items per tag, not tag occurrences per Codex review (round 1)
COUNT(DISTINCT i.id) so an item with duplicate tags (e.g. ["ux","ux"]) is
counted once — the write path doesn't enforce per-item tag uniqueness.
Adds a regression test.
|
||
|
xarmian
|
949ae03c88 |
feat(cli,mcp): pad project report + pad_project report action (TASK-1635) (#641)
Expose the report aggregation (TASK-1630) to agents:
- CLI: `pad project report [--window day|week|2wk|month] [--collections a,b]`
fetches GET /workspaces/{ws}/report and renders a colored summary (totals,
per-bucket throughput, completed-by-collection, status distribution);
--format json prints the raw payload.
- client.GetReport HTTP method.
- MCP: pad_project gains action=report (passThrough to `project report`) with
window + collections params; catalog-readonly test stub + expected maps
updated.
Parent: PLAN-1628.
|
||
|
xarmian
|
342679a364 |
Standardize plan-limit error envelope across HTTP/MCP/CLI/UI (TASK-788) (#628)
* fix: limit-hit responses were actively broken — garbled toasts, no upgrade signal
The limit enforcement responses (plan_limit_exceeded on 403) used a flat
body shape {"error": "plan_limit_exceeded", ...} that is incompatible with
every consumer: the frontend PadApiError parser, the CLI parseError path,
and the MCP classifyHTTPStatusKind all expect {"error": {"code": ...,
"message": ...}}. As a result, hitting any of the 5 plan limits (items,
members, workspaces, api_tokens, webhooks) produced garbled toasts with
undefined message text and zero upgrade signal.
Fix:
- writePlanLimitError now emits the standard nested error envelope with a
human-readable message sentence and limit details in error.details.
- CLI parseError now correctly surfacing the message (net positive, no
code change needed).
- MCP classifyHTTPStatusKind: adds ErrPlanLimitExceeded to the taxonomy
and the allowedStructuredErrorCodes whitelist so 403 plan-limit errors
pass through with code + details rather than collapsing to
ErrPermissionDenied (TASK-788).
- Frontend: exports isPlanLimitError() type-guard and planLimitMessage()
formatter from client.ts; all 4 limit-hit write call sites (item create,
member invite, workspace create, token create) now branch on the code and
show an upgrade-signal message pointing at /console/billing.
- Test: updates handlers_workspace_cap_test.go to the new body shape; adds
TestPlanLimitError_ResponseShape covering members_per_workspace limit hit.
TASK-788
* fix(R1): cover MCP stdio path, 5 more item-create sites, polish message wording
Finding A — MCP stdio transport was missing plan-limit coverage:
- cli/client.go: add PlanLimitDetails struct, AsPlanLimit() helper, and
WritePlanLimitError() that emits the pad-structured-error/v1 marker so
the MCP stdio classifier can lift code + details instead of falling
through to ErrServerError.
- cmd/pad/main.go: wire the WritePlanLimitError branch into all three
CreateItem call sites (item create, convention activate, playbook activate).
- internal/mcp: add TestClassifyHTTPStatus_PlanLimitPreservesCodeAndDetails,
TestClassifyHTTPStatus_Generic403FallsToPermissionDenied,
TestClassifyExecError_PlanLimitMarkerLiftsStructuredPayload, and
TestClassifyExecError_PlanLimitWithoutMarkerFallsThrough.
Note: extractUpstreamErrorEnvelope already parses details (json.RawMessage
field) — the codex concern about it being silently empty was a false alarm;
no fix needed there.
Finding B — 5 more item-create entry points were unguarded:
- EditorBubbleMenu.svelte (inline wiki-link capture)
- Sidebar.svelte (quick-add)
- roles/+page.svelte (board new-item, was console.error only; adds toastStore)
- conventions/+page.svelte
- playbooks/+page.svelte (both create and duplicate paths)
B1/B2 polish — server message is now statement-of-fact only, no doubled
upgrade verb. planLimitMessage() drops "Upgrade to Pro to add more." (each
surface appends its own CTA). limitStr uses hyphenated adjective form
"3-member" / "10-item" (compound modifier before "limit").
TASK-788
* feat(task-788): extend MCP-stdio plan-limit coverage to workspace, invite, webhook
Wire WritePlanLimitError into three additional CLI command error paths so
the MCP stdio classifier surfaces ErrPlanLimitExceeded with details instead
of falling through to ErrServerError:
- workspaceCreateCmd: check before fmt.Errorf wraps the APIError
- inviteCmd: check before returning the raw error
- webhooksCreateCmd: check before returning the raw error
Add TestClassifyExecError_PlanLimitWorkspaceCreate to exercise the full
workspace-create stdio round-trip through classifyExecError, asserting
ErrPlanLimitExceeded code, feature="workspaces", limit, and upgrade_url.
Token create intentionally left bare (agents don't drive token creation).
|
||
|
xarmian
|
8e7d4040fd |
feat(backlinks): server-side reverse index for [[...]] (Phase 1) (#620)
* feat(backlinks): server-side reverse index for [[...]] wiki-links (Phase 1)
First phase of PLAN-1593. Today [[REF]] is parsed only at render time
on the client and there's no way to ask "who links to TASK-5?" without
a full-text scan. This change adds a materialized reverse index
(item_wiki_links) that's written every time an item's content changes
and exposes it via REST + CLI.
Phase 1 covers ref-form links only (`[[TASK-5]]` / `[[TASK-5|Display]]`).
Phase 2 (TASK-1595) will extend to titles + cross-workspace; Phase 3
(TASK-1596) adds the web UI panel + MCP action.
What lands here:
* Migrations 061 (SQLite) and 040 (Postgres) create item_wiki_links
with partial indexes on target_item_id, (target_workspace_id, target_ref),
and target_title — the schema accommodates all 5 wiki-link forms
up-front so Phase 2 doesn't ALTER.
* internal/links/extract.go is the canonical parser. It strips fenced
and inline code regions before extracting [[...]] occurrences, so
example refs in docs / code blocks don't pollute the index. Phase 1
emits only WikiLinkKindRef rows; title and workspace_ref kinds parse
successfully but are gated out until Phase 2.
* internal/store/wiki_links.go (replaceWikiLinks + GetBacklinks +
helpers) handles write-time bookkeeping and the read query. Resolution
to target_item_id happens at parse time inside the same transaction
as the items INSERT/UPDATE, so partial state never lands. Broken refs
(target_item_id IS NULL) intentionally persist — they feed a future
broken-links report.
* internal/store/wiki_links_backfill.go + cmd/pad/main.go hook the
idempotent backfill into server startup. Existing items get indexed
on first boot after the migration; subsequent boots are near-no-ops
via an EXISTS short-circuit.
* internal/store/items.go is amended in two places: tryCreateItem
always calls replaceWikiLinks (empty content → no-op DELETE), and
UpdateItemWithPreCheck re-parses whenever input.Content was supplied.
* internal/server/handlers_backlinks.go serves
`GET /api/v1/workspaces/{ws}/items/{itemSlug}/backlinks` with
visibility + guest-grant filtering on the source items.
* internal/cli/client.go adds GetBacklinks; cmd/pad/main.go adds the
`pad item backlinks <ref>` command (registered in groups.go).
Behavior decisions (per PLAN-1593):
- code blocks excluded (fenced + inline)
- self-links filtered at query time (kept in storage)
- repeated mentions stored as separate rows by position
- ordering: source updated_at DESC, position ASC
Tests:
- internal/links/extract_test.go: 26 sub-cases covering ref/title/
workspace-ref discrimination, code-block exclusion (fenced + inline +
unclosed fence), position-is-byte-offset (UTF-8 safety), and edge
inputs.
- internal/store/wiki_links_test.go: 8 integration tests covering the
create/update/delete/self-link/broken-ref/repeated/code-block
scenarios plus backfill idempotence.
All pass. `make check` clean (lint + go test + web build).
Refs: TASK-1594, PLAN-1593, IDEA-1577
* fix(backlinks): visibility-aware pagination + case-insensitive refs per Codex review (round 1)
Two fixes from Codex code review:
P1 — GetBacklinks now takes a visibleCollectionIDs []string argument
that's applied INSIDE the SQL WHERE clause. Previously the handler
fetched LIMIT raw rows and filtered visible ones in Go, so a
restricted user asking for limit=50 could receive an empty page even
when later visible backlinks existed. Pushing visibility into SQL
makes LIMIT/OFFSET count visible rows.
nil → no restriction (owners, editors, root tokens)
[] → see nothing (returns early, no SQL)
[..] → AND s.collection_id IN (?, ?, ...)
Item-level guest grants still apply post-fetch — they're rare enough
that the residual page shrink is acceptable and pushing them into SQL
would balloon the query.
P2 — refPattern now accepts mixed/lowercase refs and parseBody
canonicalizes the prefix to uppercase at the single chokepoint.
Previously the renderer accepted `[[task-5]]` as a real link (its
REF_PATTERN is case-insensitive) but the indexer's ^[A-Z]... pattern
silently dropped it — divergent parsing on the same input. Storage
shape is canonical uppercase so the (workspace, prefix, number)
lookup against collections.prefix (also uppercase) has one shape.
New helper: canonicalizeRef("task-5") → "TASK-5".
Regressions:
internal/links/extract_test.go
+ TestCanonicalizeRef — helper unit tests
+ TestExtractWikiLinks_RefVsTitleFallback updated to assert
mixed/lowercase parses-as-ref-and-uppercases
+ edge-case test renamed from "lowercase ref" to "number-led
not a ref" (lowercase IS a ref now per Codex P2)
internal/store/wiki_links_test.go
+ TestWikiLinks_MixedCaseRefIndexed — `[[task-5]]` produces a
backlink row whose target_ref is "TASK-5"
+ TestWikiLinks_VisibilityAwarePagination — three sub-cases:
nil → all 3, visible-only limit=2 → 2 visible rows (not 1 with
hidden one consuming a slot), empty → 0
All call sites updated (8 in tests + 1 in handler).
`make check` clean (lint + tests + web build).
Refs: TASK-1594, PLAN-1593
* fix(backlinks): SQL-level item-grant filter per Codex review (round 2)
Round 1 fixed pagination for collection-level visibility but Codex
round 2 correctly flagged the same class of bug at the item-grant
layer: `visibleCollectionIDs` returns the UNION (full grants ∪
collections containing granted items), and the handler then
filtered each row's item-level visibility in Go AFTER fetching —
letting hidden rows in a granted-item's collection consume LIMIT
slots.
The refactor moves the precise predicate into SQL. New shape:
type BacklinksVisibility struct {
Unrestricted bool // admin / full-access member
FullCollectionIDs []string // direct collection grants
GrantedItemIDs []string // item-level grants
}
// SQL predicate when Unrestricted=false:
// AND (s.collection_id IN (?...) OR s.id IN (?...))
This matches `guestResourceFilter` (which returns the precise
primitives), so the handler now passes them straight through and
drops the post-fetch filter loop entirely. Pagination is correct
for guests, restricted members, and unrestricted users alike.
New test:
TestWikiLinks_ItemGrantPagination — guest with item-grant on ONE
item in an otherwise-hidden collection sees exactly that one item;
hidden siblings in the same collection do NOT leak in, and limit=2
returns 1 row (not silently shrunken).
Other call sites updated:
- TestWikiLinks_VisibilityAwarePagination → uses
BacklinksVisibility{FullCollectionIDs: ...} and
BacklinksVisibility{} for the no-access case.
- 8 existing tests → BacklinksVisibility{Unrestricted: true}.
- handlers_backlinks.go → no longer calls visibleCollectionIDs;
uses guestResourceFilter exclusively and skips the Go-side filter.
Verification:
- make check clean
- All TestWikiLinks_* pass
Refs: TASK-1594, PLAN-1593
* fix(backlinks): scan EXISTS into bool not int for Postgres parity (Codex round 3)
`SELECT EXISTS(...)` returns boolean on Postgres but integer 0/1 on
SQLite. Scanning into `int` happened to work on SQLite (the modernc.org
driver coerces) but would fail on Postgres — silently disabling the
backfill short-circuit there and meaning upgraded Postgres installs
wouldn't populate backlinks for pre-existing content until each item
got edited.
Fix: scan into bool. Both database/sql drivers in use (modernc.org/
sqlite and lib/pq) coerce their native representation into Go's bool,
so this single shape works on both engines.
make check clean.
Refs: TASK-1594, PLAN-1593
* fix(backlinks): allow CommonMark 0-3 space indented fences per Codex (round 5)
Round 5 flagged two edge cases in the code-stripping pass:
1. Multi-backtick inline code (``see [[X]]``) — traced through the
parser; my permissive close-on-next-backtick logic already covers
it correctly (range = [opener-start, after-closer-run]). Added
a regression test to lock this in:
TestExtractWikiLinks_CodeBlocksExcluded /
"multi-backtick inline code excludes ref"
2. Indented fenced blocks — CommonMark allows 0-3 leading spaces of
indentation before a fence opener (4+ spaces makes it an indented
code block, a different construct). My fencedCodeRanges only
matched fences at column 0, so ` ```\n[[X]]\n```` ` would
render as code in the UI but leak a false backlink. Fixed both
fencedCodeRanges (opener) and findFenceCloser (closer) to skip
up to 3 leading spaces, with a hard cap at 4 (which would be
indented-code, not a fence). Regression test:
TestExtractWikiLinks_CodeBlocksExcluded /
"indented fenced block (CommonMark 0-3 spaces)"
Not addressed:
- Round-4 escape-body parity finding. extract.go mirrors
renderMarkdown's regex (web/src/lib/utils/markdown.ts:300), which
is the actual render-time link parser; wikiLinksToMarkdown's more
permissive escape grammar is editor-serializer-side and the
renderer can't even consume its escaped output. Indexing what the
user actually sees as a link is the correct invariant.
make check clean.
Refs: TASK-1594, PLAN-1593
* fix(backlinks): tilde fences + strict closer lines per Codex (round 6)
Two CommonMark conformance gaps in the code-block stripping pass:
1. Tilde-fenced code blocks (~~~) were ignored. marked() treats them
the same as backtick fences, so a [[REF]] inside a tilde block
would render as code in the UI but leak as a false backlink.
Fixed by parameterizing fenceChar across fencedCodeRanges and
findFenceCloser, with separate handling for the backtick-specific
"no backtick in info string" rule (CommonMark §4.5).
2. Closer-line strictness — CommonMark requires the closing fence
line to contain only the fence + optional trailing spaces. The
previous accept-any-fence-prefixed-line check would terminate
a still-open fence prematurely on a line like ```not-closed,
leaking later refs in the still-rendered code block.
Refs reside in 4 new sub-tests under TestExtractWikiLinks_CodeBlocksExcluded:
- tilde fence excludes refs inside
- tilde fence with language tag
- mixed fence types don't pair
- closer-line strictness — backticks plus other text is not a closer
- closer-line strictness — trailing spaces OK
make check clean.
Refs: TASK-1594, PLAN-1593
* fix(backlinks): inline code closer must match opener length per Codex (round 7)
CommonMark §6.1 requires an inline-code span opened with N backticks
to close on a run of EXACTLY N backticks. The previous "close on next
backtick run of any length" logic would prematurely end the excluded
range on a stray single backtick inside a ``...`` span, leaking any
[[REF]] in the latter half of the code text as a false backlink.
Concrete failure case:
``has ` inside [[X-1]] and more``
→ old: range [0, 7], [[X-1]] indexed (bug)
→ new: range [0, end-of-closer], [[X-1]] excluded (correct)
Fix: track the opener-run length and scan only for matching-length
closer runs. Wrong-length runs in between are code text.
Two new regression sub-cases under TestExtractWikiLinks_CodeBlocksExcluded:
- inline code closer matches opener length — the main case
- single-backtick span unaffected by adjacent multi-backtick run —
asserts the opposite direction (opener=1 doesn't close on ``)
Not addressed:
- Re-flagged round-4/round-7 escape-body parity finding. extract.go
intentionally mirrors renderMarkdown's regex (markdown.ts:300), not
wikiLinksToMarkdown's more permissive escape grammar (markdown.ts:461).
renderMarkdown is the actual link parser at display time; its regex
rejects escaped-`]` bodies, so any link with an escaped `]` in its
body is NOT shown as a clickable link in the UI. Indexing it would
produce phantom backlinks the user can't see. The wikiLinksToMarkdown
permissive grammar is paranoid serialization that the renderer can't
consume — that's a pre-existing inconsistency in the editor pipeline,
not a backlinks bug.
make check clean (lint + tests + web build).
Refs: TASK-1594, PLAN-1593
* fix(backlinks): rune-align snippet end-edge to keep UTF-8 valid (Codex round 8)
The previous snippetAround() trimmed `start` to a rune boundary (so
the leading edge of the snippet was always at a valid codepoint) but
left `end` as a raw +40-byte clamp. When that landed in the middle of
a multi-byte rune — common around emoji or accented text — the
resulting slice was invalid UTF-8 and the JSON encoder would emit
replacement characters in backlink snippets.
Fix: same forward-advance pattern at the end as at the start.
Continuation bytes (10xxxxxx) get skipped until we land on a leading
byte. Going forward keeps the snippet anchored slightly past the
match rather than slightly before it, which is a small UX win
(emoji or accented text right after the link survives intact).
Regression test:
TestWikiLinks_SnippetIsValidUTF8 — pads body with enough 4-byte
emoji on each side that the ±40-byte window cuts through one;
asserts utf8.ValidString on the resulting snippet.
make check clean (lint + tests + web build).
Refs: TASK-1594, PLAN-1593
* fix(backlinks): inline code spans cross newlines, break on blank lines (Codex round 9)
CommonMark §6.1: an inline-code span can cross single newlines but
terminates at a blank line (a line containing no chars or only
whitespace, which ends the enclosing paragraph). My previous scanner
broke at every newline, so multi-line spans like
`pre
[[INSIDE-1]]
post`
would treat the opener as unclosed and leak [[INSIDE-1]] as a false
backlink. Fixed by:
1. The newline branch in the closer scan now peeks ahead via the
new isBlankLineAt() helper. Same-paragraph newlines are
traversed; blank-line breaks terminate the span unmatched.
2. isBlankLineAt() treats any line with only space/tab as blank
(mirroring CommonMark's blank-line definition).
Three new regression sub-cases under TestExtractWikiLinks_CodeBlocksExcluded:
- inline code spans single newline (CommonMark §6.1)
- inline code breaks at blank line (paragraph boundary)
- inline code breaks at whitespace-only blank line
Trade-off: a truly-unclosed inline backtick now consumes from the
opener up to the next blank line instead of just the rest of the
line. False-positive on wiki-links in that span, but the surface
area is small (unclosed backticks are rare in published prose) and
matches the renderer's behavior.
make check clean.
Refs: TASK-1594, PLAN-1593
* fix(backlinks): accept escaped wiki-link bodies per editor grammar (Codex round 10)
After 3 rounds of disagreement, capitulating on the escape-body parity
finding. My position was technically correct for the CURRENT
renderMarkdown behavior (which uses [^\]]+ and can't parse escaped-
bracket bodies), but the editor's wikiLinksToMarkdown grammar at
markdown.ts:461 explicitly produces such bodies — making the
renderer's regex the inconsistent half of the pipeline, not mine.
Mirroring the editor's grammar in the extractor makes the index
forward-compatible: when the renderer eventually gets fixed, no
change here is needed. The cost is a few "phantom" rows in the
interim (indexed links the renderer doesn't currently display as
clickable), but those are harmless and aligned with author intent.
Changes:
- wikiLinkPattern now uses `\[\[((?:\\.|[^\]\\])+)\]\]` — mirrors
markdown.ts:461 verbatim.
- New splitOnUnescapedPipe() helper — scans for the first `|`
that isn't preceded by `\`. Mirrors splitWikiBody at
markdown.ts:664.
- New unescapeWikiBody() helper — undoes `\]`, `\|`, `\\` escapes
in display text and key. Mirrors unescapeWikiBody at markdown.ts:657.
- parseBody() now uses both helpers — split on unescaped `|`,
unescape both sides.
Regression coverage:
- TestExtractWikiLinks_EscapedBodyChars (5 sub-cases): escaped `]`,
escaped `|`, escaped `\`, non-escape backslash passes through,
Position still points at opening `[[` despite escapes.
- TestSplitOnUnescapedPipe + TestUnescapeWikiBody: direct unit
tests for the helpers (round-trip safety vs the editor's
escape/unescape pair).
make check clean.
Refs: TASK-1594, PLAN-1593
* fix(backlinks): preserve display text verbatim per Codex round 11 P3
The previous parseBody trimmed the display side of [[X|Display]] but
the WikiLinkRef.Display contract promises verbatim storage and the
renderer at markdown.ts doesn't trim either. Trimming would silently
diverge on padded display text like [[TASK-1| spaces ]] (renderer
keeps the spaces, extractor stripped them).
Fix: drop TrimSpace from the suffix half of the split. Keep trimming
the key/ref side because refPattern is anchored — a leading or
trailing space in the key would force the body to fall through to
the title kind even though the renderer resolves it as a ref.
Regression test:
TestExtractWikiLinks_EscapedBodyChars / "display text preserved
verbatim (no TrimSpace)"
make check clean.
Refs: TASK-1594, PLAN-1593
* fix(backlinks): distinguish empty display override from no-pipe per Codex round 12
[[REF|]] (explicit empty display) and [[REF]] (no display) are distinct
shapes in the editor: splitWikiBody returns displayOverride="" for the
former, null for the latter, and the renderer uses `displayOverride ??
title` (nullish coalescing, NOT empty-string fallback) so "" is
preserved. The previous extractor collapsed both into display_text=NULL,
violating verbatim-display preservation for the empty-string edge case.
Fix:
- WikiLinkRef gains a HasDisplay bool. parseBody sets HasDisplay=true
iff splitOnUnescapedPipe found a pipe; downstream uses HasDisplay
(not Display!="") to decide whether to persist the override.
- replaceWikiLinks in store: NullString.Valid is keyed off HasDisplay.
display_text='' for explicit empty, NULL for no override.
Regression coverage:
- internal/links/extract_test.go:
"explicit empty display override is distinguished from no pipe"
- internal/store/wiki_links_test.go:
TestWikiLinks_EmptyDisplayDistinct (two-source assert: NOT NULL
for [[REF|]], NULL for [[REF]])
make check clean.
Refs: TASK-1594, PLAN-1593
* fix(backlinks): pointer-typed DisplayText to preserve empty distinction over JSON (Codex round 13)
Round 12 added HasDisplay on the parser side and made the store
preserve display_text='' vs NULL on the DB row, but the wire model
collapsed the distinction at JSON-serialization time:
DisplayText string `json:"display_text,omitempty"`
`omitempty` drops empty strings, so [[REF|]] (empty override) and
[[REF]] (no override) serialized identically on the API and CLI JSON
output. The end-to-end goal of round 12 wasn't reached.
Fix: change DisplayText to *string. nil → no override (field omitted
from JSON via omitempty), pointer to "" → explicit empty override
(field present with empty value). The store's NullString.Valid drives
the assignment, so the SQL round-trip matches the JSON shape.
Knock-on: the CLI's `pad item backlinks` now dereferences the pointer
and prints both populated and empty overrides ("displayed as: ").
Regression coverage:
- TestWikiLinks_EmptyDisplayDistinct extended to assert
withBL.DisplayText is non-nil-pointing-at-"" and noBL.DisplayText
is nil after a GetBacklinks round-trip.
make check clean.
Refs: TASK-1594, PLAN-1593
|
||
|
xarmian
|
de1beb47a9 |
feat(cli): pad library get + list --full + server-side category filter (TASK-1562) (#613)
CLI layer for PLAN-1560 (`pad_library` MCP tool + matching CLI surface).
Wires the HTTP work landed in TASK-1561 through to the `pad library`
subcommands.
## `pad library list` changes
- `--category` is now a server-side filter (the old client-side
display-only skip-loop is dead and removed).
- New `--full` flag. Default JSON output for playbooks now returns the
`summary` field (first non-heading paragraph, ~240 char cap) instead
of the full `content`; `--full` opts back into full bodies for
callers that want to pipe everything.
- Table output gains a summary hint line under each playbook and a
`/pad <slug>` chip when an invocation slug is declared, so the
library becomes self-documenting as a discovery surface.
- `--type` now validates explicitly instead of silently producing an
empty list for unknown values.
## NEW `pad library get <title>`
Calls `GET /api/v1/library/entry?title=X` and renders either a
conventions card (title, category, trigger, surfaces, enforcement,
commands, body) or a playbooks card (title, category, trigger, scope,
invocation slug, argument count, body). Conventions-first precedence
matches `pad library activate`.
JSON output returns the full envelope.
404 errors return a clean `not found in library: "<title>"` message
with exit code 1.
## CLI client
- `GetConventionLibrary(category)` — pass category as a server-side
query param.
- `GetPlaybookLibrary(category, summary)` — same plus the summary
toggle; `summary=true` strips Content and returns Summary instead.
- NEW `GetLibraryEntry(title)` returning `*LibraryEntryResponse`.
- `LibraryPlaybook` gained an omitempty `Summary` field so a single
type round-trips both the legacy and summary shapes.
## Drive-by
Switched `/library/entry` 400/404 from a flat `{error: "..."}` body to
the canonical `writeError(code, message)` envelope used by the rest of
the API. The CLI's `parseError` now hands back a typed `APIError` that
`pad library get` pattern-matches on `Code=="not_found"` for the clean
404 message. Updated `TestLibraryEntry_MissingTitle` and `_NotFound`
to assert the new envelope.
## Verification
go build / go vet / go test ./... all green. golangci-lint clean on
cmd/pad/..., internal/cli/..., internal/server/.... End-to-end smoke
tests via the installed binary confirmed: list summary mode, list
--full, list --category filter, get convention card, get playbook
envelope, get 404 exit-1, --type validation.
Parent: PLAN-1560. Unblocks TASK-1563 (MCP catalog wiring).
|
||
|
xarmian
|
db87b47754 |
fix(cli): pin server URL in .pad.toml for remote workspaces (BUG-1535) (#595)
* fix(cli): pin server URL in .pad.toml for remote workspaces (BUG-1535) Two fixes: 1. Replace stale api.getpad.dev references with app.getpad.dev in the --url flag help, NewClientFromURL doc, and Config.URL doc. Also fix internal/mcp/dispatch_http.go's comment to use the canonical mcp.getpad.dev/mcp URL. 2. Persist the server URL into .pad.toml when linking a directory to a non-local workspace. WriteWorkspaceLink now takes a serverURL arg; pad init / workspace link / workspace switch pass cfg.BaseURL() when Mode != local. getConfig() reads .pad.toml's URL as an override above ~/.pad/config.toml and below the --url flag, so commands like `pad collection list` from a remote-linked directory hit the right server without --url on every call. Passing --url explicitly also promotes local → remote so the directory pin is written even when the existing global config has mode=local. * fix(cli): scope .pad.toml URL override to client paths per Codex review (round 1) Round 1 review flagged that applying the .pad.toml URL override inside getConfig() contaminates server/admin commands: pad server start would advertise the wrong PublicLinkBaseURL, and pad auth setup would refuse to run locally because Mode flipped to remote. Extract the override into applyPadTomlOverride() and call it only from client-API entry points — getConfiguredConfig() and the pad init client phase. Server/admin commands (pad server start/stop, pad auth setup, pad auth configure) keep using raw getConfig() and are unaffected. Also skip the override when --url was explicitly passed (LoadedFromFlags), so the flag retains unambiguous priority. * fix(cli): preserve .pad.toml URL on workspace link/switch per Codex review (round 2) Round 2 review noted workspace link / workspace switch reached the server via getClient() (override applied) but then wrote the new .pad.toml URL using a raw getConfig() — which would drop or miswrite the url field when relinking inside a remote-pinned directory whose global config is local. Reuse the cfg returned by getClient() for padTomlURLFor so the write matches the API client. |
||
|
xarmian
|
aec67e202e |
feat(mcp): workspace.create + workspace.claim actions + claim-code mechanics (TASK-1521) (#582)
Phase B for PLAN-1519. Adds two MCP actions so agents can bring a workspace into an OAuth connection without re-auth — the agent-first onboarding story IDEA-1517 §1 set out to fix. pad_workspace.action: create - New action on the shared catalog (stdio + cloud both pick it up). - POSTs to /api/v1/workspaces; handler auto-adds the new workspace to the calling OAuth connection's allow-list (added_by='agent-create') when the grant carries may_create_workspaces=true. Phase A wired the oauth_connection_workspaces table this writes to. PAT / CLI-session callers fall through silently (no request_id → no side effect). - Backed by a new non-interactive `pad workspace create <name>` Cobra command for the stdio MCP shell-out path. `pad workspace init` remains the guided human flow. pad_workspace.action: claim - New POST /api/v1/oauth/claim endpoint redeems a 6-digit stateless HMAC code minted from (user_id, workspace_id, 5-min time bucket) with a sliding 5–10 minute lifetime. Constant-time compare. Code format derived per IDEA-1517 §4. - Verifies workspace membership before code (privilege-escalation guard); uniform 404 envelope so the endpoint can't be used to probe existence vs. membership. - Side effect inserts a row in oauth_connection_workspaces with added_by='claim'. Idempotent — re-claiming returns 200 with already_added=true. - 412 connection_not_persisted when the OAuth grant predates Phase C (no oauth_connections row); 412 claim_disabled when the deployment hasn't wired SetClaimSecret. - `pad workspace claim <code> --workspace <slug>` Cobra command backs the stdio MCP shell-out. MCP server instructions - Appended IDEA-1517 §5 paragraph teaching agents the claim flow as a peer top-level section. Same string lands universally on every MCP handshake response (stdio + cloud both read instructions.md). Tests - 10 claim-code unit tests: determinism, zero-pad, length-prefix collision guard, current/previous bucket accept, aged-out reject, wrong-everything rejects, short-secret fails closed. - 7 handler tests: 412 when secret disabled, 400/404/401 vocabulary, PAT caller note path, 412 connection_not_persisted, idempotent insert. - 5 MCP-catalog tests: actions registered, schema params advertised, description mentions both actions, route mappers produce correct HTTP shape, routeTable carries the entries. - Bumped the existing read-only catalog bijection + fixture-input fixtures so the new actions resolve cleanly. Parent: PLAN-1519. |
||
|
xarmian
|
f76520f6e7 |
feat(cli,mcp): expose 'collection delete' via CLI and MCP catalog (TASK-1511) (#573)
* feat(cli,mcp): expose 'collection delete' via CLI and MCP catalog (TASK-1511)
Mirrors TASK-1510 (collection update). The HTTP handler at
handlers_collections.go::handleDeleteCollection already supported
DELETE on a collection (owner-only, soft-deletes the collection and
every item in it). Wires both agent-facing surfaces:
- internal/cli/client.go: new DeleteCollection client method.
- cmd/pad: new 'pad collection delete <slug>' Cobra subcommand
(no --force; the help text is the confirmation contract).
- internal/mcp/catalog_collection: 'delete' action passes through
to the CLI; tool description updated.
- internal/mcp/dispatch_http_routes: simple routeSpec entry for
DELETE /api/v1/workspaces/{workspace}/collections/{slug}. No
custom mapper needed — no body, no field coercion.
Pairs with TASK-1510 as the second adaptation primitive for the
/pad onboard playbook (TASK-1499): when the onboard interview
discovers a seeded collection that doesn't fit the project, the
agent now has a way to remove it before creating the right one.
Tests:
- TestRouteTable_CollectionDelete (route substitutes correctly)
- catalog_readonly bijection + liveCmdhelpDoc fake updated.
Parent: PLAN-1496.
* docs: correct collection delete contract per Codex review (round 1)
Two findings on PR #573 — both documentation, no code behavior change:
1. CLI Long help / Short blurb / MCP description claimed delete
"removes seeded collections" and the onboard use case targets
template-seeded collections. But store.DeleteCollection refuses
any collection where is_default=true, and every template seed is
is_default=true. The advertised use case wouldn't actually work.
Updated docs to clarify: delete is for USER-CREATED collections;
template seeds must be adapted via 'pad collection update'.
2. Both CLI help and MCP description claimed "AND every item in it"
gets archived. The store delete path only sets collections.deleted_at
and never touches items. The web UI hides them via the join, but
raw API queries still surface them. Updated docs to be honest:
items are NOT cascaded.
Captured the underlying behavior limitation as a follow-up: IDEA-1513
("Lift is_default restriction on collection delete or add a
cascade-items option") — surfaces options 1-4 for lifting the guard
plus the items-orphan issue.
Parent: PLAN-1496, addressing Codex round 1 on PR #573 / TASK-1511.
* docs: tighten collection delete contract per Codex review (round 2)
Three P3 documentation-drift findings:
1. internal/cli/client.go::DeleteCollection Go doc still said "and
all items in it" — missed it in round 1. Updated to describe the
actual behavior (collections.deleted_at only; items orphaned with
soft-deleted collection_id; is_default rejected).
2. CLI Long help and MCP description claimed restore is available
"via the API," but there is no restore endpoint and no
RestoreCollection client method. Recovery is database-backup only.
Both docs updated.
3. catalog_collection.go:33 slug ParamDef only mentioned action=update;
action=delete needs it too. And the headline description still
said "list, create, and update" — three actions when there are
now four. Both fixed.
Parent: PLAN-1496, addressing Codex round 2 on PR #573 / TASK-1511.
* docs: update pad_collection action lists in instructions.md + README (round 3)
Codex round 3 finding: two top-level reference docs still advertised
pad_collection as list/create only. internal/mcp/instructions.md is
embedded into the MCP initialize() handshake instructions — stale
guidance there means MCP clients miss update/delete entirely. README's
catalog table had the same drift.
Parent: PLAN-1496, Codex round 3 on PR #573 / TASK-1511.
|
||
|
xarmian
|
e59d3904c9 |
feat(server): refuse to mark item terminal while it has open children (IDEA-1494) (#571)
* feat(server): refuse to mark item terminal while it has open children (IDEA-1494)
Server-side guard inside handleUpdateItem that rejects a non-terminal →
terminal done-field transition when the item still has at least one
non-terminal child. Returns HTTP 409 with code=open_children plus a
structured details payload listing each blocking child's
{ref, title, status, collection_slug} so MCP-driven agents can
self-recover (ship the listed children, then retry) and the CLI can
render the same list verbatim.
Escape hatch: `--force` on `pad item update` / `pad item bulk-update`
and `force: true` on the MCP pad_item.action: update / bulk-update
inputs both forward into the same ItemUpdate.Force transport field
the handler consumes before any store mutation.
Trigger conditions are tight: the PATCH must change the done-field key
(resolved via TerminalValuesForDoneField against the parent's schema +
settings) AND the new value must be terminal AND the current value
must NOT already be terminal. Terminal → terminal and no-op terminal
transitions bypass the guard; only entering the terminal set is gated.
Per-child evaluation uses the child's own collection schema so
hierarchical workspaces with custom typed collections work without
extra plumbing.
Tests cover: rejection with one open child (with mutation-safety
assertion on the parent), no children, all-terminal children, --force
override, no-op terminal → terminal, terminal → terminal,
non-terminal → non-terminal, custom collection terminal_options
honored, and a parent task (not a plan) — IDEA-1494 optional extra #3.
MCP coverage asserts --force round-trips through both ExecDispatcher
and HTTPHandlerDispatcher and is omitted when force=false.
* fix(server): open-children guard round 2 — visibility, MCP pass-through, TOCTOU (IDEA-1494)
Three Codex round-1 issues, each fixed with the recommended shape:
P1 — visibility leak. The 409 response previously listed every blocking
child by ref/title/status, including children in collections the caller
couldn't see. The INVARIANT still evaluates against ALL children (it's a
data-integrity gate — a restricted user must not be able to close a
parent whose blockers they can't see), but the response payload now
filters to caller-visible children only. Hidden blockers surface as a
new `details.hidden_blocker_count` field plus an alternate human message
when every blocker is hidden ("blocked by N open children you don't
have access to"). Mirrors the visibility helpers (`visibleCollectionIDs`
+ `isItemVisibleToGuest`) used by the per-parent progress endpoint so
the two paths can't drift.
P2 — MCP code/details pass-through. The HTTP classifier was collapsing
409 into the generic `conflict` code and dropping `details`; the stdio
classifier was matching the human "cannot " message against the
validation regex and surfacing `validation_failed`. Both now surface
`open_children` with the structured details intact:
- HTTP: classifyHTTPStatusKind's 409 branch extracts the upstream
code; any non-empty, non-"conflict" code is passed through with
its `details` RawMessage. Generalizes beyond open_children — any
future structured 409 from a handler gets the same treatment.
- Stdio: the CLI writes a `pad-error: {json}\n` marker line on
stderr before the human-readable block (single source of truth for
both views), and classifyExecError detects the marker and lifts
the envelope verbatim. Marker is duplicated as a const between
internal/cli and internal/mcp to avoid pulling the cli package
into the classifier just for one string.
A new ErrOpenChildren error code constant + `Details json.RawMessage`
field on ErrorPayload back the wire shape.
P2 — TOCTOU. The guard previously ran in the handler before the store
transaction began; a concurrent child insert / child status flip could
slip between the children-list read and the parent's UPDATE. Fix:
- New `Store.UpdateItemWithPreCheck(id, input, precheck)` runs the
caller's invariant check inside the same tx, after acquiring the
workspace seq lock AND a new parent-children advisory lock keyed
on the parent ID. UpdateItem is now a thin wrapper passing nil.
- Every UpdateItem unconditionally acquires the parent-children
advisory lock for its own parent (if any) AND for itself-as-parent,
in a fixed order (parent first) so two updaters touching the same
parent always grab that key before the more-specific one — no
AB/BA deadlock.
- New `GetChildItemsTx` reads via the caller's tx; on Postgres the
advisory lock provides the snapshot guarantee (DISTINCT precludes
`FOR UPDATE`), on SQLite the global BEGIN IMMEDIATE write lock
serializes all writers.
- Handler now passes a precheck closure into UpdateItemWithPreCheck
at all three call sites (collab-snapshot path, applier-direct-write
path, main path). The guard's openChildrenGuardError sentinel is
unwrapped after each call so the 409 surfaces cleanly.
Tests:
- TestOpenChildrenGuard_VisibilitySanitizesPayload — restricted
editor sees parent + visible child, hidden child contributes to
hidden_blocker_count, no leak of ref/title/slug.
- TestOpenChildrenGuard_AllBlockersHiddenSurfaceGenericMessage —
open_children=[], hidden_blocker_count>0, message mentions "you
don't have access to."
- TestOpenChildrenGuard_TOCTOURace — 8 iterations of a child-flip
racing a parent-terminal update; asserts the forbidden outcome
(parent=completed AND child=open) never occurs.
- TestClassifyHTTPStatus_OpenChildrenPreservesCodeAndDetails +
inverse generic-409 test.
- TestClassifyExecError_OpenChildrenMarkerLiftsStructuredPayload +
no-marker-falls-through inverse.
* fix(server): open-children guard round 3 — 7 Codex findings closed (IDEA-1494)
P1 — visibility fail-closed. The handler was swallowing
visibleCollectionIDs errors, leaving visIDs==nil which the guard
treats as unrestricted, leaking hidden-child metadata. Now surfaces
the error as 500 BEFORE installing the precheck. Test:
TestOpenChildrenGuard_VisibilityLookupErrorFailsClosed closes the
store DB and asserts no 409+children leak.
P1 — link mutations acquire the advisory lock. SetParentLink,
ClearParentLink, CreateItemLink (when link_type ∈ childLinkTypes via
new isChildLinkType helper), DeleteItemLink (same condition), and
RestoreItem now take `pad:parent-children:<id>` in canonical sorted
order via new AcquireParentChildrenLocks helper. SetParentLink locks
BOTH old and new parents (re-parenting case). Race test
TestOpenChildrenGuard_LinkMutationRace asserts the forbidden
"link-committed-before-parent-flip AND parent flip succeeded" never
occurs by comparing link.created_at to parent.updated_at. Documented
semantics: status-wins + link-after-commit is legal under the
invariant "no open children EXIST AT THE MOMENT of transition" —
the post-condition variant ("no open child may EVER attach to a
terminal parent") is intentionally deferred.
P1 — MoveItem bypass closed. New MoveItemWithPreCheck mirrors
UpdateItemWithPreCheck — acquires workspace seq lock + parent-children
locks, re-reads in tx, runs caller precheck. handleMoveItem builds
the same guard closure using the DESTINATION schema for done-field
resolution (conservative — honors the schema the item moves INTO).
CLI gains `pad item move --force`, client gains MoveItemWithForce
that appends `?force=true` to the move endpoint. MCP catalog +
mapItemMove forward `force` through the route mapper. Tests:
TestOpenChildrenGuard_MoveItem_RejectsTerminalWithOpenChildren and
…_ForceOverrides.
P2 — pre-tx field-read TOCTOU. UpdateItemWithPreCheck and
MoveItemWithPreCheck now re-read the item via new getItemTx INSIDE
the tx (after locks) and pass that fresh snapshot to the precheck
closure; the precheck classifies the transition against the in-tx
view, not the handler-side pre-tx capture. Handler precheck closure
swaps `currentFieldsJS` from the in-tx snapshot. Test:
TestOpenChildrenGuard_PrecheckReadsInTxSnapshot stages a between-load
status mutation and asserts the precheck observes the post-mutation
fields.
P2 — bulk-update carries structured errors. cmd/pad/main.go's
updateFailure struct extended with Code + Details
(json.RawMessage). When client.UpdateItem returns *cli.APIError, the
row preserves the structured envelope. Human-text output also
renders the open-children list inline. Chose JSON-envelope route
over per-row stderr markers because bulk-update already produces a
structured envelope and ExecDispatcher returns stdout verbatim on
exit-0 — no classifier change needed. Test:
TestBulkUpdateStructuredFailuresCarryOpenChildrenDetails confirms
the wire shape the CLI lifts.
P3 — marker hardening. Marker bumped to versioned form
`pad-structured-error/v1:` (was `pad-error:`). cli.StructuredErrorMarker
+ mcp.structuredErrorMarker kept in lockstep with cross-references.
mcp.allowedStructuredErrorCodes whitelists known codes (currently
just open_children); unknown codes fall back to regex classification.
Marker must start the line after whitespace trim (embedded markers
ignored). Last-marker-wins to defeat pre-emption attacks. Tests:
TestClassifyExecError_{UnknownStructuredCode,OldMarkerVersion,
MarkerEmbeddedMidLine,LastMarker}.
P3 — soft-deleted collection schemas honored. New GetCollectionAnyState
mirrors childrenDoneFiltersForParent's inclusion rule; guard uses it
so a child still attached to a soft-deleted collection is evaluated
against ITS schema (custom terminal_options) instead of the default-
status fallback (which would mis-classify and false-block). Test:
TestOpenChildrenGuard_SoftDeletedCollectionSchemaHonored seeds a
custom collection, soft-deletes it while a child remains, and
asserts the terminal status is correctly recognized.
Comprehensive store-mutation audit results recorded in the PR
description (every method touching items.fields / items.collection_id
or item_links).
* fix(server): open-children guard round 4 — multi-parent locks, enum parity, PATCH atomicity (IDEA-1494)
Four Codex round-3 (blast-radius lens) findings, each fixed with the
recommended shape.
P1 — multi-parent lock set. acquireParentChildrenLocksForUpdate and
RestoreItem previously used `LIMIT 1` against item_links, so a child
with BOTH a `parent` link to P1 AND an `implements` link to P2 only
locked one of them. The other parent's open-children precheck could
race against the child's status flip and miss it.
Fix: new listParentChildLockKeys helper runs the same query
GetChildItems' inclusion rule uses (childLinkTypes), returns ALL
distinct parent target_ids, and feeds them into the canonical
multi-lock helper. Both UpdateItemWithPreCheck and RestoreItem now
acquire locks on {self} ∪ {all-parents-via-childLinkTypes}. Test:
TestOpenChildrenGuard_MultiParentChildLocksAll races a child status
flip against terminal-updates on both parents simultaneously.
P2 — lock-order asymmetry. The pre-fix codebase had multiple lock-
acquisition shapes: parent-then-self in acquireParentChildrenLocksForUpdate,
single-key in RestoreItem / CreateItemLink / DeleteItemLink /
ClearParentLink, and a sorted multi-key in SetParentLink. Two
concurrent callers using different ad-hoc orderings could AB/BA
deadlock.
Fix: removed the per-call-site AcquireParentChildrenLock helper
entirely. Every site now goes through AcquireParentChildrenLocks
(the canonical sorted multi-lock helper) — including ones that need
only one ID (the variadic call still sorts a one-element slice).
The helper's doc comment explicitly states the contract: "Ad-hoc
single-key acquisition outside this helper is FORBIDDEN — two call
sites taking distinct keys in different orders WILL deadlock."
Test: TestOpenChildrenGuard_NoDeadlockUnderReverseOrderConcurrency
runs reverse-order re-parents with a 5-second timeout; assertion
fails on hang.
P2 — HTTP/stdio code-surface parity. Round 2's HTTP pass-through
("any non-conflict upstream code") silently widened the ErrorCode
enum beyond stdio's allow-list (`open_children` only). Agents saw
different code surfaces depending on which dispatcher delivered
the response.
Fix: HTTP 409 branch in classifyHTTPStatusKind now consults the
same allowedStructuredErrorCodes whitelist stdio does. Codes
outside the set collapse to ErrConflict (no details), matching
what stdio does for an unknown-code structured marker. Doc on
allowedStructuredErrorCodes updated to make the dual-consumer
contract explicit: "Adding a new structured code is a TWO-WAY
change." Tests:
TestClassifyHTTPStatus_UnknownConflictCodeFallsBackToErrConflict
and TestStructuredErrorCodeParityAcrossTransports.
P3 — PATCH atomicity. A combined PATCH with `parent` + `status=terminal`
on an item with open children used to commit the parent-link change
INLINE (before the guard ran) and then reject the field write.
Caller saw 409 but the parent had already moved.
Fix: parent-link mutation is now DEFERRED — captured into outer-
scope vars during fields validation, executed AFTER
UpdateItemWithPreCheck succeeds. A guard rejection returns before
the link write block, so on rejection the link is untouched.
Documented choice: "reorder, don't tx-wrap" — wrapping SetParentLink
into the same store tx would require threading a *sql.Tx through
the SetParentLink API (which is also called from the
handler_item_links path); reordering is the smaller surgery and
gives the correct outcome on the failure direction. A residual
window remains in the OTHER direction (field write commits, link
write fails) — not made worse by the reorder, and called out
inline for a future tx-wrap pass.
Test: TestOpenChildrenGuard_PatchAtomicRejectionPreservesParentLink
sets up target → oldParent → openChild, sends PATCH {parent=newParent,
status=completed}, asserts 409 AND target.parent_link still points
at oldParent.
* fix(server): open-children guard — emit details.open_children as [] not null on hidden-only rejection (IDEA-1494)
|
||
|
xarmian
|
4bbd0a210d |
feat(collections): widen LibraryPlaybook with InvocationSlug + Arguments (TASK-1398) (#527)
* feat(collections): widen LibraryPlaybook with InvocationSlug + Arguments (TASK-1398) Adds two optional fields to LibraryPlaybook: - InvocationSlug — kebab-case slug for `/pad <slug>` routing (PLAN-1377) - Arguments — argument spec mirroring the body's `## Arguments` section Both fields are tagged with `omitempty` so existing library entries (none of which set them) serialize unchanged. seedPlaybookFromLibrary() now forwards both into the seeded item's Fields JSON only when set, matching the shape ShipPlaybook() already writes. This is the foundational task that unblocks T2 through T6 of the playbook library overhaul. Parent: PLAN-1397. * fix(library): propagate invocation_slug + arguments through activation paths per Codex review (round 1) Codex round 1 caught that the activation paths for library playbooks rebuild the fields map and drop the new fields, so any library entry declaring invocation_slug/arguments would lose `/pad <slug>` routing after activation. Fixed in three places: - internal/cli/client.go LibraryPlaybook (the client-side mirror used by the CLI `pad library activate` command) - cmd/pad/main.go libraryActivate (CLI subprocess activation) - internal/mcp/dispatch_http_slice4.go dispatchLibraryActivate (MCP pad_project action=library-activate) All three now forward invocation_slug and arguments only when set, matching ShipPlaybook()'s shape exactly. Web client (web/src/lib/api/client.ts) activation payload is T2's explicit scope — left for that PR. |
||
|
xarmian
|
fed4b60e65 |
feat(playbook): add pad playbook CLI (list/show/run) + endpoints (TASK-1382) (#520)
* feat(playbook): add pad playbook CLI (list/show/run) + endpoints (TASK-1382)
PLAN-1377 T5 — first-class invokable-procedure surface. Three HTTP
endpoints + three CLI subcommands, with a strict CLI arg parser and a
side-effect-free run path.
Endpoints
GET /workspaces/{ws}/playbooks — metadata array, same
projection as bootstrap
GET /workspaces/{ws}/playbooks/{ref} — full item, resolved by
invocation_slug | ref | slug
POST /workspaces/{ws}/playbooks/{ref}/run — bind args, return body +
bound + unbound. No
execution; the agent
owns step playback.
Resolution
resolvePlaybook walks invocation_slug first (the /pad <slug>
user-facing identifier), then falls back to ResolveItem (UUID / ref /
slug). A stray TASK-5 hitting /playbooks/TASK-5 returns 404 instead
of leaking a non-playbook into the surface.
Arg parsing
ParsePlaybookCLIArgs implements the strict rules from PLAN-1377:
- Required positional args first, in declared order.
- Flag types: bareword presence sets true.
- Other types: key=value form (number is parsed as float64,
enum is validated against declared options).
The server takes either pre-parsed args (MCP / programmatic callers)
OR raw CLI tokens (CLI caller); merge logic prefers explicit args
over raw_args. The CLI sends raw_args, so there is one parser
implementation and no drift risk.
CLI
pad playbook list — table or json
pad playbook show <slug|ref> [--format] — markdown / json
pad playbook run <slug|ref> [args...] — body + bound args
Client method
cli.Client.{ListPlaybooks, ShowPlaybook, RunPlaybook(args,
rawArgs)} — runtime callers pass args; CLI passes rawArgs.
Tests
TestPlaybookList, TestPlaybookShowByInvocationSlug,
TestPlaybookShowByRef, TestPlaybookShowRejectsNonPlaybook,
TestPlaybookRunBindsArgs (with-args, missing-required, with-raw-args),
TestParsePlaybookCLIArgsErrors, TestPlaybookListEmptyShape.
Parent: PLAN-1377.
* fix(playbook): tighten arg parsing per Codex round 1 (TASK-1382)
P2.1 — Positional binding now skips optional and flag-typed slots.
The PLAN-1377 contract says ONLY required args fill positional slots;
other typed args must be key=value. Without this, a spec with an
optional arg before a required one (e.g.
[merge-strategy?, target!]) bound the caller's bareword TASK-7 to
merge-strategy instead of target.
P2.2 — number coercion now uses strconv.ParseFloat with NaN/Inf
rejection. Sscanf(%g) was sloppy: it accepted '1abc' as 1 (partial
match) and accepted NaN/Inf, which json.Marshal then refused after
the handler had already written a 200 header.
P3 — empty-body run requests now decode cleanly. The decodeJSON
wrapper folds io.EOF into 'invalid JSON: EOF' so the previous
err.Error() != "EOF" check never fired. errors.Is(err, io.EOF) on
the unwrapped chain handles the wrapping correctly.
Tests: TestPlaybookRunAcceptsEmptyBody,
TestParsePlaybookCLIArgsOptionalNotPositional,
TestCoercePlaybookValueNumberRejectsBadInput.
Parent: PLAN-1377.
|
||
|
xarmian
|
24f0445efa |
feat(bootstrap): single-roundtrip /pad context-load endpoint (TASK-1379) (#518)
* feat(bootstrap): single-roundtrip /pad context-load endpoint (TASK-1379)
Implements the agent bootstrap surface for PLAN-1377. One HTTP call
replaces the four separate /pad context-loading invocations (workspace
+ collections + conventions + roles + playbooks) the skill used to
make, cutting ~200-400ms of startup latency on every /pad command.
Wire shape:
- GET /api/v1/workspaces/{ws}/agent/bootstrap returns AgentBootstrap:
workspace { slug, name, id }, user { name, email, id },
collections [...], conventions [...always-on, status=active],
roles [...], playbooks [metadata-only — no bodies], dashboard {...},
recent_activity [... 24h].
- pad bootstrap [--format json|markdown] CLI wrapper. JSON is the
canonical wire format that the /pad skill consumes; markdown is a
human-readable summary for quick terminal inspection.
Implementation notes:
- Single source of truth: Server.BuildAgentBootstrap. The HTTP handler
is a thin wrapper, and TASK-1380 will reuse it from three MCP
surfaces (resource + set_workspace embed + pad_meta tool action).
- Dashboard reuse: handleGetDashboard's body extracted into
buildDashboardResponse(workspaceID, r) returning (*DashboardResponse,
error). The HTTP handler is now a thin wrapper; bootstrap calls the
builder directly to embed dashboard data without a second roundtrip.
- Playbook bodies are deliberately NOT included — metadata only
(~80 bytes/entry vs 5-10KB) so the bootstrap stays small for
workspaces with many playbooks. Full bodies load on invocation.
- Convention bodies ARE included for the always-on/active subset (must
be agent-known up front); trigger-specific conventions stay
load-on-demand.
- Empty slices serialize as [] not null so the agent doesn't need
defensive nil checks.
Tests:
- TestBootstrapEmptyWorkspace, TestBootstrapEmptyArraysNotNull,
TestBootstrapIncludesPlaybookMetadata,
TestPlaybookSummaryPrefersFirstParagraph.
Parent: PLAN-1377.
* fix(bootstrap): respect collection visibility + guest grants (TASK-1379)
Codex round 1: BuildAgentBootstrap was bypassing visibility filters,
so a guest admitted by RequireWorkspaceAccess could read collections,
conventions, and playbook metadata they don't have access to.
Now mirrors handleListCollections + handleGetDashboard:
1. Resolve visibleCollectionIDs(r, workspaceID) once.
2. Filter the Collections array through isCollectionVisible.
3. Gate the conventions + playbooks sub-queries on whether the caller
can see those collections at all (presence in the filtered slice
implies visibility).
4. Empty slices for inaccessible sub-resources, so the wire shape
stays consistent — no missing keys to confuse the agent skill.
5. Pass-r=nil callers (future MCP in-process dispatchers) keep the
'full visibility' shortcut, but the doc comment now spells out that
those callers MUST verify access out-of-band.
Parent: PLAN-1377.
* fix(bootstrap): apply guest item-level grants + recompute role counts (TASK-1379)
Codex round 2:
P1 — Item-level guest grants now flow into the convention + playbook
sub-queries. visibleCollectionIDs alone admits the conventions /
playbooks collection if a guest has ANY item grant inside it; without
ItemIDs filtering, those queries then return the whole always-on
convention body set or every playbook's metadata. Now mirrors the
handleListItems shape: resolve (fullCollIDs, grantedItemIDs) via
guestResourceFilter, and pass the (collIDs, itemIDs) tuple through to
collectAlwaysOnConventions + collectPlaybookMetadata so a guest with a
single grant only sees that one item.
P2 — AgentRole.ItemCount is now recomputed from the visible item set
for restricted callers, matching handleListAgentRoles. Without this,
a guest could read role counts computed across all workspace items and
infer hidden activity per role.
Helper signatures updated to accept (collIDs, itemIDs); the doc
comments explain the nil/non-nil semantics so future callers can't
silently regress this.
Parent: PLAN-1377.
* fix(bootstrap): rewrite collection counts from visible set for guests (TASK-1379)
Codex round 3: Collection.item_count + active_item_count are computed
across the whole collection by ListCollections, so a guest with one
item grant in a collection still received the collection in the
filtered list but with hidden counts. Reuse the visible item set
(already computed for role counts) to recompute collection item_count
for restricted callers. active_item_count is set equal to item_count
to avoid a separate done-rules buildup the bootstrap consumers don't
depend on — better a self-consistent number than a leaked one.
Parent: PLAN-1377.
|
||
|
xarmian
|
d915cc3cf8 |
feat(cli): per-server credentials in credentials.json with v1 → v2 migration (TASK-1228) (#435)
Implements IDEA-1226. ~/.pad/credentials.json is now a map keyed by
server URL so one developer machine can stay logged in to multiple Pad
instances simultaneously — `apm/` repo on Pad Cloud, `target/` repo on
local, `testing/` repo on staging — without each `pad init --url <other>`
clobbering the previous server's credentials.
## On-disk format
v2 (new):
{
"version": 2,
"credentials": {
"https://app.getpad.dev": {"token": "...", "user_id": "...", ...},
"http://127.0.0.1:7777": {"token": "...", "user_id": "...", ...}
}
}
v1 (legacy, read-only): {"server_url": "...", "token": "...", "user_id": "...", ...}
Reads transparently migrate v1 → v2 in memory; writes always emit v2.
Side-effect-free reads — the on-disk file stays v1 until login/logout/
setup triggers a Save, which is when migration becomes durable. This
keeps `pad <read-only-command>` from rewriting credentials.json on
every invocation just because the binary upgraded.
## API
Replaces the three top-level helpers (LoadCredentials / SaveCredentials /
DeleteCredentials) with a CredentialStore type:
- LoadStore() (*CredentialStore, error)
- (s).Get(serverURL) *Credentials // nil-receiver safe
- (s).Set(serverURL, *Credentials)
- (s).Delete(serverURL)
- (s).Save() error
- WipeCredentialsFile() error // file-level — replaces DeleteCredentials
URL canonicalization is built in: trailing slash + surrounding whitespace
are stripped before lookup/store, so http://x:7777 and http://x:7777/
hit the same bucket. Same rule cmd/pad/server_info.go was already
applying via its now-redundant normalizeURL — removed.
No top-level `default` field. The configured server (cfg.BaseURL() from
~/.pad/config.toml or --url) is always the source of truth for "which
server am I targeting" — a separate `default` would create a second
source of truth and the split-brain bugs that follow.
## Behavioral changes
- `pad init --url <other>` against a server you've authed to before now
reuses the saved credential instead of clobbering it.
- `pad auth logout` removes only the configured server's entry. Other
servers' tokens stay intact (pre-fix: wiped the whole file).
- `pad auth whoami` reads only the entry matching the configured server.
- Single-server users see no behavior change — one entry, identical
shape per entry, identical UX.
## Compat shims removed
LoadCredentials / SaveCredentials / DeleteCredentials are deleted
outright (no // Deprecated lifecycle) — they're internal package
helpers with no external API contract. All 10 call sites in cmd/pad/
and internal/cli/ are migrated to the per-server API in this PR.
## Tests
internal/cli/credentials_test.go (15 tests):
- File missing / empty → empty store (callers don't need nil checks)
- v1 format reads + migrates in memory
- v1 with empty token → empty store (no phantom entries)
- v1 migration is durable on first Save (file flips to v2)
- v2 round-trip preserves multiple entries
- Set adds + replaces; mirrors URL into ServerURL field
- Delete keeps siblings (multi-server keystone behavior)
- Delete on absent key is a no-op
- Nil receiver Get/Delete don't panic (NewClientFromURL relies on this)
- URL normalization (trailing slash + whitespace)
- Save preserves all entries across the file boundary
- Save uses 0600 permissions
- WipeCredentialsFile removes the file + is idempotent
- Garbage file errors loudly (so we never silently lose data)
Existing tests unchanged. Full suite + lint + web-check green.
Closes: TASK-1228.
Implements: IDEA-1226.
|
||
|
xarmian
|
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
|
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
|
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
|
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
|
9072e49b17 |
feat: add CLI commands for item starring (#120)
Add star/unstar/starred CLI commands (PLAN-564, TASK-570): - pad item star <ref> — star an item - pad item unstar <ref> — unstar an item - pad item starred [--all] [--format json] — list starred items Client methods: StarItem, UnstarItem, ListStarredItems. |
||
|
xarmian
|
7ca0463e70 |
feat: browser-based CLI authentication flow (#97)
Replace the email/password terminal prompt in `pad auth login` with a
browser-based auth flow. The CLI creates a pending session, prints a URL
the user opens in their browser (works for localhost, remote VPS, or
Pad Cloud), and polls until the session is approved.
- Add CLI auth session endpoints (create, poll, approve)
- Add browser approval page at /auth/cli/{code}
- Rewrite `pad auth login` to use browser flow by default
- Keep `pad auth login --interactive` as email/password fallback
- Add login page redirect param support for post-login bounce-back
- Add SQLite and PostgreSQL migrations for cli_auth_sessions table
Closes PLAN-539, IDEA-404
|
||
|
xarmian
|
5606b22007 |
fix: address 6 security findings from Codex review of TOTP 2FA
HIGH fixes: - Login-verify no longer accepts bare user_id. Now requires an HMAC-signed, IP-bound, 5-minute challenge token issued during login (prevents password bypass via known user ID + TOTP code) - Recovery codes are SHA-256 hashed before storage; plaintext is returned to the user once and never persisted MEDIUM fixes: - ConsumeRecoveryCode uses a DB transaction to prevent concurrent double-consumption of the same recovery code - EnableTOTP is atomic: WHERE clause requires totp_secret match to prevent TOCTOU race between setup and verify calls - /auth/2fa/login-verify now uses the strict Auth rate limiter (5 req/min/IP) instead of the general API limiter - CLI login detects requires_2fa response and prompts for TOTP code instead of silently saving empty credentials |
||
|
xarmian
|
872f08aa84 |
feat: add compliance audit trail with IP/UA tracking
Extend the activities table to capture IP address and user agent for all state-changing operations. Add audit events for auth (login, logout, register, bootstrap, password changes), workspace management (member invite/remove, role changes), token lifecycle, and admin settings. - SQLite migration recreates activities table with nullable workspace_id, new ip_address/user_agent columns, and relaxed CHECK constraints - PostgreSQL migration adds columns and drops constraints - New ListAuditLog store method with action/actor/workspace/date filters - GET /api/v1/audit-log endpoint (admin-only) - CLI: pad workspace audit-log [--days N] [--actor X] [--action X] |
||
|
xarmian
|
be576d9e24 |
feat: agent roles — role-based (user, role) assignment for items (#58)
* feat: agent roles — role-based (user, role) assignment for items (#PHASE-9) Introduce agent roles as a first-class concept for human-agent work assignment. Roles describe capability specializations (Planner, Implementer, Reviewer, etc.) and items can be assigned to a (user, role) pair, enabling natural handoff workflows between different AI tools. Migration: - New `agent_roles` table (workspace-scoped, slug-unique) - `assigned_user_id` + `agent_role_id` columns on `items` with FKs - Removed legacy `assignee` text field from Tasks schema Backend: - AgentRole model + full CRUD store/API - All item queries updated with LEFT JOINs to resolve assignment - Item list filtering by assigned_user_id and agent_role_id - Role transitions tracked in activity feed metadata CLI: - `pad role list/create/delete` commands - `--role` and `--assign` flags on item create/update/list - Assignment displayed in `pad item show` output Web: - TypeScript types + API client for agent roles - Role badge on item cards in list/board views - Assignment display on item detail page * fix: enforce workspace-scoped assignments and fail fast on unresolved --assign filter Addresses code review feedback from PR #58: P1: Add validateAssignmentScope() to the store layer, called by both CreateItem and UpdateItem. Verifies that assigned_user_id belongs to the workspace (via IsWorkspaceMember) and agent_role_id exists in the workspace (via GetAgentRole) before writing. Prevents cross-workspace assignment leaks. P2: The CLI `pad item list --assign <name>` now errors instead of silently returning unfiltered results when the member lookup fails or no workspace member matches the provided name. |
||
|
xarmian
|
35f3dd1da4 |
feat(conventions): add structured metadata for TASK-133 (#51)
* feat(conventions): add structured metadata for TASK-133 * fix(web): add workspace update type for CI |
||
|
xarmian
|
b59f50982f |
feat(auth): add local bootstrap setup for TASK-118 (#37)
* feat(auth): add local bootstrap setup for TASK-118 * fix(auth): honor setup-required bootstrap flow |
||
|
xarmian
|
a2a25b176a | refactor(auth): make setup state explicit for TASK-117 (#36) | ||
|
xarmian
|
46447e5504 |
feat: user management & authentication (Phase 6) (#14)
* feat: add user management database migration and models
Add migration 012_users.sql with users, sessions, and workspace_members
tables. Add user_id columns to api_tokens, items, comments, activities,
item_links, and item_versions for proper user attribution. Create Go
model structs (User, Session, WorkspaceMember) in models/user.go.
* feat: add store layer for users, sessions, and workspace members
Implement CRUD operations for user management:
- users.go: create, get, update, list, validate password (bcrypt)
- sessions.go: create, validate, delete, cleanup expired (SHA-256 hashed tokens)
- workspace_members.go: add/remove members, role management, access checks
Adds golang.org/x/crypto/bcrypt dependency. Includes 16 new tests
covering all store methods, password validation, session lifecycle,
and workspace membership operations.
* feat: rewrite auth system from single-password to user-based
Replace single-password auth with email/password user authentication:
- New endpoints: POST /auth/register, GET /auth/me
- Rewritten: POST /auth/login (email+password), GET /auth/session
(needs_setup detection), POST /auth/logout (DB session destroy)
- Delete in-memory SessionManager, use DB-backed sessions via store
- New middleware: SessionAuth (cookie→user), RequireAuth (with
fresh-install passthrough when no users exist)
- Remove Password field from config, PAD_PASSWORD env var, SetPassword()
All 23 existing server tests pass (fresh DBs have no users → passthrough).
* feat: add workspace access control middleware
Add RequireWorkspaceAccess middleware that checks workspace_members for
authenticated users, with fallback for legacy API tokens and fresh
installs (no users → implicit owner). Includes role hierarchy helpers
(workspaceRole, requireRole) for downstream permission checks.
Wire middleware into the /{slug} workspace route group.
* feat: add CLI auth commands and credential storage
Add pad login, pad logout, pad whoami commands with credential
storage in ~/.pad/credentials.json (0600 permissions). Update CLI
HTTP client to auto-attach auth tokens and X-Pad-Agent header on
all requests. Add auth API methods (Login, Register, Logout,
CheckSession, GetCurrentUser). Extend .pad.toml with optional
agent_name field. Add golang.org/x/term for masked password input.
* feat: derive actor/source from auth context in all handlers
Replace hardcoded "user"/"web" actor/source strings with auth-aware
helpers. actorFromRequest() derives actor ("user"/"agent" via
X-Pad-Agent header) and source ("web"/"cli" from auth method).
agentMeta() merges agent name into activity metadata. Update all
item, document, comment, and move handlers to use request-based
logActivity/logActivityWithMeta. Remove hardcoded CreatedBy/Source
from all CLI commands — server now determines these from auth context.
* feat: frontend auth — login, registration, auth guard, user menu
Rewrite login page with email/password fields, add registration page
for first-time setup, update auth guard to handle needs_setup redirect.
Add user menu to sidebar with logout. Update API client with new auth
methods (register, login with email, session with needs_setup flag).
* feat: migrate API tokens from workspace-scoped to user-owned
API tokens now have a user_id owner and optional workspace_id scope.
CreateAPIToken takes userID as first parameter. ValidateToken resolves
the token's user into the request context. TokenAuth middleware now
sets ctxCurrentUser when a user-owned API token is used. Add user-
scoped endpoints: GET/POST/DELETE /auth/tokens. Keep workspace-scoped
token endpoints for backwards compatibility.
* feat: workspace membership, invitations, and role enforcement
Add workspace_invitations table (migration 013) with join codes.
Implement invitation store methods (create, get by code, accept,
list). Add member management handlers: list members + invitations,
invite (auto-adds existing users or creates invitation), remove
member, change role, accept invitation by code. Add API routes
under /workspaces/{slug}/members/* and /invitations/{code}/accept.
Add CLI commands: pad members, pad invite, pad join.
* feat: auth tests and documentation updates
Add comprehensive auth endpoint tests: registration flow (first user
becomes admin), login/logout, validation errors, duplicate email,
auth enforcement (401 after users exist, exempt paths), /me endpoint.
Update CLAUDE.md and README.md to document user-based auth system,
replacing old PAD_PASSWORD references with pad login/members/invite
workflow and role-based access control.
* feat: add members management UI to workspace settings page
Add Members section to settings with: member list (avatar, name,
email, role), role change dropdown (owner only), remove button
(owner only), pending invitations display with join codes, and
invite form with email + role picker. Add members API methods to
the TypeScript client (list, invite, remove, updateRole).
* fix: backfill workspace owners for pre-migration workspaces
Add backfillWorkspaceOwners() that runs on server start. For any
workspace with no members, adds the first admin user as owner.
This handles the migration case where workspaces existed before the
user system — without it, the members list shows empty.
* feat: shareable invite links with /join/[code] page
Replace raw join codes with full shareable URLs. Server generates
join_url using its configured base URL (e.g. https://pad.example.com/
join/a3f8b2c1). New /join/[code] page handles the full flow: checks
auth → shows login/register if needed → accepts invitation → redirects
to workspace. Settings page shows "Copy invite link" button that copies
URL to clipboard. CLI outputs shareable link instead of raw code.
* fix: auto-add workspace creator as owner, integrate auth into pad init
handleCreateWorkspace now adds the authenticated user as owner of the
new workspace immediately — no more relying on the startup backfill.
pad init now checks auth status before making API calls. If no users
exist, prompts to register. If not logged in, prompts to login. After
auth, proceeds with workspace creation normally.
* fix: add join_url to invite response type in API client
* fix: address codex review — invite registration, logout token revocation, workspace scoping
- Allow registration with valid invitation_code (fixes invite flow for new users)
- Revoke Bearer session tokens on logout, not just cookies
- Filter workspace listing to user's memberships (admins see all)
|
||
|
xarmian
|
dc83d7490c |
feat: Add content templates for collections
Collections can now define a content_template in their settings — a markdown template that pre-fills new items. When creating a bug report, for example, the template can include "Steps to Reproduce", "Expected Behavior", "Actual Behavior" sections automatically. - Add content_template to CollectionSettings (Go model + TypeScript type) - Sidebar "New" button uses template when creating items - Dashboard quick-create buttons use template - Template is stored in collection settings JSON, configurable via the collection edit UI |
||
|
xarmian
|
6daa8eb68b |
Add move item between collections with field migration
Full-stack feature: move items between collections (e.g., idea → task)
with automatic field migration.
Backend:
- Field migration engine (items/migrate.go) maps matching fields,
handles type conversions, drops incompatible fields, applies defaults
- Store method updates collection_id and assigns new item_number
- POST /api/v1/workspaces/{ws}/items/{slug}/move endpoint
- Activity logging with "moved" action and from/to metadata
- 6 migration unit tests covering type matching, conversion, and edge cases
CLI:
- pad move <slug> <target-collection> [--field key=value ...]
- Accepts singular collection names (task, idea, bug, etc.)
Web UI:
- "Move to..." dropdown on item detail page
- Shows all collections except current with icons
- Redirects to the item's new URL after move
Field migration rules:
- Same type: transfer directly (validate select options)
- Compatible types (text↔url, number→text, select→text): auto-convert
- Incompatible types: drop silently
- Missing required target fields: apply defaults or error
|
||
|
xarmian
|
dbecb741c6 |
Add workspace export/import and fix create workspace modal
Export/Import:
- `pad export -o file.json` exports workspace (collections, items,
comments, links, versions) to portable JSON
- `pad import file.json --name X` creates new workspace with
regenerated UUIDs and remapped relations
- GET /workspaces/{slug}/export and POST /workspaces/import endpoints
- "Download JSON" button in workspace settings
- Import tab with drag-and-drop in the create workspace modal
- Full transaction wrapping for atomic imports
Create Workspace Modal:
- Extracted from sidebar dropdown into a proper centered modal
(sidebar's CSS transform was trapping fixed-position elements)
- Rendered at root layout level via uiStore flag
- Create and Import tabs with template picker and file drop zone
|