Skip to content

fix: dbt PR reviewer posts COMMENT, never a formal APPROVE review#870

Merged
anandgupta42 merged 1 commit into
mainfrom
fix/review-no-auto-approve
Jun 2, 2026
Merged

fix: dbt PR reviewer posts COMMENT, never a formal APPROVE review#870
anandgupta42 merged 1 commit into
mainfrom
fix/review-no-auto-approve

Conversation

@anandgupta42
Copy link
Copy Markdown
Contributor

@anandgupta42 anandgupta42 commented Jun 2, 2026

What does this PR do?

The dbt PR reviewer mapped an APPROVE verdict to a GitHub APPROVE review event, so the bot was formally approving PRs. Observed live on AltimateAI/altimate-ingestion #682, where the github-actions bot review state is APPROVED. Under branch protection / required reviews, a bot approval can satisfy the requirement and let a PR merge without human sign-off.

This maps APPROVE → a COMMENT review event in VCS_EVENT (verdict.ts). The "approved — no findings" outcome is conveyed in the comment body instead. REQUEST_CHANGES still maps through (gate mode blocks; comment mode softens to COMMENT via applyMode). This matches how CodeRabbit / Greptile / cubic behave — they comment but never grant a formal approval.

  • packages/opencode/src/altimate/review/verdict.tsVCS_EVENT.APPROVE is now "COMMENT", with a doc comment explaining the bot-must-not-approve invariant.
  • packages/opencode/test/altimate/review.test.ts — regression test asserting no verdict emits a formal APPROVE event.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Issue for this PR

Closes #869

How did you verify your code works?

  • bun turbo typecheck — clean.
  • bun test test/altimate/review.test.ts — 65 pass, 0 fail (includes the new regression test asserting VCS_EVENT.APPROVE === "COMMENT" and that Object.values(VCS_EVENT) never contains "APPROVE").
  • Marker guard (--base origin/main --strict) — no upstream-shared files modified.

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective
  • New and existing unit tests pass locally with my changes

Summary by cubic

Stop the dbt PR reviewer from submitting formal GitHub approvals; an APPROVE verdict now posts a comment so branch protection and required reviews always need human sign-off. Closes #869.

  • Bug Fixes
    • Mapped APPROVECOMMENT in VCS_EVENT (packages/opencode/src/altimate/review/verdict.ts); the “approved — no findings” outcome is conveyed in the comment body.
    • Added a regression test to ensure no verdict emits a formal APPROVE review event (packages/opencode/test/altimate/review.test.ts).

Written for commit c1099d5. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Modified approval verdict submissions to emit as comments instead of formal approvals, ensuring they cannot satisfy required review requirements.
  • Tests

    • Added test verifying approval verdicts never emit as formal approvals.

The reviewer mapped an APPROVE verdict to a GitHub "APPROVE" review event, so the
bot was formally approving PRs (observed on altimate-ingestion #682: bot review
state APPROVED). With branch protection that can satisfy required reviews and let
a PR merge without human sign-off.

Map APPROVE -> COMMENT review event (the "approved / no findings" outcome is in
the comment body). REQUEST_CHANGES still maps through (gate mode blocks; comment
mode softens to COMMENT). Matches CodeRabbit/Greptile/cubic, which comment but
never approve. + regression test asserting no verdict emits a formal APPROVE.

Closes #869
Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 2, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

The review verdict system's APPROVE mapping is changed to emit COMMENT events instead of APPROVE events, preventing bot approvals from satisfying branch protection requirements. Documentation and a regression test ensure this safety constraint is maintained.

Changes

Review Verdict Mapping Safety

Layer / File(s) Summary
Verdict mapping and documentation update
packages/opencode/src/altimate/review/verdict.ts
VCS_EVENT[APPROVE] constant changed to map "COMMENT" instead of "APPROVE". Documentation expanded to clarify that APPROVE verdicts must never emit formal approval events that could satisfy required review semantics.
Test validation for safe verdict mapping
packages/opencode/test/altimate/review.test.ts
VCS_EVENT added to imports. New regression test asserts VCS_EVENT.APPROVE equals "COMMENT" and verifies "APPROVE" is absent from enum values.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

A bot once tried to seal the deal,
with formal stamps of true appeal—
but now it whispers soft instead,
a comment kind, not (formally) wed.
🐰✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely summarizes the main change: mapping APPROVE to COMMENT instead of a formal approval.
Description check ✅ Passed The description comprehensively addresses all template requirements with detailed context, verification steps, and complete checklist.
Linked Issues check ✅ Passed The PR successfully implements all objectives from issue #869: maps APPROVE to COMMENT, preserves REQUEST_CHANGES behavior, and adds regression tests.
Out of Scope Changes check ✅ Passed All changes are directly scoped to issue #869: modifying VCS_EVENT mapping and adding regression tests, with no unrelated modifications.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/review-no-auto-approve

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
packages/opencode/src/altimate/review/verdict.ts (1)

33-35: ⚡ Quick win

Narrow VCS_EVENT’s emitted event type to exclude "APPROVE"

VCS_EVENT’s value type still includes "APPROVE" even though APPROVE maps to "COMMENT" and tests assert "APPROVE" is never present. Restrict the emitted-event union to "COMMENT" | "REQUEST_CHANGES".

Suggested change
+export type VcsReviewEvent = "COMMENT" | "REQUEST_CHANGES"
+
-export const VCS_EVENT: Record<Verdict, "APPROVE" | "COMMENT" | "REQUEST_CHANGES"> = {
+export const VCS_EVENT: Record<Verdict, VcsReviewEvent> = {
   APPROVE: "COMMENT",
   COMMENT: "COMMENT",
   REQUEST_CHANGES: "REQUEST_CHANGES",
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/opencode/src/altimate/review/verdict.ts` around lines 33 - 35,
VCS_EVENT is currently typed to allow "APPROVE" even though APPROVE maps to
"COMMENT"; change its value type to exclude "APPROVE" by updating the
declaration for VCS_EVENT to use the narrower union "COMMENT" |
"REQUEST_CHANGES" (e.g., export const VCS_EVENT: Record<Verdict, "COMMENT" |
"REQUEST_CHANGES"> = { ... }), and ensure the object literal still maps APPROVE
-> "COMMENT" and other Verdict keys to the correct narrowed values so
type-checking and tests reflect that "APPROVE" is never emitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@packages/opencode/src/altimate/review/verdict.ts`:
- Around line 33-35: VCS_EVENT is currently typed to allow "APPROVE" even though
APPROVE maps to "COMMENT"; change its value type to exclude "APPROVE" by
updating the declaration for VCS_EVENT to use the narrower union "COMMENT" |
"REQUEST_CHANGES" (e.g., export const VCS_EVENT: Record<Verdict, "COMMENT" |
"REQUEST_CHANGES"> = { ... }), and ensure the object literal still maps APPROVE
-> "COMMENT" and other Verdict keys to the correct narrowed values so
type-checking and tests reflect that "APPROVE" is never emitted.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 517bea5d-9406-4f52-a92d-a3de62197662

📥 Commits

Reviewing files that changed from the base of the PR and between e94eb20 and c1099d5.

📒 Files selected for processing (2)
  • packages/opencode/src/altimate/review/verdict.ts
  • packages/opencode/test/altimate/review.test.ts

@anandgupta42 anandgupta42 merged commit 21721ac into main Jun 2, 2026
20 checks passed
anandgupta42 added a commit that referenced this pull request Jun 2, 2026
…-approve at compile time (#872)

Follow-up to #870. That PR mapped an `APPROVE` verdict to a GitHub `COMMENT`
review event but left two loose ends:

- Doc comments in `post-github.ts` still claimed "gate mode keeps APPROVE /
  REQUEST_CHANGES", contradicting the shipped behavior. Corrected to describe
  the actual contract (APPROVE → COMMENT always; comment mode also softens
  REQUEST_CHANGES → COMMENT; gate mode keeps REQUEST_CHANGES; `env.verdict`
  arrives pre-softened from `buildEnvelope`).
- `VCS_EVENT`'s value type still permitted `"APPROVE"`. Narrowed it to
  `"COMMENT" | "REQUEST_CHANGES"` so the no-formal-approval invariant is a
  compile-time guarantee — no future edit can resurrect a formal GitHub
  approval without breaking the build. The narrower union is still assignable
  to Octokit's `createReview` event param.
- Documented `applyMode`'s APPROVE/COMMENT passthrough.

Adds a composition test asserting the positive expected GitHub event for every
verdict-producing finding-set across both modes, plus the semantic-verdict-vs-
VCS-event separation (gate + no findings → `verdict === "APPROVE"` but event
`"COMMENT"`; comment-mode blocking critical → `idealVerdict === "REQUEST_CHANGES"`,
`verdict === "COMMENT"`).

Reviewed via multi-model consensus (GPT 5.4, Gemini 3.1 Pro, GLM-5): the type
narrowing and test strengthening were panel findings incorporated during
convergence.

Closes #871

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: dbt PR reviewer must not submit a formal APPROVE review (bot auto-approves PRs)

1 participant