fix(repository): name a contradictory private/visibility pair and a null security_and_analysis in a check-time note

docs/reference/coverage.md

@@ -84,6 +84,7 @@ The side-by-side comparison with the Probot Settings app lives in the migration
 - Covers name, description, homepage, visibility/private, has_issues, has_wiki, has_projects, has_discussions, the merge-strategy toggles, squash/merge commit title and message, delete_branch_on_merge, allow_update_branch, allow_auto_merge, web_commit_signoff_required, is_template, archived, and default_branch.
 - PATCH /repos/{owner}/{repo} passthrough: documented fields are typed (toggles true or false; description and homepage also null, the API's clear); a field GitHub adds later works day one.
 - GET-only fields, a commit message without its title, and illegal squash pairs are refused at parse: each would drift or 422 every run.
+- private beside a visibility that contradicts it (true with public; false with private or internal) still parses; a run adds a note naming the pair and the fix, to remove one of the two keys.
 - Check diffs the declared keys against GET /repos; snapshot reads back the PATCH fields, topics, toggles, and GraphQL keys (docs/operate/snapshot.md).
 
 **security_and_analysis** (`repository`)
@@ -92,6 +93,7 @@ The side-by-side comparison with the Probot Settings app lives in the migration
 - Nested in the same PATCH, closed to the sub-keys the PATCH accepts (GitHub 422s any other): each feature is {status: enabled/disabled}, delegated-bypass reviewers are {reviewer_id, reviewer_type: TEAM/ROLE, mode: ALWAYS/EXEMPT}, and the GET-only dependabot_security_updates points at enable_automated_security_fixes.
 - secret_scanning_validity_checks nests here too (documented in the GHEC flavor of the spec).
 - The GET echoes all of it, so check verifies it.
+- security_and_analysis: null still parses; a run adds a note naming the key and the fix, to write each feature's status or remove the key, since the null names no feature to turn off.
 
 **Forking policy** (`repository`)
 

docs/sections/repository.docs.yml

@@ -22,6 +22,10 @@ coverage:
       - >-
         GET-only fields, a commit message without its title, and illegal squash pairs are refused at
         parse: each would drift or 422 every run.
+      - >-
+        private beside a visibility that contradicts it (true with public; false with private or
+        internal) still parses; a run adds a note naming the pair and the fix, to remove one of the
+        two keys.
       - >-
         Check diffs the declared keys against GET /repos; snapshot reads back the PATCH fields, topics,
         toggles, and GraphQL keys (docs/operate/snapshot.md).
@@ -40,6 +44,9 @@ coverage:
       - >-
         secret_scanning_validity_checks nests here too (documented in the GHEC flavor of the spec).
       - The GET echoes all of it, so check verifies it.
+      - >-
+        security_and_analysis: null still parses; a run adds a note naming the key and the fix, to
+        write each feature's status or remove the key, since the null names no feature to turn off.
   - area: "[Forking policy](https://docs.github.com/en/rest/repos/repos)"
     keys: allow_forking
     endpoints: []
@@ -167,7 +174,9 @@ schema:
   RepositoryConfig.homepage: >-
     A URL with more information about the repository; null clears it.
   RepositoryConfig.visibility: >-
-    public, private, or internal (Enterprise); a free string, since the spec omits internal.
+    public, private, or internal (Enterprise); a free string, since the spec omits internal. Beside
+    private it must agree with it: private: true with public, or private: false with private or
+    internal, is a contradiction a run notes.
   RepositoryConfig.default_branch: The default branch; an existing branch name.
   RepositoryConfig.pull_request_creation_policy: >-
     Who may create pull requests: "all" (everyone) or "collaborators_only".
@@ -193,7 +202,8 @@ schema:
   RepositoryConfig.web_commit_signoff_required: Require contributors to sign off on web-based commits (true) or not (false).
   RepositoryConfig.security_and_analysis: >-
     The security_and_analysis object of PATCH /repos/{r}: GitHub Advanced Security, secret scanning,
-    push protection, and the other nested feature toggles.
+    push protection, and the other nested feature toggles. Write each feature's status: null names
+    no feature to turn off, and a run notes it.
   RepositoryConfig.squash_merge_commit_title: >-
     The default title of a squash merge commit: PR_TITLE or COMMIT_OR_PR_TITLE. Required beside
     squash_merge_commit_message; legal pairs are PR_TITLE with PR_BODY, BLANK, or COMMIT_MESSAGES,

src/sections/repository/index.ts

@@ -491,6 +491,36 @@ function toggleTolerated(
     : `repository.${toggle.key}: ${meaning}, so nothing changed (${status})`;
 }
 
+/** The private flag each visibility means, by the PATCH's own description of the two fields; one outside the table is GitHub's to judge. A Map, so a visibility naming a prototype property cannot match. */
+const PRIVATE_BY_VISIBILITY: ReadonlyMap<unknown, boolean> = new Map([
+  ["public", false],
+  ["private", true],
+  ["internal", true],
+]);
+
+/** The notes the declaration alone draws, each naming the fix: a private flag that contradicts the visibility beside it, and a null security_and_analysis. */
+function declarationNotes(declared: RepositoryConfig): string[] {
+  const notes: string[] = [];
+
+  // The PATCH's descriptor says neither which of the two fields wins nor what a null does, hence each note's "if".
+  const meant = PRIVATE_BY_VISIBILITY.get(declared.visibility);
+  if (declared.private !== undefined && meant !== undefined && declared.private !== meant) {
+    notes.push(
+      `repository.private: ${declared.private} contradicts repository.visibility: ${JSON.stringify(declared.visibility)}, which means private: ${meant}, ` +
+        "so if GitHub keeps only one of them check reports the other as drift on every run. Remove private or visibility from the settings file",
+    );
+  }
+
+  if (declared.security_and_analysis === null) {
+    notes.push(
+      "repository.security_and_analysis: null names no feature to turn off, so check reports drift while the live repository has the mapping, " +
+        'and if GitHub ignores the null no apply clears it. Remove the key, or write each feature\'s status ("enabled" or "disabled") instead',
+    );
+  }
+
+  return notes;
+}
+
 /** The PATCH op when the declared fields differ from live; a declared key GitHub never echoes is noted first. */
 function planPatch(
   plan: RepositoryPlan,
@@ -802,7 +832,7 @@ export const repositorySection = {
   async plan(ctx, declared) {
     const section = this;
     return safeTry(async function* () {
-      const plan: RepositoryPlan = { ops: [], notes: [], drift: [] };
+      const plan: RepositoryPlan = { ops: [], notes: declarationNotes(declared), drift: [] };
       const desired: Record<string, unknown> = declared;
       const patch: Record<string, PlainData | undefined> = Object.fromEntries(
         Object.entries(declared).filter(([key]) => !SPECIAL_KEYS.has(key)),

src/sections/repository/schema.ts

@@ -499,8 +499,10 @@ const patchFieldShape = {
   homepage: CLEARABLE,
   private: repositoryToggle(),
   // A free string: `internal` is valid on Enterprise, and the pinned spec omits it.
+  // COMPAT(v4): a repository.visibility that contradicts repository.private still parses and draws the pair note of declarationNotes (index.ts); refuse the pair in RepositoryConfig's check with that note's fix, and delete the note.
   visibility: patchString().optional(),
   // Nullable as the PATCH body documents it; the object form stays closed.
+  // COMPAT(v4): repository.security_and_analysis: null still parses and draws the null note of declarationNotes (index.ts); drop .nullable() so the validator's null rule refuses it ("has no empty state"), and delete the note.
   security_and_analysis: SecurityAndAnalysisConfig.nullable().optional(),
   has_issues: repositoryToggle(),
   has_projects: repositoryToggle(),

test/sections/repository/repository.test.ts

@@ -208,6 +208,87 @@ describe("repository", () => {
     expect(result.ops.map((op) => op.role)).toEqual(["update"]);
   });
 
+  const SECRET_SCANNING_ON = { secret_scanning: { status: "enabled" } } as const;
+  const PAIR_FIX =
+    "so if GitHub keeps only one of them check reports the other as drift on every run. Remove private or visibility from the settings file";
+
+  test.each<[what: string, declared: Desired, notes: string[], drift: string[]]>([
+    [
+      "private: true beside visibility: public",
+      { private: true, visibility: "public" },
+      [
+        `repository.private: true contradicts repository.visibility: "public", which means private: false, ${PAIR_FIX}`,
+      ],
+      ["repository.private: true != false"],
+    ],
+    [
+      "private: false beside visibility: private",
+      { private: false, visibility: "private" },
+      [
+        `repository.private: false contradicts repository.visibility: "private", which means private: true, ${PAIR_FIX}`,
+      ],
+      ['repository.visibility: "private" != "public"'],
+    ],
+    [
+      "private: false beside visibility: internal",
+      { private: false, visibility: "internal" },
+      [
+        `repository.private: false contradicts repository.visibility: "internal", which means private: true, ${PAIR_FIX}`,
+      ],
+      ['repository.visibility: "internal" != "public"'],
+    ],
+    [
+      "control: private: true agrees with visibility: private",
+      { private: true, visibility: "private" },
+      [],
+      ["repository.private: true != false", 'repository.visibility: "private" != "public"'],
+    ],
+    [
+      "control: private: true agrees with visibility: internal",
+      { private: true, visibility: "internal" },
+      [],
+      ["repository.private: true != false", 'repository.visibility: "internal" != "public"'],
+    ],
+    [
+      "control: private: false agrees with visibility: public",
+      { private: false, visibility: "public" },
+      [],
+      [],
+    ],
+    [
+      "security_and_analysis: null",
+      { security_and_analysis: null },
+      [
+        "repository.security_and_analysis: null names no feature to turn off, so check reports drift while the live repository has the mapping, and if GitHub ignores the null no apply clears it. " +
+          'Remove the key, or write each feature\'s status ("enabled" or "disabled") instead',
+      ],
+      [
+        'repository.security_and_analysis: expected empty, live has {"secret_scanning":{"status":"enabled"}}',
+      ],
+    ],
+    [
+      "control: a declared security_and_analysis mapping",
+      { security_and_analysis: SECRET_SCANNING_ON },
+      [],
+      [],
+    ],
+  ])(
+    "a private/visibility pair that contradicts, and a null security_and_analysis, still parse and are noted with the fix, since the drift line names neither the cause nor the way out (%s)",
+    async (_what, declared, notes, drift) => {
+      // plan() takes its input through the validator, so a row the parse refused would fail here naming the refusal.
+      const api = new MockApi({
+        [GET]: {
+          data: { private: false, visibility: "public", security_and_analysis: SECRET_SCANNING_ON },
+        },
+      });
+      const result = await plan(api, declared);
+      expect({ notes: result.notes, drift: result.ops.flatMap((op) => op.drift) }).toEqual({
+        notes,
+        drift,
+      });
+    },
+  );
+
   test.each([
     ["vulnerability-alerts", { enabled: true }],
     ["automated-security-fixes", null],
@@ -1169,7 +1250,7 @@ describe("repository parse refusals", () => {
 
   test.each<[string, Record<string, unknown>]>([
     [
-      "security_and_analysis: null, which the PATCH body documents",
+      "security_and_analysis: null, which the PATCH body documents and the plan notes",
       { security_and_analysis: null },
     ],
     [

test/sections/repository/scenarios/repository-null-security-and-analysis-note.yml

@@ -0,0 +1,25 @@
+# security_and_analysis: null still parses. Against a live mapping, check
+# reports the drift it always did and adds one note naming the key and the fix
+# (write each feature's status), since the drift line names neither.
+name: repository-null-security-and-analysis-note
+settings:
+  repository:
+    security_and_analysis: null
+inputs:
+  mode: check
+live_state:
+  repo:
+    security_and_analysis:
+      secret_scanning:
+        status: enabled
+expect:
+  exit_code: 1
+  result: drift
+  outcomes:
+    repository: drift
+  never:
+    - "PATCH /repos/{repo}"
+  stdout_contains:
+    - "repository.security_and_analysis: null names no feature to turn off"
+    - "Remove the key, or write each feature's status"
+    - "drift: repository.security_and_analysis: expected empty, live has"

test/sections/repository/scenarios/repository-private-visibility-contradiction-note.yml

@@ -0,0 +1,22 @@
+# A private/visibility pair that contradicts (private: true beside visibility:
+# public) still parses. Check reports the drift it always did and adds one note
+# naming the pair and the fix, since the drift line names neither. Check mode
+# only: the pinned descriptor's PATCH body has no "internal" visibility.
+name: repository-private-visibility-contradiction-note
+settings:
+  repository:
+    private: true
+    visibility: public
+inputs:
+  mode: check
+expect:
+  exit_code: 1
+  result: drift
+  outcomes:
+    repository: drift
+  never:
+    - "PATCH /repos/{repo}"
+  stdout_contains:
+    - '::notice::repository.private: true contradicts repository.visibility: "public"'
+    - "Remove private or visibility from the settings file"
+    - "drift: repository.private: true != false"