feat(dashboard): reconstruct shadow-substitution on the review page #81

Merged

rcsheets merged 1 commit from feat/reconstruct-shadow-substitution into main

2026-07-24 06:04:23 +00:00

rcsheets commented

2026-07-24 04:51:01 +00:00

Owner

Record the substitution at post time and reconstruct it exactly on the
dashboard:

New nullable review_events.posted_config_id (migration 000012). On a
failed primary, it holds the config_id of the alternate whose review
reached the PR. config_id rather than the shadow's tracking id because
the shadow id is minted after the posting decision; config_id is known
up front and uniquely keys a row within a comparison group.
runner.collectAndPost stamps it on the primary row in the alternate
path only; nil means primary-posted-self, nothing-posted, or an old
row.
The review page now flags a failed primary, names and links to the
alternate that was posted, surfaces that alternate's body in place of
the empty one, and marks failed A/B entries with their error instead
of an empty "0+0 tokens" card. postedSubstitute resolves the exact row
via posted_config_id, falling back to the first completed shadow for
rows predating the column.

Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com

When the primary review fails, a shadow that reviewed the same diff is posted to the PR in its place. The primary's own review page showed an empty body and a misleading A/B entry ("0+0 tokens") with no hint that a substitution happened. Record the substitution at post time and reconstruct it exactly on the dashboard: - New nullable review_events.posted_config_id (migration 000012). On a failed primary, it holds the config_id of the alternate whose review reached the PR. config_id rather than the shadow's tracking id because the shadow id is minted after the posting decision; config_id is known up front and uniquely keys a row within a comparison group. - runner.collectAndPost stamps it on the primary row in the alternate path only; nil means primary-posted-self, nothing-posted, or an old row. - The review page now flags a failed primary, names and links to the alternate that was posted, surfaces that alternate's body in place of the empty one, and marks failed A/B entries with their error instead of an empty "0+0 tokens" card. postedSubstitute resolves the exact row via posted_config_id, falling back to the first completed shadow for rows predating the column. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

rcsheets added 1 commit

2026-07-24 04:51:01 +00:00

feat(dashboard): reconstruct shadow-substitution on the review page

ci / check (pull_request) Successful in 46s

Details

852a9eddee

When the primary review fails, a shadow that reviewed the same diff is
posted to the PR in its place. The primary's own review page showed an
empty body and a misleading A/B entry ("0+0 tokens") with no hint that a
substitution happened.

Record the substitution at post time and reconstruct it exactly on the
dashboard:

- New nullable review_events.posted_config_id (migration 000012). On a
  failed primary, it holds the config_id of the alternate whose review
  reached the PR. config_id rather than the shadow's tracking id because
  the shadow id is minted after the posting decision; config_id is known
  up front and uniquely keys a row within a comparison group.
- runner.collectAndPost stamps it on the primary row in the alternate
  path only; nil means primary-posted-self, nothing-posted, or an old
  row.
- The review page now flags a failed primary, names and links to the
  alternate that was posted, surfaces that alternate's body in place of
  the empty one, and marks failed A/B entries with their error instead
  of an empty "0+0 tokens" card. postedSubstitute resolves the exact row
  via posted_config_id, falling back to the first completed shadow for
  rows predating the column.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

pr-reviewer-bot commented

2026-07-24 04:51:03 +00:00

Collaborator

Automated review by pr-reviewer v0.40.0 | Full Review | Claude Sonnet 5 | tracking id r-62ef37-3de119
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 change is well-designed and well-tested; the only notable gap is that PostedConfigID isn't stamped in the "defensive/unreachable" fallback branch of collectAndPost, which is a minor inconsistency rather than a blocking bug.

Overall this is a clean, well-scoped change. The design rationale (storing config_id instead of the shadow's tracking id, because the id is minted after the posting decision) is sound and clearly documented in both the migration SQL comment and the Go doc comment on PostedConfigID. The migration is a simple additive nullable column with a matching down migration — low risk.

A few points worth going through:

Correctness / logic

postedSubstitute (user_handlers.go) correctly narrows to review.IsPrimary && review.Status == "failed", matching postAlternateReview's trigger condition in runner.go. The fallback to "first completed shadow" for pre-migration rows is a reasonable heuristic and is explicitly caveated in the comment as approximate.
One asymmetry to flag: collectAndPost only stamps PostedConfigID in the reactive branch (case winnerIdx >= 0: inside the loop, runner.go ~360). The default/fallback paths after the loop — specifically the "defensive... unreachable given the loop above" branch that also calls postAlternateReview — do NOT stamp PostedConfigID. If that "unreachable" path is ever actually reached (comment admits it's meant to be a defensive net, not truly guaranteed unreachable by the type system), the alternate gets posted to the PR but the dashboard will fall back to "first completed shadow" instead of using the precise recorded value. This is a latent inconsistency: since the code already treats this branch as reachable-in-theory (hence the defensive net at all), it would be more robust to set t.Event.PostedConfigID there too, or factor the stamping into postAlternateReview itself so it can never be missed. Given the "unreachable... kept as a net" framing, this is a minor risk rather than a correctness bug in practice, but it's an easy miss to guard against.
Test coverage gap: there's no runner-level test exercising this fallback/defensive path's interaction with PostedConfigID (only the reactive path is tested in TestCollectAndPostAlternateFallback). Given the PR explicitly calls out that this defensive branch exists, a small test asserting parity would tighten the guarantee described in the design write-up.

Minor readability

In review_detail.html, the A/B comparison intro paragraph and the "Review body" section both duplicate the $primaryFailed && $sub condition logic inline. Not a real problem given the html/template constraints, but a helper method producing a single "substitution summary" struct instead of scattering $sub/$primaryFailed conditionals across three separate blocks would probably read easier if this grows further test cases in future PRs. Not blocking.
postedSubstitute's loop returns &group[i] — pointer into the caller's slice. That's fine here since group is owned by the request and not mutated concurrently, just noting there's no defensive copy, which is a reasonable trade-off for a per-request slice.

Test coverage

The new dashboard rendering tests (failed_primary_render_test.go) are thorough: happy path with substitute, no-substitute case, tie-break via PostedConfigID among multiple shadows, and unaffected-clean-primary regression check. Good breadth.
runner_test.go verifies the reactive path stamps PostedConfigID correctly. As noted above, the "unreachable" fallback path isn't covered, but that's a minor gap given it's explicitly documented as defensive/unreachable.
No test directly against internal/tracker for round-tripping PostedConfigID through Record/scanEvent against a live DB, but this mirrors the existing pattern for other columns in this file (no such round-trip tests exist for sibling columns either), so it's consistent with existing conventions rather than a new gap introduced by this PR.

Breaking changes / migrations

Purely additive column, nullable, with a DROP COLUMN IF EXISTS down migration — safe to roll forward and back.

Nothing here rises to a blocking issue; the one item worth addressing (or at least explicitly acknowledging as accepted risk) is the missing PostedConfigID stamp in the defensive fallback branch of collectAndPost, since the code's own comments treat that branch as a possible (if unlikely) live path rather than truly dead code.

📚 Context used in this review

Project guidance files:

CLAUDE.md

*Automated review by [pr-reviewer](https://git.brooktrails.org/brooktrails/pr-reviewer) v0.40.0 | Full Review | Claude Sonnet 5 | tracking id `r-62ef37-3de119`* *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 change is well-designed and well-tested; the only notable gap is that PostedConfigID isn't stamped in the "defensive/unreachable" fallback branch of collectAndPost, which is a minor inconsistency rather than a blocking bug. Overall this is a clean, well-scoped change. The design rationale (storing `config_id` instead of the shadow's tracking id, because the id is minted after the posting decision) is sound and clearly documented in both the migration SQL comment and the Go doc comment on `PostedConfigID`. The migration is a simple additive nullable column with a matching down migration — low risk. A few points worth going through: **Correctness / logic** - `postedSubstitute` (user_handlers.go) correctly narrows to `review.IsPrimary && review.Status == "failed"`, matching `postAlternateReview`'s trigger condition in runner.go. The fallback to "first completed shadow" for pre-migration rows is a reasonable heuristic and is explicitly caveated in the comment as approximate. - One asymmetry to flag: `collectAndPost` only stamps `PostedConfigID` in the *reactive* branch (`case winnerIdx >= 0:` inside the loop, runner.go ~360). The `default`/fallback paths after the loop — specifically the "defensive... unreachable given the loop above" branch that also calls `postAlternateReview` — do NOT stamp `PostedConfigID`. If that "unreachable" path is ever actually reached (comment admits it's meant to be a defensive net, not truly guaranteed unreachable by the type system), the alternate gets posted to the PR but the dashboard will fall back to "first completed shadow" instead of using the precise recorded value. This is a latent inconsistency: since the code already treats this branch as reachable-in-theory (hence the defensive net at all), it would be more robust to set `t.Event.PostedConfigID` there too, or factor the stamping into `postAlternateReview` itself so it can never be missed. Given the "unreachable... kept as a net" framing, this is a minor risk rather than a correctness bug in practice, but it's an easy miss to guard against. - Test coverage gap: there's no runner-level test exercising this fallback/defensive path's interaction with `PostedConfigID` (only the reactive path is tested in `TestCollectAndPostAlternateFallback`). Given the PR explicitly calls out that this defensive branch exists, a small test asserting parity would tighten the guarantee described in the design write-up. **Minor readability** - In `review_detail.html`, the `A/B comparison` intro paragraph and the "Review body" section both duplicate the `$primaryFailed && $sub` condition logic inline. Not a real problem given the html/template constraints, but a helper method producing a single "substitution summary" struct instead of scattering `$sub`/`$primaryFailed` conditionals across three separate blocks would probably read easier if this grows further test cases in future PRs. Not blocking. - `postedSubstitute`'s loop returns `&group[i]` — pointer into the caller's slice. That's fine here since `group` is owned by the request and not mutated concurrently, just noting there's no defensive copy, which is a reasonable trade-off for a per-request slice. **Test coverage** - The new dashboard rendering tests (`failed_primary_render_test.go`) are thorough: happy path with substitute, no-substitute case, tie-break via `PostedConfigID` among multiple shadows, and unaffected-clean-primary regression check. Good breadth. - `runner_test.go` verifies the reactive path stamps `PostedConfigID` correctly. As noted above, the "unreachable" fallback path isn't covered, but that's a minor gap given it's explicitly documented as defensive/unreachable. - No test directly against `internal/tracker` for round-tripping `PostedConfigID` through `Record`/`scanEvent` against a live DB, but this mirrors the existing pattern for other columns in this file (no such round-trip tests exist for sibling columns either), so it's consistent with existing conventions rather than a new gap introduced by this PR. **Breaking changes / migrations** - Purely additive column, nullable, with a `DROP COLUMN IF EXISTS` down migration — safe to roll forward and back. Nothing here rises to a blocking issue; the one item worth addressing (or at least explicitly acknowledging as accepted risk) is the missing `PostedConfigID` stamp in the defensive fallback branch of `collectAndPost`, since the code's own comments treat that branch as a possible (if unlikely) live path rather than truly dead code. <details> <summary>📚 Context used in this review</summary> **Project guidance files:** - `CLAUDE.md` </details>

rcsheets requested review from pr-reviewer-bot

2026-07-24 04:51:11 +00:00

rcsheets merged commit 000527ee26 into main

2026-07-24 06:04:23 +00:00

rcsheets deleted branch feat/reconstruct-shadow-substitution

2026-07-24 06:04:24 +00:00

rcsheets referenced this pull request from a commit

2026-07-24 06:04:25 +00:00

Merge pull request 'feat(dashboard): reconstruct shadow-substitution on the review page' (#81) from feat/reconstruct-shadow-substitution into main