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 userSkillContext comment)

🤖 This review was automatically generated with Coder Agents.

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.

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.

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.

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.

Post-approval rebase introduced ErrSkillAmbiguous from the downstack PR. The test was updated to expect it, but the tool handlers weren't updated to surface the disambiguation information.

One new finding: when the LLM calls read_skill("deploy") and both personal/deploy and workspace/deploy exist, the handler returns a generic "failed to resolve skill" instead of telling the LLM which qualified aliases to use. The qualified aliases are safe to surface (already in the system prompt).

🤖 This review was automatically generated with Coder Agents.

All 15 findings across 8 rounds are resolved. DEREM-15 (ErrSkillAmbiguous) verified fixed: skillResolveErrorResponse now surfaces qualified aliases to the LLM via a dedicated ErrSkillAmbiguous branch, with test coverage in both ReadSkill and ReadSkillFile tools.

No new findings from the R8 panel or Netero. Clean.

🤖 This review was automatically generated with Coder Agents.

No-op rebase (zero diff in chatd files). All 15 findings remain fixed. No new findings from the R9 panel or Netero.

🤖 This review was automatically generated with Coder Agents.

No-op rebase (zero diff in chatd files). All 15 findings remain fixed. No new findings from the R10 panel or Netero.

🤖 This review was automatically generated with Coder Agents.

No-op rebase (zero diff in chatd files). All 15 findings remain fixed. No new findings from the R11 panel or Netero.

🤖 This review was automatically generated with Coder Agents.

No-op rebase (zero diff in chatd files). All 15 findings remain fixed. No new findings from the R12 panel or Netero.

🤖 This review was automatically generated with Coder Agents.