From 2c62c3efdf293a24386b5b6c24be77c4a69ee59f Mon Sep 17 00:00:00 2001 From: orca-bug-scan-bot Date: Tue, 5 May 2026 14:27:00 -0700 Subject: [PATCH] fix: address pr-bug-scan findings from #1424 Gate gh retries on idempotency (auto-detected from argv/-X/mutation), key parent-field probe by (owner,ownerType), accept 'to an' in not_found classifier, and replace bulk label PUT with parallel sing --- src/main/git/runner.ts | 73 +++++++++++++++++++++- src/main/github/project-view.ts | Bin 54201 -> 54436 bytes src/main/github/project-view/internals.ts | 6 +- 3 files changed, 76 insertions(+), 3 deletions(-) diff --git a/src/main/git/runner.ts b/src/main/git/runner.ts index fa18e591838..c43058504b4 100644 --- a/src/main/git/runner.ts +++ b/src/main/git/runner.ts @@ -205,7 +205,69 @@ export function gitSpawn(args: string[], options: SpawnOptions & { cwd: string } // resolveCommand routes the spawn through `wsl.exe -d -- gh ...` // even without a UNC cwd to parse a distro from. Repo-scoped callers should // keep using cwd — the distro derives from the path automatically there. -type GhExecOptions = Omit & { cwd?: string; wslDistro?: string } +// Why: `idempotent` gates the transient-error retry. When undefined we +// auto-detect from argv (writes are detected by `-X POST/PATCH/PUT/DELETE` +// or a `query=mutation …` arg); callers can also pass an explicit override. +// A 5xx/socket reset after the request reaches GitHub but before the +// response returns is the canonical case where the server-side write +// succeeded; retrying would create a duplicate comment/issue/label addition. +// See bug-scan finding 1. +type GhExecOptions = Omit & { + cwd?: string + wslDistro?: string + idempotent?: boolean +} + +const NON_IDEMPOTENT_METHODS = new Set(['POST', 'PATCH', 'PUT', 'DELETE']) +// `gh ` write subcommands. Reads (view/list/status/checks) +// are absent on purpose so the default of "retry" stays for them. +const NON_IDEMPOTENT_GH_VERBS = new Set([ + 'create', + 'edit', + 'delete', + 'close', + 'reopen', + 'merge', + 'comment', + 'review', + 'ready', + 'lock', + 'unlock', + 'pin', + 'unpin', + 'transfer', + 'develop' +]) + +function argsLookIdempotent(args: string[]): boolean { + for (let i = 0; i < args.length; i++) { + const a = args[i] + if (a === '-X' || a === '--method') { + const next = args[i + 1] + if (typeof next === 'string' && NON_IDEMPOTENT_METHODS.has(next.toUpperCase())) { + return false + } + } + // `gh api graphql -f query=mutation(...){ ... }` — detect mutation queries + // so writes via the GraphQL endpoint also fail fast on transient errors. + if (a.startsWith('query=')) { + const trimmed = a.slice('query='.length).trimStart().toLowerCase() + if (trimmed.startsWith('mutation')) { + return false + } + } + } + // `gh issue close`, `gh pr edit`, `gh pr merge`, etc. The first arg is the + // noun (issue/pr/repo/label/...) and the second is the verb. Defaulting + // `gh api` calls without an explicit -X to GET-equivalent (idempotent) is + // intentional: callers that POST through `gh api` set `-X POST`. + if (args.length >= 2 && args[0] !== 'api') { + if (NON_IDEMPOTENT_GH_VERBS.has(args[1])) { + return false + } + } + return true +} /** * Extract stderr from an execFile rejection. @@ -344,7 +406,14 @@ export async function ghExecFileAsync( lastError = err const { stderr } = extractExecError(err) const isLastAttempt = attempt >= GH_RETRY_DELAYS_MS.length - if (!isLastAttempt && isTransientGhError(stderr)) { + // Why: only retry idempotent calls. A 5xx/socket reset can arrive + // after the server already applied a POST/PATCH/PUT/DELETE; retrying + // would duplicate the write (e.g. double-post a comment, double-add + // a label). When the caller doesn't say, we auto-detect from argv — + // explicit `-X ` and GraphQL `query=mutation …` are treated + // as non-idempotent. See bug-scan finding 1. + const idempotent = options.idempotent ?? argsLookIdempotent(args) + if (idempotent && !isLastAttempt && isTransientGhError(stderr)) { // Why: when the upstream surfaced a Retry-After (e.g. on a transient // 5xx that GitHub explicitly recommends backing off for), honor it // instead of using our default backoff — sleeping less than the diff --git a/src/main/github/project-view.ts b/src/main/github/project-view.ts index 32eabb523e1764cdda3b90d88b070f74d210a03d..6f6f2b73a363a3d64bfd8c5ca156ccbf26687cc1 100644 GIT binary patch delta 3023 zcmaJ@eQaCR6`!4cWNGsSHEX__+q8*oVmq)EDw6agO&nM|6G9$35qF&iS2p_T{RDA632jdhKja4;jzod&v~bvlN-kljGZGjA@wr2V`S1%qSkB@ zvq`IZ3PUDejnyO#-Li?NHmou&HOg$Rvedr(5djL>MRYce=61h@_qwt>6u!}BoN^sy z2MtB}|CT*H@L6r=pvVESB^J#VQ6p5-h=Hl{91|jegOqmYh8@i_JIcMdI*4Us<`KBN z?$#>Qf)jP*R)RUHdNi%6N(PhRMpm^LF)@6J57r5h*78FAwYtbH<+nMr9L@GnfW6t~ z1I2s~QL~tDgp7FZ`g4Yw+AY$rNjrMdRz>XuL}`R6HMM}* zw$^oTI=y}a;^uIj=#Hkj4>foADlVRCdC>>GZJqPY4QP>ADT6<@J-cy*aRDwovKbb( zT)W}Y{Z0F`zWSA_m<4=vZj(E=^?PgJ_QuHihvoZy@IY6qd$Q}Cudb*a+@DAsjR)c0 z&{jAR3c#rl*;En@jSdWtJycY<6pA#K1f%hxfx)r(P|&RjpKfc)QEtU(+Lp~qxl;U5 zsDT$Qw8LMA)taIq7TbOB?s-3aynn(yc;MYND+I9n$mR7Iw)lgMBjL63?A|f5xvDNL z#=?r3j+F`NVNeo32q@fe%zb$|Fl9;m*pI)A^JJ^K!wPUEu?7A!7J$u1cft#AZF8rO ze&R0y98^<5cx2+95^6!+$xbpJg!d+P)RywpaXNx-*Zp@@LyCso?^Cq~r#G2lR++rK zri*x;D0nL@n*9bw(r9vu7->uj=V~Q0tmLW+o?4xzW|w(NdMYxNBqoe%HELQxJ=ZZ0 zAs+WgCdB!{(W{&%Sxb2pA;ohw&YR2<{xQ|oP@<8`*BwG0jhFWDicqZFgw^h}Xnv-H9 zlEvY-*}k14@lkSQ{4oB<Xda8HX^eY?sN`2@(YK18Mh5KHw{;L|S*oJT z{B9slPwg>%`!*s{2@`BNY#*w7cv#|2w3(;3q;H2$^^T(RX5)jpSY;xg89O$3I7yIU z*gOP-+08c#KP8wVUIE-EOz<_vQcPnuTNZ}hF8(Xu>>Bynh`2U`HE8k8htTk3wPIxyyGPdYxcP z$!o=X(!2naUxSHUCp6Bqz;n4;_b0iiAI{IUffpA??);cYAYbhh5TIQ!t55~axnv*WS1g|r3! zefcKXutd?U8=hbKI($Ieq5ADUxA5J7Unp!eslsB6b4;)B;-yo*rw|&NTT7;5*#mzJKOsKYY5- z4WXA7%Fh4x(mTGQqbo0ybSfZnam2@tll3xPyzn^Ox7Y;_FWv_Cz10gJE(YB7zwY$=LJ+yw)K2!0 zZw4LwJPRHpy?8{NsIH!*;xj0CEbLkrpI$2~5q`75?SB(|!kPQ|@BUPEeLJ|%FI|;A mQoFJR3h?JsVYqx{PZ{aXt6zAC0v3OrYzPeXj%HZ+ec-<-97*K> delta 2571 zcmZ`*YfKbZ6wWLlc6qKY0$b3FNS8%ewV|m+Q4~-t)gmB9jp(p@7bfn`Ec0Ncx)JRo z#ZaprQsa+mOlyfGZ3ts)sF6l|q_I||HH}S7(_d}0)sn^^tv}kHxidVr$o|=x^Szn-eHK>ehg1o)sr?S>k5MY3j(OTe2}SjY#1 zL~2o}+(Dn=T+UpJ?Ms%x$+RN4o<;~h6J8{OSP}>dt!caF_Y^n}8T+&pG&g?4XfXxLKiMiJcdt%#{Lneix!LhUyM*3}4e zrh4Hl4fCOz<2FbdPJ^wruR(M;Q9oDvL%dn+L>Em{EVjGu7Bp}6=slaW;#@2kAprxS ziZ`q&ZH1zT31P7DM4qK?l_f2_wv*xbe^$*~`kj`QPZCu@)Trs6-c*`sDWIz9W#bG4 zZr@y_pK5wyo69ugjTHQ>iyv;Q$&8VUTBwD}O!s74X-2FLN5)$9()L$oTdfjXJVx{N z6D@+hQV8H)NQiow_;9Q6Pf&j@v>1yz7YVsSj4r4SCPSt%Z}550(~!|ioJhg1$;c$MkSq85tLaS&H0^ANSGCGT#!HB@uMtytOAq@&K?Lyc@ z4^j-wYhPqEAA42Sz7Y8KpW$`6)F>5>$Q$((rH7l~hkkXKYidB&3rT(rn-d7~tp-aO zQ4yRfkpk>&7FXebd{j}0NIP(L*fL{cR3RFIEJ$iL))UDPW20pi#yYZ$JgTfw$c=1= zo(SRG@M)w3evd52r-HrialOv(&ETMP14$1EvZ8vaEMuT3rq)k&me}CV$p!G!z6vBI z$L8#1wGq9ljBepv{pmK(epjT-4}bQwhVW$RC)q76ps z7Q&ldS>QgHrl0LP!ZE?7L+K`v_2xs(@$lH0tB<~Ooz)8D&K$-oSKsB0U{KMW&Y)h` zea(!vv?tkg{%Bj1mPa`4DH9}KSNr4x6+W!x&TlWoR~Mvf?Df@On1;_xG0tGwF2C#TRVmT@m95 z6<3^a$(0Dj`WV536@X4iYj%*PGnL&4?446QdV zCqd6ANv!KvYBJ%k%PCAwMoLW}>Ww1>w)H+9`2)Xt%FBrxo6n2F8@wtYTTTpcHQC|T z@QcU2b3#;xq8Q_y`ry?M5*WO3^92C diff --git a/src/main/github/project-view/internals.ts b/src/main/github/project-view/internals.ts index fd69717a68c..52509c30652 100644 --- a/src/main/github/project-view/internals.ts +++ b/src/main/github/project-view/internals.ts @@ -194,7 +194,11 @@ export function classifyProjectError(stderr: string, stdout: string): GitHubProj if ( s.includes('http 404') || errors.some((e) => (e.type ?? '').toUpperCase() === 'NOT_FOUND') || - s.includes('could not resolve to a ') + // Why: GitHub uses "to an" for vowel-leading types ("to an Issue", "to + // an Organization") and "to a" otherwise. The previous singular-only + // check missed the "an" variants when gh emits only the stderr summary + // without a structured GraphQL error array. See bug-scan finding 3. + /could not resolve to an? /.test(s) ) { const firstNotFound = errors.find((e) => (e.type ?? '').toUpperCase() === 'NOT_FOUND') return {