Repository navigation
fix(github): finalize checks for early collection failures - #271
Conversation
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…sue236-early-collection # Conflicts: # src/adapters/github-termination.test.ts
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
frankekn
left a comment
frankekn
left a comment
There was a problem hiding this comment.
老審 staff review @ 40ad758（full first pass）
總評
目標清楚，搬法對。collection（merge-base / diff / stat / changed / prMeta）移進 pending check 之後的 try，失敗走既有 catch：完成 owned check、必要時 error comment、exit 1、operational: false、零模型呼叫。入口 closed / stale（PR_HEAD_SHA≠API head）/ same-head 仍在建 check 之前。拿掉 git rev-parse HEAD 後備正確。
17 條 early-collection 測試對得上描述的矩陣（含 timeline 抑制、entry 不建 check、POST 失敗有界、recovery 上限）。termination fixture 的 Actions slug 在 merge #264 後 head 仍在，與 ownership 合約一致。
Codex 在 5b31f25 稱無 major；對 40ad758 的重跑撞 usage limit，不算附議該 merge commit。本審獨立看完 production + test diff。
Findings
P3 — failCollectionMetadata 只打 comments URL
github-posting.test.ts 的 metadata 注入只讓 comments_url 失敗，review_comments_url 仍成功。production 兩路都會 ghJson；reviews 失敗路徑沒被這組測試釘住。要嘛補 reviews stub 失敗，要嘛測試名寫明只覆蓋 comments。
P3 — 缺 API head.sha 的錯誤語意
runGithub 現在先要求 nestedString(pr, "head", "sha")，沒有就 throw，之後才讀 PR_HEAD_SHA。這比「只禁本機 HEAD」更嚴，但合理：若只有 env、API 無 head，skipReasonForPr 會把 null !== envSha 誤判成 stale_head 而靜默 skip。維持 throw 可以，請把訊息寫成「PR API 未回 head.sha」，避免已 pin PR_HEAD_SHA 時以為是總解析失敗。
無異議
- recovery 計入 collection 失敗：有意，且有測試。
- check POST 失敗：
pendingCheckId === null，仍 error comment、不 PATCH。 - empty-diff / missing-base / unknown head 邊界與 PR 描述一致。
CI / merge
- draft；
needlefish-ci@ 40ad758 in_progress；mergeable_state=unstable。 - 結構上可進 ready（待 CI 綠）。gateClass R / #236 #213 binary coverage 不在本 PR scope，同意描述。
— 老審
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM ✅ — PASS — early collection failures now reach the owned-check finalizer; added guards are conservative.
🔍 1 not re-found (code changed)
Coverage: full diff reviewed in one pass (4 files)
Review target: PR #271 eec0a58..2a53b5c
Findings
No actionable findings. Prefer this over padding weak ones.
Checked (7)
- Verified collection (diff, stat, changed paths, metadata, bundle creation) moved inside src/adapters/github.ts:1759-1814, whose catch completes the pending check and posts the failure diagnostic.
- field=head.sha="" → src/adapters/github.ts:1689
if (!currentHeadSha)→ rejects → exits before a check rather than guessing the checkout head. - field=base.sha="" → src/adapters/github.ts:1778
if (!baseSha)→ rejects → terminal failure path after an owned check when credential preflight succeeds. - field=docsOnlyHead=false on git failure → src/adapters/github.ts:1732
if (!docsOnlyHead(...)) throw error→ rejects the docs-only credential bypass → preserves the credential error conservatively. - TRIGGER_A cleared predicate=skipReasonForPr callsites=[src/adapters/github.ts:1582 termination completion governs=yes action=complete stale/closed run neutrally; src/adapters/github.ts:1691 initial review start governs=yes action=do not begin a stale/closed review; src/adapters/github.ts:1482 final result posting governs=yes action=do not publish a stale/closed result]
- TRIGGER_C cleared carrier=isDocsOnlyFastPath @src/core/review.ts:1053 promises=requires only changed-file data body=checks changedFiles only @src/core/review.ts:1055-1059 callers=[src/adapters/local.ts:370 relies=no; src/adapters/local.ts:383 relies=no; src/adapters/github.ts:167 relies=no; src/adapters/github.ts:1814 relies=no]
- TRIGGER_D cleared swallow=src/adapters/github.ts:168 signal=git collection error previously_reached=N/A (new preflight probe) now_receives=false callers=[src/adapters/github.ts:1732 distinguishable=no safe=yes, because unknown docs-only status conservatively retains the already-raised credential error]
2 calls · review 3m 56s → critic 15s · total 4m 11s
frankekn
left a comment
frankekn
left a comment
There was a problem hiding this comment.
老審 staff review @ c951ec7（delta 40ad758..c951ec7）
總評
一個 commit，兩個 P3 都收掉。production 只改一行錯誤訊息，沒動行為。
前次 asks
- P3
failCollectionMetadata只打 comments — 已關。改成"comments" | "reviews"，https://example.invalid/reviews失敗分支放在成功分支之前，矩陣多一個reviewsstage。兩條 metadata 路徑現在都被釘住。 - P3 缺
head.sha錯誤語意 — 已關。PR API returned no head.sha; cannot resolve or verify PR head，測試 regex 同步更新。repo 內舊字串只剩預設分支上的github.ts，沒有其他依賴這段文字的地方。
新 findings
無。
CI / merge
- draft；
needlefish-ci@ c951ec7 in_progress，reviewqueued。 - 我這邊沒有開著的 ask。CI 綠了就可以 ready。
— 老審
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This comment has been minimized.
This comment has been minimized.
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
This comment has been minimized.
This comment has been minimized.
…ction Resolve the src/adapters/github.ts conflict by keeping the PR-collection block inside the moved try block and porting the base's changes into it: prDiffFromShas(repoPath, baseSha, headSha) replaces the changedFiles call, and resolvedOpts is built from resolveConnectionOptions with !isDocsOnlyFastPath(bundle) after the bundle is collected.
This comment has been minimized.
This comment has been minimized.
frankekn
left a comment
frankekn
left a comment
There was a problem hiding this comment.
老審 staff review @ c0180c5（delta c951ec7..c0180c5）
總評
三個都是 merge commit。production 衝突解得對：搬進 try 的區塊跟 base 現在的版本逐行一致（prDiffFromShas(repoPath, baseSha, headSha)、bundle 收完才 resolveConnectionOptions(..., !isDocsOnlyFastPath(bundle))），唯一的語意差是 !baseSha || !headSha 縮成 !baseSha，因為 headSha 在入口已保證非空，合理。
但測試被這次 merge 打壞了，先別 ready。
新 findings
P2 — git stub 認不出 base 新的 reviewDiff 前綴，diff/stat 注入失效
base 這輪帶進 #213 的 reviewDiff(cwd)：patch 與 --stat 兩個 call 現在是 git -c core.attributesFile=/dev/null --attr-source=<emptyTree> diff --no-textconv --no-ext-diff ...。github-posting.test.ts:338 的 stub 只看 args[0]，這兩個 call 的 args[0] 是 -c，stage 變成 ''，直接放行給真 git。merge-base 跟 changed（仍是 diff --name-only -z）不受影響。
我在 box 上用同一個分類式跑過實際 argv：patch → ""、stat → ""、merge-base / changed 正常。在 3a70d85 時 repo.ts 還沒有 reviewDiff，所以當時 CI 綠、描述裡的 179/179 成立；到 c0180c5 已不成立。
受影響的測試（都依賴 collectionGitFailure: "diff" 或 "stat" 真的失敗）：early collection failure diff、... stat、stale/closed/unknown timeline 三條、check POST failure、recovery limit，共 7 條。注入沒生效時會走到 model，runnerInvocationCount === 0 與 status === 1 會紅。same-head 那條在 collection 之前就 skip，不受影響。
修法：照真正的 git 子命令分類，跳過 -c <value> 與前置 flag。inline 附 suggestion。改完請重跑 posting 全套，PR 描述的數字一併更新。
前次 asks
無未結。40ad758 / c951ec7 兩個 P3 仍是關的。
CI / merge
- draft；
needlefish-ci、review、Needlefish@ c0180c5 都 in_progress。依上面分析，needlefish-ci預期會紅在這 7 條。 - 卡在 P2。stub 修好、CI 綠就可以 ready。
— 老審
…llection The fake git keyed on args[0], but review diffs now start with a -c/--attr-source prefix, so the diff and stat failures never fired. Skip leading options to find the subcommand. Connection validation now runs inside the owned-check scope, so its error carries the review-failed prefix. Pin the exact line.
frankekn
left a comment
frankekn
left a comment
There was a problem hiding this comment.
老審 staff review @ 5dd168a（delta c0180c5..5dd168a）
總評
一個 test-only commit，P2 收掉。production 零變動。
前次 asks
- P2 git stub 認不出
reviewDiff前綴 — 關。我在 box 用新的分類式跑實際 argv：merge-base →merge-base、-c … --attr-source=<tree> diff …→diff、加--stat→stat、diff --name-only -z→changed、hash-object/rev-parse→''、-C <dir> diff→diff。全對。你的寫法比我的 suggestion 好（-C那點成立）。唯一盲點是分開寫的--attr-source <tree>會分錯，但src/shared/repo.ts:38用的是=形式，不構成問題。 - fail-before（c0180c5 上 6/33 紅）加修正後轉綠，證據夠。
新 findings
P3 — 連線設定錯誤現在走「review failed」路徑，連 check 與 PR comment 一起發
這是 c0180c5 merge 帶進來的語意變化，這次 commit 只是把它釘進 connections-integration.test.ts:196。因為 base 的 resolveConnectionOptions(..., !isDocsOnlyFastPath(bundle)) 依賴 bundle，現在只能在 owned-check 的 try 裡跑（github.ts:1794）。後果：connections.json 壞掉時，active GitHub review 會先建 pending check，再補紅色 Needlefish: review failed，再發 PR comment，附預設的「Re-run the review.」。
- 錯誤訊息我查過
connections.ts全部的 throw，都不帶路徑或憑證，公開到 PR comment 沒有外洩。 - 但 rerun 修不好設定錯誤，提示會誤導人；而且每次失敗都吃一輪 recovery。修好設定後同一個 head 跑到第 3 次，會撞
RecoveryLimitError，只能用 recheck。 - 我不要求改 production。要嘛在 PR 描述明講接受這個行為，要嘛另開 issue 處理「設定錯誤不該建 check」，例如在
createPendingCheck前先 parse/驗證 connections 檔，bundle 相依的 model 檢查留在後面。 - 另外 GitHub 路徑的錯誤前綴現在是
needlefish review failed:，local/pr 仍是needlefish:（同檔 :185）。如果有人 grep 前綴，這是破壞性變更，描述裡也要寫。
P3 — PR 描述過時
「相對最新 base，本 PR 只改 adapter 與 posting tests」已經不對了，現在還改了 connections-integration.test.ts，也改變了 GitHub 路徑的連線錯誤行為。posting 179/179 是 stub 修好前的數字，請換成你 thread 裡回報的實測結果（相關測試 246/246；全套 2070/2104，8 個失敗 main 上也會出現），並列出那 8 個測試名稱，證明跟這個 PR 無關。
CI / merge
- draft；
needlefish-ci@ 5dd168a 還在 in_progress，review還在 queued。 - 沒有 code blocker。等 CI 綠、描述更新後就能 ready。Class R gate 照描述仍是 pending。
— 老審
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
needlefish:changes-requested
frankekn
left a comment
frankekn
left a comment
There was a problem hiding this comment.
老審 staff review @ b1544ee（delta 5dd168a..b1544ee）
總評
一個 commit，方向對。在 createPendingCheck 前先跑一次 resolveConnectionOptions(opts, repoPath, process.env, false)，壞掉的 connections.json、找不到的 --connection、不合法的 entry 都在建 check 前就丟出，走 CLI 頂層的 needlefish: 前綴、exit 1、不發 check 也不發 comment。credential 綁定仍留在 bundle 之後，docs-only fast path 不受影響。上次的 P3 基本收掉，也附議 Needlefish 的 P2（connection 設定失敗吃 recovery），但只收了一半，見下。
前次 asks
- P3 連線設定錯誤走「review failed」路徑，連 check 帶 PR comment 一起發 — 關（parse/選擇類錯誤）。
connections-integration.test.ts:194跟 base 一樣回到needlefish:前綴，而且從 regex 收緊成整句 equal，比 base 更嚴。破壞性前綴變更這點也一起消失。 - 新測試
connection setup failures leave the automatic recovery budget intact的形狀對：兩次壞 JSON 不產生任何 POST，修好後第三次照樣跑到 runner，沒撞 recovery limit。這正是 Needlefish P2 描述的情境。 - P3 PR 描述過時 — 仍開。「相對最新 base，本 PR 只改 adapter 與 posting tests」、
posting 179/179都沒更新；這次又多了「建 check 前先驗證連線設定」這個行為，描述裡沒寫。
新 findings
P3 — credential 類設定錯誤仍然吃 recovery round，也仍然叫人「Re-run」
bindCredentials=false 會跳過 new AcpEnvironmentAuth(...) 和 assertCompatible(...)（connections.ts:153-154）。所以下面兩種情況照樣在 owned check 的 try 裡失敗（github.ts:1798）：
auth.env指到的 env var 沒設、是空字串或前後有空白（connection-auth.ts:52-53）。- 同時設了
NEEDLEFISH_RUNNER_ENV_PASSTHROUGH（connection-auth.ts:64-65）。
後果跟這個 commit 要修的一樣：先建 pending check，再補紅 check 和 PR comment，然後扣一輪 recovery。CI 裡忘了設 secret、跑兩次，補好 secret 後同一個 head 第三次會撞 RecoveryLimitError。這是最常見的設定錯誤，比壞 JSON 常見得多。
我理解 credential 要等 docs-only 判定後才能綁，所以不要求硬搬。二選一：
- 在描述裡明寫「parse/選擇類錯誤在 check 前；credential 缺失仍在 check 後、會消耗 recovery」，並加一個測試把這個現狀釘住（缺 env var 跑兩次，第三次預期撞 limit 或預期不撞，選一個寫死）。
- 或者把
assertCompatible裡的 passthrough 衝突也搬到 check 前。這只看 env，不看 bundle。代價是 docs-only run 也會因為這個衝突失敗，要不要接受由你決定。
CI / merge
- 還是 draft；
needlefish-ci@ b1544ee in_progress，reviewqueued。 - 沒有 code blocker。等 CI 綠、描述補上上面兩點後就能 ready。Class R gate 照描述仍是 pending。
— 老審
This comment has been minimized.
This comment has been minimized.
Missing auth.env sources and passthrough conflicts now fail before a check exists, so they no longer spend a recovery round. A docs-only head, judged on the same merge-base and changed files as the bundle, still defers credentials.
frankekn
left a comment
frankekn
left a comment
There was a problem hiding this comment.
老審 staff review @ 2a53b5c（delta b1544ee..2a53b5c）
總評
一個 commit，比我給的兩個選項都做得多，做法對。createPendingCheck 前多跑一次 resolveConnectionOptions(..., true)，實際建構 AcpEnvironmentAuth 並呼叫 assertCompatible，所以缺 secret 和 passthrough 衝突都在建 check 前以 CLI 錯誤結束。docs-only 例外用 docsOnlyHead 判斷，走的是跟 prDiffFromShas 一樣的 merge-base 加 changedFiles(mergeBase, head)，判斷結果跟後面 :1814 的 isDocsOnlyFastPath(bundle) 一致。git 失敗時當作非 docs-only，保留 credential 錯誤，方向是 fail closed，對。
我看過 AcpEnvironmentAuth：constructor 只把值存在 private field，不寫 env、不寫檔，preflight 建出來的物件直接丟掉沒有副作用。先跑 false 再跑 true 也不是多餘：parse/selection 錯誤永遠不會被 docs-only 例外吞掉。isDocsOnlyFastPath 收窄成 Pick<Bundle, "changedFiles"> 也乾淨。
前次 asks
- P3 credential 類設定錯誤仍吃 recovery、仍叫人 Re-run：關。新測試的形狀正確：缺 env 跑兩次沒有任何 POST，補上後不加
--recheck第三次照樣跑到 runner。docs-only 的反向證明由github: docs-only auth validates configuration without requiring credentials（connections-integration.test.ts）負責，它走runGithub而且期待 exit 0，docsOnlyHead判錯就會紅。 - inline：
:1728註解說太滿：關。新註解跟實際行為一致。 - Needlefish P2（connection-auth 失敗吃 recovery budget）：上次附議為半收，這次完整收掉，附議。
- P3 PR 描述過時：仍開。「相對最新 base，本 PR 只改 adapter 與 posting tests」不對了，現在還動了
src/core/review.ts；posting 179/179也舊了（author 回覆寫 442/442）；「connection 解析、選擇與 credential 在建 check 前驗證，docs-only head 例外」這個行為描述裡完全沒有。merge 前補。
新 findings
沒有 blocker。兩點小的，都不擋：
- **nit：passthrough 衝突沒有 github 路徑的測試。**新測試只釘缺 env。同樣的形狀換成設
NEEDLEFISH_RUNNER_ENV_PASSTHROUGH跑兩次、拿掉後第三次跑到 runner，成本很低，能防日後有人把assertCompatible搬回 bundle 之後。 - **行為變化記一筆，不用改：**非 docs head 上，如果 merge-base 或 diff 收集會失敗、同時又缺 credential，現在得到的是 credential CLI 錯誤（沒有 check），而不是 early collection failure 的紅 check。credential 錯誤是真的、也可行動，修好後下一輪才會碰到 collection 錯誤，可以接受。author 回覆提到的 empty diff 加缺 credential 是同一類。
CI / merge
- 還是 draft；
needlefish-ci@ 2a53b5c in_progress，reviewqueued。 - code 面我這邊沒有剩下的 ask。CI 綠、描述補齊後可以 ready。Class R gate 照描述仍 pending。
— 老審
|
Needlefish re-review @ 2a53b5c — 🔍 1 not re-found (code changed) · ❌ 0 still open · 🆕 0 new → LGTM |
github-actions
Bot
added
needlefish:pass
Why
可信且仍開啟的 PR 在 diff 或 metadata 收集失敗時，原本會在建立 check 前離開。現在會完成本次持有的 Needlefish check，顯示失敗與重新執行提示，並輸出 exit 1、operational false，模型呼叫為零。
Scope
isDocsOnlyFastPath的參數放寬為Pick<Bundle, "changedFiles">。Tradeoffs
GitHub API 持續失敗時只能保留診斷，不保證成功建立或完成 check。同步收集期間的強制終止仍受既有 subprocess 限制。empty diff 加上缺憑證時，現在在建 check 前以憑證錯誤結束，不再進到 empty diff 的失敗 check。
Verification
Refs #236
Refs #213
Stacked on #264. 本 PR 只處理 early collection failure，未完成這兩個 issue 的 binary coverage 工作。
Owner 在 2026-10-18 豁免 Class R gate。付費 eval 與 live posting canary 都沒有跑。合併依據是目前 head 上綠的 needlefish-ci 和 Needlefish LGTM。