feat: add personal skill resolver#25362
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
/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.
Clean package introduction. The types are minimal, the dependency direction is correct (coderd/x/skills imports codersdk/workspacesdk, not the reverse), and the doc.go is a proper ADR with glossary, decision rationale, and explicit scope boundaries. The Parse/Validate layering is deliberate and well-tested. slices.Sorted(maps.Keys(...)) for deterministic iteration, proper %w wrapping, no outdated patterns. Ging-Go found nothing to modernize. Mafu-san found nothing to flag.
Two P2s, five P3s, three nits.
The P2s are the center of gravity. First: Lookup has two return states (found, not found) when the resolver's contract requires three (found, not found, ambiguous). When personal and workspace skills collide, bare-name lookup returns "skill not found" for a skill that exists in both sources. The API surface is being defined now with zero callers, so adding ErrSkillAmbiguous is a three-line fix that prevents every future consumer from reimplementing disambiguation. Three reviewers flagged this independently. Second: the missing-name error path strips all diagnostic context, producing "invalid skill name" when the user didn't provide a name at all, while the non-kebab path wraps with the offending value and the regex pattern.
Pariston: "Could the merge logic be removed entirely? Only if one source shadows the other, which the design explicitly rejects. Could the package be simpler? I tried to build an argument that the abstraction doesn't earn its keep. It does."
Notes not posted inline: six reviewers independently observed that skillsByName overrides Skill.Source from the parameter position, making the field write-only on input. The override is an intentional safety belt and works correctly. Mafuuu noted that ParsedSkill.Body is post-processed (HTML comments stripped, whitespace trimmed), not raw content; the struct doc should note this for the API layer in #25363. Mafu-san noted doc.go references the user_skills table from #25363, a forward reference that risks staleness.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
| // Package skills defines the shared model for personal and workspace skills | ||
| // used by chatd. | ||
| // | ||
| // Glossary: | ||
| // | ||
| // - Personal skill: A user-owned skill that follows the user across Coder | ||
| // chats and workspaces, stored by Coder rather than discovered from a | ||
| // workspace filesystem. | ||
| // - Workspace skill: A skill discovered from the workspace filesystem, | ||
| // currently under .agents/skills by default. | ||
| // - Skill source: The origin of a skill available to chatd, such as personal | ||
| // storage or workspace filesystem discovery. | ||
| // - Skill alias: A chat or tool lookup name for a skill. Bare aliases use the | ||
| // skill name. Collision aliases use personal/<name> or workspace/<name>. | ||
| // | ||
| // Decision: | ||
| // | ||
| // Personal skills are stored by Coder. For each chat turn, chatd fetches | ||
| // personal skill metadata fresh, combines it with workspace skill metadata, and | ||
| // injects the available skills into the existing skill prompt. | ||
| // When chatd needs skill content, it resolves personal skills through the | ||
| // read_skill flow instead of syncing files into workspace filesystems. | ||
| // | ||
| // If a personal skill and workspace skill share the same kebab-case name, both | ||
| // are exposed with collision aliases: personal/<name> for the personal skill | ||
| // and workspace/<name> for the workspace skill. One source must not silently | ||
| // override the other. | ||
| // | ||
| // Site admins can read and modify personal skill content. This is a deliberate | ||
| // privacy trade-off for operability, support, and abuse handling. Audit records | ||
| // intentionally exclude raw Markdown content, but record the actor, target | ||
| // user, and relevant metadata. | ||
| // | ||
| // Personal skill edits affect the next chat turn. Old chat turns are not exact | ||
| // snapshots of the personal skill state that existed when they ran. | ||
| // | ||
| // The v1 design does not include CLI support, web UI support, supporting files, | ||
| // organization-scoped personal skills, syncing personal skills into workspace | ||
| // filesystems, or stable public API documentation. | ||
| // | ||
| // Consequences: | ||
| // | ||
| // Chatd can use personal and workspace skills through one prompt and one read | ||
| // path, while storage remains owned by Coder instead of individual workspace | ||
| // filesystems. Fresh metadata keeps skill changes responsive, but chat history | ||
| // is less reproducible because old turns do not capture an exact copy of | ||
| // personal skill content. | ||
| // | ||
| // Explicit collision aliases make ambiguous names visible to users and tools. | ||
| // Admin access improves operability and abuse handling, but it creates a | ||
| // privacy trade-off that must remain clear in product and support expectations. | ||
| package skills |
There was a problem hiding this comment.
@johnstcn was the one that hinted on doing this
| // MaxPersonalSkillSizeBytes, frontmatter must contain a valid kebab-case name, | ||
| // the skill name must not exceed MaxPersonalSkillNameBytes, and the body after | ||
| // frontmatter must be non-empty. | ||
| func ValidatePersonalSkillMarkdown(raw []byte) (ParsedSkill, error) { |
There was a problem hiding this comment.
Is there a reason not to embed this into ParsePersonalSkillMarkdown?
Are some skills not subject to the constraints?
There was a problem hiding this comment.
good catch, will fix
| } | ||
|
|
||
| // Lookup finds a resolved skill by bare alias or qualified source alias. | ||
| func Lookup(resolved []ResolvedSkill, lookup string) (ResolvedSkill, error) { |
There was a problem hiding this comment.
I imagine it is fine to iterate over the list each time because the lists are small and this is not used in any tight loops
|
/coder-agents-review
|
There was a problem hiding this comment.
All 14 round 1 findings addressed cleanly. The DEREM-3 fix (ErrSkillAmbiguous) completes the Lookup contract. The DEREM-8 fix (MaxSkillMetaBytes centralization) was done well: the agent searched for all sibling copies and unified them without being told which files held them. Every file in the fix commit traces to a specific finding, no scope drift.
One P3, four nits.
The P3 is an edge introduced by the DEREM-3 fix: when len(matches) == 1, Lookup drops the single match and returns ErrSkillNotFound. Through MergeSkills this is unreachable (non-colliding skills match on alias, colliding skills always produce pairs), but Lookup is exported and accepts any []ResolvedSkill. Three reviewers verified this empirically. The fix is a doc comment stating the MergeSkills constraint, or handling the single-match case.
Re @Emyrk's question on skills.go:97 about embedding validation into Parse: the Parse/Validate split is intentional. Parse is lenient (accepts non-kebab names, oversized content) for use cases that need raw parsing without business rules. Validate layers constraints on top. Tests prove the boundary: AcceptsNonKebabCaseName and AcceptsOversizedContent demonstrate Parse accepts what Validate rejects.
Re @Emyrk on skills.go:182 about iterating the list: confirmed, Chopper and Hisoka both evaluated this. The lists are small (bounded by MaxPersonalSkillsPerUser on one side, workspace directory count on the other), and Lookup is called per-tool-invocation, not in a tight loop. Linear scan is appropriate.
Mafu-san: "Every file in the fix commit traces to a specific finding. No scope drift, no 'while I'm here' changes."
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review
|
There was a problem hiding this comment.
All 19 findings across three rounds addressed. The DEREM-17 fix (Lookup single-match fallback) restructures the control flow to a clean 3-way switch with a regression test that exercises the exact path. Pariston: "I tried to build a case against this change and could not. The problem is correctly understood, the solution is proportional, and the fix is at the right level."
One nit: DEREM-19's fix caught two of three "collision aliases" occurrences in doc.go but missed the third at line 49. Classic incomplete-sibling pattern.
The package is well-designed: minimal types, correct dependency direction, deliberate parse/validate split, documented preconditions, and strong test coverage (~230 lines code, ~360 lines test). All sentinel errors carry diagnostic context. All exported functions have doc comments that describe behavior, preconditions, and error conditions.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review
|
There was a problem hiding this comment.
DEREM-23 (Nit, doc.go:49) remains unaddressed. The finding asks to replace "collision aliases" with "qualified aliases" on line 49 to match the terminology fix already applied at lines 14 and 25. The new commit d140a3a modifies doc.go but does not touch line 49.
Further review is blocked until this finding is addressed or the author responds.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review
|
There was a problem hiding this comment.
DEREM-23 addressed in 01003e4. All 23 findings across 5 rounds resolved. No open items remain.
🤖 This review was automatically generated with Coder Agents.
ibetitsmike
force-pushed
the
mike/personal-skills-resolver
branch
from
01003e4 to
10b94ad
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Rebase onto new main (191dd23). No code changes to PR files since R5. All 23 findings across 5 prior rounds remain resolved. No new findings.
🤖 This review was automatically generated with Coder Agents.
2 participants
Stack Context
This stack splits experimental personal skills into smaller reviewable PRs. Personal skills are user-owned
SKILL.mdfiles stored by Coder and injected into chatd alongside workspace skills.Stack order:
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