fix(sdk): scale the token refresh lead to the token lifetime

[itzlambda]

The problem

The pre-emptive refresh window was a fixed 60 minutes:

/// Check if the token needs refresh (expires within 60 minutes)
pub fn needs_refresh(&self) -> bool {
    self.expires_within(Duration::from_secs(60 * 60))
}

That works while access tokens live a lot longer than an hour. Staging's Auth0 API token lifetime has just moved from 86,400s to 3,600s, and at that point the rule degenerates: a token minted one second ago already "expires within 60 minutes", so needs_refresh() is true for a token's entire life.

basilica-cli/src/client.rs calls this on every command, so the CLI hits Auth0's token endpoint on every single invocation — ps, ssh, exec, all of them.

Observed on staging after the lifetime change: a token issued at 07:47:19 had already been silently replaced by 07:48:21, about a minute later, with no user action in between.

The same fixed hour appears a second time as TokenManager::REFRESH_THRESHOLD in simple_manager.rs, so the SDK and the CLI each carried their own copy of the value.

The fix

Derive the window from the token instead of hardcoding it. The lifetime is available from the token's own claims (exp - iat):

fn refresh_lead(&self) -> Duration {
    match self.lifetime() {
        Some(lifetime) => MAX_REFRESH_LEAD.min(lifetime / 2),
        None => FALLBACK_REFRESH_LEAD,
    }
}

Half the lifetime, capped at the previous hour.

Token lifetime	Lead	Effect
1 hour (staging today)	30 min	first half hour quiet, then renews
24 hours (production today)	60 min	unchanged from before
15 min (if ever adopted)	7.5 min	scales down correctly
no iat	5 min	fallback

The cap is what makes this safe to land: a 24-hour token still gets exactly the 60-minute lead it had before, so this is not a behaviour change for any lifetime currently in use in production. It only fixes the case the fixed value got wrong.

The fallback matters too. With no iat there is no lifetime to measure, and the tempting default is the old 3,600s — which would quietly reintroduce the bug for any short-lived token lacking the claim. Five minutes is the same buffer token_store.rs already uses, and it fails toward "refresh a bit late" rather than "refresh constantly".

TokenManager::should_refresh now defers to TokenSet and its duplicate constant is gone, so both paths renew on one schedule.

Why this matters beyond the noise

It also unblocks tightening the lifetime further. A 900s access token is currently not viable: under the fixed hour it would refresh on every command forever. With the lead derived, 900s yields a 450s lead and behaves correctly. That decision is out of scope here, but this is the change that makes it possible.

Tests

This module had no tests at all, so is_expired, expires_within and the JWT decoding were entirely unguarded. Seven added, in pairs — a refresh threshold has two failure directions, and pinning only one of them lets "never refresh" pass as a fix:

• a_freshly_minted_short_token_does_not_need_refresh — the actual bug; fails against the previous code
• a_short_token_past_halfway_needs_refresh — its counterweight, so the fix cannot be "never renew"
• lead_is_half_the_lifetime_for_a_short_token — pins the arithmetic directly
• long_lived_tokens_keep_the_one_hour_lead + a_long_token_inside_the_last_hour_needs_refresh — enforce the backward-compatibility claim above, which is otherwise just an assertion in a PR description
• without_iat_the_lead_falls_back_to_the_small_buffer
• an_expired_token_needs_refresh — boundary where unsigned arithmetic on a negative remaining time would go wrong

All green, plus the full suites: 212 SDK, 89 CLI. Clippy and fmt clean.

Scope

Deliberately only the timing. Two related issues found in the same investigation are not addressed here:

• simple_manager::get_access_token propagates a failed refresh with ? instead of falling back to the still-valid cached token, unlike client.rs which warns and continues.
• A revoked session surfaces as Internal server error: Failed to get access token: Network error: Token refresh failed — three inaccuracies in one line, and no hint to run basilica login.

Both are error-handling rather than timing, and the second is already improved by the AuthenticationErrorKind work in the all-sessions-logout PR.

Merge order

This should go in before the all-sessions-logout PR. That branch also touches the SDK auth module, and landing the smaller, self-contained change first keeps the two diffs from tangling.

Summary by CodeRabbit

• Bug Fixes
  • Improved authentication token refresh timing based on each token’s actual lifetime.
  • Added safer handling for short-lived, long-lived, expired, and tokens missing issuance information.
  • Prevented unnecessary early refreshes while maintaining timely renewal.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 7d028708-cbef-4b37-9df6-620cb7583f38

📥 Commits

Reviewing files that changed from the base of the PR and between 1c3bb97 and c5c2c3a.

📒 Files selected for processing (2)

• crates/basilica-sdk/src/auth/simple_manager.rs
• crates/basilica-sdk/src/auth/types.rs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.

---

Walkthrough

Token refresh scheduling now derives its refresh window from JWT lifetime data. TokenSet handles expiry and refresh checks, while SimpleManager delegates both decisions. Tests cover short-lived, long-lived, missing-iat, and expired tokens.

Changes

Token refresh scheduling

Layer / File(s)	Summary
Calculate token-specific refresh windows crates/basilica-sdk/src/auth/types.rs	JWT numeric claims support lifetime calculation. The refresh lead uses half the token lifetime, caps at one hour, and falls back to five minutes when iat is unavailable.
Apply refresh windows to token checks crates/basilica-sdk/src/auth/types.rs, crates/basilica-sdk/src/auth/simple_manager.rs	TokenSet uses the calculated refresh lead and preserves zero remaining duration for expired tokens. SimpleManager delegates expiry and refresh checks to TokenSet. Unit tests cover the updated behavior.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to c5c2c

The change scales token refresh timing to token lifetime while preserving the existing behavior for long-lived tokens; no actionable merge-blocking risk remains.

Poem

I’m a rabbit with tokens tucked tight,
Refresh windows now follow their flight.
Short lives turn sooner, long lives wait,
Missing iat gets five minutes’ gate.
Expired ones rest at zero—goodnight!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: token refresh lead time now scales with token lifetime.
Docstring Coverage	✅ Passed	Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Create stacked PR
• Commit on current branch

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/cli-refresh-threshold

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[covenant-talos]

Walkthrough

This PR replaces the SDK's fixed 60-minute pre-emptive token refresh window with a value derived from each token's own lifetime (exp - iat), so short-lived access tokens stop refreshing on every CLI invocation. The refresh lead is now half the token lifetime, capped at one hour (preserving prior behavior for 24-hour production tokens), with a five-minute fallback when iat is absent. The duplicate REFRESH_THRESHOLD constant in TokenManager is removed so both the SDK and CLI renew on a single schedule, and seven new unit tests guard the previously untested TokenSet expiry and refresh logic.

Changes

Cohort / File(s)	Change Summary
Refresh-lead derivation / crates/basilica-sdk/src/auth/types.rs	Generalizes JWT claim decoding to read iat alongside exp, adds lifetime() and refresh_lead() (half lifetime, capped at one hour, 5-min fallback), rewrites needs_refresh() to use the derived lead, and introduces seven unit tests covering short/long/no-iat/expired cases.
Manager unification / crates/basilica-sdk/src/auth/simple_manager.rs	Removes the duplicate REFRESH_THRESHOLD constant and simplifies should_refresh to delegate to TokenSet::is_expired and TokenSet::needs_refresh.

Sequence Diagram(s)

This change alters refresh decision timing but does not wire a new multi-component flow, so a sequence diagram would add nothing.

Estimated review effort: 2/5 (small, self-contained timing fix with clear tests and well-documented rationale).

Instant overview - a deep technical review follows as a separate comment.

[covenant-talos]

PR #554: fix(sdk): scale the token refresh lead to the token lifetime

Summary

The PR replaces the hardcoded 60-minute pre-emptive token refresh lead with one derived from the token's own claims: min(lifetime / 2, 60min) where lifetime = exp - iat, falling back to 5 minutes when iat is absent. This fixes a real regression: with staging's new 1-hour token lifetime, the old fixed 1-hour lead was satisfied the moment a token was minted, so every CLI invocation (ps, ssh, exec) triggered an Auth0 token refresh. The cap preserves the exact previous behavior for production's 24-hour tokens, and TokenManager::should_refresh now defers to TokenSet::needs_refresh, eliminating the duplicated REFRESH_THRESHOLD constant so SDK and CLI share one schedule. Seven unit tests are added to a previously untested module, including one that demonstrably fails against the old code.

Architecture

No structural impact. The change is local to the SDK auth module (types.rs, simple_manager.rs); it consolidates refresh-window policy inside TokenSet rather than altering any flow — both existing call sites (client.rs, TokenManager) already consulted TokenSet expiry helpers.

Issues Found

CRITICAL Issues (Must Fix Before Merge)

None found.

HIGH Severity Issues (Advised to Fix Before Merge)

None found.

MEDIUM Severity Issues (Optional to Fix Before Merge)

None found.

LOW Severity Issues (Minor Improvements)

1. Capture the clock once per test to avoid a second-boundary flake
   Testing & Docs | LOW | Effort: quick win
   • Why: lead_is_half_the_lifetime_for_a_short_token calls now() twice to build the token — token_with(now(), now() + 3600) (types.rs, test module). If the wall clock ticks over a second boundary between the two calls, the measured lifetime is 3599s, refresh_lead() becomes 1799.5s, and the exact assert_eq!(t.refresh_lead(), Duration::from_secs(1800)) fails. The probability per run is tiny (microseconds-wide window), but CI accumulates runs, and this is the module's first-ever test suite — a spurious failure here would erode trust in it. The other tests are robust: the long-token exact assertion is protected by the MAX_REFRESH_LEAD cap, and the boolean assertions have hundreds of seconds of margin.
   • How: Bind the clock once and reuse it:

         #[test]
         fn lead_is_half_the_lifetime_for_a_short_token() {
             // Capture the clock once so a second-boundary tick between the two
             // claims cannot shrink the measured lifetime by a second.
             let start = now();
             let t = token_with(start, start + 3600);
             assert_eq!(t.refresh_lead(), Duration::from_secs(1800));
         }
     

     Applying the same let start = now(); pattern to the remaining tests is optional but keeps the module uniform.

Security Review

Surface swept: trust boundaries, input validation, secret handling, injection, dependencies, DoS.

• JWT parsing without signature verification: decode_jwt_claim base64-decodes and JSON-parses the payload without verifying the signature. This is a pre-existing pattern (the old decode_jwt_exp did the same) and is used solely as a local timing hint for refresh scheduling, never as an authentication/authorization decision — the token is still sent to the API, which validates it. Acceptable in this context.
• Malformed input handling: every parse step returns Option (split length check, .ok()? on decode and JSON parse, as_u64()), and lifetime() uses checked_sub, so a crafted or corrupt token in the local store cannot panic or underflow — it degrades to the fallback lead. Worst-case attacker outcome with write access to the local token store is altered refresh timing, which is low impact.
• Injection surface: the claim parameter is a JSON key lookup via serde_json::Value::get with two hardcoded call sites ("exp", "iat") — no injection path.
• Dependencies/supply chain: no changes; base64 and serde_json were already in use.
• Resource exhaustion: the change strictly reduces calls to Auth0's token endpoint in the affected configuration, removing a self-inflicted rate-limit risk.

No security findings.

Suggestions for Improvements

• Share the 5-minute fallback buffer. The PR description states FALLBACK_REFRESH_LEAD mirrors a buffer token_store.rs already uses (that file is not in this diff, so I cannot verify). If so, consider a single pub(crate) constant in the auth module to keep the two from drifting — the same duplication this PR just eliminated for the 60-minute value.
• Decode the payload once per check. needs_refresh() currently triggers three full base64+JSON parses of the same payload (two in lifetime() via separate decode_jwt_claim calls, one in expires_within() via get_expiration()). The cost is negligible at CLI-invocation frequency, but a small private helper returning (exp, iat) from one decode would be tidier if this path ever gets hotter.

Positive Observations

• Correct root-cause fix rather than a staging-specific workaround; the MAX_REFRESH_LEAD cap makes the backward-compatibility claim ("24-hour tokens behave exactly as before") true by construction, and two tests pin it so it isn't just an assertion in the PR description.
• Tests are paired across both failure directions (a_freshly_minted_short_token_does_not_need_refresh vs a_short_token_past_halfway_needs_refresh), which prevents "never refresh" from passing as a fix — good test design.
• The fallback choice is well-reasoned: defaulting to the old 3600s for iat-less tokens would silently reintroduce the bug for short-lived tokens; 5 minutes fails toward refreshing slightly late instead of constantly.
• exp.checked_sub(iat) correctly handles the iat > exp degenerate case without a panic path.
• Good DRY/coherency: the duplicate TokenManager::REFRESH_THRESHOLD is removed and both renewal paths now share one schedule; doc comments were updated to match the new semantics.
• Tests need no crypto — only the payload is read, so header/signature are inert placeholders, which is honest and sufficient.
• Scope discipline: the two related error-handling issues are explicitly documented as out of scope with rationale, rather than being silently bundled in.

Commit standards: the PR title fix(sdk): scale the token refresh lead to the token lifetime conforms to Conventional Commits. Individual commit messages were not visible in the provided material, so only the title could be verified.

Recommendation and Next Steps

APPROVE — the fix is correct, backward-compatible for all token lifetimes currently in production, well-tested against both failure directions, and carries no security or stability concerns; the single LOW item (a rare second-boundary test flake) is a quick follow-up and does not block merge.