Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe Windows Codex CLI inspection path now reports Codex CLI inspection reporting
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: 🔵 Low · up to The change has bounded impact, with remaining concerns limited to regression detection and documentation clarity rather than the runtime implementation. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
리뷰 · 우선순위 52 / 80설명 이 PR은 Windows에서 고치는 방법은 새 기능을 넣는 게 아니다. 타입 지금 라인 591 - 현재 tests/codex-integration/codex-cli-install-provenance.test.ts - Windows 테스트가 reason뿐 아니라 전체 report shape와 docs-site/.../reference/cli/agents.md (8 locale) - 설명 문장이 길어져 읽기 부담이 늘었지만, 동작과 문서가 같이 움직이는 쪽이라 범위는 맞다. 번역 품질은 기계적으로 맞춰 둔 수준으로 보이며, 의미 어긋남은 눈에 띄지 않는다. PR 베이스 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
275f3f8 to
0002656
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
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. |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed head 0002656. The production change is the reason on the no-observed-candidate Windows return. It does not add filesystem inspection, candidate execution, configuration writes, shim repair, or process control. The Windows regression observes the full unknown/unmanaged report and zero filesystem calls; the POSIX relative-config control preserves its existing reason but is not evidence about persisted POSIX state. Candidate-present lexical classification is unchanged. I independently verified author CI 34438801853 succeeded at this exact head. No new blocker found in this bounded diagnostic correction; sponsoring it for repository gates, not waiving those gates or approving the wider updater. #2811 should remain open. No local product execution was used.
Ingwannu
added
the
maintainer-sponsored
0002656 to
a514091
Compare
Ready-for-review evidenceRebased onto the current Local run — No unresolved Codex or CodeRabbit findings on this PR. |
a514091 to
58ea8b6
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58ea8b6279
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
abhisheksharma2411
left a comment
abhisheksharma2411
left a comment
There was a problem hiding this comment.
The distinction is the right one — "I did not look" and "I looked and found nothing" are different answers, and #2811 is someone reading the second when the truth was the first. 17 pass / 2 skip / 0 fail on the head.
The thing I most wanted to check
windows_inspection_deferred is not a new reason — it is already emitted at cli-install-provenance.ts:632, on the path where a candidate was observed but deeper inspection is skipped. So after this change one reason string covers two states, and your description says consumers tell them apart via candidateAvailable / candidateSource.
That holds, and I traced it rather than assume: the only consumer of inspectCodexCliInstall is src/cli/codex-cli-update.ts:93, and it prints both fields adjacently:
candidate: ${report.candidateAvailable ? "yes" : "no"}
reason: ${report.reason}
So the two deferrals surface as candidate: no / windows_inspection_deferred and candidate: yes / windows_inspection_deferred. Nothing in the tree switches on reason, so there is no branch to break — the overload is only a readability question, and the adjacent field answers it.
What I liked
The POSIX control is the part that makes this reviewable. Changing a reason string is exactly the kind of edit where the platform you did not touch quietly changes too, and a POSIX run with no observed candidate retains candidate_unavailable pins that it didn't. Asserting calls === 0 in both is the other half — the zero-I/O boundary is the premise of the whole argument, so a test that proved the new reason while silently reading a file would prove the wrong thing.
Widening the Windows assertions from two fields to nine is also worth it here: the claim is "everything else is unchanged", and previously only candidateAvailable and shim.status were holding that.
One small thing
The renamed test is now Windows reports a deferred inspection rather than an absent candidate, which describes the reason field — but the assertion that made the old name true (expect(calls).toBe(0), i.e. does not read persisted candidate state) is still in there and is still the load-bearing one. The old name is the invariant; the new name is this PR's symptom of it. Not worth a push, but if you touch it again, something like Windows defers without reading persisted candidate state keeps both.
LGTM.
f560281 to
b54013e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b54013ee87
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
e1c3b67 to
6642ce7
Compare
…sent candidate On Windows the candidate-only provenance slice performs no candidate or configuration filesystem I/O, so it never consults the persisted runtime selection. When no proof-captured CODEX_CLI_PATH candidate is present it nevertheless reported reason "candidate_unavailable", asserting that no Codex CLI candidate exists even though availability was never observed. Operators whose runtime resolves through persisted "configured" state therefore saw a missing candidate while the runtime report showed a known version. Report the deferral that actually occurred instead. The defined "windows_inspection_deferred" reason already exists but was reachable only with an environment candidate. POSIX does read persisted state, so its absent-candidate answer is exact and stays unchanged. No filesystem access is added: the Windows path still performs zero I/O, and candidateAvailable, provenance, managed, selectionAttested, versionEvidence and shim status are unchanged. This intentionally replaces the previous test statement that pinned the "candidate_unavailable" wording on Windows.
Record the Windows inspection reason distinction in structure/runtime.md, which owns src/codex/, and qualify the direct-launch candidate_unavailable outcome as POSIX-only in all eight locale agent references.
6642ce7 to
bc62253
Compare
…78-author-followup-20260912 Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
39277bd to
f04973c
Compare
4 participants
Summary
Report windows_inspection_deferred when Windows CLI inspection has no captured environment candidate, while retaining candidate_path_unavailable for ineligible captured paths and the existing POSIX result. This remains diagnostic-only and does not execute candidates or publish issue #2811 phase-two updater work.
Current author verification
f04973c3cc913f4775b7263e6e9ad154495fe17a.34959335835completed successfully on this exact published head; the run head SHA was verified. This is hosted execution, not a claim that the full matrix was repeated locally.Review readiness checklist
The validation checkbox refers to the explicit scope above. Historical run IDs and prior local results are not represented as new-head full-suite execution.
Summary by CodeRabbit
Documentation
ocx system codex-cli-update checkbehavior across supported languages.windows_inspection_deferredwhen no trusted candidate is available, while POSIX may reportcandidate_unavailable.candidate_path_unavailable.Bug Fixes