diff --git a/cmd/tools/gen-error-codes/main.go b/cmd/tools/gen-error-codes/main.go index 4c8a601..bd89808 100644 --- a/cmd/tools/gen-error-codes/main.go +++ b/cmd/tools/gen-error-codes/main.go @@ -26,7 +26,8 @@ import ( // Side-effect imports: load every package that calls errcode.Register // so the registry is populated before we read it. - _ "github.com/sqlrush/opendbx/internal/app/skills" // spec-2.1 D-6: SKILL.* codes + _ "github.com/sqlrush/opendbx/internal/app/skills" // spec-2.1 D-6: SKILL.* codes + _ "github.com/sqlrush/opendbx/internal/app/skills/invoke" // spec-2.3 D-6: SKILL.* invoke codes _ "github.com/sqlrush/opendbx/internal/entrypoints" "github.com/sqlrush/opendbx/internal/platform/errcode" _ "github.com/sqlrush/opendbx/internal/platform/logger" diff --git a/internal/app/cli/render/block/adapter/skill.go b/internal/app/cli/render/block/adapter/skill.go new file mode 100644 index 0000000..73a2e05 --- /dev/null +++ b/internal/app/cli/render/block/adapter/skill.go @@ -0,0 +1,48 @@ +// Copyright 2026 opendbx contributors. See LICENSE. +// +// Author: sqlrush + +// File skill.go — Skill tool adapter (spec-2.3 D-4). +// +// CC shows a "Using " banner when the model enters a skill +// (survey skills.md § 2.3); the exact CC tool-row text is line-level TBD +// (survey § 7 T-11), so this header is the spec-2.3 pinned provisional +// form: "Skill()" plus a compact args summary when args are +// present. Errata-aligned after T-11 verify (spec-2.1 R-1 precedent). + +package adapter + +import "strings" + +// Skill implements HeaderRenderer for the spec-2.3 SkillTool +// (wire name "Skill"; input {skill, args}). +type Skill struct{} + +// RenderHeader returns `Skill()`, appending a sorted compact +// `k=v` args summary when a non-empty args object is present. A missing +// or non-string skill field renders the "(no skill)" placeholder +// (mirrors Bash's "(no command)" convention). +func (Skill) RenderHeader(input map[string]any, ctx Context) (string, error) { + name, ok := input["skill"].(string) + if !ok || strings.TrimSpace(name) == "" { + return "Skill(no skill)", nil + } + head := "Skill(" + name + ")" + args, ok := input["args"].(map[string]any) + if !ok || len(args) == 0 { + return head, nil + } + maxLen := ctx.Cols / 2 + if maxLen < 8 { + maxLen = 8 + } + summary := compactArgs(args, maxLen-len(head)-1) + if summary == "" { + return head, nil + } + return head + " " + summary, nil +} + +func init() { + Default.Register("Skill", Skill{}) +} diff --git a/internal/app/cli/render/block/adapter/skill_test.go b/internal/app/cli/render/block/adapter/skill_test.go new file mode 100644 index 0000000..46a3714 --- /dev/null +++ b/internal/app/cli/render/block/adapter/skill_test.go @@ -0,0 +1,72 @@ +// Copyright 2026 opendbx contributors. See LICENSE. +// +// Author: sqlrush + +package adapter + +import ( + "strings" + "testing" +) + +// TestSkill_Registered — the Skill adapter self-registers under the +// CC-parity wire name (spec-2.3 D-4). +func TestSkill_Registered(t *testing.T) { + t.Parallel() + if Default.Lookup("Skill") == nil { + t.Fatal("Skill adapter not registered in Default registry") + } +} + +// TestSkill_RenderHeader — table over name/args shapes (spec-2.3 D-4). +func TestSkill_RenderHeader(t *testing.T) { + t.Parallel() + ctx := Context{Cols: 80} + tests := []struct { + name string + input map[string]any + want string + }{ + {"plain", map[string]any{"skill": "code-reviewer"}, "Skill(code-reviewer)"}, + {"missing skill", map[string]any{}, "Skill(no skill)"}, + {"non-string skill", map[string]any{"skill": 7}, "Skill(no skill)"}, + {"blank skill", map[string]any{"skill": " "}, "Skill(no skill)"}, + {"empty args object", map[string]any{"skill": "s", "args": map[string]any{}}, "Skill(s)"}, + {"non-object args ignored for display", map[string]any{"skill": "s", "args": "x"}, "Skill(s)"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got, err := Skill{}.RenderHeader(tc.input, ctx) + if err != nil || got != tc.want { + t.Errorf("RenderHeader(%v) = %q, %v; want %q", tc.input, got, err, tc.want) + } + }) + } +} + +// TestSkill_RenderHeader_ArgsSummary — args render as a sorted compact +// summary after the head; narrow columns degrade to head-only. +func TestSkill_RenderHeader_ArgsSummary(t *testing.T) { + t.Parallel() + input := map[string]any{ + "skill": "topsql", + "args": map[string]any{"limit": "10", "db": "main"}, + } + got, err := Skill{}.RenderHeader(input, Context{Cols: 120}) + if err != nil { + t.Fatal(err) + } + if !strings.HasPrefix(got, "Skill(topsql) ") { + t.Errorf("want head prefix, got %q", got) + } + // Sorted keys: db before limit. + if !strings.Contains(got, "db=") || strings.Index(got, "db=") > strings.Index(got, "limit=") { + t.Errorf("args summary must be key-sorted: %q", got) + } + // Narrow terminal → summary dropped, head kept. + gotNarrow, _ := Skill{}.RenderHeader(input, Context{Cols: 20}) + if gotNarrow != "Skill(topsql)" { + t.Errorf("narrow cols should keep head only, got %q", gotNarrow) + } +} diff --git a/internal/app/diagnose/errors.go b/internal/app/diagnose/errors.go index 25d5b70..74a28b9 100644 --- a/internal/app/diagnose/errors.go +++ b/internal/app/diagnose/errors.go @@ -17,6 +17,9 @@ package diagnose import ( + "fmt" + "strings" + "github.com/sqlrush/opendbx/internal/platform/errcode" ) @@ -60,4 +63,26 @@ var ( "收到非预期 pause_turn (spec-1.21 未启用 server tool)", "server-side tool 是未来 spec; 检查 model/请求未启用 server tool", ) + // ErrScopeToolDenied — the model called a registered tool that the + // active skill scope filters out (spec-2.3 D-3 dispatch guard). + // Feedback class — written into ToolResult.Content with IsError=true; + // the model self-corrects. NOT terminal. Registered here (not in + // app/skills/invoke, which owns the other two spec-2.3 codes) because + // the Loop composes the denial text and diagnose cannot import invoke + // (cycle); same placement precedent as DIAGNOSE.TOOL_TIMEOUT. + ErrScopeToolDenied = errcode.Register( + "SKILL.SCOPE_TOOL_DENIED", + "tool is not allowed in the active skill scope", + "use an allowed tool or invoke another skill", + ) ) + +// scopeDeniedContent composes the recoverable ToolResult.Content for a +// scope-filtered tool call (spec-2.3 D-6 pinned template — colon style, +// matching the invoke-side templates; post-impl cr MED-1). allowed is the +// active normalized filter ("Skill" is implicitly retained and therefore +// listed separately in the fixed "(+ Skill)" suffix). +func scopeDeniedContent(tool string, allowed []string) string { + return fmt.Sprintf("%s: tool %q is not allowed in the active skill scope. Allowed: [%s] (+ Skill). Hint: %s.", + ErrScopeToolDenied.Code(), tool, strings.Join(allowed, ", "), ErrScopeToolDenied.Hint()) +} diff --git a/internal/app/diagnose/loop.go b/internal/app/diagnose/loop.go index dbcecda..00c4daa 100644 --- a/internal/app/diagnose/loop.go +++ b/internal/app/diagnose/loop.go @@ -159,6 +159,13 @@ func (l *Loop) Run(ctx context.Context, req llm.Request, emit EmitFunc) (Result, tools = append(tools, l.registry.Schemas()...) } + // Run-local allowed-tools scope (spec-2.3 D-3). nil = no scope. Set + // from a successful ToolOutput.ToolFilter (fresh OR cached — never + // gated on !cached); replaced wholesale by the next one; dies with + // this Run. v1 enforcement covers registry-dispatchable tools only + // (production req.Tools is nil; spec-2.3 ❌-11). + var activeFilter []string + result := Result{} // Per-Run dedup cache (spec-1.22 D-1/D-2). Lifetime == this Run; not @@ -174,10 +181,12 @@ func (l *Loop) Run(ctx context.Context, req llm.Request, emit EmitFunc) (Result, return finalize(result, msgs, fr, "", ferr), ferr } - // Per-turn provider call. + // Per-turn provider call. The advertised tool set is re-derived + // every turn so a scope entered mid-run narrows the next turn's + // offer (spec-2.3 D-3; nil filter → same slice, zero-cost). turnReq := req turnReq.Messages = msgs - turnReq.Tools = tools + turnReq.Tools = applyFilter(tools, activeFilter) turnCtx, cancelTurn := context.WithTimeout(totalCtx, l.reqTimeout) stream, perr := l.provider.Stream(turnCtx, turnReq) if perr != nil { @@ -337,6 +346,29 @@ func (l *Loop) Run(ctx context.Context, req llm.Request, emit EmitFunc) (Result, tu := &toolUses[i] exec, _ := lookup(l.registry, tu.Name) + // spec-2.3 D-3 dispatch guard — AFTER the registry lookup + // (unknown names already went terminal above) and BEFORE + // dedup (a denied call must not touch the cache). The + // denied tool still gets a full recoverable tool_result so + // the Phase 1 EventToolCall is paired (UI never stuck + // Running) and the paired commit keeps its shape. Scope + // entered earlier in THIS same Phase 2 applies immediately + // (Q12 user decision: minimal scope beats advertised- + // earlier permissiveness). + if scopeDenied(activeFilter, tu.Name) { + results = append(results, llm.ToolResult{ + ToolUseID: tu.ID, + Content: scopeDeniedContent(tu.Name, activeFilter), + IsError: true, + }) + if eerr := emit(ctx, Event{Kind: EventToolResult, Turn: turn, ToolResult: &results[len(results)-1]}); eerr != nil { + fr, ferr := classifyEmitErr(eerr) + // errcode-lint:exempt -- spec-1.21 D-4: emit-error pass-through (EventToolResult variant). + return finalize(result, msgs, fr, "", ferr), ferr + } + continue + } + // spec-1.22 D-2: dedup interception. dKey is "" when the tool // is non-cacheable / dedup disabled / key derivation failed // (invariant #5: such a call derives no key, never stores). @@ -366,6 +398,16 @@ func (l *Loop) Run(ctx context.Context, req llm.Request, emit EmitFunc) (Result, // errcode-lint:exempt -- spec-1.21 D-4: ferr is a registered DIAGNOSE.TOTAL_TIMEOUT sentinel or ctx.Canceled from classifyToolErr; pass-through. return finalize(result, msgs, fr, code, ferr), ferr } + // spec-2.3 D-3 scope update — reads the raw `out` (NOT tr: + // classifyToolErr copies Content/IsError only, which is + // also what keeps ToolFilter off the wire). Gated on + // success alone — never on !cached, so a dedup replay + // re-applies its filter identically (spec-2.3 DoD). A nil + // ToolFilter (fresh OR cached) means inherit: the current + // scope is left untouched, never cleared (Q4/go LOW-2). + if execErr == nil && !out.IsError && out.ToolFilter != nil { + activeFilter = normalizeFilter(out.ToolFilter) + } // spec-1.22 R-4: store ONLY a freshly-executed success // (execErr==nil && !IsError). A hit is not re-stored; an // error/timeout (which may leave out zero-value) never caches. diff --git a/internal/app/diagnose/scope.go b/internal/app/diagnose/scope.go new file mode 100644 index 0000000..b19974b --- /dev/null +++ b/internal/app/diagnose/scope.go @@ -0,0 +1,86 @@ +// Copyright 2026 opendbx contributors. See LICENSE. +// +// Author: sqlrush + +// File scope.go — allowed-tools scope filtering (spec-2.3 D-3). +// +// A successful ToolOutput carrying a non-nil ToolFilter REPLACES the +// run-local execution-tool scope: subsequent turns advertise only the +// filtered set, and a dispatch of a registered-but-filtered tool is +// answered with a recoverable SKILL.SCOPE_TOOL_DENIED tool_result (never +// terminal — the model self-corrects). Scope state lives entirely in +// Loop.Run locals; it dies with the Run (spec-2.3 R-6). +// +// "Skill" is implicitly retained in both the advertised set and the +// dispatch guard (spec-2.3 Q13, user decision 4/4): it is the +// scope-control verb — keeping it reachable means a Run can always +// switch scope, while execution tools stay filtered. This is a +// scope-control exception, NOT a permission bypass. + +package diagnose + +import "github.com/sqlrush/opendbx/internal/domain/llm" + +// skillToolName is the wire name of the scope-control tool (spec-2.3 +// Q3 CC parity). Kept as a local constant because diagnose cannot import +// app/skills/invoke (invoke imports diagnose); the integration suite +// asserts invoke.ToolName == this value to prevent drift. +const skillToolName = "Skill" + +// applyFilter narrows the advertised tool set to the active scope. +// nil filter → tools returned unchanged (same slice identity, keeping +// the no-skill path byte-stable for prompt caching). A non-nil filter +// returns a NEW slice (Rule 12 immutability — the shared backing array is never +// mutated) holding tools whose Name is in the filter, plus "Skill". +func applyFilter(tools []llm.ToolSchema, filter []string) []llm.ToolSchema { + if filter == nil { + return tools + } + allowed := make(map[string]bool, len(filter)+1) + for _, n := range filter { + allowed[n] = true + } + allowed[skillToolName] = true + out := make([]llm.ToolSchema, 0, len(tools)) + for _, ts := range tools { + if allowed[ts.Name] { + out = append(out, ts) + } + } + return out +} + +// normalizeFilter canonicalizes a ToolFilter before it becomes the +// active scope (spec-2.3 R2 pinned contract): case-sensitive dedupe +// preserving first-occurrence order; nil stays nil (inherit — no scope +// change); a non-nil empty slice stays non-nil (an empty execution +// scope: only "Skill" remains callable). Entries are already trimmed +// upstream (Schema.AllowedToolsList filters blanks). +func normalizeFilter(filter []string) []string { + if filter == nil { + return nil + } + seen := make(map[string]bool, len(filter)) + out := make([]string, 0, len(filter)) + for _, n := range filter { + if !seen[n] { + seen[n] = true + out = append(out, n) + } + } + return out +} + +// scopeDenied reports whether the active filter blocks a dispatch of +// name. nil filter denies nothing; "Skill" is never denied (Q13). +func scopeDenied(filter []string, name string) bool { + if filter == nil || name == skillToolName { + return false + } + for _, n := range filter { + if n == name { + return false + } + } + return true +} diff --git a/internal/app/diagnose/scope_test.go b/internal/app/diagnose/scope_test.go new file mode 100644 index 0000000..8103e80 --- /dev/null +++ b/internal/app/diagnose/scope_test.go @@ -0,0 +1,373 @@ +// Copyright 2026 opendbx contributors. See LICENSE. +// +// Author: sqlrush + +// File scope_test.go — spec-2.3 D-3 allowed-tools scope enforcement. +// +// User-mandated regression (approve note 2026-06-05): a filtered tool +// call must (a) never leave the UI stuck Running — its EventToolCall is +// always paired with an EventToolResult — and (b) keep the transcript +// paired-commit shape intact (every tool_use has a tool_result). + +package diagnose + +import ( + "context" + "strings" + "testing" + + "github.com/sqlrush/opendbx/internal/domain/llm" +) + +// scopeTool is a scripted ToolExecutor that returns a ToolFilter — +// stands in for the spec-2.3 SkillTool without importing app/skills +// (invoke imports diagnose; the reverse would cycle). +type scopeTool struct { + name string + filter []string + isErr bool +} + +func (s scopeTool) Name() string { return s.name } +func (s scopeTool) Schema() llm.ToolSchema { + return llm.ToolSchema{Name: s.name, InputSchema: map[string]any{"type": "object"}} +} +func (s scopeTool) Execute(_ context.Context, _ map[string]any) (ToolOutput, error) { + return ToolOutput{Content: "scope:" + s.name, IsError: s.isErr, ToolFilter: s.filter}, nil +} + +// capProv wraps stubProv and records every Request the Loop sends, so +// tests can assert the advertised (filtered) tool set per turn. +type capProv struct { + stubProv + reqs []llm.Request +} + +func (p *capProv) Stream(ctx context.Context, req llm.Request) (llm.Stream, error) { + p.reqs = append(p.reqs, req) + return p.stubProv.Stream(ctx, req) +} + +// toolNames extracts the advertised tool names of a captured request. +func toolNames(req llm.Request) []string { + out := make([]string, 0, len(req.Tools)) + for _, ts := range req.Tools { + out = append(out, ts.Name) + } + return out +} + +// toolUseChunk scripts a FinishToolUse turn with the given calls. +func toolUseChunk(uses ...llm.ToolUse) llm.Chunk { + return llm.Chunk{FinishReason: llm.FinishToolUse, ToolUses: uses} +} + +// stopChunk scripts a natural FinishStop turn. +func stopChunk() llm.Chunk { return llm.Chunk{Token: "done", FinishReason: llm.FinishStop} } + +// ============================================================ +// applyFilter / normalizeFilter units +// ============================================================ + +func TestApplyFilter_NilKeepsIdentity(t *testing.T) { + t.Parallel() + tools := []llm.ToolSchema{{Name: "clock"}, {Name: "echo"}} + got := applyFilter(tools, nil) + // Slice-header identity: same length/cap AND same backing array + // (element-0 address), guarded by the len check first (go LOW-3). + if len(got) != len(tools) || cap(got) != cap(tools) { + t.Fatalf("nil filter changed slice shape: len/cap %d/%d vs %d/%d", + len(got), cap(got), len(tools), cap(tools)) + } + if len(tools) > 0 && &got[0] != &tools[0] { + t.Error("nil filter must return the original slice (identity; prompt-cache stability)") + } +} + +func TestApplyFilter_NarrowsAndRetainsSkill(t *testing.T) { + t.Parallel() + tools := []llm.ToolSchema{{Name: "clock"}, {Name: "echo"}, {Name: skillToolName}} + got := applyFilter(tools, []string{"clock"}) + if len(got) != 2 || got[0].Name != "clock" || got[1].Name != skillToolName { + t.Errorf("applyFilter = %v; want [clock %s] (Skill implicitly retained, Q13)", got, skillToolName) + } + // Original slice untouched (Rule 12 immutability). + if len(tools) != 3 { + t.Error("applyFilter mutated its input") + } + // Non-nil empty filter → only Skill survives. + if got := applyFilter(tools, []string{}); len(got) != 1 || got[0].Name != skillToolName { + t.Errorf("empty filter = %v; want [%s] only", got, skillToolName) + } +} + +func TestNormalizeFilter(t *testing.T) { + t.Parallel() + if normalizeFilter(nil) != nil { + t.Error("nil must stay nil (inherit)") + } + got := normalizeFilter([]string{"b", "a", "b", "A"}) + want := []string{"b", "a", "A"} // case-sensitive dedupe, first occurrence order + if len(got) != len(want) { + t.Fatalf("normalizeFilter = %v; want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("normalizeFilter[%d] = %q; want %q", i, got[i], want[i]) + } + } + if e := normalizeFilter([]string{}); e == nil || len(e) != 0 { + t.Error("non-nil empty must stay non-nil empty (empty execution scope)") + } +} + +// ============================================================ +// Dispatch guard — the user-mandated regression +// ============================================================ + +// TestRun_ScopeDenied_PairedAndNotStuck — turn1 enters a scope allowing +// only clock; turn2 the model calls echo (registered but filtered). The +// denied call must emit BOTH EventToolCall and EventToolResult (UI never +// stuck Running), append an IsError tool_result to the transcript +// (paired commit intact), and the Run continues — not terminal. +func TestRun_ScopeDenied_PairedAndNotStuck(t *testing.T) { + t.Parallel() + reg, _ := NewRegistry(scopeTool{name: skillToolName, filter: []string{"clock"}}, EchoTool{}, ClockTool{}) + prov := &capProv{stubProv: stubProv{turns: []stubTurn{ + {chunks: []llm.Chunk{toolUseChunk(llm.ToolUse{ID: "c1", Name: skillToolName})}}, + {chunks: []llm.Chunk{toolUseChunk(llm.ToolUse{ID: "c2", Name: "echo"})}}, + {chunks: []llm.Chunk{stopChunk()}}, + }}} + r := &recorder{} + res, err := mustNewLoop(t, Options{Provider: prov, Registry: reg}). + Run(context.Background(), userReq("go"), r.emit) + if err != nil { + t.Fatalf("denied call must not terminate the Run: %v", err) + } + if res.FinishReason != llm.FinishStop || res.Turns != 3 { + t.Fatalf("Result = %+v; want FinishStop/3", res) + } + + // (a) UI pairing: every EventToolCall has a matching EventToolResult. + calls, results := 0, 0 + var deniedResult *llm.ToolResult + for _, e := range r.events { + switch e.Kind { + case EventToolCall: + calls++ + case EventToolResult: + results++ + if e.ToolResult.ToolUseID == "c2" { + deniedResult = e.ToolResult + } + } + } + if calls != 2 || results != 2 { + t.Errorf("calls/results = %d/%d; want 2/2 (denied tool must not be stuck Running)", calls, results) + } + if deniedResult == nil { + t.Fatal("no EventToolResult for the denied call c2") + } + if !deniedResult.IsError || !strings.Contains(deniedResult.Content, "SKILL.SCOPE_TOOL_DENIED") { + t.Errorf("denied result = %+v; want IsError + SCOPE_TOOL_DENIED template", deniedResult) + } + if !strings.Contains(deniedResult.Content, "Allowed: [clock]") || + !strings.Contains(deniedResult.Content, "(+ Skill)") { + t.Errorf("denied content must list the active scope: %q", deniedResult.Content) + } + + // (b) Transcript pairing: msgs = user, asst(c1), user(result c1), asst(c2), user(result c2). + if len(res.Messages) != 5 { + t.Fatalf("Messages = %d; want 5 (paired commit per turn)", len(res.Messages)) + } + denied := res.Messages[4].Content[0].ToolResult + if denied.ToolUseID != "c2" || !denied.IsError { + t.Errorf("transcript denied result = %+v; want c2/IsError", denied) + } + + // (c) Advertise narrowing: turn2/turn3 requests offer clock + Skill only. + for i := 1; i < 3; i++ { + names := toolNames(prov.reqs[i]) + if len(names) != 2 || names[0] != skillToolName && names[1] != skillToolName { + t.Errorf("turn %d advertised %v; want [clock %s] in registry order", i+1, names, skillToolName) + } + for _, n := range names { + if n != "clock" && n != skillToolName { + t.Errorf("turn %d advertised filtered-out tool %q", i+1, n) + } + } + } +} + +// TestRun_ScopeIntraTurnImmediate — one assistant turn emits +// [Skill, echo]; the scope set by Skill (executed first, Phase 2 serial) +// applies to echo IN THE SAME TURN (Q12 user decision: immediate effect). +func TestRun_ScopeIntraTurnImmediate(t *testing.T) { + t.Parallel() + reg, _ := NewRegistry(scopeTool{name: skillToolName, filter: []string{"clock"}}, EchoTool{}, ClockTool{}) + prov := &capProv{stubProv: stubProv{turns: []stubTurn{ + {chunks: []llm.Chunk{toolUseChunk( + llm.ToolUse{ID: "c1", Name: skillToolName}, + llm.ToolUse{ID: "c2", Name: "echo"}, + )}}, + {chunks: []llm.Chunk{stopChunk()}}, + }}} + r := &recorder{} + res, err := mustNewLoop(t, Options{Provider: prov, Registry: reg}). + Run(context.Background(), userReq("go"), r.emit) + if err != nil { + t.Fatalf("Run err: %v", err) + } + // Transcript: user, asst(c1+c2), user(result c1 + result c2). + if len(res.Messages) != 3 { + t.Fatalf("Messages = %d; want 3", len(res.Messages)) + } + resultsMsg := res.Messages[2] + if len(resultsMsg.Content) != 2 { + t.Fatalf("tool_result blocks = %d; want 2 (paired)", len(resultsMsg.Content)) + } + second := resultsMsg.Content[1].ToolResult + if second.ToolUseID != "c2" || !second.IsError || + !strings.Contains(second.Content, "SKILL.SCOPE_TOOL_DENIED") { + t.Errorf("same-turn echo after Skill must be denied (immediate effect, Q12): %+v", second) + } +} + +// TestRun_ScopeReplaceAndInherit — a nil ToolFilter inherits the current +// scope; a non-nil one REPLACES it. After replacing with a scope that +// allows echo, echo executes for real (and a previously denied call is +// never served from dedup — denied calls skip the cache entirely). +func TestRun_ScopeReplaceAndInherit(t *testing.T) { + t.Parallel() + reg, _ := NewRegistry( + scopeTool{name: skillToolName, filter: []string{"clock"}}, + scopeTool{name: "inherit", filter: nil}, // registered execution tool returning nil filter + scopeTool{name: "widen", filter: []string{"echo", "inherit"}}, + EchoTool{}, ClockTool{}, + ) + prov := &capProv{stubProv: stubProv{turns: []stubTurn{ + // turn1: enter scope {clock} — note: "inherit"/"widen" become filtered. + {chunks: []llm.Chunk{toolUseChunk(llm.ToolUse{ID: "c1", Name: skillToolName})}}, + // turn2: echo denied under {clock}. + {chunks: []llm.Chunk{toolUseChunk(llm.ToolUse{ID: "c2", Name: "echo"})}}, + // turn3: Skill itself is retained; its (scripted) filter replaces scope with {clock} again — + // then turn4 calls clock: allowed, and clock's nil ToolFilter must NOT clear scope. + {chunks: []llm.Chunk{toolUseChunk(llm.ToolUse{ID: "c3", Name: skillToolName})}}, + {chunks: []llm.Chunk{toolUseChunk(llm.ToolUse{ID: "c4", Name: "clock"})}}, + // turn5: echo still denied (nil filter from clock inherited the {clock} scope). + {chunks: []llm.Chunk{toolUseChunk(llm.ToolUse{ID: "c5", Name: "echo", Input: map[string]any{"k": "v"}})}}, + {chunks: []llm.Chunk{stopChunk()}}, + }}} + r := &recorder{} + res, err := mustNewLoop(t, Options{Provider: prov, Registry: reg}). + Run(context.Background(), userReq("go"), r.emit) + if err != nil { + t.Fatalf("Run err: %v", err) + } + if res.Turns != 6 { + t.Fatalf("Turns = %d; want 6", res.Turns) + } + // c4 (clock) executed for real under scope. + clockResult := res.Messages[8].Content[0].ToolResult + if clockResult.ToolUseID != "c4" || clockResult.IsError { + t.Errorf("clock under scope must execute: %+v", clockResult) + } + // c5 (echo) still denied — clock's nil ToolFilter did not clear the scope. + echoResult := res.Messages[10].Content[0].ToolResult + if echoResult.ToolUseID != "c5" || !echoResult.IsError || + !strings.Contains(echoResult.Content, "SKILL.SCOPE_TOOL_DENIED") { + t.Errorf("nil ToolFilter must inherit (not clear) the scope: %+v", echoResult) + } +} + +// TestRun_ScopeIsErrorNoUpdate — a ToolOutput carrying IsError must not +// update the scope even when ToolFilter is non-nil. +func TestRun_ScopeIsErrorNoUpdate(t *testing.T) { + t.Parallel() + reg, _ := NewRegistry( + scopeTool{name: skillToolName, filter: []string{"clock"}, isErr: true}, + EchoTool{}, ClockTool{}, + ) + prov := &capProv{stubProv: stubProv{turns: []stubTurn{ + {chunks: []llm.Chunk{toolUseChunk(llm.ToolUse{ID: "c1", Name: skillToolName})}}, + {chunks: []llm.Chunk{toolUseChunk(llm.ToolUse{ID: "c2", Name: "echo"})}}, + {chunks: []llm.Chunk{stopChunk()}}, + }}} + r := &recorder{} + res, err := mustNewLoop(t, Options{Provider: prov, Registry: reg}). + Run(context.Background(), userReq("go"), r.emit) + if err != nil { + t.Fatalf("Run err: %v", err) + } + echoResult := res.Messages[4].Content[0].ToolResult + if echoResult.IsError { + t.Errorf("IsError output must not enter scope; echo should run free: %+v", echoResult) + } + if names := toolNames(prov.reqs[1]); len(names) != 3 { + t.Errorf("turn2 advertised %v; want all 3 (no scope from IsError)", names) + } +} + +// TestRun_ScopeFromCachedHit — a dedup cached hit must replay its +// ToolFilter (the scope update is gated on success only, NEVER on +// !cached; spec-2.3 DoD negative assertion). +func TestRun_ScopeFromCachedHit(t *testing.T) { + t.Parallel() + reg, _ := NewRegistry( + scopeTool{name: skillToolName, filter: []string{"clock"}}, + scopeTool{name: "widen", filter: []string{"echo", "widen", "clock"}}, + EchoTool{}, ClockTool{}, + ) + prov := &capProv{stubProv: stubProv{turns: []stubTurn{ + // turn1: Skill{} → scope {clock} (stored in dedup under its input hash). + {chunks: []llm.Chunk{toolUseChunk(llm.ToolUse{ID: "c1", Name: skillToolName})}}, + // turn2: Skill is retained → widen is NOT in scope... use Skill replay instead: + // call Skill again with the SAME (empty) input → cached hit → scope re-applied. + {chunks: []llm.Chunk{toolUseChunk(llm.ToolUse{ID: "c2", Name: skillToolName})}}, + // turn3: echo still denied — the cached replay kept the {clock} scope live. + {chunks: []llm.Chunk{toolUseChunk(llm.ToolUse{ID: "c3", Name: "echo"})}}, + {chunks: []llm.Chunk{stopChunk()}}, + }}} + r := &recorder{} + res, err := mustNewLoop(t, Options{Provider: prov, Registry: reg, DedupEnabled: true}). + Run(context.Background(), userReq("go"), r.emit) + if err != nil { + t.Fatalf("Run err: %v", err) + } + // The c2 result must be the cached replay (Cached flag on the event). + var c2Cached bool + for _, e := range r.events { + if e.Kind == EventToolResult && e.ToolResult.ToolUseID == "c2" { + c2Cached = e.Cached + } + } + if !c2Cached { + t.Fatal("c2 expected to be a dedup cached hit (same name+input within window)") + } + echoResult := res.Messages[6].Content[0].ToolResult + if !echoResult.IsError || !strings.Contains(echoResult.Content, "SKILL.SCOPE_TOOL_DENIED") { + t.Errorf("cached hit must replay ToolFilter (no !cached gate): %+v", echoResult) + } +} + +// TestRun_UnknownToolStillTerminal — regression: the scope guard must not +// soften the pre-existing unknown-tool terminal (loop.go FinishToolUse +// validation; spec-1.21 D-4). +func TestRun_UnknownToolStillTerminal(t *testing.T) { + t.Parallel() + reg, _ := NewRegistry(scopeTool{name: skillToolName, filter: []string{"clock"}}, ClockTool{}) + prov := &capProv{stubProv: stubProv{turns: []stubTurn{ + {chunks: []llm.Chunk{toolUseChunk(llm.ToolUse{ID: "c1", Name: skillToolName})}}, + // "ghost" is not registered at all — terminal, even though a scope is active. + {chunks: []llm.Chunk{toolUseChunk(llm.ToolUse{ID: "c2", Name: "ghost"})}}, + }}} + r := &recorder{} + res, err := mustNewLoop(t, Options{Provider: prov, Registry: reg}). + Run(context.Background(), userReq("go"), r.emit) + if err == nil { + t.Fatal("unknown tool must stay terminal") + } + if res.TermCode != ErrToolUnknown.Code() { + t.Errorf("TermCode = %q; want %q", res.TermCode, ErrToolUnknown.Code()) + } +} diff --git a/internal/app/diagnose/tool.go b/internal/app/diagnose/tool.go index f64e490..71b0785 100644 --- a/internal/app/diagnose/tool.go +++ b/internal/app/diagnose/tool.go @@ -38,6 +38,14 @@ import ( type ToolOutput struct { Content string IsError bool + // ToolFilter, when non-nil, REPLACES the execution-tool scope for the + // remainder of the current Run (spec-2.3 allowed-tools enforcement). + // nil = no scope change (inherit current scope). "Skill" is implicitly + // retained by the Loop as the scope-control verb (spec-2.3 Q13) — it + // never needs to be listed. The field is orchestration-control only: + // classifyToolErr copies Content/IsError exclusively, so ToolFilter is + // NEVER serialized to the provider wire or the transcript. + ToolFilter []string } // ToolExecutor is implemented by every diagnose-loop-callable tool. diff --git a/internal/app/skills/imports_test.go b/internal/app/skills/imports_test.go new file mode 100644 index 0000000..cb35eee --- /dev/null +++ b/internal/app/skills/imports_test.go @@ -0,0 +1,92 @@ +// Copyright 2026 opendbx contributors. See LICENSE. +// +// Author: sqlrush + +// File imports_test.go — machine-enforced import allowlists (spec-2.3 +// D-8, hardening the spec-2.2 D-9 leaf invariant): +// +// - app/skills (this package) imports ONLY errcode + yaml + stdlib. +// It must NOT import diagnose/config/logger/render — and must NOT +// import its own invoke subpackage either (Go allows a parent +// importing its child; the leaf rule is directional and needs an +// explicit gate — spec-2.3 R2 arch MED-4). +// - app/skills/invoke imports ONLY skills + diagnose + llm + errcode +// + stdlib. + +package skills + +import ( + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" +) + +const modPrefix = "github.com/sqlrush/opendbx/" + +// nonStdImports returns the non-stdlib imports of every non-test .go +// file directly inside dir (no recursion). +func nonStdImports(t *testing.T, dir string) map[string][]string { + t.Helper() + entries, err := os.ReadDir(dir) + if err != nil { + t.Fatalf("ReadDir(%s): %v", dir, err) + } + out := map[string][]string{} + fset := token.NewFileSet() + for _, e := range entries { + name := e.Name() + if e.IsDir() || !strings.HasSuffix(name, ".go") || strings.HasSuffix(name, "_test.go") { + continue + } + f, err := parser.ParseFile(fset, filepath.Join(dir, name), nil, parser.ImportsOnly) + if err != nil { + t.Fatalf("parse %s: %v", name, err) + } + for _, imp := range f.Imports { + path := strings.Trim(imp.Path.Value, `"`) + if strings.Contains(path, ".") { // stdlib has no dot in the first segment + out[name] = append(out[name], path) + } + } + } + return out +} + +// assertAllowlist fails for any import outside the allowed set. +func assertAllowlist(t *testing.T, byFile map[string][]string, allowed map[string]bool, scope string) { + t.Helper() + for file, imps := range byFile { + for _, imp := range imps { + if !allowed[imp] { + t.Errorf("%s: %s imports %q — outside the %s allowlist", scope, file, imp, scope) + } + } + } +} + +// TestSkillsLeafImports — spec-2.2 D-9 invariant + spec-2.3 directional +// rule (the leaf never imports its invoke child). +func TestSkillsLeafImports(t *testing.T) { + t.Parallel() + allowed := map[string]bool{ + modPrefix + "internal/platform/errcode": true, + "go.yaml.in/yaml/v3": true, + } + assertAllowlist(t, nonStdImports(t, "."), allowed, "app/skills leaf") +} + +// TestInvokeImports — the invoke adapter may reach skills + diagnose + +// llm + errcode and nothing else (spec-2.3 D-8). +func TestInvokeImports(t *testing.T) { + t.Parallel() + allowed := map[string]bool{ + modPrefix + "internal/app/skills": true, + modPrefix + "internal/app/diagnose": true, + modPrefix + "internal/domain/llm": true, + modPrefix + "internal/platform/errcode": true, + } + assertAllowlist(t, nonStdImports(t, "invoke"), allowed, "app/skills/invoke") +} diff --git a/internal/app/skills/invoke/doc.go b/internal/app/skills/invoke/doc.go new file mode 100644 index 0000000..2f98fb8 --- /dev/null +++ b/internal/app/skills/invoke/doc.go @@ -0,0 +1,17 @@ +// Copyright 2026 opendbx contributors. See LICENSE. +// +// Author: sqlrush + +// Package invoke adapts discovered skills (spec-2.2) to the diagnose +// loop tool surface (spec-1.21). Design: spec-2.3-skill-invocation. +// +// It deliberately imports BOTH app/skills and app/diagnose — the skills +// core package stays a pure leaf (spec-2.2 D-9 invariant: app/skills +// must not import diagnose/config/logger/render, and must not import +// this subpackage either; spec-2.3 D-8 directional rule). +// +// Naming note (spec-2.3 Q3 / R-8): the tool's wire name is "Skill" +// (capitalized) for 1:1 Claude Code parity — a deliberate divergence +// from the lowercase clock/echo convention, which only applies to +// opendbx-native tools. +package invoke diff --git a/internal/app/skills/invoke/errors.go b/internal/app/skills/invoke/errors.go new file mode 100644 index 0000000..5a03ac3 --- /dev/null +++ b/internal/app/skills/invoke/errors.go @@ -0,0 +1,59 @@ +// Copyright 2026 opendbx contributors. See LICENSE. +// +// Author: sqlrush + +// File errors.go — SKILL.* invoke-time errcode sentinels (spec-2.3 D-6; +// Rule 7 Code/Message/Hint triple). Both codes are FEEDBACK class: their composed templates +// are written into ToolOutput.Content with IsError=true so the model +// self-corrects on the next turn — they never terminate the Run. +// +// The third spec-2.3 code, SKILL.SCOPE_TOOL_DENIED, is registered in +// app/diagnose/errors.go: its text is composed by the Loop dispatch +// guard (loop.go), and diagnose cannot import this package (invoke → +// diagnose would become a cycle). Same placement precedent as +// DIAGNOSE.TOOL_TIMEOUT (feedback text composed by the Loop). + +package invoke + +import ( + "fmt" + "strings" + + "github.com/sqlrush/opendbx/internal/platform/errcode" +) + +//nolint:gochecknoglobals // spec-0.6 contract: errcode sentinels are package-level. +var ( + // ErrNotFound — the requested skill name is not in the active set. + // Feedback class (recoverable; the composed Content lists the active + // skills so the model can self-correct). + ErrNotFound = errcode.Register( + "SKILL.NOT_FOUND", + "no active skill with the requested name", + "check /debug skills for the active list", + ) + // ErrInvokeInvalid — the Skill tool input shape is invalid (missing / + // non-string "skill", or non-object "args"). Feedback class. + ErrInvokeInvalid = errcode.Register( + "SKILL.INVOKE_INVALID", + "Skill tool input is invalid", + `call Skill with {"skill": ""} and optional object "args"`, + ) +) + +// notFoundContent composes the recoverable ToolOutput.Content for an +// unknown skill name (spec-2.3 D-6 template). names must be sorted for +// deterministic output (SkillTool guarantees this). +func notFoundContent(name string, names []string) string { + return fmt.Sprintf("%s: no active skill named %q. Available skills: [%s]. Hint: %s.", + ErrNotFound.Code(), name, strings.Join(names, ", "), ErrNotFound.Hint()) +} + +// invokeInvalidContent composes the recoverable ToolOutput.Content for a +// malformed Skill tool input (spec-2.3 D-6 template). detail is one of +// the fixed-detail variants pinned by the spec (missing/non-string skill, +// non-object args). +func invokeInvalidContent(detail string) string { + return fmt.Sprintf("%s: %s. Hint: %s.", + ErrInvokeInvalid.Code(), detail, ErrInvokeInvalid.Hint()) +} diff --git a/internal/app/skills/invoke/errors_test.go b/internal/app/skills/invoke/errors_test.go new file mode 100644 index 0000000..24e4c57 --- /dev/null +++ b/internal/app/skills/invoke/errors_test.go @@ -0,0 +1,59 @@ +// Copyright 2026 opendbx contributors. See LICENSE. +// +// Author: sqlrush + +package invoke + +import ( + "strings" + "testing" + + "github.com/sqlrush/opendbx/internal/platform/errcode" +) + +// TestErrorsRegistered — both invoke-side sentinels carry the Rule 7 +// three-piece contract and the SKILL. prefix (spec-2.3 D-6). +func TestErrorsRegistered(t *testing.T) { + t.Parallel() + for _, s := range []errcode.Sentinel{ErrNotFound, ErrInvokeInvalid} { + if !strings.HasPrefix(s.Code(), "SKILL.") { + t.Errorf("code %q lacks SKILL. prefix", s.Code()) + } + if s.Message() == "" || s.Hint() == "" { + t.Errorf("%s missing message/hint (Rule 7 triple)", s.Code()) + } + } +} + +// TestNotFoundContent — template carries code, requested name, the sorted +// active list, and an actionable hint (spec-2.3 D-6 template). +func TestNotFoundContent(t *testing.T) { + t.Parallel() + got := notFoundContent("nope", []string{"alpha", "beta"}) + for _, want := range []string{ + "SKILL.NOT_FOUND", + `"nope"`, + "Available skills: [alpha, beta]", + "Hint: check /debug skills", + } { + if !strings.Contains(got, want) { + t.Errorf("notFoundContent missing %q in %q", want, got) + } + } +} + +// TestInvokeInvalidContent — template carries code, the detail variant, +// and the call-shape hint (spec-2.3 D-6 template). +func TestInvokeInvalidContent(t *testing.T) { + t.Parallel() + got := invokeInvalidContent(`missing or non-string "skill" field`) + for _, want := range []string{ + "SKILL.INVOKE_INVALID", + `missing or non-string "skill" field`, + `{"skill": ""}`, + } { + if !strings.Contains(got, want) { + t.Errorf("invokeInvalidContent missing %q in %q", want, got) + } + } +} diff --git a/internal/app/skills/invoke/skilltool.go b/internal/app/skills/invoke/skilltool.go new file mode 100644 index 0000000..724af87 --- /dev/null +++ b/internal/app/skills/invoke/skilltool.go @@ -0,0 +1,198 @@ +// Copyright 2026 opendbx contributors. See LICENSE. +// +// Author: sqlrush + +// File skilltool.go — SkillTool, the diagnose.ToolExecutor that lets the +// model enter a skill scope (spec-2.3 D-1). +// +// CC baseline: src/tools/SkillTool/ (survey skills.md § 1.5) — input +// {skill, args}; the skill body enters the model context and +// allowed-tools is enforced for the scope. Two deliberate deviations, +// both pinned by spec-2.3: +// - body is returned as the tool RESULT (tool_result block), not +// injected into the system prompt (Q11, user-approved divergence +// from the survey § 2.3 wording; revisit on T-11 verify). +// - "Skill" itself is implicitly retained by the Loop scope filter +// (Q13 meta-control router), so allowed-tools never needs to list it. +// +// Failure two-track (spec-1.21 tool.go contract): +// - semantic failures (unknown skill, bad input shape) → recoverable +// ToolOutput{IsError: true} — the model self-corrects next turn. +// - infrastructure failures (ctx cancel / deadline) → Go error, fatal; +// the Loop's classifyToolErr does the cancel-vs-timeout dispatch. + +package invoke + +import ( + "context" + "encoding/json" + "fmt" + "sort" + "strings" + + "github.com/sqlrush/opendbx/internal/app/diagnose" + "github.com/sqlrush/opendbx/internal/app/skills" + "github.com/sqlrush/opendbx/internal/domain/llm" + "github.com/sqlrush/opendbx/internal/platform/errcode" +) + +// ToolName is the wire-visible tool identifier — capitalized for 1:1 CC +// parity (spec-2.3 Q3; divergence from lowercase clock/echo is deliberate). +const ToolName = "Skill" + +// modelNoticeFmt is appended to the invoke Content when the skill's +// frontmatter requests a model override (spec-2.3 Q7: v1 never switches +// models; visible notice instead of silent ignore, Rule 7). +const modelNoticeFmt = "\n\nNote: this skill requests model %q; per-skill model switching lands in spec-3.11." + +// SkillTool maps the validated active skill set (spec-2.2 +// DiscoveryResult.Active) onto the diagnose loop. Immutable after +// construction: Execute never mutates internal state, so the process-wide +// registry sharing across Runs is safe (spec-2.3 R-6 — scope state lives +// in Loop.Run locals, never here). +type SkillTool struct { + byKey map[string]skills.Skill // Key() → Skill (case-sensitive, 2.1 D-2) + names []string // sorted; deterministic listings +} + +// NewSkillTool builds the tool from the active skill set. It defensively +// asserts the spec-2.2 Active membership contract (unique Key per skill); +// a violation is an upstream bug and returns an error — the bootstrap +// caller LOGS and SKIPS registration, it never panics (bad skills must +// not block interact; spec-2.3 user decision 4/4 2026-06-05). +func NewSkillTool(active []skills.Skill) (*SkillTool, error) { + if len(active) == 0 { + return nil, errcode.Newf(ErrInvokeInvalid.Code(), + "NewSkillTool requires a non-empty active skill set; skip construction when discovery yields none") + } + byKey := make(map[string]skills.Skill, len(active)) + names := make([]string, 0, len(active)) + for _, sk := range active { + key := sk.Key() + if _, dup := byKey[key]; dup { + // Sentinel reference (not a string literal) so a code rename + // breaks the build here, not at first runtime call (go MED-2). + return nil, errcode.Newf(skills.ErrNamespaceConflict.Code(), + "duplicate active skill key %q passed to NewSkillTool (spec-2.2 Active membership contract violated)", key) + } + byKey[key] = sk + names = append(names, key) + } + sort.Strings(names) + return &SkillTool{byKey: byKey, names: names}, nil +} + +// Name implements diagnose.ToolExecutor. +func (t *SkillTool) Name() string { return ToolName } + +// Schema implements diagnose.ToolExecutor. Input mirrors the CC SkillTool +// contract {skill, args} (survey § 1.5; line-level provisional, T-11 #3). +// Unknown extra input keys are ignored at Execute (tolerant decode). +func (t *SkillTool) Schema() llm.ToolSchema { + return llm.ToolSchema{ + Name: ToolName, + Description: "Invoke an installed skill by exact name. The skill's instructions are " + + "returned for you to follow. Use when an available skill (listed in the system " + + "prompt) matches the current task; do not guess names not on that list.", + InputSchema: map[string]any{ + "type": "object", + "properties": map[string]any{ + "skill": map[string]any{ + "type": "string", + "description": "Exact name of an available skill (case-sensitive).", + }, + "args": map[string]any{ + "type": "object", + "description": "Optional arguments for the skill.", + "additionalProperties": true, + }, + }, + "required": []string{"skill"}, + }, + } +} + +// Execute implements diagnose.ToolExecutor (spec-2.3 D-1 steps 1-6). +func (t *SkillTool) Execute(ctx context.Context, input map[string]any) (diagnose.ToolOutput, error) { + // errcode-lint:exempt -- spec-1.21 D-4 two-track: ctx errors pass through unchanged; the Loop classifies cancel-vs-timeout via errors.Is. + if err := ctx.Err(); err != nil { + return diagnose.ToolOutput{}, err + } + name, ok := input["skill"].(string) + if !ok || strings.TrimSpace(name) == "" { + return diagnose.ToolOutput{ + Content: invokeInvalidContent(`missing or non-string "skill" field`), + IsError: true, + }, nil + } + argsJSON, argsErr := canonicalArgs(input) + if argsErr != "" { + return diagnose.ToolOutput{Content: invokeInvalidContent(argsErr), IsError: true}, nil + } + sk, found := t.byKey[name] // case-sensitive (2.1 D-2: Key is byte-exact) + if !found { + return diagnose.ToolOutput{Content: notFoundContent(name, t.names), IsError: true}, nil + } + + var b strings.Builder + b.WriteString(sk.Body) // byte-verbatim (2.1 body fidelity; golden-tested) + if argsJSON != "" { + b.WriteString("\n\n## Arguments\n```json\n") + b.WriteString(argsJSON) + b.WriteString("\n```") + } + if m := sk.Schema.Model; m != "" { + fmt.Fprintf(&b, modelNoticeFmt, m) + } + return diagnose.ToolOutput{ + Content: b.String(), + ToolFilter: sk.Schema.AllowedToolsList(), // nil = inherit scope (2.1: ""→nil) + }, nil +} + +// canonicalArgs validates and renders the optional "args" input +// (spec-2.3 D-1 step 3 five-shape table): +// +// absent → ("", "") — no Arguments section +// object → (canonical key-sorted JSON, "") +// null / string / list / number / bool → ("", detail) — recoverable +// +// json.Marshal sorts map keys (stdlib), and DecodeToolInput delivers +// numbers as json.Number, so the rendering is deterministic across turns +// (this also keeps the spec-1.22 dedup key stable for identical args). +func canonicalArgs(input map[string]any) (argsJSON, errDetail string) { + v, present := input["args"] + if !present { + return "", "" + } + obj, ok := v.(map[string]any) + if !ok { + return "", fmt.Sprintf(`"args" must be a JSON object, got %s`, jsonTypeName(v)) + } + raw, err := json.Marshal(obj) + if err != nil { + // Unreachable under the DecodeToolInput invariant (JSON-decodable + // values only); reported recoverably rather than swallowed. + return "", `"args" could not be re-encoded as JSON: ` + err.Error() + } + return string(raw), "" +} + +// jsonTypeName names a decoded JSON value's type for error messages +// (model-facing wording, hence JSON terms — not Go type syntax). +func jsonTypeName(v any) string { + switch v.(type) { + case nil: + return "null" + case string: + return "string" + case []any: + return "array" + case bool: + return "boolean" + case json.Number, float64, int, int64: + return "number" + default: + return fmt.Sprintf("%T", v) + } +} diff --git a/internal/app/skills/invoke/skilltool_test.go b/internal/app/skills/invoke/skilltool_test.go new file mode 100644 index 0000000..def8482 --- /dev/null +++ b/internal/app/skills/invoke/skilltool_test.go @@ -0,0 +1,229 @@ +// Copyright 2026 opendbx contributors. See LICENSE. +// +// Author: sqlrush + +package invoke + +import ( + "context" + "errors" + "os" + "reflect" + "strings" + "testing" + + "github.com/sqlrush/opendbx/internal/app/skills" + "github.com/sqlrush/opendbx/internal/platform/errcode" +) + +// mkSkill builds a minimal valid Skill for invoke tests. +func mkSkill(name, body string) skills.Skill { + return skills.Skill{ + Schema: skills.Schema{Name: name, Description: "Test skill."}, + Body: body, + } +} + +// loadGolden parses the CC golden skill (spec-2.1 D-7 corpus) so the +// invoke path is exercised against a byte-faithful real skill. +func loadGolden(t *testing.T) skills.Skill { + t.Helper() + raw, err := os.ReadFile("../testdata/code-reviewer.md") + if err != nil { + t.Fatalf("read golden: %v", err) + } + sk, err := skills.Parse(raw, skills.SkillSource{Kind: skills.SourceProject, Precedence: 400}) + if err != nil { + t.Fatalf("parse golden: %v", err) + } + return sk +} + +// TestNewSkillTool_Validation — empty input and duplicate Key() are +// upstream contract violations and must surface as errors (the bootstrap +// caller logs + skips; spec-2.3 user decision 4/4 — never panic). +func TestNewSkillTool_Validation(t *testing.T) { + t.Parallel() + if _, err := NewSkillTool(nil); err == nil { + t.Error("NewSkillTool(nil) = nil error, want error") + } + _, err := NewSkillTool([]skills.Skill{mkSkill("dup", "a"), mkSkill("dup", "b")}) + if err == nil { + t.Fatal("NewSkillTool(dup keys) = nil error, want error") + } + var ec errcode.Error + if !errors.As(err, &ec) || ec.Code() != "SKILL.NAMESPACE_CONFLICT" { + t.Errorf("dup-key error code = %v, want SKILL.NAMESPACE_CONFLICT", err) + } +} + +// TestSkillTool_NameAndSchema — wire name is the CC-parity "Skill" and +// the schema requires the skill field (spec-2.3 Q3 + D-1). +func TestSkillTool_NameAndSchema(t *testing.T) { + t.Parallel() + st, err := NewSkillTool([]skills.Skill{mkSkill("alpha", "body")}) + if err != nil { + t.Fatal(err) + } + if st.Name() != "Skill" { + t.Errorf("Name() = %q, want Skill", st.Name()) + } + sch := st.Schema() + if sch.Name != st.Name() { + t.Errorf("Schema().Name = %q must equal Name() (registry invariant)", sch.Name) + } + req, _ := sch.InputSchema["required"].([]string) + if len(req) != 1 || req[0] != "skill" { + t.Errorf("required = %v, want [skill]", req) + } +} + +// TestExecute_CtxFatal — a cancelled context is an infrastructure +// failure: Go error return, NOT a recoverable IsError (spec-2.3 R2 +// two-track; FROZEN tool.go contract). +func TestExecute_CtxFatal(t *testing.T) { + t.Parallel() + st, _ := NewSkillTool([]skills.Skill{mkSkill("alpha", "body")}) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + out, err := st.Execute(ctx, map[string]any{"skill": "alpha"}) + if !errors.Is(err, context.Canceled) { + t.Fatalf("Execute(cancelled ctx) err = %v, want context.Canceled", err) + } + if out.IsError || out.Content != "" { + t.Errorf("cancelled ctx must not produce a recoverable output, got %+v", out) + } +} + +// TestExecute_InputValidation — semantic input failures are recoverable +// IsError with the INVOKE_INVALID / NOT_FOUND templates; the args field +// is validated across all five JSON shapes (spec-2.3 D-1 step 3). +func TestExecute_InputValidation(t *testing.T) { + t.Parallel() + st, _ := NewSkillTool([]skills.Skill{mkSkill("alpha", "body"), mkSkill("Beta", "b2")}) + tests := []struct { + name string + input map[string]any + wantPart string + }{ + {"missing skill", map[string]any{}, "SKILL.INVOKE_INVALID"}, + {"non-string skill", map[string]any{"skill": 42}, "SKILL.INVOKE_INVALID"}, + {"blank skill", map[string]any{"skill": " "}, "SKILL.INVOKE_INVALID"}, + {"unknown skill", map[string]any{"skill": "nope"}, `SKILL.NOT_FOUND: no active skill named "nope". Available skills: [Beta, alpha]`}, + {"case mismatch is unknown", map[string]any{"skill": "beta"}, "SKILL.NOT_FOUND"}, + {"args null", map[string]any{"skill": "alpha", "args": nil}, `"args" must be a JSON object, got null`}, + {"args string", map[string]any{"skill": "alpha", "args": "s"}, `"args" must be a JSON object, got string`}, + {"args array", map[string]any{"skill": "alpha", "args": []any{1}}, `"args" must be a JSON object, got array`}, + {"args number", map[string]any{"skill": "alpha", "args": float64(3)}, `"args" must be a JSON object, got number`}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + out, err := st.Execute(context.Background(), tc.input) + if err != nil { + t.Fatalf("semantic failure must not return Go error, got %v", err) + } + if !out.IsError { + t.Fatalf("want IsError=true, got %+v", out) + } + if !strings.Contains(out.Content, tc.wantPart) { + t.Errorf("Content %q missing %q", out.Content, tc.wantPart) + } + if out.ToolFilter != nil { + t.Errorf("failure output must not carry ToolFilter, got %v", out.ToolFilter) + } + }) + } +} + +// TestExecute_GoldenBody — invoking the CC golden skill returns the body +// byte-verbatim and passes allowed-tools through as the ToolFilter +// (spec-2.3 D-1; body fidelity inherited from the 2.1 golden contract). +func TestExecute_GoldenBody(t *testing.T) { + t.Parallel() + golden := loadGolden(t) + st, err := NewSkillTool([]skills.Skill{golden}) + if err != nil { + t.Fatal(err) + } + out, err := st.Execute(context.Background(), map[string]any{"skill": "code-reviewer"}) + if err != nil || out.IsError { + t.Fatalf("golden invoke failed: err=%v out=%+v", err, out) + } + // Body byte-verbatim, then the model notice (golden has model: sonnet). + if !strings.HasPrefix(out.Content, golden.Body) { + t.Errorf("Content does not start with body bytes:\n%q", out.Content) + } + if !strings.Contains(out.Content, `requests model "sonnet"`) { + t.Errorf("missing model notice (Q7):\n%q", out.Content) + } + wantFilter := []string{"Read", "Grep", "Glob", "Bash"} + if !reflect.DeepEqual(out.ToolFilter, wantFilter) { + t.Errorf("ToolFilter = %v, want %v", out.ToolFilter, wantFilter) + } +} + +// TestExecute_ArgsRendering — present-object args render as a canonical +// key-sorted JSON section after the body; absent args add no section. +func TestExecute_ArgsRendering(t *testing.T) { + t.Parallel() + st, _ := NewSkillTool([]skills.Skill{mkSkill("alpha", "BODY")}) + + out, err := st.Execute(context.Background(), map[string]any{ + "skill": "alpha", + "args": map[string]any{"zeta": "1", "alpha": "2"}, + }) + if err != nil || out.IsError { + t.Fatalf("invoke failed: %v %+v", err, out) + } + want := "BODY\n\n## Arguments\n```json\n{\"alpha\":\"2\",\"zeta\":\"1\"}\n```" + if out.Content != want { + t.Errorf("Content = %q, want %q (canonical key-sorted)", out.Content, want) + } + + out2, _ := st.Execute(context.Background(), map[string]any{"skill": "alpha"}) + if out2.Content != "BODY" { + t.Errorf("absent args must add no section, got %q", out2.Content) + } + // Empty object is present → renders an empty Arguments section. + out3, _ := st.Execute(context.Background(), map[string]any{"skill": "alpha", "args": map[string]any{}}) + if !strings.Contains(out3.Content, "## Arguments\n```json\n{}\n```") { + t.Errorf("empty-object args should render {}, got %q", out3.Content) + } +} + +// TestExecute_NoFilterWhenNoAllowedTools — a skill without allowed-tools +// yields ToolFilter nil (inherit scope; 2.1 ""→nil contract). +func TestExecute_NoFilterWhenNoAllowedTools(t *testing.T) { + t.Parallel() + st, _ := NewSkillTool([]skills.Skill{mkSkill("alpha", "b")}) + out, err := st.Execute(context.Background(), map[string]any{"skill": "alpha"}) + if err != nil || out.IsError { + t.Fatalf("invoke failed: %v %+v", err, out) + } + if out.ToolFilter != nil { + t.Errorf("ToolFilter = %v, want nil (inherit)", out.ToolFilter) + } +} + +// TestSkillTool_Immutable — Execute never mutates the tool's internal +// state (Rule 12 / spec-2.3 R-6: process-wide registry sharing). +func TestSkillTool_Immutable(t *testing.T) { + t.Parallel() + active := []skills.Skill{loadGolden(t), mkSkill("alpha", "b")} + st, _ := NewSkillTool(active) + namesBefore := append([]string{}, st.names...) + for _, in := range []map[string]any{ + {"skill": "alpha"}, + {"skill": "nope"}, + {}, + {"skill": "code-reviewer", "args": map[string]any{"k": "v"}}, + } { + if _, err := st.Execute(context.Background(), in); err != nil { + t.Fatal(err) + } + } + if !reflect.DeepEqual(namesBefore, st.names) || len(st.byKey) != 2 { + t.Error("SkillTool internal state mutated by Execute") + } +} diff --git a/internal/app/skills/prompt.go b/internal/app/skills/prompt.go new file mode 100644 index 0000000..c5319e8 --- /dev/null +++ b/internal/app/skills/prompt.go @@ -0,0 +1,78 @@ +// Copyright 2026 opendbx contributors. See LICENSE. +// +// Author: sqlrush + +// File prompt.go — system-prompt "available skills" section (spec-2.3 +// D-2). Pure, stdlib-only — this file keeps the app/skills leaf +// invariant (spec-2.2 D-9: no diagnose/config/logger/render imports; the +// invoke subpackage is likewise off-limits, spec-2.3 D-8 directional +// rule). Output format is provisional vs CC (survey § 7 T-11) and is +// errata-aligned after T-11 verify. + +package skills + +import ( + "sort" + "strings" +) + +// promptHeader precedes the per-skill list. The wording tells the model +// HOW to act on the list (invoke via the Skill tool) — the per-skill +// "when to use" signal lives in each description (CLAUDE.md § 3.3). +const promptHeader = "## Available skills\n\n" + + "Invoke a skill with the Skill tool when its description matches the task.\n" + +// PromptSection renders the system-prompt block advertising the active +// skill set (spec-2.3 D-2). Deterministic: skills are listed name-sorted +// and each description is reduced to its first non-empty line (CRLF +// normalized, TrimSpace; rule pinned by spec-2.3 R2). Returns "" for an +// empty input — the caller skips injection entirely (Q6). +// +// v1 discovery runs once at startup, so the returned bytes are stable +// for the whole session (prompt-cache friendly; spec-2.3 Q10 — note the +// system prompt transitions empty→non-empty when skills are present). +func PromptSection(active []Skill) string { + if len(active) == 0 { + return "" + } + names := make([]string, 0, len(active)) + byName := make(map[string]Skill, len(active)) + for _, sk := range active { + key := sk.Key() + if _, dup := byName[key]; dup { + continue // defensive: Active guarantees unique keys (2.2); + // first occurrence wins so a contract break upstream cannot + // produce duplicate listing lines (post-impl cr NIT-1) + } + names = append(names, key) + byName[key] = sk + } + sort.Strings(names) + + var b strings.Builder + b.WriteString(promptHeader) + for _, name := range names { + b.WriteString("- ") + b.WriteString(name) + b.WriteString(": ") + b.WriteString(firstNonEmptyLine(byName[name].Schema.Description)) + b.WriteString("\n") + } + return b.String() +} + +// firstNonEmptyLine returns the first line of s whose TrimSpace is +// non-empty, itself TrimSpace'd. CRLF is normalized to LF before +// splitting so the pick is byte-stable across line-ending styles +// (spec-2.3 R2 pinned extraction rule). Returns "" when every line is +// blank — Validate guarantees a non-blank description for active skills, +// so that case is defensive only. +func firstNonEmptyLine(s string) string { + s = strings.ReplaceAll(s, "\r\n", "\n") + for _, line := range strings.Split(s, "\n") { + if t := strings.TrimSpace(line); t != "" { + return t + } + } + return "" +} diff --git a/internal/app/skills/prompt_test.go b/internal/app/skills/prompt_test.go new file mode 100644 index 0000000..94d0f1e --- /dev/null +++ b/internal/app/skills/prompt_test.go @@ -0,0 +1,84 @@ +// Copyright 2026 opendbx contributors. See LICENSE. +// +// Author: sqlrush + +package skills + +import ( + "strings" + "testing" +) + +// mkSkill builds a minimal valid Skill for prompt tests. +func mkSkill(name, desc string) Skill { + return Skill{Schema: Schema{Name: name, Description: desc}} +} + +// TestPromptSection_Empty — empty active set renders nothing (the caller +// skips injection entirely; spec-2.3 Q6). +func TestPromptSection_Empty(t *testing.T) { + t.Parallel() + if got := PromptSection(nil); got != "" { + t.Errorf("PromptSection(nil) = %q, want empty", got) + } + if got := PromptSection([]Skill{}); got != "" { + t.Errorf("PromptSection(empty) = %q, want empty", got) + } +} + +// TestPromptSection_SortedDeterministic — listing is name-sorted +// regardless of input order, and byte-stable across calls (spec-2.3 D-2). +func TestPromptSection_SortedDeterministic(t *testing.T) { + t.Parallel() + a := []Skill{mkSkill("zeta", "Z skill."), mkSkill("alpha", "A skill.")} + b := []Skill{mkSkill("alpha", "A skill."), mkSkill("zeta", "Z skill.")} + ga, gb := PromptSection(a), PromptSection(b) + if ga != gb { + t.Fatalf("PromptSection not order-independent:\n%q\nvs\n%q", ga, gb) + } + ia, iz := strings.Index(ga, "- alpha:"), strings.Index(ga, "- zeta:") + if ia < 0 || iz < 0 || ia > iz { + t.Errorf("expected name-sorted listing, got:\n%s", ga) + } + if !strings.Contains(ga, "## Available skills") || !strings.Contains(ga, "Skill tool") { + t.Errorf("missing header/instruction:\n%s", ga) + } +} + +// TestPromptSection_FirstLineExtraction — description reduces to the +// first non-empty line; CRLF normalized; surrounding space trimmed +// (spec-2.3 R2 pinned rule). +func TestPromptSection_FirstLineExtraction(t *testing.T) { + t.Parallel() + tests := []struct { + name string + desc string + want string + }{ + {"single line", "Reviews code.", "Reviews code."}, + {"multi line", "Reviews code.\nSecond line ignored.", "Reviews code."}, + {"crlf", "Reviews code.\r\nIgnored.", "Reviews code."}, + {"leading blank lines", "\n \nActual line.\nIgnored.", "Actual line."}, + {"surrounding spaces", " padded line \nIgnored.", "padded line"}, + {"block scalar trailing newline", "Expert reviewer.\n", "Expert reviewer."}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + got := PromptSection([]Skill{mkSkill("s", tc.desc)}) + want := "- s: " + tc.want + "\n" + if !strings.Contains(got, want) { + t.Errorf("desc %q: section %q missing line %q", tc.desc, got, want) + } + }) + } +} + +// TestFirstNonEmptyLine_AllBlank — defensive: all-blank description +// yields "" (Validate prevents this for active skills). +func TestFirstNonEmptyLine_AllBlank(t *testing.T) { + t.Parallel() + if got := firstNonEmptyLine(" \n\t\n"); got != "" { + t.Errorf("firstNonEmptyLine(all blank) = %q, want empty", got) + } +} diff --git a/internal/bootstrap/skills.go b/internal/bootstrap/skills.go index d64ec81..d45483c 100644 --- a/internal/bootstrap/skills.go +++ b/internal/bootstrap/skills.go @@ -18,8 +18,11 @@ import ( "path/filepath" "sort" + "github.com/sqlrush/opendbx/internal/app/diagnose" "github.com/sqlrush/opendbx/internal/app/skills" + "github.com/sqlrush/opendbx/internal/app/skills/invoke" "github.com/sqlrush/opendbx/internal/platform/config" + "github.com/sqlrush/opendbx/internal/platform/logger" ) // PluginDir is one installed plugin's skills directory + its stable ID. @@ -130,3 +133,62 @@ func DiscoverSkills(cfg *config.Config) skills.DiscoveryResult { func DiscoverSkillsSummary(cfg *config.Config) string { return skills.SummarizeDiscovery(DiscoverSkills(cfg)) } + +// bodySizeWarnBytes is the soft warning threshold for a skill body +// injected into the transcript (spec-2.3 R-7: a huge body eats the +// context budget on a single invoke; the hard cutoff is spec-3.10). +const bodySizeWarnBytes = 64 << 10 + +// skillsForChat adapts a discovery result for the chat model (spec-2.3 +// D-5): the SkillTool executor plus the system-prompt skills section. +// +// Failure model (user decision 4/4, 2026-06-05): a NewSkillTool error — +// an upstream spec-2.2 contract violation — is LOGGED and skills are +// skipped for the session; interact always continues. Never panics. +// 0 active skills → no executor, no prompt section (spec-2.3 Q6). +func skillsForChat(res skills.DiscoveryResult) (execs []diagnose.ToolExecutor, systemPrompt string) { + logSkillDiscovery(res) + if len(res.Active) == 0 { + return nil, "" + } + st, err := invoke.NewSkillTool(res.Active) + if err != nil { + logger.WarnForceFile( + "skill tool construction failed; skills are disabled for this session", + "spec", "2.3", "deliverable", "D-5", "err", err.Error(), + ) + return nil, "" + } + section := skills.PromptSection(res.Active) + logger.InfoForceFile( // normal path — info band, not warn (dual-route post-impl finding) + "skills active for this session", + "spec", "2.3", "active", len(res.Active), "prompt_section_bytes", len(section), + ) + return []diagnose.ToolExecutor{st}, section +} + +// logSkillDiscovery surfaces discovery problems and per-skill trust +// warnings in the debug log (Rule 7 — never silent; file-only so the +// TUI cell grid is not torn). Covers spec-2.3 R-10 (plugin-cache +// provenance) and R-7/R-11 (oversized body) at startup — v1 discovery +// runs once, so bodies and sources are static for the session. +func logSkillDiscovery(res skills.DiscoveryResult) { + if n := len(res.Errors); n > 0 { + logger.WarnForceFile("skill discovery reported errors (run /debug skills)", + "spec", "2.3", "errors", n) + } + if n := len(res.Warnings); n > 0 { + logger.WarnForceFile("skill discovery reported warnings (run /debug skills)", + "spec", "2.3", "warnings", n) + } + for _, sk := range res.Active { + if sk.Source.Kind == skills.SourcePluginCache { + logger.WarnForceFile("active skill comes from the plugin cache; its body enters the model context verbatim", + "spec", "2.3", "risk", "R-10", "skill", sk.Key(), "plugin", sk.Source.PluginID, "path", sk.Source.Path) + } + if len(sk.Body) > bodySizeWarnBytes { + logger.WarnForceFile("active skill body exceeds the 64KiB soft cap; one invoke will consume significant context", + "spec", "2.3", "risk", "R-7", "skill", sk.Key(), "body_bytes", len(sk.Body)) + } + } +} diff --git a/internal/bootstrap/skills_test.go b/internal/bootstrap/skills_test.go index 91da467..7098413 100644 --- a/internal/bootstrap/skills_test.go +++ b/internal/bootstrap/skills_test.go @@ -6,6 +6,7 @@ package bootstrap import ( "path/filepath" + "strings" "testing" "github.com/sqlrush/opendbx/internal/app/skills" @@ -142,3 +143,71 @@ func keys(m map[string]skills.SkillRoot) []string { } return out } + +// mkActiveSkill builds a minimal valid active Skill for skillsForChat tests +// (spec-2.3 D-5). +func mkActiveSkill(name string) skills.Skill { + return skills.Skill{ + Schema: skills.Schema{Name: name, Description: "Test skill."}, + Body: "body of " + name, + } +} + +// TestSkillsForChat_ZeroActive — no active skills → no executor, no +// system-prompt section (spec-2.3 Q6: the Skill wire surface is absent). +func TestSkillsForChat_ZeroActive(t *testing.T) { + t.Parallel() + execs, prompt := skillsForChat(skills.DiscoveryResult{}) + if execs != nil || prompt != "" { + t.Errorf("skillsForChat(empty) = %v, %q; want nil, empty", execs, prompt) + } +} + +// TestSkillsForChat_Active — active skills yield exactly one executor +// (wire name "Skill") plus the prompt section listing each skill. +func TestSkillsForChat_Active(t *testing.T) { + t.Parallel() + res := skills.DiscoveryResult{Active: []skills.Skill{mkActiveSkill("alpha"), mkActiveSkill("beta")}} + execs, prompt := skillsForChat(res) + if len(execs) != 1 || execs[0].Name() != "Skill" { + t.Fatalf("execs = %v; want one executor named Skill", execs) + } + for _, want := range []string{"## Available skills", "- alpha:", "- beta:"} { + if !strings.Contains(prompt, want) { + t.Errorf("prompt %q missing %q", prompt, want) + } + } +} + +// TestSkillsForChat_ContractViolation_LogSkipNoPanic — a duplicate Key in +// Active (upstream spec-2.2 contract violation) must be skipped, never +// panic: interact continues without skills (user decision 4/4 2026-06-05). +func TestSkillsForChat_ContractViolation_LogSkipNoPanic(t *testing.T) { + t.Parallel() + defer func() { + if r := recover(); r != nil { + t.Fatalf("skillsForChat panicked on contract violation: %v", r) + } + }() + res := skills.DiscoveryResult{Active: []skills.Skill{mkActiveSkill("dup"), mkActiveSkill("dup")}} + execs, prompt := skillsForChat(res) + if execs != nil || prompt != "" { + t.Errorf("contract violation must skip skills entirely, got %v, %q", execs, prompt) + } +} + +// TestDiagnoseRegistryWith_SkillTool — the registry composes clock + echo +// + the extra executor; the no-extra form matches the legacy default. +func TestDiagnoseRegistryWith_SkillTool(t *testing.T) { + t.Parallel() + execs, _ := skillsForChat(skills.DiscoveryResult{Active: []skills.Skill{mkActiveSkill("alpha")}}) + reg := diagnoseRegistryWith(execs...) + for _, want := range []string{"Skill", "clock", "echo"} { + if _, ok := reg.Get(want); !ok { + t.Errorf("registry missing %q", want) + } + } + if names := defaultDiagnoseRegistry().Names(); len(names) != 2 { + t.Errorf("default registry = %v; want clock+echo only", names) + } +} diff --git a/internal/bootstrap/tui_launcher.go b/internal/bootstrap/tui_launcher.go index 0b4bc44..1e30d16 100644 --- a/internal/bootstrap/tui_launcher.go +++ b/internal/bootstrap/tui_launcher.go @@ -134,13 +134,25 @@ func newChatModel() program.Model { } emitStripThinkMigrationNotice(cfg) provider, perr := factory.New(*cfg) + // spec-2.3 D-5: one-shot skill discovery → SkillTool + system-prompt + // section. Production's FIRST SystemPrompt assignment — the request + // transitions empty→non-empty when skills are present (Q10). Failures + // log + skip; interact always starts (user decision 4/4 — no panic). + // Skipped on the provider-error path: the session cannot chat, so the + // filesystem scan would be wasted I/O (post-impl cr LOW-1). + var skillExecs []diagnose.ToolExecutor + var skillPrompt string + if perr == nil { + skillExecs, skillPrompt = skillsForChat(DiscoverSkills(cfg)) + } opts := llmapp.Options{ ModelName: cfg.LLM.ActiveModel, MaxHistory: cfg.Session.MaxHistoryMessages, StripThink: cfg.LLM.StripThink, ThinkingMode: thinkingModeFromConfig(cfg.LLM.ThinkingMode), ThinkingBudget: cfg.LLM.ThinkingBudget, - Registry: defaultDiagnoseRegistry(), + Registry: diagnoseRegistryWith(skillExecs...), + SystemPrompt: skillPrompt, // spec-1.21 D-6 user-config knobs reach the runtime here. // Per-turn LLM timeout reuses LLMConfig.RequestTimeout per spec // (NOT a duplicate Diagnose.* field) — the diagnose layer is @@ -251,9 +263,20 @@ func emitStripThinkMigrationNotice(cfg *config.Config) { // errors (registry contract violations) and panic at startup so they // surface immediately rather than at first message. func defaultDiagnoseRegistry() *diagnose.Registry { - reg, err := diagnose.NewRegistry(diagnose.ClockTool{}, diagnose.EchoTool{}) + return diagnoseRegistryWith() +} + +// diagnoseRegistryWith builds the production registry (clock + echo) +// plus any extra executors — in practice the spec-2.3 SkillTool, whose +// construction errors were already handled (log + skip) by +// skillsForChat before reaching here. The panic below therefore stays +// the spec-1.21 programmer-error precedent (registry contract +// violations: dup/empty names), unreachable from the skills path. +func diagnoseRegistryWith(extra ...diagnose.ToolExecutor) *diagnose.Registry { + execs := append([]diagnose.ToolExecutor{diagnose.ClockTool{}, diagnose.EchoTool{}}, extra...) + reg, err := diagnose.NewRegistry(execs...) if err != nil { - panic("bootstrap: defaultDiagnoseRegistry: " + err.Error()) + panic("bootstrap: diagnoseRegistryWith: " + err.Error()) } return reg } diff --git a/internal/platform/errcode/testdata/error-codes-frozen.txt b/internal/platform/errcode/testdata/error-codes-frozen.txt index 21ec90d..1042a75 100644 --- a/internal/platform/errcode/testdata/error-codes-frozen.txt +++ b/internal/platform/errcode/testdata/error-codes-frozen.txt @@ -52,13 +52,16 @@ RENDER.UNSUPPORTED_NODE REPORT.WRITE_FAILED SKILL.FILE_UNREADABLE SKILL.INVALID_NAME +SKILL.INVOKE_INVALID SKILL.MISSING_DESCRIPTION SKILL.MISSING_NAME SKILL.NAMESPACE_CONFLICT +SKILL.NOT_FOUND SKILL.NO_FRONTMATTER SKILL.PARSE_ERROR SKILL.ROOT_TOO_MANY_FILES SKILL.ROOT_UNREADABLE +SKILL.SCOPE_TOOL_DENIED SKILL.TOO_DEEP SKILL.TOO_LARGE SKILL.UNTERMINATED_FRONTMATTER diff --git a/internal/platform/logger/slog_handler.go b/internal/platform/logger/slog_handler.go index ea76b8d..af5670b 100644 --- a/internal/platform/logger/slog_handler.go +++ b/internal/platform/logger/slog_handler.go @@ -26,6 +26,16 @@ func ErrorForceFile(msg string, kv ...any) { forceFile(LevelError, msg, attrsFromKV(kv...)) } +// InfoForceFile writes an info record directly to the active debug file even +// when debug logging is disabled. It never writes to stderr and no-ops before +// Init. Use for normal-path startup telemetry that must not pollute the +// warning band. Deliberately bypasses forceFile: forceFileAttrs clamps any +// level below WARN up to WARN (the warn/error band normalizer), which would +// re-label info records as [WARN] (spec-2.3 post-impl codex MED). +func InfoForceFile(msg string, kv ...any) { + fileOnlyAttrs(LevelInfo, msg, attrsFromKV(kv...)) +} + // NewSlogHandler returns a slog.Handler that routes stdlib slog records into // the platform logger's debug file only. All levels bypass the normal logger // writer so TUI-mode diagnostics are preserved without writing to stderr, diff --git a/internal/platform/logger/slog_handler_test.go b/internal/platform/logger/slog_handler_test.go index 557752d..52fb489 100644 --- a/internal/platform/logger/slog_handler_test.go +++ b/internal/platform/logger/slog_handler_test.go @@ -51,6 +51,52 @@ func TestForceFileBypassesDebugGateAndStderr(t *testing.T) { } } +// TestInfoForceFileKeepsInfoBand — InfoForceFile must write [INFO] (not +// the forceFile WARN clamp), reach the file with debug disabled, and +// never touch stderr (spec-2.3 post-impl codex MED). +func TestInfoForceFileKeepsInfoBand(t *testing.T) { + // NOT t.Parallel: mutates logger globals and os.Stderr. + resetForTesting(t) + logPath := t.TempDir() + "/info-force.log" + + oldStderr := os.Stderr + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + os.Stderr = w + defer func() { os.Stderr = oldStderr }() + + // DebugEnabled false: the force-file family must still reach the file. + if err := Init(InitInput{SessionID: "info-force", LogPath: logPath, DisableSidecar: true}); err != nil { + t.Fatalf("Init: %v", err) + } + InfoForceFile("skills active for this session", "active", 3) + + if err := w.Close(); err != nil { + t.Fatalf("close stderr pipe writer: %v", err) + } + stderrRaw, err := io.ReadAll(r) + if err != nil { + t.Fatalf("read stderr pipe: %v", err) + } + if strings.Contains(string(stderrRaw), "skills active") { + t.Fatalf("InfoForceFile wrote to stderr: %q", stderrRaw) + } + + raw, err := os.ReadFile(logPath) + if err != nil { + t.Fatalf("read force log: %v", err) + } + got := string(raw) + if !strings.Contains(got, "[INFO] skills active for this session") || !strings.Contains(got, "active=3") { + t.Fatalf("info force log missing [INFO] message/attrs:\n%s", got) + } + if strings.Contains(got, "[WARN] skills active") { + t.Fatalf("InfoForceFile clamped to WARN band:\n%s", got) + } +} + func TestSlogHandlerLevelMapping(t *testing.T) { // NOT t.Parallel: mutates logger globals. resetForTesting(t) diff --git a/tests/integration/skillinvoke/skillinvoke_test.go b/tests/integration/skillinvoke/skillinvoke_test.go new file mode 100644 index 0000000..b2c258b --- /dev/null +++ b/tests/integration/skillinvoke/skillinvoke_test.go @@ -0,0 +1,217 @@ +// Copyright 2026 opendbx contributors. See LICENSE. +// +// Author: sqlrush + +// Package skillinvoke_test — spec-2.3 D-7 integration: the REAL +// invoke.SkillTool wired into the REAL diagnose.Loop with a scripted +// fake provider, exercised end-to-end against the CC golden skill +// (code-reviewer.md, spec-2.1 D-7 corpus): +// +// invoke → body in transcript → scope narrows → filtered tool denied +// (paired) → Skill implicitly retained → second skill replaces scope +// → now-allowed tool executes → dedup cached replay keeps the scope. +// +// This suite is also the behavioral drift-guard for the wire name: the +// Loop's implicit-retention constant and invoke.ToolName must agree, or +// the "second skill after restrictive scope" step below fails. +package skillinvoke_test + +import ( + "context" + "os" + "strings" + "testing" + + "github.com/sqlrush/opendbx/internal/app/diagnose" + "github.com/sqlrush/opendbx/internal/app/skills" + "github.com/sqlrush/opendbx/internal/app/skills/invoke" + "github.com/sqlrush/opendbx/internal/domain/llm" + "github.com/sqlrush/opendbx/internal/domain/llm/fake" +) + +// loadGolden parses the spec-2.1 CC golden skill (allowed-tools: Read, +// Grep, Glob, Bash — none registered in opendbx, and no "Skill" entry). +func loadGolden(t *testing.T) skills.Skill { + t.Helper() + raw, err := os.ReadFile("../../../internal/app/skills/testdata/code-reviewer.md") + if err != nil { + t.Fatalf("read golden: %v", err) + } + sk, err := skills.Parse(raw, skills.SkillSource{Kind: skills.SourceProject, Precedence: 400}) + if err != nil { + t.Fatalf("parse golden: %v", err) + } + return sk +} + +// dbHelper is a synthetic second skill whose scope allows echo. +func dbHelper() skills.Skill { + return skills.Skill{ + Schema: skills.Schema{ + Name: "db-helper", + Description: "Echo-driven helper for integration tests.", + AllowedTools: "echo", + }, + Body: "Use the echo tool.", + } +} + +// newLoop builds a production-shaped Loop: clock + echo + the real +// SkillTool over the given active set. +func newLoop(t *testing.T, prov llm.Provider, active []skills.Skill, dedup bool) *diagnose.Loop { + t.Helper() + st, err := invoke.NewSkillTool(active) + if err != nil { + t.Fatalf("NewSkillTool: %v", err) + } + reg, err := diagnose.NewRegistry(diagnose.ClockTool{}, diagnose.EchoTool{}, st) + if err != nil { + t.Fatalf("NewRegistry: %v", err) + } + loop, err := diagnose.NewLoop(diagnose.Options{ + Provider: prov, Registry: reg, MaxTurns: 10, DedupEnabled: dedup, + }) + if err != nil { + t.Fatalf("NewLoop: %v", err) + } + return loop +} + +func userReq(text string) llm.Request { + return llm.Request{ + Messages: []llm.Message{ + {Role: llm.RoleUser, Content: []llm.ContentBlock{{Type: llm.BlockText, Text: text}}}, + }, + MaxTokens: 1024, + } +} + +// toolResults flattens every tool_result block in transcript order. +func toolResults(msgs []llm.Message) []llm.ToolResult { + var out []llm.ToolResult + for _, m := range msgs { + for _, c := range m.Content { + if c.Type == llm.BlockToolResult && c.ToolResult != nil { + out = append(out, *c.ToolResult) + } + } + } + return out +} + +// TestSkillInvoke_FullChain — the spec-2.3 D-7 scripted journey. +func TestSkillInvoke_FullChain(t *testing.T) { + t.Parallel() + golden := loadGolden(t) + prov := fake.NewScriptedTurns( + // turn1: enter the golden skill scope {Read, Grep, Glob, Bash}. + fake.Turn{Finish: llm.FinishToolUse, ToolUses: []llm.ToolUse{ + {ID: "c1", Name: "Skill", Input: map[string]any{"skill": "code-reviewer"}}}}, + // turn2: echo is registered but not in scope → denied (recoverable). + fake.Turn{Finish: llm.FinishToolUse, ToolUses: []llm.ToolUse{ + {ID: "c2", Name: "echo", Input: map[string]any{"k": "v"}}}}, + // turn3: "Skill" is implicitly retained → switching skills works + // even though the golden scope does not list it (Q13). + fake.Turn{Finish: llm.FinishToolUse, ToolUses: []llm.ToolUse{ + {ID: "c3", Name: "Skill", Input: map[string]any{"skill": "db-helper"}}}}, + // turn4: echo now in scope → executes for real. + fake.Turn{Finish: llm.FinishToolUse, ToolUses: []llm.ToolUse{ + {ID: "c4", Name: "echo", Input: map[string]any{"k": "v"}}}}, + fake.Turn{Text: "done", Finish: llm.FinishStop}, + ) + loop := newLoop(t, prov, []skills.Skill{golden, dbHelper()}, false) + + calls, results := map[string]bool{}, map[string]bool{} + emit := func(_ context.Context, e diagnose.Event) error { + switch e.Kind { + case diagnose.EventToolCall: + calls[e.ToolUse.ID] = true + case diagnose.EventToolResult: + results[e.ToolResult.ToolUseID] = true + } + return nil + } + res, err := loop.Run(context.Background(), userReq("review my code"), emit) + if err != nil { + t.Fatalf("Run: %v", err) + } + if res.FinishReason != llm.FinishStop || res.Turns != 5 { + t.Fatalf("Result = %+v; want FinishStop/5", res) + } + + // UI pairing: every EventToolCall got its EventToolResult (user + // regression mandate — nothing stuck Running). + for id := range calls { + if !results[id] { + t.Errorf("tool call %s has no EventToolResult (stuck Running)", id) + } + } + + trs := toolResults(res.Messages) + if len(trs) != 4 { + t.Fatalf("tool_result blocks = %d; want 4 (paired commit)", len(trs)) + } + // c1: golden body byte-verbatim + Q7 model notice (model: sonnet). + if !strings.HasPrefix(trs[0].Content, golden.Body) || trs[0].IsError { + t.Errorf("c1 = %+v; want golden body prefix", trs[0]) + } + if !strings.Contains(trs[0].Content, `requests model "sonnet"`) { + t.Errorf("c1 missing Q7 model notice: %q", trs[0].Content) + } + // c2: denied with the SCOPE_TOOL_DENIED template listing the scope. + if !trs[1].IsError || !strings.Contains(trs[1].Content, "SKILL.SCOPE_TOOL_DENIED") || + !strings.Contains(trs[1].Content, "Allowed: [Read, Grep, Glob, Bash] (+ Skill)") { + t.Errorf("c2 = %+v; want SCOPE_TOOL_DENIED with scope listing", trs[1]) + } + // c3: second skill invoke succeeded (implicit Skill retention). + if trs[2].IsError || !strings.HasPrefix(trs[2].Content, "Use the echo tool.") { + t.Errorf("c3 = %+v; want db-helper body (Skill retained, Q13)", trs[2]) + } + // c4: echo executed for real under the replaced scope. + if trs[3].IsError || trs[3].Content != `{"k":"v"}` { + t.Errorf("c4 = %+v; want real echo output under widened scope", trs[3]) + } +} + +// TestSkillInvoke_CachedReplayKeepsScope — invoking the same skill with +// the same input within the dedup window replays the cached body AND its +// ToolFilter (scope update is never gated on !cached; spec-2.3 DoD). +func TestSkillInvoke_CachedReplayKeepsScope(t *testing.T) { + t.Parallel() + prov := fake.NewScriptedTurns( + fake.Turn{Finish: llm.FinishToolUse, ToolUses: []llm.ToolUse{ + {ID: "c1", Name: "Skill", Input: map[string]any{"skill": "db-helper"}}}}, + // Same name+input → dedup cached hit; ToolFilter {echo} replayed. + fake.Turn{Finish: llm.FinishToolUse, ToolUses: []llm.ToolUse{ + {ID: "c2", Name: "Skill", Input: map[string]any{"skill": "db-helper"}}}}, + // clock is registered but outside {echo} scope → still denied. + fake.Turn{Finish: llm.FinishToolUse, ToolUses: []llm.ToolUse{ + {ID: "c3", Name: "clock", Input: map[string]any{}}}}, + fake.Turn{Text: "done", Finish: llm.FinishStop}, + ) + loop := newLoop(t, prov, []skills.Skill{dbHelper()}, true) + + var c2Cached bool + emit := func(_ context.Context, e diagnose.Event) error { + if e.Kind == diagnose.EventToolResult && e.ToolResult.ToolUseID == "c2" { + c2Cached = e.Cached + } + return nil + } + res, err := loop.Run(context.Background(), userReq("go"), emit) + if err != nil { + t.Fatalf("Run: %v", err) + } + if !c2Cached { + t.Fatal("c2 expected to be a dedup cached hit") + } + trs := toolResults(res.Messages) + // Cached content byte-identical to the fresh run (§ 3.6 wire-clean). + if trs[0].Content != trs[1].Content { + t.Errorf("cached content differs from fresh:\n%q\nvs\n%q", trs[0].Content, trs[1].Content) + } + // Scope still live after the cached replay: clock denied. + if !trs[2].IsError || !strings.Contains(trs[2].Content, "SKILL.SCOPE_TOOL_DENIED") { + t.Errorf("c3 = %+v; want denial (cached replay kept scope)", trs[2]) + } +} diff --git a/tests/integration/uitest/block/testdata/visual/ToolUseResolvedSkillArgs/.gitkeep b/tests/integration/uitest/block/testdata/visual/ToolUseResolvedSkillArgs/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/tests/integration/uitest/block/testdata/visual/ToolUseRunningSkill/.gitkeep b/tests/integration/uitest/block/testdata/visual/ToolUseRunningSkill/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/tests/integration/uitest/block/testdata/visual/ToolUseRunningSkillNoName/.gitkeep b/tests/integration/uitest/block/testdata/visual/ToolUseRunningSkillNoName/.gitkeep new file mode 100644 index 0000000..e69de29 diff --git a/tests/integration/uitest/block/tooluse_render_test.go b/tests/integration/uitest/block/tooluse_render_test.go index fe7c08b..a453ac7 100644 --- a/tests/integration/uitest/block/tooluse_render_test.go +++ b/tests/integration/uitest/block/tooluse_render_test.go @@ -124,6 +124,44 @@ func TestToolUseVisualGolden(t *testing.T) { ToolUseState: "resolved", }, }, + // spec-2.3 D-4 (post-impl cr MED-2): Skill adapter Layer-2 cases. + { + name: "ToolUseRunningSkill", + tu: withState(block.NewToolUse("id8", "Skill", map[string]any{"skill": "code-reviewer"}), + block.StateRunning), + cols: 80, + meta: toolUseFixtureMetadata{ + Adapter: "skill", + Prompt: "Invoke the code-reviewer skill and capture the running Skill tool use.", + ToolUseState: "running", + }, + }, + { + name: "ToolUseResolvedSkillArgs", + tu: withState(block.NewToolUse("id9", "Skill", map[string]any{ + "skill": "topsql", + "args": map[string]any{"db": "main", "limit": "10"}, + }), block.StateResolved), + cols: 80, + meta: toolUseFixtureMetadata{ + Adapter: "skill", + Notes: "Args render as the sorted compact k=v summary after Skill().", + Prompt: "Invoke topsql with args and capture the resolved Skill tool use.", + ToolUseState: "resolved", + }, + }, + { + name: "ToolUseRunningSkillNoName", + tu: withState(block.NewToolUse("idA", "Skill", map[string]any{}), + block.StateRunning), + cols: 80, + meta: toolUseFixtureMetadata{ + Adapter: "skill", + Notes: "Missing skill field renders the Skill(no skill) placeholder.", + Prompt: "Mock a Skill call without a skill field (placeholder fallback).", + ToolUseState: "running", + }, + }, } for _, tc := range cases { @@ -242,6 +280,10 @@ func TestToolUseVisualGolden_ParkedFixtures(t *testing.T) { "ToolUseWaitingPermission", "ToolUseRunningBashWithProgress", "ToolUseResolvedRead", + // spec-2.3 D-4 Skill adapter (capture pending per SOP). + "ToolUseRunningSkill", + "ToolUseResolvedSkillArgs", + "ToolUseRunningSkillNoName", } for _, name := range required { path := visualFixturePath(t, name, "")