fix(gitea): key the tea api error shape on the envelope, not on a non-empty message - #3600
Draft
max-sixty wants to merge 1 commit into
Draft
fix(gitea): key the tea api error shape on the envelope, not on a non-empty message#3600max-sixty wants to merge 1 commit into
max-sixty wants to merge 1 commit into
Conversation
…-empty message
Gitea blanks a 500's message in production unless the token belongs to an
admin, so the body arrives as `{"message":"","url":"…"}`. Both Gitea call
sites required a non-empty message before they would call a response an
error, so that body fell through to the data path: `wt list` logged
"Failed to parse tea api pulls JSON", and `wt switch pr:<n>` told the user
"This may indicate a Gitea API change" — blaming an API change for an API
error.
The key is now the presence of the `message` key, which is the whole shape:
`tea api` never reads the HTTP status, and none of the resources read here
(a PR object, a PR array, a combined commit status) carries a `message`.
One parser, `remote_ref::gitea::api_error_message`, now serves both sites —
the CI-status backend drops its duplicate struct — and a blanked message
reaches the user as an error that says why there is no detail.
max-sixty
added a commit
that referenced
this pull request
Jul 25, 2026
Follow-up to #3600, independent of it (no shared files). GitHub and GitLab rewrote forge API failures into messages of our own, discarding what the CLI said. GitLab picked its three by prose — `error_text.starts_with("404")` — the pattern #3595/#3597 removed elsewhere. ## Before / after A GitLab MR that doesn't exist: ``` - ✗ MR !9999 not found + ✗ glab api failed for MR !9999 + ▎ glab: 404 Not found (HTTP 404) ``` A GitHub PR that doesn't exist — the one message kept, now with gh's verdict under it rather than instead of it: ``` ✗ PR #999999 not found on max-sixty/worktrunk (gh default). Check that `gh repo set-default` points to the correct repository. + ▎ gh: Not Found (HTTP 404) ``` ## The rule, now written down `src/git/remote_ref/mod.rs` gets an **Error Messages** section: forward the CLI's own line, and write our own only where the CLI *structurally cannot* report the condition. Two cases qualify — GitHub's 404 (it answers about an owner/repo *we* chose, from `gh repo set-default` or a remote; gh can't know that) and Azure's missing `azure-devops` extension (a precondition `az extension list` answers, which `az` never names). Reading more nicely than the CLI doesn't qualify. `cli_api_error` and `cli_api_error_details` gained docstrings for the mechanism: our context as the headline, the CLI's words in the gutter, so a provider with something to add passes it as `message` rather than bailing. ## Evidence Measured, not assumed — both CLIs render an API failure the same way: | | stdout | stderr | |---|---|---| | `gh api …/pulls/999999` | `{"message":"Not Found",…,"status":"404"}` | `gh: Not Found (HTTP 404)` | | `glab api …/merge_requests/…` | `{"message":"401 Unauthorized"}` | `glab: 401 Unauthorized (HTTP 401)` | `cli_api_error_details` prefers stderr, so the gutter gets the human line, not the JSON. That also surfaced a test bug: **the glab mocks never set stderr**, so they'd have enshrined output real glab doesn't produce. Fixed to match the probe. `gh` puts the status in a structured field, so its 404 arm keys on `status`. `glab` puts it only inside the message text — nothing to key on, which is why prose-matching was the only way to keep those arms. ## What this gives up A bad or expired token no longer gets "run gh auth login" / "run glab auth login"; it gets `gh: Bad credentials (HTTP 401)`. The far more common case — no auth configured at all — is unaffected: gh prints its own "please run: gh auth login" and always did fall through to forwarding. Happy to restore the GitHub 401 arm (it's structurally keyable); GitLab's can't come back without prose-matching. ## Testing `cargo run -- hook pre-merge --yes` → 4572 passed. Six snapshots reviewed individually before accepting (five updated, one new). Both paths also driven through the **real** `gh` binary (a live 404 and a live 401), not only mocks. Clippy clean with `--features shell-integration-tests`. Coverage: the GitHub 404 arm and the forwarding fallthrough each have a test, on both forges. `codecov/patch` also flagged one line — re-indenting the 404 block pulled a pre-existing gap into the patch, and it turned out to be real: the `gh repo set-default` half of the hint, the fork workflow the message exists for, had never been tested. `test_switch_pr_not_found_gh_default` closes it. > _This was written by Claude Code on behalf of Maximilian Roos_
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.
Gitea blanks a 500's message in production unless the token belongs to an admin (
services/context/api.go:if setting.IsProd && !(ctx.Doer != nil && ctx.Doer.IsAdmin) { message = "" }), so the body arrives as{"message":"","url":"…/api/swagger"}. Both Gitea call sites required a non-empty message before they would call a response an error, so that body fell through to the data path:wt listloggedFailed to parse tea api pulls JSON— blaming an API change for an API error. (The CI cell was blank either way, so this was log-only.)wt switch pr:<n>failed withFailed to parse Gitea API response for PR #N. This may indicate a Gitea API change.— the same misattribution, in the user's face.The key
The discriminator is now the presence of the
messagekey, which is the whole shape.tea apinever reads the HTTP status —runApicopies the body to stdout and returns nil regardless — so the body is the only channel, and none of the resources read here carries amessage: verified against giteamainforPullRequest,CombinedStatus, andCommitStatus.urlis deliberately not part of the key. The two mistakes aren't symmetric: an error body that omitsurlwould read as the resource, which is the bug this key exists to prevent, while requiring it would only guard against a resource one day growing amessagefield. Gitea already ships error shapes without one (APIInvalidTopicsErrorismessageplusinvalidTopics), andCombinedStatusdoes carryurl.Shape
One parser —
remote_ref::gitea::api_error_message— now serves both sites; the CI-status backend drops its duplicate struct, and the PR path checks the envelope before the resource parse, so "may indicate a Gitea API change" is reserved for a body that is neither. A blanked message reaches the user as an error that says why there's no detail:The CI cell for a blanked 500 stays blank, unchanged:
is_retriable_erroris the one gate every backend uses to turn a failure into the error indicator, and an empty message isn't retriable. Only the misleading log line is gone.Tests
Extended rather than duplicated:
test_tea_api_error_reads_the_response_shapeflips its blank-message assertion (that assertion was the bug), a newtest_api_error_message_reads_the_response_shapepins the key against every shape both sides see, andswitch_pr_gitea_blank_error_messagesnapshots the user-visible path alongside the existing 401/403/404 cases.Noted, not done
tea's
apicommand has an--includeflag that writes the status line to stderr — a genuine structured channel that would beat shape-sniffing. It's in tea'smainsource but in no released changelog entry, so sending it would break every user whoseteapredates it. It would also compose with this change rather than replace it, since the body still supplies the message text.🤖 Generated with Claude Code