test(plan-mode): serialize tests that touch the process-wide plan pointers - #528
Merged
Conversation
plan_pointer_roundtrip_and_pid_fallback failed intermittently under parallel execution (~1 run in 6), which surfaces as unrelated CI failures on whatever PR happens to hit it. The cause is not the test alone: clear_active_plan_path deliberately removes the pid-scoped and legacy pointers alongside the session one, so every ExitPlanMode test wipes state the roundtrip test is midway through asserting on. Session-scoped paths are unique per test and were never the problem. Both sides now take a shared-pointer lock — the roundtrip test and the seven tests that drive ExitPlanMode. The guard ignores mutex poisoning so a panic in one test cannot cascade into unrelated failures. Verified with 12 consecutive runs of the plan_mode suite: zero failures, against a baseline that failed roughly one run in six.
emal-avala
force-pushed
the
fix/plan-pointer-test-race
branch
from
July 27, 2026 06:52
40d8734 to
44bea50
Compare
Member
Author
|
@codex review |
emal-avala
added a commit
that referenced
this pull request
Jul 27, 2026
The catalog now lists a user <config>/themes/auto.toml as the sole 'auto' row, but the runtime facade intercepted the id before any palette lookup: from_name and resolve_theme both converted 'auto' to one-dark or solarized-light, so selecting the advertised row silently applied a built-in the user never chose. Both sites now defer when a user palette owns the id, matching how dark/light already shadow. The rule is a pure auto_detection_applies so it is testable without mutating the process environment — writing a themes dir would have needed XDG_CONFIG_HOME, and process-global env in tests is exactly the pattern behind the flake fixed in #528.
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This was referenced Jul 27, 2026
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.
Summary
plan_mode::tests::plan_pointer_roundtrip_and_pid_fallbackfails intermittently under parallel execution (~1 run in 6). It surfaces as a red CI check on whatever PR is unlucky, costing review cycles chasing a failure that has nothing to do with the change under review — it was hit on fix(bash): unquote tokens before the destructive-command scan #506 today.clear_active_plan_pathdeliberately removes the pid-scoped and legacy pointers alongside the session one, so every ExitPlanMode test wipes shared state the roundtrip test is midway through asserting on. Session-scoped paths are unique per test and were never the problem.ExitPlanMode. The guard ignores mutex poisoning so a panic in one test can't cascade into unrelated failures.Test plan
plan_modesuite: zero failures, against a baseline that failed roughly one run in sixcargo test --all-targetsgreen apart from the 3 known environmentalbwrap_*failurescargo clippy --all-targets -- -D warnings,cargo fmt --all -- --checkNote: a second known flake exists independently —
services::git::tests::test_status_and_diffraces on process-globalPATHset by MCP binding tests viaEnvGuard. That one needs the tests driven through theenvmap instead; not addressed here.