Skip to content

fix: preserve windows shell working directories - #5062

Merged
JJLiebig merged 1 commit into
masterfrom
issue/4472-windows-cwd
Oct 7, 2026
Merged

JJLiebig merged 1 commit into
masterfrom
issue/4472-windows-cwd

Conversation

@JJLiebig

@JJLiebig JJLiebig commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Windows custom shells launched through Scoop could lose their working directory across tabs and restart. Herdr read the native 64-bit process block of the shim's 32-bit CMD process, where cwd was C:\Windows; the actual 32-bit block contained the requested directory. After CMD changed directory, the shim still retained its own launch directory.

Read 32-bit cwd from the WoW64 process block, and use the shim's unambiguous direct shell child for pane cwd. Ordinary process inspection retains exact-PID semantics. Shell launch/lifetime handling, OSC authority and persistence format are unchanged; absent/unreadable shim children use the existing saved/initial fallback.

Refs #4472.

Validation:

  • Extended existing native cwd regression covers System32 and SysWOW64 CMD, launch normalization and live cd. It fails before the WoW64 fix with C:\Windows instead of the changed directory and passes afterward.
  • Shim selector regression fails before the fix and passes afterward; covers same-name wrappers, nested shells, agent descendants and absent/ambiguous children.
  • Full native Windows just check passed again after rebasing onto master 848075f: 3,377 Rust tests passed, 12 existing skips; formatting, Clippy, maintenance, integration, architecture and docs checks passed.
  • Fresh release build passed. Real Scoop v0.6.0 cmdx.exe and same-name cmd.exe shims both passed requested initial cwd, live cd, follow tab, saved workspace cwd and restart on that exact base.
  • Disposable named sessions and their outer panes removed; the main Herdr server remains running and healthy.

The shim resolution deliberately supports one direct known-shell child. An absent or ambiguous shell preserves the existing fallback rather than guessing through agents or subshells. No new dependency, protocol or persistence schema.

@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 84bfc8c6-2063-495d-b54d-94ee676ae342
📥 Commits

Reviewing files that changed from the base of the PR and between 848075f and 70e6da1.

📒 Files selected for processing (3)
  • src/pane.rs
  • src/platform/mod.rs
  • src/platform/windows.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

Windows cwd lookup now supports 32-bit WoW64 process layouts and selects the cwd of a shim’s sole direct pane-shell child. Pane cwd accessors use pane_process_cwd; persistence filtering and fallback behavior remain unchanged. Tests cover live cwd changes for native and WoW64 shells, and shim-child selection.

Suggested reviewers: ogulcancelik

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 70e6d

No actionable working-directory regression is established at the reviewed head. The change is mergeable after normal checks.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 27.27% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 11 functions across 3 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: preserving Windows shell working directories.
Description check ✅ Passed The description directly explains the WoW64 cwd fix, Scoop shim handling, fallback behavior, and validation results.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@kangal-bot

Copy link
Copy Markdown
Collaborator

@coderabbitai review
@greptileai

@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@greptile-apps

greptile-apps Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium risk] Changes how shell working directories are read on Windows.

The PR appears safe to merge; no actionable defect was found.

What we checked:

  • OSC directories still win: cwd() returns reported_cwd before calling pane_process_cwd.
  • Missing shells keep saved directories: shim_shell_pid requires exactly one direct known-shell child. If it cannot select one, the read returns None and cwd_for_persistence() keeps its saved value.

Summary

This PR reads a 32-bit Windows shell’s working directory from its own process block. For Scoop shims, pane directory reads use one direct known-shell child instead of the launcher.

  • Ordinary process_cwd calls still inspect the requested PID.
  • OSC priority, saved-directory fallback, and non-Windows behavior remain unchanged.
  • Regression tests cover live CMD directory changes and absent, nested, or ambiguous shim children.

JJLiebig explicitly limits shim support to one direct known-shell child. Other layouts keep the existing fallback rather than guessing.

Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Pane directory read] --> B{Scoop shim?}
  B -->|No| C[Read requested process]
  B -->|Yes| D{One direct known-shell child?}
  D -->|Yes| E[Read shell child]
  D -->|No| F[Return no directory]
  C --> G{WoW64 process?}
  E --> G
  G -->|Yes| H[Read 32-bit process block]
  G -->|No| I[Read native process block]
  F --> J[Persistence keeps saved directory]
Loading

Reviews (2) · Last reviewed commit: "fix: preserve windows shell working dire..." · Reviewed by Greptile

@JJLiebig
JJLiebig force-pushed the issue/4472-windows-cwd branch from fd150ce to 70e6da1 Compare October 7, 2026 21:20
@JJLiebig

JJLiebig commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai review
@greptileai

Rebased onto master 848075f (merged #5019). Please review current head 70e6da1. Full native Windows checks passed again (3,377 Rust tests, 12 existing skips), and both real-Scoop cd/follow/persist/restart smokes passed on the rebuilt binary.

@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
⚠️ Action not completed

No files to review.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@JJLiebig

JJLiebig commented Oct 7, 2026

Copy link
Copy Markdown
Collaborator Author

@coderabbitai full review

Please complete a fresh full review of rebased head 70e6da1. The new CodeRabbit status says the review was skipped due to label configuration, but the PR has no labels and the summary still covers fd150ce. We need current-head coverage after the rebase.

@coderabbitai

coderabbitai Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
✅ Action performed

Full review finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants