Repository navigation
fix: close the phase 4 and step 3b follow-ups - #256
Conversation
- StartTCP refuses a second call and a call after Stop, and checks that the host is a loopback IP literal before it binds. - MarkAuthenticated refuses any conn that is not TCP, a zero Conn too. - AuthState.Principal is nil-safe and reads "" for a nil state. - An attach whose raw client id starts with reserved-for-local: is refused, so no client can read itself as the reserved master. - The implicit close_tui / set_active_pane target and the most-recently-active pick (defaultCWD) skip read-only viewers; an explicit client id is still honoured. - The quild.log line for a TCP conn refused at accept is limited to one per minute with a count of the lines held back. Ref #250 Part of #234
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #256 +/- ##
==========================================
+ Coverage 77.55% 78.03% +0.47%
==========================================
Files 348 350 +2
Lines 47014 47676 +662
==========================================
+ Hits 36461 37202 +741
+ Misses 8341 8264 -77
+ Partials 2212 2210 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A loopback connect/close loop stays under the pending cap, so the refusal limit never ran and each conn still wrote two Info lines to quild.log. A TCP conn that has not logged in now logs its connect, disconnect and a failed write at Debug; the login is its first Info line. audit.log keeps its own capped record of those conns. The held-back refusal count is logged when the accept loop exits. close_tui logs "only read-only viewers" apart from "no client". logger.Save lets a test restore the previous logger. Ref #250 Part of #234
ParseToken and ValidNonce now accept only the canonical unpadded base64url form of exactly 32 bytes. The decoder skipped CR and LF and accepted set padding bits, so several strings passed for one value, and a nonce is signed as the string that crossed the wire. The token store keeps the first of two entries with one id and counts the rest as dropped, names a corrupt verifier in Admit's error (the daemon logs it; the client still reads "token refused"), tolerates a nil ExpireSweep callback, copies the caller's expiry in Create, and syncs the parent directory after the rename on Unix. Tests stop ignoring NewToken and Create errors, and the daemon's Stop test has its own doc comment back. Part of #250
A test that pointed the logger at a buffer put back a fixed level and io.Discard, so every later test in the package ran with a logger it never had. logger.Save restores what was there, nil handler included. Part of #250
TestBridge_ReplayOverTheTabCapClosesTooSlow sent five replay frames, but the bridge closes the tab at the third, and closing the fake daemon closes its input channel. When the bridge won the race the next send panicked with "send on closed channel" (seen on CI). feed checks and sends under the fake's lock, so it cannot interleave with Close. Part of #250
A stored or server key that decoded to anything but 32 bytes passed Verifier and read as a wrong proof, so a damaged entry left nothing in the log. It is now ErrCorruptVerifier like an undecodable one. The daemon's corrupt-verifier warning is limited to one line a minute with a held-back count, flushed at shutdown, since anyone on loopback can name the damaged id in a loop. Part of #250
The dialog's ordinary create_pane and every create_tab now carry a request id. The daemon answers such a create only when it refuses it (an error frame; success is still the next broadcast), so the 64-slot queue gains a frame only on refusal. The TUI flashes the sanitized reason, prunes the split placeholder and puts a replaced pane back: an ordinary replace now holds its old pane until the create settles instead of disposing it at send time. The create dialog's two raw-arguments gates read createPaneSendDest, which falls back to rightsDest, so a standard --connect token is checked before the first broadcast too. newTabWorktrees is keyed by destination and branch, so one host's answer cannot settle another host's same-named branch. Part of #250
- quil clients token list prints an EXPIRED column (the daemon's rule: expired at the expiry instant); an expired token stays listed. - redialTCPDest runs the version check. A mismatch refuses a destination that never attached and is logged on a mid-session reconnect, the rule --remote already follows. - sandbox login and remote setup name --connect under it. - LoopbackAddr says "names", not "would bind": --connect dials. Part of #250
- The create dialog records whether it pinned a destination, so a
dialog opened on the LOCAL project ("") keeps it after the active
project moves to a remote: the raw-arguments gate, the new-tab
worktree key, the create_tab send and the recent-dirs check all use
the pin.
- A create that re-arms a tab's reservation (dialog or split key)
first retires an earlier ordinary create's id and held pane, so its
late refusal cannot unwind the newer placeholder.
- Output for a pane held by a replace goes to the held pane, so a pane
a refusal puts back has no gap.
- create_tab ids come from nextReqGen, like create_pane's.
Part of #250
The create dialog retired a tab's earlier ordinary create before either arm had reserved anything. A split refused by SplitAtPane, or a replace whose pane has no leaf (the overlay is shown), then left the earlier create's placeholder armed with no id, and could dispose its held pane. The retire now runs inside each arm after its reservation succeeds, and an unreserved replace sends an unarmed id. Part of #250
A worktree split answers before its checkout starts, and the worker that swaps the finished pane in reads newSessionFn after the placeholder is gone. TestSplitPaneReq_WorktreeBranchPlaceholderThenSubstitute returned on that removal, so its cleanup restored the seam while the worker was still spawning: a data race under -race in CI. The create workers (worktree checkouts and template creations) now start through goCreateWorker, which counts them in createWG. The two test daemon helpers drain the conn handlers, then wait for those workers, before the seam is restored. Nothing in the daemon waits on the group. Also corrects create_req.go's comments: toggle arguments follow the order the caller named the toggles, not the plugin's declarations. Part of #250
- readonly_gates_test.go: the read-only gates no test reached, each driven through Update with full rights as the control: the other leavesViewerTab callers, notification d/D, sidebar drag presses, finishProjectDrag, commitGroupEdit, submitNewProject, palette rows built before the rights turned read-only (template, project rename), input history, restart, F1 Processes and take control. Ungroup and the move pickers are reachable only behind the context menu's execute-time check, so they are called directly. - canOpenOverlay's overlayRuns branch: a running overlay keeps its own rows live for a standard token. - Server.Start stops when the socket cannot be restricted (through a protectSocketFn seam), and CreatePrivateFile refuses an existing file on Windows. - The class-table sweep fails on a Msg* constant it cannot read instead of skipping it. Part of #250
- handleCreatePaneSplit has four refusals, not three, and only the first leaves before its teardown block. - Toggle arguments follow the caller's order (resolveToggles). - Only darwin and aix take ForkLock to create a socket. Part of #250
A split's checkout and a new tab's worktree start only after the answer or the broadcast has left, so a test holding that answer could Wait on createWG before the worker's Add. The handlers now hold a count across the send (holdCreateWorkers) and release it once the worker has started, which also drops the conn drain from the cleanup. Tests that swap newSessionFn after the helper register their own wait after the swap, so it runs before their restore. The template test that slept 300 ms to conclude no prompt was delivered now waits for the worker and for every pane's input queue to drain. goCreateWorker moves out of createFirstPaneWorktree's doc comment. Part of #250
The press never arms a tab drag for a read-only destination, but a drag armed before a reconnect re-applied read-only rights kept reordering on motion: the router dropped the reorder and this client's order diverged from its daemon's. The sidebar tab drag and the tab-bar drag now re-check on motion, flash and end the drag, as finishProjectDrag does on release. Tests: that case for both drags; the F1 Processes row is found by its label; the move-tab picker refusal asserts the flash. Part of #250
createTabFromReq's worktree arm and splitIntoWorktree's placeholder arm broadcast before the handler took the hold, so a client that saw the placeholder could still Wait ahead of the worker's Add. Both handlers now take it before the work runs and release it once the worker has started, as handleCreateTab already did. The doc on holdCreateWorkers says so. Part of #250
A confirmed Ctrl+R reload now loads the daemon's note even when its rev is lower than the editor's: a daemon that crashed inside its snapshot debounce restores an older rev, and the reload was dropped silently. A save answered since the confirmation still wins (the editor's rev is snapshotted with the buffer). Any other reload older than the editor is dropped with a flash. A note file LoadNotes cannot read keeps the notes kind pending, like the request budget, so the next launch reads it again. forgetImportFor now resets the destination's import error count, so a new connection gets its own retries, and drops the destination's pendingGroupOps, whose answers cannot arrive on a new connection. The failed-send retry of the import gets an Update-driven test on a real Router. The rules doc records why a parked host keeps the notes wait closed. Part of #247
A confirmed Ctrl+R whose answer an overwrite's answer already passed now flashes "Note reload replaced by a newer save" rather than the generic "older than the editor". The failed-send import test proves the import reached the send before it checks the retry state, and the unreadable-note test fails rather than skips when a symlink cannot be made outside Windows. Part of #247
The intake cap bounded the group list a daemon sends, but not the distinct names its projects carry in projects[].group. A host the user may not control could grow the sidebar without limit that way. A name the daemon does not list now counts against the same per-destination cap; a project past it is shown ungrouped and the cap logs once. Part of #247
Each group handler (set_project_group, group_op rename, shared_import) now gets a control, escape, C1, bidi and too-long name over real IPC. The project-name cap is driven through Update with two destinations, and the create-with-a-legacy-host-active test now types and saves the name through Update and checks the op sent to the local daemon. Part of #247
A twoDestModel router yields one link-lost per closed conn and then blocks, so the third frame's re-armed listen never returned and the package timed out. Frames and the Enter key now go through Update without running the returned Cmd; the sends are synchronous. Part of #247
The per-rebuild cap bounded one frame, not the session: names a project added that no daemon listed were never "vanished", so a host sending a new name set each frame grew the view and the saved file without limit. A daemon keeps every name its projects carry in its own list (SetProjectGroup, GroupOp, ImportShared, RestoreShared's repair), so an unlisted name now only joins a group the view already holds - listed, the user's own, or another host's - and otherwise the project is shown ungrouped, logged once per host. No state is carried between frames, and a known name no longer costs a host anything. Part of #247
ProtectDir rewrote QUIL_HOME's DACL on every daemon start, which re-propagated it through the whole tree each time, and it did so to any folder QUIL_HOME pointed at. It now reads the DACL back first and writes nothing when it already is the protected owner-only DACL (ownerOnlyDACL, a pure SDDL check). Before a write it refuses, with ErrNotQuilHome, a folder holding a top-level entry quil does not write; the daemon then warns in quild.log and starts no TCP listener (homeUnprotected). A marker-file test cannot decide this: quild writes quild.pid before Start and shellinit/ after ProtectDir, so any folder would pass from its second start. CI now cross-compiles and vets internal/ipc for Windows, so these files and their tests are at least type-checked. Part of #250
A crash in SaveTemplates leaves .templates-* at the top of QUIL_HOME, which the guard read as foreign and so refused a real quil folder. Any dot-prefixed entry now counts as quil's: an arbitrary folder always holds non-dot entries too, so detection loses nothing. The refusal now also says to move or remove the named entry, which is the fix for the default ~/.quil. A drift test checks every config path helper against the exported ipc.IsQuilHomeEntry, so a new top-level file cannot silently refuse a fresh home. Part of #250
A re-login whose version reply never came said "runs unknown - version mismatch" and sent the user off to upgrade a daemon that may already run this version. It now says no version reply came. The daemon's "tcp client logged in" line now names the peer and the token id, so two logins from one host can be told apart in quild.log. Part of #250
A background note reload that comes back older than the editor is dropped without a flash again: the editor already holds the newer text. Only a confirmed reload that is dropped says so. A split or replace whose active tab moved to another daemon while the dialog was open is refused with a flash. The dialog's gates checked the pinned daemon, so sending to another one skipped them. Part of #247
The tab-switch frame in the fixture leaves a flash of its own, which the silent-drop assertion read as the reload's. Part of #247
The tui test package declares its own min(int, int), so the builtin min over two durations failed go vet for the test build.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comments Outside DiffThese findings could not be posted inline.
|
A link loss closes an open context menu, so a menu opened before the park cannot reach its executor. The tests now check that the greyed rows and the mute key send nothing, and that a sidebar drag of a parked host's project onto a group header changes nothing.
A rename of a host's group made the new name the user's before any daemon answered. A refused rename, or one sent toward a host whose link was down, therefore stayed in the view and the groups file for good, and the host's old name came back beside it. The new name is still the user's while the daemons answer, so an in-flight frame cannot take it. sendGroupRename counts the fan-out and settles it: once a daemon accepts, the group gets its own origin back; when none does (refused, not sent, or lost with the link) the old name is put back, folding in a copy an in-flight frame re-added. A rename toward a host whose link is down is refused before anything changes.
The previous fix guessed outcomes. A rename held behind the groups import was replayed without its bookkeeping, so it never settled. A reply lost with the link was read as a refusal, so the old name came back while the daemon had the new one, and the next frame deleted the group and added it again, dropping its order and collapsed state. A rename no longer touches the group's origin. groupRename records the daemons it went to, and only they settle it: an accept, a refusal (the same group gets the old name back), or after a lost reply the first frame on the new connection. Until then rebuildGroupsView reads their lists and project groups with the new name in place of the old, so an in-flight frame cannot drop it. A replayed op finds its rename by its payload. The sendGroupRename/origin bookkeeping and the groups save on link loss are removed.
A two-destination router's listen loop blocks once both fake conns have reported closed, and updateWith runs every returned command to completion, so the deferred-rename test hung until the 5 minute timeout. updateNoWait abandons a command after 200 ms.
A rename op was found again by its names, so A->B, B->A, A->B in
flight attached the third request to the first rename, and a later
answer settled the wrong one. A host disconnected by the user while
its answer was pending was marked lost, but it never sends the frame
that decides a lost answer, so the rename stayed pending for good.
Each rename op now carries its rename by request id: sendGroupRename
files the rename with the id sendSharedOpWith mints, and a held op
carries it into the replay. A group with a rename still pending
refuses a second rename ("rename still waiting for <host>"). A host
that leaves (disconnectDest) settles its part at once through
leaveGroupRenames: with no host left, any accept keeps the new name,
and none puts the old one back. A lost link still waits for the next
frame.
A rename held behind a host's groups import was put back when the user disconnected the host, but its op stayed in deferredGroupOps. Re-adding the host and finishing the import then sent the abandoned rename. disconnectDest now drops every op held for that host, with its held names, where leaveGroupRenames runs. A lost link still keeps them for the replay after the redial.
code-spire-beaver
left a comment
code-spire-beaver
left a comment
There was a problem hiding this comment.
Re-reviewed all 11 commits since dfa9898, including the new group-rename settlement, disconnected-host guards, error-flash timing, and worktree-name feedback. The earlier Disconnect documentation finding is corrected. One new rename race can discard saved group order and collapsed state; the shared-group instructions also describe the superseded implementation.
Findings: 1 MEDIUM, 1 LOW. Reviewed head 189b65c6ac as @code-spire-beaver.
code-spire-beaver
left a comment
code-spire-beaver
left a comment
There was a problem hiding this comment.
❌ CHANGES REQUESTED
Re-reviewed PR #256 at 189b65c6ac51fa26b552336172e1a4d4d906905d, covering the complete 11-commit, 32-file delta from dfa989887cd238ebe295cf0702741f41f7e7e35a. PR base: 767218a1b6a9e3ef250c02844327c0ef4798f6c4. Service/feature: quil/followups-250-247.
Request changes for one MEDIUM correctness finding, with one LOW documentation finding. The earlier rules/L-2 Disconnect documentation finding is verified fixed; its thread was already resolved. The other five previously verified findings remain resolved and their fixes remain intact in the reviewed paths.
Findings:
- code-quality/M-2 — Keep the rename alias until the daemon frame catches up.
internal/tui/sharedgroups.go:141. An OK response removes the alias while the cached group list and project membership still carry the old name. An unrelated host frame before the renamed host's next frame makes reconciliation delete the renamed host-owned group; the later frame appends a fresh group, permanently losing its position and collapsed state. The daemon sends its answer before scheduling the 50 ms coalesced broadcast, making this an ordinary multi-host interleaving. Keep the alias until a confirming frame and add that event sequence to the accepted-rename regression. - rules/L-3 — Update the shared-group rule for daemon-settled renames.
.claude/rules/projects.md:964. The instructions still say a rename changes origin to user and a refusal leaves the group split. Document the new preserved origin, pending aliases, settlement and rollback instead.
Per-lens results:
- Security: no new findings. Reviewed request-id and destination correlation for rename responses, router-owned origin, preservation of read-only and raw-argument rights gates, group-name canonicalization and per-host caps, persisted provenance and membership admission, remote error sanitization, and destination cleanup on explicit disconnect versus link loss. The new aliases are scoped to destinations the user targeted. Semgrep was skipped under the shared-host validation policy; security validation used manual tracing and the current CodeQL results.
- Correctness: one MEDIUM finding above. Traced group renames through dialog commit, direct and deferred sends, replies, frame reconciliation, reconnect and explicit disconnect, including the daemon's reply/broadcast order. Also checked flash priority and expiry, clock use, link-down guards at action entry and commit, worktree validation/focus/rendering, and the newly extracted field-index helper and its callers.
- Rules: one LOW finding above. Applied the matching project/global instructions, checked the changed behavior against the changelog and scoped documentation, and inspected all 11 commit messages. No new issue-closing reference or authorship trailer was introduced. The working repository remains unchanged.
- Tests/build: the missing interleaving is included in
code-quality/M-2, rather than duplicated as a separate finding. Reviewed all new and modified tests and the shared fixtures: flash duration/priority, reachable parked-host menu/key/drag paths, worktree validation on blur and submit, accepted/refused/lost/deferred renames, request-id reuse, second-rename refusal, and disconnect/re-add settlement. The accepted-rename test currently feeds OK and the renamed frame consecutively, so it misses the stale-cache interval. Regression assertions were assessed statically; no local mutation probe was run.
All 12 current checks pass. The CI test log identifies this PR head in merge commit 375bb89 with the pinned base. It reports 45 passing packages in the race/coverage suite and 45 in the integration/race suite; these are package counts, not individual test counts. The TUI package passes both, with 82.7% statement coverage in the coverage run. Go vet, Linux quil/quild builds, the Windows build/vet checks for notify, quil-activate and IPC, shellcheck, and installer/changelog/sandbox-image regression checks also pass.
Context applied: supplied AGENTS instructions, .claude/CLAUDE.md, CONTRIBUTING.md, and the retained unchanged context from earlier full-PR reviews. This delta matches .claude/rules/{auto-update,client-auth,dev-environment,plugins,projects,remote-dialogs,sandbox,templates,tui-dialogs,tui-rendering,web}.md; related daemon-lifecycle and remote-transport context informed tracing of the unchanged send/broadcast paths. Applied global {blog-material-capture,documentation-maintenance,go-conventions,go-testing,no-synthetic-data,resilience-patterns,tech-debt-tracking}.md within the review's read-only scope and the supplied Git workflow. Read all four reviewer checklists and repository memory indexes, including the lessons on remote daemon taint, per-destination cleanup, field producers/consumers, documentation scope versus falsehood, and isolated mutation probes. Global reviewer memory indexes, the named Stukans memory and branch-specific review state remain absent.
Validation limits: no local builds, tests, linters or mutation probes were run, following the repository's CI-only build instructions. The rename finding is established by the code's event sequence; it was not exercised in a local runtime. Interactive terminal behavior was not manually tested. The checkout differs from the reviewed head and was left unchanged; review sources and artifacts are outside the repository.
Agent findings: 6 resolved, 2 open. CI: 12/12 checks passing. Head 189b65c6ac, reviewed by @code-spire-beaver.
Open findings:
- [code-quality/M-2] MEDIUM — #256 (comment)
- [rules/L-3] LOW — #256 (comment)
code-spire-beaver
added
review: changes-requested
The daemon answers group_op before its coalesced broadcast, so after an OK its list and its projects still carry the old name. The OK removed the alias at once, and another host's frame in between re-added the old name and deleted the renamed group; the renamed host's frame then added a fresh group at the end, losing its place and collapsed state. An OK now marks the destination renameAccepted and keeps its alias; the next frame from that destination retires it. A second rename of the group waits for that frame too.
The shared groups paragraph still said a rename made the group the user's, that this protected it from an in-flight frame, and that a refusal left the group split. It now describes preserved provenance, per-destination aliases matched by request id, one rename per group in flight, settlement by reply then frame, the rollback when no host accepts, and held ops dropped on an explicit disconnect.
code-spire-beaver
left a comment
code-spire-beaver
left a comment
There was a problem hiding this comment.
Re-reviewed the two commits since 189b65c at f94643c across security, correctness, project rules and tests. The previous two-host rename reproducer and the inaccurate documentation clauses are addressed. One MEDIUM correctness finding remains: a pre-rename snapshot from the same daemon can arrive after OK and prematurely retire the alias. All 12 checks pass; validation used source tracing and CI logs under the repository's CI-only policy.
Findings: 1 MEDIUM. Reviewed head f94643cf51 as @code-spire-beaver.
code-spire-beaver
left a comment
code-spire-beaver
left a comment
There was a problem hiding this comment.
❌ CHANGES REQUESTED
Re-reviewed PR #256 at f94643cf51486033e3fbb03f72321fbd4629d944, covering the complete two-commit, four-file delta from 189b65c6ac51fa26b552336172e1a4d4d906905d. PR base: 767218a1b6a9e3ef250c02844327c0ef4798f6c4. Service/feature: quil/followups-250-247.
Request changes for one MEDIUM correctness finding. The original two-host rename reproducer (code-quality/M-2) and the incorrect provenance/refusal documentation (rules/L-3) are addressed. Both prior threads were already resolved; verification is scoped to those concrete findings. The remaining same-daemon ordering case is tracked in a new thread on the new settlement branch.
code-quality/M-3 — Retain the rename alias across delayed pre-rename frames, internal/tui/sharedgroups.go:190 (correctness and test coverage). A snapshot taken before the rename can be queued after its OK response: the daemon releases its state lock between building and broadcasting a frame, and replies independently. With the client at revision 1, OK -> old-name revision 2 -> renamed revision 3 passes the frame revision filter. Revision 2 becomes renameAcceptedSeen, prematurely removes the alias, and makes reconciliation delete the renamed host-owned group. Revision 3 appends a fresh group, losing its original position and collapsed state. Keep the alias until a frame is known to include the operation, using a causal revision barrier or equivalent, and test this ordering. The existing pane-seen code documents and handles the same producer-ordering hazard at internal/daemon/daemon.go:4491-4496.
Per-lens results:
- Security: no new findings. Checked that the added states preserve request-ID/destination correlation, destination-scoped aliasing, capped/canonical daemon lists, membership admission and cleanup. No new wire fields, dependencies or authority boundaries were introduced in this delta. Semgrep skipped under the shared-host validation policy; reviewed current CodeQL results and traced the paths manually.
- Correctness: one MEDIUM finding above. Reviewed every changed hunk and the complete rename state transitions, including success/refusal, direct/deferred sends, frames before/after replies, a second rename, link loss and explicit disconnect. Traced snapshot creation and IPC enqueue separately to check the new assumption about the next frame.
- Rules: no separate new findings. Verified the corrected shared-group documentation and both commit messages against project instructions. The next-frame assumption in the updated prose is part of M-3. No forbidden issue-closing reference or authorship trailer was introduced. The working repository remains unchanged.
- Tests/build: the modified accepted-rename test now checks the other-host gap, and the second-rename test checks refusal after OK but before the confirming frame. Those added assertions distinguish this revision from the previous head. The other fixture updates preserve existing settlement/request-ID/disconnect coverage. The missing delayed same-daemon frame sequence is included in M-3. Regression discrimination was assessed statically; no local mutation probe was run.
All 12 current checks pass. The CI test log identifies merge commit 1edd918b2e983dca7d78bb802b7be0638afe8f7c containing the reviewed head and pinned base. The full race/coverage and integration/race runs each report 45 passing packages, not individual test counts. TUI passes both, with 82.7% statement coverage in the coverage run. Go vet, Linux quil/quild builds, Windows notify/quil-activate/IPC build and vet checks, shellcheck, and installer/changelog/sandbox-image regression phases also pass.
Context applied: supplied AGENTS instructions, .claude/CLAUDE.md, retained unchanged root/project context, and .claude/rules/dev-environment.md (the project path rule matching this delta). Read the changed .claude/rules/projects.md instructions and consulted the retained daemon-lifecycle.md, remote-transport.md and tui-rendering.md context for the unchanged callers. Applied global ~/.claude/rules/{blog-material-capture,documentation-maintenance,go-conventions,go-testing,no-synthetic-data,resilience-patterns,tech-debt-tracking}.md within the review's read-only scope. Used all four reviewer checklists and repository memory indexes. Relevant lessons consulted include security-officer/{project_remote_daemon_taint,project_per_dest_table_cleanup}.md, code-reviewer/project_trace_wire_fields_end_to_end.md, qa/{project_mutation_probe_on_a_copy,project_ingest_arrival_order_gaps,project_ipc_write_window_wiring_gap}.md, and rules-compliance/claude-md-staleness-scope-vs-falsehood.md under .claude/agent-memory/. Global reviewer memory indexes, the named Stukans memory and the branch-specific review-state file are absent.
Validation limits: no local builds, tests, linters or mutation probes were run, following the repository's CI-only build instructions. The finding is established by source-level event tracing; the missing ordering was not reproduced in a local runtime. Interactive terminal behavior was not manually tested. The checkout is at a different commit and remains untouched; review sources and artifacts are outside the repository.
Registered review: #256 (review)
One new inline thread posted: #256 (comment)
Two previous findings verified on already-resolved threads; no additional resolution transition.
Agent findings: 8 resolved, 1 open. CI: 12/12 checks passing. Head f94643cf51, reviewed by @code-spire-beaver.
Open findings:
- [code-quality/M-3] MEDIUM — #256 (comment)
After its OK the daemon can still broadcast a snapshot built before the rename, since buildWorkspaceState releases its lock before sending. The first frame after the OK retired the alias, so OK, then an old-name frame, then the renamed one deleted the group and re-created it at the end, without its place or collapsed state. An accepted alias now retires when the daemon's list shows the rename: the new name listed (the daemon refuses a rename onto a name it holds, so no pre-rename snapshot lists it), or neither name (renamed, then deleted). renameAcceptedSeen is removed.
code-spire-beaver
left a comment
code-spire-beaver
left a comment
There was a problem hiding this comment.
Re-reviewed the one-commit, three-file delta at 6427e93 across security, correctness, project rules and tests. The previously reported stale same-daemon frame after OK is fixed and now covered by the accepted-rename regression. One MEDIUM regression remains: an accepted rename can stay pending indefinitely across reconnect when the daemon now legitimately uses the old name. All 12 checks pass; validation used source tracing and CI logs under the repository's CI-only policy.
Findings: 1 MEDIUM. Reviewed head 6427e93122 as @code-spire-beaver.
code-spire-beaver
left a comment
code-spire-beaver
left a comment
There was a problem hiding this comment.
❌ CHANGES REQUESTED
Re-reviewed PR #256 at 6427e931229ebcb80ae60e45fcc4752c3cf71760, covering the complete one-commit, three-file delta from f94643cf51486033e3fbb03f72321fbd4629d944. PR base: 767218a1b6a9e3ef250c02844327c0ef4798f6c4. Service/feature: quil/followups-250-247.
Request changes for one MEDIUM correctness finding. The previous stale-frame finding, code-quality/M-3, is verified fixed: an old-name frame arriving after OK now keeps the alias, and the regression checks that exact sequence. Its thread was already resolved. The other previously verified fixes remain intact in the reviewed paths.
code-quality/M-4 — Settle accepted renames on a fresh connection, internal/tui/sharedgroups.go:203 (correctness and test coverage). After Infra -> Ops / OK / link loss, the daemon can legitimately have Infra again when the client reconnects: another client can rename it back while this client is offline, or a crash can restore a snapshot from before the rename. The OK removed the request from pendingGroupOps, so forgetImportFor never transitions this accepted rename to renameLost. The new old-name check then keeps the alias on every fresh frame, displaying Ops and refusing all further renames as still pending. Include accepted-but-unconfirmed renames in connection-loss/reconnect settlement and test OK -> linkLost -> old-name reattach, while preserving protection from stale frames on the original connection.
Per-lens results:
- Security: no new findings. Checked destination/request-ID correlation, alias scoping, canonicalized and capped daemon lists, membership/provenance rules, and explicit disconnect cleanup. This delta changes no authentication gate, wire field, dependency or external API. Semgrep skipped under the shared-host validation policy; manual analysis and current CodeQL results were used.
- Correctness: one MEDIUM finding above. Read every changed hunk and the complete shared-groups implementation, and traced its callers through rename commit, replies, deferred imports, frame reconciliation, link loss and reconnect. Checked all references to the removed enum state. Compared the reconnect sequence against the previous head: it previously retired the accepted alias on the first frame, whereas this revision can retain it indefinitely.
- Rules: no separate new findings. The shared-group rule is updated in the same commit to describe content-based settlement. Checked the scoped project/global rules and the commit message. No new issue-closing reference or authorship trailer was introduced. The checkout remains unchanged.
- Tests/build: the new old-name frame after OK in
TestGroupRename_Acceptedexercises the original defect through Update and checks membership, order and collapsed state. Its assertions distinguish this revision from its parent. Read the full rename-settlement test file and retained shared fixtures. The existing reconnect tests cover a lost reply, without a preceding OK, so they do not cover the accepted state in M-4. That coverage gap is included in the correctness finding. Test discrimination was assessed statically; no local mutation probe was run.
All 12 current checks pass. The CI test log identifies merge commit 41d675f containing the reviewed head and pinned base. The full race/coverage and integration/race suites each report 45 passing packages, not individual test counts. TUI passes both, with 82.7% statement coverage in the coverage run. Go vet, Linux quil/quild builds, Windows notify/quil-activate/IPC build and vet checks, shellcheck, and installer/changelog/sandbox-image regression phases pass.
Context applied: supplied AGENTS instructions, unchanged .claude/CLAUDE.md and root context, .claude/rules/dev-environment.md (the project path rule matching this delta), and the changed .claude/rules/projects.md. Retained related daemon-lifecycle.md, remote-transport.md and tui-rendering.md context informed caller tracing. Applied global ~/.claude/rules/{blog-material-capture,documentation-maintenance,go-conventions,go-testing,no-synthetic-data,resilience-patterns,tech-debt-tracking}.md within the review's read-only scope. Used all four reviewer checklists and the unchanged repository memory indexes, including .claude/agent-memory/security-officer/{project_remote_daemon_taint,project_per_dest_table_cleanup}.md, code-reviewer/project_trace_wire_fields_end_to_end.md, qa/{project_mutation_probe_on_a_copy,project_ingest_arrival_order_gaps,project_ipc_write_window_wiring_gap}.md, and rules-compliance/claude-md-staleness-scope-vs-falsehood.md. Global reviewer memory indexes, the named Stukans memory and the branch-specific review-state file are absent.
Validation limits: no local builds, tests, linters or mutation probes were run, following the repository's CI-only build instructions. The reconnect finding is established by tracing the state transitions; it was not reproduced in a local runtime. Interactive terminal behavior was not manually tested. The checkout is at a different commit and remains untouched; review sources and artifacts are outside the repository.
Registered review: #256 (review)
One new inline thread posted: #256 (comment)
One previous finding verified on an already-resolved thread; no additional resolution transition.
Agent findings: 9 resolved, 1 open. CI: 12/12 checks passing. Head 6427e93122, reviewed by @code-spire-beaver.
Open findings:
- [code-quality/M-4] MEDIUM — #256 (comment)
An OK removed the rename's op from pendingGroupOps, so a link lost before the confirming frame never marked it lost. If the daemon's truth moved meanwhile (a crash restoring a pre-rename snapshot, another client renaming back), the reconnect frame listed only the old name, the content rule kept the alias for good, and every later rename was refused as still waiting. forgetImportFor now turns an unconfirmed OK into a lost answer (acceptedRenamesLost), so the first frame on the new connection decides by content; the old-name protection stays for frames on the OK's own connection. accepted is set only when a daemon settles the rename, and a host the user disconnects keeps its OK as an accept, so leaveGroupRenames now runs before forgetImportFor.
code-spire-beaver
left a comment
code-spire-beaver
left a comment
There was a problem hiding this comment.
Re-reviewed the one-commit, five-file delta at 8298880 across security, correctness, project rules and tests. The remaining reconnect finding is fixed: accepted renames awaiting confirmation now enter reconnect settlement, and explicit disconnect retains an OK before cleanup. The old-name reconnect regression checks restored membership, order, collapsed state and the ability to rename again. No new findings; all 12 checks pass.
Findings: none. Reviewed head 8298880af2 as @code-spire-beaver.
code-spire-beaver
left a comment
code-spire-beaver
left a comment
There was a problem hiding this comment.
✅ APPROVED
Re-reviewed PR #256 at 8298880af256c7d39c1d595ec1a2ea45b6bd54ba, covering the complete one-commit, five-file delta from 6427e931229ebcb80ae60e45fcc4752c3cf71760. PR base: 767218a1b6a9e3ef250c02844327c0ef4798f6c4. Service/feature: quil/followups-250-247.
Approve. No new findings. The remaining reconnect finding, code-quality/M-4, is verified fixed; its thread was already resolved. All ten previous findings are now resolved and verified, with no open review blockers.
An OK waiting for a confirming frame is now tracked separately from a settled acceptance. On link loss, acceptedRenamesLost includes those entries in reconnect settlement even after their requests have left pendingGroupOps. A fresh old-name frame can restore the original group and allow another rename. Explicit disconnect settles the current answer before import cleanup can mark it lost, preserving its accepted outcome.
Per-lens results:
- Security: no new findings. Checked destination scoping of the new cleanup loop, request-ID/destination correlation, retained canonicalization and group caps, and explicit disconnect cleanup. Other destinations' pending renames and settled outcomes remain untouched. This delta adds no wire field, dependency or authentication surface. Semgrep skipped under the shared-host validation policy; used manual tracing and the current CodeQL results.
- Correctness: no new findings. Reviewed every changed hunk, complete changed methods and callers, and every read/write of the changed accepted flag. Traced both cleanup entry points (link loss and reattach), repeated cleanup, accepted versus unanswered requests, old/new-name reconnect frames, multi-host settlement, and explicit disconnect. The reordered disconnect still saves the final group view through its confirmation handler. The previous same-connection stale-frame protection remains intact.
- Rules: no new findings. The shared-group documentation describes the new connection boundary and disconnect ordering in the same commit. Checked the commit subject/body and matching project/global instructions; no forbidden issue-closing reference or authorship trailer was introduced. The checkout remains unchanged.
- Tests/build: the new old-name reconnect test exercises the reported failure through Update and redial-result handling, checks membership/order/collapsed state, requires pending state to clear, and verifies that another rename is sent. By source comparison, it fails against the previous head. The new-name case is a compatibility control that also passes the previous head; it checks preservation of the successful outcome through the same reconnect path. Reviewed the helper and existing accepted/refused/lost/deferred/disconnect tests. No local mutation probe was run.
All 12 current checks pass. The CI test log identifies merge commit fd845b5 containing the reviewed head and pinned base. The full race/coverage and integration/race suites each report 45 passing packages, not individual test counts. TUI passes both, with 82.7% statement coverage in the coverage run. Go vet, Linux quil/quild builds, Windows notify/quil-activate/IPC build and vet checks, shellcheck, and installer/changelog/sandbox-image regression phases pass. The pinned diff also passes Git's whitespace check.
Context applied: supplied AGENTS instructions, unchanged .claude/CLAUDE.md and root context, and matching project rules .claude/rules/{dev-environment,projects}.md. Retained related daemon-lifecycle.md, remote-transport.md and tui-rendering.md context informed caller tracing. Applied global ~/.claude/rules/{blog-material-capture,documentation-maintenance,go-conventions,go-testing,no-synthetic-data,resilience-patterns,tech-debt-tracking}.md within the review's read-only scope. Used all four reviewer checklists and the unchanged repository memory indexes, including the lessons on per-destination cleanup, remote daemon taint, tracing producers and consumers, test wiring/arrival order, isolated mutation probes, and documentation scope versus falsehood. Their files are .claude/agent-memory/security-officer/{project_per_dest_table_cleanup,project_remote_daemon_taint}.md, code-reviewer/project_trace_wire_fields_end_to_end.md, qa/{project_mutation_probe_on_a_copy,project_ingest_arrival_order_gaps,project_ipc_write_window_wiring_gap}.md, and rules-compliance/claude-md-staleness-scope-vs-falsehood.md. Global reviewer memory indexes, the named Stukans memory and the branch-specific review-state file are absent.
Validation limits: no local builds, tests or mutation probes were run, following the repository's CI-only build instructions. Test discrimination and additional state sequences were assessed statically; interactive terminal behavior was not manually tested. The checkout is at a different commit and remains untouched; review sources and artifacts are outside the repository.
Registered review: #256 (review)
No new inline threads. One previous finding verified on an already-resolved thread; no additional resolution transition.
Agent findings: 10 resolved, 0 open. CI: 12/12 checks passing. Head 8298880af2, reviewed by @code-spire-beaver.
code-spire-beaver
added
review: approved
2 participants
Closes the parked follow-up items of phase 4 (client auth, #250) and step 3b (shared data, #247), each fix with a test that fails without it.
Auth, IPC, daemon (#250)
MarkAuthenticatedonly for TCP conns;Principal()nil-safe ("").close_tui/set_active_pane/ default-cwd picks skip read-only viewers; an explicit id still reaches one.ProtectDir(Windows): reads the ACL back and writes nothing when it is already owner-only; aQUIL_HOMEholding non-quil entries is left alone, warned about, and gets no TCP listener. Verified natively on Windows 10 (14 tests). CI now builds and vetsinternal/ipcfor Windows.TUI and CLI (#250)
--connecttexts,quil clients token listEXPIRED column, version gate on TCP redial, neutral loopback error, new-tab worktree requests keyed by destination.Shared data (#247)
Also fixed on the way: a data race in a daemon test (create workers vs seam restore) and a race in a gateway test.
Not in this PR: a read-only viewer following one chosen client (stays open on #250).
Ref #250
Ref #247
Part of #234