[Bug]: Repo settings-preview reports `pull_requests: read` and never flags a missing `pull_requests` permission, contradicting the app's own `REQUIRED_INSTALLATION_PERMISSIONS` (which mandates `pull_requests: write`)

[philluiz2323]

Summary

The maintainer-facing repo settings preview computes its required/missing permission guidance in src/signals/settings-preview.ts, but that guidance disagrees with the GitHub App's own declared baseline in src/github/backfill.ts:

// src/github/backfill.ts:639-643  — the app's required install permissions
export const REQUIRED_INSTALLATION_PERMISSIONS: Record<string, string> = {
  metadata: "read",
  pull_requests: "write",   // <- the app needs WRITE (PR conversation comments + PR labels go through the Pull requests permission)
  issues: "write",
};

// src/signals/settings-preview.ts:469-483
function requiredInstallPermissions(settings: RepositorySettings, decision: PublicSurfaceDecision): string[] {
  const permissions = new Set(["metadata: read", "pull_requests: read"]);   // <- lists READ, never upgraded to write
  if (decision.willComment || decision.willLabel || shouldPublishPrComment(settings) || shouldApplyPrLabel(settings, "confirmed")) permissions.add("issues: write");
  if (decision.willCheckRun || settings.checkRunMode === "enabled" || settings.gateCheckMode === "enabled") permissions.add("checks: write");
  return [...permissions];
}

function activeMissingPermissions(settings: RepositorySettings, decision: PublicSurfaceDecision, installation: InstallationHealthSummary | null): string[] {
  if (!installation) return [];
  const missing = new Set(installation.missingPermissions);
  const active: string[] = [];
  if ((decision.willComment || decision.willLabel || shouldPublishPrComment(settings) || shouldApplyPrLabel(settings, "confirmed")) && missing.has("issues")) active.push("issues");
  if ((decision.willCheckRun || settings.checkRunMode === "enabled" || settings.gateCheckMode === "enabled") && missing.has("checks")) active.push("checks");
  return active;   // <- never checks missing.has("pull_requests")
}

So the preview:

1. lists the required pull_requests level as read even when the previewed behavior is to comment on / label PRs (which needs pull_requests: write), and
2. never adds pull_requests to the active-missing list, so a missing pull_requests permission is omitted from the itemized remediation entirely.

Why this is wrong

PR conversation comments (POST /repos/{owner}/{repo}/issues/{n}/comments against a PR) and PR labels (.../issues/{n}/labels against a PR) are gated by GitHub on the Pull requests permission, not Issues — which is exactly why REQUIRED_INSTALLATION_PERMISSIONS declares pull_requests: "write". The install-health surface and its tests already treat it as required:

• test/integration/api.test.ts:1316 / :1111 assert requiredPermissions: { metadata: "read", pull_requests: "write", issues: "write" }.
• test/integration/api.test.ts:1297-1299 exercise a real state where an install granting only pull_requests: "read" reports missingPermissions: ["pull_requests", "issues"] — i.e. pull_requests: read is treated as missing because write is required.

So pull_requests is a first-class required (write) permission for the comment/label outputs, but the settings-preview's requiredInstallPermissions understates it as read and activeMissingPermissions doesn't check it at all. The companion issues/checks permissions are handled correctly; pull_requests is the gap.

Downstream impact

buildRepoInstallPreview (settings-preview.ts:338-359) uses these two functions to build the maintainer-facing permission checklist:

const required = requiredInstallPermissions(args.settings, args.decision);
const missing = activeMissingPermissions(args.settings, args.decision, args.installation);
// permissionStatus is still correct here because it also ORs in args.installation.status === "needs_attention":
const permissionStatus = ... missing.length > 0 || missingEvents.length > 0 || args.installation.status === "needs_attention" ? "needs_attention" : "ready";
// ...but the itemized guidance shown to the maintainer is built from `required` and `missing`:
summary: permissionSummary(args.installation, missing, missingEvents),

The overall permissionStatus is rescued by the installation.status === "needs_attention" clause, so it does not falsely flip to "ready". But the itemized guidance the maintainer actually reads is wrong:

• installPreview.permissions.required shows pull_requests: read (the maintainer is told to grant read).
• When the install is missing pull_requests: write, the permissionSummary text (built from missing, which omits pull_requests) lists only issues/checks as missing — never pull_requests.

So a maintainer whose install lacks pull_requests: write sees "needs attention", grants the named permissions (issues, checks), and is still blocked — with no surfaced reason, because pull_requests was never named as required-at-write or as missing. Comment/label publishing then fails at runtime with GitHub's "Resource not accessible by integration".

Failure mode (concrete example)

Repo previewed with comment output enabled; installation granted metadata: read, pull_requests: read, issues: write, checks: write (so missingPermissions: ["pull_requests"]).

• Current: required lists pull_requests: read; activeMissingPermissions returns [] (it only checks issues/checks, both granted). The remediation summary names no missing permission, while installation.status keeps the checklist at "needs_attention" with no actionable item. The maintainer cannot tell that pull_requests: write is the blocker.
• Correct: required lists pull_requests: write; activeMissingPermissions returns ["pull_requests"], and the summary tells the maintainer to grant pull_requests: write.

Steps to reproduce

1. Build a repo settings preview (buildRepoInstallPreview) for a repo with comment or label output enabled and an installation whose missingPermissions includes pull_requests.
2. Inspect installPreview.permissions.required and the permissions checklist item's summary.
3. Observe pull_requests is listed at read (not write) and is absent from the missing-permission remediation, even though the app requires pull_requests: write for those outputs.

Expected behavior

The settings-preview's required/missing permission guidance matches REQUIRED_INSTALLATION_PERMISSIONS: PR comment/label outputs require pull_requests: write, and a missing pull_requests permission is surfaced in the active-missing list (and its remediation text), consistent with how issues and checks are already handled.

Actual behavior

requiredInstallPermissions lists pull_requests: read and activeMissingPermissions never checks pull_requests, so the maintainer-facing permission guidance understates the required pull_requests level and omits it from the missing list.

Suggested fix

• In requiredInstallPermissions, require pull_requests: write (not read) for the comment/label outputs (and keep read as the baseline otherwise), so the listed level matches the app's actual need.
• In activeMissingPermissions, add pull_requests to the active-missing check for the comment/label outputs:

  if ((decision.willComment || decision.willLabel || shouldPublishPrComment(settings) || shouldApplyPrLabel(settings, "confirmed")) && missing.has("pull_requests")) active.push("pull_requests");

• Best: source the baseline from the shared REQUIRED_INSTALLATION_PERMISSIONS constant so the two surfaces cannot drift again.
• Add fail-on-revert coverage: a preview with comment/label output enabled and an installation missing pull_requests must list pull_requests (at write) in both the required set and the active-missing remediation.