perf: repo-finder burns ~59-109 API calls per scan for data it already fetched#28
Merged
Merged
Conversation
…date Step 1 searched via `gh search repos` across nine queries, then Step 2 re-fetched pushed_at, open_issues_count, archived, and default_branch with one `gh api repos/OWNER/REPO` per candidate, against 50-100 candidates per scan. All four fields already ship inline in every search result. - Search via `gh api search/repositories`. The porcelain cannot return topics[], which the shortlist now carries; the REST projection returns it plus the four fields Step 2 was re-fetching. - Drop the per-candidate repo call. Step 2 reads the Step 1 record. - Nine queries -> five. Repeated `language:` unions (java|python collapse to one request); repeated `topic:` intersects, so at most one topic per query. - Delete the two free-text queries and the bare-stars catch-all: they carried neither a language nor a topic and admitted repos matching no criterion. - `archived:false` server-side; sort by stars, not updated, which reordered hourly and left the per-query cutoff non-deterministic. Live check: 5 requests, 197 unique repos after dedup, every candidate carrying all nine projected fields. Also pin the shortlist contract, which had no schema: - Add schemas/repo_shortlist.schema.json + test. - Validate before the atomic rename. validate_json degrades to a top-level required-key check without python jsonschema, so repo/scores.final are asserted separately with jq. - Fix commands/repo-finder.md rendering `.score`, a key the agent never writes. The score column printed null on every row.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRepository discovery now projects GitHub API fields once and reuses them for filtering. Shortlist persistence validates JSON, schema structure, and required row fields before atomic replacement. Rendering reads ChangesRepository shortlist
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RepoFinder
participant GitHubSearchAPI
participant Validator
participant GlobalCache
participant RepoFinderCommand
RepoFinder->>GitHubSearchAPI: search and project repository candidates
GitHubSearchAPI-->>RepoFinder: return projected fields and total_count
RepoFinder->>RepoFinder: filter candidates using projected fields
RepoFinder->>Validator: validate shortlist JSON and row invariants
Validator-->>RepoFinder: return validation result
RepoFinder->>GlobalCache: atomically write validated shortlist
RepoFinderCommand->>GlobalCache: read repo-shortlist.json
RepoFinderCommand-->>RepoFinderCommand: render scores.final
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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)
agents/repo-finder.md (1)
60-74: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo error handling for a failing
gh apicall inside the loop.Each iteration calls
gh apiwith no exit-code check. A rate-limited or network-failed request for one query would silently yield zero rows for that query rather than aborting or being retried — which the doc itself warns about elsewhere ("a partial candidate set reads as a clean scan") but doesn't guard against here.🤖 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 `@agents/repo-finder.md` around lines 60 - 74, The repository search loop around DEFAULT_QUERIES must check the exit status of each gh api invocation. Make a failed request abort the scan with a nonzero status, preserving the documented behavior that rate-limit or network failures are not treated as an empty result set.
🤖 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`:
- Around line 59-83: Update the query loop around the gh api projection to abort
immediately when any candidate item has a null or absent full_name, rather than
silently emitting or skipping it; preserve the existing projected fields for
valid rows. Also retain each response’s top-level total_count separately from
.items and report matched results against that total for every query, without
changing query thresholds or widening queries to fill the list.
In `@tests/scripts/test_repo_shortlist_schema.sh`:
- Line 3: Separate the directory resolution from the export in the
CLAUDE_PLUGIN_ROOT setup: first assign the command substitution to the variable,
allowing a failed cd to propagate under set -e, then export the variable in a
subsequent command.
---
Nitpick comments:
In `@agents/repo-finder.md`:
- Around line 60-74: The repository search loop around DEFAULT_QUERIES must
check the exit status of each gh api invocation. Make a failed request abort the
scan with a nonzero status, preserving the documented behavior that rate-limit
or network failures are not treated as an empty result set.
🪄 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: 917480e9-9c16-478e-a950-5f9388e52fee
📒 Files selected for processing (4)
agents/repo-finder.mdcommands/repo-finder.mdschemas/repo_shortlist.schema.jsontests/scripts/test_repo_shortlist_schema.sh
Review caught that Step 1 documented a hard abort on null `full_name` and matched-of-total reporting, but the query loop implemented neither: the --jq filter drilled straight into .items[], dropping total_count, with no null check. A guard that exists only in prose is the failure mode this agent is meant to remove. - Abort on a failed `gh api` search. An unchecked exit status let a rate-limited query contribute zero rows to a scan that then read as clean. - Abort on null full_name. Field-name drift yields null, not an error. - Retain total_count and report matched-of-total, reusing the same response so the request count per query is unchanged. - Feed the loop by heredoc, not `echo | while`. A piped while runs in a subshell where `exit` terminates only the subshell, so both aborts above would have been swallowed and the scan would have continued. Verified: piped form exits 0 and runs to completion after a fatal abort; heredoc form exits 1. - Pin all of it in the test, including that the loop is not piped and that the per-candidate repo call stays deleted. Also SC2155 in the new test: assign CLAUDE_PLUGIN_ROOT, then export, so a failed cd is not masked by export's exit status.
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_repo_shortlist_schema.sh`:
- Around line 66-84: The regression checks in
tests/scripts/test_repo_shortlist_schema.sh must validate executable behavior
rather than matching comments or prose. Replace the literal-text abort
assertions with representative failure and null-response fixture tests, or
strengthen them with broad syntax checks plus a positive heredoc assertion;
broaden the piped-loop rejection beyond DEFAULT_QUERIES and reject any
per-candidate gh api invocation targeting repos/, including quoted or
variable-based forms.
🪄 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: be075fab-e252-49b2-8d00-ab99aa5ded6e
📒 Files selected for processing (2)
agents/repo-finder.mdtests/scripts/test_repo_shortlist_schema.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- agents/repo-finder.md
…them Review caught that the regression checks matched literal text anywhere in the markdown, so a comment or a line of prose satisfied them. They asserted the documentation, not the behavior — the same doc-vs-code drift they were added to catch. Extract the Step 1 block and run it against a stubbed gh (no network): - failed search must exit nonzero and report FATAL - null full_name must exit nonzero and report FATAL - valid response must return rows and report matched-of-total Structural checks now run on the block with comments stripped: reject a piped `while` (its aborts cannot exit the scan) and require the heredoc form. The per-candidate call is matched on its resurrection signature — a repos/ call carrying pushed_at/open_issues_count/default_branch — not on `gh api repos/` wholesale, as review proposed: the workflow-tree, contributors, comments, and pulls calls are legitimate and must keep working. Each assertion verified against a negative control: all five fail when the behavior they guard is reverted.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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_repo_shortlist_schema.sh`:
- Around line 105-108: Update the happy-path assertions after run_step1 in the
test to first verify that the output contains the expected owner/repo candidate
row, then retain the existing “of 63 matched” total assertion. Ensure the test
fails when projected candidates are dropped even if the matched-count text is
still emitted.
- Around line 120-127: The repository-refetch check around the AGENT scan must
detect root repos/$full_name API commands regardless of projected-field
placement or multiline formatting. Normalize complete shell commands before
matching, then reject only root repository endpoints while preserving legitimate
subresource calls such as /git/trees, /pulls, contributors, and comments; update
the grep pipeline and failure condition accordingly.
🪄 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: f1e70501-ebca-4b57-9d8a-1af625dd839f
📒 Files selected for processing (1)
tests/scripts/test_repo_shortlist_schema.sh
Review caught both, and both were real. Happy path asserted only the matched-of-total line, which "0 of 63 matched" satisfies — a regression dropping every projected candidate passed. Step 1 accumulates rows into $CANDIDATES without printing them, so the runner now dumps the variable and asserts the row and default_branch survive the projection. Refetch check matched field names on the same line as a repos/ call, so `gh api "repos/$full_name"` evaded it (root endpoint returns all four fields, no field name on the line) and a line-continued command hid the field list from a line-oriented grep. Now allowlists subresources — /git/trees, /contributors, /issues, /pulls and friends stay legal — and rejects any other repos/ path, however spelled. Matching the root form directly does not work: the owner/repo may be a variable and carry no literal slash. Fixing the first hole surfaced a bug in the harness itself: command substitution strips the trailing newline, so the appended dump fused onto the extracted block's heredoc terminator and the heredoc never closed. Verified against negative controls: root refetch via variable, line-continued, literal OWNER/REPO, and -X GET forms are each caught; the four legitimate subresource calls still pass.
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
A single
/repo-finderscan makes ~59-109 GitHub API requests. Almost all of them are redundant.Step 1 runs nine searches. Step 2 then loops over the 50-100 surviving candidates and, for each one, calls:
gh api repos/OWNER/REPO --jq "[.pushed_at, .open_issues_count, .archived, .default_branch] | @tsv"All four of those fields ship inline in every search result. Step 1 fetched them, discarded them, and Step 2 buys them back one repo at a time. The existing comment (
# One repo call, not three.) shows this was already optimized 3 calls -> 1. It can go 1 -> 0.Three of the nine searches are also junk:
"spring boot"and"fastapi"are free-text matches against the name/description with no language or topic check, and the bare--stars ">20000"catch-all admits any repo above the star floor in any language, tagged or not. These are the only paths by which a repo matching no criterion enters the pool.What changed
Search via
gh api search/repositories. The porcelain cannot returntopics[]at any price, andtopics[]is what lets the agent justify a ranking instead of asserting one. It now reaches the shortlist row. The same projection returns the four fields Step 2 was re-fetching.Delete the per-candidate repo call.
$PUSHED_AT,$OPEN_ISSUES,$DEFAULT_BRANCHare read from the record already in hand. Thearchivedcheck becomes dead code oncearchived:falseis a server-side qualifier. The 30-day merged-PR check (gh pr list --search) stays — no search result carries that signal.Nine queries -> five. Repeated qualifiers do not behave alike, and this is load-bearing:
language:language:go language:rustreturns Go ∪ Rusttopic:topic:cli topic:terminalreturns only repos tagged bothSo java and python union into one request (was two), but multi-topic-OR is impossible in one query. A second
topic:silently intersects — smaller or empty set, no error. Query count is|topics|, not|langs| × |topics|. The agent now states this as a rule.Also:
sort=starsinstead ofupdated(which reordered hourly, leaving the per-query cutoff non-deterministic and nothing downstream testable), andarchived:falseserver-side so dead repos never consume a result slot.Silent-failure surface this closes
Unrecognized qualifiers are not errors.
langauge:gois parsed as free text and returnstotal_count: 0with exit 0 — the same fail-silent signature as theOR/parens bug fixed in #25. The agent now constrains everykey:token to{language, topic, stars, archived}.Dropping
--jsoninverts the field-name bug class rather than retiring it:--json fullNamwas a hard error, butjq .full_namisnull, silently. So a candidate row with a nullfull_nameis now a hard abort, not a skipped row — a partial candidate set otherwise reads as a clean scan.Shortlist contract
repo-shortlist.jsonwas the only shared-state file with no schema, and its row shape is changing here (topics[]). Addedschemas/repo_shortlist.schema.json+ test, and validation before the atomic rename.One wrinkle worth calling out for review:
validate_jsondegrades to a top-level required-key check when pythonjsonschemais absent — which it is on a stock macOS runner. Every existing schema test gets away with this because their bad fixtures violate a top-level key. Here the keys that matter (repo,scores.final) are nested insiderepos[], so the fallback would wave through the exact bug the schema exists to catch. The agent therefore asserts the row invariant separately withjq. The schema is still correct under a real validator (verified againstjsonschema; rejects a row missingrepo, missingscores.final, or arepothat is notowner/repo-shaped).Render bug:
commands/repo-finder.mdprinted\(.score), but the agent writes the score toscores.final. The score column has beennullon every row. Fixed.Verification
Ran the five queries live:
{"Java": 5, "Python": 45}on one page.language:ortopic:, no query has twotopic:, every qualifier is in the allowlist, no boolean operators.grepconfirms the per-repo field fetch is gone and the merged-PR check survives.jq-1.7.1-apple. (test_audit_impact,test_parse_workflows,test_smoke_gatefail identically on untouchedmain— missingyq/timeoutlocally.)Note for the reviewer
The write-time guard treats a zero-row shortlist as fatal, on the reading that an empty scan is far likelier a rate-limited or malformed search than a true "nothing worth contributing to" — 197 candidates surface before scoring. If you would rather a genuinely empty result write
repos: [], that is a one-line relaxation.This does not introduce the strict
language AND topicfilter. Default behavior stays a union of unconstrained axes, minus the three queries that matched nothing. A follow-up makes the strict filter opt-in via user preferences.Summary by CodeRabbit
repokey and include per-repositorytopics.scores.final.