Skip to content

fix(cli): describe the full Codex desktop restart scope - #4610

Open
luvs01 wants to merge 2 commits into
lidge-jun:devfrom
luvs01:agent/restart-scope-wording-20260914
Open

fix(cli): describe the full Codex desktop restart scope#4610
luvs01 wants to merge 2 commits into
lidge-jun:devfrom
luvs01:agent/restart-scope-wording-20260914

Conversation

@luvs01

@luvs01 luvs01 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Summary

ocx system codex-restart requests a full Codex desktop-app restart as well as app-server restarts, but its capability summary, confirmation flag and human result described only an app-server restart. An operator reading those short surfaces could underestimate the interruption.

Align those messages and the generated management reference with the existing full-desktop contract. Missing --yes now explains the interruption. The human result still says requested, and JSON retains the server result, including skipped or refused desktop outcomes. Request routing, confirmation enforcement and restart behavior are unchanged.

Verification

  • Based on upstream dev snapshot 7f9284ab1e351e97a07283197a1a164dcbbb8bc1; published head 6f4e222e5c0c15f0962e321b7853790c407eff6a (test-only follow-up to implementation head 00f4330dd7cc4bd1ab172c719b79aae3ea20eafd).
  • Typecheck, structure check, generated skill-surface check and diff check pass. The four CLI source-owner documents describe or link the same contract.
  • Existing capability, headless-parity and generated-skill suites: 103 pass / 1 timeout, 765 assertions. The timeout is the unrelated remote connect status/revoke case.
  • Impact tests explicitly pinned with bun scripts/test.ts --changed=7f9284ab1e351e97a07283197a1a164dcbbb8bc1: 253 pass / 3 fail / 1 error, 1,641 assertions across 11 files. Failures are remote connect status/revoke timeout, forward-restore timeout and a preferred-runtime readiness assertion.
  • Compared only those three failures with the unchanged base source: remote-connect timeout and preferred-runtime assertion reproduce; forward-restore passes alone. The restore timeout remains an unattributed timing failure, not a green suite. The candidate's two source files were restored and byte-verified after comparison.
  • An earlier impact invocation selected an older local origin/dev and included an already-upstream transport change. That overbroad run was stopped; its owned processes were verified gone. The pinned result above is the relevant impact run.
  • Added focused restart confirmation/output coverage in the test-only follow-up: 3 pass, 12 assertions. Missing confirmation sends zero requests; confirmed human output reports a request and JSON preserves a skipped Desktop outcome. Product source, generated help and docs are byte-identical to the implementation head. Typecheck and diff checks also pass.
  • No actual Codex restart was performed. Implementation-head CI 34828698706 completed successfully on 00f4330dd7, including all applicable ordinary gates and macOS control. Author validation combines that unchanged implementation coverage with the three passing current-head tests and typecheck; no full hosted rerun was needed for the isolated test addition. Both reviews of that implementation completed; the requested test coverage is now added and its thread resolved. This is not a claim that the full hosted suite ran on 6f4e222e5c. Ordinary gates plus macOS control are enabled; supplemental full Windows diagnostics are intentionally skipped.

Review readiness checklist

  • All CI tests are green on my local testing.
  • I pushed my PR to the latest dev commit.
  • I resolved all correct Codex and CodeRabbit findings.
  • My PR is ready for review.

Summary by CodeRabbit

  • New Features

    • The Codex restart command now fully quits and relaunches the desktop app while restarting its app-servers.
    • Confirmation remains required unless explicitly bypassed, and JSON output includes the complete server result.
    • Restart status and failure messages now clearly describe the desktop-app relaunch.
  • Documentation

    • Updated CLI, runtime, operations, and Claude Desktop documentation to clarify the restart scope and behavior.

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 4e5c5067-0809-40c8-b38d-a5f0a5ec706a

📥 Commits

Reviewing files that changed from the base of the PR and between 00f4330 and 6f4e222.

📒 Files selected for processing (1)
  • tests/cli/cli-headless-parity.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.


📝 Walkthrough

Walkthrough

The CLI now describes and reports a full Codex desktop-app relaunch with app-server restart. Tests cover confirmation, the restart request, and human-readable and JSON results. Runtime and integration documentation define the same scope.

Changes

Codex restart scope

Layer / File(s) Summary
CLI restart behavior
src/cli/capabilities.ts, src/cli/system-command.ts, tests/cli/cli-headless-parity.test.ts, skills/ocx/references/01_management_surface.md
The CLI text and messages now describe fully quitting and relaunching the Codex desktop app and restarting its app-servers. Tests verify confirmation requirements, the unchanged POST request, and skipped desktop restart results in human-readable and JSON output.
Restart scope documentation
structure/runtime.md, structure/clients/claude-desktop.md, structure/ops/docs-and-release.md, structure/config.md
Runtime documentation defines the confirmation requirement and preservation of the full --json server result. Related documents reference the restart scope contract. Config documentation adds spacing before the “Config surface” heading.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 6f4e2

This change aligns restart messaging and documentation with focused confirmation and output tests; no actionable merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: updating the CLI wording to cover the full Codex desktop restart scope.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the intake: hygiene-blocked Deterministic PR hygiene checks failed label Sep 14, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Deterministic hygiene checks failed.

  • missing_regression_test — Behavior changed under src/ or gui/src/ without a test change. Add focused coverage or obtain test-exception-approved.

@github-actions github-actions Bot added the bug Something isn't working label Sep 14, 2026
@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

✅ READY

  • all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

  • ✅ All CI tests are green on my local testing.
  • ✅ I pushed my PR to the latest dev commit.
  • ✅ I resolved all correct Codex and CodeRabbit findings.
  • ✅ My PR is ready for review.

4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers: @lidge-jun @Ingwannu

@luvs01

luvs01 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@luvs01

luvs01 commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 14, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-14T09:56:31.252476Z 6f4e222 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 00f4330dd7

ℹ️ 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".

Comment thread src/cli/system-command.ts
@github-actions github-actions Bot removed the intake: hygiene-blocked Deterministic PR hygiene checks failed label Sep 14, 2026
@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 52 / 80

이 PR은 동작이 아니라 말이 동작을 따라가게 만드는 수정이다. ocx system codex-restart는 이미 Codex 데스크톱 앱을 완전히 종료·재실행하고 app-server도 다시 띄운다. 그런데 capability 요약, --yes 설명, 확인 문구, 사람용 결과, 관리 레퍼런스(skills/ocx/references/01_management_surface.md)는 오랫동안 “app-server만 재시작”처럼 적혀 있었다. 운영자나 에이전트가 짧은 표면만 읽으면 중단 범위를 과소평가한다. 특히 에이전트가 서브커맨드를 추측해 --yes 없이/있이 호출할 때, 확인 문장이 실제 충격을 말해야 한다.

지금 dev(053cfda11) 기준 cost-guard 열차(#4546/#4609)와는 겹치지 않는 CLI/문서 정합 작업이다. 요청 라우팅, 확인 강제, 실제 restart 동작은 안 건드린다고 본문에 명시돼 있다. 바꾸는 파일은 src/cli/capabilities.ts, src/cli/system-command.ts, 관리 레퍼런스, structure/runtime.md 등 계약 문서 링크, 그리고 tests/cli/cli-headless-parity.test.ts에 확인 문구 테스트다. Windows 데스크톱 재시작 계열(#2292/#4564 쪽)과 같은 주제의 “표면이 계약을 숨기지 않기” 슬라이스다.

왜 우선순위가 중간인가. 틀린 요약은 운영 사고를 낳을 수 있어서 방치하면 안 되지만, 전송 증폭·자격 증명·풀 라우팅 같은 비용/안정성 열차보다는 급하지 않다. 다만 메시지만 고치고 테스트로 “확인 전에 요청이 나가지 않는다 / 문구가 데스크톱 중단을 말한다”를 고정한 점은 좋다. JSON 결과는 서버 결과(스킵·거절된 데스크톱 결과 포함)를 유지하고, 사람용은 여전히 requested라고 말하는 구분도 본문에 남아 있어 과장하지 않는다.

라인/경로 기준으로 보면 큰 논리 버그는 안 보인다. 다만 문서가 여러 structure 파일에 같은 한 줄 링크를 반복 추가한다. 계약의 단일 출처는 structure/runtime.md의 CLI restart scope 섹션이어야 하고, 다른 파일은 링크만 두는 지금 모양이 맞다. capabilities 요약과 system-command 확인 문구·레퍼런스 테이블이 서로 같은 동사(“fully quits and relaunches” / “desktop app and app-servers”)를 쓰는지 한 번만 맞춰 보면 된다.

경로 src/cli/capabilities.ts / system-command.ts - 요약·--yes·확인 에러 문구가 모두 “desktop + app-servers”로 정렬됐는지, 예전 “app-server only” 문자열이 테스트/다른 도움말에 남아 있지 않은지 검색으로 확인하면 좋다.
경로 tests/cli/cli-headless-parity.test.ts - 확인 전에 requests가 비는지, 에러 문구에 데스크톱 중단이 들어가는지만 본다. JSON/--yes 성공 경로의 human “requested” 문구와 management reference 테이블까지 한 테스트에 묶을지는 취향이다. 없어도 머지를 막을 정도는 아니다.
경로 structure/*.md 반복 링크 - 내용은 얇다. 링크 타깃 섹션 제목이 바뀌면 여러 파일이 한꺼번에 깨질 수 있으니, 섹션 anchor 이름만 안정적으로 유지하면 된다.
경로 실제 restart 구현 - 이 PR 범위 밖(본문도 그렇게 말함). 메시지만 맞고 구현이 여전히 부분 재시작이면 오히려 더 위험하니, 리뷰어는 “문구 = 기존 풀 데스크톱 계약”인지만 보면 된다.

메인테이너의 판단이 필요한 지점

너의 추천
CI가 통과하면 독립 머지 후보. 동작 변경이 없고 테스트가 확인 표면을 잠근다. 머지 전에 리포 전체에서 “Restart the Codex app-server” 같은 구문 잔여만 rg로 지워 두면 충분하다. cost-guard(#4611)나 types/config 분할과 충돌하지 않으니 닫을 이유도 없다. 우선순위 열차 밖이므로 #4611 hygiene 수리보다 먼저 급하지는 않다.

이 댓글은 grok-bot이 작성했습니다

@luvs01
luvs01 marked this pull request as ready for review September 14, 2026 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working review-ready

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants