[codex] Implement Google OAuth refresh lifecycle

[henrypark133]

Summary

Closes #4160.

• implement concrete Google OAuth refresh-token grant through host-mediated egress and fixed Google token endpoint policy
• wire provider-backed credential refresh through the auth-owned account service boundary, preserving existing refresh handles when Google omits a replacement refresh token
• add compensating cleanup for callback token persistence failures and shared secret-store delete support
• retry GSuite capability execution once after expired-auth responses, while keeping insufficient-scope responses intact

Validation

• cargo test -p ironclaw_auth --all-targets -- --nocapture
• cargo test -p ironclaw_reborn_composition --all-targets -- --nocapture
• cargo test -p ironclaw_secrets --all-targets -- --nocapture
• cargo test -p ironclaw_first_party_extensions --all-targets -- --nocapture
• cargo clippy -p ironclaw_auth -p ironclaw_reborn_composition --all-targets -- -D warnings
• git diff --check
• post-rebase smoke: cargo test -p ironclaw_reborn_composition google_oauth::tests::product_auth_refresh_uses_concrete_google_provider_and_updates_account -- --nocapture

Notes

A gpt-5.4-mini subagent reviewed crate/module ownership after the implementation. It initially flagged provider-backed refresh policy living in ironclaw_reborn_composition; that logic was moved into ironclaw_auth::ProviderBackedCredentialAccountService, leaving Reborn composition as wiring only. The re-check passed with no remaining boundary concerns.

[gemini-code-assist]

Code Review

This pull request implements automatic token refresh capabilities for Google OAuth credentials, introducing a ProviderBackedCredentialAccountService to coordinate token updates and updating the GSuite executor to automatically refresh expired tokens and retry failed requests. It also adds a delete method to the SecretStore trait to clean up orphaned secrets during failed OAuth callback completions. The review feedback highlights several key improvement opportunities: grouping HTTP 5xx errors under BackendUnavailable to avoid permanent credential failure states during temporary outages, raising the log level to warn for secondary cleanup failures, optimizing GSuite response checks to avoid redundant JSON parsing, removing a redundant read check before deleting secrets, and ensuring all secrets are cleaned up in delete_tokens even if an individual deletion fails.

[henrypark133]

Code Review (multi-agent)

Intent: Implement Google OAuth refresh-token grant with host-mediated egress, credential lifecycle management, and retry-on-expired-auth for GSuite extensions.

Stats: 17 findings (from 26 raw, 17 after dedup) across 6 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, pattern-refactor. Reviewers failed: none. Body-only: 1 (one Low demoted under the 15-inline cap).

Substantive PR with real correctness risks in the refresh lifecycle. Two independent reviewers (bugs + security) converged on the destructive-consume bug, and bugs + performance both flagged the TOCTOU race — those are the headline issues. REQUEST_CHANGES driven by the High-confidence refresh-token data-loss path and the concurrency race.

Bugs

1. High load_refresh_token consumes the refresh secret before new tokens are stored (crates/ironclaw_reborn_composition/src/google_oauth/secret_sink.rs:200-214, confidence 90) — anchor: secret_sink.rs:207. lease_once + consume destroys the old refresh token; if store_refreshed_tokens then fails, the account keeps a dead handle and can never auto-recover. A transient network blip during refresh permanently locks the user out until re-auth. Also flagged by security/Medium, performance/High.
2. Medium Refresh handle keyed on scope.invocation_id orphans access secrets every refresh cycle (secret_sink.rs:242-251, confidence 80) — anchor: secret_sink.rs:246. New key per request; prior access secret never deleted → unbounded orphaned live tokens. Also flagged by security/Low, local-patterns/Nit (different error mapper than sibling store_tokens).
3. Medium Retry response not re-checked for auth expiry — a 401 on retry (or on the AddAttendees PATCH leg) is silently returned as successful output (crates/ironclaw_first_party_extensions/src/gsuite/handlers.rs:96-102, confidence 78) — anchor: handlers.rs:100. Also flagged by performance/Medium.
4. Medium delete_tokens aborts on first failure, leaving remaining handles undeleted (secret_sink.rs:216-228, confidence 75) — anchor: secret_sink.rs:221. Partial cleanup leaves a dangling refresh secret. Also flagged by performance/Medium.

Security

5. High 401 retry re-resolves the account (may pick a different account for the scope) and has no rate limit on attacker-triggerable refresh cycles (handlers.rs:81-102, confidence 75) — anchor: handlers.rs:83. Quota-burn + potential cross-account credential use.
6. Medium is_google_auth_expired_response routes auth decisions off the untrusted Google response body; a crafted {"error":{"status":"UNAUTHENTICATED"}} on any status triggers refresh (handlers.rs:285-302, confidence 75) — anchor: handlers.rs:290. Gate on HTTP 401 only.

Performance / Concurrency

7. High TOCTOU race in refresh_account: non-atomic read-refresh-reread compare-and-swap; concurrent refresh for the same account corrupts status and orphans a valid token (crates/ironclaw_auth/src/credential.rs:665-755, confidence 82) — anchor: credential.rs:709. Add optimistic versioning or a per-account lock. Also flagged by security/Medium.
8. Medium spawn_blocking wraps HTTP egress per token request — blocking-pool exhaustion risk under concurrent refreshes if egress is async-backed (crates/ironclaw_reborn_composition/src/google_oauth/client.rs:253-255, confidence 75) — anchor: client.rs:253.

Tests

9. High GoogleProviderClient::refresh_token system-scope CrossScopeDenied guard has no test (client.rs:215-217, confidence 90) — anchor: client.rs:215.
10. High GoogleProviderClient::cleanup_exchange has no unit test; only a mock exists (client.rs:285-298, confidence 85) — anchor: client.rs:285.
11. Medium add_attendees 401-on-GET refresh+retry path untested (multi-step capability; "Test Through the Caller") (handlers.rs:226-231, confidence 80) — anchor: handlers.rs:229. Also flagged by conventions/Medium.
12. Medium System-ownership refresh rejection untested through the production ProviderBackedCredentialAccountService (credential.rs:758-771, confidence 75) — anchor: credential.rs:770.

Conventions

13. Medium unwrap_or_else on project_credential_recovery I/O call lacks // silent-ok justification and discards the error (credential.rs:598-601, confidence 80) — anchor: .claude/rules/error-handling.md.

Maintainability / Pattern

14. Medium ProviderBackedCredentialAccountService grows credential.rs to 831 lines — extract to its own file (credential.rs:524-831, confidence 75) — anchor: credential.rs:524. Also flagged by local-patterns/Nit (missing doc comment).
15. Medium Lift refresh-and-retry into a decorator wrapping capability execution; dissolves the sentinel Ok(expired_response) contract and the two duplicate inspection points (handlers.rs:60-117, confidence 75) — anchor: handlers.rs:81.
16. Low (body-only) with_provider_client wrapping duplicated across two parallel builder structs (crates/ironclaw_reborn_composition/src/auth.rs:328-336, confidence 60) — anchor: auth.rs:328. Apply on the ports struct only and carry through into_services.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 217a6cfa76

ℹ️ 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".

[henrypark133]

Code Review (multi-agent)

Intent: Implement Google OAuth refresh-token lifecycle — concrete refresh grant via host-mediated egress + fixed Google token-endpoint policy; provider-backed credential refresh through the auth-owned ProviderBackedCredentialAccountService, preserving the existing refresh handle when Google omits a replacement; compensating cleanup for callback persistence failures + shared secret-store delete; retry GSuite capability once after expired-auth while keeping insufficient-scope intact. Closes #4160.

Stats: 11 findings (from ~17 raw, after dedup; 1 reviewer claim verified false and dropped — see note) across 5 files. Reviewers run: security, bugs, performance, tests, conventions, local-patterns, maintainability, pattern-refactor. Reviewers failed: none. Inline comments suppressed — all 23 files are modifications to large existing files where diff-line anchors don't resolve cleanly; exact file:line anchors are below. Diff truncated for reviewer context (122 KB / 23 files); reviewers had full worktree access. REQUEST_CHANGES driven by finding #1.

Dropped (verified false): a reviewer flagged load_refresh_token's lease_once+consume as destroying the refresh token before the HTTP call. Verified against ironclaw_secrets: consume marks the lease consumed and drops retained lease material, but the underlying secret persists (get_decrypted, not delete). The refresh token survives a failed exchange. Not a bug.

Bugs / Security — High

1. High — store_refreshed_tokens compensating delete wipes the freshly-written valid access token (crates/ironclaw_reborn_composition/src/google_oauth/secret_sink.rs:186, bugs conf 90 / security conf 75). Unlike store_tokens (whose handle is unique per flow_id+invocation_id, so cleanup is safe), the refresh path's handle is fixed per account (google_refresh_token_handle → google-oauth-refresh-{kind}-{account_id}). Line 174 overwrites the live access secret with the new token; if the refresh-token put then fails, line 186 deletes that fixed handle — destroying the just-issued valid access token and leaving the account's persisted record pointing at a now-missing secret. Recovery is not guaranteed: the GSuite retry-once path only fires on HTTP 401, not on a "unknown secret" load error, so the account can stay broken until something else forces a refresh. Fix: on the refresh path, do not delete the access secret on refresh-write failure (the new access token is valid and usable) — or write the refresh token before the access token so a partial failure leaves the prior access token intact. Also flagged by: tests/High — this compensating branch has zero coverage (the analogous store_tokens cleanup has two tests).

Performance / Security — Medium

2. Medium — ProviderBackedCredentialAccountService.refresh_locks is a Mutex<HashMap<CredentialAccountId, Arc<Mutex<()>>>> that inserts on first refresh and never evicts (crates/ironclaw_auth/src/credential.rs:545, perf 85 / security 80 / conventions 75). Grows unbounded over process lifetime per distinct account; CLAUDE.md: in-memory caches must be evicted. Fix: drop the entry after the guard releases when Arc::strong_count == 1.
3. Medium — Both exchange_callback and refresh_token dispatch egress via tokio::task::spawn_blocking(|| egress.execute(...)) (crates/ironclaw_reborn_composition/src/google_oauth/client.rs:172, perf conf 90). Each in-flight token exchange/refresh pins a blocking-pool thread for the full HTTP round-trip (up to 30 s); concurrent refresh load can saturate the pool. Fix: make RuntimeHttpEgress::execute async (or use an async HTTP client) and await directly.
4. Medium — After a 401, the GSuite handler retries via resolver.resolve_account(credential.account_id) using the pre-refresh account_id rather than re-running the full resolve() selection (crates/ironclaw_first_party_extensions/src/gsuite/handlers.rs:88, security conf 70). resolve_account does not re-validate scope/ownership the way resolve() does. Fix: re-resolve through the standard resolve() path post-refresh.

Conventions / Tests — Medium

5. Medium — map_secret_store_error collapses every SecretStoreError variant into AuthProductError::BackendUnavailable via |_error| (secret_sink.rs:258, conventions conf 75); diagnostic detail (crypto vs FS vs precondition) is erased, and the compensating-cleanup paths discard the delete result (let _ =). Fix: preserve the variant in a debug! log at the mapping site so partial-failure orphans are detectable.
6. Medium — ProviderBackedCredentialAccountService is a concrete runtime service (provider calls, Mutex concurrency, refresh-serialization policy) living in ironclaw_auth, whose CLAUDE.md says "own product-facing auth vocabulary and fake services only" (credential.rs:524, conventions conf 75). The PR body explains the move from composition was deliberate after a subagent review — so this is a boundary-doc gap, not necessarily wrong placement. Fix: amend ironclaw_auth/CLAUDE.md to record the exception (or split into an ironclaw_auth_services crate).
7. Medium — The new SecretStore::delete is exercised by compensating cleanup but is not in the shared tests/secret_store_contract.rs contract suite (crates/ironclaw_secrets/tests/secret_store_contract.rs, tests conf 75). A future impl could mis-handle absent-handle return or scope isolation without a contract failure. Fix: add a delete contract test (idempotency + scoped isolation).

Low / Nit

8. Low — refresh_account issues up to 3 sequential get_account reads per refresh (credential.rs:735, perf conf 75); the post-HTTP success-path read can be dropped since the refresh lock prevents concurrent same-account mutation.
9. Low — Token-endpoint network policy is staged into the obligation store separately from the hard-coded GOOGLE_TOKEN_ENDPOINT egress call (client.rs:229, security conf 55); SSRF protection relies on RuntimeHttpEgress::execute consulting the staged policy — an implicit, type-unenforced invariant. Document/enforce it.
10. Low — execute_add_attendees returns Ok((401_response, bytes)) to signal "retry me" to dispatch, splitting the 401→refresh→retry protocol across two functions (handlers.rs:237, local-patterns conf 75). Note: the naive fix (delete the inner 401 check) is unsafe — the inner GET 401 would feed garbage into the PATCH. Restructure the GET+PATCH sequence or return a typed "auth-expired" outcome instead of an Ok-with-401 sentinel.
11. Nit — is_google_auth_expired_response has a dead (200..300) guard (401 is never 2xx); reduce to response.status == 401 (handlers.rs:298). And ProviderBackedCredentialAccountService is exported without a doc comment unlike its documented siblings (credential.rs:524).

Pattern-refactor: no consolidating reframe — confirmed the execute_add_attendees fix is already at the right zoom level and that a naive deletion would be unsafe.

[henrypark133]

Review follow-up pushed in ec695b0.

Fixed straightforward items:

• Preserved requester_extension on provider-backed refresh lookups and recovery projection; added caller-level extension-owned/shared-admin refresh coverage.
• Made expired refresh failures project stable RefreshFailed recovery through the provider-backed path.
• Stopped refresh-token write failure cleanup from deleting the stable per-account access secret; added focused regression coverage.
• Added shared SecretStore::delete idempotency + scope-isolation contract coverage.
• Evicted per-account refresh locks when no longer shared.
• Simplified auth-expired detection to strict status == 401 and pinned it with a unit test.
• Documented the ProviderBackedCredentialAccountService auth-crate ownership exception and added its public doc comment.

Not changing in this PR:

• Fresh resolve() after refresh: invalid for this contract. resolve_account still authorizes through requester-aware lookup and preserves same-account retry; fresh selection would change behavior.
• Dropping the post-HTTP account reread: invalid. The refresh lock serializes refreshes only, not all account mutations, so the reread protects against stale writes.
• spawn_blocking/async egress: valid scaling concern but requires a cross-crate egress API change.
• Add-attendees sentinel/decorator refactor: valid maintainability debt but broader than this review fix.
• Token endpoint staged-policy invariant and secret-store error detail: partial/doc-follow-up territory rather than required code change here.

Validation:

• cargo fmt --check
• cargo test -p ironclaw_auth --test auth_product_contract refresh -- --nocapture
• cargo test -p ironclaw_reborn_composition store_refreshed_tokens_keeps_access_secret_when_refresh_write_fails -- --nocapture
• cargo test -p ironclaw_secrets --test secret_store_contract secret_store_delete_is_idempotent_and_scope_isolated -- --nocapture
• cargo test -p ironclaw_first_party_extensions is_google_auth_expired_response_only_matches_401 -- --nocapture
• cargo clippy -p ironclaw_auth -p ironclaw_first_party_extensions -p ironclaw_reborn_composition -p ironclaw_secrets --all-targets -- -D warnings
• git diff --check

[henrypark133]

Handled the three remaining direct-impact follow-ups we discussed in b5b19ffc2:

• Replaced the implicit GSuite 401 response sentinel with a typed CapabilityExecutionOutcome::AuthExpired, including the add-attendees GET/PATCH path and retry accounting.
• Added caller-level coverage that production Google token egress fails closed when the token endpoint policy was not staged, with no upstream network request made.
• Added safe debug diagnostics for Google OAuth secret-store failures using stable reason codes only, including cleanup failure logging after refresh-token write failure.

Verification:

• cargo fmt --check
• cargo test -p ironclaw_first_party_extensions --test gsuite_core -- --nocapture
• cargo test -p ironclaw_reborn_composition google_provider -- --nocapture
• cargo test -p ironclaw_reborn_composition google_token_sink -- --nocapture
• cargo clippy -p ironclaw_first_party_extensions -p ironclaw_reborn_composition --all-targets -- -D warnings
• git diff --check

[henrypark133]

Code Review (multi-agent) — re-review at head 47a9a7e

Intent: Google OAuth refresh-token lifecycle — host-mediated refresh grant; provider-backed credential refresh via the auth-owned ProviderBackedCredentialAccountService; preserve the existing refresh handle when Google omits a replacement; compensating cleanup + shared secret-store delete; retry GSuite once after expired-auth. Closes #4160.

Stats: Forced re-review (new head, rebased on reborn-integration). 7 reviewers + verification of the prior round. No High/Critical → COMMENT. Inline suppressed (large modified files; anchors below). Diff 142 KB (incl. base merge); reviewers had full worktree access.

✅ Resolved since last review (head 217a6cf)

• High — store_refreshed_tokens compensating delete destroying the live access token: FIXED. Line 191 now returns the error without deleting the access secret; regression test store_refreshed_tokens_keeps_access_secret_when_refresh_write_fails locks it. Verified by 3 reviewers.
• SecretStore::delete now in the shared contract suite (secret_store_contract.rs — secret_store_delete_is_idempotent_and_scope_isolated).
• Auth-crate boundary (ProviderBackedCredentialAccountService in ironclaw_auth) — documented exception added to ironclaw_auth/CLAUDE.md.
• refresh_locks unbounded growth — release_refresh_lock now evicts on Arc::strong_count == 1; downgraded to a Low/comment item (see #6).

Remaining — Medium

1. Medium — spawn_blocking(|| egress.execute(...)) for the token endpoint pins a tokio blocking-pool thread for the full 30 s timeout, in both exchange_callback and refresh_token (crates/ironclaw_reborn_composition/src/google_oauth/client.rs:175 and :259, perf conf 85 / security conf 75). Concurrent exchange/refresh storms can exhaust the pool. Fix: make RuntimeHttpEgress::execute async, or bound concurrency with a semaphore.

Remaining — Tests (Medium)

2. Medium — store_refreshed_tokens partial-failure has a helper-level test, but no caller-level test (real InMemorySecretStore) asserting the access secret is still readable after a refresh-write failure (secret_sink.rs:179, conf 75). Per the "test through the caller" rule, add one through GoogleProviderClient.refresh_token.
3. Medium — No test for: first attempt 401→refresh succeeds→retry returns 403 insufficient-scope, asserting the 403 body passes through as a Response rather than re-triggering an auth error (gsuite/handlers.rs:108, conf 75).
4. Medium — No test that a refresh failure during the retry path preserves the first-attempt network_egress_bytes in the returned error usage (gsuite/handlers.rs:92, conf 75).

Remaining — Low / Nit

5. Low (contested) — Post-401 retry uses resolver.resolve_account(account_id) rather than full resolve() (gsuite/handlers.rs:95, conf ~60). Security flagged it as skipping select_unique_configured_account scope re-validation; bugs+conventions reviewers read resolve_account as still validating scope/extension via recoverable_lookup, with refresh_account validating through validate_refresh_target. Likely fine — worth a confirming glance that a scope-narrowed refresh surfaces as a scope error, not a silent under-scoped retry.
6. Low (informational — do NOT re-add the delete) — On a refresh-token write failure, store_refreshed_tokens leaves the just-written access secret in the store (orphaned until the next refresh overwrites the deterministic google-oauth-refresh-access-{account_id} handle). Two reviewers suggested "mirror store_tokens' compensating delete" — that suggestion is wrong here and would reintroduce the High bug just fixed: store_tokens uses a unique {flow_id}-{invocation_id} handle (safe to delete), but the refresh path's handle is the account's live fixed handle. Current behavior (keep it) is correct. Optional: add a one-line comment explaining why no cleanup runs, so a future editor doesn't "fix" it back into the bug.
7. Low — release_refresh_lock's Arc::strong_count == 1 eviction is correct under the shared std::Mutex (check-and-remove is atomic), but the rationale is non-obvious; add a brief comment (the filesystem_store.rs FILESYSTEM_RECORD_LOCKS never-evict comment is the local precedent) (crates/ironclaw_auth/src/credential.rs:562, conf 75).
8. Low — refresh_token duplicates the ~11-line Arc-clone + spawn_blocking + error-map block from exchange_callback (client.rs:259, conf 75); extract a shared execute_google_token_request helper (mirrors first_party_tools/http.rs's execute_runtime_http).
9. Low — refresh_account's Ok and RefreshFailed/TokenExchangeFailed arms repeat re-fetch + concurrent-check + update + report (~40 lines) (credential.rs:796, conf 65); extract an apply_provider_outcome helper.
10. Nit — ProviderBackedCredentialAccountService doc omits the non-obvious per-account single-flight locking (the reason for the HashMap<_, Arc<Mutex<()>>>) (credential.rs:524, conf 50).

Dropped (verified false, as last round): load_refresh_token's lease_once+consume is not token loss — consume consumes the lease, the underlying secret persists. map_secret_store_error variant-flattening is pre-existing and narrow (SecretExpired unreachable on put).

Good progress — the headline security bug is fixed and the supporting tests/contract/boundary items landed. Remaining items are hardening + coverage, none blocking.

[henrypark133]

No blocking comments merging.