Skip to content

Return a test summary from run_tests #18

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sergical merged 1 commit into from
Oct 6, 2026
Merged

Return a test summary from run_tests #18

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 24 additions & 8 deletions apps/pr-reviewer/src/agents/review.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,18 +79,34 @@ export function ReviewLead() {
description: 'Run the demo-pr test suite and return its output',
input: v.object({}),
async run({ log }) {
let output: string;
const reportPath = '/tmp/vitest.json';

@sentry sentry Bot Oct 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The code uses a hardcoded file path /tmp/vitest.json for test reports. This can cause race conditions or stale data reads if multiple reviews run concurrently on the same machine.
Severity: LOW

Suggested Fix

Use a unique filename for the temporary report file to avoid conflicts. This can be achieved by appending a unique identifier, such as a process ID (process.pid) or a timestamp, to the filename. For example, /tmp/vitest-${process.pid}.json. Ensure the temporary file is cleaned up after use.

Prompt for AI Agent 
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: apps/pr-reviewer/src/agents/review.ts#L82

Potential issue: The code writes Vitest test coverage reports to a hardcoded path,
`/tmp/vitest.json`. While the current GitHub Actions environment likely isolates runs,
preventing this issue, it introduces a latent bug. If the execution environment changes
to a persistent one (e.g., a long-running server, a different CI system, or local
development), concurrent processes could overwrite this file, leading to a race
condition. Similarly, if a run fails to clean up the file, a subsequent run might read
stale data, leading to incorrect analysis. The code relies on environmental assumptions
for correctness.

Did we get this right? 👍 / 👎 to inform future reviews.

let exitCode = 0;
try {
const { stdout, stderr } = await execFileAsync('npm', ['test'], {
cwd: 'fixtures/demo-pr',
maxBuffer: 64 * 1024 * 1024,
});
output = `${stdout}\n${stderr}`;
await execFileAsync(
'npx',
['vitest', 'run', '--reporter=json', `--outputFile=${reportPath}`],
{ cwd: 'fixtures/demo-pr', maxBuffer: 64 * 1024 * 1024 },
);
} catch (error) {
const failure = error as { code?: number; stdout?: string; stderr?: string };
const failure = error as { code?: number };
exitCode = typeof failure.code === 'number' ? failure.code : 1;
output = `Tests failed (exit ${exitCode})\n${failure.stdout ?? ''}\n${failure.stderr ?? ''}`;
}
let output: string;
try {
const report = JSON.parse(await readFile(reportPath, 'utf8'));

@cursor cursor Bot Oct 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stale test report can be reused

Low Severity

run_tests always reads /tmp/vitest.json after the process exits, including when this run never wrote that file. A leftover report from an earlier invocation is then returned as this run's counts and failure names.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9e04910. Configure here.

const failures = report.testResults.flatMap(
(file: { name: string; assertionResults: { status: string; fullName: string }[] }) =>
file.assertionResults
.filter((test) => test.status === 'failed')
.map((test) => `${path.basename(file.name)}: ${test.fullName}`),
);
output = JSON.stringify({
passed: report.numPassedTests,
failed: report.numFailedTests,
failures,
});

@cursor cursor Bot Oct 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suite failures look like success

Medium Severity

The summary keeps only numFailedTests and assertion results whose status is failed, and it is returned even when the process exited non-zero. A file that failed to load therefore shows failed as 0 with an empty failures list, so the review treats the suite as passing.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 9e04910. Configure here.

} catch {
output = `Tests failed to run (exit ${exitCode})`;
}
log.info('tests finished', { exitCode, bytes: output.length });
return output;
Expand Down
Loading