diff --git a/.github/skills/vhs-demo/SKILL.md b/.github/skills/vhs-demo/SKILL.md new file mode 100644 index 00000000000..14e4b47678f --- /dev/null +++ b/.github/skills/vhs-demo/SKILL.md @@ -0,0 +1,186 @@ +--- +name: vhs-demo +description: Use when creating a terminal GIF, recording a CLI demonstration, authoring a VHS tape, or producing visual before/after evidence for reviewers. +compatibility: Requires VHS 0.11.0+, ffmpeg, ffprobe, ttyd, and a VHS-supported shell. +--- + +# VHS Demo + +Produce a short, repeatable GIF that shows real commands and unedited output from +caller-selected executables. The GIF is evidence of one observed flow, not proof +of general correctness and not a replacement for tests. + +## Evidence contract + +A finished demo must make these claims defensible: + +- VHS rendered a saved tape; no fallback recorder or `vhs publish` was used. +- Every visible command ran against an identified executable. +- Hidden setup created state but did not create, paste, rewrite, or overlay the + output attributed to the product. +- Configuration, credentials, hosts, and data were isolated and synthetic by + default. +- The native-resolution animation was inspected, including any transient state. + +The caller decides where the artifact is ultimately used. Return local paths and +evidence; do not require an upload, PR edit, or repository change. + +## Workflow + +### 1. Define the claim + +Record the intended audience or destination, the behavior to evidence, exact +visible commands, required initial state, and any size constraint. Storyboard +the visible beats and their approximate duration before writing the tape. + +Prefer one coherent claim per GIF. Use a synthetic supporting service or fixture +only when the real product cannot safely or deterministically reach the required +state; disclose and remove it afterward. The visible command must still run the +selected real executable. Never substitute a fake or wrapper that generates +product output. Split success and error flows when combining them would obscure +either result. + +For before/after evidence, identify the controlled variable. Keep the shell, +terminal geometry, commands, typing speed, synthetic data, and equivalent +application state fixed. + +### 2. Prove the toolchain and executables + +Run the platform preflight from the repository root: + +```sh +.github/skills/vhs-demo/scripts/preflight.sh +``` + +```powershell +& .\.github\skills\vhs-demo\scripts\preflight.ps1 +``` + +If VHS, ffmpeg, ffprobe, or ttyd is missing or cannot execute, or VHS is older +than 0.11.0, name the failure and stop. Do not install tools or substitute +asciinema, screen recording, hosted VHS, or fabricated terminal text. + +The caller chooses every executable, including separate baseline and changed +binaries. Before recording each one, capture its absolute path, version, source +or revision, and preferably SHA-256. Build separate binaries from separate +checkouts; never overwrite one binary or switch the active checkout between +takes. + +Invoke absolute paths in visible commands. If natural spelling such as `gh` is +part of the presentation, create an isolated command-name mapping to that exact +path and put it ahead of the tape's `PATH`. Do not trust aliases or the ordinary +`PATH`. + +### 3. Isolate the take + +Use the platform's secure temporary-directory API to create one workspace per +GIF. Keep the tape, GIF, fixtures, config, command mappings, and inspection +frames there, never in the repository. + +Use synthetic names, repositories, hosts, and content. Remap home and config +roots into the workspace, then clear inherited credential variables and +quarantine every config source the selected product can read. For `gh`, point +`GH_CONFIG_DIR` at an empty private directory and clear `GH_TOKEN`, +`GITHUB_TOKEN`, `GH_ENTERPRISE_TOKEN`, `GITHUB_ENTERPRISE_TOKEN`, `GH_HOST`, and +`GH_REPO`. If the behavior inherently needs a service, use only an approved +synthetic endpoint and account; stop if safe credentials or data are unavailable. + +Select a shell accepted by `Set Shell` in the installed VHS version and use only +that shell's syntax in `Type` commands. An executable being installed does not +mean VHS supports it. VHS 0.11 shell definitions suppress startup profiles and +set a generic prompt; confirm both with a probe before trusting prompt waits or +`PATH`. + +### 4. Author and render the tape + +Adapt this structure, replacing every placeholder: + +```text +Output "/demo.gif" +Require +Set Shell +Set Width +Set Height +Set FontSize 22 +Set FontFamily "" +Set TypingSpeed 35ms +Set CursorBlink false +Set WaitTimeout 120s + +Env GH_CONFIG_DIR "/config" +Env HOME "/home" +Env XDG_CONFIG_HOME "/config" +Env GH_TOKEN "" +Env GITHUB_TOKEN "" +Env GH_ENTERPRISE_TOKEN "" +Env GITHUB_ENTERPRISE_TOKEN "" +Env GH_HOST "" +Env GH_REPO "" +Env NO_COLOR "1" + +Hide +Type "" +Enter +Wait+Line // +Show + +Type " " +Enter +Wait+Screen // +Sleep 2s +``` + +Use hidden commands only for fixtures, isolated config, approved synthetic +services, deterministic prompts, directory changes, and clearing the screen. +Type every visible command and let it run. Use `Wait+Screen` for observable +results and condition-based waits for prompts; use `Sleep` only for pacing. +Let waits capture spinners and other motion rather than sleeping past them. + +Verify the selected font is installed and resolves as monospaced using the +platform font registry or a native-resolution ruler probe. Render a sample +frame, then size width, height, and font for the content. Increase geometry +instead of shrinking or clipping text. + +Run `vhs `. A parse or directive failure after preflight is an +incompatibility: report it rather than deleting a safeguard. + +### 5. Inspect the evidence + +From the repository root, pass the matching helper an inspection directory +outside the repository: + +```sh +.github/skills/vhs-demo/scripts/inspect.sh +``` + +```powershell +& .\.github\skills\vhs-demo\scripts\inspect.ps1 +``` + +The helper rejects repository paths, verifies GIF metadata, and extracts +native-resolution first, final, one-frame-per-second, and every-frame sequences. + +Inspect the images and the tape together. Confirm executable selection, visible +commands, unedited output, intentional wrapping, readable monospaced text, +clean first and final frames, useful pacing, and absence of secrets, private +paths, real hosts, identities, notifications, and hidden setup. Inspect every +frame when checking privacy or any spinner, crash, exit, relaunch, or other +transient claim. Fix unsafe input at its source and rerender. Never redact the +GIF after capture. + +### 6. Return evidence + +Report the absolute tape and GIF paths; dimensions, duration, frame count when +available, and size; visible commands; each executable's path, version, +revision/source, and hash when recorded; whether sampled or all frames were +inspected; the intended use; and any limitation on what the demo proves. + +## Stop shortcuts + +| Shortcut | Required response | +| --- | --- | +| "Use my existing login/config" | Isolate config and use synthetic credentials or stop. | +| "Both clips say `gh`, so selection is obvious" | Map the name to each proven binary explicitly. | +| "Any recorder is fine if VHS is missing" | Stop and report the failed prerequisite. | +| "Put the GIF in the repo for convenience" | Keep all generated media in a temporary workspace. | +| "The final frame proves the spinner" | Inspect the transient frame sequence. | diff --git a/.github/skills/vhs-demo/scripts/inspect.ps1 b/.github/skills/vhs-demo/scripts/inspect.ps1 new file mode 100644 index 00000000000..33eb4266803 --- /dev/null +++ b/.github/skills/vhs-demo/scripts/inspect.ps1 @@ -0,0 +1,123 @@ +param( + [Parameter(Mandatory, Position = 0)] + [string]$Gif, + [Parameter(Mandatory, Position = 1)] + [string]$InspectionDirectory +) + +$ErrorActionPreference = "Stop" + +function Resolve-PhysicalPath { + param( + [Parameter(Mandatory)] + [string]$LiteralPath + ) + + $fullPath = [IO.Path]::GetFullPath($LiteralPath) + $root = [IO.Path]::GetPathRoot($fullPath) + $currentPath = $root + + foreach ($component in $fullPath.Substring($root.Length) -split "[\\/]") { + if ([string]::IsNullOrEmpty($component)) { + continue + } + + $item = Get-Item -LiteralPath ( + Join-Path $currentPath $component + ) -ErrorAction Stop + if (($item.Attributes -band [IO.FileAttributes]::ReparsePoint) -ne 0) { + $resolveLinkTarget = $item.PSObject.Methods["ResolveLinkTarget"] + if ($null -eq $resolveLinkTarget) { + throw "Cannot safely resolve linked path with this PowerShell version: $LiteralPath" + } + $item = $item.ResolveLinkTarget($true) + if ($null -eq $item) { + throw "Failed to resolve linked path: $LiteralPath" + } + } + $currentPath = $item.FullName + } + + return $currentPath +} + +$gifItem = Get-Item -LiteralPath $Gif -ErrorAction Stop +if ($gifItem.Length -eq 0) { + throw "GIF is empty: $Gif" +} +if (($gifItem.Attributes -band [IO.FileAttributes]::ReparsePoint) -ne 0) { + throw "GIF must not be a symbolic link or reparse point: $Gif" +} + +$repoRoot = Resolve-PhysicalPath ( + Join-Path $PSScriptRoot "..\..\..\.." +) +if (Test-Path -LiteralPath $InspectionDirectory) { + throw "Inspection directory already exists: $InspectionDirectory" +} +$inspectionLeaf = Split-Path -Leaf $InspectionDirectory +$inspectionParentInput = Split-Path -Parent $InspectionDirectory +if ([string]::IsNullOrEmpty($inspectionParentInput)) { + $inspectionParentInput = "." +} +$inspectionParent = Resolve-PhysicalPath $inspectionParentInput +$inspectionPath = Join-Path $inspectionParent $inspectionLeaf +$gifPath = Resolve-PhysicalPath $gifItem.FullName +$comparison = if ( + [Environment]::OSVersion.Platform -eq [PlatformID]::Win32NT +) { + [StringComparison]::OrdinalIgnoreCase +} else { + [StringComparison]::Ordinal +} +$repoPrefix = $repoRoot.TrimEnd( + [IO.Path]::DirectorySeparatorChar, + [IO.Path]::AltDirectorySeparatorChar +) + [IO.Path]::DirectorySeparatorChar + +foreach ($candidate in @($gifPath, $inspectionPath)) { + if ($candidate.Equals($repoRoot, $comparison) -or + $candidate.StartsWith($repoPrefix, $comparison)) { + throw "Generated media must be outside the repository: $candidate" + } +} + +New-Item -ItemType Directory -Path $inspectionPath | Out-Null + +$probe = & ffprobe -v error -select_streams v:0 ` + -show_entries "stream=codec_name,width,height,nb_frames:format=format_name,duration,size" ` + -of "default=noprint_wrappers=1" $gifItem.FullName +if ($LASTEXITCODE -ne 0) { + throw "ffprobe failed for: $Gif" +} +$probeText = $probe -join [Environment]::NewLine +Write-Output $probeText +if ($probe -notcontains "codec_name=gif") { + throw "Expected GIF video codec: $Gif" +} +if ($probe -notcontains "format_name=gif") { + throw "Expected GIF container: $Gif" +} + +$sampled = Join-Path $inspectionPath "sampled" +New-Item -ItemType Directory -Force -Path $sampled | Out-Null + +& ffmpeg -v error -y -i $gifItem.FullName -frames:v 1 ` + (Join-Path $inspectionPath "first.png") +if ($LASTEXITCODE -ne 0) { throw "Failed to extract first frame" } + +& ffmpeg -v error -y -i $gifItem.FullName -update 1 ` + (Join-Path $inspectionPath "final.png") +if ($LASTEXITCODE -ne 0) { throw "Failed to extract final frame" } + +& ffmpeg -v error -y -i $gifItem.FullName -vf "fps=1" ` + (Join-Path $sampled "frame-%04d.png") +if ($LASTEXITCODE -ne 0) { throw "Failed to extract sampled frames" } + +$all = Join-Path $inspectionPath "all" +New-Item -ItemType Directory -Force -Path $all | Out-Null +& ffmpeg -v error -y -i $gifItem.FullName ` + (Join-Path $all "frame-%06d.png") +if ($LASTEXITCODE -ne 0) { throw "Failed to extract all frames" } + +Write-Output "Inspection frames: $inspectionPath" diff --git a/.github/skills/vhs-demo/scripts/inspect.sh b/.github/skills/vhs-demo/scripts/inspect.sh new file mode 100755 index 00000000000..6bb52be4639 --- /dev/null +++ b/.github/skills/vhs-demo/scripts/inspect.sh @@ -0,0 +1,73 @@ +#!/bin/sh + +set -eu + +usage() { + printf 'usage: %s \n' "$0" >&2 + exit 2 +} + +[ "$#" -eq 2 ] || usage +gif=$1 +inspection_dir=$2 + +[ -s "$gif" ] || { + printf 'GIF does not exist or is empty: %s\n' "$gif" >&2 + exit 1 +} +[ ! -L "$gif" ] || { + printf 'GIF must not be a symbolic link: %s\n' "$gif" >&2 + exit 1 +} + +script_dir=$(CDPATH= cd -P "$(dirname "$0")" && pwd) +repo_root=$(CDPATH= cd -P "$script_dir/../../../.." && pwd) +gif_dir=$(CDPATH= cd -P "$(dirname "$gif")" && pwd) +[ ! -e "$inspection_dir" ] || { + printf 'Inspection directory already exists: %s\n' "$inspection_dir" >&2 + exit 1 +} +inspection_parent=$(CDPATH= cd -P "$(dirname "$inspection_dir")" && pwd) || { + printf 'Inspection parent directory does not exist: %s\n' "$(dirname "$inspection_dir")" >&2 + exit 1 +} +inspection_dir="$inspection_parent/$(basename "$inspection_dir")" + +case "$gif_dir/" in + "$repo_root/"*) + printf 'GIF must be outside the repository: %s\n' "$gif" >&2 + exit 1 + ;; +esac +case "$inspection_dir/" in + "$repo_root/"*) + printf 'Inspection directory must be outside the repository: %s\n' "$inspection_dir" >&2 + exit 1 + ;; +esac + +mkdir "$inspection_dir" + +probe=$(ffprobe -v error -select_streams v:0 \ + -show_entries stream=codec_name,width,height,nb_frames:format=format_name,duration,size \ + -of default=noprint_wrappers=1 "$gif") +printf '%s\n' "$probe" +printf '%s\n' "$probe" | grep -Eq '^codec_name=gif$' || { + printf 'Expected GIF video codec: %s\n' "$gif" >&2 + exit 1 +} +printf '%s\n' "$probe" | grep -Eq '^format_name=gif$' || { + printf 'Expected GIF container: %s\n' "$gif" >&2 + exit 1 +} + +mkdir -p "$inspection_dir/sampled" +ffmpeg -v error -y -i "$gif" -frames:v 1 "$inspection_dir/first.png" +ffmpeg -v error -y -i "$gif" -update 1 "$inspection_dir/final.png" +ffmpeg -v error -y -i "$gif" -vf fps=1 \ + "$inspection_dir/sampled/frame-%04d.png" + +mkdir -p "$inspection_dir/all" +ffmpeg -v error -y -i "$gif" "$inspection_dir/all/frame-%06d.png" + +printf 'Inspection frames: %s\n' "$inspection_dir" diff --git a/.github/skills/vhs-demo/scripts/preflight.ps1 b/.github/skills/vhs-demo/scripts/preflight.ps1 new file mode 100644 index 00000000000..519209caa86 --- /dev/null +++ b/.github/skills/vhs-demo/scripts/preflight.ps1 @@ -0,0 +1,42 @@ +$ErrorActionPreference = "Stop" + +function Invoke-VersionProbe { + param( + [Parameter(Mandatory)] + [string]$Name, + [Parameter(Mandatory)] + [string[]]$Arguments + ) + + $command = Get-Command $Name -CommandType Application -ErrorAction SilentlyContinue + if ($null -eq $command) { + throw "VHS demo preflight failed: missing required tool: $Name" + } + + $output = (& $command.Path @Arguments 2>&1 | Out-String).Trim() + if ($LASTEXITCODE -ne 0) { + throw "VHS demo preflight failed: $Name exists at $($command.Path) but could not execute" + } + if ([string]::IsNullOrWhiteSpace($output)) { + throw "VHS demo preflight failed: $Name returned no version information" + } + + $firstLine = ($output -split "\r?\n")[0] + Write-Host "${Name}: $($command.Path) ($firstLine)" + return $output +} + +$vhsOutput = Invoke-VersionProbe -Name "vhs" -Arguments @("--version") +$match = [regex]::Match($vhsOutput, 'v?(\d+\.\d+(?:\.\d+)?)') +if (-not $match.Success) { + throw "VHS demo preflight failed: unable to parse VHS version from: $vhsOutput" +} +if ([version]$match.Groups[1].Value -lt [version]"0.11.0") { + throw "VHS demo preflight failed: VHS 0.11.0 or newer is required; found $($match.Groups[1].Value)" +} + +Invoke-VersionProbe -Name "ffmpeg" -Arguments @("-version") | Out-Null +Invoke-VersionProbe -Name "ffprobe" -Arguments @("-version") | Out-Null +Invoke-VersionProbe -Name "ttyd" -Arguments @("--version") | Out-Null + +Write-Output "VHS demo prerequisites satisfied." diff --git a/.github/skills/vhs-demo/scripts/preflight.sh b/.github/skills/vhs-demo/scripts/preflight.sh new file mode 100755 index 00000000000..2b3b87bd938 --- /dev/null +++ b/.github/skills/vhs-demo/scripts/preflight.sh @@ -0,0 +1,40 @@ +#!/bin/sh + +set -eu + +fail() { + printf 'VHS demo preflight failed: %s\n' "$*" >&2 + exit 1 +} + +probe() { + tool=$1 + shift + + path=$(command -v "$tool" 2>/dev/null) || + fail "missing required tool: $tool" + output=$("$path" "$@" 2>&1) || + fail "$tool exists at $path but could not execute" + first_line=$(printf '%s\n' "$output" | sed -n '1p') + [ -n "$first_line" ] || + fail "$tool returned no version information" + printf '%s: %s (%s)\n' "$tool" "$path" "$first_line" + + if [ "$tool" = vhs ]; then + vhs_version=$(printf '%s\n' "$output" | + sed -nE 's/^[^0-9]*v?([0-9]+\.[0-9]+(\.[0-9]+)?).*/\1/p' | + sed -n '1p') + [ -n "$vhs_version" ] || + fail "unable to parse VHS version from: $first_line" + printf '%s\n' "$vhs_version" | + awk -F. '$1 > 0 || ($1 == 0 && $2 >= 11) { ok=1 } END { exit !ok }' || + fail "VHS 0.11.0 or newer is required; found $vhs_version" + fi +} + +probe vhs --version +probe ffmpeg -version +probe ffprobe -version +probe ttyd --version + +printf 'VHS demo prerequisites satisfied.\n' diff --git a/AGENTS.md b/AGENTS.md index ae04c795a30..b3ec1a157e8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,8 +17,9 @@ go test -tags acceptance ./acceptance # Acceptance tests make lint # golangci-lint (same as CI) ``` -**Before committing, ensure both tests and linter pass:** +**Before committing, run these quality gates:** ```bash +go fix ./pkg/cmd/issue/list/... # Replace with the changed package paths go test ./... make lint ``` diff --git a/internal/config/config.go b/internal/config/config.go index 6467fe77d73..6f36c7208cf 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -22,6 +22,7 @@ const ( accessiblePrompterKey = "accessible_prompter" aliasesKey = "aliases" browserKey = "browser" // used by cli/go-gh to open URLs in web browsers + clipboardKey = "clipboard" colorLabelsKey = "color_labels" apiHostKey = "api_host" // used by cli/go-gh to redirect API requests for a host editorKey = "editor" // used by cli/go-gh to open interactive text editor @@ -132,6 +133,11 @@ func (c *cfg) Browser(hostname string) gh.ConfigEntry { return c.GetOrDefault(hostname, browserKey).Unwrap() } +func (c *cfg) Clipboard() gh.ConfigEntry { + // Intentionally panic if there is no user provided value or default value (which would be a programmer error) + return c.GetOrDefault("", clipboardKey).Unwrap() +} + func (c *cfg) ColorLabels(hostname string) gh.ConfigEntry { // Intentionally panic if there is no user provided value or default value (which would be a programmer error) return c.GetOrDefault(hostname, colorLabelsKey).Unwrap() @@ -631,6 +637,8 @@ aliases: http_unix_socket: # What web browser gh should use when opening URLs. If blank, will refer to environment. browser: +# Whether to copy one-time OAuth device codes to the clipboard. Supported values: enabled, disabled +clipboard: enabled # Whether to display labels using their RGB hex color codes in terminals that support truecolor. Supported values: enabled, disabled color_labels: disabled # Whether customizable, 4-bit accessible colors should be used. Supported values: enabled, disabled @@ -641,12 +649,25 @@ accessible_prompter: disabled spinner: enabled ` +// ConfigScope controls whether a configuration option can be set globally, per host, or both. +type ConfigScope int + +const ( + // ConfigScopeGlobalOrHost allows an option to be set globally or for a specific host. + ConfigScopeGlobalOrHost ConfigScope = iota + // ConfigScopeGlobalOnly allows an option to be set only at the global level. + ConfigScopeGlobalOnly + // ConfigScopeHostOnly requires an option to be set for a specific host. + ConfigScopeHostOnly +) + +// ConfigOption describes a supported configuration key. type ConfigOption struct { Key string Description string DefaultValue string AllowedValues []string - PerHostOnly bool + Scope ConfigScope CurrentValue func(c gh.Config, hostname string) string } @@ -655,7 +676,7 @@ var Options = []ConfigOption{ Key: apiHostKey, Description: "experimental: the hostname to use when making API requests for a GitHub host. Note: this is not a security boundary and requests to the canonical host will remain authenticated", DefaultValue: "", - PerHostOnly: true, + Scope: ConfigScopeHostOnly, CurrentValue: func(c gh.Config, hostname string) string { apiHost, _ := c.Authentication().APIHostForHost(hostname) return apiHost @@ -720,6 +741,16 @@ var Options = []ConfigOption{ return c.Browser(hostname).Value }, }, + { + Key: clipboardKey, + Description: "whether to copy one-time OAuth device codes to the clipboard", + DefaultValue: "enabled", + AllowedValues: []string{"enabled", "disabled"}, + Scope: ConfigScopeGlobalOnly, + CurrentValue: func(c gh.Config, hostname string) string { + return c.Clipboard().Value + }, + }, { Key: colorLabelsKey, Description: "whether to display labels using their RGB hex color codes in terminals that support truecolor", diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 57cca23740f..b870ae905dc 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -32,6 +32,7 @@ func TestNewConfigProvidesFallback(t *testing.T) { requireKeyWithValue(t, spiedCfg, []string{aliasesKey, "co"}, "pr checkout") requireKeyWithValue(t, spiedCfg, []string{httpUnixSocketKey}, "") requireKeyWithValue(t, spiedCfg, []string{browserKey}, "") + requireKeyWithValue(t, spiedCfg, []string{clipboardKey}, "enabled") requireKeyWithValue(t, spiedCfg, []string{colorLabelsKey}, "disabled") } @@ -46,6 +47,7 @@ func TestGetOrDefaultApplicationDefaults(t *testing.T) { {pagerKey, ""}, {httpUnixSocketKey, ""}, {browserKey, ""}, + {clipboardKey, "enabled"}, } for _, tt := range tests { @@ -138,6 +140,7 @@ func TestFallbackConfig(t *testing.T) { requireKeyWithValue(t, cfg, []string{aliasesKey, "co"}, "pr checkout") requireKeyWithValue(t, cfg, []string{httpUnixSocketKey}, "") requireKeyWithValue(t, cfg, []string{browserKey}, "") + requireKeyWithValue(t, cfg, []string{clipboardKey}, "enabled") requireKeyWithValue(t, cfg, []string{colorLabelsKey}, "disabled") requireNoKey(t, cfg, []string{"unknown"}) } @@ -213,3 +216,24 @@ func TestTelemetry(t *testing.T) { require.Equal(t, gh.ConfigUserProvided, entry.Source) }) } + +func TestClipboard(t *testing.T) { + t.Run("returns default when not configured", func(t *testing.T) { + c := newTestConfig() + + entry := c.Clipboard() + + require.Equal(t, "enabled", entry.Value) + require.Equal(t, gh.ConfigDefaultProvided, entry.Source) + }) + + t.Run("returns user configured value", func(t *testing.T) { + c := newTestConfig() + c.Set("", clipboardKey, "disabled") + + entry := c.Clipboard() + + require.Equal(t, "disabled", entry.Value) + require.Equal(t, gh.ConfigUserProvided, entry.Source) + }) +} diff --git a/internal/config/test.go b/internal/config/test.go index 6f096e9436d..a2e10c5ee69 100644 --- a/internal/config/test.go +++ b/internal/config/test.go @@ -72,6 +72,9 @@ func NewMockConfigFromString(cfgString string) *ghmock.ConfigMock { mock.BrowserFunc = func(hostname string) gh.ConfigEntry { return cfg.Browser(hostname) } + mock.ClipboardFunc = func() gh.ConfigEntry { + return cfg.Clipboard() + } mock.TelemetryFunc = func() gh.ConfigEntry { return cfg.Telemetry() } diff --git a/internal/gh/gh.go b/internal/gh/gh.go index f8ca185c0b2..38c08939a7a 100644 --- a/internal/gh/gh.go +++ b/internal/gh/gh.go @@ -41,6 +41,8 @@ type Config interface { AccessiblePrompter(hostname string) ConfigEntry // Browser returns the configured browser, optionally scoped by host. Browser(hostname string) ConfigEntry + // Clipboard returns the configured clipboard setting, ignoring host scoping since clipboard is a global setting. + Clipboard() ConfigEntry // ColorLabels returns the configured color_label setting, optionally scoped by host. ColorLabels(hostname string) ConfigEntry // Editor returns the configured editor, optionally scoped by host. diff --git a/internal/gh/mock/config.go b/internal/gh/mock/config.go index 31e35cb1899..c0699e6516a 100644 --- a/internal/gh/mock/config.go +++ b/internal/gh/mock/config.go @@ -4,10 +4,9 @@ package ghmock import ( - "sync" - "github.com/cli/cli/v2/internal/gh" o "github.com/cli/cli/v2/pkg/option" + "sync" ) // Ensure, that ConfigMock does implement gh.Config. @@ -38,6 +37,9 @@ var _ gh.Config = &ConfigMock{} // CacheDirFunc: func() string { // panic("mock out the CacheDir method") // }, +// ClipboardFunc: func() gh.ConfigEntry { +// panic("mock out the Clipboard method") +// }, // ColorLabelsFunc: func(hostname string) gh.ConfigEntry { // panic("mock out the ColorLabels method") // }, @@ -105,6 +107,9 @@ type ConfigMock struct { // CacheDirFunc mocks the CacheDir method. CacheDirFunc func() string + // ClipboardFunc mocks the Clipboard method. + ClipboardFunc func() gh.ConfigEntry + // ColorLabelsFunc mocks the ColorLabels method. ColorLabelsFunc func(hostname string) gh.ConfigEntry @@ -173,6 +178,9 @@ type ConfigMock struct { // CacheDir holds details about calls to the CacheDir method. CacheDir []struct { } + // Clipboard holds details about calls to the Clipboard method. + Clipboard []struct { + } // ColorLabels holds details about calls to the ColorLabels method. ColorLabels []struct { // Hostname is the hostname argument value. @@ -250,6 +258,7 @@ type ConfigMock struct { lockAuthentication sync.RWMutex lockBrowser sync.RWMutex lockCacheDir sync.RWMutex + lockClipboard sync.RWMutex lockColorLabels sync.RWMutex lockEditor sync.RWMutex lockGetOrDefault sync.RWMutex @@ -443,6 +452,33 @@ func (mock *ConfigMock) CacheDirCalls() []struct { return calls } +// Clipboard calls ClipboardFunc. +func (mock *ConfigMock) Clipboard() gh.ConfigEntry { + if mock.ClipboardFunc == nil { + panic("ConfigMock.ClipboardFunc: method is nil but Config.Clipboard was just called") + } + callInfo := struct { + }{} + mock.lockClipboard.Lock() + mock.calls.Clipboard = append(mock.calls.Clipboard, callInfo) + mock.lockClipboard.Unlock() + return mock.ClipboardFunc() +} + +// ClipboardCalls gets all the calls that were made to Clipboard. +// Check the length with: +// +// len(mockedConfig.ClipboardCalls()) +func (mock *ConfigMock) ClipboardCalls() []struct { +} { + var calls []struct { + } + mock.lockClipboard.RLock() + calls = mock.calls.Clipboard + mock.lockClipboard.RUnlock() + return calls +} + // ColorLabels calls ColorLabelsFunc. func (mock *ConfigMock) ColorLabels(hostname string) gh.ConfigEntry { if mock.ColorLabelsFunc == nil { diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index 24d30c56244..427d32d3a5b 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -39,7 +39,7 @@ type LoginOptions struct { GitProtocol string InsecureStorage bool SkipSSHKeyPrompt bool - Clipboard bool + Clipboard *bool } func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Command { @@ -151,7 +151,7 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm cmd.Flags().StringSliceVarP(&opts.Scopes, "scopes", "s", nil, "Additional authentication scopes to request") cmd.Flags().BoolVar(&tokenStdin, "with-token", false, "Read token from standard input") cmd.Flags().BoolVarP(&opts.Web, "web", "w", false, "Open a browser to authenticate") - cmd.Flags().BoolVarP(&opts.Clipboard, "clipboard", "c", false, "Copy one-time OAuth device code to clipboard") + cmdutil.NilBoolFlag(cmd, &opts.Clipboard, "clipboard", "c", "Copy one-time OAuth device code to clipboard") cmdutil.StringEnumFlag(cmd, &opts.GitProtocol, "git-protocol", "p", "", []string{"ssh", "https"}, "The protocol to use for git operations on this host") // secure storage became the default on 2023/4/04; this flag is left as a no-op for backwards compatibility @@ -171,6 +171,7 @@ func loginRun(opts *LoginOptions) error { return err } authCfg := cfg.Authentication() + copyToClipboard := shared.ShouldCopyToClipboard(cfg, opts.Clipboard) hostname := opts.Hostname if opts.Interactive && hostname == "" { @@ -240,7 +241,7 @@ func loginRun(opts *LoginOptions) error { }, SecureStorage: !opts.InsecureStorage, SkipSSHKeyPrompt: opts.SkipSSHKeyPrompt, - CopyToClipboard: opts.Clipboard, + CopyToClipboard: copyToClipboard, }) } diff --git a/pkg/cmd/auth/login/login_test.go b/pkg/cmd/auth/login/login_test.go index f03792bc220..d66d34b7c77 100644 --- a/pkg/cmd/auth/login/login_test.go +++ b/pkg/cmd/auth/login/login_test.go @@ -137,7 +137,7 @@ func Test_NewCmdLogin(t *testing.T) { Hostname: "github.com", Web: true, Interactive: true, - Clipboard: true, + Clipboard: new(true), }, }, { @@ -146,7 +146,16 @@ func Test_NewCmdLogin(t *testing.T) { wants: LoginOptions{ Hostname: "github.com", Web: true, - Clipboard: true, + Clipboard: new(true), + }, + }, + { + name: "nontty web and clipboard disabled", + cli: "--web --clipboard=false", + wants: LoginOptions{ + Hostname: "github.com", + Web: true, + Clipboard: new(false), }, }, { diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index 8ef5e7ac57b..0fc53a2288f 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -38,7 +38,7 @@ type RefreshOptions struct { Interactive bool InsecureStorage bool - Clipboard bool + Clipboard *bool } func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra.Command { @@ -114,7 +114,7 @@ func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra. cmd.Flags().StringSliceVarP(&opts.Scopes, "scopes", "s", nil, "Additional authentication scopes for gh to have") cmd.Flags().StringSliceVarP(&opts.RemoveScopes, "remove-scopes", "r", nil, "Authentication scopes to remove from gh") cmd.Flags().BoolVar(&opts.ResetScopes, "reset-scopes", false, "Reset authentication scopes to the default minimum set of scopes") - cmd.Flags().BoolVarP(&opts.Clipboard, "clipboard", "c", false, "Copy one-time OAuth device code to clipboard") + cmdutil.NilBoolFlag(cmd, &opts.Clipboard, "clipboard", "c", "Copy one-time OAuth device code to clipboard") // secure storage became the default on 2023/4/04; this flag is left as a no-op for backwards compatibility var secureStorage bool cmd.Flags().BoolVar(&secureStorage, "secure-storage", false, "Save authentication credentials in secure credential store") @@ -136,6 +136,7 @@ func refreshRun(opts *RefreshOptions) error { return err } authCfg := cfg.Authentication() + copyToClipboard := shared.ShouldCopyToClipboard(cfg, opts.Clipboard) candidates := authCfg.Hosts() if len(candidates) == 0 { @@ -200,7 +201,7 @@ func refreshRun(opts *RefreshOptions) error { additionalScopes.RemoveValues(opts.RemoveScopes) - authedToken, authedUser, err := opts.AuthFlow(plainHTTPClient, opts.IO, hostname, additionalScopes.ToSlice(), opts.Interactive, opts.Clipboard) + authedToken, authedUser, err := opts.AuthFlow(plainHTTPClient, opts.IO, hostname, additionalScopes.ToSlice(), opts.Interactive, copyToClipboard) if err != nil { return err } diff --git a/pkg/cmd/auth/refresh/refresh_test.go b/pkg/cmd/auth/refresh/refresh_test.go index 9353de39e74..868c44a0713 100644 --- a/pkg/cmd/auth/refresh/refresh_test.go +++ b/pkg/cmd/auth/refresh/refresh_test.go @@ -39,7 +39,7 @@ func Test_NewCmdRefresh(t *testing.T) { cli: "-c", wants: RefreshOptions{ Hostname: "", - Clipboard: true, + Clipboard: new(true), }, }, { @@ -51,7 +51,7 @@ func Test_NewCmdRefresh(t *testing.T) { cli: "-h aline.cedrac -c", wants: RefreshOptions{ Hostname: "aline.cedrac", - Clipboard: true, + Clipboard: new(true), }, }, { @@ -67,7 +67,16 @@ func Test_NewCmdRefresh(t *testing.T) { cli: "-h aline.cedrac -c", wants: RefreshOptions{ Hostname: "aline.cedrac", - Clipboard: true, + Clipboard: new(true), + }, + }, + { + name: "tty hostname and clipboard disabled", + tty: true, + cli: "-h aline.cedrac --clipboard=false", + wants: RefreshOptions{ + Hostname: "aline.cedrac", + Clipboard: new(false), }, }, { @@ -219,6 +228,7 @@ func Test_refreshRun(t *testing.T) { cfgHosts []string authOut authOut oldScopes string + clipboard string wantErr string nontty bool wantAuthArgs authArgs @@ -252,6 +262,7 @@ func Test_refreshRun(t *testing.T) { hostname: "obed.morton", scopes: []string{}, secureStorage: true, + clipboard: true, }, }, { @@ -261,7 +272,7 @@ func Test_refreshRun(t *testing.T) { }, opts: &RefreshOptions{ Hostname: "", - Clipboard: true, + Clipboard: new(true), }, wantAuthArgs: authArgs{ hostname: "github.com", @@ -270,6 +281,38 @@ func Test_refreshRun(t *testing.T) { clipboard: true, }, }, + { + name: "clipboard disabled by configuration", + cfgHosts: []string{ + "github.com", + }, + clipboard: "disabled", + opts: &RefreshOptions{ + Hostname: "github.com", + }, + wantAuthArgs: authArgs{ + hostname: "github.com", + scopes: []string{}, + secureStorage: true, + clipboard: false, + }, + }, + { + name: "clipboard flag overrides enabled default", + cfgHosts: []string{ + "github.com", + }, + opts: &RefreshOptions{ + Hostname: "github.com", + Clipboard: new(false), + }, + wantAuthArgs: authArgs{ + hostname: "github.com", + scopes: []string{}, + secureStorage: true, + clipboard: false, + }, + }, { name: "no hostname, one host configured", cfgHosts: []string{ @@ -282,6 +325,7 @@ func Test_refreshRun(t *testing.T) { hostname: "github.com", scopes: []string{}, secureStorage: true, + clipboard: true, }, }, { @@ -302,6 +346,7 @@ func Test_refreshRun(t *testing.T) { hostname: "github.com", scopes: []string{}, secureStorage: true, + clipboard: true, }, }, { @@ -316,6 +361,7 @@ func Test_refreshRun(t *testing.T) { hostname: "github.com", scopes: []string{"repo:invite", "public_key:read"}, secureStorage: true, + clipboard: true, }, }, { @@ -331,6 +377,7 @@ func Test_refreshRun(t *testing.T) { hostname: "github.com", scopes: []string{"delete_repo", "codespace", "repo:invite", "public_key:read"}, secureStorage: true, + clipboard: true, }, }, { @@ -345,6 +392,7 @@ func Test_refreshRun(t *testing.T) { hostname: "obed.morton", scopes: []string{}, secureStorage: true, + clipboard: true, }, }, { @@ -357,8 +405,9 @@ func Test_refreshRun(t *testing.T) { InsecureStorage: true, }, wantAuthArgs: authArgs{ - hostname: "obed.morton", - scopes: []string{}, + hostname: "obed.morton", + scopes: []string{}, + clipboard: true, }, }, { @@ -375,6 +424,7 @@ func Test_refreshRun(t *testing.T) { hostname: "github.com", scopes: []string{}, secureStorage: true, + clipboard: true, }, }, { @@ -391,6 +441,7 @@ func Test_refreshRun(t *testing.T) { hostname: "github.com", scopes: []string{"public_key:read", "workflow"}, secureStorage: true, + clipboard: true, }, }, { @@ -407,6 +458,7 @@ func Test_refreshRun(t *testing.T) { hostname: "github.com", scopes: []string{"codespace", "public_key:read"}, secureStorage: true, + clipboard: true, }, }, { @@ -422,6 +474,7 @@ func Test_refreshRun(t *testing.T) { hostname: "github.com", scopes: []string{}, secureStorage: true, + clipboard: true, }, }, { @@ -438,6 +491,7 @@ func Test_refreshRun(t *testing.T) { hostname: "github.com", scopes: []string{"delete_repo", "public_key:read"}, secureStorage: true, + clipboard: true, }, }, { @@ -453,6 +507,7 @@ func Test_refreshRun(t *testing.T) { hostname: "github.com", scopes: []string{"delete_repo"}, secureStorage: true, + clipboard: true, }, }, { @@ -483,6 +538,9 @@ func Test_refreshRun(t *testing.T) { } cfg, _ := config.NewIsolatedTestConfig(t, "") + if tt.clipboard != "" { + cfg.Set("", "clipboard", tt.clipboard) + } for _, hostname := range tt.cfgHosts { _, err := cfg.Authentication().Login(hostname, "test-user", "abc123", "https", false) require.NoError(t, err) diff --git a/pkg/cmd/auth/shared/git_credential.go b/pkg/cmd/auth/shared/git_credential.go index e3136de43b0..d61d7090a7d 100644 --- a/pkg/cmd/auth/shared/git_credential.go +++ b/pkg/cmd/auth/shared/git_credential.go @@ -56,8 +56,7 @@ func (flow *GitCredentialFlow) Prompt(hostname string) error { // Refs: // * https://git-scm.com/docs/git-config#_description // * https://github.com/cli/cli/pull/4109 - var errNotInstalled *git.NotInstalled - if errors.As(configuredHelperErr, &errNotInstalled) { + if _, ok := errors.AsType[*git.NotInstalled](configuredHelperErr); ok { return configuredHelperErr } diff --git a/pkg/cmd/auth/shared/login_flow.go b/pkg/cmd/auth/shared/login_flow.go index a43f919b050..a02707791ba 100644 --- a/pkg/cmd/auth/shared/login_flow.go +++ b/pkg/cmd/auth/shared/login_flow.go @@ -11,6 +11,7 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/authflow" "github.com/cli/cli/v2/internal/browser" + "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/pkg/cmd/ssh-key/add" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/pkg/ssh" @@ -43,6 +44,14 @@ type LoginOptions struct { sshContext ssh.Context } +// ShouldCopyToClipboard resolves an explicit flag value before falling back to configuration. +func ShouldCopyToClipboard(cfg gh.Config, flagValue *bool) bool { + if flagValue != nil { + return *flagValue + } + return cfg.Clipboard().Value == "enabled" +} + func Login(opts *LoginOptions) error { cfg := opts.Config hostname := opts.Hostname diff --git a/pkg/cmd/auth/shared/login_flow_test.go b/pkg/cmd/auth/shared/login_flow_test.go index 31cf2c107e4..f16bb071188 100644 --- a/pkg/cmd/auth/shared/login_flow_test.go +++ b/pkg/cmd/auth/shared/login_flow_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/internal/run" "github.com/cli/cli/v2/pkg/cmd/auth/shared/gitcredentials" @@ -31,6 +32,47 @@ func (c tinyConfig) UsersForHost(hostname string) []string { return nil } +func TestShouldCopyToClipboard(t *testing.T) { + tests := []struct { + name string + configured string + flagValue *bool + want bool + }{ + { + name: "enabled by default", + want: true, + }, + { + name: "enabled by configuration", + configured: "enabled", + want: true, + }, + { + name: "explicit flag enables disabled configuration", + configured: "disabled", + flagValue: new(true), + want: true, + }, + { + name: "explicit flag disables enabled configuration", + configured: "enabled", + flagValue: new(false), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := config.NewMockConfig() + if tt.configured != "" { + cfg.Set("", "clipboard", tt.configured) + } + + require.Equal(t, tt.want, ShouldCopyToClipboard(cfg, tt.flagValue)) + }) + } +} + func TestLogin(t *testing.T) { tests := []struct { name string diff --git a/pkg/cmd/config/list/list_test.go b/pkg/cmd/config/list/list_test.go index 3a04647fdac..8bca4739aea 100644 --- a/pkg/cmd/config/list/list_test.go +++ b/pkg/cmd/config/list/list_test.go @@ -90,6 +90,7 @@ func Test_listRun(t *testing.T) { cfg.Set("HOST", "pager", "less") cfg.Set("HOST", "http_unix_socket", "") cfg.Set("HOST", "browser", "brave") + cfg.Set("", "clipboard", "enabled") return cfg }(), input: &ListOptions{Hostname: "HOST"}, @@ -102,6 +103,7 @@ func Test_listRun(t *testing.T) { pager=less http_unix_socket= browser=brave + clipboard=enabled color_labels=disabled accessible_colors=disabled accessible_prompter=disabled diff --git a/pkg/cmd/config/set/set.go b/pkg/cmd/config/set/set.go index e592d1efc1e..da15dfd04cf 100644 --- a/pkg/cmd/config/set/set.go +++ b/pkg/cmd/config/set/set.go @@ -62,8 +62,8 @@ func NewCmdConfigSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Co } func setRun(opts *SetOptions) error { - if isPerHostOnly(opts.Key) && opts.Hostname == "" { - return cmdutil.FlagErrorf("--host required when setting %s", opts.Key) + if err := validateScope(opts.Key, opts.Hostname); err != nil { + return cmdutil.FlagErrorf("%s", err) } err := ValidateKey(opts.Key) @@ -74,8 +74,7 @@ func setRun(opts *SetOptions) error { err = ValidateValue(opts.Key, opts.Value) if err != nil { - var invalidValue InvalidValueError - if errors.As(err, &invalidValue) { + if invalidValue, ok := errors.AsType[InvalidValueError](err); ok { var values []string for _, v := range invalidValue.ValidValues { values = append(values, fmt.Sprintf("'%s'", v)) @@ -103,14 +102,21 @@ func ValidateKey(key string) error { return fmt.Errorf("invalid key") } -func isPerHostOnly(key string) bool { +func validateScope(key, hostname string) error { for _, configKey := range config.Options { if key == configKey.Key { - return configKey.PerHostOnly + switch { + case configKey.Scope == config.ConfigScopeHostOnly && hostname == "": + return fmt.Errorf("--host required when setting %s", key) + case configKey.Scope == config.ConfigScopeGlobalOnly && hostname != "": + return fmt.Errorf("--host cannot be used when setting %s", key) + default: + return nil + } } } - return false + return nil } type InvalidValueError struct { diff --git a/pkg/cmd/config/set/set_test.go b/pkg/cmd/config/set/set_test.go index a03207cf793..17ecda318f7 100644 --- a/pkg/cmd/config/set/set_test.go +++ b/pkg/cmd/config/set/set_test.go @@ -113,6 +113,16 @@ func Test_setRun(t *testing.T) { }, expectedValue: "vim", }, + { + name: "set existing global setting scoped by host", + input: &SetOptions{ + Config: config.NewMockConfig(), + Hostname: "github.com", + Key: "prompt", + Value: "disabled", + }, + expectedValue: "disabled", + }, { name: "set api_host scoped by host", input: &SetOptions{ @@ -133,6 +143,17 @@ func Test_setRun(t *testing.T) { wantsErr: true, errMsg: "--host required when setting api_host", }, + { + name: "set global key with hostname", + input: &SetOptions{ + Config: config.NewMockConfig(), + Hostname: "github.example.com", + Key: "clipboard", + Value: "enabled", + }, + wantsErr: true, + errMsg: "--host cannot be used when setting clipboard", + }, { name: "set unknown key", input: &SetOptions{ @@ -188,6 +209,12 @@ func Test_ValidateValue(t *testing.T) { err = ValidateValue("editor", "vim") assert.NoError(t, err) + err = ValidateValue("clipboard", "sometimes") + assert.EqualError(t, err, "invalid value") + + err = ValidateValue("clipboard", "enabled") + assert.NoError(t, err) + err = ValidateValue("got", "123") assert.NoError(t, err) @@ -219,10 +246,20 @@ func Test_ValidateKey(t *testing.T) { err = ValidateKey("api_host") assert.NoError(t, err) + + err = ValidateKey("clipboard") + assert.NoError(t, err) } -func Test_isPerHostOnly(t *testing.T) { - assert.True(t, isPerHostOnly("api_host")) - assert.False(t, isPerHostOnly("editor")) - assert.False(t, isPerHostOnly("unknown")) +func Test_validateScope(t *testing.T) { + assert.EqualError(t, validateScope("api_host", ""), "--host required when setting api_host") + assert.NoError(t, validateScope("api_host", "github.example.com")) + assert.EqualError(t, validateScope("clipboard", "github.com"), "--host cannot be used when setting clipboard") + assert.NoError(t, validateScope("clipboard", "")) + assert.NoError(t, validateScope("prompt", "github.com")) + assert.NoError(t, validateScope("prefer_editor_prompt", "github.com")) + assert.NoError(t, validateScope("telemetry", "github.com")) + assert.NoError(t, validateScope("editor", "github.com")) + assert.NoError(t, validateScope("editor", "")) + assert.NoError(t, validateScope("unknown", "github.com")) }