refactor(switch): forward the forge CLI's own error text - #3605
Merged
Conversation
GitHub and GitLab each rewrote forge API failures into messages of our
own, discarding what the CLI said. GitLab selected its three by prose
(`error_text.starts_with("404")`), and none of them added anything: "MR
!42 not found" restates `glab: 404 Not found (HTTP 404)` with the status
dropped.
Both now end at `cli_api_error`, which renders our one line of context
above the CLI's own output in a gutter. The one message kept is GitHub's
404, because it answers about an owner/repo *we* chose — from `gh repo
set-default` or from a remote — and gh cannot know that; it now carries
gh's line beneath it rather than replacing it.
Records the rule in the `remote_ref` module docs: forward, unless the CLI
structurally cannot report the condition.
The glab mocks gained the stderr line a real `glab api` writes, which
they had been omitting.
The 404 message names where the owner/repo came from, but only the "github remote" arm had a test; the `gh repo set-default` arm — the fork workflow the message exists for — had none.
max-sixty
marked this pull request as ready for review
July 25, 2026 22:02
max-sixty
added a commit
that referenced
this pull request
Jul 27, 2026
…#3608) ## What prompted this Getting #3605 green ran into codecov reporting a `base_commit` three commits older than the real merge-base. This audits whether our config causes that. ## The cause Codecov picks a PR's base by walking back to the newest ancestor that has a coverage report. It used the real merge-base for PRs #3480, #3532 and #3602, and a stale one for #3603 and #3605. The difference is whether the merge-base uploaded a report. **29 of the last 40 main commits did not.** `ci` had one concurrency group for main pushes, and GitHub cancels the *pending* run in a group whenever a newer one joins, even with `cancel-in-progress: false`. So the question is how long a run holds the group, and a run isn't done until its slowest job is: | job | duration on main | |-----|------------------| | `fast-checks` | 2 min | | `code-coverage` | 3-4 min | | `test (windows)` | 11 min | | `collect affected coverage (windows)` | 110-129 min | Each main run held the group for ~2 hours, so nearly every subsequent main push was cancelled while queued, taking the 4-minute coverage job with it. Every cancelled main run's `updated_at` lands within a second of the next push's `created_at`. The 2 hours is real work, not queue: 2-5s from `created_at` to `started_at`, then 108 minutes inside `cargo affected collect` — 4181 tests under `-C instrument-coverage` with a per-test LLVM profile, ~5 GB of profraw. ## The fix: one workflow per cadence The three groups of jobs have incompatible needs, and one group was serving all of them. | workflow | cadence on main | why | |----------|-----------------|-----| | `ci` | every commit, ~11 min | required gate + fast checks | | `coverage` | every commit, keyed per-sha | a skipped upload leaves later PRs on a stale base | | `affected` | sampled, ~2 h | a DB a few commits old still anchors a correct superset | `affected` keeps exactly the grouping it has today, so its sampling is unchanged and deliberate. It just no longer drags the other two along. ### Scope of the impact The posted `codecov/patch` check scopes to the PR's own GitHub diff, so a stale base did **not** score PRs against other people's lines. On #3605 the posted 91.66% is exactly `github.rs`'s 11/12, while the stale-base compare object reported 64/65 across 13 files. What a stale base costs: - `codecov/project` reports "compared to \<stale sha\>" - the patch `auto` target is the stale base's project coverage (0.02pp here) - the compare API object widens to `base..head`, which is what made the investigation look like silence Separately, `test`/`lint`/`fast-checks` also stopped completing on main. Nothing load-bearing rode on that (they already ran on the PR), but it left `tend-ci-fix` with nothing to watch, since it doesn't fire on cancelled runs. ## Two smaller fixes - `ignore: "**/tests/**"` compiles to `.*/tests/.*` (confirmed against codecov's validator), which needs a leading directory and so never matched `tests/` itself. Inert today since `cargo llvm-cov` reports only `src/` (verified against a downloaded `cobertura.xml`), but now correct if that changes. Now `tests/**`. - `fail_ci_if_error` gated on `github.repository_owner`, which is the *base* repo's owner on a fork PR too, so the soft-fail its comment describes never applied. It keys off the head repo now. ## Docs The API behaviour was ours to misuse, not codecov's to explain. Three traps, all confirmed against the live API: - `file_report/<path>/` 404s with `coverage info not found` because the route swallows the trailing slash into the path. Without it the endpoint returns `line_coverage`. - `?pullid=N` always compares the PR's **current** head. `?base=&head=` asks about an earlier commit. - the compare response has no `patch_totals` key, and `.name` is `{base, head}` rather than a string, so a filename lookup silently matches nothing. A working recipe already existed in `running-tend`, but that skill is scoped to CI. `tests/CLAUDE.md` owns coverage investigation, so the queries go there and `running-tend` points at them instead of keeping a second copy. Re-running the corrected query against #3605's failing commit reproduces the miss exactly: `src/git/remote_ref/github.rs:164`, the `gh repo set-default` hint, matching what the session eventually found by hand. ## This PR demonstrates it It changes no Rust at all, only YAML and markdown. Codecov still reported a **10-file, 111-line patch** on its first commit, because it based the comparison on `203603909` rather than the real merge-base `32f380a27`. Every main commit in between has no report: | commit | ci run | report | |--------|--------|--------| | `32f380a27` | queued | no | | `9645e3e13` | cancelled | no | | `bcd1ffdfd` | cancelled | no | | `8865f20ab` | cancelled | no | Every one of those 111 patch lines belongs to somebody else's merged commit. It passed at 100% only because those commits are well covered. > _This was written by Claude Code on behalf of @max-sixty_ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com>
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
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.
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:
A GitHub PR that doesn't exist — the one message kept, now with gh's verdict under it rather than instead of it:
The rule, now written down
src/git/remote_ref/mod.rsgets 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, fromgh repo set-defaultor a remote; gh can't know that) and Azure's missingazure-devopsextension (a preconditionaz extension listanswers, whichaznever names). Reading more nicely than the CLI doesn't qualify.cli_api_errorandcli_api_error_detailsgained 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 asmessagerather than bailing.Evidence
Measured, not assumed — both CLIs render an API failure the same way:
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_detailsprefers 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.ghputs the status in a structured field, so its 404 arm keys onstatus.glabputs 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 realghbinary (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/patchalso flagged one line — re-indenting the 404 block pulled a pre-existing gap into the patch, and it turned out to be real: thegh repo set-defaulthalf of the hint, the fork workflow the message exists for, had never been tested.test_switch_pr_not_found_gh_defaultcloses it.