Skip to content

Update contribution counting script - #1611

Draft
dudejas wants to merge 2 commits into
cloudfoundry:mainfrom
Ivaylogi98:update-contribution-counting-script
Draft

Update contribution counting script#1611
dudejas wants to merge 2 commits into
cloudfoundry:mainfrom
Ivaylogi98:update-contribution-counting-script

Conversation

@dudejas

@dudejas dudejas commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

What

Reworks toc/working-groups/contributions-for-user.sh so the generated gists actually reflect a user's contribution footprint. The previous version omitted authored PRs entirely, generated empty gists for unrelated areas, and missed several categories of contribution relevant to RFC-0006 approver requirements.

Why

Running the script as a candidate for approver returned gists that contained only review/comment activity — no authored PRs, no code contributions on others' PRs, and a flood of empty gists for areas the user has never touched. This made the output unusable for assembling evidence of substantial contributions.

Changes

New sections in the gist

  • PRs Authored — PRs where the user is the author, with their open/closed/merged state. Previously absent entirely.
  • Commits on others' PRs — commits authored by the user that landed in a PR authored by someone else (i.e. pushed onto a collaborator's branch). Previously invisible.
  • Code contributions (merged PRs) — replaces the flat commit listing. Merged PRs are now top-level bullets; commits are indented underneath only when a PR has more than one. Picks up the refactor proposed in Fix yq commit message parsing in contributions script #1457.

Correctness fixes

  • Gist filename was using the undefined ${wg} instead of ${WORKING_GROUP}, producing empty-prefixed filenames.
  • yq commit-message parsing used .commit.message | split("\n")[0] which raised !!seq cannot be added to !!str. Fixed to ((.commit.message | split("\n"))[0]).
  • GitHub search index silently omits some PRs (bosh-agent#393 is one example). Added a targeted events-based fallback that does single-PR lookups for any PR numbers visible in the user's events but missing from search results.
  • The author/committer commit search used repos/{repo}/commits?author= which filters by email rather than login, missing most commits. Replaced with search/commits?q=author:${user}+repo:${repo}.
  • Normalize USERNAME via gh api users/${USERNAME} so the GitHub search API (case-sensitive on author: / reviewed-by: qualifiers) matches reliably.

Performance / robustness

  • Skip gist creation for areas with zero contributions — eliminates a dozen empty gists per run.
  • --paginate added to search/commits so >100 commits are not silently dropped.
  • Events fallback filters to payload.action == "opened" so each PR triggers at most one API lookup, not one per recorded event.
  • All deduplication moved from O(n²) array loops to O(1) associative-array sets (local -A seen).
  • emit_pr_with_commits promoted from a nested closure to a top-level function with an explicit repo parameter, removing the implicit closure-by-name on the outer function's ${repo}.
  • Dropped the now-deprecated cloak-preview and groot-preview Accept headers — both endpoints have been stable for years.
  • Shebang switched to #!/usr/bin/env bash with an explicit bash 4+ version check, since the script now relies on associative arrays. macOS users on the system bash 3.2 get a clear "install bash via Homebrew" message instead of cryptic local -A errors mid-run.

Testing

Ran against Foundational Infrastructure for an approver-candidate user. Areas with no contributions are now skipped; areas with contributions produce gists with all five sections populated, including PRs the GitHub search index had been silently dropping.

Authored with Claude Code

@beyhan
beyhan requested review from a team, Gerg, beyhan, cweibel, mkocher and stephanme and removed request for a team September 1, 2026 06:19
@beyhan beyhan added the toc label Sep 1, 2026
@beyhan beyhan moved this from Inbox to In Progress in CF Community Sep 1, 2026
@cweibel

cweibel commented Sep 1, 2026

Copy link
Copy Markdown

Reviewing this as a TOC member. I ran the script from this branch offline against a stub gh (logging every API call) with controlled fixtures, plus read-only probes against the live GitHub API to check the claims in the description. Four findings below — two are headline features in the notes that don't actually work, two are behaviour changes that aren't mentioned.


1. --paginate on search/commits does not prevent the >100 drop

--paginate added to search/commits so >100 commits are not silently dropped.

For search endpoints, gh api --paginate emits N concatenated JSON objects (one per page), not one merged object. yq cannot read multi-document JSON from stdin, so echo "${commits}" | yq '.items[].sha' stops after the first document.

Live check against search/commits?q=author:rkoster+repo:cloudfoundry/bosh (135 commits):

total_count=135
concatenated JSON docs returned by --paginate: 2
shas the script iterates: 100
stderr: Error: bad file '-': go-yaml load error in parser at L1.C771218: did not find expected <document start>

Because every yq call is 2>/dev/null-adjacent or its stderr is ignored, page 2+ is dropped exactly as silently as before — only now there's an error on stderr nobody reads. The same pattern affects every search/issues call in the script (lines 105, 143, and the reviewed-by/commenter queries): I reproduced a 1390-hit query emitting 100 items. A candidate with >100 PRs in a single repo loses everything past the first page.

total_count is read correctly only by accident — head -1 takes the first document's value.

Suggested fix: gh api --paginate --slurp, or pipe to a file and pass it as a yq file argument (yq handles multi-doc fine from a file, just not stdin), or use jq -s.


2. The events-based fallback is dead code, and the motivating example is in search

Added a targeted events-based fallback that does single-PR lookups for any PR numbers visible in the user's events but missing from search results.

Real PullRequestEvent payloads from users/{user}/events carry a minimal pull_request object. Actual keys present:

['base', 'head', 'id', 'number', 'url']

No user, no title, no merged. So these selectors match zero events:

  • line 170 — select(.payload.pull_request.user.login == strenv(user)) (authored fallback)
  • line 131 — select(.payload.pull_request.merged == true) (merged fallback)

Checked across four real users:

dudejas      PullRequestEvents=8   user.login=0 title=0 merged=0
Ivaylogi98   PullRequestEvents=5   user.login=0 title=0 merged=0
rkoster      PullRequestEvents=25  user.login=0 title=0 merged=0
beyhan       PullRequestEvents=9   user.login=0 title=0 merged=0

Running the script with real event payloads and search returning nothing produced zero single-PR lookups — the fallback never fires, so it cannot recover anything.

On the stated example, bosh-agent#393 is returned by the search index:

$ gh api "search/issues?q=repo:cloudfoundry/bosh-agent+type:pr+author:mariash+is:merged&per_page=100" --jq '[.items[].number]|tostring'
[418,415,410,409,406,393,239,69]

So the PR that motivated the fallback isn't missing from search. If PRs really are being missed, the cause is worth re-diagnosing — my money is on finding #1 (pagination), which would explain missing PRs without needing an events fallback at all.

Related pre-existing bug this makes more visible: the review-events fallback in find_prs_for_repo reads .payload.pull_request.title, which is always null, so it emits literal - : []() lines into the gist. I saw these in real runs.


3. Undisclosed silent 50-commit cap

MAX_COMMIT_LOOKUPS=50 (line 231) isn't mentioned in the notes. With 60 commits available:

commits available: 60
commit->pulls lookups performed: 50
entries in gist: 50
  NOTE: org/repo-a: more than 50 commits; remaining skipped to avoid rate-limit exhaustion

The NOTE goes to stderr, not into the gist, so the gist reader has no idea the list is truncated. This caps evidence for exactly the prolific contributors the tool exists to assess. Worth either surfacing the truncation inside the gist, or making the cap a flag.


4. Undisclosed change: reviewed-by now excludes the user's own PRs

New select(.user.login != strenv(user)) on the reviewed-by query (line 182). Same fixtures, base vs this branch, where the API returns both someone else's PR #20 and the user's own PR #21:

# this branch
### PRs Reviewed/Commented on:
- 2024-03-20: [Bobs PR](https://github.com/org/repo-a/pull/20)

# base
### PRs Commented on/Reviewed:
- 2024-03-20: [Bobs PR](https://github.com/org/repo-a/pull/20)
- 2024-03-21: [Alices OWN PR](https://github.com/org/repo-a/pull/21)

I think this is the right behaviour now that there's a dedicated "PRs Authored" section — self-reviews shouldn't count as review activity. But it's a semantic change to how contributions are counted for RFC-0006 evidence and should be called out in the description so reviewers of the output know why the reviewed-PR counts dropped versus previous runs.


Findings 1 and 2 look blocking to me, since they're two of the main features in the description. 3 and 4 are description/UX fixes. Happy to share the harness or the exact reproduction steps if useful.

@dudejas
dudejas marked this pull request as draft September 1, 2026 16:02
@dudejas

dudejas commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Hi @cweibel , thanks for the review, I'll address your comments soon :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

4 participants