fix(cli): scope recover.test.ts module mocks so they don't leak to step-runner.test.ts#32336
Merged
Merged
Conversation
…ep-runner.test.ts
dvargasfuertes
approved these changes
May 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
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.
Prompt / plan
Fix the only CI failure on
mainafter #32071 merged:(fail) exec — secret leak regression > rejects with an Error whose message contains neither the args nor any -e KEY=VALUE pair.Root cause
recover.test.tscallsmock.module("../lib/step-runner.js", () => ({ exec: execMock }))at the top of the file, outside any lifecycle hook. Bun 1.3.11'smock.moduleregistration persists across test files in the samebun testinvocation (open Bun issue: oven-sh/bun#12823). When the CI test runner reachesstep-runner.test.ts, theexecsymbol it imports is still pointing atexecMock(anasync () => {}no-op), not the realspawn-based implementation. The no-op resolves withundefined, so theawait exec(...)in the test never rejects, thethrow new Error("exec should have rejected")fires, and the assertionexpect(message).toContain("sh exited with code 125")fails withReceived: "exec should have rejected".Locally the test passed because file iteration order on my machine put
step-runner.test.tsfirst, before the mock registered. On GHA Ubuntu runners the order was reversed (deterministically — confirmed across 2 CI runs of the same commit).mock.restore()does not restore module mocks (Bun docs), so the fix is to invert the lifecycle:../lib/local.jsand../lib/step-runner.jsat module load timebeforeAllmock.modulewith the captured real exports inafterAllThis keeps
recover.test.ts's behavior unchanged (all 8 of its tests still pass), and restores the un-mockedexecfor any test file that runs after it.Test plan
bun test src/lib/__tests__/step-runner.test.ts src/__tests__/recover.test.ts— both orderings: all 10 step-runner tests passbun test src/__tests__/recover.test.ts src/lib/__tests__/step-runner.test.ts— reverse order: all passbun run testfromcli/: 486/487 pass. The one remaining fail (recover homedir() ... already exists) is a pre-existing local-only collision on my sandbox (/data/.vellumexists from previous work) — also fails onorigin/mainwith the same env, and doesn't reproduce on clean CI runnersbun run lint,bun run lint:unused,bun run typecheck,bun build --compile: all green (1 pre-existing eslint-disable warning inroadmap.ts, unchanged from main)AGENTS.md compliance
software-engineering/references/comments.mdrule ci: add web and platform CI/PR workflows #4: rationale (Bun bug ref, why beforeAll/afterAll) lives in this PR body, not in inline code comments. The inline comment describes current-state behavior only.software-engineering/references/unit-testing.mdrule docs: add comprehensive contributing guide #13: order-independence — this PR is exactly that lesson.