Add sandbox JWT support#78
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| pub fn refresh_from_env(api_url: &str) -> Option<String> { | ||
| let current = std::env::var("HOTDATA_SANDBOX_TOKEN").ok()?; | ||
| let needs_refresh = match jwt_exp(¤t) { | ||
| Some(exp) => exp.saturating_sub(REFRESH_LEEWAY_SECONDS) <= now_unix(), | ||
| None => true, | ||
| }; | ||
| if !needs_refresh { | ||
| return Some(current); | ||
| } | ||
| let rt = std::env::var("HOTDATA_SANDBOX_REFRESH_TOKEN").ok()?; | ||
| if rt.is_empty() { | ||
| return Some(current); | ||
| } | ||
| match refresh(api_url, &rt) { | ||
| Ok(new_session) => Some(new_session.access_token), | ||
| Err(_) => Some(current), | ||
| } | ||
| } |
There was a problem hiding this comment.
nit: (not blocking) The "HOTDATA_SANDBOX_TOKEN is empty" error in api.rs:69 is effectively unreachable. std::env::var(...).is_ok() is true when the var is set to an empty string, but refresh_from_env happily returns Some("") for that case (line 207's ? only triggers on NotPresent, not empty). The downstream effect is the client issues Authorization: Bearer and the user sees a confusing 401 instead of the intended message.
A short guard at the top of refresh_from_env would tighten this up:
let current = std::env::var("HOTDATA_SANDBOX_TOKEN").ok()?;
if current.is_empty() {
return None;
}This also matches the early-empty check already in sandbox_token_in_use (line 189).
| "json" => println!("{}", serde_json::json!({"public_id": sandbox_id})), | ||
| "yaml" => print!( | ||
| "{}", | ||
| serde_yaml::to_string(&serde_json::json!({"public_id": sandbox_id})).unwrap() | ||
| ), |
There was a problem hiding this comment.
nit: (not blocking) The output shape from sandbox new --output json / --output yaml regresses here — previously it was a full Sandbox object (public_id, name, markdown, created_at, updated_at), now it's just {"public_id": "..."}. Any caller doing hotdata sandbox new -o json | jq .name (or the timestamps) will break silently.
If /auth/sandbox can't return the full object, a follow-up api.get(&format!("/sandboxes/{sandbox_id}")) to render the existing shape would preserve the contract. The table branch on line 207 has the same issue — name echoes the flag rather than the server's stored name, and created_at/updated_at are gone.
| // Register before `Cli::parse`, since `--help` / `--version` exit | ||
| // from inside the parser. Safety: `atexit` is async-signal-safe; | ||
| // the callback only reads env vars / files and writes to stderr. | ||
| unsafe { atexit(print_sandbox_footer) }; |
There was a problem hiding this comment.
super nit: (not blocking) The async-signal-safety claim is misleading — atexit callbacks aren't invoked from signal context, so async-signal-safety isn't the relevant property here. And the callback as written isn't AS-safe anyway (eprintln! takes a lock + allocates, and fs::read_to_string allocates and syscalls).
What's actually being justified is that (a) the function pointer is valid for the process lifetime and (b) the callback is extern "C" and won't unwind into C. A short rewrite, e.g. "Safety: callback is extern "C" and never unwinds; lives for the program's lifetime" would be more accurate.
1 participant
No description provided.