feat: refactor CLI onto the hotdata Rust SDK#134
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}
| query: &[(&str, String)], | ||
| ) -> Result<T, ApiError> { | ||
| let cfg = self.client.configuration(); | ||
| let url = format!("{}/v1{path}", cfg.base_path); |
There was a problem hiding this comment.
nit: (not blocking) get_json, post_raw, delete_raw, and get_bytes each duplicate the same ~30-line header-attachment block (user_agent, X-Workspace-Id, X-Session-Id, X-Database-Id, bearer). Adding or changing a scope header later means editing four call sites in lockstep, which is easy to get subtly out of sync. Consider extracting a helper that takes the RequestBuilder and applies the common seam headers, e.g.:
fn apply_seam_headers(&self, mut req: reqwest::RequestBuilder, cfg: &Configuration, bearer: Option<String>) -> reqwest::RequestBuilder { ... }so each verb method just builds its method/body/accept and calls it. Purely a maintainability cleanup — the current code is correct and well-tested.
There was a problem hiding this comment.
Reviewed the full SDK-refactor diff (sdk.rs seam, jwt CliTokenProvider, raw_http, and all 15 migrated command modules). The seam's auth-source precedence, scope-header behavior, and the documented intentional deltas (dropped X-Sandbox-Id, no-op --connection, no blanket scope over-send) all check out, and the test coverage is thorough (base-path double-/v1 regression, concurrent rayon block_on, per-module wire assertions). CI test job is green. One non-blocking maintainability nit left inline on the duplicated raw-helper header blocks.
|
Addressed the duplicated-header-block nit in d5aeaca: the four raw seam helpers ( |
| // The wrapper is wired into command modules incrementally (one commit per | ||
| // module). Until every call site is migrated, parts of this seam are unused; | ||
| // the allow keeps the build warning-free through the transition and is removed | ||
| // once api.rs is retired. | ||
| #![allow(dead_code)] |
There was a problem hiding this comment.
super nit: (not blocking) This PR deletes api.rs outright, so the "transition" these comments describe is now complete — the #![allow(dead_code)] is described as something to "[remove] once api.rs is retired," which happens right here. Worth a follow-up pass to drop the module-level allow (and the matching ones on AuthMode/CliTokenProvider in jwt.rs) and clean up anything that's genuinely unused now that the legacy client is gone. Leaving a blanket dead_code allow in place can mask real dead code later. Purely cleanup — no behavior impact.
There was a problem hiding this comment.
Reviewed the SDK refactor. Auth seam (CliTokenProvider/ensure_access_token), the sync block_on wrapper, scope-header handling, and the raw escape-hatch helpers all look correct and are backed by thorough, meaningful tests (mockito wire assertions, concurrent rayon block_on, double-/v1 regression, refresh/mint decision table). Prior header-duplication nit is resolved via apply_seam_headers. One non-blocking cleanup nit left inline.
|
Addressed the stale |
There was a problem hiding this comment.
Reviewed the full SDK-refactor diff across all 25 files. The seam (src/sdk.rs), the CliTokenProvider auth bridge, and each migrated command module are correct, consistent, and well-tested — scope-header narrowing, the double-/v1 guard, concurrent block_on from rayon, and the Option<Option<T>> model flattening all have dedicated coverage. The prior review's header-dedup nit is resolved via apply_seam_headers, and the dead_code allows are now either gone (api.rs deleted) or freshly justified (incremental raw_http migration).
Note: I could not run cargo check/tests in this environment, so build verification rests on manual review.
There was a problem hiding this comment.
Reviewed the SDK seam, auth provider, raw-HTTP helper, and command-module adaptations. Auth precedence, header scoping, Arrow decoding, and the rayon concurrency contract are all correct and thoroughly tested. The apply_seam_headers dedup from the prior review is already in place. No blocking issues.
|
Extended the request-differential parity gate to near-total coverage: 103 command/flag combinations driven through both the Result: GATE PASS on both passes — 0 unexpected divergences, 0 regressions. Every delta is one of the three documented intended changes (dead Harness + full report (secret gist): https://gist.github.com/zfarrell/4e231496dcd51e1afd00570f699096e0 |
1 participant
Replaces the CLI's hand-rolled
reqwest::blockingHTTP layer with the hotdata Rust SDK (rev8d4018f, hotdata-dev/sdk-rust#32), behind a synchronousblock_onseam (src/sdk.rs) so the command modules stay sync. Auth keeps the CLI's PKCE login +session.json+client_id=hotdata-cli; aCliTokenProviderfeeds the SDK the minted JWT.Behavior changes (verified against
runtimedb+ the gateway):X-Sandbox-Id(unused in runtimedb and monopoly post-migration) and the old blanket over-send ofX-Database-Id/X-Session-Idto endpoints that ignore them (e.g. the query-runs poll).X-Workspace-Id(gateway ext-auth),X-Session-Id(datasets/indexes/query/results), andX-Database-Id(/query, dataset create) are preserved where consumed.User-Agent: hotdata-cli/<v>andclient_id=hotdata-cliinstead of the SDK defaults.querysubmits via the SDK'ssubmit_query; the ignoredconnection_idbody field is no longer sent (/queryis database-scoped), soquery --connectionis now a confirmed no-op (Remove/deprecate the dead 'query --connection' flag #132).Verified with a request-differential gate (old
mainbinary vs this branch): every API call is wire-identical except the intended deltas above.Follow-ups: #129 scenario tests · #130 arrow→get_result_arrow · #131 raw→typed · #132 dead --connection · #133 upload→upload_stream (needs SDK Content-Length first).