Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion internal/doctor/skill_health.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func CollectSkillHealthWarnings(opts SkillHealthOptions) []string {
if at == "" {
continue
}
if !toolNames[at] && !isBuiltinOrMetaTool(at) {
if !toolNames[at] && !isRegisteredBuiltin(at) && !isBuiltinOrMetaTool(at) {
// Soft: only warn when the name looks concrete and missing.
out = append(out, fmt.Sprintf("skill %q allowed-tools references %q which is not in the current registry", name, at))
}
Expand Down Expand Up @@ -124,11 +124,25 @@ func normalizedTriggers(in []string) []string {
return out
}

// isRegisteredBuiltin asks the built-in registry, which is the only list that
// cannot drift from the tools that actually ship. isBuiltinOrMetaTool below
// stays as the fallback: it covers subagent and alias names that are dispatched
// by the host rather than registered as tools, and it keeps this check working
// in builds where the builtin package is not linked in.
func isRegisteredBuiltin(name string) bool {
_, ok := tool.LookupBuiltin(name)
return ok
}

func isBuiltinOrMetaTool(name string) bool {
switch name {
case "bash", "read_file", "write_file", "edit_file", "grep", "glob", "ls",
"todo_write", "complete_step", "ask", "task", "read_only_task",
"parallel_tasks", "fleet",
// Host-wired at boot rather than registered as a compile-time built-in,
// so isRegisteredBuiltin cannot see it. The built-in skills name it in
// allowed-tools, so leaving it out warned on every session.
"use_capability",
"run_skill", "read_skill", "read_only_skill", "explore", "research",
"review", "security_review", "web_fetch", "multi_edit", "move_file",
"code_index", "wait", "bash_output", "kill_shell":
Expand Down
63 changes: 63 additions & 0 deletions internal/doctor/skill_health_registry_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package doctor

import (
"context"
"encoding/json"
"strings"
"testing"

"reasonix/internal/skill"
"reasonix/internal/tool"
)

type registryProbeTool struct{}

func (registryProbeTool) Name() string { return "doctor_registry_probe" }
func (registryProbeTool) Description() string { return "probe" }
func (registryProbeTool) Schema() json.RawMessage {
return json.RawMessage(`{"type":"object"}`)
}
func (registryProbeTool) ReadOnly() bool { return true }
func (registryProbeTool) Execute(context.Context, json.RawMessage) (string, error) {
return "", nil
}

// A skill may name any registered built-in in allowed-tools. The hand-written
// list in isBuiltinOrMetaTool had drifted from the tools that actually ship —
// use_capability, compress and update_goal were all missing, so every session
// carrying a skill that names one reported a warning for a tool that exists.
// Consult the registry, which cannot drift, before falling back to the list.
func TestAllowedToolsAcceptsAnyRegisteredBuiltin(t *testing.T) {
tool.RegisterBuiltin(registryProbeTool{})

warns := CollectSkillHealthWarnings(SkillHealthOptions{
Skills: []skill.Skill{{
Name: "probe",
Description: "ok",
AllowedTools: []string{"doctor_registry_probe"},
}},
})
for _, w := range warns {
if strings.Contains(w, "doctor_registry_probe") {
t.Fatalf("registered built-in reported as missing: %s", w)
}
}
}

// The complement: a name that is neither registered nor a known meta tool must
// still warn, so the softened check does not silence real typos.
func TestAllowedToolsStillWarnsOnAnUnknownName(t *testing.T) {
warns := CollectSkillHealthWarnings(SkillHealthOptions{
Skills: []skill.Skill{{
Name: "probe",
Description: "ok",
AllowedTools: []string{"definitely_not_a_tool"},
}},
})
for _, w := range warns {
if strings.Contains(w, "definitely_not_a_tool") {
return
}
}
t.Fatal("unknown allowed-tools name did not warn")
}