Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughResume-command generation in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RestorableAgentSession
participant ResumeProviderSpec
participant PreserveOptions
participant SessionPlacement
participant Launcher
participant EnvironmentFilter
Client->>RestorableAgentSession: request resumeArguments(snapshot, launcher)
RestorableAgentSession->>ResumeProviderSpec: select spec (launcherOverrides/defaultInvocation)
ResumeProviderSpec->>PreserveOptions: apply option-preservation policy
PreserveOptions-->>ResumeProviderSpec: sanitized args
ResumeProviderSpec->>SessionPlacement: build argv with sessionId placement
SessionPlacement-->>RestorableAgentSession: final argv
RestorableAgentSession->>EnvironmentFilter: filter env via safeEnvironmentKeys
RestorableAgentSession->>Launcher: produce resumeCommand (argv + filtered env)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 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)
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 SummaryThis PR replaces the monolithic Confidence Score: 5/5Safe to merge — all findings are minor style/clarity suggestions with no correctness impact. The refactoring preserves all prior dispatch paths (launcher overrides, default invocations, option policies), and the behavioral change (env-key allowlists split per provider) is intentional, tested, and consistent with the PR goal. The three P2 comments concern function visibility, enum-case naming clarity, and a missing comment on silent fallback — none affect runtime correctness. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[resumeShellCommand] --> B[kind.resumeProviderSpec]
B --> C{launchCommand?.launcher}
C -- "nil or missing" --> D["?? .use(defaultInvocation)"]
C -- "in launcherOverrides" --> E{launcherBehavior}
E -- ".unsupported" --> F[return nil]
E -- ".use(invocation)" --> G[commandParts]
D --> G
G --> H[strip leadingSubcommand from tail]
H --> I[sanitizeArguments]
I --> J[preserveOptions via policy]
J -- "rejectOption found" --> F
J -- "nonRestorableCommand found" --> F
J -- success --> K[sessionPlacement.build]
K -- ".afterPrefix" --> L["exec + subcommand + prefix + sessionId + preserved"]
K -- ".afterPreserved" --> M["exec + subcommand + prefix + preserved + sessionId"]
L --> N[wrap env keys via safeEnvironmentKeys]
M --> N
N --> O[shellSingleQuoted join]
Reviews (1): Last reviewed commit: "refactor: declarative agent resume specs" | Re-trigger Greptile |
![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: 1
🧹 Nitpick comments (1)
Sources/RestorableAgentSession.swift (1)
57-57: Useprivatehere.SwiftLint flagged this as
private_over_fileprivate; the helper is only used in this file, soprivatekeeps the narrower access level.♻️ Proposed fix
-fileprivate func isOpenCodeInternalWorkerArgument(_ value: String) -> Bool { +private func isOpenCodeInternalWorkerArgument(_ value: String) -> Bool {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/RestorableAgentSession.swift` at line 57, Change the access level of the helper function isOpenCodeInternalWorkerArgument from fileprivate to private; locate the function declaration for isOpenCodeInternalWorkerArgument(_ value: String) and replace fileprivate with private to narrow its visibility to this file's scope and satisfy the SwiftLint private_over_fileprivate rule.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/RestorableAgentSession.swift`:
- Around line 191-201: The variadicOptions array in RestorableAgentSession.swift
is incorrectly treating repeatable single-value flags (e.g., "--add-dir") as
variadic; because optionWidth consumes all following non-dash tokens this can
swallow the original prompt into the flag value. Remove these repeatable
single-value flags (like "--add-dir", "--file", "--mcp-config", etc.) from
variadicOptions and treat them as single-value repeatable options in the
resume-generation logic (the code paths using optionWidth and the resume command
builder), ensuring each occurrence only consumes one following token; then add a
regression assertion in the existing long "--add-dir" test case to assert that
the original prompt text does not appear as a value after "--add-dir" in the
generated resume command.
---
Nitpick comments:
In `@Sources/RestorableAgentSession.swift`:
- Line 57: Change the access level of the helper function
isOpenCodeInternalWorkerArgument from fileprivate to private; locate the
function declaration for isOpenCodeInternalWorkerArgument(_ value: String) and
replace fileprivate with private to narrow its visibility to this file's scope
and satisfy the SwiftLint private_over_fileprivate rule.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8c821e7-211d-43c0-b347-572ac4a2b8c2
📒 Files selected for processing (2)
Sources/RestorableAgentSession.swiftcmuxTests/SessionPersistenceTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/cmux.swift (1)
14069-14080:⚠️ Potential issue | 🟠 MajorRestore
--add-dirto Codex variadic options for correct multi-directory preservation.Line 14073 sets
variadicOptionsto only["--image", "-i"], omitting--add-dir. With this configuration,codex --add-dir dirA dirB --model xsanitizes to consume onlydirA;dirBbecomes a positional and stops parsing, dropping--model x. Tests incmuxTests/SessionPersistenceTests.swift(lines 1712–1728) confirm that multiple--add-direntries must survive sanitization and resume generation.RestorableAgentSession.swiftmarks--add-dirasgreedyValueOptions(line 200), andclaudeLaunchVariadicOptions(line 14275) already includes it. Apply the fix:Fix
case "codex": return sanitizedAgentOptions( tail, valueOptions: Self.codexLaunchValueOptions, optionalValueOptions: [], - variadicOptions: ["--image", "-i"], + variadicOptions: ["--add-dir", "--image", "-i"], nonRestorableCommands: Self.codexLaunchNonRestorableCommands,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 14069 - 14080, The variadicOptions passed into sanitizedAgentOptions for the codex launch is missing "--add-dir", causing multiple --add-dir arguments to be mis-parsed; update the variadicOptions array in the codex branch where sanitizedAgentOptions is called (the call that currently uses Self.codexLaunchValueOptions and variadicOptions: ["--image","-i"]) to include "--add-dir" (and its short form if applicable) so that RestorableAgentSession's greedyValueOptions and claudeLaunchVariadicOptions behavior are respected and multiple --add-dir entries survive sanitization and resume generation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/RestorableAgentSession.swift`:
- Around line 57-59: sanitizeArguments currently only inspects original.tail
while stripInternalWorkerArguments
(OpenCodeResumeArgumentFilter.stripInternalWorkerArguments) can remove the
actual worker path from argv[0], so the resumed command can still launch
original.executable (e.g., bun) and ignore provider overrides; update the
sanitization to also set/overwrite the executable when internal launcher details
are stripped: in OpenCodeResumeArgumentFilter.stripInternalWorkerArguments and
wherever sanitizeArguments is used, when you detect and remove an internal
worker argv[0] or a bun+script pattern, force the launch executable to the
provider's resolved executable (apply provider spec overrides such as cmux for
claudeTeams/omo) so both argv[0] and the executable are consistent with the
sanitized tail.
---
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 14069-14080: The variadicOptions passed into sanitizedAgentOptions
for the codex launch is missing "--add-dir", causing multiple --add-dir
arguments to be mis-parsed; update the variadicOptions array in the codex branch
where sanitizedAgentOptions is called (the call that currently uses
Self.codexLaunchValueOptions and variadicOptions: ["--image","-i"]) to include
"--add-dir" (and its short form if applicable) so that RestorableAgentSession's
greedyValueOptions and claudeLaunchVariadicOptions behavior are respected and
multiple --add-dir entries survive sanitization and resume generation.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 06b3f4c2-2d6f-4aaa-b9a6-abc91d7afc66
📒 Files selected for processing (3)
CLI/cmux.swiftSources/RestorableAgentSession.swiftcmuxTests/SessionPersistenceTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/SessionPersistenceTests.swift
1 participant
Summary
Testing
Issues
Summary by CodeRabbit
Refactor
Bug Fixes
Tests
Summary by cubic
Refactored agent resume logic to use declarative provider specs and tightened option handling. Codex now preserves repeatable
--add-dirwhile only--image/-iconsume multiple values; tests cover this and per‑provider environment allowlists.AgentResumeProviderSpecandAgentResumeOptionPolicywith declarative flag handling, dropped/reject lists, and session placement; unified resume command building and moved safe env keys to per‑provider allowlists (with tests).opencode, skips Claude hook settings, respects Codexresumepositional rules.--image/-i;--add-diris preserved as repeatable (not greedy). Updated CLI to stop treating--add-diras variadic.claudeTeamsruns ascmux claude-teams,omoascmux omo;omxandomcresume is not supported.Written for commit 3998818. Summary will update on new commits.