diff --git a/.github/skills/writing-acceptance-tests/SKILL.md b/.github/skills/writing-acceptance-tests/SKILL.md index c61c5eea597..2f57ce93bd4 100644 --- a/.github/skills/writing-acceptance-tests/SKILL.md +++ b/.github/skills/writing-acceptance-tests/SKILL.md @@ -11,6 +11,19 @@ creation and cloning without allowing concurrent scripts to interfere. Read `acceptance/README.md` and nearby scripts before editing. Test groups are discovered from `acceptance/testdata//`; do not register them manually. +## Declare token capability + +Every `.txtar` script must start with exactly one capability declaration: + +```txtar +# requires-user-capability: false +``` + +Set the declaration to `true` only when the script requires a user principal, +such as account SSH/GPG keys, personal-account forks, or user membership APIs. +Repository and organization operations supported by an installation token +should use `false`. + ## Choose one repository mode Every script must contain exactly one declaration: @@ -120,7 +133,7 @@ Run metadata checks without live credentials: ```sh go test -tags=acceptance \ - -run '^(TestSelectAcceptanceTestGroups|TestAcceptanceScriptsDeclareFixtureRepository|TestValidateFixtureRepositoryDeclaration|TestFixtureRepositoryManager)$' \ + -run '^(TestSelectAcceptanceTestGroups|TestFilterAcceptanceScripts|TestTokenHasUserCapability|TestAcceptanceScriptsDeclareUserCapabilityRequirement|TestAcceptanceScriptsDeclareFixtureRepository|TestRequiresUserCapabilityForScript|TestValidateFixtureRepositoryDeclaration|TestFixtureRepositoryManager)$' \ ./acceptance ``` diff --git a/acceptance/README.md b/acceptance/README.md index 2f41d5f73cf..d5062527d49 100644 --- a/acceptance/README.md +++ b/acceptance/README.md @@ -24,6 +24,8 @@ The token to use for authenticating with the `GH_ACCEPTANCE_HOST`. This must alr It's recommended to create and use a Legacy PAT for this; Fine-Grained PATs do not offer all the necessary privileges required. You can use an OAuth token provided via `gh auth login --web` and can provide it to the acceptance tests via `GH_ACCEPTANCE_TOKEN=$(gh auth token --hostname )` but this can be a bit confusing and annoying if you `gh auth login` again without `-s` and lose the required scopes. +The test harness infers whether a token authenticates a user from GitHub's documented token prefixes. OAuth (`gho_`), classic PAT (`ghp_`), fine-grained PAT (`github_pat_`), and GitHub App user (`ghu_`) tokens provide user capabilities. GitHub App installation (`ghs_`) tokens do not, so scripts marked `requires-user-capability: true` are omitted from unfiltered runs. Explicitly selecting an incompatible script with `GH_ACCEPTANCE_SCRIPT` fails with an error instead. + Managed fixture repositories reduce repository creation by sharing state where tests can safely coexist. @@ -76,7 +78,14 @@ The following custom environment variables are made available to the scripts: #### Script Metadata -Every script must declare exactly one repository fixture mode: +Every script must begin with a structured header comment declaring whether it +needs a token that authenticates a user: + +```txtar +# requires-user-capability: false +``` + +Every script must also declare exactly one repository fixture mode: ```txtar fixture-repo shared REPO diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index 65597121b7b..73d0e5192ca 100644 --- a/acceptance/acceptance_test.go +++ b/acceptance/acceptance_test.go @@ -645,6 +645,8 @@ func validateAcceptanceScripts(t *testing.T, tsEnv testScriptEnv, groups []strin require.NoError(t, err) for _, file := range candidates { require.NoError(t, validateFixtureRepositoryDeclaration(file)) + _, err := requiresUserCapabilityForScript(file) + require.NoError(t, err) } } } @@ -713,19 +715,128 @@ func TestSelectAcceptanceTestGroups(t *testing.T) { } } +type acceptanceScript struct { + file string + requiresUserCapability bool +} + +func filterAcceptanceScripts(candidates []acceptanceScript, filtered, hasUserCapability bool) ([]string, string, error) { + if filtered && len(candidates) == 0 { + return nil, "no selected script belongs to this command directory", nil + } + + files := make([]string, 0, len(candidates)) + for _, candidate := range candidates { + if candidate.requiresUserCapability && !hasUserCapability { + if filtered { + return nil, "", fmt.Errorf("%s requires a token that authenticates a user", candidate.file) + } + continue + } + files = append(files, candidate.file) + } + if len(files) == 0 { + return nil, "all scripts require a token that authenticates a user", nil + } + + return files, "", nil +} + +func TestFilterAcceptanceScripts(t *testing.T) { + userOnly := acceptanceScript{file: "user.txtar", requiresUserCapability: true} + compatible := acceptanceScript{file: "installation.txtar"} + + tests := []struct { + name string + candidates []acceptanceScript + filtered bool + hasUserCapability bool + wantFiles []string + wantSkip string + wantErr string + }{ + { + name: "user token keeps all scripts", + candidates: []acceptanceScript{userOnly, compatible}, + hasUserCapability: true, + wantFiles: []string{"user.txtar", "installation.txtar"}, + }, + { + name: "installation token omits user-only scripts", + candidates: []acceptanceScript{userOnly, compatible}, + wantFiles: []string{"installation.txtar"}, + }, + { + name: "all incompatible scripts skip", + candidates: []acceptanceScript{userOnly}, + wantSkip: "all scripts require a token that authenticates a user", + }, + { + name: "empty explicit selection skips", + filtered: true, + wantSkip: "no selected script belongs to this command directory", + }, + { + name: "explicit compatible selection remains", + candidates: []acceptanceScript{compatible}, + filtered: true, + wantFiles: []string{"installation.txtar"}, + }, + { + name: "explicit incompatible selection errors", + candidates: []acceptanceScript{userOnly}, + filtered: true, + wantErr: "user.txtar requires a token that authenticates a user", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + files, skipReason, err := filterAcceptanceScripts(tt.candidates, tt.filtered, tt.hasUserCapability) + if tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tt.wantFiles, files) + assert.Equal(t, tt.wantSkip, skipReason) + }) + } +} + func testScriptParamsFor(t *testing.T, tsEnv testScriptEnv, fixtureRepositories *fixtureRepositoryManager, command string) testscript.Params { t.Helper() - candidates, filtered, err := acceptanceScriptCandidates(tsEnv, command) + scriptFiles, filtered, err := acceptanceScriptCandidates(tsEnv, command) if err != nil { t.Fatal(err) } - if filtered && len(candidates) == 0 { - t.Skipf("testdata/%s: no selected script belongs to this command directory", command) + + candidates := make([]acceptanceScript, 0, len(scriptFiles)) + for _, file := range scriptFiles { + requiresUserCapability, err := requiresUserCapabilityForScript(file) + if err != nil { + t.Fatal(err) + } + candidates = append(candidates, acceptanceScript{ + file: file, + requiresUserCapability: requiresUserCapability, + }) + } + + files, skipReason, err := filterAcceptanceScripts(candidates, filtered, tsEnv.hasUserCapability) + if err != nil { + t.Fatal(err) + } + if skipReason != "" { + if filtered { + t.Skipf("testdata/%s: %s", command, skipReason) + } + t.Skip(skipReason) } return testscript.Params{ - Files: candidates, + Files: files, Setup: sharedSetup(tsEnv), Cmds: sharedCmds(tsEnv, fixtureRepositories), RequireExplicitExec: true, @@ -1174,10 +1285,11 @@ func (e missingEnvError) Error() string { } type testScriptEnv struct { - host string - org string - token string - user string + host string + org string + token string + user string + hasUserCapability bool // scripts optionally narrows a run to named scripts within the command // directory being run. Empty means run every script in the directory. @@ -1223,6 +1335,11 @@ func (e *testScriptEnv) fromEnv() error { e.host = envMap["GH_ACCEPTANCE_HOST"] e.org = envMap["GH_ACCEPTANCE_ORG"] e.token = envMap["GH_ACCEPTANCE_TOKEN"] + var err error + e.hasUserCapability, err = tokenHasUserCapability(e.token) + if err != nil { + return err + } e.scripts = parseScriptFilter(os.Getenv("GH_ACCEPTANCE_SCRIPT")) e.preserveWorkDir = os.Getenv("GH_ACCEPTANCE_PRESERVE_WORK_DIR") == "true" diff --git a/acceptance/testdata/api/basic-graphql.txtar b/acceptance/testdata/api/basic-graphql.txtar index 4a0e677c01f..ae430f04156 100644 --- a/acceptance/testdata/api/basic-graphql.txtar +++ b/acceptance/testdata/api/basic-graphql.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/api/basic-rest.txtar b/acceptance/testdata/api/basic-rest.txtar index f25aa9e4d53..c3a678c456c 100644 --- a/acceptance/testdata/api/basic-rest.txtar +++ b/acceptance/testdata/api/basic-rest.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/auth/auth-login-logout.txtar b/acceptance/testdata/auth/auth-login-logout.txtar index 9fbb0274f76..bb12ea1a0a3 100644 --- a/acceptance/testdata/auth/auth-login-logout.txtar +++ b/acceptance/testdata/auth/auth-login-logout.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/auth/auth-setup-git.txtar b/acceptance/testdata/auth/auth-setup-git.txtar index a8f631423f8..690a78ce820 100644 --- a/acceptance/testdata/auth/auth-setup-git.txtar +++ b/acceptance/testdata/auth/auth-setup-git.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/auth/auth-status.txtar b/acceptance/testdata/auth/auth-status.txtar index bddc463d6bc..80af9ef4e0e 100644 --- a/acceptance/testdata/auth/auth-status.txtar +++ b/acceptance/testdata/auth/auth-status.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/auth/auth-token.txtar b/acceptance/testdata/auth/auth-token.txtar index 2efcc6963a7..d3237ddd21e 100644 --- a/acceptance/testdata/auth/auth-token.txtar +++ b/acceptance/testdata/auth/auth-token.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/discussion/discussion-comment.txtar b/acceptance/testdata/discussion/discussion-comment.txtar index 351cad22792..e8c78cfcdec 100644 --- a/acceptance/testdata/discussion/discussion-comment.txtar +++ b/acceptance/testdata/discussion/discussion-comment.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/discussion/discussion-create.txtar b/acceptance/testdata/discussion/discussion-create.txtar index 5df879d2405..1e5fcd39c43 100644 --- a/acceptance/testdata/discussion/discussion-create.txtar +++ b/acceptance/testdata/discussion/discussion-create.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo isolated REPO diff --git a/acceptance/testdata/discussion/discussion-edit.txtar b/acceptance/testdata/discussion/discussion-edit.txtar index fb1487d9d6f..eb4f6d331f5 100644 --- a/acceptance/testdata/discussion/discussion-edit.txtar +++ b/acceptance/testdata/discussion/discussion-edit.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/discussion/discussion-list.txtar b/acceptance/testdata/discussion/discussion-list.txtar index 46a4fd096d4..dab4194cad7 100644 --- a/acceptance/testdata/discussion/discussion-list.txtar +++ b/acceptance/testdata/discussion/discussion-list.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo isolated REPO diff --git a/acceptance/testdata/discussion/discussion-view.txtar b/acceptance/testdata/discussion/discussion-view.txtar index c046b607c26..c0cae547356 100644 --- a/acceptance/testdata/discussion/discussion-view.txtar +++ b/acceptance/testdata/discussion/discussion-view.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo isolated REPO diff --git a/acceptance/testdata/extension/extension-env.txtar b/acceptance/testdata/extension/extension-env.txtar index 23736428303..2f733a5421b 100644 --- a/acceptance/testdata/extension/extension-env.txtar +++ b/acceptance/testdata/extension/extension-env.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Verify that gh tells an extension when it is being run as an extension @@ -10,7 +11,8 @@ fixture-repo none env EXT_NAME=printenv-${RANDOM_STRING} env EXT_DIR=gh-${EXT_NAME} -# Setup a local extension that reports the value of GH_EXTENSION +# Setup a local extension that reports the extension environment and verifies +# it can invoke the current gh executable without relying on PATH mkdir $EXT_DIR mv print-env.sh $EXT_DIR/$EXT_DIR chmod 777 $EXT_DIR/$EXT_DIR @@ -23,19 +25,33 @@ defer gh extension remove $EXT_NAME # Verify GH_EXTENSION is set when the extension is run as gh exec gh $EXT_NAME stdout 'GH_EXTENSION=1' +stdout 'GH_PATH was set correctly' # Verify GH_EXTENSION is set when the extension is run via gh extension exec exec gh extension exec $EXT_NAME stdout 'GH_EXTENSION=1' +stdout 'GH_PATH was set correctly' # Verify GH_EXTENSION is absent when the extension is run standalone exec ./$EXT_DIR stdout 'GH_EXTENSION=0' -# Verify GH_EXTENSION is documented +# Verify GH_EXTENSION and GH_PATH are documented exec gh help environment stdout 'GH_EXTENSION`: set to `1` by gh when it invokes an extension' +stdout 'GH_PATH`: set the path to the gh executable, useful for when gh can not properly determine' -- print-env.sh -- #!/usr/bin/env bash +set -e + echo "GH_EXTENSION=${GH_EXTENSION:-0}" + +if [[ "${GH_EXTENSION:-0}" == "1" ]]; then + expected_token=$GH_TOKEN + mkdir -p empty-path + PATH=$PWD/empty-path + actual_token=$("$GH_PATH" auth token --hostname "$GH_HOST") + [[ "$actual_token" == "$expected_token" ]] + echo "GH_PATH was set correctly" +fi diff --git a/acceptance/testdata/extension/extension.txtar b/acceptance/testdata/extension/extension.txtar index 9bfdb1b6c8a..2f8b8bff311 100644 --- a/acceptance/testdata/extension/extension.txtar +++ b/acceptance/testdata/extension/extension.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Skip if Bash is not available given script extension [!exec:bash] skip diff --git a/acceptance/testdata/gist/gist-create-view-delete.txtar b/acceptance/testdata/gist/gist-create-view-delete.txtar index b1b4660eb33..23c0ba4cc00 100644 --- a/acceptance/testdata/gist/gist-create-view-delete.txtar +++ b/acceptance/testdata/gist/gist-create-view-delete.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true fixture-repo none diff --git a/acceptance/testdata/gist/gist-edit-rename-list.txtar b/acceptance/testdata/gist/gist-edit-rename-list.txtar index 5a91cb5dee2..a552214bf0f 100644 --- a/acceptance/testdata/gist/gist-edit-rename-list.txtar +++ b/acceptance/testdata/gist/gist-edit-rename-list.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true fixture-repo none diff --git a/acceptance/testdata/gpg-key/gpg-key.txtar b/acceptance/testdata/gpg-key/gpg-key.txtar index 81e3bf9f8b8..f767ec1797f 100644 --- a/acceptance/testdata/gpg-key/gpg-key.txtar +++ b/acceptance/testdata/gpg-key/gpg-key.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true skip 'it modifies the user''s personal GitHub account GPG keys' diff --git a/acceptance/testdata/issue/issue-comment-edit-last-with-comments.txtar b/acceptance/testdata/issue/issue-comment-edit-last-with-comments.txtar index 2c43c1c4f95..914e231bc87 100644 --- a/acceptance/testdata/issue/issue-comment-edit-last-with-comments.txtar +++ b/acceptance/testdata/issue/issue-comment-edit-last-with-comments.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript diff --git a/acceptance/testdata/issue/issue-comment-edit-last-without-comments-creates.txtar b/acceptance/testdata/issue/issue-comment-edit-last-without-comments-creates.txtar index bf063a578ef..8d273a00ced 100644 --- a/acceptance/testdata/issue/issue-comment-edit-last-without-comments-creates.txtar +++ b/acceptance/testdata/issue/issue-comment-edit-last-without-comments-creates.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript diff --git a/acceptance/testdata/issue/issue-comment-edit-last-without-comments-errors.txtar b/acceptance/testdata/issue/issue-comment-edit-last-without-comments-errors.txtar index 64e27c599e7..e4c257b011f 100644 --- a/acceptance/testdata/issue/issue-comment-edit-last-without-comments-errors.txtar +++ b/acceptance/testdata/issue/issue-comment-edit-last-without-comments-errors.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript diff --git a/acceptance/testdata/issue/issue-comment-new.txtar b/acceptance/testdata/issue/issue-comment-new.txtar index a8795883963..2605e0d6ea0 100644 --- a/acceptance/testdata/issue/issue-comment-new.txtar +++ b/acceptance/testdata/issue/issue-comment-new.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript diff --git a/acceptance/testdata/issue/issue-create-basic.txtar b/acceptance/testdata/issue/issue-create-basic.txtar index 272e329b57c..e2cb71f5570 100644 --- a/acceptance/testdata/issue/issue-create-basic.txtar +++ b/acceptance/testdata/issue/issue-create-basic.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/issue/issue-create-edit-with-project.txtar b/acceptance/testdata/issue/issue-create-edit-with-project.txtar index f20f02992a4..94055e94a53 100644 --- a/acceptance/testdata/issue/issue-create-edit-with-project.txtar +++ b/acceptance/testdata/issue/issue-create-edit-with-project.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/issue/issue-create-with-metadata.txtar b/acceptance/testdata/issue/issue-create-with-metadata.txtar index b2b8bcb3e31..c9f9f36d5d5 100644 --- a/acceptance/testdata/issue/issue-create-with-metadata.txtar +++ b/acceptance/testdata/issue/issue-create-with-metadata.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/issue/issue-develop-worktree-cross-repo.txtar b/acceptance/testdata/issue/issue-develop-worktree-cross-repo.txtar index 601ad2f0c41..fc5a2e75256 100644 --- a/acceptance/testdata/issue/issue-develop-worktree-cross-repo.txtar +++ b/acceptance/testdata/issue/issue-develop-worktree-cross-repo.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/issue/issue-develop-worktree.txtar b/acceptance/testdata/issue/issue-develop-worktree.txtar index 5e8ebd5fa0a..d92bde5e038 100644 --- a/acceptance/testdata/issue/issue-develop-worktree.txtar +++ b/acceptance/testdata/issue/issue-develop-worktree.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Develop an issue in a fresh worktree, reuse that worktree, and add another # worktree after the local branch exists. diff --git a/acceptance/testdata/issue/issue-list.txtar b/acceptance/testdata/issue/issue-list.txtar index 76a0647ef1c..f3f0017b3ca 100644 --- a/acceptance/testdata/issue/issue-list.txtar +++ b/acceptance/testdata/issue/issue-list.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/issue/issue-view.txtar b/acceptance/testdata/issue/issue-view.txtar index 272e329b57c..e2cb71f5570 100644 --- a/acceptance/testdata/issue/issue-view.txtar +++ b/acceptance/testdata/issue/issue-view.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/issues-2.0/issue-create-and-edit-issue-type.txtar b/acceptance/testdata/issues-2.0/issue-create-and-edit-issue-type.txtar index ae0cb79f5fa..d3088ab9c9e 100644 --- a/acceptance/testdata/issues-2.0/issue-create-and-edit-issue-type.txtar +++ b/acceptance/testdata/issues-2.0/issue-create-and-edit-issue-type.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/issues-2.0/issue-create-and-edit-parent.txtar b/acceptance/testdata/issues-2.0/issue-create-and-edit-parent.txtar index 40a3efbbf55..b1d5efcba38 100644 --- a/acceptance/testdata/issues-2.0/issue-create-and-edit-parent.txtar +++ b/acceptance/testdata/issues-2.0/issue-create-and-edit-parent.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/issues-2.0/issue-create-and-edit-relationships.txtar b/acceptance/testdata/issues-2.0/issue-create-and-edit-relationships.txtar index 6b024ef095e..85bfe2daaa9 100644 --- a/acceptance/testdata/issues-2.0/issue-create-and-edit-relationships.txtar +++ b/acceptance/testdata/issues-2.0/issue-create-and-edit-relationships.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/issues-2.0/issue-edit-sub-issues.txtar b/acceptance/testdata/issues-2.0/issue-edit-sub-issues.txtar index f5e74f1d75c..2022839518d 100644 --- a/acceptance/testdata/issues-2.0/issue-edit-sub-issues.txtar +++ b/acceptance/testdata/issues-2.0/issue-edit-sub-issues.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/issues-2.0/issue-list-filter-by-type.txtar b/acceptance/testdata/issues-2.0/issue-list-filter-by-type.txtar index 35574acb69c..2f13dba3491 100644 --- a/acceptance/testdata/issues-2.0/issue-list-filter-by-type.txtar +++ b/acceptance/testdata/issues-2.0/issue-list-filter-by-type.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/issues-2.0/issue-view-issues-2.0-fields.txtar b/acceptance/testdata/issues-2.0/issue-view-issues-2.0-fields.txtar index 5db98697ef5..28f9adf1195 100644 --- a/acceptance/testdata/issues-2.0/issue-view-issues-2.0-fields.txtar +++ b/acceptance/testdata/issues-2.0/issue-view-issues-2.0-fields.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/label/label.txtar b/acceptance/testdata/label/label.txtar index 27bfe2f751a..2a5b8e2b5b4 100644 --- a/acceptance/testdata/label/label.txtar +++ b/acceptance/testdata/label/label.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup useful env vars diff --git a/acceptance/testdata/org/org-list.txtar b/acceptance/testdata/org/org-list.txtar index fe0b7fae732..4503387ffbf 100644 --- a/acceptance/testdata/org/org-list.txtar +++ b/acceptance/testdata/org/org-list.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true fixture-repo none diff --git a/acceptance/testdata/pr/pr-checkout-by-number.txtar b/acceptance/testdata/pr/pr-checkout-by-number.txtar index cba74cd88c5..842716aa306 100644 --- a/acceptance/testdata/pr/pr-checkout-by-number.txtar +++ b/acceptance/testdata/pr/pr-checkout-by-number.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Set up env vars diff --git a/acceptance/testdata/pr/pr-checkout-with-url-from-fork.txtar b/acceptance/testdata/pr/pr-checkout-with-url-from-fork.txtar index c74a041632b..6601ea83601 100644 --- a/acceptance/testdata/pr/pr-checkout-with-url-from-fork.txtar +++ b/acceptance/testdata/pr/pr-checkout-with-url-from-fork.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/pr/pr-checkout-worktree-detach.txtar b/acceptance/testdata/pr/pr-checkout-worktree-detach.txtar index cc10394d08c..e4bf22d11ce 100644 --- a/acceptance/testdata/pr/pr-checkout-worktree-detach.txtar +++ b/acceptance/testdata/pr/pr-checkout-worktree-detach.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Checkout a PR into a worktree with a detached HEAD, then reuse that worktree. diff --git a/acceptance/testdata/pr/pr-checkout-worktree-from-fork.txtar b/acceptance/testdata/pr/pr-checkout-worktree-from-fork.txtar index d8c675b60d7..7bda9fd2110 100644 --- a/acceptance/testdata/pr/pr-checkout-worktree-from-fork.txtar +++ b/acceptance/testdata/pr/pr-checkout-worktree-from-fork.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/pr/pr-checkout-worktree.txtar b/acceptance/testdata/pr/pr-checkout-worktree.txtar index a7f8c817ffb..2a79382be92 100644 --- a/acceptance/testdata/pr/pr-checkout-worktree.txtar +++ b/acceptance/testdata/pr/pr-checkout-worktree.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Checkout a PR into a git worktree, then reuse that worktree, rename its branch, # force sync it, and add a second worktree once the local branch already exists. diff --git a/acceptance/testdata/pr/pr-checkout.txtar b/acceptance/testdata/pr/pr-checkout.txtar index b14a41a086d..f335a1fd86b 100644 --- a/acceptance/testdata/pr/pr-checkout.txtar +++ b/acceptance/testdata/pr/pr-checkout.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/pr/pr-comment-edit-last-with-comments.txtar b/acceptance/testdata/pr/pr-comment-edit-last-with-comments.txtar index 591346e4169..6e2ec703e12 100644 --- a/acceptance/testdata/pr/pr-comment-edit-last-with-comments.txtar +++ b/acceptance/testdata/pr/pr-comment-edit-last-with-comments.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript diff --git a/acceptance/testdata/pr/pr-comment-edit-last-without-comments-creates.txtar b/acceptance/testdata/pr/pr-comment-edit-last-without-comments-creates.txtar index faa6d87bdf1..789b536ff4c 100644 --- a/acceptance/testdata/pr/pr-comment-edit-last-without-comments-creates.txtar +++ b/acceptance/testdata/pr/pr-comment-edit-last-without-comments-creates.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript diff --git a/acceptance/testdata/pr/pr-comment-edit-last-without-comments-errors.txtar b/acceptance/testdata/pr/pr-comment-edit-last-without-comments-errors.txtar index de22fb63b97..c10f00cad8e 100644 --- a/acceptance/testdata/pr/pr-comment-edit-last-without-comments-errors.txtar +++ b/acceptance/testdata/pr/pr-comment-edit-last-without-comments-errors.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript diff --git a/acceptance/testdata/pr/pr-comment-new.txtar b/acceptance/testdata/pr/pr-comment-new.txtar index 9cb9ef29177..906438502e0 100644 --- a/acceptance/testdata/pr/pr-comment-new.txtar +++ b/acceptance/testdata/pr/pr-comment-new.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript diff --git a/acceptance/testdata/pr/pr-create-basic.txtar b/acceptance/testdata/pr/pr-create-basic.txtar index 72618e7d71b..57cecd9c011 100644 --- a/acceptance/testdata/pr/pr-create-basic.txtar +++ b/acceptance/testdata/pr/pr-create-basic.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/pr/pr-create-edit-with-project.txtar b/acceptance/testdata/pr/pr-create-edit-with-project.txtar index 028b1efc1e5..733b0d7faab 100644 --- a/acceptance/testdata/pr/pr-create-edit-with-project.txtar +++ b/acceptance/testdata/pr/pr-create-edit-with-project.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/pr/pr-create-from-issue-develop-base.txtar b/acceptance/testdata/pr/pr-create-from-issue-develop-base.txtar index 3a2826ecb2e..bbd3fc42feb 100644 --- a/acceptance/testdata/pr/pr-create-from-issue-develop-base.txtar +++ b/acceptance/testdata/pr/pr-create-from-issue-develop-base.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Set up env vars diff --git a/acceptance/testdata/pr/pr-create-from-manual-merge-base.txtar b/acceptance/testdata/pr/pr-create-from-manual-merge-base.txtar index 9e99147c5a4..d0c62e5835d 100644 --- a/acceptance/testdata/pr/pr-create-from-manual-merge-base.txtar +++ b/acceptance/testdata/pr/pr-create-from-manual-merge-base.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Set up env vars diff --git a/acceptance/testdata/pr/pr-create-guesses-remote-from-sha-with-branch-name-slash.txtar b/acceptance/testdata/pr/pr-create-guesses-remote-from-sha-with-branch-name-slash.txtar index 363fce2f8d2..dca2d903bf4 100644 --- a/acceptance/testdata/pr/pr-create-guesses-remote-from-sha-with-branch-name-slash.txtar +++ b/acceptance/testdata/pr/pr-create-guesses-remote-from-sha-with-branch-name-slash.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true skip 'it creates a fork owned by the user running the test' diff --git a/acceptance/testdata/pr/pr-create-guesses-remote-from-sha.txtar b/acceptance/testdata/pr/pr-create-guesses-remote-from-sha.txtar index d290a5ab98e..eff2d668bbd 100644 --- a/acceptance/testdata/pr/pr-create-guesses-remote-from-sha.txtar +++ b/acceptance/testdata/pr/pr-create-guesses-remote-from-sha.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true skip 'it creates a fork owned by the user running the test' diff --git a/acceptance/testdata/pr/pr-create-no-local-repo.txtar b/acceptance/testdata/pr/pr-create-no-local-repo.txtar index 5caadacc3e5..4fd4e234af5 100644 --- a/acceptance/testdata/pr/pr-create-no-local-repo.txtar +++ b/acceptance/testdata/pr/pr-create-no-local-repo.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref-fork.txtar b/acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref-fork.txtar index 1814bbff655..2e039afc7f9 100644 --- a/acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref-fork.txtar +++ b/acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref-fork.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true skip 'it creates a fork owned by the user running the test' skip 'this never worked, but could be fixed if we fixed show-refs' diff --git a/acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref.txtar b/acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref.txtar index d2c5f10a464..d1a60b83d5a 100644 --- a/acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref.txtar +++ b/acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true skip 'it creates a fork owned by the user running the test' diff --git a/acceptance/testdata/pr/pr-create-remote-ref-with-branch-name-slash.txtar b/acceptance/testdata/pr/pr-create-remote-ref-with-branch-name-slash.txtar index d8835b7dd45..c982fbee1d8 100644 --- a/acceptance/testdata/pr/pr-create-remote-ref-with-branch-name-slash.txtar +++ b/acceptance/testdata/pr/pr-create-remote-ref-with-branch-name-slash.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true skip 'it creates a fork owned by the user running the test' diff --git a/acceptance/testdata/pr/pr-create-respects-branch-pushremote.txtar b/acceptance/testdata/pr/pr-create-respects-branch-pushremote.txtar index 5ad31a56844..668b0713b01 100644 --- a/acceptance/testdata/pr/pr-create-respects-branch-pushremote.txtar +++ b/acceptance/testdata/pr/pr-create-respects-branch-pushremote.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true skip 'it creates a fork owned by the user running the test' diff --git a/acceptance/testdata/pr/pr-create-respects-push-destination.txtar b/acceptance/testdata/pr/pr-create-respects-push-destination.txtar index ab8d487fee8..57f789f3ffa 100644 --- a/acceptance/testdata/pr/pr-create-respects-push-destination.txtar +++ b/acceptance/testdata/pr/pr-create-respects-push-destination.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true skip 'it creates a fork owned by the user running the test' diff --git a/acceptance/testdata/pr/pr-create-respects-remote-pushdefault.txtar b/acceptance/testdata/pr/pr-create-respects-remote-pushdefault.txtar index 4877e824f57..0b216eeba29 100644 --- a/acceptance/testdata/pr/pr-create-respects-remote-pushdefault.txtar +++ b/acceptance/testdata/pr/pr-create-respects-remote-pushdefault.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true skip 'it creates a fork owned by the user running the test' diff --git a/acceptance/testdata/pr/pr-create-respects-simple-pushdefault.txtar b/acceptance/testdata/pr/pr-create-respects-simple-pushdefault.txtar index 6fef67600d1..ea6776bf8d7 100644 --- a/acceptance/testdata/pr/pr-create-respects-simple-pushdefault.txtar +++ b/acceptance/testdata/pr/pr-create-respects-simple-pushdefault.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript diff --git a/acceptance/testdata/pr/pr-create-respects-user-colon-branch-syntax.txtar b/acceptance/testdata/pr/pr-create-respects-user-colon-branch-syntax.txtar index 03fbcbfb11c..b227f14fe88 100644 --- a/acceptance/testdata/pr/pr-create-respects-user-colon-branch-syntax.txtar +++ b/acceptance/testdata/pr/pr-create-respects-user-colon-branch-syntax.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true skip 'it creates a fork owned by the user running the test' diff --git a/acceptance/testdata/pr/pr-create-with-metadata.txtar b/acceptance/testdata/pr/pr-create-with-metadata.txtar index 2ae9daf7c1f..27075aba648 100644 --- a/acceptance/testdata/pr/pr-create-with-metadata.txtar +++ b/acceptance/testdata/pr/pr-create-with-metadata.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/pr/pr-create-without-upstream-config.txtar b/acceptance/testdata/pr/pr-create-without-upstream-config.txtar index 0396c6e96ed..f07b5388b32 100644 --- a/acceptance/testdata/pr/pr-create-without-upstream-config.txtar +++ b/acceptance/testdata/pr/pr-create-without-upstream-config.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # This test is the same as pr-create-basic, except that the git push doesn't include the -u argument # This causes a git config read to fail during gh pr create, but it should not be fatal diff --git a/acceptance/testdata/pr/pr-list.txtar b/acceptance/testdata/pr/pr-list.txtar index 10cc25e81a8..d5fcd05f3ad 100644 --- a/acceptance/testdata/pr/pr-list.txtar +++ b/acceptance/testdata/pr/pr-list.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/pr/pr-merge-merge-strategy.txtar b/acceptance/testdata/pr/pr-merge-merge-strategy.txtar index 6c5506de795..426a8fdd176 100644 --- a/acceptance/testdata/pr/pr-merge-merge-strategy.txtar +++ b/acceptance/testdata/pr/pr-merge-merge-strategy.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/pr/pr-status-respects-cross-org.txtar b/acceptance/testdata/pr/pr-status-respects-cross-org.txtar index 95a07b043aa..31e48b29fb0 100644 --- a/acceptance/testdata/pr/pr-status-respects-cross-org.txtar +++ b/acceptance/testdata/pr/pr-status-respects-cross-org.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true skip 'it creates a fork owned by the user running the test' diff --git a/acceptance/testdata/pr/pr-view-outside-repo.txtar b/acceptance/testdata/pr/pr-view-outside-repo.txtar index d68f443fd92..1f787f6961a 100644 --- a/acceptance/testdata/pr/pr-view-outside-repo.txtar +++ b/acceptance/testdata/pr/pr-view-outside-repo.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/pr/pr-view-same-org-fork.txtar b/acceptance/testdata/pr/pr-view-same-org-fork.txtar index 15cd4949d7c..dd16a45e3d2 100644 --- a/acceptance/testdata/pr/pr-view-same-org-fork.txtar +++ b/acceptance/testdata/pr/pr-view-same-org-fork.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/pr/pr-view-status-respects-branch-pushremote.txtar b/acceptance/testdata/pr/pr-view-status-respects-branch-pushremote.txtar index 1a1721d931b..49cc062608f 100644 --- a/acceptance/testdata/pr/pr-view-status-respects-branch-pushremote.txtar +++ b/acceptance/testdata/pr/pr-view-status-respects-branch-pushremote.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/pr/pr-view-status-respects-push-destination.txtar b/acceptance/testdata/pr/pr-view-status-respects-push-destination.txtar index 7880e637e4b..572de89b340 100644 --- a/acceptance/testdata/pr/pr-view-status-respects-push-destination.txtar +++ b/acceptance/testdata/pr/pr-view-status-respects-push-destination.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript diff --git a/acceptance/testdata/pr/pr-view-status-respects-remote-pushdefault.txtar b/acceptance/testdata/pr/pr-view-status-respects-remote-pushdefault.txtar index a537c264761..d8f0ca636a2 100644 --- a/acceptance/testdata/pr/pr-view-status-respects-remote-pushdefault.txtar +++ b/acceptance/testdata/pr/pr-view-status-respects-remote-pushdefault.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/pr/pr-view-status-respects-simple-pushdefault.txtar b/acceptance/testdata/pr/pr-view-status-respects-simple-pushdefault.txtar index b449d379353..251b11e9ba4 100644 --- a/acceptance/testdata/pr/pr-view-status-respects-simple-pushdefault.txtar +++ b/acceptance/testdata/pr/pr-view-status-respects-simple-pushdefault.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript diff --git a/acceptance/testdata/pr/pr-view.txtar b/acceptance/testdata/pr/pr-view.txtar index 1cafe4b19cf..88e37979cfd 100644 --- a/acceptance/testdata/pr/pr-view.txtar +++ b/acceptance/testdata/pr/pr-view.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/project/project-create-delete.txtar b/acceptance/testdata/project/project-create-delete.txtar index a7a8ab114f4..1d5b5b36afd 100644 --- a/acceptance/testdata/project/project-create-delete.txtar +++ b/acceptance/testdata/project/project-create-delete.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/release/release-create.txtar b/acceptance/testdata/release/release-create.txtar index e5c3b610b54..27a6e2a649b 100644 --- a/acceptance/testdata/release/release-create.txtar +++ b/acceptance/testdata/release/release-create.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/release/release-delete.txtar b/acceptance/testdata/release/release-delete.txtar index 02147863f81..d48fb4ffb4d 100644 --- a/acceptance/testdata/release/release-delete.txtar +++ b/acceptance/testdata/release/release-delete.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/release/release-list.txtar b/acceptance/testdata/release/release-list.txtar index e12b76f97ba..e4957438f29 100644 --- a/acceptance/testdata/release/release-list.txtar +++ b/acceptance/testdata/release/release-list.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/release/release-upload-download.txtar b/acceptance/testdata/release/release-upload-download.txtar index 6543fb9d8f0..450fd0de146 100644 --- a/acceptance/testdata/release/release-upload-download.txtar +++ b/acceptance/testdata/release/release-upload-download.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/release/release-view.txtar b/acceptance/testdata/release/release-view.txtar index 7bc6049b120..a3ff187e750 100644 --- a/acceptance/testdata/release/release-view.txtar +++ b/acceptance/testdata/release/release-view.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/repo/repo-archive-unarchive.txtar b/acceptance/testdata/repo/repo-archive-unarchive.txtar index 7c2f34e1e67..2f190d87019 100644 --- a/acceptance/testdata/repo/repo-archive-unarchive.txtar +++ b/acceptance/testdata/repo/repo-archive-unarchive.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo isolated REPO diff --git a/acceptance/testdata/repo/repo-autolink.txtar b/acceptance/testdata/repo/repo-autolink.txtar index 59d01b8df46..f3962ae60d9 100644 --- a/acceptance/testdata/repo/repo-autolink.txtar +++ b/acceptance/testdata/repo/repo-autolink.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository to hold the autolink references fixture-repo shared REPO diff --git a/acceptance/testdata/repo/repo-clone.txtar b/acceptance/testdata/repo/repo-clone.txtar index ddd08fad757..b5b773eeacc 100644 --- a/acceptance/testdata/repo/repo-clone.txtar +++ b/acceptance/testdata/repo/repo-clone.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/repo/repo-create-bare.txtar b/acceptance/testdata/repo/repo-create-bare.txtar index d71356f4756..f9ea03b92ea 100644 --- a/acceptance/testdata/repo/repo-create-bare.txtar +++ b/acceptance/testdata/repo/repo-create-bare.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # It's unclear what we want to do with these acceptance tests beyond our GHEC discovery, so skip new ones by default skip diff --git a/acceptance/testdata/repo/repo-create-view.txtar b/acceptance/testdata/repo/repo-create-view.txtar index a543f8d33bf..8085528dffa 100644 --- a/acceptance/testdata/repo/repo-create-view.txtar +++ b/acceptance/testdata/repo/repo-create-view.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/repo/repo-delete.txtar b/acceptance/testdata/repo/repo-delete.txtar index a629995b536..9df4b14c321 100644 --- a/acceptance/testdata/repo/repo-delete.txtar +++ b/acceptance/testdata/repo/repo-delete.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/repo/repo-deploy-key.txtar b/acceptance/testdata/repo/repo-deploy-key.txtar index 5cc228acd2b..5737b5270ae 100644 --- a/acceptance/testdata/repo/repo-deploy-key.txtar +++ b/acceptance/testdata/repo/repo-deploy-key.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create and clone a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/repo/repo-edit.txtar b/acceptance/testdata/repo/repo-edit.txtar index ca11a02be1e..c559ebe85a9 100644 --- a/acceptance/testdata/repo/repo-edit.txtar +++ b/acceptance/testdata/repo/repo-edit.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo isolated REPO diff --git a/acceptance/testdata/repo/repo-fork-sync.txtar b/acceptance/testdata/repo/repo-fork-sync.txtar index 9a801eb5724..a5ff6de7683 100644 --- a/acceptance/testdata/repo/repo-fork-sync.txtar +++ b/acceptance/testdata/repo/repo-fork-sync.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/repo/repo-list-rename.txtar b/acceptance/testdata/repo/repo-list-rename.txtar index c483adaab72..cc99e3b1fa4 100644 --- a/acceptance/testdata/repo/repo-list-rename.txtar +++ b/acceptance/testdata/repo/repo-list-rename.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/repo/repo-read-dir.txtar b/acceptance/testdata/repo/repo-read-dir.txtar index a997dcf71bc..1f6a7930020 100644 --- a/acceptance/testdata/repo/repo-read-dir.txtar +++ b/acceptance/testdata/repo/repo-read-dir.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # List directory contents of a repository without cloning. diff --git a/acceptance/testdata/repo/repo-read-file.txtar b/acceptance/testdata/repo/repo-read-file.txtar index 2f9786c618b..e30c4791506 100644 --- a/acceptance/testdata/repo/repo-read-file.txtar +++ b/acceptance/testdata/repo/repo-read-file.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Read files from a repository without cloning, in several modes. diff --git a/acceptance/testdata/repo/repo-rename-transfer-ownership.txtar b/acceptance/testdata/repo/repo-rename-transfer-ownership.txtar index e93bfbebd44..3761d832f02 100644 --- a/acceptance/testdata/repo/repo-rename-transfer-ownership.txtar +++ b/acceptance/testdata/repo/repo-rename-transfer-ownership.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/repo/repo-set-default.txtar b/acceptance/testdata/repo/repo-set-default.txtar index 886aa6f7896..8b9dd56d095 100644 --- a/acceptance/testdata/repo/repo-set-default.txtar +++ b/acceptance/testdata/repo/repo-set-default.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create and clone a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/repo/repo-sync-worktree.txtar b/acceptance/testdata/repo/repo-sync-worktree.txtar index 12227f008a1..d69541b2797 100644 --- a/acceptance/testdata/repo/repo-sync-worktree.txtar +++ b/acceptance/testdata/repo/repo-sync-worktree.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/repo/repo-sync.txtar b/acceptance/testdata/repo/repo-sync.txtar index e096c8993a6..020d8c430e0 100644 --- a/acceptance/testdata/repo/repo-sync.txtar +++ b/acceptance/testdata/repo/repo-sync.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/ruleset/ruleset.txtar b/acceptance/testdata/ruleset/ruleset.txtar index b810bc8fc4f..2eb0acd76a2 100644 --- a/acceptance/testdata/ruleset/ruleset.txtar +++ b/acceptance/testdata/ruleset/ruleset.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript diff --git a/acceptance/testdata/search/search-issues.txtar b/acceptance/testdata/search/search-issues.txtar index 9707ef9b793..2d1228020d2 100644 --- a/acceptance/testdata/search/search-issues.txtar +++ b/acceptance/testdata/search/search-issues.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Create a repository with a file so it has a default branch fixture-repo shared REPO diff --git a/acceptance/testdata/secret/secret-org-with-selected-visibility.txtar b/acceptance/testdata/secret/secret-org-with-selected-visibility.txtar index dd0e383b681..6211b657c08 100644 --- a/acceptance/testdata/secret/secret-org-with-selected-visibility.txtar +++ b/acceptance/testdata/secret/secret-org-with-selected-visibility.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript env2upper SECRET_NAME=${SCRIPT_NAME}_${RANDOM_STRING} diff --git a/acceptance/testdata/secret/secret-org.txtar b/acceptance/testdata/secret/secret-org.txtar index 4314dc81e6f..137eda877e1 100644 --- a/acceptance/testdata/secret/secret-org.txtar +++ b/acceptance/testdata/secret/secret-org.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript env REPO=${SCRIPT_NAME}-${RANDOM_STRING} diff --git a/acceptance/testdata/secret/secret-repo-env.txtar b/acceptance/testdata/secret/secret-repo-env.txtar index 3f1b3e45971..93d123f5c8c 100644 --- a/acceptance/testdata/secret/secret-repo-env.txtar +++ b/acceptance/testdata/secret/secret-repo-env.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript env ENV_NAME=testscripts-${RANDOM_STRING} diff --git a/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar b/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar index d3e92e26b59..080f1d7d7e5 100644 --- a/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar +++ b/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-install-force.txtar b/acceptance/testdata/skills/skills-install-force.txtar index d822fd8867b..1d9b19f906b 100644 --- a/acceptance/testdata/skills/skills-install-force.txtar +++ b/acceptance/testdata/skills/skills-install-force.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-install-from-local.txtar b/acceptance/testdata/skills/skills-install-from-local.txtar index 66fb11681cd..e4032858452 100644 --- a/acceptance/testdata/skills/skills-install-from-local.txtar +++ b/acceptance/testdata/skills/skills-install-from-local.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-install-invalid-agent.txtar b/acceptance/testdata/skills/skills-install-invalid-agent.txtar index c680c76de76..c67b6cda539 100644 --- a/acceptance/testdata/skills/skills-install-invalid-agent.txtar +++ b/acceptance/testdata/skills/skills-install-invalid-agent.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-install-invalid-repo.txtar b/acceptance/testdata/skills/skills-install-invalid-repo.txtar index 367b7965add..0b642802c4a 100644 --- a/acceptance/testdata/skills/skills-install-invalid-repo.txtar +++ b/acceptance/testdata/skills/skills-install-invalid-repo.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-install-namespaced.txtar b/acceptance/testdata/skills/skills-install-namespaced.txtar index 5ff22ad2bb6..558626670ff 100644 --- a/acceptance/testdata/skills/skills-install-namespaced.txtar +++ b/acceptance/testdata/skills/skills-install-namespaced.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-install-nested-files.txtar b/acceptance/testdata/skills/skills-install-nested-files.txtar index 2877ac55d26..18708cbaf8c 100644 --- a/acceptance/testdata/skills/skills-install-nested-files.txtar +++ b/acceptance/testdata/skills/skills-install-nested-files.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-install-nonexistent-skill.txtar b/acceptance/testdata/skills/skills-install-nonexistent-skill.txtar index 93671a8a0ac..bf8ccc581eb 100644 --- a/acceptance/testdata/skills/skills-install-nonexistent-skill.txtar +++ b/acceptance/testdata/skills/skills-install-nonexistent-skill.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-install-pin.txtar b/acceptance/testdata/skills/skills-install-pin.txtar index 84978ca61e4..d3a8b7155ee 100644 --- a/acceptance/testdata/skills/skills-install-pin.txtar +++ b/acceptance/testdata/skills/skills-install-pin.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-install-scope.txtar b/acceptance/testdata/skills/skills-install-scope.txtar index 593c052ef68..0b124137107 100644 --- a/acceptance/testdata/skills/skills-install-scope.txtar +++ b/acceptance/testdata/skills/skills-install-scope.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-install.txtar b/acceptance/testdata/skills/skills-install.txtar index bda81a85b57..16c2dca2aad 100644 --- a/acceptance/testdata/skills/skills-install.txtar +++ b/acceptance/testdata/skills/skills-install.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-preview-noninteractive.txtar b/acceptance/testdata/skills/skills-preview-noninteractive.txtar index f01327a4856..28da7364975 100644 --- a/acceptance/testdata/skills/skills-preview-noninteractive.txtar +++ b/acceptance/testdata/skills/skills-preview-noninteractive.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-preview.txtar b/acceptance/testdata/skills/skills-preview.txtar index b2050af435a..a25d17fc384 100644 --- a/acceptance/testdata/skills/skills-preview.txtar +++ b/acceptance/testdata/skills/skills-preview.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-publish-dir-remote.txtar b/acceptance/testdata/skills/skills-publish-dir-remote.txtar index 5725841fbfd..233bc39154e 100644 --- a/acceptance/testdata/skills/skills-publish-dir-remote.txtar +++ b/acceptance/testdata/skills/skills-publish-dir-remote.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-publish-dry-run.txtar b/acceptance/testdata/skills/skills-publish-dry-run.txtar index 0ab7be60ba1..050fb65bd15 100644 --- a/acceptance/testdata/skills/skills-publish-dry-run.txtar +++ b/acceptance/testdata/skills/skills-publish-dry-run.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-publish-lifecycle.txtar b/acceptance/testdata/skills/skills-publish-lifecycle.txtar index ed39c5bd211..7d772420a3e 100644 --- a/acceptance/testdata/skills/skills-publish-lifecycle.txtar +++ b/acceptance/testdata/skills/skills-publish-lifecycle.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Full publish lifecycle: create repo, publish, install from it, clean up diff --git a/acceptance/testdata/skills/skills-search.txtar b/acceptance/testdata/skills/skills-search.txtar index 7dc24db2af2..640b9433f95 100644 --- a/acceptance/testdata/skills/skills-search.txtar +++ b/acceptance/testdata/skills/skills-search.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-update-inplace.txtar b/acceptance/testdata/skills/skills-update-inplace.txtar index 4fecb809cff..ef473f3f17b 100644 --- a/acceptance/testdata/skills/skills-update-inplace.txtar +++ b/acceptance/testdata/skills/skills-update-inplace.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-update-noinstalled.txtar b/acceptance/testdata/skills/skills-update-noinstalled.txtar index 0fe60302b31..f98406c4430 100644 --- a/acceptance/testdata/skills/skills-update-noinstalled.txtar +++ b/acceptance/testdata/skills/skills-update-noinstalled.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/skills/skills-update.txtar b/acceptance/testdata/skills/skills-update.txtar index 5f5194286be..03607b8a09a 100644 --- a/acceptance/testdata/skills/skills-update.txtar +++ b/acceptance/testdata/skills/skills-update.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/ssh-key/ssh-key.txtar b/acceptance/testdata/ssh-key/ssh-key.txtar index f7fd2c63978..22044b43e0c 100644 --- a/acceptance/testdata/ssh-key/ssh-key.txtar +++ b/acceptance/testdata/ssh-key/ssh-key.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: true skip 'it modifies the user''s personal GitHub account SSH keys' diff --git a/acceptance/testdata/telemetry/accessibility-dimensions-disabled.txtar b/acceptance/testdata/telemetry/accessibility-dimensions-disabled.txtar index e3617f90c75..4fcdcf4626a 100644 --- a/acceptance/testdata/telemetry/accessibility-dimensions-disabled.txtar +++ b/acceptance/testdata/telemetry/accessibility-dimensions-disabled.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/telemetry/accessibility-dimensions.txtar b/acceptance/testdata/telemetry/accessibility-dimensions.txtar index 3988f402285..26741902ede 100644 --- a/acceptance/testdata/telemetry/accessibility-dimensions.txtar +++ b/acceptance/testdata/telemetry/accessibility-dimensions.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/telemetry/agent-dimensions.txtar b/acceptance/testdata/telemetry/agent-dimensions.txtar index bb6a208597d..7e56b5da53e 100644 --- a/acceptance/testdata/telemetry/agent-dimensions.txtar +++ b/acceptance/testdata/telemetry/agent-dimensions.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/telemetry/command-invocation.txtar b/acceptance/testdata/telemetry/command-invocation.txtar index 18abc1ac824..77ebb235449 100644 --- a/acceptance/testdata/telemetry/command-invocation.txtar +++ b/acceptance/testdata/telemetry/command-invocation.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/telemetry/no-telemetry-for-alias.txtar b/acceptance/testdata/telemetry/no-telemetry-for-alias.txtar index 9087881a55e..ab716b14d77 100644 --- a/acceptance/testdata/telemetry/no-telemetry-for-alias.txtar +++ b/acceptance/testdata/telemetry/no-telemetry-for-alias.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/telemetry/no-telemetry-for-completion.txtar b/acceptance/testdata/telemetry/no-telemetry-for-completion.txtar index 24966fa3a5b..5cae921a090 100644 --- a/acceptance/testdata/telemetry/no-telemetry-for-completion.txtar +++ b/acceptance/testdata/telemetry/no-telemetry-for-completion.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/telemetry/no-telemetry-for-extension.txtar b/acceptance/testdata/telemetry/no-telemetry-for-extension.txtar index f0f668421d1..a21c661da13 100644 --- a/acceptance/testdata/telemetry/no-telemetry-for-extension.txtar +++ b/acceptance/testdata/telemetry/no-telemetry-for-extension.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Third-party extensions must not generate telemetry events, since the # extension command name can be a user-authored identifier (e.g. an diff --git a/acceptance/testdata/telemetry/no-telemetry-for-ghes-user.txtar b/acceptance/testdata/telemetry/no-telemetry-for-ghes-user.txtar index 1c145ba4480..da8595ab0f7 100644 --- a/acceptance/testdata/telemetry/no-telemetry-for-ghes-user.txtar +++ b/acceptance/testdata/telemetry/no-telemetry-for-ghes-user.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/telemetry/no-telemetry-for-send-telemetry.txtar b/acceptance/testdata/telemetry/no-telemetry-for-send-telemetry.txtar index f11191e98af..309f190e827 100644 --- a/acceptance/testdata/telemetry/no-telemetry-for-send-telemetry.txtar +++ b/acceptance/testdata/telemetry/no-telemetry-for-send-telemetry.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/telemetry/telemetry-failure-does-not-break-command.txtar b/acceptance/testdata/telemetry/telemetry-failure-does-not-break-command.txtar index 16f96524077..8ba4a67aec4 100644 --- a/acceptance/testdata/telemetry/telemetry-failure-does-not-break-command.txtar +++ b/acceptance/testdata/telemetry/telemetry-failure-does-not-break-command.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/telemetry/telemetry-for-official-extension-stub.txtar b/acceptance/testdata/telemetry/telemetry-for-official-extension-stub.txtar index d6ddc6109af..07ae59787e1 100644 --- a/acceptance/testdata/telemetry/telemetry-for-official-extension-stub.txtar +++ b/acceptance/testdata/telemetry/telemetry-for-official-extension-stub.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/variable/variable-org.txtar b/acceptance/testdata/variable/variable-org.txtar index 7f7f6197180..5b16a35d62c 100644 --- a/acceptance/testdata/variable/variable-org.txtar +++ b/acceptance/testdata/variable/variable-org.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false fixture-repo none diff --git a/acceptance/testdata/variable/variable-repo-env.txtar b/acceptance/testdata/variable/variable-repo-env.txtar index da2445ea100..765e6a0c2cf 100644 --- a/acceptance/testdata/variable/variable-repo-env.txtar +++ b/acceptance/testdata/variable/variable-repo-env.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript env ENV_NAME=testscripts-${RANDOM_STRING} diff --git a/acceptance/testdata/variable/variable-repo.txtar b/acceptance/testdata/variable/variable-repo.txtar index cf20c9a33f4..0fc804a9c68 100644 --- a/acceptance/testdata/variable/variable-repo.txtar +++ b/acceptance/testdata/variable/variable-repo.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Setup environment variables used for testscript env2upper VAR_NAME=TESTSCRIPTS_${RANDOM_STRING} diff --git a/acceptance/testdata/workflow/cache-list-delete.txtar b/acceptance/testdata/workflow/cache-list-delete.txtar index ba10629338a..263a0fd0ba2 100644 --- a/acceptance/testdata/workflow/cache-list-delete.txtar +++ b/acceptance/testdata/workflow/cache-list-delete.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/workflow/cache-list-empty.txtar b/acceptance/testdata/workflow/cache-list-empty.txtar index 694151da8b3..bb4f4af2481 100644 --- a/acceptance/testdata/workflow/cache-list-empty.txtar +++ b/acceptance/testdata/workflow/cache-list-empty.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # It's unclear what we want to do with these acceptance tests beyond our GHEC discovery, so skip new ones by default skip diff --git a/acceptance/testdata/workflow/run-cancel.txtar b/acceptance/testdata/workflow/run-cancel.txtar index 9830bf06a24..8bbffbd0893 100644 --- a/acceptance/testdata/workflow/run-cancel.txtar +++ b/acceptance/testdata/workflow/run-cancel.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/workflow/run-delete.txtar b/acceptance/testdata/workflow/run-delete.txtar index 6b87f203216..c98a6a6e2f1 100644 --- a/acceptance/testdata/workflow/run-delete.txtar +++ b/acceptance/testdata/workflow/run-delete.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/workflow/run-download.txtar b/acceptance/testdata/workflow/run-download.txtar index 35926cba020..6afb2b0c3ef 100644 --- a/acceptance/testdata/workflow/run-download.txtar +++ b/acceptance/testdata/workflow/run-download.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Set up env diff --git a/acceptance/testdata/workflow/run-rerun.txtar b/acceptance/testdata/workflow/run-rerun.txtar index 8413aff868d..2eaadc3733e 100644 --- a/acceptance/testdata/workflow/run-rerun.txtar +++ b/acceptance/testdata/workflow/run-rerun.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/workflow/run-view-log-escape-sequences.txtar b/acceptance/testdata/workflow/run-view-log-escape-sequences.txtar index f2d407a9974..b11cf82592e 100644 --- a/acceptance/testdata/workflow/run-view-log-escape-sequences.txtar +++ b/acceptance/testdata/workflow/run-view-log-escape-sequences.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # This test ensures that a malicious workflow which emit terminal control sequences (ESC, OSC, CSI) in # its log output does not result in terminal injection when logs are displayed using `gh run view --log` diff --git a/acceptance/testdata/workflow/run-view.txtar b/acceptance/testdata/workflow/run-view.txtar index 0002afa2b60..bb82380c262 100644 --- a/acceptance/testdata/workflow/run-view.txtar +++ b/acceptance/testdata/workflow/run-view.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/workflow/workflow-enable-disable.txtar b/acceptance/testdata/workflow/workflow-enable-disable.txtar index 8965561a64f..9804ade40a4 100644 --- a/acceptance/testdata/workflow/workflow-enable-disable.txtar +++ b/acceptance/testdata/workflow/workflow-enable-disable.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/workflow/workflow-list.txtar b/acceptance/testdata/workflow/workflow-list.txtar index 70722e2e0db..17171fe65e8 100644 --- a/acceptance/testdata/workflow/workflow-list.txtar +++ b/acceptance/testdata/workflow/workflow-list.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/workflow/workflow-run.txtar b/acceptance/testdata/workflow/workflow-run.txtar index 1cbdad36728..e24171f0876 100644 --- a/acceptance/testdata/workflow/workflow-run.txtar +++ b/acceptance/testdata/workflow/workflow-run.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/testdata/workflow/workflow-view.txtar b/acceptance/testdata/workflow/workflow-view.txtar index b43c04c03d1..4fde30b9108 100644 --- a/acceptance/testdata/workflow/workflow-view.txtar +++ b/acceptance/testdata/workflow/workflow-view.txtar @@ -1,3 +1,4 @@ +# requires-user-capability: false # Use gh as a credential helper exec gh auth setup-git diff --git a/acceptance/user_capability_test.go b/acceptance/user_capability_test.go index dc78b9abc77..92484d0c8b7 100644 --- a/acceptance/user_capability_test.go +++ b/acceptance/user_capability_test.go @@ -9,6 +9,7 @@ import ( "strings" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -25,6 +26,61 @@ when the script needs clean or repository-global state, and none when the script creates its own repositories or does not need one. See acceptance/README.md#script-metadata for details.` +func tokenHasUserCapability(token string) (bool, error) { + switch { + case strings.HasPrefix(token, "ghp_"), + strings.HasPrefix(token, "gho_"), + strings.HasPrefix(token, "github_pat_"), + strings.HasPrefix(token, "ghu_"): + return true, nil + case strings.HasPrefix(token, "ghs_"): + return false, nil + default: + return false, fmt.Errorf("GH_ACCEPTANCE_TOKEN has an unsupported token prefix") + } +} + +func requiresUserCapabilityForScript(file string) (bool, error) { + f, err := os.Open(file) + if err != nil { + return false, err + } + defer f.Close() + + scanner := bufio.NewScanner(f) + if !scanner.Scan() { + if err := scanner.Err(); err != nil { + return false, err + } + return false, fmt.Errorf("%s: first line must be '# requires-user-capability: true' or '# requires-user-capability: false'", file) + } + + var requiresUserCapability bool + switch strings.TrimSuffix(scanner.Text(), "\r") { + case "# requires-user-capability: true": + requiresUserCapability = true + case "# requires-user-capability: false": + requiresUserCapability = false + default: + return false, fmt.Errorf("%s: first line must be '# requires-user-capability: true' or '# requires-user-capability: false'", file) + } + + for scanner.Scan() { + line := strings.TrimSuffix(scanner.Text(), "\r") + if strings.HasPrefix(line, "-- ") && strings.HasSuffix(line, " --") { + break + } + if strings.HasPrefix(line, "# requires-user-capability:") { + return false, fmt.Errorf("%s: script must contain exactly one requires-user-capability declaration", file) + } + } + if err := scanner.Err(); err != nil { + return false, err + } + + return requiresUserCapability, nil +} + func validateFixtureRepositoryDeclaration(file string) error { f, err := os.Open(file) if err != nil { @@ -72,6 +128,43 @@ func validateFixtureRepositoryDeclaration(file string) error { } } +func TestTokenHasUserCapability(t *testing.T) { + tests := []struct { + token string + want bool + }{ + {token: "ghp_token", want: true}, + {token: "gho_token", want: true}, + {token: "github_pat_token", want: true}, + {token: "ghu_token", want: true}, + {token: "ghs_token", want: false}, + } + + for _, tt := range tests { + t.Run(tt.token[:4], func(t *testing.T) { + got, err := tokenHasUserCapability(tt.token) + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } + + _, err := tokenHasUserCapability("unsupported") + assert.EqualError(t, err, "GH_ACCEPTANCE_TOKEN has an unsupported token prefix") +} + +func TestAcceptanceScriptsDeclareUserCapabilityRequirement(t *testing.T) { + files, err := filepath.Glob(filepath.Join("testdata", "*", "*.txtar")) + require.NoError(t, err) + require.NotEmpty(t, files) + + for _, file := range files { + t.Run(file, func(t *testing.T) { + _, err := requiresUserCapabilityForScript(file) + require.NoError(t, err) + }) + } +} + func TestAcceptanceScriptsDeclareFixtureRepository(t *testing.T) { files, err := filepath.Glob(filepath.Join("testdata", "*", "*.txtar")) require.NoError(t, err) @@ -84,6 +177,71 @@ func TestAcceptanceScriptsDeclareFixtureRepository(t *testing.T) { } } +func TestRequiresUserCapabilityForScript(t *testing.T) { + tests := []struct { + name string + content string + want bool + wantErr string + }{ + { + name: "requires user capability", + content: "# requires-user-capability: true\n", + want: true, + }, + { + name: "does not require user capability", + content: "# requires-user-capability: false\n", + }, + { + name: "supports CRLF", + content: "# requires-user-capability: true\r\n", + want: true, + }, + { + name: "missing", + content: "", + wantErr: "first line must be", + }, + { + name: "not first", + content: "# explanation\n# requires-user-capability: true\n", + wantErr: "first line must be", + }, + { + name: "invalid value", + content: "# requires-user-capability: unknown\n", + wantErr: "first line must be", + }, + { + name: "multiple", + content: "# requires-user-capability: false\n# requires-user-capability: true\n", + wantErr: "script must contain exactly one requires-user-capability declaration", + }, + { + name: "ignores archive contents", + content: "# requires-user-capability: false\n\n" + + "-- script.sh --\n" + + "# requires-user-capability: true\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + file := filepath.Join(t.TempDir(), "script.txtar") + require.NoError(t, os.WriteFile(file, []byte(tt.content), 0o600)) + + got, err := requiresUserCapabilityForScript(file) + if tt.wantErr == "" { + require.NoError(t, err) + assert.Equal(t, tt.want, got) + } else { + require.ErrorContains(t, err, tt.wantErr) + } + }) + } +} + func TestValidateFixtureRepositoryDeclaration(t *testing.T) { tests := []struct { name string diff --git a/internal/attachments/attach.go b/internal/attachments/attach.go index 73c5b7e195c..de2a59432a7 100644 --- a/internal/attachments/attach.go +++ b/internal/attachments/attach.go @@ -6,21 +6,28 @@ import ( "strings" ) +// UploadResult reports the successful upload and markdown operation counts. +type UploadResult struct { + Uploaded int + AppendOperations int + ReplaceOperations int +} + // UploadAndAttach uploads assets in order and stops at the first failure. It // points existing references at the URLs of successful uploads and appends only // successful uploads the markdown did not reference. Assets after a failure // are not attempted. // -// The returned count reports how many assets reached the server. A caller must -// write the returned markdown when that count is above zero, even when the -// returned error is non-nil. An upload cannot be undone and there is no endpoint -// to delete one, so discarding that markdown would orphan the successful assets. -// A count of zero means nothing was uploaded and nothing is lost by writing -// nothing. +// The result reports how many assets reached the server and how those successful +// uploads changed the markdown. A caller must write the returned markdown when +// Uploaded is above zero, even when the returned error is non-nil. An upload +// cannot be undone and there is no endpoint to delete one, so discarding that +// markdown would orphan the successful assets. An Uploaded count of zero means +// nothing was uploaded and nothing is lost by writing nothing. // // The markdown is returned unchanged when it could not be rewritten, so a // caller that assigns the result in place never destroys what it was given. -func (u *Uploader) UploadAndAttach(ctx context.Context, md string, assets []UserAsset) (string, int, error) { +func (u *Uploader) UploadAndAttach(ctx context.Context, md string, assets []UserAsset) (string, UploadResult, error) { args := make([]attachmentArg, len(assets)) for i, a := range assets { f := a.getAsset() @@ -29,11 +36,11 @@ func (u *Uploader) UploadAndAttach(ctx context.Context, md string, assets []User attachableMD, err := newAttachableMarkdown(md, args) if err != nil { - return md, 0, err + return md, UploadResult{}, err } var failures []error - uploaded := 0 + result := UploadResult{} for i, a := range assets { assetURL, err := u.upload(ctx, a) if err != nil { @@ -42,16 +49,18 @@ func (u *Uploader) UploadAndAttach(ctx context.Context, md string, assets []User break } args[i].URL = assetURL - uploaded++ + result.Uploaded++ } attachedMD, err := attachAssetsToMarkdown(attachableMD) if err != nil { failures = append(failures, err) - return md, uploaded, errors.Join(failures...) + return md, result, errors.Join(failures...) } - return appendUnreferenced(attachedMD, assets), uploaded, errors.Join(failures...) + result.AppendOperations = len(attachedMD.ToAppend) + result.ReplaceOperations = attachedMD.ReplaceOperations + return appendUnreferenced(attachedMD, assets), result, errors.Join(failures...) } // appendUnreferenced adds a paragraph for every attachment the author never diff --git a/internal/attachments/attach_test.go b/internal/attachments/attach_test.go index c1698587161..1288d1cf129 100644 --- a/internal/attachments/attach_test.go +++ b/internal/attachments/attach_test.go @@ -41,14 +41,15 @@ func TestUploaderUploadAndAttach(t *testing.T) { } tests := []struct { - name string - files []string - args []string - body string - uploads []upload - wantBody string - // What a caller keys on to decide whether a body is worth writing. + name string + files []string + args []string + body string + uploads []upload + wantBody string wantUploaded int + wantAppend int + wantReplace int wantErr string }{ { @@ -59,6 +60,7 @@ func TestUploaderUploadAndAttach(t *testing.T) { uploads: []upload{{201, `{"url":"https://github.com/user-attachments/assets/1"}`}}, wantBody: "See below\n\n![login](https://github.com/user-attachments/assets/1)", wantUploaded: 1, + wantAppend: 1, }, { name: "appends a video as a paragraph of its own", @@ -70,6 +72,7 @@ func TestUploaderUploadAndAttach(t *testing.T) { // paragraph, so it must not land on the end of the line above. wantBody: "Watch this:\n\nhttps://github.com/user-attachments/assets/2", wantUploaded: 1, + wantAppend: 1, }, { name: "appends to an empty body without leading blank lines", @@ -79,6 +82,7 @@ func TestUploaderUploadAndAttach(t *testing.T) { uploads: []upload{{201, `{"url":"https://github.com/user-attachments/assets/1"}`}}, wantBody: "![login](https://github.com/user-attachments/assets/1)", wantUploaded: 1, + wantAppend: 1, }, { name: "does not stack blank lines on a body that ends with them", @@ -88,6 +92,7 @@ func TestUploaderUploadAndAttach(t *testing.T) { uploads: []upload{{201, `{"url":"https://github.com/user-attachments/assets/1"}`}}, wantBody: "See below\n\n![login](https://github.com/user-attachments/assets/1)", wantUploaded: 1, + wantAppend: 1, }, { name: "appends several assets in the order they were written", @@ -100,6 +105,7 @@ func TestUploaderUploadAndAttach(t *testing.T) { "![After the fix](https://example.com/2)\n\n" + "https://example.com/3", wantUploaded: 3, + wantAppend: 3, }, { name: "rewrites a reference in place instead of appending it", @@ -109,6 +115,7 @@ func TestUploaderUploadAndAttach(t *testing.T) { uploads: []upload{{201, `{"url":"https://example.com/1"}`}}, wantBody: "The error:\n\n![the login screen](https://example.com/1)\n\nThat is all.", wantUploaded: 1, + wantReplace: 1, }, { name: "rewrites what the body references and appends what it does not", @@ -118,6 +125,8 @@ func TestUploaderUploadAndAttach(t *testing.T) { uploads: []upload{{201, `{"url":"https://example.com/1"}`}, {201, `{"url":"https://example.com/2"}`}}, wantBody: "![the login screen](https://example.com/1)\n\n![after](https://example.com/2)", wantUploaded: 2, + wantAppend: 1, + wantReplace: 1, }, { // Three files and two replies: c is never attempted, which the @@ -129,6 +138,7 @@ func TestUploaderUploadAndAttach(t *testing.T) { uploads: []upload{{201, `{"url":"https://example.com/1"}`}, {404, `{"message":"Not Found"}`}}, wantBody: "Three files\n\n![a](https://example.com/1)", wantUploaded: 1, + wantAppend: 1, wantErr: "could not upload ./b.png: attaching files requires write access to the repository", }, { @@ -170,6 +180,7 @@ func TestUploaderUploadAndAttach(t *testing.T) { uploads: []upload{{201, `{"url":"https://example.com/1"}`}}, wantBody: "[clip][c]\n\n[c]: https://example.com/1", wantUploaded: 1, + wantReplace: 1, }, { // The only place a UserAsset becomes an attachmentArg, so the only row @@ -183,6 +194,7 @@ func TestUploaderUploadAndAttach(t *testing.T) { uploads: []upload{{201, `{"url":"https://example.com/1"}`}}, wantBody: "The crash [repro.mp4](https://example.com/1) reproduces every time.", wantUploaded: 1, + wantReplace: 1, }, } @@ -202,7 +214,7 @@ func TestUploaderUploadAndAttach(t *testing.T) { ) } - body, uploaded, err := testUploader(reg).UploadAndAttach(context.Background(), tt.body, assets) + body, result, err := testUploader(reg).UploadAndAttach(context.Background(), tt.body, assets) if tt.wantErr == "" { require.NoError(t, err) @@ -210,7 +222,10 @@ func TestUploaderUploadAndAttach(t *testing.T) { require.EqualError(t, err, tt.wantErr) } assert.Equal(t, tt.wantBody, body) - assert.Equal(t, tt.wantUploaded, uploaded) + assert.Equal(t, tt.wantUploaded, result.Uploaded) + assert.Equal(t, tt.wantAppend, result.AppendOperations) + assert.Equal(t, tt.wantReplace, result.ReplaceOperations) + assert.Equal(t, result.Uploaded, result.AppendOperations+result.ReplaceOperations) // The bytes go up in the order they were attached, so a failure // stops the ones after it rather than reordering them. @@ -239,11 +254,11 @@ func TestUploaderUploadAndAttachUploadsOnceForRepeatedReferences(t *testing.T) { httpmock.StatusStringResponse(201, `{"url":"https://example.com/1"}`), ) - body, uploaded, err := testUploader(reg).UploadAndAttach(context.Background(), + body, result, err := testUploader(reg).UploadAndAttach(context.Background(), "![one](./shot.png)\n\ntext\n\n![two](./shot.png)", assets) require.NoError(t, err) - assert.Equal(t, 1, uploaded) + assert.Equal(t, UploadResult{Uploaded: 1, ReplaceOperations: 1}, result) assert.Equal(t, "![one](https://example.com/1)\n\ntext\n\n![two](https://example.com/1)", body) assert.Len(t, reg.Requests, 1) } @@ -252,10 +267,10 @@ func TestUploaderUploadAndAttachNoAssets(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t) - body, uploaded, err := testUploader(reg).UploadAndAttach(context.Background(), "unchanged\n", nil) + body, result, err := testUploader(reg).UploadAndAttach(context.Background(), "unchanged\n", nil) require.NoError(t, err) - assert.Zero(t, uploaded) + assert.Equal(t, UploadResult{}, result) assert.Equal(t, "unchanged\n", body) assert.Empty(t, reg.Requests) } @@ -299,12 +314,12 @@ func TestUploaderUploadAndAttachDoesNotLeakTheAssetURLIntoAnError(t *testing.T) httpmock.StatusStringResponse(404, `{"message":"Not Found"}`), ) - body, uploaded, err := testUploader(reg).UploadAndAttach(context.Background(), "", assets) + body, result, err := testUploader(reg).UploadAndAttach(context.Background(), "", assets) require.Error(t, err) // One asset is up and cannot be deleted, so the caller must write this // body even though the call failed. - assert.Equal(t, 1, uploaded) + assert.Equal(t, UploadResult{Uploaded: 1, AppendOperations: 1}, result) assert.NotContains(t, err.Error(), "secret-asset") assert.Contains(t, body, "secret-asset") } @@ -324,9 +339,9 @@ func TestUploaderUploadAndAttachAbsolutePathReference(t *testing.T) { httpmock.StatusStringResponse(201, `{"url":"https://example.com/1"}`), ) - body, uploaded, err := testUploader(reg).UploadAndAttach(context.Background(), "![shot](./shot.png)", assets) + body, result, err := testUploader(reg).UploadAndAttach(context.Background(), "![shot](./shot.png)", assets) require.NoError(t, err) - assert.Equal(t, 1, uploaded) + assert.Equal(t, UploadResult{Uploaded: 1, ReplaceOperations: 1}, result) assert.Equal(t, "![shot](https://example.com/1)", body) } diff --git a/internal/attachments/doc.go b/internal/attachments/doc.go index f13473e77c9..f6f7b3ed5e7 100644 --- a/internal/attachments/doc.go +++ b/internal/attachments/doc.go @@ -25,16 +25,17 @@ // // // Every reasonable cancellation possible belongs here. // -// md, uploaded, err := uploader.UploadAndAttach(ctx, md, attachmentArgs) -// if uploaded == 0 { +// md, result, err := uploader.UploadAndAttach(ctx, md, attachmentArgs) +// if result.Uploaded == 0 { // return err // } // // // Write md to target, then report err alongside whatever the write // // returned. // -// UploadAndAttach reports how many assets were successfully uploaded. The -// caller must write the markdown when that count is above zero, including after -// a partial failure, because what did upload must be referenced by something. -// At zero nothing is stranded and nothing is written. +// UploadAndAttach reports how many assets were successfully uploaded and how +// they changed the markdown. The caller must write the markdown when Uploaded +// is above zero, including after a partial failure, because what did upload must +// be referenced by something. At zero nothing is stranded and nothing is +// written. package attachments diff --git a/internal/attachments/flags.go b/internal/attachments/flags.go index 10e42fd6fca..f2fe9076f9b 100644 --- a/internal/attachments/flags.go +++ b/internal/attachments/flags.go @@ -6,7 +6,6 @@ import ( "os" "strings" - "github.com/cli/cli/v2/internal/gh/ghtelemetry" "github.com/spf13/cobra" "github.com/spf13/pflag" ) @@ -40,22 +39,10 @@ func (f *Flag) Changed() bool { return f.flag.Changed } -// RecordTelemetry records how many attachment flags were provided. -func (f *Flag) RecordTelemetry(command string, recorder ghtelemetry.CommandRecorder) { - if recorder == nil || !f.Changed() { - return - } - - recorder.SetSampleRate(ghtelemetry.SAMPLE_ALL) - recorder.Record(ghtelemetry.Event{ - Type: "attachment_invocation", - Dimensions: ghtelemetry.Dimensions{ - "command": command, - }, - Measures: ghtelemetry.Measures{ - "attach_count": int64(len(f.values)), - }, - }) +// Count returns the number of supplied attachment values before validation. +// Absent flags count as zero; empty paths and values over the limit still count. +func (f *Flag) Count() int { + return len(f.values) } // UserAssets validates the files named by the attachment flag, keeping them in diff --git a/internal/attachments/flags_test.go b/internal/attachments/flags_test.go index af22157715e..8bb6b798b00 100644 --- a/internal/attachments/flags_test.go +++ b/internal/attachments/flags_test.go @@ -11,7 +11,6 @@ import ( "github.com/cli/cli/v2/internal/telemetry" "github.com/google/shlex" "github.com/spf13/cobra" - "github.com/spf13/pflag" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -48,6 +47,8 @@ func assetsFromArgs(t *testing.T, args ...string) ([]UserAsset, error) { } func TestAddFlag(t *testing.T) { + t.Parallel() + tests := []struct { name string input string @@ -77,69 +78,118 @@ func TestAddFlag(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // Given a command with the repeatable attachment flag cmd, attachFlag := attachCmd(t, tt.input) - slice, ok := attachFlag.flag.Value.(pflag.SliceValue) - require.True(t, ok) - assert.Equal(t, tt.want, slice.GetSlice()) - assert.Equal(t, tt.input != "", attachFlag.Changed()) - assert.Empty(t, attachFlag.flag.Shorthand) - assert.Equal(t, "Attach an image or video `file`, in '#' format", attachFlag.flag.Usage) - assert.Same(t, attachFlag.flag, cmd.Flags().Lookup(flagName)) + // When the parsed flag values are read + values, err := cmd.Flags().GetStringArray("attach") + + // Then values retain their spelling and order + require.NoError(t, err) + assert.Equal(t, tt.want, values) + assert.Equal(t, tt.input != "", attachFlag.Changed(), "Changed reports whether --attach was supplied") + flag := cmd.Flags().Lookup("attach") + assert.Empty(t, flag.Shorthand) + assert.Equal(t, "Attach an image or video `file`, in '#' format", flag.Usage) }) } } -func TestFlagRecordTelemetry(t *testing.T) { +func TestAttachmentTelemetry(t *testing.T) { + t.Parallel() + + t.Run("flag not passed ignores operation updates", func(t *testing.T) { + t.Parallel() + + // Given a command with no attachment flag + _, attachFlag := attachCmd(t, "") + recorder := &telemetry.InvocationRecorderSpy{} + + // When an operation update is attempted without an attachment event + event := BeginTelemetry(recorder, "gh issue comment", attachFlag.Count()) + event.RecordOperations(UploadResult{AppendOperations: 1, ReplaceOperations: 1}) + + // Then no event or sampling promotion occurs + assert.Nil(t, event) + assert.Empty(t, recorder.Events()) + assert.Zero(t, recorder.LastSampleRate) + }) + tests := []struct { name string input string - wantEvent bool wantCount int64 }{ - { - name: "flag not passed", - input: "", - }, { name: "one attachment", input: "--attach ./first.png", - wantEvent: true, wantCount: 1, }, { name: "several attachments before validation", input: "--attach ./first.png --attach ./second.png --attach ./third.png", - wantEvent: true, wantCount: 3, }, + { + name: "empty path counts before validation", + input: `--attach ""`, + wantCount: 1, + }, + { + name: "over limit counts before validation", + input: strings.Repeat("--attach ./missing.png ", maxAttachments+1), + wantCount: maxAttachments + 1, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // Given raw attachment inputs that have not been validated _, attachFlag := attachCmd(t, tt.input) - recorder := &telemetry.CommandRecorderSpy{} + recorder := &telemetry.InvocationRecorderSpy{} - attachFlag.RecordTelemetry("gh issue comment", recorder) + // When an attachment event is recorded without any upload operations + BeginTelemetry(recorder, "gh issue comment", attachFlag.Count()) - if !tt.wantEvent { - assert.Empty(t, recorder.Events) - assert.Zero(t, recorder.LastSampleRate) - return - } - - require.Equal(t, ghtelemetry.SAMPLE_ALL, recorder.LastSampleRate) - require.Equal(t, []ghtelemetry.Event{{ + // Then the raw count is retained at full sampling with zero operations + assert.Equal(t, ghtelemetry.SAMPLE_ALL, recorder.LastSampleRate) + assert.Equal(t, []ghtelemetry.Event{{ Type: "attachment_invocation", Dimensions: ghtelemetry.Dimensions{ "command": "gh issue comment", }, Measures: ghtelemetry.Measures{ - "attach_count": tt.wantCount, + "attach_count": tt.wantCount, + "append_ops_count": 0, + "replace_ops_count": 0, }, - }}, recorder.Events) + }}, recorder.Events()) }) } + + t.Run("completed markdown operations", func(t *testing.T) { + t.Parallel() + + // Given a pending event for four attachments + recorder := &telemetry.InvocationRecorderSpy{} + event := BeginTelemetry(recorder, "gh issue comment", 4) + + // When four uploads produce one append and three replacements + event.RecordOperations(UploadResult{Uploaded: 4, AppendOperations: 1, ReplaceOperations: 3}) + + // Then telemetry retains the attachment count and records one append and three replacements + events := recorder.Events() + require.Len(t, events, 1) + assert.Equal(t, ghtelemetry.Measures{ + "attach_count": 4, + "append_ops_count": 1, + "replace_ops_count": 3, + }, events[0].Measures) + }) } func TestFlagUserAssets(t *testing.T) { @@ -187,11 +237,6 @@ func TestFlagUserAssets(t *testing.T) { wantErr: "./gone.png: ", wantErrIs: fs.ErrNotExist, }, - { - name: "several files, in the order written", - input: "--attach ./b.png --attach ./a.png", - wantPaths: []string{"./b.png", "./a.png"}, - }, { name: "too many attachments are rejected before filesystem validation", input: strings.Repeat("--attach ./missing.txt ", maxAttachments+1), @@ -204,9 +249,7 @@ func TestFlagUserAssets(t *testing.T) { wantErrIs: fs.ErrNotExist, }, { - // pflag reads this flag back as holding nothing at all, so - // without the length check the command would post with no - // attachment and no error. + // An explicitly empty value is invalid, not an absent flag. name: "a lone empty value", input: `--attach ""`, wantErr: "cannot attach an empty path; --attach needs a file path", @@ -226,11 +269,6 @@ func TestFlagUserAssets(t *testing.T) { input: `--attach ./before,after.png`, wantPaths: []string{"./before,after.png"}, }, - { - name: "keeps the order the arguments were written in", - input: "--attach './b.png#Second' --attach ./a.png --attach ./c.mp4", - wantPaths: []string{"./b.png", "./a.png", "./c.mp4"}, - }, { name: "the same file twice", input: "--attach ./a.png --attach './a.png#Another caption'", @@ -262,12 +300,6 @@ func TestFlagUserAssets(t *testing.T) { input: "--attach ./a.png --attach ./hard.png", wantErr: "./a.png and ./hard.png are the same file; attached files must be unique", }, - { - // GitHub gives each its own asset URL. - name: "two separate files with identical contents", - input: "--attach ./a.png --attach ./b.png", - wantPaths: []string{"./a.png", "./b.png"}, - }, { name: "reports the first invalid file", input: "--attach ./a.png --attach ./notes.txt", @@ -277,6 +309,7 @@ func TestFlagUserAssets(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // Given the named files and raw attachment arguments t.Chdir(t.TempDir()) for _, name := range []string{ "shot.png", @@ -284,8 +317,6 @@ func TestFlagUserAssets(t *testing.T) { "shot#dark.png#first.png", "caption.png", "a.png", - "b.png", - "c.mp4", "before,after.png", "notes.txt", } { @@ -297,8 +328,10 @@ func TestFlagUserAssets(t *testing.T) { _, attachFlag := attachCmd(t, tt.input) + // When the attachment files are resolved resolved, err := attachFlag.UserAssets() + // Then invalid inputs fail or the resolved paths retain their order if tt.wantErrIs != nil { require.ErrorIs(t, err, tt.wantErrIs) require.ErrorContains(t, err, tt.wantErr) @@ -324,6 +357,36 @@ func TestFlagUserAssets(t *testing.T) { }) } + t.Run("preserves input order", func(t *testing.T) { + // Given two files with different contents + t.Chdir(t.TempDir()) + require.NoError(t, os.WriteFile("a.png", []byte("first"), 0o600)) + require.NoError(t, os.WriteFile("b.png", []byte("second"), 0o600)) + + // When files are supplied in non-alphabetical order + resolved, err := assetsFromArgs(t, "./b.png", "./a.png") + + // Then the resolved files retain that order + require.NoError(t, err) + require.Len(t, resolved, 2) + assert.Equal(t, []string{"./b.png", "./a.png"}, []string{resolved[0].Path(), resolved[1].Path()}) + }) + + t.Run("allows distinct files with identical contents", func(t *testing.T) { + // Given two separate files containing the same bytes + t.Chdir(t.TempDir()) + require.NoError(t, os.WriteFile("a.png", []byte("same contents"), 0o600)) + require.NoError(t, os.WriteFile("b.png", []byte("same contents"), 0o600)) + + // When both files are supplied + resolved, err := assetsFromArgs(t, "./a.png", "./b.png") + + // Then both files are accepted + require.NoError(t, err) + require.Len(t, resolved, 2) + assert.ElementsMatch(t, []string{"./a.png", "./b.png"}, []string{resolved[0].Path(), resolved[1].Path()}) + }) + t.Run("maximum number of attachments", func(t *testing.T) { names := make([]string, maxAttachments) for i := range names { diff --git a/internal/attachments/references.go b/internal/attachments/references.go index 85db0bd345f..7cc42f3cb15 100644 --- a/internal/attachments/references.go +++ b/internal/attachments/references.go @@ -14,9 +14,9 @@ import ( ) // This file finds the places a body already points at an attached file, and -// repoints them. A file the author never mentioned is not its business: it is -// reported in ToAppend and appended by attach.go, which knows that an image and -// a video append different markdown. +// repoints them. A file with no rewritten reference is reported in ToAppend and +// appended by attach.go, which knows that an image and a video append different +// markdown. // // The parts below run in that order: the two entry points, finding the // references, working out which attached file each one names, locating its @@ -43,13 +43,15 @@ type attachmentArg struct { RendersAsPlayer bool } -// attachedMarkdown is the outcome of attaching, one field per flow. A file the -// author already referenced is rewritten where they wrote it. A file they never -// mentioned is left for the caller to append, because appending needs to know -// how an image and a video each render and this file does not. +// attachedMarkdown is the outcome of attaching, one field per flow. A file +// rewritten where the author referenced it contributes one replacement, +// regardless of how many references changed. A file that was not rewritten is +// left for the caller to append, because appending needs to know how an image +// and a video each render and this file does not. type attachedMarkdown struct { - Rewritten string - ToAppend []attachmentArg + Rewritten string + ToAppend []attachmentArg + ReplaceOperations int } // attachableMarkdown is markdown scanned for the files it references, held @@ -78,9 +80,10 @@ func newAttachableMarkdown(md string, attachmentArgs []attachmentArg) (attachabl return attachableMarkdown{markdown: md, refs: refs, args: attachmentArgs}, nil } -// attachAssetsToMarkdown points every markdown reference to an attached file -// at that file's asset URL, and reports which arguments the markdown never -// referenced. +// attachAssetsToMarkdown points every rewritable markdown reference to an +// attached file at that file's asset URL. It reports one replacement per +// successfully rewritten attachment and which uploaded arguments remain to +// append. // // ![alt](./file) alone in a paragraph image: swap the path video: the bare URL, which plays // ![alt](./file) anywhere else image: swap the path video: becomes [alt](URL) @@ -173,12 +176,22 @@ func attachAssetsToMarkdown(v attachableMarkdown) (attachedMarkdown, error) { out, written := applyEdits(md, edits) var unreferenced []attachmentArg + replaced := 0 for i, a := range attachmentArgs { - if a.URL != "" && !written[i] { + if a.URL == "" { + continue + } + if written[i] { + replaced++ + } else { unreferenced = append(unreferenced, a) } } - return attachedMarkdown{Rewritten: out, ToAppend: unreferenced}, nil + return attachedMarkdown{ + Rewritten: out, + ToAppend: unreferenced, + ReplaceOperations: replaced, + }, nil } // checkVideoReferenceImages returns an error naming every video the markdown diff --git a/internal/attachments/telemetry.go b/internal/attachments/telemetry.go new file mode 100644 index 00000000000..3631ff81f6a --- /dev/null +++ b/internal/attachments/telemetry.go @@ -0,0 +1,50 @@ +package attachments + +import "github.com/cli/cli/v2/internal/gh/ghtelemetry" + +// TelemetryEvent tracks attachment-specific facts in a pending telemetry event. +// [ghtelemetry.Service.Finish] owns completion; later updates are ignored. +type TelemetryEvent struct { + pendingEvent ghtelemetry.PendingEvent +} + +// BeginTelemetry starts an attachment event at full sampling for the raw supplied count. +// A zero count returns nil without changing sampling. recorder is required; +// use a no-op service when telemetry is disabled. +// Call immediately before Flag.UserAssets so invalid and over-limit inputs count. +func BeginTelemetry(recorder ghtelemetry.InvocationRecorder, command string, attachCount int) *TelemetryEvent { + if attachCount == 0 { + return nil + } + + recorder.SetSampleRate(ghtelemetry.SAMPLE_ALL) + pendingEvent := recorder.Begin(ghtelemetry.Event{ + Type: "attachment_invocation", + Dimensions: ghtelemetry.Dimensions{ + "command": command, + }, + Measures: ghtelemetry.Measures{ + "attach_count": int64(attachCount), + "append_ops_count": 0, + "replace_ops_count": 0, + }, + }) + + return &TelemetryEvent{ + pendingEvent: pendingEvent, + } +} + +// RecordOperations replaces, rather than adds to, the completed operation counts. +// Pass partial results before handling an upload error so successful work is retained. +// A nil event, returned by Begin for zero attachments, is a no-op. +func (e *TelemetryEvent) RecordOperations(result UploadResult) { + if e == nil { + return + } + + e.pendingEvent.UpsertMeasures(ghtelemetry.Measures{ + "append_ops_count": int64(result.AppendOperations), + "replace_ops_count": int64(result.ReplaceOperations), + }) +} diff --git a/internal/attachments/test.go b/internal/attachments/test.go index 12106dbcd90..af6b6e649aa 100644 --- a/internal/attachments/test.go +++ b/internal/attachments/test.go @@ -7,6 +7,7 @@ import ( "strconv" "testing" + "github.com/cli/cli/v2/internal/gh/ghtelemetry" "github.com/cli/cli/v2/pkg/httpmock" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" @@ -39,6 +40,23 @@ func NewTestAssets(t *testing.T, names ...string) []UserAsset { return assets } +// AssertTestTelemetryEvents verifies the completed invocation event shape. +func AssertTestTelemetryEvents(t *testing.T, events []ghtelemetry.Event, attachCount int, result UploadResult) { + t.Helper() + + require.Equal(t, []ghtelemetry.Event{ + { + Type: "attachment_invocation", + Dimensions: ghtelemetry.Dimensions{"command": "gh test"}, + Measures: ghtelemetry.Measures{ + "attach_count": int64(attachCount), + "append_ops_count": int64(result.AppendOperations), + "replace_ops_count": int64(result.ReplaceOperations), + }, + }, + }, events) +} + // StubUpload registers one upload of name against repositoryID, answering with // status and body. // diff --git a/internal/gh/ghtelemetry/telemetry.go b/internal/gh/ghtelemetry/telemetry.go index 197b955b4c1..70c0e056359 100644 --- a/internal/gh/ghtelemetry/telemetry.go +++ b/internal/gh/ghtelemetry/telemetry.go @@ -10,23 +10,36 @@ type Event struct { Measures Measures } +// PendingEvent accepts additional facts until its invocation finishes. +// Upserts copy supplied entries, inserting new keys and overwriting existing ones. +// Unspecified keys are unchanged. Calls after completion have no effect. +type PendingEvent interface { + UpsertDimensions(Dimensions) + UpsertMeasures(Measures) +} + type Disabler interface { Disable() } +// EventRecorder produces complete or in-progress events. type EventRecorder interface { Record(event Event) - Disabler + // Begin records initial facts that can be updated until invocation completion. + Begin(Event) PendingEvent } -type CommandRecorder interface { +// InvocationRecorder produces events and controls invocation-wide sampling. +type InvocationRecorder interface { EventRecorder SetSampleRate(rate int) } +// Service collects telemetry for one command execution and sends it on Finish. type Service interface { - CommandRecorder - Flush() + InvocationRecorder + Disabler + Finish() } const SAMPLE_ALL = 100 diff --git a/internal/ghcmd/cmd.go b/internal/ghcmd/cmd.go index ed4e1d0b574..eb034389633 100644 --- a/internal/ghcmd/cmd.go +++ b/internal/ghcmd/cmd.go @@ -128,7 +128,8 @@ func Main() exitCode { return exitError } } - defer telemetryService.Flush() + // Complete and send events even when returning before Cobra reaches RunE. + defer telemetryService.Finish() cmdFactory := factory.New(buildVersion, string(invokingAgent), cfgFunc, ioStreams, ghExecutablePath, telemetryService) diff --git a/internal/telemetry/fake.go b/internal/telemetry/fake.go index 4eb22e898a5..4086fc765b3 100644 --- a/internal/telemetry/fake.go +++ b/internal/telemetry/fake.go @@ -1,35 +1,67 @@ package telemetry -import "github.com/cli/cli/v2/internal/gh/ghtelemetry" +import ( + "maps" + "github.com/cli/cli/v2/internal/gh/ghtelemetry" +) + +var ( + _ ghtelemetry.EventRecorder = (*EventRecorderSpy)(nil) + _ ghtelemetry.InvocationRecorder = (*InvocationRecorderSpy)(nil) +) + +// EventRecorderSpy captures recorded and pending events in recording order. type EventRecorderSpy struct { - Events []ghtelemetry.Event + events []*ghtelemetry.Event } +// Record captures a complete event. func (r *EventRecorderSpy) Record(event ghtelemetry.Event) { - r.Events = append(r.Events, event) + r.Begin(event) } -func (r *EventRecorderSpy) Disable() {} +// Begin captures initial facts and returns a handle for subsequent updates. +func (r *EventRecorderSpy) Begin(event ghtelemetry.Event) ghtelemetry.PendingEvent { + event = cloneEvent(event) + r.events = append(r.events, &event) + return &pendingEventSpy{event: &event} +} -func (r *EventRecorderSpy) Flush() {} +// Events returns copies of all captured events with their latest updates. +func (r *EventRecorderSpy) Events() []ghtelemetry.Event { + var events []ghtelemetry.Event + for _, event := range r.events { + events = append(events, cloneEvent(*event)) + } + return events +} -// CommandRecorderSpy is a test double for ghtelemetry.CommandRecorder. -// It captures recorded events and the most recent SetSampleRate call so tests can -// assert on the sampling behavior commands attempt to configure. -type CommandRecorderSpy struct { - Events []ghtelemetry.Event +// InvocationRecorderSpy adds invocation sampling to EventRecorderSpy. +type InvocationRecorderSpy struct { + EventRecorderSpy LastSampleRate int } -func (r *CommandRecorderSpy) Record(event ghtelemetry.Event) { - r.Events = append(r.Events, event) +// SetSampleRate captures the sampling policy requested by a command. +func (r *InvocationRecorderSpy) SetSampleRate(rate int) { + r.LastSampleRate = rate } -func (r *CommandRecorderSpy) Disable() {} +type pendingEventSpy struct { + event *ghtelemetry.Event +} -func (r *CommandRecorderSpy) SetSampleRate(rate int) { - r.LastSampleRate = rate +func (p *pendingEventSpy) UpsertDimensions(dimensions ghtelemetry.Dimensions) { + if p.event.Dimensions == nil { + p.event.Dimensions = make(ghtelemetry.Dimensions) + } + maps.Copy(p.event.Dimensions, dimensions) } -func (r *CommandRecorderSpy) Flush() {} +func (p *pendingEventSpy) UpsertMeasures(measures ghtelemetry.Measures) { + if p.event.Measures == nil { + p.event.Measures = make(ghtelemetry.Measures) + } + maps.Copy(p.event.Measures, measures) +} diff --git a/internal/telemetry/service_test.go b/internal/telemetry/service_test.go new file mode 100644 index 00000000000..d08bc67a7e3 --- /dev/null +++ b/internal/telemetry/service_test.go @@ -0,0 +1,157 @@ +package telemetry + +import ( + "sync" + "testing" + "time" + + "github.com/cli/cli/v2/internal/gh/ghtelemetry" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestServiceCopiesRecordedEvents(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + // Given a recorded event + var payload SendTelemetryPayload + service := NewService(func(p SendTelemetryPayload) { payload = p }) + event := ghtelemetry.Event{ + Type: "command_invocation", + Dimensions: ghtelemetry.Dimensions{"command": "gh issue create"}, + Measures: ghtelemetry.Measures{"count": 1}, + } + service.Record(event) + + // When the original maps are mutated before delivery + event.Dimensions["command"] = "changed" + event.Measures["count"] = 2 + service.Finish() + + // Then the recorded values are unchanged + require.Len(t, payload.Events, 1) + assert.Equal(t, "gh issue create", payload.Events[0].Dimensions["command"]) + assert.Equal(t, int64(1), payload.Events[0].Measures["count"]) +} + +func TestServicePromotesAllEventsBeforeCompletion(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + // Given events recorded under a sampling policy that would exclude them + var payload SendTelemetryPayload + svc := NewService(func(p SendTelemetryPayload) { payload = p }, WithSampleRate(1)) + svc.(*service).sampleBucket = 99 + svc.Record(ghtelemetry.Event{Type: "completed_step"}) + svc.Begin(ghtelemetry.Event{Type: "attachment_invocation"}) + + // When sampling is promoted before completion + svc.SetSampleRate(ghtelemetry.SAMPLE_ALL) + svc.Finish() + + // Then both events are delivered + assert.Len(t, payload.Events, 2) +} + +func TestServiceDisablingOverridesPromotedPendingEvents(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + // Given immediate and pending events in a fully sampled invocation + var payloads []SendTelemetryPayload + service := NewService(func(p SendTelemetryPayload) { payloads = append(payloads, p) }) + service.Record(ghtelemetry.Event{Type: "completed_step"}) + pending := service.Begin(ghtelemetry.Event{Type: "attachment_invocation"}) + service.SetSampleRate(ghtelemetry.SAMPLE_ALL) + + // When host discovery disables telemetry before completion + service.Disable() + pending.UpsertMeasures(ghtelemetry.Measures{"attach_count": 2}) + service.Record(ghtelemetry.Event{Type: "another_step"}) + service.Finish() + + // Then delivery receives only the empty payload used by log mode + require.Len(t, payloads, 1) + assert.Empty(t, payloads[0].Events) +} + +func TestServiceFinishDeliversOnce(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + // Given an invocation with a recorded event + deliveries := 0 + service := NewService(func(SendTelemetryPayload) { deliveries++ }) + service.Record(ghtelemetry.Event{Type: "test"}) + + // When completion is called twice + service.Finish() + service.Finish() + + // Then the payload is delivered only once + assert.Equal(t, 1, deliveries) +} + +func TestServiceRecordingDoesNotWaitForDelivery(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + // Given an invocation whose delivery is blocked + sendStarted := make(chan struct{}) + allowSend := make(chan struct{}) + service := NewService(func(SendTelemetryPayload) { + close(sendStarted) + <-allowSend + }) + service.Record(ghtelemetry.Event{Type: "test"}) + deliveryDone := make(chan struct{}) + go func() { + service.Finish() + close(deliveryDone) + }() + <-sendStarted + + // When another recording is attempted + recordingDone := make(chan struct{}) + go func() { + service.Record(ghtelemetry.Event{Type: "too_late"}) + close(recordingDone) + }() + + // Then recording returns without waiting for delivery + select { + case <-recordingDone: + case <-time.After(5 * time.Second): + t.Error("recording blocked while telemetry was being sent") + } + close(allowSend) + <-deliveryDone + <-recordingDone +} + +func TestServiceCollectsConcurrentFacts(t *testing.T) { + t.Cleanup(stubDeviceID("test-device")) + + // Given two command activities contributing to the same pending event + var payload SendTelemetryPayload + service := NewService(func(p SendTelemetryPayload) { payload = p }) + pending := service.Begin(ghtelemetry.Event{Type: "attachment_invocation"}) + + // When both activities finish before command completion + var workers sync.WaitGroup + workers.Go(func() { + pending.UpsertDimensions(ghtelemetry.Dimensions{"command": "gh issue create"}) + pending.UpsertMeasures(ghtelemetry.Measures{"append_ops_count": 1}) + }) + workers.Go(func() { + pending.UpsertDimensions(ghtelemetry.Dimensions{"flags": "attach"}) + pending.UpsertMeasures(ghtelemetry.Measures{"replace_ops_count": 2}) + }) + workers.Wait() + service.Finish() + + // Then the completed event contains both activities' facts + require.Len(t, payload.Events, 1) + assert.Equal(t, "gh issue create", payload.Events[0].Dimensions["command"]) + assert.Equal(t, "attach", payload.Events[0].Dimensions["flags"]) + assert.Equal(t, map[string]int64{ + "append_ops_count": 1, + "replace_ops_count": 2, + }, payload.Events[0].Measures) +} diff --git a/internal/telemetry/telemetry.go b/internal/telemetry/telemetry.go index 3943060b124..c7509b5a2aa 100644 --- a/internal/telemetry/telemetry.go +++ b/internal/telemetry/telemetry.go @@ -158,7 +158,7 @@ func WithAdditionalCommonDimensions(dimensions ghtelemetry.Dimensions) telemetry } } -// WithSampleRate allows setting a sample rate (0-100) for telemetry events. Events recorded with the Unsampled option will be sent regardless of the sample rate. +// WithSampleRate allows setting a sample rate (0-100) for telemetry events. Rates 0 and 100 retain all events. // Sampling is based on invocation ID, so an entire invocation will be included or excluded as a whole. This ensures that related events are not split between sampled and unsampled, // which could lead to incomplete data and incorrect assumptions. func WithSampleRate(rate int) telemetryServiceOption { @@ -252,19 +252,21 @@ type recordedEvent struct { } type service struct { - mu sync.RWMutex - flush func(payload SendTelemetryPayload) - previouslyCalled bool + mu sync.RWMutex + flush func(payload SendTelemetryPayload) + finished bool commonDimensions ghtelemetry.Dimensions sampleRate int sampleBucket byte - events []recordedEvent + events []*recordedEvent disabled bool } +// Disable suppresses all events in the invocation, including already recorded +// events. It must be called before Finish. func (s *service) Disable() { s.mu.Lock() defer s.mu.Unlock() @@ -272,32 +274,61 @@ func (s *service) Disable() { s.disabled = true } +// Record copies a complete event into the service. +// Recording after Finish has no effect. func (s *service) Record(event ghtelemetry.Event) { s.mu.Lock() defer s.mu.Unlock() - s.events = append(s.events, recordedEvent{event: event, recordedAt: time.Now()}) + if s.finished { + return + } + s.events = append(s.events, &recordedEvent{event: cloneEvent(event), recordedAt: time.Now()}) +} + +// Begin copies an event's initial facts and returns a handle for adding +// facts until Finish. Events begun after Finish are not recorded. +func (s *service) Begin(event ghtelemetry.Event) ghtelemetry.PendingEvent { + s.mu.Lock() + defer s.mu.Unlock() + + if s.finished { + return noOpPendingEvent{} + } + recorded := &recordedEvent{ + event: cloneEvent(event), + recordedAt: time.Now(), + } + s.events = append(s.events, recorded) + return &pendingEvent{service: s, recorded: recorded} } +// SetSampleRate selects the sampling policy for the whole invocation. +// Changes after Finish have no effect. func (s *service) SetSampleRate(rate int) { s.mu.Lock() defer s.mu.Unlock() + if s.finished { + return + } s.sampleRate = rate s.commonDimensions["sample_rate"] = strconv.Itoa(rate) } -func (s *service) Flush() { - // This shouldn't really be required since flush should only be called once, but just in case... +// Finish snapshots the recorded events once and sends their payload after releasing the lock. +// Sampling and telemetry eligibility apply to immediate and pending events alike. +func (s *service) Finish() { s.mu.Lock() - defer s.mu.Unlock() - if s.previouslyCalled { + if s.finished { + s.mu.Unlock() return } - s.previouslyCalled = true + s.finished = true if s.sampleRate > 0 && s.sampleRate < 100 && int(s.sampleBucket) >= s.sampleRate { + s.mu.Unlock() return } @@ -315,22 +346,64 @@ func (s *service) Flush() { } for i, recorded := range events { + event := recorded.event + dimensions := map[string]string{ "timestamp": recorded.recordedAt.UTC().Format("2006-01-02T15:04:05.000Z"), } maps.Copy(dimensions, s.commonDimensions) - maps.Copy(dimensions, recorded.event.Dimensions) + maps.Copy(dimensions, event.Dimensions) payload.Events[i] = PayloadEvent{ - Type: recorded.event.Type, + Type: event.Type, Dimensions: dimensions, - Measures: recorded.event.Measures, + Measures: maps.Clone(event.Measures), } } + s.mu.Unlock() s.flush(payload) } +type pendingEvent struct { + service *service + recorded *recordedEvent +} + +func (p *pendingEvent) UpsertDimensions(dimensions ghtelemetry.Dimensions) { + p.service.mu.Lock() + defer p.service.mu.Unlock() + + if p.service.finished { + return + } + if p.recorded.event.Dimensions == nil { + p.recorded.event.Dimensions = make(ghtelemetry.Dimensions) + } + maps.Copy(p.recorded.event.Dimensions, dimensions) +} + +func (p *pendingEvent) UpsertMeasures(measures ghtelemetry.Measures) { + p.service.mu.Lock() + defer p.service.mu.Unlock() + + if p.service.finished { + return + } + if p.recorded.event.Measures == nil { + p.recorded.event.Measures = make(ghtelemetry.Measures) + } + maps.Copy(p.recorded.event.Measures, measures) +} + +func cloneEvent(event ghtelemetry.Event) ghtelemetry.Event { + return ghtelemetry.Event{ + Type: event.Type, + Dimensions: maps.Clone(event.Dimensions), + Measures: maps.Clone(event.Measures), + } +} + // maxPayloadSize is a safety limit for the telemetry payload written to the // child process stdin pipe. This bounds the data transferred to a reasonable // size and avoids blocking on pipe buffer capacity (typically 16-64 KB). @@ -414,12 +487,27 @@ func SpawnSendTelemetry(executable string, payload SendTelemetryPayload) { _ = cmd.Process.Release() } +type noOpPendingEvent struct{} + +func (noOpPendingEvent) UpsertDimensions(ghtelemetry.Dimensions) {} +func (noOpPendingEvent) UpsertMeasures(ghtelemetry.Measures) {} + +// NoOpService discards telemetry when collection is disabled. type NoOpService struct{} +// Record discards the event. func (s *NoOpService) Record(event ghtelemetry.Event) {} +// Begin returns an inert handle without retaining the event. +func (s *NoOpService) Begin(event ghtelemetry.Event) ghtelemetry.PendingEvent { + return noOpPendingEvent{} +} + +// Disable leaves telemetry disabled. func (s *NoOpService) Disable() {} +// SetSampleRate leaves telemetry disabled. func (s *NoOpService) SetSampleRate(rate int) {} -func (s *NoOpService) Flush() {} +// Finish has no payload to complete. +func (s *NoOpService) Finish() {} diff --git a/internal/telemetry/telemetry_test.go b/internal/telemetry/telemetry_test.go index 98180a1263c..352b6c02e25 100644 --- a/internal/telemetry/telemetry_test.go +++ b/internal/telemetry/telemetry_test.go @@ -3,16 +3,13 @@ package telemetry import ( "bytes" "errors" - "maps" "os" "path/filepath" "strings" "sync" "testing" - "time" "github.com/cli/cli/v2/internal/gh/ghtelemetry" - "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -41,27 +38,6 @@ func stubLookupEnv(fn func(string) (string, bool)) func() { return func() { lookupEnvFunc = orig } } -// newService is a test helper that constructs the internal service struct -// directly, bypassing the config/env parsing of NewService but still -// resolving common dimensions like device_id and invocation_id. -func newService(flusher func(SendTelemetryPayload), additionalDimensions ghtelemetry.Dimensions) *service { - deviceID, err := deviceIDFunc() - if err != nil { - deviceID = "" - } - - commonDimensions := ghtelemetry.Dimensions{ - "device_id": deviceID, - "invocation_id": uuid.NewString(), - } - maps.Copy(commonDimensions, additionalDimensions) - - return &service{ - flush: flusher, - commonDimensions: commonDimensions, - } -} - func TestGetOrCreateDeviceID(t *testing.T) { t.Run("creates new ID on first call", func(t *testing.T) { tmpDir := t.TempDir() @@ -318,17 +294,19 @@ func TestParseTelemetryState(t *testing.T) { } func TestNewServiceLogModeFlushesToWriter(t *testing.T) { + // Given an invocation with log delivery t.Cleanup(stubDeviceID("test-device")) - var buf bytes.Buffer - svc := NewService(LogFlusher(&buf, false)) + service := NewService(LogFlusher(&buf, false)) - svc.Record(ghtelemetry.Event{ + // When the invocation finishes + service.Record(ghtelemetry.Event{ Type: "test_event", Dimensions: map[string]string{"key": "value"}, }) - svc.Flush() + service.Finish() + // Then the writer receives the recorded event output := buf.String() assert.Contains(t, output, "Telemetry payload:") assert.Contains(t, output, "test_event") @@ -337,17 +315,18 @@ func TestNewServiceLogModeFlushesToWriter(t *testing.T) { } func TestNewServiceLogModeWithColorLogsToWriter(t *testing.T) { + // Given an invocation with colored log delivery t.Cleanup(stubDeviceID("test-device")) - var buf bytes.Buffer - svc := NewService(LogFlusher(&buf, true)) + service := NewService(LogFlusher(&buf, true)) - svc.Record(ghtelemetry.Event{Type: "color_event"}) - svc.Flush() + // When the invocation finishes + service.Record(ghtelemetry.Event{Type: "color_event"}) + service.Finish() + // Then the writer receives the event with ANSI color codes output := buf.String() assert.Contains(t, output, "color_event") - // Verify ANSI color codes are present in the output assert.Contains(t, output, "\033[", "expected ANSI escape sequences when color is enabled") } @@ -369,340 +348,188 @@ func TestLogFlusherWritesNoneMarkerForEmptyPayload(t *testing.T) { } func TestServiceDeviceIDFallback(t *testing.T) { + // Given device ID discovery fails t.Cleanup(stubDeviceIDError(errors.New("no device id"))) - var captured SendTelemetryPayload - svc := newService(func(p SendTelemetryPayload) { captured = p }, nil) + service := NewService(func(p SendTelemetryPayload) { captured = p }) - svc.Record(ghtelemetry.Event{Type: "test"}) - svc.Flush() + // When a recorded event is completed and delivered + service.Record(ghtelemetry.Event{Type: "test"}) + service.Finish() + // Then the payload identifies the device as unknown require.Len(t, captured.Events, 1) assert.Equal(t, "", captured.Events[0].Dimensions["device_id"]) } -func TestServiceFlush(t *testing.T) { - t.Run("calls flusher with empty payload when no events recorded", func(t *testing.T) { +func TestServiceFinish(t *testing.T) { + t.Run("logs none when no events recorded", func(t *testing.T) { + // Given an invocation without events and log delivery t.Cleanup(stubDeviceID("test-device")) + var buf bytes.Buffer + service := NewService(LogFlusher(&buf, false)) - var captured SendTelemetryPayload - called := false - svc := newService(func(p SendTelemetryPayload) { - called = true - captured = p - }, nil) - svc.Flush() - - assert.True(t, called, "flusher should be called even with no events so log mode can surface the absence") - assert.Empty(t, captured.Events, "payload should have no events") + // When the invocation finishes + service.Finish() + + // Then log mode explains the absence of telemetry + assert.Equal(t, "Telemetry payload: none\n", buf.String()) }) - t.Run("flushes events with merged dimensions", func(t *testing.T) { + t.Run("delivers events with merged dimensions", func(t *testing.T) { + // Given an invocation with common dimensions t.Cleanup(stubDeviceID("test-device")) - var captured SendTelemetryPayload - svc := newService(func(p SendTelemetryPayload) { captured = p }, ghtelemetry.Dimensions{"version": "2.45.0"}) - - svc.Record(ghtelemetry.Event{ + service := NewService(func(p SendTelemetryPayload) { captured = p }, + WithAdditionalCommonDimensions(ghtelemetry.Dimensions{ + "version": "2.45.0", + "agent": "none", + }), + ) + + // When an event with its own dimensions and measures is completed and delivered + service.Record(ghtelemetry.Event{ Type: "command_invocation", Dimensions: map[string]string{"command": "gh pr list"}, Measures: map[string]int64{"duration_ms": 150}, }) - svc.Flush() + service.Finish() + // Then the payload includes both common and event-specific facts require.Len(t, captured.Events, 1) event := captured.Events[0] assert.Equal(t, "command_invocation", event.Type) assert.Equal(t, "gh pr list", event.Dimensions["command"]) assert.Equal(t, "2.45.0", event.Dimensions["version"]) + assert.Equal(t, "none", event.Dimensions["agent"]) assert.Equal(t, "test-device", event.Dimensions["device_id"]) assert.NotEmpty(t, event.Dimensions["timestamp"]) assert.NotEmpty(t, event.Dimensions["invocation_id"]) + assert.NotEmpty(t, event.Dimensions["os"]) + assert.NotEmpty(t, event.Dimensions["architecture"]) assert.Equal(t, int64(150), event.Measures["duration_ms"]) }) - t.Run("flushes multiple events", func(t *testing.T) { - t.Cleanup(stubDeviceID("test-device")) - - var captured SendTelemetryPayload - svc := newService(func(p SendTelemetryPayload) { captured = p }, nil) - - svc.Record(ghtelemetry.Event{Type: "event1"}) - svc.Record(ghtelemetry.Event{Type: "event2"}) - svc.Flush() - - require.Len(t, captured.Events, 2) - assert.Equal(t, "event1", captured.Events[0].Type) - assert.Equal(t, "event2", captured.Events[1].Type) - }) - - t.Run("is idempotent", func(t *testing.T) { - t.Cleanup(stubDeviceID("test-device")) - - callCount := 0 - svc := newService(func(SendTelemetryPayload) { callCount++ }, nil) - svc.Record(ghtelemetry.Event{Type: "test"}) - - svc.Flush() - svc.Flush() - svc.Flush() - - assert.Equal(t, 1, callCount, "flusher should only be called once") - }) - t.Run("event dimensions override common dimensions", func(t *testing.T) { + // Given common and event dimensions share a key t.Cleanup(stubDeviceID("test-device")) - var captured SendTelemetryPayload - svc := newService(func(p SendTelemetryPayload) { captured = p }, ghtelemetry.Dimensions{"shared": "common"}) - - svc.Record(ghtelemetry.Event{ + service := NewService(func(p SendTelemetryPayload) { captured = p }, WithAdditionalCommonDimensions(ghtelemetry.Dimensions{"shared": "common"})) + service.Record(ghtelemetry.Event{ Type: "test", Dimensions: map[string]string{"shared": "event-level"}, }) - svc.Flush() + // When the invocation finishes + service.Finish() + + // Then the event dimension takes precedence require.Len(t, captured.Events, 1) - // Event dimensions are copied last via maps.Copy, so they override common assert.Equal(t, "event-level", captured.Events[0].Dimensions["shared"]) }) - - t.Run("timestamps reflect record time not flush time", func(t *testing.T) { - t.Cleanup(stubDeviceID("test-device")) - - var captured SendTelemetryPayload - svc := newService(func(p SendTelemetryPayload) { captured = p }, nil) - - svc.Record(ghtelemetry.Event{Type: "early"}) - time.Sleep(50 * time.Millisecond) - svc.Record(ghtelemetry.Event{Type: "late"}) - svc.Flush() - - require.Len(t, captured.Events, 2) - ts1 := captured.Events[0].Dimensions["timestamp"] - ts2 := captured.Events[1].Dimensions["timestamp"] - require.NotEmpty(t, ts1) - require.NotEmpty(t, ts2) - - t1, err := time.Parse("2006-01-02T15:04:05.000Z", ts1) - require.NoError(t, err) - t2, err := time.Parse("2006-01-02T15:04:05.000Z", ts2) - require.NoError(t, err) - - assert.True(t, t2.After(t1), "second event timestamp %s should be after first %s", ts2, ts1) - }) } func TestServiceSampling(t *testing.T) { - t.Run("sampleRate 0 sends all events", func(t *testing.T) { - t.Cleanup(stubDeviceID("test-device")) - - var captured SendTelemetryPayload - svc := newService(func(p SendTelemetryPayload) { captured = p }, nil) - svc.sampleRate = 0 - svc.sampleBucket = 99 - - svc.Record(ghtelemetry.Event{Type: "test"}) - svc.Flush() - - require.Len(t, captured.Events, 1) - }) - - t.Run("sampleRate 100 sends all events regardless of bucket", func(t *testing.T) { - t.Cleanup(stubDeviceID("test-device")) - - var captured SendTelemetryPayload - svc := newService(func(p SendTelemetryPayload) { captured = p }, nil) - svc.sampleRate = 100 - svc.sampleBucket = 99 - - svc.Record(ghtelemetry.Event{Type: "test"}) - svc.Flush() - - require.Len(t, captured.Events, 1) - }) - - t.Run("bucket below sampleRate sends events", func(t *testing.T) { - t.Cleanup(stubDeviceID("test-device")) - - var captured SendTelemetryPayload - svc := newService(func(p SendTelemetryPayload) { captured = p }, nil) - svc.sampleRate = 50 - svc.sampleBucket = 49 // below rate, should be included - - svc.Record(ghtelemetry.Event{Type: "test"}) - svc.Flush() - - require.Len(t, captured.Events, 1) - }) - - t.Run("bucket at sampleRate drops events", func(t *testing.T) { - t.Cleanup(stubDeviceID("test-device")) - - called := false - svc := newService(func(SendTelemetryPayload) { called = true }, nil) - svc.sampleRate = 50 - svc.sampleBucket = 50 // at rate boundary, should be excluded - - svc.Record(ghtelemetry.Event{Type: "test"}) - svc.Flush() - - assert.False(t, called, "flusher should not be called when bucket >= sampleRate") - }) - - t.Run("bucket above sampleRate drops events", func(t *testing.T) { - t.Cleanup(stubDeviceID("test-device")) - - called := false - svc := newService(func(SendTelemetryPayload) { called = true }, nil) - svc.sampleRate = 1 - svc.sampleBucket = 50 - - svc.Record(ghtelemetry.Event{Type: "test"}) - svc.Flush() - - assert.False(t, called, "flusher should not be called when bucket >= sampleRate") - }) - - t.Run("SetSampleRate changes flush behavior", func(t *testing.T) { - t.Cleanup(stubDeviceID("test-device")) - - called := false - svc := newService(func(SendTelemetryPayload) { called = true }, nil) - svc.sampleBucket = 50 - - // Initially rate=0, which sends everything - svc.SetSampleRate(10) // Now bucket=50 >= rate=10, should drop - svc.Record(ghtelemetry.Event{Type: "test"}) - svc.Flush() - - assert.False(t, called, "flusher should not be called after SetSampleRate reduced the rate") - }) - - t.Run("SetSampleRate updates sample_rate dimension", func(t *testing.T) { - t.Cleanup(stubDeviceID("test-device")) - - var captured SendTelemetryPayload - svc := newService(func(p SendTelemetryPayload) { captured = p }, ghtelemetry.Dimensions{ - "sample_rate": "1", + tests := []struct { + name string + sampleRate int + sampleBucket byte + wantPayloads int + }{ + { + name: "sampleRate 0 sends all events", + sampleRate: 0, + sampleBucket: 99, + wantPayloads: 1, + }, + { + name: "sampleRate 100 sends all events regardless of bucket", + sampleRate: 100, + sampleBucket: 99, + wantPayloads: 1, + }, + { + name: "bucket below sampleRate sends events", + sampleRate: 50, + sampleBucket: 49, + wantPayloads: 1, + }, + { + name: "bucket at sampleRate drops events", + sampleRate: 50, + sampleBucket: 50, + wantPayloads: 0, + }, + { + name: "bucket above sampleRate drops events", + sampleRate: 1, + sampleBucket: 50, + wantPayloads: 0, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Given a configured sample rate and a deterministic sampling bucket + t.Cleanup(stubDeviceID("test-device")) + var payloads []SendTelemetryPayload + svc := NewService(func(p SendTelemetryPayload) { payloads = append(payloads, p) }, WithSampleRate(tt.sampleRate)) + // Fix the random bucket so sampling boundaries can be asserted through delivery. + svc.(*service).sampleBucket = tt.sampleBucket + svc.Record(ghtelemetry.Event{Type: "test"}) + + // When the invocation finishes + svc.Finish() + + // Then the sampling policy determines whether a payload is delivered + assert.Len(t, payloads, tt.wantPayloads) }) - svc.sampleRate = 1 - svc.sampleBucket = 0 - - svc.SetSampleRate(100) - svc.Record(ghtelemetry.Event{Type: "test"}) - svc.Flush() - - require.Len(t, captured.Events, 1) - assert.Equal(t, "100", captured.Events[0].Dimensions["sample_rate"]) - }) - - t.Run("WithSampleRate option sets rate on construction", func(t *testing.T) { - t.Cleanup(stubDeviceID("test-device")) - - called := false - svc := NewService(func(SendTelemetryPayload) { called = true }, WithSampleRate(1)) - - svc.Record(ghtelemetry.Event{Type: "test"}) - svc.Flush() - - // We can't control the bucket from NewService, so we just verify - // the service was created without error and Flush doesn't panic. - // The actual sampling behavior is tested via direct struct manipulation above. - _ = called - }) + } } -func TestWithAdditionalCommonDimensions(t *testing.T) { +func TestServiceReducingSampleRateExcludesInvocation(t *testing.T) { + // Given an invocation that initially sends all events t.Cleanup(stubDeviceID("test-device")) + var payloads []SendTelemetryPayload + svc := NewService(func(p SendTelemetryPayload) { payloads = append(payloads, p) }, WithSampleRate(0)) + svc.(*service).sampleBucket = 50 - var captured SendTelemetryPayload - svc := NewService( - func(p SendTelemetryPayload) { captured = p }, - WithAdditionalCommonDimensions(ghtelemetry.Dimensions{ - "version": "2.45.0", - "agent": "none", - }), - ) - + // When its sample rate excludes the bucket before completion + svc.SetSampleRate(10) svc.Record(ghtelemetry.Event{Type: "test"}) - svc.Flush() + svc.Finish() - require.Len(t, captured.Events, 1) - assert.Equal(t, "2.45.0", captured.Events[0].Dimensions["version"]) - assert.Equal(t, "none", captured.Events[0].Dimensions["agent"]) - // Standard common dimensions should also be present - assert.Equal(t, "test-device", captured.Events[0].Dimensions["device_id"]) - assert.NotEmpty(t, captured.Events[0].Dimensions["invocation_id"]) - assert.NotEmpty(t, captured.Events[0].Dimensions["os"]) - assert.NotEmpty(t, captured.Events[0].Dimensions["architecture"]) + // Then no payload is delivered + assert.Empty(t, payloads) } -func TestServiceDisable(t *testing.T) { - t.Run("drops recorded events from flushed payload", func(t *testing.T) { - t.Cleanup(stubDeviceID("test-device")) - - var captured SendTelemetryPayload - called := false - svc := newService(func(p SendTelemetryPayload) { - called = true - captured = p - }, nil) - - svc.Record(ghtelemetry.Event{Type: "test"}) - svc.Disable() - svc.Flush() - - assert.True(t, called, "flusher should still be called so log mode can surface the absence of events") - assert.Empty(t, captured.Events, "recorded events should be dropped after Disable()") - }) - - t.Run("drops events even with multiple recorded events", func(t *testing.T) { - t.Cleanup(stubDeviceID("test-device")) - - var captured SendTelemetryPayload - called := false - svc := newService(func(p SendTelemetryPayload) { - called = true - captured = p - }, nil) - - svc.Record(ghtelemetry.Event{Type: "event1"}) - svc.Record(ghtelemetry.Event{Type: "event2"}) - svc.Record(ghtelemetry.Event{Type: "event3"}) - svc.Disable() - svc.Flush() - - assert.True(t, called, "flusher should still be called") - assert.Empty(t, captured.Events, "recorded events should be dropped after Disable()") - }) - - t.Run("can be called before any events are recorded", func(t *testing.T) { - t.Cleanup(stubDeviceID("test-device")) - - var captured SendTelemetryPayload - called := false - svc := newService(func(p SendTelemetryPayload) { - called = true - captured = p - }, nil) - - svc.Disable() - svc.Record(ghtelemetry.Event{Type: "test"}) - svc.Flush() - - assert.True(t, called, "flusher should still be called") - assert.Empty(t, captured.Events, "events recorded after Disable() should be dropped") - }) +func TestServiceDisabledBeforeRecordingDropsLaterEvents(t *testing.T) { + // Given an invocation disabled before any events are recorded + t.Cleanup(stubDeviceID("test-device")) + var payloads []SendTelemetryPayload + service := NewService(func(p SendTelemetryPayload) { payloads = append(payloads, p) }) + service.Disable() + + // When immediate and pending events are recorded and the invocation completes + service.Record(ghtelemetry.Event{Type: "completed_step"}) + pending := service.Begin(ghtelemetry.Event{Type: "attachment_invocation"}) + pending.UpsertMeasures(ghtelemetry.Measures{"attach_count": 2}) + service.Finish() + + // Then the later events are excluded from the delivered payload + require.Len(t, payloads, 1) + assert.Empty(t, payloads[0].Events, "events recorded after Disable() should be dropped") } func TestNoOpService(t *testing.T) { - svc := &NoOpService{} + service := &NoOpService{} // All methods should be safe to call without panicking - svc.Record(ghtelemetry.Event{Type: "test"}) - svc.Disable() - svc.SetSampleRate(50) - svc.Flush() + service.Record(ghtelemetry.Event{Type: "test"}) + event := service.Begin(ghtelemetry.Event{Type: "pending"}) + event.UpsertDimensions(ghtelemetry.Dimensions{"key": "value"}) + event.UpsertMeasures(ghtelemetry.Measures{"count": 1}) + service.Disable() + service.SetSampleRate(50) + service.Finish() } func TestSpawnSendTelemetryRejectsOversizedPayload(t *testing.T) { diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index e67ca517a76..9075577ab66 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -449,7 +449,7 @@ func TestNewCmdApiTelemetry(t *testing.T) { _, err := cmd.ExecuteC() require.NoError(t, err) - recorder.Flush() + recorder.Finish() assert.Empty(t, payload.Events) } diff --git a/pkg/cmd/copilot/copilot.go b/pkg/cmd/copilot/copilot.go index a0a0ce5348b..e3c610de3c8 100644 --- a/pkg/cmd/copilot/copilot.go +++ b/pkg/cmd/copilot/copilot.go @@ -39,7 +39,7 @@ type CopilotOptions struct { Remove bool } -func NewCmdCopilot(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF func(*CopilotOptions) error) *cobra.Command { +func NewCmdCopilot(f *cmdutil.Factory, telemetry ghtelemetry.InvocationRecorder, runF func(*CopilotOptions) error) *cobra.Command { opts := &CopilotOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, diff --git a/pkg/cmd/copilot/copilot_test.go b/pkg/cmd/copilot/copilot_test.go index 18e3efe78d2..570ab6c45de 100644 --- a/pkg/cmd/copilot/copilot_test.go +++ b/pkg/cmd/copilot/copilot_test.go @@ -115,7 +115,7 @@ func TestNewCmdCopilot(t *testing.T) { assert.NoError(t, err) var gotOpts *CopilotOptions - spy := &telemetry.CommandRecorderSpy{} + spy := &telemetry.InvocationRecorderSpy{} cmd := NewCmdCopilot(f, spy, func(opts *CopilotOptions) error { gotOpts = opts return nil diff --git a/pkg/cmd/extension/manager.go b/pkg/cmd/extension/manager.go index 112efad876b..ca802bf2a71 100644 --- a/pkg/cmd/extension/manager.go +++ b/pkg/cmd/extension/manager.go @@ -53,10 +53,11 @@ type Manager struct { gitClient gitClient config gh.Config io *iostreams.IOStreams + ghPath string dryRunMode bool } -func NewManager(ios *iostreams.IOStreams, gc *git.Client) *Manager { +func NewManager(ios *iostreams.IOStreams, gc *git.Client, ghPath string) *Manager { return &Manager{ dataDir: config.DataDir, updateDir: func() string { @@ -74,6 +75,7 @@ func NewManager(ios *iostreams.IOStreams, gc *git.Client) *Manager { }, io: ios, gitClient: &gitExecuter{client: gc}, + ghPath: ghPath, } } @@ -128,9 +130,8 @@ func (m *Manager) Dispatch(args []string, stdin io.Reader, stdout, stderr io.Wri forwardArgs = append([]string{"-c", `command "$@"`, "--", exe}, forwardArgs...) externalCmd = m.newCommand(shExe, forwardArgs...) } - // Signal to the extension that it is being run by gh rather than standalone, so it can - // adjust things like usage strings. - externalCmd.Env = append(externalCmd.Environ(), "GH_EXTENSION=1") + // Tell the extension that gh dispatched it and provide a reliable path back to this executable. + externalCmd.Env = append(externalCmd.Environ(), "GH_EXTENSION=1", "GH_PATH="+m.ghPath) externalCmd.Stdin = stdin externalCmd.Stdout = stdout diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 202f7ae556c..0b79b4b6fc8 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -271,7 +271,7 @@ func branchFunc(f *cmdutil.Factory) func() (string, error) { } func extensionManager(f *cmdutil.Factory) *extension.Manager { - em := extension.NewManager(f.IOStreams, f.GitClient) + em := extension.NewManager(f.IOStreams, f.GitClient, f.ExecutablePath) cfg, err := f.Config() if err != nil { diff --git a/pkg/cmd/issue/comment/comment.go b/pkg/cmd/issue/comment/comment.go index 3c972d14f20..54a97db0da8 100644 --- a/pkg/cmd/issue/comment/comment.go +++ b/pkg/cmd/issue/comment/comment.go @@ -12,7 +12,7 @@ import ( "github.com/spf13/cobra" ) -func NewCmdComment(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF func(*prShared.CommentableOptions) error) *cobra.Command { +func NewCmdComment(f *cmdutil.Factory, telemetry ghtelemetry.InvocationRecorder, runF func(*prShared.CommentableOptions) error) *cobra.Command { opts := &prShared.CommentableOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, @@ -59,8 +59,6 @@ func NewCmdComment(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, ru `), Args: cobra.ExactArgs(1), PreRunE: func(cmd *cobra.Command, args []string) error { - opts.AttachFlag.RecordTelemetry(cmd.CommandPath(), telemetry) - opts.RetrieveCommentable = func() (prShared.Commentable, ghrepo.Interface, error) { // TODO wm: more testing issueNumber, parsedBaseRepo, err := shared.ParseIssueFromArg(args[0]) @@ -101,7 +99,7 @@ func NewCmdComment(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, ru return issue, baseRepo, nil } - if err := prShared.CommentablePreRun(cmd, opts); err != nil { + if err := prShared.CommentablePreRun(cmd, opts, telemetry); err != nil { return err } diff --git a/pkg/cmd/issue/comment/comment_test.go b/pkg/cmd/issue/comment/comment_test.go index 08ad22e7757..8db65cdf122 100644 --- a/pkg/cmd/issue/comment/comment_test.go +++ b/pkg/cmd/issue/comment/comment_test.go @@ -22,6 +22,7 @@ import ( "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" + "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -34,6 +35,14 @@ func TestNewCmdComment(t *testing.T) { tmpImage := filepath.Join(t.TempDir(), "login.png") require.NoError(t, os.WriteFile(tmpImage, []byte("the bytes"), 0600)) + attachmentEvent := []ghtelemetry.Event{{ + Type: "attachment_invocation", + Dimensions: ghtelemetry.Dimensions{"command": "comment"}, + Measures: ghtelemetry.Measures{ + "attach_count": 1, "append_ops_count": 0, "replace_ops_count": 0, + }, + }} + tests := []struct { name string input string @@ -42,6 +51,8 @@ func TestNewCmdComment(t *testing.T) { wantsErr bool wantsErrContains string isTTY bool + wantEvents []ghtelemetry.Event + wantSampleRate int // Which fields the lookup asks for is decided at construction, so it // can only be proved here. @@ -265,8 +276,10 @@ func TestNewCmdComment(t *testing.T) { wantsErr: true, }, { - name: "--attach alone is enough of a body to post without prompting", - input: fmt.Sprintf("1 --attach '%s'", tmpImage), + name: "--attach alone is enough of a body to post without prompting", + input: fmt.Sprintf("1 --attach '%s'", tmpImage), + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, output: shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -276,8 +289,16 @@ func TestNewCmdComment(t *testing.T) { wantsErr: false, }, { - name: "--attach with --body keeps the body and records that one was given", - input: fmt.Sprintf("1 --body test --attach '%s'", tmpImage), + name: "argument validation skips attachment telemetry", + input: fmt.Sprintf("--attach '%s'", tmpImage), + isTTY: false, + wantsErr: true, + }, + { + name: "--attach with --body keeps the body and records that one was given", + input: fmt.Sprintf("1 --body test --attach '%s'", tmpImage), + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, output: shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -288,8 +309,10 @@ func TestNewCmdComment(t *testing.T) { wantsErr: false, }, { - name: "--attach with --edit-last is accepted and needs no body", - input: fmt.Sprintf("1 --edit-last --attach '%s'", tmpImage), + name: "--attach with --edit-last is accepted and needs no body", + input: fmt.Sprintf("1 --edit-last --attach '%s'", tmpImage), + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, output: shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -301,8 +324,10 @@ func TestNewCmdComment(t *testing.T) { { // KeepExistingBody is set either way. BodyProvided is what decides // that this one replaces the comment rather than keeping it. - name: "--attach with --edit-last and --body records the body that replaces the comment", - input: fmt.Sprintf("1 --edit-last --body 'a new body' --attach '%s'", tmpImage), + name: "--attach with --edit-last and --body records the body that replaces the comment", + input: fmt.Sprintf("1 --edit-last --body 'a new body' --attach '%s'", tmpImage), + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, output: shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -333,10 +358,14 @@ func TestNewCmdComment(t *testing.T) { isTTY: true, wantsErr: true, wantsErrContains: "./nope.png: ", + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, }, { - name: "--attach asks the issue lookup for the repository id", - input: fmt.Sprintf("1 --attach '%s'", tmpImage), + name: "--attach asks the issue lookup for the repository id", + input: fmt.Sprintf("1 --attach '%s'", tmpImage), + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, output: shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -361,6 +390,7 @@ func TestNewCmdComment(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // Given command inputs and an invocation recorder ios, stdin, _, _ := iostreams.Test() isTTY := tt.isTTY ios.SetStdoutTTY(isTTY) @@ -407,10 +437,10 @@ func TestNewCmdComment(t *testing.T) { } argv, err := shlex.Split(tt.input) - assert.NoError(t, err) + require.NoError(t, err) var gotOpts *shared.CommentableOptions - recorder := &telemetry.CommandRecorderSpy{} + recorder := &telemetry.InvocationRecorderSpy{} cmd := NewCmdComment(f, recorder, func(opts *shared.CommentableOptions) error { gotOpts = opts return nil @@ -422,28 +452,20 @@ func TestNewCmdComment(t *testing.T) { cmd.SetOut(&bytes.Buffer{}) cmd.SetErr(&bytes.Buffer{}) + // When the command executes _, err = cmd.ExecuteC() - if cmd.Flags().Changed("attach") { - values, flagErr := cmd.Flags().GetStringArray("attach") - require.NoError(t, flagErr) - require.Equal(t, ghtelemetry.SAMPLE_ALL, recorder.LastSampleRate) - require.Len(t, recorder.Events, 1) - assert.Equal(t, "attachment_invocation", recorder.Events[0].Type) - assert.Equal(t, cmd.CommandPath(), recorder.Events[0].Dimensions["command"]) - assert.Equal(t, int64(len(values)), recorder.Events[0].Measures["attach_count"]) - } else { - assert.Empty(t, recorder.Events) - assert.Zero(t, recorder.LastSampleRate) - } + // Then telemetry starts only if execution reaches attachment validation + assert.Equal(t, tt.wantEvents, recorder.Events()) + assert.Equal(t, tt.wantSampleRate, recorder.LastSampleRate) if tt.wantsErr { - assert.Error(t, err) + require.Error(t, err) if tt.wantsErrContains != "" { - assert.ErrorContains(t, err, tt.wantsErrContains) + require.ErrorContains(t, err, tt.wantsErrContains) } return } - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, tt.output.Interactive, gotOpts.Interactive) assert.Equal(t, tt.output.InputType, gotOpts.InputType) assert.Equal(t, tt.output.Body, gotOpts.Body) @@ -469,6 +491,82 @@ func TestNewCmdComment(t *testing.T) { } } +func TestNewCmdCommentRecordsRejectedAttachmentCount(t *testing.T) { + // Given 51 attachment values and a normally sampled telemetry service + // NewService reads or creates a device-id file, so isolate it from the user's state. + // An injectable device-ID lookup could avoid this process-wide override and allow parallel tests. + t.Setenv("XDG_STATE_HOME", t.TempDir()) + ios, _, _, _ := iostreams.Test() + f := &cmdutil.Factory{ + IOStreams: ios, + Browser: &browser.Stub{}, + } + var payload telemetry.SendTelemetryPayload + service := telemetry.NewService(func(p telemetry.SendTelemetryPayload) { + payload = p + }, telemetry.WithSampleRate(1)) + cmd := NewCmdComment(f, service, func(*shared.CommentableOptions) error { + return errors.New("run should not be called") + }) + args := []string{"1"} + for i := range 51 { + args = append(args, "--attach", fmt.Sprintf("./shot-%d.png", i)) + } + cmd.SetArgs(args) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + // When attachment validation rejects the limit and telemetry is completed + err := cmd.Execute() + service.Finish() + + // Then the payload preserves the rejected count without any completed operations + require.EqualError(t, err, "`--attach` accepts at most 50 values per command") + require.Len(t, payload.Events, 1) + event := payload.Events[0] + assert.Equal(t, "attachment_invocation", event.Type) + assert.Equal(t, "comment", event.Dimensions["command"]) + assert.Equal(t, "100", event.Dimensions["sample_rate"]) + assert.Equal(t, map[string]int64{ + "attach_count": 51, + "append_ops_count": 0, + "replace_ops_count": 0, + }, event.Measures) +} + +func TestNewCmdCommentSkipsAttachmentsOnPersistentPreRunError(t *testing.T) { + t.Parallel() + + // Given an attachment command whose parent rejects execution before PreRunE + ios, _, _, _ := iostreams.Test() + f := &cmdutil.Factory{ + IOStreams: ios, + Browser: &browser.Stub{}, + } + recorder := &telemetry.InvocationRecorderSpy{} + cmd := NewCmdComment(f, recorder, func(*shared.CommentableOptions) error { + return errors.New("run should not be called") + }) + root := &cobra.Command{ + Use: "gh", + SilenceErrors: true, + SilenceUsage: true, + PersistentPreRunE: func(*cobra.Command, []string) error { + return errors.New("authentication failed") + }, + } + root.AddCommand(cmd) + root.SetArgs([]string{"comment", "1", "--attach", "./shot.png"}) + + // When authentication fails + _, err := root.ExecuteC() + + // Then attachment validation has not begun, so no event or sampling promotion occurs + require.EqualError(t, err, "authentication failed") + assert.Empty(t, recorder.Events()) + assert.Zero(t, recorder.LastSampleRate) +} + func Test_commentRun(t *testing.T) { tests := []struct { name string diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index c0e6adfa481..d4d9a806221 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -58,11 +58,12 @@ type CreateOptions struct { BlockedBy []string Blocking []string - AttachFlag *attachments.Flag - Assets []attachments.UserAsset + AttachFlag *attachments.Flag + AttachEvent *attachments.TelemetryEvent + Assets []attachments.UserAsset } -func NewCmdCreate(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF func(*CreateOptions) error) *cobra.Command { +func NewCmdCreate(f *cmdutil.Factory, telemetry ghtelemetry.InvocationRecorder, runF func(*CreateOptions) error) *cobra.Command { opts := &CreateOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, @@ -121,8 +122,6 @@ func NewCmdCreate(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, run Args: cmdutil.NoArgsQuoteReminder, Aliases: []string{"new"}, RunE: func(cmd *cobra.Command, args []string) error { - opts.AttachFlag.RecordTelemetry(cmd.CommandPath(), telemetry) - // support `-R, --repo` override opts.BaseRepo = f.BaseRepo opts.HasRepoOverride = cmd.Flags().Changed("repo") @@ -166,6 +165,7 @@ func NewCmdCreate(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, run return err } + opts.AttachEvent = attachments.BeginTelemetry(telemetry, cmd.CommandPath(), opts.AttachFlag.Count()) opts.Assets, err = opts.AttachFlag.UserAssets() if err != nil { return err @@ -466,8 +466,9 @@ func createRun(opts *CreateOptions) (err error) { // not exist, so no issue is created. Once anything has uploaded the // issue is created and the failures are reported. if uploader != nil { - body, uploaded, uploadErr := uploader.UploadAndAttach(context.Background(), tb.Body, opts.Assets) - if uploadErr != nil && uploaded == 0 { + body, uploadResult, uploadErr := uploader.UploadAndAttach(context.Background(), tb.Body, opts.Assets) + opts.AttachEvent.RecordOperations(uploadResult) + if uploadErr != nil && uploadResult.Uploaded == 0 { err = uploadErr return } diff --git a/pkg/cmd/issue/create/create_test.go b/pkg/cmd/issue/create/create_test.go index e0463fb9504..813e9e6a8ce 100644 --- a/pkg/cmd/issue/create/create_test.go +++ b/pkg/cmd/issue/create/create_test.go @@ -41,6 +41,14 @@ func TestNewCmdCreate(t *testing.T) { tmpImage := filepath.Join(t.TempDir(), "shot.png") require.NoError(t, os.WriteFile(tmpImage, []byte("the bytes"), 0600)) + attachmentEvent := []ghtelemetry.Event{{ + Type: "attachment_invocation", + Dimensions: ghtelemetry.Dimensions{"command": "create"}, + Measures: ghtelemetry.Measures{ + "attach_count": 1, "append_ops_count": 0, "replace_ops_count": 0, + }, + }} + tests := []struct { name string tty bool @@ -54,6 +62,8 @@ func TestNewCmdCreate(t *testing.T) { wantErrIsNotExist bool wantsOpts CreateOptions wantAssetPaths []string + wantEvents []ghtelemetry.Event + wantSampleRate int }{ { name: "empty non-tty", @@ -275,6 +285,14 @@ func TestNewCmdCreate(t *testing.T) { Body: "mybody", }, wantAssetPaths: []string{tmpImage}, + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, + }, + { + name: "argument validation skips attachment telemetry", + tty: false, + cli: fmt.Sprintf(`unexpected --attach '%s'`, tmpImage), + wantsErr: true, }, { name: "attach conflict is reported before a missing file", @@ -290,10 +308,13 @@ func TestNewCmdCreate(t *testing.T) { wantsErr: true, wantsErrMsg: "./nope.png: ", wantErrIsNotExist: true, + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // Given command inputs and an invocation recorder ios, stdin, stdout, stderr := iostreams.Test() if tt.stdin != "" { _, _ = stdin.WriteString(tt.stdin) @@ -313,7 +334,7 @@ func TestNewCmdCreate(t *testing.T) { } var opts *CreateOptions - recorder := &telemetry.CommandRecorderSpy{} + recorder := &telemetry.InvocationRecorderSpy{} cmd := NewCmdCreate(f, recorder, func(o *CreateOptions) error { opts = o return nil @@ -324,19 +345,11 @@ func TestNewCmdCreate(t *testing.T) { cmd.SetArgs(args) cmd.SetOut(io.Discard) cmd.SetErr(io.Discard) + // When the command executes _, err = cmd.ExecuteC() - if cmd.Flags().Changed("attach") { - values, flagErr := cmd.Flags().GetStringArray("attach") - require.NoError(t, flagErr) - require.Equal(t, ghtelemetry.SAMPLE_ALL, recorder.LastSampleRate) - require.Len(t, recorder.Events, 1) - assert.Equal(t, "attachment_invocation", recorder.Events[0].Type) - assert.Equal(t, cmd.CommandPath(), recorder.Events[0].Dimensions["command"]) - assert.Equal(t, int64(len(values)), recorder.Events[0].Measures["attach_count"]) - } else { - assert.Empty(t, recorder.Events) - assert.Zero(t, recorder.LastSampleRate) - } + // Then telemetry starts only if execution reaches attachment validation + assert.Equal(t, tt.wantEvents, recorder.Events()) + assert.Equal(t, tt.wantSampleRate, recorder.LastSampleRate) if tt.wantsErr { require.Error(t, err) if tt.wantsErrMsg != "" { @@ -376,17 +389,18 @@ func TestNewCmdCreate(t *testing.T) { func Test_createRun(t *testing.T) { tests := []struct { - name string - opts CreateOptions - attach []string - host string - config string - httpStubs func(*testing.T, *httpmock.Registry) - promptStubs func(*testing.T, *prompter.PrompterMock) - wantsStdout string - wantsStderr string - wantsBrowse string - wantsErr string + name string + opts CreateOptions + attach []string + host string + config string + httpStubs func(*testing.T, *httpmock.Registry) + promptStubs func(*testing.T, *prompter.PrompterMock) + wantsStdout string + wantsStderr string + wantsBrowse string + wantsErr string + wantOperations *attachments.UploadResult }{ { name: "no args", @@ -593,9 +607,10 @@ func Test_createRun(t *testing.T) { return "title", "from editor ![shot](./shot.png)", nil }, }, - attach: []string{"shot.png"}, - wantsStdout: "https://github.com/OWNER/REPO/issues/12\n", - wantsStderr: "\nCreating issue in OWNER/REPO\n\n", + attach: []string{"shot.png"}, + wantsStdout: "https://github.com/OWNER/REPO/issues/12\n", + wantsStderr: "\nCreating issue in OWNER/REPO\n\n", + wantOperations: &attachments.UploadResult{ReplaceOperations: 1}, }, { name: "editor and template", @@ -1185,7 +1200,8 @@ func Test_createRun(t *testing.T) { assert.Equal(t, "a body\n\n![first](https://github.com/user-attachments/assets/AAA)", inputs["body"]) })) }, - wantsErr: "could not upload ./second.png: attaching files requires write access to the repository", + wantsErr: "could not upload ./second.png: attaching files requires write access to the repository", + wantOperations: &attachments.UploadResult{AppendOperations: 1}, }, { name: "a create that fails after an upload failed reports both", @@ -1211,7 +1227,8 @@ func Test_createRun(t *testing.T) { httpmock.GraphQL(`mutation IssueCreate\b`), httpmock.StringResponse(`{ "errors": [{ "message": "the create failed" }] }`)) }, - wantsErr: "could not upload ./second.png: attaching files requires write access to the repository\nGraphQL: the create failed", + wantsErr: "could not upload ./second.png: attaching files requires write access to the repository\nGraphQL: the create failed", + wantOperations: &attachments.UploadResult{AppendOperations: 1}, }, { name: "a body the attachment cannot be written into creates no issue", @@ -1415,6 +1432,11 @@ func Test_createRun(t *testing.T) { if len(tt.attach) > 0 { opts.Assets = attachments.NewTestAssets(t, tt.attach...) } + // Given a pending event when operation counts are under test + attachmentRecorder := &telemetry.InvocationRecorderSpy{} + if tt.wantOperations != nil { + opts.AttachEvent = attachments.BeginTelemetry(attachmentRecorder, "gh test", len(tt.attach)) + } opts.Config = func() (gh.Config, error) { cfg := tt.config if cfg == "" { @@ -1423,7 +1445,12 @@ func Test_createRun(t *testing.T) { return config.NewMockConfigFromString(cfg), nil } + // When issue creation runs err := createRun(opts) + if tt.wantOperations != nil { + // Then telemetry retains completed operations, including partial results + attachments.AssertTestTelemetryEvents(t, attachmentRecorder.Events(), len(tt.attach), *tt.wantOperations) + } if tt.wantsErr == "" { require.NoError(t, err) } else { diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index 3b8557abee6..a98b64dbffa 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -51,14 +51,15 @@ type EditOptions struct { AddBlocking []string RemoveBlocking []string - AttachFlag *attachments.Flag - Assets []attachments.UserAsset - Config func() (gh.Config, error) + AttachFlag *attachments.Flag + AttachEvent *attachments.TelemetryEvent + Assets []attachments.UserAsset + Config func() (gh.Config, error) prShared.Editable } -func NewCmdEdit(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF func(*EditOptions) error) *cobra.Command { +func NewCmdEdit(f *cmdutil.Factory, telemetry ghtelemetry.InvocationRecorder, runF func(*EditOptions) error) *cobra.Command { opts := &EditOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, @@ -123,8 +124,6 @@ func NewCmdEdit(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF `), Args: cobra.MinimumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - opts.AttachFlag.RecordTelemetry(cmd.CommandPath(), telemetry) - issueNumbers, baseRepo, err := issueShared.ParseIssuesFromArgs(args) if err != nil { return err @@ -213,11 +212,11 @@ func NewCmdEdit(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF opts.Editable.IssueType.Edited = true } - resolved, err := opts.AttachFlag.UserAssets() + opts.AttachEvent = attachments.BeginTelemetry(telemetry, cmd.CommandPath(), opts.AttachFlag.Count()) + opts.Assets, err = opts.AttachFlag.UserAssets() if err != nil { return err } - opts.Assets = resolved // An empty --parent resolves to no work, so it counts as an edit // only here, where passing the flag at all suppresses the survey. @@ -418,10 +417,11 @@ func editRun(opts *EditOptions) error { // This sits outside the loop below so each file uploads once, and // Clone carries the merged body into the issue. Nothing that can // prompt or cancel may follow an upload. - var uploaded int - body, uploaded, uploadErr = uploader.UploadAndAttach(context.Background(), body, opts.Assets) + var uploadResult attachments.UploadResult + body, uploadResult, uploadErr = uploader.UploadAndAttach(context.Background(), body, opts.Assets) + opts.AttachEvent.RecordOperations(uploadResult) - if uploaded > 0 { + if uploadResult.Uploaded > 0 { editable.Body.Value = body editable.Body.Edited = true } else { diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index 5e80ddc57a3..893c3f296e5 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -41,6 +41,14 @@ func TestNewCmdEdit(t *testing.T) { tmpImage := filepath.Join(t.TempDir(), "shot.png") require.NoError(t, os.WriteFile(tmpImage, []byte("the bytes"), 0600)) + attachmentEvent := []ghtelemetry.Event{{ + Type: "attachment_invocation", + Dimensions: ghtelemetry.Dimensions{"command": "edit"}, + Measures: ghtelemetry.Measures{ + "attach_count": 1, "append_ops_count": 0, "replace_ops_count": 0, + }, + }} + tests := []struct { name string input string @@ -53,6 +61,8 @@ func TestNewCmdEdit(t *testing.T) { // wantErrIsNotExist covers an error whose text the operating system // words differently, so the assertion cannot be on the message. wantErrIsNotExist bool + wantEvents []ghtelemetry.Event + wantSampleRate int }{ { name: "no argument", @@ -408,6 +418,14 @@ func TestNewCmdEdit(t *testing.T) { Interactive: false, }, wantAssetPaths: []string{tmpImage}, + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, + }, + { + name: "argument validation skips attachment telemetry", + input: fmt.Sprintf("--attach '%s'", tmpImage), + wantsErr: true, + wantsErrMsg: "requires at least 1 arg(s), only received 0", }, { name: "attach flag beside another edit flag", @@ -423,12 +441,22 @@ func TestNewCmdEdit(t *testing.T) { }, }, wantAssetPaths: []string{tmpImage}, + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, + }, + { + name: "attach flag with more than one issue", + input: fmt.Sprintf("23 34 --attach '%s'", tmpImage), + wantsErr: true, + wantsErrMsg: "`--attach` cannot be used when editing multiple issues", + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, }, { - name: "attach flag with more than one issue", - input: fmt.Sprintf("23 34 --attach '%s'", tmpImage), + name: "body flag conflict skips attachment telemetry", + input: fmt.Sprintf("23 --body test --body-file '%s' --attach '%s'", tmpFile, tmpImage), wantsErr: true, - wantsErrMsg: "`--attach` cannot be used when editing multiple issues", + wantsErrMsg: "specify only one of `--body` or `--body-file`", }, { name: "attach flag naming a file that does not exist", @@ -436,10 +464,13 @@ func TestNewCmdEdit(t *testing.T) { wantsErr: true, wantsErrMsg: "./nope.png: ", wantErrIsNotExist: true, + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // Given command inputs and an invocation recorder ios, stdin, _, _ := iostreams.Test() ios.SetStdoutTTY(true) ios.SetStdinTTY(true) @@ -454,10 +485,10 @@ func TestNewCmdEdit(t *testing.T) { } argv, err := shlex.Split(tt.input) - assert.NoError(t, err) + require.NoError(t, err) var gotOpts *EditOptions - recorder := &telemetry.CommandRecorderSpy{} + recorder := &telemetry.InvocationRecorderSpy{} cmd := NewCmdEdit(f, recorder, func(opts *EditOptions) error { gotOpts = opts return nil @@ -469,19 +500,11 @@ func TestNewCmdEdit(t *testing.T) { cmd.SetOut(&bytes.Buffer{}) cmd.SetErr(&bytes.Buffer{}) + // When the command executes _, err = cmd.ExecuteC() - if cmd.Flags().Changed("attach") { - values, flagErr := cmd.Flags().GetStringArray("attach") - require.NoError(t, flagErr) - require.Equal(t, ghtelemetry.SAMPLE_ALL, recorder.LastSampleRate) - require.Len(t, recorder.Events, 1) - assert.Equal(t, "attachment_invocation", recorder.Events[0].Type) - assert.Equal(t, cmd.CommandPath(), recorder.Events[0].Dimensions["command"]) - assert.Equal(t, int64(len(values)), recorder.Events[0].Measures["attach_count"]) - } else { - assert.Empty(t, recorder.Events) - assert.Zero(t, recorder.LastSampleRate) - } + // Then telemetry starts only if execution reaches attachment validation + assert.Equal(t, tt.wantEvents, recorder.Events()) + assert.Equal(t, tt.wantSampleRate, recorder.LastSampleRate) if tt.wantsErr { require.Error(t, err) if tt.wantsErrMsg != "" { @@ -547,6 +570,7 @@ func Test_editRun(t *testing.T) { // Used instead of wantErrMsg when the subject is which errors survive, // leaving their wording to the layer that formats them. wantErrContains []string + wantOperations *attachments.UploadResult }{ { name: "non-interactive", @@ -1378,7 +1402,8 @@ func Test_editRun(t *testing.T) { mockIssueGetWithRepository(reg, "the original body", 1234, "WRITE") mockIssueUpdateWithBody(t, reg, "the original body\n\n![shot](https://example.com/1)") }, - stdout: "https://github.com/OWNER/REPO/issue/123\n", + stdout: "https://github.com/OWNER/REPO/issue/123\n", + wantOperations: &attachments.UploadResult{AppendOperations: 1}, }, { name: "a body flag replaces the body the attachment is then appended to", @@ -1533,6 +1558,7 @@ func Test_editRun(t *testing.T) { stdout: "https://github.com/OWNER/REPO/issue/123\n", wantErr: true, wantErrContains: []string{"./second.png"}, + wantOperations: &attachments.UploadResult{AppendOperations: 1}, }, { name: "a sole failed upload does not write the body", @@ -1746,6 +1772,11 @@ func Test_editRun(t *testing.T) { if len(tt.attach) > 0 { tt.input.Assets = attachments.NewTestAssets(t, tt.attach...) } + // Given a pending event when operation counts are under test + attachmentRecorder := &telemetry.InvocationRecorderSpy{} + if tt.wantOperations != nil { + tt.input.AttachEvent = attachments.BeginTelemetry(attachmentRecorder, "gh test", len(tt.attach)) + } hostTokens := tt.hostTokens if hostTokens == nil { @@ -1755,7 +1786,12 @@ func Test_editRun(t *testing.T) { return config.NewMockConfigFromString(hostsConfig(hostTokens)), nil } + // When issue editing runs err := editRun(tt.input) + if tt.wantOperations != nil { + // Then telemetry retains completed operations, including partial results + attachments.AssertTestTelemetryEvents(t, attachmentRecorder.Events(), len(tt.attach), *tt.wantOperations) + } if tt.wantErr { require.Error(t, err) if tt.wantErrMsg != "" { diff --git a/pkg/cmd/issue/issue.go b/pkg/cmd/issue/issue.go index 65f43929cee..4473bfe38d8 100644 --- a/pkg/cmd/issue/issue.go +++ b/pkg/cmd/issue/issue.go @@ -21,7 +21,7 @@ import ( "github.com/spf13/cobra" ) -func NewCmdIssue(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder) *cobra.Command { +func NewCmdIssue(f *cmdutil.Factory, telemetry ghtelemetry.InvocationRecorder) *cobra.Command { cmd := &cobra.Command{ Use: "issue ", Short: "Manage issues", diff --git a/pkg/cmd/pr/comment/comment.go b/pkg/cmd/pr/comment/comment.go index e7f7d504674..d68ab287a0e 100644 --- a/pkg/cmd/pr/comment/comment.go +++ b/pkg/cmd/pr/comment/comment.go @@ -10,7 +10,7 @@ import ( "github.com/spf13/cobra" ) -func NewCmdComment(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF func(*shared.CommentableOptions) error) *cobra.Command { +func NewCmdComment(f *cmdutil.Factory, telemetry ghtelemetry.InvocationRecorder, runF func(*shared.CommentableOptions) error) *cobra.Command { opts := &shared.CommentableOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, @@ -57,8 +57,6 @@ func NewCmdComment(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, ru `), Args: cobra.MaximumNArgs(1), PreRunE: func(cmd *cobra.Command, args []string) error { - opts.AttachFlag.RecordTelemetry(cmd.CommandPath(), telemetry) - if repoOverride, _ := cmd.Flags().GetString("repo"); repoOverride != "" && len(args) == 0 { return cmdutil.FlagErrorf("argument required when using the --repo flag") } @@ -80,7 +78,7 @@ func NewCmdComment(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, ru Fields: fields, }) } - if err := shared.CommentablePreRun(cmd, opts); err != nil { + if err := shared.CommentablePreRun(cmd, opts, telemetry); err != nil { return err } diff --git a/pkg/cmd/pr/comment/comment_test.go b/pkg/cmd/pr/comment/comment_test.go index c09c83cb32d..cecde19aa94 100644 --- a/pkg/cmd/pr/comment/comment_test.go +++ b/pkg/cmd/pr/comment/comment_test.go @@ -34,6 +34,14 @@ func TestNewCmdComment(t *testing.T) { tmpImage := filepath.Join(t.TempDir(), "login.png") require.NoError(t, os.WriteFile(tmpImage, []byte("the bytes"), 0600)) + attachmentEvent := []ghtelemetry.Event{{ + Type: "attachment_invocation", + Dimensions: ghtelemetry.Dimensions{"command": "comment"}, + Measures: ghtelemetry.Measures{ + "attach_count": 1, "append_ops_count": 0, "replace_ops_count": 0, + }, + }} + tests := []struct { name string input string @@ -42,6 +50,8 @@ func TestNewCmdComment(t *testing.T) { wantsErr bool wantsErrContains string isTTY bool + wantEvents []ghtelemetry.Event + wantSampleRate int // Which fields the lookup asks for is decided at construction, so it // can only be proved here. @@ -287,8 +297,10 @@ func TestNewCmdComment(t *testing.T) { wantsErr: true, }, { - name: "--attach alone is enough of a body to post without prompting", - input: fmt.Sprintf("1 --attach '%s'", tmpImage), + name: "--attach alone is enough of a body to post without prompting", + input: fmt.Sprintf("1 --attach '%s'", tmpImage), + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, output: shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -298,8 +310,16 @@ func TestNewCmdComment(t *testing.T) { wantsErr: false, }, { - name: "--attach with --body keeps the body and records that one was given", - input: fmt.Sprintf("1 --body test --attach '%s'", tmpImage), + name: "argument validation skips attachment telemetry", + input: fmt.Sprintf("1 2 --attach '%s'", tmpImage), + isTTY: false, + wantsErr: true, + }, + { + name: "--attach with --body keeps the body and records that one was given", + input: fmt.Sprintf("1 --body test --attach '%s'", tmpImage), + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, output: shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -310,8 +330,10 @@ func TestNewCmdComment(t *testing.T) { wantsErr: false, }, { - name: "--attach with --edit-last is accepted and needs no body", - input: fmt.Sprintf("1 --edit-last --attach '%s'", tmpImage), + name: "--attach with --edit-last is accepted and needs no body", + input: fmt.Sprintf("1 --edit-last --attach '%s'", tmpImage), + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, output: shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -323,8 +345,10 @@ func TestNewCmdComment(t *testing.T) { { // KeepExistingBody is set either way. BodyProvided is what decides // that this one replaces the comment rather than keeping it. - name: "--attach with --edit-last and --body records the body that replaces the comment", - input: fmt.Sprintf("1 --edit-last --body 'a new body' --attach '%s'", tmpImage), + name: "--attach with --edit-last and --body records the body that replaces the comment", + input: fmt.Sprintf("1 --edit-last --body 'a new body' --attach '%s'", tmpImage), + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, output: shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -355,10 +379,14 @@ func TestNewCmdComment(t *testing.T) { isTTY: true, wantsErr: true, wantsErrContains: "./nope.png: ", + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, }, { - name: "--attach asks the pull request lookup for the repository id", - input: fmt.Sprintf("1 --attach '%s'", tmpImage), + name: "--attach asks the pull request lookup for the repository id", + input: fmt.Sprintf("1 --attach '%s'", tmpImage), + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, output: shared.CommentableOptions{ Interactive: false, InputType: shared.InputTypeInline, @@ -383,6 +411,7 @@ func TestNewCmdComment(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // Given command inputs and an invocation recorder ios, stdin, _, _ := iostreams.Test() isTTY := tt.isTTY ios.SetStdoutTTY(isTTY) @@ -429,10 +458,10 @@ func TestNewCmdComment(t *testing.T) { } argv, err := shlex.Split(tt.input) - assert.NoError(t, err) + require.NoError(t, err) var gotOpts *shared.CommentableOptions - recorder := &telemetry.CommandRecorderSpy{} + recorder := &telemetry.InvocationRecorderSpy{} cmd := NewCmdComment(f, recorder, func(opts *shared.CommentableOptions) error { gotOpts = opts return nil @@ -444,28 +473,20 @@ func TestNewCmdComment(t *testing.T) { cmd.SetOut(&bytes.Buffer{}) cmd.SetErr(&bytes.Buffer{}) + // When the command executes _, err = cmd.ExecuteC() - if cmd.Flags().Changed("attach") { - values, flagErr := cmd.Flags().GetStringArray("attach") - require.NoError(t, flagErr) - require.Equal(t, ghtelemetry.SAMPLE_ALL, recorder.LastSampleRate) - require.Len(t, recorder.Events, 1) - assert.Equal(t, "attachment_invocation", recorder.Events[0].Type) - assert.Equal(t, cmd.CommandPath(), recorder.Events[0].Dimensions["command"]) - assert.Equal(t, int64(len(values)), recorder.Events[0].Measures["attach_count"]) - } else { - assert.Empty(t, recorder.Events) - assert.Zero(t, recorder.LastSampleRate) - } + // Then telemetry starts only if execution reaches attachment validation + assert.Equal(t, tt.wantEvents, recorder.Events()) + assert.Equal(t, tt.wantSampleRate, recorder.LastSampleRate) if tt.wantsErr { - assert.Error(t, err) + require.Error(t, err) if tt.wantsErrContains != "" { - assert.ErrorContains(t, err, tt.wantsErrContains) + require.ErrorContains(t, err, tt.wantsErrContains) } return } - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, tt.output.Interactive, gotOpts.Interactive) assert.Equal(t, tt.output.InputType, gotOpts.InputType) assert.Equal(t, tt.output.Body, gotOpts.Body) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index c365fadebab..794481d82ff 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -77,8 +77,9 @@ type CreateOptions struct { DryRun bool - AttachFlag *attachments.Flag - Assets []attachments.UserAsset + AttachFlag *attachments.Flag + AttachEvent *attachments.TelemetryEvent + Assets []attachments.UserAsset } // creationRefs is an interface that provides the necessary information for creating a pull request in the API. @@ -196,7 +197,7 @@ type CreateContext struct { GitClient *git.Client } -func NewCmdCreate(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF func(*CreateOptions) error) *cobra.Command { +func NewCmdCreate(f *cmdutil.Factory, telemetry ghtelemetry.InvocationRecorder, runF func(*CreateOptions) error) *cobra.Command { opts := &CreateOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, @@ -275,8 +276,6 @@ func NewCmdCreate(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, run Args: cmdutil.NoArgsQuoteReminder, Aliases: []string{"new"}, RunE: func(cmd *cobra.Command, args []string) error { - opts.AttachFlag.RecordTelemetry(cmd.CommandPath(), telemetry) - opts.Finder = shared.NewFinder(f) opts.TitleProvided = cmd.Flags().Changed("title") @@ -370,6 +369,7 @@ func NewCmdCreate(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, run return err } + opts.AttachEvent = attachments.BeginTelemetry(telemetry, cmd.CommandPath(), opts.AttachFlag.Count()) opts.Assets, err = opts.AttachFlag.UserAssets() if err != nil { return err @@ -1127,12 +1127,13 @@ func submitPR(opts CreateOptions, ctx CreateContext, state shared.IssueMetadataS var uploadErr error if uploader != nil { - body, uploaded, err := uploader.UploadAndAttach(context.Background(), state.Body, opts.Assets) + body, uploadResult, err := uploader.UploadAndAttach(context.Background(), state.Body, opts.Assets) + opts.AttachEvent.RecordOperations(uploadResult) // With nothing uploaded, a body that lost the files it was written // around is not what the caller asked to create. The branch is already // pushed by now, so the message says what was not created rather than // claiming the run had no effect. - if err != nil && uploaded == 0 { + if err != nil && uploadResult.Uploaded == 0 { return fmt.Errorf("%w\nno pull request was created", err) } uploadErr = err diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index e6ecc1047a4..ddcae53e0bf 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -44,6 +44,14 @@ func TestNewCmdCreate(t *testing.T) { tmpImage := filepath.Join(t.TempDir(), "shot.png") require.NoError(t, os.WriteFile(tmpImage, []byte("the bytes"), 0600)) + attachmentEvent := []ghtelemetry.Event{{ + Type: "attachment_invocation", + Dimensions: ghtelemetry.Dimensions{"command": "create"}, + Measures: ghtelemetry.Measures{ + "attach_count": 1, "append_ops_count": 0, "replace_ops_count": 0, + }, + }} + tests := []struct { name string tty bool @@ -57,6 +65,8 @@ func TestNewCmdCreate(t *testing.T) { wantErrIsNotExist bool wantAssetPaths []string wantsOpts CreateOptions + wantEvents []ghtelemetry.Event + wantSampleRate int }{ { name: "empty non-tty", @@ -295,6 +305,13 @@ func TestNewCmdCreate(t *testing.T) { MaintainerCanModify: true, }, wantAssetPaths: []string{tmpImage}, + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, + }, + { + name: "argument validation skips attachment telemetry", + cli: fmt.Sprintf("unexpected --attach '%s'", tmpImage), + wantsErr: true, }, { name: "attach rejects a missing file", @@ -302,6 +319,8 @@ func TestNewCmdCreate(t *testing.T) { wantsErr: true, wantsErrMsg: "./nope.png: ", wantErrIsNotExist: true, + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, }, { name: "attach conflict is reported before a missing file", @@ -318,6 +337,7 @@ func TestNewCmdCreate(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // Given command inputs and an invocation recorder ios, stdin, stdout, stderr := iostreams.Test() if tt.stdin != "" { _, _ = stdin.WriteString(tt.stdin) @@ -337,7 +357,7 @@ func TestNewCmdCreate(t *testing.T) { } var opts *CreateOptions - recorder := &telemetry.CommandRecorderSpy{} + recorder := &telemetry.InvocationRecorderSpy{} cmd := NewCmdCreate(f, recorder, func(o *CreateOptions) error { opts = o return nil @@ -348,19 +368,11 @@ func TestNewCmdCreate(t *testing.T) { cmd.SetArgs(args) cmd.SetOut(stderr) cmd.SetErr(stderr) + // When the command executes _, err = cmd.ExecuteC() - if cmd.Flags().Changed("attach") { - values, flagErr := cmd.Flags().GetStringArray("attach") - require.NoError(t, flagErr) - require.Equal(t, ghtelemetry.SAMPLE_ALL, recorder.LastSampleRate) - require.Len(t, recorder.Events, 1) - assert.Equal(t, "attachment_invocation", recorder.Events[0].Type) - assert.Equal(t, cmd.CommandPath(), recorder.Events[0].Dimensions["command"]) - assert.Equal(t, int64(len(values)), recorder.Events[0].Measures["attach_count"]) - } else { - assert.Empty(t, recorder.Events) - assert.Zero(t, recorder.LastSampleRate) - } + // Then telemetry starts only if execution reaches attachment validation + assert.Equal(t, tt.wantEvents, recorder.Events()) + assert.Equal(t, tt.wantSampleRate, recorder.LastSampleRate) if tt.wantsErr { if tt.wantsErrMsg != "" { if tt.wantErrIsNotExist { @@ -419,6 +431,7 @@ func Test_createRun(t *testing.T) { customBranchConfig bool // Defaults to WRITE, which can upload. repoPermission string + wantOperations *attachments.UploadResult }{ { name: "nontty web", @@ -1775,7 +1788,8 @@ func Test_createRun(t *testing.T) { assert.Equal(t, "before ![the shot](https://github.com/user-attachments/assets/ASSET) after", input["body"]) })) }, - expectedOut: "https://github.com/OWNER/REPO/pull/12\n", + expectedOut: "https://github.com/OWNER/REPO/pull/12\n", + wantOperations: &attachments.UploadResult{ReplaceOperations: 1}, }, { @@ -1869,8 +1883,9 @@ func Test_createRun(t *testing.T) { assert.Equal(t, "my body\n\n![good](https://github.com/user-attachments/assets/ASSET)", input["body"]) })) }, - expectedOut: "https://github.com/OWNER/REPO/pull/12\n", - wantErr: "could not upload ./bad.png: attaching files requires write access to the repository", + expectedOut: "https://github.com/OWNER/REPO/pull/12\n", + wantErr: "could not upload ./bad.png: attaching files requires write access to the repository", + wantOperations: &attachments.UploadResult{AppendOperations: 1}, }, { name: "the only upload failing creates no pull request", @@ -1927,7 +1942,8 @@ func Test_createRun(t *testing.T) { httpmock.GraphQL(`mutation PullRequestCreate\b`), httpmock.StringResponse(`{"errors":[{"message":"the create failed"}]}`)) }, - wantErr: "could not upload ./bad.png: attaching files requires write access to the repository\npull request create failed: GraphQL: the create failed", + wantErr: "could not upload ./bad.png: attaching files requires write access to the repository\npull request create failed: GraphQL: the create failed", + wantOperations: &attachments.UploadResult{AppendOperations: 1}, }, { name: "a permission that cannot upload stops the command before it prompts", @@ -2041,6 +2057,11 @@ func Test_createRun(t *testing.T) { cleanSetup = tt.setup(&opts, t) } defer cleanSetup() + // Given a pending event when operation counts are under test + attachmentRecorder := &telemetry.InvocationRecorderSpy{} + if tt.wantOperations != nil { + opts.AttachEvent = attachments.BeginTelemetry(attachmentRecorder, "gh test", len(opts.Assets)) + } // All tests in this function use github.com behavior opts.Detector = &fd.EnabledDetectorMock{} @@ -2049,7 +2070,12 @@ func Test_createRun(t *testing.T) { cs.Register(`git status --porcelain`, 0, "") } + // When pull request creation runs err := createRun(&opts) + if tt.wantOperations != nil { + // Then telemetry retains completed operations, including partial results + attachments.AssertTestTelemetryEvents(t, attachmentRecorder.Events(), len(opts.Assets), *tt.wantOperations) + } output := &test.CmdOut{ OutBuf: stdout, ErrBuf: stderr, diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index fb8161f823c..a72ff954525 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -40,14 +40,15 @@ type EditOptions struct { SelectorArg string Interactive bool - AttachFlag *attachments.Flag - Assets []attachments.UserAsset - Config func() (gh.Config, error) + AttachFlag *attachments.Flag + AttachEvent *attachments.TelemetryEvent + Assets []attachments.UserAsset + Config func() (gh.Config, error) shared.Editable } -func NewCmdEdit(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF func(*EditOptions) error) *cobra.Command { +func NewCmdEdit(f *cmdutil.Factory, telemetry ghtelemetry.InvocationRecorder, runF func(*EditOptions) error) *cobra.Command { opts := &EditOptions{ IO: f.IOStreams, HttpClient: f.HttpClient, @@ -135,8 +136,6 @@ func NewCmdEdit(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - opts.AttachFlag.RecordTelemetry(cmd.CommandPath(), telemetry) - opts.Finder = shared.NewFinder(f) // support `-R, --repo` override @@ -221,11 +220,12 @@ func NewCmdEdit(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF // see the `Editable.MilestoneId` method. } - resolved, err := opts.AttachFlag.UserAssets() + var err error + opts.AttachEvent = attachments.BeginTelemetry(telemetry, cmd.CommandPath(), opts.AttachFlag.Count()) + opts.Assets, err = opts.AttachFlag.UserAssets() if err != nil { return err } - opts.Assets = resolved if !opts.Editable.Dirty() && len(opts.Assets) == 0 { opts.Interactive = true @@ -423,14 +423,15 @@ func editRun(opts *EditOptions) error { } // Nothing that can prompt or cancel may follow this. - var uploaded int - body, uploaded, uploadErr = uploader.UploadAndAttach(context.Background(), body, opts.Assets) + var uploadResult attachments.UploadResult + body, uploadResult, uploadErr = uploader.UploadAndAttach(context.Background(), body, opts.Assets) + opts.AttachEvent.RecordOperations(uploadResult) // With nothing uploaded, even a body the caller typed goes unwritten: // its references are still local paths, which render broken. The other // fields are innocent of the upload, so they proceed either way. - editable.Body.Edited = uploaded > 0 - if uploaded > 0 { + editable.Body.Edited = uploadResult.Uploaded > 0 + if uploadResult.Uploaded > 0 { editable.Body.Value = body } diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 98cbd60b2d9..dd7c6524173 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -35,6 +35,14 @@ func TestNewCmdEdit(t *testing.T) { tmpImage := filepath.Join(t.TempDir(), "shot.png") require.NoError(t, os.WriteFile(tmpImage, []byte("the bytes"), 0600)) + attachmentEvent := []ghtelemetry.Event{{ + Type: "attachment_invocation", + Dimensions: ghtelemetry.Dimensions{"command": "edit"}, + Measures: ghtelemetry.Measures{ + "attach_count": 1, "append_ops_count": 0, "replace_ops_count": 0, + }, + }} + tests := []struct { name string input string @@ -43,6 +51,8 @@ func TestNewCmdEdit(t *testing.T) { wantAssetPaths []string expectedBaseRepo ghrepo.Interface wantsErr bool + wantEvents []ghtelemetry.Event + wantSampleRate int }{ { name: "no argument", @@ -315,6 +325,13 @@ func TestNewCmdEdit(t *testing.T) { Interactive: false, }, wantAssetPaths: []string{tmpImage}, + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, + }, + { + name: "argument validation skips attachment telemetry", + input: fmt.Sprintf("23 24 --attach '%s'", tmpImage), + wantsErr: true, }, { name: "attach with body records the body that replaces the old one", @@ -330,15 +347,25 @@ func TestNewCmdEdit(t *testing.T) { }, }, wantAssetPaths: []string{tmpImage}, + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, + }, + { + name: "attach rejects a missing file", + input: "23 --attach ./nope.png", + wantsErr: true, + wantEvents: attachmentEvent, + wantSampleRate: ghtelemetry.SAMPLE_ALL, }, { - name: "attach rejects a missing file", - input: "23 --attach ./nope.png", + name: "body flag conflict skips attachment telemetry", + input: fmt.Sprintf("23 --body test --body-file '%s' --attach '%s'", tmpFile, tmpImage), wantsErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // Given command inputs and an invocation recorder ios, stdin, _, _ := iostreams.Test() ios.SetStdoutTTY(true) ios.SetStdinTTY(true) @@ -353,10 +380,10 @@ func TestNewCmdEdit(t *testing.T) { } argv, err := shlex.Split(tt.input) - assert.NoError(t, err) + require.NoError(t, err) var gotOpts *EditOptions - recorder := &telemetry.CommandRecorderSpy{} + recorder := &telemetry.InvocationRecorderSpy{} cmd := NewCmdEdit(f, recorder, func(opts *EditOptions) error { gotOpts = opts return nil @@ -368,25 +395,17 @@ func TestNewCmdEdit(t *testing.T) { cmd.SetOut(&bytes.Buffer{}) cmd.SetErr(&bytes.Buffer{}) + // When the command executes _, err = cmd.ExecuteC() - if cmd.Flags().Changed("attach") { - values, flagErr := cmd.Flags().GetStringArray("attach") - require.NoError(t, flagErr) - require.Equal(t, ghtelemetry.SAMPLE_ALL, recorder.LastSampleRate) - require.Len(t, recorder.Events, 1) - assert.Equal(t, "attachment_invocation", recorder.Events[0].Type) - assert.Equal(t, cmd.CommandPath(), recorder.Events[0].Dimensions["command"]) - assert.Equal(t, int64(len(values)), recorder.Events[0].Measures["attach_count"]) - } else { - assert.Empty(t, recorder.Events) - assert.Zero(t, recorder.LastSampleRate) - } + // Then telemetry starts only if execution reaches attachment validation + assert.Equal(t, tt.wantEvents, recorder.Events()) + assert.Equal(t, tt.wantSampleRate, recorder.LastSampleRate) if tt.wantsErr { - assert.Error(t, err) + require.Error(t, err) return } - assert.NoError(t, err) + require.NoError(t, err) assert.Equal(t, tt.output.SelectorArg, gotOpts.SelectorArg) assert.Equal(t, tt.output.Interactive, gotOpts.Interactive) assert.Equal(t, tt.output.Editable, gotOpts.Editable) @@ -428,6 +447,7 @@ func Test_editRun(t *testing.T) { stdout string stderr string wantErr string + wantOperations *attachments.UploadResult }{ { name: "non-interactive", @@ -1289,7 +1309,8 @@ func Test_editRun(t *testing.T) { httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockPullRequestUpdateWithBody(t, reg, "the original body\n\n![shot](https://example.com/1)") }, - stdout: "https://github.com/OWNER/REPO/pull/123\n", + stdout: "https://github.com/OWNER/REPO/pull/123\n", + wantOperations: &attachments.UploadResult{AppendOperations: 1}, }, { name: "an empty body flag clears the body and leaves the attachment", @@ -1369,8 +1390,9 @@ func Test_editRun(t *testing.T) { httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockPullRequestUpdateWithBody(t, reg, "the original body\n\n![a](https://example.com/1)") }, - stdout: "https://github.com/OWNER/REPO/pull/123\n", - wantErr: "could not upload ./b.png: attaching files requires write access to the repository", + stdout: "https://github.com/OWNER/REPO/pull/123\n", + wantErr: "could not upload ./b.png: attaching files requires write access to the repository", + wantOperations: &attachments.UploadResult{AppendOperations: 1}, }, { name: "a sole failed upload leaves the body alone and still edits the title", @@ -1674,6 +1696,11 @@ func Test_editRun(t *testing.T) { if len(tt.attach) > 0 { tt.input.Assets = attachments.NewTestAssets(t, tt.attach...) } + // Given a pending event when operation counts are under test + attachmentRecorder := &telemetry.InvocationRecorderSpy{} + if tt.wantOperations != nil { + tt.input.AttachEvent = attachments.BeginTelemetry(attachmentRecorder, "gh test", len(tt.attach)) + } // The host comes from the pull request the row's finder returns, so // a row can hold a token for a host the config's default is not. @@ -1692,7 +1719,12 @@ func Test_editRun(t *testing.T) { var lookupFields []string tt.input.Finder = fieldCapturingFinder{PRFinder: tt.input.Finder, fields: &lookupFields} + // When pull request editing runs err := editRun(tt.input) + if tt.wantOperations != nil { + // Then telemetry retains completed operations, including partial results + attachments.AssertTestTelemetryEvents(t, attachmentRecorder.Events(), len(tt.attach), *tt.wantOperations) + } if tt.wantErr != "" { require.EqualError(t, err, tt.wantErr) } else { diff --git a/pkg/cmd/pr/pr.go b/pkg/cmd/pr/pr.go index 225b62ca7b7..aa40813143a 100644 --- a/pkg/cmd/pr/pr.go +++ b/pkg/cmd/pr/pr.go @@ -24,7 +24,7 @@ import ( "github.com/spf13/cobra" ) -func NewCmdPR(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder) *cobra.Command { +func NewCmdPR(f *cmdutil.Factory, telemetry ghtelemetry.InvocationRecorder) *cobra.Command { cmd := &cobra.Command{ Use: "pr ", Short: "Manage pull requests", diff --git a/pkg/cmd/pr/shared/commentable.go b/pkg/cmd/pr/shared/commentable.go index db241e5e064..43e2e0621f6 100644 --- a/pkg/cmd/pr/shared/commentable.go +++ b/pkg/cmd/pr/shared/commentable.go @@ -11,6 +11,7 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/attachments" "github.com/cli/cli/v2/internal/gh" + "github.com/cli/cli/v2/internal/gh/ghtelemetry" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/text" "github.com/cli/cli/v2/pkg/cmdutil" @@ -61,11 +62,12 @@ type CommentableOptions struct { BodyProvided bool KeepExistingBody bool AttachFlag *attachments.Flag + AttachEvent *attachments.TelemetryEvent Assets []attachments.UserAsset Config func() (gh.Config, error) } -func CommentablePreRun(cmd *cobra.Command, opts *CommentableOptions) error { +func CommentablePreRun(cmd *cobra.Command, opts *CommentableOptions, telemetry ghtelemetry.InvocationRecorder) error { inputFlags := 0 if cmd.Flags().Changed("body") { opts.InputType = InputTypeInline @@ -103,11 +105,12 @@ func CommentablePreRun(cmd *cobra.Command, opts *CommentableOptions) error { return err } - resolved, err := opts.AttachFlag.UserAssets() + var err error + opts.AttachEvent = attachments.BeginTelemetry(telemetry, cmd.CommandPath(), opts.AttachFlag.Count()) + opts.Assets, err = opts.AttachFlag.UserAssets() if err != nil { return err } - opts.Assets = resolved // An asset is a body input on its own, so `--attach shot.png` alone // posts an image with no text. It is not part of the mutually exclusive @@ -353,8 +356,9 @@ func bodyForWrite(opts *CommentableOptions, uploader *attachments.Uploader) (bod if uploader == nil { return opts.Body, true, nil } - body, uploaded, err := uploader.UploadAndAttach(context.Background(), opts.Body, opts.Assets) - if err != nil && uploaded == 0 { + body, uploadResult, err := uploader.UploadAndAttach(context.Background(), opts.Body, opts.Assets) + opts.AttachEvent.RecordOperations(uploadResult) + if err != nil && uploadResult.Uploaded == 0 { return "", false, err } return body, true, err diff --git a/pkg/cmd/pr/shared/commentable_test.go b/pkg/cmd/pr/shared/commentable_test.go index 41629dae092..3b559a87985 100644 --- a/pkg/cmd/pr/shared/commentable_test.go +++ b/pkg/cmd/pr/shared/commentable_test.go @@ -14,6 +14,7 @@ import ( "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/telemetry" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" @@ -60,8 +61,6 @@ func TestCommentablePreRun(t *testing.T) { wantBodyProvided bool }{ { - // A change that separates counting the input from resolving it - // fails here, since this row asserts both from one run. name: "attach alone is a body input and is resolved", input: "--attach ./shot.png", wantInputType: InputTypeInline, @@ -136,6 +135,7 @@ func TestCommentablePreRun(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // Given comment inputs and the files they reference t.Chdir(t.TempDir()) require.NoError(t, os.WriteFile("shot.png", []byte("the bytes"), 0o600)) @@ -147,8 +147,10 @@ func TestCommentablePreRun(t *testing.T) { opts := &CommentableOptions{IO: ios} cmd := commentableCmd(t, opts, tt.input) - err := CommentablePreRun(cmd, opts) + // When comment preparation validates those inputs + err := CommentablePreRun(cmd, opts, &telemetry.NoOpService{}) + // Then it reports the validation error or prepares the comment options if tt.wantErr != "" { if tt.wantErrIsNotExist { require.ErrorIs(t, err, fs.ErrNotExist) @@ -189,6 +191,7 @@ func TestCommentableRunUploadsAndWritesBodies(t *testing.T) { wantStdout string wantErr string wantUploads int + wantOperations *attachments.UploadResult }{ { name: "creating with no asset uploads nothing", @@ -207,6 +210,7 @@ func TestCommentableRunUploadsAndWritesBodies(t *testing.T) { wantBody: "see below\n\n![shot](https://example.com/1)", wantStdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", wantUploads: 1, + wantOperations: &attachments.UploadResult{AppendOperations: 1}, }, { name: "creating writes what uploaded when one upload fails", @@ -217,11 +221,12 @@ func TestCommentableRunUploadsAndWritesBodies(t *testing.T) { {Name: "a.png", Status: 201, Body: `{"url":"https://example.com/1"}`}, {Name: "b.png", Status: 404, Body: `{"message":"Not Found"}`}, }, - wantQuery: `mutation CommentCreate\b`, - wantBody: "see below\n\n![a](https://example.com/1)", - wantStdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", - wantErr: "could not upload ./b.png: attaching files requires write access to the repository", - wantUploads: 2, + wantQuery: `mutation CommentCreate\b`, + wantBody: "see below\n\n![a](https://example.com/1)", + wantStdout: "https://github.com/OWNER/REPO/pull/123#issuecomment-456\n", + wantErr: "could not upload ./b.png: attaching files requires write access to the repository", + wantUploads: 2, + wantOperations: &attachments.UploadResult{AppendOperations: 1}, }, { name: "creating writes nothing when every upload fails", @@ -231,6 +236,7 @@ func TestCommentableRunUploadsAndWritesBodies(t *testing.T) { uploads: []attachments.UploadStub{{Name: "a.png", Status: 404, Body: `{"message":"Not Found"}`}}, wantErr: "could not upload ./a.png: attaching files requires write access to the repository\nno comment was posted", wantUploads: 1, + wantOperations: &attachments.UploadResult{}, }, { name: "creating writes nothing when every upload fails and the body has text", @@ -247,6 +253,7 @@ func TestCommentableRunUploadsAndWritesBodies(t *testing.T) { attach: []string{"repro.mp4"}, repositoryDatabaseID: 1234, wantErr: "cannot embed a video as a reference-style image: ./repro.mp4\nno comment was posted", + wantOperations: &attachments.UploadResult{}, }, { name: "creating reports the upload and the write when both fail", @@ -257,10 +264,11 @@ func TestCommentableRunUploadsAndWritesBodies(t *testing.T) { {Name: "a.png", Status: 201, Body: `{"url":"https://example.com/1"}`}, {Name: "b.png", Status: 404, Body: `{"message":"Not Found"}`}, }, - wantQuery: `mutation CommentCreate\b`, - writeFails: true, - wantErr: "could not upload ./b.png: attaching files requires write access to the repository\nGraphQL: the write failed", - wantUploads: 2, + wantQuery: `mutation CommentCreate\b`, + writeFails: true, + wantErr: "could not upload ./b.png: attaching files requires write access to the repository\nGraphQL: the write failed", + wantUploads: 2, + wantOperations: &attachments.UploadResult{AppendOperations: 1}, }, { name: "editing keeps the comment when no body flag was given", @@ -435,6 +443,11 @@ func TestCommentableRunUploadsAndWritesBodies(t *testing.T) { if len(tt.attach) > 0 { opts.Assets = attachments.NewTestAssets(t, tt.attach...) } + // Given a pending event when operation counts are under test + attachmentRecorder := &telemetry.InvocationRecorderSpy{} + if tt.wantOperations != nil { + opts.AttachEvent = attachments.BeginTelemetry(attachmentRecorder, "gh test", len(tt.attach)) + } host := tt.host if host == "" { @@ -473,7 +486,12 @@ func TestCommentableRunUploadsAndWritesBodies(t *testing.T) { }, ghrepo.NewWithHost("OWNER", "REPO", host), nil } + // When comment creation or editing runs err := CommentableRun(&opts) + if tt.wantOperations != nil { + // Then telemetry retains completed operations, including partial results + attachments.AssertTestTelemetryEvents(t, attachmentRecorder.Events(), len(tt.attach), *tt.wantOperations) + } if tt.wantErr != "" { require.EqualError(t, err, tt.wantErr) diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index f10d6a6ff8d..e8bc2660648 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -931,6 +931,9 @@ func interactiveRepoNameAndOwner(client *http.Client, hostname string, prompter if err != nil { return "", "", err } + if strings.TrimSpace(name) == "" { + return "", "", errors.New("repository name cannot be blank") + } name, owner, err := splitNameAndOwner(name) if err != nil { diff --git a/pkg/cmd/repo/create/create_test.go b/pkg/cmd/repo/create/create_test.go index 6c28858ee12..eac15bf45ce 100644 --- a/pkg/cmd/repo/create/create_test.go +++ b/pkg/cmd/repo/create/create_test.go @@ -198,6 +198,52 @@ func Test_createRun(t *testing.T) { wantErr bool errMsg string }{ + { + name: "interactive create from scratch with empty name", + opts: &CreateOptions{Interactive: true}, + tty: true, + promptStubs: func(p *prompter.PrompterMock) { + p.InputFunc = func(message, defaultValue string) (string, error) { + if message != "Repository name" || len(p.InputCalls()) != 1 { + return "", fmt.Errorf("unexpected input prompt: %s", message) + } + return "", nil + } + p.SelectFunc = func(message, defaultValue string, options []string) (int, error) { + switch message { + case "What would you like to do?": + return prompter.IndexFor(options, "Create a new repository on github.com from scratch") + default: + return 0, fmt.Errorf("unexpected select prompt: %s", message) + } + } + }, + wantErr: true, + errMsg: "repository name cannot be blank", + }, + { + name: "interactive create from scratch with whitespace-only name", + opts: &CreateOptions{Interactive: true}, + tty: true, + promptStubs: func(p *prompter.PrompterMock) { + p.InputFunc = func(message, defaultValue string) (string, error) { + if message != "Repository name" || len(p.InputCalls()) != 1 { + return "", fmt.Errorf("unexpected input prompt: %s", message) + } + return " \t ", nil + } + p.SelectFunc = func(message, defaultValue string, options []string) (int, error) { + switch message { + case "What would you like to do?": + return prompter.IndexFor(options, "Create a new repository on github.com from scratch") + default: + return 0, fmt.Errorf("unexpected select prompt: %s", message) + } + } + }, + wantErr: true, + errMsg: "repository name cannot be blank", + }, { name: "interactive create from scratch with gitignore and license", opts: &CreateOptions{Interactive: true}, @@ -1077,9 +1123,9 @@ func Test_createRun(t *testing.T) { if tt.wantErr { require.Error(t, err) assert.Contains(t, err.Error(), tt.errMsg) - return + } else { + require.NoError(t, err) } - require.NoError(t, err) assert.Equal(t, tt.wantStdout, stdout.String()) assert.Equal(t, "", stderr.String()) }) diff --git a/pkg/cmd/root/help_topic.go b/pkg/cmd/root/help_topic.go index 491750bbb4e..c431c10489c 100644 --- a/pkg/cmd/root/help_topic.go +++ b/pkg/cmd/root/help_topic.go @@ -111,7 +111,8 @@ var HelpTopics = []helpTopic{ %[1]sGH_PROMPT_DISABLED%[1]s: set to any value to disable interactive prompting in the terminal. %[1]sGH_PATH%[1]s: set the path to the gh executable, useful for when gh can not properly determine - its own path such as in the cygwin terminal. + its own path such as in the cygwin terminal. gh also sets this when invoking extensions so they + can call back into the same gh executable. %[1]sGH_MDWIDTH%[1]s: default maximum width for markdown render wrapping. The max width of lines wrapped on the terminal will be taken as the lesser of the terminal width, this value, or 120 if diff --git a/pkg/cmd/root/root.go b/pkg/cmd/root/root.go index 985a0d82a41..124708cb46a 100644 --- a/pkg/cmd/root/root.go +++ b/pkg/cmd/root/root.go @@ -61,7 +61,7 @@ func (ae *AuthError) Error() string { return ae.err.Error() } -func NewCmdRoot(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, version, buildDate string) (*cobra.Command, error) { +func NewCmdRoot(f *cmdutil.Factory, telemetry ghtelemetry.InvocationRecorder, version, buildDate string) (*cobra.Command, error) { io := f.IOStreams cfg, err := f.Config() if err != nil { diff --git a/pkg/cmd/skills/install/install.go b/pkg/cmd/skills/install/install.go index 64be5da5a79..2095f95a3a5 100644 --- a/pkg/cmd/skills/install/install.go +++ b/pkg/cmd/skills/install/install.go @@ -73,7 +73,7 @@ type InstallOptions struct { } // NewCmdInstall creates the "skills install" command. -func NewCmdInstall(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF func(*InstallOptions) error) *cobra.Command { +func NewCmdInstall(f *cmdutil.Factory, telemetry ghtelemetry.EventRecorder, runF func(*InstallOptions) error) *cobra.Command { opts := &InstallOptions{ IO: f.IOStreams, Telemetry: telemetry, diff --git a/pkg/cmd/skills/install/install_test.go b/pkg/cmd/skills/install/install_test.go index 34f9188a855..4f488578b1d 100644 --- a/pkg/cmd/skills/install/install_test.go +++ b/pkg/cmd/skills/install/install_test.go @@ -157,7 +157,7 @@ func TestNewCmdInstall(t *testing.T) { } var gotOpts *InstallOptions - cmd := NewCmdInstall(f, &telemetry.NoOpService{}, func(opts *InstallOptions) error { + cmd := NewCmdInstall(f, &telemetry.EventRecorderSpy{}, func(opts *InstallOptions) error { gotOpts = opts return nil }) @@ -198,7 +198,7 @@ func TestNewCmdInstall(t *testing.T) { t.Run("command metadata", func(t *testing.T) { ios, _, _, _ := iostreams.Test() f := &cmdutil.Factory{IOStreams: ios, Prompter: &prompter.PrompterMock{}, GitClient: &git.Client{}} - cmd := NewCmdInstall(f, &telemetry.NoOpService{}, nil) + cmd := NewCmdInstall(f, &telemetry.EventRecorderSpy{}, nil) assert.Equal(t, "install [] [flags]", cmd.Use) assert.NotEmpty(t, cmd.Short) @@ -2624,8 +2624,9 @@ func TestInstallRun_TelemetryVisibility(t *testing.T) { }) require.NoError(t, err) - require.Len(t, recorder.Events, 1) - event := recorder.Events[0] + events := recorder.Events() + require.Len(t, events, 1) + event := events[0] assert.Equal(t, "skill_install", event.Type) assert.NotEmpty(t, event.Dimensions["agent_hosts"], "agent_hosts should always be present") @@ -2720,8 +2721,9 @@ func TestInstallRun_TelemetryMultipleSkills(t *testing.T) { }) require.NoError(t, err) - require.Len(t, recorder.Events, 1) - event := recorder.Events[0] + events := recorder.Events() + require.Len(t, events, 1) + event := events[0] assert.Equal(t, "skill_install", event.Type) assert.Equal(t, "public", event.Dimensions["repo_visibility"]) diff --git a/pkg/cmd/skills/list/list.go b/pkg/cmd/skills/list/list.go index 88c6a0d3910..6ec067626d5 100644 --- a/pkg/cmd/skills/list/list.go +++ b/pkg/cmd/skills/list/list.go @@ -106,7 +106,7 @@ func (s listedSkill) ExportData(fields []string) map[string]any { } // NewCmdList creates the "skills list" command. -func NewCmdList(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF func(*ListOptions) error) *cobra.Command { +func NewCmdList(f *cmdutil.Factory, telemetry ghtelemetry.EventRecorder, runF func(*ListOptions) error) *cobra.Command { opts := &ListOptions{ IO: f.IOStreams, Telemetry: telemetry, diff --git a/pkg/cmd/skills/list/list_test.go b/pkg/cmd/skills/list/list_test.go index 94295c7ba6a..9eda58ebeaa 100644 --- a/pkg/cmd/skills/list/list_test.go +++ b/pkg/cmd/skills/list/list_test.go @@ -88,7 +88,7 @@ func TestNewCmdList(t *testing.T) { } var gotOpts *ListOptions - cmd := NewCmdList(f, &telemetry.NoOpService{}, func(opts *ListOptions) error { + cmd := NewCmdList(f, &telemetry.EventRecorderSpy{}, func(opts *ListOptions) error { gotOpts = opts return nil }) @@ -123,18 +123,18 @@ func TestListRun(t *testing.T) { tests := []struct { name string setup func(t *testing.T, repoDir, homeDir string) - opts func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.CommandRecorderSpy) *ListOptions + opts func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.EventRecorderSpy) *ListOptions wantStdout string wantJSON string wantErr string - verify func(t *testing.T, stdout string, spy *telemetry.CommandRecorderSpy) + verify func(t *testing.T, stdout string, spy *telemetry.EventRecorderSpy) }{ { name: "lists project skill for selected shared agent", setup: func(t *testing.T, repoDir, homeDir string) { writeSkill(t, repoDir, ".agents/skills/git-commit", remoteSkillFrontmatter("git-commit", "skills/git-commit", "refs/tags/v1.0.0", "")) }, - opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.CommandRecorderSpy) *ListOptions { + opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.EventRecorderSpy) *ListOptions { return &ListOptions{ IO: ios, Telemetry: spy, @@ -144,9 +144,10 @@ func TestListRun(t *testing.T) { } }, wantStdout: "git-commit\tcursor\tproject\tmonalisa/skills-repo\n", - verify: func(t *testing.T, stdout string, spy *telemetry.CommandRecorderSpy) { - require.Len(t, spy.Events, 1) - event := spy.Events[0] + verify: func(t *testing.T, stdout string, spy *telemetry.EventRecorderSpy) { + events := spy.Events() + require.Len(t, events, 1) + event := events[0] assert.Equal(t, "skill_list", event.Type) assert.Equal(t, "cursor", event.Dimensions["agent_hosts"]) assert.Equal(t, "project", event.Dimensions["scope"]) @@ -158,7 +159,7 @@ func TestListRun(t *testing.T) { setup: func(t *testing.T, repoDir, homeDir string) { writeSkill(t, homeDir, ".copilot/skills/code-review", remoteSkillFrontmatter("code-review", "skills/code-review", "refs/tags/v2.0.0", "v2.0.0")) }, - opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.CommandRecorderSpy) *ListOptions { + opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.EventRecorderSpy) *ListOptions { exporter := cmdutil.NewJSONExporter() exporter.SetFields([]string{"skillName", "agentHosts", "scope", "sourceURL", "version", "pinned", "path"}) return &ListOptions{ @@ -181,8 +182,8 @@ func TestListRun(t *testing.T) { "path": %q } ]`, filepath.Join("HOME", ".copilot", "skills", "code-review")), - verify: func(t *testing.T, stdout string, spy *telemetry.CommandRecorderSpy) { - assert.Equal(t, "json", spy.Events[0].Dimensions["format"]) + verify: func(t *testing.T, stdout string, spy *telemetry.EventRecorderSpy) { + assert.Equal(t, "json", spy.Events()[0].Dimensions["format"]) }, }, { @@ -190,7 +191,7 @@ func TestListRun(t *testing.T) { setup: func(t *testing.T, repoDir, homeDir string) { writeSkill(t, homeDir, ".copilot/skills/tenant-skill", remoteSkillFrontmatterForRepo("tenant-skill", "https://octocorp.ghe.com/monalisa/skills-repo", "skills/tenant-skill", "refs/heads/main", "")) }, - opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.CommandRecorderSpy) *ListOptions { + opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.EventRecorderSpy) *ListOptions { exporter := cmdutil.NewJSONExporter() exporter.SetFields([]string{"skillName", "sourceURL", "path"}) return &ListOptions{ @@ -223,7 +224,7 @@ func TestListRun(t *testing.T) { Body `)) }, - opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.CommandRecorderSpy) *ListOptions { + opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.EventRecorderSpy) *ListOptions { return &ListOptions{ IO: ios, Telemetry: spy, @@ -235,7 +236,7 @@ func TestListRun(t *testing.T) { }, { name: "custom directory must exist", - opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.CommandRecorderSpy) *ListOptions { + opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.EventRecorderSpy) *ListOptions { return &ListOptions{ IO: ios, Telemetry: spy, @@ -263,7 +264,7 @@ func TestListRun(t *testing.T) { Body `)) }, - opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.CommandRecorderSpy) *ListOptions { + opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.EventRecorderSpy) *ListOptions { return &ListOptions{ IO: ios, Telemetry: spy, @@ -278,7 +279,7 @@ func TestListRun(t *testing.T) { setup: func(t *testing.T, repoDir, homeDir string) { writeSkill(t, repoDir, "skills/openclaw-helper", remoteSkillFrontmatter("openclaw-helper", "skills/openclaw-helper", "refs/heads/main", "")) }, - opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.CommandRecorderSpy) *ListOptions { + opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.EventRecorderSpy) *ListOptions { return &ListOptions{ IO: ios, Telemetry: spy, @@ -294,7 +295,7 @@ func TestListRun(t *testing.T) { setup: func(t *testing.T, repoDir, homeDir string) { writeSkill(t, repoDir, ".agents/skills/xlsx-pro", remoteSkillFrontmatter("xlsx-pro", "skills/bob/xlsx-pro", "refs/heads/main", "")) }, - opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.CommandRecorderSpy) *ListOptions { + opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.EventRecorderSpy) *ListOptions { return &ListOptions{ IO: ios, Telemetry: spy, @@ -310,7 +311,7 @@ func TestListRun(t *testing.T) { setup: func(t *testing.T, repoDir, homeDir string) { writeSkill(t, repoDir, ".agents/skills/foo", remoteSkillFrontmatter("foo", "plugins/myplugin/skills/foo", "refs/heads/main", "")) }, - opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.CommandRecorderSpy) *ListOptions { + opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.EventRecorderSpy) *ListOptions { return &ListOptions{ IO: ios, Telemetry: spy, @@ -333,7 +334,7 @@ func TestListRun(t *testing.T) { Body `)) }, - opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.CommandRecorderSpy) *ListOptions { + opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.EventRecorderSpy) *ListOptions { exporter := cmdutil.NewJSONExporter() exporter.SetFields([]string{"skillName", "sourceURL", "version", "pinned"}) return &ListOptions{ @@ -356,7 +357,7 @@ func TestListRun(t *testing.T) { }, { name: "no installed skills returns no results", - opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.CommandRecorderSpy) *ListOptions { + opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.EventRecorderSpy) *ListOptions { return &ListOptions{ IO: ios, Telemetry: spy, @@ -369,7 +370,7 @@ func TestListRun(t *testing.T) { }, { name: "no installed skills with json returns empty array", - opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.CommandRecorderSpy) *ListOptions { + opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.EventRecorderSpy) *ListOptions { exporter := cmdutil.NewJSONExporter() exporter.SetFields([]string{"skillName"}) return &ListOptions{ @@ -393,7 +394,7 @@ func TestListRun(t *testing.T) { require.NoError(t, os.WriteFile(target, []byte("---\nname: linked\nmetadata:\n local-path: /src/linked\n---\nBody\n"), 0o644)) require.NoError(t, os.Symlink(target, filepath.Join(skillDir, "SKILL.md"))) }, - opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.CommandRecorderSpy) *ListOptions { + opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.EventRecorderSpy) *ListOptions { return &ListOptions{ IO: ios, Telemetry: spy, @@ -413,7 +414,7 @@ func TestListRun(t *testing.T) { require.NoError(t, os.MkdirAll(targetDir, 0o755)) require.NoError(t, os.Symlink(targetDir, filepath.Join(skillDir, "SKILL.md"))) }, - opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.CommandRecorderSpy) *ListOptions { + opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.EventRecorderSpy) *ListOptions { return &ListOptions{ IO: ios, Telemetry: spy, @@ -437,7 +438,7 @@ func TestListRun(t *testing.T) { Body `)) }, - opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.CommandRecorderSpy) *ListOptions { + opts: func(ios *iostreams.IOStreams, repoDir, homeDir string, spy *telemetry.EventRecorderSpy) *ListOptions { return &ListOptions{ IO: ios, Telemetry: spy, @@ -462,7 +463,7 @@ func TestListRun(t *testing.T) { ios, _, stdout, _ := iostreams.Test() ios.SetStdoutTTY(false) - spy := &telemetry.CommandRecorderSpy{} + spy := &telemetry.EventRecorderSpy{} opts := tt.opts(ios, repoDir, homeDir, spy) err := listRun(opts) diff --git a/pkg/cmd/skills/preview/preview.go b/pkg/cmd/skills/preview/preview.go index 500af05924e..85b88db7564 100644 --- a/pkg/cmd/skills/preview/preview.go +++ b/pkg/cmd/skills/preview/preview.go @@ -41,7 +41,7 @@ type PreviewOptions struct { } // NewCmdPreview creates the "skills preview" command. -func NewCmdPreview(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF func(*PreviewOptions) error) *cobra.Command { +func NewCmdPreview(f *cmdutil.Factory, telemetry ghtelemetry.EventRecorder, runF func(*PreviewOptions) error) *cobra.Command { opts := &PreviewOptions{ IO: f.IOStreams, Telemetry: telemetry, diff --git a/pkg/cmd/skills/preview/preview_test.go b/pkg/cmd/skills/preview/preview_test.go index 4b74b0622e0..ec73168ecca 100644 --- a/pkg/cmd/skills/preview/preview_test.go +++ b/pkg/cmd/skills/preview/preview_test.go @@ -84,7 +84,7 @@ func TestNewCmdPreview(t *testing.T) { } var gotOpts *PreviewOptions - cmd := NewCmdPreview(f, &telemetry.NoOpService{}, func(opts *PreviewOptions) error { + cmd := NewCmdPreview(f, &telemetry.EventRecorderSpy{}, func(opts *PreviewOptions) error { gotOpts = opts return nil }) @@ -976,8 +976,9 @@ func TestPreviewRun_InteractiveTelemetryCapturesSelectedSkillName(t *testing.T) require.NoError(t, err) // Verify the telemetry event captured the interactively-selected skill name, not empty string - require.Len(t, recorder.Events, 1) - event := recorder.Events[0] + events := recorder.Events() + require.Len(t, events, 1) + event := events[0] assert.Equal(t, "skill_preview", event.Type) assert.Equal(t, "beta", event.Dimensions["skill_name"], "telemetry should capture the selected skill name, not the empty opts.SkillName") } @@ -1086,8 +1087,9 @@ func TestPreviewRun_TelemetryVisibility(t *testing.T) { err := previewRun(opts) require.NoError(t, err) - require.Len(t, recorder.Events, 1) - event := recorder.Events[0] + events := recorder.Events() + require.Len(t, events, 1) + event := events[0] assert.Equal(t, "skill_preview", event.Type) // skill_host_type is always recorded (categorized, no raw hostname for enterprise/tenancy). diff --git a/pkg/cmd/skills/search/search.go b/pkg/cmd/skills/search/search.go index 30b873a2123..aeb025ec521 100644 --- a/pkg/cmd/skills/search/search.go +++ b/pkg/cmd/skills/search/search.go @@ -65,7 +65,7 @@ type SearchOptions struct { } // NewCmdSearch creates the "skills search" command. -func NewCmdSearch(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, runF func(*SearchOptions) error) *cobra.Command { +func NewCmdSearch(f *cmdutil.Factory, telemetry ghtelemetry.EventRecorder, runF func(*SearchOptions) error) *cobra.Command { opts := &SearchOptions{ IO: f.IOStreams, Telemetry: telemetry, diff --git a/pkg/cmd/skills/search/search_test.go b/pkg/cmd/skills/search/search_test.go index 2b412f037ba..d2983f5da7a 100644 --- a/pkg/cmd/skills/search/search_test.go +++ b/pkg/cmd/skills/search/search_test.go @@ -104,7 +104,7 @@ func TestNewCmdSearch(t *testing.T) { t.Run(tt.name, func(t *testing.T) { f := &cmdutil.Factory{} var gotOpts *SearchOptions - cmd := NewCmdSearch(f, &telemetry.NoOpService{}, func(opts *SearchOptions) error { + cmd := NewCmdSearch(f, &telemetry.EventRecorderSpy{}, func(opts *SearchOptions) error { gotOpts = opts return nil }) @@ -636,9 +636,10 @@ func TestSearchRun_TelemetryRecordsInstallFromResults(t *testing.T) { // The search command no longer records a separate skill_search event; // only the follow-up skill_search_install event fires when the user // proceeds to install from the results. - require.Len(t, recorder.Events, 1) + events := recorder.Events() + require.Len(t, events, 1) - installEvent := recorder.Events[0] + installEvent := events[0] assert.Equal(t, "skill_search_install", installEvent.Type, "an install triggered from search results should be recorded as a distinct event") assert.Equal(t, int64(1), installEvent.Measures["install_count"], diff --git a/pkg/cmd/skills/skills.go b/pkg/cmd/skills/skills.go index 1399d049b73..0a9d493a97a 100644 --- a/pkg/cmd/skills/skills.go +++ b/pkg/cmd/skills/skills.go @@ -14,7 +14,7 @@ import ( ) // NewCmdSkills returns the top-level "skill" command. -func NewCmdSkills(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder) *cobra.Command { +func NewCmdSkills(f *cmdutil.Factory, telemetry ghtelemetry.InvocationRecorder) *cobra.Command { cmd := &cobra.Command{ Use: "skill ", Short: "Install and manage agent skills (preview)", diff --git a/pkg/cmd/skills/skills_test.go b/pkg/cmd/skills/skills_test.go index eb8bb465c0e..30f7eb3572b 100644 --- a/pkg/cmd/skills/skills_test.go +++ b/pkg/cmd/skills/skills_test.go @@ -11,7 +11,7 @@ import ( ) func TestSkillCommandsAreSampledAt100(t *testing.T) { - spy := &telemetry.CommandRecorderSpy{} + spy := &telemetry.InvocationRecorderSpy{} factory := &cmdutil.Factory{} cmd := skills.NewCmdSkills(factory, spy) cmd.PersistentPreRunE(nil, []string{}) diff --git a/pkg/cmdutil/telemetry.go b/pkg/cmdutil/telemetry.go index 42169beecec..6bbdcabdc7c 100644 --- a/pkg/cmdutil/telemetry.go +++ b/pkg/cmdutil/telemetry.go @@ -9,6 +9,7 @@ import ( "github.com/spf13/pflag" ) +// RecordTelemetry instruments a command with an invocation-owned telemetry event. func RecordTelemetry(cmd *cobra.Command, telemetry ghtelemetry.EventRecorder) { if isTelemetryDisabled(cmd) { return @@ -18,28 +19,41 @@ func RecordTelemetry(cmd *cobra.Command, telemetry ghtelemetry.EventRecorder) { return } - currentRunE := cmd.RunE - cmd.RunE = func(cmd *cobra.Command, args []string) error { - runErr := currentRunE(cmd, args) - - var flags []string - cmd.Flags().Visit(func(f *pflag.Flag) { - flags = append(flags, f.Name) - }) - slices.Sort(flags) - - telemetry.Record(ghtelemetry.Event{ + var event ghtelemetry.PendingEvent + currentArgs := cmd.Args + cmd.Args = func(cmd *cobra.Command, args []string) error { + event = telemetry.Begin(ghtelemetry.Event{ Type: "command_invocation", - Dimensions: map[string]string{ + Dimensions: ghtelemetry.Dimensions{ "command": cmd.CommandPath(), - "flags": strings.Join(flags, ","), + "flags": telemetryFlags(cmd), }, }) + if currentArgs != nil { + return currentArgs(cmd, args) + } + return nil + } + currentRunE := cmd.RunE + cmd.RunE = func(cmd *cobra.Command, args []string) error { + runErr := currentRunE(cmd, args) + // Commands with DisableFlagParsing may parse their flags inside RunE. + event.UpsertDimensions(ghtelemetry.Dimensions{"flags": telemetryFlags(cmd)}) return runErr } } +func telemetryFlags(cmd *cobra.Command) string { + var flags []string + cmd.Flags().Visit(func(f *pflag.Flag) { + flags = append(flags, f.Name) + }) + slices.Sort(flags) + return strings.Join(flags, ",") +} + +// RecordTelemetryForSubcommands instruments all descendants of a command. func RecordTelemetryForSubcommands(cmd *cobra.Command, telemetry ghtelemetry.EventRecorder) { for _, c := range cmd.Commands() { RecordTelemetry(c, telemetry) diff --git a/pkg/cmdutil/telemetry_test.go b/pkg/cmdutil/telemetry_test.go index bfe4c420ca0..f0789c756af 100644 --- a/pkg/cmdutil/telemetry_test.go +++ b/pkg/cmdutil/telemetry_test.go @@ -1,9 +1,11 @@ package cmdutil_test import ( + "bytes" "fmt" "testing" + "github.com/cli/cli/v2/internal/gh/ghtelemetry" "github.com/cli/cli/v2/internal/telemetry" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/spf13/cobra" @@ -12,157 +14,254 @@ import ( ) func TestRecordTelemetry(t *testing.T) { - t.Run("records command path and flags", func(t *testing.T) { + t.Run("records commands that fail argument validation", func(t *testing.T) { + t.Parallel() + + // Given a command that requires a positional argument recorder := &telemetry.EventRecorderSpy{} cmd := &cobra.Command{ - Use: "list", - RunE: func(cmd *cobra.Command, args []string) error { return nil }, + Use: "list", + Args: cobra.ExactArgs(1), + SilenceErrors: true, + SilenceUsage: true, + RunE: func(cmd *cobra.Command, args []string) error { return nil }, } cmd.Flags().Bool("web", false, "") - cmd.Flags().String("repo", "", "") - - parent := &cobra.Command{Use: "pr"} - root := &cobra.Command{Use: "gh"} - root.AddCommand(parent) - parent.AddCommand(cmd) - + cmd.SetArgs([]string{"--web"}) cmdutil.RecordTelemetry(cmd, recorder) - require.NoError(t, cmd.Flags().Set("web", "true")) - require.NoError(t, cmd.Flags().Set("repo", "cli/cli")) - require.NoError(t, cmd.RunE(cmd, nil)) + // When Cobra rejects its arguments + _, err := cmd.ExecuteC() - require.Len(t, recorder.Events, 1) - event := recorder.Events[0] - assert.Equal(t, "command_invocation", event.Type) - assert.Equal(t, "gh pr list", event.Dimensions["command"]) - assert.Equal(t, "repo,web", event.Dimensions["flags"]) + // Then the failure is preserved and the command is still recorded + require.EqualError(t, err, "accepts 1 arg(s), received 0") + events := recorder.Events() + require.Len(t, events, 1) + assert.Equal(t, "command_invocation", events[0].Type) + assert.Equal(t, "list", events[0].Dimensions["command"]) + assert.Equal(t, "web", events[0].Dimensions["flags"]) }) + tests := []struct { + name string + args []string + flags string + }{ + { + name: "records sorted flag names without argument or flag values", + args: []string{"--web", "--repo", "private/repository", "private argument"}, + flags: "repo,web", + }, + { + name: "accepts positional arguments with nil Args and records empty flags", + args: []string{"private argument"}, + flags: "", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + // Given a command without argument validation + recorder := &telemetry.EventRecorderSpy{} + cmd := &cobra.Command{ + Use: "list", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + cmd.Flags().Bool("web", false, "") + cmd.Flags().String("repo", "", "") + cmd.Flags().Bool("unused", false, "") + parent := &cobra.Command{Use: "pr"} + root := &cobra.Command{Use: "gh"} + root.AddCommand(parent) + parent.AddCommand(cmd) + root.SetArgs(append([]string{"pr", "list"}, tt.args...)) + cmdutil.RecordTelemetry(cmd, recorder) + + // When Cobra executes the command + _, err := root.ExecuteC() + + // Then only the command path and explicitly supplied flag names are recorded + require.NoError(t, err) + events := recorder.Events() + require.Len(t, events, 1) + assert.Equal(t, "command_invocation", events[0].Type) + assert.Equal(t, ghtelemetry.Dimensions{ + "command": "gh pr list", + "flags": tt.flags, + }, events[0].Dimensions) + }) + } + t.Run("is a no-op when original RunE is nil", func(t *testing.T) { - recorder := &telemetry.EventRecorderSpy{} - cmd := &cobra.Command{Use: "test"} + t.Parallel() + // Given a command using Run instead of RunE + recorder := &telemetry.EventRecorderSpy{} + output := &bytes.Buffer{} + cmd := &cobra.Command{ + Use: "test", + Run: func(cmd *cobra.Command, args []string) { + fmt.Fprintln(cmd.OutOrStdout(), "command output") + }, + } + cmd.SetOut(output) + cmd.SetArgs([]string{}) cmdutil.RecordTelemetry(cmd, recorder) - assert.Nil(t, cmd.RunE, "RunE should remain nil when it was nil before") - assert.Empty(t, recorder.Events, "no telemetry should be recorded") + // When Cobra executes the command + _, err := cmd.ExecuteC() + + // Then its original behavior is retained without telemetry + require.NoError(t, err) + assert.Equal(t, "command output\n", output.String()) + assert.Empty(t, recorder.Events()) }) - t.Run("propagates error from original RunE", func(t *testing.T) { + t.Run("records flags parsed during RunE even when execution fails", func(t *testing.T) { + t.Parallel() + + // Given a command that manually parses flags before execution fails recorder := &telemetry.EventRecorderSpy{} expectedErr := fmt.Errorf("something went wrong") cmd := &cobra.Command{ - Use: "fail", - RunE: func(cmd *cobra.Command, args []string) error { return expectedErr }, + Use: "copilot", + DisableFlagParsing: true, + SilenceErrors: true, + SilenceUsage: true, + RunE: func(cmd *cobra.Command, args []string) error { + cmd.DisableFlagParsing = false + if err := cmd.ParseFlags(args); err != nil { + return err + } + return expectedErr + }, } - + cmd.Flags().Bool("remove", false, "") + cmd.SetArgs([]string{"--remove"}) cmdutil.RecordTelemetry(cmd, recorder) - err := cmd.RunE(cmd, nil) - assert.ErrorIs(t, err, expectedErr) - // Telemetry is still recorded even on error - require.Len(t, recorder.Events, 1) - assert.Equal(t, "command_invocation", recorder.Events[0].Type) + // When Cobra executes the command + _, err := cmd.ExecuteC() + + // Then the error is preserved and the late-parsed flag is recorded + require.ErrorIs(t, err, expectedErr) + events := recorder.Events() + require.Len(t, events, 1) + assert.Equal(t, "command_invocation", events[0].Type) + assert.Equal(t, "copilot", events[0].Dimensions["command"]) + assert.Equal(t, "remove", events[0].Dimensions["flags"]) }) - t.Run("flags are sorted alphabetically", func(t *testing.T) { + t.Run("records commands rejected by a parent persistent pre-run", func(t *testing.T) { + t.Parallel() + + // Given a command whose parent rejects execution before RunE recorder := &telemetry.EventRecorderSpy{} + expectedErr := fmt.Errorf("authentication required") + root := &cobra.Command{ + Use: "gh", + SilenceErrors: true, + SilenceUsage: true, + PersistentPreRunE: func(cmd *cobra.Command, args []string) error { return expectedErr }, + } cmd := &cobra.Command{ - Use: "test", + Use: "list", RunE: func(cmd *cobra.Command, args []string) error { return nil }, } - cmd.Flags().Bool("zebra", false, "") - cmd.Flags().Bool("alpha", false, "") - cmd.Flags().Bool("middle", false, "") - + cmd.Flags().Bool("web", false, "") + root.AddCommand(cmd) + root.SetArgs([]string{"list", "--web"}) cmdutil.RecordTelemetry(cmd, recorder) - require.NoError(t, cmd.Flags().Set("zebra", "true")) - require.NoError(t, cmd.Flags().Set("alpha", "true")) - require.NoError(t, cmd.Flags().Set("middle", "true")) - require.NoError(t, cmd.RunE(cmd, nil)) + // When Cobra rejects the command + _, err := root.ExecuteC() - require.Len(t, recorder.Events, 1) - assert.Equal(t, "alpha,middle,zebra", recorder.Events[0].Dimensions["flags"]) + // Then the parent error is preserved and the attempted command is recorded + require.ErrorIs(t, err, expectedErr) + events := recorder.Events() + require.Len(t, events, 1) + assert.Equal(t, "command_invocation", events[0].Type) + assert.Equal(t, "gh list", events[0].Dimensions["command"]) + assert.Equal(t, "web", events[0].Dimensions["flags"]) }) - t.Run("no flags set records empty flags string", func(t *testing.T) { + t.Run("records commands rejected by their pre-run", func(t *testing.T) { + t.Parallel() + + // Given a command whose pre-run validation fails recorder := &telemetry.EventRecorderSpy{} + expectedErr := fmt.Errorf("incompatible flags") cmd := &cobra.Command{ - Use: "test", - RunE: func(cmd *cobra.Command, args []string) error { return nil }, + Use: "list", + SilenceErrors: true, + SilenceUsage: true, + PreRunE: func(cmd *cobra.Command, args []string) error { return expectedErr }, + RunE: func(cmd *cobra.Command, args []string) error { return nil }, } - cmd.Flags().Bool("unused", false, "") - + cmd.SetArgs([]string{}) cmdutil.RecordTelemetry(cmd, recorder) - require.NoError(t, cmd.RunE(cmd, nil)) - require.Len(t, recorder.Events, 1) - assert.Equal(t, "", recorder.Events[0].Dimensions["flags"]) + // When Cobra rejects the command + _, err := cmd.ExecuteC() + + // Then the validation error is preserved and the attempted command is recorded + require.ErrorIs(t, err, expectedErr) + events := recorder.Events() + require.Len(t, events, 1) + assert.Equal(t, "command_invocation", events[0].Type) + assert.Equal(t, "list", events[0].Dimensions["command"]) }) t.Run("skips commands with telemetry disabled", func(t *testing.T) { + t.Parallel() + + // Given a command with telemetry disabled recorder := &telemetry.EventRecorderSpy{} cmd := &cobra.Command{ Use: "internal", RunE: func(cmd *cobra.Command, args []string) error { return nil }, } + cmd.SetArgs([]string{}) cmdutil.DisableTelemetry(cmd) cmdutil.RecordTelemetry(cmd, recorder) - require.NoError(t, cmd.RunE(cmd, nil)) - assert.Empty(t, recorder.Events, "telemetry should not be recorded for disabled commands") + // When Cobra executes the command + _, err := cmd.ExecuteC() + + // Then the command succeeds without recording telemetry + require.NoError(t, err) + assert.Empty(t, recorder.Events(), "telemetry should not be recorded for disabled commands") }) } func TestRecordTelemetryForSubcommands(t *testing.T) { - t.Run("instruments nested subcommands", func(t *testing.T) { - recorder := &telemetry.EventRecorderSpy{} - - root := &cobra.Command{Use: "gh"} - parent := &cobra.Command{Use: "pr"} - child := &cobra.Command{ - Use: "list", - RunE: func(cmd *cobra.Command, args []string) error { return nil }, - } - root.AddCommand(parent) - parent.AddCommand(child) - - cmdutil.RecordTelemetryForSubcommands(root, recorder) - require.NoError(t, child.RunE(child, nil)) - - require.Len(t, recorder.Events, 1) - assert.Equal(t, "command_invocation", recorder.Events[0].Type) - assert.Equal(t, "gh pr list", recorder.Events[0].Dimensions["command"]) - }) - - t.Run("skips subcommands with nil RunE", func(t *testing.T) { - recorder := &telemetry.EventRecorderSpy{} - - root := &cobra.Command{Use: "gh"} - child := &cobra.Command{Use: "help"} // no RunE - root.AddCommand(child) - - cmdutil.RecordTelemetryForSubcommands(root, recorder) - - assert.Nil(t, child.RunE, "nil RunE should remain nil") - }) - - t.Run("skips subcommands with telemetry disabled", func(t *testing.T) { - recorder := &telemetry.EventRecorderSpy{} - - root := &cobra.Command{Use: "gh"} - child := &cobra.Command{ - Use: "send-telemetry", - RunE: func(cmd *cobra.Command, args []string) error { return nil }, - } - cmdutil.DisableTelemetry(child) - root.AddCommand(child) - - cmdutil.RecordTelemetryForSubcommands(root, recorder) - require.NoError(t, child.RunE(child, nil)) - - assert.Empty(t, recorder.Events, "disabled commands should not record telemetry") + t.Parallel() + + // Given a command tree instrumented from its root + recorder := &telemetry.EventRecorderSpy{} + root := &cobra.Command{Use: "gh"} + parent := &cobra.Command{Use: "pr"} + child := &cobra.Command{ + Use: "list", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, + } + root.AddCommand(parent) + root.AddCommand(&cobra.Command{ + Use: "version", + RunE: func(cmd *cobra.Command, args []string) error { return nil }, }) + parent.AddCommand(child) + root.SetArgs([]string{"pr", "list"}) + cmdutil.RecordTelemetryForSubcommands(root, recorder) + + // When Cobra executes a nested command + _, err := root.ExecuteC() + + // Then only the invoked descendant is recorded + require.NoError(t, err) + events := recorder.Events() + require.Len(t, events, 1) + assert.Equal(t, "command_invocation", events[0].Type) + assert.Equal(t, "gh pr list", events[0].Dimensions["command"]) }