Repository navigation
Conversation
|
@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.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| report. Owned processes were stopped and checked absent. No resume, replacement | ||
| campaign or post-revocation model call was used to rescue the result. | ||
|
|
||
| The subsequent local repair supplies a critic-only structured output contract |
There was a problem hiding this comment.
P1 Run the required Class R gate before shipping this pipeline
This PR changes critic output admission and failed-review accounting, which changes model inputs/output acceptance. The added record explicitly states that the repair requires a new Class R source/method/criteria seal and that no formal R/D qualification or canary succeeded. Project policy requires reverting a pipeline/prompt change when its required gate fails or is absent; merging this candidate would ship an unqualified recall-affecting pipeline.
Fix: Revert the recall-affecting pipeline changes, or complete and pass the required production-lane full Class R x3 gate with sealed holdouts before merge.
Validate: Run the predeclared full production-lane Class R x3 gate on this exact source, including sealed holdouts and required confirmation controls.
| } | ||
|
|
||
| function hasUnverifiableInvocationEnv(args: RunArgs): boolean { | ||
| if (args.runner === "codex" && args.publicLauncherArtifact !== null) return Object.hasOwn(args.env, "HOME") || Object.hasOwn(args.env, "USERPROFILE") || runnerEnvironment(args).includes('"<required>"') || runnerEnvironment(args).includes('"<redacted>"'); |
There was a problem hiding this comment.
P2 Mark Codex reports with private config inputs as non-resumable
For a Codex run with --public-launcher-artifact and an ordinary process HOME, runnerConfigIdentities reads ~/.codex/auth.json and config.toml (lines 735-741), but this branch returns false unless HOME was supplied through --env or the public projection contains a marker. writeReport therefore emits privateEnvironment: false (line 1247). On a later process, resume accepts old draws because its consumer guard only compares the public runnerEnvironment and privateEnvironment state (lines 456-460); it cannot see a changed private Codex config/auth source. This contradicts the new contract that reports requiring private config/auth inputs cannot resume across processes, and can combine draws from different effective runner configurations.
Fix: Expose whether private Codex config/auth inputs were consulted and make hasUnverifiableInvocationEnv return true for that case, so reports are marked private and resume/compare reject them; alternatively persist an opaque local-only configuration identity and require it to match before reuse.
Validate: Add a regression that creates a public-artifact Codex report using the default HOME, changes ~/.codex/config.toml between processes, and verifies resume refuses the prior report.
|
@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 @ f5e238a（delta 2742b12→f5e238a）
Draft · 3 commits（c5b9632、6d7b4d1、f5e238a），11 個檔，+72/−22，只動 Pi identity、public quote schema 和測試。CI：f5e238a 0 個 check run。 PR 又變成 mergeable: false / dirty（落後 main 8 commits，有衝突），pull_request workflow 不會跑。上一個 head 6d7b4d1 的 needlefish-ci 是 failure：2962 tests 只掛 1 個，就是 compareWeekly: matching implicit model and effort still compare。f5e238a 修的正是這條，但沒有任何 CI 在 f5e238a 上證明。
P0: 0 · P1: 0 · P2: 0 新 · P3: 2 新（AR、AS）
AR（P3）：critic schema 失敗測試的清理斷言放太寬
critic-output-schema.test.ts:204 從「managed 根目錄為空」改成「沒有子目錄」。Linux 上 owner/sweep lock 檔會留在根目錄，放寬有理由。可是現在任何留在根目錄的一般檔案都會被放過。schema 檔和 log 另外有兩行斷言守著，其他東西沒有。要求： 只排除 temp-lifecycle.ts 那兩種 lock 檔名（見 inline suggestion）。.ready-* 會在 acquireLock 的 finish 刪掉，.needlefish-owner.json 在子目錄裡，都不用排除。
AS（P3）：quote pattern 用了 lookahead，跟同一份 schema 其他 pattern 不一致
review-result.v1.schema.json:308 的結尾是 (?![\s\S])。JSON Schema 的 pattern 是 ECMA-262 語意，沒有 m flag 時 $ 本來就是絕對結尾。我在 node 用 14 個 case 比過（含 \u2028、\u3000、CRLF、surrogate pair），^\S(?:[^\r\n]*\S)?$ 跟現在的寫法判定完全相同，也跟 review-result.ts:241 的 parser 一致。lookahead 只對 Python 這類 $ 會停在最後一個 \n 前的引擎有差，可是 RE2 系的 validator（例如 Go）根本編不過 lookahead。同一份檔案 299、456 行也都用 $。不擋 merge，挑一種語意統一就好；真要防非 ECMA 引擎，就三個 pattern 一起處理。
舊項狀態
- AQ（Pi identity）：關。 producer 端在
effectiveInvocationEnv補PI_MODEL，在parseArgs補PI_DEFAULT_THINKING；reader 端hasResolvedModelIdentity拿掉了 Pi 預設。resolvePiThinking和parseArgs共用同一個常數，Pi 也沒有別的 env 能改 thinking，所以記下來的 effort 就是實際跑的值。eval.test.ts鎖了預設、env、explicit 三種情況。副作用要知道：舊的 Pi 報告（effort 為 null）現在 resume 會因為 effort 不同被拒，weekly compare 也會 withhold。這是預期行為，body 已寫明 readers no longer infer。黑名單 nit（default／n/a）仍開，不擋。 - AM 尾巴（#298 部署順序缺口）：關。 body 補了。最後一段有三處漏空格（
All39、reviewed6d7b4d1、The6d），順手修。 - needlefish 的 quote schema P2（
schemas/review-result.v1.schema.json:301）：附議，已修。 邊界用的\S和String.prototype.trim()在 ECMA-262 是同一組字元；parser agreement 測試補了 12 個 case。 - AN（P2，critic schema 沒有 capability gate）、AO（schema 與 parser 一致性測試）、AP（accounting 版本）：仍開。 這個 delta 沒碰，thread 上也沒回覆。AN 仍是我最在意的一項：不支援 json_schema 的 route 會讓每一次 critic 失敗，而且不會降級。
- AP 附帶的
review.ts:1115註解 suggestion：仍開。 - AJ、AK、AL、AG、AD、AA、AB、F、H、I、J、N、O、P、W、Y、Z、
parse_failed分桶、review.ts:1164nit、#297 C：仍開，沒碰。 - installed 0.4.10-self.1 hosted review 的
malformed critic outputNOT_PASS，以及完整 Class R ×3、hosted review、canary：仍是 merge obligation。
needlefish @ 6d7b4d1 新提的 7 項
- P2
src/adapters/local.ts:305（rename 的 numstat）：不附議，判斷錯了。 它把git diff --numstat -z的 rename 格式套到git apply --numstat -z上。我在 box 用 git 2.47.3 重現：git mv再改內容，用UNCOMMITTED_DIFF的參數產 patch（確實是rename from／rename to），再照capturedPatchMetadata的做法在 bare repo 跑git apply --numstat -z。輸出是1\t0\tnew.txt\0，--reverse是0\t1\told.txt\0，都是單一路徑，沒有空路徑。正反兩次 apply 剛好把新舊路徑都收進來。可以回覆後 resolve。 - P2
docs/connections.md:350：部分附議，降成 P3。 352 行的 synopsis 寫成每個 operation 都能帶--repo，args.ts:220對review會拒絕。不過 430 行已經寫明 review 不收--repo，所以只是 synopsis 不精確。synopsis 改成只列 list/edit/validate/apply/inspect 就好。 - P1 Class R gate（
eval/RESULTS.md:2413）：附議。 跟我列的 merge obligation 是同一件事。 - 其他四項（
review.yml:195和action.yml:103的 stale head check、eval/run.ts:826的 private Codex config resume、review-result.ts的 URL 正規化）：這輪沒驗，不表態。
結論
這個 delta 很乾淨，AQ 修得對，沒有新的程式 blocker。擋路的是流程：又跟 main 衝突，f5e238a 沒有任何 CI；AN 還沒處理；Class R ×3、hosted review 和 canary 都還沒跑。合併就緒：否（Draft、衝突、無 CI、AN、gate 未跑）。
— 老審
|
@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 @ 3376a4b（delta f5e238a→3376a4b）
Draft · 11 commits。其中 #262（capability preflight）的 7 個 commit 加上它的 merge 是從 main 進來的，已在 main 上審過，這裡不重審。PR 自己的改動只有 047c100（measurement 綁 job check）、194c734（call accounting 版本）和 merge 3376a4b，共 18 個檔。CI：needlefish-ci、review 都還在跑，我送出時這個 head 還沒有結論。衝突解掉了（mergeable: true），現在的 blocked 是 Draft 和 check 未完成造成的。
P0: 0 · P1: 0 · P2: 1 新（AT）· P3: 1 新（AU）
AT（P2）：047c100 之後，collector 在正式 workflow 上綁不到任何 receipt
eval/tools/review-measurements.ts:188 現在要求 job 的 check_run_url ID 等於 Needlefish check 的 ID。正式流程裡這兩個 ID 永遠不同。src/adapters/github.ts:975 的 createPendingCheck 會自己 POST 一個新的 Needlefish check run，而 job 自己的 check run 是 Actions 建的，名字是 review（review.yml:121 沒設 name）。結果每一筆真實 receipt 都會落到 check_job_association_unverified，producer_attested 只會出現在測試裡。測試 fixture 把 job 的 check_run_url 改成 check-runs/30，讓它等於 Needlefish check，這在 GitHub 上不會發生。
needlefish 在 :187 提的偽造問題我附議，fail closed 的方向也對。問題是修完之後，adoption 和 latency 的 join 在 production 會全部變成 unknown。README 那句「A separate self-created Needlefish check remains unknown」讀起來像邊界情況，實際上是 100% 的情況。thread 上那句「Receipts now require the exact check_run_url ID」也沒講到這個後果。
要求（二選一）：
- 接受目前沒有可證明的綁定：在 PR body、
eval/tools/README.md和 RESULTS 直接寫明，現行review.yml產出的 receipt 全部會是 unknown，weekly adoption/latency 不會有 producer-attested 的列。再補一個照真實拓撲的測試（job check 名為review，Needlefish check 另有自己的 ID），斷言結果是 unknown，並把 happy-path fixture 標明為假設的拓撲。 - 要保留 attested：先找到一個 GitHub 平台本身能證明的關聯，再改 collector。run ID、job name、
external_id都還是 producer 自己的說法，不算證明。
這不影響 code 的正確性，可是 measurement lane 實際上能做到的跟描述不符，merge 前要講清楚。
AU（P3）：沒有任何程式會寫出 callAccountingVersion: 1
eval/shared/call-accounting.ts:4 接受 1 | 2，RESULTS 也寫「Explicit version 1 reports can compare with version 1」。可是版本欄位是這個 delta 才加的，producer 只寫 2，舊報告沒有這個欄位，會被拒。1 只能靠手改 JSON 產生。要求： 要嘛拿掉 1（型別、version()、RESULTS 那句一起改），要嘛寫明誰在什麼情況下會把舊報告標成 1。另一個 nit：weekly-compare.ts 用 callAccountingError(latest, latest) 把比對函式當成驗證函式用，讀的人得停下來想。可以另外 export 一個檢查版本是否支援的 helper。
舊項狀態
- AP（accounting 版本）：關。
writeReport會寫callAccountingVersion: 2，resume、compare和 weekly 三處都拒絕混用或沒記錄的版本。版本不合時 tier-1 alert 仍然保留，weekly-compare.test.ts有鎖住。副作用：merge 後第一次 weekly 會因為前一份報告沒有版本而跳過 regression 比較。RESULTS 已寫明，可以接受。 - AP 附帶的
review.ts註解：關。 1116 行補回了說明。 - needlefish 在
review-measurements.ts:187的 P2：問題附議，但修法過頭，見 AT。 - AN（critic schema 沒有 capability gate）：仍開。 #262 的 capability preflight 管的是 repository-read（promptOnly/unknown），不管 route 收不收
json_schema。這仍是我最在意的一項。 - AO、AR、AS、AQ 黑名單 nit、AJ、AK、AL、AG、AD、AA、AB、F、H、I、J、N、O、P、W、Y、Z、
parse_failed分桶、review.ts:1164nit、#297 C、docs/connections.md:352synopsis：仍開，這輪沒碰。 - PR body 那三處漏空格（
All39、reviewed6d7b4d1、The6d）還在。 - Class R ×3、installed-release hosted review 和 canary：仍是 merge obligation，附議 needlefish 的 P1。
結論
194c734 很乾淨，AP 收掉了。047c100 方向對，但讓 measurement collector 在 production 綁不到任何東西，要嘛寫清楚，要嘛換一種證明方式。合併就緒：否（Draft、CI 未完成、AN、AT、gate 還沒跑）。
— 老審
| const matches = allJobs?.filter((item) => item.name === query.jobName); | ||
| const match = matches?.length === 1 ? matches[0] : null; | ||
| if (!match || !id(match.id) || !id(match.run_id) || String(match.run_id) !== workflow.runId || match.run_attempt !== workflow.runAttempt || jobCheckId(match.check_run_url, query.repository) === null) binding = unknown(allJobs === null ? "jobs_incomplete" : "job_binding_conflict"); | ||
| else if (jobCheckId(match.check_run_url, query.repository) !== check.id) binding = unknown("check_job_association_unverified"); |
There was a problem hiding this comment.
AT（P2）：這個 ID 在正式流程裡不可能相等。createPendingCheck（src/adapters/github.ts:975）會 POST 一個新的 Needlefish check，job 的 check_run_url 指向的是 Actions 自己為 review job 建的 check run。所以現行 review.yml 的每一筆 receipt 都會停在這裡，變成 check_job_association_unverified。fail closed 沒問題，可是請在 README、RESULTS 和 PR body 寫明 production 上目前沒有 attested 的列，並補一個照真實拓撲的測試（兩個不同 ID，斷言 unknown）。不然請換一個 GitHub 平台本身能證明的關聯。
— 老審
| export const CALL_ACCOUNTING_VERSION = 2; | ||
|
|
||
| export interface CallAccountingMetadata { | ||
| readonly callAccountingVersion?: 1 | 2; |
There was a problem hiding this comment.
AU（P3）：沒有任何 producer 會寫出 1，舊報告是沒有這個欄位。要嘛拿掉 1（連同 version() 和 RESULTS 那句），要嘛寫明誰會把歷史報告標成 v1。
— 老審
This comment has been minimized.
This comment has been minimized.
| ? `${process.env.GITHUB_SERVER_URL}/${process.env.GITHUB_REPOSITORY}/actions/runs/${process.env.GITHUB_RUN_ID}` | ||
| : undefined, | ||
| name: "Needlefish", head_sha: headSha, status: "in_progress", | ||
| external_id: JSON.stringify(round), |
There was a problem hiding this comment.
P1 Keep check-run external IDs within GitHub's 40-character limit
JSON.stringify(round) contains the v2 field names plus a 36-character UUID roundId, so it is well over GitHub Checks' 40-character external_id limit. GitHub rejects the pending-check POST; createPendingCheck then returns null, and runGithub aborts delivery at src/adapters/github.ts:1786 before any review or terminal verdict is posted.
Fix: Use an API-compliant opaque external_id (<=40 characters) and retain the full round receipt in a separately bounded, validated field.
Validate: Add a GitHub API fixture rejecting external_id values over 40 characters, then run pnpm test.
|
@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 @ f81e2e4（delta 3376a4b→f81e2e4）
Draft · 3 commits（e4fbf9f、41d55e4、f81e2e4），10 個檔，+136/−33。動到的是 persisted URL 結構檢查、critic parser 收緊、author-skip 前重讀 PR、Bash 3 相容，還有 synopsis。CI：needlefish-ci 還在跑，review 還在排隊，我送出時這個 head 還沒有結論。
P0: 0 · P1: 0 · P2: 0 新 · P3: 2 新（AV、AW）+ 1 nit
這輪關掉的
- needlefish 在
review-result.v1.schema.json:453提的 URL 邊界：關，附議。 我在 box 上用同一條 regex 跑過：raw 的\n、尾端空白、:00080、U+00A0、U+2028 都會被拒；這些字串經 WHATWG 正規化後的href（含 IDN 轉 punycode、[::1]:443）全部通過。所以正規化過的合法輸出不會被誤殺。(?![\s\S])取代$是為了讓非 JS 的 validator 也不會放過尾端換行，方向對。 - needlefish 在
review.yml:206/action.yml:115提的 stale/closed skip check：關，附議。 POST 前重讀state和head.sha，讀不到就 fail closed，closed 或 superseded 就不發 check。GET 和 POST 之間還有一個很小的 race，這擋不掉，可以接受。 docs/connections.md:352synopsis：關。critic-output-schema.test.ts改成只過濾 lock 檔：這是 6d CI 失敗的修正，比原本只看目錄更嚴，OK。- needlefish 在
local.ts:305的 numstat 那條，作者用git apply --numstat -z的實際輸出反駁，論點成立。我沒有重現。
needlefish 在 src/adapters/github.ts:981 的 P1（external_id 40 字元）：不附議，但要拿證據收掉
GitHub Checks REST 文件沒有寫 external_id 有長度上限。40 字元的上限是寫在 actions[].description 上的，看起來 needlefish 把兩者混在一起了。這個 PR 上 3376a4b 的 Needlefish check 是 installed release 發的，external_id 是 11 字元的 run id，所以也拿不出「長 ID 在 GitHub 上可以用」的實證。另外要注意，action.yml 的 author-skip POST 用的也是 {v:2,prNumber,roundId,runId} 這個約 80 字元的 JSON，如果這個限制是真的，composite 那條路也會一起壞，而且在 -eo pipefail 下會讓 trust step 直接失敗。要求： 在 thread 上用一次真實 POST（scratch PR 就可以）或文件連結收掉。不要沒證據就改格式。
AV（P3）：critic parser 收緊了，但只有 Codex 有 output schema
review.ts:509-516 現在拒絕 checked 裡的空白項，也拒絕 residual 多帶的 key 或型別不對。可是 runCritic 只在 runner === "codex" 時傳 codexOutputSchema（review.ts:607）。Claude、Grok、Pi、ACP 的 critic 沒有 schema 約束，以前這些輸出會被 coerce 掉，現在重試 3 次後整個 review 失敗。PR body 也寫了 installed release 的 hosted review 曾經因為 malformed critic result 失敗，這個改動是往那個方向推的。要求： RESULTS 或 PR body 寫明這次收緊影響所有 runner，但 schema 只保護 Codex。Class R gate 至少要有一條非 Codex 的 critic lane，量 malformed 比率；如果 R 只跑 Codex，就把這個缺口寫成已知風險。這也讓 AN 更急了，見下面。
AW（P3）：URL regex 現在有兩份手抄
src/shared/review-result.ts:29 的 PERSISTED_HTTP_URL 和 schemas/review-result.v1.schema.json:454 的 pattern 是逐字複製的。現在的 corpus 測試只抓得到列出來的那些 case 的漂移。要求： export 這個常數，在 review-result-schema.test.ts 加一行斷言 PERSISTED_HTTP_URL.source 等於 schema 讀出來的 pattern。或者乾脆讓 parser 直接從 schema 取 pattern。
nit：Bash 3 測試在 Linux CI 上量不到 Bash 3
review-workflow-author-trust.test.mjs:551 用的是 /bin/bash。這在 macOS 是 3.2，在 Ubuntu CI 是 5.x，所以 CI 綠了也不代表相容 Bash 3。可以補一個靜態檢查，確認 action.yml 的 run script 裡沒有 mapfile、readarray、declare -A、${x,,}，或者把測試名稱改成只在本機有意義。
舊項狀態
- AN（critic schema 沒有 capability gate）：仍開，優先度上調。 因為 AV，非 Codex route 上 schema 和 parser 的落差現在會直接變成 review 失敗。
- AT（collector 在正式拓撲綁不到 receipt）：仍開，這輪沒碰，README、RESULTS、PR body 和測試都還沒動。
- AU（沒有 producer 寫
callAccountingVersion: 1）：仍開。 - AO、AR、AS、AQ 黑名單 nit、AJ、AK、AL、AG、AD、AA、AB、F、H、I、J、N、O、P、W、Y、Z、
parse_failed分桶、review.ts:1164nit、#297 C：仍開，這輪沒碰。 - PR body 那三處漏空格（
All39、reviewed6d7b4d1、The6d）還在。 - Class R ×3、installed-release hosted review 和 canary：仍是 merge obligation。
結論
這輪的邊界修正是對的，needlefish 那三條我附議並關掉。critic 收緊讓非 Codex runner 的失敗面變大，要寫清楚並放進 R gate。external_id 那條 P1 要用證據收，不要直接照改。合併就緒：否（Draft、CI 未完成、AN、AT、gate 還沒跑）。
— 老審
| if (!Array.isArray(raw.checked) || raw.checked.some((item: unknown) => typeof item !== "string" || !item.trim())) { | ||
| throw new Error("malformed critic output: checked entries must be non-blank strings"); | ||
| } | ||
| if (!Array.isArray(raw.residual_risks) || raw.residual_risks.some((risk: unknown) => |
There was a problem hiding this comment.
AV（P3）：這裡現在嚴格等同 CRITIC_OUTPUT_SCHEMA，可是 schema 只在 runner === "codex" 時才傳給模型（:607）。非 Codex 的 critic 以前多帶 key 或空白 checked 會被 coerce，現在重試 3 次後整個 review 失敗。請在 RESULTS/PR body 寫明影響面，R gate 至少放一條非 Codex 的 critic lane 量 malformed 比率，或把這列為已知風險。
— 老審
| @@ -22,6 +26,7 @@ import { | |||
| type JsonRecord = Record<string, unknown>; | |||
|
|
|||
| const LABEL = "malformed review result"; | |||
| const PERSISTED_HTTP_URL = new RegExp("^https?://(?:[^\\s:/?#@\\[\\]\\\\]+|\\[[0-9A-Fa-f:.]+\\])(?::(?:0|[1-9][0-9]{0,3}|[1-5][0-9]{4}|6[0-4][0-9]{3}|65[0-4][0-9]{2}|655[0-2][0-9]|6553[0-5]))?(?:[/?#][^\\s]*)?(?![\\s\\S])"); | |||
There was a problem hiding this comment.
AW（P3）：這是 review-result.v1.schema.json:454 pattern 的逐字複本。請 export 出來，在 schema 測試加一行 assert.equal(PERSISTED_HTTP_URL.source, <schema pattern>)，不然兩份遲早會漂。
— 老審
This comment has been minimized.
This comment has been minimized.
| git(["fetch", "--quiet", sourceRepoPath, options.targetHeadSha], sandboxPath); | ||
| const cloneSource = options.snapshot?.repoPath ?? sourceRepoPath; | ||
| const headSha = options.snapshot?.expectedHeadSha ?? options.targetHeadSha; | ||
| git(["clone", "--quiet", "--no-hardlinks", "--no-checkout", cloneSource, sandboxPath], cloneSource); |
There was a problem hiding this comment.
P2 Apply the review deadline to sandbox setup Git commands
Ordered routes use one absolute review deadline (src/core/review.ts:1111-1112), but sandbox clone/fetch calls omit remainingReviewMs. Their consumer therefore uses its default 30,000 ms timeout (src/shared/runner-sandbox.ts:562-571), so a session with budgetMs=1 can remain blocked in clone/fetch for up to 30 seconds before the next deadline check. This also applies to the LFS ls-files calls, which likewise omit the deadline.
Fix: Thread remainingReviewMs through every sandbox Git operation, including clone, fetch, checkout, remote removal, and gitNulList, and cap each subprocess timeout to the remaining shared budget.
Validate: Add a sandbox test with a blocking Git stub and a 1 ms review budget; verify review returns near the budget rather than after the 30 s Git timeout. Then run pnpm test -- src/shared/runner-sandbox.test.ts.
|
|
||
| Execute both base and head with the same controlled input. Assert correct base output and the exact head failure or unauthorized effect. Use fake credentials and local files only. For protocol containment, execute the real installed curl against a local file URL. No HTTP server is required. For auditor provenance, create a local Git repository with distinct trusted and proposal commits and execute the selected Python auditor. The tests do not contact any provider or run an eval model. | ||
|
|
||
| Use the existing loadFixture to verify real generated patch anchors. Check the catalog contains 101 fixtures, 15 Tier-1 positives, the exact seven-family counts, and seven new sealed holdouts. The existing 93 spec files remain byte-identical. Do not rewrite historical reports, README scores, prompts, matcher, scorer, qualification policy, or calibration tools. |
There was a problem hiding this comment.
P2 Correct the Phase 2 catalog validation contract
The newly committed verification contract tells operators to require 101 fixtures and says the prior 93 specs are byte-identical. The active catalog consumer instead loads 109 fixtures and asserts that count at eval/shared/tier-one-phase-two-fixtures.test.ts:169; additionally, this PR changes the pre-existing neg-hard-dead-code-delete spec. Following the documented contract will reject the current valid catalog or incorrectly treat its fixture identity as unchanged, undermining reproducible Phase 2 admission.
Fix: Scope the 101/unchanged claim to its frozen historical source, or update the current verification contract to the active catalog identity and explicitly record the modified fixture.
Validate: Reconcile the contract with the active catalog and run node --test --import tsx eval/shared/tier-one-phase-two-fixtures.test.ts.
| explain` 留言指令存在於本 repo 的 `.github/workflows/commands.yml`；已發 | ||
| 布的 composite action 不會安裝該 workflow。 | ||
|
|
||
| 原生 Codex 額度分類已驗證 CLI 0.163 的 JSON events。其他版本尚未取得 |
There was a problem hiding this comment.
P2 Document quota fallback as schema-based, not v0.163-only
This says only Codex CLI 0.163 is eligible for automatic quota fallback. In fact, nativeCodexQuotaExhausted accepts any nonzero-exit terminal JSONL stream with the recognized schema and exhaustion message, without checking a CLI version (src/shared/codex.ts:1104-1123); a recognized result then advances an ordered route at src/core/review.ts:1194. Thus users of a later compatible CLI are told fallback will not occur when it will. The English reference correctly says admission is not version-whitelisted.
Fix: State that 0.163 has native-event proof, while runtime fallback admission is based on strict terminal JSONL schema and exhaustion semantics rather than a version whitelist.
Validate: Review docs/reference.zh-TW.md against docs/reference.md:620-625, then run node --test --import tsx src/shared/codex-quota.test.ts src/core/review-ordered-connections.test.ts.
|
@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 @ cf6bd7e（delta f81e2e4→cf6bd7e）
Draft · 13 commits，大多是把 #257（incomplete-review delivery）的線 merge 進來，5 個檔 +77/−19。真正動到邏輯的只有 github.ts 的 hasNewerRound，其他是 docs（en/zh-TW exit code、required check、docs-only skip）和 5 條 dedupe 測試。CI：needlefish-ci 跑中，review 排隊，送出時這個 head 還沒有結論。
P0: 0 · P1: 0 · P2: 0 新 · P3: 1 新（AX）+ 附議 needlefish 3 條（降為 P3）
這輪的 delta
hasNewerRound：同 head 上比 receipt 新、而且已完成的 legacy check（external_idparse 不出 v2 round）現在也會讓 dedupe 失效。方向是 fail-safe，最壞是多跑一次 review，不會發錯 verdict。recoveryAttempt的priorRounds仍然略過 legacy check，所以不會吃掉 recovery 預算。OK。- 新測試：bound/legacy × terminated/failed/superseded/needs_human 都會重審，較新的 pass 仍蓋過較舊的 terminated。這組 case 選得對。
- docs：en 跟 zh-TW 這次對齊了，exit 0/1/130/143 和 same-head 沿用 verdict exit status 的寫法一致。沒問題。
AX（P3）：legacy 分支在 mixed-version 拓撲下會讓 same-head dedupe 永遠不命中
github.ts:1559。legacy 分支不分 conclusion，較新的 legacy pass 也算 newer round，現在沒有測試釘住這個 case。更要緊的是，這個 repo 先前 Needlefish check 是 installed release 發的，external_id 是 11 字元的 run id，parse 不出 round。只要 canary 或 R gate 期間 installed release 跟 candidate 在同一個 head 都發 Needlefish check，candidate 每次觸發都會重審。要求： (1) 補一條「較新的 legacy pass 也會重審」的測試，或者改成只認非 success 的 legacy check。兩種都行，但要明確選一個。(2) canary 和 R gate 的紀錄不要把這種重審算成 dedupe regression，在 RESULTS 寫一行說明。
附議 needlefish @ f81e2e4（review 5561816068）
runner-sandbox.ts:117sandbox git 不吃 review deadline：附議，P3。 我讀過 cf6bd7e：clone、fetch、checkout走的是git()，沒有傳remainingReviewMs。gitNulList也寫死timeout: 30000（:595）。只有runGit會用Math.min(30000, remaining)。clone 來源是本機，實際超時機會不大，但 ordered route 的共用 deadline 在這裡不成立。要求： 把remainingReviewMs傳進 clone、fetch、checkout、severSourceRemote和gitNulList，補一條 1ms budget 搭配阻塞 git stub 的測試。reference.zh-TW.md:417quota fallback：附議，P3。 zh-TW 寫「其他版本尚未取得自動額度 fallback 資格」，enreference.md:625-627寫的是「not a version whitelist」。nativeCodexQuotaExhausted（codex.ts:1104）確實沒有檢查版本，zh-TW 的寫法是錯的。照 en 改。eval/contracts/issue123-tier1-fixtures.md:24：部分附議，P3。 合約寫 101 fixtures、93 個舊 spec byte-identical。現在測試斷言 catalog 是 109、original 是 101（tier-one-phase-two-fixtures.test.ts:169），而且這個 PR 改了neg-hard-dead-code-delete/spec.ts（#84 的 closed-application contract）。不過這是標了日期的 preregistration，不要改原文。要求： 在文末加一段 addendum，寫明 101/93 是針對 7e5c473 的凍結基線，目前的 catalog identity 是 109 /cd483733f1d115ce，另外列出neg-hard-dead-code-delete的改動和原因。
external_id 40 字元 P1（github.ts:981）
立場不變：不附議。#264 上已經在真實 GitHub 看到 47 字元的 external_id，所以 40 字元不是硬上限。但約 80–96 字元的 v2 receipt 還沒有實證。這條併進 R gate 和 canary：第一次真實 pending-check POST 成功，就附 check id 在 thread 上收掉。
舊項狀態
- AV、AW、Bash 3 nit：這輪沒碰，仍開。
- AN（仍開，優先度維持上調）、AT、AU：仍開。
- AO、AR、AS、AQ、AJ、AK、AL、AG、AD、AA、AB、F、H、I、J、N、O、P、W、Y、Z、
parse_failed分桶、review.ts:1164nit、#297 C：仍開。 - PR body 三處漏空格（
All39、reviewed6d7b4d1、The6d）還在。 - Class R ×3、installed-release hosted review 和 canary：仍是 merge obligation。
結論
merge 進來的 dedupe 修正是對的，而且是 fail-safe。唯一要處理的是 legacy check 在 mixed-version canary 下的行為，要嘛測試釘住，要嘛收窄。needlefish 新提的三條都成立，但都是 P3。合併就緒：否（Draft、CI 未完成、AN、AT、gate 還沒跑）。
— 老審
| if (typeof check.id !== "number" || check.id <= receipt.checkId || check.head_sha !== state.headSha || | ||
| !isRecord(check.app) || check.app.id !== receipt.appId || check.name !== "Needlefish") return false; | ||
| const round = parseCheckRound(check.external_id); | ||
| return round ? round.prNumber === prNumber : check.status === "completed"; |
There was a problem hiding this comment.
AX（P3）：legacy 分支不看 conclusion，所以較新的 legacy pass 也會讓 dedupe 失效，這個 case 目前沒有測試。這個 repo 的 installed release 發的 Needlefish check，external_id 是 run id，parse 不出 round。canary 或 R gate 期間兩個版本只要在同一個 head 都發 check，candidate 每次觸發都會重審。請二選一：補一條「較新的 legacy pass 也重審」的測試，或改成 check.status === "completed" && check.conclusion !== "success"。另外在 RESULTS 註明這種重審不算 dedupe regression。
— 老審
|
Needlefish re-review @ cf6bd7e — ❔ 3 undetermined · ❌ 0 still open · 🆕 2 new → CHANGES REQUESTED |
|
|
||
| Historical reports and the 4f95 NOT_PASS/EARLY_ABORTED evidence are unchanged. | ||
| This metadata repair does not alter model inputs, scoring arithmetic, scorer | ||
| identity or fixture answers. The combined pipeline change remains Class R and |
There was a problem hiding this comment.
P1 Complete the required Class R gate before merging
The changed record states that the combined pipeline is Class R and still lacks its current-source gate. The documented prior campaign was early-aborted after 11 terminal-invalid outputs exceeded its maximum of 1, so it cannot qualify this source. Merging would ship recall/output-admission changes without the required full x3 sealed-holdout evaluation.
Fix: Revert the recall-affecting changes, or complete and pass the required current-source production-lane Class R gate before merge.
Validate: Run the sealed full production-lane Class R x3 gate on this exact source, including holdouts and required confirmation; verify its report is complete and passes all declared thresholds.
| and captured job key must also match the supplied contract. The exact display | ||
| name must uniquely identify a job in that run attempt. Named and matrix jobs | ||
| require their rendered display name. Duplicate names remain unknown. A workflow merge-ref SHA is reported as | ||
| `workflowRefSha`; it never replaces `targetHeadSha`. The Actions job check ID |
There was a problem hiding this comment.
P2 Preserve separate job and Needlefish check identities
The new public contract says the Actions job check ID is separate from the self-created Needlefish check ID. For that documented case, the collector's equality guard at eval/tools/review-measurements.ts:188 marks the association unknown; eval/tools/review-measurements.ts:206 then emits evidenceStatus: "unknown" rather than the documented delivered evidence.
Fix: Carry and validate an explicit association to the workflow job check instead of requiring the two distinct check IDs to be equal.
Validate: Add a collector fixture with distinct Needlefish and job check IDs that are explicitly associated, assert producer_attested/delivered, then run node --test --import tsx eval/tools/review-measurements.test.ts.
Result
Integrates fixed local snapshots, ordered and session routes, conservative delivery, bounded literal context, prompt substitution and public measurements into one qualification candidate. Advanced desktop changes remain in #294.
The critic returns keep/drop decisions for candidate refs. The pipeline restores original finding content and locations, validates decisions, preserves model residual decisions and keeps system failures blocking. The new critic output schema constrains the JSON shape. Failed reviews retain emitted logical-call accounting. Grok reports capture their effective model before attestation; readers require a recorded or explicit identity.
This head incorporates current main and the merged temporary-allocation cleanup and native terminal-order fixes. The accounting fixture now supplies isolated, literal fake auth and blocks reads outside its owned test root.
Validation
Remaining acceptance
Gate class R. The immutable 4f95 campaign ended NOT_PASS, early-aborted at 114/327 with 11 final-invalid results above its sealed maximum of 1. Its reports and raw evidence remain unchanged; it cannot be resumed or transferred to this source.
A fresh full x3 production-lane Class R gate including sealed holdouts, applicable D properties/drift controls, full hosted PR review, and an isolated installed canary with automatic rollback remain required. The earlier hosted review failed on the installed release with a malformed critic result. New-source schema capability and campaign execution remain held until the reviewed method and exact source/runtime/dependency bindings are ready. No production installation or pin changed.
This Draft remains NOT_MERGE_READY. Constituent issues retain their own acceptance requirements.
Refs #81, #84, #123, #149, #152, #193, #207, #213, #220.
Current contract and CI repairs at 6d7b4d1
The schema-failure tests now distinguish removed managed attempt directories from Linux lifecycle coordination lock files. Both failure tests and seven allocation-failure controls passed on Ubuntu; 23 schema/persisted-result tests passed locally. The public quote schema now rejects surrounding whitespace and terminal LF/CR/CRLF/Unicode separators exactly where the persisted reader already rejects them; all nine published-schema tests passed after the final correction.
New Pi eval reports capture the effective default model and thinking effort at production. Readers no longer infer missing historical Pi identity from mutable package defaults. The existing Pi default native invocation remains unchanged. All 141 attestation/runtime/eval cross-consumer tests passed. Pinned pnpm10.34.4 check and lint passed. Normal independent review of the complete ten-file repair delta finished exit0 with no actionable findings. This does not claim a new full-suite or formal evaluation pass.
Current-head hosted CI/review must still pass. The Class R full production-lane x3 gate with sealed holdouts, source-bound schema capability, installed-release hosted review and live canary remain merge obligations. The old 4f95 NOT_PASS/EARLY_ABORTED report is unchanged and cannot qualify this source. The #298 deployment-order gap remains open: an older installed binary may still reroute contradictory streams, and hosted 0.4.10-self.1-12789bf PR/head identity output is not yet verified. No deployment, current/pin change, real schema-capability call or new R/D campaign occurred in this repair.
Current test-only follow-up at f5e238a corrects the two remaining weekly Pi assertions that expected a reader to invent missing default identities. It explicitly rejects unrecorded effort/model and accepts a fully recorded Pi identity. All39 weekly comparison tests passed. Production bytes are identical to the reviewed6d7b4d1; no new full-suite, schema capability, evaluation or canary result is claimed. The6d CI failure is retained and current-head hosted checks must pass.
2026-10-18 compatibility and current-main repair
2026-10-18 persisted, critic and live-delivery boundaries
Current main 3046b30 integration
Preserve #257 delivery/exit rules and Frank's revised wording, alongside binary coverage and receipt-bound delivery. A newer owned completed legacy check without round metadata invalidates an older pass; explicit PR-bound rounds retain their association checks. Four regression controls failed before the fix. All 327 posting/exit/cached/fallback/reconcile tests, typecheck and lint passed after integration. The complete merge delta passed independent P2 review with exit 0 and no actionable findings. Formal qualification remains held.