fix(stop): name the real refusal cause instead of asserting ownership - #4237
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
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. |
|
✅ Deterministic PR hygiene checks passed. |
리뷰 · 우선순위 69 / 80이 PR은 지금 더 나쁜 건 그 다음 조언이 루프라는 점입니다. 서버 거절 문구는 API 클라이언트를 향해 “
베이스
메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
POST /api/stop refuses for three distinct reasons - respawnable_service, self_unload_service and service_state_unknown - but when no readable message arrived, stopProxy fell back to a single hardcoded sentence naming a fourth cause the server never reports: a CODEX_HOME/OPENCODEX_HOME ownership mismatch. The reporter's homes matched exactly and the server had answered respawnable_service, so three attempts went into re-exporting CODEX_HOME. Carried from #4170 by @yeongjunyoo: the refusal code is captured next to the message, the fallback wording is selected from that code, and the refusal is returned per attempt rather than published to module state, so two overlapping stops cannot lend each other the wrong cause. This also closes the second half of the issue, which the carry left open. #4169 asks that "the recommended next command should not be the command the operator just ran", and it is still is: ocx stop reaches POST /api/stop through handleStop -> stopWithDeferral -> stopProxy, and the server answers "the stop must be run by `ocx stop`" because that refusal is written for an API client. The CLI echoes it verbatim, so following the advice returns the operator to the same message. The carried fallback said "Run `ocx stop`" too, making the empty-body path a tighter loop than before. The refusal code now travels on the error, the fallback messages name the cause only, and refusalNextStep supplies the command. The only callers of stopProxy are ocx stop and the service manager's own cleanup, and by the time either reaches this point the service manager has already been asked to stop, so no branch answers with the command that just failed - every one points at `ocx service status`, which is what actually reports the wrapper. ProxyOwnershipRefusedError keeps its name. #4169 suggests renaming it and the name does overstate what it carries, but the carrying author deliberately left that out as a separate wider change and it is not part of the issue's expected behaviour. Closes #4169 Co-authored-by: yeongjunyoo <47925973+yeongjunyoo@users.noreply.github.com>
022d46a to
1ada8f5
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: 022d46a3ec
ℹ️ 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".
| // recommends `ocx stop` — the command printing it. Following that advice returns | ||
| // the operator to this exact message, which is the loop #4169 was filed for. The | ||
| // service manager was already asked to stop above, so name what is actually left. | ||
| console.error(` ${refusalNextStep(err.code)}`); |
There was a problem hiding this comment.
When POST /api/stop returns its normal respawnable_service or self_unload_service body, err.message is printed immediately above this line, and those server messages explicitly say to run ocx stop (src/server/management-api.ts:300 and :314). Appending a second sentence that contradicts that recommendation still leaves the operator being told to rerun the command that just failed, so the intended loop remains. For these known refusal codes, replace or sanitize the API-oriented message before printing it rather than merely adding another next step; apply the same change to the orphan-recovery catch.
Useful? React with 👍 / 👎.
1 participant
Summary
POST /api/stoprefuses for three distinct reasons, but when no readable message arrivedstopProxyfell back to one hardcoded sentence naming a fourth cause the server never reports — aCODEX_HOME/OPENCODEX_HOMEownership mismatch. In the reported case the homes matched exactly and the server had answeredrespawnable_service, so three attempts went into re-exportingCODEX_HOMEbefore the endpoint was probed directly.Carried from #4170 by @yeongjunyoo. That PR captures the refusal
codenext to the message, selects the fallback wording from it, and returns the refusal per attempt instead of publishing it to module state, so two overlapping stops cannot lend each other the wrong cause.This also closes the second half of the issue, which the carry left open. #4169's Expected section asks for two things: that a refusal not be described as an ownership mismatch, and that "the recommended next command should not be the command the operator just ran." Only the first was addressed.
ocx stopreachesPOST /api/stopthroughhandleStop→stopWithDeferral→stopProxy, and the server answers "the stop must be run byocx stop" because that refusal is written for an API client. The CLI echoes it verbatim, so following the advice returns the operator to the same message. The carried fallback said "Runocx stop" as well, which made the empty-body path a tighter loop than the one being fixed.So the refusal code now travels on the error, the fallback messages name the cause only, and a new
refusalNextStepsupplies the command, printed by the CLI beneath the refusal. The only callers ofstopProxyareocx stopand the service manager's own cleanup, and by the time either reaches this point the service manager has already been asked to stop — so no branch answers with the command that just failed. Every one points atocx service status, which is what actually reports the wrapper state.What the operator sees now, for a
respawnable_servicerefusal that arrives with the server's message:ProxyOwnershipRefusedErrorkeeps its name. The issue suggests renaming it and the name does overstate what it now carries, but @yeongjunyoo deliberately left that out as a separate wider change and it is not part of the issue's Expected behaviour. Recorded here rather than decided unilaterally.Verification
The local product suite, typecheck and GUI build were NOT RUN, by operator instruction for this dispatch round.
bun test,bun run test,bun run test:changed,bun run typecheck,bun run build:guiandbun installwere all NOT RUN. Hosted CI on this exact pushed head is the product evidence for this PR.What was checked instead, by read-only review agents reading the source and by deterministic text checks:
ocx stopdoes reachPOST /api/stop, the message-present path was untouched by the carry, and there is no header, flag or route that lets the refusal printer tell a CLI-originated stop from an API client — which is why the correction belongs in the CLI, the one caller that knows which it is.`ocx stop`, which its own production wording contains in order to rule it out. The assertion now bans a recommendation (/Run \ocx stop`/`) rather than a mention.tests/providers/xai/grok-lifecycle.test.tsthat reads these two files as text was re-counted against the current source: theProxyOwnershipRefusedError/ownershipBlockedpairs, the teardown-skip lines, and the detail echoes are all still exactly 2, and the newrefusalNextStep(err.code)print sites are 2.src/server/management-api.tsare untouched — the server's own messages are not changed by this PR.Checklist
This targets
codex/260911-l4-service-cli(PR #4235) as a stacked child and will be retargeted todevonce the parent lands. No hunks were dropped from #4170: all four of its files are inside this lane's ownership.Closes #4169
Co-authored-by: yeongjunyoo 47925973+yeongjunyoo@users.noreply.github.com