mirror of
https://github.com/PerpetualSoftware/pad.git
synced 2026-09-23 02:53:31 +00:00
f3aa86503a36d3661005954cc9eefcb3edda5cd1
435 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 xarmian
|
f3aa86503a |
fix(events): assign the in-memory id under the lock that orders the buffer (BUG-2736)
Codex round 18, pointed away from the Redis bus that seventeen rounds had concentrated on. Three findings; one belonged in this unit, one is filed, one is documented. THE ID WAS ASSIGNED BEFORE THE REPLAY LOCK, so two concurrent publishes could take N and N+1 and append in the other order. replayBuffer.since computes oldest and newest by POSITION, so a buffer holding [N+1, N] reports N as its newest and answers a resume from N+1 with sync_required — a client told to resync at the moment it was exactly current. Pre-existing in shape, but it is the same invariant this unit buys for the Redis bus with an atomic publish script, on the same buffer, for the same reason: publish order must equal id order because the buffer's own ordering assumptions are otherwise false. Fixing one and leaving the other would be half an invariant. The test drives 300 concurrent publishes and asserts both the order and its consequence — that every id in the buffer is servable as a cursor, which under the race the newest ones were not. Verified to fail 5 of 5 with the assignment moved back out. FILED, NOT FOLDED IN: BUG-2737. Neither activity bus refuses a subscription after Close, so a handler that subscribes during shutdown holds a channel nobody will close and blocks for the full 30s deadline. It is a shutdown-lifecycle defect rather than an id-space one, it spans both implementations, and internal/watchevents already fixed the identical thing in BUG-2651 — so the fix is porting a decided question, not answering one. DOCUMENTED: the SSE data body carries the event id as a JSON NUMBER, which is now around 1.8e18 and past JavaScript's MAX_SAFE_INTEGER. It cannot be removed — the Redis bus's phase-1 wire form carries the id there and nowhere else — so the frame writer now says that the "id:" field is the one a client may use, and why web's ItemEvent deliberately declares no id. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
d393126d80 |
test(events): close the gaps a tests-as-production-code pass found (BUG-2736)
Codex round 4, on the tests themselves. Eight findings, all real; three of them were behaviours in this diff with no test at all. NO TEST AT ALL: - The real receive path. Every reconciliation test drove fanOutFromRedis directly and the publish tests read the wire with a raw subscriber, so a regression that decoded the epoch correctly and then handed 0 to the fan-out would have passed all of them -- reconciliation silently never running in production. Now driven through Subscribe/Publish and back through Redis, with the mutation checked. - The atomic script's ordering claim. Every phase-2 test published once or ran the script sequentially, so a two-call INCR-then-PUBLISH implementation passed them all -- and that ordering is load-bearing, because the receive path reads a descending id as a counter reset. 300 concurrent publishes now assert arrival order equals id order; verified to FAIL 5 of 5 against a two-call implementation and pass 3 of 3 against the script, so the instrument discriminates rather than merely being green. - The TOML tag. The env-var test proved PAD_EVENTS_PUBLISH_EPOCH reaches the field and said nothing about the toml:"events_publish_epoch" tag -- the exact form the rollback procedure warns about, since a file value outlives an unset env var. PASSING FOR THE WRONG REASON: - The production config wiring was still unexecuted: passing an empty config.Config at both RunE call sites compiled and passed everything. The source-text guard that already counts those call sites now also requires them to pass the loaded config. - The phase-2 wire assertions accepted a well-formed payload with an empty event body. They now assert the body survives. - The Redis metrics subtest had no served-resume control, so a bus that refused every resume would have passed. It now round-trips a publish through Redis first. - TestResumeGapIsReportedForBothWaysOfNotServing never proved ws-warm HAD a buffer, so its second half could silently duplicate its first. - internal/server's cold-resume tests still sent a literal 4200, which the incarnation guard now answers before the handler's no-buffer path is reached. My own round-1 sweep of this class stopped at four packages and never looked at internal/server: reviewer-named instances are a sample (team CONVE-18), and so, evidently, are self-named ones. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
86b0f7508f |
docs(server): name the subscribe-then-replay window where it lives (BUG-2730)
Codex round 18 was asked to assume exactly one defect survived sixteen rounds and to find it rather than survey. What it returned is the subscribe-then-replay duplicate window — a REAL defect, and one already filed on BUG-2730 by two earlier rounds. That it went that deep and surfaced a known residual rather than a new defect is the useful result. But the code said nothing at the site, so a successor reading handleSSE would have to re-derive it, exactly as three review rounds did. Now stated where it happens: the window, what it costs (a duplicate toast and duplicate work, never a lost event), how internal/watchevents closed the same window with SubscribeAndReplaySince, and why closing it here is its own unit — a new method on events.EventBus across three implementations, folded together with the admission check SubscribeIfAllowed already performs. That is a change about DELIVERY, and this one is about COVERAGE. No behaviour change. Refs BUG-2730, BUG-2731 |
||
|
xarmian
|
f2a037e393 |
docs: nine claims about other people's code that I had not checked (BUG-2731)
Codex round 16, aimed at every factual assertion this diff makes about
code OUTSIDE it — go-redis, the SSE spec, HTTP header handling, the web
client, internal/watchevents, Prometheus. The angle was chosen because
this diff had already been caught twice asserting library behaviour that
was false, and claims about other people's code are the one class no test
in this repo can falsify.
It found nine. Every one is mine, and every one claimed more than I had
verified.
- "no reconnect in 24 seconds of probing" cited an experiment that is
not in the tree — the probe was deleted with the test it belonged to.
The MECHANISM is checkable from the library source and now says so
with the call named; the unretained number is gone.
- "the SSE `id:` field has no room for an ID-space identity" is wrong.
The spec allows an arbitrary UTF-8 event ID. What excludes it is PAD's
own contract — an int64 every deployed client already parses — which
is a stronger and more honest statement of the constraint, and it is
the one BUG-2736 has to argue against.
- "the spec defines an empty header as no position" overstated it. The
spec governs what a client SENDS. What a server does with a value it
cannot use is our policy, and the test now says so.
- "HTTP strips optional whitespace from header values" is too broad: Go
trims on the way OUT, while the incoming MIME parser only TrimLefts.
What I measured was the round trip, and the comment now claims exactly
that.
- "every gap is a full resync / full re-fetch" is wrong in three places.
The web client answers sync_required with an incremental /changes
delta and only falls back to a full refresh after a long absence or a
failure. This one matters beyond wording: the load argument for the
whole fix rests on what a gap costs a client.
- "a wrapper cannot see that a resume gap occurred" — it can see the nil;
what it cannot see is WHY. I had already corrected this in the metrics
adapter and left the overbroad version in the seam it describes.
- internal/watchevents' `since` no longer "mirrors internal/events
exactly" — that stopped being true when knownFrom went into the
latter's `since`. Now states where the two differ and why.
- "the counter returns to baseline" — a Prometheus counter only
increases; its RATE returns to baseline. Two places.
- "the only case where INCR fails while PUBLISH still reaches
subscribers" — an ACL permitting one and denying the other is another.
The test now names the SHAPE as what matters and its arrangement as
one route to it.
No behaviour changes; comments, docs and test prose only.
Separately verified while waiting on this round, and now cited rather than
asserted: the three WHATWG steps that make the empty `id:` cursor
retirement work. That claim was the one thing in the diff I had taken from
memory of a spec rather than read, and it is load-bearing — if wrong, the
feature is theatre.
Refs BUG-2731
|
||
|
xarmian
|
5cbeb52784 |
docs(events): three claims the split left describing code that is gone (BUG-2731)
Codex round 14, a post-split damage review. Two findings, both stale
prose, which is the failure mode a SUBTRACTION has: the code is correct and
the comments describe the version that was removed.
- fanOut still credited "publishScript" for the ID. The activity bus does
INCR then PUBLISH again; the script went to the migration.
- replayBuffer.knownFrom and handleSSE both listed an ID-space reset among
the things that invalidate coverage. That detector went with the
migration, so a reset Redis counter can still leave two incarnations'
IDs in one buffer.
The second matters more than a wording slip: it claimed a guarantee the
remaining code does not provide, which is the shape that gets a successor
to trust a boundary that is not there. Both now NAME the omission and point
at BUG-2736 instead of implying coverage they do not have — the same
boundary-declaration the knownFrom comment already carries.
Codex's verdict on the rest, worth recording because it is what the gate
was for: "the replay-coverage logic and corrected tests are otherwise
coherent on their own."
Refs BUG-2731, BUG-2736
|
||
|
xarmian
|
9f88e94832 |
fix(events): a resume must not be answered from coverage we never had (BUG-2731)
internal/events answered a Last-Event-ID resume with an empty-but-non-nil
slice whenever the workspace's replay buffer could not speak to the span
being asked about. The SSE handler reads that as "caught up", so the client
sat on a live stream believing it was current while everything between its
cursor and now was silently gone.
COVERAGE. replayBuffer gains knownFrom: the lowest event ID from which this
instance's coverage of a workspace can be vouched for. A resume from below
it answers nil, which the handler already turns into sync_required. Covers
a buffer that does not exist (cold start, restart, scale-up, or simply the
first connection to a workspace on this instance), a buffer that exists but
starts above the cursor — NOT full and NOT empty, reachable on any
multi-instance deployment with no eviction and no restart — and a non-zero
cursor from a previous incarnation of a single process.
knownFrom here means RECEIVING-continuity, never ID-contiguity, and the
defining comment says so with the measurement attached.
internal/watchevents has a field of the same name that ALSO detects holes
by noticing a non-consecutive ID; porting that would have been a serious
regression, because this bus has a global counter and per-workspace
buffers, so a workspace's buffer holds non-consecutive IDs by construction
(four publishes alternating across two workspaces measure as W=[1 4],
X=[2 3]). An ID-contiguity check would fire on nearly every append and turn
every resume into sync_required — the false-positive inversion of this bug.
LIFECYCLE. Coverage now ends where it really ends:
- a stopped workspace subscription drops its replay buffer. Keeping it
"in case they come back" looks like a free win and is the bug: events
published elsewhere never enter it while it goes on looking complete.
- subscriptions are generation-numbered, so a straggler from an ended
subscription cannot re-create a buffer and vouch for coverage that
ended with it — including the case where the workspace has already been
resubscribed under the stale goroutine.
- a pub/sub reconnect ends that workspace's coverage. PubSub.Channel
resubscribes transparently, so a Redis failover left a hole the buffer
had no idea about; the loop reads pubsub.Receive instead. It must
RECOVER rather than exit — returning on a transient error would leave
an instance publishing fine and receiving nothing — and it drops ONE
workspace's buffer, since a dropped subscription says nothing about any
other channel.
Subscribers are indexed by workspace because the replay buffers moved under
the same mutex (necessary for the straggler race): scanning every local
subscriber under that lock would make one hot workspace the serialization
point for every other workspace's fan-out and every resume.
Also removes Publish's local-counter fallback on a failed INCR, which
minted an ID from a process-local space and published it — every receiving
instance reads that as the counter having been reset. It bought nothing:
this bus has no local fan-out path, so an event that does not reach Redis
reaches no subscriber here either.
SIBLING. internal/watchevents had the identical cold-resume defect on its
MemoryBus — its RedisBus guards it, MemoryBus reached the buffer directly —
so a single-process instance answered a post-restart resume as caught up.
Found by a cross-artifact review pass; the guard goes in `since` so both
implementations inherit it, and is tested through SubscribeAndReplaySince
as well as EventsSince because that is the path the handler uses.
Refs BUG-2731
|
||
|
xarmian
|
bb003dd6bb |
fix: five claims the final comment-truth round found (BUG-2724, BUG-2726)
The bounded process the lead set: N rounds, an author prune pass, one final comment-truth round. This is that round's output, and the loop stops here. Two were mechanisms I had wrong, and both are the kind a reader would reuse without re-deriving: - "Different Redis DB numbers do not help" was half true. Ordinary keys ARE DB-scoped, so two installations on different DBs keep separate presence registries; it is pub/sub that ignores DBs entirely, which is why the buses cross-feed regardless. Stating it as "does not help" made the namespace look like the only fix for a problem it only half is. - A namespace cutover's client resync was attributed to the epoch check. That check needs an OLD epoch to compare against and a freshly namespaced bus has none — the resync comes from the cold replay-buffer coverage check instead (knownFrom is zero, so every resume falls below it). Same honest outcome, different mechanism, and the mechanism is what someone reasoning about a cutover would use. Three were stale or over-general after earlier changes: the admission comment still said the global limit is passed to the bus as 0 (that parameter is gone), `pad watch --help` and the plugin monitor description lumped a missing .pad.toml's hourly retry in with the 5s-to-5min backoff, and CLAUDE.md said clients must back off without the browser exception docs/deployment.md spells out. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
461c5a3e3d |
refactor: prune the claim surface, and turn one prose claim into a test
The review could not converge on this diff's comments because each round of corrections re-expanded the surface it was reviewing — rounds 16 and 17 found errors inside 15 and 16's fixes. That is a production rate being measured, not a backlog being drained, so the treatment is to write fewer claims rather than review the same ones again. PRUNED, ~135 comment lines: process narration. "An earlier version said X", "found by mutation testing", "codex round N caught this", the scoreboards. Every one of those is already in a commit message, which is where the archaeology belongs; in the source they are claims a future reader has to verify, about a past that no longer exists. KEPT, because they earn it and a reader would otherwise re-derive them: metric semantics, reachability boundaries, what a test does and does not discriminate, why the obvious alternative was rejected, and the hazards that cannot be enforced in code. MOVED TO A TEST, per the rule this run earned the hard way: a comment asserting countable behaviour belongs in the suite. Two test comments in internal/watchevents relied on "this constructor waits for its SUBSCRIBE to be confirmed" — prose, and the same assumption applied to the OTHER bus (which subscribes asynchronously) is what made a namespace test flake. It is now asserted with no polling and no sleep, and the mutation that removes the wait fails it. That rule generalises and is why round 17's find mattered: "counts every unservable resume" was prose, so its falseness could hide a real metric gap. Prose is for claims that cannot be asserted. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
35e564298b |
fix: seven more prose claims, one real metric gap, and a flaky test of my own (codex round 17)
The prose angle again, and it is still finding things — which is itself the finding: this diff's comment density is generating wrong beliefs faster than the review is removing them, in the one dimension where the defect is a reader's understanding rather than the program's behaviour. Everything below was a claim I wrote. ONE WAS A REAL GAP, not just wording. pad_watchevents_resume_gaps_total was documented as counting every unservable resume, and counted only the half decided by the shared counter. The LOCAL half — a cursor below what this instance can vouch for, from a hole or a cold start — returns nil from replaySince, becomes sync_required for the client, and reported nothing. Now counted, on the deferred path so it fires with the lock released. Its test needed a second pass to be an instrument: the first version arranged a hole and asserted the counter moved, but the shared counter disagreed too, so resumeOutrunsLocalView reported and the mutation survived. It now sets the counter to AGREE with what the instance has seen, which is the only arrangement that isolates the local path. The prose corrections, swept by grep rather than by instance this time: - MemoryBus's comment said a single-process deployment never wires an observer. cmd_server wires one, deliberately — that is what makes the drop counter meaningful there, which is a claim I had just added elsewhere. - "Every write path works with Redis down" was too strong in three places. Push answers 503 for an unresolvable targeted push and 502 push_unconfirmed on publish failure — the paths whose job IS cross-instance delivery. - Presence-failure consequences were stated as certainties in four more places after round 16 fixed one. A failure means an error was REPORTED; Redis can fail a pipeline after applying it. - The deployment metrics table still described pad_eventbus_publish_total as "Events published" after the Help string had been corrected to attempts. - The reserved-namespace rationale called prefix nesting a "collision". It is nesting; an exact collision would need the namespace to match a workspace UUID. Refused anyway, and now for the reason that is true. - A presence cutover was described as stranding one renewal interval of stale entries. It is the full 90s TTL — three intervals. AND A FLAKE OF MY OWN, caught by the full suite rather than by the targeted runs: the activity-bus namespace test asserted subscription state immediately, but that bus subscribes ASYNCHRONOUSLY (the watch bus waits for confirmation; the two differ). It now polls, and the asymmetry is named in both tests so the next reader does not assume symmetry the way I did. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
7c8ed3c815 |
fix: nine false or overstated claims in this diff's own prose (codex round 15)
An angle worth naming, because it found more than several code-shaped
ones did: check the COMMENTS against the CODE. This diff is
comment-heavy and its comments make specific factual claims. Nine were
wrong.
The one that mattered most was a false argument for a correct rule.
redisns.Parse rejects colons, and justified it with a collision example
that does not happen: ns "a:events" builds pad:a:events:events:<ws>, not
pad:a:events:<ws>, because the suffix is appended too. The rule stands on
its own grounds (a colon spans segments and makes the keyspace ambiguous
to read back) — but a false example is worse than none, because the next
reader trusts it.
Chasing that turned up a REAL collision needing no colon: a namespace
equal to one of Pad's own first segments nests this installation inside
the default one's keyspace. Namespace "events" puts every key under
pad:events:*, which is the default installation's activity channel space
— the exact cross-feed the namespace exists to prevent, arriving through
the namespace. Now rejected, with a control leg asserting that names
merely CONTAINING a reserved word ("events-eu", "prod-session") stay
valid.
The other eight:
- "The three keyspaces cannot drift" — overstated. Each constructor takes
its own Keys; a source-reading test is what enforces it, which is
weaker than a compiler and now says so.
- Two docs claimed both SSE endpoints incur a presence registration. Only
the watch stream registers.
- The Redis metrics section said they "stay at zero" without Redis, while
pad_redis_up is deliberately unregistered — the section contradicted
the field three lines below it.
- The presence-failure metric's HELP string still carried the blanket
"leaves sessions unlisted and untargetable" that the field comment had
already been corrected away from. Two of the four ops fail in the
opposite direction.
- A nil from MGET was described as proof the process died. Eviction, a
restart and a manual DEL produce the same nil, and this file's own doc
says eviction is indistinguishable from expiry.
- A test comment claimed to cover both corrupt-entry shapes; the second
is unreachable and the subtest is deliberately absent, as the note ten
lines down already said.
- "Enumerates every refusal path" covered per-instance and per-workspace
and not per-user — the same undercount as round 13's, one round later.
Both per-user paths added.
- The Observer contract said a go-redis drop is reported as a sequence
gap. Only if a LATER notification arrives to expose the hole: drop the
newest message on a bus that then goes quiet and nothing is reported.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
|
||
|
xarmian
|
9d54f24626 |
fix(server,cli): the half of round 12's fix I missed (BUG-2726)
Codex round 13, unanchored, found that my previous commit fixed one of the two refusal paths on /api/v1/events. The admission check moved above the SSE headers; the PER-WORKSPACE check stayed below them, so half the 429s on that endpoint still carried the JSON error envelope under Content-Type: text/event-stream — the exact defect the commit said it fixed. Team CONVE-18 in its own shape: the reviewer named one instance, I fixed that instance, and the class had two members. The enumeration I owed was "how many ways can this handler refuse", and it takes ten seconds to read. Every refusal is now above the header block, with a line saying nothing below it refuses. The contract test made the same omission and is the reason this reached another round: it drove the admission bound on both endpoints and never the per-workspace one, so it agreed with a handler that was half fixed. It now enumerates all three refusal paths, and the mutation that reintroduces the defect fails it by name. Also from round 13: `pad project watch`'s 429 message named the two knobs that cover both streams and omitted PAD_SSE_MAX_PER_WORKSPACE, which is the one most likely to be the cause on a busy workspace — true as far as it went, and pointing the reader away from the answer. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
3e3170e915 |
fix(server,cli,docs): the consumer contract, per codex round 12 (BUG-2726)
An angle no earlier round took: what does a CLIENT see. Two of the five
findings were about consumers I had never opened.
- `pad project watch` returned "event stream returned 429: {json}" and
exited, which sends the reader looking for a bug rather than at a
limit. It now says what happened and which knobs govern it, and names
the fact that those knobs cover this stream and the agent watch stream
together. It still exits rather than backing off — it is interactive,
and a human can decide — unlike the unattended monitor, which already
folds 429 into its ladder.
- Both endpoints now answer a refusal through one helper: same status,
same code, same message, plus `Retry-After`. `/api/v1/events` was
setting `Content-Type: text/event-stream` BEFORE the admission check,
so its 429 carried the JSON error envelope under an SSE content type —
a different contract from its sibling's for the same refusal. Admission
moved above the headers, which is where it belonged anyway.
- The anonymous-caller rule was documented as if it applied to both
endpoints. It applies to `/api/v1/events` only; the watch stream
requires a resolved user and answers 401 without one.
- docs/architecture.md described one SSE endpoint and one bus. It now has
the table: two streams, two buses, different scopes and consumers, one
shared connection budget, one Redis namespace.
FILED, not fixed: the web UI's `EventSource` cannot see a 429 or a
`Retry-After` — the spec exposes neither to the page — so a refused
browser tab reconnects at a constant rate while the CLI backs off. That
asymmetry means reaching the limit sheds load from the population that
respects it and not from the one that grows fastest under it. No
server-side change closes it; the fix is a client-side reconnect wrapper.
BUG-2733, and docs/deployment.md warns operators to size the limit with
it in mind.
Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X
|
||
|
xarmian
|
a167005654 |
fix(server): register the stream gauge per metrics instance, and drop the comments the last refactor falsified (BUG-2726)
Codex round 11, reviewing the collector conversion: - The registration guard was a sync.Once per Server, which is wrong in the direction that hides: SetMetrics can install a DIFFERENT registry, and the second one would silently never get pad_stream_connections_active. An absent metric is worse than a wrong one, because a dashboard with no data reads as a deployment with no traffic. Now tracked per metrics instance, which still cannot double-register (MustRegister panics) and follows a replacement. - Two comments and a test comment still explained the deleted observer machinery — "the discarded gate keeps its observer, so its releases keep driving the gauge" — which stopped being true when that machinery was removed one commit earlier. The reasoning they were making still holds for a different reason (a replaced gate is still reachable through Server.admission(), so the scrape reads the new gate and looks plausible), so they say that instead. And heldTotal's doc still opened with the old name. Falsifying your own comments while fixing something else is the failure this diff has hit repeatedly; catching it one round later is the loop working, not the comments being unimportant. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
a51320c861 |
refactor(metrics,server): make pad_stream_connections_active a scrape-time collector (BUG-2726)
Asking the round-21 question of my own work, because the signal was there: rounds 9 and 10 each found a defect inside this one mechanism, and my own record says several consecutive rounds finding defects inside one addition is a signal about the ADDITION, not a run of bad luck. The pushed gauge fired a callback on every admit and release. Round 9 found those callbacks running under the gate's lock — a deadlock for any callback that touched the gate. Round 10 found the fix reorderable: one admission could capture 1, a concurrent one capture 2, and the stale 1 land last, leaving the gauge permanently BELOW the real total, which reads to an operator as spare capacity that is not there. Each fix was correct. The mechanism was the problem. A GaugeFunc reading the total on scrape has neither failure mode and deletes both fixes with it: the observer field, the notify mutex, the snapshot closure, and the deferred-notify plumbing in acquire and release. The repo already had the pattern — RegisterDBCollector — and the argument that ruled it out for pad_redis_up does not apply here: reading an in-memory counter has no I/O, no side effects, and cannot make the value depend on who is asking, which is exactly what a Redis PING on scrape would have done. The tests got simpler and stronger in the same move. The concurrent one no longer needs a deliberately widened race window to be an instrument — that version's first attempt survived five runs against a broken build, which is the shape of a test that cannot fail — and it now scrapes WHILE the churn runs rather than only at rest. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
7e20e29754 |
fix(server): two defects codex round 10 found INSIDE round 9's fixes (BUG-2726, BUG-2727)
Aiming a round at the previous round's fixes paid, which is the second time this week that has been the sharpest angle available. 1. The lifecycle fix guarded the FIELDS and not the OPERATION. Stop cleared started, released the lock, and only then cancelled and waited — so a Start racing into that window installed a fresh loop the in-flight Stop neither cancelled nor could end, while both shared one WaitGroup. Start and Stop are now serialized end to end by a dedicated lifecycleMu, deliberately not the state mutex, because probe() takes that one on the goroutine Stop waits for. 2. The gauge fix made the callback reorderable. It captured the total under the lock and fired outside it, so one admission could capture 1, a concurrent one capture 2, and the stale 1 land last — leaving pad_stream_connections_active permanently BELOW the real total, which reads to an operator as spare capacity that is not there. The callback now reads the total at notify time under a notify mutex, so whichever fires last also read last and the gauge converges. Both tests are concurrent, because the sequential ones structurally could not see either defect — round 10 said so and it was right. The gauge test needed a second pass to become an instrument. The first version SURVIVED five runs against the broken build: the reorder window is real but too narrow to hit reliably, so the test could not fail and would have shipped as coverage that proves nothing. It now widens the window deliberately with a sleep inside the observer, and the broken build fails it by name (gauge = 9 against a true total of 8). The lifecycle test catches its mutation as a data race plus a failure. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
640c5a02a1 |
fix(watchevents,server): four concurrency findings from codex round 9 (BUG-2727, BUG-2726)
None of these can bite the shipped wiring today. All four are hazards on EXPORTED seams whose safety depended on a caller reading a comment, which is a deadlock with a delay on it. 1. Observer callbacks fired while the bus held its mutex, on both implementations. An observer that subscribed or published would deadlock the receive loop. Reports are now collected inside the locked section and flushed after it, via a defer registered before the unlock, so re-entrancy is a property of the BUS rather than a rule for implementers. The new test drives an observer that calls back into the bus and bounds itself, because the failure is a hang rather than a wrong value. Writing that test found a second hazard the fix cannot remove: an observer must not call a bus method that itself REPORTS (SubscribeAndReplaySince can raise a resume gap), because that is unbounded mutual recursion. Found by the test hanging when it tried exactly that. Now named in the contract. 2. RedisHealth's lifecycle raced: cancel was written by Start and read by Stop with no synchronisation, a repeated Start leaked the first loop and left Stop waiting forever, and Stop was unsafe on a prober that never started. All three closed, and the onProbe contract now says it must not call Stop — that waits on the goroutine it runs on. 3. The admission gauge callback ran under the gate's lock. Same fix, same reason. 4. SetSSELimits' doc oversold what it supports. It also writes plain Server fields that handlers read, so a live call races regardless of how careful the gate is. It is config-time; the in-place gate update exists so a late call does not silently over-grant capacity, not as a live-reconfiguration feature. Mutation-verified, and the first attempt at that matrix was wrong in a way worth recording: it moved the flush defer to a position that STILL ran after the unlock (defers are LIFO), so both mutations "survived" and looked like weak tests. The corrected mutation deadlocks the package binary, which is the shape the fix prevents. Also added the missing lifecycle test — Start's idempotency had no instrument at all, which is why that mutation survived honestly. Race detector clean: go test -race across internal/events, internal/watchevents, internal/metrics, internal/redisns, and the new internal/server suites. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
44efef5970 |
refactor(events): remove the dead global-limit parameter from SubscribeIfAllowed (BUG-2726)
BUG-2726 moved the global streaming bound to internal/server's streamAdmission, which both SSE endpoints acquire from before subscribing. Since then the handler passed maxGlobal=0 and the bus's own global branch was unreachable from any shipped path — a policy knob that looked live and was not. Removed rather than left behind, on the lead's ruling and for the reason the round-21 registry-cap removal gave one layer up: dead policy surface is scope, not cleanup. Somebody eventually configures a knob that looks live, and its silence gets diagnosed as a bug. The successor is streamAdmission (internal/server/stream_admission.go), named in the interface doc so the archaeology is one hop: a global bound is a property of the PROCESS, and Pad serves two SSE endpoints over two different buses, so neither bus could enforce it alone — one counting its own subscribers would let a caller exhaust the machine through the other while every configured limit still read as satisfied. The per-workspace bound stays on the bus. It is genuinely workspace-scoped, the other endpoint has no workspace to count against, and it keeps the package-level test added in round 5 — which was the first instrument it ever had. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
ec70f13608 |
refactor: act on codex round 8's scope review (BUG-2727)
The "should this be here at all" angle, which my own record says I do not reliably ask of my own work. Six findings; one was a real inconsistency, the rest were claims that needed stating rather than code that needed removing. REMOVED: the presence observer's interface, adapter type and constructor, in favour of a plain callback. One method, one production consumer — and the same diff already uses bare callbacks for RedisHealth and the stream gauge, so this was inconsistent with itself. internal/watchevents keeps an interface because it reports five distinct conditions; one does not earn one. TRIMMED: .env.example's per-variable prose down to the upgrade-relevant facts plus a pointer at docs/deployment.md, which is canonical. The same policy was restated in seven artifacts and that is a drift surface. KEPT, with the reason written where a reader will ask: - The receive-loop-exit counter is expected to stay at zero, and that is what it is for — a should-never-fire alarm on a state undetectable from outside the process (an instance that publishes fine, answers health checks and receives nothing). BUG-2727 filed the silent return as the defect, and a log line nobody greps is not the same artifact as a counter somebody alerts on. - The prober's synchronous first probe duplicates cmd_server's dial-time ping. Deliberate: reusing that result would couple this type to its caller's startup sequence for one round trip that runs once per process. The consequence is now stated too — because the dial-time ping is FATAL, the prober's "unreachable at startup" branch cannot fire in the shipped binary. - The keyspace wiring guard parses source and will break on a rename. The alternative on offer needs three packages' constructors collapsed into one API. A guard that costs a one-line update after a deliberate rename beats an invariant with no enforcement, which is what the package comment alone amounts to. RAISED WITH THE LEAD, not decided here: events.EventBus.Publish's global limit parameter is now dead in production, since the handler passes 0 and the process-wide gate owns that bound. Removing it is the clean seam and it is an interface change in a shared package, which is a structural call. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
9a94398bc2 |
fix(server): restore the readiness route I broke, and stop the tests bypassing it (BUG-2727)
A scripted comment edit in the previous commit replaced every "/health/ready" in server.go — including the ROUTE REGISTRATION, inside the /api/v1 group. The real endpoint became /api/v1/api/v1/health/ready, so readiness 404'd at the path the docs, the k8s manifest and every runbook name. Codex round 7 found it. Two failures, and the second is the one worth fixing: - A blanket string replace on a file where the same literal appears as both prose and code. I verified the diff of the comments I meant to change, which is the half that was correct. - The health tests called srv.handleHealthReady directly, so the suite had no opinion about the URL at all and stayed green through a broken route. They now go through srv.ServeHTTP, and a new test pins all three health paths as mounted — with a negative control asserting the double-prefixed path is NOT served, so a router that answered everything could not pass. Mutation-verified: reintroducing the exact defect fails both. Also from round 7, both pre-existing and neither fixed here: - pad_eventbus_publish_total counts publish ATTEMPTS. Publish returns nothing, so a failed Redis publish is logged and still counted, and the counter climbs at its normal rate through an outage. The Help string and the wrapper now say so; the real fix is Publish reporting acceptance, which is BUG-2699's change one bus over. Filed as BUG-2732. - waitForDrain leaks its waiter goroutine when the drain times out. Accepted and now documented at the function: it runs only from Close, so it is one goroutine seconds from process exit, and a cancellable wait would mean tracking every renewal for a benefit that expires with the process. And the admission gate is now reconfigured IN PLACE rather than replaced (round 7 P2). Replacing it left the old gate holding every open connection's slot while the new one started at zero, so those connections stopped counting and the process silently over-granted capacity. Worth recording how the test for it landed: the first version asserted the GAUGE and survived the mutation, because the discarded gate keeps its observer and its releases keep the gauge looking right while the budget is wrong. The visible signal stayed correct and the invisible one went wrong — so the test now asserts that a held slot still refuses the next connection. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
a790810bd6 |
docs: close the cross-artifact gaps codex round 6 found (BUG-2724, BUG-2726, BUG-2727)
The angle no earlier round probed: which artifacts a human or agent CONSUMES should have changed and did not. Five, and the pattern is the one my own record keeps naming — the caveat existed in the artifacts I was editing and not in the ones that get read. - .env.example had neither new variable and still described PAD_SSE_MAX_CONNECTIONS with its old single-endpoint meaning. It is the file an operator copies; docs/deployment.md being right does not help someone who never opens it. - docs/deployment.md called the readiness endpoint /health/ready. The route is /api/v1/health/ready, so every instruction to go read the new redis block pointed at a 404. Corrected there and in four code comments, and the Health Check section now actually shows the three endpoints, the healthy payload, and the degraded one — it previously demonstrated only /api/v1/health, which is the build-info endpoint and says nothing about readiness. - CLAUDE.md listed /api/v1/events and not /api/v1/events/stream at all, so the endpoint this unit bounds was undocumented in the file agents read first. Added, with the limits and the 429 contract. - `pad watch --stream --help` said silence means "no workspace linked or padd unreachable". A capacity refusal now produces the same silence through the same backoff, so the help was enumerating a set that had quietly grown. - The plugin skill told agents "silence means nothing changed" — now false in the same way, and worse, because an agent repeats it to a user as though the quiet were evidence. Rewritten to say what silence does and does not prove. The plugin monitor description had the same enumeration and got the same fix. Checked rather than assumed: there are two SKILL.md files, and only the plugin copy carries a notifications section — the embedded one has no monitor guidance to correct. NOT changed, and raised with the lead instead: deploy/k8s/deployment.yaml points both probes at /api/v1/health, so the readiness endpoint is never consumed. Fixing it is right but it changes rollout behaviour for anyone using the shipped manifest (a database blip would start pulling pods from the load balancer), which is a deployment-posture call rather than part of this unit. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
c10f1200c1 |
test: close the coverage gaps codex round 5 enumerated (BUG-2724, BUG-2726, BUG-2727)
Round 5 asked what has NO instrument, as opposed to what is exercised incidentally. Six real gaps, each now closed by a test that fails when the behaviour is reverted (10 mutations applied, 10 caught): - Config plumbing for PAD_REDIS_NAMESPACE and PAD_SSE_MAX_PER_USER, and the per-user default of 50. The parser and the gate each had tests; a Load() that never populated either field would have passed both while the deployment ran with no namespace and no per-user bound. Same knob-versus-wiring split as day-49's batch-id finding. - The metrics adapter's MAPPING. Both sides of that seam were tested and neither proved the wires were not crossed; an adapter that incremented the sequence-gap counter on a resume gap would have passed everything and sent an incident the wrong way. Each event now fires a different number of times so a crossed wire cannot produce the expected totals. - The REDIS bus's slow-subscriber drop. Only the memory bus's was covered, and they are different loops in different files — the Redis one being the only one a multi-instance deployment runs. - Presence renew and deregister failures. They fail in OPPOSITE directions, which is why they are counted separately, and neither was instrumented. - events.EventBus.SubscribeIfAllowed's own bounds, which had no package-level test at all. BUG-2726 moved the global bound to the admission gate, so the handler passes maxGlobal=0 and that branch is now unreachable from production — it would have been a branch nobody could vouch for. Both bounds are now tested where they live. Also records what round 5 correctly noticed about the existing SSE limit tests: they still pass, but for a DIFFERENT reason than before — the admission gate refuses before the bus is reached. Their names no longer say what they exercise, so the call site says it instead. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
9afedbe1a0 |
fix(server,metrics,watchevents): seven codex round-4 findings — operator and next-author angle (BUG-2727, BUG-2724)
Round 4 read the diff as the operator of a running deployment and as the author of the next change. Five findings were claims my own text made that the code does not support, which is the failure mode this angle is for. 1. The degradation list said Redis loss costs "cross-instance activity events". It costs ALL of them: events.RedisBus.Publish logs its failure and returns without a local fan-out, so subscribers on the originating instance stop receiving too. A responder told only about cross-instance delivery would have looked elsewhere. Corrected in the health payload, both prober log lines, and the docs. 2. config.go promised that connected clients resync after a namespace change. True of the watch stream, false of the activity stream, whose cold replay buffer answers a resume as "caught up" (BUG-2731). The docs already carried the asymmetry; the comment did not, and the comment is what the next author reads. 3. Resume-detected gaps were counted nowhere. They are the only gap shape that is always USER-VISIBLE — the client gets sync_required — so an incident reading pad_watchevents_sequence_gaps_total would have missed the failure mode with the clearest symptom. New pad_watchevents_resume_gaps_total, kept separate rather than folded in because the two are diagnosed differently: one is a delivery fault, the other is any cursor this instance cannot vouch for. 4. The presence-failure metric's doc said every failure leaves sessions unlisted and untargetable. Two of the four ops fail in the OPPOSITE direction — a failed deregister leaves a dead session listed, so a push aimed at it is accepted and reaches nobody — and a generic alert on the total would send a responder the wrong way. Now documented per op, in the code and in the docs table. 5. The go-redis log bridge levels everything at WARN, and the comment justified that with "benign reconnect chatter" I had never enumerated. Enumerated now: the stream carries genuine failures, state changes and informational fallbacks with no severity attached. WARN stays — INFO would bury the dropped-message line the bridge exists for, and classifying by message TEXT would make Pad's log levels depend on go-redis's prose — and a component=go-redis field makes it routable instead. 6. internal/redisns centralizes key construction but cannot stop a future contributor wiring one bus with a different Keys than another: every package compiles, every unit test passes, and the deployment runs split across two keyspaces while looking configured. Adds a wiring drift guard that reads cmd_server.go and fails if the three constructors do not share one Parse-produced value. The rule was already written down in a package comment; this is its enforcement step. 7. The limits are per-process and the startup log, log fields and gauge Help called them "global". Renamed to per-instance / per-principal throughout, with the no-shared-counter caveat in the startup line. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
c03a4851bd |
fix(server,redisns): two codex round-3 findings — DoS via legacy tokens, blank namespace (BUG-2724, BUG-2726)
1. Callers with no user id skipped the per-user bound entirely, so one legacy workspace-scoped token could fill the global budget and 429 everyone else — a denial of service through a deprecated auth path. My own comment argued for the skip on the grounds that bucketing every anonymous caller under one empty string would make unrelated callers evict each other. That was right about the empty-string bucket and wrong about the conclusion: the fix is a better key, not no key. They are now bucketed by workspace, the finest granularity actually available — from the token's own workspace id where it has one, from the resolved workspace otherwise. The residual trade (two legacy tokens for one workspace share a bucket) is stated in the code and in the docs rather than left for a reader to discover. 2. PAD_REDIS_NAMESPACE=" " trimmed to Default, so a broken template substitution silently restored the historical keyspace and collided with the installation the namespace was set to separate from — the exact leak, arriving through the mechanism meant to prevent it. Only a genuinely unset value is Default now; whitespace-only is a startup error naming both alternatives. The first fix needed a second instrument. Mutating the handler to pass currentUserID instead of streamPrincipal SURVIVED the unit tests, which drive the helper directly — the same defect shape as day-49's batch-id finding: testing a knob at the layer that consumes it proves the knob, while the caller passing it is a separate claim. The new handler-level test drives the fresh-install no-auth window through HTTP and fails by name when that wiring is reverted. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
03518466ab |
fix(server,metrics,docs): five codex round-2 findings (BUG-2724, BUG-2726)
Round 2 probed angles round 1 did not: rolling upgrade and rollback, request cancellation, and whether any operator-facing text now contradicts the code. Four of the five were the latter. 1. The admission slot was held through the Redis presence cleanup. Defers run LIFO, so the acquire-site release ran LAST — after Remove's round trip, bounded by presenceOpTimeout (5s) and a wait on the renewal goroutine. A reconnect arriving inside that window could be refused by a bound the connection had already stopped consuming, and the window is widest during a Redis outage, which is when clients reconnect most. A second deferred release, registered later so it runs first, closes it; the acquire-site defer stays as the safety net for early returns, and release is idempotent so deferring twice releases once. 2. pad_sse_connections_active is written by the events.EventBus wrapper, so it has only ever counted the workspace stream. That was every SSE connection Pad had a limit for until this branch; it no longer is, so an operator watching it against the global limit would be reading one endpoint's share of a two-endpoint budget. Adds pad_stream_connections_active, driven by the admission gate itself, and both Help strings now name their population. Wired from either SetMetrics or SetSSELimits (either can land first) and from the lazily-built gate, each covered by a test — a gauge stuck at zero while streams are held is the same shape of lie as a metric that is not registered at all. 3. The limits are enforced in-process and the docs called them "Global". With the shipped k8s manifest's two replicas, 1000 admits ~2000 and a user can hold 50 per pod. Documented as per-instance, with the multiply-by-replicas note and a pointer at the new gauge. 4. A namespace cutover partitions a rolling upgrade — namespaced and un-namespaced replicas are two installations for the length of the rollout — and rolling back with the variable still set silently restores the split. Both now stated, with the env var and the binary having to move together in both directions. 5. Client resync across that cutover is honest on the watch stream (the epoch key detects the changed id space) and SILENT on the activity stream, whose cold replay buffer answers a resume as "caught up". Documented, and filed as BUG-2731 rather than fixed here: it is pre-existing, fires on any replica restart, and the minimal fix changes reconnect behaviour for every deployment, which wants a ruling rather than a quiet patch. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
2b33184ef1 |
fix(metrics,watchevents,server): three codex round-1 findings (BUG-2727)
1. pad_redis_up was registered unconditionally, so a deployment with no Redis exported a permanent 0 — which reads as "Redis is down" to anything scraping it and would have every single-process binary alerting on a dependency it does not have. It now registers only inside the PAD_REDIS_URL branch, matching /health/ready, which already omitted its redis block on the same condition. My own field comment claimed the absent behaviour while the code did the opposite. 2. The receive loop could report a false exit during shutdown: Close cancels the context AND closes the pubsub, and Go picks between ready select cases at random. A context re-check makes the outcome independent of that. Scope stated honestly, because it is narrower than the finding implies. With the guard removed, 200 Close cycles under publish traffic produced zero false exits — and removing it AND reversing Close's ordering still produced none, because Close waits on the receive goroutine and the goroutine observes the cancelled context either way. So no test fails if these three lines are deleted, and both the code comment and the test doc say so rather than implying coverage that does not exist. It is kept as defence against a future reordering, not as a fix for observed behaviour. 3. Corrupt session entries returned a list error without incrementing the failure counter, so pad_session_presence_failures_total under-reported precisely the case an operator is least likely to find another way — a dead Redis is obvious, a corrupt row is not. Both corrupt shapes now count. The non-string arm is unreachable through MGET (Redis answers nil for a key holding a non-string value, verified), so it is annotated as defensive and the test says no leg drives it instead of quietly covering only the reachable one. Test-power notes are measured, not asserted: the Close test catches removal of the select's ctx case (mutation-verified) and does not discriminate the guard. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
0877b260c1 |
feat(redis): namespace every Redis keyspace from one shared config value (BUG-2724)
Every Redis key and channel Pad uses was flat — pad:events:, pad:event_seq, pad:watchevents*, pad:session:* — so two Pad installations pointed at one Redis endpoint cross-feed each other's notifications and merge each other's session-presence registries. Different logical DB numbers do not help: Redis pub/sub is not namespaced by DB at all. The exposure is narrow but real. Delivery is filtered per caller on user id, and user ids are per-installation UUIDs, so cross-feed needs the same id in both installations — a CLONED database, such as a staging environment restored from a production dump. For that case it is a genuine cross-tenant leak: foreign sessions listed in the picker, and a private push deliverable across installations. Fixed the way internal/watchevents' existing ruling demanded: not by one package growing a prefix the others lack, but through internal/redisns — one value parsed in cmd/pad/cmd_server.go and passed into all three constructors. The three cannot drift because there is nothing to drift from, and the operator rule is stateable in one sentence for every keyspace. PAD_REDIS_NAMESPACE defaults to empty, which reproduces the historical names byte for byte, so an existing deployment keeps addressing its own replay buffers, counters and presence entries across the upgrade. Tests assert both directions per keyspace — present under the namespace AND absent under the historical names — because an implementation that wrote both would still cross-feed while passing a one-directional test. Namespaces are validated at startup, and a colon is rejected specifically: it is Pad's own separator, so namespace "a:events" would build pad:a:events:<ws> and collide with installation "a"'s channel — reintroducing the cross-feed through the mechanism meant to fix it. Names are built through a function rather than assembled from a literal at each site, and redisns' doc says why: "pad:" also begins Pad's OAuth SCOPE values (pad:read / pad:write / pad:admin) in four files, so a grep-driven prefix sweep would break authorization. Not included, deliberately: hash tags for Redis Cluster. BUG-2724's trail recommended shipping them alongside on cost-sharing grounds; that premise is falsified by publishScript, which spans four keys in one EVAL and fails CROSSSLOT exactly as presence's MGET does. There is no cheap half, and no cluster client here to exercise tagged keys against, so they would ship untested by construction. Cluster stays documented as unsupported and the future unit is named on the trail. Renaming is a CUTOVER for the buses (the seq and epoch keys carry Last-Event-ID meaning, so connected clients resync) and free for presence (90s TTL). Both stated in docs/deployment.md and at the constructors. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
720b792176 |
feat(server,config): bound the watch-events stream, with one budget across both SSE endpoints (BUG-2726)
GET /api/v1/events/stream had no concurrent-connection limit of any kind. PAD_SSE_MAX_* gated only /api/v1/events, and the API rate limiter caps how FAST connections are opened, not how many are HELD — so one authenticated user could hold arbitrarily many streams, each costing a goroutine, a bus subscription and, since BUG-2698, a presence registration in shared Redis. The bound is a process-wide admission gate rather than a second per-bus limit. Each bus can bound its own subscribers atomically and events.EventBus already does, but neither can bound the two together, and a held connection costs the same process resources whichever endpoint opened it. A global limit on one bus would have let a user exhaust the machine through the other while every configured limit still read as satisfied. So PAD_SSE_MAX_CONNECTIONS now covers BOTH endpoints and is passed to the events bus as 0. That is a deliberate re-point of an existing knob, ruled rather than assumed: an operator who tuned it for one endpoint is now bounding both and may reach the limit sooner. A knob that silently bounded half the connections it named is the worse failure — invisible — where this one announces itself and is tunable. A startup log line reports the effective limits and which endpoints each covers, so the change is visible without reading release notes. New PAD_SSE_MAX_PER_USER (default 50) applies to both endpoints. The global bound alone lets one user exhaust the process for everyone, which the per-workspace limit cannot prevent — the watch stream has no workspace to count against. Per-workspace stays /api/v1/events-only for the same reason. Refusal is 429 sse_limit_exceeded, matching the existing endpoint. The CLI monitor folds any non-200 into its backoff ladder (linear, 5s base, 5min cap, reset on connect), verified rather than assumed, so a refused stream backs off instead of spinning. Deliberately NOT a registry-side cap: PR #1175 added one in review round 17 and removed it in round 21, because it bounded one of three resources a held stream consumes, was never hard (admitted renewals must bypass it), and cost delivered_sessions its honesty. The admission check is upstream of all of that — refusing costs one connection instead of making a live session untargetable. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
a061c17298 |
test(server): pin the Redis health prober and presence failure reporting (BUG-2727)
The health test's load-bearing assertion is that an unreachable Redis leaves /health/ready at 200 — a test that only checked the payload would pass against a handler that also 503'd, which is exactly the regression that would pull healthy replicas out of a load balancer over a degraded feature. Each test asserts its own premise first: no redis block without a prober, nothing reported before Start, healthy presence operations reporting zero failures. Without those legs an always-reporting implementation would be indistinguishable from a correct one. Both suites bound DialTimeout explicitly. go-redis does not apply a command context to connection establishment, so the unreachable legs would otherwise wait out the 5s default per probe. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
8dea9abca3 |
feat(watchevents,metrics): operational observability for the Redis notification bus (BUG-2727)
The watch bus detects four conditions an operator would want to alert on — a notification dropped for a slow local subscriber, a gap in the received id sequence, an id-space reset, and the receive loop stopping — and until now reported all four to slog and nowhere else. Log lines are not alertable without someone already looking, and the last of the four was not even logged: the loop returned silently, leaving an instance that publishes fine and receives nothing indistinguishable from a quiet workspace. Adds watchevents.Observer, an adapter seam rather than a bus wrapper. The events.EventBus wrapper shape does not work here: every condition is detected on the RECEIVE path, inside the bus, and is invisible at the Bus interface — a wrapper can count publishes and subscribers, but not a notification that never arrived. Two corrections to BUG-2727's filing, both verified against go-redis v9.22.0 rather than assumed: - Its proposed fix — "re-subscribe rather than exiting where the cause is recoverable" — would be dead code. PubSub.Channel's message channel is closed ONLY on pool.ErrClosed; every other receive error is retried indefinitely, and a health-check goroutine pings every 3s and reconnects on failure. So go-redis already does the re-subscribing. The exit gets an ERROR log and a counter instead, which is what the condition actually needs. - The genuinely silent path is go-redis DROPPING messages when a subscription's 100-deep buffer stays full past its 60s send timeout, logged only through go-redis's own logger. Pad cannot count that directly, so it is reported by its CONSEQUENCE (a sequence gap) and its cause is made visible by routing go-redis's logger into slog. Observer's doc comment states that boundary, so a gap is not misread as evidence of any particular cause. Session presence gets the same treatment for the same reason: it is fail-soft everywhere by design, so its failures have no user-visible signal beyond a push that quietly reaches fewer sessions than it should. The renew counter is deliberately NOT throttled where its log line is — throttling the metric would make it under-report during the incident it exists for. Tests assert the CONDITION increments the counter, not that the counter exists, and each asserts its own premise first (a healthy subscriber reports nothing; contiguous ids report nothing; a cold start reports nothing) so a bus that reported on every notification could not pass. The receive-loop test drives the real closed-client condition rather than calling the reporter. Claude-Session: https://claude.ai/code/session_01JVDBKbgn3Xt7ndW1YoYd8X |
||
|
xarmian
|
ea139272ce |
fix(server,watchevents): shared session presence + honest push acceptance (BUG-2698, BUG-2699) (#1175)
Two coupled defects in the push path, fixed as one unit because 2699's honest-acceptance signature is the substrate 2698's fix reports through. BUG-2699 — Bus.Publish reports acceptance. The endpoint returned 200 pushed:true for a publish that was dropped, because Publish returned nothing and swallowed every failure. An error is two outcomes and they are kept apart: ErrBusClosed proves nothing was published (503 unavailable, safe to resend), while any other error means UNCONFIRMED — go-redis retries a command whose reply was lost, which is why the publish script already carries a dedupe token — and gets 502 push_unconfirmed, deliberately off the web client's safe-to-resend list. MemoryBus was the worse case, not the exempt one: neither implementation checked `closed`, and the in-process one dropped silently with no log at all. Seven production call sites, not the six the item named; the six best-effort producers discard through one named helper, and an AST-based test fails when a new producer publishes directly. BUG-2698 — RedisSessionPresence. A session-targeted push was resolved against the answering replica's presence registry, and the handler skips the publish when the target is absent, so a POST landing on A for a session held on B dropped the instruction and answered delivered_sessions:0. Fixed at the REGISTRY rather than the gate: a shared registry makes the snapshot right, which makes the picker complete and restores the gate's original premise, so the existing skip becomes correct for the reason it was written. Entry and index are written atomically under a TTL renewed by a goroutine that lives exactly as long as the connection; a crashed process stops renewing and Redis clears it. Staleness is unchanged and now stated in full: ~30s for a dropped client, ~90s for a dead instance. delivered_sessions becomes nullable — null means published-but-uncountable, never zero — documented as three states at every consumer. 35 Codex review rounds. Notable: a per-user registry cap was added and then removed after three consecutive rounds found defects inside it and a fourth was asked whether it belonged in this PR at all; a context bound was documented, disproved by its own test (go-redis does not apply a command context to connection establishment — 5.0s measured against a 150ms ctx), and rewritten to say what is true. Every fix was mutation-checked; one instrument was deleted for passing on broken code and one for not asserting its own premise. Filed rather than folded in: BUG-2724 (Redis keyspace namespacing + Cluster), BUG-2725 (delivered_sessions is an estimate with error in both directions), BUG-2726 (no concurrent-connection limit on the watch stream), BUG-2727 (Redis absent from readiness/metrics; silent subscriber loss), BUG-2728 (epoch-reset resume lead). Gates: build · make lint 0 issues · go test ./... (25 pkgs) · svelte-check 0 errors · vitest 1738 passed · CI 7/7 including Go (PostgreSQL) and Nix. |
||
|
xarmian
|
6a37512227 |
feat(server): outbox drain — webhooks delivered from the choke point (TASK-2714) (#1173)
* test(store): pin the events/1 taxonomy as an independent copy (TASK-2714)
TestCanonicalEventsAreFullyDeclared iterated kernelevents.Canonical() and
asserted each entry resolved something non-empty. That check cannot fail for
any table the compiler accepts: eventSpec requires both fields, so a corrupted
table — an entry deleted, an entry added, item.deleted quietly rebased onto the
ref-only payload — passed its own validation. A test that agrees with whatever
the table says is not a test of the table.
The sixteen name/subject/family triples are now written out as literals, so the
test DISAGREES with the table when the table moves. The wire strings behind the
name constants are pinned separately, because the triple map is keyed on
literals and a renamed constant would otherwise slip through as long as the
constant and the table moved together.
Ordered as this unit's first commit because TASK-2714 edits that table (the
handler-path bulk mapping): an independent copy earns its keep at the moment of
the edit, not before.
Mutation matrix, 4/4 caught: drop member.joined (17 -> 15 count mismatch and a
missing-name error), rehome item.deleted onto ref_only (family mismatch),
rename ItemMoved's wire string to item.move (constant leg), add an undeclared
item.frobnicated entry (count + undeclared-name + non-canonical legs). The
fourth reported "survived" on its first run because the sed never matched the
table's alignment — the mutation was verified present in the file before the
result was believed.
TASK-2714 requirement 4 (lead pass on #1172).
* feat(store): max-age prune for undispatched outbox rows (TASK-2714)
Requirement 3's missing half. PruneDispatchedOutbox filters on dispatched_at
IS NOT NULL, so a row that can never be delivered — a workspace whose only
webhook was deleted, an endpoint that 4xxs forever — is unreachable by it and
keeps its frozen payload indefinitely.
That matters because SPEC-3 makes payload privacy TEMPORAL. An outbox payload
is a frozen snapshot and account deletion's de-identify posture reaches only
live rows, so the retention window is the whole privacy claim; a window only
one of its two halves can close is not a window.
The trade is stated in the doc comment rather than left to be inferred:
at-least-once holds WITHIN the retention window and not past it, which is why
the caller's max-age must be far larger than any retry schedule. Deleting
rather than stamping the rows dispatched is deliberate — a dispatched stamp
would be a lie in the durable record, and this table is the only evidence of
what the kernel emitted.
Mutation matrix, 2/2 caught: drop the dispatched_at IS NULL clause (prunes the
aged DISPATCHED row too, handing retention two owners with different windows),
drop the occurred_at cutoff (prunes a young pending row a retry is still
owed). The test asserts its own premise — all three seeded rows are confirmed
present before the survivor checks, which would otherwise pass for a reason
unrelated to the prune.
No caller yet: the drain loop wires it up in the next commit.
* feat(events): derive SSE names from the taxonomy; retire item.updated_with_comment (TASK-2714)
SPEC-3 §"the choke point owns the canonical→surface name mapping". SSE's
snake_case vocabulary and the webhook dot-form vocabulary drifted because
nothing tied them together — each was hand-passed at its own call sites. This
ties them.
v1.5 pins what "derive" means: NAME derivation, not delivery path. SSE stays
direct-published at the mutation site, because it carries request-scoped
attribution (Actor / ActorName / Source) that a frozen outbox payload
deliberately does not hold; only its NAME now comes from the taxonomy. Moving
SSE behind the drain is TASK-2722.
- eventSpec gains an `sse` field — ONE table, not a second map, for the reason
round 11 of the last unit established: a separate map can disagree with the
first and fails open exactly when it matters. Empty is a real value (attachment,
member and pack events have no SSE surface) and SurfaceSSE reports false for it,
so silence can't be mistaken for a name.
- Several canonical events derive the SAME SSE name — status_changed and moved
both surface as item_updated — because the SSE vocabulary is coarser than
events/1 and the UI never distinguished them. The finer name is what the
webhook wire and bindings get.
- The 12 canonical SSE publish sites take their names from derived package vars,
resolved AT INIT. Every call site is a compile-time constant, so a missing
surface is a startup panic rather than a per-request decision between "log and
drop" and "publish under an empty name".
- handlers_item_links.go keeps the events.ItemUpdated literal, commented: link
mutations are silent in events/1 (v1.5), so there is no canonical name to
derive from. TASK-2723 carries link.created / link.removed.
- item.updated_with_comment retired (v1.2, Dave's ruling). One producer deleted;
the events.ItemUpdatedWithComment constant deleted with it — it had no producer
and no web consumer (grepped .go/.ts/.svelte), so leaving it would leave a name
a future publisher could reach for.
The compat guard is what makes this a refactor rather than a wire change:
TestDerivedSSENamesMatchTheLegacyWireVocabulary asserts each derived name equals
the events.* constant clients are pinned to. A derivation producing
"item.created" or "item_deleted" would break the live UI while every other Go
test still passed.
Mutation matrix, 3/3 caught: rename item.deleted's SSE surface to item_deleted
(both the taxonomy test and the compat guard fail), split item.moved onto its own
SSE name (same), make SurfaceSSE return (spec.sse, ok) so no-surface events fail
open (the taxonomy test's silence leg names all four). Running total 9/9.
go test ./internal/server ./internal/store ./internal/events: all green.
* feat(webhooks): synchronous DeliverEvent seam with per-endpoint outcome (TASK-2714)
Requirements 1 and 2. Dispatch returns once its per-hook goroutines are
spawned and reports nothing, so a drain built on it would stamp rows
dispatched while the HTTP requests were still in flight — losing exactly the
events the outbox exists to make unlosable. DeliverEvent blocks and tallies.
- Delivery carries WorkspaceID / EventID / Event / OccurredAt / Payload.
OccurredAt is the EVENT's timestamp, not dispatch time: SPEC-3 pins
time-relative binding predicates to it, so stamping time.Now() would make
every consumer's notion of when a mutation happened depend on how backed up
the queue was. Payload is json.RawMessage — []byte would base64 the snapshot
into a string that is valid JSON and completely unusable.
- WebhookPayload gains ID, the consumer dedupe key SPEC-3 §Delivery guarantees
already told consumers to use. Before this, that instruction named a field
nobody could see. omitempty, because the "webhook.test" ping is not a kernel
event, has no outbox row, and must not invent an id.
- DeliveryOutcome counts rather than a status, because one event fans out to N
endpoints and the answers differ. Three distinctions the drain branches on:
Matched==0 is SUCCESS (a webhook-less workspace is owed nothing; reading it
as undelivered would back up every event in every such workspace until
retention deleted it); Permanent does not hold the event pending (re-sending
to an endpoint that will reject it again costs the queue its progress);
Transient does. Retryable() states the ack rule once instead of letting each
caller re-derive it.
- A returned error is reserved for the SERVER's failures — listing hooks,
marshalling. Those must not ack: nothing was attempted, so the event is
still owed in full.
- Dispatch keeps its async shape for its one remaining caller and says so.
deliver() now returns the outcome it always computed; the async path
discards it.
Mutation matrix, 6/6 caught: stamp dispatch time instead of occurred_at; drop
the envelope id; pass the payload as []byte (base64); deliver asynchronously
and assume success (the synchronous leg names it exactly); count a permanent
rejection as transient; swallow a store failure into a zero outcome (the test
prints the outcome that would have acked an undelivered event).
Running total 15/15. go test ./internal/webhooks green.
* feat(store): batch_id correlation for handler-path bulk mutations (TASK-2714)
F2's write half. A lane-wide bulk action is a handler LOOP over per-item store
mutations with no enclosing transaction, so each member writes its own
canonical outbox row — which is what keeps SPEC-3's per-member binding
evaluation free, and also means that without a marker the drain would put 200
item.deleted events on the webhook wire for a 200-item lane archive: exactly
the flood TASK-1668's batch event exists to prevent.
RECORDED, NEVER INFERRED (SPEC-3 v1.5). The schema-free alternative was
grouping pending rows by workspace and a time window, which would fold two
unrelated single updates into somebody's bulk event whenever they landed in
the same tick. A wire event saying "these five items changed together" is only
true if something recorded that they did.
- migrations 082 / pgmigrations 060: nullable event_outbox.batch_id, no FK
(a batch is not a row anywhere, it is a name the handler minted), plus a
partial index on the pending set.
- store.MutationOption / WithEventBatch: variadic, because every existing call
site is a single-item mutation with nothing to declare and making all of them
pass a zero value would bury the one case that matters.
- The handler mints one id per bulk OPERATION, before the loop and
unconditionally — deciding mid-loop whether a run "counts as" a batch would
make the correlation depend on how far the loop got.
POPULATION CORRECTED: my escalation said four store methods; it is FIVE.
archive (DeleteItem), restore (RestoreItem), move (MoveItemWithPreCheck), field
update (UpdateItemWithPreCheck) and assign (UpdateItem) are the complete set of
mutating store calls handlers_items_bulk.go makes — restore was the one I
missed, which is CONVE-18's exact lesson arriving one level up. The test drives
all five rather than sampling, because the failure is per-method: a signature
that accepts the option and never threads it compiles, passes everything else,
and silently un-batches one of the six bulk verbs.
Mutation matrix, 5/5 caught across the four distinct emit sites: drop the stamp
on the update path (both Update legs fail), on delete, on restore, on move. The
delete mutation first read as SURVIVED — it had made the package fail to BUILD
(opt then unused), and the grep for test-level FAIL lines printed nothing. The
compiler catch is the stronger result, but the instrument mis-reported it, so
it was re-run with opt kept alive and the test named it directly.
go test ./internal/store ./internal/server green.
* test(server): anchor the SSE compat guard to the client's literal strings (TASK-2714)
The guard compared the derivation against events.* — the Go side. A
coordinated rename of the taxonomy AND the constants passes that, and is
exactly the change that breaks the browser: the client is pinned to the
STRINGS, in web/src/lib/services/sse.svelte.ts's ITEM_EVENTS.
The wanted column is now a literal copy of what the client listens for, with
the file named. events.* is asserted alongside as a second leg, so a drift
between the Go constants and the client is attributed rather than merely
reported. Same disagree-with-the-table principle as the taxonomy test, one
layer out: this file has to be edited by hand when the wire vocabulary
intentionally changes, and that edit is when someone goes and changes the
client too.
Mutation matrix, 2/2, each hitting only its own leg: rename events.ItemCreated
to the dot-form with the taxonomy untouched (drift leg fires), and make the
taxonomy publish the dot-form on SSE (browser leg fires). Running total 22/22.
Lead's catch on the day-49 review of commit
|
||
|
xarmian
|
2ed6e71ad3 |
feat(store): transactional event outbox — the SPEC-3 choke point (TASK-2658) (#1172)
* feat(store): transactional event outbox + events/1 item taxonomy (TASK-2658, SPEC-3)
Phase-0 unit 2 of PLAN-2656, store half. Events are now written to an
outbox in the SAME transaction as the mutation that produced them, so a
committed mutation cannot lose its event and a rolled-back one cannot
leak one. Nothing drains the outbox yet — behaviour is unchanged.
- migrations 081 / pgmigrations 059: event_outbox. Deliberately no FKs on
workspace_id / subject_id: an outbox row must outlive its subject, or
item.deleted cascades away exactly when it matters. Retention, not
referential integrity, bounds the table.
- internal/kernelevents: the closed events/1 name set (SPEC-3 v1.3) with
IsCanonical enforcing the closure rule at the choke point.
- store/event_outbox.go: writeOutboxTx (tx-scoped, hard-fails the
mutation rather than degrading to best-effort), the item payload shape
(snapshot EMBEDDED so query/1 predicates apply verbatim, prior_status
alongside as the envelope pseudo-field), and the drain-side primitives.
- item.created / updated / status_changed / moved / deleted / restored
emitted from inside their mutations' transactions, from in-tx snapshot
read-backs rather than caller input.
- SPEC-3 v1.3 disjoint-delta rule: canonical events partition a
mutation's delta and a mutation emits every event whose slice changed.
The seam diffs slices rather than branching on "was this a status
update" — branching drops the item.updated half of a mixed update.
- ImportWorkspace stays silent per the SPEC-3 ruling, commented at the
INSERT so it reads as a decision. insertItemTx's "every creation side
effect lives in this one place" comment corrected: it is API-path only,
and import is the counterexample two units have now been misled by.
* feat(store): comment / attachment / member events on the outbox (TASK-2658)
Completes the store half of the choke point. Same rule throughout: the
event is written on the mutation's own transaction, from an in-tx
read-back rather than caller input.
- comment.created / comment.updated. GetComment gains a Queryer form so
the emit reads through the tx: a pool read takes a different
connection and cannot see the uncommitted write, so it would return
the PRE-write row and the event would describe a state that is not the
one committing (mutation-verified).
- attachment.added, gated to user-visible originals. Variants are
attachment rows too — a thumbnail carries parent_id plus a variant tag
— so an ungated emit announces three events per image upload, two for
files no user added. Transform outputs stay admitted: no parent, and a
user did add them.
- member.joined. AddWorkspaceMember becomes transactional to carry it; a
self-committing INSERT plus a separate emit is the shape that loses
events on a crash.
item.bulk_updated is NOT here, and not by omission: bulk is a handler
loop over per-item store mutations, each already emitting canonically
from its own transaction. There is no bulk transaction to write it in,
so the batch event is delivery-side aggregation — it belongs with the
drain in TASK-2714, where SPEC-3's per-member binding evaluation is
already satisfied by the per-item rows.
* fix(store): emit item.deleted for a cross-workspace move's source archive (TASK-2658)
Self-caught during the diff review. archiveItemForCopyTx deliberately
REPRODUCES DeleteItem's UPDATE inside the copy's transaction rather than
calling it, so it did not inherit DeleteItem's new emit: a cross-workspace
move archived the source silently while an ordinary archive of the same
item announced itself. Invisible until something drains the outbox, at
which point moves would just stop being observable.
Same ordering as DeleteItem — snapshot in-tx BEFORE the UPDATE, while the
row is still live, because SPEC-3 requires the final pre-archive state.
Also amends the file's DR-14 header. DR-14 says no fanout inside the
transaction because a rollback would leak the event; an outbox row written
on the SAME transaction rolls back WITH the copy, so that rationale does
not reach it. The three things DR-14 actually names — activity row, SSE
publish, webhook — still happen post-commit at the caller, unchanged. A
documented decision should not be silently contradicted by the code.
* fix(store): compare the move's event slices against an in-tx pre-move snapshot (TASK-2658)
Codex round 1, P2 — a defect in my own round-1 code. MoveItemWithPreCheck
refreshes `existing` in-tx only on the precheck path; on the no-precheck
path it stays the PRE-LOCK pool read. The emit block compared it against
the post-move in-tx snapshot, violating a precondition documented on
itemUpdatedSliceChanged itself (both snapshots must come from getItemTx,
or rendering differences read as changes), and a stale CollectionID makes
the item.moved decision wrong outright.
Adds a dedicated `preMove` in-tx snapshot and tightens the read: it used
to tolerate a failure by silently keeping the pre-lock value, which only
degraded from_status. It now also decides which events fire, so a
degraded read is no longer an acceptable outcome — under a held lock on a
row just resolved live, an error or missing row means something is wrong.
* feat(store): item.bulk_updated for store-side bulk mutations; purge the outbox (TASK-2658)
Codex rounds 1 and 2. Two more item-mutation write paths emitted nothing,
and both are single-transaction bulk mutations, so their emits are WRITES
and belong in this unit rather than with the drain:
- collections.go: renaming a select OPTION rewrites items.fields on every
row carrying the old value.
- wiki_links.go: renaming an item rewrites the CONTENT of every item that
links to it by title.
Each emits ONE in-tx item.bulk_updated rather than per-row item.updated:
the user performed one action, and per-row fan-out is the flood TASK-1668
already decided against. Per-member snapshots keep item-level bindings
evaluable, which is what makes batching safe (SPEC-3 v1.1). Payload size
is deliberately unbounded in v1 — capping members silently drops binding
evaluation for the tail, and dropping `content` would break exactly the
bindings the wiki cascade exists for.
Also from round 2:
- Workspace purge now deletes event_outbox. It has no FK by design (a row
must outlive its subject), so nothing deleted it on the purge's behalf,
and payloads hold full item content and comment bodies — a purged
workspace's text would have stayed readable indefinitely. Added to
wsChildTables so the exhaustive-purge test covers it.
- Documented that ListPendingOutboxEvents is deliberately cross-workspace
and unauthorized, and must never be reachable from a request path.
- The two callers that discarded AddWorkspaceMember's error now log it.
Not fatal (that is BUG-2715), but this unit made the call transactional
and so gave it a new way to fail; widening a swallowed error without
making it visible is how a failure mode goes unnoticed.
* fix(store): classification correctness + dialect-neutral payload validation (TASK-2658)
Codex round 3, five findings.
A REAL SILENT-EVENT BUG in the classifier. The done-key mask ran
unconditionally, but the status machinery (extractFieldValue) only reads a
done-key value when it is a JSON STRING. So on a collection whose done
field holds a number, `{"stage":1}` → `{"stage":2}` produced NO EVENT AT
ALL: status_changed could not see it, and the mask deleted the key from
both snapshots so item.updated could not either. Now the key is masked
only when both sides hold a string there — exactly the condition under
which status_changed will describe it. When it will not, the change falls
back to item.updated's slice, where something can.
Payload JSON is now validated in Go. The column types DISAGREED: Postgres
JSONB rejects malformed JSON at the INSERT, SQLite's TEXT accepts it, so
the same bad payload failed a mutation on one backend and silently
persisted an undeliverable event on the other.
Corrected an overclaim of my own: the exclusion-list comment said a new
column is compared by default. True only of columns that reach
models.Item's JSON — last_restore_seq and the content-flush watermarks are
invisible to the diff no matter what the list says. Unreachable today
(every caller that moves them also writes content or fields), but not
structurally guaranteed, and now written down as a constraint on adding
persisted columns.
Tests: a custom done-field key (every previous classification test used
"status", so a classifier hard-coded to that key would have passed them
all), non-string and non-object blobs, malformed payload rejection, and
the bulk test now asserts member IDENTITY and the delta rather than a
count and a substring.
* fix: comment-accuracy sweep + no-op comment gate + enumerate the remaining discards (TASK-2658)
Codex round 4, aimed at the claims my own comments make. Three of them
were false or overclaiming, which is the point of pointing a review round
at your own prose.
- taxonomy.go and migration 081 described the END STATE — a drain loop, a
unified SSE/webhook vocabulary — as if it existed. Both now say plainly
that nothing drains the table, that the legacy hand-calls still fire
unchanged, and that the mapping and retirement are TASK-2714. A comment
describing the intended end state in the present tense is how a reader
concludes a feature is broken.
- The hop bound and the §L5 quota text read as running behaviour. Nothing
propagates a hop yet (no binding kernel), so every production write
leaves it 0 and the depth check is exercised only by tests. Said so,
and recorded the surfacing obligation as an obligation.
- The re-delete comment was wrong TWICE. The zero-row return exits before
the nil-snapshot guard, so that guard does not participate in re-delete
at all — it is what keeps this correct if the order or predicate ever
changes. My round-3 "correction" swapped one wrong mechanism for
another because I reasoned from a mutation result instead of the code.
Real behaviour fixes in the same round:
- A no-op comment edit no longer emits. The UPDATE matches on id alone,
so re-saving an identical body touched the row and emitted
comment.updated; the row-count check never suppressed it. Comparing the
body does, which also makes comment.updated consistent with the item
events.
- applyFieldMigrationsTx returns 0, not totalAffected, when emission
fails. Every error there rolls the caller's transaction back, so the
count described writes that never committed.
- Two MORE callers still discarded AddWorkspaceMember's error (the JSON
import and bundle import paths). Round 2 named two; I fixed those two
and did not enumerate. All nine call sites checked this time; the two
remaining discards now log.
Filed BUG-2716: the activity row commits before the comment and cannot be
reordered (the comment carries its id), so a failed comment write leaves
an orphan "commented" activity. Documented at the call site.
* fix(store): partition item.bulk_updated by the members' own workspace (TASK-2658)
Found in my own multi-tenancy probe while round 5 ran, not by the oracle.
emitBulkItemEventTx published every member under the workspace the CALLER
passed. For the collection-option rename that is right. The wiki-title
cascade is not so obviously safe: its source query selects on
target_item_id alone and carries each source row's workspace_id per-row
rather than assuming the renamed item's, so a member in another workspace
is not excluded by construction. That would have put one workspace's item
content on another workspace's webhook.
Whether it is reachable through today's queries is not the question worth
answering — "unreachable" is a property of the current query, not of this
function. Partitioning costs one map and makes it impossible.
Population, per CONVE-18: five emit helpers. Four derive the workspace
from the subject row itself (item, comment, attachment) or from the
membership being written (member.joined), so they are correct by
construction. One — bulk — took a caller-supplied id, and is fixed.
* fix(store): prior_status must be present on a transition FROM an empty status (TASK-2658)
Codex round 6, spec-conformance angle. SPEC-3 §Bindings makes prior_status
the envelope pseudo-field that lets a predicate filter "nonterminal →
terminal". An item can transition FROM no status at all — "" → "open" is a
real status change and item.status_changed fires for it — but `omitempty`
on a plain string dropped the key entirely, leaving a predicate unable to
tell "the prior status was empty" from "this event carries no prior
status".
Now a *string: nil on every event that has no prior status, and
present-and-possibly-empty on item.status_changed, where the empty value
is data. My original reasoning — that an empty string should never appear
"where a prior status is meaningless" — was right about the events where
it is meaningless and wrong about the one where it is not.
Also documents the bulk-snapshot read cost at itemSnapshotsTx rather than
leaving it to be discovered: N sequential joined reads under the caller's
lock, which roughly doubles an already-N-long hold (the migration loop it
serves already issues N sequential UPDATEs under that lock by design).
Batching it is BUG-2718; BUG-2717 covers the redundant post-commit re-read
on move and restore. Both spun off rather than folded, because each adds
an unreviewed path to a change that has been through six review rounds.
* fix(store): keep assignee name and email out of event payloads (TASK-2658)
Found in my own privacy-lifecycle probe while round 7 ran; round 7
independently reported the wider class.
An outbox payload is a frozen snapshot that outlives its subject by
design. Account deletion's de-identify pass (DeleteAccountAtomic) nulls
identity on LIVE rows so a departed user stops being legible — it cannot
reach a frozen payload. Every item event for an assigned item was
carrying the assignee's NAME AND EMAIL, and nothing drains or prunes the
table today, so those stayed readable indefinitely.
The rule applied, stated as a rule rather than a proxy: remove directly
identifying personal data, keep opaque identifiers and row state.
assigned_user_id stays — a predicate filters on it, and once the account
is gone it is a dangling reference to nobody.
Population enumerated rather than fixed one instance at a time (CONVE-18):
five payload shapes reach the outbox. Item-single and item-bulk carried
JOIN-populated name + email and are scrubbed. Comment (`author`),
attachment (`uploaded_by`) and member.joined (`user_id`) carry only their
own row's columns. Exactly one shape needed it, and what made it stand out
is that it was the only one carrying a join rather than the row.
* feat(store): comment.deleted + attachment.removed, ref-only (TASK-2658, SPEC-3 v1.4)
Round 7's privacy-lifecycle findings, resolved by adding the vocabulary
the conflict was missing rather than by deleting rows.
Without a delete marker, a hard-deleted subject's undispatched
created/updated rows were the ONLY record it ever existed — forcing a
false choice between dropping committed events (breaking the outbox
guarantee) and delivering deleted content forever. With one: the create
event still delivers, the deletion is announced REF-ONLY, and retention
prunes both. Privacy of a frozen payload is temporal, which makes the
drain load-bearing for privacy and not only for delivery (TASK-2714).
REF-ONLY is the contract, not a detail. A deletion event must not re-ship
what it deletes — the consumer needs to reconcile its model, not receive a
copy of what the user removed. Sharper for attachments, whose full
snapshot carries filename, content hash and STORAGE KEY: a locator for
bytes the system just reclaimed. Deliberately asymmetric with
item.deleted, whose subject is an archive and stays addressable.
- DeleteComment becomes transactional and emits comment.deleted. Refs are
read before the DELETE, because afterwards there is no row to read.
- ClaimSoftDeletedAttachment emits attachment.removed. The transaction
does not weaken the BUG-2415 claim protocol: the claim's conditionality
lives in the DELETE's WHERE clause, unchanged.
- ClaimNeverAttachedAttachment stays SILENT, deliberately. It reclaims
rows that were never attached to an item, and attachment.added fires
only for attachments written against a live item — so those rows never
announced their arrival, and announcing their removal would hand a
consumer a deletion for an id it has never seen. Tested as an asymmetry,
not left to inference.
- HardDeleteAttachment has no production caller; not wired.
No outbox row is ever deleted on subject death. That was my first
instinct and it was wrong: it trades a real durability guarantee for a
partial privacy one, through the privacy door.
* fix(store): make the attachment.removed gate symmetric with attachment.added (TASK-2658)
Codex round 8, and it falsified a claim I had written into the code as
verified one commit earlier.
I checked that never-attached implies never-announced — true, and the
verification stands: no path sets attachments.item_id back to NULL, and
every birth path producing a NULL item_id is non-emitting. Then I stated
the conclusion for BOTH directions, which does not follow. Rows reach
ClaimSoftDeletedAttachment having never emitted attachment.added by at
least three routes: VARIANTS (written silently because they carry a
parent, then tombstoned by their original's cascade), attachments cloned
by a cross-workspace copy, and attachments created by workspace import.
So the path announced removals for subjects no consumer had ever seen.
The emit now carries the SAME gate as attachment.added — a user-visible
original, attached to an item — so the two are symmetric by construction
rather than by argument. That closes the variant route, which is the
systematic one, and the test asserts the premise (the variant emitted
nothing on creation) before asserting the conclusion.
Residue, stated rather than papered over: an import- or copy-created
attachment still passes the gate while never having announced itself. The
failure mode is noise rather than harm — an unknown id in a delete is
ignorable, where announced-but-never-retracted would leave stale state —
and the cause is the deliberate silence of the import and copy paths.
Round 8 returned CLEAN on the ref-only payloads, the transaction wrapping
(contractually — it does broaden the SQLite writer-lock window, which is
inherent to making the delete and the emit atomic), scrubItemPII, and the
prior_status pointer.
* fix(store): derive subject_kind from the taxonomy instead of trusting the caller (TASK-2658)
Codex round 9, run explicitly as a convergence round — asked to find what
eight rounds would systematically miss rather than to re-check what they
covered. It found this, which is a fair answer to that question.
writeOutboxTx derived subject_kind only when the caller left it blank, so
a non-empty value was taken as given. subject_kind is a pure function of
the event name: a caller-supplied value can only agree with the taxonomy
or be wrong, and a wrong one persists silently and misroutes the event at
drain time — item.created stored as subject_kind "comment" would be routed
as a comment. Every existing test passed either the correct value or none,
which is exactly the blind spot that lets a defect survive review rounds
aimed elsewhere.
Now derived unconditionally. A caller that supplied a DIFFERENT kind
believes something false about the taxonomy, so that is an error rather
than a silent overwrite: correcting the row quietly would fix one write
and leave the belief in place.
* fix(store): stamp occurred_at rather than accepting it, and enumerate the rest of the class (TASK-2658)
Round 9 found that subject_kind was caller-trusted. Rather than fix the
named instance and wait for a review to name the next one (CONVE-18), I
enumerated the class: of the eight fields on OutboxEvent, event_type is
validated against the closed set, payload is validated as non-empty JSON,
hop is bounded, subject_kind is now derived, and id defaults but fails
LOUDLY on a duplicate. occurred_at was the remaining member with the same
shape of silent harm — SPEC-3 pins time-relative `within` predicates to
it, so a supplied value quietly changes how a predicate evaluates. It is
now stamped at write time; no caller sets it, and "the moment the event
was written" is the only honest value while the write is transactional
with the mutation.
That leaves workspace_id and subject_id as genuine caller inputs. Neither
is derivable, both are checked at their own call sites, and the bulk
emitter partitions by member workspace rather than trusting the one it is
handed. The enumeration is in the code so the next reader does not redo it.
* refactor(store): payload families, an honest helper name, proportionate comments (TASK-2658)
Codex round 10, run as a maintainability convergence round — read the diff
as someone who has to live with it for two years and did not write it.
Three findings, all fair.
PAYLOAD FAMILIES. The emitter helpers take an arbitrary event name and
writeOutboxTx validated only canonical MEMBERSHIP — so a caller could pair
item.created with a ref-only deletion payload and the write would be
accepted, having validated the half that was already obviously correct.
Each canonical event now declares its payload shape in the taxonomy, every
emit site declares what it marshalled, and the two are checked against each
other. The declaration is write-side only and never stored: the event name
already determines the shape, and persisting it would create a second
source of truth that could disagree with the first. A test walks the
canonical set so the two maps cannot drift.
HONEST NAME. itemSnapshotsTx is now outboxMemberSnapshotsTx, because it is
not a general "read these items" helper: it de-duplicates, silently skips
rows that no longer resolve, and scrubs assignee identity. Any of those
makes a general-purpose caller's result quietly incomplete rather than
wrong-looking, and the old name invited exactly that reuse.
PROPORTIONATE COMMENTS. Every canonical event now carries compact contract
documentation — comment.*, member.joined and pack.* had none, and pack.*
now says plainly that nothing emits it yet so a reader does not hunt for a
producer. In the other direction, three comments that had grown into
accounts of how I got something wrong are trimmed to the invariant and the
counterexample. The process belongs on the task trail and the identity
doc; the code should carry what is true.
* fix(kernelevents): one taxonomy table — round 10's family map could fail open (TASK-2658)
Codex round 11 BLOCKED on a defect round 10 introduced, which is the
review loop doing exactly what my own rule says it should: when a fix
introduces a mechanism, the mechanism needs the next round's attention
more than the original bug did.
The defect: writeOutboxTx discarded the ok from PayloadFamily. A canonical
event missing from the separate family map would resolve to the empty
family — which a caller declaring nothing then MATCHES. The check would
pass precisely when it had no idea what the answer should be, and the two
maps keyed on the same names were free to drift into that state.
Fixed structurally rather than by adding the missing ok test: subject kind
and payload family now live in ONE canonical table entry per event. A
second map is a second source of truth; co-locating makes the drift
unrepresentable instead of tested-for, and the compiler requires both
fields so a new event cannot arrive half-declared.
The fail-closed arm stays as a guard for a future table that separates
them again, and its comment says plainly that it is UNREACHABLE today —
verified by mutation: disabling it changes no test, because the mismatch
check catches every reachable case. A guard whose comment implies it is
the protection, when something else is doing the work, is the kind of
claim this unit has cost me several times.
The test now checks both directions: every canonical event resolves a
subject kind AND a family, and a non-canonical name resolves neither —
the second leg being the one that matters, since an unknown name must
report ok=false rather than an empty string a caller would match.
|
||
|
xarmian
|
402f79e016 |
feat(store,server,web): collection kernel traits — de-hardcode conventions/playbooks slugs (TASK-2657, BUG-2702) (#1171)
Implements SPEC-5 §Collection traits (approved v1.1) — the first unit of
PLAN-2656 phase 0. Three kernel behaviors were keyed on the literal collection
slugs "conventions" and "playbooks": what the agent bootstrap loads, which
items route by invocation slug, and which items export as portable artifacts.
Collections now DECLARE those behaviors and the kernel resolves them from the
declarations.
Fixes the KERNEL half of BUG-2702, which stays open for the rest (see below).
A slug is not a stable identifier —
UpdateCollection re-slugs on any name change, and renaming a collection is a
documented onboarding step (TASK-1510) — so renaming either collection silently
detached all three behaviors from it, with the items still present and no error
anywhere. Measured on origin/main before the fix: conventions and
convention_index dropped 1 -> 0, playbooks 1 -> 0, and GET /playbooks/{slug}
went 200 -> 404, so `/pad ship` stopped resolving with no sign the playbook
still existed. Both halves are locked by regression tests observed failing on
unfixed code.
BUG-2702 is NOT fully closed here, deliberately. Every kernel behavior follows
the trait, and library activation on the MCP dispatcher and CLI was converted
too — but the pack's own dedicated web routes (/conventions, /playbooks list and
detail, /library) still address their collection by literal slug and render
empty after a rename. Filed as BUG-2705 with the route paths and the likely fix
shape; 2702 closes when that lands. Degradation there is bounded: no data loss,
and the collection stays usable at its own /[collection] route and in the
sidebar.
SPEC-5 was amended to v1.1 BEFORE any code, per the spec tree's own discipline:
bootstrap_include becomes a LIST of {mode, filter, key} because v1.0 could not
express convention_index at all; the conventions filter is now normative and
includes status=active, which v1.0's shorthand omitted and which the
implementation does enforce (implementing v1.0 literally would have leaked
draft conventions into every agent's boot payload); v1 filters are field-
equality maps with query/1 named as the widening path, since SPEC-2 is phase 1
and PLAN-2656 forbids growing toward it; and invocation_field is constrained to
the literal `invocation_slug`, because any other field name falls outside the
partial unique indexes in migrations/054 and pgmigrations/033 that are the real
uniqueness guard.
Traits get their own column rather than a key inside the schema JSON. The
schema column is overwritten wholesale on update and every client rebuilds it
fields-only, so a traits key stored there is destroyed by one ordinary
collection edit — measured during this task, not assumed. Trait authority
cannot rest on a value an unrelated UI save deletes. UpdateCollection writes
traits only when explicitly supplied, so pre-existing clients leave them alone;
an explicit "{}" still clears.
Bootstrap keeps its three payload keys as first-party views fed from the
declarations, and gains a generic bootstrap_includes array for any other
declared key — so the boot surface is genuinely generic rather than three
hardcoded payloads, and no consumer breaks.
Existing workspaces are backfilled slug-keyed in both dialects, guarded on
traits='{}' so a re-run cannot clobber a workspace's own declarations. The
backfill inherits today's blind spot (a workspace that renamed the collection
before upgrading is not reached) but cannot do worse than the status quo, which
is itself slug-keyed; from the backfill forward the hazard is structurally gone.
Malformed declarations are refused at create and update rather than stored:
an unparseable blob degrades to "declares nothing", which is silently the wrong
behavior instead of a loud error (SPEC-0 L6).
Web groups agent-facing collections by bootstrap_include presence, replacing a
hardcoded two-slug array repeated at five call sites.
Not done, deliberately: no MCP catalog change (traits are first-party kernel
declarations, no agent needs to set them, and the separate column means
pad_collection.update passes through harmlessly — no ToolSurfaceVersion bump);
bootstrap's collections[] projection does not carry traits (PLAN-1410 trimmed
that payload and nothing consumes them there); prefix.go's NormalizeSlug is
untouched (a pure function with no workspace context, and de-hardcoding two of
its six slugs would make it less coherent, not more).
Eight Codex review rounds found nineteen real defects, all fixed here. The
serious one:
bootstrap_include filter keys FAIL OPEN. The item store's field-filter path
drops any key its sanitizer rejects, removing the predicate rather than matching
nothing, so a declaration filtering on `"stat us"` would narrow nothing and ship
every convention — drafts included — to every agent at boot, defeating the
status=active guarantee this change makes normative. Filter keys are now
validated against the store's own sanitizer shape and pinned by a cross-package
agreement test, since models cannot import store and a future divergence would
silently reopen it. SPEC-5 amended to v1.2 with the rule and its fail-open /
fail-closed asymmetry. Also fixed: an unknown declared artifact_kind reached
artifact.Encode and surfaced as a 500 (now a 400 at the export boundary, since
SPEC-5 permits unknown kinds as legal non-round-tripping declarations); and
workspace import validated traits as JSON only, so an archive could persist a
declaration that degrades to "declares nothing" (now validated, degrading to
"{}" with a warning rather than refusing an import that may be the only copy).
Later rounds found more, and several were defects this change itself created.
A hidden collection could SHADOW a visible one: resolution used to name exactly
one collection, so with several declaring, resolving across all of them and
rejecting afterwards on visibility made a visible playbook unreachable behind a
hidden one — candidates are now filtered by visibility before selection, in both
playbook resolution and artifact import. Importing a pre-traits archive produced
an INERT workspace: the migration backfill cannot reach rows inserted long after
it ran, so conventions/playbooks arrived declaring nothing, and canonical
declarations are now inferred from the slug when a collection declares none
(never overriding declarations that survived the round trip). The generic
include path had no L4 boot budget and is now capped with an overflow count.
Trait parsing claimed to be strict but json.Decoder ignores trailing bytes, so
`{...} garbage` parsed cleanly. First-party payload keys are now mode-pinned,
since their projections have fixed shapes and declaring the other mode would be
silently ignored. Duplicate artifact_kind / invocation_field declarations are
refused at the collection API, and a conflicting archive warns on import.
Agent-facing text was updated with the rest, not after it: SKILL.md,
instructions.md and the MCP catalog said the literal slugs, which is exactly the
artifact an agent acts on. ToolSurfaceVersion 0.24 -> 0.25 for the
pad_library.activate behaviour change.
Trait uniqueness is a documented BEST-EFFORT gate, not an invariant, by lead
ruling. The gate reads then writes without a lock, import bypasses it, and a
rename can mint a duplicate without touching that path. The database-level
enforcement (partial unique indexes on the extracted traits) cannot ship first:
existing deployments can already hold duplicates via rename-then-reseed, so the
index would fail the migration on precisely the databases that most need
repairing. TASK-2710 carries the de-duplication pass and the indexes; SPEC-5
v1.3 records the deferral and the reason. L6's requirement that conflicts fail
loud is met by the refusal plus the warning — the mechanism is deferred, the
principle is not.
Gates: build · make lint 0 issues · go test ./internal/... · make test-pg ·
svelte-check 0 errors · vitest 99 files / 1734 tests. Mutation-verified across
four matrices, 20 mutations, 19 caught; the survivor is a seeding path whose
trait-vs-slug difference is unreachable today (SeedCollectionsFromTemplate
creates any missing template collection before it seeds items), recorded on the
task trail rather than papered over with a test that proves nothing.
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
|
||
|
xarmian
|
25c7cd20f5 |
feat(watchevents): Redis-backed bus so watch notifications cross instances (BUG-2651) (#1167)
* feat(watchevents): Redis-backed bus so watch notifications cross instances (BUG-2651) internal/watchevents shipped MemoryBus only, so in a multi-instance deployment a notification published on instance A never reached a stream held open on instance B — watches appeared to work and silently dropped. Bus was an interface from day one for exactly this; adding RedisBus changed no producer and no consumer. NOT A MECHANICAL PORT of internal/events.RedisBus. Three deliberate divergences, each documented at the point someone diffing the two files would call it a mistake: - ONE channel and ONE replay buffer, because this package has exactly one logical stream by contract (DOC-2479 DR-2: all per-caller filtering happens in the consumer). Most of the template's bookkeeping — per- workspace counts, subscriptions, buffers — has nothing to key on here. - EAGER subscription for the bus's lifetime, not lazily on first local subscriber. The replay buffer fills from the RECEIVE path, so a lazily torn-down subscription stops filling it at precisely the moment before a Last-Event-ID resume — for one harness monitor holding one stream, that makes resume structurally useless. The template can afford lazy because per-workspace means N idle subscriptions; here it is one. - ONE mutex across subscriber membership and the replay buffer, held through the whole local fan-out. The template uses two and offers only separate Subscribe + EventsSince, which cannot provide SubscribeAndReplaySince's guarantee. Copying its locking would have handed back the double-delivery window this package's interface exists to close. Publish fails CLOSED when INCR fails, where the template falls back to a local counter. Two instances falling back at once mint ids from independent counters into a shared stream, and replayBuffer.since() reasons on monotonicity — so the damage is silent replay corruption, not a visible error. INCR and PUBLISH share a connection anyway, so the fallback mostly lets a doomed publish proceed carrying a poisoned id. Both load-bearing tests were VACUOUS as first written; the mutation matrix is the only reason I know: - the concurrency test's producer finished before the subscriber joined, so the channel leg was never exercised and a split-lock mutant survived 50 iterations. Now paced, with a both-legs-non-empty precondition that fails a run which never approached the boundary, plus a dedicated detector (600 attempts, 8/8 kills, 0.02s after switching the drain to non-blocking — exact, because the duplicate is already buffered when the call returns). - the fail-closed test asserted nothing was delivered, which is true of the fallback too: Publish never delivers locally, so with Redis down neither policy delivers. Rewritten around a go-redis ProcessHook that records attempted commands, which is where the policies actually differ (INCR-then-stop vs INCR-then-PUBLISH). Also corrects session_presence.go, which told the next person these two had to be fixed together. Delivery is now cross-instance; the registry's under-report is unchanged, so the remaining defect is a picker that under-reports rather than a push that lies. The PLAN-2558 S3 gate stays, for that reason instead of the old one. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents): make id assignment and publish atomic; close the bus on shutdown (Codex round 1) P1 — INCR and PUBLISH as two client calls are not order-preserving, and the failure is concrete: A gets id 1 and is descheduled, B gets id 2 and publishes, A publishes 1. Every subscriber receives 2 before 1, the replay buffer appends in ARRIVAL order, and replayBuffer.since() reasons on monotonicity — so a resume from 2 hits the sinceID > newestID branch and answers 'gap too large', turning a healthy reconnect into a spurious sync_required, while a resume from 1 silently skips the late arrival. Fixed at the source with a Lua script: Redis runs it atomically on its single thread, so INCR and PUBLISH for one instance both complete before another's script begins, and publish order equals id order globally with no coordination on our side. The id rides as a '<id>|<json>' prefix rather than being edited into the JSON from Lua; the id is digits and the FIRST '|' separates, so a '|' in the body is unambiguous. A pleasant consequence: there is no longer a window where an id exists but the publish has not happened, so the fail-closed decision and the publish decision became the same decision. P2 — Stop() never closed the watch bus. That was survivable for MemoryBus, whose Close only drops channels; RedisBus holds a receive goroutine and a Redis subscription from construction, so it leaked both for the process's life. Closed after bg.Wait(), so a background producer cannot publish into a bus already tearing down. nits, all real, all in artifacts someone reads: - 'exactly-once delivery' was simply wrong. Redis pub/sub is at-most-once and the local send is deliberately non-blocking. The property the round trip actually buys is NO DOUBLE DELIVERY to the publishing instance; the comment now says that and names the replay buffer as the bounded recovery mechanism for the rest. - the Bus interface comment still said only MemoryBus existed. - cmd_server.go's session-presence note still claimed the same caveat as 'the watch bus directly above', which had just stopped applying. - session_presence.go now says delivery is fixed WHEN PAD_REDIS_URL is set, rather than unconditionally. Tests: the fail-closed assertion moved from 'nothing was delivered' — still true under the two-call version — to 'no bare INCR or PUBLISH was issued', which is what distinguishes atomic from not. Mutation-verified by splitting the script back into two calls. Added a decode round-trip test covering the new wire format, a '|' inside the body, and four malformed payloads, since that decoder consumes bytes from a channel any holder of the Redis credentials can publish to. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents,server): correct the targeted-push claim; close the bus before HTTP shutdown (Codex round 2) P2 — I claimed cross-instance DELIVERY was fixed. Half true, and the false half was mine to catch: handlers_push.go gates a session-targeted push on the LOCAL presence registry and skips the publish entirely when the id is not there, so a POST landing on A for a session held on B still delivers nothing. The bus would carry it; the gate means it never reaches the bus. Broadcast pushes and every other notification kind ARE fixed. I asserted that behaviour from reading the bus and session_presence.go without reading the push handler — the exact thing I hold myself to not doing. Corrected in all three places the claim was made (the package doc, session_presence.go, and the KindPush comment), with the correction recorded rather than quietly overwritten. The gate's own justification is now stale too, and worth more than a tweak: 'a target this instance cannot see is a guaranteed no-op' was TRUE under MemoryBus and is FALSE under RedisBus, where another instance may hold that session. Left in place deliberately — publishing unconditionally would fix delivery and immediately make delivered_sessions=0 a lie in the other direction, which is a question about what that field promises. It belongs with the shared-state SessionPresence that PLAN-2558 S3 already gates on: fixing the registry makes the snapshot right, and then the skip is correct again for its original reason. Both open halves collapse into that one implementation. P2 — the watch bus was closed only in Server.Stop(), which runs AFTER http.Server.Shutdown. The event bus is closed before Shutdown precisely so its SSE handlers unblock; the watch stream is the same shape, so an open one would have held Shutdown to its full 30s deadline. Now closed alongside eventBus, with the Stop() close kept as the path for other callers — both implementations are idempotent. nit — MemoryBus and RedisBus disagreed after Close: RedisBus handed a late Subscribe an already-closed channel, MemoryBus registered one nobody would ever close, so a consumer racing shutdown blocked forever. MemoryBus now matches, and its Close is idempotent, which the CLI's double close relies on. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents): report a missed notification as a replay gap (Codex round 3) P2 — a divergence MemoryBus structurally cannot have. It assigns every id itself, so its replay buffer is contiguous and the only gap it can report is eviction. RedisBus receives ids over at-most-once pub/sub, so a blipped subscription can miss 101 and receive 102: the buffer holds a hole, is nowhere near full, and replayBuffer.since() answers a resume from 100 with just [102]. The consumer loses a nudge and is never told. RedisBus now tracks the id at which the sequence resumed after the most recent hole, and answers nil — the same signal eviction already gives, which the SSE handler already turns into sync_required — for a resume that would have to span it. Resumes that do not span it still replay normally, and sinceID=0 is treated as a fresh subscriber rather than a resume, so a hole nobody spanned is not turned into a spurious resync. The atomic publish script is what makes this readable: publish order is id order globally, so a non-consecutive id means MISSED, not reordered. Mutation-verified by disabling the check; the test fails on both the spanning resumes and would have failed the over-broad version too (it asserts the non-spanning resumes still work). Two residuals documented rather than fixed, both because the fix is the same shared-state SessionPresence that PLAN-2558 S3 gates on: - delivered_sessions is now wrong in BOTH directions for a broadcast push — the count is local while delivery is global, so a replica can report 1 while two sessions receive it, or 0 while a remote one does. No local arithmetic fixes that; it is asking one replica what all of them are doing. - the Redis channel and counter names are not deployment-scoped, so two installations sharing a Redis endpoint cross-feed (and picking different logical DBs does not help — pub/sub ignores them). Left flat to match internal/events rather than giving one of the two buses a prefix the other lacks; the rule is one Redis endpoint per installation, and relaxing it should cover both buses at once. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents): a cold-started replica must report a gap too (Codex round 4) P1 — the round-3 hole check only fired BETWEEN two received messages, so it never fired for the first one. A replica restarting while Redis is already at 101 has an empty buffer; its first received message is 102, nothing looks like a hole, and a client reconnecting to that replica with Last-Event-ID 100 was handed [102] — skipping 101 exactly as silently as the case round 3 fixed, by a different route. Replaced contiguousFrom with knownFrom: the lowest id from which this instance's buffer is contiguous. SET on the first append (before which this instance knows nothing) and RESET on every hole (before which it no longer knows anything usable). One variable, both failures. The boundary is pinned in both directions, which is what stops this being an over-broad 'always gap after a restart': a resume from exactly the id before our first (101 when we started at 102) IS contiguous with our view and replays normally. Mutation-verified by disabling the cold-start arm. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents): idempotent publish, confirmed subscription, and real Redis tests (Codex round 5) P2 — go-redis retries a command whose reply is lost to a network error, and the publish script was not idempotent: the same notification would be published twice under two different ids. Both copies look valid — ordered, distinct — so nothing downstream could tell them apart, and on the push path a duplicate is a duplicate DISPATCH into an agent harness. The script now takes a caller-generated token and SET NX's it, so a retry carrying the same arguments returns 0 without publishing. TWO THINGS THIS UNIT OWES ITS TESTS, both found within minutes of each other and both invisible to the hermetic ones: 1. The idempotency script shipped indexing ARGV[3] while Publish passed two arguments. Caught by re-reading, which is not a control worth relying on for the next Lua edit. 2. NewRedisBus returned before go-redis had established the subscription, so notifications published in that window were lost to this instance, silently. Surfaced as a test flake; the production shape is a rolling deploy, where a replica takes traffic before its subscription is live. The constructor now waits for the confirmation (bounded, and a failure is logged rather than fatal since Channel() re-subscribes on reconnect). So miniredis is now a test dependency, and the round-trip tests it enables cover what fanOutLocally-driven tests structurally cannot: the channel name, the KEYS/ARGV mapping, the id prefix wire format, the shared counter across two buses, cross-instance delivery (the actual bug), the dedupe token, and Close tearing down the SERVER-side subscription rather than just local channels. Verified by restoring the ARGV[3] bug: the round-trip test fails on it. The two findings I am NOT fixing here are unchanged and documented where the reasoning is met — the targeted-push gate and delivered_sessions are both consequences of the per-process presence registry, and both are closed by the shared-state SessionPresence that PLAN-2558 S3 gates on, not by anything in this package. make vuln: 0 vulnerabilities in imported packages. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents): survive a Redis counter reset without replaying stale ids (Codex round 6) P2 — pad:watchevents_seq has no TTL but can still vanish: evicted under maxmemory, dropped by a FLUSHDB, or restored from an older snapshot. Ids then restart at 1 while this instance's ring still holds the hundreds. Keeping both is what corrupts replay — the two id spaces are not comparable, so a resume from 2 in the NEW space would be handed the stale 99/100/101 as though they were newer. A backwards id now drops the replay buffer and re-anchors knownFrom. Every resume from the old space then exceeds the newest id held and gets nil — the resync signal that is the only honest answer once the ids stopped meaning what the client thinks they mean — while clients in the new space keep working immediately. The test asserts BOTH halves, which is what makes it a detector rather than a description: a build that logged the reset and kept the buffer passes 'the old resume reports a gap' and fails 'the new resume never returns a pre-reset entry'. Mutation-verified on exactly that. Hardened while I was here: the epoch-reset path REBUILDS the buffer at runtime, so a bus constructed with a non-positive replay size would have turned a counter reset into a panic (newReplayBuffer(0)'s first append indexes a zero-length slice) rather than a resync. The constructor now normalizes. MemoryBus has the same trap for a caller passing 0; left alone as pre-existing and off this path, but named in the comment rather than silently fixed or silently ignored. nit — this file's header still claimed there was no miniredis dependency and no round-trip coverage, which the previous commit made false. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(watchevents): actually correct the hermetic test header (Codex round 7) The previous commit's message claimed this fix. It did not contain it: the edit ran as one of two scripts in a single command, its assertion failed with a traceback, and the second script's success is what I read. The header kept saying there was no miniredis dependency and no round-trip coverage — both false since two commits ago, in the file a reader consults to find out what IS covered. That is the adjacent-success-signal failure exactly: a success line from the step next to the one I cared about. The tell was in the output and I walked past it, then asserted the change in a commit message. Recording it here rather than quietly fixing, because a commit that claims a change it does not make is worse than one that omits it. Verified this time by reading the file back and grepping for the stale phrases: zero. Round 7's other three findings are the documented residuals re-raised for the third time — the targeted-push gate, delivered_sessions, and the unnamespaced Redis keys. All three are dispositioned at the line a reader meets them, all three are consequences of the per-process SessionPresence registry or of matching internal/events' existing convention, and none is fixable inside this package. They stay open, on the record, and with the lead. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(watchevents,cli): correct pad push --help; document the reset-window residual (Codex round 8) nit, and the one that stings — cmd_push.go's Long help still said pushes go over the 'in-memory watch-events bus'. That is the text a user reads when they run pad push --help, and it has been false since this branch's first commit. I have a standing pre-push step to grep the artifacts a CONSUMER reads for exactly this, and I ran it as a code search (watchevents.New) rather than a prose search, so --help never came up. The help now distinguishes broadcast (reaches every instance) from session-targeted (still resolved against the handling server) and names the bug. P2 — the counter-reset handling fires when the first post-reset notification ARRIVES, so there is a window between Redis losing the counter and the next publish in which this instance still replays old ids to a reconnecting client. Documented as accepted rather than closed: nothing local can detect the reset earlier (the counter is in Redis and we learn of it by receiving something), and the two shapes that would — a GET per resume, or a background poller — put network I/O on a latency-sensitive path or spend a goroutine and a round trip per tick forever against a condition measured in years. The exposure is redelivery of notifications the client already has, bounded by the window and self-healing on the next publish. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents): a replica that has received nothing must not answer 'caught up' (Codex round 9) P1 — the coverage check was skipped entirely while knownFrom was still 0, so a bus that had received NOTHING answered any cursor with an empty-but-non-nil replay, which the SSE handler reads as caught-up. The scenario is a restart, not an exotic one: replica B comes up while Redis is at 100, id 101 is published before B's subscription is live, and a client reconnects to B with Last-Event-ID 100 before 102 arrives. B says caught-up, then delivers 102 live, and 101 is gone with nothing to tell anyone. The principle the code now follows: having received nothing is strictly LESS knowledge than 'contiguous from X', so it must produce at least as strong a signal. A non-zero cursor against an empty bus is a gap. Both sides pinned, because the over-broad version is a real risk here — answering every fresh connection with a resync would be its own bug. A sinceID of 0 is not a resume and still gets an empty replay rather than a gap. Mutation-verified on the new arm. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(watchevents,cli): name the trailing-gap and shutdown trades (Codex round 10) Two findings that are decisions rather than defects, so both are documented at the line where the reasoning is met and taken to the plan instead of being settled unilaterally after ten review rounds. P1 as reported — the TRAILING gap. Everything the coverage bookkeeping does reasons about what this instance HAS received; it cannot see a notification missed at the END of the sequence. Hold 100, miss 101 to a disconnect, and a client resuming from 100 before 102 arrives is told caught-up. The hole only becomes visible when 102 lands, which is too late for that connection. What would reveal it is a GET of the sequence key: a value above lastAppendedID means ids exist we never saw, and a value BELOW it reveals the counter reset documented last round — one mechanism, both open windows. It is not done here because it is product-visible in the other direction: INCR happens before the message propagates, so the counter legitimately runs ahead of every instance for microseconds after each publish, and a strict comparison turns ordinary in-flight traffic into spurious sync_required responses with no principled tolerance to pick. A resync is recoverable and a lost nudge is not, which is the argument for doing it — but that is a call about how chatty the resync path should be. P2 — closing the watch bus before Shutdown drains handlers means a push already in flight can publish into a closed bus and still return 200 with pushed:true. Closing after would instead hold every shutdown to its 30s deadline on any open stream. eventBus already makes the same trade the same way; naming it rather than inheriting it silently. The honest fix is Bus.Publish reporting the drop so the handler can, which is an interface change and a different unit. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * feat(watchevents): close the trailing gap with a settle-window authority check (lead ruling) Lead's ruling on BUG-2651: a silently lost nudge is unbounded staleness, a spurious resync costs one redundant fetch, so the gap must not survive — and don't pick a magnitude tolerance, because the reason the counter legitimately runs ahead is in-flight propagation, which is TIME-bounded while a genuinely missed message never arrives. So the discriminator is time. On a resume (and only on a resume), read the shared counter: if it disagrees with this instance's high-water mark, wait out one settle window and read again. In-flight ids land during the beat and the resume proceeds normally; missed ones never do and the resume is answered with a gap. That converts an unprincipled 'how many ids behind is too many' threshold into a principled propagation bound. The same read also catches the counter having gone BACKWARDS, so the counter-reset window documented last round is closed by the same mechanism rather than needing its own — the arrival-time reset handling stays, because it is what repairs the instance's own state and what covers a bus with no reconnecting clients. Ordering matters and is documented at the call: the check runs WITHOUT the mutex (it sleeps and does network I/O, neither of which may happen inside the lock fan-out needs) and BEFORE subscribing rather than between subscribe and replay, which would reopen the double-delivery window SubscribeAndReplaySince exists to close. Nothing is lost by waiting first — fanOutLocally buffers regardless of subscribers. An unreadable counter falls back to local knowledge rather than failing closed: turning a Redis hiccup into a resync for every reconnecting client at once is a worse failure than the one being guarded against. EventsSince deliberately does NOT do this and says so — it is the local primitive the Bus interface already describes as being for tests and non-resuming callers, and making it sleep and hit the network would surprise every one of them. Five tests, each pinning a different half: the missed tail reports a gap; a current instance does NOT (the control that stops this being 'always resync'); an id arriving mid-settle is tolerated; an unreadable counter falls back; a fresh subscriber neither waits nor gets a gap. Mutation-verified twice — disabling the check, and removing the settle beat — each killed by the test that names it. Also filed at the lead's direction, so the two remaining cross-instance defects have tracked homes rather than only comments: BUG-2698 (targeted push resolved against local presence, plus the delivered_sessions inaccuracy — one shared-state SessionPresence closes both) and BUG-2699 (push returns 200 pushed:true for a dropped publish; Bus.Publish reports nothing, and fixing it is an interface change). Every disposition comment now cites its item. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents): compare two FRESH reads, not one stale snapshot (Codex round 11) P1 — the settle beat re-read only the local side, so the comparison was against a counter SNAPSHOT taken before the wait. Id 2 arrives during the beat while id 3 is published and missed: the stale remote is still 2, the check declares convergence, and 3 is silently lost — the exact failure this whole mechanism exists to prevent, reintroduced inside it. P2 — the same staleness in the other direction. A GET can land just before a publish completes and report a value BELOW what this instance already holds; that never matches, so a client who had missed nothing got a full resync. Both are one defect: agreement between the authority and this instance has to be evaluated on two FRESH reads or it is not agreement. Now re-reads both sides after the beat, and treats any remaining disagreement as a gap in either direction — still behind means ids never reached us, still ahead means the counter was reset under us and our buffer belongs to a dead id space. Two tests, one per direction, each mutation-verified against the re-read-locally-only version: the second counter advance must produce a gap, and the raced read must NOT produce a resync. Without the second test the fix could have been 'always report a gap', which passes the first. Documented the cost side of the lead's ruling while I was in here: the condition is agreement, so a resume during CONTINUOUS publishing across the whole settle window can disagree every time and resync. Bounded by this stream being low-volume by design and resumes only happening on reconnect; if a workload makes it chatty, the answer is a longer window, not a magnitude threshold. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(watchevents): an absent sequence key is zero, not unreadable (Codex round 12) P2 — the counter key can DISAPPEAR after this bus has seen ids (FLUSHDB, eviction). Reading redis.Nil as 'unreadable' meant falling back to local knowledge and cheerfully replaying an id space the authority no longer has — while the next publish starts again at 1 and collides with it. Absent is a VALUE. Returning zero-and-readable makes the case fall out of the ordinary comparison with no special branch: an instance holding 101 disagrees with an authority at 0, does not converge, and the resume is answered with a gap. A genuinely fresh deployment still agrees at zero and is not resynced — which is the control leg, and the reason 'absent means gap' would have been the wrong fix: it passes the first test while resyncing every first connection on a new install. P1 as reported — the equality fast path returning without settling — is not closed, and the comment now says why rather than leaving it to be re-found. A notification published AFTER that read and missed by this instance is invisible to any check made here, and settling anyway would not close it: the same race exists in the instant after the function returns. The check's honest scope is what was missed BEFORE the resume. A message missed after it is a property of at-most-once pub/sub with no per-connection ack, and the real answer is a durable stream (Redis Streams with consumer groups), not a longer wait. Mutation-verified: restoring redis.Nil to the unreadable branch fails the disappearing-counter test. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * feat(watchevents): epoch marker, so a reset that caught up is still a reset (Codex round 13) P2 — numeric detection is blind to a reset that has already climbed past this instance's high-water mark. Hold 100, lose the connection, the counter resets and ids 1-101 are published, and the only one that reaches us is 101 — the perfect contiguous successor of 100. Every arithmetic check passes, the buffer quietly mixes two id spaces, and a client resuming from OLD 100 is handed NEW 101 having silently missed the new space's 1-100. No amount of comparing numbers fixes that, because the question is not 'is this bigger' but 'is this the same sequence'. The publish script now mints an epoch once per id space (SET NX, so every publisher can offer one and the first wins) and carries it on every message; a change drops the buffer and re-anchors. The subtle half, and the one the first attempt got wrong: after an epoch change the cold-start rule must NOT admit its usual contiguous-with-our-view cursor. Within an epoch, a client at n.ID-1 is genuinely adjacent to our first id. Across one it is ambiguous — id spaces overlap, so that cursor may be the OLD sequence's n.ID-1, a different notification entirely — and admitting it hands them the new epoch's id as though it followed theirs, which is exactly the failure the epoch exists to prevent. Letting it back in one line later would have been a poor joke. The test caught it; the control leg (a cursor genuinely inside the new epoch is still served) is what stops the fix becoming 'resync everyone forever after any reset'. Wire format changed to <epoch>|<id>|<json>. Free of compat cost, checked rather than assumed: redis_bus.go does not exist on origin/main, so no released build produces or consumes the old shape. The numeric backward check stays — it covers a counter reset where the epoch key survived (eviction picks keys individually), and it is what repairs an instance with no reconnecting clients at all. Mutation-verified: ignoring the epoch change fails the new test. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(watchevents): the wire format comments say <epoch>|<id>|<json> (Codex round 14) Three comments still described the pre-epoch format. Worth more than a tidy-up: a maintainer following them would conclude the epoch prefix is vestigial and remove it, which reintroduces exactly the cross-epoch replay corruption round 13 existed to fix. The publishScript comment now also says outright that the epoch is not decoration and points at redisWatchEpochKey before anyone considers it removable. Verified by grepping for the old shape rather than by trusting the edits — zero remaining, which is the check I owed after getting this wrong in round 7. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * chore(nix): update vendorHash for the miniredis test dependency (BUG-2651) CI's Nix job failed on a fixed-output hash mismatch, and it is neither a flake nor a surprise once seen: nix/package.nix pins the vendored module set, and adding miniredis (plus gopher-lua, its Lua interpreter) to go.mod changed it. Regenerated per the procedure the file itself documents — build and read the 'got:' line. Run on CI rather than locally because this box has no nix; the hash is a content hash of the module set determined by go.mod/go.sum, so the same inputs produce it in either place. Worth naming as a gate lesson rather than just fixing: my pre-merge matrix had build, lint, test, test-pg, vuln and Codex, and none of them can see this. A dependency change has a SEVENTH consumer — the Nix packaging — and the only thing that checks it is the CI job that just did. Adding a dependency means checking the packaging, not only the security scan. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V |
||
|
xarmian
|
449ac109e9 |
fix(server,mcp,cli): refuse system-metadata keys in fields_patch + retry-hostile error code (BUG-2627 part 2, BUG-2675) (#1166)
* fix(server,mcp,cli): refuse system-metadata keys in fields_patch + retry-hostile error code (BUG-2627 part 2, BUG-2675) Part 2 of BUG-2627 closes the door that mints the defect parts 1 and 3 dealt with: `--field implementation_notes=<json>` stored the entries as a JSON-ENCODED STRING, which is invisible to every reader and — since part 3's guard — disables `pad item note` on that item until the row is repaired. Refused SERVER-SIDE in `fields_patch`, not at the CLI as the item's scope line proposed. The deviation is deliberate and recorded on the trail: the CLI is one of three clients, and all three lower a user field-setter into the same key (`pad item update --field` at cmd_item.go, the MCP `field` param via dispatch_http_advanced.go on remote, and stdio by shelling out to that CLI). One gate closes all three; a CLI-only refusal would have left remote MCP writing the key. Both call sites were read, and the CLI's lowering is now pinned by a test rather than left as an assumption. Scope, stated because it is deliberate: this closes UPDATE only. The full `fields` blob stays open because that door is SHARED — `pad item note` / `decide` / `github link` send one, and so does convention activation via BuildConventionItemFields -> ItemCreate. Closing it would break the system writers the gate exists to protect. Item create therefore remains a mint site, tracked with the rest of that surface in BUG-2685. The refusal message is per-key: implementation_notes -> `pad item note`, decision_log -> `pad item decide`, github_pr -> the GitHub link flow, and `convention` refuses WITHOUT naming a command, because none writes it. PATTE-135 wants a remedy that works in the failing state; a single "use pad item note" line would have been wrong for three of the four keys. BUG-2675 rides along on one ToolSurfaceVersion bump, as ruled. The append refusal from part 3 reached MCP agents as `server_error` — not our fault, and not transient, so agents could reasonably retry a failure that is deterministic forever. New closed-set code `stored_state_unreadable`, emitted on BOTH transports: HTTP classifies the sentinel error directly, stdio via a `pad-structured-error/v1:` marker the CLI now writes for its own local refusal (the first marker generated without an upstream APIError). v0.16-then-v0.17 is what a one-transport fix costs. Also here: - items.ReservedOverrideKeys -> ReservedFieldKeysIn. The second caller passes a patch, not an override map, and the old doc comment said fields_patch was an open exposure — true until this commit. - `Extract* returns nil for THREE reasons` -> FOUR. The comment listed four; the count was corrected everywhere except the code. - Consumer-read artifacts updated where the claim is ACTED on, not only where it is documented: instructions.md (incl. a "do not retry this code" section), the catalog `field` param description, `pad item update --help`, README. Gates: build · make lint · go test ./... · make test-pg · Codex. Eleven-mutation matrix run against the new tests; every one killed by an assertion (two were rewritten after killing by compile error / surviving, which proves nothing). Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(server,mcp): honest remedy when the stored value is already unreadable; name the MCP-facing code (Codex round 1) Three findings from the pre-push review, all real: P2 — the refusal named `pad item note` unconditionally, but on an item whose stored value is ALREADY undecodable that command refuses too (part 3's guard). The caller was routed in a circle: field write refused -> run the note -> refused -> back again. That is exactly the failure PATTE-135 exists to prevent, and my own trail had reasoned the remedy was safe on the strength of the HEALTHY case only. The message now inspects the item's stored value and, when the key is unparseable, says so and points at the one action that works in that state (inspection), noting that the repair needs a full `fields` write no CLI flag exposes. P2 — two doc claims were false where an actor reads them. The catalog said reserved keys are refused "on every action that accepts field", which includes CREATE, and create is deliberately NOT gated; and both the catalog and instructions.md named `validation_error` (the HTTP code) where an MCP client actually receives `validation_failed`. Both corrected, and the create exception is now stated rather than implied by omission — an agent that reads only "refused on update" will otherwise assume create is fine, which is how a hole gets used. nit — the destructive-downstream sentence claimed every reserved key becomes unreadable and trips an append guard. True only for the two append-backed keys; github_pr and convention are simply overwritten. The clause is now per-key, because a confident wrong explanation is worse than a vague right one. Two more mutations run against the new branch: always-readable (the circular remedy returns) and never-readable (the working remedy disappears) — both killed by assertions. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(models,mcp,cli): one appendability predicate, per-key docs, stdio hint parity (Codex round 2) Five findings, all real. P2 — the message's readability check and the guard it describes were two different decodes. Mine unmarshalled into []json.RawMessage; the guard uses []ItemImplementationNote. A stored `[1]` passed mine and fails the guard, so the message would again have prescribed a command that refuses — the same circularity round 1 caught, through a narrower door. Replaced with models.StructuredFieldIsAppendable, which ASKS the guard rather than re-deriving it, plus an agreement test over 12 shapes x 2 keys that compares the predicate against the real Append* helpers. Verified by restoring the RawMessage version: the table catches it on `[1]`. P2 — stdio lost the new code's hint. Remote MCP told the agent retrying is pointless and how to inspect; stdio got the code with an empty hint, because the CLI's marker envelope carried none and the classifier parsed none. Both fixed, with the hint hoisted into paired constants (the same duplication StructuredErrorMarker already uses) and the test comparing the two TRANSPORTS' envelopes rather than either against a literal. P2 — doc text was still false for `convention`: the catalog, the instructions and `--help` all said reserved keys are maintained by note/decide/the GitHub flow, which is true of three of the four. Each key now names its own writer, and `convention` names library activation. Also dropped the `malformed_override` advertisement — that is the SERVER's code; an MCP client sees validation_failed for both refusals. nit — the classification test called structuredAppendErrorResult directly, so deleting either dispatcher call site left it green. Added dispatcher-level tests driving the real server + store, asserting the code, the hint, and that the item's stored fields are byte-identical afterwards. Mutation-verified by reverting the note call site. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(items,models,mcp): github_pr stays writable through fields_patch; no nil-map panic (Codex round 3) P1 — the gate refused `github_pr`, and that was wrong. My model was "system writers use the full fields blob, user setters use fields_patch", which holds for three of the four reserved keys and fails for this one: `pad github link` needs a local git checkout and the `gh` CLI, so it is excluded from remote MCP BY NAME, and internal/mcp/dispatch_http.go's noRemoteEquivalent map tells remote agents in so many words to use `item update --field github_pr=...` instead. For that audience the patch door is not a bypass of the writer — it IS the writer. So the refusal deleted a documented capability from remote agents, and answered with a message naming a command they cannot run: the same circular remedy round 1 caught, aimed this time at the people the gate was meant to help. items.PatchRefusedFieldKeysIn now exempts the key and records the rule being applied — refuse a raw write where a real writer exists — rather than the list it produces. Whether remote agents should get a proper PR-link action, so the key can be closed too, is a product question and is left as one. P2 — the hint told agents to read the bad value with `pad_item action=get`. They cannot: stripDuplicatedFieldsKeys removes implementation_notes and decision_log from every MCP response's fields blob, and the top-level arrays come from the extractor, which returns nil for exactly this shape. The value is invisible on the whole surface. The hint now says so and routes to a human, who can read it with `pad item show --format json`. P2 — `fields` holding a literal `null` unmarshals into a NIL map with no error, and both Append* helpers assign into what they get back, so `pad item note` PANICKED ("assignment to entry in nil map") instead of appending. Reproduced, fixed in parseMutableItemFields, and pinned by a test that fails on a panic rather than taking the process down. An absent blob and a null blob mean the same thing to every caller. Pre-existing, but it sits in the function family this bug is about and the message was about to recommend the command that panics. nit — README claimed a "closed eight-code taxonomy" (17 codes, and I had just added one) and read as if create lowers into fields_patch. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(models,mcp): predicate matches the append on malformed blobs; stop promising a broken workaround (Codex round 4) P1 — round 3 exempted `github_pr` from the update gate on the strength of noRemoteEquivalent's documented workaround. That workaround does not work: ingestFieldKVP (remote) and parseFieldFlag (CLI, and so stdio) both store a `field` value as a STRING, so the PR data lands double-encoded and no link appears — the BUG-2627 shape one key over. Filed as BUG-2696 with the three candidate fixes; NOT folded in, because the narrowest of them changes how every field value is typed. The exemption stands regardless: refusing would leave remote agents with strictly less than a broken door. What changes is what we may PROMISE. The catalog, instructions.md, version.go and README said "this is how you link a PR"; they now say the door is open and broken, and to hand PR linking to a human. Advertising a capability that isn't there is the failure mode this whole unit keeps circling. P2 — StructuredFieldIsAppendable returned TRUE when the whole fields blob was unparseable, on the reasoning that a broken outer blob is a different problem. True of the cause, irrelevant to the caller: the Append* helpers bail on that same parse, so the message again named a command that fails. It now returns false, which is simply the honest answer to the question asked, and the agreement table grew a malformed-outer-blob leg — the gap that let the disagreement through. P2 — the message claimed a raw field write always stores something Pad cannot read back. That holds for the CLI and MCP (a `--field` value is typed by schema lookup and these keys are in no schema) but not for a direct REST caller sending a valid array, who is refused for ownership reasons alone. Reworded to say both parts. nit — a misplaced parenthetical in the README read as if item CREATE lowers into fields_patch. It does not; it sends the full blob. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(mcp,models): stop the remote hint advertising the broken PR workaround; classify an unparseable blob as retry-hostile (Codex round 5) P1 — I corrected four artifacts that pointed agents at the github_pr field write and missed the fifth: noRemoteEquivalent's own text, which IS the message a remote agent receives when it calls `github link`, and which Codex had quoted at me in round 3 to establish the workaround existed. The nearest artifact to the actor was the one I did not open. Both entries now say there is no working remote path and name BUG-2696, with a test pinning the negative so a future edit cannot quietly reinstate the advice while the write is still broken. P2 — a fields blob that will not parse at all produced a bare parse error, so `note` / `decide` reached agents as `server_error`: transient- looking, and therefore retried, for a failure that is as deterministic as the per-key one BUG-2675 exists for. Both Append* helpers now wrap that parse failure in ErrStructuredFieldUnreadable, which both transports already classify, and the malformed-blob test asserts the sentinel rather than just an error. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(mcp,cli): qualify what an agent can actually see when the state is unreadable (Codex round 6 nit) Round 5 widened stored_state_unreadable to cover a fields blob that fails to parse outright, which made half of its own hint false: MCP's normalization strips a broken structured KEY (so `get` hides it), but leaves an unparseable BLOB as a raw string (so `get` shows it). The hint and instructions.md asserted the first case for both. Now stated per layer, in the two paired constants and the instructions. The reason it is worth the words rather than being cut: an agent told 'you cannot see this' does not look, and would have missed a value that was in fact right there in the response it already had. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(mcp): classify the move/copy reserved-key refusal as validation on stdio too (Codex round 7) P2 — carried over from v0.22, surfaced because THIS bump documents the two reserved-key refusals as agreeing across transports. The move/copy message ("Field(s) reserved for system metadata and not settable here") matched none of the stdio validation patterns, so the same deterministic 400 arrived as validation_failed on remote and server_error on stdio — and server_error reads as transient, so an agent retries a refusal that can never pass. One pattern added, plus a test that drives both real classifiers with the real server message text for both refusals, so a reworded message that stops matching fails here rather than in the field. nit — the github_pr exemption is UPDATE-only; move and copy still refuse it, because there the argument is BUG-2674's (an override reintroduces the key the migration just dropped), not this one's. The catalog and instructions said "not refused" without that qualifier. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(mcp): cover the copy path's own refusal wording in the stdio classifier (Codex round 8) P2 — round 7 fixed the MOVE wording; the copy path words the same class of refusal differently ("Destination collection has no field(s): ..."), so it kept arriving as server_error on stdio and validation_failed on remote. Third message in one family, and the round-7 test used the move text for every case, which is why it missed this. The parity table now carries all three real messages plus a control leg using one the pattern list already covered — without it the table could pass by matching everything. Recorded in the pattern list's comment rather than left implicit: matching prose is a stopgap, the structural fix is the pad-structured-error/v1 marker that carries the code instead of inferring it, and until a refusal emits one, this test is where a new wording has to be added. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * test(mcp): use the real upstream codes in the parity fixtures (Codex round 9 nit) The copy legs carried `validation_error` where the handlers actually emit `malformed_override` and `invalid_override`. The 400 branch ignores the body code today, so the test passed either way — which is exactly why the fixture mattered: it was quietly recording a wrong contract, and a future code-aware classifier would regress against a table that agrees with it. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(mcp): the upstream code is not forwarded to MCP clients (Codex round 10 nit) The catalog said the server's own code (validation_error / malformed_override) appears in the MCP message. It does not: the 400 branch emits code=validation_failed with a fixed "Validation failed." message and the server's text in the HINT, discarding the finer-grained code. Reworded to say what an agent actually receives, and to say that telling the two refusals apart means reading the message. Also carried the update-only qualifier on the github_pr exemption into the README, matching the catalog and instructions. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(items): state the exemption predicate, not the exemption list (lead ruling) The lead's ruling on the github_pr reversal: make the REASON what the code says, so the next key added to reserved metadata is evaluated against 'does this audience have a real writer?' rather than pattern-matched onto a list that happened to be wrong for one key. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V |
||
|
xarmian
|
de96cce900 |
fix(items,server,web): reserved metadata survives a move; referential metadata travels only within its context (BUG-2674) (#1165)
* fix(items,server): reserved metadata survives a move, and dropped fields are reported (BUG-2674)
Moving an item destroyed its implementation notes, decision log and linked-PR
metadata. Well-formed data, on a routine documented operation, silently, with a
success message.
Reproduced before the fix: a note written through `pad item note` — correct
shape, visible on every surface — was gone after `pad item move`, leaving
fields as `{"status":"new"}`.
## Why it happened
items.MigrateFields drops every key absent from the TARGET schema. The reserved
keys — implementation_notes, decision_log, github_pr, convention — are system
metadata that NO collection schema declares; each renders from its own dedicated
surface rather than as a generic field. So they are absent from every targetDefs
and were dropped on every move.
That blindness is structural, not incidental: any code path reasoning about
fields BY CONSULTING A SCHEMA cannot see these keys. It is the shared root of
this bug and of BUG-2627, where the CLI types a --field value by schema lookup
and these keys fall through to a raw string.
## The enumeration comes first, deliberately
Before this there were four constants and exactly ONE non-test consumer treating
them as a set — an inline || chain in a CLI display path. Naming the set inline
again here would have created the SECOND hand-maintained list, which is the
generator pattern behind both bugs reproduced inside its own fix: the next
reserved field lands in the constants, gets wired into whichever surface
prompted it, and silently misses the other.
So models.IsReservedItemField is now the single place that knows, MigrateFields
consults it, and the CLI's || chain is converted to it — the only way it is
provably THE list rather than A list. (formatChangeValue keeps its per-key
switch: it needs to know WHICH reserved key it has, to say "notes" vs "entries",
not whether the key is reserved.)
`convention` is IN the set, settled with evidence rather than by the principle
alone: 35 of 36 conventions in a live workspace do not store the key at all, and
the one that does holds a blob that is a redundant mirror of the alias keys
beside it. No user types a `convention` object — ApplyItemConventionMetadata
writes it, via library activation and the web form. System-stamped.
## Contract
System-minted non-referential data carries; anything dropped is reported.
PLAN-2357 DR-17 settled the analogous case — tags carry because "there is no
workspace-scoped foreign key to break, so dropping them would lose information
for no safety reason". These are the same shape: inert JSON with nothing that
could dangle in a destination. The plan's carry list simply never considered
them, so there was no deliberate semantics to defer to. DR-17's own heading is
"None of this may be silent."
## The reporting half
MigrateResult.Dropped has always existed and the single-move handler has always
thrown it away, so the only record of a field disappearing was the field being
gone. It now rides the move's audit metadata — not the response body, which is
the bare item and would break every consumer, and the activity timeline is where
someone asking "what happened to my item" looks. Joined into one string because
that map is map[string]string and a raw array renders as a Go map literal in the
timeline (BUG-2628).
## Verified
Unit: reserved keys carry with their payload INTACT (asserted on the value, not
merely the key — a carry that re-encoded or zeroed it would pass a presence
check), and bypass schema matching entirely, so a target declaring
`implementation_notes` as `text` cannot coerce them. Mutants run: guard removed
-> both new tests fail; carried-but-also-reported-dropped -> the not-dropped
assertion fails; carry-everything -> the control leg fails alongside three
pre-existing tests.
Live, against a server built from this branch: the note survives the move
byte-identical, and the move's activity metadata carries
`dropped_fields: "priority, status"` for the values the target schema genuinely
has no home for.
## Known scope limit
The BULK move path still discards its Dropped list — a reporting gap only, since
the carry-through lives in MigrateFields and bulk inherits it. Threading the list
out crosses two function boundaries whose signatures serve every bulk operation,
so it is a refactor of the bulk dispatch's return contract rather than a line.
Filed as BUG-2683 rather than smuggled in here.
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
* fix(server,web): close the four gaps Codex round 1 found on the carry-through (BUG-2674)
Round 1 raised no P1 and four P2s. Three were real defects introduced or exposed
by the carry-through; one was a genuine overclaim in the previous commit. All
four closed here, each mutation-verified rather than asserted.
## A schema may no longer declare a reserved key
MigrateFields carries these keys by identity, but every caller then validates
against the target schema — and ValidateFieldsDetailed iterates schema.Fields,
so it DOES see a declared key. A target declaring implementation_notes as `text`
would receive the carried array and reject it, turning a move that previously
destroyed the notes into one that fails outright. That is a worse failure than
the one being fixed: loud, but it blocks an operation that used to work.
The gate already existed — validateNoReservedFieldKeys, with its
grandfathering — and listed only parent/plan. The four metadata keys join it,
sourced from models.ReservedItemFieldKeys() so the two lists cannot drift.
Forbidding the declaration is the honest fix; coercing the value, or skipping
validation for a key the schema genuinely declares, would be guessing at which
meaning the author wanted.
The web's RESERVED_FIELD_KEYS gains the same four, preserving the existing
deliberate asymmetry (the client lowercases and is therefore stricter than the
server's exact match) so the UI steers authors away before the 400.
## The copy preflight no longer under-reports
`carried` is built by walking the DESTINATION SCHEMA, and these keys are declared
by no schema anywhere — so after the carry-through they appeared in NEITHER
bucket. A copy of an item whose content is its notes would report "nothing
carries over" while in fact retaining them. Before the carry-through they at
least showed under `dropped`, accurately. Reporting in neither is a regression
in the preflight's honesty, which is the same defect class as the move that
reported nothing.
They are now appended to `carried` after the schema-ordered entries, marked
`type: "system"` with a rendered label since they have no author-supplied one.
The bucket's doc comment says so: a client must no longer assume every `carried`
entry resolves to a destination FieldDef.
## The audit report now reaches a human
The previous commit claimed the activity timeline is where someone asks "what
happened to my item" — true, and the timeline renderer ignored the key, so the
report existed only for API and CLI consumers. Stored-but-invisible is not
reported. TimelineActivityCard renders the dropped keys on a move.
## Test aliasing
The "untouched" assertions compared the result against the SAME objects passed
in, so an in-place mutation would change both sides and DeepEqual would stay
true. The expectations are now independent deep copies — the only thing that
makes "untouched" mean untouched.
## Mutants, each run
Preflight pass removed -> the carried assertion fails. Timeline block disabled
-> the render assertion fails. Timeline action guard dropped -> the non-move
negative leg fails (a presence-only test would have passed it). Reserved-set
helper returning everything -> the IsReservedItemField control leg fails.
## Not fixed here
Codex's remaining observation — that a cross-workspace copy now carries
github_pr into a workspace whose repository it does not describe, and leaves a
convention blob detectable on an item outside the conventions collection — is a
product question about what a copy MEANS, not a defect in this mechanism. Raised
for a ruling rather than decided inside a bug fix.
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
* fix(items,server): referential system metadata travels only within its context (BUG-2674)
Lead ruling on the copy-semantics fork Codex round 1 raised. It does not add an
exception to the carry rule — it applies the qualifier the rule already had.
The contract was "system-minted NON-REFERENTIAL data carries". github_pr is
referential: it names a repository that is a property of the SOURCE workspace's
project, and it hydrates into code_context and renders as a live PR link. Carried
into another workspace that link is a false statement about the destination's
project, not preserved information. implementation_notes and decision_log
describe the item's own history and are true wherever the item is.
So the rule stays one sentence: non-referential system data carries everywhere;
referential system data carries only where its referent's context still holds.
## Scope is a required argument
MigrateFields takes items.MigrateScope. Required rather than defaulted because
BOTH wrong answers lose something: SameWorkspace on a cross-workspace copy
carries a PR link into a workspace it does not describe, and CrossWorkspace on
an ordinary move DROPS metadata from an item whose repo context never changed. A
caller that must name its scope cannot pick one by omission.
The two move handlers pass SameWorkspace as a property of the endpoint, not a
guess — a move changes an item's COLLECTION and cannot change its workspace.
The copy and its preflight COMPUTE it by comparing workspace ids rather than
assuming cross-workspace, because that endpoint accepts a target_workspace equal
to the source; hardcoding would drop a github_pr from a same-workspace duplicate.
Both sides use the same helper, or the preview promises a carry the copy drops —
the DR-6 divergence the shared endpoint exists to prevent.
## The drop is reported, with a reason that explains itself
PLAN-2357 DR-17: "None of this may be silent." It would be perverse to
reintroduce a silent drop inside this fix's own new branch.
The preflight reports it as `referent_not_portable` rather than the generic
`no_target_field`. That generic reason would be actively misleading here: no
schema declares these keys ANYWHERE, so "the destination has no such field" is
equally true of the source and explains nothing about why the value is being
left behind.
## Verified
Mutants run: scope ignored (always carry) -> the cross-workspace leg fails;
generic reason on the preflight drop -> the reason assertion fails. The
same-workspace leg and the non-referential-sibling leg are what stop an
implementation that ignores scope in EITHER direction from passing — each half
alone is satisfiable by a constant.
Gates re-run for THIS commit: lint 0 · go test ./... 0 · make test-pg 0 (3282).
Web gates NOT re-run and not claimed: this commit touches no web file (the web
half of BUG-2674 shipped in
|
||
|
xarmian
|
bc68b84848 |
fix(cli,mcp): send raw collection slug so an alias can't shadow a real collection (BUG-2630) (#1162)
* fix(cli,mcp): send raw collection slug so an alias can't shadow a real collection (BUG-2630)
The client-side alias map (collections.NormalizeSlug) rewrote seven hardcoded
singulars ("task", "plan", …) to their plurals BEFORE the request. In a
workspace whose collection slug IS one of those singulars, the user's exact
name was rewritten away and their create/list/move landed in a DIFFERENT
collection — silently, with a success message naming the wrong one.
Fix, per lead ruling on the BUG-2630 trail, split by transport:
CLI (real HTTP, may hit a pre-resolver server) — Option 2, one shared helper
cli.WithCollectionAliasFallback: send the RAW slug first (the server's
exact-match-first resolver from BUG-2578 wins, so an exact name is never
shadowed), and retry with the alias ONLY on a collection-not-found error, only
when the alias differs. Keying on collection-not-found is load-bearing: a
request to a collection that exists but fails for another reason is never
retried into the alias (that would recreate the bug). Both the schema fetch and
the create funnel through the helper so typed --field values parse against — and
the item lands in — one collection. On a genuine double-miss the error names the
RAW slug the user typed (collection "widget" not found), not the alias.
MCP remote transport (in-process ServeHTTP against the SAME binary, which always
carries the resolver — no version skew) — drop client-side normalization
entirely and send raw. Also removed the dormant expandPath collection
normalization: no routeSpec uses a {collection}/{target_collection} path
placeholder, so the branch was dead code in the area this fixes.
Search is deliberately out of scope (filed BUG-2659): its collection is a global
c.slug=? FILTER, not a path — a miss returns 200 + zero results, not
collection-not-found, so the retry can't key on it; and handleSearch is
cross-workspace, so the per-workspace resolver has no single workspace to run
against. Cross-workspace copy is excluded too (DR-13 forbids auto-retrying the
copy mutation).
Verified live against a real server: create/list/move into a singular collection
that collides with its plural now land in the named singular; shorthand still
resolves; genuine misses error naming the raw slug. New MCP integration test
reproduces the original shadow (item → PLANS-1) when normalization is restored.
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
* fix(server,cli): own collection resolution server-side + capability-gate the CLI retry (BUG-2630 Codex r1)
Addresses all three Codex round-1 findings, via the lead's ruling that
dissolves the earlier "retry vs archived-protection" tension by making the
server the sole owner of resolution semantics.
Finding #2 (MCP lost the legacy abbreviations t/i/p/d and phase/phases -> plans,
which the server's ±s resolver did not cover): fold the legacy alias map into
collectionSlugCandidates as a LAST-resort candidate. Exact-match-first and the
archived-claims refusal run for the input and every structural candidate before
the alias is reached, so it never shadows or redirects around a real/archived
collection. Now every client can send the raw slug — including the MCP transport
that can't retry — and lose nothing.
Finding #1 (the client retry re-opened the archived/hidden redirect the server
deliberately refused, because not_found can't be told from absent): add a
collection_resolution capability flag to GET /server/capabilities and gate the
CLI retry on it. Happy path unchanged (raw slug, one request). On
collection-not-found ONLY, the client probes capabilities once (cached): if the
server advertises resolution, its not-found is authoritative — the slug is
absent, archived, or hidden — so the client does NOT retry. Only an older server
that lacks the flag (or 404s the endpoint) triggers the legacy alias retry,
which is non-regressive there since old servers never had the protection. The
probe fails safe toward retry. This makes the follow-up distinct-error-code bug
unnecessary.
Finding #3 (double-fail masked a substantive alias error as "collection not
found"): the helper now surfaces a substantive alias-attempt error verbatim, and
only collapses to the raw-named not-found when the alias ALSO 404s.
Verified live against a resolving server: create/list/move into a singular that
collides with its plural land in the named singular; the abbreviation `i`
resolves to `ideas`; and after archiving `plan`, `create plan` honestly fails
("collection \"plan\" not found") instead of being retried into a live `plans`.
Gates: make lint 0 issues; go test ./... green; make test-pg green.
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
* fix(cli): fail-closed capability probe + always-retry the schema lookup (BUG-2630 Codex r2)
P1: the capability probe cached ANY failure as "no resolver", so a single
transient blip (timeout/5xx) permanently re-enabled the alias retry and could
bypass the archived/hidden protection on a resolving server. Now the probe
distinguishes a DEFINITIVE verdict (HTTP 200 with the flag, or a clean 404 =
legacy build) from an INDETERMINATE one (transport error / 5xx): only definitive
verdicts are cached, and an indeterminate probe fails CLOSED (trusts the
not-found, no retry) without caching, so the next call re-probes. A genuine old
server still returns a clean 404, so its retry is unaffected. Renamed the
predicate to CollectionNotFoundIsAuthoritative to name what it actually decides.
P2: the create schema lookup hits exact-match-only GetCollection, which does NOT
resolve slugs server-side, so capability-gating it made `create task
--field amount=3` 404 the schema fetch, skip the retry, and send amount as the
string "3". The schema lookup now always retries the alias (nil gate),
restoring typed-field parsing against an aliased collection's schema. Best-effort
as before: a genuine miss still degrades to string fields.
New client test covers the probe: definitive verdicts cache (one probe), and a
transient failure fails closed AND re-probes on the next call (mutation-verified).
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
* docs(cli): note fail-closed-on-indeterminate as a deliberate safety asymmetry (BUG-2630)
Per lead review: make explicit in CollectionNotFoundIsAuthoritative's doc that
failing closed on an indeterminate capability probe is deliberate — a recoverable
alias-shorthand miss is the safer side of the trade vs a retry doing an
un-undoable wrong-write. Comment-only.
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
|
||
|
xarmian
|
b5f0cd3963 |
feat(web,server): render embedded image attachments on share pages via a variants-only byte endpoint (BUG-2389 2b, TASK-2637) (#1153)
Merged by lead on accepted infra cancellation, Dave-approved in chat (day-44). Evidence basis: E2E tests demonstrably pass at pinned SHA
|
||
|
杨成锴
|
22c5a858a1 |
fix(server): stop counting disabled conventions as completed work (#1152)
Merged after two codex review rounds (converged) on top of the community-loop supply-chain/static review. Review found two narrow follow-ups — the guest item-grant leg of the grouped terminal query keeps pre-PR over-matching semantics, and the standup/changelog display layer hardcodes `status` — both pre-existing edges, filed internally as follow-up work. Thanks @asjdf for a well-tested fix, and for honoring the per-collection terminal_options contract on both the CLI and server paths. |
||
|
xarmian
|
625cab9984 |
fix: bound item history and stop resolving bodies nobody reads (BUG-2608) (#1147)
* fix: bound item history and stop resolving bodies nobody reads (BUG-2608)
Item history was unbounded on every surface, and summary mode paid for what it
discarded: the endpoint resolved EVERY version by walking the item's whole
reverse-patch chain, and both the CLI and the MCP dispatcher then projected
that away to metadata. An item edited under collab records a version every few
seconds while someone types, so this is routinely hundreds of full-content
reconstructions per history call, for output that shows none of them.
Two independent fixes, because they address different costs.
SUMMARY SKIPS THE WALK. `?summary=true` returns metadata from the raw rows and
never resolves a patch. That is the dominant win: the resolution was pure waste
for every caller except --full. Content and is_diff are cleared TOGETHER — an
empty body still claiming to be a reverse patch would tell a consumer to
resolve something that is not there.
LIMIT BOUNDS THE WINDOW, newest-first. That direction is not a preference: with
reverse patches, reconstructing any version means walking back from current
content, so a newest-end window is the cheap prefix of that walk while an older
one still pays for everything above it. That is also why there is deliberately
no offset — it would advertise a pagination whose later pages cost the same as
no bound at all.
Absent limit stays UNBOUNDED on the endpoint, following the item-list
precedent (maxItemListQueryLimit: "a zero/absent limit is left unbounded — this
only clamps an explicit oversized request"). The defaults live on the CLIENTS,
where a token budget is actually known: `pad item history` defaults to 50 with
--limit to change it, and the MCP catalog action injects 50 (max 300, the same
pair list and backlinks already use). A server that truncates a request nobody
bounded is a silent-truncation trap for third-party API consumers.
The MCP default goes in the CATALOG action rather than either dispatcher, so it
reaches BOTH transports — HTTP reads it off the input, and stdio receives it as
the CLI's new --limit through BuildCLIArgs. ToolSurfaceVersion 0.20 -> 0.21
with a changelog entry, plus instructions.md and README, per the 2304-family
contract discipline. Additive param bump: `limit` already existed, nothing
changed shape, and a v0.20 consumer that sends no limit now gets the newest 50
instead of all — which is the fix, not a break in it.
The restore and single-version-expand paths still resolve the FULL chain, and a
test pins that: bounding their walk would strand exactly the old versions those
paths exist to reach.
Eight mutations, each failing only the leg it targets. Three fixture problems
surfaced that way and are worth naming, because each made a test that could not
fail:
- force_version in a PATCH body does nothing (`json:"-"` on ItemUpdate), so
the throttle collapsed six edits into one version; varying the source per
edit is what actually records them.
- an 8-byte body is cheaper stored whole than as a patch, so no version was
ever is_diff=true and the is_diff assertion was inert. The fixture now uses
a body large enough that the store really stores patches.
- the cmdhelp test fixture lacked the new --limit flag, so BuildCLIArgs
silently dropped it. Verified against the REAL cmdhelp tree that the flag
is present and typed int, so the fixture mirrors the CLI rather than
flattering it.
* docs: bring CLAUDE.md to v0.21 and name why the two result caps differ (BUG-2608)
Codex round 1, both findings.
CLAUDE.md still described the MCP surface as v0.20 — stale because of my own
bump, in the document every agent working this repo reads first. README and
instructions.md are held to the version by a test; CLAUDE.md is not, which is
exactly why it drifts.
The cap "mismatch" (MCP max 300, endpoint clamp 500) is deliberate layering,
not an oversight — item lists have the identical split (300 in the catalog,
1000 at the endpoint) because the two answer different questions: an agent
token budget is only knowable in the catalog, while the endpoint's clamp is a
server-resource ceiling on what any caller may ask for. But nothing said so
anywhere, so a reader comparing the two numbers had no way to tell design from
accident — which is precisely the report Codex filed. Now stated at the
constant and in CLAUDE.md, including why the versions ceiling is LOWER than the
list one (resolving a version can cost a patch application per row, not just a
row read) and why an absent limit is left unbounded at the endpoint.
* fix+test: honest truncation notice, armed fixtures, and the residual named (BUG-2608)
Codex round 2, both findings, and the second is the more useful one.
CLI TRUNCATION NOTICE was wrong in both directions: it compared the response
length against the requested limit, so an item with exactly N versions was
reported as truncated, and a --limit above the server's ceiling was clamped
there and reported as complete. It now asks for ONE MORE row than it shows and
reports truncation only when that extra row comes back. The one case this still
cannot detect — an ask above the server's own ceiling, where the probe row is
clamped away with everything else — is stated in the code rather than papered
over by hardcoding the server's constant in the CLI.
UNDER-ARMED FIXTURES. The unbounded test seeded 5 versions, so a server quietly
defaulting to 50 would have passed the assertion that denies exactly that; it
now seeds 60. The clamp test seeded 2 and could not observe a clamp at all;
the clamp is now asserted directly against a parseItemVersionsLimit function
extracted for the purpose, over the inputs a URL can really carry (absent, 0,
negative, unparseable, either side of the ceiling).
That extraction replaced my own first attempt, which was worse than no test: it
re-implemented the clamp arithmetic in the test body and asserted the result
against itself. It could not have failed.
THE RESIDUAL, NAMED RATHER THAN IMPLIED. Codex's sharpest point is that the
summary tests cannot detect "resolve everything, then clear the fields" —
verified by mutation: pointing the summary branch at the resolving reader
leaves every handler test green, because the response is byte-identical either
way. So the performance claim does not rest on them. It rests on the handler's
summary branch calling ListItemVersionsPage (one reviewable line) plus a new
store test proving that reader really returns unresolved rows rather than
quietly resolving them — mutation-verified from the other side by making the
resolver a passthrough. The test file says all of this, including that an
end-to-end assertion would need a patch-application counter in the production
path, and why that is not worth it when the cost of being wrong is performance
rather than correctness.
* fix(cli): don't resolve for table output, guard the probe overflow, finish the CLAUDE.md bump (BUG-2608)
Codex round 3, four findings.
--full was treated as "content needed" regardless of output format, but the
table path prints no bodies at any setting — so `pad item history --full`
without --format json made the server walk the entire patch chain to build
content the CLI then dropped. That is the exact waste this bug is about,
reintroduced through the flag meant to opt into it. Content is now resolved
only when it will actually be shown.
The limit+1 probe overflowed at MaxInt: it wrapped negative, the client omitted
the parameter, and a request the user bounded came back unbounded — the
opposite of the ask. Guarded.
The truncation notice's documented blind spot was understated: it is AT the
server ceiling as well as above it, since the probe row is clamped away with
everything else. Wording corrected rather than resolved — the CLI still does
not duplicate the server's constant, because a copied ceiling goes stale
silently and asking for hundreds of versions is already opting out of a bound.
Two more CLAUDE.md sites still called v0.19 current; I fixed only the first on
the previous pass. That document describes the contract in three places and I
updated one, which is its own small lesson about grepping for every instance of
a claim rather than the first.
Live-verified against a real server: --limit 2 truncates and says so, --limit 4
on a 4-version item says nothing (the false positive Codex found), --limit 0
returns everything, --full --format json carries bodies, and the default JSON
shape carries metadata only.
* fix(server): clamp an out-of-range limit instead of treating it as unbounded (BUG-2608)
Codex round 4. `?limit=9223372036854775808` made strconv.Atoi return ErrRange,
which my parser lumped in with 'unparseable' and mapped to 0 — unbounded. So an
absurdly large number defeated the very ceiling the function exists to impose,
which is worse than no clamp at all: the bound looked enforced and was not.
Atoi hands back the saturated value alongside ErrRange, so a range-positive
input is now clamped to the ceiling — it is an oversized ASK, not a malformed
one. Range-negative still falls through to unbounded, matching a plain
negative.
Regression cases cover both overflow directions, and the mutation that
reinstates the old behaviour fails on the exact input.
This is the input-domain lesson again: I enumerated absent, zero, negative and
non-numeric, and stopped at what a person would plausibly type rather than at
what the TYPE admits.
* docs(cli): say in --help that a large --limit is capped server-side (BUG-2608)
Codex round 6, and the third time this ceiling came up — which is the tell that
the disposition was in the wrong place. I had documented it twice in CODE
comments, where the person affected never looks. `--help` says what the flag
does, and it said 'show only the newest N versions (0 = all)', which is false
for an N above the server's cap.
Now stated in both the flag help and the command's long description, including
that the truncation notice cannot detect that case and that --limit 0 is the
way to ask for a complete history.
Deliberately not naming the number: a constant duplicated into help text goes
stale silently, and 'capped server-side' is the part a user needs in order to
act. Behaviour unchanged — this is the artifact catching up with it, which is
the same correction the BUG-2301 sentinel comment and the instructions.md
overclaim both came down to.
* test: seed versions through the store so the fixture stops tripping the rate limiter (BUG-2608)
CI caught this and it is my defect, not a flake. Arming the unbounded test
above the plausible default meant seeding 60 versions, and the fixture did that
with 60 HTTP PATCHes in a burst — which trips the server's rate limiter. It
passed locally and in the Go job and failed under Nix, which is the signature
of a test that was always going to fail intermittently rather than one that
broke.
Seeding now goes through the store. That is not a weakening: versions are
recorded by the store on either path, and the endpoint under test is the READ
side, so seeding underneath the transport costs the assertions nothing while
removing a burst the server is entitled to refuse.
The three things that make this fixture work are now stated where someone would
otherwise undo them by accident — the large body (a small one is stored whole,
so no version is ever is_diff and every diff assertion goes vacuous), the
rotating source (the throttle collapses same-(actor, source) bursts into one
version), and the store-not-HTTP seeding with the rate-limit reason attached.
Re-verified after the change: the fixture still records more than 50 versions
and still produces reverse-patch rows, and the default-cap mutation now uses
the REALISTIC default of 50 rather than the 3 I first tested with — the old
5-version fixture could only have caught an implausibly small cap.
|
||
|
xarmian
|
50a442d048 |
fix(server): resolve collection slugs against the workspace's real collections (BUG-2578) (#1146)
* fix(server): resolve collection slugs against the workspace's real collections (BUG-2578) `pad item create spec` failed with "Collection not found" in a workspace whose collections include `specs`, because the singular forms live in collections.NormalizeSlug — a hardcoded switch over the DEFAULT templates' names, called from the CLI and the MCP dispatcher, both CLIENT side and neither with any view of the workspace. So a template-defined or user-created collection got no shorthand, and the spec template's central object was the one thing with no way to abbreviate it while peripheral `idea` had one. Resolving on the SERVER is what makes this general: the workspace's collection list only exists here, so one resolver covers the CLI, the remote MCP transport, the web UI and any direct API consumer, instead of teaching each client the same trick. `spec` is not in the client map, so it already arrives intact; a test in internal/mcp pins that pass-through, since a future map entry for it would silently take the fix away from MCP agents. EXACT MATCH ALWAYS WINS, and that is the property the design turns on. The fallbacks fire only when the input names no collection at all, so the resolver can never redirect a request that already succeeded — which is what makes it safe to add underneath five existing call sites. It has its own test, with the mutation that inverts the order failing it. Deliberately NOT wired into store.GetCollectionBySlug. That has 23 call sites including authorization paths (authz_cross_workspace, handlers_grants, handlers_share_links), and fuzzy resolution inside a function used for permission checks is how a check and the action it guards come to disagree about which collection they mean. Scope is the five user-typed item operations: create, list, move, bulk move, cross-workspace copy. Internal derivations (artifact import's collectionSlugForKind) and the web-only progress endpoints keep exact matching. Two things worth noting for whoever reads this next: The list handler resolved the collection for its visibility gate and then filtered items by the RAW url parameter, so a singular returned 200 with an empty list — a resolve-then-pass-the-unresolved-value bug my own wiring introduced, caught by the test that asserts listing works, not by the one that asserts creating does. This does NOT fix the sibling defect the re-derivation turned up: the client map SHADOWS an exact match, so in a workspace holding both `plans` and a user-created `plan`, `pad item create plan` silently files into `plans`. Verified still reproducing after this change, because the rewrite happens before the server sees the slug. Filed as BUG-2630 with a live repro; the lead ruled option 2 (send raw, retry on collection-not-found) and it rides a later PR, since changing wire behaviour is a compatibility call rather than part of this fix. * fix(server): canonicalize the resolved slug downstream in bulk move and items-index (BUG-2578) Codex round 1, and both findings are the same defect class as the one my own list test caught: resolve the collection, then keep using the caller's raw input for everything downstream. Bulk move is the one that matters, and it was reachable only BECAUSE the resolver made `spec` succeed at all — so the inconsistency arrived with this change rather than predating it. req.Collection is compared against item.CollectionSlug to decide whether the op even IS a cross-collection move, written into activity metadata as to_collection, and used as the SSE scope the arrival event is addressed to. Left raw, a move into `specs` would log a to_collection of "spec" that no reader can look up, address the arrival event to a lane no client watches, and — for an item already in `specs` — compare unequal and categorise a same-collection no-op as a move. Canonicalized once up front rather than at each of the four use sites, so a fifth use cannot reintroduce it. items-index filtered by exact slug too, so `?collection=spec` returned an empty index rather than an error. The web client sends canonical slugs and is unaffected; this is for direct API consumers, and it keeps the same exact-match-wins property, so no existing query changes meaning. A slug that resolves to nothing is passed through untouched, preserving today's behaviour. Both are mutation-verified: removing the canonicalization fails the activity assertion with the literal to_collection "spec", and removing the index resolution returns the empty result set. * test: cover cross-workspace copy and drive the MCP claim end to end (BUG-2578) Codex round 2, two coverage gaps, both real. The cross-workspace copy call site was wired to the resolver and never exercised: every existing copy test passes an exact slug, so reverting that line would have gone unnoticed. Now covered through BOTH halves — preflight and the mutating copy — because they resolve the destination separately, and a preflight that accepts a name the copy then rejects is the worse of the two failures. Mutation-verified: reverting the call site fails it with "Destination collection not found". The MCP test was scoped to what the dispatcher BUILDS — that the slug is passed through rather than rewritten — and its comment said so, but a URL assertion is a claim about the dispatcher, not about what an agent receives. Since the bug's body makes a claim about MCP agents specifically, that claim now has a test that drives the real server and store over the transport: create in `spec`, then LIST by the same shorthand, because an agent that can create something it cannot then list is not fixed. Mutation-verified: removing the server fallback fails it with the exact user-visible error the bug reports. The pass-through test stays. It guards a different thing — that a future entry in the client-side alias map would silently take the server fix away from MCP by rewriting the slug before it arrives — and has its own control (adding `spec` to the map fails it). The copy fixture uses a permissive destination schema on purpose: the shared dstSchemaJSON has required fields the source item does not carry, and a validation rejection would mask the resolution result under test. * fix(server): case-fold before pluralizing, pin the list by ID, resolve the bulk target once (BUG-2578) Codex round 3, three findings, all correct. CANDIDATE ORDER (P1). Pluralization was tried before the case-folded form, so `Spec` resolved to `specs` in a workspace holding both `spec` and `specs`. That is the same misfiling the exact-match-wins rule exists to prevent, reached by a different route: `Spec` names `spec` more closely than it names that name's plural. Folded form now goes first. My own candidate test had the wrong order baked into its expectation, which is why it did not catch this — the new end-to-end case asserts where the write actually lands, and both fail on the old order. LIST PINNED BY ID (P1). Visibility was checked against coll.ID and the query then filtered on a SLUG. A slug can be freed by a rename or delete and taken by another collection in between, so the response could carry a different collection's items — possibly one the caller cannot see. The ID cannot be reassigned, and both filters are ANDed, so a concurrent rename now yields an empty list rather than someone else's rows. Note this predates the diff in kind: the handler filtered by the RAW slug before, with the same gap. BULK RESOLVES ONCE, AND NOW THAT IS TRUE (P2). The previous commit canonicalized the target up front and said it did so "rather than resolving it per-item further down" — but the per-item path went on calling the resolver for every row, so a 300-item batch with an unresolvable target could run ~1,200 lookups. The comment and the commit message both overstated the code. The resolved collection is now threaded through applyBulkOp into bulkMoveCollection, an unresolvable target fails the request up front instead of once per item, and the claim matches the implementation. That last one is the failure I keep meeting from different sides: the code was defensible and the sentence describing it was not true. Worth naming plainly rather than quietly fixing, because a reviewer reading that comment would have had no reason to check. * fix(server): revert the CollectionIDs pin — it was a visibility leak, not a scope filter (BUG-2578) Codex round 4. The P1 is a hole I opened one commit earlier, and it is the worst thing on this branch. To close a slug-reuse race I "pinned" the collection-item list by setting params.CollectionIDs to the resolved collection, and wrote a comment asserting the two filters were ANDed so a concurrent rename would fail safe. I did not read the query. CollectionIDs and ItemIDs are a PERMISSION PAIR and the store combines them with OR — "in a fully-granted collection, OR specifically granted". So pinning CollectionIDs while the item-grant branch of the same handler set ItemIDs rewrote the caller's grants into `collection_id IN (this) OR id IN (granted)`, handing a caller whose only claim on the collection is ONE item grant every item in it. Reverted. The race it was meant to fix is filed as BUG-2631, WITH the reason this fix is wrong, because setting CollectionIDs is the obvious move and the next person will reach for it too; the real fix needs a scoping parameter distinct from the permission pair. A regression test now covers the leak over both auth classes, and it fails with the ungranted sibling in the response body when the pin is reinstated. Every other test in that file uses an unrestricted owner, which is precisely why none of them noticed — the property was invisible to the whole fixture family I had been writing. Two round-4 P2s, both fixed: The bulk endpoint refused an unresolvable target with a 400 while an existing-but-hidden target failed per item inside a normal 200 envelope. That status difference is an existence oracle — a restricted caller can probe slugs and learn which collections they may not see exist. Unresolvable targets now take the same per-item path, which is also the pre-change behaviour, and a test asserts the two responses are indistinguishable. items-index discarded the resolver's error and continued with the raw alias, answering a database failure with a successful EMPTY index. It now surfaces the error. The lesson I am taking, since it is the second time today the same shape bit: I asserted a mechanism (AND semantics) in a comment without reading the code that implements it, and the comment made the change look considered. Last time that produced a wrong explanation on a trail; this time it produced a permission bypass. * docs+test: correct three overstatements and strengthen the oracle test (BUG-2578) Codex round 5. Three of the four findings are my own prose claiming more than the code does — the same failure mode this branch has now produced four times, so it is worth fixing rather than shrugging at. The resolver's doc said a singular form works for "every collection". It handles a trailing ASCII `s`, so `spec`/`specs` resolves and `category`/`categories` does not. The doc now says "a regular singular/plural pair", names the limit, and points at the paragraph explaining why -s is a deliberate stopping point rather than a gap to close with an inflector. bulkMoveCollection's doc said its targetColl parameter "is never nil". The immediately preceding commit made it deliberately nil for an unresolved target — that is what keeps a hidden and a nonexistent collection failing identically — and the function has a nil check three lines down. Now says so. The MCP test's comment implied the transport. It drives the dispatcher against a real in-process server, which proves the resolution reaches an MCP tool call; it does not go over the remote /mcp HTTP transport or its OAuth layer. Scope stated in the test so nobody reads more into a green run. The fourth is a real test weakness: the existence-oracle test compared only HTTP status, so an implementation returning both cases inside a 200 envelope with different error codes would have passed while still leaking. It now compares the per-item failure shape too, with item ids stripped since those legitimately differ, and a non-JSON body compared verbatim rather than normalized to empty — which would have made two different errors look identical. Mutation-verified: changing only the unresolved-target error code, leaving the status alone, now fails it. Round 5's P1 — that cross-workspace copy requires workspace-level edit on the destination before any collection-grant check, so a destination collection grant is unusable — is NOT addressed here and is not mine to judge on this branch. The ordering predates this diff (I only swapped the lookup call), and the scope constructor is explicitly named CrossWorkspaceWorkspaceOnlyScope, which reads deliberate rather than accidental. Raised with the lead as an unverified observation rather than filed as a defect, since I have not read PLAN-2357's authorization design and would be filing a design question dressed as a bug. * test: read the failure field the endpoint actually emits (BUG-2578) Codex round 6. normalizeBulkFailures decoded failed[].message; the endpoint emits failed[].error (bulkItemFailure). So the message half of the existence-oracle comparison decoded to the empty string for every row and compared equal always — dead since the moment I added it to close exactly that gap, and my mutation had changed the code AND the message together, so it failed on the code and told me nothing about the message. Fixed, and re-verified with a mutation that leaves the status and the error code identical and changes only the message: it now fails. The struct carries a note that the field names mirror bulkItemFailure, since an invented name here fails silently rather than loudly. Third time on this branch that a test I wrote to be rigorous was not, and the tell each time was that I checked it passed on good code without checking WHICH part of it could fail. * fix(server): an archived collection blocks the alias instead of handing its name away (BUG-2578) Codex round 7, and it took a real judgement call rather than a mechanical fix. GetCollectionBySlug skips soft-deleted rows, so with an archived `spec` alongside a live `specs`, the exact lookup missed and the alias fallback picked up `specs` — archiving a collection would quietly start routing its writes into a different one, and a later restore would leave those items stranded where they were rerouted. I first read this as acceptable: an archived collection is not a writable target, so resolving to the live neighbour looks like the alias feature doing its job. What decided it the other way is that this branch already refuses exactly this trade on the client side. BUG-2630's whole complaint is that a silent misroute into a different collection is worse than an honest error, and the same reasoning cannot be right there and wrong here just because the redirect happens to be convenient. Archived rows now claim their name: the exact form returns not-found rather than falling through. The narrow store method (ArchivedCollectionClaimsSlug) answers a boolean rather than returning the row, because an archived collection is never a valid target — it only blocks the name, and returning it would invite a caller to use it. Covered end to end with the fixture armed first (the collection resolves to itself while live, so the assertion is about the archive edge and not about the resolver being broken generally), and mutation-verified: removing the guard fails it with the item sitting in `specs`. * fix(server): run the archived-name guard for every candidate, not just the input (BUG-2578) Codex round 8. The previous commit checked the archived claim only for the raw input, so an archived `spec` beside a live `specs` still let `Spec` through: the exact form missed, the case-folded candidate `spec` found no LIVE row (GetCollectionBySlug skips soft-deleted), and resolution walked on to `specs`. The archived name was stepped over by a spelling of itself. Restructured so the sequence is uniform — the raw input and every fallback ask the same two questions in the same order, is there a live collection with this name and does an archived one claim it. That is also easier to reason about than a guard bolted in front of a loop, which is how the hole existed. Mutation-verified with the previous shape restored: guarding index 0 only fails the new test with the item sitting in `specs`. |
||
|
xarmian
|
2c8ddffcb0 |
fix(store): cover documents and comment bodies in the attachment reference walks (BUG-2614, BUG-2615) (#1145)
* fix(store): cover documents and comment bodies in the attachment reference walks (BUG-2614, BUG-2615)
Two defects with one shape: a content surface that carries `pad-attachment:`
references was missing from a walk meant to cover every such surface. Both were
found by Codex during BUG-2415 and both predate it.
BUG-2614 — the orphan GC could reclaim a live reference. AttachmentReferenced
counted items and comments; documents.content was never scanned, and neither
document write path stamped. An attachment referenced only from a document was
therefore both invisible to the sweep's scan AND unprotected by the stamp that
covers references landing mid-sweep.
The filing asked whether the documents surface is dead enough to delete instead
of widening the scan. Evidence says widen, and I am not making the deletion
call inside a bug fix: /workspaces/{ws}/documents has full CRUD mounted and
authenticated today (list/create/get/patch/delete plus restore, versions and
activity), so a direct API consumer can still write one. It IS legacy — the
route block says "v1 — will be replaced by items in Phase 2" and no first-party
client reaches it (zero references in the web API client and in cmd/pad) — and
production carries 4 document rows, all soft-deleted, none referencing an
attachment, newest touched 2026-04-27. "Reachable but unused by us" is not
dead, and the conservative fix is a few lines. Retiring the surface belongs
with the Phase 2 migration, deliberately.
CreateDocument had no transaction, so it gains one: the stamp has to commit
atomically with the content carrying the reference or it cannot serialize
against a concurrent claim, which is the whole point. UpdateDocument already
had a transaction and only needed the call — and only when content is actually
written, since a metadata-only PATCH neither adds nor keeps a reference and
must not vouch for one.
BUG-2615 — the bundle import's remap rewrote item content and fields but not
comment bodies, so an imported comment kept the SOURCE workspace's ids: broken
references in the destination, and the rehydrated rows they should point at
left referenced by nothing. Bundles do carry comments (export.go exports them,
ImportWorkspace re-inserts them); they carry no documents, so this stays scoped
to comments.
The remap also now stamps what the rewrites point AT. ImportWorkspace already
stamps each comment body at insert, but the body still holds the source ids
then and the remap runs later in the handler, so those stamps land on nothing
that ends up referenced — leaving a fresh clone referenced only by text the
transaction just wrote and carrying no stamp, which is exactly the shape the
never-attached claim reclaims. The REWRITTEN TEXTS are passed rather than every
id in the map, so a clone nothing references is not vouched for and does not
survive an extra GC window.
Seven negative controls, one mutation at a time, each failing exactly the test
that covers it: the documents scan leg, each of the two stamps, the comment
write-back (at store and end-to-end level), the remap stamp, and an over-broad
stamp-the-whole-map variant that the precision test catches. Per the standing
bar out of BUG-2301, every regression test here was RUN against the unfixed
code and observed to fail — including the end-to-end bundle fixture the filing
asked for, whose item deliberately carries no reference so that the items walk
alone cannot rescue it.
* fix(store): stamp before the remap's content writes, and state the caller precondition (BUG-2615)
Codex round 2, two P1s.
The first is mine and is a straight violation of the protocol I was mirroring:
I stamped AFTER the item and comment UPDATEs. stampAttachmentRefsTx's own
contract says to call it before the content statement, for two reasons that
both bite here. On Postgres the stamp row-locks the attachment rows for the
rest of the transaction, so a concurrent GC claim blocks and re-evaluates
against the fresh stamp — stamping last instead lets a claim delete the target
while the rewritten text is still uncommitted, after which the stamp matches
zero rows and the transaction commits a dangling reference. And every other
writer takes attachments before content rows, so writing content first inverts
the lock order and deadlocks against them. The texts are known as soon as both
scans finish, so the stamp simply moves up.
The second — the scan-then-write over comments has no row lock and no
old-value predicate, so a concurrent edit committed in between is clobbered —
is real as a shape but not reachable at the only call site, and is NOT fixed
here. The bundle import runs this against a workspace it has just created,
which no other session can reach yet: there is no concurrent writer to lose an
edit to, and no contention for the long transaction to hold up. The
pre-existing items walk has the identical shape, so this is a property of the
function rather than of the comment leg. Adding row locks or a compare-and-swap
would be machinery for an unreachable window.
What that argument does require is that the precondition stop being tribal
knowledge, since it is about the CALLER and the next caller is exactly who
would break it. It is now stated at the top of the function, where someone
adding a second call site reads it, rather than in this message.
Also declined, both pre-existing and neither introduced here: the one-transaction
scan of the whole population (same reasoning — one caller, fresh workspace), and
document slug allocation outside the create transaction, which predates the
transaction existing at all and yields a spurious unique-violation rather than
partial state.
NOT COVERED BY A TEST, stated rather than implied: the stamp ORDERING. The
existing guard asserts the stamp is present and fails without it, but it reads
end state, so it cannot distinguish before-the-writes from after. Proving the
order needs a concurrent-session Postgres instrument of the kind BUG-2409 used;
that is not built here. The ordering rests on the reasoning above and on the
contract documented at stampAttachmentRefsTx.
* docs(store): make the scanned-surface set an explicit contract (BUG-2614)
Codex round 3 P2. Both comments a maintainer reads still described the scan as
covering items and comment bodies — AttachmentReferenced's doc, and the
orphan-GC sweep's "Item content references the attachment" branch — so the
change that added documents left the two artifacts that explain it stale. Same
class as the sentinel comment on BUG-2301: the code was right and the text
someone acts on was not.
They now also say the thing neither said before, which is why this defect
happened twice: the SET of scanned surfaces is the contract. Any surface that
persists user-authored text containing a `pad-attachment:` token has to be
listed there, and adding one without adding it here silently makes its
references invisible to the GC. Comments (IDEA-1650) and documents (BUG-2614)
were both found after the fact, which is the argument for writing the rule down
rather than the two instances.
Round 3's P1 — restore paths do not re-stamp, so a reference reclaimed while
archived is dangling after restore — is filed as BUG-2629, not fixed here. It
is pre-existing and uniform: RestoreItem does not stamp either, so fixing only
RestoreDocument would leave the larger hole open while making documents
inconsistently better-protected. The filing records the asymmetry that decides
its priority: items are usually shielded by the claim's own item_id IS NULL
predicate, while a document-referenced attachment has no document_id column to
be shielded by and is always claimable.
* docs(store): mark the unstamped rename cascades in place, pointing at BUG-2629 (BUG-2614)
Codex raised the title-rename cascade's missing stamp in two separate rounds
despite being told it was filed. Being raised twice is the signal that the
disposition was only in a bug tracker and not where a reader of this code
meets the problem — the same correction BUG-2301 ended on.
Both sites now carry it: documents.go::updateLinksInTx and
wiki_links.go::cascadeTitleRename, each naming BUG-2629, why it is not fixed
here (uniform across both surfaces, so half-fixing makes them inconsistent),
and why it is the weakest member of that family (the cascade rewrites link text
in content whose references were already stamped and are still visible to the
scan, so a genuinely new reference needs a title containing a pad-attachment
token).
Comments only.
|
||
|
xarmian
|
6f16003199 |
fix: surface implementation notes + decision log in the item timeline (BUG-2301) (#1144)
* fix(server): merge implementation notes + decision log into the item timeline (BUG-2301) `pad item note` and `pad item decide` have written structured entries since |
||
|
xarmian
|
08dfbdb318 |
fix(server): rowless-blob GC sweep — reclaim blobs no attachments row references (BUG-2406) (#1137)
* fix(server): rowless-blob GC sweep — reclaim blobs no attachments row references (BUG-2406) Every attachment write path calls AttachmentStore.Put BEFORE inserting the attachments row, so a failure (or crash) between the two leaves a blob on disk that nothing references — and the row-driven orphan sweep, which walks Store.OrphanedAttachments, can never see it. Disk that is never returned; the upload handler's failure comment even claimed the GC would reclaim it. Fix: a rowless-blob sweep that runs after the row sweep on the same GC tick. attachments.Lister is a new OPTIONAL backend capability (ListBlobs → key/hash/size/mtime); FSStore implements it via one WalkDir of the sharded tree with a base-name validHash gate (excludes Put's dot-prefixed temp files and anything the store didn't write). Backends without the capability are skipped with a once-per-process notice. Candidate = blob whose content hash has ZERO rows in ANY state (soft-deleted rows still own their bytes under the row sweep's row-before-bytes claim protocol, BUG-2415) AND whose mtime predates the same operator-configured GC grace the row sweep uses — a young rowless blob is just an upload whose insert hasn't happened yet. Delete-time guards run under inFlightHashesMu: the in-flight fence plus a single-hash row RE-CHECK that closes the subtraction-to-delete TOCTOU (the writer that marked, inserted, and released entirely inside the gap). Cost: O(blobs) per tick, 24h cadence, never on a request path. Also retro-reclaims blobs stranded by past row-sweep delete failures. The wrong claim in handleUploadAttachment's failure path is corrected to point at this sweep. Tests: FSStore.ListBlobs impostor coverage; five sweep legs (aged-rowless reclaimed with a row-sweep-can't-see-it counterfactual, young kept, live/soft-deleted-row kept, in-flight kept then reclaimed after release, hook-injected delete-time row kept) — mutation-verified: removing the re-check, the age gate, or the in-flight fence each fails its leg; the store-level subtraction contract is pinned separately. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * docs(store): state the any-row rule's real rationale per Codex review (round 1) Codex flagged the thumbnail refusal-cleanup's grace-window protection as inconsistent with the sweep comment's claim that deleting bytes under any existing row violates the claim protocol. The cleanup (and the row sweep itself) deliberately end a row's hash-protection when its own grace expires — CountProtectingAttachmentsForHash documents exactly that, and the row machinery may do it because its claim protocol coordinates row and blob fates within a sweep. The overstatement was mine: the rowless sweep's any-row rule is chosen because it holds no claim on any row and has no such coordination, not because past-grace stranding is forbidden to the machinery that does. Comment corrected; no behavior change on either path. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V |
||
|
xarmian
|
31075d996a |
fix(store): route cross-workspace copy's lock-held reads through the copy transaction (BUG-2409) (#1136)
* fix(store): route cross-workspace copy's lock-held reads through the copy transaction (BUG-2409) The copy transaction holds advisory locks on BOTH workspaces, but the attachment planner (PlanAttachmentCopy) and the server's per-row attachment authorizer read through the connection pool. Under enough concurrent copies every pooled connection can be occupied by a lock-waiter while the lock holder waits for a spare connection — starvation presenting as a hang. Fix: a store.Queryer interface (satisfied by *sql.DB and *sql.Tx) threaded through the planner and the AttachmentAuthorizer callback, so the mutating copy plans and authorizes on its own transaction's connection while the preflight keeps planning through the pool — one implementation, two executors, preserving TASK-2354's no-drift shape. Mechanical *Q variants added for the store reads the authorizer transitively needs (GetItem, GetUser, GetWorkspaceMember, VisibleCollectionIDs, GetMemberCollectionAccess, ListSystemCollectionIDs, GuestVisibleCollectionIDs, GuestVisibleResources(+IncludeDeleted), ResolveBacklinksVisibility) and Q-cores behind existing-signature server wrappers (checkItemVisible, guestResourceFilterCore, resolveAttachmentParentItem, attachmentCallerIsRestricted). No decision logic changed anywhere — executor threading only. GetItem/getItemTx/GetItemIncludeDeleted's three duplicate scan bodies collapse into one getItemScanQ. Regression test: TestCopyItemAcrossWorkspaces_NoPoolIOUnderLocks pins the invariant deterministically — with MaxOpenConns(1) the transaction owns the only connection, so ANY pool read under the locks deadlocks. Fails by timeout on the pre-fix executor (verified); passes in 0.16s fixed. The test's authorizer performs a real read through the handed Queryer, pinning the callback leg too. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V * fix(store): quota check reads through the copy transaction too, per Codex review (round 2) Codex's targeted round found the third lock-held pool-read leg: CheckLimitTx routed only the feature COUNT through the caller's transaction while checkLimitOn's owner lookup, GetUser, and resolveLimit's platform-setting read stayed on the pool — the same starvation shape under the copy's advisory locks. checkLimitOn is now parameterized over a single Queryer for every read (CheckLimit passes the pool, CheckLimitTx the transaction), with resolveLimitQ / GetPlatformSettingQ variants behind existing-signature wrappers. The regression test now arms this leg deliberately: a FREE-plan owner with EnforceItemLimit and no plan override drives the full quota read chain under MaxOpenConns(1) — verified deadlocking before this commit, 0.16s after. Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V |
||
|
xarmian
|
cc26288794 |
fix(web): share pages render attachment refs as honest placeholders (BUG-2389) (#1135)
The public share route (/s/{token}) rendered item content with a bare
marked() call, so pad-attachment: references fell through as broken
<img src="pad-attachment:..."> tags and dead links. Two halves:
1. CommentThread.svelte is deleted outright — grep proved it was
unmounted dead code (its only reference was a prose mention in
ItemDetail.svelte), so its half of the bug resolves by deletion
rather than by fixing a component nothing renders.
2. The share route now renders through a new opt-in wrapper,
renderMarkedWithAttachments(), which threads an AttachmentRenderContext
into the existing marked renderer hooks. With a null resolver and the
new renderAttachmentUnavailable() placeholder, every ref becomes an
honest "Attachments aren't available on shared pages yet" chip —
deliberately NOT the "missing or has been deleted" wording, because
the attachment exists; the share surface just cannot serve its bytes.
Sanitization is unchanged: the wrapper returns unsanitized HTML and
the share page keeps its single DOMPurify pass.
The `missing` hook is a parameter (default: renderAttachmentMissing) so
authed surfaces keep their existing wording, and the wrapper clears the
module context in a finally block so bare marked() callers are
unaffected (pinned by test).
The token-scoped byte endpoint that would serve real images on share
pages (2b) is deliberately NOT built here — it adds a new
unauthenticated ACL surface and is tracked separately pending approval.
A real resolver through the same wrapper is the plug-in point (pinned
by test).
Tests: markdown.shareAttachments.test.ts (6 unit legs incl. bare-marked
opt-in control and context-clearing) and
bug-2389-share-attachment-placeholder.spec.ts (e2e: real upload → item
ref → item share link → anonymous visit; verified failing on the
pre-fix build).
Claude-Session: https://claude.ai/code/session_017jD6t1zjxGSq47SQpZfp1V
|
||
|
xarmian
|
e0c5792ce9 |
fix(store): attachment delete vs thumbnail derivation race — atomic cascade, locked conditional insert, orphaned-variant GC class (BUG-2388) (#1134)
* fix(store): attachment delete vs thumbnail derivation race — atomic cascade, conditional variant insert, orphaned-variant GC class (BUG-2388) Deleting an attachment while thumbnails were still deriving could mint a live, unreachable variant row under a tombstoned parent: the delete cascade tombstoned original and variants in separate statements, and derivation checked parent liveness once, then inserted uncondition- ally. The leaked row was invisible in the UI, counted toward quota forever, and no GC class could reclaim it (the old code's comment claimed a 'deleted-parent path' existed; it did not). Three parts, all the BUG-2415 claim-by-statement discipline: - SoftDeleteAttachment tombstones original + variants in ONE transaction. - CreateAttachmentVariantIfParentLive makes the parent-liveness check part of the variant INSERT itself (INSERT..SELECT WHERE EXISTS parent live); persistThumbnail cleans up the just-Put blob on refusal under the in-flight hash fence it already holds, honoring the same hash-dedupe protections as the sweep. - Orphan GC gains the orphaned-variant class: live variant whose parent is tombstoned/gone, tried FIRST for live parented candidates (an item_id-NULL leak would otherwise hide behind a content reference to its dead parent in the never-attached scan). The claim re-asserts parent-not-live at delete time, so a concurrent parent restore wins and a restored original keeps its thumbnails. This class also retro-reclaims rows already leaked. Tests: the filed race pinned deterministically (persistThumbnail with a pre-delete parent snapshot — control build mints the leaked row verbatim); retro-reclaim sweep test with a restore-wins leg, its leak fixture deliberately ATTACHED so only the new class can reclaim it (control build: row survives). * fixup: codex round 1 — parent row-locks on the conditional insert + variant claim (CreateAttachmentForLiveItem precedent), fenced+config-aware refusal blob cleanup, store-level restore-refusal claim test, blob-cleanup assertion * fixup: count inside the in-flight fence — a completed upload lifecycle could stale an outside count (codex round 2) |
||
|
xarmian
|
2e4f3d5dc2 |
fix(server): refuse a PATCH carrying both a fields hierarchy key and top-level parent_id (BUG-2594) (#1133)
* fix(server): refuse a PATCH carrying both a fields/fields_patch hierarchy key and top-level parent_id (BUG-2594) extractParentLink staged the item_links write (including the empty- string clear) while ItemUpdate.ParentID stamped the parent_id column unconditionally in the same transaction — one request could clear the link AND re-parent the column, leaving silently inconsistent hierarchy state (unparentedItemPredicate still saw a parent). The shape is raw-HTTP-only: no first-party client sends top-level parent_id on item update (CLI resolves --parent into the patch; the web client and MCP catalog never carry it). Both update paths (full fields + fields_patch) now refuse the pair with a validation error naming both keys — refused, not silently resolved, per the clear_parent contract family's standing rule (v0.19). Solo parent_id and solo fields-patch hierarchy writes are deliberately unchanged (BUG-2379 tracks the adjacent undeclared- override family). Six handler tests: refusal on clear+id, set+id, the plan alias, and the full-fields sibling path — each verified failing (200) on the unguarded control build — plus both solo-write controls. * fixup: assert the validation_error code + plan alias in the refusal envelope (codex round 1) |
||
|
xarmian
|
d68474f775 |
feat(server): armed-session declaration + push delivery filter (PLAN-2613 S1, TASK-2616) (#1130)
Adds a server-side consent gate for push delivery ahead of the plugin/CLI version flip: a stream now declares armed=true at connect (query param) to receive KindPush notifications, while legacy (unarmed) streams keep ordinary watch-matched delivery during the skew window. LiveSession exposes the armed bit so the web target picker can eventually show honest accepting-pushes counts, and push delivery counts are now armed-aware end to end (broadcast, targeted, and the pre-publish snapshot used to skip a guaranteed no-op). |
||
|
xarmian
|
8cdeeb166b |
fix(store): orphan GC claim protocol — writer reference stamps + conditional row-first deletes (BUG-2415) (#1129)
* fix(store): orphan GC claim protocol — writer reference stamps + conditional row-first deletes (BUG-2415) The sweep scanned content for pad-attachment: references, then deleted the BLOB, then the row — with nothing serializing it against content writers. A reference committing between scan and reclaim left either a dangling id or, worse, a surviving row whose bytes were already gone. Claim protocol: - attachments.last_referenced_at (dual-dialect migration): every content writer that persists a pad-attachment: reference stamps the rows INSIDE its own write transaction (stampAttachmentRefsTx), wired at the four store chokepoints every surface funnels through — CreateItem, the UpdateItem core (item PATCH, collab-snapshot flush, version restore, bulk update), CreateComment, UpdateComment (both now transactional). Workspace-scoped; covers content AND fields, matching AttachmentReferenced's scan surface. - The sweep's row deletion is now the atomic claim: a conditional DELETE re-asserting reclaimable state in the statement itself (ClaimNeverAttachedAttachment: unattached + live + no fresh stamp; ClaimSoftDeletedAttachment: still deleted + still past grace, so a mid-sweep restore survives too). Writer stamp and claim serialize at the database; whichever commits first wins and the loser observes it. - Row BEFORE bytes: the blob is reclaimed only after a successful claim, so a surviving row implies surviving bytes — the old order's worst failure mode (row without content) is structurally impossible. - orphanGCRefStaleWindow (15m) is documented as a correctness parameter: the stamp only covers references landing after the scan, so the window bounds scan-to-claim latency plus a maximally stalled writer transaction — not a lease on long-lived references (the LIKE scan still guards those). Sweep-level test pins the filed race (fresh stamp survives sweep, row AND blob) with a counterfactual arm (aged stamp reclaims); verified discriminating against a compiling control build of the old sweep order. Store tests cover every claim predicate leg, stamp wiring on all four chokepoints, and workspace scoping. * fixup: codex round 1 — stamp move-override + workspace-import paths, parent-aware variant protection (scan by parent id + claim NOT EXISTS fresh parent stamp), variant test with total-loss control * fixup: codex round 3 — stamps ordered BEFORE content statements (PG row-lock makes the claim wait out the writer tx), chunked stamp IN-lists, BlobDeleteFailures counter * fixup: codex round 4 — stamp variants of referenced originals (own-row lock protects concurrently-claimed thumbnails), bounded-duration residual + irrevocability docs |