Migrate screen commands to v4.1 API and improve error messages

[514sid]

Moves all screen operations from v3/v4 to the v4.1 API and improves how API errors are reported to the user.

Changes

Screen API migration (v3/v4 → v4.1)

• screen list and screen get — updated from v4/screens to v4.1/screens
• screen add — replaced the manual v3 POST (which returned a single object and required wrapping it in an array) with commands::post against v4.1/screens, which returns an array natively
• screen delete — replaced DELETE /v3/screens/<id>/ with PostgREST filter syntax DELETE /v4.1/screens?id=eq.<id>, consistent with how other v4.1 endpoints work
• MCP screen tools updated to match

Error message improvements

• Added ApiError(String) variant to CommandError for cases where the API returns a human-readable message in the response body
• commands::post now parses the error field from non-2xx JSON responses and returns ApiError instead of a bare status code — so screen add with a wrong pin now shows "Invalid pin" instead of "unexpected response status: 400"
• Renamed WrongResponseStatus message from "unknown error: {N}" to "unexpected response status: {N}" for clarity
• The catch-all error handler now uses Display formatting instead of Debug, so users see the #[error("...")] message rather than the internal Rust variant name

[sergey-borovkov]

Review — Migrate screen commands to v4.1 API and improve error messages

Overview

Switches screen list/get/add/delete (plus MCP screen tools) from v3/v4 to v4.1 endpoints, and surfaces server-supplied error messages from commands::post via a new CommandError::ApiError(String) variant. Net +49/−36 across 5 files. Tests are updated and a new test covers the ApiError path.

Strengths

• Endpoint migration is consistent across CLI, MCP tools, and tests — no stragglers on the screen path.
• screen add now reuses commands::post instead of an open-coded reqwest call, eliminating duplicated POST plumbing and the v3 array-wrapping hack.
• commands::post switches from response.text()? to response.text().unwrap_or_default() on the error path. This is an actual bug fix: previously, if reading the error body failed, the request error masked the original status error.
• The ApiError test (test_add_screen_wrong_pin_should_fail_with_api_error_message) is tight and verifies the user-visible string, not just the variant.

Issues / Suggestions

1. ApiError parsing is only in post (src/commands/mod.rs:180–189)
get, delete, and patch all still return WrongResponseStatus(N) on non-2xx — so a wrong-pin equivalent through a different verb (e.g. a future screen update via patch, or a v4.1 delete that 400s) will still show unexpected response status: 400. If the goal is users see the API's error message, consider lifting the JSON-error extraction into a small helper and calling it from all four functions. Even better: also fall back to message/detail keys, since not every API surface uses error.

2. Inconsistent Debug → Display change (src/cli.rs:512)
Only the one catch-all site changed from {e:?} to {e}. The other ~14 error!(\"Error occurred: {e:?}\") sites in cli.rs (lines 582, 674, 815, 850, 861, 876, 887, 901, 1028, …) still use Debug. If the rationale is users shouldn't see Rust variant names, this should be applied uniformly — otherwise behaviour depends on which command path you hit. Either sweep them all or add a comment explaining why this one site differs.

3. Subtle behaviour change in screen add acceptance (src/commands/screen.rs:42)
The old open-coded path required exactly 201 CREATED. commands::post accepts 201, 200, or 204. If v4.1 ever returns 200 for a duplicate-or-similar case, screen add will now silently succeed where it used to error. Likely fine for PostgREST conventions (returns 201 for inserts), but worth confirming against the v4.1 spec.

4. Delete test only asserts URL shape (src/commands/screen.rs:160–166)
The new delete test verifies the v4.1 path/query syntax but doesn't add an assert! against the mock or assert success of the result. Minor — existing pattern probably covers this elsewhere — but a delete_mock.assert(); assert!(result.is_ok()); would lock the behaviour.

Risk Assessment

• Backwards compatibility: v3 screen add returned a single object; v4.1 returns an array. The PR handles this correctly and the public Screens wrapper hides the shape from callers. Anyone scripting against --json output would see no change (was already wrapped in an array).
• Error format change: Users who grep CLI stderr for unknown error: 400 will break. Low risk but worth a note in release notes.
• Security: No new auth surface or input-validation concerns.

Verdict

Solid migration with a real UX improvement. Main thing I'd push back on is the inconsistency: error-message extraction lives only in post, and the Debug→Display switch hit only 1 of 15 sites. Either of those is approvable on its own; together they make the error message UX half of the PR feel half-done. Worth either expanding the scope or splitting into a follow-up issue.

[514sid]

@sergey-borovkov

1. Applied to get, delete, and patch.
2. Swept all remaining {e:?} sites in cli.rs (11 total) to {e}.
3. No action needed.
4. Added delete_mock.assert() and assert!(result.is_ok()).

[sergey-borovkov]

Nice cleanup, and good error-message coverage. Two small things before merge:

1. The description says v4.1/screens "returns an array natively" — can you confirm that's what the live endpoint sends on POST (not an empty 201)? commands::post does serde_json::from_str(&body) on 201, so an empty body would make screen add fail after the screen is actually created. If you've already run it against staging, a one-line note is enough. If there's any doubt, a test that add handles a 201/empty body without erroring would lock the contract down.
2. Minor: the *,screens_configs(*),… select string is copy-pasted across the CLI and MCP call sites — worth a shared const so they can't drift.

[514sid]

Thanks @sergey-borovkov , both done. Heads up though, poking at the live endpoint turned up a real bug, so this isn't just the two follow-ups.

On the empty-body thing, I went defensive instead of trusting the shape. post now treats an empty 201/200 as null rather than failing the parse, so screen add can't blow up after the screen's already created. Added a test. While I was there I checked what the endpoint actually sends back, and POST /v4.1/screens returns a bare {"id": "<ulid>"}, not an array and not the full screen (it's RETURN json_build_object('id', ...) in the backend's functions.sql). The old add test mocked a full-screen array, which passed but wasn't realistic, so I switched it to mock {"id": ...}.

Pulled the select string into a shared SCREEN_SELECT const for the CLI and MCP sites like you suggested.

The bigger one: wiring that const up, I ran screen list against prod and got a 400. The nested resource names were pluralized (screens_configs, screens_pings, and so on), but the real relationships are singular: screen_configs, screen_pings, screen_reports, screen_statuses. select=* on its own is fine (200), any screens_* embed 400s with an empty body, so all anyone saw was unexpected response status: 400. It broke list, get, and delete (delete does a name-lookup GET first). The backend confirms the singular names, both the nginx v4 routes and their own test_screens.py use them. Fixed the const and the mock assertions, and verified list plus a full add/delete cycle work end-to-end against prod now.

[sergey-borovkov]

Overview

Migrates screen list/get/add/delete (CLI + MCP) from v3/v4 to the v4.1 PostgREST API, and makes HTTP error bodies surface as human-readable messages via a new CommandError::ApiError variant.

I checked out the branch locally: cargo fmt --check is clean and cargo test passes (209 passed, 0 failed).

What's good

• SCREEN_SELECT as a shared pub const is the right call — the select string can't drift between CLI and MCP.
• api_error_from_body is small, pure, and directly unit-tested for all three key names plus both fallback paths.
• Replacing println!("Response: {:?}", …) in get() with debug! removes stray stdout noise that would corrupt --output json/csv piping.
• Swapping response.text()? for .unwrap_or_default() on error paths fixes a real latent bug: a body-read failure previously masked the actual status code.
• No code anywhere matches on CommandError::WrongResponseStatus(_) for control flow, so widening those paths to ApiError is safe.

Issues

1. screen add now prints an empty table (high)

The PR's own test pins the new response shape as a bare object ({"id": "01KY…"}), and Screens::new stores it as-is. But format_value only iterates when the value is an array:

if let Some(values) = value.value().as_array() {   // src/commands/mod.rs:60, :80

I verified this against the branch by formatting {"id": "…"} through Screens:

+----+------+---------+----------+------------------+---------+-----------+--------+
| Id | Name | Enabled | Priority | Hardware Version | In Sync | Last Ping | Uptime |
+----+------+---------+----------+------------------+---------+-----------+--------+

Header-only table, header-only CSV. The deleted v3 code wrapped the object in an array precisely for this reason ("Our newer endpoints all return arrays so let's just convert the output"). --output json also changes from [{…full screen…}] to {"id": "…"}, which breaks any script doing .[0].id.

Fix: re-wrap in an array, and — since v4.1 returns only the id — consider following with a get(id) so the table still shows Name/Enabled/etc. as it did before. Either way, test_add_screen_should_send_correct_request should assert the formatted output, not just v.value, so this class of regression is caught.

2. screen delete can report success without deleting anything (medium)

DELETE /v4.1/screens?id=eq.<id> is a PostgREST filtered delete: it returns 204 when zero rows match — nonexistent id, wrong team, or RLS denying the write. commands::delete only checks the status, so the CLI prints "Screen deleted successfully." The old DELETE /v3/screens/<id>/ returned 404/403 in those cases.

The CLI path is partly shielded by get_screen_name running first, but that's a TOCTOU-ish guard and doesn't cover RLS-denied deletes. Consider sending Prefer: return=representation (or count=exact) on delete and treating an empty result as an error.

3. Nested selects are fetched but never used; fixtures still v4-shaped (medium)

SCREEN_SELECT pulls screen_configs, screen_pings, screen_reports, screen_statuses for every screen, but the Screens formatter still reads hardware_version, in_sync, last_ping, uptime, priority from the top level. If v4.1 moved any of those into the nested resources (which the presence of the select hints at), the table silently degrades to N/A/❌ — and the tests can't catch it because every fixture still uses the flat v4 payload plus the old "config": {…} blob.

Worth confirming against a real v4.1 response and updating at least one fixture to the actual shape. If the nested data genuinely isn't needed for the table, dropping it from the select would also cut a fair amount of payload on large fleets.

4. Smaller points

• {e:?} → {e} drops detail for two variants. Authentication(#[from] AuthenticationError) is #[error("auth error")] and OpenBrowserError(String) is #[error("Failed to open browser")] — both discard the inner value. In handle_cli_screen_command/asset/edge_app, which have no Authentication special case, users now see a bare auth error. One-line fix: #[error("auth error: {0}")].
• ApiError drops the status code. A 500 with {"message": "boom"} now renders as just boom. Including the status (ApiError(u16, String)) would help triage without costing readability.
• get() lost its body dump. It used to always print the error body; now a body that matches none of error/message/detail produces only unexpected response status: 400, visible only under debug logging. A truncated-body fallback in api_error_from_body would preserve diagnosability.
• id isn't URL-encoded in v4.1/screens?id=eq.{id} (get and now delete). Pre-existing for get, newly applicable to delete. Not exploitable into a mass delete — PostgREST ANDs duplicate filters, so the id=eq. predicate can't be dropped — but encoding it is cheap correctness.
• PR description understates the scope: api_error_from_body is wired into get/delete/patch as well as post, so every command's error messages change, not just screen add.

Verdict

The error-message work is solid and well-tested. Issue #1 is a user-visible regression in screen add that I'd want fixed before merge; #2 and #3 depend on real v4.1 behavior worth confirming with the API team.