[codex] fix auth session persistence on transient /auth/me#3746
Conversation
|
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:
📝 WalkthroughWalkthroughAdds JWT payload extraction, tolerates transient ChangesDeferred backend validation for transient /auth/me failures
Test environment locking and serialization
Sequence Diagram(s)sequenceDiagram
participant OAuth
participant store_session
participant fetch_current_user_for_session_store
participant auth_me_GET as GET /auth/me
participant disk as Profile Storage
participant snapshot
participant backend_refresh as Backend Refresh
OAuth->>store_session: sessionToken + user
store_session->>fetch_current_user_for_session_store: sessionToken
fetch_current_user_for_session_store->>auth_me_GET: GET /auth/me
alt 200 OK
auth_me_GET-->>fetch_current_user_for_session_store: user
fetch_current_user_for_session_store-->>store_session: Ok(user)
store_session->>disk: persist user (validated)
else 5xx / transient
auth_me_GET-->>fetch_current_user_for_session_store: transient error
fetch_current_user_for_session_store->>auth_me_GET: retry after delay
auth_me_GET-->>fetch_current_user_for_session_store: transient error again
fetch_current_user_for_session_store-->>store_session: Err(transient)
store_session->>store_session: jwt_exp_live_at?
alt JWT exp still live
store_session->>disk: persist fallback user (pendingBackendValidation=true)
else JWT expired
store_session-->>OAuth: Err (no persist)
end
else 401 / non-transient
auth_me_GET-->>fetch_current_user_for_session_store: auth error
fetch_current_user_for_session_store-->>store_session: Err(non-transient)
store_session-->>OAuth: Err (no persist)
end
OAuth-->>snapshot: snapshot() call
snapshot->>snapshot: detect pending_backend_validation
alt pending flag present
snapshot->>backend_refresh: timeout-wrapped fetch
alt 200 OK with user
backend_refresh-->>snapshot: user
snapshot->>disk: persist revalidated session, clear flag
snapshot-->>OAuth: authenticated with refreshed user
else 200 OK empty
backend_refresh-->>snapshot: empty
snapshot->>disk: clear deferred session, sign out
snapshot-->>OAuth: unauthenticated
else timeout or error
snapshot-->>OAuth: fallback to stored user
end
else no pending flag
snapshot-->>OAuth: use cached/fresh user
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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
added
bug
rust-core
![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: 6eef72fff2
ℹ️ 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".
| session_validation_logs.push(format!( | ||
| "session JWT accepted with deferred GET /auth/me validation on {} after transient failure", | ||
| api_url.trim_end_matches('/') | ||
| )); | ||
| fallback_session_user_for_deferred_validation( |
There was a problem hiding this comment.
When this branch persists a session after a transient /auth/me failure, there is no later fail-closed revalidation path: app_state::fetch_current_user treats any non-2xx /auth/me response as Ok(None), and snapshot falls back to the stored user instead of clearing the session. If the token is revoked/forged or the transient 5xx masked a backend-side invalidation while the local exp is still live, subsequent snapshots keep reporting is_authenticated=true with this pending profile until some unrelated backend call happens to fail differently. This deferred path should make the next /auth/me 401/non-success clear or reject the stored session rather than silently falling back.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in follow-up commit 1b30dd8.
Pending app-session snapshots now bypass the current-user cache for revalidation. A successful /auth/me response persists the refreshed user with pendingBackendValidation removed, while a non-success /auth/me response clears the stored app-session profile and returns a signed-out snapshot instead of falling back to the pending stored user. Added coverage in app_state_credentials_raw_coverage_e2e for both success and rejection paths.
![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
🧹 Nitpick comments (1)
src/openhuman/credentials/ops.rs (1)
422-441: ⚡ Quick winConsider logging the retry attempt at debug level.
As per coding guidelines, "Include debug logging with verbose diagnostics on all new/changed flows: log entry/exit, branches, external calls, retries/timeouts, state transitions, and errors." The retry after
tokio::time::sleep(AUTH_ME_STORE_RETRY_DELAY)is not logged. Adding a debug log before the retry would help diagnose intermittent/auth/mefailures.📋 Proposed enhancement
} tokio::time::sleep(AUTH_ME_STORE_RETRY_DELAY).await; + tracing::debug!( + domain = "credentials", + operation = "fetch_current_user_for_session_store", + "[credentials][auth-store] retrying GET /auth/me after transient failure" + ); client .fetch_current_user(token)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/credentials/ops.rs` around lines 422 - 441, The function `fetch_current_user_for_session_store` lacks debug logging for the retry attempt after the sleep delay. When the first call to `client.fetch_current_user(token)` fails with a transient error and the function proceeds to retry after the sleep, add a debug log statement before the second `client.fetch_current_user(token).await` call to log the retry attempt, including relevant context such as the reason for the first failure. This will help with diagnosing intermittent authentication failures.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 `@src/openhuman/credentials/ops.rs`:
- Around line 159-214: In the snapshot refresh logic where
fetch_current_user_cached() is called to revalidate the session, add explicit
handling for the pendingBackendValidation flag: check if the stored session has
this flag set, clear it after successful revalidation via
fetch_current_user_cached(), and if revalidation fails (invalid token or backend
rejection), handle the failure distinctly rather than silently falling back to
the stored user with the flag still persisting. This ensures deferred validation
is actually cleared on the next snapshot refresh as intended, preventing the
flag from remaining indefinitely in stored sessions.
---
Nitpick comments:
In `@src/openhuman/credentials/ops.rs`:
- Around line 422-441: The function `fetch_current_user_for_session_store` lacks
debug logging for the retry attempt after the sleep delay. When the first call
to `client.fetch_current_user(token)` fails with a transient error and the
function proceeds to retry after the sleep, add a debug log statement before the
second `client.fetch_current_user(token).await` call to log the retry attempt,
including relevant context such as the reason for the first failure. This will
help with diagnosing intermittent authentication failures.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2157ecb3-027f-4bd6-9252-ae09d96a008a
📒 Files selected for processing (3)
src/api/jwt.rssrc/openhuman/credentials/ops.rssrc/openhuman/credentials/ops_tests.rs
|
Pushed follow-up commit 1b30dd8 to address the unresolved deferred-session review feedback. What changed:
Validation:
Ready for re-review when CI completes. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/openhuman/app_state/ops.rs (1)
718-724: ⚡ Quick winAdd
req_idto the new revalidation logs.These pending-validation state-transition/error logs can interleave across concurrent snapshots; include the existing
req_idcorrelation field.As per coding guidelines, “Include debug logging with verbose diagnostics on all new/changed flows” with “stable grep-friendly prefixes” and “correlation fields.”
Also applies to: 732-747
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/app_state/ops.rs` around lines 718 - 724, Add the req_id correlation field to the debug and warn log statements related to pending backend validation state transitions. In both the successful path (where debug logs are called after clearing pending backend validation) and the error path (where warn logs are called when persistence fails), include req_id as an additional parameter to the logging macros. This ensures that logs across concurrent snapshots can be properly correlated and traced, following the coding guidelines for grep-friendly prefixes and correlation fields.Source: Coding guidelines
tests/app_state_credentials_raw_coverage_e2e.rs (1)
455-470: ⚡ Quick winAssert the
authfields changed by the snapshot branch.The new snapshot path updates
auth.user_idon successful revalidation and clearsauth.profile_idon rejection, but these tests only covercurrent_user/session-token behavior.Cover the auth enrichment/clearing contract
assert!(snap.auth.is_authenticated); + assert_eq!(snap.auth.user_id.as_deref(), Some("fresh-pending-user")); + assert_eq!( + snap.auth.user.as_ref().and_then(|v| v.get("id")), + Some(&json!("fresh-pending-user")) + ); assert_eq!( snap.session_token.as_deref(), Some("round14.pending.success") ); @@ assert!(snap.auth.user.is_none()); + assert!(snap.auth.user_id.is_none()); + assert!(snap.auth.profile_id.is_none()); assert!(snap.current_user.is_none()); assert!(snap.session_token.is_none());Also applies to: 532-538
🤖 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 `@tests/app_state_credentials_raw_coverage_e2e.rs` around lines 455 - 470, The test assertions in the block starting around line 455 are only checking current_user and session_token fields, but they should also verify that the auth struct fields are properly updated during snapshot revalidation. Add assertions to verify that snap.auth.user_id is set to the expected fresh user identifier on successful revalidation and that snap.auth.profile_id is cleared (is None) on rejection. Apply the same assertions to the second test location mentioned at lines 532-538 to ensure the auth enrichment and clearing contract is fully covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/app_state/ops.rs`:
- Around line 741-750: When handling fetch errors (both the Ok(Err(error)) case
and the Err(_) timeout case in the current user refresh logic), check if the
stored_user has pendingBackendValidation set to true before falling back to
SnapshotCurrentUser::User(stored_user.clone()). If pendingBackendValidation is
true, reject the session and return an appropriate error state instead of
falling back to the unvalidated user, ensuring that unvalidated sessions do not
remain authenticated when the backend is unreachable. Only fall back to
stored_user when it does not have pending validation.
- Around line 395-412: The clear_deferred_session_after_backend_rejection
function clears in-memory auth state only after the profile removal operation
completes successfully, but if the remove_profile call fails, the function
returns early without clearing CURRENT_USER_CACHE and calling
scheduler_gate::set_signed_out. Move the clearing of CURRENT_USER_CACHE and the
crate::openhuman::scheduler_gate::set_signed_out(true) call to execute before
the result check on the spawn_blocking operation, ensuring in-memory auth state
is cleared to a safe state regardless of whether the storage removal succeeds or
fails.
---
Nitpick comments:
In `@src/openhuman/app_state/ops.rs`:
- Around line 718-724: Add the req_id correlation field to the debug and warn
log statements related to pending backend validation state transitions. In both
the successful path (where debug logs are called after clearing pending backend
validation) and the error path (where warn logs are called when persistence
fails), include req_id as an additional parameter to the logging macros. This
ensures that logs across concurrent snapshots can be properly correlated and
traced, following the coding guidelines for grep-friendly prefixes and
correlation fields.
In `@tests/app_state_credentials_raw_coverage_e2e.rs`:
- Around line 455-470: The test assertions in the block starting around line 455
are only checking current_user and session_token fields, but they should also
verify that the auth struct fields are properly updated during snapshot
revalidation. Add assertions to verify that snap.auth.user_id is set to the
expected fresh user identifier on successful revalidation and that
snap.auth.profile_id is cleared (is None) on rejection. Apply the same
assertions to the second test location mentioned at lines 532-538 to ensure the
auth enrichment and clearing contract is fully covered.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a8ba7401-da1e-4ae3-bf58-50d2004893c0
📒 Files selected for processing (3)
src/openhuman/app_state/ops.rssrc/openhuman/credentials/ops.rstests/app_state_credentials_raw_coverage_e2e.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/openhuman/credentials/ops.rs
|
Pushed follow-up commit c250a6c for the two CodeRabbit app_state review threads. What changed:
Validation:
Ready for re-review when CI completes. |
|
@codex review Updated the branch after the latest review/CI pass:
Local validation:
|
There was a problem hiding this comment.
💡 Codex Review
openhuman/src/openhuman/credentials/ops.rs
Line 233 in 0aaef63
Preserve pending marker when storing deferred users
If /auth/me fails transiently and the caller supplied a non-empty user object, settings contains that user plus pendingBackendValidation, but this line stores the original supplied user instead. The next app_state_snapshot therefore does not see the pending marker and will treat the session as fully validated, skipping the fail-closed revalidation/clear path on backend rejection. This affects RPC/CLI callers that pass user details with the token; store the deferred settings value in this branch.
ℹ️ 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".
| AuthService::from_config(&config) | ||
| .remove_profile(APP_SESSION_PROVIDER, DEFAULT_AUTH_PROFILE_NAME) | ||
| .map(|_| ()) |
There was a problem hiding this comment.
When a deferred session included a user id, store_session has already written active_user.toml and started the login-gated services for that user. On a later /auth/me rejection/error/timeout this path only removes the auth profile and flips the scheduler gate, so subsequent config loads still resolve through the rejected user's active directory and services such as voice/autocomplete/screen intelligence remain running while the snapshot reports signed out. Please reuse clear_session semantics here, or mirror its active-user/service/Sentry/subconscious teardown after clearing the in-memory state.
Useful? React with 👍 / 👎.
|
@codex review Addressed the remaining pending-session feedback in the latest commits:
Validation run locally:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2083e8e56
ℹ️ 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".
| fn auth_me_store_failure_is_transient(reason: &str) -> bool { | ||
| if crate::core::observability::contains_transient_transport_phrase(reason) { | ||
| return true; |
There was a problem hiding this comment.
When /auth/me returns a hard auth failure such as 401 but its response body contains one of the transport phrases (for example session timeout), the complete GET /auth/me failed (401 ...): <body> string reaches this early phrase check and is classified as transient. store_session then follows the deferred-persistence path for any locally live exp, so revoked/invalid credentials can be stored instead of fail-closed; parse or prioritize the explicit HTTP status before applying body-wide transport phrase matching.
Useful? React with 👍 / 👎.
| Ok(Ok(None)) if pending_backend_validation => { | ||
| warn!( | ||
| "{LOG_PREFIX} backend rejected pending session revalidation; clearing stored app session" | ||
| ); | ||
| if let Err(error) = clear_deferred_session_after_backend_rejection(&config).await { |
There was a problem hiding this comment.
For a session stored with pendingBackendValidation, a continuing backend outage still returns Ok(None) because fetch_current_user maps every non-2xx /auth/me response, including 500/503, to None; this branch treats that as rejection and clears the profile. That means the first app_state_snapshot during the same transient outage can immediately undo the deferred store and bounce the user back to signed out, so distinguish real auth rejection from transient/non-success responses and keep the pending stored user for transient failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in follow-up commit 7fdd449.
fetch_current_user no longer maps every non-2xx /auth/me response to Ok(None). It now distinguishes:
TransientResponsefor retryable HTTP statuses such as 500/503, which keeps the pending stored user so deferred validation can retry during a backend outage;Rejectedfor hard auth/non-retryable statuses, which clears the pending session;FetchFailedplus the outer timeout path, which still fail closed and use the full pending-session cleanup path.
Regression coverage added in app_state_credentials_raw_coverage_e2e: snapshot_preserves_pending_session_when_backend_revalidation_is_transient verifies the 500 retry-preserve path, while the request-error and timeout tests continue to assert fail-closed clearing.
|
@codex review Follow-up pushed in 7fdd449. What changed:
Local validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7fdd449b24
ℹ️ 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".
| if let Some(user_id) = user_id_from_profile_payload(&user) { | ||
| metadata.insert("user_id".to_string(), user_id); |
There was a problem hiding this comment.
When the transient store-time path persisted a pending session without a resolved user id (for example the deep-link caller passes {} and the JWT only has exp), the profile is stored under the pre-login config. On the later successful /auth/me revalidation, this code only writes the canonical user_id into that same profile and never runs the active-user/user-directory activation path used by store_session, so the snapshot can report an authenticated real user while subsequent config loads and services continue using users/local or a fallback user's workspace.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in follow-up commit c383513.
Pending-session revalidation now activates the backend user id before storing refreshed app-session credentials, reloads the active config via active_user.toml, stores the cleaned profile under that user-scoped config, removes the original pre-login/source pending profile after the active store succeeds, and binds memory/conversation persistence/subconscious/login-gated services against the active config.
Added/updated focused coverage for both pending sessions with an existing user_id and pending sessions with no initial user id.
Validation:
- cargo fmt --manifest-path Cargo.toml --all --check
- git diff --check
- cargo test --manifest-path Cargo.toml -p openhuman --lib auth_me_transient -- --nocapture
- cargo test --manifest-path Cargo.toml --test app_state_credentials_raw_coverage_e2e -- --nocapture
- pre-push hook passed: format:check, lint warnings only, compile, rust:check, lint:commands-tokens
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5dcc842d20
ℹ️ 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".
| if let Ok(root_dir) = crate::openhuman::config::default_root_openhuman_dir() { | ||
| if let Err(error) = crate::openhuman::config::clear_active_user(&root_dir) { |
There was a problem hiding this comment.
When OPENHUMAN_WORKSPACE scopes the snapshot to a throwaway/debug config, a rejected pending session removes the profile from that env-scoped config above but this block still clears the default ~/.openhuman/active_user.toml. The env-scoped success path in this file avoids moving default auth state, so this rejection path can sign the real desktop user out after a failed revalidation in an env-scoped core/test; skip clearing active_user.toml when config_is_workspace_env_scoped(config) or only clear it when this pending session actually activated the default user.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in follow-up commit 7aee83f. Rejected pending-session cleanup now removes the env-scoped pending profile but only clears default active_user.toml when the default active user matches that pending session user id. This preserves an unrelated desktop active user while keeping the supplied-user rejection cleanup path intact. Validation: cargo fmt --manifest-path Cargo.toml --all --check, git diff --check, cargo test --manifest-path Cargo.toml --test app_state_credentials_raw_coverage_e2e -- --nocapture, and cargo test --manifest-path Cargo.toml -p openhuman --lib auth_me_transient -- --nocapture.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30273b6180
ℹ️ 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".
| if let Err(error) = crate::openhuman::subconscious::global::bootstrap_after_login().await { | ||
| warn!("{LOG_PREFIX} subconscious bootstrap failed after pending session revalidation: {error}"); | ||
| } | ||
| crate::openhuman::credentials::start_login_gated_services(config).await; |
There was a problem hiding this comment.
When a deferred session is created, store_session has already called start_login_gated_services(&effective_config) after persisting the pending profile; a later successful pending revalidation reaches this new call and starts the same services again without stopping/rebinding them. This is observable on Windows/Linux with dictation enabled because voice::dictation_listener::start_if_enabled creates a new rdev listener and only replaces the stored task handle, leaving the old listener alive, so a transient /auth/me followed by successful revalidation can duplicate hotkey events until logout.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in follow-up commit 7ffdebc. Successful pending revalidation no longer restarts login-gated services when the auth profile remains in the same config, avoiding duplicate same-process listeners. If revalidation moves the profile from a source config into a newly activated user config, it now stops source-scoped services before starting them once under the target config. Validation: cargo fmt --manifest-path Cargo.toml --all --check, git diff --check, cargo test --manifest-path Cargo.toml --test app_state_credentials_raw_coverage_e2e -- --nocapture, and cargo test --manifest-path Cargo.toml -p openhuman --lib auth_me_transient -- --nocapture.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7aee83f28e
ℹ️ 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".
| let source_config = config.clone(); | ||
| let source_moved = user_id.is_some() && !same_config_state_dir(config, &target_config); | ||
| let token = token.to_string(); | ||
| let mut metadata: HashMap<String, String> = base_metadata.into_iter().collect(); |
There was a problem hiding this comment.
When a pending session is revalidated by any non-empty /auth/me payload that lacks an id, this copies the original base_metadata and then only overwrites user_id when user_id_from_profile_payload(&user) succeeds. In that scenario a callback/JWT-derived user_id from the unvalidated pending profile remains in metadata while user_json has had pendingBackendValidation removed, so future snapshots treat that unconfirmed identity as fully validated. For pending revalidation, either require a backend user id before clearing the marker or remove the stale user_id when the fresh payload does not supply one.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ffdebce1c
ℹ️ 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".
| Config::load_from_default_paths() | ||
| .await |
There was a problem hiding this comment.
When revalidating a pending session for a different account, this reload still runs even if the create_dir_all or write_active_user_id call above failed. In that case load_from_default_paths() resolves the previous active user (or pre-login) config, and persist_revalidated_session_user then writes the newly validated JWT/user metadata into the wrong auth store and may overwrite/remove the source profile. Please return an error or keep the original config unless active_user.toml was successfully written.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fbf6cc595
ℹ️ 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".
| )); | ||
| } | ||
|
|
||
| let Some(exp) = jwt_exp_live_at(trimmed_token, chrono::Utc::now()) else { |
There was a problem hiding this comment.
When /auth/me returns a transient status or transport error, this branch treats any token with a decodable future exp as sufficient to persist the app session. decode_jwt_exp is explicitly only a payload decode and does not verify the signature, so a corrupted/forged JWT with a future exp will be stored, reported as authenticated, and can start login-gated services until a later snapshot rejects it. That breaks the intended fail-closed behavior for invalid credentials during backend degradation; defer only tokens whose authenticity was established, or keep the session unauthenticated until revalidation succeeds.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 33229029ce
ℹ️ 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".
| "[credentials][auth-store] GET /auth/me transient validation failed on {} — session NOT persisted; backend proof required before storing remote JWT: {reason}", | ||
| api_url.trim_end_matches('/') | ||
| ); | ||
| return Err(format!( |
There was a problem hiding this comment.
When /auth/me still fails with a transient status after the retry, this branch returns before reaching AuthService::store_provider_token, so no profile with pendingBackendValidation is ever written for the snapshot revalidation path to recover. In the desktop/deep-link login flow the caller only has the JWT plus an empty callback user, so a valid unexpired JWT still bounces the user back to sign-in whenever the store-time /auth/me gets a 5xx/timeout, instead of being persisted for deferred validation.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21f30c96d5
ℹ️ 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".
| } | ||
| let fresh_user = clear_pending_backend_validation_flag(fresh_user); | ||
| if pending_backend_validation { | ||
| match persist_revalidated_session_user( |
There was a problem hiding this comment.
When a deferred session was stored without a user id, this call can activate a newly revalidated user directory and write the profile there, but the surrounding snapshot continues using the config that was loaded before the switch for local_state, onboarding flags, runtime config, and keyring policy. In the first successful revalidation after a transient /auth/me, React can therefore commit the pre-login or previous user's state (including onboarding/local-state values such as the stored encryption key) for the newly authenticated user until a later refresh corrects it; return/reload the target config from the activation path before building the rest of the snapshot.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8dda53746b
ℹ️ 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".
| "session JWT accepted with deferred GET /auth/me validation on {} after transient failure", | ||
| api_url.trim_end_matches('/') | ||
| )); | ||
| fallback_session_user_for_deferred_validation() |
There was a problem hiding this comment.
When /auth/me is transient while a user is already signed in, this fallback user has no id, so resolved_user_id stays None and the later effective_config remains the currently active user's config. The pending callback token is then stored as app-session:default, overwriting that user's valid session; if revalidation later rejects or keeps failing, the existing user is signed out/left with an unvalidated token even though the new login never proved an identity. Defer in an isolated/pre-login profile or require a resolved user id before replacing the active session.
Useful? React with 👍 / 👎.
# Conflicts: # src/openhuman/agentbox/disabled_mode_tests.rs # src/openhuman/agentbox/env_tests.rs # src/openhuman/agentbox/status.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 007a090e05
ℹ️ 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".
| Ok(Err(CurrentUserFetchError::FetchFailed(error))) if pending_backend_validation => { | ||
| warn!( | ||
| "{LOG_PREFIX} pending current user refresh failed before a backend response; keeping stored pending session for retry: {error}" | ||
| ); | ||
| snapshot_current_user_result(stored_user.clone()) |
There was a problem hiding this comment.
When a pending session's /auth/me revalidation keeps hitting transport failures or retryable backend statuses after the JWT exp recorded at store time has passed, this fallback still returns the stored pending user and leaves the profile/token in place for another retry. Because deferred validation is only trusted while the local JWT is live, these pending retry branches should fail closed or clear once session_expires_at is expired; otherwise a backend outage can keep an expired, never-revalidated session authenticated indefinitely.
Useful? React with 👍 / 👎.
| let target_config = if !workspace_env_scoped { | ||
| activate_revalidated_user_dir(&user_id).await? | ||
| } else { |
There was a problem hiding this comment.
When a no-id pending session successfully revalidates in a non-env-scoped config, this activates active_user.toml before the new app-session profile is written. If the subsequent store_provider_token fails due to the auth-profile lock, keychain/encryption, or a filesystem write error, the caller falls back to the old pending user for this snapshot, but future config loads now resolve to the newly activated user directory with no session profile, hiding the source pending profile and losing the retry path.
Useful? React with 👍 / 👎.
| if let Err(error) = remove_revalidated_source_profile(&source_config).await { | ||
| warn!( | ||
| "{LOG_PREFIX} failed to remove source pending session profile after user activation: {error}" | ||
| ); |
There was a problem hiding this comment.
When source_moved is true and removing the pre-login/source pending profile fails, this still completes revalidation successfully with two app-session profiles on disk. A later logout only clears the active user-scoped profile and active_user.toml; the stale source profile then becomes visible again on the next snapshot and can revalidate the user back in, so this cleanup failure should either abort/roll back or leave a durable tombstone/retry instead of being only a warning.
Useful? React with 👍 / 👎.
2 participants
Summary
GET /auth/mevalidation once for transient failures before deciding whether to persist the app session./auth/mestill fails transiently but the session JWT has a live localexp, persist the session withpendingBackendValidation: trueso the next snapshot can recover./auth/mefailures still reject the session and leave auth state unauthenticated.Problem
store_sessionvalidates the session JWT viaGET /auth/mebefore writing the app-session profile.Solution
/auth/mefailures at session-store time.expbefore deferring backend validation, so malformed/expired tokens are not persisted just because/auth/meis degraded.pendingBackendValidation.Submission Checklist
## Related— N/A: no matrix feature ID changed./auth/meserver.docs/RELEASE-MANUAL-SMOKE.md) — N/A: no release-cut checklist surface changed.Closes #NNNin the## RelatedsectionImpact
/auth/metimeout or gateway failure during sign-in no longer bounces the user back to signin when the JWT is locally live.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
fix/auth-store-session-transient-me6eef72fff2d1fe64ec32a7ab99ab5ed32db4861bValidation Run
pnpm --filter openhuman-app format:check— passed via pre-push hook;cargo fmt --manifest-path Cargo.toml --all --checkalso passed locally.pnpm typecheck— passed.cargo test --manifest-path Cargo.toml openhuman::credentials::ops::tests -- --nocapture— 41 passed.cargo fmt --manifest-path Cargo.toml --all --check;cargo check --manifest-path Cargo.toml; pre-pushpnpm --filter openhuman-app rust:check— passed with existing warnings.app/src-taurisource changes; pre-push Taurirust:checkstill passed with existing warnings.Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
/auth/mestore-time failures can defer backend validation when the JWT has a live localexp.Parity Contract
/auth/mehard-fail behavior are preserved.pendingBackendValidation.Duplicate / Superseded PR Handling
Summary by CodeRabbit
auth/mefailures and deferring revalidation when the JWTexpis still live.expparsing from integer or float claims (no signature verification).