feat(client): auto-load API key for aw-client-rust#586
feat(client): auto-load API key for aw-client-rust#586TimeToBuildBob wants to merge 2 commits intoActivityWatch:masterfrom
Conversation
Greptile SummaryThis PR adds automatic API key loading in
Confidence Score: 4/5Safe to merge on Linux-only CI, but the new integration test will fail on macOS and Windows runners due to the XDG_CONFIG_HOME-only config redirect. The core client logic in lib.rs is correct and well-implemented. The single P1 finding is confined to the test harness: aw-client-rust/tests/test.rs — platform portability of Important Files Changed
Sequence DiagramsequenceDiagram
participant App
participant AwClient
participant ConfigFS as Config File (config.toml)
participant ReqwestClient
participant Server
App->>AwClient: new(host, port, name)
AwClient->>ConfigFS: load_local_api_key(host, port)
Note over AwClient,ConfigFS: Only for 127.0.0.1 / localhost
ConfigFS-->>AwClient: Option<api_key>
AwClient->>ReqwestClient: build_client(api_key)
Note over ReqwestClient: Sets default_headers<br/>Authorization: Bearer <key><br/>(marked sensitive)
ReqwestClient-->>AwClient: reqwest::Client
AwClient-->>App: AwClient
App->>AwClient: get_bucket / create_bucket / etc.
AwClient->>Server: HTTP request + Authorization header (if key found)
Server->>Server: ApiKeyCheck fairing validates Bearer token
Server-->>AwClient: 200 OK (or 401 if key missing/wrong)
AwClient-->>App: Result
Reviews (1): Last reviewed commit: "feat(client): auto-load local API keys f..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| fn with_config_home<T>(config_home: &Path, f: impl FnOnce() -> T) -> T { | ||
| let _guard = ENV_LOCK.lock().unwrap(); | ||
| let old_value = std::env::var_os("XDG_CONFIG_HOME"); | ||
| std::env::set_var("XDG_CONFIG_HOME", config_home); | ||
| let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(f)); | ||
| if let Some(old_value) = old_value { | ||
| std::env::set_var("XDG_CONFIG_HOME", old_value); | ||
| } else { | ||
| std::env::remove_var("XDG_CONFIG_HOME"); | ||
| } | ||
| let _ = fs::remove_dir_all(config_home); | ||
| result.unwrap() | ||
| } |
There was a problem hiding this comment.
with_config_home only works on Linux
std::env::set_var("XDG_CONFIG_HOME", ...) is only respected by dirs::config_dir() on Linux. On macOS it returns $HOME/Library/Application Support, and on Windows it returns %APPDATA% — both ignore XDG_CONFIG_HOME. As a result, load_local_api_key in the new test reads the real user config directory (finding nothing), returns None, the client sends no Authorization header, and the server (configured with "secret123") rejects every authenticated request with 401. The test will fail on any macOS or Windows CI runner.
A portable fix is to use a platform-specific env override or to expose a AwClient::new_with_config_dir constructor for testing, or to use #[cfg(target_os = "linux")] to gate the test.
| fn reserve_port() -> u16 { | ||
| TcpListener::bind("127.0.0.1:0") | ||
| .unwrap() | ||
| .local_addr() | ||
| .unwrap() | ||
| .port() | ||
| } |
There was a problem hiding this comment.
reserve_port
The TcpListener is dropped immediately after .port() is called, releasing the OS-assigned port before the test server tries to bind it. Another process (or another parallel test) can claim the same port in that window, causing a flaky bind failure in setup_testserver. The idiomatic fix is to keep the listener alive until the server binds, or to pass the already-bound TcpListener directly to the server instead of a port number.
| @@ -16,9 +16,10 @@ tokio = { version = "1.28.2", features = ["rt"] } | |||
| rand = "0.9" | |||
There was a problem hiding this comment.
rand dependency
rand = "0.9" is listed under [dependencies] but is not imported or used anywhere in src/ or tests/. reserve_port() uses TcpListener::bind("127.0.0.1:0") instead. If this was added by this PR, it should be removed to avoid bloating compile times and binary size.
| fn test_full() { | ||
| let clientname = "aw-client-rust-test"; | ||
|
|
||
| let client: AwClient = | ||
| AwClient::new("127.0.0.1", PORT, clientname).expect("Client creation failed"); | ||
|
|
There was a problem hiding this comment.
test_full doesn't hold ENV_LOCK during client construction
AwClient::new calls load_local_api_key, which reads XDG_CONFIG_HOME via dirs::config_dir(). If test_full and test_reads_api_key_from_matching_server_config run in parallel, the env-var mutation in with_config_home is visible inside test_full's load_local_api_key call without synchronisation. In the current setup this doesn't cause wrong behavior (the temp config only holds the reserved port, not 41293), but adding ENV_LOCK acquisition here would make the intent consistent and guard against future port collisions.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #586 +/- ##
==========================================
- Coverage 70.81% 68.65% -2.16%
==========================================
Files 51 55 +4
Lines 2916 3267 +351
==========================================
+ Hits 2065 2243 +178
- Misses 851 1024 +173 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Greptile flagged rand as unused - confirmed no rand:: imports exist in src/ or tests/.
|
Addressed Greptile's findings:
|
| default_port: 5600, | ||
| }, | ||
| ConfigCandidate { | ||
| filename: "config-testing.toml", |
There was a problem hiding this comment.
What? It should only read this in testing mode...
2 participants
Summary
api_keyfrom the localaw-server-rustconfig when connecting tolocalhostor127.0.0.1Authorization: Bearer ...header for authenticated requestsNotes
rustls-tls-native-rootsinstead of the native-tls default, which removes the OpenSSL system dependency while keeping HTTPS supportTesting
cargo test -p aw-client-rust