refactor(sections)!: milestones, webhooks, and rulesets join listSection; duplicate live items fail loudly everywhere

docs/operate/snapshot.md

@@ -74,7 +74,7 @@ GitHub never returns a secret's value, so the file cannot hold one. The snapshot
 |---|---|---|
 | A repository Actions secret named `DEPLOY_TOKEN` | `value: $SECRET_ACTIONS_DEPLOY_TOKEN` | `SECRET_ACTIONS_DEPLOY_TOKEN` |
 | The same name in another store (`dependabot_secrets`, `codespaces_secrets`, `agents_secrets`) | `$SECRET_DEPENDABOT_DEPLOY_TOKEN`, `$SECRET_CODESPACES_DEPLOY_TOKEN`, `$SECRET_AGENTS_DEPLOY_TOKEN` | One variable per store, so two stores holding the same name never share a value by accident |
-| A webhook's `config.secret`, one per hook | `secret: $WEBHOOK_SECRET_601` | `WEBHOOK_SECRET_<id>`, the hook's own id, so the reference survives a reordering |
+| A webhook's `config.secret`, one per hook | `secret: $SECRET_WEBHOOK_601` | `SECRET_WEBHOOK_<id>`, the hook's own id, so the reference survives a reordering |
 | An environment secret named `DEPLOY_TOKEN` in the `production` environment | `value: $SECRET_ENVIRONMENT_PRODUCTION_DEPLOY_TOKEN` | `SECRET_ENVIRONMENT_<ENV>_<NAME>`, the environment name uppercased with every other character folded to `_`, so same-named secrets in two environments get two variables; two names the fold collapses (`prod-eu`, `prod_eu`) share one, and a header line names the entries to edit |
 
 Wire them the way the [secrets guide](../reference/secrets-and-vaults.md) describes: an `env:` block on the apply step, fed from GitHub Secrets or a vault action. A reference that is not exported fails the run that reads the file, naming the variable.
@@ -122,7 +122,7 @@ A snapshot is written so that applying it changes nothing and checking it reads
 | Exception | Why | What the file holds |
 |---|---|---|
 | Secret values | GitHub returns names only | A `$NAME` reference and a note per secret |
-| `webhooks[].config.secret` | GitHub echoes `********` | A `$WEBHOOK_SECRET_N` reference and a note per hook |
+| `webhooks[].config.secret` | GitHub echoes `********` | A `$SECRET_WEBHOOK_<id>` reference and a note per hook |
 | `interaction_limits.expiry` | GitHub reports only the computed `expires_at` | No `expiry` key; apply re-arms the limit with GitHub's default unless you declare one |
 | `check_suite_preferences` | GitHub exposes no read endpoint | Nothing; the header says so |
 | `repository.name` | The repository's identity: a reused file would rename its target | Nothing |

docs/upgrading/v2-to-v3.md

@@ -4,7 +4,7 @@ order: 20
 
 # Upgrading from v2 to v3
 
-Ten breaks (the ninth is for library consumers). One is silent (the fallback), so run `mode: check` before the first v3 apply. The changelog entry for 3.0.0 will carry the release-please footers in the [CHANGELOG](https://github.com/Vivswan/github-settings-as-code/blob/main/CHANGELOG.md).
+Fifteen breaks (the ninth is for library consumers). One is silent (the fallback), so run `mode: check` before the first v3 apply. The changelog entry for 3.0.0 will carry the release-please footers in the [CHANGELOG](https://github.com/Vivswan/github-settings-as-code/blob/main/CHANGELOG.md).
 
 | Break | v2 | v3 | What the old form does now |
 |---|---|---|---|
@@ -18,6 +18,11 @@ Ten breaks (the ninth is for library consumers). One is silent (the fallback), s
 | A snapshot section that fails on its own fails the target | `result: partial`, exit 0, the file written without that section | `result: failed`, exit 1, no file for that target; `init` refuses to write | The workflow step fails where it passed; [section 8](#8-a-failed-snapshot-section-fails-the-target) |
 | Library: a documented public entry, an internal one, and the v3 names | One entry, 192 names; `GithubApi`, `RepoRunReport`, `validateSettings` returning `warnings` | `.` holds the 118 documented names, `./internal` the rest; `GitHubApi`, `CheckReport`, every report carries `log` | The old import fails to compile, naming the missing export; every rename is in [section 9](#9-library-the-public-entry-and-the-v3-names) |
 | One redacted label in every mode | A single-repository run labelled its hidden target `private repository`; a fleet numbered them `private repository #N` | `private repository #N` everywhere; a run over one repository is `#1` | No error. A log filter or artifact-report reader matching `private repository:` exactly no longer matches; [section 10](#10-one-redacted-label-in-every-mode) |
+| Two live items under one identity fail the section | The last one listed won silently in most sections | Every list section refuses, naming the pair | The section fails until one is deleted on GitHub; [section 11](#11-two-live-items-under-one-identity-fail-the-section) |
+| The webhook snapshot placeholder | `$WEBHOOK_SECRET_<id>` | `$SECRET_WEBHOOK_<id>` | No error: an old reference keeps resolving from its old export; a new snapshot writes the new name, so move both together; [section 12](#12-the-webhook-snapshot-placeholder-leads-with-secret_) |
+| One wording per concept in drift lines and notes | Per-section spellings of "cannot verify", "left out", and field drift; quoted webhook labels | One template each on the converted sites | Only a grep over the output notices; [section 13](#13-one-wording-per-concept-in-drift-lines-and-notes) |
+| Webhooks manage web hooks only | A service hook was matched and deleted like any other | A service hook or url-less hook is outside the section | It is left alone and noted by snapshot; [section 14](#14-webhooks-manage-web-hooks-only) |
+| A ruleset without `source_type` is repository-owned | Kept with a note under `_undeclared: delete` | Deleted like any other undeclared repository ruleset | [section 15](#15-a-ruleset-without-source_type-is-repository-owned) |
 
 ## 1. The defaults-file fallback
 
@@ -184,10 +189,63 @@ The `artifact` report channel heads that run's report `<!-- private repository #
 
 Snapshot targets ride the same seal now: a private target's notes, file path, and section detail close sealed exactly as a multi-repo apply target's do, and a private target that fails gets the same one-line annotation (`private repository #N: failed - <sections>`) a fleet target gets. The [private repositories guide](../operate/private-repositories.md#one-seal-every-mode) owns the rule.
 
+## 11. Two live items under one identity fail the section
+
+v2 picked one silently (the factory sections refused only a claimed pair; milestones, rulesets, webhooks, custom properties, secret scanning patterns, environment secrets and variables kept the last one listed). v3 refuses every list section the same way, whether or not the settings file declares the pair:
+
+```text
+webhooks: GitHub holds webhooks that resolve to one identity: "https://ci.example.com/hook (hook id 11)" and "https://ci.example.com/hook (hook id 12)". This section manages one webhook per identity, so it cannot tell them apart; delete all but one of each on GitHub, then run again
+```
+
+Snapshot says the same. Delete the duplicates on GitHub, then re-run.
+
+## 12. The webhook snapshot placeholder leads with SECRET_
+
+```yaml settings
+webhooks:
+  entries:
+    - config:
+        url: https://ci.example.com/hook
+        secret: $SECRET_WEBHOOK_601   # v2 wrote $WEBHOOK_SECRET_601
+```
+
+Every snapshot secret now leads with `SECRET_`, the store name second. A file holding the old reference keeps working with its old export; to move, change the reference in the file and the exported variable together (`WEBHOOK_SECRET_601` becomes `SECRET_WEBHOOK_601` in both), or re-snapshot and export the new name.
+
+## 13. One wording per concept in drift lines and notes
+
+Anything that greps the check output for these lines needs the new spelling. Two templates reach only the sites this release converts:
+
+- the cannot-verify line: the webhook secret and the write-only `check_suite_preferences` note;
+- the left-out line: rulesets, webhooks, and the secondary snapshot reads (actions, environments, repository) a denied grant skips under `on-missing-permission: warn`; a denied primary read still says `skipped`.
+
+The other cannot-verify and omission notes (interaction_limits, teams, collaborators) keep their v2 line until a later release.
+
+| Line | v2 | v3 |
+|---|---|---|
+| A webhook's label | `webhooks["https://ci.example.com/hook"].active` | `webhooks[https://ci.example.com/hook].active` |
+| A field mismatch (milestones, rulesets, collaborators, teams, every list section) | `milestones[v1].state: "closed" != "open"` and `teams[platform]: live role "read" != declared "write"` | `milestones[v1].state: declared "closed" != live "open"; apply will set the declared value` |
+| A webhook's events | `... declared [...] != live [...] (compared order-insensitively)` | one line per element: `webhooks[<url>].events: missing "release"` |
+| A value check mode cannot compare | `... so the declared value cannot be verified; apply re-sends it on every run so rotations propagate` | `<label>: <why>, so check mode cannot verify <what>; apply <re-sends it> on every run` |
+| A resource a snapshot reads but does not declare | `rulesets[x]: inherited from the organization ..., so it is not part of the repository's snapshot` | `<label>: left out of the snapshot - <reason>` |
+| A ruleset update's change line | `updated ruleset "main" (id 42)` | `updated ruleset "main"` |
+| A milestone delete's change line | `DELETED undeclared milestone "v0.9" (detached from every issue that carried it)` | `DELETED undeclared milestone "v0.9"` (the drift line beside it still names the detaching) |
+
+## 14. Webhooks manage web hooks only
+
+- A legacy service hook (`name` other than `web`) or a hook without a `config.url` is outside the section: plan neither matches, notes, nor deletes it (v2 deleted one under `_undeclared: delete`); snapshot leaves it out with a note.
+- No write carries `name`: GitHub defaults a new hook to `web`, the one value the slice admits, and the update endpoint takes no name.
+- A declared `insecure_ssl: 0` is written as GitHub stores it, the string `"0"`.
+- A snapshot writes a hook's `config` keys in the order GitHub lists them, the `$SECRET_WEBHOOK_<id>` reference last.
+
+## 15. A ruleset without `source_type` is repository-owned
+
+v2 refused to delete an undeclared ruleset whose list entry lacked `source_type`, with a note. v3 reads a missing `source_type` as `Repository`, the only kind the repository endpoints can write, so `_undeclared: delete` deletes it.
+
 ## Order of operations
 
 1. Rename any settings file whose path contains a comma, and rename `undeclared` to `_undeclared` in every settings file; the v2 line accepts the old spelling only, so do both together with the pin move.
 2. Rename `skippedSections` to `skipped-sections` in every step expression that reads `repos-result`, and repoint `jq` filters at the `--json` envelope.
-3. Move the pin to `@v3` with `mode: check`.
-4. Read the fallback notices and the drift; add merge steps where a target needs the old overlay behavior.
-5. Switch back to apply.
+3. Where a snapshot wrote a `$WEBHOOK_SECRET_<id>` reference, change the reference and its exported variable to `SECRET_WEBHOOK_<id>` together, or re-snapshot.
+4. Move the pin to `@v3` with `mode: check`.
+5. Read the fallback notices and the drift; add merge steps where a target needs the old overlay behavior; delete duplicated live items the sections now refuse.
+6. Switch back to apply.

src/engine/secrets.ts

@@ -48,7 +48,7 @@ export interface MintedSecretReference {
  * the same grammar and reserved-prefix rule the settings file enforces, so a snapshot can never
  * emit a reference an apply would refuse. `label` names the secret for the BUG prose.
  */
-export function mintSecretReference(variable: string, label: string): MintedSecretReference {
+function mintSecretReference(variable: string, label: string): MintedSecretReference {
   const reference = `$${variable}`;
   const checked = validateSecretRef(reference, "operator", label);
   if (!checked.ok) {

src/sections/collaborators/collaborators.test.ts

@@ -45,7 +45,7 @@ describe("collaborators", () => {
           payload: { permission: "push" },
           describe: 'updating collaborator "alice"',
           drift: [
-            'collaborators[alice]: live role "read" != declared "write"; apply will set the declared permission',
+            'collaborators[alice]: declared "write" != live "read"; apply will set the declared permission',
           ],
           change: 'updated collaborator "alice" (push)',
         },
@@ -132,7 +132,7 @@ describe("collaborators", () => {
           payload: { permissions: "write" },
           describe: 'updating the pending invitation for "alice"',
           drift: [
-            'collaborators[alice]: pending invitation permission "read" != declared "write"; apply will update the invitation',
+            'collaborators[alice] (pending invitation): declared "write" != live "read"; apply will update the invitation',
           ],
           change: 'updated pending invitation for "alice" (push)',
         },

src/sections/collaborators/index.ts

@@ -2,7 +2,7 @@
 
 import { z } from "zod";
 import type { EndpointDecl } from "../contract/endpoints.js";
-import { parseLive } from "../contract/live.js";
+import { liveByIdentity, parseLive } from "../contract/live.js";
 import {
   defaultUndeclaredPolicy,
   loosen,
@@ -11,6 +11,7 @@ import {
   undeclaredDrift,
   undeclaredNote,
   undeclaredPolicy,
+  valueDrift,
 } from "../contract/module.js";
 import type { SectionPermission } from "../contract/permissions.js";
 import type { PlanContext, PlannedOp, SectionPlan } from "../contract/plan.js";
@@ -91,13 +92,15 @@ function declaredPermission(label: string, role: string): string {
   return permission;
 }
 
-/** Both pools in one read, so plan() and snapshot() see the same live access. */
+/** Both pools in one read, each indexed under the guard, so plan() and snapshot() see the same live access. */
 async function readLiveAccess(
   ctx: CollaboratorsContext,
   section: SectionMeta,
 ): Promise<{
   collaborators: LiveCollaborator[];
+  liveByLogin: Map<string, LiveCollaborator>;
   invitations: NamedInvitation[];
+  inviteByLogin: Map<string, NamedInvitation>;
   emailInvitations: LiveInvitation[];
 }> {
   const collaborators = parseLive(
@@ -112,9 +115,24 @@ async function readLiveAccess(
     z.array(LiveInvitation),
     await ctx.read.listInvitations.listAll(),
   );
+  const invitations = allInvitations.filter(isNamedInvitation);
   return {
     collaborators,
-    invitations: allInvitations.filter(isNamedInvitation),
+    liveByLogin: liveByIdentity(
+      section,
+      "collaborator",
+      collaborators,
+      (c) => c.login.toLowerCase(),
+      (c) => c.login,
+    ),
+    invitations,
+    inviteByLogin: liveByIdentity(
+      section,
+      "pending invitation",
+      invitations,
+      (invitation) => invitation.invitee.login.toLowerCase(),
+      (invitation) => invitation.invitee.login,
+    ),
     emailInvitations: allInvitations.filter((invitation) => !isNamedInvitation(invitation)),
   };
 }
@@ -150,11 +168,13 @@ export const collaboratorsSection = {
     // Both pools are resolved BEFORE the declared walk, so a declared user is never mistaken for
     // undeclared in the other pool; email invitations (null invitee, which no username can declare)
     // split into their own pool.
-    const { collaborators: live, invitations, emailInvitations } = await readLiveAccess(ctx, this);
-    const liveByLogin = new Map(live.map((c) => [c.login.toLowerCase(), c]));
-    const inviteByLogin = new Map(
-      invitations.map((invitation) => [invitation.invitee.login.toLowerCase(), invitation]),
-    );
+    const {
+      collaborators: live,
+      liveByLogin,
+      invitations,
+      inviteByLogin,
+      emailInvitations,
+    } = await readLiveAccess(ctx, this);
     const declaredKeys = new Set<string>();
     const plan: SectionPlan<PlannedOp<typeof ENDPOINTS>> = { ops: [], notes: [], drift: [] };
 
@@ -175,7 +195,12 @@ export const collaboratorsSection = {
             payload: { permission: wantPermission },
             describe: `updating collaborator "${username}"`,
             drift: [
-              `${label}: live role "${existing.role_name}" != declared "${wantRole}"; apply will set the declared permission`,
+              valueDrift(
+                label,
+                JSON.stringify(wantRole),
+                JSON.stringify(existing.role_name),
+                "apply will set the declared permission",
+              ),
             ],
             change: `updated collaborator "${username}" (${wantPermission})`,
           });
@@ -200,7 +225,12 @@ export const collaboratorsSection = {
             payload: { permissions: wantRole },
             describe: `updating the pending invitation for "${username}"`,
             drift: [
-              `${label}: pending invitation permission "${invitation.permissions}" != declared "${wantRole}"; apply will update the invitation`,
+              valueDrift(
+                `${label} (pending invitation)`,
+                JSON.stringify(wantRole),
+                JSON.stringify(invitation.permissions),
+                "apply will update the invitation",
+              ),
             ],
             change: `updated pending invitation for "${username}" (${wantPermission})`,
           });

src/sections/contract/live.ts

@@ -7,6 +7,34 @@
 import type { z } from "zod";
 import { type EndpointDecl, endpointMethod, endpointPath } from "./endpoints.js";
 import type { SectionMeta } from "./module.js";
+import { collidingPairs } from "./requests.js";
+
+/** The plural of a section noun for a message ("custom property" -> "custom properties", "deploy key" -> "deploy keys"). */
+export function plural(noun: string): string {
+  return /[^aeiou]y$/.test(noun) ? `${noun.slice(0, -1)}ies` : `${noun}s`;
+}
+
+/**
+ * Live items indexed by the identity the section manages them under. GitHub may hold two under one
+ * (repeated deploy-key titles, repeated hook urls, two names one fold apart), which a single-slot map
+ * would silently collapse into "the last one listed"; plan() and snapshot() both refuse that here, so
+ * every section fails the same way and names the pairs.
+ */
+export function liveByIdentity<T, Key extends string>(
+  section: SectionMeta,
+  noun: string,
+  items: readonly T[],
+  keyOf: (item: T) => Key,
+  describe: (item: T) => string = keyOf,
+): Map<Key, T> {
+  const collisions = collidingPairs(items, keyOf, describe);
+  if (collisions.length > 0) {
+    throw new Error(
+      `${section.key}: GitHub holds ${plural(noun)} that resolve to one identity: ${collisions.join("; ")}. This section manages one ${noun} per identity, so it cannot tell them apart; delete all but one of each on GitHub, then run again`,
+    );
+  }
+  return new Map(items.map((item) => [keyOf(item), item]));
+}
 
 /**
  * Schemas stay loose objects, so passthrough fields survive for subsetDiff/phantomKeys. `describe` names