test(#245): add 001-basic-comments review eval case

[rh-hemartin]

Summary

• Adds 001-basic-comments under eval/review/cases/ -- a simple review eval case with a PR containing a resource leak (open() without context manager) and SQL injection (f-string in query). Validates the review agent posts comments identifying both.

Closes #245 (partially; does not address the REVIEW_TOKEN identity blocker yet).

Test plan

• Run the review eval locally against this case
• Confirm the agent finds both bugs and requests changes

🤖 Generated with Claude Code

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:31 AM UTC · Completed 10:41 AM UTC
Commit: 25a2050 · View workflow run →

[fullsend-ai-review]

Review

Findings

Medium

• [error-handling] eval/scripts/run-fullsend.sh:144 — The mint command captures both stdout and stderr into REVIEW_TOKEN via 2>&1. On the success path, if fullsend mint token writes any diagnostic or warning text to stderr while still exiting 0, the captured output will contain non-token text, producing an invalid REVIEW_TOKEN that silently causes authentication failures downstream.
  Remediation: Capture stderr to a temporary file or /dev/null on the success path, so REVIEW_TOKEN receives only stdout (e.g., REVIEW_TOKEN=$(fullsend mint token ... 2>"$mint_err") and include $mint_err in the failure diagnostic).

Low

• [secrets-handling] eval/scripts/run-fullsend.sh:146 — On mint failure, the combined stdout+stderr of fullsend mint token is logged to stderr via echo. While this is error output (not a valid token), the output could contain partial tokens or internal service details that appear in CI logs.
  Remediation: Consider redacting or truncating the captured output in the error message.

• [undocumented-dependency] eval/scripts/run-fullsend.sh:141 — The script introduces a dependency on FULLSEND_MINT_URL (with a hardcoded default) and calls fullsend mint token --role review, but eval/README.md does not document this variable or explain when/where the mint service is available.
  Remediation: Add FULLSEND_MINT_URL to the environment variables documentation in eval/README.md.

• [undocumented-behavior] eval/README.md:85 — The "Derived" section states that REVIEW_TOKEN is simply derived from GH_TOKEN, but the new logic in run-fullsend.sh conditionally mints a dedicated review token for review agent PRs.
  Remediation: Update the REVIEW_TOKEN row in the "Derived" table to explain conditional minting.

• [pr-body-stale] — The PR body states "does not address the REVIEW_TOKEN identity blocker yet," but the diff includes REVIEW_TOKEN minting logic that directly implements the identity blocker fix described in issue Reintroduce review eval tests — self-review token blocker #245.
  Remediation: Update the PR description to reflect that this change addresses the REVIEW_TOKEN identity blocker.

Previous run

Review

Findings

High

• [stale-reference] eval/scripts/run-fullsend.sh — The PR modifies run-fullsend.sh against a version that predates significant refactoring on main. The current main branch version uses an emit_env helper for injection-safe dotenv writing, includes input shape validation, supports the fix agent, and has a different workspace layout. The PR's diff uses raw echo statements and the old layout. This will produce merge conflicts and, if merged, would regress injection defenses and fix-agent support.
  Remediation: Rebase the branch onto current main and reimplement the REVIEW_TOKEN minting logic using the emit_env helper and current file structure.

Medium

• [error-handling] eval/scripts/run-fullsend.sh:59 — The mint command captures both stdout and stderr into MINTED via 2>&1. On the success path, REVIEW_TOKEN is set to the full captured output. If fullsend mint token writes any diagnostic or warning output to stderr while still exiting 0, the token value will be contaminated with non-token text, producing an invalid REVIEW_TOKEN that silently causes authentication failures downstream. See also: [injection] finding on the same code.
  Remediation: Capture stderr separately so the success path gets only stdout (e.g., redirect stderr to a temp file and reference it only in the failure message).

Low

• [injection] eval/scripts/run-fullsend.sh — The MINTED variable may contain newlines (from stderr capture via 2>&1). This value is written via raw echo without newline validation, which could allow injection of additional environment variables into the dotenv file. This concern is subsumed by the stale-reference finding: once rebased onto main, emit_env rejects newlines. See also: [error-handling] finding on the same code.
  Remediation: When rebasing onto current main, ensure the REVIEW_TOKEN write uses emit_env rather than raw echo.

• [undocumented-dependency] eval/scripts/run-fullsend.sh:56 — The script introduces a dependency on FULLSEND_MINT_URL (with a hardcoded default) and calls fullsend mint token --role review, but eval/README.md does not document this variable or explain when/where the mint service is available.
  Remediation: Add FULLSEND_MINT_URL to the environment variables documentation in eval/README.md.

• [pr-body-stale] — The PR body states "does not address the REVIEW_TOKEN identity blocker yet," but the changes to run-fullsend.sh directly implement the REVIEW_TOKEN minting logic that is the identity blocker described in issue Reintroduce review eval tests — self-review token blocker #245.
  Remediation: Update the PR description to reflect that this change addresses the REVIEW_TOKEN identity blocker.

Previous run (2)

Review

Findings

Low

• [scope-creep] eval/review/cases/001-basic-comments/input.yaml — PR adds a new review eval case (001-basic-comments). The three explicit to-do items in issue Reintroduce review eval tests — self-review token blocker #245 do not list "add new eval cases," but the issue title ("Reintroduce review eval tests" — plural) and the empty cases directory (.gitkeep only) suggest that adding eval cases is within the spirit of the issue. The PR is also marked as a draft and states "Closes Reintroduce review eval tests — self-review token blocker #245 (partially)," indicating incremental progress rather than full completion.
  Remediation: Consider linking this work to the issue in a comment explaining that 001-basic-comments is prerequisite infrastructure for reintroducing review eval tests, or file a brief sub-issue if the team prefers explicit authorization for each eval case.

Previous run (3)

Review

Findings

Low

• [scope-creep] eval/review/cases/001-basic-comments/input.yaml — PR adds a new review eval case (001-basic-comments). The three explicit to-do items in issue Reintroduce review eval tests — self-review token blocker #245 do not list "add new eval cases," but the issue title ("Reintroduce review eval tests" — plural) and the empty cases directory (.gitkeep only) suggest that adding eval cases is within the spirit of the issue. The PR is also marked as a draft and states "Closes Reintroduce review eval tests — self-review token blocker #245 (partially)," indicating incremental progress rather than full completion.
  Remediation: Consider linking this work to the issue in a comment explaining that 001-basic-comments is prerequisite infrastructure for reintroducing review eval tests, or file a brief sub-issue if the team prefers explicit authorization for each eval case.

---

Labels: PR adds review agent eval test case infrastructure under eval/review/cases/

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:45 AM UTC · Completed 10:55 AM UTC
Commit: e65fe01 · View workflow run →

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 11:10 AM UTC · Ended 11:23 AM UTC
Commit: 27e9b95 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 11:25 AM UTC · Completed 11:41 AM UTC
Commit: d7143b9 · View workflow run →

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:12 AM UTC · Completed 8:29 AM UTC
Commit: 09b94d9 · View workflow run →

Superseded by updated review

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[medium] error-handling

The mint command captures both stdout and stderr into REVIEW_TOKEN via 2>&1. On the success path, if fullsend mint token writes any diagnostic or warning text to stderr while still exiting 0, the captured output will contain non-token text, producing an invalid REVIEW_TOKEN that silently causes authentication failures downstream.

Suggested fix: Capture stderr to a temporary file or /dev/null on the success path, so REVIEW_TOKEN receives only stdout.

[fullsend-ai-review]

[low] secrets-handling

On mint failure, the combined stdout+stderr of fullsend mint token is logged to stderr via echo. While this is error output (not a valid token), the output could contain partial tokens or internal service details that appear in CI logs.

Suggested fix: Consider redacting or truncating the captured output in the error message.

[fullsend-ai-review]

[low] undocumented-dependency

The script introduces a dependency on FULLSEND_MINT_URL (with a hardcoded default) and calls fullsend mint token --role review, but eval/README.md does not document this variable or explain when/where the mint service is available.

Suggested fix: Add FULLSEND_MINT_URL to the environment variables documentation in eval/README.md.