Skip to content

fix(settings): load Windows profiles with malformed shell values - #28217

Closed
brennanb2025 wants to merge 1 commit into
mainfrom
brennanb2025/chat-permission-shell-guard
Closed

brennanb2025 wants to merge 1 commit into
mainfrom
brennanb2025/chat-permission-shell-guard

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​105 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​104
Prod 1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​4 $\color{#cf222e}{\Huge{\mathbf{−}}}$​1 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​3

ELI5

After #28100, a Windows profile with an invalid saved shell value could fail to load if it had no saved chat permission choice. These profiles now load successfully and use the normal PowerShell family when choosing their initial chat permission setting.

What Changed

Check that the saved Windows shell is a string before passing it to chat permission seeding. Valid strings, including custom executable paths, pass through unchanged; other values use the existing shell default. Saved permission choices, migrated arguments, and the requirement that both Claude and Codex bypass approvals keep their current behavior.

Why

Persisted profile data can contain values outside the declared type. Checking this value where it enters permission seeding prevents the settings-load failure while keeping the existing shell resolver and default.

Linked Issue

Follow-up to #28100; addresses review finding R28100-READY-01.

Visual Proof

N/A — this is a settings-loading guard with no interface change.

Testing

  • I manually tested these changes locally
  • Automated tests added/updated, or explained why not below

Tests ran on macOS with the Windows platform simulated. The new normalization cases reproduced eight shell?.trim is not a function failures before the guard. With the guard, all 116 targeted tests passed across settings normalization, chat permissions, and launch arguments; the real pnpm tc:node also passed. The cases cover malformed values, default quoting, shell-specific quoting and custom paths, saved permission choices, both agents, and arguments after migration.

orca-ci-checks --no-typecheck: 22/23 passed. The only failure was the known local Bun tooling error, Script not found "check"; the real node typecheck passed separately. Native Windows confirmation is being coordinated separately.

Review

Self-reviewed the two-file diff. No shell parser, permission-config, or interface changes.

Agent skill upstream boundary

  • Not applicable, or this change copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.

Notes

The existing host-side settings path handles the guard. There are no transport or remote wire changes. This prevents permission seeding from failing; it does not repair the malformed stored shell value or change later terminal launches.

Checklist

  • This PR is small and focused
  • I explained what changed and why (ELI5, the user-facing before/after, the mechanism, and why over the alternatives)
  • Before/after screenshots or videos attached for UI changes, or N/A with reason
  • Self-reviewed for correctness, security, and performance
  • Cross-platform, SSH/remote, and path/shortcut impact considered (or N/A)
  • pnpm lint, pnpm typecheck, pnpm test, and pnpm build pass (or CI will cover; local preferred)

CI covers the remaining projects and build checks.

@coderabbitai

coderabbitai Bot commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

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: b722bb25-200e-4614-be75-ded1e05ce91f
📥 Commits

Reviewing files that changed from the base of the PR and between 687190e and aa52cf6.

📒 Files selected for processing (2)
  • src/main/persistence/loading-store/normalize-loaded-global-settings.test.ts
  • src/main/persistence/loading-store/normalize-loaded-global-settings.ts

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

📝 Walkthrough

The normalization path now passes terminalWindowsShell to loadedChatPermissionSetting only when the persisted value is a string. Windows-specific tests cover permission seeding for missing, malformed, and valid shell values, as well as saved permission choices and migrated arguments.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to aa52c

No actionable issue is established for this change; it is mergeable after normal checks.

Pre-merge checks · Passed 4 · Failed 1

Failed Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks (4 passed)
Check name Status Explanation
Title check Passed The title clearly and concisely identifies the main change: fixing Windows profile loading when saved shell values are malformed.
Description check Passed The description is complete and directly addresses the required sections. It explains the user impact, implementation, rationale, linked issue, lack of visual changes, test coverage, review scope, and…
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
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
  • Autofix · 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.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Closing. A real-Windows repro showed this guard doesn't change the outcome: with a non-text saved Windows shell, Orca already fails to start before #28100 (the startup shell PATH setup trims the same value), and with this PR it fails one step later in the same way. Nothing in Orca writes a non-text value, so it needs a hand-edited or corrupted settings file. Not pursuing for now.

@brennanb2025
brennanb2025 deleted the brennanb2025/chat-permission-shell-guard branch October 14, 2026 05:54
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.

1 participant