GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||
GeezerVison is showing github.com — feat: load organization skills in chats by ibetitsmike · Pull Request #30613 · coder/coder Conversation
|
|
@codex review |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@codex review |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@codex review |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
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". |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@codex review |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Codex Review: Didn't find any major issues. Breezy! 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". |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
…icker and read-only Enabled status
|
@codex review |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Here are some automated review suggestions for this pull request.
Reviewed commit: f97b1710f7
Your 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".
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
## Stack Part of [CODAGT-1341](https://linear.app/codercom/issue/CODAGT-1341/support-organization-level-skills-in-coder-agents). This stack adds organization skills to Coder Agents: admins publish a skill once per organization, share it with groups or members, and chats in that organization load it next to personal and workspace skills. It replaces the 21-PR draft stack #30584 to #30604 with five PRs, bottom to top: 1. **#30609** data model: one migration, queries, RBAC and dbauthz (this PR) 2. #30610 organization skill API routes and the scope-neutral SDK 3. #30611 sharing ACL routes 4. #30612 settings UI: shared skills table, admin page, member section 5. #30613 chats: chatd loading, `/` menu, docs ## What changes This PR adds the data model and authorization for organization skills. Personal skills keep working unchanged: `git diff origin/main -- codersdk site` shows only generated RBAC and API key scope additions (`apikey_scopes_gen.go`, `rbacresources_gen.go`, `rbacresourcesGenerated.ts`, the RBAC and scope unions in `typesGenerated.ts`) and `codersdk.ResourceTypeOrganizationSkill` with its friendly string. No personal route, SDK type or site component changes, and `coderd/userskills_test.go` changes only by mechanical constant renames. **One migration, `000621_skills`** (please review it closely): - renames `user_skills` to `skills` with its constraints, index and triggers, and recreates the cap function and `delete_deleted_user_resources`, whose PL/pgSQL bodies name the table; - adds `organization_id` and `project_id` (`ON DELETE CASCADE`), `enabled` (default true), and `group_acl` / `user_acl` (`jsonb`, default `{}`), with CHECKs for exactly one owner, ACLs that are JSON objects, and ACLs that stay empty unless an organization owns the row, plus partial unique name indexes per owner kind; - replaces the cap trigger with `enforce_skills_per_owner_limit()`: 100 rows per owner, disabled rows included, locking the owner row `FOR NO KEY UPDATE` so unrelated foreign key checks are not blocked; - adds a `BEFORE DELETE` trigger on `organization_members` that removes a departing member from `user_acl` on that organization's skills, so rejoining doesn't restore a direct grant; - adds the `organization_skill` `resource_type` and `api_key_scope` enum values last; nothing in the migration uses them. The down migration undoes the three old migrations in reverse order: it drops organization and project rows and restores `user_skills`, its names, the cap function and `user_id NOT NULL`. Authorization: a new RBAC resource `organization_skill` (create, read, update, delete, share). Owner and org admin get it; site and org auditors get read; members read only through the skill's ACL, which applies only to members of its organization. `Skill.RBACObject()` returns the organization object with both ACLs; project rows stay denied to everyone. New sqlc queries cover organization insert, get, list (SQL-filtered through `GetAuthorizedOrganizationSkillMetadata` and a `regosql` converter), update and delete, with dbauthz wrappers and the audit table entries. No routes yet. ## Commits The first commit reproduces the reviewed code from the old stack; each later commit is a labeled trim or fix. - `baa3150e3b7` feat: add the organization skills data model. Reproduction: every non-site file this commit changes equals the reviewed old `authz` head `1e16b3004c4`, except that the old stack's migrations 000615, 000616 and 000617 are merged into one migration (then `000615_skills`, now `000621_skills`) with one fixture (`testdata/fixtures/000621_skills.up.sql`) and one migration test (now `testMigration000621Skills`). At that commit, `dump.sql` was byte-identical to the old head's. - `1b84e83463a` test(coderd/database): keep only the SQL paths of the org skill matrix. `TestOrganizationSkillAuthorization` becomes `TestOrganizationSkillQueries` instead of being deleted, because coverage showed it is this PR's only run of the `GetAuthorizedOrganizationSkillMetadata` row scan (80% to 65% without it) and of the organization update and delete queries (100% to 0%). Swapping `group_acl` and `user_acl` in the converter makes `GrantedMember` fail. The removed cases are decided by `TestRolePermissions`, `TestSkillRBACObjectOrganizationACL` and the dbauthz method suite. - `e2931b412e9` test(coderd/database): fold the skill cap cases into existing coverage. The user case is covered by `TestUserSkillLimit` and `TestUserSkillLimitConcurrentCreates`; the organization case folds into `ConcurrentOrganizationInsertsHonorCap`, which now also checks that disabled rows count and another organization is unaffected (mutation checked); the project case stays. - `be62aea9021` test(coderd/rbac): drop `TestOrganizationSkillReadScope`, covered by `TestExternalScopeNames` and the generated scope file. - `0c275ba37de` merges main at `454a2fcc697`, which took 000615 to 000618: it renumbers the migration to 000619 and resolves `dump.sql`, `check_constraint.go` and the generated API docs. `8f2b4b87ee2` merges main at `ad85646164e`, which took 000619: it renumbers the migration to 000620. - `5248d56b64f` merges main at `e7b76872098`, which took 000620 (`000620_audit_chat_project_instructions_resource_type`): it renumbers the migration, fixture and migration test to `000621_skills` (step `Migration000621Skills`, version 621), and resolves `enterprise/audit/table.go` (keeping main's `AuditableChatProjectInstructions` entries and the `Skill` entry), `dump.sql` and `models.go` (the `resource_type` enum gets `chat_project_instructions`, then `organization_skill`) and the regenerated `schemas.md`. The up and down SQL are unchanged by every renumbering. ## Size Production +549/-67, tests +728/-33, generated +881/-314; total +2158/-414. This is above the 500-line reviewer-friendly limit on purpose: Mike asked for 4 to 5 PRs instead of the 21 small ones this stack replaces. ## Replaces #30584 (rename `user_skills` to `skills`), #30585 (skill owners), #30586 (organization skill authorization). ## Validation - Migration test: up keeps the existing personal row and its ID; down restores the `user_skills` names, cap function and `NOT NULL`, and drops organization rows; soft-deleting a user removes their skills after up, after down, and after re-up. - `go test` passed on `be62aea9021`, before the main merges, for `coderd` (skill tests), `coderd/database`, `dbauthz`, `migrations`, `gentest`, `coderd/rbac/...`, `coderd/x/chatd`, `coderd/x/skills`, `enterprise/audit` and `enterprise/coderd`; `make lint/check-scopes` passed; the four personal skills Vitest files pass unchanged. - On `5248d56b64f`: build, vet, `TestMigrate`, `TestMigrationChain`, the `migrations`, `database`, `dbauthz`, `gentest`, `rbac` and `enterprise/audit` packages pass; `make gen` run twice is clean, and `make pre-commit` passes. - Remote UAT round 7 through Coder Agents passed on the whole stack; see #30613's Validation for scope and findings. > Xum opened this PR on behalf of @ibetitsmike.
## Stack Part of [CODAGT-1341](https://linear.app/codercom/issue/CODAGT-1341/support-organization-level-skills-in-coder-agents). This stack adds organization skills to Coder Agents: admins publish a skill once per organization, share it with groups or members, and chats in that organization load it next to personal and workspace skills. It replaces the 21-PR draft stack #30584 to #30604 with five PRs, bottom to top: 1. #30609 data model: one migration, queries, RBAC and dbauthz 2. **#30610** organization skill API routes and the scope-neutral SDK (this PR) 3. #30611 sharing ACL routes 4. #30612 settings UI: shared skills table, admin page, member section 5. #30613 chats: chatd loading, `/` menu, docs ## What changes This PR adds the organization skill CRUD routes and makes the skill SDK types owner-neutral so personal and organization skills share them. **API contract change (experimental API, personal and organization skills):** skill list and get responses gain `enabled`, and `PATCH .../skills/{skillName}` now takes `{content?, enabled?}`, so `content` is optional and a toggle doesn't resend it. A PATCH with neither field returns 400; `content`, when present, is validated as before. In the SDK, `UserSkill`, `UserSkillMetadata`, `CreateUserSkillRequest` and `UpdateUserSkillRequest` become `Skill`, `SkillMetadata`, `CreateSkillRequest` and `UpdateSkillRequest` (`codersdk/userskills.go` moves to `codersdk/skills.go`), and `UpdateSkillRequest.Content` is now `*string`. The personal client method names are unchanged. The site changes are the handwritten edits that rename forces (`api.ts`, `queries/userSkills.ts`, `testHelpers/skills.ts` and three call sites), with no UI change. Organization routes under `/api/experimental/organizations/{organization}/skills`, not gated by an experiment or license, like the personal ones: - `GET` lists with the caller's own permissions, so admins, the site owner and org auditors see every skill and members see only skills shared with them. `GET /{skillName}` goes through the new `httpmw.ExtractOrganizationSkillParam` and admits only read, so a missing skill and a denied one both return 404. - `POST` checks `create` on the organization before reading the body, then seeds `group_acl` with the Everyone group (whose ID is the organization ID) so a new skill reaches every member until an admin narrows it. The 100-skill cap and a duplicate name return 409. - `PATCH` admits only callers who can read the skill, because its response includes the content; `DELETE` admits callers who can read or delete it. A reader who can't change the skill gets a write-specific 403 from the database wrapper, and everyone else gets 404. PATCH locks the row as system, updates as the caller in the same transaction, and audits the locked row only after the transaction commits. - Audit: `database.AuditableOrganizationSkill` records resource type `organization_skill` with the organization ID; personal skills keep `user_skill`. Personal and organization handlers share one create and update path in `coderd/userskills.go` (`createSkill`, `updateSkill`, and a `skillWriteErrors` table per owner and action). ## Commits The first commit reproduces the reviewed code from the old stack; each later commit is a labeled trim or fix. - `49cb2c84276` feat: add organization skill API routes. Reproduction: every file this commit changes, including the SDK layer's site edits, equals the reviewed old `write-routes` head `aa597100f28`; the rest of the tree differs only by #30609's migration merge and main. - `dc893bc60d6` refactor(coderd): share the skill create and update path across owners (production +164/-188). It keeps the organization create check before the body read, the lock as system then update as caller order, and audit assignment after the transaction. No swagger, `codersdk` or `typesGenerated.ts` change; `make gen` is clean. - `f94925425ef` test(coderd): merge duplicate personal and organization skill tests. `TestSkillsCRUD`, `TestSkillValidationAndConflicts` and `TestSkillAudit` are table-driven over Personal, OrgAdmin and SiteOwner, alongside `TestOrganizationSkillAuditsDeniedWrites` and one `ScopedAPIKeys` subtest; `TestPatchUserSkill` is deleted because none of its blocks were unique. Coverage of `userskills.go`, `organizationskills.go` and `httpmw/organizationskillparam.go` is 101 of 117 blocks before and after, the same block set. - `ad0a7a37504` fix(coderd): require read access to patch an organization skill. The organization PATCH route now admits only `read`, because its response carries the skill's content: a key scoped to `organization_skill:update` without read gets 404, like GET, instead of 200 with the content. Readers without update still get 403, and DELETE is unchanged. `TestOrganizationSkillAccess/ScopedAPIKeys` asserts the 404, that the response has no content, and that the skill is unchanged; against the old route it fails with 200. - `69d00ded0b6` and `f67b79343ec` merge the updated #30609 layer, including main; `69d00ded0b6` resolves a conflict in the personal skills page with main. No other changes of their own. - `18373ffd59e` merges the updated #30609 layer (main at `e7b76872098`) and reapplies the `AuditableOrganizationSkill` entry in `enterprise/audit/table.go` alongside main's new `AuditableChatProjectInstructions` entries. ## Size Production +838/-308, tests +733/-322, generated +801/-274; total +2372/-904. This is above the 500-line reviewer-friendly limit on purpose: Mike asked for 4 to 5 PRs instead of the 21 small ones this stack replaces. ## Replaces #30587 (scope-neutral SDK and personal enabled state), #30588 (read routes), #30589 (write routes and audit). ## Validation - The coverage comparison above, before and after the test merge. - `make gen` leaves no diff after the shared create and update refactor. - On `18373ffd59e`: `make gen` is clean; build, vet, the `coderd` and `enterprise/coderd` skill tests, `rbac`, `httpmw`, `dbauthz` and `enterprise/audit` pass; `pnpm check`, `pnpm lint` and `make pre-commit` pass. - Remote UAT round 7 through Coder Agents passed on the whole stack; see #30613's Validation for scope and findings. > Xum opened this PR on behalf of @ibetitsmike.
## Stack Part of [CODAGT-1341](https://linear.app/codercom/issue/CODAGT-1341/support-organization-level-skills-in-coder-agents). This stack adds organization skills to Coder Agents: admins publish a skill once per organization, share it with groups or members, and chats in that organization load it next to personal and workspace skills. It replaces the 21-PR draft stack #30584 to #30604 with five PRs, bottom to top: 1. #30609 data model: one migration, queries, RBAC and dbauthz 2. #30610 organization skill API routes and the scope-neutral SDK 3. **#30611** sharing ACL routes (this PR) 4. #30612 settings UI: shared skills table, admin page, member section 5. #30613 chats: chatd loading, `/` menu, docs ## What changes This PR adds the routes that let admins choose who can read an organization skill. It reuses the MCP server ACL machinery rather than adding a second implementation. Routes under `/api/experimental/organizations/{organization}/skills/{skillName}`, all behind `httpmw.ExtractOrganizationSkillParam` with `share`, so a caller without share gets 404: - `GET /acl` returns the resolved user and group grants. - `GET /acl/available` returns the organization's users and groups that can be granted, with the same `q`, `after_id`, `limit` and `offset` paging as the MCP endpoint. - `PATCH /acl` takes a sparse `UpdateOrganizationSkillACLRequest`: only the listed principals change, `read` grants and `""` removes. It returns 204 and writes a `write` audit entry with the old and new ACL. **Concurrency (please review closely):** the PATCH transaction first locks the granted users' `organization_members` rows `FOR KEY SHARE` (new query `LockOrganizationMembersByUserIDsForShare`, run as system), then locks the skill row `FOR UPDATE`, then validates the update on the transaction handle. Member removal takes the same locks in the same order (its membership row, then the skill rows through the #30609 trigger), so the two can't deadlock, and a removal can't miss a grant written by a concurrent PATCH. A skill deleted between the middleware read and the row lock stays a 404. Grants are users and groups, the same as MCP server configs (Mike's decision, for parity). Only the `read` role exists; any other role is a validation error, as are users or groups outside the organization. Supporting changes: - `coderd/mcp_acl.go`: the MCP ACL handlers' resolution, candidate listing and validation move into generic helpers (`writeOrganizationACLAvailable`, `validateOrganizationACLUpdate`, `resolveACLUsers`, `resolveACLGroups`) shared by both resources; `acl.Validate` accepts a named `acl.Role` constraint that now includes `codersdk.OrganizationSkillRole`. - `codersdk/skills.go`: `OrganizationSkillRole`, `OrganizationSkillACL` with its user and group entries, `UpdateOrganizationSkillACLRequest`, and client methods. - New sqlc query `UpdateOrganizationSkillACLByID` with its dbauthz wrapper (requires `share`). ## Commits The first commit reproduces the reviewed code from the old stack; each later commit is a labeled trim or fix. - `b1c85560b69` feat: add organization skill sharing ACL routes. Reproduction: every file this commit changes equals the reviewed old `acl-routes` head `80c1c3594e6`; the rest of the tree differs only by #30609's migration merge and main. - `b5d2ee265c8` test(coderd): merge duplicate organization skill ACL route tests (+49/-111). The removed cases repeated `TestMCPServerConfigACL`, `TestMCPServerConfigACLAvailable` or `TestRolePermissions`; with the MCP tests included, the covered block set of `organizationskills_acl.go`, `mcp_acl.go` and `rbac/acl/updatevalidator.go` is identical before and after. Kept: `TestOrganizationSkillACL` (`OrgAdminManagesACL`, `ReadersGetNotFound`, `SparseUpdateChangesReaders` with the audit diff, `RejectsUnsupportedRole`, `ScopedAPIKeys`), `TestOrganizationSkillACLConcurrentDelete`, and the lock order tests `TestOrganizationSkillACLMemberRemoval` (`RemovalBeforeGrant`, `RemovalAfterGrant`). `OrgAdminManagesACL` now also checks that users from another organization are not candidates (mutation checked). Sharing the param extractor with MCP was skipped: it would save about 10 lines of security-sensitive code. - `e09c4dba66b` Merge the updated org-api layer into sharing. Brings in #30610's `ad0a7a37504`; the merge has no changes of its own. - `1d3817dc639` and `c8c88f84d18` merge the updated #30610 layer, including main; no changes of their own. - `c4d22c47990` merges the updated #30610 layer (main at `e7b76872098`); no changes of its own. ## Size Production +431/-102, tests +377/-0, generated +918/-0; total +1726/-102. This is above the 500-line reviewer-friendly limit on purpose: Mike asked for 4 to 5 PRs instead of the 21 small ones this stack replaces. ## Replaces #30591 (ACL queries), #30592 (ACL routes). ## Validation - `go build` and `go vet` passed; `make gen` exited 0 with no diff. - `go test` passed for `coderd` (skill, MCP and ACL tests), `coderd/database`, `dbauthz`, `migrations`, `gentest`, `coderd/rbac/...`, `coderd/httpmw`, `coderd/x/skills/...`, `coderd/x/chatd/...`, and the audit and skill tests in `enterprise/audit` and `enterprise/coderd`. - The covered block comparison above, before and after the test merge. - On `c4d22c47990`: the same checks as #30610's head, plus `go test ./coderd/ -run 'Skill|MCP|ACL'`, pass. - Remote UAT round 7 through Coder Agents passed on the whole stack; see #30613's Validation for scope and findings. > Xum opened this PR on behalf of @ibetitsmike.
…30612) ## Stack Part of [CODAGT-1341](https://linear.app/codercom/issue/CODAGT-1341/support-organization-level-skills-in-coder-agents). This stack adds organization skills to Coder Agents: admins publish a skill once per organization, share it with groups or members, and chats in that organization load it next to personal and workspace skills. It replaces the 21-PR draft stack #30584 to #30604 with five PRs, bottom to top: 1. #30609 data model: one migration, queries, RBAC and dbauthz 2. #30610 organization skill API routes and the scope-neutral SDK 3. #30611 sharing ACL routes 4. **#30612** settings UI: shared skills table, admin page, member section (this PR) 5. #30613 chats: chatd loading, `/` menu, docs ## What changes This PR turns the personal skills page into a skills table that works for any owner, then uses it for an organization admin page and a read-only member section. It changes only `site/`. **Shared skills table.** The personal page's files move to owner-neutral paths (kept as git renames, so the diff shows edits): `AgentSettingsPersonalSkillsPageView.tsx` becomes `components/SkillsTableView.tsx`, `AgentSettingsPersonalSkillsPage.tsx` becomes `components/SkillsTable.tsx`, `PersonalSkillEditor` becomes `SkillEditor`, and `utils/personalSkills` becomes `utils/skills`. `queries/userSkills.ts` keeps main's query names and keys for personal skills, and the new `queries/organizationSkills.ts` holds the organization and ACL queries. The table takes a `SkillsScope` (list, detail, create, update and delete queries), built by `userSkillsScope` or `organizationSkillsScope` in `utils/skills.ts`, plus an `access` object (`create`, `update`, `delete`) and its copy as props. - **Enabled toggle:** `SkillEnabledSwitch` runs one update mutation per row from the table's scope, is disabled while it is pending, and invalidates the list when it settles, which also drops a skill deleted elsewhere. - **Read-only status:** a table without update access (the member section, and auditors on the admin page) shows a green check ("Enabled") or a gray cross ("Disabled") in the Enabled column instead of a disabled switch. Editable tables keep the switch. - **Read-only view:** when the caller can't update a skill, the row opens `SkillEditor` in read-only mode with a Close button, replacing the old View dialog. It omits the naming hint and starts focus on Close. - Dialogs return focus to the control that opened them (`useDialogFocusReturn`). - Create, update and delete invalidate the skill list instead of patching it by hand. Update also writes the PATCH response to the skill's detail, so a reopened editor shows the new content. - Saving an ACL refreshes the organization skill list and the skill's detail. Deleting a skill removes its detail and the ACL queries cached under it, so a skill recreated with the same name doesn't inherit them. **Admin page** at `/ai/settings/skills?org=<name>` (`AISettingsPage/SkillsPage`): lists, creates, edits, toggles and deletes organization skills with the caller's per-organization permissions, and opens `OrganizationSkillSharingDialog` for users with share. A requested organization the caller can't read shows the permission error instead of falling back to another organization, so a skill is never added to an organization the URL didn't name. The new `viewAnyOrganizationSkills` permission adds Skills to the AI settings sidebar, shows the navbar's AI settings link, and lets the AI settings index redirect to the skills page. The sidebar's Skills link keeps the current `?org=` only when the user can read that organization's skills, using the page's own filter (`readableSkillsOrganizations`), so following the link never lands on the permission error. **Member section.** The agent settings page becomes `/agents/settings/skills` ("Skills" in the sidebar; the old `/agents/settings/personal-skills` URL redirects). Below personal skills, "From your organizations" shows one organization at a time, read-only. Its picker lists the organizations the user is a member of that have enabled skills for them (an organization still loading or whose list failed stays listed), starts on the default organization, and is hidden when only one organization qualifies. Export all downloads only the selected organization's skills. If the selected organization's list fails, a retry button (labeled "Retry loading _name_ skills") replaces the table. **Shared sharing helpers.** `ChatModelPrincipalAutocomplete` becomes `components/ACLPrincipalAutocomplete`, used by the model, MCP server and skill sharing dialogs; `MCPServerPrincipalAutocomplete` is deleted and the common ACL diff logic moves to `aclSharing.ts`. A new `ChatModelSharingDialog.test.tsx` guards the model dialog through the swap. Declined on purpose (Mike's call): extra toggle cache hardening, namely a pending-toggle map, reconciling a View that 404s, and guarding a stale refetch against a retried toggle. Invalidating on settle already refetches the list after every toggle. ## Commits This PR reworks the old site layers rather than reproducing them. The commits go in review order: shared helpers, renames, the shared table, the two pages, then review fixes. 1. `6463992b717` refactor: share the ACL principal autocomplete and sharing helpers. 2. `e18599f5049` refactor: move the personal skills files to shared skills paths (file moves plus import path updates only). 3. `3c71e6c2168` refactor: drop the personal prefix from shared skill symbols. 4. `3be35bcde68` refactor: key the skills table and queries by owner. 5. `d0c1dfcfdd1` fix: return focus to the opener when a skills dialog closes. 6. `92ef6351eb4` feat: add an Enabled toggle to the skills table. 7. `31c103e5396` refactor: move the skill edit and delete dialogs to their own file. 8. `cff0bef4576` feat: view a skill read-only when the user cannot edit it. 9. `a8b54a22981` feat: add the organization skills admin page with sharing. 10. `391401b62b7` feat: show organization skills on the personal skills page. 11. `2cf6e250a60` refactor: keep the skill dialogs in the table view and extract focus return. This undoes the file split from commit 7 so `SkillsTableView.tsx` still diffs as a rename (497 lines then, 507 at the head). 12. `0af93452c7d` test: keep only the role wiring test for the skills page. 13. `8032c6708a8` fix: refetch the skill list after a create when no list is cached (superseded by item 22). 14. `a52f9ba659c` refactor: tidy skills copy, editor prop naming, and story fixtures. 15. `f10bc316305` fix: refresh the organization skill list after saving its ACL. Saving an ACL bumps the skill's `updated_at`, so the save now also invalidates the organization skill list and the skill's detail; a new test in `api/queries/skills.test.ts` (now `organizationSkills.test.ts`) covers it. 16. `698a6d0b55f` Merge the updated sharing layer into settings-ui, bringing in #30610's PATCH fix. 17. `aaa356f0fd5` fix: drop the editing hint and field focus from the read-only skill view. From the remote UAT: read-only View no longer shows the Name field's naming hint, and the dialog opens with focus on Close instead of a field (`onOpenAutoFocus`, like `ChatSearchDialog`). The new `SkillEditor.test.tsx` case "starts focus on Close when read-only" fails without the fix. 18. `698dd83946d` fix: drop a deleted skill's ACL caches with its detail. The delete now removes every query under the skill's key, not only the exact detail; a new case in `api/queries/skills.test.ts` (now `organizationSkills.test.ts`) covers it. 19. `f82573cd1c2` fix: carry only a skill-readable organization on the Skills link, with a table-driven `AISettingsSidebar.test.tsx` case. 20. `0e55137de7b` fix: share the skill-readable organization filter (`SkillsPage/readableSkillsOrganizations.ts`, used by the sidebar and `SkillsPage`) and cover an organization that is listed but unreadable. 21. `97d42fd3cd8` refactor: split skill queries by owner. `queries/skills.ts` goes back to main's `userSkills.ts` (main's names and keys) plus a new `organizationSkills.ts` for the organization and ACL queries; the table takes a `SkillsScope` instead of an owner union. 22. `4ddfb80ac54` refactor: invalidate skill lists after writes, instead of patching the cached list; update still writes the detail from the PATCH response. 23. `14d5b755087` refactor: pass skill organizations to the sidebar as `Organization` objects instead of names. 24. `463df6a5e69` feat: pick one organization on the member skills page. 25. `9c802557b9c` refactor: tighten the skill query comments and tests. 26. `9002dc60382` test: cover the failed sharing save in Vitest (`OrganizationSkillSharingDialog.test.tsx`). 27. `a9df756b976` and `5ebf7b8984a` merge the updated sharing layer, which brings in main. `a9df756b976` moves the `toast` imports in `OrganizationSkillSharingDialog.tsx` and `SkillEnabledSwitch.tsx` to `#/components/Toaster/toast`, as main's Biome import rule now requires, and resolves conflicts with main in the model and MCP server sharing dialogs. 28. `ac3e62c83b7` test: assert the member page's organization switch through requests. After a switch, the test checks that the list and the viewed skill are requested from the selected organization (a Codex review finding). 29. `63bb42754e3` fix: show read-only skill status instead of a disabled switch (from jaaydenh's review). `SkillEnabledSwitch` loses its `disabled` prop; a new `ReadOnly` story in `SkillsTableView.stories.tsx` shows the icons, and the read-only case in `SkillsTable.test.tsx` asserted the status icon and no switch (removed again in item 30). 30. `82b2487ad17` test: address Codex review on skills tests. The `SkillsPage.test.tsx` update-only case now edits the description, saves, and asserts the `updateOrganizationSkill` call. The read-only status case in `SkillsTable.test.tsx` is removed because the `ReadOnly` story covers that visual state, and `renderTable` loses its `access` argument. `AISettingsSidebar.test.tsx`'s hoisted permissions mock drops its cast for an explicit return type. 31. `658be8b8df5` merges the updated sharing layer (main at `e7b76872098`); no changes of its own. ## Size Production +1465/-617, tests and stories +1104/-141 (tests +690 net, stories +273 net), generated +4/-0; total +2573/-758. The site changes in this PR and #30613 net +2263 lines, against +3058 for the same scope in the old stack. This is above the 500-line reviewer-friendly limit on purpose: Mike asked for 4 to 5 PRs instead of the 21 small ones this stack replaces. ## Replaces #30590 (site rename), #30595 (site API), #30596 (skills table), #30597 (Enabled toggle), #30598 (organization page), #30599 (ACL autocomplete), #30600 (permissions), #30603 (member page). ## Validation - `pnpm run lint` clean (tsc, oxlint, knip, React Compiler) and Biome clean; `pnpm run lint` reran clean on `13f03d095ff`, which contains this PR up to `0e55137de7b`. - Vitest: 9 files, 49 tests. Storybook Vitest: 7 files, 101 tests. These full runs were on `a52f9ba659c`; on `0e55137de7b`, the three test files the later fixes change (`api/queries/skills.test.ts`, `AISettingsSidebar.test.tsx`, `SkillEditor.test.tsx`) and the two `SkillsPage` tests that cover the changed `SkillsPage.tsx` pass, 20 tests. - On `a6f8f63ad13`, which contains this PR up to `5ebf7b8984a` and both main merges: 38 Vitest files (the skill queries, the sidebar, the skill pages and components, the slash menu, and `pages/AgentsPage/utils`) pass, 542 tests with 2 skipped. - On `63bb42754e3`: `pnpm check` and the full `pnpm run lint` pass; Vitest (`SkillsTable`, `SkillsPage`, `AgentSettingsSkillsPage`: 6 tests) and Storybook Vitest (the `SkillsTableView`, `AgentSettingsSkillsPage` and `SkillsPage` stories: 34 tests) pass; `coder-frontend-review` passed. - On `82b2487ad17`: `pnpm check`, the full `pnpm run lint`, and Vitest on the three changed test files (7 tests) pass. - On `658be8b8df5`: `go build`, `pnpm check` and the full `pnpm lint` pass; 6 Vitest files with 39 tests pass. - `frontend-review` (FE1 to FE10) and `coder-frontend-review` (8 reviewer roles); findings fixed or declined as noted above. - Remote UAT round 7 through Coder Agents passed on the whole stack, last on `13f03d095ff`, which contains this PR up to `0e55137de7b`; see #30613's Validation for scope and findings. - Remote UAT round 8 passed on `b03c14c86de` (one item graded on intent; see #30613), which contains this PR up to `9c802557b9c`: the member page organization picker, list refresh after writes on the personal and admin tables, no stale ACL after deleting and recreating a skill, and the sidebar Skills `?org=`. See #30613 for details. `9002dc60382`, `ac3e62c83b7`, `63bb42754e3`, `82b2487ad17` and the main merges have tests but no UAT run. > Xum opened this PR on behalf of @ibetitsmike.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.