diff --git a/docs/review-guidance.md b/docs/review-guidance.md index e232f5a9..62af16e9 100644 --- a/docs/review-guidance.md +++ b/docs/review-guidance.md @@ -46,6 +46,7 @@ artifact directory. The file `dossier/final/repo-guidance.md` records: - the trust-boundary note that PR-head guidance changes do not affect that run - whether the base branch guidance source was available, missing, unreadable, or invalid +- any declared agent that could not be honored and was skipped This file is intended for reviewers and operators who need to understand which repo guidance influenced a review without reading pipeline code. @@ -55,6 +56,13 @@ profile or flag agents. Unreadable or invalid repo guidance remains blocking because it indicates that maintainers attempted to declare authoritative review behavior that could not be honored. +A source can also load *partially*. A malformed agent or category — a bad field, +a missing `index.yaml` or `prompt.md` — disqualifies only itself; its siblings +still load and the source reports as available. A skipped agent is warned, not +blocking, and each skip is named in the dossier under "Guidance not honoured", +because the declaration that could not be honored is scoped to that agent rather +than to the repo. A source where *nothing* loads is still invalid. + Without an explicit positive `--max-agents`, all applicable repo-local reviewers and all matching `required_on_match` reviewers run, and the orchestrator may select up to five optional shared reviewers. A positive `--max-agents` is a hard diff --git a/internal/agents/agents.go b/internal/agents/agents.go index 5beb414d..18be8d6c 100644 --- a/internal/agents/agents.go +++ b/internal/agents/agents.go @@ -76,7 +76,27 @@ type SourceInfo struct { Status SourceStatus `json:"status"` Fingerprint string `json:"fingerprint,omitempty"` Warnings []string `json:"warnings,omitempty"` - Error string `json:"error,omitempty"` + // Skipped records definitions this source declared but could not honor, so a + // caller can branch on the degradation instead of parsing Warnings prose. + // A source with Skipped entries and Status available loaded only in part. + Skipped []SkippedDefinition `json:"skipped,omitempty"` + Error string `json:"error,omitempty"` +} + +// SkippedDefinition is one agent or category that was declared but not loaded. +// Agent is empty when the whole category was skipped. +type SkippedDefinition struct { + Category string `json:"category"` + Agent string `json:"agent,omitempty"` + Reason string `json:"reason"` +} + +// String renders the entry for display surfaces that take prose. +func (s SkippedDefinition) String() string { + if s.Agent == "" { + return fmt.Sprintf("skipped category %q: %s", s.Category, s.Reason) + } + return fmt.Sprintf("skipped agent %s/%s: %s", s.Category, s.Agent, s.Reason) } // Provenance identifies the winning source for one loaded agent. @@ -90,6 +110,9 @@ type Provenance struct { CanonicalPath string `json:"canonical_path,omitempty"` Fingerprint string `json:"fingerprint,omitempty"` Warnings []string `json:"warnings,omitempty"` + // Skipped mirrors SourceInfo.Skipped so a SourceInfo round-tripped through + // Provenance keeps the definitions this source could not honor. + Skipped []SkippedDefinition `json:"skipped,omitempty"` } // String returns the user-facing provenance label. @@ -124,6 +147,7 @@ func (p Provenance) SourceInfo() SourceInfo { Status: SourceStatusAvailable, Fingerprint: p.Fingerprint, Warnings: append([]string(nil), p.Warnings...), + Skipped: append([]SkippedDefinition(nil), p.Skipped...), } return info } @@ -449,8 +473,19 @@ func readFileAgent(agentPath string, category Category, pathName string, provena return newAgent(category, pathName, index, string(prompt), provenance), nil } -func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenance, allowSoftFailures bool) ([]Agent, SourceInfo, error) { +func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenance, allowSoftFailures bool) (loaded []Agent, info SourceInfo, err error) { repoSource := provenance.SourceInfo() + var skipped []SkippedDefinition + // Flush on every return, including ones added later that do not go through + // fail(). Dropping the skip list would reproduce, one layer down, the silent + // failure this loader exists to remove — so the invariant is enforced here + // rather than left to each exit remembering. + defer func() { + for _, entry := range skipped { + info.Skipped = append(info.Skipped, entry) + info.Warnings = append(info.Warnings, entry.String()) + } + }() fail := func(err error) ([]Agent, SourceInfo, error) { if classified, ok := classifyRepoCatalogError(repoSource, err); allowSoftFailures && ok { return nil, classified, nil @@ -493,29 +528,77 @@ func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenanc continue } if err := validateName("category", categoryName); err != nil { + if entry, ok := skipIfInvalid(err, SkippedDefinition{Category: categoryName}); ok { + skipped = append(skipped, entry) + continue + } return fail(err) } categoryPath := path.Join(repoAgentsRoot, categoryName) category, err := readRepoCategory(ctx, source.Reader, source.Ref, baseSHA, categoryPath, categoryName) if err != nil { + if entry, ok := skipIfInvalid(err, SkippedDefinition{Category: categoryName}); ok { + skipped = append(skipped, entry) + continue + } return fail(err) } - categoryAgents, err := readRepoAgents(ctx, source.Reader, source.Ref, baseSHA, categoryPath, category, provenance) + categoryAgents, agentSkips, err := readRepoAgents(ctx, source.Reader, source.Ref, baseSHA, categoryPath, category, provenance) + skipped = append(skipped, agentSkips...) if err != nil { return fail(err) } if len(categoryAgents) == 0 { - return fail(fmt.Errorf("%w: repo source %s category %q contains no agents", ErrInvalid, repoAgentsRoot, categoryName)) + // When the agents skipped themselves they already carry the actionable + // detail; a second entry for the same root cause makes the operator read + // the failure twice, once without it. A category that declared nothing + // is a different situation and says so. + if len(agentSkips) == 0 { + skipped = append(skipped, SkippedDefinition{Category: categoryName, Reason: "declares no agents"}) + } + continue } loadedAgent = true agents = append(agents, categoryAgents...) } + // Only a source with nothing usable is treated as invalid. One malformed + // definition disabling every other agent in the repo is what this avoids: + // the failure is indistinguishable from a review that ran and found + // problems, so it can sit unnoticed for a long time. if !loadedAgent { - return fail(fmt.Errorf("%w: repo source %s contains no agents", ErrInvalid, repoAgentsRoot)) + return fail(fmt.Errorf("%w: repo source %s contains no usable agents", ErrInvalid, repoAgentsRoot)) } return agents, repoSource, nil } +// skipIfInvalid decides whether err disqualifies just this definition or the whole +// source. Kept in one place so a future skip-safe error class, or a fix to the +// classification itself, cannot reach three of the four call sites and miss one. +func skipIfInvalid(err error, def SkippedDefinition) (SkippedDefinition, bool) { + if !errors.Is(err, ErrInvalid) { + return SkippedDefinition{}, false + } + def.Reason = err.Error() + return def, true +} + +// scopedToDefinition reclassifies a missing file *beneath* one agent or category +// directory as a malformed definition. Whether a failure disqualifies one agent or +// the whole source is a question of path scope, not error class: forgetting to commit +// prompt.md is as much a malformed agent as a bad field in index.yaml, and only the +// caller here knows the path is agent-scoped. Reader failures on the tree itself, and +// any non-NotFound transport error, are left alone and still fail the source. +func scopedToDefinition(err error, format string, args ...any) error { + if !errors.Is(err, gitprovider.ErrNotFound) { + return err + } + // Keep the provider error in the chain: these reasons reach the operator, and a + // NOT_FOUND raised for something other than an absent blob leaves nothing to + // diagnose with otherwise. Safe for classification — skipIfInvalid and + // classifyRepoCatalogError both test ErrInvalid first. + return fmt.Errorf("%w: %s: %w", ErrInvalid, fmt.Sprintf(format, args...), err) +} + func classifyRepoCatalogError(source SourceInfo, err error) (SourceInfo, bool) { switch { case errors.Is(err, ErrInvalid): @@ -539,7 +622,7 @@ func repoSourceError(source SourceInfo, status SourceStatus, err error) SourceIn func readRepoCategory(ctx context.Context, reader RepoReader, ref gitprovider.PRRef, gitRef, categoryPath, pathName string) (Category, error) { var index categoryYAML if err := decodeRepoYAML(ctx, reader, ref, gitRef, path.Join(categoryPath, "index.yaml"), &index); err != nil { - return Category{}, err + return Category{}, scopedToDefinition(err, "category %s is missing or unreadable index.yaml", pathName) } if err := validateMatchingName("category", pathName, index.Name); err != nil { return Category{}, err @@ -547,14 +630,26 @@ func readRepoCategory(ctx context.Context, reader RepoReader, ref gitprovider.PR return Category{Name: pathName, Description: index.Description, Owner: index.Owner}, nil } -func readRepoAgents(ctx context.Context, reader RepoReader, ref gitprovider.PRRef, gitRef, categoryPath string, category Category, provenance Provenance) ([]Agent, error) { +// readRepoAgents returns the agents it could load, plus a message per agent it +// skipped. A malformed definition disqualifies that agent only — an unusable +// agent is never selected, so skipping it is as safe as refusing the whole +// source and leaves the rest of the repo's guidance working. +// +// This deliberately diverges from readFileAgents, which still fails on the first +// bad agent. The difference is the trust boundary, not an oversight: profile +// sources are the operator's own configuration, where failing loudly is what they +// want, while repo sources are PR-adjacent content the operator does not own and +// cannot fix in the run. One contributor's malformed agent should not silently +// disarm review for everyone else's PRs. +func readRepoAgents(ctx context.Context, reader RepoReader, ref gitprovider.PRRef, gitRef, categoryPath string, category Category, provenance Provenance) ([]Agent, []SkippedDefinition, error) { entries, err := reader.ListTreeAtRef(ctx, ref, gitRef, categoryPath) if err != nil { - return nil, err + return nil, nil, err } sortTreeEntries(entries) var agents []Agent + var skipped []SkippedDefinition for _, entry := range entries { if entry.Type != "tree" { continue @@ -564,22 +659,30 @@ func readRepoAgents(ctx context.Context, reader RepoReader, ref gitprovider.PRRe continue } if err := validateName("agent", agentName); err != nil { - return nil, err + if entry, ok := skipIfInvalid(err, SkippedDefinition{Category: category.Name, Agent: agentName}); ok { + skipped = append(skipped, entry) + continue + } + return nil, skipped, err } agentPath := path.Join(categoryPath, agentName) agent, err := readRepoAgent(ctx, reader, ref, gitRef, agentPath, category, agentName, provenance) if err != nil { - return nil, err + if entry, ok := skipIfInvalid(err, SkippedDefinition{Category: category.Name, Agent: agentName}); ok { + skipped = append(skipped, entry) + continue + } + return nil, skipped, err } agents = append(agents, agent) } - return agents, nil + return agents, skipped, nil } func readRepoAgent(ctx context.Context, reader RepoReader, ref gitprovider.PRRef, gitRef, agentPath string, category Category, pathName string, provenance Provenance) (Agent, error) { var index agentYAML if err := decodeRepoYAML(ctx, reader, ref, gitRef, path.Join(agentPath, "index.yaml"), &index); err != nil { - return Agent{}, err + return Agent{}, scopedToDefinition(err, "agent %s:%s is missing or unreadable index.yaml", category.Name, pathName) } if err := validateMatchingName("agent", pathName, index.Name); err != nil { return Agent{}, err @@ -589,7 +692,7 @@ func readRepoAgent(ctx context.Context, reader RepoReader, ref gitprovider.PRRef } prompt, err := reader.GetFileAtRef(ctx, ref, gitRef, path.Join(agentPath, "prompt.md")) if err != nil { - return Agent{}, err + return Agent{}, scopedToDefinition(err, "agent %s:%s is missing prompt.md", category.Name, pathName) } return newAgent(category, pathName, index, string(prompt), provenance), nil } @@ -639,7 +742,7 @@ func validateAgentYAML(categoryName, agentName string, index agentYAML) error { } for _, pattern := range index.FileGlobs { if _, err := glob.Compile(pattern, '/'); err != nil { - return fmt.Errorf("%w: agent %s:%s file_glob %q is invalid: %w", ErrInvalid, categoryName, agentName, pattern, err) + return fmt.Errorf("%w: agent %s:%s file_glob %q is invalid: %w", ErrInvalid, categoryName, agentName, pattern, err) } } } @@ -800,6 +903,7 @@ func provenanceFromSource(source SourceInfo) Provenance { CanonicalPath: source.CanonicalPath, Fingerprint: source.Fingerprint, Warnings: append([]string(nil), source.Warnings...), + Skipped: append([]SkippedDefinition(nil), source.Skipped...), } } diff --git a/internal/agents/agents_test.go b/internal/agents/agents_test.go index 08ceea91..1fd2bb36 100644 --- a/internal/agents/agents_test.go +++ b/internal/agents/agents_test.go @@ -505,7 +505,10 @@ func TestMissingRepoAgentsTreeIsEmptySource(t *testing.T) { } } -func TestRepoLoadClassifiesMissingNestedFilesAsUnreadableSource(t *testing.T) { +// A file missing beneath an agent directory is now a malformed agent, not an +// unreadable source — see scopedToDefinition. With that agent skipped and no +// sibling to fall back on, the source has nothing usable and is invalid. +func TestRepoLoadClassifiesMissingNestedFilesAsInvalidSource(t *testing.T) { ref := testPRRef() pr := testPR("base-sha", "head-sha") reader := newRepoReader() @@ -522,8 +525,8 @@ func TestRepoLoadClassifiesMissingNestedFilesAsUnreadableSource(t *testing.T) { if len(catalog.Agents) != 0 { t.Fatalf("agents = %#v, want none", catalog.Agents) } - if len(catalog.Sources) != 1 || catalog.Sources[0].Status != SourceStatusUnreadable || !catalog.Sources[0].Present || catalog.Sources[0].Error == "" { - t.Fatalf("sources = %#v, want unreadable repo source", catalog.Sources) + if len(catalog.Sources) != 1 || catalog.Sources[0].Status != SourceStatusInvalid || !catalog.Sources[0].Present || catalog.Sources[0].Error == "" { + t.Fatalf("sources = %#v, want invalid repo source", catalog.Sources) } } @@ -547,6 +550,285 @@ func TestRepoLoadClassifiesInvalidCatalogAsInvalidSource(t *testing.T) { } } +// The real-world case this guards: one agent in a repo set `model:` instead of +// `model_tier:`, which disqualified every other agent in that repo for two +// months. The malformed agent is skipped; its siblings still load. +func TestRepoLoadSkipsMalformedAgentAndKeepsSiblings(t *testing.T) { + ref := testPRRef() + pr := testPR("base-sha", "head-sha") + reader := newRepoReader() + categoryPath := repoAgentsRoot + "/cat" + reader.addTree(ref, pr.Base.SHA, repoAgentsRoot, gitprovider.TreeEntry{Path: "cat", Type: "tree"}) + reader.addFile(ref, pr.Base.SHA, categoryPath+"/index.yaml", []byte("name: cat\ndescription: cat category\nowner: owner\n")) + reader.addTree(ref, pr.Base.SHA, categoryPath, gitprovider.TreeEntry{Path: categoryPath + "/good", Type: "tree"}) + reader.addTree(ref, pr.Base.SHA, categoryPath, gitprovider.TreeEntry{Path: categoryPath + "/bad", Type: "tree"}) + reader.addFile(ref, pr.Base.SHA, categoryPath+"/good/index.yaml", []byte(agentIndexYAML("good", "desc", "medium", "medium"))) + reader.addFile(ref, pr.Base.SHA, categoryPath+"/good/prompt.md", []byte("good prompt")) + // Unknown field: the exact shape that broke two repos. + reader.addFile(ref, pr.Base.SHA, categoryPath+"/bad/index.yaml", []byte("name: bad\ndescription: d\nmodel: sonnet\neffort: medium\n")) + reader.addFile(ref, pr.Base.SHA, categoryPath+"/bad/prompt.md", []byte("bad prompt")) + + catalog, err := Load(context.Background(), LoadOptions{Repo: &RepoSource{Reader: reader, Ref: ref, PR: pr}, AllowSoftRepoFailures: true}) + if err != nil { + t.Fatalf("Load: %v", err) + } + if len(catalog.Agents) != 1 || catalog.Agents[0].Name != "good" { + t.Fatalf("agents = %#v, want only the good agent", catalog.Agents) + } + if len(catalog.Sources) != 1 || catalog.Sources[0].Status == SourceStatusInvalid { + t.Fatalf("sources = %#v, want a usable repo source", catalog.Sources) + } + // The skip must be visible; a silent one is how this went unnoticed. + if !warningsMention(catalog.Sources[0].Warnings, "bad") { + t.Fatalf("warnings = %#v, want one naming the skipped agent", catalog.Sources[0].Warnings) + } +} + +// Skipping is per-agent, not a blanket pass: a source where nothing loads is +// still invalid, so guidance never silently degrades to nothing. +func TestRepoLoadInvalidWhenEveryAgentIsMalformed(t *testing.T) { + ref := testPRRef() + pr := testPR("base-sha", "head-sha") + reader := newRepoReader() + categoryPath := repoAgentsRoot + "/cat" + reader.addTree(ref, pr.Base.SHA, repoAgentsRoot, gitprovider.TreeEntry{Path: "cat", Type: "tree"}) + reader.addFile(ref, pr.Base.SHA, categoryPath+"/index.yaml", []byte("name: cat\ndescription: cat category\nowner: owner\n")) + reader.addTree(ref, pr.Base.SHA, categoryPath, gitprovider.TreeEntry{Path: categoryPath + "/bad", Type: "tree"}) + reader.addFile(ref, pr.Base.SHA, categoryPath+"/bad/index.yaml", []byte("name: bad\ndescription: d\nmodel: sonnet\neffort: medium\n")) + reader.addFile(ref, pr.Base.SHA, categoryPath+"/bad/prompt.md", []byte("bad prompt")) + + catalog, err := Load(context.Background(), LoadOptions{Repo: &RepoSource{Reader: reader, Ref: ref, PR: pr}, AllowSoftRepoFailures: true}) + if err != nil { + t.Fatalf("Load: %v", err) + } + if len(catalog.Agents) != 0 { + t.Fatalf("agents = %#v, want none", catalog.Agents) + } + if len(catalog.Sources) != 1 || catalog.Sources[0].Status != SourceStatusInvalid { + t.Fatalf("sources = %#v, want invalid repo source", catalog.Sources) + } +} + +// Category-level skips were untested: three branches in loadRepoSource skip a +// category, and a regression turning one back into a whole-source failure would +// have passed CI. +func TestRepoLoadSkipsMalformedCategoryAndKeepsSiblingCategories(t *testing.T) { + ref := testPRRef() + pr := testPR("base-sha", "head-sha") + reader := newRepoReader() + goodPath := repoAgentsRoot + "/good-cat" + badPath := repoAgentsRoot + "/bad-cat" + reader.addTree(ref, pr.Base.SHA, repoAgentsRoot, gitprovider.TreeEntry{Path: "good-cat", Type: "tree"}) + reader.addTree(ref, pr.Base.SHA, repoAgentsRoot, gitprovider.TreeEntry{Path: "bad-cat", Type: "tree"}) + + reader.addFile(ref, pr.Base.SHA, goodPath+"/index.yaml", []byte("name: good-cat\ndescription: c\nowner: owner\n")) + reader.addTree(ref, pr.Base.SHA, goodPath, gitprovider.TreeEntry{Path: goodPath + "/agent", Type: "tree"}) + reader.addFile(ref, pr.Base.SHA, goodPath+"/agent/index.yaml", []byte(agentIndexYAML("agent", "desc", "medium", "medium"))) + reader.addFile(ref, pr.Base.SHA, goodPath+"/agent/prompt.md", []byte("prompt")) + + // Category index naming a different category: malformed at the category level. + reader.addFile(ref, pr.Base.SHA, badPath+"/index.yaml", []byte("name: mismatched\ndescription: c\nowner: owner\n")) + + catalog, err := Load(context.Background(), LoadOptions{Repo: &RepoSource{Reader: reader, Ref: ref, PR: pr}, AllowSoftRepoFailures: true}) + if err != nil { + t.Fatalf("Load: %v", err) + } + if len(catalog.Agents) != 1 || catalog.Agents[0].Category.Name != "good-cat" { + t.Fatalf("agents = %#v, want the sibling category's agent", catalog.Agents) + } + if len(catalog.Sources) != 1 || catalog.Sources[0].Status == SourceStatusInvalid { + t.Fatalf("sources = %#v, want a usable repo source", catalog.Sources) + } + if !warningsMention(catalog.Sources[0].Warnings, "bad-cat") { + t.Fatalf("warnings = %#v, want one naming the skipped category", catalog.Sources[0].Warnings) + } +} + +// A file missing inside one agent directory is a malformed agent, not an +// unreadable source: forgetting to commit prompt.md must not disarm the repo. +func TestRepoLoadSkipsAgentMissingPromptAndKeepsSiblings(t *testing.T) { + ref := testPRRef() + pr := testPR("base-sha", "head-sha") + reader := newRepoReader() + categoryPath := repoAgentsRoot + "/cat" + reader.addTree(ref, pr.Base.SHA, repoAgentsRoot, gitprovider.TreeEntry{Path: "cat", Type: "tree"}) + reader.addFile(ref, pr.Base.SHA, categoryPath+"/index.yaml", []byte("name: cat\ndescription: c\nowner: owner\n")) + reader.addTree(ref, pr.Base.SHA, categoryPath, gitprovider.TreeEntry{Path: categoryPath + "/good", Type: "tree"}) + reader.addTree(ref, pr.Base.SHA, categoryPath, gitprovider.TreeEntry{Path: categoryPath + "/no-prompt", Type: "tree"}) + reader.addFile(ref, pr.Base.SHA, categoryPath+"/good/index.yaml", []byte(agentIndexYAML("good", "desc", "medium", "medium"))) + reader.addFile(ref, pr.Base.SHA, categoryPath+"/good/prompt.md", []byte("prompt")) + // index.yaml present and valid; prompt.md never committed. + reader.addFile(ref, pr.Base.SHA, categoryPath+"/no-prompt/index.yaml", []byte(agentIndexYAML("no-prompt", "desc", "medium", "medium"))) + + catalog, err := Load(context.Background(), LoadOptions{Repo: &RepoSource{Reader: reader, Ref: ref, PR: pr}, AllowSoftRepoFailures: true}) + if err != nil { + t.Fatalf("Load: %v", err) + } + if len(catalog.Agents) != 1 || catalog.Agents[0].Name != "good" { + t.Fatalf("agents = %#v, want only the complete agent", catalog.Agents) + } + if len(catalog.Sources) != 1 || catalog.Sources[0].Status != SourceStatusAvailable { + t.Fatalf("sources = %#v, want an available repo source", catalog.Sources) + } + if !warningsMention(catalog.Sources[0].Warnings, "no-prompt") { + t.Fatalf("warnings = %#v, want one naming the skipped agent", catalog.Sources[0].Warnings) + } +} + +// An unsafe category tree name is the third category-level skip branch; without a +// healthy sibling it was only ever seen as a whole-source failure. +func TestRepoLoadSkipsUnsafeCategoryNameAndKeepsSiblingCategories(t *testing.T) { + ref := testPRRef() + pr := testPR("base-sha", "head-sha") + reader := newRepoReader() + goodPath := repoAgentsRoot + "/good-cat" + reader.addTree(ref, pr.Base.SHA, repoAgentsRoot, gitprovider.TreeEntry{Path: "good-cat", Type: "tree"}) + reader.addTree(ref, pr.Base.SHA, repoAgentsRoot, gitprovider.TreeEntry{Path: "..", Type: "tree"}) + reader.addFile(ref, pr.Base.SHA, goodPath+"/index.yaml", []byte("name: good-cat\ndescription: c\nowner: owner\n")) + reader.addTree(ref, pr.Base.SHA, goodPath, gitprovider.TreeEntry{Path: goodPath + "/agent", Type: "tree"}) + reader.addFile(ref, pr.Base.SHA, goodPath+"/agent/index.yaml", []byte(agentIndexYAML("agent", "desc", "medium", "medium"))) + reader.addFile(ref, pr.Base.SHA, goodPath+"/agent/prompt.md", []byte("prompt")) + + catalog, err := Load(context.Background(), LoadOptions{Repo: &RepoSource{Reader: reader, Ref: ref, PR: pr}, AllowSoftRepoFailures: true}) + if err != nil { + t.Fatalf("Load: %v", err) + } + if len(catalog.Agents) != 1 || catalog.Agents[0].Category.Name != "good-cat" { + t.Fatalf("agents = %#v, want the sibling category's agent", catalog.Agents) + } + if catalog.Sources[0].Status == SourceStatusInvalid { + t.Fatalf("sources = %#v, want a usable repo source", catalog.Sources) + } +} + +// A category that parses but whose only agent is malformed is the third branch: +// with a healthy sibling category the source must stay usable. +func TestRepoLoadSkipsCategoryWithNoUsableAgentsAndKeepsSiblings(t *testing.T) { + ref := testPRRef() + pr := testPR("base-sha", "head-sha") + reader := newRepoReader() + goodPath := repoAgentsRoot + "/good-cat" + emptyPath := repoAgentsRoot + "/empty-cat" + reader.addTree(ref, pr.Base.SHA, repoAgentsRoot, gitprovider.TreeEntry{Path: "good-cat", Type: "tree"}) + reader.addTree(ref, pr.Base.SHA, repoAgentsRoot, gitprovider.TreeEntry{Path: "empty-cat", Type: "tree"}) + + reader.addFile(ref, pr.Base.SHA, goodPath+"/index.yaml", []byte("name: good-cat\ndescription: c\nowner: owner\n")) + reader.addTree(ref, pr.Base.SHA, goodPath, gitprovider.TreeEntry{Path: goodPath + "/agent", Type: "tree"}) + reader.addFile(ref, pr.Base.SHA, goodPath+"/agent/index.yaml", []byte(agentIndexYAML("agent", "desc", "medium", "medium"))) + reader.addFile(ref, pr.Base.SHA, goodPath+"/agent/prompt.md", []byte("prompt")) + + reader.addFile(ref, pr.Base.SHA, emptyPath+"/index.yaml", []byte("name: empty-cat\ndescription: c\nowner: owner\n")) + reader.addTree(ref, pr.Base.SHA, emptyPath, gitprovider.TreeEntry{Path: emptyPath + "/bad", Type: "tree"}) + reader.addFile(ref, pr.Base.SHA, emptyPath+"/bad/index.yaml", []byte("name: bad\ndescription: d\nmodel: sonnet\neffort: medium\n")) + reader.addFile(ref, pr.Base.SHA, emptyPath+"/bad/prompt.md", []byte("p")) + + catalog, err := Load(context.Background(), LoadOptions{Repo: &RepoSource{Reader: reader, Ref: ref, PR: pr}, AllowSoftRepoFailures: true}) + if err != nil { + t.Fatalf("Load: %v", err) + } + if len(catalog.Agents) != 1 || catalog.Agents[0].Category.Name != "good-cat" { + t.Fatalf("agents = %#v, want the sibling category's agent", catalog.Agents) + } + // The malformed agent carries the actionable detail; a second category-level + // entry for the same root cause would make the operator read it twice, once + // without that detail. + if !skippedMentions(catalog.Sources[0].Skipped, "empty-cat", "bad") { + t.Fatalf("skipped = %#v, want the malformed agent recorded", catalog.Sources[0].Skipped) + } + for _, entry := range catalog.Sources[0].Skipped { + if entry.Category == "empty-cat" && entry.Agent == "" { + t.Fatalf("skipped = %#v, want no redundant category entry alongside the agent entry", catalog.Sources[0].Skipped) + } + } +} + +// A category directory that declares no agents at all is a different situation +// from one whose agents are malformed, and reports as such. +func TestRepoLoadRecordsCategoryDeclaringNoAgents(t *testing.T) { + ref := testPRRef() + pr := testPR("base-sha", "head-sha") + reader := newRepoReader() + goodPath := repoAgentsRoot + "/good-cat" + emptyPath := repoAgentsRoot + "/empty-cat" + reader.addTree(ref, pr.Base.SHA, repoAgentsRoot, gitprovider.TreeEntry{Path: "good-cat", Type: "tree"}) + reader.addTree(ref, pr.Base.SHA, repoAgentsRoot, gitprovider.TreeEntry{Path: "empty-cat", Type: "tree"}) + + reader.addFile(ref, pr.Base.SHA, goodPath+"/index.yaml", []byte("name: good-cat\ndescription: c\nowner: owner\n")) + reader.addTree(ref, pr.Base.SHA, goodPath, gitprovider.TreeEntry{Path: goodPath + "/agent", Type: "tree"}) + reader.addFile(ref, pr.Base.SHA, goodPath+"/agent/index.yaml", []byte(agentIndexYAML("agent", "desc", "medium", "medium"))) + reader.addFile(ref, pr.Base.SHA, goodPath+"/agent/prompt.md", []byte("prompt")) + + // Category index only; the category tree exists but declares no agent dirs. + reader.addFile(ref, pr.Base.SHA, emptyPath+"/index.yaml", []byte("name: empty-cat\ndescription: c\nowner: owner\n")) + reader.trees[repoFileSelector{ref: ref, gitRef: pr.Base.SHA, path: emptyPath}] = nil + + catalog, err := Load(context.Background(), LoadOptions{Repo: &RepoSource{Reader: reader, Ref: ref, PR: pr}, AllowSoftRepoFailures: true}) + if err != nil { + t.Fatalf("Load: %v", err) + } + if len(catalog.Agents) != 1 { + t.Fatalf("agents = %#v, want the sibling category's agent", catalog.Agents) + } + if !skippedMentions(catalog.Sources[0].Skipped, "empty-cat", "declares no agents") { + t.Fatalf("skipped = %#v, want the empty category reported as declaring nothing", catalog.Sources[0].Skipped) + } +} + +// The flush-on-every-exit invariant: a skip recorded before a later hard failure +// must survive into the returned SourceInfo. This was a real bug once. +func TestRepoLoadKeepsSkipsWhenALaterCategoryFailsHard(t *testing.T) { + ref := testPRRef() + pr := testPR("base-sha", "head-sha") + reader := newRepoReader() + // "a-cat" sorts first and contributes a skip; "z-cat" then fails hard because + // its category tree is never registered, so ListTreeAtRef returns ErrNotFound. + aPath := repoAgentsRoot + "/a-cat" + zPath := repoAgentsRoot + "/z-cat" + reader.addTree(ref, pr.Base.SHA, repoAgentsRoot, gitprovider.TreeEntry{Path: "a-cat", Type: "tree"}) + reader.addTree(ref, pr.Base.SHA, repoAgentsRoot, gitprovider.TreeEntry{Path: "z-cat", Type: "tree"}) + + reader.addFile(ref, pr.Base.SHA, aPath+"/index.yaml", []byte("name: a-cat\ndescription: c\nowner: owner\n")) + reader.addTree(ref, pr.Base.SHA, aPath, gitprovider.TreeEntry{Path: aPath + "/good", Type: "tree"}) + reader.addTree(ref, pr.Base.SHA, aPath, gitprovider.TreeEntry{Path: aPath + "/bad", Type: "tree"}) + reader.addFile(ref, pr.Base.SHA, aPath+"/good/index.yaml", []byte(agentIndexYAML("good", "desc", "medium", "medium"))) + reader.addFile(ref, pr.Base.SHA, aPath+"/good/prompt.md", []byte("prompt")) + reader.addFile(ref, pr.Base.SHA, aPath+"/bad/index.yaml", []byte("name: bad\ndescription: d\nmodel: sonnet\neffort: medium\n")) + reader.addFile(ref, pr.Base.SHA, aPath+"/bad/prompt.md", []byte("p")) + + reader.addFile(ref, pr.Base.SHA, zPath+"/index.yaml", []byte("name: z-cat\ndescription: c\nowner: owner\n")) + // No tree registered for zPath: readRepoAgents' ListTreeAtRef fails, and that + // is not routed through scopedToDefinition, so it reaches fail() directly. + + catalog, err := Load(context.Background(), LoadOptions{Repo: &RepoSource{Reader: reader, Ref: ref, PR: pr}, AllowSoftRepoFailures: true}) + if err != nil { + t.Fatalf("Load: %v", err) + } + if !skippedMentions(catalog.Sources[0].Skipped, "a-cat", "bad") { + t.Fatalf("skipped = %#v, want the earlier skip to survive the later hard failure", catalog.Sources[0].Skipped) + } +} + +func skippedMentions(skipped []SkippedDefinition, category, needle string) bool { + for _, entry := range skipped { + if entry.Category != category { + continue + } + if strings.Contains(entry.String(), needle) { + return true + } + } + return false +} + +func warningsMention(warnings []string, needle string) bool { + for _, w := range warnings { + if strings.Contains(w, needle) { + return true + } + } + return false +} + func TestLoadRejectsEmptyFilesystemSource(t *testing.T) { _, err := Load(context.Background(), LoadOptions{ProfileDirs: []string{""}}) if !errors.Is(err, ErrInvalid) { diff --git a/internal/dossier/dossier.go b/internal/dossier/dossier.go index c0785777..deb06383 100644 --- a/internal/dossier/dossier.go +++ b/internal/dossier/dossier.go @@ -1149,6 +1149,14 @@ func renderDossierRepoGuidance(repo dossierRepoContextArtifact) string { out.WriteString("\n") } } + // A partially loaded source still reports "available", so without this the + // run reads as if every declared agent had been honored. Skips belong where + // the run is observed, not only in the artifact. + for _, entry := range source.Skipped { + out.WriteString("Guidance not honored: ") + out.WriteString(entry.String()) + out.WriteString("\n") + } } if note := strings.TrimSpace(repo.RepoInfo.TrustNote()); note != "" { out.WriteString("\n") @@ -1187,6 +1195,13 @@ func RepoGuidanceUnavailableReason(sources []agents.SourceInfo) string { if msg := strings.TrimSpace(source.Error); msg != "" { reason += " Source detail: " + msg } + // The generic "contains no usable agents" error says nothing about which + // definitions failed or why. This is the blocking case, so the body that gets + // posted is exactly where that detail is needed — otherwise the operator has + // to re-run cr agents to learn what to fix. + for _, entry := range source.Skipped { + reason += " " + entry.String() + "." + } return reason } diff --git a/internal/pipeline/pipeline_test.go b/internal/pipeline/pipeline_test.go index 224adf70..0ac0685b 100644 --- a/internal/pipeline/pipeline_test.go +++ b/internal/pipeline/pipeline_test.go @@ -1947,7 +1947,10 @@ func TestDryRunInvalidOrUnreadableRepoGuidanceForcesRequestChangesWithoutReviewe wantState agents.SourceStatus }{ { - name: "unreadable", + // A file missing beneath an agent directory is a malformed agent, not an + // unreadable source. The agent is skipped; with no sibling to fall back + // on the source has nothing usable, so it is invalid — and still blocking. + name: "agent missing prompt, nothing else to load", setupRepo: func(t *testing.T, provider *readOnlyProvider) { t.Helper() removeRepoAgentFixture(provider) @@ -1957,8 +1960,8 @@ func TestDryRunInvalidOrUnreadableRepoGuidanceForcesRequestChangesWithoutReviewe provider.trees[fileKey{gitRef: provider.pr.Base.SHA, path: categoryPath}] = []gitprovider.TreeEntry{{Path: categoryPath + "/agent", Type: "tree"}} provider.files[fileKey{gitRef: provider.pr.Base.SHA, path: categoryPath + "/agent/index.yaml"}] = []byte("name: agent\ndescription: desc\nmodel_tier: medium\neffort: medium\n") }, - wantText: "Base branch `.codereview/agents/` could not be read as trusted review guidance.", - wantState: agents.SourceStatusUnreadable, + wantText: "Base branch `.codereview/agents/` was invalid and could not be used as trusted review guidance.", + wantState: agents.SourceStatusInvalid, }, { name: "invalid",