Skip to content

test: stabilize command-population admission order - #36111

Closed
brennanb2025 wants to merge 1 commit into
mainfrom
brennanb2025/main-ci-fix-1029
Closed

brennanb2025 wants to merge 1 commit into
mainfrom
brennanb2025/main-ci-fix-1029

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

The command-population test can fail with three probe starts while expecting four. It assumes the first two legacy subscriptions hold the two permits, but asynchronous filesystem preparation can admit context 2 first. The explicit request for context 2 then correctly shares admitted work instead of starting another probe.

This PR waits for the first two legacy probes before adding later subscriptions. That establishes which contexts hold the permits and preserves every existing population, preparation, permit, frame, retirement and replacement assertion. Shipped behavior is unchanged. The saved failed CI log matches the count mismatch but lacks the assertion location and iteration, so exact attribution of that historical failure remains unproven.

Plan: establish the fixture's admission precondition, retain all assertions, review the production admission/sharing mechanism, and validate the changed test in CI.

Light-path classification:

  • Small: four added test lines in one file; zero production files or lines.
  • Outside risky areas: only fixture setup scheduling changes; no shipped ownership, lifecycle, persistence, concurrency, wire, process, auth, build, packaging or default changes.
  • Not a repeat: the touched file's last-30-day history contains its introduction in Prevent resting chats from delaying native command menus #35069 and this repair; no intervening same-symptom fix or revert.
  • Local effect: test-only admission setup, with no user-visible change.

Validation: git diff --check passed. CI run 40960795741 passed on a11cb27c480bd5c838e9eb329fb72a11a7a433c8, including static analysis/typecheck, all ten root unit shards, relay integration, cross-version wire compatibility and the aggregate PR gate. Shard 9/10 confirms this test file passed all seven tests in 3121ms. One fresh adversarial light-path source review found no issues. No dependencies were installed or local tests/apps run; QA is skipped under the tests-only row.

Other reported main failures are already repaired on the base: sidebar/adoption fixes in #36080 and #36096, and duplicate browser type/performance-test line-cap fixes in #36101. This PR contains no duplicate repairs for those.

Wait for the first two legacy probes before subscribing queued contexts so asynchronous filesystem preparation cannot change which request promotes queued work.

Co-Authored-By: Codex <noreply@openai.com>
@greptile-apps

greptile-apps Bot commented Oct 29, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Low impact] The PR appears safe to merge.

Summary

Makes the command-population test deterministic by waiting for the first two legacy probes before adding later subscriptions.

  • The population test fixes which legacy probes start first.

Reviews (1) · Last reviewed commit: "test: pin legacy command probe admission..." · Reviewed by Greptile

@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Walkthrough

The mixed real-host admission test now waits for the expected probe-start count after each of the first two legacy subscriptions. It adds no such wait for later subscriptions.

Priority: Low

Merge Risk: Minimal · up to a11cb

No actionable merge-blocking risk was identified in this test-only change.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Description check Warning The description explains the failure, the test change, scope, and validation, but it does not follow the required template. It omits the required section headings, an actual linked issue, visual-proof… Reformat the description using all required template sections. Add the actual issue reference after “Fixes #”, state N/A with a reason under Visual Proof, complete Testing and AI Disclosure as applicable, address Review, Agent skill upstr…
Passed Passed checks: 4 passed
Check name Status Explanation
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
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.
Title check Passed The title clearly and concisely describes the test-only change that stabilizes command-population admission order.
Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 1 files.

Full details: Description check

Explanation

The description explains the failure, the test change, scope, and validation, but it does not follow the required template. It omits the required section headings, an actual linked issue, visual-proof status, AI disclosure, review notes, and checklist confirmations.

Resolution

Reformat the description using all required template sections. Add the actual issue reference after “Fixes #”, state N/A with a reason under Visual Proof, complete Testing and AI Disclosure as applicable, address Review, Agent skill upstream boundary, Notes, and Checklist items, and preserve the existing technical explanation and CI evidence.

Finishing Touches
📝 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 brennanb2025 changed the title fix(ci): repair main's line-cap, adoption fixture and at-rest count breakages test: stabilize command-population admission order Oct 29, 2026
@brennanb2025

Copy link
Copy Markdown
Contributor Author

The command-population test assumed that subscription order determined which two legacy requests held permits. Filesystem preparation can reorder admission, so an overlap request may correctly share an already-running probe while the test expects another start. This PR waits for the first two probes before adding later subscriptions. All existing count, permit, preparation, retirement and replacement assertions remain intact; shipped behavior is unchanged.

The light-path review finished in one fresh adversarial source-review round with no findings, fixes or deferrals. The reviewer traced the preparation/admission race and checked the original history. The saved failed log matches the three-versus-four mismatch but does not identify its assertion or iteration, so exact attribution of that historical failure remains unproven.

Validation: CI run 40960795741 passed on a11cb27c480bd5c838e9eb329fb72a11a7a433c8: static analysis/typecheck, all ten unit shards, relay integration, cross-version compatibility and the aggregate PR gate. Shard 9/10 confirms the changed test file passed all seven tests in 3121ms. git diff --check passed. Application QA is skipped under the tests-only row. No local dependency installation, lint, typecheck, tests or app launch was performed.

For Brennan's review: the fixture barrier and retained promotion assertions. No screenshots are required for this test-only change.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Closing: main already removed the flaky at-rest admission test in #36134, so this fix is no longer needed.

@brennanb2025
brennanb2025 deleted the brennanb2025/main-ci-fix-1029 branch October 29, 2026 22:53
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