259 Commits

Author	SHA1	Message	Date
xarmian	effd0199cd	fix(events): detect a half-open Redis connection with a bus heartbeat (BUG-2738) (#1195) ... * fix(events): detect a half-open Redis connection with a bus heartbeat (BUG-2738) A Redis connection can stop carrying traffic without closing -- no FIN, no RST, just a route that stopped working. The instance blocks on a read that never returns, receives nothing, and its replay buffer goes on looking complete, so every resume is answered "caught up" from a coverage window that ended when the route did. go-redis cannot see it: PubSub.Ping writes the command and never reads a reply (v9.22.0), so its health check reports healthy for as long as the socket accepts writes. Measured on day-52 against a proxy that silently stopped forwarding: no reconnect in 24 seconds. Each subscription now records when it last received ANYTHING -- event, heartbeat, or subscription acknowledgement -- and a background pass ends the coverage of any workspace whose stamp goes stale past 3T, then REPLACES the connection. Drop alone would not recover: the resync it demands is served from the same dead socket, so the detector fires again on the next pass. Dave's day-49 ruling dissolves the threshold rather than tuning it. The bus publishes its own frame every T=30s and fires at 3T=90s, which turns "is this workspace quiet or is the route dead?" -- unanswerable, deployment-dependent -- into "did our heartbeat arrive?". TWO PHASES, ORDER NOT OPTIONAL. The frame must travel on the workspace's event channel, because that connection is what needs proving. A pre-phase-1 binary cannot classify it: the frame reaches the event decoder, fails, and since BUG-2739 that is a hole in coverage -- so an early flip makes every un-upgraded instance drop its buffer and resync all its clients, every 30s, per workspace, for the length of a mixed deployment. Phase 1 recognises and ignores; PAD_EVENTS_HEARTBEAT is phase 2, a constructor parameter with no default so every call site states its phase. The idle detector is a THIRD actor in a region whose invariants were designed around request goroutines plus Close. Four rules, each commented at cycleIdleSubscriptions and each with a test: 1. It refuses to cycle while pendingSubs holds a record, and MINTS the record itself before tearing anything down -- subscribeAndReplay checks pendingSubs before wsSubs, so a subscriber arriving mid-cycle joins the replacement instead of being admitted into the doomed subscription. 2. lastSeen is stamped at INSTALL, not left at the zero value, which reads as 1970 and would cycle hardest on an unconfirmed admission -- the workspaces already having a bad time. 3. wsCounts is re-read under the lock that performs the teardown. 4. Re-establishment runs on b.ctx with a nil establisher; the bus has no subscriber registration of its own to unwind. Two decisions beyond the plan: A NEW COUNTER, not just the reset reason. dropWorkspaceCoverage reports a reset only when a buffer existed to drop, and the incidents this detector exists for skew hard toward having none -- a route that wedged early on a quiet workspace. Reading cycles off the reset label alone would under-report exactly the case it was built to find, so pad_event_subscription_cycled_total is the dependable count and idle_timeout is corroboration. Both comments say which is which. THE CADENCE IS A LIVE TUNABLE -- a timer re-read under b.mu each pass plus a buffered kick, not a ticker constructed once. A ticker captures the interval at goroutine start, which makes the field write-once while its comment calls it a tunable and makes any later write a data race; it also leaves no deterministic way to test the WIRING other than a test-only constructor. decodePayload's signature grew a payloadKind. The classification belongs to the decoder, not the call site, so no future caller can reintroduce the coverage drop; and the prefix (rather than an exact payload) means a later frame version needs no third roll. Also swept, per the team's prose convention: receiveMessages' doc comment and deployment.md both said this gap was open and needed a decision. Both now say what closes it -- and deployment.md says the watch stream still has the same defect by the same mechanism, which is its own unit. Trio kept together: ResetReasonIdleTimeout, the metric Help strings, and docs/deployment.md's rollout order with the mixed-fleet failure named. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(events): rebuild the instruments the BUG-2738 matrix showed were blind The mutation matrix found a defect in the fix itself and three tests that could not have caught what they were named for. THE DEFECT: the idle scan skipped a subscription whose lastSeen was the zero value. That reads as belt-and-braces beside the install-time stamp and is the opposite -- it makes a subscription that has NEVER received anything permanently uncyclable, which is the BUG-2747 unconfirmed admission: the one population the plan singles out as mattering most, and the one where a wedged route would then be undetectable forever. It was also masking rule 2: with the skip present, removing the install stamp survived every test. Skip removed; that mutation is now caught. Re-adding it is undetectable by construction and the comment says so, because a guard that only acts once a real one has broken converts a caught defect into a silent one. THREE INSTRUMENTS THAT WERE NOT MEASURING: - "Drop only, never cycle" passed because establishSubscription overwrites wsSubs, so a generation check cannot see a replacement installed WITHOUT tearing the old connection down -- a leaked PubSub, connection and receive goroutine per cycle, forever, on exactly the wedged route where they never die on their own. Now asserted on the receive loop exiting. - The Close test was vacuous. Close drains wsSubs, so a loop that ignored b.ctx entirely would find no workspaces and publish nothing: silence after Close was evidence of nothing. maintenanceStopped makes the goroutine's exit observable, which is the same reason Observer.ReceiveLoopExited exists. - The joint test HUNG rather than failing under the drop-only mutation: the seam never fires, so the joiner goroutine was never spawned and an unbounded receive waited forever. The harness then aborted mid-run and LEFT THE MUTATION APPLIED to the working tree, which a grep caught and a green test run would not have. The wait is bounded and names the failure; the harness bounds each run, reports a hang as its own status, and restores in a finally. Added: a direct test that a straggler frame from a replaced generation cannot refresh its successor's liveness -- on a wedged route, the dead connection's buffered tail would otherwise suppress the detector for the replacement. RULE 3 IS AN OPTIMISATION, NOT A CORRECTNESS GUARD, and the matrix says so rather than an argument: removing the whole second read -- liveness, generation and count terms together -- survives every test, because establishSubscription's abandon path already refuses to install for an emptied workspace and retires the record in the same critical section (BUG-2749). The first read is redundant more sharply still: reaching zero takes the subscription down with it, so this loop never sees such a workspace. Both are kept, because neither DEPENDS on that coupling, and both comments now carry the per-term reading instead of describing tested defence in depth. The generation term is unreachable while the establishment record is held, by rule 1's own mechanism. Matrix: 16/22 detected, plus 4 follow-ups. Every survivor is documented at its line with why it survives. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(events): gate idle detection on heartbeat phase 2 (BUG-2738, codex r1) Codex round 1 found a defect the first draft had shipped WITH A COMMENT JUSTIFYING IT, plus two coupling hazards. P2-as-filed, P1 in effect: idle detection ran on every instance from phase 1, on the reasoning that it could "detect off whatever traffic the deployment already carries". That holds only for a BUSY workspace. A QUIET one on phase 1 has no events and no heartbeat, so a perfectly healthy subscription crossed the 90s threshold on every pass and was cycled: replay coverage dropped, every live subscriber told to resync, indefinitely -- on the DEFAULT configuration every deployment lands in before it flips anything. A resync storm shipped as the default, by the feature whose stated purpose is to avoid exactly that load inversion. Publishing and detecting are now one switch, which is what they always were: an instance detects off its OWN frames -- it publishes to the channels it subscribes to and receives them back -- so it never depended on peers having flipped, and there was never a reason for the two to be separable. Phase 1 is "recognise the frame so a phase-2 peer costs you nothing", and nothing else. Regression test plus its counterfactual, so "no cycles" cannot be satisfied by a detector that has simply stopped working. P1: the maintenance loop published heartbeats and scanned for idleness on one goroutine. publishHeartbeats makes N synchronous Redis publishes, and against the failure this feature exists to detect those are precisely the calls that block -- bounded by go-redis's own Dial/Read/WriteTimeout, not by any context we can pass. A stalled publisher could therefore delay detection for as long as those timeouts take, on the very instance whose connections had wedged, and for longer the more workspaces it carried. Two goroutines with their own kick channels; a stalled publisher now just produces silence, which is what the detector reads. P3: the cycle held the workspace's establishment record across a synchronous observer report, so an Observer callback that subscribed to that workspace would wait on a record only the reporting goroutine could retire. Moved the SubscriptionCycled report past establishment. The narrower half is older than this code -- confirmSubscription's late-acknowledgement path already reported from inside that window -- so it is documented on the Observer interface as a contract rather than silently worked around: a callback may publish, read and unsubscribe; it may not subscribe. Prose swept for what the gate falsified, per the team convention: the constructor comment that argued for the defect, config.EventsHeartbeat's rollback paragraph, the config test's inverted-rationale comment, ResetReasonIdleTimeout, both metric Help strings, and deployment.md's phase table and rollback section. All of them now say that phase 1 detects nothing and that the cycled counter is STRUCTURALLY zero there -- a zero on phase 1 says nothing about whether a route has wedged, which is the reading an operator would otherwise get wrong. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(events): prove a resuming joiner is told sync_required across a cycle Codex round 2 raised that a subscriber arriving DURING an idle cycle gets no gap signal, because dropWorkspaceCoverage only signals subscribers present when it runs. True, and for a RESUMING caller the gap signal is not what protects it: the registration mark is. It registers while the workspace has no buffer, so its mark cannot match whatever buffer exists by the time it reads, and eventsSinceMarkLocked answers nil -- sync_required rather than a false "caught up". A FRESH caller is deliberately not signalled and the finding is DECLINED for that case, with reasons recorded at the test: it holds no prior position, so there is no span it could be missing; it is admitted only after the replacement subscription is acknowledged, because it waits on the cycle's establishment record which finishPending closes after the confirmation; and on the unconfirmed-admission path it IS told to reconcile when the acknowledgement lands. Signalling it anyway would demand a resync of a client with nothing to reconcile -- the load inversion this unit already had to fix once. THE FIRST TWO VERSIONS OF THIS TEST DID NOT DISCRIMINATE, which is the part worth keeping. Version one asserted the empty case: the cycle leaves no buffer, so eventsSinceMarkLocked returned nil from its `!ok` term and removing the mark check entirely still passed. Version two published inside afterSubscriptionConfirmed so a FRESH buffer exists before the joiner reads -- and deleting the `mark.buffer == nil` term still survived, because the keep arithmetic in that function already reduces to zero for a nil mark. Only replacing eventsSinceMarkLocked with the unmarked eventsSinceLocked fails the test, handing the joiner the post-cycle event as though it followed its cursor. That is the mutation the test is built against, and the redundancy inside eventsSinceMarkLocked is recorded rather than mistaken for coverage. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(events): only count a cycle that actually replaced the connection (codex r3) Three findings from a fresh-angle round on shutdown, wire format and doc accuracy. The wire-format angle came back clean -- events:<workspace> cannot collide with watchevents under validated namespaces, and no valid activity payload can be mistaken for an hb| frame. P3, and the one that stings: config.EventsHeartbeat still said phase 1 "already runs idle detection off whatever traffic exists". That is the exact sentence the previous commit's sweep existed to remove, in a file that sweep edited. A grep for the phrasing I remembered writing missed the paraphrase sitting four lines above the paragraph I did fix. P3: SubscriptionCycled was reported unconditionally after establishSubscription returned, but establishment has two reasons to install nothing -- the bus closed, or the workspace emptied while we dialled. The counter's documented meaning is "torn down AND replaced", and counting an aborted establishment is wrong in the direction that matters: an operator reading a non-zero rate concludes connections are being blackholed, so a shutdown would manufacture that signal. Now reported only when a replacement is installed, verified by generation. Both Help strings and deployment.md say "counts replacements, not teardowns"; the teardown stays visible through the idle_timeout reset reason. P2: Close does not join the maintenance goroutines. Kept that way and documented on Close, because the publish half makes synchronous Redis calls bounded by go-redis's own timeouts -- the calls that stall on exactly the wedged route this feature detects -- so joining would let a dead network hold shutdown open. What has to hold instead is that a cycle already past its ctx check leaves nothing behind, which is now pinned by a test that closes the bus from inside the cycle's establishment: no subscription installed, no establishment record stranded, no counter moved. liveGen moved from the test file into the package -- production needs it now. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(events): restore the coverage the phase gate silently removed The mutation matrix, re-run against the post-codex code, showed M3 -- removing the install-time lastSeen stamp -- going from DETECTED back to SURVIVED. The cause was my own round-1 fix: gating idle detection on heartbeat phase 2 means a phase-1 bus never scans, and TestAnUnconfirmedAdmissionIsNotCycledAsIdle built its own phase-1 bus. It was the only test that could observe a zero lastSeen, because the plain fresh-subscription case is stamped twice over -- at install, and again by the acknowledgement. Flipped to phase 2 and re-verified: removing the stamp fails it again. Worth naming the shape rather than just the fix. A behaviour change that narrows when code runs silently narrows what the tests reach, and nothing in a green suite says so -- the tests still pass, they just stopped asking. Only re-running the matrix after the change surfaced it. Two harness bugs fixed alongside, both of which had been reporting non-results as if they were readings: - A mutation that INSERTS keeps its own anchor, so the "did the edit land?" check read every insertion as ANCHOR-ERROR. It compares the file now. - The two rule-3 mutations left `sub`/`live` unused and came back BUILD-BREAK rather than answering the question; they carry the same discard the follow-up harness already used. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(events): close the wiring and barrier gaps codex round 4 found Concurrency and lock discipline came back CLEAN -- the establishment record and the generation checks cover two racing cycles, Unsubscribe, Publish and a stale resubscription frame, with no lock-order deadlock. The four findings were all about whether the tests measure what they claim. P2, and it is the convention I had cited three commits earlier: the heartbeat flip had no wiring test. internal/events proves a bus built with publishHeartbeat=true emits frames and detects idleness, and every one of those tests passes if newObservedEventBus hardcodes false -- the deployment would simply never detect a wedged connection, which is indistinguishable from a deployment that has none. Both directions asserted, because a helper that ignored its config and hardcoded EITHER value passes a one-directional test. Mutation-checked against exactly that edit. P2: the metrics adapter test never touched SubscriptionCycled or the idle_timeout reason, so an adapter that folded the counter into the reset series -- destroying the very distinction those two are built to keep apart -- would have passed. Both added with counts that differ from their neighbours', the pattern that file already uses so a label-dropping adapter cannot satisfy the totals by coincidence. P3: TestAHeartbeatConsumesNoEventID "waited" on a predicate that returned true unconditionally. Not a slow wait -- no wait at all: the counter was read with the publishes still in flight, so a heartbeat that DID consume an id could land afterwards and the test would still pass. It now waits on the frames arriving, and fails against a mutation that publishes an event alongside each heartbeat. P3: the maintenance goroutines started on phase 1, where both halves are guaranteed no-ops -- two goroutines and two timers per process waking every 30s for the life of a deployment that asked for none of it, and phase 1 is the DEFAULT. The flag is constructor-only so the decision is taken once. The in-function gates stay: those are the correctness ones, and the tests reach them directly without a loop. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(events): validate the heartbeat frame and stop serialising recovery (r5) Client-facing behaviour came back CLEAN: an idle cycle signals each local subscriber, the SSE handler emits an in-band sync_required with an empty id while holding the connection open, EventSource retires its cursor and the web client runs the documented reconciliation. Two P2s on the other angles. FRAME VALIDATION. Accepting any "hb|..." created a silently-ignored class on the workspace event channel, where before this feature EVERY unreadable payload ended coverage loudly and moved undecodable_message -- the counter whose documented job is "suspect a namespace collision". A foreign or buggy publisher whose bytes happened to start with the prefix slipped through that signal without a trace. A frame is now hb|<version> plus optional short tokens under a length cap; anything else wearing the prefix goes back to being a coverage-ending decode failure, and the forward compatibility the prefix was chosen for survives for a disciplined future frame. What this deliberately does NOT try to fix, because it is not a hole: a forged frame cannot fake liveness. Liveness means "this socket carried traffic", and a frame that ARRIVES demonstrates exactly that whoever sent it -- which is why stampLastSeen already fires for undecodable frames. There is no coverage claim inside a heartbeat to forge. CADENCE DRIFT, which was self-defeating rather than merely untidy. The timer restarted after each pass, so the real period was T plus however long the pass took. For the publisher that means an instance whose publishes are slow emits heartbeats further apart, its own subscription sees them further apart, and it can cross its own 3T threshold and cycle connections that were never wedged -- the slowness manufacturing the incident. Scheduling is deadline-based now, and resets rather than bursting when a pass overruns badly. SERIAL RECOVERY. One idle pass re-established every due workspace in sequence, each re-dial bounded by go-redis's own timeouts, so recovery took N x that timeout with the last workspaces reporting themselves uncovered throughout. The failure that puts many workspaces on the due list at once is a Redis failover, so the serial case was the common one. Bounded-parallel at 8 -- each entry already owns its establishment record so they are independent by construction, and an unbounded fan-out would answer a struggling Redis with one dial per workspace at once. Test covers more workspaces than the cap, and fails against a version that drops the overflow. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * docs(events): idle_timeout means coverage ended, not connection replaced (r6) Codex round 6 came back clean on the non-Redis path (MemoryBus ignores the Redis-only flag; EventBus and Close have not drifted), on the rollback rehearsal (phase-2 to phase-1 and a mixed fleet are safe as documented, including a bus mid-cycle -- Close cancels it, prevents installation and retires its pending record), and on the operator surface (PAD_EVENTS_HEARTBEAT is a server env/TOML setting; `pad configure` is client connection config and needs no new surface). The one finding is a contract drift I introduced two commits ago and then wrote prose for in the same commit. Making SubscriptionCycled mean "replaced" was right; what I missed is that the idle_timeout RESET REASON is emitted earlier -- dropWorkspaceCoverage runs before the re-establishment -- so it can fire when nothing is replaced, which is exactly the shutdown case the counter was changed to exclude. Three doc sites and one log line said "replaced the connection" anyway. They now say what is true at the moment each fires: idle_timeout means COVERAGE ENDED, only pad_event_subscription_cycled_total proves a replacement, and the log says "attempting to replace" rather than "replacing". The log wording matters on its own -- an operator correlating it with the counter would otherwise find the log without the counter and go hunting a bug that isn't there. Third time this unit has produced prose the next change falsified, and each time a different reviewer angle caught it rather than the sweep I ran at the time. The pattern is that a behaviour change and the prose describing it land in one commit, so there is no diff between them to notice. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(cmd): drive the heartbeat wiring test instead of sleeping at it (r7) Codex round 7 found no leftovers across seven rounds of edits, and confirmed the mass-cycle case does NOT produce a reconnect storm -- the SSE connections stay open across a sync_required, so the admission limits are never consulted. P3, and it is the failure I have been criticising in other people's tests: the wiring test used a 300ms sleep as its ordering barrier. Under -race or on a loaded CI box, a phase-1 bus that is correctly silent and a phase-2 goroutine that merely has not been scheduled yet are indistinguishable, so the test could pass or fail for reasons unrelated to the flip it exists to check. It now drives one publish pass synchronously through a named test hook and uses an ordinary event on the same channel as the barrier, which Redis delivers in publish order. No timing left. Verified: still fails against the flag being hardcoded false, and ten consecutive -race runs are green. That replaces SetMaintenanceCadenceForTest with PublishHeartbeatsForTest rather than adding to the exported test surface -- the loop's own wiring is covered inside internal/events, where the unexported setter is available. P2 is FILED, NOT FIXED, as BUG-2761: a mass coverage drop tells every connected subscriber of every affected workspace to resync at once, and each browser tab independently calls /changes with per-tab coalescing but no jitter and no global budget. The fix is a web-client change plus possibly a wire-format hint, which is independent of half-open detection and would materially expand this diff. Worth filing rather than shrugging at because this unit makes the simultaneous case MORE likely: it adds a third trigger of a class that already existed (Redis failover, epoch change), and its natural cause is exactly a network event that wedges many routes at once. deployment.md carries the residual with the bug ref so an operator meets it before the incident does. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(events): make the tests prove what their comments claim (codex r8) Round 8 was claim verification rather than bug hunting -- check the diff's load-bearing assertions against the actual code -- and it was the highest-yield round of the eight. The go-redis assertions (Ping writes without reading, the channel path sets no read deadline, TLS dials ignore cancellation) and the four claims about neighbouring functions all held. Seven other assertions did not. TESTS THAT DID NOT PROVE THEIR OWN HEADLINE. This is the substance of the round, and every one of these passed before and after: - The JOINT TEST -- this unit's flagship -- claimed to discriminate the two-subscriptions failure and did not. Fan-out is per subscriber, so a joiner that opened its OWN second subscription still delivers the event to everyone exactly as the test expected. Nothing separates one subscription from two except counting them, which it now does at Redis, plus a duplicate-delivery check for the second receive loop. Fails against the pending record not being minted in the scan. - The remedy test said "the old connection must also be gone" and waited for a receive-loop exit. stopRedisSubscription does two things and the loop exits on the first alone, so it passed against a version that cancelled the loop and left the PubSub and its health check open. Counted at Redis now; fails against exactly that mutation. - The parallel-recovery test could not tell serial from parallel -- a serial pass cycles all thirteen workspaces too. It now uses a rendezvous, asserts the peak concurrency is above one AND within the cap, and fails against a serial implementation. - The prefixed-garbage test only exercised the classifier. Whether receiveMessages ACTS on the error is a different claim, now driven through the real Redis path. - The metrics adapter test's comment said "every reason this bus can emit" while subscription_unconfirmed was missing; its zero-assertion proved non-leakage, not mapping. Emitted now with a count distinct from its neighbour's, so a merging adapter cannot satisfy both. PROSE THAT OUTLIVED THE CODE, again. The latency arithmetic still described the single shared ticker that round 5 replaced with two independent loops; from lastSeen [3T,4T) still holds, but from FAULT ONSET it is roughly [2T,4T) because the publisher has its own phase. And a second "and replaces the connection" in deployment.md that round 6's sweep missed. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * docs(events): correct three contract statements (codex r9) Round 9 was cross-artifact conformance: every commitment the plan made was checked against the code. All met -- wire classifier, lastSeen placement and locking and install stamp and every-frame stamping, heartbeats bypassing Publish and the shared counter, the drop-and-cycle remedy under the single-establisher invariant, all four joint rules, the two-phase rollout with its inverted-rationale test, and the reason/Help/deployment.md trio with the rollout order. It also confirmed the three documented mutation survivors are correctly dispositioned: both wsCounts checks are redundant-but-cheap under the current invariant, and omitting the lastSeen.IsZero() skip is right because adding it would mask a regression in the install stamp. Three statements were wrong. The env-var contract. My test comment said an unparseable PAD_EVENTS_HEARTBEAT "must leave the flip off", which is true from a default config and false from a config file that set it true -- there the value is left alone, as the precedence test already asserts. The BEHAVIOUR is right and matches the epoch flag: a typo must not move a migration in either direction, and silently rolling an operator back to phase 1 would disable detection on a fleet that had opted in with nothing saying so. Only the prose overclaimed, and it overclaimed in the direction that invites someone to "fix" the ignore into a fail-closed reset. The constructor. NewRedisBusWithKeys documented publishEpoch and said nothing about publishHeartbeat sitting next to it -- two adjacent booleans of the same type belonging to two independent migrations, which is a shape that gets swapped or dropped in a maintenance edit. Both now documented in order, with a note that any combination is valid. A stale count. EventSequenceResetsTotal's comment said "Five reasons" and there are seven; it was already wrong by one before this unit added another. Replaced with the count plus a pointer to the three artifacts that are authoritative and move together, since the count itself is the part that goes stale first and is read last. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(events): make the cadence arithmetic testable, and justify a guard pair Matrix 5 (29 mutations, 21 detected) surfaced two things the previous run could not, because both concern code the codex rounds added. THE DRIFT FIX HAD NO TEST. Restoring the sleep-after-work form survived every test in the package, and would have kept surviving: the only way to observe drift through the loop is to time it, and a timing assertion is a flaky assertion. Extracting nextTick makes the arithmetic checkable without a clock, and the four cases now pin what the schedule is for -- a slow pass does not push the next tick out, ten slow passes accumulate no drift, an overrun beyond one interval resets instead of replaying the missed ticks, and an overrun WITHIN one interval still catches up rather than re-phasing the schedule permanently. Both directions mutation-checked. The property is worth this much because breaking it is self-defeating rather than merely untidy: an instance whose passes are slow emits heartbeats further apart, its own subscription sees them further apart, and it crosses its own 3T threshold and cycles connections that were never wedged. A GUARD PAIR THAT ONLY DIES TOGETHER, which the team lesson says to treat as a question rather than a clearance. The loop's ctx.Done select arm and its post-wait ctx check each survive removal alone. Checked rather than assumed: they cover disjoint moments and each is independently right -- the select arm is the exit while WAITING, which is where the goroutine spends its life, and the post-wait check stops a bus that closed DURING a pass from starting another one against a cancelled context and a drained wsSubs. Removing BOTH is detected. Reasoning recorded at the code, and the combined mutation added to the matrix so the pair cannot quietly become a single point of failure. Also fixed an ineffassign the lint gate caught in the new test. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * docs(events): state what the detector does not cover (codex r10) Round 10 was adversarial: refute the unit's central claim rather than look for defects in it. It partly succeeded, and the corrections are worth more than most of the bug findings. The claim was "a wedged connection is detected, coverage is ended, and the connection is replaced so delivery resumes". Three parts of that were too strong, and all three limits were checked against go-redis v9.22.0 rather than argued: IT IS A RECEIVE-SIDE DETECTOR, not a round-trip health check. It measures whether frames ARRIVE. A subscription whose outbound direction is broken but which still receives reads as healthy -- correctly, since nothing is lost, but that is a narrower claim than "the connection is healthy". IT CANNOT COVER THE PUBLISH PATH. PUBLISH travels on the client's connPool while a subscription holds a connection from the separate pubSubPool (redis.go:363, :1956) -- different sockets, different fates, and a reconnect of one repairs nothing about the other. An instance whose publish path is wedged loses its own events for every other instance and this feature will not say so. That is a real gap in the family's coverage, now written down rather than implied away. REPLACEMENT IS ATTEMPTED, NOT GUARANTEED. If the path is still blackholed when the cycle re-dials, the replacement cannot receive either. Coverage stays ended so nothing is falsely claimed, but delivery resuming is a statement about the network rather than about this code. Filed BUG-2764 rather than folded in: establishSubscription's `b.client.Subscribe(dialCtx, channel)` silently discards the SUBSCRIBE error, because go-redis's own Client.Subscribe drops it (`_ = pubsub.Subscribe(...)`, redis.go). A failed subscribe therefore installs a connection that looks live and is subscribed to nothing. It is pre-existing, it lives in the establishment path three bugs have already converged on, and changing how that function issues its SUBSCRIBE does not belong in a diff about idle detection. Worth knowing here because it is the one way the replacement can fail on a HEALTHY network -- and because the detector now cycles it on the next pass, which is why it self-heals on phase 2 and stays dead forever on phase 1. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(events): do not cycle a workspace that recovered before its turn (r11 P1) Codex round 11 attacked three claims. Phase-1 safety and rollback safety both came back clean -- a phase-1 receiver stamps lastSeen and nothing else, touches no buffer, metric, client, ID or epoch, and its maintenance loop is not started at all, so that timestamp is inert; heartbeats leave no state in Redis or across a process replacement, and a mid-cycle shutdown rechecks b.ctx before installing. The third claim did not survive. FALSE POSITIVES ON A HEALTHY SYSTEM, which is the property this design cares about most: cycling a working subscription drops its coverage and resyncs every one of its subscribers for nothing. cycleIdleSubscriptions selects its victims under the lock and releases it; the cycles run afterwards. Its re-checks asked about generation, subscriber count and bus liveness -- and never re-asked the question the scan had asked. A subscription that started receiving again in that window was cycled anyway. The window is not theoretical, and this unit widened it itself: the 8-way concurrency cap added in round 5 makes a workspace wait behind earlier batches of slow replacement dials, and a GC or CPU pause leaves a backlog of heartbeats undrained in the receive loop. Both are ordinary conditions on a loaded box. cycleOne now validates, ends coverage and tears down WITHOUT RELEASING THE LOCK in between, which needed dropWorkspaceCoverage split into a locked variant that returns its reason for the caller to report after unlocking. That also removes the ordering fragility the previous version documented rather than fixed: there is no longer any window in which coverage is ended for a workspace this function then decides to leave alone. The log moved after the decision for the same reason -- it could previously describe a cycle that then abandoned. The freshness term is load-bearing and says so, next to the three neighbouring terms whose mutation survivals are recorded as redundant-but-cheap. Removing it is detected, by a test that lands the recovery in the exact gap through a new positional seam. NTP steps were checked and are not a hazard: time.Time carries a monotonic reading, so a wall-clock step cannot make a subscription look idle. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * perf(events): take logging and PubSub.Close off the global lock (codex r12) Round 12 verified round 11's freshness fix: validation, coverage invalidation and teardown are atomic under b.mu with no lock cycle, dropWorkspaceCoverageLocked preserved the original semantics exactly including the no-buffer branch that still signals subscribers, reset reporting happens after unlocking, and the replacement metric still lands only when a new generation does. Slow establishment stays outside b.mu, wg.Wait only delays the next pass, and Close cancellation retires pending records. Two P2s, both about what round 11 put UNDER that lock: slog.Warn ran while b.mu was held. slog invokes the installed handler synchronously, and b.mu is the lock every fan-out and every Subscribe on the instance contends for -- a slow or custom handler stalls all of them, and one that calls back into the bus deadlocks. Moved after the unlock; it still has to come after the DECISION, for round 6's reason, so both constraints are now stated together at the call. PubSub.Close ran under b.mu too. It takes go-redis's own mutex, which the health check can hold across reconnect work, so a network-bound wait sat inside the instance's hottest lock. That was survivable when teardown only happened as a workspace lost its last subscriber; the idle detector makes it happen on every cycle, which is what turned a latent cost into a real one. Handed off to a goroutine: nothing references the PubSub once the map entry is gone, and cancel() -- which is what actually stops delivery -- still happens under the lock. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(events): do not read our own failed probe as a dead peer (codex r13) Round 13 asked for a production-approval review. Four findings; the second is the sharpest of the whole run because it is the mirror image of the failure this feature exists to find. A FAILED HEARTBEAT PUBLISH WAS READ AS A DEAD SUBSCRIPTION. The detector's inference is "we published a frame and nothing came back, so the receive path is dead" -- valid only if the publish actually happened. PUBLISH travels on the client's connPool while the subscription holds a connection from the separate pubSubPool, so a publish-side failure (pool exhaustion, a wedged outbound route, Redis refusing writes) says nothing about whether that subscription can receive. The detector was reading its own inability to probe as evidence about the peer, and tearing down healthy connections on a schedule: a resync for every subscriber of every workspace, every 90s, for as long as the outbound path stayed broken. The third load inversion this unit has had to fix. redisSub.lastProbeOK now records the last SUCCESSFUL publish, and detection is suspended while it is stale -- checked in the scan and again in cycleOne, which is a pair that only dies together and is therefore justified at the code: the scan's keeps a workspace off the due list so no record is minted and no joiner waits, cycleOne's covers the probe failing AFTER selection, a window the concurrency cap makes real. Neither subsumes the other; removing both is detected. New counter pad_event_heartbeat_publish_failures_total, documented as DETECTION DEGRADED rather than as a peer being broken. THE END-TO-END TEST THAT DID NOT EXIST. Every other test drives this through a fake clock -- necessary, since the threshold is 90s by construction and miniredis always answers, but it means they all ASSUME the wedge rather than produce it. A TCP proxy that stops delivering server->client on the connections already open, while writes keep succeeding and new connections stay healthy, produces the real thing. The test asserts both halves of the claim: the wedge is detected, and the replacement delivers. Both halves mutation-checked (detector disabled; drop-only with no replacement). The proxy's first version was vacuous -- a global flag consulted at read time meant re-enabling delivery for future connections also revived the ones meant to be dark. Per-connection now, and the comment says why. Also: PubSub.Close taken off b.mu in Close() too (round 12 fixed only the cycle path), and the replacement counter now takes an explicit installed result from establishSubscription rather than inferring one from the live generation -- inference misattributed an unrelated caller's fresh subscription as this cycle's replacement, and missed a real replacement that had lost its last subscriber. Both mutation-checked. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(events): bind the probe stamp to a generation; make the proxy test honest Round 14 returned a BLOCK verdict on two P2s, both mine, both in the fix that round 13 had just added. lastProbeOK WAS NOT GENERATION-BOUND. publishHeartbeats snapshots the workspace list, publishes off the lock -- for as long as go-redis's timeouts allow -- and then stamped whatever subscription occupied that workspace by the time it returned. A probe sent for generation A could credit generation B, which never received one; if later probes then failed, B could be cycled while looking recently probed. Exactly the hazard stampLastSeen already guards on the same map, and I did not carry it across. The generation now travels with the snapshot and is validated before stamping. THE END-TO-END TEST COULD PASS WITHOUT EXERCISING WHAT IT CLAIMED. It darkened the receive direction of every open connection, including the ordinary pooled connection PUBLISH uses -- so the probe may have been failing too, and the run would then have been exercising the cannot-probe path rather than a half-open route, which is the very distinction round 13 added the premise check for. The proxy now classifies connections as it forwards and darkens only one that has carried a SUBSCRIBE, leaving the publish path healthy, and the test asserts zero probe failures so a run that drifts back into the other case fails loudly instead of passing quietly. Still fails against a disabled detector and against drop-only. Also covered the new counter's mapping in the metrics adapter test, with a count distinct from both neighbours -- cycled, idle_timeout and heartbeat-publish-failure say three different things and an operator acts on the difference. Verified by the same round: install-time stamping does not permanently suppress detection, establishSubscription returns false only on abandon and true on all three installed paths including the cancelled-establisher goroutine, and Close's deferred PubSub.Close runs after the unlock. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(events): pin the probe-across-replacement race (closes r15's residual) Round 15 returned CLEAN and approve-with-comments, naming one residual: the generation binding on lastProbeOK had no deterministic test, only the argument that it mirrors stampLastSeen. This closes it with a positional seam between the publish and the stamp, which is the only place that interleave can be forced. TWO INSTRUMENT DEFECTS ON THE WAY, both caught by mutation rather than by reading: The first version compared the credited stamp against the PROBE's timestamp. On a frozen clock the replacement's install stamp and a wrongly-credited probe are the same value, so it could not tell them apart -- it failed on the install stamp while claiming a credit had happened, and removing the generation binding still passed. It now compares against what the replacement was INSTALLED with, and the clock advances inside the seam so a buggy write lands strictly later. The second version was FLAKY: 2 failures in 3 runs. The heartbeat that was just published comes back through miniredis on another goroutine, and if it lands between the forced-stale write and the scan it refreshes lastSeen, the workspace is not due, and no replacement happens. Retried until the generation actually moves. Now 5 of 5 green unmutated and 5 of 5 detected mutated -- which is the bar, because a 2-in-3 detector reads as coverage while being noise. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(events): on-call signals — log the cycle outcome, correct two claims (r16) Round 16 read the diff as the person paged at 3am. Four findings. THE CYCLE LOGGED ITS ATTEMPT AND NEVER ITS OUTCOME. The line says "attempting to replace", which is correct and, on the one path where the replacement does not happen, left an on-call with a warning, no counter movement, and no explanation. Now there is a second line naming the reason. pad_event_receive_loop_exits_total's documentation was falsified by this unit and neither doc site said so: every idle cycle stops a receive loop while its subscribers are still connected, and the comment still claimed exits happen only at shutdown or when the last subscriber leaves. Both sites corrected, with the expectation that it tracks the cycle counter during an incident. A CLAIM I MADE AND THEN COULD NOT SUPPORT, recorded rather than quietly kept. Round 16 argued the age-based premise check ("has a probe succeeded within the threshold") failed to suspend detection where an ordering rule ("has a probe succeeded since anything last arrived") would, and I rewrote the rule on that argument and wrote a test named for the defect. The mutation matrix then refused to confirm it: reverting to the age form leaves the test green, and so does removing both copies of the check, and no case separates the two — on any healthy path the two stamps advance together, because a probe whose frame arrives sets both, and they diverge only on the wedge where both forms cycle. The ordering rule is kept, because it states the intent exactly and is never weaker. But the test and the comment now say what they actually establish — that a probe which has started failing stops the detector concluding from silence, which is the property both forms share and neither had before — rather than claiming a fixed defect I cannot demonstrate. The two remaining P2s are already-filed residuals: the cycled counter proves an install rather than a working replacement (BUG-2764), and repeated cycling amplifies /changes load with no jitter or global budget (BUG-2761). Both are documented in deployment.md with their refs. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * docs(events): record what the final matrix actually says about four guards Final matrix: 34 mutations, 22 detected, baseline restored green. Every survivor is now documented at its line with why it survives, and two of them turned out to be instrument defects rather than coverage gaps. lastProbeOK's INSTALL STAMP IS REDUNDANT and the comment claimed otherwise. It said a zero value "would permanently disqualify a subscription from ever being cycled" -- true of the age-based premise it was written for, false under the ordering rule that replaced it, because a zero value fails `lastProbeOK.After(lastSeen)` exactly as an install stamp equal to lastSeen does. Kept, for a reason it earns: it makes the field's invariant true by construction, so a future rule reasoning about this value's AGE gets a real timestamp rather than 1970 -- which is the trap the age-based rule fell into one field over. THE TWO cycleOne ABANDON GUARDS DIE ONLY TOGETHER AND ARE NOT REDUNDANT, which took checking rather than assuming. They catch different shapes of the same recovery: an arrival that has not been re-probed pushes lastSeen past lastProbeOK so the premise case fires and the freshness case is unreachable -- that is the shape the test produces, and it is why removing either alone stays green. But the publisher runs on its own goroutine at its own cadence and can land a successful probe between the arrival and the decision, putting lastProbeOK ahead again; there only the freshness case stops a healthy subscription being torn down. Deleting it on the strength of the matrix would remove the second shape's only guard. Close's off-the-lock PubSub.Close is UNTESTED BY DESIGN, recorded rather than papered over. It is a contention property, and the only assertion that separates it is a timing one, which in this suite is a flaky one. TWO HARNESS DEFECTS, both of which produced false survivors that would have gone into the evidence package as findings. M11a inserted its mutation AFTER the gate it was meant to disable -- unique anchor, wrong placement, so the early return still fired and nothing changed; with a correct anchor it is detected. M20 left variables unused and came back BUILD-BREAK rather than answering; in compiling form it genuinely survives, consistent with establishSubscription's abandon path already covering it. The lesson worth keeping: when I rewrote all 34 anchors against current source I verified each matched exactly ONCE, and uniqueness is not placement. An anchor can be unique and still land somewhere that changes no behaviour. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(events): barrier the probe test on delivery — it was flaky, CI caught it Go (PostgreSQL) failed on af7001ab, in a test I added two commits ago. Not a timeout and not the race step: TestAFailedProbeAfterASuccessfulOneStillSuspends Detection asserted no cycle and got one. The test killed Redis immediately after a successful probe, without waiting for that probe's frame to be delivered back. If the frame never lands, lastSeen stays at the install stamp, the successful probe is then legitimately "after the last arrival", the workspace is genuinely due — and the code cycles it FOR THE RIGHT REASON under a test asserting it should not. The premise the test is named for simply did not hold on a slower machine. So this was not a false alarm in CI and not a defect in the code: it was my test asserting an outcome whose precondition it never established. Waiting for lastSeen to move makes the precondition real. Eight consecutive local runs green, and removing both premise checks still fails it, so the barrier did not neuter what it was measuring. Worth naming because it is the third instrument defect in this unit found by something other than reading it — after the harness restore that ate an edit and the unique-but-misplaced mutation anchor. A test that depends on an unsynchronised delivery is a test that passes on the machine that wrote it. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-24 16:57:52 -04:00
xarmian	5003718802	fix(push): apply delivery's visibility gate to delivered_sessions (BUG-2725) (#1187) ... deliveredSessionCount applied three of watchNotificationVisible's four gates, missing the first thing delivery checks: vis.allows(CollectionID, ItemID). Broadcast over-reported. Targeted was worse — the publish-skip reads this count, so the gate passed, the push went out, the stream dropped it on visibility, and the response said delivered_sessions: 1. An instruction lost behind a success. Per Dave's day-49 ruling, visibility is RE-RESOLVED at push time rather than snapshotted: membership and grants are revocable, so a value cached at connect goes wrong exactly when revocation is what makes it matter. The one input that cannot be re-resolved is the target connection's auth transport — computeWatchAccessVisibility consults isBearerAuth exactly once, inside the admin bypass, and the pushing request only knows its own. So SessionOrigin.BearerAuth is recorded at Add(). That is NOT the snapshot the ruling rejected: auth transport is a property of the connection, fixed when it opened and not revocable while held, so it cannot go stale. Armed is the precedent. SessionOrigin is kept separate from SessionIdentity because that type documents itself as self-declared and never verified; folding a server-derived security fact in there would silently retract the warning for one field. Both comments state the rule for future extenders: connection properties are admissible, derived authorization state never is. computeWatchAccessVisibility now takes a bool instead of an *http.Request, which makes the per-connection input visible in the signature and lets the count answer for a connection it is not serving. COST: "re-resolve per counted session" reads like N access checks per push. It is at most TWO, and sessionVisibility's memo makes that true by construction rather than by careful calling — every other input is per-user and identical across the sessions counted, so one varying boolean bounds the answers at two. Pinned by a test with 50 sessions. Codex round 1 (P1): the first version swallowed store errors into "not visible", reintroducing BUG-2698 through this fix — a targeted push reporting 0 SKIPS the publish, so a DB blip would drop the instruction and answer 200, in a function whose own doc comment says why 0 is load-bearing. Round 2 (P1): the same class one layer down — computeWatchAccessVisibility collapsed FOUR store failures into a denial, two discarded into underscores. Fixed as a class per CONVE-18. Resolution and policy are now separate: stream-side callers discard the error explicitly with reasons, only the counting caller propagates. Round 3 CLEAN. CONVE-23 sweep found three consumer-facing artifacts still describing the old mechanism, none on a line this diff touched: the plugin skill doc, the web push dialog, and pad push --help. All three corrected to name what actually remains rather than deleting the caveat. Plugin 0.3.1 -> 0.3.2, since installed plugins are version-pinned at install. NOT fixed, deliberately: the UNDER-count. A stream past maxSessionsPerUser receives broadcasts while never entering the registry. delivered_sessions remains an estimate with error in both directions, and every consumer-facing description now says so. Two coverage gaps recorded rather than rounded off: mutation M11 survives (the reporting test reaches only the first of four store calls, because closing the DB fails it first), and no test drives the whole chain store-fault-to-503 (the DB-close instrument kills the request earlier, so such a test would have gone green against the wrong 500 — deleted rather than relaxed). Also lands the BUG-2752 refutation sentinel: that item claimed the OAuth workspace allow-list went unenforced on /api/v1/events/stream. Refuted — no allow-list-bearing credential can authenticate to /api/v1/* at all. The test guards that format gate, so if it ever widens, the refutation's premise fails loudly instead of silently reopening a leak. Gates on the merged tip: make test 27 pkgs, make lint 0 issues, full Postgres suite 27 pkgs, govulncheck, codex CLEAN, CI 7/7. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 09:45:48 -04:00
xarmian	72336aacb5	fix(events): release SSE admission slots when a client leaves mid-establishment (BUG-2749) (#1186) ... `GET /api/v1/events` reserved its admission slot, then blocked in `SubscribeAndReplaySince` while the workspace's Redis subscription was dialled and — since BUG-2747 — acknowledged. Nothing propagated the request's cancellation into that wait, so a client that disconnected during establishment left a process-wide slot, a per-principal slot and a per-workspace slot held for the whole of it. The connection was gone; the capacity was not. Cancellation is now DEREGISTRATION, and `wsCounts` — which already answers "is anyone still here" — decides everything downstream. No ownership hand-off and no reaper: the arbiter already existed. (One thing IS handed off, and only one — the remainder of the confirmation wait; see below.) The two cancellation positions take different paths, and only one of them owes the joiners anything: - Before the install: the existing post-dial critical section already abandons and retires correctly when nobody is left. It needed one ordering rule — the departed establisher stops being counted IN THAT SAME SECTION, before the count is read. If joiners registered while we dialled, the count is still non-zero and they get the subscription; that is the hand-off the filing asked about, expressed as a count rather than a transfer of ownership. - During the confirmation wait: the subscription is already installed with its receive loop running, so the connection is not at risk — but the WAIT is what releases the joiners, and dropping it would admit them into a subscription Redis has not acknowledged while telling them nothing. That is BUG-2747's defect re-created at the seam between the two designs. So the remainder of the wait moves to a goroutine that finishes exactly as the caller would have: same arms, same `markUnconfirmedAdmission` on the bound, same `finishPending`. Bounded by `confirmTimeout`; no reaper needed, because teardown stays count-driven. A departure is not a refusal. `ok bool` is replaced by a `SubscribeOutcome` enum across the three `EventBus` Subscribe methods, so `SubscribeWorkspaceLimit` and `SubscribeCancelled` cannot be collapsed: answering a departed client with 429 would have written a limit refusal into the logs and counters that anyone would use to tune that limit. An enum rather than a second bool or an error because the switch has to name the case — by construction rather than by argument. Caller population, with its search boundary: 3 production implementations (events.MemoryBus, events.RedisBus, metrics.InstrumentedBus), 1 test double (server.gapEventBus, which embeds the interface), 2 production call sites (both in handlers_events.go). Searched this repo four ways — the three method names, `.Subscribe(`, method declarations, and interface embedding. collab.OpBus and watchevents.Bus are different interfaces and are out of scope; no other repo links this package. WHAT THIS DOES NOT FIX, verified in go-redis v9.22.0 rather than inferred from its doc comment (which says Subscribe "does not wait on a response from Redis" and so reads as though no dial happens on the request path — it does; only the reply is unawaited). On plaintext, dialConn derives its per-attempt deadline from the caller's context and the default dialer is net.Dialer.DialContext, so cancellation aborts the dial. Under TLS the same dialer calls tls.DialWithDialer, which takes no context, so the dial stays bounded by DialTimeout alone. On a TLS deployment this shrinks the held slot from (dial + confirm bound) to (dial), not to zero. Review round 2 (codex) found a P1 in this unit's own first draft, of exactly the shape the filing warned about. A cancellation check at the top of the establish loop could return while the caller still OWNED an unretired establishment record: section 1 had already named it the establisher, so the record stayed in pendingSubs with nobody behind it, its done channel never closed. The next subscriber for that workspace would join it and wait forever — and its own registration keeps wsCounts non-zero, so no later caller would establish either. A permanently dead stream that looks alive, produced by a guard whose only purpose was to save a dial. The guard is gone: a cancelled caller now goes THROUGH establishSubscription, which is the only code that knows how to put the record down. Regression test included, and reinstating the guard turns it red. Round 2 also found a P2 shutdown regression: routing the dial to the caller's context alone took away Close()'s ability to interrupt a stalled dial, which it had before. The dial now runs on a context ended by EITHER the caller or the bus, and each half is pinned by its own test — dropping either one is detected. Review round 1 (codex): no P1. One nit fixed as a class — three comments elsewhere in the file asserted the dial was "NOT bounded by the context we pass", which this change falsified; the sweep found and corrected all three (establishSubscription, defaultSubscribeConfirmTimeout, Subscribe). The TLS half of its P2 is filed as BUG-2754: the fix belongs at client construction, where it covers every Redis call rather than this one. Class sweep filed separately as BUG-2751 (lead-ruled: one region, one design per diff): internal/watchevents has no per-request establishment, but its resume path blocks on a 250ms settle window bound to the bus's context rather than the request's, while /api/v1/events/stream holds the same admission slots across it. Tests: five cancellation cases in internal/events (before install, during the wait alone, during the wait with a joiner, a cancelled joiner, an already-dead caller), a dial-binding assertion, and the handler-level binding in internal/server asserting the admission slot itself is released — the half of the bug that does not live in the bus. Mutation matrix, 8 mutations: 7 detected, each by the test named for it. The one survivor is the ctx term in the retry re-decide, and it survives because it is an OPTIMISATION rather than a correctness guard — a departed caller that mints a second record still establishes, deregisters and retires correctly; the term only saves a pointless dial. The code says so rather than implying the guard is load-bearing. The earlier draft's entry guard and loop-top break formed a redundant pair the matrix could only detect when both were removed. That redundancy was the smell, and round 2 found the substance under it: one of the two was not redundant, it was wrong. With it gone the entry guard is detected on its own.	2026-08-24 08:24:34 -04:00
xarmian	2e9ace4194	docs: nine overclaims across code, metrics, docs and the CLI (BUG-2739, codex round 5) ... A cross-artifact pass, which is the angle that keeps paying on this family. Every item below was a statement of mine that was false or unsupported; the code did not change. WRONG FACTS: - Watch epochs are opaque UUIDs, not numeric generations. I had copied internal/events' wording, where they ARE numeric — the distinction is the subject of internal/idspace's package comment. - undecodable_message was described as proof a notification was missed. The instance knows only that something it could not read arrived on its channel; it cannot tell whether that was ours. It stops vouching BECAUSE it cannot tell, which is a different and weaker claim. Corrected in four places. - The failover-cost paragraph said every SSE client on the instance reconciles. Wrong twice: a watch-bus resubscription ends the WATCH stream's coverage (activity coverage is per-workspace), and the one client that uses that stream today — pad watch --stream — answers sync_required by clearing its cursor and keeping the connection open, so it issues no request at all. Verified in cmd_watch.go rather than assumed. - The midstream/reset ratio is not fan-out in aggregate: the announcement counter also carries gaps and slow-subscriber drops and coalesces per connection. Only a reset observed in isolation reads that way. - 'The watch stream's only signal was a later non-contiguous notification' is true for a client HOLDING A STREAM OPEN. A reconnecting client was always covered, because a resume asks the shared counter instead of local state. Scoped in the doc and in the test header. - The dropped-confirmation fallback said coverage still ends. Usually, not necessarily: with no traffic during the outage nothing was lost, and if the drops continue through whatever would expose the hole and the stream goes quiet, nothing ever does — BUG-2727's boundary. Named both. - The new Observer Close warning was overbroad: reports run on the receive goroutine only on the RedisBus receive path, while a ResumeGap runs on the caller's and MemoryBus has no such goroutine. The rule stays unconditional, since a callback cannot tell which case it is in, but it now says why. STALE AFTER THIS BRANCH: - metrics.go's WatchSequenceResetsTotal comment listed two reset reasons. - observer_test.go said 'both reset reasons'. - The constructor comment named Channel() after the loop moved to ChannelWithSubscriptions, and did not say the Receive beneath it is load-bearing for that loop having no skip-the-first flag. It does now, and names the test that fails if it goes away. - cmd_watch.go's sync_required cause list predated BUG-2739 (and did not mention the mid-stream delivery BUG-2730 added). Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-23 13:33:58 +00:00
xarmian	b7ae022b6f	refactor(events): every way of subscribing hands back the gap signal (BUG-2730, codex round 5) ... Subscribe allocated and raised a gap channel its callers could not read, which round 5 called dead work. The read is right and the disposition is the other one: an interface method whose subscribers CANNOT be told they missed something is a silent under-delivery waiting for its first production caller, and internal/watchevents' Subscribe already returns the signal, so the asymmetry was the defect rather than the allocation. Subscribe now returns it too, on all three implementations. No production caller changes — the handlers use SubscribeIfAllowed and SubscribeAndReplaySince — so this is a test-call-site sweep plus one signature.	2026-08-23 01:01:59 +00:00
xarmian	6afe683389	fix(events): do not arm reset detection where interleave is ordinary traffic (BUG-2736) ... Codex round 9, from the 3am-operator angle. Six findings; one of them was a regression this diff would have shipped in the DEFAULT configuration, and the review framed it as a log-volume problem. THE REGRESSION. Phase 1 publishes with a two-call INCR-then-PUBLISH, so on any multi-instance deployment two publishers interleave routinely and a lower ID arrives after a higher one as ordinary traffic. main has no counter-backwards detection at all; this diff added it. Armed unconditionally, it would have fired on that ordinary interleave, dropped EVERY workspace's replay buffer, and resynced every client -- in phase 1, which is where every deployment sits until an operator flips phase 2. The check is now armed only once an epoch has been adopted. What that costs is stated rather than hidden: a genuine counter reset on a never-flipped deployment goes undetected, which is exactly the behaviour before this change and precisely the case phase 2 exists to fix. The new test asserts the gate, and also asserts what is NOT claimed -- the interleaved workspace's own buffer still holds ids out of order, so a cursor at the higher one reads as foreign. That is pre-existing, unchanged here, and strictly less harmful than a global drop; it is asserted rather than described so a future change to since() surfaces there. THE REST ARE THE OPERATOR'S SIGNALS, which were unreadable: - The effective phase was invisible. pad_event_sequence_resets_total cannot be interpreted without it -- a counter_backward rate is expected on phase 1 and an anomaly on phase 2 -- and the setting can arrive from an env var, a TOML file, or neither. It is now on the startup line as id_space_phase. - An unparseable PAD_EVENTS_PUBLISH_EPOCH was silently ignored, so an operator who typed "yes" believed they had flipped. Ignoring it stays the right behaviour; being silent about it does not. - Both publish-failure logs said only "failed to publish". They now say what the operator needs, which differs by phase: phase 1 may or may not have reached subscribers, and phase 2's script is atomic so it did not half-execute, but a lost reply means it may have published anyway -- do not re-publish by hand. - Adopting an epoch with empty buffers is the moment the documented residual becomes possible on that replica, and it happened silently. It now logs at INFO -- not a reset count, deliberately, since counting it would give the reset metric a per-deploy baseline. Declined with reasons: a cause label on the resume-gap counter and a publish failure counter are both pre-existing shapes rather than anything this diff changed, and the straggler log is already bounded by the recovery window. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 20:05:48 +00:00
xarmian	736a8c48f7	docs: thirteen claims about code that had moved under them (BUG-2736) ... Codex round 5, cross-artifact consistency. Every one was a claim in a comment, help string, doc, or test name that the code no longer supported — and this diff created most of them by moving the code. The ones that would have misled an operator: - pad_event_sequence_resets_total documented ONE reason in both the Go doc comment and the Prometheus help text, and the deployment table said the same. It has emitted three since this branch. An operator reading the help string to build an alert would have alerted on a third of the signal. - deployment.md said every published message carries an epoch prefix. Phase 1 publishes bare JSON — which is the entire point of having two phases. - deployment.md said the first flipped message reaches each replica and every resuming client gets sync_required. A replica learns the epoch only from a message it RECEIVES, so only replicas subscribed to a workspace with traffic see it; and a replica with empty buffers adopts without dropping or counting, deliberately. - deployment.md said a restart's IDs cannot collide. internal/idspace documents a bounded case — the earlier process publishing more than 2^20 events per millisecond of its life. Stated as the bound it is, with the backwards-clock direction named as the safe one. - cmd_watch.go described sync_required as eviction-only. It has had four other causes since BUG-2731 and gained a fifth here. The ones that would have misled the next person editing this code: - bus.go said the Redis half was unwritten and a reset counter could still merge two ID spaces. It is written, three commits back on this branch. - bus.go and watchevents.go said in-memory IDs restart from 1. They count from an incarnation base. - redis_bus.go described this bus's epoch as an opaque uuid equivalent to the watch bus's, twice, after round 3 made it a Redis-minted generation. Only the watch bus still uses uuids. - observer.go said counter_backward happens only during mixed-version rolls. Phase 1's two-call publish produces it in steady state too. - redisns.go said the publish script spans four keys (it is five here now, plus a two-key assign script), and its hand-kept reserved-name inventory never gained event_epoch or event_epoch_gen — so a namespace equal to either would have nested one installation inside another's keyspace unrefused. - A test comment referenced idIncarnationShift, which moved to internal/idspace.Shift when the package was extracted. - Two tests called themselves process-restart tests while constructing successive buses in one process. They test bus incarnations; the comment now says so and says why that is the equivalent thing. And one reasoning error rather than a stale fact: the counter-backwards branch justified raising the floor by asserting the arriving ID is necessarily in the SAME numeric space. It is not — a phase-1 counter reset publishes low IDs with no epoch to explain them, which is a NEW space we cannot see. The behaviour is unchanged and still correct (the lead's day-52 ruling: raise unconditionally, prefer a loud bounded resync loop to a silent skip), but it now says what it actually knows, which is nothing, and names the cost on a real phase-1 reset. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 19:28:17 +00:00
xarmian	d393126d80	test(events): close the gaps a tests-as-production-code pass found (BUG-2736) ... Codex round 4, on the tests themselves. Eight findings, all real; three of them were behaviours in this diff with no test at all. NO TEST AT ALL: - The real receive path. Every reconciliation test drove fanOutFromRedis directly and the publish tests read the wire with a raw subscriber, so a regression that decoded the epoch correctly and then handed 0 to the fan-out would have passed all of them -- reconciliation silently never running in production. Now driven through Subscribe/Publish and back through Redis, with the mutation checked. - The atomic script's ordering claim. Every phase-2 test published once or ran the script sequentially, so a two-call INCR-then-PUBLISH implementation passed them all -- and that ordering is load-bearing, because the receive path reads a descending id as a counter reset. 300 concurrent publishes now assert arrival order equals id order; verified to FAIL 5 of 5 against a two-call implementation and pass 3 of 3 against the script, so the instrument discriminates rather than merely being green. - The TOML tag. The env-var test proved PAD_EVENTS_PUBLISH_EPOCH reaches the field and said nothing about the toml:"events_publish_epoch" tag -- the exact form the rollback procedure warns about, since a file value outlives an unset env var. PASSING FOR THE WRONG REASON: - The production config wiring was still unexecuted: passing an empty config.Config at both RunE call sites compiled and passed everything. The source-text guard that already counts those call sites now also requires them to pass the loaded config. - The phase-2 wire assertions accepted a well-formed payload with an empty event body. They now assert the body survives. - The Redis metrics subtest had no served-resume control, so a bus that refused every resume would have passed. It now round-trips a publish through Redis first. - TestResumeGapIsReportedForBothWaysOfNotServing never proved ws-warm HAD a buffer, so its second half could silently duplicate its first. - internal/server's cold-resume tests still sent a literal 4200, which the incarnation guard now answers before the handler's no-buffer path is reached. My own round-1 sweep of this class stopped at four packages and never looked at internal/server: reviewer-named instances are a sample (team CONVE-18), and so, evidently, are self-named ones. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 19:15:12 +00:00
xarmian	a9544a57ba	test(events): repair the resume tests the base guard made vacuous (BUG-2736) ... Codex round 1 named three sites; the class was six, across four packages. Every MemoryBus test that spelled out a cursor as a small literal now passes through the incarnation guard before reaching the branch it is named for. The worst were the two that exist precisely to distinguish branches: the both-ways-of-not-serving observer test would have gone green with BOTH of its branches deleted, and the watch bus's eviction test would have gone green with eviction deleted. Cursors are now base-relative or read back from what the bus issued. Where the test has only the EventBus interface and no access to the base, the cursor is derived from a published event's id instead. Mutation-checked in the direction that matters: with the no-buffer branch, the coverage check, and the eviction check each made inert in turn, the tests named for them fail. Before this commit they did not. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 18:41:10 +00:00
xarmian	4a6a748c85	feat(events): identify the shared Redis ID space, behind a two-phase flip (BUG-2736) ... The activity event counter lives in Redis and is shared by every instance, so no instance can compute an identity for it the way MemoryBus computes its own incarnation base. If that counter is ever reset -- evicted under maxmemory, deleted by hand, a fresh Redis after a restore -- IDs start again from 1, and a replica buffering the old sequence cannot tell the new 101 from the old 101. It merges two ID spaces into one replay buffer and answers a resume across the boundary as though nothing was missed. Numeric detection alone cannot see it. By the time the new sequence passes the replica's high-water mark it looks like ordinary progress -- which is the case the epoch exists for, and the high-water check is what catches the OTHER case (a publisher that never learned the epoch), so both are kept. So the identity travels WITH each message, as an opaque token in a "<epoch>|<id>|<json>" prefix. A prefix rather than an envelope field: an older instance would unmarshal an envelope object SILENTLY -- no matching keys, no error, a zero-valued Event delivered to its clients -- and fails loudly on the prefix instead. TWO PHASES, because the failure is asymmetric. Every instance ACCEPTS both wire forms from this release; only emission is gated, on PAD_EVENTS_PUBLISH_EPOCH. Phase 1 rolls the binary everywhere publishing the historical bare JSON; phase 2 sets the flag and rolls again. Flipping before every instance is upgraded is the one direction that LOSES events rather than resyncing: a pre-phase-1 binary cannot parse the prefix at all. Rollback is symmetric and safe. docs/deployment.md carries the procedure both ways, what the reset counters should read during each roll, and what remains unfixed. Phase 2 also moves ID assignment into one atomic script. The two-call INCR-then-PUBLISH lets two instances interleave, so a receiving instance can append 6 before 5 -- a window older than this change, and already wrong, but load-bearing here because counter-backwards detection reads a descending ID as a reset. The script carries a dedupe token for the same reason internal/watchevents' does: go-redis retries a command whose REPLY was lost, so a publish can happen AND return an error, and the retry would deliver a second copy that looks perfectly valid. THE COUNTER-BACKWARDS FLOOR STAYS, and the earlier hope that this unit would delete it was wrong. Its trigger is mixed-VERSION ordering -- an older binary assigning and publishing in two calls -- not mixed-FORMAT payloads, so publish-old-until-flip removes the format window only. It lives for as long as a deployment can run two publisher versions at once, which is every rolling upgrade, and the code now says so where it fires. THE ASYMMETRY WITH MemoryBus IS DECLARED IN BOTH BUSES, in both packages: an opaque epoch where the counter is shared, a numeric base where one process owns it. They are not two spellings of one idea and must not be symmetrized. A numeric base for Redis would close more -- it would refuse cross-incarnation cursors, which the epoch cannot -- and is deferred rather than rejected: at the flip, IDs would jump to ~1.8e18 in one step and every un-flipped publisher's message would read as a massive backwards jump, dropping every buffer across the whole roll. It is a candidate follow-on once the flip has soaked. What this does NOT fix is stated in the code and the docs rather than implied: the client cursor is still a bare integer with no epoch, so an old and a new ID of the same value remain indistinguishable TO A RESUME even though the buffers can no longer mix them. The flip is read inside newObservedEventBus, which now takes the whole Config. As a hand-picked argument at the two RunE call sites it was untested wiring: replacing it with `false` compiled, passed the entire tree, and left the deployment silently on phase 1 -- indistinguishable from a correct phase-1 deployment, since phase 1 is the default. Mutation-checked in both directions, because a helper that ignores its config and hardcodes either value would pass a one-directional test. Also: the epoch and dedupe keys join the namespace assertions (an epoch shared between two installations is a cross-feed with teeth -- each would read the other's ID-space changes as its own), and this package's four-key EVAL is now recorded on BUG-2724's cluster deferral, which had one call site and now has two. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 18:31:02 +00:00
xarmian	c017ad359d	fix(events): give each in-memory bus incarnation its own ID space (BUG-2736) ... Both in-process buses assigned Last-Event-ID values from a counter that restarted at 1 on every process start. A client holding cursor 2 from a previous incarnation could reconnect to a restarted server, pass every coverage check BUG-2731 added, and be replayed the NEW space's 3, 4, 5 as though they followed the OLD space's 2 -- silently missing everything the dead space held above 2. Nothing local could tell the two 2s apart. The cursor carries no epoch, and in internal/events per-workspace IDs are non-consecutive by construction, so "did we issue this ID?" was numerically undecidable. The four adjacent levers were checked rather than assumed: comparing in memory has nothing to compare against; persisting the counter makes single-process Pad carry durable event-bus state and still resets on data loss; refusing cursors we did not issue is the undecidable one; and a nonce on a second channel is unavailable because EventSource echoes Last-Event-ID and nothing else, and cannot rewrite its URL on an automatic reconnect. So the ID space's identity goes in the ID's VALUE while its FORMAT is unchanged: still a bare int64, still ParseInt on the way back. internal/idspace mints a base of processStartUnixMilli<<20 and each bus counts up from it. Two incarnations can only collide if the earlier process published more than 2^20 events per millisecond of its own lifetime -- a deterministic bound, not the probabilistic one BUG-2736's body rules out. A CAS makes bases strictly increasing within a process too, which the clock alone does not do for two buses constructed in the same millisecond. A backwards clock step degrades in the SAFE direction: a lower base puts old cursors ABOVE the new buffer's newest ID, so they are refused rather than answered wrongly. The overflow bound is computed, not estimated: the last start instant that fits is 2248-09-26T15:10:22Z. Each bus then answers the resume question exactly instead of inferring it: a non-zero cursor at or below this incarnation's base was issued by a dead space. That is strictly stronger than the coverage check alone, which serves the ADJACENT cursor on reasoning that only holds within one ID space. In internal/watchevents the check lives in one helper both entry points call. Written inline in EventsSince it was absent from SubscribeAndReplaySince -- the path the SSE handler actually uses -- so the component was fixed and its wiring was not (team CONVE-19). A test now drives both. web's ItemEvent no longer declares `id?: number`. Nothing read it, which is the only reason it was harmless; a base of ~1.8e18 is past JavaScript's MAX_SAFE_INTEGER, so the first reader would have silently got a rounded number. Defused while still unread. Tests that spelled out IDs now read back what the bus assigned -- a literal 1 is a cursor from a dead space, which turned two negative controls into their own opposite. The two watchevents guards (cold buffer, dead incarnation) are tested separately, because a single test covering both would keep passing with either deleted. The Redis half is not here. Its counter is shared across processes, so identifying its ID space needs an epoch travelling with each message; that is the next commit on this branch. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 18:14:45 +00:00
xarmian	9f88e94832	fix(events): a resume must not be answered from coverage we never had (BUG-2731) ... internal/events answered a Last-Event-ID resume with an empty-but-non-nil slice whenever the workspace's replay buffer could not speak to the span being asked about. The SSE handler reads that as "caught up", so the client sat on a live stream believing it was current while everything between its cursor and now was silently gone. COVERAGE. replayBuffer gains knownFrom: the lowest event ID from which this instance's coverage of a workspace can be vouched for. A resume from below it answers nil, which the handler already turns into sync_required. Covers a buffer that does not exist (cold start, restart, scale-up, or simply the first connection to a workspace on this instance), a buffer that exists but starts above the cursor — NOT full and NOT empty, reachable on any multi-instance deployment with no eviction and no restart — and a non-zero cursor from a previous incarnation of a single process. knownFrom here means RECEIVING-continuity, never ID-contiguity, and the defining comment says so with the measurement attached. internal/watchevents has a field of the same name that ALSO detects holes by noticing a non-consecutive ID; porting that would have been a serious regression, because this bus has a global counter and per-workspace buffers, so a workspace's buffer holds non-consecutive IDs by construction (four publishes alternating across two workspaces measure as W=[1 4], X=[2 3]). An ID-contiguity check would fire on nearly every append and turn every resume into sync_required — the false-positive inversion of this bug. LIFECYCLE. Coverage now ends where it really ends: - a stopped workspace subscription drops its replay buffer. Keeping it "in case they come back" looks like a free win and is the bug: events published elsewhere never enter it while it goes on looking complete. - subscriptions are generation-numbered, so a straggler from an ended subscription cannot re-create a buffer and vouch for coverage that ended with it — including the case where the workspace has already been resubscribed under the stale goroutine. - a pub/sub reconnect ends that workspace's coverage. PubSub.Channel resubscribes transparently, so a Redis failover left a hole the buffer had no idea about; the loop reads pubsub.Receive instead. It must RECOVER rather than exit — returning on a transient error would leave an instance publishing fine and receiving nothing — and it drops ONE workspace's buffer, since a dropped subscription says nothing about any other channel. Subscribers are indexed by workspace because the replay buffers moved under the same mutex (necessary for the straggler race): scanning every local subscriber under that lock would make one hot workspace the serialization point for every other workspace's fan-out and every resume. Also removes Publish's local-counter fallback on a failed INCR, which minted an ID from a process-local space and published it — every receiving instance reads that as the counter having been reset. It bought nothing: this bus has no local fan-out path, so an event that does not reach Redis reaches no subscriber here either. SIBLING. internal/watchevents had the identical cold-resume defect on its MemoryBus — its RedisBus guards it, MemoryBus reached the buffer directly — so a single-process instance answered a post-restart resume as caught up. Found by a cross-artifact review pass; the guard goes in `since` so both implementations inherit it, and is tested through SubscribeAndReplaySince as well as EventsSince because that is the path the handler uses. Refs BUG-2731	2026-08-22 14:23:40 +00:00
xarmian	bb003dd6bb	fix: five claims the final comment-truth round found (BUG-2724, BUG-2726) ... The bounded process the lead set: N rounds, an author prune pass, one final comment-truth round. This is that round's output, and the loop stops here. Two were mechanisms I had wrong, and both are the kind a reader would reuse without re-deriving: - "Different Redis DB numbers do not help" was half true. Ordinary keys ARE DB-scoped, so two installations on different DBs keep separate presence registries; it is pub/sub that ignores DBs entirely, which is why the buses cross-feed regardless. Stating it as "does not help" made the namespace look like the only fix for a problem it only half is. - A namespace cutover's client resync was attributed to the epoch check. That check needs an OLD epoch to compare against and a freshly namespaced bus has none — the resync comes from the cold replay-buffer coverage check instead (knownFrom is zero, so every resume falls below it). Same honest outcome, different mechanism, and the mechanism is what someone reasoning about a cutover would use. Three were stale or over-general after earlier changes: the admission comment still said the global limit is passed to the bus as 0 (that parameter is gone), `pad watch --help` and the plugin monitor description lumped a missing .pad.toml's hourly retry in with the 5s-to-5min backoff, and CLAUDE.md said clients must back off without the browser exception docs/deployment.md spells out. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 05:21:01 +00:00
xarmian	461c5a3e3d	refactor: prune the claim surface, and turn one prose claim into a test ... The review could not converge on this diff's comments because each round of corrections re-expanded the surface it was reviewing — rounds 16 and 17 found errors inside 15 and 16's fixes. That is a production rate being measured, not a backlog being drained, so the treatment is to write fewer claims rather than review the same ones again. PRUNED, ~135 comment lines: process narration. "An earlier version said X", "found by mutation testing", "codex round N caught this", the scoreboards. Every one of those is already in a commit message, which is where the archaeology belongs; in the source they are claims a future reader has to verify, about a past that no longer exists. KEPT, because they earn it and a reader would otherwise re-derive them: metric semantics, reachability boundaries, what a test does and does not discriminate, why the obvious alternative was rejected, and the hazards that cannot be enforced in code. MOVED TO A TEST, per the rule this run earned the hard way: a comment asserting countable behaviour belongs in the suite. Two test comments in internal/watchevents relied on "this constructor waits for its SUBSCRIBE to be confirmed" — prose, and the same assumption applied to the OTHER bus (which subscribes asynchronously) is what made a namespace test flake. It is now asserted with no polling and no sleep, and the mutation that removes the wait fails it. That rule generalises and is why round 17's find mattered: "counts every unservable resume" was prose, so its falseness could hide a real metric gap. Prose is for claims that cannot be asserted. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 05:09:29 +00:00
xarmian	35e564298b	fix: seven more prose claims, one real metric gap, and a flaky test of my own (codex round 17) ... The prose angle again, and it is still finding things — which is itself the finding: this diff's comment density is generating wrong beliefs faster than the review is removing them, in the one dimension where the defect is a reader's understanding rather than the program's behaviour. Everything below was a claim I wrote. ONE WAS A REAL GAP, not just wording. pad_watchevents_resume_gaps_total was documented as counting every unservable resume, and counted only the half decided by the shared counter. The LOCAL half — a cursor below what this instance can vouch for, from a hole or a cold start — returns nil from replaySince, becomes sync_required for the client, and reported nothing. Now counted, on the deferred path so it fires with the lock released. Its test needed a second pass to be an instrument: the first version arranged a hole and asserted the counter moved, but the shared counter disagreed too, so resumeOutrunsLocalView reported and the mutation survived. It now sets the counter to AGREE with what the instance has seen, which is the only arrangement that isolates the local path. The prose corrections, swept by grep rather than by instance this time: - MemoryBus's comment said a single-process deployment never wires an observer. cmd_server wires one, deliberately — that is what makes the drop counter meaningful there, which is a claim I had just added elsewhere. - "Every write path works with Redis down" was too strong in three places. Push answers 503 for an unresolvable targeted push and 502 push_unconfirmed on publish failure — the paths whose job IS cross-instance delivery. - Presence-failure consequences were stated as certainties in four more places after round 16 fixed one. A failure means an error was REPORTED; Redis can fail a pipeline after applying it. - The deployment metrics table still described pad_eventbus_publish_total as "Events published" after the Help string had been corrected to attempts. - The reserved-namespace rationale called prefix nesting a "collision". It is nesting; an exact collision would need the namespace to match a workspace UUID. Refused anyway, and now for the reason that is true. - A presence cutover was described as stranding one renewal interval of stale entries. It is the full 90s TTL — three intervals. AND A FLAKE OF MY OWN, caught by the full suite rather than by the targeted runs: the activity-bus namespace test asserted subscription state immediately, but that bus subscribes ASYNCHRONOUSLY (the watch bus waits for confirmation; the two differ). It now polls, and the asymmetry is named in both tests so the next reader does not assume symmetry the way I did. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 05:00:18 +00:00
xarmian	2fa1316853	fix: three claims round 15's corrections got wrong or missed (codex round 16) ... Reviewing the corrections found three more, which is the honest shape of this: the prose angle keeps paying because the errors are in prose. - My round-15 correction said the Redis counters "stay at zero" on a single-process binary. That is wrong for one of them: pad_watchevents_notifications_dropped_total moves there, because MemoryBus has the same slow-subscriber drop and is wired to the same observer. So the comment was wrong before AND after, in opposite directions. It now says which counters are Redis-only by construction (everything sequence-related — MemoryBus assigns contiguous ids and has no subscription to lose) and which are not, and a test pins both halves. - "The three keyspaces cannot drift" survived in cmd_server.go. Round 15 fixed the copy in redisns.go and not this one — a two-member class, fixed one member, which is team CONVE-18 for the second time in this branch. - The presence-failure consequences were stated as certainties. Redis can fail a pipeline or a script AFTER it applied, so a failure means the operation reported an error, not that it did not happen. Now phrased as what a failure risks. Codex's reserved-namespace audit came back complete: the set covers every current suffix root (watchevents:pub: is covered by watchevents), every configuration path goes through Parse, all three production constructors receive the parsed value, and the exact-match controls do not reject names that merely contain a reserved word. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 04:43:14 +00:00
xarmian	9d54f24626	fix(server,cli): the half of round 12's fix I missed (BUG-2726) ... Codex round 13, unanchored, found that my previous commit fixed one of the two refusal paths on /api/v1/events. The admission check moved above the SSE headers; the PER-WORKSPACE check stayed below them, so half the 429s on that endpoint still carried the JSON error envelope under Content-Type: text/event-stream — the exact defect the commit said it fixed. Team CONVE-18 in its own shape: the reviewer named one instance, I fixed that instance, and the class had two members. The enumeration I owed was "how many ways can this handler refuse", and it takes ten seconds to read. Every refusal is now above the header block, with a line saying nothing below it refuses. The contract test made the same omission and is the reason this reached another round: it drove the admission bound on both endpoints and never the per-workspace one, so it agreed with a handler that was half fixed. It now enumerates all three refusal paths, and the mutation that reintroduces the defect fails it by name. Also from round 13: `pad project watch`'s 429 message named the two knobs that cover both streams and omitted PAD_SSE_MAX_PER_WORKSPACE, which is the one most likely to be the cause on a busy workspace — true as far as it went, and pointing the reader away from the answer. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 04:18:18 +00:00
xarmian	3e3170e915	fix(server,cli,docs): the consumer contract, per codex round 12 (BUG-2726) ... An angle no earlier round took: what does a CLIENT see. Two of the five findings were about consumers I had never opened. - `pad project watch` returned "event stream returned 429: {json}" and exited, which sends the reader looking for a bug rather than at a limit. It now says what happened and which knobs govern it, and names the fact that those knobs cover this stream and the agent watch stream together. It still exits rather than backing off — it is interactive, and a human can decide — unlike the unattended monitor, which already folds 429 into its ladder. - Both endpoints now answer a refusal through one helper: same status, same code, same message, plus `Retry-After`. `/api/v1/events` was setting `Content-Type: text/event-stream` BEFORE the admission check, so its 429 carried the JSON error envelope under an SSE content type — a different contract from its sibling's for the same refusal. Admission moved above the headers, which is where it belonged anyway. - The anonymous-caller rule was documented as if it applied to both endpoints. It applies to `/api/v1/events` only; the watch stream requires a resolved user and answers 401 without one. - docs/architecture.md described one SSE endpoint and one bus. It now has the table: two streams, two buses, different scopes and consumers, one shared connection budget, one Redis namespace. FILED, not fixed: the web UI's `EventSource` cannot see a 429 or a `Retry-After` — the spec exposes neither to the page — so a refused browser tab reconnects at a constant rate while the CLI backs off. That asymmetry means reaching the limit sheds load from the population that respects it and not from the one that grows fastest under it. No server-side change closes it; the fix is a client-side reconnect wrapper. BUG-2733, and docs/deployment.md warns operators to size the limit with it in mind. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 04:10:06 +00:00
xarmian	ec70f13608	refactor: act on codex round 8's scope review (BUG-2727) ... The "should this be here at all" angle, which my own record says I do not reliably ask of my own work. Six findings; one was a real inconsistency, the rest were claims that needed stating rather than code that needed removing. REMOVED: the presence observer's interface, adapter type and constructor, in favour of a plain callback. One method, one production consumer — and the same diff already uses bare callbacks for RedisHealth and the stream gauge, so this was inconsistent with itself. internal/watchevents keeps an interface because it reports five distinct conditions; one does not earn one. TRIMMED: .env.example's per-variable prose down to the upgrade-relevant facts plus a pointer at docs/deployment.md, which is canonical. The same policy was restated in seven artifacts and that is a drift surface. KEPT, with the reason written where a reader will ask: - The receive-loop-exit counter is expected to stay at zero, and that is what it is for — a should-never-fire alarm on a state undetectable from outside the process (an instance that publishes fine, answers health checks and receives nothing). BUG-2727 filed the silent return as the defect, and a log line nobody greps is not the same artifact as a counter somebody alerts on. - The prober's synchronous first probe duplicates cmd_server's dial-time ping. Deliberate: reusing that result would couple this type to its caller's startup sequence for one round trip that runs once per process. The consequence is now stated too — because the dial-time ping is FATAL, the prober's "unreachable at startup" branch cannot fire in the shipped binary. - The keyspace wiring guard parses source and will break on a rename. The alternative on offer needs three packages' constructors collapsed into one API. A guard that costs a one-line update after a deliberate rename beats an invariant with no enforcement, which is what the package comment alone amounts to. RAISED WITH THE LEAD, not decided here: events.EventBus.Publish's global limit parameter is now dead in production, since the handler passes 0 and the process-wide gate owns that bound. Removing it is the clean seam and it is an interface change in a shared package, which is a structural call. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 03:16:07 +00:00
xarmian	a790810bd6	docs: close the cross-artifact gaps codex round 6 found (BUG-2724, BUG-2726, BUG-2727) ... The angle no earlier round probed: which artifacts a human or agent CONSUMES should have changed and did not. Five, and the pattern is the one my own record keeps naming — the caveat existed in the artifacts I was editing and not in the ones that get read. - .env.example had neither new variable and still described PAD_SSE_MAX_CONNECTIONS with its old single-endpoint meaning. It is the file an operator copies; docs/deployment.md being right does not help someone who never opens it. - docs/deployment.md called the readiness endpoint /health/ready. The route is /api/v1/health/ready, so every instruction to go read the new redis block pointed at a 404. Corrected there and in four code comments, and the Health Check section now actually shows the three endpoints, the healthy payload, and the degraded one — it previously demonstrated only /api/v1/health, which is the build-info endpoint and says nothing about readiness. - CLAUDE.md listed /api/v1/events and not /api/v1/events/stream at all, so the endpoint this unit bounds was undocumented in the file agents read first. Added, with the limits and the 429 contract. - `pad watch --stream --help` said silence means "no workspace linked or padd unreachable". A capacity refusal now produces the same silence through the same backoff, so the help was enumerating a set that had quietly grown. - The plugin skill told agents "silence means nothing changed" — now false in the same way, and worse, because an agent repeats it to a user as though the quiet were evidence. Rewritten to say what silence does and does not prove. The plugin monitor description had the same enumeration and got the same fix. Checked rather than assumed: there are two SKILL.md files, and only the plugin copy carries a notifications section — the embedded one has no monitor guidance to correct. NOT changed, and raised with the lead instead: deploy/k8s/deployment.yaml points both probes at /api/v1/health, so the readiness endpoint is never consumed. Fixing it is right but it changes rollout behaviour for anyone using the shipped manifest (a database blip would start pulling pods from the load balancer), which is a deployment-posture call rather than part of this unit. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 02:45:53 +00:00
xarmian	9afedbe1a0	fix(server,metrics,watchevents): seven codex round-4 findings — operator and next-author angle (BUG-2727, BUG-2724) ... Round 4 read the diff as the operator of a running deployment and as the author of the next change. Five findings were claims my own text made that the code does not support, which is the failure mode this angle is for. 1. The degradation list said Redis loss costs "cross-instance activity events". It costs ALL of them: events.RedisBus.Publish logs its failure and returns without a local fan-out, so subscribers on the originating instance stop receiving too. A responder told only about cross-instance delivery would have looked elsewhere. Corrected in the health payload, both prober log lines, and the docs. 2. config.go promised that connected clients resync after a namespace change. True of the watch stream, false of the activity stream, whose cold replay buffer answers a resume as "caught up" (BUG-2731). The docs already carried the asymmetry; the comment did not, and the comment is what the next author reads. 3. Resume-detected gaps were counted nowhere. They are the only gap shape that is always USER-VISIBLE — the client gets sync_required — so an incident reading pad_watchevents_sequence_gaps_total would have missed the failure mode with the clearest symptom. New pad_watchevents_resume_gaps_total, kept separate rather than folded in because the two are diagnosed differently: one is a delivery fault, the other is any cursor this instance cannot vouch for. 4. The presence-failure metric's doc said every failure leaves sessions unlisted and untargetable. Two of the four ops fail in the OPPOSITE direction — a failed deregister leaves a dead session listed, so a push aimed at it is accepted and reaches nobody — and a generic alert on the total would send a responder the wrong way. Now documented per op, in the code and in the docs table. 5. The go-redis log bridge levels everything at WARN, and the comment justified that with "benign reconnect chatter" I had never enumerated. Enumerated now: the stream carries genuine failures, state changes and informational fallbacks with no severity attached. WARN stays — INFO would bury the dropped-message line the bridge exists for, and classifying by message TEXT would make Pad's log levels depend on go-redis's prose — and a component=go-redis field makes it routable instead. 6. internal/redisns centralizes key construction but cannot stop a future contributor wiring one bus with a different Keys than another: every package compiles, every unit test passes, and the deployment runs split across two keyspaces while looking configured. Adds a wiring drift guard that reads cmd_server.go and fails if the three constructors do not share one Parse-produced value. The rule was already written down in a package comment; this is its enforcement step. 7. The limits are per-process and the startup log, log fields and gauge Help called them "global". Renamed to per-instance / per-principal throughout, with the no-shared-counter caveat in the startup line. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 02:28:50 +00:00
xarmian	2b33184ef1	fix(metrics,watchevents,server): three codex round-1 findings (BUG-2727) ... 1. pad_redis_up was registered unconditionally, so a deployment with no Redis exported a permanent 0 — which reads as "Redis is down" to anything scraping it and would have every single-process binary alerting on a dependency it does not have. It now registers only inside the PAD_REDIS_URL branch, matching /health/ready, which already omitted its redis block on the same condition. My own field comment claimed the absent behaviour while the code did the opposite. 2. The receive loop could report a false exit during shutdown: Close cancels the context AND closes the pubsub, and Go picks between ready select cases at random. A context re-check makes the outcome independent of that. Scope stated honestly, because it is narrower than the finding implies. With the guard removed, 200 Close cycles under publish traffic produced zero false exits — and removing it AND reversing Close's ordering still produced none, because Close waits on the receive goroutine and the goroutine observes the cancelled context either way. So no test fails if these three lines are deleted, and both the code comment and the test doc say so rather than implying coverage that does not exist. It is kept as defence against a future reordering, not as a fix for observed behaviour. 3. Corrupt session entries returned a list error without incrementing the failure counter, so pad_session_presence_failures_total under-reported precisely the case an operator is least likely to find another way — a dead Redis is obvious, a corrupt row is not. Both corrupt shapes now count. The non-string arm is unreachable through MGET (Redis answers nil for a key holding a non-string value, verified), so it is annotated as defensive and the test says no leg drives it instead of quietly covering only the reachable one. Test-power notes are measured, not asserted: the Close test catches removal of the select's ctx case (mutation-verified) and does not discriminate the guard. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 01:55:59 +00:00
xarmian	0877b260c1	feat(redis): namespace every Redis keyspace from one shared config value (BUG-2724) ... Every Redis key and channel Pad uses was flat — pad:events:, pad:event_seq, pad:watchevents*, pad:session:* — so two Pad installations pointed at one Redis endpoint cross-feed each other's notifications and merge each other's session-presence registries. Different logical DB numbers do not help: Redis pub/sub is not namespaced by DB at all. The exposure is narrow but real. Delivery is filtered per caller on user id, and user ids are per-installation UUIDs, so cross-feed needs the same id in both installations — a CLONED database, such as a staging environment restored from a production dump. For that case it is a genuine cross-tenant leak: foreign sessions listed in the picker, and a private push deliverable across installations. Fixed the way internal/watchevents' existing ruling demanded: not by one package growing a prefix the others lack, but through internal/redisns — one value parsed in cmd/pad/cmd_server.go and passed into all three constructors. The three cannot drift because there is nothing to drift from, and the operator rule is stateable in one sentence for every keyspace. PAD_REDIS_NAMESPACE defaults to empty, which reproduces the historical names byte for byte, so an existing deployment keeps addressing its own replay buffers, counters and presence entries across the upgrade. Tests assert both directions per keyspace — present under the namespace AND absent under the historical names — because an implementation that wrote both would still cross-feed while passing a one-directional test. Namespaces are validated at startup, and a colon is rejected specifically: it is Pad's own separator, so namespace "a:events" would build pad:a:events:<ws> and collide with installation "a"'s channel — reintroducing the cross-feed through the mechanism meant to fix it. Names are built through a function rather than assembled from a literal at each site, and redisns' doc says why: "pad:" also begins Pad's OAuth SCOPE values (pad:read / pad:write / pad:admin) in four files, so a grep-driven prefix sweep would break authorization. Not included, deliberately: hash tags for Redis Cluster. BUG-2724's trail recommended shipping them alongside on cost-sharing grounds; that premise is falsified by publishScript, which spans four keys in one EVAL and fails CROSSSLOT exactly as presence's MGET does. There is no cheap half, and no cluster client here to exercise tagged keys against, so they would ship untested by construction. Cluster stays documented as unsupported and the future unit is named on the trail. Renaming is a CUTOVER for the buses (the seq and epoch keys carry Last-Event-ID meaning, so connected clients resync) and free for presence (90s TTL). Both stated in docs/deployment.md and at the constructors. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 01:41:31 +00:00
xarmian	720b792176	feat(server,config): bound the watch-events stream, with one budget across both SSE endpoints (BUG-2726) ... GET /api/v1/events/stream had no concurrent-connection limit of any kind. PAD_SSE_MAX_* gated only /api/v1/events, and the API rate limiter caps how FAST connections are opened, not how many are HELD — so one authenticated user could hold arbitrarily many streams, each costing a goroutine, a bus subscription and, since BUG-2698, a presence registration in shared Redis. The bound is a process-wide admission gate rather than a second per-bus limit. Each bus can bound its own subscribers atomically and events.EventBus already does, but neither can bound the two together, and a held connection costs the same process resources whichever endpoint opened it. A global limit on one bus would have let a user exhaust the machine through the other while every configured limit still read as satisfied. So PAD_SSE_MAX_CONNECTIONS now covers BOTH endpoints and is passed to the events bus as 0. That is a deliberate re-point of an existing knob, ruled rather than assumed: an operator who tuned it for one endpoint is now bounding both and may reach the limit sooner. A knob that silently bounded half the connections it named is the worse failure — invisible — where this one announces itself and is tunable. A startup log line reports the effective limits and which endpoints each covers, so the change is visible without reading release notes. New PAD_SSE_MAX_PER_USER (default 50) applies to both endpoints. The global bound alone lets one user exhaust the process for everyone, which the per-workspace limit cannot prevent — the watch stream has no workspace to count against. Per-workspace stays /api/v1/events-only for the same reason. Refusal is 429 sse_limit_exceeded, matching the existing endpoint. The CLI monitor folds any non-200 into its backoff ladder (linear, 5s base, 5min cap, reset on connect), verified rather than assumed, so a refused stream backs off instead of spinning. Deliberately NOT a registry-side cap: PR #1175 added one in review round 17 and removed it in round 21, because it bounded one of three resources a held stream consumes, was never hard (admitted renewals must bypass it), and cost delivered_sessions its honesty. The admission check is upstream of all of that — refusing costs one connection instead of making a live session untargetable. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 01:31:33 +00:00
xarmian	8dea9abca3	feat(watchevents,metrics): operational observability for the Redis notification bus (BUG-2727) ... The watch bus detects four conditions an operator would want to alert on — a notification dropped for a slow local subscriber, a gap in the received id sequence, an id-space reset, and the receive loop stopping — and until now reported all four to slog and nowhere else. Log lines are not alertable without someone already looking, and the last of the four was not even logged: the loop returned silently, leaving an instance that publishes fine and receives nothing indistinguishable from a quiet workspace. Adds watchevents.Observer, an adapter seam rather than a bus wrapper. The events.EventBus wrapper shape does not work here: every condition is detected on the RECEIVE path, inside the bus, and is invisible at the Bus interface — a wrapper can count publishes and subscribers, but not a notification that never arrived. Two corrections to BUG-2727's filing, both verified against go-redis v9.22.0 rather than assumed: - Its proposed fix — "re-subscribe rather than exiting where the cause is recoverable" — would be dead code. PubSub.Channel's message channel is closed ONLY on pool.ErrClosed; every other receive error is retried indefinitely, and a health-check goroutine pings every 3s and reconnects on failure. So go-redis already does the re-subscribing. The exit gets an ERROR log and a counter instead, which is what the condition actually needs. - The genuinely silent path is go-redis DROPPING messages when a subscription's 100-deep buffer stays full past its 60s send timeout, logged only through go-redis's own logger. Pad cannot count that directly, so it is reported by its CONSEQUENCE (a sequence gap) and its cause is made visible by routing go-redis's logger into slog. Observer's doc comment states that boundary, so a gap is not misread as evidence of any particular cause. Session presence gets the same treatment for the same reason: it is fail-soft everywhere by design, so its failures have no user-visible signal beyond a push that quietly reaches fewer sessions than it should. The renew counter is deliberately NOT throttled where its log line is — throttling the metric would make it under-report during the incident it exists for. Tests assert the CONDITION increments the counter, not that the counter exists, and each asserts its own premise first (a healthy subscriber reports nothing; contiguous ids report nothing; a cold start reports nothing) so a bus that reported on every notification could not pass. The receive-loop test drives the real closed-client condition rather than calling the reporter. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 01:21:50 +00:00
xarmian	ea139272ce	fix(server,watchevents): shared session presence + honest push acceptance (BUG-2698, BUG-2699) (#1175) ... Two coupled defects in the push path, fixed as one unit because 2699's honest-acceptance signature is the substrate 2698's fix reports through. BUG-2699 — Bus.Publish reports acceptance. The endpoint returned 200 pushed:true for a publish that was dropped, because Publish returned nothing and swallowed every failure. An error is two outcomes and they are kept apart: ErrBusClosed proves nothing was published (503 unavailable, safe to resend), while any other error means UNCONFIRMED — go-redis retries a command whose reply was lost, which is why the publish script already carries a dedupe token — and gets 502 push_unconfirmed, deliberately off the web client's safe-to-resend list. MemoryBus was the worse case, not the exempt one: neither implementation checked `closed`, and the in-process one dropped silently with no log at all. Seven production call sites, not the six the item named; the six best-effort producers discard through one named helper, and an AST-based test fails when a new producer publishes directly. BUG-2698 — RedisSessionPresence. A session-targeted push was resolved against the answering replica's presence registry, and the handler skips the publish when the target is absent, so a POST landing on A for a session held on B dropped the instruction and answered delivered_sessions:0. Fixed at the REGISTRY rather than the gate: a shared registry makes the snapshot right, which makes the picker complete and restores the gate's original premise, so the existing skip becomes correct for the reason it was written. Entry and index are written atomically under a TTL renewed by a goroutine that lives exactly as long as the connection; a crashed process stops renewing and Redis clears it. Staleness is unchanged and now stated in full: ~30s for a dropped client, ~90s for a dead instance. delivered_sessions becomes nullable — null means published-but-uncountable, never zero — documented as three states at every consumer. 35 Codex review rounds. Notable: a per-user registry cap was added and then removed after three consecutive rounds found defects inside it and a fourth was asked whether it belonged in this PR at all; a context bound was documented, disproved by its own test (go-redis does not apply a command context to connection establishment — 5.0s measured against a 150ms ctx), and rewritten to say what is true. Every fix was mutation-checked; one instrument was deleted for passing on broken code and one for not asserting its own premise. Filed rather than folded in: BUG-2724 (Redis keyspace namespacing + Cluster), BUG-2725 (delivered_sessions is an estimate with error in both directions), BUG-2726 (no concurrent-connection limit on the watch stream), BUG-2727 (Redis absent from readiness/metrics; silent subscriber loss), BUG-2728 (epoch-reset resume lead). Gates: build · make lint 0 issues · go test ./... (25 pkgs) · svelte-check 0 errors · vitest 1738 passed · CI 7/7 including Go (PostgreSQL) and Nix.	2026-08-21 20:43:20 -04:00
xarmian	6a37512227	feat(server): outbox drain — webhooks delivered from the choke point (TASK-2714) (#1173) ... * test(store): pin the events/1 taxonomy as an independent copy (TASK-2714) TestCanonicalEventsAreFullyDeclared iterated kernelevents.Canonical() and asserted each entry resolved something non-empty. That check cannot fail for any table the compiler accepts: eventSpec requires both fields, so a corrupted table — an entry deleted, an entry added, item.deleted quietly rebased onto the ref-only payload — passed its own validation. A test that agrees with whatever the table says is not a test of the table. The sixteen name/subject/family triples are now written out as literals, so the test DISAGREES with the table when the table moves. The wire strings behind the name constants are pinned separately, because the triple map is keyed on literals and a renamed constant would otherwise slip through as long as the constant and the table moved together. Ordered as this unit's first commit because TASK-2714 edits that table (the handler-path bulk mapping): an independent copy earns its keep at the moment of the edit, not before. Mutation matrix, 4/4 caught: drop member.joined (17 -> 15 count mismatch and a missing-name error), rehome item.deleted onto ref_only (family mismatch), rename ItemMoved's wire string to item.move (constant leg), add an undeclared item.frobnicated entry (count + undeclared-name + non-canonical legs). The fourth reported "survived" on its first run because the sed never matched the table's alignment — the mutation was verified present in the file before the result was believed. TASK-2714 requirement 4 (lead pass on #1172). * feat(store): max-age prune for undispatched outbox rows (TASK-2714) Requirement 3's missing half. PruneDispatchedOutbox filters on dispatched_at IS NOT NULL, so a row that can never be delivered — a workspace whose only webhook was deleted, an endpoint that 4xxs forever — is unreachable by it and keeps its frozen payload indefinitely. That matters because SPEC-3 makes payload privacy TEMPORAL. An outbox payload is a frozen snapshot and account deletion's de-identify posture reaches only live rows, so the retention window is the whole privacy claim; a window only one of its two halves can close is not a window. The trade is stated in the doc comment rather than left to be inferred: at-least-once holds WITHIN the retention window and not past it, which is why the caller's max-age must be far larger than any retry schedule. Deleting rather than stamping the rows dispatched is deliberate — a dispatched stamp would be a lie in the durable record, and this table is the only evidence of what the kernel emitted. Mutation matrix, 2/2 caught: drop the dispatched_at IS NULL clause (prunes the aged DISPATCHED row too, handing retention two owners with different windows), drop the occurred_at cutoff (prunes a young pending row a retry is still owed). The test asserts its own premise — all three seeded rows are confirmed present before the survivor checks, which would otherwise pass for a reason unrelated to the prune. No caller yet: the drain loop wires it up in the next commit. * feat(events): derive SSE names from the taxonomy; retire item.updated_with_comment (TASK-2714) SPEC-3 §"the choke point owns the canonical→surface name mapping". SSE's snake_case vocabulary and the webhook dot-form vocabulary drifted because nothing tied them together — each was hand-passed at its own call sites. This ties them. v1.5 pins what "derive" means: NAME derivation, not delivery path. SSE stays direct-published at the mutation site, because it carries request-scoped attribution (Actor / ActorName / Source) that a frozen outbox payload deliberately does not hold; only its NAME now comes from the taxonomy. Moving SSE behind the drain is TASK-2722. - eventSpec gains an `sse` field — ONE table, not a second map, for the reason round 11 of the last unit established: a separate map can disagree with the first and fails open exactly when it matters. Empty is a real value (attachment, member and pack events have no SSE surface) and SurfaceSSE reports false for it, so silence can't be mistaken for a name. - Several canonical events derive the SAME SSE name — status_changed and moved both surface as item_updated — because the SSE vocabulary is coarser than events/1 and the UI never distinguished them. The finer name is what the webhook wire and bindings get. - The 12 canonical SSE publish sites take their names from derived package vars, resolved AT INIT. Every call site is a compile-time constant, so a missing surface is a startup panic rather than a per-request decision between "log and drop" and "publish under an empty name". - handlers_item_links.go keeps the events.ItemUpdated literal, commented: link mutations are silent in events/1 (v1.5), so there is no canonical name to derive from. TASK-2723 carries link.created / link.removed. - item.updated_with_comment retired (v1.2, Dave's ruling). One producer deleted; the events.ItemUpdatedWithComment constant deleted with it — it had no producer and no web consumer (grepped .go/.ts/.svelte), so leaving it would leave a name a future publisher could reach for. The compat guard is what makes this a refactor rather than a wire change: TestDerivedSSENamesMatchTheLegacyWireVocabulary asserts each derived name equals the events.* constant clients are pinned to. A derivation producing "item.created" or "item_deleted" would break the live UI while every other Go test still passed. Mutation matrix, 3/3 caught: rename item.deleted's SSE surface to item_deleted (both the taxonomy test and the compat guard fail), split item.moved onto its own SSE name (same), make SurfaceSSE return (spec.sse, ok) so no-surface events fail open (the taxonomy test's silence leg names all four). Running total 9/9. go test ./internal/server ./internal/store ./internal/events: all green. * feat(webhooks): synchronous DeliverEvent seam with per-endpoint outcome (TASK-2714) Requirements 1 and 2. Dispatch returns once its per-hook goroutines are spawned and reports nothing, so a drain built on it would stamp rows dispatched while the HTTP requests were still in flight — losing exactly the events the outbox exists to make unlosable. DeliverEvent blocks and tallies. - Delivery carries WorkspaceID / EventID / Event / OccurredAt / Payload. OccurredAt is the EVENT's timestamp, not dispatch time: SPEC-3 pins time-relative binding predicates to it, so stamping time.Now() would make every consumer's notion of when a mutation happened depend on how backed up the queue was. Payload is json.RawMessage — []byte would base64 the snapshot into a string that is valid JSON and completely unusable. - WebhookPayload gains ID, the consumer dedupe key SPEC-3 §Delivery guarantees already told consumers to use. Before this, that instruction named a field nobody could see. omitempty, because the "webhook.test" ping is not a kernel event, has no outbox row, and must not invent an id. - DeliveryOutcome counts rather than a status, because one event fans out to N endpoints and the answers differ. Three distinctions the drain branches on: Matched==0 is SUCCESS (a webhook-less workspace is owed nothing; reading it as undelivered would back up every event in every such workspace until retention deleted it); Permanent does not hold the event pending (re-sending to an endpoint that will reject it again costs the queue its progress); Transient does. Retryable() states the ack rule once instead of letting each caller re-derive it. - A returned error is reserved for the SERVER's failures — listing hooks, marshalling. Those must not ack: nothing was attempted, so the event is still owed in full. - Dispatch keeps its async shape for its one remaining caller and says so. deliver() now returns the outcome it always computed; the async path discards it. Mutation matrix, 6/6 caught: stamp dispatch time instead of occurred_at; drop the envelope id; pass the payload as []byte (base64); deliver asynchronously and assume success (the synchronous leg names it exactly); count a permanent rejection as transient; swallow a store failure into a zero outcome (the test prints the outcome that would have acked an undelivered event). Running total 15/15. go test ./internal/webhooks green. * feat(store): batch_id correlation for handler-path bulk mutations (TASK-2714) F2's write half. A lane-wide bulk action is a handler LOOP over per-item store mutations with no enclosing transaction, so each member writes its own canonical outbox row — which is what keeps SPEC-3's per-member binding evaluation free, and also means that without a marker the drain would put 200 item.deleted events on the webhook wire for a 200-item lane archive: exactly the flood TASK-1668's batch event exists to prevent. RECORDED, NEVER INFERRED (SPEC-3 v1.5). The schema-free alternative was grouping pending rows by workspace and a time window, which would fold two unrelated single updates into somebody's bulk event whenever they landed in the same tick. A wire event saying "these five items changed together" is only true if something recorded that they did. - migrations 082 / pgmigrations 060: nullable event_outbox.batch_id, no FK (a batch is not a row anywhere, it is a name the handler minted), plus a partial index on the pending set. - store.MutationOption / WithEventBatch: variadic, because every existing call site is a single-item mutation with nothing to declare and making all of them pass a zero value would bury the one case that matters. - The handler mints one id per bulk OPERATION, before the loop and unconditionally — deciding mid-loop whether a run "counts as" a batch would make the correlation depend on how far the loop got. POPULATION CORRECTED: my escalation said four store methods; it is FIVE. archive (DeleteItem), restore (RestoreItem), move (MoveItemWithPreCheck), field update (UpdateItemWithPreCheck) and assign (UpdateItem) are the complete set of mutating store calls handlers_items_bulk.go makes — restore was the one I missed, which is CONVE-18's exact lesson arriving one level up. The test drives all five rather than sampling, because the failure is per-method: a signature that accepts the option and never threads it compiles, passes everything else, and silently un-batches one of the six bulk verbs. Mutation matrix, 5/5 caught across the four distinct emit sites: drop the stamp on the update path (both Update legs fail), on delete, on restore, on move. The delete mutation first read as SURVIVED — it had made the package fail to BUILD (opt then unused), and the grep for test-level FAIL lines printed nothing. The compiler catch is the stronger result, but the instrument mis-reported it, so it was re-run with opt kept alive and the test named it directly. go test ./internal/store ./internal/server green. * test(server): anchor the SSE compat guard to the client's literal strings (TASK-2714) The guard compared the derivation against events.* — the Go side. A coordinated rename of the taxonomy AND the constants passes that, and is exactly the change that breaks the browser: the client is pinned to the STRINGS, in web/src/lib/services/sse.svelte.ts's ITEM_EVENTS. The wanted column is now a literal copy of what the client listens for, with the file named. events.* is asserted alongside as a second leg, so a drift between the Go constants and the client is attributed rather than merely reported. Same disagree-with-the-table principle as the taxonomy test, one layer out: this file has to be edited by hand when the wire vocabulary intentionally changes, and that edit is when someone goes and changes the client too. Mutation matrix, 2/2, each hitting only its own leg: rename events.ItemCreated to the dot-form with the taxonomy untouched (drift leg fires), and make the taxonomy publish the dot-form on SSE (browser leg fires). Running total 22/22. Lead's catch on the day-49 review of commit 33662da0. * feat(store): outbox claim protocol with lease and whole-batch claiming (TASK-2714) F3. Every instance of a cloud deployment runs the drain, so an unclaimed pending row is delivered once PER INSTANCE by construction. SPEC-3 permits duplicates — consumers dedupe on the event id — but "occasionally, after a crash" and "always, once per instance" are different promises, and only the first is one a consumer can budget for. - migrations 083 / pgmigrations 061: claimed_at + claimed_by, dialect-uniform conditional UPDATE (BUG-2415's orphan-GC protocol). Postgres FOR UPDATE SKIP LOCKED plus a separate SQLite path would be two implementations of one behaviour, only one of which runs where it matters. - claimed_at doubles as the lease: an instance that dies between claiming and dispatching must not strand its rows, and at-least-once is exactly what makes re-claiming safe. - BATCHES ARE CLAIMED WHOLE, past the limit. The limit is a throughput knob; letting it split a batch would make one bulk operation arrive as two wire events each reporting a partial member count. - MarkOutboxAttemptFailed RELEASES the claim rather than letting it expire. A transient failure means the event is owed and nothing is in flight; on a single-instance deployment the lease would otherwise be the only reason a retry ever waited. THE EXCLUSIVITY TEST WAS VACUOUS AND THE MATRIX CAUGHT IT. Removing the availability predicate from the claim UPDATE left it green: the candidate query already filters held rows, so single-threaded the end state is identical (CONVE-12 — another mechanism produces it). That implementation double-claims every row two instances select in the same moment, which is the entire bug. claimOutboxIDs is now split out so a test can drive the arbiter with a deliberately STALE candidate list, and the same mutation fails it by name. Mutation matrix, 4/4: drop the UPDATE's availability predicate (survived the first test, named by the race test); drop the batch expansion; keep the claim on a failed attempt; and the vacuity finding above. Running total 26/26. go test ./internal/store green. * feat(server): the outbox drain — claim, fold, deliver, retain (TASK-2714) The half of SPEC-3's choke point that turns stored events into delivered ones. 2a built the fill side; until this, the table filled and nothing read it. NOT STARTED YET, deliberately: the hand-called dispatchWebhook sites are still in place, so wiring the loop here would double-deliver every canonical event. Starting it is the next commit, together with deleting them — the unit's behaviour edge, kept as one reviewable diff. - Two declared payload shapes for item.bulk_updated (SPEC-3 v1.6). The store-side single-tx producers know every member at write time and embed snapshots; the handler-path HEADER knows the operation, the shared delta and the member refs, with snapshots living on the members' own rows. Declared rather than loosened: stuffing placeholder snapshots to satisfy a single-shape check would be a lie in the durable record, and dropping the gate would drop it on the one event with two producers. - EmitBulkHeaderEvent + bulkEventDelta: the delta is captured where it is KNOWN. By the time the drain sees member rows they carry post-mutation snapshots, and a diff of a snapshot against nothing is not a delta. - The fold: header plus whatever member rows of that batch are still undispatched. Members whose header is not in this claim deliver individually — not a fallback, the defined behaviour for the window between the loop committing and the header landing. batch_id is on the wire so a consumer can tie the singles to the batch. - Per-unit acking: a folded batch is many rows and ONE delivery, so a partially acked batch would re-deliver. - Retention runs every tick, both halves. The undispatched one is the privacy bound; PruneDispatchedOutbox looks like it covers retention until you notice which rows it can never see. TWO REAL BUGS THE TESTS FOUND, both in this commit's own code: 1. DEFAULTS APPLIED ONLY IN StartOutboxDrain. A tick reached directly ran with a ZERO undispatched max age, making the retention cutoff `now` and deleting the entire pending set on its first pass. Every test does this, and so would any future admin-triggered drain. Fixed by construction — one resolver both entry points call — with a refusal guard behind it. 2. THE GUARD'S FIRST TEST WAS VACUOUS AND PASSED WITH THE GUARD REMOVED. RFC3339 is second-granular, so a row written in the same second as a zero-window cutoff survives `occurred_at < cutoff` either way: the end state was reachable by another mechanism, and that mechanism was the clock. runOutboxRetention now returns its refusal so the test asserts the refusal rather than the survival, plus a positive control. Mutation matrix, 7/7 after the instrument fix: ack regardless of outcome; ack only when something succeeded (permanent failures would wedge the queue); fold without acking its members; drop members that have no header instead of delivering them; remove the retention guard; remove the resolver's max-age default. Two mutations initially read as survivors — one had failed to build, one met the vacuous test — and both are recorded above rather than counted as passes. Running total 33/33. go test ./internal/server ./internal/store green. * feat(server): deliver canonical webhooks from the drain, not from the handlers (TASK-2714) The unit's behaviour edge, kept as its own commit. The drain starts, and the nine remaining hand-called dispatchWebhook sites go: comment.created, comment.updated, item.created (x2 — plain and copy), item.updated, item.deleted (x2), item.moved, item.bulk_updated. Each was verified to have an outbox producer before its deletion, not assumed to. The Server.dispatchWebhook helper goes with them — it had no production callers left. Three copy tests used it as a probe and now call s.webhooks.Dispatch directly, which is what it did. WHAT CHANGES ON THE WIRE, stated plainly because "no behaviour change" would be false here: - TIMING. Deliveries were post-commit and inline; they are now up to one drain interval (5s default) later. In exchange a delivery survives a crash: the event is committed with the mutation it describes. - THE DISJOINT-DELTA RULE ARRIVES (SPEC-3 v1.3, ruled in 2a). A bare status flip now emits item.status_changed ONLY, where the hand-call always emitted item.updated. A mixed update emits both. This was ruled while the webhook surface has no known consumers; it is the same grounding as the v1.2 fold. - PAYLOADS. The envelope gains `id` (the dedupe key SPEC-3 already told consumers to use), and `timestamp` is now the event's occurred_at rather than dispatch time. Item snapshots come from the in-transaction read-back and are PII-scrubbed — the joined assignee name and email are gone, deliberately (see scrubItemPII: a frozen payload outlives account de-identification). - item.bulk_updated carries batch_id, the shared delta, and the member snapshots folded in from the member rows. Two copy tests needed real changes, not cosmetic ones: the DR-14 emission matrix they assert (which workspace hears what) is unchanged, but nothing arrives until a drain pass runs, and the fixture's own backlog — member joins, filler items — would otherwise be reported as the copy's output. The observer now drains once before the receivers are registered, which is what its "baseline" has always meant, and drainWebhooks runs a pass before collecting. go test ./internal/server ./internal/store ./internal/webhooks green. * fix: codex round 1 — unbatched bulk verbs, member dedup, comment overclaims (TASK-2714) THE P1, and it is CONVE-18 for the third time in this unit: batchID was threaded into the bulk helpers' SIGNATURES but not passed at three of the six store CALLS (set-priority/move-status via UpdateItemWithPreCheck, tag/untag via UpdateItem, move via MoveItemWithPreCheck). Those verbs' member rows stayed unbatched while the header was still written — N individual wire deliveries plus a header claiming they were a batch. My store-level test could not see it. It called the five store methods directly with the option, so it proved the option WORKS and said nothing about whether the handler passes it. TestBulkItems_EveryVerbStampsOneBatchID drives all seven legs through the HTTP handler and asserts every row of the operation shares one non-empty batch id with exactly one header. Reverting one stamp fails it by name (set-priority and move-status both). Writing that test also surfaced two legs that asserted nothing: untag and assign were no-ops in the fixture (no such tag; nothing assigned), so no member events existed at all. Both now perform real mutations, and the leg fails if fewer than two rows appear. Also from round 1: - FOLD DEDUPS MEMBERS. The disjoint-delta rule means one member can write two or three rows (a move that also changes status emits item.moved AND item.status_changed), so the folded payload listed the same item repeatedly while `count` reported ITEMS — the wire event contradicting itself. Keeps the LAST snapshot per id; an unreadable snapshot is kept rather than dropped. - BULK MOVE DELTA carries both collection and status when both were sent. bulkMoveCollection applies req.Status as a field override, so a move-with-status changes two things. - FIVE COMMENT OVERCLAIMS, all mine or inherited and all now matching the code: the taxonomy package doc still said nothing drains the outbox; "every event produces exactly one payload shape" predates the batch event's second shape; two places said "the dispatcher runs item-level selectors against each member snapshot" when no binding engine exists and the dispatcher filters on event NAME only; my own retirement comment said this path emits "item.updated + comment.created" transactionally, when the item half is whichever slice moved (a status-only update emits status_changed) and the comment is a separate transaction; migration 083 described the claim as one statement doing both the select and the mark. One finding recorded rather than fixed: affectedIDs counts rows TOUCHED, not rows semantically changed, so an all-no-op operation writes a header with a count and no members. Verified against origin/main — the webhook this replaces fired on the identical condition with the identical count, so it is inherited, and narrowing it is a wire change to count/item_ids that belongs with a contract version rather than a delivery refactor. Gates: build clean, make lint 0 issues, go test ./internal/... green, make test-pg exit 0 / 3463 PASS / 0 FAIL with the new outbox tests verified present in the Postgres run. * fix: codex round 2 — batch correlation on the wire, prior_status survival (TASK-2714) Round 2 was aimed at round 1's own fixes, and that is where both P1s were. - BATCH_ID REACHED ONLY THE FOLDED HALF. A member delivered individually — the window this whole design accounts for — carried an item snapshot with no batch anywhere in it, while three comments claimed consumers could correlate the singles with the batch. They could not. batch_id is now an ENVELOPE field on every delivery of a batched event, singles included, which is the only place a consumer can read it for a member. - FOLD DEDUP COULD DROP prior_status. A mixed update writes item.status_changed (carrying the transition) and item.updated (not); round 1's last-wins kept the later row and silently lost the one field a "nonterminal → terminal" binding needs, in exactly the case that produces both rows. The snapshot is still last-wins — every field IS fresher on the later row — but prior_status is carried forward, because it is envelope metadata only one of the two events ever has. - Sibling scans deduped: a 100-row candidate slice from one batch ran the same query 100 times. - The "only when something actually changed" comment on the bulk emission condition is corrected rather than left to be re-derived: the condition is that a row was TOUCHED without erroring. Untagging a tag nobody has succeeds on every row and changes nothing, so the header fires with a count while the store writes no member events. Same inherited asymmetry round 1 recorded; now the comment says it where the code is. - MY OWN COUNTS WERE WRONG IN THREE PLACES, which is the number-discipline lesson landing on documentation instead of a report: the handler test said "six verbs" while driving seven legs and "three of the six verbs" for what was three CALL SITES across four verbs; the store test implied it covered the verbs when it covers entry points, and now says out loud that it is not sufficient alone — round 1's bug lived one layer above it. Mutations, 2/2 on the new fixes: deliver singles with an empty batch id (the member leg names it twice, once per member); revert the dedupe to plain last-wins (the prior_status leg names it). go test ./internal/... green. * fix: codex round 3 — ack and release are conditioned on the claim (TASK-2714) The P1, and it is round 2's area again: claim tokens were minted and never checked. MarkOutboxDispatched and MarkOutboxAttemptFailed matched on the row id alone, so once a lease expired, a slow pass could still reach rows a newer pass legitimately owned — a late ack stamping a row the new holder is mid-delivery on, and a late release CLEARING a live claim and handing the event to a third pass. Reachable, not theoretical: a workspace's endpoints are delivered sequentially, each with three attempts and a 10s timeout, and the "well under a minute" estimate behind the lease default is an estimate rather than a bound. Both writes now carry the token and condition on claimed_by, and an empty token is refused outright rather than matching NULL. OutboxEvent carries ClaimToken so the drain never has to track it separately. A stale ack matches zero rows, which is exactly right — the event has become the new claim's problem. Also round 3, all P3: - The fold's "embedded VERBATIM" claim now names its one exception: a deduped survivor is re-encoded to carry prior_status across. Non-duplicate members are untouched bytes. - Four stale comments corrected where they live, not just where they were introduced: migration 081 still said nothing drained the table and webhooks fired from hand-calls; createItemChecked's summary still ended in "webhook dispatch"; handlers_watch_notify still called publishBulkItemsEvent "the SSE/webhook bulk path"; and two copies of the PII rationale said nothing drains or prunes, when the window is now bounded (bounded is not zero, which is why the scrub still does the work). Mutations, 2/2: drop claimed_by from the ack (the stale-ack leg fires), drop it from the release (the stale-release leg fires, naming the instance that took the freed row). Both mutations were verified present in the file first — the initial pair silently failed to apply and reported green, which is the third instrument mis-report this unit. Gates: go test ./internal/... green, make lint 0 issues. * fix: codex round 4 — retention spares live claims, token refusal is unconditional (TASK-2714) - RETENTION COULD DELETE A ROW MID-DELIVERY. Every instance runs retention, so one instance's prune could remove an old undispatched row another instance was actively delivering: the delivery would succeed while the ack matched zero rows, and a crash in that window loses an event the outbox had already committed. Live claims are now exempt, using the same lease predicate the claim itself uses. An EXPIRED claim stays fair game — that is what expiry means — and the test asserts both directions. - THE EMPTY-TOKEN REFUSAL SAT BEHIND THE EMPTY-ID SHORT-CIRCUIT, so MarkOutboxDispatched("", nil) returned nil: a contract that depended on the argument it was not about. Token check first. - The taxonomy comment claimed per-member events for ALL bulk mutations. True only of the handler path; the store-side single-transaction producers have no loop, and for those the snapshots INSIDE the payload are the only per-member view there is. Both mechanisms now named, since the distinction is visible in the payloads. - ListPendingOutboxEvents is documented as the diagnostic reader. The drain claims; a reader finding two pending-row queries should not have to guess which one production uses. Mutation, 1/1: drop the claim predicate from the prune (the new test names the count). Verified applied before the result was read. Round 4 also found a REGRESSION I am not fixing here because it is a fork: create-with-parent webhooks carry a pre-link snapshot. CreateItem writes the item.created row in its own transaction, SetParentLink runs in a separate one, and main's hand-called webhook dispatched the RE-READ item — so the parent and the post-link seq were visible then and are not now. Escalated with a recommendation (emit item.updated from SetParentLink's own transaction, which also covers the general case); it sits close enough to SPEC-3 v1.5's link-silence ruling that it is not mine to infer. go test ./internal/... green. * fix: SetParentLink emits item.updated on its own transaction (TASK-2714) Codex round 4's regression, ruled (a) by the lead with the F4 boundary made mechanical rather than inferred (SPEC-3 v1.6): a mutation that writes the ITEM'S OWN ROW emits item.updated; a relationship-graph link, which writes only the links table, stays silent. A parent link advances seq and flips the is_unparented bit, so it is on the emitting side of that line. The regression it closes: createItemChecked calls SetParentLink AFTER CreateItem has already committed item.created with a pre-link snapshot, then re-reads the item for its response. Main's hand-called webhook dispatched that re-read, so a consumer saw the parent and the post-link seq; under the drain the frozen created row was all there was, with nothing to correct it. created(pre-link) then updated(post-link) is a true history. Placed in setParentLinkOnce, not in the shared setParentLinkTx: UpdateItemWithParentLink reuses that core inside the item-update transaction and already emits from the field diff, so the shared site would double-emit. The snapshot comes from getItemTx, and the test enforces that rather than the comment doing it alone — mutating the read to the pool's GetItem fails on the seq assertion, because a different connection cannot see the uncommitted write and would emit the pre-link row under a post-link event. Mutations, 2/2: delete the emit (no event after linking); read the snapshot from the pool (seq is the create's). Plus a control leg asserting the PARENT emits nothing — the link does not write its row. go test ./internal/... green. * fix: codex round 5 — parent-only updates emit; the parent-emit claim is narrowed to the truth (TASK-2714) Round 5 aimed at round 4's own fix and found two P1s in it. Four-for-four on that angle now. 1. THE PARENT-ONLY UPDATE PATH STILL EMITTED NOTHING. SetParentLink's fix covers its own transaction; UpdateItemWithParentLink writes the hierarchy inside the ITEM-UPDATE transaction and emits from a snapshot DIFF — and a parent write leaves nothing in a snapshot to diff, since items.parent_id is legacy and untouched, the link lives in its own table, and seq/updated_at are excluded as metadata. So a fields_patch carrying only `parent` mutated the row and emitted zero events. The emitter now takes hierarchyChanged from the caller, which knows what it wrote; the diff cannot see it and must not have to. Covers set AND clear, with a control leg asserting a genuinely empty update still emits nothing — without it the fix could be "always emit", which would undo the disjoint-delta rule. 2. MY OWN ROUND-4 COMMENT AND TEST OVERCLAIMED. Both said the event carries a "post-link snapshot"; the payload is the item ROW, and the parent EDGE is not on it — IsUnparented is populated only by the local-first index queries, so the test's is_unparented assertion passed VACUOUSLY against an absent field. What the emit actually restores is the row change (a fresh seq and updated_at), which is exactly what main's hand-called webhook carried: it dispatched the handler's post-link re-read, the same scan. The comment now says that, and the test asserts the ABSENCE so the next reader cannot infer linkage data that has never been on this wire. Third instance this unit of the same shape: a partial verification written up as a complete one. Also round 5, both comment-level: - handlers_item_links.go said item-link mutations are silent in events/1. True per link TYPE, not per handler: parent crosses the "writes the item's own row" line and emits, blocks/blocked-by and implements do not. The comment now states the criterion and names the consequence — this handler publishes SSE for both kinds, so the SSE and events/1 pictures deliberately differ. - EmitBulkHeaderEvent's guard said "a bulk operation that changed nothing is not an event" while being an empty-LIST guard. Corrected in place with the reason it stays: the webhook it replaced fired on the identical condition with the identical count, so narrowing it is a wire change for a contract version, not a fix. Mutation, 1/1: drop the hierarchy force (the parent-only leg fails by name). Verified applied before the result was read. go test ./internal/... green. * fix: codex round 6 — parent DETACH emits on every route (TASK-2714) Round 6 aimed at round 5's fix and found two more in it. Five-for-five. 1. DETACH WAS SILENT ON TWO OF THREE ROUTES. Attach emitted from SetParentLink and from the update path, but ClearParentLink (its own transaction) and DeleteItemLink on a parent row (what DELETE /links/{id} actually calls) wrote the item's row and emitted nothing. A consumer's model would keep a parent the user had removed, with every attach route observable — the worst shape for this kind of gap, because the wire looks healthy. Routes are now enumerated in one test rather than sampled. 2. hierarchyChanged MEANT "PROVIDED", NOT "CHANGED". Clearing an already-unparented item deletes zero rows; round 5's flag still forced item.updated, putting an event on a public wire for a mutation that did not happen. clearParentLinkTx now reports whether it removed a link and the flag comes from that. The set branch stays unconditional — it is a DELETE-then-INSERT and bumps the row either way. IMPLEMENTS IS FLAGGED, NOT DECIDED. It bumps the same row (so the v1.6 mechanical criterion would include it) but it is a relationship-graph link (so v1.5's silence would exclude it). The contract does not resolve that case, and inventing an answer inside a delivery refactor is how a public wire acquires an event nobody ruled on. Recorded at the call site and raised with the lead. Mutations, 2/2: drop the parent-detach emit from DeleteItemLink (the route's leg fails); treat provided as changed on the clear branch (the no-op leg fails by name). The second mutation first failed to BUILD — `removed` then unused, which is the compiler catching it and my grep reading the empty result as a pass — so it was re-run with the variable kept alive. Fourth instrument mis-report this unit; all four are on the record. Gates: go test ./internal/... green, make lint 0 issues, make test-pg exit 0 on the pre-round-6 tip (re-run pending on the final tip). * fix: codex round 7 — the batch delta matches the mutation (TASK-2714) Round 7 returned no P1s; the parent-detach work from round 6 came back clean on transaction scope, lock ordering, error paths and duplicate emissions. - BULK DELTA REPORTED THE REQUEST, NOT THE COMMIT. bulkEventDelta echoed raw request values while the mutation normalizes: bulkTagUpdate trims added tags and skips ones that go empty, and the store's assignment SET clause gives a NON-EMPTY id precedence over the clear flag (BUG-2566). So a request with both an id and clear=true announced a clear while the row was assigned — the delta describing the opposite of what committed. This is the one field of the batch payload the drain cannot derive, so nothing downstream corrects it: whatever it says is what a consumer believes. Now normalized the same way, including untag matching RAW because the mutation removes by exact match. - THE implements COMMENT WAS WRONG, and it was mine from round 5: it listed implements with the silent link types when implements DOES bump the source row, exactly as parent does. Corrected in both places, and the case is stated as UNRESOLVED rather than settled — the mechanical criterion (writes the item's row) would have it emit, v1.5's relationship-link silence would not, and deciding it inside a delivery refactor would put an event nobody ruled on onto a public wire. With the lead. Mutations, 2/2: give the clear flag precedence over a non-empty id (the precedence leg fails, naming the row it would misdescribe); stop trimming added tags (the trim leg fails). The first mutation initially failed to build and was rewritten to compile before its result was believed. Gates on the round-6 tip: go test ./internal/... green, make lint 0 issues, make test-pg exit 0 / 3445 PASS / 0 FAIL with 13 of this unit's new test legs verified present in the Postgres output. Re-run pending on the final tip. * fix: codex round 8 — tag delta dedups, link SSE name derives (TASK-2714) Two P2s, both small and both the same shape: a claim in a comment that the code did not quite meet. - THE TAG DELTA TRIMMED BUT DID NOT DEDUPE, while bulkTagUpdate does both — it skips a tag already in its `seen` set. So tag ["foo", " foo "] added one tag and advertised two, under a comment saying the delta is normalized "the same way the mutation does". Round 7 fixed half of that sentence; this fixes the other half. - handlers_item_links.go PUBLISHED UNDER THE events.ItemUpdated LITERAL. The wire value happens to match, which is exactly why it was worth changing: it recreates the drift the central mapping exists to prevent, one rename away from being wrong. The name now derives like every other SSE site, and the comment separates the two facts a reader has to keep apart — the NAME derives, the events/1 EVENT still does not exist for relationship links. Mutation, 1/1: drop the `seen` check from the delta's tag loop (the dedup test names the duplicated value). go test ./internal/... green. * fix: codex round 9 — untag delta dedups, version restore derives its SSE name (TASK-2714) Both are the same shape as round 8's, one layer further out. - THE UNTAG DELTA STILL ECHOED DUPLICATES. bulkTagUpdate builds a removal SET, so ["foo","foo"] removes one tag; the delta advertised two. The two verbs normalize DIFFERENTLY — tag trims and dedups, untag dedups but matches raw, because removal is by exact string — and the delta now mirrors each side's own rule rather than applying one of them to both. - handlers_item_versions.go PUBLISHED A RAW "item_updated" STRING. A second source of SSE vocabulary, and the harder kind to find: it does not even reference the events package, so a grep for events.ItemUpdated misses it. Now derived like every other site. go test ./internal/... green. Gates on the round-8 tip: make test-pg exit 0, 3447 PASS, 0 FAIL, with 150 lines of this unit's own test legs verified present in the Postgres output.	2026-08-20 23:33:26 -04:00
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
David Barkhausen	5784d907c0	feat(cli): PAD_TOKEN environment override for stored credentials (#879) (#1160) ... * feat(cli): PAD_TOKEN environment override for stored credentials (#879) Layer 1 of #879: if PAD_TOKEN is set, the CLI uses it as the bearer token and skips the credential-store lookup — gh's GH_TOKEN convention. Reads never write credentials.json, so a read-only override sidesteps the multi-agent identity contention completely; the store is never touched under the override. Per the acceptance grounding notes: - NewClientFromURL resolves PAD_TOKEN before the per-server store lookup (the single token-attachment chokepoint). - whoami no longer lies under the override: it skips the store short-circuit and reports the effective identity via a real /me fetch, with an 'Auth: PAD_TOKEN environment override' line. - auth login/logout print a gh-style stderr notice when the override is active. logout additionally pins its server-side session invalidation to the STORED token — an unpinned Logout() after the constructor change would have invalidated the env token's session — and skips the server call when there is no stored session. - pad init's status line and server info's report disclose the override (env_token_override field; the auth probe uses the token every other command would use). Zero behaviour change when PAD_TOKEN is unset. Token minting stays web-only; a minimal 'pad token' CLI is offered as a follow-up. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(cli): review round 1 — init fails on a rejected PAD_TOKEN; login shortcut skipped under the override; logout asymmetry documented Per the PR #1160 round-1 review: - Bug 1: pad init's auth step no longer falls back to stored credentials when a set PAD_TOKEN is rejected — it fails with the distinct rejected-token message (mirroring whoami), which also makes the status line's override disclosure truthful. Test drives the real padInitCmd flow and asserts the stored identity is never consulted. - Bug 2: login's 'Already logged in as <stored user>' shortcut is skipped when the override is active — it reads the store, and firing it right after envTokenNotice contradicted the notice. A second test pins the unchanged no-override shortcut behaviour. - Doc ask: the deliberate logout asymmetry (the env token's own session is never invalidated; its lifecycle belongs to the minter, GH_TOKEN posture) is now stated in env_token.go's doc comment and the README PAD_TOKEN section. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>	2026-08-20 07:54:13 -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
杨成锴	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	052c971785	feat(plugin): consent-gated push monitor + tri-state arm/disarm + envelope (PLAN-2613 S3, TASK-2618) (#1150) ... * feat(plugin): consent-gated push monitor + tri-state arm/disarm + envelope (PLAN-2613 S3, TASK-2618) The plugin layer of the push-consent gate. S2 built the CLI arm/disarm/status verbs and the arm-state file; S3 makes the monitor existence itself the gate (D1) and adds the tri-state, the envelope, and the connect ritual. - Tri-state arm-state file: a session can be explicitly ARMED, explicitly DISARMED, or absent. `pad session disarm` now writes a session-scoped OFF marker (not a file removal), so a within-session disconnect wins even in an auto_arm=true repo — the disconnect verb must not be a lie there. The marker dies with the session (same liveness), so across sessions auto_arm remains the standing contract. ResolveAnnouncedArmed folds the tri-state over auto_arm; the monitor announces its result. - Gated monitors (monitors.json): the single always-on monitor is replaced by two — an `always` auto-arm monitor and an `on-skill-invoke:connect` manual monitor — both running scripts/pad-monitor.sh. The wrapper gates on a new hidden `pad session should-arm`, dedupes concurrent monitors with a liveness-aware per-session lockfile, and carries the reconnect loop so an in-session disarm stops the stream on its next reconnect. No consent → the monitor exits → nothing listening. - D5 envelope: a push notification carries the verbatim direction-with-authority framing (confirm in-session before anything destructive/irreversible); item- change kinds stay a light informational label. - /pad:connect + /pad:disconnect skills; /pad:status gains a one-line connection header from `pad session status`. /pad:connect runs the workspace's on-session-start playbooks on the first connect only (D8), tracked by a Booted flag carried forward across arm/disarm. plugin 0.2.1 → 0.3.0. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(plugin): address Codex R1 on S3 (disarm stops active stream, fail-closed local state) - HIGH-1: a within-session disarm now stops an ACTIVE stream, not just the next reconnect. The monitor re-checks consent every 2s while streaming and cancels the connection when it flips to not-armed, then exits (D1's whole- stream-behind-consent gate at the top of the loop), so the plugin wrapper keeps it dead. Fixes /pad:disconnect being a lie for an idle SSE that might never naturally reconnect. - HIGH-2: a corrupt/unreadable local arm-state file now fails CLOSED (LocalArmError -> not armed) instead of falling through to auto_arm, so a corrupted disarm marker can't silently re-arm an auto_arm repo. It is not reaped (reaping would re-arm on the next read); it is session-keyed and a re-arm overwrites it. - Shell wrapper: an empty (mid-startup) lock pid is treated as live so two monitors can't both steal the lock; INT/TERM now exit (a trap otherwise resumes the loop and reconnects without a lock). - Docs: plugin/skills/pad describes the new push-envelope line format; connect/status skills distinguish "consent set (armed)" from the server's observed connection counts rather than claiming "Connected". Bounded/safe-direction residuals documented in code: the reap TOCTOU and the Booted carry-forward race (both fail-closed / benign), and lock pid-reuse (dedupe only, fails toward not-streaming). Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(plugin): address Codex R2 on S3 (disarm-watcher timing, semantic corruption fail-closed) - HIGH-1: the disarm-watcher now starts BEFORE the connection is opened, so a disarm during connection/header negotiation cancels the request too (the request is built on streamCtx). streamWatchEvents also re-checks consent before delivering each notification and stops the stream if it was withdrawn, so no push is printed after a disarm even within the poll window. - HIGH-2: a syntactically-valid but semantically-garbage arm-state file (e.g. {} or {"pid":1}) now fails CLOSED via a well-formedness check (StartedAt + PID must be present, as our writer always stamps them) before liveness or reaping — so it can't be judged owner-dead, reaped, and re-armed through auto_arm, nor mistaken for a live headless arm naming init. - LOW: the cleanup trap uses condition 0 (portable) rather than the EXIT name. The disconnect skill note reflects the ~2s active-stream drop. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(plugin): /pad:disconnect always disarms, never gated on a linked workspace (Codex R3) Consent is session-scoped (keyed by the messaging socket, not the workspace), so a session that connected in one repo must be able to disconnect from anywhere — including a directory with no .pad.toml. The old precondition let a session move to an unlinked directory, "disconnect", and keep receiving pushes. Verified: `pad session disarm` from an unlinked cwd disarms the socket-keyed session state; should-arm then reports not-armed back in the original repo. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(cli): enforce the Armed != Disarmed writer invariant in arm-state validation (Codex R4) armStateWellFormed checked only StartedAt + PID, so a well-stamped file that violated the writer invariant — both armed and disarmed false (or both true) — passed validation and, since SessionArmState only branches on Disarmed, resolved to LocalArmOn and armed. The writer always sets exactly one of the two; require it, so a neither/both file fails closed (LocalArmError). Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V	2026-08-18 00:24:19 -04:00
xarmian	e40df6b31c	feat(cli): pad session arm/disarm/status + consent config resolution (PLAN-2613 S2, TASK-2617) (#1149) ... * feat(cli): pad session arm/disarm/status + consent config resolution (PLAN-2613 S2, TASK-2617) The S2 CLI contract S3's plugin skills and S4's web composer build against. S1 gated push delivery on a server-side armed bit declared at stream connect; nothing decided WHETHER to arm or sent the declaration. S2 adds both, defaulting off everywhere. - ResolveAutoArm (internal/cli/arm_consent.go): pure consent resolver. .pad.toml [push] auto_arm is the only per-repo enabler (D4); a per-user config auto_arm=false vetoes it (deny-wins); default off. Config surfaces: PadToml.Push.AutoArm + config.Config.Push.AutoArm (*bool, unset != false), both nil-safe. - Wire contract: StreamSessionIdentity.Armed sends ?armed=true on the event stream — S1's server gate finally has a sender. The monitor announces armed = live local arm OR resolved auto_arm, so a repo opt-in works end to end with a safe default-off skew. - Verbs pad session arm/disarm/status: arm/disarm manage a per-session local arm-state file; status reports the resolved local/auto decision plus the server's own armed/connected counts (new Client.ListSessions), degrading gracefully when padd is unreachable. - Arm-state file (session_arm_state.go): keyed per session by CLAUDE_CODE_MESSAGING_SOCKET (cwd fallback for headless, secondary to auto_arm). Mandatory liveness — a dead-owner file (socket vanished / pid gone) reads as disarmed and is reaped, so a crashed session can never arm a future monitor. Local client state only; the server's armed bit stays the sole delivery authority. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(cli): address Codex R1 on push-consent (fail-closed config, owner-identity liveness) - HIGH-1: user config.toml read now fails CLOSED. config.LoadPushConfigAutoArm reads the [push] auto_arm value strictly — absent → no opinion, but present-but-unparseable → error — and ResolveAutoArmFromDisk refuses to auto-arm when it can't confirm the user's veto (was: swallowed by the lenient config.Load and treated as no-opinion). - HIGH-2: arm-state liveness now checks owner IDENTITY, not just presence. Socket-keyed files record the socket's mtime and require an exact match, so a reused socket path can't revive a stale file. Headless files record a Linux /proc start-time token (portable fallback documented) to reject a reused pid. - MED-1: arm-state writes are atomic (temp + rename) and reaping is non-destructive (re-checks staleness before removing) — a concurrent re-arm is never clobbered. - MED-2: pad session status applies the .pad.toml URL override, so it queries the same server the monitor connects to. - LOW: malformed arm-state files are now reaped (safe now that writes are atomic — a corrupt file can't be a torn in-progress write). Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(cli): address Codex R2 on push-consent (atomic config write, stronger owner identity) - HIGH-1: Config.Save() is now atomic (temp + rename), so a monitor reconnecting while `pad configure` rewrites config.toml can't read a truncated/partial file, miss a [push] auto_arm=false veto, and arm. - finding 2: socket owner identity now uses inode+device (unix) as the primary signal, with mtime as the non-unix fallback — a rebound socket or a lingering stale node at the same path gets a new inode and is rejected, closing the mtime-collision / reused-node gaps. - finding 3: headless liveness fails closed when a proc-start token was recorded but can't be re-verified (was: fell back to bare pid-liveness, which a reused pid passes); zombies (state 'Z') now report not-alive. - finding 5: `pad session status` applies an explicit --url override too, not just the .pad.toml one. - finding 4 (connect-time TOCTOU): documented as an accepted, bounded residual — a disarm racing an in-flight connect is corrected on the next reconnect; fully closing it needs S3's server-side disarm-on-open signal. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V	2026-08-17 22:44:31 -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	9c155ac185	fix(cli): gate promptAndBootstrap on canPromptForConfig() (BUG-2597) (#1119) ... Third member of the BUG-2577 family (offerSkillInstall #1111, installInteractive #1116): promptAndBootstrap — the legacy --cli-prompt admin bootstrap — guarded its prompts on stdin-only term.IsTerminal, so a pty-backed harness with a redirected stdout got " Email: " printed into the pipe and then blocked on the read. Swap to canPromptForConfig() (stdin AND stdout) with the family's boundary comment; the BUG-988 refuse-with-headless-hint behavior is unchanged. The error message no longer blames stdin specifically ("not running in an interactive terminal") since the widened gate can fire when stdin IS a terminal and stdout isn't; the existing non-TTY test's assertion updated to match. Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM	2026-08-16 00:00:48 -04:00
xarmian	4a2c4c1a39	fix(cli): suppress pad agent install's dangling (Y/n) prompt in non-interactive contexts (BUG-2593) (#1116) ... installInteractive gated its prompt on cli.IsTerminal() (stdin only), so a pty-backed harness whose stdin looks like a char device — with nobody able to answer — got "Install /pad skill for all N? (Y/n): " printed and then hung at readChoice. Same shape and same fix as offerSkillInstall's BUG-2577 (PR #1111): swap to canPromptForConfig() (stdin AND stdout), document the both-pty undetectable boundary, keep the auto-install behavior unchanged. Test mirrors #1111's offerSkillInstall test and pins the closed-stdin no-prompt path; the discriminating pty-stdin case is live-verified on the trail (pre-fix binary prints the prompt and hangs to a 10s kill, fixed binary installs silently and exits 0 — identical undriven-pty harness). Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM	2026-08-15 22:04:00 -04:00
xarmian	2580c2c8bb	fix(cli): gate pad init's Step-4 login on canPromptForConfig() (BUG-2592) (#1115) ... * fix(cli): gate pad init's Step-4 login on canPromptForConfig() (BUG-2592) A configured-but-unauthenticated non-interactive `pad init` fell into doBrowserLogin and blocked on the poll wait (wall-clock-bounded since BUG-2572, still minutes of hang nobody can complete) instead of failing fast — Step 3 has had this exact gate since init.go:205, and cmd_workspace.go got it in PR #1111 (BUG-2538). The gate sits AFTER the saved-credentials check so a headless run with valid stored credentials proceeds untouched. Remedy text per the corrected trail ruling (the r1 constraint was refuted by r2): piped `pad auth login --interactive` IS a working non-interactive login (doInteractiveLogin reads a plain bufio.Reader, piped-bytes-safe since BUG-1886), so the message points there — and deliberately not at pad init's --email/--name/--password, which only fire when SetupRequired. Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM * docs(plugin): pad init no longer hangs in the session-expired case — update the three claims + plugin 0.2.1 The BUG-2592 gate makes three plugin-skill passages stale (same shape as PR #1111's codex r3 self-invalidation): capture and onboard said `pad init` can still hang on the browser flow when configured-but- unauthenticated, and the pad skill's whoami-guidance said the same at its "not a safer probe" sentence. All three now state the fixed truth, live-verified this session: fixed binary fails fast in 0.1s with the piped-login remedy; pre-fix control binary hangs to the timeout kill in the identical sandbox state; the remedy itself (piped `pad auth login --interactive`) logs in and restores credentials. Plugin 0.2.1 — text reaches nobody without a bump (version-pinned at install). Claude-Session: https://claude.ai/code/session_018qREYgDd6Ag1X1SDmqhyFM	2026-08-15 21:33:38 -04:00
xarmian	ef903f0b22	feat(cli,mcp): --clear-parent / clear_parent to detach an item's parent (BUG-2078) (#1113) ... * feat(cli,mcp): add --clear-parent / clear_parent to detach an item's parent (BUG-2078) The server has honoured a present-but-empty "parent" key in fields_patch as "clear the link" since BUG-2013, but neither the CLI (--parent "" silently no-ops) nor MCP (parent is a plain string with the usual "empty means not provided" convention) could reach it. Mirrors the clear_assigned_user/clear_agent_role shape from IDEA-2584: a boolean that carries its destructive meaning in its name and survives the trip to local stdio MCP via BuildCLIArgs' snake_case-to-flag mapping. Bumps ToolSurfaceVersion 0.18 -> 0.19 and updates the drift-pinned docs (instructions.md, README.md) accordingly. * test(cli,mcp): cover --clear-parent / clear_parent on both transports (BUG-2078) CLI: --clear-parent sends fields_patch{"parent":""}; is absent when not passed; conflicts with --parent and refuses without issuing a PATCH; item create pins the deliberate create/update asymmetry. MCP: clear_parent detaches through the real store+server (not a recording handler) so the assertion is "item ends up unparented", not just "payload shaped correctly"; clear_parent=false is inert; a plain empty `parent` string stays a no-op (control leg); a simultaneous parent + clear_parent is refused via both the direct param and the --field-lifted route. * fix(cli,mcp): close --clear-parent bypass via --field parent/plan aliases (BUG-2078, codex r1 P1) extractParentLink (internal/server/handlers_items.go) resolves the parent link from either a "parent" or a "plan" key in fields_patch, with no early exit, so the later key in its own loop wins. The clear_parent conflict check only covered one path each on the two client surfaces: - CLI: the check ran BEFORE the --field overlay and only compared against --parent's own value, so `--clear-parent --field parent=X` (or `--field plan=X`) reached the wire unrejected — the --field loop ran after clearParent's own `patch["parent"] = ""` and silently overwrote it. - MCP HTTP dispatcher: the check ran after the --field overlay (correct ordering) but only inspected `patch["parent"]`, missing the "plan" alias route. Both surfaces now run the clear_parent check after every patch-building step (named flags, --field overlay, column lift) and check both "parent" and "plan" for a competing non-empty value. * fix(cli,mcp): refuse --clear-parent/clear_parent when schema shadows "parent"/"plan" (BUG-2078, codex r2 #2) extractParentLink (internal/server/handlers_items.go ~L606-610) is a pre-existing, deliberate policy: it skips hierarchy handling entirely when a collection's schema declares its own field literally named "parent" or "plan", letting the value fall through as an ordinary field write instead. Once {"parent":""} reaches the server it can no longer distinguish clear-hierarchy intent from a legitimate blank-my-schema-field write, so a client-side clear_parent request against a shadowed collection used to report success while silently blanking the data field AND leaving the real hierarchy link untouched -- reproduced empirically before this guard existed. The ambiguity is created at the surface that accepted the clear request, so that surface refuses rather than pushing the decision server-side (server-side refusal would also break legitimate blanking of a real schema field). CLI: the check is free -- collSchema is already fetched for --field type parsing whenever any field change (including a bare --clear-parent) happens. MCP HTTP dispatcher: adds one conditional collection lookup, paid only when clear_parent=true -- the common update path fetches no schema today and doesn't start. * docs: sync repo CLAUDE.md tool-surface contract to v0.19 (BUG-2078, codex r3 P2) CLAUDE.md's MCP tool-surface prose still said "currently v0.18" and its changelog omitted clear_parent -- a consumed-artifact gap, same rule as the SKILL.md case: the doc a diff invalidates ships with the diff. Synced three spots (intro paragraph, Tools bullet, ToolSurfaceVersion stability-contract changelog) to v0.19, matching internal/mcp/version.go's in-code entry's wording, plus the schema-shadow refusal (BUG-2078's second follow-up commit) at the same level of detail the changelog already gives the parent/plan alias conflict-refusal. Grepped the rest of CLAUDE.md for any other 0.18/tool-surface reference -- none found outside these three lines. * docs: add schema-shadow refusal to version.go's v0.19 changelog entry (BUG-2078, codex r3 follow-up) The in-code changelog is the canonical source; it was missing the codex r2 schema-shadow refusal that a later commit added, which is why CLAUDE.md and version.go briefly disagreed. Completes version.go instead of letting CLAUDE.md drift ahead of it.	2026-08-15 20:24:31 -04:00
xarmian	ac05d8a2b1	fix(cli): fail fast and quiet on non-interactive workspace init (BUG-2538, BUG-2577) (#1111) ... * Fail fast and quiet on non-interactive `pad workspace init` BUG-2538: initCmd drove runBrowserSetup/doBrowserLogin unconditionally when the instance needed first-run setup or login, blocking a non-interactive caller (script, CI, headless agent) on a browser handoff nobody can complete. Gate both branches on canPromptForConfig(), mirroring the precedent already used by `pad init` (init.go:205-206), and fail fast with a hint pointing at `pad init --email/--name/--password` or `pad auth setup`/`pad auth login`. BUG-2577: offerSkillInstall (shared by workspace init and workspace link) printed a "(Y/n): " prompt even when the answer would be auto-defaulted rather than read, because it gated on cli.IsTerminal() (stdin only). Switch to canPromptForConfig() (stdin AND stdout), which is the same predicate now used for BUG-2538 and the more robust of the two checks already in the codebase. Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt * Fix wrong remedy in BUG-2538's !Authenticated error message codex r1: the !Authenticated branch suggested `pad init --email/--name/--password`, but those headless flags only bootstrap the first admin account and only fire when SetupRequired — for an already-set-up-but-unauthenticated instance, `pad init` falls through to its own ungated Step 4 re-auth (BUG-2592), so the suggestion relocated the hang instead of avoiding it. Drop the pad-init suggestion in this branch only; point at `pad auth login` and note there's no non-interactive login path yet. SetupRequired branch is unchanged — its pad-init suggestion is correct for that state. Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt * Fix two more inaccurate remedies flagged by codex r2 1. SetupRequired branch: `pad init --email/--name/--password` silently eats the caller's workspace name/--template — pad init creates its own CWD-named workspace as a side effect, so a re-run of the original `pad workspace init <name> --template <t>` short-circuits on the link pad init just made with no signal <name>/<t> were ignored. Switch the remedy to `pad auth setup --email/--name/--password`, which bootstraps the admin account only (no workspace side effects), then re-run the original command. 2. !Authenticated branch: the "no non-interactive login path exists" claim was false — `pad auth login --interactive` reads email/password off a plain, TTY-ungated bufio.Reader (doInteractiveLogin, cmd_auth.go:554+; BUG-1886 made it piped-bytes-safe), so it works fine when credentials are piped in. Reworded to point at it and dropped the incorrect BUG-2592 reference (that bug tracks pad init's ungated Step 4, not a missing login mechanism). TestWorkspaceInitNonTTYSetupRequired's assertion updated from "pad init" to "pad auth setup" to match; TestWorkspaceInitNonTTYNotAuthenticated needed no change (still asserts "pad auth login"). Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt * Update skill docs invalidated by the non-interactive fast-fail fix codex r3: BUG-2538/BUG-2577 made this diff's own docs stale. Four files (skills/pad/SKILL.md, plugin/skills/pad/SKILL.md, plugin/skills/onboard/SKILL.md, plugin/skills/capture/SKILL.md) still say non-interactive `pad workspace init` on a configured-but- unauthenticated machine "blocks for minutes with no non-interactive fallback" — that was true pre-fix (per BUG-2541's verification) and is false now. Reworded the WHY without dropping the underlying do-not-run-blind guidance: an agent's tool call is always non-interactive, so it now gets a fast, actionable error instead of a hang, but the error still just says a human needs an interactive terminal — `pad auth whoami` remains the right check to run instead. Where the docs' `pad init` claims are about the still-unfixed session-expired path (BUG-2592, this diff's Step-4 sibling, left untouched), those claims are unchanged and now cite BUG-2592 explicitly. skills/INSTALL.md:24 updated separately (P3): notes the non-interactive silent-install branch of `pad workspace init`'s skill offer, alongside the existing interactive-prompt description. Docs only, no Go changes — go build/test and embed.go's //go:embed skills/pad/SKILL.md still resolve; no test asserts the old wording. Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt	2026-08-15 18:41:50 -04:00
xarmian	7d5d3bd672	fix(cli): give the CLI auth poll loop its own wall-clock timeout (BUG-2572) (#1109) ... * fix(cli): bound pollAndSaveCLIAuth with its own wall-clock timeout (BUG-2572) pollAndSaveCLIAuth had no wall-clock limit of its own — the ~5m bound users rely on was purely the server-side session TTL, so an unreachable server after session creation left the poll loop spinning forever on Ctrl-C alone. Add a 20m timer (matching the longer of the two server TTLs, since this helper is shared by both the plain login and first-run setup flows) plus a consecutive-transient-error bound so a permanently unreachable server fails fast with a network-shaped error instead of waiting out the full timeout. Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt * fix(cli): make poll-error headline accurate for HTTP-error servers (BUG-2572 r2) The consecutive-error bail-out message claimed "could not reach server", but client.get returns an error for both transport failures and non-2xx HTTP responses, so a server that's reachable but persistently returning 500 got misreported as unreachable. Bailing out fast is still correct for that case; only the headline was wrong. Switch to a cause-neutral message and let the wrapped error carry the specifics (codex round 2). Claude-Session: https://claude.ai/code/session_01BhQoeaWXxJbvw86ezzK8dt	2026-08-15 17:16:29 -04:00
xarmian	d7da237198	feat(mcp,cli): clear_assigned_user / clear_agent_role — a discoverable unassign (IDEA-2584) (#1107) ... * feat(mcp,cli): clear_assigned_user / clear_agent_role — a discoverable unassign (IDEA-2584) v0.16 and v0.17 made unassigning WORK. Nothing advertised it. The params that do it — `assigned_user_id` / `agent_role_id` — were never in the catalog, so an agent reading the tool schema to find out how saw only `assign` (a name) and reached for `assign: ""`, which is a no-op and deliberately stays one. The capability existed with no name an agent could find. `clear_assigned_user` / `clear_agent_role` booleans on `pad_item`, backed by new `--clear-assigned-user` / `--clear-agent-role` bareword flags on `pad item update`. WHY BOOLEANS rather than declaring the existing string params. Two reasons, and the second decided it: 1. An empty DECLARED string is inert everywhere else on this tool (title, content, comment, tags), so a client that pads optional params with "" instead of omitting them is harmless today. Giving one a destructive meaning would turn that same client into one that silently unassigns every item it touches. A boolean carries its meaning in its name and can't be tripped that way. 2. Only a boolean can REACH local stdio. BuildCLIArgs emits the CLI's real flags, so a catalog param with no flag behind it is dropped before dispatch — declaring `assigned_user_id` would have left the direct form remote-only, i.e. would not have closed the gap this change exists to close. That fact reframed the design fork and is what the ruling turned on. Server-side this is WIRING, not new semantics: models.ItemUpdate.ClearAssignedUser / ClearAgentRole already existed and the store has honoured them since BUG-2566, on the same branch as the empty-string form. The older forms keep working and are NOT deprecated; they're just not what the schema advertises. UPDATE ONLY, deliberately asymmetric with create, and recorded in-place at both the flag registration and the catalog description so a symmetry-minded reader meets the reasoning before the "fix": clearing at create is a request to not-set something never set, whose only honest behaviour is a no-op — it teaches a wrong affordance and pads every create call's schema. A test fails if someone adds them there. CLI precedence is the OPPOSITE of the --field lift's, deliberately: an explicit `--clear-assigned-user` beats `--assign`, because that combination is a contradiction the user typed and the reading that cannot silently assign somebody is the safer one. Tested. The dispatcher forwards the booleans VERBATIM rather than only-when-true. A `&& b` guard would read as the thing protecting a param-padding client and would be lying: what makes `false` inert is the store. Same call I made on #1106's `len(patch) > 0` — a guard that reads as load-bearing while doing nothing is worse than none. ToolSurfaceVersion 0.17 -> 0.18, ADDITIVE bump per the v0.5 / v0.6 precedent: no existing tool, action or param changed shape. Consumed artifacts moved in the same commit, which is the whole point of this change — the schema IS the deliverable: catalog_item.go (the schema agents read, plus an `assign` description that now says where to find the clear), instructions.md (leads with the boolean, mentions the older forms as still-working), version.go, README, CLAUDE.md. VERIFIED LIVE, five legs, both transports: CLI --clear-assigned-user -> assigned=None, role intact CLI --clear-agent-role -> role=None stdio clear_assigned_user:false -> assignment SURVIVES and the update still applied (title changed) — the control that makes the boolean safe to declare at all stdio clear_assigned_user:true -> assigned=None stdio clear_agent_role:true -> role=None Three mutations, each failing only its own tests: dropping the dispatcher forwarding; hardcoding true in the dispatcher (fails the false-control); dropping the CLI flag wiring. go test ./cmd/pad ./internal/mcp — pass. gofmt clean. Closes IDEA-2584. Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd * fix(mcp,cli): refuse a simultaneous set-and-clear (codex round 1) Codex found a real bug, and the more useful half of the finding is that MY OWN TEST FOR IT WAS VACUOUS. The store's branch order is `if AssignedUserID != "" { set } else if ClearAssignedUser { clear }`. So `--assign wren --clear-assigned-user` assigned Wren and the clear evaporated. My in-place comment claimed the opposite ("an explicit clear wins"), and the test I wrote to prove it asserted `body["clear_assigned_user"] == true` — that the FLAG was set, not that the item ended up unassigned. The flag was set. The behaviour was backwards. A test that asserts a field is present says nothing about which field wins. Both surfaces now REFUSE the contradiction rather than silently resolving it. Rejecting beats picking a winner here: the store already picks one silently, which is the bug; and a caller who typed both wants to be told, not guessed at. Precedent in the same command family — `item list` already makes `--parent` and `--unparented` mutually exclusive. PLACEMENT IS THE LOAD-BEARING PART, and I got it wrong first. There are two routes to a competing value: `--assign` / `assigned_user_id`, which resolve early, and `field: ["assigned_user_id=<uuid>"]`, which reaches the payload via liftFieldsToColumns LATER. My first version checked between them and its comment asserted the lift "has already" run — it hadn't. That version rejects the direct case and lets the lifted case through: a half-fix that reads as complete. The check now runs after both, in the CLI after --assign/--role resolution and the lift, in the dispatcher immediately before the body marshal. That mutation is now a test: moving the dispatcher check back to the pre-lift view fails ONLY the two `lifted …` subtests and passes the direct one — the exact shape of the bug I nearly shipped. Tests assert the OUTCOME, not the message: a refused conflict must leave the item's assignment AND role untouched, and the CLI must issue no PATCH at all. An error string alone wouldn't prove the write didn't happen. Agent-facing text moved with it (the consumed-artifact step): both catalog descriptions, instructions.md, and the v0.18 version entry now say the combination is refused. An agent that pairs them gets a structured refusal, so the schema has to say so. go test ./cmd/pad ./internal/mcp — pass. gofmt clean. Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd	2026-08-15 13:17:01 -04:00
xarmian	847ee73327	fix(cli): lift assigned_user_id / agent_role_id onto their columns (BUG-2583) (#1106) ... * fix(cli): lift assigned_user_id / agent_role_id onto their columns (BUG-2583) `pad item update TASK-9 --field assigned_user_id=<uuid>` wrote the pair into the item's FIELDS JSON BLOB while the column stayed stale, and then printed "Updated TASK-9". Two defects in one line: a success message for a write that did nothing the caller asked for, and a blob key shadowing a real column's name, so the CLI surface diverged from store/HTTP/MCP truth. The empty-string case was the same defect wearing a worse hat — it was the only route an agent had to unassign an item. Blast radius beyond the CLI: local stdio MCP (`pad mcp serve` — Claude Desktop, Cursor, Windsurf) dispatches through ExecDispatcher, which shells out to this CLI. So TASK-2571's fix reached the remote /mcp transport only, and the transport most agents actually use still could not unassign. This closes that half. `cmd/pad/cmd_item.go` now lifts `columnFieldKeys` out of the --field map onto the column pointers, on CREATE and UPDATE both, mirroring internal/mcp/dispatch_http.go's liftFieldsToColumns — including its INVARIANT, which is the part that matters: only keys with defined clear-to-NULL semantics for "" belong in the list, and `tags` never does (an empty write corrupts a JSONB column rather than clearing it). A test fails if anyone adds it. Two compat changes, ruled separately by the lead: Q1 non-empty values move to the COLUMN and stop writing the blob key. Accepted: relying on the old behaviour is relying on a shadowing defect. Q2 empty values clear the column. Falls out of the lift, inheriting BUG-2566's store semantics. `agent_role_id` gets identical treatment. Existing stray blob keys are left alone per the ruling — this stops minting new ones; a sweep would be its own change. Precedence is explicit and tested: `--assign` / `--role` win over a lifted --field value, matching liftFieldsToColumns' "caller-supplied top-level values win". It is delivered by the ORDER of two blocks in the command, which is exactly the kind of thing that gets reordered by accident, so there is a test whose only job is to fail when it does. A non-string --field value is deliberately NOT lifted: a collection that genuinely declares a field with one of these names makes parseFieldFlag return a typed value, which cannot address a column. It stays in the blob — today's behaviour and the only lossless option. ToolSurfaceVersion 0.16 -> 0.17, and v0.16's transport-scope paragraph now points forward rather than claiming a limitation that no longer holds. Behaviour-only bump again, same grounds as v0.16 and v0.9. The CLI's own marker, CmdhelpVersion, deliberately does NOT move: its contract is flag/arg SCHEMAS, and no flag or argument changed shape. instructions.md — the text agents receive at handshake — drops the "remote only" caveat it carried since TASK-2571. That file is the reason this PR exists in the shape it does: it is the artifact the actor reads, and it was the one place the previous PR overclaimed. VERIFIED LIVE against a running server, with a negative control, because the claim is about a transport rather than a function: legs, fixed binary --field assigned_user_id= -> column CLEARED, blob clean --field assigned_user_id=<uuid> -> column SET, blob clean --field agent_role_id= / <uuid> -> same, sibling column untouched stdio MCP tools/call pad_item action=update field=["assigned_user_id="] -> column CLEARED, blob clean control, PRE-FIX binary, same server + same item + same JSON-RPC bytes -> column UNCHANGED, blob polluted with {"assigned_user_id":""} Six unit tests in cmd/pad/item_column_fields_test.go, four mutations each failing only its own test (no lift; drop non-strings; flip the lift/assign precedence; add `tags` to the list). One assertion was rewritten after mutation testing showed it was VACUOUS: `len(fields_patch) != 0` passes whether the key is absent or present-and-empty, so it now asserts key PRESENCE — confirmed by mutating `omitempty` off the model field and watching the old form stay green. The redundant `len(patch) > 0` guard that assertion was meant to cover is gone too; `omitempty` already does that job, and a guard that reads as load-bearing while doing nothing is worse than no guard. go test ./cmd/pad ./internal/mcp — pass. gofmt clean. Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd * test(cli): cover the create half of the column lift (BUG-2583) Codex came back CLEAN, but the review reminded me I'd changed `item create` and only tested it through `liftColumnFields` directly — no test asserted what create actually puts on the wire. That's the weaker half to leave uncovered, not the stronger one: on update a wrong write contradicts a visible prior value, while on create the column-named key is simply baked into the blob at birth with nothing to contradict it. The assertion has to parse rather than index, because ItemCreate.Fields is a JSON-encoded STRING and not a nested object — a body["fields"]["…"] lookup would have been vacuous in a way that looks fine. Mutation-tested like the rest: neutralizing the create-side lift fails this test and only this test. Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd * docs(mcp): say WHICH form of the unassign works on which transport (codex round 2) Codex round 2, and it is the same class of defect as the previous PR's round 2 — an overclaim in the artifact agents actually read. My instructions.md said "works on BOTH transports" of two forms that do not behave the same: field: ["assigned_user_id="] clears on BOTH transports assigned_user_id: "" clears on REMOTE ONLY The direct params are not declared in pad_item's schema. They reach the remote mapper only by riding the verbatim input map; on stdio, BuildCLIArgs drops unknown keys, so the call does nothing. VERIFIED, not accepted on the reviewer's word, and the verification corrected my own first reading. My initial probe appeared to show the stdio call CORRUPTING the fields blob — but that blob key was leftover state from the earlier pre-fix control leg, not something the probe wrote. Re-run against a freshly created item, the two forms separate cleanly: before assigned=b6786b13... fields={priority,status} after assigned_user_id:"" assigned=b6786b13... fields={priority,status} (clean no-op) after field:["assigned_user_id="] assigned=None fields={priority,status} (cleared) So the stdio behaviour of the direct param is a DROP, not a corruption — worth stating precisely, because "it corrupts the blob" would have sent the next reader hunting a bug that isn't there. (Identity-doc rule: a guessed mechanism stated as the reason is a claim, not a hedge.) instructions.md now leads with the form that works everywhere and names the remote-only limitation of the other; version.go and CLAUDE.md say the same. IDEA-2584 — declare the params properly — is the fix that would collapse this distinction, and is now cited from all three. Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd * fix(cli): don't lift a field the collection actually DECLARES (codex round 3) Nothing reserves `assigned_user_id` or `agent_role_id` as field names, so a collection may legally declare a field with one of those keys. For that collection `--field assigned_user_id=foo` means the DECLARED field — and the lift I just added would redirect it to the assignment column while dropping the value the user set. Two wrongs from one line: the intended write vanishes and an unintended one happens. liftColumnFields is now schema-aware and never lifts a declared key. Cheap to do here because both call sites already fetch the collection schema for parseFieldFlag. The check is PER-KEY — an undeclared sibling still lifts, so one collision doesn't disable the feature — and a schema-fetch failure degrades toward lifting, matching how the rest of --field handling degrades. This makes the CLI deliberately STRICTER than the MCP dispatcher it otherwise mirrors. liftFieldsToColumns has the identical collision and can't make the same check as written: it builds its fields map straight from the tool input without fetching a schema. Filed as IDEA-2587 rather than fixed here, because closing it costs a round-trip on a hot path while the CLI fix was free — and recorded so the divergence is KNOWN, in the safe direction, rather than something a later reader "fixes" by loosening the CLI to match. The old non-string branch stays as belt-and-braces: parseFieldFlag only returns a non-string for a declared field, which the new check already catches, but if that stops being true a non-string still can't address a column. Mutation-tested: ignoring the schema declaration fails the new test and only that test. Claude-Session: https://claude.ai/code/session_01QCMLhHQBrMHVML3YKdm4Cd	2026-08-15 12:12:32 -04:00
David Barkhausen	b9381bf5f1	feat(cli): markdown output on the remaining list surfaces; broaden ANSI stripping (#1080) ... Completes #898 and fixes #1076. Markdown on the seven surfaces left out of #1070, so `--format markdown` is now honestly global and the flag help collapses to "table, json, markdown": - `item comments`, `item deps`, `project activity`, `attachment list`, `library list`, `role list`, `workspace members`. Two of those are not tabular, and markdown follows the terminal shape rather than forcing a table onto them: - `item comments` keeps the attribution-line-then-body form, and the body is emitted VERBATIM. A comment body is authored as markdown; escaping it would turn its lists and code fences into literal text. Only the attribution line, which we construct, is sanitized. - `item deps` keeps its two sections as `## Blocks` / `## Blocked by` lists. Colour carried the direction in the terminal (yellow out, red in); headings carry it here. New shared spine: `cli.RenderMarkdownTable(w, headers, rows)`. Every cell is escaped, and ragged rows are padded or truncated to the header width so a short or long row can't shift the column count and break the table. Wiring a surface is now naming columns and mapping rows. #1076 — ANSI stripping covered only SGR (`ESC[…m`), so non-SGR CSI sequences, OSC-8 hyperlinks, and stray C0 controls survived, both in the table width maths and in markdown output whose doc comment promised escape-free text. Replaced `sgrPattern` with `ansiPattern` + `stripANSI` covering OSC, CSI, two-character Fe escapes, and stray C0/DEL, with TAB/LF/CR deliberately preserved for callers that normalize them. `displayWidth` now uses it too: a control sequence is zero-width, so counting it was a column-alignment bug of the same family. Tests: 12 stripping cases, 4 table-helper cases (including ragged rows), 4 renderer cases for the two non-tabular surfaces, and the routing test extended to 8 subtests — one per surface, driven through cobra against an httptest server. Also covers the two gaps named in #1076: `item starred` and the scoped `item list <collection>` path. Each new guard was proven by mutating the source and watching it fail, not just by passing. Gates: go build ./... PASS; go vet PASS; gofmt clean; golangci-lint 0 issues. Both touched packages show the same 6+2 pre-existing Windows failures as clean main under an identical sandboxed run.	2026-08-14 22:35:23 -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	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