feat(review): use "Looks good to me" when no findings

[rh-hemartin]

Summary

• When the review agent finds no issues, it now sets the comment body to "Looks good to me" instead of posting a structured ## Review / ### Findings / No findings. block
• Omits the ## Review header and ### Findings section entirely in the zero-findings case
• Updates the outcome table to clarify that body defaults to "Looks good to me" for approve with no findings

Closes #1499

Test plan

• Trigger a review agent run on a clean PR with no findings — confirm sticky comment reads "Looks good to me" with no structured block
• Trigger a review agent run on a PR with low/info findings — confirm full structured comment still appears
• Confirm approval is still submitted via GitHub's formal review mechanism in both cases

🤖 Generated with Claude Code

[github-actions]

E2E tests are running

Authorization passed for this commit. See the E2E Tests workflow for results.

[github-actions]

Site preview

Preview: https://b1b35a41-site.fullsend-ai.workers.dev

Commit: 6b2c71fb01599659c61a7fa05147971b41cdfa1a

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 8:56 AM UTC · Completed 9:05 AM UTC
Commit: 4f6dbc3 · View workflow run →

[ifireball]

Looks good to Me :)

[fullsend-ai-review]

Review

Findings

Medium

• [stale-reference] skills/pr-review/SKILL.md:699 — The non-scaffolded copy at skills/pr-review/SKILL.md is currently identical to the scaffolded copy at internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md, but this PR only updates the scaffolded copy. After merge, the scaffolded copy will instruct agents to emit "Looks good to me" while the non-scaffolded copy (used by this repo's own review agent) will still say "state 'No findings.' in place of the findings section", creating a behavioral divergence between this repo and scaffolded repos.
  Remediation: Apply the same two changes (formatting-rules bullet and approve table row) to skills/pr-review/SKILL.md so both copies stay in sync.

Low

• [pattern-inconsistency] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:717 — The updated approve table cell contains a redundant parenthetical "(preceded by the hidden SHA comment)" that duplicates information already documented in the formatting rules section (line 687). The body field is also mentioned twice in the cell — once in the required-fields list and again in the special instruction.
  Remediation: Simplify to: body, head_sha; set body to "Looks good to me" when there are no findings; include findings[] when low/info findings are actionable follow-up work.

Previous run

Review

Findings

High

• [api-contract] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:713 — The updated table says body "defaults to" a value, implying it is no longer required for approve actions. However, the JSON schema at internal/scaffold/fullsend-repo/schemas/review-result.schema.json unconditionally requires body for approve via "then": { "required": ["body", "head_sha"] } with minLength: 1. The table column header says "Required fields" but the new wording removes body from the required position and uses "defaults to", which an LLM agent could interpret as body being optional — causing schema validation failure at the fullsend-check-output step.
  Remediation: Either (1) update the schema to make body optional for approve with a harness-level default, or (2) reword the table to: `body`, `head_sha`; set `body` to "Looks good to me" when there are no findings — keeping body clearly listed as required.

Medium

• [internal-consistency] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:695 — The formatting rules now say the body should be just "Looks good to me" with no ## Review header. However, lines 650–660 require the head SHA to be embedded in a hidden HTML comment on the first line of the body. A body of just "Looks good to me" would omit the SHA comment. The pre-fetch-prior-review.sh script extracts the SHA from the body using the **Head SHA:** pattern — a body without this comment would cause SHA extraction to fail on re-reviews.
  Remediation: Clarify that the no-findings body should still be prefixed with the hidden HTML comment, e.g., <!-- **Head SHA:** abc123 -->\nLooks good to me.

• [stale-reference] skills/pr-review/SKILL.md:695 — The non-scaffolded copy at skills/pr-review/SKILL.md (used by this repo's own review agent) is currently identical to the scaffolded copy but is not updated by this PR. This creates a behavioral divergence: scaffolded repos will emit "Looks good to me" while this repo's own reviews will continue emitting the structured "No findings." block.
  Remediation: Apply the same changes to skills/pr-review/SKILL.md so both copies stay in sync.

Low

• [pattern-inconsistency] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:713 — The new table cell reorders fields so head_sha appears first without body, breaking the pattern where required fields are listed upfront. See also: [api-contract] finding at this location.

Info

• [scope-exceeded] internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md:695 — Issue Review agent should reduce or completely skip commenting when no comments #1499 presents two options: skip comments entirely or shorten to LGTM. This PR implements option 2 without documenting why comment skipping was not chosen. Consider adding a note in the issue.

[fullsend-ai-review]

See the review comment for full details.

[fullsend-ai-review]

[high] api-contract

The updated table says body 'defaults to' a value, implying it is no longer required for approve actions. However, the JSON schema unconditionally requires body for approve actions with minLength: 1. The table column header says 'Required fields' but the new wording removes body from the required position and uses 'defaults to', which an LLM agent could interpret as body being optional — causing schema validation failure.

Suggested fix: Either update the schema to make body optional for approve, or reword the table to: 'body, head_sha; set body to "Looks good to me" when there are no findings' — keeping body clearly listed as required.

[fullsend-ai-review]

[medium] internal-consistency

The formatting rules now say the body should be just 'Looks good to me' with no ## Review header. However, lines 650-660 require the head SHA to be embedded in a hidden HTML comment on the first line of the body. A body of just 'Looks good to me' would omit the SHA comment. The pre-fetch-prior-review.sh script extracts the SHA using the Head SHA: pattern — a body without this comment would cause SHA extraction to fail on re-reviews.

Suggested fix: Clarify that the no-findings body should still be prefixed with the hidden HTML comment containing the head SHA.

[fullsend-ai-review]

[low] pattern-inconsistency

The new table cell reorders fields so head_sha appears first without body, breaking the pattern where required fields are listed upfront in the 'Required fields' column.

[fullsend-ai-review]

[info] scope-exceeded

Issue #1499 presents two options: skip comments entirely or shorten to LGTM. This PR implements option 2 without documenting why comment skipping was not chosen.

[ben-alkov]

The High [api-contract] and Medium [internal-consistency] findings need to be addressed.

[fullsend-ai-review]

🤖 Review · Started 6:22 AM UTC
Commit: 6b2c71f · View workflow run →

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 6:22 AM UTC · Completed 6:33 AM UTC
Commit: 6b2c71f · View workflow run →

[ben-alkov]

LGTM

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 1:07 PM UTC · Completed 1:14 PM UTC
Commit: 6b2c71f · View workflow run →

[fullsend-ai-retro]

Retro: PR #2159 — feat(review): use "Looks good to me" when no findings

Timeline

1. Jun 11 08:52 — Human author (rh-hemartin) + Claude Sonnet 4.6 open PR #2159: a 4-line change to internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md to emit "Looks good to me" instead of a structured empty block when the review agent finds no issues. Closes #1499.
2. Jun 11 09:01 — Human reviewer (ifireball) approves the PR before the review agent finishes.
3. Jun 11 09:05 — Review agent (run 27335489420) submits CHANGES_REQUESTED with a High [api-contract] finding ("defaults to" wording makes body appear optional when the JSON schema requires it) and a Medium [internal-consistency] finding (missing hidden SHA HTML comment). Also flags a Medium [stale-reference] (non-scaffolded skills/pr-review/SKILL.md not updated).
4. Jun 11 15:45 — Second human reviewer (ben-alkov) requests changes, citing the High and Medium findings.
5. Jun 12 06:19 — Author pushes fix commit addressing api-contract and internal-consistency.
6. Jun 12 06:33 — Second review (run 27398525459) confirms the High and Medium fixes resolved. Remaining: Medium [stale-reference] (still unfixed) and Low [pattern-inconsistency]. Post-script applies requires-manual-review label.
7. Jun 12 12:55 — ben-alkov approves (LGTM) despite the remaining medium finding.
8. Jun 12 13:03 — PR merged.

Assessment

Review quality: strong. The review agent caught two legitimate bugs (api-contract, internal-consistency) that would have caused schema validation failures and SHA extraction breakage in production. The human reviewer (ben-alkov) validated these findings independently.

Rework rate: 1 iteration. Author pushed one fix commit that resolved both High and Medium findings. Reasonable for the complexity.

Remaining gap: The [stale-reference] finding (non-scaffolded skills/pr-review/SKILL.md not updated, creating behavioral divergence) was flagged in both review runs but merged unresolved. This is a known class of issue — existing issue #1646 proposes a lint check to detect scaffold/non-scaffold divergence, which would have caught this.

Early human approval: ifireball approved at 09:01, 4 minutes before the review agent found a High severity issue at 09:05. Existing issue #2099 proposes alerting when humans approve with unresolved Medium+ findings.

Proposals

No new proposals. The improvement areas identified are already covered by existing open issues:

• Early approval race: #2099
• Scaffold divergence lint: #1646

This PR is a good data point supporting the priority of both issues.