diff --git a/README.md b/README.md index 5ebe7248..708833b6 100644 --- a/README.md +++ b/README.md @@ -230,12 +230,6 @@ Host matching is case-insensitive after normalization, while namespace and repo matching are case-sensitive after trimming whitespace. An explicit `--profile` bypasses repository routing. Route targets still use the profile's configured auth mode. Passing `--profile ""` is invalid. -For GitHub App reviewer entities, the review profile chooses whether `cr review` -discovers the installation from each PR owner/repo or uses one pinned -installation ID stored in profile config. Discovery is the normal choice for -profiles routed to multiple organizations or users; pinning is only appropriate -when every route for that profile uses the same installation. - Add or replace one credential later: ```bash @@ -260,17 +254,16 @@ target user's credential store. The staging commands must run as the target OS user who will run `cr`, not as root, SYSTEM, or an administrator account whose keyring is different from the target user's keyring. -`reviewer_credentials` may use a PAT or GitHub App auth. A shared reviewer PAT -is an access secret even when distributed org-wide. A GitHub App private key is -also a deployment secret; `cr` signs short-lived JWTs locally, mints -installation tokens as needed, and never stores those minted tokens in -`config.yml`. Pre-stage reviewer credentials into each target user's credential -store, rotate or revoke them with the same `set-credential --overwrite` flow, -and do not store them in config files, agent sources, installers, logs, or -shell profiles. +`reviewer_credentials` must use PAT auth for opinionated GitHub reviews. A +shared reviewer PAT is an access secret even when distributed org-wide. +Pre-stage reviewer credentials into each target user's credential store, rotate +or revoke them with the same `set-credential --overwrite` flow, and do not +store them in config files, agent sources, installers, logs, or shell +profiles. -For GitHub App reviewer credentials, grant the app only the repository -permissions needed by the enabled review workflow: +GitHub App auth remains supported for repository access. When using GitHub App +repository access, grant the app only the repository permissions needed by the +enabled review workflow: | GitHub App permission | Access | Used for | |-----------------------|--------|----------| @@ -303,10 +296,10 @@ profiles: store: local-os name: codereview/work reviewer_credentials: - auth_mode: github_app + auth_mode: pat credential: store: work-1password - name: codereview/work-reviewer-app + name: codereview/work-reviewer llm: provider: anthropic auth: api_key @@ -409,18 +402,12 @@ When deploying a profile without the interactive wizard, run then use `set-credential` only for secrets that you are intentionally staging outside init. Example: -GitHub App reviewer credentials store the App ID and private key only. The -review profile stores installation routing: `discover_from_repository` for PR -context lookup, or `pinned` with one numeric installation ID when every route for -that profile shares the same installation. - ```bash cr --profile work init --non-interactive \ --git-host github.com \ --git-credential-ref codereview/work \ - --reviewer-auth-mode github_app \ - --reviewer-github-app-id "$GITHUB_APP_ID" \ - --reviewer-credential-ref codereview/work-reviewer-app \ + --reviewer-auth-mode pat \ + --reviewer-credential-ref codereview/work-reviewer \ --llm-provider anthropic \ --llm-auth api_key \ --llm-adapter anthropic_api \ @@ -433,10 +420,10 @@ printf '%s' "$USER_GITHUB_TOKEN" | cr set-credential \ --stdin \ --overwrite -printf '%s' "$GITHUB_APP_PRIVATE_KEY" | cr set-credential \ +printf '%s' "$REVIEWER_GITHUB_TOKEN" | cr set-credential \ --store local-os \ - --name codereview/work-reviewer-app \ - --key github_app_private_key \ + --name codereview/work-reviewer \ + --key git_token \ --stdin \ --overwrite @@ -592,8 +579,8 @@ Credential key matrix: |---------------|---------|---------------|---------------|---------------|-------------| | `git.credential` | User Git host auth | `pat` | `git_token` | None | Supported | | `reviewer_credentials.credential` | Reviewer Git host auth | `pat` | `git_token` | None | Supported; must use a distinct credential location from `git.credential` in the same profile | -| `git.github_app.app_id` / reviewer entity `github_app.app_id` | Git host auth | `github_app` | Config field, not a credential key | None | Non-secret GitHub App ID stored in config. Scripted init uses `--git-github-app-id` or `--reviewer-github-app-id` | -| `git.credential` / reviewer entity credential | Git host auth | `github_app` | `github_app_private_key` | None | Supported for GitHub. Reviewer GitHub App installation routing lives on `profiles..reviewer.github_app_installation`, not in the credential bundle | +| `git.github_app.app_id` | Git host auth | `github_app` | Config field, not a credential key | None | Non-secret GitHub App ID stored in config. Scripted init uses `--git-github-app-id`. | +| `git.credential` | Git host auth | `github_app` | `github_app_private_key` | None | Supported for GitHub repository access. | | `git.credential` / `reviewer_credentials.credential` | Git host auth | `oauth_device` | None | None | Reserved; config recognizes the mode but v1 rejects it and does not accept future keys such as `git_oauth_access_token` or `git_oauth_refresh_token` | | `llm.credential` | Anthropic direct API auth | `api_key` + `anthropic` | `anthropic_api_key` | None | Supported | | `llm.credential` | OpenAI direct API auth | `api_key` + `openai` | `openai_api_key` | None | Supported | @@ -754,7 +741,7 @@ Flags: | `--git-token-stdin` | Read the Git token from stdin and write key `git_token`. | | `--git-token-from-env ` | Read the Git token from an environment variable and write key `git_token`. | | `--reviewer-credential-ref ` | Credential name for reviewer Git auth. Defaults to `codereview/-reviewer` when reviewer credentials are requested. | -| `--reviewer-auth-mode ` | Reviewer Git auth mode, default `pat`. `github_app` writes config and prints follow-up credential commands. | +| `--reviewer-auth-mode ` | Reviewer Git auth mode. Only `pat` is supported for reviewer identities. | | `--reviewer-token-stdin` | Read the reviewer Git token from stdin and write key `git_token`; PAT reviewer auth only. | | `--reviewer-token-from-env ` | Read the reviewer Git token from an environment variable and write key `git_token`; PAT reviewer auth only. | | `--llm-provider ` | LLM provider, default `anthropic`; also selects whether API-key ingress writes `anthropic_api_key` or `openai_api_key`. `pi` is adapter-managed and does not accept API-key ingress. | @@ -774,10 +761,8 @@ Flags: Only one stdin secret ingress flag may be used at a time. PAT reviewer credentials use key `git_token` under their own credential name, so `--reviewer-credential-ref` must differ from `--git-credential-ref`. GitHub App -reviewers store their non-secret App ID in config via `--reviewer-github-app-id` -and use `github_app_private_key` as the only credential-store key; installation -routing is profile config, not a credential key. `init` does not accept -reviewer token ingress for `--reviewer-auth-mode github_app`. LLM API-key ingress requires +reviewer identities are not supported because GitHub does not count their +approvals or request-changes toward PR state. LLM API-key ingress requires `--llm-auth api_key`. `--overwrite` with API-key auth requires an LLM key ingress flag. `--allow-self-review` is intentionally runtime-only on `cr review`; `init` only stores the profile-level self-approval policy. diff --git a/docs/architecture.md b/docs/architecture.md new file mode 100644 index 00000000..193b1a78 --- /dev/null +++ b/docs/architecture.md @@ -0,0 +1,111 @@ +# Architecture Decisions + +This document records review-pipeline boundaries that are intended to stay +stable as the implementation evolves. + +## Durable LLM Execution Boundary + +All production structured LLM actions must flow through +`internal/llmlifecycle`. Callers describe the task ID, phase, prompt input, +structured output contract, model/effort, artifact paths, and run/session +scope. The lifecycle runner owns provider invocation, structured-output retry, +provider-session resume, pre-flight reuse, task metadata, accepted-output +artifacts, session persistence, progress breadcrumbs, and failure +classification. + +The final commit marker for a task is `llm-tasks//metadata.json`. Writers +must publish validated output or failed-attempt payloads first, persist the +ledger session row when the task is run-owned, and write metadata last. Resume +code must trust only the final metadata path, never temporary files or partial +payloads. + +New LLM-backed components must not call provider adapters or `internal/llm` +structured helpers directly. They should call `llmlifecycle` through explicit, +fakeable dependencies in unit tests and should return domain results rather +than posting comments or mutating provider state. This guardrail is enforced by +`internal/architecture/llm_lifecycle_test.go`. + +Most lifecycle tasks are run-owned and must have a matching ledger session row +when a provider session is available. Caller-owned no-run tasks are allowed only +where no review run exists yet, such as `SelectionOnly` and the pre-run approval +override classifier. Those tasks may reuse artifact metadata without a ledger +session row, but they still use the same metadata schema and lifecycle runner. + +## Stage Model Resolution + +Runtime model choice must be resolved through `internal/stagemodel`. Code that +executes an LLM stage must not hard-code model IDs and must not call +`config.ResolveModelTier` directly. + +`stagemodel.ResolveStageModel` is the single runtime path from profile +preferences and command overrides to a concrete model and effort. The request +must include the named stage, requested tier, default effort, and any explicit +operator override. The resolver applies user profile `llm.model_map` values, +built-in provider defaults, and configured tier floors before returning the +concrete runtime choice. + +This boundary exists so model catalog data, provider capabilities, token costs, +and profile-level tier floors can be added without touching individual review +stages. Runtime hard-coding bypasses user preference and is a bug. + +Reviewer `agent.model_id` is an exact provider-specific model override. It must +still enter runtime execution through `stagemodel.ResolveStageModel` as a model +override rather than bypassing the resolver, but it intentionally bypasses the +tier map because the agent author selected a concrete model. + +The direct `config.ResolveModelTier` exception is config inspection and the +resolver implementation itself. The architecture guardrail is enforced by +`internal/architecture/model_resolution_test.go`. + +## Git Provider Writes + +Provider writes have one durable path: planned actions in the ledger followed +by outbox execution. Commands and domain analyzers should not post comments, +reply to review threads, resolve threads, submit reviews, or mutate provider +state directly. + +This keeps markers, retries, reconciliation, idempotency, and resume behavior in +one place. New commands such as `cr respond` should produce planned thread +actions and let the reviewplan/ledger/outbox flow perform provider writes. + +## Inline Thread Lifecycle + +Inline PR discussion threads are domain input, not provider-specific prompt +data. The intended decomposition is: + +- `internal/threadcontext` normalizes `gitprovider.InlineThread`, detects + codereview-authored finding threads, detects latest human replies, strips + shared markers, collapses resolved threads to the latest sanitized comment, + and produces file-scoped reviewer context. +- `internal/threadanalysis` accepts normalized thread context and returns + reusable domain decisions: thread ID, decision, reply body, summary, resolve + flag, and rationale. + +Resolved inline threads should not be reprocessed as full conversations on +every review. Their durable context is the latest sanitized comment on the +resolved thread, with marker metadata retained when the comment contains a +codereview thread-summary marker. Reviewer prompts should receive compact +file-scoped summaries so agents avoid re-raising issues that have already been +discussed and resolved. + +`cr review` and `cr respond` should share the same normalization, filtering, +model resolution, LLM execution, and action-planning components. `cr respond` +is a command-shaped reuse of the thread-response portion of the review +pipeline, not a separate posting system. + +## Retention And Cleanup + +Durable run-owned LLM tasks, thread-analysis results, and artifacts must be +owned by a run and must be safe to delete through the existing data lifecycle +commands. Database rows should reference `runs(run_id)` with cascade delete +semantics, and large artifacts should live under the run artifact directory. + +When the retention window elapses, normal prune/GC commands should remove these +results along with the rest of the run. If a user deletes retained state, a +future review may need to spend time and tokens recreating it; that is the +expected tradeoff for user-controlled local data retention. + +Caller-owned no-run task artifacts must live under the configured data root or +an explicit caller artifact root. If no run is eventually allocated for that +artifact root, the directory is treated as orphaned local data and must be safe +for `cr data purge` to remove. diff --git a/docs/config-command-surface-design.md b/docs/config-command-surface-design.md index 650288b7..8383aa89 100644 --- a/docs/config-command-surface-design.md +++ b/docs/config-command-surface-design.md @@ -46,7 +46,7 @@ The codebase already supports: - `repository_profiles` in config schema - repository-aware profile resolution in runtime paths used by `cr review` and `cr agents` -- GitHub App reviewer auth +- PAT-backed reviewer auth only for counted GitHub reviews The missing piece is command coverage. Today `cr config` exposes `show`, `clear`, and `llm models`, but path/route/route-preview/agent-source diff --git a/docs/development.md b/docs/development.md index 2bbf9a2a..c307b76a 100644 --- a/docs/development.md +++ b/docs/development.md @@ -13,8 +13,13 @@ session management, and local data lifecycle commands. The current Go code is a Cobra command tree in `internal/cmd/*` with a thin `cmd/cr` entrypoint, shared exit-code mapping in `internal/cmd/exitcode`, and version plumbing in `internal/version`. Review orchestration is split across -`internal/pipeline`, `internal/reviewrun`, `internal/reviewplan`, -`internal/outbox`, `internal/gate`, and `internal/gateio`. +`internal/pipeline`, `internal/reviewrun`, `internal/threadrespond`, +`internal/reviewplan`, `internal/outbox`, `internal/gate`, and +`internal/gateio`. + +Architecture guardrails for LLM execution, model resolution, Git provider +writes, inline thread lifecycle, and retention live in +[`docs/architecture.md`](architecture.md). Within `internal/pipeline`, the public entry points are `DryRun`, `Live`, and `SelectionOnly`. `DryRun` and `Live` execute the full review pipeline, while @@ -80,17 +85,17 @@ make clean # remove build artifacts state/config adapters in `internal/config`, `internal/ledger`, and `internal/statepaths`, provider/LLM adapters in their owning packages, and review posting/gating in `internal/outbox`, `internal/gate`, and - `internal/gateio`. + `internal/gateio`, and response-only inline discussion handling in + `internal/threadrespond`. ## Interactive Init Notes `cr init` interactive mode keeps all writes draft-local until the user commits -staged changes. Reviewer setup may collect PAT or GitHub App reviewer secrets -inside the reviewer-entity flow, but those values are only written to the -credential store during **Commit staged changes and exit**. When the selected -LLM auth mode is `api_key`, the wizard saves the non-secret profile shape and -prints a follow-up `cr set-credential` command instead of collecting the API key -inline. +staged changes. Reviewer setup may collect PAT reviewer secrets inside the +reviewer-entity flow, but those values are only written to the credential store +during **Commit staged changes and exit**. When the selected LLM auth mode is +`api_key`, the wizard saves the non-secret profile shape and prints a follow-up +`cr set-credential` command instead of collecting the API key inline. Interactive `git.host` edits now route through the repository-route stage when the target profile already participates in `repository_profiles` routing. The diff --git a/docs/init-config-surface.md b/docs/init-config-surface.md index 106b7c86..7f6d137e 100644 --- a/docs/init-config-surface.md +++ b/docs/init-config-surface.md @@ -51,11 +51,11 @@ intentionally unsupported. | config.repository_profiles[].match.repos[] | Route wizard supports namespace-wide route when omitted or repo-specific routes when provided. | Existing repeatable `cr config route set --repo` and `cr config route unset --repo`. | Omitted means namespace-wide route. Existing repos are pre-populated in deterministic order. | Preserve on skip. Add/edit/remove dedupes and sorts via shared route helper. Clear repos converts only when the user explicitly chooses namespace-wide routing. | #177 route helper. #185 repo route and namespace conversion tests. | | config.profiles. | Core profile wizard selects existing profile, creates a new profile, or renames an existing profile. | Existing global `--profile` with `cr init --non-interactive` owns scripted create/replace. Scripted rename is intentionally unsupported by init and belongs to future profile-management command design. | New profile names start blank. Existing profile names are pre-populated. | Create requires a valid profile body. Rename preserves credential locations by default, updates repository routes, and does not delete old credential-store entries. | #177 profile rename helper. #180 tests create/select/rename and validation. | | config.profiles..repository_access | Review profile composition selects one configured repository access definition. | Interactive review-profile flow only for now. Existing `cr init --git-*` flags populate or update repository access during the staged CLI rewrite. | New profiles require an explicit repository access selection. Existing legacy profile Git blocks may be projected into this field during the staged rewrite. | Must reference `config.repository_access`. Route host validation uses the selected repository access Git host. | Repository-access reference validation tests and init profile composition tests. | -| config.reviewer_entities | Configure reviewer entities creates and edits reusable posting identities independently from review profiles. Entity definitions include host, auth mode, credential location, display name, and identity cache. | Interactive main-menu reviewer-entity flow only for now. Future config commands may own scripted reviewer-entity CRUD. | Omitted means no separate reviewer identities are configured yet. Profiles can still choose `git_identity`. Existing entities are pre-populated in the inventory. | Entity names are stable ids. PAT and GitHub App entities store explicit credential locations and never the secret values. Deleting a referenced entity requires affected profiles to select a replacement or use Git identity. | First-class reviewer-entity data model tests and init reviewer-entity UX tests. | +| config.reviewer_entities | Configure reviewer entities creates and edits reusable posting identities independently from review profiles. Entity definitions include host, auth mode, credential location, display name, and identity cache. | Interactive main-menu reviewer-entity flow only for now. Future config commands may own scripted reviewer-entity CRUD. | Omitted means no separate reviewer identities are configured yet. Profiles can still choose `git_identity`. Existing entities are pre-populated in the inventory. | Entity names are stable ids. Reviewer entities are PAT-only for counted GitHub reviews and store explicit credential locations, never secret values. Deleting a referenced entity requires affected profiles to select a replacement or use Git identity. | First-class reviewer-entity data model tests and init reviewer-entity UX tests. | | config.profiles..reviewer.kind | Review profile composition chooses whether posting uses the profile Git account or a configured reviewer entity. | Interactive review-profile flow only for now. | New profiles require an explicit choice. Existing value is pre-populated. | `git_identity` requires empty `reviewer.entity`; `entity` requires an existing reviewer entity whose host matches profile Git host. | First-class reviewer-reference validation tests. | | config.profiles..reviewer.entity | Review profile composition selects one configured reviewer entity when `reviewer.kind` is `entity`. | Interactive review-profile flow only for now. | Empty for `git_identity`. Existing entity reference is pre-populated. | Must reference `config.reviewer_entities`. The entity credential must differ from Git and selected LLM runtime credentials when the store also matches. | First-class reviewer-reference validation tests. | -| config.profiles..reviewer.github_app_installation.mode | Review profile composition chooses how a GitHub App reviewer entity resolves its installation. | Interactive review-profile flow only for now. | Required when `reviewer.entity` uses GitHub App auth. New GitHub App reviewer selections default to `discover_from_repository`. | `discover_from_repository` resolves from PR repository context; `pinned` requires `installation_id`. Must be empty for PAT and Git identity reviewers. | GitHub App reviewer installation routing tests. | -| config.profiles..reviewer.github_app_installation.installation_id | Review profile composition stores an optional pinned GitHub App installation id for this profile. | Interactive review-profile flow only for now. | Empty unless installation mode is `pinned`. | Must be a decimal GitHub App installation id when present. This is profile routing data, not secret material. | GitHub App reviewer installation routing tests. | +| config.profiles..reviewer.github_app_installation.mode | Legacy unsupported reviewer-installation routing field retained only so old configs fail with an actionable validation error. Interactive init must not surface it for new configuration. | None. | Empty for supported configs. | Must remain empty because reviewer identities are PAT-only for counted GitHub reviews. | Validation coverage for legacy-config rejection. | +| config.profiles..reviewer.github_app_installation.installation_id | Legacy unsupported reviewer-installation id retained only so old configs fail with an actionable validation error. Interactive init must not surface it for new configuration. | None. | Empty for supported configs. | Must remain empty because reviewer identities are PAT-only for counted GitHub reviews. | Validation coverage for legacy-config rejection. | | config.profiles..llm_runtime | Review profile composition selects one configured LLM runtime by name. | Interactive review-profile flow only for now. Future config commands may own scripted profile composition. | New profiles require an explicit runtime choice. Existing runtime reference is pre-populated. | Must reference `config.llm_runtimes`. API-key runtime credentials must differ from Git and selected reviewer entity credentials when the store also matches. | First-class LLM-runtime reference validation tests. | | config.profiles..agent_sources[] | Direct trusted-directory editor shows existing profile agent-source paths in one multiline field with explanatory notes about trust and separate repo-local `.codereview/agents` discovery. | Existing `cr init --agent-source`; existing `cr config agent-source add/remove`; #186 docs sequence. | Omitted means no additional profile-specific trusted directories. Existing values are pre-populated line-by-line. | Preserve on skip. Clearing the field removes all profile-specific agent sources. Entered paths are normalized and deduped through the shared config-edit helper. | #177 extracts helper. #183 tests add/remove/reset/preserve. #289 prompt tests cover explanatory copy, multiline entry, and Back. | | config.profiles..review_policy.major_event | Review-policy wizard chooses request_changes or comment. | Existing `cr init --major-event`. | Current init defaults to `request_changes`. Existing value is pre-populated. | Preserve on skip. Set validates enum. Reset clears to normalized default `request_changes`. | #183 tests prompt, reset, and validation. | @@ -132,7 +132,7 @@ secret ingress. |---------|---------------|-------------|---------------|---------------|--------------------------------|----------------| | User Git auth | `pat` | `codereview/` | `git_token` | None | Keep preserves existing store/name and key. Defer stores the credential location and prints follow-up `cr set-credential --store ... --name ...`. Overwrite writes `git_token` through the selected credential store only. | Profile rename preserves the credential location. Regenerate only with explicit key migration or overwrite. | | Reviewer Git auth | `pat` | `codereview/-reviewer`, or label-derived for new interactive reviewer entities | `git_token` | None | Interactive reviewer setup collects `git_token` inline before staging a new or changed reviewer credential location. Keep preserves an unchanged credential location. Clearing the reviewer section removes the location from config but does not delete secrets. | Profile rename and reviewer label edits preserve existing credential locations. No implicit rename/migration. | -| User Git auth or reviewer entity auth | `github_app` | Same purpose-specific defaults as PAT | `github_app_private_key` | None | Keep preserves bundle. GitHub App IDs are non-secret config (`github_app.app_id`). Interactive reviewer setup collects the App ID as config and the private key as the only secret before staging a new or changed reviewer credential location. Reviewer profile installation routing is stored in `profiles..reviewer.github_app_installation`, not the credential bundle. Scripted/deferred flows print a follow-up command for the private key only. Overwrite writes only keys the user provided, with required-key validation before saving. | Migration is explicit only: never leave config pointing at a partially moved credential location. | +| User Git auth | `github_app` | Same purpose-specific defaults as PAT | `github_app_private_key` | None | Keep preserves bundle. GitHub App IDs are non-secret config (`github_app.app_id`). Interactive repository-access setup collects the App ID as config and the private key as the only secret before staging a new or changed Git credential location. Scripted/deferred flows print a follow-up command for the private key only. Overwrite writes only keys the user provided, with required-key validation before saving. | Migration is explicit only: never leave config pointing at a partially moved credential location. | | Git auth | `oauth_device` | None | None | None | Unsupported in v1. The wizard must not offer it as a selectable mode. | Future OAuth work must amend this document. | | LLM API key | `anthropic` + `api_key` | `codereview/-llm` | `anthropic_api_key` | None | Keep preserves existing credential location. Defer stores the location only if the key already exists or follow-up is clearly rendered. Overwrite writes provider key through the selected credential store. | Preserve custom locations on rename. Regeneration requires explicit migration/overwrite. | | LLM API key | `openai` + `api_key` | `codereview/-llm` | `openai_api_key` | None | Same as Anthropic API-key auth. | Same as Anthropic API-key auth. | diff --git a/docs/init-ux-contract.md b/docs/init-ux-contract.md index 2ac3d200..c6d86fc2 100644 --- a/docs/init-ux-contract.md +++ b/docs/init-ux-contract.md @@ -29,6 +29,10 @@ Interactive `init` should use these primary user-facing terms: and GitHub Enterprise hosts such as `github.mycompany.com`. - **Reviewer entity**: the actor that posts `COMMENT`, `APPROVE`, or `REQUEST_CHANGES` on pull requests. + On GitHub, a reviewer entity must resolve to a repository-authorized + identity for `APPROVE` or `REQUEST_CHANGES` to count toward PR state. Live + review stops before LLM or posting work when the selected reviewer can write + a review object but GitHub would not treat it as an opinionated review. - **LLM runtime**: the way reviewer agents run and authenticate, such as Claude CLI subscription auth, Codex CLI subscription auth, Pi local runtime, or a direct API-key-backed provider path. @@ -154,10 +158,10 @@ Interactive `init` must offer both: Credential values may be collected inside the relevant subflow once the user has enough local context to understand why each secret is needed. Reviewer-entity -setup collects the required PAT or GitHub App reviewer secrets inline on the -reviewer details page before a new reviewer can be staged. Those values remain -draft-local until commit; final commit still handles untouched or deferred Git -and LLM credential locations. +setup collects the required PAT reviewer secret inline on the reviewer details +page before a new reviewer can be staged. Those values remain draft-local until +commit; final commit still handles untouched or deferred Git and LLM credential +locations. If the user cancels during credential collection, whether from a subflow or after choosing **Commit staged changes and exit**, any pending secret values remain @@ -169,9 +173,6 @@ entity setup should show non-secret, per-key credential readiness for the selected reviewer credential location: - PAT reviewers show `git_token`. -- GitHub App reviewers show non-secret GitHub App ID as config plus required - `github_app_private_key` readiness. Installation routing is review-profile - config, not a reviewer credential key. - Each key may be shown as `missing`, `existing`, `staged`, `skipped optional`, `deferred`, `optional`, or `status unavailable`. - `missing` means the backend was consulted and no staged or existing value was @@ -198,8 +199,8 @@ follow-up credential work without leaking values. All credential-bearing init flows should show equivalent non-secret destination context before collecting secret values. This includes repository-access Git -credentials, reviewer PAT/GitHub App credentials, and LLM API keys handled by -the shared credential collector. +credentials, reviewer PAT credentials, and LLM API keys handled by the shared +credential collector. Destination summaries should include: @@ -250,7 +251,7 @@ When the UI has additional profile context, it may render equivalent contextual variants of the same fallback choice, such as: - **Post as rianjs (GitHub PAT)** -- **Post as acme-review-bot (GitHub App)** +- **Post as acme-review-bot (GitHub PAT)** - **Post using this profile's Git account (GitHub PAT)** This means: @@ -262,24 +263,23 @@ This means: Interactive `init` may also offer separate reviewer entities such as: - a personal access token (PAT) reviewer -- a GitHub App reviewer -For GitHub organizations, the UX should explain that a GitHub App is often the -preferred team/shared reviewer path. +For GitHub organizations, the UX should explain that reviewer identities must +use PAT auth when the review needs GitHub to count `APPROVE` or +`REQUEST_CHANGES` toward PR state. Separate reviewer entities should also support an optional human-friendly display name. When present, the configured reviewer-entity chooser should prefer that display name and keep the technical reviewer type as supporting text, for example: -- **OC Collective bot (GitHub App reviewer)** - **Release reviewer (PAT reviewer)** When no explicit display name exists, the chooser should fall back to stable, deterministic identity text derived from the credential name or equivalent profile context, for example: -- **open-cli-collective-rianjs-bot (GitHub App reviewer)** +- **open-cli-collective-rianjs-bot (PAT reviewer)** - **reviewer-pat (PAT reviewer)** The profile-Git-account fallback is not a separately named reviewer entity and @@ -310,6 +310,9 @@ and saved config must stay stable: profile selects it with `profiles..reviewer.kind: entity` and `profiles..reviewer.entity`. The Git-account fallback maps to `profiles..reviewer.kind: git_identity`. + A GitHub reviewer entity is not just posting credentials: the resolved + identity must also have repository authority for GitHub to count blocking or + approving reviews toward the PR decision. - **Review profile** maps to one saved entry under `profiles.`. This section is intentionally high level. The detailed field inventory and diff --git a/docs/llm-task-artifacts.md b/docs/llm-task-artifacts.md index e36735d0..0907c983 100644 --- a/docs/llm-task-artifacts.md +++ b/docs/llm-task-artifacts.md @@ -5,7 +5,7 @@ reviewer, and rollup calls must be isolated from each other so one failed task does not erase successful upstream work or force unrelated LLM sessions to run again. -Task artifacts live under a run artifact directory: +Task artifacts usually live under a run artifact directory: ```text llm-tasks// @@ -18,6 +18,8 @@ llm-tasks// Raw failed-attempt files are named `