From abdfd3e28cead2feaaa6fa42de693692db919a6b Mon Sep 17 00:00:00 2001 From: Bohan Jiang <52446949+Bohan-J@users.noreply.github.com> Date: Fri, 31 Jul 2026 13:27:50 +0800 Subject: [PATCH] refactor(skills): make the brief's skill list a names-only index (MUL-5529) (#6207) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * refactor(skills): make the brief's skill list a names-only index (MUL-5529) Step 3 of MUL-5529. Every runtime CLI discovers the SKILL.md files the daemon writes and builds its own listing from their frontmatter — verified against 11 locally installed CLIs plus official docs for 5 more. The brief's copy of those descriptions was therefore the same routing signal paid for twice: measured on a real task, `## Skills` was 13,295 chars, 40% of the entire brief, against a 16,304-char CLI listing of the same 28 skills. Now 850 chars for that same set — roughly 3,100 tokens back per brief. The index itself stays. It is the one skill listing Multica controls; each CLI's own listing is theirs, and its format — or its existence — can change with any release. Three changes: - Descriptions dropped from the `## Skills` entries. - The per-provider branch is gone. Its fallback told providers outside a hardcoded list to read `.agent_context/skills/`, but the only providers that ever reached it were grok and traecli, whose files are written to `.grok/skills` and `.traecli/skills` and which discover natively. The pointer was wrong for everyone it addressed, so removing the branch deletes the bug rather than relocating it. This closes MUL-5537. - issue_context.md and its quick-create / autopilot variants no longer render `## Agent Skills`. That copy duplicated the brief once both were names-only, and nothing ever read it: no prompt references the path, and grepping the server finds only the writer. `.agent_context/skills/` had the same fate for hermes (issue #5242). Quick-create, previously skipped in the brief and served only by that unread copy, now gets the brief section like every other kind — one index, one place. Not included: skills carrying `disable-model-invocation` are still written to disk for every provider. The plan assumed that key needed provider-specific handling for everything except claude; probing the installed CLIs shows 9 of 11 honor it, and only opencode and hermes do not. The remaining question is narrow and a genuine product tradeoff — withholding the file honors the author's intent but also removes explicit invocation — so it is left to a separate decision rather than folded in here. Co-authored-by: multica-agent * docs(skills): align stale comments with the names-only brief contract (MUL-5529) Co-authored-by: multica-agent --------- Co-authored-by: Bohan-J Co-authored-by: multica-agent Co-authored-by: Steve Jobs (Multica Agent) --- server/internal/daemon/execenv/context.go | 35 ++----------- .../internal/daemon/execenv/execenv_test.go | 30 +++++------ .../execenv/runtime_config_kind_test.go | 6 +-- .../daemon/execenv/runtime_config_sections.go | 45 ++++++++-------- .../daemon/execenv/runtime_config_test.go | 51 +++++++++++++++++++ .../daemon/execenv/skill_visibility.go | 14 +++-- .../daemon/execenv/skill_visibility_test.go | 48 +++++++++-------- 7 files changed, 125 insertions(+), 104 deletions(-) diff --git a/server/internal/daemon/execenv/context.go b/server/internal/daemon/execenv/context.go index 978d51099..23f0015c7 100644 --- a/server/internal/daemon/execenv/context.go +++ b/server/internal/daemon/execenv/context.go @@ -1007,23 +1007,14 @@ func renderIssueContext(provider string, ctx TaskContextForEnv) string { b.WriteString("## Quick Start\n\n") fmt.Fprintf(&b, "Run `multica issue get %s --output json` to fetch the full issue details.\n\n", ctx.IssueID) - skills := modelVisibleSkills(ctx.AgentSkills) - if len(skills) > 0 { - b.WriteString("## Agent Skills\n\n") - b.WriteString("The following skills are available to you:\n\n") - for _, skill := range skills { - fmt.Fprintf(&b, "- **%s**\n", skill.Name) - } - b.WriteString("\n") - } - return b.String() } // renderQuickCreateContext renders issue_context.md for quick-create tasks. -// This file carries only task data (user input, skills). Behavioral rules -// and guardrails live in AGENTS.md (runtime config) and the per-turn prompt -// to avoid redundancy and conflicting instructions. +// This file carries only task data (the user input). Behavioral rules and +// guardrails live in AGENTS.md (runtime config) and the per-turn prompt to +// avoid redundancy and conflicting instructions; the skill index lives in the +// runtime brief like every other kind (MUL-5529). func renderQuickCreateContext(ctx TaskContextForEnv) string { var b strings.Builder b.WriteString("# Quick Create\n\n") @@ -1032,14 +1023,6 @@ func renderQuickCreateContext(ctx TaskContextForEnv) string { b.WriteString("> ") b.WriteString(ctx.QuickCreatePrompt) b.WriteString("\n\n") - skills := modelVisibleSkills(ctx.AgentSkills) - if len(skills) > 0 { - b.WriteString("## Agent Skills\n\n") - for _, skill := range skills { - fmt.Fprintf(&b, "- **%s**\n", skill.Name) - } - b.WriteString("\n") - } return b.String() } @@ -1072,15 +1055,5 @@ func renderAutopilotContext(ctx TaskContextForEnv) string { b.WriteString("\n\n") } - skills := modelVisibleSkills(ctx.AgentSkills) - if len(skills) > 0 { - b.WriteString("## Agent Skills\n\n") - b.WriteString("The following skills are available to you:\n\n") - for _, skill := range skills { - fmt.Fprintf(&b, "- **%s**\n", skill.Name) - } - b.WriteString("\n") - } - return b.String() } diff --git a/server/internal/daemon/execenv/execenv_test.go b/server/internal/daemon/execenv/execenv_test.go index b49d68014..c4eaf9fef 100644 --- a/server/internal/daemon/execenv/execenv_test.go +++ b/server/internal/daemon/execenv/execenv_test.go @@ -129,11 +129,12 @@ func TestPrepareDirectoryMode(t *testing.T) { if err != nil { t.Fatalf("failed to read issue_context.md: %v", err) } - // "Code Review" is listed by its on-disk slug (MUL-5529). - for _, want := range []string{"a1b2c3d4-e5f6-7890-abcd-ef1234567890", "code-review"} { - if !strings.Contains(string(content), want) { - t.Fatalf("issue_context.md missing %q", want) - } + if !strings.Contains(string(content), "a1b2c3d4-e5f6-7890-abcd-ef1234567890") { + t.Fatalf("issue_context.md missing the issue id") + } + // The skill list lives in the runtime brief only (MUL-5529). + if strings.Contains(string(content), "code-review") { + t.Fatalf("issue_context.md should no longer carry a skill list:\n%s", content) } markerContent, err := os.ReadFile(filepath.Join(env.WorkDir, TaskContextMarkerRelPath)) @@ -449,22 +450,17 @@ func TestWriteContextFiles(t *testing.T) { } s := string(content) - for _, want := range []string{ - "test-issue-id-1234", - "## Agent Skills", - // Listed by on-disk slug, not the "Go Conventions" display name — - // the slug is the only name the model can invoke (MUL-5529). - "go-conventions", - } { - if !strings.Contains(s, want) { - t.Errorf("content missing %q", want) - } + if !strings.Contains(s, "test-issue-id-1234") { + t.Errorf("content missing %q", "test-issue-id-1234") } // Issue details should NOT be in the context file (agent fetches via CLI). - for _, absent := range []string{"## Description", "## Workspace Context"} { + // + // Nor the skill list: nothing ever read this copy, and the runtime brief + // carries the same names-only index (MUL-5529). + for _, absent := range []string{"## Description", "## Workspace Context", "## Agent Skills", "go-conventions"} { if strings.Contains(s, absent) { - t.Errorf("content should NOT contain %q — agent fetches details via CLI", absent) + t.Errorf("content should NOT contain %q", absent) } } diff --git a/server/internal/daemon/execenv/runtime_config_kind_test.go b/server/internal/daemon/execenv/runtime_config_kind_test.go index dd72b65e0..6058ba3ac 100644 --- a/server/internal/daemon/execenv/runtime_config_kind_test.go +++ b/server/internal/daemon/execenv/runtime_config_kind_test.go @@ -143,9 +143,9 @@ func TestBuildMetaSkillContentSlimKindMatrix(t *testing.T) { {"## Issue Metadata", issueKinds}, {"## Instruction Precedence", issueKinds}, {"## Sub-issue Creation", issueKinds}, - {"## Skills", map[taskKind]bool{ - kindIssue: true, kindAutopilotRunOnly: true, kindChat: true, - }}, + // Quick-create included: it used to be skipped here and carry its own + // copy in issue_context.md, which nothing read. One index, one place. + {"## Skills", allKinds}, {"## Mentions", issueKinds}, {"## Attachments", issueKinds}, } diff --git a/server/internal/daemon/execenv/runtime_config_sections.go b/server/internal/daemon/execenv/runtime_config_sections.go index a3f3c34d3..e2f0e4d3d 100644 --- a/server/internal/daemon/execenv/runtime_config_sections.go +++ b/server/internal/daemon/execenv/runtime_config_sections.go @@ -548,28 +548,32 @@ func writeSubIssueCreation(b *strings.Builder) { b.WriteString("**Ordering with stages.** For phased plans, group children with `--stage ` (N ≥ 1) instead of hand-promoting the backlog chain — stage members run together, and the parent wakes once per stage. Use `--stage k --status backlog` for later stages, then `multica issue children ` to inspect groupings before promoting. Reach for stages whenever a plan has more than one step or a step must wait for a group.\n\n") } -// writeSkills emits the Skills section listing skill names + descriptions. -func writeSkills(b *strings.Builder, provider string, ctx TaskContextForEnv) { +// writeSkills emits the Skills section: an index of invocable skill names. +// +// Names only, deliberately. Every runtime CLI discovers the SKILL.md files the +// daemon writes and builds its own listing from their frontmatter, so repeating +// the descriptions here bought a second, more expensive copy of what the model +// already had — measured at ~3,100 tokens per brief on a real task, 40% of the +// whole brief — and no extra routing signal (MUL-5529). +// +// The index itself stays because it is the one skill listing Multica controls. +// Each CLI's own listing is theirs: its format, and whether it exists at all, +// can change with any release. +// +// There is no per-provider branch. The old fallback told providers outside a +// hardcoded list to read `.agent_context/skills/`, which was the wrong path for +// every provider that actually reached it — grok and traecli write to +// `.grok/skills` and `.traecli/skills` — while both discover natively and never +// needed the pointer. +func writeSkills(b *strings.Builder, ctx TaskContextForEnv) { skills := modelVisibleSkills(ctx.AgentSkills) if len(skills) == 0 { return } b.WriteString("## Skills\n\n") - switch provider { - case "claude", "codebuddy", "codex", "copilot", "opencode", "deveco", "openclaw", "hermes", "pi", "cursor", "kimi", "kiro", "qoder", "antigravity", "qwen": - // Hermes discovers these from its per-task HERMES_HOME/skills (seeded by - // the daemon), so it needs the same "discovered automatically" framing - // as the other native-discovery runtimes rather than a path pointer. - b.WriteString("You have the following skills installed (discovered automatically):\n\n") - default: - b.WriteString("Detailed skill instructions are in `.agent_context/skills/`. Each subdirectory contains a `SKILL.md`.\n\n") - } + b.WriteString("You have the following skills installed (discovered automatically):\n\n") for _, skill := range skills { - if desc := strings.TrimSpace(skill.Description); desc != "" { - fmt.Fprintf(b, "- **%s** — %s\n", skill.Name, desc) - } else { - fmt.Fprintf(b, "- **%s**\n", skill.Name) - } + fmt.Fprintf(b, "- **%s**\n", skill.Name) } b.WriteString("\n") } @@ -685,7 +689,7 @@ func writeOutput(b *strings.Builder, kind taskKind, ctx TaskContextForEnv) { // Issue Metadata | ✓ | ✓ | — | — | — // Instruction Precedence| — | ✓ | — | — | — // Sub-issue Creation | ✓ | ✓ | — | — | — -// Skills | ✓ | ✓ | ✓ | — | ✓ +// Skills | ✓ | ✓ | ✓ | ✓ | ✓ // Mentions | ✓ | ✓ | — | — | — // Attachments | ✓ | ✓ | — | — | — // @@ -749,9 +753,10 @@ func buildMetaSkillContentSlim(provider string, ctx TaskContextForEnv) string { writeSubIssueCreation(&b) } - if kind != kindQuickCreate { - writeSkills(&b, provider, ctx) - } + // Every kind, quick-create included. Quick-create used to be skipped here + // and carried its own copy in issue_context.md instead; now that both are + // the same names-only index, the brief is the one that survives. + writeSkills(&b, ctx) if kind == kindIssue { writeMentions(&b) diff --git a/server/internal/daemon/execenv/runtime_config_test.go b/server/internal/daemon/execenv/runtime_config_test.go index 16928a14c..9b41f3e67 100644 --- a/server/internal/daemon/execenv/runtime_config_test.go +++ b/server/internal/daemon/execenv/runtime_config_test.go @@ -1744,3 +1744,54 @@ func TestBriefByteIdenticalAcrossRunsForEveryKind(t *testing.T) { }) } } + +// TestBriefSkillsListIsNamesOnly pins the shape of the `## Skills` section: an +// index of invocable names, with no descriptions and no per-provider branch. +// +// Descriptions were removed because every runtime CLI already builds its own +// listing from the SKILL.md frontmatter the daemon writes, so the brief's copy +// was the same routing signal paid for twice — ~3,100 tokens per brief on a +// real task, 40% of the whole brief (MUL-5529). +// +// The provider branch was removed because its fallback was wrong: it told +// providers outside a hardcoded list to look in `.agent_context/skills/`, but +// the only providers that ever reached it — grok and traecli — have their files +// written to `.grok/skills` and `.traecli/skills` and discover them natively. +func TestBriefSkillsListIsNamesOnly(t *testing.T) { + t.Parallel() + + ctx := TaskContextForEnv{ + IssueID: "issue-1", + AgentName: "Eve", + AgentID: "eve-1", + AgentSkills: []SkillContextForEnv{ + { + Name: "PR Review", + Description: "Use when reviewing a pull request for the Multica project.", + Content: "---\nname: pr-review\n---\n\nbody", + }, + }, + } + + // grok and traecli are the providers that used to take the removed branch; + // the rest are a spread across the native-discovery list. + for _, provider := range []string{"claude", "codex", "opencode", "hermes", "grok", "traecli", "some-unknown-provider"} { + t.Run(provider, func(t *testing.T) { + t.Parallel() + out := buildMetaSkillContent(provider, ctx) + + if !strings.Contains(out, "- **pr-review**\n") { + t.Errorf("brief does not list the skill by slug:\n%s", out) + } + if strings.Contains(out, "Use when reviewing a pull request") { + t.Errorf("brief still carries the skill description; the CLI's own listing already has it:\n%s", out) + } + if strings.Contains(out, ".agent_context/skills/") { + t.Errorf("brief still points at the removed fallback path:\n%s", out) + } + if !strings.Contains(out, "discovered automatically") { + t.Errorf("brief lost the native-discovery framing:\n%s", out) + } + }) + } +} diff --git a/server/internal/daemon/execenv/skill_visibility.go b/server/internal/daemon/execenv/skill_visibility.go index 832433c77..bb275276a 100644 --- a/server/internal/daemon/execenv/skill_visibility.go +++ b/server/internal/daemon/execenv/skill_visibility.go @@ -9,14 +9,12 @@ import ( // modelVisibleSkills returns the skills a model may invoke, with Name // normalized to the on-disk slug. // -// The normalization is not cosmetic. Every caller renders these entries into a -// listing the model reads to pick a skill, and the only name it can actually -// invoke is the directory slug writeSkillFiles lays down — sanitizeSkillName of -// the same field. A workspace skill's Name is its human display name ("PR -// review"), so listing it verbatim hands the model an identifier that does not -// resolve (MUL-5529). Normalizing here rather than at each call site keeps the -// four listings (runtime brief + the three issue_context renderers) from -// drifting apart. +// The normalization is not cosmetic. The runtime brief renders these entries +// into the listing the model reads to pick a skill, and the only name it can +// actually invoke is the directory slug writeSkillFiles lays down — +// sanitizeSkillName of the same field. A workspace skill's Name is its human +// display name ("PR review"), so listing it verbatim hands the model an +// identifier that does not resolve (MUL-5529). // // Slugs come from resolveSkillSlugs over the *unfiltered* batch, because // writeSkillFiles lays down every skill — including the ones hidden here — and diff --git a/server/internal/daemon/execenv/skill_visibility_test.go b/server/internal/daemon/execenv/skill_visibility_test.go index c6bc7c9bd..fd2a4f2ae 100644 --- a/server/internal/daemon/execenv/skill_visibility_test.go +++ b/server/internal/daemon/execenv/skill_visibility_test.go @@ -117,37 +117,35 @@ Hidden body.`, }, } - rendered := map[string]string{ - "runtime config": buildMetaSkillContent("codex", TaskContextForEnv{ - IssueID: ctx.IssueID, - AgentName: ctx.AgentName, - AgentID: ctx.AgentID, - AgentSkills: ctx.AgentSkills, - }), - "issue context": renderIssueContext("codex", TaskContextForEnv{ - IssueID: ctx.IssueID, - AgentSkills: ctx.AgentSkills, - }), - "quick create": renderQuickCreateContext(TaskContextForEnv{ - QuickCreatePrompt: ctx.QuickCreatePrompt, - AgentSkills: ctx.AgentSkills, - }), - "autopilot": renderAutopilotContext(TaskContextForEnv{ - AutopilotRunID: ctx.AutopilotRunID, - AgentSkills: ctx.AgentSkills, - }), - } - - for name, out := range rendered { + // The brief is now the only Multica-rendered skill listing, for every kind. + for _, kind := range []TaskContextForEnv{ + {IssueID: ctx.IssueID, AgentName: ctx.AgentName, AgentID: ctx.AgentID, AgentSkills: ctx.AgentSkills}, + {QuickCreatePrompt: ctx.QuickCreatePrompt, AgentName: ctx.AgentName, AgentID: ctx.AgentID, AgentSkills: ctx.AgentSkills}, + {AutopilotRunID: ctx.AutopilotRunID, AgentName: ctx.AgentName, AgentID: ctx.AgentID, AgentSkills: ctx.AgentSkills}, + {ChatSessionID: "c-1", AgentName: ctx.AgentName, AgentID: ctx.AgentID, AgentSkills: ctx.AgentSkills}, + } { + out := buildMetaSkillContent("codex", kind) // Listings carry the on-disk slug, not the display name (MUL-5529). if !strings.Contains(out, "visible-skill") { - t.Errorf("%s missing visible skill:\n%s", name, out) + t.Errorf("brief missing visible skill:\n%s", out) } if strings.Contains(out, "Visible Skill") { - t.Errorf("%s lists the display name instead of the slug:\n%s", name, out) + t.Errorf("brief lists the display name instead of the slug:\n%s", out) } if strings.Contains(out, "hidden-skill") || strings.Contains(out, "hidden description") { - t.Errorf("%s advertised disable-model-invocation skill:\n%s", name, out) + t.Errorf("brief advertised disable-model-invocation skill:\n%s", out) + } + } + + // issue_context.md and its quick-create / autopilot variants no longer + // carry a skill list at all; nothing read that copy. + for name, out := range map[string]string{ + "issue context": renderIssueContext("codex", TaskContextForEnv{IssueID: ctx.IssueID, AgentSkills: ctx.AgentSkills}), + "quick create": renderQuickCreateContext(TaskContextForEnv{QuickCreatePrompt: ctx.QuickCreatePrompt, AgentSkills: ctx.AgentSkills}), + "autopilot": renderAutopilotContext(TaskContextForEnv{AutopilotRunID: ctx.AutopilotRunID, AgentSkills: ctx.AgentSkills}), + } { + if strings.Contains(out, "## Agent Skills") || strings.Contains(out, "visible-skill") { + t.Errorf("%s still renders a skill list:\n%s", name, out) } } }