mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-10 23:15:40 +00:00
d24df5567060efea62cfef40cb5c2d84f34dfd54
1363 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
d24df55670 |
refactor(web): delete unmounted components carrying real logic (TASK-2632) (#1163)
A one-shot sweep, not a standing process. Two dead components had been found incidentally in one week, each discovered only because someone was about to change behaviour it appeared to depend on -- VersionHistory during BUG-2608 (its apparent liveness would have blocked a default history limit) and EditorToolbar before it. Retired UI left in-tree costs every future reader who greps for a component, finds a plausible implementation, and reasons about behaviour nobody mounts; it also silently constrains fixes. Instrument, two passes over all 110 components under web/src/lib/components: 1. Plain substring grep of each basename across web/src + web/e2e. Four zero-hit. This pass counts COMMENTS as liveness, so it under-reports deadness -- conservative in the safe direction. 2. Import/mount-only regex (a from-import of the .svelte path, a dynamic import of it, or a <Name element). Six zero-hit; the two extras were exactly the comment-shadowed cases pass 1 could not see. Controls: a known-live component (BacklinksPanel) resolves to its single consumer under both passes; each of the six candidates then took a repo-wide plain grep with no include filters, and every surviving hit was read. Pass 2's one known blind spot -- a component referenced only by vi.mock(path) -- was checked by enumerating every vi.mock target ending in .svelte; all are .svelte.ts store/service modules except CommentEditor, which is independently imported. None of the six is in that set. Deleted (six dead, two cascade orphans): - activity/ActivityFeed.svelte -- a live /activity route page and TimelineActivityCard both exist; neither touches it. - charts/LineChart.svelte and charts/layers/Lines.svelte -- from the TASK-1632 LayerCake library; only BarChart reached the insights pages. Lines had exactly one consumer (LineChart), so it falls with it. AxisX/AxisY stay: shared with BarChart. - charts/Sparkline.svelte (TASK-1638) -- its only repo-wide reference was a prose comment recording that PLAN-1542 chose not to show it. Zero mounts. - editor/MermaidRenderer.svelte -- superseded by the MermaidCodeBlock NodeView in Editor.svelte, which owns the render queue, toggle and error state. - versions/VersionHistory.svelte -- the BUG-2608 find. The live path is ItemTimeline to TimelineVersionCard to DiffView; DiffView stays. - attachments/fixtures/LightboxStub.svelte and fixtures/lightboxStub.ts -- a pair that referenced only each other. Their last consumer was removed by the TASK-2489 atomic cutover, so they were orphaned rather than born dead. Nothing was reclassified live-but-obscure, so no import-site comments were owed. Two docs updated so no artifact points at a deleted file: the web README component tree drops the activity/ line, and the UserOverviewTab comment now says the Sparkline component was deleted here and is recoverable from history, rather than leaving a dangling decision record. Git history is the archive; anything worth resurrecting is one revert away. 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
|
b62c6692c8 |
ci: stabilize two required gates that go red with no code involvement (BUG-2645 opt 1, BUG-2568 opt 1) (#1158)
Two self-inflicting required gates, one reviewed workflow unit (CONVE-2438: its own PR, never a rider; minimal diff, action pins untouched). - E2E (Playwright) timeout-minutes 10 -> 15 (BUG-2645 mechanism b). The 10m cap covers build-web + build-binary + install-playwright + ~190 tests; a clean pass is ~8-10m, so a single flaky-test retry tips the total past the cap and the job is CANCELLED mid-suite while every test passed. 15m absorbs one retry and still kills a genuine hang. Right-sizing evidence: an 8m10s clean rerun vs a 10m18s cancel at the SAME SHA (PLAN-2636 unit 2, #1157). - golangci-lint-action verify: false (BUG-2568). The action's pre-lint `config verify` fetches its JSON schema over the network from golangci-lint.run and fails the required Go gate when that host is slow/unreachable — no lint finding, no code involvement (once skipped govulncheck entirely). A malformed .golangci.yml is still caught by the lint run itself, with a worse message. Scope: this closes BUG-2645 mechanism (b) only. The other two 2645 mechanisms remain, documented as known and out of scope for this unit: (a) the CI/Nix twin-workflow concurrency race that cancels an already-passed run, and (c) a job-rerun expiring inside a parent run already terminal-cancelled. Both are concurrency/rerun-config issues, not a timeout bump. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V |
||
|
xarmian
|
e36be90f05 |
fix(web): order-and-merge contract for the localIndex cache — tombstones, durable retag overlay, equal-seq merge (PLAN-2636 unit 2, BUG-2633/2634/2635) (#1157)
The persisted localIndex cache decided writes with a bare seq compare that
(a) had no memory of hard deletes, (b) could not arbitrate out-of-band
fields (collection_slug), and (c) blind-accepted at equal seq. Those are one
gap seen three ways. This lands the single write policy + read overlay the
PLAN-2636 unit-2 design checkpoint specifies.
- itemRowMerge.ts (new, pure): resolveRowWrite(stored, tombstone, incoming)
replaces the boolean shouldWriteRow at both persistence call sites —
tombstone gate (2633) -> seq guard + no-seq asymmetry (unchanged from 2609)
-> equal-seq MERGE (2635) -> preserveProjectionMetadata on write. The two
projection helpers move here verbatim from localIndex.svelte.ts so the IDB
layer can share them without importing the Svelte store; RAM stays
bit-identical (pinned by localIndexUnparented.svelte.test.ts).
- Tombstones (2633): new `tombstones` object store, IDB format v1->v2 (row
shape unchanged, LOCAL_INDEX_SCHEMA_VERSION stays 3). Hard removals in
persistDelta stamp deletedAtSeq=cursor; persistRemovals stamps the persisted
meta.sync cursor, or — when no sync has run — the removed row's own seq, so
an upsert-then-remove before the first sync is still un-resurrectable (codex
F1). raiseTombstone never lowers an existing stamp, so an out-of-order
cross-tab eviction can't weaken the gate (codex F4). A stale/seq-less
snapshot behind the stamp can't resurrect; a strictly-newer write supersedes
and clears the tombstone. persistReplace clears the store. Old build opening
a v2 DB gets a VersionError -> memory-only degrade.
- Durable retag overlay (2634): persistRetag also upserts {key:'retags', map}
in the meta store; hydrate reapplies it to matching rows (by collection_id)
after the read, so a rename survives a racing older-slug delta and a reload.
persistReplace drops the key (server rows carry live slugs — BUG-2601).
Tests: itemRowMerge.test.ts (pure decision matrix), localIndexPersistence
.unit2.idb.test.ts (tombstone/overlay/cross-tab/migration outcomes through
fake-indexeddb), harness raw-readers for tombstones + retags. Every regression
mutation-verified discriminating — including the codex-round-1 fixes: neuter
tombstone gate / blind-accept equal seq / skip overlay-apply / drop preserve /
break the shared helper / skip pre-sync tombstone (F1) / blind tombstone
overwrite (F4) / ignore overlay membership (F5) — each reddens exactly the
matching test across RAM and persistence. shouldWriteRow removed (superseded);
its sibling test repointed.
Gates: npm run test 1730 passed (98 files); npm run check 0 errors;
check:tiptap-pins OK; vite build clean. WEB-ONLY, zero Go, no dep churn
(fake-indexeddb already on main from unit 1).
Codex round 1: F1/F4/F5 fixed above. F2 (persistReplace clears tombstones for
omitted ids — pre-existing cross-tab window, self-healing) and F3 (durable
overlay can revert a newer authoritative slug after a missed rename SSE; no
local disambiguator — collection_slug is out-of-band) are lead-accepted as
documented residuals (F2 at persistReplace, F3 at the hydrate overlay-apply
site, each with its trigger + heal paths). True F3 disambiguator would be
server-side collection-slug versioning; ruling on the PLAN-2636 trail.
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
|
||
|
xarmian
|
b759898d5b |
test(web): fake-indexeddb harness for the localIndex persistence layer (PLAN-2636 unit 1) (#1156)
The localIndex IndexedDB persistence layer ran as a no-op under vitest — there is no IndexedDB in the node test environment, so `isSupported()` returned false and every persist/hydrate call short-circuited. The entire layer, and BUG-2609's seq-guard fix, shipped on live-browser evidence runs only (the #1148 finding). This adds the harness that makes it testable, the prerequisite for unit 2's order-and-merge regression matrix. - fake-indexeddb dev dependency (exact-pinned). - A dedicated `idb` vitest project (glob `*.idb.test.ts`, node env) whose setup installs fake-indexeddb's globals and a fresh IDBFactory per test. It self-disables when the dep can't be resolved — mirroring the jsdom project — so a symlinked worktree without it keeps `npm run test` green, and CI activates it once installed. The idb glob is excluded from the node project so the persistence layer can't no-op there and pass vacuously. - Harness helpers unit 2 builds on: a second cross-tab connection to the same database (2635), a v1-database seed + higher-format-version reopen + downgrade VersionError (the v1→v2 migration exercise), a fresh-module loader that clears the connection cache, and raw ground-truth reads. `harnessDbName` mirrors the module's `dbName` exactly, pinned by a test so a drift can't make assertions read an empty sibling database. - BUG-2609's evidence run is ported as a deterministic sequential regression: a newer delta commits its atomic rows+cursor transaction, then a stale older-seq snapshot lands last and is refused (IDB serializes overlapping transactions, so no interleaving control is needed). Plus a raw serialization characterization pinning that platform guarantee. No production code changes. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V |
||
|
xarmian
|
7680919dbd |
fix(store): re-stamp attachment refs on item/document restore so a racing GC claim is refused (BUG-2629) (#1155)
An attachment referenced only from soft-deleted content was reclaimable by the orphan GC: AttachmentReferenced scans LIVE rows only, so the archived reference is invisible to the sweep, and past the grace period the claim reclaims the blob. On restore the reference is live again and dangles. The reachable case is a never-attached upload (item_id NULL) referenced from a document — documents have no document_id column, so such a row is necessarily never-attached and the ClaimNeverAttachedAttachment predicate applies with nothing else standing in its way — or from an item's content where the attachment was uploaded unattached. RestoreItem and RestoreDocument now call stampAttachmentRefsTx before clearing deleted_at, inside the restoring transaction, per that helper's ORDERING contract: the stamp row-locks the attachment, so a GC claim racing the restore blocks until commit and re-evaluates last_referenced_at against the fresh stamp — refusing. RestoreItem stamps content + fields; RestoreDocument (previously a bare Exec) is wrapped in a transaction that reads the soft-deleted content + workspace and stamps content. This is prevention only: a blob already reclaimed before the restore is gone (the claim is irrevocable by design), and the restore-time stamp matches zero rows. Surfacing an already-dangling reference to the user on restore is tracked separately as IDEA-2646. Regression tests archive content holding the only reference, age the stamp past the claim's stale window, restore, then run the claim directly (the sweep's live scan would protect now-live content and pass for the wrong reason). All three legs fail on unfixed code. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V |
||
|
xarmian
|
b5f0cd3963 |
feat(web,server): render embedded image attachments on share pages via a variants-only byte endpoint (BUG-2389 2b, TASK-2637) (#1153)
Merged by lead on accepted infra cancellation, Dave-approved in chat (day-44). Evidence basis: E2E tests demonstrably pass at pinned SHA
|
||
|
杨成锴
|
22c5a858a1 |
fix(server): stop counting disabled conventions as completed work (#1152)
Merged after two codex review rounds (converged) on top of the community-loop supply-chain/static review. Review found two narrow follow-ups — the guest item-grant leg of the grouped terminal query keeps pre-PR over-matching semantics, and the standup/changelog display layer hardcodes `status` — both pre-existing edges, filed internally as follow-up work. Thanks @asjdf for a well-tested fix, and for honoring the per-collection terminal_options contract on both the CLI and server paths. |
||
|
xarmian
|
482815aa58 |
feat(web): composer + quick actions target armed sessions with honest counts (PLAN-2613 S4, TASK-2619) (#1151)
* feat(web): push composer + quick actions target armed sessions with honest counts (PLAN-2613 S4, TASK-2619) Presentation truthfulness (D3): only an armed session receives a push (the server filters delivery to armed sessions), so connected is not the same as accepting. Surfaces that decided push-vs-copy or enabled Send on the raw connected count would fire-and-forget into a connected-but-unarmed session and lose the instruction. - LiveSession TS type gains `armed` (S1 shipped it server-side; the web type had not caught up). - PushToAgentDialog: counts and targets the ACCEPTING (armed) subset. The presence line shows the split honestly — "M sessions accepting pushes (of N connected)", and the "N connected, 0 accepting pushes" empty state with /pad:connect enable instructions rather than hiding connected-but-unarmed sessions behind a bare zero. Send is gated on accepting > 0; the picker offers only armed sessions; broadcast reaches only accepting sessions (server- filtered). Degrades to N == M once the S3 rollout completes. - QuickActionsMenu: routes push-vs-copy on the accepting count, so a quick action copies (never silently pushes) when nothing is accepting; tagline and the shared dispatch copy say "accepting pushes", not "connected". No new server state — this consumes S1's per-session armed bit from GET /api/v1/sessions. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(web): quick-actions tagline shows the accepting-of-connected split (Codex R2) The menu tagline showed only the accepting count, hiding connected-but-unarmed sessions — the split D3 wants for quick actions as much as for the composer. PushPresence now carries `connected` alongside the accepting `count` (routing still keys on accepting), and the tagline renders "M accepting session(s) (of N connected)", plus the "N connected, 0 accepting pushes — run /pad:connect to enable" state instead of collapsing to a bare zero. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V |
||
|
xarmian
|
052c971785 |
feat(plugin): consent-gated push monitor + tri-state arm/disarm + envelope (PLAN-2613 S3, TASK-2618) (#1150)
* feat(plugin): consent-gated push monitor + tri-state arm/disarm + envelope (PLAN-2613 S3, TASK-2618) The plugin layer of the push-consent gate. S2 built the CLI arm/disarm/status verbs and the arm-state file; S3 makes the monitor existence itself the gate (D1) and adds the tri-state, the envelope, and the connect ritual. - Tri-state arm-state file: a session can be explicitly ARMED, explicitly DISARMED, or absent. `pad session disarm` now writes a session-scoped OFF marker (not a file removal), so a within-session disconnect wins even in an auto_arm=true repo — the disconnect verb must not be a lie there. The marker dies with the session (same liveness), so across sessions auto_arm remains the standing contract. ResolveAnnouncedArmed folds the tri-state over auto_arm; the monitor announces its result. - Gated monitors (monitors.json): the single always-on monitor is replaced by two — an `always` auto-arm monitor and an `on-skill-invoke:connect` manual monitor — both running scripts/pad-monitor.sh. The wrapper gates on a new hidden `pad session should-arm`, dedupes concurrent monitors with a liveness-aware per-session lockfile, and carries the reconnect loop so an in-session disarm stops the stream on its next reconnect. No consent → the monitor exits → nothing listening. - D5 envelope: a push notification carries the verbatim direction-with-authority framing (confirm in-session before anything destructive/irreversible); item- change kinds stay a light informational label. - /pad:connect + /pad:disconnect skills; /pad:status gains a one-line connection header from `pad session status`. /pad:connect runs the workspace's on-session-start playbooks on the first connect only (D8), tracked by a Booted flag carried forward across arm/disarm. plugin 0.2.1 → 0.3.0. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(plugin): address Codex R1 on S3 (disarm stops active stream, fail-closed local state) - HIGH-1: a within-session disarm now stops an ACTIVE stream, not just the next reconnect. The monitor re-checks consent every 2s while streaming and cancels the connection when it flips to not-armed, then exits (D1's whole- stream-behind-consent gate at the top of the loop), so the plugin wrapper keeps it dead. Fixes /pad:disconnect being a lie for an idle SSE that might never naturally reconnect. - HIGH-2: a corrupt/unreadable local arm-state file now fails CLOSED (LocalArmError -> not armed) instead of falling through to auto_arm, so a corrupted disarm marker can't silently re-arm an auto_arm repo. It is not reaped (reaping would re-arm on the next read); it is session-keyed and a re-arm overwrites it. - Shell wrapper: an empty (mid-startup) lock pid is treated as live so two monitors can't both steal the lock; INT/TERM now exit (a trap otherwise resumes the loop and reconnects without a lock). - Docs: plugin/skills/pad describes the new push-envelope line format; connect/status skills distinguish "consent set (armed)" from the server's observed connection counts rather than claiming "Connected". Bounded/safe-direction residuals documented in code: the reap TOCTOU and the Booted carry-forward race (both fail-closed / benign), and lock pid-reuse (dedupe only, fails toward not-streaming). Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(plugin): address Codex R2 on S3 (disarm-watcher timing, semantic corruption fail-closed) - HIGH-1: the disarm-watcher now starts BEFORE the connection is opened, so a disarm during connection/header negotiation cancels the request too (the request is built on streamCtx). streamWatchEvents also re-checks consent before delivering each notification and stops the stream if it was withdrawn, so no push is printed after a disarm even within the poll window. - HIGH-2: a syntactically-valid but semantically-garbage arm-state file (e.g. {} or {"pid":1}) now fails CLOSED via a well-formedness check (StartedAt + PID must be present, as our writer always stamps them) before liveness or reaping — so it can't be judged owner-dead, reaped, and re-armed through auto_arm, nor mistaken for a live headless arm naming init. - LOW: the cleanup trap uses condition 0 (portable) rather than the EXIT name. The disconnect skill note reflects the ~2s active-stream drop. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(plugin): /pad:disconnect always disarms, never gated on a linked workspace (Codex R3) Consent is session-scoped (keyed by the messaging socket, not the workspace), so a session that connected in one repo must be able to disconnect from anywhere — including a directory with no .pad.toml. The old precondition let a session move to an unlinked directory, "disconnect", and keep receiving pushes. Verified: `pad session disarm` from an unlinked cwd disarms the socket-keyed session state; should-arm then reports not-armed back in the original repo. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(cli): enforce the Armed != Disarmed writer invariant in arm-state validation (Codex R4) armStateWellFormed checked only StartedAt + PID, so a well-stamped file that violated the writer invariant — both armed and disarmed false (or both true) — passed validation and, since SessionArmState only branches on Disarmed, resolved to LocalArmOn and armed. The writer always sets exactly one of the two; require it, so a neither/both file fails closed (LocalArmError). 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
|
2322fb273f |
fix(web): give the IDB write path the seq guard RAM already had (BUG-2609) (#1148)
* fix(web): give the IDB write path the seq guard RAM already had (BUG-2609)
`upsert` and `applyRetag` hand persistUpserts a snapshot taken from RAM and do
not await it. An SSE delta for the same row can commit its own atomic
rows+cursor transaction in between, after which the older snapshot lands LAST:
IndexedDB then holds a pre-delta row while the persisted cursor sits past that
delta. Warm boot hydrates the stale row and `/items-changes?since=cursor` never
returns it again, so it stays stale until the item happens to change. RAM is
unaffected — single-threaded and already seq-guarded by mergeRow — which is why
this only ever showed up as a warm-boot regression.
The fix is the guard localIndex has had in RAM all along, applied at the layer
that lacked it: a read-modify-write inside the transaction, comparing against
what is STORED at write time rather than what was in RAM at snapshot time. Both
writers get it, because the race runs in both directions — a delta must not
overwrite a row that is already newer in the cache either.
Three boundary decisions, each with a reason rather than a default:
- EQUAL seq still writes. RAM merges same-seq projections before persisting,
so the incoming row IS the merged one; refusing it would drop that merge
and leave the cache behind RAM with no seq difference left to correct it.
- A MISSING seq on either side writes. Absence is not evidence of being
older, and refusing would silently disable the cache for any row the server
has not stamped.
- seq 0 is a value, not a blank. A falsy check here would let a stale row
through, so the comparison tests for undefined explicitly.
Worth stating because the outcome looks lossy and is not: `applyRetag` rewrites
collection_slug WITHOUT bumping seq, so a delta at a higher seq now SKIPS the
retag write. That leaves IDB agreeing with the persisted cursor while its slug
lags RAM — which self-heals through the sync-pass reconcile (BUG-2601). What it
replaces does not self-heal: a row behind the cursor is invisible to delta sync
by construction.
VERIFIED IN A REAL BROWSER, because the failure mode of getting this wrong is
silent. Awaiting inside an idb transaction risks the transaction auto-closing
mid-loop, after which the remaining puts throw into a swallowed catch and the
cache quietly stops being written — worse than the bug. In-repo precedent said
it was safe (hydrate already awaits a get and keeps using the same tx), and a
live run confirmed it: 2625 rows persisted, all seq-stamped, cursor advanced,
zero page errors. The instrument was checked against a control build whose
guard refuses every write — 0 rows, cursor still advanced, which is the
cursor-ahead-of-rows divergence this bug is about, so the check demonstrably
reads the thing it claims to.
The decision itself is covered by unit tests, each mutation-verified against
the specific assertion written for it (always-allow, strict-greater-than, and a
falsy seq check each fail only their own cases).
* fix(web): route retags through a field-level write, and stop overclaiming the guard (BUG-2609)
Codex round 1. The P2 is a correction to my own commit message, and the more
important of the two findings.
I wrote that a retag write skipped by the seq guard "self-heals through the
sync-pass reconcile (BUG-2601)". It does not, and the code says so where I
should have read it: `applyRetag` does not bump seq, a collection rename
touches no items so no item delta ever re-stamps them, and localIndex's own
comment states pendingRetags is "not persisted (the window it guards is within
a single session)". A persisted slug that loses its rename stays wrong across
reloads with nothing left to correct it — so my guard would have turned a
last-write-wins race into a permanent staleness.
That is the second time today I stated a mechanism I had not read, and this
one made it into a commit message as the justification for shipping.
Fixed by design rather than by rewording. A retag is a FIELD-LEVEL intent:
"these rows are in a collection that got renamed". Expressing it as a whole-row
put makes a second claim — that every other field still matches a RAM snapshot
— and it is that claim the guard has to refuse. persistRetag reads each row
inside the transaction and changes only collection_slug, so the newer row's
fields survive AND the rename lands. Rows absent from the cache are skipped:
nothing to rename, and inserting a snapshot there would resurrect rows a delta
may have removed.
Codex's P1 — a delayed snapshot can resurrect a HARD-deleted row, because a
deleted row leaves no `existing` for the guard to compare against — is real,
pre-existing (a blind put resurrected it too), and not fixed here. Refusing it
needs a tombstone carrying the seq it was removed at, i.e. an IDB schema change
plus a version bump. Filed as BUG-2633.
The guard's doc no longer implies it covers either case. It now names both
exclusions, which is what it should have said before Codex had to ask.
Re-verified live after the redesign: 2625 rows persisted, all seq-stamped,
cursor advanced, zero page errors.
* docs+test: name the direction persistRetag does NOT close, and stop the tests implying coverage (BUG-2609)
Codex round 2, three findings.
P1 is real and I had only closed half the problem. persistRetag stops a late
RETAG from being refused, but a delta captured BEFORE the rename can commit
after it, whole-row put an older collection_slug at a NEWER seq, and pass the
guard legitimately. The root shape is that collection_slug is OUT-OF-BAND
relative to the item's own version — it changes without seq changing — so no
seq comparison can arbitrate it in either direction. Pre-existing (a blind put
lost the same race) and filed as BUG-2634 with the fix that actually closes it:
make the rename DURABLE, persisting the retag intent and reapplying it on
hydrate, the way localIndex already does in RAM with pendingRetags. That is a
different mechanism from this bug's guard, which is why it is filed rather than
folded in. The guard's doc now names the direction it does not cover instead of
implying it covers renames outright.
The other two are accuracy defects in what I wrote. A comment still said
`applyRetag` hands snapshots to persistUpserts, two paragraphs above the
exclusion list explaining that it no longer does. And the new test block read
as persistRetag coverage while testing only shouldWriteRow — a no-op
persistRetag would pass this entire file. It now says so in its name and its
doc, and the harness note says the same about the live browser run: that was
evidence the write path commits, not a regression test, and it will not run
again on its own.
Three of this bug's four review findings have been about my prose claiming more
than the code does, which is worth noting as the pattern rather than fixing
quietly a third time.
* docs: name the equal-seq residual where the decision is made (BUG-2609)
Codex round 3. Equal seq is permitted, and the justification I gave — RAM has
already merged the projection — holds WITHIN a tab and not across them: a tab
that never had the row in RAM can persist an unmerged snapshot over another
tab's merged one at the same seq.
Measured the reachability rather than accepting the report as stated. After
this branch, persistUpserts has exactly one caller, and `localIndex.upsert`
returns early when `next.seq <= existing.seq` — so it cannot persist an
equal-seq row when it HAS the row in RAM. The regression needs two tabs, the
writing tab lacking the row, and the other having merged a same-seq projection.
Narrow, and strictly better than before this branch, when a blind put regressed
rows at ANY seq.
Not fixed here because accept-or-refuse is the wrong axis: refusing would
mirror the bug onto persistDelta, where a re-delivered same-seq row carrying
computed projection fields would be skipped. What equal seq actually wants is a
MERGE, which this layer does not have and which is a deliberate layering
decision (persistence importing projection semantics). Filed as BUG-2635.
That makes three residuals from this one bug, and they are one family rather
than three coincidences — the persisted cache has no total order and no merge
semantics of its own. BUG-2633 (deletes have no seq to compare against),
BUG-2634 (collection_slug is out-of-band relative to seq), BUG-2635 (equal seq
needs merging). Each has a distinct fix, but they touch the same two functions
and share the same testing prerequisite, so 2635 carries the cross-links and
suggests triaging them as a batch — one pass giving the cache real
order-and-merge semantics beats three patches.
* fix(web): refuse a seq-less snapshot over a stamped row (BUG-2609)
Codex round 4, and this one was a real defect in my guard that my own test
enshrined.
I treated a missing seq symmetrically — "absence is not evidence of being
older, so write it" — which is true in one direction and wrong in the other.
The optimistic reorder path deliberately clears `seq` so the row bypasses
localIndex's RAM guard and the drag paints immediately (TASK-1357, verified at
the call site). Persisting that copy is incidental to the intent, and my rule
let a delayed seq-less snapshot overwrite an authoritative row that had already
landed at a real seq.
The result is worse than an ordinary stale row: the persisted row then has NO
seq at all, so neither this guard nor the RAM guard can order it on the next
warm boot, while the cursor sits past the delta that would have corrected it.
Now asymmetric, with the reasoning in place: an incoming row carrying ordering
evidence beats a stored row with none; two unstamped rows have nothing to
arbitrate; a stored STAMPED row refuses a snapshot that has no seq. Refusing
costs the reorder nothing — RAM still shows the optimistic order, the
authoritative response persists with a real seq moments later, and the cursor
has not advanced past that response, so a warm boot in between simply refetches
it.
The test that asserted the old behaviour has been replaced rather than
adjusted, and the mutation restoring the symmetric rule now fails only the new
case. Re-verified live after the change: 2625 rows persisted, all seq-stamped,
cursor advanced, zero page errors.
Distinct from the three filed residuals — this is the guard being wrong, not a
gap it deliberately leaves.
* fix(web): re-check collection membership inside the retag transaction (BUG-2609)
Codex round 5, a defect in the function I added last round. persistRetag read
each row by id and applied the renamed collection's slug without re-checking
that the row was still IN that collection. A row that moves between the RAM
retag and this transaction would then be persisted with a collection_id and a
collection_slug that disagree — behind the cursor, so no delta repairs it.
The irony is the point: persistRetag exists BECAUSE trusting a RAM snapshot at
write time is unsafe, and it went on trusting the snapshot's collection
membership. Re-reading the row was never the whole fix; re-checking what the
row says is.
The per-row decision is extracted as shouldApplyRetag so it is reachable by
tests at all — the transaction itself is not, in a harness with no IndexedDB —
and it now covers three refusals with a reason each: a row that moved
(mismatched id/slug persisted behind the cursor), a row already carrying the
new slug (idempotence), and an absent row (inserting one would resurrect it
behind the cursor, the BUG-2633 shape).
Each mutation-verified against its own assertion: dropping the membership check
fails only the moved-row case, and inserting absent rows fails only the
resurrection case. Re-verified live: 2625 rows persisted, all seq-stamped,
cursor advanced, zero page errors.
* docs: correct two claims this file made about itself (BUG-2609)
Codex round 7, both accuracy defects in my own prose after five rounds of
edits to it.
shouldWriteRow's opening line still said it returns false ONLY for strictly
older rows, which round 4 made untrue — it also refuses a seq-less row over a
stamped one. The paragraph below described the exception correctly, so the
summary contradicted its own body. Now states both cases up front.
The test file asserted "jsdom has no IndexedDB", and this file does not run in
jsdom: a plain .test.ts belongs to vitest's `node` project, per vitest.config.
The conclusion happened to hold — Node has no indexedDB global either — which
is the part worth flagging: right answer, wrong stated reason, and I had not
opened the config before writing it. It also claimed every function in the
module is a no-op under vitest, which is false of the exported decision
helpers being tested two screens above; that distinction is the whole reason
they were extracted.
Fixing the first attempt at this broke the file, and the cause is worth
recording: the glob I wrote to name the vitest project contained the character
pair that ENDS a block comment, so the doc terminated early and the rest parsed
as code — 19 type errors and a suite that reported "no tests". Same family as
backticks inside a double-quoted shell string: content carrying a delimiter the
surrounding syntax acts on. Rephrased to avoid the sequence rather than
escaping around it.
* docs: name the abort trigger at persistRetag, folded into BUG-2634 (BUG-2609)
Codex round 9. persistRetag is best-effort like everything in this module, so
an aborted transaction (quota, eviction, tab freeze) loses the rename outright
— and a lost rename does not self-heal for the same reason it cannot be
reordered: no item delta re-stamps those rows, and pendingRetags is in-memory.
Not a fourth filing. It is BUG-2634 reached by failure instead of by racing,
and the fix already proposed there — persist the retag INTENT and reapply it on
hydrate — closes both, because a recorded intent survives a failed write as
readily as a lost race. The real defect is that a rename is persisted as an
EFFECT with no durable intent, which makes it losable by anything.
Recorded on BUG-2634 so whoever takes it builds for both triggers (an
ordering-only fix would leave the abort case open and look complete), and noted
at persistRetag so a reader there meets the limit rather than inferring the
function is reliable.
|
||
|
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
|
50a442d048 |
fix(server): resolve collection slugs against the workspace's real collections (BUG-2578) (#1146)
* fix(server): resolve collection slugs against the workspace's real collections (BUG-2578) `pad item create spec` failed with "Collection not found" in a workspace whose collections include `specs`, because the singular forms live in collections.NormalizeSlug — a hardcoded switch over the DEFAULT templates' names, called from the CLI and the MCP dispatcher, both CLIENT side and neither with any view of the workspace. So a template-defined or user-created collection got no shorthand, and the spec template's central object was the one thing with no way to abbreviate it while peripheral `idea` had one. Resolving on the SERVER is what makes this general: the workspace's collection list only exists here, so one resolver covers the CLI, the remote MCP transport, the web UI and any direct API consumer, instead of teaching each client the same trick. `spec` is not in the client map, so it already arrives intact; a test in internal/mcp pins that pass-through, since a future map entry for it would silently take the fix away from MCP agents. EXACT MATCH ALWAYS WINS, and that is the property the design turns on. The fallbacks fire only when the input names no collection at all, so the resolver can never redirect a request that already succeeded — which is what makes it safe to add underneath five existing call sites. It has its own test, with the mutation that inverts the order failing it. Deliberately NOT wired into store.GetCollectionBySlug. That has 23 call sites including authorization paths (authz_cross_workspace, handlers_grants, handlers_share_links), and fuzzy resolution inside a function used for permission checks is how a check and the action it guards come to disagree about which collection they mean. Scope is the five user-typed item operations: create, list, move, bulk move, cross-workspace copy. Internal derivations (artifact import's collectionSlugForKind) and the web-only progress endpoints keep exact matching. Two things worth noting for whoever reads this next: The list handler resolved the collection for its visibility gate and then filtered items by the RAW url parameter, so a singular returned 200 with an empty list — a resolve-then-pass-the-unresolved-value bug my own wiring introduced, caught by the test that asserts listing works, not by the one that asserts creating does. This does NOT fix the sibling defect the re-derivation turned up: the client map SHADOWS an exact match, so in a workspace holding both `plans` and a user-created `plan`, `pad item create plan` silently files into `plans`. Verified still reproducing after this change, because the rewrite happens before the server sees the slug. Filed as BUG-2630 with a live repro; the lead ruled option 2 (send raw, retry on collection-not-found) and it rides a later PR, since changing wire behaviour is a compatibility call rather than part of this fix. * fix(server): canonicalize the resolved slug downstream in bulk move and items-index (BUG-2578) Codex round 1, and both findings are the same defect class as the one my own list test caught: resolve the collection, then keep using the caller's raw input for everything downstream. Bulk move is the one that matters, and it was reachable only BECAUSE the resolver made `spec` succeed at all — so the inconsistency arrived with this change rather than predating it. req.Collection is compared against item.CollectionSlug to decide whether the op even IS a cross-collection move, written into activity metadata as to_collection, and used as the SSE scope the arrival event is addressed to. Left raw, a move into `specs` would log a to_collection of "spec" that no reader can look up, address the arrival event to a lane no client watches, and — for an item already in `specs` — compare unequal and categorise a same-collection no-op as a move. Canonicalized once up front rather than at each of the four use sites, so a fifth use cannot reintroduce it. items-index filtered by exact slug too, so `?collection=spec` returned an empty index rather than an error. The web client sends canonical slugs and is unaffected; this is for direct API consumers, and it keeps the same exact-match-wins property, so no existing query changes meaning. A slug that resolves to nothing is passed through untouched, preserving today's behaviour. Both are mutation-verified: removing the canonicalization fails the activity assertion with the literal to_collection "spec", and removing the index resolution returns the empty result set. * test: cover cross-workspace copy and drive the MCP claim end to end (BUG-2578) Codex round 2, two coverage gaps, both real. The cross-workspace copy call site was wired to the resolver and never exercised: every existing copy test passes an exact slug, so reverting that line would have gone unnoticed. Now covered through BOTH halves — preflight and the mutating copy — because they resolve the destination separately, and a preflight that accepts a name the copy then rejects is the worse of the two failures. Mutation-verified: reverting the call site fails it with "Destination collection not found". The MCP test was scoped to what the dispatcher BUILDS — that the slug is passed through rather than rewritten — and its comment said so, but a URL assertion is a claim about the dispatcher, not about what an agent receives. Since the bug's body makes a claim about MCP agents specifically, that claim now has a test that drives the real server and store over the transport: create in `spec`, then LIST by the same shorthand, because an agent that can create something it cannot then list is not fixed. Mutation-verified: removing the server fallback fails it with the exact user-visible error the bug reports. The pass-through test stays. It guards a different thing — that a future entry in the client-side alias map would silently take the server fix away from MCP by rewriting the slug before it arrives — and has its own control (adding `spec` to the map fails it). The copy fixture uses a permissive destination schema on purpose: the shared dstSchemaJSON has required fields the source item does not carry, and a validation rejection would mask the resolution result under test. * fix(server): case-fold before pluralizing, pin the list by ID, resolve the bulk target once (BUG-2578) Codex round 3, three findings, all correct. CANDIDATE ORDER (P1). Pluralization was tried before the case-folded form, so `Spec` resolved to `specs` in a workspace holding both `spec` and `specs`. That is the same misfiling the exact-match-wins rule exists to prevent, reached by a different route: `Spec` names `spec` more closely than it names that name's plural. Folded form now goes first. My own candidate test had the wrong order baked into its expectation, which is why it did not catch this — the new end-to-end case asserts where the write actually lands, and both fail on the old order. LIST PINNED BY ID (P1). Visibility was checked against coll.ID and the query then filtered on a SLUG. A slug can be freed by a rename or delete and taken by another collection in between, so the response could carry a different collection's items — possibly one the caller cannot see. The ID cannot be reassigned, and both filters are ANDed, so a concurrent rename now yields an empty list rather than someone else's rows. Note this predates the diff in kind: the handler filtered by the RAW slug before, with the same gap. BULK RESOLVES ONCE, AND NOW THAT IS TRUE (P2). The previous commit canonicalized the target up front and said it did so "rather than resolving it per-item further down" — but the per-item path went on calling the resolver for every row, so a 300-item batch with an unresolvable target could run ~1,200 lookups. The comment and the commit message both overstated the code. The resolved collection is now threaded through applyBulkOp into bulkMoveCollection, an unresolvable target fails the request up front instead of once per item, and the claim matches the implementation. That last one is the failure I keep meeting from different sides: the code was defensible and the sentence describing it was not true. Worth naming plainly rather than quietly fixing, because a reviewer reading that comment would have had no reason to check. * fix(server): revert the CollectionIDs pin — it was a visibility leak, not a scope filter (BUG-2578) Codex round 4. The P1 is a hole I opened one commit earlier, and it is the worst thing on this branch. To close a slug-reuse race I "pinned" the collection-item list by setting params.CollectionIDs to the resolved collection, and wrote a comment asserting the two filters were ANDed so a concurrent rename would fail safe. I did not read the query. CollectionIDs and ItemIDs are a PERMISSION PAIR and the store combines them with OR — "in a fully-granted collection, OR specifically granted". So pinning CollectionIDs while the item-grant branch of the same handler set ItemIDs rewrote the caller's grants into `collection_id IN (this) OR id IN (granted)`, handing a caller whose only claim on the collection is ONE item grant every item in it. Reverted. The race it was meant to fix is filed as BUG-2631, WITH the reason this fix is wrong, because setting CollectionIDs is the obvious move and the next person will reach for it too; the real fix needs a scoping parameter distinct from the permission pair. A regression test now covers the leak over both auth classes, and it fails with the ungranted sibling in the response body when the pin is reinstated. Every other test in that file uses an unrestricted owner, which is precisely why none of them noticed — the property was invisible to the whole fixture family I had been writing. Two round-4 P2s, both fixed: The bulk endpoint refused an unresolvable target with a 400 while an existing-but-hidden target failed per item inside a normal 200 envelope. That status difference is an existence oracle — a restricted caller can probe slugs and learn which collections they may not see exist. Unresolvable targets now take the same per-item path, which is also the pre-change behaviour, and a test asserts the two responses are indistinguishable. items-index discarded the resolver's error and continued with the raw alias, answering a database failure with a successful EMPTY index. It now surfaces the error. The lesson I am taking, since it is the second time today the same shape bit: I asserted a mechanism (AND semantics) in a comment without reading the code that implements it, and the comment made the change look considered. Last time that produced a wrong explanation on a trail; this time it produced a permission bypass. * docs+test: correct three overstatements and strengthen the oracle test (BUG-2578) Codex round 5. Three of the four findings are my own prose claiming more than the code does — the same failure mode this branch has now produced four times, so it is worth fixing rather than shrugging at. The resolver's doc said a singular form works for "every collection". It handles a trailing ASCII `s`, so `spec`/`specs` resolves and `category`/`categories` does not. The doc now says "a regular singular/plural pair", names the limit, and points at the paragraph explaining why -s is a deliberate stopping point rather than a gap to close with an inflector. bulkMoveCollection's doc said its targetColl parameter "is never nil". The immediately preceding commit made it deliberately nil for an unresolved target — that is what keeps a hidden and a nonexistent collection failing identically — and the function has a nil check three lines down. Now says so. The MCP test's comment implied the transport. It drives the dispatcher against a real in-process server, which proves the resolution reaches an MCP tool call; it does not go over the remote /mcp HTTP transport or its OAuth layer. Scope stated in the test so nobody reads more into a green run. The fourth is a real test weakness: the existence-oracle test compared only HTTP status, so an implementation returning both cases inside a 200 envelope with different error codes would have passed while still leaking. It now compares the per-item failure shape too, with item ids stripped since those legitimately differ, and a non-JSON body compared verbatim rather than normalized to empty — which would have made two different errors look identical. Mutation-verified: changing only the unresolved-target error code, leaving the status alone, now fails it. Round 5's P1 — that cross-workspace copy requires workspace-level edit on the destination before any collection-grant check, so a destination collection grant is unusable — is NOT addressed here and is not mine to judge on this branch. The ordering predates this diff (I only swapped the lookup call), and the scope constructor is explicitly named CrossWorkspaceWorkspaceOnlyScope, which reads deliberate rather than accidental. Raised with the lead as an unverified observation rather than filed as a defect, since I have not read PLAN-2357's authorization design and would be filing a design question dressed as a bug. * test: read the failure field the endpoint actually emits (BUG-2578) Codex round 6. normalizeBulkFailures decoded failed[].message; the endpoint emits failed[].error (bulkItemFailure). So the message half of the existence-oracle comparison decoded to the empty string for every row and compared equal always — dead since the moment I added it to close exactly that gap, and my mutation had changed the code AND the message together, so it failed on the code and told me nothing about the message. Fixed, and re-verified with a mutation that leaves the status and the error code identical and changes only the message: it now fails. The struct carries a note that the field names mirror bulkItemFailure, since an invented name here fails silently rather than loudly. Third time on this branch that a test I wrote to be rigorous was not, and the tell each time was that I checked it passed on good code without checking WHICH part of it could fail. * fix(server): an archived collection blocks the alias instead of handing its name away (BUG-2578) Codex round 7, and it took a real judgement call rather than a mechanical fix. GetCollectionBySlug skips soft-deleted rows, so with an archived `spec` alongside a live `specs`, the exact lookup missed and the alias fallback picked up `specs` — archiving a collection would quietly start routing its writes into a different one, and a later restore would leave those items stranded where they were rerouted. I first read this as acceptable: an archived collection is not a writable target, so resolving to the live neighbour looks like the alias feature doing its job. What decided it the other way is that this branch already refuses exactly this trade on the client side. BUG-2630's whole complaint is that a silent misroute into a different collection is worse than an honest error, and the same reasoning cannot be right there and wrong here just because the redirect happens to be convenient. Archived rows now claim their name: the exact form returns not-found rather than falling through. The narrow store method (ArchivedCollectionClaimsSlug) answers a boolean rather than returning the row, because an archived collection is never a valid target — it only blocks the name, and returning it would invite a caller to use it. Covered end to end with the fixture armed first (the collection resolves to itself while live, so the assertion is about the archive edge and not about the resolver being broken generally), and mutation-verified: removing the guard fails it with the item sitting in `specs`. * fix(server): run the archived-name guard for every candidate, not just the input (BUG-2578) Codex round 8. The previous commit checked the archived claim only for the raw input, so an archived `spec` beside a live `specs` still let `Spec` through: the exact form missed, the case-folded candidate `spec` found no LIVE row (GetCollectionBySlug skips soft-deleted), and resolution walked on to `specs`. The archived name was stepped over by a spelling of itself. Restructured so the sequence is uniform — the raw input and every fallback ask the same two questions in the same order, is there a live collection with this name and does an archived one claim it. That is also easier to reason about than a guard bolted in front of a loop, which is how the hole existed. Mutation-verified with the previous shape restored: guarding index 0 only fails the new test with the item sitting in `specs`. |
||
|
xarmian
|
2c8ddffcb0 |
fix(store): cover documents and comment bodies in the attachment reference walks (BUG-2614, BUG-2615) (#1145)
* fix(store): cover documents and comment bodies in the attachment reference walks (BUG-2614, BUG-2615)
Two defects with one shape: a content surface that carries `pad-attachment:`
references was missing from a walk meant to cover every such surface. Both were
found by Codex during BUG-2415 and both predate it.
BUG-2614 — the orphan GC could reclaim a live reference. AttachmentReferenced
counted items and comments; documents.content was never scanned, and neither
document write path stamped. An attachment referenced only from a document was
therefore both invisible to the sweep's scan AND unprotected by the stamp that
covers references landing mid-sweep.
The filing asked whether the documents surface is dead enough to delete instead
of widening the scan. Evidence says widen, and I am not making the deletion
call inside a bug fix: /workspaces/{ws}/documents has full CRUD mounted and
authenticated today (list/create/get/patch/delete plus restore, versions and
activity), so a direct API consumer can still write one. It IS legacy — the
route block says "v1 — will be replaced by items in Phase 2" and no first-party
client reaches it (zero references in the web API client and in cmd/pad) — and
production carries 4 document rows, all soft-deleted, none referencing an
attachment, newest touched 2026-04-27. "Reachable but unused by us" is not
dead, and the conservative fix is a few lines. Retiring the surface belongs
with the Phase 2 migration, deliberately.
CreateDocument had no transaction, so it gains one: the stamp has to commit
atomically with the content carrying the reference or it cannot serialize
against a concurrent claim, which is the whole point. UpdateDocument already
had a transaction and only needed the call — and only when content is actually
written, since a metadata-only PATCH neither adds nor keeps a reference and
must not vouch for one.
BUG-2615 — the bundle import's remap rewrote item content and fields but not
comment bodies, so an imported comment kept the SOURCE workspace's ids: broken
references in the destination, and the rehydrated rows they should point at
left referenced by nothing. Bundles do carry comments (export.go exports them,
ImportWorkspace re-inserts them); they carry no documents, so this stays scoped
to comments.
The remap also now stamps what the rewrites point AT. ImportWorkspace already
stamps each comment body at insert, but the body still holds the source ids
then and the remap runs later in the handler, so those stamps land on nothing
that ends up referenced — leaving a fresh clone referenced only by text the
transaction just wrote and carrying no stamp, which is exactly the shape the
never-attached claim reclaims. The REWRITTEN TEXTS are passed rather than every
id in the map, so a clone nothing references is not vouched for and does not
survive an extra GC window.
Seven negative controls, one mutation at a time, each failing exactly the test
that covers it: the documents scan leg, each of the two stamps, the comment
write-back (at store and end-to-end level), the remap stamp, and an over-broad
stamp-the-whole-map variant that the precision test catches. Per the standing
bar out of BUG-2301, every regression test here was RUN against the unfixed
code and observed to fail — including the end-to-end bundle fixture the filing
asked for, whose item deliberately carries no reference so that the items walk
alone cannot rescue it.
* fix(store): stamp before the remap's content writes, and state the caller precondition (BUG-2615)
Codex round 2, two P1s.
The first is mine and is a straight violation of the protocol I was mirroring:
I stamped AFTER the item and comment UPDATEs. stampAttachmentRefsTx's own
contract says to call it before the content statement, for two reasons that
both bite here. On Postgres the stamp row-locks the attachment rows for the
rest of the transaction, so a concurrent GC claim blocks and re-evaluates
against the fresh stamp — stamping last instead lets a claim delete the target
while the rewritten text is still uncommitted, after which the stamp matches
zero rows and the transaction commits a dangling reference. And every other
writer takes attachments before content rows, so writing content first inverts
the lock order and deadlocks against them. The texts are known as soon as both
scans finish, so the stamp simply moves up.
The second — the scan-then-write over comments has no row lock and no
old-value predicate, so a concurrent edit committed in between is clobbered —
is real as a shape but not reachable at the only call site, and is NOT fixed
here. The bundle import runs this against a workspace it has just created,
which no other session can reach yet: there is no concurrent writer to lose an
edit to, and no contention for the long transaction to hold up. The
pre-existing items walk has the identical shape, so this is a property of the
function rather than of the comment leg. Adding row locks or a compare-and-swap
would be machinery for an unreachable window.
What that argument does require is that the precondition stop being tribal
knowledge, since it is about the CALLER and the next caller is exactly who
would break it. It is now stated at the top of the function, where someone
adding a second call site reads it, rather than in this message.
Also declined, both pre-existing and neither introduced here: the one-transaction
scan of the whole population (same reasoning — one caller, fresh workspace), and
document slug allocation outside the create transaction, which predates the
transaction existing at all and yields a spurious unique-violation rather than
partial state.
NOT COVERED BY A TEST, stated rather than implied: the stamp ORDERING. The
existing guard asserts the stamp is present and fails without it, but it reads
end state, so it cannot distinguish before-the-writes from after. Proving the
order needs a concurrent-session Postgres instrument of the kind BUG-2409 used;
that is not built here. The ordering rests on the reasoning above and on the
contract documented at stampAttachmentRefsTx.
* docs(store): make the scanned-surface set an explicit contract (BUG-2614)
Codex round 3 P2. Both comments a maintainer reads still described the scan as
covering items and comment bodies — AttachmentReferenced's doc, and the
orphan-GC sweep's "Item content references the attachment" branch — so the
change that added documents left the two artifacts that explain it stale. Same
class as the sentinel comment on BUG-2301: the code was right and the text
someone acts on was not.
They now also say the thing neither said before, which is why this defect
happened twice: the SET of scanned surfaces is the contract. Any surface that
persists user-authored text containing a `pad-attachment:` token has to be
listed there, and adding one without adding it here silently makes its
references invisible to the GC. Comments (IDEA-1650) and documents (BUG-2614)
were both found after the fact, which is the argument for writing the rule down
rather than the two instances.
Round 3's P1 — restore paths do not re-stamp, so a reference reclaimed while
archived is dangling after restore — is filed as BUG-2629, not fixed here. It
is pre-existing and uniform: RestoreItem does not stamp either, so fixing only
RestoreDocument would leave the larger hole open while making documents
inconsistently better-protected. The filing records the asymmetry that decides
its priority: items are usually shielded by the claim's own item_id IS NULL
predicate, while a document-referenced attachment has no document_id column to
be shielded by and is always claimable.
* docs(store): mark the unstamped rename cascades in place, pointing at BUG-2629 (BUG-2614)
Codex raised the title-rename cascade's missing stamp in two separate rounds
despite being told it was filed. Being raised twice is the signal that the
disposition was only in a bug tracker and not where a reader of this code
meets the problem — the same correction BUG-2301 ended on.
Both sites now carry it: documents.go::updateLinksInTx and
wiki_links.go::cascadeTitleRename, each naming BUG-2629, why it is not fixed
here (uniform across both surfaces, so half-fixing makes them inconsistent),
and why it is the weakest member of that family (the cascade rewrites link text
in content whose references were already stamped and are still visible to the
scan, so a genuinely new reference needs a title containing a pad-attachment
token).
Comments only.
|
||
|
xarmian
|
6f16003199 |
fix: surface implementation notes + decision log in the item timeline (BUG-2301) (#1144)
* fix(server): merge implementation notes + decision log into the item timeline (BUG-2301) `pad item note` and `pad item decide` have written structured entries since |
||
|
xarmian
|
8798de7e99 |
docs: correct the onboard playbook's mode vocabulary in CLAUDE.md (#1142)
The Onboarding section described "four modes: build/audit/revisit/ defaults". The playbook's actual arguments declaration (playbook_library_onboard.go) is a mode enum of auto/build/audit/revisit (auto default, routing any user-created item to revisit) with `defaults` a separate fast-path FLAG, not a mode. This error propagated into BUG-2574's body and from there into a skill rewrite before Codex caught it against the source (PR #1139 round 1); fixing the origin so it can't propagate again. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V |
||
|
xarmian
|
aa1a4f9d8c |
fix(store): workspace-scope the orphaned-variant GC's parent check (BUG-2622) (#1143)
A live variant whose parent_id resolves to a LIVE row in ANOTHER workspace — malformed data with no FK or same-workspace constraint behind it, the class PLAN-2397 repairs — was shielded by that foreign parent in both halves of the orphaned-variant class: the candidate SELECT's NOT EXISTS and ClaimOrphanedVariantAttachment's delete-time re-assert read any live parent as protective. An item-bound malformed variant then matched no GC class at all, forever (an unbound one eventually ages into the never-attached class, so the item-bound shape is the real leak). Per DR-11a's rule one level down — already enforced by the copy planner's attachmentVariantsInWorkspace for the same reason — a foreign row is not a legitimate parent, so both predicates now require p.workspace_id = attachments.workspace_id; in the claim, ErrNoRows covers hard-gone and foreign alike, and a foreign parent's restore needs no serialization since its liveness is irrelevant to this row either way. Deleting the malformed variant touches nothing in the foreign workspace (the child holds the pointer) and bytes stay behind the hash-protection + in-flight fences downstream. Test: TestOrphanedVariant_ForeignParentDoesNotShield — both legs (candidate SELECT + claim) verified failing pre-fix on the item-bound shape, with a live same-workspace-parent control pinning that the fix did not widen into reclaiming healthy thumbnails, and the foreign parent asserted untouched. Found by Codex round 4 of PR #1139 (out of that PR's scope; filed then fixed separately). Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V |
||
|
xarmian
|
a963e68395 |
docs(skills): de-assume the slash-command surface + route onboard shortcut through the canonical playbook (BUG-2573/2574/2575) (#1139)
* docs(skills): de-assume the slash-command surface + route onboard shortcut through the canonical playbook (BUG-2573, BUG-2574, BUG-2575)
Three coherent drift fixes across the two skill trees:
BUG-2573 — skills/pad/SKILL.md is the embed source `pad agent install`
writes for Claude Code, Codex, Cursor, Windsurf, OpenCode, Amazon Q,
Junie AND pure-MCP agents, but three sentences presented the Claude Code
slash command as THE invocation ("There is one command: /pad <anything>",
"On every /pad invocation", "the first token after /pad"). Reframed per
the PLAN-1847 house pattern: natural language is canonical, typed forms
are per-surface shortcuts, and a read-/pad-as-shorthand rule covers the
rest of the document. Verified through the installed artifact, not just
the diff: built the binary and ran `pad agent install codex` — the
reframed text reaches the non-Claude skill verbatim.
BUG-2574 — plugin/skills/onboard/SKILL.md (the most direct onboarding
route a plugin user has) inlined its own post-link setup script, silently
opting that surface out of the workspace-owned, user-editable onboard
playbook — a customized playbook never fired via the shortcut, and the
inline copy covered roughly the build mode only. The post-link half now
loads and follows the playbook (with exact-title library activation —
`pad library activate "Onboard a workspace"`, verified against the CLI's
actual arg form) and routes needs_onboarding=false to the playbook's
revisit mode. The pre-link whoami-gated half stays as BUG-2541 left it.
Checked the other dedicated plugin skills for the same class: status and
capture inline nothing playbook-owned — no change needed.
BUG-2575 — plugin/skills/pad/SKILL.md didn't know specs are decomposable:
added the "break SPEC-1 into tasks" routing entry and the plan-or-spec
wording in the decompose workflow, matching decomposePlaybookBody and the
embed source. Also synced the one other surface-agnostic drift found in
the sweep: the convention_index note that a list without --full has no
content field.
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
* docs(skills): fix onboard-skill mode enum, needs_onboarding semantics, and reactivation path per Codex review (round 1)
Three corrections to the rewritten post-link half, all verified against
playbook_library_onboard.go: the mode enum is auto/build/audit/revisit
(auto default) with `defaults` a separate fast-path flag — the "four
modes incl. defaults" framing came from the tracking bug's own body and
was wrong; needs_onboarding:false only means a user-created item exists,
not that onboarding ever ran, so the skill no longer declares setup
complete on it; and a draft/deprecated onboard playbook must be
reactivated in place, since invocation_slug is workspace-unique and
library activation beside an existing entry duplicates or fails.
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
* docs(skills): activation before load + honest auto-mode routing per Codex review (round 2)
The post-link half now runs as an ordered three-step: ensure-active
(reactivate in place, library only when absent), THEN load the body,
then mode framing — a literal reader of the previous text ran
`pad playbook show onboard` before the existence check, failing on
missing playbooks and loading stale drafts. And the mode note no longer
claims auto picks "a fuller pass": verified against the playbook's
pre-flight, auto routes ANY user-created item to revisit, so the skill
now says to pass an explicit mode=build/audit override (which the
playbook honors) when the user says the workspace was never really set
up.
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
* docs(skills,mcp): propagate activation ordering + auto-mode routing to the sibling onboarding routes per Codex review (round 3)
The round-2 corrections lived only in the focused onboard skill; the
embed skill's Onboarding entry, the plugin pad skill's, and the
pad_onboard MCP prompt still said load-then-activate with a bare
library-activate fallback, and none warned that the playbook's auto mode
routes any workspace with user-created items to revisit. All three now
carry the same semantics: ensure-active first (reactivate a
draft/deprecated entry in place — invocation_slug is workspace-unique,
so library activation beside an existing entry duplicates or fails),
then load, plus the explicit mode=build/audit override note.
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
|
||
|
xarmian
|
08dfbdb318 |
fix(server): rowless-blob GC sweep — reclaim blobs no attachments row references (BUG-2406) (#1137)
* fix(server): rowless-blob GC sweep — reclaim blobs no attachments row references (BUG-2406) Every attachment write path calls AttachmentStore.Put BEFORE inserting the attachments row, so a failure (or crash) between the two leaves a blob on disk that nothing references — and the row-driven orphan sweep, which walks Store.OrphanedAttachments, can never see it. Disk that is never returned; the upload handler's failure comment even claimed the GC would reclaim it. Fix: a rowless-blob sweep that runs after the row sweep on the same GC tick. attachments.Lister is a new OPTIONAL backend capability (ListBlobs → key/hash/size/mtime); FSStore implements it via one WalkDir of the sharded tree with a base-name validHash gate (excludes Put's dot-prefixed temp files and anything the store didn't write). Backends without the capability are skipped with a once-per-process notice. Candidate = blob whose content hash has ZERO rows in ANY state (soft-deleted rows still own their bytes under the row sweep's row-before-bytes claim protocol, BUG-2415) AND whose mtime predates the same operator-configured GC grace the row sweep uses — a young rowless blob is just an upload whose insert hasn't happened yet. Delete-time guards run under inFlightHashesMu: the in-flight fence plus a single-hash row RE-CHECK that closes the subtraction-to-delete TOCTOU (the writer that marked, inserted, and released entirely inside the gap). Cost: O(blobs) per tick, 24h cadence, never on a request path. Also retro-reclaims blobs stranded by past row-sweep delete failures. The wrong claim in handleUploadAttachment's failure path is corrected to point at this sweep. Tests: FSStore.ListBlobs impostor coverage; five sweep legs (aged-rowless reclaimed with a row-sweep-can't-see-it counterfactual, young kept, live/soft-deleted-row kept, in-flight kept then reclaimed after release, hook-injected delete-time row kept) — mutation-verified: removing the re-check, the age gate, or the in-flight fence each fails its leg; the store-level subtraction contract is pinned separately. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(store): state the any-row rule's real rationale per Codex review (round 1) Codex flagged the thumbnail refusal-cleanup's grace-window protection as inconsistent with the sweep comment's claim that deleting bytes under any existing row violates the claim protocol. The cleanup (and the row sweep itself) deliberately end a row's hash-protection when its own grace expires — CountProtectingAttachmentsForHash documents exactly that, and the row machinery may do it because its claim protocol coordinates row and blob fates within a sweep. The overstatement was mine: the rowless sweep's any-row rule is chosen because it holds no claim on any row and has no such coordination, not because past-grace stranding is forbidden to the machinery that does. Comment corrected; no behavior change on either path. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V |
||
|
xarmian
|
31075d996a |
fix(store): route cross-workspace copy's lock-held reads through the copy transaction (BUG-2409) (#1136)
* fix(store): route cross-workspace copy's lock-held reads through the copy transaction (BUG-2409) The copy transaction holds advisory locks on BOTH workspaces, but the attachment planner (PlanAttachmentCopy) and the server's per-row attachment authorizer read through the connection pool. Under enough concurrent copies every pooled connection can be occupied by a lock-waiter while the lock holder waits for a spare connection — starvation presenting as a hang. Fix: a store.Queryer interface (satisfied by *sql.DB and *sql.Tx) threaded through the planner and the AttachmentAuthorizer callback, so the mutating copy plans and authorizes on its own transaction's connection while the preflight keeps planning through the pool — one implementation, two executors, preserving TASK-2354's no-drift shape. Mechanical *Q variants added for the store reads the authorizer transitively needs (GetItem, GetUser, GetWorkspaceMember, VisibleCollectionIDs, GetMemberCollectionAccess, ListSystemCollectionIDs, GuestVisibleCollectionIDs, GuestVisibleResources(+IncludeDeleted), ResolveBacklinksVisibility) and Q-cores behind existing-signature server wrappers (checkItemVisible, guestResourceFilterCore, resolveAttachmentParentItem, attachmentCallerIsRestricted). No decision logic changed anywhere — executor threading only. GetItem/getItemTx/GetItemIncludeDeleted's three duplicate scan bodies collapse into one getItemScanQ. Regression test: TestCopyItemAcrossWorkspaces_NoPoolIOUnderLocks pins the invariant deterministically — with MaxOpenConns(1) the transaction owns the only connection, so ANY pool read under the locks deadlocks. Fails by timeout on the pre-fix executor (verified); passes in 0.16s fixed. The test's authorizer performs a real read through the handed Queryer, pinning the callback leg too. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(store): quota check reads through the copy transaction too, per Codex review (round 2) Codex's targeted round found the third lock-held pool-read leg: CheckLimitTx routed only the feature COUNT through the caller's transaction while checkLimitOn's owner lookup, GetUser, and resolveLimit's platform-setting read stayed on the pool — the same starvation shape under the copy's advisory locks. checkLimitOn is now parameterized over a single Queryer for every read (CheckLimit passes the pool, CheckLimitTx the transaction), with resolveLimitQ / GetPlatformSettingQ variants behind existing-signature wrappers. The regression test now arms this leg deliberately: a FREE-plan owner with EnforceItemLimit and no plan override drives the full quota read chain under MaxOpenConns(1) — verified deadlocking before this commit, 0.16s after. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V |
||
|
xarmian
|
cc26288794 |
fix(web): share pages render attachment refs as honest placeholders (BUG-2389) (#1135)
The public share route (/s/{token}) rendered item content with a bare
marked() call, so pad-attachment: references fell through as broken
<img src="pad-attachment:..."> tags and dead links. Two halves:
1. CommentThread.svelte is deleted outright — grep proved it was
unmounted dead code (its only reference was a prose mention in
ItemDetail.svelte), so its half of the bug resolves by deletion
rather than by fixing a component nothing renders.
2. The share route now renders through a new opt-in wrapper,
renderMarkedWithAttachments(), which threads an AttachmentRenderContext
into the existing marked renderer hooks. With a null resolver and the
new renderAttachmentUnavailable() placeholder, every ref becomes an
honest "Attachments aren't available on shared pages yet" chip —
deliberately NOT the "missing or has been deleted" wording, because
the attachment exists; the share surface just cannot serve its bytes.
Sanitization is unchanged: the wrapper returns unsanitized HTML and
the share page keeps its single DOMPurify pass.
The `missing` hook is a parameter (default: renderAttachmentMissing) so
authed surfaces keep their existing wording, and the wrapper clears the
module context in a finally block so bare marked() callers are
unaffected (pinned by test).
The token-scoped byte endpoint that would serve real images on share
pages (2b) is deliberately NOT built here — it adds a new
unauthenticated ACL surface and is tracked separately pending approval.
A real resolver through the same wrapper is the plug-in point (pinned
by test).
Tests: markdown.shareAttachments.test.ts (6 unit legs incl. bare-marked
opt-in control and context-clearing) and
bug-2389-share-attachment-placeholder.spec.ts (e2e: real upload → item
ref → item share link → anonymous visit; verified failing on the
pre-fix build).
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
|
||
|
xarmian
|
e0c5792ce9 |
fix(store): attachment delete vs thumbnail derivation race — atomic cascade, locked conditional insert, orphaned-variant GC class (BUG-2388) (#1134)
* fix(store): attachment delete vs thumbnail derivation race — atomic cascade, conditional variant insert, orphaned-variant GC class (BUG-2388) Deleting an attachment while thumbnails were still deriving could mint a live, unreachable variant row under a tombstoned parent: the delete cascade tombstoned original and variants in separate statements, and derivation checked parent liveness once, then inserted uncondition- ally. The leaked row was invisible in the UI, counted toward quota forever, and no GC class could reclaim it (the old code's comment claimed a 'deleted-parent path' existed; it did not). Three parts, all the BUG-2415 claim-by-statement discipline: - SoftDeleteAttachment tombstones original + variants in ONE transaction. - CreateAttachmentVariantIfParentLive makes the parent-liveness check part of the variant INSERT itself (INSERT..SELECT WHERE EXISTS parent live); persistThumbnail cleans up the just-Put blob on refusal under the in-flight hash fence it already holds, honoring the same hash-dedupe protections as the sweep. - Orphan GC gains the orphaned-variant class: live variant whose parent is tombstoned/gone, tried FIRST for live parented candidates (an item_id-NULL leak would otherwise hide behind a content reference to its dead parent in the never-attached scan). The claim re-asserts parent-not-live at delete time, so a concurrent parent restore wins and a restored original keeps its thumbnails. This class also retro-reclaims rows already leaked. Tests: the filed race pinned deterministically (persistThumbnail with a pre-delete parent snapshot — control build mints the leaked row verbatim); retro-reclaim sweep test with a restore-wins leg, its leak fixture deliberately ATTACHED so only the new class can reclaim it (control build: row survives). * fixup: codex round 1 — parent row-locks on the conditional insert + variant claim (CreateAttachmentForLiveItem precedent), fenced+config-aware refusal blob cleanup, store-level restore-refusal claim test, blob-cleanup assertion * fixup: count inside the in-flight fence — a completed upload lifecycle could stale an outside count (codex round 2) |
||
|
xarmian
|
2e4f3d5dc2 |
fix(server): refuse a PATCH carrying both a fields hierarchy key and top-level parent_id (BUG-2594) (#1133)
* fix(server): refuse a PATCH carrying both a fields/fields_patch hierarchy key and top-level parent_id (BUG-2594) extractParentLink staged the item_links write (including the empty- string clear) while ItemUpdate.ParentID stamped the parent_id column unconditionally in the same transaction — one request could clear the link AND re-parent the column, leaving silently inconsistent hierarchy state (unparentedItemPredicate still saw a parent). The shape is raw-HTTP-only: no first-party client sends top-level parent_id on item update (CLI resolves --parent into the patch; the web client and MCP catalog never carry it). Both update paths (full fields + fields_patch) now refuse the pair with a validation error naming both keys — refused, not silently resolved, per the clear_parent contract family's standing rule (v0.19). Solo parent_id and solo fields-patch hierarchy writes are deliberately unchanged (BUG-2379 tracks the adjacent undeclared- override family). Six handler tests: refusal on clear+id, set+id, the plan alias, and the full-fields sibling path — each verified failing (200) on the unguarded control build — plus both solo-write controls. * fixup: assert the validation_error code + plan alias in the refusal envelope (codex round 1) |
||
|
xarmian
|
2521e3e1c7 |
fix(web): portal the pane action-bar menus — anchored panels clipped against the pane's scroll container (BUG-2610) (#1132)
* fix(web): portal the pane action-bar menus — anchored panels clipped against the pane's scroll container (BUG-2610)
In split view the item pane is an overflow-y:auto scroll container,
which computes overflow-x:auto too — the quick-actions (⚡) and ⋯
menus were ANCHORED panels inside it, and a right-aligned panel
opening from the pane's action bar extends left past the pane's edge,
so the container clipped it mid-text (Dave's screenshots: 'age
actions' for 'Manage actions', truncated tagline).
Both menus now use the Menu component's portal mode — built precisely
to escape overflow containment (fixed coords portaled to <body>,
viewport clamping, flip-when-cramped, scroll dismissal), and already
the mode of every board-card menu. Widths cover each menu's content
(QA: the 230px qa-body min-width + chrome; ⋯: the longest row).
Regression e2e uses a PAINT-level oracle — clipping doesn't shrink
getBoundingClientRect, so elementFromPoint just inside the panel's
left edge must resolve to the panel; verified failing on the anchored
control build with the exact reported symptom, plus a geometry
precondition so the probe can't pass vacuously. Existing e2e + unit
lookups that scoped menu rows under .item-pane / the master column
are page-scoped now (the portaled panel lives in <body>; only one
menu is ever open, and the scoped trigger click is what ties it to
its column).
* fixup: scope portal scroll-dismiss to anchor-moving containers — any-scroll dismissal closed pane menus under live SSE churn (found via parallel e2e instability vs a clean control build)
* fixup: codex round 1 — route nav-key bail includes portaled [role=menu] (pre-existing leak for board menus too), stopPropagation on handled menu keys, exempt-aware scroll dismiss, per-menu e2e geometry preconditions
* fixup: page-scope the two graph-drawer menu lookups in pane-content-link-anchors (codex round 2)
|
||
|
xarmian
|
fbea9484d6 |
fix(web): source-identity guards on the SSE non-sync listeners (BUG-2611) (#1131)
close() does not retract already-queued event tasks, so on a fast workspace switch a torn-down EventSource's queued events could fire after the next workspace's source existed. BUG-2540 guarded onopen / onerror / 'connected'; the remaining listeners had no guard, so workspace A's stale events dispatched into workspace B's callbacks — spurious sync passes and cross-workspace item events fanned onto B's BroadcastChannel, and the sharp member: a stale 'unauthorized' closed B's LIVE EventSource, flipped the status indicator, and cleared currentWorkspace over A's auth state, with nothing reconnecting until a navigation. Same one-line guard on all four (sync_required, items_bulk_updated, unauthorized, the ITEM_EVENTS loop); unauthorized additionally closes its OWN source rather than whatever eventSource currently points at — the guard has just proven they are the same, and the old shape is what made the stale path destructive. Unit harness (BUG-2540's stubbed-EventSource pattern): fast A→B switch, fire each event type on the torn-down source — zero dispatch into B — with a live-source control arm per leg so a guard silencing both cannot pass. All four legs verified failing on the unguarded build. |
||
|
xarmian
|
d68474f775 |
feat(server): armed-session declaration + push delivery filter (PLAN-2613 S1, TASK-2616) (#1130)
Adds a server-side consent gate for push delivery ahead of the plugin/CLI version flip: a stream now declares armed=true at connect (query param) to receive KindPush notifications, while legacy (unarmed) streams keep ordinary watch-matched delivery during the skew window. LiveSession exposes the armed bit so the web target picker can eventually show honest accepting-pushes counts, and push delivery counts are now armed-aware end to end (broadcast, targeted, and the pre-publish snapshot used to skip a guaranteed no-op). |
||
|
xarmian
|
8cdeeb166b |
fix(store): orphan GC claim protocol — writer reference stamps + conditional row-first deletes (BUG-2415) (#1129)
* fix(store): orphan GC claim protocol — writer reference stamps + conditional row-first deletes (BUG-2415) The sweep scanned content for pad-attachment: references, then deleted the BLOB, then the row — with nothing serializing it against content writers. A reference committing between scan and reclaim left either a dangling id or, worse, a surviving row whose bytes were already gone. Claim protocol: - attachments.last_referenced_at (dual-dialect migration): every content writer that persists a pad-attachment: reference stamps the rows INSIDE its own write transaction (stampAttachmentRefsTx), wired at the four store chokepoints every surface funnels through — CreateItem, the UpdateItem core (item PATCH, collab-snapshot flush, version restore, bulk update), CreateComment, UpdateComment (both now transactional). Workspace-scoped; covers content AND fields, matching AttachmentReferenced's scan surface. - The sweep's row deletion is now the atomic claim: a conditional DELETE re-asserting reclaimable state in the statement itself (ClaimNeverAttachedAttachment: unattached + live + no fresh stamp; ClaimSoftDeletedAttachment: still deleted + still past grace, so a mid-sweep restore survives too). Writer stamp and claim serialize at the database; whichever commits first wins and the loser observes it. - Row BEFORE bytes: the blob is reclaimed only after a successful claim, so a surviving row implies surviving bytes — the old order's worst failure mode (row without content) is structurally impossible. - orphanGCRefStaleWindow (15m) is documented as a correctness parameter: the stamp only covers references landing after the scan, so the window bounds scan-to-claim latency plus a maximally stalled writer transaction — not a lease on long-lived references (the LIKE scan still guards those). Sweep-level test pins the filed race (fresh stamp survives sweep, row AND blob) with a counterfactual arm (aged stamp reclaims); verified discriminating against a compiling control build of the old sweep order. Store tests cover every claim predicate leg, stamp wiring on all four chokepoints, and workspace scoping. * fixup: codex round 1 — stamp move-override + workspace-import paths, parent-aware variant protection (scan by parent id + claim NOT EXISTS fresh parent stamp), variant test with total-loss control * fixup: codex round 3 — stamps ordered BEFORE content statements (PG row-lock makes the claim wait out the writer tx), chunked stamp IN-lists, BlobDeleteFailures counter * fixup: codex round 4 — stamp variants of referenced originals (own-row lock protects concurrently-claimed thumbnails), bounded-duration residual + irrevocability docs |
||
|
xarmian
|
54526c5b33 |
fix(web): route ItemDetail collection writes through a semantic adopt gate (BUG-2602) (#1128)
* fix(web): route ItemDetail collection writes through a semantic adopt gate (BUG-2602) Seven sites assigned ItemDetail's collection snapshot under fences that ordered STARTS, and loadData's cross-collection escape hatch admitted any generation-stale write that fetched a different collection — so a loadData continuation spanning a cross-collection MOVE restored the SOURCE collection over the freshly adopted TARGET (the live item, itemGen-fenced, kept the move: the pane rendered the item against the wrong collection's schema). All writes now route through adoptCollection, backed by the pure shouldAdoptCollection decision: (1) a snapshot disagreeing with the LIVE item's collection_id is vetoed regardless of freshness — id, not slug, so renames still apply and a reused slug can't satisfy it (this also closes a latent foreign-write in the SSE collection_updated refresh, which fetched by slug); (2) same-collection refreshes keep newest-started-wins; (3) the legitimate cross-collection correction the old hatch existed for still lands when the live item agrees. On the veto path, an embedded pane whose collection was still null (fresh mount — refreshCollectionIfMoved's !collection guard skips there too) converges on the live item's collection instead of being left schema-less (adoptOrConvergeToLiveCollection); non-embedded masters stay route-authoritative per the existing policy. e2e reproduces the filed race deterministically (route-hold on the realColl fetch, API move mid-hold, release): the control build renders the moved item against SrcMarkerField's schema verbatim; the fixed build converges on the target's. * fixup: codex round 1 — post-converge myGen re-checks, schema-less error surfacing, pre-fetch convergeGen, hard collection_id oracle in e2e * fixup: codex round 3 — empty-string collection_id normalizes to no-anchor, else-branch schema-less surfacing, itemGen re-check before singleton claims * fixup: two stale comments (codex round 5 docs) |
||
|
xarmian
|
904878522a |
fix(web): heal collection renames missed by SSE — sync-pass route reconcile + localIndex retag (BUG-2601) (#1127)
* fix(web): heal collection renames missed by SSE — sync-pass route reconcile + localIndex retag (BUG-2601) Two stranding layers, both from the same root: a collection rename changes the slug without touching items, so nothing item-shaped ever re-announces it. 1. ROUTE: delta-sync catch-up covers item changes only, and /changes says nothing about renames — a rename-only gap even reports caught_up — so a client that missed the collection_updated SSE (replay gap, disconnect) kept a dead route slug; slug-keyed fetches 404'd until a manual navigation. The collection route now reconciles its slug against the live collections list by STABLE collection id on every sync pass (resolveSyncRenameTarget — pure, unit-tested — wired via reconcileRouteCollectionSlug), mirroring the BUG-2272 SSE/reorder-404 heals and sharing the renameNav intent tracker. 2. DATA (discovered by this fix's own e2e, present on the LIVE SSE path too): cached localIndex rows keep the old collection_slug — rows only re-stamp when the item itself changes — so EVERY rename-healed route rendered an empty board while the sidebar counted the items. New localIndex.retagCollection re-stamps rows by stable collection id (search + IDB write-through, upsert pattern), called from the workspace layout's global collection_updated subscriber (any route, live SSE) and from the sync heal (missed SSE, where the layout subscriber also missed the event). e2e covers both paths with an aborted-SSE missed-event leg and a live SSE leg; both specs fail on the pre-fix build (verified) and the missed-SSE spec guards its own vacuity (asserts the strand before triggering the heal). * fixup: codex round 1 — foreign-snapshot gate on sync heal, pendingRetags for pre-hydration renames, layout loadCollections widened, goto-failure renameNav reset, worker-unique e2e slugs * fixup: stamp pendingRetags with recording user's identity; discard on mismatch at warm-hydrate apply (codex round 2) * fixup: heal the full-page item route's collection segment on sync pass too (codex round 3 — the bug body's own example route) * fixup: reconcileCollectionSegment switch-safety — pre-await fence, destroyed guard, identity-compared bridge cleanup (codex round 4) * fixup: compare the read-back $state proxy, not the raw literal (codex round 5) * fixup: codex round 7 — navigating guard on both healers, replaceState on the list heal, null-owner adoption for pendingRetags, in-window vacuity pins in e2e |
||
|
xarmian
|
f0cbcb5df4 |
fix(mcp): route backlinks/history/report over HTTP transport + catalog↔route parity test (BUG-2304) (#1126)
* fix(mcp): route backlinks/history/report over HTTP transport + catalog↔route parity test (BUG-2304) Three catalog actions were advertised on the remote /mcp transport but had no route: pad_item.backlinks, pad_item.history, pad_project.report all answered 'not yet implemented over HTTP transport' on Pad Cloud. All three have working REST endpoints — the gap was mappers, and the absence of any catalog↔routeTable parity check is how they shipped silently. - item backlinks + project report: plain routeSpecs (their CLI JSON is the endpoint response verbatim, so a GET reproduces the stdio shape). - item history: hand-written dispatchItemHistory, because the versions endpoint returns full content bodies while the CLI projects to the token-light itemVersionSummary; full=true opts back in, matching the stdio --full path. - The special-case dispatch switch is now an introspectable specialRoutes() map, and a new parity test drives EVERY catalog action through its real ActionFn, captures the dispatched cmdPath, and fails on any action missing from routeTable ∪ specialRoutes ∪ itemLinkSpecs — or any action the fixture can no longer exercise. No ToolSurfaceVersion bump: no tool names, action enums, or parameter shapes changed — advertised actions now work as documented. Also folds in CLAUDE.md catalog-version drift (still said v0.19; 0.20 shipped in BUG-2302/2305). * fixup: cache specialRoutes map (sync.Once); kind-aware item_not_found on history 404 (codex round 1) * fixup: single request path for history — full=true keeps the kind-aware 404 envelope (codex round 2) * fixup: version.go no-bump changelog note, instructions full:true note, parity-test scope comment (codex round 3) |
||
|
xarmian
|
f756e853fe |
fix(oauth): keep zero-workspace consent authorizable via the wildcard path (BUG-2303) (#1124)
The consent template gated the whole workspace fieldset on the user having memberships; with zero workspaces no access radio rendered and the inline script permanently disabled Authorize — a dead end, even though parseConsentPayload's wildcard path accepts a zero-workspace workspace_access=all consent with no membership validation. Render the 'All my workspaces' radio unconditionally (force-checked when memberships are zero — it is the only option, and an unchecked radio group would re-disable the button), keep the specific radio + picker gated on memberships, and replace the dead-end copy with a pointer at the workspace-creation checkbox so the client can create the user's first workspace. |
||
|
xarmian
|
94441b4eb2 |
chore(nix): bump package version to 0.14.0 ahead of the release tag
Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dtv0.14.0-rc.1 v0.14.0 |
||
|
xarmian
|
cada8777e7 |
fix(web): full-page pane host gets its own navigate-away handling (BUG-2178) (#1123)
The full-page item host reused the shared controller's handlePaneNavigateAway, whose goto targets are collection-host-shaped (the base route IS the collection page). On this host both emits abandoned the master: a pane collection-rename landed on the renamed collection's root page, and a pane item-move hard-navigated to the moved item's full page. The host now wires planFullPageNavigateAway (new pure planner in paneController.ts, next to planPaneDrill and friends): - COLLECTION RENAME (keeps-pane emit): IGNORE. Nothing the host owns is invalidated by the emit alone — if the renamed collection is the MASTER's, the master ItemDetail's own BUG-2272 SSE rename handler already gotos the new-slug URL with the full search string (?item= included), so the route self-heals with the pane intact; a foreign collection never touched the master route. The embedded pane needs no URL change either — it trusts item.collection_slug. - ITEM MOVE (no ?item=): RETARGET the pane to the moved item's slug via the existing drill machinery (navigatePaneTo), which preserves the master pathname by construction and handles depth/ownership/ focus. When ?item= already held the slug (a same-workspace move keeps slugs, so the drill same-ref-guards to a noop) the pane self-heals via the item_updated SSE refetch instead. - Malformed/pathless URLs: IGNORE — staying on the master beats navigating somewhere unparseable. (decodeURIComponent throws on malformed percent sequences; the hostile-input unit test caught that crash before it shipped.) The controller's handlePaneNavigateAway is annotated collection-host- only; every property of its spec comment is untouched for that host. Tests: - planFullPageNavigateAway unit table (both real emit shapes verbatim, encoded slugs, trailing slash, malformed/empty/foreign-origin). - E2E (pane-full-page-capstone.spec.ts): move the PANE item to another collection from the docked pane; assert the pathname never leaves the master route and ?item= retargets to the moved slug. Mutation- verified against a control binary with the old wiring: it fails with ?item= gone and the master abandoned — the reported bug, verbatim. The spec header's BUG-2178 deferred note updated to covered. Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM |
||
|
xarmian
|
900b0c428a |
fix(mcp): summary-shaped item list on the HTTP transport (BUG-2305) (#1122)
A bare pad_item.list over remote /mcp returned up to 50 items with
FULL content bodies — the exact token blowup TASK-2000's limit was
written to prevent, unmitigated on the transport the zero-CLI plugin
makes primary. The limit was symmetric (actionItemList injects it
before dispatch); the SHAPE was not: the exec path projects through
cli.ToItemSummaries in the CLI, while the HTTP path forwarded the raw
handler response.
Fix follows the trail's pre-committed approach (the body's "add a
summary param to mapItemList" is refuted there — the server has no
projection parameter and a RouteMapper has no response hook): a
hand-written dispatchItemList, same shape and same cli.ToItemSummaries
projection as HTTPResourceFetcher.fetchItemList. mapItemList stays the
single URL/filter builder; the routeTable entry is removed so the
hand-written method is the one path. full:true opts into complete
bodies on both transports (stdio forwards it as the CLI's --full).
Audited the other list-shaped actions per the body's ask: pad_search
is symmetric (the STORE zeroes Item.Content in search results —
internal/store/search.go); pad_project.activity returns enrichment
metadata, no content bodies, same endpoint on both transports. Only
item list had the asymmetry.
Also rewrites the misleading actionItemList comment ("summary vs full
is a CLI-side concern") that misdescribed the HTTP path.
Old scope/verified-email fixtures stubbed `{"items":[]}` — an object
shape the real endpoint never returns (it writes a bare array); they
only passed because the routeTable path packaged blindly. Fixtures
corrected to the real shape.
Tests: TestHTTPItemList_DefaultIsSummaryShape +
TestHTTPItemList_FullOptsIntoCompleteBodies drive the REAL server +
store as a counterfactual pair — the marker sits past the
content_preview cut, the full leg proves it flows through the same
path, the default leg proves the projection strips it.
Mutation-verified: removing the dispatch case fails (route gone);
skipping the projection fails (leak caught).
No ToolSurfaceVersion bump here: BUG-2302's PR carries the single
0.19→0.20 bump; this PR appends its lines to that changelog entry
after it lands (lead's sequencing ruling — every changelog sentence
true at its own merge time).
Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM
|
||
|
xarmian
|
727cd80927 |
fix(mcp): explicit tool annotations from catalog write-shape knowledge (BUG-2302) (#1121)
mcp-go's NewTool injects default annotations on every tool — ReadOnlyHint:false, DestructiveHint:true, OpenWorldHint:true — and buildToolFromDef never overrode them, so every Pad tool advertised itself as destructive, including pure reads like pad_search and pad_project. Hosts use destructiveHint to decide whether to prompt; mislabeling reads trains users to click through prompts. Derive the block in buildToolFromDef from the catalog's own knowledge (readOnlyActions — the same single source the tool-surface serializer uses — plus a new sibling additiveWriteActions allowlist): - every action read-only → ReadOnlyHint:true, DestructiveHint:false, IdempotentHint:true (pad_search, pad_project, pad_attachment, pad_meta, pad_playbook); - writes all purely ADDITIVE → ReadOnlyHint:false, DestructiveHint:false (pad_workspace: invite/create/claim/restore; pad_library: activate — codex round 1: marking additive writes destructive reintroduces the prompt-training harm at tool level); - any overwrite/delete-capable action → the conservative ReadOnlyHint:false, DestructiveHint:true (pad_item, pad_collection, pad_role) — unchanged on the wire from the old defaults; - OpenWorldHint:false everywhere (pad tools are closed-world). pad_set_workspace gets a hand-written block (write, non-destructive, idempotent, closed-world). Also adds the missing pad_item.history entry to readOnlyActions — documented read-only since v0.14 but reported read_only:false on the tool-surface descriptor. ToolSurfaceVersion 0.19 → 0.20 (behavior bump, v0.9/v0.16 precedent): no tool names, action enums, or param shapes changed. instructions.md and README headings retitled per the drift tests. The changelog entry describes only this change; BUG-2305 appends to it if it ships in the same window (one bump total). Tests: TestCatalogTools_AnnotationsExplicit pins a literal per-tool read/additive/destructive table (deliberate second enumeration — a new tool, or a write action added to an all-read or all-additive tool, fails loudly until someone decides its class); TestAdditiveWriteActions_NoStaleEntries guards the new allowlist (real catalog pairs only, never overlapping readOnlyActions); TestSetWorkspaceTool_AnnotationsExplicit covers both deployment variants; pad_item.history joins the read spot-checks. Mutation-verified both directions: destructive-polarity flip fails 10 tools; always-destructive fails the two additive rows. Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM |
||
|
xarmian
|
aa33dc407e |
fix(server): serve RFC 9728 PRM at path-aware well-known (BUG-2266) (#1120)
A client configured with the path-suffixed transport URL (https://mcp.getpad.dev/mcp — the shape every FastMCP example uses) constructs its protected-resource-metadata URL per RFC 9728 §3.1 by inserting the well-known segment before the path: /.well-known/oauth-protected-resource/mcp. Pad only registered the exact-match root route, so that request fell through to the SPA catch-all and OAuth discovery died JSON-parsing HTML (Kimi CLI / FastMCP 3.2.4). Register the path-aware route for the two shapes a pasted transport URL actually produces (/mcp and trailing-slash /mcp/), serving the identical canonical document. Bounded rather than a wildcard: the handler emits Cache-Control public max-age, and a wildcard would hand a CDN one cacheable object per attacker-chosen suffix (codex round 2). Deliberately NOT touched: NormalizeAudience / audienceMatchingStrategy (the body's "secondary" fix) — shared by the AS-side strategy and the RS-side token check; widening it is a separate security-boundary item. For the same reason the suffixed doc keeps the canonical bare-host `resource`: echoing .../mcp would steer compliant clients into an audience the AS still rejects (codex round 1, declined — doc-following clients converge on the canonical audience and work end-to-end). Test: TestMCP_DiscoveryDoc_PathAwareWellKnown decodes both suffixed variants into the typed doc and compares field-by-field against the root response (SPA HTML cannot satisfy it), pins that an arbitrary suffix does NOT get the doc, and the path-aware URL joins the cloud-mode-off 404 list. Mutation-verified: with the route lines removed the test fails 404. Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM |
||
|
xarmian
|
9c155ac185 |
fix(cli): gate promptAndBootstrap on canPromptForConfig() (BUG-2597) (#1119)
Third member of the BUG-2577 family (offerSkillInstall #1111, installInteractive #1116): promptAndBootstrap — the legacy --cli-prompt admin bootstrap — guarded its prompts on stdin-only term.IsTerminal, so a pty-backed harness with a redirected stdout got " Email: " printed into the pipe and then blocked on the read. Swap to canPromptForConfig() (stdin AND stdout) with the family's boundary comment; the BUG-988 refuse-with-headless-hint behavior is unchanged. The error message no longer blames stdin specifically ("not running in an interactive terminal") since the widened gate can fire when stdin IS a terminal and stdout isn't; the existing non-TTY test's assertion updated to match. Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM |
||
|
xarmian
|
d843752091 |
docs: worktree web-tooling rules in CLAUDE.md; fix vitest.config.ts's dangling pointer (TASK-2590) (#1118)
CLAUDE.md gains the "Working in a git worktree" section that web/vitest.config.ts:41 has pointed at since the fs.allow fix — it never existed (grep worktree/npm ci/node_modules: zero hits). Content per the corrected day-38 ruling on TASK-2590, not the task's original body: the symlink stays fine and stays the recommendation; the real prerequisite is `npx svelte-kit sync` (a fresh worktree has no generated web/.svelte-kit, and vitest fails on the missing tsconfig either way — the 2x2 on the trail shows the symlink was never the variable); and npm ci through a symlinked node_modules is the one genuinely destructive move (deletes the shared tree, stalls every session), which the original "npm ci, never symlink" rule would have instructed agents to do. Both documented legs verified as written in this very worktree: fresh + symlink -> vitest fails with the exact quoted TSCONFIG_ERROR; npx svelte-kit sync -> same test passes through the symlink (and through this edited config file). Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM |
||
|
xarmian
|
3098a1f569 |
fix(web): search Enter go-to accepts full refs like TASK-1345 (BUG-2128) (#1117)
* fix(web): search Enter go-to accepts full refs like TASK-1345 (BUG-2128) The palette's Enter fast-path only matched bare digits (/^\d+$/, BUG-910), so typing a full ref + Enter fell through to the arrow-selection guard and did nothing. Extract the routing decision as parseGoToTarget() beside REF_PATTERN_RE (pure, unit-tested): bare number keeps its match-any-collection semantics; a PREFIX-N ref (case-insensitive) must match prefix AND number via formatItemRef, so a typo'd prefix is an honest no-op rather than a cross-collection jump, and TASK-007 deliberately matches nothing rather than guessing TASK-7. The server-search fallback queries the bare number for both forms — the query shape the item_number path has always relied on. Live-verified against a sandboxed build (playwright, 4 legs): TASK-9 and task-9 navigate to /tasks/TASK-9, bare 9 still navigates (regression leg), TASH-9 stays put (control leg — the instrument detects non-navigation, which is exactly what the pre-fix build does on a full ref). Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM * fix(web): ref miss probes without clobbering results; stale-query fence (codex r1) Three findings from review: (1) a ref-form miss overwrote the palette's results/total/facets with the bare-number probe's result set — query and display diverged, and loadMore() would page the typed ref against numeric results; the ref probe now reads into a local and leaves displayed state alone. (2) the async fallback had no guard against the user typing past the pending probe (pre-existing on the numeric path, newly exposed for refs) — fenced on the typed-at-Enter query. (3) the numeric miss fallback now searches exactly what was typed again (leading-zero queries had silently switched to the canonical number). Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM |
||
|
xarmian
|
4a2c4c1a39 |
fix(cli): suppress pad agent install's dangling (Y/n) prompt in non-interactive contexts (BUG-2593) (#1116)
installInteractive gated its prompt on cli.IsTerminal() (stdin only), so a pty-backed harness whose stdin looks like a char device — with nobody able to answer — got "Install /pad skill for all N? (Y/n): " printed and then hung at readChoice. Same shape and same fix as offerSkillInstall's BUG-2577 (PR #1111): swap to canPromptForConfig() (stdin AND stdout), document the both-pty undetectable boundary, keep the auto-install behavior unchanged. Test mirrors #1111's offerSkillInstall test and pins the closed-stdin no-prompt path; the discriminating pty-stdin case is live-verified on the trail (pre-fix binary prints the prompt and hangs to a 10s kill, fixed binary installs silently and exits 0 — identical undriven-pty harness). Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM |
||
|
xarmian
|
2580c2c8bb |
fix(cli): gate pad init's Step-4 login on canPromptForConfig() (BUG-2592) (#1115)
* fix(cli): gate pad init's Step-4 login on canPromptForConfig() (BUG-2592) A configured-but-unauthenticated non-interactive `pad init` fell into doBrowserLogin and blocked on the poll wait (wall-clock-bounded since BUG-2572, still minutes of hang nobody can complete) instead of failing fast — Step 3 has had this exact gate since init.go:205, and cmd_workspace.go got it in PR #1111 (BUG-2538). The gate sits AFTER the saved-credentials check so a headless run with valid stored credentials proceeds untouched. Remedy text per the corrected trail ruling (the r1 constraint was refuted by r2): piped `pad auth login --interactive` IS a working non-interactive login (doInteractiveLogin reads a plain bufio.Reader, piped-bytes-safe since BUG-1886), so the message points there — and deliberately not at pad init's --email/--name/--password, which only fire when SetupRequired. Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM * docs(plugin): pad init no longer hangs in the session-expired case — update the three claims + plugin 0.2.1 The BUG-2592 gate makes three plugin-skill passages stale (same shape as PR #1111's codex r3 self-invalidation): capture and onboard said `pad init` can still hang on the browser flow when configured-but- unauthenticated, and the pad skill's whoami-guidance said the same at its "not a safer probe" sentence. All three now state the fixed truth, live-verified this session: fixed binary fails fast in 0.1s with the piped-login remedy; pre-fix control binary hangs to the timeout kill in the identical sandbox state; the remedy itself (piped `pad auth login --interactive`) logs in and restores credentials. Plugin 0.2.1 — text reaches nobody without a bump (version-pinned at install). Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM |
||
|
xarmian
|
1882206bce |
docs(plugin): push-targeting etiquette + assignment-is-watch-only wording; plugin 0.2.0 (TASK-2591) (#1114)
* docs(plugin): push-targeting etiquette + assignment-is-watch-only wording; plugin 0.2.0 (TASK-2591) PLAN-2558 S6, the plugin-visible half that TASK-2551 deferred and S5 (PR #1108) made necessary: - monitors.json + SKILL.md no longer call assignment an addressed-to-you event (Phase 2 removed it from the addressed stream; assignment now arrives only via explicit watches) — the exact stale lines TASK-2564 recorded from PR #1092's codex round. - SKILL.md push etiquette covers S5 targeting: a push may be broadcast or targeted at one session (web composer picker / target_session_id; CLI always broadcasts), the notification line is identical either way, delivered_sessions is a pre-publish presence prediction (never a receipt, ~30s staleness on ungraceful drops), and pushes are never auto-retried — with the targeted-miss exception (delivered_sessions=0 on a targeted push means the publish was skipped, so a resend is safe by construction). - plugin.json 0.1.0 -> 0.2.0: the plugin is version-pinned at install (day-33, HANDO-120 delta (e)), so no text lands without the bump. - handlers_watch_events.go: the KNOWN-STALE pointer comment now records the fix instead of promising it. No behavior change. Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM * docs(plugin): delivered_sessions is API-response-only — CLI reports acceptance only (codex r1 P2) The sender-side bullet claimed the count was visible via pad push --format json; cli.PushResult omits DeliveredSessions, so CLI JSON cannot show it. State the truth instead: the API response carries it, the CLI surfaces nothing about delivery. Whether the CLI should surface it is a separate item, not a midnight scope expansion. Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM * docs(plugin): watches deliver item events, not pushes (codex r2 P3) "cover every event on the watched item" implied a watcher sees pushes on that item; a push is addressed dispatch (the KindPush branch returns before the watch map) and reaches only its addressee. Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM |
||
|
xarmian
|
ef903f0b22 |
feat(cli,mcp): --clear-parent / clear_parent to detach an item's parent (BUG-2078) (#1113)
* feat(cli,mcp): add --clear-parent / clear_parent to detach an item's parent (BUG-2078)
The server has honoured a present-but-empty "parent" key in fields_patch
as "clear the link" since BUG-2013, but neither the CLI (--parent ""
silently no-ops) nor MCP (parent is a plain string with the usual
"empty means not provided" convention) could reach it. Mirrors the
clear_assigned_user/clear_agent_role shape from IDEA-2584: a boolean
that carries its destructive meaning in its name and survives the trip
to local stdio MCP via BuildCLIArgs' snake_case-to-flag mapping.
Bumps ToolSurfaceVersion 0.18 -> 0.19 and updates the drift-pinned docs
(instructions.md, README.md) accordingly.
* test(cli,mcp): cover --clear-parent / clear_parent on both transports (BUG-2078)
CLI: --clear-parent sends fields_patch{"parent":""}; is absent when not
passed; conflicts with --parent and refuses without issuing a PATCH;
item create pins the deliberate create/update asymmetry.
MCP: clear_parent detaches through the real store+server (not a
recording handler) so the assertion is "item ends up unparented", not
just "payload shaped correctly"; clear_parent=false is inert; a plain
empty `parent` string stays a no-op (control leg); a simultaneous
parent + clear_parent is refused via both the direct param and the
--field-lifted route.
* fix(cli,mcp): close --clear-parent bypass via --field parent/plan aliases (BUG-2078, codex r1 P1)
extractParentLink (internal/server/handlers_items.go) resolves the parent
link from either a "parent" or a "plan" key in fields_patch, with no
early exit, so the later key in its own loop wins. The clear_parent
conflict check only covered one path each on the two client surfaces:
- CLI: the check ran BEFORE the --field overlay and only compared
against --parent's own value, so `--clear-parent --field parent=X`
(or `--field plan=X`) reached the wire unrejected — the --field loop
ran after clearParent's own `patch["parent"] = ""` and silently
overwrote it.
- MCP HTTP dispatcher: the check ran after the --field overlay (correct
ordering) but only inspected `patch["parent"]`, missing the "plan"
alias route.
Both surfaces now run the clear_parent check after every patch-building
step (named flags, --field overlay, column lift) and check both
"parent" and "plan" for a competing non-empty value.
* fix(cli,mcp): refuse --clear-parent/clear_parent when schema shadows "parent"/"plan" (BUG-2078, codex r2 #2)
extractParentLink (internal/server/handlers_items.go ~L606-610) is a
pre-existing, deliberate policy: it skips hierarchy handling entirely
when a collection's schema declares its own field literally named
"parent" or "plan", letting the value fall through as an ordinary
field write instead. Once {"parent":""} reaches the server it can no
longer distinguish clear-hierarchy intent from a legitimate
blank-my-schema-field write, so a client-side clear_parent request
against a shadowed collection used to report success while silently
blanking the data field AND leaving the real hierarchy link untouched
-- reproduced empirically before this guard existed.
The ambiguity is created at the surface that accepted the clear
request, so that surface refuses rather than pushing the decision
server-side (server-side refusal would also break legitimate blanking
of a real schema field).
CLI: the check is free -- collSchema is already fetched for --field
type parsing whenever any field change (including a bare
--clear-parent) happens.
MCP HTTP dispatcher: adds one conditional collection lookup, paid only
when clear_parent=true -- the common update path fetches no schema
today and doesn't start.
* docs: sync repo CLAUDE.md tool-surface contract to v0.19 (BUG-2078, codex r3 P2)
CLAUDE.md's MCP tool-surface prose still said "currently v0.18" and its
changelog omitted clear_parent -- a consumed-artifact gap, same rule as
the SKILL.md case: the doc a diff invalidates ships with the diff.
Synced three spots (intro paragraph, Tools bullet, ToolSurfaceVersion
stability-contract changelog) to v0.19, matching internal/mcp/version.go's
in-code entry's wording, plus the schema-shadow refusal (BUG-2078's
second follow-up commit) at the same level of detail the changelog
already gives the parent/plan alias conflict-refusal.
Grepped the rest of CLAUDE.md for any other 0.18/tool-surface reference
-- none found outside these three lines.
* docs: add schema-shadow refusal to version.go's v0.19 changelog entry (BUG-2078, codex r3 follow-up)
The in-code changelog is the canonical source; it was missing the
codex r2 schema-shadow refusal that a later commit added, which is
why CLAUDE.md and version.go briefly disagreed. Completes version.go
instead of letting CLAUDE.md drift ahead of it.
|
||
|
xarmian
|
d895418ea2 |
fix(server): gate RequireAuth's cloud-secret bypass on validated session (BUG-1944) (#1112)
Sibling of TASK-1932's CSRFProtect fix: RequireAuth's isCloudAdminPath + hasCloudSecretMarker bypass fired on marker presence, not validated secret. Mirror TASK-1932's currentUser(r) == nil gate exactly. Concretely closes a disabled-admin gap: without the gate, a marker with the wrong secret let RequireAuth's own user.IsDisabled() check be skipped whenever a session was present, reaching handlers that trust a resolved admin session as an alternative to validateCloudSecret. In-handler validation for every cloudAdminPaths handler is unchanged and remains the independent layer for the genuine no-session sidecar case. |
||
|
xarmian
|
ac05d8a2b1 |
fix(cli): fail fast and quiet on non-interactive workspace init (BUG-2538, BUG-2577) (#1111)
* Fail fast and quiet on non-interactive `pad workspace init` BUG-2538: initCmd drove runBrowserSetup/doBrowserLogin unconditionally when the instance needed first-run setup or login, blocking a non-interactive caller (script, CI, headless agent) on a browser handoff nobody can complete. Gate both branches on canPromptForConfig(), mirroring the precedent already used by `pad init` (init.go:205-206), and fail fast with a hint pointing at `pad init --email/--name/--password` or `pad auth setup`/`pad auth login`. BUG-2577: offerSkillInstall (shared by workspace init and workspace link) printed a "(Y/n): " prompt even when the answer would be auto-defaulted rather than read, because it gated on cli.IsTerminal() (stdin only). Switch to canPromptForConfig() (stdin AND stdout), which is the same predicate now used for BUG-2538 and the more robust of the two checks already in the codebase. Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt * Fix wrong remedy in BUG-2538's !Authenticated error message codex r1: the !Authenticated branch suggested `pad init --email/--name/--password`, but those headless flags only bootstrap the first admin account and only fire when SetupRequired — for an already-set-up-but-unauthenticated instance, `pad init` falls through to its own ungated Step 4 re-auth (BUG-2592), so the suggestion relocated the hang instead of avoiding it. Drop the pad-init suggestion in this branch only; point at `pad auth login` and note there's no non-interactive login path yet. SetupRequired branch is unchanged — its pad-init suggestion is correct for that state. Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt * Fix two more inaccurate remedies flagged by codex r2 1. SetupRequired branch: `pad init --email/--name/--password` silently eats the caller's workspace name/--template — pad init creates its own CWD-named workspace as a side effect, so a re-run of the original `pad workspace init <name> --template <t>` short-circuits on the link pad init just made with no signal <name>/<t> were ignored. Switch the remedy to `pad auth setup --email/--name/--password`, which bootstraps the admin account only (no workspace side effects), then re-run the original command. 2. !Authenticated branch: the "no non-interactive login path exists" claim was false — `pad auth login --interactive` reads email/password off a plain, TTY-ungated bufio.Reader (doInteractiveLogin, cmd_auth.go:554+; BUG-1886 made it piped-bytes-safe), so it works fine when credentials are piped in. Reworded to point at it and dropped the incorrect BUG-2592 reference (that bug tracks pad init's ungated Step 4, not a missing login mechanism). TestWorkspaceInitNonTTYSetupRequired's assertion updated from "pad init" to "pad auth setup" to match; TestWorkspaceInitNonTTYNotAuthenticated needed no change (still asserts "pad auth login"). Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt * Update skill docs invalidated by the non-interactive fast-fail fix codex r3: BUG-2538/BUG-2577 made this diff's own docs stale. Four files (skills/pad/SKILL.md, plugin/skills/pad/SKILL.md, plugin/skills/onboard/SKILL.md, plugin/skills/capture/SKILL.md) still say non-interactive `pad workspace init` on a configured-but- unauthenticated machine "blocks for minutes with no non-interactive fallback" — that was true pre-fix (per BUG-2541's verification) and is false now. Reworded the WHY without dropping the underlying do-not-run-blind guidance: an agent's tool call is always non-interactive, so it now gets a fast, actionable error instead of a hang, but the error still just says a human needs an interactive terminal — `pad auth whoami` remains the right check to run instead. Where the docs' `pad init` claims are about the still-unfixed session-expired path (BUG-2592, this diff's Step-4 sibling, left untouched), those claims are unchanged and now cite BUG-2592 explicitly. skills/INSTALL.md:24 updated separately (P3): notes the non-interactive silent-install branch of `pad workspace init`'s skill offer, alongside the existing interactive-prompt description. Docs only, no Go changes — go build/test and embed.go's //go:embed skills/pad/SKILL.md still resolve; no test asserts the old wording. Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt |
||
|
xarmian
|
30274057eb |
fix(release): keep RC tags off brew and docker :latest (BUG-2524) (#1110)
A prerelease tag (vX.Y.Z-rc.N) published the RC to both mainstream install channels: homebrew_casks had no skip_upload (goreleaser's default uploads the cask for prereleases too) and dockers_v2 listed "latest" unconditionally, so every tag moved the floating tag. Both were verified serving 0.12.0-rc.1 during the v0.12.0 cut. skip_upload: auto skips cask upload for prereleases; the conditional latest template evaluates empty on RCs, and goreleaser ignores empty tags (documented behavior, and the docs' own conditional-tag example uses this exact shape). Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt |
||
|
xarmian
|
7d5d3bd672 |
fix(cli): give the CLI auth poll loop its own wall-clock timeout (BUG-2572) (#1109)
* fix(cli): bound pollAndSaveCLIAuth with its own wall-clock timeout (BUG-2572) pollAndSaveCLIAuth had no wall-clock limit of its own — the ~5m bound users rely on was purely the server-side session TTL, so an unreachable server after session creation left the poll loop spinning forever on Ctrl-C alone. Add a 20m timer (matching the longer of the two server TTLs, since this helper is shared by both the plain login and first-run setup flows) plus a consecutive-transient-error bound so a permanently unreachable server fails fast with a network-shaped error instead of waiting out the full timeout. Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt * fix(cli): make poll-error headline accurate for HTTP-error servers (BUG-2572 r2) The consecutive-error bail-out message claimed "could not reach server", but client.get returns an error for both transport failures and non-2xx HTTP responses, so a server that's reachable but persistently returning 500 got misreported as unreachable. Bailing out fast is still correct for that case; only the headline was wrong. Switch to a cause-neutral message and let the wrapped error carry the specifics (codex round 2). Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt |
||
|
xarmian
|
00a91dfcf4 |
feat(push): session targeting — target_session_id + delivered_sessions (TASK-2588) (#1108)
* watchevents: add session-targeted push delivery predicate
PLAN-2558 S5 (TASK-2588). Notification gains TargetSessionID,
evaluated in the existing per-connection KindPush predicate in
watchNotificationVisible alongside TargetUserID — one delivery path,
targeted is broadcast-with-a-predicate, no bus changes. Empty
TargetSessionID (the pre-S5 shape) still matches every one of the
target user's sessions.
* server: accept target_session_id on push, report delivered_sessions
PLAN-2558 S5 (TASK-2588). POST .../items/{slug}/push accepts an
optional target_session_id (an id from GET /api/v1/sessions) and the
response gains delivered_sessions — a prediction read from the S1
presence registry at push time, scoped to the caller's own
ListForUser(userID) so a vanished id and one belonging to a different
user are both an honest 200/0 with no existence oracle across users.
Omitting the field keeps the exact pre-S5 request/response shape.
* web: session picker in the push composer, targeted-miss handling
PLAN-2558 S5 (TASK-2588). PushToAgentDialog gains a target picker
(broadcast default + one option per live session), reusing the
presence read already fetched for the count — no second GET
/api/v1/sessions. Selecting a session passes target_session_id;
leaving it untouched keeps the exact pre-S5 3-argument push() call.
A targeted miss (delivered_sessions === 0) toasts "that session is
gone — refresh the list", drops the selection back to broadcast, and
re-polls presence instead of closing — zero delivery means nothing
was sent, so nothing is duplicated by resending.
* server: bound target_session_id, skip publish on a targeted miss
Codex round 1 fixes for TASK-2588:
- Cap target_session_id at 256 runes (400 over-cap) so an authenticated
caller can't park arbitrary garbage in the bus's shared replay buffer;
a registry-issued id (36 runes) can never hit this bound.
- Snapshot presence BEFORE publish instead of counting after: the old
order raced a target disconnecting between publish and count, which
could report delivered_sessions=0 on a push that had already landed
once. A targeted push now skips the publish entirely when its id
isn't in the pre-publish snapshot — session ids are per-connection
and never reused, so a target absent now can never be matched later,
making the 0 a guarantee rather than a race. Broadcast is unaffected
(still publish-always, pre-publish count).
Strengthened the targeted-miss and cross-user tests to assert the bus
does not grow (not just that the notification fails to arrive
downstream) — verified this fails if the skip-on-miss guard is
reverted.
* push targeting: document the pushed ruling, fix stale picker selection, guard mixed-version responses
Codex round 2 dispositions for TASK-2588:
- pushed:true on a skipped publish is RULED, not a bug (dispatcher):
moved the ruling from a test comment onto the contract itself —
pushResponse.Pushed's own doc comment in Go, mirrored in the TS
ItemPushResult doc comment.
- Fixed a real sharp edge: when a presence refresh drops the selected
session, a <select> can visually fall back to "All connected
sessions" while the bound value stays the stale id, so the wire
would carry a dead target the UI no longer shows as selected.
Added reconcileSelectedSession(), called at every point `sessions`
is reassigned outside the fresh-open reset (a live poll, a failed
read, and the staleness-expiry path).
- Guarded the mixed-version hazard with a cheap check, not capability
negotiation (the deployment shape — web assets embedded in the
server binary — bounds this to a transient stale tab, argument
recorded in the comment): delivered_sessions is now optional on the
wire type, and a targeted send whose response omits it entirely is
treated as UNKNOWN (info toast, dismiss like a normal success) —
never inferred as a confirmed miss.
Verified all three new/changed legs actually catch their regression
by temporarily reverting each fix and confirming the corresponding
test fails, then restoring.
* push targeting: fix stale publish-guarantee comments (codex round 3)
Two doc-comment remnants of round 2's skip-on-miss fix, both claiming
push unconditionally publishes:
- watchevents.KindPush's doc comment ("publishes exactly one of
these") now notes handlePushToItem decides whether to publish at
all, and points at TargetSessionID / pushResponse.DeliveredSessions
for why.
- api.items.push()'s JSDoc in client.ts no longer claims a resolved
promise means "published to the bus" unconditionally — a targeted
miss resolves with delivered_sessions: 0 and nothing published.
Comment-only; no behavior change.
|
||
|
xarmian
|
d7da237198 |
feat(mcp,cli): clear_assigned_user / clear_agent_role — a discoverable unassign (IDEA-2584) (#1107)
* feat(mcp,cli): clear_assigned_user / clear_agent_role — a discoverable unassign (IDEA-2584)
v0.16 and v0.17 made unassigning WORK. Nothing advertised it. The params
that do it — `assigned_user_id` / `agent_role_id` — were never in the
catalog, so an agent reading the tool schema to find out how saw only
`assign` (a name) and reached for `assign: ""`, which is a no-op and
deliberately stays one. The capability existed with no name an agent
could find.
`clear_assigned_user` / `clear_agent_role` booleans on `pad_item`, backed
by new `--clear-assigned-user` / `--clear-agent-role` bareword flags on
`pad item update`.
WHY BOOLEANS rather than declaring the existing string params. Two
reasons, and the second decided it:
1. An empty DECLARED string is inert everywhere else on this tool
(title, content, comment, tags), so a client that pads optional
params with "" instead of omitting them is harmless today. Giving
one a destructive meaning would turn that same client into one that
silently unassigns every item it touches. A boolean carries its
meaning in its name and can't be tripped that way.
2. Only a boolean can REACH local stdio. BuildCLIArgs emits the CLI's
real flags, so a catalog param with no flag behind it is dropped
before dispatch — declaring `assigned_user_id` would have left the
direct form remote-only, i.e. would not have closed the gap this
change exists to close. That fact reframed the design fork and is
what the ruling turned on.
Server-side this is WIRING, not new semantics:
models.ItemUpdate.ClearAssignedUser / ClearAgentRole already existed and
the store has honoured them since BUG-2566, on the same branch as the
empty-string form. The older forms keep working and are NOT deprecated;
they're just not what the schema advertises.
UPDATE ONLY, deliberately asymmetric with create, and recorded in-place
at both the flag registration and the catalog description so a
symmetry-minded reader meets the reasoning before the "fix": clearing at
create is a request to not-set something never set, whose only honest
behaviour is a no-op — it teaches a wrong affordance and pads every
create call's schema. A test fails if someone adds them there.
CLI precedence is the OPPOSITE of the --field lift's, deliberately: an
explicit `--clear-assigned-user` beats `--assign`, because that
combination is a contradiction the user typed and the reading that
cannot silently assign somebody is the safer one. Tested.
The dispatcher forwards the booleans VERBATIM rather than only-when-true.
A `&& b` guard would read as the thing protecting a param-padding client
and would be lying: what makes `false` inert is the store. Same call I
made on #1106's `len(patch) > 0` — a guard that reads as load-bearing
while doing nothing is worse than none.
ToolSurfaceVersion 0.17 -> 0.18, ADDITIVE bump per the v0.5 / v0.6
precedent: no existing tool, action or param changed shape.
Consumed artifacts moved in the same commit, which is the whole point of
this change — the schema IS the deliverable: catalog_item.go (the schema
agents read, plus an `assign` description that now says where to find the
clear), instructions.md (leads with the boolean, mentions the older forms
as still-working), version.go, README, CLAUDE.md.
VERIFIED LIVE, five legs, both transports:
CLI --clear-assigned-user -> assigned=None, role intact
CLI --clear-agent-role -> role=None
stdio clear_assigned_user:false -> assignment SURVIVES and the
update still applied (title
changed) — the control that
makes the boolean safe to
declare at all
stdio clear_assigned_user:true -> assigned=None
stdio clear_agent_role:true -> role=None
Three mutations, each failing only its own tests: dropping the dispatcher
forwarding; hardcoding true in the dispatcher (fails the false-control);
dropping the CLI flag wiring.
go test ./cmd/pad ./internal/mcp — pass. gofmt clean.
Closes IDEA-2584.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
* fix(mcp,cli): refuse a simultaneous set-and-clear (codex round 1)
Codex found a real bug, and the more useful half of the finding is that
MY OWN TEST FOR IT WAS VACUOUS.
The store's branch order is `if AssignedUserID != "" { set } else if
ClearAssignedUser { clear }`. So `--assign wren --clear-assigned-user`
assigned Wren and the clear evaporated. My in-place comment claimed the
opposite ("an explicit clear wins"), and the test I wrote to prove it
asserted `body["clear_assigned_user"] == true` — that the FLAG was set,
not that the item ended up unassigned. The flag was set. The behaviour
was backwards. A test that asserts a field is present says nothing about
which field wins.
Both surfaces now REFUSE the contradiction rather than silently resolving
it. Rejecting beats picking a winner here: the store already picks one
silently, which is the bug; and a caller who typed both wants to be told,
not guessed at. Precedent in the same command family — `item list`
already makes `--parent` and `--unparented` mutually exclusive.
PLACEMENT IS THE LOAD-BEARING PART, and I got it wrong first. There are
two routes to a competing value: `--assign` / `assigned_user_id`, which
resolve early, and `field: ["assigned_user_id=<uuid>"]`, which reaches
the payload via liftFieldsToColumns LATER. My first version checked
between them and its comment asserted the lift "has already" run — it
hadn't. That version rejects the direct case and lets the lifted case
through: a half-fix that reads as complete. The check now runs after
both, in the CLI after --assign/--role resolution and the lift, in the
dispatcher immediately before the body marshal.
That mutation is now a test: moving the dispatcher check back to the
pre-lift view fails ONLY the two `lifted …` subtests and passes the
direct one — the exact shape of the bug I nearly shipped.
Tests assert the OUTCOME, not the message: a refused conflict must leave
the item's assignment AND role untouched, and the CLI must issue no PATCH
at all. An error string alone wouldn't prove the write didn't happen.
Agent-facing text moved with it (the consumed-artifact step): both
catalog descriptions, instructions.md, and the v0.18 version entry now
say the combination is refused. An agent that pairs them gets a
structured refusal, so the schema has to say so.
go test ./cmd/pad ./internal/mcp — pass. gofmt clean.
Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd
|