39 Commits

Author	SHA1	Message	Date
xarmian	c73584088f	fix(watchevents): detect a half-open Redis connection with a bus heartbeat (BUG-2769) (#1199) ... * fix(watchevents): detect a half-open Redis connection with a bus heartbeat (BUG-2769) internal/watchevents had the same defect as internal/events did, by the same mechanism: ChannelWithSubscriptions on a connection whose go-redis health check only writes. PubSub.Ping calls writeCmd and returns without reading a reply (v9.22.0), so a route that stops carrying traffic without closing is invisible — the instance blocks on a read forever while its replay buffer goes on looking complete. Named as a class sweep in BUG-2738's filing and deferred there. It became load-bearing when that unit shipped: docs/deployment.md told operators the gap was "closed on the activity stream and still open on the watch stream". This diff falsifies that, which is why the prose sweep is part of it. THE PORT IS SMALLER THAN THE ORIGINAL BY DESIGN. This bus holds ONE process-wide subscription created in its constructor, off any request path, so none of BUG-2747's establishment machinery exists to interact with: no per-workspace map, no establishment record, no single-establisher wall, no concurrency cap, no bounded-parallel recovery, and no per-workspace cycle scoping. Cost is flat too — one frame per instance per interval regardless of workspace count. NO COMPANION COUNTER, and that was CHECKED rather than inherited. internal/events needs pad_event_subscription_cycled_total because its dropWorkspaceCoverage returns early when a workspace has no buffer, so the reset reason under-reports the early-wedge case. dropCoverage here has no such branch: it replaces the buffer and reports unconditionally, so idle_timeout is a complete count on its own and a second metric would be a number needing to be explained against its neighbour for no signal. THE RECEIVE LOOP NOW OWNS ITS SUBSCRIPTION AND CONTEXT. A cycle replaces the subscription under a running bus, and the loop reading the old one must tell "I was replaced" from "the client died" — the second logs an ERROR and moves a counter documented to mean the instance has gone deaf. The cycle cancels that loop's own context before closing its PubSub, so it leaves by the quiet door. Its own test. I PORTED A FLAW ALONG WITH THE STRUCTURE, and the wiring test caught it: both maintenance halves shared one kick channel, so whichever goroutine was waiting consumed it and the other stayed on the stale cadence. internal/events' mutation matrix found exactly that (M11c) and fixed it; the fix did not come across. That is the contamination hazard this port's grounding warned about, in its literal form, caught by the CONVE-19 test rather than by review. Two more found by mutation, both missing tests rather than missing code: nothing asserted that ordinary traffic keeps the instance alive (removing the per-frame stamp survived, because every other test drives idleness through the clock), and nothing asked for a SECOND detection (a replacement inheriting stale stamps gives a detector that works exactly once, which is worse than one that never runs because it looks like it works). The second needed a direct assertion on the install stamps, because the behavioural route re-stamps the field it was meant to be testing. Trio in one commit as required: reason enumeration, the pad_watchevents_sequence_resets_total Help string, and docs/deployment.md — plus the two BUG-2738 sentences this falsifies and a new section explaining how the watch bus differs from the activity one. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(watchevents): fence stragglers and re-validate before the drop (codex r1) Three findings, and two of them are BUG-2738 fixes I again failed to bring across with the structure. That is now three times in one port: the shared kick channel, the stale idle decision, and the missing generation. The mechanism is the same each time — I ported what the code DOES and not what its review history taught it, and each was caught by a test or a reviewer rather than by me reading the source I was copying. STALE IDLE DECISION. cycleIfIdle decided under one lock and tore down under another; a heartbeat or notification arriving between them left a demonstrably alive subscription being dropped and every client on the instance resynced for nothing. BUG-2738 fixed exactly this at its round 11. Re-validated immediately before the drop, with a positional seam so a test can land the recovery inside the window rather than racing it. NO GENERATION FENCE. Cancelling a receive loop and closing its PubSub does not JOIN the goroutine, and go-redis's channel is buffered, so a frame from a replaced subscription could still stamp the replacement's liveness, append to its buffer, or drop its coverage. On a wedged route that is the worst direction: the dead connection's buffered tail suppressing the detector for its successor. One check at the top of the frame handler covers all three, because the three must agree about whether a frame belongs to the live subscription. The probe stamp is fenced separately, since a slow publish can outlive the subscription it was sent for. A COPIED COST PARAGRAPH THAT CONTRADICTED ITS OWN SECTION. The activity bus's "each workspace has its own subscription, N frames per interval" text sat below the new watch-specific section saying the opposite. Retitled and moved above it. FOUR INSTRUMENT DEFECTS ON THE WAY, all found by mutation: - Nothing asserted ordinary traffic keeps the instance alive — every other test drives idleness through the clock, so removing the per-frame stamp survived. - Nothing asked for a SECOND detection, so a replacement inheriting stale stamps gave a detector that works exactly once — worse than one that never runs, because it looks like it works. Needed a direct assertion on the install stamps, since the behavioural route re-stamps the field under test. - The generation tests asserted the PREDICATE, not that the loop calls it. - And that wiring test could not discriminate on a frozen clock, where a stamp writes the value already there. It advances the clock first now. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(watchevents): make the generation fence atomic with what it guards (r2) Two P1s, both mine, both the same shape: a check in one lock acquisition and the mutation it guards in another. THE FENCE WAS NOT ATOMIC WITH ITS MUTATIONS. One check at the top of the frame handler read well and guarded nothing reliably — a replacement between that check and stampLastSeen / fanOutFromRedis / dropCoverage let a straggler through to any of them. The generation now travels TO each mutation and is re-checked under the same lock that mutates. A stale notification entering the replacement's buffer is the worst of the three: it makes the instance vouch for a span it never received, which is the false coverage claim this whole family exists to remove. THE OLD GENERATION STAYED CURRENT ACROSS THE REPLACEMENT. subGen was incremented only after the new subscription was confirmed, leaving the cancel, the close, the dial and a round trip during which the OLD generation still passed every fence. Retired at teardown now, so during resubscribe NO generation is current and a late frame is ignored everywhere. That also makes the failure path honest: the "no notifications until restarted" log was false — no generation is current, so the next idle tick tries again. Revalidation and the drop are now ONE critical section rather than two, for the same reason at one level down: a frame arriving between them was silently discarded by a drop already decided on. Also: phase 1 no longer starts the maintenance goroutines, and the watch bus's phase is logged at startup — an operator cannot read an absence of idle_timeout without knowing whether the detector was running, and the two flags are independent. DOCS still described the workspace model in the section that claims to cover both buses: one heartbeat "per subscribed workspace", a phase table naming only PAD_EVENTS_HEARTBEAT, and coverage described as a workspace's. Generalised. Two more instrument gaps, both found by mutation: nothing asserted a straggler cannot enter the replacement's BUFFER (only the stamp was covered), and the phase-1 goroutine gate is untested by design — removing it changes no behaviour, only goroutine count, and the only assertion is a census that would be flaky here. Said out loud rather than left to look like coverage. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(watchevents): prove each generation fence on its own Round 3's fix put a generation check in each of the four places a frame from a replaced subscription can mutate shared state, rather than one check at the top of the receive path — a check in one lock acquisition and a write in another is a TOCTOU, which is what codex blocked. Four checks means four mutations, and the matrix found the first pass of tests could not tell them apart: removing the append's check, or the coverage drop's, left every test green. Not because the guards were redundant — because no test drove those paths with a stale generation. The straggler tests all enter through fanOutFromRedis, whose own guard returns early and hides the one below it, and nothing at all drove dropCoverageForGen with a straggler. So the fences are asserted one at a time, each through the entry point that actually reaches it: epoch bookkeeping fanOutFromRedis with a foreign epoch — the loudest of the four, since an accepted straggler would rewrite the id space and resync every client on the instance buffer append fanOutLocally directly, under the guard above it coverage drop dropCoverageForGen, previously undriven liveness stamp stampLastSeen, which would otherwise let a dead socket's traffic hold detection open Each fails against removal of the single check it names (M16/M17/M19 and the existing stamp mutation), and the four together still pass the end-to-end straggler tests unchanged. Refs BUG-2769 * test(metrics): prove the two new watch signals reach the registry Both were wired and neither was asserted at the metrics layer, which is where docs/deployment.md's claims about them actually live. A reason or a callback that never reaches the registry is a runbook pointing at a series that does not exist, and nothing in internal/watchevents can catch that — its observer is an interface, satisfied by a test double. pad_watchevents_heartbeat_publish_failures_total incremented six times, a count no other assertion in that test uses, so a callback wired to the wrong counter cannot land on the right number by coincidence. Fails when the increment is pointed at a neighbour. sequence_resets_total{reason="idle_timeout"} asserted with the literal label, alongside the four spellings already pinned there and for the same reason BUG-2739's rename left that test behind. Fails when the constant drifts. Also corrects the shared "what happens if you run them out of order" paragraph, which moved under a heading covering both buses while still describing only one: it said the frame travels on "the workspace's event channel" and that an un-upgraded instance resyncs "for every workspace", neither of which is the watch bus, where there is one channel and one buffer per instance. The blast radius differs in scale between the two and the paragraph now says so. Refs BUG-2769 * docs(watchevents): correct three counted claims that stopped being true All three said "three" where the code now has four, and each was accurate when written — the fourth fence (the epoch bookkeeping in fanOutFromRedis) was identified after them, in the pass that found the matrix could not tell the guards apart. That is the whole failure mode: a count is a claim, and a claim written before the last change is wrong afterwards with nothing to notice it. Two of the three sat inside a comment ABOUT how carefully the guards were enumerated, and one names them now instead of counting them, so the next site added has to appear in the list or contradict it visibly. Found by sweeping the branch diff for counted prose rather than by rereading, which is what had already missed them twice. Refs BUG-2769 * test(config): close the other half of the two-flag independence claim The flag tests asserted PAD_WATCH_HEARTBEAT does not move EventsHeartbeat and stopped there, while the comment above them and the deployment doc both claim the two buses roll INDEPENDENTLY. That is a biconditional and one leg does not establish it: a Load() that pointed PAD_EVENTS_HEARTBEAT at both fields passed everything. Now both directions are asserted, and the events leg checks its own premise first, so a fixture that stopped setting the flag fails as a fixture rather than as a pass. Also pins env-over-file precedence for the watch flag, in the direction that actually matters: PAD_WATCH_HEARTBEAT=false over watch_heartbeat=true in config.toml. That is the rollback for a bad phase-2 flip, and an operator reaching for it mid-incident cannot be editing a file on every host. Mutation matrix, each detected: the env var wired to the neighbouring field, the env var never read at all, and the toml tag dropped. Refs BUG-2769 * test(watchevents): fix five tests that passed for the wrong reason Codex round 4 went at test honesty rather than correctness and found no BLOCK, but it found five assertions that hold whether or not the thing they name works. Each is now driven through the path it claims, and each was mutation-checked against the specific defect it exists to catch. the malformed-frame contract only ever called isWatchHeartbeat. The predicate can be perfect while the receive loop routes every "hb|…" payload to the ignore arm without asking it, which is the defect, and the test's name promises coverage ends — a claim about the loop. Now published on the real channel, with a well-formed frame as the control so the assertion cannot be satisfied by a loop that finds everything undecodable. the receive-loop wiring test published, slept 300ms, and asserted nothing had changed. A loop that stalled or never started satisfies that perfectly. There is no natural signal to wait on instead, because a frame the fence refuses is by design invisible — hence a seam that fires after the loop handles a frame whichever arm it took. Bounded, so a stalled loop fails with a message rather than a package timeout, and followed by a control that the same loop still accepts a frame whose generation matches. the quiet-exit test asserted only that no loud exit was reported, which a replaced goroutine that never exits at all also satisfies — a leak, and the worse outcome. Now joins the loop first via a process-wide live-loop count, then checks the counter, so it is a statement about a goroutine that has finished. the maintenance-loop wiring test claimed both halves and observed a heartbeat, which a loop that started only the publisher passes. The idle half cannot be proved there at all: against a live miniredis this bus's own heartbeats come back and refresh liveness every cadence, so wedging it with the loop running is a race against the publisher — which is what my first fix for this turned out to be, flaky at 2 in 3. Renamed to what it proves, pointing at the blackhole end-to-end test, which drives the scanner for real and detects both mutations. the straggler test never delivered a straggler. It incremented subGen by hand, called isCurrentGen, and compared an unchanged timestamp without touching a mutation path — green with every fence removed. Deleted rather than repaired: the four-way per-fence test added earlier covers it properly, and isCurrentGen went with it. Plus two ordering changes in Close/resubscribe that ARE NOT fixes for an observed race, and say so in the test. Making b.pubsub reassignable made Close's unlocked read of it look wrong, and resubscribe's wg.Add outside the lock look like it could land after Close reached Wait. Both windows turn out to be shut already by resubscribe's b.closed check, which sits under the same acquisition as the count — reverting either fix leaves the new Close-during-cycle test green. Kept as defence because the invariant they lean on is three functions away, and documented so nobody later reads them as evidence of a bug that existed. Also corrects the metric help and two comments that said an idle cycle "replaced the connection" when it attempts a replacement that can fail; the deployment doc already said attempted. And the deployment doc's rollback, frame-validation, what-to-watch and startup-log paragraphs, all of which moved under a heading covering both buses while still describing only the activity one. Refs BUG-2769 * refactor(watchevents): drop an always-empty return and the branch reading it dropCoverageIfStillIdle returned (string, bool) where the string was never anything but empty — the reset it reports goes out through the pending/flush path inside the lock, so the caller's `if report != ""` was unreachable. A second reporting path that exists in the signature and never fires is a thing a later change wires up by accident. Refs BUG-2769 * fix(watchevents): a failed re-dial retries without re-dropping coverage Codex round 5, on behaviour across a full Redis outage. No BLOCK; this was its one P2 and it is real. The probe-failure suspension does not cover this case, and the reason is worth stating because the suspension looks like it should. Suspension asks "did our last probe get through", and that can be YES with the route already gone: the last successful publish stamps lastProbeOK, Redis dies before that frame comes back, and lastSeen stays behind it. From there both timestamps are frozen — the probe fails so nothing stamps lastProbeOK, nothing arrives so nothing stamps lastSeen — and the cycle's precondition stays true for the whole outage. Every pass then dropped coverage, announced to every subscriber, and re-dialled. Only the re-dial is owed. The second drop empties an already-empty buffer and re-announces a hole every subscriber has been told about, and it moves pad_watchevents_sequence_resets_total{reason="idle_timeout"} once per cadence — so a five-minute outage read as ten incidents on the series operators are told to alert on. cycleIfIdle now has a retry-only arm ahead of the decision, entered when there is no subscription at all, and the teardown clears b.pubsub / b.subCancel so that state is representable. Clearing them also stops Close closing an already-closed PubSub a second time. Two tests, discriminating in OPPOSITE directions, because the obvious fix for the noise is to suspend the pass and that would trade a noisy outage for one the instance never returns from — retrying the dial IS the recovery path: three passes with Redis away one reset, not three Redis returns after a failed pass the subscription is re-established and the counter does not move again Matrix: removing the retry arm, making it return without retrying, and leaving the torn-down subscription in place are each detected, the middle one only by the recovery test. internal/events has no equivalent defect. Its teardown deletes the workspace's subscription entry, so its next scan finds nothing live and abandons; recovery there runs off the request path. Refs BUG-2769 * fix(watchevents): only one caller may install a replacement subscription Codex round 6, verifying round 5's fix. No BLOCK; this was its P2. Both the cycle and its new retry arm dial with the lock RELEASED, which is deliberate — a Redis round trip under the bus's hot mutex would stall every fan-out on the instance — so two passes can each find no subscription and each dial one. Installing both is wrong twice over: two receive loops would run on the SAME generation, so both accept every frame and each notification is processed twice, and the loser's PubSub would be untracked, closed by nothing including Close. The install is what needs serialising, not the dial, so the loser discards its own connection under the lock rather than the two racing to overwrite b.pubsub. Only the idle scanner calls this today, so this guards an invariant rather than fixing an observed fault. Written down because the invariant lives in a different file from the code relying on it, and because the failure is silent duplication rather than a crash. The test races two resubscribes through the install seam. Two details it needed, both found by running it rather than reading it: the loop count is incremented INSIDE the goroutine, so sampling it right after the constructor returns reads zero — the first version did, and measured every later count against that wrong baseline. It waits for the loop now. the seam release is deferred, because without it the guard's mutation parks both callers in the callback, Close waits on receive loops that cannot start, and the detection arrives as a package-wide hang with no message. That is how the mutation first appeared to pass. Also completes the idle_timeout reason in three comment/help sites that still enumerated four reasons and said "the last two" — the same stale count corrected in the observer contract earlier on this branch, missed in its neighbours because I fixed the one the reviewer named instead of grepping for the claim. Refs BUG-2769 * test(watchevents): count installs instead of waiting for one that never comes Codex round 7 returned no BLOCK and no P2 on the production code, and two NITs on what round 6 added. Both are real. The concurrency test synchronised on a WaitGroup expecting BOTH callers to reach the install seam. Only the winner does — that is the property under test — so in the passing case the goroutine waiting on it blocks forever. A leak inside a test written to prove a leak does not happen is not a shape to leave standing. An atomic the abandoning caller never touches carries the same information and blocks nobody, and it removes the release channel and its deferred close along with it. The final assertion also moved off liveReceiveLoops and onto that count. A loop starts AFTER its install, so reading the loop count can catch a second caller's goroutine before it has begun and see the passing value on a failing run. Both callers have returned by the time the install count is read, so it is final. Detection over ten runs with the guard removed: 10/10, where the loop-count version was a race against a goroutine's first instruction. Also softens the retry arm's log line. It said the instance receives no notifications until an attempt succeeds, which is true for today's single scanner and stale the moment there are two: one caller's dial can fail while another has already installed. It now claims only what the failing call knows. Refs BUG-2769 * test(watchevents): hold both callers at the window, and say what that misses Codex round 8's P2, on the test the previous commit rewrote. Starting two goroutines from a start gate makes overlap likely and guarantees nothing: one can finish resubscribe before the other begins, so the window the install guard closes need never have been open. A seam at the dial/install boundary — connection dialled, lock not yet taken — lets both callers announce their arrival and wait for each other. Now the window is open by construction rather than by luck, and the test fails as a fixture if only one caller ever reaches it, instead of passing on evidence it never gathered. AND IT STILL DOES NOT DETECT EVERYTHING, which the test now says in place of leaving it implied. Measured: guard removed entirely 10 runs, 10 detected guard checked in its own acquisition, then 10 runs, 0 detected the lock retaken to install The second is the regression round 8 asked about, and catching it would mean landing the second caller inside a check-to-install gap that exists only in the mutant — there is nothing to yield on there, and no seam can be placed in code that is not written. So this test covers "a guard exists", not "the guard is in the right critical section". The latter is held by the comment at the guard and by review, and a test comment claiming otherwise would be worth less than the honest note. Refs BUG-2769 * fix(watchevents): make the frame seam and the cycle log tell the truth Codex round 9 was asked whether this should merge and said hold for a cleanup pass. Five findings, no correctness blocker, and every one of them a claim that had stopped matching the code. the frame seam did not fire for every arm, though its comment said so. The arms that decline to act — a heartbeat, an undecodable payload, an unsubscribe confirmation — were `continue` statements, which skipped everything after the switch. A test waiting on the seam for one of those frames would have HUNG rather than failed, which is the worst way to find this out. The switch is now its own method so every arm ends the frame by returning, and a test drives one frame per publisher-reachable arm and counts three. Detected against restoring the skip. the idle-cycle warning was emitted before the revalidation that can abandon the cycle, so it could announce coverage ending and resumes answering sync_required for a subscription that was then left alone — a log line with no counter behind it, and an on-call hunting a bug that is not there. internal/events learned this at its own round 6; the reason did not come across with the port. Moved after the decision is final, still saying "attempting" to replace because the resubscribe can fail. the quiet-exit test sampled liveReceiveLoops instead of waiting for it, so its "the replaced loop left" assertion could be satisfied by a loop that never ran. Same defect fixed in the sibling concurrency test a commit earlier and missed here, because I looked at the test the reviewer named rather than at the pattern. Latent rather than observed: sampling survives 10 runs, so this removes a possibility. the probe-failure log and metric help called an errored Publish a failure to publish. A returned error can also mean the reply was lost after Redis accepted the frame, so the honest claim is that the probe is UNCONFIRMED. It changes no behaviour — an unconfirmed probe is not evidence about the receive path either, so detection suspends the same way — but an operator reading the counter should not be told more than the instance knows. the deployment doc said the watch stream differs in "three things" and listed four, the fourth being the bullet I added last round. Third instance of that species on this branch; the count is gone rather than corrected. Refs BUG-2769 * docs(watchevents): stop one unconfirmed probe standing in for a broken path Codex round 10 confirmed four of round 9's five fixes and held the fifth as partial. It was right on all three residual sites. Renaming the condition to "could not confirm" did not fix the sentences downstream of it. The log still said silence cannot be read as a finding "when we could not ask" — but we may well have asked, and lost only the answer. And both the metric help and the observer contract said an instance in this state "is also failing to deliver its own notifications to every other instance", which is a conclusion about the outbound path drawn from a single call that did not come back. The inference is sound at a SUSTAINED rate and worthless at one increment, so both now say which is which. That distinction is the whole value of the counter to an on-call: a blip is a lost reply, a rate is a broken path, and the same wording for both makes the first look like the second. No behaviour change. An unconfirmed probe suspends detection exactly as a definite failure does, because it is not evidence about the receive path either way. Refs BUG-2769 * docs: sweep the BUG-2738 prose this change makes false BUG-2738 shipped documentation that describes the watch stream as still carrying the half-open defect. Merging this makes those sentences wrong, and I flagged the sweep as owed twice during the groundwork and then did not do it — the lead caught that the package said nothing about it. Five sites, each re-read after editing rather than grepped for, because grepping for a phrasing I chose is how I have twice verified a sweep that had not landed: the residual enumeration opened "One gap remains everywhere, and a second remains on the watch stream only", then described one gap and said it was open on both. The second WAS the half-open case. Now states one gap, on both streams, and says where the second went. the half-open paragraph already said "closed on both streams" — the one site I had fixed — but omitted that each half is behind its own phase-2 flag, so a reader takes it as closed on their deployment when it is closed only once they turn it on. "A third residual" counted the item it followed. With the second gone the ordinal was wrong; it does not need one. "these two gaps" in the closing sentence, same arithmetic. the pad_event_subscription_cycled_total row told an operator to read heartbeat_phase off the startup log. There are now two such fields on two lines under two flags, and only one bears on that counter. It names the line. No code change; suite 28/28 and lint 0 re-run because the branch is under review and a docs commit that skips them is a commit nobody checked. Refs BUG-2769	2026-08-25 11:04:18 -04:00
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	5f8252ce88	test(events): cover the migration's central claim and three untested branches (BUG-2736) ... Codex round 12 asked only "what has no test at all", which is a different question from "is each fix pinned" and found things twelve rounds of the second question had not. THE CENTRAL CLAIM HAD NO TEST. Parsing, publishing and fan-out were each exercised alone; that a phase-1 and a phase-2 instance on one Redis deliver each other's events -- the thing the whole two-phase design rests on -- needed both buses at once and nothing did that. Now both directions, with both receivers asserted, and the premise that the two payload forms really did differ. THE BACKWARDS GUARD IS `<=` AND EVERY TEST USED A STRICTLY LOWER ID. A `<` implementation passed all of them while letting a REPEATED id into the buffer -- a duplicate delivery and a replay that can serve the same id twice, with no reset reported. Mutation-checked. THE ROLLBACK PRECEDENCE HAD NO TEST. The procedure tells an operator to make the EFFECTIVE value false and warns that unsetting the env var is not the same thing; neither half was pinned, so a load order letting the file win over an explicit env-var false would have kept a deployment on phase 2 while its operator believed they had rolled back. AND THE RESIDUAL IS NOW EXECUTABLE. A bus with empty buffers adopts an epoch without dropping, so its first buffer starts at the first id it sees and a client holding the id one below is served. That was described in three places and asserted in none. It is now a characterization test that states the load trade in its own comment, so a future change to it is a decision rather than a side effect -- and the boundary is pinned exactly: adjacent is served, one lower is a gap. Declined with reasons: the startup phase log cannot be driven without standing up a server and its wrongness is visible on first read; the race between a phase-1 epoch delete and a concurrent phase-2 mint is documented, bounded at one extra buffer drop, and not deterministically reproducible; and the cross-Redis failover retry needs two Redises with controlled replication lag, which miniredis cannot represent honestly. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 20:34:31 +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	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	94cc2492fc	fix(events): a phase-1 counter restart must not leave a live epoch behind (BUG-2736) ... Codex round 2, on the rollout angle. Four findings; one was a real silent-loss hole and three were claims in the docs and config comment that the code does not support. THE HOLE. Phase 2 mints an epoch and the counter climbs; the deployment rolls back to phase 1; the seq key is then evicted or deleted; phase-1 publishers climb from 1 again; phase 2 is re-enabled and its SET NX finds the OLD epoch still there. A receiver that had adopted it sees no change, and if its high-water mark is below the new sequence -- a replica that just started, or one whose buffers were empty -- the numeric check does not see the reset either. Two ID spaces merge in one buffer silently, which is the outcome this whole unit exists to prevent. Phase 2's rotation cannot cover it: that rotation fires when the SCRIPT's own INCR returns 1, and by then the counter has climbed past 1 under the phase-1 path. So phase 1 now deletes the epoch when its own INCR returns 1. Deleting rather than rotating, because that path publishes no epoch and has none to propose, and an absent key is what phase 2's SET NX expects. The cost is one extra buffer drop if a phase-1 publisher deletes an epoch a flipped publisher just minted during the phase-2 roll -- loud and bounded, which is the direction this family always chooses over a silent merge. THE THREE CLAIMS. - "Rolling back is symmetric: unset the variable and roll" was true only of the roll back to PHASE 1. Downgrading past it is a second step in reverse order, because a pre-phase-1 binary still cannot parse the prefix, and introducing one while any flipped instance publishes drops events on it. - Unsetting the environment variable is not the same as setting the value false: events_publish_epoch can come from config.toml, whose value stands when the variable is absent. - counter_backward was documented as expected during mixed-version rolls and near zero between them. On phase 1 it can be non-zero at any time: that path keeps the two-call INCR-then-PUBLISH, so instances can interleave. The expectation is now stated per phase. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 18:47:18 +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	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	d6e153a583	fix(test): the new config test needed a writable HOME (Nix CI) ... `config.Load()` resolves and CREATES ~/.pad, so TestStreamAndRedisEnvMapping passed on any machine with a writable HOME and failed in the Nix check sandbox, which sets HOME to an unwritable path: "Load: mkdir /homeless-shelter: permission denied". Every other test in this package already sets HOME to a t.TempDir() for exactly this reason. Mine did not, and no local gate could tell me — build, lint, `go test ./...` and `-race` all run with my HOME writable. Only the Nix leg exercises the sandboxed shape, which is the second time that leg has been the sole instrument for a class the whole local matrix is blind to. Verified both directions locally by compiling the test binary and running it under a read-only HOME rather than trusting CI to re-run: fixed passes, unfixed fails with the same message the sandbox reported. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 05:44:32 +00:00
xarmian	c10f1200c1	test: close the coverage gaps codex round 5 enumerated (BUG-2724, BUG-2726, BUG-2727) ... Round 5 asked what has NO instrument, as opposed to what is exercised incidentally. Six real gaps, each now closed by a test that fails when the behaviour is reverted (10 mutations applied, 10 caught): - Config plumbing for PAD_REDIS_NAMESPACE and PAD_SSE_MAX_PER_USER, and the per-user default of 50. The parser and the gate each had tests; a Load() that never populated either field would have passed both while the deployment ran with no namespace and no per-user bound. Same knob-versus-wiring split as day-49's batch-id finding. - The metrics adapter's MAPPING. Both sides of that seam were tested and neither proved the wires were not crossed; an adapter that incremented the sequence-gap counter on a resume gap would have passed everything and sent an incident the wrong way. Each event now fires a different number of times so a crossed wire cannot produce the expected totals. - The REDIS bus's slow-subscriber drop. Only the memory bus's was covered, and they are different loops in different files — the Redis one being the only one a multi-instance deployment runs. - Presence renew and deregister failures. They fail in OPPOSITE directions, which is why they are counted separately, and neither was instrumented. - events.EventBus.SubscribeIfAllowed's own bounds, which had no package-level test at all. BUG-2726 moved the global bound to the admission gate, so the handler passes maxGlobal=0 and that branch is now unreachable from production — it would have been a branch nobody could vouch for. Both bounds are now tested where they live. Also records what round 5 correctly noticed about the existing SSE limit tests: they still pass, but for a DIFFERENT reason than before — the admission gate refuses before the bus is reached. Their names no longer say what they exercise, so the call site says it instead. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-22 02:38:39 +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	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	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	3f69b76b06	feat(security): enforce session UA binding under strict mode (TASK-2056) (#912) ... Session IP/User-Agent binding was log-only by default, so a stolen session token granted durable any-origin access. IP-change enforcement already existed behind PAD_IP_CHANGE_ENFORCE=strict; this extends the same single toggle to also enforce the User-Agent-hash binding. When strict enforce is ON, a request whose client IP OR User-Agent hash no longer matches the session's stored binding now revokes the session (DeleteSessionIfExists) and rejects the request (401 for API, revoked-passthrough for public/browser paths), killing the stolen token. When enforce is OFF (default), behavior is unchanged: UA mismatch is logged (slog only, no new audit row) and the request proceeds, so existing self-host users see no behavior change and routine client churn (browser/WebView updates, DevTools emulation, mobile-app rebuilds) is tolerated. The UA hash is stable within a real session, so UA-mismatch enforce carries fewer false positives than IP enforce (mobile roaming, VPN toggles, carrier NAT) — documented in the handler comment. Adds the ActionSessionUAChanged audit action, emitted only in strict mode. No DB migration: reuses the existing IPChangeEnforce config flag and the existing session store primitives. Claude-Session: https://claude.ai/code/session_015yuBJQYfDj95cgX3DaD8SF	2026-07-10 23:32:22 -04:00
xarmian	d36f27c29f	fix(server): auth-perimeter hardening — B6–B9 from the IDEA-1927 audit (TASK-1932) (#811) ... * fix(server): stop autoCreateWorkspace from swallowing member-add errors (B6, TASK-1932) A failed AddWorkspaceMember after workspace creation used to be silently discarded, leaving a workspace that's completely unreachable (owner_id alone grants no access) and invisible in the console forever. Retry once, then clean up the orphaned workspace and log loudly on continued failure so on-call can act on it. * fix(server): fail fast when cloud mode runs without secure cookies (B7, TASK-1932) SetCloudMode never forced secureCookies on, so PAD_CLOUD=true without PAD_SECURE_COOKIES was an unenforced ops contract: OAuth's __Host-prefixed session cookie is silently invisible to pad's own cookie reader without Secure set, producing a "logged in but appears logged out" failure mode. Add Config.ValidateCloudSecureCookies and check it at server startup, next to the existing PAD_CLOUD_SECRET requirement, so the misconfiguration is a startup error instead of a runtime mystery. * fix(server): align OAuth session TTL with web session TTL (B9, TASK-1932) handleOAuthLogin minted a 30-day session while every other web login used the 7-day webSessionTTL. createAuthSession derives the store session row, session cookie MaxAge, and CSRF cookie MaxAge all from one ttl argument, so the longer OAuth cookie outlived its own server-side session — the browser kept presenting a cookie whose session had already expired, producing silent 401s. Use webSessionTTL for OAuth logins too. * fix(server): narrow the /api/v1/auth/* CSRF exemption to anonymous endpoints (B8, TASK-1932) The CSRF middleware exempted the entire /api/v1/auth/ prefix, which also covered mutating cookie-authenticated endpoints: PATCH /me, oauth-unlink, 2FA setup/verify/disable, delete-account, token create/delete/rotate, CLI- session approve, and logout. Replace the prefix bypass with an exact-path allowlist of the endpoints that are genuinely pre-session (login, register, bootstrap, password reset, verify-email, resend-verification, 2FA login challenge, CLI session create) or authenticate purely via a cloud secret rather than a cookie (oauth-login, oauth-link — never touch the session, so CSRF isn't a meaningful threat model for them and the sidecar has no CSRF cookie to send). Everything else now requires the double-submit token like any other authenticated mutation; the web client already sends it on every non-GET/HEAD request, so no frontend change is needed. * docs(server): pin the deliberate CSRF-cookie legacy-fallback asymmetry (TASK-1932) Codex review (round 1) flagged that SessionAuth falls back from the __Host-pad_session cookie to the legacy unprefixed name, but the CSRF cookie lookup has no equivalent fallback — meaning a browser holding pre-secure-cookies-flip legacy cookies stays authenticated but gets 403'd on B8's newly CSRF-required endpoints until it re-logs-in. This asymmetry is deliberate, not a bug: the session cookie's value is an unguessable secret regardless of which name carries it, but the CSRF cookie's security property depends on the attacker being unable to set the cookie itself — an unprefixed name is settable from a sibling subdomain, which is exactly the hole __Host- exists to close. Restoring "symmetry" here would silently reopen it. Document the reasoning at the cookie lookup so a future maintainer doesn't "fix" it, and add a pinning test that exercises the exact scenario (secureCookies=true, legacy session + CSRF cookies, CSRF-required endpoint) end to end. * fix(server): require CSRF for session-authenticated requests to exempt auth paths (TASK-1932) Codex round 2 found a P1: handleRegister has an admin-session branch (an already-logged-in admin can create a verified account with no invitation code), but /api/v1/auth/register was unconditionally CSRF-exempt by path. A cross-site POST could ride the admin's cookie into that branch with no CSRF token — the same class of hole as the oauth-unlink case B8 already closed, just missed because register's other paths are genuinely anonymous. Fix generically rather than register-specifically: gate the authCSRFExemptPaths exemption on currentUser(r) == nil. SessionAuth runs before CSRFProtect, so a request that resolved to a real session falls through to the normal double-submit check instead of the early exemption, while a genuinely anonymous request keeps it. This also covers any future session-authenticated branch a handler on this list grows, with no handler changes. Bearer/PAT and cloud-secret (oauth-login/oauth-link) callers are unaffected — they have their own unconditional exemptions later in the same function. * fix(server): require validated Bearer/cloud-secret auth for CSRF exemption (TASK-1932) Codex round 3 found that CSRFProtect's Bearer and X-Cloud-Secret exemptions fired on header/marker PRESENCE, not validation. TokenAuth deliberately falls through (rejectInvalidBearer) instead of 401ing invalid Bearers on /api/v1/auth/* paths to support CLI-token recovery, so a cross-site request carrying a victim's real session cookie plus a garbage Bearer header could ride the cookie past CSRF on any newly-CSRF-required endpoint. The same presence-only pattern in the X-Cloud-Secret exemption is concretely exploitable too: handleSetPlan (and similarly-shaped handlers) accept an admin cookie session as an alternative to the secret, so a garbage X-Cloud-Secret plus a stolen admin cookie could set an arbitrary user's plan with no CSRF token at all. Add ctxValidatedSessionBearer (set by TokenAuth only on successful ValidateSession for CLI session-bearer tokens) alongside the existing ctxIsAPIToken, and a combined isValidatedBearerAuth() helper. CSRFProtect now exempts unconditionally only on validated Bearer auth; an unvalidated Bearer header or cloud-secret marker is exempt only when no session was also resolved for the request (currentUser(r) == nil), preserving the CLI-recovery contract (stale token, no cookie -> 401 from auth, not csrf_error) while closing the cookie-riding case. * fix(server): split CSRF auth-exempt allowlist by session sensitivity (TASK-1932) Codex round 2 gated the entire authCSRFExemptPaths allowlist on currentUser(r) == nil to close handleRegister's admin-session branch, but that gate applied to every anonymous endpoint on the list, not just register. CI's E2E suite caught the regression: the harness bootstraps an admin (minting a session cookie) then POSTs /login to re-authenticate, and the ambient cookie stripped /login of its exemption, producing a spurious 403 csrf_error. login/bootstrap/forgot-password/reset-password/local-reset/verify-email/ resend-verification/2fa-login-verify/oauth-login/oauth-link/cli-sessions- create derive their authority entirely from the request body (credentials, a token, a shared secret), never from the ambient cookie, and pad mints the CSRF cookie AT LOGIN — a pre-session endpoint categorically cannot require a token that doesn't exist yet. Split the allowlist: authCSRFUnconditionalExemptPaths (everything above, exempt regardless of cookie) and authCSRFSessionGatedExemptPaths (register only, exempt only when currentUser(r) == nil, since it alone has a session-privileged admin-account-creation branch). The round-2 security property (admin session + register + no CSRF -> still blocked) and round-3's Bearer/ cloud-secret validated-vs-present composite are unaffected.	2026-07-04 11:33:18 -04:00
xarmian	db87b47754	fix(cli): pin server URL in .pad.toml for remote workspaces (BUG-1535) (#595) ... * fix(cli): pin server URL in .pad.toml for remote workspaces (BUG-1535) Two fixes: 1. Replace stale api.getpad.dev references with app.getpad.dev in the --url flag help, NewClientFromURL doc, and Config.URL doc. Also fix internal/mcp/dispatch_http.go's comment to use the canonical mcp.getpad.dev/mcp URL. 2. Persist the server URL into .pad.toml when linking a directory to a non-local workspace. WriteWorkspaceLink now takes a serverURL arg; pad init / workspace link / workspace switch pass cfg.BaseURL() when Mode != local. getConfig() reads .pad.toml's URL as an override above ~/.pad/config.toml and below the --url flag, so commands like `pad collection list` from a remote-linked directory hit the right server without --url on every call. Passing --url explicitly also promotes local → remote so the directory pin is written even when the existing global config has mode=local. * fix(cli): scope .pad.toml URL override to client paths per Codex review (round 1) Round 1 review flagged that applying the .pad.toml URL override inside getConfig() contaminates server/admin commands: pad server start would advertise the wrong PublicLinkBaseURL, and pad auth setup would refuse to run locally because Mode flipped to remote. Extract the override into applyPadTomlOverride() and call it only from client-API entry points — getConfiguredConfig() and the pad init client phase. Server/admin commands (pad server start/stop, pad auth setup, pad auth configure) keep using raw getConfig() and are unaffected. Also skip the override when --url was explicitly passed (LoadedFromFlags), so the flag retains unambiguous priority. * fix(cli): preserve .pad.toml URL on workspace link/switch per Codex review (round 2) Round 2 review noted workspace link / workspace switch reached the server via getClient() (override applied) but then wrote the new .pad.toml URL using a raw getConfig() — which would drop or miswrite the url field when relinking inside a remote-pinned directory whose global config is local. Reuse the cfg returned by getClient() for padTomlURLFor so the write matches the API client.	2026-05-19 17:02:46 -04:00
xarmian	ba303e456f	fix(mcp): publish PAD_MCP_PUBLIC_URL verbatim as canonical resource (no /mcp suffix) (#381) ... Per the MCP authorization spec the client MUST verify the URL it was given matches the discovery doc's `resource` field exactly; auto- suffixing was forcing operators publishing the bare hostname (the industry convention — mcp.stripe.com, mcp.linear.app, mcp.atlassian.com) into a permanent client-side mismatch and Claude Desktop / Cursor reject pasting `https://mcp.getpad.dev` even though everything else works. Both production sites that previously appended "/mcp" to MCPPublicURL now use the value verbatim: - cmd/pad/main.go: AllowedAudience for the OAuth server constructor. Tokens are now audience-bound to MCPPublicURL exactly. - internal/server/handlers_well_known.go: the protected-resource discovery doc's `resource` field is the bare MCPPublicURL. The transport itself is unchanged — pad still mounts at /mcp on the chi router; pad-cloud's nginx router transparently rewrites mcp.* root → /mcp (TASK-997 PR #28) so external clients see a single canonical URL regardless of the internal HTTP path. The audience binding is just a string; it doesn't have to equal the internal mount path. config.go's MCPPublicURL doc updated to reflect the new semantic ("canonical URL clients paste") rather than the old "vhost URL we suffix-mangle". Operators who want the old shape just include the /mcp suffix in PAD_MCP_PUBLIC_URL — the operator owns the canonical. Test fixtures: testCanonicalAudience flipped from "https://mcp.test.example/mcp" to "https://mcp.test.example", and the two SetMCPTransport call sites that previously stripped /mcp now pass it directly. The TestMCP_DiscoveryDoc_PopulatedFromConfig assertion uses testCanonicalAudience so future renames stay consistent. All other test sites (audience= form fields, aud claim checks, mismatch fixtures) keep working unchanged because they reference testCanonicalAudience symbolically.	2026-05-02 18:36:55 -04:00
xarmian	521853e0a1	feat(mcp): mount /mcp Streamable HTTP transport + OAuth discovery (TASK-950) (#369) ... * feat(mcp): mount /mcp Streamable HTTP transport + OAuth discovery (TASK-950) First public cut of pad-cloud as a remote MCP server (PLAN-943). Mounts the Streamable HTTP transport on /mcp, the RFC 9728 protected-resource discovery doc on /.well-known/oauth-protected-resource, and a 501 stub for RFC 8414 auth-server metadata that TASK-951 will fill in. - internal/server/handlers_mcp.go — Server.SetMCPTransport + chi route registration under cloud-mode gate (self-host stays free of MCP overhead unless explicitly opted in). - internal/server/middleware_mcp_auth.go — Bearer auth that produces the spec-shape 401 + WWW-Authenticate (resource_metadata pointer) MCP clients expect, distinct from /api/v1's JSON-only 401 envelope. Reuses the existing PAT (api_tokens) validation path; OAuth-issued tokens layer in via this same middleware in TASK-951. - internal/server/handlers_well_known.go — RFC 9728 discovery doc + RFC 8414 stub. URLs come from PAD_MCP_PUBLIC_URL + PAD_AUTH_SERVER_URL with request-host fallback for local dev. - internal/server/handlers_mcp_test.go — 7 tests covering cloud-off routes-absent, cloud-on-no-transport routes-absent, discovery doc shape, 501 stub, no-token 401+WWW-Authenticate, bad-format-token 401+WWW-Authenticate, and the valid-PAT happy path with user attached to transport context. - cmd/pad/main.go — wires mcpserver.NewServer + HTTPHandlerDispatcher + StreamableHTTPServer in cloud mode, after SetCloudMode. - internal/config — adds PAD_MCP_PUBLIC_URL and PAD_AUTH_SERVER_URL. Resources are intentionally skipped in this v1 — they require an HTTPResourceFetcher equivalent of ExecResourceFetcher and that's a follow-up task. Tools, prompts, instructions, and meta all flow through identically to the stdio surface (verified via spike against mcp-go v0.50.0's StreamableHTTPServer before writing the real PR). * fix(mcp): enforce PAT scopes on /mcp + WWW-Authenticate fallback per Codex review (round 1) Two findings from PR #369 round 1: 1. SECURITY: A PAT with scopes ["read"] could drive write MCP tools. MCPBearerAuth skipped tokenScopeAllows entirely; the dispatcher's synthesized in-process request bypassed TokenAuth's chain-level check (because WithCurrentUser was already set), so a read-scoped token could POST item create / PATCH update / DELETE silently. Fix: stash apiToken.Scopes via server.WithTokenScopes in MCPBearerAuth; re-check per synthesized request in HTTPHandlerDispatcher.executeRequest using the public server.TokenScopeAllows wrapper. Read-scoped tokens can still drive read-only tools (their HTTP method is GET) — only writes are rejected, with a structured permission_denied envelope. 2. DISCOVERY: writeMCPUnauthorized dropped the WWW-Authenticate header when PAD_MCP_PUBLIC_URL was unset. Cloud-mode deploys without that env var mounted /mcp but broke the discovery handshake — fresh MCP clients rely on the header to find /.well-known/oauth-protected- resource. Fix: pass *http.Request through to writeMCPUnauthorized, derive "https://" + r.Host as the fallback (matches handleOAuthProtected- Resource's existing fallback). Tests: - handlers_mcp_test.go: TestMCP_NoToken_FallsBackToHostWhenPublicURLUnset pins the WWW-Authenticate fallback. TestMCP_ReadScopedPAT_StashesScopes- InContext + TestTokenScopeAllows_PublicWrapper pin the scope-stash side. - dispatch_http_test.go: TestHTTPHandlerDispatcher_ScopeEnforcement_* pin the dispatcher-side enforcement (read-on-write rejected, read-on-read allowed, no-scope-context allows-all). - recordingHandler updated to handle nil r.Body so the read-only GET path can be exercised. * fix(mcp): move scope check to buildAuthedRequest so bulk-update can't bypass it per Codex review (round 2) Round 1 enforced scopes in executeRequest, but dispatch_http_project.go's item bulk-update path constructs each per-item PATCH directly via buildAuthedRequest + d.Handler.ServeHTTP, skipping executeRequest. Net result: a PAT with ["read"] scope could still mutate items through bulk-update even after the round-1 fix. Move the scope check from executeRequest into buildAuthedRequest so every synthesized request — main writes, RMW prefetches, bulk-update per-item PATCHes, link-create POSTs, attachment HEADs — passes through the same gate uniformly. The check is dropped from executeRequest to avoid double-checking; buildAuthedRequest is the universal funnel everything calls. Reads (GET/HEAD/OPTIONS) under ["read"] scope still pass — bulk- update's per-item GET prefetch succeeds, the subsequent PATCH fails at request-build time with permission_denied. The bulk operation returns successfully with all-errors recorded per ref (the "no abort on per-item failure" contract is unchanged). Test: TestHTTPHandlerDispatcher_ScopeEnforcement_BulkUpdateBlockedOnReadScope spies on the test handler; asserts the PATCH never reaches it under ["read"] scope and that each per-item entry carries permission_denied.	2026-05-02 09:25:25 -04:00
xarmian	f8ed3e10a7	fix(search): explicit selection on Enter + numeric go-to (BUG-864, BUG-910) (#320) ... * fix(search): require explicit selection on Enter; add bare-number go-to (BUG-864, BUG-910) The command palette had two related issues: - BUG-864: Pressing Enter armed the first search result automatically — the user could close the modal and navigate without ever pressing an arrow key. selectedIdx now starts at -1 and only advances on ArrowDown/ArrowUp. - BUG-910: Typing a bare number (e.g. "843") returned no results because parseItemRef requires PREFIX-NUMBER and FTS doesn't index item_number. Backend (internal/store): - Add parseItemNumber() helper alongside parseItemRef. - In Search(), add a bare-numeric direct-lookup path that mirrors the existing ref-lookup block but without a collection prefix filter. item_number is unique per workspace (idx_items_workspace_number) so this resolves to at most one direct hit, prepended with rank=-1000. Frontend (CommandPalette.svelte): - selectedIdx defaults to -1; reset to -1 (not 0) on modal open and after every search. - Enter on a non-numeric query is a no-op unless the user has arrow-selected. - Numeric queries are a deliberate exception: Enter on a bare-number query flushes the debounce, navigates directly to the matching item, and lets the search palette double as a quick "go to item N" jump. Tests: - TestSearch_BareNumericQueryFindsItemByNumber covers the new path. - TestParseItemNumber covers helper edge cases. * fix(search): exclude direct hits from FTS WHERE to keep pagination correct Codex review (round 1) on PR #320: > Numeric direct hits are appended before the FTS query, but the later > pagination only removes duplicates after SQL LIMIT/OFFSET. If item #2 > also matches FTS for query "2" through its title/content, that > duplicate consumes an FTS slot, so page 1 can return fewer than `limit` > results and later pages can repeat/skip rows. Hoist the direct-hit (ref + numeric) snapshot to before the FTS query is built, then append `AND i.id NOT IN (...)` to both the SELECT and COUNT FTS queries. After a successful count, add refCount back so SearchResponse.Total still reflects the full result set (since FTS itself no longer counts those rows). The flaw also applied to the pre-existing parseItemRef path; this fix covers both. The post-LIMIT dedup loop is now defense-in-depth. New test TestSearch_BareNumericQueryDedupsAgainstFTS guards the case: an item whose title/content literally contains its own item_number (so it matches both the direct lookup and FTS) appears exactly once in Results and Total counts it exactly once. * fix(search): paginate direct hits properly across workspaces Codex review (round 2) on PR #320: > P1: Bare numeric direct hits break pagination in global search. > item_number is only unique per workspace, so q=1 with WorkspaceIDs > spanning N workspaces returns N direct hits — all appended without > being sliced to Limit. limit=1 with three workspaces each having #1 > returns three results on page 0, and offset=1 drops all direct hits > then returns FTS rows instead of the second direct hit. The same flaw applied to the pre-existing parseItemRef path: the global search "TASK-5" can match TASK-5 in multiple workspaces. Fix: - Add deterministic ORDER BY i.workspace_id, i.id to both ref and bare- numeric direct-hit lookups so pagination is stable across pages. - Replace the offset==0/offset>0 branching pagination with a uniform slice: directStart = min(Offset, refCount); directEnd = min(Offset+Limit, refCount); results = results[directStart:directEnd]; ftsLimit = Limit - directConsumed; ftsOffset = max(Offset - refCount, 0). This honours (offset, limit) whether direct hits, FTS, or both fill the page. Total stays correct because the FTS count was already excluding direct hits (round-1 fix) and we add refCount back unconditionally. New test TestSearch_BareNumericQueryPaginatesAcrossWorkspaces creates three workspaces each with item #1 and verifies that limit=1 with offsets 0/1/2 returns three different direct hits in stable order, and limit=10 returns all three. * chore: gofmt — column alignment in struct field declarations CI Go (SQLite) lint failed on two files: - internal/store/store_test.go (TestParseItemNumber, this PR's new test) — unaligned column widths and inconsistent comment spacing. - internal/config/config.go (drive-by) — pre-existing alignment regression in the Config struct that snuck in via an earlier landed PR; included here because it blocks merge. No semantic changes — `gofmt -w` only.	2026-04-30 14:23:23 -04:00
xarmian	10309fc599	fix(config): read PUBLIC_URL for emailed link generation (BUG-899) (#318) ... * fix(config): read PUBLIC_URL for emailed link generation (BUG-899) The Pad Cloud deployment binds pad to 0.0.0.0 (Dockerfile, k8s configmap, pad-cloud's docker-compose) and never set PAD_URL on the pad service, so cfg.BaseURL() fell through to "http://0.0.0.0:7777" — that string ended up in password-reset (and invite + share-link + admin-invitation) emails and was unreachable to recipients. Adds a PUBLIC_URL env var read by the server only (does not flip CLI to remote mode the way PAD_URL does — PUBLIC_URL is a generic env var name commonly set in unrelated deployment contexts). Stored in a separate Config.PublicURL field consulted by BaseURL() as a fallback after URL. Resolution order in BaseURL(): PAD_URL > PUBLIC_URL > host:port. Also logs a WARN at server startup if the resolved base URL has an unspecified bind-all host (0.0.0.0, ::, [::]) — a backstop that would have caught BUG-899 the first time email went out. Tests cover the precedence ladder, mode-not-flipping, PAD_URL-beats- PUBLIC_URL, and the BUG-899 repro shape (Host=0.0.0.0 with no URL set yields the broken http://0.0.0.0 URL). Companion change in pad-cloud/docker-compose.yml passes PUBLIC_URL through to the pad service so the Cloud deployment stops shipping broken email links. Parent: BUG-899 (TASK-908). * fix(config): keep PUBLIC_URL out of IsConfigured() per Codex review (round 2) PUBLIC_URL was setting LoadedFromEnv = true, which IsConfigured() consults to decide whether the CLI has explicit configuration. A generic PUBLIC_URL in the environment (very common name) would have made any host appear "configured" to the CLI and skipped the not-configured / setup branch — the exact footgun the separate-field design was supposed to avoid. PUBLIC_URL is purely a server-side fact; LoadedFromEnv is purely a CLI affordance. Stop conflating them. Adds a focused regression test pinning the IsConfigured() invariant. * fix(config): split PublicLinkBaseURL from BaseURL per Codex review (round 3) Round 2's BaseURL fall-through to PublicURL leaked PUBLIC_URL into ~20 CLI-client call sites (cli.NewClientFromURL(cfg.BaseURL()) patterns across cmd/pad/main.go, init.go, server_info.go, configure.go) — same footgun the separate-field design was meant to avoid: a developer with a host-level PUBLIC_URL set for unrelated reasons would have their CLI silently route requests to that URL instead of the local server. Restore BaseURL() to its original CLI-only contract (URL > host:port). Add PublicLinkBaseURL() with the URL > PublicURL > host:port ladder that's used at exactly the two server-side call sites that build emailed-link targets: - cmd/pad/main.go:279 srv.SetBaseURL(cfg.PublicLinkBaseURL()) - cmd/pad/main.go:464 email.NewSender(..., cfg.PublicLinkBaseURL()) Tests pin both contracts: BaseURL() ignores PublicURL even when set; PublicLinkBaseURL() honors the precedence ladder. PAD_URL still wins in both, preserving back-compat. * fix(config): drop public_url toml tag to prevent CLI persistence per Codex review (round 4) Round 3 left PublicURL serializable to ~/.pad/config.toml via toml: "public_url". A CLI user who runs `pad init` or `pad configure` on a host where PUBLIC_URL is set for unrelated reasons would end up with that URL persisted into their config file, surviving any later unset of the env var and contaminating server-side emailed link generation indefinitely (server reads ~/.pad/config.toml on the next boot). Switch the field to toml:"-". PUBLIC_URL is a deployment-time fact (env var / docker-compose / k8s); operators who want a config-file equivalent already have `url` (the PAD_URL path), which serializes properly. Adds a regression test pinning that Save() never writes PublicURL to the file.	2026-04-30 08:40:33 -04:00
xarmian	afe721d202	feat(cli): add Cloud mode to pad init, drop Docker option (TASK-837, TASK-838) (#272) ... Merging despite Go (PostgreSQL) red — those failures (TestListItems_FTS_HyphenatedSearchTerm/task-five + TestAdminBillingStats_SidecarSidecarError_DegradesToLocalOnly TempDir cleanup race) are pre-existing on main and tracked in BUG-842. Codex reviewed in 3 rounds (round 1 clean → round 2 found a real semantic bug → fix → round 3 clean). Tests, vet, and lint all green; remaining check failures are documented pre-existing.	2026-04-28 09:41:50 -04:00
xarmian	189b22825e	fix(cli): use cfg.BaseURL()/BrowserURL() in pad init success message + pad open (TASK-834) (#269) ... * fix(cli): use cfg.BaseURL() in pad init success message (TASK-834) The "Or open the web UI at http://localhost:7777" line in printOnboardingHints was hardcoded, which is wrong for any non-local connection mode (Remote, Docker, eventual Cloud). The CLI already knows the configured base URL — it just used it to talk to the server. Same hardcoded URL existed in the workspace-onboard skip path ("You can activate conventions from the library: ..."). Both call sites now use cfg.BaseURL(), which yields the correct URL for every mode: - Local: http://127.0.0.1:7777 (default host:port) - Remote/Docker/Cloud: the configured URL (e.g. https://app.getpad.dev) printOnboardingHints now takes a *config.Config; both call sites already had cfg in scope. Parent: PLAN-833 (pad init UX gaps + Pad Cloud onboarding fixes). Source: IDEA-831 issue #5. * fix(config): add BrowserURL() that normalizes 0.0.0.0 to 127.0.0.1 Per Codex review (round 1): when local mode runs with --host 0.0.0.0 (bind-all), cfg.BaseURL() returned "http://0.0.0.0:7777" — a bind address that browsers don't reliably accept. BrowserURL() behaves like BaseURL() except that when constructing from host:port, an unspecified bind-all host (empty, "0.0.0.0", "::", "[::]") is rewritten to "127.0.0.1". Explicit URL configurations (Remote/Docker/Cloud) are returned unchanged. The two onboarding-hint call sites updated in the previous commit now use BrowserURL() so the success message and skip-path show a clickable URL in every supported configuration. Tests cover loopback, named hosts, empty/0.0.0.0/::/[::] normalization, and explicit-URL precedence. Parent: PLAN-833. * fix(cli): use BrowserURL() in pad open for bind-all safety Per Codex review (round 2): the 'pad open' command prints and opens cfg.BaseURL(), which produces 'http://0.0.0.0:7777' when the local server is bound bind-all. Same class of bug as the onboarding hint fix in this PR — switch to cfg.BrowserURL() so the URL is a usable browser destination. A second related issue Codex flagged — the server-issued CLI auth URL in doBrowserLogin (which goes through internal/server/handlers_cli_auth.go using r.Host) — is a different surface with multiple possible fix strategies and overlaps with the post-v0.1.0 OAuth-architecture work. Deferred to TASK-839 with a written-up runbook so it isn't lost. Parent: PLAN-833.	2026-04-27 21:48:50 -04:00
xarmian	157ca4e88f	chore: bump Go toolchain to 1.26 (TASK-763) (#247) ... * chore: bump Go toolchain to 1.26 (TASK-763) Bump Go from 1.25 to 1.26 across all toolchain pins: - go.mod — go 1.25.0 → go 1.26.0 - Dockerfile — golang:1.25-alpine → golang:1.26-alpine - .github/workflows/ci.yml — three setup-go steps (Go, Go-Postgres, E2E jobs) - .github/workflows/release.yml — release pipeline No `toolchain` directive: the repo is pre-launch with no external contributors yet, so we set the floor where we want it (hard requirement). Verified locally before commit: - golangci-lint v2.11.4 builds and runs under Go 1.26.2 (pinned in CI) - golang:1.26-alpine and 1.26.2-alpine images present on Docker Hub - go build ./... clean - go vet ./... clean - go test ./... all pass Parent: PLAN-644 (OSS Repo Hygiene and Launch Polish). * chore: gofmt -w under Go 1.26 (TASK-763) Apply Go 1.26's gofmt to the codebase. ~41 files reformatted, all struct-tag whitespace realignment — no semantic changes. Verified: - gofmt -l ./cmd ./internal returns empty after - go build ./... still clean - go test ./... still passes (run before commit) Bundling the gofmt diff with the toolchain bump in the same PR because the formatting drift is a direct consequence of moving from 1.25 to 1.26; splitting them creates a mandatory two-PR ordering for no value. Parent: PLAN-644. * docs: bump documented Go floor to 1.26 (TASK-763) Match go.mod's hard 1.26.0 requirement in the source-build instructions. Caught by Codex review round 1 on PR #247. - README.md:158 — "Go 1.25+" → "Go 1.26+" - CONTRIBUTING.md:9 — "Go 1.25+" → "Go 1.26+"	2026-04-25 11:35:19 -04:00
xarmian	6cda2da48d	feat(billing): cancel Stripe customer on account delete (TASK-690) (#227) ... * feat(billing): cancel Stripe customer on account delete (TASK-690) Parent: PLAN-645. Pair with pad-cloud PR #12. * fix(billing): abort on all non-200 per Codex review (round 1) * fix(billing): env wiring + docstrings + partial_delete test per Codex review (round 2) * fix(compose): wire cloud env vars from .env per Codex review (round 3)	2026-04-23 19:35:12 -04:00
xarmian	46fa72ca0f	feat(server): log session IP changes, add optional PAD_IP_CHANGE_ENFORCE=strict (TASK-666) (#191) ... * feat(server): log session IP changes, add optional PAD_IP_CHANGE_ENFORCE=strict (TASK-666) Sessions stored a client IP at creation but never rechecked it. A stolen cookie could be used from anywhere with no signal to the owner. This change adds mid-lifetime IP-change detection without breaking legitimate mobility (mobile roaming, VPN toggles, carrier NAT) by default. - New audit action ActionSessionIPChanged captures {old_ip, new_ip} in the audit metadata. Visible via the existing /api/v1/admin/audit-log. - handleSessionIPChange wired into both SessionAuth (cookies) and TokenAuth (padsess_ bearer). After UA check passes, compares stored session IP to clientIP(r). On mismatch: - log one audit row - update the stored session IP so we don't spam the log - strict mode: DeleteSession + 401 "session_ip_changed" - default mode: let the request through - Store.UpdateSessionIP lets middleware refresh the recorded IP without tearing down the session. - PAD_IP_CHANGE_ENFORCE=strict env var + ip_change_enforce TOML key + Server.SetIPChangeEnforce setter (case-insensitive, trims whitespace). - Table-driven tests cover log-only, strict rejection with session destruction, and setter parsing edge cases. Parent: PLAN-643 (OSS Security Hardening). * fix(server): dedupe session-IP-change audit via CAS, handle browser vs API paths per Codex review Addresses two P2 comments on PR #191: 1. Race: parallel requests after an IP change could each emit ActionSessionIPChanged before any of them updated the stored IP, producing duplicate audit rows for a single transition. - Replace UpdateSessionIP with UpdateSessionIPIfEquals (compare-and-set on ip_address). Only the request that actually rotates the stored value logs; concurrent siblings lose the CAS and skip logging. - New test TestSessionIPChange_CASDedupesRace fires 20 concurrent requests from the new IP and asserts exactly 1 audit row. 2. Strict-mode 401 on non-API paths: - In current routing the SPA is mounted on the root router outside the auth Group, so SessionAuth only fires for /api/* in practice. The original concern about JSON 401s on browser navigation doesn't surface today, but defense-in-depth keeps the code forward-safe: restructure handleSessionIPChange to return a four-state outcome (Continue / AllowedLogged / Revoked / Terminated) and only write the JSON 401 on /api/* paths. Revoked + non-API falls through unauthenticated so a future SPA-in-group configuration would still render a login screen instead of raw JSON. - Clear the session cookie (MaxAge=-1) in strict rejection so the browser stops sending the now-revoked token on the next request. TestSessionIPChange_StrictClearsCookies verifies the Set-Cookie. Parent: PLAN-643 (OSS Security Hardening), TASK-666. * fix(server): strict mode destroys session atomically, never rotate stored IP when destroying (TASK-666) Addresses Codex P1 on PR #191: previously we rotated the session's stored ip_address via UpdateSessionIPIfEquals BEFORE attempting DeleteSession. If the DELETE failed (transient DB error) the row remained alive — rebound to the attacker's new IP — so follow-up requests saw stored IP == client IP and passed handleSessionIPChange's "match, no-op" branch. That silently defeated strict enforcement. - New Store.DeleteSessionIfExists returns (bool, error) to serve as the CAS primitive for strict mode: only the caller whose DELETE affected a row emits the audit entry, and a DB error fails closed (500 — "Unable to validate session") rather than letting the request through. - handleSessionIPChange splits into two paths: * log-only mode: UpdateSessionIPIfEquals for CAS dedup (unchanged) * strict mode: DeleteSessionIfExists is the CAS; stored IP is NEVER rotated so any failure leaves the session bound to the OLD IP and subsequent requests from the new IP still mismatch + still reject. - TestSessionIPChange_StrictDestroysSessionAtomically regression test verifies a second request from the new IP with the same token still fails after the first strict-mode rejection. Parent: PLAN-643 (OSS Security Hardening). * fix(server): exempt public API paths from strict IP-change termination (TASK-666) Addresses Codex P2 on PR #191: SessionAuth runs for every /api/* path, including public endpoints like /api/v1/auth/login, /api/v1/auth/register, /api/v1/health, /api/v1/s/* (share links), and /api/v1/plan-limits. In strict mode, a stale session cookie on those requests was rejected with a 401 session_ip_changed BEFORE the public handler could run — the user literally couldn't log back in because their own stale cookie blocked the login call. - Extract isPublicAPIPath as a shared helper between RequireAuth and handleSessionIPChange so they can't drift out of sync. - handleSessionIPChange strict-mode flow now: destroy session + clear cookies + audit log (unchanged), then for public API paths return Revoked so the handler still runs. For authenticated-only API paths still return Terminated (401). For non-API paths return Revoked for the SPA fallback. - Updated TokenAuth Revoked handler to match: pass through unauth on public paths, 401 on authenticated-only. - TestSessionIPChange_StrictAllowsPublicAPIPaths regression test: a stale session cookie on /api/v1/auth/login must NOT produce session_ip_changed; /api/v1/plan-limits must still return 200. Parent: PLAN-643 (OSS Security Hardening). * fix(server): short-circuit SessionAuth on token auth + fix IPv6 clientIP parsing (TASK-666) Addresses two more Codex comments on PR #191: P1 — SessionAuth 401'd API-token-authenticated requests: TokenAuth sets currentUser for user-owned tokens AND tokenWorkspaceID for legacy workspace-scoped tokens. SessionAuth short-circuited only on currentUser, so a workspace-scoped-token request that happened to carry a stale session cookie with a mismatched IP would be rejected by the IP-change strict path before RequireAuth could honor the token. Extend the short-circuit to also check tokenWorkspaceID; either signal is enough to say "token auth already succeeded, skip cookie validation". P2 — clientIP mangled IPv6 addresses: clientIP used strings.LastIndex(":") on RemoteAddr. For bare IPv6 addresses like "2001:db8::1" (which TrustedProxyRealIP writes verbatim from X-Forwarded-For, no brackets/port), that strips the final hextet to "2001:db8:" — unusable for comparison in the new IP-change audit path and incorrect for rate-limit keys too. Switch to net.SplitHostPort which handles both "host:port" and "[ipv6]:port", falling back to the raw RemoteAddr when no port is present (the trusted-proxy rewrite case). Tests: - TestClientIP_IPv6NotMangled covers IPv4 w/wo port, bracketed IPv6, bare IPv6 (no port, no brackets), and loopback forms. - TestSessionAuth_ShortCircuitsOnAPITokenAuth exercises the worst case: strict mode + valid API token + stale session cookie + new client IP. Request must succeed (token wins) and NO new session_ip_changed audit row must appear. Parent: PLAN-643 (OSS Security Hardening). * fix(server): canonicalize IPs before session-IP-change comparison (TASK-666) Addresses Codex P2 on PR #191: raw-string comparison of session.IPAddress vs clientIP(r) would fire session_ip_changed spuriously when the same IPv6 address arrived in different valid textual representations (the trusted-proxy path writes X-Forwarded-For verbatim, and different hops normalize differently — "2001:0db8::1" vs "2001:db8::1" etc.). - canonicalIP helper: net.ParseIP + stringify to collapse equivalent IPv6 forms (compressed vs expanded, case, leading zeros) and IPv4-in- IPv6 into a single canonical string. Non-parseable inputs pass through unchanged so debug/malformed values behave predictably. - handleSessionIPChange compares and logs the canonical forms. The CAS still passes session.IPAddress (the raw stored value) to the DB — the compare-and-set is about row identity — but the new IP written in is the canonical form so future comparisons are stable. - TestCanonicalIP covers empty, IPv4, shorthand "::1", expanded 8-group equivalent, mixed-case 2001:DB8::1, fully expanded 2001:0db8:…:0001, and non-IP fallback. Parent: PLAN-643 (OSS Security Hardening).	2026-04-22 10:43:22 -04:00
xarmian	6f468d37b6	fix(config): auto-generate PAD_ENCRYPTION_KEY on first run (TASK-668) (#189) ... * fix(config): auto-generate PAD_ENCRYPTION_KEY on first run (TASK-668) store/encryption.go silently accepted an empty key and stored TOTP seeds in plaintext; cmd/pad/main.go only logged a WARN. Operators who never saw the warning (or saw it and ignored it) ran for months with sensitive data at rest in the clear. Change: encryption is now mandatory. Resolution order inside Config.EnsureEncryptionKey: 1. PAD_ENCRYPTION_KEY env var (EncryptionKeySource = "env"). 2. encryption_key in config.toml (source = "config"). 3. <DataDir>/encryption.key file (source = "file"). 4. Generate a fresh 32-byte AES-256 key, persist it to the file above with 0600 permissions, continue (source = "generated"). Generation step never fails silently — mkdir + write errors propagate out of main.go and abort startup. main.go: - drop the "if cfg.EncryptionKey != '' { enable } else { warn }" fork. - call cfg.EnsureEncryptionKey(), fail startup on error, log at WARN when a key is freshly generated so operators notice the new file. Tests (internal/config/encryption_key_test.go): - generates when missing (file permissions 0600, 32-byte key). - loads existing file (strips trailing newline). - respects already-configured values (no file write). - idempotent across restarts (same key across two Config objects sharing a DataDir). Parent: PLAN-643 (OSS Security Hardening). * fix(config): refuse to auto-generate key in clustered deployments per Codex P1 Codex caught that auto-generating a per-process key on a Postgres- backed multi-replica deployment would give each replica its own key — cross-instance decryption of shared DB rows would fail with GCM auth errors. Change: EnsureEncryptionKey now takes an allowGenerate bool. main.go passes (dbDriver != 'postgres'): single-instance SQLite deployments get the zero-config auto-generation path; Postgres deployments must set PAD_ENCRYPTION_KEY explicitly. Operators who DO share a volume across replicas can pre-seed the file and it still loads (the generate step is the only thing gated). Tests: - TestEnsureEncryptionKey_RefusesToGenerateWhenClustered — allowGenerate=false + no existing file → error, no file written. - TestEnsureEncryptionKey_ClusteredWithPreSeededFileStillLoads — the file path works in clustered mode when the file is already present. - Existing idempotency test updated to exercise the mixed case (first boot generates, second boot loads with allowGenerate=false). * fix(config): atomic encryption key file creation per Codex P2 Codex caught that the check-then-write sequence for encryption.key had a race: two processes starting together could both pass the os.ReadFile IsNotExist check, generate different keys, and race the write. Whichever process wrote first would end up with an in-memory key that no longer matched the persisted file, and future restarts of THAT process would decrypt with the 'wrong' key. Switch to os.OpenFile with O_CREATE|O_EXCL: on EEXIST we re-read the file and converge on whichever key won the race. Every racing process ends up with the same key or a clear startup error. Test: TestEnsureEncryptionKey_ConcurrentStartIsRaceSafe fires 16 goroutines at a shared DataDir and asserts they all observe the same key. Also runs clean under -race. * fix(config): fully-written key guaranteed via temp+hardlink per Codex P2 Codex caught that O_CREATE|O_EXCL + ReadFile-on-EEXIST still had a window where a loser could read an empty/partial file between the winner's create and its first write. Hex/length validation would then fail startup with a confusing error. Switch to temp-file + os.Link: 1. Write the full key to a uniquely-named temp file (fully closed). 2. os.Link(temp, keyPath) atomically creates the final file as a hardlink to the complete temp inode. EEXIST means a loser; the file they'd read is another process's already-complete temp. 3. defer os.Remove(tmpPath) cleans up in every path. The race-safety test now also covers the 'read partial' case implicitly — if any goroutine loaded an empty/partial key the hex decode in main.go would fail in production; the test asserts all 16 goroutines observe the same non-empty key. * fix(config): reject world/group-readable encryption.key per Codex P2 Codex flagged that the file-load path blindly accepted any mode on encryption.key. On a multi-user host, a pre-seeded file chmod'd to 0644 would hand the AES key to every local user, defeating the whole purpose of encrypting TOTP seeds at rest. Stat the file and reject any mode where group or other bits are set (0077 mask). Error message points the operator at the fix (chmod 600). Skipped on Windows where Unix permission bits aren't enforced. Test: TestEnsureEncryptionKey_RejectsWorldReadableFile pre-seeds the file at 0644 and verifies startup fails with the chmod hint. * fix(config): always allow key auto-gen; warn on Postgres per Codex P1 Codex caught that gating auto-generation on 'not postgres' broke the first-boot experience for every Postgres deployment that wasn't already provisioning PAD_ENCRYPTION_KEY — which includes our own docker-compose.yml and deploy/k8s/configmap.yaml. Server would exit with 'encryption key required' before even starting. Revert the gate: EnsureEncryptionKey(true) always, for every driver. In exchange, log a WARN specifically on Postgres when we generate a key, pointing operators at the multi-replica concern. Trade-off accepted: single-instance Postgres just works; multi-replica operators get a visible warning and clear failure mode (GCM auth errors on first cross-replica read) if they don't act on it. Better than a startup crash for the single-replica majority. * fix(config): Postgres requires explicit PAD_ENCRYPTION_KEY; provision it in deployments Codex was right twice — both concerns are real, and this commit resolves them together: 1. Restore the Postgres gate: EnsureEncryptionKey(false) when dbDriver == "postgres". Multi-replica deployments must share a key; auto-generating per pod would fail cross-replica decryption. 2. Update the shipped Postgres deployments to provision a shared PAD_ENCRYPTION_KEY so first-boot works out of the box: - docker-compose.yml: PAD_ENCRYPTION_KEY via ${VAR:?err} shell substitution (fails "docker compose up" with a clear message if missing, matching the POSTGRES_PASSWORD pattern). - .env.example: document PAD_ENCRYPTION_KEY as REQUIRED on Postgres with an "openssl rand -hex 32" hint. - deploy/k8s/secret.yaml: add PAD_ENCRYPTION_KEY with a CHANGE_ME placeholder, explain why the replicas: 2 deployment requires a shared key. SQLite deployments continue to auto-generate on first boot (the TASK-668 happy path), so single-user installs stay zero-config.	2026-04-22 08:45:56 -04:00
xarmian	7d3b468fc8	feat(server): gate /metrics behind loopback + bearer token (TASK-653) (#180) ... cmd/pad/main.go:277 unconditionally registered Prometheus metrics and internal/server/server.go:229 served /metrics with no auth/CSRF. Any caller on the network could read workspace counts, API usage patterns, and (via label enumeration) user/workspace IDs. Three-layer gate: 1. Loopback-only default. No PAD_METRICS_TOKEN configured → /metrics accepts loopback peers only (safe for self-hosters running Prometheus on the same host, which is the common case). Non-loopback peers get 403 with a clear message. 2. Bearer-token mode. PAD_METRICS_TOKEN set → every scrape must send "Authorization: Bearer <token>", compared in constant time. Missing or wrong header → 401 with WWW-Authenticate: Bearer realm="metrics". 3. Rate-limit/logging chain still wraps the endpoint from the outer router.Use calls. Wiring: - internal/config/config.go — MetricsToken field + PAD_METRICS_TOKEN env. - cmd/pad/main.go — plumb cfg.MetricsToken into SetMetricsToken. - .env.example — document PAD_METRICS_TOKEN with openssl-rand hint. - internal/server/server.go — metricsAuth middleware + subtle.ConstantTimeCompare. Tests: metrics_auth_test.go covers loopback allowed, LAN denied, missing/wrong/correct Bearer, non-Bearer scheme rejected, WWW-Authenticate header, and the SetMetrics-absent 404. Parent: PLAN-643 (OSS Security Hardening).	2026-04-21 21:27:33 -04:00
xarmian	ec9edef68c	fix(server): gate RealIP on PAD_TRUSTED_PROXIES (TASK-660) (#173) ... Replace the unconditional chimiddleware.RealIP with a middleware that only trusts X-Real-IP / X-Forwarded-For when the direct TCP peer is within a configured CIDR. With the safe default (PAD_TRUSTED_PROXIES unset) proxy headers are ignored entirely — the real TCP peer address is used for rate limiting, the bootstrap loopback check, and audit logs. Why: previously any client could set X-Forwarded-For to bypass per-IP rate limits AND the bootstrap loopback check (handlers_auth.go). On a direct-exposed Docker deploy (see M6, TASK-661) this compounded into a full-takeover chain. Gating RealIP breaks that chain even when the operator forgets to firewall the port. - internal/server/middleware_realip.go — new TrustedProxyRealIP middleware + ParseTrustedProxyCIDRs helper (accepts CIDRs or bare IPs, invalid entries logged+skipped, empty = nil result = no-op middleware). - internal/server/server.go — swap chimiddleware.RealIP for the gated version; add trustedProxyCIDRs field and SetTrustedProxies wiring. - internal/config/config.go — TrustedProxies field + PAD_TRUSTED_PROXIES env var. - cmd/pad/main.go — plumb config to the server. - internal/server/middleware_realip_test.go — covers no-trust default, untrusted peer, trusted peer with X-Real-IP, X-Forwarded-For first entry, and invalid header. Parent: PLAN-643 (OSS Security Hardening).	2026-04-21 19:03:49 -04:00
xarmian	d0518216c5	feat: add cloud infrastructure for hosted Pad (PLAN-427) ... Add the foundation for running Pad as a hosted service at app.getpad.dev. Same binary in cloud mode with a thin sidecar for OAuth and Stripe. Cloud mode (PAD_CLOUD=true): - PAD_CLOUD flag with cloud secret for sidecar communication - Account-level billing: plan field on users, CheckLimit enforcement - Free/Pro tiers with configurable limits stored in platform_settings - Three-tier limit resolution: user overrides → DB defaults → hardcoded fallback - Plan enforcement on workspace, item, member, webhook, and token creation Authentication & security: - OAuth login endpoint (POST /api/v1/auth/oauth-login) with cloud secret gate - Verified email requirement for OAuth, 2FA bypass protection - Cloud secret rotation support (comma-separated keys) - TOTP secret encryption at rest (AES-256-GCM via PAD_ENCRYPTION_KEY) - Rate limiting on OAuth login endpoint - Bootstrap disabled in cloud mode - Password max length enforcement (128 chars) - Config file written with 0600 permissions Admin & billing: - Admin user management API (list, detail, update plan/overrides) - Configurable plan limits API (GET/PATCH /api/v1/admin/limits) - Platform stats endpoint - Admin plan endpoint for sidecar to set user plans - GDPR: account deletion and data export endpoints Console UI (cloud mode only): - /console — workspace list with owned/shared sections - /console/new — create workspace wizard with slug preview - /console/settings — profile, password, API tokens - /console/billing — plan status, upgrade/manage links - /console/admin — user management, plan overrides, limits editor - OAuth buttons (GitHub/Google) on login page in cloud mode Auto-create default workspace on signup in cloud mode. Migration 035: plan, plan_expires_at, stripe_customer_id, plan_overrides on users.	2026-04-12 17:43:40 +00:00
xarmian	82e93d6014	feat: implement SSE connection limits ... Add configurable global and per-workspace SSE connection limits to prevent memory exhaustion from unbounded connections. Returns HTTP 429 when limits are reached. Logs warnings at 80% capacity. Configurable via PAD_SSE_MAX_CONNECTIONS (default 1000) and PAD_SSE_MAX_PER_WORKSPACE (default 100), or config.toml. Adds WorkspaceSubscriberCount to EventBus interface for per-workspace tracking (MemoryBus iterates subscribers, RedisBus uses existing wsCounts map). Resolves TASK-165	2026-04-06 01:23:34 +00:00
xarmian	8aa6481421	PHASE-12: Security Hardening for Pad Cloud (#67) ... * feat: enforce RBAC role checks on all mutation endpoints (TASK-150) Add requireMinRole helper and role enforcement to 30+ mutation handlers. Viewers are now blocked from all state-changing operations, editors can mutate items/docs/comments/views but not collections/webhooks/workspace settings, and only owners can perform administrative operations. Includes 11 integration tests with real auth covering viewer/editor/owner access across items, collections, documents, comments, agent roles, item links, and workspace operations. * fix: scope search results to user's workspaces (TASK-151) Search without a ?workspace= param previously returned results from all workspaces in the database. Now the handler resolves the authenticated user's workspace memberships and passes their IDs to the store query, ensuring results only include items from workspaces the user belongs to. Fresh installs (no users) retain unscoped search for backward compat. Includes integration test proving cross-workspace isolation. * fix: add webhook URL validation and SSRF protection (TASK-152) Webhook creation now validates URLs before accepting them: only HTTP(S) schemes allowed, embedded credentials rejected, private/reserved IPs blocked (loopback, RFC1918, link-local, cloud metadata 169.254.169.254), and hostnames are DNS-resolved to verify they don't point to private IPs. Defense-in-depth check also added to the dispatcher's deliver function so existing webhooks with unsafe URLs are blocked at delivery time. * feat: add CSRF protection with double-submit cookie pattern (TASK-153) Implements CSRF middleware that validates X-CSRF-Token header matches the pad_csrf cookie on all state-changing API requests. Bearer token auth, auth endpoints, and fresh installs are exempt. The frontend client reads the CSRF cookie and attaches the header on mutations. * feat: add per-endpoint rate limiting middleware (TASK-154) Adds IP-based rate limiting for auth endpoints (5/min login, 3/hr password reset, 5/hr registration) and user-based limits for API (100/min) and search (30/min). Uses golang.org/x/time/rate with automatic stale-entry cleanup. Adds chi RealIP middleware for correct client IP behind proxies. Returns 429 with Retry-After. * fix: sanitize error responses and remove PII from logs (TASK-155) Replace all writeError(500, err.Error()) calls with writeInternalError that logs the real error server-side and returns a generic message to clients. Remove email addresses, user IDs, and password reset tokens from log output to prevent PII leakage. * feat: add security headers, configurable CORS, and secure cookies (TASK-160) Add SecurityHeaders middleware (CSP, X-Frame-Options, nosniff, Referrer-Policy, Permissions-Policy). Make CORS origins configurable via PAD_CORS_ORIGINS env var. Add PAD_SECURE_COOKIES for TLS deployments (sets Secure flag on session/CSRF cookies and enables HSTS). Also adds X-CSRF-Token to CORS allowed headers. * fix: address PR review — lazy router init and trusted IP for rate limits Fix two issues flagged by Codex: 1. CORS/HSTS config was ignored because setupRouter() ran in New() before SetCORSOrigins/SetSecureCookies were called. Now uses sync.Once to lazily build the router on first ServeHTTP/Listen. 2. Rate limiter read X-Real-IP directly from untrusted headers, allowing clients to spoof IPs. Now uses RemoteAddr only (which chimiddleware.RealIP already sanitizes from trusted proxy headers).	2026-04-05 10:26:00 -04:00
xarmian	5db077a2a3	refactor(cli): limit local server autostart to local mode for TASK-116 (#35)	2026-04-01 21:56:18 -04:00
xarmian	123a7aec98	feat(cli): add client configure flow for TASK-115 (#34)	2026-04-01 21:32:55 -04:00
xarmian	d94a28c3e8	feat: account settings, email infrastructure, and UX polish (#23) ... - Add Account tab to settings: profile editing, password change, API token management - Add PATCH /api/v1/auth/me endpoint for profile updates with password verification - Add email sending infrastructure via Maileroo with contextual sender names - Add Platform settings tab (admin-only) for email configuration with test send - Add platform_settings table for instance-wide configuration - Add tab visibility refresh: silently sync data when browser tab regains focus - Fix filters icon: replace broken Unicode character with proper SVG funnel - Add cancel invitation support, TypeScript User/APIToken types	2026-03-29 18:29:52 -04:00
xarmian	46447e5504	feat: user management & authentication (Phase 6) (#14) ... * feat: add user management database migration and models Add migration 012_users.sql with users, sessions, and workspace_members tables. Add user_id columns to api_tokens, items, comments, activities, item_links, and item_versions for proper user attribution. Create Go model structs (User, Session, WorkspaceMember) in models/user.go. * feat: add store layer for users, sessions, and workspace members Implement CRUD operations for user management: - users.go: create, get, update, list, validate password (bcrypt) - sessions.go: create, validate, delete, cleanup expired (SHA-256 hashed tokens) - workspace_members.go: add/remove members, role management, access checks Adds golang.org/x/crypto/bcrypt dependency. Includes 16 new tests covering all store methods, password validation, session lifecycle, and workspace membership operations. * feat: rewrite auth system from single-password to user-based Replace single-password auth with email/password user authentication: - New endpoints: POST /auth/register, GET /auth/me - Rewritten: POST /auth/login (email+password), GET /auth/session (needs_setup detection), POST /auth/logout (DB session destroy) - Delete in-memory SessionManager, use DB-backed sessions via store - New middleware: SessionAuth (cookie→user), RequireAuth (with fresh-install passthrough when no users exist) - Remove Password field from config, PAD_PASSWORD env var, SetPassword() All 23 existing server tests pass (fresh DBs have no users → passthrough). * feat: add workspace access control middleware Add RequireWorkspaceAccess middleware that checks workspace_members for authenticated users, with fallback for legacy API tokens and fresh installs (no users → implicit owner). Includes role hierarchy helpers (workspaceRole, requireRole) for downstream permission checks. Wire middleware into the /{slug} workspace route group. * feat: add CLI auth commands and credential storage Add pad login, pad logout, pad whoami commands with credential storage in ~/.pad/credentials.json (0600 permissions). Update CLI HTTP client to auto-attach auth tokens and X-Pad-Agent header on all requests. Add auth API methods (Login, Register, Logout, CheckSession, GetCurrentUser). Extend .pad.toml with optional agent_name field. Add golang.org/x/term for masked password input. * feat: derive actor/source from auth context in all handlers Replace hardcoded "user"/"web" actor/source strings with auth-aware helpers. actorFromRequest() derives actor ("user"/"agent" via X-Pad-Agent header) and source ("web"/"cli" from auth method). agentMeta() merges agent name into activity metadata. Update all item, document, comment, and move handlers to use request-based logActivity/logActivityWithMeta. Remove hardcoded CreatedBy/Source from all CLI commands — server now determines these from auth context. * feat: frontend auth — login, registration, auth guard, user menu Rewrite login page with email/password fields, add registration page for first-time setup, update auth guard to handle needs_setup redirect. Add user menu to sidebar with logout. Update API client with new auth methods (register, login with email, session with needs_setup flag). * feat: migrate API tokens from workspace-scoped to user-owned API tokens now have a user_id owner and optional workspace_id scope. CreateAPIToken takes userID as first parameter. ValidateToken resolves the token's user into the request context. TokenAuth middleware now sets ctxCurrentUser when a user-owned API token is used. Add user- scoped endpoints: GET/POST/DELETE /auth/tokens. Keep workspace-scoped token endpoints for backwards compatibility. * feat: workspace membership, invitations, and role enforcement Add workspace_invitations table (migration 013) with join codes. Implement invitation store methods (create, get by code, accept, list). Add member management handlers: list members + invitations, invite (auto-adds existing users or creates invitation), remove member, change role, accept invitation by code. Add API routes under /workspaces/{slug}/members/* and /invitations/{code}/accept. Add CLI commands: pad members, pad invite, pad join. * feat: auth tests and documentation updates Add comprehensive auth endpoint tests: registration flow (first user becomes admin), login/logout, validation errors, duplicate email, auth enforcement (401 after users exist, exempt paths), /me endpoint. Update CLAUDE.md and README.md to document user-based auth system, replacing old PAD_PASSWORD references with pad login/members/invite workflow and role-based access control. * feat: add members management UI to workspace settings page Add Members section to settings with: member list (avatar, name, email, role), role change dropdown (owner only), remove button (owner only), pending invitations display with join codes, and invite form with email + role picker. Add members API methods to the TypeScript client (list, invite, remove, updateRole). * fix: backfill workspace owners for pre-migration workspaces Add backfillWorkspaceOwners() that runs on server start. For any workspace with no members, adds the first admin user as owner. This handles the migration case where workspaces existed before the user system — without it, the members list shows empty. * feat: shareable invite links with /join/[code] page Replace raw join codes with full shareable URLs. Server generates join_url using its configured base URL (e.g. https://pad.example.com/ join/a3f8b2c1). New /join/[code] page handles the full flow: checks auth → shows login/register if needed → accepts invitation → redirects to workspace. Settings page shows "Copy invite link" button that copies URL to clipboard. CLI outputs shareable link instead of raw code. * fix: auto-add workspace creator as owner, integrate auth into pad init handleCreateWorkspace now adds the authenticated user as owner of the new workspace immediately — no more relying on the startup backfill. pad init now checks auth status before making API calls. If no users exist, prompts to register. If not logged in, prompts to login. After auth, proceeds with workspace creation normally. * fix: add join_url to invite response type in API client * fix: address codex review — invite registration, logout token revocation, workspace scoping - Allow registration with valid invitation_code (fixes invite flow for new users) - Revoke Bearer session tokens on logout, not just cookies - Filter workspace listing to user's memberships (admins see all)	2026-03-28 15:43:09 -04:00
xarmian	a30655aa0e	feat: add optional password authentication for web UI (#6) ... When PAD_PASSWORD is set (env var) or password is configured in ~/.pad/config.toml, the server requires authentication: Backend: - SessionManager with HMAC-SHA256 signed cookies (7-day TTL) - POST /api/v1/auth/login — validates password, sets session cookie - GET /api/v1/auth/session — returns auth status (exempt from auth) - POST /api/v1/auth/logout — destroys session, clears cookie - PasswordAuth middleware gates all API/page requests - API tokens still work independently (no change to CLI flow) - Constant-time password comparison + 500ms delay on failure Frontend: - Login page at /login with password form and error handling - Root layout checks auth status before loading app shell - Global 401 handler in API client redirects to /login - Login page renders without sidebar/app shell When no password is configured, everything works exactly as before (zero-friction localhost). This is a security requirement for any deployment that exposes the server beyond localhost.	2026-03-28 11:36:10 -04:00
xarmian	a2e5a6622d	Add Docker support and PAD_DATA_DIR config ... - Multi-stage Dockerfile for building from source - Dockerfile.goreleaser for GoReleaser-built release images - GoReleaser config: publish multi-arch Docker images to ghcr.io - Add PAD_DATA_DIR environment variable for configuring data directory - Update README with Docker install option - Add .dockerignore	2026-03-28 04:34:17 +00:00
xarmian	81579847c6	Initial release ... Pad — project management for developers and AI agents. Single Go binary with embedded SvelteKit web UI, SQLite storage, CLI, and Claude Code /pad skill integration. https://getpad.dev	2026-03-26 01:52:36 +00:00