403 Commits

Author	SHA1	Message	Date
xarmian	402f79e016	feat(store,server,web): collection kernel traits — de-hardcode conventions/playbooks slugs (TASK-2657, BUG-2702) (#1171) ... Implements SPEC-5 §Collection traits (approved v1.1) — the first unit of PLAN-2656 phase 0. Three kernel behaviors were keyed on the literal collection slugs "conventions" and "playbooks": what the agent bootstrap loads, which items route by invocation slug, and which items export as portable artifacts. Collections now DECLARE those behaviors and the kernel resolves them from the declarations. Fixes the KERNEL half of BUG-2702, which stays open for the rest (see below). A slug is not a stable identifier — UpdateCollection re-slugs on any name change, and renaming a collection is a documented onboarding step (TASK-1510) — so renaming either collection silently detached all three behaviors from it, with the items still present and no error anywhere. Measured on origin/main before the fix: conventions and convention_index dropped 1 -> 0, playbooks 1 -> 0, and GET /playbooks/{slug} went 200 -> 404, so `/pad ship` stopped resolving with no sign the playbook still existed. Both halves are locked by regression tests observed failing on unfixed code. BUG-2702 is NOT fully closed here, deliberately. Every kernel behavior follows the trait, and library activation on the MCP dispatcher and CLI was converted too — but the pack's own dedicated web routes (/conventions, /playbooks list and detail, /library) still address their collection by literal slug and render empty after a rename. Filed as BUG-2705 with the route paths and the likely fix shape; 2702 closes when that lands. Degradation there is bounded: no data loss, and the collection stays usable at its own /[collection] route and in the sidebar. SPEC-5 was amended to v1.1 BEFORE any code, per the spec tree's own discipline: bootstrap_include becomes a LIST of {mode, filter, key} because v1.0 could not express convention_index at all; the conventions filter is now normative and includes status=active, which v1.0's shorthand omitted and which the implementation does enforce (implementing v1.0 literally would have leaked draft conventions into every agent's boot payload); v1 filters are field- equality maps with query/1 named as the widening path, since SPEC-2 is phase 1 and PLAN-2656 forbids growing toward it; and invocation_field is constrained to the literal `invocation_slug`, because any other field name falls outside the partial unique indexes in migrations/054 and pgmigrations/033 that are the real uniqueness guard. Traits get their own column rather than a key inside the schema JSON. The schema column is overwritten wholesale on update and every client rebuilds it fields-only, so a traits key stored there is destroyed by one ordinary collection edit — measured during this task, not assumed. Trait authority cannot rest on a value an unrelated UI save deletes. UpdateCollection writes traits only when explicitly supplied, so pre-existing clients leave them alone; an explicit "{}" still clears. Bootstrap keeps its three payload keys as first-party views fed from the declarations, and gains a generic bootstrap_includes array for any other declared key — so the boot surface is genuinely generic rather than three hardcoded payloads, and no consumer breaks. Existing workspaces are backfilled slug-keyed in both dialects, guarded on traits='{}' so a re-run cannot clobber a workspace's own declarations. The backfill inherits today's blind spot (a workspace that renamed the collection before upgrading is not reached) but cannot do worse than the status quo, which is itself slug-keyed; from the backfill forward the hazard is structurally gone. Malformed declarations are refused at create and update rather than stored: an unparseable blob degrades to "declares nothing", which is silently the wrong behavior instead of a loud error (SPEC-0 L6). Web groups agent-facing collections by bootstrap_include presence, replacing a hardcoded two-slug array repeated at five call sites. Not done, deliberately: no MCP catalog change (traits are first-party kernel declarations, no agent needs to set them, and the separate column means pad_collection.update passes through harmlessly — no ToolSurfaceVersion bump); bootstrap's collections[] projection does not carry traits (PLAN-1410 trimmed that payload and nothing consumes them there); prefix.go's NormalizeSlug is untouched (a pure function with no workspace context, and de-hardcoding two of its six slugs would make it less coherent, not more). Eight Codex review rounds found nineteen real defects, all fixed here. The serious one: bootstrap_include filter keys FAIL OPEN. The item store's field-filter path drops any key its sanitizer rejects, removing the predicate rather than matching nothing, so a declaration filtering on `"stat us"` would narrow nothing and ship every convention — drafts included — to every agent at boot, defeating the status=active guarantee this change makes normative. Filter keys are now validated against the store's own sanitizer shape and pinned by a cross-package agreement test, since models cannot import store and a future divergence would silently reopen it. SPEC-5 amended to v1.2 with the rule and its fail-open / fail-closed asymmetry. Also fixed: an unknown declared artifact_kind reached artifact.Encode and surfaced as a 500 (now a 400 at the export boundary, since SPEC-5 permits unknown kinds as legal non-round-tripping declarations); and workspace import validated traits as JSON only, so an archive could persist a declaration that degrades to "declares nothing" (now validated, degrading to "{}" with a warning rather than refusing an import that may be the only copy). Later rounds found more, and several were defects this change itself created. A hidden collection could SHADOW a visible one: resolution used to name exactly one collection, so with several declaring, resolving across all of them and rejecting afterwards on visibility made a visible playbook unreachable behind a hidden one — candidates are now filtered by visibility before selection, in both playbook resolution and artifact import. Importing a pre-traits archive produced an INERT workspace: the migration backfill cannot reach rows inserted long after it ran, so conventions/playbooks arrived declaring nothing, and canonical declarations are now inferred from the slug when a collection declares none (never overriding declarations that survived the round trip). The generic include path had no L4 boot budget and is now capped with an overflow count. Trait parsing claimed to be strict but json.Decoder ignores trailing bytes, so `{...} garbage` parsed cleanly. First-party payload keys are now mode-pinned, since their projections have fixed shapes and declaring the other mode would be silently ignored. Duplicate artifact_kind / invocation_field declarations are refused at the collection API, and a conflicting archive warns on import. Agent-facing text was updated with the rest, not after it: SKILL.md, instructions.md and the MCP catalog said the literal slugs, which is exactly the artifact an agent acts on. ToolSurfaceVersion 0.24 -> 0.25 for the pad_library.activate behaviour change. Trait uniqueness is a documented BEST-EFFORT gate, not an invariant, by lead ruling. The gate reads then writes without a lock, import bypasses it, and a rename can mint a duplicate without touching that path. The database-level enforcement (partial unique indexes on the extracted traits) cannot ship first: existing deployments can already hold duplicates via rename-then-reseed, so the index would fail the migration on precisely the databases that most need repairing. TASK-2710 carries the de-duplication pass and the indexes; SPEC-5 v1.3 records the deferral and the reason. L6's requirement that conflicts fail loud is met by the refusal plus the warning — the mechanism is deferred, the principle is not. Gates: build · make lint 0 issues · go test ./internal/... · make test-pg · svelte-check 0 errors · vitest 99 files / 1734 tests. Mutation-verified across four matrices, 20 mutations, 19 caught; the survivor is a seeding path whose trait-vs-slug difference is unreachable today (SeedCollectionsFromTemplate creates any missing template collection before it seeds items), recorded on the task trail rather than papered over with a test that proves nothing. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V	2026-08-20 13:26:05 -04:00
xarmian	25c7cd20f5	feat(watchevents): Redis-backed bus so watch notifications cross instances (BUG-2651) (#1167) ... * feat(watchevents): Redis-backed bus so watch notifications cross instances (BUG-2651) internal/watchevents shipped MemoryBus only, so in a multi-instance deployment a notification published on instance A never reached a stream held open on instance B — watches appeared to work and silently dropped. Bus was an interface from day one for exactly this; adding RedisBus changed no producer and no consumer. NOT A MECHANICAL PORT of internal/events.RedisBus. Three deliberate divergences, each documented at the point someone diffing the two files would call it a mistake: - ONE channel and ONE replay buffer, because this package has exactly one logical stream by contract (DOC-2479 DR-2: all per-caller filtering happens in the consumer). Most of the template's bookkeeping — per- workspace counts, subscriptions, buffers — has nothing to key on here. - EAGER subscription for the bus's lifetime, not lazily on first local subscriber. The replay buffer fills from the RECEIVE path, so a lazily torn-down subscription stops filling it at precisely the moment before a Last-Event-ID resume — for one harness monitor holding one stream, that makes resume structurally useless. The template can afford lazy because per-workspace means N idle subscriptions; here it is one. - ONE mutex across subscriber membership and the replay buffer, held through the whole local fan-out. The template uses two and offers only separate Subscribe + EventsSince, which cannot provide SubscribeAndReplaySince's guarantee. Copying its locking would have handed back the double-delivery window this package's interface exists to close. Publish fails CLOSED when INCR fails, where the template falls back to a local counter. Two instances falling back at once mint ids from independent counters into a shared stream, and replayBuffer.since() reasons on monotonicity — so the damage is silent replay corruption, not a visible error. INCR and PUBLISH share a connection anyway, so the fallback mostly lets a doomed publish proceed carrying a poisoned id. Both load-bearing tests were VACUOUS as first written; the mutation matrix is the only reason I know: - the concurrency test's producer finished before the subscriber joined, so the channel leg was never exercised and a split-lock mutant survived 50 iterations. Now paced, with a both-legs-non-empty precondition that fails a run which never approached the boundary, plus a dedicated detector (600 attempts, 8/8 kills, 0.02s after switching the drain to non-blocking — exact, because the duplicate is already buffered when the call returns). - the fail-closed test asserted nothing was delivered, which is true of the fallback too: Publish never delivers locally, so with Redis down neither policy delivers. Rewritten around a go-redis ProcessHook that records attempted commands, which is where the policies actually differ (INCR-then-stop vs INCR-then-PUBLISH). Also corrects session_presence.go, which told the next person these two had to be fixed together. Delivery is now cross-instance; the registry's under-report is unchanged, so the remaining defect is a picker that under-reports rather than a push that lies. The PLAN-2558 S3 gate stays, for that reason instead of the old one. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents): make id assignment and publish atomic; close the bus on shutdown (Codex round 1) P1 — INCR and PUBLISH as two client calls are not order-preserving, and the failure is concrete: A gets id 1 and is descheduled, B gets id 2 and publishes, A publishes 1. Every subscriber receives 2 before 1, the replay buffer appends in ARRIVAL order, and replayBuffer.since() reasons on monotonicity — so a resume from 2 hits the sinceID > newestID branch and answers 'gap too large', turning a healthy reconnect into a spurious sync_required, while a resume from 1 silently skips the late arrival. Fixed at the source with a Lua script: Redis runs it atomically on its single thread, so INCR and PUBLISH for one instance both complete before another's script begins, and publish order equals id order globally with no coordination on our side. The id rides as a '<id>|<json>' prefix rather than being edited into the JSON from Lua; the id is digits and the FIRST '|' separates, so a '|' in the body is unambiguous. A pleasant consequence: there is no longer a window where an id exists but the publish has not happened, so the fail-closed decision and the publish decision became the same decision. P2 — Stop() never closed the watch bus. That was survivable for MemoryBus, whose Close only drops channels; RedisBus holds a receive goroutine and a Redis subscription from construction, so it leaked both for the process's life. Closed after bg.Wait(), so a background producer cannot publish into a bus already tearing down. nits, all real, all in artifacts someone reads: - 'exactly-once delivery' was simply wrong. Redis pub/sub is at-most-once and the local send is deliberately non-blocking. The property the round trip actually buys is NO DOUBLE DELIVERY to the publishing instance; the comment now says that and names the replay buffer as the bounded recovery mechanism for the rest. - the Bus interface comment still said only MemoryBus existed. - cmd_server.go's session-presence note still claimed the same caveat as 'the watch bus directly above', which had just stopped applying. - session_presence.go now says delivery is fixed WHEN PAD_REDIS_URL is set, rather than unconditionally. Tests: the fail-closed assertion moved from 'nothing was delivered' — still true under the two-call version — to 'no bare INCR or PUBLISH was issued', which is what distinguishes atomic from not. Mutation-verified by splitting the script back into two calls. Added a decode round-trip test covering the new wire format, a '|' inside the body, and four malformed payloads, since that decoder consumes bytes from a channel any holder of the Redis credentials can publish to. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents,server): correct the targeted-push claim; close the bus before HTTP shutdown (Codex round 2) P2 — I claimed cross-instance DELIVERY was fixed. Half true, and the false half was mine to catch: handlers_push.go gates a session-targeted push on the LOCAL presence registry and skips the publish entirely when the id is not there, so a POST landing on A for a session held on B still delivers nothing. The bus would carry it; the gate means it never reaches the bus. Broadcast pushes and every other notification kind ARE fixed. I asserted that behaviour from reading the bus and session_presence.go without reading the push handler — the exact thing I hold myself to not doing. Corrected in all three places the claim was made (the package doc, session_presence.go, and the KindPush comment), with the correction recorded rather than quietly overwritten. The gate's own justification is now stale too, and worth more than a tweak: 'a target this instance cannot see is a guaranteed no-op' was TRUE under MemoryBus and is FALSE under RedisBus, where another instance may hold that session. Left in place deliberately — publishing unconditionally would fix delivery and immediately make delivered_sessions=0 a lie in the other direction, which is a question about what that field promises. It belongs with the shared-state SessionPresence that PLAN-2558 S3 already gates on: fixing the registry makes the snapshot right, and then the skip is correct again for its original reason. Both open halves collapse into that one implementation. P2 — the watch bus was closed only in Server.Stop(), which runs AFTER http.Server.Shutdown. The event bus is closed before Shutdown precisely so its SSE handlers unblock; the watch stream is the same shape, so an open one would have held Shutdown to its full 30s deadline. Now closed alongside eventBus, with the Stop() close kept as the path for other callers — both implementations are idempotent. nit — MemoryBus and RedisBus disagreed after Close: RedisBus handed a late Subscribe an already-closed channel, MemoryBus registered one nobody would ever close, so a consumer racing shutdown blocked forever. MemoryBus now matches, and its Close is idempotent, which the CLI's double close relies on. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents): report a missed notification as a replay gap (Codex round 3) P2 — a divergence MemoryBus structurally cannot have. It assigns every id itself, so its replay buffer is contiguous and the only gap it can report is eviction. RedisBus receives ids over at-most-once pub/sub, so a blipped subscription can miss 101 and receive 102: the buffer holds a hole, is nowhere near full, and replayBuffer.since() answers a resume from 100 with just [102]. The consumer loses a nudge and is never told. RedisBus now tracks the id at which the sequence resumed after the most recent hole, and answers nil — the same signal eviction already gives, which the SSE handler already turns into sync_required — for a resume that would have to span it. Resumes that do not span it still replay normally, and sinceID=0 is treated as a fresh subscriber rather than a resume, so a hole nobody spanned is not turned into a spurious resync. The atomic publish script is what makes this readable: publish order is id order globally, so a non-consecutive id means MISSED, not reordered. Mutation-verified by disabling the check; the test fails on both the spanning resumes and would have failed the over-broad version too (it asserts the non-spanning resumes still work). Two residuals documented rather than fixed, both because the fix is the same shared-state SessionPresence that PLAN-2558 S3 gates on: - delivered_sessions is now wrong in BOTH directions for a broadcast push — the count is local while delivery is global, so a replica can report 1 while two sessions receive it, or 0 while a remote one does. No local arithmetic fixes that; it is asking one replica what all of them are doing. - the Redis channel and counter names are not deployment-scoped, so two installations sharing a Redis endpoint cross-feed (and picking different logical DBs does not help — pub/sub ignores them). Left flat to match internal/events rather than giving one of the two buses a prefix the other lacks; the rule is one Redis endpoint per installation, and relaxing it should cover both buses at once. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents): a cold-started replica must report a gap too (Codex round 4) P1 — the round-3 hole check only fired BETWEEN two received messages, so it never fired for the first one. A replica restarting while Redis is already at 101 has an empty buffer; its first received message is 102, nothing looks like a hole, and a client reconnecting to that replica with Last-Event-ID 100 was handed [102] — skipping 101 exactly as silently as the case round 3 fixed, by a different route. Replaced contiguousFrom with knownFrom: the lowest id from which this instance's buffer is contiguous. SET on the first append (before which this instance knows nothing) and RESET on every hole (before which it no longer knows anything usable). One variable, both failures. The boundary is pinned in both directions, which is what stops this being an over-broad 'always gap after a restart': a resume from exactly the id before our first (101 when we started at 102) IS contiguous with our view and replays normally. Mutation-verified by disabling the cold-start arm. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents): idempotent publish, confirmed subscription, and real Redis tests (Codex round 5) P2 — go-redis retries a command whose reply is lost to a network error, and the publish script was not idempotent: the same notification would be published twice under two different ids. Both copies look valid — ordered, distinct — so nothing downstream could tell them apart, and on the push path a duplicate is a duplicate DISPATCH into an agent harness. The script now takes a caller-generated token and SET NX's it, so a retry carrying the same arguments returns 0 without publishing. TWO THINGS THIS UNIT OWES ITS TESTS, both found within minutes of each other and both invisible to the hermetic ones: 1. The idempotency script shipped indexing ARGV[3] while Publish passed two arguments. Caught by re-reading, which is not a control worth relying on for the next Lua edit. 2. NewRedisBus returned before go-redis had established the subscription, so notifications published in that window were lost to this instance, silently. Surfaced as a test flake; the production shape is a rolling deploy, where a replica takes traffic before its subscription is live. The constructor now waits for the confirmation (bounded, and a failure is logged rather than fatal since Channel() re-subscribes on reconnect). So miniredis is now a test dependency, and the round-trip tests it enables cover what fanOutLocally-driven tests structurally cannot: the channel name, the KEYS/ARGV mapping, the id prefix wire format, the shared counter across two buses, cross-instance delivery (the actual bug), the dedupe token, and Close tearing down the SERVER-side subscription rather than just local channels. Verified by restoring the ARGV[3] bug: the round-trip test fails on it. The two findings I am NOT fixing here are unchanged and documented where the reasoning is met — the targeted-push gate and delivered_sessions are both consequences of the per-process presence registry, and both are closed by the shared-state SessionPresence that PLAN-2558 S3 gates on, not by anything in this package. make vuln: 0 vulnerabilities in imported packages. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents): survive a Redis counter reset without replaying stale ids (Codex round 6) P2 — pad:watchevents_seq has no TTL but can still vanish: evicted under maxmemory, dropped by a FLUSHDB, or restored from an older snapshot. Ids then restart at 1 while this instance's ring still holds the hundreds. Keeping both is what corrupts replay — the two id spaces are not comparable, so a resume from 2 in the NEW space would be handed the stale 99/100/101 as though they were newer. A backwards id now drops the replay buffer and re-anchors knownFrom. Every resume from the old space then exceeds the newest id held and gets nil — the resync signal that is the only honest answer once the ids stopped meaning what the client thinks they mean — while clients in the new space keep working immediately. The test asserts BOTH halves, which is what makes it a detector rather than a description: a build that logged the reset and kept the buffer passes 'the old resume reports a gap' and fails 'the new resume never returns a pre-reset entry'. Mutation-verified on exactly that. Hardened while I was here: the epoch-reset path REBUILDS the buffer at runtime, so a bus constructed with a non-positive replay size would have turned a counter reset into a panic (newReplayBuffer(0)'s first append indexes a zero-length slice) rather than a resync. The constructor now normalizes. MemoryBus has the same trap for a caller passing 0; left alone as pre-existing and off this path, but named in the comment rather than silently fixed or silently ignored. nit — this file's header still claimed there was no miniredis dependency and no round-trip coverage, which the previous commit made false. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(watchevents): actually correct the hermetic test header (Codex round 7) The previous commit's message claimed this fix. It did not contain it: the edit ran as one of two scripts in a single command, its assertion failed with a traceback, and the second script's success is what I read. The header kept saying there was no miniredis dependency and no round-trip coverage — both false since two commits ago, in the file a reader consults to find out what IS covered. That is the adjacent-success-signal failure exactly: a success line from the step next to the one I cared about. The tell was in the output and I walked past it, then asserted the change in a commit message. Recording it here rather than quietly fixing, because a commit that claims a change it does not make is worse than one that omits it. Verified this time by reading the file back and grepping for the stale phrases: zero. Round 7's other three findings are the documented residuals re-raised for the third time — the targeted-push gate, delivered_sessions, and the unnamespaced Redis keys. All three are dispositioned at the line a reader meets them, all three are consequences of the per-process SessionPresence registry or of matching internal/events' existing convention, and none is fixable inside this package. They stay open, on the record, and with the lead. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(watchevents,cli): correct pad push --help; document the reset-window residual (Codex round 8) nit, and the one that stings — cmd_push.go's Long help still said pushes go over the 'in-memory watch-events bus'. That is the text a user reads when they run pad push --help, and it has been false since this branch's first commit. I have a standing pre-push step to grep the artifacts a CONSUMER reads for exactly this, and I ran it as a code search (watchevents.New) rather than a prose search, so --help never came up. The help now distinguishes broadcast (reaches every instance) from session-targeted (still resolved against the handling server) and names the bug. P2 — the counter-reset handling fires when the first post-reset notification ARRIVES, so there is a window between Redis losing the counter and the next publish in which this instance still replays old ids to a reconnecting client. Documented as accepted rather than closed: nothing local can detect the reset earlier (the counter is in Redis and we learn of it by receiving something), and the two shapes that would — a GET per resume, or a background poller — put network I/O on a latency-sensitive path or spend a goroutine and a round trip per tick forever against a condition measured in years. The exposure is redelivery of notifications the client already has, bounded by the window and self-healing on the next publish. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents): a replica that has received nothing must not answer 'caught up' (Codex round 9) P1 — the coverage check was skipped entirely while knownFrom was still 0, so a bus that had received NOTHING answered any cursor with an empty-but-non-nil replay, which the SSE handler reads as caught-up. The scenario is a restart, not an exotic one: replica B comes up while Redis is at 100, id 101 is published before B's subscription is live, and a client reconnects to B with Last-Event-ID 100 before 102 arrives. B says caught-up, then delivers 102 live, and 101 is gone with nothing to tell anyone. The principle the code now follows: having received nothing is strictly LESS knowledge than 'contiguous from X', so it must produce at least as strong a signal. A non-zero cursor against an empty bus is a gap. Both sides pinned, because the over-broad version is a real risk here — answering every fresh connection with a resync would be its own bug. A sinceID of 0 is not a resume and still gets an empty replay rather than a gap. Mutation-verified on the new arm. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(watchevents,cli): name the trailing-gap and shutdown trades (Codex round 10) Two findings that are decisions rather than defects, so both are documented at the line where the reasoning is met and taken to the plan instead of being settled unilaterally after ten review rounds. P1 as reported — the TRAILING gap. Everything the coverage bookkeeping does reasons about what this instance HAS received; it cannot see a notification missed at the END of the sequence. Hold 100, miss 101 to a disconnect, and a client resuming from 100 before 102 arrives is told caught-up. The hole only becomes visible when 102 lands, which is too late for that connection. What would reveal it is a GET of the sequence key: a value above lastAppendedID means ids exist we never saw, and a value BELOW it reveals the counter reset documented last round — one mechanism, both open windows. It is not done here because it is product-visible in the other direction: INCR happens before the message propagates, so the counter legitimately runs ahead of every instance for microseconds after each publish, and a strict comparison turns ordinary in-flight traffic into spurious sync_required responses with no principled tolerance to pick. A resync is recoverable and a lost nudge is not, which is the argument for doing it — but that is a call about how chatty the resync path should be. P2 — closing the watch bus before Shutdown drains handlers means a push already in flight can publish into a closed bus and still return 200 with pushed:true. Closing after would instead hold every shutdown to its 30s deadline on any open stream. eventBus already makes the same trade the same way; naming it rather than inheriting it silently. The honest fix is Bus.Publish reporting the drop so the handler can, which is an interface change and a different unit. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * feat(watchevents): close the trailing gap with a settle-window authority check (lead ruling) Lead's ruling on BUG-2651: a silently lost nudge is unbounded staleness, a spurious resync costs one redundant fetch, so the gap must not survive — and don't pick a magnitude tolerance, because the reason the counter legitimately runs ahead is in-flight propagation, which is TIME-bounded while a genuinely missed message never arrives. So the discriminator is time. On a resume (and only on a resume), read the shared counter: if it disagrees with this instance's high-water mark, wait out one settle window and read again. In-flight ids land during the beat and the resume proceeds normally; missed ones never do and the resume is answered with a gap. That converts an unprincipled 'how many ids behind is too many' threshold into a principled propagation bound. The same read also catches the counter having gone BACKWARDS, so the counter-reset window documented last round is closed by the same mechanism rather than needing its own — the arrival-time reset handling stays, because it is what repairs the instance's own state and what covers a bus with no reconnecting clients. Ordering matters and is documented at the call: the check runs WITHOUT the mutex (it sleeps and does network I/O, neither of which may happen inside the lock fan-out needs) and BEFORE subscribing rather than between subscribe and replay, which would reopen the double-delivery window SubscribeAndReplaySince exists to close. Nothing is lost by waiting first — fanOutLocally buffers regardless of subscribers. An unreadable counter falls back to local knowledge rather than failing closed: turning a Redis hiccup into a resync for every reconnecting client at once is a worse failure than the one being guarded against. EventsSince deliberately does NOT do this and says so — it is the local primitive the Bus interface already describes as being for tests and non-resuming callers, and making it sleep and hit the network would surprise every one of them. Five tests, each pinning a different half: the missed tail reports a gap; a current instance does NOT (the control that stops this being 'always resync'); an id arriving mid-settle is tolerated; an unreadable counter falls back; a fresh subscriber neither waits nor gets a gap. Mutation-verified twice — disabling the check, and removing the settle beat — each killed by the test that names it. Also filed at the lead's direction, so the two remaining cross-instance defects have tracked homes rather than only comments: BUG-2698 (targeted push resolved against local presence, plus the delivered_sessions inaccuracy — one shared-state SessionPresence closes both) and BUG-2699 (push returns 200 pushed:true for a dropped publish; Bus.Publish reports nothing, and fixing it is an interface change). Every disposition comment now cites its item. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents): compare two FRESH reads, not one stale snapshot (Codex round 11) P1 — the settle beat re-read only the local side, so the comparison was against a counter SNAPSHOT taken before the wait. Id 2 arrives during the beat while id 3 is published and missed: the stale remote is still 2, the check declares convergence, and 3 is silently lost — the exact failure this whole mechanism exists to prevent, reintroduced inside it. P2 — the same staleness in the other direction. A GET can land just before a publish completes and report a value BELOW what this instance already holds; that never matches, so a client who had missed nothing got a full resync. Both are one defect: agreement between the authority and this instance has to be evaluated on two FRESH reads or it is not agreement. Now re-reads both sides after the beat, and treats any remaining disagreement as a gap in either direction — still behind means ids never reached us, still ahead means the counter was reset under us and our buffer belongs to a dead id space. Two tests, one per direction, each mutation-verified against the re-read-locally-only version: the second counter advance must produce a gap, and the raced read must NOT produce a resync. Without the second test the fix could have been 'always report a gap', which passes the first. Documented the cost side of the lead's ruling while I was in here: the condition is agreement, so a resume during CONTINUOUS publishing across the whole settle window can disagree every time and resync. Bounded by this stream being low-volume by design and resumes only happening on reconnect; if a workload makes it chatty, the answer is a longer window, not a magnitude threshold. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents): an absent sequence key is zero, not unreadable (Codex round 12) P2 — the counter key can DISAPPEAR after this bus has seen ids (FLUSHDB, eviction). Reading redis.Nil as 'unreadable' meant falling back to local knowledge and cheerfully replaying an id space the authority no longer has — while the next publish starts again at 1 and collides with it. Absent is a VALUE. Returning zero-and-readable makes the case fall out of the ordinary comparison with no special branch: an instance holding 101 disagrees with an authority at 0, does not converge, and the resume is answered with a gap. A genuinely fresh deployment still agrees at zero and is not resynced — which is the control leg, and the reason 'absent means gap' would have been the wrong fix: it passes the first test while resyncing every first connection on a new install. P1 as reported — the equality fast path returning without settling — is not closed, and the comment now says why rather than leaving it to be re-found. A notification published AFTER that read and missed by this instance is invisible to any check made here, and settling anyway would not close it: the same race exists in the instant after the function returns. The check's honest scope is what was missed BEFORE the resume. A message missed after it is a property of at-most-once pub/sub with no per-connection ack, and the real answer is a durable stream (Redis Streams with consumer groups), not a longer wait. Mutation-verified: restoring redis.Nil to the unreadable branch fails the disappearing-counter test. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * feat(watchevents): epoch marker, so a reset that caught up is still a reset (Codex round 13) P2 — numeric detection is blind to a reset that has already climbed past this instance's high-water mark. Hold 100, lose the connection, the counter resets and ids 1-101 are published, and the only one that reaches us is 101 — the perfect contiguous successor of 100. Every arithmetic check passes, the buffer quietly mixes two id spaces, and a client resuming from OLD 100 is handed NEW 101 having silently missed the new space's 1-100. No amount of comparing numbers fixes that, because the question is not 'is this bigger' but 'is this the same sequence'. The publish script now mints an epoch once per id space (SET NX, so every publisher can offer one and the first wins) and carries it on every message; a change drops the buffer and re-anchors. The subtle half, and the one the first attempt got wrong: after an epoch change the cold-start rule must NOT admit its usual contiguous-with-our-view cursor. Within an epoch, a client at n.ID-1 is genuinely adjacent to our first id. Across one it is ambiguous — id spaces overlap, so that cursor may be the OLD sequence's n.ID-1, a different notification entirely — and admitting it hands them the new epoch's id as though it followed theirs, which is exactly the failure the epoch exists to prevent. Letting it back in one line later would have been a poor joke. The test caught it; the control leg (a cursor genuinely inside the new epoch is still served) is what stops the fix becoming 'resync everyone forever after any reset'. Wire format changed to <epoch>|<id>|<json>. Free of compat cost, checked rather than assumed: redis_bus.go does not exist on origin/main, so no released build produces or consumes the old shape. The numeric backward check stays — it covers a counter reset where the epoch key survived (eviction picks keys individually), and it is what repairs an instance with no reconnecting clients at all. Mutation-verified: ignoring the epoch change fails the new test. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(watchevents): the wire format comments say <epoch>|<id>|<json> (Codex round 14) Three comments still described the pre-epoch format. Worth more than a tidy-up: a maintainer following them would conclude the epoch prefix is vestigial and remove it, which reintroduces exactly the cross-epoch replay corruption round 13 existed to fix. The publishScript comment now also says outright that the epoch is not decoration and points at redisWatchEpochKey before anyone considers it removable. Verified by grepping for the old shape rather than by trusting the edits — zero remaining, which is the check I owed after getting this wrong in round 7. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * chore(nix): update vendorHash for the miniredis test dependency (BUG-2651) CI's Nix job failed on a fixed-output hash mismatch, and it is neither a flake nor a surprise once seen: nix/package.nix pins the vendored module set, and adding miniredis (plus gopher-lua, its Lua interpreter) to go.mod changed it. Regenerated per the procedure the file itself documents — build and read the 'got:' line. Run on CI rather than locally because this box has no nix; the hash is a content hash of the module set determined by go.mod/go.sum, so the same inputs produce it in either place. Worth naming as a gate lesson rather than just fixing: my pre-merge matrix had build, lint, test, test-pg, vuln and Codex, and none of them can see this. A dependency change has a SEVENTH consumer — the Nix packaging — and the only thing that checks it is the CI job that just did. Adding a dependency means checking the packaging, not only the security scan. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V	2026-08-20 02:54:53 -04:00
xarmian	449ac109e9	fix(server,mcp,cli): refuse system-metadata keys in fields_patch + retry-hostile error code (BUG-2627 part 2, BUG-2675) (#1166) ... * fix(server,mcp,cli): refuse system-metadata keys in fields_patch + retry-hostile error code (BUG-2627 part 2, BUG-2675) Part 2 of BUG-2627 closes the door that mints the defect parts 1 and 3 dealt with: `--field implementation_notes=<json>` stored the entries as a JSON-ENCODED STRING, which is invisible to every reader and — since part 3's guard — disables `pad item note` on that item until the row is repaired. Refused SERVER-SIDE in `fields_patch`, not at the CLI as the item's scope line proposed. The deviation is deliberate and recorded on the trail: the CLI is one of three clients, and all three lower a user field-setter into the same key (`pad item update --field` at cmd_item.go, the MCP `field` param via dispatch_http_advanced.go on remote, and stdio by shelling out to that CLI). One gate closes all three; a CLI-only refusal would have left remote MCP writing the key. Both call sites were read, and the CLI's lowering is now pinned by a test rather than left as an assumption. Scope, stated because it is deliberate: this closes UPDATE only. The full `fields` blob stays open because that door is SHARED — `pad item note` / `decide` / `github link` send one, and so does convention activation via BuildConventionItemFields -> ItemCreate. Closing it would break the system writers the gate exists to protect. Item create therefore remains a mint site, tracked with the rest of that surface in BUG-2685. The refusal message is per-key: implementation_notes -> `pad item note`, decision_log -> `pad item decide`, github_pr -> the GitHub link flow, and `convention` refuses WITHOUT naming a command, because none writes it. PATTE-135 wants a remedy that works in the failing state; a single "use pad item note" line would have been wrong for three of the four keys. BUG-2675 rides along on one ToolSurfaceVersion bump, as ruled. The append refusal from part 3 reached MCP agents as `server_error` — not our fault, and not transient, so agents could reasonably retry a failure that is deterministic forever. New closed-set code `stored_state_unreadable`, emitted on BOTH transports: HTTP classifies the sentinel error directly, stdio via a `pad-structured-error/v1:` marker the CLI now writes for its own local refusal (the first marker generated without an upstream APIError). v0.16-then-v0.17 is what a one-transport fix costs. Also here: - items.ReservedOverrideKeys -> ReservedFieldKeysIn. The second caller passes a patch, not an override map, and the old doc comment said fields_patch was an open exposure — true until this commit. - `Extract* returns nil for THREE reasons` -> FOUR. The comment listed four; the count was corrected everywhere except the code. - Consumer-read artifacts updated where the claim is ACTED on, not only where it is documented: instructions.md (incl. a "do not retry this code" section), the catalog `field` param description, `pad item update --help`, README. Gates: build · make lint · go test ./... · make test-pg · Codex. Eleven-mutation matrix run against the new tests; every one killed by an assertion (two were rewritten after killing by compile error / surviving, which proves nothing). Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(server,mcp): honest remedy when the stored value is already unreadable; name the MCP-facing code (Codex round 1) Three findings from the pre-push review, all real: P2 — the refusal named `pad item note` unconditionally, but on an item whose stored value is ALREADY undecodable that command refuses too (part 3's guard). The caller was routed in a circle: field write refused -> run the note -> refused -> back again. That is exactly the failure PATTE-135 exists to prevent, and my own trail had reasoned the remedy was safe on the strength of the HEALTHY case only. The message now inspects the item's stored value and, when the key is unparseable, says so and points at the one action that works in that state (inspection), noting that the repair needs a full `fields` write no CLI flag exposes. P2 — two doc claims were false where an actor reads them. The catalog said reserved keys are refused "on every action that accepts field", which includes CREATE, and create is deliberately NOT gated; and both the catalog and instructions.md named `validation_error` (the HTTP code) where an MCP client actually receives `validation_failed`. Both corrected, and the create exception is now stated rather than implied by omission — an agent that reads only "refused on update" will otherwise assume create is fine, which is how a hole gets used. nit — the destructive-downstream sentence claimed every reserved key becomes unreadable and trips an append guard. True only for the two append-backed keys; github_pr and convention are simply overwritten. The clause is now per-key, because a confident wrong explanation is worse than a vague right one. Two more mutations run against the new branch: always-readable (the circular remedy returns) and never-readable (the working remedy disappears) — both killed by assertions. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(models,mcp,cli): one appendability predicate, per-key docs, stdio hint parity (Codex round 2) Five findings, all real. P2 — the message's readability check and the guard it describes were two different decodes. Mine unmarshalled into []json.RawMessage; the guard uses []ItemImplementationNote. A stored `[1]` passed mine and fails the guard, so the message would again have prescribed a command that refuses — the same circularity round 1 caught, through a narrower door. Replaced with models.StructuredFieldIsAppendable, which ASKS the guard rather than re-deriving it, plus an agreement test over 12 shapes x 2 keys that compares the predicate against the real Append* helpers. Verified by restoring the RawMessage version: the table catches it on `[1]`. P2 — stdio lost the new code's hint. Remote MCP told the agent retrying is pointless and how to inspect; stdio got the code with an empty hint, because the CLI's marker envelope carried none and the classifier parsed none. Both fixed, with the hint hoisted into paired constants (the same duplication StructuredErrorMarker already uses) and the test comparing the two TRANSPORTS' envelopes rather than either against a literal. P2 — doc text was still false for `convention`: the catalog, the instructions and `--help` all said reserved keys are maintained by note/decide/the GitHub flow, which is true of three of the four. Each key now names its own writer, and `convention` names library activation. Also dropped the `malformed_override` advertisement — that is the SERVER's code; an MCP client sees validation_failed for both refusals. nit — the classification test called structuredAppendErrorResult directly, so deleting either dispatcher call site left it green. Added dispatcher-level tests driving the real server + store, asserting the code, the hint, and that the item's stored fields are byte-identical afterwards. Mutation-verified by reverting the note call site. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(items,models,mcp): github_pr stays writable through fields_patch; no nil-map panic (Codex round 3) P1 — the gate refused `github_pr`, and that was wrong. My model was "system writers use the full fields blob, user setters use fields_patch", which holds for three of the four reserved keys and fails for this one: `pad github link` needs a local git checkout and the `gh` CLI, so it is excluded from remote MCP BY NAME, and internal/mcp/dispatch_http.go's noRemoteEquivalent map tells remote agents in so many words to use `item update --field github_pr=...` instead. For that audience the patch door is not a bypass of the writer — it IS the writer. So the refusal deleted a documented capability from remote agents, and answered with a message naming a command they cannot run: the same circular remedy round 1 caught, aimed this time at the people the gate was meant to help. items.PatchRefusedFieldKeysIn now exempts the key and records the rule being applied — refuse a raw write where a real writer exists — rather than the list it produces. Whether remote agents should get a proper PR-link action, so the key can be closed too, is a product question and is left as one. P2 — the hint told agents to read the bad value with `pad_item action=get`. They cannot: stripDuplicatedFieldsKeys removes implementation_notes and decision_log from every MCP response's fields blob, and the top-level arrays come from the extractor, which returns nil for exactly this shape. The value is invisible on the whole surface. The hint now says so and routes to a human, who can read it with `pad item show --format json`. P2 — `fields` holding a literal `null` unmarshals into a NIL map with no error, and both Append* helpers assign into what they get back, so `pad item note` PANICKED ("assignment to entry in nil map") instead of appending. Reproduced, fixed in parseMutableItemFields, and pinned by a test that fails on a panic rather than taking the process down. An absent blob and a null blob mean the same thing to every caller. Pre-existing, but it sits in the function family this bug is about and the message was about to recommend the command that panics. nit — README claimed a "closed eight-code taxonomy" (17 codes, and I had just added one) and read as if create lowers into fields_patch. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(models,mcp): predicate matches the append on malformed blobs; stop promising a broken workaround (Codex round 4) P1 — round 3 exempted `github_pr` from the update gate on the strength of noRemoteEquivalent's documented workaround. That workaround does not work: ingestFieldKVP (remote) and parseFieldFlag (CLI, and so stdio) both store a `field` value as a STRING, so the PR data lands double-encoded and no link appears — the BUG-2627 shape one key over. Filed as BUG-2696 with the three candidate fixes; NOT folded in, because the narrowest of them changes how every field value is typed. The exemption stands regardless: refusing would leave remote agents with strictly less than a broken door. What changes is what we may PROMISE. The catalog, instructions.md, version.go and README said "this is how you link a PR"; they now say the door is open and broken, and to hand PR linking to a human. Advertising a capability that isn't there is the failure mode this whole unit keeps circling. P2 — StructuredFieldIsAppendable returned TRUE when the whole fields blob was unparseable, on the reasoning that a broken outer blob is a different problem. True of the cause, irrelevant to the caller: the Append* helpers bail on that same parse, so the message again named a command that fails. It now returns false, which is simply the honest answer to the question asked, and the agreement table grew a malformed-outer-blob leg — the gap that let the disagreement through. P2 — the message claimed a raw field write always stores something Pad cannot read back. That holds for the CLI and MCP (a `--field` value is typed by schema lookup and these keys are in no schema) but not for a direct REST caller sending a valid array, who is refused for ownership reasons alone. Reworded to say both parts. nit — a misplaced parenthetical in the README read as if item CREATE lowers into fields_patch. It does not; it sends the full blob. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(mcp,models): stop the remote hint advertising the broken PR workaround; classify an unparseable blob as retry-hostile (Codex round 5) P1 — I corrected four artifacts that pointed agents at the github_pr field write and missed the fifth: noRemoteEquivalent's own text, which IS the message a remote agent receives when it calls `github link`, and which Codex had quoted at me in round 3 to establish the workaround existed. The nearest artifact to the actor was the one I did not open. Both entries now say there is no working remote path and name BUG-2696, with a test pinning the negative so a future edit cannot quietly reinstate the advice while the write is still broken. P2 — a fields blob that will not parse at all produced a bare parse error, so `note` / `decide` reached agents as `server_error`: transient- looking, and therefore retried, for a failure that is as deterministic as the per-key one BUG-2675 exists for. Both Append* helpers now wrap that parse failure in ErrStructuredFieldUnreadable, which both transports already classify, and the malformed-blob test asserts the sentinel rather than just an error. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(mcp,cli): qualify what an agent can actually see when the state is unreadable (Codex round 6 nit) Round 5 widened stored_state_unreadable to cover a fields blob that fails to parse outright, which made half of its own hint false: MCP's normalization strips a broken structured KEY (so `get` hides it), but leaves an unparseable BLOB as a raw string (so `get` shows it). The hint and instructions.md asserted the first case for both. Now stated per layer, in the two paired constants and the instructions. The reason it is worth the words rather than being cut: an agent told 'you cannot see this' does not look, and would have missed a value that was in fact right there in the response it already had. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(mcp): classify the move/copy reserved-key refusal as validation on stdio too (Codex round 7) P2 — carried over from v0.22, surfaced because THIS bump documents the two reserved-key refusals as agreeing across transports. The move/copy message ("Field(s) reserved for system metadata and not settable here") matched none of the stdio validation patterns, so the same deterministic 400 arrived as validation_failed on remote and server_error on stdio — and server_error reads as transient, so an agent retries a refusal that can never pass. One pattern added, plus a test that drives both real classifiers with the real server message text for both refusals, so a reworded message that stops matching fails here rather than in the field. nit — the github_pr exemption is UPDATE-only; move and copy still refuse it, because there the argument is BUG-2674's (an override reintroduces the key the migration just dropped), not this one's. The catalog and instructions said "not refused" without that qualifier. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(mcp): cover the copy path's own refusal wording in the stdio classifier (Codex round 8) P2 — round 7 fixed the MOVE wording; the copy path words the same class of refusal differently ("Destination collection has no field(s): ..."), so it kept arriving as server_error on stdio and validation_failed on remote. Third message in one family, and the round-7 test used the move text for every case, which is why it missed this. The parity table now carries all three real messages plus a control leg using one the pattern list already covered — without it the table could pass by matching everything. Recorded in the pattern list's comment rather than left implicit: matching prose is a stopgap, the structural fix is the pad-structured-error/v1 marker that carries the code instead of inferring it, and until a refusal emits one, this test is where a new wording has to be added. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * test(mcp): use the real upstream codes in the parity fixtures (Codex round 9 nit) The copy legs carried `validation_error` where the handlers actually emit `malformed_override` and `invalid_override`. The 400 branch ignores the body code today, so the test passed either way — which is exactly why the fixture mattered: it was quietly recording a wrong contract, and a future code-aware classifier would regress against a table that agrees with it. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(mcp): the upstream code is not forwarded to MCP clients (Codex round 10 nit) The catalog said the server's own code (validation_error / malformed_override) appears in the MCP message. It does not: the 400 branch emits code=validation_failed with a fixed "Validation failed." message and the server's text in the HINT, discarding the finer-grained code. Reworded to say what an agent actually receives, and to say that telling the two refusals apart means reading the message. Also carried the update-only qualifier on the github_pr exemption into the README, matching the catalog and instructions. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(items): state the exemption predicate, not the exemption list (lead ruling) The lead's ruling on the github_pr reversal: make the REASON what the code says, so the next key added to reserved metadata is evaluated against 'does this audience have a real writer?' rather than pattern-matched onto a list that happened to be wrong for one key. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V	2026-08-19 23:17:24 -04:00
xarmian	de96cce900	fix(items,server,web): reserved metadata survives a move; referential metadata travels only within its context (BUG-2674) (#1165) ... * fix(items,server): reserved metadata survives a move, and dropped fields are reported (BUG-2674) Moving an item destroyed its implementation notes, decision log and linked-PR metadata. Well-formed data, on a routine documented operation, silently, with a success message. Reproduced before the fix: a note written through `pad item note` — correct shape, visible on every surface — was gone after `pad item move`, leaving fields as `{"status":"new"}`. ## Why it happened items.MigrateFields drops every key absent from the TARGET schema. The reserved keys — implementation_notes, decision_log, github_pr, convention — are system metadata that NO collection schema declares; each renders from its own dedicated surface rather than as a generic field. So they are absent from every targetDefs and were dropped on every move. That blindness is structural, not incidental: any code path reasoning about fields BY CONSULTING A SCHEMA cannot see these keys. It is the shared root of this bug and of BUG-2627, where the CLI types a --field value by schema lookup and these keys fall through to a raw string. ## The enumeration comes first, deliberately Before this there were four constants and exactly ONE non-test consumer treating them as a set — an inline || chain in a CLI display path. Naming the set inline again here would have created the SECOND hand-maintained list, which is the generator pattern behind both bugs reproduced inside its own fix: the next reserved field lands in the constants, gets wired into whichever surface prompted it, and silently misses the other. So models.IsReservedItemField is now the single place that knows, MigrateFields consults it, and the CLI's || chain is converted to it — the only way it is provably THE list rather than A list. (formatChangeValue keeps its per-key switch: it needs to know WHICH reserved key it has, to say "notes" vs "entries", not whether the key is reserved.) `convention` is IN the set, settled with evidence rather than by the principle alone: 35 of 36 conventions in a live workspace do not store the key at all, and the one that does holds a blob that is a redundant mirror of the alias keys beside it. No user types a `convention` object — ApplyItemConventionMetadata writes it, via library activation and the web form. System-stamped. ## Contract System-minted non-referential data carries; anything dropped is reported. PLAN-2357 DR-17 settled the analogous case — tags carry because "there is no workspace-scoped foreign key to break, so dropping them would lose information for no safety reason". These are the same shape: inert JSON with nothing that could dangle in a destination. The plan's carry list simply never considered them, so there was no deliberate semantics to defer to. DR-17's own heading is "None of this may be silent." ## The reporting half MigrateResult.Dropped has always existed and the single-move handler has always thrown it away, so the only record of a field disappearing was the field being gone. It now rides the move's audit metadata — not the response body, which is the bare item and would break every consumer, and the activity timeline is where someone asking "what happened to my item" looks. Joined into one string because that map is map[string]string and a raw array renders as a Go map literal in the timeline (BUG-2628). ## Verified Unit: reserved keys carry with their payload INTACT (asserted on the value, not merely the key — a carry that re-encoded or zeroed it would pass a presence check), and bypass schema matching entirely, so a target declaring `implementation_notes` as `text` cannot coerce them. Mutants run: guard removed -> both new tests fail; carried-but-also-reported-dropped -> the not-dropped assertion fails; carry-everything -> the control leg fails alongside three pre-existing tests. Live, against a server built from this branch: the note survives the move byte-identical, and the move's activity metadata carries `dropped_fields: "priority, status"` for the values the target schema genuinely has no home for. ## Known scope limit The BULK move path still discards its Dropped list — a reporting gap only, since the carry-through lives in MigrateFields and bulk inherits it. Threading the list out crosses two function boundaries whose signatures serve every bulk operation, so it is a refactor of the bulk dispatch's return contract rather than a line. Filed as BUG-2683 rather than smuggled in here. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(server,web): close the four gaps Codex round 1 found on the carry-through (BUG-2674) Round 1 raised no P1 and four P2s. Three were real defects introduced or exposed by the carry-through; one was a genuine overclaim in the previous commit. All four closed here, each mutation-verified rather than asserted. ## A schema may no longer declare a reserved key MigrateFields carries these keys by identity, but every caller then validates against the target schema — and ValidateFieldsDetailed iterates schema.Fields, so it DOES see a declared key. A target declaring implementation_notes as `text` would receive the carried array and reject it, turning a move that previously destroyed the notes into one that fails outright. That is a worse failure than the one being fixed: loud, but it blocks an operation that used to work. The gate already existed — validateNoReservedFieldKeys, with its grandfathering — and listed only parent/plan. The four metadata keys join it, sourced from models.ReservedItemFieldKeys() so the two lists cannot drift. Forbidding the declaration is the honest fix; coercing the value, or skipping validation for a key the schema genuinely declares, would be guessing at which meaning the author wanted. The web's RESERVED_FIELD_KEYS gains the same four, preserving the existing deliberate asymmetry (the client lowercases and is therefore stricter than the server's exact match) so the UI steers authors away before the 400. ## The copy preflight no longer under-reports `carried` is built by walking the DESTINATION SCHEMA, and these keys are declared by no schema anywhere — so after the carry-through they appeared in NEITHER bucket. A copy of an item whose content is its notes would report "nothing carries over" while in fact retaining them. Before the carry-through they at least showed under `dropped`, accurately. Reporting in neither is a regression in the preflight's honesty, which is the same defect class as the move that reported nothing. They are now appended to `carried` after the schema-ordered entries, marked `type: "system"` with a rendered label since they have no author-supplied one. The bucket's doc comment says so: a client must no longer assume every `carried` entry resolves to a destination FieldDef. ## The audit report now reaches a human The previous commit claimed the activity timeline is where someone asks "what happened to my item" — true, and the timeline renderer ignored the key, so the report existed only for API and CLI consumers. Stored-but-invisible is not reported. TimelineActivityCard renders the dropped keys on a move. ## Test aliasing The "untouched" assertions compared the result against the SAME objects passed in, so an in-place mutation would change both sides and DeepEqual would stay true. The expectations are now independent deep copies — the only thing that makes "untouched" mean untouched. ## Mutants, each run Preflight pass removed -> the carried assertion fails. Timeline block disabled -> the render assertion fails. Timeline action guard dropped -> the non-move negative leg fails (a presence-only test would have passed it). Reserved-set helper returning everything -> the IsReservedItemField control leg fails. ## Not fixed here Codex's remaining observation — that a cross-workspace copy now carries github_pr into a workspace whose repository it does not describe, and leaves a convention blob detectable on an item outside the conventions collection — is a product question about what a copy MEANS, not a defect in this mechanism. Raised for a ruling rather than decided inside a bug fix. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(items,server): referential system metadata travels only within its context (BUG-2674) Lead ruling on the copy-semantics fork Codex round 1 raised. It does not add an exception to the carry rule — it applies the qualifier the rule already had. The contract was "system-minted NON-REFERENTIAL data carries". github_pr is referential: it names a repository that is a property of the SOURCE workspace's project, and it hydrates into code_context and renders as a live PR link. Carried into another workspace that link is a false statement about the destination's project, not preserved information. implementation_notes and decision_log describe the item's own history and are true wherever the item is. So the rule stays one sentence: non-referential system data carries everywhere; referential system data carries only where its referent's context still holds. ## Scope is a required argument MigrateFields takes items.MigrateScope. Required rather than defaulted because BOTH wrong answers lose something: SameWorkspace on a cross-workspace copy carries a PR link into a workspace it does not describe, and CrossWorkspace on an ordinary move DROPS metadata from an item whose repo context never changed. A caller that must name its scope cannot pick one by omission. The two move handlers pass SameWorkspace as a property of the endpoint, not a guess — a move changes an item's COLLECTION and cannot change its workspace. The copy and its preflight COMPUTE it by comparing workspace ids rather than assuming cross-workspace, because that endpoint accepts a target_workspace equal to the source; hardcoding would drop a github_pr from a same-workspace duplicate. Both sides use the same helper, or the preview promises a carry the copy drops — the DR-6 divergence the shared endpoint exists to prevent. ## The drop is reported, with a reason that explains itself PLAN-2357 DR-17: "None of this may be silent." It would be perverse to reintroduce a silent drop inside this fix's own new branch. The preflight reports it as `referent_not_portable` rather than the generic `no_target_field`. That generic reason would be actively misleading here: no schema declares these keys ANYWHERE, so "the destination has no such field" is equally true of the source and explains nothing about why the value is being left behind. ## Verified Mutants run: scope ignored (always carry) -> the cross-workspace leg fails; generic reason on the preflight drop -> the reason assertion fails. The same-workspace leg and the non-referential-sibling leg are what stop an implementation that ignores scope in EITHER direction from passing — each half alone is satisfiable by a constant. Gates re-run for THIS commit: lint 0 · go test ./... 0 · make test-pg 0 (3282). Web gates NOT re-run and not claimed: this commit touches no web file (the web half of BUG-2674 shipped in 82577a74 and is unchanged here). ## Noted, not fixed handlers_items_copy_preflight.go already documents the same defect class for RELATION fields — a same-named relation carries a SOURCE-workspace item id across workspaces and is reported as a clean carry — and says the fix "belongs in MigrateFields, for both callers at once". MigrateScope is now the mechanism that comment asks for, but wiring relation fields through it is a separate change with its own semantics to settle. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(items,server): close Codex round 2 — grandfathered schemas, stale drop reports, scope coverage (BUG-2674) Round 2 raised no P1 and three P2s plus a nit. All four were real; two are defects in round 1's own fixes. ## Grandfathered schemas that already declare a reserved key Round 1 added the four metadata keys to validateNoReservedFieldKeys, which stops the collision being CREATED — and that gate deliberately GRANDFATHERS schemas that already have one. I did not follow through: such a FieldDef still reached ValidateFieldsDetailed, met the system-owned array MigrateFields hands through by identity, and rejected it. A collection whose only sin is a field name someone was once allowed to pick would fail every move and copy. ValidateFieldsDetailed now skips reserved keys outright. That is not "ignoring validation": these values have no user-authored schema to validate against, by design — the schema entry is the anomaly, not the value. ValidateFields inherits it through the same call. This also closes the second half of the same finding: the preflight could report one key in BOTH needs_value and carried, because the issue came from validating a key the carried-append also emits. No issue, no collision. ## Dropped reports that were no longer true MigrateFields computes Dropped BEFORE overrides merge and before defaults are injected, so a key it lists may have been supplied moments later. Both the move audit (which I added in this branch) and the preflight's dropped bucket reported those anyway — claiming "we discarded your due_date" about an item that HAS a due_date. That is worse than the silence it replaced: silence at least does not send someone hunting for data sitting on the item, and a report that cries loss over visible data teaches the reader to distrust the channel. items.StillDropped filters against the FINAL map so the report is true at the moment it is written. ## Scope coverage attachments_copy_plan_test models a copy from workspace A into B and passed SameWorkspace — the wrong scope stated confidently in a test whose whole subject is a cross-workspace copy. It came from the bulk edit that threaded the argument through, which picked a value rather than reading each fixture. And nothing proved the MUTATING copy honours scope at all, so a call site passing the wrong one — precisely the mistake a required argument exists to prevent — would have shipped green. TestCopyEndpoint_ReferentialMetadataTravels- OnlyWithinItsWorkspace covers both directions end to end. Mutant run: the store call site pinned to SameWorkspace now fails the cross-workspace leg. ## The nit was an overclaim, so it is fixed in the code 38fa8fec said the copy and its preflight "use the same helper". They did not — the helper lived in the server package and the store duplicated the comparison inline, which is how a preview and its copy drift apart. items.ScopeFor now lives in the package that defines the type and both call it. Gates: lint 0 (after a gofmt fix lint caught) · go test ./... 0 · make test-pg 0 (3283). No web file touched; web gates not re-run. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(items,server): move the validation skip to the right altitude, and finish the drop-report fix (BUG-2674) Codex round 3, no P1, two P2s. Both say round 2's fixes were applied at the wrong altitude — correct in the case in front of me, wrong for the callers I did not enumerate. ## The validation skip was global; the problem is local Round 2 made ValidateFieldsDetailed skip reserved keys. That validator is shared with create, full update, artifact import and every bulk path — none of which migrate anything. On a GRANDFATHERED schema (one that already declared a reserved key before the round-1 gate), those paths genuinely did validate the key, and the skip stopped them: arbitrary junk could be written into implementation_notes through create, while fields_patch kept rejecting it via ValidatePartialFields. Full and partial updates disagreeing about the same key is a worse bug than the one I was fixing. Reverted. items.SchemaForMigratedFields strips reserved FieldDefs from the schema used to validate the OUTPUT of a migration, and only the four migration and copy sites call it. Create and update keep enforcing the declaration, because on those paths the user really is authoring that key. ## StillDropped reached two of three surfaces The move audit and the preflight were filtered; the MUTATING copy was not. migrateCopyFields returned the raw pre-override list and the 201 response exposes it as warnings.dropped_fields — so one request could report the key carried in the preview, PERSIST it, and still call it dropped in the copy's own response. Three surfaces, two answers. ## And StillDropped's own test was too weak Presence is not the test — present-and-non-nil is. The move path writes overrides straight into the map including a nil, where the copy path deletes the key, so `{"due_date": null}` on a move left the key present carrying nothing. Treating that as restored suppresses a REAL drop, which is the silent loss this change exists to end. ## A mutant survived, and the fixture was why `out.Fields = schema.Fields[:0]` + appends mutates the caller's backing array. The first version of the input-not-mutated assertion passed it twice: once because it checked length (Go passes the struct by value, so the caller's slice HEADER survives), and again after fixing that, because the reserved key was LAST in the fixture — the one surviving field was written back into the slot it already occupied. With the reserved key FIRST the corruption lands in slot 0 and the mutant dies. Recorded in the test, because the next person writing a "does not mutate its input" assertion in Go will reach for len() too. ## Comment accuracy The reserved-set doc claimed callers "inherit additions without edits". True for membership tests, false for the three places that need something a set cannot supply — referentialItemFieldKeys, reservedFieldLabel, and the web's separate RESERVED_FIELD_KEYS. Now listed, with the test that fires as the reminder. The collections-handler comment described only parent/plan and now says it covers two unrelated groups. Gates: lint 0 · go test ./... 0 · make test-pg 0 (3285). No web file touched. ## Flagged, not fixed The preflight labels a destination DEFAULT as from:"migrated" when the source had the key but migration dropped it — origin is keyed on presence in the source map, not on where the final value came from. Pre-existing and untouched by this branch; filed separately rather than folded in. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(items,server): close Codex round 4 — grandfathered defaults, override holes, duplicate carried entries (BUG-2674) Round 4, no P1, three P2s. All three are the same case I kept half-fixing: a GRANDFATHERED schema that declares a reserved key. ## Reserved declarations were still live in the defaults pass MigrateFields carried reserved keys by identity but then ran the target schema's defaults/required loop over them unchanged. A legacy Default was injected into system metadata as though a user had authored it, and a legacy Required produced a migration ERROR — which bulk move rejects on BEFORE reaching the stripped-schema validation. So a legacy target requiring implementation_notes failed bulk move while single move and copy succeeded: same key, same item, two answers depending on which button was pressed. ## Overrides were a hole straight through the rule A field override naming a reserved key was merged and then validated against the STRIPPED schema — i.e. not validated at all. Two consequences, the second worse than the first: - arbitrary junk could be written into implementation_notes / decision_log, bypassing the append guard BUG-2627 exists to enforce; - on a cross-workspace copy, an override could reintroduce the github_pr that MigrateFields had just dropped for leaving its workspace — defeating the scope rule by the simplest available route. The copy paths now gate overrides against the stripped schema, so a reserved key is undeclared there by construction and takes the existing malformed_override refusal. The MOVE path had no declared-key gate at all and gets a dedicated one (items.ReservedOverrideKeys). Refused rather than silently dropped: a caller who asked for a value and got an item without it has no way to tell. ## The preflight emitted reserved keys twice The carried walk iterated the raw target schema, so a grandfathered declaration was emitted there AND appended again by the reserved pass. The existing preflight/copy parity helper collapses carried entries into a map, so it could not see it — a check that de-duplicates before comparing cannot detect duplication. The walk now uses the stripped schema. ## Two mutants survived, and both were the test's fault - The defaults fix had no test at all. Written after the fact, it fails on the unfixed code on both halves (injected default, spurious required error). - The override test passed with the stripping REMOVED, because the ordinary destination does not declare github_pr — so UndeclaredOverrideKeys refuses it either way. Only a schema that DECLARES the key distinguishes the two implementations. The grandfathered fixture added for that fails the mutant with the PR link visibly written onto the copy. Also added the falsy-value legs to StillDropped (false / 0 / "" are restorations, not absences — a truthiness filter would report them lost) and drove SchemaForMigratedFields off the canonical set so a mutant stripping only implementation_notes fails. Gates: lint 0 · go test ./... 0 · make test-pg 0 (3289). No web file touched. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(items): correct the scope claim on ReservedOverrideKeys (BUG-2674) Codex round 5. The previous commit message said reserved keys are refused "on any path". True only for FIELD-OVERRIDE maps — the same-workspace move, the copy preflight and the mutating copy. An ordinary `fields` / `fields_patch` map still reaches them from the CLI, MCP, the web editor, artifact import, and Pad's own note / decision / convention / GitHub writers, which is by design for the system writers and a pre-existing exposure for the rest. The doc comment now says which paths it covers and, more importantly, what it is NOT — a general write gate. That distinction is the kind a future reader would otherwise take on trust from the function name. Round 5 was asked a different question than rounds 1-4: not "what is wrong with this diff" but "enumerate every path that could meet a declared reserved key, and is this approach right at all". It found ~10 further latent sites (create, full and partial update, artifact import, bulk status/priority, terminal options, unique_scope, computed, the web field editor, search, share presentation) — all PRE-EXISTING, none regressions from this branch, and all in the same grandfathered-schema case rounds 3, 4 and 5 kept surfacing. They are filed as BUG-2685 with the full map rather than patched here. Four rounds each finding another site is evidence about the DESIGN — reserved metadata living in the generic fields blob means every schema-aware consumer has to remember a special rule — and that is TASK-2657's territory, not a bigger version of this bug. This branch's scope was: a move destroys system metadata. That is fixed, tested and mutation-verified. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(mcp,cli): disclose the move/copy metadata rules where the ACTOR reads them; ToolSurfaceVersion 0.22 (BUG-2674) Caught by the pre-push step my own record exists for: I had documented this change carefully in commit messages, the PR body and the item trail — every one of them read by a human REVIEWING the work — and not at all in the artifacts read by the agent or operator ACTING on it. That is the same miss twice before, both times in this exact file. `field` is accepted for `pad_item.action=move` (catalog_item.go), so the refusal this branch adds is a limit an MCP agent will hit. It now says so in the param's own description and in instructions.md, which is the text agents receive at handshake. CLAUDE.md's `pad item move` and `pad item copy` blocks — the operator- facing reference — gain the carry rules and the github_pr exception. ## ToolSurfaceVersion 0.21 -> 0.22 BEHAVIOR bump on the v0.9 / v0.16 / v0.17 grounds: no tool, action enum or param SHAPE changed, but two things an agent can observe did. A move used to DESTROY implementation_notes / decision_log / github_pr / convention, silently, and now preserves them; drops of ordinary fields are reported in the move's activity entry instead of vanishing. And a `field` setter naming one of those keys answers `malformed_override` instead of writing it — a write that was never legitimate, since it bypassed BUG-2627's append guard and could reintroduce a github_pr the migration had just dropped. Compat posture stated deliberately: a caller passing such a setter today gets a 400 where it previously got a silent corrupt write. Relying on the old behaviour is relying on a defect — the same reading v0.17 took for the fields-blob shadowing. The bump was not free, which is the point: TestInstructionsMDVersionMatchesTool- Surface and TestReadmeVersionMatchesToolSurface both went red and forced the two other surfaces to be updated. That is the enforcement working — a version constant nobody could change without visiting every place it is published. Gates re-run for this commit: lint 0 · go test ./... 0 · make test-pg 0 (3289). CI was already 7/7 green on f6775bcb; pushing this restarts it, which is the correct trade against shipping agent-facing docs that describe the old behaviour. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V	2026-08-19 16:53:03 -04:00
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	2026-08-19 08:13:29 -04:00
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 d3c8234e — run 1: 191 passed (3.7m), job cancelled by CI/Nix twin-run concurrency race; run 2: 190 passed + 1 flaky (pane-controller, PLAN-2154's known flake, unrelated to this diff), job cancelled by the 10-minute cap after the flaky retry; run 3: rerun expired inside terminal-cancelled parent at 38s, no test signal. All six code-testing checks green (Go, Go-PG, Web, Nix, Smoke×2). The gate defect is filed as BUG-2645 (cap breachable by one flaky retry + twin-run race); the fix ships as its own reviewed workflow unit. Lead spot-check of the diff and full pin inventory: TASK-2637 trail.	2026-08-18 12:26:00 -04:00
杨成锴	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.	2026-08-18 10:44:50 -04:00
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.	2026-08-17 19:02:53 -04:00
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`.	2026-08-17 16:34:46 -04:00
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.	2026-08-17 14:26:09 -04:00
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 c61f4cda, and 998716ae deleted their renderer the next day as collateral of the unified-timeline PR. The write paths kept working on CLI and MCP, so the entries accumulated with no read surface outside `pad item show`. Surface them as two more timeline kinds rather than rebuilding a separate renderer: the endpoint already merges comments, activities and versions under cursor pagination, and notes/decisions carry the same timestamp/actor/body shape the merge handles. They differ from the other three kinds in one way that matters. They are elements of the item's fields blob, not rows, so they arrive whole on the already-resolved item instead of through a cursor query. Without an explicit filter they would therefore repeat on every page, so structuredTimelineEntries applies the same (created_at, id) predicate the SQL sources use. The blob is also hand-writable, which makes three shapes representable that a table would not, all covered: - no created_at: anchored at the item's own creation instant, the earliest moment the entry could have existed. A zero-time fallback would render as 1970 and sort below everything real. - no id: positional fallback, keeping the sort total and the cursor stable. - not an array at all: models.ExtractItem* already returns nil, so it contributes nothing. One live docapp item is in exactly this state (double-encoded JSON string) — filed as BUG-2627, a different defect. Every guard here was mutation-verified: dropping the merge, neutering the cursor predicate, and removing each of the two fallbacks in turn each fail the tests that cover them. That pass also caught a vacuous assertion in the actor test, which now counts the entries it asserts on (CONVE-12). Frontend wiring follows in the next commit; the kinds are invisible until ItemDetail's visibleKinds whitelist admits them. * fix(web): render note + decision timeline entries and admit them to the tab filter (BUG-2301) The server half is inert without this. `visibleKinds` is a WHITELIST with one live call site, so a kind ItemDetail does not list renders on NEITHER tab — a perfectly merged feed and an empty Activity tab, which is how this feature shipped invisible the first time. Two halves, both needed and both covered by mutation-verified tests: - ItemTimeline gains render branches for the `note` and `decision` kinds plus their rail dots. Without a branch the entry falls through the {#if} chain and draws an empty rail. - ItemDetail admits both to the Activity set. They belong there rather than with Versions: they record things that happened to the item, not restore points. One TimelineStructuredCard serves both kinds. They share a shape — headline, optional body, actor, timestamp — and differ in label, accent and weight, so a variant keeps them from drifting the way two near-identical components would. A decision carries the heavier treatment: it is the thing you go back looking for. Body text renders as plain text with `white-space: pre-wrap`, never through the markdown pipeline, because that is what the writers produce — `pad item note --details` and `--stdin` take raw text. A test pins that markup in an entry stays inert. The actor label reads the entry's self-declared `created_by`. That field lives inside the item's fields blob and no server stamps it (BUG-2542), so the label reports a claim, not a verified author; the comment in the card says so. * docs(skill): document `pad item note` / `pad item decide` now that they have a read surface (BUG-2301) The bug's own measurement found 185 notes and 33 decisions across seven workspaces written by people and agents who found these commands on their own — nothing in the skill, no convention, no playbook ever mentioned them. That was defensible while the entries were invisible outside `pad item show`; it is not once they render in the item timeline. Flag names verified against the built binary's `--help` rather than the source, since the skill is what an agent acts on. * test(server): assert timeline paging is exactly-once, on both drivers (BUG-2301) The single-page cursor assertions cover the predicate but not the property that matters to a reader scrolling an item: every entry appears exactly once across the whole feed. A too-loose predicate repeats the in-blob entries on every page and a too-tight one drops them at a boundary, and neither is visible from one page. Run on Postgres as well as SQLite because there is a genuine seam here: the structured entries are filtered in Go against a parsed time.Time while the comment/activity/version sources are filtered in SQL against a formatted string, and this endpoint has a Postgres-specific paging history (BUG-1086, the \xff sentinel). Portability is asserted, not assumed. The Postgres leg asserts the driver before doing anything, so it cannot pass by silently re-running SQLite — verified both ways: it SKIPs without PAD_TEST_POSTGRES_URL and PASSes with it. Mutation-verified too: neutering the cursor predicate fails the leg on both drivers. * fix(server): align the structured cursor with the SQL predicate and make blob ids unique (BUG-2301) Three defects from Codex round 2, all in the cursor path this change added. 1. The "g" sentinel split the two kinds on their first letter. When a client sends `before` without `before_id` the handler substitutes "g" — an upper bound whose whole job is to KEEP same-second entries, and which does that only because every lowercase-hex UUID character sorts below it. Structured ids are not UUIDs: `note-…` sorts above "g" and `decision-…` below, so comparing against it literally dropped every note at the cursor instant while keeping every decision. The handler now says whether beforeID is synthetic, and the filter honours what the sentinel MEANS. 2. Two comparison spaces met on one page boundary. The SQL sources format the cursor to whole-second RFC3339 text and compare against a text column, while this filter compared full-precision time.Time. A structured entry can carry sub-second precision — a hand-written created_at, or the item's own createdAt standing in for an absent one — so the two predicates could resolve the same boundary differently and drop or repeat entries around it. Both sides now compare formatted whole-second text; the seam is removed rather than compensated for. 3. Duplicate ids were trusted. Nothing validates them on write, and a repeat is not cosmetic: it collides in the client's keyed {#each} (a hard render error), the client's loadMore dedupes by id and would drop the older entry, and the cursor cannot page past two entries it cannot tell apart. Repeats now take the same positional fallback an absent id takes, in one map shared across both kinds since they land in one merged stream. Round 2's fourth item was a test gap rather than a defect, and is closed here too: the paged walk asserted only that the three structured ids appeared once, so a boundary mismatch that repeated a COMMENT or a VERSION would have passed. It now asserts no entry of any kind repeats. Round 1's only finding — structured entries do not live-refresh because the SSE filter excludes item_updated — is DECLINED and recorded on the item. That exclusion predates this diff and is deliberate (refreshing on every content save caused visible shakiness and rate-limit errors); version entries already carry the identical staleness, and these kinds have no web writer at all, so no user acts and waits on one. Each fix has its own negative control: removing the sentinel branch, reverting to full-precision comparison, and trusting raw ids each fail exactly the test that covers them. * fix(server): truncate structured entry timestamps to the shared whole-second space (BUG-2301) Codex round 3, P1 — and a correction to the previous commit, which fixed the comparison and left the value itself alone. Filtering in formatted whole-second text made the PREDICATE agree with SQL, but the entry still carried full-precision time, so two paths stayed wrong: - the merge sorts on TimelineEntry.CreatedAt, so a fractional structured entry interleaved against same-second rows by a component those rows do not have, in an order the SQL ORDER BY cannot reproduce. - the client echoes the last entry's created_at back as the next page's `before`, where the store formats it down to the second. A cursor of 10:00:00.5 becomes 10:00:00Z and EXCLUDES same-second rows that were still owed — silent data loss in comments and versions, sources this change never touched. Truncating where the entry is built puts it in the same space as every other source for all three purposes at once, which is what the fix should have been the first time. Covered end to end: a fractional entry at a page boundary must not cost a same-second row on the next page. Round 3's P2 (a `has_more` heuristic that can stay true without pagination progress when an over-fetched source is emptied by dedup) is NOT addressed here. It is pre-existing — the heuristic and the discards it counts on both predate this branch, and structured entries are never discarded by buildTimeline, so this diff neither causes nor worsens it. I have not reproduced it; recorded on the item for triage rather than asserted as real. * fix: render payload-less structured entries, and make the fractional-boundary test actually discriminate (BUG-2301) Codex round 4, all three findings. The important one is against my own test. The fractional-timestamp regression test walked two structured entries and no SQL-sourced row, so the data loss it was named for could not occur in it — and confirmed by mutation: with the truncation removed it still passed. Reworking it to include a real comment at the note's own second was not enough either, and the reason is worth writing down: the cursor's second term is the id, the SQL sources keep same-second rows with `id < before_id`, and a realistic `note-<nanos>` id sorts ABOVE every lowercase-hex UUID. The sibling row was rescued by the tie-break no matter what the timestamp did. With an id below the UUID space the loss is reachable, and the test now fails on the unfixed code by dropping the comment outright. Two rounds of a correct-looking test that could not fail. The tell both times was the same: I checked that the test passed with the fix and not that it failed without it, on a fixture I had reasoned about rather than run. Also: - A structured entry whose payload is missing now still renders its card. Guarding the branch on the payload left the rail dot and connector drawn beside nothing, which reads as a broken render rather than a thin entry; the card was already null-safe. Covered, and mutation-verified by restoring the guard. - Corrected a comment that claimed a zero-time fallback renders as 1970. Go's zero time is year 1, not the Unix epoch. * docs(models): qualify the timeline paging claim to the static-dataset case (BUG-2301) Codex round 5. The finding — the five sources are read at five instants with no shared snapshot, so a concurrent note write can land between the item resolve and the activity query and put one page briefly out of step — is real but is NOT fixed here, deliberately: - It is the endpoint's existing shape, not something the structured kinds introduce. Comments, activities and versions were already three separate reads at three instants; this adds a fourth source, not a fourth class of problem. - Nothing is durably lost. The blob is authoritative and the very next fetch is consistent; the window is a request's worth of milliseconds on a read-only feed. - Every fix that would actually close it (a shared snapshot or a read transaction spanning all five sources) is a change to the endpoint's contract and the store's API, which is not something to do inside a bug fix for a missing renderer. What IS wrong and is fixed: my own comment claimed paging "behaves identically for all five" without qualification, and the earlier commit claimed exactly-once paging flatly. Both are true over a stable dataset and neither said so. That is the failure mode I keep hitting from the other side — being precise in the artifact I am editing while an unqualified claim sits where the next maintainer will actually read it. The type's doc comment now states the limit and says whose problem it is. * docs(web): record why the structured kinds inherit the timeline's SSE staleness (BUG-2301) Codex raised the live-refresh gap twice and it was declined twice, which is itself the signal that the reasoning belonged in the code rather than in a review thread. The exclusion's comment now says what the two structured kinds inherit from it and why admitting item_updated would be a bad trade. * docs(server): name the cursor sentinel's UUID assumption at the sentinel (BUG-2301) Lead's pre-merge ask, and the existing text was worse than merely silent: case 3 stated that the "g" sentinel keeps same-second entries, full stop. That is true only for ids from the lowercase-hex UUID alphabet. Anything sorting above "g" is dropped at the cursor instant instead, and a source whose ids straddle it is split in half on their first character — which is exactly what happened to `note-…` and `decision-…` here. So the assumption is now named where someone adding a non-UUID id will read it, rather than only in the helper that already works around it. An unqualified claim at the point of use is the failure mode I keep meeting from both sides; this is the same fix as qualifying the paging comment two commits ago. Comments only — no behaviour change.	2026-08-17 12:58:09 -04:00
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	2026-08-17 09:03:44 -04:00
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	2026-08-17 08:16:02 -04:00
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	2026-08-17 05:19:25 -04:00
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)	2026-08-17 04:21:32 -04:00
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)	2026-08-17 03:42:08 -04:00
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).	2026-08-17 01:59:02 -04:00
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	2026-08-17 01:33:18 -04:00
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.	2026-08-16 18:25:23 -04:00
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	2026-08-16 12:33:12 -04:00
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	2026-08-15 20:57:50 -04:00
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.	2026-08-15 19:18:55 -04:00
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.	2026-08-15 14:52:25 -04:00
xarmian	79b3220c61	test(server): drive the reval-fault test off the fault seam instead of a sleep (BUG-2570) (#1102) ... * test(server): drive the reval-fault test off the fault seam instead of a sleep (BUG-2570) The reload-fault closure now narrows the member's access on faulting tick 1 and lifts the fault on faulting tick 2, so consecutive reload failures stop at exactly 2 — strictly below the clear-the-watch-set bound — and the green path carries no timing bet at any load. The 300ms sleep is gone; readiness is signaled by the tick sequence itself. Codex round 1 on this fix surfaced that regression DETECTION still has a window (a successful tick 3 masks a hypothetical reset-skipped-on- fault regression), so the interval is set to 500ms to give the revoked PATCH ~10x headroom over measured loaded-runner request latency, and the control-leg wait — the one that timed out in both CI instances — is widened to 10s since it asserts delivery-at-all, not latency. Verified: 5x -race green at both 50ms and 500ms; counterfactual mutant (reset moved to the reload success path) leaks 3/3; full suite + lint green; Postgres leg 2x -race green. Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt * test(server): drive reval ticks through a seam — deterministic in both directions (BUG-2570) Codex rounds on the first fix found two regression-DETECTION windows the interval-tuned shape could not close: a stray successful tick before fault installation or after the tick-2 lift resets visCache / reloads the watch list, masking the reset-skipped-on-fault regression this test exists to catch. Interval tuning trades green-determinism against detection-determinism; a free-running ticker cannot give both. So the handler gains watchRevalTickOverride — a test seam mirroring watchPredicatesLoadFault (atomic pointer, read once at stream setup) that lets a test substitute the reval tick source. The test now drives exactly ONE tick, after the access change, with the reload fault active: no early tick can mask via a pre-fault reset, no late tick can mask via a post-lift reload, and one faulting tick can never reach the clear-the-watch-set bound. No sleeps, no interval mutation, no wall- clock bets in either direction. Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt	2026-08-15 09:33:29 -04:00
xarmian	e03ba45b5c	feat(web): push-to-agent composer in the item view (TASK-2561) (#1099) ... * feat(web): push-to-agent composer in the item view (TASK-2561) PLAN-2558 S3 — the web half of IDEA-2544's push-to-harness. Adds `api.items.push`, a new `api.sessions.list`, and a "Push to agent…" row in the item pane's ⋯ menu that opens a small composer. The deliverable is the presence line, not the textarea. `pad push` is fire-and-forget — no durable inbox, no ack, no "nobody was listening" warning — which is defensible for a CLI verb typed by someone who knows their own session is running, and indefensible for a button. So the dialog answers "is anything listening?" before the click, and keeps three states apart rather than two: N > 0 send, worded "N session connected", never "will be delivered" — the registry can name a session that died up to ~30s ago and no push gets a receipt N == 0 send DISABLED. Nothing listening means the message is lost, not queued; the empty state offers the clipboard instead (the fallback S4 rules for quick actions) can't tell send ENABLED, uncertainty stated. A 503/401/network failure is not zero — rendering it as zero is the exact lie handleListSessions returns 503 rather than an empty list to avoid The menu row is gated on a resolved user, not on canEdit: push is self-addressed, so a viewer pushing an item into their own session is a read. Without a user the endpoint 401s. $lib/push/message mirrors the server's rune-after-collapse accounting so an over-length message is caught in the composer instead of coming back as a 400. It deliberately does not use JS `\s`: Go's unicode.IsSpace and `\s` disagree in both directions (U+0085 is whitespace to Go only, U+FEFF to JS only), so a `\s` client under-counts a pasted BOM and over-counts a pasted NEL. The agreement is pinned by a shared fixture (internal/server/testdata/push_message_cases.json) read by BOTH internal/server/push_message_collapse_test.go and the web unit test — a TS-only table would assert a belief about Go rather than Go's behaviour. Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd * fix(web): close the push composer's races and ambiguity gaps (codex review) Round-1 review findings on the S3 composer, all real: - ItemDetail did not reset `pushDialogOpen` on an item switch. The dialog is {#key itemSlug}-remounted while `open` is owned by the parent, so a stale `true` silently REOPENED the composer pointed at the new item. The reset block's existing comment (written for copyDialogOpen) describes this exact failure. Verified live, with the counterfactual: reverting the one-line fix reopens the dialog on item B after a client-side navigation. (The typed draft does NOT carry over — the {#key} remount clears it — so the defect is the silent reopen, not a retargeted message.) - Presence polls shared one generation counter, which fences OPENINGS, not requests. A stalled poll could resolve after a later one and overwrite a fresh count with a stale one, re-arming Push against a session list already known to be empty. Added a per-request sequence; only a strictly newer response is applied. - Nothing bounded a `/sessions` read, and 'checking' disables Push, so a request that never settled stranded the composer with a dead button and no explanation. It now degrades to the honest "can't tell" state after 5s; a later response still lands and upgrades the answer. - A failed send re-armed Push unconditionally. The handler publishes BEFORE writing its response, so an unstructured failure (rejected fetch, non-JSON 502) leaves the outcome genuinely unknown and a second click can deliver the instruction twice on an endpoint with no idempotency key. Split on the same line CopyItemDialog draws (DR-13): a structured PadApiError means the server refused before publishing — re-arm; anything else latches an outcome-unknown state. - `willCollapse` compared against `String.trim()`, reintroducing the very JS-vs-Go whitespace mismatch $lib/push/message exists to avoid (JS trims a leading U+FEFF the server keeps; it leaves a U+0085 the server strips). Added `trimPushMessage`, which trims with Go's class. - The textarea described only the counter, so the collapse note and the over-length error reached no screen reader. Both now live in one stable referenced node that swaps text rather than mounting and unmounting — an aria-describedby pointing at an absent id resolves to nothing. - Positive presence wording implied the count was current. It now says "as of the last check" and names the ~30s window. Test changes: the Go fixture test duplicated `strings.Fields` rather than invoking the handler, so a change to the handler's normalization would have left BOTH suites green — demonstrated by mutating the join separator, which the copied-expression test did not notice and the new handler-driven test caught on 22 cases. The bound is likewise now asserted through the endpoint at 4096/4097 instead of comparing the constant to a copy of itself. Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd * fix(web): fence the push composer against destroyed instances, unrecognised errors, and a frozen count (codex round 2) Three findings, one of them introduced by round 1's own fix: - The send/copy continuation fence used the generation counter, which cannot see a keyed REMOUNT. `{#key itemSlug}` gives item B a fresh instance with its own counter, so item A's in-flight send still saw its own `gen` unchanged and called the SHARED parent `onclose` — closing the composer the user had just opened for B. Added a per-instance `destroyed` flag, which is what actually distinguishes "still mine to close" from "I no longer exist". - The outcome-unknown split treated any PadApiError as proof the server refused before publishing. It isn't: the API client turns EVERY JSON error envelope into one, including a gateway 5xx invented after the handler published. Replaced with a whitelist of codes the handler and its middleware actually emit pre-publish; everything unrecognised is now ambiguous. The asymmetry is deliberate — an unnecessary "we can't tell" costs the user a check, a wrong re-arm delivers twice. - PRESENCE_STALL_MS only rescued the FIRST read. A later poll that hung froze the count at its last value indefinitely while the UI kept rendering "1 session connected" as fact. A known answer now expires to "can't tell" after 30s without a refresh — the server's own presence staleness bound, so past it our answer carries no more authority. Also dropped the status→alert role swap on the composer's live region: changing a live region's role and its text together is not reliably honoured, so the escalation was a promise the markup couldn't keep. The blocking condition rides `aria-invalid` on the textarea instead. The "latest ARRIVED, not latest ISSUED" behaviour of the sequence fence is kept and now documented as a choice: dropping an early-arriving response because a newer request exists strands the UI when that newer request is the one that never settles. Each fix mutation-tested 1:1 against its new test. Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd * fix(web): complete the pre-publish whitelist and retire in-flight polls on expiry (codex round 3) Two of round 3's three findings were real: - `csrf_error` and `email_not_verified` are middleware refusals, written strictly before the handler runs, so they belong in PRE_PUBLISH_ERROR_CODES. Without them a CSRF mismatch told the user we couldn't tell whether their message was sent, when nothing had been. - The 30s staleness expiry didn't fence requests already in flight. A poll issued before the expiry could land after it and reinstate the very count we had just declared too old to trust. Expiry now advances `presenceAppliedSeq` to the current `presenceSeq`, retiring those responses; the poll issued in the same tick carries a newer seq and still applies. The third finding — that `archived` belongs in the whitelist, and that the launcher should be hidden for archived items because "the endpoint always rejects them" — is REFUTED. handlePushToItem has no archived gate (`requireItemVisible` admits archived items), and pushing to an archived item against a running server returns 200 with `pushed: true`. There is no `archived` error code on this path to whitelist, and hiding the launcher would remove a capability that works. Recorded rather than silently skipped so the next reader doesn't re-derive it. Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd	2026-08-15 08:22:09 -04:00
xarmian	c84cf7437c	feat(sessions): announce session identity on the event stream (PLAN-2558 S2, TASK-2560) (#1094) ... * feat(sessions): announce session identity on the event stream (TASK-2560) PLAN-2558 S2. S1 gave the presence registry a count of anonymous uuids; this makes each row nameable, which is what S3 needs for an honest empty state and S5 needs for a target picker. A monitor now announces itself when it opens the stream: X-Pad-Session-Label (the working directory's basename) and X-Pad-Session-Pid. The server sanitizes both and stores them on the LiveSession; GET /api/v1/sessions returns them. TRANSPORT. The task body sketched "the stream connect carries it" without picking a mechanism and explicitly left the call open. Headers, because a query param would put the label and pid into every access-log line (this server logs path= for each request) and any proxy log in front of it — which is the same "don't let local detail travel further than it needs to" the privacy line below is about — and a separate registration POST would need its own correlation to the connection it describes, plus a matching lifecycle, when the registry entry already lives and dies with the stream. Headers ride the request that exists and sit alongside Last-Event-ID, already doing this job on this endpoint. Cost, written into the code rather than discovered later: a browser EventSource cannot set headers, so a future web-tab consumer needs a deliberate query-param fallback or a fetch-based SSE reader. PRIVACY. The basename crosses, never the full cwd — "/home/dave/Dev/ docapp" additionally hands over a home directory and usually an account name for no gain — and messaging_socket_path never leaves the machine. Pinned by a test rather than by the implementation being one line. WHAT THIS DELIBERATELY DOESN'T DO: read ~/.pad/sessions/. The task framed S2 as giving `pad session register` its first consumer, and the monitor cannot honestly be one. Registry entries are written by whatever process ran that command — a different pid — and the only matchable fields are pid and cwd, so two agent sessions in one checkout are indistinguishable and "pick the newest" is a coin flip that would put a confident wrong name in the S5 picker. Process ancestry settles it exactly and is platform-specific (this binary ships for macOS and Windows). The monitor's own cwd basename and pid are never wrong and answer the question the label exists to answer; correlating a stream to the agent session that spawned it needs an identifier the harness passes down, which is worth doing when something needs it and worth not faking until then. Also moves S1's STALENESS doc block, which sat above LiveSession.Label where it read as documenting the name rather than the whole entry. Tests: sanitizer units (whitespace collapse, control-char stripping, rune-not-byte truncation), header wiring, the end-to-end labelled session, the unannounced-client compatibility leg (a pre-S2 monitor must still register and still stream), a hostile-input leg over the wire, the client's omit-when-unset behaviour, and the basename promise. Measured rather than assumed: Go's server answers 400 to a header value containing a control byte before any handler runs (verified with a raw socket, since Go's own client refuses to send one and the two refusals are indistinguishable from a normal client test). So that arm of the sanitizer is unreachable over HTTP; it stays as defence in depth for the next caller in, and both the comment and the wire test say so instead of the test quietly passing because the transport refused the input. Mutation-tested four ways, each revert grep-verified: handler ignoring the parsed identity, monitor sending the full cwd, dropping the truncation, and the client always setting the headers. Refs TASK-2560, PLAN-2558 * fix(cli): sanitize the session label client-side per Codex review (round 1) Codex round 1's only finding, and it is a bigger deal than a missing label. Unix directory names may contain control bytes — "doc\napp" is a legal directory — and Go's http.Client REFUSES to send a request whose header value holds one: Do returns "invalid header field value" and nothing is transmitted. In the monitor that is indistinguishable from an unreachable padd, so the retry loop backs off and tries again, forever, printing nothing by contract. A user who named a directory that way would simply stop receiving notifications, with no signal anywhere. The server cannot defend against a request that never arrives. Reproduced before fixing, with a real directory and a real client, rather than reasoned about from the error message. Sanitizing in NewWatchEventsStreamRequest rather than in monitorSessionIdentity: the invariant is "this function never builds an unsendable request", which belongs at the point where a value becomes a header, not at one caller. The client's cap (256 runes) is deliberately looser than and independent of the server's (64): the server decides what a label should look like, the client only has to keep the request sane, and neither has to track the other to stay correct. The regression test does the ROUND TRIP instead of inspecting the header, because the header contents were never the bug — http.Header.Set stores anything, so an assertion on the value passes against the broken version too. Only attempting the request tells the two apart. Mutation-verified: reverting the sanitizer fails the test with exactly the "invalid header field value" error from the field report.	2026-08-14 19:03:33 -04:00
xarmian	599fdbd3f4	feat(watch): drop assignment from the addressed-to-you stream (IDEA-2544 Phase 2, TASK-2551) (#1092) ... * feat(watch): drop assignment from the addressed-to-you stream (TASK-2551) IDEA-2544 Phase 2. Assignment is bookkeeping (who owns this); push is dispatch (where attention goes now). Conflating them meant one triage session assigning N items sprayed N notifications into every open session of the assignee, so Dave's product call (day-33) was to drop assignment from addressed-to-you entirely — no opt-in flag, no config key. watchNotificationVisible loses its KindAssignment early-return; an assignment notification now falls through to the watch-map check like any other item-level fact, which is what an unconditional watch already promises to deliver. Producers are untouched and AssignedUserID is still populated, so a future opt-in re-addressing would be a consumer-side change only. KindPush is now the only addressed kind. Tests: six tests rode the deleted path and are reworked, not deleted. The two mid-stream visibility tests needed new vehicles — the persistent-reload-failure test uses a push (same watch-map-independent property), and the reval-ordering test uses collection-access revocation with a still-granted control item, since push is self-addressed only and its subject is a user losing access. That test's reval interval goes 50ms -> 200ms: at 50ms the clear-the-watch-set bound landed ~30ms behind the assertion and the control leg lost the race. New coverage for the asymmetry the change creates: a push stays exclusive of watch-matched delivery, an assignment does not — a watcher is entitled to see who an item was assigned to. Mutation-tested three ways (restore the old branch; make assignment exclusive addressed-only; couple visCache.reset() to reload success); each is caught by the intended test and each revert was grep-verified. Live: assigning a fresh unwatched item to the connected user leaves the plugin monitor silent, pushing the same item prints one line, and assigning a WATCHED item still delivers — verified end to end against a sandboxed server, not just in tests. Refs TASK-2551, IDEA-2544 * docs(watch): note the deferred plugin wording per Codex review (round 1) Codex's only finding: plugin/monitors/monitors.json and plugin/skills/pad/SKILL.md still describe assignment as addressed-to-you traffic. Correct observation, deliberately out of scope — installed plugins are version-pinned at install, so plugin-visible text reaches nobody without a version bump, and TASK-2564 (PLAN-2558 S6) owns the wording and the bump together. Recording it in code next to the deleted branch rather than leaving a reader to discover the mismatch, and on TASK-2564 with the exact line refs so the follow-up does not have to re-find them.	2026-08-14 17:16:10 -04:00
xarmian	21001bc4c3	feat(sessions): session-presence registry + GET /api/v1/sessions (PLAN-2558 S1) (#1091) ... * feat(sessions): session-presence registry + GET /api/v1/sessions (PLAN-2558 S1) Slice 1 of PLAN-2558 (IDEA-2544 Phase 3, web-UI push). The server can now answer "is anything actually listening right now?" for the calling user. WHY. `pad push` (Phase 1, da6ce642) is fire-and-forget with no "no session connected" warning. That's a defensible contract for a CLI verb typed by someone who knows whether their own session is running. It is not a defensible contract for a web-UI button: "Push to Claude" that silently goes nowhere is worse than the clipboard ferry it replaces, because the user cannot tell the two outcomes apart. Presence lets the UI answer the question before the click, and — once sessions carry a label (S2) — turns the same data into the target picker S5 needs. This also closes the substrate half of PLAN-2469 Phase 3 ("presence surface: SessionStart hook -> live-sessions view", IDEA-2464). The two Phase 3s were the same work; see PLAN-2558's opening section. - internal/server/session_presence.go: SessionPresence interface + MemorySessionPresence. Registered from handleWatchEventsStream, bracketed to the SUBSCRIPTION's lifetime (defer pairs with Unsubscribe's on the adjacent line) so every exit path — ctx.Done, a failed SSE write, the replay-loop returns, the reval-tick paths — releases both or neither. A leaked entry is the failure that matters: it makes the UI promise a listener that is gone, i.e. the same silent nowhere-push with a confident label on it. - internal/server/handlers_sessions.go: GET /api/v1/sessions, self-scoped. No ?user_id=, no admin bypass — who has an agent session open is a presence signal about a person, and the same reasoning that made push self-addressed only applies. 503 (not 200-with-empty-list) when no registry is wired: "I can't tell" and "nobody is listening" must not look the same to the UI, since collapsing them is exactly the dishonesty this slice exists to remove. Interface from day one because MemorySessionPresence is per-process. Its doc comment states the boundary precisely rather than hand-waving it: watchevents.Bus is blind in the SAME direction (a push published on instance A never reaches a stream on instance B), so per-instance presence is as accurate as per-instance delivery and both stop being trustworthy at the same boundary — except that a load balancer may route a POST and a GET to different instances, at which point they disagree. A Redis-backed presence must therefore land WITH the Redis watchevents.Bus that package already anticipates, not separately. No pad-cloud change required (checked, not assumed): /api/v1/sessions is a plain JSON GET served by nginx-router.conf's default `location /` pass-through — the special long-lived-connection blocks are for /api/v1/events and /api/v1/collab/ only. Verified: go test ./... (SQLite) clean; make test-pg clean (25 pkgs, exit 0); make lint 0 issues; new tests pass under -race. Live, on the installed binary: 0 sessions with nothing connected -> 1 with one stream open -> 2 with two, oldest-first -> back to 0 after both disconnect, with a second user's list staying empty throughout. * fix(sessions): no-store the presence response; document two lifetime constraints (PLAN-2558 S1) Codex round 2 findings, both verified against source before acting. P2 — Cache-Control. GET /api/v1/sessions set no cache header: writeJSON sets none and the jsonContentType middleware only sets Content-Type, so the response was heuristically cacheable. Now `private, no-store`, matching the house pattern for per-user sensitive responses (handlers_attachments.go:585). Wrong two ways without it: a shared cache could serve one user's presence to another (the same boundary this endpoint's absent admin view exists to hold), and a cached liveness answer is exactly the confident-but-wrong "1 session connected" the slice exists to prevent. Pinned by a test. P2 — Shutdown, REFINED rather than adopted as reported. Server.Shutdown delegates to http.Server.Shutdown, which does not cancel an in-flight handler's context; SSE handlers therefore hang until their own ctx.Done or a failed write. True, but for MemorySessionPresence it is HARMLESS, and that is the useful half: the registry lives in the process that is going away, so its entries die with it. There is nothing to reap. A Redis-backed implementation does not inherit that — its entries outlive the writing process, so a crash strands them permanently rather than for 30 seconds. Recorded as a hard constraint on the interface: any out-of-process implementation must carry its own reaping story (TTL plus heartbeat renewal, or instance-keyed ownership swept at startup). Also documents the staleness window neither codex round surfaced, found in my own pass: a clean disconnect deregisters immediately, an ungraceful one is invisible until the next keepalive write fails, and the keepalive is 30s. So the list can name a dead listener for up to ~30 seconds. That bound is fine for a fire-and-forget channel — a push to a session that died 5 seconds ago loses a message that was lost anyway — but consumers must not upgrade it into a delivery guarantee. Shortening it means shortening the keepalive, which taxes every idle connection; the right answer for a consumer that needs delivery confidence is an ack, not a faster heartbeat. Verified: go build, go vet, make lint 0 issues, presence tests green under -race.	2026-08-14 17:16:07 -04:00
xarmian	da6ce642da	feat(push): pad push — user-authored instruction dispatch to agent sessions (IDEA-2544 Phase 1) (#1090) ... * feat(push): add pad push <ref> -m vertical (IDEA-2544 Phase 1) Self-addressed, human-to-harness dispatch over the existing watch-events bus/stream: CLI -> POST .../items/{itemSlug}/push -> a new KindPush Notification (carrying the generalized TargetUserID addressed-to field KindAsk will later share) -> watchNotificationVisible delivers it back to the pushing user's own connected monitor sessions. Transient, fire-and-forget by design (no migration, no durable inbox) since assignment already covers the durable-notification case and this is meant to be the explicit, no-inference dispatch verb instead. * docs(plugin): document the push notification contract (IDEA-2544 Phase 1) Push is the one notification kind that IS an instruction rather than a passive fact, so it gets its own lead bullet in the plugin skill's notification-etiquette section (ahead of the read-only/park default, which it explicitly lifts) and a mention in the monitor's description. The embed-source skills/pad/SKILL.md has no notification section to mirror this into (the two files diverge by design) and is left untouched. * fix(push): reject over-long push messages instead of unbounded Summary Comments truncate their notification Summary to a preview (the full body is still fetchable), but a push message IS the payload — silently truncating it would corrupt the instruction with nothing to recover it from. Add maxPushMessageLen (4096, measured post-collapse) and reject anything over it with a 400 rather than truncating; state the same bound in `pad push --help` so it's discoverable before a 400, not only from one. * fix(push): close the watch-fallthrough leak, disambiguate SKILL.md exceptions Codex round 1 P1: watchNotificationVisible's push branch only returned early on a MATCH — a non-target caller fell through to the watch-map check below it, so anyone holding an unconditional (or predicated) watch on the item received every push addressed to every OTHER user, instruction text included. Push is addressed private dispatch, not an item-level fact watchers have a legitimate claim on (unlike assignment, which watchers are expected to see per `pad watch --help`) — the branch now returns unconditionally for KindPush, gating strictly on TargetUserID and never reaching the watch-map fallback either way. Pinned explicitly since Phase 4's session targeting is expected to inherit this same exclusivity. Also (codex P2): reworded the SKILL.md notification-etiquette bullets — the new push exception and the pre-existing assignment/ask exception literally contradicted each other ("the ONE narrow exception" claimed singularity after push had already claimed exception status). Now explicitly enumerated as the first and second exceptions to the never-write rule. * test(cli): pin that PushItem inherits X-Pad-Agent (BUG-2542 rebase) Verified, not assumed: PushItem builds its request via c.post -> c.newRequest like every other mutating client method (CreateWatch included), so the attribution fix's client.agentName wiring covers it for free with zero code changes needed on this branch. Adds a live httptest assertion rather than trusting the code-path read alone — the same shape as TestClientSendsResolvedAgentHeader, scoped to PushItem specifically since that's the one method this PR added. * fix(push): disambiguate workspace in the monitor line and skill contract Codex round 2 P1: the watch-events stream is user-scoped ACROSS every workspace a caller has watches in, but formatMonitorLine printed only ItemRef/Kind/Actor/Summary and dropped the Workspace field the wire payload already carried — a session linked to workspace A receiving a notification for workspace B would resolve the wrong item (or 404) with no signal in the line that anything was off. Fixed universally, not push-only: grepped plugin/ and skills/ for anything parsing "PAD ..." lines and found none — the Claude Code plugin host ingests the stdout line as free-text notification prose, formatMonitorLine's only real consumer is its own fmt.Println, so there is no wire-format consumer a workspace prefix could break. The ambiguity predates push (any watched item across workspaces already had it); push just makes the consequence sharper because it carries an instruction rather than a passive fact. SKILL.md's push bullet now tells the agent to resolve with `pad --workspace <workspace> item show <ref>` using the slug read off the notification line, not a bare `pad item show <ref>`. * fix(push): respect --format json instead of hardcoding plain text Codex round 2 P2: pushCmd's RunE ignored the global format flag and always printed "Pushed <ref>", silently discarding --format json. - server.pushResponse replaces the bare map the handler wrote before — a typed {ref, workspace, pushed, message} shape, with workspace resolved to the CANONICAL slug via s.getWorkspace (not merely echoed from whatever the URL contained), matching the same disambiguation need the round-2 P1 fix addressed for the monitor line. - cli.PushItem now returns (*PushResult, error) instead of discarding the response body. - pushCmd checks formatFlag == "json" and calls cli.PrintJSON, mirroring runCreateWatch's existing pattern. internal/cli/agent_identity_test.go's TestPushItemSendsResolvedAgentHeader needed a one-line update for PushItem's new two-value return — caught by `go vet ./...`, not `go build ./...` (which doesn't compile test files); folding vet into my own pre-flight going forward.	2026-08-13 18:41:46 -04:00
xarmian	212d59e7c6	fix(cli,server): make agent attribution actually happen (BUG-2542) (#1088) ... * fix(cli,server): make agent attribution actually happen (BUG-2542) Agent CLI writes were recorded as the human whose credentials they used. Three independent defects, each verified by reading the path AND by probing a live instance — the item deliberately held the mechanism open, so none of this is inherited. 1. THE HEADER WAS NEVER SENT. actorFromRequest sets actor="agent" on one signal: the X-Pad-Agent header. The only code that sets it took the value from `agent_name` in .pad.toml and nowhere else — no environment detection, no session detection. This repo's .pad.toml has only `workspace`, so the header has never been sent from here and every agent write has looked human. ResolveAgentName now resolves .pad.toml → $PAD_AGENT → detected runtime. 2. ITEM CREATE DISCARDED THE ACTOR. createItemChecked called actorFromRequest and kept only the source (`_, src :=`), never setting input.CreatedBy, so store.CreateItem fell through to its "user" default — even for an agent that DID send the header. Comments have always stamped it correctly; item creation silently did not, which made the skill's own contract false on its own terms. 3. SINGLE-ITEM PATCH NEVER STAMPED LastModifiedBy. Bulk ops do (handlers_items_bulk.go); the single-item path did not, so an item edited only by agents read as human-edited. Only entries VERIFIED against a live session belong in the runtime detection table, so it has exactly one: Claude Code exports CLAUDECODE=1 to child processes, confirmed by reading a pad subprocess's environment inside one. Guessing at Cursor/Windsurf/Aider variable names would put unverified claims in a shipped binary and misattribute silently when wrong; those set $PAD_AGENT until someone confirms a signature. WHAT THIS DOES NOT DO, stated in the code and the skill rather than left for someone to assume: the header is client-supplied and self-declared. An agent that omits it is indistinguishable from the human it borrows credentials from, and a human running `! pad ...` inside an agent's terminal inherits that environment and is attributed to the agent. This makes the trail HONEST, not VERIFIED — it is not a basis for machine-verifiable human-approval provenance, which needs a channel the agent cannot author at all. The incident behind this item is exactly that distinction: an agent's relay of a human's words was recorded indistinguishably from the human typing them. Contract corrected in both skill copies, since the item's first question was which of contract and behavior was wrong. It was the contract: it promised automatic agent attribution that only ever applied to workspaces that had opted in. Tests, each mutation-tested against its own defect reverted alone: - TestResolveAgentName — precedence plus the negative that makes it mean something: a plain human shell must still resolve to "". Fails 2/5 reverted. - TestItemAttribution_AgentVsHuman — agent and human legs for create, update and the create-stamp-survives-edit invariant. Fails on the create stamp reverted; fails 2/2 on the update stamp reverted. The update leg deliberately uses the OTHER writer: insertItemTx seeds last_modified_by FROM created_by, so a same-writer edit passes whether or not the PATCH stamps anything — the first version of this test did exactly that and passed its own counterfactual. Caught only because each fix was reverted separately. - TestItemAttribution_ExplicitBodyValueWins — an explicit body value still beats the header. End-to-end on a live instance through the real CLI, no .pad.toml opt-in: agent session → created_by/last_modified_by/comment all `agent`; same binary with CLAUDECODE stripped → all `user`. Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag * fix(server): artifact import wrote a UUID into created_by (BUG-2542) Found by Codex while reviewing the attribution fix. handleImportArtifact set `input.CreatedBy = u.ID`, which is the wrong DOMAIN for the field rather than merely the wrong value: created_by holds the role — "user" or "agent" — and consumers compare it against those literals (CommentThread.svelte, TimelineVersionCard.svelte). An imported item matched neither and rendered as neither. It also would have defeated the fix in the parent commit at this path: a non-empty CreatedBy suppresses the actor stamp, so imports would have kept a UUID while every other create path started recording the actor. The line contradicted the comment directly above it, which said Source was being left blank precisely so createItemChecked could stamp it "like every other create path". Now both fields are left blank and stamped together. The user's identity has its own home — the items.created_by_user_id column — which no create path currently populates. That is a separate gap and is not widened into this change. Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag * fix: close the remaining attribution bypasses Codex found (BUG-2542) Review found no P1s and three P2 families beyond the artifact-import bug already fixed in 6fac5dec. Two are closed here; two are deliberately not, and the reasons matter more than the diff. CLOSED — paths that asserted "user" and so SUPPRESSED the new stamp, which made them worse after the parent commit rather than merely stale: - cmd/pad/notes.go sent CreatedBy/LastModifiedBy = "user" from the CLIENT on all four note/decision writes. An explicit body value beats the header by design, so every agent note claimed a human wrote it, and would have kept claiming it. The client shouldn't assert an attribution it cannot know; all four now leave it to the server. - handlers_item_versions.go hardcoded LastModifiedBy "user" / Source "web" on restore, so an agent-driven restore recorded itself as a human web edit. Now stamped from the request. Also closed Codex's nit that the tests injected X-Pad-Agent directly and never proved the resolver reaches the wire — TestClientSendsResolvedAgentHeader runs the real client against an httptest server and asserts the header, with a human-shell leg asserting its ABSENCE. Fails when the client wiring is reverted. And the Source assertion now pins "web" rather than merely non-empty. NOT CLOSED, on purpose: - Collab flush. An agent PATCH stamps `agent`, then the browser's later ?source=collab-snapshot PATCH stamps `user`. Codex reads that as lost attribution; I'm not convinced it's wrong — the browser really is the writer of that flush, and the agent's edit is already recorded on the PATCH that carried it. Deciding whose name belongs on a human-flushed doc containing agent edits is a semantics call about what last_modified_by MEANS, not a bug I should settle inside a fix commit. Filed rather than guessed. - Move paths don't touch last_modified_by at all. That predates this change and is the same question (is a move an edit?), so it goes with the above. Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag * fix(cli): note/decision entries self-declare instead of going authorless Self-caught regression from the previous commit, found by checking the thing I changed rather than assuming it behaved like its neighbours. I removed the CLI's hardcoded CreatedBy: "user" from note and decision entries on the reasoning that applies to every OTHER write in that file: an explicit value suppresses the server's stamp, so the client should stay quiet and let the request context decide. That reasoning does not reach these two. The entries live INSIDE the item's fields JSON, which the server stores as an opaque blob and never parses for attribution — so nothing downstream fills the gap, and blanking it would have written authorless notes. Worse than the bug I was fixing: "user" was at least right half the time. They now carry cli.ActorKind() — the same self-declared signal as the header, reduced to the user/agent enum the field holds. Its doc says plainly that this is the ONE place a client should assert attribution, and why, so the next person doesn't generalise it back the wrong way. The item-level LastModifiedBy in the same functions stays server-stamped; that half of the previous commit was right. TestActorKind covers agent, explicit PAD_AGENT, and human-shell legs. Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag * fix(server): stamp the actor on non-parent item links (BUG-2542) Last P2 from the review. Parent links pass the actor to SetParentLink; every other link type (blocks / blocked-by / relates / implements) goes through CreateItemLink, which the CLI calls without created_by, so the store defaulted it to "user" and an agent's `pad item block` recorded a human. Same one-line shape as the create path, explicit body value still wins. TestItemLinkAttribution_AgentVsHuman covers both writers; fails on the agent leg when the stamp is reverted, control passes either way. That closes every actor-dropping path the review found except the two filed as IDEA-2549 (collab flush, move), which are semantics questions about what last_modified_by means rather than defects — Codex agrees the deferral holds if the field means content author, and flags that they become real follow-ups if we decide it means sender-of-write. Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag * test(server): table-drive every non-parent link type (BUG-2542) Codex nit: the link regression only covered `blocks`, and "shared routing makes the other types fine" was doing the work. It cost nothing to stop assuming, and the table earned itself on the first run — my initial list included `blocked-by`, which is CLI surface sugar that inverts source/target into a `blocks` row rather than a stored link type. The API rejects it with a 400, on BOTH writer legs, which is also how that failure reads differently from an attribution one. Now covers blocks / related / implements / supersedes / split_from against both writers. One precision fix owed on 06938079's message: it says "THE HEADER WAS NEVER SENT". Not true in general — a workspace with agent_name in .pad.toml did send it, which is exactly how I probed the behaviour before fixing it. Accurate version: the header was absent for anything that had not opted in, which is every workspace I can see, including this repo's. The body of that commit says it correctly; the headline overstates. Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag * style: gofmt notes.go after the attribution edit (BUG-2542) Removing the hardcoded LastModifiedBy from the two ItemUpdate literals left the surviving fields aligned to a column that no longer had a member, so gofmt disagreed and CI's golangci-lint failed the Go job in 42s. The real fault is upstream of the whitespace: my gates line for #1088 read "go test ./... green · Codex to CLEAN" and lint was simply not in it. The omission in the report and the failure in CI are the same fact — I reported a matrix that did not include the axis that broke. `make lint` runs the pinned suite CI runs and takes seconds; it belongs in every report I make, alongside test and build. Claude-Session: https://claude.ai/code/session_01QGbUKZBAZoWdEgiTNWsXag	2026-08-13 16:21:10 -04:00
xarmian	ec7fd027fc	feat(server,cli): watches, user-scoped event stream, plugin monitor command — PLAN-2469 Phase 1 (TASK-2533) (#1082) ... * feat(store): race-free status/assignment mutation signal (TASK-2533) Adds models.Item.LastMutation (ItemMutationSignal), populated inside the SAME transaction that already writes status_transitions / assigned_user_id in UpdateItemWithParentLink and MoveItemWithPreCheck. This is the foundation for TASK-2533's watch-notification pipeline: a before/after snapshot taken in the HTTP handler layer would race concurrent writers of the same item, so the signal is computed where the authoritative diff already happens, in-transaction. * feat(store): watches table migration, both drivers (TASK-2533) watches(id, workspace_id, user_id, item_id, predicate, created_at) per DOC-2479's subscription-table design: durable, server-side subscriptions that survive both the plugin-monitor process and a padd restart. uq_watches_user_item makes `pad watch <ref>` idempotent (re-watching upserts the predicate). Wires watches into the workspace-purge child-delete list, mirroring item_stars. * feat(watchevents): add in-process notification bus (TASK-2533) New package: a global (not per-workspace) in-process pub/sub bus carrying watch-worthy Notifications (status-change / assignment / comment; ask reserved in the enum with no producer yet — see the follow-up server commit). Bus is an interface specifically so a Redis-backed implementation can slot in later without touching any caller; only MemoryBus exists today. Package doc comment states the single-process/multi-instance limitation explicitly, mirroring internal/events' shape. * feat(store): watches CRUD (TASK-2533) models.Watch + Store.CreateWatch (upsert on user+item)/GetWatchByUserItem/ ListWatchesForUser (unscoped by workspace — a watch is personal, and the event-stream handler needs every watch a caller holds across all their workspaces)/DeleteWatch. * feat(server): watch/nudge event stream + CRUD endpoints (TASK-2533) GET /api/v1/events/stream (DOC-2479): a user-scoped, cross-workspace SSE stream, filtered server-side to the caller's watches (with optional --until field=value predicate) plus "addressed to you" — narrowed to assignment-to-you only for Phase 1, confirmed with the dispatcher: this codebase has neither a Collection.Kind field nor any user->active-role binding to ground DOC-2479's "human-gate-shaped collection targets your active role" half mechanically. watchevents.KindAsk stays in the wire enum with no producer. `pad session register` is the natural future hook for a session-carried role identity. POST/DELETE .../items/{slug}/watch, GET /api/v1/watches (unscoped, mirrors /auth/tokens' shape for a personal, not workspace, resource). Producer wiring (TASK-2533 audit) publishes from every live mutation path that can produce a LastMutation signal or a new comment: handleUpdateItem (incl. its collab sub-paths and the comment-attached-to-update path, which bypasses handleCreateComment entirely), handleMoveItem, handleCreateComment, item creation with an initial assignee, and the bulk-items loop (covers archive/restore/move/set-priority/tag/untag/assign uniformly via one call site). Named, not silent, bypasses: import bundle, status_transitions backfill, workspace restore/purge — none are live human-facing mutations. Known Phase-1 tradeoff, flagged not fixed: bulk mutations are NOT batched into one notification the way the existing SSE/webhook bulk path is — a bulk-assign of N items surfaces N individual notifications. Each is still correctly scoped by the recipient's own watches/addressed-to-you filter (a narrower audience than the workspace-wide SSE firehose the existing batching protects), so this is a noise-discipline tradeoff, not a leak. * feat(cli): pad watch + pad session register (TASK-2533) pad watch <ref> [--until field=value] creates/upserts a durable watch; pad watch list / pad watch remove <ref> are the hygiene companions the dispatcher asked to be included explicitly rather than silently added. pad watch --stream --for-session is the plugin-monitor command: one stdout line per matching event ("PAD TASK-214 -> kind (actor): summary"), silent on startup with no .pad.toml (hourly retry) or an unreachable padd (backoff retry) per DOC-2479's noise-discipline contract. The retry/ backoff math and line formatting are pure, unit-tested functions; the actual sleep loop is not (per the dispatcher's ask). pad session register writes ~/.pad/sessions/<pid>.json (pid, cwd, CLAUDE_CODE_MESSAGING_SOCKET when set) -- forward-looking infra for Phase 3's live-sessions/presence surface; nothing consumes it yet in Phase 1/2. * fix(server): comment replies never published a watch notification (TASK-2533) Codex round 1 finding 2 (verified real, not a false positive): handleCreateReply is a SEPARATE code path from handleCreateComment — it calls store.CreateComment directly via POST .../comments/{id}/replies, not POST .../comments — and was missing the watch-notification hook entirely. A reply to a comment on a watched item produced zero notification. Same kind=comment publish as the top-level path, plus a regression test covering the reply route specifically. * fix(server): re-check current access before serving/delivering watches (TASK-2533) Codex round 1 finding 1: ListWatchesForUser filtered only by user_id — a watch row survives a revoked workspace membership or grant (nothing deletes it), so GET /api/v1/watches and the event-stream's notification filter could keep leaking item title/ref, workspace slug, actor, and summary for access the caller no longer has. Adds Store.ListWatchesForUser's ItemCollectionID column (needed for the visibility check) and server.filterWatchesByCurrentAccess, which mirrors computeSSEVisibility's RBAC resolution (handlers_events.go) — admin bypass, VisibleCollectionIDs for member/guest full-collection access, GuestVisibleResources for item-level grants — grouped by workspace since a caller's watches can span many, unlike a single SSE connection scoped to one. Fails closed on any lookup error. Wired into handleListWatches here; the event-stream's loadWatchPredicates call site picks up the same filter in the next commit, which also restructures that function's Subscribe/replay sequence and therefore touches the same lines. * fix(watchevents): atomic ID assignment + subscribe-and-replay (TASK-2533) Codex round 1, findings 3 and 4 (same subsystem, fixed together): Finding 4 — sequence assignment and replay-buffer insertion happened under SEPARATE locks in MemoryBus.Publish. Two concurrent Publish calls could append to the ring buffer out of ID order, corrupting since()'s ordering assumptions (it walks the ring oldest→newest assuming monotonic IDs). Fixed by unifying seq assignment, buffer append, and the subscriber-list snapshot under one lock; the (already non-blocking) fan-out send still happens after releasing it. Finding 3 — GET /api/v1/events/stream called Subscribe() and, later (when resuming via Last-Event-ID), EventsSince() as two separate calls. A Notification published in the window between them landed in BOTH the replay result and the live channel, double-delivering it. Bus gains SubscribeAndReplaySince(sinceID), which atomically subscribes and reads the replay buffer under the SAME lock; the stream handler now uses it whenever a Last-Event-ID is present (this commit carries that call-site change, plus the finding-1 loadWatchPredicates filter wiring from the previous commit — both land in the same lines of this function). Adds a concurrent-publish ID-ordering test and a subscribe-then- concurrent-publish no-duplicate test, both run with -race. * fix(cli): monitor silent-start ordering + sync_required handling (TASK-2533) Codex round 1, findings 5 and 6: Finding 5 (P1) — runWatchMonitor called getClient() once, before the loop and before the .pad.toml check. getClient() -> getConfiguredConfig() os.Exit(1)s when unconfigured with no TTY, or launches an INTERACTIVE configuration wizard when one is attached — either way a direct violation of DOC-2479's silent-start contract, which requires "not ready yet" to be a silent retry, never a crash or a prompt. Adds monitorClient(), which builds the client the same way but returns a plain error instead of exiting or prompting; client construction now happens INSIDE the loop, after the .pad.toml gate, on every iteration, and its failure folds into the existing padd-unreachable backoff path. Finding 6 (P2) — streamWatchEvents ignored "sync_required" (the server's signal that the requested Last-Event-ID was evicted from its replay buffer), so a stale cursor got resent on every reconnect forever. Now clears the cursor on sync_required so the next reconnect is a fresh, non-resuming subscription instead. Both covered by tests that assert the goroutine returns promptly on context cancellation (proving no os.Exit / no blocking prompt was hit, since the test process itself is still running to observe the return) and that streamWatchEvents clears/re-tracks the cursor correctly around sync_required. * fix(server): uniform current-access gate for watch AND addressed-to-you delivery (TASK-2533) Codex round 2, findings 1 and 2 — same subsystem (watch/nudge delivery access control), fixed together; finding 2 explicitly falsifies finding 1's fix's own admin-bypass argument, so this replaces that reasoning rather than patching around it. Finding 1 (confirmed real): VisibleCollectionIDs / GuestVisibleCollectionIDs deliberately over-widen for navigation — a collection ID is included if the caller has an item grant on ANY item inside it, explicitly leaving item-level narrowing to the caller (their own doc comments say so). computeWatchAccessVisibility used that over-wide set directly as the "fully visible" gate, so a guest granted item A was treated as having full access to A's WHOLE collection, including an ungranted sibling item B. Fixed by building the "genuinely full access" set from GuestVisibleResources' fullCollectionIDs (populated only from direct collection_grants, never widened by an item grant) + GetMemberCollectionAccess / ListSystemCollectionIDs for an actual member — exactly computeSSEVisibility's own fullCollSet construction, not an approximation of it. Finding 2 (confirmed real): the addressed-to-you (KindAssignment) branch in watchNotificationVisible returned true unconditionally, with NO access check. validateAssignmentScope (internal/store/items.go) only checks WORKSPACE membership, never collection access, so an item can be assigned to a "specific"-access member whose granted collections don't include it at all — an ordinary assignment, no revocation timing required. Fixed by gating EVERY notification kind — watch-matched and addressed-to-you alike — through the SAME watchAccessVisibility check before either branch runs. watchevents.Notification gains CollectionID so the check has what it needs without a second lookup; the stream handler resolves it lazily per workspace via a small connection-scoped cache (workspaces aren't known in advance for addressed-to-you the way watch workspaces are), cleared on the same reval tick that reloads the watches map. This also required replacing computeWatchAccessVisibility's admin-bypass argument, not just its code: "every call site filters the caller's OWN watches" stopped being a sufficient justification once addressed-to-you (which is fundamentally about *this* caller's own assignment activity across every workspace) shares the same gate — a bearer-borne admin token unconditionally trusted for that is exactly BUG-1616's blast radius. Now mirrors computeSSEVisibility's cookie-vs-bearer distinction exactly. Tests: guest-with-item-grant no longer sees a sibling item's watch or stream notification (filter-level and HTTP/SSE-level); an assignment outside a restricted member's granted collections is denied at both levels; addressed-to-you is proven still gated (denied with no access, visible once granted) as a pure unit test. * fix(store): always re-read existing under lock, not just for precheck/patch updates (TASK-2533) Codex round 2 finding 4, verified real: updateItemWithParentLinkOnce's `existing` snapshot was only refreshed under the write lock when precheck != nil, ExpectedUpdatedAt != "", or FieldsPatch != nil — any update touching none of those (e.g. a plain title-only PATCH) kept the STALE pre-tx `existing` for the rest of the function, including the LastMutation assignment-delta comparison added in TASK-2533's first round. A concurrent OTHER transaction's assignment change landing between this transaction's pre-tx read and its lock acquisition would get misattributed to THIS transaction: a title-only update could report a spurious, wrongly-attributed AssignmentChanged for a transition it never made, duplicating the one the other transaction already reported correctly (or missing a real one, depending on interleaving). The status-transition capture already defended against exactly this with its own separate conditional re-read; the assignment-delta capture added later did not replicate that guard. Fixed by making the re-read unconditional — once, right after the locks are held, before any SET- clause building or the UPDATE itself — so every existing.* comparison in this function is race-free by construction, not by each caller remembering to guard itself. Also removes the now-redundant duplicate re-read the status code had of its own. Reproduces the exact race deterministically using UpdateItemWithPreCheck's precheck hook as a synchronization point (TX2's assignment change blocks mid-transaction while TX1's title-only update races its own pre-tx read against it) — the new test fails reliably against the pre-fix code and passes reliably (including under -race, and in Postgres mode) against the fix. * fix(watchevents): send under the same lock Unsubscribe/Close use (TASK-2533) Codex round 2 finding 3, confirmed real and high-severity: Publish snapshotted subscriber channels under the lock, released it, and only then sent to them. A concurrent Unsubscribe or Close could close one of those channels in the window between the snapshot and the send — a send on a closed channel PANICS in Go, which crashes the whole padd process, not just one subscriber's connection. The reasoning for releasing the lock before sending ("a slow subscriber would stall everyone else") didn't hold up: the send is already non-blocking (select/default — a full channel is dropped-and-logged, never awaited), so holding the lock through it costs nothing and closes the window structurally. Adds a hammer test (many iterations of concurrent Publish / Subscribe / Unsubscribe / Close, short-lived churned channels, recover()-wrapped so a regression fails cleanly instead of crashing the whole `go test` run) that reproduces "send on closed channel" dozens of times per run against the pre-fix code (plus an independent -race detection) and passes cleanly, repeatedly, against the fix. * fix(server): re-fetch the user, not just the vis map, on each reval tick (TASK-2533) Codex round 3, confirmed real: watchVisCache captured *models.User ONCE at connect time (newWatchVisCache) and never re-fetched it; reset() cleared only the per-workspace visibility map. computeSSEVisibility's own doc comment explains why it re-fetches the user fresh on every call — "so mid-stream role changes (admin demotion, user.disabled flips) take effect on the next tick" — and the round-2 commit claimed to mirror that "exactly," but only carried over the collection/bearer logic, not the re-fetch itself. Net effect: a demoted or disabled admin kept fullAccess on an open stream (both watch-matched and addressed-to-you delivery, since both go through this same cache) until reconnect. Adds watchVisCache.refreshUser, called by both the constructor and reset() so the cadence matches computeSSEVisibility's actual cadence in handlers_events.go (that function is invoked once at connect and again only on each membershipCheck tick — never per event — so "per cache reset" here is the same cadence, not a narrower one). Deliberately fails CLOSED (not open-to-stale like computeSSEVisibility's own transient-error fallback) on a fetch error, a deleted user, or a disabled user — a nudge stream's wrong failure mode is delivering a fact to someone who shouldn't see it, not a dropped UI update, so this trades computeSSEVisibility's availability-leaning fallback for a stricter one and says so in the comment rather than repeating the "mirrors exactly" claim the fix falsified. Tests: a unit-level pair (mirroring handlers_events_revalidation_test.go's existing admin-demotion/disable coverage of the analogous SSE gap exactly) proves an admin loses fullAccess after a demotion + reset(), and a disabled user is denied outright; an HTTP/SSE-level test proves a live stream stops delivering entirely once its connected user is disabled and a reval tick passes. All three reproduce the bug reliably against the pre-fix code and pass cleanly against the fix. The HTTP-level test deliberately runs serially (not t.Parallel()): it mutates the package-level watchListRevalInterval var, which every other parallel watch-stream test in this package also reads via its own ticker — writing to it from a t.Parallel() test raced against those reads under -race (misattributed by the race detector to a whole cluster of unrelated concurrently-running tests before this was diagnosed). Full server package -race pass is clean after the fix. * fix(server): decouple vis-cache reset from watch-list reload success (TASK-2533) Codex round 4, confirmed real: on a reval tick, if ListWatchesForUser errored, the handler's `continue` skipped visCache.reset() entirely — the two were coupled, with reset() only reachable on the reload's success path. A demoted or disabled user's stale identity/visibility (round 3's fix) stayed live for exactly as long as that UNRELATED query kept failing, so the round-3 leak reopens for the duration of any watch-list reload error. Fixed by running visCache.reset() first, unconditionally, before attempting the watch-list reload. On a reload failure, the stale watch list is kept (its own staleness is already bounded by watchListRevalInterval's "eventually consistent" contract) but is now gated by the FRESH visCache regardless — a demoted/disabled user is denied via visCache even while the watch list itself lags a tick. Chose this over dropping all delivery for the tick (the other option the finding offered) because tying stream availability to an unrelated query's transient health seemed like the wrong tradeoff; the comment at the call site states this choice explicitly. Adds a watchPredicatesLoadFault test seam on *Server (mirrors the existing restoreAckFault pattern) so the reload failure can be forced deterministically without breaking the DB connection for the whole test. Reproduces the exact bug: forces the reload to fail on every tick while concurrently disabling the connected user, and asserts addressed-to-you delivery (which depends only on visCache, never the watch list) is denied anyway. Fails reliably against a reverted (pre-fix, coupled) version of the reval branch and passes cleanly against the fix. Full server package -race pass, full suite (SQLite + Postgres) pass, lint clean — this is the pre-PR verification matrix; round 5 will be a narrow re-verify of this fix only. * fix(server): bound stale watch set under persistent reload failure; atomic test seam (TASK-2533) Codex round 5, two P2s, both confirmed real: Finding 1 — `watches = fresh` only ran on the reload's success path, so under a PERSISTENT (not single-tick) reload failure the watch set stayed live indefinitely: a dead watch (removed, item deleted) kept matching forever, and a watch created during the outage was silently missed forever — visCache (round 4) gates current ACCESS, not whether a watch still legitimately exists, so it couldn't catch this on its own. Fixed by tracking consecutive reload failures and clearing the watch set once maxConsecutiveWatchReloadFailures (3 ticks) is crossed, failing closed on watch-matched delivery specifically while addressed-to-you delivery (visCache-only, unaffected either way) continues throughout. Updated the tradeoff comment at the call site so the "eventually-consistent" claim now matches the bounded, not unbounded, behavior it actually describes. Finding 2 — the watchPredicatesLoadFault test seam was a plain `func() error` field, written by a test AFTER the SSE stream's background goroutine was already running and reading it on every reval tick: genuinely racy, unlike restoreAckFault's own use of the identical field shape, which is set once, synchronously, before the single HTTP request that reads it — goroutine creation's happens-before edge makes THAT usage safe without any extra synchronization. Verified restoreAckFault does not share the flaw and left it untouched. Fixed the watch seam with atomic.Pointer[func() error] instead. Test for finding 1: forces maxConsecutiveWatchReloadFailures+1 consecutive reload failures via the (now-atomic) fault seam and asserts watch-matched delivery is suppressed once the bound is crossed while addressed-to-you keeps delivering, then clears the fault and confirms watch-matched delivery resumes on the next successful reload — a bounded outage response, not a one-way ratchet. Fails reliably against the bound disabled, passes cleanly restored. This is the (re-run) pre-PR verification matrix per the dispatcher: SQLite + Postgres + full-suite -race + lint + gofmt, all clean. Round 6 is a narrow re-verify of these two fixes only. * test(store): bound the concurrent mutation-signal test's wait (TASK-2533) CI-triage follow-up: PR #1082's plain Postgres step hit go test's default 10-minute per-binary timeout. Investigated whether any store test added by this branch scales with runner slowness (lock-wait defaults, sleep-based polling, transaction-hold durations): - Watches CRUD tests (8): 0.63-0.80s each under Postgres, isolated and in the full 741-test package run. - Mutation-signal tests (6), including the precheck-hook two-transaction race test: 0.48-0.80s each; the race test held at 0.48-0.51s across 10 consecutive runs (no variance) and across the full-package run. - Full store package under Postgres: 279.17s and 277.12s across two runs on this branch, matching the ~275s/297s baseline team-lead measured locally and on PR #1081 — no reproducible slowdown from anything this branch adds. No pathological test found locally. The one test with genuine cross-goroutine DB lock contention (TestLastMutation_AssignmentDelta_ NotMisattributedUnderConcurrentWrite) had an unbounded wg.Wait() as its only unbounded wait — TX2's release was already unconditional (fixed 50ms sleep, not gated on TX1's progress), so there's no deadlock risk, but there was no ceiling on how long legitimate lock contention could stretch it under a slow/shared runner. Replaced with a bounded 10s wait that fails fast with a diagnostic instead of silently consuming test-binary budget if it's ever exceeded. Verified the regression test still fails reliably (5/5) against a revert of the round-2 fix it guards. Could not reproduce the CI timeout locally; likely the pre-existing ~297s CI baseline (already noted as close to the 10-minute ceiling) plus environmental variance on the shared runner, not a specific test this branch adds.	2026-08-12 15:50:41 -04:00
xarmian	3bd6244001	fix(server): force-download unknown and disallowed stored MIME (BUG-2413) ... The attachment read path chose Content-Disposition from the stored MIME and DEFAULTED unknown types to inline, flipping to attachment only for the RenderForceDownload bucket. A legacy or mislabelled image/svg+xml, an extensionless SVG stored as text/xml, or an unrecognized row was therefore served inline from the app's own origin — active same-origin content, one click away once 3c-ii's converged surface gives every row a Copy-link. Fail closed. Content-Disposition now defaults to attachment; a row is served inline only when its stored MIME is on the allowlist AND in an EXPLICIT inline-safe set (MIMEEntry.ServeInline) — the passive raster/audio/video types the app embeds, plus PDF and plain text. The set is a standalone allowlist, not a function of RenderMode, so a future RenderInline entry can't silently auto-inline an active type; a new type fails safe (downloads) until explicitly listed. A MIME that isn't on the allowlist at all is additionally served as application/octet-stream so its bytes are never echoed back as a type the browser might act on. X-Content-Type-Options: nosniff was already set. The gate is at the single choke point: GET, HEAD, share-link access, and the ?variant= path all flow through handleGetAttachment (the transform endpoint only decodes images into a new raster thumbnail; the bundle/account exports never serve individual bytes inline). Regression tests cover an SVG-labelled row, a text/xml row, an unknown-MIME row (attachment + octet-stream), and the variant path forced to attachment, plus PDF and plain text staying inline — GET and HEAD. Mutation-verified: reverting to the old fail-open default fails exactly the SVG/text-xml/unknown/variant tests. Reviewed to a fresh-angle CLEAN. Claude-Session: https://claude.ai/code/session_01WFBYxdBuSZs2tjipATxAZu	2026-08-08 20:37:39 +00:00
xarmian	417929c5a4	Merge pull request #1037 from jairbj/feat/nix-flake-packaging ... feat(nix): add flake packaging with CI build	2026-08-06 01:21:28 -04:00
xarmian	b90e7edaeb	docs(attachments): record the lock-held pool I/O hazard at the call site (BUG-2409)	2026-08-02 05:22:30 +00:00
xarmian	e12feb46cb	fix(copy): authorize attachment references in cross-workspace copy (TASK-2408) ... Cross-workspace copy authorized the source item and the destination collection but never the individual attachments it cloned. PlanAttachmentCopy scoped every lookup to `workspace_id = SourceWorkspaceID AND deleted_at IS NULL` — but the workspace is not the caller, so a restricted member who could edit any item in the source workspace could paste `pad-attachment:<uuid>` for an attachment on an item they could not see, copy that item into a workspace they own, and read the bytes through the ordinary blob endpoint (BUG-2407). The planner now consults an AttachmentAuthorizer supplied by the caller, applied to every row it resolves: the referenced rows, the parents it adopts as clone roots, and the variants it follows. A denial DELETES the row from the resolution map, so it is indistinguishable from a row that was never there — the reference lands in UnresolvableRefs beside dangling, soft-deleted and foreign ids, and attachment_count / attachment_bytes / unresolvable_ref_count read identically. The preflight's numbers stay oracle-free. It is a callback because the rule is the read path's — resolve the parent, reject a foreign or non-live one, check item visibility, apply the orphan rule — and every input to it lives in package server. It cannot run BEFORE planning either: the copy re-reads the source content under its locks and computes destination fields inside its transaction, so a reference set enumerated beforehand is not the set the planner resolves. Authorizing the rows the planner actually resolved keeps the dry run and the copy on one path, which is the property DR-11 exists to protect. Both endpoints take the authorizer off the same shared resolution (resolveAuthorizedCopy), so what the preview calls unresolvable is what the copy refuses to clone. Mutation-verified: without the authorizer the secret PNG is cloned into the destination, referenced by the rewritten body, and served byte-identical to the attacker through the destination workspace.	2026-08-02 04:54:31 +00:00
xarmian	2318a17e49	fix(attachments): classify derived rows after authorization on delete ... Found by the convergence sweep of this branch, which enumerated every attachment-touching path and compared each against its siblings' gates. The delete handler answered 400 derived_attachment as soon as it saw a ParentID, before any visibility, restriction, role or edit gate. That 400 is reachable only for a row that exists and is live, so a guessed thumbnail UUID answered 400 while an absent, foreign, or deleted id answered the shared 404 — and a caller who could not see the parent, or was restricted out of its collection, learned about the row anyway. Fifth instance of this handler family's existence oracle. Moved after the authorization switch. The classification is a usage error, so it may only be reported to someone already entitled to act on the row; the test pins BOTH halves, so the fix cannot regress into blanket-404ing a legitimate mistake by an authorized caller. Mutation-verified: restoring the previous position makes the restricted caller receive 400 again. Gates: make check exit 0, make test-pg exit 0, zero failures.	2026-08-02 01:50:34 +00:00
xarmian	ba848af85f	fix(attachments): check restriction before the role gate on orphan delete ... Found by the convergence review of this branch. The orphan branch of the delete path called requireMinRole("editor") before attachmentCallerIsRestricted, so a restricted member who guessed a live orphan's UUID got 403 while a bad UUID got 404 — confirming the row exists. Fourth instance of the same existence oracle on this branch, and the one path whose gate ORDER the refactor did not re-check. Notable because attachmentCallerIsRestricted's own contract, added in the previous commit, states that callers must apply it ahead of any role gate that would answer 403. Centralizing the invariant did not fix call-site ordering; only re-reviewing did. Test covers both restricted roles: a viewer and an editor answer differently at the role gate (403 vs success), and NEITHER may be distinguishable from the lookup miss. Mutation-verified — restoring the previous order yields exactly "status = 403, want 404". Gates: make check exit 0, make test-pg exit 0, zero failures.	2026-08-02 01:21:31 +00:00
xarmian	1da96106e8	refactor(attachments): centralize parent resolution, close orphan-read and delete-denial gaps ... Per the final full-diff review of this branch. The six task commits each added authorization to a different attachment path, and each was reviewed CLEAN on its own — but they hand-rolled the same invariant four ways, and the drift between them opened two real gaps that no per-task review could see. Root cause: the blob read, transform, thumbnail derivation and delete paths each loaded the parent item, checked workspace identity and checked liveness in their own shape. resolveAttachmentParentItem is now the one place that invariant lives, returning a four-way outcome (orphan / ok / gone / foreign) so callers keep their own denial behaviour — which is deliberate, not accidental: the HTTP paths must not distinguish the outcomes (any split is an existence oracle), derivation logs a distinct WARN per outcome (greppable ahead of PLAN-2397's repair), and delete passes includeArchived because the storage listing intentionally surfaces archived-parent rows so their quota can be reclaimed. Gaps the drift opened, both closed here: - Orphan GET lacked the full-access gate transform and delete apply, so a restricted member who guessed an orphan attachment's UUID could download it — while transform, delete and the listing all refused. Now shared as attachmentCallerIsRestricted, applied ahead of any role gate, since a 403 reached only for rows that exist is itself the oracle. - The delete path still routed invisible parents through requireItemVisible ("Item not found") while missing and foreign attachments got "Attachment not found" — the same existence oracle already closed twice on this branch, left inconsistent on the one path the tasks did not touch. Every delete denial now goes through the shared writer, asserted byte-identical. Also folds in the live-parent write invariant on upload, which had been applied to transform only: upload validated the item before spooling and then inserted with plain CreateAttachment, so archiving during the upload window bound a row to an archived parent. Derivation deliberately still does NOT take the lock — that trade is documented on deriveThumbnails. Gates: make check exit 0, make test-pg exit 0 (zero failures). Both new guards mutation-verified; attachment authz suite clean under -race -count=2.	2026-08-02 00:55:20 +00:00
xarmian	90eb871da3	fix(attachments): skip derivation for an archived parent (TASK-2404) ... deriveThumbnails checked only that the parent ATTACHMENT row was live and then copied parent.ItemID verbatim into every derived row. After TASK-2401's read gate that is a waste with a cost: a variant of an archived item's attachment is quota-counted storage that the blob path (DR-13) refuses to serve, so the bytes are written, charged, and unreadable until the item is restored. The same holds for a malformed item_id — the column has no FK and no same-workspace constraint, so a row can name a foreign-workspace item or no item at all. Derivation now resolves the parent item at entry, before the blob is even opened, and skips when it is soft-deleted, unresolvable, or in another workspace. GetItem, not GetItemIncludeDeleted, so "live" means the same thing here as on the read path. Orphan rows (item_id NULL) have no item to check and still derive. This is internal background work with no HTTP response, so there is no 404 shape to match: it skips and logs a WARN alongside the existing decode/resize/persist skip logs, with the malformed cases carrying distinct messages so they are greppable ahead of PLAN-2397's repair. The post-check window is DELIBERATELY ACCEPTED, and the comment on thumbnailParentItemLive says so at length so the next reader does not file it as a bug. The check is point-in-time — item deletion commits in its own transaction and the read/decode/resize/encode/Put in between is unbounded work — so an item archived mid-flight can still get a variant. Transform (TASK-2402) closes its equivalent window with store.CreateAttachmentForLiveItem; derivation deliberately does NOT, and makes the opposite trade: transform is user-initiated and low-volume, whereas derivation is a background worker fanning out from every image upload, so an item lock here is disproportionate to the harm. What leaks through is a thumbnail — small, unreadable for as long as its item stays archived, and tombstoned by the delete cascade with its parent attachment. Tests cover the sequential cases only: already-archived (with a sanity check that DeleteItem really is a soft delete), unresolvable item_id, and a foreign-workspace item_id that resolution alone would accept. The raced case is deliberately not asserted — it is permitted behaviour, and pinning it either way would constrain what the design leaves free. Two controls keep the skips honest: a live parent and an orphan row must both still derive from the same fixture and the same bytes, so a fixture that stopped deriving at all would fail loudly rather than pass the skip assertions vacuously. All three skip tests were mutation-verified against a short-circuited guard, and the file passes -race -count=3 and make test-pg. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC	2026-08-01 23:27:34 +00:00
xarmian	380b75e12c	fix(attachments): gate transform on item visibility (TASK-2402) ... handleTransformAttachment opened with a flat requireMinRole("editor") and never looked at the attachment's parent item at all. A restricted editor — one whose collection access excludes that item — could transform an attachment on an item they cannot see, given only the attachment id: the handler read the source blob and returned output metadata plus a new row. The output URL inherits ItemID and is gated by TASK-2401's read gate, so this was not direct byte exfiltration, but it crossed the same boundary and leaked processing behaviour and metadata for an invisible item. The handler now authorizes per-attachment, in the order the read path uses (PLAN-2391 DR-10): load the row -> workspace identity -> load the parent with GetItem -> parent workspace identity -> checkItemVisible -> edit permission -> transform. Every denial goes through writeAttachmentNotFound, so a missing attachment, a foreign parent, a soft-deleted parent and an invisible item are byte-identical; a distinguishable code or message would be an existence oracle. Malformed non-null parents that resolve nowhere are rejected by the same guard. Edit permission is requireEditPermission rather than the flat editor role: an item- or collection-grant editor can already attach to the item (BUG-1661), so refusing them a rotate on their own upload would be an inconsistency, not a boundary. Orphan rows keep the flat editor gate and, matching the DELETE path (PLAN-2382 DR-4), require unrestricted workspace access — the storage listing hides orphans from restricted members, so the transform must not confirm one exists. DR-14's race is closed, not narrowed. The parent check is point-in-time: item deletion commits in its own transaction, and the blob read, decode, transform, encode and Put in between are unbounded work, so the item can be archived mid-flight and the insert then writes a quota-counted live row against an archived item whose bytes DR-13 refuses to serve. The new store.CreateAttachmentForLiveItem re-checks the parent under a row lock inside the insert's own transaction: the row is written against a live item or not written at all. FOR NO KEY UPDATE, not FOR UPDATE — DeleteItem's UPDATE touches no key column so the archival still blocks, while the many tables with a REFERENCES items(id) foreign key (comments, stars, the Yjs op-log) keep taking FOR KEY SHARE on the parent uncontended. SQLite skips the clause: _txlock=immediate already serializes writers there. Tests fail against the pre-fix code: the restricted-editor transform returns 404 with a body byte-identical to the missing-attachment body, and the mid-flight test archives the item from inside the processor's Encode — between the up-front check and the insert — asserting the hook actually ran so it cannot pass vacuously. The Postgres lock test polls pg_stat_activity until the statement is registered as lock-blocked rather than sleeping, and watches the completion channel so a missing lock fails immediately. Both were mutation-verified. Recorded, not fixed here: a refused insert leaves a rowless blob on disk, and the orphan GC is row-driven so nothing reclaims it. Pre-existing on the upload and thumbnail paths too; filed as BUG-2406 with the dedupe guard a correct fix needs. The comment claiming GC reclaims a transform's original was wrong and is corrected — only an orphan original is GC-eligible. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC	2026-08-01 23:05:26 +00:00
xarmian	6e2b972fb0	fix(attachments): gate blob reads on item visibility (TASK-2401) ... handleGetAttachment opened with a flat requireMinRole("viewer"). roleLevel("guest") is 0, below viewer's 1, so every grant-based guest was rejected before any item-level check ran and inline images broke in items shared with them (BUG-2386). The handler now authorizes per-attachment, in the order PLAN-2391 DR-10 fixes: load the row -> verify the parent item's workspace identity -> check item visibility -> serve. Orphan rows keep the flat viewer+ gate; the workspace-wide storage listing is untouched. Also closes two defects sitting immediately around that gate: DR-16 - GetAttachmentVariant scoped on parent_id/variant/deleted_at but not workspace_id, so a foreign-workspace variant sharing a parent id would be served after the local parent was authorized. Fixed at the store API rather than in the handler because the other caller, thumbnail derivation, has its own stake in the scope: an unscoped "does this variant exist?" probe lets a foreign row suppress generation of a legitimate local one. DR-13 - the parent is loaded with GetItem, so a soft-deleted parent 404s. The DELETE path keeps GetItemIncludeDeleted, unchanged. Denial paths now carry Cache-Control: private, no-store, set as the handler's first statement (writeError calls WriteHeader immediately, so anything later never reaches the wire); the positive private, max-age=3600 is set only after authorization succeeds. Every authorization-dependent refusal goes through one writer so the responses are byte-identical and can't be used as an existence oracle. The MCP image resource pad://workspace/{ws}/attachments/{id} inherits the gate; asserted against a real server rather than assumed. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC	2026-08-01 20:42:28 +00:00
xarmian	27b71fe4f6	fix(attachments): resolve item_id across both upload channels (TASK-2400) ... The upload handler read item_id from two places with different rules: authorization resolved only the query-string value, while the association step fell back to the multipart-form value and persisted it verbatim. Since ResolveItem accepts a UUID, a ref, or a slug, a form-supplied ref or a foreign-workspace id could land in attachments.item_id unauthorized and unresolvable — the malformed-row invariant BUG-2387's cross-workspace leak rests on. Three coupled changes (PLAN-2391 DR-2): 1. One effective item_id. Each non-empty channel is resolved in the request workspace and the RESOLVED canonical ids are compared — not the caller's spelling, so query "TASK-12" + form "<uuid>" is agreement, not conflict. Absent and explicitly-empty both mean "no value" (compared after TrimSpace). item.ID is what gets persisted. The form value is read from r.MultipartForm.Value rather than r.FormValue, which merges the query string back in and would collapse the two channels into one. A channel that repeats item_id has every value resolved rather than first-wins, since net/http otherwise silently discards the rest; the value count per channel is capped, because exact-string dedup can't bound the lookups on its own (TASK-7 / task-7 / TASK-0007 resolve alike). 2. Auth ordering. The no-item workspace-editor gate is deferred until after multipart parsing; firing it pre-parse 403'd a form-only item-grant guest (the CLI's shape) before the association that authorizes them was read. The query channel is still resolved and authorized pre-parse so a doomed upload never spools. The route's auth/workspace-access middleware chain is unchanged. 3. Spool cleanup. file.Close() closes the spooled multipart temp file but never removes it; added r.MultipartForm.RemoveAll() on every exit path, including success, where it leaked today too. Status codes (the pinned contract): an item_id that does not resolve in the request workspace → 404 item_not_found on either channel, cross-workspace UUIDs included; two channels — or two values on one channel — that each resolve but to different items → 400 item_id_conflict. Folded in from review: each resolved item is gated on requireItemVisible (404) before the values are compared and before requireEditPermission (403). Without that, the status split is an existence oracle for items a restricted member or ungranted guest can't see — directly via 404-vs-403, or by pairing a visible id with the id being probed and reading 400-vs-404. It also closes requireEditPermission's editor/owner fast path, which never consults collection visibility, so a collection_access="specific" member could otherwise attach to an item in a collection hidden from them. Two intentional behaviour narrowings, both following from DR-2's "reject a non-empty value that does not resolve": an item_id for a soft-deleted item now 404s where a workspace editor previously got a 201 (ResolveItem is live-only) — consistent with DR-13/DR-14 keeping archived parents from accruing new bytes; and an unresolvable item_id no longer falls back to the flat editor gate and silently stores the caller's string. Tests: extends TestUpload_GrantBasedEditorCanAttach with the form-only and both-channel grant-guest cases, the ungranted-item 404, and the paired-probe oracle check; adds canonical-UUID persistence, 404/400 rejection with no row written, repeated conflicting values, the value-count cap, and a >1 MiB isolated-TMPDIR fixture for the spool (a tiny in-memory body never spills to disk, so it would pass either way). The auth-ordering and spool tests were mutation-checked against the pre-fix behaviour. Gates: make check (exit 0), make test-pg (exit 0). Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC	2026-08-01 19:53:12 +00:00
xarmian	e115bb255e	feat(web): delete attachments from the item strip (TASK-2384) ... Adds the first in-item delete path for an attachment (PLAN-2382 phase 2). Before this the only surface was Settings > Storage, which is workspace-wide and disconnected from the item you're looking at. Server: handleDeleteWorkspaceAttachment no longer opens with a flat requireMinRole("editor"). That gate contradicted the UI's grant-aware canEdit (permissions.ts::canEditItem), which is true for a viewer holding an item- or collection-level edit grant -- so that user saw the affordance and got a 403, even though upload already admits them (BUG-1661). Authorization is now per-attachment, mirroring the upload handler: - item-bound: requireItemVisible THEN requireEditPermission. The order is load-bearing -- an attachment on an item the caller can't see must keep returning 404, not the 403 that would confirm it exists. - orphans: unchanged flat editor-role gate plus the guest filter, since there's no item context to authorize against. UI: per-tile delete control, in the DOM unconditionally so it's keyboard reachable (CSS reveals it on hover/focus-within). Gated on ItemDetail's mutationsEnabled, not raw canEdit, so a peeking master stays a complete read-only freeze. Optimistic removal with rollback + toast on failure, fenced so a switch mid-delete can't resurrect A's tile under B. The confirm warns when the id is referenced in this item's body, and deliberately hedges otherwise -- comment bodies, other items' content and fields JSON are not visible client-side, so it says "may still be referenced" rather than claiming non-use. Editor: the attachment-image NodeView assigned img.src with no error path, so a delete left the browser's broken-image glyph until reload -- reading as a network blip for what is a permanent state. It now degrades to the same .attachment-missing placeholder the markdown renderer uses, re-armed on uuid swap so rotate/crop clears a stale placeholder. Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC	2026-08-01 12:57:37 +00:00
xarmian	d9d96b85c9	refactor(store): delete two unused item-workspace-move accessors (TASK-2374)	2026-07-31 17:28:05 +00:00
xarmian	98c638fc86	refactor(server): extract resolveAuthorizedCopy shared by preflight and copy (TASK-2370)	2026-07-31 14:43:40 +00:00
xarmian	cfc83e8c57	fix(server): report partial and legacy relationships in the copy dry-run (TASK-2369) ... Two ways the cross-workspace copy preflight told a user "nothing to lose" when there was, both violations of PLAN-2357 DR-17's "none of this may be silent". P1 — the five relationship counters are ACL-filtered by the caller's collection visibility (correct, and TASK-2364 chose it deliberately), but "none" and "none that you can see" rendered identically. A caller with edit rights on the source and none on its relatives could read `children_orphaned: false` and run a MOVE believing nothing was stranded, while hidden children were orphaned in place. The filtering stays; the uncertainty is now surfaced. Every point that drops a relationship for visibility reasons sets a new `warnings.relationships_partial` boolean. It is a BARE BOOLEAN by design: how many are hidden, of what type and in which collection are exactly the facts the filter exists to withhold, and a marker that varied with the hidden count would reinstate the leak DR-10a, DR-10b and the moved-to pointer each closed separately. A negative test asserts byte equality of the whole warnings block across two workspaces that differ only in how much is hidden. It is false for an unrestricted caller AND for a restricted caller with nothing hidden, so the common case renders exactly as it did before. P2 — a child reachable only by a lone legacy `plan` edge was invisible to GetChildItems (its join is restricted to store.ChildLinkTypes), so an incoming `plan` relationship reported `child_count: 0` / `children_orphaned: false` even though archiving the source strands it. The link scan now folds such an edge into the child set, deduplicated against the two mechanisms already covered and subject to the same visibility, liveness and workspace guards. The outgoing direction (the item's own parent) already reported correctly. The mutating copy reports no relationship counters at all (ItemCopyResultWarnings is deliberately narrower), so there is nothing for assertPreflightMatchesCopy to disagree about. CLI renders the qualifier on the five affected lines plus a plain-language explanation; TS types carry the field for Phase 3's dialog. Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT	2026-07-31 04:26:55 +00:00
xarmian	f15ba86db0	docs(server): correct the cross-workspace authz re-check contract per final review ... The helper's doc mandated that a mutating caller "re-apply the check" inside its write transaction. Its only mutating consumer deliberately does not, and is right not to: these functions read through s.store rather than the caller's tx, so under READ COMMITTED the re-check would judge locked resources against authorization state read at several unsynchronised moments — reading as a write-time guarantee while providing none. State what a mutating caller actually owes (re-read the authorized resource IDENTITY in-tx and refuse if it moved) and what it must not do, so the contract and copyResourceInvariantPreCheck no longer disagree. Found by the final full-diff Codex pass over PLAN-2357 (P1: a documented write-time guard was in fact a TOCTOU check). Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT	2026-07-31 02:35:53 +00:00
xarmian	f8ff5742e5	feat(server): add cross-workspace copy endpoint with post-commit fanout (TASK-2365) ... Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT	2026-07-31 00:59:25 +00:00
xarmian	01d640978c	feat(server): add cross-workspace copy dry-run preflight endpoint (TASK-2364) ... Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT	2026-07-30 20:15:00 +00:00
xarmian	1eb1c9eda6	feat(server): expose ACL-gated moved-to pointer on item GET (TASK-2359) ... An item MOVED to another workspace — copied, then archived — can now say where it went. GET on a single item gains an optional `moved_to` block naming each destination in displayable terms (workspace slug + item ref + title + collection slug), so a consumer can render a link without a second call. No HTTP redirect, no resolver change. The ACL gate is the point. A destination is revealed only after the caller independently passes AuthorizeCrossWorkspaceRead (TASK-2358) with an ITEM scope on the destination item itself. Workspace-level access is not sufficient: a restricted member of the destination workspace, or a guest holding one unrelated item grant there, has a role in that workspace while having no right to the copied item's collection. A caller who fails that check sees NO hint a destination exists. The key is omitted entirely — not a null, not an empty array, not a boolean — so the response is byte-identical to an archived item with no move record at all. A structurally distinguishable response is itself the leak. Restore decision: the block is OMITTED for a non-archived source. Restoring a moved-out source leaves two live items with the same content in two workspaces, which is legitimate, but at that instant the source has not moved anywhere and the response must stop asserting that it did. Past-tense provenance is the back-pointer question and applies equally to plain copies, which this field must never claim as moves. Also honored: DR-2a (only archived_source rows feed the pointer; plain copies are back-pointer material only), per-destination filtering over the forward lookup's SET with no short-circuit on the first hit or first denial, newest-first ordering, a scan bound on the per-GET authorization cost, and deliberate isolation of the hand-rolled public share-link DTO — pinned by an explicit negative test that freezes its key set. Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT	2026-07-30 12:20:38 +00:00