Repository navigation
ci: run the renderer-memory gate and unblock the test registry - #13747
Merged
teamleaderleo merged 1 commit intoSep 22, 2026
Merged
Conversation
Two preflight defects: The focused renderer-memory step exports a plain CMUX_RENDERER_MEMORY_REGRESSION=1, but run-app-host-xcodebuild.sh only forwards named TEST_RUNNER_* variables into the test host, and Xcode does not inherit the driver environment. The guarded test has therefore been skipping every run instead of asserting the one-versus-five renderer footprint. Forward opt-in gate variables through the TEST_RUNNER_ channel. tests/test_sync_test_wiring.py is registered twice in tests/test-execution.toml, so validate_test_execution_registry.py fails on main and in every pull request preflight. Drop the duplicate entry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
All contributors have signed the CLA ✍️ ✅ |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe app host build script forwards ChangesRenderer Memory Gate
Test Registry Cleanup
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The CI gate reaches the test host correctly, and the duplicate registry entry was removed without losing coverage. 🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Collaborator
Author
|
Triage note — this is the most under-noticed PR in the open queue and it should land first out of the registry cluster. Verified against
Conflict you need to know about: #13745 deletes the same four lines. Its Note that #13745 would not have prevented today's failure on its own. It downgrades unregistered tests inherited from the base branch to warnings, but explicitly keeps "malformed or duplicated entries" as hard failures. A duplicate entry on The renderer-memory half is independent of all of that — Also closed today: #13712, which was opened to fix an earlier instance of exactly this "registry failure reddens every PR" problem. Its nine registrations are all on |
teamleaderleo
added a commit
to teamleaderleo/cmux
that referenced
this pull request
Sep 22, 2026
`run-in-console-session.sh` re-enters the console user's Aqua session through `sudo -n launchctl asuser ... sudo -n -u <user> -E env ...`. The outer sudo has no `-E`, so the environment is reset there and rebuilt from an explicit `forward=(...)` allowlist. `CMUX_RENDERER_MEMORY_REGRESSION=1` was never added to that allowlist, so it did not survive the hop. The opt-in forwarding added to `run-app-host-xcodebuild.sh` in manaflow-ai#13747 therefore saw an unset variable, appended no `TEST_RUNNER_` pair, and the regression kept skipping itself at TerminalAndGhosttyTests.swift:4187 — on every run, while CI reported success. Add the variable to the allowlist, and add a guard so the next opt-in gate does not fail the same silent way: the guard walks every backslash-continued assignment prefixing a `run-in-console-session.sh` call across all workflows and fails if any name is missing from the allowlist. It fails on the unfixed tree naming exactly this variable, and passes over all 34 call sites with the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
teamleaderleo
added a commit
to teamleaderleo/cmux
that referenced
this pull request
Sep 22, 2026
`run-in-console-session.sh` re-enters the console user's Aqua session through `sudo -n launchctl asuser ... sudo -n -u <user> -E env ...`. The outer sudo has no `-E`, so the environment is reset there and rebuilt from an explicit `forward=(...)` allowlist. `CMUX_RENDERER_MEMORY_REGRESSION=1` was never added to that allowlist, so it did not survive the hop. The opt-in forwarding added to `run-app-host-xcodebuild.sh` in manaflow-ai#13747 therefore saw an unset variable, appended no `TEST_RUNNER_` pair, and the regression kept skipping itself at TerminalAndGhosttyTests.swift:4187 — on every run, while CI reported success. Add the variable to the allowlist, and add a guard so the next opt-in gate does not fail the same silent way: the guard walks every backslash-continued assignment prefixing a `run-in-console-session.sh` call across all workflows and fails if any name is missing from the allowlist. It fails on the unfixed tree naming exactly this variable, and passes over all 34 call sites with the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
teamleaderleo
added a commit
that referenced
this pull request
Sep 22, 2026
…13776) * ci: forward the renderer-memory gate across the console-session hop `run-in-console-session.sh` re-enters the console user's Aqua session through `sudo -n launchctl asuser ... sudo -n -u <user> -E env ...`. The outer sudo has no `-E`, so the environment is reset there and rebuilt from an explicit `forward=(...)` allowlist. `CMUX_RENDERER_MEMORY_REGRESSION=1` was never added to that allowlist, so it did not survive the hop. The opt-in forwarding added to `run-app-host-xcodebuild.sh` in #13747 therefore saw an unset variable, appended no `TEST_RUNNER_` pair, and the regression kept skipping itself at TerminalAndGhosttyTests.swift:4187 — on every run, while CI reported success. Add the variable to the allowlist, and add a guard so the next opt-in gate does not fail the same silent way: the guard walks every backslash-continued assignment prefixing a `run-in-console-session.sh` call across all workflows and fails if any name is missing from the allowlist. It fails on the unfixed tree naming exactly this variable, and passes over all 34 call sites with the fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * ci: harden the console-session forwarding guard against its blind spots Review of the first version found the regex walk only inspected backslash- continued lines strictly above the wrapper, so a single-line invocation (FOO=1 scripts/ci/run-in-console-session.sh ...) was invisible -- the exact shape the guard exists to catch. It also globbed .github/workflows/*.yml non-recursively, missing .yaml and composite actions, and harvested names out of comments inside forward=(), which would count a commented-out entry as forwarded. Parse the workflows as YAML and walk each step's run: block instead. Scan the invocation line itself as well as its continuations, recurse over .github for both extensions, and strip comments before reading the allowlist. Failures now name the job and step rather than a line number. Scope stayed at command prefixes deliberately. Extending it to ambient environment -- step/job/workflow env: and plain export -- was tried and produced 60+ false positives on the real workflows (CI cache URLs, Xcode selection, shard indices), because the allowlist is intentionally selective. A prefix is the explicit statement that a variable is meant for this command, and it is the existing convention at every call site that needs one. The docstring records this so the next reader does not re-litigate it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
austinywang
added a commit
that referenced
this pull request
Sep 23, 2026
…on-screen window The renderer-memory regression started asserting once #13747 and #13776 carried CMUX_RENDERER_MEMORY_REGRESSION into the app host (the second across the console-session hop); before that it skipped itself on every run. It now fails in app-host shard 6 (run 35800041885): none of the five renderers is ever presented, so every release and restoration assertion after that fails too. The fixture predates the portal rendering authority (a81d39e, #12414). setVisibleInUI folds its request through Workspace.portalRenderingEnabled(for:), which denies a tab id that no registered manager has selected, and the test built its surfaces with UUID(). Build them in a registered, selected workspace through a shared AppDelegate test seam, the same registration the direct-interaction suites use on fix/app-host-green. The window was also ordered in after the terminals attached. A terminal samples its window's visibility on attach and afterwards only on an occlusion, key, or screen change. This borderless window never becomes key and the headless host never reports occlusion .visible (the case a1f0cf9 handles), so the renderers could keep seeing a hidden window. Order it in first. The memory thresholds and assertions are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
teamleaderleo
pushed a commit
that referenced
this pull request
Sep 23, 2026
* test: give the five-tab renderer gate a live portal workspace and an on-screen window The renderer-memory regression started asserting once #13747 and #13776 carried CMUX_RENDERER_MEMORY_REGRESSION into the app host (the second across the console-session hop); before that it skipped itself on every run. It now fails in app-host shard 6 (run 35800041885): none of the five renderers is ever presented, so every release and restoration assertion after that fails too. The fixture predates the portal rendering authority (a81d39e, #12414). setVisibleInUI folds its request through Workspace.portalRenderingEnabled(for:), which denies a tab id that no registered manager has selected, and the test built its surfaces with UUID(). Build them in a registered, selected workspace through a shared AppDelegate test seam, the same registration the direct-interaction suites use on fix/app-host-green. The window was also ordered in after the terminals attached. A terminal samples its window's visibility on attach and afterwards only on an occlusion, key, or screen change. This borderless window never becomes key and the headless host never reports occlusion .visible (the case a1f0cf9 handles), so the renderers could keep seeing a hidden window. Order it in first. The memory thresholds and assertions are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> * test: give the tmux mirror sizing fixtures manual-mirror panes and a settled start offPlanGeometryWithUnchangedSizingInputsReconverges fails on main and on fix/app-host-green. Its diagnostic on run 35800041885 (shard 3) read rearmsAtPerturbation=3 rearmsDuringRecovery=0: the output-parity judge had spent its whole re-arm budget before the test perturbed anything. The fixture built process panels (.pacedSessionRestore). Once those are authorized and on screen, a process surface's grid follows its view, because the assigned-grid pin only holds manual-IO surfaces at tmux's 61x35 assignment. The judge's grid-parity half never cleared, so the baseline spent all three re-arms and left none for the recovery the test measures. The fixture now builds its panes with Workspace.makeRemoteTmuxPanePanel, the manual-mirror factory production mirrors use. It waits for grid parity as well as frame parity before capturing its fixed point, and it requires an unspent re-arm budget at the perturbation, so a starved baseline fails as a precondition instead of reading like the liveness hole the test pins. swallowedDividerResizeIsProvenNoOpByTheBarrierAndHeals failed once on the same shard at "no recovery may fire before the barrier's verdict" and passed in other runs. Its convergence pump checked frames only. Frames can match the plan while the render-frame restate, or any other ignoring-inputs trigger, has cleared the completed sizing inputs and queued a pass. The likely failure: the pump resumed in that gap, and the synchronous test code after it never yields, so the test read the cleared inputs and blamed the swallowed resize. The drag fixtures now also wait for no scheduled pass and a completed sizing transaction, and the swallowed, errored, and superseded divider tests require that state before the drag starts. The divider-drag test's post-reply pump waits for the same state before its late ack arrives. No assertion changed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two preflight defects found while profiling app-host CI.
The renderer-memory regression test never runs
.github/workflows/ci-macos.ymlruns its focused step with a plainCMUX_RENDERER_MEMORY_REGRESSION=1, butscripts/ci/run-app-host-xcodebuild.shonly forwards namedTEST_RUNNER_*variables into the test host, and (as that script's own comment says) Xcode does not inherit the driver environment. So the guard atcmuxTests/TerminalAndGhosttyTests.swift:4248has been skipping every run, and the one-versus-five renderer footprint has not been asserted in CI.Fix: forward opt-in gate variables through the
TEST_RUNNER_channel. The workflow step is unchanged, so the same trap won't catch the next gated suite added the same way.The test-execution registry fails on main
tests/test_sync_test_wiring.pyis registered twice intests/test-execution.toml, soscripts/ci/validate_test_execution_registry.pyexits 1 on main and in every PR preflight that runs it. Dropping the duplicate makes it pass: 223 tests (legacy=61, linux-guard=101, macos-cli-no-socket=48, macos-cli-no-socket-post-fish=11, macos-shell=1, macos-shell-fish=1).Testing
bash -n scripts/ci/run-app-host-xcodebuild.shpython3 scripts/ci/validate_test_execution_registry.py(fails on main, passes here)python3 tests/test_ci_linux_guard_routing.py🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes two CI preflight defects: the renderer-memory regression test never ran, and the test-execution registry validation failed on main.
CMUX_RENDERER_MEMORY_REGRESSIONthrough theTEST_RUNNER_channel so Xcode test hosts pick it up; previously the guard skipped every run because Xcode doesn't inherit the driver environment.tests/test_sync_test_wiring.pyentry fromtests/test-execution.tomlthat madescripts/ci/validate_test_execution_registry.pyexit 1 on main and every preflight.Written for commit 753ef17. Summary will update on new commits.
Summary by CodeRabbit