Repository navigation
(agents): follow-ups from the #374 review - #487
Conversation
…only in case findSession, the descriptor-id change check and the rescan throttle compared session ids strictly, so a descriptor naming the session in a different case updated the status cache but never triggered the busy-to-idle rescan. They now use the same lowercased key as statusKey. Also, from the devsuitup#374 review: tests for a live job of another session, for uppercase descriptor insertion and removal and for the bg-agent-live-job route; the folder-archive doc now matches the code when "Archive the sessions" is unchecked; runClaudeCommand's rationale comments are left to the context doc. Claude-Session: https://claude.ai/code/session_01TLG3EcjMdqPmYky3m2QrYA
|
@devsuitup this follow-up to #374 is ready for review. I cannot add you as a reviewer (read-only access), so I am mentioning you here. The CI runs need your approval, since this PR comes from a fork. |
1d78ec5 to
103c280
Compare
devsuitup
left a comment
There was a problem hiding this comment.
Review at 103c280. Thanks for the follow-ups. findSession, the descriptor-changed check and the rescan throttle now share statusKey, the rescan is reported under the app's own id, runClaudeCommand is identical after the comment removal (no code change hidden in it), both archive docs match the unchecked "Archive the sessions" path, the changelog entry ends (#487), and the new tests cover a live job of another session (job file and cache), an uppercase descriptor, a mixed-case rescan and the bg-agent-live-job route. 233 tests in 28 related files pass locally on Node 24. The branch is level with main.
Non-blocking
findSession(cli-session-state.js~205) returns the first active entry whose key matches in any case, even when another entry matches exactly. Two live sessions whose ids differ only by case would be confused. UUIDs do not do that, so I do not block on it; preferring the exact match first, then the case-folded one, would close it.- The tests never change the descriptor or app-id case between two observations and never attempt a second rapid rescan, so reverting the descriptor comparison or the throttle normalisation would survive them (by reasoning, not executed).
Not run: Linux Node 20/22 with c8, lint, fresh CI.
Follow-ups the maintainer listed as non-blocking when approving #374.
cli-session-state.js.findSession, the check for a descriptor whose session id changed, and the rescan throttle compared ids strictly. A descriptor naming the session in a different case therefore updated the status cache but never triggered the busy-to-idle rescan. They now use the same lowercased key asstatusKey, and the rescan is reported under the app's own id.bg-agent-live-jobroute is now covered intest/bg-agents-ipc.test.js.stopBeforeArchive: it only hides the folder, archives and detaches nothing, and the job stays in the Agents view.docs/background-agents.mdand.ai/contexts/bg-agents.mdnow say so. I aligned the doc rather than the code because hiding a folder is reversible and touches no session.runClaudeCommand(main.js) are removed. "Running the CLI" in.ai/contexts/bg-agents.mdalready covers them, and the function keeps a one-line pointer to it.task checkpasses locally on Linux (4562 pass, 0 fail). Not run on Windows.https://claude.ai/code/session_01TLG3EcjMdqPmYky3m2QrYA