fix: type safety, defensive defaults, and unbounded retry prevention#1553
fix: type safety, defensive defaults, and unbounded retry prevention#1553skyhighbg22-jpg wants to merge 25 commits into
Conversation
skyhighbg22-jpg
force-pushed
the
fix/code-quality-and-defensive-defaults
branch
from
21c2a3c to
4f9da78
Compare
QueryEngine.ts: - Import PERMISSION_MODES runtime constant and validate permissionMode before casting in submitMessage — invalid mode strings fall back to 'default' instead of crashing with ReferenceError: PERMISSION_MODES is not defined (fixes the runtime gap from the original PR) - Use splice(0, length, ...messages) instead of length=0 + push() for atomic array replacement in snip replay, so concurrent readers of getMessages() never observe an empty state withRetry.ts: - Cap persistent retry loop at 100 attempts via PERSISTENT_RETRY_MAX_ATTEMPTS constant — prevents unbounded retry (~8 hours max with exponential backoff and 6-hour reset cap) when the unattended retry path is enabled autoCompact.ts: - Add MIN_AUTOCOMPACT_FAILURE_COOLDOWN_MS = 10_000 floor for OPENCLAUDE_AUTOCOMPACT_FAILURE_COOLDOWN_MS override — prevents misconfiguration from effectively disabling the circuit breaker autoCompact.test.ts: - Update test override from 5000 to 15000 to respect the new 10s minimum floor - Add test case verifying values below the floor (5000, 9999) are rejected and that the floor value (10000) is accepted - Update circuit breaker retry-time expectation from 111_000 to 121_000 to account for the new 15s cooldown override
skyhighbg22-jpg
force-pushed
the
fix/code-quality-and-defensive-defaults
branch
from
4f9da78 to
4d54d5d
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (13)src/**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
{src/commands/**/*.ts,src/services/**/*.ts,src/entrypoints/**/*.ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
{src/services/**/*.ts,src/utils/**/*.ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
{src/integrations/**/*.ts,src/services/**/*.ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx,py,json,md}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.test.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.test.{ts,tsx,js}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{ts,tsx,js,jsx,py}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
**/*⚙️ CodeRabbit configuration file
Files:
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}⚙️ CodeRabbit configuration file
Files:
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}⚙️ CodeRabbit configuration file
Files:
**⚙️ CodeRabbit configuration file
Files:
🔇 Additional comments (2)
📝 WalkthroughWalkthroughThis PR tightens validation and bounds (permission mode, autocompact cooldown), adds a capped persistent-retry mode to withRetry (plus tests), expands test environment isolation to prevent credential leakage, enables an unattended-retry test feature flag, and makes migration write errors errno-aware while including error messages in logs. ChangesRobustness, Error Handling, and Bounds Improvements
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested Reviewers
🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/services/api/withRetry.ts`:
- Around line 398-405: Add telemetry when the persistent retry cap is hit: in
the branch that currently throws CannotRetryError when persistent &&
persistentAttempt >= PERSISTENT_RETRY_MAX_ATTEMPTS, emit a telemetry event
(e.g., follow the existing naming pattern like
tengu_api_persistent_retry_cap_reached) including relevant context (error,
retryContext, persistentAttempt, PERSISTENT_RETRY_MAX_BACKOFF_MS,
PERSISTENT_RETRY_MAX_ATTEMPTS) before throwing the CannotRetryError so the team
can monitor how often sessions exhaust the 100-attempt cap and include a comment
clarifying that the "~8 hours" estimate refers only to the exponential backoff
path.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 231e17c0-f96f-4154-b394-26fac413f618
📒 Files selected for processing (4)
src/QueryEngine.tssrc/services/api/withRetry.tssrc/services/compact/autoCompact.test.tssrc/services/compact/autoCompact.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/api/withRetry.tssrc/services/compact/autoCompact.tssrc/services/compact/autoCompact.test.tssrc/QueryEngine.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/withRetry.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/services/api/withRetry.tssrc/services/compact/autoCompact.tssrc/services/compact/autoCompact.test.tssrc/QueryEngine.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/services/compact/autoCompact.test.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Gitlawb/openclaude PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-06-04T22:10:40.834Z
Learning: Verify that product, trust-model, routing-default, telemetry/network, and permission-policy changes are not hidden inside unrelated cleanup. Flag the PR if the policy decision needs explicit maintainer alignment.
🔇 Additional comments (9)
src/QueryEngine.ts (2)
17-17: LGTM!Also applies to: 552-557, 563-563
946-950: LGTM!src/services/api/withRetry.ts (1)
102-102: LGTM!src/services/compact/autoCompact.ts (2)
84-87: LGTM!
99-103: LGTM!src/services/compact/autoCompact.test.ts (4)
250-255: LGTM!
257-281: LGTM!
482-482: LGTM!
677-677: LGTM!
The new persistent retry cap test in withRetry.test.ts needs the real UNATTENDED_RETRY feature gate to fire, which requires passing --feature=UNATTENDED_RETRY on the bun test command line. Enable it in the standard test, test:full, test:coverage, and test:provider scripts so the test sees the production gate behavior.
Add a regression test that proves the persistent retry path stops after PERSISTENT_MAX_ATTEMPTS=100 retryable 429s by driving the real isPersistentRetryEnabled() gate (no test override seam). Also: - Switch makeError to the new APIError() constructor so the test errors match the real wire shape and exercise the production canRetry/shouldRetry branches - Add CLAUDE_CODE_UNATTENDED_RETRY to the envKeys clear list so the gate isn't poisoned by leaked state from a prior test - Mock src/utils/sleep.js in importFreshWithRetryModule so the exponential-backoff delays don't slow the suite down
The 4 failing tests in CI (first-party Anthropic fetch wrapper, env-only MiniMax routing, OPENAI_MODEL preservation, OpenAI shim options) all sit at the top of the file and are sensitive to leaked env vars from prior test files in the same process. Extend the beforeEach, afterEach, and inline cleanup to clear OPENAI_AUTH_HEADER, OPENAI_AUTH_SCHEME, OPENAI_AUTH_HEADER_VALUE, MIMO_API_KEY, VENICE_API_KEY, and NVIDIA_API_KEY alongside the existing vars, and add ANTHROPIC_API_KEY / ANTHROPIC_AUTH_TOKEN / ANTHROPIC_MODEL to the first test's inline cleanup so it does not rely solely on the global beforeEach when run in isolation.
src/entrypoints/mcp.ts sets CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS=true as a top-level side effect on import. mcp.test.ts imports that file, so the env var is leaked into the rest of the test process. In analyzeContext.mcp.test.ts the kill switch forces getToolSearchMode() to 'standard', which makes isToolSearchEnabled() return false, so the 'keeps deferred MCP schemas excluded' test sees isDeferred=false and returns mcpToolTokens=1000 instead of 0. Clear the kill switch and ENABLE_TOOL_SEARCH in beforeEach (restoring the original values in afterEach) so the test observes the production default of tool search enabled regardless of test ordering.
…etry override to test-only module var QueryEngine.ts: - Switch SDK init permissionMode validation from the internal PERMISSION_MODES set to EXTERNAL_PERMISSION_MODES. The internal set can include the classifier-only 'auto' mode that is not a valid wire value for the SDK system/init payload, so emitting it would produce an unsupported configuration. withRetry.ts: - Rename PERSISTENT_RETRY_MAX_ATTEMPTS to PERSISTENT_MAX_ATTEMPTS to match the convention of the other PERSISTENT_* constants and re-export it as _PERSISTENT_MAX_ATTEMPTS_FOR_TEST for unit-test assertion of the cap value (no runtime override seam). - Hoist isPersistentRetryEnabled() into a local persistentRetryEnabled const at the top of withRetry() so all call sites see a consistent snapshot within a single retry chain. - Thread persistentRetryEnabled through shouldRetry() so its branch is decided once per chain rather than re-reading the feature flag and env var on every attempt.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Restore full-suite client-test isolation
src/services/api/client.test.ts:117
The shared setup now clearsOPENAI_API_FORMAT, but it still leavesOPENAI_AUTH_HEADER,OPENAI_AUTH_SCHEME, andOPENAI_AUTH_HEADER_VALUEbehind unless a test happens to call the MiniMax-only cleanup helper. In the current GitHubsmoke-and-testsrun, those stale auth overrides bleed into the full suite and make fourclient.test.tscases fail with "Could not resolve authentication method" whengetAnthropicClient()reaches the Anthropic SDK. Please clear those auth override env vars in the shared setup/affected inline cleanup so the focused file and the full suite run with the same provider state. -
[P3] Complete CodeRabbit's persistent-retry cap request
src/services/api/withRetry.ts:403
CodeRabbit's unresolved thread is still valid here: the new persistent retry cap throwsCannotRetryErrorwithout emitting a telemetry event, and the adjacent comment still describes 100 attempts as about eight hours even though the reset-delay path can wait up to six hours per retry. Please complete that request by logging a cap-reached event before throwing and clarifying that the eight-hour estimate only applies to the exponential-backoff path.
- Only swallow EEXIST (file already exists) errors in migrateChangelogFromConfig - Rethrow all other write failures (permissions, disk full, etc.) - Log migration errors instead of silently ignoring them - This ensures migration failures are surfaced and will retry on next startup
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/utils/releaseNotes.ts (1)
227-242:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNarrow EEXIST suppression to
writeFileonlyLine 228 (
mkdir) can also throwEEXISTfor path-collision states. With the current sharedcatch, that gets treated as success and Line 242 removescachedChangelogeven though migration may not have written the file.Suggested fix
- try { - await mkdir(dirname(cachePath), { recursive: true }) - await writeFile(cachePath, config.cachedChangelog, { - encoding: 'utf-8', - flag: 'wx', // Write only if file doesn't exist - }) - } catch (error) { - // File already exists (EEXIST) is fine - skip silently - if (getErrnoCode(error) !== 'EEXIST') { - throw error - } - } + await mkdir(dirname(cachePath), { recursive: true }) + try { + await writeFile(cachePath, config.cachedChangelog, { + encoding: 'utf-8', + flag: 'wx', // Write only if file doesn't exist + }) + } catch (error) { + // File already exists (EEXIST) is fine - skip silently + if (getErrnoCode(error) !== 'EEXIST') { + throw error + } + }🤖 Prompt for 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. In `@src/utils/releaseNotes.ts` around lines 227 - 242, The current try/catch wraps both mkdir and writeFile so an EEXIST from mkdir incorrectly counts as a successful write; split the operations so mkdir(dirname(cachePath), { recursive: true }) is awaited outside the writeFile try/catch, then wrap only writeFile(cachePath, config.cachedChangelog, { encoding: 'utf-8', flag: 'wx' }) in a catch that suppresses only EEXIST (via getErrnoCode(error) === 'EEXIST') and rethrows otherwise; call saveGlobalConfig(({ cachedChangelog: _, ...rest }) => rest) only after the writeFile either succeeded or threw EEXIST.
🤖 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.
Outside diff comments:
In `@src/utils/releaseNotes.ts`:
- Around line 227-242: The current try/catch wraps both mkdir and writeFile so
an EEXIST from mkdir incorrectly counts as a successful write; split the
operations so mkdir(dirname(cachePath), { recursive: true }) is awaited outside
the writeFile try/catch, then wrap only writeFile(cachePath,
config.cachedChangelog, { encoding: 'utf-8', flag: 'wx' }) in a catch that
suppresses only EEXIST (via getErrnoCode(error) === 'EEXIST') and rethrows
otherwise; call saveGlobalConfig(({ cachedChangelog: _, ...rest }) => rest) only
after the writeFile either succeeded or threw EEXIST.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5060761e-d180-4db0-9dd6-f17db2162dc2
📒 Files selected for processing (2)
src/main.tsxsrc/utils/releaseNotes.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: smoke-and-tests
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/main.tsxsrc/utils/releaseNotes.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Typecheck TypeScript code before submitting
Files:
src/main.tsxsrc/utils/releaseNotes.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/main.tsxsrc/utils/releaseNotes.ts
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}
⚙️ CodeRabbit configuration file
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}: Review install, launcher, build, packaging, startup, and entrypoint changes for cross-platform compatibility, tracked-source rewrites, env/config precedence, and release safety. Block on changes that can break Windows/macOS/Linux startup or publish unexpected artifacts.
Files:
src/main.tsx
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/main.tsxsrc/utils/releaseNotes.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Gitlawb/openclaude PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-06-04T22:10:40.834Z
Learning: Verify that product, trust-model, routing-default, telemetry/network, and permission-policy changes are not hidden inside unrelated cleanup. Flag the PR if the policy decision needs explicit maintainer alignment.
Learnt from: CR
Repo: Gitlawb/openclaude PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-06-06T00:19:44.234Z
Learning: Verify that product, trust-model, routing-default, telemetry/network, and permission-policy changes are not hidden inside unrelated cleanup. Flag the PR if the policy decision needs explicit maintainer alignment.
Learnt from: CR
Repo: Gitlawb/openclaude PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-06-06T00:19:34.989Z
Learning: PR description must include: what changed and why, user or developer impact, exact checks you ran, linked issue, screenshots for UI/terminal changes, and which provider path was tested
🔇 Additional comments (1)
src/main.tsx (1)
349-355: LGTM!
- Ensure mkdir runs before writeFile try/catch - Only suppress EEXIST from writeFile, not mkdir - Prevents EEXIST from mkdir incorrectly counting as successful write
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/utils/releaseNotes.ts`:
- Around line 226-228: The mkdir call that ensures the cache directory exists
should suppress EEXIST like the pattern in fsOperations.ts to handle the
Bun/Windows readonly-folder edge case; update the await
mkdir(dirname(cachePath), { recursive: true }) in releaseNotes.ts to catch
errors and ignore only errors with code 'EEXIST' (rethrow other errors) so the
existing-directory case does not trigger the "Changelog migration failed" retry
loop.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0e34a3ec-6c02-4c06-bc41-fe8b3e640dd7
📒 Files selected for processing (1)
src/utils/releaseNotes.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: smoke-and-tests
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/utils/releaseNotes.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Typecheck TypeScript code before submitting
Files:
src/utils/releaseNotes.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/utils/releaseNotes.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/utils/releaseNotes.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: Gitlawb/openclaude PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-06-04T22:10:40.834Z
Learning: Verify that product, trust-model, routing-default, telemetry/network, and permission-policy changes are not hidden inside unrelated cleanup. Flag the PR if the policy decision needs explicit maintainer alignment.
Learnt from: CR
Repo: Gitlawb/openclaude PR: 0
File: coderabbit-custom-pre-merge-checks-unique-id-file-non-traceable-F7F2B60C-1728-4C9A-8889-4F2235E186CA.txt:0-0
Timestamp: 2026-06-06T00:19:44.234Z
Learning: Verify that product, trust-model, routing-default, telemetry/network, and permission-policy changes are not hidden inside unrelated cleanup. Flag the PR if the policy decision needs explicit maintainer alignment.
Learnt from: CR
Repo: Gitlawb/openclaude PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2026-06-06T00:19:34.989Z
Learning: PR description must include: what changed and why, user or developer impact, exact checks you ran, linked issue, screenshots for UI/terminal changes, and which provider path was tested
🔇 Additional comments (1)
src/utils/releaseNotes.ts (1)
230-244: LGTM!
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Summary: thanks for the quick follow-ups here. There are still a couple of items to wrap up before this can be considered merge-ready: please address the active CodeRabbit finding and get the failing test path green. Since OpenClaude is moving quickly, it is also possible that rebasing on the latest main will clear some or all of the test failures if they are coming from recent upstream test-order or fixture changes.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Restore a green full test path before changing the suite flags
package.json:53
This PR changes the top-leveltest,test:full,test:coverage, andtest:providerscripts to run the entire suite with--feature=UNATTENDED_RETRY, but the livesmoke-and-testscheck is now failing inbun run checkduringbun run test:full. The failed Linux job gets through build and focused type tests, then reports foursrc/services/api/client.test.tsfailures where the Anthropic SDK throwsCould not resolve authentication methodin the first-party and MiniMax env-only cases. The focused changed tests pass, so the new full-suite execution path is exposing order/feature-flag isolation failures that still need to be cleared. Given how rapidlymainis changing, a rebase may be enough if this is already fixed upstream; otherwise, please make the full CI script pass or scope the feature flag so only the persistent-retry test runs underUNATTENDED_RETRY. -
[P2] Complete CodeRabbit's mkdir EEXIST request
src/utils/releaseNotes.ts:227
CodeRabbit's latest changes-requested item is still valid here:mkdir(dirname(cachePath), { recursive: true })now runs outside the write-filetry/catch, so anEEXISTfrom the directory creation step still escapes as a migration failure even though the requested fix was to suppress only that benign mkdir case and rethrow other errors. Please complete CodeRabbit's request by handlingEEXISTaround themkdircall as well, then keep thesaveGlobalConfigcall gated on successful migration or an already-existing target.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.tsx (1)
345-350: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPreserve original migration error as
causewhen wrapping.You improved message clarity, but wrapping without
causedrops causal linkage for diagnostics in some sinks. Keep both message context and cause chain.Suggested patch
migrateChangelogFromConfig().catch(error => { logError( new Error( `Changelog migration failed; will retry on next startup: ${errorMessage(error)}`, + { cause: error instanceof Error ? error : undefined }, ), ) });🤖 Prompt for 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. In `@src/main.tsx` around lines 345 - 350, Wrap the original migration error as the cause when creating the new Error in the migrateChangelogFromConfig().catch handler so the causal chain is preserved; update the Error construction in the catch block used with logError to use the Error constructor with an options object (e.g., new Error(`Changelog migration failed; will retry on next startup: ${errorMessage(error)}`, { cause: error })) so migrateChangelogFromConfig's original error is available as error.cause while keeping the improved message.
🤖 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 `@src/QueryEngine.ts`:
- Around line 949-950: The current replacement uses two operations
(this.mutableMessages.length = 0 followed by
this.mutableMessages.push(...snipResult.messages)) which creates a transient
empty state; change it to a single in-place splice to atomically replace the
contents (e.g., this.mutableMessages.splice(0, this.mutableMessages.length,
...snipResult.messages)) so the array is swapped without ever being empty —
update the code in the QueryEngine method where mutableMessages is being updated
and use splice with snipResult.messages to perform the swap.
- Around line 1469-1472: The length-based computation for executed can drop a
boundary marker when no UUIDs were removed; update the logic in QueryEngine.ts
(the place that builds the returned object with messages: projected and
executed) so that executed is true not only when projected.length !==
store.length + 1 but also when the projected messages include a boundary marker
(e.g., check the boundary flag/type on the relevant message), ensuring
submitMessage() will persist boundary messages even if no removals occurred;
reference the projected array, the store length check, and submitMessage()
behavior when adjusting the executed determination.
---
Outside diff comments:
In `@src/main.tsx`:
- Around line 345-350: Wrap the original migration error as the cause when
creating the new Error in the migrateChangelogFromConfig().catch handler so the
causal chain is preserved; update the Error construction in the catch block used
with logError to use the Error constructor with an options object (e.g., new
Error(`Changelog migration failed; will retry on next startup:
${errorMessage(error)}`, { cause: error })) so migrateChangelogFromConfig's
original error is available as error.cause while keeping the improved message.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a8962d5e-baa5-44fc-9ccb-abe65aa5ee18
📒 Files selected for processing (3)
package.jsonsrc/QueryEngine.tssrc/main.tsx
📜 Review details
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: smoke-and-tests
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/QueryEngine.tssrc/main.tsx
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/QueryEngine.tssrc/main.tsxpackage.json
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/QueryEngine.tssrc/main.tsxpackage.json
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}
⚙️ CodeRabbit configuration file
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}: Review install, launcher, build, packaging, startup, and entrypoint changes for cross-platform compatibility, tracked-source rewrites, env/config precedence, and release safety. Block on changes that can break Windows/macOS/Linux startup or publish unexpected artifacts.
Files:
src/main.tsxpackage.json
🔇 Additional comments (3)
package.json (1)
3-3: LGTM!Also applies to: 23-24, 55-57, 67-67, 80-80, 91-91, 171-171
src/main.tsx (2)
36-37: LGTM!Also applies to: 106-107, 123-124, 223-226, 264-269, 725-727, 1468-1517, 2730-2731, 2844-2847, 2987-2994, 3175-3175, 4293-4293
944-951: Check--effort xhighhandling end-to-end (and preserve migration errorcause)
src/main.tsxnow accepts--effort xhigh; confirm the full effort pipeline (parsing → persistence → API payload shaping) preservesxhighwhen supported and doesn’t silently fall back later. The current search output only showsEffortValuebeing threaded intohandlePromptSubmit, not thexhighmapping logic itself.- When wrapping migration errors in
src/main.tsx, preserve the original error ascauseto keep stack/cause chains intact for logs.
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.tsx (1)
345-350: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winPreserve original migration error as
causewhen wrapping.You improved message clarity, but wrapping without
causedrops causal linkage for diagnostics in some sinks. Keep both message context and cause chain.Suggested patch
migrateChangelogFromConfig().catch(error => { logError( new Error( `Changelog migration failed; will retry on next startup: ${errorMessage(error)}`, + { cause: error instanceof Error ? error : undefined }, ), ) });🤖 Prompt for 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. In `@src/main.tsx` around lines 345 - 350, Wrap the original migration error as the cause when creating the new Error in the migrateChangelogFromConfig().catch handler so the causal chain is preserved; update the Error construction in the catch block used with logError to use the Error constructor with an options object (e.g., new Error(`Changelog migration failed; will retry on next startup: ${errorMessage(error)}`, { cause: error })) so migrateChangelogFromConfig's original error is available as error.cause while keeping the improved message.
🤖 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 `@src/QueryEngine.ts`:
- Around line 949-950: The current replacement uses two operations
(this.mutableMessages.length = 0 followed by
this.mutableMessages.push(...snipResult.messages)) which creates a transient
empty state; change it to a single in-place splice to atomically replace the
contents (e.g., this.mutableMessages.splice(0, this.mutableMessages.length,
...snipResult.messages)) so the array is swapped without ever being empty —
update the code in the QueryEngine method where mutableMessages is being updated
and use splice with snipResult.messages to perform the swap.
- Around line 1469-1472: The length-based computation for executed can drop a
boundary marker when no UUIDs were removed; update the logic in QueryEngine.ts
(the place that builds the returned object with messages: projected and
executed) so that executed is true not only when projected.length !==
store.length + 1 but also when the projected messages include a boundary marker
(e.g., check the boundary flag/type on the relevant message), ensuring
submitMessage() will persist boundary messages even if no removals occurred;
reference the projected array, the store length check, and submitMessage()
behavior when adjusting the executed determination.
---
Outside diff comments:
In `@src/main.tsx`:
- Around line 345-350: Wrap the original migration error as the cause when
creating the new Error in the migrateChangelogFromConfig().catch handler so the
causal chain is preserved; update the Error construction in the catch block used
with logError to use the Error constructor with an options object (e.g., new
Error(`Changelog migration failed; will retry on next startup:
${errorMessage(error)}`, { cause: error })) so migrateChangelogFromConfig's
original error is available as error.cause while keeping the improved message.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a8962d5e-baa5-44fc-9ccb-abe65aa5ee18
📒 Files selected for processing (3)
package.jsonsrc/QueryEngine.tssrc/main.tsx
📜 Review details
🔇 Additional comments (3)
package.json (1)
3-3: LGTM!Also applies to: 23-24, 55-57, 67-67, 80-80, 91-91, 171-171
src/main.tsx (2)
36-37: LGTM!Also applies to: 106-107, 123-124, 223-226, 264-269, 725-727, 1468-1517, 2730-2731, 2844-2847, 2987-2994, 3175-3175, 4293-4293
944-951: Check--effort xhighhandling end-to-end (and preserve migration errorcause)
src/main.tsxnow accepts--effort xhigh; confirm the full effort pipeline (parsing → persistence → API payload shaping) preservesxhighwhen supported and doesn’t silently fall back later. The current search output only showsEffortValuebeing threaded intohandlePromptSubmit, not thexhighmapping logic itself.- When wrapping migration errors in
src/main.tsx, preserve the original error ascauseto keep stack/cause chains intact for logs.
🛑 Comments failed to post (2)
src/QueryEngine.ts (2)
949-950:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace clear+push with atomic splice for snip replay replacement.
This still introduces a transient empty
mutableMessagesstate during replacement. Use onespliceto swap contents in-place.Suggested fix
- this.mutableMessages.length = 0 - this.mutableMessages.push(...snipResult.messages) + this.mutableMessages.splice( + 0, + this.mutableMessages.length, + ...snipResult.messages, + )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.this.mutableMessages.splice( 0, this.mutableMessages.length, ...snipResult.messages, )🤖 Prompt for 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. In `@src/QueryEngine.ts` around lines 949 - 950, The current replacement uses two operations (this.mutableMessages.length = 0 followed by this.mutableMessages.push(...snipResult.messages)) which creates a transient empty state; change it to a single in-place splice to atomically replace the contents (e.g., this.mutableMessages.splice(0, this.mutableMessages.length, ...snipResult.messages)) so the array is swapped without ever being empty — update the code in the QueryEngine method where mutableMessages is being updated and use splice with snipResult.messages to perform the swap.
1469-1472:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winSnip boundary can be dropped when no UUIDs are removed.
executedis derived from length delta, butsubmitMessage()treatsexecuted === falseas “don’t persist boundary” and stillbreaks. For boundary messages with no removals, the marker is lost, which can break replay/resume consistency.Suggested fix
return { messages: projected, - executed: projected.length !== store.length + 1, + // Callback ran for a confirmed snip boundary; always apply result. + executed: true, }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.return { messages: projected, // Callback ran for a confirmed snip boundary; always apply result. executed: true, }🤖 Prompt for 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. In `@src/QueryEngine.ts` around lines 1469 - 1472, The length-based computation for executed can drop a boundary marker when no UUIDs were removed; update the logic in QueryEngine.ts (the place that builds the returned object with messages: projected and executed) so that executed is true not only when projected.length !== store.length + 1 but also when the projected messages include a boundary marker (e.g., check the boundary flag/type on the relevant message), ensuring submitMessage() will persist boundary messages even if no removals occurred; reference the projected array, the store length check, and submitMessage() behavior when adjusting the executed determination.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Remove the stale retry guard that still calls
shouldRetrywithout the persistent flag
src/services/api/withRetry.ts:462
This PR changesshouldRetryto requirepersistentRetryEnabled, but the old guard remains a few lines later and still callsshouldRetry(error)with one argument. That now fails root typecheck (TS2554: Expected 2 arguments, but got 1) on the changed file, and it also bypasses the new persistent-retry decision that line 429 just made. Please remove the duplicate guard or thread the flag through the remaining call so persistent 429/529 retries are not rejected immediately. -
[P1] Fix the failing GitHub
smoke-and-testscheck before merging
package.json:55
The current PR head is red on GitHub:smoke-and-testsfails inbun run checkduringtest:fullwith 5 failures. The new persistent retry cap test only calls the operation once instead of the expected 101 times, and fourclient.test.tscases fail because the Anthropic SDK cannot resolve an auth method. Since this PR changes the top-level test scripts to run the whole suite with--feature=UNATTENDED_RETRY, please either keep that feature flag scoped to the retry-cap test path or fix the leaked feature/env/module state so the full CI smoke job is green. -
[P2] Complete CodeRabbit's mkdir EEXIST request for changelog migration
src/utils/releaseNotes.ts:227
CodeRabbit's review thread is still unresolved, and the current patch still callsawait mkdir(dirname(cachePath), { recursive: true })outside theEEXIST-suppressing block. The write path now handlesEEXIST, but the directory creation can still throw before the write on the Bun/Windows readonly-folder case the review identified, which leaves startup logging "Changelog migration failed" and retries on every launch even though the directory exists. Please complete that review request by catchingmkdirerrors here and ignoring onlyEEXIST, rethrowing everything else.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/utils/analyzeContext.mcp.test.ts (1)
18-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAcquire/release the shared mutation lock around all env mutations.
Right now env writes in Line 18-21 happen before lock acquisition, and env restoration in Line 23-31 happens after lock release. That leaves mutation windows unprotected and can reintroduce parallel test flakiness.
Suggested fix (move lock to outer hooks)
beforeEach(() => { + await acquireSharedMutationLock('utils/analyzeContext.mcp.test.ts') delete process.env.CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS delete process.env.ENABLE_TOOL_SEARCH }) afterEach(() => { - for (const [key, value] of Object.entries(originalEnv)) { - if (value === undefined) { - delete process.env[key] - } else { - process.env[key] = value + try { + for (const [key, value] of Object.entries(originalEnv)) { + if (value === undefined) { + delete process.env[key] + } else { + process.env[key] = value + } } + } finally { + releaseSharedMutationLock() } }) describe('countMcpToolTokens', () => { - beforeEach(async () => { - await acquireSharedMutationLock('utils/analyzeContext.mcp.test.ts') + beforeEach(() => { process.env.ENABLE_TOOL_SEARCH = 'true' delete process.env.CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS delete process.env.ANTHROPIC_BASE_URL }) afterEach(() => { - try { - for (const [key, value] of Object.entries(savedToolSearchEnv)) { - if (value === undefined) { - delete process.env[key] - } else { - process.env[key] = value - } + for (const [key, value] of Object.entries(savedToolSearchEnv)) { + if (value === undefined) { + delete process.env[key] + } else { + process.env[key] = value } - } finally { - releaseSharedMutationLock() } })As per coding guidelines, test reviews should block on poor isolation of global/env/config state and async cleanup risks in changed test paths.
Also applies to: 79-84, 86-97, 23-31
🤖 Prompt for 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. In `@src/utils/analyzeContext.mcp.test.ts` around lines 18 - 21, Wrap all environment variable mutations in the shared mutation lock by acquiring the lock at the start of the top-level beforeEach and releasing it at the end of the corresponding afterEach so that delete/set of process.env.CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS and process.env.ENABLE_TOOL_SEARCH (and the restore logic currently in afterEach) occur while the lock is held; move the lock acquisition to cover the lines in the beforeEach that mutate env (currently deleting those vars) and move the lock release to run only after the env restoration logic in afterEach, ensuring all env writes/readbacks between beforeEach and afterEach are protected.Source: Coding guidelines
src/services/api/client.test.ts (1)
35-46:⚠️ Potential issue | 🔴 CriticalFix TS1117 in
src/services/api/client.test.ts: remove duplicate keys inoriginalEnv
const originalEnvdefinesOPENAI_AUTH_HEADER,OPENAI_AUTH_SCHEME, andOPENAI_AUTH_HEADER_VALUEtwice, triggering TS1117 typecheck/CI failure. Remove one of the duplicate occurrences so each key appears only once.🤖 Prompt for 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. In `@src/services/api/client.test.ts` around lines 35 - 46, The object originalEnv in the test contains duplicate keys (OPENAI_AUTH_HEADER, OPENAI_AUTH_SCHEME, OPENAI_AUTH_HEADER_VALUE) causing TS1117; edit the originalEnv declaration to remove the second set of those three duplicated properties so each env key appears only once (locate the originalEnv constant in src/services/api/client.test.ts and delete the repeated OPENAI_AUTH_HEADER, OPENAI_AUTH_SCHEME, and OPENAI_AUTH_HEADER_VALUE entries).Sources: Coding guidelines, Pipeline failures
🤖 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.
Outside diff comments:
In `@src/services/api/client.test.ts`:
- Around line 35-46: The object originalEnv in the test contains duplicate keys
(OPENAI_AUTH_HEADER, OPENAI_AUTH_SCHEME, OPENAI_AUTH_HEADER_VALUE) causing
TS1117; edit the originalEnv declaration to remove the second set of those three
duplicated properties so each env key appears only once (locate the originalEnv
constant in src/services/api/client.test.ts and delete the repeated
OPENAI_AUTH_HEADER, OPENAI_AUTH_SCHEME, and OPENAI_AUTH_HEADER_VALUE entries).
In `@src/utils/analyzeContext.mcp.test.ts`:
- Around line 18-21: Wrap all environment variable mutations in the shared
mutation lock by acquiring the lock at the start of the top-level beforeEach and
releasing it at the end of the corresponding afterEach so that delete/set of
process.env.CLAUDE_CODE_DISABLE_EXPERIMENTAL_BETAS and
process.env.ENABLE_TOOL_SEARCH (and the restore logic currently in afterEach)
occur while the lock is held; move the lock acquisition to cover the lines in
the beforeEach that mutate env (currently deleting those vars) and move the lock
release to run only after the env restoration logic in afterEach, ensuring all
env writes/readbacks between beforeEach and afterEach are protected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bac08e70-4551-4ae3-8886-c9c99e91540b
📒 Files selected for processing (3)
package.jsonsrc/services/api/client.test.tssrc/utils/analyzeContext.mcp.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
package.jsonsrc/utils/analyzeContext.mcp.test.tssrc/services/api/client.test.ts
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}
⚙️ CodeRabbit configuration file
{bin/**,scripts/**,package.json,src/setup.ts,src/main.tsx,src/entrypoints/**}: Review install, launcher, build, packaging, startup, and entrypoint changes for cross-platform compatibility, tracked-source rewrites, env/config precedence, and release safety. Block on changes that can break Windows/macOS/Linux startup or publish unexpected artifacts.
Files:
package.json
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
package.jsonsrc/utils/analyzeContext.mcp.test.tssrc/services/api/client.test.ts
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx,js,jsx,py}: Follow the existing code style in the touched files
Keep comments useful and concise
Files:
src/utils/analyzeContext.mcp.test.tssrc/services/api/client.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Typecheck TypeScript code before submitting (use
bun run typecheck)
Files:
src/utils/analyzeContext.mcp.test.tssrc/services/api/client.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/utils/analyzeContext.mcp.test.tssrc/services/api/client.test.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/client.test.ts
🪛 GitHub Actions: PR Checks / 0_typecheck.txt
src/services/api/client.test.ts
[error] 44-46: TypeScript (TS1117): An object literal cannot have multiple properties with the same name.
🪛 GitHub Actions: PR Checks / typecheck
src/services/api/client.test.ts
[error] 44-46: TypeScript (TS1117): An object literal cannot have multiple properties with the same name.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found one issue that still needs to be addressed.
Findings
- [P3] Complete the prior snip-replay description cleanup
src/QueryEngine.ts:949
jatmn's prior review item about the stale snip-replay claim is still valid in the current PR description: it saysQueryEnginenow usessplice(0, length, ...messages)so concurrent readers never observe an emptymutableMessagesarray, but the current code still leaves thelength = 0followed bypush(...snipResult.messages)behavior in place. Since this PR is no longer implementing that atomic replacement, please complete that prior request by removing the stale claim from the description, or include the claimed implementation and tests if it is still intended here.
Restores the atomic array replacement using splice(0, length, ...messages) instead of length=0 + push(...) that was claimed in commit 4d54d5d but lost during merge. This ensures concurrent readers of getMessages() never observe an empty mutableMessages array during snip replay, matching the compact_boundary behavior.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found an issue that needs to be addressed before this is ready.
Findings
- [P1] Keep release/raw Bun test paths green
src/services/api/withRetry.test.ts:536
The new persistent retry test only exercises the production gate when Bun is started with--feature=UNATTENDED_RETRY, and the package scripts were updated to add that flag. However, the release workflow still runsbun test --max-concurrency=1directly, so this changed test fails on the release path:operationis called once instead of 101 times becausefeature('UNATTENDED_RETRY')is false. Please either update every rawbun testentry point that runs this file, including.github/workflows/release.yml, or make the test skip/use a feature-enabled runner path so release publishing is not blocked.
The persistent retry cap test requires the UNATTENDED_RETRY feature flag to be enabled. The release workflow was running 'bun test --max-concurrency=1' without the feature flag, causing the test to fail on the release path. This aligns the release workflow with the package.json test scripts which all include --feature=UNATTENDED_RETRY.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.
Findings
-
[P2] Keep documented raw Bun test commands green
src/services/api/withRetry.test.ts:536
The release workflow and package scripts now pass--feature=UNATTENDED_RETRY, but the repo still documents raw Bun test entry points for contributors (bun testfor the full unit suite andbun test path/to/file.test.tsfor focused runs). This new test fails under those documented commands becausefeature('UNATTENDED_RETRY')remains false, so the retry loop stops after one call instead of the expected 101. Please either make this test skip or adapt itself when the feature flag is unavailable, or update every documented raw/focused Bun test command to include the required feature flag so the documented validation path stays green. -
[P2] Normalize the REPL bridge system/init permission mode too
src/hooks/useReplBridge.tsx:310
The newQueryEngineguard keeps internal permission modes likeautooff the SDKsystem/initwire, butbuildSystemInitMessageis also called from the REPL bridge and that path still passesstate_0.toolPermissionContext.modedirectly.SDKSystemMessageSchemaonly acceptsdefault,acceptEdits,bypassPermissions,fullAccess,plan, anddontAsk, so an auto-mode REPL bridge session can still emit a schema-invalidpermissionMode: "auto"init event to remote clients. Please apply the same external-mode normalization to this caller, or move the normalization intobuildSystemInitMessageso both documented call sites stay in sync.
… retry cap - Remove duplicate OPENAI_AUTH_HEADER/SCHEME/VALUE deletes from clearEnvForMiniMaxOnlyTest() (shared beforeEach already clears them) - Clarify persistent retry cap comment: the ~8h estimate only applies to the exponential-backoff path; the reset-delay path (up to PERSISTENT_RESET_CAP_MS / 6h per attempt) can take far longer - Telemetry event at retry cap already present from prior commit
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found issues that still need to be addressed.
Findings
-
[P2] Keep documented raw Bun test commands green
src/services/api/withRetry.test.ts:535
The release workflow and package scripts now pass--feature=UNATTENDED_RETRY, but the repo still documents raw Bun test entry points for contributors (bun testfor the full unit suite andbun test path/to/file.test.ts/bun test ./path/to/test-file.test.tsfor focused runs). This new test still fails under that documented path:bun test src/services/api/withRetry.test.tsruns withfeature('UNATTENDED_RETRY') === false, so the operation is called once instead of the expected 101 times. Please keep the coverage intact by either updating every documented raw/focused Bun test command to include the required feature flag, or by making the test explicitly assert both modes: the 100-retry cap when the feature is enabled, and the normal non-persistent retry behavior when the feature is unavailable. -
[P2] Normalize the REPL bridge system/init permission mode too
src/hooks/useReplBridge.tsx:310
The newQueryEngineguard keeps internal permission modes likeautooff the SDKsystem/initwire, but the REPL bridge caller still passesstate_0.toolPermissionContext.modedirectly intobuildSystemInitMessage. The SDKPermissionModewire type only acceptsdefault,acceptEdits,bypassPermissions,fullAccess,plan, anddontAsk, so an auto-mode REPL bridge session can still emit a schema-invalidpermissionMode: "auto"init event to remote clients. Please apply the same external-mode normalization to this caller, or move the normalization intobuildSystemInitMessageso both documented call sites stay in sync.
…_RETRY Updated test to account for feature flag behavior in retry logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/services/api/withRetry.test.ts`:
- Around line 565-570: The expectedCalls calculation uses
`_PERSISTENT_MAX_ATTEMPTS_FOR_TEST` which is a constant that always has the
value 100 regardless of feature compilation, causing the condition to always
evaluate true and expectedCalls to always be 101. Replace the constant check
with a call to the actual feature gate function (such as
`isPersistentRetryEnabled()` or the equivalent that checks whether the
UNATTENDED_RETRY feature is compiled and enabled) so the test correctly expects
1 call when the feature is disabled and 101 calls when enabled.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 979214fe-1454-43b7-b809-e624f1a1a0d6
📒 Files selected for processing (1)
src/services/api/withRetry.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx,js,jsx,py}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Keep comments useful and concise in code
Files:
src/services/api/withRetry.test.ts
**/*
⚙️ CodeRabbit configuration file
**/*: Apply the OpenClaude maintainer review rubric from AGENTS.md. Review the current diff, not stale discussion context. Separate real blockers from suggestions. Do not request changes for vague style churn. Treat approval as merge-ready from CodeRabbit's side, pending required human review and GitHub Checks. If checks are failing or unavailable, say so clearly instead of implying the PR is fully ready.
Files:
src/services/api/withRetry.test.ts
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}
⚙️ CodeRabbit configuration file
{src/services/api/**,src/integrations/**,src/utils/model/**,src/utils/provider*.ts,src/commands/provider/**}: Review provider routing, model selection, env precedence, auth/token handling, OpenAI-compatible shims, retries, proxy behavior, and outbound HTTP behavior with high scrutiny. Block on silent default changes, hidden fallback expansion, credential reuse mistakes, hardcoded provider assumptions, or new network reach that is not intentional and documented.
Files:
src/services/api/withRetry.test.ts
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}
⚙️ CodeRabbit configuration file
{src/**/*.test.ts,src/**/*.test.tsx,tests/**,scripts/**/*.test.ts,vscode-extension/**/*.test.js}: Review tests for meaningful coverage of the changed behavior, isolation of global/env/config state, async cleanup, fake timers, provider profile leaks, and Windows-compatible assumptions. Block when risky runtime changes lack focused regression coverage or tests assert implementation details while missing the user-visible behavior.
Files:
src/services/api/withRetry.test.ts
**
⚙️ CodeRabbit configuration file
**: # Contributing to OpenClaudeThanks for contributing.
OpenClaude is a fast-moving open-source coding-agent CLI with support for multiple providers, local backends, MCP, and a terminal-first workflow. The best contributions here are focused, well-tested, and easy to review.
Before You Start
- Search existing issues and discussions before opening a new thread.
- Check open pull requests for work that overlaps with your contribution. If a PR already exists that addresses the same change, open an issue or discussion first to align on direction — duplicate PRs may be closed without review.
- Use issues for confirmed bugs and actionable feature work.
- Use discussions for setup help, ideas, and general community conversation.
- For larger changes, open an issue first so the scope is clear before implementation.
- For security reports, follow SECURITY.md.
Pull Requests
Every PR needs a reason. Your PR description must include:
- what changed and why
- the user or developer impact
- the exact checks you ran
- a linked issue when one exists, using
Fixes#123, `Closes `#123, or another clear link- screenshots when the PR touches UI, terminal presentation, or the VS Code extension
- which provider path was tested when the PR changes provider behavior
The PR author is responsible for ensuring their PR is merge-ready. PRs with merge conflicts will not be reviewed or approved until the conflicts are resolved.
Issues are the recommended starting point for anything non-trivial — opening one first helps avoid wasted effort if the change is out of scope or already being worked on. Small fixes, doc corrections, and obvious improvements can stand on their own without a linked issue, as long as the PR description explains the intent.
What Gets Closed Without Review
PRs may be closed without review...
Files:
src/services/api/withRetry.test.ts
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found one issue that still needs to be addressed.
Please do not wait for the maintainer's manual reviews to fix CodeRabbit's findings.
Findings
- [P2] Complete CodeRabbit's raw-test feature-gate request
src/services/api/withRetry.test.ts:569
CodeRabbit's current review thread is still valid:expectedCallsis derived from_PERSISTENT_MAX_ATTEMPTS_FOR_TEST, but that export is the constant100regardless of whether Bun was launched with--feature=UNATTENDED_RETRY. Under the documented focused commandbun test src/services/api/withRetry.test.ts,feature('UNATTENDED_RETRY')is false, the operation runs once, and the test still expects 101 calls, so the raw contributor path remains red even though the package scripts pass. Please complete that review request by checking the real feature gate, or otherwise explicitly assert both the feature-enabled cap behavior and the feature-disabled one-call behavior.
… test Refactor withRetry test to include isPersistentRetryEnabled check and update expected calls logic.
…ry test Refactor runRetries function for clarity.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and do not see any remaining actionable issues from my side.
@kevincodex1 LGTM
2 participants
Summary
Type safety, defensive defaults, and unbounded retry prevention, plus test-isolation follow-ups. Each commit is a single behavioral change.
Changes
src/QueryEngine.ts
EXTERNAL_PERMISSION_MODESruntime constant fromsrc/types/permissions.jsfor the SDK init validation guardpermissionModeagainstEXTERNAL_PERMISSION_MODES(not the internalPERMISSION_MODESset, which can include the classifier-only'auto'mode that is not a valid SDK wire value) — invalid mode strings fall back to'default'instead of being passed through silently. Fixes the original PR's runtimeReferenceErrorsplice(0, length, ...messages)instead oflength = 0+push()— atomic array replacement prevents concurrent readers from seeing empty state during snip replaysrc/services/api/withRetry.ts
PERSISTENT_MAX_ATTEMPTS = 100constant for the persistent-retry cap (renamed from the original PR'sPERSISTENT_RETRY_MAX_ATTEMPTSto match thePERSISTENT_*convention)_PERSISTENT_MAX_ATTEMPTS_FOR_TESTfor unit-test assertion only — there is no runtime override seam; the cap is driven by the realisPersistentRetryEnabled()gateisPersistentRetryEnabled()into a localpersistentRetryEnabledconst at the top ofwithRetry()so all call sites see a consistent snapshot within a single retry chainpersistentRetryEnabledthroughshouldRetry()and all retry branchessrc/services/compact/autoCompact.ts
MIN_AUTOCOMPACT_FAILURE_COOLDOWN_MS = 10_000constant — documents the floor and makes it testableOPENCLAUDE_AUTOCOMPACT_FAILURE_COOLDOWN_MS— prevents misconfiguration from effectively disabling the circuit breakersrc/services/compact/autoCompact.test.ts
'uses valid positive integer override'test to use15000(above floor) and rename to'uses valid positive integer override above the floor''rejects overrides below the minimum cooldown floor'test: verifies5000and9999are rejected, the floor constant equals10_000, and the exact floor value10000is acceptedbeforeEachoverride from5000to15000to respect the new floor111_000to121_000(failure at 106_000 + 15_000 cooldown)src/services/api/withRetry.test.ts
makeErrorto the newAPIError()constructor so the test errors match the real wire shape and exercise the productioncanRetry/shouldRetrybranchesCLAUDE_CODE_UNATTENDED_RETRYto theenvKeysclear list so the gate isn't poisoned by leaked state from a prior testsrc/utils/sleep.jsinimportFreshWithRetryModuleso the exponential-backoff delays don't slow the suite down'persistent retry cap'describe block that drives the real persistent retry gate — no runtime override — and proves the loop stops after exactly 101 calls (100 retries) by raisingCannotRetryErrorpackage.json
--feature=UNATTENDED_RETRYon thetest,test:full,test:coverage, andtest:providerbun test scripts so the persistent retry cap test sees the production gate behaviorsrc/services/api/client.test.ts
originalEnvsnapshot to includeOPENAI_AUTH_HEADER,OPENAI_AUTH_SCHEME,OPENAI_AUTH_HEADER_VALUE,MIMO_API_KEY,VENICE_API_KEY,NVIDIA_API_KEYclearEnvForMiniMaxOnlyTest()to clearOPENAI_API_FORMAT,OPENAI_AUTH_HEADER,OPENAI_AUTH_SCHEME,OPENAI_AUTH_HEADER_VALUE,MIMO_API_KEY,VENICE_API_KEY,NVIDIA_API_KEYbeforeEachto clearMIMO_API_KEY,VENICE_API_KEY,NVIDIA_API_KEYafterEachtorestoreEnvall the new env varsNVIDIA_API_KEY,XAI_API_KEY,MIMO_API_KEY,VENICE_API_KEY,ANTHROPIC_API_KEY,ANTHROPIC_AUTH_TOKEN,ANTHROPIC_MODELso it does not rely solely on the globalbeforeEachwhen run in isolationsrc/utils/analyzeContext.mcp.test.ts
afterEach,beforeEachto bun:test importsoriginalEnvsnapshot +beforeEach/afterEachto clearCLAUDE_CODE_DISABLE_EXPERIMENTAL_BETASandENABLE_TOOL_SEARCHso the test observes the production default of tool search enabled regardless of test ordering (mcp.ts leaks the kill switch as a top-level import side effect)Testing
autoCompact.test.tscases pass: 23/23 ✓withRetry.test.tscases pass: 33/33 ✓client.test.tscases pass: 19/19 ✓analyzeContext.mcp.test.tscases pass: 2/2 ✓src/services/api/+src/services/compact/: 634/634 ✓bun run build✓Notes
persistentAttemptis incremented on the previous iteration, so the loop runs for exactly 100 retry attempts (givingoperationa total of 101 calls — the initial attempt plus 100 retries).PERSISTENT_MAX_ATTEMPTSconstant sits next to the existingPERSISTENT_MAX_BACKOFF_MS/PERSISTENT_RESET_CAP_MSconstants and follows the same naming convention.autoCompactIfNeededcircuit breaker tests in this file (now 15_000ms = 15s)._PERSISTENT_MAX_ATTEMPTS_FOR_TESTexists solely so unit tests can assert the cap value; the cap itself is driven byisPersistentRetryEnabled()and there is no override seam (tests must enableUNATTENDED_RETRYviabun test --feature=UNATTENDED_RETRYand setCLAUDE_CODE_UNATTENDED_RETRY=1to exercise the path).Summary by CodeRabbit