Fix session ownership follow-up review issues / 修复会话归属后续评审问题#4273
Conversation
Clean stale memory index entries, remove old scoped copies when a memory changes scope, keep session destruction waiting on already-killed jobs, and route memory suggestions through the selected desktop tab. Co-authored-by: lifu963 <56394323+lifu963@users.noreply.github.com> Co-authored-by: ashishexee <144021866+ashishexee@users.noreply.github.com> Co-authored-by: AresNing <49557311+AresNing@users.noreply.github.com>
|
荣幸之至😄 |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a381032aac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const path = selectedTabId | ||
| ? await app.AcceptMemorySuggestionForTab(selectedTabId, candidate) | ||
| : await app.AcceptMemorySuggestion(candidate); |
There was a problem hiding this comment.
Reset suggestions when switching memory tabs
When a user scans suggestions in one tab and then changes the Memory page selector to another tab, the existing suggestions state is left visible and autoSuggestions will not rescan because it is non-null. Accepting one of those stale candidates now calls AcceptMemorySuggestionForTab(selectedTabId, candidate), so history-derived memories from the previous tab can be written into the newly selected tab's memory store; the same stale-state issue applies to skill suggestions just below. Clear/key the suggestions by tab or force a refresh on selectedTabId changes before accepting.
Useful? React with 👍 / 👎.
| tab := a.tabByIDLocked(tabID) | ||
| workspaceRoot := "" | ||
| if tab != nil { | ||
| workspaceRoot = tab.WorkspaceRoot |
There was a problem hiding this comment.
Avoid global skill creation for stale tab IDs
If the Memory panel keeps a selected tab id after that tab is closed (or any caller passes an invalid id), tabByIDLocked returns nil here but the method continues with an empty workspaceRoot; a project-scoped suggestion is then downgraded by !st.HasProjectScope() and written into the user's global skill store. That differs from the tab-scoped memory accept path, which no-ops when the target tab is missing, and can persist a workspace-specific workflow globally. Return without creating anything when a non-empty tabID does not resolve to a tab.
Useful? React with 👍 / 👎.
| if p != "" || indexContainsIn(dir, name) { | ||
| if err := flushIndexIn(dir, indexLinesExceptIn(dir, name)); err != nil { |
There was a problem hiding this comment.
Preserve hand-written MEMORY.md content when pruning stale links
When Archive/Delete is called for a name whose <name>.md file is already gone but whose index line remains, this new indexContainsIn branch rewrites MEMORY.md via flushIndexIn(indexLinesExceptIn(...)). indexLinesExceptIn only carries forward managed link lines, so any hand-written headings, notes, or imports in that MEMORY.md are silently dropped even though the requested cleanup was just a stale link; remove only the matching line or preserve non-managed lines when handling the missing-file case.
Useful? React with 👍 / 👎.
Summary
Follow-up to #4250 for the bot review threads that were left as post-merge cleanup:
MEMORY.mdentries even when the active memory file is already absent.DestroySessionwaiting on jobs that were already markedKilledbut have not finished unwinding.complete_stepreview point is already resolved on currentmain-v2by refactor(control): drop phrase-matched approved-plan continuation #4266, so this PR leaves that controller path unchanged.Credits / Attribution
This follow-up is part of the #4250 integration and keeps the original contributor attribution:
SubagentStore, background job ownership, session destroy cleanup, Desktop restore guarding, and late writeback prevention. This PR fixes the follow-upDestroySessionedge for already-killed jobs.GlobalDirrouting and tab-aware memory panel behavior. This PR fixes stale memory scope/index cleanup and tab-scoped suggestion APIs on top of that work.main-v2by refactor(control): drop phrase-matched approved-plan continuation #4266.The commit includes
Co-authored-bytrailers for @lifu963, @ashishexee, and @AresNing so GitHub can credit them as co-authors.Verification
PATH="$(go env GOPATH)/bin:$PATH" wails generate modulewith Wails 2.12.0go test ./internal/memorygo test ./internal/jobsgo test ./internal/control -run 'TestApprovedPlan'go test . -run 'TestMemorySuggestions'indesktop/npm run buildindesktop/frontend