diff --git a/pkg/github/instructions.go b/pkg/github/instructions.go index 3a5fb54bb..496efcccd 100644 --- a/pkg/github/instructions.go +++ b/pkg/github/instructions.go @@ -3,9 +3,164 @@ package github import ( "os" "slices" + "sort" "strings" ) +// ============================================================================= +// SPIKE: Tool-based instruction specificity rules +// ============================================================================= + +// InstructionRule associates an instruction with a set of tools. +// Rules with more specific tool sets (supersets) take precedence over +// rules with fewer tools when the same tools are involved. +type InstructionRule struct { + // ID is a unique identifier for the rule (for debugging/logging) + ID string + // ToolNames is the set of tool names this rule applies to. + // The rule is active when ALL of these tools are present in the active set. + ToolNames map[string]bool + // Instruction is the instruction text for this rule. + Instruction string +} + +// NewInstructionRule creates an InstructionRule from a list of tool names. +func NewInstructionRule(id string, instruction string, toolNames ...string) InstructionRule { + tools := make(map[string]bool, len(toolNames)) + for _, name := range toolNames { + tools[name] = true + } + return InstructionRule{ + ID: id, + ToolNames: tools, + Instruction: instruction, + } +} + +// InstructionResolver resolves which instructions apply based on the +// most-specificity rule: when multiple rules match, rules that are +// supersets of other matching rules shadow those smaller rules. +type InstructionResolver struct { + rules []InstructionRule +} + +// NewInstructionResolver creates a new resolver with the given rules. +func NewInstructionResolver(rules []InstructionRule) *InstructionResolver { + return &InstructionResolver{rules: rules} +} + +// isSubset returns true if a is a subset of b (all elements of a are in b). +func isSubset(a, b map[string]bool) bool { + for key := range a { + if !b[key] { + return false + } + } + return true +} + +// isProperSubset returns true if a is a proper subset of b (a ⊂ b, not equal). +func isProperSubset(a, b map[string]bool) bool { + return len(a) < len(b) && isSubset(a, b) +} + +// matchingRule represents a rule that matched the active tools. +type matchingRule struct { + rule InstructionRule + shadowed bool +} + +// ResolveInstructions returns the instructions that apply to the given active tools, +// using the most-specificity rule to eliminate shadowed rules. +// +// A rule is "shadowed" if another matching rule's ToolNames is a proper superset +// of this rule's ToolNames. The idea is that the more specific rule (covering more +// tools) should take precedence. +func (r *InstructionResolver) ResolveInstructions(activeTools []string) []string { + // Build active tools set for O(1) lookup + activeSet := make(map[string]bool, len(activeTools)) + for _, tool := range activeTools { + activeSet[tool] = true + } + + // Find all rules where ALL required tools are present in activeSet + var matches []matchingRule + for _, rule := range r.rules { + if isSubset(rule.ToolNames, activeSet) { + matches = append(matches, matchingRule{rule: rule}) + } + } + + // Apply shadowing: mark rules as shadowed if another rule is a proper superset + for i := range matches { + for j := range matches { + if i == j { + continue + } + // If rule j's tools are a proper superset of rule i's tools, + // then rule i is shadowed by rule j + if isProperSubset(matches[i].rule.ToolNames, matches[j].rule.ToolNames) { + matches[i].shadowed = true + break // Once shadowed, no need to check further + } + } + } + + // Collect non-shadowed instructions, sorted by rule ID for determinism + var result []string + for _, m := range matches { + if !m.shadowed { + result = append(result, m.rule.Instruction) + } + } + + // Sort for deterministic output (by instruction content as a simple approach) + sort.Strings(result) + + return result +} + +// MatchingRuleIDs returns the IDs of rules that match and are not shadowed. +// Useful for debugging/testing. +func (r *InstructionResolver) MatchingRuleIDs(activeTools []string) []string { + activeSet := make(map[string]bool, len(activeTools)) + for _, tool := range activeTools { + activeSet[tool] = true + } + + var matches []matchingRule + for _, rule := range r.rules { + if isSubset(rule.ToolNames, activeSet) { + matches = append(matches, matchingRule{rule: rule}) + } + } + + for i := range matches { + for j := range matches { + if i == j { + continue + } + if isProperSubset(matches[i].rule.ToolNames, matches[j].rule.ToolNames) { + matches[i].shadowed = true + break + } + } + } + + var ids []string + for _, m := range matches { + if !m.shadowed { + ids = append(ids, m.rule.ID) + } + } + sort.Strings(ids) + return ids +} + +// ============================================================================= +// END SPIKE +// ============================================================================= + // GenerateInstructions creates server instructions based on enabled toolsets func GenerateInstructions(enabledToolsets []string) string { // For testing - add a flag to disable instructions diff --git a/pkg/github/instructions_test.go b/pkg/github/instructions_test.go index b8ad2ba8c..17f211cd9 100644 --- a/pkg/github/instructions_test.go +++ b/pkg/github/instructions_test.go @@ -184,3 +184,211 @@ func TestGetToolsetInstructions(t *testing.T) { }) } } + +// ============================================================================= +// SPIKE TESTS: InstructionResolver with most-specificity rule +// ============================================================================= + +func TestInstructionResolver_BasicMatching(t *testing.T) { + resolver := NewInstructionResolver([]InstructionRule{ + NewInstructionRule("rule-a", "Instruction A", "tool1"), + NewInstructionRule("rule-b", "Instruction B", "tool2"), + NewInstructionRule("rule-c", "Instruction C", "tool3"), + }) + + tests := []struct { + name string + activeTools []string + expectedIDs []string + expectedInst []string + }{ + { + name: "single tool matches single rule", + activeTools: []string{"tool1"}, + expectedIDs: []string{"rule-a"}, + expectedInst: []string{"Instruction A"}, + }, + { + name: "two tools match two rules", + activeTools: []string{"tool1", "tool2"}, + expectedIDs: []string{"rule-a", "rule-b"}, + expectedInst: []string{"Instruction A", "Instruction B"}, + }, + { + name: "no matching tools", + activeTools: []string{"tool4"}, + expectedIDs: []string{}, + expectedInst: []string{}, + }, + { + name: "all tools match all rules", + activeTools: []string{"tool1", "tool2", "tool3"}, + expectedIDs: []string{"rule-a", "rule-b", "rule-c"}, + expectedInst: []string{"Instruction A", "Instruction B", "Instruction C"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ids := resolver.MatchingRuleIDs(tt.activeTools) + instructions := resolver.ResolveInstructions(tt.activeTools) + + if len(ids) != len(tt.expectedIDs) { + t.Errorf("Expected %d rule IDs, got %d: %v", len(tt.expectedIDs), len(ids), ids) + } + for i, id := range tt.expectedIDs { + if i >= len(ids) || ids[i] != id { + t.Errorf("Expected rule ID %s at position %d, got %v", id, i, ids) + } + } + + if len(instructions) != len(tt.expectedInst) { + t.Errorf("Expected %d instructions, got %d: %v", len(tt.expectedInst), len(instructions), instructions) + } + }) + } +} + +func TestInstructionResolver_SupersetShadowing(t *testing.T) { + // Rule with more tools (superset) shadows rules with fewer tools + resolver := NewInstructionResolver([]InstructionRule{ + NewInstructionRule("issues-read", "Read issues instruction", "get_issue", "list_issues"), + NewInstructionRule("issues-all", "All issues instruction", "get_issue", "list_issues", "create_issue"), + NewInstructionRule("create-only", "Create only instruction", "create_issue"), + }) + + tests := []struct { + name string + activeTools []string + expectedIDs []string + description string + }{ + { + name: "superset rule shadows subset rules", + activeTools: []string{"get_issue", "list_issues", "create_issue"}, + expectedIDs: []string{"issues-all"}, + description: "issues-all shadows issues-read (superset) and create-only (superset)", + }, + { + name: "no shadowing when superset rule doesn't match", + activeTools: []string{"get_issue", "list_issues"}, + expectedIDs: []string{"issues-read"}, + description: "issues-all doesn't match (missing create_issue), so issues-read is not shadowed", + }, + { + name: "single tool rule not shadowed when superset doesn't match", + activeTools: []string{"create_issue"}, + expectedIDs: []string{"create-only"}, + description: "Only create-only matches", + }, + { + name: "extra active tools don't affect shadowing", + activeTools: []string{"get_issue", "list_issues", "create_issue", "get_me", "other_tool"}, + expectedIDs: []string{"issues-all"}, + description: "Extra tools in activeTools don't prevent shadowing", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ids := resolver.MatchingRuleIDs(tt.activeTools) + + if len(ids) != len(tt.expectedIDs) { + t.Errorf("%s: Expected %d rule IDs %v, got %d: %v", + tt.description, len(tt.expectedIDs), tt.expectedIDs, len(ids), ids) + return + } + for i, id := range tt.expectedIDs { + if i >= len(ids) || ids[i] != id { + t.Errorf("%s: Expected rule ID %s at position %d, got %v", + tt.description, id, i, ids) + } + } + }) + } +} + +func TestInstructionResolver_PartialOverlapNoShadowing(t *testing.T) { + // Rules with partial overlap (neither is superset of other) should both apply + resolver := NewInstructionResolver([]InstructionRule{ + NewInstructionRule("rule-ab", "AB instruction", "tool_a", "tool_b"), + NewInstructionRule("rule-bc", "BC instruction", "tool_b", "tool_c"), + }) + + // When all three tools are active, both rules match + // Neither is a superset of the other, so neither is shadowed + ids := resolver.MatchingRuleIDs([]string{"tool_a", "tool_b", "tool_c"}) + + if len(ids) != 2 { + t.Errorf("Expected 2 rules (partial overlap, no shadowing), got %d: %v", len(ids), ids) + } +} + +func TestInstructionResolver_ComplexHierarchy(t *testing.T) { + // Create a hierarchy: rule1 ⊂ rule2 ⊂ rule3 + resolver := NewInstructionResolver([]InstructionRule{ + NewInstructionRule("level1", "Level 1 instruction", "t1"), + NewInstructionRule("level2", "Level 2 instruction", "t1", "t2"), + NewInstructionRule("level3", "Level 3 instruction", "t1", "t2", "t3"), + }) + + tests := []struct { + name string + activeTools []string + expectedIDs []string + }{ + { + name: "only level1 active", + activeTools: []string{"t1"}, + expectedIDs: []string{"level1"}, + }, + { + name: "level1 and level2 tools active", + activeTools: []string{"t1", "t2"}, + expectedIDs: []string{"level2"}, // shadows level1 + }, + { + name: "all three levels active", + activeTools: []string{"t1", "t2", "t3"}, + expectedIDs: []string{"level3"}, // shadows level1 and level2 + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ids := resolver.MatchingRuleIDs(tt.activeTools) + if len(ids) != len(tt.expectedIDs) { + t.Errorf("Expected %v, got %v", tt.expectedIDs, ids) + } + }) + } +} + +func TestInstructionResolver_EmptyRules(t *testing.T) { + resolver := NewInstructionResolver([]InstructionRule{}) + + ids := resolver.MatchingRuleIDs([]string{"tool1", "tool2"}) + if len(ids) != 0 { + t.Errorf("Expected no matches for empty resolver, got %v", ids) + } + + instructions := resolver.ResolveInstructions([]string{"tool1"}) + if len(instructions) != 0 { + t.Errorf("Expected no instructions for empty resolver, got %v", instructions) + } +} + +func TestInstructionResolver_EmptyActiveTools(t *testing.T) { + resolver := NewInstructionResolver([]InstructionRule{ + NewInstructionRule("rule1", "Instruction 1", "tool1"), + }) + + ids := resolver.MatchingRuleIDs([]string{}) + if len(ids) != 0 { + t.Errorf("Expected no matches for empty active tools, got %v", ids) + } +} + +// ============================================================================= +// END SPIKE TESTS +// =============================================================================