feat: add personal skill resolver by ibetitsmike · Pull Request #25362 · coder/coder

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.