diff --git a/docs/reference/coverage.md b/docs/reference/coverage.md index 0192a475..654b8fb1 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; 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`) diff --git a/docs/sections/repository.docs.yml b/docs/sections/repository.docs.yml index dd1fd58b..54bc58cd 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; 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, diff --git a/src/sections/repository/index.ts b/src/sections/repository/index.ts index efaf3b76..80f34ed4 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 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 = 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..96111381 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 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]>([ [ - "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..83d884c6 --- /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: + - '::notice::repository.private: true contradicts repository.visibility: "public"' + - "Remove private or visibility from the settings file" + - "drift: repository.private: true != false"