fix(reborn): proactively refresh Google OAuth tokens before expiry (#5071) - #5087
Merged
henrypark133 merged 19 commits intoJun 21, 2026
Merged
Conversation
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…5071 WS1) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… classification (#5071 WS2) - Add DEFAULT_ACCESS_REFRESH_MARGIN (5 min) to skip refresh when token still fresh - Wire secret_store into ProductAuthRuntimeCredentialAccountRefresher via optional builder on RebornProductAuthServices - Parse OAuth error body in execute_token_request; classify invalid_grant as AuthProductError::InvalidGrant - Handle InvalidGrant in credential.rs by marking account Revoked (permanent, not transient) - Add InvalidGrant to non-exhaustive match in auth_interaction service (maps to StaleAuth) - Fix pre-existing 3-arg put() calls in tests (expires_at=None); fix crash-safe write-order test Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…resh (#5071 WS3) Blocking pg_advisory_lock serializes concurrent refreshers (second waits, then refreshes with the rotated token) rather than racing the token endpoint. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WS4) Implements WS4 of the proactive Google OAuth token refresh feature: - B1: cross-owner candidate enumeration on FilesystemAuthProductServices<F> via new `list_refresh_candidates` method; adds `root: Option<Arc<F>>` and `new_with_root` constructor to enable scanning outside the scoped mount. - B2/B3: new `credential_refresh_worker` module (mirrors trigger_poller.rs) with `CredentialRefreshCandidateSource` trait, `CredentialRefreshWorkerDeps`, `spawn_credential_refresh_worker`, and the tick loop (startup jitter, idle-threshold filter, per-tick cap, advisory-locked refresh). - B4: `CredentialRefreshSettings` in runtime_input.rs; added to `RebornRuntimeInput` with builder method. - Spawn wiring: `build_reborn_runtime` takes the candidate source from `RebornServices` and spawns the worker, storing the handle in `RebornRuntime`; `shutdown()` drains with a 5-second timeout. - All new code gated behind `#[cfg(any(feature = "libsql", feature = "postgres"))]` so no-db builds (reborn_cli default) compile warning-free. - Logging: `debug!` only — never `info!`/`warn!` (REPL/TUI invariant). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…w fixes + tests (#5071) - Replace per-account advisory lock with a deployment-wide leader-election lock used by the worker only (pg_try_advisory_lock, one fixed key, 1 conn); inline path reverts to the in-process refresh_locks guard. - Enable the worker for the Serve caller via the CLI with an IRONCLAW_CREDENTIAL_REFRESH_ENABLED kill-switch (struct default stays off). - Review fixes: worker debug!-only logging, accurate advisory-lock comment, cleanup log message, startup jitter default, cancellation between refreshes, silent-ok annotation, settings doc/Default consistency. - Simplifications: secret_store non-optional; inline refresh margin is config. - Tests: worker tick/candidate-filter/transient-error, spawn guard, A2 margin (fresh-skip/within-margin/absent-expiry), filesystem expires_at roundtrip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🚅 Deployed to the ironclaw-pr-5087 environment in ironclaw-ci-preview
|
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5087
June 19, 2026 06:33
Destroyed
github-actions
Bot
added
scope: docs
|
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 (4)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughAdds proactive Google OAuth token refresh: ChangesProactive Google OAuth Refresh
Sequence DiagramsequenceDiagram
rect rgba(100, 149, 237, 0.5)
Note over CredentialRefreshWorker: Background tick (leader only)
end
CredentialRefreshWorker->>CredentialRefreshLeaderLock: run_as_leader(sweep)
CredentialRefreshLeaderLock->>PostgreSQL: pg_try_advisory_xact_lock
PostgreSQL-->>CredentialRefreshLeaderLock: acquired
CredentialRefreshLeaderLock->>FilesystemAuthProductServices: list_refresh_candidates()
FilesystemAuthProductServices-->>CredentialRefreshLeaderLock: Vec~CredentialAccount~
CredentialRefreshLeaderLock->>CredentialRefreshLeaderLock: select_idle_candidates(cutoff, max_per_tick)
CredentialRefreshLeaderLock->>RebornProductAuthServices: refresh_credential_account(request)
RebornProductAuthServices->>OAuthProviderClient: POST /token (refresh_token grant)
OAuthProviderClient->>SecretStore: put(refresh_handle, material, None)
OAuthProviderClient->>SecretStore: put(access_handle, material, expires_at)
RebornProductAuthServices-->>CredentialRefreshLeaderLock: refreshed=true
CredentialRefreshLeaderLock->>PostgreSQL: COMMIT (release advisory lock)
sequenceDiagram
rect rgba(100, 200, 100, 0.5)
Note over ProductAuthRuntimeCredentialAccountRefresher: Inline staging path
end
ProductAuthRuntimeCredentialAccountRefresher->>SecretStore: metadata(access_handle)
SecretStore-->>ProductAuthRuntimeCredentialAccountRefresher: expires_at
alt expires_at > now + 5min
ProductAuthRuntimeCredentialAccountRefresher-->>Caller: existing account (skip)
else within margin or missing
ProductAuthRuntimeCredentialAccountRefresher->>RebornProductAuthServices: refresh_credential_account(request)
RebornProductAuthServices-->>ProductAuthRuntimeCredentialAccountRefresher: refreshed account
ProductAuthRuntimeCredentialAccountRefresher-->>Caller: refreshed account
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces proactive Google OAuth token refresh and keepalive mechanisms, including a background keepalive worker, a Postgres advisory-lock leader election utility, and margin-aware conditional inline refreshes based on persisted access-token expiry. The code review identified several potential panic vectors due to integer overflow or underflow: specifically, clamping token TTLs to i64::MAX can still overflow chrono::Duration calculations, subtracting large margins from expiration timestamps can underflow, and sliding-window cutoff calculations need safe handling for extremely large windows to prevent incorrect trimming.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 745c7bca15
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/ironclaw_secrets/src/filesystem_store.rs (1)
1705-1723: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd a non-
Noneexpiry regression for the new filesystem contract.All visible
putcalls still passNone, so the new persistedexpires_atpath can regress without a failing test. Cover both metadata round-trip and expired-secret lease denial.Proposed test coverage
async fn filesystem_secret_store_round_trips_material() { let fs = Arc::new(InMemoryBackend::new()); let scoped = default_scoped_fs(Arc::clone(&fs)); let store = FilesystemSecretStore::new(scoped, test_crypto()); @@ let second = store.consume(&scope, lease.id).await.unwrap_err(); assert!(second.is_consumed()); } + + #[tokio::test] + async fn filesystem_secret_store_round_trips_expiry_metadata() { + let fs = Arc::new(InMemoryBackend::new()); + let scoped = default_scoped_fs(Arc::clone(&fs)); + let store = FilesystemSecretStore::new(scoped, test_crypto()); + let scope = sample_scope("tenant-a", "user-a"); + let handle = SecretHandle::new("oauth_access").unwrap(); + let expires_at = Utc::now() + chrono::Duration::minutes(30); + + let metadata = store + .put( + scope.clone(), + handle.clone(), + SecretMaterial::from("access-token"), + Some(expires_at), + ) + .await + .unwrap(); + + assert_eq!(metadata.expires_at, Some(expires_at)); + assert_eq!( + store + .metadata(&scope, &handle) + .await + .unwrap() + .expect("metadata") + .expires_at, + Some(expires_at) + ); + } + + #[tokio::test] + async fn filesystem_secret_store_rejects_expired_secret_lease() { + let fs = Arc::new(InMemoryBackend::new()); + let scoped = default_scoped_fs(Arc::clone(&fs)); + let store = FilesystemSecretStore::new(scoped, test_crypto()); + let scope = sample_scope("tenant-a", "user-a"); + let handle = SecretHandle::new("expired_access").unwrap(); + + store + .put( + scope.clone(), + handle.clone(), + SecretMaterial::from("expired-token"), + Some(Utc::now() - chrono::Duration::seconds(1)), + ) + .await + .unwrap(); + + assert!(store.lease_once(&scope, &handle).await.unwrap_err().is_expired()); + }🤖 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 `@crates/ironclaw_secrets/src/filesystem_store.rs` around lines 1705 - 1723, The test filesystem_secret_store_round_trips_material currently only tests the put operation with None for the expiry parameter, leaving the expires_at persistence code without test coverage. Add an additional test case that covers non-None expiry scenarios by passing a valid expiration timestamp to the put call instead of None, then verify that the metadata correctly reflects the expires_at value in the round-trip, and additionally test that attempting to lease an expired secret properly denies the operation.crates/ironclaw_host_runtime/src/egress/credential.rs (1)
590-594:⚠️ Potential issue | 🔴 CriticalAdd missing
expires_atargument toSecretStore::putcallLine 590–594 call to
store.put()passes three arguments but the trait requires four; missingexpires_at: Option<Timestamp>. This breaks compilation and fails the pre-PRcargo buildcheck.Fix
block_on_test(store.put( scope.clone(), handle.clone(), SecretMaterial::from("sk-test-secret"), + None, ))🤖 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 `@crates/ironclaw_host_runtime/src/egress/credential.rs` around lines 590 - 594, The block_on_test call to store.put() in the credential module is missing a required argument. The SecretStore::put trait method requires four arguments: scope, handle, secret material, and expires_at, but the current call only provides three. Add the missing expires_at parameter as an Option<Timestamp> argument to the store.put() call to match the trait signature.Source: Coding guidelines
crates/ironclaw_reborn_composition/src/runtime.rs (1)
1673-1689:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the shutdown contract for the new worker.
Line 1673 still says shutdown stops only the turn-runner and budget projection, but this method now also owns trigger-poller, credential-refresh, and trace-flush shutdown.
Suggested doc update
- /// Stop the turn-runner worker and the budget-event projection. - /// Awaits both tasks before returning so background state is fully - /// drained when the runtime drops. + /// Stop runtime-owned background workers and projections. + /// Awaits/cancels the trigger poller, credential refresh worker, trace + /// flush worker, turn-runner worker, and budget-event projection before + /// returning so background state is fully drained when the runtime drops.As per coding guidelines, behavior changes must re-read and update adjacent docstrings because comments are part of the contract.
🤖 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 `@crates/ironclaw_reborn_composition/src/runtime.rs` around lines 1673 - 1689, The docstring for the shutdown method at the top of the function is outdated and does not accurately describe all the workers being shut down. Update the docstring to reflect that the shutdown method now also handles stopping the trigger-poller worker, the credential-refresh worker (when the corresponding feature flags are enabled), and the trace-flush worker, in addition to the turn-runner and budget-event projection mentioned in the current comment. Ensure the updated docstring clearly documents all shutdown responsibilities of the shutdown method to maintain the accuracy of the contract.Source: Coding guidelines
🤖 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 `@crates/ironclaw_reborn_composition/src/credential_refresh_worker.rs`:
- Around line 353-360: The test spawn_returns_none_when_disabled verifies the
default settings flag but does not actually invoke the function under test. Add
a call to spawn_credential_refresh_worker passing the disabled settings, and
assert that the return value is None to verify the early-return guard is working
as expected.
In `@crates/ironclaw_reborn_composition/src/factory.rs`:
- Around line 3381-3384: The comment in the credential_refresh_leader_lock field
description states that the value should be None when candidate_source is None,
but the current assignment unconditionally sets it to Some(leader_lock). Fix
this by making the assignment conditional: check whether candidate_source is
None, and if so, assign None to credential_refresh_leader_lock, otherwise assign
Some(leader_lock). This will align the code implementation with the documented
behavior described in the comment.
- Around line 1013-1015: The runtime_credential_account_refresh_service calls
are hardcoding DEFAULT_ACCESS_REFRESH_MARGIN instead of using the configurable
value from CredentialRefreshSettings, which causes runtime-level margin
configuration to be ignored. Replace the hardcoded DEFAULT_ACCESS_REFRESH_MARGIN
constant with the actual access_refresh_margin value from the
CredentialRefreshSettings configuration object in both locations where
runtime_credential_account_refresh_service is invoked (around lines 1013-1015
and 3347-3349). This ensures the configured access_refresh_margin setting is
properly wired into the refresher construction for both local-dev and production
environments.
In `@crates/ironclaw_reborn_composition/src/lib.rs`:
- Around line 36-38: The module declaration for credential_refresh_worker has a
duplicate #[cfg] attribute on consecutive lines. Remove one of the identical
#[cfg(any(feature = "libsql", feature = "postgres"))] attributes, keeping only a
single instance before the mod credential_refresh_worker; declaration.
In `@crates/ironclaw_reborn_composition/src/oauth_provider_client.rs`:
- Around line 369-384: The rollback logic around line 402 unconditionally
deletes the refresh_secret after an access-write failure, which is unsafe for
account-scoped refresh flows where the account may already reference that
refresh token. Add a parameter (such as rollback_refresh_on_access_failure) to
control whether to delete refresh_secret on failure, pass true for
callback/exchange paths where rollback is safe, and pass false for account
refresh paths to preserve the active refresh token and maintain consistency with
the account's stored state.
- Around line 354-357: The code in the `access_expires_at` calculation uses
unsafe DateTime arithmetic that can panic when the result overflows the valid
DateTime range, even though the `expires_in_seconds` value is clamped to
`i64::MAX`. Replace the `map` call with `and_then` to properly handle the result
of checked arithmetic, and change the unsafe addition operator `+` to use the
`checked_add_signed()` method when adding the chrono Duration to `Utc::now()`.
This follows the same safe DateTime arithmetic pattern used consistently
throughout the codebase in files like ironclaw_turns/memory.rs and
ironclaw_reborn_webui_ingress/session.rs.
In `@crates/ironclaw_reborn_composition/src/oauth_provider_client/tests.rs`:
- Around line 714-717: In the HostOAuthProviderClient::new constructor call
around line 714, the egress parameter is being double-wrapped with Arc::new()
when it is already an Arc<RecordingEgress>. Remove the Arc::new() wrapper and
pass egress directly as the second argument to HostOAuthProviderClient::new so
that it receives Arc<RecordingEgress> instead of Arc<Arc<RecordingEgress>>.
In `@crates/ironclaw_reborn_composition/src/product_auth_durable.rs`:
- Around line 707-721: The async closure in the product_auth_durable.rs file
contains multiple uses of the `?` operator without the required inline `//
silent-ok: <reason>` annotations. Per coding guidelines, each intentional
error/None suppression must be justified with an inline comment. Add `//
silent-ok:` comments before each `?` operator in the closure (at
VirtualPath::new, root.get, serde_json::from_slice, and
account.refresh_secret.as_ref calls) to document why the failure is acceptable,
such as filtering out invalid entries or missing optional data.
In `@crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rs`:
- Around line 20-24: The resolver construction in the factory context is
hardcoding DEFAULT_ACCESS_REFRESH_MARGIN for both local and production paths
instead of threading the configured refresh margin value. Locate where the
resolver is being instantiated (in the factory context for both environments)
and replace the hardcoded DEFAULT_ACCESS_REFRESH_MARGIN constant with the actual
configured refresh margin value retrieved from CredentialRefreshSettings or the
equivalent configuration object. This ensures that the configured margin is
consistently applied across all code paths rather than always using the default,
preventing inconsistency between the worker and inline fallback.
In `@crates/ironclaw_reborn_composition/src/runtime_input.rs`:
- Around line 229-233: The top-level docstring for the worker mentions that
inline access-token expiry margin logic uses a fixed
DEFAULT_ACCESS_REFRESH_MARGIN value and does not participate in inline control,
but this contradicts the new access_refresh_margin field (lines 266-271) that
now allows operators to tune this behavior. Update the docstring (lines 229-233)
to remove the claim about fixed DEFAULT_ACCESS_REFRESH_MARGIN and instead
document that operators can control the inline margin logic by tuning the
access_refresh_margin field on the type.
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 2959-2975: The match statement handling credential refresh worker
spawning silently returns None when dependencies are missing, but in production
when credential_refresh.enabled is true, this violates the activation contract.
Refactor the logic to add a guard condition that checks if
credential_refresh.enabled is true before entering the match block. If enabled
and any dependency (candidate_source, leader_lock, or product_auth) is absent,
panic or fail the startup. Only allow the silent None return when
credential_refresh is not enabled. This ensures the worker actually runs in
production when explicitly enabled.
---
Outside diff comments:
In `@crates/ironclaw_host_runtime/src/egress/credential.rs`:
- Around line 590-594: The block_on_test call to store.put() in the credential
module is missing a required argument. The SecretStore::put trait method
requires four arguments: scope, handle, secret material, and expires_at, but the
current call only provides three. Add the missing expires_at parameter as an
Option<Timestamp> argument to the store.put() call to match the trait signature.
In `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 1673-1689: The docstring for the shutdown method at the top of the
function is outdated and does not accurately describe all the workers being shut
down. Update the docstring to reflect that the shutdown method now also handles
stopping the trigger-poller worker, the credential-refresh worker (when the
corresponding feature flags are enabled), and the trace-flush worker, in
addition to the turn-runner and budget-event projection mentioned in the current
comment. Ensure the updated docstring clearly documents all shutdown
responsibilities of the shutdown method to maintain the accuracy of the
contract.
In `@crates/ironclaw_secrets/src/filesystem_store.rs`:
- Around line 1705-1723: The test filesystem_secret_store_round_trips_material
currently only tests the put operation with None for the expiry parameter,
leaving the expires_at persistence code without test coverage. Add an additional
test case that covers non-None expiry scenarios by passing a valid expiration
timestamp to the put call instead of None, then verify that the metadata
correctly reflects the expires_at value in the round-trip, and additionally test
that attempting to lease an expired secret properly denies the operation.
🪄 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: d0236b40-aa3c-48fc-b65c-178372c80ad1
📒 Files selected for processing (32)
crates/ironclaw_auth/src/credential.rscrates/ironclaw_auth/src/error.rscrates/ironclaw_host_runtime/src/egress/credential.rscrates/ironclaw_host_runtime/src/obligations.rscrates/ironclaw_host_runtime/tests/host_runtime_services_contract.rscrates/ironclaw_host_runtime/tests/runtime_http_egress_contract.rscrates/ironclaw_product_workflow/src/auth_interaction/service.rscrates/ironclaw_reborn_cli/src/runtime/mod.rscrates/ironclaw_reborn_composition/src/auth.rscrates/ironclaw_reborn_composition/src/credential_refresh_worker.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/llm_config_service.rscrates/ironclaw_reborn_composition/src/llm_key_store.rscrates/ironclaw_reborn_composition/src/oauth_dcr.rscrates/ironclaw_reborn_composition/src/oauth_gate.rscrates/ironclaw_reborn_composition/src/oauth_provider_client.rscrates/ironclaw_reborn_composition/src/oauth_provider_client/tests.rscrates/ironclaw_reborn_composition/src/product_auth_durable.rscrates/ironclaw_reborn_composition/src/product_auth_durable/interactions.rscrates/ironclaw_reborn_composition/src/product_auth_durable/tests.rscrates/ironclaw_reborn_composition/src/product_auth_providers.rscrates/ironclaw_reborn_composition/src/product_auth_refresh_lock.rscrates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rscrates/ironclaw_reborn_composition/src/product_auth_runtime_credentials/tests.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime_input.rscrates/ironclaw_secrets/src/filesystem_store.rscrates/ironclaw_secrets/src/lib.rscrates/ironclaw_secrets/tests/secret_store_contract.rsdocs/plans/2026-06-18-reborn-proactive-google-oauth-refresh.mdtests/support/reborn/harness.rs
Comment on lines
+2959
to
+2975
| let credential_refresh_worker_handle = match ( | ||
| services.credential_refresh_candidate_source.take(), | ||
| services.credential_refresh_leader_lock.take(), | ||
| services.product_auth.clone(), | ||
| ) { | ||
| (Some(candidate_source), Some(leader_lock), Some(refresh_port)) => { | ||
| crate::credential_refresh_worker::spawn_credential_refresh_worker( | ||
| credential_refresh, | ||
| crate::credential_refresh_worker::CredentialRefreshWorkerDeps { | ||
| candidate_source, | ||
| refresh_port, | ||
| leader_lock: std::sync::Arc::new(leader_lock), | ||
| }, | ||
| ) | ||
| } | ||
| _ => None, | ||
| }; |
There was a problem hiding this comment.
Fail production startup when enabled refresh cannot spawn.
Line 2974 silently disables the worker if any dependency is absent. With credential_refresh.enabled in production, that boots successfully but no keepalive runs, violating the production activation contract.
Suggested guard
#[cfg(any(feature = "libsql", feature = "postgres"))]
- let credential_refresh_worker_handle = match (
- services.credential_refresh_candidate_source.take(),
- services.credential_refresh_leader_lock.take(),
- services.product_auth.clone(),
- ) {
- (Some(candidate_source), Some(leader_lock), Some(refresh_port)) => {
- crate::credential_refresh_worker::spawn_credential_refresh_worker(
- credential_refresh,
- crate::credential_refresh_worker::CredentialRefreshWorkerDeps {
- candidate_source,
- refresh_port,
- leader_lock: std::sync::Arc::new(leader_lock),
- },
- )
- }
- _ => None,
+ let credential_refresh_worker_handle = {
+ let credential_refresh_enabled = credential_refresh.enabled;
+ let handle = match (
+ services.credential_refresh_candidate_source.take(),
+ services.credential_refresh_leader_lock.take(),
+ services.product_auth.clone(),
+ ) {
+ (Some(candidate_source), Some(leader_lock), Some(refresh_port)) => {
+ crate::credential_refresh_worker::spawn_credential_refresh_worker(
+ credential_refresh,
+ crate::credential_refresh_worker::CredentialRefreshWorkerDeps {
+ candidate_source,
+ refresh_port,
+ leader_lock: std::sync::Arc::new(leader_lock),
+ },
+ )
+ }
+ _ => None,
+ };
+ if credential_refresh_enabled
+ && profile == RebornCompositionProfile::Production
+ && handle.is_none()
+ {
+ return Err(RebornRuntimeError::InvalidArgument {
+ reason: "credential refresh is enabled for production but worker dependencies are unavailable"
+ .to_string(),
+ });
+ }
+ handle
};As per coding guidelines, Reborn production composition must fail closed on missing required handles, and the PR objective requires the worker to run in production when enabled.
📝 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.
Suggested change
| let credential_refresh_worker_handle = match ( | |
| services.credential_refresh_candidate_source.take(), | |
| services.credential_refresh_leader_lock.take(), | |
| services.product_auth.clone(), | |
| ) { | |
| (Some(candidate_source), Some(leader_lock), Some(refresh_port)) => { | |
| crate::credential_refresh_worker::spawn_credential_refresh_worker( | |
| credential_refresh, | |
| crate::credential_refresh_worker::CredentialRefreshWorkerDeps { | |
| candidate_source, | |
| refresh_port, | |
| leader_lock: std::sync::Arc::new(leader_lock), | |
| }, | |
| ) | |
| } | |
| _ => None, | |
| }; | |
| #[cfg(any(feature = "libsql", feature = "postgres"))] | |
| let credential_refresh_worker_handle = { | |
| let credential_refresh_enabled = credential_refresh.enabled; | |
| let handle = match ( | |
| services.credential_refresh_candidate_source.take(), | |
| services.credential_refresh_leader_lock.take(), | |
| services.product_auth.clone(), | |
| ) { | |
| (Some(candidate_source), Some(leader_lock), Some(refresh_port)) => { | |
| crate::credential_refresh_worker::spawn_credential_refresh_worker( | |
| credential_refresh, | |
| crate::credential_refresh_worker::CredentialRefreshWorkerDeps { | |
| candidate_source, | |
| refresh_port, | |
| leader_lock: std::sync::Arc::new(leader_lock), | |
| }, | |
| ) | |
| } | |
| _ => None, | |
| }; | |
| if credential_refresh_enabled | |
| && profile == RebornCompositionProfile::Production | |
| && handle.is_none() | |
| { | |
| return Err(RebornRuntimeError::InvalidArgument { | |
| reason: "credential refresh is enabled for production but worker dependencies are unavailable" | |
| .to_string(), | |
| }); | |
| } | |
| handle | |
| }; |
🤖 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 `@crates/ironclaw_reborn_composition/src/runtime.rs` around lines 2959 - 2975,
The match statement handling credential refresh worker spawning silently returns
None when dependencies are missing, but in production when
credential_refresh.enabled is true, this violates the activation contract.
Refactor the logic to add a guard condition that checks if
credential_refresh.enabled is true before entering the match block. If enabled
and any dependency (candidate_source, leader_lock, or product_auth) is absent,
panic or fail the startup. Only allow the silent None return when
credential_refresh is not enabled. This ensures the worker actually runs in
production when explicitly enabled.
Source: Coding guidelines
Collaborator
Author
There was a problem hiding this comment.
This is currently a deliberate choice: the worker logs warn! and continues if a dependency is missing rather than failing startup. enabled is gated to the Serve caller where the deps are always wired, so the missing-deps path should not occur in production. I can switch to a hard startup failure (fail-closed on misconfig) if you prefer — flagging for a decision.
There was a problem hiding this comment.
@henrypark133 I’d keep the hard-fail for Production && credential_refresh.enabled.
Evidence/invariant: the crates/ironclaw_reborn_composition/**/*.rs rule says production composition must fail closed on missing required handles. Once the worker is enabled, candidate_source, leader_lock, and product_auth are required handles for the PR objective “worker runs in production composition.”
Given your note that Serve always wires these deps, the guard should be unreachable in the valid path. That makes it cheap insurance against future miscomposition:
- non-prod / disabled: warn+continue is fine
- production / enabled / missing deps: return
RebornRuntimeError::InvalidArgument
So my preference: switch to hard startup failure for that single production-enabled case. The current warn! is still useful for the non-prod/disabled diagnostic path.
✏️ Learnings added
Learnt from: henrypark133
Repo: nearai/ironclaw PR: 5087
File: crates/ironclaw_reborn_composition/src/runtime.rs:2959-2988
Timestamp: 2026-06-20T22:29:08.870Z
Learning: In `crates/ironclaw_reborn_composition/src/runtime.rs` for PR 5087, `CredentialRefreshSettings.enabled` is gated to the Serve caller, where credential refresh worker dependencies (`credential_refresh_candidate_source`, `credential_refresh_leader_lock`, and `product_auth`) are expected to be wired; the missing-dependencies path is intended as a misconfiguration/regression diagnostic rather than a normal production path.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: zmanian
Repo: nearai/ironclaw PR: 4559
File: crates/ironclaw_reborn_composition/src/runtime.rs:4763-4859
Timestamp: 2026-06-15T13:25:49.116Z
Learning: In this crate’s Trace Commons autonomous capture tests, trace attribution must use the event-derived composite scope `trace_scope_key(tenant, owner)`. When validating behavior for non-runtime-owner callers, the tests should fail if attribution is set using only a bare `owner` id (without `tenant`) or if attribution targets a runtime-wide owner scope instead of the caller’s non-runtime owner scope.
Learnt from: henrypark133
Repo: nearai/ironclaw PR: 4953
File: crates/ironclaw_reborn_composition/src/slack_delivery.rs:2520-2520
Timestamp: 2026-06-17T04:13:18.772Z
Learning: When suppressing Clippy with `#[allow(clippy::too_many_arguments)]`, require an “architecture exemption” comment immediately above the attribute that explains why the many-arguments signature is justified (e.g., what bundled concepts the function needs) and provides a concrete rationale for the exemption. Avoid silent `#[allow(...)]` for this lint.
serrrfirat
requested changes
Jun 19, 2026
serrrfirat
left a comment
serrrfirat
left a comment
Collaborator
There was a problem hiding this comment.
Multi-agent review complete for 745c7bca154dc7e09a57af77aa5bd4c74d21f4e1.
Reviewed by: security, bugs, performance/concurrency, tests, conventions. Security returned no findings. I filtered one conventions false positive for a file outside this PR.
Requesting changes because the refresh-token rollback path can turn a transient access-secret write failure into a lost refresh secret for an existing account. Other comments cover keepalive coverage gaps, leader-lock failure behavior, cancellation, and config/test hygiene.
serrrfirat
requested changes
Jun 19, 2026
serrrfirat
left a comment
serrrfirat
left a comment
Collaborator
There was a problem hiding this comment.
Thermo-nuclear maintainability review for 745c7bca154dc7e09a57af77aa5bd4c74d21f4e1.
Requesting changes on structural grounds. The feature is doing the right kind of work, but the implementation adds several fragile seams: a dead-looking config field, a hidden secret-store dependency, a duplicate product-auth path traversal, and a leader-lock abstraction whose behavior is harder to reason about than its name suggests. These are fixable by tightening ownership boundaries rather than adding more local branches.
serrrfirat
requested changes
Jun 19, 2026
serrrfirat
left a comment
serrrfirat
left a comment
Collaborator
There was a problem hiding this comment.
Multi-agent review completed for #5087 at 745c7bc.
Summary: requesting changes for high-confidence correctness/concurrency risks in the OAuth refresh worker path.
Primary findings:
- High: access-token write failure after refresh-token rotation can delete the currently live deterministic refresh-token handle.
- High: deployment-wide keepalive candidate enumeration only scans the plain user product-auth root and misses agent/project-scoped runtime credentials.
- High: the Postgres session advisory lock is not cancellation-safe while the sweep performs filesystem and HTTP work.
- Medium: leader-lock acquisition failures fail open, so every process can sweep during DB pressure.
- Medium: max_per_tick caps refresh calls only after full account-tree enumeration has already completed.
Additional issues to address:
- Add regression coverage for InvalidGrant revoking accounts, worker idle/cap filtering, candidate enumeration filters, and Postgres leader-lock contention.
- Document IRONCLAW_CREDENTIAL_REFRESH_ENABLED in .env.example because CLAUDE.md says that file lists all env vars.
- Fix the malformed arch-exempt annotation in factory.rs, and remove the duplicate cfg on the credential_refresh_worker module declaration.
| let idle_cutoff = now - idle_threshold; | ||
|
|
||
| // B1: enumerate all Google/Configured/has-refresh accounts across all owners. | ||
| let candidates = deps.candidate_source.list_refresh_candidates().await; |
Collaborator
There was a problem hiding this comment.
Medium severity resource issue: max_per_tick is applied only after list_refresh_candidates has walked every tenant/user/product-auth tree and materialized every Google configured account. Large deployments still do O(all account files) reads and allocations per tick while holding the Postgres leader-lock connection. Push idle_cutoff and max_per_tick into the candidate source, stream/early-stop candidates, or use an indexed projection.
Collaborator
Author
There was a problem hiding this comment.
Correct — max_per_tick is applied after enumeration. At the current scale (low tick rate, small account counts) this is acceptable and truncation is logged. Pushing the cutoff/cap into the candidate source or an indexed projection is a scale follow-up; flagging it rather than building it now.
… rollback, margin wiring, annotations (#5071) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5087
June 20, 2026 01:59
Destroyed
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 `@crates/ironclaw_reborn_composition/src/oauth_provider_client.rs`:
- Around line 351-357: Update the comment block above the access_expires_at
variable assignment to accurately reflect that the code clamps to i32::MAX
instead of i64::MAX. The comment currently references guarding against u64
values exceeding i64::MAX on lines 352-353, but the actual implementation at the
.min() call uses i32::MAX as the upper bound. Correct the comment to state
i32::MAX and explain that this provides ample safety margin (approximately 68
years) while still being a saturating cast guard.
🪄 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: 1ded8adb-a718-422a-aa27-ba7e4590b205
📒 Files selected for processing (9)
.env.examplecrates/ironclaw_reborn_composition/src/credential_refresh_worker.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/lib.rscrates/ironclaw_reborn_composition/src/oauth_provider_client.rscrates/ironclaw_reborn_composition/src/oauth_provider_client/tests.rscrates/ironclaw_reborn_composition/src/product_auth_durable.rscrates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rscrates/ironclaw_reborn_composition/src/runtime_input.rs
💤 Files with no reviewable changes (1)
- crates/ironclaw_reborn_composition/src/lib.rs
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5087
June 20, 2026 03:00
Destroyed
…s missing (#5087) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5087
June 20, 2026 03:02
Destroyed
Collaborator
Author
|
Thanks all — addressed the review across commits Correctness / HIGH
Leader lock (@serrrfirat)
Config / contract
Smaller: Deferred (noted): bounding |
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 (3)
crates/ironclaw_reborn_composition/src/oauth_provider_client.rs (2)
260-279:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTreat provider rate limiting as transient, not stale auth.
Line 260 only preserves 5xx as
BackendUnavailable; a refresh429falls through toRefreshFailed, which the supplied workflow mapping treats as stale auth. That can force reauth on a provider throttle instead of retrying next tick.Suggested fix
- if (500..600).contains(&response.status) { + if response.status == 429 || (500..600).contains(&response.status) { return Err(AuthProductError::BackendUnavailable); }🤖 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 `@crates/ironclaw_reborn_composition/src/oauth_provider_client.rs` around lines 260 - 279, The code currently only treats 5xx errors as transient BackendUnavailable failures; when a 429 (rate limit) response occurs on a refresh request, it falls through to RefreshFailed which is treated as stale auth. Add a check for the 429 status code in the same error handling block where the 5xx check occurs (before the refresh_request conditional logic), and return AuthProductError::BackendUnavailable for 429 responses as well, so rate limiting is properly handled as a transient error rather than forcing reauthentication.
295-309:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly retain the refresh secret on account refresh, not initial exchange failure.
store_tokens()uses flow/invocation-scoped handles before an account references them, but Line 400 now always keeps a written refresh token when the access write fails. That fixes account refresh rotation, but leaks an orphan refresh token for first-time OAuth exchange failures. Split the failure policy by caller: cleanup exchange-created refresh handles, retain deterministic account refresh handles.Suggested direction
- self.store_token_pair(scope, access_secret, refresh_secret, tokens) + self.store_token_pair( + scope, + access_secret, + refresh_secret, + tokens, + true, // cleanup_refresh_on_access_failure: exchange handle is not account-owned yet + ) .await @@ - self.store_token_pair(scope, access_secret, refresh_secret, tokens) + self.store_token_pair( + scope, + access_secret, + refresh_secret, + tokens, + false, // account refresh handle is already durable account state + ) .await @@ refresh_secret: Option<SecretHandle>, tokens: OAuthTokenResponse, + cleanup_refresh_on_access_failure: bool, @@ { + if cleanup_refresh_on_access_failure + && let Some(handle) = &refresh_secret + { + let _ = self.secret_store.delete(&scope, handle).await; + } // Access write failed. Do NOT delete the refresh secret that wasAlso applies to: 328-340, 370-407
🤖 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 `@crates/ironclaw_reborn_composition/src/oauth_provider_client.rs` around lines 295 - 309, The store_tokens() method currently applies the same failure policy for both initial OAuth exchange and account refresh scenarios, which causes orphan refresh tokens to leak during exchange failures. Modify store_tokens() to differentiate between these two callers by adding a parameter that indicates whether this is an initial exchange or an account refresh operation. When an access token write fails during initial exchange (in the exchange_token_handle call for "access"), cleanup the orphan refresh_secret handle that was created. When the same failure occurs during account refresh, retain the refresh_secret handle deterministically as the current behavior does. This split policy should be applied consistently across all affected call sites including the token pair storage and error handling paths around lines 328-340 and 370-407.crates/ironclaw_reborn_composition/src/credential_refresh_worker.rs (1)
272-279:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClassify
Ok(report)byreport.refreshed, not transport success.The refresh contract can return
Ok(report)withrefreshed == falsefor recoverable credential failures/status changes, but this branch counts everyOkas refreshed. That hides needs-reauth outcomes in the worker counters and contradicts the operator-distinguishability objective.Suggested classification fix
- Ok(_) => { - refreshed += 1; - tracing::debug!( - provider = %account.provider, - "credential refresh worker: account refreshed" - ); - } + Ok(report) if report.refreshed => { + refreshed += 1; + tracing::debug!( + provider = %account.provider, + "credential refresh worker: account refreshed" + ); + } + Ok(report) if report.account.status == ironclaw_auth::CredentialAccountStatus::Configured => { + skipped += 1; + tracing::debug!( + provider = %account.provider, + status = ?report.account.status, + "credential refresh worker: account refresh completed without rotating" + ); + } + Ok(report) => { + failed += 1; + tracing::debug!( + provider = %account.provider, + status = ?report.account.status, + "credential refresh worker: account refresh produced a recoverable failure status" + ); + }🤖 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 `@crates/ironclaw_reborn_composition/src/credential_refresh_worker.rs` around lines 272 - 279, The refresh_credential_account method returns an Ok(report) that contains a refreshed field, but the current match arm treats all Ok responses as successful refreshes by incrementing the counter immediately. Instead of counting every Ok response, destructure the Ok response to extract the report object, then check the report.refreshed field to determine if the credential was actually refreshed. Only increment the refreshed counter when report.refreshed is true, and handle the case where report.refreshed is false separately to properly track recoverable failures and status changes in the worker counters.
🤖 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 `@crates/ironclaw_reborn_composition/src/product_auth_durable.rs`:
- Around line 543-545: The documentation comment for this function guarantees
that it "never projects secret handles," but the implementation pushes full
CredentialAccount values with their access_secret and refresh_secret handles
still populated, violating this guarantee. To fix this, either create and return
a narrowed DTO struct that excludes the secret handle fields (access_secret and
refresh_secret) instead of the full CredentialAccount, or soften the
documentation comment to state that only "raw secret material" is excluded and
explicitly note that consumers must not log or serialize the returned values.
Ensure any guarantee made in the comment is either enforced by the type system
or clearly described as an intent rather than a hard contract.
In
`@crates/ironclaw_reborn_composition/src/product_auth_runtime_credentials/tests.rs`:
- Around line 1197-1205: The test setup code for the secret store operations is
silently discarding errors from the put() method calls by using .ok(), which
prevents the test from failing if the fixture setup fails. Replace the .ok()
calls on the secret_store.put() operations (in both the first occurrence at
lines 1197-1205 and the second occurrence at lines 1243-1251) with proper error
handling such as .expect() with a descriptive message that clearly indicates the
setup operation must succeed, so that any setup failures cause the test to fail
loudly rather than continuing with incomplete test state.
---
Outside diff comments:
In `@crates/ironclaw_reborn_composition/src/credential_refresh_worker.rs`:
- Around line 272-279: The refresh_credential_account method returns an
Ok(report) that contains a refreshed field, but the current match arm treats all
Ok responses as successful refreshes by incrementing the counter immediately.
Instead of counting every Ok response, destructure the Ok response to extract
the report object, then check the report.refreshed field to determine if the
credential was actually refreshed. Only increment the refreshed counter when
report.refreshed is true, and handle the case where report.refreshed is false
separately to properly track recoverable failures and status changes in the
worker counters.
In `@crates/ironclaw_reborn_composition/src/oauth_provider_client.rs`:
- Around line 260-279: The code currently only treats 5xx errors as transient
BackendUnavailable failures; when a 429 (rate limit) response occurs on a
refresh request, it falls through to RefreshFailed which is treated as stale
auth. Add a check for the 429 status code in the same error handling block where
the 5xx check occurs (before the refresh_request conditional logic), and return
AuthProductError::BackendUnavailable for 429 responses as well, so rate limiting
is properly handled as a transient error rather than forcing reauthentication.
- Around line 295-309: The store_tokens() method currently applies the same
failure policy for both initial OAuth exchange and account refresh scenarios,
which causes orphan refresh tokens to leak during exchange failures. Modify
store_tokens() to differentiate between these two callers by adding a parameter
that indicates whether this is an initial exchange or an account refresh
operation. When an access token write fails during initial exchange (in the
exchange_token_handle call for "access"), cleanup the orphan refresh_secret
handle that was created. When the same failure occurs during account refresh,
retain the refresh_secret handle deterministically as the current behavior does.
This split policy should be applied consistently across all affected call sites
including the token pair storage and error handling paths around lines 328-340
and 370-407.
🪄 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: 73ba6dcd-6da5-4417-b26b-772dba419286
📒 Files selected for processing (12)
crates/ironclaw_reborn_composition/src/auth.rscrates/ironclaw_reborn_composition/src/credential_refresh_worker.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/oauth_provider_client.rscrates/ironclaw_reborn_composition/src/product_auth_durable.rscrates/ironclaw_reborn_composition/src/product_auth_durable/tests.rscrates/ironclaw_reborn_composition/src/product_auth_refresh_lock.rscrates/ironclaw_reborn_composition/src/product_auth_runtime_credentials.rscrates/ironclaw_reborn_composition/src/product_auth_runtime_credentials/tests.rscrates/ironclaw_reborn_composition/src/runtime.rscrates/ironclaw_reborn_composition/src/runtime_input.rscrates/ironclaw_secrets/src/lib.rs
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…google-oauth-refresh-5071
henrypark133
commented
Jun 21, 2026
henrypark133
left a comment
henrypark133
left a comment
Collaborator
Author
There was a problem hiding this comment.
Code Review (multi-agent)
Intent: Proactively refresh Google OAuth credentials before expiry so accounts stay usable without manual reconnects.
Stats: 3 findings (from 9 raw; 5 off-diff findings dropped; 1 acknowledged scale follow-up not reposted) across 3 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, approach. Reviewers failed: none. Body-only: 0
Approach
- Medium The sweep cap can permanently starve the same accounts (
crates/ironclaw_reborn_composition/src/credential_refresh_worker.rs:233-235, confidence 91) - anchor: crates/ironclaw_reborn_composition/src/credential_refresh_worker.rs:233
The worker applies max_per_tick after filtering idle candidates, but the candidate source returns a stable account-id order. Once more than max_per_tick Google accounts are idle, each tick can refresh the same low-id subset while later accounts remain idle forever, defeating the keepalive goal for those accounts. Fix: Order idle candidates by oldest updated_at before truncating, or persist a rotating cursor so each tick advances through a different slice.
Tests
- Low InvalidGrant needs a code-mapping regression test (
crates/ironclaw_auth/src/error.rs:49-91, confidence 96) - anchor: crates/ironclaw_auth/src/error.rs:91
The new AuthProductError::InvalidGrant variant maps to the stable sanitized RefreshFailed code, but the existing sanitized-code contract still only exercises BackendUnavailable, TokenExchangeFailed, and RefreshFailed. That leaves the new stable boundary unverified. Fix: Extend auth_product_contract::serde_redaction_contract::backend_failures_are_reported_as_stable_sanitized_codes to include AuthProductError::InvalidGrant -> AuthErrorCode::RefreshFailed. - Medium Invalid_grant revocation path has no caller-level test (
crates/ironclaw_auth/src/credential.rs:969-976, confidence 92) - anchor: crates/ironclaw_auth/src/credential.rs:969
The new InvalidGrant branch is distinct from the existing generic refresh-failure coverage: it should mark the account Revoked and project AccountRevoked recovery, not RefreshFailed. I found coverage for generic refresh failures, but not this new branch. Fix: Add an auth_product_contract refresh test where the provider returns invalid_grant and assert the persisted account is Revoked with AccountRevoked recovery.
…erging main (#5071) main #5095 added recorded QA fixtures whose harness seeds the secret store via SecretStore::put; this branch changed that signature to take expires_at. The merge of the two collided (E0061) in the branch-merged-into-main CI ref while the branch alone compiled. Add the `None` expiry arg to both seed calls in tests/support/reborn/qa_trace.rs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5087
June 21, 2026 01:43
Destroyed
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)
crates/ironclaw_reborn_composition/src/runtime.rs (1)
1733-1749:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the shutdown contract comment.
shutdownnow also stops the trigger poller, credential-refresh worker, and trace flush worker, so the “both tasks” docstring is stale.As per coding guidelines, “When you change behavior in a function, re-read its docstring and adjacent comments — update or delete them in the same change.”
Suggested doc update
- /// Stop the turn-runner worker and the budget-event projection. - /// Awaits both tasks before returning so background state is fully - /// drained when the runtime drops. + /// Stop all runtime-owned background workers and projections. + /// Awaits shutdown before returning so background state is fully + /// drained when the runtime drops.🤖 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 `@crates/ironclaw_reborn_composition/src/runtime.rs` around lines 1733 - 1749, The docstring for the `shutdown` method is outdated and refers to shutting down "both tasks" and only mentions the "turn-runner worker and the budget-event projection". Update the docstring to accurately reflect that the method now shuts down multiple workers including the trigger_poller_handle, credential_refresh_worker_handle, and trace flush worker. Replace the stale reference to "both tasks" with an accurate description of all workers being shut down.Source: Coding guidelines
🤖 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 `@crates/ironclaw_reborn_composition/src/runtime.rs`:
- Around line 1733-1749: The docstring for the `shutdown` method is outdated and
refers to shutting down "both tasks" and only mentions the "turn-runner worker
and the budget-event projection". Update the docstring to accurately reflect
that the method now shuts down multiple workers including the
trigger_poller_handle, credential_refresh_worker_handle, and trace flush worker.
Replace the stale reference to "both tasks" with an accurate description of all
workers being shut down.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 131c4b03-652d-4dfc-810b-0fe09c3faae5
📒 Files selected for processing (5)
.env.examplecrates/ironclaw_reborn_composition/src/auth.rscrates/ironclaw_reborn_composition/src/factory.rscrates/ironclaw_reborn_composition/src/runtime.rstests/support/reborn/qa_trace.rs
💤 Files with no reviewable changes (1)
- tests/support/reborn/qa_trace.rs
…on (#5071) Address PR #5087 review (review 4539067623): - credential_refresh_worker: sort idle candidates oldest-first (by updated_at) before applying max_per_tick. The oldest accounts are closest to Google's 7-day refresh-token idle-death, so they must be served first — this stops a head-of-enumeration subset (e.g. accounts stuck in transient errors that keep their old updated_at) from starving the tail when more than max_per_tick accounts are idle. Unit test now asserts oldest-first selection. - auth contract: lock the stable sanitized-code mapping AuthProductError::InvalidGrant -> AuthErrorCode::RefreshFailed. - auth contract: add a caller-level test for the new invalid_grant branch — provider returns InvalidGrant, assert the persisted account is Revoked and the recovery projection is ReauthorizeRequired/AccountRevoked (distinct from the generic RefreshFailed path). Adds an invalid_grant_next_refresh_for_tests hook to the auth fakes, mirroring the existing fail_next_refresh_for_tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
railway-app
Bot
temporarily deployed
to
ironclaw-ci-preview / ironclaw-pr-5087
June 21, 2026 01:58
Destroyed
This was referenced Jun 21, 2026
This was referenced Jun 23, 2026
This was referenced Jun 23, 2026
This was referenced Jun 28, 2026
Closed
4 tasks
18 tasks
This was referenced Jul 6, 2026
This was referenced Jul 15, 2026
test(stores): FaultInjecting backend decorator; drive store fault tests through the real store
#6400
Merged
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5071.
What
Keeps Google OAuth credentials usable without manual reconnect, two mechanisms by TTL:
How
SecretStore::putgainsexpires_at;FilesystemSecretStorealready had the field. Inline refresh reads it back viametadata()and applies a config margin (CredentialRefreshSettings::access_refresh_margin, default 5 min).invalid_grant→ accountRevoked→ caller-level reauth (not a generic tool failure); 5xx/network → transient, no status change. Only theerrorcode is parsed — no token/body/secret reaches logs/Debug/serde.credential_refresh_worker.rs), mirrorstrigger_poller: enumerates idle GoogleConfiguredaccounts with a refresh secret (over the existing account store — no new table/SQL), refreshes each,debug!-only, jittered, cancellation-aware.pg_try_advisory_lock; non-leaders skip. Exactly one process sweeps, holding one connection. The inline hot path takes no cross-process lock (just the existing in-process guard) — so no pooled connection is held across the Google HTTP call.Servesurface with anIRONCLAW_CREDENTIAL_REFRESH_ENABLEDkill-switch (mirrors the trigger poller).Design notes / review
Two rounds of multi-agent review (plan + code). The cross-process guard was simplified from a per-account blocking lock (held across the HTTP refresh — pool-exhaustion + unlock-leak risk) to the worker-only leader lock above. Also applied:
secret_storemade non-optional (removes a silent margin-skip no-op), margin moved to config, logging/jitter/comment/cancellation fixes.Verification (mock-based, no real tokens): unit + caller-level tests for the worker tick/candidate-filter/transient-error, spawn guard, inline margin (fresh-skip / within-margin / absent-expiry fail-safe), and
expires_atput→metadata roundtrip.cargo clippyclean (libsql + postgres),ironclaw_architectureboundary tests pass. The refresh request shape and the consume-doesn't-destroy-the-token property were audited.Out of scope / follow-ups: worker candidate enumeration loads accounts in memory with a sequential outer walk (fine at current scale; bound it for very large deployments); the postgres leader-lock two-process serialization test needs a testcontainer (compiles, gated). A Postgres integration test and an optional operator-run live smoke harness can land separately.
Known flaky (pre-existing, not introduced here):
runtime::tests::local_dev_runtime_*collide under high test parallelism (one case already fails onmain); the suite passes serially.🤖 Generated with Claude Code