brooktrails/pr-reviewer
Watch
1
0
Fork
You've already forked pr-reviewer
 0
Code Issues 2 Pull requests Projects Releases Packages Wiki Activity Actions

feat: store review-pass model input and show it on the comparison page #79

Merged
rcsheets merged 1 commit from feat/review-prompt-storage into main 2026-07-23 11:52:21 +00:00
Conversation 3 Commits 1 Files changed 13 +202 -31
rcsheets commented 2026-07-23 10:15:07 +00:00
Owner
Copy link

Persists the exact input each config's review call sent to the model — resolved system prompt + BuildUserMessage rendering — and surfaces it on /admin/comparisons/{id} as a collapsed Model input section per config.

  • Migration 000011: prompt_system / prompt_user TEXT columns on review_events (default ''; old rows and rows whose review pass never ran read back empty, and the template hides the section)
  • Runner: prompts are rendered at fan-out, before the backend call, so failed runs keep their input too; each config row gets its own copy since system overrides and token budgets diverge per config
  • Reviewer: new ResolveReviewSystemPrompt helper, now used by all three backends' Review methods, so the stored copy cannot drift from what was sent
  • Scope is the final review pass only (not preflight/discovery/extraction sub-passes)

Note: full-tier prompts embed the diff (≤100k chars) and changed-file contents (≤50k/file), so rows grow into the hundreds-of-KB range per config. They TOAST out of line; growth is visible on the new /admin/storage page (#78).

🤖 Generated with Claude Code

Persists the exact input each config's review call sent to the model — resolved system prompt + `BuildUserMessage` rendering — and surfaces it on `/admin/comparisons/{id}` as a collapsed **Model input** section per config. - **Migration 000011**: `prompt_system` / `prompt_user` TEXT columns on `review_events` (default `''`; old rows and rows whose review pass never ran read back empty, and the template hides the section) - **Runner**: prompts are rendered at fan-out, before the backend call, so failed runs keep their input too; each config row gets its own copy since system overrides and token budgets diverge per config - **Reviewer**: new `ResolveReviewSystemPrompt` helper, now used by all three backends' `Review` methods, so the stored copy cannot drift from what was sent - Scope is the final review pass only (not preflight/discovery/extraction sub-passes) Note: full-tier prompts embed the diff (≤100k chars) and changed-file contents (≤50k/file), so rows grow into the hundreds-of-KB range per config. They TOAST out of line; growth is visible on the new `/admin/storage` page (#78). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
feat: store review-pass model input and show it on the comparison page
Some checks failed
ci / check (pull_request) Failing after 3s
Details
4927e4be2b 
Persist the exact input each config's review call sent — the resolved
system prompt and the BuildUserMessage rendering — on review_events
(migration 000011), captured at fan-out so failed calls keep their
input too. The admin comparison page shows it in a collapsed "Model
input" section per config; rows predating the migration hide it.

Backends now resolve their review system prompt through the shared
ResolveReviewSystemPrompt helper so the stored copy cannot drift from
what was sent.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
pr-reviewer-bot commented 2026-07-23 10:15:08 +00:00
Collaborator
Copy link

Automated review by pr-reviewer v0.37.0 | Safety Check | Mistral Small | tracking id r-61e9ac-f8f489
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

✅ Verdict: LGTM — No major issues found.

No obvious bugs, security issues, or accidentally committed files detected.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.37.0 | Safety Check | Mistral Small | tracking id `r-61e9ac-f8f489`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- **✅ Verdict: LGTM** — No major issues found. No obvious bugs, security issues, or accidentally committed files detected.
rcsheets force-pushed feat/review-prompt-storage from 4927e4be2b
Some checks failed
ci / check (pull_request) Failing after 3s
Details
to 08b7c39c93
All checks were successful
ci / check (pull_request) Successful in 45s
Details
2026-07-23 11:46:06 +00:00 Compare
pr-reviewer-bot commented 2026-07-23 11:46:06 +00:00
Collaborator
Copy link

Automated review by pr-reviewer v0.37.0 | Safety Check | Mistral Small | tracking id r-61fefe-bc2a40
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

✅ Verdict: LGTM — No blocking issues found. Changes appear safe.

No obvious bugs, security issues, or accidentally committed files detected. The migration and prompt storage logic are well-contained and the UI changes are appropriately guarded.

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.37.0 | Safety Check | Mistral Small | tracking id `r-61fefe-bc2a40`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- **✅ Verdict: LGTM** — No blocking issues found. Changes appear safe. No obvious bugs, security issues, or accidentally committed files detected. The migration and prompt storage logic are well-contained and the UI changes are appropriately guarded.
pr-reviewer-bot commented 2026-07-23 11:49:53 +00:00
Collaborator
Copy link

Automated review by pr-reviewer v0.38.0 | Full Review | Claude Sonnet 5 | tracking id r-61ffe1-c8a8e9
This is an AI-generated review and may contain mistakes.

Status: ✅ Completed

⚠️ The primary review (Mistral Small) failed because the diff is too large for that model. The review below is from Claude Sonnet 5, which reviewed the same diff.

✅ Verdict: Approve — The prompt-capture design is structurally sound (single resolution helper prevents drift), migration and template changes are safe and well-tested, and the only notable concern — multiplied storage growth per fan-out config — is already acknowledged and cross-referenced to the storage-tracking follow-up.

Overall this is a clean, well-scoped change that does what it says: capture the exact review-pass input at fan-out time and surface it on the comparison page. The design rationale in the migration comments and code comments is unusually thorough, and the approach of centralizing prompt resolution in ResolveReviewSystemPrompt (used by both the backends and the runner) is exactly right — it structurally prevents the stored copy from drifting from what was actually sent, which was the main risk in this kind of feature.

Things I looked at specifically:

Correctness of prompt capture

  • fanOut now renders req via buildReviewRequest, then calls ResolveReviewSystemPrompt(req) and BuildUserMessage(req) before invoking runOne. Since runOne/Review also call these exact same helpers against the identical req value, the stored fields are guaranteed to match the sent fields — no logic duplication that could diverge. Good.
  • recordConfigResult stamps event.PromptSystem/PromptUser unconditionally, including on failure, matching the PR's stated intent ("failed runs keep their input too"). This makes sense since the prompt is rendered before the network call.
  • One subtle nuance worth flagging in review (not necessarily a blocker): if runOne returns an error before the request is actually sent to the network (e.g., "no backend registered for provider" in runOne), the stored prompt is still recorded, even though nothing was actually transmitted. The doc comments already partially acknowledge this ("usually sent"), so this seems like a deliberate, accepted tradeoff rather than an oversight — worth confirming that's intentional but not a blocking issue.

Migration

  • Default '' with backfill-free ADD COLUMN ... DEFAULT '' is the right call for a TEXT column of unknown historical volume — no full-table rewrite needed on modern Postgres since the default is constant, and old/failed rows read back empty cleanly. Down migration is trivial and correct.
  • Comments do a good job flagging that full-tier rows will be substantial (hundreds of KB), and this is cross-referenced against the storage page (#78) rather than solved here, which seems like the right scope boundary for this PR.

Storage / performance concern

  • This is the one area I'd want a comment on, not necessarily a blocker: every full-tier review event will now carry a copy of the diff + file contents + guidance/context fragments per config, duplicated N times for N configs in the fan-out (since "Stored per row, not per comparison group" is explicit in the migration). For an A/B test with e.g. 3 configs, that's 3x the storage per PR review for what's often near-identical content across configs (only system override and token budget differ; the diff/file content struct is shared). The PR description acknowledges the growth is "visible" on the storage page, but visibility isn't mitigation — there's no TTL/pruning story mentioned here or apparently elsewhere. This is fine for a first cut but is likely to need a retention policy sooner rather than later given full-tier prompts can be ~150KB+ each.

Template / dashboard

  • The {{if .PromptUser}} guard correctly hides the whole section when empty; {{if .PromptSystem}} further guards the inner system-prompt block for cases where only user message exists (shouldn't happen given resolution logic always returns non-empty system, but harmless defensive code).
  • No escaping concerns: Go's html/template auto-escapes {{.PromptSystem}}/{{.PromptUser}} by default, so arbitrary diff/file content rendered into an admin-only page is safe from XSS as long as the dashboard continues to use html/template (which it appears to, given render() is templated HTML elsewhere in this file).
  • No indication of any collapse/redaction for potential secrets embedded in file contents that get rendered on the admin page — but this is consistent with existing ReviewBody/diff handling elsewhere in the dashboard, so not a new risk introduced by this PR specifically, and the page is presumably gated by RequireAdmin.

Test coverage

  • compare_render_test.go covers both the "shows" and "hides" cases for the template, which is the primary rendering behavior that could regress.
  • runner_test.go's TestRunnerStoresPromptInput covers both completed and failed paths, verifying the resolved system prompt and that the diff shows up in the user message. This directly covers the main risk (failure path losing the prompt).
  • Not covered, but low risk: the SQL round-trip (scan/insert) for the two new columns — though this is a fairly mechanical addition following the exact pattern of finish_reason added previously, so the risk of the SQL wiring being subtly wrong is low, and it will be caught quickly in an integration/smoke environment.
  • No test seems to exercise the "shadow config" case with a SystemPromptOverride set to confirm the per-row independence claim (that two configs in a fan-out get different persisted prompts when one has an override) — this is the crux of the "each config row gets its own copy" design claim. Might be worth a quick unit test asserting configA.PromptSystem != configB.PromptSystem when overrides differ, since correctly capturing this divergence is the whole point of doing this at fan-out instead of once. Minor gap, not blocking.

Readability

  • Refactor of runOne into buildReviewRequest + runOne is a clean way to share the request construction without duplicating field-by-field logic, and it's clearly commented as to why.
  • Comment density throughout is very high but appropriate given how much of this PR is about invariants ("stored copy cannot drift from what was sent") that aren't obvious from the code shape alone.

No breaking changes to existing behavior; migration is additive and backward compatible (default '', IF NOT EXISTS guards). No security issues beyond the general note above about admin-page content escaping, which is already covered by Go's template auto-escaping.

This is a solid, well-tested, appropriately-scoped PR. My only substantive ask would be some acknowledgement of the storage growth mitigation plan (even just a follow-up ticket reference), but the PR description already gestures at this via the #78 storage page, so I'm not going to block on it.

📚 Context used in this review

Project guidance files:

  • CLAUDE.md
*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.38.0 | Full Review | Claude Sonnet 5 | tracking id `r-61ffe1-c8a8e9`* *This is an AI-generated review and may contain mistakes.* **Status:** ✅ Completed --- > ⚠️ The primary review (**Mistral Small**) failed because the diff is too large for that model. The review below is from **Claude Sonnet 5**, which reviewed the same diff. **✅ Verdict: Approve** — The prompt-capture design is structurally sound (single resolution helper prevents drift), migration and template changes are safe and well-tested, and the only notable concern — multiplied storage growth per fan-out config — is already acknowledged and cross-referenced to the storage-tracking follow-up. Overall this is a clean, well-scoped change that does what it says: capture the exact review-pass input at fan-out time and surface it on the comparison page. The design rationale in the migration comments and code comments is unusually thorough, and the approach of centralizing prompt resolution in `ResolveReviewSystemPrompt` (used by both the backends and the runner) is exactly right — it structurally prevents the stored copy from drifting from what was actually sent, which was the main risk in this kind of feature. Things I looked at specifically: **Correctness of prompt capture** - `fanOut` now renders `req` via `buildReviewRequest`, then calls `ResolveReviewSystemPrompt(req)` and `BuildUserMessage(req)` before invoking `runOne`. Since `runOne`/`Review` also call these exact same helpers against the identical `req` value, the stored fields are guaranteed to match the sent fields — no logic duplication that could diverge. Good. - `recordConfigResult` stamps `event.PromptSystem`/`PromptUser` unconditionally, including on failure, matching the PR's stated intent ("failed runs keep their input too"). This makes sense since the prompt is rendered before the network call. - One subtle nuance worth flagging in review (not necessarily a blocker): if `runOne` returns an error *before* the request is actually sent to the network (e.g., "no backend registered for provider" in `runOne`), the stored prompt is still recorded, even though nothing was actually transmitted. The doc comments already partially acknowledge this ("usually sent"), so this seems like a deliberate, accepted tradeoff rather than an oversight — worth confirming that's intentional but not a blocking issue. **Migration** - Default `''` with backfill-free `ADD COLUMN ... DEFAULT ''` is the right call for a TEXT column of unknown historical volume — no full-table rewrite needed on modern Postgres since the default is constant, and old/failed rows read back empty cleanly. Down migration is trivial and correct. - Comments do a good job flagging that full-tier rows will be substantial (hundreds of KB), and this is cross-referenced against the storage page (#78) rather than solved here, which seems like the right scope boundary for this PR. **Storage / performance concern** - This is the one area I'd want a comment on, not necessarily a blocker: every full-tier review event will now carry a copy of the diff + file contents + guidance/context fragments per config, duplicated N times for N configs in the fan-out (since "Stored per row, not per comparison group" is explicit in the migration). For an A/B test with e.g. 3 configs, that's 3x the storage per PR review for what's often near-identical content across configs (only system override and token budget differ; the diff/file content struct is shared). The PR description acknowledges the growth is "visible" on the storage page, but visibility isn't mitigation — there's no TTL/pruning story mentioned here or apparently elsewhere. This is fine for a first cut but is likely to need a retention policy sooner rather than later given full-tier prompts can be ~150KB+ each. **Template / dashboard** - The `{{if .PromptUser}}` guard correctly hides the whole section when empty; `{{if .PromptSystem}}` further guards the inner system-prompt block for cases where only user message exists (shouldn't happen given resolution logic always returns non-empty system, but harmless defensive code). - No escaping concerns: Go's `html/template` auto-escapes `{{.PromptSystem}}`/`{{.PromptUser}}` by default, so arbitrary diff/file content rendered into an admin-only page is safe from XSS as long as the dashboard continues to use `html/template` (which it appears to, given `render()` is templated HTML elsewhere in this file). - No indication of any collapse/redaction for potential secrets embedded in file contents that get rendered on the admin page — but this is consistent with existing `ReviewBody`/diff handling elsewhere in the dashboard, so not a new risk introduced by this PR specifically, and the page is presumably gated by `RequireAdmin`. **Test coverage** - `compare_render_test.go` covers both the "shows" and "hides" cases for the template, which is the primary rendering behavior that could regress. - `runner_test.go`'s `TestRunnerStoresPromptInput` covers both completed and failed paths, verifying the resolved system prompt and that the diff shows up in the user message. This directly covers the main risk (failure path losing the prompt). - Not covered, but low risk: the SQL round-trip (scan/insert) for the two new columns — though this is a fairly mechanical addition following the exact pattern of `finish_reason` added previously, so the risk of the SQL wiring being subtly wrong is low, and it will be caught quickly in an integration/smoke environment. - No test seems to exercise the "shadow config" case with a `SystemPromptOverride` set to confirm the *per-row* independence claim (that two configs in a fan-out get different persisted prompts when one has an override) — this is the crux of the "each config row gets its own copy" design claim. Might be worth a quick unit test asserting `configA.PromptSystem != configB.PromptSystem` when overrides differ, since correctly capturing this divergence is the whole point of doing this at fan-out instead of once. Minor gap, not blocking. **Readability** - Refactor of `runOne` into `buildReviewRequest` + `runOne` is a clean way to share the request construction without duplicating field-by-field logic, and it's clearly commented as to why. - Comment density throughout is very high but appropriate given how much of this PR is about invariants ("stored copy cannot drift from what was sent") that aren't obvious from the code shape alone. No breaking changes to existing behavior; migration is additive and backward compatible (default `''`, `IF NOT EXISTS` guards). No security issues beyond the general note above about admin-page content escaping, which is already covered by Go's template auto-escaping. This is a solid, well-tested, appropriately-scoped PR. My only substantive ask would be some acknowledgement of the storage growth mitigation plan (even just a follow-up ticket reference), but the PR description already gestures at this via the #78 storage page, so I'm not going to block on it. <details> <summary>📚 Context used in this review</summary> **Project guidance files:** - `CLAUDE.md` </details>
rcsheets deleted branch feat/review-prompt-storage 2026-07-23 11:52:21 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
pr-reviewer-bot
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
brooktrails/pr-reviewer!79
No description provided.