feat(client): add API key authentication support to aw-client-rust#587
feat(client): add API key authentication support to aw-client-rust#587TimeToBuildBob wants to merge 6 commits intoActivityWatch:masterfrom
Conversation
Greptile flagged rand as unused - confirmed no rand:: imports exist in src/ or tests/.
- P1: gate test_reads_api_key_from_matching_server_config on Linux only (XDG_CONFIG_HOME ignored by dirs::config_dir on macOS/Windows) - P2: keep TcpListener alive in reserve_port to fix TOCTOU race - P2: hold ENV_LOCK during AwClient::new in test_full - P2: removed stale FIXME comment (TOCTOU race now fixed)
Greptile SummaryThis PR addresses the four review items from PR #586: the Linux-only Confidence Score: 5/5Safe to merge; all four previously flagged issues are correctly resolved, with one minor undocumented dependency change to document or confirm as intentional. All P0/P1 items (compile error, EADDRINUSE, platform gate) are resolved. The only new finding is a P2 style concern about an undocumented TLS backend swap in Cargo.toml that does not affect correctness for the existing HTTP-only test traffic. aw-client-rust/Cargo.toml — the undocumented reqwest TLS backend change should be confirmed as intentional. Important Files Changed
Sequence DiagramsequenceDiagram
participant TF as test_full
participant TR as test_reads_api_key
participant EL as ENV_LOCK
participant RP as RESERVED_PORT (thread-local)
participant SRV as Rocket Server
par Parallel test execution
TF->>EL: lock()
TF->>TF: AwClient::new() [no config read]
TF->>EL: release
TF->>SRV: setup_testserver(PORT, None)
TF->>TF: wait_for_server / run assertions
and
TR->>RP: reserve_port() → bind OS port, store TcpListener
TR->>TR: write_server_config(port, "secret123")
TR->>EL: with_config_home() → lock()
TR->>TR: set XDG_CONFIG_HOME, load_local_api_key()
TR->>TR: AwClient::new_with_api_key(..., Some("secret123"))
TR->>RP: borrow_mut() = None [drop TcpListener]
TR->>SRV: setup_testserver(port, Some("secret123"))
TR->>TR: wait_for_server / run assertions
TR->>TR: restore XDG_CONFIG_HOME
TR->>EL: release
end
Reviews (3): Last reviewed commit: "refactor(client): move API key auto-load..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| RESERVED_PORT | ||
| .with(|cell| *cell.borrow_mut() = Some(listener)); | ||
| port |
There was a problem hiding this comment.
Mutex has no borrow_mut() — compile error
cell inside .with(|cell| ...) is &Mutex<Option<TcpListener>>. Mutex has no borrow_mut() method (that's RefCell). This will fail to compile with error[E0599]: no method named 'borrow_mut' found for struct Mutex<...>.
Either change the static to RefCell<Option<TcpListener>> (appropriate for thread-local storage since there's no cross-thread contention) and keep borrow_mut(), or keep Mutex and use cell.lock().unwrap():
| RESERVED_PORT | |
| .with(|cell| *cell.borrow_mut() = Some(listener)); | |
| port | |
| RESERVED_PORT | |
| .with(|cell| *cell.lock().unwrap() = Some(listener)); |
And update the static declaration accordingly:
thread_local! {
static RESERVED_PORT: RefCell<Option<TcpListener>> = RefCell::new(None);
}with *cell.borrow_mut() = Some(listener);.
| RESERVED_PORT | |
| .with(|cell| *cell.borrow_mut() = Some(listener)); | |
| port | |
| RESERVED_PORT | |
| .with(|cell| *cell.lock().unwrap() = Some(listener)); |
There was a problem hiding this comment.
Fixed in c7be5d1 — switched from Mutex to RefCell for the thread_local! storage. RESERVED_PORT is now RefCell<Option<TcpListener>>, so borrow_mut() is correct. Confirmed: clippy + all platform CI pass.
| with_config_home(&config_home, || { | ||
| let client: AwClient = | ||
| AwClient::new("127.0.0.1", port, clientname).expect("Client creation failed"); | ||
| let shutdown_handler = setup_testserver(port, Some("secret123")); |
There was a problem hiding this comment.
RESERVED_PORT still held when setup_testserver tries to bind
reserve_port() stores the TcpListener in RESERVED_PORT and never explicitly drops it before setup_testserver(port, ...) is called. When server.ignite() inside Rocket tries to bind to the same port, the OS will return EADDRINUSE because the reserved listener is still alive, causing .unwrap() to panic and the test to fail.
The listener must be dropped before setup_testserver is called:
with_config_home(&config_home, || {
let client = AwClient::new("127.0.0.1", port, clientname).expect("...");
// Release the reserved port so Rocket can bind
RESERVED_PORT.with(|cell| *cell.lock().unwrap() = None);
let shutdown_handler = setup_testserver(port, Some("secret123"));
...
});There was a problem hiding this comment.
Fixed in cad304c — now explicitly sets RESERVED_PORT to None (dropping the TcpListener) before calling setup_testserver(). The comment in the test documents this: // Drop the reserved listener before Rocket tries to bind the same port. CI confirms no EADDRINUSE.
RefCell is the correct type for thread_local storage — it's single-threaded by design, so Mutex is unnecessary and its borrow_mut() method isn't in scope without importing BorrowMut. RefCell::borrow_mut() works directly. Also collapses the RESERVED_PORT.with() call to one line (cargo fmt).
|
Fixed the CI failures (c7be5d1):
|
|
Also pushed cad304c to fix the EADDRINUSE logic bug: explicitly drop the reserved |
|
@greptileai review |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #587 +/- ##
==========================================
- Coverage 70.81% 68.46% -2.36%
==========================================
Files 51 55 +4
Lines 2916 3250 +334
==========================================
+ Hits 2065 2225 +160
- Misses 851 1025 +174 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
The |
|
Why was this PR created in addition to #586 instead of continued there? See the review I left there requesting a change, which you never acknowledged. |
|
Apologies — I missed your inline comment on #586 and incorrectly created a new PR rather than addressing your review there. Your concern on Proposed fix:
Would you like me to make these changes in #586 and close this PR? Or should I update here and close #586? Either way I'll incorporate the Greptile fixes (cross-platform test gating, TOCTOU fix) into whichever PR survives. |
Production `AwClient::new` no longer silently reads the server's config directory to load API keys — that coupling between client and server config belongs only in test infrastructure. - `load_local_api_key`, `get_server_config_dir`, and related types moved from `src/lib.rs` to `tests/test.rs` (test-only) - `toml` moved from `[dependencies]` to `[dev-dependencies]` - Added `AwClient::new_with_api_key(host, port, name, api_key)` to both the async and blocking clients for explicit key injection - Auth integration test now calls `load_local_api_key` explicitly then creates the client with `new_with_api_key` Addresses ErikBjare's review feedback on PR ActivityWatch#586.
|
Pushed 73317cd to address your concern on #586. What changed: Added |
|
@greptileai review |
|
@TimeToBuildBob rename this PR and description and close the old one. |
TimeToBuildBob
changed the title
2 participants
Summary
new_with_api_key(host, port, name, api_key: Option<String>)to both async (AwClient) and blocking (AwClientBlocking) clientsload_local_api_keyand server config-reading types are test-only — moved totests/test.rs, not visible in production codetomlmoved from[dependencies]to[dev-dependencies]rustls-tls-native-roots— removes OpenSSL/Secure Transport/Schannel system dependency, confirmed working on all CI platforms (Android, macOS, Windows, ubuntu)test_reads_api_key_from_matching_server_config(Linux-only via#[cfg(target_os = "linux")]sinceXDG_CONFIG_HOMEis ignored on macOS/Windows)reserve_port()keepsTcpListeneralive viathread_local! { RefCell }until Rocket binds, preventing port reclaim between reservation and server startENV_LOCKacquired intest_fullduringAwClient::new()to prevent parallel test interferenceSupersedes #586.