Skip to content

feat(review): open on a caller-specified base and diff type (--base / --diff-type) - #1484

Merged
backnotprop merged 1 commit into
mainfrom
feat/review-open-state
Sep 10, 2026
Merged

backnotprop merged 1 commit into
mainfrom
feat/review-open-state

Conversation

@backnotprop

Copy link
Copy Markdown
Owner

What

Implements the review open-state feature: plannotator review can open on a caller-specified base ref and diff type, as per-invocation session state that persists nothing.

# stack: main → feature/part-1 → feature/part-2 (HEAD)
plannotator review --base feature/part-1
# opens: "All changes since feature/part-1"
#        = merge-base(feature/part-1, HEAD) vs working tree + untracked

plannotator review --base feature/part-1 --diff-type merge-base   # committed work on this layer only
plannotator review --base origin/feature/part-1                    # remote refs, tags, SHAs, HEAD~N all work

Built on #1483 (strict argument parsing), which is this PR's base branch.

Parser (packages/shared/review-args.ts)

  • --base <ref> / --diff-type <id>, space-separated, at most once, value consumed before the positional collector (so --base main <PR_URL> keeps the URL). Missing/duplicate/invalid values and unknown diff ids are errors[] entries.
  • REVIEW_OPEN_DIFF_TYPES exported and test-pinned against GIT_DIFF_TYPES (now exported from vcs-core).

Pure validator (packages/shared/review-open-state.ts, new)

  • Provider matrix: explicit errors on GitButler / jj / P4 / multi-repo workspace / PR-URL mode — never accept-and-ignore.
  • --base with a base-irrelevant saved default promotes the session to since-base with one stderr notice; the explicit --base X --diff-type uncommitted contradiction is fatal.
  • A probed base that does not resolve is fatal (Base ref not found + up to five near-match suggestions) — the guard against review-core's silent merge-base → HEAD degrade under a confident label.
  • When --base is given, the base-relative diff type is requested explicitly, so --base trunk makes since-base work even on repos where getGitContext cannot offer it (spec §4.6).

CLI (apps/hook/server/index.ts, both the review branch and the opencode-review bridge)

  • git rev-parse --verify --quiet --end-of-options "<ref>^{commit}" probe before the server starts; suggestions come from listBranches only on the failure path.
  • Threads requestedBase/requestedDiffType into prepareLocalReviewDiff and forwards initialBase (the base the patch was actually computed against) + initialBaseExplicit + openStatePinned into startReviewServer.

Server (both runtimes: packages/server/review.ts + apps/pi-extension/server/serverReview.ts)

  • New initialBaseExplicit option seeds baseExplicitlyChosen: no bare-local-name → origin/* canonicalization, no startup base upgrade, no "Baseline is behind" nagging — --base main means the local ref on purpose. Opt-in and additive: Pi's documented forward-the-local-name-and-let-it-upgrade behavior is unchanged without it (test-pinned).
  • New openStatePinned option rides /api/diff only (mount is the only place it matters).

Client (packages/review-editor)

  • New pure shouldOfferReviewSetup predicate, composed before initializeReviewSetup() in the && chain — the one-time setup cookie is consumed by calling that function, so a pinned session skips the dialog without consuming it (order pinned by a source-level test).
  • New pure shouldRepairPanelPair guard: the panel-pair self-heal fully bails for a pinned session (its other half is a config.json write).
  • Commit-diff reload un-trap: comment added tying it to a future --diff-type commit:<sha> (no behavior change in v1).
  • Q10 fix (corrected ruling): the since-base dropdown label now renders from the live active base (same rewrite pattern as merge-base), so it agrees with the adjacent base picker after a base switch or a --base launch. The two comments claiming a "live header" drives this (DiffTypePicker.tsx, review-core.ts) are deleted — there is no such on-screen header.

Pi + OpenCode

  • Pi: re-vendored (review-open-state added to vendor.sh); /plannotator-review forwards base/diffType with a new openStateFromFlags opt-in on CodeReviewOptions that turns on strict validation + the explicit/pinned bits for the slash-command path only; parse/validator errors surface through Pi's notifier. Programmatic openCodeReview callers are byte-identical.
  • OpenCode embedded plugin: same probe + validation + threading; startReviewServer injectable through CommandDeps for tests.
  • OpenCode CLI bridge: free — rawArgs forward + the bridge branch above.

Skills / docs (§10 sweep run at implementation time)

git grep -ln "plannotator review\|plannotator-review" over apps/skills, apps/kiro-cli, apps//commands, apps//skills — every hit updated: core knowledge skill (usage line, session-only semantics, the stacked-branch hint, git-only note), core + Claude + Kiro review skills, Kiro agent prompt, OpenCode/Copilot/Gemini command stubs, Droid README, Amp helpText, Pi command description. Plus cli.ts help + cli.test.ts literals, marketing docs (code-review.md new "Opening on a specific base" section incl. the no-behind-banner note; configuration.md; opencode.md), and the CLAUDE.md "review OPENS on" sentence. apps/kiro-cli/README.md hits name the skills without enumerating flags — no change needed. droid-plugin/commands/plannotator-review.js verified as pure passthrough.

Nothing is persisted (the maintainer's constraint)

No POST /api/config, no cookie writes, no defaultDiffType, no reviewPanelView, no last-used memo, and the review-setup-seen cookie survives untouched. Pinned-session no-persistence is test-pinned (config.json byte-identical after a flagged session), and the smoke run confirmed no config.json is created.

Tests

  • review-args.test.ts: value parsing, URL preservation, missing/dup values, unknown diff id (lists the set), range-syntax rejection, REVIEW_OPEN_DIFF_TYPES == GIT_DIFF_TYPES.
  • review-open-state.test.ts (new, pure): full provider/PR/workspace matrix, promote-with-notice, contradiction, base-not-found, near-match suggestions.
  • packages/server/review-open-state.test.ts (new, dual-runtime Bun + Pi): explicit base served verbatim; startup upgrade suppressed for an explicit main on a repo with origin/main (with the unpinned control proving the probe fires); canonicalization off on a non-explicit switch echo; openStatePinned present only when set; config.json byte-identical after a pinned session.
  • Client: shouldOfferReviewSetup / shouldRepairPanelPair pure tests, cookie-not-consumed composition test, source-level pin of the &&-chain order; DOM-gated DiffTypePicker live-label tests (incl. SHA shortening and the no-live-base fallback).
  • Parity: vendored parser + vendored validator tests (Pi), source-level pin of Pi's forward, and OpenCode handleReviewCommand tests (flag base lands in both the patch and the server options; unresolvable base refuses to start and never reaches startReviewServer).

Revert-verify evidence: server tests fail 4/10 without the server change; parser tests fail to load on the pre-flag parser; the App source-order pin and the DiffTypePicker DOM tests fail against the pre-change files.

bun run typecheck clean (includes Pi re-vendor); builds verified in order (review → hook → opencode); check:manifest in sync (viewer graph untouched); DOM_TESTS=1 bun test --isolate green on the touched DOM suites; full bun test under a temp PLANNOTATOR_DATA_DIR: 4413 pass / 899 skip / 0 fail (5312 tests, 473 files).

Out of scope (per spec §1.1)

--branch, --hide-whitespace, commit:<sha>/worktree/gitbutler/jj diff-type values, jj --base (the error message names jj-line as the future), persisting the flags, a "reset to launch state" affordance, PR-URL mode support.

@backnotprop
backnotprop changed the base branch from feat/review-strict-args to main September 10, 2026 03:17
Adds two session-only flags to plannotator review, parsed in the shared
parser so every host inherits them together:

- --base <ref> opens the session against a caller-chosen compare target
  (branch, origin/<branch>, tag, SHA, HEAD~N), probed with git rev-parse
  --verify --end-of-options before the server starts so a typo'd ref is a
  startup error with near-match suggestions instead of a silently
  mislabelled merge-base->HEAD diff.
- --diff-type <id> opens the session in one of the nine flat git diff
  modes (REVIEW_OPEN_DIFF_TYPES, pinned against GIT_DIFF_TYPES).

The flags are a seed, never a setting: nothing writes config.json or any
review cookie, and the UI stays fully mutable. Validation is pure in
packages/shared/review-open-state.ts (provider matrix errors on
jj/GitButler/P4/workspace/PR mode, promote-with-notice when the saved
default is base-irrelevant, fatal explicit contradiction).

A flagged base rides explicitBase semantics: the new initialBaseExplicit
server option (both runtimes) seeds baseExplicitlyChosen, suppressing the
startup origin/* upgrade and canonicalization, and openStatePinned rides
/api/diff so the client neither offers the first-run setup dialog (its
one-time cookie is NOT consumed) nor runs the panel-pair self-heal for a
pinned session. The since-base dropdown label now renders from the live
active base, matching the adjacent base picker.

Coverage: Bun CLI, opencode-review bridge, OpenCode embedded plugin, and
the Pi extension (re-vendored; strict validation on the slash-command
path only, programmatic callers unchanged). Skills, command stubs, help
text, and docs updated across every host surface.
@backnotprop
backnotprop force-pushed the feat/review-open-state branch from 18880b0 to d393121 Compare September 10, 2026 03:51
@backnotprop
backnotprop merged commit 9130d2d into main Sep 10, 2026
28 checks passed
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