interface:
  display_name: "Cherry Studio Code Review"
  short_description: "Review code and architecture against project rules"
  default_prompt: "Use $gh-pr-review to review the current branch against Cherry Studio code and architecture rules."

# Checklist Evolution

Rules for updating review checklists. Goal: keep checklists minimal and
high-signal — each item should direct AI attention to a distinct class of real
issues, not catalog every possible bug pattern.

## Step 1: Draft candidates

For each uncovered pattern, draft a candidate item. ALL rules below MUST be
satisfied — violation makes the candidate invalid:

1. One assertive phrase describing the expected state (not a question)
2. Generic: applies across files, not tied to a specific variable, function, or bug
3. Atomic: one checkable concern per item (not "X and Y")
4. No overlap: if the issue is a specific case of an existing item, do NOT add it
5. Place under the most specific existing category; create a new category only when
   no existing one fits
6. Each category stays within 3–8 items. Below 3, merge into a related category.
   Above 8, first try merging overlapping items; only split if each resulting
   sub-category has a distinct focus expressible in 2–3 words
7. Prefer fewer, broader items — the checklist is a prompt for attention directions,
   not an exhaustive bug catalog

When uncertain whether a new item overlaps with an existing one, do NOT add it.

## Step 2: User confirmation

Present candidates via multi-select. Each option label is the candidate item
text. Unchecked candidates are discarded. If all are discarded, stop.

## Step 3: Insert

Insert accepted items into the checklist file at the appropriate position per the
category and priority rules above.

# Cherry Review Guidance

Use this reference as the Cherry Studio project-specific lens for code and
architecture reviews. It complements `code-checklist.md`; it does not replace
evidence requirements. Only report issues that are grounded in current code.

## Scope Triage

Classify each reviewed module before looking for issues:

| Area | Common files | Review focus |
| --- | --- | --- |
| Data system | `src/main/data/`, `src/shared/data/`, `src/renderer/data/`, `docs/references/data/` | Correct system choice, DataApi scope, migrations, row/entity boundaries |
| Service boundary | `src/main/data/services/`, `src/main/services/` | Owning service, cross-service calls, transactions, side effects |
| IPC / preload | `src/shared/ipc/`, `src/main/ipc/`, `src/preload/`, `src/renderer/ipc/`, legacy `src/shared/IpcChannel.ts` | IpcApi routing, input validation, exposure, compatibility, migration completeness |
| Lifecycle / windows / paths | `src/main/core/`, window services, path access | Lifecycle ownership, cleanup, `application.getPath`, WindowManager |
| Main architecture | `src/main/` moves, additions, imports, services, features | Closed top level, placement, dependency direction, public boundaries |
| Renderer architecture | `src/renderer/` moves, additions, imports | Type/domain placement, downward dependencies, feature isolation, public boundaries |
| Shared layer | `src/shared/` | Actual cross-process demand, immutable/stateless surface, closed top level, API contracts |
| Renderer data hooks | `src/renderer/data/`, hooks using `useQuery`, `useMutation`, cache/preference hooks | SWR keys, invalidation, optimistic updates, external store snapshots |
| React UI | `src/renderer/`, `packages/ui/` | `@cherrystudio/ui`, i18n, a11y, hooks correctness, design-system fit |
| Network downloads | Package-manager configuration, lockfiles, install/download code, model or binary manifests | Global and China-accelerated sources, artifact parity, source selection, integrity checks |
| Naming / module shape | Added, renamed, or moved files/directories; new classes and barrels | Path casing, export-role naming, Service/Manager roles, promotion, barrel boundaries |

## Anti-Fragmentation Review Principles

Use these principles before proposing a fix. They prevent scattered local
patches, one-off service APIs, and speculative abstractions from spreading
through the codebase.

1. Fix upstream, not downstream.
   - If a consumer adds a workaround because a shared module, service, hook, or
     component has a limitation, ask whether the shared upstream surface should
     be fixed instead.
   - Flag downstream patches when the same limitation can affect other
     consumers, when multiple consumers duplicate the same guard, or when the
     patch hides an upstream contract bug.
   - Do not demand an upstream rewrite for a truly isolated compatibility shim;
     ask for the boundary and expiration condition instead.
2. Generalize clear public service needs before specializing.
   - When the requirement is a stable domain operation or a likely shared
     capability, prefer a clear method on the owning service, hook, or component
     API over a page-specific helper or endpoint.
   - The need must be concrete. Do not generalize only for imagined future
     callers.
   - A specialized implementation is acceptable for a one-off workflow when it
     remains local and does not duplicate a public capability.
3. Stay simple and restrained.
   - Avoid extra layers, registries, state machines, adapters, config systems,
     or extension points without current evidence.
   - Do not flag "missing abstraction" unless there is real duplication,
     ownership confusion, or a clear public service requirement.
   - Prefer the smallest fix that repairs the boundary and keeps the system
     understandable.

Report these as:

- **Blocker** when fragmentation creates a runtime/data/security risk or breaks
  a public contract.
- **Warning** when a one-off patch or specialized helper makes ownership unclear
  and the smaller upstream/general fix is evident.
- **Notice** when the diff needs author confirmation about whether a capability
  should be upstreamed, generalized, or intentionally kept local.

## Network Download Source Gate

Every component fetched over the network during development, build,
installation, or runtime must have both a usable global source and a usable
China-accelerated source. This includes package-manager dependencies such as
npm packages, runtime and toolchain binaries, offline models, and other
downloaded assets.

- For registry packages, the supported install path must work with both the
  default global registry and a China mirror; dependency declarations do not
  need duplicate URLs.
- For models, binaries, and URL-addressed assets, both sources must resolve to
  the same version and content and use the same integrity validation when one
  is available.
- A hard-coded single source, or a second source that no supported code or
  configuration path can select, does not satisfy this requirement.

Treat any new or changed network download that lacks either usable source as a
**Blocker**. Do not approve or recommend merging the change until both sources
are provided.

## Reference Routing

Load references by changed area. Do not paste every external guide into every
review. Project docs and repository code win over external references when they
conflict.

### Internal Repository Docs

| Changed area | Consult |
| --- | --- |
| Added, renamed, or moved files/directories; new classes, services, managers, features, or barrels | `docs/references/architecture/naming-conventions.md` |
| `src/main/` placement, imports, top-level structure, features, services, or utils | `docs/references/architecture/main-process.md`, plus the subsystem reference it routes to |
| `src/renderer/` placement, imports, top-level structure, pages, features, shared buckets, or public APIs | `docs/references/architecture/renderer.md` |
| `src/shared/` placement, exports, runtime state, top-level structure, or cross-process contracts | `docs/references/architecture/shared-layer.md` |
| Choosing among DataApi, Cache, Preference, BootConfig, and `app_state` | `docs/references/data/README.md`; stop there unless the diff enters one of the subsystem rows below |
| DataApi contracts, schemas, types, or errors | `docs/references/data/data-api-overview.md`, `api-design-guidelines.md`, `api-types.md` |
| DataApi handlers, services, or renderer hooks | Add `docs/references/data/data-api-in-main.md` for main handlers/services and `data-api-in-renderer.md` for renderer consumers |
| Cache storage, hooks, service calls, or keys | `docs/references/data/cache-overview.md`; add `cache-usage.md` for consumers and `cache-schema-guide.md` only when keys/schemas change |
| Preference storage, hooks, service calls, or keys | `docs/references/data/preference-overview.md`; add `preference-usage.md` for consumers and `preference-schema-guide.md` only when keys/schemas change |
| BootConfig behavior, access, or keys | `docs/references/data/boot-config-overview.md`; add `boot-config-schema-guide.md` only when keys/schemas/mappings change |
| Internal startup continuity markers | `docs/references/data/app-state-overview.md` |
| v1-to-v2 migrators or migration mappings | `docs/references/data/v2-migration-guide.md` plus the affected target subsystem guide |
| SQLite schemas, transactions, migrations, defaults, or nullability | `docs/references/data/database-patterns.md`; add `database-construction.md` for migration/custom-SQL/FTS build changes and `best-practice-default-values-and-nullability.md` for default/nullability changes |
| Sortable resources or order keys | `docs/references/data/data-ordering-guide.md` |
| Offset/cursor pagination or paginated hooks | `docs/references/data/data-pagination-guide.md` |
| Database seeders or seeding policies | `docs/references/data/database-seeding-guide.md` |
| Static presets with user overrides | `docs/references/data/best-practice-layered-preset-pattern.md` |
| Main-process services and long-lived resources | `docs/references/lifecycle/README.md`, `docs/references/lifecycle/lifecycle-usage.md`, `docs/references/lifecycle/lifecycle-decision-guide.md` |
| IpcApi routes/events, preload exposure, main handlers, renderer calls, or legacy IPC migration | `docs/references/ipc/README.md`; then `ipc-usage.md` for implementation, `ipc-schema-guide.md` for contracts/naming, and `ipc-migration-guide.md` when legacy IPC is touched |
| Windows | `docs/references/window-manager/README.md` |
| Main-process filesystem paths | `src/main/core/paths/README.md` |
| SQLite services, handlers, seeders, migrations | `docs/references/testing/database-testing.md`, `tests/__mocks__/README.md` |
| UI and shared components | `DESIGN.md`, `packages/ui/`, component usage near the diff |
| Repository skills | `.agents/skills/README.md`, `.agents/skills/create-skill/SKILL.md`, `.agents/skills/gh-pr-review/SKILL.md` |

Treat the listed architecture documents as the authority for their scopes.
Read the relevant sections before judging placement or dependency direction;
nearby code can reflect a documented current deviation and is not a stronger
precedent than the target architecture. Do not load unrelated subsystem guides.

### Internal Skills

Use these skills when they are available in the current runtime:

- Never hard-code machine-local skill paths. Refer to a skill by name and use
  the runtime-provided skill path only when the active environment exposes one.
- `vercel-react-best-practices`: React and Next.js performance, rendering,
  data-fetching, and bundle review.
- `create-skill`: repository-specific skill creation, public skill whitelist,
  `skills:sync`, and Claude symlink rules.
- `skill-creator`: general skill authoring rules, progressive disclosure,
  metadata, references, and validation.
- `gh-create-pr`: PR template compliance when reviewing PR workflow or PR
  documentation changes.
- `cherry-pr-test`: Electron UI test workflow when review findings need local
  app reproduction.

### External Skills And Websites

Use external sources only to clarify framework semantics or to strengthen a
project-specific finding. Do not report an issue solely because an external
source prefers a different style.

| Topic | Reference |
| --- | --- |
| React component composition, boolean-prop growth, compound components | `vercel-composition-patterns`: https://skills.sh/vercel-labs/agent-skills/vercel-composition-patterns |
| Tailwind design systems, tokens, variants, responsive/accessibility patterns | `tailwind-design-system`: https://skills.sh/wshobson/agents/tailwind-design-system |
| Advanced TypeScript types, discriminated unions, conditional/mapped/template literal types | `typescript-advanced-types`: https://skills.sh/wshobson/agents/typescript-advanced-types and https://www.typescriptlang.org/docs/ |
| shadcn/ui composition and component conventions | `shadcn`: https://skills.sh/shadcn/ui/shadcn and https://ui.shadcn.com/docs |
| React Hooks semantics | https://react.dev/reference/react/useEffect, https://react.dev/reference/react/useEffectEvent, https://react.dev/reference/react/useMemo, https://react.dev/reference/react/useCallback, https://react.dev/reference/react/useSyncExternalStore, https://react.dev/learn/you-might-not-need-an-effect |
| SWR cache, mutation, revalidation, and optimistic update semantics | https://swr.vercel.app/docs/getting-started, https://swr.vercel.app/docs/mutation, https://swr.vercel.app/docs/revalidation |
| Tailwind CSS utility semantics | https://tailwindcss.com/docs |

## Naming And Module Shape

Use `docs/references/architecture/naming-conventions.md` as the authority when the diff adds,
renames, or moves a path, changes a primary export's role, or creates a module
boundary. Do not infer the rule from whichever nearby legacy file is easiest to
copy.

Review for:

- File casing matching the primary export and its zone: renderer business
  components use `PascalCase.tsx`; hooks/functions use `camelCase.ts`; class
  files use `PascalCase.ts`; `packages/ui` and renderer route paths use their
  documented `kebab-case` conventions.
- Tests using `*.test.ts(x)`, never `.spec.*`, and case-only renames being safe
  on macOS, Windows, and Linux.
- Stateful singleton capabilities using a class with the correct `Service`
  (default) or `Manager` (homogeneous instance pool) role. Multi-instance
  helper classes and stateless modules must not acquire those suffixes merely
  because they contain methods.
- Single files growing into topic directories only when multiple artifacts
  exist, and domains moving to `features/<domain>/` only when they are large,
  complex, and span concerns.
- `index.ts` being a real, lint-enforced encapsulation boundary: explicit named
  re-exports only, no logic, no `export *`, no nesting, and no `index.tsx`.
- New top-level directories being rejected unless the governing process
  architecture explicitly permits them.

## Main, Renderer, And Shared Architecture

Apply the process-specific architecture document whenever the diff changes
placement, imports, public entry points, or ownership. A documented target/current
deviation is context, not permission to introduce more of the deviation.

For `src/main/`, review for:

- New code routed into the closed top-level set by responsibility; business
  code must not leak into `core/`, and a new capability must not create a new
  top-level directory.
- Dependencies flowing toward the foundation: features stay mutually isolated,
  `ai/` does not import features, and main/preload never import renderer code.
- IPC handlers acting as boundary adapters and resolving owning services through
  `application.get` rather than importing domain implementation directly.
- Topic directories and feature public APIs having one curated entry point,
  while bucket roots such as `services/` and `utils/` have no aggregate barrel.

For `src/renderer/`, review for:

- Dependencies flowing down app/composition -> domain feature -> shared
  renderer layer -> primitives. Shared components/hooks/services must not
  import pages, windows, or features.
- Sibling features not importing one another and pages not importing other
  pages. Cross-domain composition belongs above the features; reusable pieces
  move down to the shared renderer layer.
- External feature consumers entering through the feature's curated `index.ts`;
  no deep imports across the boundary.
- A domain earning `features/<domain>/` only at the documented promotion
  threshold; small pieces remain in the appropriate type bucket.

For `src/shared/`, review for:

- Actual use by both main and renderer before placement in `@shared` (except
  the documented Cache schema-registry carve-out). Prospective reuse is not
  sufficient.
- No exported mutable runtime state or live singleton instances. Shared may
  expose types, pure functions, immutable data, and class blueprints only.
- New code fitting the closed `ai`, `data`, `ipc`, `types`, or `utils` top-level
  set. Single-process code stays in its owning process.
- Topic barrels being curated and bucket roots remaining barrel-free.

## IpcApi Boundary

IpcApi is the default command/RPC boundary for non-data main-process
capabilities. Legacy `IpcChannel` entries describe migration residue, not the
pattern for new work.

Review for:

- SQLite-backed business data using DataApi; user settings using Preference;
  disposable/shared state using Cache; pre-lifecycle flags using BootConfig;
  every other renderer-to-main command using `ipcApi.request` unless it meets a
  documented escape hatch.
- A complete typed route: shared zod schema, main handler, generic preload
  bridge, renderer facade call, and typed errors/events where applicable.
- Handlers remaining thin: validate at the boundary, use `IpcContext` where
  caller identity matters, and delegate stateful business/resource ownership
  to the lifecycle or owning service.
- Route and event names following dot `snake_case`, payload fields remaining
  camelCase, and types being derived from schemas instead of duplicated.
- Main-to-renderer pushes using typed `broadcast`/`send` plus `useIpcOn`;
  high-frequency topic streams use directed send and batching rather than an
  untyped channel.
- Legacy domain migration landing atomically across schema, handler, preload,
  renderer, and obsolete channel deletion. Native exceptions must be explicitly
  sender-validated and documented by the IPC migration guide.

## Data System And DataApi Boundaries

DataApi is for SQLite-backed, irreplaceable business data. It is not a
general-purpose RPC layer.

Flag these as real issues when introduced by the diff:

- A DataApi endpoint wraps process/window control, external service calls,
  notifications, or other pure side effects instead of SQLite business data.
- A handler contains business rules, cross-table query logic, validation
  workflows, or transaction orchestration. Handlers should extract request data,
  call a service, and return the result.
- Renderer code reconstructs business workflows from multiple raw DataApi calls
  when the workflow belongs in a main-process service.
- A new BootConfig key is added without a clear reason it must load before the
  lifecycle system. BootConfig should be extremely rare; ask for tech-lead
  confirmation unless the pre-lifecycle requirement is obvious.
- Row-to-entity mapping leaks SQLite `null`, DB rows, or ORM implementation
  details to renderer DTOs.

When judging system choice:

- Regenerable or disposable data -> Cache.
- Stable user settings with fixed keys -> Preference.
- Process-level config needed before lifecycle -> BootConfig, but only after
  explicit justification.
- User-created, structured, irreplaceable data with a table -> DataApi.
- Pure command / side effect -> IPC or lifecycle service, not DataApi.

## Service Ownership, Cross-Table Access, Transactions

Data services own their domain tables and the business rules around those
tables. Cross-domain collaboration is allowed, but the ownership boundary must
stay visible.

Flag these as issues:

- A service reimplements another domain's business logic instead of calling the
  owning service's public method.
- A service imports another domain's table to bypass validation, soft-delete
  filters, ordering rules, permission checks, or row/entity mapping.
- A cross-table write is split across services without one explicit transaction
  boundary or rollback story.
- A handler coordinates multiple service writes directly. Put orchestration in a
  service method.
- A service opens its own transaction in a method that is used as part of a
  larger workflow, preventing callers from composing one atomic transaction.
- A response embeds full cross-domain objects that can become stale when IDs
  would preserve the boundary.

Do not over-report:

- A read-only `left join` for data matching is acceptable when it does not
  encode another domain's business rules. The reviewer should verify it remains
  read-only and does not replace the owning service's validation or mapping.
- Repository files are strongly discouraged, but a private helper inside the
  owning service is fine for complex query readability.
- A registry service is only for read-only "static preset + DB override" merge
  patterns. It should call the owning entity service for DB data.

## Renderer Data Hooks

`useQuery`, `useMutation`, `useInfiniteQuery`, and `usePaginatedQuery` use SWR
semantics: cache keys, deduplication, stale-while-revalidate, mutation refresh,
optimistic updates, and revalidation ordering.

Review for:

- Unstable query keys caused by including non-result-affecting fields or
  re-created query objects.
- Mixing concrete paths and template paths within one module in a way that makes
  refresh reasoning hard, even if the final cache key is equivalent.
- `refresh` that is too narrow and leaves stale UI, or too broad (`/*` over a
  high-cardinality resource) and revalidates unrelated data.
- Template-path `useMutation` triggered concurrently for different IDs from one
  hook instance. Use per-row concrete-path hooks for parallel writes.
- Optimistic updates without rollback or later revalidation.
- Manual cache writes in `onSuccess` that race with pending revalidation.
- Direct use of `useSWRConfig().cache`, `unstable_serialize`, or raw SWR internals
  outside the sanctioned DataApi cache helpers.

`useCache` and `usePreference` use `useSyncExternalStore`-style external store
semantics. Review for:

- `subscribe` returns cleanup and does not leak listeners.
- `getSnapshot` returns the same value when the store has not changed.
- Mutable stores create new object/array snapshots only when data changes.
- Async initialization is not performed during render.

## React Hooks And UI

React issues are worth reporting when they can cause stale data, missed cleanup,
excessive work in hot paths, or incorrect UI state.

Review for:

- `useEffect` used for pure render-derived state, event-specific logic, or
  parent/child state synchronization that can be handled during render or in an
  event handler.
- Missing effect dependencies, or deleted dependencies used to silence reruns.
- Missing cleanup for listeners, timers, observers, subscriptions, abortable
  requests, and third-party widgets.
- `useEffectEvent` used outside Effect-owned non-reactive callbacks, or used to
  evade dependencies. It is appropriate for subscription/timer callbacks that
  need latest props/state without restarting the Effect.
- `useMemo` used as a correctness mechanism. It is only for expensive
  calculations, stable object/array props to memoized children, or stable hook
  dependencies.
- `useCallback` wrapped around ordinary inline handlers with no identity-sensitive
  consumer. It is useful for `memo` children, hook dependencies, or stable custom
  hook APIs.
- `useMemo` / `useCallback` dependencies that are incomplete or defeated by
  always-new object dependencies.
- Custom hooks that leak internal state-machine details or unstable callbacks to
  callers.

UI-specific checks:

- New UI should use `@cherrystudio/ui` and project design rules.
- User-visible text must use i18n.
- Interactive controls need keyboard behavior and accessible names.
- Prefer established component composition over boolean-prop growth.

## Type And Contract Review

Flag type issues when they create runtime mismatch or caller ambiguity:

- DataApi schema type, runtime validation, and service return shape diverge.
- DTOs expose DB rows, ORM fields, or internal-only persistence details.
- Public unions are not discriminated enough for exhaustive handling.
- Complex generic / conditional types make call-site errors unreadable without
  reducing real runtime risk.
- `null` vs `undefined` semantics are inconsistent across DB row, service entity,
  IPC payload, and renderer type.

## Reporting Shape

Every finding should answer:

1. Where is the code? Give `file:line` and a short snippet.
2. What project boundary or runtime behavior is violated?
3. What realistic failure or maintenance risk follows?
4. What is the smallest reasonable fix or author question?

Use severity language carefully:

- **Blocker**: runtime correctness, data loss, security, broken contract,
  unsafe migration, or high-risk infrastructure change.
- **Warning**: likely maintainability or boundary issue with a clear fix.
- **Notice**: design intent needs author confirmation; do not present as a bug
  unless code evidence shows failure.

# Code Review Checklist

Review in priority order: A (highest impact) → B → C. The reviewer prompt specifies
which levels to check. Test code: only check for obvious implementation errors.

Project rules loaded in context override this checklist.

## React & Performance Deep Reference

For React component and performance reviews, also consult
`vercel-react-best-practices` skill (`../../vercel-react-best-practices/SKILL.md`).
It provides 62 detailed rules covering re-render optimization, bundle size,
async patterns, rendering performance, and advanced React patterns. The checklist
items below (B1, B8, A5, A6) are high-level checks — the Vercel rules provide
specific patterns and code examples for deeper analysis.

## Cherry Studio Deep Reference

For Cherry Studio modules, also apply `cherry-review-guidance.md`. It contains
project-specific rules for DataApi scope, handler/service boundaries, service
ownership, cross-table access, renderer data hooks, React Hooks, UI conventions,
and type contracts. Treat it as project rules loaded in context.

---

## A. Correctness & Safety

Issues that directly affect runtime behavior.

### A1. Code Correctness
- Return values / out-parameters set correctly in all branches (including error paths)
- Implementation matches behavior described by function name / comments
- Conditional logic free of && / || mix-ups, missing negation, precedence errors
- switch/case covers all branches with no unintended fall-through

### A2. Boundary Conditions
> For internal (non-public-API) functions: if callers provably guarantee a
> precondition (e.g., non-null, non-empty, within range), the guard is
> unnecessary — do not flag. Verify the guarantee by reading actual callers.
- Division-by-zero protected (both float and integer)
- Empty container checked before indexing (array[0], .at(0), etc.)
- Null / undefined dereference guarded (especially optional chaining gaps)
- Integer overflow / underflow handled (especially unsigned subtraction)
- Array / string bounds checked

### A3. Error Handling
- I/O operation results checked for errors
- Parse results validated before use (JSON.parse, parseInt, etc.)
- External input validated for legality
- Failed calls have reasonable fallback / safe return
- Promises / async calls properly awaited with error handling (try/catch or .catch)
- IPC calls validated in main process handlers
- DataApi handlers delegate errors through services and `DataApiErrorFactory`
  rather than exposing raw database or IPC failures

### A4. Injection & Sensitive Data
- User input sanitized before DOM insertion (innerHTML, dangerouslySetInnerHTML,
  v-html, document.write, etc.)
- URL parameters, localStorage, postMessage data validated before use
- No hard-coded API keys, tokens, or credentials in client-side code
- Node.js APIs not exposed directly to renderer; use contextBridge in preload
- SQL inputs parameterized (Drizzle ORM prepared statements)

### A5. Resource Management
- Event listeners, timers, subscriptions, observers cleaned up on unmount / scope
  exit (useEffect cleanup)
- File handles / system resources properly closed
- Database connections / network sockets released in finally blocks
- AbortController used and cleaned up for cancellable async operations
- IPC listeners removed when no longer needed

### A6. Memory Safety
- No stale closure captures in useEffect / useCallback / useMemo
- No dangling references to unmounted component state (setState after unmount)
- WeakRef / WeakMap used where appropriate to avoid memory leaks
- Large objects not inadvertently retained in closures

### A7. Thread Safety & Concurrency
> Only flag when the access pattern is clearly unsafe.
- Shared mutable state in main process accessed safely across IPC handlers
- Race conditions in async operations (concurrent state mutations)
- Web Worker message handling with proper serialization
- Electron main/renderer process boundary respected

---

## B. Refactoring & Optimization

Improvements to code quality, performance, and maintainability.

### B1. Performance
- Container space pre-allocated when size is predictable
- No unnecessary deep copies (only flag when semantic equivalence is certain)
- Loop-invariant expressions hoisted outside loops
- Frequent string concatenation inside loops optimized
- No unnecessary temporary object construction
- No unnecessary re-renders from missing memoization, unstable references, or inline
  object/function creation in props (React.memo, useMemo, useCallback)
- No full imports of large dependencies when only a small part is used (tree-shaking)

### B2. Code Simplification
- Clearly duplicated or similar logic extracted (judge by complexity and maintenance
  cost, not count threshold)
- Deep nested if/else simplified with early return
- Redundant conditional checks merged or eliminated
- Overly long functions split into single-responsibility sub-methods

### B3. Module Architecture
> Only flag when the diff introduces a new dependency or moves code across module
> boundaries.
- Module responsibilities clear with no boundary violations
- Main, renderer, and shared placement and dependency direction follow their
  authoritative architecture references
- No circular dependencies
- New command-style cross-process calls use IpcApi; legacy `IpcChannel` entries
  are migration residue, not precedent
- DataApi endpoints are only used for SQLite-backed business data, not pure
  commands or side effects
- Handlers stay thin; business rules, validation, transactions, and row/entity
  mapping live in services
- Services do not reimplement another domain's business logic or bypass the
  owning service's invariants

### B4. Interface Usage
- Called APIs used according to their design intent and documentation
- No use of deprecated interfaces
- Vercel AI SDK v5 patterns followed correctly

### B5. Interface Changes
> Flag only — describe the change and its scope for the coordinator to assess.
- Public API signature or class interface changes identified and described
- IPC channel contract changes identified

### B6. Test Coverage
> Flag only — report for awareness, do not auto-fix.
- Changed logic paths have corresponding test cases
- Boundary conditions have test coverage
- Error paths have test coverage

### B7. Regression Risk
> Flag only — report for awareness, do not auto-fix.
- Modification impact on other callers assessed
- Behavior changes consistent across all target platforms (macOS, Windows, Linux)

### B8. Rendering Correctness
- List items rendered with stable, unique key (not array index)
- Side effects correctly placed in useEffect with proper dependency arrays
- Component state derived correctly (no stale closures, no out-of-sync derived state)
- Renderer data hooks use stable SWR keys, precise refresh targets, safe
  optimistic updates, and stable external-store snapshots
- No new Redux, Dexie, or ElectronStore dependency is introduced on `main`;
  v1 maintenance belongs on the `v1` branch

---

## C. Conventions & Documentation

Coding standards and documentation consistency.

### C1. Project Conventions
- Naming follows `docs/references/architecture/naming-conventions.md`, based on path zone and
  primary export role rather than nearby legacy precedent
- Variable names semantically clear, no unnecessary abbreviations
- Names in new code consistent with style in the same file
- Logging uses `loggerService` with proper context — no `console.log`
- All user-visible strings use i18next — no hardcoded UI strings

### C2. File Organization
- Renderer business components use `PascalCase.tsx`; class-primary files use
  `PascalCase.ts`; hooks/functions use `camelCase.ts`; `packages/ui` and route
  files follow their documented kebab-case rules
- Test files use `*.test.ts(x)`, never `.spec.*`, alongside source or in
  `__tests__/`
- Import order follows simple-import-sort conventions

### C3. Type Safety
- No implicit narrowing conversions
- No `any` types where a proper type exists
- Magic numbers extracted as named constants (unless context already makes meaning
  clear)
- TypeScript strict mode respected

### C4. Const Correctness
- Unmodified variables declared with const
- Objects/arrays that should not be reassigned use const
- Readonly types used for function parameters where appropriate

### C5. Documentation Consistency
- Type names in code consistent with project documentation
- Value ranges in comments consistent with implementation

### C6. Public API Comments
- Public API comments accurately describe current behavior, parameters, return values
- Comments updated when corresponding API behavior changes
- JSDoc present on exported functions/classes where non-obvious

### C7. Accessibility
- Images have meaningful alt text (empty alt for decorative images)
- Form inputs have associated labels
- Interactive elements keyboard-navigable with semantic HTML
- ARIA attributes used correctly with the current component primitives

---

## Exclusion List

> Project rules override this exclusion list. If project rules have explicit requirements
> for an excluded issue type, that type is **not excluded** — review per project rules.

1. Pure style preferences within formatting tool scope (not required by project rules)
2. Formatting already handled by Biome (indentation, whitespace, trailing commas, etc.)
3. Suggestions based on assumed future requirements, not current code
4. Code following project's existing style but not matching some external standard
5. Priority C issues in test code (unless project rules require otherwise)
6. "Better alternative" suggestions for existing stable, bug-free code
7. Missing guards in internal functions when callers provably guarantee the precondition
   (only applies to non-public-API code; verify by reading actual call sites)

# Diagnosis

Analyze the most recent `/gh-pr-review` session in this conversation to find
defects in the skill files themselves — checklist gaps, ambiguous instructions,
missing exclusion rules, etc. The goal is to make the skill more accurate and
reliable, NOT to re-review the project code.

Work entirely from the session context. Only read a skill file when you need to
confirm the exact wording of a rule before suggesting a change.

## Prerequisites

If no `/gh-pr-review` session exists in the current conversation, inform the
user and stop.

## Analyze

Scan the **entire** session from start to finish for these signals and report
findings. Do not stop after finding the first issue — exhaustively check every
user message and system notification. Key evidence includes user rollbacks of
auto-fixes, manual corrections or overrides the user had to provide, steps the
AI deviated from, and user interventions to unblock a stalled flow (e.g., the
user asking "why did you stop?" or manually prompting the AI to continue). For
each finding, state which skill file to change and what the change should be.

### False positives

Issues reported or auto-fixed that the user rejected, reverted, or corrected.
For each: what was wrong, and which checklist item, exclusion rule, or
judgment-matrix rule should be added or revised.

### Judgment errors

Issues where the user disagreed with the risk level or worth-fixing decision
(e.g., reverted an auto-fix, or explicitly overrode a skip). For each: what
the session assigned vs what it should have been, and how to revise
judgment-matrix.md.

### Flow deviations

Steps the AI skipped, reordered, or executed incorrectly. For each: which
step in which file was violated, and how to clarify the instruction.

### Other improvements

Anything else observed in the session that points to a concrete skill file
change — e.g., redundant steps, missing guardrails, unclear wording. Only
include if the change is specific and actionable.

## Apply

If any finding has a concrete file edit, present all actionable edits via
multi-select. Each option label is a one-line summary of the edit. Unchecked
edits are discarded.

Apply selected edits.

# Document Review Checklist

Review in priority order: A (highest impact) → B → C. The reviewer prompt specifies
which levels to check. Project rules loaded in context override this checklist.

---

## A. Accuracy

Issues where the document contains incorrect, contradictory, or incomplete information.

### A1. Code-Document Accuracy
- Described behaviors match actual code implementation
- Parameter names, types, default values consistent with code
- Return values and error conditions accurately documented
- Described algorithms / processing steps consistent with implementation
- Version numbers, format identifiers, constants correct
- Value ranges and constraints accurate
- Enum values and meanings consistent with code definitions
- IpcApi route/event names, schemas, handlers, and payload types agree; legacy
  `IpcChannel` references are identified as migration-only where applicable

### A2. Internal Consistency
- Different sections describing the same concept agree with each other
- Constraints and rules consistent across the document (no contradictions like
  "must be >= 0" in one section with a negative default in another)
- Same rule appearing in multiple places identical in meaning

### A3. Completeness
- All public APIs / features documented
- Newly added features or parameters reflected in the document
- Removed or deprecated features marked accordingly
- Edge cases and limitations documented
- All conditional branches exhaustively covered (no undocumented "else" cases)
- Sequential steps complete with no missing intermediate steps
- Undefined behaviors identified (input combinations with no documented result)

### A4. Reference Validity
> Do not attempt to verify URL reachability.
- Internal cross-references point to existing sections or files
- External links / URLs well-formed and not obviously outdated
- Referenced file paths, tool names, command examples actually exist

---

## B. Clarity & Structure

Improvements to readability, unambiguity, and organization.

### B1. Ambiguity Detection
- No descriptions interpretable in multiple ways
- Conditional statements precise ("should" vs "must", "may" vs "will")
- Boundary conditions clearly stated (inclusive vs exclusive, "at least" vs "exactly")

### B2. Simplification
> Only flag when the same information is stated more than once in different sections.
- Redundant paragraphs or sections repeating the same information consolidated

### B3. Logical Flow
- Information presented in logical order
- Related topics grouped together
- Forward references minimized

### B4. Examples & Illustrations
- Existing examples correct and consistent with described behavior

### B5. Terminology Consistency
- Terms used consistently throughout the document
- Terms match codebase usage (class names, function names, enum values)
- Abbreviations defined on first use

---

## C. Formatting & Style

Polish and stylistic consistency.

### C1. Formatting Consistency
- Headings, lists, tables formatted consistently
- Code snippets properly formatted and syntax-highlighted

### C2. Grammar & Wording
- No grammatical errors or awkward phrasing
- Writing style consistent throughout
- Tone appropriate for target audience

### C3. Section Organization
- Sections appropriately sized (not too long or too short)
- Table of contents (if present) consistent with actual sections
- Deprecated or obsolete sections cleaned up

---

## Exclusion List

> Project rules override this exclusion list.

1. Pure formatting preferences not affecting readability
2. Stylistic rewrites that don't improve clarity or accuracy
3. Suggestions based on assumed future requirements, not current content
4. Suggestions to add content beyond the document's stated scope
5. Removing default or verbose attributes from examples — examples in specs and
   tutorials are intentionally detailed to demonstrate available options

# Judgment Matrix

## Risk Level Assessment

Risk level is per-issue, not per-type — the same category (e.g., rename) can be
low or high risk depending on scope and impact.

| Risk | Rule | Examples |
|------|------|----------|
| Low | Only one reasonable fix exists | null check, fix incorrect comment, rename to match convention, remove redundant duplicate code, fix obvious off-by-one error, missing useEffect cleanup, missing i18n key, over-broad DataApi refresh with an obvious narrower key |
| Medium | Multiple fixes possible, but no design decision or external contract involved | extracting shared logic across functions, removing unused internal methods, simplifying cross-function control flow, adjusting internal module boundaries, moving handler business logic into an existing service method, fixing unstable SWR keys or external-store snapshots |
| High | Involves design decisions or external contracts | public API change (signature, behavior, deprecation), IpcApi contract change, architecture restructuring, algorithm replacement with multiple viable approaches, introducing a new dependency, changing data persistence/serialization format, performance optimization involving space-time trade-offs, user-facing behavior change beyond the stated bug scope, build system configuration change, new DataApi endpoint for non-SQLite side effects, new BootConfig key, cross-service transaction redesign, persistence migration |

## Handling by Risk Level

| `FIX_MODE` | Low risk | Medium risk | High risk |
|------------|----------|-------------|-----------|
| full       | Auto-fix | Auto-fix    | Auto-fix  |
| low_medium | Auto-fix | Auto-fix    | Confirm   |
| low        | Auto-fix | Confirm     | Confirm   |

**Special rule for "full" mode**: issues that would change test baselines
(screenshot comparisons, golden files) are always deferred for user confirmation,
regardless of risk level.

**Legacy-data rule on `main`**: Redux is removed, and Dexie/ElectronStore are
throwaway v1 stacks. Do not repair or extend them. When the diff introduces new
v1 use, report it and route the implementation to Cache, Preference, DataApi,
or the v2 migrators as appropriate. When already editing an area, removal of
dead v1 residue is allowed; unrelated cleanup remains out of scope. A true v1
maintenance fix belongs on the `v1` branch and must not be auto-fixed on
`main`.

## Worth Fixing?

Code-checklist and doc-checklist define **what to look for**. This section
defines **whether to fix** a discovered issue.

### Decision principles

1. **Must fix** — The issue affects runtime correctness, safety, or security.
2. **Fix when clear** — The issue improves code quality (performance,
   simplification, architecture). Fix only when the solution is unambiguous and
   does not introduce new risk. Performance changes require both high confidence
   in semantic equivalence and a net benefit after weighing the gain against
   added code complexity.
3. **Fix when inconsistent** — The issue involves naming, initialization,
   comments, or file organization. Fix only when it violates project rules
   loaded in context or contradicts the surrounding code's established patterns.
4. **Always skip** — Pure style preferences (not violating any consistency
   rule), suggestions based on assumed future requirements rather than current
   code, and alternative implementation rewrites for stable code that has no
   correctness issue.

### Exceptions

- Duplicate code extraction: fix when identical logic is clearly duplicated
  (not by count threshold — judge by complexity and maintenance cost).
- Public API signature changes that are not bug fixes: fix only when justified
  by clear benefit to API consumers. Always high risk.
- Test coverage gaps and regression risks are **flagged, not fixed** — report
  them for the user's awareness rather than auto-fixing.
- `console.log` → `loggerService`: always worth fixing (project convention).
- Hardcoded UI strings → i18n: always worth fixing (project convention).
- DataApi misuse for pure side effects: always worth reporting; fixing is high
  risk if it changes IPC/API contracts.
- Handler-level business logic: worth fixing when the owning service and
  smallest move are clear; otherwise report as a design confirmation.
- Bypassing an owning service's business rules: worth reporting. Do not flag
  read-only `left join` data matching unless it reimplements another domain's
  validation, filtering, ordering, or row mapping.
- Renderer data hook bugs that can leave stale UI, corrupt optimistic state, or
  leak subscriptions are worth reporting.
- New BootConfig keys require explicit pre-lifecycle justification and tech-lead
  confirmation.

## Anti-patterns (Do NOT Fix)

Patterns that frequently produce false positives. Skip unless there is strong
evidence of an actual bug:

- **Speculative optimizations** — build system tweaks, caching additions, or
  conditional guards with no proven failure or measured bottleneck.
- **Documentation example "simplification"** — removing attributes, parameters,
  or steps from examples that are intentionally verbose for pedagogical
  purposes. This is NOT the same as removing redundant code.
- **Behavior changes disguised as bug fixes** — if a proposed fix changes
  observable behavior (not just implementation details), verify the original
  behavior is actually a bug, not an intentional design choice. When intent
  cannot be confirmed from the diff context alone, flag but do not fix.
- **Legacy-stack repairs on `main`** — do not fix Dexie/ElectronStore behavior
  or reintroduce Redux. Report newly introduced dependencies; route v1
  maintenance to the `v1` branch. Removing dead residue in an already-touched
  area is allowed when it cannot affect live v2 behavior.

# Local Review

Single-agent review for local changes. Reviews the diff, presents confirmed
issues, and lets the user interactively choose which ones to fix.

## References

| File | Purpose |
|------|---------|
| `code-checklist.md` | Code review checklist |
| `doc-checklist.md` | Document review checklist |
| `cherry-review-guidance.md` | Cherry Studio project-specific review boundaries |
| `judgment-matrix.md` | Risk levels, worth-fixing criteria, special rules |
| `checklist-evolution.md` | Checklist update flow and rules |

---

## Step 1: Scope

Determine the diff to review based on `$ARGUMENTS` and working tree state:

- **Empty `$ARGUMENTS`**, **uncommitted changes exist**: scope is
  uncommitted changes only. Fetch with `git diff HEAD` (staged + unstaged
  tracked files). Also check for untracked files with `git status --porcelain`
  (`??` lines) and read their contents for review.
- **Empty `$ARGUMENTS`**, **no uncommitted changes**: find the base branch by
  checking common base branches in order: `main`, `master`. Use the first one
  that exists. Fetch the branch diff:
  ```
  git merge-base origin/{base_branch} HEAD
  git diff <merge-base-sha>
  ```
  Also check for untracked files with `git status --porcelain` (`??` lines).
- **Commit hash** (e.g., `abc123`): validate with `git rev-parse --verify`,
  then `git show`.
- **Commit range** (e.g., `abc123..def456` or `abc123...def456`): validate both
  endpoints. Fetch the diff including both endpoints:
  ```
  git diff A~1..B
  ```
- **File/directory paths**: verify all paths exist on disk, then read file
  contents.

If diff is empty → show usage examples and exit:
`/gh-pr-review` (uncommitted changes or current branch),
`/gh-pr-review a1b2c3d`, `/gh-pr-review a1b2c3d..e4f5g6h`,
`/gh-pr-review src/foo.ts`, `/gh-pr-review 123`,
`/gh-pr-review https://github.com/.../pull/123`.

---

## Step 2: Review

Review the diff. Apply `code-checklist.md` to code files,
`doc-checklist.md` to documentation files. Apply `cherry-review-guidance.md` to
code, mixed, Cherry architecture documentation, and project-skill changes. For
React component changes, also consult `vercel-react-best-practices` skill for
detailed performance patterns. When changed lines depend on surrounding context,
read the relevant sections or related definitions as needed. Untracked files
have no diff — review their full contents as new code.

If the branch has an associated GitHub PR, inspect its checks with `gh pr
checks` and include failing or pending CI in the review. Do not run `pnpm lint`,
`pnpm test`, or `pnpm format` locally during review. If no associated PR exists,
state that CI validation is unavailable and keep the result explicitly limited
to static review.

For each issue found:
- Provide a code citation (file:line + snippet) from the current tree.
- Self-verify by re-reading the code — confirm or withdraw.
- If a cited path/line no longer exists, locate the correct file/path via `git diff --name-only` or file search before reporting.

**Output rule**: only present the final confirmed issues to the user. Do not
output analysis process, exclusion reasoning, or issues that were considered
but ruled out.

---

## Step 3: Filter

Consult `judgment-matrix.md` for risk level assessment, worth-fixing criteria,
and special rules. Discard issues that are not worth reporting.

If no issues remain after filtering → report "no issues found" and exit.

---

## Step 4: Report and fix

Present a summary of what was reviewed, then list all confirmed issues. Ask
which ones to fix via multi-select. Each option's label is the issue summary
(e.g., `[risk] file:line — description`). Follow the grouping rule in
`SKILL.md`: ≤4 items → one question; >4 items → group by priority or category
(each group ≤4 options), then present all groups as separate questions in a
single prompt.

If the user selects any issues, apply the fixes. Do not run local lint, test, or
format commands as part of the review flow. Report that existing CI covers the
reviewed commit, not unpushed local fixes; re-check CI only after the fixes are
published through a user-authorized workflow.

---

## Step 5: Checklist evolution

Review all confirmed issues from this session. If any represent a recurring
pattern not covered by the current checklist, read `checklist-evolution.md` and
follow its steps.

# PR Review

PR review uses **Worktree mode** — fetch the PR branch locally so review can
read related code across modules, at the exact version of the PR branch. This
is critical for review accuracy.

## References

| File | Purpose |
|------|---------|
| `code-checklist.md` | Code review checklist |
| `doc-checklist.md` | Document review checklist |
| `cherry-review-guidance.md` | Cherry Studio project-specific review boundaries and reference routing |
| `judgment-matrix.md` | Worth-fixing criteria and special rules |
| `checklist-evolution.md` | Checklist update flow and rules |

---

## Step 1: Create worktree

If `$ARGUMENTS` is a URL, extract the PR number from it.

Validate PR target:
```bash
gh repo view --json nameWithOwner --jq .nameWithOwner
gh pr view {number} --json headRefName,baseRefName,headRefOid,state,body
```
Record `OWNER_REPO`. Extract: `PR_BRANCH`, `BASE_BRANCH`, `HEAD_SHA`, `STATE`,
`PR_BODY`.
If either command fails, inform the user and abort.
If `$ARGUMENTS` is a URL containing `{owner}/{repo}`, verify it matches
`OWNER_REPO`. If not, inform the user that cross-repo PR review is not
supported and abort.
If `STATE` is not `OPEN`, inform the user and exit.

Always create an isolated detached worktree. Never reuse the caller's current
worktree, even when its branch and HEAD match the PR: review must read the exact
remote PR snapshot without mixing in caller-side uncommitted changes. Use a
PR-and-SHA-specific path so unrelated review worktrees are never swept or
deleted. Record the absolute path `/tmp/pr-review-{number}-{short_HEAD_SHA}`
as `REVIEW_DIR` and the caller repository's `git rev-parse --show-toplevel`
result as `MAIN_REPO_DIR` in coordinator state; do not rely on shell variables
or `cd` persisting across tool calls.

Before adding, check whether that exact path is already registered with
`git worktree list --porcelain`. If it exists, reuse it only when it points to
`HEAD_SHA` and `git -C {REVIEW_DIR} status --porcelain` is empty. Never remove
or overwrite a dirty, mismatched, or unrelated worktree without user approval.
Otherwise create it:
```bash
git fetch origin pull/{number}/head
git worktree add --detach "{REVIEW_DIR}" "{HEAD_SHA}"
```

If the fetch fails with `couldn't find remote ref`, the local `origin` is
likely a fork (typical for contributors). Inspect remotes and retry against
`upstream`:
```bash
git remote -v
# If `origin` points to your fork and `upstream` points to the canonical
# repo, fetch from upstream instead:
git fetch upstream pull/{number}/head
git worktree add --detach "{REVIEW_DIR}" "{HEAD_SHA}"
```
If `upstream` is not configured, ask the user for the canonical remote URL
before retrying. Do not guess.

If worktree creation fails for any other reason, inform the user and abort.

For later review and validation filesystem/command calls, pass the recorded
absolute `REVIEW_DIR` as the explicit working directory. For shell snippets
that cannot set a working directory, use `git -C "{REVIEW_DIR}" ...`. Never
assume a prior `cd`, environment variable, or shell session still exists.
Cleanup is the exception: run it from `MAIN_REPO_DIR`, never from inside the
worktree being removed.

> **Platform note (Windows)**: If the active runtime cannot read the Git Bash
> `/tmp/...` path, convert the recorded `REVIEW_DIR` with `cygpath -w` before
> passing it to filesystem tools. On macOS/Linux, use the path as-is.

---

## Step 2: Collect diff and context

```bash
git -C "{REVIEW_DIR}" fetch origin {BASE_BRANCH}
git -C "{REVIEW_DIR}" merge-base origin/{BASE_BRANCH} HEAD
git -C "{REVIEW_DIR}" diff <merge-base-sha>
```
If the diff exceeds 200 lines, first run `git diff --stat` to get an overview,
then read the diff per file using `git -C "{REVIEW_DIR}" diff -- {file}` to
avoid output truncation.

If diff is empty → clean up worktree and exit.

Fetch existing PR review comments for de-duplication:
```bash
gh api repos/{OWNER_REPO}/pulls/{number}/comments
```

Inspect CI with:
```bash
gh pr checks {number} --repo {OWNER_REPO}
```
Record failing, pending, and successful checks as the review's validation
signal. Do not replace CI with local lint, test, or format runs.

---

## Step 3: Review

**Internal analysis**:

1. Based on the diff, read relevant code context as needed to understand the
   change's correctness (e.g., surrounding logic, base classes, callers).
2. Read `PR_BODY` to understand the stated motivation. Verify the
   implementation actually achieves what the author describes.
3. Apply `code-checklist.md` to code files and `doc-checklist.md` to
   documentation files. Apply `cherry-review-guidance.md` to code, mixed,
   Cherry architecture documentation, and project-skill changes, loading only
   the internal references it routes to for the changed areas. For React
   component changes, also consult `vercel-react-best-practices` for detailed
   performance patterns. Use `judgment-matrix.md` to decide whether each issue
   is worth reporting.
4. Check whether issues raised in previous PR comments have been fixed.
5. For each potential issue, perform a second-pass verification: re-read the
   surrounding code and check — is there a guard or early return elsewhere
   that handles this? Does the call chain guarantee preconditions? Am I
   misunderstanding lifetime or ownership?
6. **Discard all ruled-out issues. Keep only issues confirmed to exist.**
7. De-duplicate confirmed issues against existing PR comments.

**Output rule**: only present the final confirmed issues to the user. Do not
output analysis process, exclusion reasoning, or issues that were considered
but ruled out.

---

## Step 4: Clean up and report

If a worktree was created, clean it up:
```bash
git -C "{MAIN_REPO_DIR}" worktree remove "{REVIEW_DIR}"
```

> **Cleanup is best-effort.** If `git worktree remove` fails (e.g.,
> `Permission denied` on Windows when a file handle is still open), the
> review result is still valid — do not block on cleanup. From the main
> repo, run `git worktree prune` to clear stale worktree references; the
> directory can be removed manually afterward. Never force-remove a worktree
> containing unexplained changes; inspect it and request approval first.

Present results to user:
- Summary: one paragraph describing the purpose and scope of the change.
- Overall assessment: code quality evaluation and key improvement directions.
- Issue list (or "no issues found" if clean).

If no issues → ask whether to submit an approval review AND merge the PR:

1. Submit Approval:
   ```bash
   gh pr-review review start --repo {OWNER_REPO} --pr {number}
   # Save the returned review-id
   gh pr-review review submit --repo {OWNER_REPO} --pr {number} \
     --review-id "<review-id>" --event "APPROVE" --body "LGTM"
   ```

2. Merge (squash):
   ```bash
   gh pr merge {number} --squash --delete-branch
   ```

If the user declines, do nothing. Skip the comment submission below.

If issues found → present confirmed issues to user in the following format:

```
{N}. [{priority}] {file}:{line} — {description of the problem and suggested fix}
```

Where `{priority}` is the checklist item ID (e.g., A2, B1, C7). Then ask the
user to select which issues to submit using **a single multi-select question**
where each option's label is the issue summary (e.g.,
`[A2] file:line — description`). User checks multiple options in one prompt.
Unchecked issues are skipped.

### Prerequisites

The `gh-pr-review` extension must be installed. If not present, install it:
```bash
gh extension install EurFelux/gh-pr-review
```

### Submit review via gh-pr-review

Use the `gh-pr-review` extension for structured pending reviews with inline
comments. Do not use `gh pr comment` or raw `gh api` for review submission.

1. Start a pending review:
   ```bash
   gh pr-review review start --repo {OWNER_REPO} --pr {number}
   ```
   Save the returned `id` as `REVIEW_ID`.

2. Add inline comments for each selected issue:
   ```bash
   gh pr-review review add-comment --repo {OWNER_REPO} --pr {number} \
     --review-id "{REVIEW_ID}" \
     --path "{file_path}" \
     --line {line_number} \
     --body "**[{priority}]** {description and suggested fix}"
   ```
   For multi-line ranges:
   ```bash
   gh pr-review review add-comment --repo {OWNER_REPO} --pr {number} \
     --review-id "{REVIEW_ID}" \
     --path "{file_path}" \
     --line {end_line} --start-line {start_line} \
     --body "**[{priority}]** {description and suggested fix}"
   ```

3. Preview before submitting:
   ```bash
   gh pr-review review preview --repo {OWNER_REPO} --pr {number} \
     --review-id "{REVIEW_ID}"
   ```
   Show preview to user and ask for confirmation. Skip if user explicitly
   waives preview.

4. Submit the review:
   ```bash
   gh pr-review review submit --repo {OWNER_REPO} --pr {number} \
     --review-id "{REVIEW_ID}" \
     --event "<COMMENT|REQUEST_CHANGES>" \
     --body "{review summary}"
   ```
   Choose event based on severity:
   - `COMMENT` — observations and suggestions, nothing blocking
   - `REQUEST_CHANGES` — critical or significant issues that must be addressed

**Line number rules:**
- `--line` is the absolute line number in the **new** file (RIGHT side). Must
  be determined during Step 3 by reading the actual file in the worktree — do
  not derive from diff hunk offsets.
- The line must fall within a diff hunk range. Check hunk headers:
  `@@ -oldStart,oldCount +newStart,newCount @@` — valid range for RIGHT side
  is `newStart` to `newStart + newCount - 1`.
- For comments on deleted lines, use `--side LEFT` and line numbers from the
  old file.

**Comment body guidelines:**
- Lead with a bold severity/priority label (e.g., `**[A2]**`, `**[B1]**`).
- Explain the problem clearly.
- Provide a concrete suggestion with code snippet when applicable.
- Write in the user's conversation language.

Summary of issues found / submitted / skipped.

---

## Step 5: Checklist evolution

Review all confirmed issues from this session. If any represent a recurring
pattern not covered by the current checklist, read `checklist-evolution.md` and
follow its steps.

# Teams Review

You are the **coordinator**. Dispatch reviewer, verifier, and fixer agents with
the runtime-provided subagent coordination tools. Never modify source files
directly. Read code only for arbitration, diagnosis, and fix verification.

Always process all auto-fixable issues before involving the user. Do NOT pause
to ask the user anything until Confirm (Phase 5) or Report (Phase 6).

The reviewer–verifier adversarial pair is the core quality mechanism: reviewers
find issues, verifiers challenge them. This two-party check significantly reduces
false positives. Reviewers and verifiers MUST NOT see each other's output or
share conversation history.

## Input from SKILL.md

- `FIX_MODE`: low | low_medium | full

## References

| File | Purpose |
|------|---------|
| `code-checklist.md` | Code review checklist |
| `doc-checklist.md` | Document review checklist |
| `cherry-review-guidance.md` | Cherry Studio project-specific review boundaries |
| `judgment-matrix.md` | Risk levels, worth-fixing criteria, special rules |
| `checklist-evolution.md` | Checklist update flow and rules |

## Flow

```
Scope → Review → Filter → Fix/Validate → Confirm → Report
```

- **Filter** routes auto-fixable issues to Fix/Validate; remaining go to Confirm.
  If nothing to fix or confirm, skip directly to Report.
- **Confirm** ↔ **Fix/Validate** loop until no pending issues remain.

---

## Phase 1: Scope

Determine the diff to review based on `$ARGUMENTS`:

- **Empty arguments**: find the base branch by checking common base branches
  in order: `main`, `master`. Use the first one that exists. Fetch the branch
  diff:
  ```
  git merge-base origin/{base_branch} HEAD
  git diff <merge-base-sha>
  ```
- **Commit hash** (e.g., `abc123`): validate with `git rev-parse --verify`,
  then `git show`.
- **Commit range** (e.g., `abc123..def456` or `abc123...def456`): validate both
  endpoints. Fetch the diff including both endpoints:
  ```
  git diff A~1..B
  ```
- **File/directory paths**: verify all paths exist on disk, then read file
  contents.

If diff is empty → show usage examples and exit:
`/gh-pr-review` (uncommitted changes or current branch),
`/gh-pr-review a1b2c3d`, `/gh-pr-review a1b2c3d..e4f5g6h`,
`/gh-pr-review src/foo.ts`, `/gh-pr-review 123`,
`/gh-pr-review https://github.com/.../pull/123`.

### Associated PR comments

If `gh` is available, check whether the current branch has an open PR:
```
gh pr view --json number,state --jq 'select(.state == "OPEN") | .number' 2>/dev/null
```
If an open PR exists, fetch its line-level review comments:
```
gh api repos/{owner}/{repo}/pulls/{number}/comments
```
Store as `PR_COMMENTS` for verification in the review step.

Also inspect its CI checks with `gh pr checks`. Record failing, pending, and
successful checks as review evidence. Do not run local lint, test, or format
commands during review.

### CI baseline

When an associated PR exists, use `gh pr checks` as the validation baseline and
record failing or pending jobs. If no PR exists, state that CI validation is
unavailable and continue with static review only. Never substitute a local
`pnpm lint`, `pnpm test`, or `pnpm format` run.

### Module partition

Partition files in scope into **review modules** for parallel review. Each
module is a self-contained logical unit. Split large files by section/function
group; group related small files together. Classify each module as `code`,
`doc`, or `mixed`.

Suggested module boundaries for this project:
- `src/main/data/` — DataApi handlers, data services, migrations, schemas
- `src/main/core/` — lifecycle, application, windows, paths, logger
- `src/main/services/` — Main-process business services and side effects
- `src/renderer/data/` — DataApi hooks, Cache, Preference, renderer stores
- `src/renderer/` — React UI components, hooks, pages, features, windows
- `packages/aiCore/` — AI SDK middleware & providers
- `src/shared/` — Cross-process primitives, DataApi/IpcApi schemas, types, pure utilities
- `packages/ui/` — Shared UI primitives
- `src/shared/ipc/`, `src/main/ipc/`, `src/preload/`, `src/renderer/ipc/` — IpcApi contract and bridge
- `docs/references/data/` — Data architecture documentation
- `.agents/skills/` — Agent skills and review instructions

### Issue tracking

The coordinator tracks all issues in memory throughout the session. Each issue
has:
- Brief description
- Status: `pending` | `approved` | `fixed` | `failed` | `skipped`
- Risk: low | medium | high
- File: file path:line
- Proposed fix (medium/high risk only)

---

## Phase 2: Review

### Agent setup

Launch agents with the coordination tools exposed by the current runtime:

- One independent reviewer per module.
- One fresh independent **verifier**, launched after all reviewers complete.

Do not prescribe tool names, agent types, or parameters the runtime does not
expose. Keep reviewer and verifier contexts separate; pass tasks through the
runtime's spawn/delegate interface and collect their returned reports.

**Module merging**: if the total diff is ≤1000 changed lines AND ≤20 files,
merge all modules into a single reviewer. The overhead of multiple agents
(startup, coordination, forwarding) outweighs the parallelism benefit at
this scale.

Launch reviewers concurrently when the runtime supports parallel subagents.

### Reviewer prompt

Stance: **thorough** — discover as many real issues as possible, self-verify
before submitting.

Each reviewer receives:
- **Scope**: file list + changed line ranges for its module. Reviewers fetch
  diffs and read additional context themselves as needed — coordinator does NOT
  pass raw diff or file contents.
- **Checklist**: `code-checklist.md` for code, `doc-checklist.md` for doc, both
  for mixed. Include the checklist content verbatim in the reviewer prompt.
  Include `cherry-review-guidance.md` verbatim for code, mixed, architecture
  documentation, and project-skill modules. For doc-only modules outside Cherry
  architecture/policies, include it only when the document describes project
  behavior, paths, tools, or review rules.
  For React/performance-heavy modules, also include relevant rules from
  `vercel-react-best-practices` skill as supplementary checks.
- **Evidence requirement**: every issue must have a code citation (file:line +
  snippet) from the current tree.
- **Checklist exclusion**: see the exclusion section in the corresponding
  checklist. Project rules loaded in context take priority.
- **Self-check**: before submitting, re-read the relevant code and verify each
  issue. Mark as confirmed or withdrawn. Only submit confirmed issues. If a cited
  path/line no longer exists, locate the correct file/path via `git diff --name-only`
  or file search before reporting.
- **Output format**: `[file:line] [A/B/C] — [description] — [key lines]`

**PR comment reviewer** (when `PR_COMMENTS` exist): one additional agent to
verify PR review comments against current code. Same output format, same
verification pipeline.

### Verification

Stance: **adversarial** — default to doubting the reviewer, actively look for
reasons each issue is wrong. Reject with real evidence, confirm if it holds up.
This step is mandatory — the coordinator MUST NOT skip it or perform
verification itself. **Exception**: if every reviewer explicitly reports zero
issues (LGTM / no issues found), skip verification and proceed directly to
Phase 3.

After all reviewer agents complete, collect their findings. Launch a single
verifier agent with ALL findings combined. Include the following verbatim in
the verifier's prompt:

```
You are a code review verifier. Your stance is adversarial — default to doubting the
reviewer's conclusion and actively look for reasons why the issue might be wrong. Your
job is to stress-test each issue so that only real problems survive.

For each issue you receive:

1. Read the cited code (file:line) and sufficient surrounding context.
2. Actively try to disprove the issue: Is the reviewer's reasoning flawed? Is there
   context that makes this a non-issue (e.g., invariants guaranteed by callers, platform
   constraints, intentional design)? Does the code actually behave as the reviewer
   claims? Look for the strongest counter-argument you can find.
3. Output for each issue:
   - Verdict: REJECT or CONFIRM
   - Reasoning: for REJECT, state the concrete counter-argument. For CONFIRM, briefly
     note what you checked and why no valid counter-argument exists.

Important constraints:
- Your counter-arguments must be grounded in real evidence from the code. Do not
  fabricate hypothetical defenses or invent caller guarantees that are not visible in
  the codebase.
- A CONFIRM verdict is not a failure — it means the reviewer found a real issue and
  your challenge validated it.
```

### After review

Before entering Phase 3, confirm: (1) all reviewers have submitted their final
reports; (2) the verifier has given a CONFIRM/REJECT verdict for every finding,
OR all reviewers reported zero issues and verification was skipped.

---

## Phase 3: Filter — coordinator only

Your stance here is **neutral** — trust no single party. Treat reviewer reports
and verifier rebuttals as equally weighted inputs. Use your project-wide view to
consider cross-module impact, conventions, and architectural intent that local
reviewers may miss.

### 3.1 De-dup

Remove cross-reviewer duplicates (same location, same topic).

### 3.2 Existence check

| Verifier verdict | Action |
|-----------------|--------|
| CONFIRM | Plausibility check — verify description matches cited code. Read code if anything looks off. |
| REJECT | Read code. Evaluate both arguments. Drop only if counter-argument is sound. |

### 3.3 Risk level

Consult `judgment-matrix.md` for risk level assessment, worth-fixing criteria,
handling by risk level, and special rules.

**Fix approach** (Medium/High only): specify the chosen approach and reasoning.
Record in the issue's `Proposed` field. Low risk: single obvious fix, no guidance.

### 3.4 Route

All confirmed issues are recorded with risk level.

| Risk vs `FIX_MODE` | → |
|---------------------|---|
| At or below threshold | auto-fix queue |
| Above threshold | `pending` (for Phase 5 Confirm) |

- Cross-module impact: if a fix requires updates outside the fixer's module,
  add it to the current fix queue and assign to the appropriate fixer.

Always auto-fix eligible issues first — do NOT present `pending` issues to the
user before all auto-fixable issues have been processed and validated.
Phase 4 if auto-fix queue is non-empty. Otherwise jump to Phase 5 if pending
issues exist, or Phase 6 if none.

---

## Phase 4: Fix/Validate

### Fix

Stance: **precise** — apply each fix completely and correctly, never expand
scope. The coordinator MUST NOT apply fixes directly.

**Agent assignment**: launch fixer agents with the runtime-provided coordination
tools. Prefer reusing an existing reviewer only when the runtime preserves that
agent's context; otherwise start a fixer with the minimum verified issue context:

- Issue in a file that a reviewer already analyzed → include that context in the
  fixer prompt.
- Cross-module issues → single fixer agent with all relevant file paths.
- Multi-file renames → single atomic task assigned to one agent.

One agent may receive multiple fix tasks if it covers several files. Avoid
assigning the same file to multiple agents to prevent concurrent edit conflicts.

Each fixer receives (include verbatim in every fixer prompt):

```
Fix rules:
1. Do not stage or commit. The coordinator validates all edits before any commit.
2. Only modify files explicitly assigned by the coordinator.
3. If a fix requires changes to unassigned files, stop and report to the coordinator
   for re-assignment.
4. Keep each issue's edits separable and report the exact changed files.
5. When in doubt, skip the fix rather than risk a wrong change.
6. Do not run build or tests.
7. Do not modify public API function signatures or class definitions (comments are OK),
   unless the coordinator's issue description explicitly requires an API signature fix.
8. After each fix, check whether the change affects related comments or documentation
   within your assigned files (function/class doc-comments, inline comments describing
   the changed logic). If so, update them as part of the same fix.
   Cross-module documentation updates (README, spec files, other modules) are handled
   separately by the coordinator.
9. When done, report the changed files for each fix and list any skipped issues
   with the reason for skipping.
```

Fixers leave all edits uncommitted. The review workflow never stages or commits
fixes: repository policy requires local validation before a commit, while code
review is CI-only. Hand verified patches to a separate user-authorized
publish/commit workflow; that workflow owns the required local checks,
Conventional Commit with a specific kebab-case scope, and `--signoff`. Never
stage pre-existing user changes.

### Verify fixes (coordinator)

Wait for all fixers. Before running validation, the coordinator reads the
working-tree diff for every assigned file and verifies:
1. The fix correctly addresses the original issue
2. No new issues introduced (naming inconsistencies, missing updates in
   surrounding code, logic errors)
3. Fix scope matches the issue — no unintended changes

If a problem is found, launch a correction agent with specific details
(max 1 retry). If the retry fails, mark it `failed`; never discard pre-existing
user changes while removing an unsuccessful fixer edit.

### Validate fixes

Re-read every fixer diff and repeat the relevant reviewer/verifier checks. Do
not run local lint, test, format, or build commands. Existing CI validates the
reviewed remote commit and does not cover unpushed fixes; state that limitation
in the report. If a later user-authorized publish workflow pushes the fixes,
inspect the resulting CI before claiming them fully validated.

- **Static verification passes** → mark issues `fixed`, with CI pending when
  the fix is not yet published.
- **Static verification fails** → retry via a correction agent with failure
  details (max 2 retries). If still unresolved, mark the issue `failed` and ask
  before removing its exact patch; never reset, checkout, or otherwise discard
  unrelated or pre-existing changes.

### After validation

| Condition | → |
|-----------|---|
| `pending` or `failed` issues exist | Phase 5 (Confirm) |
| Otherwise | Phase 6 (Report) |

If Phase 5 approves further fixes, launch new fixer agents and re-enter Phase 4.

---

## Phase 5: Confirm

Present `pending` + `failed` issues grouped by risk (high → low), sorted by
file path within each group:
`[number] [file:line] [risk] [reason] — [description]`

Then present issues via multi-select. Each option label is the issue summary
(e.g., `[risk] file:line — description`).
Checked → `approved`, unchecked → `skipped`.

If the user replies with a bulk instruction (e.g., "fix all", "skip the rest"),
apply it only to issues **at or below** the current `FIX_MODE` threshold.
Issues above the threshold still require individual confirmation.

- **All skipped** → Phase 6.
- **Any approved** → Phase 4 (Fix/Validate). After validation, if more
  `pending`/`failed` remain, return here (Phase 5). If nothing remains,
  proceed to Phase 6.

---

## Phase 6: Report

Summary:
- Issues found / fixed / skipped / failed
- Rolled-back issues and reasons
- Associated PR CI status, or "unavailable" when there is no PR
- Unpushed fixes: static verification only, CI pending
- Issues from PR comments (when `PR_COMMENTS` existed)
- Note: "To verify fix quality, run `/gh-pr-review` again."

### Checklist evolution

Review all confirmed issues from this session. If any represent a recurring
pattern not covered by the current checklist, read `checklist-evolution.md` and
follow its steps.