test: cover resolveSubagentPath candidates#331
test: cover resolveSubagentPath candidates#331looooown2006 wants to merge 2 commits intoexospherehost:mainfrom
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds a comprehensive Vitest test suite for the ChangesSubagent Path Resolution Tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 1
🧹 Nitpick comments (1)
__tests__/lib/resolve-subagent-path.test.ts (1)
32-51: ⚡ Quick winConsider testing candidate priority order.
The current tests verify each candidate location in isolation, but don't confirm that candidate 1 is preferred when multiple candidates exist. Adding a test that creates all three candidates and verifies candidate 1 is returned would strengthen confidence in the priority logic.
📋 Example priority test
it("returns candidate 1 when all candidates exist", async () => { const candidate1 = join(projectsPath, projectName, `agent-${agentId}.jsonl`); const candidate2 = join(projectsPath, projectName, sessionId, `agent-${agentId}.jsonl`); const candidate3 = join(projectsPath, projectName, sessionId, "subagents", `agent-${agentId}.jsonl`); await touch(candidate3); await touch(candidate2); await touch(candidate1); await expect(resolveSubagentPath(projectsPath, projectName, sessionId, agentId)).resolves.toBe(candidate1); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@__tests__/lib/resolve-subagent-path.test.ts` around lines 32 - 51, Add a test that verifies candidate priority by creating all three possible files and asserting resolveSubagentPath returns the project-level path (candidate 1). Use the existing helper touch to create candidate3 (join(projectsPath, projectName, sessionId, "subagents", `agent-${agentId}.jsonl`)), candidate2 (join(projectsPath, projectName, sessionId, `agent-${agentId}.jsonl`)), then candidate1 (join(projectsPath, projectName, `agent-${agentId}.jsonl`)) and call await expect(resolveSubagentPath(projectsPath, projectName, sessionId, agentId)).resolves.toBe(candidate1) to confirm priority.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@__tests__/lib/resolve-subagent-path.test.ts`:
- Around line 57-64: The test currently leaves an escaped file outside
fixtureRoot because traversalAgentId ("../../../../escape") normalizes above
projectsPath; change the fixture so the `escapedCandidate` path normalizes
inside `fixtureRoot` while still being outside `projectsPath` (e.g., deepen the
temp fixture directory structure and pick a traversalAgentId that climbs out of
`projectsPath` but remains within the fixtureRoot tree), ensure any directories
for `escapedCandidate` are created before calling `touch`, and/or explicitly
remove the created escaped file in the test teardown (afterEach) to guarantee
cleanup; update references in the test to use the adjusted
traversalAgentId/fixture layout (symbols: traversalAgentId, escapedCandidate,
projectsPath, fixtureRoot, touch, afterEach, resolveSubagentPath).
---
Nitpick comments:
In `@__tests__/lib/resolve-subagent-path.test.ts`:
- Around line 32-51: Add a test that verifies candidate priority by creating all
three possible files and asserting resolveSubagentPath returns the project-level
path (candidate 1). Use the existing helper touch to create candidate3
(join(projectsPath, projectName, sessionId, "subagents",
`agent-${agentId}.jsonl`)), candidate2 (join(projectsPath, projectName,
sessionId, `agent-${agentId}.jsonl`)), then candidate1 (join(projectsPath,
projectName, `agent-${agentId}.jsonl`)) and call await
expect(resolveSubagentPath(projectsPath, projectName, sessionId,
agentId)).resolves.toBe(candidate1) to confirm priority.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9280d3d8-3dff-41eb-a80d-915de7878ea5
📒 Files selected for processing (1)
__tests__/lib/resolve-subagent-path.test.ts
| it("does not resolve an existing candidate outside the projects path", async () => { | ||
| const traversalAgentId = "../../../../escape"; | ||
| const escapedCandidate = join(projectsPath, projectName, `agent-${traversalAgentId}.jsonl`); | ||
| expect(relative(projectsPath, escapedCandidate).startsWith("..")).toBe(true); | ||
| await touch(escapedCandidate); | ||
|
|
||
| await expect(resolveSubagentPath(projectsPath, projectName, sessionId, traversalAgentId)).resolves.toBeNull(); | ||
| }); |
There was a problem hiding this comment.
Test leak: escaped file not cleaned up.
The path traversal test creates a file outside fixtureRoot (at /tmp/escape.jsonl or similar), but the afterEach hook only removes fixtureRoot. This leaves the escaped file behind, polluting the test environment.
The calculation:
join(projectsPath, projectName, "agent-../../../../escape.jsonl")- With
projectsPath = "/tmp/failproofai-subagents-XXXXXX/projects" - Normalizes to:
/tmp/escape.jsonl(outside the fixture)
🧹 Proposed fix: deepen fixture structure to contain the escaped path
beforeEach(async () => {
fixtureRoot = await mkdtemp(join(tmpdir(), "failproofai-subagents-"));
- projectsPath = join(fixtureRoot, "projects");
+ // Use a deeper structure so that even with traversal, we stay within fixtureRoot
+ projectsPath = join(fixtureRoot, "a", "b", "c", "d", "projects");
await mkdir(join(projectsPath, projectName, sessionId, "subagents"), { recursive: true });
});With this change:
escapedCandidatenormalizes to/tmp/failproofai-subagents-XXXXXX/a/b/c/escape.jsonl- Still outside
projectsPath(satisfies line 60's assertion) - But inside
fixtureRoot(properly cleaned up)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@__tests__/lib/resolve-subagent-path.test.ts` around lines 57 - 64, The test
currently leaves an escaped file outside fixtureRoot because traversalAgentId
("../../../../escape") normalizes above projectsPath; change the fixture so the
`escapedCandidate` path normalizes inside `fixtureRoot` while still being
outside `projectsPath` (e.g., deepen the temp fixture directory structure and
pick a traversalAgentId that climbs out of `projectsPath` but remains within the
fixtureRoot tree), ensure any directories for `escapedCandidate` are created
before calling `touch`, and/or explicitly remove the created escaped file in the
test teardown (afterEach) to guarantee cleanup; update references in the test to
use the adjusted traversalAgentId/fixture layout (symbols: traversalAgentId,
escapedCandidate, projectsPath, fixtureRoot, touch, afterEach,
resolveSubagentPath).
1 participant
Summary
Add unit coverage for
resolveSubagentPath(...).Changes
__tests__/lib/resolve-subagent-path.test.ts.agent-<id>.jsonlagent-<id>.jsonlsubagents/agent-<id>.jsonlagentIdby creating an existing candidate outsideprojectsPathand verifying it is still refused.Tests
bun run test:run __tests__/lib/resolve-subagent-path.test.tsgit diff --cached --checkFull-suite note:
bun run test:runwas also run in this Windows workspace after repairing the local Bun test entrypoint. The newresolve-subagent-pathtest passed, but the full suite is not green locally due to unrelated Windows/path and incomplete local dependency-install issues (for example POSIX path expectation failures and missingnext/*imports afterbun installrepeatedly timed out).Refs #328.
Summary by CodeRabbit