Repository navigation
fix(cli): stop reporting success for failed management calls (#2696 #2697 #2698) - #2775
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (16)
📝 WalkthroughWalkthroughChangesCLI transport and credential handling
WP3 stale-check plan amendment
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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 |
github-actions
Bot
added
the
intake: hygiene-blocked
|
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
![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: dabb43b542
ℹ️ 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".
| export function assertNotAdminToken(token: string): void { | ||
| if (!token.startsWith(ADMIN_TOKEN_PREFIX)) return; |
There was a problem hiding this comment.
When OPENCODEX_ADMIN_AUTH_TOKEN is an arbitrary value such as shared-secret, setting OPENCODEX_API_AUTH_TOKEN to the same value still passes this prefix-only check. The service then writes that value as its data-plane token, while initializeManagementAuthState detects the equality and disables every management route with 503. Compare the candidate against configuredAdminToken() in addition to recognizing generated ocx_admin_ tokens.
AGENTS.md reference: src/AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
| export function dataPlaneCredentialCollisionCheck(env: NodeJS.ProcessEnv = process.env): OAuthDoctorCheck { | ||
| const dataPlane = env.OPENCODEX_API_AUTH_TOKEN?.trim(); | ||
| if (!dataPlane) { | ||
| return { level: "OK", message: "No data-plane token is set, so it cannot collide with the management token." }; |
There was a problem hiding this comment.
For the already-broken installs this check is intended to diagnose, a later ocx doctor invocation normally has no OPENCODEX_API_AUTH_TOKEN in its shell; the service wrapper obtains the data-plane token from service-api-token only when starting the proxy. This branch therefore reports OK without examining the credential that actually fenced the running service. Read the installed service token file when the environment value is absent and compare it with the configured admin token, without exposing either value.
AGENTS.md reference: src/AGENTS.md:L20-L20
Useful? React with 👍 / 👎.
| return { | ||
| status: 0, | ||
| json: {}, | ||
| transportError: error instanceof Error ? error.message : String(error), | ||
| }; |
There was a problem hiding this comment.
When an account-management fetch rejects, this now stores the cause, but no production caller reads transportError: direct call sites still invoke proxyUnreachable() without it, and the FamilyRows helpers discard it while setting networkDown. Consequently DNS, TLS, reset, and refusal failures still print only the old generic message. Thread the cause through direct results and FamilyRows, then pass it to proxyUnreachable.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| export function apiError(json: Record<string, unknown>, fallback: string, status?: number): number { | ||
| const primary = accountStringField(json, "error") ?? fallback; | ||
| const lines = [`Error: ${primary}`]; | ||
| const reason = accountStringField(json, "reason"); | ||
| if (reason && reason !== primary) lines.push(`reason: ${reason}`); | ||
| const hint = accountStringField(json, "hint"); | ||
| if (hint && hint !== primary) lines.push(`hint: ${hint}`); | ||
| for (const line of lines) console.error(line); | ||
| if (json.cleanupRequired === true) { | ||
| console.error("Warning: native-login staging cleanup is still required; run 'ocx account main doctor'."); | ||
| } | ||
| return 1; | ||
| return status === 404 ? 4 : status === 409 ? 5 : 1; |
There was a problem hiding this comment.
The new status mapping is unreachable from real account commands because every production apiError call still supplies only the JSON and fallback; the optional status is always undefined. Thus a missing account returned as 404 still exits 1 rather than 4, and a concurrent mutation returned as 409 still exits 1 rather than 5. Pass each response or FamilyRows status through at the call sites and cover an actual command path rather than only calling this helper directly.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
리뷰 · 우선순위 71 / 80이 PR은 지금 두 번째 구멍은 메시지입니다. 세 번째는 토큰 충돌입니다. 서비스가 토큰 파일을 라인 src/cli/dispatch.ts provider/models - 라인 src/service.ts assertNotAdminToken - 라인 src/cli/account-api.ts apiJson catch - 지금 라인 src/cli/account-api.ts apiError - status를 안 넘기면 예전처럼 1입니다. 호출부가 새 인자를 빼먹으면 404/409 구분이 살아나지 않습니다. 테스트가 호출부를 붙잡고 있는지 봐야 합니다. 라인 src/cli/doctor.ts dataPlaneCredentialCollisionCheck - 값을 로그/출력하지 않습니다. 이미 깨진 설치를 고치지는 않고 이름만 붙입니다. 그게 doctor 계약입니다. 경로 tests/cli-transport-honesty.test.ts - 새 파일 16케이스. 회귀 가드가 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Three defects made unattended agent operation impossible. The provider and models dispatch runners awaited their handler and returned a literal 0, so index.ts called process.exit(0) and erased the exit code the handler had set. 'ocx provider quota' against a stopped proxy printed an error and exited 0 (#2697). responseMessage read only error/message/detail, so the reason and hint the server sets on a refusal never reached the operator: a fenced management plane was indistinguishable from a generic failure (#2698). The account client was worse, collapsing every transport error into a bare sentinel that discarded the cause, and exiting 1 for everything including 404 and 409. The service exports its token file as OPENCODEX_API_AUTH_TOKEN, and nothing refused a management token there. When the two match the server fences the whole management plane closed at boot, so every /api/* returns 503 even on a loopback install that never needed a data-plane secret (#2696). The write path now refuses it, and doctor names an install already in that state. The recurrence guard for the exit-code defect corrected its own plan: the allowlist is debug and login, verified because both exit(1) from inside the handler, not the three commands the plan guessed. login reporting success for a failed OAuth flow is left to wp3b's exit-code contract. Closes #2696 Closes #2697 Closes #2698
Reading process.exitCode only reports the current command's outcome if nothing earlier in the process set it. Four handlers in index.ts write it (handleEnsure, handleTrayProxyStart, handleProxyRestart, handleStop), and while only one command runs per process today, the service runner already resets first for exactly this reason. Mirror it rather than depending on that invariant holding.
Two independent reviewers found the same defect: the fix was half-inert. apiError gained a status parameter and apiJson gained transportError, and no production caller passed or read either, so the 404 to 4 and 409 to 5 mapping never ran and the retained transport cause was never printed. The behavior existed only in this phase's own tests while the commit message claimed it shipped. Status is now threaded through all 19 apiError call sites and the cause through the 14 status === 0 guards; two new tests assert the call sites rather than the helpers, so it cannot go inert again. tray had the identical #2697 defect: windowsTrayCommand reports failure through process.exitCode and returns void, so ocx tray install printed an error and exited 0. The recurrence guard could not see it because the pattern was anchored on 'await handle\w+(', scoping it to a naming convention rather than to the defect class. Broadened to 'await [\w.]+(', which surfaced four more candidates; each was verified in its handler before being allowlisted with its own reason. Also corrects the login allowlist reason (it throws rather than exits 1), passes the env seam through to configuredAdminToken so doctor's check no longer reads real machine state for half its comparison, and extends assertNotAdminToken with the equality arm that catches an operator-set admin token without the ocx_admin_ prefix. The #180 matrix test asserting a 404 exits 1 now expects 4. That issue is a feature request, not an exit-code contract; the test described then-current behavior, and the change is the one 010.3 specified and the PR disclosed.
A review of the #2696 work noted that doctor announced a total management outage at the same level as a directory-permission note. OAuthDoctorCheck gains a FAIL level for a condition that makes the surface unusable rather than degraded, and the credential-collision check uses it. Doctor's exit code remains wp3b's scope.
Four parallel read-only reviews of 020 against the current tree all returned FAIL, on one shared root cause: the doc counts grep line hits and calls them routes. Nine amendments in 021. The load-bearing one is check 3. 020 calls it "the unit's central claim -- if this gate can pass vacuously, nothing else in the unit holds", and specifies it as registryRoutes == literalCount + allowlistCount. That identity cannot balance against the real source: one literal line can register two routes (PUT || PATCH), 19 literal lines decide their method in an enclosing block, and two live routes use a negated guard the prescribed === scan never sees. One module -- routing-analytics-routes.ts -- has one route and zero literals, so a literal-keyed test omits it entirely. For GET /api/storage the scan finds only the dead shadowed copy and never the live one; the gate would have mistaken the corpse for the patient. It now reconciles on distinct (method, path) pairs and fails loudly rather than guessing a method it cannot resolve. The 188-literal figure was measured over a wider file set than the "40+ invisible" claim it justifies: 159 in management scope, and the 29-line difference is exactly the codex-auth and native-profile files counted as invisible. Real non-literal count is 18, now enumerated with mechanism. The lab exemption claimed 20 reads. The family is 21 routes, 7 of them mutating, and a local SQLite read cannot start an automation run -- so accept criterion 2 was violated on day one by routes that already exist. Those seven get a bounded deferred-verb exemption naming wp7 as owner, and the parity test asserts the owner phase still exists, so an exemption cannot outlive its debt. 020.4 aimed the version field at isOpencodexHealthz, which returns boolean and cannot carry a value; the body it validates is discarded one frame up in proxyIdentityAt. It also omitted tests/proxy-liveness.test.ts, where 12 exhaustive toEqual assertions break, and asked for a push into a warnings array that does not exist -- the only candidate funnels into codexRuntime.warning, which would file a stale-PATH warning under the Codex runtime heading. 020.2's premise was that 20 USAGE constants are dead. 20 is the file count; 37 constants exist, 12 are exported, and the 12 exports are one-line aliases whose targets carry 243 live references threaded through rejectArgs into CliUsageError. Deleting them is a contract change to how the CLI reports argument errors, not a deletion sweep. Also: 020 never mentioned the Lab boundary, whose protected set includes src/server/management-api.ts -- the file that would import the new route registry. 021 gives the data-only mechanism and the guard command that proves it. Accept criterion 5 is new: the reconciliation gate must be driven red before it counts as evidence. 020's original gate would have failed on correct code while believing itself rigorous, which is what a gate nobody has attacked looks like.
The A-gate reviewer for 021 died twice on provider rate limits, so the audit was done directly. That substitution is recorded in A10 rather than glossed: an unaudited amendment correcting an unaudited plan is the same failure one level up. Four defects, two blocking. A1 told the scanner to resolve a route's method from the same line, the next two lines, or the enclosing block. All three miss how lab-routes.ts actually works: a preceding sibling guard at :352 (`if (req.method !== "GET") return null`) narrows the eight path literals that follow it. storage-log-guard-routes.ts inverts the nesting again, with the path outer at :112 and the method as an inner early return at :113. A forward-and-outward scanner resolves neither, so the fix for 020's arithmetic was itself unimplementable. Method resolution is now a small ordered walk over guard statements that maintains a narrowing context -- which means admitting the fail-loud requirement cannot be satisfied by a regex. A3 asked the parity test to check that every deferred-verb owner still exists in the goalplan. The goalplan is gitignored and untracked, so that test passes here and cannot find the file in CI -- a vacuous gate introduced by the fix rather than by the original. The exemption now names a tracked devlog doc instead, so the assertion means the same thing in both places. A5 had each module's const USAGE become a table lookup. Those constants are top-level, and ESM answers a top-level cycle with undefined rather than an error, so a future back-import would silently empty the usage text in every rejectArgs call -- a quiet regression in the exact error-reporting surface these issues are about. capabilities.ts is now a leaf data module with a guard test and a non-empty assertion. A6 proposed capability entries for help and --version. Neither reaches the dispatch table: both exit in the CLI head at root.ts:28, so --version has no runner key to parity-check against. And help's absence is not a gap -- tests/cli-registry.test.ts:14-21 excludes it in a comment and a headHandled set as a deliberate decision. A6 read that decision as an oversight. Head-handled surfaces now live in a separate list that feeds the banner and the JSON output without touching runner-key parity. Also verified rather than assumed: agent-settings-routes.ts and oauth-account-routes.ts are correctly absent from A2's non-literal table. Their startsWith/slice hits manipulate payloads, not paths.
…ofile test CI failed in `tests/cli-native-profile.test.ts`: a 409 from `stage/finish` returned 5 where the test expected 1. Both cannot be right, so this is a contract question rather than a broken assertion, and it gets answered rather than silenced. 5 is correct. `apiError` maps 404 to 4 and 409 to 5 so a script can tell a missing account from a concurrent mutation, which is the whole point of #2698; the plan for this unit declares that vocabulary explicitly. The test asserted 1 only because every account-family failure used to exit 1 regardless of status -- it was pinning the defect, not the intent. What the test actually covers, the idempotent cancel fallback and the absence of a spurious cleanup warning, is unchanged and still asserted. The expectation is updated with that reasoning recorded inline, so the next reader sees why 5 is not a regression. The comment citing the old bare-catch defect is reworded for the same reason as the devlog lines: the hygiene scanner matches the construct in prose. WP9MSG && git log --oneline -1
5b0c273 to
c1f41a6
Compare
github-actions
Bot
removed
the
intake: hygiene-blocked
Doctor only inspected the calling shell, so a fenced loopback install looked healthy. Read the installed service token, share the admin comparison with start/install, and forward transport errors out of family reads so 404/409 mapping cannot compile back to a silent 1. Co-authored-by: Cursor <cursoragent@cursor.com>
…honesty fix(cli): stop reporting success for failed management calls (lidge-jun#2696 lidge-jun#2697 lidge-jun#2698)
…honesty fix(cli): stop reporting success for failed management calls (lidge-jun#2696 lidge-jun#2697 lidge-jun#2698)
Summary
Stacked on #2773 (targets
codex/ocx-agentic-control-roadmap, notdev).Implements
devlog/_plan/260828_ocx_agentic_control/010_phase_transport_honesty.md.Three defects made the
ocxCLI unusable for unattended operation, and they share atheme: a failed management call did not look like a failure.
#2697 — exit codes discarded. The
providerandmodelsdispatch runners awaitedtheir handler then
return 0, soindex.ts's singleprocess.exit(...)erased thecode the handler had set.
ocx provider quotaagainst a stopped proxy printed an errorand exited 0 — silently green in any script or CI. Both runners now return
Number(process.exitCode ?? 0), the pattern ten other runners already use, and resetit first like the
servicerunner does.#2698 — the server's diagnosis was thrown away.
responseMessageread onlyerror/message/detail, neverreasonorhint— the two fields the managementplane sets to say why it refused and what to do. Routes that return
{ok:false, reason:"…"}with noerrorkey degraded to a genericManagement request failed (409). The body was already attached to the thrown error,so this was pure rendering loss. The account client was worse: it collapsed every
transport error into a bare status-0 sentinel inside
catch {}, discarding the cause,and exited 1 for everything including 404 and 409.
#2696 — a management token could fence the whole management plane closed. The
service exports its token file as
OPENCODEX_API_AUTH_TOKENbefore starting the proxy,and nothing refused a management token in that slot. When the two match, the server
treats the admin credential as a data-plane admission secret and fails the entire
management plane closed at boot: every
/api/*returns 503, even on a loopback installthat never needed a data-plane secret. The CLI could not recover it, because the fence
is decided server-side at startup.
assertNotAdminTokennow refuses it at the write chokepoint and inassertServiceAuthEnvironment— before the loopback short-circuit, since a loopbackinstall writes the token file too and returning early there is what let the broken
state through. For an install already in that state,
ocx doctornames the cause andthe remedy without echoing the credential.
The recurrence guard corrected its own plan
The plan specified an allowlist of
["login", "logout", "tray"]for runners permittedto return a literal 0. The guard contradicted it: the real offenders are
debugandlogin, and the pattern the plan proposed ([^)]*) matched neither target runnerbecause argument lists here contain nested calls. The test now uses
[^;]*and assertsred-first against the pre-fix shape, so a vacuous pattern cannot slip in.
debugandloginare allowlisted on a verified basis: both report every failure withprocess.exit(1)from inside the handler, soreturn 0is reached only on success.That is narrower than "cannot fail" —
loginreporting success for a failed OAuth flowis a real gap, and it belongs to the uniform exit-code contract phase rather than this
one. Recorded in
011rather than silently absorbed.Verification
bun run typecheck— clean. Proven non-vacuous: injecting a type error intoaccount-api.tsproducedTS2322at the exact line; restoring returned it to clean.bun test tests/cli-transport-honesty.test.ts— 16 pass (new file).bun testovercli-dispatch,cli-management-auth,cli-account,service,cli-registry,cli-headless-parity— 297 pass.apiError's newstatusparameter is optional, so every existing call site keepsits current exit code.
One honest caveat, recorded in
011: the first run of the six-file batch reported onefailure in
cli-headless-parity'svision --listcase. It did not reproduce in 26subsequent runs and passes in isolation. The likely mechanism is port contention
between the real
Bun.servefakes that harness starts per test, which this diff doesnot touch — but it is written down rather than dismissed as flaky, so CI recurrence has
a starting point.
Checklist
bun run typecheckclean, and verified to actually inspect the changed filessrc/service.ts,src/cli/doctor.ts) — flaggedfor the security review
MAINTAINERS.mdrequires. No credential value islogged, printed, or returned; the checks compare shapes only
AGENTS.md; retarget todevonce docs(devlog): plan the ocx agentic control surface roadmap #2773 landsCloses #2696
Closes #2697
Closes #2698
Summary by CodeRabbit
New Features
Bug Fixes
Documentation