test(lib): add comprehensive unit tests for resolve-subagent-path (#328) - #628
test(lib): add comprehensive unit tests for resolve-subagent-path (#328)#628AVPthegreat wants to merge 1 commit into
Conversation
|
Your PR is awaiting review by a reviewer. Till then you can join the Discord for conversation: https://discord.befailproof.ai |
📝 WalkthroughWalkthroughThe test suite now simulates filesystem access errors, resets mock state between tests, and verifies project-name path traversal is rejected while unexpected access errors return ChangesResolver test coverage
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
__tests__/lib/resolve-subagent-path.test.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. 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: 2
🤖 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 95-101: Update the traversalProject fixture in the
resolveSubagentPath traversal test to use ../outside or another path that
remains beneath the per-test fixtureRoot while still escaping projectsPath. Keep
the escaped-candidate setup and expected null result unchanged.
- Around line 33-40: Update the test setup in beforeEach to clear the access
mock’s call history, then strengthen the EACCES test assertions to verify access
is called exactly once with the project-level candidate path, ensuring candidate
search stops on unexpected errors. Apply the same assertion update to the
related test around the second referenced range.
🪄 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 Plus
Run ID: 29b4e022-ac71-4767-a30c-c26d61136f26
📒 Files selected for processing (1)
__tests__/lib/resolve-subagent-path.test.ts
| mockAccessError = null; | ||
| fixtureRoot = await mkdtemp(join(tmpdir(), "failproofai-subagents-")); | ||
| projectsPath = join(fixtureRoot, "projects"); | ||
| await mkdir(join(projectsPath, projectName, sessionId, "subagents"), { recursive: true }); | ||
| }); | ||
|
|
||
| afterEach(async () => { | ||
| mockAccessError = null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file and related symbols =="
git ls-files | rg '(^|/)resolve-subagent-path\.(ts|js)$|resolve-subagent-path.test.ts' || true
echo
echo "== excerpt test file around mentioned lines =="
if [ -f "__tests__/lib/resolve-subagent-path.test.ts" ]; then
wc -l "__tests__/lib/resolve-subagent-path.test.ts"
sed -n '1,150p' "__tests__/lib/resolve-subagent-path.test.ts" | nl -ba -v1
fi
echo
echo "== find source file(s) =="
fd -a 'resolve-subagent-path' . | sed 's#^\./##'Repository: FailproofAI/failproofai
Length of output: 409
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== test file =="
cat -n "__tests__/lib/resolve-subagent-path.test.ts"
echo
echo "== source file =="
cat -n "lib/resolve-subagent-path.ts"Repository: FailproofAI/failproofai
Length of output: 6744
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package vitest availability =="
if [ -f package.json ]; then
cat package.json | sed -n '1,160p'
fi
echo
echo "== deterministic call-history behavior for the mocked fs/promises access =="
node - <<'JS'
const calls = [];
const original = async (path) => {
if (path.endsWith("candidate2.jsonl")) throw Object.assign(new Error("EACCES"), { code: "EACCES" });
};
async function resolveSubagentPath() {
const candidatePaths = ["candidate1.jsonl", "candidate2.jsonl", "candidate3.jsonl"];
for (const candidatePath of candidatePaths) {
try {
await original(candidatePath);
} catch (e) {
if ((e).code === "ENOENT") continue;
break;
}
}
return null;
}
(async () => {
const result = await resolveSubagentPath();
console.log(JSON.stringify({ result, totalCalls: calls.length }));
})();
JS
echo
echo "== test assertions for access.calls/callCount =="
rg -n "access\\.calls|\.calls\\.length|callCount|mocked\(" "__tests__" "lib" || trueRepository: FailproofAI/failproofai
Length of output: 48531
Assert that unexpected errors stop candidate search.
The EACCES expectation only checks result === null; it would still pass if the loop checked the remaining candidates. Clear the mock call history in beforeEach, then assert that access was called exactly once with the project-level candidate in the error case.
Also applies to: 103-110
🤖 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 33 - 40, Update the
test setup in beforeEach to clear the access mock’s call history, then
strengthen the EACCES test assertions to verify access is called exactly once
with the project-level candidate path, ensuring candidate search stops on
unexpected errors. Apply the same assertion update to the related test around
the second referenced range.
| it("prevents path traversal when projectName attempts to escape projectsPath", async () => { | ||
| const traversalProject = "../../outside"; | ||
| const escapedCandidate = join(projectsPath, traversalProject, `agent-${agentId}.jsonl`); | ||
| await touch(escapedCandidate); | ||
|
|
||
| await expect(resolveSubagentPath(projectsPath, traversalProject, sessionId, agentId)).resolves.toBeNull(); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Keep traversal fixtures inside the per-test temporary directory.
../../outside escapes above fixtureRoot, so touch() creates an artifact that the test cleanup will not remove. Use ../outside (or another path beneath fixtureRoot) while remaining outside projectsPath.
🤖 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 95 - 101, Update
the traversalProject fixture in the resolveSubagentPath traversal test to use
../outside or another path that remains beneath the per-test fixtureRoot while
still escaping projectsPath. Keep the escaped-candidate setup and expected null
result unchanged.
Summary
Fixes #328 by adding comprehensive unit tests for
lib/resolve-subagent-path.tsin__tests__/lib/resolve-subagent-path.test.ts, covering candidate fallback order, path traversal guards (projectName,sessionId,agentId), and non-ENOENTerror handling (e.g.EACCES).🧪 Test Coverage Added
Added test scenarios in
__tests__/lib/resolve-subagent-path.test.ts:{projectsPath}/{projectName}/agent-{agentId}.jsonl{projectsPath}/{projectName}/{sessionId}/agent-{agentId}.jsonl{projectsPath}/{projectName}/{sessionId}/subagents/agent-{agentId}.jsonl../../outsideor../../../../escape) inprojectName,sessionId, oragentIdare rejected and returnnull.EACCES) break the search loop and returnnull.🏁 Verification Results
vitest run __tests__/lib/resolve-subagent-path.test.ts✓ 8 passed (8)Summary by CodeRabbit
Bug Fixes
Tests