Skip to content

feat(runtime): support explicit Git Bash selection on Windows - #2684

Merged
Astro-Han merged 12 commits into
apache:mainfrom
me2seeks:feat/2195-git-bash-shell
Aug 19, 2026
Merged

feat(runtime): support explicit Git Bash selection on Windows#2684
Astro-Han merged 12 commits into
apache:mainfrom
me2seeks:feat/2195-git-bash-shell

Conversation

@me2seeks

@me2seeks me2seeks commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a Host-owned auto / git_bash shell preference while keeping the existing PowerShell-first Windows detection unchanged by default
  • validate an explicit bash.exe on the Runtime Host before persisting it, then use the resolved ShellPlan for Bash tools, child agents, background and PTY execution, the integrated terminal, and model-facing shell guidance
  • add the Desktop setting and preserve older Runtime Policy documents through the schema migration; the legacy System32\bash.exe WSL shim uses stdin for non-PTY commands

The executable path is intentionally resolved on the Runtime Host, not the Desktop Client. This keeps remote Desktop connections honest and makes an invalid or later-missing explicit path fail closed instead of silently returning to PowerShell.

Fixes #2195

Desktop

Git Bash setting in Desktop

Verification

  • npm run build:test
  • npm run test:dist:serial — all workspace tests passed
  • npm run typecheck
  • npx biome check .
  • git diff --check
  • rendered and inspected Product/Settings/Pages/General Git Bash in Storybook at 1440×1000

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No
中文对照

概要

  • 增加由 Runtime Host 持有的 auto / git_bash shell 偏好;默认仍保持现有的 Windows PowerShell 优先检测
  • 显式选择时,由 Runtime Host 在持久化前验证 bash.exe,并将解析出的同一个 ShellPlan 用于 Bash 工具、子 Agent、后台与 PTY 执行、集成终端以及面向模型的 shell 方言说明
  • 增加 Desktop 设置,并通过 schema 迁移兼容旧 Runtime Policy 文档;旧版 System32\bash.exe WSL shim 的非 PTY 命令通过 stdin 输入

可执行文件路径刻意在 Runtime Host 而不是 Desktop Client 上解析。这样远程 Desktop 连接不会误用客户端机器的路径;显式路径无效或之后消失时也会失败关闭,而不会静默回退到 PowerShell。

AI use

  • No generative tool was used for implementation.
  • Generative tooling was used and the result was reviewed and verified by the author.

Tool(s) and scope: OpenAI Codex (Maka) assisted with the original implementation, tests, and review remediation. OpenAI Codex handled the current-main rebase, compatibility-epoch conflict resolution, and final local verification.

Final squash trailers:

Generated-by: Maka
Generated-by: Codex

@me2seeks
me2seeks force-pushed the feat/2195-git-bash-shell branch from b7edf2a to 086dedb Compare August 12, 2026 14:15

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex automated review

The Runtime Host is the right owner for explicit shell resolution, and the direct spawn plans preserve the existing fail-closed boundary without introducing a second shell authority. The PR is one coherent vertical slice and its current checks are green, although the branch now conflicts with main.

I found one production-blocking PTY failure and two narrower resource/working-directory issues; details are inline. I did not find a P0 issue, a useful independent split, or low-value tests that should simply be removed.

Disclosure: This is an automated review performed by Codex using delegated adversarial review passes and a final evidence check. It has not been independently verified by Astro-Han or another human reviewer, does not constitute human approval, and does not represent the final judgment of a human reviewer.

Comment thread packages/runtime-host/src/server/runtime-resource-coordinator.ts Outdated
Comment thread packages/runtime-host/src/server/runtime-resource-coordinator.ts
Comment thread packages/runtime-host/src/server/runtime-resource-coordinator.ts
@me2seeks
me2seeks force-pushed the feat/2195-git-bash-shell branch 3 times, most recently from 4e80aa8 to 3d6636a Compare August 17, 2026 14:44
@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Problem solved

This PR adds Windows shell preferences owned by the Runtime Host:

  • auto preserves PowerShell 7 → Windows PowerShell → cmd.exe detection.
  • git_bash supports a validated Git Bash executable.
  • Legacy System32\bash.exe uses stdin-based execution.
  • The resolved ShellPlan applies to Bash tools, child agents, background runs, PTY runs, integrated terminals, and shell guidance.
  • Invalid or missing paths fail closed without PowerShell fallback.
  • Desktop settings expose the preference and executable path.
  • Runtime Policy schema migration preserves older documents.

Source of truth

This PR extends the existing Runtime Policy, shell detection, and process execution paths. It does not create a parallel client-side shell resolver. The Runtime Host validates and resolves the executable. The Desktop Client edits the persisted preference.

Solution scope and complexity

The change is the smallest coherent solution for host-owned shell resolution.

TurnShellPlan keeps the shell choice and setup status stable for each turn. Host-side validation is required because the host performs execution. Separate Git Bash and legacy WSL spawn plans are required because their command and stdin handling differ.

No clear implementation or test deletion is available without weakening fail-closed behavior, migration coverage, or cross-path regression coverage. Some fixture and activation plumbing could be simplified in a follow-up, but the current diff provides regression coverage for lifecycle consistency and should not be removed without equivalent tests.

Validation

The PR adds coverage for:

  • Shell preference normalization and invalid values.
  • Runtime Policy schema version 2 migration.
  • Host-side mutation rejection.
  • Git Bash and legacy WSL resolution.
  • Missing executable behavior without fallback.
  • Foreground, background, PTY, child-agent, and managed execution.
  • Turn-scoped setup errors and guidance.
  • Legacy WSL stdin execution.
  • Git Bash PTY working-directory preservation.
  • Desktop Git Bash settings and Storybook rendering.
  • Windows test inventory detection and install preflight behavior.

Reported verification includes 2,896 runtime tests, 986 runtime-host tests, 922 Desktop tests, Desktop typechecking, and a 62-entry Windows inventory. The Windows baseline now validates three PTY tests and three passes. Final required-check status remains unverified from the provided evidence.

Complexity delta

  • Authorities: Shell resolution has one Runtime Host authority. Desktop-side executable resolution is avoided.
  • States: The PR adds auto, git_bash, legacy WSL Bash, and turn-scoped setup-error states.
  • Branches: Spawn logic adds Git Bash, legacy WSL stdin, validation, and fail-closed branches.
  • Configuration: Runtime Policy and application settings gain shell preference and executable fields.
  • Public surface: The PR adds shell types, errors, resolvers, mutation support, and TurnShellPlan.
  • Test burden: Coverage expands across policy, migration, resolution, execution, PTY, Windows inventory, and UI behavior.
  • Removed complexity: Shell choice no longer travels as an unvalidated client-resolved value. Invalid paths no longer trigger implicit fallback.

Total maintenance complexity increases in the short term. The increase is justified by host consistency, explicit configuration, fail-closed behavior, and compatibility protection. The current evidence does not show that the added complexity is unnecessary.

Review-relevant risks

  • Windows users receive new shell settings and fail-closed execution errors. Material user-visible behavior changes require independent human review under repository policy.
  • Runtime Policy schema version 3 and compatibility epoch 27 affect persisted data and peer compatibility. Material release and migration changes require independent human review under repository policy.
  • New process-spawn and stdin handling affects command execution boundaries. Material security and runtime behavior changes require independent human review under repository policy.
  • New exported runtime APIs affect public contracts. Material public API changes require independent human review under repository policy.
  • No licensing or governance effect was identified in the current diff.

The person performing the merge reviews the final diff. A maintainer makes the final determination.

Walkthrough

The PR adds persisted automatic or Git Bash shell selection. Runtime policy migration, validation, host orchestration, shell execution, desktop settings, Windows documentation, and integration tests now support the selected shell.

Changes

Shell selection

Layer / File(s) Summary
Policy contracts and migration
packages/core/..., packages/storage/...
Settings and runtime policy include normalized shell preferences. Schema version 2 documents migrate to version 3 with an automatic shell default.
Shell resolution and execution
packages/runtime/src/shell-*.ts, packages/runtime/src/builtin-tools.ts, packages/runtime/src/pipe-process-driver.ts
The runtime supports Git Bash and legacy WSL Bash plans, PTY execution, stdin scripts, GNU Bash validation, turn-scoped errors, and fail-closed execution.
Runtime host orchestration
packages/runtime-host/src/server/*, packages/runtime-host/src/protocol/index.ts, packages/runtime-host/src/__tests__/*
The host validates set_shell, resolves plans for resources and turns, rebuilds child tools, reports invalid requests, and increments the compatibility epoch.
Desktop settings flow
apps/desktop/src/main/*, apps/desktop/src/renderer/settings/*, apps/desktop/src/renderer/locales/*, apps/desktop/stories/settings/*
The desktop UI exposes automatic and Git Bash selection, executable validation, guarded saves, localized status messages, and a Git Bash Storybook fixture.
Validation and Windows inventory
.github/workflows/*, docs/*, scripts/*, package.json
Windows checks include the Git Bash login PTY test. Documentation and inventory records cover shell selection, validation, path resolution, and portable tests.

Estimated code review effort: 5 (Critical) | ~90 minutes

Merge Risk: 🟡 Moderate · up to 7d5d4

Git Bash execution can report a child process exit even when input was only partially delivered, and Windows test inventory generation can misclassify valid skip conditions. The PR is not merge-ready until these bounded correctness issues are fixed or explicitly accepted.

Sequence Diagram(s)

sequenceDiagram
  participant SettingsUI
  participant RuntimeHostIPC
  participant HostPolicyCoordinator
  participant RuntimePolicyStore
  participant ShellResolver
  participant BashTool
  SettingsUI->>RuntimeHostIPC: save shell preference
  RuntimeHostIPC->>HostPolicyCoordinator: submit set_shell mutation
  HostPolicyCoordinator->>ShellResolver: validate executable
  ShellResolver-->>HostPolicyCoordinator: validation result
  HostPolicyCoordinator->>RuntimePolicyStore: persist validated policy
  BashTool->>ShellResolver: resolve turn shell plan
  ShellResolver-->>BashTool: return Git Bash or automatic plan
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Ai Use Disclosure ⚠️ Warning The PR discloses OpenAI Codex (Maka) use in implementation, tests, and documentation, but 9 of 11 introduced commits lack a standalone Generated-by trailer. Add Generated-by: Maka to each affected commit, then verify it survives squash or amend. Follow CONTRIBUTING.md, Human ownership and AI attribution.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes implement the requested opt-in Git Bash selection, validation, propagation, migration, PTY support, and default PowerShell behavior for [#2195].
Out of Scope Changes check ✅ Passed The workflow, inventory, documentation, migration, Storybook, and regression-test changes support the shell-selection implementation without clear unrelated product changes.
Title check ✅ Passed The title clearly and concisely identifies the main change: explicit Git Bash selection on Windows.
Description check ✅ Passed The description covers the required summary, issue reference, verification, AI use, checklist, behavior change, and user-visible evidence.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed exact head 3d6636ab96bdb0b70be8a92565a24aebf4929dfa, including the full vertical slice, Windows evidence, CI logs, and all resolved review threads.

The problem definition and primary owner are correct. Runtime Host should own shell selection, and one ShellPlan should drive model-facing guidance, Bash execution, child/background work, PTY, and terminal behavior. Direct Git Bash execution, the schema migration, and fail-closed handling are the right direction. The previous WSL PTY, Host-drain, and login-cwd findings are fixed and are not being repeated.

Two related P2 lifecycle issues remain:

  1. createInteractiveRunComposerFactory resolves and validates the saved Git Bash executable while constructing every backend. If a previously valid executable is moved or uninstalled, an ordinary text-only turn fails before any Bash call. A repairable optional-tool configuration error is therefore widened into whole-turn unavailability. Keep execution fail-closed at the Bash/PTY boundary without silently falling back to another shell.

  2. The composer derives model guidance from the turn’s policy snapshot, but the resource coordinator rereads the current shell setting for each operation and overwrites the supplied plan. If the user changes the setting during a turn, the model can be instructed to write PowerShell while the command is executed by Git Bash, or vice versa.

Both findings have one root correction: resolve one Host-owned ShellPlan at turn admission, keep it stable for that turn, and apply configuration changes to subsequent turns. Integrated terminals can similarly capture their plan at launch.

The branch also currently conflicts with the latest main; it needs a rebase and refreshed CI after the lifecycle fix. This remains one cohesive vertical slice and should not be split.

Disclosure: Codex performed the read-only source, lifecycle, Windows evidence, CI, and prior-feedback analysis. The human contributor remains responsible for verifying the findings and deciding whether to merge.

中文

ShellPlan authority 的方向正确,但生命周期未收敛:失效的 Git Bash 路径会阻断普通对话;turn 中途改配置会让 prompt guidance 和实际执行使用不同 shell。应在 turn admission 固定一个 plan,并先 rebase 当前冲突分支。

@me2seeks
me2seeks force-pushed the feat/2195-git-bash-shell branch from 3d6636a to d23d816 Compare August 18, 2026 09:48
@me2seeks

Copy link
Copy Markdown
Contributor Author

Both P2 lifecycle findings are fixed at head d23d81661, and the branch is rebased onto latest main (49f867568; the only conflict was the compatibility epoch, now bumped to 25 with the shell-preference note retained).

Root correction, as suggested: one Host-owned TurnShellPlan is resolved at turn (backend) admission via a new non-throwing resolveTurnShellPlan, stays stable for the turn, and configuration changes apply to subsequent turns.

  1. Broken saved executable no longer fails composition. resolveTurnShellPlan captures a ShellPreferenceError into setupError instead of throwing, so text-only turns compose normally. The Bash/PTY boundary stays fail-closed: all three Bash tool variants (managed, executor, local foreground) rethrow the captured error via throwIfShellSetupFailed before anything spawns — no silent fallback to another shell. The tool description also stops naming a dialect that cannot run and instead declares the outage plus repair (bashToolTurnShellGuidance), and the session environment fragment surfaces Unavailable (...) rather than a stale shell name.
  2. No more per-operation re-resolution over the turn plan. The coordinator now keeps a caller-supplied plan (input.shell ?? resolveShell()); the settings snapshot is only the fallback for plan-less callers. Integrated terminals still capture their plan at launch in #start, matching the suggestion. Child-agent runs resolve at their own admission, so guidance and execution stay consistent within each child even if settings change mid-parent-turn.

Regressions (each verified against the unfixed behavior):

  • shell-detect: broken preference captured without throwing, fail-closed gate, outage guidance, display name (5 tests).
  • shell-tools: description declares the outage; local/managed (foreground + background) execution rejects with ShellPreferenceError before any spawn or host round-trip.
  • runtime-resource-coordinator: supplied turn plan wins for foreground and background without even consulting the snapshot; snapshot fallback retained for plan-less callers.
  • execution-model-composition: backend composition survives a moved Git Bash executable while Bash fails closed and prompts still resolve.

Evidence: runtime 2891 passed / 0 failed (7 skipped), runtime-host 973/973, full-workspace typecheck clean, biome clean. CI is running on the rebased head.

中文

两条 P2 已按建议的根因修复:turn admission 解析一个 Host 持有的 TurnShellPlan,整 turn 稳定,配置变更从后续 turn 生效。失效的 Git Bash 路径不再阻断普通对话(setupError 随 plan 携带,Bash/PTY 边界 fail-closed 抛出,不回退其他 shell);coordinator 不再覆盖调用方 plan,仅对无 plan 调用方(如终端启动,launch 时捕获)回退到设置快照。新增回归均先对未修复行为验证失败。分支已 rebase 到最新 main(仅 compatibility epoch 冲突,提升为 25)。

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (3)
packages/runtime/src/shell-detect.ts (1)

301-306: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Merge the two identical PTY branches.

Both branches return the same value. One condition covers both shell kinds.

♻️ Proposed simplification
-  if (shell.kind === 'legacy-wsl-bash') {
-    return { file: requireExplicitShellExecutable(shell), args: ['-c', command] };
-  }
-  if (shell.kind === 'git-bash') {
+  if (shell.kind === 'legacy-wsl-bash' || shell.kind === 'git-bash') {
     return { file: requireExplicitShellExecutable(shell), args: ['-c', command] };
   }

Disposition: optional.

Source: Path instructions

packages/runtime/src/__tests__/shell-detect.test.ts (1)

151-151: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Inconsistent Windows path escaping in the new shell fixtures. The new fixtures double every separator, so the literal value is C:\\Program Files\\Git\\bin\\bash.exe instead of a real path. Assertions still pass, but the fixtures disagree with the single-separator form used elsewhere in the same suite.

  • packages/runtime/src/__tests__/shell-detect.test.ts#L151-L151: change the fixture to 'C:\\Program Files\\Git\\bin\\bash.exe', matching line 74 in the same file.
  • packages/runtime-host/src/__tests__/runtime-resource-coordinator.test.ts#L351-L351: apply the same single-separator form to the Git Bash executable in the caller-plan test.
  • packages/runtime-host/src/__tests__/runtime-resource-coordinator.test.ts#L370-L370: apply the same single-separator form to the Git Bash executable in the host-fallback test.

Disposition: optional.

Source: Path instructions

packages/runtime/src/shell-tools.ts (1)

341-350: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Delete withShellGuidance. No callers exist outside its definition. Remove the unused helper and its documentation.

Source: Path instructions


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0a6775f9-1d45-4b74-91e3-5abd91ed2908

📥 Commits

Reviewing files that changed from the base of the PR and between 5d9a5b0 and d23d816.

📒 Files selected for processing (32)
  • .github/workflows/windows-baseline.yml
  • apps/desktop/src/main/runtime-host-settings-ipc-main.ts
  • apps/desktop/src/renderer/locales/settings-preferences-copy.ts
  • apps/desktop/src/renderer/settings/general-settings-page.tsx
  • apps/desktop/stories/settings/settings-pages.stories.tsx
  • docs/windows-support.md
  • docs/windows-test-inventory.md
  • packages/core/src/__tests__/runtime-policy-codec.test.ts
  • packages/core/src/__tests__/settings.test.ts
  • packages/core/src/runtime-policy.ts
  • packages/core/src/runtime-policy/policy-codec.ts
  • packages/core/src/settings.ts
  • packages/runtime-host/src/__tests__/execution-model-composition.test.ts
  • packages/runtime-host/src/__tests__/runtime-policy-coordinator.test.ts
  • packages/runtime-host/src/__tests__/runtime-resource-coordinator.test.ts
  • packages/runtime-host/src/protocol/index.ts
  • packages/runtime-host/src/server/execution-composition.ts
  • packages/runtime-host/src/server/interactive-run-composer.ts
  • packages/runtime-host/src/server/runtime-policy-coordinator.ts
  • packages/runtime-host/src/server/runtime-resource-coordinator.ts
  • packages/runtime/src/__tests__/builtin-tools.test.ts
  • packages/runtime/src/__tests__/shell-detect.test.ts
  • packages/runtime/src/__tests__/shell-exec.test.ts
  • packages/runtime/src/__tests__/shell-run-manager.test.ts
  • packages/runtime/src/__tests__/shell-tools.test.ts
  • packages/runtime/src/builtin-tools.ts
  • packages/runtime/src/pipe-process-driver.ts
  • packages/runtime/src/shell-detect.ts
  • packages/runtime/src/shell-exec.ts
  • packages/runtime/src/shell-tools.ts
  • packages/storage/src/__tests__/runtime-policy-stores.test.ts
  • packages/storage/src/runtime-policy/policy-document.ts

Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.

Comment thread docs/windows-test-inventory.md Outdated
Comment thread packages/runtime/src/__tests__/shell-run-manager.test.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4c8bc538-4d6b-4c1b-a44f-19a652ff4546

📥 Commits

Reviewing files that changed from the base of the PR and between d23d816 and 7fc28bb.

📒 Files selected for processing (5)
  • docs/windows-test-inventory.md
  • packages/runtime-host/src/__tests__/execution-model-composition.test.ts
  • packages/runtime-host/src/server/interactive-run-composer.ts
  • packages/runtime/src/__tests__/shell-run-manager.test.ts
  • scripts/windows-test-inventory.mjs
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/runtime/src/tests/shell-run-manager.test.ts
  • packages/runtime-host/src/tests/execution-model-composition.test.ts
  • packages/runtime-host/src/server/interactive-run-composer.ts

Included review availability: Your plan includes up to 3 reviews per rolling hour; 0 remain after this review.

Comment thread scripts/windows-test-inventory.mjs

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shell selection now has one coherent authority per launch/turn: policy resolution produces a concrete plan, the same plan drives model guidance and Bash/PTY execution, child turns resolve their own matching tool surface, and invalid explicit preferences fail closed instead of silently falling back. The Windows spawn boundary remains direct and avoids a second shell interpretation layer.

I found no reproducible P0-P2 issue in the latest head. git merge-tree against current main is clean despite GitHub's stale conflicting status. The only red check is the unrelated slash-command-menu E2E lookup; all shell/runtime-focused checks, including Windows baseline, pass.

Review performed with Codex reviewer agents and DeepSeek V4 Flash as advisory tools; I verified the conclusion against the latest head, current main, and live CI logs.

中文评论

当前 shell selection 在每次 launch/turn 中只有一个清晰权威:policy resolution 生成具体 plan,同一 plan 同时驱动模型提示和 Bash/PTY 执行,child turn 解析自己的匹配 tool surface,无效的显式偏好 fail-closed,不会静默 fallback。Windows spawn 边界保持直接执行,没有第二层 shell 解释。

最新 head 上未发现可复现 P0-P2。尽管 GitHub 暂时显示 conflicting,git merge-tree 对当前 main 的结果是干净的。唯一红色检查是无关的 slash-command-menu E2E 元素查找;shell/runtime 相关检查(包括 Windows baseline)均通过。

本次审查使用了 Codex reviewer agents 与 DeepSeek V4 Flash 作为辅助工具;我已依据最新 head、当前 main 和实时 CI 日志复核结论。

@me2seeks
me2seeks force-pushed the feat/2195-git-bash-shell branch from de572b6 to ab816d7 Compare August 18, 2026 15:37
@me2seeks

Copy link
Copy Markdown
Contributor Author

@Astro-Han The branch is rebased on current main with protocol epoch 26, and the approved one-ShellPlan-per-turn contract is unchanged. Runtime 2896, Runtime Host 986, Desktop 922, Desktop typecheck, and the 62-entry Windows inventory all pass locally. Could you re-review the rebased head when convenient?

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Host-owned, turn-scoped shell selection is the correct authority: the PR validates explicit Git Bash paths, persists one policy, bumps the compatibility epoch, and routes parent Bash/background/PTY surfaces through the resolved plan. One execution path still bypasses that authority. The simplest fix is to stop maintaining a validation-only child-tool composition and have child execution consume the same turn-scoped tool set.

The current package check is failing; I did not attribute that failure to this finding without log evidence.

AI-assisted review disclosure: Codex verified the final finding against the current head and traced Runtime Policy, Host composition, child-tool catalog validation, Runtime Kernel child activation, and Bash construction. Two independent reviewer-agent passes and an OpenCode Go DeepSeek V4 Flash (high) adversarial pass were used as inputs; the alleged stale-branch deletion was discarded because a three-dot PR diff does not delete base-only commits. No local tests were run.

中文复核

由 Host 管理 turn-scoped shell selection 是正确权威:显式 Git Bash 路径得到校验,policy 唯一持久化,compatibility epoch 已提升,父级 Bash/background/PTY 都经过同一 plan。但 child execution 仍绕过这份权威。最小修复是删除只用于校验/catalog 的并行 child-tool composition,让实际 child activation 直接消费同一份 turn-scoped tools。当前 package CI 失败;没有日志证据前,我没有把它归因于这个 finding。

本次为 AI 辅助审查:Codex 在最新 head 上追踪 Runtime Policy、Host composition、child-tool catalog 校验、Runtime Kernel child activation 与 Bash 构造;另使用两次独立 reviewer 及一次 OpenCode Go DeepSeek V4 Flash(high)对抗审查。所谓旧分支删除 main 功能的指控已剔除,因为 three-dot PR diff 不会删除 base-only commit。未运行本地测试。

Comment thread packages/runtime-host/src/server/execution-composition.ts
@Astro-Han

Copy link
Copy Markdown
Contributor

/agentic_review

@qodo-code-review

qodo-code-review Bot commented Aug 18, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Shell updates are discarded ✓ Resolved 🐞 Bug ≡ Correctness
Description
The new shell save submits a shell-only patch, but hasRuntimeHostSettingsPatch does not classify
shell as Host-owned, so SettingsSurface sends it to updateClient, whose ownership filter
removes the field and performs no write. Users can click Save without persisting either the Git Bash
preference or executable, violating the PR's stated Desktop behavior.
Code

apps/desktop/src/renderer/settings/general-settings-page.tsx[R230-232]

+      await props.onUpdate({
+        shell: { preference, executable: normalizedExecutable },
+      });
Relevance

●●● Strong

Direct persistence bug; recent team history accepts correctness fixes where settings state silently
fails or is discarded.

PR-#3147
PR-#2523

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The added save call emits only shell; the central ownership predicate omits that field, and the
resulting client route filters it out. The Host IPC already implements patch.shell, proving the
existing Host route is the intended authority and that no additional persistence mechanism is
required.

apps/desktop/src/renderer/settings/general-settings-page.tsx[226-232]
apps/desktop/src/shared/settings-ownership.ts[7-46]
apps/desktop/src/renderer/settings/settings-surface.tsx[411-430]
apps/desktop/src/main/client-settings-ipc-main.ts[25-36]
apps/desktop/src/main/runtime-host-settings-ipc-main.ts[274-279]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Shell-only updates are incorrectly routed to the client-owned settings endpoint and discarded. Reuse the existing Runtime Host ownership seam by recognizing `patch.shell` in `hasRuntimeHostSettingsPatch`; no new state, branch, configuration, or public surface is needed.

## Issue Context
`ShellSettingsSection` submits `{ shell: ... }`. `SettingsSurface.updateSettings` chooses the Host endpoint only when `hasRuntimeHostSettingsPatch` returns true, while `clientOwnedSettingsPatch` intentionally excludes Host-owned fields.

## Fix Focus Areas
- apps/desktop/src/shared/settings-ownership.ts[33-46]
- apps/desktop/src/renderer/settings/general-settings-page.tsx[226-232]
- apps/desktop/src/renderer/settings/settings-surface.tsx[411-430]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
Review mode: 🧠 Deep: This is a behavior-heavy cross-cutting change spanning policy/schema migration, Host validation, shell detection, foreground/background/PTY execution, terminal startup, model guidance, and Desktop IPC/UI, creating multiple independent defect opportunities that benefit from redundant review passes.

Grey Divider

Tip of the day
💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread apps/desktop/src/renderer/settings/general-settings-page.tsx

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shell-selection authority and discovery design are otherwise coherent: Runtime Host owns the policy, explicit Git Bash paths are validated host-side, fallback/precedence and quoting are consistent, and the current Windows baseline/recovery/sandbox/CLI lanes pass.\n\nOne child-admission race remains below. It is a single-snapshot problem, not a reason to add another shell abstraction: resolve one TurnShellPlan inside the backend activation gate and use that same object for both the bound Bash tool and turn guidance.\n\nThe live Windows package check is currently failing because the packaged renderer did not expose CDP within 30 seconds. The same failure occurred on the preceding head, so I am not attributing it to this diff, but it still needs a successful rerun before merge.\n\nReviewed with Codex using three independent reviewer agents and OpenCode Go DeepSeek V4 Flash (high); I verified the exact head, shell discovery/quoting, child activation paths, current-main seams, Windows checks, and the failing package log classification.\n\n

中文\n\n除下面一个问题外,shell-selection 的权威和发现设计是一致的:Runtime Host 持有 policy,显式 Git Bash path 在 host 侧校验,fallback/precedence 与 quoting 一致,当前 Windows baseline/recovery/sandbox/CLI lanes 均通过。\n\n剩余问题是 child admission 的单一 snapshot 竞态,不需要再新增 shell 抽象。应在 backend activation gate 内解析一次 TurnShellPlan,并让 bound Bash tool 与 turn guidance 复用同一个对象。\n\n当前 Windows package check 因 packaged renderer 在 30 秒内未暴露 CDP 而失败;前一个 head 也有同样失败,因此不归因于本 diff,但合并前仍需要成功重跑。\n\n本次由 Codex 配合三个独立 reviewer agent,以及 OpenCode Go DeepSeek V4 Flash(high)审查;我核验了精确 head、shell discovery/quoting、child activation 路径、当前 main 扩展点、Windows checks 和 package 失败分类。\n\n

Comment thread packages/runtime/src/runtime-kernel.ts Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 79748731-b129-4696-99e2-a1b6f15ee2cc

📥 Commits

Reviewing files that changed from the base of the PR and between 7fc28bb and 31580c5.

📒 Files selected for processing (14)
  • apps/desktop/src/shared/settings-ownership.ts
  • apps/desktop/stories/settings/settings-pages.stories.tsx
  • package.json
  • packages/runtime-host/src/__tests__/execution-model-composition.test.ts
  • packages/runtime-host/src/__tests__/runtime-resource-coordinator.test.ts
  • packages/runtime-host/src/protocol/index.ts
  • packages/runtime-host/src/server/execution-composition.ts
  • packages/runtime-host/src/server/interactive-run-composer.ts
  • packages/runtime-host/src/server/runtime-resource-coordinator.ts
  • packages/runtime/src/__tests__/session-manager.test.ts
  • packages/runtime/src/runtime-kernel.ts
  • packages/runtime/src/session-manager.ts
  • scripts/windows-test-inventory.mjs
  • scripts/windows-test-inventory.test.mjs

Included review availability: Your plan provides up to 3 included reviews per hour; 0 remain after this review.

Comment thread scripts/windows-test-inventory.mjs

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new delta is limited to making the Windows skip inventory understand parenthesized expressions, and the broader Git Bash design remains coherent: Runtime Host resolves one turn shell plan, child activation reuses that snapshot, explicit paths avoid an extra quoting shell, and invalid selections fail closed rather than silently falling back.

No remaining P0-P3 findings on this exact head. Approval is for the code; the still-running Windows/package checks must of course finish successfully before merge.

AI-assisted review disclosure: Codex re-reviewed exact head 1b0dacfd, ran the focused inventory check and diff check, and verified that all current review threads are resolved.

中文说明

最新增量只让 Windows skip inventory 正确识别带括号的表达式。整体 Git Bash 设计仍保持单一权威:Runtime Host 在 turn admission 固定 shell plan,后续路径复用同一 snapshot;显式路径不会再套一层 shell quoting,失效配置也会 fail closed。当前 head 没有剩余 finding;合并前仍需等待正在运行的 Windows/package CI 通过。

@me2seeks
me2seeks force-pushed the feat/2195-git-bash-shell branch from 1b0dacf to 7d5d46f Compare August 19, 2026 13:12
@coderabbitai

coderabbitai Bot commented Aug 19, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@me2seeks

Copy link
Copy Markdown
Contributor Author

Rebased onto current main to resolve the merge conflict. The only manual conflict was the Runtime Host compatibility epoch: main already owns epoch 26 for external-session origins, so this PR now advances the shell-policy wire change to epoch 27 rather than reusing 26.

Local verification on head 7d5d46f:

  • npm run build:test
  • npm run test:dist:serial (all workspace tests passed)
  • npm run typecheck
  • npx biome check .
  • git diff --check

The prior approval predates the rebase, so this head is ready for re-review.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 1205151b-fcc4-4e80-81f1-ab34c08634d7

📥 Commits

Reviewing files that changed from the base of the PR and between 88be145 and 7d5d46f.

📒 Files selected for processing (39)
  • .github/workflows/windows-baseline.yml
  • apps/desktop/src/main/runtime-host-settings-ipc-main.ts
  • apps/desktop/src/renderer/locales/settings-preferences-copy.ts
  • apps/desktop/src/renderer/settings/general-settings-page.tsx
  • apps/desktop/src/shared/settings-ownership.ts
  • apps/desktop/stories/settings/settings-pages.stories.tsx
  • docs/windows-support.md
  • docs/windows-test-inventory.md
  • package.json
  • packages/core/src/__tests__/runtime-policy-codec.test.ts
  • packages/core/src/__tests__/settings.test.ts
  • packages/core/src/runtime-policy.ts
  • packages/core/src/runtime-policy/policy-codec.ts
  • packages/core/src/settings.ts
  • packages/runtime-host/src/__tests__/execution-model-composition.test.ts
  • packages/runtime-host/src/__tests__/runtime-policy-coordinator.test.ts
  • packages/runtime-host/src/__tests__/runtime-resource-coordinator.test.ts
  • packages/runtime-host/src/protocol/index.ts
  • packages/runtime-host/src/server/execution-composition.ts
  • packages/runtime-host/src/server/interactive-run-composer.ts
  • packages/runtime-host/src/server/runtime-policy-coordinator.ts
  • packages/runtime-host/src/server/runtime-resource-coordinator.ts
  • packages/runtime/src/__tests__/builtin-tools.test.ts
  • packages/runtime/src/__tests__/session-manager.test.ts
  • packages/runtime/src/__tests__/shell-detect.test.ts
  • packages/runtime/src/__tests__/shell-exec.test.ts
  • packages/runtime/src/__tests__/shell-run-manager.test.ts
  • packages/runtime/src/__tests__/shell-tools.test.ts
  • packages/runtime/src/builtin-tools.ts
  • packages/runtime/src/pipe-process-driver.ts
  • packages/runtime/src/runtime-kernel.ts
  • packages/runtime/src/session-manager.ts
  • packages/runtime/src/shell-detect.ts
  • packages/runtime/src/shell-exec.ts
  • packages/runtime/src/shell-tools.ts
  • packages/storage/src/__tests__/runtime-policy-stores.test.ts
  • packages/storage/src/runtime-policy/policy-document.ts
  • scripts/windows-test-inventory.mjs
  • scripts/windows-test-inventory.test.mjs
🚧 Files skipped from review as they are similar to previous changes (35)
  • package.json
  • packages/runtime-host/src/tests/runtime-policy-coordinator.test.ts
  • .github/workflows/windows-baseline.yml
  • packages/storage/src/tests/runtime-policy-stores.test.ts
  • packages/runtime/src/tests/shell-exec.test.ts
  • packages/runtime-host/src/protocol/index.ts
  • scripts/windows-test-inventory.test.mjs
  • apps/desktop/src/shared/settings-ownership.ts
  • docs/windows-support.md
  • docs/windows-test-inventory.md
  • packages/runtime-host/src/server/interactive-run-composer.ts
  • packages/runtime/src/tests/shell-detect.test.ts
  • apps/desktop/src/main/runtime-host-settings-ipc-main.ts
  • packages/runtime/src/tests/shell-run-manager.test.ts
  • apps/desktop/src/renderer/locales/settings-preferences-copy.ts
  • scripts/windows-test-inventory.mjs
  • packages/core/src/tests/runtime-policy-codec.test.ts
  • packages/core/src/tests/settings.test.ts
  • packages/runtime-host/src/server/runtime-policy-coordinator.ts
  • packages/runtime/src/shell-exec.ts
  • packages/runtime/src/builtin-tools.ts
  • packages/core/src/runtime-policy/policy-codec.ts
  • packages/runtime/src/tests/builtin-tools.test.ts
  • packages/runtime-host/src/tests/runtime-resource-coordinator.test.ts
  • packages/core/src/settings.ts
  • apps/desktop/stories/settings/settings-pages.stories.tsx
  • apps/desktop/src/renderer/settings/general-settings-page.tsx
  • packages/core/src/runtime-policy.ts
  • packages/runtime-host/src/tests/execution-model-composition.test.ts
  • packages/runtime/src/shell-tools.ts
  • packages/runtime/src/shell-detect.ts
  • packages/runtime/src/tests/shell-tools.test.ts
  • packages/runtime-host/src/server/execution-composition.ts
  • packages/storage/src/runtime-policy/policy-document.ts
  • packages/runtime/src/session-manager.ts

Included review availability: Your plan provides up to 3 included reviews per hour; 0 remain after this review.

Comment thread packages/runtime/src/pipe-process-driver.ts Outdated
@me2seeks
me2seeks force-pushed the feat/2195-git-bash-shell branch from 7d5d46f to 493ebc9 Compare August 19, 2026 13:47
@me2seeks

Copy link
Copy Markdown
Contributor Author

Exact-head CI approval request: all seven workflows for 493ebc9 are action_required after the move to apache/maka. I attempted the GitHub approve endpoint for every run, but it requires repository admin rights; this account has pull-only permission. Please approve the fork workflow runs beginning with CI https://github.com/apache/maka/actions/runs/32260085961 and re-review the new stdin-settlement fix after they complete. Local current-main build, Runtime 2961 pass / 7 platform skips, MCP 115/115, full typecheck, Biome, and diff checks are green. No merge or auto-merge action was taken.

@me2seeks
me2seeks requested a review from Astro-Han August 19, 2026 14:10

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update. I re-reviewed exact head 493ebc93dd04fc45e7a675e31e9a662cf43191c3.

The new process-driver delta looks sound: stdin settlement is included in final exit settlement, expected early-close errors are handled, failure ordering is preserved, and the new regression covers a large payload plus the relevant event order. I found no new P0–P2 code issue, the existing threads are resolved, and the actual Git Bash Settings screenshot satisfies the UI evidence gate.

I cannot approve this head yet because commit-level AI provenance is incomplete. The PR body says Codex materially contributed to the original implementation, tests, and review remediation, but only 3 of the 12 current commits carry a Generated-by: trailer; the first 9 substantive commits do not. Please add the truthful trailer to each materially AI-assisted commit, or narrow the body’s AI-use scope for commits that were entirely human-authored, and preserve the provenance through the final squash/amend.

I have approved the exact-head fork workflows; they are now queued/running and must also finish green before merge readiness.

AI-assisted review disclosure: OpenAI Codex performed the exact-head delta, provenance, UI-evidence, and CI analysis; I verified the cited behavior and live GitHub state before posting.

中文说明

代码 delta 本身没有新的 P0–P2,实际 Git Bash 设置截图也符合要求。当前阻塞是提交级 AI provenance:PR body 声明 Codex 实质参与了原始实现、测试和修复,但当前 12 个提交中只有 3 个带 Generated-by:。请按真实情况给前 9 个实质性 AI 辅助提交补 trailer;若其中确有纯人工提交,则相应收窄 body 的说明。当前 head 的 fork workflows 已授权,仍需等待全部绿色。

A moved or uninstalled saved Git Bash executable previously threw while
constructing every interactive backend, widening a repairable optional-tool
configuration error into whole-turn unavailability; and the resource
coordinator re-resolved the current shell setting per operation, overwriting
the plan the composer had already declared to the model, so a mid-turn
settings change could split guidance from execution.

Resolve one Host-owned TurnShellPlan at turn (backend) admission and keep it
stable for that turn: resolveTurnShellPlan captures a ShellPreferenceError
instead of throwing, the Bash tools (managed, executor, and local
foreground) fail closed with that error at the execution boundary without
falling back to another shell, and the tool description declares the outage
instead of dialect guidance that cannot run. The coordinator keeps a
caller-supplied plan and only consults the settings snapshot for plan-less
callers such as integrated terminal launches, which capture their plan at
launch. Configuration changes apply to subsequent turns.

Regressions: broken-preference composition survives with a fail-closed Bash;
coordinator keeps the turn plan for foreground and background without
consulting the snapshot; shell-detect covers resolution, guidance, and the
boundary gate. Each test was verified against the unfixed behavior.

Generated-by: Maka
Exclude Windows-only regressions from the Windows-skip inventory, exercise missing saved executables through a deterministic Windows resolver, and skip the real Git Bash PTY probe when the runner does not provide Git Bash.

Generated-by: Maka
Resolve child tools and their TurnShellPlan under the same backend activation boundary, pass that plan through backend composition, and revalidate retries against the final admitted snapshot.

Generated-by: Maka
Strip only balanced parentheses that enclose a complete skip expression before applying the Windows-false ternary exemption.

Generated-by: Maka
@me2seeks
me2seeks force-pushed the feat/2195-git-bash-shell branch from 493ebc9 to 48ae714 Compare August 19, 2026 15:32
@me2seeks

Copy link
Copy Markdown
Contributor Author

Addressed at exact head 48ae714. I rewrote commit metadata only: the first nine substantive commits now carry Generated-by: Maka, the two existing Maka remediation commits retain that trailer, and the stdin-settlement commit retains Generated-by: Codex. All 12 commits now match the truthful AI-use scope in the PR body. The final tree is unchanged at 7d2475d08624e7946caf0207dcdf51406b48f28f, identical to reviewed and fully tested head 493ebc9; git diff between the two heads is empty. The metadata rewrite created new exact-head workflow runs, so they may require fork approval again. Please re-review and approve the new workflows when convenient. No merge or auto-merge action was taken.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved exact head 48ae714672d44e187f63ed8f289d9a9b44067ac1.

I verified that this is a provenance-only history rewrite: its tree is byte-identical to the previously reviewed code head, all 12 commits now carry truthful Generated-by: trailers consistent with the PR-body disclosure, all review threads are resolved, and the actual Git Bash Settings screenshot remains available. I found no P0–P3 issue.

I have approved the new exact-head fork workflows. This approval is conditional on those required checks actually running and finishing green before merge.

AI-assisted review disclosure: OpenAI Codex performed the exact-head tree-equivalence, provenance, screenshot, thread, and CI analysis; I verified the commit/tree identities and live GitHub state before approving.

中文说明

新 head 只是 provenance history rewrite,代码 tree 与已审版本完全一致;12/12 commits 的 Generated-by: 已补齐并与 body 一致,截图和线程也都合规,没有 P0–P3。当前 head 的 fork workflows 已授权,全部绿色后才算 merge-ready。

@Astro-Han
Astro-Han merged commit 9aea2c2 into apache:main Aug 19, 2026
19 of 20 checks passed
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.

Allow users to explicitly select Git Bash as the Bash tool shell on Windows

2 participants