fix: retry transient integration tool-list failures during discovery - #4333
Conversation
…-inbox#874) Sentry VERYFRONT-AGENT-C: a single transient failure of the idempotent integration tool-list request abandons discovery immediately and emits the error-level 'Failed to fetch remote integration tool definitions' signature. Red tests assert bounded retry for transient network errors and 5xx responses, recovery without the error log, and single-log degradation on persistent failure.
The tool-list request is read-only and idempotent, yet a single dropped connection or 5xx response abandoned discovery on the first attempt and emitted the error-level "Failed to fetch remote integration tool definitions" log (Sentry VERYFRONT-AGENT-C). Discovery now retries transient failures (network errors and 5xx) up to MAX_INTEGRATION_TOOL_LIST_ATTEMPTS total attempts with a short linear backoff that aborts as soon as the caller's signal fires. Client errors such as 400/401/403 stay non-retried, the projectless-400 debug skip is preserved, and tool execution remains retry-free because tool calls are not idempotent. On exhaustion discovery still degrades to an unavailable catalog with the error-level signature logged exactly once.
|
Warning Review limit reachedNext included review available in 1 minute. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 |
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
|
Note Automatic reviews are paused because your trial's included automatic processing has been used for this period. Upgrade now, or comment "Gitar review" to run a review anytime. Code Review ✅ ApprovedAdds retry logic with bounded backoff to OptionsDisplay: compact → Showing less information. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Gitar |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Reviewer: Codex Findings, ordered by severity:
Verification:
Score breakdown: correctness/completeness 38/40, tests/verification 15/20, reliability/security 15/15, standards/maintainability 11/15, scope/docs/rollout 5/10. Verdict: REQUEST_CHANGES Review-Gate: |
There was a problem hiding this comment.
kwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
There was a problem hiding this comment.
kwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
|
Addressed the findings from the
Verification on pinned Deno 2.7.7: focused suites, all 260 integration steps, |
|
Reviewer: Claude Independent review against issue veryfront/veryfront-issue-inbox#874 (Sentry VERYFRONT-AGENT-C) and the PR acceptance criteria. Findings (ordered by severity)No critical, high, or medium actionable defects found. Non-blocking observations:
Correctness verification
Test verification (local, read-only)Ran with the repository-pinned Deno 2.7.7:
(Note: under an unpinned Deno 2.9.4 the suite fails at module load in Gaps: I did not re-run the red reproduction at VerdictAll seven acceptance criteria are met with matching test evidence; the diff is dependency-free and confined to the integration discovery boundary plus focused tests. The remaining pending CI gates are already required by the PR's own merge conditions. Score: 92/100 Review-Gate: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2a8d335d1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Reviewer: Codex Findings, ordered by severity:
Verification:
Score breakdown: correctness/completeness 32/40, tests/verification 16/20, reliability/security 12/15, standards/maintainability 13/15, scope/docs/rollout 9/10. Verdict: REQUEST_CHANGES Review-Gate: |
There was a problem hiding this comment.
kwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
Code Review — PR #4333Reviewer: Claude SummaryThis PR adds bounded retry (3 attempts, linear backoff) for the idempotent FindingsNo actionable findings. The implementation is correct and well-tested. Detailed AssessmentCorrectness/Completeness (37/40)
Tests/Verification (19/20)
Reliability/Security (14/15)
Standards/Maintainability (14/15)
Scope/Docs/Rollout (9/10)
Verification
Gaps
Score: 93/100 Review-Gate: |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea47fdfe4a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Reviewer: Codex Findings, ordered by severity:
Verification:
Score breakdown: correctness/completeness 35/40, tests/verification 16/20, reliability/security 12/15, standards/maintainability 13/15, scope/docs/rollout 8/10. Verdict: REQUEST_CHANGES Review-Gate: |
There was a problem hiding this comment.
kwakayama has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Claude Code Review — PR #4333Reviewer: Claude SummaryThis PR adds bounded retry (3 attempts, linear backoff) to the idempotent Findings (ordered by severity)1. [Low] Timeout budget is per-attempt, not end-to-end — verify intentFile:
This is likely intentional for discovery (each attempt gets a fair 30s window), and the PR body says "per-request deadline" not "end-to-end deadline", so the contract is consistent. But worth confirming this is the desired behavior, especially since the Severity: Low (documentation ambiguity, not a bug) 2. [Low]
|
| Check | Status |
|---|---|
| CI format | ✅ pass |
| CI test-layout | ✅ pass |
| npm smoke | ✅ pass |
| Windows localhost | ✅ pass |
| proxy binary | ✅ pass |
| CI lint | ⏳ pending |
| CI typecheck | ⏳ pending |
| Coverage shards | ⏳ pending |
| CodeQL analysis | ⏳ pending |
All completed checks pass. Lint, typecheck, and coverage are still pending at review time.
Test coverage assessment
Tests are thorough and well-structured:
- Retry recovery: transient fetch TypeError, 503 server error, response-body transport failure — all verify
fetchCalls === 2and successful recovery (remote-tools.test.ts) - Retry exhaustion: persistent 500 verifies
fetchCalls === MAX_INTEGRATION_TOOL_LIST_ATTEMPTS, error logged exactly once, retries logged at debug (remote-tools.test.ts) - Non-retryable paths: invalid base URL (
fetchCalls === 0), invalid project slug (fetchCalls === 0), malformed UTF-8 body (fetchCalls === 1) (remote-tools.test.ts, hardening test) - Concurrency: concurrent callers share one retry sequence (remote-tools.test.ts)
- Cancellation: caller abort during retry backoff without extra request (hardening test)
- Timeout preservation: integration timeout reason preserved without retry (hardening test)
- Cache interaction: persistent failure cached for current run, retried next run (remote-tools.test.ts)
Gaps
- No test for 4xx non-retryable server responses (e.g., 400, 401, 403 without the projectless-400 path). The
isTransientToolListFailureonly retries>= 500, so 4xx errors are implicitly non-retryable, but an explicit assertion for e.g. 429 or 403 would harden the boundary. - Pending CI checks — lint, typecheck, and coverage shards are not yet complete. No code-level issues suggest they would fail, but final merge should await completion.
Spec compliance (veryfront/veryfront-issue-inbox#874)
| Acceptance criterion | Status |
|---|---|
| Deterministic red regression test | ✅ PR body documents red evidence at ee9dbf8a |
| Minimal implementation fix turns it green | ✅ Focused to discovery retry only |
| Success/negative and retry/cancellation/concurrency coverage | ✅ Comprehensive |
| Lint, typecheck, targeted tests pass | ⏳ Targeted tests pass; lint/typecheck pending CI |
| PR links issue and documents root cause | ✅ Links #874, documents scope and evidence |
Prior review feedback resolution
All three Codex P1/P2 comments from the earlier review rounds have been addressed:
- P1 (reuse retryWithBackoff): ✅ Resolved — now uses
retryWithBackofffrom#veryfront/errors - P2 (avoid retrying local validation): ✅ Resolved — slug + URL validated before retry entry
- P2 (retry body transport failures): ✅ Resolved —
TypeError(excludingInvalidResponseBodyError) is retryable - P2 (preserve timeout reason): ✅ Resolved —
createIntegrationRequestTimeoutError()translation in the attempt callback
Verdict: The implementation is clean, well-scoped, and addresses all spec acceptance criteria. The retryWithBackoff reuse is correct and idiomatic per AGENTS.md line 12. All prior review comments are resolved. The only non-informational finding is a low-severity documentation ambiguity about per-attempt vs end-to-end timeout semantics, which does not block merge. Final merge should await the pending CI checks (lint, typecheck, coverage).
Review-Gate:
Reviewer: Claude
Reviewed-SHA: c7a41ef
Score: 92/100
Actionable-Findings: 0
Verdict: APPROVE
|
Reviewer: Codex Findings, ordered by severity: No actionable findings. Evidence:
Verification with repository-pinned Deno 2.7.7:
Score breakdown: correctness/completeness 40/40, tests/verification 19/20, reliability/security 15/15, standards/maintainability 13/15, scope/docs/rollout 9/10. Verdict: APPROVE Review-Gate: |
Independent reviewReviewer: Codex Findings, ordered by severity: No actionable findings. Evidence:
Verification:
Verdict: APPROVE Review-Gate: |
|
Remote integration tool discovery abandoned the idempotent
/integrations/tools/listrequest on the first transient transport failure or 5xx response, immediately emitting the error-levelFailed to fetch remote integration tool definitionssignature tracked by Sentry group VERYFRONT-AGENT-C. This change adds three bounded attempts with short linear backoff insidediscoverRemoteIntegrationToolCatalog, so a run-scoped promise cache shares one retry sequence across concurrent callers.Scope stays narrow:
retryWithBackoffhelper for attempts, deadlines, backoff, and cancellation.{ status: "unavailable", reason: "request_failed" }.Fixes veryfront/veryfront-issue-inbox#874.
Red evidence
At regression commit
ee9dbf8a11f8bbcd0a4752b1e893cc6fb9c29cfa:All three retry assertions failed because discovery made one request only:
Green verification
Use the repository-pinned Deno 2.7.7:
Final regression coverage proves:
ok.ok.Failed to fetch remote integration tool definitionsrecord.Revert check
Before the final test consolidation, reverting implementation commit
ca5cb4c45cbefbdab16263c95043795d52897850reproduced the three red assertions, and restoring the implementation returned the focused suite to green. The final equivalent assertions now live insrc/integrations/remote-tools.test.tsandsrc/integrations/remote-tools.hardening.test.ts.Acceptance criteria
Deployment and post-deploy verification
Merge only after the exact-head Codex and Claude review gates meet the required score, all threads are resolved, and GitHub reports both
quality gate (merge)andquality gate (artifact)green with no unexplained skipped checks.After the framework release reaches the agent runtime, monitor Sentry VERYFRONT-AGENT-C through at least one normal integration-discovery traffic window. Confirm transient recoveries no longer create events and any persistent outage creates at most one event per exhausted discovery sequence. Keep issue #874 open until the deployed group is quiet or explicitly classified with evidence.