What happened
On PR #4602, human reviewer @PatAKnight flagged on Sep 11 that the new ScalarAggregationType public type export from scorecard-common was only consumed by test files, suggesting a local type alias instead of expanding public API surface. The review agent ran 9 times (costing ~$60.87) but did not raise this concern until Sep 14—three days later—and framed it as a changeset bump-level issue rather than an API surface concern. The agent's framing ("changeset marks as minor but changes are limited to type annotation refactoring") was less actionable than the human's direct question of whether the export was needed at all.
What could go better
The review agent lacks specific guidance about evaluating whether new public exports from shared packages (scorecard-common, scorecard-node) are consumed by production code. The scorecard workspace AGENTS.md has detailed guidance for metric IDs, threshold resolution, and provider architecture, but nothing about public API surface management for shared packages. This gap meant the agent focused on style/convention concerns while missing the higher-value architectural question. I'm moderately confident this guidance would have helped on the first review pass: the agent already reads workspace AGENTS.md and follows its review guidance sections (e.g., threshold change severity). Adding explicit API surface guidance should produce similar compliance. Uncertainty: the agent might still not catch every case, since determining whether an export is test-only requires cross-file analysis.
Proposed change
Add a "Review guidance for public API changes" section to workspaces/scorecard/AGENTS.md (after the existing "Review guidance for threshold changes" section). The guidance should instruct: (1) When a PR adds or modifies public type exports in scorecard-common or scorecard-node, verify the exported types are consumed by production (non-test) code in at least one other package. (2) If a new public export is only consumed by test files, flag it as [medium] and suggest using a local type alias within the test file or test utilities instead. (3) If the author intentionally broadens usage to justify the export (as happened in this PR), accept that as a valid resolution but verify the changeset bump level reflects the API surface addition.
Validation criteria
On the next scorecard PR that adds a public type export to scorecard-common or scorecard-node, the review agent should flag the export and ask whether it is consumed by non-test production code. Verify over the next 3 scorecard PRs that touch shared package exports. Success: the agent raises the API surface concern on the first review pass (not the 4th+), and the finding is actionable enough that the author responds to it.
Generated by retro agent from #4602
What happened
On PR #4602, human reviewer @PatAKnight flagged on Sep 11 that the new
ScalarAggregationTypepublic type export fromscorecard-commonwas only consumed by test files, suggesting a local type alias instead of expanding public API surface. The review agent ran 9 times (costing ~$60.87) but did not raise this concern until Sep 14—three days later—and framed it as a changeset bump-level issue rather than an API surface concern. The agent's framing ("changeset marks as minor but changes are limited to type annotation refactoring") was less actionable than the human's direct question of whether the export was needed at all.What could go better
The review agent lacks specific guidance about evaluating whether new public exports from shared packages (
scorecard-common,scorecard-node) are consumed by production code. The scorecard workspace AGENTS.md has detailed guidance for metric IDs, threshold resolution, and provider architecture, but nothing about public API surface management for shared packages. This gap meant the agent focused on style/convention concerns while missing the higher-value architectural question. I'm moderately confident this guidance would have helped on the first review pass: the agent already reads workspace AGENTS.md and follows its review guidance sections (e.g., threshold change severity). Adding explicit API surface guidance should produce similar compliance. Uncertainty: the agent might still not catch every case, since determining whether an export is test-only requires cross-file analysis.Proposed change
Add a "Review guidance for public API changes" section to
workspaces/scorecard/AGENTS.md(after the existing "Review guidance for threshold changes" section). The guidance should instruct: (1) When a PR adds or modifies public type exports inscorecard-commonorscorecard-node, verify the exported types are consumed by production (non-test) code in at least one other package. (2) If a new public export is only consumed by test files, flag it as [medium] and suggest using a local type alias within the test file or test utilities instead. (3) If the author intentionally broadens usage to justify the export (as happened in this PR), accept that as a valid resolution but verify the changeset bump level reflects the API surface addition.Validation criteria
On the next scorecard PR that adds a public type export to scorecard-common or scorecard-node, the review agent should flag the export and ask whether it is consumed by non-test production code. Verify over the next 3 scorecard PRs that touch shared package exports. Success: the agent raises the API surface concern on the first review pass (not the 4th+), and the finding is actionable enough that the author responds to it.
Generated by retro agent from #4602