Skip to content

fix(git): retry a PR lookup without CI fields only when gh names that field - #825

Open
eric-engberg wants to merge 2 commits into
sirmalloc:mainfrom
eric-engberg:fix/gh-ci-field-retry
Open

eric-engberg wants to merge 2 commits into
sirmalloc:mainfrom
eric-engberg:fix/gh-ci-field-retry

Conversation

@eric-engberg

Copy link
Copy Markdown
Contributor

What

When a Git CI widget is on the line and gh pr view fails for an ordinary reason (no PR for the branch, an auth or network error), the PR lookup no longer runs gh a second time without the CI fields. Each failed gh run used to be followed by a second one, which doubled the time the lookup spent inside its 5s budget. Now a PR opened from a fork is found with a slow gh instead of being cached as "no PR". The run without CI fields still happens when gh can't give the CI field.

Why

With a CI widget, the lookup asks for --json url,number,title,state,reviewDecision,statusCheckRollup. If that fails and isCiFieldUnavailableError says gh couldn't give the field, the lookup runs again without statusCheckRollup. That check searched the error's message and stderr together for statuscheckrollup. execFileSync starts the message with the command line, and the command line asks for statusCheckRollup. So every failure matched.

What execFileSync throws for a gh that exits 1 with no pull requests found for branch "feature/x" (the same under Node 26.10.0 and Bun 1.4.2):

message: Command failed: <path>/gh pr view --json url,number,title,state,reviewDecision,statusCheckRollup -- feature/x
         no pull requests found for branch "feature/x"
stderr:  no pull requests found for branch "feature/x"

What gh itself prints when it can't give a field:

  • Its --json check (pkg/cmdutil/json_flags.go) builds fmt.Errorf("Unknown JSON field: %q\nAvailable fields:\n %s", ...). gh 2.102.0, run offline with a scratch config and a dummy token, prints Unknown JSON field: "statusCheckRollupX", then Available fields: and the list. The wording is the same in v1.9.0, the April 2021 release that added --json, and statusCheckRollup was already in its PR fields then.
  • A GraphQL error gives the path of the field it failed on (GraphQL: <message> (<path>), from go-gh's GraphQLError.Error()). gh queries the field as statusCheckRollup: commits(last: 1) { … }, so an error on it names statusCheckRollup.
  • gh older than 1.9.0 has no --json at all, so the run without CI fields would fail the same way.

Measured with a stand-in gh first on PATH that logs every call and sleeps before failing, a scratch repo on branch feature/no-pr with an https://github.com/… origin, and the background refresh a CI widget starts (ccstatusline --internal-refresh-git-review-cache <repo> checks <lock>). The lookup asks gh twice: first with gh's own repo resolution, then pinned to origin with --repo.

Case Runtime main this branch
No PR for the branch; each gh pr view takes 1s Bun 1.4.2 4.2s; 4 gh runs, 2 of them without CI fields 2.2s; 2 gh runs
same Node 26.10.0 4.4s; 4 gh runs 2.3s; 2 gh runs
No PR; each run takes 2s Bun 1.4.2 5.1s; 3 gh runs, the pinned one cut off at the budget 4.2s; 2 gh runs
same Node 26.10.0 5.3s; 3 gh runs 4.3s; 2 gh runs
PR opened from a fork, so only the --repo lookup finds it; each run takes 1.8s Bun 1.4.2 5.1s; 3 gh runs; cached as no PR 3.8s; 2 gh runs; PR #7 cached with its checks
same Node 26.10.0 5.3s; 3 gh runs; cached as no PR 4.0s; 2 gh runs; PR #7 cached with its checks
gh prints Unknown JSON field: "statusCheckRollup" for the CI fields Bun 1.4.2 and Node 26.10.0 2 gh runs; PR cached without checks the same

A "no PR" result stays cached for 30s, so in the fork case the Git PR widget showed nothing until the next refresh, which then ran out of time the same way.

How

  • The lookup decides whether to run without CI fields from gh's stderr only, not the error's message. The text it looks for is unchanged: statuscheckrollup, which gh's Unknown JSON field error and a GraphQL error on the field both contain, or resource not accessible by integration.
  • In git-review-cache.test.ts, the stand-in gh failures are now shaped like execFileSync's errors: the message starts with the command line asking for statusCheckRollup, and gh's output is in stderr. "does not retry metadata-only for ordinary CI lookup failures" uses gh's no pull requests found for branch "…" and now fails on main (3 gh runs instead of 2). The two tests for the retry use gh's Unknown JSON field: "statusCheckRollup" and a GraphQL Resource not accessible by integration error on the field's path.

Left as is:

  • When gh doesn't know some other field, its Available fields: list names statusCheckRollup, so that error would still match. It can't happen here: every field the lookup asks for has been in gh since 1.9.0 added --json.

Overlap with open upstream PRs: #618 (caching confirmed absent PRs) also changes git-review-cache.ts and conflicts with this one.

Demo

No GIFs: the line only differs when gh is slow enough to cut off the fork lookup (the 1.8s rows above), so the timings and gh run counts above show the change.

Testing

  • On main as of 2.2.34 (0c94df7): it merges cleanly. bun run lint is clean, and bun test on Linux CI gives 4403 pass, 0 fail (4403 on main). The built CLI under Bun 1.4.4 and Node 26.13.0 agrees across runtimes and matches main in plain and Powerline mode, and the TUI opens and exits cleanly under both. It adds no JavaScript or Node API newer than Node 14, the build's target.
  • Three tests in git-review-cache.test.ts changed (no new tests). One of them fails on main under both Bun and Node vitest. The two retry tests pass on both and fail when the retry is turned off. bun run lint is clean.
  • git-review-cache.test.ts under vitest on Node 26.10.0: 33 of 33 pass, as on main.
  • Runtime check: the built CLI renders the same as main in plain and Powerline mode, and Bun 1.4.2 and Node 26.10.0 give the same output. The TUI opens and exits cleanly under both.
  • End to end with a stand-in gh: the table above.
  • Merges cleanly with fix(git-review): find PRs on slow self-hosted links and behind forge-named SSH aliases, and count only a re-run check's newest run #824, which reworks this file. The merged git-review-cache.test.ts passes 59 of 59.

… field

With a Git CI widget on the line, the PR lookup asks `gh pr view` for
statusCheckRollup and runs again without it when gh can't give that
field. The check searched the whole error text for "statuscheckrollup".
execFileSync starts that error's message with the command line, which
asks for statusCheckRollup, so every failure matched: no PR for the
branch, an auth error, a network error. Each failing lookup ran gh twice
inside the one 5s budget that the lookup with gh's own repo resolution
and the one pinned to origin with `--repo` share. On a branch with no PR
that was four gh runs instead of two. When gh takes about 1.7s or more,
the pinned lookup, which is what finds a PR opened from a fork, ran out
of time and the lookup was cached as no PR.

Decide from gh's stderr alone. gh names the field when it can't give
it: its --json check prints `Unknown JSON field: "statusCheckRollup"`,
and a GraphQL error on the field gives the field's path.

The tests' stand-in gh failures now carry the command line in their
message and gh's output in stderr, as execFileSync's errors do.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant