From 5c8510a5c8304e1d1311508d41c8040f5dec6479 Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Wed, 11 Nov 2026 22:57:51 -0500 Subject: [PATCH 1/3] fix(repository): name a contradictory private/visibility pair and a null security_and_analysis in a check-time note A `repository:` section that declares `private` beside a `visibility` that contradicts it (true with public; false with private or internal), or `security_and_analysis: null`, parsed and ran with nothing but a drift line that names neither the cause nor the fix. The plan now adds one note for each, built from the declaration alone and carried on the path the phantom-key note already uses. Each note names the key or the pair and the fix: remove one of the two keys, or write each feature's status instead of null. The drift lines are unchanged and nothing new is refused, so no settings file that ran before stops running. Both inputs complete their apply today, so the parse refusals wait for the next major: two COMPAT(v4) markers in the section's schema name the refusal that replaces each note. Each note states its consequence conditionally, because the PATCH's descriptor says neither which of the two fields wins nor what a null does to the mapping. --- docs/reference/coverage.md | 2 + docs/sections/repository.docs.yml | 14 +++- src/sections/repository/index.ts | 32 ++++++- src/sections/repository/schema.ts | 2 + test/sections/repository/repository.test.ts | 83 ++++++++++++++++++- ...sitory-null-security-and-analysis-note.yml | 25 ++++++ ...-private-visibility-contradiction-note.yml | 22 +++++ 7 files changed, 176 insertions(+), 4 deletions(-) create mode 100644 test/sections/repository/scenarios/repository-null-security-and-analysis-note.yml create mode 100644 test/sections/repository/scenarios/repository-private-visibility-contradiction-note.yml diff --git a/docs/reference/coverage.md b/docs/reference/coverage.md index b9148f1e..170e109e 100644 --- a/docs/reference/coverage.md +++ b/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; check 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; check 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`) diff --git a/docs/sections/repository.docs.yml b/docs/sections/repository.docs.yml index dd1fd58b..ebe8ca9e 100644 --- a/docs/sections/repository.docs.yml +++ b/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; check 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; check 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 check 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 check 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, diff --git a/src/sections/repository/index.ts b/src/sections/repository/index.ts index efaf3b76..e79faab3 100644 --- a/src/sections/repository/index.ts +++ b/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 = 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 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 = declared; const patch: Record = Object.fromEntries( Object.entries(declared).filter(([key]) => !SPECIAL_KEYS.has(key)), diff --git a/src/sections/repository/schema.ts b/src/sections/repository/schema.ts index c564d293..fa8b7efc 100644 --- a/src/sections/repository/schema.ts +++ b/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(), diff --git a/test/sections/repository/repository.test.ts b/test/sections/repository/repository.test.ts index 8c941412..05c66084 100644 --- a/test/sections/repository/repository.test.ts +++ b/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 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 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 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]>([ [ - "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 }, ], [ diff --git a/test/sections/repository/scenarios/repository-null-security-and-analysis-note.yml b/test/sections/repository/scenarios/repository-null-security-and-analysis-note.yml new file mode 100644 index 00000000..230a66b0 --- /dev/null +++ b/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" diff --git a/test/sections/repository/scenarios/repository-private-visibility-contradiction-note.yml b/test/sections/repository/scenarios/repository-private-visibility-contradiction-note.yml new file mode 100644 index 00000000..e511ce2c --- /dev/null +++ b/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: + - 'repository: private: true contradicts visibility: "public"' + - "Remove private or visibility from the settings file" + - "drift: repository.private: true != false" From 7bd23f11c1ee35fe7e5992f22bee9d40e6f53bbf Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Wed, 11 Nov 2026 23:26:44 -0500 Subject: [PATCH 2/3] fix(repository): lead the private/visibility note with its key path The pair note led with the section key, which the engine already prefixes to every line a section prints, so a run printed `repository: repository: private: true contradicts ...`. The note now leads with the key's path, as the security_and_analysis note and the drift lines do: `repository.private: true contradicts repository.visibility: "public", ...`. The four sentences in the section's docs said check adds the note. A plan's notes print in apply too, so they now say a run adds it, and the coverage page is regenerated from them. --- docs/reference/coverage.md | 4 ++-- docs/sections/repository.docs.yml | 8 ++++---- src/sections/repository/index.ts | 2 +- test/sections/repository/repository.test.ts | 6 +++--- .../repository-private-visibility-contradiction-note.yml | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/docs/reference/coverage.md b/docs/reference/coverage.md index 170e109e..9739acb3 100644 --- a/docs/reference/coverage.md +++ b/docs/reference/coverage.md @@ -84,7 +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; check adds a note naming the pair and the fix, to remove one of the two keys. +- 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`) @@ -93,7 +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; check 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. +- 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`) diff --git a/docs/sections/repository.docs.yml b/docs/sections/repository.docs.yml index ebe8ca9e..54bc58cd 100644 --- a/docs/sections/repository.docs.yml +++ b/docs/sections/repository.docs.yml @@ -24,7 +24,7 @@ coverage: 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; check adds a note naming the pair and the fix, to remove one of the + 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, @@ -45,7 +45,7 @@ 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; check adds a note naming the key and the fix, to + 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 @@ -176,7 +176,7 @@ schema: RepositoryConfig.visibility: >- 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 check notes. + 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". @@ -203,7 +203,7 @@ schema: 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. Write each feature's status: null names - no feature to turn off, and check notes it. + 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, diff --git a/src/sections/repository/index.ts b/src/sections/repository/index.ts index e79faab3..80f34ed4 100644 --- a/src/sections/repository/index.ts +++ b/src/sections/repository/index.ts @@ -506,7 +506,7 @@ function declarationNotes(declared: RepositoryConfig): string[] { const meant = PRIVATE_BY_VISIBILITY.get(declared.visibility); if (declared.private !== undefined && meant !== undefined && declared.private !== meant) { notes.push( - `repository: private: ${declared.private} contradicts visibility: ${JSON.stringify(declared.visibility)}, which means private: ${meant}, ` + + `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", ); } diff --git a/test/sections/repository/repository.test.ts b/test/sections/repository/repository.test.ts index 05c66084..96111381 100644 --- a/test/sections/repository/repository.test.ts +++ b/test/sections/repository/repository.test.ts @@ -217,7 +217,7 @@ describe("repository", () => { "private: true beside visibility: public", { private: true, visibility: "public" }, [ - `repository: private: true contradicts visibility: "public", which means private: false, ${PAIR_FIX}`, + `repository.private: true contradicts repository.visibility: "public", which means private: false, ${PAIR_FIX}`, ], ["repository.private: true != false"], ], @@ -225,7 +225,7 @@ describe("repository", () => { "private: false beside visibility: private", { private: false, visibility: "private" }, [ - `repository: private: false contradicts visibility: "private", which means private: true, ${PAIR_FIX}`, + `repository.private: false contradicts repository.visibility: "private", which means private: true, ${PAIR_FIX}`, ], ['repository.visibility: "private" != "public"'], ], @@ -233,7 +233,7 @@ describe("repository", () => { "private: false beside visibility: internal", { private: false, visibility: "internal" }, [ - `repository: private: false contradicts visibility: "internal", which means private: true, ${PAIR_FIX}`, + `repository.private: false contradicts repository.visibility: "internal", which means private: true, ${PAIR_FIX}`, ], ['repository.visibility: "internal" != "public"'], ], diff --git a/test/sections/repository/scenarios/repository-private-visibility-contradiction-note.yml b/test/sections/repository/scenarios/repository-private-visibility-contradiction-note.yml index e511ce2c..b3881fe8 100644 --- a/test/sections/repository/scenarios/repository-private-visibility-contradiction-note.yml +++ b/test/sections/repository/scenarios/repository-private-visibility-contradiction-note.yml @@ -17,6 +17,6 @@ expect: never: - "PATCH /repos/{repo}" stdout_contains: - - 'repository: private: true contradicts visibility: "public"' + - '::notice::repository: repository.private: true contradicts repository.visibility: "public"' - "Remove private or visibility from the settings file" - "drift: repository.private: true != false" From 6873e6a4de011f901a9851879e8022bedce41250 Mon Sep 17 00:00:00 2001 From: Vivswan Shah <58091053+Vivswan@users.noreply.github.com> Date: Thu, 12 Nov 2026 00:00:33 -0500 Subject: [PATCH 3/3] test(repository): expect the pair note under its key printed once The engine now leaves alone a section line that already leads with its key, so a run prints `::notice::repository.private: ...` where it printed `::notice::repository: repository.private: ...`. The scenario pinned the old line and failed on the merged tree. It now pins the line as a run prints it, and the note's own text is unchanged. --- .../repository-private-visibility-contradiction-note.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/sections/repository/scenarios/repository-private-visibility-contradiction-note.yml b/test/sections/repository/scenarios/repository-private-visibility-contradiction-note.yml index b3881fe8..83d884c6 100644 --- a/test/sections/repository/scenarios/repository-private-visibility-contradiction-note.yml +++ b/test/sections/repository/scenarios/repository-private-visibility-contradiction-note.yml @@ -17,6 +17,6 @@ expect: never: - "PATCH /repos/{repo}" stdout_contains: - - '::notice::repository: repository.private: true contradicts repository.visibility: "public"' + - '::notice::repository.private: true contradicts repository.visibility: "public"' - "Remove private or visibility from the settings file" - "drift: repository.private: true != false"