Repository navigation
fix(kiro): prioritize tool search results within catalog budget - #2475
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe Kiro adapter now prioritizes tools loaded by ChangesKiro catalog budgeting
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
2/4 boxes ticked. Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked. |
리뷰 · 우선순위 64 / 80설명: 이 풀 리퀘스트는 키로가 도구 검색으로 되살린 정의를 카탈로그 한도 안에서 앞에 두는 고침이다. 작성자는 mchlkim 이다. 오늘 열일곱 시 삼 분에 열렸다. 초안이다. 점검 네 칸이 비어 있다. 위생은 통과다. 베이스는 지금 HEAD 지금 HEAD 를 열었다. src/responses/parser.ts 743줄은 검색으로 되살린 정의에 이 PR 은 한도를 넘길 때만 순서를 바꾼다. 우선은 검색으로 되살린 정의, 그 다음 검색 관문, 그 다음 일반 도구다. 같은 칸 안에서는 호출 순서를 지킨다. 한도 안이면 원래 순서를 그대로 둔다. 마흔여덟과 구만육천은 안 바꾼다. 시험은 일반 도구 예순여덟, 검색 관문 하나, 되살린 정의 하나를 넣는다. 나가는 마흔여덟의 앞이 되살린 정의와 관문인지 본다. 생략 알림에 일반 도구 이름만 남는지 본다. 구멍의 모양과 같다. 바이트만 넘기는 길은 이 시험이 안 잠근다. 작성자는 모아 둔 시험이 다른 파일에서 멈췄다고 적었다. 깃허브 점검 네 칸은 비어 있다. 초안이다. 지금 머지하지 말 것. 커서 쪽 src/adapters/cursor/request-builder.ts 50줄은 이미 검색 되살림을 순위에 넣는다. 다만 실행 길이와 wait 와 apply_patch 보다 뒤다. 키로에는 그 고정 칸이 없다. 이 PR 이 되살린 정의를 맨 앞에 두는 것은 2407 에 맞다. 한도를 안 넘기면 순위를 안 매기는 것도 맞다. 201줄 뒤만 자르기 정책은 이 구멍 때문에 바뀐다. 그건 이 이슈의 목적이다. types.ts/config.ts 가르기와 상관없다. 닫고 다시 밑지 말 것. 프리뷰 배포가 아니다. 사용자 길이로는 키로 큰 카탈로그에서 검색으로 찾은 도구가 바로 다음 턴에 사라져서 64. 초안이라 지금 착지하면 안 된다. 2407 은 이 PR 이 착지한 뒤에 닫는다. 2474 는 스크립트 작업 폴더 고침이고 이 구멍과 다르다. 2473 은 웹소켓 초안이다. 2472 는 연다. 2463 과 2464 과 2465 는 구현 파일이 없어서 닫지 않는다. 2468 은 메인에 넣지 말 것. 2451 과 2467 과 2471 은 다시 열지 말 것. 카탈로그 팁은 Ox Alpha x-preview-f-free + deepseek-v4-flash-vision-exp. Cursor 정적 카탈로그는 opus-4-8-fast / opus-5-fast. 2334 CursorCredentialRouter 는 여전히 src/providers/cursor-pool.ts 모듈+테스트만 있고 어댑터에 연결되지 않았다. 2332 H2 는 discovery 전용. 2320 overflow + 2342 는 이미 dev. 2188 사이드카는 이미 dev. 2361 src/adapters/kiro-tools.ts 라인 201 - HEAD 주석은 순위를 매기지 말고 뒤만 자르라고 한다. 이 PR 이 한도 초과일 때만 순위를 켠다 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
6b2c28f to
404151f
Compare
|
Updated after the draft-gate feedback:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 404151fb51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| || serializedToolCatalogBytes([...convertedTools, entry.converted]) > MAX_KIRO_TOOL_CATALOG_BYTES | ||
| ) { | ||
| omittedAt = index; | ||
| break; |
There was a problem hiding this comment.
When multiple restored tools have large schemas, one may fit while the next exceeds the remaining 96 KiB budget; this break then drops every later candidate, including the small tool_search gateway and ordinary tools that would still fit. An individually oversized restored tool similarly reduces the advertised catalog to zero. Continue scanning after a byte-budget miss and track omitted entries individually rather than treating the first miss as the start of an omitted suffix.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
|
|
||
| function boundedCatalogPriority(tool: OcxTool): number { | ||
| if (tool.loadedFromToolSearch) return 0; | ||
| if (tool.toolSearch) return 1; |
There was a problem hiding this comment.
When a continuation contains at least 48 definitions restored from prior tool_search_output items, every restored definition sorts ahead of the gateway and fills the count limit, so tool_search is omitted and the session can no longer discover additional deferred tools. Since src/responses/parser.ts accumulates restored definitions across the input history, this state can persist on subsequent turns; pin the gateway ahead of restored definitions or reserve one of the 48 slots for it.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
Under Codex code mode, shell, file edits, apply_patch and every MCP helper are reachable only as nested tools.<name>(...) calls inside one freeform exec tool. Kiro's catalog budget ranked that tool as ordinary filler, so a crowded session could drop it and emit a catalog in which every admitted tool is uncallable. Cursor pins its execution path for the same reason (request-builder.ts, #399). Priority alone does not fix it. loadedFromToolSearch tools outrank exec and arrive unbounded -- the Responses parser pushes every tool_search_output spec -- so a session holding MAX_KIRO_TOOL_COUNT loaded tools exhausts the count budget before reaching the exec tier and drops the one tool that makes the other 48 callable. Reserve instead of evict. The fill loop projects exec into every count and byte check, so it admits one fewer tool rather than taking one back. That distinction is what keeps #2475's loaded-result guarantee intact: Cursor's evictNonExecutionPath exempts only the execution path and could evict a loaded tool. Byte checks measure the projected final array because the budget is computed over the serialized array, where separators shift. Omissions are now derived by set difference. The previous candidates.slice(omittedAt) assumed every candidate after the first rejection was omitted, which stops being true once one of them is reserved and admitted -- the notice would have named exec unavailable while it was on the wire. Emitted order is rebuilt from the sorted candidates so the wire order stays loaded -> exec -> gateway -> filler. The wp1 test asserting an omitted exec is never named now uses a structured exec, which stays ordinary filler and stays droppable, so both invariants keep coverage. Plan and audit: devlog/_plan/260827_kiro_subagent_delegation_unblock/040, /041
Under Codex code mode, shell, file edits, apply_patch and every MCP helper are reachable only as nested tools.<name>(...) calls inside one freeform exec tool. Kiro's catalog budget ranked that tool as ordinary filler, so a crowded session could drop it and emit a catalog in which every admitted tool is uncallable. Cursor pins its execution path for the same reason (request-builder.ts, lidge-jun#399). Priority alone does not fix it. loadedFromToolSearch tools outrank exec and arrive unbounded -- the Responses parser pushes every tool_search_output spec -- so a session holding MAX_KIRO_TOOL_COUNT loaded tools exhausts the count budget before reaching the exec tier and drops the one tool that makes the other 48 callable. Reserve instead of evict. The fill loop projects exec into every count and byte check, so it admits one fewer tool rather than taking one back. That distinction is what keeps lidge-jun#2475's loaded-result guarantee intact: Cursor's evictNonExecutionPath exempts only the execution path and could evict a loaded tool. Byte checks measure the projected final array because the budget is computed over the serialized array, where separators shift. Omissions are now derived by set difference. The previous candidates.slice(omittedAt) assumed every candidate after the first rejection was omitted, which stops being true once one of them is reserved and admitted -- the notice would have named exec unavailable while it was on the wire. Emitted order is rebuilt from the sorted candidates so the wire order stays loaded -> exec -> gateway -> filler. The wp1 test asserting an omitted exec is never named now uses a structured exec, which stays ordinary filler and stays droppable, so both invariants keep coverage. Plan and audit: devlog/_plan/260827_kiro_subagent_delegation_unblock/040, /041
Under Codex code mode, shell, file edits, apply_patch and every MCP helper are reachable only as nested tools.<name>(...) calls inside one freeform exec tool. Kiro's catalog budget ranked that tool as ordinary filler, so a crowded session could drop it and emit a catalog in which every admitted tool is uncallable. Cursor pins its execution path for the same reason (request-builder.ts, lidge-jun#399). Priority alone does not fix it. loadedFromToolSearch tools outrank exec and arrive unbounded -- the Responses parser pushes every tool_search_output spec -- so a session holding MAX_KIRO_TOOL_COUNT loaded tools exhausts the count budget before reaching the exec tier and drops the one tool that makes the other 48 callable. Reserve instead of evict. The fill loop projects exec into every count and byte check, so it admits one fewer tool rather than taking one back. That distinction is what keeps lidge-jun#2475's loaded-result guarantee intact: Cursor's evictNonExecutionPath exempts only the execution path and could evict a loaded tool. Byte checks measure the projected final array because the budget is computed over the serialized array, where separators shift. Omissions are now derived by set difference. The previous candidates.slice(omittedAt) assumed every candidate after the first rejection was omitted, which stops being true once one of them is reserved and admitted -- the notice would have named exec unavailable while it was on the wire. Emitted order is rebuilt from the sorted candidates so the wire order stays loaded -> exec -> gateway -> filler. The wp1 test asserting an omitted exec is never named now uses a structured exec, which stays ordinary filler and stays droppable, so both invariants keep coverage. Plan and audit: devlog/_plan/260827_kiro_subagent_delegation_unblock/040, /041
2 participants
Summary
tool_searchwhen Kiro's outbound catalog exceeds its count or byte budget.tool_searchgateway immediately behind restored definitions so deferred discovery remains available.Fixes #2407
Verification
bun test tests/kiro-adapter.test.ts— 56 passed, 0 failed (Bun 1.3.14)bun test tests/server-kiro-completion-e2e.test.ts— 4 passed, 0 failed (unrestricted loopback E2E)bun run typecheck— passedbun run privacy:scan— passedgit diff --check upstream/dev...HEAD— passedbun run testin the sandbox is not representative: loopback E2E setup is denied withEADDRINUSE/EPERMbefore test logic runs. An unrestricted full-suite result is still required to tick the first readiness item.Checklist
OcxTool.loadedFromToolSearch.)Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Summary by CodeRabbit