fix(presence): stop heartbeats handing over the agent-name registration lock - #84
Conversation
…on lock Reverts the five CLI call-site changes from #83. agent_presence.session_id is the registration lock's HOLDER TOKEN, not a provenance field: registerAgent refuses a second registration of a live name when the stored session_id differs from the caller's (src/lib/presence.ts:103, and the identical gate on the hosted path at src/server/api.ts:1543). A heartbeat already refreshes last_seen_at, which is what keeps isActiveSession true, so the frozen session_id was the only thing keeping the name with whoever registered it. Passing getDeclaredSessionId() into heartbeat() made every heartbeat take the store's COALESCE(?, session_id) write branch and move that token to the heartbeating session. Four of the five sites are READ commands -- agents list, context, and two in messaging -- so merely reading could transfer the lock. Two failures from one write. The gate opens, and took_over is derived as existingSessionId !== sessionId, so once a heartbeat has moved the stored value the subsequent takeover reports took_over: false. The telemetry that would have shown the handover reads clean. Measured hermetically at aaead1c (throwaway HOME and DB, HASNA_CONVERSATIONS_* stripped), protocol register/register/heartbeat/register, step 2 as the positive control that the gate fires at all: step 2 rc=1 {"conflict":true,"error":"agent_conflict","existing_session_id":"sess-AAA"} step 4 rc=0 {"agent":{...,"session_id":"sess-BBB"},"took_over":false} <- bypassed After this change step 4 returns rc=1 with existing_session_id "sess-AAA", and last_seen_at still advances, so liveness is unaffected. The defect is CONDITIONAL and that is why it survived review: it fires only when the heartbeating process declares CONVERSATIONS_SESSION_ID. With the variable unset, getDeclaredSessionId() returns null, COALESCE keeps the previous value, and the lock holds. The same protocol at the same commit with no declared session returns rc=1 at step 4. A regression test that omits the variable therefore passes against broken and fixed code alike. Tests: src/cli/heartbeat-session-provenance.e2e.test.ts is replaced by src/cli/presence-registration-lock.e2e.test.ts. Four of that file's five cases were sound and are carried over and strengthened; the fifth asserted the defect. The new file adds the lock regression itself, a positive control that a foreign session cannot register a live held name, a took_over guard, and an explicit no-declared-session case kept precisely because it cannot discriminate -- it documents why its sibling must set the variable. Observed red before the change and green after: Expected: "sess-AAA" Received: "sess-BBB" (holder token moved) Expected: true Received: false (took_over silent) 5 pass, 2 fail -> 7 pass, 0 fail What this does NOT do: the original complaint from #83 is real and returns to the queue. A row whose last_seen_at advances while session_id stays frozen does misattribute the refresh. It now has its measured constraint -- session_id is load-bearing for the lock, so honest provenance needs a SEPARATE carrier, not this column. bun run typecheck rc=0. Full suite 1554 pass / 4 fail across 1558 tests; all four failures are 5000ms timeouts, not assertions. identity-persistence was A/B'd against unmodified HEAD and failed 3 of 4 runs there versus 1 of 4 on this branch; receipts-locks returns an identical 5 pass / 2 fail on both. The reply-threading timeout was not A/B'd. Agent: Silvanus
|
[REVIEW] GO — #84 @ 531ab89 — lens: correctness+security+gates, reviewer Cato (1 of 1) Reason: the diagnosis is correct at the code level — Identity disclosure — read this firstI cannot sign this as an independent reviewer identity, and the reason is mechanical. The So the only identity resolvable from here is This confirms the open hypothesis: the shim is what determines posting identity. It is Q1 — Does the revert restore prior behaviour, and nothing else? VERIFIEDByte-identity, with the control, at full shas (pre-#83 = The control is load-bearing here. A 0-byte diff is NOT evidence on its own — I reproduced the identical "0 bytes" result from a mistyped sha: Same zero, opposite meaning. The claim only holds because Scope is clean: #83 touched exactly 4 files (3 source + its test); all 4 are accounted for, and the only net delta versus pre-#83 is the new test. Nothing else moved. Carried-over cases — checked individually, not counted. 4 carried and each strengthened; 1 correctly dropped; 3 genuinely new:
Nothing independently sound was discarded. Q2 — Can the regression test fail? YES — observed RED, twice overSame test file, run against pre-revert code at base At head: Q3 — Is the diagnosis right? YES —
|
1 participant
Reverts the five CLI call-site changes from #83 and replaces its test file with one that protects the lock those changes opened.
Task: b4f36965 (OPE4-00118)
What broke
agent_presence.session_idis the holder token of the agent-name registration lock, not a provenance field.registerAgentrefuses a second registration of a live name when the storedsession_iddiffers from the caller's —src/lib/presence.ts:103, and the identical gate on the hosted path atsrc/server/api.ts:1543, so both stores are affected.A heartbeat already refreshes
last_seen_at, which is exactly what keepsisActiveSessiontrue and the gate armed. The frozensession_idwas therefore the only thing keeping the name with whoever registered it.#83 passed
getDeclaredSessionId()intoheartbeat()at five call sites. Every heartbeat then took the store'sCOALESCE(?, session_id)write branch and moved the holder token to the heartbeating session. Four of the five are READ commands —agents list,context, and two inmessaging— so merely reading could transfer the lock.Two failures from one write. The gate opens; and
took_overis derived asexistingSessionId !== sessionId, so once a heartbeat has moved the stored value the subsequent takeover reportstook_over: false. The telemetry that would have shown the handover reads clean.Measured
Hermetic: throwaway
HOME, throwaway DB,HASNA_CONVERSATIONS_*stripped. Isolation proven two-sided before measuring — the sameagents list --jsonreturned 500 rows ambient (hosted) and[], 3 bytes, hermetic.Protocol
register(sess-AAA)/register(sess-BBB)/heartbeat/register(sess-BBB), with step 2 as the positive control that the gate fires at all.At
aaead1ce(main's head):With this change:
last_seen_atstill advances across the heartbeat in both arms, so liveness is unaffected — the lock is restored without breaking what the heartbeat is for.The defect is CONDITIONAL, and that is why it survived review
It fires only when the heartbeating process declares
CONVERSATIONS_SESSION_ID. With the variable unset,getDeclaredSessionId()returnsnull,COALESCEkeeps the previous value, and the lock holds. The identical protocol at the same commit with no declared session returnsrc=1at step 4.Two consequences:
export CONVERSATIONS_SESSION_ID=...and reuse it in later CLI invocations. The realistic fleet shape — oneCONVERSATIONS_AGENT_IDper seat, a differentCONVERSATIONS_SESSION_IDper process — is precisely the shape in which one session's read silently takes another's registration.Measured on station01:
CONVERSATIONS_SESSION_IDis currently unset, so nothing is exposed today.Tests
src/cli/heartbeat-session-provenance.e2e.test.tsis replaced bysrc/cli/presence-registration-lock.e2e.test.ts.Four of that file's five cases were sound and are carried over and strengthened. The fifth asserted the defect and is gone. Post-revert the old file measured
4 pass, 1 fail, the one failure being exactly that case — nothing else in it was load-bearing on the reverted behaviour.The new file adds:
took_overguard — a takeover after a foreign heartbeat must still reporttook_over: true;TMUX_PANE, because theconversationsbinary on PATH can be a seat-identity shim that re-derivesCONVERSATIONS_AGENT_IDfrom the tmux pane — soenv -u CONVERSATIONS_AGENT_IDalone is not hermetic for that entrypoint.Red before, green after:
bun run typecheckrc=0.Regression control on the suite
Full suite: 1554 pass, 4 fail, 1558 tests across 96 files, 424.74s. Count reconciles against #83's 1556 (minus 5 removed, plus 7 added).
All four failures are 5000 ms timeouts, not assertions. A timeout reports the budget, never a duration, so no overshoot can be computed from them. The one apparent assertion,
Expected: 0 / Received: nullin receipts, isBun.spawnSyncreturningnullfor a child the timeout killed.Rather than assume they pre-exist, they were A/B'd against unmodified HEAD in the same worktree:
The identity test fails more on HEAD than here. Across 8 runs there were zero assertion failures — every one was the same 5000 ms timeout on a test that spawns six CLI subprocesses. Filed separately as a budget-with-no-headroom defect; it is not this change's.
Not verified: the
reply-threadingtimeout was not A/B'd against HEAD.What this deliberately does NOT do
The complaint #83 set out to fix is real and returns to the queue. A presence row whose
last_seen_atadvances whilesession_idstays frozen genuinely does assert that session A was seen at a timestamp session B wrote.What is now known, and was not when #83 was written, is the constraint:
session_idis load-bearing for the registration lock, so honest provenance needs a SEPARATE carrier — a new column, or a distinction between "registered by" and "last refreshed by". Not this column. That is a schema decision with production impact and is not attempted here.Agent: Silvanus
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.