Skip to content

feat(review): show what each review cost on the review comment - #1005

Closed
guyoron1 wants to merge 1 commit into
fullsend-ai:mainfrom
guyoron1:feat/review-cost-footer
Closed

guyoron1 wants to merge 1 commit into
fullsend-ai:mainfrom
guyoron1:feat/review-cost-footer

Conversation

@guyoron1

Copy link
Copy Markdown
Contributor

The runner records total_cost_usd and model into metrics.json, but that file only ever reaches whoever downloads the workflow artifact — so in practice nobody sees what a review costs.

metrics.json sits in the post-script's working directory (run.go sets postCmd.Dir = runDir, and the post-script runs from a defer that fires after the metrics are written), so the footer is assembled here alongside the other body mutations — severity filtering, protected-path downgrade, action hints — rather than in the runner.

Degradation is the design

duration_seconds and over_budget are read optionally and are not yet emitted by any released runner — they arrive with the max_cost_usd work in fullsend-ai/fullsend. Until then the footer renders cost and model alone. Every field is independently omittable, so no separator dangles and nothing prints as null:

$2.9187 · 11m 19s · claude-opus-4-6                     # once the runner ships both fields
$1.44 · claude-opus-4-6                                 # today
                                                        # degenerate metrics: no footer at all

This means the PR can merge against the currently deployed CLI and simply gets richer later, rather than needing a coordinated release.

The failure case

The footer is never appended to a result with no body. A failure result carries reason and no body (schemas/review-result.schema.json), and jq's null + string would have made the footer the entire comment — replacing "This PR was NOT reviewed. Do not count this as an approval" with a price tag. Unreadable metrics skip the footer entirely: cost reporting must never be the reason a review fails to post.

Testing

scripts/post-review-cost-footer-test.sh extracts the jq program from the shipped script — so the test cannot drift from what runs — and covers 12 cases: full metrics, an older runner missing fields, budget-capped, sub-minute and exactly-one-minute durations, zero cost, empty model, non-boolean over_budget, and degenerate metrics. Wired into make script-test, and bash-3.2 safe.

Negative-checked both ways: breaking the jq fails the tests, and breaking the extraction anchor fails loudly rather than silently testing an empty program.

Note for review

scripts/post-review.sh is a generated bundle. The change is in post-review.src.sh and the generated file, byte-identical in the region the bundler passes through unchanged (the bundler only inlines source scripts/lib/*.lib.sh lines). make check-bundle enforces this.

The runner records total_cost_usd and model into metrics.json, but that
file only ever reaches whoever downloads the workflow artifact, so the
per-review cost is invisible in practice. metrics.json sits in the
post-script's working directory, so the footer is assembled here
alongside the other body mutations rather than in the runner.

duration_seconds and over_budget are read optionally and are not yet
emitted by any released runner — they arrive with the max_cost_usd work
in fullsend. Until then the footer renders cost and model alone; every
field is independently omittable, so no separator dangles and no field
prints as null.

The footer is never appended to a result with no body: a failure result
carries reason and no body, and jq's null + string would have made the
footer the entire comment, replacing the 'This PR was NOT reviewed'
notice with a price tag. Unreadable metrics skip the footer entirely —
cost reporting must never be the reason a review fails to post.

Signed-off-by: guy oron <goron@redhat.com>
@github-actions

Copy link
Copy Markdown

Functional tests did not run

Functional tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

@guyoron1

Copy link
Copy Markdown
Contributor Author

Closing in favor of surfacing cost runner-side: the runner already builds a run-info footer (runtime/model/effort/cost) that renders on completion comments where status notifications are enabled — see the live example on fullsend-ai/pi-xai-vertex#4. Per maintainer feedback the right shape is an opt-in cost summary in status_notifications for all agents, not a review-only footer in the post-script. The remaining delta is a small toggle in the runner, which supersedes this.

@guyoron1 guyoron1 closed this Aug 26, 2026
@fullsend-ai-retro

fullsend-ai-retro Bot commented Aug 26, 2026 •

Copy link
Copy Markdown

🤖 Finished Retro · ✅ Success · Started 4:16 AM UTC · Completed 4:23 AM UTC

Commit: ae8ad89 · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.89

@fullsend-ai-retro

Copy link
Copy Markdown

Retro: PR #1005 — feat(review): show what each review cost on the review comment

This PR was opened by external contributor guyoron1 on 2026-08-25 with a well-crafted implementation (217 additions, 12 test cases) to append a cost/timing footer to review comments via post-review.sh. After ~20 hours with no on-platform review, the author self-closed the PR citing off-platform maintainer feedback that cost visibility should be surfaced runner-side (via status_notifications) rather than in the post-script.

Agent involvement: none

No fullsend agents (triage, review, code, fix) were dispatched on this PR. The fullsend dispatch routing correctly produced an empty matrix because the contributor has NONE author association and no ok-to-test label was applied. The functional tests gate also correctly blocked execution for the same reason.

Improvement opportunity (already tracked)

This retro run is additional evidence for #349 — Generalize pre-retro early exit to skip retro on any PR with zero fullsend agent involvement. The current pre-retro.sh validates the originating URL but does not check whether any agents were dispatched. This retro consumed an opus-model agent run to analyze a PR with zero agent workflow to retrospect on. Implementing #349 would avoid this class of wasted run.

No new proposals filed

All identified improvement opportunities are already covered by existing open issues. The workflow behaved as designed — routing, functional tests gating, and dispatch all operated correctly for an external contributor PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant