Skip to content

feat: remediate reviewer cache permissions - #70

Closed
Sayt-0 wants to merge 1 commit into
mainfrom
feat/fix-reviewer-cache-permissions
Closed

feat: remediate reviewer cache permissions#70
Sayt-0 wants to merge 1 commit into
mainfrom
feat/fix-reviewer-cache-permissions

Conversation

@Sayt-0

@Sayt-0 Sayt-0 commented Jul 29, 2026

Copy link
Copy Markdown
Member

Summary

Adds remediation automation for consumer workflows that grant only actions: read to the job calling the PR reviewer reusable workflow. Those callers fail validation because the nested review job requires actions: write for cache and review-lock operations.

Changes

  • add a manual fix-reviewer-cache-permissions workflow with dry-run enabled by default and an optional repository allowlist
  • discover reviewer consumers across the Docker organization and update only the calling job's permission from actions: read to actions: write
  • open idempotent same-repository PRs when push access is available, or fork-based PRs when the reviewer has read-only access
  • preserve workflow formatting and report unsupported permission shapes for manual review rather than opening partial PRs
  • add a tested TypeScript CLI with machine-readable remediation statuses

Expected behavior

Consumer state Result
Calling job has actions: read Change to actions: write and prepare a PR
Calling job already has actions: write Skip as compliant
Upstream is read-only and forkable Open a cross-repository PR from a fork
Permission shape cannot be safely rewritten Report under Needs manual review; do not open a partial PR
Dry run enabled Show route and diff without remote writes

Validation

  • 808 unit tests passed
  • 16 integration tests passed; 3 credential-dependent tests skipped
  • TypeScript typecheck passed
  • Biome passed
  • build passed
  • actionlint passed
  • shell integration tests passed

@Sayt-0
Sayt-0 enabled auto-merge (squash) July 29, 2026 10:41

@derekmisler derekmisler left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Automated PR Reviewthis comment was posted by the pr-reviewer bot, not by a human or the implementer agent

Assessment: 🟢 APPROVE

The implementation is solid. The indentation-based YAML scanner, regex patterns, four-bucket file classification (changedFiles / manualFiles / compliantFiles / skippedFiles), and CRLF/quote handling are all correct and well-tested.

Hypotheses investigated and dismissed:

  1. Partial-fix commit when a file is in both changedFiles and manualFiles — the bash workflow explicitly checks MANUAL_FILES before the commit step and runs cleanup_workdir; continue to abort the entire repo if any manual files exist. No partial commit can occur.

  2. USES_RE regex with quoted uses: + inline comment inside quotes — the malformed pattern (uses: "…@sha # comment") is rejected by the GitHub Actions runner as an invalid workflow reference (spaces disallowed in the ref). No real-world impact.

  3. jobKeyOf with colon-containing quoted YAML job keys — GitHub Actions restricts job IDs to [a-zA-Z_][a-zA-Z0-9_-]*; colons are forbidden, making this unreachable in practice.

Other observations from the workflow YAML:

  • The 2>&1 >/dev/null redirect order in the FORK_ERR capture is intentional and correct (stderr → $() pipe, stdout → /dev/null).
  • Fork-parent validation before committing (FORK_PARENT != REPO guard) correctly prevents writing to an unrelated same-named repo.
  • The ADD_ARGS loop is safe: bash command substitution strips trailing newlines from $CHANGED_FILES, so no spurious empty --add "" argument is passed to signed-commit.js.
  • printf -v PR_BODY uses only hardcoded strings and the runner-controlled $RUN_URL; no injection surface.

@Sayt-0 Sayt-0 closed this Jul 29, 2026
auto-merge was automatically disabled July 29, 2026 11:39

Pull request was closed

@Sayt-0
Sayt-0 deleted the feat/fix-reviewer-cache-permissions branch July 29, 2026 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants