feat(tasks): add golden PR evals for coding agents - #107390
pauldambra wants to merge 7 commits into
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review 🦔 PostHog Review reviewed this pull requestFound 0 must fix, 2 should fix, 0 consider. Published 2 findings (view the review). |
🤖 CI report🚨 Trunk lane — universal laneThis PR is assigned to the universal lane. It cannot merge in parallel with other PRs, so it can take longer to merge. Ask dev-ex if you think this is wrong. ✅ Duplication (Python) — cleanNew Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying. ✅ Duplication (TypeScript) — cleanNew TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
|
| File | Comment lines | Added lines |
|---|---|---|
products/tasks/evals/golden_prs/workspace.py |
16 | 139 |
products/tasks/evals/golden_prs/scoring.py |
5 | 136 |
products/tasks/evals/golden_prs/agents.py |
4 | 93 |
products/tasks/evals/golden_prs/cases.py |
2 | 42 |
products/tasks/evals/golden_prs/__main__.py |
1 | 137 |
This check does not block merging. It updates on every push and clears when the share drops.
✅ Bundle size — no change
Uncompressed size of every built .js bundle, compared against the base branch.
Total: 68.88 MiB · no change
No file changed by more than 1000 B.
Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report
✅ Eager graph — within budget
How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.
| Root | Eager (shipped) | Δ vs base | Budget |
|---|---|---|---|
entry (logged-out pages, app bootstrap)src/index.tsx |
1.57 MiB · 22 files | no change | █████████░ 85.5% of 1.84 MiB |
logged-out boot: index + App + bootApp (preloaded by every page, including /login)src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts |
3.51 MiB · 629 files | no change | █████████░ 87.2% of 4.03 MiB |
authenticated shell (every logged-in page)src/scenes/AuthenticatedShell.tsx |
7.34 MiB · 2,339 files | no change | █████████░ 88.0% of 8.34 MiB |
🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/layout/navigation-3000/navigationLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/scenes/dashboard/dashboardLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/lemon-ui/LemonMarkdown/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/RichContentEditor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/CodeSnippet/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/taxonomy/core-filter-definitions-by-group.json stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
Largest files eagerly shipped from src/index.tsx
| Size | File |
|---|---|
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 24.6 KiB | ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js |
| 6.3 KiB | ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js |
| 4.5 KiB | ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js |
| 3.9 KiB | ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js |
| 1.4 KiB | ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js |
| 1.3 KiB | src/index.tsx |
| 1.3 KiB | src/RootErrorBoundary.tsx |
| 912 B | ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js |
| 854 B | src/scenes/ChunkLoadErrorBoundary.tsx |
Largest files eagerly shipped from src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
| Size | File |
|---|---|
| 301.8 KiB | ../node_modules/.pnpm/posthog-js@1.434.14_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 216.0 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 100.5 KiB | src/lib/api.ts |
| 88.4 KiB | src/products.tsx |
| 69.4 KiB | src/lib/lemon-ui/icons/icons.tsx |
| 40.1 KiB | src/lib/utils/eventUsageLogic.ts |
| 38.7 KiB | ../node_modules/.pnpm/@dnd-kit+core@6.0.8_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@dnd-kit/core/dist/core.esm.js |
| 33.9 KiB | ../node_modules/.pnpm/kea@4.0.0-pre.6_patch_hash=139b8d1f1304f9d9da452a9a1244c94ea679dbcb85687d8999563146879fb6f5_react@18.3.1/node_modules/kea/lib/index.cjs.js |
| 28.4 KiB | src/scenes/scenes.ts |
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
| Size | File |
|---|---|
| 301.8 KiB | ../node_modules/.pnpm/posthog-js@1.434.14_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs |
| 271.7 KiB | src/taxonomy/core-filter-definitions-by-group.json |
| 216.0 KiB | ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js |
| 153.7 KiB | ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js |
| 126.8 KiB | ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js |
| 100.5 KiB | src/lib/api.ts |
| 98.5 KiB | ../packages/quill/packages/quill/dist/index.js |
| 93.3 KiB | ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js |
| 90.6 KiB | ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js |
| 88.4 KiB | src/products.tsx |
Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479
✅ Toolbar bundle — eager 2.16 MiB within budget
What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.
| Metric | Size | Δ vs base | Budget |
|---|---|---|---|
| Eager (shipped) entry + static imports |
2.16 MiB · 19 files | no change | ████░░░░░░ 37.7% of 5.72 MiB |
| Deferred (lazy) | 2.10 MiB · 44 files | no change | n/a — loads on demand |
Loader dist/toolbar.js |
1.2 KiB | no change | █░░░░░░░░░ 6.0% of 19.5 KiB |
Largest eagerly-shipped chunks
| Size | File |
|---|---|
| 800.5 KiB | dist/toolbar/toolbar-app-QUJ43CJ4.css |
| 651.6 KiB | dist/toolbar/chunk-chunk-ZPCK2O6G.js |
| 259.4 KiB | dist/toolbar/chunk-chunk-CV2VU6SQ.js |
| 138.3 KiB | dist/toolbar/chunk-chunk-DYPTRYMF.js |
| 131.8 KiB | dist/toolbar/chunk-chunk-FDH2IBXT.js |
| 75.2 KiB | dist/toolbar/toolbar-app-4HYNQ5KU.js |
| 69.0 KiB | dist/toolbar/chunk-chunk-TSAL54PB.js |
| 35.6 KiB | dist/toolbar/chunk-chunk-HOKNU4ZL.js |
| 21.0 KiB | dist/toolbar/chunk-chunk-Z5ELNJKM.js |
| 6.8 KiB | dist/toolbar/chunk-chunk-DV7IWQNF.js |
Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile
✅ Dist folder size — no change
Total size of the built frontend/dist folder (all assets), compared against the base branch.
Total: 946.25 MiB · no change
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Golden PR evaluation tool with a dataset of 12 pull requests. The CLI runs Claude or Codex in a temporary parent checkout, scores candidate diffs, and saves results and reports. A manual GitHub Actions workflow selects cases, runs evaluations, and publishes combined results. The change also adds usage documentation, test coverage, and the evaluation tests to the backend test command. Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to This is a manually triggered, internal evaluation tool, so production users are unaffected. However, the candidate-diff step can run an agent-configured command with the evaluator's API key and let the agent fake its own diff, and several scoring and reporting accuracy issues remain open. Resolve the diff sanitization before relying on these scores or running with secrets. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The new evaluation workflow may expose credentials to commands influenced by evaluated code. Manual triggering and isolated runs limit the likely scope, but the credential boundary needs attention. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 5
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (3)
products/tasks/evals/golden_prs/cases.py-34-37 (1)
34-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMaterialize
numbersbefore checking membership.If a caller passes a generator,
set(numbers)exhausts it. The return expression then selects no PRs, even when every number is valid. Convertnumbersto a list once and use that list for both operations.products/tasks/evals/golden_prs/scoring.py-60-60 (1)
60-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDistinguish added source lines from file headers.
A diff line such as
+++counterrepresents added source text++counter, but this condition discards it. Line F1 can therefore undercount valid additions. Exclude the actual+++ b/...header instead of every line with the+++prefix..github/workflows/golden-pr-evals.yml-66-66 (1)
66-66: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDeduplicate PR numbers before creating the matrix.
If
prscontains the same number twice, both jobs use the samegolden-pr-${{ matrix.pr }}artifact name.upload-artifactdoes not allow two jobs to create an artifact with the same name, so one evaluation can fail after consuming agent time. Preserve the requested order while removing duplicates. (github.com)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 731ee24d-0807-4af4-9b9b-92a8d96472af
📒 Files selected for processing (13)
.github/workflows/golden-pr-evals.ymlproducts/tasks/evals/__init__.pyproducts/tasks/evals/golden_prs/.gitignoreproducts/tasks/evals/golden_prs/README.mdproducts/tasks/evals/golden_prs/__init__.pyproducts/tasks/evals/golden_prs/__main__.pyproducts/tasks/evals/golden_prs/agents.pyproducts/tasks/evals/golden_prs/cases.pyproducts/tasks/evals/golden_prs/golden_prs.jsonproducts/tasks/evals/golden_prs/scoring.pyproducts/tasks/evals/golden_prs/test_golden_prs.pyproducts/tasks/evals/golden_prs/workspace.pyproducts/tasks/package.json
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 5 remain after this review.
| f"<golden_diff>\n{_bounded(golden)}\n</golden_diff>\n\n" | ||
| f"<candidate_diff>\n{_bounded(candidate)}\n</candidate_diff>" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove artifact sections before bounding judge diffs.
score_diffs excludes artifacts, but judge sends them to _bounded. If a snapshot section fills the first 120,000 characters, the judge never receives later source changes and can score an otherwise relevant attempt incorrectly. Filter artifact sections from both diffs before truncation.
|
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
| def _bounded(diff: str) -> str: | ||
| if len(diff) <= MAX_DIFF_CHARS_FOR_JUDGE: | ||
| return diff | ||
| return diff[:MAX_DIFF_CHARS_FOR_JUDGE] + "\n[diff truncated for the judge]\n" |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
Preserve relevant changes when bounding judge input
Issue description
_bounded keeps only the first 120,000 characters. If important changes occur later in a large diff, the judge cannot consider them even though it must score the full behavior. This can produce misleading scores for large PRs or candidate diffs.
Why we think it's a valid issue
- Checked: Traced
_boundedinproducts/tasks/evals/golden_prs/scoring.pyand its calls fromjudge(). Checked howevaluate()uses the verdict and what the report displays. - Found:
products/tasks/evals/golden_prs/scoring.py:88-91returns only the first 120,000 characters and adds a truncation marker.products/tasks/evals/golden_prs/scoring.py:113-114applies this independently to both diffs. The judge prompt atproducts/tasks/evals/golden_prs/scoring.py:22-29asks for a behavioral score, but does not tell the judge to treat omitted content as unknown.products/tasks/evals/golden_prs/__main__.py:51-65stores that score, which the report presents. - Impact: Large diffs can contain relevant behavior after the cutoff, and no code ensures the retained prefix represents the full change. The judge can therefore assign a misleading score based on incomplete evidence. This meets the correctness bar for the eval's reported judge score.
Suggested fix
Bound diffs in a way that represents changes across the full diff, such as selecting hunks across files and listing omitted files. Tell the judge what was omitted so it does not treat the partial diff as complete.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/scoring.py#L88-91
<issue_description>
`_bounded` keeps only the first 120,000 characters. If important changes occur later in a large diff, the judge cannot consider them even though it must score the full behavior. This can produce misleading scores for large PRs or candidate diffs.
</issue_description>
<issue_validation>
- **Checked:** Traced `_bounded` in `products/tasks/evals/golden_prs/scoring.py` and its calls from `judge()`. Checked how `evaluate()` uses the verdict and what the report displays.
- **Found:** `products/tasks/evals/golden_prs/scoring.py:88-91` returns only the first 120,000 characters and adds a truncation marker. `products/tasks/evals/golden_prs/scoring.py:113-114` applies this independently to both diffs. The judge prompt at `products/tasks/evals/golden_prs/scoring.py:22-29` asks for a behavioral score, but does not tell the judge to treat omitted content as unknown. `products/tasks/evals/golden_prs/__main__.py:51-65` stores that score, which the report presents.
- **Impact:** Large diffs can contain relevant behavior after the cutoff, and no code ensures the retained prefix represents the full change. The judge can therefore assign a misleading score based on incomplete evidence. This meets the correctness bar for the eval's reported judge score.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Bound diffs in a way that represents changes across the full diff, such as selecting hunks across files and listing omitted files. Tell the judge what was omitted so it does not treat the partial diff as complete.
</potential_solution>
| if not candidate.strip(): | ||
| return Verdict(score=0.0, reasoning="The agent changed no files.") | ||
| client = client or anthropic.Anthropic() | ||
| response = client.messages.parse( |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
Preserve eval results when the judge request fails
Issue description
If the Anthropic request fails after its retries, judge() raises and aborts the evaluation. The runner writes the candidate diff, agent log, and deterministic scores only after judge() returns, so a transient API failure loses the completed agent run and prevents later PRs in the batch from running.
Why we think it's a valid issue
- Checked: Traced the judge call through
evaluate()and the CLI loop inproducts/tasks/evals/golden_prs/__main__.py. Checked the GitHub workflow’s matrix failure behavior and artifact upload condition. - Found:
products/tasks/evals/golden_prs/scoring.py:104makes the API request without handling request errors.products/tasks/evals/golden_prs/__main__.py:51callsjudge()before returning the case result;products/tasks/evals/golden_prs/__main__.py:136-137writes the diff, log, and scores only afterevaluate()returns. A CLI batch therefore stops at a failed judge request without saving that case. In the workflow,fail-fast: falseallows other matrix jobs to continue, but the failed case still has no result files for the unconditional artifact upload to preserve. - Impact: A judge request failure after SDK retries can discard a completed agent run and its deterministic scores, requiring an expensive rerun to recover them. The later-PR impact applies to CLI batches, not the GitHub matrix.
Suggested fix
Handle judge failures as a separately recorded outcome. Persist the candidate diff, agent log, and deterministic scores even when no verdict is available, and report the judge error without treating it as a genuine zero score.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/scoring.py#L104
<issue_description>
If the Anthropic request fails after its retries, `judge()` raises and aborts the evaluation. The runner writes the candidate diff, agent log, and deterministic scores only after `judge()` returns, so a transient API failure loses the completed agent run and prevents later PRs in the batch from running.
</issue_description>
<issue_validation>
- **Checked:** Traced the judge call through `evaluate()` and the CLI loop in `products/tasks/evals/golden_prs/__main__.py`. Checked the GitHub workflow’s matrix failure behavior and artifact upload condition.
- **Found:** `products/tasks/evals/golden_prs/scoring.py:104` makes the API request without handling request errors. `products/tasks/evals/golden_prs/__main__.py:51` calls `judge()` before returning the case result; `products/tasks/evals/golden_prs/__main__.py:136-137` writes the diff, log, and scores only after `evaluate()` returns. A CLI batch therefore stops at a failed judge request without saving that case. In the workflow, `fail-fast: false` allows other matrix jobs to continue, but the failed case still has no result files for the unconditional artifact upload to preserve.
- **Impact:** A judge request failure after SDK retries can discard a completed agent run and its deterministic scores, requiring an expensive rerun to recover them. The later-PR impact applies to CLI batches, not the GitHub matrix.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Handle judge failures as a separately recorded outcome. Persist the candidate diff, agent log, and deterministic scores even when no verdict is available, and report the judge error without treating it as a genuine zero score.
</potential_solution>
| completed = subprocess.run( | ||
| agent_command(runtime, model), | ||
| cwd=workdir, | ||
| env=agent_environment(os.environ), | ||
| input=prompt, | ||
| capture_output=True, | ||
| text=True, | ||
| timeout=timeout_seconds, | ||
| check=False, |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
[should_fix] Terminate agent child processes on timeout
Issue description
When the timeout expires, subprocess.run terminates the CLI process but does not terminate its child processes. Agent CLIs can start shell commands and other tools, so those processes may keep using resources or modifying the checkout after the run times out. This can also make the collected candidate diff unreliable.
Why we think it's a valid issue
- Checked: Reviewed the CLI launch flags and the timeout handling in
run_agent(). - Found:
run_agent()starts the CLI withsubprocess.run(..., timeout=timeout_seconds)atproducts/tasks/evals/golden_prs/agents.py:59-67. The CLI runs with approval and sandbox checks bypassed atproducts/tasks/evals/golden_prs/agents.py:30-33, so it can launch shell commands and other child processes. The timeout handler records the run as timed out but does not manage a process group atproducts/tasks/evals/golden_prs/agents.py:67-67. - Impact: If a child process survives termination of the CLI, it can keep consuming resources or modify the checkout while
evaluate()collects the candidate diff. This can leave local runs with lingering processes and make results unreliable.
Suggested fix
Run the CLI in a new process session and, on timeout, terminate its process group with a graceful signal followed by a forced kill if needed. This keeps the timeout bounded for the full agent process tree.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/agents.py#L59-67
<issue_description>
When the timeout expires, `subprocess.run` terminates the CLI process but does not terminate its child processes. Agent CLIs can start shell commands and other tools, so those processes may keep using resources or modifying the checkout after the run times out. This can also make the collected candidate diff unreliable.
</issue_description>
<issue_validation>
- **Checked:** Reviewed the CLI launch flags and the timeout handling in `run_agent()`.
- **Found:** `run_agent()` starts the CLI with `subprocess.run(..., timeout=timeout_seconds)` at `products/tasks/evals/golden_prs/agents.py:59-67`. The CLI runs with approval and sandbox checks bypassed at `products/tasks/evals/golden_prs/agents.py:30-33`, so it can launch shell commands and other child processes. The timeout handler records the run as timed out but does not manage a process group at `products/tasks/evals/golden_prs/agents.py:67-67`.
- **Impact:** If a child process survives termination of the CLI, it can keep consuming resources or modify the checkout while `evaluate()` collects the candidate diff. This can leave local runs with lingering processes and make results unreliable.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Run the CLI in a new process session and, on timeout, terminate its process group with a graceful signal followed by a forced kill if needed. This keeps the timeout bounded for the full agent process tree.
</potential_solution>
| completed = subprocess.run( | ||
| agent_command(runtime, model), | ||
| cwd=workdir, | ||
| env=agent_environment(os.environ), | ||
| input=prompt, | ||
| capture_output=True, | ||
| text=True, |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
[should_fix] Bound captured agent output
Issue description
capture_output=True buffers all stdout and stderr in memory until the agent exits. A run can last 30 minutes, and Codex emits a JSON event stream, so a verbose run can consume substantial memory and fail the evaluation worker before results are written.
Why we think it's a valid issue
- Checked: Traced how
run_agent()captures output and howevaluate()stores it. - Found:
subprocess.run()captures all stdout and stderr in memory atproducts/tasks/evals/golden_prs/agents.py:59-68, andAgentRunretains both strings atproducts/tasks/evals/golden_prs/agents.py:73-81.evaluate()combines them into the agent log atproducts/tasks/evals/golden_prs/__main__.py:71, then writes the log only after the run atproducts/tasks/evals/golden_prs/__main__.py:74-78. There is no output-size limit or incremental write. - Impact: If an agent command produces large output during a run, the runner holds that output in memory until the run finishes and can exhaust available memory before it writes the results.
Suggested fix
Stream output to a log file or process it incrementally, and keep only a bounded amount in memory. Preserve the Claude usage fields needed for scoring while avoiding full-output buffering.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/agents.py#L59-65
<issue_description>
`capture_output=True` buffers all stdout and stderr in memory until the agent exits. A run can last 30 minutes, and Codex emits a JSON event stream, so a verbose run can consume substantial memory and fail the evaluation worker before results are written.
</issue_description>
<issue_validation>
- **Checked:** Traced how `run_agent()` captures output and how `evaluate()` stores it.
- **Found:** `subprocess.run()` captures all stdout and stderr in memory at `products/tasks/evals/golden_prs/agents.py:59-68`, and `AgentRun` retains both strings at `products/tasks/evals/golden_prs/agents.py:73-81`. `evaluate()` combines them into the agent log at `products/tasks/evals/golden_prs/__main__.py:71`, then writes the log only after the run at `products/tasks/evals/golden_prs/__main__.py:74-78`. There is no output-size limit or incremental write.
- **Impact:** If an agent command produces large output during a run, the runner holds that output in memory until the run finishes and can exhaust available memory before it writes the results.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Stream output to a log file or process it incrementally, and keep only a bounded amount in memory. Preserve the Claude usage fields needed for scoring while avoiding full-output buffering.
</potential_solution>
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/tasks/evals/golden_prs/__main__.py-43-45 (1)
43-45: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPersist the agent failure reason for partial diffs.
When the agent exits nonzero after producing a candidate diff, keep judging the diff, but also store
agent_failure(run)in a nullableagent_errorfield. The JSON result currently stores onlyexit_code, andreportshows only judge reasoning. The.agent.logretains raw output, but the structured result and report do not show why the run failed.Suggested fix
@@ duration_seconds: float exit_code: int + agent_error: str | None timed_out: bool @@ - verdict = verdict_for(run, prompt, candidate, golden, judge_model) + failure = agent_failure(run) + verdict = verdict_for(run, prompt, candidate, golden, judge_model) @@ duration_seconds=run.duration_seconds, exit_code=run.exit_code, + agent_error=failure, timed_out=run.timed_out, @@ - reasoning = "\n".join(f"- **#{r['pr']}** ({r['judge_score']:.2f}): {r['judge_reasoning']}" for r in results) + reasoning = "\n".join( + f"- **#{r['pr']}** ({r['judge_score']:.2f}): {r['judge_reasoning']}" + + (f" Agent error: {r['agent_error']}" if r.get("agent_error") else "") + for r in results + )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: f4a497ac-3b38-493a-9d2c-c7d29aa25121
📒 Files selected for processing (3)
products/tasks/evals/golden_prs/__main__.pyproducts/tasks/evals/golden_prs/agents.pyproducts/tasks/evals/golden_prs/test_golden_prs.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.
| def agent_usage(run: AgentRun) -> dict[str, float | int]: | ||
| """Cost and turn count as the Claude CLI reports them; Codex emits an event stream we do not parse.""" | ||
| report = _claude_report(run) | ||
| return {key: report[key] for key in ("total_cost_usd", "num_turns") if key in report} |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
[should_fix] Do not report unavailable Codex cost as zero
Issue description
agent_usage returns no usage data for Codex. The report then displays a missing cost as $0.00, which makes an unknown cost look measured and can mislead users comparing runtimes.
Why we think it's a valid issue
- Checked: Traced
agent_usagethroughevaluateintoreport, and checked the report test for missing usage. - Found:
products/tasks/evals/golden_prs/agents.py:44-56returns no usage for Codex.products/tasks/evals/golden_prs/__main__.py:74stores that result, andproducts/tasks/evals/golden_prs/__main__.py:100formats a missingtotal_cost_usdas0.00.products/tasks/evals/golden_prs/test_golden_prs.py:188confirms the zero-cost display for empty usage. - Impact: The report presents an unmeasured cost as a measured zero, making cost comparisons between runtimes misleading.
Suggested fix
Parse Codex usage data when available, or display an unavailable marker instead of defaulting a missing cost to zero.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/agents.py#L53-56
<issue_description>
`agent_usage` returns no usage data for Codex. The report then displays a missing cost as `$0.00`, which makes an unknown cost look measured and can mislead users comparing runtimes.
</issue_description>
<issue_validation>
- **Checked:** Traced `agent_usage` through `evaluate` into `report`, and checked the report test for missing usage.
- **Found:** `products/tasks/evals/golden_prs/agents.py:44-56` returns no usage for Codex. `products/tasks/evals/golden_prs/__main__.py:74` stores that result, and `products/tasks/evals/golden_prs/__main__.py:100` formats a missing `total_cost_usd` as `0.00`. `products/tasks/evals/golden_prs/test_golden_prs.py:188` confirms the zero-cost display for empty usage.
- **Impact:** The report presents an unmeasured cost as a measured zero, making cost comparisons between runtimes misleading.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Parse Codex usage data when available, or display an unavailable marker instead of defaulting a missing cost to zero.
</potential_solution>
| if header: | ||
| counting = not is_artifact(header.group(2)) | ||
| elif counting and line.startswith("+") and not line.startswith("+++"): | ||
| content = line[1:].strip() |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
[should_fix] Preserve indentation in added-line scoring
Issue description
strip() removes leading indentation before the line is scored. In Python, indentation can change behavior or make code invalid, but lines such as return value and return value then count as an exact match. This can inflate the deterministic line F1 score for an incorrect candidate.
Why we think it's a valid issue
- Checked: Traced
added_linesinto_f1and the displayedadded_line_f1score. Checked the existing test for added-line normalization. - Found:
products/tasks/evals/golden_prs/scoring.py:61strips leading and trailing whitespace before storing each added line.products/tasks/evals/golden_prs/scoring.py:67-73then counts normalized lines as exact overlap.products/tasks/evals/golden_prs/__main__.py:98displays that score. The test atproducts/tasks/evals/golden_prs/test_golden_prs.py:84-86covers blank-line skipping but does not check indentation. - Impact: Python indentation can change syntax and control flow. The F1 score can count differently indented lines as identical, so it can overstate line overlap for a materially different candidate.
Suggested fix
Preserve leading whitespace when extracting added lines. Skip blank lines with a separate content.strip() check, and only remove trailing whitespace if that is the intended normalization.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/scoring.py#L61
<issue_description>
`strip()` removes leading indentation before the line is scored. In Python, indentation can change behavior or make code invalid, but lines such as ` return value` and `return value` then count as an exact match. This can inflate the deterministic line F1 score for an incorrect candidate.
</issue_description>
<issue_validation>
- **Checked:** Traced `added_lines` into `_f1` and the displayed `added_line_f1` score. Checked the existing test for added-line normalization.
- **Found:** `products/tasks/evals/golden_prs/scoring.py:61` strips leading and trailing whitespace before storing each added line. `products/tasks/evals/golden_prs/scoring.py:67-73` then counts normalized lines as exact overlap. `products/tasks/evals/golden_prs/__main__.py:98` displays that score. The test at `products/tasks/evals/golden_prs/test_golden_prs.py:84-86` covers blank-line skipping but does not check indentation.
- **Impact:** Python indentation can change syntax and control flow. The F1 score can count differently indented lines as identical, so it can overstate line overlap for a materially different candidate.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Preserve leading whitespace when extracting added lines. Skip blank lines with a separate `content.strip()` check, and only remove trailing whitespace if that is the intended normalization.
</potential_solution>
| with checkout_parent(repo, pr) as workdir: | ||
| run = run_agent(runtime, model, prompt, workdir, timeout_seconds) | ||
| candidate = candidate_diff(workdir) | ||
| verdict = verdict_for(run, prompt, candidate, golden, judge_model) |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
[should_fix] Preserve agent artifacts when judging fails
Issue description
If the Anthropic request in judge() fails after its retries, evaluate() raises before returning the candidate diff and agent log. The caller therefore never reaches write_result(), and the workflow's always-run artifact upload has no completed agent output to preserve. This discards work already spent running the agent and prevents retrying only the judge.
Why we think it's a valid issue
- Checked: Traced
evaluate(),judge(), the result-writing call, and the workflow’s artifact upload step. - Found:
evaluate()callsverdict_for()before it returns the result, diff, and log atproducts/tasks/evals/golden_prs/__main__.py:55-58.judge()makes the Anthropic request without handling request errors atproducts/tasks/evals/golden_prs/scoring.py:103-119.main()callswrite_result()only afterevaluate()returns atproducts/tasks/evals/golden_prs/__main__.py:143-144. The workflow upload runs even after a failure, but uploads only files already present in the results directory at.github/workflows/golden-pr-evals.yml:166-172. - Impact: If the judge request raises, the completed agent diff and log are not persisted. The workflow cannot preserve those artifacts, and the judge cannot be retried without running the agent again.
Suggested fix
Persist the candidate diff and agent log before calling the judge, or catch judge failures and write a result that records the judge error while retaining the deterministic scores and agent artifacts.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/__main__.py#L58
<issue_description>
If the Anthropic request in `judge()` fails after its retries, `evaluate()` raises before returning the candidate diff and agent log. The caller therefore never reaches `write_result()`, and the workflow's always-run artifact upload has no completed agent output to preserve. This discards work already spent running the agent and prevents retrying only the judge.
</issue_description>
<issue_validation>
- **Checked:** Traced `evaluate()`, `judge()`, the result-writing call, and the workflow’s artifact upload step.
- **Found:** `evaluate()` calls `verdict_for()` before it returns the result, diff, and log at `products/tasks/evals/golden_prs/__main__.py:55-58`. `judge()` makes the Anthropic request without handling request errors at `products/tasks/evals/golden_prs/scoring.py:103-119`. `main()` calls `write_result()` only after `evaluate()` returns at `products/tasks/evals/golden_prs/__main__.py:143-144`. The workflow upload runs even after a failure, but uploads only files already present in the results directory at `.github/workflows/golden-pr-evals.yml:166-172`.
- **Impact:** If the judge request raises, the completed agent diff and log are not persisted. The workflow cannot preserve those artifacts, and the judge cannot be retried without running the agent again.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Persist the candidate diff and agent log before calling the judge, or catch judge failures and write a result that records the judge error while retaining the deterministic scores and agent artifacts.
</potential_solution>
| header = DIFF_HEADER.match(line) | ||
| if header: | ||
| counting = not is_artifact(header.group(2)) | ||
| elif counting and line.startswith("+") and not line.startswith("+++"): |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
Count added lines that begin with ++
Issue description
This condition skips every added diff line that starts with +++, not only the +++ b/... file header. A valid source line such as ++counter is therefore omitted from the added-line score, which can distort the reported F1.
Why we think it's a valid issue
- Checked: Traced
added_linesand searched the repository for valid source lines that begin with++after the diff prefix. - Found:
products/tasks/evals/golden_prs/scoring.py:60excludes every added line whose diff text starts with+++. The repository contains valid TypeScript lines such as++indexinfrontend/src/scenes/terminal/terminalHogql.ts:150. Another diff parser explicitly preserves content like++i;inproducts/desktop/packages/agent/src/adapters/codex-app-server/mapping.ts:162. - Impact: When such a line appears in a candidate or golden diff,
added_linesomits it and can distort the reportedadded_line_f1score. The trigger is uncommon, so the reportedconsiderpriority is appropriate.
Suggested fix
Exclude only the actual +++ b/... file header, or track whether the parser is inside a hunk before counting added lines.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/scoring.py#L60
<issue_description>
This condition skips every added diff line that starts with `+++`, not only the `+++ b/...` file header. A valid source line such as `++counter` is therefore omitted from the added-line score, which can distort the reported F1.
</issue_description>
<issue_validation>
- **Checked:** Traced `added_lines` and searched the repository for valid source lines that begin with `++` after the diff prefix.
- **Found:** `products/tasks/evals/golden_prs/scoring.py:60` excludes every added line whose diff text starts with `+++`. The repository contains valid TypeScript lines such as `++index` in `frontend/src/scenes/terminal/terminalHogql.ts:150`. Another diff parser explicitly preserves content like `++i;` in `products/desktop/packages/agent/src/adapters/codex-app-server/mapping.ts:162`.
- **Impact:** When such a line appears in a candidate or golden diff, `added_lines` omits it and can distort the reported `added_line_f1` score. The trigger is uncommon, so the reported `consider` priority is appropriate.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Exclude only the actual `+++ b/...` file header, or track whether the parser is inside a hunk before counting added lines.
</potential_solution>
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/tasks/evals/golden_prs/README.md-27-28 (1)
27-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winState the Claude judge prerequisite.
If a devbox has
codexbut noclaudeorANTHROPIC_API_KEY, the agent can run but the judge fails afterward. State that a Codex run also needs an API key or a signed-inclaudeCLI.
🧹 Nitpick comments (1)
products/tasks/evals/golden_prs/test_golden_prs.py (1)
185-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScrub repository-location variables in temporary-repository tests. An inherited
GIT_DIRorGIT_WORK_TREEcan redirect the Git commands despite theircwd.
products/tasks/evals/golden_prs/test_golden_prs.py#L185-L185: use a scrubbed environment for the prefix-test Git commands; keep theGIT_CONFIG_*settings under test.products/tasks/evals/golden_prs/test_golden_prs.py#L196-L196: use a scrubbed environment for the checkout-test Git commands, including commit and revision lookup.
Based on learnings: tests that spawn Git against temporary repositories should scrub repository-location variables.Source: Learnings
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 1942c479-d703-4b88-a2f4-21a2094c0660
📒 Files selected for processing (4)
products/tasks/evals/golden_prs/README.mdproducts/tasks/evals/golden_prs/scoring.pyproducts/tasks/evals/golden_prs/test_golden_prs.pyproducts/tasks/evals/golden_prs/workspace.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 7 remain after this review.
|
|
||
| # Generated or binary files: an agent cannot regenerate them without running the test suite, | ||
| # so they must not count against it. | ||
| ARTIFACT_SUFFIXES = (".ambr", ".snap", ".png", ".jpg", ".jpeg", ".gif", ".webp", ".ico") |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
Exclude SVG images from deterministic scores
Issue description
ARTIFACT_SUFFIXES does not include .svg, so is_artifact() counts SVG assets as changed source files and added_lines() scores their markup. When a golden PR changes an SVG, the file recall and line F1 can penalize an agent for not reproducing an image even though the evaluation says to ignore images.
Why we think it's a valid issue
- Checked: Traced
is_artifact()throughchanged_files()andadded_lines()inproducts/tasks/evals/golden_prs/scoring.py:49,products/tasks/evals/golden_prs/scoring.py:53, andproducts/tasks/evals/golden_prs/scoring.py:57. Checked the documented image-exclusion behavior inproducts/tasks/evals/golden_prs/README.md:20and the current golden PR files. - Found:
ARTIFACT_SUFFIXESatproducts/tasks/evals/golden_prs/scoring.py:16omits.svg. Both deterministic scoring paths useis_artifact(), so an SVG diff contributes to file overlap and added-line F1. The current golden PRs do not include SVG files, but the documented workflow supports adding more cases. - Impact: Adding a golden case that changes an SVG would make the deterministic scores count image markup, contrary to the documented image-exclusion behavior.
- Priority: Lowered to
considerbecause no current golden case is affected; the scoring mismatch applies when an SVG-bearing case is added.
Suggested fix
Add .svg and any other supported image formats to ARTIFACT_SUFFIXES, and cover the exclusion in the scoring tests.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/scoring.py#L16
<issue_description>
`ARTIFACT_SUFFIXES` does not include `.svg`, so `is_artifact()` counts SVG assets as changed source files and `added_lines()` scores their markup. When a golden PR changes an SVG, the file recall and line F1 can penalize an agent for not reproducing an image even though the evaluation says to ignore images.
</issue_description>
<issue_validation>
- **Checked:** Traced `is_artifact()` through `changed_files()` and `added_lines()` in `products/tasks/evals/golden_prs/scoring.py:49`, `products/tasks/evals/golden_prs/scoring.py:53`, and `products/tasks/evals/golden_prs/scoring.py:57`. Checked the documented image-exclusion behavior in `products/tasks/evals/golden_prs/README.md:20` and the current golden PR files.
- **Found:** `ARTIFACT_SUFFIXES` at `products/tasks/evals/golden_prs/scoring.py:16` omits `.svg`. Both deterministic scoring paths use `is_artifact()`, so an SVG diff contributes to file overlap and added-line F1. The current golden PRs do not include SVG files, but the documented workflow supports adding more cases.
- **Impact:** Adding a golden case that changes an SVG would make the deterministic scores count image markup, contrary to the documented image-exclusion behavior.
- **Priority:** Lowered to `consider` because no current golden case is affected; the scoring mismatch applies when an SVG-bearing case is added.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Add `.svg` and any other supported image formats to `ARTIFACT_SUFFIXES`, and cover the exclusion in the scoring tests.
</potential_solution>
|
🤖 Robot comment: first full end-to-end run of the golden PR evals, from a Mac with the signed-in claude claude-opus-5
codex gpt-5.5
Codex cost shows 0.00 because the runner does not parse usage from the What each agent got right or wrong
Scores that look wrong for the change
Known follow-ups, not in this run: parse Codex usage and cost, verify the SDK judge path with an API key set, and stop the agents from running the eval workspace's tests with the host venv. |
|
🤖 Robot comment: second golden PR run, this time with the two Claude and two GPT models we want to compare: claude-opus-5-5, claude-fable-5-1, gpt-6-sol and gpt-6-astra. Same setup as the first comment: signed-in claude claude-opus-5-5
claude claude-fable-5-1
codex gpt-6-sol
codex gpt-6-astra
Codex cost still shows 0.00 because the runner does not parse usage from the Mean scores across all six runs
The four new models sit close together. Opus 5.5 hits the right files and lines most often, and was the fastest and cheapest Claude run. Fable 5.1 costs about three times more per case than Opus 5.5 for a lower judge score. What each agent got right or wrong
Scores that look wrong for the change
The earlier follow-ups still stand: parse Codex usage and cost, verify the SDK judge path with an API key set, and stop the agents from setting up an environment in the eval workspace. The last one cost the Claude agents most of their time on some cases. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fce1687376
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| with checkout_parent(repo, pr) as workdir: | ||
| run = run_agent(runtime, model, prompt, workdir, timeout_seconds) | ||
| candidate = candidate_diff(workdir) | ||
| verdict = verdict_for(run, prompt, candidate, golden, judge_model) |
There was a problem hiding this comment.
If the judge raises—for example because the dispatch supplied an invalid judge model or the Anthropic request fails—the already-collected candidate diff and agent log exist only in memory here, and write_result() is never reached. The workspace has also been deleted by this point, so the artifact upload has no diagnostic files and the report simply omits this case. Persist the raw outputs before judging or convert judge exceptions into an explicit result.
Useful? React with 👍 / 👎.
pauldambra
left a comment
pauldambra
left a comment
There was a problem hiding this comment.
🤖 Robot comment: QA Swarm review complete. See inline comments and the summary comment.
|
🤖 Robot comment: Note 🤖 Automated comment by QA Swarm — not written by a human Multi-perspective review: router (cheap-first pass) + delegated reviewers (qa-team, paul-reviewer, xp-reviewer, security-audit as warranted) Verdict: ✅ APPROVE (round 3 @ 678729f)Round 3 reviewed only the delta since round 2 ( Key findings
ConvergenceNone. Reviewer summaries
Previous rounds (2)round 1 @ fce1687 — Automated by QA Swarm — not a human review |
| if failure and not candidate.strip(): | ||
| return Verdict(score=0.0, reasoning=f"The agent failed before changing any file: {failure}") | ||
| return judge(prompt, candidate, golden, model=judge_model) |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
Report failed agent runs as failures, not normal scores
Issue description
When an agent exits unsuccessfully after changing files, this code still asks the judge to score its diff. The report does not show the non-zero exit, so it presents that score as a normal completed run. When the failed agent changed nothing, the verdict is zero and the report includes it in the mean. Both outcomes can misrepresent an agent failure as its evaluation score.
Why we think it's a valid issue
- Checked: Traced failure handling in
products/tasks/evals/golden_prs/__main__.pyandproducts/tasks/evals/golden_prs/agents.py, including howreport()formats rows and calculates means. - Found:
verdict_for()calls the judge for any non-empty candidate, even whenagent_failure(run)returns a failure atproducts/tasks/evals/golden_prs/__main__.py:41-45.CaseResultrecordsexit_codeatproducts/tasks/evals/golden_prs/__main__.py:67-72, butreport()does not display it; it marks timeouts only atproducts/tasks/evals/golden_prs/__main__.py:95-100. - Found: The report calculates its file, line, and judge means across every result at
products/tasks/evals/golden_prs/__main__.py:103-105. A failed run with no candidate receives a zero verdict atproducts/tasks/evals/golden_prs/__main__.py:42-44and remains in those means. - Impact: The summary does not distinguish failed runs from completed evaluations, so readers can mistake partial or failed attempts for comparable scores. Including failed runs in the means can distort comparisons between agents.
Suggested fix
Record and display the agent failure as a separate run status. Do not include failed runs as ordinary judge scores in the report mean; if you keep a score for a partial diff, label it clearly as a failed run.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/__main__.py#L43-45
<issue_description>
When an agent exits unsuccessfully after changing files, this code still asks the judge to score its diff. The report does not show the non-zero exit, so it presents that score as a normal completed run. When the failed agent changed nothing, the verdict is zero and the report includes it in the mean. Both outcomes can misrepresent an agent failure as its evaluation score.
</issue_description>
<issue_validation>
- **Checked:** Traced failure handling in `products/tasks/evals/golden_prs/__main__.py` and `products/tasks/evals/golden_prs/agents.py`, including how `report()` formats rows and calculates means.
- **Found:** `verdict_for()` calls the judge for any non-empty candidate, even when `agent_failure(run)` returns a failure at `products/tasks/evals/golden_prs/__main__.py:41-45`. `CaseResult` records `exit_code` at `products/tasks/evals/golden_prs/__main__.py:67-72`, but `report()` does not display it; it marks timeouts only at `products/tasks/evals/golden_prs/__main__.py:95-100`.
- **Found:** The report calculates its file, line, and judge means across every result at `products/tasks/evals/golden_prs/__main__.py:103-105`. A failed run with no candidate receives a zero verdict at `products/tasks/evals/golden_prs/__main__.py:42-44` and remains in those means.
- **Impact:** The summary does not distinguish failed runs from completed evaluations, so readers can mistake partial or failed attempts for comparable scores. Including failed runs in the means can distort comparisons between agents.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Record and display the agent failure as a separate run status. Do not include failed runs as ordinary judge scores in the report mean; if you keep a score for a partial diff, label it clearly as a failed run.
</potential_solution>
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
.github/workflows/golden-pr-evals.yml-165-170 (1)
165-170: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNormalize
CASE_TIMEOUT_MINUTESto base 10 before arithmetic expansion.The validation accepts
08and010, but Bash arithmetic treats leading-zero values as octal.08can fail, and010becomes 8 minutes instead of 10.Suggested fix
esac + CASE_TIMEOUT_MINUTES=$((10#$CASE_TIMEOUT_MINUTES)) if [ "$CASE_TIMEOUT_MINUTES" -lt 1 ] || [ "$CASE_TIMEOUT_MINUTES" -gt 45 ]; then
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 1b8629b9-6fe6-4253-bc89-47fdb00f0fd5
📒 Files selected for processing (9)
.github/workflows/golden-pr-evals.ymlproducts/tasks/evals/golden_prs/README.mdproducts/tasks/evals/golden_prs/__main__.pyproducts/tasks/evals/golden_prs/agents.pyproducts/tasks/evals/golden_prs/cases.pyproducts/tasks/evals/golden_prs/golden_prs.jsonproducts/tasks/evals/golden_prs/scoring.pyproducts/tasks/evals/golden_prs/test_golden_prs.pyproducts/tasks/evals/golden_prs/workspace.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review.
| workflow_dispatch: | ||
| inputs: |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
[must_fix] Restrict provider secrets to trusted workflow refs
Issue description
This workflow can be dispatched against a non-default branch, and GitHub runs the workflow version from that ref. A modified workflow on that branch could read and exfiltrate the repository secrets used by the evaluation, even before it starts the agent. The workflow_dispatch branch selector and ref behavior make this a separate exposure path from the agent prompt. (docs.github.com)
Why we think it's a valid issue
- Checked: Reviewed
.github/workflows/golden-pr-evals.ymlfrom dispatch through the evaluation job, including its secret checks and provider-key use. - Found: The workflow allows
workflow_dispatchat.github/workflows/golden-pr-evals.yml:11. Theevaluatejob reads repository secrets at.github/workflows/golden-pr-evals.yml:92-93and passes both provider keys to the eval step at.github/workflows/golden-pr-evals.yml:153-154. The job does not declare a protected environment. - Impact: A modified workflow on a dispatched ref can change its steps and access the repository secrets.
persist-credentials: falsedoes not restrict access to these explicitly injected provider keys. Keeping the keys only as environment secrets, with deployment limited to trusted refs, prevents a branch from accessing them without the required environment binding and approval. - Priority:
must_fixis appropriate because this exposes paid provider credentials to workflow code on an untrusted ref.
Suggested fix
Store the provider keys in a protected GitHub environment that only allows the trusted default branch, and bind the evaluate job to that environment. Add required approval if appropriate. A guard in this workflow alone is not sufficient because a modified branch can remove it.
Prompt to fix with AI (copy-paste)
## Context
@.github/workflows/golden-pr-evals.yml#L11-12
<issue_description>
This workflow can be dispatched against a non-default branch, and GitHub runs the workflow version from that ref. A modified workflow on that branch could read and exfiltrate the repository secrets used by the evaluation, even before it starts the agent. The `workflow_dispatch` branch selector and ref behavior make this a separate exposure path from the agent prompt. ([docs.github.com](https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows?utm_source=openai))
</issue_description>
<issue_validation>
- **Checked:** Reviewed `.github/workflows/golden-pr-evals.yml` from dispatch through the evaluation job, including its secret checks and provider-key use.
- **Found:** The workflow allows `workflow_dispatch` at `.github/workflows/golden-pr-evals.yml:11`. The `evaluate` job reads repository secrets at `.github/workflows/golden-pr-evals.yml:92-93` and passes both provider keys to the eval step at `.github/workflows/golden-pr-evals.yml:153-154`. The job does not declare a protected environment.
- **Impact:** A modified workflow on a dispatched ref can change its steps and access the repository secrets. `persist-credentials: false` does not restrict access to these explicitly injected provider keys. Keeping the keys only as environment secrets, with deployment limited to trusted refs, prevents a branch from accessing them without the required environment binding and approval.
- **Priority:** `must_fix` is appropriate because this exposes paid provider credentials to workflow code on an untrusted ref.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Store the provider keys in a protected GitHub environment that only allows the trusted default branch, and bind the `evaluate` job to that environment. Add required approval if appropriate. A guard in this workflow alone is not sufficient because a modified branch can remove it.
</potential_solution>
| npm install -g @openai/codex | ||
| else | ||
| npm install -g @anthropic-ai/claude-code |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
[should_fix] Pin the agent CLI before exposing credentials
Issue description
These commands install the latest CLI version on every run. The eval later gives the selected provider key to the agent process. If a registry release is compromised or changes unexpectedly, the installed CLI can read or send that key. This also makes runs less reproducible.
Why we think it's a valid issue
- Checked: Traced the CLI installation at
.github/workflows/golden-pr-evals.yml:128-138to the later agent launch and environment filtering inproducts/tasks/evals/golden_prs/agents.py:39-45. - Found: Both
npm install -gcommands omit a version, so each run installs the package version currently selected by the registry. The launched CLI receives the selected provider key throughagent_environment; only the other provider key is removed. - Impact: A compromised or malicious newly published CLI version can access and send the provider key when the agent starts. Pinning the CLI version makes upgrades intentional and reduces this exposure. The issue also affects reproducibility because runs can use different CLI versions.
Suggested fix
Install an explicitly reviewed CLI version instead of the unqualified latest version. Keep version updates intentional and controlled.
Prompt to fix with AI (copy-paste)
## Context
@.github/workflows/golden-pr-evals.yml#L134-136
<issue_description>
These commands install the latest CLI version on every run. The eval later gives the selected provider key to the agent process. If a registry release is compromised or changes unexpectedly, the installed CLI can read or send that key. This also makes runs less reproducible.
</issue_description>
<issue_validation>
- **Checked:** Traced the CLI installation at `.github/workflows/golden-pr-evals.yml:128-138` to the later agent launch and environment filtering in `products/tasks/evals/golden_prs/agents.py:39-45`.
- **Found:** Both `npm install -g` commands omit a version, so each run installs the package version currently selected by the registry. The launched CLI receives the selected provider key through `agent_environment`; only the other provider key is removed.
- **Impact:** A compromised or malicious newly published CLI version can access and send the provider key when the agent starts. Pinning the CLI version makes upgrades intentional and reduces this exposure. The issue also affects reproducibility because runs can use different CLI versions.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Install an explicitly reviewed CLI version instead of the unqualified latest version. Keep version updates intentional and controlled.
</potential_solution>
| def agent_failure(run: AgentRun) -> str | None: | ||
| """Why the agent exited non-zero, so a login or network failure never reads as a bad attempt.""" | ||
| if run.exit_code == 0: | ||
| return None | ||
| if run.timed_out: | ||
| return "The agent hit the case timeout." | ||
| report = _claude_report(run) | ||
| if report.get("is_error") and report.get("result"): | ||
| return str(report["result"]) | ||
| stderr_lines = [line for line in run.stderr.splitlines() if line.strip()] | ||
| return stderr_lines[-1] if stderr_lines else f"The agent exited with code {run.exit_code}." |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
Report failures when the agent leaves a partial diff
Issue description
When the agent exits with an error after changing files, agent_failure() returns a failure reason, but the runner uses it only when the candidate diff is empty. For a non-empty diff, the run is judged and the failure reason is discarded. The report can then show a normal judge score without explaining that the agent exited unsuccessfully; it only marks timeouts explicitly.
Why we think it's a valid issue
- Checked: Traced
agent_failure()throughverdict_for(),CaseResult, andreport()inproducts/tasks/evals/golden_prs/agents.pyandproducts/tasks/evals/golden_prs/__main__.py. - Found:
verdict_for()uses the failure reason only when the candidate diff is empty; otherwise it returns the judge verdict (products/tasks/evals/golden_prs/__main__.py:41-45).CaseResultrecordsexit_codeandtimed_out, but not the failure reason (products/tasks/evals/golden_prs/__main__.py:20-38). The report marks timeouts, but does not show other non-zero exits or their reasons (products/tasks/evals/golden_prs/__main__.py:96-100). - Found: The existing failure test covers a non-zero exit only with an empty diff, so it does not catch this reporting gap (
products/tasks/evals/golden_prs/test_golden_prs.py:175-179). - Impact: A non-zero exit after producing a partial diff receives a judge score, while the report gives readers no indication that the agent run failed. This can make an incomplete attempt look like a completed one in the evaluation summary.
Suggested fix
Preserve the failure reason in the case result and show it in the report for every non-zero exit. The judge can still score a partial diff, but mark that result as a failed run so readers can distinguish it from a completed attempt.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/agents.py#L68-78
<issue_description>
When the agent exits with an error after changing files, `agent_failure()` returns a failure reason, but the runner uses it only when the candidate diff is empty. For a non-empty diff, the run is judged and the failure reason is discarded. The report can then show a normal judge score without explaining that the agent exited unsuccessfully; it only marks timeouts explicitly.
</issue_description>
<issue_validation>
- **Checked:** Traced `agent_failure()` through `verdict_for()`, `CaseResult`, and `report()` in `products/tasks/evals/golden_prs/agents.py` and `products/tasks/evals/golden_prs/__main__.py`.
- **Found:** `verdict_for()` uses the failure reason only when the candidate diff is empty; otherwise it returns the judge verdict (`products/tasks/evals/golden_prs/__main__.py:41-45`). `CaseResult` records `exit_code` and `timed_out`, but not the failure reason (`products/tasks/evals/golden_prs/__main__.py:20-38`). The report marks timeouts, but does not show other non-zero exits or their reasons (`products/tasks/evals/golden_prs/__main__.py:96-100`).
- **Found:** The existing failure test covers a non-zero exit only with an empty diff, so it does not catch this reporting gap (`products/tasks/evals/golden_prs/test_golden_prs.py:175-179`).
- **Impact:** A non-zero exit after producing a partial diff receives a judge score, while the report gives readers no indication that the agent run failed. This can make an incomplete attempt look like a completed one in the evaluation summary.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Preserve the failure reason in the case result and show it in the report for every non-zero exit. The judge can still score a partial diff, but mark that result as a failed run so readers can distinguish it from a completed attempt.
</potential_solution>
| - uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7.0.0 | ||
| with: | ||
| pattern: golden-pr-* | ||
| path: ${{ env.RESULTS_DIR }} | ||
| merge-multiple: true |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
[should_fix] Mark incomplete score summaries
Issue description
If an eval fails before it writes a result file, its artifact can be empty while other PR artifacts still download. The report then calculates scores and the mean from only the PRs that produced results, without marking the table as incomplete. This can make model comparisons misleading.
Why we think it's a valid issue
- Checked: Traced PR selection, matrix execution, artifact upload and report generation in
.github/workflows/golden-pr-evals.yml, plus result loading and mean calculation inproducts/tasks/evals/golden_prs/__main__.py. - Found:
.github/workflows/golden-pr-evals.yml:48-49exposes the selected PR list fromplan, but the report job at.github/workflows/golden-pr-evals.yml:191-195depends only onevaluate. The upload step at.github/workflows/golden-pr-evals.yml:183-189runs after failure and warns when no result files exist. - Found:
products/tasks/evals/golden_prs/__main__.py:88-89loads only JSON files that exist. Atproducts/tasks/evals/golden_prs/__main__.py:103-105,reportcalculates means across those loaded results without checking them against the selected PRs. - Impact: When at least one PR produces a result and another does not, the run summary presents a mean over only the available results without indicating that the selected set is incomplete. This can mislead model comparisons, so the finding meets the bar for a real reporting correctness issue.
Suggested fix
Pass the selected PR list to the report job and compare it with the downloaded results. Mark missing PRs as failed or label the summary as incomplete so readers do not treat a partial mean as a complete run.
Prompt to fix with AI (copy-paste)
## Context
@.github/workflows/golden-pr-evals.yml#L212-216
<issue_description>
If an eval fails before it writes a result file, its artifact can be empty while other PR artifacts still download. The report then calculates scores and the mean from only the PRs that produced results, without marking the table as incomplete. This can make model comparisons misleading.
</issue_description>
<issue_validation>
- **Checked:** Traced PR selection, matrix execution, artifact upload and report generation in `.github/workflows/golden-pr-evals.yml`, plus result loading and mean calculation in `products/tasks/evals/golden_prs/__main__.py`.
- **Found:** `.github/workflows/golden-pr-evals.yml:48-49` exposes the selected PR list from `plan`, but the report job at `.github/workflows/golden-pr-evals.yml:191-195` depends only on `evaluate`. The upload step at `.github/workflows/golden-pr-evals.yml:183-189` runs after failure and warns when no result files exist.
- **Found:** `products/tasks/evals/golden_prs/__main__.py:88-89` loads only JSON files that exist. At `products/tasks/evals/golden_prs/__main__.py:103-105`, `report` calculates means across those loaded results without checking them against the selected PRs.
- **Impact:** When at least one PR produces a result and another does not, the run summary presents a mean over only the available results without indicating that the selected set is incomplete. This can mislead model comparisons, so the finding meets the bar for a real reporting correctness issue.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Pass the selected PR list to the report job and compare it with the downloaded results. Mark missing PRs as failed or label the summary as incomplete so readers do not treat a partial mean as a complete run.
</potential_solution>
| return 0 | ||
| selected = select_golden_prs(golden_prs, args.pr) if args.pr else golden_prs | ||
| model = args.model or DEFAULT_MODELS[args.runtime] | ||
| results_dir = args.results_dir / f"{datetime.now(UTC):%Y%m%dT%H%M%S}-{args.runtime}-{model}" |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
[should_fix] Prevent model input from escaping the results directory
Issue description
The workflow accepts inputs.model as an unrestricted string and passes it here. A model value containing path separators and .. can make results_dir resolve outside args.results_dir. write_result() then writes the PR result files at that location, so an input can overwrite files on the runner instead of only creating evaluation artifacts.
Why we think it's a valid issue
- Checked: Traced
--modelthroughparse_args()andmain(), and checked the workflow input. The CLI accepts any model string atproducts/tasks/evals/golden_prs/__main__.py:125, and the workflow declaresmodelas an unrestricted string at.github/workflows/golden-pr-evals.yml:20-23. - Found:
products/tasks/evals/golden_prs/__main__.py:147interpolates the model intoresults_dirwithout validation. A model containing enough/..components makes the resulting path resolve outsideargs.results_dir.write_result()creates that directory and writes three files there atproducts/tasks/evals/golden_prs/__main__.py:81-85. - Impact: A reachable workflow dispatch or local CLI run can write evaluation result files outside the configured results directory. This is a path traversal security issue, so the suggested change is worth addressing.
Suggested fix
Keep the supplied model value for the agent and result metadata, but use a sanitized slug or fixed run ID for the directory name. Also verify the resolved path stays inside the configured results directory.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/__main__.py#L147
<issue_description>
The workflow accepts `inputs.model` as an unrestricted string and passes it here. A model value containing path separators and `..` can make `results_dir` resolve outside `args.results_dir`. `write_result()` then writes the PR result files at that location, so an input can overwrite files on the runner instead of only creating evaluation artifacts.
</issue_description>
<issue_validation>
- **Checked:** Traced `--model` through `parse_args()` and `main()`, and checked the workflow input. The CLI accepts any model string at `products/tasks/evals/golden_prs/__main__.py:125`, and the workflow declares `model` as an unrestricted string at `.github/workflows/golden-pr-evals.yml:20-23`.
- **Found:** `products/tasks/evals/golden_prs/__main__.py:147` interpolates the model into `results_dir` without validation. A model containing enough `/..` components makes the resulting path resolve outside `args.results_dir`. `write_result()` creates that directory and writes three files there at `products/tasks/evals/golden_prs/__main__.py:81-85`.
- **Impact:** A reachable workflow dispatch or local CLI run can write evaluation result files outside the configured results directory. This is a path traversal security issue, so the suggested change is worth addressing.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Keep the supplied model value for the agent and result metadata, but use a sanitized slug or fixed run ID for the directory name. Also verify the resolved path stays inside the configured results directory.
</potential_solution>
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
products/tasks/evals/golden_prs/workspace.py-89-92 (1)
89-92: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winParse Git tree paths with NUL delimiters.
If an export-ignored parent path contains characters that Git quotes,
ls-treereturns the escaped path. The restore loop then passes that escaped path togit show, which can raiseCalledProcessErrorand abortcheckout_parent.Suggested fix
- entries = _git(repo, "ls-tree", "-r", ref).stdout.splitlines() + entries = _git(repo, "ls-tree", "-r", "-z", ref).stdout.rstrip("\0").split("\0")
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: PostHog/posthog/.coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: ac84d0c0-2a05-47cb-8ffa-5ce4f5d673fb
📒 Files selected for processing (3)
.github/workflows/golden-pr-evals.ymlproducts/tasks/evals/golden_prs/agents.pyproducts/tasks/evals/golden_prs/workspace.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 6 remain after this review.
| _git(workdir, "add", "-A", deny_secrets=True) | ||
| baseline = _git(workdir, "rev-parse", "--verify", "--quiet", BASELINE_REF, check=False) | ||
| if baseline.returncode == 0: | ||
| return _diff(workdir, "--cached", BASELINE_REF) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,170p' products/tasks/evals/golden_prs/workspace.py
sed -n '1,120p' products/tasks/evals/golden_prs/agents.py
sed -n '40,80p' products/tasks/evals/golden_prs/__main__.py
sed -n '20,30p' products/tasks/evals/golden_prs/README.mdRepository: PostHog/posthog
Length of output: 13954
Sensitive Data Exposure
Reachability: External
Exploitability: Difficult
CWE: CWE-522 — Insufficiently Protected Credentials
Sanitize the candidate diff subprocess.
candidate_diff removes secrets only from git add; _diff uses the default environment and permits diff.external and textconv. An agent can write .git/config, causing Git to execute an agent-selected command with evaluator-held keys, such as ANTHROPIC_API_KEY during Codex runs. The command can also emit fabricated diff output and corrupt scoring. The runner is already unsandboxed with a documented soft boundary, so critical severity is overstated; this remains a major security issue.
Suggested fix
- return _git(cwd, "diff", "--src-prefix=a/", "--dst-prefix=b/", *args).stdout
+ return _git(
+ cwd,
+ "diff",
+ "--no-ext-diff",
+ "--no-textconv",
+ "--src-prefix=a/",
+ "--dst-prefix=b/",
+ *args,
+ deny_secrets=True,
+ ).stdout![stamphog[bot]](https://avatars.githubusercontent.com/in/3000332?s=60&v=4){kind=small}
There was a problem hiding this comment.
Not approved yet — waiting on the conditions below.
Re-add the stamphog label to request another review once you have addressed this.
This pull request was refused automatically before any code review took place.
Deny-list gate — FAILED (infra_cicd): the new workflow file .github/workflows/golden-pr-evals.yml matches a path pattern that stamphog never auto-clears, regardless of content.
Size gate — FAILED: 997 substantive lines across 11 files (ceiling is 800), rising to 1300 lines across 13 files including docs and generated snapshots.
Tier gate — FAILED: classified as T2-never, since it spans two distinct areas (CI infra and product code under products/tasks/evals) as a feature-sized change.
Because CI/infra changes are excluded from auto-review, this pull request needs a human reviewer regardless of size; splitting the CI workflow from the products/tasks/evals code would also help future auto-review, but a human must look at the workflow file either way.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✗ | matches: infra_cicd |
| size | ✗ | too large for auto-review (997L substantive in global — ceiling is 800L; 997L, 11F total, 2 binary; 1300L/13F incl. docs/generated/snapshots) |
| tier | ✗ | classified as T2-never: T2-never (1300L, 13F, two-areas, feat) |
| stamphog 2.2.0 | .stamphog/policy.yml @ unknown · reviewed head b915476 |
| return 0 | ||
| selected = select_golden_prs(golden_prs, args.pr) if args.pr else golden_prs | ||
| model = args.model or DEFAULT_MODELS[args.runtime] | ||
| results_dir = args.results_dir / f"{datetime.now(UTC):%Y%m%dT%H%M%S}-{args.runtime}-{model}" |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
Use a unique directory for each run
Issue description
The run directory uses a timestamp with only second-level precision. Two runs started in the same second with the same runtime and model use the same directory. Their files for matching PR numbers then overwrite each other, which can lose results or mix artifacts from different runs.
Why we think it's a valid issue
- Checked: Read the full run flow and the workflow’s matrix-job setup.
main()creates the run directory before evaluating cases, andwrite_result()writes files named by PR number. - Found:
products/tasks/evals/golden_prs/__main__.py:147builds the directory from a second-resolution timestamp, runtime, and model.products/tasks/evals/golden_prs/__main__.py:82allows an existing directory, and lines 83–85 overwrite the same PR’s result, diff, and log files. The workflow uses separate matrix jobs, but concurrent local runs can share the default results directory. - Impact: Concurrent local runs with matching directory components and an overlapping PR can replace each other’s saved results. This is a concrete data-loss risk, so the finding meets the bar.
Suggested fix
Add a unique run identifier, such as a UUID, to the directory name and create the directory exclusively so each run keeps its own results.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/__main__.py#L147
<issue_description>
The run directory uses a timestamp with only second-level precision. Two runs started in the same second with the same runtime and model use the same directory. Their files for matching PR numbers then overwrite each other, which can lose results or mix artifacts from different runs.
</issue_description>
<issue_validation>
- **Checked:** Read the full run flow and the workflow’s matrix-job setup. `main()` creates the run directory before evaluating cases, and `write_result()` writes files named by PR number.
- **Found:** `products/tasks/evals/golden_prs/__main__.py:147` builds the directory from a second-resolution timestamp, runtime, and model. `products/tasks/evals/golden_prs/__main__.py:82` allows an existing directory, and lines 83–85 overwrite the same PR’s result, diff, and log files. The workflow uses separate matrix jobs, but concurrent local runs can share the default results directory.
- **Impact:** Concurrent local runs with matching directory components and an overlapping PR can replace each other’s saved results. This is a concrete data-loss risk, so the finding meets the bar.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Add a unique run identifier, such as a UUID, to the directory name and create the directory exclusively so each run keeps its own results.
</potential_solution>
| import json, os | ||
| golden = [entry["number"] for entry in json.load(open("products/tasks/evals/golden_prs/golden_prs.json"))] | ||
| wanted = os.environ["PRS"].strip() | ||
| selected = golden if wanted == "all" else [int(n) for n in wanted.split(",") if n.strip()] |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
[should_fix] Reject an empty PR selection
Issue description
A blank or comma-only prs input produces an empty list. The evaluate job then skips, but the report job still runs and has no results to summarize. This can make an invalid dispatch look like a completed run with no scores.
Why we think it's a valid issue
- Checked:
.github/workflows/golden-pr-evals.yml:63-74filters blank PR tokens but does not reject an empty selection. The plan step can therefore output[]. - Found:
.github/workflows/golden-pr-evals.yml:79-80skipsevaluatefor[], butreportat.github/workflows/golden-pr-evals.yml:191-195has no corresponding selection guard.products/tasks/evals/golden_prs/__main__.py:92-94returnsNo results found.\nfor empty results, andproducts/tasks/evals/golden_prs/__main__.py:142-144exits successfully after printing it. - Impact: A blank or comma-only dispatch can finish successfully without scores, so the run does not clearly signal that no PRs were evaluated. Rejecting the empty selection or skipping the report addresses this reachable input case.
Suggested fix
Fail the plan step with a clear error when selected is empty, or explicitly skip the report job when no PRs were selected.
Prompt to fix with AI (copy-paste)
## Context
@.github/workflows/golden-pr-evals.yml#L67
<issue_description>
A blank or comma-only `prs` input produces an empty list. The evaluate job then skips, but the report job still runs and has no results to summarize. This can make an invalid dispatch look like a completed run with no scores.
</issue_description>
<issue_validation>
- **Checked:** `.github/workflows/golden-pr-evals.yml:63-74` filters blank PR tokens but does not reject an empty selection. The plan step can therefore output `[]`.
- **Found:** `.github/workflows/golden-pr-evals.yml:79-80` skips `evaluate` for `[]`, but `report` at `.github/workflows/golden-pr-evals.yml:191-195` has no corresponding selection guard. `products/tasks/evals/golden_prs/__main__.py:92-94` returns `No results found.\n` for empty results, and `products/tasks/evals/golden_prs/__main__.py:142-144` exits successfully after printing it.
- **Impact:** A blank or comma-only dispatch can finish successfully without scores, so the run does not clearly signal that no PRs were evaluated. Rejecting the empty selection or skipping the report addresses this reachable input case.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Fail the plan step with a clear error when `selected` is empty, or explicitly skip the report job when no PRs were selected.
</potential_solution>
| baseline = _git(workdir, "rev-parse", "--verify", "--quiet", BASELINE_REF, check=False) | ||
| if baseline.returncode == 0: | ||
| return _diff(workdir, "--cached", BASELINE_REF) |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
Keep the baseline identity outside agent-writable refs
Issue description
The agent can run git update-ref refs/golden-eval/baseline <candidate-commit> inside its writable checkout. Since candidate_diff() resolves this ref after the agent exits, it can compare against the agent’s commit instead of the original baseline and produce misleading scores.
Why we think it's a valid issue
- Checked: Traced
evaluateinproducts/tasks/evals/golden_prs/__main__.py:55-57and the agent command inproducts/tasks/evals/golden_prs/agents.py:27-34; the agent runs in the writable checkout with permission and sandbox bypasses beforecandidate_diffruns. - Found:
products/tasks/evals/golden_prs/workspace.py:154stores the baseline in a normal writable Git ref.products/tasks/evals/golden_prs/workspace.py:162-164resolves and uses that ref after the agent exits. The agent can move it withgit update-ref. - Impact: If the agent moves the ref to its candidate commit, the diff can omit its changes and produce incorrect evaluation scores. Passing the baseline SHA captured before the agent starts avoids trusting an agent-writable ref.
Suggested fix
Capture the baseline commit SHA before starting the agent and pass that SHA to candidate_diff() instead of resolving a ref the agent can move.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/workspace.py#L162-164
<issue_description>
The agent can run `git update-ref refs/golden-eval/baseline <candidate-commit>` inside its writable checkout. Since `candidate_diff()` resolves this ref after the agent exits, it can compare against the agent’s commit instead of the original baseline and produce misleading scores.
</issue_description>
<issue_validation>
- **Checked:** Traced `evaluate` in `products/tasks/evals/golden_prs/__main__.py:55-57` and the agent command in `products/tasks/evals/golden_prs/agents.py:27-34`; the agent runs in the writable checkout with permission and sandbox bypasses before `candidate_diff` runs.
- **Found:** `products/tasks/evals/golden_prs/workspace.py:154` stores the baseline in a normal writable Git ref. `products/tasks/evals/golden_prs/workspace.py:162-164` resolves and uses that ref after the agent exits. The agent can move it with `git update-ref`.
- **Impact:** If the agent moves the ref to its candidate commit, the diff can omit its changes and produce incorrect evaluation scores. Passing the baseline SHA captured before the agent starts avoids trusting an agent-writable ref.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Capture the baseline commit SHA before starting the agent and pass that SHA to `candidate_diff()` instead of resolving a ref the agent can move.
</potential_solution>
Score how well a coding agent one-shots a human-written PostHog PR from 2023 or 2024. The runner checks out the commit before the PR, prompts the agent with the PR description, and compares the diff with the merged PR. A workflow_dispatch workflow runs it on demand, one job per PR. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 5eb47e0a-d342-4b0d-9c45-839c78360e9a
…tempt A dry run with a logged-out CLI scored 0 with "the agent changed no files", which reads like the model failed the task. The result now carries the CLI's own error, and the judge is not called for a run that failed before changing anything. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Generated-By: PostHog Desktop Task-Id: 5eb47e0a-d342-4b0d-9c45-839c78360e9a
The judge falls back to the signed-in claude CLI when ANTHROPIC_API_KEY is not set, so a devbox does not need an API key. The candidate and golden diffs force a/ and b/ prefixes, because a host diff.mnemonicPrefix or diff.noprefix setting made the scorer miss every file. The agent workspace gets back the files that git archive drops as export-ignore, because without .gitignore the agent's build output was staged into the candidate diff. Generated-By: PostHog Desktop Task-Id: 334f773a-ab58-4546-98cf-39d3f79448cc
Keep the golden PR number out of the agent workspace, drop the other provider key and GitHub tokens from the agent environment, bound every subprocess with a timeout, restore directory-level export-ignored files, validate the case timeout input, and strip tracker references from the fixture PR bodies. Generated-By: PostHog Desktop Task-Id: 334f773a-ab58-4546-98cf-39d3f79448cc
Generated-By: PostHog Desktop Task-Id: 334f773a-ab58-4546-98cf-39d3f79448cc
Probe the agent CLI version before the run, bound the archive extraction, keep ignored tracked files and symlinks in the baseline, score the candidate diff against a fixed baseline ref, withhold secrets from the staging git call, and parse the case timeout as base 10. Generated-By: PostHog Desktop Task-Id: 334f773a-ab58-4546-98cf-39d3f79448cc
Generated-By: PostHog Desktop Task-Id: 334f773a-ab58-4546-98cf-39d3f79448cc
b915476 to
f6eceeb
Compare
| result, candidate, agent_log = evaluate(pr, args.runtime, model, args.judge_model, args.case_timeout, args.repo) | ||
| write_result(results_dir, result, candidate, agent_log) |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
Continue the batch when one case fails
Issue description
The CLI runs selected PRs in a serial loop, but an exception from evaluate() or write_result() exits the loop. In the default run, one case-specific failure can prevent every later PR from being evaluated.
Why we think it's a valid issue
- Checked: Read
main()and the evaluation and result-writing paths inproducts/tasks/evals/golden_prs/__main__.py, plus the local-run instructions inproducts/tasks/evals/golden_prs/README.md. - Found:
main()callsevaluate()andwrite_result()directly inside the loop atproducts/tasks/evals/golden_prs/__main__.py:148-151, with no per-case exception handling. Those operations can raise: for example,ensure_golden_commits()runs a checked Git fetch atproducts/tasks/evals/golden_prs/workspace.py:64-69, andcheckout_parent()raises when archive or extraction fails atproducts/tasks/evals/golden_prs/workspace.py:135-142. - Impact: The README documents running all PRs locally by omitting
--pratproducts/tasks/evals/golden_prs/README.md:36-38. If one case raises, the process exits before evaluating later cases or printing the final report atproducts/tasks/evals/golden_prs/__main__.py:153-155. Per-case failure reporting and a non-zero final status would preserve the batch results while still signaling failure.
Suggested fix
Handle failures per PR so the loop can continue, and include each failed case in the final summary with its error. Return a non-zero exit status after the batch if any case failed.
Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/__main__.py#L150-151
<issue_description>
The CLI runs selected PRs in a serial loop, but an exception from `evaluate()` or `write_result()` exits the loop. In the default run, one case-specific failure can prevent every later PR from being evaluated.
</issue_description>
<issue_validation>
- **Checked:** Read `main()` and the evaluation and result-writing paths in `products/tasks/evals/golden_prs/__main__.py`, plus the local-run instructions in `products/tasks/evals/golden_prs/README.md`.
- **Found:** `main()` calls `evaluate()` and `write_result()` directly inside the loop at `products/tasks/evals/golden_prs/__main__.py:148-151`, with no per-case exception handling. Those operations can raise: for example, `ensure_golden_commits()` runs a checked Git fetch at `products/tasks/evals/golden_prs/workspace.py:64-69`, and `checkout_parent()` raises when archive or extraction fails at `products/tasks/evals/golden_prs/workspace.py:135-142`.
- **Impact:** The README documents running all PRs locally by omitting `--pr` at `products/tasks/evals/golden_prs/README.md:36-38`. If one case raises, the process exits before evaluating later cases or printing the final report at `products/tasks/evals/golden_prs/__main__.py:153-155`. Per-case failure reporting and a non-zero final status would preserve the batch results while still signaling failure.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Handle failures per PR so the loop can continue, and include each failed case in the final summary with its error. Return a non-zero exit status after the batch if any case failed.
</potential_solution>
| ${MODEL:+--model "$MODEL"} \ | ||
| --judge-model "$JUDGE_MODEL" \ | ||
| --case-timeout "$((10#$CASE_TIMEOUT_MINUTES * 60))" \ | ||
| --results-dir "$RESULTS_DIR" |
There was a problem hiding this comment.
FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
[should_fix] Keep agent-written files out of score results
Issue description
The agent inherits RESULTS_DIR and runs without a filesystem sandbox. It can write a fabricated .json file under this directory while it works. The runner later loads every JSON file recursively from the same directory, so that file can add fake scores to the report and distort the mean.
Why we think it's a valid issue
- Checked: Traced
RESULTS_DIRfrom.github/workflows/golden-pr-evals.ymlthroughrun_agentand the artifact/report steps. - Found: The workflow passes
RESULTS_DIRto the eval command at.github/workflows/golden-pr-evals.yml:181.agent_environmentremoves only GitHub credentials and the other provider key, so the agent inheritsRESULTS_DIR(products/tasks/evals/golden_prs/agents.py:39-41). The workflow uploads that directory (.github/workflows/golden-pr-evals.yml:183-189), then downloads the artifacts into it and runs the report (.github/workflows/golden-pr-evals.yml:212-229).load_resultsreads every*.jsonrecursively (products/tasks/evals/golden_prs/__main__.py:88-89), andreportincludes every loaded result in the rows and means (products/tasks/evals/golden_prs/__main__.py:92-108). - Impact: An agent-written JSON file can be included in the uploaded artifacts and loaded by the report job, adding a fabricated score to the table and changing its means. This is a reachable score-integrity bug, so the finding meets the bar.
Suggested fix
Isolate the agent from the evaluator's result directory, then have the report read only the expected result files for the selected PRs. A separate container or user for the agent provides a stronger boundary than hiding the path from its environment.
Prompt to fix with AI (copy-paste)
## Context
@.github/workflows/golden-pr-evals.yml#L181
<issue_description>
The agent inherits `RESULTS_DIR` and runs without a filesystem sandbox. It can write a fabricated `.json` file under this directory while it works. The runner later loads every JSON file recursively from the same directory, so that file can add fake scores to the report and distort the mean.
</issue_description>
<issue_validation>
- **Checked:** Traced `RESULTS_DIR` from `.github/workflows/golden-pr-evals.yml` through `run_agent` and the artifact/report steps.
- **Found:** The workflow passes `RESULTS_DIR` to the eval command at `.github/workflows/golden-pr-evals.yml:181`. `agent_environment` removes only GitHub credentials and the other provider key, so the agent inherits `RESULTS_DIR` (`products/tasks/evals/golden_prs/agents.py:39-41`). The workflow uploads that directory (`.github/workflows/golden-pr-evals.yml:183-189`), then downloads the artifacts into it and runs the report (`.github/workflows/golden-pr-evals.yml:212-229`). `load_results` reads every `*.json` recursively (`products/tasks/evals/golden_prs/__main__.py:88-89`), and `report` includes every loaded result in the rows and means (`products/tasks/evals/golden_prs/__main__.py:92-108`).
- **Impact:** An agent-written JSON file can be included in the uploaded artifacts and loaded by the report job, adding a fabricated score to the table and changing its means. This is a reachable score-integrity bug, so the finding meets the bar.
</issue_validation>
## Task
Investigate the issue and solve it
<potential_solution>
Isolate the agent from the evaluator's result directory, then have the report read only the expected result files for the selected PRs. A separate container or user for the agent provides a stronger boundary than hiding the path from its environment.
</potential_solution>
This branch has not been deployed
1 participant
Problem
Changes
claudeorcodex), a model, and PR numbers, and read a score table in the run summary.python -m products.tasks.evals.golden_prs run --pr 25832. Results, diffs and agent logs land in a gitignored results directory.ANTHROPIC_API_KEYis set, and the signed-inclaudeCLI otherwise, so a devbox needs no API key.git archivedrops as export-ignore, including every.gitignore, so the agent's build output stays out of the candidate diff.README.mdshows how to add one.@dataclass(frozen=True, kw_only=True, slots=True)rather thanposthog.dataclasses.frozen, whose package imports Django on load.flowchart LR D([workflow_dispatch]) --> P[plan: select PRs] P --> E1[evaluate PR a] P --> E2[evaluate PR b] E1 --> R[report: score table] E2 --> R E1 --> A[(diff, agent log, scores)] E2 --> A classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff; classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000; classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000; class E1,E2 phBlue; class D,R phYellow; class A,P phGray;Note
The deterministic scores are strict. A correct change written a different way scores low on them, so read them together with the judge score and its reasoning. The six runs show three gaps to fix next: line F1 ignores deleted lines, files hit punishes a different file layout, and one judge sample per case is not stable when the approach differs from the golden one. Running the golden PR's own tests against the candidate is a possible next step, but needs the full stack at a 2023 commit and is out of scope here.
How did you test this code?
test_golden_prs.py: parameterized tests for diff parsing (rename, snapshot and image exclusion), the overlap scores (identical, empty, wrong files, partial, extra file), prompt cleaning of PR template noise, the judge short-circuit on an empty diff, credential stripping, and the report table. They also check every golden set entry is complete and from the four authors.claudeandcodexCLIs, judged by claude-opus-5 through the CLI fallback. Tables, one line per PR, and the scores that look wrong are in the first results comment (claude-opus-5, gpt-5.5) and the second (claude-opus-5-5, claude-fable-5-1, gpt-6-sol, gpt-6-astra).workflow_dispatchonly lists a workflow that exists onmaster, so the first dispatch happens after merge.hogli lint:workflows,actionlint,hogli ci:plan, andhogli product:lint taskspass locally.Release status
Automatic notifications
Docs update
None. The runner's README covers usage.
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Claude Fable 5.1
Skills invoked: /writing-evals, /authoring-ci-workflows, /writing-tests, /writing-dataclasses, /writing-code-comments, /writing-user-facing-copy, /writing-pr-descriptions, /claude-api.
Later sessions on a Mac added the agent failure reporting, the CLI judge fallback, forced
a/andb/diff prefixes (a hostdiff.mnemonicPrefixmade the scorer miss every file), and the export-ignore restore, then ran the six model comparisons in the comments.Decisions: the runner is a standalone package rather than a suite in
products/posthog_ai/eval_harness, because that harness boots the test database, live server, LLM gateway, MCP server and Temporal, none of which this eval uses. The isolated checkout usesgit archiveplus git plumbing (write-tree,commit-tree) rather than a worktree orgit commit, so shared objects and host commit hooks stay out of the agent's way. No open PR covers this;gh pr list --searchfound nothing.Created with PostHog Desktop
🤖 Generated with Claude Code