refactor(loop): single loop-queue-mode predicate, document non-unification with server accept gate (lr-6fe5fc) - #397
Conversation
…n-unification with server accept gate (lr-6fe5fc)
|
PEACHES - clean (0 findings) This PR correctly refactors duplicated client-side queue-mode logic into a single isLoopQueueSession derivation. The narrower framing is right. The task's original claim of a client/server predicate mismatch is wrong; AMoS correctly identified the real defect as independently-duplicated CLIENT-SIDE logic (two hand-rolled copies that drifted, causing lr-fd38ac). The server and client gates ARE intentionally different:
Extraction is faithful. The new 3-condition predicate replaces duplicated hand-rolled logic in recheckLoopQueueModeFromState identically. updateLoopInputVisibility changes behavior (drops per-session checks) but correctly: finishLoop ensures loopCurrentSessionId equals activeSessionId already implies session is active. Test line 112-116 validates this. Test is non-vacuous. Line 86-87 asserts isLoopQueueSession is exported; pre-fix that symbol did not exist, test would fail. Boundary conditions verify the logic. lr-fd38ac behavior holds: ended sessions stay resumable, queueing during running loop not regressed. |
|
BOBBIE -- clean Reviewed lib/public/modules/app-loop-ui.js, lib/project-loop.js, test/loop-queue-mode-predicate-lr-6fe5fc.test.js at head 7412eb4 (base main 61c5e9d). Server gate confirmed unchanged: lib/project-loop.js diff is comment-only insertion above the loop_message handler; the loop_message / loopState.active accept check is byte-identical to main. No logic altered. Client latch: isLoopQueueSession(loopActive, loopCurrentSessionId, activeSessionId) in app-loop-ui.js consolidates two hand-rolled copies (updateLoopInputVisibility, recheckLoopQueueModeFromState) into one exported predicate. The extraction drops the per-session loop.active and loop.role crafting-exclusion conjunct previously checked inline in updateLoopInputVisibility, on the documented claim that project-loop.js only ever sets loopCurrentSessionId to a coder or judge session and clears it atomically with loop.active in finishLoop(), making that conjunct always redundant with loopCurrentSessionId equal to activeSessionId. Verified this claim is test-enforced, not just asserted: the new test file explicitly passes a stale, contradictory loop object (active false, role crafting) through updateLoopInputVisibility() against store state that says the loop is active and current, and asserts loopQueueMode still comes out true; the removed conjunct is proven inert, not silently dropped. This remains a UI input-routing latch only; the security-relevant accept decision stays server-side and unchanged, consistent with the two prior audits of predecessor PR #396. No secrets, no new dependency manifest changes (package.json and package-lock.json untouched by this diff), no SAST hits. Scanners: gitleaks detect over main to head, no leaks found. semgrep config=auto over the three changed files, ran successfully this time (200 rules, 3 files, 0 findings; unlike the #396 final-SHA egress failure). osv-scanner not applicable, no dependency manifest changed in this diff. Findings: none. |
|
Merged via clagentic-loadout v0.2.0
|
What changed
Structural follow-up to lr-fd38ac (PR #396), scoped per lr-6fe5fc. Design pass first, per the task brief, then the minimal change the design actually supports.
Design pass summary
Enumerated every reader/writer of loopState.active (server global flag) and session.loop.active (per-session marker), client and server. Confirmed the load-bearing invariant PEACHES verified in PR #396 still holds structurally: loopCurrentSessionId/judgeSessionId are only ever set to a session whose own loop.active was just set true (coder/judge roles only), and finishLoop() atomically nulls loopCurrentSessionId in the same pass that clears every affected session's loop.active. Confirmed exactly 4 writers of the per-session marker (coder, judge, 2 crafting-branch sites) plus 1 scoped clearer, matching the task brief.
Key finding that shaped the design: the client's composer gate (loopQueueMode, in app-loop-ui.js) and the server's loop_message accept gate (handleLoopMessage in project-loop.js) are NOT computing the same predicate, and should not be forced into one. The server gate answers a coarser, already-correctly-single-sourced question: does the project have a running loop that will accept a queued message at all (loopState.active alone -- loop_message carries no sessionId; pendingUserMessages is one project-wide queue delivered at the next iteration boundary regardless of which session sent the WS message). The client gate answers a stricter UX question: should THIS session's composer show queue-mode UI (loopActive && loopCurrentSessionId === activeSessionId -- don't claim queued for the next iteration on a session that has nothing to do with that queue). Collapsing these into one shared function would misrepresent one question as the other and relocate the disagreement rather than remove it.
The actual disagreement that produced 4 bugs (lr-4165, lr-0cae, lr-e31b, lr-fd38ac) was never two different predicates computing different answers -- it was the client-side derivation being hand-rolled independently at two call sites (updateLoopInputVisibility() and the reactive recheckLoopQueueModeFromState()), with the per-session loop.active/loop.role fields still nominally checked at one site despite being provably redundant given the invariant above. Two independently-maintained copies of the same boolean is exactly the shape of drift that produced lr-fd38ac's own fold-in bug.
What actually changed
What is explicitly NOT unified, and why (design-pass deliverable)
Why
lr-6fe5fc: four sequential bugs (lr-4165, lr-0cae, lr-e31b, lr-fd38ac) all patched inside the same two-derivations-of-one-decision model, each patch adding a new way for the two halves to disagree. This closes the actual defect class (independently-maintained duplicate boolean expressions) without pretending the client and server answer literally the same question, which they do not and should not.
Tests
TASK: lr-6fe5fc