🤖 tests: reset the process-global workflow archive guard in WorkflowService context tests - #4530
Merged
Merged
Conversation
…vice context tests
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This was referenced Sep 25, 2026
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Sep 25, 2026
…sts (coder#4536) ## Summary `evaluateScreening.test.ts` resets the process-global workflow archive admission guard before each test, the same one-line fix coder#4530 applied to `WorkflowService.context.test.ts`. ## Background Every `WorkspaceService` constructor installs a process-global archive admission guard bound to its own config. WorkspaceService suites built on partial config doubles (`workspaceService.aiSettings`, `workspaceService.bashAndFiles`, `terminalService`) leave that guard behind; when CI's size-balanced shard split puts one of them before this file, all 7 screening tests fail with `this.config.loadConfigOrDefault is not a function`. This hit the WS7 stack's merge group and PR CI (Test / Unit 2/6) after an unrelated reshuffle. ## Validation - `bun test src/node/services/workspaceService.aiSettings.test.ts src/node/services/workflows/evaluateScreening.test.ts`: 7 fail before, 38 pass after. Refs the test-audit campaign (WS7 landing blocker; the root cause, partial WorkspaceService doubles installing a real global guard, belongs to the WorkspaceService harness work). --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$13.67`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=13.67 -->
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Sep 25, 2026
…ions instead of a process-global guard (coder#4562) ## Summary Removes the process-wide workflow archive guard. Workflow start/resume/retry admissions now ask the owning `WorkspaceService` directly (`getWorkflowArchiveRefusal`), passed in through `WorkflowServiceContext` and a required `WorkflowService` constructor option. With nothing registered at module scope, a `WorkspaceService` built in one test file can no longer decide admissions in another. The three per-file guard resets, including coder#4530's, are deleted. ## Background Every `WorkspaceService` constructor called `setWorkflowArchiveAdmissionGuard(...)` with a closure over its own config and never cleared it. After coder#4475 split the WorkspaceService tests, a split file sharing a unit shard with `WorkflowService.context.test.ts` left a guard over a partial config double behind, and 9 context tests failed. coder#4530 reset the guard in that one file; `evaluateScreening.test.ts` and `WorkflowService.test.ts` carried the same reset. The leak itself stayed open. A compare-and-clear release on dispose would not have closed it: about 170 test fixtures build a `WorkspaceService` without disposing it, so the last guard would still outlive its file. ## Implementation - `workflowArchiveAdmission.ts`: - `admissionGuard` and `setWorkflowArchiveAdmissionGuard` are gone. - `acquireWorkflowArchiveAdmission(guard, workspaceId)` takes a `WorkflowArchiveAdmissionGuard`. - The in-process admission/runner counters stay process-wide: WorkflowService instances are per-request, and the archive sink must see admissions from any of them. - `WorkspaceService`: the constructor closure became the public `getWorkflowArchiveRefusal(workspaceId)`, with the same body. The class declares `implements WorkflowArchiveAdmissionGuard`. - `WorkflowService`: new required `archiveAdmission` option, so tsgo finds every construction site. It is wired at all three production sites: - `resolveWorkflowContext` passes `context.workspaceService`. - `cli/workflow.ts` passes the CLI's own `services.workspaceService`, which is the instance whose guard was installed before. - Tool-started runs in `TurnRequestBuilder` get a new `workflowArchiveAdmission` binding. It is set in `di/layers/core.ts` next to the other WorkspaceService bindings, and the builder asserts it when `taskService` is bound. - The top-level `startWorkflowRun`/`resumeWorkflowRun`/`retryWorkflowRunFromCheckpoint` pass `context.workspaceService`. Tests: - The refusing-guard test in `WorkflowService.test.ts` injects its gate instead of setting and resetting a global. - New owner test: `WorkspaceService` refuses archived and archiving workspaces, and admits active, unarchived and unknown ones. The logic previously had no owner-side coverage. - New wiring test: `startWorkflowRun` refuses through the context's WorkspaceService before creating a run. Replacing both the top-level and the class-level gate with an admit-all stub makes it fail. ## Validation - The coder#4530 reproduction order passes with the resets removed: 131 pass. The command was `bun test workspaceService.archive workspaceService.bashAndFiles workflows/WorkflowService.context workflows/evaluateScreening workflows/WorkflowService`. - `bun test src/node/services/workflows`: 347 pass. `bun test src/node/services/{workspaceService,taskService,turnRequestBuilder}* src/node/services/serviceContainer.test.ts`: 1701 pass. `src/node/services/{agentSession*,tools}` and `src/node/orpc` also pass. - `src/cli` passes after `make build-main`. One run under host load 66 timed out "CLI run closes the AppFiberScope first…" at 5 s; `workflow.test.ts` then passed 3 of 3 runs on its own. - `make static-check` passes. ## Risks Low. Admission semantics are unchanged in production, which has one WorkspaceService per process; the refusal is now looked up explicitly instead of through a module variable. A future `WorkflowService` construction must supply the gate, and the type system enforces that. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high -->
yermakoffivan
pushed a commit
to yermakoffivan/mux
that referenced
this pull request
Sep 26, 2026
…aluation (coder#4642) ## Summary `GeneralSection` computed `isBrowserMode` (no `window.api`) once, when its module was evaluated. It now reads it where it is used. Same result in the app (`window.api` never changes at runtime), but the answer no longer depends on which environment first imported the module. ## Background In one bun process, `SettingsPage.test.tsx` imports `GeneralSection` before a DOM exists, which caches `isBrowserMode = false`. A later `GeneralSection.test.tsx` then fails "loads the SSH host setting in browser mode". Reproduced on current `main`: ``` bun test ./src/browser/features/Settings/SettingsPage.test.tsx ./src/browser/features/Settings/Sections/GeneralSection.test.tsx ``` CI passes today only because shard balancing keeps the two files in different unit processes. The WS2 test-audit stack (coder#4632–coder#4634) changes other test files, which reshuffles shard 4 so both land in one process, and `Test / Unit (4/6)` fails there. This is the same latent-order class as coder#4530. ## Validation The pair above goes from 1 fail to 47 pass. Typecheck and the React Compiler coverage check pass. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high` • Cost: `$46.10`_ <!-- mux-attribution: model=anthropic:claude-opus-5-5 thinking=high costs=46.10 -->
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
Makes
WorkflowService.context.test.tsindependent of test order by resetting the process-global workflow archive admission guard before each test. Test-only.Background
WorkspaceService's constructor installs a module-level guard withsetWorkflowArchiveAdmissionGuardthat callsthis.config.loadConfigOrDefault(). Since #4475 splitworkspaceService.test.ts, the split files can land in the same bun unit shard asWorkflowService.context.test.ts. When one runs first with a partial config double, the leftover guard throwsthis.config.loadConfigOrDefault is not a functioninside every workflow start/resume, and 9 context tests fail.Reproduced on current
main(7c133d0) with one process:Main's own CI passes only because shard balancing currently keeps those files apart. The WS2 test-audit stack (#4483–#4487) removes test files, which reshuffles the shards, and its merge groups failed
Test / Unit (4/6)on exactly these tests.Implementation
beforeEachresets the guard to admit everything, the same resetWorkflowService.test.tsalready uses. The underlying leak (everyWorkspaceServiceinstance overwrites a process-global guard and never unregisters it) belongs to the WorkspaceService owner and is reported to that workstream rather than redesigned here.Validation
The reproduction command above: 9 failures before, 102 pass after.
bun test src/node/services/workflows/passes.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$4.45