Fixed test failures

[lsunay]

Description

Fixed two test failures in the skills-discovery.test.ts file:

Added null check in afterAll cleanup: Prevents TypeError when tmpDir is undefined if beforeAll fails
Create .tmp directory before mkdtemp: Prevents ENOENT error by ensuring the base temp directory exists before creating temp folders

These changes ensure the test suite passes reliably in CI environments.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Documentation update
• Release preparation

Checklist

• I have updated the CHANGELOG.md
• I have added tests that prove my fix/feature works
• All new and existing tests pass (bun test)
• I have updated documentation if needed

Related Issues

Additional Notes

This is a minimal fix to ensure test reliability in CI environments. The changes are defensive programming practices to prevent test failures when:

beforeAll fails to initialize tmpDir
The .tmp base directory doesn't exist

No new tests were needed as we're fixing existing test infrastructure rather than adding new functionality.

Summary by CodeRabbit

Release Notes

• Tests
  • Improved test isolation and reliability through enhanced setup, teardown, and filesystem handling procedures.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Test infrastructure improvements strengthen isolation and safety by creating a dedicated temporary directory structure, implementing conditional cleanup logic, and converting filesystem-dependent tests to explicit async operations.

Changes

Cohort / File(s)	Summary
Test Setup & Teardown Refactoring packages/orchestrator/test/unit/skills-discovery.test.ts	Reworked beforeAll/afterAll blocks with dedicated ".tmp" directory creation, nested directory initialization, and guarded cleanup that removes temp directory only if it was created; converted filesystem-dependent tests to async operations

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 With burrows nested deep and true,
Our tests now clean up after brew,
Each temp directory plays its part,
Isolation—a testing work of art! 🌱

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Fixed test failures' is vague and generic, lacking specific details about which test failures or what the actual fix entails.	Make the title more specific by describing the actual fix, e.g., 'Fix test failures in skills-discovery by ensuring .tmp directory exists' or 'Add null checks to prevent test cleanup errors'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description check	✅ Passed	The pull request description covers most required sections including a clear description of changes, type of change marked, and relevant checklist items completed; however, the CHANGELOG.md was not updated as indicated in the template.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

packages/orchestrator/test/unit/skills-discovery.test.ts (1)

17-18: Consider using os.tmpdir() for better portability.

The fix correctly creates the base directory before mkdtemp, addressing the ENOENT error. However, creating .tmp in the project root may clutter the workspace. Consider using Node's os.tmpdir() instead of process.cwd() for a more portable solution that follows OS conventions.

🔎 Alternative approach using os.tmpdir()

+import { tmpdir } from "node:os";
+
 	beforeAll(async () => {
-		await mkdir(join(process.cwd(), ".tmp"), { recursive: true });
-		tmpDir = await mkdtemp(join(process.cwd(), ".tmp", "skills-discovery-"));
+		tmpDir = await mkdtemp(join(tmpdir(), "skills-discovery-"));
 		projectDir = join(tmpDir, "repo");

Note: os.tmpdir() returns a directory that already exists, eliminating the need for the explicit mkdir call.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9b87906 and 8a65660.

📒 Files selected for processing (1)

• packages/orchestrator/test/unit/skills-discovery.test.ts

🧰 Additional context used

🧬 Code graph analysis (1)

packages/orchestrator/test/unit/skills-discovery.test.ts (1)

packages/orchestrator/src/skills/discovery.ts (2)

• getSkillSearchRoots (49-84)
• discoverSkills (86-114)

🔇 Additional comments (2)

packages/orchestrator/test/unit/skills-discovery.test.ts (2)

30-34: Good defensive cleanup logic.

The null check prevents a TypeError when beforeAll fails to initialize tmpDir. Combined with force: true, this ensures cleanup is robust even in failure scenarios.

---

52-96: LGTM!

The test correctly uses async/await for file system operations and properly structures parallel operations with Promise.all for efficiency.