Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: lidge-jun/opencodex/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe Devin adapter now surfaces provider-stated 429 reset messages immediately. It does not wait or replay the request. New tests cover long and short reset periods, and test-layout mappings include the new integration test. ChangesDevin reset handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Devin reset errors now surface immediately without replaying requests or holding active-turn capacity. The adapter and helper tests cover this behavior, leaving no concrete merge-blocking risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 5 functions across 3 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches🧪 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. |
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 62 / 80Devin이 한도를 넘기면 "21분 뒤에 풀린다" 같은 429를 돌려줍니다. 지금까지 그 요청은 이미 받은 자리(활성 턴)를 잡은 채로 그 시간 동안 잠들어 있었습니다. 이 자리는 서버 전체가 같이 씁니다. 한 요청이 오래 기다리면 다른 요청이 그 자리를 못 받습니다. 이 PR은 Devin 어댑터가 재시도 함수를 부를 때 기다림 한도를 0으로 고정합니다. 한도가 0이면, 메시지에 적힌 대기 시간이 1초라도 있으면 잠들지 않고 원래 429를 바로 던집니다. 어댑터는 그걸 실패로 끝내고 자리는 풀립니다. 설명 한 줄과, 한도 0이면 sleep이 호출되지 않는 테스트가 같이 들어갔습니다. src/adapters/devin.ts:666 - 주석은 파싱한 Retry-After를 클라이언트에 보여 준다고 적혀 있습니다. 초 숫자는 기다릴지 말지 고를 때만 쓰이고, 그 값은 버려집니다. 스트리밍 응답은 HTTP 200으로 이미 나가고 본문은 response.failed입니다. Retry-After 헤더는 없습니다. 영어 문장 "reset in 21 minutes"만 에러 메시지 안에 남습니다. 헤더만 보고 다시 시도하는 클라이언트는 곧장 다시 들어올 수 있습니다. tests/providers/devin-stated-reset-retry.test.ts:189 - 새 테스트는 헬퍼에 maxWaitMs: 0을 직접 넣습니다. 어댑터가 그 값을 넘기는지는 보지 않습니다. 어댑터의 그 한 줄을 지워도 이 테스트는 통과합니다. 메인테이너의 판단이 필요한 지점 35초처럼 짧은 리셋도 이제 안에서 기다리지 않습니다. 환경 변수 OPENCODEX_DEVIN_STATED_RESET_WAIT_MS는 헬퍼를 직접 부르는 쪽에만 남고, 실제 Devin 경로는 항상 0이라 이 값을 무시합니다. 콤보 시도는 첫 사건이 에러면 HTTP 502로 감싸고 Retry-After를 넣지 않습니다 (src/server/responses/run-turn-execution.ts:382). 이 429가 이제 그 길로 바로 갑니다. 너의 추천 자리를 붙잡고 잠드는 동작은 끊는 쪽이 맞습니다. 머지해도 됩니다. 실패 메시지에 클라이언트가 이미 읽는 초 단위 힌트가 들어가는지만 한 번 확인해 주세요. 테스트는 어댑터가 maxWaitMs: 0을 넘긴다는 사실만 고정하면 됩니다. 이 댓글은 grok-bot이 작성했습니다 |
|
Review feedback applied on |
The module-mock spy leaked into sibling test files in the same bun test process, so devin-stated-reset-retry and devin-stated-reset-hardening saw a stubbed helper. Drive the real runTurn with a stubbed fetch returning a Connect EOS-trailer refusal instead: the asserted signals are the surfaced error text and the physical GetChatMessage count, which fail fast under the 5s abort signal if the adapter's maxWaitMs: 0 pin is removed or weakened. Register the file in the test layout map and fixture.
|
Consolidated into #5541 as a single related-function aggregate. Source head: All unique Devin contribution commits are carried with original authors and provenance. Six match complete stable patch IDs; the alias-host change matches the code/test/runtime-document patch IDs and its one table-row conflict preserves both contracts. The common f700c56 role-fixture change is already present in both corresponding provider objects on the base, so it was not duplicated. Integration regressions verify exact 429 timing, no raw credential reflection, one physical send, and the alias-bound EU tenant path. Devin tests: 252 passed; layout:18 passed; ratchet:9 passed; typecheck/structure/privacy passed. Full cross-platform CI and independent review remain pending. Closing this duplicate standalone review entry at the author's request after verifying migration. This is not a merge or release claim; remaining integration checks and reviews are tracked on the draft replacement. Original branches are retained. |
…nect trailers (#5574) * fix(devin): suppress untrusted Connect trailer text Connect-RPC EOS trailers propagated upstream-controlled error.message text into adapter error events, API responses, and diagnostic logs, and that text can reflect the credential carried by the request. Parse trailers with an allowlist for known Connect codes, keep the raw message inside the parser for quota/opaque-denial detection, and throw content-free errors carrying only the allowlisted code and hex trace id. (cherry picked from commit 50eab5a) (cherry picked from commit 6efc759) * fix(devin): preserve stated reset delay on content-free trailer errors The trailer suppression left the thrown CloudChatError without the upstream "limit will reset in N" sentence that streamChatEventsWithResetRetry scraped for its cooldown, so a stated-reset 429 failed immediately instead of waiting out the quota window. Parse the delay inside the trailer parser while the raw text still exists and carry it as a typed retryAfterSeconds field; the retry prefers that field and keeps the message scrape for errors whose text still carries a hint. Adds a regression test that drives the real Connect EOS trailer path. (cherry picked from commit 7b7a8b8) (cherry picked from commit b841abd) * fix(devin): carry the stated reset delay in the outward trailer error (cherry picked from commit 4b723ef) (cherry picked from commit 82d452c) * fix(devin): prevent reset waits from holding active-turn capacity (cherry picked from commit df07bc6) (cherry picked from commit 98c9298) * fix(devin): pin the adapter's zero reset wait and correct the surface comment (cherry picked from commit 0e73736) (cherry picked from commit a2936ca) * test(devin): prove the zero reset wait through the adapter's own sends The module-mock spy leaked into sibling test files in the same bun test process, so devin-stated-reset-retry and devin-stated-reset-hardening saw a stubbed helper. Drive the real runTurn with a stubbed fetch returning a Connect EOS-trailer refusal instead: the asserted signals are the surfaced error text and the physical GetChatMessage count, which fail fast under the 5s abort signal if the adapter's maxWaitMs: 0 pin is removed or weakened. Register the file in the test layout map and fixture. (cherry picked from commit 1abc035) (cherry picked from commit 2c4bf00) * fix(devin): bind alias tenant host to credential key (cherry picked from commit d225fc2) (cherry picked from commit 9b54aeb) * test(devin): verify sanitized reset refusals across bound tenants Reconcile #5455 zero-wait coverage with #5143 content-free Connect diagnostics: retain 429 and generated retry timing while proving reflected credentials do not escape and only one inference send occurs. Exercise a matching deprecated alias credential through tenant dispatch and the same refusal path. Preserve the zero wait pin, host allowlist, credential binding, and typed reset field. Document the safe typed retry delay and opt-in helper replay contract. (cherry picked from commit 6f2901b) * fix(devin): select tenant host from the account that owns the transmitted key The consolidated fix bound alias-slot tenant hosts to the transmitted key but still returned the active literal slot's host for any key. Devin keeps several accounts per provider id and the request path injects the admitted account's token, which need not be the active one, while a configured key, a forwarded bearer, or a test token is not stored at all. Each of those could inherit another account's EU or FedStart tenant. resolveDevinApiServer now looks up the stored credential whose access token is exactly the transmitted key, in the configured provider id first and then its deprecated alias, active account first. Only that owner's validated host is used; an owner with an unusable host, an unmatched key, or no key falls back to the validated configured base URL and then the US default. The lookup uses getCredential and listAccounts, which store mocks elsewhere in the suite leave real. Tests pass the owning key where a stored tenant is expected, so the invalid alias-host case now reaches the host validator. New cases cover a mismatched literal slot, an alias owner beside an occupied literal slot, a key neither slot owns, a missing key, a non-active owning account, and a configured key at the adapter boundary. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> --------- Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> Co-authored-by: Epinephrine <luvs01@hanmail.net>
2 participants
Motivation
limit will reset in 21 minutes) was sleeping inside the admitted HTTP turn, holding globally shared active-turn capacity for the whole wait.Description
maxWaitMs: 0tostreamChatEventsWithResetRetryin the Devin adapter so a provider-stated 429 surfaces its parsed Retry-After to the client instead of retaining capacity while waiting.structure/providers-and-adapters.mddocuments the surfaced-reset behavior.tests/providers/devin-stated-reset-retry.test.tsasserts a zero wait allowance surfaces the stated reset without sleeping.Testing
bun test tests/providers/devin-stated-reset-retry.test.ts: 13 tests pass.Summary by CodeRabbit
Bug Fixes
Documentation