⚡ Bolt: 에이전트 멘션 스윕 PR 조회 N+1 API 병목 해결 - #926
Conversation
scripts/ci/agent_mention_sweep.py의 list_recent_pull_requests에서 리포지토리별 PR 조회 시 발생하는 N+1 순차 API 호출을 concurrent.futures.ThreadPoolExecutor를 활용해 병렬화했습니다.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
📝 WalkthroughWalkthrough에이전트 멘션 스윕이 여러 저장소의 최근 PR을 병렬 조회합니다. 단일 저장소는 순차 조회합니다. 조회 결과는 수집 후 반환하며, 페이지네이션과 저장소별 오류 격리는 유지합니다. Changes에이전트 멘션 스윕
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant list_recent_pull_requests
participant ThreadPoolExecutor
participant GitHub API
list_recent_pull_requests->>ThreadPoolExecutor: 저장소별 조회 작업 제출
ThreadPoolExecutor->>GitHub API: 최근 PR 및 페이지 조회
GitHub API-->>ThreadPoolExecutor: 저장소별 PR 결과
ThreadPoolExecutor-->>list_recent_pull_requests: 수집된 결과 반환
list_recent_pull_requests-->>list_recent_pull_requests: 수집 순서대로 PR 생성
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 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 `@scripts/ci/agent_mention_sweep.py`:
- Line 158: fetch_repo_pulls 함수에 docstring을 추가해 다중 저장소 처리와 저장소별 오류 격리 동작을 간단히
설명하십시오.
- Around line 211-214: Update the worker flow around fetch_repo_pulls so workers
return pull-request results and exceptions instead of invoking on_error
directly; have the caller thread process those results in repository order and
invoke on_error serially. Update fetch_repo_pulls’s docstring to document this
behavior, and add tests covering concurrent errors and exact failure-counter
values.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ff2c489a-6b53-443c-ad23-f99b89c68e63
📒 Files selected for processing (2)
.jules/bolt.mdscripts/ci/agent_mention_sweep.py
scripts/ci/agent_mention_sweep.py의 list_recent_pull_requests에서 리포지토리별 PR 조회 시 발생하는 N+1 순차 API 호출을 concurrent.futures.ThreadPoolExecutor를 활용해 병렬화했습니다. 이에 따라 테스트 시나리오(tests/test_agent_mention_sweep.py)에도 예외 처리 브랜치 등 커버리지를 만족하는 테스트를 추가했습니다.
scripts/ci/agent_mention_sweep.py의 list_recent_pull_requests에서 리포지토리별 PR 조회 시 발생하는 N+1 순차 API 호출을 concurrent.futures.ThreadPoolExecutor를 활용해 병렬화했습니다. 이에 따라 테스트 시나리오(tests/test_agent_mention_sweep.py)에도 예외 처리 브랜치 등 커버리지를 만족하는 테스트를 추가했습니다.
|
@opencode-agent review Please review exact current head |
|
Superseded by #938 at exact head |
Understood. Acknowledging that this work is now obsolete as it has been superseded by #938, and stopping work on this task. |
… 2's 502 gap Two more findings from a sixth Devin Review pass, both verified directly against the vendored contextual-orchestrator source before acting: 1. Empty-string content precision. ModelClient._response_content checks isinstance(content, str) before ever inspecting reasoning, so a genuinely empty string "" (not missing/null) is treated as a valid, non-erroring return and never reaches the reasoning-without-content branch. Verified this is NOT an implementation bug: PR #1452's already-shipped _response_has_reasoning_without_content predicate independently treats content == "" the same as missing content (reusing _chat_response_has_text's own "empty or missing" definition), deliberately broader than _response_content's own narrower condition, and already escalates this case correctly. Fixed as a documentation-precision matter: Trigger B's definition now states explicitly that "no usable content" includes a genuinely empty string, with a precision note clarifying the _response_content citation is the motivating signature this preflight generalizes from, not a claim of exact behavioral equivalence. 2. Layer 2 502 misclassification -- a genuine scope gap, not a wording issue. server.py's except ProviderResponseError: handler is one blanket catch that doesn't even bind the exception, collapsing both of _response_content's distinct failure causes (reasoning-without-content vs. no-content-at-all) into an identical 502 invalid_structured_output body with no machine-readable distinguishing field. Layer 2's sidecar script therefore classifies this as Trigger A by elimination and retries it up to 3 times, rather than failing fast as the correctly- classified Trigger B. Verified this requires an out-of-scope contextual-orchestrator change to fix properly -- no in-repo workaround avoids fragile message-text matching, which this org's own no-heuristics convention already rejects elsewhere in this ADR. Documented as a known, accepted, tracked Layer 2 limitation (Decision Section 1 at the point of definition, Consequences, and Decision Section 4's upstream-tracking list) rather than worked around, filed as ContextualWisdomLab/contextual-orchestrator#932 following the existing #926/#927 pattern. Does not change Layer 2's stated 360s worst case (same shared Trigger-A attempt budget). Updated CHANGELOG.md and docs/product-technical-gap-baseline.md's repeated summaries to match, per Devin's own suggested fix scope. 1897 tests pass (unchanged, docs-only); this branch's own test-plan scope (105 passed, 1 subtest) re-verified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
Four findings, weighed against this org's convergence rule at 26+ review threads across seven rounds on a docs-only PR: 1. Trivial, fixed: Evidence trail's upstream-issue citation still named only #926/#927, missing #932 from the round just landed. 2. Cross-reference gap, not reopened: Layer 1's 160s worst-case claim (Decision Section 3) never referenced #1455 anywhere in this ADR's own text, even though #1455 (the discovery-timing gap) was filed and fully reasoned during the implementation pass on the stacked PR. Added the cross-reference at the point of definition and in Consequences; the underlying discovery-timing question itself stays tracked on #1455, not re-litigated here. 3. Genuinely new, verified real against the actual code (not just the ADR prose): REVIEW_PREFLIGHT_MAX_ESCALATIONS's shared budget is consumed in deterministic catalog order (alphabetical by provider/model, not random), so a later-sorting healthy candidate can be denied its own escalation attempt purely because 4 earlier candidates already claimed the shared budget. Considered a cheap reordering fix (round-robin, random shuffling) and rejected it on the merits: any selection policy for a fixed-size shared budget smaller than the candidate pool still has to deny someone a slot, so reordering only changes which candidates are favored, not whether the trade-off exists -- and picking a specific policy without real telemetry on which candidates actually need escalation more often would itself be exactly the unjustified heuristic this ADR already rejects elsewhere. Documented as a known, accepted, tracked limitation (#1458, matching the #1454/#1455/#932 pattern) rather than redesigned. 4. No action: the gap-baseline's repeated review-round narrative is this repo's own documented, intentional convention (docs/adr/0002-product-technical-gap-baseline.md: the baseline is "an operational snapshot" and "live PR metadata inventory," a distinct role from the ADR's design record and the CHANGELOG's terse pointers), not accidental redundancy. Updated CHANGELOG.md and docs/product-technical-gap-baseline.md to match. 1897 tests pass (unchanged, docs-only); this branch's own test-plan scope (105 passed, 1 subtest) re-verified. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Gs7KmNvH75nxz1sL8mKjw
1 participant
Outcome
Parallelize organization-wide recent-PR discovery without allowing a bounded mention sweep to wait for irrelevant repository work after reaching its dispatch limit.
Exact identity
6eb06cdd08c79a06f7b390069d4ffa49e2eb7dba;93eae45d78fc56b319a7c9f9ccf600180577c196;bolt-fix-agent-mention-n-plus-one-11378016381235193798;Root cause and repair
The original bounded
ThreadPoolExecutorimplementation removed the N+1 API bottleneck and serialized repository-local failures, but the executor context manager still usedshutdown(wait=True). Whensweep()reachedmax_dispatches, returning from the consumer did not explicitly close its candidate iterator and could wait for every running or queued repository fetch.The repaired contract:
shutdown(wait=False, cancel_futures=True);Test-first evidence
The hosted full-repository, security, dependency, and supply-chain runs on the current head remain authoritative for integration.
Duplicate lineage
#938 independently reintroduced the same N+1 optimization from the same protected base, but lacks this cancellation contract and currently fails its quality gate. Preserve #938 as duplicate evidence and converge on this older, fully tested branch once its active Jules writer is no longer targeting it.
Acceptance
Keep merge fail-closed until all current-head workflows complete successfully, current semantic review has no valid unresolved finding, qualifying independent human approval exists, live protected-main ancestry is revalidated, and protected-main operational acceptance proves bounded early termination.