feat(review): role + setup foundation (PR 1 of 3)#1241
Open
peyton-alt wants to merge 8 commits into
Open
Conversation
Adds Role enum (Reviewer/Fixer/Both/Skip) to ReviewConfig and a FixAfterReview mode to EntireSettings + ClonePreferences. Provides NormalizeRoles / MigrateLegacyRoles / ReviewersOf / FixerOf helpers and runs an idempotent in-memory migration on settings.Load that derives roles from the legacy ReviewFixAgent field. The schema is persisted lazily — the next code path that writes settings (e.g., `entire review setup`) writes the migrated form. Foundation for the entire review redesign — no behavior change yet for users running entire review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 5ecddc347c0f
Two-step picker: pick a role per installed agent (Reviewer/Fixer/Both/Skip), then pick skills only for Reviewer/Both agents. At-most-one-fixer is enforced via NormalizeRoles. The "Additional instructions" widget uses huh.NewInput so plain Enter submits — no modifier key needed on any platform. The legacy in-flow picker on `entire review` is unchanged in this commit; Chunk 3 replaces it with a `Run: entire review setup` pointer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Entire-Checkpoint: 4efdb600636d
Contributor
There was a problem hiding this comment.
Pull request overview
Foundation work for the upcoming entire review redesign: introduces role-based review configuration (Reviewer/Fixer/Both/Skip), in-memory migration from legacy config, and a new entire review setup subcommand to capture roles + skills, while keeping the existing entire review flow unchanged in this PR.
Changes:
- Add
Role+FixAfterReviewsettings schema (project + clone preferences) and in-memory legacy migration onsettings.Load. - Add
entire review setuptwo-step interactive flow (pick roles per installed agent, then skills/instructions for reviewer-side agents) and a post-setup banner. - Add role helpers (
NormalizeRoles,ReviewersOf,FixerOf) plus comprehensive unit tests for migration/setup/roles.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| t1.txt | Adds a new text file (appears unrelated to the feature). |
| cmd/entire/cli/settings/settings.go | Adds Role, FixAfterReview settings fields, applies clone pref override, and triggers in-memory legacy role migration during load. |
| cmd/entire/cli/settings/settings_test.go | Adds tests for Role.Valid and settings JSON round-tripping with roles/modes. |
| cmd/entire/cli/settings/migration.go | Implements MigrateLegacyRoles to upgrade legacy review configs in-memory. |
| cmd/entire/cli/settings/migration_test.go | Adds tests ensuring migration is in-memory only and schema saves/loads as expected. |
| cmd/entire/cli/review/setup.go | Implements entire review setup flow: role picker, skills picker, save, and banner output. |
| cmd/entire/cli/review/setup_test.go | Adds tests for setup flow, field construction, banner output, and subcommand registration. |
| cmd/entire/cli/review/roles.go | Adds role normalization and query helpers used by setup/run flows. |
| cmd/entire/cli/review/roles_test.go | Adds tests for normalization behavior and helper outputs. |
| cmd/entire/cli/review/cmd.go | Registers the new setup subcommand under entire review. |
Comments suppressed due to low confidence (1)
cmd/entire/cli/settings/settings.go:742
- The new fix_after_review setting is merged from JSON but never validated (unlike commit_linking). A hand-edited invalid value would silently load and could cause unexpected behavior later. Add validation during load/merge (e.g., allow only "" and "always") and return a clear error for invalid values.
if err := mergeRawStringNonEmpty(raw, "fix_after_review", (*string)(&settings.FixAfterReview)); err != nil {
return err
}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 92f419f. Configure here.
- Remove stray t1.txt CI-trigger artifact. - review setup: persist only the review map to clone-local preferences (.git/entire/preferences.json) via SaveReviewConfig instead of saving the full merged settings object to .entire/settings.json. The old path promoted clone-local/local-override values — and, on a settings.Load error, an empty EntireSettings with Enabled=false — into the committed project file. SaveReviewConfig writes just the review keys and refuses to clobber a malformed preferences file. Updates the subcommand help to match. (Cursor High + Copilot) - review setup: map context.Canceled to a SilentError so Ctrl+C during the role/skills form aborts quietly instead of printing "roles picker: ...: context canceled". - fix_after_review: introduce a non-empty "ask" sentinel (FixAfterReviewAsk) so a clone-local preference can override a project-level "always" back to Ask. Empty string still means "no override / inherit". Add FixAfterReviewMode.Valid(). - settings.Load: validate review roles and fix_after_review after merge+migration, rejecting hand-edited values that would otherwise load silently with surprising behavior. - Tests: invalid-role / invalid-mode rejection, clone "ask" overriding project "always", and setup persisting to clone-local (not project). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # cmd/entire/cli/settings/settings.go # cmd/entire/cli/settings/settings_test.go
peyton-alt
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://entire.io/gh/entireio/cli/trails/405
Summary
Foundation PR for the
entire reviewredesign. Pure-additive: no behavior change to existingentire reviewfor users on this PR alone. The cutover (drop legacy picker, add flags, inline prompts, live tokens) lands in PR 2.Roleenum (Reviewer/Fixer/Both/Skip) onReviewConfigand aFixAfterReviewmode onEntireSettings+ClonePreferences. Idempotent in-memory migration onsettings.Loadderives roles from the legacyReviewFixAgentfield (no auto-write; persisted lazily on next settings write).NormalizeRoles(at-most-one-fixer with alphabetical winner),MigrateLegacyRoles,ReviewersOf,FixerOf, plus a publicsettings.Save(ctx, *EntireSettings).entire review setup— role-first picker (one inline Select per installed agent), then skills + optional instructions only for Reviewer/Both agents. Saves to.entire/settings.jsonwith a display-label banner. InlineValidaterefuses a second Fixer/Both pick so the at-most-one-fixer rule shows up immediately, not silently post-pick.Why two PRs: foundation here is safe to land alone; the run-flow cutover (which deletes the legacy picker, adds
--reviewers/--fixerflags, inline post-review fix prompt, recursion guard, invoker-aware fallback, live tokens) is a separate review surface.Test plan
mise run checkclean (fmt, lint, 58/58 unit + integration, 4/4 canary). Run post-merge with main./tmp/smoke-test: build,entire enable --agent claude-codethen add codex + gemini,entire review setupdrives the role picker → skills picker → banner →.entire/settings.jsonpersisted.entire review(no args) still hits the legacy picker untouched.review_fix_agent: codex, ran a command that callssettings.Load, confirmed sha256 unchanged.MigrateLegacyRoles(nil, empty, idempotent, fix-agent-with-no-skills, preserves-existing-roles),NormalizeRoles,ReviewersOf,FixerOf,BuildPickRolesFields,RunSetup,PrintSetupBanner,BuildSetupSkillsFields, subcommand registration.🤖 Generated with Claude Code
Note
Medium Risk
Introduces new persisted settings fields and an in-memory migration on
settings.Load, plus a new interactivereview setupflow; while intended to be additive, schema/migration changes can subtly affect downstream settings consumers and review configuration behavior.Overview
Adds role-based review configuration to settings by introducing
settings.RoleonReviewConfig(Reviewer/Fixer/Both/Skip) and a newfix_after_reviewmode on both project settings and clone preferences.Implements an idempotent, in-memory legacy migration (
settings.MigrateLegacyRoles) run duringsettings.Loadto derive roles fromreview_fix_agentwithout auto-writing files, plus helper utilities inreview(NormalizeRoles,ReviewersOf,FixerOf).Introduces a new
entire review setupsubcommand that walks users through a two-step TUI (pick per-agent roles with a one-fixer constraint, then pick skills/instructions for reviewer agents) and persists the resultingreviewmap, including a post-setup banner; adds comprehensive unit tests for the new schema, migration, and setup flow.Reviewed by Cursor Bugbot for commit 92f419f. Configure here.