Skip to content

test(codex-cloud): budget the shell-policy case for Windows Git Bash spawns - #2255

Merged
BigSimmo merged 2 commits into
mainfrom
claude/clever-edison-167y82
Aug 21, 2026
Merged

test(codex-cloud): budget the shell-policy case for Windows Git Bash spawns#2255
BigSimmo merged 2 commits into
mainfrom
claude/clever-edison-167y82

Conversation

@BigSimmo

Copy link
Copy Markdown
Owner

Summary

  • Give codex-cloud-setup › "writes managed shell policy behaviorally and preserves unrelated Codex config" an explicit 120_000 ms budget, matching the sibling case directly below it, and record why in a comment.

This closes out the first of the five failures reported from a full npm run test on a Windows workstation. It is not the state failure it looked like.

What the Windows evidence showed

Re-running the two deterministic cases on that machine in isolation, both passed — but the timings are the finding:

✓ tests/hosted-migration-role-guard.test.ts (5 tests) 1557ms
✓ accepts the current repository state 1550ms
✓ tests/codex-cloud-setup.test.ts (34 tests) 149859ms
✓ writes managed shell policy behaviorally and preserves unrelated Codex config 24958ms
✓ pins connected retrieval mode and rejects unsafe shell-policy configs 112948ms

The failing case spawns bash scripts/setup-codex-cloud.sh twice. That is cheap on Linux and expensive on Windows, where every spawn goes through Git Bash. 24.96 s against the 30 s testTimeout in vitest.config.mts is 83% of the budget with nothing else running. A full npm run test puts four workers (maxWorkers: 4) on the machine at once, and it tips over — which is exactly why it failed there in the full suite, passed there in isolation, and passes everywhere on Linux.

The sibling case immediately below already carries }, 120_000) for the same reason, and measures 112.9 s on that machine. This one was simply missed when that budget was added.

Correcting the prior analysis

PR #2252 recorded these two cases as deterministic state assertions, and reasoned that a Windows-only failure in either would therefore be a real Windows-only defect rather than load noise. That reasoning was wrong for this case: the assertions are deterministic, but the runtime is not, because of the subprocess cost. The conclusion it led to — "prioritise item 2, it suggests real repo drift" — was also wrong. validateRepository() is happy on both platforms.

hosted-migration-role-guard › "accepts the current repository state" also passes now at 1.55 s, nowhere near its budget. Its original failure is therefore still unexplained: 1.55 s would have to degrade roughly twentyfold under load to time out, which is a stretch, and nothing in this PR addresses it. It may have been a transient condition on that machine — a stray untracked file under a guarded path would do it, since repositoryEntries() grades untracked files — that has since cleared. Left open rather than assumed fixed.

Verification

  • npx vitest run tests/codex-cloud-setup.test.tsTests 34 passed (34)
  • npm run lint — passed (4371 input files)
  • npm run typecheck — passed (4371 input files)
  • npm run format — clean; tests/codex-cloud-setup.test.ts (unchanged)

Verification not run: npm run test (full suite) — the full offline suite was run green on this content's parent in PR #2252 (696 passed (696) files, 7770 passed | 4 skipped); this diff changes one test's time budget and no assertion or source behaviour, so it cannot alter any other file's result.

UI verification not run: no production UI, routing, styling, or browser behaviour in the diff.

Risk and rollout

  • Risk: Very low. One argument on one it() call plus a comment. No assertion changes, no source changes, no behaviour change — the test does the same work and checks the same things; only the time it is allowed to take moves. The risk of raising a timeout is that a genuine hang takes longer to surface; bounded here by the same 120 s the neighbouring case already uses.
  • Rollback: git revert the single commit.
  • Provider or production effects: None.
  • RAG impact: none

Notes

Four of the five originally reported failures remain open:

Worth running on the Windows workstation when convenient, since the timings are the diagnostic:

npx vitest run tests/http-readiness.test.ts tests/document-viewer-page-virtualization.dom.test.tsx

Generated by Claude Code

…spawns
"writes managed shell policy behaviorally and preserves unrelated Codex config"
failed in a full `npm run test` on a Windows workstation while passing in
isolation there, and passing everywhere on Linux.
It is not a state failure. The test spawns `bash scripts/setup-codex-cloud.sh`
twice, which is cheap on Linux and expensive on Windows, where every spawn goes
through Git Bash. Measured on that workstation running this file alone: 24.96s
against the 30s default in vitest.config.mts — 83% of the budget with nothing
else competing. A full run puts four workers on the machine at once, and it
tips over.
The sibling case immediately below already carries an explicit 120s budget for
exactly this reason (it measures 112.9s on the same machine). This one was
missed. Give it the same budget and record why, so the next person reading it
does not have to re-derive the Windows cost.
No assertion changes: the test does the same work and checks the same things.
Only the time it is allowed to take moves.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JEzkroXdtL9kYo8rW5eZh2
@supabase

supabaseBot commented Aug 21, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project sjrfecxgysukkwxsowpy because there are no changes detected in supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@coderabbitai

coderabbitaiBot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your current included review allowance is based on your included PR review attempts over the past 7 days.

Next review available in:27 minutes

Limit details: You’ve used the included review currently available. Your 88 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab.

How can I continue?

Wait for the limit to reset, then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: aff41ab7-bbf5-453a-a147-b84cf9e71b86

📥 Commits

Reviewing files that changed from the base of the PR and between 20e9aaf and c962da7.

📒 Files selected for processing (1)
  • tests/codex-cloud-setup.test.ts

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

@BigSimmo
BigSimmo marked this pull request as ready for review August 21, 2026 19:48
@BigSimmo
BigSimmo enabled auto-merge (squash) August 21, 2026 19:53
@BigSimmo
BigSimmo merged commit e60ec44 into mainAug 21, 2026
24 checks passed
@BigSimmo
BigSimmo deleted the claude/clever-edison-167y82 branch August 21, 2026 19:58
BigSimmo pushed a commit that referenced this pull request Aug 21, 2026
… was wrong
Four merge-safe inbox requests; none touches the canonical ledger.
Corrects my own earlier record. The request filed in #2252 reasoned that
failures 1 and 2 were deterministic state assertions and therefore real
Windows-only defects rather than load noise. That was wrong for failure 1: the
assertions are fixed, but the runtime is not, because the test spawns Git Bash
twice. It is cancelled and replaced by one record carrying the resolved status
of all five — two fixed (#2255, #2256), two unreproducible with a legibility fix
(#2252), and hosted-migration-role-guard still genuinely unexplained, with the
untracked-file hypothesis and the diagnostic to capture if it recurs.
Updates #CCZ4HB with the second measurement of the review-bot budget: every PR
opened this session was skipped, the countdown shrank across it, and the binding
constraint has moved from the rate limit to the organisation spending cap — a
billing decision no repo gate can substitute for. It also names the interaction
worth writing down: the draft-by-default rule guarantees no bot review until
someone undrafts, and undrafting mid-CI cancels the run and reds the required
aggregate. Draft default, review budget and cancel-in-progress churn are one
problem seen from three sides.
Updates #6GW95D with a wider scope than it records. A stray `npx vitest` from
the home directory enumerated 253 checkouts of this repository across six
distinct roots — .codex, .gemini, .copilot, opencode, Documents/Codex, and a
pnpm store with its own .worktrees. A sweep scoped to .claude/worktrees covers
none of them. The #XCAX01 safety preconditions still apply before any pruning.
Both updates restate the prior text verbatim rather than replacing it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JEzkroXdtL9kYo8rW5eZh2
Sign up for freeto 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

@BigSimmo@claude