Repository navigation
Conversation
Docs previewCheck off each page once it's been reviewed. If a page changes in a later push, its checkbox clears automatically so it gets a fresh look. Pages not yet wired into the docs navigation aren't listed here. |
|
/coder-agents-review |
|
Chat: Review posted | View chat Review historydeep-review v0.13.0 | Round 2 | Last posted: Round 2, 63 findings (1 P1, 12 P2, 23 P3, 24 P4, 3 Nit), COMMENT. Review Finding inventoryFinding inventory: PR #30683Findings
Law analysisR1. Effective LOC +1860 -89 (42 files). Head 6f91b9b. Verdict: Don't split. Enforcement: Advisory. Ordering constraint noted: this PR appends to Contested and acknowledgedCRF-17 (P3, chats.sql:643) - Per-read-site deleted-project predicate
CRF-46 (P3, chatprojects.sql:151) - Second execution-state writer
Round logRound 1Full panel (round 1). Netero + Law (effective additions 1860 > 1000), then 17 panel reviewers: ging-go, ging-ts, ging-react, killua, knuckle, kurapika, luffy, melody, nami, ryosuke, takumi (triggered specialists), pariston, mafuuu (core), bisky (tests changed), gon, leorio (once-per-PR floor, round 1 default), meruem (wildcard, random draw). Size target 9-12 exceeded because 11 triggered specialists plus core already total 13. Ging variants: no findings. Cross-check: convergent findings merged (CRF-4, 5, 7, 9, 13, 15, 17, 19, 20, 21, 35, 46) at the higher severity. CRF-3: kept P1; keep-argument (stack merges bottom-up, so this PR would be retargeted onto a main that already applied 000618, and every deleted-filter query fails) outweighs downgrade argument (merging into the parent branch as currently targeted is correct). Gon's 11 comment-bloat findings posted at the persona's fixed P2. Bisky Nit on family_test.go:271 moved to out of scope (pre-existing copy); its querier_test copy is covered by CRF-44. Five affirmation Notes dropped. Reviewed against e0fbe12..6f91b9b. Round 2Restructure round (migration renamed 000618 to 000622; coderd/audit.go and audit_internal_test.go added). Churn guard PROCEED: 39 addressed, 2 contested. Netero advisory (CRF-60 to 63); Law skipped (effective additions +114 since R1). Full panel of 18: same 17 personas as R1 (Gon and Leorio mandatory on restructure) plus Kite (wildcard, random draw from distilled set). Meruem, Ryosuke, Pariston resumed via message_agent for the dispute check-in. New: 1 P2, 6 P3, 14 P4, 1 Nit (CRF-60 to CRF-81). Re-raised: CRF-21 (Gon), CRF-46 (Meruem). Reviewed against 1e98984..3121c76.
About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
This PR changes project delete to a tombstone that hides the project's chats at once, with a background cleanup and sweep that archive them; 1 P1, 11 P2, 17 P3, 10 P4, and 2 Nit findings.
Comment quality is a pattern in this PR: 11 new comments restate code or repeat a fact another comment holds, most often the cleanup sequence (chatprojects.sql:101, family.go:170, errors.go:53, ARCHITECTURE.md). The PR description's claim that running chats finish their current turn is also wrong; see the ARCHITECTURE.md:1023 comment.
Out of scope (needs a ticket or explicit acceptance by a human):
coderd/database/dump.sql:5374: a hard delete of the project owner'susersrow cascades the project and setschats.project_idNULL, so a deleted project's chats become visible again as chats outside any project.coderd/x/chatd/chatd.go:3243: watch events go only to the chat owner, and there is no project-level event, so sharees with no chats in a deleted project keep its sidebar folder and composer until reload, and a send from it gets 404.site/src/api/queries/chats.ts:554:applyWatchedChatArchivedhas the same per-event refetch-and-abort pattern for family archive bursts.coderd/database/dump.sql:5404:chats_parent_chat_id_fkeyandchats_root_chat_id_fkeyareON DELETE SET NULL, so when retention deletes a root first, its direct child is treated as a root byLockChatProjectRootChats,GetChats, and the auto-archive index.coderd/audit.go:406:auditLogIsResourceDeletedhas noResourceTypeChatProjectcase, so deleted chat project audit entries never show "(deleted)".codersdk/chats.go:2175:ChatWatchEventKindDeletedis published for archive, not deletion, which misleads watch clients.coderd/x/chatd/chatstate/family_test.go:271: an older inlinepg_stat_activitypoll duplicates the newwaitForLockWaithelper.
Notes:
coderd/httpmw/chatparam.go:52: the deleted-project check usesAsSystemRestricted;AsChatdalso holdschat_project:read, butAsSystemRestrictedmatches the rest ofhttpmw.coderd/x/chatd/projects.go:47:scheduleArchiveDebugCleanupstarts one concurrentDeleteChatDebugDataByChatIDper archived chat, so a large project cleanup queues one delete per chat on the connection pool.000618_chat_project_sharing.down.sql:4: rolling back permanently deletes every chat of a deleted project, including chats retention 0 would keep, with no operator warning.coderd/x/chatd/projects.go:39: a delete authorized only bychat_project:deletearchives and hides sharees' chats, which they cannot recover or export; the decision log records this as intended.000618_chat_project_sharing.up.sql:21: with retention 0, a deleted project's chats and memories stay in the database permanently and no API path can read or delete them.coderd/chat_projects.go:282: root chats are audited asdeleteat request time while their rows stay until retention, and forever with retention 0; sub-chats are archived the same way but not audited.
coderd/database/queries/chats.sql:2436
P3 [CRF-1] GetChatsByWorkspaceIDs does not skip chats of a deleted project, so GET /chats/by-workspace keeps returning them until the cleanup archives them. (Netero)
This query filters only archived = false, and a tombstoned chat stays unarchived until ArchiveDeletedChatProject runs, or indefinitely with no chat daemon. chatsByWorkspace (coderd/exp_chats.go:385) can then return the hidden chat's ID instead of a visible chat on the same workspace, and that link 404s at ExtractChatParam. DeleteHidesChats checks GetChatsByWorkspace only after waiting for the cleanup, so the window is untested. Add the same NOT EXISTS (... chat_projects.deleted) clause and assert an empty result in DeleteWithoutAIGateway.
🤖
coderd/audit.go:493
P3 [CRF-18] Audit entries for chats in a deleted project, including the delete entries this PR writes, report is_deleted: false. (Pariston)
This case assumes chats are hard-deleted and calls GetChatByID, which returns the archived row of a deleted project's chat. The audit page then shows "deleted chat X" with no "(deleted)" marker and a /agents/{id} link that ExtractChatParam answers with 404, for as long as the row lives, which is forever with retention 0. Treat a chat whose project is deleted as deleted here, and update the comment.
🤖
coderd/x/chatd/chatstate/family.go:23
P4 [CRF-39] The RootID doc says ErrChatNotFound means an unknown chat, but this PR also returns it when unarchiving a root whose project is deleted. (Leorio)
A caller reading the input contract will map the error to "the chat was purged". Add: "Unarchiving a root whose project is deleted also returns [ErrChatNotFound]."
🤖
🤖 This review was automatically generated with Coder Agents.
Moves the deletion schema into migration 000622 and addresses review: the tombstone deletes memories and instructions and 404s on a repeat, per-chat delete audits are dropped, the cleanup skips archived roots, reloads only the chats it archived, and publishes project_deleted for every project chat, and GetChatsByWorkspaceIDs and audit logs treat deleted-project chats as gone.
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2 of the chat project delete review: 39 of 41 round 1 findings are verified fixed, the per-read-site filter dispute is closed, and this round adds 1 P2, 6 P3, 14 P4, and 1 Nit, plus two re-raised threads.
Re-raised: the comment on SetFamilyArchived's project check still names the delete, and the execution-state SET list copy still has no back-reference at UpdateChatExecutionState. Two of this round's findings come from round 1 fixes: skipping archived roots opened the unarchive race, and dropping cancelChatListRefetches lets a pre-tombstone refetch re-add a deleted chat.
Out of scope (needs a ticket or explicit acceptance by a human):
coderd/x/chatd/chatd.go:3256: watch events go only to the chat owner, so users who see a project chat through the chat's own ACL get noproject_deletedevent and keep it in their sidebar until a refetch.
🤖 This review was automatically generated with Coder Agents.
| spyOn(API.experimental, "getChats") | ||
| .mockResolvedValueOnce([watchedChat()]) | ||
| .mockResolvedValue([]); | ||
| spyOn(API.experimental, "getChat").mockRejectedValue( |
There was a problem hiding this comment.
P2 [CRF-60] ProjectDeletedWatchEventRemovesOpenChat fails on every run, so the project_deleted handler has no working story coverage. (Netero)
The getChat mock rejects with mockApiError(...), which has no HTTP status, and AgentChatPage.tsx:721 renders AgentChatPageNotFoundView only when the status is 404, so the page shows the generic error view and findByText("Chat not found") times out. Reproduced 3 of 3 with pnpm exec vitest run --project=storybook src/pages/AgentsPage/AgentsPageLayout.stories.tsx -t 'Project Deleted Watch Event'; CI's test-js runs only --project=unit, so it does not catch this. Reject with { ...mockApiError({ message: "Resource not found." }), status: 404 }; with that change the story passes, and it fails when the project_deleted branch in AgentsPageLayout.tsx is disabled.
🤖
| } | ||
| } | ||
| return false | ||
| case database.ResourceTypeChatProject: |
There was a problem hiding this comment.
P4 [CRF-61] The new ResourceTypeChatProject case has no test. (Netero)
TestAuditLogIsResourceDeleted covers only ResourceTypeChat, so if this case regresses, a deleted project's delete audit entry reports is_deleted: false and nothing fails. Add a row that mocks GetChatProjectByID returning sql.ErrNoRows and expects true.
🤖
| case <-ctx.Done(): | ||
| return | ||
| } | ||
| w.sweepDeletedChatProjects(ctx) |
There was a problem hiding this comment.
P4 [CRF-62] No test reaches the sweep through deletedChatProjectsLoop's ticker. (Netero)
Coverage shows this call never runs: TestSweepDeletedChatProjects calls the method directly and testOptions sets the interval to an hour. This loop is the only retry for failed cleanups and for deletes on a replica that stopped, so miswiring it would leave deleted projects' chats unarchived with no failing test. Advance the mock clock by the sweep interval and assert the chat is archived.
🤖
| delete(pending, event.Chat.ID) | ||
| } | ||
| case <-ctx.Done(): | ||
| t.Fatalf("missing removed events for %v", pending) |
There was a problem hiding this comment.
P4 [CRF-63] The failure message "missing removed events for %v" still uses the event name from before the rename. (Netero)
The loop waits for ChatWatchEventKindProjectDeleted; change the message to "missing project_deleted events for %v".
🤖
| WHERE id = @id::uuid AND NOT deleted | ||
| RETURNING id | ||
| ), | ||
| memories AS ( |
There was a problem hiding this comment.
P3 [CRF-64] A memory insert or instructions upsert that is in flight when the tombstone runs still commits, and the row stays on the deleted project with no API path to remove it. (Knuckle, Kurapika, Takumi, Kite, Melody, Mafuuu)
The writers (writeProjectMemories, putChatProjectInstructions) check the project with a plain GetChatProjectByID, their foreign key check takes FOR KEY SHARE, which does not conflict with the tombstone's UPDATE, and these CTEs delete only rows visible in the statement's snapshot. Reproduced independently by several reviewers on Postgres 13 and 16 and through chattool.InsertProjectMemory: the tombstone returned without waiting and the deleted project kept 1 memory, which dbpurge never removes while retention is 0. Locking the project FOR SHARE in the writers alone is not enough: the tombstone then waited but this CTE kept its pre-wait snapshot and still left the memory. Have both writers read the project WHERE id = @project_id AND NOT deleted FOR SHARE in their transaction (also add NOT deleted to UpsertChatProjectInstructions' SELECT), and run the tombstone as UPDATE ... RETURNING id followed by the two DELETEs as separate statements in one InTx; that sequence left 0 rows.
🤖
| AND root.project_id IS NOT NULL | ||
| AND chats.project_id IS NULL; | ||
|
|
||
| COMMENT ON COLUMN chats.project_id IS 'Sub-chats carry their root chat''s project; an insert trigger sets it and replaces any passed value.'; |
There was a problem hiding this comment.
P4 [CRF-77] The new chats.project_id comment documents only sub-chats, so the column no longer says what it means for a root chat. (Gon)
sqlc copies it to ChatTable.ProjectID. Suggested: 'Optional project the chat belongs to. Sub-chats carry their root chat''s project; an insert trigger sets it and replaces any passed value.'
🤖
| name={project.name} | ||
| confirmLoading={deleteMutation.isPending} | ||
| info="Its memories will be deleted. Chats in it, including chats started by people it is shared with, will be kept and move back to each owner's Chats list." | ||
| info="Every chat in this project, including chats started by people it is shared with, is hidden right away, and running chats are stopped. If conversation retention is enabled, the chats are permanently deleted when the retention period passes." |
There was a problem hiding this comment.
P3 [CRF-78] The delete dialog no longer says the project's memories and instructions are deleted, and they are now deleted the moment the user confirms. (Leorio, Gon)
The base text began "Its memories will be deleted."; the new text covers only chats, so an owner can conclude the project's contents survive for the retention period when its memories and instructions are gone with no restore path. Start the text with "Its memories and instructions are deleted now." The PR description's UI bullet should say the same.
🤖
| } | ||
| confirmLoading={deleteProjectMutation.isPending} | ||
| info="Its memories will be deleted. Chats in it, including chats started by people it is shared with, will be kept and move back to each owner's Chats list." | ||
| info="Every chat in this project, including chats started by people it is shared with, is hidden right away, and running chats are stopped. If conversation retention is enabled, the chats are permanently deleted when the retention period passes." |
There was a problem hiding this comment.
P3 [CRF-79] Same omission as the project page dialog: this text no longer says memories and instructions are deleted at once. (Gon, Leorio)
Apply the same sentence so both dialogs stay identical.
🤖
| // Chats are hard-deleted, so a 404 means deleted. | ||
| _, err := api.Database.GetChatByID(ctx, alog.AuditLog.ResourceID) | ||
| // Chats are hard-deleted, so a 404 means deleted. A deleted | ||
| // project's chats keep their rows until retention but are gone. |
There was a problem hiding this comment.
P4 [CRF-80] "A deleted project's chats keep their rows until retention but are gone" says the rows exist and are gone in one sentence. (Leorio)
Suggested: "A chat in a deleted project keeps its row until chat retention removes it, but counts as deleted because no API can read it."
🤖
| return false | ||
| } | ||
| if chat.ProjectID.Valid { | ||
| //nolint:gocritic // Only whether the project is deleted is used. |
There was a problem hiding this comment.
Nit [CRF-81] The //nolint:gocritic justification says what the result is used for, not why the system context is needed. (Kite)
Use the reason ExtractChatParam gives at httpmw/chatparam.go:51 (the viewer may not be able to read the project) here and at line 515.
🤖
Database and authorization layer for chat project sharing (experiment `chat-projects`), part 1 of 2. #30439 adds the API on top of it. Nothing here changes behavior: projects start with empty ACLs, no endpoint writes them yet, and deleting a project still detaches its chats. Project deletion is in #30683. - **Schema:** migration `000621` adds `user_acl` and `group_acl` jsonb columns to `chat_projects`, typed as `ChatACL` like chats, with GIN indexes. It also adds the `chat_project:share` API key scope. - **Queries:** - `GetChatProjectsOwnedOrSharedWithUserID`: owned projects plus projects with an ACL entry for the user directly, through a group, or through the Everyone group (whose ID is the organization ID). Entries count only for members of the project's organization. Keys match regardless of actions, so dbauthz filters the rows. - `IsChatProjectAccessibleByUserID`: whether a user owns a project or holds a `read` or `*` grant on it, for code running under a system subject. With sharing disabled, dbauthz answers owner-only. - `GetChatProjectByIDForUpdate`: lets the ACL update read, modify, and write in one transaction. - `UpdateChatProjectACLByID`. - **RBAC:** - New `share` action on `chat_project`. dbauthz requires it to update the ACL. - `ChatProject.RBACObject` attaches the project ACL. Memory objects get a derived ACL: project `read` grants memory read, and `read` with `update` grants memory create and delete. Wildcard grants count. Read sharees change memories through their chats' agents, which #30439 gates. Project instructions authorize through the same object, so a `read` grant reads them and an `update` grant edits them. - `--disable-chat-sharing` also covers projects: ACLs are ignored, the ACL update is rejected, and roles deny `chat_project:share`. The help text and chat sharing docs say so. - **Audit:** `user_acl` and `group_acl` changes are tracked. - **Tests:** - Querier tests run the access queries both raw and through dbauthz (owner, direct user, group, Everyone, wildcard grants, a member of another org, grants without read). - RBAC object and memory ACL tests, with an authorizer check, in both sharing modes. - dbauthz cases including the disabled guards, and the `chat_project:share` negation in the roles test. The generated TypeScript changes only add the new scope value. > Generated by Coder Agents.
Stacked on #30439. Deleting a chat project now removes its chats, including chats started by people it is shared with. Before this, deleting a project detached its chats, and #30479 and #30439 keep that behavior.
DELETE /chats/projects/{project}setschat_projects.deleted(a one-way tombstone), deletes the project's memories and instructions in the same statement, and returns 204; a repeat returns 404. It takes no chat locks, so it can't fail or ask the caller to retry. Only the project delete is audited.ExtractChatParam, and file download skip a deleted project and its chats. Root chat creation holds the projectFOR SHARE, so a creation racing the delete is either cleaned up with the rest or gets 404. Unarchiving is refused, and audit entries show the chats as deleted.Server.ScheduleDeletedChatProjectCleanuprunschatstate.ArchiveDeletedChatProject. It locks the project's unarchived root chats, archives every unarchived chat in the project with no worker or runner, and clears their queues. It then publisheschat:update(runners stop mid-turn), schedules debug-data cleanup, and publishes a newproject_deletedwatch event for every chat in the project, archived ones included (sidebars evict the chat). Running chats keep generating until the cleanup runs.GetDeletedChatProjectIDsWithUnarchivedChats). The sweep covers cleanups that failed, for example on a deadlock with a chat purge, or never ran because the replica stopped. A second pass leaves chats that are already archived alone, so their retention clock doesn't restart.project_id. Migration 000622 backfills it and adds an insert trigger that sets it, so project queries match on that column alone. A sub-chat stays in its project even if retention deletes its root first.Decision log
ChatMachinetransition, to keep the root locks short; its SET list mirrorsUpdateChatExecutionState.chats_expandedcolumn, to avoid a project join on every chat read.