-
Notifications
You must be signed in to change notification settings - Fork 0
fix(#369): cap evidence issues at 5 per parent #417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -123,6 +123,59 @@ Use multiple searches with different keyword combinations if the first returns n | |
|
|
||
| When skipping, note the duplicate in your `summary` field so the human understands what was filtered and why. | ||
|
|
||
| ### Evidence issue cap | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] instruction ambiguity / ordering The instructions do not explicitly state whether the evidence cap check should run before or after the broader duplicate search. Running the cap check first (one API call) could save the cost of a subagent dedup search when the cap is already reached. Suggested fix: Add a sentence like: 'For evidence proposals, run the cap check before the broader duplicate search — if the cap is already reached, you can skip both the evidence proposal and its dedup search.' |
||
|
|
||
| Evidence proposals use the title format `Evidence for #N` (where `N` is | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] convention-enforcement The 'Evidence for #N' title convention is only enforced via prompt text. The output schema and post-retro script do not validate this format, so the cap check's accuracy depends on consistent agent compliance. Suggested fix: Consider adding title-format validation in the post-retro script in a future PR. |
||
| the parent issue number). This convention is how the retro agent links | ||
| data points back to a systemic issue — always use this exact format | ||
| when filing evidence. | ||
|
|
||
| Before filing an evidence proposal, check how many open evidence issues | ||
| already exist for that parent. | ||
|
|
||
| Dispatch a subagent to count existing evidence issues: | ||
|
|
||
| ```bash | ||
| gh api \ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [low] API contract / query correctness The gh api query uses in:title with a quoted phrase, which matches substrings. An issue titled e.g. 'Revert: Evidence for #42' would also match, potentially inflating the count. Given the cap is advisory and the title convention is defined by this skill itself, false-positive matches are extremely unlikely in practice. |
||
| "search/issues?q=repo:<target_repo>+is:issue+is:open+in:title+%22Evidence+for+%23N%22&per_page=1" \ | ||
| --jq '.total_count' | ||
| ``` | ||
|
|
||
| Replace `<target_repo>` with the proposal's target repository and `N` | ||
| with the parent issue number. | ||
|
|
||
| If the count query fails, returns a non-numeric result, or the | ||
| subagent errors out, treat the count as >= 5 (fail closed) and skip | ||
| the evidence proposal. Do not file evidence when the cap cannot be | ||
| verified. | ||
|
|
||
| **If the count is >= 5:** | ||
|
|
||
| 1. **Skip the evidence proposal entirely.** Do not include it in the | ||
| `proposals` array. | ||
| 2. **Note the cap in your summary.** Mention that the pattern is | ||
| already well-documented with sufficient data points. Include the | ||
| parent issue number and the current PR/issue as an additional data | ||
| point so the information is not lost — it just does not warrant a | ||
| new issue. | ||
|
|
||
| Example summary note: | ||
|
|
||
| > Pattern for #N is well-documented (≥5 evidence issues). This | ||
| > PR adds another data point but does not warrant a new evidence | ||
| > issue. | ||
|
|
||
| **If the count is < 5:** proceed with filing the evidence proposal | ||
| normally. | ||
|
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] edge-case-precision Concurrency note states two concurrent retro runs can exceed the cap 'by one.' This is correct for exactly two runs but does not address N>2 concurrent runs, which could exceed the cap by N-1. Given the cap is advisory, this is acceptable but imprecise. Suggested fix: Change 'exceeding the cap by one' to 'briefly exceeding the cap' or note the overshoot is bounded by the number of concurrent runs. |
||
| Only open evidence issues count toward the cap — closed issues do not. | ||
|
|
||
| **Concurrency note:** Two retro runs triggered by different PRs can | ||
| both query the count before either files, causing both to see the same | ||
| value and potentially exceeding the cap by one. This is an accepted | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [medium] incorrect claim about runtime mechanism The concurrency note states the concurrency group in retro.yml limits the window for the evidence cap race condition. However, retro.yml concurrency group is scoped per triggering PR/issue (fullsend-retro-{source_repo}-{pr_or_issue_number}), not per parent issue. Two retro runs from different PRs filing evidence for the same parent issue have different concurrency groups and run fully in parallel. The stated mitigation does not function for the described race. Suggested fix: Remove the incorrect claim about the concurrency group. Replace with: Concurrency note: Two retro runs triggered by different PRs can both query the count before either files, causing both to see the same value and potentially exceeding the cap by one. This is an accepted limitation — the cap is advisory and a brief overshoot does not meaningfully harm the issue tracker. |
||
| limitation — the cap is advisory and a brief overshoot does not | ||
| meaningfully harm the issue tracker. | ||
|
|
||
| ## Localization guidance | ||
|
|
||
| When deciding where a proposed change belongs: | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[low] document-structure
The new '### Evidence issue cap' is a subsection of '## Before proposing' but the existing duplicate-check content under the same H2 has no heading, creating a structural asymmetry.
Suggested fix: Consider restructuring with two explicit H3 subsections: '### Duplicate check' and '### Evidence issue cap'.