What happened
In PR #4474, the review agent approved the test changes without flagging a test adequacy gap. Human reviewer gustavolira identified that the test mock for predecessor deployment collection only branched on input.from === DORA_PREDECESSOR_COLLECT_FROM, meaning that removing fetchItemsLimit: predecessorRange.fetchItemsLimit from the production code would leave the entire test suite green while production starts requesting 1000 deployments since 1970 on every DORA run.
The reviewer also noted that the catch/fallback path in collectSuccessfulProductionDeploymentsWithPreWindowBoundary had no test coverage — a transient rate limit during predecessor lookup would produce a misleading error message pointing at deployment data rather than the API.
The review agent's test assessment was limited to verifying that tests existed for the new functionality, not that they adequately covered the parameter contracts between callers and collectors.
What could go better
The scorecard workspace AGENTS.md has no guidance on what constitutes adequate test coverage for collector interactions. The existing mock pattern (mockResolvedValueOnce with conditional returns based on a single input field) makes it easy to add tests that pass without actually verifying the full contract between provider and collector.
The human reviewer caught this because they understood that fetchItemsLimit was the key behavioral difference between the normal collect and the predecessor collect — without it, the predecessor query degrades to an unbounded scan. This is a pattern-level issue: any time a provider passes structured input to a collector, tests should assert on the full input shape, not just the fields used for mock branching.
I am moderately confident this would help the review agent. The agent already reads and evaluates test files during review — giving it explicit criteria for what constitutes sufficient collector parameter testing would raise the bar from "tests exist" to "tests verify the contract." However, the agent's ability to reason about which parameters are behaviorally significant (vs. cosmetic) may still be limited without the pipeline context from proposal 1.
This proposal complements the pipeline guidance proposal — together, they give the agent both the domain context to understand why parameters matter and the testing convention to verify they're asserted.
Proposed change
Add a subsection to the proposed "DORA Collector Pipeline" section (or as a standalone section) in workspaces/scorecard/AGENTS.md titled "Testing conventions for collector interactions", covering:
-
Assert full collector input contracts: When a provider calls collectorsService.collect(), tests must use toHaveBeenCalledWith (or equivalent) to assert on the full input object — including from, to, fetchItemsLimit, and any other parameters — not just the fields used for mock branching. Dropping a parameter from the production call must cause a test failure.
-
Cover error/fallback paths: When collector calls are wrapped in try/catch with fallback behavior (e.g., falling back to fewer deployments on rate limit), add a test case using mockRejectedValueOnce to verify that (a) the fallback produces the correct output and (b) the warning message distinguishes the failure type.
-
Review checklist item: When reviewing test changes for DORA providers, verify that mock assertions cover the distinguishing parameters of each collector call. Flag tests where the mock branches on one field but the production code sends additional fields that affect behavior.
Validation criteria
On the next 2 PRs that modify DORA provider tests in the scorecard workspace, the review agent should flag collector mock assertions that only branch on input.from without verifying other behavioral parameters like fetchItemsLimit or to. The review agent should also flag new try/catch blocks around collector calls that lack corresponding test coverage for the error path.
Generated by retro agent from #4474
What happened
In PR #4474, the review agent approved the test changes without flagging a test adequacy gap. Human reviewer gustavolira identified that the test mock for predecessor deployment collection only branched on
input.from === DORA_PREDECESSOR_COLLECT_FROM, meaning that removingfetchItemsLimit: predecessorRange.fetchItemsLimitfrom the production code would leave the entire test suite green while production starts requesting 1000 deployments since 1970 on every DORA run.The reviewer also noted that the catch/fallback path in
collectSuccessfulProductionDeploymentsWithPreWindowBoundaryhad no test coverage — a transient rate limit during predecessor lookup would produce a misleading error message pointing at deployment data rather than the API.The review agent's test assessment was limited to verifying that tests existed for the new functionality, not that they adequately covered the parameter contracts between callers and collectors.
What could go better
The scorecard workspace AGENTS.md has no guidance on what constitutes adequate test coverage for collector interactions. The existing mock pattern (
mockResolvedValueOncewith conditional returns based on a single input field) makes it easy to add tests that pass without actually verifying the full contract between provider and collector.The human reviewer caught this because they understood that
fetchItemsLimitwas the key behavioral difference between the normal collect and the predecessor collect — without it, the predecessor query degrades to an unbounded scan. This is a pattern-level issue: any time a provider passes structured input to a collector, tests should assert on the full input shape, not just the fields used for mock branching.I am moderately confident this would help the review agent. The agent already reads and evaluates test files during review — giving it explicit criteria for what constitutes sufficient collector parameter testing would raise the bar from "tests exist" to "tests verify the contract." However, the agent's ability to reason about which parameters are behaviorally significant (vs. cosmetic) may still be limited without the pipeline context from proposal 1.
This proposal complements the pipeline guidance proposal — together, they give the agent both the domain context to understand why parameters matter and the testing convention to verify they're asserted.
Proposed change
Add a subsection to the proposed "DORA Collector Pipeline" section (or as a standalone section) in
workspaces/scorecard/AGENTS.mdtitled "Testing conventions for collector interactions", covering:Assert full collector input contracts: When a provider calls
collectorsService.collect(), tests must usetoHaveBeenCalledWith(or equivalent) to assert on the full input object — includingfrom,to,fetchItemsLimit, and any other parameters — not just the fields used for mock branching. Dropping a parameter from the production call must cause a test failure.Cover error/fallback paths: When collector calls are wrapped in try/catch with fallback behavior (e.g., falling back to fewer deployments on rate limit), add a test case using
mockRejectedValueOnceto verify that (a) the fallback produces the correct output and (b) the warning message distinguishes the failure type.Review checklist item: When reviewing test changes for DORA providers, verify that mock assertions cover the distinguishing parameters of each collector call. Flag tests where the mock branches on one field but the production code sends additional fields that affect behavior.
Validation criteria
On the next 2 PRs that modify DORA provider tests in the scorecard workspace, the review agent should flag collector mock assertions that only branch on
input.fromwithout verifying other behavioral parameters likefetchItemsLimitorto. The review agent should also flag new try/catch blocks around collector calls that lack corresponding test coverage for the error path.Generated by retro agent from #4474