fix(repo-finder): grade de facto maintainers whose association under-reports - #34
fix(repo-finder): grade de facto maintainers whose association under-reports#34imkp1 wants to merge 2 commits into
Conversation
…reports author_association reports public org membership only, so a private member or a lead never added to the org comments as CONTRIBUTOR and their confirmation is discarded. Add a third set, strictly narrower than the 3c union: authors of >=5 of the last 20 merged PRs, bots excluded. Grade on association union de facto. $mca and maintainer_comment_assoc stay association-only, and the union stays ungraded, so the distinction PR gaurav0107#25 drew is unchanged. Threshold measured across 18 repos, not picked: with bots removed the per-author distribution has a clean gap between 3 and 5 of 20. All six non-bot authors at >=5 are insiders whose association under-reports. A cutoff of 3 admits 23 authors and re-creates the commit-rank error gaurav0107#25 fixed. --defacto is optional; an absent list restores the previous behaviour byte for byte. Reuses the merged-PR list from 3a, so no extra API calls.
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe repo-finder workflow now derives de facto maintainers from recent merged PR authors, passes them to the triage filter, and includes them in maintainer signal grading while retaining association-only comment metadata. Tests cover de facto confirmations and union-only exclusions. ChangesDe facto maintainer signal
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RepoFinder
participant triage_filter.sh
participant jq
RepoFinder->>RepoFinder: Derive DEFACTO from merged PR authors
RepoFinder->>triage_filter.sh: Pass --defacto DEFACTO
triage_filter.sh->>jq: Load maintainer and de facto sets
jq->>triage_filter.sh: Grade association or de facto comments
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/scripts/test_triage_filter.sh (1)
204-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the legacy invocation without
--defacto.This test now always supplies the new file, so it does not verify the explicitly supported backward-compatible path. Add a run without
--defactoand assert that association-only grading remains unchanged.Based on the PR objective, preserving behavior when
--defactois absent is part of the contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/scripts/test_triage_filter.sh` around lines 204 - 205, Extend the test around the FILTER invocation to also run without the --defacto option, using the same issues, maintainers, and timestamp inputs. Assert that association-only grading matches the existing expected output, preserving the legacy invocation behavior while retaining the current --defacto coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agents/repo-finder.md`:
- Line 775: Update the “Maintainer signal” documentation to state that
gradeability requires association or de facto maintainer membership, while
keeping the broader contributor union ungraded. Preserve the existing scoring
values and de facto threshold, and express the rule as association ∪ de facto
rather than association alone.
- Around line 622-623: Update the DEFACTO generation pipeline using MERGED_PRS
so jq excludes missing or null author logins before grouping, preventing null
candidates. Preserve valid zero-result output, but propagate actual jq, sort, or
bot-filter failures instead of converting them to an empty DEFACTO file; avoid
grep pipeline status behavior that treats expected no matches as an upstream
failure.
---
Nitpick comments:
In `@tests/scripts/test_triage_filter.sh`:
- Around line 204-205: Extend the test around the FILTER invocation to also run
without the --defacto option, using the same issues, maintainers, and timestamp
inputs. Assert that association-only grading matches the existing expected
output, preserving the legacy invocation behavior while retaining the current
--defacto coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ecc6462f-d812-4003-81a1-d9e40b18e26a
📒 Files selected for processing (3)
agents/repo-finder.mdscripts/orchestrator/triage_filter.shtests/scripts/test_triage_filter.sh
…g them
Three defects in the de facto tier as first written.
$defacto was documented as a strict subset of $maint and only unioned into
the grading set. It is not a subset by construction: $maint ranks contributors
by all-time commits, so a lead who joined recently clears 5 of the last 20
merged PRs and still sits outside the top 25, and $maint's association legs
cannot catch them either — under-reported association is why they are in the
de facto list at all. Such a lead was graded but absent from $mc, which drives
the triage gate, maintainer_commented and last_maintainer_comment. The issue
then failed the gate and never reached the ranker, which is the silent discard
this tier exists to fix; when a good-first-issue label rescued it, the row read
maintainer_commented: false beside maintainer_signal: confirms. Union $defacto
into $mc instead of assuming containment.
The derivation masked its own failures. grep exits 1 on no match, and no match
is the ordinary case here, so `|| : > "$DEFACTO"` could not tell an empty and
correct result from a jq that died. Both filters now run inside jq and a real
failure is fatal. It also counted ghost authors: a deleted account serializes
as author: null, and five of them grouped into a literal `null` maintainer.
Guard with `// empty`, as 3c already does. An unset $BOT_NAMES would make
test("") match every login and empty the set in silence; assert it instead.
Rubric prose said association is the prerequisite, which the de facto tier
contradicts. Gradeable is association ∪ de facto; the 3c union stays ungraded.
Tests: fixture #928 is a de facto lead outside the union, labelled `bug` only
so no label can rescue it at the gate — it fails without the $mc change. A
legacy run without --defacto asserts #927 falls back to `none`, #928 fails the
gate, and every other verdict is byte-identical.
61/64 scripts pass; test_audit_impact, test_parse_workflows and test_smoke_gate
fail identically on main here for missing yq and GNU timeout.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Fixes #32.
author_associationreports public org membership only. A private org member, or a lead who was never added to the org, comments asCONTRIBUTOR. Signal grading reads$mca, which is association only, so those confirmations are discarded and the issue gradesnone. The failure is silent: nothing in the output separates "no maintainer commented" from "the maintainer commented and we did not count them".I originally filed #32 saying
$mcashould just use the$maintfallback. That was wrong and I have corrected the issue. The$mca/$mcsplit is deliberate, stated in the code, in the rubric prose, and asserted by a fixture that says PR #25's distinction must survive. The union is top-25 by commit count, and commit rank is not authority.What this does
Adds a third set, strictly narrower than the union: authors of >= 5 of the last 20 merged PRs, bots excluded. Grading runs on
association ∪ de facto.$mcaandmaintainer_comment_assocstay association onlynoneThreshold
Measured, not picked. Per-author share of the last 20 merged PRs across 18 Go and AI repos:
Clean gap between 3 and 5 once bots are out. All six non-bot authors at >= 5 are insiders whose association under-reports, none of them public org members. A cutoff of 3 admits 23 authors and re-creates the commit-rank error #25 fixed.
Notes
--defactois optional. An absent list reads as empty, which restores the previous behaviour exactly. I diffed the output againstmainon a union-only fixture to confirm.$MERGED_PRSfrom 3a. No extra API calls.$BOT_NAMESmisses handles like...-cherrypick-robot, wherebotis not a whole word. Safe: the filter's own bot regex is a superset and runs before grading. Noted in a comment rather than duplicating the guard.Tests
New fixture #927: a de facto lead commenting as
CONTRIBUTORmust gradeconfirmswhilemaintainer_comment_assocstaysfalse. Fails before the change withunknown argument: --defacto, passes after.61/64scripts pass. The three failures (test_audit_impact,test_parse_workflows,test_smoke_gate) fail identically onmainin my environment for missingyqand GNUtimeout. shellcheck is not installed locally, so CI covers it.Summary by CodeRabbit