Skip to content

fix: validate review START mode before lineage prerequisites - #1183

Open
mvanhorn wants to merge 1 commit into
Gentleman-Programming:mainfrom
mvanhorn:fix/992-review-start-mode-validation
Open

mvanhorn wants to merge 1 commit into
Gentleman-Programming:mainfrom
mvanhorn:fix/992-review-start-mode-validation

Conversation

@mvanhorn

@mvanhorn mvanhorn commented Sep 18, 2026

Copy link
Copy Markdown

Issue #992 reports committed-range START failures, incorrect INSPECT bases, and ASSESS counting unrelated base-branch changes, with independent confirmations across Linux, macOS, and Windows. The remaining directly verifiable host bug is that a missing or unsupported START mode falls into the Judgment Day path, which asks for idempotencyKey and lineageId before parseStartInput reports the actual invalid mode. Adding those unrelated credentials therefore changes the diagnostic without making the original ordinary-review request valid. This plan addresses that prerequisite chain and must use Refs #992, not a closing reference; it does not claim to make every reported committed-range review succeed.

Summary

Validate the START mode immediately after parseControllerJson in executeReviewControllerOperation, before the ordinary/ Judgment Day branch split and before either branch's prerequisites or native work. Reuse the existing actionable mode diagnostic (ordinary or judgment-day, JSON-string input, no lineage created, no STATUS/ADVANCE recovery); move or share only the mode check if needed, without invoking the full legacy parseStartInput on ordinary requests or silently defaulting the mode. Preserve explicit ordinary routing, explicit Judgment Day credential checks, and all candidate, consent, and provider authority safeguards.

Test Plan

  • Reproduce the first two issue calls with baseRef plus committedOnly and omitted mode, first with idempotencyKey and then with lineageId added: both must report the missing/unsupported mode first.
  • Table-drive omitted, null, numeric, and unknown string modes with all four credential-presence combinations; assert the actionable mode diagnostic and absence of .git/gentle-ai/reviews/graph-v1 after failure.
  • Use the existing controller/native injection hooks to prove rejected modes invoke neither native targetStatus/start nor authority creation; add no test-only production abstraction.
  • Preserve existing malformed JSON and nested-object errors, including the no-lineage guidance.
  • Preserve explicit ordinary START behavior without caller-supplied legacy credentials, and explicit judgment-day missing-credential diagnostics and successful replay coverage.
  • During implementation, run node --experimental-strip-types --test tests/review-controller.test.ts tests/review-controller-native-routing.test.ts; then pnpm run typecheck and pnpm run check:runtime-modules. Run the standard pnpm test suite where the required installed toolchain/native binary is available, without treating a missing binary as a passing native integration test.

Closes #992

Summary by CodeRabbit

  • Bug Fixes
    • Native ordinary review starts now proceed through the native review flow.
    • Judgment Day review starts retain their existing graph-based behavior.
    • Invalid or unsupported review modes are rejected consistently.
    • Repeated identical Judgment Day starts are handled idempotently.
    • Review starts now validate required idempotency and lineage information.
    • Unsupported native review environments return a clear status without creating graph state.

@coderabbitai

coderabbitai Bot commented Sep 18, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: d2961de6-6c04-4025-b86e-ab8e7e52a53e

📥 Commits

Reviewing files that changed from the base of the PR and between 29f52a2 and 4de6fd7.

📒 Files selected for processing (2)
  • extensions/gentle-ai.ts
  • tests/review-controller.test.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The change validates START modes before dispatch, allows ordinary START requests to use the native review flow, and preserves the Judgment Day graph-v1 path. Tests cover replay, required inputs, unsupported modes, graph-v1 state, and native CLI invocation.

Changes

START mode flow

Layer / File(s) Summary
START validation and dispatch
extensions/gentle-ai.ts
START mode validation is centralized and runs before authority access. Ordinary START proceeds through native review startup. Judgment Day retains graph-v1 setup.
START behavior coverage
tests/review-controller.test.ts
Tests inject a native CLI and cover ordinary START responses, Judgment Day requirements and replay, unsupported mode and credential combinations, graph-v1 state, and native CLI call suppression.

Priority: ⬆️ High

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix · Severity of issue fixed: High

Suggested reviewers: alan-thegentleman

Merge Risk: ⚪ Minimal · up to 4de6f

START mode handling now validates invalid requests early while preserving the ordinary native and Judgment Day paths. No actionable current-head risk remains.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: validating review START mode before lineage prerequisites.
Linked Issues check ✅ Passed The PR implements the directly addressed coding objective in #992. assertSupportedReviewStartMode accepts only ordinary and judgment-day before native review processing. Tests cover omitted, `nu…
Out of Scope Changes check ✅ Passed The changed runtime parameter supports injection of the native review client for isolation in the new tests. The added and updated tests verify mode validation order, credential handling, native-call …
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

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

Labels

None yet

Projects

None yet

1 participant