fix(sidebar): preserve in-progress session rename across list rebroadcasts (lr-16b88d) - #405
Conversation
…casts (lr-16b88d)
…commits (lr-16b88d)
|
BOBBIE security audit — PR #405 (fix/lr-16b88d-inline-rename-broadcast-commit). Scope: base 197b945..head 5d92fd6. Two files changed: lib/public/modules/sidebar-sessions.js, test/frontend-state-correlation-lr-fb49.test.js. Findings: none.
Reasoned checks: title rendering uses textContent (not innerHTML) for both the committed rename value and the resumed value/caret restore — no DOM XSS surface introduced. The daemon-side rename_session handler (lib/project-sessions.js:607-622), named in the task context re: prior LORE engram 7493955 (rename-during-daemon-restart / in-memory-history-loss-vs-empty-history race), is confirmed untouched by this diff (git diff --name-only on the exact base..head range). The outbound WS payload shape and its guard condition (newTitle && newTitle !== currentTitle && connected) are unchanged by this PR. The actual behavioral change is a reduction in spurious rename_session sends (commit now fires only on real user settle — Enter/blur/Escape — instead of on every session_list rebroadcast during an actively-streaming session), which narrows rather than widens exposure to that pre-existing daemon-side race. That race remains a legitimate follow-up but is not a citable exposure this diff introduces. scanners_run: gitleaks(clean), trufflehog(clean), semgrep(clean), osv-scanner(baseline-only, out of diff scope) BOBBIE — clean |
|
PEACHES — blocking lib/public/modules/sidebar-sessions.js:1332 — amos.code-craft.12 — suspend() captures state but does not prevent old input blur listener from firing during rebuild. When innerHTML="" runs at line 1361, the detached input can still hit the blur listener (line 826) and send rename_session (line 793) on DOM removal, committing partial/abandoned edits and setting titleManuallySet=true—violating core acceptance: no rename_session for in-progress edit. lib/public/modules/sidebar-sessions.js:1489 — amos.code-craft.12 — Missing post-rebuild target does not guarantee activeRename is cleared. If session/loop filtered out, startInlineRename()/startLoopInlineRename() return early without clearing stale activeRename, allowing later rename to commit abandoned detached edit. lib/public/modules/sidebar-sessions.js:768 — amos.code-craft.12 — Resume path does not prevent committing the suspended instance. When reopen calls startInlineRename(), it immediately does activeRename.commit() before creating new input, commits the suspended partial edit twice (once on blur, once on re-open). test/frontend-state-correlation-lr-fb49.test.js:245–319 — clagentic-console.demonstrated-test-failure — Source-text assertions cannot catch blur/lifecycle defects. Tests assert suspend() presence but not actual blur/reopen lifecycle—missing the critical flaw that suspended rename still commits. |
…ced blur cannot commit it (lr-16b88d)
…guard (lr-16b88d)
|
BOBBIE re-audit of PR #405 (task lr-16b88d), superseding the prior clean verdict on 5d92fd6 (comment 5403368621). Self-fetched head_sha via bobbie-github-security read: 7952b50 (confirmed against live PR API, matches HOLDEN report). Scope: base origin/main (197b945) resolved successfully; diffed base..head across the 3 new commits (4db6059, 43d51c1, 7952b50) added on top of the previously-audited SHA. Findings: none blocking, none nit. Re-verification of the concern flagged in the task: rename-commit rendering still uses textContent exclusively — lib/public/modules/sidebar-sessions.js:819 (textSpan.textContent = newTitle) and :901 (nameNode.textContent = newName). No innerHTML assignment of user-controlled title/name text anywhere in the diff; innerHTML is only used to clear/restore pre-existing (already-escaped) markup via originalHtml, never to inject the typed value. This holds unchanged from the prior clean audit. The suspend/resume fix (4db6059) correctly closes the PEACHES-flagged gap: suspendRename() now flips the settled short-circuit flag itself before returning the snapshot, so a rebuild-induced synthetic blur on the detached input is a no-op instead of committing a partial title as a real rename_session. commitRename/cancelRename in both startInlineRename and startLoopInlineRename now identity-check activeRename.commit before nulling the module-level slot, preventing a stale closure from clobbering a newer rename. No injection, no auth-bypass, no credential-handling change introduced. test/sidebar-sessions-rename-lifecycle-lr-16b88d.test.js (new, 539 lines): hand-rolled DOM stub used only to drive real renderSessionList() under node --test. No eval/Function-constructor DOM parsing, no real network I/O (WS is a local in-memory stub pushing to an array), no real filesystem writes, no hardcoded credentials. Test-only, not reachable from any production code path. semgrep flagged javascript.lang.security.audit.path-traversal.path-join-resolve-traversal at test/frontend-state-correlation-lr-fb49.test.js:20 (path.join(__dirname, ..., rel)) -- pre-existing helper, untouched by this diff (diff only touches lines 229+), and rel is always a hardcoded literal within the same file, never external input. Dropped per Pre-Report Gate: not in this PRs diff hunks. No package.json/package-lock.json changes in this diff -- bobbie.dep.* not applicable. scanners_run: gitleaks detect (5 commits scanned, no leaks), semgrep --config=auto (changed files; 1 hit, out-of-scope pre-existing pattern, dropped), osv-scanner (skipped -- no dependency manifest changed in diff). |
|
PEACHES — clean (0 nits) All four blocking defects from prior review verified as fixed:
Fingerprint ordering corrected: computed BEFORE suspend(), so no-op renders never settle interrupted edits. No brand violations. Codex confirms no regressions; tests pass under direct node --test invocation. |
|
Merged via clagentic-loadout v0.2.0
|
What changed
renderSessionList in sidebar-sessions.js no longer force-commits an in-progress inline rename (session or loop) on every rebuild. Instead it suspends the edit -- captures value, selectionStart and selectionEnd from the live rename input, with no WS send, no commit, no cancel -- before a real rebuild runs, then re-opens the rename on the matching session item or loop group afterward, restoring the typed value and caret position.
Why
MILLER diagnosis fnd-fcdaf1, confidence 0.92: renderSessionList called activeRename.commit unconditionally, ten lines before the fingerprint no-op check. commit is the same function bound to the rename input blur handler, so every inbound session_list broadcast ended the in-progress edit and shipped the partial text as a real rename_session. An actively-streaming session broadcasts session_list many times per turn -- activity-token transitions, per-turn result, startQuery -- so a rename committed within a keystroke or two. Escape was unreachable because a broadcast almost always won the race and set settled first.
Two consequences this closes:
Reordering alone (moving the commit below the fingerprint check) was insufficient: the fingerprint helper includes lastActivity and the derived activity-indicator class, both of which change on nearly every broadcast, so a real rebuild really does run on almost every message. This PR implements MILLER fix-shape option 2 -- preserve the edit across rebuild by re-opening the rename afterward -- not the option 3 fallback of suppressing rebuild while editing, so no fallback justification is needed.
startLoopInlineRename, an identical defect on the same activeRename slot, is fixed in the same diff per the task explicit in-scope note.
Test status
npm test run twice via scoped-test-wait (full suite, check-test-count.js wrapper -- per-file completion floor plus 1300-test floor both enforced): 1471 of 1471 passing, 0 failures, both runs identical counts. This repo has no jsdom or DOM-harness dependency (adding one is out of scope, allow_new_deps not granted), so per this project own established convention for DOM-heavy frontend modules (see frontend-state-correlation-lr-fb49.test.js own header, and popover-position-lr-a10a.test.js), coverage here is source-text regression testing against the exact new code shapes -- suspend and resume capture, non-settling suspend, setSelectionRange restore, the re-open block at the end of renderSessionList -- not a source-text-absence proxy for a claim that cannot otherwise be verified at this layer.
The existing section F tests in frontend-state-correlation-lr-fb49.test.js asserted the OLD unconditional-commit behavior as correct -- that assertion is now factually the regression this task fixes, so it was updated (not silently weakened) to assert the corrected suspend and resume contract, plus two new tests for the resume re-open path and the setSelectionRange restore. This is an update to make a test assert the right invariant, not a change to make a test pass around a real defect.
Task
lr-16b88d
PEACHES fold-in fix, same PR and branch. Fixed 3 defects: suspend now settles the rename before returning, so the detach-induced synthetic blur on rebuild cannot commit the partial edit. renderSessionList nulls activeRename immediately after suspend, before rebuild, so no stale reference survives for a vanished target or a second resume-path commit. commitRename and cancelRename identity-check activeRename.commit before nulling it. Fingerprint is computed before suspend is called so a no-op render never settles a live edit. Added a real end-to-end runtime lifecycle test with no jsdom driving renderSessionList, real contextmenu, real Rename click, real detach blur; verified as a negative control against the pre-fix code. Updated stale source-shape assertions in the fb49 test file. Full suite 1476 of 1476 passing, run twice. Task lr-16b88d