fix: 401 token refresh silently skipped when refreshToken is null#325
Conversation
The 401 retry path required refreshToken != null, but config.json often has apiKey without refreshToken. When Keychain loading failed, refreshToken stayed null and 401 recovery was silently skipped with no log message. Fix: - Add ensureRefreshToken() that re-reads from Keychain/file on 401 - Log warning when no refresh token is available (was silent before) - Also picks up fresher access token from Keychain if available Closes #324 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAnthropicClient now reloads credentials from a pluggable credential supplier on 401, performs synchronized credential recovery that may update accessToken and/or refreshToken or indicate refresh availability, exposes package-private test hooks, and adds comprehensive tests for recovery scenarios and concurrency. CI config (.coderabbit.yaml) was also tightened. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Anthropic as AnthropicClient
participant Keychain as CredentialSupplier
participant Auth as TokenEndpoint
participant API as AnthropicAPI
Client->>Anthropic: sendMessage()/streamMessage(request)
Anthropic->>API: request with accessToken
API-->>Anthropic: 401 Unauthorized
Anthropic->>Anthropic: synchronized recoverCredentials()
Anthropic->>Keychain: credentialSupplier.get()
Keychain-->>Anthropic: Credential (accessToken?, refreshToken?, expiry?)
alt Keychain provides valid accessToken (not expired)
Anthropic->>API: retry with updated accessToken
API-->>Client: successful response
else refreshToken available
Anthropic->>Auth: refreshAccessToken(refreshToken)
Auth-->>Anthropic: new accessToken (+ optional refreshToken)
Anthropic->>API: retry with new accessToken
API-->>Client: successful response
else no recovery possible
Anthropic-->>Client: warn and return error (instruct restart / re-login)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 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 the current code and only fix it if needed.
Inline comments:
In `@aceclaw-llm/src/main/java/dev/aceclaw/llm/anthropic/AnthropicClient.java`:
- Around line 421-437: The ensureRefreshToken() method currently returns early
when this.refreshToken is non-null and also requires cred.refreshToken() to be
non-null to update tokens, so it never checks the keychain for a potentially
fresher access token; change ensureRefreshToken() (and its use of
KeychainCredentialReader.read(), cred.refreshToken(), cred.accessToken(), and
cred.isExpired()) to always consult the keychain if available: call
KeychainCredentialReader.read() even when this.refreshToken != null, update
this.accessToken if cred.accessToken() is non-null and !cred.isExpired(), and
only set this.refreshToken when cred.refreshToken() is present; keep the method
returning true when either a valid in-memory refreshToken exists or a
refreshToken was loaded, but ensure accessToken is refreshed from the keychain
whenever possible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 35ae219c-a71e-4fe9-8454-4be0cd75cb93
📒 Files selected for processing (1)
aceclaw-llm/src/main/java/dev/aceclaw/llm/anthropic/AnthropicClient.java
- Remove early return when refreshToken != null — Keychain may have been updated externally by Claude CLI since last check - Independently pick up access token (if not expired) and refresh token (if present) without gating one on the other - Return true only when refresh token is actually available Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
7 tests covering: - OAuth mode detection from token prefix - Null/blank access token rejection - ensureRefreshToken with existing refresh token - ensureRefreshToken with null refresh token (Keychain fallback) - ensureRefreshToken called twice (no early return, checks Keychain both times) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai review — added AnthropicClientTokenRefreshTest with 7 unit tests covering ensureRefreshToken, constructor validation, and repeated-call behavior. |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
Add pluggable credentialSupplier (package-private constructor) for testing without real Keychain. Add try-catch around credential read so Keychain failures don't crash 401 recovery. 12 tests covering all CodeRabbit requirements: - ensureRefreshToken with existing refresh token (still checks Keychain) - Keychain has fresher tokens → updates in-memory - Null refresh token + Keychain has both → loads tokens - Null refresh token + Keychain returns null → returns false - Partial: Keychain has access only (no refresh) → updates access only - Partial: Keychain has refresh only (expired access) → updates refresh only - Keychain throws exception → returns false gracefully - Concurrent calls (8 threads) → consistent state - Called twice → checks Keychain both times (no caching) - Constructor validation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai review — added 12 comprehensive tests via pluggable credentialSupplier: existing token, fresher Keychain, null refresh, partial updates, Keychain failure, concurrent calls, no-caching. Also added try-catch in ensureRefreshToken for supplier exceptions. |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
aceclaw-llm/src/test/java/dev/aceclaw/llm/anthropic/AnthropicClientTokenRefreshTest.java (2)
154-188: Test name overclaims concurrency guarantees.
ensureRefreshToken_concurrentCalls_noRaceConditioncurrently proves concurrent invocation and consistent final state, not absence of races in general. Consider renaming to reflect actual guarantees.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-llm/src/test/java/dev/aceclaw/llm/anthropic/AnthropicClientTokenRefreshTest.java` around lines 154 - 188, The test ensureRefreshToken_concurrentCalls_noRaceCondition overstates its guarantee; rename the JUnit test method to accurately reflect that it verifies concurrent invocations produce a consistent final state rather than proving absence of all race conditions—e.g., rename ensureRefreshToken_concurrentCalls_consistentFinalState or ensureRefreshToken_concurrentInvocations_consistentFinalState in AnthropicClientTokenRefreshTest and update any references to that test name (method identifier) so test runners and comments reflect the new, narrower intent for methods createClient, ensureRefreshToken, accessTokenForTest, and refreshTokenForTest.
96-103: Add an explicit warning-log assertion for the “no refresh token” path.These tests validate return values and state, but not the warning visibility introduced by this PR objective. Please add at least one assertion that the warning is emitted when refresh is unavailable after 401 recovery.
Also applies to: 140-149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-llm/src/test/java/dev/aceclaw/llm/anthropic/AnthropicClientTokenRefreshTest.java` around lines 96 - 103, Add an assertion that a warning-level log is emitted when no refresh token is available: in the test ensureRefreshToken_nullRefreshToken_keychainReturnsNull_returnsFalse (and the sibling test at lines 140-149), capture the client/logger output created by createClient and assert that calling ensureRefreshToken() both returns false and produced a warning log entry indicating refresh was unavailable; use the existing test logger/test appender or a mock to verify a warn() call (or that a warning message containing “no refresh token” is logged) and keep the existing assertions on refreshTokenForTest().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@aceclaw-llm/src/test/java/dev/aceclaw/llm/anthropic/AnthropicClientTokenRefreshTest.java`:
- Around line 30-33: The test constructor_oauthToken_detectsOAuthMode currently
only asserts provider() which doesn't verify OAuth header selection; update the
test (constructor_oauthToken_detectsOAuthMode) to assert mode-specific behavior
from the created client by using createClient(OAUTH_TOKEN, REFRESH_TOKEN, () ->
null) and then either assert a dedicated auth-mode indicator (e.g.,
client.authMode() == AuthMode.OAUTH) or assert that the client will send an
Authorization header (not x-api-key) for OAuth tokens (for example via a method
that returns headers or a prepareRequest/authorizationHeader helper); adjust
assertions to validate Authorization header usage for tokens starting with
sk-ant-oat01-* or rename the test if you only want to keep the provider() check.
---
Nitpick comments:
In
`@aceclaw-llm/src/test/java/dev/aceclaw/llm/anthropic/AnthropicClientTokenRefreshTest.java`:
- Around line 154-188: The test
ensureRefreshToken_concurrentCalls_noRaceCondition overstates its guarantee;
rename the JUnit test method to accurately reflect that it verifies concurrent
invocations produce a consistent final state rather than proving absence of all
race conditions—e.g., rename
ensureRefreshToken_concurrentCalls_consistentFinalState or
ensureRefreshToken_concurrentInvocations_consistentFinalState in
AnthropicClientTokenRefreshTest and update any references to that test name
(method identifier) so test runners and comments reflect the new, narrower
intent for methods createClient, ensureRefreshToken, accessTokenForTest, and
refreshTokenForTest.
- Around line 96-103: Add an assertion that a warning-level log is emitted when
no refresh token is available: in the test
ensureRefreshToken_nullRefreshToken_keychainReturnsNull_returnsFalse (and the
sibling test at lines 140-149), capture the client/logger output created by
createClient and assert that calling ensureRefreshToken() both returns false and
produced a warning log entry indicating refresh was unavailable; use the
existing test logger/test appender or a mock to verify a warn() call (or that a
warning message containing “no refresh token” is logged) and keep the existing
assertions on refreshTokenForTest().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f62933b-e2e9-4b48-b342-0630f7252e2b
📒 Files selected for processing (2)
aceclaw-llm/src/main/java/dev/aceclaw/llm/anthropic/AnthropicClient.javaaceclaw-llm/src/test/java/dev/aceclaw/llm/anthropic/AnthropicClientTokenRefreshTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- aceclaw-llm/src/main/java/dev/aceclaw/llm/anthropic/AnthropicClient.java
| void constructor_oauthToken_detectsOAuthMode() { | ||
| var client = createClient(OAUTH_TOKEN, REFRESH_TOKEN, () -> null); | ||
| assertThat(client.provider()).isEqualTo("anthropic"); | ||
| } |
There was a problem hiding this comment.
Test intent mismatch: this does not actually verify OAuth mode detection.
At Line 30, the test name says OAuth mode is detected, but the assertion only checks provider(). That can pass even if auth header mode selection regresses. Please assert mode-specific behavior (or rename the test to match what it really checks).
Based on learnings "Support two authentication modes in AnthropicClient: API key (sk-ant-api03-) via x-api-key header and OAuth token (sk-ant-oat01-) via Authorization header with beta flags".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@aceclaw-llm/src/test/java/dev/aceclaw/llm/anthropic/AnthropicClientTokenRefreshTest.java`
around lines 30 - 33, The test constructor_oauthToken_detectsOAuthMode currently
only asserts provider() which doesn't verify OAuth header selection; update the
test (constructor_oauthToken_detectsOAuthMode) to assert mode-specific behavior
from the created client by using createClient(OAUTH_TOKEN, REFRESH_TOKEN, () ->
null) and then either assert a dedicated auth-mode indicator (e.g.,
client.authMode() == AuthMode.OAUTH) or assert that the client will send an
Authorization header (not x-api-key) for OAuth tokens (for example via a method
that returns headers or a prepareRequest/authorizationHeader helper); adjust
assertions to validate Authorization header usage for tokens starting with
sk-ant-oat01-* or rename the test if you only want to keep the provider() check.
…fresh token P1: Replace boolean ensureRefreshToken() with CredentialRecovery enum: - ACCESS_TOKEN_UPDATED: Keychain had fresher access token → retry immediately - REFRESH_AVAILABLE: has refresh token → do full OAuth refresh - NO_RECOVERY: nothing available → log warning Previously, getting a fresh access token from Keychain but no refresh token meant the request still failed because the caller only checked the boolean return. Both sendMessage and streamMessage 401 paths now use switch on the enum to handle all three cases. 14 tests covering all recovery paths, partial updates, failures, concurrent access, and no-caching behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai review — replaced boolean ensureRefreshToken with CredentialRecovery enum (ACCESS_TOKEN_UPDATED/REFRESH_AVAILABLE/NO_RECOVERY). 401 paths now retry with fresh access token even without refresh token. 14 tests covering all recovery paths. |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
…and-swap CodeRabbit finding: read-compare-write of accessToken was non-atomic even though fields are volatile. Concurrent 401 responses could see stale previousAccessToken. Synchronized the method. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@coderabbitai review — synchronized recoverCredentials() to fix race condition on token compare-and-swap. The stale test comment refers to an old revision that no longer exists. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
aceclaw-llm/src/main/java/dev/aceclaw/llm/anthropic/AnthropicClient.java (1)
178-201: Consider extracting duplicated 401 recovery switch logic.The switch statement handling
CredentialRecoveryoutcomes is nearly identical insendMessageandstreamMessage. Extracting a shared helper would reduce duplication and ensure consistent behavior if the recovery logic changes.For example, a private method returning a boolean indicating whether retry should proceed:
private boolean handleRecoveryOutcome(CredentialRecovery recovery) { return switch (recovery) { case ACCESS_TOKEN_UPDATED -> { log.info("Access token updated from credential store, retrying..."); yield true; } case REFRESH_AVAILABLE -> { log.info("OAuth token expired, attempting refresh..."); yield refreshAccessToken(); } case NO_RECOVERY -> { log.warn("OAuth token expired (401) but no recovery available; " + "restart daemon or re-run /login to refresh credentials"); yield false; } }; }Also applies to: 242-262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-llm/src/main/java/dev/aceclaw/llm/anthropic/AnthropicClient.java` around lines 178 - 201, The 401 recovery switch used in sendMessage and streamMessage is duplicated—extract a private helper (e.g., handleRecoveryOutcome(CredentialRecovery)) that encapsulates the switch, performs the same logging, calls refreshAccessToken() when needed, and returns a boolean indicating whether to retry; then in both sendMessage and streamMessage call recoverCredentials(), pass the result to handleRecoveryOutcome, and if it returns true rebuild the request with buildRequest(...) and resend via httpClient.send(...) to avoid duplicated switch logic and keep behavior consistent.aceclaw-llm/src/test/java/dev/aceclaw/llm/anthropic/AnthropicClientTokenRefreshTest.java (1)
158-191: Consider wrapping executor in try-finally to prevent thread leaks on test failure.If an assertion fails before
executor.shutdown()is called, the thread pool won't be cleaned up properly. While this is minor for test code, it's good hygiene.♻️ Suggested improvement
int threads = 8; var latch = new CountDownLatch(1); ExecutorService executor = Executors.newFixedThreadPool(threads); - - for (int i = 0; i < threads; i++) { - executor.submit(() -> { - try { - latch.await(); - client.recoverCredentials(); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } - }); + try { + for (int i = 0; i < threads; i++) { + executor.submit(() -> { + try { + latch.await(); + client.recoverCredentials(); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + } + }); + } + latch.countDown(); + executor.shutdown(); + assertThat(executor.awaitTermination(10, TimeUnit.SECONDS)).isTrue(); + } finally { + executor.shutdownNow(); } - latch.countDown(); - executor.shutdown(); - assertThat(executor.awaitTermination(10, TimeUnit.SECONDS)).isTrue();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@aceclaw-llm/src/test/java/dev/aceclaw/llm/anthropic/AnthropicClientTokenRefreshTest.java` around lines 158 - 191, The test recoverCredentials_concurrentCalls_consistentState should ensure the ExecutorService is always cleaned up: wrap the executor usage in a try-finally around the loop and latch such that executor.shutdown() and a bounded awaitTermination (and if needed executor.shutdownNow() when await times out) are invoked in the finally block; this guarantees no thread-pool leak if an assertion throws before shutdown. Use the existing executor variable and latch in that try block and perform cleanup in finally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@aceclaw-llm/src/main/java/dev/aceclaw/llm/anthropic/AnthropicClient.java`:
- Around line 473-478: recoverCredentials currently assigns this.accessToken
when cred.accessToken() is non-null but does not check for blank, which can
violate the AnthropicClient constructor invariant that accessToken must be
non-null and non-blank; update the recoverCredentials logic (the branch that
checks cred.accessToken()) to also verify the token is not blank (e.g.,
!cred.accessToken().isBlank()) before setting this.accessToken, and add a
debug/warn log if a blank token is returned so the assignment is skipped and the
invariant is preserved.
---
Nitpick comments:
In `@aceclaw-llm/src/main/java/dev/aceclaw/llm/anthropic/AnthropicClient.java`:
- Around line 178-201: The 401 recovery switch used in sendMessage and
streamMessage is duplicated—extract a private helper (e.g.,
handleRecoveryOutcome(CredentialRecovery)) that encapsulates the switch,
performs the same logging, calls refreshAccessToken() when needed, and returns a
boolean indicating whether to retry; then in both sendMessage and streamMessage
call recoverCredentials(), pass the result to handleRecoveryOutcome, and if it
returns true rebuild the request with buildRequest(...) and resend via
httpClient.send(...) to avoid duplicated switch logic and keep behavior
consistent.
In
`@aceclaw-llm/src/test/java/dev/aceclaw/llm/anthropic/AnthropicClientTokenRefreshTest.java`:
- Around line 158-191: The test
recoverCredentials_concurrentCalls_consistentState should ensure the
ExecutorService is always cleaned up: wrap the executor usage in a try-finally
around the loop and latch such that executor.shutdown() and a bounded
awaitTermination (and if needed executor.shutdownNow() when await times out) are
invoked in the finally block; this guarantees no thread-pool leak if an
assertion throws before shutdown. Use the existing executor variable and latch
in that try block and perform cleanup in finally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65d1c3dd-8b07-4aa3-a60d-882c2aa00fdb
📒 Files selected for processing (2)
aceclaw-llm/src/main/java/dev/aceclaw/llm/anthropic/AnthropicClient.javaaceclaw-llm/src/test/java/dev/aceclaw/llm/anthropic/AnthropicClientTokenRefreshTest.java
| if (cred != null) { | ||
| // Pick up fresher access token independently of refresh token availability | ||
| if (cred.accessToken() != null && !cred.isExpired()) { | ||
| this.accessToken = cred.accessToken(); | ||
| log.debug("Updated access token from credential store"); | ||
| } |
There was a problem hiding this comment.
Add blank check for cred.accessToken() to preserve constructor invariant.
The constructor (line 130-131) validates that accessToken is neither null nor blank. However, recoverCredentials only checks for null before updating this.accessToken. If the credential supplier returns a blank access token, this would violate the class invariant.
🔧 Suggested fix
if (cred != null) {
// Pick up fresher access token independently of refresh token availability
- if (cred.accessToken() != null && !cred.isExpired()) {
+ if (cred.accessToken() != null && !cred.accessToken().isBlank() && !cred.isExpired()) {
this.accessToken = cred.accessToken();
log.debug("Updated access token from credential store");
}As per coding guidelines: constructor validates "API key / access token must not be null or blank", so recovery should maintain the same invariant.
📝 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.
| if (cred != null) { | |
| // Pick up fresher access token independently of refresh token availability | |
| if (cred.accessToken() != null && !cred.isExpired()) { | |
| this.accessToken = cred.accessToken(); | |
| log.debug("Updated access token from credential store"); | |
| } | |
| if (cred != null) { | |
| // Pick up fresher access token independently of refresh token availability | |
| if (cred.accessToken() != null && !cred.accessToken().isBlank() && !cred.isExpired()) { | |
| this.accessToken = cred.accessToken(); | |
| log.debug("Updated access token from credential store"); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@aceclaw-llm/src/main/java/dev/aceclaw/llm/anthropic/AnthropicClient.java`
around lines 473 - 478, recoverCredentials currently assigns this.accessToken
when cred.accessToken() is non-null but does not check for blank, which can
violate the AnthropicClient constructor invariant that accessToken must be
non-null and non-blank; update the recoverCredentials logic (the branch that
checks cred.accessToken()) to also verify the token is not blank (e.g.,
!cred.accessToken().isBlank()) before setting this.accessToken, and add a
debug/warn log if a blank token is returned so the assignment is skipped and the
invariant is preserved.
- Add auto-labeling (security, breaking-change, needs-tests) - Add path filters to skip non-essential files (docs, images, scripts) - Add pre-merge checks: test coverage for new logic, API breaking changes - Enable commit_status, fail_commit_status, PMD, docstrings, unit tests - Restructure path_instructions for readability Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.coderabbit.yaml:
- Around line 109-119: The `tools` and `finishing_touches` blocks are
incorrectly placed at the root and should be nested under `reviews`; move the
entire `tools:` (including `pmd:` and its `enabled`) and `finishing_touches:`
(including `docstrings:` and `unit_tests:`) sections so they are children of the
`reviews:` mapping (you can place them before or after `path_instructions`),
preserving proper YAML indentation and structure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e7ce375-a35c-4dcc-be48-62e64edd8665
📒 Files selected for processing (1)
.coderabbit.yaml
| # --- Tools --- | ||
| tools: | ||
| pmd: | ||
| enabled: true | ||
|
|
||
| # --- Finishing Touches --- | ||
| finishing_touches: | ||
| docstrings: | ||
| enabled: true | ||
| unit_tests: | ||
| enabled: true |
There was a problem hiding this comment.
tools and finishing_touches are at incorrect nesting level and will be ignored.
Per the schema, both tools and finishing_touches must be nested under reviews, not at the root level. The current placement will cause these configurations to have no effect since the schema uses additionalProperties: true at root (allows but ignores unknown keys).
🔧 Proposed fix to correct the nesting
Move these sections under reviews: (before or after path_instructions):
path_instructions:
# ... existing path_instructions ...
+ # --- Tools ---
+ tools:
+ pmd:
+ enabled: true
+
+ # --- Finishing Touches ---
+ finishing_touches:
+ docstrings:
+ enabled: true
+ unit_tests:
+ enabled: true
# --- Chat Settings ---
chat:
auto_reply: true
-# --- Tools ---
-tools:
- pmd:
- enabled: true
-
-# --- Finishing Touches ---
-finishing_touches:
- docstrings:
- enabled: true
- unit_tests:
- enabled: true
-
# --- Knowledge Base ---
knowledge_base:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.coderabbit.yaml around lines 109 - 119, The `tools` and `finishing_touches`
blocks are incorrectly placed at the root and should be nested under `reviews`;
move the entire `tools:` (including `pmd:` and its `enabled`) and
`finishing_touches:` (including `docstrings:` and `unit_tests:`) sections so
they are children of the `reviews:` mapping (you can place them before or after
`path_instructions`), preserving proper YAML indentation and structure.
…g_touches) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 participant
Summary
401 retry was silently skipped because
refreshTokenwas null (config.json has apiKey but no refreshToken). Now:ensureRefreshToken()re-reads from Keychain/credential file on 401Closes #324
Test plan
./gradlew :aceclaw-llm:testpasses🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores