Skip to content

refactor: migrate teams/roles/workspaces/artifacts to ApiClient#78

Merged
shadowhand merged 18 commits intorefactor-add-jsonapi-http-prompt-scaffoldingfrom
refactor-migrate-resource-commands-to-apiclient
Apr 28, 2026
Merged

refactor: migrate teams/roles/workspaces/artifacts to ApiClient#78
shadowhand merged 18 commits intorefactor-add-jsonapi-http-prompt-scaffoldingfrom
refactor-migrate-resource-commands-to-apiclient

Conversation

@shadowhand
Copy link
Copy Markdown
Contributor

Summary

Stacked on #77.

Migrates the four read-mostly resource commands to the new ApiClient and jsonapi envelopes. Per-resource *ListResponse / *Resource types are deleted; commands deserialize directly into jsonapi::List<TeamAttributes> / Single<…> etc.

Net: +55 / −212 LOC across 5 files.

Before / after

// Before — repeated in every command
let auth_header = super::auth::require_auth(ctx).await?;
let client = Client::new();
let url = format!("{}/teams", ctx.base_url.trim_end_matches('/'));
let resp = client.get(&url).header(AUTHORIZATION, &auth_header).send().await
    .context("GET /teams failed")?;
let status = resp.status();
let body = resp.text().await.context("failed to read response body")?;
if !status.is_success() { bail!("API returned {}: {}", status, body); }
let list: TeamListResponse = serde_json::from_str(&body)
    .context("failed to parse teams response")?;
// After
let api = ApiClient::new(ctx).await?;
let resp: List<TeamAttributes> = api.get("teams").await?;

Files changed

  • commands/teams.rslist / show migrated; TeamListResponse / TeamResource deleted; TeamAttributes retained as pub(crate) for cross-module use.
  • commands/roles.rslist / show migrated. resolve_role (still called by commands::clinicians) keeps its raw-reqwest signature but now parses via List<RoleAttributes>. Full ApiClient migration of resolve_role happens when clinicians moves over.
  • commands/workspaces.rslist / show migrated; per-resource types deleted.
  • commands/artifacts.rslist migrated using api.get_query(…, &[("filter[type]", …), …]); per-resource types deleted.
  • commands/clinicians.rs — single import / type swap to keep using teams.

Stack ahead

  1. Promote to lib.rs + integration tests
  2. Split commands/clinicians.rs into a module
  3. Migrate clinicians (and roles::resolve_role) to ApiClient
  4. Split commands/config.rs into a module
  5. Move dispatch into per-subcommand modules
  6. Extract Stage + slug to a domain module
  7. CI hardening

Test plan

  • cargo fmt --check clean
  • cargo clippy --all-targets — no new warnings (11 pre-existing, identical set as before)
  • cargo test — 277 pass (no test additions; pure code-replacement refactor)

https://claude.ai/code/session_012fVDyKBysF1ERtvVuEAw2Y


Generated by Claude Code

claude added 2 commits April 28, 2026 01:35
Replaces the ~10-line auth+url+send+status+parse boilerplate in every
public command with a single `ApiClient::new(ctx).await?` followed by a
typed `api.get(path).await?` (or `get_query`). Per-resource
`*ListResponse` / `*Resource` structs are gone — commands now deserialize
directly into `jsonapi::List<TeamAttributes>` etc.

- `teams::list` / `teams::show` use ApiClient.
- `roles::list` / `roles::show` use ApiClient. `roles::resolve_role`
  retains its raw-reqwest signature for `commands::clinicians`; will
  fold into the ApiClient migration when clinicians moves over.
- `workspaces::list` / `workspaces::show` use ApiClient.
- `artifacts::list` uses ApiClient with `get_query` for the JSON:API
  filter params.
- `commands::clinicians::resolve_team` updated to deserialize via
  `List<TeamAttributes>` instead of the deleted `TeamListResponse`.

Net: -157 LOC across four files; behavior unchanged; 277 tests still
pass. Pre-existing clippy warnings unchanged.
Splits the crate into:

- `src/lib.rs` — `pub mod` declarations for every top-level module,
  giving integration tests (and any future external consumers) access
  to internals.
- `src/main.rs` — thin binary that imports from `rw::*`. The dispatch
  shell stays here for now; PR7 will move it into per-subcommand
  modules.

Adds `tests/cli.rs` with 11 end-to-end tests using `assert_cmd` +
`tempfile`. They cover argument parsing, error reporting for invalid
input, and the basic dispatch paths the unit tests can't reach.

The new test cache is seeded with the current `CARGO_PKG_VERSION` so
`version_check::check_and_update` short-circuits on cache hit and
never makes a live GitHub HEAD request during tests.

Removes the `#![allow(dead_code)]` attributes on `http`, `jsonapi`, and
`prompt` — once the modules are `pub` in the library crate, dead-code
analysis no longer fires on the unused-internally items.

Test counts:
- Before: 277 unit
- After:  270 lib + 7 bin + 11 integration = 288 total
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 2 additional findings.

Open in Devin Review

devin-ai-integration[bot]

This comment was marked as resolved.

claude added 6 commits April 28, 2026 01:47
The file was 2985 lines (largest in the crate). Now 11 files in
`commands/clinicians/`, each focused on one concern:

- `mod.rs`           — module wiring + re-exports
- `data.rs`          — JSON:API deserialization types (Clinician*)
- `output.rs`        — output types (Prepare/Grant/Assign/etc.)
- `client.rs`        — legacy reqwest helpers shared across commands
                       (resolve_team, fetch_clinician_*, apply_auth, etc.)
- `testing.rs`       — `#[cfg(test)]` shared fixtures (TestAuthGuard,
                       *_response builders) used by every command's tests
- `assign.rs`        — `pub fn assign` + `patch_clinician_team` + tests
- `enable.rs`        — `pub fn enable` / `disable` + `set_enabled` + tests
- `grant.rs`         — `pub fn grant` + `patch_clinician_role` + tests
- `prepare.rs`       — `pub fn prepare` + workspace helpers + tests
- `register.rs`      — `pub fn register` + tests
- `show.rs`          — `pub fn show` + tests
- `update.rs`        — `pub fn update` + validation + tests

No behavior change. All 288 tests still pass (270 lib + 7 bin + 11
integration). Largest remaining file is `prepare.rs` at 832 lines, of
which most is the elaborate multi-mock test setup; `update.rs` is 557
for the same reason. Production code per file is now well under 200
lines each.

The cross-command helpers in `client.rs` retain their raw-reqwest
signatures rather than taking an `&ApiClient`. They will fold into
the ApiClient migration in the next PR.
Folds the clinician-side raw-reqwest helpers into ApiClient calls and
switches every command to deserialize via `jsonapi::Single` /
`jsonapi::List`. Removes 347 lines of HTTP/parse boilerplate.

Per-file changes:

- `data.rs`: dropped `ClinicianResource` / `ClinicianListResponse` /
  `ClinicianSingleResponse`; added a `Clinician` type alias for
  `Resource<ClinicianAttributes>`.
- `client.rs` (177 → 75 LOC): every helper now takes `&ApiClient`
  instead of `(&Client, &str, &str)`. `apply_auth` deleted (ApiClient
  attaches auth itself).
- `commands::roles::resolve_role`: signature changed to
  `(&ApiClient, &str)`. The "legacy helper" docstring is removed.
- `grant.rs`, `assign.rs`, `enable.rs`, `show.rs`, `update.rs`,
  `register.rs`, `prepare.rs`: each command now opens with
  `let api = ApiClient::new(ctx).await?;` and uses `api.get/patch/post`
  in place of the inline reqwest pipeline. Per-command
  `patch_clinician_*` helpers deleted; bodies inlined as
  `serde_json::json!()` literals at the call site.
- `prepare.rs`: workspace POSTs now use `api.post_void`.

288 tests still pass (270 lib + 7 bin + 11 integration), unchanged.
The file was 1424 lines with three orthogonal subcommand groups
(profile, updates, default) and five interactive prompt helpers all
mixed together. Now five files under commands/config/:

- mod.rs (15)         — module wiring + re-exports
- prompts.rs (97)     — local prompt_* helpers; will be replaced by
                        crate::prompt in a follow-up
- profile.rs (779)    — list/show/use/set/rm/add/auth + their output
                        types + tests
- updates.rs (126)    — show/enable/disable + output types + tests
- default.rs (474)    — set/get/list/rm + key validation + output
                        types + tests

profile.rs is the largest by virtue of having seven subcommands and
the bulk of the test code. updates.rs and default.rs are now sized
appropriately for their scope.

288 tests still pass (270 lib + 7 bin + 11 integration), unchanged.
Each top-level subcommand module now exposes a `dispatch` function
that owns the inner match for its `*Commands` enum. main.rs becomes a
thin shell that maps `Commands` variants to those.

main.rs::run is now a flat 11-arm match (was a deeply nested ~230-line
ladder). build_ctx is still called per-arm — different subcommands need
slightly different argument shapes so a uniform dispatcher signature
isn't worth the awkward AppContext-or-config plumbing.

`commands::*::dispatch` signatures:

- Most: `(args, &AppContext, &Output)` — auth, api, artifacts, clinicians,
  roles, teams, workspaces
- `update::dispatch(out)` — no args, no ctx
- `skills::dispatch(args, out)` — args, no ctx
- `config::dispatch(args, &mut Config, &cfg_path, &config_dir,
  profile_override, out)` — needs Config, not AppContext

main.rs: 464 → 302 lines (-162). 288 tests still pass.
Pulls domain types out of cli.rs (which now is pure clap parsing)
and consolidates the WorkOS configuration that lived in
commands/auth.rs onto Stage as a method.

- src/domain/stage.rs: Stage enum + `Stage::api_url(org)` (replaces
  api.rs::resolve_api as a free function — kept as a thin facade for
  backwards compat) + `Stage::workos_config()` (replaces the consts
  + match in commands/auth.rs).
- src/domain/slug.rs: validate_slug now uses a compiled regex via
  LazyLock instead of the hand-rolled char-by-char loop. The regex
  crate is already a dependency.
- cli.rs: re-exports `Stage` and `validate_slug` from `crate::domain`
  so existing call sites keep working without churn.
- commands/auth.rs: WorkOsConfig + PRODUCTION/DEV consts deleted;
  workos_config() is now a one-line forward to `stage.workos_config()`.
- commands/config/profile.rs: switched from local prompts.rs to
  `crate::prompt as p`. prompts.rs is deleted (97 LOC dropped).

Net: domain types now have a single home, cli.rs is 70 LOC slimmer,
auth.rs's WorkOS plumbing collapsed, and the duplicated
config/prompts.rs is gone.

Test counts: 273 lib (+3 from new stage tests) + 7 bin + 11 integration
= 291. No behavior change.
- Clippy now runs with --all-targets --all-features and -D warnings,
  so any new warning fails CI.
- New release-build job catches release-only compile/link issues
  (LTO, codegen-units, etc.) before they hit a tag.
- New audit job runs cargo-audit against the lockfile to surface
  known-vulnerable transitive dependencies.

Fixes the 10 pre-existing clippy warnings that accumulated across
the codebase so the strict lint passes:

- src/commands/api.rs: useless format!() in a test fixture
- src/commands/clinicians/prepare.rs: setup_prepare_mocks_by_uuid
  has 10 args by design (mock-construction helper); allowed locally
- src/commands/config/default.rs: map_or(true, ...) → is_none_or(...)
  (3 occurrences in tests)
- src/commands/skills.rs: needless &Path::path() borrows in tests
- src/config.rs, src/version_check.rs: field-reassign-with-default
  in three test fixtures, replaced with struct-update syntax

CONTRIBUTING.md: documented the strict clippy invocation and the
release build command so local checks match CI.

288 → 291 tests still pass (no test changes; the +3 came in PR8 from
Stage::workos_config tests).
claude and others added 3 commits April 28, 2026 02:18
The new cargo-audit job in CI surfaced three vulnerabilities in
transitive dependencies. All three resolve to semver-compatible
patch versions reachable via `cargo update`:

- rustls-webpki 0.103.10 → 0.103.13
  - RUSTSEC-2026-0098: name constraints for URI names
  - RUSTSEC-2026-0099: name constraints accepted for wildcard certs
  - RUSTSEC-2026-0104: reachable panic in CRL parsing
- rand 0.9.2 → 0.9.4
  - RUSTSEC-2026-0097: unsoundness with custom logger via rand::rng()
- fastrand 2.4.0 → 2.4.1 (yanked → published)

No source changes; only Cargo.lock. All 291 tests still pass.
ci: harden lint, add release build and audit jobs
The generic `Resource<A, R = ()>` introduced in #77 silently regressed
deserialization: the previous hand-rolled `*Resource` types had no
`relationships` field at all, so serde skipped it; the new generic
declares it explicitly as `R` and defaults `R` to `()`, which only
deserializes from null or absent.

Real JSON:API responses for clinicians, teams, and others carry
populated relationships objects (e.g. `{"team": {"data": {...}}}`).
Against the live API those calls would have failed at runtime with
`invalid type: map, expected unit`. Test mocks all omitted the
relationships key, so the regression slipped through CI.

Switching the default to `serde_json::Value` accepts any JSON shape
on the field — null, object, array, primitive, or absent — and
callers that don't read it can ignore it. Callers that need typed
relationships still pass an explicit `R` (see test_typed_relationships
and the existing call sites for `prepare`'s WorkspaceAttrs etc.,
which use named structs and are unaffected).

Adds three regression tests covering null, empty object, and populated
object cases — the third would have caught this had it been there.

Spotted by Devin AI in review of #78.
…fault

fix(jsonapi): default Resource relationships to serde_json::Value
refactor: extract Stage + slug into a domain module
…subcommands

refactor: move dispatch into per-subcommand modules
refactor: split commands/config.rs into a module
…to-apiclient

refactor: migrate commands::clinicians to ApiClient
@shadowhand shadowhand merged commit de7503e into refactor-add-jsonapi-http-prompt-scaffolding Apr 28, 2026
4 checks passed
@shadowhand shadowhand deleted the refactor-migrate-resource-commands-to-apiclient branch April 28, 2026 03:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants