fix: complete build isolation - no HOME pollution, no files outside out/#2730
Open
rockygeekz wants to merge 9 commits intoansible:mainfrom
Open
fix: complete build isolation - no HOME pollution, no files outside out/#2730rockygeekz wants to merge 9 commits intoansible:mainfrom
rockygeekz wants to merge 9 commits intoansible:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRepointed runtime/dev scaffolding, marker/PID files, and content-creator template paths into out/ subdirectories; centralized/exported fixture base path; isolated test HOME/TMP under out/ temp dirs; replaced ESM dirname boilerplate with import.meta.dirname; updated tests to use per-run temp dirs and synchronous file I/O. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/ansible-mcp-server/test/tools/ansibleLint.test.ts (1)
39-42: Apply the same teardown guard here as well.This has the same failure-mode risk: if setup aborts early, unconditional
rmSync(testDir, ...)can throw inafterAll.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ansible-mcp-server/test/tools/ansibleLint.test.ts` around lines 39 - 42, The afterAll teardown currently calls rmSync(testDir, { recursive: true, force: true }) unconditionally which can throw if setup aborted; update the afterAll block that references testDir to first check that testDir is defined and exists (e.g., typeof testDir !== 'undefined' && existsSync(testDir)) before calling rmSync so the teardown is guarded and safe to run even when setup failed.packages/ansible-mcp-server/test/tools/ansibleNavigator.test.ts (1)
41-43: Guard teardown to avoid masking setup failures.If
beforeAllfails beforetestDiris assigned,rmSync(testDir, ...)can throw and hide the original failure.Proposed refactor
afterAll(() => { // Clean up temp directory - rmSync(testDir, { recursive: true, force: true }); + if (testDir) { + rmSync(testDir, { recursive: true, force: true }); + } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ansible-mcp-server/test/tools/ansibleNavigator.test.ts` around lines 41 - 43, The afterAll teardown currently calls rmSync(testDir, { recursive: true, force: true }) which can throw if testDir was never set due to a failing beforeAll; update the afterAll in ansibleNavigator.test.ts to guard the cleanup by checking that testDir is defined and exists before calling rmSync (e.g., use a condition with testDir && existsSync(testDir)) or wrap rmSync in a try/catch to avoid masking the original setup failure while still attempting to remove the temp directory; reference the afterAll block and the rmSync call to locate where to add the guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/ansible-language-server/Taskfile.yml`:
- Around line 49-51: The TMP/TMPDIR environment variables in the Taskfile.yml
point to out/als/tmp which may not exist; modify the task that runs "pnpm run
test" to ensure that directory is created beforehand (e.g., run a mkdir -p for
"{{ .TASKFILE_DIR }}/../../out/als/tmp" or similar) so tools using TMP/TMPDIR
won't fail; locate the task that sets env TMP/TMPDIR in Taskfile.yml and add a
pre-step to create the directory before executing the test command.
In `@packages/ansible-mcp-server/test/tools/ansibleNavigator.test.ts`:
- Around line 3-5: The test still references __dirname (which is undefined in
ESM) — replace that usage with the module-scoped testDir variable (initialized
in beforeAll) so the test runs in the temp directory and keeps isolation; locate
the lingering __dirname reference in the ansibleNavigator.test.ts test code and
swap it to use testDir (preserving any path joins) so all file paths are
resolved against the temp testDir created in beforeAll.
In `@Taskfile.yml`:
- Around line 253-255: The unit task sets TMP/TMPDIR to out/unit/tmp but never
creates that directory, causing temp-file ops to fail on clean workspaces;
modify the Taskfile's unit task (the task that runs Vitest) to create the
directory before tests run—e.g., add a step that runs mkdir -p "{{ .TASKFILE_DIR
}}/out/unit/tmp" or use a precondition to ensure out/unit/tmp exists—so that the
environment variables TMP and TMPDIR point to an existing directory when Vitest
executes.
---
Nitpick comments:
In `@packages/ansible-mcp-server/test/tools/ansibleLint.test.ts`:
- Around line 39-42: The afterAll teardown currently calls rmSync(testDir, {
recursive: true, force: true }) unconditionally which can throw if setup
aborted; update the afterAll block that references testDir to first check that
testDir is defined and exists (e.g., typeof testDir !== 'undefined' &&
existsSync(testDir)) before calling rmSync so the teardown is guarded and safe
to run even when setup failed.
In `@packages/ansible-mcp-server/test/tools/ansibleNavigator.test.ts`:
- Around line 41-43: The afterAll teardown currently calls rmSync(testDir, {
recursive: true, force: true }) which can throw if testDir was never set due to
a failing beforeAll; update the afterAll in ansibleNavigator.test.ts to guard
the cleanup by checking that testDir is defined and exists before calling rmSync
(e.g., use a condition with testDir && existsSync(testDir)) or wrap rmSync in a
try/catch to avoid masking the original setup failure while still attempting to
remove the temp directory; reference the afterAll block and the rmSync call to
locate where to add the guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 68b3151b-041a-4481-8e8a-465816a6b443
📒 Files selected for processing (17)
.gitignoreTaskfile.base.ymlTaskfile.ymleslint.config.mjspackages/ansible-language-server/Taskfile.ymlpackages/ansible-language-server/test/globalSetup.tspackages/ansible-language-server/test/helper.tspackages/ansible-language-server/test/utils/getAnsibleMetaData.test.tspackages/ansible-language-server/test/utils/pathUtils.test.tspackages/ansible-mcp-server/test/tools/ansibleLint.test.tspackages/ansible-mcp-server/test/tools/ansibleNavigator.test.tssrc/features/lightspeed/vue/views/webviewMessageHandlers.tssrc/webviewHtml.tstest/unit/globalSetup.tstools/uv.shvite.config.mtsvitest.config.ts
rockygeekz
force-pushed
the
fix/complete-build-isolation
branch
from
1eacb75 to
c21b40a
Compare
rockygeekz
force-pushed
the
fix/complete-build-isolation
branch
from
3afee94 to
12ea6c5
Compare
❌ 25 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
rockygeekz
force-pushed
the
fix/complete-build-isolation
branch
from
4d21170 to
ae7440a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/unit/contentCreator/scaffolding.test.ts (1)
140-146: Optional: centralize repeatedtemplateSourcePathconstruction.The same joined path appears in many test cases; extracting a helper/constant would make test intent clearer and reduce maintenance overhead.
Also applies to: 165-170, 192-197, 218-223, 243-248, 259-264, 285-290, 307-312, 328-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/contentCreator/scaffolding.test.ts` around lines 140 - 146, Several tests repeatedly construct the same templateSourcePath using path.join; extract a single helper (e.g., a function buildTemplateSourcePath(subpath: string) or a constant TEMPLATE_DEVCONTAINER_PATH generator) in the test file and use it instead of duplicating path.join across cases. Replace occurrences like the variable templateSourcePath and direct path.join(...) calls passed into messageHandlers["scaffoldDevcontainerStructure"] (and the similar uses at the other listed ranges) with a call to the new helper to centralize the path construction and reduce duplication.test/unit/webviewHtml.test.ts (1)
63-121: Optional: extract marker setup helper to reduce duplication.
outDircreation + marker write is repeated in many tests; a small helper would reduce noise and future drift.♻️ Suggested refactor
+function writeDevServerMarker(baseDir: string, url = "http://localhost:5173") { + const outDir = path.join(baseDir, "out"); + fs.mkdirSync(outDir, { recursive: true }); + fs.writeFileSync(path.join(outDir, ".vite-dev-server-url"), url); +} ... -const outDir = path.join(tmpDir, "out"); -fs.mkdirSync(outDir, { recursive: true }); -fs.writeFileSync(path.join(outDir, ".vite-dev-server-url"), "http://localhost:5173"); +writeDevServerMarker(tmpDir);Also applies to: 182-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/webviewHtml.test.ts` around lines 63 - 121, The tests duplicate setup for creating outDir and writing the marker file; extract a small helper (e.g., createOutDirWithMarker or writeViteMarker) that accepts the base tmpDir and marker content, creates the out directory, and writes ".vite-dev-server-url" so each spec can call that helper before asserting getDevServerUrl(tmpDir); update all occurrences (including the other block at lines ~182-231) to use this helper and remove the repeated fs.mkdirSync / fs.writeFileSync logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@vitest.config.ts`:
- Around line 50-53: Update the env object in vitest.config.ts to set
USERPROFILE in addition to HOME so Windows tests use the isolated directory;
specifically, in the env literal where HOME is assigned via
path.resolve(__dirname, "out/unit/tmp/home"), add USERPROFILE with the same
path.resolve(...) value (keeping the existing HOME entry) so both HOME and
USERPROFILE point to the test isolation folder.
---
Nitpick comments:
In `@test/unit/contentCreator/scaffolding.test.ts`:
- Around line 140-146: Several tests repeatedly construct the same
templateSourcePath using path.join; extract a single helper (e.g., a function
buildTemplateSourcePath(subpath: string) or a constant
TEMPLATE_DEVCONTAINER_PATH generator) in the test file and use it instead of
duplicating path.join across cases. Replace occurrences like the variable
templateSourcePath and direct path.join(...) calls passed into
messageHandlers["scaffoldDevcontainerStructure"] (and the similar uses at the
other listed ranges) with a call to the new helper to centralize the path
construction and reduce duplication.
In `@test/unit/webviewHtml.test.ts`:
- Around line 63-121: The tests duplicate setup for creating outDir and writing
the marker file; extract a small helper (e.g., createOutDirWithMarker or
writeViteMarker) that accepts the base tmpDir and marker content, creates the
out directory, and writes ".vite-dev-server-url" so each spec can call that
helper before asserting getDevServerUrl(tmpDir); update all occurrences
(including the other block at lines ~182-231) to use this helper and remove the
repeated fs.mkdirSync / fs.writeFileSync logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d8a867e2-60a3-4ba5-b554-c9ed5ed879fb
📒 Files selected for processing (20)
.gitignoreTaskfile.base.ymlTaskfile.ymleslint.config.mjspackages/ansible-language-server/Taskfile.ymlpackages/ansible-language-server/test/fixtures/diagnostics/invalid_yaml.ymlpackages/ansible-language-server/test/globalSetup.tspackages/ansible-language-server/test/helper.tspackages/ansible-language-server/test/utils/getAnsibleMetaData.test.tspackages/ansible-language-server/test/utils/pathUtils.test.tspackages/ansible-mcp-server/test/tools/ansibleLint.test.tspackages/ansible-mcp-server/test/tools/ansibleNavigator.test.tssrc/features/lightspeed/vue/views/webviewMessageHandlers.tssrc/webviewHtml.tstest/unit/contentCreator/scaffolding.test.tstest/unit/globalSetup.tstest/unit/webviewHtml.test.tstools/uv.shvite.config.mtsvitest.config.ts
✅ Files skipped from review due to trivial changes (7)
- packages/ansible-language-server/test/utils/pathUtils.test.ts
- packages/ansible-language-server/test/fixtures/diagnostics/invalid_yaml.yml
- tools/uv.sh
- packages/ansible-language-server/test/globalSetup.ts
- Taskfile.base.yml
- .gitignore
- src/features/lightspeed/vue/views/webviewMessageHandlers.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- eslint.config.mjs
- packages/ansible-language-server/test/helper.ts
- test/unit/globalSetup.ts
- Taskfile.yml
- packages/ansible-mcp-server/test/tools/ansibleLint.test.ts
- packages/ansible-language-server/Taskfile.yml
rockygeekz
force-pushed
the
fix/complete-build-isolation
branch
from
4454976 to
3dbc886
Compare
rockygeekz
force-pushed
the
fix/complete-build-isolation
branch
from
1aadbc2 to
566c42f
Compare
rockygeekz
force-pushed
the
fix/complete-build-isolation
branch
from
1acdf96 to
d286d9c
Compare
rockygeekz
force-pushed
the
fix/complete-build-isolation
branch
from
d286d9c to
356f5ce
Compare
goneri
requested changes
Apr 13, 2026
| console.info( | ||
| "Cleaning up any leftover podman containers from previous runs...", | ||
| ); | ||
| spawnSync("podman", ["system", "reset", "--force"], { stdio: "inherit" }); |
Collaborator
There was a problem hiding this comment.
I like the PR, but this is change in particular a bit like bringing an atomic nuke in a bar brawl :-). Are you 100% sure this only clean the testHome directory?
What if the user has different configuration for CONTAINER_HOST, XDG_CONFIG_HOME, XDG_DATA_HOME, XDG_RUNTIME_DIR, etc?
Overall, I'm not sure WHY this had to be done?
Contributor
Author
There was a problem hiding this comment.
Yeah you are right. This is aggressive and I was trying to clean up leftover containers from failed test runs and I have removed those aggressive changes if CI fails I'll handle it by cleanup or something.
| try { | ||
| if (!SKIP_PODMAN) { | ||
| console.info("Cleaning up podman containers and storage..."); | ||
| spawnSync("podman", ["system", "reset", "--force"], { stdio: "inherit" }); |
| ln -fs "$creator_resources_path/common/devcontainer/.devcontainer" "out/resources/contentCreator/createDevcontainer/" | ||
|
|
||
| log notice "Using $(python3 --version) from $(uv run which python3)" | ||
| if [[ "$(which python3)" != ${VIRTUAL_ENV}/bin/python3 ]]; then |
Collaborator
There was a problem hiding this comment.
$VIRTUAL_ENV also points outside of the source directory:
./.config/mise.toml:VIRTUAL_ENV = "{{ xdg_cache_home }}/.local/share/virtualenvs/vsa"
Contributor
Author
There was a problem hiding this comment.
Yeah, VIRTUAL_ENV does point outside the repo, but that's intentional. It's managed by mise (not by our build system), and moving it inside out/ would break mise's venv management and force reinstalling all Python deps on every clean. The goal of this PR is to prevent build artifacts and temp test files from polluting HOME/repo root - not to isolate the Python environment itself. The venv is already properly isolated per-project by mise/uv.
goneri
reviewed
Apr 13, 2026
.github/workflows/ci.yaml
Outdated
| find out -name '*:*' -exec rm -r {} \; || true | ||
| rm -rf dist | ||
| # Remove container overlay files to avoid gitleaks false positives | ||
| sudo rm -rf out/als/tmp/home/.local/share/containers/ || true |
Collaborator
There was a problem hiding this comment.
Does this prevent the CI from caching the images? Maybe we can set-up some symlink instead to continue to safe the images are at place that will be restored across builds?
Contributor
Author
There was a problem hiding this comment.
and yeah about caching. However, we can't use symlinks here because:
- It would defeat the isolation purpose - we'd be writing to user's actual container storage
- Container images are already cached at system level by podman/docker (in
/var/lib/containers/or~/.local/share/containers/on the runner), not in the workspace - We're only removing the overlay files to avoid gitleaks false positives (they contain strings that look like API keys) The actual image layers are still cached at system level, we're just cleaning up the overlay storage used during test runs.
…ks false positives
…scanning stale containers
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
2 participants
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
Prevents builds and tests from creating files outside
out/or pollutingthe user HOME directory, completing the remaining work for AAP-65204.
Changes
HOME isolation for unit test suites
process.env.HOMEto isolated directories underout/in globalSetupfor ALS and extension unit tests
TMP/TMPDIRenv vars to unit, ALS, and e2e tasks in TaskfileRelocated build artifacts to
out/.vite-dev-server-url,.vite-dev.pid)resources/contentCreator/symlinks created byuv.shCleanup
__dirnamederivations in favor ofimport.meta.dirnameandshared
FIXTURES_BASE_PATHexport.gitignoreentries for files no longer created at rootexclude:entries from Taskfile sourcesrelated: AAP-65204
depends-on: #2726
Prevents builds/tests from polluting the user HOME or writing outside out/ by isolating HOME/TMP, relocating build artifacts into out/, and updating tests and tooling to use isolated or system temp directories.
related: AAP-65204