From f3afbb9172103999bf1f6b86d335c28e20e88ff0 Mon Sep 17 00:00:00 2001 From: Sayt-0 Date: Wed, 29 Jul 2026 12:40:18 +0200 Subject: [PATCH] feat: remediate reviewer cache permissions --- .../fix-reviewer-cache-permissions.yml | 485 ++++++++++ AGENTS.md | 1 + .../__tests__/fix-permission.test.ts | 873 ++++++++++++++++++ .../fix-permission.ts | 376 ++++++++ src/fix-reviewer-cache-permission/index.ts | 70 ++ tsup.config.ts | 1 + 6 files changed, 1806 insertions(+) create mode 100644 .github/workflows/fix-reviewer-cache-permissions.yml create mode 100644 src/fix-reviewer-cache-permission/__tests__/fix-permission.test.ts create mode 100644 src/fix-reviewer-cache-permission/fix-permission.ts create mode 100644 src/fix-reviewer-cache-permission/index.ts diff --git a/.github/workflows/fix-reviewer-cache-permissions.yml b/.github/workflows/fix-reviewer-cache-permissions.yml new file mode 100644 index 0000000..cf20c39 --- /dev/null +++ b/.github/workflows/fix-reviewer-cache-permissions.yml @@ -0,0 +1,485 @@ +# Copyright The Docker Agent Action authors +# SPDX-License-Identifier: Apache-2.0 + +name: Fix reviewer cache permissions + +# Remediation automation for consumer repos calling the PR-review reusable +# workflow (docker/docker-agent-action/.github/workflows/review-pr.yml) with +# `actions: read` on the calling job. +# +# The reusable workflow saves caches on behalf of the caller (review lock, +# review memory, binary cache), and cache writes require `actions: write`; +# with `actions: read` they fail with a 403. `write` implicitly includes +# `read`, so artifact/cache downloads keep working after the upgrade. +# +# The workflow searches org:docker for workflow files referencing the +# reusable workflow, clones each consumer, flips `actions: read` to +# `actions: write` in the permissions block of the calling job(s) only, and +# opens one PR per repo via signed commits. Consumers whose calling jobs all +# already have `actions: write` are reported as already compliant. Calling +# jobs the safe flip cannot make compliant (no permissions block, read-all, +# no actions entry, actions: none, unsupported YAML shapes) put the whole +# repo under "Needs manual review": no commit, PR, or any other GitHub write +# is made for it — even when another job in the repo was fixable — so a +# partial PR can never masquerade as a complete fix. +# +# Routing mirrors migrate-consumers.yml: direct PR when the machine user has +# write access, fork PR when it does not but forking is allowed, otherwise +# the repo is reported under Skipped for a manual update. Re-runs are +# idempotent: a stable branch is force-rebuilt from the default branch and an +# existing open PR is updated instead of duplicated. Dry-run (default ON) +# shows the diff and the direct/fork/skip route without any writes. +# +# All rewrite logic lives in src/fix-reviewer-cache-permission (TypeScript, +# unit tested) — this workflow only orchestrates discovery, cloning, and PRs. + +on: + workflow_dispatch: + inputs: + repos: + description: "Comma-separated allowlist of repos to process (e.g. docker/sailor,docker/compose). Empty = all discovered consumers." + required: false + type: string + default: "" + dry-run: + description: "Dry run: show the diffs that would be committed, but do not push commits or open PRs." + required: false + type: boolean + default: true + +permissions: + contents: read + id-token: write + +concurrency: + group: fix-reviewer-cache-permissions + cancel-in-progress: false + +jobs: + fix-reviewer-cache-permissions: + name: Fix consumer cache permissions + runs-on: ubuntu-latest + steps: + + - name: Checkout + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 + with: + ref: ${{ github.sha }} + persist-credentials: false + + - name: Setup pnpm + uses: pnpm/action-setup@8912a9102ac27614460f54aedde9e1e7f9aec20d # v6.0.5 + with: + run_install: false + + - name: Setup Node.js + uses: actions/setup-node@48b55a011bda9f5d6aeb4c2d9c7362e8dae4041e # v6.4.0 + with: + node-version: 24 + cache: pnpm + + # dist/ is gitignored on non-release refs, so the CLI tools (and the + # credentials bundle used by setup-credentials below) must be built + # from source before the composite action can run. + - name: Build CLI tools + run: pnpm install --frozen-lockfile && pnpm build + + - name: Setup credentials + uses: ./setup-credentials + + # setup-credentials already masks the token via core.setSecret, but the + # invariant lives in another module — re-mask explicitly here so this + # workflow stays safe even if that implementation detail changes. + - name: Mask app token + run: echo "::add-mask::$GITHUB_APP_TOKEN" + + - name: Discover consumer repos + id: discover + env: + GH_TOKEN: ${{ env.GITHUB_APP_TOKEN }} + REPO_ALLOWLIST: ${{ inputs.repos }} + run: | + set -euo pipefail + + # Search for references to the reviewer reusable workflow (any ref). + # GitHub code search treats the quoted string as a literal substring. + echo "Searching org:docker for review-pr.yml consumers..." + REPOS=$(gh api --method GET --paginate '/search/code?per_page=100' \ + -f q='org:docker "docker/docker-agent-action/.github/workflows/review-pr.yml@" language:YAML path:.github/workflows' \ + --jq '[.items[].repository.full_name] | unique | .[]') + + if [ -z "$REPOS" ]; then + echo "No consumer repos found." + echo "repos=" >> $GITHUB_OUTPUT + exit 0 + fi + + # Never process the action repos themselves: this repo references + # its own reusable workflow via local paths and contains this + # workflow's own search string, and the legacy repo's automation + # embeds the same string in a script block — neither is a consumer. + REPOS=$(printf '%s\n' "$REPOS" | grep -vxF \ + -e 'docker/docker-agent-action' \ + -e 'docker/cagent-action' || true) + + if [ -z "$REPOS" ]; then + echo "No consumer repos found after exclusions." + echo "repos=" >> $GITHUB_OUTPUT + exit 0 + fi + + # Apply the allowlist filter if provided (pilot runs). + if [ -n "$REPO_ALLOWLIST" ]; then + FILTERED="" + IFS=',' read -ra ALLOWED <<< "$REPO_ALLOWLIST" + while IFS= read -r REPO; do + for A in "${ALLOWED[@]}"; do + # Trim surrounding whitespace via parameter expansion — unlike + # xargs this does not interpret backslashes or quotes, so a + # malformed entry cannot corrupt the filtering. + A_TRIMMED="${A#"${A%%[![:space:]]*}"}" + A_TRIMMED="${A_TRIMMED%"${A_TRIMMED##*[![:space:]]}"}" + if [ "$REPO" = "$A_TRIMMED" ]; then + FILTERED+="$REPO"$'\n' + fi + done + done <<< "$REPOS" + REPOS=$(printf '%s' "$FILTERED") + if [ -z "$REPOS" ]; then + echo "::warning::Allowlist did not match any discovered consumer repos" + echo "repos=" >> $GITHUB_OUTPUT + exit 0 + fi + fi + + echo "Consumer repos to process:" + echo "$REPOS" + { + echo 'repos<> $GITHUB_OUTPUT + + - name: Fix permissions and open PRs + if: steps.discover.outputs.repos != '' + env: + GH_TOKEN: ${{ env.GITHUB_APP_TOKEN }} + REPOS: ${{ steps.discover.outputs.repos }} + DRY_RUN: ${{ inputs.dry-run }} + RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} + run: | + set -euo pipefail + + BRANCH="fix/reviewer-cache-permissions" + PR_TITLE="fix: allow reviewer cache writes" + FIX_CLI="$GITHUB_WORKSPACE/dist/fix-reviewer-cache-permission.js" + SUMMARY_CHANGED="" + SUMMARY_MANUAL="" + SUMMARY_SKIPPED="" + + # Single EXIT trap registered once, referencing a global — avoids + # re-registering a trap per loop iteration (traps don't stack; a + # single-quoted per-iteration trap would silently replace the + # previous handler). cleanup_workdir is also called explicitly on + # every skip/continue path and is a no-op when already cleaned. + CURRENT_WORK_DIR="" + cleanup_workdir() { + cd / + if [ -n "$CURRENT_WORK_DIR" ]; then + rm -rf "$CURRENT_WORK_DIR" + CURRENT_WORK_DIR="" + fi + } + trap cleanup_workdir EXIT + + while IFS= read -r REPO; do + [ -z "$REPO" ] && continue + echo "==========================================" + echo "Processing ${REPO}..." + echo "==========================================" + + WORK_DIR=$(mktemp -d) + CURRENT_WORK_DIR="$WORK_DIR" + if ! gh repo clone "$REPO" "$WORK_DIR" -- --depth=1 2>/dev/null; then + echo "::warning::Failed to clone $REPO — skipping (token may lack access)" + SUMMARY_SKIPPED+="- ${REPO} (clone failed)"$'\n' + cleanup_workdir + continue + fi + + cd "$WORK_DIR" + + # Find every workflow file referencing the reusable workflow. + # grep is only a cheap pre-filter — the tested CLI decides which + # files actually have a calling job with `actions: read`. grep -l + # exits 1 when nothing matches, so guard with || true. + MATCHING_FILES=$(grep -rlE --include='*.yml' --include='*.yaml' \ + 'docker/docker-agent-action/\.github/workflows/review-pr\.yml@' \ + .github/workflows/ 2>/dev/null || true) + if [ -z "$MATCHING_FILES" ]; then + echo "No reusable-workflow references found in $REPO — skipping" + SUMMARY_SKIPPED+="- ${REPO} (no local references)"$'\n' + cleanup_workdir + continue + fi + + # Rewrite the matching files via the tested TS tool. Build the + # argument list as a bash array (no eval, see AGENTS.md). The CLI + # prints one " " line per file — changed, + # needs-manual, compliant, skipped; a mixed file appears under + # both changed and needs-manual — and exits 1 on any per-file + # error, so a partial CHANGED_FILES list is never committed. + FILE_ARGS=() + while IFS= read -r F; do + FILE_ARGS+=("$F") + done <<< "$MATCHING_FILES" + + FIX_OUTPUT=$(node "$FIX_CLI" "${FILE_ARGS[@]}") || { + echo "::warning::fix-reviewer-cache-permission failed for $REPO — skipping" + SUMMARY_SKIPPED+="- ${REPO} (rewrite failed)"$'\n' + cleanup_workdir + continue + } + CHANGED_FILES=$(printf '%s\n' "$FIX_OUTPUT" | sed -n 's/^changed //p') + MANUAL_FILES=$(printf '%s\n' "$FIX_OUTPUT" | sed -n 's/^needs-manual //p') + COMPLIANT_FILES=$(printf '%s\n' "$FIX_OUTPUT" | sed -n 's/^compliant //p') + + # A calling job the safe flip cannot make compliant keeps failing + # with 403 even after the flip, so committing the fixable rest + # would open a PR that looks complete while it is not. Route the + # whole repo to a human instead, with no GitHub writes (no fork, + # no commit, no PR) — in dry-run and real mode alike. + if [ -n "$MANUAL_FILES" ]; then + echo "Files with calling jobs the safe flip cannot make compliant in $REPO:" + echo "$MANUAL_FILES" + echo "Not committing or opening a PR — a partial fix would hide the remaining 403s." + MANUAL_LIST="" + while IFS= read -r F; do + [ -z "$F" ] && continue + MANUAL_LIST+="${MANUAL_LIST:+, }${F}" + done <<< "$MANUAL_FILES" + SUMMARY_MANUAL+="- ${REPO}: ${MANUAL_LIST}"$'\n' + cleanup_workdir + continue + fi + + if [ -z "$CHANGED_FILES" ] || git diff --quiet; then + if [ -n "$COMPLIANT_FILES" ]; then + echo "No changes needed — every reviewer-calling job already has actions: write" + SUMMARY_SKIPPED+="- ${REPO} (already compliant — actions: write already set)"$'\n' + else + echo "No reviewer-calling jobs in the matching files — nothing to fix" + SUMMARY_SKIPPED+="- ${REPO} (no reviewer-calling jobs)"$'\n' + fi + cleanup_workdir + continue + fi + + echo "Changed files:" + echo "$CHANGED_FILES" + + # Resolve how this repo would be fixed (read-only) so a dry run + # reports the routing without performing any writes: + # direct: the machine user has write, so commit the branch into the + # repo and open a same-repo PR. + # fork: no write access. Fork under the machine user, commit on + # the fork, and open a cross-repo PR (head "owner:branch"). Needs + # only read on the upstream, like an external contribution. + # skip: no write access AND forking disabled. Cannot be fixed + # automatically; reported under Skipped for a manual update. + # Fetch the repo metadata in one call (race-free vs separate calls) + # and coerce missing fields with jq's `// false`: `gh ... --jq` prints + # the string "null" for an absent field, which is neither "true" nor + # "false" and would misroute a repo. default_branch comes from the + # same response; an empty value (or a failed call -> "{}") skips the + # repo rather than aborting the whole run under set -euo pipefail. + REPO_META=$(gh api "repos/$REPO" 2>/dev/null || echo '{}') + DEFAULT_BRANCH=$(printf '%s' "$REPO_META" | jq -r '.default_branch // empty') + if [ -z "$DEFAULT_BRANCH" ]; then + echo "::warning::Failed to resolve default branch for $REPO — skipping" + SUMMARY_SKIPPED+="- ${REPO} (default-branch lookup failed)"$'\n' + cleanup_workdir + continue + fi + CAN_PUSH=$(printf '%s' "$REPO_META" | jq -r '.permissions.push // false') + ALLOW_FORKING=$(printf '%s' "$REPO_META" | jq -r '.allow_forking // false') + if [ "$CAN_PUSH" = "true" ]; then + ROUTE="direct" + elif [ "$ALLOW_FORKING" = "true" ]; then + ROUTE="fork" + else + ROUTE="skip" + fi + + if [ "$DRY_RUN" = "true" ]; then + N_FILES=$(echo "$CHANGED_FILES" | wc -l | tr -d ' ') + echo "DRY RUN — route: ${ROUTE}; diff that would be committed:" + git --no-pager diff + if [ "$ROUTE" = "skip" ]; then + SUMMARY_SKIPPED+="- ${REPO} (dry run — no write access, forking disabled)"$'\n' + else + SUMMARY_CHANGED+="- ${REPO} (dry run — ${ROUTE} PR, ${N_FILES} file(s))"$'\n' + fi + cleanup_workdir + continue + fi + + # Resolve the repo + head ref to commit to, per the route above. + if [ "$ROUTE" = "skip" ]; then + echo "::warning::No write access and forking disabled on $REPO — skipping (needs a write grant or manual update)" + SUMMARY_SKIPPED+="- ${REPO} (no write access, forking disabled)"$'\n' + cleanup_workdir + continue + elif [ "$ROUTE" = "direct" ]; then + COMMIT_REPO="$REPO" + PR_HEAD="$BRANCH" + else + FORK_OWNER=$(gh api user --jq .login) || { + echo "::warning::Could not resolve the machine-user login for $REPO — skipping" + SUMMARY_SKIPPED+="- ${REPO} (could not resolve fork owner)"$'\n' + cleanup_workdir + continue + } + if [ -z "$FORK_OWNER" ]; then + echo "::warning::Machine-user login resolved to empty for $REPO — skipping" + SUMMARY_SKIPPED+="- ${REPO} (empty fork owner)"$'\n' + cleanup_workdir + continue + fi + FORK="${FORK_OWNER}/$(basename "$REPO")" + echo "No write access on $REPO — opening a fork PR from ${FORK}" + # Fork is idempotent (no-op when it already exists) and created + # asynchronously, so poll until the API can see it. The exit code is + # ignored on purpose (some gh versions return non-zero when the fork + # already exists), but stderr is kept so a genuine fork failure is + # distinguishable from slow async readiness in the skip warning. + FORK_ERR=$(gh repo fork "$REPO" --clone=false --default-branch-only 2>&1 >/dev/null || true) + FORK_READY="" + for _ in $(seq 1 10); do + if gh api "repos/$FORK" >/dev/null 2>&1; then FORK_READY=1; break; fi + sleep 3 + done + if [ -z "$FORK_READY" ]; then + echo "::warning::Fork ${FORK} did not become available — skipping $REPO${FORK_ERR:+ (fork error: $FORK_ERR)}" + SUMMARY_SKIPPED+="- ${REPO} (fork not ready)"$'\n' + cleanup_workdir + continue + fi + # Guard against a name collision: proceed only if $FORK is really a + # fork of $REPO. gh can rename a fork, and the machine user may own + # an unrelated repo of the same basename — committing to the wrong + # repo must never happen. + FORK_PARENT=$(gh api "repos/$FORK" --jq '.parent.full_name // empty' 2>/dev/null || true) + if [ "$FORK_PARENT" != "$REPO" ]; then + echo "::warning::${FORK} is not a fork of ${REPO} (parent='${FORK_PARENT:-none}') — skipping to avoid writing to the wrong repo" + SUMMARY_SKIPPED+="- ${REPO} (fork name collision)"$'\n' + cleanup_workdir + continue + fi + # Force-sync the fork's default branch to upstream so the cross-repo + # PR diff shows only the fix, not drift from a stale fork. + gh repo sync "$FORK" --branch "$DEFAULT_BRANCH" --force >/dev/null 2>&1 || true + COMMIT_REPO="$FORK" + PR_HEAD="${FORK_OWNER}:${BRANCH}" + fi + + # Create one signed commit covering all changed files, on whichever + # repo was resolved above (the upstream when we have write, else the + # fork). --force rebuilds the stable branch from the default branch + # tip, keeping re-runs idempotent. + ADD_ARGS=() + while IFS= read -r F; do + ADD_ARGS+=(--add "$F") + done <<< "$CHANGED_FILES" + + COMMIT_OID=$(GITHUB_TOKEN="${GH_TOKEN}" node "$GITHUB_WORKSPACE/dist/signed-commit.js" \ + --repo "$COMMIT_REPO" \ + --branch "$BRANCH" \ + --base-ref "$DEFAULT_BRANCH" \ + --force \ + --message "$PR_TITLE" \ + "${ADD_ARGS[@]}") || { + echo "::warning::Failed to create signed commit in $COMMIT_REPO (may lack write access)" + SUMMARY_SKIPPED+="- ${REPO} (commit failed)"$'\n' + cleanup_workdir + continue + } + + echo "Signed commit: $COMMIT_OID" + + # Look up an existing open PR for idempotent re-runs. `gh pr list + # --head` matches a branch NAME only and does NOT support the + # "owner:branch" form, so a fork PR is looked up via the REST pulls + # endpoint, whose head=owner:branch filter does. `// empty` keeps an + # absent PR from becoming the literal string "null". + if [ "$ROUTE" = "fork" ]; then + EXISTING_PR=$(gh api -X GET "repos/$REPO/pulls" -f state=open -f head="${FORK_OWNER}:${BRANCH}" --jq '.[0].number // empty' 2>/dev/null || true) + else + EXISTING_PR=$(gh pr list --repo "$REPO" --head "$BRANCH" --state open --json number --jq '.[0].number // empty') + fi + + printf -v PR_BODY '%s\n\n%s\n%s\n%s\n\n%s\n\n%s' \ + "## Summary" \ + "The \`docker/docker-agent-action\` PR-review reusable workflow saves caches on behalf of the calling job (review lock, review memory, binary cache), and cache writes require \`actions: write\` on the caller — with \`actions: read\` they fail with a 403." \ + "This PR upgrades \`actions: read\` to \`actions: write\` in the job(s) calling the reusable workflow." \ + "\`actions: write\` implicitly includes \`read\`, so artifact and cache downloads keep working." \ + "No other permission is changed." \ + "> Auto-generated by the [fix-reviewer-cache-permissions](${RUN_URL}) workflow." + + if [ -n "$EXISTING_PR" ]; then + echo "Updating existing PR #$EXISTING_PR in $REPO" + gh pr edit "$EXISTING_PR" --repo "$REPO" \ + --title "$PR_TITLE" \ + --body "$PR_BODY" 2>&1 || echo "::warning::Failed to update PR #$EXISTING_PR in $REPO (may be non-fatal)" + PR_URL=$(gh pr view "$EXISTING_PR" --repo "$REPO" --json url --jq .url 2>/dev/null || echo "") + else + echo "Creating new PR in $REPO" + PR_URL=$(gh pr create --repo "$REPO" \ + --head "$PR_HEAD" \ + --base "$DEFAULT_BRANCH" \ + --title "$PR_TITLE" \ + --body "$PR_BODY") || { + echo "::warning::Failed to create PR in $REPO" + PR_URL="" + } + fi + + # Reviewer assignment is best-effort and decoupled from PR + # creation: `gh pr create --reviewer` fails the ENTIRE create when + # the reviewer is not a collaborator of the consumer repo, which + # would silently drop the fix PR. + if [ -n "$PR_URL" ]; then + gh pr edit "$PR_URL" --add-reviewer "derekmisler" 2>&1 \ + || echo "::warning::Could not add reviewer on $PR_URL (non-fatal)" + fi + + SUMMARY_CHANGED+="- ${REPO} ${PR_URL:+(${PR_URL})}"$'\n' + + cleanup_workdir + echo "" + done <<< "$REPOS" + + # Job summary for triage. + { + echo "## Fix reviewer cache permissions — $([ "$DRY_RUN" = "true" ] && echo 'DRY RUN' || echo 'EXECUTED')" + echo "" + if [ -n "$SUMMARY_CHANGED" ]; then + echo "### PRs opened / repos with changes" + printf '%s' "$SUMMARY_CHANGED" + echo "" + fi + if [ -n "$SUMMARY_MANUAL" ]; then + echo "### Needs manual review" + echo "" + echo "Calling jobs in the listed files cannot be made compliant by the safe flip (no permissions block, read-all, no actions entry, actions: none, or unsupported YAML). No commit or PR was made for these repos — they still need a manual permissions fix." + echo "" + printf '%s' "$SUMMARY_MANUAL" + echo "" + fi + if [ -n "$SUMMARY_SKIPPED" ]; then + echo "### Skipped" + printf '%s' "$SUMMARY_SKIPPED" + fi + } >> "$GITHUB_STEP_SUMMARY" + + echo "Done." diff --git a/AGENTS.md b/AGENTS.md index 5fc4c1b..9756b87 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -200,6 +200,7 @@ The action runs untrusted input (PR titles, bodies, comments, diffs) through an | `update-consumers.yml` | Pushes version updates to downstream consumer repos. | | `migrate-consumers.yml` | Consumer migration to the new repo: opens PRs across consumer repos rewriting `docker/cagent-action` refs to `docker/docker-agent-action` (incremental, no deadline — old repo stays live; dry-run by default, `repos` allowlist for pilots). | | `manual-test-pirate-agent.yml` | Manual smoke test with a toy agent. | +| `fix-reviewer-cache-permissions.yml` | Consumer remediation: opens PRs across consumer repos upgrading `actions: read` to `actions: write` on the job calling the review-pr reusable workflow (cache writes need `write`; rewrite logic in `src/fix-reviewer-cache-permission/`; dry-run by default, `repos` allowlist for pilots). | ## Common tasks (cheat sheet) diff --git a/src/fix-reviewer-cache-permission/__tests__/fix-permission.test.ts b/src/fix-reviewer-cache-permission/__tests__/fix-permission.test.ts new file mode 100644 index 0000000..63348b3 --- /dev/null +++ b/src/fix-reviewer-cache-permission/__tests__/fix-permission.test.ts @@ -0,0 +1,873 @@ +// Copyright The Docker Agent Action authors +// SPDX-License-Identifier: Apache-2.0 + +/** + * Unit tests for src/fix-reviewer-cache-permission. + * + * Covers: + * - the canonical 1-workflow and 2-workflow consumer patterns from + * .agents/skills/add-pr-reviewer-to-repo/SKILL.md + * - ref shapes (SHA-pinned with comment, tag, branch), quoting, key order + * - comment/indentation/CRLF preservation (byte-for-byte except the value) + * - scoping: only the reviewer-calling job's permissions block is touched — + * never a top-level permissions block, another job, or embedded scripts + * - classification: jobs already on `actions: write` are distinguished + * from jobs the safe flip cannot fix (no permissions block, + * read-all/write-all, no actions entry, actions: none, multi-line flow + * mappings), including mixed multi-job files that are flipped AND still + * flagged for manual review + * - idempotency + * - applyFix I/O wrapper: in-place rewrite, changed/compliant/manual/ + * skipped classification (mixed files land in two buckets), per-file + * error collection + * - statusLines: the machine-readable stdout contract the + * fix-reviewer-cache-permissions workflow greps + */ +import { readFileSync } from 'node:fs'; +import { mkdtemp, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { describe, expect, it } from 'vitest'; +import { + applyFix, + fixReviewerCachePermission, + REVIEWER_WORKFLOW_PATH, + statusLines, +} from '../fix-permission.js'; + +const SHA = '3f5dc9969f307d3c76acb7e9ccaefdd96bd62f4b'; + +/** Canonical 1-workflow consumer (SKILL.md §4a) with a given actions value. */ +function oneWorkflowConsumer(actionsLine: string, ref = 'v2.0.0'): string { + return [ + 'name: PR Review', + 'on:', + ' pull_request:', + ' types: [ready_for_review, opened, review_requested]', + '', + 'permissions:', + ' contents: read', + '', + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@${ref}`, + ' permissions:', + ' contents: read # Read repository files and PR diffs', + ' pull-requests: write # Post review comments', + ' issues: write # Create security incident issues if secrets detected', + ' checks: write # (Optional) Show review progress as a check run', + ' id-token: write # Required for OIDC authentication to AWS Secrets Manager', + actionsLine, + '', + ].join('\n'); +} + +// ═════════════════════════════════════════════════════════════════════════════ +// Basic fix behaviour +// ═════════════════════════════════════════════════════════════════════════════ + +describe('fixReviewerCachePermission — basic fix', () => { + it('flips actions: read to actions: write in the calling job', () => { + const input = oneWorkflowConsumer(' actions: read'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(true); + expect(result.hasReviewerJob).toBe(true); + expect(result.replacedCount).toBe(1); + expect(result.manualJobs).toEqual([]); + expect(result.content).toBe(oneWorkflowConsumer(' actions: write')); + }); + + it('preserves a trailing comment on the actions line', () => { + const input = oneWorkflowConsumer(' actions: read # Cache and artifact access'); + const result = fixReviewerCachePermission(input); + expect(result.content).toBe( + oneWorkflowConsumer(' actions: write # Cache and artifact access'), + ); + }); + + it('changes nothing else in the file (byte-for-byte)', () => { + const input = oneWorkflowConsumer(' actions: read'); + const result = fixReviewerCachePermission(input); + const inLines = input.split('\n'); + const outLines = result.content.split('\n'); + expect(outLines).toHaveLength(inLines.length); + for (let i = 0; i < inLines.length; i++) { + if (inLines[i] === ' actions: read') { + expect(outLines[i]).toBe(' actions: write'); + } else { + expect(outLines[i]).toBe(inLines[i]); + } + } + }); + + it.each([ + ['SHA-pinned with version comment', `${SHA} # v2.0.0`], + ['SHA-pinned', SHA], + ['tag', 'v2.0.0'], + ['branch', 'main'], + ])('matches the reusable workflow ref: %s', (_name, ref) => { + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@${ref}`, + ' permissions:', + ' actions: read', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(true); + expect(result.content).toContain(' actions: write'); + }); + + it('handles a quoted uses value', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: "${REVIEWER_WORKFLOW_PATH}@v2.0.0"`, + ' permissions:', + ' actions: read', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(true); + }); + + it.each([ + ['double-quoted', ' actions: "read"', ' actions: "write"'], + ['single-quoted', " actions: 'read'", " actions: 'write'"], + ])('handles %s permission values', (_name, before, after) => { + const input = oneWorkflowConsumer(before); + const result = fixReviewerCachePermission(input); + expect(result.content).toBe(oneWorkflowConsumer(after)); + }); + + it('handles permissions listed before uses (key order is not fixed)', () => { + const input = [ + 'jobs:', + ' review:', + ' permissions:', + ' contents: read', + ' actions: read', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' secrets: inherit', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(true); + expect(result.content).toContain(' actions: write'); + expect(result.content).toContain(' contents: read'); + }); + + it('handles blank lines and comment lines inside the permissions block', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions:', + ' contents: read', + '', + ' # cache access', + ' actions: read', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(true); + expect(result.content).toContain(' # cache access\n actions: write'); + }); + + it('handles a comment after the permissions: key', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions: # least privilege', + ' actions: read', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(true); + }); + + it('handles a single-line flow mapping', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions: { contents: read, actions: read, id-token: write }', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(true); + expect(result.content).toContain( + ' permissions: { contents: read, actions: write, id-token: write }', + ); + }); + + it('handles a flow mapping where actions is the only entry', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions: {actions: read}', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.content).toContain(' permissions: {actions: write}'); + }); + + it('preserves CRLF line endings', () => { + const input = oneWorkflowConsumer(' actions: read').replace(/\n/g, '\r\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(true); + expect(result.content).toBe(oneWorkflowConsumer(' actions: write').replace(/\n/g, '\r\n')); + }); + + it('fixes deeper-indented (4-space) consumer files', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions:', + ' actions: read', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(true); + expect(result.content).toContain(' actions: write'); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════════ +// Scoping — never modify anything but the calling job's permissions +// ═════════════════════════════════════════════════════════════════════════════ + +describe('fixReviewerCachePermission — scoping', () => { + it('never modifies a top-level permissions block', () => { + const input = [ + 'name: PR Review', + 'permissions:', + ' contents: read', + ' actions: read', + '', + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions:', + ' actions: read', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.replacedCount).toBe(1); + const lines = result.content.split('\n'); + expect(lines[3]).toBe(' actions: read'); // top-level untouched + expect(lines[9]).toBe(' actions: write'); + }); + + it('never modifies a job that does not call the reviewer workflow', () => { + const input = [ + 'jobs:', + ' other:', + ' runs-on: ubuntu-latest', + ' permissions:', + ' actions: read', + ' steps:', + ' - run: echo hi', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions:', + ' actions: read', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.replacedCount).toBe(1); + const lines = result.content.split('\n'); + expect(lines[4]).toBe(' actions: read'); // other job untouched + expect(lines[10]).toBe(' actions: write'); + }); + + it('fixes every job that calls the reviewer workflow', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions:', + ' actions: read', + ' review-again:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@main`, + ' permissions:', + ' actions: read', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.replacedCount).toBe(2); + expect(result.manualJobs).toEqual([]); + expect(result.content).not.toContain('actions: read'); + }); + + it('ignores step-level uses of the root action', () => { + const input = [ + 'jobs:', + ' agent:', + ' runs-on: ubuntu-latest', + ' permissions:', + ' actions: read', + ' steps:', + ` - uses: docker/docker-agent-action@${SHA} # v2.0.0`, + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + expect(result.hasReviewerJob).toBe(false); + }); + + it('ignores a local self-reference to review-pr.yml (this repo dogfooding)', () => { + const input = [ + 'jobs:', + ' review:', + ' uses: ./.github/workflows/review-pr.yml', + ' permissions:', + ' actions: read', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + expect(result.hasReviewerJob).toBe(false); + }); + + it('ignores the old docker/cagent-action slug', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: docker/cagent-action/.github/workflows/review-pr.yml@${SHA}`, + ' permissions:', + ' actions: read', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + expect(result.hasReviewerJob).toBe(false); + }); + + it('ignores other reusable workflows from the same repo', () => { + const input = [ + 'jobs:', + ' e2e:', + ` uses: docker/docker-agent-action/.github/workflows/test-e2e.yml@${SHA}`, + ' permissions:', + ' actions: read', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + expect(result.hasReviewerJob).toBe(false); + }); + + it('ignores workflow-shaped text inside a run block scalar', () => { + const input = [ + 'jobs:', + ' helper:', + ' runs-on: ubuntu-latest', + ' steps:', + ' - run: |', + " cat <<'EOF' > fake.yml", + ' jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions:', + ' actions: read', + ' EOF', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + expect(result.hasReviewerJob).toBe(false); + }); + + it('does not touch an actions: read entry in a sibling block after jobs', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions:', + ' actions: read', + 'env:', + ' FOO: bar', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.replacedCount).toBe(1); + expect(result.content).toContain(' FOO: bar'); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════════ +// Already compliant — every reviewer-calling job has actions: write +// ═════════════════════════════════════════════════════════════════════════════ + +describe('fixReviewerCachePermission — already compliant', () => { + it('is a no-op when actions: write is already set', () => { + const input = oneWorkflowConsumer(' actions: write # Cache read/write'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + expect(result.hasReviewerJob).toBe(true); + expect(result.manualJobs).toEqual([]); + expect(result.content).toBe(input); + }); + + it('recognises a quoted actions: "write" as compliant', () => { + const result = fixReviewerCachePermission(oneWorkflowConsumer(' actions: "write"')); + expect(result.changed).toBe(false); + expect(result.hasReviewerJob).toBe(true); + expect(result.manualJobs).toEqual([]); + }); + + it('recognises actions: write inside a single-line flow mapping as compliant', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions: { contents: read, actions: write }', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + expect(result.hasReviewerJob).toBe(true); + expect(result.manualJobs).toEqual([]); + }); + + it('is idempotent: running twice produces the same output and reports compliant', () => { + const input = oneWorkflowConsumer(' actions: read'); + const once = fixReviewerCachePermission(input); + const twice = fixReviewerCachePermission(once.content); + expect(twice.changed).toBe(false); + expect(twice.manualJobs).toEqual([]); + expect(twice.content).toBe(once.content); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════════ +// Needs manual review — reviewer jobs the safe flip cannot make compliant. +// ═════════════════════════════════════════════════════════════════════════════ + +describe('fixReviewerCachePermission — needs manual review', () => { + it('reports a calling job without a permissions block (inserting keys is out of scope)', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' secrets: inherit', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + expect(result.hasReviewerJob).toBe(true); + expect(result.manualJobs).toEqual(['review']); + expect(result.content).toBe(input); + }); + + it('reports permissions: read-all (not a per-scope mapping the flip can edit)', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions: read-all', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + expect(result.hasReviewerJob).toBe(true); + expect(result.manualJobs).toEqual(['review']); + }); + + it('reports permissions: write-all (only an explicit actions: write counts as compliant)', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions: write-all', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + expect(result.manualJobs).toEqual(['review']); + }); + + it('reports an explicit actions: none instead of overriding it silently', () => { + const input = oneWorkflowConsumer(' actions: none'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + expect(result.hasReviewerJob).toBe(true); + expect(result.manualJobs).toEqual(['review']); + expect(result.content).toBe(input); + }); + + it('reports a permissions block without an actions entry', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions:', + ' contents: read', + ' pull-requests: write', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + expect(result.hasReviewerJob).toBe(true); + expect(result.manualJobs).toEqual(['review']); + }); + + it('reports a single-line flow mapping without an actions entry', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions: { contents: read, pull-requests: write }', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + expect(result.manualJobs).toEqual(['review']); + }); + + it('reports a multi-line flow mapping as unsupported (documented limitation)', () => { + // The scanner only recognises single-line flow mappings; spanning lines + // is valid YAML but out of scope, so the job must surface as manual — + // and the file must NOT be half-edited. + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions: {', + ' actions: read }', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + expect(result.manualJobs).toEqual(['review']); + expect(result.content).toBe(input); + }); + + it('flips the fixable job AND reports the non-fixable one in a mixed file', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions:', + ' actions: read', + ' review-fork:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@main`, + ' secrets: inherit', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(true); + expect(result.replacedCount).toBe(1); + expect(result.content).toContain(' actions: write'); + expect(result.manualJobs).toEqual(['review-fork']); + }); + + it('reports the manual job even when another job is already compliant', () => { + const input = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions:', + ' actions: write', + ' review-fork:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@main`, + ' permissions: read-all', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + expect(result.hasReviewerJob).toBe(true); + expect(result.manualJobs).toEqual(['review-fork']); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════════ +// No reviewer job / degenerate files +// ═════════════════════════════════════════════════════════════════════════════ + +describe('fixReviewerCachePermission — no reviewer job', () => { + it('returns unchanged for a file with no jobs section', () => { + const input = 'name: CI\non: push\n'; + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + expect(result.hasReviewerJob).toBe(false); + expect(result.manualJobs).toEqual([]); + expect(result.content).toBe(input); + }); + + it('returns unchanged for an empty jobs section', () => { + const input = 'name: CI\non: push\njobs:\n'; + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(false); + }); + + it('returns unchanged for an empty file', () => { + const result = fixReviewerCachePermission(''); + expect(result.changed).toBe(false); + expect(result.content).toBe(''); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════════ +// Realistic consumer file shapes +// ═════════════════════════════════════════════════════════════════════════════ + +describe('fixReviewerCachePermission — realistic consumer workflows', () => { + it('2-workflow fork pattern with a multi-line if condition', () => { + const input = [ + 'name: PR Review', + 'on:', + ' issue_comment:', + ' types: [created]', + ' workflow_run:', + ' workflows: ["PR Review - Trigger"]', + ' types: [completed]', + '', + 'permissions:', + ' contents: read', + '', + 'jobs:', + ' review:', + ' if: |', + " (github.event_name == 'issue_comment' &&", + " github.event.comment.user.login != 'docker-agent' &&", + " github.event.comment.user.type != 'Bot') ||", + " github.event.workflow_run.conclusion == 'success'", + ` uses: ${REVIEWER_WORKFLOW_PATH}@${SHA} # v2.0.0`, + ' permissions:', + ' contents: read # Read repository files and PR diffs', + ' pull-requests: write # Post review comments', + ' issues: write # Create security incident issues if secrets detected', + ' checks: write # (Optional) Show review progress as a check run', + ' id-token: write # Required for OIDC authentication to AWS Secrets Manager', + ' actions: read # download-artifact across workflow_run boundary', + ' with:', + // biome-ignore lint/suspicious/noTemplateCurlyInString: GitHub Actions expression in a test fixture + " trigger-run-id: ${{ github.event_name == 'workflow_run' && format('{0}', github.event.workflow_run.id) || '' }}", + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.changed).toBe(true); + expect(result.replacedCount).toBe(1); + expect(result.content).toContain( + ' actions: write # download-artifact across workflow_run boundary', + ); + expect(result.content).toContain(' contents: read\n'); // top-level untouched + }); + + it('caller with trigger job and reviewer job in the same file', () => { + const input = [ + 'jobs:', + ' save-context:', + ' runs-on: ubuntu-latest', + ' permissions:', + ' actions: read', + ' steps:', + ' - name: Save event context', + ' run: echo saved', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions:', + ' contents: read', + ' actions: read', + ' secrets: inherit', + '', + ].join('\n'); + const result = fixReviewerCachePermission(input); + expect(result.replacedCount).toBe(1); + const lines = result.content.split('\n'); + expect(lines[4]).toBe(' actions: read'); // save-context untouched + expect(lines[12]).toBe(' actions: write'); + }); +}); + +// ═════════════════════════════════════════════════════════════════════════════ +// applyFix — I/O behaviour +// ═════════════════════════════════════════════════════════════════════════════ + +describe('applyFix — I/O behaviour', () => { + async function makeTmpDir(): Promise { + return mkdtemp(join(tmpdir(), 'fix-reviewer-cache-permission-test-')); + } + + it('rewrites files in-place and classifies changed/compliant/manual/skipped', async () => { + const dir = await makeTmpDir(); + try { + const toFix = join(dir, 'pr-review.yml'); + const alreadyFixed = join(dir, 'pr-review-fixed.yaml'); + const manual = join(dir, 'pr-review-read-all.yml'); + const unrelated = join(dir, 'ci.yml'); + const readAll = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions: read-all', + '', + ].join('\n'); + await writeFile(toFix, oneWorkflowConsumer(' actions: read'), 'utf-8'); + await writeFile(alreadyFixed, oneWorkflowConsumer(' actions: write'), 'utf-8'); + await writeFile(manual, readAll, 'utf-8'); + await writeFile(unrelated, 'name: CI\non: push\njobs:\n build:\n steps: []\n', 'utf-8'); + + const result = applyFix([toFix, alreadyFixed, manual, unrelated]); + + expect(result.changedFiles).toEqual([toFix]); + expect(result.compliantFiles).toEqual([alreadyFixed]); + expect(result.manualFiles).toEqual([manual]); + expect(result.skippedFiles).toEqual([unrelated]); + expect(result.errors).toHaveLength(0); + expect(readFileSync(toFix, 'utf-8')).toBe(oneWorkflowConsumer(' actions: write')); + expect(readFileSync(alreadyFixed, 'utf-8')).toBe(oneWorkflowConsumer(' actions: write')); + expect(readFileSync(manual, 'utf-8')).toBe(readAll); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + it('puts a mixed file in BOTH changedFiles and manualFiles (flipped but unfinished)', async () => { + const dir = await makeTmpDir(); + try { + const mixed = join(dir, 'pr-review.yml'); + const content = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions:', + ' actions: read', + ' review-fork:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@main`, + ' secrets: inherit', + '', + ].join('\n'); + await writeFile(mixed, content, 'utf-8'); + + const result = applyFix([mixed]); + + expect(result.changedFiles).toEqual([mixed]); + expect(result.manualFiles).toEqual([mixed]); + expect(result.compliantFiles).toHaveLength(0); + expect(result.skippedFiles).toHaveLength(0); + // The safe flip IS applied on disk even though the file stays flagged. + expect(readFileSync(mixed, 'utf-8')).toContain(' actions: write'); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + it('collects per-file errors without aborting the remaining files', async () => { + const dir = await makeTmpDir(); + try { + const good = join(dir, 'good.yml'); + const missing = join(dir, 'does-not-exist.yml'); + await writeFile(good, oneWorkflowConsumer(' actions: read'), 'utf-8'); + + // The failing file comes FIRST — the good file after it must still be processed. + const result = applyFix([missing, good]); + + expect(result.errors).toHaveLength(1); + expect(result.errors[0].file).toBe(missing); + expect(result.changedFiles).toEqual([good]); + expect(readFileSync(good, 'utf-8')).toBe(oneWorkflowConsumer(' actions: write')); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + it('does not rewrite files that need no change (mtime-safe by content check)', async () => { + const dir = await makeTmpDir(); + try { + const f = join(dir, 'no-permissions.yml'); + const content = [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' secrets: inherit', + '', + ].join('\n'); + await writeFile(f, content, 'utf-8'); + + const result = applyFix([f]); + + expect(result.changedFiles).toHaveLength(0); + // No permissions block: not fixable by the safe flip, so it must land + // in manualFiles — never in compliantFiles. + expect(result.manualFiles).toEqual([f]); + expect(result.compliantFiles).toHaveLength(0); + expect(readFileSync(f, 'utf-8')).toBe(content); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); +}); + +// ═════════════════════════════════════════════════════════════════════════════ +// statusLines — the CLI stdout contract grepped by the workflow +// ═════════════════════════════════════════════════════════════════════════════ + +describe('statusLines — CLI stdout contract', () => { + it('renders one status-prefixed line per file, in bucket order', () => { + const lines = statusLines({ + changedFiles: ['a.yml'], + manualFiles: ['b.yml'], + compliantFiles: ['c.yml'], + skippedFiles: ['d.yml'], + errors: [], + }); + expect(lines).toEqual([ + 'changed a.yml', + 'needs-manual b.yml', + 'compliant c.yml', + 'skipped d.yml', + ]); + }); + + it('emits BOTH changed and needs-manual for a mixed file', () => { + const lines = statusLines({ + changedFiles: ['mixed.yml'], + manualFiles: ['mixed.yml'], + compliantFiles: [], + skippedFiles: [], + errors: [], + }); + expect(lines).toEqual(['changed mixed.yml', 'needs-manual mixed.yml']); + }); + + it('matches the end-to-end applyFix output for a mixed + compliant set', async () => { + const dir = await mkdtemp(join(tmpdir(), 'fix-reviewer-cache-permission-test-')); + try { + const mixed = join(dir, 'mixed.yml'); + const compliant = join(dir, 'ok.yml'); + await writeFile( + mixed, + [ + 'jobs:', + ' review:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@v2.0.0`, + ' permissions:', + ' actions: read', + ' review-fork:', + ` uses: ${REVIEWER_WORKFLOW_PATH}@main`, + ' permissions: read-all', + '', + ].join('\n'), + 'utf-8', + ); + await writeFile(compliant, oneWorkflowConsumer(' actions: write'), 'utf-8'); + + const lines = statusLines(applyFix([mixed, compliant])); + + expect(lines).toEqual([ + `changed ${mixed}`, + `needs-manual ${mixed}`, + `compliant ${compliant}`, + ]); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/src/fix-reviewer-cache-permission/fix-permission.ts b/src/fix-reviewer-cache-permission/fix-permission.ts new file mode 100644 index 0000000..caa8a37 --- /dev/null +++ b/src/fix-reviewer-cache-permission/fix-permission.ts @@ -0,0 +1,376 @@ +// Copyright The Docker Agent Action authors +// SPDX-License-Identifier: Apache-2.0 + +/** + * fix-reviewer-cache-permission — core logic for upgrading `actions: read` + * to `actions: write` in consumer workflow files that call the PR-review + * reusable workflow (`docker/docker-agent-action/.github/workflows/review-pr.yml`). + * + * The reusable workflow saves caches (review lock, memory, binary cache) on + * behalf of the caller, and cache writes fail with `actions: read`. The fix + * is a single-value flip in the `permissions` block of the CALLING job — + * `actions: write` implicitly includes read, so artifact/cache downloads + * keep working. + * + * Deliberately not YAML-parser based: consumer files must be preserved + * byte-for-byte apart from the one flipped value (indentation, comments, + * quoting, key order, CRLF). A parse/re-serialize round-trip cannot + * guarantee that. Instead, a small indentation-based scanner locates: + * + * 1. the top-level `jobs:` mapping (column-0 content is always a top-level + * key — block-scalar content must be indented past its parent key, so a + * `jobs:` line inside a `run: |` script cannot sit at column 0); + * 2. each job block under it (content lines at the job-key indentation); + * 3. jobs whose direct `uses:` property references the reusable workflow + * at any ref (SHA, tag, branch), optionally quoted or commented; + * 4. that job's direct `permissions:` property — block mapping or + * single-line flow mapping (`permissions: { ... }`). + * + * Only `actions: read` entries inside a matched job's permissions block are + * rewritten. Top-level `permissions:` blocks, other jobs, step-level `uses:` + * lines, and `with:` inputs are never touched. Files without a matching job + * are left unchanged. Running twice is a no-op (idempotent). + * + * Out of scope for the safe flip (left unchanged), but never conflated with + * already-compliant jobs: each is reported via FixResult.manualJobs and + * surfaced by applyFix as manualFiles so callers can route it to a human: + * - calling jobs with no `permissions:` block, a non-mapping value + * (`read-all`, `write-all`), or a block without an `actions:` entry — + * remediating those requires inserting keys, which is a different + * (more invasive) edit; + * - `actions: none` — an explicit opt-out that should not be overridden + * silently; + * - multi-line flow mappings (`permissions: {` spanning several lines) — + * the scanner only recognises single-line flow mappings. + * A file is fully compliant only when every reviewer-calling job either + * already had `actions: write` or was just flipped; a file mixing a flipped + * job with a manual one is reported as BOTH changed and needing manual work. + * + * Pure functions plus a thin I/O wrapper (applyFix) used by the CLI in + * index.ts — mirroring the migrate-consumer-refs module layout. + */ +import { readFileSync, writeFileSync } from 'node:fs'; + +export const REVIEWER_WORKFLOW_PATH = 'docker/docker-agent-action/.github/workflows/review-pr.yml'; + +/** + * Escape a string for use inside a RegExp. + */ +function escapeRegExp(s: string): string { + return s.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); +} + +/** + * A job-level `uses:` line calling the reviewer reusable workflow at any ref, + * with optional quotes and trailing comment. The path must start immediately + * after the optional quote, so a local self-reference + * (`./.github/workflows/review-pr.yml`), another repo's copy, or the old + * `docker/cagent-action` slug never match. Step-level `- uses:` lines are + * excluded by the missing dash (and by the direct-property indentation check + * in the scanner). + */ +const USES_RE = new RegExp( + `^\\s*uses:\\s*["']?${escapeRegExp(REVIEWER_WORKFLOW_PATH)}@[^\\s"'#]+["']?\\s*(?:#.*)?$`, +); + +/** `permissions:` job property introducing a block mapping (entries on following lines). */ +const PERMISSIONS_BLOCK_RE = /^\s*permissions:\s*(?:#.*)?$/; + +/** `permissions: { ... }` job property with a single-line flow mapping. */ +const PERMISSIONS_FLOW_RE = /^\s*permissions:\s*\{.*\}\s*(?:#.*)?$/; + +/** + * Block-style `actions: read` entry. Everything except the value — leading + * indentation, optional quotes, trailing spacing and comment — is captured + * and preserved verbatim. + */ +const ACTIONS_READ_ENTRY_RE = /^(\s*actions:\s*["']?)read(["']?\s*(?:#.*)?)$/; + +/** `actions: read` inside a single-line flow mapping, bounded by `{`/`,` and `,`/`}`. */ +const ACTIONS_READ_FLOW_RE = /([{,]\s*actions:\s*["']?)read(["']?\s*[,}])/; + +/** Block-style `actions: write` entry — the job is already compliant. */ +const ACTIONS_WRITE_ENTRY_RE = /^\s*actions:\s*["']?write["']?\s*(?:#.*)?$/; + +/** `actions: write` inside a single-line flow mapping — the job is already compliant. */ +const ACTIONS_WRITE_FLOW_RE = /[{,]\s*actions:\s*["']?write["']?\s*[,}]/; + +export interface FixResult { + /** Rewritten file content. Identical to input when nothing was replaced. */ + content: string; + /** True when at least one `actions: read` was flipped to `actions: write`. */ + changed: boolean; + /** True when the file contains a job calling the reviewer reusable workflow. */ + hasReviewerJob: boolean; + /** Count of `actions: read` entries flipped to `actions: write`. */ + replacedCount: number; + /** + * Keys of reviewer-calling jobs the safe flip cannot make compliant: no + * `permissions:` block, a non-mapping value (`read-all`, `write-all`), no + * `actions:` entry, `actions: none`, or an unsupported YAML form (e.g. a + * multi-line flow mapping). Empty when every reviewer-calling job has (or + * now has) `actions: write`. + */ + manualJobs: string[]; +} + +function indentOf(text: string): number { + let i = 0; + while (i < text.length && text[i] === ' ') i++; + return i; +} + +function isContent(text: string): boolean { + const t = text.trim(); + return t !== '' && !t.startsWith('#'); +} + +/** Extract the job key from its mapping line (` review: # x` → `review`). */ +function jobKeyOf(text: string): string { + const trimmed = text.trim(); + const colon = trimmed.indexOf(':'); + return colon === -1 ? trimmed : trimmed.slice(0, colon).trim(); +} + +/** + * Rewrite `actions: read` → `actions: write` in the permissions block of + * every job calling the reviewer reusable workflow. + */ +export function fixReviewerCachePermission(content: string): FixResult { + const unchanged: FixResult = { + content, + changed: false, + hasReviewerJob: false, + replacedCount: 0, + manualJobs: [], + }; + + const rawLines = content.split('\n'); + // Tolerate CRLF: strip a trailing \r for matching, re-append on reassembly. + const crFlags = rawLines.map((raw) => raw.endsWith('\r')); + const texts = rawLines.map((raw, i) => (crFlags[i] ? raw.slice(0, -1) : raw)); + + const jobsIdx = texts.findIndex((t) => /^jobs:\s*(?:#.*)?$/.test(t)); + if (jobsIdx === -1) return unchanged; + + // The jobs section ends at the next top-level key (content at column 0), + // so a top-level `permissions:` block before or after it is out of reach. + let jobsEnd = texts.length; + for (let i = jobsIdx + 1; i < texts.length; i++) { + if (isContent(texts[i]) && indentOf(texts[i]) === 0) { + jobsEnd = i; + break; + } + } + + // Job keys sit at the indentation of the first content line in the section + // (YAML requires sibling mapping keys to share their indentation). + let jobIndent = -1; + for (let i = jobsIdx + 1; i < jobsEnd; i++) { + if (isContent(texts[i])) { + jobIndent = indentOf(texts[i]); + break; + } + } + if (jobIndent === -1) return unchanged; // empty jobs section + + const jobStarts: number[] = []; + for (let i = jobsIdx + 1; i < jobsEnd; i++) { + if (isContent(texts[i]) && indentOf(texts[i]) === jobIndent) jobStarts.push(i); + } + + const out = [...texts]; + let hasReviewerJob = false; + let replacedCount = 0; + const manualJobs: string[] = []; + + for (let j = 0; j < jobStarts.length; j++) { + const start = jobStarts[j]; + const end = j + 1 < jobStarts.length ? jobStarts[j + 1] : jobsEnd; + + // Direct properties of the job share the indentation of the first + // content line inside the block; deeper lines belong to sub-blocks + // (steps, with, secrets, ...) and are never scanned as properties. + let propIndent = -1; + for (let i = start + 1; i < end; i++) { + if (isContent(texts[i])) { + propIndent = indentOf(texts[i]); + break; + } + } + if (propIndent <= jobIndent) continue; // empty or malformed job block + + let usesFound = false; + let permLine = -1; + let permFlow = false; + for (let i = start + 1; i < end; i++) { + if (!isContent(texts[i]) || indentOf(texts[i]) !== propIndent) continue; + if (USES_RE.test(texts[i])) { + usesFound = true; + } else if (PERMISSIONS_FLOW_RE.test(texts[i])) { + permLine = i; + permFlow = true; + } else if (PERMISSIONS_BLOCK_RE.test(texts[i])) { + permLine = i; + permFlow = false; + } + } + + if (!usesFound) continue; + hasReviewerJob = true; + const jobKey = jobKeyOf(texts[start]); + if (permLine === -1) { + // No permissions block, a non-mapping value (read-all/write-all), or an + // unsupported form such as a multi-line flow mapping — inserting or + // rewriting keys there is out of scope, so the job needs a manual fix. + manualJobs.push(jobKey); + continue; + } + + if (permFlow) { + const replaced = out[permLine].replace(ACTIONS_READ_FLOW_RE, '$1write$2'); + if (replaced !== out[permLine]) { + out[permLine] = replaced; + replacedCount++; + } else if (!ACTIONS_WRITE_FLOW_RE.test(out[permLine])) { + manualJobs.push(jobKey); // no actions entry, or a value like none + } + continue; + } + + // Block mapping: entries run until the next content line at or above the + // property indentation. Blank and comment lines inside do not end it. + let jobReplaced = false; + let hasWrite = false; + for (let i = permLine + 1; i < end; i++) { + if (isContent(texts[i]) && indentOf(texts[i]) <= propIndent) break; + const replaced = out[i].replace(ACTIONS_READ_ENTRY_RE, '$1write$2'); + if (replaced !== out[i]) { + out[i] = replaced; + replacedCount++; + jobReplaced = true; + } else if (ACTIONS_WRITE_ENTRY_RE.test(out[i])) { + hasWrite = true; + } + } + if (!jobReplaced && !hasWrite) { + manualJobs.push(jobKey); // no actions entry, or actions: none + } + } + + if (replacedCount === 0) { + return { content, changed: false, hasReviewerJob, replacedCount: 0, manualJobs }; + } + + const result = out.map((t, i) => (crFlags[i] ? `${t}\r` : t)).join('\n'); + return { + content: result, + changed: result !== content, + hasReviewerJob, + replacedCount, + manualJobs, + }; +} + +// --------------------------------------------------------------------------- +// I/O wrapper (used by the CLI entry point) +// --------------------------------------------------------------------------- + +export interface ApplyFixResult { + /** Files that were rewritten on disk (in input order). */ + changedFiles: string[]; + /** Files whose reviewer-calling jobs ALL already have `actions: write`. */ + compliantFiles: string[]; + /** + * Files with at least one reviewer-calling job the safe flip cannot make + * compliant (see FixResult.manualJobs). Overlaps with changedFiles when a + * file mixes a flipped job with a non-fixable one — such a file is still + * not fully compliant after the rewrite. + */ + manualFiles: string[]; + /** Files without any job calling the reviewer workflow — never modified. */ + skippedFiles: string[]; + /** Per-file failures (read/write errors). Files in this list were NOT partially written. */ + errors: Array<{ file: string; message: string }>; +} + +/** + * Apply fixReviewerCachePermission to each file in-place. + * + * The buckets are NOT mutually exclusive: a file mixing a flipped job with a + * non-fixable one lands in both changedFiles and manualFiles, so the safe + * flips are applied while the file is still flagged as unfinished. + * compliantFiles is reserved for files whose reviewer-calling jobs all + * already have `actions: write` — never conflated with manualFiles. + * + * Per-file errors (unreadable file, write failure) are collected instead of + * aborting the loop, so a single bad file cannot leave the caller with a + * silently truncated "changed files" list. Callers MUST treat a non-empty + * `errors` array as a failure (the CLI exits 1) — this prevents the + * fix-reviewer-cache-permissions workflow from committing a partial fix. + * + * Progress messages are written to stderr. + */ +export function applyFix(files: string[]): ApplyFixResult { + const result: ApplyFixResult = { + changedFiles: [], + compliantFiles: [], + manualFiles: [], + skippedFiles: [], + errors: [], + }; + + for (const file of files) { + try { + const before = readFileSync(file, 'utf-8'); + const fix = fixReviewerCachePermission(before); + if (fix.changed) { + writeFileSync(file, fix.content, 'utf-8'); + result.changedFiles.push(file); + process.stderr.write(`✅ ${file}: ${fix.replacedCount} actions: read → write\n`); + } + if (fix.manualJobs.length > 0) { + result.manualFiles.push(file); + process.stderr.write( + `⚠️ ${file}: job(s) the safe flip cannot fix — needs manual review: ${fix.manualJobs.join(', ')}\n`, + ); + } else if (!fix.changed) { + if (fix.hasReviewerJob) { + result.compliantFiles.push(file); + process.stderr.write( + `ℹ️ ${file}: every reviewer-calling job already has actions: write\n`, + ); + } else { + result.skippedFiles.push(file); + process.stderr.write(`ℹ️ ${file}: no job calls the reviewer workflow — skipped\n`); + } + } + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + result.errors.push({ file, message }); + process.stderr.write(`⚠️ ${file}: ${message}\n`); + } + } + + return result; +} + +/** + * Render an ApplyFixResult as the CLI's machine-readable stdout contract: + * one ` ` line per file, statuses `changed`, `needs-manual`, + * `compliant`, `skipped`. A file appears under BOTH `changed` and + * `needs-manual` when it mixes a flipped job with a non-fixable one — + * consumers must treat any `needs-manual` line as "this file is not fully + * fixed" regardless of the `changed` lines. + * + * The fix-reviewer-cache-permissions workflow greps these prefixes; do not + * rename them without updating it. + */ +export function statusLines(result: ApplyFixResult): string[] { + return [ + ...result.changedFiles.map((f) => `changed ${f}`), + ...result.manualFiles.map((f) => `needs-manual ${f}`), + ...result.compliantFiles.map((f) => `compliant ${f}`), + ...result.skippedFiles.map((f) => `skipped ${f}`), + ]; +} diff --git a/src/fix-reviewer-cache-permission/index.ts b/src/fix-reviewer-cache-permission/index.ts new file mode 100644 index 0000000..1203cd6 --- /dev/null +++ b/src/fix-reviewer-cache-permission/index.ts @@ -0,0 +1,70 @@ +// Copyright The Docker Agent Action authors +// SPDX-License-Identifier: Apache-2.0 + +/** + * fix-reviewer-cache-permission CLI entrypoint. + * + * Upgrades `actions: read` to `actions: write` in the permissions block of + * jobs calling the PR-review reusable workflow + * (docker/docker-agent-action/.github/workflows/review-pr.yml), in-place. + * Cache writes (review lock, memory, binary cache) require `actions: write` + * on the calling job; `write` implicitly includes `read`. + * + * Usage: + * node dist/fix-reviewer-cache-permission.js [ ...] + * + * Output (stdout): one ` ` line per file: + * changed actions: read flipped to write and written to disk + * needs-manual a reviewer-calling job the safe flip cannot make + * compliant (no permissions block, read-all, no + * actions entry, actions: none, unsupported YAML) — + * the file is NOT fully fixed even when a `changed` + * line for the same path is also present + * compliant every reviewer-calling job already has actions: write + * skipped no job calls the reviewer workflow + * Progress/diagnostics go to stderr. + * + * Exit codes: + * 0 all files processed (whether or not anything changed; needs-manual + * files are reported via stdout, not the exit code) + * 1 at least one file failed to read/write, or bad arguments. + * Per-file failures do NOT abort the loop — every file is attempted — + * but the non-zero exit tells callers the run is incomplete so they + * must not commit a partial fix. + */ +import { applyFix, statusLines } from './fix-permission.js'; + +function main(): void { + const files = process.argv.slice(2); + + if (files.length === 0 || files.some((f) => f.startsWith('--'))) { + process.stderr.write('Usage: fix-reviewer-cache-permission [ ...]\n'); + process.exit(1); + } + + const result = applyFix(files); + const { changedFiles, compliantFiles, manualFiles, skippedFiles, errors } = result; + + for (const line of statusLines(result)) { + process.stdout.write(`${line}\n`); + } + + process.stderr.write( + `Done: ${changedFiles.length} changed, ${manualFiles.length} needing manual review, ` + + `${compliantFiles.length} compliant, ${skippedFiles.length} skipped of ${files.length} file(s)\n`, + ); + + if (errors.length > 0) { + process.stderr.write( + `Error: ${errors.length} file(s) could not be processed — failing so callers do not commit a partial fix\n`, + ); + process.exit(1); + } +} + +try { + main(); +} catch (err) { + process.stderr.write(`Error: ${err instanceof Error ? err.message : String(err)}\n`); + process.exit(1); +} diff --git a/tsup.config.ts b/tsup.config.ts index e1c31d1..4560f87 100644 --- a/tsup.config.ts +++ b/tsup.config.ts @@ -28,6 +28,7 @@ const entry = { credentials: src('credentials'), 'dedupe-findings': src('dedupe-findings'), 'filter-diff': src('filter-diff'), + 'fix-reviewer-cache-permission': src('fix-reviewer-cache-permission'), 'incremental-review': src('incremental-review'), main: src('main'), 'mention-reply': src('mention-reply'),