Fan the three human state-loads' per-candidate fetches out through map_bounded

[thedavidmeister]

Closes #235

What

Three human-facing state-loads fetched their candidates serially, one blocking
gh pr view / gh issue view subprocess at a time, on an assumption the code
stated and nothing enforced ("the population is small"). map_bounded — bounded
to QUEUE_FETCH_CONCURRENCY = 8 scoped threads, input-order results, work-stolen
off one atomic counter, already tested — sat in the same file with exactly one
production caller. These three now go through it:

• next_design_fetch → nd_classify
• next_close_candidate_fetch → ncc_classify
• leak_scan_with

unvetted_fetch is deliberately untouched: #233 is rewriting it and, after #233,
it WRITES to GitHub in a guaranteed order. The issue records it as a follow-up.

The two constraints the shape exists to hold

The fetches parallelise; the counting does not. Each converted function fans
the reads out through map_bounded, which hands back one outcome per candidate
in candidate order, then folds those outcomes serially into counts, the
withheld/stranded list and the error list. That is the split presentable_queue
already models, and it is what keeps aiDesign / flagged provably the sum of
their parts and both human-read lists in the order the queue enumerated them. A
fold in completion order changes no count and silently reshuffles two of the
three lists on every run.

A failed fetch is an outcome, never a skip. The continue-plus-counter-bump
arms became named variants — NdOutcome::FetchFailed, NccOutcome::FetchFailed
/ Unaddressable, and leak_scan_with's existing unreadable — folded into the
same buckets, in the same order, with the same why strings. Nothing new is
counted and nothing is dropped.

Output shape

Unchanged. No document key added, removed or renamed; no list reordered; no cap
touched. nd_pr_detail / ncc_issue_detail ask for byte-identical --json field
sets to the inline calls they replace. This is a latency fix and the diff is one.

The one test edit outside the new tests is the cc_gate source-scan pin: the
human inbox's classification moved from next_close_candidate_fetch into
ncc_outcome, so the pin names ncc_outcome, and a new assertion was added
beside it so the chain is closed at the far end (next_close_candidate_fetch
must reach ncc_classify) — naming the classifier alone would let an item that
classifies correctly and is called by nothing satisfy the rule.

Measured

next_design end to end against the real GitHub API, aiDesign: 22, release
build, same box, same token, driven through the MCP server on stdin:

	before (main)	after
run 1	17.44s	5.50s
run 2	17.74s	5.10s
run 3	17.50s	5.45s

3.3x, 17.6s → 5.3s. The residue is the one org-wide search plus the slowest
of the eight in-flight reads. The 22 candidate reads went from ~14s of sequential
round trips to ~2s.

The two documents were compared key by key and are identical — same counts,
same withheld list in the same order, same next row.

next_close_candidate (flagged: 0) and next_leak (1 leak candidate) have no
population to fan out today, so neither shows a change (4.09s → 4.11s and 6.21s →
6.33s, both inside the noise); both were re-run and their documents are identical
too. The design lane is the one with a population, which is what #235 measured.

QA

• Discriminating tests: next_design_tests::the_design_reads_fan_out_and_every_list_stays_in_candidate_order, next_close_candidate_tests::the_flag_reads_fan_out_and_every_list_stays_in_hit_order, next_leak_tests::the_leak_reads_fan_out_and_the_unknowns_stay_in_candidate_order, next_design_tests::a_design_read_that_failed_is_an_error_row_not_a_missing_one, next_close_candidate_tests::a_flag_read_that_failed_is_an_error_row_not_a_missing_one, next_design_tests::the_design_counts_partition_the_whole_population_after_the_fan_out, next_close_candidate_tests::the_flag_counts_partition_the_whole_population_after_the_fan_out — the three order tests inject a fetch whose candidate 0 blocks until some other candidate has finished, so completion order provably differs from candidate order, then assert every emitted list is still in candidate order. The functions under test do not exist on base, so "fails on base" was verified by mutating the merged code back to base's implementation (map_bounded(…) → a serial .iter().map(…).collect()): all three then fail on the inversion assertion, which is the serial behaviour they are written to discriminate.
• Mutations applied: nd_classify map_bounded → serial .iter().map() → the_design_reads_fan_out_and_every_list_stays_in_candidate_order; ncc_classify same → the_flag_reads_fan_out_and_every_list_stays_in_hit_order; leak_scan_with same → the_leak_reads_fan_out_and_the_unknowns_stay_in_candidate_order; nd_classify fold for out in outcomes → outcomes.into_iter().rev() (completion-order proxy) → the_design_reads_fan_out_and_every_list_stays_in_candidate_order; nd_apply_outcome NdOutcome::FetchFailed arm → no-op skip → a_design_read_that_failed_is_an_error_row_not_a_missing_one; ncc_apply_outcome NccOutcome::FetchFailed arm → no-op skip → a_flag_read_that_failed_is_an_error_row_not_a_missing_one; leak_scan_with unreadable.push(...) → bare continue → a_failed_comment_read_is_unknown_and_never_a_clean_bill (existing) and the_leak_reads_fan_out_and_the_unknowns_stay_in_candidate_order; nd_classify raw: live.len() + frozen → live.len() → the_design_counts_partition_the_whole_population_after_the_fan_out; ncc_apply_outcome CcGate::Unvetted => counts.unvetted += 1 → counts.no_flag += 1 → the_flag_counts_partition_the_whole_population_after_the_fan_out.
• Oracle: the SERIAL implementation each function had before, read off the base file — the candidate order the search returned, the same why constants (ND_WHY_NO_QUESTION / ND_WHY_FETCH_FAILED, "unparseable issue ref", "gh issue view failed — not classified this run"), and the partition arithmetic the two count structs' doc comments state (aiDesign == draft + unaddressable + presentable + noQuestion + fetchErrors + archivedRepo; flagged == presentable + vetterCloseVerdict + tornHumanClose + unvetted + noProducerFlag + humanRuled + vetterRejectedStillFlagged + fetchErrors). Expected sequences are computed from the candidate index in the test, never read back out of the value under test. The latency claim's oracle is the wall clock against the live API, both binaries built release from the same tree.
• Category check: issue asks for next_design_fetch, next_close_candidate_fetch and leak_scan_with routed through map_bounded; ordering preserved by folding serially in input order; failures surviving the fan-out into the same buckets; the cap and the output shape untouched; unvetted_fetch left alone. Covered — all three converted, all three carry an order test driven by an injected out-of-order fetch, both error paths carry their own test, both partitions are asserted after the fan-out, QUEUE_FETCH_CONCURRENCY and every document key are unchanged, and unvetted_fetch is not in the diff.

[coderabbitai]

Warning

Review limit reached

@thedavidmeister, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 12 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 210c4551-376c-4263-8bda-3c0a913c51d8

📥 Commits

Reviewing files that changed from the base of the PR and between c6e1ecd and ff77beb.

📒 Files selected for processing (1)

• pr-review-report-rs/src/main.rs

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[thedavidmeister]

Adversarial pass over this diff

I tried to construct an ordering or failure interleaving where a converted function returns rows in the wrong order, loses a candidate, double-counts one, or reports a failed read as a clean result. What I found:

No hole in the ordering or the accounting. map_bounded restores order structurally — a worker writes into the slot at the index fetch_add handed it, so each slot has exactly one writer and the returned sequence is the input sequence for any interleaving. All three folds consume that sequence with a plain for / zip, so there is no path where a completion order reaches a count or a list. Losing a candidate is not expressible: slots is built 1:1 from items, an unfilled slot panics rather than being skipped, and a panicking worker re-panics out of thread::scope before that is reached. Double-counting is not expressible either: an index is handed out once. Three mutants were written specifically to falsify these (serial map, reversed fold, dropped failure arm) and all three are killed.

The one construct that could have hidden a loss is zip. leak_scan_with folds candidates.iter().zip(reads), which silently truncates to the shorter side — a short map_bounded return would drop candidates off the end of unreadable with nothing to show for it. That is covered directly: the_leak_reads_fan_out_and_the_unknowns_stay_in_candidate_order asserts leaks.len() + unreadable.len() == candidates.len().

Everything running on a worker thread is pure. nd_outcome and ncc_outcome only read JSON (last_design_question, cc_gate and its inputs); the crate has no set_var, no statics behind these paths, and TRUSTED_AUTHOR is a const. The only side effect in a worker is Command::new("gh"), which presentable_queue has already been doing 8-way for some time.

One residual hazard, named rather than fixed here

These three still read through gh_json, which is the COLLAPSED shim: it has no typed failure and no retrying_rate_limit. presentable_queue's reads go through queue_pr_detail → gh_retrying → typed GhFailure, which is what lets it report rateLimited separately from fetchError (#129). So if a burst of 8 concurrent reads ever draws GitHub's secondary limit, these three count it as fetchErrors — the same laundering #129 exists to stop — and concurrency makes that marginally likelier than a serial queue did.

It is unchanged by this PR (they used gh_json before too) and out of scope for a latency fix that must not change the output shape — rateLimited is a count key neither document has. Measured: 3 runs at aiDesign: 22, zero fetchErrors, identical documents. Converting these two fetchers to the typed path is a real follow-up, alongside the unvetted_fetch one #235 already records.

[thedavidmeister]

Reviewed ff77beb: approve

Read the conversion sites and the constraint surface myself rather than grading the summary.

The three folds are correct, and correct in the way that matters. Each fans its per-candidate read through map_bounded and then folds SERIALLY over that sequence — for out in outcomes { nd_apply_outcome(..) }, for out in outcomes { ncc_apply_outcome(..) }, and for (c, read) in candidates.iter().zip(reads). Because map_bounded returns results in input order by structural slot-claiming, the emitted counts, withheld, errors and unreadable lists are the same lists in the same order the serial code produced. That is the property a concurrency conversion actually has to preserve here, since two runs over identical state must not hand a human differently-ordered rows. It also mirrors the existing candidate_outcome / apply_outcome shape at the one pre-existing map_bounded caller, so this is the house pattern rather than a new one.

The seam widening is right. leak_scan_with goes FnMut → Fn + Sync because it is now called from several threads at once; a seam that accumulated into captured state would race. The production read owns nothing (it spawns a subprocess), so the constraint costs nothing real.

The zip is the one construct that could lose a candidate silently, and it is guarded: scan.leaks.len() + scan.unreadable.len() == candidates.len() is asserted, so a truncating zip fails rather than reporting a shorter queue as a clean one. Given map_bounded's exactly-once test the lengths match by construction, but the assertion is what makes that a checked fact rather than an inherited assumption.

Constraints checked individually, not taken on trust. unvetted_fetch appears nowhere in the diff (0 occurrences) — correctly out of scope, since #233 was rewriting it and it now writes to GitHub. QUEUE_FETCH_CONCURRENCY is unchanged. No JSON output key is added, removed or reordered: every "key": literal on an added line is a test fixture, not an emitted document. Failed reads still land in their own buckets through the *_apply_outcome arms rather than being skipped.

The conformance pin was the hunk I most wanted to see, and the widening is the right call. The cc_gate scan asserted next_close_candidate_fetch contains cc_gate(; the classification moved into ncc_outcome. Renaming the target alone would have left the chain open at the far end — an ncc_classify that stopped calling ncc_outcome and hand-rolled its own gate would satisfy a single-hop pin while BEING the divergence the pin forbids. Pinning both hops (next_close_candidate_fetch → ncc_classify → ncc_outcome) closes that, and the comment states why.

The claim is measured, not asserted: next_design 17.44/17.74/17.50s → 5.50/5.10/5.45s on the live API, with before/after documents compared key by key and identical. next_close_candidate and next_leak show no change because neither has a population to fan out today (0 flags, 1 leak candidate) — reported as such rather than dressed up.

Known residual, unchanged by this PR and stated on it: these three still read through gh_json, which has no typed failure and no rate-limit retry, so a secondary-limit 403 counts as an ordinary fetch error — and 8-way concurrency makes that marginally likelier than serial did. Fixing it would add a count key, which the scope explicitly excluded. Zero fetchErrors across three live runs. A genuine follow-up alongside the unvetted_fetch one #235 records.

CI 20/20 green, mergeStateStatus: CLEAN.

Rulings-conformance: graded against the repo's CLAUDE.md north star and every ruling the human stated for this work, each named with how the artifact obeys it.

• "i don't understand why we assume the population is small in a tool?? could easily be 100+ — file and agent a fix for this" — obeyed in both halves: State-loads fetch candidates serially on a "population is small" assumption nothing enforces — map_bounded already exists and has one caller #235 filed naming the assumption, its line references and the primitive already present, and the fix delegated to an agent rather than hand-rolled. The assumption is gone at all three sites, not patched at the one that hurt.
• "merge 236" — obeyed: merging on the human's explicit per-PR word, at this reviewed SHA.
• The brief's four constraints — each obeyed and each verified above rather than assumed: concurrent fetch with a serial fold; a failed read is an outcome not a skip; cap and output shape untouched; unvetted_fetch out of scope.
• "a latency fix nobody measured is a claim" — obeyed: three runs before and after against the live API, with the documents diffed key by key so the speedup is not bought by returning something different.
• "producer resolves merge conflicts — merge base in, never rebase" and "semantic conflicts hide outside the markers — run the full suite on the merge commit" — obeyed: when Rename the design-question read from /nd to /ndd #234 landed mid-flight, main was merged in (clean) and the full suite re-run on the merge commit.
• Merge-form rulings — obeyed: --merge --admin, never squash, no --delete-branch.
• "comments describe CURRENT behaviour only" — obeyed: the added comments state what each rule holds and what breaks without it, with no process narration and no issue numbers standing in for reasons.
• CLAUDE.md north star (the Rust tool is the ONLY transition function) — obeyed: this is entirely internal to the binary, adds no prompt-level gh, and reuses the crate's own tested concurrency primitive instead of introducing a runtime or a dependency.