fix(loop): clear loop.active on every loop session, gate resume on global state (lr-fd38ac) - #396
Conversation
…obal state (lr-fd38ac)
|
Reviewed lib/project-loop.js finishLoop() cross-session write, lib/public/modules/app-loop-ui.js client-side loop-message gate, and test/loop-resume-ended-session-lr-fd38ac.test.js against base..head (863f359..d3402b8). finishLoop() now iterates sm.sessions and calls saveSessionFile() on every session matching loopState.loopId with loop.active===true (lib/project-loop.js:788-795). sm is project-scoped (created per-project inside createProjectContext -> createSessionManager, lib/project.js), and the write is further filtered by exact loopId equality, not just project membership -- confirmed by the new test asserting a session from a different loopId is never touched or saved (test/loop-resume-ended-session-lr-fd38ac.test.js:163-165,182-183,194-195). saveSessionFile/rewriteMetaLineInPlace (lib/sessions.js:145-177) only rewrites the meta line of the session own on-disk file via atomic tmp-file+rename at 0o600, keyed by the session cliSessionId -- no path injection, no cross-session content exposure, no privilege field written (loop.active is a UI-latch boolean, not an authz grant). The client-side gate change (lib/public/modules/app-loop-ui.js updateLoopInputVisibility) is UI routing only. The server loop_message handler (lib/project-loop.js:979-983) independently re-validates loopState.active and rejects with an error when no loop is running, regardless of what the client latch computed -- so a stale or bypassed client gate cannot cause an unauthorized state transition server-side; worst case is exactly the bug being fixed (a normal send misrouted client-side, then correctly rejected server-side). No dependency manifest changes in this diff -- osv-scanner not applicable. Scanners: gitleaks detect (0 leaks), trufflehog git (0 verified/unverified secrets), semgrep --config=p/javascript --config=p/security-audit on the 3 touched files (0 findings, 83 rules, 3/3 targets). Zero findings. |
|
PEACHES — blocking FindingsPart 2 (lib/public/modules/app-loop-ui.js): Missing re-evaluation on loop state changes — blockingIssue: The new four-gate logic depends on global store values set by loop_started and loop_iteration handlers, but those handlers never call updateLoopInputVisibility to re-arm loopQueueMode. Scenario: Start a loop from the session you are currently viewing.
Consequence: The iteration SDK input stream is single-turn and closed (input.js:239). With loopQueueMode stale-false, input.js:244 routes send as message instead of loop_message and the user message is dropped instead of queued. Same class of bug lr-e31b fixed, reintroduced through new global-state dependency. Evidence:
Fix: Add updateLoopInputVisibility to subscribe block when loopActive or loopCurrentSessionId changes. Retrieve current session loop object. Add regression test that sets globals without direct call and asserts loopQueueMode follows. Verified CleanPart 1 (lib/project-loop.js): All four writers set loopId. forEach loop correctly filters and persists. No violations. Test Part 1: Creates multi-session scenario, invokes finishLoop via real timer. Not vacuous. Test Part 2: Direct calls with pre-seeded store. Do NOT cover re-evaluation bug. Brand, dependencies, paths: Clean on all rules. |
…udging move loopCurrentSessionId (lr-fd38ac) PEACHES-blocking on PR #396 Part 2: loop_iteration and loop_judging (app-messages.js) write loopActive/loopCurrentSessionId via store.set() without re-invoking updateLoopInputVisibility(). A client already viewing the session that becomes the loop's own current session (e.g. it started the loop from that exact session) was left with a stale loopQueueMode: false even once every gate condition became true - silently dropping a message that should have queued for the next iteration boundary, the inverse of the bug this PR fixes and the same class lr-e31b previously addressed. Fix: app-loop-ui.js's store.subscribe() now also reacts to loopActive/loopCurrentSessionId/activeSessionId and re-derives loopQueueMode from store state alone (recheckLoopQueueModeFromState()). This is safe without the per-session loop object updateLoopInputVisibility()'s other callers pass in because of a server-side invariant in project-loop.js: loopCurrentSessionId is only ever set to a session whose own loop.active is true and role is coder/judge (never crafting, never stale - finishLoop() clears loopCurrentSessionId to null in the same pass that clears every affected session's loop.active). Once loopCurrentSessionId === activeSessionId, the viewed session IS the loop's own current session. Chose reactive re-derivation over explicit re-invocation at the two handlers because it also required duplicating the invariant's reasoning into three call sites and each site remembering the future gate change; the subscribe path fixes it once for every current and future writer of those two fields. Also fixed loop_judging, which had the identical gap (same file, same pattern, same root cause - fold-in per AMoS contract). Required test added (loop-resume-ended-session-lr-fd38ac.test.js): drives the state transition via plain store.set() calls exactly as loop_started/loop_iteration do, never calling updateLoopInputVisibility() directly, and asserts loopQueueMode follows automatically through the new reactive subscriber. Confirmed failing before this fix (only this new assertion failing) and passing after. Also covers the inverse: an ended loop (loopActive:false, loopCurrentSessionId:null) does not re-arm queue mode through the same reactive path. Tests: npm test - 1411 tests, 1410 pass, 0 fail, 1 skipped (full suite, clean). TASK: lr-fd38ac
|
PEACHES — clean Verified the previous blocking finding is resolved. The reactive subscription (initLoopQueueModeSync) correctly re-arms loopQueueMode when loop_iteration/loop_judging set loopCurrentSessionId, without requiring explicit re-invocation at each handler. Server-side invariant verified: loopCurrentSessionId only points to sessions with loop.active=true and role="coder"/"judge" (project-loop.js runNextIteration/runJudge/finishLoop). finishLoop() atomically clears loopCurrentSessionId to null AND flips loop.active to false on every affected session in the same pass (lines 776-795). Reactive path safe: store.subscribe(['loopActive', 'loopCurrentSessionId', 'activeSessionId']) fires when any of those change. recheckLoopQueueModeFromState() derives queueMode from global state alone (loopActive && loopCurrentSessionId != null && loopCurrentSessionId === activeSessionId). No per-session loop object needed because the invariant guarantees loopCurrentSessionId can only point to an active, non-crafting session. No re-entrancy: initLoopQueueModeSync() is called once in initLoopUi(). applyLoopQueueMode() is idempotent. session_switched's updateLoopInputVisibility() (with per-session loop) and the reactive path may both run, but both converge on the same gate condition. Test non-vacuous: Calls initLoopQueueModeSync() before store.set() to install the subscription, then drives state transitions via plain store.set() (never calls updateLoopInputVisibility directly). Asserts loopQueueMode follows automatically: stays false when loopCurrentSessionId is null, arms true once it matches activeSessionId, disarms when loop ends. Confirmed the reactive path is exercised (not mocked). loop_judging scope valid: Same root cause as loop_iteration (both write loopCurrentSessionId without re-invoking gate). Reactive fix covers both plus any future writers of those fields. Avoids duplicating invariant reasoning into three call sites. All findings from previous review addressed. Code follows amos.code-craft rules (.1 minimal, .4 regression test, .5 no test modification, .8 task cited, .9 no deps, .10 no secrets). No rulebook violations. Zero nits. |
|
BOBBIE clean review Re-audit of PR #396 at new head 58ea6c8 (prior audit: SHA d3402b8, clean, comment 5318294897). Scope: base 863f359..head 58ea6c8, files lib/project-loop.js, lib/public/modules/app-loop-ui.js, lib/public/modules/app-messages.js, test/loop-resume-ended-session-lr-fd38ac.test.js. Summary: this revision folds in a persistence fix for finishLoop() (project-loop.js lines ~772-796: session.loop.active is now cleared and persisted via sm.saveSessionFile() for EVERY session belonging to the finished loopId, not just the last coder session) plus a new client-side reactive gate (app-loop-ui.js: initLoopQueueModeSync()/recheckLoopQueueModeFromState(), store.subscribe on loopActive/loopCurrentSessionId/activeSessionId) that re-derives the loopQueueMode UI latch whenever those store fields change, instead of only on handler-initiated calls. app-messages.js changes in this revision are comment-only (loop_iteration/loop_judging logic unchanged from the prior audited SHA). Boundary re-verification (per task instruction): confirmed the server-side authorization boundary is unchanged and independent of the new client reactive path. lib/project-loop.js handleLoopMessage() loop_message case (lines 959-992, untouched by this diff) still gates solely on the single server-side loopState.active flag before queuing any message; unauthorized calls receive loop_message_error No loop is currently running and return early. loop_message carries no client-supplied session-routing field; accepted messages are appended to the one global loopState.pendingUserMessages array, consumed by the servers own runNextIteration() against whatever session is actually current server-side. loopCurrentSessionId/loopActive (read by the new subscriber) are themselves set only from server-pushed WS fields (loop_iteration/loop_judging msg.sessionId, loop_started/loop_finished msg fields), never client-derived, so the new reactive latch cannot be driven by anything the client controls unilaterally. The client change alters WHEN and HOW OFTEN loopQueueMode is recomputed, not what authorizes a queued send server-side; a client that arms queue mode incorrectly can at most misroute a send into loop_message, which the server still independently rejects when no loop is running, or accepts into the one legitimate loop-wide queue when one is. No new session-context escape identified. Prior audits conclusion (client gate is UI routing only, not an authorization boundary) still holds at this SHA. No secrets, no PII, no path or command injection patterns in the new or changed code. No dependency manifest changes in this diff (bobbie.dep rules not applicable). New test file (test/loop-resume-ended-session-lr-fd38ac.test.js, 454 lines) exercises both the server persistence fix and the client gate fix together via a real saveSessionFile spy; no fixture credentials. scanners_run: gitleaks detect (base..head commit range), clean, no leaks. semgrep unavailable: registry-fetch blocked, no outbound network egress in this environment; both semgrep --config auto and semgrep --config p/javascript against the head-SHA blobs of the four changed files hung retrieving rules and were terminated after 120s with no further output; proceeded judgment-only for SAST against the diff. osv-scanner not run, no dependency manifest touched by this diff (out of scope). Findings: none. findings: [] |
|
Merged via clagentic-loadout v0.2.0
|
Problem: A loop session that has ENDED cannot be replied to (whole class of ended loop sessions, not one instance). Switching into any finished loop session shows a contradictory pair -- Queued for the next loop iteration (grey) alongside Loop: No loop is currently running (red) -- and every send is misrouted.
Root cause (per lr-fd38ac diagnosis, MILLER): two independent bugs, both fixed here.
Server (lib/project-loop.js finishLoop()): only the LAST coder session had session.loop.active cleared, and that clear was never persisted via sm.saveSessionFile(). Every judge session and every prior iteration coder session stayed active:true forever; even the one session that WAS cleared reverted to active:true across a daemon restart, because sessions.js rehydrates session.loop straight off the persisted on-disk meta line.
Client (lib/public/modules/app-loop-ui.js updateLoopInputVisibility()): the loopQueueMode routing latch was armed from the per-session loop.active marker alone, never cross-checked against whether a loop is actually running (store.loopActive) or whether the session being viewed is the loop own current session. A stale per-session marker (from bug 1, or from a client that missed a loop_finished broadcast, e.g. reconnect) re-arms queue mode, so input.js routes the send as loop_message, which the server correctly rejects since the global loop flag was cleared. Both UI strings are honest; they read different sources of truth.
Fix:
Per the task brief, the double-loop structural unification (making the per-session marker and global flag not both be independently authoritative) is intentionally NOT attempted here -- that is filed separately as lr-6fe5fc.
Tests: test/loop-resume-ended-session-lr-fd38ac.test.js (3 tests, all new).
Verified per the task brief requirement: reverted both fixes and re-ran the full suite -- all three new tests failed (expected false, actual true on the stale-marker assertions) against unpatched code, confirming they are not vacuous. Restored the fix and reran: all three pass.
npm test: 1409/1410 pass, 1 pre-existing skip, on the fix-applied runs (one run hit an unrelated pre-existing flake in test/daemon-bootstrap-guard.test.js -- a daemon-subprocess-exit-code timing test untouched by this diff -- confirmed flaky by an immediate clean rerun, not caused by this change).
TASK: lr-fd38ac