fix(terminal): avoid retained PowerShell for stock Codex on Windows - #9052

@coderabbitai review

@coderabbitai full review

No actionable comments were generated in the recent review. 🎉

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 @coderabbitai help to get the list of available commands.}

@coderabbitai The outside-diff max-lines finding is not introduced by this PR: /* oxlint-disable max-lines */ is byte-identical at line 1 in origin/main, while this diff changes only lines 4398-4403 in that file. Removing the grandfathered suppression would be unrelated scope; pnpm lint and check:max-lines-ratchet both pass and confirm no new bypass. Please treat that outside-diff item as non-actionable for this PR.

@coderabbitai Please list the exact TypeScript functions counted as documented and undocumented for the 17.46% docstring coverage result, excluding test callbacks if the metric already does so. I will add concise contract-focused documentation to the appropriate production APIs.

@slashdevcorpse Thanks for the precise verification. Agreed: the suppression predates this PR, is unchanged, and the passing ratchet check confirms this change introduces no new bypass. I’m treating the outside-diff max-lines finding as non-actionable and withdrawing it.

Learnt from: slashdevcorpse
URL: https://github.com/stablyai/orca/pull/9052

Timestamp: 2026-07-16T19:10:55.301Z
Learning: For PR `#9052`, the `/* oxlint-disable max-lines */` suppression at Line 1 of the affected file is grandfathered and byte-identical to `origin/main`; the PR changes only Lines 4398–4403. `pnpm lint` and `check:max-lines-ratchet` pass, so this PR does not introduce a new max-lines bypass.

#!/bin/bash
set -euo pipefail

printf '%s\n' '== References to docstring coverage / percentage =='
rg -n -i -C 3 'docstring|documentation coverage|17\.46|max-lines-ratchet' \
  -g '!*node_modules*' -g '!*.lock' . || true

printf '%s\n' '== Candidate config and script filenames =='
fd -HI -t f -i '(doc|string|coverage|lint|ratchet)' . \
  -E node_modules -E .git -E dist -E build | sed -n '1,240p'

printf '%s\n' '== package scripts mentioning documentation or coverage =='
if [ -f package.json ]; then
  jq -r '.scripts // {} | to_entries[] | select((.key + " " + .value | test("doc|string|coverage"; "i"))) | "\(.key): \(.value)"' package.json
fi

#!/bin/bash
set -euo pipefail

repo="stablyai/orca"
pr="9052"

printf '%s\n' '== PR reviews =='
gh api "repos/$repo/pulls/$pr/reviews?per_page=100" --paginate \
  --jq '.[] | {id, user: .user.login, state, body, submitted_at}' || true

printf '%s\n' '== PR issue comments matching coverage/docstrings =='
gh api "repos/$repo/issues/$pr/comments?per_page=100" --paginate \
  --jq '.[] | select((.body | test("17\\.46|docstring|documentation coverage"; "i"))) | {id, user: .user.login, body, created_at, html_url}' || true

printf '%s\n' '== PR review comments matching coverage/docstrings =='
gh api "repos/$repo/pulls/$pr/comments?per_page=100" --paginate \
  --jq '.[] | select((.body | test("17\\.46|docstring|documentation coverage"; "i"))) | {id, user: .user.login, path, line, body, created_at, html_url}' || true

@slashdevcorpse I can’t truthfully provide that exact list from this run. The available pre-merge artifact exposes only the scalar result—17.46% and an 80% threshold—and does not include:

• the documented/undocumented numerator and denominator;
• the function identities or source ranges;
• the docstring-recognition rule; or
• whether .test.ts/.test.tsx callbacks were excluded.

I also verified that this repository has no docstring-coverage script, configuration, or generated report from which the inventory could be reproduced. Producing a manual AST-derived list would therefore not be the exact set used by the reported metric.

Accordingly, this warning is not actionable in its current form: it needs an itemized report (at minimum file, function, line, and documented status), plus the test-file/callback exclusion rule, before it can support targeted production documentation work.

@coderabbitai Since you confirmed the 17.46% docstring warning has no itemized functions, denominator, recognition rule, or test exclusion rule—and therefore is not actionable—please withdraw that pre-merge warning for this PR. Adding speculative filler across production and test callbacks would conflict with the repository's concise, non-obvious-why comment policy. The new public contracts and safety constraints are already documented where they are non-obvious.

@slashdevcorpse Agreed. The 17.46% docstring-coverage warning lacks the itemized and rule-level evidence needed to produce a precise, reviewable remediation request, so it is non-actionable for this PR. I’m withdrawing it.

I also recognize the repository preference for concise documentation focused on non-obvious contracts and rationale; the relevant new public contracts and safety constraints are already documented accordingly.

Learnt from: slashdevcorpse
URL: https://github.com/stablyai/orca/pull/9052

Timestamp: 2026-07-16T19:17:00.695Z
Learning: For PR `#9052`, do not request speculative documentation solely to satisfy an unauditable docstring-coverage percentage. This repository prefers concise comments and documentation focused on non-obvious contracts and rationale; the Windows Codex handoff public contracts and safety constraints are already documented where needed.

@coderabbitai review

@coderabbitai review

@coderabbitai full review

@coderabbitai The rebased full review has reintroduced the same unitemized docstring-coverage warning, now at 20.00%. Your earlier analysis and withdrawal still apply: the artifact provides no function inventory, denominator, recognition rule, or test/callback exclusion rule, while the rebase is patch-identical (git range-diff marks both commits =). Please withdraw this repeated pre-merge warning for the rebased head as non-actionable; no production contract changed during the rebase.

@coderabbitai review

@coderabbitai review

@coderabbitai review

@coderabbitai The final head 363d0a7 addresses every actionable and nitpick item, including the collapsed TerminalHostSubprocessOptions refactor, and your final incremental review finished with zero new threads. The walkthrough still repeats the unitemized 20.00% docstring warning that you previously withdrew at #9052 (comment) as non-actionable. Please withdraw/remove that repeated warning for the final head: it still provides no function inventory, denominator, recognition rules, or exclusions, and adding blanket documentation would conflict with this repository's concise why-only comment guidance.

@slashdevcorpse Agreed. The repeated 20.00% docstring-coverage walkthrough warning remains non-actionable: it still lacks the itemized function set, denominator, recognition criteria, and exclusion rules needed for a targeted, reviewable request.

Given the patch-identical rebase and no relevant production-contract changes, I’m withdrawing that repeated warning for the final head. No blanket documentation additions are warranted.

🐇 ✅

@AmethystLiang @brennanb2025 — PR #9052 is finalized and ready for review at 363d0a7. It is linked to and will close #8814, is mergeable, and CodeRabbit's final-head review is complete with every actionable finding and nitpick addressed, the repeated non-actionable warning withdrawn, and zero unresolved threads. The three fork workflows are waiting for maintainer approval (PR Checks, Computer-use e2e, and Linux Wayland GPU Sandbox)

Re-assigning to @brennanb2025 for reviewing since Brennan is working on codex integration recently

Closing as not planned. The retained PowerShell memory is material, but this fast path intentionally skips the selected shell profile and execution-policy semantics while Codex runs, changing PATH/environment/auth/tool setup for those panes. It also adds a native handoff host and pinned upstream shim/entrypoint allowlists across dozens of files. That fails zero user-facing trade-offs and low ongoing maintenance.