Repository navigation
feat: move a chat family between projects in chatd - #30893
ibetitsmike wants to merge 1 commit into
Conversation
chatd gains MoveChatToProject, which sets the project of a root chat. Its subagent chats follow because they use the project of the root. The chatstate machine gets a SetProject transition, allowed only from W, E0 and E1, like SetArchived. SetFamilyProject locks the root row before its subagents, in the same order as archiving, so a busy family gets ErrTransitionNotAllowed and a move never overlaps a step. A move to the project the chat is already in changes nothing, even while the chat runs. After the commit, chatd publishes a project_change watch event for each chat of the family. A root chat outside a project now has an empty memory index, so a chat that leaves its project, or whose project is deleted, gets the memories it saw removed at its next turn start.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b06cf6d4bd
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
chatd can now move a root chat, together with its subagent chats, into a project, to another project, or out of its project. Nothing calls this yet. #30894 adds the API.
Stack (moving chats between projects, bottom first):
PATCH /api/v2/chats/{chat}takesproject_idWhy: today a chat keeps the project it was created with. A move must follow the family rules of archiving, because a step that runs during a move would still have the memory tools of the old project.
chatstate.Tx.SetProjectruns only from W, E0 and E1, likeSetArchived.SetFamilyProject()locks the root row before its subagents, in the same order as archiving, so a busy family getsErrTransitionNotAllowedand nothing changes.project_id, and subagents use the project of the root. After the commit, chatd publishes a newproject_changewatch event for each chat of the family. The move keepsupdated_at, so lists keep their order.What to scrutinize: the lock order and the same-project check under the lock in
SetFamilyProject()(chatstate/family.go), and the turn-start condition inmemoryIndexMessage()(memory.go).Generated (
make gen):queries.sql.go,querier.go,dbmockanddbmetricscome fromqueries/chats.sql.coderd/apidoc,docs/reference/api/schemas.mdandtypesGenerated.tscome from the new watch event kind, which is also why twochats.test.tsfixtures gainproject_change: false.coderd/x/chatd/ARCHITECTURE.mdhas no edit. Lines 60 (the project of a root chat can now change), 126 (the transition list needsSetProject), 470 (the move follows the family rules of archiving) and 1009 (a chat that leaves its project gets its memory index removed) need the author's update.Testing
make pre-commitpasses.go testpasses forcoderd/x/chatd/chatstate,coderd/database/dbauthz(TestMethodTestSuite) and the chatd memory tests.TestSetFamilyProjectLocksRootFirstfails with 55P03 when a subagent row is locked first.TestSetFamilyProjectKeepsTheCurrentProjectcovers the same-project move during a run. The transition matrix test gainsSetProjectcases for W, E0 and E1.TestMovedChatUsesNewProjectruns two real turns around a move: the second turn uses the instructions and memories of the new project, andOutOfProjectfails without the memory index removal.