From 2b149f4468776fb5fef6756644f5414c054ac182 Mon Sep 17 00:00:00 2001 From: Julien Danjou Date: Tue, 5 May 2026 13:16:03 +0200 Subject: [PATCH] docs(port): add port review checklist for HTTP/test/UX parity pitfalls MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 2026.5.5.x release shipped a ``ci scopes-send`` regression (fixed in #1356): the Rust port deserialized the response body via ``let _: serde_json::Value = client.post(...)`` while the Python original ignored the body, and the Mergify endpoint returns an empty body on success. ``Value`` still requires valid JSON, so the Rust version surfaced ``parse response JSON: error decoding response body`` to every user once it shipped. Test mocks used ``set_body_json(json!({}))`` instead of an empty body, so the bug hid through unit tests too. The fix is structural — split the HTTP client so callers pick ``post`` (parses) vs ``post_no_response`` (drops) up front — but the bug class is broader than this one command. Capturing the lessons in AGENTS.md so the next porter (human or assistant) walks through them before opening a PR. Three categories of pitfall, all observed during the port so far: 1. **HTTP response handling** — match the Python original. If Python ignores the response, Rust must use ``post_no_response`` etc.; if Python parses, optional/nullable fields must be ``Option`` + ``#[serde(default)]``. 2. **Test fixtures** — mirror the real server. ``ResponseTemplate`` bodies must match what the endpoint actually returns; an empty body is not the same as ``json!({})``. 3. **UX parity with click** — char-counted validators use ``chars().count()`` (not ``len()``); confirmation prompts go to stderr; resolve all required state before prompting. Audit of existing/in-flight ports against the checklist: - ``config validate``, ``config simulate`` (main): match - ``ci scopes-send`` (main): regressed, fix in #1356 - ``queue pause`` / ``unpause`` (#1352): match - ``ci git-refs`` / ``queue-info`` (#1353): no HTTP, no risk Co-Authored-By: Claude Opus 4.7 (1M context) Change-Id: I1fd9757e7f0213b0aeecbb6eb52b59e689cdf05a --- AGENTS.md | 44 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 0c863e7f..2dc581e5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -95,6 +95,50 @@ fine when the command is being deprecated/dropped from the CLI — the rule is "no two live copies of the same command", not "every Python copy must be replaced". +### Port review checklist + +Walk through this before opening a port PR. Most regressions caught so +far have been a violation of one of these. + +**HTTP calls — match the Python original's response handling exactly.** + +- If the Python original calls a method on the client and **ignores + the response** (e.g. `await client.post(...)` with no `.json()`), + use `post_no_response` / `delete_if_exists` / equivalent on the + Rust side. Do NOT write `let _: serde_json::Value = client.post(...)` + — `Value` still requires a valid JSON body, and a server that + returns an empty body fails at deserialization with `parse response + JSON: error decoding response body`. This regressed `ci scopes-send` + once (fixed in #1356); the cure is structural — pick the right HTTP + method up-front. +- If the Python original parses the response, keep the same shape on + the Rust side. Fields the Python type-hints as `T | None` (or that + the server has historically tolerated as missing/null) should be + `Option` with `#[serde(default)]` so a missing/null field doesn't + abort deserialization. + +**Test fixtures — mirror the real server, not what's convenient.** + +- Wiremock body templates must match what the real endpoint actually + returns. If the real endpoint sends an empty body, use + `ResponseTemplate::new(200)` — never `set_body_json(json!({}))`. The + latter passes deserialization in tests and masks empty-body bugs + that fail in production. +- Add a regression test for the specific failure mode you're + preventing — exit code, error message, missing-field tolerance — + not just the happy path. + +**Strings & UX parity with click.** + +- Char-bounded validators ("must be ≤ N characters") use + `s.chars().count()`, not `s.len()`. The latter counts UTF-8 bytes + and rejects valid non-ASCII input. +- Confirmation prompts go to stderr, matching click's `confirm` / + `prompt`. Stdout stays pipeable. +- Resolve all required state (repository, token, API URL) before + prompting the user. A missing-token error surfacing after the user + types `y` is bad UX and worse for non-interactive invocations. + ## Documentation When adding or changing a CLI feature, always update the documentation: