fix: skip malformed repo agents instead of rejecting the whole source

[piekstra]

Requested by @rianjs: "we should have cr not break in the event of a malformed prompt. It should just get skipped."

Problem

A single unparseable index.yaml under .codereview/agents/ disqualifies every agent in that repo. loadRepoSource calls fail(err) on the first ErrInvalid, the source is classified SourceStatusInvalid, and the review is posted as request_changes with no reviewers run.

That outcome is visually indistinguishable from a reviewer that ran and objected, which is why it hid. Two SignalFT repos had this for ~2 months from one typo — model: where the schema field is model_tier: — including the repo where that agent was originally added.

Change

An agent that fails to load is never selected, so skipping it is exactly as safe as refusing the whole source, and it leaves the rest of the repo working.

• readRepoAgents returns ([]Agent, []string, error) — skips agents whose definition is ErrInvalid, returning one message per skip.
• loadRepoSource skips categories the same way, and no longer fails a category that ends up with no usable agents.
• A source where nothing loads is still SourceStatusInvalid. Guidance cannot silently degrade to nothing.
• Skips land on SourceInfo.Warnings, naming each agent — a silent skip is how the original problem hid.

Non-ErrInvalid errors (I/O, unreadable trees) still fail the source unchanged.

Tests

• TestRepoLoadSkipsMalformedAgentAndKeepsSiblings — a category with one good and one model:-typo agent loads the good one and warns about the bad one. Uses the exact malformed shape from the real incident.
• TestRepoLoadInvalidWhenEveryAgentIsMalformed — all-bad still yields SourceStatusInvalid.

Existing internal/agents, internal/pipeline, internal/dossier, internal/reviewplan suites pass unchanged.

Note

The two repo-side typos are being fixed separately (SignalFT/monit-terraform-monitapp#1382, SignalFT/monit-keycloak#472). Those PRs cannot be reviewed by cr today, because it loads guidance from the base branch — which still contains the file that breaks loading. This change removes that deadlock for the future.

[monit-reviewer]

Automated PR Review

Reviewed commit: 9d38769242ef
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	1
structure:repo-health	1
harness-engineering:repo-health	0
architecture:solid-reviewer-agnostic	3

go:implementation-tests (1 finding)

Major - internal/agents/agents_test.go:613

The diff adds three new category-level skip branches in loadRepoSource (invalid category name at agents.go:497-500, malformed category index.yaml at agents.go:506-509, and a category whose agents all skip at agents.go:517-520), each appending a skipped category %q: ... warning and continuing to the next category instead of failing the source. Only the agent-level skip path (readRepoAgents) is exercised by the new tests (TestRepoLoadSkipsMalformedAgentAndKeepsSiblings, TestRepoLoadInvalidWhenEveryAgentIsMalformed) — both fixtures use a single category with a good/bad agent pair, so readRepoCategory/validateName("category", ...) returning ErrInvalid never executes on this PR. Because loadRepoSource is the exact function the PR is fixing (a malformed definition disqualifying more than it should), a regression that turns a category-level skip back into a whole-source failure (or vice versa, silently absorbs a genuine category malformation) would pass CI untested. Add a case with two categories where one has a malformed index.yaml (or an unsafe category name) alongside a well-formed sibling category, asserting the sibling's agents still load and the source stays available with a skipped category "<name>": ... warning.

structure:repo-health (1 finding)

Minor - internal/agents/agents.go:524

The invariant this PR establishes is that skips must be visible in SourceInfo.Warnings (tested by TestRepoLoadSkipsMalformedAgentAndKeepsSiblings). But skipped is only flushed into repoSource.Warnings once, after the category loop finishes (line 524). The three early return fail(err) exits inside the loop (lines ~501, ~510, ~515) — reached when a later category or agent hits a non-ErrInvalid error (e.g. a transient GetFileAtRef failure) after an earlier category already accumulated ErrInvalid skip messages — return before that flush, so every already-collected skip message is silently dropped from the returned SourceInfo. That's exactly the failure mode this PR was written to eliminate (a real problem hiding because nothing records it), just moved one layer down. Fix: assign repoSource.Warnings = append(repoSource.Warnings, skipped...) before each early return in the loop (or restructure fail to always flush skipped first), and add a test mixing a prior ErrInvalid skip with a later hard I/O error to lock in the fix.

architecture:solid-reviewer-agnostic (3 findings)

Major - internal/agents/agents.go:524

U-G1 / U-L2: SourceInfo.Warnings is the wrong channel for the visibility the PR relies on, and the documented contract it changes is not amended.

On the review path, repo-source warnings are copied into the run artifact (internal/pipeline/artifacts.go:116) and rendered by cr agents / cr config — but nothing in a cr review run reads them. renderDossierRepoGuidance (internal/dossier/dossier.go:1124) prints only Status and Error; RepoGuidanceUnavailableReason returns "" for an available source; opts.emitWarning is never called with them. A repo whose reviewer silently stopped loading now gets a review that looks completely normal, and the skip is discoverable only by inspecting artifacts or separately running cr agents. The PR's own stated guard — "a silent skip is how the original problem hid" — is only half-delivered.

docs/review-guidance.md:47-56 is the project's stated contract for this surface and now conflicts with the code on two points: it enumerates the dossier statuses as "available, missing, unreadable, or invalid" with no notion of partially loaded, and it justifies invalid-is-blocking as "maintainers attempted to declare authoritative review behavior that could not be honored" — which is exactly what a skipped agent is, now reported as available. Per reviewer conduct, that is an amendment, not something to let erode.

Suggested fix, both halves: surface the skips where the run is observed — add them to renderDossierRepoGuidance under the status line (the doc promises that file is for operators "without reading pipeline code") and/or opts.emitWarning one line per skip during the run — and update docs/review-guidance.md to describe the partially-loaded case and that a skipped agent is warned, not blocking.

Major - internal/agents/agents.go:597

U-L2: the skip predicate keys on error class (ErrInvalid) rather than on the axis that actually matters here — whether the failure is scoped to one agent or to the whole source. readRepoAgent reaches the reader twice (decodeRepoYAML on index.yaml, GetFileAtRef on prompt.md), and both surface a missing file as gitprovider.ErrNotFound, not ErrInvalid. So an agent directory whose prompt.md was never committed — or a category directory missing index.yaml, via readRepoCategory at line 506 — still falls through to fail(err), gets classified SourceStatusUnreadable, and disqualifies every other agent in the repo. That is the same blast radius this PR removes for the model:-typo shape, reached by a defect that is at least as easy to commit (add the agent dir, forget the prompt file).

The PR body frames the untouched class as "I/O, unreadable trees", which is the right thing to keep fatal — but ErrNotFound on a path inside one agent directory is not that; it is a malformed agent. Concrete fix: draw the boundary by path scope rather than by error class. In readRepoAgent/readRepoCategory, wrap a gitprovider.ErrNotFound on a file beneath the agent/category directory as ErrInvalid (fmt.Errorf("%w: agent %s:%s is missing %s", ErrInvalid, ...)), leaving reader failures on ListTreeAtRef and non-NotFound transport errors to fail the source unchanged. That also makes the skip decision legible at the point where the scope is known, instead of inferring it from an error class two layers up.

Minor - internal/agents/agents.go:566

U-L1: the two loaders that feed the same Catalog now have divergent failure semantics with no stated reason. readFileAgents (line 408) still returns on the first validateName/readFileAgent error, so a profile source with one malformed agent loses all of them — while a repo source skips and keeps its siblings. The new doc comment justifies skipping in terms that apply identically to both ("an unusable agent is never selected, so skipping it is as safe as refusing the whole source"), which reads as an oversight rather than a decision, and invites the next change to close the gap by copying the repo behavior into the filesystem path without thinking about it.

There is a good rationale available — repo sources are PR-adjacent content the operator does not own, profile sources are the operator's own configuration where failing loudly is correct — but it is nowhere in the diff. Either say that in the comment (naming the trust boundary, not just the safety argument), or mirror the behavior in readFileAgents. Given docs/review-guidance.md treats repo guidance as the authoritative-but-untrusted surface, stating the boundary is the cheaper of the two.

Reviewer Coverage

• go:implementation-tests — complete (broad)
• structure:repo-health — complete (broad); inspected 1 of 2 files: internal/agents/agents.go
• harness-engineering:repo-health — complete (broad); Scope limited to internal/agents/agents.go and agents_test.go; docs/review-guidance.md is cited only as context for a finding anchored in the assigned diff, not itself inspected as a changed file.
• architecture:solid-reviewer-agnostic — complete (broad); inspected 1 of 2 files: internal/agents/agents.go; Assignment scoped to internal/agents/agents.go; internal/agents/agents_test.go, internal/pipeline, internal/dossier and docs/review-guidance.md were read for context only, so findings that imply changes there are anchored on the assigned file. Verified with go build ./... and go test ./internal/agents/ in the head checkout: both pass. No red check is attributable to this diff.

Inspected files (2)

• internal/agents/agents.go
• internal/agents/agents_test.go

0 PR discussion threads considered. 0 summarized; 0 resolved.

---

Completed in 5m 57s | ~$5.12 (est.) | claude-sonnet-5, claude-opus-5 | cr 0.10.277

Field	Value
Model	claude-sonnet-5, claude-opus-5
Reviewers	go:implementation-tests, structure:repo-health, harness-engineering:repo-health, architecture:solid-reviewer-agnostic
Engine	claude_cli · claude-sonnet-5, claude-opus-5
Reviewed by	cr · monit-reviewer
Duration	5m 57s wall · 13m 05s compute
Cost	~$5.12 (est.)
Tokens	112 in / 42.9k out

Per-workstream usage

• orchestrator-selection — claude-sonnet-5
  • In: 6
  • Out: 2.1k
  • Cache read: 61.3k
  • Cache create: 100.9k
  • Cost: ~$0.43 (est.)
  • Duration: 30s
• go:implementation-tests — claude-sonnet-5
  • In: 18
  • Out: 6.2k
  • Cache read: 572.2k
  • Cache create: 132.2k
  • Cost: ~$0.76 (est.)
  • Duration: 1m 37s
• structure:repo-health — claude-sonnet-5
  • In: 16
  • Out: 9.3k
  • Cache read: 593.4k
  • Cache create: 116.3k
  • Cost: ~$0.75 (est.)
  • Duration: 2m 01s
• harness-engineering:repo-health — claude-sonnet-5
  • In: 22
  • Out: 6.6k
  • Cache read: 922.0k
  • Cache create: 120.9k
  • Cost: ~$0.83 (est.)
  • Duration: 3m 39s
• architecture:solid-reviewer-agnostic — claude-opus-5
  • In: 44
  • Out: 15.6k
  • Cache read: 1.5M
  • Cache create: 124.0k
  • Cost: ~$1.92 (est.)
  • Duration: 4m 34s
• orchestrator-rollup — claude-sonnet-5
  • In: 6
  • Out: 3.0k
  • Cache read: 98.2k
  • Cache create: 93.9k
  • Cost: ~$0.43 (est.)
  • Duration: 42s

[monit-reviewer]

The invariant this PR establishes is that skips must be visible in SourceInfo.Warnings (tested by TestRepoLoadSkipsMalformedAgentAndKeepsSiblings). But skipped is only flushed into repoSource.Warnings once, after the category loop finishes (line 524). The three early return fail(err) exits inside the loop (lines ~501, ~510, ~515) — reached when a later category or agent hits a non-ErrInvalid error (e.g. a transient GetFileAtRef failure) after an earlier category already accumulated ErrInvalid skip messages — return before that flush, so every already-collected skip message is silently dropped from the returned SourceInfo. That's exactly the failure mode this PR was written to eliminate (a real problem hiding because nothing records it), just moved one layer down. Fix: assign repoSource.Warnings = append(repoSource.Warnings, skipped...) before each early return in the loop (or restructure fail to always flush skipped first), and add a test mixing a prior ErrInvalid skip with a later hard I/O error to lock in the fix.

Reply inline to this comment.

[monit-reviewer]

U-G1 / U-L2: SourceInfo.Warnings is the wrong channel for the visibility the PR relies on, and the documented contract it changes is not amended.

On the review path, repo-source warnings are copied into the run artifact (internal/pipeline/artifacts.go:116) and rendered by cr agents / cr config — but nothing in a cr review run reads them. renderDossierRepoGuidance (internal/dossier/dossier.go:1124) prints only Status and Error; RepoGuidanceUnavailableReason returns "" for an available source; opts.emitWarning is never called with them. A repo whose reviewer silently stopped loading now gets a review that looks completely normal, and the skip is discoverable only by inspecting artifacts or separately running cr agents. The PR's own stated guard — "a silent skip is how the original problem hid" — is only half-delivered.

docs/review-guidance.md:47-56 is the project's stated contract for this surface and now conflicts with the code on two points: it enumerates the dossier statuses as "available, missing, unreadable, or invalid" with no notion of partially loaded, and it justifies invalid-is-blocking as "maintainers attempted to declare authoritative review behavior that could not be honored" — which is exactly what a skipped agent is, now reported as available. Per reviewer conduct, that is an amendment, not something to let erode.

Suggested fix, both halves: surface the skips where the run is observed — add them to renderDossierRepoGuidance under the status line (the doc promises that file is for operators "without reading pipeline code") and/or opts.emitWarning one line per skip during the run — and update docs/review-guidance.md to describe the partially-loaded case and that a skipped agent is warned, not blocking.

Reply inline to this comment.

[monit-reviewer]

The diff adds three new category-level skip branches in loadRepoSource (invalid category name at agents.go:497-500, malformed category index.yaml at agents.go:506-509, and a category whose agents all skip at agents.go:517-520), each appending a skipped category %q: ... warning and continuing to the next category instead of failing the source. Only the agent-level skip path (readRepoAgents) is exercised by the new tests (TestRepoLoadSkipsMalformedAgentAndKeepsSiblings, TestRepoLoadInvalidWhenEveryAgentIsMalformed) — both fixtures use a single category with a good/bad agent pair, so readRepoCategory/validateName("category", ...) returning ErrInvalid never executes on this PR. Because loadRepoSource is the exact function the PR is fixing (a malformed definition disqualifying more than it should), a regression that turns a category-level skip back into a whole-source failure (or vice versa, silently absorbs a genuine category malformation) would pass CI untested. Add a case with two categories where one has a malformed index.yaml (or an unsafe category name) alongside a well-formed sibling category, asserting the sibling's agents still load and the source stays available with a skipped category "<name>": ... warning.

Reply inline to this comment.

[monit-reviewer]

U-L1: the two loaders that feed the same Catalog now have divergent failure semantics with no stated reason. readFileAgents (line 408) still returns on the first validateName/readFileAgent error, so a profile source with one malformed agent loses all of them — while a repo source skips and keeps its siblings. The new doc comment justifies skipping in terms that apply identically to both ("an unusable agent is never selected, so skipping it is as safe as refusing the whole source"), which reads as an oversight rather than a decision, and invites the next change to close the gap by copying the repo behavior into the filesystem path without thinking about it.

There is a good rationale available — repo sources are PR-adjacent content the operator does not own, profile sources are the operator's own configuration where failing loudly is correct — but it is nowhere in the diff. Either say that in the comment (naming the trust boundary, not just the safety argument), or mirror the behavior in readFileAgents. Given docs/review-guidance.md treats repo guidance as the authoritative-but-untrusted surface, stating the boundary is the cheaper of the two.

Reply inline to this comment.

[monit-reviewer]

U-L2: the skip predicate keys on error class (ErrInvalid) rather than on the axis that actually matters here — whether the failure is scoped to one agent or to the whole source. readRepoAgent reaches the reader twice (decodeRepoYAML on index.yaml, GetFileAtRef on prompt.md), and both surface a missing file as gitprovider.ErrNotFound, not ErrInvalid. So an agent directory whose prompt.md was never committed — or a category directory missing index.yaml, via readRepoCategory at line 506 — still falls through to fail(err), gets classified SourceStatusUnreadable, and disqualifies every other agent in the repo. That is the same blast radius this PR removes for the model:-typo shape, reached by a defect that is at least as easy to commit (add the agent dir, forget the prompt file).

The PR body frames the untouched class as "I/O, unreadable trees", which is the right thing to keep fatal — but ErrNotFound on a path inside one agent directory is not that; it is a malformed agent. Concrete fix: draw the boundary by path scope rather than by error class. In readRepoAgent/readRepoCategory, wrap a gitprovider.ErrNotFound on a file beneath the agent/category directory as ErrInvalid (fmt.Errorf("%w: agent %s:%s is missing %s", ErrInvalid, ...)), leaving reader failures on ListTreeAtRef and non-NotFound transport errors to fail the source unchanged. That also makes the skip decision legible at the point where the scope is known, instead of inferring it from an error class two layers up.

Reply inline to this comment.

[monit-reviewer]

Automated PR Review

Reviewed commit: 271a82a4eecc
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	1
structure:repo-health	1
harness-engineering:repo-health	1
architecture:solid-reviewer-agnostic	2

go:implementation-tests (1 finding)

Minor - internal/dossier/dossier.go:1149

RepoGuidanceUnavailableReason (used to build both the request_changes review body via pipeline.go's RepoGuidanceUnavailable/RepoGuidanceUnavailableReason plumbing, and the dry-run text asserted in pipeline_test.go) only reads source.Error, never source.Warnings. In the all-agents-malformed case (TestRepoLoadInvalidWhenEveryAgentIsMalformed / SourceStatusInvalid), source.Error is the generic "repo source .../agents contains no usable agents" message, while the specific per-agent skip reasons (e.g. "skipped agent cat/bad: ...") sit in source.Warnings, which this function never surfaces. That's the one scenario where the operator most needs the detail: the blocking review is exactly the silent-failure case this PR is fixing, and here the diagnostic collected for that purpose is dropped from the message actually posted. renderDossierRepoGuidance (a few lines above, in the same file) already knows how to append source.Warnings — RepoGuidanceUnavailableReason should do the same when status is Invalid so the request_changes body names which agents/categories failed and why, not just that nothing loaded.

structure:repo-health (1 finding)

Minor - internal/agents/agents.go:521

loadRepoSource has three distinct category-level skip-and-continue branches: invalid category name (line 500-506), malformed category index.yaml (line 508-515), and a category that loads but ends up with zero usable agents (line 521-524, 'no usable agents'). Only the second is covered by a sibling-continuation test (TestRepoLoadSkipsMalformedCategoryAndKeepsSiblingCategories). The invalid-category-name and 'no usable agents' branches are only exercised in single-category tests where the whole source ends up SourceStatusInvalid (TestRepoLoadRejectsUnsafeTreeAndYAMLNames's 'unsafe category tree name' case, and TestRepoLoadInvalidWhenEveryAgentIsMalformed), so a regression that turned either of these two branches back into a whole-source failure when a good sibling category exists would pass CI unnoticed — the same class of silent breakage this PR exists to close. Add a case per branch with a healthy sibling category present, mirroring TestRepoLoadSkipsMalformedCategoryAndKeepsSiblingCategories's shape (e.g. one category with an unsafe tree name plus one valid category; one category whose only agent is malformed, alongside a valid category), asserting catalog.Agents still contains the sibling's agent and Sources[0].Warnings names the skipped category.

harness-engineering:repo-health (1 finding)

Minor - internal/agents/agents_test.go:505

loadRepoSource's fail() now flushes accumulated skipped warnings into repoSource.Warnings before an early return, fixing the prior bug where a hard failure after some skips had already been recorded would silently drop them. But no test exercises that exact sequence: a category that accumulates a skip warning followed by a later category/agent hitting a non-ErrInvalid error (e.g. readRepoAgents' own reader.ListTreeAtRef call returning gitprovider.ErrNotFound, which is not routed through scopedToDefinition and so reaches fail(err) directly). All current tests either skip-and-succeed or fail-with-nothing-loaded; none asserts that an earlier skip message survives a later hard failure. Since this was already a real bug once, add a test constructing that ordering and asserting the pre-fail skip messages remain in catalog.Sources[0].Warnings, so a regression is caught mechanically instead of by re-review.

architecture:solid-reviewer-agnostic (2 findings)

Major - internal/agents/agents.go:535

U-L2 (errors are contracts) / U-I1 (narrow contracts, consumer-owned): a partially-loaded source is now representable only as free-text prose in SourceInfo.Warnings, so no downstream consumer can branch on "this source degraded".

After this change a source that skipped agents still returns Status = SourceStatusAvailable. Two consumers read that status as "every declared agent was honoured":

• dossier.RepoGuidanceUnavailableReason (internal/dossier/dossier.go:1179) returns "" for available, so pipeline.go:755 does not take the guidance-unavailable path and reviewplan.renderRepoGuidanceUnavailableRollup never runs. The posted review body therefore contains nothing about the skip. The prose lands in the dossier's repo-guidance section (fed to reviewer prompts) and in cr agents/cr config output — neither is where the human who must fix the typo is looking. The PR's own framing is that the original bug hid because the outcome was indistinguishable from a normal review; the partial-load case reproduces that property, just in the other direction.
• pipeline.ensureRequiredOnMatchAgents (internal/pipeline/pipeline.go:1520) iterates catalog.Agents. A skipped agent is not in the catalog, so a required_on_match agent whose index.yaml is malformed is silently dropped from the mandatory-reviewer set and the PR can be approved without it. Previously that repo was SourceStatusInvalid and the run requested changes. The skip cannot be reasoned about here precisely because the definition did not parse — we cannot know whether what we dropped was required.

The change in behaviour is intended and I am not arguing against it; the gap is that the degradation is only expressible as a string, so the decision "is a partial load acceptable for this run" cannot be made by any caller.

Suggested fix: give the loader a programmatic signal alongside the human-readable warnings — either a distinct SourceStatusPartial (with RepoGuidanceUnavailableReason and the rollup builder deciding what it means), or a structured field such as SourceInfo.Skipped []SkippedDefinition{Category, Agent, Reason string}. Then the rollup can state "N repo agents were skipped" in the posted review, and the required-on-match backstop has something to key on. Keep the strings for display; add the shape callers need.

Defensible either way on severity: if the maintainers accept that a skipped agent is never required-on-match in practice, this drops to minor. Evidence that would change my verdict: a rendering path I missed that puts SourceInfo.Warnings into the posted review body rather than only the reviewer prompt.

Minor - internal/agents/agents.go:455

U-S1 (one reason to change): the comment states an invariant the function does not hold, and nothing enforces it.

"Every exit flushes the skips collected so far" is not true of loadRepoSource — lines 466, 469, 472, 477 and the ListTreeAtRef error path all return nil, repoSource, ... without going through fail, and the success path at line 535 repeats the flush by hand. Today that is harmless because those five exits all precede the first possible append to skipped, but the ordering is the only thing making it correct, and it is invisible at the call sites. The next validation added anywhere below line 490 that returns directly silently drops the skip list — which is precisely the failure mode the comment says the loader exists to prevent.

The underlying cause is that loadRepoSource now does two jobs: pre-flight argument validation (which cannot have skips) and tree traversal with skip accounting (which always can). Suggested fix, either:

• extract the pre-flight block into a validateRepoSource(source) (baseSHA string, err error) helper so the remainder of the body has exactly two exits, fail and success; or
• use named results and defer func() { info.Warnings = append(info.Warnings, skipped...) }(), which makes the flush unconditional and lets the hand-written append at line 535 and inside fail both go away.

Either makes the comment describe an enforced property instead of a convention.

Reviewer Coverage

• go:implementation-tests — complete (constrained); inspected 4 of 5 files: internal/agents/agents.go, internal/agents/agents_test.go, internal/dossier/dossier.go, internal/pipeline/pipeline_test.go
• structure:repo-health — complete (constrained); inspected 1 of 5 files: internal/agents/agents.go; Only internal/agents/agents.go was in scope; internal/agents/agents_test.go was inspected read-only for coverage context, not reviewed for its own findings.
• harness-engineering:repo-health — complete (constrained); inspected 3 of 5 files: docs/review-guidance.md, internal/agents/agents.go, internal/agents/agents_test.go
• architecture:solid-reviewer-agnostic — complete (constrained); inspected 1 of 5 files: internal/agents/agents.go; Assignment scoped to internal/agents/agents.go; the companion changes in internal/dossier/dossier.go, internal/agents/agents_test.go and internal/pipeline/pipeline_test.go were read for context only and are not reported on. Verified locally: go build ./..., go vet ./internal/agents, and go test ./internal/agents ./internal/dossier ./internal/pipeline all pass at 271a82a.

Inspected files (5)

• docs/review-guidance.md
• internal/agents/agents.go
• internal/agents/agents_test.go
• internal/dossier/dossier.go
• internal/pipeline/pipeline_test.go

0 PR discussion threads considered. 0 summarized; 0 resolved.

---

Completed in 7m 14s | ~$3.85 (est.) | claude-sonnet-5, claude-opus-5 | cr 0.10.277

Field	Value
Model	claude-sonnet-5, claude-opus-5
Reviewers	go:implementation-tests, structure:repo-health, harness-engineering:repo-health, architecture:solid-reviewer-agnostic
Engine	claude_cli · claude-sonnet-5, claude-opus-5
Reviewed by	cr · monit-reviewer
Duration	7m 14s wall · 7m 58s compute
Cost	~$3.85 (est.)
Tokens	62 in / 19.6k out

Per-workstream usage

• go:implementation-tests — claude-sonnet-5
  • In: 8
  • Out: 1.7k
  • Cache read: 224.2k
  • Cache create: 132.3k
  • Cost: ~$0.59 (est.)
  • Duration: 37s
• structure:repo-health — claude-sonnet-5
  • In: 8
  • Out: 1.6k
  • Cache read: 246.5k
  • Cache create: 155.4k
  • Cost: ~$0.68 (est.)
  • Duration: 28s
• harness-engineering:repo-health — claude-sonnet-5
  • In: 6
  • Out: 1.1k
  • Cache read: 135.4k
  • Cache create: 142.9k
  • Cost: ~$0.59 (est.)
  • Duration: 18s
• architecture:solid-reviewer-agnostic — claude-opus-5
  • In: 34
  • Out: 13.9k
  • Cache read: 1.1M
  • Cache create: 98.0k
  • Cost: ~$1.52 (est.)
  • Duration: 6m 07s
• orchestrator-rollup — claude-sonnet-5
  • In: 6
  • Out: 1.2k
  • Cache read: 107.3k
  • Cache create: 111.2k
  • Cost: ~$0.47 (est.)
  • Duration: 25s

[monit-reviewer]

File-level note: internal/agents/agents_test.go

loadRepoSource's fail() now flushes accumulated skipped warnings into repoSource.Warnings before an early return, fixing the prior bug where a hard failure after some skips had already been recorded would silently drop them. But no test exercises that exact sequence: a category that accumulates a skip warning followed by a later category/agent hitting a non-ErrInvalid error (e.g. readRepoAgents' own reader.ListTreeAtRef call returning gitprovider.ErrNotFound, which is not routed through scopedToDefinition and so reaches fail(err) directly). All current tests either skip-and-succeed or fail-with-nothing-loaded; none asserts that an earlier skip message survives a later hard failure. Since this was already a real bug once, add a test constructing that ordering and asserting the pre-fail skip messages remain in catalog.Sources[0].Warnings, so a regression is caught mechanically instead of by re-review.

Reply inline to this comment.

[monit-reviewer]

File-level note: internal/dossier/dossier.go

RepoGuidanceUnavailableReason (used to build both the request_changes review body via pipeline.go's RepoGuidanceUnavailable/RepoGuidanceUnavailableReason plumbing, and the dry-run text asserted in pipeline_test.go) only reads source.Error, never source.Warnings. In the all-agents-malformed case (TestRepoLoadInvalidWhenEveryAgentIsMalformed / SourceStatusInvalid), source.Error is the generic "repo source .../agents contains no usable agents" message, while the specific per-agent skip reasons (e.g. "skipped agent cat/bad: ...") sit in source.Warnings, which this function never surfaces. That's the one scenario where the operator most needs the detail: the blocking review is exactly the silent-failure case this PR is fixing, and here the diagnostic collected for that purpose is dropped from the message actually posted. renderDossierRepoGuidance (a few lines above, in the same file) already knows how to append source.Warnings — RepoGuidanceUnavailableReason should do the same when status is Invalid so the request_changes body names which agents/categories failed and why, not just that nothing loaded.

Reply inline to this comment.

[monit-reviewer]

U-S1 (one reason to change): the comment states an invariant the function does not hold, and nothing enforces it.

"Every exit flushes the skips collected so far" is not true of loadRepoSource — lines 466, 469, 472, 477 and the ListTreeAtRef error path all return nil, repoSource, ... without going through fail, and the success path at line 535 repeats the flush by hand. Today that is harmless because those five exits all precede the first possible append to skipped, but the ordering is the only thing making it correct, and it is invisible at the call sites. The next validation added anywhere below line 490 that returns directly silently drops the skip list — which is precisely the failure mode the comment says the loader exists to prevent.

The underlying cause is that loadRepoSource now does two jobs: pre-flight argument validation (which cannot have skips) and tree traversal with skip accounting (which always can). Suggested fix, either:

• extract the pre-flight block into a validateRepoSource(source) (baseSHA string, err error) helper so the remainder of the body has exactly two exits, fail and success; or
• use named results and defer func() { info.Warnings = append(info.Warnings, skipped...) }(), which makes the flush unconditional and lets the hand-written append at line 535 and inside fail both go away.

Either makes the comment describe an enforced property instead of a convention.

Reply inline to this comment.

[monit-reviewer]

loadRepoSource has three distinct category-level skip-and-continue branches: invalid category name (line 500-506), malformed category index.yaml (line 508-515), and a category that loads but ends up with zero usable agents (line 521-524, 'no usable agents'). Only the second is covered by a sibling-continuation test (TestRepoLoadSkipsMalformedCategoryAndKeepsSiblingCategories). The invalid-category-name and 'no usable agents' branches are only exercised in single-category tests where the whole source ends up SourceStatusInvalid (TestRepoLoadRejectsUnsafeTreeAndYAMLNames's 'unsafe category tree name' case, and TestRepoLoadInvalidWhenEveryAgentIsMalformed), so a regression that turned either of these two branches back into a whole-source failure when a good sibling category exists would pass CI unnoticed — the same class of silent breakage this PR exists to close. Add a case per branch with a healthy sibling category present, mirroring TestRepoLoadSkipsMalformedCategoryAndKeepsSiblingCategories's shape (e.g. one category with an unsafe tree name plus one valid category; one category whose only agent is malformed, alongside a valid category), asserting catalog.Agents still contains the sibling's agent and Sources[0].Warnings names the skipped category.

Reply inline to this comment.

[monit-reviewer]

U-L2 (errors are contracts) / U-I1 (narrow contracts, consumer-owned): a partially-loaded source is now representable only as free-text prose in SourceInfo.Warnings, so no downstream consumer can branch on "this source degraded".

After this change a source that skipped agents still returns Status = SourceStatusAvailable. Two consumers read that status as "every declared agent was honoured":

• dossier.RepoGuidanceUnavailableReason (internal/dossier/dossier.go:1179) returns "" for available, so pipeline.go:755 does not take the guidance-unavailable path and reviewplan.renderRepoGuidanceUnavailableRollup never runs. The posted review body therefore contains nothing about the skip. The prose lands in the dossier's repo-guidance section (fed to reviewer prompts) and in cr agents/cr config output — neither is where the human who must fix the typo is looking. The PR's own framing is that the original bug hid because the outcome was indistinguishable from a normal review; the partial-load case reproduces that property, just in the other direction.
• pipeline.ensureRequiredOnMatchAgents (internal/pipeline/pipeline.go:1520) iterates catalog.Agents. A skipped agent is not in the catalog, so a required_on_match agent whose index.yaml is malformed is silently dropped from the mandatory-reviewer set and the PR can be approved without it. Previously that repo was SourceStatusInvalid and the run requested changes. The skip cannot be reasoned about here precisely because the definition did not parse — we cannot know whether what we dropped was required.

The change in behaviour is intended and I am not arguing against it; the gap is that the degradation is only expressible as a string, so the decision "is a partial load acceptable for this run" cannot be made by any caller.

Suggested fix: give the loader a programmatic signal alongside the human-readable warnings — either a distinct SourceStatusPartial (with RepoGuidanceUnavailableReason and the rollup builder deciding what it means), or a structured field such as SourceInfo.Skipped []SkippedDefinition{Category, Agent, Reason string}. Then the rollup can state "N repo agents were skipped" in the posted review, and the required-on-match backstop has something to key on. Keep the strings for display; add the shape callers need.

Defensible either way on severity: if the maintainers accept that a skipped agent is never required-on-match in practice, this drops to minor. Evidence that would change my verdict: a rendering path I missed that puts SourceInfo.Warnings into the posted review body rather than only the reviewer prompt.

Reply inline to this comment.

[monit-reviewer]

Automated PR Review

Reviewed commit: 2fe76b359104
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	0
structure:repo-health	1
harness-engineering:repo-health	0
architecture:solid-reviewer-agnostic	0

structure:repo-health (1 finding)

Minor - internal/agents/agents.go:526

The 'skip if ErrInvalid, else fail' shape is now hand-duplicated four times (category-name validation at ~526, readRepoCategory at ~536, and the two branches inside readRepoAgents' loop at ~636 and ~645), each re-typing the same errors.Is(err, ErrInvalid) check and SkippedDefinition construction. This is exactly the kind of copy-pasted branch that drifts silently: a future error class that should also be skip-safe (or a fix to the classification logic itself) is one missed call site away from being applied to only three of the four. Extract a small helper, e.g. skipIfInvalid(err error, def SkippedDefinition) (SkippedDefinition, bool, error) or a closure capturing category.Name, and call it at all four sites so the skip predicate has one place to change.

Reviewer Coverage

• go:implementation-tests — ⚠️ failed; llm subprocess: Claude background job failed: exit 1 within 5s of spawn ×3
• structure:repo-health — complete (constrained); Findings scoped to internal/agents/agents.go per assignment; dossier.go and agents_test.go changes referenced in prior review threads are out of scope for this pass and not re-evaluated here.
• harness-engineering:repo-health — ⚠️ failed; llm subprocess: Claude background job failed: exit 1 within 5s of spawn ×3
• architecture:solid-reviewer-agnostic — ⚠️ failed; llm subprocess: Claude background job failed: exit 1 within 5s of spawn ×3

Inspected files (1)

• internal/agents/agents.go

Reviewer Diagnostics

• architecture:solid-reviewer-agnostic — failed: llm subprocess: Claude background job failed: exit 1 within 5s of spawn ×3
• go:implementation-tests — failed: llm subprocess: Claude background job failed: exit 1 within 5s of spawn ×3
• harness-engineering:repo-health — failed: llm subprocess: Claude background job failed: exit 1 within 5s of spawn ×3

0 PR discussion threads considered. 0 summarized; 0 resolved.

---

Completed in 3m 36s | ~$1.21 (est.) | claude-sonnet-5, claude-opus-5 | cr 0.10.277

Field	Value
Model	claude-sonnet-5, claude-opus-5
Reviewers	go:implementation-tests, structure:repo-health, harness-engineering:repo-health, architecture:solid-reviewer-agnostic
Engine	claude_cli · claude-sonnet-5, claude-opus-5
Reviewed by	cr · monit-reviewer
Duration	3m 36s wall · 4m 23s compute
Cost	~$1.21 (est.)
Tokens	unavailable

Per-workstream usage

• go:implementation-tests — claude-sonnet-5
  • In: unavailable
  • Out: unavailable
  • Cache read: unavailable
  • Cache create: unavailable
  • Cost: ~$0.00 (est.)
  • Duration: 29s
• structure:repo-health — claude-sonnet-5
  • In: 26
  • Out: 8.5k
  • Cache read: 823.0k
  • Cache create: 89.8k
  • Cost: ~$0.71 (est.)
  • Duration: 2m 40s
• harness-engineering:repo-health — claude-sonnet-5
  • In: unavailable
  • Out: unavailable
  • Cache read: unavailable
  • Cache create: unavailable
  • Cost: ~$0.00 (est.)
  • Duration: 29s
• architecture:solid-reviewer-agnostic — claude-opus-5
  • In: unavailable
  • Out: unavailable
  • Cache read: unavailable
  • Cache create: unavailable
  • Cost: ~$0.00 (est.)
  • Duration: 29s
• orchestrator-rollup — claude-sonnet-5
  • In: 6
  • Out: 543
  • Cache read: 111.6k
  • Cache create: 122.0k
  • Cost: ~$0.50 (est.)
  • Duration: 13s

[monit-reviewer]

The 'skip if ErrInvalid, else fail' shape is now hand-duplicated four times (category-name validation at ~526, readRepoCategory at ~536, and the two branches inside readRepoAgents' loop at ~636 and ~645), each re-typing the same errors.Is(err, ErrInvalid) check and SkippedDefinition construction. This is exactly the kind of copy-pasted branch that drifts silently: a future error class that should also be skip-safe (or a fix to the classification logic itself) is one missed call site away from being applied to only three of the four. Extract a small helper, e.g. skipIfInvalid(err error, def SkippedDefinition) (SkippedDefinition, bool, error) or a closure capturing category.Name, and call it at all four sites so the skip predicate has one place to change.

Reply inline to this comment.

[monit-reviewer]

Automated PR Review

Reviewed commit: e7fbab1a3ea1
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	0
structure:repo-health	0
harness-engineering:repo-health	0
architecture:solid-reviewer-agnostic	4

architecture:solid-reviewer-agnostic (4 findings)

Blocking - internal/agents/agents.go:79

Diff-caused lint failure: the repo's lint CI job (.github/workflows/ci.yml:73, make lint -> golangci-lint run) has misspell enabled, and this line trips it.

internal/agents/agents.go:79:68: `honour` is a misspelling of `honor` (misspell)
internal/dossier/dossier.go:1153:52: `honoured` is a misspelling of `honored` (misspell)
internal/dossier/dossier.go:1156:34: `honoured` is a misspelling of `honored` (misspell)

All three flagged lines are added by this diff, and golangci-lint run is clean on the surrounding untouched code, so this is caused by the change rather than pre-existing (project rule, order-of-authority #1; U-G1). Note dossier.go:1156 is not a comment — it is the literal "Guidance not honoured: " written into the dossier artifact, so the fix there changes rendered output and any dossier fixture/expectation asserting that string needs updating with it.

Fix: honour -> honor here, honoured -> honored at the two dossier.go sites, and re-run make lint.

Minor - internal/agents/agents.go:548

This category-level skip double-reports whenever the category's agents were themselves skipped, and it conflates two different situations under one reason string (U-S1: one entry should mean one thing).

When a category's only agent is malformed, agentSkips has already recorded {Category: cat, Agent: bad, Reason: ...} two lines above; this then appends a second entry {Category: cat, Reason: "no usable agents"} for the same root cause. TestRepoLoadSkipsCategoryWithNoUsableAgentsAndKeepsSiblings produces exactly that pair. Both are rendered: renderDossierRepoGuidance emits a Guidance not honoured: line per entry, and in the blocking all-malformed case RepoGuidanceUnavailableReason concatenates every entry into the posted review body — so the operator reads the same failure twice, once with the actionable detail and once without.

Separately, the same entry is emitted for a category directory that simply declares no agent subdirectories at all. SkippedDefinition is documented as "declared but not loaded"; an empty directory declared nothing, and calling it "no usable agents" is indistinguishable from the all-malformed case.

Fix: only record the category entry when the category yielded no agents and no per-agent skips were recorded for it (i.e. len(agentSkips) == 0), so the per-agent entries stand alone when they exist; and give the genuinely-empty case a reason that says so (e.g. "declares no agents").

Minor - internal/agents/agents.go:585

scopedToDefinition reclassifies the error but discards the cause: fmt.Errorf("%w: %s", ErrInvalid, ...) wraps only ErrInvalid, so the original gitprovider error — its Operation, the path it failed on, and any transport detail — is dropped from the chain entirely (U-L2: errors are contracts; a swallowed cause needs an explicit, commented decision, and the comment justifies the reclassification but not the discard).

Concrete impact: this now feeds the operator-facing text. In the all-agents-malformed case these reasons are appended to the posted review body via dossier.RepoGuidanceUnavailableReason, and the only thing an operator gets is agent cat:foo is missing prompt.md. If the provider returned NOT_FOUND for a reason other than an absent blob, there is nothing left to diagnose it with, and the previous behaviour (source classified unreadable with the provider error attached) is gone.

Fix: keep both in the chain — fmt.Errorf("%w: %s: %w", ErrInvalid, fmt.Sprintf(format, args...), err). This is safe for the existing classification: skipIfInvalid and classifyRepoCatalogError both test ErrInvalid first, so matching gitprovider.ErrNotFound as well does not change which branch is taken.

Minor - internal/agents/agents.go:82

The new Skipped slice field opts out of the copy convention this struct already has for its other slice field, so two existing "clone" sites silently stop being clones (U-G1: new public surface should carry the house pattern; U-D1: shared mutable state).

SourceInfo is copied in at least two places that deliberately deep-copy Warnings after a shallow struct copy:

• internal/pipeline/artifacts.go:116 — artifact.Sources[i].Warnings = append([]string(nil), catalog.Sources[i].Warnings...)
• internal/view/agents.go:257 (cloneSources) — same shape

Both carry Skipped only as an aliased slice header, so cloneSources no longer does what its name says and the pipeline artifact shares backing storage with the live catalog. It is harmless today because nothing mutates Skipped after load, but the defensive copy of Warnings exists precisely because relying on that is fragile, and a partial application of the convention is worse than none — the next reader cannot tell which fields are safe.

Fix: add out[i].Skipped = append([]SkippedDefinition(nil), sources[i].Skipped...) (and the artifacts.go equivalent) alongside the existing Warnings copies.

Reviewer Coverage

• go:implementation-tests — complete (constrained); inspected 4 of 5 files: internal/agents/agents.go, internal/agents/agents_test.go, internal/dossier/dossier.go, internal/pipeline/pipeline_test.go
• structure:repo-health — complete (constrained); inspected 1 of 5 files: internal/agents/agents.go; Assigned scope is limited to internal/agents/agents.go; the dossier.go warnings-surfacing gap raised in prior review rounds was not re-assessed here since it is out of scope for this reviewer's file assignment.
• harness-engineering:repo-health — complete (constrained); inspected 3 of 5 files: docs/review-guidance.md, internal/agents/agents.go, internal/agents/agents_test.go
• architecture:solid-reviewer-agnostic — complete (constrained); inspected 1 of 5 files: internal/agents/agents.go; Assignment is limited to internal/agents/agents.go; the change also touches internal/dossier/dossier.go, internal/agents/agents_test.go, internal/pipeline/pipeline_test.go and docs/review-guidance.md, which I read for context but cannot anchor findings on. Verification run locally at e7fbab1: go build ./... clean, go test ./internal/agents/... ./internal/dossier/... pass, gofmt -l clean, golangci-lint run ./internal/agents/... ./internal/dossier/... reports 3 diff-introduced misspell violations.

Inspected files (5)

• docs/review-guidance.md
• internal/agents/agents.go
• internal/agents/agents_test.go
• internal/dossier/dossier.go
• internal/pipeline/pipeline_test.go

0 PR discussion threads considered. 0 summarized; 0 resolved.

---

Completed in 8m 33s | ~$4.89 (est.) | claude-sonnet-5, claude-opus-5 | cr 0.10.277

Field	Value
Model	claude-sonnet-5, claude-opus-5
Reviewers	go:implementation-tests, structure:repo-health, harness-engineering:repo-health, architecture:solid-reviewer-agnostic
Engine	claude_cli · claude-sonnet-5, claude-opus-5
Reviewed by	cr · monit-reviewer
Duration	8m 33s wall · 17m 35s compute
Cost	~$4.89 (est.)
Tokens	122 in / 42.4k out

Per-workstream usage

• go:implementation-tests — claude-sonnet-5
  • In: 32
  • Out: 7.7k
  • Cache read: 1.2M
  • Cache create: 104.9k
  • Cost: ~$0.87 (est.)
  • Duration: 4m 26s
• structure:repo-health — claude-sonnet-5
  • In: 12
  • Out: 2.5k
  • Cache read: 316.1k
  • Cache create: 81.5k
  • Cost: ~$0.44 (est.)
  • Duration: 1m 26s
• harness-engineering:repo-health — claude-sonnet-5
  • In: 26
  • Out: 6.4k
  • Cache read: 937.8k
  • Cache create: 97.9k
  • Cost: ~$0.75 (est.)
  • Duration: 4m 11s
• architecture:solid-reviewer-agnostic — claude-opus-5
  • In: 46
  • Out: 25.0k
  • Cache read: 1.8M
  • Cache create: 122.9k
  • Cost: ~$2.30 (est.)
  • Duration: 7m 07s
• orchestrator-rollup — claude-sonnet-5
  • In: 6
  • Out: 708
  • Cache read: 117.4k
  • Cache create: 131.6k
  • Cost: ~$0.54 (est.)
  • Duration: 24s

[monit-reviewer]

scopedToDefinition reclassifies the error but discards the cause: fmt.Errorf("%w: %s", ErrInvalid, ...) wraps only ErrInvalid, so the original gitprovider error — its Operation, the path it failed on, and any transport detail — is dropped from the chain entirely (U-L2: errors are contracts; a swallowed cause needs an explicit, commented decision, and the comment justifies the reclassification but not the discard).

Concrete impact: this now feeds the operator-facing text. In the all-agents-malformed case these reasons are appended to the posted review body via dossier.RepoGuidanceUnavailableReason, and the only thing an operator gets is agent cat:foo is missing prompt.md. If the provider returned NOT_FOUND for a reason other than an absent blob, there is nothing left to diagnose it with, and the previous behaviour (source classified unreadable with the provider error attached) is gone.

Fix: keep both in the chain — fmt.Errorf("%w: %s: %w", ErrInvalid, fmt.Sprintf(format, args...), err). This is safe for the existing classification: skipIfInvalid and classifyRepoCatalogError both test ErrInvalid first, so matching gitprovider.ErrNotFound as well does not change which branch is taken.

Reply inline to this comment.

[monit-reviewer]

The new Skipped slice field opts out of the copy convention this struct already has for its other slice field, so two existing "clone" sites silently stop being clones (U-G1: new public surface should carry the house pattern; U-D1: shared mutable state).

SourceInfo is copied in at least two places that deliberately deep-copy Warnings after a shallow struct copy:

• internal/pipeline/artifacts.go:116 — artifact.Sources[i].Warnings = append([]string(nil), catalog.Sources[i].Warnings...)
• internal/view/agents.go:257 (cloneSources) — same shape

Both carry Skipped only as an aliased slice header, so cloneSources no longer does what its name says and the pipeline artifact shares backing storage with the live catalog. It is harmless today because nothing mutates Skipped after load, but the defensive copy of Warnings exists precisely because relying on that is fragile, and a partial application of the convention is worse than none — the next reader cannot tell which fields are safe.

Fix: add out[i].Skipped = append([]SkippedDefinition(nil), sources[i].Skipped...) (and the artifacts.go equivalent) alongside the existing Warnings copies.

Reply inline to this comment.

[monit-reviewer]

Diff-caused lint failure: the repo's lint CI job (.github/workflows/ci.yml:73, make lint -> golangci-lint run) has misspell enabled, and this line trips it.

internal/agents/agents.go:79:68: `honour` is a misspelling of `honor` (misspell)
internal/dossier/dossier.go:1153:52: `honoured` is a misspelling of `honored` (misspell)
internal/dossier/dossier.go:1156:34: `honoured` is a misspelling of `honored` (misspell)

All three flagged lines are added by this diff, and golangci-lint run is clean on the surrounding untouched code, so this is caused by the change rather than pre-existing (project rule, order-of-authority #1; U-G1). Note dossier.go:1156 is not a comment — it is the literal "Guidance not honoured: " written into the dossier artifact, so the fix there changes rendered output and any dossier fixture/expectation asserting that string needs updating with it.

Fix: honour -> honor here, honoured -> honored at the two dossier.go sites, and re-run make lint.

Reply inline to this comment.

[monit-reviewer]

This category-level skip double-reports whenever the category's agents were themselves skipped, and it conflates two different situations under one reason string (U-S1: one entry should mean one thing).

When a category's only agent is malformed, agentSkips has already recorded {Category: cat, Agent: bad, Reason: ...} two lines above; this then appends a second entry {Category: cat, Reason: "no usable agents"} for the same root cause. TestRepoLoadSkipsCategoryWithNoUsableAgentsAndKeepsSiblings produces exactly that pair. Both are rendered: renderDossierRepoGuidance emits a Guidance not honoured: line per entry, and in the blocking all-malformed case RepoGuidanceUnavailableReason concatenates every entry into the posted review body — so the operator reads the same failure twice, once with the actionable detail and once without.

Separately, the same entry is emitted for a category directory that simply declares no agent subdirectories at all. SkippedDefinition is documented as "declared but not loaded"; an empty directory declared nothing, and calling it "no usable agents" is indistinguishable from the all-malformed case.

Fix: only record the category entry when the category yielded no agents and no per-agent skips were recorded for it (i.e. len(agentSkips) == 0), so the per-agent entries stand alone when they exist; and give the genuinely-empty case a reason that says so (e.g. "declares no agents").

Reply inline to this comment.

[monit-reviewer]

Automated PR Review

Reviewed commit: 21b177b9132a
Profile: claude-monit-reviewer - Posting as: monit-reviewer

Summary

Reviewer	Findings
go:implementation-tests	0
structure:repo-health	0
harness-engineering:repo-health	1
architecture:solid-reviewer-agnostic	2

harness-engineering:repo-health (1 finding)

Nits - docs/review-guidance.md:62

The doc quotes the dossier skip-line label as "Guidance not honoured" (British spelling), but internal/dossier/dossier.go writes the literal string "Guidance not honored: " (American spelling, and with a trailing colon the doc omits). Since this is one of the small set of files that map cr's on-disk artifacts to what an operator or agent should grep for, an exact-string mismatch here is exactly the kind of drift this doc exists to prevent — someone searching a dossier for the documented label won't find it. Fix: quote the string exactly as emitted, "Guidance not honored: ".

architecture:solid-reviewer-agnostic (2 findings)

Minor - internal/agents/agents.go:82

U-G1: the new exported slice field extends a per-field deep-copy obligation that lives in consumers, and two of the four copy sites were not updated. Provenance.SourceInfo (line 150) and provenanceFromSource (line 906) now clone Skipped, but the two out-of-package sites that exist precisely to break aliasing still enumerate only Warnings: internal/pipeline/artifacts.go:110-116 (Sources: append([]agents.SourceInfo(nil), catalog.Sources...) then artifact.Sources[i].Warnings = append(...)) and internal/view/agents.go:249-258 (cloneSources). After a shallow struct copy those two carry a Skipped slice whose backing array is shared with the live catalog. Nothing appends to Skipped post-load today, so this is latent rather than a live bug — but the convention is now inconsistent, and a reader of cloneSources cannot tell that one slice field is deliberately shared.

Suggested fix: put the copy contract on the type instead of re-deriving it at each site — add func (s SourceInfo) Clone() SourceInfo (and, if kept, func (p Provenance) Clone() Provenance) in this file that copies both slice fields, and have artifacts.go and view/agents.go call it. Then a future slice field on SourceInfo is safe by construction rather than by four call sites remembering.

Minor - internal/agents/agents.go:115

U-O1 (the premature-abstraction direction): Provenance.Skipped has no producer anywhere in the tree, so the round-trip it exists to preserve cannot currently occur. The only SourceInfo -> Provenance conversion is provenanceFromSource, called at line 390 (file sources, whose loader never skips), 840 and 884 (only to compute .String()); and repo agents are built with the provenance value constructed before loadRepoSource runs, so Provenance.Skipped is always empty. No test sets it either. It is state carried, copied in two places, and serialized, that is unreachable.

It is also worth deciding whether it should ever be populated: Provenance.SourceInfo() is invoked per agent (internal/pipeline/artifacts.go:128, internal/view/agents.go:72,97), so a populated Skipped would stamp the whole source-wide skip list onto every loaded agent's rendered/serialized Source — N copies of a fact that belongs to the source, not to any one agent that loaded fine.

Suggested fix: drop Provenance.Skipped and its two copy sites (lines 150, 906) and let skips live only on SourceInfo, where loadRepoSource actually writes them and internal/dossier actually reads them. If a real round-trip need appears later, reintroduce it together with the producer and a test.

Reviewer Coverage

• go:implementation-tests — complete (constrained); inspected 4 of 5 files: internal/agents/agents.go, internal/agents/agents_test.go, internal/dossier/dossier.go, internal/pipeline/pipeline_test.go
• structure:repo-health — complete (constrained); inspected 1 of 5 files: internal/agents/agents.go; Assigned scope is internal/agents/agents.go only; the finding below names two out-of-scope call sites (internal/pipeline/artifacts.go, internal/view/agents.go) as evidence but no fix can be applied to them from this file.
• harness-engineering:repo-health — complete (constrained); inspected 3 of 5 files: docs/review-guidance.md, internal/agents/agents.go, internal/agents/agents_test.go; internal/dossier/dossier.go and internal/pipeline/pipeline_test.go are part of this PR's diff but not in the assigned file list, so their content was read only as context for judging the three assigned files, not reviewed for independent findings.
• architecture:solid-reviewer-agnostic — complete (constrained); inspected 1 of 5 files: internal/agents/agents.go; Assigned file is internal/agents/agents.go only; internal/dossier/dossier.go, internal/agents/agents_test.go and docs/review-guidance.md were read for context but are not reported on. The core design decision (skip a malformed definition rather than refuse the source, keep SourceStatusInvalid when nothing loads) is sound and adequately covered by the nine new load tests; findings below are confined to the shape of the new exported state. Verification run: go build ./..., go test ./internal/agents/... ./internal/dossier/... and go vet ./internal/agents/... all pass at 21b177b. make lint was not run (golangci-lint not invoked); the previously reported 'honour' misspell no longer appears in any Go file.

Inspected files (5)

• docs/review-guidance.md
• internal/agents/agents.go
• internal/agents/agents_test.go
• internal/dossier/dossier.go
• internal/pipeline/pipeline_test.go

0 PR discussion threads considered. 0 summarized; 0 resolved.

---

Completed in 6m 34s | ~$4.64 (est.) | claude-sonnet-5, claude-opus-5 | cr 0.10.277

Field	Value
Model	claude-sonnet-5, claude-opus-5
Reviewers	go:implementation-tests, structure:repo-health, harness-engineering:repo-health, architecture:solid-reviewer-agnostic
Engine	claude_cli · claude-sonnet-5, claude-opus-5
Reviewed by	cr · monit-reviewer
Duration	6m 34s wall · 12m 43s compute
Cost	~$4.64 (est.)
Tokens	126 in / 36.7k out

Per-workstream usage

• go:implementation-tests — claude-sonnet-5
  • In: 28
  • Out: 6.1k
  • Cache read: 952.0k
  • Cache create: 90.9k
  • Cost: ~$0.72 (est.)
  • Duration: 3m 38s
• structure:repo-health — claude-sonnet-5
  • In: 32
  • Out: 7.5k
  • Cache read: 1.2M
  • Cache create: 105.1k
  • Cost: ~$0.88 (est.)
  • Duration: 2m 13s
• harness-engineering:repo-health — claude-sonnet-5
  • In: 20
  • Out: 4.8k
  • Cache read: 693.9k
  • Cache create: 98.0k
  • Cost: ~$0.65 (est.)
  • Duration: 1m 22s
• architecture:solid-reviewer-agnostic — claude-opus-5
  • In: 42
  • Out: 17.1k
  • Cache read: 1.5M
  • Cache create: 106.6k
  • Cost: ~$1.83 (est.)
  • Duration: 5m 06s
• orchestrator-rollup — claude-sonnet-5
  • In: 4
  • Out: 1.2k
  • Cache read: 27.8k
  • Cache create: 142.7k
  • Cost: ~$0.56 (est.)
  • Duration: 23s

[monit-reviewer]

The doc quotes the dossier skip-line label as "Guidance not honoured" (British spelling), but internal/dossier/dossier.go writes the literal string "Guidance not honored: " (American spelling, and with a trailing colon the doc omits). Since this is one of the small set of files that map cr's on-disk artifacts to what an operator or agent should grep for, an exact-string mismatch here is exactly the kind of drift this doc exists to prevent — someone searching a dossier for the documented label won't find it. Fix: quote the string exactly as emitted, "Guidance not honored: ".

Reply inline to this comment.

[monit-reviewer]

U-G1: the new exported slice field extends a per-field deep-copy obligation that lives in consumers, and two of the four copy sites were not updated. Provenance.SourceInfo (line 150) and provenanceFromSource (line 906) now clone Skipped, but the two out-of-package sites that exist precisely to break aliasing still enumerate only Warnings: internal/pipeline/artifacts.go:110-116 (Sources: append([]agents.SourceInfo(nil), catalog.Sources...) then artifact.Sources[i].Warnings = append(...)) and internal/view/agents.go:249-258 (cloneSources). After a shallow struct copy those two carry a Skipped slice whose backing array is shared with the live catalog. Nothing appends to Skipped post-load today, so this is latent rather than a live bug — but the convention is now inconsistent, and a reader of cloneSources cannot tell that one slice field is deliberately shared.

Suggested fix: put the copy contract on the type instead of re-deriving it at each site — add func (s SourceInfo) Clone() SourceInfo (and, if kept, func (p Provenance) Clone() Provenance) in this file that copies both slice fields, and have artifacts.go and view/agents.go call it. Then a future slice field on SourceInfo is safe by construction rather than by four call sites remembering.

Reply inline to this comment.

[monit-reviewer]

U-O1 (the premature-abstraction direction): Provenance.Skipped has no producer anywhere in the tree, so the round-trip it exists to preserve cannot currently occur. The only SourceInfo -> Provenance conversion is provenanceFromSource, called at line 390 (file sources, whose loader never skips), 840 and 884 (only to compute .String()); and repo agents are built with the provenance value constructed before loadRepoSource runs, so Provenance.Skipped is always empty. No test sets it either. It is state carried, copied in two places, and serialized, that is unreachable.

It is also worth deciding whether it should ever be populated: Provenance.SourceInfo() is invoked per agent (internal/pipeline/artifacts.go:128, internal/view/agents.go:72,97), so a populated Skipped would stamp the whole source-wide skip list onto every loaded agent's rendered/serialized Source — N copies of a fact that belongs to the source, not to any one agent that loaded fine.

Suggested fix: drop Provenance.Skipped and its two copy sites (lines 150, 906) and let skips live only on SourceInfo, where loadRepoSource actually writes them and internal/dossier actually reads them. If a real round-trip need appears later, reintroduce it together with the producer and a test.

Reply inline to this comment.