From 6269af4ecfb55ab985d7edc1ad7fa865d430df00 Mon Sep 17 00:00:00 2001 From: piekstra Date: Mon, 2 Nov 2026 18:13:32 -0500 Subject: [PATCH 1/3] feat(invites): accept repository invitations as the posting identity Add `cr invites list` and `cr invites accept`. A profile's posting identity can now accept pending repository invitations with the token cr already holds, so a reviewer bot can be granted write access without anyone handling its token. accept requires an explicit --owner/--repo allowlist, supports --dry-run, reports every pending invitation as accepted, would_accept, skipped or failed, and exits 1 when any accept failed. The GitHub adapter implements GET /user/repository_invitations and PATCH /user/repository_invitations/{id} behind a new gitprovider.RepositoryInvitations seam; GitLab and GitHub App credentials return config.ErrUnsupported. Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0 --- README.md | 47 ++ cmd/cr/main.go | 2 + cmd/cr/main_test.go | 2 + docs/architecture.md | 10 + docs/development.md | 8 +- .../architecture/command_boundaries_test.go | 1 + .../architecture/invitation_writes_test.go | 64 +++ internal/cmd/invitescmd/invitescmd.go | 310 ++++++++++++ internal/cmd/invitescmd/invitescmd_test.go | 477 ++++++++++++++++++ .../invitescmd/testdata/accept.golden.json | 64 +++ .../cmd/invitescmd/testdata/list.golden.json | 44 ++ internal/cmd/noleak/noleak_test.go | 71 ++- internal/gitprovider/fake.go | 2 + internal/gitprovider/fake_invitations.go | 81 +++ internal/gitprovider/fake_invitations_test.go | 69 +++ internal/gitprovider/github/invitations.go | 104 ++++ .../gitprovider/github/invitations_test.go | 197 ++++++++ .../repository_invitations_page1.json | 72 +++ .../repository_invitations_page2.json | 32 ++ internal/gitprovider/gitlab/invitations.go | 27 + .../gitprovider/gitlab/invitations_test.go | 26 + internal/gitprovider/invitations.go | 37 ++ internal/gitprovider/operation.go | 3 + internal/invitations/invitations.go | 201 ++++++++ internal/invitations/invitations_test.go | 207 ++++++++ internal/view/invites.go | 201 ++++++++ internal/view/invites_test.go | 50 ++ 27 files changed, 2405 insertions(+), 4 deletions(-) create mode 100644 internal/architecture/invitation_writes_test.go create mode 100644 internal/cmd/invitescmd/invitescmd.go create mode 100644 internal/cmd/invitescmd/invitescmd_test.go create mode 100644 internal/cmd/invitescmd/testdata/accept.golden.json create mode 100644 internal/cmd/invitescmd/testdata/list.golden.json create mode 100644 internal/gitprovider/fake_invitations.go create mode 100644 internal/gitprovider/fake_invitations_test.go create mode 100644 internal/gitprovider/github/invitations.go create mode 100644 internal/gitprovider/github/invitations_test.go create mode 100644 internal/gitprovider/github/testdata/repository_invitations_page1.json create mode 100644 internal/gitprovider/github/testdata/repository_invitations_page2.json create mode 100644 internal/gitprovider/gitlab/invitations.go create mode 100644 internal/gitprovider/gitlab/invitations_test.go create mode 100644 internal/gitprovider/invitations.go create mode 100644 internal/invitations/invitations.go create mode 100644 internal/invitations/invitations_test.go create mode 100644 internal/view/invites.go create mode 100644 internal/view/invites_test.go diff --git a/README.md b/README.md index 209e154e..fecd6ea2 100644 --- a/README.md +++ b/README.md @@ -1209,6 +1209,53 @@ and verifies both user Git and reviewer identity credentials when a profile declares both. `--json` emits structured output. `--all` cannot be combined with `--profile`. +### `cr invites list` + +```text +cr invites list --profile [--owner ...] [--repo ...] [--json] +``` + +Lists the pending repository invitations addressed to the profile's posting +identity, the identity `cr review` posts as: its `reviewer_credentials` when +the profile declares them, otherwise its Git credential. Each invitation shows +the repository, inviter, permission, creation time, and whether it has +expired. `--owner` and `--repo` narrow the list; matching ignores case. + +On GitHub, an `APPROVE` counts toward a required review only when the posting +identity has write access to the repository. When the repository owner invites +that identity as a collaborator, `cr invites accept` accepts the invitation +with the token `cr` already holds. The token is never printed or exported. + +Requires a GitHub personal access token for the posting identity. GitHub App +credentials and GitLab profiles fail with a configuration error before the +credential store is opened. GitLab adds existing users as project members +directly, so it has no invitation to accept. + +### `cr invites accept` + +```text +cr invites accept --profile (--owner | --repo )... [--dry-run] [--json] +``` + +Accepts the pending invitations whose repository matches the allowlist. At +least one `--owner` or `--repo` is required; without one the command fails with +a usage error and reads nothing. + +| Flag | Semantics | +|------|-----------| +| `--owner ` | Accept invitations to repositories this account owns. Repeatable. | +| `--repo ` | Accept the invitation to this repository. Repeatable. | +| `--dry-run` | Report what would be accepted without accepting anything. | +| `--json` | Emit JSON. | + +Every pending invitation gets one result: `accepted`, `would_accept` under +`--dry-run`, `skipped` with a reason (not on the allowlist, or expired), or +`failed` with the host's error. One failure does not stop the remaining +accepts. After printing every result, the command exits `1` if any accept +failed. JSON output carries the identity (`profile`, `credential_source`, +`host`, `login`), `dry_run`, the `allowlist`, the per-invitation `results`, and +a `summary` count per status. + ### `cr agents list` ```text diff --git a/cmd/cr/main.go b/cmd/cr/main.go index 5b20df05..481a6239 100644 --- a/cmd/cr/main.go +++ b/cmd/cr/main.go @@ -19,6 +19,7 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/cmd/datacmd" "github.com/open-cli-collective/codereview-cli/internal/cmd/exitcode" "github.com/open-cli-collective/codereview-cli/internal/cmd/initcmd" + "github.com/open-cli-collective/codereview-cli/internal/cmd/invitescmd" "github.com/open-cli-collective/codereview-cli/internal/cmd/mecmd" "github.com/open-cli-collective/codereview-cli/internal/cmd/pireviewtoolcmd" "github.com/open-cli-collective/codereview-cli/internal/cmd/respondcmd" @@ -56,6 +57,7 @@ func buildRootCommand(stdin io.Reader, stdout, stderr io.Writer) (*cobra.Command credentialcmd.Register, initcmd.Register, mecmd.Register, + invitescmd.Register, pireviewtoolcmd.Register, agentscmd.Register, reviewcmd.Register, diff --git a/cmd/cr/main_test.go b/cmd/cr/main_test.go index 2317cd1e..005e5a63 100644 --- a/cmd/cr/main_test.go +++ b/cmd/cr/main_test.go @@ -34,6 +34,8 @@ func TestRun(t *testing.T) { {name: "dash-v is not version", args: []string{"-v"}, wantCode: 2, wantStderr: "unknown shorthand flag", wantEmptyStdout: true}, {name: "help flag shows usage", args: []string{"--help"}, wantCode: 0, wantStdout: "Usage:", wantStdoutSubstring: true}, {name: "me command wired", args: []string{"me", "--help"}, wantCode: 0, wantStdout: "Resolve and cache", wantStdoutSubstring: true}, + {name: "invites command wired", args: []string{"invites", "--help"}, wantCode: 0, wantStdout: "List and accept the pending repository invitations", wantStdoutSubstring: true}, + {name: "invites accept command wired", args: []string{"invites", "accept", "--help"}, wantCode: 0, wantStdout: "Accept the pending invitations whose repository is on the allowlist", wantStdoutSubstring: true}, {name: "agents command wired", args: []string{"agents", "--help"}, wantCode: 0, wantStdout: "Inspect trusted review agents", wantStdoutSubstring: true}, {name: "review command wired", args: []string{"review", "--help"}, wantCode: 0, wantStdout: "Run an automated pull-request review", wantStdoutSubstring: true}, {name: "sessions command wired", args: []string{"sessions", "--help"}, wantCode: 0, wantStdout: "Manage named orchestrator sessions", wantStdoutSubstring: true}, diff --git a/docs/architecture.md b/docs/architecture.md index ac6f8101..7091e923 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -102,6 +102,16 @@ 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. +Accepting a repository invitation is the one exception. It changes the posting +identity's account, not a pull request, so it has no marker, no run, and +nothing to reconcile, and accepting an invitation that is already gone fails +harmlessly. `cr invites accept` therefore writes directly through +`gitprovider.RepositoryInvitations`, a seam separate from `GitProvider` so the +review runtime wrappers never carry it. Only `internal/invitations`, which +enforces the required owner/repository allowlist and `--dry-run`, may call +`AcceptRepositoryInvitation`; `internal/architecture/invitation_writes_test.go` +enforces that. + ## Authenticated Git Reads `internal/gitexec` is the narrow transport adapter for non-interactive Git diff --git a/docs/development.md b/docs/development.md index bbd27e71..2d8ec88c 100644 --- a/docs/development.md +++ b/docs/development.md @@ -9,7 +9,8 @@ codereview-cli is the Open CLI Collective code-review CLI and ships the `cr` binary. It provides configuration and credential commands, trusted-agent inspection, dry-run and live pull-request review orchestration, inline thread response handling through `cr respond`, named orchestrator sessions, automatic -PR-scoped reviewer cohorts, and local data lifecycle commands. +PR-scoped reviewer cohorts, local data lifecycle commands, and `cr invites` for +accepting the posting identity's repository invitations. 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 @@ -198,9 +199,10 @@ release, change `PI_RUNTIME_VERSION` and run the target locally first. 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`, and response-only + `internal/gateio`, response-only inline discussion handling in - `internal/threadrespond`. + `internal/threadrespond`, and allowlisted repository-invitation acceptance + in `internal/invitations`. ## Interactive Init Notes diff --git a/internal/architecture/command_boundaries_test.go b/internal/architecture/command_boundaries_test.go index 4f4959ca..1b7871ed 100644 --- a/internal/architecture/command_boundaries_test.go +++ b/internal/architecture/command_boundaries_test.go @@ -45,6 +45,7 @@ func TestCommandPackageTestsKeepFeatureCommandImportsAtCommandTreeBoundaries(t * "internal/cmd/noleak/noleak_test.go -> github.com/open-cli-collective/codereview-cli/internal/cmd/credentialcmd": "command-surface noleak harness registers the command tree", "internal/cmd/noleak/noleak_test.go -> github.com/open-cli-collective/codereview-cli/internal/cmd/datacmd": "command-surface noleak harness registers the command tree", "internal/cmd/noleak/noleak_test.go -> github.com/open-cli-collective/codereview-cli/internal/cmd/initcmd": "command-surface noleak harness registers the command tree", + "internal/cmd/noleak/noleak_test.go -> github.com/open-cli-collective/codereview-cli/internal/cmd/invitescmd": "command-surface noleak harness registers the command tree", "internal/cmd/noleak/noleak_test.go -> github.com/open-cli-collective/codereview-cli/internal/cmd/mecmd": "command-surface noleak harness registers the command tree", "internal/cmd/noleak/noleak_test.go -> github.com/open-cli-collective/codereview-cli/internal/cmd/reviewcmd": "command-surface noleak harness registers the command tree", "internal/cmd/noleak/noleak_test.go -> github.com/open-cli-collective/codereview-cli/internal/cmd/sessionscmd": "command-surface noleak harness registers the command tree", diff --git a/internal/architecture/invitation_writes_test.go b/internal/architecture/invitation_writes_test.go new file mode 100644 index 00000000..d30b1a27 --- /dev/null +++ b/internal/architecture/invitation_writes_test.go @@ -0,0 +1,64 @@ +package architecture_test + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "path/filepath" + "strings" + "testing" +) + +// Accepting a repository invitation is the one account-level provider write. +// It stays inside internal/invitations, which enforces the allowlist and +// dry-run, so no other caller can accept invitations without them. +func TestRepositoryInvitationAcceptsStayInInvitationsPackage(t *testing.T) { + repoRoot := repoRootFromTest(t) + allowedDirs := []string{ + "internal/invitations", + "internal/gitprovider", + } + + fset := token.NewFileSet() + err := filepath.WalkDir(repoRoot, func(path string, entry fs.DirEntry, walkErr error) error { + if walkErr != nil { + return walkErr + } + if entry.IsDir() { + switch entry.Name() { + case ".git", "dist", "vendor": + return filepath.SkipDir + default: + return nil + } + } + if filepath.Ext(path) != ".go" || strings.HasSuffix(path, "_test.go") { + return nil + } + rel := filepath.ToSlash(mustRel(t, repoRoot, path)) + if pathInAllowedDirs(rel, allowedDirs) { + return nil + } + parsed, err := parser.ParseFile(fset, path, nil, 0) + if err != nil { + return err + } + ast.Inspect(parsed, func(node ast.Node) bool { + call, ok := node.(*ast.CallExpr) + if !ok { + return true + } + selector, ok := call.Fun.(*ast.SelectorExpr) + if !ok || selector.Sel.Name != "AcceptRepositoryInvitation" { + return true + } + t.Fatalf("%s calls AcceptRepositoryInvitation outside internal/invitations", fset.Position(selector.Pos())) + return false + }) + return nil + }) + if err != nil { + t.Fatalf("WalkDir(%s): %v", repoRoot, err) + } +} diff --git a/internal/cmd/invitescmd/invitescmd.go b/internal/cmd/invitescmd/invitescmd.go new file mode 100644 index 00000000..b7ea2e04 --- /dev/null +++ b/internal/cmd/invitescmd/invitescmd.go @@ -0,0 +1,310 @@ +// Package invitescmd wires the `cr invites` command surface. +package invitescmd + +import ( + "context" + "errors" + "fmt" + "io" + + "github.com/open-cli-collective/cli-common/credstore" + "github.com/spf13/cobra" + + "github.com/open-cli-collective/codereview-cli/internal/cmd/cmderr" + "github.com/open-cli-collective/codereview-cli/internal/cmd/cmdruntime" + "github.com/open-cli-collective/codereview-cli/internal/cmd/exitcode" + "github.com/open-cli-collective/codereview-cli/internal/cmd/root" + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/credentials" + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/gitproviders" + "github.com/open-cli-collective/codereview-cli/internal/invitations" + "github.com/open-cli-collective/codereview-cli/internal/view" +) + +// Provider is the provider surface `cr invites` needs: the identity lookup +// plus the account's repository invitations. +type Provider interface { + WhoAmI(ctx context.Context, creds gitprovider.Credential) (gitprovider.Identity, error) + gitprovider.RepositoryInvitations +} + +// ProviderFactory builds the provider authenticated as one profile's posting +// identity. The returned cleanup, when non-nil, closes the credential store. +type ProviderFactory func(cmd *cobra.Command, opts *root.Options, cfg config.File, profileName string, git config.GitConfig) (Provider, gitprovider.Credential, func(), error) + +const ( + sourceGit = "git" + sourceReviewer = "reviewer_credentials" +) + +const invitesLong = `List and accept the pending repository invitations addressed to the selected +profile's posting identity: its reviewer_credentials when the profile declares +them, otherwise its git credential. That is the identity cr review posts as. + +An approval counts toward a required review only when the reviewer identity +has write access to the repository. Accepting the owner's invitation grants +that access using the token cr already holds; cr never prints or exports it. + +Requires a GitHub personal access token. GitHub App credentials and GitLab +profiles are unsupported.` + +const acceptLong = `Accept the pending invitations whose repository is on the allowlist built +from --owner and --repo. At least one is required, so accept never takes every +invitation by default. + +Every pending invitation is reported: accepted, would_accept under --dry-run, +skipped with a reason (not on the allowlist, or expired), or failed with the +host's error. --dry-run accepts nothing. The command prints every result and +then exits 1 if any accept failed.` + +// Register attaches the invites command tree to rootCmd. +func Register(rootCmd *cobra.Command, opts *root.Options) { + RegisterWithFactory(rootCmd, opts, ConfiguredProviderFactory(gitproviders.Options{})) +} + +// RegisterWithFactory attaches the invites command tree with an injected +// provider factory. +func RegisterWithFactory(rootCmd *cobra.Command, opts *root.Options, factory ProviderFactory) { + cmd := &cobra.Command{ + Use: "invites", + Short: "List and accept repository invitations for the posting identity", + Long: invitesLong, + } + cmd.AddCommand(newListCommand(opts, factory), newAcceptCommand(opts, factory)) + rootCmd.AddCommand(cmd) +} + +type allowlistFlags struct { + owners []string + repos []string +} + +func (f *allowlistFlags) add(cmd *cobra.Command, verb string) { + cmd.Flags().StringArrayVar(&f.owners, "owner", nil, verb+" invitations to repositories owned by this login; repeat for multiple owners") + cmd.Flags().StringArrayVar(&f.repos, "repo", nil, verb+" the invitation to this owner/name repository; repeat for multiple repos") +} + +func newListCommand(opts *root.Options, factory ProviderFactory) *cobra.Command { + var filter allowlistFlags + var jsonOutput bool + cmd := &cobra.Command{ + Use: "list", + Short: "List pending repository invitations", + Long: invitesLong, + Example: ` cr invites list --profile reviewer + cr invites list --profile reviewer --owner example-org --json`, + Args: exitcode.NoArgs("invites list takes no arguments"), + RunE: func(cmd *cobra.Command, _ []string) error { + allow, err := invitations.NewAllowlist(filter.owners, filter.repos) + if err != nil { + return exitcode.Usage(err) + } + session, err := openSession(cmd, opts, factory) + if err != nil { + return err + } + defer session.close() + pending, err := invitations.List(cmd.Context(), session.provider, allow) + if err != nil { + return mapRunError(err) + } + result := view.NewInvitesList(session.identity, pending) + return view.Render(opts.Stdout, jsonOutput, result, func(w io.Writer) error { + return view.RenderInvitesListText(w, result) + }) + }, + } + filter.add(cmd, "Only list") + root.AddJSONFlag(cmd, &jsonOutput) + return cmd +} + +func newAcceptCommand(opts *root.Options, factory ProviderFactory) *cobra.Command { + var allowFlags allowlistFlags + var dryRun bool + var jsonOutput bool + cmd := &cobra.Command{ + Use: "accept (--owner | --repo )...", + Short: "Accept allowlisted repository invitations", + Long: acceptLong, + Example: ` cr invites accept --profile reviewer --owner example-org --dry-run + cr invites accept --profile reviewer --owner example-org + cr invites accept --profile reviewer --repo example-org/service --json`, + Args: exitcode.NoArgs("invites accept takes no arguments; name invitations with --owner or --repo"), + RunE: func(cmd *cobra.Command, _ []string) error { + allow, err := invitations.NewAllowlist(allowFlags.owners, allowFlags.repos) + if err != nil { + return exitcode.Usage(err) + } + if allow.Empty() { + return exitcode.Usage(fmt.Errorf("invites accept requires at least one --owner or --repo")) + } + session, err := openSession(cmd, opts, factory) + if err != nil { + return err + } + defer session.close() + outcomes, err := invitations.Accept(cmd.Context(), session.provider, allow, dryRun) + if err != nil { + return mapRunError(err) + } + result := view.NewInvitesAccept(session.identity, allow, dryRun, outcomes) + if err := view.Render(opts.Stdout, jsonOutput, result, func(w io.Writer) error { + return view.RenderInvitesAcceptText(w, result) + }); err != nil { + return err + } + if failed := result.Summary.Failed; failed > 0 { + return exitcode.With(exitcode.Failure, fmt.Errorf("invites accept: %d of %d accept attempts failed", failed, failed+result.Summary.Accepted)) + } + return nil + }, + } + allowFlags.add(cmd, "Accept") + cmd.Flags().BoolVar(&dryRun, "dry-run", false, "Report what would be accepted without accepting anything") + root.AddJSONFlag(cmd, &jsonOutput) + return cmd +} + +type session struct { + provider Provider + identity view.InvitesIdentity + cleanup func() +} + +func (s session) close() { + if s.cleanup != nil { + s.cleanup() + } +} + +// openSession resolves the profile's posting credential, builds the provider, +// and confirms the live identity before any invitation is read. +func openSession(cmd *cobra.Command, opts *root.Options, factory ProviderFactory) (session, error) { + cfg, err := cmdruntime.LoadConfig(opts) + if err != nil { + return session{}, cmderr.Config(err) + } + name, profile, err := config.ResolveProfile(cfg, opts.Profile) + if err != nil { + return session{}, mapRunError(err) + } + source, git := postingGitConfig(profile) + if err := prevalidate(name, source, git); err != nil { + return session{}, mapRunError(err) + } + provider, credential, cleanup, err := factory(cmd, opts, cfg, name, git) + if err != nil { + return session{}, mapRunError(err) + } + opened := session{provider: provider, cleanup: cleanup} + live, err := provider.WhoAmI(cmd.Context(), credential) + if err != nil { + opened.close() + return session{}, mapRunError(err) + } + opened.identity = view.InvitesIdentity{ + Profile: name, + CredentialSource: source, + Host: git.Host, + Login: live.Login, + } + return opened, nil +} + +// postingGitConfig selects the identity cr review posts as, matching cr me. +func postingGitConfig(profile config.Profile) (string, config.GitConfig) { + if profile.ReviewerCredentials == nil { + return sourceGit, profile.Git + } + return sourceReviewer, config.GitConfig{ + Provider: profile.Git.Provider, + Host: profile.Git.Host, + AuthMode: profile.ReviewerCredentials.AuthMode, + Credential: profile.ReviewerCredentials.Credential, + IdentityCache: profile.ReviewerCredentials.IdentityCache, + } +} + +// prevalidate rejects credentials that cannot hold repository invitations +// before the credential store is opened, so no keychain prompt is wasted. +func prevalidate(profileName, source string, git config.GitConfig) error { + field := fmt.Sprintf("profiles.%s.%s.auth_mode", profileName, source) + if !git.AuthMode.Valid() { + return fmt.Errorf("%w: %s %q", config.ErrInvalid, field, git.AuthMode) + } + if git.AuthMode != config.GitAuthModePAT { + return fmt.Errorf("%w: %s %q; repository invitations need a personal access token", config.ErrUnsupported, field, git.AuthMode) + } + if git.ProviderKind() == config.GitProviderGitLab { + return fmt.Errorf("%w: repository invitations for provider gitlab; GitLab adds existing users as project members without an invitation to accept", config.ErrUnsupported) + } + return nil +} + +func mapRunError(err error) error { + switch { + case errors.Is(err, invitations.ErrInvalidAllowlist), + errors.Is(err, invitations.ErrAllowlistRequired): + return exitcode.Usage(err) + case errors.Is(err, config.ErrInvalid), + errors.Is(err, config.ErrNotConfigured), + errors.Is(err, config.ErrProfileNotFound), + errors.Is(err, config.ErrSecretsStoreNotFound), + errors.Is(err, config.ErrUnsupported): + return cmderr.Config(err) + case errors.Is(err, gitprovider.ErrAuth), + errors.Is(err, gitprovider.ErrPermission), + errors.Is(err, gitprovider.ErrRetryable), + errors.Is(err, gitprovider.ErrNotFound), + errors.Is(err, gitprovider.ErrConflict): + return cmderr.Provider(err) + // Keep this list in sync with cmderr.Credential. The explicit guard avoids + // classifying arbitrary errors as credential-domain failures. + case errors.Is(err, credentials.ErrInvalidBackendSelection), + errors.Is(err, credentials.ErrWrongService), + errors.Is(err, credstore.ErrRefEmpty), + errors.Is(err, credstore.ErrRefSegmentCount), + errors.Is(err, credstore.ErrRefInvalidChar), + errors.Is(err, credstore.ErrKeyNotAllowed), + errors.Is(err, credstore.ErrExists), + errors.Is(err, credstore.ErrFilePassphraseRequired), + errors.Is(err, credstore.ErrSecretServiceFailClosed), + errors.Is(err, credstore.ErrStoreClosed), + errors.Is(err, credstore.ErrBackendNotImplemented): + return cmderr.Credential(err) + } + return err +} + +// ConfiguredProviderFactory returns the production factory: it opens the +// profile's credential store and builds the host adapter with options. +func ConfiguredProviderFactory(options gitproviders.Options) ProviderFactory { + return func(cmd *cobra.Command, opts *root.Options, cfg config.File, profileName string, git config.GitConfig) (Provider, gitprovider.Credential, func(), error) { + return newConfiguredProvider(cmd, opts, cfg, profileName, git, options) + } +} + +func newConfiguredProvider(cmd *cobra.Command, opts *root.Options, cfg config.File, profileName string, git config.GitConfig, options gitproviders.Options) (Provider, gitprovider.Credential, func(), error) { + resolved, err := credentials.ResolveSecretsStoreForRef(cfg, git.Credential.Name, profileName) + if err != nil { + return nil, gitprovider.Credential{}, nil, err + } + store, err := credentials.OpenResolvedStore(opts.Backend, cmderr.BackendFlagChanged(cmd), cfg, resolved) + if err != nil { + return nil, gitprovider.Credential{}, nil, err + } + reader := credentials.ProgressStoreReader("invites", root.NewProgressLogger(opts), resolved, store) + client, credential, err := gitproviders.New(git, reader, options) + if err != nil { + _ = store.Close() + return nil, gitprovider.Credential{}, nil, err + } + provider, ok := client.(Provider) + if !ok { + _ = store.Close() + return nil, gitprovider.Credential{}, nil, fmt.Errorf("%w: repository invitations for provider %s", config.ErrUnsupported, git.ProviderKind()) + } + return provider, credential, func() { _ = store.Close() }, nil +} diff --git a/internal/cmd/invitescmd/invitescmd_test.go b/internal/cmd/invitescmd/invitescmd_test.go new file mode 100644 index 00000000..1b84c8be --- /dev/null +++ b/internal/cmd/invitescmd/invitescmd_test.go @@ -0,0 +1,477 @@ +package invitescmd + +import ( + "bytes" + "encoding/json" + "errors" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "reflect" + "strings" + "testing" + "time" + + "github.com/open-cli-collective/cli-common/credstore" + "github.com/spf13/cobra" + + "github.com/open-cli-collective/codereview-cli/internal/cmd/cmdtest" + "github.com/open-cli-collective/codereview-cli/internal/cmd/exitcode" + "github.com/open-cli-collective/codereview-cli/internal/cmd/root" + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/config/configtest" + "github.com/open-cli-collective/codereview-cli/internal/credentials" + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + githubprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/github" + "github.com/open-cli-collective/codereview-cli/internal/gitproviders" + "github.com/open-cli-collective/codereview-cli/internal/view" +) + +func TestAcceptRequiresAllowlistBeforeOpeningCredentials(t *testing.T) { + h := newHarness(t, testConfig()) + for _, args := range [][]string{ + {"--profile", "home", "invites", "accept"}, + {"--profile", "home", "invites", "accept", "--dry-run"}, + } { + _, err := h.run(args...) + if exitcode.FromError(err) != exitcode.UsageError || !strings.Contains(err.Error(), "--owner or --repo") { + t.Fatalf("%v error = %v (exit %d), want usage error naming --owner/--repo", args, err, exitcode.FromError(err)) + } + } + if h.factoryCalls != 0 { + t.Fatalf("factory calls = %d, want 0", h.factoryCalls) + } +} + +func TestMalformedAllowlistIsUsageError(t *testing.T) { + h := newHarness(t, testConfig()) + for _, args := range [][]string{ + {"--profile", "home", "invites", "accept", "--repo", "no-slash"}, + {"--profile", "home", "invites", "accept", "--owner", "org/repo"}, + {"--profile", "home", "invites", "list", "--repo", "a/b/c"}, + {"--profile", "home", "invites", "accept", "--owner", "example-org", "extra"}, + } { + if _, err := h.run(args...); exitcode.FromError(err) != exitcode.UsageError { + t.Fatalf("%v error = %v (exit %d), want usage error", args, err, exitcode.FromError(err)) + } + } + if h.factoryCalls != 0 { + t.Fatalf("factory calls = %d, want 0", h.factoryCalls) + } +} + +func TestAcceptDryRunMakesNoWrites(t *testing.T) { + h := newHarness(t, testConfig()) + var calls []gitprovider.Operation + h.fake.SetCallHook(func(op gitprovider.Operation) { calls = append(calls, op) }) + + out, err := h.run("--profile", "home", "invites", "accept", "--owner", "example-org", "--repo", "other-org/expired", "--dry-run") + if err != nil { + t.Fatalf("Execute: %v\n%s", err, out) + } + if accepted := h.fake.RecordedAcceptedInvitations(); len(accepted) != 0 { + t.Fatalf("accepted = %v, want none under --dry-run", accepted) + } + want := []gitprovider.Operation{gitprovider.OperationWhoAmI, gitprovider.OperationListRepositoryInvitations} + if !reflect.DeepEqual(calls, want) { + t.Fatalf("provider calls = %v, want %v", calls, want) + } + for _, line := range []string{ + "Dry run: true", + " - example-org/docs: would_accept", + " - example-org/service: would_accept", + " - other-org/expired: skipped (invitation expired; ask the inviter to send a new one)", + " - other-org/tooling: skipped (not on the allowlist)", + "Summary: accepted=0 would_accept=2 skipped=2 failed=0", + } { + if !strings.Contains(out, line+"\n") { + t.Fatalf("stdout missing %q:\n%s", line, out) + } + } +} + +func TestAcceptPartialFailureReportsEveryResultThenExitsNonzero(t *testing.T) { + h := newHarness(t, testConfig()) + h.fake.SetAcceptInvitationError("3", gitprovider.WrapError(gitprovider.ErrPermission, gitprovider.OperationAcceptRepositoryInvitation, errors.New("github: status 403"))) + + out, err := h.run("--profile", "home", "invites", "accept", "--owner", "example-org", "--json") + if err == nil { + t.Fatalf("Execute error = nil, want failure\n%s", out) + } + if code := exitcode.FromError(err); code != exitcode.Failure { + t.Fatalf("exit code = %d, want %d", code, exitcode.Failure) + } + if !strings.Contains(err.Error(), "1 of 2 accept attempts failed") { + t.Fatalf("error = %v, want failure count", err) + } + var result view.InvitesAccept + if err := json.Unmarshal([]byte(out), &result); err != nil { + t.Fatalf("stdout is not the JSON result: %v\n%s", err, out) + } + if result.Summary != (view.InvitesAcceptCounts{Accepted: 1, Skipped: 2, Failed: 1}) { + t.Fatalf("summary = %#v", result.Summary) + } + if result.Results[0].Repository != "example-org/docs" || result.Results[0].Status != "failed" || !strings.Contains(result.Results[0].Reason, "permission denied") { + t.Fatalf("first result = %#v, want failed docs invitation with reason", result.Results[0]) + } + if accepted := h.fake.RecordedAcceptedInvitations(); !reflect.DeepEqual(accepted, []gitprovider.InvitationID{"1"}) { + t.Fatalf("accepted = %v, want the service invitation despite the docs failure", accepted) + } +} + +func TestAcceptJSONShape(t *testing.T) { + h := newHarness(t, testConfig()) + + out, err := h.run("--profile", "home", "invites", "accept", "--owner", "example-org", "--repo", "other-org/tooling", "--json") + if err != nil { + t.Fatalf("Execute: %v\n%s", err, out) + } + assertGolden(t, "accept.golden.json", out) +} + +func TestListJSONShapeAndFilter(t *testing.T) { + h := newHarness(t, testConfig()) + + out, err := h.run("--profile", "home", "invites", "list", "--json") + if err != nil { + t.Fatalf("Execute: %v\n%s", err, out) + } + assertGolden(t, "list.golden.json", out) + + out, err = h.run("--profile", "home", "invites", "list", "--repo", "Other-Org/Tooling") + if err != nil { + t.Fatalf("Execute filtered: %v\n%s", err, out) + } + want := strings.Join([]string{ + "Profile: home", + "Credential source: git", + "Host: github.com", + "Login: review-bot", + "Invitations:", + " - other-org/tooling", + " Permission: read", + " Inviter: other-owner", + " Created: 2026-10-29T08:00:00Z", + "", + }, "\n") + if out != want { + t.Fatalf("filtered list =\n%s\nwant\n%s", out, want) + } + if accepted := h.fake.RecordedAcceptedInvitations(); len(accepted) != 0 { + t.Fatalf("list accepted %v, want no writes", accepted) + } +} + +func TestReviewerCredentialsAreThePostingIdentity(t *testing.T) { + h := newHarness(t, testConfig()) + + out, err := h.run("--profile", "work", "invites", "list", "--json") + if err != nil { + t.Fatalf("Execute: %v\n%s", err, out) + } + if h.lastGit.Credential.Name != "codereview/work-reviewer" || h.lastProfile != "work" { + t.Fatalf("factory got profile %q credential %q, want work reviewer credential", h.lastProfile, h.lastGit.Credential.Name) + } + var result view.InvitesList + if err := json.Unmarshal([]byte(out), &result); err != nil { + t.Fatalf("Unmarshal: %v", err) + } + if result.CredentialSource != "reviewer_credentials" { + t.Fatalf("credential_source = %q, want reviewer_credentials", result.CredentialSource) + } +} + +func TestUnsupportedPostingCredentialsFailBeforeOpeningCredentials(t *testing.T) { + cfg := testConfig() + app := cfg.Profiles["work"] + app.ReviewerCredentials.AuthMode = config.GitAuthModeGitHubApp + app.ReviewerCredentials.GitHubApp = &config.GitHubAppConfig{AppID: "12345"} + cfg.Profiles["app"] = app + gitlab := cfg.Profiles["home"] + gitlab.Git.Provider = config.GitProviderGitLab + gitlab.Git.Host = "gitlab.com" + cfg.Profiles["gitlab"] = gitlab + h := newHarness(t, cfg) + + for _, profile := range []string{"app", "gitlab"} { + _, err := h.run("--profile", profile, "invites", "list") + if exitcode.FromError(err) != exitcode.AuthConfigError || !errors.Is(err, config.ErrUnsupported) { + t.Fatalf("%s error = %v (exit %d), want unsupported config error", profile, err, exitcode.FromError(err)) + } + } + if h.factoryCalls != 0 { + t.Fatalf("factory calls = %d, want 0", h.factoryCalls) + } +} + +func TestProviderFailuresMapToExitCodes(t *testing.T) { + tests := []struct { + name string + op gitprovider.Operation + err error + want int + }{ + {name: "identity auth", op: gitprovider.OperationWhoAmI, err: gitprovider.WrapError(gitprovider.ErrAuth, gitprovider.OperationWhoAmI, nil), want: exitcode.AuthConfigError}, + {name: "list retryable", op: gitprovider.OperationListRepositoryInvitations, err: gitprovider.WrapError(gitprovider.ErrRetryable, gitprovider.OperationListRepositoryInvitations, nil), want: exitcode.UpstreamError}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + h := newHarness(t, testConfig()) + h.fake.SetError(tt.op, tt.err) + out, err := h.run("--profile", "home", "invites", "accept", "--owner", "example-org") + if code := exitcode.FromError(err); code != tt.want { + t.Fatalf("exit code = %d (%v), want %d", code, err, tt.want) + } + if out != "" { + t.Fatalf("stdout = %q, want nothing before the identity and listing succeed", out) + } + if !h.closed { + t.Fatal("credential store cleanup did not run") + } + }) + } +} + +func TestMissingProfileIsConfigError(t *testing.T) { + h := newHarness(t, testConfig()) + if _, err := h.run("invites", "list"); exitcode.FromError(err) != exitcode.AuthConfigError || !errors.Is(err, config.ErrProfileNotFound) { + t.Fatalf("error = %v, want profile-not-found config error", err) + } +} + +func TestConfiguredProviderAcceptsWithStoredTokenWithoutPrintingIt(t *testing.T) { + const token = "cr-invites-distinctive-token-0001" // #nosec G101 -- distinctive test canary, not a real credential. + store := openFileStore(t) + if _, err := store.SetBundle("home", map[string]string{credentials.GitTokenKey: token}, credstore.WithOverwrite()); err != nil { + t.Fatalf("SetBundle home: %v", err) + } + var patched []string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("Authorization"); got != "Bearer "+token { + t.Errorf("Authorization = %q, want the stored token", got) + http.Error(w, "bad credential", http.StatusUnauthorized) + return + } + switch { + case r.Method == http.MethodGet && r.URL.EscapedPath() == "/user": + writeJSON(t, w, map[string]any{"login": "review-bot", "id": 9100}) + case r.Method == http.MethodGet && r.URL.EscapedPath() == "/user/repository_invitations": + writeJSON(t, w, []map[string]any{ + { + "id": 301001, + "repository": map[string]any{"name": "service", "owner": map[string]any{"login": "example-org"}}, + "inviter": map[string]any{"login": "example-owner"}, + "permissions": "write", + "created_at": "2026-10-30T14:52:50Z", + }, + { + "id": 301002, + "repository": map[string]any{"name": "docs", "owner": map[string]any{"login": "example-org"}}, + "inviter": map[string]any{"login": "example-owner"}, + "permissions": "write", + "created_at": "2026-10-30T14:52:50Z", + }, + }) + case r.Method == http.MethodPatch && r.URL.EscapedPath() == "/user/repository_invitations/301001": + patched = append(patched, r.URL.EscapedPath()) + w.WriteHeader(http.StatusNoContent) + case r.Method == http.MethodPatch && r.URL.EscapedPath() == "/user/repository_invitations/301002": + patched = append(patched, r.URL.EscapedPath()) + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message":"token ` + token + ` lacks scope"}`)) + default: + t.Errorf("unexpected request %s %s", r.Method, r.URL.String()) + http.NotFound(w, r) + } + })) + defer server.Close() + path := saveConfig(t, fileBackendConfig()) + factory := ConfiguredProviderFactory(gitproviders.Options{GitHub: githubprovider.Options{ + BaseURL: server.URL, + GraphQLURL: server.URL + "/graphql", + }}) + + for _, args := range [][]string{ + {"--profile", "home", "invites", "list"}, + {"--profile", "home", "invites", "list", "--json"}, + {"--profile", "home", "invites", "accept", "--owner", "example-org"}, + {"--profile", "home", "invites", "accept", "--owner", "example-org", "--json"}, + } { + stdout, stderr, err := runWithFactory(path, factory, args...) + for label, data := range map[string]string{"stdout": stdout, "stderr": stderr} { + if err := credstore.NoLeakAssertion([]byte(data), token); err != nil { + t.Fatalf("%v %s leaked the token: %v", args, label, err) + } + } + if err != nil { + if leakErr := credstore.NoLeakAssertion([]byte(err.Error()), token); leakErr != nil { + t.Fatalf("%v error leaked the token: %v", args, leakErr) + } + } + if args[3] == "accept" && exitcode.FromError(err) != exitcode.Failure { + t.Fatalf("%v error = %v, want the 403 to fail the run", args, err) + } + if args[3] == "list" && err != nil { + t.Fatalf("%v error = %v", args, err) + } + } + if len(patched) != 4 { + t.Fatalf("PATCH requests = %v, want both invitations accepted on each of the two runs", patched) + } +} + +type harness struct { + t *testing.T + path string + fake *gitprovider.Fake + factoryCalls int + lastProfile string + lastGit config.GitConfig + closed bool +} + +func newHarness(t *testing.T, cfg config.File) *harness { + t.Helper() + fake := &gitprovider.Fake{} + fake.SetIdentity(gitprovider.Identity{Login: "review-bot", ID: "9100"}) + fake.SetRepositoryInvitations([]gitprovider.RepositoryInvitation{ + testInvitation("1", "example-org", "service", "write", time.Date(2026, 10, 30, 14, 52, 50, 0, time.UTC), false), + testInvitation("2", "other-org", "tooling", "read", time.Date(2026, 10, 29, 8, 0, 0, 0, time.UTC), false), + testInvitation("3", "example-org", "docs", "maintain", time.Date(2026, 10, 31, 9, 15, 0, 0, time.UTC), false), + testInvitation("4", "other-org", "expired", "write", time.Date(2026, 9, 1, 8, 0, 0, 0, time.UTC), true), + }) + return &harness{t: t, path: saveConfig(t, cfg), fake: fake} +} + +func testInvitation(id, owner, repo, permission string, created time.Time, expired bool) gitprovider.RepositoryInvitation { + inviter := "example-owner" + if owner == "other-org" { + inviter = "other-owner" + } + return gitprovider.RepositoryInvitation{ + ID: gitprovider.InvitationID(id), + Owner: owner, + Repo: repo, + Inviter: gitprovider.Identity{Login: inviter}, + Invitee: gitprovider.Identity{Login: "review-bot"}, + Permission: permission, + CreatedAt: created, + Expired: expired, + URL: "https://github.com/" + owner + "/" + repo + "/invitations", + } +} + +func (h *harness) factory(_ *cobra.Command, _ *root.Options, _ config.File, profileName string, git config.GitConfig) (Provider, gitprovider.Credential, func(), error) { + h.factoryCalls++ + h.lastProfile = profileName + h.lastGit = git + return h.fake, gitprovider.Credential{Type: "pat", Token: "fake-token"}, func() { h.closed = true }, nil +} + +func (h *harness) run(args ...string) (string, error) { + h.t.Helper() + stdout, _, err := runWithFactory(h.path, h.factory, args...) + return stdout, err +} + +func runWithFactory(path string, factory ProviderFactory, args ...string) (string, string, error) { + opts := &root.Options{ConfigPath: path, Stdin: strings.NewReader("")} + cmd, stdout, stderr := cmdtest.New(opts, func(cmd *cobra.Command, opts *root.Options) { + RegisterWithFactory(cmd, opts, factory) + }) + err := root.Execute(cmd, args) + return stdout.String(), stderr.String(), err +} + +func assertGolden(t *testing.T, name, got string) { + t.Helper() + path := filepath.Join("testdata", name) + want, err := os.ReadFile(path) // #nosec G304 -- fixed golden names under testdata. + if err != nil { + t.Fatalf("ReadFile(%s): %v", path, err) + } + if got != string(want) { + t.Fatalf("%s differs\ngot:\n%s\nwant:\n%s", name, got, want) + } +} + +func saveConfig(t *testing.T, cfg config.File) string { + t.Helper() + path := filepath.Join(t.TempDir(), "config.yml") + if err := config.Save(path, cfg); err != nil { + t.Fatalf("Save: %v", err) + } + return path +} + +func openFileStore(t *testing.T) *credstore.Store { + t.Helper() + t.Setenv("XDG_DATA_HOME", t.TempDir()) + t.Setenv("CODEREVIEW_KEYRING_PASSPHRASE", "test-passphrase") + store, err := credstore.Open(credentials.ServiceName, &credstore.Options{ + AllowedKeys: credentials.AllowedKeys(), + Backend: credstore.BackendFile, + }) + if err != nil { + t.Fatalf("Open: %v", err) + } + t.Cleanup(func() { _ = store.Close() }) + return store +} + +func fileBackendConfig() config.File { + cfg := testConfig() + cfg.Secrets.Stores["test-file"] = config.SecretsStore{ + DisplayName: "Test File Store", + Backend: config.SecretsStoreBackend{Kind: config.SecretsBackendKind(credstore.BackendFile)}, + } + home := cfg.Profiles["home"] + home.Git.Credential.Store = "test-file" + cfg.Profiles["home"] = home + return cfg +} + +func testConfig() config.File { + llm := config.LLMConfig{ + Provider: config.LLMProviderAnthropic, + Auth: config.LLMAuthSubscription, + Adapter: config.LLMAdapterClaudeCLI, + } + return configtest.File( + configtest.WithoutRepositoryProfiles(), + configtest.HomeProfile(config.Profile{ + Git: config.GitConfig{ + Host: "github.com", + AuthMode: config.GitAuthModePAT, + Credential: config.CredentialLocation{Store: "test-memory", Name: "codereview/home"}, + }, + LLM: llm, + ReviewPolicy: config.ReviewPolicy{MajorEvent: config.ReviewMajorEventComment}, + }), + configtest.Profile("work", config.Profile{ + Git: config.GitConfig{ + Host: "github.com", + AuthMode: config.GitAuthModePAT, + Credential: config.CredentialLocation{Store: "test-memory", Name: "codereview/work"}, + }, + ReviewerCredentials: &config.ReviewerCredentials{ + AuthMode: config.GitAuthModePAT, + Credential: config.CredentialLocation{Store: "test-memory", Name: "codereview/work-reviewer"}, + }, + LLM: llm, + ReviewPolicy: config.ReviewPolicy{MajorEvent: config.ReviewMajorEventComment}, + }), + ) +} + +func writeJSON(t *testing.T, w http.ResponseWriter, value any) { + t.Helper() + var buf bytes.Buffer + if err := json.NewEncoder(&buf).Encode(value); err != nil { + t.Errorf("Encode: %v", err) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write(buf.Bytes()) +} diff --git a/internal/cmd/invitescmd/testdata/accept.golden.json b/internal/cmd/invitescmd/testdata/accept.golden.json new file mode 100644 index 00000000..7efb4091 --- /dev/null +++ b/internal/cmd/invitescmd/testdata/accept.golden.json @@ -0,0 +1,64 @@ +{ + "profile": "home", + "credential_source": "git", + "host": "github.com", + "login": "review-bot", + "dry_run": false, + "allowlist": { + "owners": [ + "example-org" + ], + "repos": [ + "other-org/tooling" + ] + }, + "results": [ + { + "id": "3", + "repository": "example-org/docs", + "inviter": "example-owner", + "permission": "maintain", + "created_at": "2026-10-31T09:15:00Z", + "expired": false, + "url": "https://github.com/example-org/docs/invitations", + "status": "accepted" + }, + { + "id": "1", + "repository": "example-org/service", + "inviter": "example-owner", + "permission": "write", + "created_at": "2026-10-30T14:52:50Z", + "expired": false, + "url": "https://github.com/example-org/service/invitations", + "status": "accepted" + }, + { + "id": "4", + "repository": "other-org/expired", + "inviter": "other-owner", + "permission": "write", + "created_at": "2026-09-01T08:00:00Z", + "expired": true, + "url": "https://github.com/other-org/expired/invitations", + "status": "skipped", + "reason": "not on the allowlist" + }, + { + "id": "2", + "repository": "other-org/tooling", + "inviter": "other-owner", + "permission": "read", + "created_at": "2026-10-29T08:00:00Z", + "expired": false, + "url": "https://github.com/other-org/tooling/invitations", + "status": "accepted" + } + ], + "summary": { + "accepted": 3, + "would_accept": 0, + "skipped": 1, + "failed": 0 + } +} diff --git a/internal/cmd/invitescmd/testdata/list.golden.json b/internal/cmd/invitescmd/testdata/list.golden.json new file mode 100644 index 00000000..70696f9b --- /dev/null +++ b/internal/cmd/invitescmd/testdata/list.golden.json @@ -0,0 +1,44 @@ +{ + "profile": "home", + "credential_source": "git", + "host": "github.com", + "login": "review-bot", + "invitations": [ + { + "id": "3", + "repository": "example-org/docs", + "inviter": "example-owner", + "permission": "maintain", + "created_at": "2026-10-31T09:15:00Z", + "expired": false, + "url": "https://github.com/example-org/docs/invitations" + }, + { + "id": "1", + "repository": "example-org/service", + "inviter": "example-owner", + "permission": "write", + "created_at": "2026-10-30T14:52:50Z", + "expired": false, + "url": "https://github.com/example-org/service/invitations" + }, + { + "id": "4", + "repository": "other-org/expired", + "inviter": "other-owner", + "permission": "write", + "created_at": "2026-09-01T08:00:00Z", + "expired": true, + "url": "https://github.com/other-org/expired/invitations" + }, + { + "id": "2", + "repository": "other-org/tooling", + "inviter": "other-owner", + "permission": "read", + "created_at": "2026-10-29T08:00:00Z", + "expired": false, + "url": "https://github.com/other-org/tooling/invitations" + } + ] +} diff --git a/internal/cmd/noleak/noleak_test.go b/internal/cmd/noleak/noleak_test.go index 791fa4b7..95f21918 100644 --- a/internal/cmd/noleak/noleak_test.go +++ b/internal/cmd/noleak/noleak_test.go @@ -31,6 +31,7 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/cmd/credentialcmd" "github.com/open-cli-collective/codereview-cli/internal/cmd/datacmd" "github.com/open-cli-collective/codereview-cli/internal/cmd/initcmd" + "github.com/open-cli-collective/codereview-cli/internal/cmd/invitescmd" "github.com/open-cli-collective/codereview-cli/internal/cmd/mecmd" "github.com/open-cli-collective/codereview-cli/internal/cmd/reviewcmd" "github.com/open-cli-collective/codereview-cli/internal/cmd/root" @@ -154,6 +155,32 @@ func TestCommandSurfacesDoNotLeakSeededSecrets(t *testing.T) { prepare: seedGitHubAppGitCredentials, args: staticArgs("me", "--json"), }, + { + name: "invites list text", + prepare: seedConfiguredCredentials, + args: staticArgs("invites", "list"), + }, + { + name: "invites list json", + prepare: seedConfiguredCredentials, + args: staticArgs("invites", "list", "--json"), + }, + { + name: "invites accept dry-run text", + prepare: seedConfiguredCredentials, + args: staticArgs("invites", "accept", "--owner", "open-cli-collective", "--dry-run"), + }, + { + name: "invites accept json", + prepare: seedConfiguredCredentials, + args: staticArgs("invites", "accept", "--repo", "open-cli-collective/codereview-cli", "--json"), + }, + { + name: "invites accept failure", + prepare: seedConfiguredCredentials, + args: staticArgs("invites", "accept", "--repo", "open-cli-collective/denied"), + wantErr: true, + }, { name: "agents list json", prepare: seedConfiguredCredentials, @@ -480,6 +507,7 @@ func (h *auditHarness) run(args []string) (string, string, error) { sessionscmd.Register(cmd, opts) mecmd.RegisterWithFactory(cmd, opts, h.identityFactory) agentscmd.RegisterWithFactory(cmd, opts, h.providerFactory) + invitescmd.RegisterWithFactory(cmd, opts, h.invitesFactory) reviewcmd.RegisterWithFactory(cmd, opts, h.reviewRuntimeFactory) err := root.Execute(cmd, args) @@ -492,7 +520,7 @@ func auditArgsWithExplicitProfile(args []string) []string { } command := auditRootCommand(args) switch command { - case "config", "me", "agents", "review": + case "config", "me", "agents", "review", "invites": out := []string{"--profile", "default"} out = append(out, args...) return out @@ -761,6 +789,19 @@ func (h *auditHarness) providerFactory(_ *cobra.Command, _ *root.Options, _ conf return provider, func() { _ = store.Close() }, nil } +func (h *auditHarness) invitesFactory(_ *cobra.Command, _ *root.Options, _ config.File, _ string, git config.GitConfig) (invitescmd.Provider, gitprovider.Credential, func(), error) { + store, err := h.openCredentialStore() + if err != nil { + return nil, gitprovider.Credential{}, nil, err + } + provider, credential, err := h.newGitHubProvider(git, store, nil) + if err != nil { + _ = store.Close() + return nil, gitprovider.Credential{}, nil, err + } + return provider, credential, func() { _ = store.Close() }, nil +} + func (h *auditHarness) reviewRuntimeFactory(ctx context.Context, runtimeOpts app.OpenRequest) (app.Runtime, error) { profile := runtimeOpts.Profile store, err := h.openCredentialStore() @@ -982,6 +1023,34 @@ func (h *auditHarness) handleGitHub(w http.ResponseWriter, r *http.Request) { id = 1005 } writeHTTPJSON(h.t, w, map[string]any{"login": login, "id": id, "name": login}) + case r.Method == http.MethodGet && r.URL.EscapedPath() == "/user/repository_invitations": + if !h.requireBearer(w, r, h.reviewerSecret) { + return + } + invitation := func(id int, repo string) map[string]any { + return map[string]any{ + "id": id, + "repository": map[string]any{"name": repo, "owner": map[string]any{"login": h.prRef.Owner, "id": 1004}}, + "invitee": map[string]any{"login": "reviewer-user", "id": 1002}, + "inviter": map[string]any{"login": "author-user", "id": 1003}, + "permissions": "write", + "created_at": h.now.Format(time.RFC3339), + "html_url": fmt.Sprintf("https://%s/%s/%s/invitations", h.prRef.Host, h.prRef.Owner, repo), + } + } + writeHTTPJSON(h.t, w, []map[string]any{invitation(401, h.prRef.Repo), invitation(402, "denied")}) + case r.Method == http.MethodPatch && r.URL.EscapedPath() == "/user/repository_invitations/401": + if !h.requireBearer(w, r, h.reviewerSecret) { + return + } + w.WriteHeader(http.StatusNoContent) + case r.Method == http.MethodPatch && r.URL.EscapedPath() == "/user/repository_invitations/402": + if !h.requireBearer(w, r, h.reviewerSecret) { + return + } + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusForbidden) + _, _ = fmt.Fprintf(w, `{"message":"credential %s may not accept", "errors":[{"resource":"RepositoryInvitation","code":"forbidden"}]}`, h.reviewerSecret) case r.Method == http.MethodGet && r.URL.EscapedPath() == pullPath && r.Header.Get("Accept") == "application/vnd.github.v3.diff": if !h.requireBearer(w, r, h.gitSecret, h.reviewerSecret, h.githubAppInstallationToken) { return diff --git a/internal/gitprovider/fake.go b/internal/gitprovider/fake.go index e0707ea1..bfe662d1 100644 --- a/internal/gitprovider/fake.go +++ b/internal/gitprovider/fake.go @@ -48,6 +48,8 @@ type Fake struct { issueCommentWrites map[PRRef][]string reviewWrites map[PRRef][]ReviewRequest + invitations fakeInvitations + nextCommentID int nextReviewID int } diff --git a/internal/gitprovider/fake_invitations.go b/internal/gitprovider/fake_invitations.go new file mode 100644 index 00000000..23c3fc14 --- /dev/null +++ b/internal/gitprovider/fake_invitations.go @@ -0,0 +1,81 @@ +package gitprovider + +import "context" + +type fakeInvitations struct { + pending []RepositoryInvitation + acceptErrors map[InvitationID]error + accepted []InvitationID +} + +var _ RepositoryInvitations = (*Fake)(nil) + +// SetRepositoryInvitations configures the pending invitations returned by +// ListRepositoryInvitations. +func (f *Fake) SetRepositoryInvitations(invitations []RepositoryInvitation) { + f.mu.Lock() + defer f.mu.Unlock() + f.invitations.pending = append([]RepositoryInvitation(nil), invitations...) +} + +// SetAcceptInvitationError injects err for accepting one invitation. Passing +// nil clears it. Use SetError with OperationAcceptRepositoryInvitation to fail +// every accept. +func (f *Fake) SetAcceptInvitationError(id InvitationID, err error) { + f.mu.Lock() + defer f.mu.Unlock() + if f.invitations.acceptErrors == nil { + f.invitations.acceptErrors = make(map[InvitationID]error) + } + if err == nil { + delete(f.invitations.acceptErrors, id) + return + } + f.invitations.acceptErrors[id] = err +} + +// ListRepositoryInvitations returns the configured pending invitations. +func (f *Fake) ListRepositoryInvitations(_ context.Context) ([]RepositoryInvitation, error) { + f.call(OperationListRepositoryInvitations) + f.mu.Lock() + defer f.mu.Unlock() + f.ensureLocked() + if err := f.errorLocked(OperationListRepositoryInvitations); err != nil { + return nil, err + } + return append([]RepositoryInvitation(nil), f.invitations.pending...), nil +} + +// AcceptRepositoryInvitation accepts one pending invitation, removing it from +// the pending list and recording the write. +func (f *Fake) AcceptRepositoryInvitation(_ context.Context, id InvitationID) error { + f.call(OperationAcceptRepositoryInvitation) + if err := requireNonEmpty("invitation ID", string(id)); err != nil { + return err + } + f.mu.Lock() + defer f.mu.Unlock() + f.ensureLocked() + if err := f.errorLocked(OperationAcceptRepositoryInvitation); err != nil { + return err + } + if err := f.invitations.acceptErrors[id]; err != nil { + return err + } + for index, invitation := range f.invitations.pending { + if invitation.ID != id { + continue + } + f.invitations.pending = append(f.invitations.pending[:index:index], f.invitations.pending[index+1:]...) + f.invitations.accepted = append(f.invitations.accepted, id) + return nil + } + return WrapError(ErrNotFound, OperationAcceptRepositoryInvitation, nil) +} + +// RecordedAcceptedInvitations returns successful invitation accepts in order. +func (f *Fake) RecordedAcceptedInvitations() []InvitationID { + f.mu.Lock() + defer f.mu.Unlock() + return append([]InvitationID(nil), f.invitations.accepted...) +} diff --git a/internal/gitprovider/fake_invitations_test.go b/internal/gitprovider/fake_invitations_test.go new file mode 100644 index 00000000..390d7b46 --- /dev/null +++ b/internal/gitprovider/fake_invitations_test.go @@ -0,0 +1,69 @@ +package gitprovider + +import ( + "context" + "errors" + "reflect" + "testing" +) + +func TestFakeRepositoryInvitationsAcceptRemovesPendingAndRecords(t *testing.T) { + var fake Fake + var calls []Operation + fake.SetCallHook(func(op Operation) { calls = append(calls, op) }) + fake.SetRepositoryInvitations([]RepositoryInvitation{ + {ID: "1", Owner: "example-org", Repo: "service"}, + {ID: "2", Owner: "example-org", Repo: "docs"}, + }) + + if err := fake.AcceptRepositoryInvitation(context.Background(), "1"); err != nil { + t.Fatalf("AcceptRepositoryInvitation: %v", err) + } + pending, err := fake.ListRepositoryInvitations(context.Background()) + if err != nil { + t.Fatalf("ListRepositoryInvitations: %v", err) + } + if len(pending) != 1 || pending[0].ID != "2" { + t.Fatalf("pending = %#v, want only invitation 2", pending) + } + if got := fake.RecordedAcceptedInvitations(); !reflect.DeepEqual(got, []InvitationID{"1"}) { + t.Fatalf("accepted = %#v, want [1]", got) + } + if err := fake.AcceptRepositoryInvitation(context.Background(), "1"); !errors.Is(err, ErrNotFound) { + t.Fatalf("second accept error = %v, want ErrNotFound", err) + } + if err := fake.AcceptRepositoryInvitation(context.Background(), " "); err == nil { + t.Fatal("blank invitation ID error = nil") + } + want := []Operation{OperationAcceptRepositoryInvitation, OperationListRepositoryInvitations, OperationAcceptRepositoryInvitation, OperationAcceptRepositoryInvitation} + if !reflect.DeepEqual(calls, want) { + t.Fatalf("calls = %#v, want %#v", calls, want) + } +} + +func TestFakeRepositoryInvitationsInjectErrors(t *testing.T) { + var fake Fake + fake.SetRepositoryInvitations([]RepositoryInvitation{{ID: "1", Owner: "o", Repo: "a"}, {ID: "2", Owner: "o", Repo: "b"}}) + perID := errors.New("per-id failure") + fake.SetAcceptInvitationError("1", perID) + + if err := fake.AcceptRepositoryInvitation(context.Background(), "1"); !errors.Is(err, perID) { + t.Fatalf("accept 1 error = %v, want per-id failure", err) + } + if err := fake.AcceptRepositoryInvitation(context.Background(), "2"); err != nil { + t.Fatalf("accept 2: %v", err) + } + fake.SetAcceptInvitationError("1", nil) + opErr := errors.New("op failure") + fake.SetError(OperationAcceptRepositoryInvitation, opErr) + if err := fake.AcceptRepositoryInvitation(context.Background(), "1"); !errors.Is(err, opErr) { + t.Fatalf("accept 1 with op error = %v, want op failure", err) + } + fake.SetError(OperationListRepositoryInvitations, opErr) + if _, err := fake.ListRepositoryInvitations(context.Background()); !errors.Is(err, opErr) { + t.Fatalf("list error = %v, want op failure", err) + } + if got := fake.RecordedAcceptedInvitations(); !reflect.DeepEqual(got, []InvitationID{"2"}) { + t.Fatalf("accepted = %#v, want [2]", got) + } +} diff --git a/internal/gitprovider/github/invitations.go b/internal/gitprovider/github/invitations.go new file mode 100644 index 00000000..45a8476d --- /dev/null +++ b/internal/gitprovider/github/invitations.go @@ -0,0 +1,104 @@ +package github + +import ( + "context" + "fmt" + "net/http" + "strconv" + "strings" + "time" + + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" +) + +type repositoryInvitationResponse struct { + ID int64 `json:"id"` + Repository repoResponse `json:"repository"` + Invitee *userResponse `json:"invitee"` + Inviter *userResponse `json:"inviter"` + Permissions string `json:"permissions"` + CreatedAt time.Time `json:"created_at"` + Expired bool `json:"expired"` + HTMLURL string `json:"html_url"` +} + +var _ gitprovider.RepositoryInvitations = (*Client)(nil) + +// ListRepositoryInvitations returns the pending repository invitations +// addressed to the authenticated user. +func (c *Client) ListRepositoryInvitations(ctx context.Context) ([]gitprovider.RepositoryInvitation, error) { + const op = gitprovider.OperationListRepositoryInvitations + if err := c.requireUserToken(op); err != nil { + return nil, err + } + endpoint := restURL(c.baseURL, "user", "repository_invitations") + "?per_page=100" + payloads, err := doRESTPages[repositoryInvitationResponse](ctx, c, op, endpoint, acceptJSON) + if err != nil { + return nil, err + } + invitations := make([]gitprovider.RepositoryInvitation, 0, len(payloads)) + for _, payload := range payloads { + invitation, err := repositoryInvitationFromResponse(payload) + if err != nil { + return nil, err + } + invitations = append(invitations, invitation) + } + return invitations, nil +} + +// AcceptRepositoryInvitation accepts one pending repository invitation for +// the authenticated user. +func (c *Client) AcceptRepositoryInvitation(ctx context.Context, id gitprovider.InvitationID) error { + const op = gitprovider.OperationAcceptRepositoryInvitation + if err := c.requireUserToken(op); err != nil { + return err + } + numericID, err := strconv.ParseInt(strings.TrimSpace(string(id)), 10, 64) + if err != nil || numericID <= 0 { + return fmt.Errorf("%w: invitation ID %q must be a positive integer", ErrValidation, id) + } + endpoint := restURL(c.baseURL, "user", "repository_invitations", strconv.FormatInt(numericID, 10)) + _, _, err = c.doREST(ctx, op, http.MethodPatch, endpoint, acceptJSON, nil) + return err +} + +// requireUserToken rejects GitHub App clients: installation tokens act for an +// app installation, which has no repository invitations of its own. +func (c *Client) requireUserToken(op gitprovider.Operation) error { + if c == nil { + return fmt.Errorf("%w: nil client", ErrValidation) + } + if c.appAuth != nil { + return fmt.Errorf("%w: %s requires a personal access token; GitHub App installations are granted repository access by installation, not invitation", config.ErrUnsupported, op) + } + return nil +} + +func repositoryInvitationFromResponse(payload repositoryInvitationResponse) (gitprovider.RepositoryInvitation, error) { + if payload.ID <= 0 { + return gitprovider.RepositoryInvitation{}, fmt.Errorf("%w: repository invitation without an ID", ErrValidation) + } + owner := strings.TrimSpace(payload.Repository.Owner.Login) + repo := strings.TrimSpace(payload.Repository.Name) + if owner == "" || repo == "" { + return gitprovider.RepositoryInvitation{}, fmt.Errorf("%w: repository invitation %d without a repository owner and name", ErrValidation, payload.ID) + } + invitation := gitprovider.RepositoryInvitation{ + ID: gitprovider.InvitationID(strconv.FormatInt(payload.ID, 10)), + Owner: owner, + Repo: repo, + Permission: strings.TrimSpace(payload.Permissions), + CreatedAt: payload.CreatedAt, + Expired: payload.Expired, + URL: payload.HTMLURL, + } + if payload.Inviter != nil { + invitation.Inviter = identityFromUser(*payload.Inviter) + } + if payload.Invitee != nil { + invitation.Invitee = identityFromUser(*payload.Invitee) + } + return invitation, nil +} diff --git a/internal/gitprovider/github/invitations_test.go b/internal/gitprovider/github/invitations_test.go new file mode 100644 index 00000000..9b250cf2 --- /dev/null +++ b/internal/gitprovider/github/invitations_test.go @@ -0,0 +1,197 @@ +package github + +import ( + "context" + "errors" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" +) + +func TestListRepositoryInvitationsMapsPagesAndUsesUserToken(t *testing.T) { + page1 := readInvitationFixture(t, "repository_invitations_page1.json") + page2 := readInvitationFixture(t, "repository_invitations_page2.json") + var requests []string + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests = append(requests, r.Method+" "+r.URL.RequestURI()) + if r.Method != http.MethodGet || r.URL.EscapedPath() != "/user/repository_invitations" { + t.Fatalf("unexpected request %s %s", r.Method, r.URL.String()) + } + if got := r.Header.Get("Authorization"); got != "Bearer user-token" { + t.Fatalf("Authorization = %q, want user token", got) + } + if got := r.Header.Get("Accept"); got != acceptJSON { + t.Fatalf("Accept = %q, want %q", got, acceptJSON) + } + w.Header().Set("Content-Type", "application/json") + if r.URL.Query().Get("page") == "2" { + _, _ = w.Write(page2) + return + } + if got := r.URL.Query().Get("per_page"); got != "100" { + t.Fatalf("per_page = %q, want 100", got) + } + w.Header().Set("Link", `<`+server.URL+`/user/repository_invitations?per_page=100&page=2>; rel="next"`) + _, _ = w.Write(page1) + })) + defer server.Close() + client := mustClient(t, Options{Token: "user-token", BaseURL: server.URL, GraphQLURL: server.URL + "/graphql"}) + + invitations, err := client.ListRepositoryInvitations(context.Background()) + if err != nil { + t.Fatalf("ListRepositoryInvitations: %v", err) + } + if len(requests) != 2 { + t.Fatalf("requests = %#v, want two pages", requests) + } + want := []gitprovider.RepositoryInvitation{ + { + ID: "301001", + Owner: "example-org", + Repo: "service", + Inviter: gitprovider.Identity{Login: "example-owner", ID: "9002"}, + Invitee: gitprovider.Identity{Login: "review-bot", ID: "9100"}, + Permission: "write", + CreatedAt: time.Date(2026, 10, 30, 14, 52, 50, 0, time.UTC), + URL: "https://github.com/example-org/service/invitations", + }, + { + ID: "301002", + Owner: "other-org", + Repo: "tooling", + Inviter: gitprovider.Identity{Login: "other-owner", ID: "9004"}, + Invitee: gitprovider.Identity{Login: "review-bot", ID: "9100"}, + Permission: "read", + CreatedAt: time.Date(2026, 9, 1, 8, 0, 0, 0, time.UTC), + Expired: true, + URL: "https://github.com/other-org/tooling/invitations", + }, + { + ID: "301003", + Owner: "example-org", + Repo: "docs", + Invitee: gitprovider.Identity{Login: "review-bot", ID: "9100"}, + Permission: "maintain", + CreatedAt: time.Date(2026, 10, 31, 9, 15, 0, 0, time.UTC), + URL: "https://github.com/example-org/docs/invitations", + }, + } + if len(invitations) != len(want) { + t.Fatalf("invitations = %#v, want %d", invitations, len(want)) + } + for i := range want { + got := invitations[i] + if !got.CreatedAt.Equal(want[i].CreatedAt) { + t.Fatalf("invitation %d CreatedAt = %s, want %s", i, got.CreatedAt, want[i].CreatedAt) + } + got.CreatedAt = want[i].CreatedAt + if got != want[i] { + t.Fatalf("invitation %d = %#v, want %#v", i, got, want[i]) + } + } +} + +func TestListRepositoryInvitationsRejectsInvitationWithoutRepository(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + writeJSON(t, w, []map[string]any{{"id": 5, "repository": map[string]any{"name": "service"}}}) + })) + defer server.Close() + client := mustClient(t, Options{BaseURL: server.URL, GraphQLURL: server.URL + "/graphql"}) + + _, err := client.ListRepositoryInvitations(context.Background()) + if !errors.Is(err, ErrValidation) { + t.Fatalf("ListRepositoryInvitations error = %v, want ErrValidation", err) + } +} + +func TestAcceptRepositoryInvitationPatchesWithoutBody(t *testing.T) { + var method, path, auth string + var body []byte + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + method, path, auth = r.Method, r.URL.EscapedPath(), r.Header.Get("Authorization") + body, _ = io.ReadAll(r.Body) + w.WriteHeader(http.StatusNoContent) + })) + defer server.Close() + client := mustClient(t, Options{Token: "user-token", BaseURL: server.URL, GraphQLURL: server.URL + "/graphql"}) + + if err := client.AcceptRepositoryInvitation(context.Background(), "301001"); err != nil { + t.Fatalf("AcceptRepositoryInvitation: %v", err) + } + if method != http.MethodPatch || path != "/user/repository_invitations/301001" || auth != "Bearer user-token" || len(body) != 0 { + t.Fatalf("request = %s %s auth=%q body=%q, want bodiless PATCH with user token", method, path, auth, body) + } +} + +func TestAcceptRepositoryInvitationMapsHostFailures(t *testing.T) { + tests := []struct { + status int + want error + }{ + {status: http.StatusNotFound, want: gitprovider.ErrNotFound}, + {status: http.StatusForbidden, want: gitprovider.ErrPermission}, + {status: http.StatusUnauthorized, want: gitprovider.ErrAuth}, + {status: http.StatusConflict, want: gitprovider.ErrConflict}, + } + for _, tt := range tests { + t.Run(http.StatusText(tt.status), func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(tt.status) + _, _ = w.Write([]byte(`{"message":"echoed user-token"}`)) + })) + defer server.Close() + client := mustClient(t, Options{Token: "user-token", BaseURL: server.URL, GraphQLURL: server.URL + "/graphql"}) + + err := client.AcceptRepositoryInvitation(context.Background(), "301001") + if !errors.Is(err, tt.want) { + t.Fatalf("error = %v, want %v", err, tt.want) + } + if strings.Contains(err.Error(), "user-token") { + t.Fatalf("error leaked token material: %v", err) + } + }) + } +} + +func TestAcceptRepositoryInvitationRejectsNonNumericIDBeforeRequest(t *testing.T) { + client := mustClient(t, Options{BaseURL: "http://127.0.0.1:1", GraphQLURL: "http://127.0.0.1:1/graphql"}) + for _, id := range []gitprovider.InvitationID{"", "abc", "../1", "-4", "0"} { + if err := client.AcceptRepositoryInvitation(context.Background(), id); !errors.Is(err, ErrValidation) { + t.Fatalf("AcceptRepositoryInvitation(%q) error = %v, want ErrValidation", id, err) + } + } +} + +func TestRepositoryInvitationsRejectGitHubAppClients(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + t.Fatalf("unexpected request %s %s", r.Method, r.URL.String()) + })) + defer server.Close() + client := mustClient(t, Options{BaseURL: server.URL, GraphQLURL: server.URL + "/graphql"}) + client.appAuth = &githubAppAuth{} + + if _, err := client.ListRepositoryInvitations(context.Background()); !errors.Is(err, config.ErrUnsupported) { + t.Fatalf("ListRepositoryInvitations error = %v, want config.ErrUnsupported", err) + } + if err := client.AcceptRepositoryInvitation(context.Background(), "1"); !errors.Is(err, config.ErrUnsupported) { + t.Fatalf("AcceptRepositoryInvitation error = %v, want config.ErrUnsupported", err) + } +} + +func readInvitationFixture(t *testing.T, name string) []byte { + t.Helper() + data, err := os.ReadFile(filepath.Join("testdata", name)) // #nosec G304 -- fixed fixture names under testdata. + if err != nil { + t.Fatalf("read fixture %s: %v", name, err) + } + return data +} diff --git a/internal/gitprovider/github/testdata/repository_invitations_page1.json b/internal/gitprovider/github/testdata/repository_invitations_page1.json new file mode 100644 index 00000000..3444f9de --- /dev/null +++ b/internal/gitprovider/github/testdata/repository_invitations_page1.json @@ -0,0 +1,72 @@ +[ + { + "id": 301001, + "node_id": "MDIwOlJlcG9zaXRvcnlJbnZpdGF0aW9uMzAxMDAx", + "repository": { + "id": 7001, + "node_id": "MDEwOlJlcG9zaXRvcnk3MDAx", + "name": "service", + "full_name": "example-org/service", + "private": false, + "owner": { + "login": "example-org", + "id": 9001, + "type": "Organization", + "site_admin": false + }, + "html_url": "https://github.com/example-org/service" + }, + "invitee": { + "login": "review-bot", + "id": 9100, + "type": "User", + "site_admin": false + }, + "inviter": { + "login": "example-owner", + "id": 9002, + "type": "User", + "site_admin": false + }, + "permissions": "write", + "created_at": "2026-10-30T14:52:50Z", + "expired": false, + "url": "https://api.github.com/user/repository_invitations/301001", + "html_url": "https://github.com/example-org/service/invitations" + }, + { + "id": 301002, + "node_id": "MDIwOlJlcG9zaXRvcnlJbnZpdGF0aW9uMzAxMDAy", + "repository": { + "id": 7002, + "node_id": "MDEwOlJlcG9zaXRvcnk3MDAy", + "name": "tooling", + "full_name": "other-org/tooling", + "private": true, + "owner": { + "login": "other-org", + "id": 9003, + "type": "Organization", + "site_admin": false + }, + "html_url": "https://github.com/other-org/tooling" + }, + "invitee": { + "login": "review-bot", + "id": 9100, + "type": "User", + "site_admin": false + }, + "inviter": { + "login": "other-owner", + "id": 9004, + "type": "User", + "site_admin": false + }, + "permissions": "read", + "created_at": "2026-09-01T08:00:00Z", + "expired": true, + "url": "https://api.github.com/user/repository_invitations/301002", + "html_url": "https://github.com/other-org/tooling/invitations" + } +] diff --git a/internal/gitprovider/github/testdata/repository_invitations_page2.json b/internal/gitprovider/github/testdata/repository_invitations_page2.json new file mode 100644 index 00000000..76f277f4 --- /dev/null +++ b/internal/gitprovider/github/testdata/repository_invitations_page2.json @@ -0,0 +1,32 @@ +[ + { + "id": 301003, + "node_id": "MDIwOlJlcG9zaXRvcnlJbnZpdGF0aW9uMzAxMDAz", + "repository": { + "id": 7003, + "node_id": "MDEwOlJlcG9zaXRvcnk3MDAz", + "name": "docs", + "full_name": "example-org/docs", + "private": false, + "owner": { + "login": "example-org", + "id": 9001, + "type": "Organization", + "site_admin": false + }, + "html_url": "https://github.com/example-org/docs" + }, + "invitee": { + "login": "review-bot", + "id": 9100, + "type": "User", + "site_admin": false + }, + "inviter": null, + "permissions": "maintain", + "created_at": "2026-10-31T09:15:00Z", + "expired": false, + "url": "https://api.github.com/user/repository_invitations/301003", + "html_url": "https://github.com/example-org/docs/invitations" + } +] diff --git a/internal/gitprovider/gitlab/invitations.go b/internal/gitprovider/gitlab/invitations.go new file mode 100644 index 00000000..a34fd4e5 --- /dev/null +++ b/internal/gitprovider/gitlab/invitations.go @@ -0,0 +1,27 @@ +package gitlab + +import ( + "context" + "fmt" + + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" +) + +var _ gitprovider.RepositoryInvitations = (*Client)(nil) + +// ListRepositoryInvitations is unsupported: GitLab adds an existing user to a +// project as a member directly, so there is no pending invitation to list. +func (c *Client) ListRepositoryInvitations(context.Context) ([]gitprovider.RepositoryInvitation, error) { + return nil, errInvitationsUnsupported(gitprovider.OperationListRepositoryInvitations) +} + +// AcceptRepositoryInvitation is unsupported for the same reason as +// ListRepositoryInvitations. +func (c *Client) AcceptRepositoryInvitation(context.Context, gitprovider.InvitationID) error { + return errInvitationsUnsupported(gitprovider.OperationAcceptRepositoryInvitation) +} + +func errInvitationsUnsupported(op gitprovider.Operation) error { + return fmt.Errorf("%w: %s for provider gitlab; GitLab adds existing users as project members without an invitation to accept", config.ErrUnsupported, op) +} diff --git a/internal/gitprovider/gitlab/invitations_test.go b/internal/gitprovider/gitlab/invitations_test.go new file mode 100644 index 00000000..60710234 --- /dev/null +++ b/internal/gitprovider/gitlab/invitations_test.go @@ -0,0 +1,26 @@ +package gitlab + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/config" +) + +func TestRepositoryInvitationsAreUnsupportedWithoutRequests(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) { + t.Fatalf("unexpected request %s %s", r.Method, r.URL.String()) + })) + defer server.Close() + client := mustClient(t, Options{BaseURL: server.URL}) + + if _, err := client.ListRepositoryInvitations(context.Background()); !errors.Is(err, config.ErrUnsupported) { + t.Fatalf("ListRepositoryInvitations error = %v, want config.ErrUnsupported", err) + } + if err := client.AcceptRepositoryInvitation(context.Background(), "1"); !errors.Is(err, config.ErrUnsupported) { + t.Fatalf("AcceptRepositoryInvitation error = %v, want config.ErrUnsupported", err) + } +} diff --git a/internal/gitprovider/invitations.go b/internal/gitprovider/invitations.go new file mode 100644 index 00000000..ad090b41 --- /dev/null +++ b/internal/gitprovider/invitations.go @@ -0,0 +1,37 @@ +package gitprovider + +import ( + "context" + "time" +) + +// InvitationID identifies a pending repository invitation on one host. +type InvitationID string + +// RepositoryInvitation is a pending invitation for the authenticated identity +// to collaborate on one repository. +type RepositoryInvitation struct { + ID InvitationID + Owner string + Repo string + Inviter Identity + Invitee Identity + Permission string + CreatedAt time.Time + Expired bool + URL string +} + +// FullName returns the owner/repo name of the invited repository. +func (i RepositoryInvitation) FullName() string { + return i.Owner + "/" + i.Repo +} + +// RepositoryInvitations lists and accepts the pending repository invitations +// addressed to the authenticated identity. It is separate from GitProvider +// because it acts on the account rather than on a pull request, so review +// runtime wrappers need not forward it. +type RepositoryInvitations interface { + ListRepositoryInvitations(ctx context.Context) ([]RepositoryInvitation, error) + AcceptRepositoryInvitation(ctx context.Context, id InvitationID) error +} diff --git a/internal/gitprovider/operation.go b/internal/gitprovider/operation.go index 77d34ae9..db0adbc6 100644 --- a/internal/gitprovider/operation.go +++ b/internal/gitprovider/operation.go @@ -22,4 +22,7 @@ const ( OperationResolveThread Operation = "ResolveThread" OperationPostIssueComment Operation = "PostIssueComment" OperationSubmitReview Operation = "SubmitReview" + + OperationListRepositoryInvitations Operation = "ListRepositoryInvitations" + OperationAcceptRepositoryInvitation Operation = "AcceptRepositoryInvitation" ) diff --git a/internal/invitations/invitations.go b/internal/invitations/invitations.go new file mode 100644 index 00000000..a655e110 --- /dev/null +++ b/internal/invitations/invitations.go @@ -0,0 +1,201 @@ +// Package invitations selects and accepts the pending repository invitations +// of one git-host identity against an explicit owner/repository allowlist. +package invitations + +import ( + "context" + "errors" + "fmt" + "sort" + "strings" + "unicode" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" +) + +// ErrInvalidAllowlist identifies an allowlist entry that is empty or malformed. +var ErrInvalidAllowlist = errors.New("invitations: invalid allowlist") + +// ErrAllowlistRequired identifies an accept request without any allowlist entry. +var ErrAllowlistRequired = errors.New("invitations: accept requires at least one allowlisted owner or repository") + +// Allowlist names the repository owners and owner/name repositories whose +// invitations may be accepted. Matching is case-insensitive, as host logins +// and repository names are. +type Allowlist struct { + Owners []string + Repos []string +} + +// NewAllowlist validates and normalizes owner logins and owner/name +// repositories. Both may be empty; Accept separately requires a non-empty one. +func NewAllowlist(owners, repos []string) (Allowlist, error) { + var allow Allowlist + for _, owner := range owners { + owner = strings.TrimSpace(owner) + if owner == "" || strings.Contains(owner, "/") || strings.IndexFunc(owner, unicode.IsSpace) >= 0 { + return Allowlist{}, fmt.Errorf("%w: owner %q must be a single account login", ErrInvalidAllowlist, owner) + } + allow.Owners = appendUnique(allow.Owners, owner) + } + for _, repo := range repos { + repo = strings.TrimSpace(repo) + owner, name, ok := strings.Cut(repo, "/") + if !ok || owner == "" || name == "" || strings.Contains(name, "/") || strings.IndexFunc(repo, unicode.IsSpace) >= 0 { + return Allowlist{}, fmt.Errorf("%w: repository %q must be owner/name", ErrInvalidAllowlist, repo) + } + allow.Repos = appendUnique(allow.Repos, repo) + } + return allow, nil +} + +// Empty reports whether the allowlist names nothing. +func (a Allowlist) Empty() bool { + return len(a.Owners) == 0 && len(a.Repos) == 0 +} + +// Matches reports whether the invitation's repository is on the allowlist. +func (a Allowlist) Matches(invitation gitprovider.RepositoryInvitation) bool { + for _, owner := range a.Owners { + if strings.EqualFold(owner, invitation.Owner) { + return true + } + } + fullName := invitation.FullName() + for _, repo := range a.Repos { + if strings.EqualFold(repo, fullName) { + return true + } + } + return false +} + +// List returns the pending invitations, sorted by repository. A non-empty +// filter keeps only matching invitations. +func List(ctx context.Context, provider gitprovider.RepositoryInvitations, filter Allowlist) ([]gitprovider.RepositoryInvitation, error) { + pending, err := provider.ListRepositoryInvitations(ctx) + if err != nil { + return nil, err + } + sortInvitations(pending) + if filter.Empty() { + return pending, nil + } + matched := make([]gitprovider.RepositoryInvitation, 0, len(pending)) + for _, invitation := range pending { + if filter.Matches(invitation) { + matched = append(matched, invitation) + } + } + return matched, nil +} + +// Status is the outcome of one invitation in an accept run. +type Status string + +// Accept outcomes. +const ( + StatusAccepted Status = "accepted" + StatusWouldAccept Status = "would_accept" + StatusSkipped Status = "skipped" + StatusFailed Status = "failed" +) + +// Skip reasons. +const ( + ReasonNotAllowlisted = "not on the allowlist" + ReasonExpired = "invitation expired; ask the inviter to send a new one" +) + +// Outcome records what an accept run did with one pending invitation. +type Outcome struct { + Invitation gitprovider.RepositoryInvitation + Status Status + Reason string + Err error +} + +// Summary counts outcomes by status. +type Summary struct { + Accepted int + WouldAccept int + Skipped int + Failed int +} + +// Summarize counts outcomes by status. +func Summarize(outcomes []Outcome) Summary { + var summary Summary + for _, outcome := range outcomes { + switch outcome.Status { + case StatusAccepted: + summary.Accepted++ + case StatusWouldAccept: + summary.WouldAccept++ + case StatusSkipped: + summary.Skipped++ + case StatusFailed: + summary.Failed++ + } + } + return summary +} + +// Accept accepts every pending, unexpired invitation on the allowlist and +// reports an outcome for every pending invitation, accepted or not. With +// dryRun it reports what it would accept and makes no writes. One failed +// accept does not stop the others; callers inspect the outcomes. The returned +// error is reserved for a missing allowlist or a failed listing. +func Accept(ctx context.Context, provider gitprovider.RepositoryInvitations, allow Allowlist, dryRun bool) ([]Outcome, error) { + if allow.Empty() { + return nil, ErrAllowlistRequired + } + pending, err := List(ctx, provider, Allowlist{}) + if err != nil { + return nil, err + } + outcomes := make([]Outcome, 0, len(pending)) + for _, invitation := range pending { + outcome := Outcome{Invitation: invitation} + switch { + case !allow.Matches(invitation): + outcome.Status = StatusSkipped + outcome.Reason = ReasonNotAllowlisted + case invitation.Expired: + outcome.Status = StatusSkipped + outcome.Reason = ReasonExpired + case dryRun: + outcome.Status = StatusWouldAccept + default: + if err := provider.AcceptRepositoryInvitation(ctx, invitation.ID); err != nil { + outcome.Status = StatusFailed + outcome.Reason = err.Error() + outcome.Err = err + } else { + outcome.Status = StatusAccepted + } + } + outcomes = append(outcomes, outcome) + } + return outcomes, nil +} + +func sortInvitations(invitations []gitprovider.RepositoryInvitation) { + sort.SliceStable(invitations, func(i, j int) bool { + left := strings.ToLower(invitations[i].FullName()) + right := strings.ToLower(invitations[j].FullName()) + if left != right { + return left < right + } + return invitations[i].ID < invitations[j].ID + }) +} + +func appendUnique(values []string, value string) []string { + for _, existing := range values { + if strings.EqualFold(existing, value) { + return values + } + } + return append(values, value) +} diff --git a/internal/invitations/invitations_test.go b/internal/invitations/invitations_test.go new file mode 100644 index 00000000..1360addd --- /dev/null +++ b/internal/invitations/invitations_test.go @@ -0,0 +1,207 @@ +package invitations + +import ( + "context" + "errors" + "reflect" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" +) + +func TestNewAllowlistValidatesAndDeduplicates(t *testing.T) { + allow, err := NewAllowlist([]string{" example-org ", "Example-Org", "other"}, []string{"a/b", "A/B", " c/d "}) + if err != nil { + t.Fatalf("NewAllowlist: %v", err) + } + if !reflect.DeepEqual(allow.Owners, []string{"example-org", "other"}) || !reflect.DeepEqual(allow.Repos, []string{"a/b", "c/d"}) { + t.Fatalf("allowlist = %#v, want trimmed case-insensitive unique entries", allow) + } + if empty, err := NewAllowlist(nil, nil); err != nil || !empty.Empty() { + t.Fatalf("NewAllowlist(nil, nil) = %#v, %v; want empty allowlist", empty, err) + } + for _, owner := range []string{"", " ", "org/repo", "two words"} { + if _, err := NewAllowlist([]string{owner}, nil); !errors.Is(err, ErrInvalidAllowlist) { + t.Fatalf("owner %q error = %v, want ErrInvalidAllowlist", owner, err) + } + } + for _, repo := range []string{"", "org", "org/", "/repo", "org/repo/extra", "org /repo", "org/re po"} { + if _, err := NewAllowlist(nil, []string{repo}); !errors.Is(err, ErrInvalidAllowlist) { + t.Fatalf("repo %q error = %v, want ErrInvalidAllowlist", repo, err) + } + } +} + +func TestAllowlistMatchesOwnerOrRepositoryCaseInsensitively(t *testing.T) { + allow, err := NewAllowlist([]string{"Example-Org"}, []string{"other-org/Tooling"}) + if err != nil { + t.Fatalf("NewAllowlist: %v", err) + } + tests := []struct { + invitation gitprovider.RepositoryInvitation + want bool + }{ + {gitprovider.RepositoryInvitation{Owner: "example-org", Repo: "anything"}, true}, + {gitprovider.RepositoryInvitation{Owner: "OTHER-ORG", Repo: "tooling"}, true}, + {gitprovider.RepositoryInvitation{Owner: "other-org", Repo: "tooling-extra"}, false}, + {gitprovider.RepositoryInvitation{Owner: "example-org-fork", Repo: "service"}, false}, + } + for _, tt := range tests { + if got := allow.Matches(tt.invitation); got != tt.want { + t.Fatalf("Matches(%s) = %v, want %v", tt.invitation.FullName(), got, tt.want) + } + } +} + +func TestListSortsAndFilters(t *testing.T) { + fake := fakeWithInvitations() + all, err := List(context.Background(), fake, Allowlist{}) + if err != nil { + t.Fatalf("List: %v", err) + } + if got := fullNames(all); !reflect.DeepEqual(got, []string{"example-org/docs", "example-org/service", "other-org/expired", "other-org/tooling"}) { + t.Fatalf("List = %v, want sorted pending invitations", got) + } + allow, _ := NewAllowlist(nil, []string{"other-org/tooling"}) + filtered, err := List(context.Background(), fake, allow) + if err != nil { + t.Fatalf("List filtered: %v", err) + } + if got := fullNames(filtered); !reflect.DeepEqual(got, []string{"other-org/tooling"}) { + t.Fatalf("filtered = %v, want only other-org/tooling", got) + } +} + +func TestAcceptRequiresAllowlistBeforeAnyProviderCall(t *testing.T) { + fake := fakeWithInvitations() + var calls []gitprovider.Operation + fake.SetCallHook(func(op gitprovider.Operation) { calls = append(calls, op) }) + + _, err := Accept(context.Background(), fake, Allowlist{}, false) + if !errors.Is(err, ErrAllowlistRequired) { + t.Fatalf("Accept error = %v, want ErrAllowlistRequired", err) + } + if len(calls) != 0 { + t.Fatalf("provider calls = %v, want none", calls) + } +} + +func TestAcceptReportsEveryInvitationAndAcceptsOnlyAllowlisted(t *testing.T) { + fake := fakeWithInvitations() + allow, _ := NewAllowlist([]string{"example-org", "other-org"}, nil) + + outcomes, err := Accept(context.Background(), fake, allow, false) + if err != nil { + t.Fatalf("Accept: %v", err) + } + got := statuses(outcomes) + want := map[string]Status{ + "example-org/docs": StatusAccepted, + "example-org/service": StatusAccepted, + "other-org/expired": StatusSkipped, + "other-org/tooling": StatusAccepted, + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("statuses = %v, want %v", got, want) + } + if outcomes[2].Reason != ReasonExpired { + t.Fatalf("expired reason = %q, want %q", outcomes[2].Reason, ReasonExpired) + } + if accepted := fake.RecordedAcceptedInvitations(); !reflect.DeepEqual(accepted, []gitprovider.InvitationID{"3", "1", "2"}) { + t.Fatalf("accepted = %v, want sorted allowlisted unexpired invitations", accepted) + } + + narrow := fakeWithInvitations() + repoOnly, _ := NewAllowlist(nil, []string{"example-org/service"}) + outcomes, err = Accept(context.Background(), narrow, repoOnly, false) + if err != nil { + t.Fatalf("Accept narrow: %v", err) + } + for _, outcome := range outcomes { + if outcome.Invitation.FullName() != "example-org/service" && (outcome.Status != StatusSkipped || outcome.Reason != ReasonNotAllowlisted) { + t.Fatalf("outcome %s = %s (%s), want skipped off the allowlist", outcome.Invitation.FullName(), outcome.Status, outcome.Reason) + } + } + if accepted := narrow.RecordedAcceptedInvitations(); !reflect.DeepEqual(accepted, []gitprovider.InvitationID{"1"}) { + t.Fatalf("accepted = %v, want only invitation 1", accepted) + } +} + +func TestAcceptDryRunMakesNoWrites(t *testing.T) { + fake := fakeWithInvitations() + var calls []gitprovider.Operation + fake.SetCallHook(func(op gitprovider.Operation) { calls = append(calls, op) }) + allow, _ := NewAllowlist([]string{"example-org"}, nil) + + outcomes, err := Accept(context.Background(), fake, allow, true) + if err != nil { + t.Fatalf("Accept: %v", err) + } + if !reflect.DeepEqual(calls, []gitprovider.Operation{gitprovider.OperationListRepositoryInvitations}) { + t.Fatalf("provider calls = %v, want only the listing", calls) + } + if summary := Summarize(outcomes); summary != (Summary{WouldAccept: 2, Skipped: 2}) { + t.Fatalf("summary = %#v, want two would_accept and two skipped", summary) + } +} + +func TestAcceptContinuesPastFailures(t *testing.T) { + fake := fakeWithInvitations() + failure := gitprovider.WrapError(gitprovider.ErrPermission, gitprovider.OperationAcceptRepositoryInvitation, errors.New("github: status 403")) + fake.SetAcceptInvitationError("3", failure) + allow, _ := NewAllowlist([]string{"example-org"}, nil) + + outcomes, err := Accept(context.Background(), fake, allow, false) + if err != nil { + t.Fatalf("Accept: %v", err) + } + if outcomes[0].Status != StatusFailed || !errors.Is(outcomes[0].Err, gitprovider.ErrPermission) || outcomes[0].Reason != failure.Error() { + t.Fatalf("first outcome = %#v, want failed with the provider error", outcomes[0]) + } + if outcomes[1].Status != StatusAccepted { + t.Fatalf("second outcome = %#v, want accepted after the failure", outcomes[1]) + } + if summary := Summarize(outcomes); summary != (Summary{Accepted: 1, Skipped: 2, Failed: 1}) { + t.Fatalf("summary = %#v", summary) + } +} + +func TestAcceptReturnsListingFailure(t *testing.T) { + fake := fakeWithInvitations() + fake.SetError(gitprovider.OperationListRepositoryInvitations, gitprovider.ErrAuth) + allow, _ := NewAllowlist([]string{"example-org"}, nil) + + if _, err := Accept(context.Background(), fake, allow, false); !errors.Is(err, gitprovider.ErrAuth) { + t.Fatalf("Accept error = %v, want ErrAuth", err) + } + if accepted := fake.RecordedAcceptedInvitations(); len(accepted) != 0 { + t.Fatalf("accepted = %v, want none", accepted) + } +} + +func fakeWithInvitations() *gitprovider.Fake { + fake := &gitprovider.Fake{} + fake.SetRepositoryInvitations([]gitprovider.RepositoryInvitation{ + {ID: "1", Owner: "example-org", Repo: "service", Permission: "write"}, + {ID: "2", Owner: "other-org", Repo: "tooling", Permission: "read"}, + {ID: "3", Owner: "example-org", Repo: "docs", Permission: "write"}, + {ID: "4", Owner: "other-org", Repo: "expired", Permission: "write", Expired: true}, + }) + return fake +} + +func fullNames(invitations []gitprovider.RepositoryInvitation) []string { + names := make([]string, 0, len(invitations)) + for _, invitation := range invitations { + names = append(names, invitation.FullName()) + } + return names +} + +func statuses(outcomes []Outcome) map[string]Status { + out := make(map[string]Status, len(outcomes)) + for _, outcome := range outcomes { + out[outcome.Invitation.FullName()] = outcome.Status + } + return out +} diff --git a/internal/view/invites.go b/internal/view/invites.go new file mode 100644 index 00000000..9b518bc5 --- /dev/null +++ b/internal/view/invites.go @@ -0,0 +1,201 @@ +package view + +import ( + "fmt" + "io" + "time" + + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + "github.com/open-cli-collective/codereview-cli/internal/invitations" +) + +// InvitesIdentity describes the git-host identity whose invitations are shown. +type InvitesIdentity struct { + Profile string `json:"profile"` + CredentialSource string `json:"credential_source"` + Host string `json:"host"` + Login string `json:"login"` +} + +// InvitesList is the presentation model for `cr invites list`. +type InvitesList struct { + InvitesIdentity + Invitations []InviteItem `json:"invitations"` +} + +// InvitesAccept is the presentation model for `cr invites accept`. +type InvitesAccept struct { + InvitesIdentity + DryRun bool `json:"dry_run"` + Allowlist InvitesAllowlist `json:"allowlist"` + Results []InviteAcceptItem `json:"results"` + Summary InvitesAcceptCounts `json:"summary"` +} + +// InvitesAllowlist echoes the owners and repositories accept was limited to. +type InvitesAllowlist struct { + Owners []string `json:"owners"` + Repos []string `json:"repos"` +} + +// InviteItem describes one pending repository invitation. +type InviteItem struct { + ID string `json:"id"` + Repository string `json:"repository"` + Inviter string `json:"inviter"` + Permission string `json:"permission"` + CreatedAt time.Time `json:"created_at"` + Expired bool `json:"expired"` + URL string `json:"url,omitempty"` +} + +// InviteAcceptItem describes what accept did with one pending invitation. +type InviteAcceptItem struct { + InviteItem + Status string `json:"status"` + Reason string `json:"reason,omitempty"` +} + +// InvitesAcceptCounts counts accept outcomes by status. +type InvitesAcceptCounts struct { + Accepted int `json:"accepted"` + WouldAccept int `json:"would_accept"` + Skipped int `json:"skipped"` + Failed int `json:"failed"` +} + +// NewInvitesList builds the invites list presentation model. +func NewInvitesList(identity InvitesIdentity, pending []gitprovider.RepositoryInvitation) InvitesList { + items := make([]InviteItem, 0, len(pending)) + for _, invitation := range pending { + items = append(items, newInviteItem(invitation)) + } + return InvitesList{InvitesIdentity: identity, Invitations: items} +} + +// NewInvitesAccept builds the invites accept presentation model. +func NewInvitesAccept(identity InvitesIdentity, allow invitations.Allowlist, dryRun bool, outcomes []invitations.Outcome) InvitesAccept { + results := make([]InviteAcceptItem, 0, len(outcomes)) + for _, outcome := range outcomes { + results = append(results, InviteAcceptItem{ + InviteItem: newInviteItem(outcome.Invitation), + Status: string(outcome.Status), + Reason: outcome.Reason, + }) + } + summary := invitations.Summarize(outcomes) + return InvitesAccept{ + InvitesIdentity: identity, + DryRun: dryRun, + Allowlist: InvitesAllowlist{ + Owners: append([]string{}, allow.Owners...), + Repos: append([]string{}, allow.Repos...), + }, + Results: results, + Summary: InvitesAcceptCounts{ + Accepted: summary.Accepted, + WouldAccept: summary.WouldAccept, + Skipped: summary.Skipped, + Failed: summary.Failed, + }, + } +} + +func newInviteItem(invitation gitprovider.RepositoryInvitation) InviteItem { + return InviteItem{ + ID: string(invitation.ID), + Repository: invitation.FullName(), + Inviter: invitation.Inviter.Login, + Permission: invitation.Permission, + CreatedAt: invitation.CreatedAt, + Expired: invitation.Expired, + URL: invitation.URL, + } +} + +// RenderInvitesListText writes a stable human-readable invitation list. +func RenderInvitesListText(w io.Writer, result InvitesList) error { + if err := writeInvitesIdentity(w, result.InvitesIdentity); err != nil { + return err + } + if len(result.Invitations) == 0 { + _, err := fmt.Fprintln(w, "Invitations: none") + return err + } + if _, err := fmt.Fprintln(w, "Invitations:"); err != nil { + return err + } + for _, item := range result.Invitations { + if _, err := fmt.Fprintf(w, " - %s\n", item.Repository); err != nil { + return err + } + if err := writeInviteDetails(w, item); err != nil { + return err + } + } + return nil +} + +// RenderInvitesAcceptText writes a stable human-readable accept report. +func RenderInvitesAcceptText(w io.Writer, result InvitesAccept) error { + if err := writeInvitesIdentity(w, result.InvitesIdentity); err != nil { + return err + } + if err := writeKV(w, "Dry run", fmt.Sprint(result.DryRun)); err != nil { + return err + } + if len(result.Results) == 0 { + if _, err := fmt.Fprintln(w, "Invitations: none"); err != nil { + return err + } + } else { + if _, err := fmt.Fprintln(w, "Invitations:"); err != nil { + return err + } + for _, item := range result.Results { + line := fmt.Sprintf(" - %s: %s", item.Repository, item.Status) + if item.Reason != "" { + line += " (" + item.Reason + ")" + } + if _, err := fmt.Fprintln(w, line); err != nil { + return err + } + if err := writeInviteDetails(w, item.InviteItem); err != nil { + return err + } + } + } + summary := result.Summary + return writeKV(w, "Summary", fmt.Sprintf("accepted=%d would_accept=%d skipped=%d failed=%d", summary.Accepted, summary.WouldAccept, summary.Skipped, summary.Failed)) +} + +func writeInvitesIdentity(w io.Writer, identity InvitesIdentity) error { + if err := writeKV(w, "Profile", identity.Profile); err != nil { + return err + } + if err := writeKV(w, "Credential source", identity.CredentialSource); err != nil { + return err + } + if err := writeKV(w, "Host", identity.Host); err != nil { + return err + } + return writeKV(w, "Login", identity.Login) +} + +func writeInviteDetails(w io.Writer, item InviteItem) error { + if err := writeKV(w, " Permission", item.Permission); err != nil { + return err + } + if err := writeOptionalKV(w, " Inviter", item.Inviter); err != nil { + return err + } + if !item.CreatedAt.IsZero() { + if err := writeKV(w, " Created", item.CreatedAt.UTC().Format(time.RFC3339)); err != nil { + return err + } + } + if item.Expired { + return writeKV(w, " Expired", "true") + } + return nil +} diff --git a/internal/view/invites_test.go b/internal/view/invites_test.go new file mode 100644 index 00000000..d476e9a8 --- /dev/null +++ b/internal/view/invites_test.go @@ -0,0 +1,50 @@ +package view + +import ( + "bytes" + "strings" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/invitations" +) + +func TestInvitesTextWithNoPendingInvitations(t *testing.T) { + identity := InvitesIdentity{Profile: "reviewer", CredentialSource: "git", Host: "github.com", Login: "review-bot"} + + var list bytes.Buffer + if err := RenderInvitesListText(&list, NewInvitesList(identity, nil)); err != nil { + t.Fatalf("RenderInvitesListText: %v", err) + } + wantList := "Profile: reviewer\nCredential source: git\nHost: github.com\nLogin: review-bot\nInvitations: none\n" + if list.String() != wantList { + t.Fatalf("list text = %q, want %q", list.String(), wantList) + } + + allow, err := invitations.NewAllowlist([]string{"example-org"}, nil) + if err != nil { + t.Fatalf("NewAllowlist: %v", err) + } + var accept bytes.Buffer + if err := RenderInvitesAcceptText(&accept, NewInvitesAccept(identity, allow, true, nil)); err != nil { + t.Fatalf("RenderInvitesAcceptText: %v", err) + } + if !strings.HasSuffix(accept.String(), "Dry run: true\nInvitations: none\nSummary: accepted=0 would_accept=0 skipped=0 failed=0\n") { + t.Fatalf("accept text = %q", accept.String()) + } +} + +func TestInvitesAcceptJSONKeepsEmptyAllowlistArrays(t *testing.T) { + allow, err := invitations.NewAllowlist(nil, []string{"example-org/service"}) + if err != nil { + t.Fatalf("NewAllowlist: %v", err) + } + var out bytes.Buffer + if err := RenderJSON(&out, NewInvitesAccept(InvitesIdentity{}, allow, false, nil)); err != nil { + t.Fatalf("RenderJSON: %v", err) + } + for _, want := range []string{`"owners": []`, `"repos": [`, `"results": []`} { + if !strings.Contains(out.String(), want) { + t.Fatalf("JSON missing %s:\n%s", want, out.String()) + } + } +} From b67a0520fa9df38f0a6f801393c80b79c62cd463 Mon Sep 17 00:00:00 2001 From: piekstra Date: Mon, 2 Nov 2026 18:26:29 -0500 Subject: [PATCH 2/3] fix(invites): address review findings - Add a closed reason_code to accept results; the view derives the text. - Share one error classifier, cmderr.Classify, between cr me and cr invites. - Build the invitation client through gitproviders.NewInvitationClient and drop the dead capability assertion; document the adapter contract. - Catch method values in the AcceptRepositoryInvitation guard and allow only internal/invitations. - Reword the --owner row and list cr invites in the runtime-only flag audit. Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0 --- README.md | 7 +- docs/init-config-surface.md | 1 + .../architecture/invitation_writes_test.go | 18 ++-- internal/cmd/cmderr/cmderr.go | 102 +++++++++++++----- internal/cmd/cmderr/cmderr_test.go | 40 +++++++ internal/cmd/invitescmd/invitescmd.go | 48 +-------- internal/cmd/invitescmd/invitescmd_test.go | 4 +- .../invitescmd/testdata/accept.golden.json | 13 ++- internal/cmd/mecmd/mecmd.go | 33 +----- internal/gitprovider/invitations.go | 6 ++ internal/gitproviders/invitations.go | 35 ++++++ internal/gitproviders/invitations_test.go | 44 ++++++++ internal/invitations/invitations.go | 45 +++++++- internal/invitations/invitations_test.go | 23 +++- internal/view/invites.go | 33 +++++- 15 files changed, 323 insertions(+), 129 deletions(-) create mode 100644 internal/gitproviders/invitations.go create mode 100644 internal/gitproviders/invitations_test.go diff --git a/README.md b/README.md index fecd6ea2..38e35ba4 100644 --- a/README.md +++ b/README.md @@ -1243,7 +1243,7 @@ a usage error and reads nothing. | Flag | Semantics | |------|-----------| -| `--owner ` | Accept invitations to repositories this account owns. Repeatable. | +| `--owner ` | Accept invitations to repositories owned by this login (user or organization). Repeatable. | | `--repo ` | Accept the invitation to this repository. Repeatable. | | `--dry-run` | Report what would be accepted without accepting anything. | | `--json` | Emit JSON. | @@ -1254,7 +1254,10 @@ Every pending invitation gets one result: `accepted`, `would_accept` under accepts. After printing every result, the command exits `1` if any accept failed. JSON output carries the identity (`profile`, `credential_source`, `host`, `login`), `dry_run`, the `allowlist`, the per-invitation `results`, and -a `summary` count per status. +a `summary` count per status. Each skipped or failed result carries a stable +`reason_code` for scripts and a human-readable `reason`. Skips use +`not_allowlisted` or `expired`. Failures use `auth`, `permission`, `not_found`, +`conflict`, `retryable`, `canceled`, or `error`. ### `cr agents list` diff --git a/docs/init-config-surface.md b/docs/init-config-surface.md index 49b1f9c2..c445ce4f 100644 --- a/docs/init-config-surface.md +++ b/docs/init-config-surface.md @@ -258,6 +258,7 @@ The following flags are intentionally not durable init configuration. | `cr data show` | `--json` | Presentation only. | | `cr agents` | `--agents-dir`, `--json` | Per-run inspection inputs and output. Durable trusted sources use `agent_sources`. | | `cr me` | `--all`, `--json` | Identity refresh/read behavior, not wizard configuration. | +| `cr invites` | `--owner`, `--repo`, `--dry-run`, `--json` | Per-run allowlist, filter, and preview controls for accepting the posting identity's repository invitations, not profile config. | | `cr sessions` | `--json` | Presentation only. | | `cr benchmark` | `--candidate`, `--case`, `--results-dir`, `--cr-bin`, `--json` | Benchmark harness controls, outside init. | diff --git a/internal/architecture/invitation_writes_test.go b/internal/architecture/invitation_writes_test.go index d30b1a27..6bfab295 100644 --- a/internal/architecture/invitation_writes_test.go +++ b/internal/architecture/invitation_writes_test.go @@ -12,13 +12,13 @@ import ( // Accepting a repository invitation is the one account-level provider write. // It stays inside internal/invitations, which enforces the allowlist and -// dry-run, so no other caller can accept invitations without them. +// dry-run, so no other caller can accept invitations without them. Any +// selector naming the method counts, so a method value or method expression +// cannot slip past where a direct call would be caught. Adapters only declare +// the method, and declarations are not selectors. func TestRepositoryInvitationAcceptsStayInInvitationsPackage(t *testing.T) { repoRoot := repoRootFromTest(t) - allowedDirs := []string{ - "internal/invitations", - "internal/gitprovider", - } + allowedDirs := []string{"internal/invitations"} fset := token.NewFileSet() err := filepath.WalkDir(repoRoot, func(path string, entry fs.DirEntry, walkErr error) error { @@ -45,15 +45,11 @@ func TestRepositoryInvitationAcceptsStayInInvitationsPackage(t *testing.T) { return err } ast.Inspect(parsed, func(node ast.Node) bool { - call, ok := node.(*ast.CallExpr) - if !ok { - return true - } - selector, ok := call.Fun.(*ast.SelectorExpr) + selector, ok := node.(*ast.SelectorExpr) if !ok || selector.Sel.Name != "AcceptRepositoryInvitation" { return true } - t.Fatalf("%s calls AcceptRepositoryInvitation outside internal/invitations", fset.Position(selector.Pos())) + t.Fatalf("%s references AcceptRepositoryInvitation outside internal/invitations", fset.Position(selector.Pos())) return false }) return nil diff --git a/internal/cmd/cmderr/cmderr.go b/internal/cmd/cmderr/cmderr.go index a6945284..267e2dd2 100644 --- a/internal/cmd/cmderr/cmderr.go +++ b/internal/cmd/cmderr/cmderr.go @@ -13,17 +13,56 @@ import ( "github.com/open-cli-collective/codereview-cli/internal/gitprovider" ) +// Sentinel groups shared by the mappers below and Classify, so each error +// class is listed once. +var ( + configUsageErrors = []error{ + config.ErrInvalid, + config.ErrUnsupportedEffort, + } + configAuthErrors = []error{ + config.ErrNotConfigured, + config.ErrProfileNotFound, + config.ErrRepositoryProfileAmbiguous, + config.ErrSecretsStoreNotFound, + config.ErrUnsupported, + } + credentialUsageErrors = []error{ + credentials.ErrInvalidBackendSelection, + credentials.ErrWrongService, + credstore.ErrRefEmpty, + credstore.ErrRefSegmentCount, + credstore.ErrRefInvalidChar, + credstore.ErrKeyNotAllowed, + credstore.ErrExists, + } + credentialAuthErrors = []error{ + credstore.ErrFilePassphraseRequired, + credstore.ErrSecretServiceFailClosed, + credstore.ErrStoreClosed, + credstore.ErrBackendNotImplemented, + } + providerAuthErrors = []error{ + gitprovider.ErrAuth, + gitprovider.ErrPermission, + gitprovider.ErrIneligibleReviewAuthority, + } + providerUpstreamErrors = []error{ + gitprovider.ErrRetryable, + } + providerFailureErrors = []error{ + gitprovider.ErrNotFound, + gitprovider.ErrConflict, + gitprovider.ErrStaleSHA, + } +) + // Config maps config package errors to scriptable command errors. func Config(err error) error { switch { - case errors.Is(err, config.ErrInvalid), - errors.Is(err, config.ErrUnsupportedEffort): + case isAny(err, configUsageErrors): return exitcode.Usage(err) - case errors.Is(err, config.ErrNotConfigured), - errors.Is(err, config.ErrProfileNotFound), - errors.Is(err, config.ErrRepositoryProfileAmbiguous), - errors.Is(err, config.ErrSecretsStoreNotFound), - errors.Is(err, config.ErrUnsupported): + case isAny(err, configAuthErrors): return exitcode.AuthConfig(err) default: return err @@ -33,18 +72,9 @@ func Config(err error) error { // Credential maps credstore-related errors to scriptable command errors. func Credential(err error) error { switch { - case errors.Is(err, credentials.ErrInvalidBackendSelection), - errors.Is(err, credentials.ErrWrongService), - errors.Is(err, credstore.ErrRefEmpty), - errors.Is(err, credstore.ErrRefSegmentCount), - errors.Is(err, credstore.ErrRefInvalidChar), - errors.Is(err, credstore.ErrKeyNotAllowed), - errors.Is(err, credstore.ErrExists): + case isAny(err, credentialUsageErrors): return exitcode.Usage(err) - case errors.Is(err, credstore.ErrFilePassphraseRequired), - errors.Is(err, credstore.ErrSecretServiceFailClosed), - errors.Is(err, credstore.ErrStoreClosed), - errors.Is(err, credstore.ErrBackendNotImplemented): + case isAny(err, credentialAuthErrors): return exitcode.AuthConfig(err) default: return err @@ -62,15 +92,11 @@ func Provider(err error) error { func providerKind(err error, kind error) error { switch { - case errors.Is(kind, gitprovider.ErrAuth), - errors.Is(kind, gitprovider.ErrPermission), - errors.Is(kind, gitprovider.ErrIneligibleReviewAuthority): + case isAny(kind, providerAuthErrors): return exitcode.AuthConfig(err) - case errors.Is(kind, gitprovider.ErrRetryable): + case isAny(kind, providerUpstreamErrors): return exitcode.Upstream(err) - case errors.Is(kind, gitprovider.ErrNotFound), - errors.Is(kind, gitprovider.ErrConflict), - errors.Is(kind, gitprovider.ErrStaleSHA): + case isAny(kind, providerFailureErrors): return exitcode.With(exitcode.Failure, err) default: // Unknown provider errors intentionally fall through. exitcode.FromError @@ -80,6 +106,32 @@ func providerKind(err error, kind error) error { } } +// Classify maps config, git-provider, and credential-store errors onto the +// exit taxonomy, checked in that order, and returns any other error +// unchanged. Commands that load a profile, open its credential store, and +// call the provider use it so their exit codes stay aligned. +func Classify(err error) error { + switch { + case isAny(err, configUsageErrors), isAny(err, configAuthErrors): + return Config(err) + case isAny(err, providerAuthErrors), isAny(err, providerUpstreamErrors), isAny(err, providerFailureErrors): + return Provider(err) + case isAny(err, credentialUsageErrors), isAny(err, credentialAuthErrors): + return Credential(err) + default: + return err + } +} + +func isAny(err error, sentinels []error) bool { + for _, sentinel := range sentinels { + if errors.Is(err, sentinel) { + return true + } + } + return false +} + // BackendFlagChanged reports whether the inherited --backend flag was supplied. func BackendFlagChanged(cmd *cobra.Command) bool { flag := cmd.Flag(credstore.BackendFlagName) diff --git a/internal/cmd/cmderr/cmderr_test.go b/internal/cmd/cmderr/cmderr_test.go index ec71804e..7d2ba8bd 100644 --- a/internal/cmd/cmderr/cmderr_test.go +++ b/internal/cmd/cmderr/cmderr_test.go @@ -93,3 +93,43 @@ func TestProviderErrorMapping(t *testing.T) { }) } } + +func TestClassifyMapsEveryDomainAndLeavesOthersAlone(t *testing.T) { + tests := []struct { + name string + err error + want int + }{ + {name: "config usage", err: config.ErrInvalid, want: exitcode.UsageError}, + {name: "config auth", err: config.ErrProfileNotFound, want: exitcode.AuthConfigError}, + {name: "config unsupported", err: config.ErrUnsupported, want: exitcode.AuthConfigError}, + {name: "provider auth", err: gitprovider.WrapError(gitprovider.ErrAuth, gitprovider.OperationWhoAmI, nil), want: exitcode.AuthConfigError}, + {name: "provider upstream", err: gitprovider.WrapError(gitprovider.ErrRetryable, gitprovider.OperationWhoAmI, nil), want: exitcode.UpstreamError}, + {name: "provider failure", err: gitprovider.WrapError(gitprovider.ErrNotFound, gitprovider.OperationWhoAmI, nil), want: exitcode.Failure}, + {name: "provider auth beats credential cause", err: gitprovider.WrapError(gitprovider.ErrAuth, "", credstore.ErrFilePassphraseRequired), want: exitcode.AuthConfigError}, + {name: "credential usage", err: credstore.ErrRefEmpty, want: exitcode.UsageError}, + {name: "credential auth", err: credstore.ErrStoreClosed, want: exitcode.AuthConfigError}, + {name: "credential backend selection", err: credentials.ErrInvalidBackendSelection, want: exitcode.UsageError}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := Classify(tt.err) + if got := exitcode.FromError(err); got != tt.want { + t.Fatalf("exit code = %d, want %d", got, tt.want) + } + if !errors.Is(err, tt.err) { + t.Fatalf("mapped error = %v, want it to wrap %v", err, tt.err) + } + }) + } + + other := errors.New("unclassified") + got := Classify(other) + if !errors.Is(got, other) || exitcode.FromError(got) != exitcode.Failure { + t.Fatalf("Classify(unclassified) = %v (exit %d), want it unchanged with the generic failure code", got, exitcode.FromError(got)) + } + var coded interface{ ExitCode() int } + if errors.As(got, &coded) { + t.Fatalf("Classify(unclassified) = %T, want no exit-code wrapper", got) + } +} diff --git a/internal/cmd/invitescmd/invitescmd.go b/internal/cmd/invitescmd/invitescmd.go index b7ea2e04..69d48e2a 100644 --- a/internal/cmd/invitescmd/invitescmd.go +++ b/internal/cmd/invitescmd/invitescmd.go @@ -2,12 +2,10 @@ package invitescmd import ( - "context" "errors" "fmt" "io" - "github.com/open-cli-collective/cli-common/credstore" "github.com/spf13/cobra" "github.com/open-cli-collective/codereview-cli/internal/cmd/cmderr" @@ -24,10 +22,7 @@ import ( // Provider is the provider surface `cr invites` needs: the identity lookup // plus the account's repository invitations. -type Provider interface { - WhoAmI(ctx context.Context, creds gitprovider.Credential) (gitprovider.Identity, error) - gitprovider.RepositoryInvitations -} +type Provider = gitproviders.InvitationClient // ProviderFactory builds the provider authenticated as one profile's posting // identity. The returned cleanup, when non-nil, closes the credential store. @@ -244,38 +239,10 @@ func prevalidate(profileName, source string, git config.GitConfig) error { } func mapRunError(err error) error { - switch { - case errors.Is(err, invitations.ErrInvalidAllowlist), - errors.Is(err, invitations.ErrAllowlistRequired): + if errors.Is(err, invitations.ErrInvalidAllowlist) || errors.Is(err, invitations.ErrAllowlistRequired) { return exitcode.Usage(err) - case errors.Is(err, config.ErrInvalid), - errors.Is(err, config.ErrNotConfigured), - errors.Is(err, config.ErrProfileNotFound), - errors.Is(err, config.ErrSecretsStoreNotFound), - errors.Is(err, config.ErrUnsupported): - return cmderr.Config(err) - case errors.Is(err, gitprovider.ErrAuth), - errors.Is(err, gitprovider.ErrPermission), - errors.Is(err, gitprovider.ErrRetryable), - errors.Is(err, gitprovider.ErrNotFound), - errors.Is(err, gitprovider.ErrConflict): - return cmderr.Provider(err) - // Keep this list in sync with cmderr.Credential. The explicit guard avoids - // classifying arbitrary errors as credential-domain failures. - case errors.Is(err, credentials.ErrInvalidBackendSelection), - errors.Is(err, credentials.ErrWrongService), - errors.Is(err, credstore.ErrRefEmpty), - errors.Is(err, credstore.ErrRefSegmentCount), - errors.Is(err, credstore.ErrRefInvalidChar), - errors.Is(err, credstore.ErrKeyNotAllowed), - errors.Is(err, credstore.ErrExists), - errors.Is(err, credstore.ErrFilePassphraseRequired), - errors.Is(err, credstore.ErrSecretServiceFailClosed), - errors.Is(err, credstore.ErrStoreClosed), - errors.Is(err, credstore.ErrBackendNotImplemented): - return cmderr.Credential(err) } - return err + return cmderr.Classify(err) } // ConfiguredProviderFactory returns the production factory: it opens the @@ -296,15 +263,10 @@ func newConfiguredProvider(cmd *cobra.Command, opts *root.Options, cfg config.Fi return nil, gitprovider.Credential{}, nil, err } reader := credentials.ProgressStoreReader("invites", root.NewProgressLogger(opts), resolved, store) - client, credential, err := gitproviders.New(git, reader, options) + client, credential, err := gitproviders.NewInvitationClient(git, reader, options) if err != nil { _ = store.Close() return nil, gitprovider.Credential{}, nil, err } - provider, ok := client.(Provider) - if !ok { - _ = store.Close() - return nil, gitprovider.Credential{}, nil, fmt.Errorf("%w: repository invitations for provider %s", config.ErrUnsupported, git.ProviderKind()) - } - return provider, credential, func() { _ = store.Close() }, nil + return client, credential, func() { _ = store.Close() }, nil } diff --git a/internal/cmd/invitescmd/invitescmd_test.go b/internal/cmd/invitescmd/invitescmd_test.go index 1b84c8be..5660e3e0 100644 --- a/internal/cmd/invitescmd/invitescmd_test.go +++ b/internal/cmd/invitescmd/invitescmd_test.go @@ -112,7 +112,7 @@ func TestAcceptPartialFailureReportsEveryResultThenExitsNonzero(t *testing.T) { if result.Summary != (view.InvitesAcceptCounts{Accepted: 1, Skipped: 2, Failed: 1}) { t.Fatalf("summary = %#v", result.Summary) } - if result.Results[0].Repository != "example-org/docs" || result.Results[0].Status != "failed" || !strings.Contains(result.Results[0].Reason, "permission denied") { + if result.Results[0].Repository != "example-org/docs" || result.Results[0].Status != "failed" || result.Results[0].ReasonCode != "permission" || !strings.Contains(result.Results[0].Reason, "permission denied") { t.Fatalf("first result = %#v, want failed docs invitation with reason", result.Results[0]) } if accepted := h.fake.RecordedAcceptedInvitations(); !reflect.DeepEqual(accepted, []gitprovider.InvitationID{"1"}) { @@ -123,7 +123,7 @@ func TestAcceptPartialFailureReportsEveryResultThenExitsNonzero(t *testing.T) { func TestAcceptJSONShape(t *testing.T) { h := newHarness(t, testConfig()) - out, err := h.run("--profile", "home", "invites", "accept", "--owner", "example-org", "--repo", "other-org/tooling", "--json") + out, err := h.run("--profile", "home", "invites", "accept", "--owner", "example-org", "--repo", "other-org/expired", "--json") if err != nil { t.Fatalf("Execute: %v\n%s", err, out) } diff --git a/internal/cmd/invitescmd/testdata/accept.golden.json b/internal/cmd/invitescmd/testdata/accept.golden.json index 7efb4091..ed5e3aeb 100644 --- a/internal/cmd/invitescmd/testdata/accept.golden.json +++ b/internal/cmd/invitescmd/testdata/accept.golden.json @@ -9,7 +9,7 @@ "example-org" ], "repos": [ - "other-org/tooling" + "other-org/expired" ] }, "results": [ @@ -42,7 +42,8 @@ "expired": true, "url": "https://github.com/other-org/expired/invitations", "status": "skipped", - "reason": "not on the allowlist" + "reason_code": "expired", + "reason": "invitation expired; ask the inviter to send a new one" }, { "id": "2", @@ -52,13 +53,15 @@ "created_at": "2026-10-29T08:00:00Z", "expired": false, "url": "https://github.com/other-org/tooling/invitations", - "status": "accepted" + "status": "skipped", + "reason_code": "not_allowlisted", + "reason": "not on the allowlist" } ], "summary": { - "accepted": 3, + "accepted": 2, "would_accept": 0, - "skipped": 1, + "skipped": 2, "failed": 0 } } diff --git a/internal/cmd/mecmd/mecmd.go b/internal/cmd/mecmd/mecmd.go index 081b5eb2..34a27216 100644 --- a/internal/cmd/mecmd/mecmd.go +++ b/internal/cmd/mecmd/mecmd.go @@ -3,13 +3,11 @@ package mecmd import ( "context" - "errors" "fmt" "io" "sort" "strings" - "github.com/open-cli-collective/cli-common/credstore" "github.com/spf13/cobra" "github.com/open-cli-collective/codereview-cli/internal/cmd/cmderr" @@ -159,36 +157,7 @@ func prevalidateAllIdentityProfile(name string, profile config.Profile) error { } func mapRunError(err error) error { - switch { - case errors.Is(err, config.ErrInvalid), - errors.Is(err, config.ErrNotConfigured), - errors.Is(err, config.ErrProfileNotFound), - errors.Is(err, config.ErrSecretsStoreNotFound), - errors.Is(err, config.ErrUnsupported): - return cmderr.Config(err) - case errors.Is(err, gitprovider.ErrAuth), - errors.Is(err, gitprovider.ErrPermission), - errors.Is(err, gitprovider.ErrRetryable), - errors.Is(err, gitprovider.ErrNotFound), - errors.Is(err, gitprovider.ErrConflict), - errors.Is(err, gitprovider.ErrStaleSHA): - return cmderr.Provider(err) - // Keep this list in sync with cmderr.Credential. The explicit guard avoids - // accidentally classifying arbitrary errors as credential-domain failures. - case errors.Is(err, credentials.ErrInvalidBackendSelection), - errors.Is(err, credentials.ErrWrongService), - errors.Is(err, credstore.ErrRefEmpty), - errors.Is(err, credstore.ErrRefSegmentCount), - errors.Is(err, credstore.ErrRefInvalidChar), - errors.Is(err, credstore.ErrKeyNotAllowed), - errors.Is(err, credstore.ErrExists), - errors.Is(err, credstore.ErrFilePassphraseRequired), - errors.Is(err, credstore.ErrSecretServiceFailClosed), - errors.Is(err, credstore.ErrStoreClosed), - errors.Is(err, credstore.ErrBackendNotImplemented): - return cmderr.Credential(err) - } - return err + return cmderr.Classify(err) } func newProviderResolver(cmd *cobra.Command, opts *root.Options, cfg config.File) (identity.Resolver, func(), error) { diff --git a/internal/gitprovider/invitations.go b/internal/gitprovider/invitations.go index ad090b41..7a94a1d2 100644 --- a/internal/gitprovider/invitations.go +++ b/internal/gitprovider/invitations.go @@ -31,6 +31,12 @@ func (i RepositoryInvitation) FullName() string { // addressed to the authenticated identity. It is separate from GitProvider // because it acts on the account rather than on a pull request, so review // runtime wrappers need not forward it. +// +// Every host adapter implements it, so gitproviders can construct an +// invitation client for any configured host without a capability check. A +// host or credential with no invitations to accept, such as GitLab or a +// GitHub App installation, returns an error wrapping config.ErrUnsupported +// from both methods; the adapter owns that decision. type RepositoryInvitations interface { ListRepositoryInvitations(ctx context.Context) ([]RepositoryInvitation, error) AcceptRepositoryInvitation(ctx context.Context, id InvitationID) error diff --git a/internal/gitproviders/invitations.go b/internal/gitproviders/invitations.go new file mode 100644 index 00000000..50ceb3eb --- /dev/null +++ b/internal/gitproviders/invitations.go @@ -0,0 +1,35 @@ +package gitproviders + +import ( + "context" + + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/credentials" + "github.com/open-cli-collective/codereview-cli/internal/gitprovider" + githubprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/github" + gitlabprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/gitlab" +) + +// InvitationClient is a host adapter's identity lookup plus the repository +// invitations addressed to that identity. +type InvitationClient interface { + WhoAmI(ctx context.Context, creds gitprovider.Credential) (gitprovider.Identity, error) + gitprovider.RepositoryInvitations +} + +// NewInvitationClient builds the adapter selected by the git config, typed for +// repository invitations, along with the credential used to authenticate it. +func NewInvitationClient(git config.GitConfig, store credentials.Reader, opts Options) (InvitationClient, gitprovider.Credential, error) { + if git.ProviderKind() == config.GitProviderGitLab { + client, credential, err := gitlabprovider.NewFromGitConfig(git, store, opts.GitLab) + if err != nil { + return nil, gitprovider.Credential{}, err + } + return client, credential, nil + } + client, credential, err := githubprovider.NewFromGitConfig(git, store, opts.GitHub) + if err != nil { + return nil, gitprovider.Credential{}, err + } + return client, credential, nil +} diff --git a/internal/gitproviders/invitations_test.go b/internal/gitproviders/invitations_test.go new file mode 100644 index 00000000..49fd4823 --- /dev/null +++ b/internal/gitproviders/invitations_test.go @@ -0,0 +1,44 @@ +package gitproviders + +import ( + "errors" + "testing" + + "github.com/open-cli-collective/codereview-cli/internal/config" + "github.com/open-cli-collective/codereview-cli/internal/credentials" + githubprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/github" + gitlabprovider "github.com/open-cli-collective/codereview-cli/internal/gitprovider/gitlab" +) + +func TestNewInvitationClientDispatchesOnProviderKind(t *testing.T) { + store := tokenStore{"work": {credentials.GitTokenKey: "token"}} + client, credential, err := NewInvitationClient(testGitConfig("", "github.example.com"), store, Options{}) + if err != nil { + t.Fatalf("NewInvitationClient github: %v", err) + } + if _, ok := client.(*githubprovider.Client); !ok { + t.Fatalf("client = %T, want github client for empty provider kind", client) + } + if credential.Type != "pat" || credential.Token != "token" { + t.Fatalf("credential = %#v, want PAT", credential) + } + client, _, err = NewInvitationClient(testGitConfig(config.GitProviderGitLab, "gitlab.example.com"), store, Options{}) + if err != nil { + t.Fatalf("NewInvitationClient gitlab: %v", err) + } + if _, ok := client.(*gitlabprovider.Client); !ok { + t.Fatalf("client = %T, want gitlab client", client) + } +} + +func TestNewInvitationClientReturnsNilClientOnError(t *testing.T) { + for _, provider := range []config.GitProviderKind{config.GitProviderGitHub, config.GitProviderGitLab} { + client, _, err := NewInvitationClient(testGitConfig(provider, "example.com"), tokenStore{}, Options{}) + if !errors.Is(err, errTokenNotFound) { + t.Fatalf("%s error = %v, want missing token", provider, err) + } + if client != nil { + t.Fatalf("%s client = %#v, want a nil interface on error", provider, client) + } + } +} diff --git a/internal/invitations/invitations.go b/internal/invitations/invitations.go index a655e110..926dca54 100644 --- a/internal/invitations/invitations.go +++ b/internal/invitations/invitations.go @@ -101,17 +101,33 @@ const ( StatusFailed Status = "failed" ) +// ReasonCode is the closed, machine-readable reason for a skipped or failed +// outcome. Accepted and would-accept outcomes carry no reason. +type ReasonCode string + // Skip reasons. const ( - ReasonNotAllowlisted = "not on the allowlist" - ReasonExpired = "invitation expired; ask the inviter to send a new one" + ReasonNotAllowlisted ReasonCode = "not_allowlisted" + ReasonExpired ReasonCode = "expired" +) + +// Failure reasons, classified from the provider error. +const ( + ReasonAuth ReasonCode = "auth" + ReasonPermission ReasonCode = "permission" + ReasonNotFound ReasonCode = "not_found" + ReasonConflict ReasonCode = "conflict" + ReasonRetryable ReasonCode = "retryable" + ReasonCanceled ReasonCode = "canceled" + ReasonError ReasonCode = "error" ) -// Outcome records what an accept run did with one pending invitation. +// Outcome records what an accept run did with one pending invitation. Err +// holds the provider error of a failed outcome. type Outcome struct { Invitation gitprovider.RepositoryInvitation Status Status - Reason string + Reason ReasonCode Err error } @@ -169,7 +185,7 @@ func Accept(ctx context.Context, provider gitprovider.RepositoryInvitations, all default: if err := provider.AcceptRepositoryInvitation(ctx, invitation.ID); err != nil { outcome.Status = StatusFailed - outcome.Reason = err.Error() + outcome.Reason = failureReason(err) outcome.Err = err } else { outcome.Status = StatusAccepted @@ -180,6 +196,25 @@ func Accept(ctx context.Context, provider gitprovider.RepositoryInvitations, all return outcomes, nil } +func failureReason(err error) ReasonCode { + switch { + case errors.Is(err, context.Canceled), errors.Is(err, context.DeadlineExceeded): + return ReasonCanceled + case errors.Is(err, gitprovider.ErrAuth): + return ReasonAuth + case errors.Is(err, gitprovider.ErrPermission): + return ReasonPermission + case errors.Is(err, gitprovider.ErrNotFound): + return ReasonNotFound + case errors.Is(err, gitprovider.ErrConflict): + return ReasonConflict + case errors.Is(err, gitprovider.ErrRetryable): + return ReasonRetryable + default: + return ReasonError + } +} + func sortInvitations(invitations []gitprovider.RepositoryInvitation) { sort.SliceStable(invitations, func(i, j int) bool { left := strings.ToLower(invitations[i].FullName()) diff --git a/internal/invitations/invitations_test.go b/internal/invitations/invitations_test.go index 1360addd..fba642e1 100644 --- a/internal/invitations/invitations_test.go +++ b/internal/invitations/invitations_test.go @@ -155,7 +155,7 @@ func TestAcceptContinuesPastFailures(t *testing.T) { if err != nil { t.Fatalf("Accept: %v", err) } - if outcomes[0].Status != StatusFailed || !errors.Is(outcomes[0].Err, gitprovider.ErrPermission) || outcomes[0].Reason != failure.Error() { + if outcomes[0].Status != StatusFailed || !errors.Is(outcomes[0].Err, gitprovider.ErrPermission) || outcomes[0].Reason != ReasonPermission { t.Fatalf("first outcome = %#v, want failed with the provider error", outcomes[0]) } if outcomes[1].Status != StatusAccepted { @@ -179,6 +179,27 @@ func TestAcceptReturnsListingFailure(t *testing.T) { } } +func TestFailureReasonClassifiesProviderErrors(t *testing.T) { + tests := []struct { + err error + want ReasonCode + }{ + {gitprovider.WrapError(gitprovider.ErrAuth, gitprovider.OperationAcceptRepositoryInvitation, nil), ReasonAuth}, + {gitprovider.WrapError(gitprovider.ErrPermission, gitprovider.OperationAcceptRepositoryInvitation, nil), ReasonPermission}, + {gitprovider.WrapError(gitprovider.ErrNotFound, gitprovider.OperationAcceptRepositoryInvitation, nil), ReasonNotFound}, + {gitprovider.WrapError(gitprovider.ErrConflict, gitprovider.OperationAcceptRepositoryInvitation, nil), ReasonConflict}, + {gitprovider.WrapError(gitprovider.ErrRetryable, gitprovider.OperationAcceptRepositoryInvitation, nil), ReasonRetryable}, + {context.Canceled, ReasonCanceled}, + {context.DeadlineExceeded, ReasonCanceled}, + {errors.New("github: validation error"), ReasonError}, + } + for _, tt := range tests { + if got := failureReason(tt.err); got != tt.want { + t.Fatalf("failureReason(%v) = %q, want %q", tt.err, got, tt.want) + } + } +} + func fakeWithInvitations() *gitprovider.Fake { fake := &gitprovider.Fake{} fake.SetRepositoryInvitations([]gitprovider.RepositoryInvitation{ diff --git a/internal/view/invites.go b/internal/view/invites.go index 9b518bc5..5b11475d 100644 --- a/internal/view/invites.go +++ b/internal/view/invites.go @@ -50,10 +50,12 @@ type InviteItem struct { } // InviteAcceptItem describes what accept did with one pending invitation. +// ReasonCode is the stable value for scripts; Reason is the human text. type InviteAcceptItem struct { InviteItem - Status string `json:"status"` - Reason string `json:"reason,omitempty"` + Status string `json:"status"` + ReasonCode string `json:"reason_code,omitempty"` + Reason string `json:"reason,omitempty"` } // InvitesAcceptCounts counts accept outcomes by status. @@ -80,7 +82,8 @@ func NewInvitesAccept(identity InvitesIdentity, allow invitations.Allowlist, dry results = append(results, InviteAcceptItem{ InviteItem: newInviteItem(outcome.Invitation), Status: string(outcome.Status), - Reason: outcome.Reason, + ReasonCode: string(outcome.Reason), + Reason: inviteReasonText(outcome), }) } summary := invitations.Summarize(outcomes) @@ -101,6 +104,30 @@ func NewInvitesAccept(identity InvitesIdentity, allow invitations.Allowlist, dry } } +// inviteReasonText gives skips a fixed sentence and failures the provider's +// error text, which the adapters already scrub of response bodies. +func inviteReasonText(outcome invitations.Outcome) string { + switch outcome.Reason { + case invitations.ReasonNotAllowlisted: + return "not on the allowlist" + case invitations.ReasonExpired: + return "invitation expired; ask the inviter to send a new one" + case invitations.ReasonAuth, + invitations.ReasonPermission, + invitations.ReasonNotFound, + invitations.ReasonConflict, + invitations.ReasonRetryable, + invitations.ReasonCanceled, + invitations.ReasonError: + if outcome.Err != nil { + return outcome.Err.Error() + } + return string(outcome.Reason) + default: + return "" + } +} + func newInviteItem(invitation gitprovider.RepositoryInvitation) InviteItem { return InviteItem{ ID: string(invitation.ID), From 032cccd2f135963c798c59d632f9e5445368c254 Mon Sep 17 00:00:00 2001 From: piekstra Date: Mon, 2 Nov 2026 18:34:07 -0500 Subject: [PATCH 3/3] fix(invites): keep host timeouts retryable in reason codes Twapp-Session: 59d912f3-982f-402e-8bd8-3e31269a28b0 --- internal/invitations/invitations.go | 9 ++++++--- internal/invitations/invitations_test.go | 4 +++- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/internal/invitations/invitations.go b/internal/invitations/invitations.go index 926dca54..4ae617f3 100644 --- a/internal/invitations/invitations.go +++ b/internal/invitations/invitations.go @@ -196,10 +196,11 @@ func Accept(ctx context.Context, provider gitprovider.RepositoryInvitations, all return outcomes, nil } +// failureReason classifies an accept failure. The adapter's provider kind +// wins: a host timeout arrives as ErrRetryable with the deadline still in its +// chain and must stay retryable. Only a caller cancellation is "canceled". func failureReason(err error) ReasonCode { switch { - case errors.Is(err, context.Canceled), errors.Is(err, context.DeadlineExceeded): - return ReasonCanceled case errors.Is(err, gitprovider.ErrAuth): return ReasonAuth case errors.Is(err, gitprovider.ErrPermission): @@ -208,8 +209,10 @@ func failureReason(err error) ReasonCode { return ReasonNotFound case errors.Is(err, gitprovider.ErrConflict): return ReasonConflict - case errors.Is(err, gitprovider.ErrRetryable): + case errors.Is(err, gitprovider.ErrRetryable), errors.Is(err, context.DeadlineExceeded): return ReasonRetryable + case errors.Is(err, context.Canceled): + return ReasonCanceled default: return ReasonError } diff --git a/internal/invitations/invitations_test.go b/internal/invitations/invitations_test.go index fba642e1..42e12af8 100644 --- a/internal/invitations/invitations_test.go +++ b/internal/invitations/invitations_test.go @@ -3,6 +3,7 @@ package invitations import ( "context" "errors" + "fmt" "reflect" "testing" @@ -189,8 +190,9 @@ func TestFailureReasonClassifiesProviderErrors(t *testing.T) { {gitprovider.WrapError(gitprovider.ErrNotFound, gitprovider.OperationAcceptRepositoryInvitation, nil), ReasonNotFound}, {gitprovider.WrapError(gitprovider.ErrConflict, gitprovider.OperationAcceptRepositoryInvitation, nil), ReasonConflict}, {gitprovider.WrapError(gitprovider.ErrRetryable, gitprovider.OperationAcceptRepositoryInvitation, nil), ReasonRetryable}, + {gitprovider.WrapError(gitprovider.ErrRetryable, gitprovider.OperationAcceptRepositoryInvitation, fmt.Errorf("client timeout: %w", context.DeadlineExceeded)), ReasonRetryable}, + {context.DeadlineExceeded, ReasonRetryable}, {context.Canceled, ReasonCanceled}, - {context.DeadlineExceeded, ReasonCanceled}, {errors.New("github: validation error"), ReasonError}, } for _, tt := range tests {