feat(chat): persist always-approve tool selections across sessions#561
feat(chat): persist always-approve tool selections across sessions#561anfibiacreativa wants to merge 8 commits into
Conversation
- add loadAutoApprovedTools / saveAutoApprovedTools to persistence.js; each does a read-modify-write so messages and sessionId are never overwritten - load persisted approvals alongside messages in loadInitialMessages so the Set is restored from IndexedDB on every page load - fire-and-forget saveAutoApprovedTools after each always-approve action - remove the _autoApprovedTools reset from clear() so approvals survive conversation resets per spec Co-Authored-By: Claude <[email protected]>
|
Implementation wise I think it's good. I'm wondering if this is what we want - this makes it so there's no way to ever revoke auto approval if I understand correctly? I could see a usecase where in a session you want auto approve, but in a next session you want to review before approving. Maybe we should add a UI for resetting? |
yep, your understanding is right. right now clear() doesn't reset the auto-approved set and there's no revoke path. that's intentional for this PR since persistence was the whole point, but you're right that there's no escape hatch yet. i captured two follow-ups in the description: a "manage always-approved tools" UI to revoke individual approvals, and the per-session-vs-persisted case you're raising. do you think the revoke UI should block this merge, or land as a fast follow? i'd lean fast follow, since without it we're not regressing anything (today approvals don't survive at all, so you re-approve constantly), we're just not yet giving a way to undo the new persistence. |
…e-persist # Conflicts: # WORKLOG.md # nx2/blocks/chat/chat-controller.js # nx2/blocks/chat/utils/persistence.js
Summary
loadAutoApprovedTools/saveAutoApprovedToolsAPI inpersistence.jsperforms read-modify-write to avoid clobbering messages or sessionId.loadInitialMessagesloads persisted approvals in parallel with messages on startup.clear()no longer resets_autoApprovedTools— approvals intentionally survive conversation resets per spec.Problem / Root Cause
Previously
_autoApprovedToolswas an in-memorySetinitialised to empty on every load (and cleared on everyclear()call). Users who checked "always approve" for a tool had to re-approve on every page reload or after clicking "New conversation", which was friction-heavy for power users.What Changed
nx2/blocks/chat/utils/persistence.jsblankRecord(room)factory to eliminate duplicated blank-record skeletons.loadAutoApprovedTools(room)— readonly IndexedDB transaction; returns aSet<string>; resolves to an empty Set on any error or missing record.saveAutoApprovedTools(room, toolNamesSet)— readwrite transaction with read-modify-write: reads the existing record, spreads in the new tool names array, writes back; wraps the entire async flow in a realPromise(resolves ontx.oncomplete, rejects ontx.onerror/req.onerror/ thrown errors).nx2/blocks/chat/chat-controller.jsloadInitialMessagesnow runsloadMessagesandloadAutoApprovedToolsin parallel viaPromise.alland assigns the result to_autoApprovedTools.approveToolCall(always path) firessaveAutoApprovedToolsas a fire-and-forget with.catch(() => {})to avoid unhandled rejections.??= new Set()guard fromapproveToolCall(the Set is always initialised before any tool call is possible)._autoApprovedTools = new Set()reset fromclear(); added a comment explaining the intentional omission.test/nx2/blocks/chat/utils/persistence.test.jsloadAutoApprovedToolsandsaveAutoApprovedTools:WORKLOG.md— updated with implementation notes (2 entries).Diff stat
How to Test
da-chat→conversations; verify the record has anautoApprovedToolsarray containing the tool name.npm test— all 967 tests should pass.Risk / Follow-ups
saveAutoApprovedToolsis fire-and-forget fromapproveToolCall; a quota-exceeded failure logs aconsole.warnbut does not surface to the user. Acceptable for this feature; a toast notification could be added later.1; if a future schema change needs to add theautoApprovedToolsfield to existing records a migration path will be needed (currently handled gracefully by theArray.isArrayguard inloadAutoApprovedTools).