docs(port): add port review checklist for HTTP/test/UX parity pitfalls#1357
Merged
mergify[bot] merged 1 commit intomainfrom May 6, 2026
Conversation
Member
Author
|
This pull request is part of a Mergify stack:
|
Contributor
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 🤖 Continuous IntegrationWonderful, this rule succeeded.
🟢 👀 Review RequirementsWonderful, this rule succeeded.
🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
🟢 🔎 ReviewsWonderful, this rule succeeded.
🟢 📕 PR descriptionWonderful, this rule succeeded.
|
jd
force-pushed
the
devs/jd/worktree-rust-port/drop-port-status-toml-inventory-favor-port-delete--77522cb9
branch
from
549a14d to
857f9fc
Compare
jd
force-pushed
the
devs/jd/worktree-rust-port/add-port-review-checklist-http-test-ux-parity--1fd9757e
branch
from
ed2b8b5 to
6b1d62e
Compare
Member
Author
Revision history
|
jd
added a commit
that referenced
this pull request
May 5, 2026
The Rust binary now serves ``mergify queue status`` natively. The Python implementation (``mergify_cli/queue/cli.py:status`` plus the batch/scope/topology helpers it depended on) is removed in the same PR — the port-and-delete rule we adopted in #1322 keeps a single live copy of every command. ## What ports ``mergify queue status [-r REPO] [-t TOKEN] [-u URL] [-b BRANCH] [--json]``: 1. Resolves repository / token / API URL via the shared ``mergify_queue::auth`` resolver introduced in #1352. 2. Fetches ``GET /v1/repos/<repo>/merge-queue/status``, optionally with ``?branch=<branch>`` (URL-encoded via ``url::form_urlencoded::byte_serialize``). 3. With ``--json``: pretty-prints the raw response. The schema is Mergify's API contract, not this CLI's, so we deserialize into ``serde_json::Value`` and emit verbatim — unknown fields and future schema additions survive the round trip. 4. Without ``--json``: deserializes into a typed ``StatusView`` that uses ``#[serde(default)] Option<…>`` for every field the Mergify API has historically treated as optional/nullable (matches the port checklist from #1357), then renders a header, an optional pause indicator, the batch tree (grouped by scope when there is more than one), and the waiting-PR list. Status icons (``● ◑ ◌ ✓ ✗ ◎ ⏳ ↻ ⏰ ❄``) and relative times (``5m ago`` / ``~1h``) match the Python implementation. ## Rendering departures Python used Rich's ``Tree`` for batches. The Rust port emits flat indented text instead — same data, simpler rendering. Both are line-oriented and round-trip cleanly through pipes; the fancy box-drawing was visual sugar that didn't survive piping anyway. ## Tests (24 new, in mergify-queue::status) - ``build_path`` covers no-branch, branch, and URL-encoding of a branch name with slashes + spaces (e.g. ``feature/foo bar`` becomes ``feature%2Ffoo+bar`` in the query). - ``relative_time`` covers seconds / minutes / hours / days, future prefix, and graceful empty-string return on a malformed timestamp (matches Python's "degrade rather than fail"). - ``topological_sort`` covers parents-before-children ordering and tolerance of ``parent_ids`` that reference missing batches. - ``group_by_scope`` covers the ``[]`` → ``"default"`` fallback and multi-scope batches appearing under each scope they claim. - ``status_icon`` covers known + unknown codes. - End-to-end wiremock tests: empty queue, paused queue, batches + waiting PRs, multi-scope grouping, ``?branch=…`` query threading, JSON-passthrough preserving an ``extra_field``, and tolerance of a response that omits all optional fields. ## Wiring - ``crates/mergify-queue/Cargo.toml``: adds ``chrono`` (relative time math), ``indexmap`` (scope groups in insertion order), promotes ``serde_json`` from dev to runtime (used for the ``serde_json::Value`` passthrough). - ``crates/mergify-cli/src/main.rs``: registers the ``status`` subcommand under ``QueueSubcommand``, threads the ``--branch``/``--json`` flags, dispatches to ``mergify_queue::status::run``. Adds ``status`` to ``looks_native``. - ``mergify_cli/queue/api.py``: removes ``QueueStatusResponse``, ``QueueBatch``, ``QueuePause``, ``QueueChecksSummary``, ``QueueBatchStatus``, ``QueuePullRequest``, ``QueuePullRequestAuthor``, and ``get_queue_status`` — all now Rust-native. ``QueuePullResponse`` and friends stay for the still-shimmed ``queue show`` (next phase). - ``mergify_cli/queue/cli.py``: removes the ``@queue.command status`` block and the helpers it owned (``STATUS_STYLES``, ``_status_text``, ``_batch_label``, ``_pr_label``, ``_topological_sort``, ``_group_batches_by_scope``, ``_print_batches``, ``_print_waiting_prs``). ``_relative_time`` stays — ``show`` still uses it. - ``mergify_cli/tests/queue/test_cli.py``: deletes ``TestStatusCommand``, ``TestTopologicalSort``, and the ``_invoke_status`` helper. ``TestRelativeTime`` stays. Workspace: 138 Rust tests green, 590 Python tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Change-Id: I8cebcd325f05173dfa41083da2ec6516a6ec3a3f
jd
added a commit
that referenced
this pull request
May 5, 2026
The Rust binary now serves ``mergify queue status`` natively. The Python implementation (``mergify_cli/queue/cli.py:status`` plus the batch/scope/topology helpers it depended on) is removed in the same PR — the port-and-delete rule we adopted in #1322 keeps a single live copy of every command. ## What ports ``mergify queue status [-r REPO] [-t TOKEN] [-u URL] [-b BRANCH] [--json]``: 1. Resolves repository / token / API URL via the shared ``mergify_queue::auth`` resolver introduced in #1352. 2. Fetches ``GET /v1/repos/<repo>/merge-queue/status``, optionally with ``?branch=<branch>`` (URL-encoded via ``url::form_urlencoded::byte_serialize``). 3. With ``--json``: pretty-prints the raw response. The schema is Mergify's API contract, not this CLI's, so we deserialize into ``serde_json::Value`` and emit verbatim — unknown fields and future schema additions survive the round trip. 4. Without ``--json``: deserializes into a typed ``StatusView`` that uses ``#[serde(default)] Option<…>`` for every field the Mergify API has historically treated as optional/nullable (matches the port checklist from #1357), then renders a header, an optional pause indicator, the batch tree (grouped by scope when there is more than one), and the waiting-PR list. Status icons (``● ◑ ◌ ✓ ✗ ◎ ⏳ ↻ ⏰ ❄``) and relative times (``5m ago`` / ``~1h``) match the Python implementation. ## Rendering departures Python used Rich's ``Tree`` for batches. The Rust port emits flat indented text instead — same data, simpler rendering. Both are line-oriented and round-trip cleanly through pipes; the fancy box-drawing was visual sugar that didn't survive piping anyway. ## Tests (24 new, in mergify-queue::status) - ``build_path`` covers no-branch, branch, and URL-encoding of a branch name with slashes + spaces (e.g. ``feature/foo bar`` becomes ``feature%2Ffoo+bar`` in the query). - ``relative_time`` covers seconds / minutes / hours / days, future prefix, and graceful empty-string return on a malformed timestamp (matches Python's "degrade rather than fail"). - ``topological_sort`` covers parents-before-children ordering and tolerance of ``parent_ids`` that reference missing batches. - ``group_by_scope`` covers the ``[]`` → ``"default"`` fallback and multi-scope batches appearing under each scope they claim. - ``status_icon`` covers known + unknown codes. - End-to-end wiremock tests: empty queue, paused queue, batches + waiting PRs, multi-scope grouping, ``?branch=…`` query threading, JSON-passthrough preserving an ``extra_field``, and tolerance of a response that omits all optional fields. ## Wiring - ``crates/mergify-queue/Cargo.toml``: adds ``chrono`` (relative time math), ``indexmap`` (scope groups in insertion order), promotes ``serde_json`` from dev to runtime (used for the ``serde_json::Value`` passthrough). - ``crates/mergify-cli/src/main.rs``: registers the ``status`` subcommand under ``QueueSubcommand``, threads the ``--branch``/``--json`` flags, dispatches to ``mergify_queue::status::run``. Adds ``status`` to ``looks_native``. - ``mergify_cli/queue/api.py``: removes ``QueueStatusResponse``, ``QueueBatch``, ``QueuePause``, ``QueueChecksSummary``, ``QueueBatchStatus``, ``QueuePullRequest``, ``QueuePullRequestAuthor``, and ``get_queue_status`` — all now Rust-native. ``QueuePullResponse`` and friends stay for the still-shimmed ``queue show`` (next phase). - ``mergify_cli/queue/cli.py``: removes the ``@queue.command status`` block and the helpers it owned (``STATUS_STYLES``, ``_status_text``, ``_batch_label``, ``_pr_label``, ``_topological_sort``, ``_group_batches_by_scope``, ``_print_batches``, ``_print_waiting_prs``). ``_relative_time`` stays — ``show`` still uses it. - ``mergify_cli/tests/queue/test_cli.py``: deletes ``TestStatusCommand``, ``TestTopologicalSort``, and the ``_invoke_status`` helper. ``TestRelativeTime`` stays. Workspace: 138 Rust tests green, 590 Python tests green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Change-Id: I8cebcd325f05173dfa41083da2ec6516a6ec3a3f
jd
force-pushed
the
devs/jd/worktree-rust-port/add-port-review-checklist-http-test-ux-parity--1fd9757e
branch
from
6b1d62e to
d8f08a0
Compare
jd
force-pushed
the
devs/jd/worktree-rust-port/drop-port-status-toml-inventory-favor-port-delete--77522cb9
branch
from
857f9fc to
10698c0
Compare
Base automatically changed from
devs/jd/worktree-rust-port/drop-port-status-toml-inventory-favor-port-delete--77522cb9
to
main
May 6, 2026 08:49
Contributor
|
@jd this pull request is now in conflict 😩 |
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<T>`` + ``#[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) <noreply@anthropic.com> Change-Id: I1fd9757e7f0213b0aeecbb6eb52b59e689cdf05a
jd
force-pushed
the
devs/jd/worktree-rust-port/add-port-review-checklist-http-test-ux-parity--1fd9757e
branch
from
d8f08a0 to
2b149f4
Compare
remyduthu
approved these changes
May 6, 2026
sileht
approved these changes
May 6, 2026
Contributor
Merge Queue Status
This pull request spent 14 minutes 17 seconds in the queue, including 13 minutes 50 seconds running CI. Required conditions to merge
|
36 tasks
mergify
Bot
deleted the
devs/jd/worktree-rust-port/add-port-review-checklist-http-test-ux-parity--1fd9757e
branch
38 tasks
mergify Bot
pushed a commit
that referenced
this pull request
May 6, 2026
…1364) Centralizes the `--token` / `--api-url` / `--repository` resolvers into one module so every ported command (config simulate, ci scopes-send, the queue commands that follow in this stack) shares the same fallback behavior: - ``resolve_token``: explicit → ``MERGIFY_TOKEN`` → ``GITHUB_TOKEN`` → ``gh auth token`` (subprocess; missing-binary or non-zero-exit is treated as "no token from gh", same as Python's ``CommandError`` catch in ``utils.get_default_token``). - ``resolve_repository``: explicit → ``GITHUB_REPOSITORY`` → ``git config --get remote.origin.url`` parsed via ``parse_slug`` (handles HTTPS and SSH shapes, strips ``.git`` and trailing slashes). - ``resolve_api_url``: explicit → ``MERGIFY_API_URL`` → default ``https://api.mergify.com``. The ``gh`` and ``git`` fallbacks are best-effort: missing binaries or non-zero exits propagate as "fall through to the next source" rather than surfacing errors. Mirrors the Python implementation. ``mergify-config::simulate`` and ``mergify-ci::scopes_send`` are refactored in this same commit to use the new module — their local copies of the resolvers (which were missing the gh / git fallbacks) are removed. The duplicate tests that lived in those modules are dropped; the env-precedence and slug-parsing coverage now lives once in ``mergify_core::auth::tests``. 13 new tests in ``mergify_core::auth::tests``: env-var precedence for token/api-url/repository, default API URL, error message mentions ``gh client``, slug parsing for HTTPS / SSH / dot-git / trailing-slash / empty-owner / path-without-repo. The subprocess-fallback paths themselves aren't unit-tested (they shell out to real binaries); the ``resolve_token_error_message_mentions_gh`` test exercises the "PATH points to a directory with no gh" case to confirm the fallback degrades cleanly. The queue ports later in the stack consume this module from the start — there's no transition period where they ship with a private auth resolver. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Depends-On: #1357
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The 2026.5.5.x release shipped a
ci scopes-sendregression(fixed in #1356): the Rust port deserialized the response body via
let _: serde_json::Value = client.post(...)while the Pythonoriginal ignored the body, and the Mergify endpoint returns an
empty body on success.
Valuestill requires valid JSON, so theRust version surfaced
parse response JSON: error decoding response bodyto every user once it shipped. Test mocks usedset_body_json(json!({}))instead of an empty body, so the bughid through unit tests too.
The fix is structural — split the HTTP client so callers pick
post(parses) vspost_no_response(drops) up front — butthe 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:
HTTP response handling — match the Python original. If
Python ignores the response, Rust must use
post_no_responseetc.; if Python parses, optional/nullable fields must be
Option<T>+#[serde(default)].Test fixtures — mirror the real server.
ResponseTemplatebodies must match what the endpoint actually returns; an empty
body is not the same as
json!({}).UX parity with click — char-counted validators use
chars().count()(notlen()); confirmation prompts go tostderr; resolve all required state before prompting.
Audit of existing/in-flight ports against the checklist:
config validate,config simulate(main): matchci scopes-send(main): regressed, fix in fix(rust): don't parse response body for ci scopes-send #1356queue pause/unpause(feat(rust): port queue pause and unpause to native Rust #1352): matchci git-refs/queue-info(feat(rust): port ci git-refs and ci queue-info to native Rust #1353): no HTTP, no riskCo-Authored-By: Claude Opus 4.7 (1M context) noreply@anthropic.com