C0: address verified gadfly findings (trivial fixes)
From the PR #8 review (all graded in the gadfly MCP): - skip empty palette names + dedupe by final tool name, instead of producing a "skill__" tool or an opaque box.Add duplicate error. - delegationResult: no trailing blank line when a non-ok child produced no output. - delegationErr: fold a child's partial output into the hard-failure error so it isn't silently dropped. Deferred to C0b (design-level, not trivial): route delegation through the tool.Registry gate/audit wrappers; expose the skill's real input schema to the LLM instead of a generic inputs map. typed-nil PaletteSource is left as a caller contract (the == nil guard catches the untyped-nil interface). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
+31
-7
@@ -23,38 +23,49 @@ func addDelegationTools(box *llm.Toolbox, ra RunnableAgent, inv tool.Invocation,
|
||||
if palette == nil {
|
||||
return nil
|
||||
}
|
||||
seen := map[string]bool{} // dedupe across both palettes by final tool name
|
||||
for _, name := range ra.SkillPalette {
|
||||
name := name // capture
|
||||
toolName := "skill__" + name
|
||||
if name == "" || seen[toolName] { // skip empty / duplicate palette entries
|
||||
continue
|
||||
}
|
||||
seen[toolName] = true
|
||||
t := llm.DefineTool(
|
||||
"skill__"+name,
|
||||
toolName,
|
||||
fmt.Sprintf("Delegate the task to the %q skill. Provide its declared inputs.", name),
|
||||
func(ctx context.Context, args skillDelegateArgs) (any, error) {
|
||||
out, _, status, err := palette.InvokeSkill(ctx, inv.CallerID, inv.ChannelID, name, args.Inputs, inv.RunID)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("skill %q failed: %w", name, err)
|
||||
return nil, delegationErr("skill", name, out, err)
|
||||
}
|
||||
return delegationResult(name, "skill", out, status), nil
|
||||
},
|
||||
)
|
||||
if err := box.Add(t); err != nil {
|
||||
return fmt.Errorf("add skill__%s: %w", name, err)
|
||||
return fmt.Errorf("add %s: %w", toolName, err)
|
||||
}
|
||||
}
|
||||
for _, name := range ra.SubAgentPalette {
|
||||
name := name // capture
|
||||
toolName := "agent__" + name
|
||||
if name == "" || seen[toolName] {
|
||||
continue
|
||||
}
|
||||
seen[toolName] = true
|
||||
t := llm.DefineTool(
|
||||
"agent__"+name,
|
||||
toolName,
|
||||
fmt.Sprintf("Delegate the task to the %q sub-agent with a natural-language prompt.", name),
|
||||
func(ctx context.Context, args agentDelegateArgs) (any, error) {
|
||||
out, _, status, err := palette.InvokeAgent(ctx, inv.CallerID, inv.ChannelID, name, args.Prompt, inv.RunID, "", "", nil, nil)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("agent %q failed: %w", name, err)
|
||||
return nil, delegationErr("agent", name, out, err)
|
||||
}
|
||||
return delegationResult(name, "agent", out, status), nil
|
||||
},
|
||||
)
|
||||
if err := box.Add(t); err != nil {
|
||||
return fmt.Errorf("add agent__%s: %w", name, err)
|
||||
return fmt.Errorf("add %s: %w", toolName, err)
|
||||
}
|
||||
}
|
||||
return nil
|
||||
@@ -64,11 +75,24 @@ func addDelegationTools(box *llm.Toolbox, ra RunnableAgent, inv tool.Invocation,
|
||||
// react to a timeout/cancel/budget stop) while still passing the partial output.
|
||||
func delegationResult(name, kind, out, status string) string {
|
||||
if status != "" && status != "ok" {
|
||||
return fmt.Sprintf("[%s %q ended with status %q]\n%s", kind, name, status, out)
|
||||
header := fmt.Sprintf("[%s %q ended with status %q]", kind, name, status)
|
||||
if out == "" { // no trailing blank line when there's no body
|
||||
return header
|
||||
}
|
||||
return header + "\n" + out
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
// delegationErr wraps a hard delegation failure, folding in any partial output
|
||||
// the child produced before failing (so it isn't silently lost).
|
||||
func delegationErr(kind, name, partial string, err error) error {
|
||||
if partial != "" {
|
||||
return fmt.Errorf("%s %q failed (partial output: %q): %w", kind, name, partial, err)
|
||||
}
|
||||
return fmt.Errorf("%s %q failed: %w", kind, name, err)
|
||||
}
|
||||
|
||||
type skillDelegateArgs struct {
|
||||
Inputs map[string]any `json:"inputs" description:"Inputs for the skill, matching its declared input schema."`
|
||||
}
|
||||
|
||||
@@ -99,3 +99,27 @@ func TestNoPaletteNoDelegationTools(t *testing.T) {
|
||||
t.Fatalf("nil-palette run failed: %v / %q", res.Err, res.Output)
|
||||
}
|
||||
}
|
||||
|
||||
// TestDelegationDedupeAndEmptySkip: empty + duplicate palette names are skipped,
|
||||
// not turned into "skill__"/duplicate tools that error at box.Add (gadfly C0).
|
||||
func TestDelegationDedupeAndEmptySkip(t *testing.T) {
|
||||
pal := &recordingPalette{}
|
||||
fp := fake.New("fake")
|
||||
fp.Enqueue("m", fake.Reply("ok"))
|
||||
m, _ := fp.Model("m")
|
||||
ex := run.New(run.Config{
|
||||
Registry: tool.NewRegistry(),
|
||||
Models: func(ctx context.Context, _ string) (context.Context, llm.Model, error) { return ctx, m, nil },
|
||||
Ports: run.Ports{Palette: pal},
|
||||
})
|
||||
// "" (empty) and a duplicate "helper" must not break the build.
|
||||
res := ex.Run(context.Background(),
|
||||
run.RunnableAgent{Name: "x", ModelTier: "m", SkillPalette: []string{"helper", "", "helper"}},
|
||||
tool.Invocation{RunID: "r"}, "hi")
|
||||
if res.Err != nil {
|
||||
t.Fatalf("empty/duplicate palette names should be skipped, not error: %v", res.Err)
|
||||
}
|
||||
if res.Output != "ok" {
|
||||
t.Fatalf("output = %q", res.Output)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user