feat: support personal skills in chats#25366
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
ibetitsmike
changed the title
ibetitsmike
changed the base branch from
mike/personal-skills-api-tests
to
graphite-base/25366
ibetitsmike
changed the title
ibetitsmike
changed the base branch from
graphite-base/25366
to
mike/personal-skills-api-tests
|
/coder-agents-review |
![coder-agents-review[bot]](https://avatars.githubusercontent.com/in/3454270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Well-structured integration PR. The source-aware skill resolution, collision aliasing, error sanitization, and mid-turn refresh are all cleanly implemented with thorough test coverage (1:1.76 code-to-test ratio). The userSkillContext RBAC comment is one of the better security justifications in the codebase.
Severity breakdown: 5 P3, 1 P4, 2 Nit. No blocking issues.
The strongest signal across the panel is the cross-package coupling between isSkillIndexMessage (chatd.go) and renderSkillIndex (skill.go), which 10+ reviewers flagged independently. A shared constant for the XML tags would make the dependency explicit and compiler-enforced.
Process note: workspace skill errors still leak raw err.Error() to the LLM (pre-existing), while personal skill errors are properly sanitized. This PR sets the better standard; aligning workspace errors would be a good follow-up.
"The person reading this at 2 AM knows exactly why this actor exists and what it can't do." (Leorio, on the
userSkillContextcomment)
🤖 This review was automatically generated with Coder Agents.
ibetitsmike
force-pushed
the
mike/personal-skills-chattool
branch
from
5acacdf to
ddcbe2a
Compare
ibetitsmike
force-pushed
the
mike/personal-skills-api-tests
branch
from
3066778 to
b0d7678
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All 8 R1 findings addressed cleanly. Constants for the XML tags, single-site logging, explicit error type checks, injectedSkillIndex sync, skill name in error messages, doc comments, simplified delegation, and consistent JSON nullability are all verified.
One new finding from R2: ParsePersonalSkillMarkdown failures in loadPersonalSkillBody produce no server-side log entry. The DB error path logs, the parse error path does not.
"If validation rules evolve between versions or if a migration corrupts content, the operator has no trace of the root cause." (Chopper)
🤖 This review was automatically generated with Coder Agents.
ibetitsmike
force-pushed
the
mike/personal-skills-api-tests
branch
from
b0d7678 to
6231ffc
Compare
ibetitsmike
force-pushed
the
mike/personal-skills-chattool
branch
from
ddcbe2a to
83fc999
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
DEREM-12 verified fixed. All 12 prior findings are closed.
One new finding: the test helper that validates the synthesized RBAC actor only checks the actor ID, not the security-critical role.
Razor traced a pre-existing edge case worth knowing: workspace skill names are not validated against SkillNamePattern, so a workspace SKILL.md declaring name: personal/deploy could shadow a personal skill's qualified alias. The fix belongs in workspace skill discovery, not this PR.
🤖 This review was automatically generated with Coder Agents.
ibetitsmike
force-pushed
the
mike/personal-skills-api-tests
branch
from
6231ffc to
81c22bf
Compare
ibetitsmike
force-pushed
the
mike/personal-skills-chattool
branch
from
83fc999 to
53e45b3
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All 13 prior findings verified fixed. DEREM-13 (role assertion) confirmed addressed.
One new finding: the description tag on ReadSkillArgs.Name and ReadSkillFileArgs.Name says "kebab-case" but qualified aliases (personal/deploy) contain /. Models that weight schema descriptions may avoid passing qualified aliases, causing "not found" on collision scenarios.
coderd/x/chatd/chattool/skill.go:282
P3 [DEREM-14] The description tag reads "The kebab-case name of the skill to read." (same pattern at line 353 for ReadSkillFileArgs). This PR introduces collision aliases of the form personal/deploy and workspace/deploy, which contain / and are not kebab-case. The schema description now contradicts the tool's actual input contract.
The system prompt mitigates with "When a skill is listed as personal/name or workspace/name, pass that qualified alias to read_skill." But models that weight schema descriptions heavily may still avoid non-kebab-case inputs, causing "not found" on collision scenarios.
Could we update both descriptions to "The name or qualified alias of the skill to read."? (Mafuuu P3)
🤖
🤖 This review was automatically generated with Coder Agents.
ibetitsmike
force-pushed
the
mike/personal-skills-api-tests
branch
from
81c22bf to
eb5d4a3
Compare
ibetitsmike
force-pushed
the
mike/personal-skills-chattool
branch
from
53e45b3 to
c3af07b
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Review blocked. DEREM-14 (P3) has not been addressed or acknowledged.
DEREM-14 (skill.go:282,353): ReadSkillArgs.Name and ReadSkillFileArgs.Name description tags say "The kebab-case name of the skill to read" but this PR introduces qualified aliases (personal/deploy, workspace/deploy) that contain / and are not kebab-case. The schema description contradicts the tool's input contract. Models that weight schema descriptions may avoid passing qualified aliases, causing "not found" on collision scenarios. Suggested fix: update both descriptions to "The name or qualified alias of the skill to read."
Note: this finding was folded into the R4 review body because line 282 is outside the diff hunks; it may have been easy to miss.
Further review is blocked until this finding is addressed, acknowledged, or contested.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
ibetitsmike
force-pushed
the
mike/personal-skills-chattool
branch
from
990a6d5 to
819bb7b
Compare
ibetitsmike
force-pushed
the
mike/personal-skills-chattool
branch
from
819bb7b to
1ce975e
Compare
ibetitsmike
force-pushed
the
mike/personal-skills-api-tests
branch
from
5b19fe4 to
af115ac
Compare
|
/coder-agents-review |
> Mux updated this PR on behalf of Mike. ## Stack Context This stack splits experimental personal skills into smaller reviewable PRs. Personal skills are user-owned `SKILL.md` files stored by Coder and injected into chatd alongside workspace skills. Stack order: 1. #25362 personal skill resolver 2. #25363 storage, permissions, API, and SDK 3. #25365 API test coverage 4. #25366 chattool and chatd integration 5. #25066 settings UI and docs 6. #25386 personal skills slash menu ## What? Adds the shared personal skill parser and resolver package, plus reusable skill-name validation exported from `workspacesdk`. The parser enforces the full personal skill contract: max raw size, kebab-case name, max name length, and non-empty body. ## Why? The rest of the stack needs one source-aware resolver for personal and workspace skills, including collision handling and qualified aliases. Keeping personal skill constraints in the parser prevents callers from accidentally parsing invalid personal skills. ## Validation - `go test ./coderd/x/skills ./codersdk/workspacesdk` - pre-commit hooks on this branch
ibetitsmike
force-pushed
the
mike/personal-skills-api-tests
branch
from
af115ac to
eccb82b
Compare
ibetitsmike
force-pushed
the
mike/personal-skills-chattool
branch
from
1ce975e to
3d0b1a0
Compare
ibetitsmike
force-pushed
the
mike/personal-skills-api-tests
branch
from
eccb82b to
e07c510
Compare
ibetitsmike
force-pushed
the
mike/personal-skills-chattool
branch
from
3d0b1a0 to
34c794f
Compare
> Mux updated this PR on behalf of Mike. ## Stack Context This PR is the storage, permissions, API, and SDK layer for experimental personal skills. #25362 has landed on `main`, so this branch is restacked directly on `main`. Stack order: 1. #25363 storage, permissions, API, and SDK 2. #25365 API test coverage 3. #25366 chattool and chatd integration 4. #25066 settings UI and docs 5. #25386 personal skills slash menu ## What? Adds the `user_skills` database table, generated queries, RBAC resources and scopes, audit resource handling, experimental user-scoped CRUD endpoints, SDK types, and generated API/site types. Follow-up review and restack fixes: - Enforce a bounded personal skill description in parser and database constraints. - Return `403 Forbidden` for unauthorized create and update attempts. - Return explicit conflict responses when soft-deleted users are targeted. - Keep user admins out of personal skills, while site owners can read and delete but not create or update. - Document trigger-raised constraint names and keep schema constants covered by tests. - Reuse `UserSkillMetadata` in the full `UserSkill` SDK response type. - Generate user skill IDs in Go instead of relying on a database default. - Rebase on latest `main` and renumber the user skills migration to `000502_user_skills`. ## Why? Personal skills need durable user-owned storage with owner authorization, limited site-owner moderation, and a hidden API surface before chatd can consume them. ## Validation - `make gen` - `go test ./coderd/database -run '^TestUserSkillSchemaConstants$' -count=1` - `go test ./coderd/database/dbauthz -run '^TestMethodTestSuite/TestUserSkills$' -count=1` - `go test ./coderd -run '^TestPatchUserSkill$' -count=1` - `go test ./codersdk ./coderd/database/db2sdk` - `make lint` - pre-commit hook on `97fd58108d`
ibetitsmike
force-pushed
the
mike/personal-skills-api-tests
branch
3 times, most recently
from
844b187 to
604997b
Compare
ibetitsmike
force-pushed
the
mike/personal-skills-chattool
branch
from
34c794f to
1883c4a
Compare
ibetitsmike
force-pushed
the
mike/personal-skills-api-tests
branch
from
604997b to
488acb8
Compare
ibetitsmike
force-pushed
the
mike/personal-skills-chattool
branch
from
1883c4a to
fab1f7d
Compare
> Mux updated this PR on behalf of Mike. ## Stack Context This PR is the API test coverage slice in the experimental personal skills stack. The storage, schema, permissions, API, and SDK implementation merged in #25363. Stack order: 1. #25362 personal skill resolver 2. #25363 storage, permissions, API, and SDK 3. #25365 API test coverage 4. #25366 chattool and chatd integration 5. #25066 settings UI and docs 6. #25386 personal skills slash menu ## What? Adds API and audit tests for personal skill CRUD, validation failures, limits, authorization, soft-delete cleanup, and audit content tracking. This PR is now test-only. It does not include migrations, generated database code, or API implementation changes. ## Why? The feature touches storage, permissions, and audit behavior. These tests make the server behavior reviewable and protected without re-reviewing the implementation that already merged in #25363. ## Validation - `go test ./coderd -run '^(TestUserSkill|TestPatchUserSkill)' -count=1` - `go test ./enterprise/coderd -run '^TestUserSkillAuditDiffTracksContent$' -count=1` - pre-commit hook via `gt modify --no-edit`
Emyrk
left a comment
Emyrk
left a comment
There was a problem hiding this comment.
Chatd diffs 👍 .
Won't apply scrutiny
2 participants
Stack Context
This PR builds on #25365 in the experimental personal skills stack and completes the chat integration.
Stack order:
What?
Updates chattool skill formatting and
read_skillresolution so tools can read personal skills from the database, then injects personal skill metadata into chatd prompts and registers the skill-reading tools when skills are available.This branch has also been merged with current
origin/mainto resolve merge conflicts.Why?
The chattool and chatd changes need to land together so the intermediate stack state stays buildable. This completes personal skill availability in chats without syncing personal skills into workspace filesystems.
Validation
go test -count=1 ./coderd/x/chatd/chattool -run 'TestFormatResolvedSkillIndex|TestReadSkillTool|TestReadSkillFileTool'go test -count=1 ./coderd/x/chatd -run 'TestPersonalSkillsInSystemPrompt|TestPersonalAndWorkspaceSkillCollisionInSystemPrompt|TestSkillIndexRefreshReplacesStaleAliases|TestFetchPersonalSkillMetadata|TestLoadPersonalSkillBody'go test -count=1 ./coderd -run 'Test.*UserSkill'git diff --cached --checkmake lint