Skip to content

treat transferred issues as closed in discovery scoring#415

Merged
anderdc merged 8 commits intoentrius:testfrom
nudiltoys-cmyk:transferred-issues-as-closed
Apr 17, 2026
Merged

treat transferred issues as closed in discovery scoring#415
anderdc merged 8 commits intoentrius:testfrom
nudiltoys-cmyk:transferred-issues-as-closed

Conversation

@nudiltoys-cmyk
Copy link
Copy Markdown
Contributor

closes #404
closes #405

summary

transferred issues were being counted as solved in discovery scoring, so a miner could file an issue, have it transferred, and still collect emissions
repo_scan never populated is_transferred even though the rest response carries state_reason, and the pr-linked path never had the data because the graphql query didn't select stateReason
this adds stateReason to the closingIssuesReferences selection, sets is_transferred in both construction paths, and routes transferred issues to closed_count in _collect_issues_from_prs and _merge_scan_issues

test plan

pytest covers transferred vs non-transferred in the pr-linked scoring path, the scan merge path, and the repo_scan field population from state_reason transferred/completed. ruff and pyright pass

@anderdc
Copy link
Copy Markdown
Collaborator

anderdc commented Apr 14, 2026

Review

What works

  • Transferred issues are correctly routed to closed_count in both _collect_issues_from_prs and _merge_scan_issues
  • stateReason added to GraphQL query, state_reason read from REST — both construction paths covered
  • Test coverage is solid: scoring paths, repo_scan field population, factory fixtures

Issues

NOT_PLANNED gap — GitHub stateReason has three values: COMPLETED, NOT_PLANNED, TRANSFERRED. The PR only handles TRANSFERRED. A NOT_PLANNED issue closed without a merged PR currently falls through to solved_count in _merge_scan_issues (line: if issue.state == 'CLOSED' and issue.closed_at). This is the same class of anti-gaming hole — a miner files an issue, it gets closed as not-planned, and they still get positive credibility. Recommendation: store state_reason on Issue instead of (or alongside) is_transferred, then gate on state_reason == 'COMPLETED' for solved classification, or at minimum add is_not_planned handling identical to transferred.

_merge_scan_issues solved classification is too loose — Any closed issue with closed_at set counts as solved. After the transferred check, a NOT_PLANNED issue still hits data.solved_count += 1. This predates the PR but the PR should fix it since it's the same category of bug and the data (stateReason) is now available.

Boolean field limits future extensibilityis_transferred: bool on Issue means if you later need NOT_PLANNED handling, you add another boolean. A state_reason: Optional[str] field would be more natural and let scoring logic branch on all three values. The boolean can stay as a convenience property if desired.

Minor: no test for NOT_PLANNED — Even if NOT_PLANNED handling is deferred, a test documenting current behavior for state_reason='not_planned' would make the gap explicit.

Verdict

The PR properly solves what it claims (transferred issues). The NOT_PLANNED gap is a real scoring loophole that should be addressed — either in this PR or as a fast-follow. Storing state_reason on Issue instead of only is_transferred would let both be handled cleanly.

@nudiltoys-cmyk
Copy link
Copy Markdown
Contributor Author

thanks for the review

pushed a follow-up commit covering all four points:

  1. swapped is_transferred: bool for state_reason: Optional[str] on Issue, kept is_transferred as a @property returning self.state_reason == 'TRANSFERRED' so downstream readers are untouched
  2. populated state_reason from GraphQL stateReason and REST state_reason (normalized to uppercase for a single gate)
  3. gated solved classification in _collect_issues_from_prs and _merge_scan_issues on state_reason == 'COMPLETED' — anything else (NOT_PLANNED, TRANSFERRED, None) routes to closed_count. None is effectively unreachable inside the 35-day lookback since GitHub auto-populates stateReason on close, but treating it as not-solved is the safer default if that ever changes
  4. added COMPLETED, NOT_PLANNED, TRANSFERRED, and None test cases in both test files alongside the existing transferred ones

12/12 tests in the touched files pass, ruff clean. ready for another look

@nudiltoys-cmyk
Copy link
Copy Markdown
Contributor Author

pushed three style-only commits to clear the lint failure -just ruff format on the three files CI flagged, no logic changes. mind re-approving the workflow run?

@corevibe555
Copy link
Copy Markdown
Contributor

Why is it that having two issues in one PR?
One is repo scan level and the second issue is about scoring after successful issue scan.

@nudiltoys-cmyk
Copy link
Copy Markdown
Contributor Author

both issues come from the same bug. transferred issues weren't being excluded in two adjacent scoring paths (pr-linked and scan-merge). the fix is one coherent change across those paths, splitting into two PRs would just create merge conflicts between them

@corevibe555
Copy link
Copy Markdown
Contributor

No there won't be a conflict.
And there is literally testable way to verify each of the tickets.
github_api_tool.py, repo_scan.py and its test as PR 1
scoring.py and its test as PR2

You will see the PR size reasonable, this already became larger.
Sorry to interrupt but I am commenting because I don't understand the reason of mixing two issues in a PR.

@anderdc
Copy link
Copy Markdown
Collaborator

anderdc commented Apr 16, 2026

Implementation looks good -- addresses both transferred and NOT_PLANNED correctly. Two things before this can merge:

  • Rebase onto test -- feat(issue-discovery): precise post-merge edit detection upgrade #434 landed and changed _merge_scan_issues to take 4 args (miner_evaluations). This PR still calls it with 3, so CI will break. The test helper _run_scan_path needs the same fix.
  • Fix CI -- approve the workflow run after rebasing so we can verify everything passes.

nudiltoys-cmyk added 3 commits April 16, 2026 15:37
fixes the anti-gaming gap where transferred issues counted as solved
in the pr-linked path and were not flagged during repo scan. adds
stateReason to the closingIssuesReferences graphql selection,
populates is_transferred in both construction paths, and routes
transferred issues to closed_count in _collect_issues_from_prs and
_merge_scan_issues
replaces is_transferred: bool with state_reason: Optional[str] on Issue
and keeps is_transferred as a convenience property. gates solved
classification in _collect_issues_from_prs and _merge_scan_issues on
state_reason == 'COMPLETED', so NOT_PLANNED and TRANSFERRED both route
to closed_count. None is effectively unreachable inside the 35-day
lookback since github auto-populates stateReason on close, but the
strict gate is the safer default if that ever changes

tests cover COMPLETED, TRANSFERRED, NOT_PLANNED, and None in both the
pr-linked path and the scan path, plus the REST uppercase normalization
@nudiltoys-cmyk nudiltoys-cmyk force-pushed the transferred-issues-as-closed branch from c15b5f9 to 116ebfd Compare April 16, 2026 15:05
@nudiltoys-cmyk
Copy link
Copy Markdown
Contributor Author

rebased onto test, resolved the classes.py conflict with body_or_title_edited_at. ready for ci approval

@anderdc anderdc added the bug Something isn't working label Apr 16, 2026
@anderdc
Copy link
Copy Markdown
Collaborator

anderdc commented Apr 16, 2026

failing tests

@nudiltoys-cmyk
Copy link
Copy Markdown
Contributor Author

my new test was calling asyncio.run, which leaves the event loop as None and breaks the get_event_loop() helpers in test_pat_handler and test_repo_scan. ebdd683 switches it to the same pattern, passes locally

Copy link
Copy Markdown
Collaborator

@anderdc anderdc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thorough re-review complete. Both scoring paths correctly gate on state_reason == 'COMPLETED' -- NOT_PLANNED, TRANSFERRED, and None all route to closed_count as intended. GraphQL and REST data flows verified, no double counting, backward compat preserved via is_transferred property. 12 tests cover all four state_reason values across both paths. CI green. LGTM.

@anderdc anderdc merged commit 59227bc into entrius:test Apr 17, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

3 participants