fix(sections): never plan the owner, hedge a concealed team 404, and advise the grant the writes need

docs/reference/coverage.md

@@ -408,9 +408,10 @@ The side-by-side comparison with the Probot Settings app lives in the migration
 
 - Direct collaborators only. PUT/DELETE /repos/{owner}/{repo}/collaborators/{username} (affiliation=direct); new users get invitations.
 - Vocabulary mapping push<->write, pull<->read for check mode; custom org role names pass through.
-- Undeclared direct collaborators are REMOVED by default (kept as notes under the wrapped `_undeclared: keep` form); the owner is never touched.
+- Undeclared direct collaborators are REMOVED by default (kept as notes under the wrapped `_undeclared: keep` form).
+- The repository owner is never touched: it is never removed as undeclared, and an entry naming it (an organization's own login included) plans no write and earns a note saying to remove the entry.
 - Snapshot reads the direct collaborators and the pending username invitations back as entries under the delete default, each role through the inverse mapping (write -> push, read -> pull, custom role names verbatim).
-- The snapshot leaves out the repository owner, email invitations, and expired invitations with a note; a live role no declaration plans as (a custom role named "push" or "pull") fails the collaborators section, and the snapshot is written as partial without it.
+- The snapshot leaves out the repository owner, email invitations, and expired invitations with a note; a live role no declaration plans as (a custom role named "push" or "pull") fails the collaborators section, and a failed section fails the target: the result is `failed` and no file is written for it.
 
 **Repository invitations** (`collaborators`)
 
@@ -423,6 +424,7 @@ The side-by-side comparison with the Probot Settings app lives in the migration
 
 - Org repos only. PUT/DELETE /orgs/{org}/teams/{slug}/repos/{owner}/{repo}; the section probes GET /orgs/{owner} and no-ops with a note on personal accounts (404 only; 403/5xx still fail).
 - Both modes probe each declared team's access with the v3.repository media type to read role_name, and the grant is issued only when the team lacks access or its live role diverges.
+- A declared team the repository's team list names, whose probe answers 404, is still planned as a grant, and its check line names both readings of the 404 (no access, or a fine-grained token missing the grant), as the snapshot's note does.
 - The repository's team list (GET /repos/{owner}/{repo}/teams) names the undeclared teams: kept by default and noted (a team is often granted by the org for reasons outside one repository's file).
 - Under the wrapped `_undeclared: delete` form a team's DIRECT grant is revoked, while access granted at the organization or enterprise level is noted as beyond the repository's reach.
 - A declared child team whose access comes through an undeclared parent's grant loses it with that revocation (the probe reads the inherited role as converged): declare the parent too, or keep.

docs/sections/collaborators.docs.yml

@@ -15,15 +15,20 @@ coverage:
         through.
       - >-
         Undeclared direct collaborators are REMOVED by default (kept as notes under the wrapped
-        `_undeclared: keep` form); the owner is never touched.
+        `_undeclared: keep` form).
+      - >-
+        The repository owner is never touched: it is never removed as undeclared, and an entry
+        naming it (an organization's own login included) plans no write and earns a note saying to
+        remove the entry.
       - >-
         Snapshot reads the direct collaborators and the pending username invitations back as entries
         under the delete default, each role through the inverse mapping (write -> push, read -> pull,
         custom role names verbatim).
       - >-
         The snapshot leaves out the repository owner, email invitations, and expired invitations with
         a note; a live role no declaration plans as (a custom role named "push" or "pull") fails the
-        collaborators section, and the snapshot is written as partial without it.
+        collaborators section, and a failed section fails the target: the result is `failed` and no
+        file is written for it.
   - area: "[Repository invitations](https://docs.github.com/en/rest/collaborators/invitations)"
     endpoints: [listInvitations, updateInvitation, cancelInvitation]
     notes:

docs/sections/teams.docs.yml

@@ -13,6 +13,10 @@ coverage:
       - >-
         Both modes probe each declared team's access with the v3.repository media type to read
         role_name, and the grant is issued only when the team lacks access or its live role diverges.
+      - >-
+        A declared team the repository's team list names, whose probe answers 404, is still planned
+        as a grant, and its check line names both readings of the 404 (no access, or a fine-grained
+        token missing the grant), as the snapshot's note does.
       - >-
         The repository's team list (GET /repos/{owner}/{repo}/teams) names the undeclared teams: kept
         by default and noted (a team is often granted by the org for reasons outside one repository's

src/sections/collaborators/index.ts

@@ -1,7 +1,7 @@
 /**
  * `collaborators:` section: direct collaborators by username plus their pending invitations; the owner is
- * never removed. Bespoke, not on listSection: one declared username resolves against two live pools (the
- * collaborator list and the pending invitations), each with its own writes.
+ * never touched, declared or not. Bespoke, not on listSection: one declared username resolves against two
+ * live pools (the collaborator list and the pending invitations), each with its own writes.
  */
 
 import { err, ok, type Result, safeTry } from "neverthrow";
@@ -32,7 +32,6 @@ import { CollaboratorConfig } from "./schema.js";
 
 const LiveCollaborator = z.looseObject({
   login: z.string(),
-  permissions: z.record(z.string(), z.boolean()).optional(),
   role_name: z.string().optional(),
 });
 type LiveCollaborator = z.infer<typeof LiveCollaborator>;
@@ -256,20 +255,30 @@ function inviteDeclared(
   });
 }
 
-/** One declared entry settled against the collaborator it already is, else its pending invitation, else invited. */
+/**
+ * One declared entry settled against the collaborator it already is, else its pending invitation, else invited.
+ * The owner is none of the three: an entry naming it earns a note, never a write.
+ */
 function settleDeclared(
+  ctx: CollaboratorsContext,
   collaborator: CollaboratorConfig,
   access: LiveAccess,
   plan: CollaboratorsPlan,
 ): void {
+  const login = loginKey(collaborator.username);
+  if (isOwner(ctx, login)) {
+    plan.notes.push(
+      `collaborator "${collaborator.username}" is the repository owner, whose access is implicit and never managed; the entry is ignored - remove it from the settings file to silence this note`,
+    );
+    return;
+  }
+
   const permission = collaborator.permission ?? DEFAULT_ROLE;
   const wanted: Wanted = {
     username: collaborator.username,
     permission,
     role: roleForPermission(permission),
   };
-  const login = loginKey(collaborator.username);
-
   const existing = access.liveByLogin.get(login);
   if (existing) {
     // On GitHub a user is never a collaborator AND an invitee at once, so this branch settles the entry.
@@ -465,7 +474,7 @@ export const collaboratorsSection = {
       const declaredKeys = new Set<LoginKey>();
       for (const collaborator of desired) {
         declaredKeys.add(loginKey(collaborator.username));
-        settleDeclared(collaborator, access, plan);
+        settleDeclared(ctx, collaborator, access, plan);
       }
 
       const undeclared: Undeclared = { declaredKeys, policy, listDefault };

src/sections/contract/errors.ts

@@ -8,7 +8,12 @@ import {
   sectionGrant,
   sectionOperations,
 } from "./operations.js";
-import { grantFor, type SectionPermission, samePermission } from "./permissions.js";
+import {
+  coversPermission,
+  grantFor,
+  type SectionPermission,
+  samePermission,
+} from "./permissions.js";
 
 /**
  * What ends a section's work, as a value. `message` is the whole line the loops report; `kind` is read for policy
@@ -87,9 +92,9 @@ function sentence(clause: string): string {
 
 /**
  * `operation` says WHAT was being done in settings-file terms. `op` supplies the hints and denial hint and resolves
- * the EFFECTIVE permission: an override renders its own grant advice, and a public operation ("none") cannot be a
- * missing-grant failure, so its 403/404 takes the generic branch; a GraphQL failure renders `GRAPHQL <opName>` in
- * the method/path slot.
+ * the EFFECTIVE permission: an override the section's grant does not cover renders its own grant advice, and a
+ * public operation ("none") cannot be a missing-grant failure, so its 403/404 takes the generic branch; a GraphQL
+ * failure renders `GRAPHQL <opName>` in the method/path slot.
  */
 interface FailureContext {
   readonly operation?: string;
@@ -117,14 +122,19 @@ function requestLines(
   };
 }
 
-/** The denial's advice: the grant of the EFFECTIVE permission at the level the section needs, or the section's own, then the endpoint's hint. */
+/**
+ * The denial's advice: the section's own grant whenever it covers the EFFECTIVE permission, else that permission's
+ * grant at the level the section needs; then the endpoint's hint. An override the section's grant covers (the team
+ * list, gated at the repository half alone) is no second grant to ask for: advising only its own read would pass
+ * the preflight and be denied at the section's first write.
+ */
 function denialAdvice(
   section: SectionMeta,
   effective: SectionPermission | undefined,
   op: FailingOp | undefined,
 ): string {
   const grant =
-    effective !== undefined && !samePermission(effective, section.permission)
+    effective !== undefined && !coversPermission(section.permission, effective)
       ? grantFor(effective, undefined, overrideAdviceLevel(section, effective))
       : sectionGrant(section);
   const denialHint = op?.denialHint ? `. Note: ${op.denialHint}` : "";

src/sections/contract/permissions.ts

@@ -39,6 +39,17 @@ export function samePermission(
   return a.org === b.org && resources.size === others.size && resources.isSubsetOf(others);
 }
 
+/**
+ * Whether a token granted `grant` passes `asked`: whichever alternative of `grant` the user picks is one `asked`
+ * accepts, and `asked` wants no organization permission `grant` lacks. The teams grant covers its own list read
+ * this way, which GitHub gates at the repository half alone.
+ */
+export function coversPermission(grant: SectionPermission, asked: SectionPermission): boolean {
+  const offered = new Set(grant.repo);
+  const accepted = new Set(asked.repo);
+  return (asked.org === undefined || asked.org === grant.org) && offered.isSubsetOf(accepted);
+}
+
 /** Human-facing label for each PAT resource, as shown in the token UI. */
 export const RESOURCE_LABEL: Record<PatResource, string> = {
   administration: "Administration",

src/sections/teams/index.ts

@@ -133,15 +133,39 @@ function teamsBySlug(
   );
 }
 
-/** The drift line a declared team's probe earns, or undefined once its live role matches. */
+/**
+ * A probe 404 for a team the repository's list names, in the words plan() and snapshot() share. Neither can tell
+ * no access from a probe the token is denied, and check mode and a snapshot write nothing that would surface the
+ * denial, so both readings are named. `so` is what the caller does under the first reading.
+ */
+function listedProbe404(
+  ctx: TeamsContext,
+  section: SectionMeta,
+  opts: { so?: string; rerun: "check" | "snapshot" },
+): string {
+  const so = opts.so === undefined ? "" : `, so ${opts.so}`;
+  return (
+    `listed with access to ${ctx.repo.slug}, but the access probe answered 404, read here as no access${so}. ` +
+    "A fine-grained token missing the grant gets the same answer; if the team does have access, " +
+    `${sectionGrant(section)}, then ${opts.rerun} again`
+  );
+}
+
+/** The drift line a declared team's probe earns, or undefined once its live role matches; `listed` says the repository's list names the team, which a probe 404 contradicts. */
 function declaredTeamDrift(
-  repo: string,
+  ctx: TeamsContext,
+  section: SectionMeta,
   team: TeamConfig,
   role: string,
-  probe: TeamAccess,
+  found: { probe: TeamAccess; listed: boolean },
 ): string | undefined {
+  const label = `teams[${team.name}]`;
+  const { probe } = found;
   if (!probe.access) {
-    return `teams[${team.name}]: no access to ${repo}; apply will grant "${role}"`;
+    const grant = `apply will grant "${role}"`;
+    return found.listed
+      ? `${label}: ${listedProbe404(ctx, section, { so: grant, rerun: "check" })}`
+      : `${label}: no access to ${ctx.repo.slug}; ${grant}`;
   }
 
   const wantRole = roleForPermission(role);
@@ -150,7 +174,7 @@ function declaredTeamDrift(
     return undefined;
   }
 
-  return valueDrift(`teams[${team.name}]`, JSON.stringify(wantRole), JSON.stringify(liveRole), {
+  return valueDrift(label, JSON.stringify(wantRole), JSON.stringify(liveRole), {
     remedy: "apply will set the declared permission",
   });
 }
@@ -221,13 +245,7 @@ function snapshotRole(ctx: TeamsContext, section: SectionMeta, team: LiveTeam):
 
   return probeTeamRole(ctx, team.slug).map((probe): SnapshotRole => {
     if (!probe.access) {
-      // No write follows to surface a denial, so the note names both readings of the 404.
-      return {
-        leftOut:
-          `listed with access to ${ctx.repo.slug}, but the access probe answered 404, read here as no access. ` +
-          "A fine-grained token missing the grant gets the same answer; if the team does have access, " +
-          `${sectionGrant(section)}, then snapshot again`,
-      };
+      return { leftOut: listedProbe404(ctx, section, { rerun: "snapshot" }) };
     }
     if (probe.role === undefined) {
       return {
@@ -260,15 +278,17 @@ export const teamsSection = {
       );
       const plan: SectionPlan<TeamsOp> = { ops: [], notes: [], drift: [] };
       // The listing is read BEFORE the declared walk, so the undeclared teams are judged against the state the grants
-      // below start from; a declared team's role still comes from the probe, which names a custom role.
+      // below start from; a declared team's role still comes from the probe, which names a custom role, and the
+      // listing only says whether a probe 404 contradicts it.
       const live = yield* ctx.read.list
         .listAll(LiveTeam)
         .andThen((teams) => teamsBySlug(section, teams));
       const declaredSlugs = new Set(desired.map((team) => teamKey(team.name)));
       for (const team of desired) {
         const role = team.permission ?? DEFAULT_ROLE;
         const probe = yield* probeTeamRole(ctx, team.name);
-        const drift = declaredTeamDrift(ctx.repo.slug, team, role, probe);
+        const listed = live.has(teamKey(team.name));
+        const drift = declaredTeamDrift(ctx, section, team, role, { probe, listed });
         if (drift === undefined) {
           continue;
         }

test/sections/collaborators/collaborators.test.ts

@@ -82,6 +82,25 @@ describe("collaborators", () => {
     expect(api.calls.map((c) => `${c.method} ${c.path}`)).toEqual([LIST, INVITATIONS]);
   });
 
+  test.each<[label: string, username: string, live: Array<{ login: string; role_name: string }>]>([
+    // Settled like anyone else, the listed owner would be re-granted at the declared role.
+    ["a user-owned repository's owner, listed as admin", "o", [{ login: "o", role_name: "admin" }]],
+    // No list carries an organization, so its login would read as a missing collaborator and be invited.
+    ["an organization's own login in another case, which no list carries", "O", []],
+  ])(
+    "a declared repository owner plans no write and is noted with the fix: %s",
+    async (_label, username, live) => {
+      const api = new MockApi({ [LIST]: { data: live }, [INVITATIONS]: { data: [] } });
+      expect(await plan(api, [{ username, permission: "push" }])).toEqual({
+        ops: [],
+        notes: [
+          `collaborator "${username}" is the repository owner, whose access is implicit and never managed; the entry is ignored - remove it from the settings file to silence this note`,
+        ],
+        drift: [],
+      });
+    },
+  );
+
   test("a matching role and a matching pending invitation plan nothing; keep turns the undeclared ones into notes", async () => {
     const api = new MockApi({
       [LIST]: {

test/sections/collaborators/scenarios/collaborators-declared-owner-note.yml

@@ -0,0 +1,31 @@
+name: collaborators-declared-owner-note
+description: >-
+  The settings file names the repository owner beside a collaborator already
+  at the declared role, on a user-owned repository whose list carries the
+  owner as admin. Settled like any other entry the owner would be re-granted
+  at the declared "push"; instead apply plans no write for it, the entry
+  earns a note saying to remove it, and a check rerun converges.
+settings:
+  collaborators:
+    - username: e2e-owner
+      permission: push
+    - username: e2e-keeper
+      permission: push
+owner_kind: user
+live_state:
+  collaborators:
+    - login: e2e-owner
+      role_name: admin
+    - login: e2e-keeper
+      role_name: write
+expect:
+  exit_code: 0
+  result: applied
+  outcomes:
+    collaborators: applied
+  never:
+    - "PUT /repos/{repo}/collaborators/"
+    - "DELETE /repos/{repo}/collaborators/"
+  stdout_contains:
+    - 'collaborator "e2e-owner" is the repository owner, whose access is implicit and never managed; the entry is ignored - remove it from the settings file to silence this note'
+  fixpoint: converges

test/sections/contract/contract.test.ts

@@ -30,6 +30,7 @@ import {
   writeGatedReads,
 } from "../../../src/sections/contract/operations.js";
 import {
+  coversPermission,
   type SectionPermission,
   samePermission,
 } from "../../../src/sections/contract/permissions.js";
@@ -1548,6 +1549,40 @@ describe("DenialPolicy", () => {
   });
 });
 
+describe("coversPermission", () => {
+  // Only the first row is a shipped section (teams): the others keep the direction and the alternatives honest, where a
+  // symmetric or reversed comparison would advise a section grant that leaves the denied endpoint denied.
+  test.each<[label: string, grant: SectionPermission, asked: SectionPermission, covers: boolean]>([
+    [
+      "a read gated at the repository half of a grant that also carries an organization half",
+      { repo: ["administration"], org: "members" },
+      { repo: ["administration"] },
+      true,
+    ],
+    [
+      "an organization half the grant lacks",
+      { repo: ["administration"] },
+      { repo: ["administration"], org: "members" },
+      false,
+    ],
+    [
+      "a grant alternative the asked permission does not accept, which the user may be the one to pick",
+      { repo: ["administration", "code_scanning_alerts"] },
+      { repo: ["administration"] },
+      false,
+    ],
+    [
+      "a grant whose every alternative the asked permission accepts",
+      { repo: ["administration"] },
+      { repo: ["code_scanning_alerts", "administration"] },
+      true,
+    ],
+    ["a different resource", { repo: ["administration"] }, { repo: ["contents"] }, false],
+  ])("%s: %p against %p covers %p", (_label, grant, asked, covers) => {
+    expect(coversPermission(grant, asked)).toBe(covers);
+  });
+});
+
 describe("samePermission", () => {
   test.each<
     [label: string, a: SectionPermission | "none", b: SectionPermission | "none", same: boolean]