1566 Commits

This Branch

This Branch

All Branches

Author	SHA1	Message	Date
xarmian	f465d4c51e	fix(server): a malformed before_id is a 400, not a 500 (BUG-2774) (#1205) ... * fix(server): a malformed before_id is a 400, not a 500 (BUG-2774) before_id went from the query string into the cursor predicate unchecked. Postgres refuses a text parameter that is not valid UTF-8 or that carries a NUL (SQLSTATE 22021/22P05), so the store call errored and the handler answered 500 — the server announcing its own failure for a client's bad input, and SQLSTATE noise in the logs for an input problem. SQLite accepts the same bytes and matches nothing, so the identical request was a 200 there; the failure mode diverged by dialect from one line of unvalidated input. validCursorID rejects exactly what the DATABASE rejects rather than what an id should look like, and deliberately carries NO length or format bound: the structured kinds' ids come from the item's own fields blob, nothing validates them on write, and an imported artifact may carry any string — so a cap could only ever fire on a legitimate cursor, while the cost of an over-long one is a single indexed comparison against a parameter the URL length limit already bounds. Its comment says so, because the obvious review question is why there isn't one. Tests: three malformed shapes reachable from a plain URL (%FF, an embedded NUL, invalid bytes mid-string) plus three controls — a UUID, a structured note id, and a long non-ASCII id — without which "reject every before_id" would pass and paging would be dead rather than honest. A store-level test pins the PREMISE where it is real: on Postgres the query itself fails, on SQLite it succeeds and returns nothing, and both halves are asserted in the one place that sees both, each with a message saying what it means if the backend's behaviour has moved. Verified on Postgres 17 (private container, not the shared port). The handler legs fail with the validation removed. Refs: BUG-2774 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(server): never emit a cursor the handler would refuse (codex round 1) The validation had a second direction I had not closed. A structured entry's id comes from the item's fields blob, which nothing validates on write, so a JSON \u0000 escape arrives as a real NUL on SQLite — Postgres's jsonb refuses it at the door, which is why this is a one-backend hazard. That id became the entry's id, the entry's id became next_before_id, and the client sending it back got a 400 from the validation this unit just added: the server handing out a cursor it then refuses, wedging paging on that item. Such an id now takes the positional fallback that empty and duplicate ids already take — the id has to be usable as a CURSOR, not merely unique. One condition on an existing branch rather than new machinery. Its test uses two notes, one NUL-bearing and one clean, so it distinguishes 'replaced the unusable id' from 'stopped using raw ids at all'. Verified against the unmutated branch: the NUL id is emitted verbatim and the leg fails. Refs: BUG-2774 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test: assert the envelope, the emitted cursor, and split the dialect premise (codex round 3) Three test weaknesses, all mine. Status-only assertions: a bare 400 or the wrong code would have passed, and clients branch on the code rather than the number. The legs now decode the envelope and require invalid_cursor with a non-empty message. The emitted-cursor test claimed to cover next_before_id and did not: at limit=50 the fixture emits no cursor at all, so it only checked entry ids. A third note and a truncating limit make the NUL-bearing one the LAST kept entry, which is what puts it in the cursor — and the test now asserts the cursor is valid AND that paging with it returns 200. The mutant (emitted ids unchecked) still dies, now on the thing the test is named for. The dialect test is CHARACTERIZATION and now says so: it passes with or without this change, because it describes the backends rather than the handler, and that is its job — it is the premise the 400 rests on. Its 'both halves in one place' claim was also false in an unconfigured run, where NewPostgres skipped the whole test and the SQLite half never ran either. Split into subtests: the SQLite half runs everywhere, the Postgres half skips alone. Refs: BUG-2774 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(server): a one-sided cursor is a 400, not a silent no-op (codex round 4) before_id alone could never match anything: the id is the tie-break AT the cursor instant, and before defaults to now+1m, which no row shares. So it was accepted, ignored, and the caller paged from the beginning believing they had a cursor — the accepted-and-does-nothing shape, which is the worst answer of the three available. The other direction stays supported deliberately, and has a control leg saying so: before alone is the external-client case the "g" sentinel exists for, and rejecting the pair symmetrically would break something the handler goes out of its way to serve. In scope for this unit rather than a separate filing because it is the same parameter and the same class of answer — and because BUG-2765's PR body documented "both fields or neither" as the contract, which until now nothing enforced. Codex's other round-4 finding — a structured entry id can collide with a comment/activity/version id, because the dedupe map holds only structured ids — is real, pre-existing, and a cross-source id decision rather than a validation. Filed as BUG-2783. Refs: BUG-2774, BUG-2783 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * docs: the cursor contract is asymmetric, and now says so (codex round 6) BUG-2765's prose said "both fields, or neither" in the handler doc comment and the TS client. This unit then enforced it in one direction only — deliberately, because `before` alone is the external-client shape the id sentinel exists to serve, while `before_id` alone matches nothing and silently pages from the beginning. The slogan was mine and the asymmetry is mine; leaving both in place would have left a reader to discover the difference from a 400. Both sites now state which one-sided form is accepted, which is refused, and why each. Refs: BUG-2774 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-25 19:17:49 -04:00
xarmian	31aba0bb5b	fix(store): compare-and-set the activity debounce merge (BUG-2770) (#1204) ... CreateActivityDebounced merged by reading a row's metadata, combining it in Go, and writing the result back — a read-modify-write across two statements. Two writers that selected the SAME row both read the pre-merge blob, each merged its own change into it, and the second UPDATE erased the first one's change entry with no error and no trace. Since BUG-2763 the racing pair must be the same account, actor kind AND agent name, which is exactly the ordinary case: one editor's autosave burst, or one agent issuing concurrent PATCHes. The merge UPDATE now carries the blob the caller merged from as a compare-and-set arm, and the caller is a bounded attempt loop that re-reads its candidate each time — so a losing attempt merges into the WINNER's blob rather than the stale one it first read. Adding that arm gives one UPDATE two ways to affect zero rows, and they want OPPOSITE dispositions: a comment-linked row (TASK-2760's freeze) must NOT be retried, since the answer will not change, while a contended row must NOT start a new run, since retrying is what keeps the entry whole. debounceRowUnchanged asks the CAS arm's own question after a refusal to tell them apart — one extra primary-key probe, only on the rare refusal path. Both statements spell that arm with ONE shared constant so they cannot drift into asking different questions. Also fixed here, found by review of the same code: - mergeActivityMeta panicked on a JSON `null` blob. `null` unmarshals with no error and leaves the map nil; the overlay then assigns into it. Valid JSON, valid JSONB, storable in the TEXT column — a crash on a write path, not a bad merge. - The merge timestamp is now taken per ATTEMPT. Hoisted out of the loop, a retry backdated a row whose newest change was younger than the stamp, and created_at is what the cooldown window, the timeline ordering and the status-transition backfill all read. - Two comments corrected: one claimed a lost CAS proves another merge landed (deletion does it too), and maxDebounceCandidates' comment predicted a query plan it has no business predicting. DUAL DIALECT: activities.metadata is TEXT on SQLite and JSONB on Postgres, so the CAS is byte equality on one and jsonb equality on the other. Verified empirically on postgres:17-alpine, both discriminating legs (a stale expectation refuses, a matching one writes), which is what rules out the comparison being stuck true or stuck false there. Tests drive the interleaving through a Store test seam rather than racing goroutines: the defect needs the competing write to land strictly between one call's read and its write, and real goroutines produce that ordering only sometimes — a detector with an unknown rate reads as coverage without being it. Against the pre-fix behaviour the regression test fails with the competitor's change silently absent, which is the filed symptom verbatim. Eight mutations of the fix all die; three tests carry explicit notes naming what they cannot discriminate. Six pre-existing defects surfaced by the review are filed rather than folded: BUG-2776, BUG-2777, BUG-2778, BUG-2779, BUG-2781.	2026-08-25 18:37:26 -04:00
xarmian	a2195997f7	fix(web): an entry missing from a refreshed first page is not necessarily deleted (BUG-2773) (#1203) ... * fix(web): an entry missing from a refreshed first page is not necessarily deleted (BUG-2773) The SSE refresh re-fetches the FIRST page — the newest N entries — and treated anything previously on it and now absent as deleted. Once enough newer entries exist, a perfectly alive entry rolls off that window, and it disappeared from the reader's view; for anyone who had pressed Load More it vanished from the MIDDLE of a timeline whose neighbours on both sides were still shown. A full reload brought it back, which is the tell that this was display state and not data. Per the lead's ruling (option 1): deletion is inferred only for a position the fresh page still COVERS — at or newer than its oldest entry, compared in the same (created_at, id) space the server's cursor uses. Anything older is out of window and left alone. An empty fresh page covers nothing and so deletes nothing: it means every row in that window was unrenderable, not that the history was erased. Every test leg pairs a roll-off with a real deletion, because a fix that simply stopped removing anything passes the roll-off half alone. The refresh helper asserts the refresh actually FIRED — the first version of these tests waited 400ms against a 500ms debounce, and one leg passed vacuously on a refresh that never happened. Mutation matrix, each detected by its own leg: the old rule (3 legs); string comparison instead of instants at the boundary (the sub-second leg); an empty fresh page covering everything; the id tie-break dropped. Refs: BUG-2773 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): a final page covers everything, and coverage replaces the first-page gate (codex round 1) Two findings, both real. has_more was ignored. A refresh whose page is FINAL returned the whole history, so an entry the client holds and that page does not contain has nothing to have rolled off into — it is gone, deleted or no longer renderable. Coverage now extends to everything on a final page, which is also what makes an empty FINAL page clear the view while an empty page with more behind it still deletes nothing. Both directions have a leg; asserting either alone would let "empty always clears" or "empty never clears" pass. firstPageIds is retired rather than repaired. It tracked the last first-page fetch so older-page entries would not be judged by a first-page comparison — which coverage now does directly and better: an older-page entry sits below the floor and is left alone, while an entry INSIDE coverage that the page does not contain is gone regardless of which page delivered it. Keeping both would have leaked: an entry preserved as a roll-off dropped out of the tracked set, so a later window expanding back over it could never remove it again. Mutation matrix, six mutations, each detected: no final-page rule (2 legs); the old missing-means-deleted rule (3); never deleting (5); string comparison at the boundary; the id tie-break dropped; an empty non-final page treated as covering. Refs: BUG-2773 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): fence overlapping SSE refreshes, and state what inference cannot do (codex round 2) Only the newest dispatched refresh may write. The item/workspace check catches a switch but not two refreshes of the SAME item in flight at once — the retry path fires 2s after a failure while a newly debounced one is already running — and an older response landing last re-adds an entry the newer one removed or removes one it added. A monotonic seq, a plain let rather than $state because it is read and written inside the refresh (CONVE-1688). Its test holds both in flight and resolves them out of order; it fails with the fence removed. Two other round-2 findings are DOCUMENTED, not fixed, because they are limits of inferring deletions from a first-page comparison rather than defects of this change, and the lead ruled the event-based alternative out of scope for this unit: - an entry deleted below a non-final page's floor is never inspected again and stays until a reload. The old rule removed it — by removing every rolled-off entry with it, which is the bug being fixed. Strictly better, not complete. - whether a row renders depends on the window it was fetched in, since the cross-source drops need both rows in one fetch, so an entry that rendered on an older page can be absent-and-covered here. That matches what a fresh load shows, which is the ceiling for any first-page comparison. The fourth finding — the refresh not adopting has_more/next_cursor — is declined a second time, on the same grounds and now with its concrete harm checked: after paging to the end the reader already HOLDS everything behind page one, so a fresh has_more=true strands nothing. Refs: BUG-2773 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test+docs: a leg that fails against the OLD rule, and three claims corrected (codex round 3) The two final-page legs passed against main: the old gate removed any entry missing from a refreshed first page unconditionally, so they discriminated against an intermediate version of this fix and not against the behaviour it replaces. Added the leg that does — an entry loaded via LOAD MORE, which the old gate preserved unconditionally because it had never been on a first page, and which a final refreshed page must now remove. Verified by restoring the pre-fix rule: that leg fails, along with three others. Two comments were false and are corrected rather than softened: - Sub-second timestamps are NOT reachable from this server. The store writes RFC3339 seconds and the handler truncates the structured kinds' hand-written ones to match (handlers_timeline.go's stamp()). The instant-vs-string comparison is a guard on what the ordering MEANS, not a live scenario, and both the code and the test that pins it now say so. The claim came from the BUG-2765 unit and was wrong there too. - Comment-linked activities are excluded by the store's SQL whether or not the comment is in the window, so they are not an example of window-dependent rendering. The version-coincidence suppression is; the note names that one now. Refs: BUG-2773 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): a stale Load More page cannot resurrect a deleted entry (codex round 4) A paging request can be in flight while a refresh removes an entry as deleted. Its page is older than the refresh and still carries that row, so appending it verbatim put the entry back on screen — visibly undoing a deletion the reader had already seen happen. The refresh now records what it removed, and Load More filters its page through those tombstones. Chosen over discarding the whole stale page (the reader's click would do nothing) and over a generation fence (same). The set is cleared whenever loadTimeline resets the view, so it is bounded by one mount's deletions. Its test holds the paging response, deletes the entry via a refresh in between, then lands the stale page and asserts both halves: the deleted row does not come back AND the rest of the page still does — without the second, discarding everything would pass. Fails with the tombstone check removed. Refs: BUG-2773 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): discard a stale Load More page instead of tombstoning what it may resurrect (codex round 5) Round 4's tombstone set answered the right question with the wrong machinery, and round 5 found the bill: it grew for the life of a mount with no prune, its clear point raced older paging continuations (a same-item mutation calls loadTimeline, clears the set, and a stale page can then resurrect a deleted entry anyway), and it would suppress a same-id structured entry legitimately rewritten. Each of those is fixable; together they are a sign the mechanism was too clever for the race it guards. Replaced with the fence already used for overlapping refreshes: loadMore captures the refresh sequence before its await and discards the response if a refresh applied in between. One integer, no growth, no lifetime, nothing to clear at a switch. The page is dropped WHOLE rather than filtered — it was assembled before the deletion and nothing in it reflects the current view — and the cursor is untouched, so Load More is still offered and the next click fetches the same page against the current state. The test now asserts all three halves: the deleted entry does not come back, the rest of the stale page does not land either, and the button is still there. Without the third that would read as a silent drop rather than a deliberate one. Fails with the fence removed. Refs: BUG-2773 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): one view generation for reloads as well as refreshes (codex round 6) The fence only counted SSE refreshes, so the path that most obviously replaces the view did not advance it: a local comment delete calls loadTimeline, page 1 comes back without the entry, and a Load More page in flight from before that re-adds it. Identity and sequence both pass, because neither noticed. One counter now, incremented by every reload and every refresh, captured by every continuation that writes entries — including loadTimeline itself, which could otherwise overwrite a newer view with an older page-1 response. "The view was replaced" is the same fact whichever path replaced it. Tested through the harness-reachable form: a Load More page held across a switch AWAY and BACK. By the time it resolves the identity check passes again — same item — and only the generation can tell that the view it was fetched against has been replaced twice. Fails with the reload's increment removed. The local-delete path itself is not directly driven: it needs the comment controls, which need mutationsEnabled plus canEditItem plus the confirm flow. It goes through the same single increment as the switch case, but that is an argument, not a test, and this note is here so nobody reads the coverage as wider than it is. Refs: BUG-2773 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): count view replacements that LANDED, and fence the cleanup paths (codex round 7) Two regressions my own fence introduced. The generation advanced at DISPATCH, so a refresh that then FAILED still invalidated an in-flight reload: the reload's good response was discarded and the view stayed empty until something else redrew it. It now advances where the write lands — a request that never writes is not a replacement. Its test holds the initial load, fails a refresh across it, and asserts the load still renders; it fails when counting goes back to dispatches. The catch and finally only checked identity, so a stale request's cleanup could clear the spinner a newer one owns or restore an old error over a current load. Both are gated on ownership now. Ownership needed a flag rather than a bare reqGen === viewGen: the writer advances the generation ITSELF, so after writing it reads as stale by its own test — the first version of this blocked its own `loading = false` and left an empty page under a permanent spinner. Caught by two existing tests failing, not by review, which is the instrument working. (This message is a re-write: the first one was passed through a double-quoted shell string and the backticked span was executed and blanked — CONVE-13, the convention that exists because of exactly this. Caught by re-reading the artifact rather than the success line, which is the other half of it.) Refs: BUG-2773 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): ticket plus high-water mark, so newest wins and a failure costs nothing (codex round 8) One counter could not express both halves, and each single-counter version failed a different way. Counting DISPATCHES let a refresh that then failed invalidate an in-flight reload (round 7). Counting APPLIES let an OLDER response landing first claim the view and lock the newer one out behind it (round 8) — the mirror of the case the out-of-order test already covered, and the reason that test alone was not enough. Every view-replacing request now takes a unique increasing ticket at dispatch and may write only if it beats the high-water mark of what has actually written, which it then owns. Newest wins among concurrent responses, and a request that never lands costs nothing. Second round-8 finding, same root: an overlapped load declines to clear the spinner once a newer write has landed, so the writer has to. Without it an empty refresh result sat under a permanent spinner and the list never rendered. Mutation checks, each with its anchor count asserted after two mutations silently failed to apply and left a GREEN run that proved nothing: the refresh not clearing the spinner fails the new spinner leg; the round-7 applied-counter semantics fail the new newest-wins leg; the loadMore fence removed fails the switch-away-and-back and stale-page legs. Refs: BUG-2773 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): a reload clears loadingMore too (codex round 9) loadMore's cleanup is identity-guarded, so a page resolving while the reader is on another item never cleared the flag. Coming back found Load More permanently disabled — a dead control, which reads as "there is nothing more" rather than as a bug. Pre-existing (the identity guard is TASK-2112's), and one line to close now that loadTimeline is already the single place paging state resets. The existing switch test released its stale page AFTER returning, which is the ordering that never exercised this; the new leg resolves it while away and asserts the button comes back enabled. Fails with the reset removed. Refs: BUG-2773 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): the coverage boundary is the server's cursor, not the oldest row returned (codex round 10) How far back a page LOOKED and how far back it RETURNED rows are different positions, and BUG-2765 made the first one available: next_before is where the next page starts, so everything at or newer than it was examined. When one source exhausts its over-fetch window while another returns an older rendered row, the cursor sits NEWER than the page's oldest entry — and judging by the returned floor then treats a live entry from the exhausted source, one this page never reached, as deleted. Using the cursor makes the rule say what it means. The returned floor stays as the fallback for a server predating that field; it is the slightly-too-eager version, and still narrower than the rule this unit replaces. Two comments corrected rather than left: "the oldest entry the fresh page reached" was the oldest RETURNED, and "has_more=false means the server returned everything it has" ignored that rows are fetched and dropped as unrenderable — which is precisely why absence from a final page still means gone. The new leg builds the shape only a server can produce (cursor newer than the oldest returned row) and asserts both sides of the boundary: the entry below the cursor is kept, the one above it is removed. Fails with the boundary put back to the returned floor. Refs: BUG-2765, BUG-2773 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * docs: three of my own claims corrected, including one a commit message overstated (codex round 12) - The refresh's comment still said only the newest DISPATCHED refresh may write. Round 8 replaced that with ticket-versus-high-water, which lets an older response write first and be replaced by the newer one — deliberately, and with its own test. The comment described the scheme two rounds ago. - The final-page test still said has_more=false means the server "returned everything it has". It reached the end of the rows; some were dropped as unrenderable on the way, which is why absence from a final page still means gone. My round-10 commit claimed this wording was corrected — it was, in the component, and not here. The claim was true of half the sites and written as though it covered both. - The file header said every leg pairs a roll-off with a real deletion. That was true of the first leg and stopped being true as the legs accumulated; and the empty-final-page leg described "both refreshes" when it makes one. Both now say what the tests do. Refs: BUG-2773 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): a superseded refresh does not retry (codex round 13) The retry exists so a transient failure does not leave the panel quietly stale (BUG-2508). Once a newer refresh has written the view there is nothing stale to repair: the retry is traffic for a question already answered, and its answer would arrive older than what is on screen. Guarded on the same high-water mark every other write path uses. Its test fails an older refresh after a newer one landed and asserts the request COUNT does not move across the retry backoff — the entries look identical either way, so the count is the only thing that distinguishes the two behaviours. Fails with the guard removed. Refs: BUG-2773 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-25 18:01:22 -04:00
xarmian	11f67b0a98	fix: timeline can answer has_more=true with zero entries, and the client cannot page past it (BUG-2765) (#1202) ... * fix(server): timeline returns the cursor for its next page (BUG-2765) The timeline over-fetches 3x per source and drops rows that cannot render (read/searched actions, empty-metadata updates, activities a version or a comment already stands for, collapsed autosave bursts), so a page can carry fewer entries than the rows it consumed — or none, while has_more is true. The client derived its cursor from the last RENDERED entry, which fails in two ways. With no entries it cannot form a cursor at all, so the first page is a dead end. With a fully-dropped window LATER in the history it re-sends the same cursor forever: nothing is appended, the oldest entry does not move, and paging is wedged at that position permanently. The filing named the first; the second is the one that bites an ordinary item, since a run of read activities anywhere in its history is enough. Both are the same root cause — the response says WHETHER to continue and not WHERE — so the server now returns next_before / next_before_id whenever has_more is true: - page truncated: the last entry KEPT, because the ones cut off must be re-fetched. Unchanged from what the client derived. - window exhausted: the NEWEST tail among the sources that filled their window. A short source has nothing older to come back for and must not drag the cursor forward; resuming at the oldest tail instead would step over a newer source's unexamined rows, and repeats are absorbed by the client's dedup while gaps are not recoverable. Progress is guaranteed because every candidate is a row this page fetched and the store's cursor predicate is strict. Tests: an all-dropped window returns a cursor that reaches the history behind it; paging across a dropped MIDDLE stretch terminates and yields each renderable entry exactly once; and a control leg pins that an untruncated page still resumes at its last rendered entry — without it, "always resume from the oldest row touched" passes while silently skipping what truncation cut. The two-full-source selection rule is pinned as a unit test, because a full source whose rows RENDER puts 3x limit entries on the page and takes the truncation branch instead, so the handler cannot cheaply reach it. Mutation matrix, each independently detected: cursor from rendered entries; oldest tail instead of newest; the full-window flag ignored; no cursor at all. Client half follows in the next commit. Refs: BUG-2765 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): page the timeline with the server's cursor (BUG-2765) Client half. The component derived its next-page cursor from the last RENDERED entry, so a page the server had emptied by dropping rows either gave it nothing to page from (the first page, where loadMore returned early on entries.length === 0) or gave it the SAME cursor it already held (a later page, where nothing was appended and the oldest entry did not move). The second case is a permanent wedge with a live button and a running spinner. It now pages with next_before / next_before_id when the server sends them, and falls back to the last entry otherwise — which is exactly the old behaviour, including its wedge, and is there only for a server that predates the field. One press walks at most MAX_EMPTY_HOPS pages while every row keeps dropping. That bound is UX, not correctness, and its comment says so: a single hop is already correct now that the cursor advances; the loop exists so a user crossing a long run of read activity sees entries appear rather than a spinner and nothing, and it is small so a pathological item cannot turn one click into an unbounded request fan. Tests assert the cursors the component ASKS FOR, not only what it displays — a component that shows the right thing by re-fetching page one forever is the bug. Mutation matrix: ignoring the server cursor fails three of four legs (the fallback leg survives, correctly, since that is the path it pins); a single hop fails the advance leg; an unbounded hop count fails the bound leg. vitest 109 files / 1862 tests pass; vite build clean; svelte-check 0 errors (6 warnings, all pre-existing and in other files). Refs: BUG-2765 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix: cursor must clear BOTH bounds, and a later page must merge not append (codex round 1) Two findings, both real, both consequences of the cursor itself. P1 — truncation and an exhausted window are INDEPENDENT bounds, and the second is not implied by the first. A source whose rows all drop contributes nothing to the page, so the truncation cursor can sit older than that source's tail, and every unexamined row between the two falls in a gap neither page fetches. The cursor is now the NEWEST candidate across both reasons. Its regression puts one renderable activity in exactly that gap: two comments forcing truncation at limit=1, three read rows filling the activity window above them, and the row at risk in between. Run against the previous commit it comes back 0 times — the row is not late, it is gone. P2 — a later page can legitimately carry entries NEWER than the oldest one already shown, because the cursor deliberately re-covers ground when one source's window ran out before another's. Concatenating printed those below older entries. The client merges by (created_at desc, id desc) now, comparing INSTANTS rather than strings: precision is not uniform — the store writes whole seconds but a structured note can carry a sub-second timestamp — and lexicographically "…:05.123Z" sorts before "…:05Z". Mutation checks: truncation ignoring the exhausted candidate fails the new gap test; concatenating instead of merging fails the new order test and nothing else. internal/server suite green; timeline vitest 9 files / 83 tests green. Refs: BUG-2765 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): stop the hop loop when the cursor does not advance (codex round 2) Against a server that predates next_before, the fallback re-derives the same last entry on every hop, so a single Load More click fired five identical requests where the pre-fix component fired one. The client cannot give an old server a cursor it does not have — that wedge is the old behaviour and stays — but amplifying it was new, and mine. A cursor that did not move cannot make progress, so the loop stops on it. Its test asserts the request COUNT, which is the only thing that distinguishes this from the behaviour it replaces: the entries rendered are identical either way. Refs: BUG-2765 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): order the SSE-refresh merge too, and share one comparator (codex round 3) The refresh prepended its genuinely-new entries on the assumption that a fresh first page is always newer than everything on screen. Normally it is — but a structured note or decision carries a hand-written created_at and can arrive backdated, and the assumption was never stated, only relied on. Both merge points now go through one byNewestFirst, which is the server's own ordering and compares instants rather than strings. Two of codex's three round-3 findings are not folded in: - The refresh not adopting the response's has_more / cursor is DECLINED, not missed. The refresh re-fetches the NEWEST window, which says nothing about where the reader's paging frontier is; the stored cursor stays valid because the refresh consumes no older rows, and adopting the fresh page's has_more after the reader has paged deeper would point the cursor back at history they already hold. One wasted request, absorbed by the no-advance stop, in exchange for a correctness claim I cannot make. - firstPageIds treating any entry missing from a refreshed first page as deleted is real, pre-existing, and a semantics call about what counts as a deletion rather than a patch: filed as BUG-2773. Refs: BUG-2765, BUG-2773 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(server): derive the expected entry set instead of naming two ids (codex round 4) The test claimed every renderable entry came back exactly once and checked the two ids it had seeded. The item's own `created` activity could have vanished or repeated underneath that claim — the same partial-verification shape as asserting one direction and writing the symmetric conclusion. The expectation now comes from the store: every activity on the item minus the kinds buildTimeline drops unconditionally, compared in both directions, so a fixture that grows a row cannot fall outside what the test says it covers. Still fails with the cursor withheld. Refs: BUG-2765 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * docs: state the timeline cursor contract where consumers read it (codex round 5) The TypeScript type documented next_before/next_before_id, but the handler's own doc comment — what a REST consumer reads — still described before + limit only, and the API client method said nothing. A consumer following either could still derive a cursor from its last visible entry, which is precisely the invalid contract this change exists to replace. Both now state the pair, that it must be forwarded rather than re-derived, why (dropped rows make the last entry a different position, sometimes no position), and that the id is the tie-break among entries sharing a second — send both or neither. Refs: BUG-2765 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(web): clear paging state when the timeline reloads (codex round 6) The previous item's entries stay on screen while the new one's page 1 is in flight — the list renders on `!loading || entries.length > 0` — so Load More is clickable during a switch, and its cursor was the OLD item's position aimed at the NEW item. Pre-existing in shape (the pre-fix code derived the same stale position from the same stale entries), but now it is one line to close, and the dedicated cursor variable is mine. loadTimeline replaces entries with page 1 when it resolves, so clearing the cursor and has_more before the await throws away no state that would have survived; it just stops offering paging for a position that is no longer known. Test holds the switch's fetch unresolved and asserts no request goes out in that window, with the button's absence as the observable. Fails with the two lines removed. Refs: BUG-2765 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-25 16:11:47 -04:00
xarmian	e747a1610c	feat(session): registry keyed on the harness session, carrying the agent name; pad session list / prune (TASK-2767) (#1200) ... ## Summary TASK-2767 (IDEA-2750 part 2, with part 3 riding along — the keying fix and the reaping are one mechanism). The local session registry (`~/.pad/sessions`) was keyed on the pid of the `pad session register` subprocess, which is dead before anyone reads the file. One session left a new file per call and its own pid appeared in none of them; the only live identifier was the harness pid a reader could parse out of the socket path's basename. In practice nothing wrote it (zero callers in `plugin/`, `skills/`, or hooks) and nothing read it. Now: - **One record per session, keyed on the harness session pid** — `$PAD_SESSION_PID` (harness-agnostic override), else `$CLAUDE_PID` (verified present in both the tool shell and a live plugin monitor's `/proc/<pid>/environ`), else the calling process. A set-but-invalid value is an error, not a silent fall-through. - **The record carries the agent name** the session's writes are attributed to (`ResolveAgentName`: `.pad.toml agent_name` → `$PAD_AGENT` → detected runtime; `--agent` overrides, `--agent ""` is anonymous), the harness session id, and the messaging socket's identity (inode/device/mtime — the same binding the arm-state file uses). - **One owner-identity type, one verdict.** `internal/cli/session_owner.go`: `SessionOwner` + tri-state `OwnerLiveness` (`alive` / `dead` / `unknown`). `armStateOwnerAlive` is now `OwnerLiveness(...) == alive` with its file contract preserved (socket identity else mtime; headless pid + start token; fail closed). The registry pruner takes the opposite posture on `unknown`: on Windows `pidAlive` reports dead for every pid, and a reaper built on that would delete every live session's record. - **Verbs:** `pad session register [--agent]` (writes/refreshes; prunes dead records), `pad session list [--agent] [--cwd] [--all] [--format json]` (liveness per row, newest first; dead hidden unless `--all`), `pad session prune [--older-than DUR]` (dead always; unknown only under an explicit bound; alive never). Nothing on MCP — host-local filesystem state. - **Who registers:** `plugin/scripts/pad-monitor.sh` runs `pad session register` on start, BEFORE the consent gate — presence is a fact, consent is a grant, and the record is local/0600/never on the wire. - **Legacy v1 files** list as `legacy` rows: owner = socket-basename pid (else registrar pid), liveness by pid only (v1 recorded no socket identity, and the socket-without-identity rule would have judged every legacy record dead while its session ran). A legacy row can say a session exists, never who it is. Lead rulings on the four open decisions, all as built: `agent`/`--agent` vocabulary; no server-presence merge in `list`; register from the monitor script before the gate; wire follow-on (agent name on the stream) filed separately as IDEA-2750 part 2b. One ordering change from the plan's section A: pid precedence is `PAD_SESSION_PID` > `CLAUDE_PID` > self (explicit override beats detection, mirroring `PAD_AGENT` over runtime detection); the plan listed `CLAUDE_PID` first. ## Behaviour changes for existing users of `~/.pad/sessions` / `pad session register` - Registry files are keyed on the **harness session pid** (`PAD_SESSION_PID` → `CLAUDE_PID` → self), not the `pad` command's pid; repeated registrations overwrite one record instead of accumulating. - `pad session register` records the agent name, harness session id and socket identity; stores the **real path** of the cwd; prints a different text line and a different JSON shape (the full `SessionRecord`); and **rejects** an invalid `PAD_SESSION_PID` / `CLAUDE_PID` instead of silently keying on itself. - Existing v1 files are read as `legacy` rows (owner = socket-basename pid, no agent name) and dead ones are pruned by the next register. - The plugin monitor now registers (and prunes) on every start, before the consent gate. - `armStateOwnerAlive` now delegates to the shared `OwnerLiveness`; the consent gate's observable behaviour is unchanged on every platform and key type (codex round 4 traced every caller; matrix M29 pins the socket-keyed mapping). https://claude.ai/code/session_016zc6oxBvpax6Z3iQMsAJno	2026-08-25 15:31:16 -04:00
xarmian	3a63b52334	fix(store): activity debounce must not merge across writers (BUG-2763) (#1201) ... * fix(store): activity debounce must not merge across writers (BUG-2763) CreateActivityDebounced coalesced two "updated" writes whenever they shared a document, an action, a user account and the cooldown window. The account is not the writer: an agent authenticates as the human it works for, so a human's write and an agent's write — and two different agents' writes — matched each other. The surviving row is wrong in both orderings, because the merge UPDATE writes only metadata and created_at (so the row keeps the FIRST write's actor) while mergeActivityMeta overlays the incoming `agent` key last-writer-wins: human then agent — row stays actor='user' and the agent's name is ignored, so the agent's edit renders as the person's. agent then human — row stays actor='agent' and no incoming key overlays the stale name, so the person's edit renders under the agent's name. Both are visible since TASK-2760 named the actor: TimelineActivityCard reads the stamped name only when actor == 'agent'. Refuse the merge unless the candidate row's actor AND its metadata agent name both equal the incoming write's; on refusal fall through to CreateActivity, the same fall-through the comment-link refusal already uses. The check is in Go rather than in the UPDATE's predicate, where the comment-link refusal lives (TASK-2760 codex round 6), because nothing can invalidate it between the read and the write: actor is written once at INSERT and no statement here updates it, and the metadata agent key can only be rewritten by a merge that passed this same check, which leaves the name equal to what it already was. A predicate in the UPDATE would guard a change that cannot happen. Tests drive the real PATCH route with and without X-Pad-Agent and read the timeline endpoint (CONVE-19: the binding, not the store call), covering both orderings and two agents on one account, each with a control leg asserting that a same-writer run still coalesces — without which "never debounce" would pass. Store-level legs cover the same matrix directly. Refs: BUG-2763 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(store): make the debounce identity matrix order-independent and kill the actor mutant The first matrix run left one mutant alive: removing the actor comparison and keeping only the agent-name comparison passed every leg. That is the pair-only- dies-together shape, so it needed an answer rather than a note. The reason it survived is real and worth stating: through the only production caller, actor and agent name both derive from the X-Pad-Agent header, so they cannot disagree, and comparing names alone catches every header-driven case. The store's API admits the decoupled state, and the new leg is what it means — a caller declaring an agent write without stamping a name is still not the human. That leg kills the mutant at the layer the guard lives in; the handler suite legitimately cannot, and the comment says so. Also stopped asserting row order. Both writes land in the same second and ListDocumentActivity orders by created_at alone, so ties come back driver- dependent — stable on SQLite, unordered on Postgres, where this suite also runs. Rows are matched by the change each write recorded instead. Refs: BUG-2763 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(store): pick the debounce candidate by writer, not by recency alone (BUG-2763) Codex round 1 on this branch: the identity guard fixed attribution and broke the coalescing it guards. created_at is whole-second, so with the candidate query still taking the newest row overall, a writer returning inside the window lands on the OTHER writer's row, is refused on identity, and starts a third row — exactly the per-save spam the debounce exists to prevent. Select the recent rows for THIS writer instead: actor as a SQL predicate, and the agent name matched in Go over the candidates (it lives in the metadata JSON and this store targets two dialects whose JSON accessors differ). The first candidate whose name matches is the row to extend; none means a new run. maxDebounceCandidates=10 bounds the scan, not the semantics — past it the loop fails to find the writer's own row and starts a new one, costing an extra row and never a wrong attribution. Its comment carries that reasoning. The regression drives agent → human → agent with real 1.1s gaps rather than three writes in one second: the same-second form is ordered by whatever the driver returns, so it would fail on one backend and pass on the other for reasons unrelated to the fix. Strictly increasing timestamps make the wrong candidate deterministically the newest row. Run against the pre-fix selection it produces 3 rows. Refs: BUG-2763 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * docs(store): correct why the agent-name match is in Go, and sharpen the scan bound Two comment corrections, no behaviour change. The first claimed the name is matched in Go because the two dialects' JSON accessors differ. They do, but this store HAS a portable accessor (s.dialect.JSONExtractText), so that was not the reason and reads as though nobody checked. The real reason is agreement with the renderer: models.AgentNameFromMetadata is the twin of the accessor the timeline uses, and a SQL predicate would disagree with it wherever a stored value is odd — a non-string agent reads as absent in Go and as its text form in SQL, and malformed metadata is one skipped row in Go versus a failed query on SQLite. The second described maxDebounceCandidates as covering "the writers that plausibly interleave", which overstates what competes: the query already filters to one document, one account and one actor kind, so only rows carrying a DIFFERENT agent name can crowd the writer's own out. For a human write that is normally none, and only possible at all for rows written before this fix. Codex round 2 read the bound as incomplete coalescing; declined on those grounds, with the reasoning recorded where the number lives. Refs: BUG-2763 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * docs(store): three comment corrections from codex round 4 No behaviour change; each of these was a claim in a comment that did not survive being checked. - "mergeActivityMeta overlays agent last-writer-wins" is only half true: it overlays keys the INCOMING metadata carries, so a write with no agent key leaves the stale name standing. That asymmetry is precisely the agent-then- human direction of this bug, so the comment now states both directions. - The SQLite half of the dialect-divergence note said json_extract yields a non-string value's "text form". It yields a native number or boolean, which no text comparison matches. Same conclusion, correct mechanism. - The comment-link refusal's residual window was described as ordering ("an update that completes before the comment links its row at all"). Visibility is the better frame: the comment row is written in its own transaction, so on Postgres a link committing while the UPDATE's snapshot is already open is missed exactly as an unwritten one is. Still BUG-2716's to close. Codex's fourth finding — concurrent same-writer calls read-modify-write the same metadata blob and the second UPDATE silently drops the first's change entry — is real, pre-existing, and orthogonal to this fix. Filed as BUG-2770 rather than folded in. Refs: BUG-2763, BUG-2770 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * test(server): run the debounce attribution test on a real account (codex round 5) The test's own comment said both writes ride one account. They rode none: doAttributionRequest sends no session and no bearer token, so currentUserID was empty and every row landed with a NULL user_id — which coalesces through the predicate's IS NULL branch, a different path from the one the bug is about. A regression in the authenticated handler-to-store user-id flow would have passed here, and the claim in the comment was simply false. Bootstrap an owner, create the workspace and item as that owner, and send every PATCH with its bearer token, varying only X-Pad-Agent. The account is now also ASSERTED rather than described: each entry must carry the same non-empty user_id, so the premise fails loudly instead of quietly becoming untrue. Mutation matrix re-run against the reworked test: dropping the agent-name match fails two-agents-on-one-account, dropping both fails all three identity legs. Dropping the actor predicate alone is still store-only, for the reason already recorded there — actor and agent name both derive from X-Pad-Agent, so no handler-path input can separate them. Refs: BUG-2763 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * docs+test: sweep the prose this change falsified, and de-vacuum one leg (codex round 7) CONVE-23: two statements elsewhere were true when written and are not any more. handlers_items.go said a concurrent same-user update can debounce-merge into the not-yet-linked activity and overlay the agent stamp the comment will carry. Since the identity guard it can only merge as the SAME writer, so the stamp it overlays is the one the comment already carries; what survives is the created_at bump, still BUG-2716's window. comments_agent_name_test.go's rationale described the cross-agent re-attribution it was built for. The same change that made that prose stale also weakened the test: its second assertion (second != first) was left passing on the identity guard alone. MEASURED, not assumed — with the comment-link predicate deleted and the two agent names restored, that assertion still passed and only the final leg caught the deletion. Every write in it now declares one name, which puts the refusal back as the only mechanism that can split those rows; the cross-agent shape moved to the BUG-2763 matrix. Sweep boundary: grepped Go under internal/ and cmd/ for debounce/coalesce vocabulary, and .go/.md/.svelte/.ts repo-wide for overlay / re-attribute / last-writer / agent-stamp phrasing. Two other sites (reports.go, status_transitions_backfill.go) describe coalescing as undercounting rapid hops, which stays true — there is simply less coalescing. Web-side hits are all client SSE-refresh debouncing, unrelated. Codex's remaining round-7 finding — the remote /mcp dispatcher never sets X-Pad-Agent, so every write over that transport records as the human and no identity fix in the store can see through it — is real, pre-existing, and needs a design call about where the name comes from. Filed as BUG-2772. Refs: BUG-2763, BUG-2772 Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-25 14:28:29 -04:00
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	cc3cfeef2b	fix(redis): honour the caller's context on TLS dials (BUG-2754) (#1198) ... * fix(redis): honour the caller's context on TLS dials (BUG-2754) go-redis's default dialer (v9.22.0, options.go NewDialer) honours the caller's context on plaintext and NOT on TLS: the TLS branch returns tls.DialWithDialer, which takes no context at all, so a cancelled caller could not shorten the dial and it was bounded only by DialTimeout. BUG-2749 put SSE subscription establishment on the request's context so a client that disconnects stops holding its admission slots. On plaintext that covered the dial. On TLS the dial was the one segment cancellation could not reach, so the guarantee shrank from "released at once" to "released after up to DialTimeout" — and a managed Redis is a rediss:// URL, which is the ordinary production shape rather than an exotic one. Fixed at CLIENT CONSTRUCTION rather than in any consumer, because the same dial serves Publish, the Lua scripts, the presence registry and the watch bus's reads. internal/redisdial is a small package so the thing can be tested directly; cmd/pad/cmd_server.go installs it on the one client Pad builds. THREE THINGS THAT FAIL QUIETLY IF THE REPLACEMENT GETS THEM WRONG, each with a test that fails against getting it wrong: ServerName. tls.DialWithDialer infers it from the dialled address when the config leaves it empty; a hand-rolled tls.Client does not, and an empty ServerName leaves certificate verification with no name to check. That would turn a latency fix into a silent authentication regression. Replicated, on a CLONE — mutating the caller's config would leak one host's name into every later dial that shares it. Tested by dialling a certificate issued for another name and requiring an x509.HostnameError, per the lead's correction: asserting the field is set proves the code sets a field, not that the name is checked. Verified in the pinned source rather than assumed — redis.ParseURL DOES set ServerName for rediss:// (options.go:708), so Pad's path does not depend on the fallback today; it is there because it is what the replaced code did. The timeout must bound the HANDSHAKE, not just the connect. Otherwise a server that accepts and then stalls hangs for as long as the context lives — trading a bounded failure for an unbounded one, worse than the bug being fixed. It must not EXTEND an earlier deadline. context.WithTimeout takes the sooner of the two, matching go-redis's own promise about DialTimeout. PROSE SWEPT, and the sweep found two sites my first pass missed because it only grepped non-test files: five comments across internal/events said the TLS dial could not be cancelled, including one carrying an explicit "See BUG-2754 for the TLS half" forward reference. All five now say what is true, and the confirmTimeout budget comment records that it has been amended twice. Two instrument corrections: the certificate fixture put IP literals in DNSNames where x509 will never match them, and two tests detected their mutations BY HANGING — which is not a result anyone can act on, and which stranded the mutation harness with its edit still applied. Both bound the dial in a goroutine now, so a hang is a named failure. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(redis): resolve DialTimeout, keep the keep-alive, share one budget (r1) Codex round 1 found three, and the P1 was introduced BY the first draft of this fix rather than inherited — the worst kind, since the diff was sold as closing a hang. DIALTIMEOUT READ AS ZERO. go-redis's Options.init() defaults it to 5s, but NewClient CLONES the options first (redis.go:1924), so a caller reading opt.DialTimeout in order to install a Dialer — the only time it can — reads zero for the ordinary URL that sets none. And PubSubPool.NewConn calls the dialer DIRECTLY with no timeout of its own (internal/pool/pubsub.go:45), so nothing downstream supplies one either. Resolved in the package, with the coupling named. The mutation matrix then refused to confirm the failure mode the finding described, which changed the test rather than the fix. An unresolved zero does not hang HERE: this dialer wraps the dial in context.WithTimeout, and a zero duration is an already-expired deadline, so every dial would fail INSTANTLY — nothing connects at all. The original draft would have hung; this one refuses. The assertion that separates them is a healthy server being reached, not a stalled one giving up, and the comment says which draft did which. KEEPALIVECONFIG DROPPED. go-redis's default dialer sets it (options.go:608) and it governs how quickly a dead peer is noticed on every Redis connection this process holds. Reverting to OS defaults would change that across the whole client as an invisible side effect of a cancellation fix — invisible because nothing fails. My first test for it compared our copy against go-redis's published numbers, which says nothing about whether the dialer USES it: deleting the field from the dialer left that test green. Replaced with a Linux-tagged test that reads SO_KEEPALIVE and TCP_KEEPIDLE off the accepted socket. Honest partial, stated at the test: the property is platform-independent, the observation is not, and the Smoke jobs on macOS and Windows skip the file. The value-comparison test is kept as well — it catches the copy drifting from what it mirrors, which the socket test cannot. TWO SEPARATE BUDGETS. DialTimeout was applied to the TCP connect and then a fresh one started for the handshake, allowing up to 2x on the pub/sub path, which has no outer deadline to mask it. tls.DialWithDialer bounds both as one interval; this must not be laxer than the code it replaces. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(redis): honour an explicitly disabled dial timeout; finish the sweep (r2) Codex round 2, four findings and one correction to a claim I had already made. EXPLICIT dial_timeout=0 WAS BEING OVERRULED. ParseURL encodes an explicit zero or negative as -1, which go-redis preserves as "no timeout at all". Treating every non-positive value as unset collapsed that into the 5s default and silently overruled an operator who had deliberately disabled the bound — the same defect as the one round 1 found, in the opposite direction. `== 0` for the unresolved case now, with a negative carried through as no bound, and a test that goes through ParseURL rather than passing -1 by hand so it pins the real path. A DRIFT GUARD for the two copied constants, compared against go-redis's RESOLVED options (NewClient runs init() on its clone and Options() returns the result) rather than against a literal. A copy that silently diverges from what it mirrors is what would make this package worse than none. TWO STALE COMMENTS I HAD CLAIMED WERE FIXED. My sweep commit said "all five now say what is true"; it was three. The first patch batch aborted on a failed anchor and, because that helper writes only after every pair matches, none of its edits landed — I re-applied some by hand and did not re-verify the rest. The grep I ran afterwards searched for phrasings the surviving comments did not use. Both now corrected: establishSubscription's two-bullet plaintext/TLS split and the mutex comment that named TLS as the case cancellation could not reach. TWO TESTS RELABELLED RATHER THAN LEFT LOOKING LIKE COVERAGE. The single-budget test does not discriminate — the server accepts immediately, so the connect consumes none of the budget and the two-budget implementation finishes in the same time. Staging a slow connect against a local listener is not deterministic, so what holds that property is structural (one context, created before the connect, passed through the handshake) and the test says so. And the Linux-only keepalive test now states what its build tag does and does not cost: the behaviour is platform-independent and the full suite runs on Linux CI, so a removal is caught; the macOS and Windows Smoke jobs are build-and-start checks and were never the guard. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-25 00:27:27 -04:00
xarmian	d8b098fbed	fix(watchevents): bound the resume settle window by the request context (BUG-2751) (#1197) ... * fix(watchevents): bound the resume settle window by the request (BUG-2751) GET /api/v1/events/stream takes a global AND a per-user admission slot before subscribing (BUG-2726) and releases them by defer when the handler returns. The resume path could WAIT inside that window: resumeOutrunsLocalView does a Redis GET, waits 250ms for propagation, then does a second GET -- and its select waited on b.ctx, the BUS's lifetime, never the request's. A resuming client that disconnected mid-window held both slots for the remainder of it plus two round trips. The connection was gone; the capacity was not. The context is threaded through SubscribeAndReplaySince -> resumeOutrunsLocalView -> sharedCounter and MERGED with the bus context rather than replacing it. Swapping to the caller's alone would trade one leak for another: b.ctx is what lets Close cut a wait short, so dropping it leaves shutdown blocked behind a client that is still perfectly connected. internal/events lost exactly that half in its own first draft. Both endings are reasons to stop, and each has its own test that fails against the other one's implementation. ENDING THE WAIT EARLY IS HALF A FIX (codex round 1). resumeOutrunsLocalView answers false on cancellation, which reads as an ordinary converged resume, so the rest of the call went on to register a subscriber and build a replay slice for a connection that was unwinding. A cancelled caller is now declined outright, returning the same shape as the closed-bus branch -- a closed channel, never nil, because the handler treats nil as "fall back to plain Subscribe" and would have re-registered the very caller being declined. MemoryBus was CHECKED rather than assumed clear, which is the scope note this bug carries and also how it was found (BUG-2749's filing asked for this package to be checked). It has no bounded wait and no I/O, so there is nothing for a cancelled caller to stop paying for -- but it declines one too, because the two implementations must not disagree about whether a departed client ends up registered. That divergence is invisible on a single-process deployment right up until it is a leak on a clustered one. Six tests, each failing against the specific thing it names: bus-ctx-only, caller-ctx-only, an implementation that never settles, no decline on RedisBus, no decline on MemoryBus, and -- the binding one, in internal/server -- a handler that passes context.Background(). The last is what CONVE-19 asks for: the bus tests vouch for the bus honouring cancellation and say nothing about whether the handler ever hands it one. The mid-settle cancellation lands through a new positional seam rather than a sleep. A sleep-timed cancellation that arrives late does not fail safe here; it silently measures the already-cancelled path instead. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(watchevents): decline a cancelled resume before it touches Redis (r2) Codex round 2 verified round 1's fixes and came back clean on the two dimensions that could have made this change dangerous: every caller handles the declined closed channel correctly (the SSE handler falls back to plain Subscribe only on NIL, and the CLI treats EOF as reconnect), and there is no path where a LIVE caller's context is cancelled -- the route has no timeout middleware, and bus shutdown stays separately bounded through the merged context. Its P3 was a real one: the cancellation check sat AFTER resumeOutrunsLocalView, so an already-cancelled caller still entered it and made the first Redis GET. That fails on the dead context and logs "could not read the sequence counter to validate a resume; answering from local knowledge only" at WARN -- a line that means "Redis is unhealthy" to whoever reads it. Ordinary disconnect churn would have fired it on every client that hung up a moment before its resume landed. Moved ahead of the settle path. A METRIC WAS DECLINED, with the reasoning at the code. The finding asked for a cancellation counter so operators could distinguish disconnect churn from no resume activity. A client hanging up during its own resume is ORDINARY on a mobile network, so that counter would be a number nobody can act on, sitting next to pad_watchevents_resume_gaps_total where it would read as a fault. The condition an operator does act on -- capacity held by connections that no longer exist -- is already visible in the admission counts, and this change is what keeps those honest. Debug log instead. THE FIRST INSTRUMENT FOR THIS MEASURED NOTHING. I asserted "no Redis reads" as a proxy for "no misleading log", and go-redis short-circuits a cancelled context before it touches the wire -- so no GET reaches miniredis whether or not the early decline exists, and removing it survived. The assertion is on the LOG now, through a capture handler, because the log is the only thing that distinguishes the two. Fails with the exact WARN quoted back. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(watchevents): our own cancellation is not a Redis fault (codex r3) Round 3 approved with comments and found the twin of round 2's finding. The entry-side decline catches a caller that was ALREADY gone; a caller can also leave while the first counter GET is in flight, and the error that comes back is context.Canceled -- indistinguishable, at the sequence-counter WARN, from Redis being unreachable. On a stream where clients hang up mid-resume that line would manufacture exactly the alarm an operator would chase. Suppressed to Debug when ctx is already dead, with a test that intercepts the GET from inside miniredis's command hook -- cancelling there and failing the command is what makes it the in-flight case rather than the entry case, without any timing. Fails with the exact WARN quoted back. Also cleared by that round, recorded because each was a real question rather than a rubber stamp: no self-sustaining state; slog.SetDefault is restored and the capturing test is non-parallel so it cannot bleed into package t.Parallel() tests; and whicheverEndsFirst is a correct second package-local copy of internal/events.mergeCancellation rather than a candidate for extraction -- a four-line shared utility would be coupling two packages for nothing. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X * fix(watchevents): classify the caller-gone case on the error, not the context Codex round 4 BLOCKED, correctly. Round 3's suppression asked ctx.Err() rather than what the error actually was, so a GENUINE Redis failure arriving while the context happened to be dead would be downgraded to Debug and disappear — the signal an operator most needs, hidden by the change meant to reduce noise. My own new test demonstrated it: it returned a real server error and asserted no WARN. Now a predicate on the error alone, callerIsGone(err), and extracting it is the substance rather than tidiness. The two integration legs I had written CANNOT tell the two forms apart: both agree on every case a live client can be made to produce on demand — a cancelled context yields a context error, a live one yields a server error — so that pair passed against the blocked implementation too. Verified by mutation rather than assumed; reverting to ctx.Err() left them green. They disagree on exactly one case, a Redis failure whose error arrives while the context is already dead, and staging that through a client is a race by construction because go-redis decides by timing which error it returns. As a predicate there is no timing, and that case is a table row. Kept all three: the predicate table for the classification, and the two integration legs for the wiring — a cancelled read stays quiet, a genuine failure still warns. The second is the control without which "no WARN" is satisfied by a bus that has stopped reporting Redis trouble at all. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-24 22:32:01 -04:00
xarmian	99ffad1bca	feat(server): timeline comment rows carry the agent name (TASK-2760) (#1196) ... * feat(server): carry the agent name onto comment rows in the timeline (TASK-2760) An agent's comment rendered under the human's name: the name is stamped only on the linked 'commented' activity, which the timeline suppresses because the comment card stands in for it. The comment list queries now LEFT JOIN that activity and surface the name as Comment.AgentName (top-level and nested replies, on the timeline and the comments endpoint alike, through one scan helper), mirrored onto comment-kind TimelineEntry.agent_name to match the actor_name idiom. The web comment card renders it verbatim in an isolated <bdi>, separate from the human author. Store join rather than a handler-side match: the two lists are paginated independently, so a handler join misses at page edges and reads as intermittently-correct attribution. Metadata is parsed in Go, not SQL, to keep the query free of a SQLite/Postgres dialect fork. * test(store): make the activity-window premise strict, not a same-second coin flip (TASK-2760) * fix(server): replies log + link their commented activity so the agent name reaches them (TASK-2760, codex r1) The dedicated reply route wrote no 'commented' activity, and the activity is the only row that carries the writing agent's name — so a reply through the web UI rendered under a generic chip no matter what the client sent. Also rewrites the README + SKILL.md claim that comments never show the name, moves the reply test onto the real route, and asserts order/limit under the join. * fix(store): exclude comment-linked activities in the timeline's activity query (TASK-2760, codex r2) buildTimeline suppressed a comment's linked activity only when that comment was on the same page; the two sources are paginated separately, so an activity could slip through as a standalone 'commented' card. The query now excludes linked rows via NOT EXISTS on idx_comments_activity (both dialects), exact regardless of either window, and the page-local guard is removed rather than kept as a dead one that reads as load-bearing. * fix(store): item-scope the comment/activity link and freeze comment-linked activities against debounce merges (TASK-2760, codex r3) The join keyed on activity id alone while nothing in the schema ties a comment's activity to its item — scope both the LEFT JOIN and the NOT EXISTS to the item. And CreateActivityDebounced could merge a later update into the 'updated' row a comment links to, overlaying its agent stamp and bumping created_at, so two agents under one set of credentials would silently re-attribute an earlier comment; comment-linked rows are no longer merge targets. Prose corrected: the linked row is a 'commented' row OR the 'updated' row of an update that carried the comment. * fix(server,web): keep the read-skew guard beside the SQL exclusion; nowrap on every 24ch agent label (TASK-2760, codex r4) The page-local guard covers a distinct failure from the query exclusion — a comment fetched then hard-deleted before the activity query runs — so it returns with that reason written down. Sweep: of the seven 24ch agent-label rules, three lacked white-space: nowrap (both timeline cards and EpisodeFeed), so a name with spaces wrapped instead of ellipsizing; the other four already had it. Prose nits corrected; the pre-link debounce race on update-with-comment is recorded on BUG-2716 with a pointer in the handler. * docs(server,cli): state the reverse read-skew at the guard and the CLI non-rendering decision (TASK-2760, codex r5) * fix(store): debounce merge refuses a comment-linked row inside the UPDATE itself (TASK-2760, codex r6) The read-then-write left a window in which a comment could link the chosen row before the merge overwrote its agent stamp. The merge is now one statement whose predicate re-checks the link under the row write, and a zero-row merge falls through to a fresh insert. Prose corrected: a later update looks past a frozen row, to an older unlinked one or a fresh one. * fix(store,test): one freeze mechanism, and the window-edge leak proven end to end (TASK-2760, matrix survivors) The debounce SELECT-side exclusion became redundant once the UPDATE's own predicate refused linked rows, and its 'look past to an older unlinked row' semantics folded a later change into an earlier entry — a linked row now simply ends the coalescing run. And the server suite could no longer tell the SQL exclusion from the restored in-memory guard, because it only exercised the same-page case; a test now drives the page-edge case codex found (comment outside its window, activity inside), where only the query can help. * fix(web): drop a duplicate nowrap in EpisodeFeed — the rule already had it (TASK-2760, codex r7) Corrects the round-4 sweep count: of seven 24ch agent-label rules, two lacked white-space: nowrap (both timeline cards), not three.	2026-08-24 18:09:02 -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	381d4b0add	Merge pull request #1194 from PerpetualSoftware/feat/TASK-2759-agent-name-surfacing ... feat(web): surface agent display names wherever agent actors render (TASK-2759)	2026-08-24 14:45:29 -04:00
xarmian	927202a7a1	docs(web): put the leaf-not-fragment rule where the next edit will read it (TASK-2759) ... Lead's one follow-up on the package: the round-8 reasoning had to live in the code, not only in the evidence. Two places. displayUser now says WHY the spoofing vector exists rather than only what was done about it: this is ResolveAgentName's documented attribution-honesty problem (agent_identity.go, "WHAT THIS IS NOT") arriving through the renderer. The header records honesty rather than identity because the actor authors it — and a surface that COMPOSES with an authored value inherits that, handing the author influence over the parts they did not write. The rule that falls out is stated for future edits: a self-declared value is a leaf, never a fragment something else is built around. That covers a new column, a tooltip, an export or a search summary, none of which exist yet. The same rule goes in agentActor.ts, since that is the file every surface imports and the first place a maintainer looks. Stated there as what it is — not a softening of the verbatim contract two paragraphs above it, because isolation alters no characters and rejects no names; it refuses to let one value redraw another. Comments only; no behaviour change. It moves the tip, so the gate pointing at 501ba836 is void and I have re-asked rather than carrying the green across. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 18:22:02 +00:00
xarmian	501ba836f4	test(web): close the generic-id gap in the admin suite (TASK-2759) ... The final mutation matrix, re-run on the tip, found one survivor in 26 (mutation, test-file) pairs: reinstating the retired GENERIC_AGENT_IDS filter in the shared helper left the admin suite green, because every fixture there used 'wren' — a value the filter would not have swallowed. Exactly the hole I closed in the feed and audit-log suites earlier in this run, repeated when I added this file eight commits later. The lesson landed in two files and not in my habit, so it is now a case in all four. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 18:15:23 +00:00
xarmian	3a213d9918	test(web): cover the overview binding and agentNameOf's own contract (TASK-2759) ... Codex round 14, reading the test files as a suite rather than one at a time. The overview tab renders the same rows through its own markup and its own writes-only filter, so it is a second BINDING and I had tested only the first — my own CONVE-19 rule, missed on the surface I added two commits ago. agentNameOf was pinned only by an equivalence assertion against the string form. Four of the five surfaces call the parsed-object form, so its edges were covered incidentally through component tests and not stated anywhere as its contract. Direct cases now: named, generic id unfiltered, and every not-a-name shape. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 18:07:59 +00:00
xarmian	de3c9b818f	feat(web): name the agent on the admin per-user activity views too (TASK-2759) ... Codex round 12 — and it corrects MY exemption, not codex's reading of it. I listed these two tabs as exempt because their local row type omitted `metadata`. True, and the wrong reason: handleAdminGetUserActivity serializes whole models.Activity rows, so the stamped name was on the wire the entire time and only the client type dropped it. By this unit's own discriminator — does the surface hold an Activity? — they were never exempt. Verified against the handler before changing anything. The consequence was the exact gap the audit log had, on the same rows: an admin reading a user's activity saw "Updated an item via cli" with no way to tell which agent acted. The lead ruled the audit log IN on this discriminator; these belong in for the same reason. Rendered with the same rules as every other surface — <bdi>, bounded at 24ch, title for the full value, nothing shown when no name was stamped. Tests assert the binding at this surface (CONVE-19), including the empty case, the non-agent case and the bidi one. Docs updated: the surface list in the README and both SKILL.md copies now names the admin console's audit AND per-user activity views. The precision of that list is what round 2 was about, so it moves with the code. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 17:53:12 +00:00
xarmian	817bb0a5ce	fix(web): widen the name bound off my own over-correction; assert bdi at every binding (TASK-2759) ... Codex round 11. P2 — the width bound was an over-correction, and it landed on people. The activity page marks any actor_name `named`, so the 16ch rule I added for hostile AGENT names was clipping ordinary human ones; the episode feed's 20ch did the same. One value now, 24ch, which fits an ordinary full name ("Alexandra Whitfield" is 19) while still bounding the pathological case. The number is written down as a judgement, not dressed up as a measurement, next to what it does NOT cover: `title` is unreachable by touch, so a clipped name is effectively unreadable on a phone, and a real disclosure affordance rather than a wider bound is the actual fix. P2 — only the audit cell asserted the <bdi> element; the other four bindings checked text and classes, so swapping bdi back to span passed all of them. Each now asserts the tag with a bidi-carrying name. P2 on the casing tests, declined with the reason already in the code: they assert the `named` class and not the CSS rule, because Svelte component styles are not injected under this vitest setup (0 style elements, so getComputedStyle resolves nothing). Covering the rule itself needs an e2e with a real browser. The boundary is stated in the test comment rather than implied away — a source-text assertion about the stylesheet would be an instrument with an adversary, not coverage. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 17:43:52 +00:00
xarmian	46e3430f2b	fix(web): restore the chip title, make the feed's fold-key test discriminating (TASK-2759) ... Codex round 10, reading the assembled files rather than the diff — both findings are the same class: a later round invalidated an earlier round's premise, and nothing in either diff pointed at the other. Round 5 removed the timeline chip's title because the chip never clipped. Round 8 then bounded that label at 18ch to stop a hostile name widening the card. So the label clips now and the reason for removing its title is gone; a long name was being truncated with no way to read the rest. Title restored, comment rewritten to say why it is there. The EpisodeFeed test claiming to prove the fold key follows the agent name put its two events on DIFFERENT items, which yields two cards no matter how the actors are keyed — it only ever proved that labels render. Same item now, same window, two names, and the card count is asserted, so a fold that ignored the name produces one card and fails. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 17:33:25 +00:00
xarmian	0d08e7004c	fix(web): isolate self-declared agent names so they cannot rewrite the audit around them (TASK-2759) ... Codex round 8, probing adversarial names — the sharpest finding of the run and a defect this unit introduced. The agent name is text chosen by whoever is writing, and the admin audit log built its cell as `${agent} (via ${human})`. A writer could therefore pick a name that forges the construction (`admin (via root)` renders as nested attribution), or one carrying U+202E, which reorders everything appended after it — the audited party editing how the audit reads. Not an auth bypass: the stored actor stays correct. It is an audit-integrity defect, on the one surface whose job is to be trusted when trust is in question. displayUser now returns the PARTS and the template renders them as separate elements, each in its own <bdi>. That bounds a hostile name to its own isolate: it still displays exactly as sent, but it cannot reorder the " (via " literal or the account name, and the account half is structure rather than string, so a name spelling "(via root)" is visibly text inside the agent's element. The via span is styled distinctly for the same reason. Swept the sibling renders rather than the reported one (CONVE-18): the two badges, the episode label, the timeline chip and the human name beside it are all <bdi> now, since every one of them sits inline next to other text. P3 from the same round — the timeline chip and the audit User cell were the two name surfaces still unbounded. Both bounded, ellipsis, full value on the title where the element clips. This does NOT weaken the verbatim contract, and the distinction is the whole point: isolation changes no characters and rejects no names, it just renders each value as its own unit. Storing raw and rendering safely are compatible; an allow-list would not be. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 17:24:21 +00:00
xarmian	08b165af05	fix(web): restore the audit-log formatter guard I narrowed (TASK-2759) ... Codex round 6, and it was my own regression from round 5. Hoisting the row parse out of formatMetadata left its try/catch wrapped around only the parse that had moved away, so the FORMATTERS below lost their guard. They can throw on well-formed JSON — `String(data.keys)` cannot convert `{"keys":{"toString":null}}` to a primitive — and what used to render an em dash would now break the admin audit page. The try now wraps the switch and the fallback, which is what it always covered. Absent and unparseable metadata behave as before. Also the test gap that let it through: this suite drove only action `updated` and read only the User column, so nothing here could see formatMetadata at all. Added two Details-column cases — the throwing one, and a known action proving the hoisted object actually reaches the formatter rather than only displayUser. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 17:12:10 +00:00
xarmian	454afe573a	perf+a11y(web): one metadata parse per audit row, drop a redundant tooltip (TASK-2759) ... Codex round 5. P2 — the admin audit log parsed each row's metadata twice once this unit added a second reader (displayUser alongside formatMetadata), on a table that grows through "Load more". Hoisted to one `parseMetadata` per row, passed to both. formatMetadata now takes the parsed object, which also removes the try/catch it no longer needs. Left alone, with the reason: the dashboard also reads metadata twice per row, but it renders at most ten and its other reader (parseActivityChanges) has callers outside this diff whose signature I am not changing for ten rows. P3 — the title I put on the timeline actor chip duplicated text that is always fully visible: that row wraps and the chip never clips, so it added no information and gives assistive technology the same string twice. Removed. The titles on the two badges and the episode label stay — those DO clip, and there the attribute is the only way to the full value. Codex reported clean on reachability (no code-reachable wrong-name case beyond the documented self-declared limitation and the excluded BUG-2763) and on history coherence. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 17:07:45 +00:00
xarmian	e09216dfc4	fix(web): keep named actors out of the anonymous fold key; type the dashboard fixture (TASK-2759) ... Codex round 4. P2 — the fold key was built from the DISPLAY LABEL, so an agent that sends `agent` in X-Pad-Agent folded together with every agent that sent no name at all: two different claims ("this actor" and "we have no name for this actor") sharing one key, which also contradicted the file's own comment about a named agent getting its own key. Named and anonymous are now separate namespaces. Swept the sibling rather than the reported half (CONVE-18): the user branch had the identical defect for a person whose display name is `cli` or `web`. Both fixed, both asserted. P2 test gap — the dashboard route fixture was typed `Activity`, but `recent_activity` is a REDUCED DTO with no id/workspace_id/document_id and an OPTIONAL metadata. A fixture richer than the real payload cannot fail when the payload changes, and it hid a reachable case: rows logged by the audit helpers never call agentMeta, so absent metadata is a shape the server really sends. Fixture now derives from DashboardResponse and both route suites cover absent metadata. P2 on activity debounce — real, verified against the store rather than taken on report, and outside this unit's web-only boundary. CreateActivityDebounced matches on (document, action, user) without actor, and its UPDATE leaves the original `actor` in place while mergeActivityMeta overlays the newer `agent` name, so a coalesced row can name the wrong writer in either direction. Filed as BUG-2763 with both orderings worked through. This unit did not cause it; it is the first thing to make it visible. Codex also reported SSR/hydration CLEAN and verified this diff's claims about the Go side against the Go code. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 17:01:33 +00:00
xarmian	6dc6499a6e	refactor(web): drop the one-use helper wrapper, bound name width (TASK-2759) ... Codex round 3. P3 — `agentActorLabel` had a single caller. Every other site already holds parsed metadata and reaches for `agentNameOf`, and each supplies its own fallback anyway, so the wrapper saved one `?? 'agent'` and cost an inconsistency in how five call sites looked. Removed; the reasoning stays in the file so it is not re-added. P2 — names are unbounded text in fixed-layout rows. Before this unit the agent badge held one of four fixed words; it now holds whatever a client put in X-Pad-Agent, while still being `flex-shrink: 0`, so one long name pushes the timestamp off the row. Bounded with an ellipsis at the two badges and the episode actor label (which has the same exposure for people's names, and had it before this change), with the full value on the title attribute. The timeline chip and the audit-log cell both wrap, so they take the title only. P2 on presentation consistency — three surfaces show the (agent, human) pair three ways, and this unit invented one of the three. Declined as scope rather than as wrong, and filed as IDEA-2762 with what a decision has to cover. Codex also independently confirmed the exempt set: comments, versions and structured entries genuinely do not carry the name, and the linked comment activity that does is skipped when the card renders. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 16:52:09 +00:00
xarmian	fa22b6680e	docs+test: correct two over-claims and pin name escaping (TASK-2759) ... Codex round 2, fresh angles. P1, accepted — my own docs over-claimed. The README and both SKILL.md copies said the name appears wherever agent actors appear, including "item timelines". Comments, version snapshots and note/decision entries carry the actor KIND and no name (that is the exempt set the plan named, and TASK-2760 files the comment half), so on a timeline only ACTIVITY entries show it. Both now say which entries carry it and which read "Agent". P2, accepted — the README's fallback was wrong in a way that mattered. When nothing resolves a name, the CLI omits X-Pad-Agent entirely (client.go:1884), so actorFromRequest records the write as "user": it is attributed to the PERSON, not to a generic "agent". Verified both call sites rather than reasoning from the label. The generic "agent" rows that do exist come from pre-naming writes and from audit events logged without agentMeta. P2, accepted — the name is attacker-influenced text and every test used benign values, so a rewrite to {@html} would have passed. Added a markup payload at two surfaces that build their labels through different paths, asserting no element is created and the text survives intact. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 16:45:35 +00:00
xarmian	d1c5c3976e	fix(web): stop the badge CSS upper-casing a stamped agent name (TASK-2759) ... Codex round 1. `.actor-badge` sets text-transform: uppercase, so the activity page's audit rows and the dashboard's recent activity rendered `Wren` and `wren` as the same pixels — the verbatim contract broken in CSS rather than in code, and invisible to every textContent assertion in the suite. The codebase already draws this line: `.actor-badge.user` opts out of the transform, because a human's badge carries a NAME while "agent" / "cli" / "web" are CATEGORY words that read as chips. A stamped agent name is a name, so it follows the same rule via a `named` modifier; the generic fallback stays a chip. Swept the class rather than fixing the two reported sites (CONVE-18): the other three surfaces are unaffected — Chip has no transform, EpisodeFeed's uppercase rule is .section-label ("HAPPENING NOW"), and the audit log's cell is untransformed. Two sites, both fixed. Tests assert the class the markup applies, and say so: Svelte component styles are not injected under this vitest setup (0 style elements, so getComputedStyle resolves nothing), which leaves the adjacent CSS rule outside what the suite can observe. The class is the half a refactor drops. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 16:40:05 +00:00
xarmian	a3ba6eec6c	docs: the "name your agents" story for agent attribution (TASK-2759) ... The README's For AI Agents section promised that agent actions are attributed, and said nothing about naming the agent — which was fair while nothing rendered the name. Now that five surfaces do, the section carries the precedence (.pad.toml agent_name -> $PAD_AGENT -> detected runtime), where the name shows up, and that Pad renders it verbatim rather than keeping a list of approved names. The honesty framing is QUOTED from ResolveAgentName's own contract comment rather than restated: the header is self-declared, an agent that omits it is indistinguishable from the human whose credentials it uses, and a human running `! pad ...` in an agent's terminal inherits that attribution. It is a label an actor chose, not evidence about who acted — which is also why the admin audit log shows both the agent and the account. Both SKILL.md copies gain one clause: the name an agent sends is now DISPLAYED, so a specific name beats a generic client id. Their existing attribution principle was already accurate and is otherwise untouched. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 16:32:19 +00:00
xarmian	39844197ce	test(web): close two coverage holes the mutation matrix found (TASK-2759) ... Per-file negative controls showed EpisodeFeed and the console audit-log tests staying GREEN when the retired GENERIC_AGENT_IDS filter was reinstated: neither file used a value the filter would have swallowed, so both measured 'a name reaches the surface' without measuring 'an unfiltered name does'. One claude-code fixture each. 7/7 mutations now detected per-file. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 16:26:41 +00:00
xarmian	0719286910	test(web): assert the agent-name binding from each consuming surface (TASK-2759) ... Five render sites, five consuming-side assertions (CONVE-19). The helper has its own unit tests, and a correct helper that a page never calls — or calls with the wrong argument — passes every one of them; the Audit view's defect was exactly that shape, with the metadata parsed three lines above the call that ignored it. Each file's load-bearing legs are the negative ones: the generic label a pre-fix build produced is asserted absent where a name is stamped, and asserted present for every stamp shape that carries no name (missing key, empty string, non-string, unparseable). Two also pin that a non-agent row never reads the stamp, since the metadata blob is shared and agentMeta merges into it by string splice. activityEpisodes.test.ts's shim case inverted with the shim it named. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 16:24:14 +00:00
xarmian	7e7d6e8efa	feat(web): render agents' stamped names wherever agent actors display (TASK-2759) ... The input half has existed since BUG-2542: the CLI resolves an agent name (.pad.toml agent_name -> $PAD_AGENT -> detected runtime) and sends it as X-Pad-Agent, and the server stamps it into activity metadata as `agent`. Nothing rendered it. Every agent write displayed as an undifferentiated "agent", and on the console audit log it displayed under the name of the HUMAN whose credentials the write rode on. Recon's discriminator: metadata.agent is stamped only by agentMeta(), reached only from logActivityWithMetaReturningID, so workspace Activity rows are the only carrier in the data model. Comments, versions, items, structured note/decision entries and SSE events record the actor KIND and no name. That makes render-vs-exempt mechanical rather than per-surface judgement: does this surface hold an Activity? Rendering (5 sites, each already holding the metadata): - the activity page's Live view fold (activityEpisodes.ts) - the activity page's Audit rows (getSourceLabel) - the dashboard's Recent Activity rows - TimelineActivityCard on the item timeline - the console audit log's user column Exempt, name absent from the payload: comment authorship (TASK-2760 files the server half), version cards, structured note/decision cards, the SSE toast, ItemDetail's "Created by", and the console UserActivityTab (its row type omits metadata). Retires the GENERIC_AGENT_IDS shim on its own stated retirement condition (CONVE-2757 rule 4, PR #1192): it filtered a hardcoded list of one team's client ids out of the Live view, which made display quality depend on that team's naming habits. Names now render verbatim -- no allow-list, no normalization, no title-casing; any transform is a doorway for a workspace's vocabulary to re-enter product logic. Historical claude-code rows render as claude-code, which is honest: a reader learns every write came from one undifferentiated client, which the filter concealed. The audit log renders both facts rather than replacing one with the other ("wren (via Dave)") -- the agent acted, and that account is who it acted as, and an ops surface needs both. The shim's test inverted with it, and the file's header doc asserted that "every current seat sends the generic client id claude-code" -- falsified by this change, so rewritten rather than left (CONVE-23). Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 16:15:44 +00:00
dependabot[bot]	3f9daa5c7a	chore(deps)(deps): bump the npm-minor-and-patch group (#1190) ... Bumps the npm-minor-and-patch group in /web with 21 updates: | Package | From | To | | --- | --- | --- | | [@tiptap/core](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/core) | `3.30.1` | `3.30.2` | | [@tiptap/extension-bubble-menu](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/extension-bubble-menu) | `3.30.1` | `3.30.2` | | [@tiptap/extension-code-block-lowlight](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/extension-code-block-lowlight) | `3.30.1` | `3.30.2` | | [@tiptap/extension-collaboration](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/extension-collaboration) | `3.30.1` | `3.30.2` | | [@tiptap/extension-collaboration-caret](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/extension-collaboration-caret) | `3.30.1` | `3.30.2` | | [@tiptap/extension-link](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/extension-link) | `3.30.1` | `3.30.2` | | [@tiptap/extension-placeholder](https://github.com/ueberdosis/tiptap/tree/HEAD/packages-deprecated/extension-placeholder) | `3.30.1` | `3.30.2` | | [@tiptap/extension-table](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/extension-table) | `3.30.1` | `3.30.2` | | [@tiptap/extension-task-item](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/extension-task-item) | `3.30.1` | `3.30.2` | | [@tiptap/extension-task-list](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/extension-task-list) | `3.30.1` | `3.30.2` | | [@tiptap/pm](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/pm) | `3.30.1` | `3.30.2` | | [@tiptap/starter-kit](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/starter-kit) | `3.30.1` | `3.30.2` | | [@tiptap/suggestion](https://github.com/ueberdosis/tiptap/tree/HEAD/packages/suggestion) | `3.30.1` | `3.30.2` | | [@tiptap/y-tiptap](https://github.com/ueberdosis/y-tiptap) | `3.0.8` | `3.0.9` | | [dompurify](https://github.com/cure53/DOMPurify) | `3.4.13` | `3.4.14` | | [mermaid](https://github.com/mermaid-js/mermaid) | `11.16.1` | `11.17.0` | | [@sveltejs/kit](https://github.com/sveltejs/kit/tree/HEAD/packages/kit) | `2.70.2` | `2.70.3` | | [marked](https://github.com/markedjs/marked) | `18.0.9` | `18.0.10` | | [svelte](https://github.com/sveltejs/svelte/tree/HEAD/packages/svelte) | `5.56.9` | `5.56.10` | | [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) | `8.2.1` | `8.2.2` | | [vitest](https://github.com/vitest-dev/vitest/tree/HEAD/packages/vitest) | `4.1.10` | `4.1.11` | Updates `@tiptap/core` from 3.30.1 to 3.30.2 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/main/packages/core/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.2/packages/core) Updates `@tiptap/extension-bubble-menu` from 3.30.1 to 3.30.2 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/main/packages/extension-bubble-menu/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.2/packages/extension-bubble-menu) Updates `@tiptap/extension-code-block-lowlight` from 3.30.1 to 3.30.2 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/main/packages/extension-code-block-lowlight/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.2/packages/extension-code-block-lowlight) Updates `@tiptap/extension-collaboration` from 3.30.1 to 3.30.2 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/main/packages/extension-collaboration/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.2/packages/extension-collaboration) Updates `@tiptap/extension-collaboration-caret` from 3.30.1 to 3.30.2 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/main/packages/extension-collaboration-caret/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.2/packages/extension-collaboration-caret) Updates `@tiptap/extension-link` from 3.30.1 to 3.30.2 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/main/packages/extension-link/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.2/packages/extension-link) Updates `@tiptap/extension-placeholder` from 3.30.1 to 3.30.2 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/main/packages-deprecated/extension-placeholder/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.2/packages-deprecated/extension-placeholder) Updates `@tiptap/extension-table` from 3.30.1 to 3.30.2 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/main/packages/extension-table/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.2/packages/extension-table) Updates `@tiptap/extension-task-item` from 3.30.1 to 3.30.2 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.2/packages/extension-task-item) Updates `@tiptap/extension-task-list` from 3.30.1 to 3.30.2 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.2/packages/extension-task-list) Updates `@tiptap/pm` from 3.30.1 to 3.30.2 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/main/packages/pm/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.2/packages/pm) Updates `@tiptap/starter-kit` from 3.30.1 to 3.30.2 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/main/packages/starter-kit/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.2/packages/starter-kit) Updates `@tiptap/suggestion` from 3.30.1 to 3.30.2 - [Release notes](https://github.com/ueberdosis/tiptap/releases) - [Changelog](https://github.com/ueberdosis/tiptap/blob/main/packages/suggestion/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/tiptap/commits/v3.30.2/packages/suggestion) Updates `@tiptap/y-tiptap` from 3.0.8 to 3.0.9 - [Changelog](https://github.com/ueberdosis/y-tiptap/blob/main/CHANGELOG.md) - [Commits](https://github.com/ueberdosis/y-tiptap/commits) Updates `dompurify` from 3.4.13 to 3.4.14 - [Release notes](https://github.com/cure53/DOMPurify/releases) - [Commits](https://github.com/cure53/DOMPurify/compare/3.4.13...3.4.14) Updates `mermaid` from 11.16.1 to 11.17.0 - [Release notes](https://github.com/mermaid-js/mermaid/releases) - [Commits](https://github.com/mermaid-js/mermaid/compare/mermaid@11.16.1...mermaid@11.17.0) Updates `@sveltejs/kit` from 2.70.2 to 2.70.3 - [Release notes](https://github.com/sveltejs/kit/releases) - [Changelog](https://github.com/sveltejs/kit/blob/@sveltejs/kit@2.70.3/packages/kit/CHANGELOG.md) - [Commits](https://github.com/sveltejs/kit/commits/@sveltejs/kit@2.70.3/packages/kit) Updates `marked` from 18.0.9 to 18.0.10 - [Release notes](https://github.com/markedjs/marked/releases) - [Commits](https://github.com/markedjs/marked/compare/v18.0.9...v18.0.10) Updates `svelte` from 5.56.9 to 5.56.10 - [Release notes](https://github.com/sveltejs/svelte/releases) - [Changelog](https://github.com/sveltejs/svelte/blob/main/packages/svelte/CHANGELOG.md) - [Commits](https://github.com/sveltejs/svelte/commits/svelte@5.56.10/packages/svelte) Updates `vite` from 8.2.1 to 8.2.2 - [Release notes](https://github.com/vitejs/vite/releases) - [Changelog](https://github.com/vitejs/vite/blob/main/packages/vite/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite/commits/v8.2.2/packages/vite) Updates `vitest` from 4.1.10 to 4.1.11 - [Release notes](https://github.com/vitest-dev/vitest/releases) - [Changelog](https://github.com/vitest-dev/vitest/blob/main/docs/releases.md) - [Commits](https://github.com/vitest-dev/vitest/commits/v4.1.11/packages/vitest) --- updated-dependencies: - dependency-name: "@tiptap/core" dependency-version: 3.30.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-bubble-menu" dependency-version: 3.30.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-code-block-lowlight" dependency-version: 3.30.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-collaboration" dependency-version: 3.30.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-collaboration-caret" dependency-version: 3.30.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-link" dependency-version: 3.30.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-placeholder" dependency-version: 3.30.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-table" dependency-version: 3.30.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-task-item" dependency-version: 3.30.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/extension-task-list" dependency-version: 3.30.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/pm" dependency-version: 3.30.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/starter-kit" dependency-version: 3.30.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/suggestion" dependency-version: 3.30.2 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: "@tiptap/y-tiptap" dependency-version: 3.0.9 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: dompurify dependency-version: 3.4.14 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: mermaid dependency-version: 11.17.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: npm-minor-and-patch - dependency-name: "@sveltejs/kit" dependency-version: 2.70.3 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: marked dependency-version: 18.0.10 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: svelte dependency-version: 5.56.10 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: vite dependency-version: 8.2.2 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: npm-minor-and-patch - dependency-name: vitest dependency-version: 4.1.11 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: npm-minor-and-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>	2026-08-24 11:58:36 -04:00
xarmian	1af9fa255f	test(mcp): guard the OAuth workspace allow-list population (TASK-2753) (#1193) ... The sweep TASK-2753 asked for found NOTHING WRONG. Every workspace-global MCP-reachable route already filters TokenAllowedWorkspaceSet, refuses allow-listed tokens outright, or is structurally exempt. The task's premise was wrong, and correcting it is half the value: the hand-filtering in handlers_workspaces.go and handlers_audit.go is not scattered evidence of an unswept class, it is the OUTPUT of BUG-2102 (PR #935, squash 9f6c1d8f), which closed five consent-scoping bypasses and centralized the allow-set logic. So the obligation has now been discharged by hand TWICE and is enforced by NOTHING — and nothing failed when an unfiltered route landed, which is the exact condition that produced BUG-2102. A sweep ending at "verified today" only schedules the third hand-sweep. Hence a guard rather than a report. The population rests on one verified fact: MCP-dispatched requests traverse the FULL middleware chain, so {slug} routes are gated by RequireWorkspaceAccess. buildHTTPRequest strips chi.RouteCtxKey specifically so routing runs from scratch against the root mux, and explicitly preserves TokenAllowedWorkspaces. Without that the sweep would have had to cover the whole catalog. WHAT THE GUARD DOES. Every routed command is DRIVEN and the URLs it issues are OBSERVED — routeTable mappers by calling them, specialRoutes and the item-link family against a recording handler. A command whose every URL carries a workspace segment needs no entry. One that issues a workspace-global URL must be classified with a reason, so allowlistCoverage holds exactly the cases where somebody had to decide something. The three classes now differ in what they ENFORCE, not just what they say: workspaceScoped is verified against the observed URLs; filtersAllowlist requires the named handler to contain a real allow-list call; exempt is either mechanically checked (no store access) or must open with JUDGMENT: so an argument cannot ride as a verified fact. Stale entries fail, reasonless entries fail, and the /{slug} subrouter is checked to still apply RequireWorkspaceAccess — the classifier's foundation, which nothing had asserted. NINE CODEX ROUNDS FOUND NINE DEFECTS, ALL IN THE GUARD. Recorded because the pattern is the point: an instrument that asserts facts about source is code with an adversary. - Two false greens. The recorder returned {}, so link drives died at their prefetch and the only URL observed was that (workspace-scoped) prefetch. Then the write assertion matched on path alone, so the un-* commands passed on the GET issued on the way to their DELETE. - A fail-open escalated across three rounds: URL shape is not proof of middleware coverage (workspace restore looks scoped and is a sibling of the subrouter), and the first fix's loose suffix match wrongly caught item restore, which IS inside. - braceBlock opened on a brace inside the "/{slug}" STRING LITERAL, so the subrouter block was zero-length and every gated route was reported as an unclassified sibling — beneath a comment asserting string-literal braces were not a problem here. - Comment blindness let the middleware assertion pass on a commented-out r.Use — the exact deletion it exists to catch — and was introduced inside a fix for comment blindness in the same file. Every one was found by RUNNING a control, not by reading. CONVERGENCE, at nine rounds and deliberately not CLEAN. Per the lead's criterion, a source-scanning guard converges when it covers the registration grammar the codebase actually uses AND fails closed outside it. Both hold. The remaining findings concern styles nobody writes here (r.With, r.Mount, paths in variables, block comments), and teaching the scanner to TRUST more shapes would cost the fail-closed posture that makes the rest sound. The recognized grammar and that posture are stated in the file as a CONTRACT: routes registered via unrecognized forms FAIL until the scanner is taught them, so a future style migration reads the red as the guard asking to be taught rather than as the guard being wrong. ALSO CORRECTED: the workspace-create exemption originally claimed no capability existed for creation and that a created workspace is unreachable by its creator. Both false — may_create_workspaces exists, and maybeAutoAddCreatorConnection adds the new workspace to the allow-list when it is set (PLAN-1519/TASK-1521). IDEA-2756 was filed on that wrong premise and has been corrected; the live question is whether may_create_workspaces=false should REFUSE creation rather than merely skip the auto-add. Gates: make test 27 pkgs, make lint 0 issues, full Postgres suite 27 pkgs, CI 7/7. Rebased onto current main before gating so the green is measured against the base it merges into. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 11:56:02 -04:00
xarmian	8a95d29a15	chore(web): name the GENERIC_AGENT_IDS shim's retirement condition (CONVE-2757) (#1192) ... Product code temporarily encoding a convention-shaped assumption carries the item whose completion deletes it: IDEA-2750 part 1. Claude-Session: https://claude.ai/code/session_01HvAuiZ7JaWyCqqyV99LyWt	2026-08-24 11:26:44 -04:00
dependabot[bot]	019c335a87	chore(docker)(deps): bump golang in the docker-minor-and-patch group (#1188) ... Bumps the docker-minor-and-patch group with 1 update: golang. Updates `golang` from 1.26-alpine to 1.27-alpine --- updated-dependencies: - dependency-name: golang dependency-version: 1.27-alpine dependency-type: direct:production dependency-group: docker-minor-and-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>	2026-08-24 11:09:33 -04:00
dependabot[bot]	55fce493f1	chore(ci)(deps): bump docker/setup-buildx-action (#1189) ... Bumps the actions-minor-and-patch group with 1 update: [docker/setup-buildx-action](https://github.com/docker/setup-buildx-action). Updates `docker/setup-buildx-action` from 4.2.0 to 4.3.0 - [Release notes](https://github.com/docker/setup-buildx-action/releases) - [Commits](https://github.com/docker/setup-buildx-action/compare/bb05f3f5519dd87d3ba754cc423b652a5edd6d2c...37fe631027851001ddb9b187196cc803df7f5f0e) --- updated-dependencies: - dependency-name: docker/setup-buildx-action dependency-version: 4.3.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: actions-minor-and-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>	2026-08-24 11:09:26 -04:00
xarmian	1649adb6c1	Merge pull request #1191 from PerpetualSoftware/feat/activity-live-episodes ... feat(web): Live view on the activity page — the feed folded into episodes (IDEA-2755)	2026-08-24 10:42:26 -04:00
xarmian	d4e7be4a24	feat(web): Live view on the activity page — the feed folded into episodes (IDEA-2755) ... An episode is a run of consecutive events by one actor on one item, split on a 30m gap: audit-grain rows become work-grain cards. The Live/Audit toggle persists per browser; server HTML and the hydration pass both render the 'live' default and the stored choice applies in onMount, strictly after hydration. Liveness is claimed only from event age. Live cards enrich with the newest comment's first line (best-effort, first four only, no polling) — the trail's checkpoint discipline is what makes that line worth showing. Seat identity: the fold reads metadata.agent (the X-Pad-Agent stamp); generic client ids render as 'agent', and a seat that sends its own name lights up its label with no further change — concept B's lanes want exactly that. Design canvas and decision record on IDEA-2755. Review loop: 4 rounds, 5 findings fixed (wire-contract phantom, agent metadata field, Node-25 localStorage guard, hydration mismatch, cross-type fixture bleed). Claude-Session: https://claude.ai/code/session_01HvAuiZ7JaWyCqqyV99LyWt	2026-08-24 14:17:35 +00:00
xarmian	ad0deacb43	fix(web): Activity's item_id was a phantom — the wire field is document_id ... internal/models/activity.go serializes the referenced item's UUID as document_id (the audit trail predates the document→item rename); the TS Activity type declared item_id, which no server response ever carries. Nothing read it until the episode fold tried to — its primary key never fired and ref-less rows would have folded into one workspace episode. The timeline test fixture carried the same phantom field, internally consistent with the type and unlike any real payload. Note the deliberate asymmetry: Comment's wire field IS item_id (models/comment.go) — the two types genuinely differ, which is exactly how the phantom survived review. Claude-Session: https://claude.ai/code/session_01HvAuiZ7JaWyCqqyV99LyWt	2026-08-24 14:17:35 +00:00
xarmian	5003718802	fix(push): apply delivery's visibility gate to delivered_sessions (BUG-2725) (#1187) ... deliveredSessionCount applied three of watchNotificationVisible's four gates, missing the first thing delivery checks: vis.allows(CollectionID, ItemID). Broadcast over-reported. Targeted was worse — the publish-skip reads this count, so the gate passed, the push went out, the stream dropped it on visibility, and the response said delivered_sessions: 1. An instruction lost behind a success. Per Dave's day-49 ruling, visibility is RE-RESOLVED at push time rather than snapshotted: membership and grants are revocable, so a value cached at connect goes wrong exactly when revocation is what makes it matter. The one input that cannot be re-resolved is the target connection's auth transport — computeWatchAccessVisibility consults isBearerAuth exactly once, inside the admin bypass, and the pushing request only knows its own. So SessionOrigin.BearerAuth is recorded at Add(). That is NOT the snapshot the ruling rejected: auth transport is a property of the connection, fixed when it opened and not revocable while held, so it cannot go stale. Armed is the precedent. SessionOrigin is kept separate from SessionIdentity because that type documents itself as self-declared and never verified; folding a server-derived security fact in there would silently retract the warning for one field. Both comments state the rule for future extenders: connection properties are admissible, derived authorization state never is. computeWatchAccessVisibility now takes a bool instead of an *http.Request, which makes the per-connection input visible in the signature and lets the count answer for a connection it is not serving. COST: "re-resolve per counted session" reads like N access checks per push. It is at most TWO, and sessionVisibility's memo makes that true by construction rather than by careful calling — every other input is per-user and identical across the sessions counted, so one varying boolean bounds the answers at two. Pinned by a test with 50 sessions. Codex round 1 (P1): the first version swallowed store errors into "not visible", reintroducing BUG-2698 through this fix — a targeted push reporting 0 SKIPS the publish, so a DB blip would drop the instruction and answer 200, in a function whose own doc comment says why 0 is load-bearing. Round 2 (P1): the same class one layer down — computeWatchAccessVisibility collapsed FOUR store failures into a denial, two discarded into underscores. Fixed as a class per CONVE-18. Resolution and policy are now separate: stream-side callers discard the error explicitly with reasons, only the counting caller propagates. Round 3 CLEAN. CONVE-23 sweep found three consumer-facing artifacts still describing the old mechanism, none on a line this diff touched: the plugin skill doc, the web push dialog, and pad push --help. All three corrected to name what actually remains rather than deleting the caveat. Plugin 0.3.1 -> 0.3.2, since installed plugins are version-pinned at install. NOT fixed, deliberately: the UNDER-count. A stream past maxSessionsPerUser receives broadcasts while never entering the registry. delivered_sessions remains an estimate with error in both directions, and every consumer-facing description now says so. Two coverage gaps recorded rather than rounded off: mutation M11 survives (the reporting test reaches only the first of four store calls, because closing the DB fails it first), and no test drives the whole chain store-fault-to-503 (the DB-close instrument kills the request earlier, so such a test would have gone green against the wrong 500 — deleted rather than relaxed). Also lands the BUG-2752 refutation sentinel: that item claimed the OAuth workspace allow-list went unenforced on /api/v1/events/stream. Refuted — no allow-list-bearing credential can authenticate to /api/v1/* at all. The test guards that format gate, so if it ever widens, the refutation's premise fails loudly instead of silently reopening a leak. Gates on the merged tip: make test 27 pkgs, make lint 0 issues, full Postgres suite 27 pkgs, govulncheck, codex CLEAN, CI 7/7. Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 09:45:48 -04:00
xarmian	72336aacb5	fix(events): release SSE admission slots when a client leaves mid-establishment (BUG-2749) (#1186) ... `GET /api/v1/events` reserved its admission slot, then blocked in `SubscribeAndReplaySince` while the workspace's Redis subscription was dialled and — since BUG-2747 — acknowledged. Nothing propagated the request's cancellation into that wait, so a client that disconnected during establishment left a process-wide slot, a per-principal slot and a per-workspace slot held for the whole of it. The connection was gone; the capacity was not. Cancellation is now DEREGISTRATION, and `wsCounts` — which already answers "is anyone still here" — decides everything downstream. No ownership hand-off and no reaper: the arbiter already existed. (One thing IS handed off, and only one — the remainder of the confirmation wait; see below.) The two cancellation positions take different paths, and only one of them owes the joiners anything: - Before the install: the existing post-dial critical section already abandons and retires correctly when nobody is left. It needed one ordering rule — the departed establisher stops being counted IN THAT SAME SECTION, before the count is read. If joiners registered while we dialled, the count is still non-zero and they get the subscription; that is the hand-off the filing asked about, expressed as a count rather than a transfer of ownership. - During the confirmation wait: the subscription is already installed with its receive loop running, so the connection is not at risk — but the WAIT is what releases the joiners, and dropping it would admit them into a subscription Redis has not acknowledged while telling them nothing. That is BUG-2747's defect re-created at the seam between the two designs. So the remainder of the wait moves to a goroutine that finishes exactly as the caller would have: same arms, same `markUnconfirmedAdmission` on the bound, same `finishPending`. Bounded by `confirmTimeout`; no reaper needed, because teardown stays count-driven. A departure is not a refusal. `ok bool` is replaced by a `SubscribeOutcome` enum across the three `EventBus` Subscribe methods, so `SubscribeWorkspaceLimit` and `SubscribeCancelled` cannot be collapsed: answering a departed client with 429 would have written a limit refusal into the logs and counters that anyone would use to tune that limit. An enum rather than a second bool or an error because the switch has to name the case — by construction rather than by argument. Caller population, with its search boundary: 3 production implementations (events.MemoryBus, events.RedisBus, metrics.InstrumentedBus), 1 test double (server.gapEventBus, which embeds the interface), 2 production call sites (both in handlers_events.go). Searched this repo four ways — the three method names, `.Subscribe(`, method declarations, and interface embedding. collab.OpBus and watchevents.Bus are different interfaces and are out of scope; no other repo links this package. WHAT THIS DOES NOT FIX, verified in go-redis v9.22.0 rather than inferred from its doc comment (which says Subscribe "does not wait on a response from Redis" and so reads as though no dial happens on the request path — it does; only the reply is unawaited). On plaintext, dialConn derives its per-attempt deadline from the caller's context and the default dialer is net.Dialer.DialContext, so cancellation aborts the dial. Under TLS the same dialer calls tls.DialWithDialer, which takes no context, so the dial stays bounded by DialTimeout alone. On a TLS deployment this shrinks the held slot from (dial + confirm bound) to (dial), not to zero. Review round 2 (codex) found a P1 in this unit's own first draft, of exactly the shape the filing warned about. A cancellation check at the top of the establish loop could return while the caller still OWNED an unretired establishment record: section 1 had already named it the establisher, so the record stayed in pendingSubs with nobody behind it, its done channel never closed. The next subscriber for that workspace would join it and wait forever — and its own registration keeps wsCounts non-zero, so no later caller would establish either. A permanently dead stream that looks alive, produced by a guard whose only purpose was to save a dial. The guard is gone: a cancelled caller now goes THROUGH establishSubscription, which is the only code that knows how to put the record down. Regression test included, and reinstating the guard turns it red. Round 2 also found a P2 shutdown regression: routing the dial to the caller's context alone took away Close()'s ability to interrupt a stalled dial, which it had before. The dial now runs on a context ended by EITHER the caller or the bus, and each half is pinned by its own test — dropping either one is detected. Review round 1 (codex): no P1. One nit fixed as a class — three comments elsewhere in the file asserted the dial was "NOT bounded by the context we pass", which this change falsified; the sweep found and corrected all three (establishSubscription, defaultSubscribeConfirmTimeout, Subscribe). The TLS half of its P2 is filed as BUG-2754: the fix belongs at client construction, where it covers every Redis call rather than this one. Class sweep filed separately as BUG-2751 (lead-ruled: one region, one design per diff): internal/watchevents has no per-request establishment, but its resume path blocks on a 250ms settle window bound to the bus's context rather than the request's, while /api/v1/events/stream holds the same admission slots across it. Tests: five cancellation cases in internal/events (before install, during the wait alone, during the wait with a joiner, a cancelled joiner, an already-dead caller), a dial-binding assertion, and the handler-level binding in internal/server asserting the admission slot itself is released — the half of the bug that does not live in the bus. Mutation matrix, 8 mutations: 7 detected, each by the test named for it. The one survivor is the ctx term in the retry re-decide, and it survives because it is an OPTIMISATION rather than a correctness guard — a departed caller that mints a second record still establishes, deregisters and retires correctly; the term only saves a pointless dial. The code says so rather than implying the guard is load-bearing. The earlier draft's entry guard and loop-top break formed a redundant pair the matrix could only detect when both were removed. That redundancy was the smell, and round 2 found the substance under it: one of the two was not redundant, it was wrong. With it gone the entry guard is detected on its own.	2026-08-24 08:24:34 -04:00
xarmian	692b3e1a84	fix(store): erase a deleted account's user id from frozen outbox payloads (TASK-2719) (#1185) ... * fix(store): erase a deleted account's user id from frozen outbox payloads DeleteAccountAtomic's de-identify posture reached only LIVE rows; outbox payloads froze user ids at emit time, so a deleted user's id stayed legible in undispatched and dispatched-retained rows — in workspaces they didn't own — until TASK-2714's retention window closed on the row. Dave's ruling on TASK-2719: 'delete my account' means prompt erasure, not a bounded window. ScrubOutboxUserRefsTx runs inside the deletion transaction: a key-scoped, value-matched recursive rewrite (assigned_user_id / user_id / uploaded_by, any depth — covers item_batch member nesting) plus a value-equality NULL of the subject_id column, whose only user-valued rows are member events. Scrub uniformly, delete nothing (lead ruling): erasure is this pass's job, row lifecycle stays with retention. A scrubbed member row degrades to a parseable resync signal — verified against the drain (opaque bytes) and memberEventPayload (absent user_id unmarshals to ""), so SPEC-3's tombstone branch is not needed. Population per CONVE-18: five payload families enumerated at outboxUserRefKeys; boundary stated (fields-blob interiors not entered, matching the live-row posture). scrubItemPII's emit-time keep of assigned_user_id is now SUPERSEDED at deletion time — both comments name the winner. Rewrite is Go, not SQL (dual-dialect: payload is JSONB on PG, TEXT on SQLite; the row-finding LIKE casts for the same reason). Read-fully-then- write on the one transaction (BUG-2409 shape). json.Number preserves numeric literals across the rewrite. TASK-2719 Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86 * fix(store): CAS the outbox scrub rewrite; pin key-scoping and number safety Codex round 1 on TASK-2719. The rewrite is now a compare-and-swap conditioned on the payload we read: two concurrent account deletions can hold one bulk payload (naming both users) on Postgres READ COMMITTED, and the later blind write would reintroduce the earlier deletion's id from its stale copy. Zero rows matched means re-read and redo against the fresh bytes; SQLite's single writer never takes the path; bounded loudly at 5. Documented rather than fixed, with the mechanics: the residual concurrent-emit window (FK KEY SHARE serializes every path that would CREATE a reference to the dying user; what survives is a re-freeze of an existing one, e.g. a title update on a still-assigned item, bounded by TASK-2714 retention — closing it needs a table lock on a once-per-account path), and the two prefilter invariants (newID() uuids carry no LIKE metacharacters and nothing JSON escapes; writeOutboxTx's json.Valid gate makes multi-value payloads unrepresentable). New tests pin what the existing set couldn't fail on: a decoy field whose VALUE is the deleted id under a non-target key survives (key-scoping), a 2^53+1 seq literal crosses the rewrite verbatim (json.Number), and the CAS retry leg is driven deterministically with a stale payload copy, asserting the stored bytes win and the stale copy's ids are not reintroduced. TASK-2719 Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86 * docs(store): state the outbox-scrub residual window honestly Codex round 2: the round-1 note overclaimed. On Postgres the escape is not just the re-freeze case — a KEY SHARE acquired before the deletion reaches DELETE FROM users means the DELETION waits and the emit commits first, and attachments.uploaded_by has no FK at all (migrations 047/026), so post- commit emits are not structurally clean either. All three paths stay retention-bounded; the comment now enumerates them instead of asserting cleanliness. TASK-2719 Claude-Session: https://claude.ai/code/session_01Cpr3teiHHsgcTmg2xhHA86	2026-08-24 00:22:17 -04:00
xarmian	308ed994b0	Merge pull request #1184 from PerpetualSoftware/fix/subscribe-confirmation-window ... fix(events): await Redis registration before admitting an SSE subscriber (BUG-2747, BUG-2748)	2026-08-23 22:50:35 -04:00
xarmian	cbf2dd29c8	test(events,metrics): assert exactly-once, cover the new counter, drop an overclaim ... Codex round 7. The confirmation bound's comment said it bounds establishment. It does not: the dial and go-redis's HELLO/AUTH handshake run inside client.Subscribe before the timer starts and are bounded by the CLIENT's DialTimeout instead, so the worst case composes to roughly DialTimeout plus this. Anyone reasoning about connect latency needs both numbers. The concurrency test asserted topology — subscriber count and pendingSubs — while reading one event per channel, so a duplicate from a second establishment could sit in the channel undetected. It now asserts exactly-once, which is the behaviour the topology was standing in for. The new Observer method, counter and deployment contract had no adapter test. Added, including the half that matters: the count must NOT also land on the reset series, since an adapter that merged them would pass a total-only assertion while destroying the distinction an operator acts on. And the abandonment test now says out loud that it fabricates its state, because the establishing caller is blocked inside Subscribe and nobody can unsubscribe it — the same reason the retry it exercises is defence in depth rather than a reachable path. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-24 02:12:59 +00:00
xarmian	4d84453f41	fix(events): apply the registration bound to the buffer, before the cursor filter ... Codex round 5. The P1 is the same two-spaces mistake the ID-valued ceiling made, committed again inside the fix for it: asking since() for the events above the cursor and then dropping the last (appends - mark) of them mixes a filtered list with an unfiltered count. A post-registration straggler whose id falls at or below the cursor is absent from the slice but still counted in the drop, so the count eats a legitimate pre-registration event instead. Pre-mark [5 30 20], post-mark [6 40], cursor 10 handed the caller [30] and lost 20. replayBuffer.sinceBounded applies the window to the BUFFER and shares every coverage rule with since(), which now delegates to it, so the two cannot disagree about what cannot-vouch means. This also closes the residual round 3 left accepted: if the wait's appends evict everything the buffer held at registration, keep goes to zero and the span is refused rather than partially served. Three overclaiming comments corrected — the wait is bounded, and saying Subscribe returns only once Redis has acknowledged is false on the timeout path. And the ceiling test's own claim: it appends straight to the buffer, so it pins the boundary arithmetic and says nothing about replay-XOR-channel, which is a different test. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-24 02:04:45 +00:00
xarmian	f5ca67cbca	perf(events): shorten the subscribe-confirmation bound to a measured 1s ... Lead ruling on codex round 3's second finding: file the ctx-plumbing as its own unit (BUG-2749), bound the exposure here. The bound is not a guess at how fast Redis is. Establishment either completes in single-digit milliseconds or does not complete at all, so past the top of the fast mode waiting longer buys nothing and only holds an SSE admission slot, global and per-workspace, for a client that may already be gone. Measured on a containerised Redis over loopback, 300 establishments, timing the whole of Subscribe: p50 388us / p99 679us / max 1.73ms idle; p50 693us / p90 5.1ms / p99 12.1ms / max 18.5ms under 24 busy loops on 8 cores. One second is ~54x the loaded maximum. Being too short costs an admission whose coverage this instance cannot describe — counted, logged, and reconciled to the client when the acknowledgement lands. Waiting is the silent direction. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-24 01:46:15 +00:00
xarmian	650d9df270	fix(events): bound the replay by append POSITION and buffer identity ... Codex round 3. Both P1s real. The ceiling used lastAppendedID as if it were a time boundary. This bus's ids come from a counter shared across workspaces and a phase-1 publish assigns and publishes in two calls, so arrival order and numeric order genuinely disagree. Against an id-valued bound both directions break at once: a straggler arriving after registration is replayed although it also went to the caller's channel, and a pre-registration event carrying a higher id is filtered out and never replayed at all. replayBuffer now counts its appends, and the bound is a position — the entries to withhold are simply the final (appends - mark) of whatever since() returned, which may trim from the front but never the back. The mark also carries the BUFFER, not just a position in it. An ID-space reset during the wait replaces the buffer wholesale; a position in the old one describes nothing in the new one, and knownFrom may still accept an adjacent cursor, so the mismatch does not announce itself. Also corrected, all found by the same round and all mine: the Observer comment claimed this counter never reaches SequenceReset, which the late-confirmation path contradicts; the reason enumeration in metrics.go, its Help string and docs/deployment.md were never updated for the sixth reason; and both the metric and its comment said every increment is a client when it is one establishment however many subscribers were waiting. Accepted, not fixed: since() evaluates eviction over the whole buffer including post-registration appends, so a flood inside the wait can evict a cursor that missed nothing and force a sync_required. It costs a spurious resync, never silent loss, which is the direction this family chooses every time. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-24 01:38:46 +00:00
xarmian	cadf0fab33	docs(events): name the joiner retry as defence in depth, and make it loud ... The mutation matrix could not reach it: no mutation of the surrounding code makes a test take that path, because the same-lock retire in the abandon path makes the strand unreachable rather than recoverable. A joiner increments wsCounts under b.mu before the establisher's count check reads it, so a registered joiner prevents the abandon; a joiner arriving after the check cannot find the record, because it is gone in that same section. That is an argument, not a measurement, so the retry stays — a permanently dead stream that looks alive is worth one wasted pass in a case that should never happen — but it now logs when it fires, so a wrong argument surfaces in production instead of limping silently. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-24 01:22:06 +00:00
xarmian	1f4ab9b549	fix(events): abandon and teardown must not strand a joiner or leak a PubSub ... Codex round 2, lifecycle angle. Both findings real, plus one my own fix introduced. P1 — Close cancels the context before it takes the lock and drains wsSubs, so an establishment that locked afterwards installed into a map Close had already emptied. Its receive loop exits on the cancelled context and neither subCancel nor pubsub.Close ever runs: the PubSub and its health-check goroutine outlive the bus. establishSubscription now refuses to install into a closing bus, and Close clears wsCounts alongside the subscribers it counts, so the two structures cannot disagree. P1 — abandoning because the workspace emptied retired the establishment record in a separate critical section from the decision. A subscriber arriving in between registered, waited on a promise nobody would keep, and returned with a channel wired to nothing — permanently, since its own registration keeps wsCounts non-zero so no later caller establishes either. The record is now retired under the same lock as the decision, and a joiner verifies a live subscription afterwards rather than assuming one, taking the establishment over once if there is none. And one I introduced writing that: the re-check created a pending record on the loop's final pass with nobody left to establish behind it, which is a worse version of the same defect. Records are now created only at the top of an iteration that will use them. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-24 01:20:20 +00:00
xarmian	c719bf44b1	test(events): make the confirm-versus-timer race deterministic ... The repetition version caught the mutation that removes the confirmClosed re-check in 0 of 10 runs at 500 establishments each: a near-zero bound makes the timer win outright far more often than it ties, and winning outright is the ordinary timeout path, not the race. A one-in-ten detector reads as coverage and is not. beforeUnconfirmedMark holds the mark until the acknowledgement has landed, reproducing the interleave every time. 10 of 10 against the same mutation. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-24 01:09:16 +00:00
xarmian	703e746922	fix(events): bound the replay at registration instead of withholding fan-out ... Codex round 1, three findings, all real. P1 — a subscriber arriving mid-establishment was admitted immediately. establishSubscription installs wsSubs and only THEN waits for the acknowledgement, so for that interval the workspace looks live and is not. Reading wsSubs first let a second subscriber straight into the unconfirmed window this change exists to close. pendingSubs is now checked first. P1 — withholding fan-out from a not-yet-admitted subscriber dropped events for the very population BUG-2747 is about. A fresh subscriber (sinceID == 0) reads no replay at all, so an event skipped on the theory that the replay would carry it was skipped and then never replayed. Replaced with a replay CEILING captured at registration: the subscriber is live in fan-out from the moment it registers and receives everything after that on its channel, while its replay is bounded above by what the buffer held then. That is the same division the single critical section gave for free, generalised to the case where a wait separates the two halves. P2 — the confirm timer could set unconfirmedAdmitted after the acknowledgement had already cleared it, leaving a subscriber counted as unconfirmed and never told to reconcile. markUnconfirmedAdmission now checks confirmClosed under the lock that closes it. Two tests added for the two P1s. The double-delivery test moved from the establishing caller to a JOINER, because the establisher can never exercise it: establishing implies no live subscription, losing the last subscriber deletes the buffer, so its replay is always nil and there is nothing to duplicate. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-24 01:06:52 +00:00
xarmian	739f573b3b	docs(events): the namespace helper's comment described a window this fix closed ... It said the wait was covering up a production window with no remedy, pointing at BUG-2747 as where it was tracked. That is now the fixed thing, so the comment was teaching the next reader something false about the bus they were looking at. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X	2026-08-24 00:44:54 +00:00