merge queue: embarking main (c30b79b), #1373 and #1364 together#1377
Closed
mergify[bot] wants to merge 5 commits into
Closed
merge queue: embarking main (c30b79b), #1373 and #1364 together#1377mergify[bot] wants to merge 5 commits into
mergify[bot] wants to merge 5 commits into
Conversation
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
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> Change-Id: I3ae3542b75bf407150b2607f50255ec91e9e9587
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
1 participant
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.
🎉 This pull request has been checked successfully and will be merged soon. 🎉
Branch main (c30b79b), #1373 and #1364 are embarked together for merge.
This pull request has been created by Mergify to speculatively check the mergeability of #1364.
You don't need to do anything. Mergify will close this pull request automatically when it is complete.
Required conditions of queue rule
defaultfor merge:depends-on = Mergifyio/mergify-cli#1357[⛓️ docs(port): add port review checklist for HTTP/test/UX parity pitfalls #1357]title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert|ui)(?:\(.+\))?:#approved-reviews-by>=2author = dependabot[bot]author = mergify-ci-botauthor = renovate[bot]body ~= (?ms:.{48,})#changes-requested-reviews-by = 0#review-requested = 0#review-threads-unresolved = 0check-success=ci-gateRequired conditions to stay in the queue:
base=maindepends-on = Mergifyio/mergify-cli#1357[⛓️ docs(port): add port review checklist for HTTP/test/UX parity pitfalls #1357]label!=manual mergetitle ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert|ui)(?:\(.+\))?:#approved-reviews-by>=2author = dependabot[bot]author = mergify-ci-botauthor = renovate[bot]body ~= (?ms:.{48,})#changes-requested-reviews-by = 0#review-requested = 0#review-threads-unresolved = 0check-success=ci-gate