From 9d38769242ef5204f542c86e86ef34ec2fcfad40 Mon Sep 17 00:00:00 2001 From: piekstra Date: Wed, 5 Aug 2026 17:13:39 -0400 Subject: [PATCH 1/5] Skip malformed repo agents instead of rejecting the whole source MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A single unparseable index.yaml under .codereview/agents/ disqualified every agent in the repo. The review then came back request_changes with zero findings, which is visually indistinguishable from a reviewer that ran and objected — so it went unnoticed in two SignalFT repos for about two months, one of them the repo where that agent was added. An agent that fails to load is never selected, so skipping it is exactly as safe as refusing the entire source, and it leaves the rest of the repo's guidance working. Category-level failures are skipped the same way. A source where nothing loads is still invalid, so guidance cannot silently degrade to nothing. Skips are recorded on SourceInfo.Warnings naming each agent, because a silent skip is how the original problem hid. --- internal/agents/agents.go | 45 ++++++++++++++++++----- internal/agents/agents_test.go | 65 ++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 8 deletions(-) diff --git a/internal/agents/agents.go b/internal/agents/agents.go index 5beb414d..7c7983d8 100644 --- a/internal/agents/agents.go +++ b/internal/agents/agents.go @@ -483,6 +483,7 @@ func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenanc sortTreeEntries(rootEntries) var agents []Agent + var skipped []string loadedAgent := false for _, entry := range rootEntries { if entry.Type != "tree" { @@ -493,25 +494,40 @@ func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenanc continue } if err := validateName("category", categoryName); err != nil { + if errors.Is(err, ErrInvalid) { + skipped = append(skipped, fmt.Sprintf("skipped category %q: %v", categoryName, err)) + continue + } return fail(err) } categoryPath := path.Join(repoAgentsRoot, categoryName) category, err := readRepoCategory(ctx, source.Reader, source.Ref, baseSHA, categoryPath, categoryName) if err != nil { + if errors.Is(err, ErrInvalid) { + skipped = append(skipped, fmt.Sprintf("skipped category %q: %v", categoryName, err)) + 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)) + skipped = append(skipped, fmt.Sprintf("skipped category %q: no usable agents", categoryName)) + continue } loadedAgent = true agents = append(agents, categoryAgents...) } + repoSource.Warnings = append(repoSource.Warnings, skipped...) + // 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 } @@ -547,14 +563,19 @@ 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. +func readRepoAgents(ctx context.Context, reader RepoReader, ref gitprovider.PRRef, gitRef, categoryPath string, category Category, provenance Provenance) ([]Agent, []string, 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 []string for _, entry := range entries { if entry.Type != "tree" { continue @@ -564,16 +585,24 @@ func readRepoAgents(ctx context.Context, reader RepoReader, ref gitprovider.PRRe continue } if err := validateName("agent", agentName); err != nil { - return nil, err + if errors.Is(err, ErrInvalid) { + skipped = append(skipped, fmt.Sprintf("skipped agent %s/%s: %v", category.Name, agentName, err)) + 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 errors.Is(err, ErrInvalid) { + skipped = append(skipped, fmt.Sprintf("skipped agent %s/%s: %v", category.Name, agentName, err)) + 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) { diff --git a/internal/agents/agents_test.go b/internal/agents/agents_test.go index 08ceea91..10e708ec 100644 --- a/internal/agents/agents_test.go +++ b/internal/agents/agents_test.go @@ -547,6 +547,71 @@ 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. + var mentionsBad bool + for _, w := range catalog.Sources[0].Warnings { + if strings.Contains(w, "bad") { + mentionsBad = true + } + } + if !mentionsBad { + 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) + } +} + func TestLoadRejectsEmptyFilesystemSource(t *testing.T) { _, err := Load(context.Background(), LoadOptions{ProfileDirs: []string{""}}) if !errors.Is(err, ErrInvalid) { From 271a82a4eeccd122ce449d154319a7d0d8d0b17c Mon Sep 17 00:00:00 2001 From: piekstra Date: Wed, 5 Aug 2026 17:59:06 -0400 Subject: [PATCH 2/5] Address review: scope the skip by path, and make it visible MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five findings from review, all taken. Skips were only flushed into SourceInfo.Warnings after the category loop, so any early return dropped the ones already collected — the same silent failure this change exists to remove, one layer down. fail() now flushes on every exit. The skip predicate keyed on error class (ErrInvalid) rather than on the axis that decides blast radius: whether the failure is scoped to one agent or to the whole source. A missing prompt.md surfaced as ErrNotFound and still disqualified every other agent in the repo, which is at least as easy a mistake to make as the field typo that prompted this. scopedToDefinition now reclassifies a missing file beneath an agent or category directory as a malformed definition. Reader failures on the tree itself, and non-NotFound transport errors, still fail the source. That reclassification is a deliberate contract change: a missing nested file was "unreadable" and is now "invalid" when nothing else loads. Still blocking, so behavior for that case is unchanged; two tests that encoded the old label are updated. SourceInfo.Warnings was the wrong channel on its own — nothing in a review run read it, so a repo whose reviewer silently stopped loading still produced a normal-looking review. Skips now render in the dossier under "Guidance not honoured", and docs/review-guidance.md describes the partially-loaded case instead of only available/missing/unreadable/invalid. readFileAgents still fails on the first bad agent. That divergence is the trust boundary, not an oversight: profile sources are the operator's own configuration where failing loudly is correct, repo sources are PR-adjacent content they do not own. Now stated at the call site. Adds the category-level and missing-prompt cases, which had no coverage. --- docs/review-guidance.md | 8 +++ internal/agents/agents.go | 34 +++++++++-- internal/agents/agents_test.go | 92 ++++++++++++++++++++++++++---- internal/dossier/dossier.go | 10 ++++ internal/pipeline/pipeline_test.go | 9 ++- 5 files changed, 135 insertions(+), 18 deletions(-) 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 7c7983d8..63b71876 100644 --- a/internal/agents/agents.go +++ b/internal/agents/agents.go @@ -451,7 +451,12 @@ func readFileAgent(agentPath string, category Category, pathName string, provena func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenance, allowSoftFailures bool) ([]Agent, SourceInfo, error) { repoSource := provenance.SourceInfo() + var skipped []string + // Every exit flushes the skips collected so far. An early return that dropped + // them would reproduce, one layer down, the silent failure this loader exists + // to remove: a real problem hiding because nothing recorded it. fail := func(err error) ([]Agent, SourceInfo, error) { + repoSource.Warnings = append(repoSource.Warnings, skipped...) if classified, ok := classifyRepoCatalogError(repoSource, err); allowSoftFailures && ok { return nil, classified, nil } @@ -483,7 +488,6 @@ func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenanc sortTreeEntries(rootEntries) var agents []Agent - var skipped []string loadedAgent := false for _, entry := range rootEntries { if entry.Type != "tree" { @@ -521,7 +525,6 @@ func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenanc loadedAgent = true agents = append(agents, categoryAgents...) } - repoSource.Warnings = append(repoSource.Warnings, skipped...) // 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 @@ -529,9 +532,23 @@ func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenanc if !loadedAgent { return fail(fmt.Errorf("%w: repo source %s contains no usable agents", ErrInvalid, repoAgentsRoot)) } + repoSource.Warnings = append(repoSource.Warnings, skipped...) return agents, repoSource, nil } +// 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 + } + return fmt.Errorf("%w: %s", ErrInvalid, fmt.Sprintf(format, args...)) +} + func classifyRepoCatalogError(source SourceInfo, err error) (SourceInfo, bool) { switch { case errors.Is(err, ErrInvalid): @@ -555,7 +572,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 @@ -567,6 +584,13 @@ func readRepoCategory(ctx context.Context, reader RepoReader, ref gitprovider.PR // 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, []string, error) { entries, err := reader.ListTreeAtRef(ctx, ref, gitRef, categoryPath) if err != nil { @@ -608,7 +632,7 @@ func readRepoAgents(ctx context.Context, reader RepoReader, ref gitprovider.PRRe 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 @@ -618,7 +642,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 } diff --git a/internal/agents/agents_test.go b/internal/agents/agents_test.go index 10e708ec..9482f4a1 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) } } @@ -576,13 +579,7 @@ func TestRepoLoadSkipsMalformedAgentAndKeepsSiblings(t *testing.T) { t.Fatalf("sources = %#v, want a usable repo source", catalog.Sources) } // The skip must be visible; a silent one is how this went unnoticed. - var mentionsBad bool - for _, w := range catalog.Sources[0].Warnings { - if strings.Contains(w, "bad") { - mentionsBad = true - } - } - if !mentionsBad { + if !warningsMention(catalog.Sources[0].Warnings, "bad") { t.Fatalf("warnings = %#v, want one naming the skipped agent", catalog.Sources[0].Warnings) } } @@ -612,6 +609,81 @@ func TestRepoLoadInvalidWhenEveryAgentIsMalformed(t *testing.T) { } } +// 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) + } +} + +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..ba6788b4 100644 --- a/internal/dossier/dossier.go +++ b/internal/dossier/dossier.go @@ -1149,6 +1149,16 @@ 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 honoured. Skips belong where + // the run is observed, not only in the artifact. + for _, warning := range source.Warnings { + if msg := strings.TrimSpace(warning); msg != "" { + out.WriteString("Guidance not honoured: ") + out.WriteString(msg) + out.WriteString("\n") + } + } } if note := strings.TrimSpace(repo.RepoInfo.TrustNote()); note != "" { out.WriteString("\n") 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", From 2fe76b35910494c4afa795fbe35716fb0e412ffc Mon Sep 17 00:00:00 2001 From: piekstra Date: Wed, 5 Aug 2026 18:24:20 -0400 Subject: [PATCH 3/5] Address review: make the degradation programmatic, not prose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A partially loaded source was expressible only as free text in SourceInfo.Warnings, so no caller could branch on it. Two consequences, both reintroducing the indistinguishable-from-normal property this change exists to remove: RepoGuidanceUnavailableReason returns "" for an available source, so the posted review body said nothing about a skip; and a required_on_match agent whose definition is malformed drops out of catalog.Agents silently, where it previously blocked the run. Adds SourceInfo.Skipped []SkippedDefinition alongside the display strings, so the shape is available to callers. Whether a partial load should still satisfy required_on_match is a policy question for maintainers — this makes it expressible; it does not decide it. RepoGuidanceUnavailableReason now appends the skip reasons when the source is invalid. That is the blocking case, where the generic "contains no usable agents" error names neither the failing definition nor why, and the posted body is exactly where the operator needs it. "Every exit flushes the skips" described a convention, not an enforced property: five early returns bypass fail(), and only their position above the first append made that safe. Replaced with a defer on named results, so an exit added later cannot silently drop the list. Adds the three uncovered branches: unsafe category name with a healthy sibling, a category whose only agent is malformed with a healthy sibling, and a skip recorded before a later hard failure — the last locking in a bug that already happened once. --- internal/agents/agents.go | 57 +++++++++++++----- internal/agents/agents_test.go | 105 +++++++++++++++++++++++++++++++++ internal/dossier/dossier.go | 17 ++++-- 3 files changed, 157 insertions(+), 22 deletions(-) diff --git a/internal/agents/agents.go b/internal/agents/agents.go index 63b71876..a9af4fd9 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 honour, 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. @@ -449,14 +469,20 @@ 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 []string - // Every exit flushes the skips collected so far. An early return that dropped - // them would reproduce, one layer down, the silent failure this loader exists - // to remove: a real problem hiding because nothing recorded it. + 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) { - repoSource.Warnings = append(repoSource.Warnings, skipped...) if classified, ok := classifyRepoCatalogError(repoSource, err); allowSoftFailures && ok { return nil, classified, nil } @@ -499,7 +525,7 @@ func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenanc } if err := validateName("category", categoryName); err != nil { if errors.Is(err, ErrInvalid) { - skipped = append(skipped, fmt.Sprintf("skipped category %q: %v", categoryName, err)) + skipped = append(skipped, SkippedDefinition{Category: categoryName, Reason: err.Error()}) continue } return fail(err) @@ -508,7 +534,7 @@ func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenanc category, err := readRepoCategory(ctx, source.Reader, source.Ref, baseSHA, categoryPath, categoryName) if err != nil { if errors.Is(err, ErrInvalid) { - skipped = append(skipped, fmt.Sprintf("skipped category %q: %v", categoryName, err)) + skipped = append(skipped, SkippedDefinition{Category: categoryName, Reason: err.Error()}) continue } return fail(err) @@ -519,7 +545,7 @@ func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenanc return fail(err) } if len(categoryAgents) == 0 { - skipped = append(skipped, fmt.Sprintf("skipped category %q: no usable agents", categoryName)) + skipped = append(skipped, SkippedDefinition{Category: categoryName, Reason: "no usable agents"}) continue } loadedAgent = true @@ -532,7 +558,6 @@ func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenanc if !loadedAgent { return fail(fmt.Errorf("%w: repo source %s contains no usable agents", ErrInvalid, repoAgentsRoot)) } - repoSource.Warnings = append(repoSource.Warnings, skipped...) return agents, repoSource, nil } @@ -591,7 +616,7 @@ func readRepoCategory(ctx context.Context, reader RepoReader, ref gitprovider.PR // 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, []string, error) { +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, nil, err @@ -599,7 +624,7 @@ func readRepoAgents(ctx context.Context, reader RepoReader, ref gitprovider.PRRe sortTreeEntries(entries) var agents []Agent - var skipped []string + var skipped []SkippedDefinition for _, entry := range entries { if entry.Type != "tree" { continue @@ -610,7 +635,7 @@ func readRepoAgents(ctx context.Context, reader RepoReader, ref gitprovider.PRRe } if err := validateName("agent", agentName); err != nil { if errors.Is(err, ErrInvalid) { - skipped = append(skipped, fmt.Sprintf("skipped agent %s/%s: %v", category.Name, agentName, err)) + skipped = append(skipped, SkippedDefinition{Category: category.Name, Agent: agentName, Reason: err.Error()}) continue } return nil, skipped, err @@ -619,7 +644,7 @@ func readRepoAgents(ctx context.Context, reader RepoReader, ref gitprovider.PRRe agent, err := readRepoAgent(ctx, reader, ref, gitRef, agentPath, category, agentName, provenance) if err != nil { if errors.Is(err, ErrInvalid) { - skipped = append(skipped, fmt.Sprintf("skipped agent %s/%s: %v", category.Name, agentName, err)) + skipped = append(skipped, SkippedDefinition{Category: category.Name, Agent: agentName, Reason: err.Error()}) continue } return nil, skipped, err @@ -692,7 +717,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) } } } diff --git a/internal/agents/agents_test.go b/internal/agents/agents_test.go index 9482f4a1..31a79d0a 100644 --- a/internal/agents/agents_test.go +++ b/internal/agents/agents_test.go @@ -675,6 +675,111 @@ func TestRepoLoadSkipsAgentMissingPromptAndKeepsSiblings(t *testing.T) { } } +// 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) + } + if !skippedMentions(catalog.Sources[0].Skipped, "empty-cat", "no usable agents") { + t.Fatalf("skipped = %#v, want the empty category recorded", 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) { diff --git a/internal/dossier/dossier.go b/internal/dossier/dossier.go index ba6788b4..5a878fcc 100644 --- a/internal/dossier/dossier.go +++ b/internal/dossier/dossier.go @@ -1152,12 +1152,10 @@ func renderDossierRepoGuidance(repo dossierRepoContextArtifact) string { // A partially loaded source still reports "available", so without this the // run reads as if every declared agent had been honoured. Skips belong where // the run is observed, not only in the artifact. - for _, warning := range source.Warnings { - if msg := strings.TrimSpace(warning); msg != "" { - out.WriteString("Guidance not honoured: ") - out.WriteString(msg) - out.WriteString("\n") - } + for _, entry := range source.Skipped { + out.WriteString("Guidance not honoured: ") + out.WriteString(entry.String()) + out.WriteString("\n") } } if note := strings.TrimSpace(repo.RepoInfo.TrustNote()); note != "" { @@ -1197,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 } From e7fbab1a3ea16fdbda8d463138f2b718bb4050ae Mon Sep 17 00:00:00 2001 From: piekstra Date: Wed, 5 Aug 2026 18:31:17 -0400 Subject: [PATCH 4/5] Extract the skip predicate into one helper The 'skip if ErrInvalid, else fail' shape was hand-written at four call sites, each re-typing the errors.Is check and building the SkippedDefinition. A future skip-safe error class, or a fix to the classification itself, could reach three of them and miss one. skipIfInvalid now owns that decision. --- internal/agents/agents.go | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/internal/agents/agents.go b/internal/agents/agents.go index a9af4fd9..bef2ee0a 100644 --- a/internal/agents/agents.go +++ b/internal/agents/agents.go @@ -524,8 +524,8 @@ func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenanc continue } if err := validateName("category", categoryName); err != nil { - if errors.Is(err, ErrInvalid) { - skipped = append(skipped, SkippedDefinition{Category: categoryName, Reason: err.Error()}) + if entry, ok := skipIfInvalid(err, SkippedDefinition{Category: categoryName}); ok { + skipped = append(skipped, entry) continue } return fail(err) @@ -533,8 +533,8 @@ func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenanc categoryPath := path.Join(repoAgentsRoot, categoryName) category, err := readRepoCategory(ctx, source.Reader, source.Ref, baseSHA, categoryPath, categoryName) if err != nil { - if errors.Is(err, ErrInvalid) { - skipped = append(skipped, SkippedDefinition{Category: categoryName, Reason: err.Error()}) + if entry, ok := skipIfInvalid(err, SkippedDefinition{Category: categoryName}); ok { + skipped = append(skipped, entry) continue } return fail(err) @@ -561,6 +561,17 @@ func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenanc 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 @@ -634,8 +645,8 @@ func readRepoAgents(ctx context.Context, reader RepoReader, ref gitprovider.PRRe continue } if err := validateName("agent", agentName); err != nil { - if errors.Is(err, ErrInvalid) { - skipped = append(skipped, SkippedDefinition{Category: category.Name, Agent: agentName, Reason: err.Error()}) + if entry, ok := skipIfInvalid(err, SkippedDefinition{Category: category.Name, Agent: agentName}); ok { + skipped = append(skipped, entry) continue } return nil, skipped, err @@ -643,8 +654,8 @@ func readRepoAgents(ctx context.Context, reader RepoReader, ref gitprovider.PRRe agentPath := path.Join(categoryPath, agentName) agent, err := readRepoAgent(ctx, reader, ref, gitRef, agentPath, category, agentName, provenance) if err != nil { - if errors.Is(err, ErrInvalid) { - skipped = append(skipped, SkippedDefinition{Category: category.Name, Agent: agentName, Reason: err.Error()}) + if entry, ok := skipIfInvalid(err, SkippedDefinition{Category: category.Name, Agent: agentName}); ok { + skipped = append(skipped, entry) continue } return nil, skipped, err From 21b177b9132a6aec0c901ee10f15250ef9c0526c Mon Sep 17 00:00:00 2001 From: piekstra Date: Wed, 5 Aug 2026 18:44:33 -0400 Subject: [PATCH 5/5] Address review: lint, duplicate skip entries, dropped causes, clone parity Blocking: "honour"/"honoured" tripped the repo's misspell linter. One of those was not a comment but the literal rendered into the dossier, so the artifact text changes with it. golangci-lint is clean on the changed packages. A category whose agents were themselves skipped reported twice: once per agent with the actionable detail, then again as "no usable agents" without it. The category entry is now recorded only when no per-agent skip explains it, and a category that declares no agent directories at all says "declares no agents" rather than borrowing the malformed-agent wording. scopedToDefinition wrapped only ErrInvalid, discarding the provider error. These reasons reach the operator in the posted body, so a NOT_FOUND raised for anything other than an absent blob left nothing to diagnose with. Both are in the chain now; classification is unaffected because ErrInvalid is tested first. SourceInfo.Skipped opted out of the deep-copy the neighbouring Warnings field gets, so a SourceInfo round-tripped through Provenance silently lost it. Added the field to Provenance and both conversion sites. The category test asserted the double-report this removes; updated, and the genuinely-empty case now has its own coverage. --- internal/agents/agents.go | 21 +++++++++++++--- internal/agents/agents_test.go | 44 ++++++++++++++++++++++++++++++++-- internal/dossier/dossier.go | 4 ++-- 3 files changed, 62 insertions(+), 7 deletions(-) diff --git a/internal/agents/agents.go b/internal/agents/agents.go index bef2ee0a..18be8d6c 100644 --- a/internal/agents/agents.go +++ b/internal/agents/agents.go @@ -76,7 +76,7 @@ type SourceInfo struct { Status SourceStatus `json:"status"` Fingerprint string `json:"fingerprint,omitempty"` Warnings []string `json:"warnings,omitempty"` - // Skipped records definitions this source declared but could not honour, so a + // 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"` @@ -110,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. @@ -144,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 } @@ -545,7 +549,13 @@ func loadRepoSource(ctx context.Context, source RepoSource, provenance Provenanc return fail(err) } if len(categoryAgents) == 0 { - skipped = append(skipped, SkippedDefinition{Category: categoryName, Reason: "no usable agents"}) + // 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 @@ -582,7 +592,11 @@ func scopedToDefinition(err error, format string, args ...any) error { if !errors.Is(err, gitprovider.ErrNotFound) { return err } - return fmt.Errorf("%w: %s", ErrInvalid, fmt.Sprintf(format, args...)) + // 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) { @@ -889,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 31a79d0a..1fd2bb36 100644 --- a/internal/agents/agents_test.go +++ b/internal/agents/agents_test.go @@ -729,8 +729,48 @@ func TestRepoLoadSkipsCategoryWithNoUsableAgentsAndKeepsSiblings(t *testing.T) { 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 !skippedMentions(catalog.Sources[0].Skipped, "empty-cat", "no usable agents") { - t.Fatalf("skipped = %#v, want the empty category recorded", catalog.Sources[0].Skipped) + // 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) } } diff --git a/internal/dossier/dossier.go b/internal/dossier/dossier.go index 5a878fcc..deb06383 100644 --- a/internal/dossier/dossier.go +++ b/internal/dossier/dossier.go @@ -1150,10 +1150,10 @@ func renderDossierRepoGuidance(repo dossierRepoContextArtifact) string { } } // A partially loaded source still reports "available", so without this the - // run reads as if every declared agent had been honoured. Skips belong where + // 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 honoured: ") + out.WriteString("Guidance not honored: ") out.WriteString(entry.String()) out.WriteString("\n") }