feat: specialist suite — configurable + custom review lenses (one consolidated comment)
Build & push image / build-and-push (push) Successful in 8s
Build & push image / build-and-push (push) Successful in 8s
Replace the single generic review with a suite of focused specialists, each its own review+recheck pass, merged into ONE comment (a collapsible section per lens, led by the worst verdict; the optional `improvements` lens never escalates it). - cmd/gadfly/specialists.go: built-in lenses + default suite (security, correctness, maintainability, performance, error-handling) + opt-in (tests, docs, conventions, improvements). Selection via GADFLY_SPECIALISTS (csv/"all"); custom defs via GADFLY_SPECIALIST_<NAME> env and a repo .gadfly.yml (specialists + define). Precedence: built-ins < file < env. Unknown names error but don't sink the run. - cmd/gadfly/consolidate.go: verdict parse + one-comment render. - main.go: loop specialists; per-lens failure is an inline notice, never fatal. Default timeout bumped to 600s (suite runs sequentially). - base system prompt trimmed to persona+tools+discipline+output; lens-specific focus is appended per specialist (semantic re-derivation discipline kept in base). - entrypoint default models -> single model (suite already gives breadth; cost ~= specialists × models × 2). Adds gopkg.in/yaml.v3. - docs/examples: README "Specialists" section, examples/.gadfly.yml, stub var, CLAUDE.md architecture/config. Dynamic `auto` selection is the planned next step. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
676c9d4f07
commit
7809d1b93d
@@ -0,0 +1,118 @@
|
||||
package main
|
||||
|
||||
import (
|
||||
"os"
|
||||
"path/filepath"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func names(specs []Specialist) []string {
|
||||
out := make([]string, len(specs))
|
||||
for i, s := range specs {
|
||||
out[i] = s.Name
|
||||
}
|
||||
return out
|
||||
}
|
||||
|
||||
func eq(a, b []string) bool {
|
||||
if len(a) != len(b) {
|
||||
return false
|
||||
}
|
||||
for i := range a {
|
||||
if a[i] != b[i] {
|
||||
return false
|
||||
}
|
||||
}
|
||||
return true
|
||||
}
|
||||
|
||||
func TestResolveSpecialists_DefaultSuite(t *testing.T) {
|
||||
t.Setenv("GADFLY_SPECIALISTS", "")
|
||||
specs, errs := resolveSpecialists(t.TempDir())
|
||||
if len(errs) != 0 {
|
||||
t.Fatalf("unexpected errors: %v", errs)
|
||||
}
|
||||
if !eq(names(specs), defaultSuite) {
|
||||
t.Errorf("default = %v, want %v", names(specs), defaultSuite)
|
||||
}
|
||||
}
|
||||
|
||||
func TestResolveSpecialists_EnvSelection(t *testing.T) {
|
||||
t.Setenv("GADFLY_SPECIALISTS", "security, tests")
|
||||
specs, errs := resolveSpecialists(t.TempDir())
|
||||
if len(errs) != 0 {
|
||||
t.Fatalf("unexpected errors: %v", errs)
|
||||
}
|
||||
if !eq(names(specs), []string{"security", "tests"}) {
|
||||
t.Errorf("got %v", names(specs))
|
||||
}
|
||||
}
|
||||
|
||||
func TestResolveSpecialists_UnknownNameErrors(t *testing.T) {
|
||||
t.Setenv("GADFLY_SPECIALISTS", "security,bogus")
|
||||
specs, errs := resolveSpecialists(t.TempDir())
|
||||
if len(errs) == 0 {
|
||||
t.Fatal("expected an error for unknown specialist")
|
||||
}
|
||||
if !eq(names(specs), []string{"security"}) {
|
||||
t.Errorf("valid ones should still resolve, got %v", names(specs))
|
||||
}
|
||||
}
|
||||
|
||||
func TestResolveSpecialists_EnvCustomDefinition(t *testing.T) {
|
||||
t.Setenv("GADFLY_SPECIALIST_MIGRATIONS", "Review DB migrations for destructive ops.")
|
||||
t.Setenv("GADFLY_SPECIALISTS", "migrations")
|
||||
specs, errs := resolveSpecialists(t.TempDir())
|
||||
if len(errs) != 0 {
|
||||
t.Fatalf("unexpected errors: %v", errs)
|
||||
}
|
||||
if len(specs) != 1 || specs[0].Name != "migrations" || specs[0].Focus == "" {
|
||||
t.Fatalf("custom specialist not registered: %+v", specs)
|
||||
}
|
||||
}
|
||||
|
||||
func TestResolveSpecialists_FileConfig(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
cfg := `specialists: [security, migrations]
|
||||
define:
|
||||
- name: migrations
|
||||
title: "DB migrations"
|
||||
focus: "Review schema migrations for destructive or unindexed changes."
|
||||
`
|
||||
if err := os.WriteFile(filepath.Join(dir, ".gadfly.yml"), []byte(cfg), 0o644); err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
t.Setenv("GADFLY_SPECIALISTS", "") // let the file drive selection
|
||||
specs, errs := resolveSpecialists(dir)
|
||||
if len(errs) != 0 {
|
||||
t.Fatalf("unexpected errors: %v", errs)
|
||||
}
|
||||
if !eq(names(specs), []string{"security", "migrations"}) {
|
||||
t.Errorf("got %v", names(specs))
|
||||
}
|
||||
}
|
||||
|
||||
func TestParseVerdictAndWorst(t *testing.T) {
|
||||
cases := map[string]verdict{
|
||||
"VERDICT: No material issues found.": verdictClean,
|
||||
"Minor issues\n- nit": verdictMinor,
|
||||
"**Blocking issues found**": verdictBlocking,
|
||||
"something unparseable": verdictUnknown,
|
||||
}
|
||||
for in, want := range cases {
|
||||
if got := parseVerdict(in); got != want {
|
||||
t.Errorf("parseVerdict(%q) = %v, want %v", in, got, want)
|
||||
}
|
||||
}
|
||||
results := []specialistResult{
|
||||
{spec: Specialist{Name: "security"}, verdict: verdictMinor},
|
||||
{spec: Specialist{Name: "correctness"}, verdict: verdictBlocking},
|
||||
{spec: Specialist{Name: "improvements"}, verdict: verdictBlocking}, // must not count
|
||||
}
|
||||
if w := worstVerdict(results[:2]); w != verdictBlocking {
|
||||
t.Errorf("worst = %v, want blocking", w)
|
||||
}
|
||||
if w := worstVerdict([]specialistResult{results[0], results[2]}); w != verdictMinor {
|
||||
t.Errorf("improvements should not escalate; worst = %v, want minor", w)
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user