Fix Codex TUI goal proxy handling#462
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
📝 WalkthroughWalkthroughextends runtime-rotation-proxy with Codex thread goal endpoint support, implementing upstream request forwarding with error fallback to local goal persistence, rewrites TOML config-storage settings during rotation setup, adds SQLite file special handling and runtime-shadow-home management in scripts, and covers these changes with corresponding proxy and config test cases. Changes
Sequence DiagramsequenceDiagram
participant Client
participant RuntimeProxy as Runtime Rotation Proxy
participant Codex as Upstream Codex Service
participant SessionStore as Session/Goal Storage
Client->>RuntimeProxy: POST /thread/goal/set (goal data)
RuntimeProxy->>RuntimeProxy: Normalize path to /codex/thread/goal/set
RuntimeProxy->>RuntimeProxy: Build RequestContext (session key, auth)
RuntimeProxy->>Codex: Forward POST with managed-account Bearer token
alt Upstream Success (< 400)
Codex-->>RuntimeProxy: HTTP 200 + response
RuntimeProxy->>SessionStore: Persist goal
RuntimeProxy-->>Client: HTTP 200 + response
else Upstream Error (>= 400)
Codex-->>RuntimeProxy: HTTP 403/5xx
RuntimeProxy->>RuntimeProxy: Consume error, record account active
RuntimeProxy->>SessionStore: Persist goal from request body
RuntimeProxy-->>Client: HTTP 200 (fallback handling)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
key concerns:missing regression tests: thread goal GET requests lack explicit coverage—only POST to windows edge cases: concurrency risk: logic density: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/codex.js (1)
2882-2893:⚠️ Potential issue | 🟠 Major | ⚡ Quick windo not detach the helper after a failed tui launch.
options.detachOnExit === truebypasses the exit-code check here, so a tui startup that exits non-zero still leaves the helper running until the 12h idle timeout inscripts/codex.js:2543-2551andscripts/codex.js:2740-2754. that leaks orphan helpers and shadow homes on repeated failed launches. please add a regression test for the non-zero exit path.proposed fix
const cleanup = async ({ exitCode } = {}) => { const livedMs = Date.now() - startedAt; - if ( - options.detachOnExit === true || - (exitCode === 0 && livedMs < detachGraceMs) - ) { + const shouldDetach = + exitCode === 0 && + (options.detachOnExit === true || livedMs < detachGraceMs); + if (shouldDetach) { helper.stdout?.destroy(); helper.stderr?.destroy(); helper.unref(); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/codex.js` around lines 2882 - 2893, The cleanup function currently detaches the helper whenever options.detachOnExit === true, which lets failed TUI launches (non-zero exitCode) leak; modify cleanup to only allow detaching when exitCode is 0 (or undefined) or when livedMs < detachGraceMs but never when exitCode !== 0 even if options.detachOnExit is true: change the conditional that checks options.detachOnExit to also require (exitCode === 0 || exitCode == null). Ensure stopRuntimeRotationAppHelper(helper) is awaited for non-zero exits and remove helper.unref() path for failure cases. Add a regression test that simulates a non-zero TUI startup exit and asserts the helper is stopped/cleaned (no unref / no lingering helper) to cover the non-zero exit path.lib/runtime-rotation-proxy.ts (1)
927-938:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
"diagnostic"is outside the runtime usage operation contract.
createRuntimeUsageRecorderreceives"diagnostic"here, butlib/usage/types.ts:8-19does not define that operation, andlib/policy/runtime-policy.ts:219-230expects the existingUsageLedgerOperationunion. this will either fail typecheck or introduce a ledger value the rest of the runtime policy code does not understand.safe short-term fix
usageRecorder = createRuntimeUsageRecorder({ source: "runtime-proxy", operation: isModelsRequest ? "models" : isThreadGoalRequest - ? "diagnostic" + ? "responses" : "responses",as per coding guidelines,
lib/**: focus on auth rotation, windows filesystem io, and concurrency. verify every change cites affected tests (vitest) and that new queues handle ebusy/429 scenarios. check for logging that leaks tokens or emails.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/runtime-rotation-proxy.ts` around lines 927 - 938, The code passes an unsupported operation string "diagnostic" into createRuntimeUsageRecorder (see the createRuntimeUsageRecorder call in runtime-rotation-proxy.ts); fix by either (A) replacing "diagnostic" with a currently allowed UsageLedgerOperation literal (e.g. use the same operation as other diagnostic-like flows, such as "responses") directly in the createRuntimeUsageRecorder call, or (B) if "diagnostic" is required, add "diagnostic" to the UsageLedgerOperation union in lib/usage/types.ts and update lib/policy/runtime-policy.ts to handle the new operation branch consistently; after the change update/ add vitest cases that exercise the isThreadGoalRequest path and ensure logging changes do not leak tokens/emails and that any new queues handle EBUSY/429 scenarios.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/runtime-rotation-proxy.ts`:
- Around line 1130-1156: The current block at isThreadGoalRequest
unconditionally treats any upstream.status >= 400 as a success for thread-goal
routes; restrict this to only the specific upstream storage-failure codes you
expect (e.g., 502, 503, 504) so we don't turn client 4xx errors into 200s.
Update the conditional that references upstream.status to only trigger the
fallback path for those storage-failure statuses (instead of >=400); move or
remove accountManager.recordSuccess, persistRuntimeActiveAccount, and
usageRecorder.record(...) success calls from this fallback branch so they are
not invoked for errors, and for non-storage errors forward the original
upstream.status and body (using readErrorBody and writeJson) instead of
returning {ok:true}. Keep the threadGoalFallbacks.set(...) + writeJson OK
behavior only when the status is one of the allowed storage-failure codes and
add/adjust vitest regression tests to assert that 4xx client errors (e.g.,
400/401/403) are passed through and do not trigger recordSuccess or fallback
storage behavior.
- Around line 400-421: The buildThreadGoalRequestContext function only derives
sessionKey from headers and body, so GET requests with query params (e.g.,
/thread/goal/get?thread_id=...) fall back to "default" and can return another
thread's cached goal; update buildThreadGoalRequestContext to also parse the
IncomingMessage URL query (for GET) and derive sessionKey from query param names
"thread_id" and "threadId" (fallback to resolveSessionKey(headers, parsedBody)
only if not present), and add a vitest regression in the test suite that issues
GET /thread/goal/get?thread_id=... to ensure the local fallback keys by query
string; when adding this change, name the test clearly (e.g.,
thread-goal-query-param-keying.test.ts), handle edge cases where parsedBody is
null, and ensure no logs leak tokens or emails.
In `@test/codex-bin-wrapper.test.ts`:
- Around line 1046-1048: Replace the brittle substring assertion on
shadowHomeMatch[1] with a real ancestry check: compute expectedRoot =
path.resolve(originalHome, "multi-auth", "runtime-shadow-homes") and actual =
path.resolve(shadowHomeMatch[1]); then use path.relative(expectedRoot, actual)
and assert the result does not start with ".." and is not an absolute path (e.g.
expect(relative).not.toMatch(/^\.\./) &&
expect(path.isAbsolute(relative)).toBe(false)) so Windows path normalization is
handled deterministically; change the assertion around shadowHomeMatch and
originalHome accordingly.
In `@test/runtime-rotation-proxy.test.ts`:
- Around line 527-619: Add two deterministic vitest regression tests in the same
suite: (1) "rejects unauthenticated thread-goal calls" — startProxy with an
AccountManager but send postThreadGoal without credentials (or with expired/no
access token via createStorage) and assert the response is 401/unauthorized and
no upstream fetch (calls.length === 0); (2) "concurrent fallback writes isolated
per thread" — simulate upstream 403 via createRecordingFetch, then concurrently
call postThreadGoal for two different threadIds using Promise.all against the
proxy (ensure using the same AccountManager/storage), then assert both local
set/get succeed and that stored goals are isolated per thread (read back via
postThreadGoal get) and that two upstream calls were attempted; use the existing
helpers postThreadGoal, startProxy, AccountManager, createRecordingFetch,
createStorage so tests mirror existing patterns and remain deterministic.
---
Outside diff comments:
In `@lib/runtime-rotation-proxy.ts`:
- Around line 927-938: The code passes an unsupported operation string
"diagnostic" into createRuntimeUsageRecorder (see the createRuntimeUsageRecorder
call in runtime-rotation-proxy.ts); fix by either (A) replacing "diagnostic"
with a currently allowed UsageLedgerOperation literal (e.g. use the same
operation as other diagnostic-like flows, such as "responses") directly in the
createRuntimeUsageRecorder call, or (B) if "diagnostic" is required, add
"diagnostic" to the UsageLedgerOperation union in lib/usage/types.ts and update
lib/policy/runtime-policy.ts to handle the new operation branch consistently;
after the change update/ add vitest cases that exercise the isThreadGoalRequest
path and ensure logging changes do not leak tokens/emails and that any new
queues handle EBUSY/429 scenarios.
In `@scripts/codex.js`:
- Around line 2882-2893: The cleanup function currently detaches the helper
whenever options.detachOnExit === true, which lets failed TUI launches (non-zero
exitCode) leak; modify cleanup to only allow detaching when exitCode is 0 (or
undefined) or when livedMs < detachGraceMs but never when exitCode !== 0 even if
options.detachOnExit is true: change the conditional that checks
options.detachOnExit to also require (exitCode === 0 || exitCode == null).
Ensure stopRuntimeRotationAppHelper(helper) is awaited for non-zero exits and
remove helper.unref() path for failure cases. Add a regression test that
simulates a non-zero TUI startup exit and asserts the helper is stopped/cleaned
(no unref / no lingering helper) to cover the non-zero exit path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f886a0fb-105f-477c-8e66-22bd57bbb1ed
📒 Files selected for processing (6)
lib/runtime-rotation-proxy.tslib/runtime/config-toml.tsscripts/codex.jstest/app-bind.test.tstest/codex-bin-wrapper.test.tstest/runtime-rotation-proxy.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Greptile Review
🧰 Additional context used
📓 Path-based instructions (2)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/app-bind.test.tstest/runtime-rotation-proxy.test.tstest/codex-bin-wrapper.test.ts
lib/**
⚙️ CodeRabbit configuration file
focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.
Files:
lib/runtime-rotation-proxy.tslib/runtime/config-toml.ts
🪛 ast-grep (0.42.1)
lib/runtime/config-toml.ts
[warning] 103-103: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(^\\s*${key}\\s*=)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (3)
test/codex-bin-wrapper.test.ts (1)
941-942: good hardlink-vs-symlink regression coverage.this correctly validates the non-symlink invariant and keeps the realtime state coupling check intact. ref:
test/codex-bin-wrapper.test.ts:941,test/codex-bin-wrapper.test.ts:1022.Also applies to: 1022-1023
test/app-bind.test.ts (1)
130-134: rewrite/restore expectations are scoped correctly.this captures the intended contract: top-level
disable_response_storageflips tofalse, profile-scoped value stays untouched, and full restore still round-trips. ref:test/app-bind.test.ts:130,test/app-bind.test.ts:154,lib/runtime/config-toml.ts:78.Also applies to: 154-155
test/runtime-rotation-proxy.test.ts (1)
152-168: thread-goal request helper is clean and consistent.this keeps request construction aligned with the existing responses/models helpers and keeps tests readable. ref:
test/runtime-rotation-proxy.test.ts:152.
1 participant
Summary:
Risk notes:
Verification:
npm test -- runtime-rotation-proxynpm test -- codex-bin-wrappernpm test -- runtime-policynpm run typechecknpm run lint -- --quietgit diff --check(only Windows line-ending warnings)Docs and Governance Checklist:
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
proxies codex TUI thread goal
get/setroutes through the runtime rotation proxy with a 403-triggered LRU in-process fallback (512-entry, keyed bythread_id/session header); shadow homes are relocated under the real codex home to guarantee same-volume hard links for sqlite state files, anddisable_response_storage = falseis force-written into the shadow config so the TUI can read thread goals. TUI invocations now usedetachOnExit: trueto keep the app helper alive across the TUI session.Confidence Score: 5/5
safe to merge; all findings are p2 style/coverage gaps with no runtime impact on the happy path
prior p1 findings (session-key collision, recordSuccess on 4xx) are addressed in this revision. remaining findings are a missing restore-path test and no EBUSY retry on Windows hard-link creation — neither causes data loss or incorrect behavior on supported platforms.
lib/runtime/config-toml.ts restore path and scripts/codex.js materializeFileIntoShadowHome
Important Files Changed
Sequence Diagram
sequenceDiagram participant TUI as Codex TUI participant P as Runtime Rotation Proxy participant U as Upstream (OpenAI) participant F as threadGoalFallbacks (Map) TUI->>P: POST /thread/goal/set {threadId, goal} P->>U: POST /codex/thread/goal/set (managed auth) alt upstream 2xx U-->>P: 200 OK P-->>TUI: forward response else upstream 403 U-->>P: 403 Forbidden P->>F: setThreadGoalFallback(sessionKey, goal) P-->>TUI: 200 {ok:true, goal} else upstream other 4xx U-->>P: 4xx P-->>TUI: forward error response end TUI->>P: POST /thread/goal/get {threadId} P->>U: POST /codex/thread/goal/get (managed auth) alt upstream 2xx U-->>P: 200 {goal} P-->>TUI: forward response else upstream 403 U-->>P: 403 Forbidden P->>F: getThreadGoalFallback(sessionKey) F-->>P: goal (or null) P-->>TUI: 200 {goal} endPrompt To Fix All With AI
Reviews (4): Last reviewed commit: "Resolve PR body follow-up findings" | Re-trigger Greptile