Return a test summary from run_tests

[sergical]

run_tests now runs vitest with the JSON reporter and returns only the passed and failed counts plus the names of failing tests, so the full verbose log no longer enters the reviewer's prompt. The tests themselves, including the slow integration suite, are unchanged.

Created with Claude Code

[sentry]

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.}

[cursor]

Cursor Bugbot has reviewed your changes and found 2 potential issues.

^{❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.}

^{Reviewed by Cursor Bugbot for commit 9e04910. Configure here.}

[cursor]

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.

 

^{Reviewed by Cursor Bugbot for commit 9e04910. Configure here.}

[cursor]

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)

• apps/pr-reviewer/src/agents/review.ts#L89-L93

 

^{Reviewed by Cursor Bugbot for commit 9e04910. Configure here.}

[github-actions]

Verdict: Approve with minor follow-ups — the refactor to structured JSON test output works (tests ran: 36 passed, 0 failed), but there are real robustness gaps worth addressing before merge.

Correctness

• Stale report reuse (real bug): reportPath (/tmp/vitest.json) is never deleted before invoking vitest. If npx vitest run ... fails before it manages to write the JSON reporter output (e.g., command not found, config crash, or killed process), the code will happily parse a leftover report from a previous run and report its pass/fail counts as if they came from the current run — silently producing wrong results instead of surfacing the failure. Delete/truncate reportPath (or write to a fresh unique path) before each run.
• Lost diagnostics on failure: The previous implementation surfaced stdout/stderr on failure. Now, if vitest fails outright (and no valid report is written), the only output is the generic string Tests failed to run (exit ${exitCode}) — no indication of why (missing binary, syntax error, etc.). This is a real regression in debuggability; consider still capturing/logging stdout/stderr from the exec call for this case.
• Hardcoded shared temp path: Using a fixed /tmp/vitest.json is unsafe if this tool is ever invoked concurrently (parallel CI jobs, multiple agent runs on one host) — runs could clobber or read each other's reports. Prefer a unique path per invocation (os.tmpdir() + random suffix) and clean up afterward.
• Untyped report shape: report from JSON.parse is effectively any; numPassedTests/numFailedTests/testResults and the nested test in .filter/.map have no compile-time guarantee of matching vitest's actual JSON reporter schema. A schema mismatch (e.g. across vitest versions) would silently yield undefined fields rather than a clear error.
• Minor: output shape is inconsistent between the success path (JSON string) and the failure path (plain string) — downstream consumers of this tool's return value now have to branch on format.

Style

• The bare catch {} around the report-parsing swallows the real error; at least log.warn/log.error it so failures to read/parse the report are debuggable.
• Extract the inline anonymous type for the flatMap callback parameter (file: { name: string; assertionResults: {...}[] }) into a named interface (e.g. VitestFileResult) for readability and reuse.
• Consider naming the constant vitestReportPath for clarity, and defining a small VitestJsonReport type for the parsed report instead of leaving it implicitly any.
• The narrowing const failure = error as { code?: number } is an unchecked assumption about the error shape (pre-existing pattern, but worth a comment now that it sits next to more careful JSON handling).

Tests

run_tests passed: {"passed":36,"failed":0,"failures":[]} — no failures to report.

[linear-code]

MAR-1676