diff --git a/.github/skills/dependabot-triager/SKILL.md b/.github/skills/dependabot-triager/SKILL.md index c70bee442b7..107c6b0b757 100644 --- a/.github/skills/dependabot-triager/SKILL.md +++ b/.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//` + 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//`, 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**. diff --git a/.github/workflows/dependabot-triage.lock.yml b/.github/workflows/dependabot-triage.lock.yml index 072c741e561..2be3365ee4e 100644 --- a/.github/workflows/dependabot-triage.lock.yml +++ b/.github/workflows/dependabot-triage.lock.yml @@ -1,5 +1,5 @@ -# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"34f23dedf99812709f0a81e6ec95fe19ec072bcc9cdf979f061750fa15ffa5d9","body_hash":"e9a29443e1284ccedb91aa26eff179146134430abdec75cc5206b2be2d5e2227","compiler_version":"v0.83.4","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.75"}} -# gh-aw-manifest: {"version":1,"secrets":["CLI_TRIAGE_APP_CLIENT_ID","CLI_TRIAGE_APP_PRIVATE_KEY","COPILOT_GITHUB_TOKEN","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"3d3c42e5aac5ba805825da76410c181273ba90b1","version":"v7.0.1"},{"repo":"actions/create-github-app-token","sha":"bcd2ba49218906704ab6c1aa796996da409d3eb1","version":"v3.2.0"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-node","sha":"820762786026740c76f36085b0efc47a31fe5020","version":"v7.0.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"},{"repo":"github/gh-aw-actions/setup","sha":"e89c65e17eb281bbd5ff2ff9e9199a03e96654c7","version":"v0.83.4"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.42","digest":"sha256:26a8af4e5566485b02f52af59ee03803ae798271a9619d4767e94d07806deb9b","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.42@sha256:26a8af4e5566485b02f52af59ee03803ae798271a9619d4767e94d07806deb9b"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.42","digest":"sha256:944f2686c9ab9bec338fd14b662461662f77cd12cd0ea8a3e7cb8c0987cd1607","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.42@sha256:944f2686c9ab9bec338fd14b662461662f77cd12cd0ea8a3e7cb8c0987cd1607"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.42","digest":"sha256:42dfeb649c680a8558cd5423dbc530b653a69413e35ffbe5e71da5d48c94bdf0","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.42@sha256:42dfeb649c680a8558cd5423dbc530b653a69413e35ffbe5e71da5d48c94bdf0"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.4.6","digest":"sha256:fecabec51bbc41f2ad61076d6bcd9a36ef23b142e672a444e054d37fc29de93c","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.4.6@sha256:fecabec51bbc41f2ad61076d6bcd9a36ef23b142e672a444e054d37fc29de93c"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:a8082161d7dceda14b68f32eb39d0eaa96b825d07f5895b096afab9d9e0c7748","pinned_image":"ghcr.io/github/gh-aw-node@sha256:a8082161d7dceda14b68f32eb39d0eaa96b825d07f5895b096afab9d9e0c7748"},{"image":"ghcr.io/github/github-mcp-server:v1.7.0","digest":"sha256:c491ffdf6f4c85cb5397021bc655edb8ab825c6f5f568e7597d77a1bd7c4d308","pinned_image":"ghcr.io/github/github-mcp-server:v1.7.0@sha256:c491ffdf6f4c85cb5397021bc655edb8ab825c6f5f568e7597d77a1bd7c4d308"}]} +# gh-aw-metadata: {"schema_version":"v4","frontmatter_hash":"7d5da2963a661e9f51c02e53d1732a97e784cb8eb6a309eae820992633e672ef","body_hash":"005d8b2f51af3736602c09405ed6d06f99dcb5b44c5bedecb506f3e7385d3bd5","compiler_version":"v0.83.4","strict":true,"agent_id":"copilot","engine_versions":{"copilot":"1.0.75"}} +# gh-aw-manifest: {"version":1,"secrets":["CLI_TRIAGE_APP_CLIENT_ID","CLI_TRIAGE_APP_PRIVATE_KEY","COPILOT_GITHUB_TOKEN","GH_AW_GITHUB_MCP_SERVER_TOKEN","GH_AW_GITHUB_TOKEN","GITHUB_TOKEN"],"actions":[{"repo":"actions/cache/restore","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/cache/save","sha":"55cc8345863c7cc4c66a329aec7e433d2d1c52a9","version":"v6.1.0"},{"repo":"actions/checkout","sha":"3d3c42e5aac5ba805825da76410c181273ba90b1","version":"v7.0.1"},{"repo":"actions/create-github-app-token","sha":"bcd2ba49218906704ab6c1aa796996da409d3eb1","version":"v3.2.0"},{"repo":"actions/download-artifact","sha":"3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c","version":"v8.0.1"},{"repo":"actions/github-script","sha":"3a2844b7e9c422d3c10d287c895573f7108da1b3","version":"v9.0.0"},{"repo":"actions/setup-go","sha":"b7ad1dad31e06c5925ef5d2fc7ad053ef454303e","version":"v7.0.0"},{"repo":"actions/setup-node","sha":"820762786026740c76f36085b0efc47a31fe5020","version":"v7.0.0"},{"repo":"actions/upload-artifact","sha":"043fb46d1a93c77aae656e7c1c64a875d1fc6a0a","version":"v7.0.1"},{"repo":"github/gh-aw-actions/setup","sha":"e89c65e17eb281bbd5ff2ff9e9199a03e96654c7","version":"v0.83.4"}],"containers":[{"image":"ghcr.io/github/gh-aw-firewall/agent:0.27.42","digest":"sha256:26a8af4e5566485b02f52af59ee03803ae798271a9619d4767e94d07806deb9b","pinned_image":"ghcr.io/github/gh-aw-firewall/agent:0.27.42@sha256:26a8af4e5566485b02f52af59ee03803ae798271a9619d4767e94d07806deb9b"},{"image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.42","digest":"sha256:944f2686c9ab9bec338fd14b662461662f77cd12cd0ea8a3e7cb8c0987cd1607","pinned_image":"ghcr.io/github/gh-aw-firewall/api-proxy:0.27.42@sha256:944f2686c9ab9bec338fd14b662461662f77cd12cd0ea8a3e7cb8c0987cd1607"},{"image":"ghcr.io/github/gh-aw-firewall/squid:0.27.42","digest":"sha256:42dfeb649c680a8558cd5423dbc530b653a69413e35ffbe5e71da5d48c94bdf0","pinned_image":"ghcr.io/github/gh-aw-firewall/squid:0.27.42@sha256:42dfeb649c680a8558cd5423dbc530b653a69413e35ffbe5e71da5d48c94bdf0"},{"image":"ghcr.io/github/gh-aw-mcpg:v0.4.6","digest":"sha256:fecabec51bbc41f2ad61076d6bcd9a36ef23b142e672a444e054d37fc29de93c","pinned_image":"ghcr.io/github/gh-aw-mcpg:v0.4.6@sha256:fecabec51bbc41f2ad61076d6bcd9a36ef23b142e672a444e054d37fc29de93c"},{"image":"ghcr.io/github/gh-aw-node","digest":"sha256:a8082161d7dceda14b68f32eb39d0eaa96b825d07f5895b096afab9d9e0c7748","pinned_image":"ghcr.io/github/gh-aw-node@sha256:a8082161d7dceda14b68f32eb39d0eaa96b825d07f5895b096afab9d9e0c7748"},{"image":"ghcr.io/github/github-mcp-server:v1.7.0","digest":"sha256:c491ffdf6f4c85cb5397021bc655edb8ab825c6f5f568e7597d77a1bd7c4d308","pinned_image":"ghcr.io/github/github-mcp-server:v1.7.0@sha256:c491ffdf6f4c85cb5397021bc655edb8ab825c6f5f568e7597d77a1bd7c4d308"}]} # This file was automatically generated by gh-aw (v0.83.4). DO NOT EDIT. To debug this workflow, load the skill at https://github.com/github/gh-aw/blob/main/debug.md # # ___ _ _ @@ -50,6 +50,7 @@ # - actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 # - actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 # - actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 (source v9) +# - actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 # - actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 # - actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 # - github/gh-aw-actions/setup@e89c65e17eb281bbd5ff2ff9e9199a03e96654c7 # v0.83.4 @@ -456,6 +457,13 @@ jobs: uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: persist-credentials: false + - name: Setup Go + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version: '1.26' + cache: false + - name: Capture GOROOT for AWF chroot mode + run: echo "GOROOT=$(go env GOROOT)" >> "$GITHUB_ENV" - name: Create gh-aw temp directory run: bash "${RUNNER_TEMP}/gh-aw/actions/create_gh_aw_tmp_dir.sh" - name: Configure gh CLI for GitHub Enterprise @@ -472,8 +480,12 @@ jobs: GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} GITHUB_REPOSITORY: ${{ github.repository }} PR_NUMBER_INPUT: ${{ github.event.inputs.pr_number }} + id: worklist name: Compute Dependabot triage work list - run: "set -euo pipefail\nmkdir -p /tmp/gh-aw\nWORKLIST=/tmp/gh-aw/dependabot-worklist.json\n\n# The safe-outputs directory is created by a later generated step, so\n# create it here before appending. Fall back to the compiler's own path if\n# the variable is ever empty rather than failing under `set -u`.\nSAFE_OUT=\"${GH_AW_SAFE_OUTPUTS:-${RUNNER_TEMP}/gh-aw/safeoutputs/outputs.jsonl}\"\nmkdir -p \"$(dirname \"$SAFE_OUT\")\"\n\n# Treat the dispatch input as a PR number and nothing else.\nsingle=\"\"\nif [ -n \"${PR_NUMBER_INPUT:-}\" ]; then\n if printf '%s' \"$PR_NUMBER_INPUT\" | grep -qE '^[1-9][0-9]*$'; then\n single=\"$PR_NUMBER_INPUT\"\n echo \"Dispatch input restricts this run to PR #$single\"\n else\n echo \"Ignoring non-numeric pr_number input\"\n echo '[]' > \"$WORKLIST\"\n echo '{\"type\":\"noop\",\"message\":\"pr_number input was not a positive integer\"}' >> \"$SAFE_OUT\"\n exit 0\n fi\nfi\n\nprs=$(gh pr list --repo \"$GITHUB_REPOSITORY\" --state open \\\n --author app/dependabot --limit 100 \\\n --json number,headRefOid,statusCheckRollup)\n\n# gh truncates silently at --limit, and the listing order is stable, so\n# anything past the cap would never be reached on a later run either. The\n# cap is well above both the realistic number of open Dependabot PRs and\n# the safe-output comment cap, so say so rather than paginate for a case\n# that would already be degenerate.\nif [ \"$(printf '%s' \"$prs\" | jq length)\" -ge 100 ]; then\n echo \"::warning::Open Dependabot PRs hit the 100 listing cap; any beyond it are not being triaged.\"\nfi\n\nif [ -n \"$single\" ]; then\n prs=$(printf '%s' \"$prs\" | jq --argjson n \"$single\" '[.[] | select(.number == $n)]')\nfi\n\n# A PR is ready to assess only when every check has reached a terminal\n# state. statusCheckRollup mixes CheckRun (has .status) and StatusContext\n# (has .state) shapes, so both are handled. A null rollup means the checks\n# could not be read at all rather than that there are none - a dropped\n# `checks:`/`statuses:` permission would look like this - so count it as\n# pending. Treating it as ready would silently assess PRs mid-CI.\njq_pending='\n def pending:\n if has(\"status\") then (.status != \"COMPLETED\")\n else ((.state // \"SUCCESS\") as $s | $s == \"PENDING\" or $s == \"EXPECTED\")\n end;\n def pending_names:\n if .statusCheckRollup == null then [\"\"]\n else [.statusCheckRollup[] | select(pending) | (.name // .context // \"unnamed\")]\n end;\n'\n\nready=$(printf '%s' \"$prs\" | jq -c \"$jq_pending\"'\n [ .[]\n | select((pending_names | length) == 0)\n | {number: .number, head_sha: .headRefOid} ]')\n\n# Name the PRs this gate excluded. A check that never reaches a terminal\n# state would otherwise keep a PR out of triage forever, silently.\nprintf '%s' \"$prs\" | jq -r \"$jq_pending\"'\n .[]\n | . as $pr\n | pending_names\n | select(length > 0)\n | \"PR #\\($pr.number): skipped, checks still pending: \\(join(\", \"))\"'\n\necho \"PRs with terminal CI: $(printf '%s' \"$ready\" | jq length)\"\n\nwork='[]'\nfor row in $(printf '%s' \"$ready\" | jq -r '.[] | @base64'); do\n entry=$(printf '%s' \"$row\" | base64 --decode)\n n=$(printf '%s' \"$entry\" | jq -r '.number')\n head=$(printf '%s' \"$entry\" | jq -r '.head_sha')\n\n # Find the newest dedup marker in our own comments. This read depends on\n # `integrity-proxy: false` in the imported envelope: the pre-agent DIFC\n # proxy applies min-integrity but not trusted-users, so with it enabled\n # our own comments are filtered out here and dedup silently fails open.\n assessed=$(gh api \"repos/$GITHUB_REPOSITORY/issues/$n/comments\" --paginate \\\n --jq '.[] | select(.user.login == \"cli-triage[bot]\") | .body' \\\n | grep -oE '_Assessed at head commit `[0-9a-f]{40}`\\._' \\\n | tail -1 | grep -oE '[0-9a-f]{40}' || true)\n\n if [ \"$assessed\" = \"$head\" ]; then\n echo \"PR #$n: already assessed at $head, skipping\"\n else\n echo \"PR #$n: needs assessment (head $head, last assessed '${assessed:-none}')\"\n work=$(printf '%s' \"$work\" | jq -c --argjson e \"$entry\" '. + [$e]')\n fi\ndone\n\nprintf '%s' \"$work\" > \"$WORKLIST\"\ncount=$(printf '%s' \"$work\" | jq length)\necho \"Work list: $count PR(s) -> $WORKLIST\"\n\nif [ \"$count\" -eq 0 ]; then\n 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\"\nfi" + run: "set -euo pipefail\nmkdir -p /tmp/gh-aw\nWORKLIST=/tmp/gh-aw/dependabot-worklist.json\n\n# The safe-outputs directory is created by a later generated step, so\n# create it here before appending. Fall back to the compiler's own path if\n# the variable is ever empty rather than failing under `set -u`.\nSAFE_OUT=\"${GH_AW_SAFE_OUTPUTS:-${RUNNER_TEMP}/gh-aw/safeoutputs/outputs.jsonl}\"\nmkdir -p \"$(dirname \"$SAFE_OUT\")\"\n\n# Treat the dispatch input as a PR number and nothing else.\nsingle=\"\"\nif [ -n \"${PR_NUMBER_INPUT:-}\" ]; then\n if printf '%s' \"$PR_NUMBER_INPUT\" | grep -qE '^[1-9][0-9]*$'; then\n single=\"$PR_NUMBER_INPUT\"\n echo \"Dispatch input restricts this run to PR #$single\"\n else\n echo \"Ignoring non-numeric pr_number input\"\n echo '[]' > \"$WORKLIST\"\n echo \"needs_go=false\" >> \"$GITHUB_OUTPUT\"\n echo '{\"type\":\"noop\",\"message\":\"pr_number input was not a positive integer\"}' >> \"$SAFE_OUT\"\n exit 0\n fi\nfi\n\nprs=$(gh pr list --repo \"$GITHUB_REPOSITORY\" --state open \\\n --author app/dependabot --limit 100 \\\n --json number,headRefOid,statusCheckRollup)\n\n# gh truncates silently at --limit, and the listing order is stable, so\n# anything past the cap would never be reached on a later run either. The\n# cap is well above both the realistic number of open Dependabot PRs and\n# the safe-output comment cap, so say so rather than paginate for a case\n# that would already be degenerate.\nif [ \"$(printf '%s' \"$prs\" | jq length)\" -ge 100 ]; then\n echo \"::warning::Open Dependabot PRs hit the 100 listing cap; any beyond it are not being triaged.\"\nfi\n\nif [ -n \"$single\" ]; then\n prs=$(printf '%s' \"$prs\" | jq --argjson n \"$single\" '[.[] | select(.number == $n)]')\nfi\n\n# A PR is ready to assess only when every check has reached a terminal\n# state. statusCheckRollup mixes CheckRun (has .status) and StatusContext\n# (has .state) shapes, so both are handled. A null rollup means the checks\n# could not be read at all rather than that there are none - a dropped\n# `checks:`/`statuses:` permission would look like this - so count it as\n# pending. Treating it as ready would silently assess PRs mid-CI.\njq_pending='\n def pending:\n if has(\"status\") then (.status != \"COMPLETED\")\n else ((.state // \"SUCCESS\") as $s | $s == \"PENDING\" or $s == \"EXPECTED\")\n end;\n def pending_names:\n if .statusCheckRollup == null then [\"\"]\n else [.statusCheckRollup[] | select(pending) | (.name // .context // \"unnamed\")]\n end;\n'\n\nready=$(printf '%s' \"$prs\" | jq -c \"$jq_pending\"'\n [ .[]\n | select((pending_names | length) == 0)\n | {number: .number, head_sha: .headRefOid} ]')\n\n# Name the PRs this gate excluded. A check that never reaches a terminal\n# state would otherwise keep a PR out of triage forever, silently.\nprintf '%s' \"$prs\" | jq -r \"$jq_pending\"'\n .[]\n | . as $pr\n | pending_names\n | select(length > 0)\n | \"PR #\\($pr.number): skipped, checks still pending: \\(join(\", \"))\"'\n\necho \"PRs with terminal CI: $(printf '%s' \"$ready\" | jq length)\"\n\nwork='[]'\nneeds_go=false\nfor row in $(printf '%s' \"$ready\" | jq -r '.[] | @base64'); do\n entry=$(printf '%s' \"$row\" | base64 --decode)\n n=$(printf '%s' \"$entry\" | jq -r '.number')\n head=$(printf '%s' \"$entry\" | jq -r '.head_sha')\n\n # Find the newest dedup marker in our own comments. This read depends on\n # `integrity-proxy: false` in the imported envelope: the pre-agent DIFC\n # proxy applies min-integrity but not trusted-users, so with it enabled\n # our own comments are filtered out here and dedup silently fails open.\n assessed=$(gh api \"repos/$GITHUB_REPOSITORY/issues/$n/comments\" --paginate \\\n --jq '.[] | select(.user.login == \"cli-triage[bot]\") | .body' \\\n | grep -oE '_Assessed at head commit `[0-9a-f]{40}`\\._' \\\n | tail -1 | grep -oE '[0-9a-f]{40}' || true)\n\n if [ \"$assessed\" = \"$head\" ]; then\n echo \"PR #$n: already assessed at $head, skipping\"\n else\n echo \"PR #$n: needs assessment (head $head, last assessed '${assessed:-none}')\"\n work=$(printf '%s' \"$work\" | jq -c --argjson e \"$entry\" '. + [$e]')\n\n # Most Dependabot traffic here bumps GitHub Actions, not Go modules,\n # and the vendored Go artifacts are meaningless for those. Only pay\n # for vendoring when something in scope actually moves the Go\n # manifests. Treat an unreadable file list as \"might be Go\" so a\n # transient API failure degrades to wasted work rather than to\n # missing evidence.\n files=$(gh pr view \"$n\" --repo \"$GITHUB_REPOSITORY\" --json files \\\n --jq '.files[].path' 2>/dev/null) || files=\"go.mod\"\n if printf '%s\\n' \"$files\" | grep -qE '^(go\\.mod|go\\.sum)$'; then\n needs_go=true\n fi\n fi\ndone\n\nprintf '%s' \"$work\" > \"$WORKLIST\"\ncount=$(printf '%s' \"$work\" | jq length)\necho \"Work list: $count PR(s) -> $WORKLIST\"\n\n# Gates the vendoring step below, so a run with no Go dependency work\n# costs no module downloads on top of costing no AI Credits.\necho \"needs_go=$needs_go\" >> \"$GITHUB_OUTPUT\"\necho \"Go reachability evidence needed: $needs_go\"\n\nif [ \"$count\" -eq 0 ]; then\n 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\"\nfi\n" + - if: steps.worklist.outputs.needs_go == 'true' + name: Vendor dependency source for the agent + run: "set -uo pipefail\nPKGS=/tmp/gh-aw/go-production-packages.txt\nrm -f \"$PKGS\" \"$PKGS.tmp\"\n\n# Deliberately not fatal. Missing evidence should degrade the assessment,\n# not cancel triage: the skill treats an absent artifact as an\n# unobtainable evidence item and caps confidence at Medium, which is\n# visible in the posted comment. A hard failure would post nothing at all.\nif ! go mod vendor; then\n echo \"::warning::go mod vendor failed; the agent has no reachability evidence this run.\"\n rm -rf vendor\n exit 0\nfi\n\n# `go list -deps` evaluates build constraints for one GOOS/GOARCH/cgo\n# combination, so a single invocation would miss platform-guarded imports\n# and understate what a change can reach. Union the exact release matrix\n# from .goreleaser.yml, including linux's CGO_ENABLED=0, so the evidence\n# describes what we actually ship. Today every combination yields the same\n# set, but that is a property of the current dependencies, not a guarantee.\nfor target in \\\n \"darwin amd64 1\" \"darwin arm64 1\" \\\n \"linux 386 0\" \"linux arm 0\" \"linux amd64 0\" \"linux arm64 0\" \\\n \"windows 386 1\" \"windows amd64 1\" \"windows arm64 1\"; do\n # shellcheck disable=SC2086\n set -- $target\n if ! GOOS=\"$1\" GOARCH=\"$2\" CGO_ENABLED=\"$3\" go list -deps ./cmd/gh >> \"$PKGS.tmp\"; then\n echo \"::warning::go list failed for GOOS=$1 GOARCH=$2; production package list is incomplete and will not be written.\"\n rm -f \"$PKGS.tmp\"\n exit 0\n fi\ndone\nsort -u \"$PKGS.tmp\" -o \"$PKGS\"\nrm -f \"$PKGS.tmp\"\n\necho \"Vendored $(grep -c '^# ' vendor/modules.txt) modules into vendor/\"\necho \"Shipped binary compiles $(wc -l < \"$PKGS\" | tr -d ' ') packages -> $PKGS\"" - name: Configure Git credentials env: diff --git a/.github/workflows/dependabot-triage.md b/.github/workflows/dependabot-triage.md index cfa825f7cf5..d62b5e2f137 100644 --- a/.github/workflows/dependabot-triage.md +++ b/.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,6 +190,18 @@ 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 @@ -194,10 +209,87 @@ steps: 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. diff --git a/acceptance/testdata/release/release-delete.txtar b/acceptance/testdata/release/release-delete.txtar new file mode 100644 index 00000000000..3eaf5c12bc6 --- /dev/null +++ b/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! diff --git a/pkg/cmd/release/delete-asset/delete_asset.go b/pkg/cmd/release/delete-asset/delete_asset.go index 3aedc3e3a46..f89e1fe7302 100644 --- a/pkg/cmd/release/delete-asset/delete_asset.go +++ b/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) } diff --git a/pkg/cmd/release/delete-asset/delete_asset_test.go b/pkg/cmd/release/delete-asset/delete_asset_test.go index e302ca3d1f5..62f1e0d078c 100644 --- a/pkg/cmd/release/delete-asset/delete_asset_test.go +++ b/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") +} diff --git a/pkg/cmd/release/delete/delete.go b/pkg/cmd/release/delete/delete.go index 108ebae7ece..e4d22be477e 100644 --- a/pkg/cmd/release/delete/delete.go +++ b/pkg/cmd/release/delete/delete.go @@ -7,7 +7,6 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/git" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/cmd/release/shared" @@ -93,7 +92,7 @@ func deleteRun(opts *DeleteOptions) error { } } - err = deleteRelease(httpClient, safeurl.NewImmutableSafeURL(release.APIURL)) + err = deleteRelease(httpClient, baseRepo.RepoHost(), safeurl.NewImmutableSafeURL(release.APIURL)) if err != nil { return err } @@ -122,42 +121,20 @@ func deleteRun(opts *DeleteOptions) error { return nil } -func deleteRelease(httpClient *http.Client, releaseURL safeurl.SafeURL) error { - req, err := http.NewRequest("DELETE", releaseURL.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 deleteRelease(httpClient *http.Client, host string, releaseURL 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, releaseURL.String(), nil, nil) } func deleteTag(httpClient *http.Client, baseRepo ghrepo.Interface, tagName string) error { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(baseRepo.RepoHost()), "repos", baseRepo.RepoOwner(), baseRepo.RepoName(), "git", "refs", fmt.Sprintf("tags/%s", tagName)) + path, err := safeurl.JoinPath("repos", baseRepo.RepoOwner(), baseRepo.RepoName(), "git", "refs", fmt.Sprintf("tags/%s", tagName)) if err != nil { return err } - req, err := http.NewRequest("DELETE", url.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 + // 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(baseRepo.RepoHost(), http.MethodDelete, path.String(), nil, nil) } diff --git a/pkg/cmd/release/delete/delete_test.go b/pkg/cmd/release/delete/delete_test.go index 13904ff3501..b7a80908a4f 100644 --- a/pkg/cmd/release/delete/delete_test.go +++ b/pkg/cmd/release/delete/delete_test.go @@ -7,10 +7,12 @@ import ( "testing" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/internal/run" + "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" @@ -241,3 +243,42 @@ func Test_deleteRun(t *testing.T) { }) } } + +func Test_deleteRelease_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/23456" && + req.URL.Host == "api.github.com" + }, + httpmock.StatusStringResponse(404, `{"message":"Not Found"}`), + ) + + httpClient := &http.Client{Transport: reg} + err := deleteRelease(httpClient, "example.com", safeurl.NewImmutableSafeURL("https://api.github.com/repos/OWNER/REPO/releases/23456")) + + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, http.StatusNotFound, httpErr.StatusCode) + assert.Contains(t, err.Error(), "HTTP 404") +} + +func Test_deleteTag_httpError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/tags%2Fv1.2.3"), + httpmock.StatusStringResponse(404, `{"message":"Not Found"}`), + ) + + httpClient := &http.Client{Transport: reg} + baseRepo, _ := ghrepo.FromFullName("OWNER/REPO") + err := deleteTag(httpClient, baseRepo, "v1.2.3") + + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, http.StatusNotFound, httpErr.StatusCode) + assert.Contains(t, err.Error(), "HTTP 404") +} diff --git a/pkg/cmd/release/edit/edit_test.go b/pkg/cmd/release/edit/edit_test.go index 180051d1271..3a59b7e5f6c 100644 --- a/pkg/cmd/release/edit/edit_test.go +++ b/pkg/cmd/release/edit/edit_test.go @@ -2,12 +2,14 @@ package edit import ( "bytes" + "errors" "fmt" "io" "net/http" "os" "testing" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmd/release/shared" "github.com/cli/cli/v2/pkg/cmdutil" @@ -480,6 +482,99 @@ func mockSuccessfulEditResponse(reg *httpmock.Registry, cb func(params map[strin reg.Register(matcher, responder) } +func Test_editRelease_httpError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + func(req *http.Request) bool { + return req.Method == http.MethodPatch && + req.URL.EscapedPath() == "/repos/OWNER/REPO/releases/12345" && + req.URL.Host == "api.github.com" + }, + httpmock.StatusStringResponse(404, `{"message":"Not Found"}`), + ) + + httpClient := &http.Client{Transport: reg} + release, err := editRelease(httpClient, ghrepo.New("OWNER", "REPO"), 12345, map[string]interface{}{"tag_name": "v1.2.3"}) + + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, http.StatusNotFound, httpErr.StatusCode) + assert.Contains(t, err.Error(), "HTTP 404") + assert.Nil(t, release) +} + +func Test_editRelease_decodeError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + func(req *http.Request) bool { + return req.Method == http.MethodPatch && + req.URL.EscapedPath() == "/repos/OWNER/REPO/releases/12345" && + req.URL.Host == "api.github.com" + }, + httpmock.StatusStringResponse(200, `{`), + ) + + httpClient := &http.Client{Transport: reg} + release, err := editRelease(httpClient, ghrepo.New("OWNER", "REPO"), 12345, map[string]interface{}{"tag_name": "v1.2.3"}) + + require.Error(t, err) + assert.NotNil(t, release) // decode was attempted - non-nil pointer even on decode error +} + +func Test_editRelease_204(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + func(req *http.Request) bool { + return req.Method == http.MethodPatch && + req.URL.EscapedPath() == "/repos/OWNER/REPO/releases/12345" && + req.URL.Host == "api.github.com" + }, + httpmock.StatusStringResponse(204, ""), + ) + + httpClient := &http.Client{Transport: reg} + release, err := editRelease(httpClient, ghrepo.New("OWNER", "REPO"), 12345, map[string]interface{}{"tag_name": "v1.2.3"}) + + require.Error(t, err) + assert.Contains(t, err.Error(), "unexpected end of JSON input") + assert.NotNil(t, release) +} + +func Test_editRelease_bodyReadError(t *testing.T) { + readErr := errors.New("read: connection reset by peer") + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + func(req *http.Request) bool { + return req.Method == http.MethodPatch && + req.URL.EscapedPath() == "/repos/OWNER/REPO/releases/12345" && + req.URL.Host == "api.github.com" + }, + func(_ *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: 200, + Body: io.NopCloser(errorReader{err: readErr}), + Header: http.Header{}, + }, nil + }, + ) + + httpClient := &http.Client{Transport: reg} + release, err := editRelease(httpClient, ghrepo.New("OWNER", "REPO"), 12345, map[string]interface{}{"tag_name": "v1.2.3"}) + + require.Error(t, err) + assert.ErrorIs(t, err, readErr) + assert.Nil(t, release) +} + +// errorReader always returns the given error on Read, used to simulate body read failures. +type errorReader struct{ err error } + +func (e errorReader) Read(_ []byte) (int, error) { return 0, e.err } + func boolPtr(b bool) *bool { return &b } diff --git a/pkg/cmd/release/edit/http.go b/pkg/cmd/release/edit/http.go index 291123ad3ea..4087fe23c39 100644 --- a/pkg/cmd/release/edit/http.go +++ b/pkg/cmd/release/edit/http.go @@ -33,6 +33,8 @@ func editRelease(httpClient *http.Client, repo ghrepo.Interface, releaseID int64 req.Header.Set("Content-Type", "application/json; charset=utf-8") + // TODO(api-client-rollout) + // This has been deferred from moving to api.Client because its return shape depends on the response status code, which api.Client.REST does not expose on success. resp, err := httpClient.Do(req) if err != nil { return nil, err diff --git a/pkg/cmd/run/download/http.go b/pkg/cmd/run/download/http.go index a832f924b20..645c0a9a7a7 100644 --- a/pkg/cmd/run/download/http.go +++ b/pkg/cmd/run/download/http.go @@ -29,6 +29,8 @@ func (p *apiPlatform) Download(url safeurl.SafeURL, dir safepaths.Absolute) erro } func downloadArtifact(httpClient *http.Client, url safeurl.SafeURL, destDir safepaths.Absolute) error { + // TODO(api-client-rollout) + // This has been deferred from moving to api.Client due to streaming the artifact ZIP response body to disk instead of decoding JSON. req, err := http.NewRequest("GET", url.String(), nil) if err != nil { return err diff --git a/pkg/cmd/run/shared/artifacts.go b/pkg/cmd/run/shared/artifacts.go index 36d0b39e73c..064b62ee8b1 100644 --- a/pkg/cmd/run/shared/artifacts.go +++ b/pkg/cmd/run/shared/artifacts.go @@ -1,13 +1,10 @@ package shared import ( - "encoding/json" "net/http" - "regexp" "strconv" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" ) @@ -26,14 +23,13 @@ type artifactsPayload struct { func ListArtifacts(httpClient *http.Client, repo ghrepo.Interface, runID string) ([]Artifact, error) { var results []Artifact - restPrefix := ghinstance.RESTPrefix(repo.RepoHost()) perPage := 100 - u, err := safeurl.JoinPathWithHostPrefix(restPrefix, "repos", repo.RepoOwner(), repo.RepoName(), "actions", "artifacts") + u, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "actions", "artifacts") if err != nil { return nil, err } if runID != "" { - u, err = safeurl.JoinPathWithHostPrefix(restPrefix, "repos", repo.RepoOwner(), repo.RepoName(), "actions", "runs", runID, "artifacts") + u, err = safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "actions", "runs", runID, "artifacts") if err != nil { return nil, err } @@ -41,9 +37,14 @@ func ListArtifacts(httpClient *http.Client, repo ghrepo.Interface, runID string) u.SetQuery("per_page", strconv.Itoa(perPage)) var pageURL safeurl.SafeURL = u + // 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 + client := api.NewClientFromHTTP(httpClient) + for { var payload artifactsPayload - nextURL, err := apiGet(httpClient, pageURL, &payload) + nextURL, err := client.RESTWithNext(repo.RepoHost(), http.MethodGet, pageURL.String(), nil, &payload) if err != nil { return nil, err } @@ -57,38 +58,3 @@ func ListArtifacts(httpClient *http.Client, repo ghrepo.Interface, runID string) return results, nil } - -func apiGet(httpClient *http.Client, url safeurl.SafeURL, data interface{}) (string, error) { - req, err := http.NewRequest("GET", url.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) - } - - dec := json.NewDecoder(resp.Body) - if err := dec.Decode(data); err != nil { - return "", err - } - - return findNextPage(resp), nil -} - -var linkRE = regexp.MustCompile(`<([^>]+)>;\s*rel="([^"]+)"`) - -func findNextPage(resp *http.Response) string { - for _, m := range linkRE.FindAllStringSubmatch(resp.Header.Get("Link"), -1) { - if len(m) > 2 && m[2] == "next" { - return m[1] - } - } - return "" -} diff --git a/pkg/cmd/run/shared/artifacts_test.go b/pkg/cmd/run/shared/artifacts_test.go index 31345598e92..c83613981fb 100644 --- a/pkg/cmd/run/shared/artifacts_test.go +++ b/pkg/cmd/run/shared/artifacts_test.go @@ -6,9 +6,11 @@ import ( "net/url" "testing" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/httpmock" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestDownloadWorkflowArtifactsPageinates(t *testing.T) { @@ -64,3 +66,24 @@ func TestDownloadWorkflowArtifactsPageinates(t *testing.T) { assert.NoError(t, err) assert.Equal(t, []Artifact{firstArtifact, secondArtifact}, result) } + +func TestListArtifactsReturnsAPIErrorMessage(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.QueryMatcher( + "GET", + "repos/OWNER/REPO/actions/artifacts", + url.Values{"per_page": []string{"100"}}, + ), + httpmock.StatusStringResponse(http.StatusNotFound, `{"message":"Not Found"}`), + ) + + _, err := ListArtifacts(&http.Client{Transport: reg}, ghrepo.New("OWNER", "REPO"), "") + + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + require.Equal(t, http.StatusNotFound, httpErr.StatusCode) + require.EqualError(t, err, "HTTP 404 (https://api.github.com/repos/OWNER/REPO/actions/artifacts?per_page=100)") +} diff --git a/pkg/cmd/run/view/logs.go b/pkg/cmd/run/view/logs.go index ab1232837bb..3a19e2df7b4 100644 --- a/pkg/cmd/run/view/logs.go +++ b/pkg/cmd/run/view/logs.go @@ -45,6 +45,8 @@ func (f *apiLogFetcher) GetLog() (io.ReadCloser, error) { return nil, err } + // TODO(api-client-rollout) + // This has been deferred from moving to api.Client due to returning the job log response body as an io.ReadCloser instead of decoding JSON. req, err := http.NewRequest("GET", logURL.String(), nil) if err != nil { return nil, err diff --git a/pkg/cmd/run/view/view.go b/pkg/cmd/run/view/view.go index 95cdc891291..efa0bc6af0b 100644 --- a/pkg/cmd/run/view/view.go +++ b/pkg/cmd/run/view/view.go @@ -471,6 +471,8 @@ func shouldFetchJobs(opts *ViewOptions) bool { } func getLog(httpClient *http.Client, logURL safeurl.SafeURL) (io.ReadCloser, error) { + // TODO(api-client-rollout) + // This has been deferred from moving to api.Client due to streaming the run log ZIP response body for archive processing instead of decoding JSON. req, err := http.NewRequest("GET", logURL.String(), nil) if err != nil { return nil, err