[codex] Support Codex title sync and native passthrough#168
Merged
leeroybrun merged 7 commits intoMay 11, 2026
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:
WalkthroughThis PR integrates native thread title synchronization with Codex, centralizes provider/session CLI argument parsing across handlers, forwards Codex native arguments through the execution pipeline, and adds Windows .cmd shim support. The changes span runtime behavior, CLI routing, argument partitioning, and platform-specific test utilities. ChangesNative Thread Title Synchronization
Centralized Session Argument Partitioning
Codex Arguments & Resume ID Forwarding
Windows Platform Compatibility
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 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 unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
jiuchuanll
force-pushed
the
ll-dev-fixAndOptimize-0505
branch
from
2aa4a8d to
d7df8da
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cli/src/backends/codex/appServer/runtime.test.ts (1)
526-528: ⚡ Quick winReplace
anywithunknownfor better type safety.The
readRequestLoghelper usesanyforparams,result, anderrorfields without justification. While this is a boundary helper parsing external JSON, usingunknownwould be more type-safe and force proper type guards at usage sites.🔧 Proposed fix
- async function readRequestLog(requestLogPath: string): Promise<Array<{ id: unknown; method: string; params: any; result: any; error: any }>> { + async function readRequestLog(requestLogPath: string): Promise<Array<{ id: unknown; method: string; params: unknown; result: unknown; error: unknown }>> { return (await readFile(requestLogPath, 'utf8')).trim().split('\n').map((line) => JSON.parse(line)); }As per coding guidelines: "No untyped escape hatches in production or tests; broad as any casts are forbidden except in boundary fixtures/harnesses with a one-line justification."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/backends/codex/appServer/runtime.test.ts` around lines 526 - 528, Update the readRequestLog helper to remove the use of any for parsed JSON fields: change the return type so params, result, and error are typed as unknown instead of any, i.e., Promise<Array<{ id: unknown; method: string; params: unknown; result: unknown; error: unknown }>>; keep the JSON.parse usage but ensure callers perform proper type-narrowing or validation when accessing those fields (refer to readRequestLog function name and its returned object shape).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/cli/src/backends/codex/appServer/runtime.test.ts`:
- Around line 526-528: Update the readRequestLog helper to remove the use of any
for parsed JSON fields: change the return type so params, result, and error are
typed as unknown instead of any, i.e., Promise<Array<{ id: unknown; method:
string; params: unknown; result: unknown; error: unknown }>>; keep the
JSON.parse usage but ensure callers perform proper type-narrowing or validation
when accessing those fields (refer to readRequestLog function name and its
returned object shape).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 70f96be7-304b-4819-ad55-fa1ef38a7de7
📒 Files selected for processing (6)
apps/cli/src/backends/codex/appServer/runtime.test.tsapps/cli/src/backends/codex/appServer/runtime.tsapps/cli/src/cli/providerCliPassthrough.test.tsapps/cli/src/cli/providerCliPassthrough.tspackages/agents/src/manifest.tspackages/agents/src/types.ts
jiuchuanll
force-pushed
the
ll-dev-fixAndOptimize-0505
branch
from
d7df8da to
1157093
Compare
Contributor
Greptile SummaryThis PR adds two major capabilities to the Codex backend: native thread title sync (when a Happier
Confidence Score: 5/5Safe to merge; changes are well-scoped and well-tested with no blocking defects found. The core title-sync and native-passthrough logic is covered end-to-end by integration tests (blank titles, failed results, native-sync failures, arg forwarding). The shared The
|
| Filename | Overview |
|---|---|
| apps/cli/src/backends/codex/appServer/runtime.ts | Adds native Codex thread title sync: intercepts completed change_title MCP tool-call results, validates success (with content-envelope support and blank-title guard), then calls thread/name/set on the app-server client. Failure is non-fatal. |
| apps/cli/src/cli/providerSessionArgPartition.ts | New shared argument-partitioning utility that consolidates provider/session flag parsing into one place, replacing duplicated per-command loops. |
| apps/cli/src/backends/claude/cli/command.ts | Refactors Claude CLI command handler to use shared partitionProviderSessionArgs. Version display now runs claude --version and always returns early. An unusual ReferenceError catch in the version handler is flagged. |
| apps/cli/src/backends/codex/codexLocalLauncher.ts | Adds codexArgs native passthrough, refactors child-kill logic into stopChildIfRunning(), and adds fresh-remote-switch path when rollout discovery times out. |
| apps/cli/src/cli/dispatch.ts | Removes maybePassthroughProviderCliInfoRequest short-circuit; help/version requests are now delegated to each provider's command handler. |
| apps/cli/src/cli/runBackendSessionCliCommand.ts | Adopts partitionProviderSessionArgs, adds directoryFlags/versionFlags options, integrates combined-help rendering. No logic regressions found. |
| apps/cli/src/backends/codex/runCodex.ts | Adds codexArgs propagation through the pipeline; fixes fast-start local launch to pass resumeIdFromArgs instead of hard-coded null. |
| apps/cli/src/backends/codex/cli/command.ts | Adopts shared partitioning; passes parsed.providerArgs as codexArgs for native Codex subcommand passthrough. |
Sequence Diagram
sequenceDiagram
participant CLI as happier codex CLI
participant RBC as runBackendSessionCliCommand
participant RC as runCodex
participant LMP as runCodexLocalModePass
participant CLL as codexLocalLauncher
participant TUI as Codex TUI
CLI->>RBC: args ['--permission-mode','yolo','resume','--all']
RBC->>RBC: "partitionProviderSessionArgs()<br/>permissionMode=yolo, providerArgs=['resume','--all']"
RBC->>RC: "runCodex({permissionMode, codexArgs:['resume','--all']})"
RC->>LMP: "runCodexLocalModePass({codexArgs:['resume','--all']})"
LMP->>CLL: "codexLocalLauncher({codexArgs:['resume','--all']})"
CLL->>CLL: "buildCodexTuiArgs() -> ['--cd',path,'resume','--all']"
CLL->>TUI: spawn codex --cd path resume --all
Note over RC,TUI: Title Sync Flow
TUI-->>RC: item/completed (change_title, success:true)
RC->>RC: "didHappierTitleToolSucceed() -> true"
RC->>TUI: "thread/name/set {threadId, name}"
Reviews (2): Last reviewed commit: "fix(cli): refine provider info passthrou..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cli/src/backends/codex/appServer/runtime.test.ts (1)
591-593: 💤 Low valueConsider consolidating test log parsing or adopting the helper more broadly.
The
readRequestLoghelper reduces duplication, but it's only used in the 4 new tests while existing tests (lines 612, 666, 706, etc.) still use inline parsing. This creates inconsistency. Consider either:
- Refactoring existing tests to use this helper, or
- Using inline parsing in the new tests to match the existing pattern
The
anytypes forparams,result, anderrorare acceptable for a test fixture but could be more specific for better type safety.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/backends/codex/appServer/runtime.test.ts` around lines 591 - 593, The new readRequestLog helper function centralizes log parsing but existing tests still parse logs inline; either refactor those existing inline parsers to call readRequestLog or revert the new tests to use the same inline parsing style for consistency. Locate the helper readRequestLog in runtime.test.ts and replace inline (await readFile(...).trim().split('\n').map(JSON.parse)) occurrences in other tests with calls to readRequestLog, or update the four new tests to perform the inline parsing used elsewhere—also consider tightening the helper's return type by replacing the broad any for params/result/error with more specific types if desired for stronger type safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/cli/src/backends/codex/appServer/runtime.test.ts`:
- Around line 591-593: The new readRequestLog helper function centralizes log
parsing but existing tests still parse logs inline; either refactor those
existing inline parsers to call readRequestLog or revert the new tests to use
the same inline parsing style for consistency. Locate the helper readRequestLog
in runtime.test.ts and replace inline (await
readFile(...).trim().split('\n').map(JSON.parse)) occurrences in other tests
with calls to readRequestLog, or update the four new tests to perform the inline
parsing used elsewhere—also consider tightening the helper's return type by
replacing the broad any for params/result/error with more specific types if
desired for stronger type safety.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fbad7915-e36c-45c5-ae94-aab8cbc32cab
📒 Files selected for processing (4)
apps/cli/src/backends/codex/appServer/runtime.test.tsapps/cli/src/backends/codex/appServer/runtime.tsapps/cli/src/cli/providerCliPassthrough.test.tsapps/cli/src/cli/providerCliPassthrough.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/cli/src/cli/providerCliPassthrough.test.ts
- apps/cli/src/cli/providerCliPassthrough.ts
- apps/cli/src/backends/codex/appServer/runtime.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/cli/src/backends/codex/codexLocalLauncher.ts (1)
462-474: ⚡ Quick winOptional: extract the child-stop logic into a helper.
The
child.exitCode === null+ Windows/POSIX kill block is now duplicated four times in this file (here, 553-564, 598-609, 651-661). Extracting a smallstopChildIfRunning()closure insidecodexLocalLauncherwould localize the platform branching and make future tweaks (e.g., adjustinggraceMs) one-line changes. Deferable — no behavior change required for this PR.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/backends/codex/codexLocalLauncher.ts` around lines 462 - 474, Extract the duplicated child-stop logic into a single helper (e.g., stopChildIfRunning) inside codexLocalLauncher: move the check child && child.exitCode === null plus the Windows branch that calls killProcessTree(child, { graceMs: 250 }) and the POSIX branch that calls child.kill('SIGTERM') (with the same try/catch) into that helper, replace the four duplicated blocks with a call to stopChildIfRunning(), and keep existing semantics (childStopRequested flag and exitReason assignment should remain where they are but set childStopRequested inside the helper as needed); this centralizes platform branching and makes future changes (like adjusting graceMs) one-line edits to stopChildIfRunning.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/cli/src/backends/codex/codexLocalLauncher.ts`:
- Around line 462-474: Extract the duplicated child-stop logic into a single
helper (e.g., stopChildIfRunning) inside codexLocalLauncher: move the check
child && child.exitCode === null plus the Windows branch that calls
killProcessTree(child, { graceMs: 250 }) and the POSIX branch that calls
child.kill('SIGTERM') (with the same try/catch) into that helper, replace the
four duplicated blocks with a call to stopChildIfRunning(), and keep existing
semantics (childStopRequested flag and exitReason assignment should remain where
they are but set childStopRequested inside the helper as needed); this
centralizes platform branching and makes future changes (like adjusting graceMs)
one-line edits to stopChildIfRunning.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 722d6f74-6544-4385-a6d3-706701cbab8c
📒 Files selected for processing (5)
apps/cli/src/backends/codex/codexLocalLauncher.integration.test.tsapps/cli/src/backends/codex/codexLocalLauncher.testkit.tsapps/cli/src/backends/codex/codexLocalLauncher.tsapps/cli/src/backends/codex/runCodex.fastStart.integration.test.tsapps/cli/src/backends/codex/runCodex.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/stack/scripts/utils/expo/command_heap_limit_env.test.mjs (1)
87-132: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftPlease switch these new test wrappers to canonical test helpers.
writeExpoShimPairCaptureInvocationand the expanded local env override wrapper add more ad hoc test harness primitives instead of reusing shared stack test helpers.As per coding guidelines, "Reuse canonical stack runner/discovery helpers from
scripts/utils/test/**instead of adding new ad hoc tempdir/env/spawn wrappers in test files or domain testkits." and "Reuse canonical stack-local testkit primitives fromscripts/testkit/core/**instead of adding new ad hoc tempdir/env/spawn wrappers."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/stack/scripts/utils/expo/command_heap_limit_env.test.mjs` around lines 87 - 132, Remove the ad-hoc test helpers writeExpoShimPairCaptureInvocation and applyEnvOverrides and replace their usage with the repository's canonical test helpers: import and use the shared shim-writing/tempdir utility and the shared environment-override helper from the stack test utilities, then update tests to call those helpers instead of the local functions; ensure the imported helpers provide equivalent behavior (creating both posix and cmd shim executables and restoring env vars after the test) so you can delete writeExpoShimPairCaptureInvocation and applyEnvOverrides from this file and wire the tests to the shared helpers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@apps/stack/scripts/utils/expo/command_heap_limit_env.test.mjs`:
- Around line 87-132: Remove the ad-hoc test helpers
writeExpoShimPairCaptureInvocation and applyEnvOverrides and replace their usage
with the repository's canonical test helpers: import and use the shared
shim-writing/tempdir utility and the shared environment-override helper from the
stack test utilities, then update tests to call those helpers instead of the
local functions; ensure the imported helpers provide equivalent behavior
(creating both posix and cmd shim executables and restoring env vars after the
test) so you can delete writeExpoShimPairCaptureInvocation and applyEnvOverrides
from this file and wire the tests to the shared helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ffa058ca-f8a0-4f53-8ab9-d507a89a1718
📒 Files selected for processing (5)
apps/cli/src/backends/codex/appServer/runtime.test.tsapps/cli/src/backends/codex/appServer/runtime.tsapps/stack/scripts/utils/expo/command.mjsapps/stack/scripts/utils/expo/command_heap_limit_env.test.mjsapps/stack/scripts/utils/expo/command_workspace_deps_built.test.mjs
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/cli/src/backends/codex/appServer/runtime.test.ts
- apps/cli/src/backends/codex/appServer/runtime.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/stack/scripts/utils/expo/command_heap_limit_env.test.mjs (1)
15-49: ⚡ Quick winConsolidate duplicated Yarn stub setup into testkit core.
This local
writeYarnStubnow overlaps with other Expo test files; moving it to a shared helper underscripts/testkit/core/**would reduce cross-platform drift and future maintenance overhead.As per coding guidelines, “Reuse canonical stack-local testkit primitives from
scripts/testkit/core/**instead of adding new ad hoc tempdir/env/spawn wrappers.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/stack/scripts/utils/expo/command_heap_limit_env.test.mjs` around lines 15 - 49, The writeYarnStub helper in this test duplicates shared logic and should be replaced with the canonical testkit helper: remove the local async function writeYarnStub and import/consume the shared yarn-stub helper from the testkit core (use the exported helper name from scripts/testkit/core that provides cross-platform yarn and yarn.cmd stubs), ensuring the test still sets executable permissions (chmod behavior) and produces the same --version and no-op install semantics; update the test to call the shared helper and delete the duplicated implementation in this file.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/stack/scripts/utils/expo/command_heap_limit_env.test.mjs`:
- Around line 15-49: The writeYarnStub helper in this test duplicates shared
logic and should be replaced with the canonical testkit helper: remove the local
async function writeYarnStub and import/consume the shared yarn-stub helper from
the testkit core (use the exported helper name from scripts/testkit/core that
provides cross-platform yarn and yarn.cmd stubs), ensuring the test still sets
executable permissions (chmod behavior) and produces the same --version and
no-op install semantics; update the test to call the shared helper and delete
the duplicated implementation in this file.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 66e1aa75-2cd6-44cc-8888-69b85a22f970
📒 Files selected for processing (7)
apps/cli/src/backends/codex/appServer/runtime.test.tsapps/cli/src/backends/codex/codexLocalLauncher.tsapps/cli/src/cli/providerCliPassthrough.tsapps/stack/scripts/testkit/core/env_scope.mjsapps/stack/scripts/testkit/core/expo_command_shims.mjsapps/stack/scripts/utils/expo/command_heap_limit_env.test.mjsapps/stack/scripts/utils/expo/command_workspace_deps_built.test.mjs
✅ Files skipped from review due to trivial changes (1)
- apps/stack/scripts/testkit/core/expo_command_shims.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/cli/src/cli/providerCliPassthrough.ts
- apps/cli/src/backends/codex/appServer/runtime.test.ts
- apps/cli/src/backends/codex/codexLocalLauncher.ts
Co-authored-by: 九川三江 <903682402@qq.com>
Co-authored-by: 九川三江 <903682402@qq.com>
Collaborator
|
Thank you very much @jiuchuanll, great ideas ! I've actually refined the commands passthrough logic to extract and generalize the logic we had for Claude instead, which is in fact the inverted logic of your PR proposition: all the commands like That way, users can use @CodeRabbit @greptile please review deeply |
|
Feedback to incorporate: Perform a deep review focusing on the generalized provider CLI passthrough logic in ✅ Actions performedFull review triggered. |
|
A full deep review has been triggered with that context in mind. The review will focus on:
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
apps/stack/scripts/testkit/core/expo_command_shims.mjs (1)
20-28: 💤 Low valueInconsistency: Windows shim uses hardcoded path; POSIX shim uses environment variable.
The POSIX shim (line 13) reads the output path from the
OUTPUT_PATHenvironment variable:echo "shim=posix bin=$0 args=$*" >> "${OUTPUT_PATH:?}"The Windows shim (line 24) uses the JavaScript
outputPathparameter interpolated directly into the batch file:`echo shim=cmd bin=%~f0 args=%*>>"${outputPath}"`This means:
- POSIX shim: Can be controlled via
OUTPUT_PATHenvironment variable at runtime- Windows shim: Output path is baked into the batch file at creation time
For test purposes this likely works fine, but it's architecturally inconsistent. If the Windows shim should also support runtime configuration via environment, update line 24:
- `echo shim=cmd bin=%~f0 args=%*>>"${outputPath}"`, + `echo shim=cmd bin=%~f0 args=%*>>"%OUTPUT_PATH%"`,And ensure tests set
OUTPUT_PATHin the environment before invoking the Windows shim.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/stack/scripts/testkit/core/expo_command_shims.mjs` around lines 20 - 28, The Windows shim written by writeFile (creating 'expo.cmd') currently hardcodes the JS outputPath into the batch content; change the batch line inside the array passed to writeFile (the line creating `echo shim=cmd bin=%~f0 args=%*>>"${outputPath}"`) to instead read the runtime environment variable (use %OUTPUT_PATH% or the Windows batch parameter syntax) so the batch file appends to the path provided at runtime, mirroring the POSIX shim's use of OUTPUT_PATH; update tests to set OUTPUT_PATH in the environment before invoking the produced expo.cmd shim.apps/stack/scripts/testkit/core/env_scope.mjs (1)
74-77: ⚡ Quick winBoth
PATHandPathare set on Windows, which is inefficient.When
overridesincludesPATHon Windows, the current code spreadsoverrides(keepingPATH) and then addsPath: overrides.PATH, resulting in both keys ineffectiveOverrides. While this works correctly (both are set inprocess.envand both are restored), it's wasteful.If the intent is to use only the Windows convention (
Path), consider using destructuring to excludePATH:const effectiveOverrides = process.platform === 'win32' && Object.prototype.hasOwnProperty.call(overrides, 'PATH') ? { ...Object.fromEntries(Object.entries(overrides).filter(([k]) => k !== 'PATH')), Path: overrides.PATH } : overrides;Or more simply, rely on caller convention (tests already pass
PATH; let them handle the Windows mapping if needed).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/stack/scripts/testkit/core/env_scope.mjs` around lines 74 - 77, The current effectiveOverrides assignment on Windows copies overrides and then adds Path: overrides.PATH, leaving both PATH and Path set; change it so when process.platform === 'win32' and overrides has 'PATH' you build effectiveOverrides by copying overrides but excluding the 'PATH' key and then add Path: overrides.PATH (i.e., remove 'PATH' before adding 'Path'), referencing effectiveOverrides, overrides, process.platform, 'PATH' and 'Path' to locate the code to change.apps/cli/src/backends/codex/appServer/runtime.test.ts (1)
1596-1760: ⚡ Quick winExcellent test coverage for title sync! Consider adding edge case tests.
The five new tests comprehensively validate the native title sync behavior:
- ✅ Successful sync with standard title
- ✅ Successful sync with MCP content envelope
- ✅ No sync for blank/whitespace titles
- ✅ No sync for failed tool results
- ✅ Tool result remains accepted even when native sync fails
The tests properly use try-finally for cleanup and verify both the
thread/name/setcalls in the request log and the forwarded tool results.Optional enhancement: Consider adding tests for these edge cases (currently only covered by the runtime implementation logic):
- Title tool completion in a sidechain context (should not sync to parent thread)
- Title tool completion before
threadIdis established (should not attempt sync)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/backends/codex/appServer/runtime.test.ts` around lines 1596 - 1760, Add two new tests in the same suite in runtime.test.ts: one named "does not sync title tool calls from sidechain context to parent thread" that uses createRuntimeFixture(...) and createCodexAppServerRuntime(...) then runtime.sendPrompt('bridge-mcp-title-tool-sidechain') and asserts requestLog has no 'thread/name/set' entries and that sendCodexMessage tool-call-result was still sent; and another named "does not sync title tool calls before threadId established" that triggers a title-complete prompt before the runtime establishes the parent thread (e.g., sendPrompt immediately after startOrLoad without creating a thread) and asserts no 'thread/name/set' requests were made. Reference createRuntimeFixture, createCodexAppServerRuntime, runtime.sendPrompt, readRequestLog, and the 'thread/name/set' method in your assertions.apps/cli/src/backends/codex/appServer/runtime.ts (1)
783-811: 💤 Low valueLGTM! Consider adding test coverage for edge cases.
The tool-result handling correctly implements best-effort native title synchronization:
- Retrieves and clears the pending title immediately
- Validates all necessary preconditions before syncing
- Appropriately catches and logs sync failures without propagating them
The conditions at line 801 properly guard against syncing in sidechains or when no native threadId is available. Consider adding test coverage for these edge cases:
- Sidechain case (
context.sidechainIdis set)- Missing threadId case (e.g., before thread/started notification arrives)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/backends/codex/appServer/runtime.ts` around lines 783 - 811, Add unit/integration tests for the Happier title sync branch around the tool-result handling: exercise the branch that sends a 'tool-result'/'tool-call-result' and verify pendingHappierTitleToolNamesByCallId is cleared and no native thread rename is attempted when context.sidechainId is set, and similarly test the case where threadId is missing (e.g., before thread/started) so didHappierTitleToolSucceed(update.output) is true but ensureClient()/client.request('thread/name/set', ...) is not called; use mocks/spies for ensureClient and client.request to assert no rename attempts and for logger.debug to ensure failures are safely caught when client.request would throw.apps/cli/src/cli/providerSessionArgPartition.ts (2)
222-227: 💤 Low value
--account-settings-version-hintsilently consumes its value.The flag is recognized and its value is skipped, but nothing about it is returned to the caller and there's no comment explaining why we accept it. If this is intentional backward-compat (e.g., daemon-injected hint kept for forward compatibility), please add a one-line comment; if it's leftover from a prior parser, drop it under the YAGNI rule.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/cli/providerSessionArgPartition.ts` around lines 222 - 227, The branch handling the '--account-settings-version-hint' flag currently consumes an optional value via readOptionalValue(args, i) and increments i but never returns or records it; either remove this unused branch (delete the if (arg === '--account-settings-version-hint') block) or, if it must be accepted for backward/forward compatibility, replace it with a one-line explanatory comment above the block that explains it's intentionally ignored (e.g., "daemon-injected hint accepted and discarded for forward compatibility") and keep the current consume-and-skip logic in the same symbols (arg, readOptionalValue, i) so future readers understand it's intentional.
1-8: ⚡ Quick winMissing file-level JSDoc explaining responsibilities.
This new module owns the contract for splitting argv into Happier session flags vs. provider-owned args used by Claude/Codex command handlers,
runBackendSessionCliCommand, andsessionStartArgs. A top-of-file JSDoc would set expectations for the forwarding rules (forwardModelFlag,forwardResumeFlag,yoloProviderArgs,directoryFlags,versionFlags) so future provider integrations don't drift.As per coding guidelines: "Include comprehensive JSDoc comments at the top of each file explaining responsibilities".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/cli/providerSessionArgPartition.ts` around lines 1 - 8, Add a top-of-file JSDoc that documents this module's responsibilities: that it splits process argv into Happier session flags vs provider-owned args and enumerates forwarding rules used by runBackendSessionCliCommand and sessionStartArgs (specifically describe forwardModelFlag, forwardResumeFlag, yoloProviderArgs, directoryFlags, and versionFlags), include expected semantics for each flag/group (what is forwarded, when filtered, and who owns parsing), and list any invariants/contract callers rely on (e.g., order preservation, prefix handling). Place this comment at the very top of providerSessionArgPartition.ts so future provider integrations follow the same forwarding contract.apps/cli/src/cli/providerCliPassthrough.ts (1)
8-17: 💤 Low valueHelp-over-version precedence is implicit; consider a short comment.
detectProviderCliInfoRequestreturns the first matching--helpflag before falling back to version flags, sohappier codex --help --versionyields the help flag. That precedence is intentional (mirrors prior behavior), but a one-line comment would make it obvious to future readers given the recent return-type widening.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/cli/providerCliPassthrough.ts` around lines 8 - 17, The function detectProviderCliInfoRequest currently checks HELP_FLAGS before VERSION_FLAGS, so a combined invocation like "happier codex --help --version" intentionally returns the help flag; add a one-line comment above the function (or above the HELP_FLAGS/VERSION_FLAGS definitions) noting that help has precedence over version to make the behavior explicit for future readers and to justify the order of checks in detectProviderCliInfoRequest (referencing HELP_FLAGS, VERSION_FLAGS, and the args parameter).apps/cli/src/cli/dispatch.providerInfoPassthrough.test.ts (1)
1-53: 💤 Low valueTest file name/scope no longer matches what it verifies.
After the dispatch refactor the only remaining assertion is that
maybePassthroughProviderCliInfoRequestis never called and that the provider command handler runs instead. The file/describe still says "provider info passthrough", which now misleads readers. Two small improvements would help:
- Rename to something like
dispatch.providerCommandHandler.test.ts(and update thedescribeblock) or merge this case into an existing dispatch test file.- The
@/backends/catalogmock at Lines 18-26 isn't exercised by the current test path (the registeredgeminihandler short-circuits before the default catalog fallback) — either drop it or add a complementary test that actually hits the default-handler path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/cli/dispatch.providerInfoPassthrough.test.ts` around lines 1 - 53, The test name and describe text no longer match what is asserted: update the file and describe to reflect that this verifies the provider command handler runs (suggest renaming file to dispatch.providerCommandHandler.test.ts and change the describe string accordingly), and either remove the unused '@/backends/catalog' mock (the vi.mock that defines requireCatalogEntry) or add a complementary test that clears/unsets the registered gemini handler (e.g. make commandRegistry.gemini undefined) so dispatchCli exercises the requireCatalogEntry fallback path; ensure the complementary test asserts maybePassthroughProviderCliInfoRequest (passthroughSpy) is not called and that requireCatalogEntry/getCliCommandHandler path is invoked instead, referencing dispatchCli, geminiHandlerSpy, passthroughSpy and requireCatalogEntry to locate the code to change.apps/cli/src/backends/codex/codexLocalLauncher.ts (1)
120-152: ⚡ Quick winDocument
codexArgsprecedence overresumeIdinbuildCodexTuiArgs.When
codexArgsis non-empty the function pushes it and returns immediately, intentionally droppingopts.resumeId(per the new integration test asserting the session id is not appended when codexArgs are forwarded). Since both options are still accepted in the type, the precedence is invisible at the call site. A short JSDoc onbuildCodexTuiArgs(and ideallycodexLocalLauncher/resolveCodexTuiInvocation) noting "whencodexArgsis provided, native Codex args win andresumeIdis ignored" would prevent surprising regressions when wiring new callers.📝 Suggested JSDoc
+/** + * Builds Codex TUI argv. + * + * Precedence: when `opts.codexArgs` is provided and non-empty, it is forwarded + * verbatim and `opts.resumeId` is ignored (the caller is presumed to be in + * native Codex passthrough mode and to have authored its own resume args). + * Otherwise `opts.resumeId`, if present, is mapped to `resume <id>`. + */ function buildCodexTuiArgs(opts: { cwd: string; resumeId?: string | null; permissionMode: PermissionMode; codexArgs?: readonly string[]; }): string[] {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/backends/codex/codexLocalLauncher.ts` around lines 120 - 152, Add a short JSDoc to buildCodexTuiArgs explaining that when the codexArgs parameter is provided and non-empty the function forwards those native Codex args and returns immediately, therefore any resumeId is ignored; also add similar brief JSDoc notes to codexLocalLauncher and resolveCodexTuiInvocation to document the precedence ("codexArgs wins, resumeId ignored") so callers are not surprised. Ensure the JSDoc references the codexArgs and resumeId parameters by name and mirrors the existing style in nearby functions.apps/cli/src/cli/sessionStartArgs.ts (1)
15-29: ⚡ Quick winConsolidate the partition→
ParsedSessionStartArgspicker.The seven-field mapping here is duplicated verbatim by
pickSessionStartArgsinapps/cli/src/cli/runBackendSessionCliCommand.ts(lines 63–73). Any future field additions toParsedSessionStartArgswill require touching both spots and silently break in one if missed. Consider exporting a singlepickSessionStartArgs(parsed)from this module and reusing it from both callers.♻️ Proposed consolidation
+export function pickSessionStartArgs( + parsed: Pick< + import('@/cli/providerSessionArgPartition').ProviderSessionArgPartitionResult, + | 'startedBy' + | 'permissionMode' + | 'permissionModeUpdatedAt' + | 'agentModeId' + | 'agentModeUpdatedAt' + | 'modelId' + | 'modelUpdatedAt' + >, +): ParsedSessionStartArgs { + return { + startedBy: parsed.startedBy, + permissionMode: parsed.permissionMode, + permissionModeUpdatedAt: parsed.permissionModeUpdatedAt, + agentModeId: parsed.agentModeId, + agentModeUpdatedAt: parsed.agentModeUpdatedAt, + modelId: parsed.modelId, + modelUpdatedAt: parsed.modelUpdatedAt, + }; +} + export function parseSessionStartArgs(args: string[]): ParsedSessionStartArgs { - const parsed = partitionProviderSessionArgs({ - args: args[0] === 'happier' ? args.slice(1) : args, - }); - - return { - startedBy: parsed.startedBy, - permissionMode: parsed.permissionMode, - permissionModeUpdatedAt: parsed.permissionModeUpdatedAt, - agentModeId: parsed.agentModeId, - agentModeUpdatedAt: parsed.agentModeUpdatedAt, - modelId: parsed.modelId, - modelUpdatedAt: parsed.modelUpdatedAt, - }; + const parsed = partitionProviderSessionArgs({ + args: args[0] === 'happier' ? args.slice(1) : args, + }); + return pickSessionStartArgs(parsed); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/cli/sessionStartArgs.ts` around lines 15 - 29, Introduce a single picker function (e.g., export function pickSessionStartArgs(parsed: ReturnType<typeof partitionProviderSessionArgs>)) that maps the partitionProviderSessionArgs result to the seven ParsedSessionStartArgs fields, replace the inline mapping in parseSessionStartArgs to call pickSessionStartArgs(partitioned) (where partitioned is the result of partitionProviderSessionArgs), and update the other caller (pickSessionStartArgs usage in runBackendSessionCliCommand.ts) to import and reuse this new exported pickSessionStartArgs so the mapping logic lives in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/backends/codex/codexLocalLauncher.ts`:
- Around line 485-493: The branch that sets exitReason = { type: 'switch',
resumeId: null } when remoteSwitchIntentObserved && !opts.resumeId may
inadvertently drop native codex passthrough args stored in opts.codexArgs (e.g.,
['resume', id]) even when opts.resumeId is null; update the guard to also check
whether native passthrough is present (inspect opts.codexArgs for a resume
intent) and avoid taking the fresh-remote path if native resume args exist, or
explicitly preserve/forward those args into the exitReason/object handed to the
launcher (references: remoteSwitchIntentObserved, opts.resumeId, opts.codexArgs,
exitReason, stopChildIfRunning). Ensure behavior is documented with a short
comment if you intentionally prefer to discard native args.
In `@apps/cli/src/backends/codex/runCodex.ts`:
- Line 142: The initial local-launch branches are not forwarding opts.codexArgs,
which causes provider-specific args to be dropped on first launch; update the
callers inside runCodex (the fast-start spawnVendor path and the direct mode ===
'local' launcher call) to pass codexArgs: opts.codexArgs through to the same
object shape used by runCodexLocalModePass so that the remote→local branch and
all local-launch paths receive the same codexArgs; specifically modify the
spawnVendor invocation and the direct local launcher call to include codexArgs:
opts.codexArgs (matching the parameter name codexArgs and the
runCodexLocalModePass signature).
---
Nitpick comments:
In `@apps/cli/src/backends/codex/appServer/runtime.test.ts`:
- Around line 1596-1760: Add two new tests in the same suite in runtime.test.ts:
one named "does not sync title tool calls from sidechain context to parent
thread" that uses createRuntimeFixture(...) and createCodexAppServerRuntime(...)
then runtime.sendPrompt('bridge-mcp-title-tool-sidechain') and asserts
requestLog has no 'thread/name/set' entries and that sendCodexMessage
tool-call-result was still sent; and another named "does not sync title tool
calls before threadId established" that triggers a title-complete prompt before
the runtime establishes the parent thread (e.g., sendPrompt immediately after
startOrLoad without creating a thread) and asserts no 'thread/name/set' requests
were made. Reference createRuntimeFixture, createCodexAppServerRuntime,
runtime.sendPrompt, readRequestLog, and the 'thread/name/set' method in your
assertions.
In `@apps/cli/src/backends/codex/appServer/runtime.ts`:
- Around line 783-811: Add unit/integration tests for the Happier title sync
branch around the tool-result handling: exercise the branch that sends a
'tool-result'/'tool-call-result' and verify pendingHappierTitleToolNamesByCallId
is cleared and no native thread rename is attempted when context.sidechainId is
set, and similarly test the case where threadId is missing (e.g., before
thread/started) so didHappierTitleToolSucceed(update.output) is true but
ensureClient()/client.request('thread/name/set', ...) is not called; use
mocks/spies for ensureClient and client.request to assert no rename attempts and
for logger.debug to ensure failures are safely caught when client.request would
throw.
In `@apps/cli/src/backends/codex/codexLocalLauncher.ts`:
- Around line 120-152: Add a short JSDoc to buildCodexTuiArgs explaining that
when the codexArgs parameter is provided and non-empty the function forwards
those native Codex args and returns immediately, therefore any resumeId is
ignored; also add similar brief JSDoc notes to codexLocalLauncher and
resolveCodexTuiInvocation to document the precedence ("codexArgs wins, resumeId
ignored") so callers are not surprised. Ensure the JSDoc references the
codexArgs and resumeId parameters by name and mirrors the existing style in
nearby functions.
In `@apps/cli/src/cli/dispatch.providerInfoPassthrough.test.ts`:
- Around line 1-53: The test name and describe text no longer match what is
asserted: update the file and describe to reflect that this verifies the
provider command handler runs (suggest renaming file to
dispatch.providerCommandHandler.test.ts and change the describe string
accordingly), and either remove the unused '@/backends/catalog' mock (the
vi.mock that defines requireCatalogEntry) or add a complementary test that
clears/unsets the registered gemini handler (e.g. make commandRegistry.gemini
undefined) so dispatchCli exercises the requireCatalogEntry fallback path;
ensure the complementary test asserts maybePassthroughProviderCliInfoRequest
(passthroughSpy) is not called and that requireCatalogEntry/getCliCommandHandler
path is invoked instead, referencing dispatchCli, geminiHandlerSpy,
passthroughSpy and requireCatalogEntry to locate the code to change.
In `@apps/cli/src/cli/providerCliPassthrough.ts`:
- Around line 8-17: The function detectProviderCliInfoRequest currently checks
HELP_FLAGS before VERSION_FLAGS, so a combined invocation like "happier codex
--help --version" intentionally returns the help flag; add a one-line comment
above the function (or above the HELP_FLAGS/VERSION_FLAGS definitions) noting
that help has precedence over version to make the behavior explicit for future
readers and to justify the order of checks in detectProviderCliInfoRequest
(referencing HELP_FLAGS, VERSION_FLAGS, and the args parameter).
In `@apps/cli/src/cli/providerSessionArgPartition.ts`:
- Around line 222-227: The branch handling the '--account-settings-version-hint'
flag currently consumes an optional value via readOptionalValue(args, i) and
increments i but never returns or records it; either remove this unused branch
(delete the if (arg === '--account-settings-version-hint') block) or, if it must
be accepted for backward/forward compatibility, replace it with a one-line
explanatory comment above the block that explains it's intentionally ignored
(e.g., "daemon-injected hint accepted and discarded for forward compatibility")
and keep the current consume-and-skip logic in the same symbols (arg,
readOptionalValue, i) so future readers understand it's intentional.
- Around line 1-8: Add a top-of-file JSDoc that documents this module's
responsibilities: that it splits process argv into Happier session flags vs
provider-owned args and enumerates forwarding rules used by
runBackendSessionCliCommand and sessionStartArgs (specifically describe
forwardModelFlag, forwardResumeFlag, yoloProviderArgs, directoryFlags, and
versionFlags), include expected semantics for each flag/group (what is
forwarded, when filtered, and who owns parsing), and list any
invariants/contract callers rely on (e.g., order preservation, prefix handling).
Place this comment at the very top of providerSessionArgPartition.ts so future
provider integrations follow the same forwarding contract.
In `@apps/cli/src/cli/sessionStartArgs.ts`:
- Around line 15-29: Introduce a single picker function (e.g., export function
pickSessionStartArgs(parsed: ReturnType<typeof partitionProviderSessionArgs>))
that maps the partitionProviderSessionArgs result to the seven
ParsedSessionStartArgs fields, replace the inline mapping in
parseSessionStartArgs to call pickSessionStartArgs(partitioned) (where
partitioned is the result of partitionProviderSessionArgs), and update the other
caller (pickSessionStartArgs usage in runBackendSessionCliCommand.ts) to import
and reuse this new exported pickSessionStartArgs so the mapping logic lives in
one place.
In `@apps/stack/scripts/testkit/core/env_scope.mjs`:
- Around line 74-77: The current effectiveOverrides assignment on Windows copies
overrides and then adds Path: overrides.PATH, leaving both PATH and Path set;
change it so when process.platform === 'win32' and overrides has 'PATH' you
build effectiveOverrides by copying overrides but excluding the 'PATH' key and
then add Path: overrides.PATH (i.e., remove 'PATH' before adding 'Path'),
referencing effectiveOverrides, overrides, process.platform, 'PATH' and 'Path'
to locate the code to change.
In `@apps/stack/scripts/testkit/core/expo_command_shims.mjs`:
- Around line 20-28: The Windows shim written by writeFile (creating 'expo.cmd')
currently hardcodes the JS outputPath into the batch content; change the batch
line inside the array passed to writeFile (the line creating `echo shim=cmd
bin=%~f0 args=%*>>"${outputPath}"`) to instead read the runtime environment
variable (use %OUTPUT_PATH% or the Windows batch parameter syntax) so the batch
file appends to the path provided at runtime, mirroring the POSIX shim's use of
OUTPUT_PATH; update tests to set OUTPUT_PATH in the environment before invoking
the produced expo.cmd shim.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 20c50072-d826-4b93-ba9e-697fc363d6bd
📒 Files selected for processing (29)
apps/cli/src/backends/claude/cli/command.help.test.tsapps/cli/src/backends/claude/cli/command.tsapps/cli/src/backends/claude/cli/command.version.test.tsapps/cli/src/backends/codex/appServer/runtime.test.tsapps/cli/src/backends/codex/appServer/runtime.tsapps/cli/src/backends/codex/cli/command.test.tsapps/cli/src/backends/codex/cli/command.tsapps/cli/src/backends/codex/codexLocalLauncher.integration.test.tsapps/cli/src/backends/codex/codexLocalLauncher.testkit.tsapps/cli/src/backends/codex/codexLocalLauncher.tsapps/cli/src/backends/codex/runCodex.fastStart.integration.test.tsapps/cli/src/backends/codex/runCodex.tsapps/cli/src/backends/codex/runtime/localModePass.test.tsapps/cli/src/backends/codex/runtime/localModePass.tsapps/cli/src/cli/dispatch.providerInfoPassthrough.test.tsapps/cli/src/cli/dispatch.tsapps/cli/src/cli/providerCliPassthrough.test.tsapps/cli/src/cli/providerCliPassthrough.tsapps/cli/src/cli/providerSessionArgPartition.test.tsapps/cli/src/cli/providerSessionArgPartition.tsapps/cli/src/cli/runBackendSessionCliCommand.test.tsapps/cli/src/cli/runBackendSessionCliCommand.tsapps/cli/src/cli/sessionStartArgs.test.tsapps/cli/src/cli/sessionStartArgs.tsapps/stack/scripts/testkit/core/env_scope.mjsapps/stack/scripts/testkit/core/expo_command_shims.mjsapps/stack/scripts/utils/expo/command.mjsapps/stack/scripts/utils/expo/command_heap_limit_env.test.mjsapps/stack/scripts/utils/expo/command_workspace_deps_built.test.mjs
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
apps/cli/src/backends/codex/runCodex.ts (1)
407-414:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
codexArgsstill dropped on initial local launches.The PR threads
opts.codexArgs ?? []intorunCodexLocalModePassat Line 1342, but the two initial local-launch paths still callcodexLocalLauncherwithoutcodexArgs:
- Fast-start
spawnVendor(Lines 407–414)- Direct
mode === 'local'launcher call (Lines 663–672)So provider-native args like
resume --allfromhappier codex …reach the local TUI only after a remote→local switch, not on first launch. The previous review flagged this and was marked addressed, but the gap persists for the initial launch sites.💡 Suggested patch
@@ spawnVendor artifacts.localResult = await codexLocalLauncher<EnhancedMode>({ path: requestedDirectory, api: null, session: artifacts.deferredSession as unknown as ApiSessionClient, messageQueue, permissionMode: initialPermissionMode, resumeId: resumeIdFromArgs, + codexArgs: opts.codexArgs ?? [], }); @@ mode === 'local' const localResult = await (localLauncherPromise ?? codexLocalLauncher<EnhancedMode>({ path: workspaceDirFromMetadata ?? requestedDirectory, api, session, messageQueue, permissionMode: initialPermissionMode, resumeId: resumeIdFromArgs, + codexArgs: opts.codexArgs ?? [], }));Also applies to: 663-672
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/backends/codex/runCodex.ts` around lines 407 - 414, The initial local-launch call sites are missing provider-native args: update the two codexLocalLauncher invocations (the fast-start spawnVendor path where artifacts.localResult is set and the direct mode === 'local' launcher call) to pass codexArgs: opts.codexArgs ?? [] (same threading used for runCodexLocalModePass) so that provider args like "resume --all" are forwarded on first launch; locate the calls to codexLocalLauncher and add the codexArgs property to the options object passed in.apps/cli/src/backends/codex/codexLocalLauncher.ts (1)
485-493:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNative
codexArgsmay still be discarded on fresh-remote fallback.This branch was previously flagged: when a user runs
happier codex resume <id>(native), the partitioner puts['resume', <id>]intoopts.codexArgswhileopts.resumeIdstaysnull. If a remote-switch intent fires before a rollout is discovered (queued UI message, pending-queue watcher, orswitchRPC), this fallback will hand the user off to a fresh remote (resumeId: null), silently dropping their native resume target.Consider gating on
opts.codexArgs?.lengthhere as well, or document why discarding is intentional for the remote path.🛡️ Possible fix
- if (remoteSwitchIntentObserved && !opts.resumeId && now >= deadline) { + const hasNativeCodexArgs = (opts.codexArgs?.length ?? 0) > 0; + if (remoteSwitchIntentObserved && !opts.resumeId && !hasNativeCodexArgs && now >= deadline) { logger.debug('[codex] switch: starting fresh remote because no local rollout was discovered', { elapsedSinceStartMs: now - startedAtMs, }); exitReason = { type: 'switch', resumeId: null }; stopChildIfRunning(); childExited = true; break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/backends/codex/codexLocalLauncher.ts` around lines 485 - 493, The fallback branch that sets exitReason = { type: 'switch', resumeId: null } can drop native resume targets stored in opts.codexArgs; update the condition in the codexLocalLauncher logic so it also checks for opts.codexArgs?.length (or extracts a resume id from opts.codexArgs when present) before treating this as a fresh-remote fallback, and if a native resume arg exists preserve it by setting exitReason.resumeId to that id instead of null; modify the block containing stopChildIfRunning()/childExited=true to respect opts.codexArgs and opts.resumeId accordingly.
🧹 Nitpick comments (4)
apps/cli/src/cli/providerSessionArgPartition.ts (2)
9-37: ⚡ Quick winUse
interfacefor exported object shapesThese exported object-shape declarations should be interfaces to match repository TS conventions.
As per coding guidelines, "Prefer
interfaceovertypefor defining object shapes in TypeScript".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/cli/providerSessionArgPartition.ts` around lines 9 - 37, Replace the two exported type aliases with exported interfaces: change ProviderSessionArgPartitionResult and ProviderSessionArgPartitionOptions from "export type ... = Readonly<{ ... }>" to "export interface ..." preserving all property names, optionality, union types and readonly array signatures (e.g. args: readonly string[]), remove the outer Readonly<> wrapper and make each property readonly where appropriate so the shape and immutability are preserved; keep the exported names exactly (ProviderSessionArgPartitionResult, ProviderSessionArgPartitionOptions) so downstream code continues to work.
1-8: ⚡ Quick winAdd a file-level JSDoc responsibility header
This new CLI parser module should include a top-of-file JSDoc block describing responsibilities and boundary behavior.
As per coding guidelines, "Include comprehensive JSDoc comments at the top of each file explaining responsibilities".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/cli/providerSessionArgPartition.ts` around lines 1 - 8, Add a top-of-file JSDoc block to providerSessionArgPartition.ts that states this module's responsibility (parsing provider/session-related CLI arguments and partitioning permission mode aliases), its boundary behavior (pure parsing only, no side effects such as I/O or process.exit), expected inputs/outputs (uses isPermissionMode, PermissionMode, PERMISSION_INTENTS, and parsePermissionModeAliasShared to validate/translate aliases), and error/edge-case handling (how invalid modes are reported). Keep the block concise but explicit about exported behavior, dependencies, and any assumptions so readers know how to use the parser and what it does not do.apps/cli/src/backends/claude/cli/command.ts (1)
91-100: 💤 Low valueMisleading error when
--js-runtimehas no value.When
--js-runtimeis the last argument or followed by another flag,args[i + 1]isundefined, and the code printsInvalid --js-runtime value: undefined. Consider distinguishing "missing value" from "invalid value" for a clearer message, and ensure a value that starts with-is not silently treated as a runtime selector.♻️ Suggested refinement
if (arg === '--js-runtime') { const runtime = args[i + 1]; + if (typeof runtime !== 'string' || runtime.startsWith('-')) { + console.error(chalk.red(`Missing value for --js-runtime. Must be 'node' or 'bun'`)); + process.exit(1); + } if (runtime !== 'node' && runtime !== 'bun') { console.error(chalk.red(`Invalid --js-runtime value: ${runtime}. Must be 'node' or 'bun'`)); process.exit(1); } jsRuntime = runtime; i += 1; continue; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/backends/claude/cli/command.ts` around lines 91 - 100, In the if (arg === '--js-runtime') handler, change the validation to first check the next token (args[i + 1]) for existence and that it doesn't start with '-' (treating that as a missing value) and emit a clear "missing value for --js-runtime" error if so; otherwise assign runtime = args[i + 1] and then validate that runtime === 'node' || runtime === 'bun' and emit the existing "invalid --js-runtime value" message only for other invalid strings; update jsRuntime assignment, increment i, and continue as before (refer to the --js-runtime branch, jsRuntime variable, and the args[i + 1] usage).apps/cli/src/cli/runBackendSessionCliCommand.test.ts (1)
517-587: 💤 Low valueLGTM with one note on copy-pinning.
Coverage is good: provider help passthrough is verified by spawning the resolved provider binary with
--help/exec --helpand asserting backend startup is bypassed.One soft note:
expect.stringContaining('codex CLI Options')andexpect.stringContaining('codex exec --help')couple the test to the exact banner copy inrunBackendSessionCliCommand.ts. If the banner wording is the published contract these are fine; otherwise prefer a more stable marker (e.g., a header sentinel constant exported fromrunBackendSessionCliCommandorbuildRootHelpText).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/src/cli/runBackendSessionCliCommand.test.ts` around lines 517 - 587, The test is brittle because it asserts exact banner copy ("codex CLI Options" and "codex exec --help") from runBackendSessionCliCommand; export a stable header sentinel (e.g., HELP_HEADER or ROOT_HELP_SENTINEL) from runBackendSessionCliCommand or buildRootHelpText and update the tests to assert expect.stringContaining(HELP_HEADER) and expect.stringContaining(`${providerName} exec --help` or a provider-specific sentinel) instead of the hard-coded phrases so wording changes don't break tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/backends/codex/cli/command.ts`:
- Line 15: Remove the lowercase '-v' entry from the versionFlags array in
command.ts (the symbol to edit is versionFlags) so the array no longer includes
'-v' (e.g. change ['-v','-V','--version'] to ['-V','--version'] or ['--version']
per project convention); update any nearby comments/tests/docs that reference
'-v' if present to avoid claiming it is a Codex CLI version flag.
In `@apps/cli/src/cli/providerSessionArgPartition.ts`:
- Around line 222-226: The flag handling for '--account-settings-version-hint'
currently calls readOptionalValue which treats '-1' as "missing" and thus leaves
'-1' to be forwarded into providerArgs on the next loop; change the branch in
providerSessionArgPartition so that when arg ===
'--account-settings-version-hint' you always consume the next token if present
(check args[i+1] and increment i) regardless of readOptionalValue's boolean
return, and do not push that consumed value into providerArgs; reference the
variables/function names arg, args, i, readOptionalValue, and providerArgs when
making this change.
- Around line 56-64: The function readRequiredValue currently treats any next
token as a value, so option-like tokens (e.g., "--help" or "-h") can be
misinterpreted as values; update readRequiredValue to detect tokens that look
like options (start with '-') and treat them as missing by calling fail with the
same message. Specifically, in readRequiredValue (and keeping the existing fail
function usage), after obtaining value = args[index + 1], add a check like
value.startsWith('-') and call fail(`Missing value for ${label}`) when true,
preserving the existing string/empty checks.
In `@apps/stack/scripts/testkit/core/expo_command_shims.mjs`:
- Around line 13-14: The posix shim currently writes to the environment variable
OUTPUT_PATH instead of using the function argument outputPath (while the other
shim uses outputPath), so update the posix shim string (the one containing 'echo
"shim=posix bin=$0 args=$*" >> "${OUTPUT_PATH:?}"') to use the passed-in
outputPath value consistently (same style as the other shim), ensuring both shim
variants reference the outputPath parameter rather than the OUTPUT_PATH env var.
---
Duplicate comments:
In `@apps/cli/src/backends/codex/codexLocalLauncher.ts`:
- Around line 485-493: The fallback branch that sets exitReason = { type:
'switch', resumeId: null } can drop native resume targets stored in
opts.codexArgs; update the condition in the codexLocalLauncher logic so it also
checks for opts.codexArgs?.length (or extracts a resume id from opts.codexArgs
when present) before treating this as a fresh-remote fallback, and if a native
resume arg exists preserve it by setting exitReason.resumeId to that id instead
of null; modify the block containing stopChildIfRunning()/childExited=true to
respect opts.codexArgs and opts.resumeId accordingly.
In `@apps/cli/src/backends/codex/runCodex.ts`:
- Around line 407-414: The initial local-launch call sites are missing
provider-native args: update the two codexLocalLauncher invocations (the
fast-start spawnVendor path where artifacts.localResult is set and the direct
mode === 'local' launcher call) to pass codexArgs: opts.codexArgs ?? [] (same
threading used for runCodexLocalModePass) so that provider args like "resume
--all" are forwarded on first launch; locate the calls to codexLocalLauncher and
add the codexArgs property to the options object passed in.
---
Nitpick comments:
In `@apps/cli/src/backends/claude/cli/command.ts`:
- Around line 91-100: In the if (arg === '--js-runtime') handler, change the
validation to first check the next token (args[i + 1]) for existence and that it
doesn't start with '-' (treating that as a missing value) and emit a clear
"missing value for --js-runtime" error if so; otherwise assign runtime = args[i
+ 1] and then validate that runtime === 'node' || runtime === 'bun' and emit the
existing "invalid --js-runtime value" message only for other invalid strings;
update jsRuntime assignment, increment i, and continue as before (refer to the
--js-runtime branch, jsRuntime variable, and the args[i + 1] usage).
In `@apps/cli/src/cli/providerSessionArgPartition.ts`:
- Around line 9-37: Replace the two exported type aliases with exported
interfaces: change ProviderSessionArgPartitionResult and
ProviderSessionArgPartitionOptions from "export type ... = Readonly<{ ... }>" to
"export interface ..." preserving all property names, optionality, union types
and readonly array signatures (e.g. args: readonly string[]), remove the outer
Readonly<> wrapper and make each property readonly where appropriate so the
shape and immutability are preserved; keep the exported names exactly
(ProviderSessionArgPartitionResult, ProviderSessionArgPartitionOptions) so
downstream code continues to work.
- Around line 1-8: Add a top-of-file JSDoc block to
providerSessionArgPartition.ts that states this module's responsibility (parsing
provider/session-related CLI arguments and partitioning permission mode
aliases), its boundary behavior (pure parsing only, no side effects such as I/O
or process.exit), expected inputs/outputs (uses isPermissionMode,
PermissionMode, PERMISSION_INTENTS, and parsePermissionModeAliasShared to
validate/translate aliases), and error/edge-case handling (how invalid modes are
reported). Keep the block concise but explicit about exported behavior,
dependencies, and any assumptions so readers know how to use the parser and what
it does not do.
In `@apps/cli/src/cli/runBackendSessionCliCommand.test.ts`:
- Around line 517-587: The test is brittle because it asserts exact banner copy
("codex CLI Options" and "codex exec --help") from runBackendSessionCliCommand;
export a stable header sentinel (e.g., HELP_HEADER or ROOT_HELP_SENTINEL) from
runBackendSessionCliCommand or buildRootHelpText and update the tests to assert
expect.stringContaining(HELP_HEADER) and
expect.stringContaining(`${providerName} exec --help` or a provider-specific
sentinel) instead of the hard-coded phrases so wording changes don't break
tests.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4d77634c-be5c-47c1-a1f2-109bb05cd235
📒 Files selected for processing (29)
apps/cli/src/backends/claude/cli/command.help.test.tsapps/cli/src/backends/claude/cli/command.tsapps/cli/src/backends/claude/cli/command.version.test.tsapps/cli/src/backends/codex/appServer/runtime.test.tsapps/cli/src/backends/codex/appServer/runtime.tsapps/cli/src/backends/codex/cli/command.test.tsapps/cli/src/backends/codex/cli/command.tsapps/cli/src/backends/codex/codexLocalLauncher.integration.test.tsapps/cli/src/backends/codex/codexLocalLauncher.testkit.tsapps/cli/src/backends/codex/codexLocalLauncher.tsapps/cli/src/backends/codex/runCodex.fastStart.integration.test.tsapps/cli/src/backends/codex/runCodex.tsapps/cli/src/backends/codex/runtime/localModePass.test.tsapps/cli/src/backends/codex/runtime/localModePass.tsapps/cli/src/cli/dispatch.providerInfoPassthrough.test.tsapps/cli/src/cli/dispatch.tsapps/cli/src/cli/providerCliPassthrough.test.tsapps/cli/src/cli/providerCliPassthrough.tsapps/cli/src/cli/providerSessionArgPartition.test.tsapps/cli/src/cli/providerSessionArgPartition.tsapps/cli/src/cli/runBackendSessionCliCommand.test.tsapps/cli/src/cli/runBackendSessionCliCommand.tsapps/cli/src/cli/sessionStartArgs.test.tsapps/cli/src/cli/sessionStartArgs.tsapps/stack/scripts/testkit/core/env_scope.mjsapps/stack/scripts/testkit/core/expo_command_shims.mjsapps/stack/scripts/utils/expo/command.mjsapps/stack/scripts/utils/expo/command_heap_limit_env.test.mjsapps/stack/scripts/utils/expo/command_workspace_deps_built.test.mjs
Refs happier-dev#168 Co-authored-by: 九川三江 <jiuchuanll@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Validation
yarn workspace @happier-dev/cli vitest run src/cli/providerSessionArgPartition.test.ts src/backends/codex/cli/command.test.ts src/backends/claude/cli/command.help.test.tsyarn workspace @happier-dev/cli vitest run src/backends/codex/codexLocalLauncher.integration.test.ts --config vitest.integration.config.tsyarn workspace @happier-dev/cli vitest run src/backends/codex/runCodex.fastStart.integration.test.ts --config vitest.integration.config.tsnode --test apps/stack/scripts/utils/expo/command_heap_limit_env.test.mjsyarn workspace @happier-dev/cli typecheckgit diff --checkyarn workspace @happier-dev/cli test:integration src/backends/codex/codexLocalLauncher.integration.test.tsruns the sharded integration lane, not just the requested file. The Codex launcher file passed, but that broad shard run surfaced unrelated Claude integration failures inclaudeLocalLauncher.integration.test.tsandrunClaude.fastStart.integration.test.ts.Notes
jiuchuanll:ll-dev-fixAndOptimize-0505devab93fcc866cf4ad6a8c27fbff68a4b2bfb251f6cNeed help on this PR? Tag
@codesmithwith what you need.Summary by CodeRabbit