[pull] trunk from cli:trunk

.github/skills/dependabot-triager/SKILL.md

@@ -38,9 +38,18 @@ will fail. Use the MCP tools named below.
 
 You also have the repository checked out at the base branch, and you can read
 and grep it with your local file tools. This is how you establish facts about
-*this* repository: whether a dependency is direct or transitive, and which of its
-APIs the repository actually imports. Never infer either from the PR title, the
-Dependabot summary, or memory. Read `go.mod` and grep the tree.
+*this* repository: whether a dependency is direct or transitive, and how the
+change can reach us. Never infer either from the PR title, the Dependabot
+summary, or memory. Read the manifest, and use the reachability method
+"Required evidence" gives for the ecosystem in question.
+
+The pre-flight step also leaves two artifacts for you when this run includes a
+Go dependency update: `vendor/`, containing the source of every dependency the
+build needs, and `/tmp/gh-aw/go-production-packages.txt`, listing the packages
+compiled into the shipped `gh` binary. They are absent on runs that only bump
+GitHub Actions, which is expected and is not a missing evidence item. `vendor/`
+is generated tooling output, not repository code, so never describe it as a
+change this PR makes.
 
 The checkout is the base branch, not the PR head. To see what the PR changes,
 use `pull_request_read(method: "get_diff", ...)`.
@@ -95,11 +104,67 @@ one exists because guessing it has produced a wrong assessment in the past.
    meaning, and a reorganised or hand-edited file can mix them freely. State
    this only after reading the line.
 
-3. **The repository's usage.** Grep the checkout for the dependency's import
-   paths and record which packages the repository actually imports. An upstream
-   change to a package this repository never imports cannot reach it, and saying
-   otherwise is a false alarm. Conversely, a change to a package that is imported
-   deserves attention even when the release notes sound routine.
+3. **The reachability of each updated dependency.** How you establish this
+   depends on the ecosystem, and getting the method wrong is what produced the
+   worst assessment this skill has made.
+
+   **For a Go module update**, a pre-flight step has vendored the dependency
+   source into `vendor/` and written the packages compiled into the shipped `gh`
+   binary to `/tmp/gh-aw/go-production-packages.txt`. Use those two files. Do
+   **not** answer this by grepping this repository's source for the module's
+   import path.
+
+   That grep answers "does code we wrote import it", which for an indirect
+   dependency is always no, by definition. Reading the silence as "the change
+   cannot reach us" is a tautology, and it has already produced a wrong `High`
+   confidence assessment: a `github.com/docker/cli` bump was reported as
+   carrying no risk because nothing here imports it, when five of its packages
+   are compiled into the shipped binary by way of `go-containerregistry/pkg/authn`.
+
+   Classify each module:
+
+   - If `/tmp/gh-aw/go-production-packages.txt` has any line that is exactly the
+     module path or begins with the module path followed by `/`, the module is
+     **compiled into the shipped binary**, and those exact lines are its
+     reachable surface. Match literally rather than by regex: module paths
+     contain `.`, so a naive pattern can match the wrong module.
+   - Otherwise, if it appears in `vendor/modules.txt`, it is built only for
+     **tests or tooling**. Lower stakes, and worth saying so, but do not call it
+     unreachable.
+   - Otherwise it is **not built at all**.
+
+   `vendor/modules.txt` lists, per module, the exact packages the build graph
+   requires, so it is indifferent to whether the import is ours or another
+   dependency's. That is why it can answer a question the grep cannot.
+
+   Then intersect that reachable surface with the packages the upstream release
+   actually changed (evidence item 4). An empty intersection is a real "no
+   impact" finding you can defend. A non-empty one names the exact packages to
+   scrutinise, and their source is already on disk under `vendor/<module path>/`
+   for you to read.
+
+   If a Go dependency update is in scope but `vendor/modules.txt` or the
+   production package list is missing, this evidence item is unavailable: say
+   which, and cap confidence at `Medium`.
+
+   **For a GitHub Actions update**, the vendored Go artifacts say nothing at all.
+   A bumped action is not a Go module, so it will be absent from both files, and
+   you must not read that absence as "not built" or as any kind of safety
+   signal. Establish reachability by grepping `.github/` for `uses:` lines
+   naming the action, and record every workflow and job that calls it.
+
+   Grepping is the correct method here, and the reason it is correct for actions
+   but not for Go is worth understanding: a workflow reaches an action only by
+   naming it in a `uses:` line in our own files, so there is no equivalent of an
+   indirect dependency that our source never mentions. If no `uses:` line names
+   it, check whether the reference lives in a generated `.lock.yml` or a
+   `# gh-aw-manifest:` block before concluding it is unused.
+
+   Then judge the change against how those call sites use it: which inputs they
+   pass, which outputs they consume, and what permissions the job grants it.
+
+   **For any other ecosystem**, say plainly in the prose that you had no
+   mechanical way to establish reachability, and cap confidence at `Medium`.
 
 4. **Upstream release evidence** for the target version, via the `repos` tools.
 
@@ -148,9 +213,9 @@ is:
 
 Confidence is about the evidence your conclusion actually depends on, not about
 how much of the upstream history you read. If a bump spans four releases but
-touches nothing this repository imports, and you verified that by reading the
-manifest and grepping the tree, that is `High`. You do not need to read all four
-releases to be certain of a conclusion that does not depend on them.
+changes nothing within the reachable surface you established in evidence item 3,
+that is `High`. You do not need to read all four releases to be certain of a
+conclusion that does not depend on them.
 
 A negative recommendation can still have high confidence. For example, if CI is
 reproducibly red, use `Do not merge, Confidence: High`.
@@ -187,9 +252,11 @@ not restate metadata that the PR page already shows.
   whether a "patch" is genuinely small.
 
 Keep this bounded by relevance, not by a call budget. Read until the questions
-your recommendation depends on are answered, then stop. Use the usage trace from
-the required evidence to decide what is relevant: changes to packages this
-repository does not import do not need to be chased.
+your recommendation depends on are answered, then stop. Use the reachable
+surface from the required evidence to decide what is relevant: changes outside
+it do not need to be chased. Changes inside it do, and for a Go module the
+affected code is already on disk under `vendor/<module path>/`, so read it
+rather than inferring from release notes.
 
 If the upstream history genuinely is too large to establish something your
 recommendation depends on, say so in the prose and cap confidence at **Medium**.

.github/workflows/dependabot-triage.md

@@ -84,6 +84,7 @@ timeout-minutes: 30
 # correctly.
 steps:
   - name: Compute Dependabot triage work list
+    id: worklist
     env:
       GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
       GITHUB_REPOSITORY: ${{ github.repository }}
@@ -112,6 +113,7 @@ steps:
         else
           echo "Ignoring non-numeric pr_number input"
           echo '[]' > "$WORKLIST"
+          echo "needs_go=false" >> "$GITHUB_OUTPUT"
           echo '{"type":"noop","message":"pr_number input was not a positive integer"}' >> "$SAFE_OUT"
           exit 0
         fi
@@ -168,6 +170,7 @@ steps:
       echo "PRs with terminal CI: $(printf '%s' "$ready" | jq length)"
 
       work='[]'
+      needs_go=false
       for row in $(printf '%s' "$ready" | jq -r '.[] | @base64'); do
         entry=$(printf '%s' "$row" | base64 --decode)
         n=$(printf '%s' "$entry" | jq -r '.number')
@@ -187,17 +190,106 @@ steps:
         else
           echo "PR #$n: needs assessment (head $head, last assessed '${assessed:-none}')"
           work=$(printf '%s' "$work" | jq -c --argjson e "$entry" '. + [$e]')
+
+          # Most Dependabot traffic here bumps GitHub Actions, not Go modules,
+          # and the vendored Go artifacts are meaningless for those. Only pay
+          # for vendoring when something in scope actually moves the Go
+          # manifests. Treat an unreadable file list as "might be Go" so a
+          # transient API failure degrades to wasted work rather than to
+          # missing evidence.
+          files=$(gh pr view "$n" --repo "$GITHUB_REPOSITORY" --json files \
+                    --jq '.files[].path' 2>/dev/null) || files="go.mod"
+          if printf '%s\n' "$files" | grep -qE '^(go\.mod|go\.sum)$'; then
+            needs_go=true
+          fi
         fi
       done
 
       printf '%s' "$work" > "$WORKLIST"
       count=$(printf '%s' "$work" | jq length)
       echo "Work list: $count PR(s) -> $WORKLIST"
 
+      # Gates the vendoring step below, so a run with no Go dependency work
+      # costs no module downloads on top of costing no AI Credits.
+      echo "needs_go=$needs_go" >> "$GITHUB_OUTPUT"
+      echo "Go reachability evidence needed: $needs_go"
+
       if [ "$count" -eq 0 ]; then
         echo '{"type":"noop","message":"No Dependabot PRs need triage: all open PRs are already assessed at their current head commit, or their CI is still pending."}' >> "$SAFE_OUT"
       fi
 
+  # Dependency reachability evidence. The agent is asked whether an upstream
+  # change can reach this repository. It used to answer that by grepping our
+  # own source for the module's import path, which for an indirect dependency
+  # always finds nothing - by definition, since "indirect" means precisely that
+  # we do not import it. Reading that silence as safety is a tautology, and it
+  # produced a wrong `High` confidence assessment on PR #14066: a
+  # `github.com/docker/cli` bump was called risk-free when five of its packages
+  # are compiled into the shipped binary via `go-containerregistry/pkg/authn`.
+  #
+  # These steps replace that inference with the build graph. `go mod vendor`
+  # resolves what the module graph actually needs, so it is indifferent to who
+  # writes the import, and its `vendor/modules.txt` is a per-module list of the
+  # exact packages required. The vendored tree also puts the dependency source
+  # itself in the workspace, which the agent already has mounted, so it can read
+  # the changed code rather than reasoning from release notes alone.
+  #
+  # `vendor/` is gitignored and nothing in this job commits, so this is a
+  # read-only side effect on the runner's checkout.
+  #
+  # Gated on a Go manifest actually moving. Most Dependabot traffic in this
+  # repository bumps GitHub Actions, where these artifacts say nothing, so
+  # vendoring unconditionally would download tens of megabytes per run to
+  # produce evidence the agent must ignore. The skill tells the agent to
+  # establish Actions reachability by grepping `.github/` for `uses:` instead,
+  # and warns it not to read a module's absence from these files as safety.
+  #
+  # There is deliberately no `actions/setup-go` step here. The compiler detects
+  # the `go` invocations below and emits its own Setup Go step, taking the
+  # version from `go.mod`, so an explicit one would be silently replaced and
+  # would drift. That generated step is not conditional, so a run with no Go
+  # work still pays for the toolchain but not for the module downloads below.
+  - name: Vendor dependency source for the agent
+    if: steps.worklist.outputs.needs_go == 'true'
+    run: |
+      set -uo pipefail
+      PKGS=/tmp/gh-aw/go-production-packages.txt
+      rm -f "$PKGS" "$PKGS.tmp"
+
+      # Deliberately not fatal. Missing evidence should degrade the assessment,
+      # not cancel triage: the skill treats an absent artifact as an
+      # unobtainable evidence item and caps confidence at Medium, which is
+      # visible in the posted comment. A hard failure would post nothing at all.
+      if ! go mod vendor; then
+        echo "::warning::go mod vendor failed; the agent has no reachability evidence this run."
+        rm -rf vendor
+        exit 0
+      fi
+
+      # `go list -deps` evaluates build constraints for one GOOS/GOARCH/cgo
+      # combination, so a single invocation would miss platform-guarded imports
+      # and understate what a change can reach. Union the exact release matrix
+      # from .goreleaser.yml, including linux's CGO_ENABLED=0, so the evidence
+      # describes what we actually ship. Today every combination yields the same
+      # set, but that is a property of the current dependencies, not a guarantee.
+      for target in \
+        "darwin amd64 1" "darwin arm64 1" \
+        "linux 386 0" "linux arm 0" "linux amd64 0" "linux arm64 0" \
+        "windows 386 1" "windows amd64 1" "windows arm64 1"; do
+        # shellcheck disable=SC2086
+        set -- $target
+        if ! GOOS="$1" GOARCH="$2" CGO_ENABLED="$3" go list -deps ./cmd/gh >> "$PKGS.tmp"; then
+          echo "::warning::go list failed for GOOS=$1 GOARCH=$2; production package list is incomplete and will not be written."
+          rm -f "$PKGS.tmp"
+          exit 0
+        fi
+      done
+      sort -u "$PKGS.tmp" -o "$PKGS"
+      rm -f "$PKGS.tmp"
+
+      echo "Vendored $(grep -c '^# ' vendor/modules.txt) modules into vendor/"
+      echo "Shipped binary compiles $(wc -l < "$PKGS" | tr -d ' ') packages -> $PKGS"
+
 # Security + output envelope (read-only GitHub tools, GitHub App posting
 # identity, comment-only safe-output). Vendored locally so this workflow has no
 # cross-repository dependency; see the note at the bottom of this file.
@@ -227,6 +319,14 @@ assessed at their current head commit, and it has already applied the optional
 
 Read that file. It is a JSON array of objects with `number` and `head_sha`.
 
+The same pre-flight step has also, when this run includes a Go dependency
+update, vendored the dependency source into `vendor/` and written the packages
+compiled into the shipped `gh` binary to
+`/tmp/gh-aw/go-production-packages.txt`. Those are your Go reachability
+evidence; the skill explains how to use them, and how to establish reachability
+for GitHub Actions updates, where those files do not apply and their absence
+means nothing.
+
 That array is your entire working scope for this run. Assess every entry in it,
 and never comment on anything outside it. If the array is empty, do nothing.
 
@@ -243,8 +343,10 @@ For each entry in the work list, follow the `dependabot-triager` skill precisely
 
 1. Gather the skill's four required evidence items, including the PR's own diff,
    the dependency's direct/indirect position read from the manifest in the
-   checkout, and a usage trace grepped from the checked-out source tree. Never
-   infer these from the PR title or the Dependabot summary.
+   checkout, and the reachability of each updated dependency established by the
+   method the skill gives for that ecosystem. Never infer these from the PR
+   title or the Dependabot summary, and never treat a dependency's absence from
+   the Go artifacts as evidence of safety.
 2. Check in-repo coherence: whether the PR edits generated files and leaves
    embedded version pins or metadata inconsistent.
 3. Decide the recommendation and confidence, and post exactly one comment.

acceptance/testdata/release/release-delete.txtar

@@ -0,0 +1,39 @@
+# Create a repository with a file so it has a default branch
+exec gh repo create $ORG/$SCRIPT_NAME-$RANDOM_STRING --add-readme --private
+
+# Defer repo cleanup
+defer gh repo delete --yes $ORG/$SCRIPT_NAME-$RANDOM_STRING
+
+# Create a release in the repo
+exec gh release create v1.2.3 --repo $ORG/$SCRIPT_NAME-$RANDOM_STRING --notes 'awesome release' --latest
+
+# Upload an asset to the release
+exec gh release upload v1.2.3 --repo $ORG/$SCRIPT_NAME-$RANDOM_STRING asset.txt
+
+# Delete the asset from the release
+exec gh release delete-asset v1.2.3 asset.txt --repo $ORG/$SCRIPT_NAME-$RANDOM_STRING --yes
+
+# Verify the release has no assets
+exec gh release view v1.2.3 --repo $ORG/$SCRIPT_NAME-$RANDOM_STRING --json assets --jq '.assets | length'
+stdout '0'
+
+# Downloading the deleted asset should fail
+! exec gh release download v1.2.3 --repo $ORG/$SCRIPT_NAME-$RANDOM_STRING
+stderr 'no assets to download'
+
+# Delete the release and its tag
+exec gh release delete v1.2.3 --repo $ORG/$SCRIPT_NAME-$RANDOM_STRING --yes --cleanup-tag
+
+# Wait for tag deletion to become visible through the ref lookup
+sleep 5
+
+# Verify the release is gone
+! exec gh release view v1.2.3 --repo $ORG/$SCRIPT_NAME-$RANDOM_STRING
+stderr 'release not found'
+
+# Verify the tag is gone
+! exec gh api repos/$ORG/$SCRIPT_NAME-$RANDOM_STRING/git/ref/tags/v1.2.3
+stderr 'Not Found'
+
+-- asset.txt --
+Hello, world!

pkg/cmd/release/delete-asset/delete_asset.go

@@ -97,7 +97,7 @@ func deleteAssetRun(opts *DeleteAssetOptions) error {
 		return fmt.Errorf("asset %s not found in release %s", opts.AssetName, release.TagName)
 	}
 
-	err = deleteAsset(httpClient, safeurl.NewImmutableSafeURL(assetURL))
+	err = deleteAsset(httpClient, baseRepo.RepoHost(), safeurl.NewImmutableSafeURL(assetURL))
 	if err != nil {
 		return err
 	}
@@ -112,20 +112,9 @@ func deleteAssetRun(opts *DeleteAssetOptions) error {
 	return nil
 }
 
-func deleteAsset(httpClient *http.Client, assetURL safeurl.SafeURL) error {
-	req, err := http.NewRequest("DELETE", assetURL.String(), nil)
-	if err != nil {
-		return err
-	}
-
-	resp, err := httpClient.Do(req)
-	if err != nil {
-		return err
-	}
-	defer resp.Body.Close()
-
-	if resp.StatusCode > 299 {
-		return api.HandleHTTPError(resp)
-	}
-	return nil
+func deleteAsset(httpClient *http.Client, host string, assetURL safeurl.SafeURL) error {
+	// TODO(api-client-rollout)
+	// This line of code is part of a mechanical roll out of the api client.
+	// As a follow up, consider whether the api client can be injected to this call site, rather than constructed
+	return api.NewClientFromHTTP(httpClient).REST(host, http.MethodDelete, assetURL.String(), nil, nil)
 }

pkg/cmd/release/delete-asset/delete_asset_test.go

@@ -6,8 +6,10 @@ import (
 	"net/http"
 	"testing"
 
+	"github.com/cli/cli/v2/api"
 	"github.com/cli/cli/v2/internal/ghrepo"
 	"github.com/cli/cli/v2/internal/prompter"
+	"github.com/cli/cli/v2/internal/safeurl"
 	"github.com/cli/cli/v2/pkg/cmd/release/shared"
 	"github.com/cli/cli/v2/pkg/cmdutil"
 	"github.com/cli/cli/v2/pkg/httpmock"
@@ -199,3 +201,24 @@ func Test_deleteAssetRun(t *testing.T) {
 		})
 	}
 }
+
+func Test_deleteAsset_httpError(t *testing.T) {
+	reg := &httpmock.Registry{}
+	defer reg.Verify(t)
+	reg.Register(
+		func(req *http.Request) bool {
+			return req.Method == http.MethodDelete &&
+				req.URL.EscapedPath() == "/repos/OWNER/REPO/releases/assets/1" &&
+				req.URL.Host == "api.github.com"
+		},
+		httpmock.StatusStringResponse(404, `{"message":"Not Found"}`),
+	)
+
+	httpClient := &http.Client{Transport: reg}
+	err := deleteAsset(httpClient, "example.com", safeurl.NewImmutableSafeURL("https://api.github.com/repos/OWNER/REPO/releases/assets/1"))
+
+	var httpErr api.HTTPError
+	require.ErrorAs(t, err, &httpErr)
+	assert.Equal(t, http.StatusNotFound, httpErr.StatusCode)
+	assert.Contains(t, err.Error(), "HTTP 404")
+}