fix(repo-finder): grade maintainer signal from comment text, not association#31
Merged
Merged
Conversation
The approval bonus was set from comment association alone, so any comment by an OWNER/MEMBER/COLLABORATOR earned the full +3 and rendered as "Maintainer approved: yes" — refutations and bare links included. Replace the boolean with a signal graded from the comment text: invites_pr +5, confirms +3, neutral +1, none 0. Association stays a prerequisite and stops being sufficient. Close two claims the association and assignee tests cannot see: - a maintainer who confirms and claims in one sentence; the claim test now runs before the tiering, which would otherwise score it as approval - a non-maintainer claim, windowed at 14 days so an abandoned one does not fence the issue off permanently Strip links and mentions before grading and require prose in what remains. A comment that is only a pointer states no position. Reads the bulk issue payload already in hand; no additional API calls.
Match before measuring. The prose floor ran ahead of the pattern match, so "PRs welcome!" — 12 characters, the strongest signal there is — graded as no signal. The floor now separates a remark from a pointer and decides neutral-or-none only. Narrow the claim test. Bare "on our side" also matches "the regression is on our side", which owns the defect and invites a fix rather than claiming it. Require an intent to act. Guard the claim timestamp. A comment can serialize without createdAt, and null | fromdateiso8601 throws, aborting the batch and taking every other issue with it. Undated now reads as expired rather than as a live claim.
📝 WalkthroughWalkthroughThe triage filter now classifies maintainer engagement into graded signals, skips active outsider claims, and emits the new field. Repo-finder scoring, shortlist schema, examples, and tests are updated from ChangesMaintainer signal flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant IssueData
participant triage_filter.sh
participant repo_finder
participant ShortlistSchema
IssueData->>triage_filter.sh: Process issue comments
triage_filter.sh->>triage_filter.sh: Classify maintainer_signal and live claims
triage_filter.sh->>repo_finder: Emit KEEP or SKIP result
repo_finder->>repo_finder: Apply signal scoring and dispute skips
repo_finder->>ShortlistSchema: Validate maintainer_signal
Possibly related PRs
🚥 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: 1
🤖 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 `@tests/scripts/test_triage_filter.sh`:
- Around line 16-17: Update the RECENT and STALE fixture definitions in
tests/scripts/test_triage_filter.sh to derive timestamps relative to jq’s
current time using now and todateiso8601, keeping RECENT within the 14-day
live-claim window while preserving STALE as an older abandoned claim. Avoid
hardcoded dates and shell-specific timestamp commands.
🪄 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: 9bcd7db0-a256-4f05-a1f2-beda4b70280e
📒 Files selected for processing (4)
agents/repo-finder.mdschemas/repo_shortlist.schema.jsonscripts/orchestrator/triage_filter.shtests/scripts/test_triage_filter.sh
Two regexes credited or skipped on text they had not actually matched. - `reproduc` matched its own negation, so a maintainer who could not reproduce graded `confirms` (+3). Negated phrases are now deleted from the graded text before the tier test, not short-circuited on, so a second maintainer who did reproduce it still earns the tier. Not a decline: the report may still be sound, so it falls to `neutral`. - `can i (take|work on|pick up)` had no object anchor, so "take a look at this" read as a claim and skipped the issue. Anchored on `(this|it)` like every other alternative. Regression tests for both, plus guards that neither over-corrects.
Contributor
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/scripts/test_triage_filter.sh (1)
382-384: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd missing
KEEPassertion for issue 924.Before asserting the
maintainer_signalfield, please consider explicitly asserting that issue 924 is kept by the triage filter. This maintains test consistency with the other fixtures (e.g., 923, 925) and provides a clearer error message if the issue is accidentally filtered out upstream.♻️ Proposed refactor
# Negations are deleted from the graded text, not short-circuited on: one # maintainer missing it cannot mute another who reproduced it. +want 924 KEEP "one maintainer reproducing overrides another missing it" [ "$(field 924 maintainer_signal)" = "confirms" ] || { echo "FAIL `#924` signal: got '$(field 924 maintainer_signal)'"; exit 1; }🤖 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 382 - 384, Add an explicit KEEP assertion for issue 924 before the existing maintainer_signal check, matching the assertion pattern used by fixtures 923 and 925; retain the current signal assertion afterward.
🤖 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.
Nitpick comments:
In `@tests/scripts/test_triage_filter.sh`:
- Around line 382-384: Add an explicit KEEP assertion for issue 924 before the
existing maintainer_signal check, matching the assertion pattern used by
fixtures 923 and 925; retain the current signal assertion afterward.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ffb410f5-422f-4bd5-a5a0-5424302fdaf6
📒 Files selected for processing (2)
scripts/orchestrator/triage_filter.shtests/scripts/test_triage_filter.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/orchestrator/triage_filter.sh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The Step-4 approval bonus was set from comment association alone. Any comment from an
OWNER/MEMBER/COLLABORATORearned the full+3and rendered as "Maintainer approved: yes" — regardless of what the comment said.The contract was explicit that this was association, not approval (the note under the rubric: the field "answers one question — did an account GitHub vouches for as a maintainer speak on this issue?"), while the rubric line directly above it promised
+3for a comment "with approval signal". Nothing read the comment text, so the two never agreed.In practice a maintainer refuting the report, a maintainer routing it to a colleague, and a maintainer inviting a PR were scored identically.
Change
Association becomes a prerequisite and stops being sufficient.
maintainer_signalis graded from the comment text:invites_prconfirmsneutralnoneThree supporting changes, each covering a case association could not see:
@mentionsare stripped before grading. A comment that is only a pointer somewhere else states no position on the issue, and no sentiment tier can read one.Cost
None. Every input comes from the bulk
gh issue listpayload the scan already fetches — the contract's existing "never issue a per-issue call for data the bulk list already returned" rule still holds. No new API calls, no added latency.Limits, stated plainly
The filter matches prose with regexes and will misgrade. A refutation phrased in the project's own domain terms disputes the premise while matching none of the decline patterns. That case is handled in contract prose — the agent is told to read the graded comment, hard-skip on a disputed premise, and record any override in
notesso the gaps stay visible. That is genuinely weaker than the code path.Hard-skip rather than score-down is deliberate: zeroing the bonus still leaves bug + repro + small scope at 7, and labels plus recency clear the 8 threshold without any maintainer signal at all.
This strictly reduces the candidate set. Issues that previously cleared the threshold on an unearned
+3no longer do. That is the intent, but it will show up as fewer results per scan, not just better-ordered ones.Breaking
best_issue.has_maintainer_approval(boolean) is replaced bybest_issue.maintainer_signal(enum) inschemas/repo_shortlist.schema.json. A boolean cannot carry four states. The field was optional and no consumer read it outside the rubric and the report template, both updated here.Tests
tests/scripts/test_triage_filter.sh— 9 new fixtures covering each tier, confirm-and-claim, live vs. stale outsider claim, bare-link comment, terse invite, and an undated comment. All pre-existing assertions still pass, including the union-only-commenter distinction.The second commit fixes three defects found reviewing the first, each with a regression test: the prose floor ran ahead of the pattern match and graded
"PRs welcome!"as no signal; the claim pattern was broad enough to match "the regression is on our side", which owns a defect rather than claiming it; and an undated comment reachedfromdateiso8601, which throws on null and aborted the whole batch.Full suite: 62 pass.
test_audit_impact,test_parse_workflowsandtest_smoke_gatefail identically onmainin this environment (missingyqand GNUtimeout) and are unrelated.Summary by CodeRabbit
New Features
invites_pr,confirms,neutral, ornone.maintainer_signalinstead of the prior maintainer approval boolean.Bug Fixes
Documentation
maintainer_signal.Tests