Repository navigation
Fix fish resume cwd guard - #6891
Closed
austinywang wants to merge 24 commits into
Closed
austinywang wants to merge 24 commits into
austinywang wants to merge 24 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughPortable shell wrapping is added for restorable startup commands and Hermes bootstrapping. Related tests now assert the new command shapes, and a Fish helper adds sanitized cache-variable naming with integration coverage. ChangesPortable startup command rendering
Fish cache variable helper
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings)
✅ Passed checks (21 passed)
✨ 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 |
Contributor
Greptile SummaryFixes fish shell incompatibility with resume working-directory guards by replacing POSIX brace groups (
Confidence Score: 5/5Safe to merge. The guard format change is fully backward-compatible: stripping handles all prior formats, the new format round-trips correctly, and fish runtime tests confirm end-to-end behavior. All changed production paths are covered by direct unit tests and fish integration tests. The stripping/re-prefixing logic is idempotent and handles the full matrix of old formats. No actor isolation, blocking runtime, or data-loss concerns were introduced. No files require special attention. The two style notes about duplicated literalSingleQuoted and the inlined regex in portableShellCommandParts are non-blocking cleanup opportunities. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Incoming persisted command"] --> B{"Detect format"}
B -- "/bin/sh -c|lc 'payload' (3 words)" --> C["portableShellCommandParts\n(strip legacy guard from payload,\nrewrap with new format)"]
B -- "{ cd ... } && command\n(legacy brace group)" --> D["strippedLegacyChangeDirectoryPrefix\n(matches { cd ... } prefix)"]
B -- "cd -- 'path' ... ] && command\n(new portable format)" --> E["strippedLegacyChangeDirectoryPrefix\n(matches portableParentCdPrefix)"]
B -- "no cwd guard" --> F["command unchanged"]
C --> G["prefix(stripped, cwd)\n→ cd -- 'path' 2>/dev/null || [ ! -d 'path' ] && /bin/sh -c|lc 'payload'"]
D --> G2["prefix(stripped, cwd)\n→ cd -- 'path' 2>/dev/null || [ ! -d 'path' ] && command"]
E --> G2
subgraph "New guard format (fish-safe)"
G
G2
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["Incoming persisted command"] --> B{"Detect format"}
B -- "/bin/sh -c|lc 'payload' (3 words)" --> C["portableShellCommandParts\n(strip legacy guard from payload,\nrewrap with new format)"]
B -- "{ cd ... } && command\n(legacy brace group)" --> D["strippedLegacyChangeDirectoryPrefix\n(matches { cd ... } prefix)"]
B -- "cd -- 'path' ... ] && command\n(new portable format)" --> E["strippedLegacyChangeDirectoryPrefix\n(matches portableParentCdPrefix)"]
B -- "no cwd guard" --> F["command unchanged"]
C --> G["prefix(stripped, cwd)\n→ cd -- 'path' 2>/dev/null || [ ! -d 'path' ] && /bin/sh -c|lc 'payload'"]
D --> G2["prefix(stripped, cwd)\n→ cd -- 'path' 2>/dev/null || [ ! -d 'path' ] && command"]
E --> G2
subgraph "New guard format (fish-safe)"
G
G2
end
Reviews (18): Last reviewed commit: "Merge branch 'main' of https://github.co..." | Re-trigger Greptile |
![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
🤖 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 `@cmuxTests/SessionPersistenceResumeBindingTests.swift`:
- Around line 408-413: The oversized restore test is assuming restoredInput is
always in the “/bin/sh -c <payload>” shape, but the startup-input path can emit
a launcher script such as “/bin/zsh '<script>'” for long commands. Update the
restore assertion flow around restoredInput and portableShellCommandPayload so
it can extract and validate the launcher-script payload before checking the
portable-wrapper contents, or keep the fixture within the inline limit; apply
the same handling in the duplicate restore assertions referenced by the
SessionPersistenceResumeBindingTests helper.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: cf5a1904-4f4f-4dc1-818a-3d536ce27c86
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (2)
Sources/SurfaceResumeCommandCanonicalizer+PortableAgentExecutable.swiftcmuxTests/SessionPersistenceResumeBindingTests.swift
austinywang
force-pushed
the
issue-6285-fish-resume-cwd-guard
branch
from
June 26, 2026 17:59
a8457a7 to
46154bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@cmuxTests/SessionPersistenceResumeBindingTests.swift`:
- Around line 555-560: The test helper in SessionPersistenceResumeBindingTests
is parsing launcher-script content with a hardcoded dropFirst(2), which is
fragile if SurfaceResumeBindingScriptStore.writeLauncherScript changes its
header layout or returnToLoginShell behavior. Update the parsing in the
inlineInput extraction path to identify the actual command section via a
sentinel or other structure-aware logic instead of relying on a fixed line
count, while keeping the /bin/zsh branch and portableShellCommandPayload(from:)
flow intact.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 09796850-5ca0-4b28-a034-4ea4b2d90f43
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (1)
cmuxTests/SessionPersistenceResumeBindingTests.swift
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)
cmuxTests/SessionPersistenceTests.swift (1)
4426-4438: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winKeep the cwd invariant in this regression test.
This now passes even if normalization drops the working-directory hop entirely;
/bin/sh -c 'codex resume session'still satisfies all three assertions. Reuse the existingleadingCdCommand/assertZshCommandChangesDirectoryhelpers here so the test still proves the issue’s cwd-preservation contract.Suggested tightening
-func testAgentHookSurfaceResumeBindingNormalizesLegacyGuardToSingleExternalCommand() { +func testAgentHookSurfaceResumeBindingNormalizesLegacyGuardToSingleExternalCommand() throws { let binding = SurfaceResumeBindingSnapshot( command: "{ cd -- '/tmp/project' 2>/dev/null || [ ! -d '/tmp/project' ]; } && codex resume session", cwd: "/tmp/project", source: "agent-hook", updatedAt: 1 ) XCTAssertTrue(binding.command.hasPrefix("/bin/sh -c "), binding.command) XCTAssertFalse(binding.command.hasPrefix("{ "), binding.command) XCTAssertTrue(binding.command.contains("codex resume session"), binding.command) + let cdCommand = try leadingCdCommand(from: binding.command) + try assertZshCommandChangesDirectory(cdCommand, expectedPath: "/tmp/project") }As per coding guidelines, "When a user says tests missed a bug, add or adjust behavior-level coverage around the exact repro path before claiming the fix is complete."
🤖 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 `@cmuxTests/SessionPersistenceTests.swift` around lines 4426 - 4438, The regression test for SurfaceResumeBindingSnapshot normalization is too weak because it only checks the command prefix and resume text, so it can still pass when the cwd-preserving hop is removed. Tighten testAgentHookSurfaceResumeBindingNormalizesLegacyGuardToSingleExternalCommand by reusing the existing leadingCdCommand and assertZshCommandChangesDirectory helpers to assert the command still performs the directory change before invoking codex resume session, ensuring the cwd invariant remains part of the behavior-level coverage.Source: Coding guidelines
🤖 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 `@cmuxTests/SessionPersistenceTests.swift`:
- Around line 4426-4438: The regression test for SurfaceResumeBindingSnapshot
normalization is too weak because it only checks the command prefix and resume
text, so it can still pass when the cwd-preserving hop is removed. Tighten
testAgentHookSurfaceResumeBindingNormalizesLegacyGuardToSingleExternalCommand by
reusing the existing leadingCdCommand and assertZshCommandChangesDirectory
helpers to assert the command still performs the directory change before
invoking codex resume session, ensuring the cwd invariant remains part of the
behavior-level coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 39097329-0763-4d66-91f1-76ea454cfb2d
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (1)
cmuxTests/SessionPersistenceTests.swift
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
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
3 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.
Fixes #6285.
Summary
Regression structure
Validation
Summary by cubic
Makes resume working-directory guards fish-safe by replacing legacy brace guards with a parent cd, running the payload directly or via
/bin/sh -conly when needed. Safely rewrites portable-shell payloads (literal single-quoted only), preserves nonliteral quoting, bootstraps Hermes inside cwd wrappers, and repairs stale executables and legacy Codex provider names. Fixes #6285.Bug Fixes
cd; detect and unwrap/bin/sh -c/-lcto edit only the inner payload, then rewrap with the original option; choose direct exec vs portable shell based on the payload._cmux_pr_cache_variable_nameto sanitize PR-cache variable names.Refactors
TerminalStartupShellQuotinghelpers.Written for commit 53539ea. Summary will update on new commits.
Summary by CodeRabbit
/bin/sh -cand/bin/sh -lcwrappers, including more reliable working-directory prefixing/stripping and shell quoting.