diff --git a/.github/scripts/gen-action-docs.ts b/.github/scripts/gen-action-docs.ts index f178b055d..7a049b3b3 100644 --- a/.github/scripts/gen-action-docs.ts +++ b/.github/scripts/gen-action-docs.ts @@ -199,6 +199,10 @@ export const POLICY_ROW_PROSE: Record = caveat: "owner always exempt", override: "manage listed people without removing others", }, + teams: { + caveat: "a grant made at the organization level is never touched", + override: "make the file the complete inventory of direct team grants, revoking the rest", + }, actions_variables: { override: "declare the managed variables, tolerate the rest" }, agents_variables: { override: "declare the managed variables, tolerate the rest" }, rulesets: { override: "make the file the complete ruleset inventory" }, diff --git a/COVERAGE.md b/COVERAGE.md index 598abe6b8..733281756 100644 --- a/COVERAGE.md +++ b/COVERAGE.md @@ -36,7 +36,7 @@ The tenet: this action can control everything about a repository and nothing abo | [Code scanning default setup](https://docs.github.com/en/rest/code-scanning/code-scanning) | `code_scanning_default_setup` | GET/PATCH /repos/{owner}/{repo}/code-scanning/default-setup, PATCH body verbatim (state, query_suite, languages, runner_type, runner_label, threat_model). A 202 answer means GitHub rolls the change out in a configuration run, which the log names. Check compares declared keys only, languages as a set. Needs GitHub Advanced Security on private repositories; a 403 can mean that (or an archived repository) rather than a missing permission. Snapshot reads the GET body back on the declared keys; a null the slice does not take (a not-configured setup's runner_type) is omitted. | | [Code quality setup](https://docs.github.com/en/rest/code-quality/code-quality) | `code_quality_setup` | GET/PATCH /repos/{owner}/{repo}/code-quality/setup, PATCH body verbatim (state, languages, runner_type, runner_label, ai_findings_option), the near field-for-field mirror of code_scanning_default_setup. A 202 answer means GitHub rolls the change out in a configuration run, which the log names; a 409 means a configuration run is already in progress (re-run the workflow after it finishes); a 403 can mean code quality is unavailable on the repository or the repository is archived, rather than a missing permission, and a 422 (the change cannot be applied) carries GitHub's message verbatim. Check compares declared keys only, languages as a set. ASSUMPTION: the GET is graded as a plain read under the Administration permission, mirroring code scanning - the fine-grained grading of these endpoints is not verified against production. The sibling /code-quality/findings endpoints are read-only and stay out of scope. Snapshot reads the GET body back on the declared keys; a null the slice does not take (a not-configured setup's runner_type) is omitted. | | [Collaborators](https://docs.github.com/en/rest/collaborators/collaborators) (direct) | `collaborators` | PUT/DELETE /repos/{owner}/{repo}/collaborators/{username} (affiliation=direct); pending invitations reconciled alongside via GET /repos/{owner}/{repo}/invitations and PATCH/DELETE /repos/{owner}/{repo}/invitations/{invitation_id}: a declared user's matching pending invitation converges, a stale permission is PATCHed in place (a declared custom role cannot be verified against an invitation - the standard-roles enum - so the invitation is kept with a note), an expired invitation is cancelled and re-sent, and undeclared pending invitations follow the section's one undeclared policy (email invitations, which no username can declare, are noted and left untouched); vocabulary mapping push<->write, pull<->read for check mode; custom org role names pass through; undeclared direct collaborators REMOVED by default (kept as notes under the wrapped `_undeclared: keep` form), owner never touched; new users get invitations. 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 repository owner, email invitations, and expired invitations are noted, not declared, and a live role no declaration plans as (a custom role named "push" or "pull") fails the collaborators section; the snapshot is written as partial without it. | -| [Team repository permissions](https://docs.github.com/en/rest/teams/teams) (org repos) | `teams` | PUT /orgs/{org}/teams/{slug}/repos/{owner}/{repo}; probes GET /orgs/{owner} and no-ops with a note on personal accounts (404 only; 403/5xx still fail). Both modes probe each 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. Snapshot lists the teams with access (GET /repos/{owner}/{repo}/teams) and reads each team's role through the same probe, so a custom role reads back by name; noted, not declared: access granted at the organization or enterprise level, a probe 404 (no access, or a fine-grained token missing the grant; the note names both), a role the probe does not report, and a role no declaration plans as (a custom role named "push" or "pull"). A personal account snapshots nothing. | +| [Team repository permissions](https://docs.github.com/en/rest/teams/teams) (org repos) | `teams` | PUT/DELETE /orgs/{org}/teams/{slug}/repos/{owner}/{repo}; 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. 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. Snapshot lists the same teams and reads each team's role through the same probe, so a custom role reads back by name, and writes the wrapped form with the keep default spelled out; noted, not declared: access granted at the organization or enterprise level, a probe 404 (no access, or a fine-grained token missing the grant; the note names both), a role the probe does not report, and a role no declaration plans as (a custom role named "push" or "pull"). A personal account snapshots nothing. | | [Milestones](https://docs.github.com/en/rest/issues/milestones) | `milestones` | POST/PATCH/DELETE /repos/{owner}/{repo}/milestones, matched by title, declared-keys-only (description/state untouched unless declared); undeclared milestones kept by default and surfaced as notes, because deleting a milestone DETACHES it from every issue carrying it - the wrapped `_undeclared: delete` form opts into exactly that. Snapshot reads every milestone, open and closed, back as entries under the keep default. | | [Interaction limits](https://docs.github.com/en/rest/interactions/repos) | `interaction_limits` | GET/PUT/DELETE /repos/{owner}/{repo}/interaction-limits, plus the two routed keys below. Limits self-expire (expiry tops out at six_months), so apply re-arms the declared limit on EVERY run and check mode reports drift once it lapses - schedule apply more often than the chosen expiry to keep it armed. The declared expiry is write-only (GitHub reads back only the computed expires_at), so check verifies the limit value, not the duration. `interaction_limits: null` clears a live repo-level limit - the base limit only, never the cap or bypass list below. In a mode: merge fold, interaction_limits: null over a lower layer's declaration removes the key from the merged document instead, mirroring pages: null. A 409 means an organization- or user-level limit overrides the repository's; writes surface that as a note, not a failure, while check mode still reports a mismatched declaration as drift - the org is the place to change it. `pull_request_creation_cap` routes to GET/PATCH /repos/{owner}/{repo}/interaction-limits/pulls/creation-cap: persistent desired state with no self-expiry (enabled plus max_open_pull_requests 1-1000, read back verbatim), so check diffs it exactly and apply PATCHes only on divergence - no re-arm. Where the cap is unavailable the endpoints answer 405, which apply surfaces as a note (like the 409) and check as honest drift. `pull_request_creation_bypass` routes to GET/PUT/DELETE /repos/{owner}/{repo}/interaction-limits/pulls/bypass-list and reconciles: apply DELETEs only the undeclared logins and then PUTs only the missing ones ({users: [logins]}, compared case-insensitively; removals go first because the list holds at most 100 users, which also caps the declaration), never a wholesale replace; a declared empty list removes everyone. Snapshot reads back the repository-level limit (an inherited one is noted, not declared, and the expiry duration is not readable), the cap when the feature exists and is enabled, and the bypass list when it has members. | | [Actions variables](https://docs.github.com/en/rest/actions/variables) | `actions_variables` | GET/POST /repos/{owner}/{repo}/actions/variables and PATCH/DELETE /repos/{owner}/{repo}/actions/variables/{name}: plain-text repository variables, upserted by name. Names are case-insensitive (GitHub stores them uppercased regardless of how they are entered), so matching and duplicate rejection compare uppercased names. Values read back in full, which is what makes exact check-mode diffing possible; undeclared variables are DELETED by default, kept as notes under the wrapped `_undeclared: keep` form. The list endpoint caps per_page at 30 (not the usual 100), which the page loop honors so a large inventory is never truncated. Secrets are write-only material and deliberately NOT this section: they live in the `actions_secrets` section above. Snapshot reads the variables back as name and value entries under the delete default. | diff --git a/action.yml b/action.yml index e1b140ff3..3da0b9629 100644 --- a/action.yml +++ b/action.yml @@ -52,11 +52,10 @@ inputs: document is written to (parent directories are created). The file holds exactly what apply would run: every section validated, each section that takes an undeclared policy in its policy-wrapper form with the policy - made explicit, the other sections in their own shape, and private - underscore keys and the _layering directives dropped. Feed it to a later - apply or check step as its settings-file. Must not name one of the - settings-file layers (the merge would overwrite it). Fails when set in - apply or check. + made explicit, the other sections in their own shape, and the _layering + directives dropped. Feed it to a later apply or check step as its + settings-file. Must not name one of the settings-file layers (the merge + would overwrite it). Fails when set in apply or check. required: false default: "" snapshot-file: diff --git a/docs/operate/layering.md b/docs/operate/layering.md index 993c3eb56..5871c3d28 100644 --- a/docs/operate/layering.md +++ b/docs/operate/layering.md @@ -70,7 +70,7 @@ The step ends with `result: merged` and exit 0, or exit 1 with an error naming t | `_layering` | A list section's `{entries}` wrapper, or a file's top level | Merge time | Overrides `layering` for that section, or for every section of that file. Consumed by the merge: the merged file never carries it | | `_undeclared` | A list section's `{entries}` wrapper | Live state | What apply does to live resources the document does not declare: `keep` or `delete`. Travels through the merge and is resolved in the merged file. The [undeclared policy](../reference/undeclared-policy.md) page owns it | -The two underscore keys are this action's directives, never GitHub settings. Any other underscore key at a file's top level is a private note and is dropped from the merged file. +The two underscore keys are this action's directives, never GitHub settings, and they are the whole underscore vocabulary: any other underscore key, at a file's top level or on a wrapper, fails validation with an error naming these two (a note belongs in a YAML comment). A misspelled directive can therefore never pass as a private note and quietly merge a layer meant to replace. ## The rules @@ -193,8 +193,8 @@ Reading it back: The merged file is exactly what apply runs, so it is worth knowing its shape: -- Every list section that takes the `_undeclared` knob (the fifteen the [undeclared policy](../reference/undeclared-policy.md) lists) is in its `{_undeclared, entries}` wrapper form, with `_undeclared` resolved to an explicit `keep` or `delete`. Other lists (`branches`, `environments`, and the nested per-environment lists) stay as written. -- No `_layering` anywhere, and no private underscore notes: both are consumed before the file is written. +- Every list section that takes the `_undeclared` knob (the sections the [undeclared policy](../reference/undeclared-policy.md) counts in its opening sentence) is in its `{_undeclared, entries}` wrapper form, with `_undeclared` resolved to an explicit `keep` or `delete`. Other lists (`branches`, `environments`, and the nested per-environment lists) stay as written. +- No `_layering` anywhere: the directive is consumed before the file is written, and YAML comments do not survive the fold. - A `null` that met nothing below stays, keeping its engine meaning. - Every layer was validated on its own before the fold, and the result is validated again before it is written. @@ -256,6 +256,7 @@ The per-layer validation catches what a standalone settings file could not say, | A list section that is not a list or an `{entries}` wrapper (`labels: oops`) | Validation: `labels: Invalid input: expected a list of entries, or a mapping with "entries" (and an optional "_undeclared" policy), but this section parsed as string` | | A keyed entry without its key, here a label with no `name` (`labels: [{name: bug}, {color: d73a4a}]`) | Validation: `labels[1].name: Invalid input: expected string, received undefined` | | A non-mapping entry (`milestones: [v2]`) | Validation: `milestones[0]: Invalid input: expected object, received string` | +| An underscore key that is not a directive on a wrapper (`labels: {_notes: x, entries: [{name: bug}]}`) | Validation: `labels: Unrecognized key: "_notes"; the wrapper's directives are "_undeclared" and, on a top-level section, "_layering", and nothing else - there are no private-note keys. Remove the key, or keep the note as a YAML comment` | The fold itself refuses what only a merge can judge (`layer ".github/settings/repo.yml": ...`). A fold refusal names the key path (entries by index) and the kind of problem, never a value from the document: the merge runs without a repository's redaction context, so a label name or rule type echoed here could put a private repository's settings into a public log. @@ -285,7 +286,7 @@ A merge-mode log can therefore show your settings file's structure and, through | `_layering: merge` on a section with no layering key, on its wrapper or reached from the file level (`milestones: {_layering: merge, entries: [{title: v1}]}` or `{_layering: merge, milestones: [{title: v1}]}`) | `milestones has no layering key, so it cannot be layered by "merge"; declare _layering: replace or drop the directive` | | An unknown `_layering` value on a wrapper (`labels: {_layering: union, entries: [{name: bug}]}`) | `labels._layering must be "merge" or "replace"; got a string that is neither` | | An unknown `_layering` value at the file's top level (`_layering: union`) | `_layering must be "merge" or "replace"; got a string that is neither` | -| A YAML anchor aliased inside its own node (`_notes: &loop {self: *loop}`) | `the document contains a reference cycle (a YAML anchor that includes itself); layers must be trees` | +| A YAML anchor aliased inside its own node (`repository: &loop {self: *loop}`) | `the document contains a reference cycle (a YAML anchor that includes itself); layers must be trees` | ## Where to go next diff --git a/docs/operate/troubleshooting.md b/docs/operate/troubleshooting.md index ba20c1ad4..4407ef60a 100644 --- a/docs/operate/troubleshooting.md +++ b/docs/operate/troubleshooting.md @@ -50,10 +50,10 @@ What you see: the run fails during validation, naming the unknown keys and listi What it means: a misspelled section that silently did nothing would break the loud-failure promise, so unknown top-level keys are hard errors (see [Forward compatibility](../reference/forward-compatibility.md)). -What to do: the message names all three options. Fix the typo. Or, for a deliberate private key, prefix it with an underscore, which the validator skips: +What to do: the message names both options. Fix the typo, keeping any note as a YAML comment (an underscore key is not a private note: `_owner_notes: ...` fails with its own error naming the two directives the underscore is reserved for): ```yaml settings -_owner_notes: contact the platform team before editing this file +# owner notes: contact the platform team before editing this file labels: - name: bug color: d73a4a diff --git a/docs/playbooks/incident-freeze.md b/docs/playbooks/incident-freeze.md index 3f9fd796a..3dfc1b1d8 100644 --- a/docs/playbooks/incident-freeze.md +++ b/docs/playbooks/incident-freeze.md @@ -7,8 +7,7 @@ order: 390 During an incident, merges must stop in minutes and be restored just as fast, with both directions on the record. Two profile files and one dispatch workflow do it. The freeze: ```yaml settings -# .github/profiles/freeze.yml -_incident: INC-4471 +# .github/profiles/freeze.yml, incident INC-4471 rulesets: - name: incident freeze diff --git a/docs/reference/inputs.md b/docs/reference/inputs.md index 4b9248e9c..256c58119 100644 --- a/docs/reference/inputs.md +++ b/docs/reference/inputs.md @@ -48,3 +48,17 @@ The discovery-only inputs apply to `repos: "*"`; the [multi-repo guide](../opera All three outputs are set on every run, whatever the mode and however it ended. A `check` run exits 1 on any drift, so a downstream step usually reads `result` only when the step runs with `continue-on-error: true` or through `if: always()`. [Check mode](../operate/check-mode.md) has the exit-code table. + +## Environment variables + +Beside the inputs, a run reads these from its environment. The Actions runner sets every `GITHUB_*` and `ACTIONS_*` name below except `GITHUB_TOKEN`, which a workflow exports itself; the [command line](../start/cli.md) reads `GITHUB_TOKEN`, `GITHUB_REPOSITORY`, `GITHUB_API_URL`, and `GSAC_RETRY_BASE_MS` under the same names. + +| Variable | Read for | +|---|---| +| `GITHUB_TOKEN` | The token when the `token` input (or `--token`) is empty | +| `GITHUB_REPOSITORY` | The default `repository`, the owner of a bare `.yml` under `repos-dir`, and the one target never redacted | +| `GITHUB_SERVER_URL`, `GITHUB_RUN_ID` | The run link a private report carries | +| `GITHUB_API_URL` | The REST base URL (GitHub Enterprise Server) | +| `GITHUB_OUTPUT`, `GITHUB_STEP_SUMMARY` | Where the action writes its outputs and the step summary (the command line prints them instead) | +| `ACTIONS_RUNTIME_TOKEN` | The artifact service's credential; `private-report: artifact` warns and uploads nothing without it (GitHub Enterprise Server) | +| `GSAC_RETRY_BASE_MS` | A test knob: the real milliseconds in one retry-backoff second. Set, it also selects the immediate scheduler, so the rate-limit `Retry-After` waits and the write limiter's spacing are skipped rather than scaled. Unset, a second is a second and the waits are real. Set it only to make a retry scenario finish in milliseconds against a mock; against GitHub the skipped waits earn the next secondary rate limit | diff --git a/docs/reference/sections.md b/docs/reference/sections.md index 92a0d0869..b0deec386 100644 --- a/docs/reference/sections.md +++ b/docs/reference/sections.md @@ -26,7 +26,7 @@ Every top-level key a settings file can declare, one row per section: the endpoi | `code_scanning_default_setup` | code scanning default setup | Administration or Code scanning alerts: write | untouched | `state`, `query_suite`, `languages`; needs Advanced Security on private repositories | | `code_quality_setup` | code-quality setup | Administration: write | untouched | `state`, `languages`, runner and AI-findings options; a 202 means GitHub rolls the change out in a configuration run; needs code quality available on the repository | | `collaborators` | direct collaborators + pending invitations | Administration: write | deleted (settable) | invitations for new users, pending ones reconciled (stale permission updated, expired re-sent, undeclared cancelled); the repository owner is never touched | -| `teams` | org team repo permissions + repo team list | Members: read (org permission) + Administration: write | untouched | org repos only, skipped with a notice on personal accounts | +| `teams` | org team repo permissions + repo team list | Members: read (org permission) + Administration: write | kept (settable) | org repos only, skipped with a notice on personal accounts; undeclared teams kept by default (`_undeclared: delete` revokes their direct access) | | `milestones` | milestones | Issues: write | kept (settable) | upsert by title; deleting a milestone detaches it from every issue carrying it, which is why keep is the default | | `interaction_limits` | interaction-limits + pulls creation-cap/bypass-list | Administration: write | untouched | re-arms the self-expiring limit every apply run; `null` clears it (base limit only); a 409 (org/user-level limit overrides) becomes a note; the PR creation cap is persistent (PATCHed only on divergence, 405 where unavailable) and its bypass logins reconcile add/remove | | `actions_variables` | Actions variables CRUD | Variables: write | deleted (settable) | plain-text variables upserted by name (case-insensitive); values read back in full, so check mode diffs them exactly | diff --git a/docs/reference/undeclared-policy.md b/docs/reference/undeclared-policy.md index ecee82abf..05156db26 100644 --- a/docs/reference/undeclared-policy.md +++ b/docs/reference/undeclared-policy.md @@ -5,7 +5,7 @@ order: 150 # The undeclared policy -Fifteen sections list the live resources sitting next to the declared ones: `labels`, `autolinks`, `collaborators`, `actions_variables`, `agents_variables`, `rulesets`, `actions_secrets`, `dependabot_secrets`, `codespaces_secrets`, `agents_secrets`, `milestones`, `webhooks`, `custom_properties`, `deploy_keys`, and `secret_scanning_custom_patterns`. +Sixteen sections list the live resources sitting next to the declared ones: `labels`, `autolinks`, `collaborators`, `actions_variables`, `agents_variables`, `rulesets`, `actions_secrets`, `dependabot_secrets`, `codespaces_secrets`, `agents_secrets`, `teams`, `milestones`, `webhooks`, `custom_properties`, `deploy_keys`, and `secret_scanning_custom_patterns`. Each has a default answer for a live resource the settings file does not declare, and each accepts a wrapped form that overrides it per file. This page is the normative statement of that policy: the knob, the defaults per section, and how it travels through a layered merge. The [Sections table](sections.md) states each section's default in its Undeclared default column; this page says what the defaults mean and how to change them. @@ -51,6 +51,7 @@ Before v3 the policy key was spelled `undeclared`. A file still writing it fails | `dependabot_secrets` | keep | `delete`: prune stale secrets - a deleted secret's value is unrecoverable | | `codespaces_secrets` | keep | `delete`: prune stale secrets - a deleted secret's value is unrecoverable | | `agents_secrets` | keep | `delete`: prune stale secrets - a deleted secret's value is unrecoverable | +| `teams` | keep (a grant made at the organization level is never touched) | `delete`: make the file the complete inventory of direct team grants, revoking the rest | | `milestones` | keep | `delete`: prune stale milestones, with the caveat below | | `webhooks` | keep (integrations create their own hooks) | `delete`: make the file the complete hook inventory | | `custom_properties` | keep (an unset can revert to an org default the file does not model) | `delete`: make the file the complete property-value inventory, unsetting the rest | diff --git a/docs/start/examples.md b/docs/start/examples.md index 9490976cd..1ac7cc966 100644 --- a/docs/start/examples.md +++ b/docs/start/examples.md @@ -282,13 +282,12 @@ branches: In a `mode: merge` fold, a `null` over a key a lower layer declared removes that key from the merged document instead; the [layering guide](../operate/layering.md) has the rules. A multi-repo `defaults-file` never merges into a target's file, so a `null` there keeps the meanings above. A few individual fields accept `null` as a value of their own too, such as `pages.cname` to remove a custom domain; the [published schema](https://github.com/Vivswan/github-settings-as-code/blob/main/lib/settings.schema.json) marks those. -## Private notes +## Notes in the file -Unknown top-level sections are hard errors, so a typo cannot silently do nothing (the one exception: under a `sections` allowlist, unknown keys outside the allowlist warn instead of failing, which eases version skew; the [troubleshooting guide](../operate/troubleshooting.md) covers it). Top-level keys starting with an underscore are the escape hatch: they are ignored, which makes them usable as private notes. The one exception is `_layering`, a merge-time directive the [layering guide](../operate/layering.md) owns. +Unknown top-level sections are hard errors, so a typo cannot silently do nothing (the one exception: under a `sections` allowlist, unknown keys outside the allowlist warn instead of failing, which eases version skew; the [troubleshooting guide](../operate/troubleshooting.md) covers it). An underscore key is not an escape hatch: the underscore marks this action's two directives, `_layering` and `_undeclared` (the [layering guide](../operate/layering.md) owns them), and any other underscore key fails validation the same way. A note belongs in a YAML comment: ```yaml settings -_owner: platform-team, see runbook RB-112 - +# owner: platform-team, see runbook RB-112 labels: - name: bug color: "d73a4a" diff --git a/docs/start/migrating-from-probot.md b/docs/start/migrating-from-probot.md index 2b92dc198..3604286b7 100644 --- a/docs/start/migrating-from-probot.md +++ b/docs/start/migrating-from-probot.md @@ -33,7 +33,7 @@ Your existing settings.yml keeps working for `repository`, `labels`, `branches`, The delivery model is a workflow plus a fine-grained PAT, not an app installation. You mint the token, scope it to exactly the sections your file declares, and save it as a repository secret; permission errors name the exact grant to add. See [Token permissions](../reference/permissions.md). -Failures are loud. An unknown top-level key in the settings file is a hard error, not a silent no-op, because a misspelled section that quietly did nothing is the app's failure mode this action exists to replace. Prefix a deliberate private key with an underscore to keep notes in the file. The closed sections also reject entry keys they do not recognize: in `collaborators` and `teams` a misspelled `permission` key would silently grant the default role, and in `workflows` the enable/disable calls send no payload, so an unrecognized key would silently do nothing. [Forward compatibility](../reference/forward-compatibility.md) lists the full closed set. +Failures are loud. An unknown top-level key in the settings file is a hard error, not a silent no-op, because a misspelled section that quietly did nothing is the app's failure mode this action exists to replace; an unknown underscore key is one too, so a note goes in a YAML comment. The closed sections also reject entry keys they do not recognize: in `collaborators` and `teams` a misspelled `permission` key would silently grant the default role, and in `workflows` the enable/disable calls send no payload, so an unrecognized key would silently do nothing. [Forward compatibility](../reference/forward-compatibility.md) lists the full closed set. The engine is stateless. There is no state file and nothing is stored between runs; resources are matched by their natural names, and only declared keys are ever applied or compared. Removing a section from the file stops managing it; it does not revert anything. diff --git a/docs/upgrading/v2-to-v3.md b/docs/upgrading/v2-to-v3.md index c4cff6864..6b4e778f8 100644 --- a/docs/upgrading/v2-to-v3.md +++ b/docs/upgrading/v2-to-v3.md @@ -4,7 +4,7 @@ order: 20 # Upgrading from v2 to v3 -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). +Eighteen 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 | |---|---|---|---| @@ -23,6 +23,9 @@ Fifteen breaks (the ninth is for library consumers). One is silent (the fallback | 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) | +| Underscore keys are directives, never notes | An unknown `_note: ...` at the top level was dropped silently | Only `_layering` and `_undeclared` exist; any other underscore key fails validation, on a wrapper and at the top level alike | Validation fails before any section runs, naming the two directives; [section 16](#16-underscore-keys-are-directives-never-notes) | +| `teams` takes the `_undeclared` knob | A plain array; an undeclared team was never listed or touched | `teams: {_undeclared: keep, entries: [...]}` accepted, default `keep`; `delete` revokes undeclared direct grants | No error. Every run now lists the repository's teams and notes each undeclared one; snapshots write the wrapper form; [section 17](#17-teams-takes-the-_undeclared-knob) | +| `GSAC_RETRY_BASE_MS` | `RETRY_BASE_MS`, undocumented | `GSAC_RETRY_BASE_MS`, in the inputs reference | No error: an unknown environment variable is ignored, so a harness setting the old name waits real seconds; [section 18](#18-gsac_retry_base_ms) | ## 1. The defaults-file fallback @@ -54,7 +57,7 @@ The old spelling fails validation before any section runs, so a check run finds .github/settings.yml has malformed section entries: labels: Unrecognized key: "undeclared"; the wrapper's policy key "undeclared" was renamed to "_undeclared" in v3 (a directive, like _layering) - write _undeclared: keep or _undeclared: delete. Fix these values in the settings file (only the named keys are validated; extra fields pass through, except in closed sections and strict nested objects like actions.cache, which reject unrecognized keys) ``` -The underscore marks the action's directives: `_undeclared` (live axis) and `_layering` (merge time). Other underscore keys at the top level stay private notes. The [undeclared policy](../reference/undeclared-policy.md) page owns the knob. +The underscore marks the action's directives: `_undeclared` (live axis) and `_layering` (merge time), and nothing else ([section 16](#16-underscore-keys-are-directives-never-notes)). The [undeclared policy](../reference/undeclared-policy.md) page owns the knob. ## 3. Layering only in mode: merge @@ -241,9 +244,42 @@ The other cannot-verify and omission notes (interaction_limits, teams, collabora 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. +## 16. Underscore keys are directives, never notes + +v2 dropped any unknown top-level key starting with `_` as a private note, while rejecting the same key inside a section's `{entries}` wrapper. v3 has one rule everywhere: the underscore belongs to the two directives, `_layering` and `_undeclared`, and any other underscore key fails validation before any section runs. The one corner it does not reach: a `null`-valued underscore key inside a wrapper (`labels: {_notes: null, entries: []}`) is a merge marker the fold strips before it judges the layer, so a `mode: merge` run passes it silently. + +```yaml settings +# owner: platform-team, see runbook RB-112 +labels: + - name: bug + color: "d73a4a" +``` + +A v2 file with `_owner: platform-team` at its top level now fails with: + +```text +unknown underscore key(s) in .github/settings.yml: _owner. The underscore marks this action's directives, "_layering" (a file's top level or a list section's {entries} wrapper) and "_undeclared" (a wrapper), and nothing else; there are no private-note keys. Remove the key, or keep the note as a YAML comment +``` + +A `sections` allowlist does not soften it (an unknown plain section outside the allowlist still only warns). The reason is the loud-failure promise: a misspelled `_layerin: replace` dropped as a note would merge a layer its author meant to replace. Move each note into a YAML comment; the [layering guide](../operate/layering.md#three-knobs) states the rule beside the two directives. + +## 17. teams takes the _undeclared knob + +`teams` joins the sections that take the `{_undeclared, entries}` wrapper, with the default `keep`: a team with direct access that the file does not name is left alone, as before. What changes: + +- Every apply and check now lists the repository's teams (`GET /repos/{owner}/{repo}/teams`; the same Administration grant) and prints one note per undeclared direct team, naming the knob. +- `_undeclared: delete` revokes the direct grants the file does not name (`DELETE /orgs/{org}/teams/{slug}/repos/{owner}/{repo}`). Access granted at the organization or enterprise level is never touched; under `delete` it is noted as beyond the repository's reach. +- `mode: snapshot` and `gsac init` write the wrapper form with `_undeclared: keep` spelled out, as the other knobbed sections do. + +The plain array form keeps working, and the [undeclared policy](../reference/undeclared-policy.md) page lists the default beside the others. + +## 18. GSAC_RETRY_BASE_MS + +The one environment variable of the tool's own, the retry-timing test knob, is `GSAC_RETRY_BASE_MS`; v2 read it as `RETRY_BASE_MS`, undocumented. An environment variable has no channel for a loud error, so the old name is simply ignored: a harness that set `RETRY_BASE_MS=1` to speed a mock run up now waits real seconds until it is renamed. The [inputs reference](../reference/inputs.md#environment-variables) lists it with the other variables a run reads. + ## 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. +1. Rename any settings file whose path contains a comma, rename `undeclared` to `_undeclared` in every settings file, and move every other underscore key into a YAML comment; the v2 line accepts the old spellings only, so do all three 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. Where a snapshot wrote a `$WEBHOOK_SECRET_` reference, change the reference and its exported variable to `SECRET_WEBHOOK_` together, or re-snapshot. 4. Move the pin to `@v3` with `mode: check`. diff --git a/lib/settings.schema.json b/lib/settings.schema.json index 32e34d314..0e899a79d 100644 --- a/lib/settings.schema.json +++ b/lib/settings.schema.json @@ -1274,11 +1274,18 @@ ] }, "teams": { - "description": "Org team access to the repo; skipped on personal accounts.", - "type": "array", - "items": { - "$ref": "#/definitions/TeamConfig" - } + "description": "Org team access to the repo; skipped on personal accounts. Undeclared teams are kept by default and noted; the wrapped form can set `_undeclared: delete` to revoke their direct access (a grant made at the organization level is never touched). A declared child team can hold its access through an undeclared parent's grant, which `delete` revokes: declare the parent too, or keep.", + "anyOf": [ + { + "type": "array", + "items": { + "$ref": "#/definitions/TeamConfig" + } + }, + { + "$ref": "#/definitions/UndeclaredPolicyList%3CTeamConfig%3E" + } + ] }, "milestones": { "description": "Milestones, upserted by title; undeclared ones are kept by default (the wrapped form can set `_undeclared: delete`, which detaches deleted milestones from their issues).", @@ -1986,6 +1993,39 @@ ], "additionalProperties": false }, + "UndeclaredPolicyList": { + "description": "The wrapped form of a list, overriding what happens to live resources the file does not declare. The plain array form keeps the list's own default policy (for a top-level section that is the section default, and a multi-repo defaults file can set it; a nested list such as environments[].variables has its own fixed default and never inherits one); this wrapper can set it explicitly, and with `_undeclared` omitted it behaves exactly like the plain array. The wrapper is this action's own vocabulary (nothing here passes through to GitHub), so its keys are strict: anything besides `_undeclared`, `entries`, and `_layering` (the directives wear the underscore; a nested list's wrapper does not take `_layering`) is rejected upfront as a typo.", + "type": "object", + "properties": { + "_undeclared": { + "description": "What apply does to live resources `entries` does not declare: \"delete\" removes them, \"keep\" leaves them alone and surfaces each as a note. Omitted, the list's own default applies.", + "allOf": [ + { + "$ref": "#/definitions/UndeclaredPolicy" + } + ] + }, + "entries": { + "description": "The declared entries, exactly as the plain array form lists them.", + "type": "array", + "items": { + "$ref": "#/definitions/TeamConfig" + } + }, + "_layering": { + "description": "A merge-time directive: how this section combines with the layers BELOW it in a layered merge. \"replace\" lets this layer's list win. \"merge\" unions the entries by identity and is accepted only on a section whose entries have one (labels by name, rulesets by name with their rules by type); on any other section it is refused, so declare \"replace\" or omit the key. Omitted, the run's layering applies. It has no effect on a single document, the merged document never carries it, and a list nested inside a section entry does not take it (a higher layer replaces such a list wholesale).", + "type": "string", + "enum": [ + "merge", + "replace" + ] + } + }, + "required": [ + "entries" + ], + "additionalProperties": false + }, "UndeclaredPolicyList": { "description": "The wrapped form of a list, overriding what happens to live resources the file does not declare. The plain array form keeps the list's own default policy (for a top-level section that is the section default, and a multi-repo defaults file can set it; a nested list such as environments[].variables has its own fixed default and never inherits one); this wrapper can set it explicitly, and with `_undeclared` omitted it behaves exactly like the plain array. The wrapper is this action's own vocabulary (nothing here passes through to GitHub), so its keys are strict: anything besides `_undeclared`, `entries`, and `_layering` (the directives wear the underscore; a nested list's wrapper does not take `_layering`) is rejected upfront as a typo.", "type": "object", diff --git a/src/engine/layers.ts b/src/engine/layers.ts index 4253eac59..c28b565b5 100644 --- a/src/engine/layers.ts +++ b/src/engine/layers.ts @@ -74,7 +74,7 @@ function resolveUndeclaredPolicies(merged: Record): void { /** * The clone in progress of every node on the current descent: a node met again inside itself gets that clone, so a * cyclic document terminates. A node aliased twice WITHOUT enclosing itself is cloned per occurrence, in the position - * each sits in (a wrapper shared between a private key and `rulesets` is data under one and a keyed list under the other). + * each sits in (a wrapper aliased under an open section and under `rulesets` is data under one and a keyed list under the other). */ type Descent = WeakMap; diff --git a/src/engine/orchestrate.ts b/src/engine/orchestrate.ts index a83245a4c..58e8582ba 100644 --- a/src/engine/orchestrate.ts +++ b/src/engine/orchestrate.ts @@ -8,7 +8,12 @@ import type { RepoRef } from "../discovery/targets.js"; import type { GitHubClient } from "../github/api.js"; import type { Io } from "../io.js"; import type { SettingsProblem, TopLevelShape } from "../problem.js"; -import { SECTION_KEYS, type SectionKey, type SettingsFile } from "../schema.js"; +import { + DOCUMENT_DIRECTIVE_KEYS, + SECTION_KEYS, + type SectionKey, + type SettingsFile, +} from "../schema.js"; import { PermissionDenied } from "../sections/contract/errors.js"; import { type ExecTools, @@ -85,7 +90,8 @@ export function skippedSectionKeys( /** * The ONE boundary that turns a raw parsed document into the ValidatedSettings the engine accepts. Unknown top-level - * keys are errors, except outside a non-empty `sections` allowlist, where they downgrade to a warning. + * keys are errors, except outside a non-empty `sections` allowlist, where they downgrade to a warning; an unknown + * underscore key is an error under every allowlist, since the underscore names this action's directives and nothing else. */ export function validateSettingsDoc( settings: unknown, @@ -107,12 +113,22 @@ export function validateSettingsDoc( return err({ code: "settings-not-plain-mapping", source: sourceLabel }); } const knownSections = new Set(SECTION_KEYS); + const directives = new Set(DOCUMENT_DIRECTIVE_KEYS); const allowed: ReadonlySet = onlySections; - // A misspelled section silently doing nothing would break the loud-failure promise; underscore-prefixed keys are the - // sanctioned place for private notes. - const unknownKeys = Object.keys(settings).filter( - (key) => !knownSections.has(key) && !key.startsWith("_"), + // A misspelled section silently doing nothing would break the loud-failure promise, and so would a misspelled + // directive: `_layerin: replace` dropped as a private note would merge a layer the author meant to replace. + const strangers = Object.keys(settings).filter( + (key) => !knownSections.has(key) && !directives.has(key), ); + const unknownDirectives = strangers.filter((key) => key.startsWith("_")); + if (unknownDirectives.length > 0) { + return err({ + code: "settings-unknown-directives", + source: sourceLabel, + unknown: unknownDirectives, + }); + } + const unknownKeys = strangers.filter((key) => !key.startsWith("_")); if (unknownKeys.length > 0) { if (onlySections.size === 0 || unknownKeys.some((key) => allowed.has(key))) { return err({ diff --git a/src/flows/inputs.ts b/src/flows/inputs.ts index ddb8c7fd3..41d929649 100644 --- a/src/flows/inputs.ts +++ b/src/flows/inputs.ts @@ -114,8 +114,8 @@ export const INPUT_DECLS = { "mode: merge only, and required there: the path the merged settings document is written to " + "(parent directories are created). The file holds exactly what apply would run: every " + "section validated, each section that takes an undeclared policy in its policy-wrapper form " + - "with the policy made explicit, the other sections in their own shape, and private " + - "underscore keys and the _layering directives dropped. Feed it to a later apply or check " + + "with the policy made explicit, the other sections in their own shape, and the _layering " + + "directives dropped. Feed it to a later apply or check " + "step as its settings-file. Must not name one of the settings-file layers (the merge would " + "overwrite it). Fails when set in apply or check.", default: "", diff --git a/src/github/api.ts b/src/github/api.ts index 77a25241f..9ac08eae9 100644 --- a/src/github/api.ts +++ b/src/github/api.ts @@ -273,9 +273,9 @@ function isHttpError(error: unknown): error is OctokitHttpError { ); } -/** RETRY_BASE_MS is the one knob the e2e runner sets: millisecond plugin units and the immediate scheduler for the spawned bundle. */ +/** GSAC_RETRY_BASE_MS is the one knob the e2e runner sets: millisecond plugin units and the immediate scheduler for the spawned bundle. */ function envRetryBaseMs(): number | undefined { - const value = Number(process.env.RETRY_BASE_MS ?? ""); + const value = Number(process.env.GSAC_RETRY_BASE_MS ?? ""); return Number.isFinite(value) && value > 0 ? value : undefined; } @@ -386,10 +386,10 @@ export interface GitHubApiOptions { apiVersion?: string; /** * Real milliseconds in one plugin second: Retry-After units, the retry backoff step, and the write limiter's gap. - * Undefined reads RETRY_BASE_MS once; the plugin topology is the same at every value. + * Undefined reads GSAC_RETRY_BASE_MS once; the plugin topology is the same at every value. */ retryBaseMs?: number; - /** The limiter the throttling plugin paces through; TIMERS_SCHEDULER unless RETRY_BASE_MS selects the immediate one. */ + /** The limiter the throttling plugin paces through; TIMERS_SCHEDULER unless GSAC_RETRY_BASE_MS selects the immediate one. */ scheduler?: Scheduler; /** Passed to octokit verbatim; octokit-core's own agent string when omitted. */ userAgent?: string; diff --git a/src/problem.ts b/src/problem.ts index 94814869a..488664b95 100644 --- a/src/problem.ts +++ b/src/problem.ts @@ -137,6 +137,11 @@ export type Problem = readonly unknown: readonly string[]; readonly known: readonly string[]; } + | { + readonly code: "settings-unknown-directives"; + readonly source: string; + readonly unknown: readonly string[]; + } | { readonly code: "settings-malformed-sections"; readonly source: string; @@ -243,6 +248,7 @@ export type SettingsProblem = ProblemOf< | "settings-not-mapping" | "settings-not-plain-mapping" | "settings-unknown-sections" + | "settings-unknown-directives" | "settings-malformed-sections" >; @@ -250,6 +256,15 @@ const PAT_ADVICE = "Discovery needs a user PAT; the workflow GITHUB_TOKEN and GitHub App installation tokens " + 'cannot enumerate a user\'s repositories. List the target repositories explicitly in the "repos" input'; +/** + * The underscore rule at the document level; the wrapper's line (src/sections/shared/schema-helpers.ts) says the + * same in the wrapper's terms. The two directives are all the underscore ever means. + */ +const DIRECTIVES_ADVICE = + "The underscore marks this action's directives, \"_layering\" (a file's top level or a list section's {entries} " + + 'wrapper) and "_undeclared" (a wrapper), and nothing else; there are no private-note keys. Remove the key, or ' + + "keep the note as a YAML comment"; + const PASSTHROUGH_ADVICE = "Fix these values in the settings file (only the named keys are validated; extra fields pass " + "through, except in closed sections and strict nested objects like actions.cache, which reject " + @@ -459,7 +474,9 @@ export function describeProblem(problem: Problem): string { case "settings-not-plain-mapping": return `${problem.source} must be a plain YAML mapping of section names to settings, but its top level parsed as another type (a YAML-tagged value like !!timestamp parses to a Date). Rewrite the top level as "section: ..." keys`; case "settings-unknown-sections": - return `unknown top-level section(s) in ${problem.source}: ${problem.unknown.join(", ")} (known: ${problem.known.join(", ")}). Fix the typo, or prefix private keys with "_", or set the "sections" input to limit processing`; + return `unknown top-level section(s) in ${problem.source}: ${problem.unknown.join(", ")} (known: ${problem.known.join(", ")}). Fix the typo, or set the "sections" input to limit processing`; + case "settings-unknown-directives": + return `unknown underscore key(s) in ${problem.source}: ${problem.unknown.join(", ")}. ${DIRECTIVES_ADVICE}`; case "settings-malformed-sections": return `${problem.source} has malformed section entries: ${problem.issues.join("; ")}. ${PASSTHROUGH_ADVICE}`; case "yaml-invalid": diff --git a/src/schema.ts b/src/schema.ts index f3544c0c8..802ee013d 100644 --- a/src/schema.ts +++ b/src/schema.ts @@ -37,7 +37,7 @@ import { RepositoryConfig } from "./sections/repository/schema.js"; import { RulesetConfig } from "./sections/rulesets/schema.js"; import { SecretScanningPatternConfig } from "./sections/secret_scanning_custom_patterns/schema.js"; import { knobbed, LayeringSchema } from "./sections/shared/schema-helpers.js"; -import { TeamsConfig } from "./sections/teams/schema.js"; +import { TeamConfig } from "./sections/teams/schema.js"; import { WebhookConfig } from "./sections/webhooks/schema.js"; import { WorkflowsConfig } from "./sections/workflows/schema.js"; import type { MustBeNever } from "./types.js"; @@ -63,7 +63,7 @@ export const SettingsFile = z code_scanning_default_setup: CodeScanningDefaultSetupConfig.optional(), code_quality_setup: CodeQualitySetupConfig.optional(), collaborators: knobbed(CollaboratorConfig).optional(), - teams: TeamsConfig.optional(), + teams: knobbed(TeamConfig).optional(), milestones: knobbed(MilestoneConfig).optional(), interaction_limits: InteractionLimitsConfig.optional(), actions_variables: knobbed(ActionsVariableConfig).optional(), @@ -125,6 +125,7 @@ export const UNDECLARED_POLICY_SECTIONS = [ "codespaces_secrets", "agents_secrets", "collaborators", + "teams", "milestones", "actions_variables", "agents_variables", @@ -198,7 +199,7 @@ type SliceDerivation = { code_scanning_default_setup: typeof CodeScanningDefaultSetupConfig; code_quality_setup: typeof CodeQualitySetupConfig; collaborators: ReturnType>; - teams: typeof TeamsConfig; + teams: ReturnType>; milestones: ReturnType>; interaction_limits: typeof InteractionLimitsConfig; actions_variables: ReturnType>; diff --git a/src/sections/actions_secrets/scenarios/actions_secrets-snapshot-roundtrip.yml b/src/sections/actions_secrets/scenarios/actions-secrets-snapshot-roundtrip.yml similarity index 96% rename from src/sections/actions_secrets/scenarios/actions_secrets-snapshot-roundtrip.yml rename to src/sections/actions_secrets/scenarios/actions-secrets-snapshot-roundtrip.yml index ec6218420..a49a423f9 100644 --- a/src/sections/actions_secrets/scenarios/actions_secrets-snapshot-roundtrip.yml +++ b/src/sections/actions_secrets/scenarios/actions-secrets-snapshot-roundtrip.yml @@ -1,4 +1,4 @@ -name: actions_secrets-snapshot-roundtrip +name: actions-secrets-snapshot-roundtrip description: "mode: snapshot reads the live actions_secrets back into snapshot.yml and writes nothing to GitHub. The actions_secrets names read back as per-store $SECRET_ACTIONS_ references, one note each in the file header and as a notice; the values, which GitHub never reveals, come from the environment when the snapshot is checked back. The written file, fed back to a check run against the same state, reads clean." settings: {} live_state: diff --git a/src/sections/actions_variables/scenarios/actions_variables-snapshot-roundtrip.yml b/src/sections/actions_variables/scenarios/actions-variables-snapshot-roundtrip.yml similarity index 95% rename from src/sections/actions_variables/scenarios/actions_variables-snapshot-roundtrip.yml rename to src/sections/actions_variables/scenarios/actions-variables-snapshot-roundtrip.yml index b31d44ae0..0b0fca4e4 100644 --- a/src/sections/actions_variables/scenarios/actions_variables-snapshot-roundtrip.yml +++ b/src/sections/actions_variables/scenarios/actions-variables-snapshot-roundtrip.yml @@ -1,4 +1,4 @@ -name: actions_variables-snapshot-roundtrip +name: actions-variables-snapshot-roundtrip description: "mode: snapshot reads the live actions_variables back into snapshot.yml and writes nothing to GitHub. The actions_variables read back as name and value, the timestamps dropped. The written file, fed back to a check run against the same state, reads clean." settings: {} live_state: diff --git a/src/sections/agents_secrets/scenarios/agents_secrets-snapshot-roundtrip.yml b/src/sections/agents_secrets/scenarios/agents-secrets-snapshot-roundtrip.yml similarity index 96% rename from src/sections/agents_secrets/scenarios/agents_secrets-snapshot-roundtrip.yml rename to src/sections/agents_secrets/scenarios/agents-secrets-snapshot-roundtrip.yml index 3235032bc..97f5f765d 100644 --- a/src/sections/agents_secrets/scenarios/agents_secrets-snapshot-roundtrip.yml +++ b/src/sections/agents_secrets/scenarios/agents-secrets-snapshot-roundtrip.yml @@ -1,4 +1,4 @@ -name: agents_secrets-snapshot-roundtrip +name: agents-secrets-snapshot-roundtrip description: "mode: snapshot reads the live agents_secrets back into snapshot.yml and writes nothing to GitHub. The agents_secrets names read back as per-store $SECRET_AGENTS_ references, one note each in the file header and as a notice; the values, which GitHub never reveals, come from the environment when the snapshot is checked back. The written file, fed back to a check run against the same state, reads clean." settings: {} live_state: diff --git a/src/sections/agents_variables/scenarios/agents_variables-snapshot-roundtrip.yml b/src/sections/agents_variables/scenarios/agents-variables-snapshot-roundtrip.yml similarity index 95% rename from src/sections/agents_variables/scenarios/agents_variables-snapshot-roundtrip.yml rename to src/sections/agents_variables/scenarios/agents-variables-snapshot-roundtrip.yml index db252d32f..eec2c5cda 100644 --- a/src/sections/agents_variables/scenarios/agents_variables-snapshot-roundtrip.yml +++ b/src/sections/agents_variables/scenarios/agents-variables-snapshot-roundtrip.yml @@ -1,4 +1,4 @@ -name: agents_variables-snapshot-roundtrip +name: agents-variables-snapshot-roundtrip description: "mode: snapshot reads the live agents_variables back into snapshot.yml and writes nothing to GitHub. The agents_variables read back as name and value, the timestamps dropped. The written file, fed back to a check run against the same state, reads clean." settings: {} live_state: diff --git a/src/sections/code_quality_setup/scenarios/code_quality_setup-snapshot-roundtrip.yml b/src/sections/code_quality_setup/scenarios/code-quality-setup-snapshot-roundtrip.yml similarity index 94% rename from src/sections/code_quality_setup/scenarios/code_quality_setup-snapshot-roundtrip.yml rename to src/sections/code_quality_setup/scenarios/code-quality-setup-snapshot-roundtrip.yml index ba5ec1c92..b75080db4 100644 --- a/src/sections/code_quality_setup/scenarios/code_quality_setup-snapshot-roundtrip.yml +++ b/src/sections/code_quality_setup/scenarios/code-quality-setup-snapshot-roundtrip.yml @@ -1,4 +1,4 @@ -name: code_quality_setup-snapshot-roundtrip +name: code-quality-setup-snapshot-roundtrip description: "mode: snapshot reads the live code_quality_setup back into snapshot.yml and writes nothing to GitHub. The not-configured default reads back with the null runner_type omitted (the slice takes no null there) and the nullable runner_label kept. The written file, fed back to a check run against the same state, reads clean." settings: {} live_state: {} diff --git a/src/sections/code_scanning_default_setup/scenarios/code_scanning_default_setup-snapshot-roundtrip.yml b/src/sections/code_scanning_default_setup/scenarios/code-scanning-default-setup-snapshot-roundtrip.yml similarity index 95% rename from src/sections/code_scanning_default_setup/scenarios/code_scanning_default_setup-snapshot-roundtrip.yml rename to src/sections/code_scanning_default_setup/scenarios/code-scanning-default-setup-snapshot-roundtrip.yml index cdc714863..01d0525c8 100644 --- a/src/sections/code_scanning_default_setup/scenarios/code_scanning_default_setup-snapshot-roundtrip.yml +++ b/src/sections/code_scanning_default_setup/scenarios/code-scanning-default-setup-snapshot-roundtrip.yml @@ -1,4 +1,4 @@ -name: code_scanning_default_setup-snapshot-roundtrip +name: code-scanning-default-setup-snapshot-roundtrip description: "mode: snapshot reads the live code_scanning_default_setup back into snapshot.yml and writes nothing to GitHub. A configured default setup reads back as its declarable fields; updated_at and the schedule fall away. The written file, fed back to a check run against the same state, reads clean." settings: {} live_state: diff --git a/src/sections/codespaces_secrets/scenarios/codespaces_secrets-snapshot-roundtrip.yml b/src/sections/codespaces_secrets/scenarios/codespaces-secrets-snapshot-roundtrip.yml similarity index 96% rename from src/sections/codespaces_secrets/scenarios/codespaces_secrets-snapshot-roundtrip.yml rename to src/sections/codespaces_secrets/scenarios/codespaces-secrets-snapshot-roundtrip.yml index 83953ce6e..d290aaf18 100644 --- a/src/sections/codespaces_secrets/scenarios/codespaces_secrets-snapshot-roundtrip.yml +++ b/src/sections/codespaces_secrets/scenarios/codespaces-secrets-snapshot-roundtrip.yml @@ -1,4 +1,4 @@ -name: codespaces_secrets-snapshot-roundtrip +name: codespaces-secrets-snapshot-roundtrip description: "mode: snapshot reads the live codespaces_secrets back into snapshot.yml and writes nothing to GitHub. The codespaces_secrets names read back as per-store $SECRET_CODESPACES_ references, one note each in the file header and as a notice; the values, which GitHub never reveals, come from the environment when the snapshot is checked back. The written file, fed back to a check run against the same state, reads clean." settings: {} live_state: diff --git a/src/sections/custom_properties/scenarios/custom-properties.yml b/src/sections/custom_properties/scenarios/custom-properties-apply-converges.yml similarity index 100% rename from src/sections/custom_properties/scenarios/custom-properties.yml rename to src/sections/custom_properties/scenarios/custom-properties-apply-converges.yml diff --git a/src/sections/custom_properties/scenarios/custom_properties-snapshot-roundtrip.yml b/src/sections/custom_properties/scenarios/custom-properties-snapshot-roundtrip.yml similarity index 95% rename from src/sections/custom_properties/scenarios/custom_properties-snapshot-roundtrip.yml rename to src/sections/custom_properties/scenarios/custom-properties-snapshot-roundtrip.yml index 3320104a7..3e73d4a6e 100644 --- a/src/sections/custom_properties/scenarios/custom_properties-snapshot-roundtrip.yml +++ b/src/sections/custom_properties/scenarios/custom-properties-snapshot-roundtrip.yml @@ -1,4 +1,4 @@ -name: custom_properties-snapshot-roundtrip +name: custom-properties-snapshot-roundtrip description: "mode: snapshot reads the live custom_properties back into snapshot.yml and writes nothing to GitHub. Set property values read back; a property with a null value is unset and omitted. The written file, fed back to a check run against the same state, reads clean." settings: {} live_state: diff --git a/src/sections/dependabot_secrets/scenarios/dependabot_secrets-snapshot-roundtrip.yml b/src/sections/dependabot_secrets/scenarios/dependabot-secrets-snapshot-roundtrip.yml similarity index 96% rename from src/sections/dependabot_secrets/scenarios/dependabot_secrets-snapshot-roundtrip.yml rename to src/sections/dependabot_secrets/scenarios/dependabot-secrets-snapshot-roundtrip.yml index 2de088cfc..3697543fc 100644 --- a/src/sections/dependabot_secrets/scenarios/dependabot_secrets-snapshot-roundtrip.yml +++ b/src/sections/dependabot_secrets/scenarios/dependabot-secrets-snapshot-roundtrip.yml @@ -1,4 +1,4 @@ -name: dependabot_secrets-snapshot-roundtrip +name: dependabot-secrets-snapshot-roundtrip description: "mode: snapshot reads the live dependabot_secrets back into snapshot.yml and writes nothing to GitHub. The dependabot_secrets names read back as per-store $SECRET_DEPENDABOT_ references, one note each in the file header and as a notice; the values, which GitHub never reveals, come from the environment when the snapshot is checked back. The written file, fed back to a check run against the same state, reads clean." settings: {} live_state: diff --git a/src/sections/deploy_keys/scenarios/deploy_keys-snapshot-roundtrip.yml b/src/sections/deploy_keys/scenarios/deploy-keys-snapshot-roundtrip.yml similarity index 96% rename from src/sections/deploy_keys/scenarios/deploy_keys-snapshot-roundtrip.yml rename to src/sections/deploy_keys/scenarios/deploy-keys-snapshot-roundtrip.yml index 7fa382a1d..e934f6bd5 100644 --- a/src/sections/deploy_keys/scenarios/deploy_keys-snapshot-roundtrip.yml +++ b/src/sections/deploy_keys/scenarios/deploy-keys-snapshot-roundtrip.yml @@ -1,4 +1,4 @@ -name: deploy_keys-snapshot-roundtrip +name: deploy-keys-snapshot-roundtrip description: "mode: snapshot reads the live deploy_keys back into snapshot.yml and writes nothing to GitHub. Live deploy keys read back by title, key (the comment GitHub drops is dropped here too), and read_only flag. The written file, fed back to a check run against the same state, reads clean." settings: {} live_state: diff --git a/src/sections/interaction_limits/scenarios/interaction_limits-snapshot-roundtrip.yml b/src/sections/interaction_limits/scenarios/interaction-limits-snapshot-roundtrip.yml similarity index 96% rename from src/sections/interaction_limits/scenarios/interaction_limits-snapshot-roundtrip.yml rename to src/sections/interaction_limits/scenarios/interaction-limits-snapshot-roundtrip.yml index ea83affab..d8151aadd 100644 --- a/src/sections/interaction_limits/scenarios/interaction_limits-snapshot-roundtrip.yml +++ b/src/sections/interaction_limits/scenarios/interaction-limits-snapshot-roundtrip.yml @@ -1,4 +1,4 @@ -name: interaction_limits-snapshot-roundtrip +name: interaction-limits-snapshot-roundtrip description: "mode: snapshot reads the live interaction_limits back into snapshot.yml and writes nothing to GitHub. The live limit, the pull-request creation cap, and its bypass list read back; the expiry cannot (GitHub reports only the computed expires_at), which the header notes. The written file, fed back to a check run against the same state, reads clean." settings: {} live_state: diff --git a/src/sections/labels/scenarios/labels-retry-429-then-200.yml b/src/sections/labels/scenarios/labels-retry-429-then-200.yml index 91cab0889..09e942a90 100644 --- a/src/sections/labels/scenarios/labels-retry-429-then-200.yml +++ b/src/sections/labels/scenarios/labels-retry-429-then-200.yml @@ -1,7 +1,7 @@ # A 429_then_200 fault answers the real secondary-rate-limit wire shape (the # "secondary rate limit" message body plus a small positive Retry-After) on the # labels list read once; the throttling plugin honors the Retry-After (in -# milliseconds under RETRY_BASE_MS=1, which the runner sets) and the retry +# milliseconds under GSAC_RETRY_BASE_MS=1, which the runner sets) and the retry # succeeds, so the section proceeds and applies normally - proving the client # recovers from a transient 429. The same plugin's recognition of this exact # shape is pinned at the unit level in server.test.ts. diff --git a/src/sections/secret_scanning_custom_patterns/scenarios/secret_scanning_custom_patterns-snapshot-roundtrip.yml b/src/sections/secret_scanning_custom_patterns/scenarios/secret-scanning-custom-patterns-snapshot-roundtrip.yml similarity index 95% rename from src/sections/secret_scanning_custom_patterns/scenarios/secret_scanning_custom_patterns-snapshot-roundtrip.yml rename to src/sections/secret_scanning_custom_patterns/scenarios/secret-scanning-custom-patterns-snapshot-roundtrip.yml index 990ee9ccd..7e3fc6200 100644 --- a/src/sections/secret_scanning_custom_patterns/scenarios/secret_scanning_custom_patterns-snapshot-roundtrip.yml +++ b/src/sections/secret_scanning_custom_patterns/scenarios/secret-scanning-custom-patterns-snapshot-roundtrip.yml @@ -1,4 +1,4 @@ -name: secret_scanning_custom_patterns-snapshot-roundtrip +name: secret-scanning-custom-patterns-snapshot-roundtrip description: "mode: snapshot reads the live secret_scanning_custom_patterns back into snapshot.yml and writes nothing to GitHub. A published custom pattern reads back as its declarable fields; the server-assigned id, slug, version, and state fall away. The written file, fed back to a check run against the same state, reads clean." settings: {} live_state: diff --git a/src/sections/shared/schema-helpers.ts b/src/sections/shared/schema-helpers.ts index 14b651e75..fb3273047 100644 --- a/src/sections/shared/schema-helpers.ts +++ b/src/sections/shared/schema-helpers.ts @@ -15,13 +15,38 @@ const UndeclaredPolicySchema = z.enum(["keep", "delete"]).meta({ id: "Undeclared */ export const LayeringSchema = z.enum(["merge", "replace"]); -const wrapperKeyError = renamedKeyError( +const renamedPolicyKeyError = renamedKeyError( "wrapper's policy", "undeclared", "_undeclared", "in v3 (a directive, like _layering) - write _undeclared: keep or _undeclared: delete", ); +/** + * The wrapper's unrecognized keys, one clause per kind, joined: the pre-v3 policy spelling names its rename, and + * any other underscore key names the two directives, since a wrapper takes no private notes either (the document + * level says the same in src/problem.ts). A misspelled entry field beside them stays on zod's own line, so the + * directives clause names the underscore keys it is about whenever the list holds anything else. + */ +function wrapperKeyError(issue: z.core.$ZodRawIssue): string | undefined { + if (issue.code !== "unrecognized_keys") { + return undefined; + } + const renamed = renamedPolicyKeyError(issue); + const directives = issue.keys.filter((key) => key.startsWith("_")); + if (directives.length === 0) { + return renamed; + } + const quoted = (keys: readonly string[]) => keys.map((key) => JSON.stringify(key)).join(", "); + const clause = + `${directives.length < issue.keys.length ? `${quoted(directives)}: ` : ""}the wrapper's directives are ` + + '"_undeclared" and, on a top-level section, "_layering", and nothing else - there are no ' + + "private-note keys. Remove the key, or keep the note as a YAML comment"; + return renamed === undefined + ? `Unrecognized key${issue.keys.length === 1 ? "" : "s"}: ${quoted(issue.keys)}; ${clause}` + : `${renamed}; ${clause}`; +} + /** * loosen() (../contract/module.ts) recognizes this union and rewraps it with the routed check that keeps * per-entry issue paths. The wrapper's definition name derives from the entry's own .meta({id}), so the diff --git a/src/sections/teams/generators.ts b/src/sections/teams/generators.ts index 94e9bdd37..b4ec65bdd 100644 --- a/src/sections/teams/generators.ts +++ b/src/sections/teams/generators.ts @@ -5,9 +5,9 @@ import { generatorFromSlice, type Json, uniqueBy } from "../../../test/e2e/gen-support.js"; import type { Rng } from "../../../test/e2e/prng.js"; -import { TeamsConfig } from "./schema.js"; +import { TeamConfig } from "./schema.js"; -const genTeam = generatorFromSlice(TeamsConfig.element, { +const genTeam = generatorFromSlice(TeamConfig, { fields: { name: (rng) => rng.pick(["core", "reviewers", "ops"]), permission: (rng) => rng.pick(["pull", "push", "maintain", "admin"]), diff --git a/src/sections/teams/index.ts b/src/sections/teams/index.ts index 962b26a23..4cf32d634 100644 --- a/src/sections/teams/index.ts +++ b/src/sections/teams/index.ts @@ -1,20 +1,30 @@ -/** `teams:` section: team repository access. Organization repos only, so a personal account no-ops with a note. */ +/** + * `teams:` section: team repository access. Organization repos only, so a personal account no-ops with a note. An + * undeclared team keeps its access by default (a team is often granted by the org, for reasons outside one + * repository's file); `_undeclared: delete` revokes the direct grants the file does not name. + */ import { z } from "zod"; import type { EndpointDecl } from "../contract/endpoints.js"; import { parseLive } from "../contract/live.js"; import { + defaultUndeclaredPolicy, loosen, type SectionMeta, type SectionModule, sectionGrant, + undeclaredDrift, + undeclaredNote, + undeclaredPolicy, valueDrift, } from "../contract/module.js"; import type { SectionPermission } from "../contract/permissions.js"; import type { PlanContext, PlannedOp, SectionPlan } from "../contract/plan.js"; import { rejectDuplicates } from "../contract/requests.js"; import { DEFAULT_ROLE, permissionForRole, roleForPermission } from "../shared/roles.js"; -import { type TeamConfig, TeamsConfig } from "./schema.js"; +import { knobbed } from "../shared/schema-helpers.js"; +import { knobbedSnapshot } from "../shared/snapshot-helpers.js"; +import { TeamConfig } from "./schema.js"; const permission: SectionPermission = { repo: ["administration"], org: "members" }; @@ -24,11 +34,14 @@ const ENDPOINTS = { route: "GET /orgs/{org}", statuses: { 200: "the organization", 404: "not an organization (a personal account)" }, permission: "none", - primaryRead: { notFound: "absent" }, }, + // GitHub gates the repository's team list at repository Administration (read) alone: the first read a fine-grained + // token can be denied, on a repository the org probe just proved exists, so its 404 is a denial. list: { route: "GET /repos/{owner}/{repo}/teams", statuses: { 200: "the teams with access to the repository" }, + permission: { repo: ["administration"] }, + primaryRead: { notFound: "denied" }, }, probe: { route: "GET /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo}", @@ -38,6 +51,10 @@ const ENDPOINTS = { route: "PUT /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo}", statuses: { 204: "team access granted" }, }, + revoke: { + route: "DELETE /orgs/{org}/teams/{team_slug}/repos/{owner}/{repo}", + statuses: { 204: "team access revoked" }, + }, } as const satisfies Record; type TeamsContext = PlanContext; @@ -51,6 +68,19 @@ const LiveTeamRepo = z.looseObject({ role_name: z.string().optional() }).nullish * (present only in a repository listing; absent reads as direct). */ const LiveTeam = z.looseObject({ slug: z.string(), access_source: z.string().optional() }); +type LiveTeam = z.infer; + +/** Access granted above the repository (the organization, the enterprise), which no repository call revokes. */ +function inheritedAccess(team: LiveTeam): string | undefined { + return team.access_source !== undefined && team.access_source !== "direct" + ? team.access_source + : undefined; +} + +/** The note plan() and snapshot() open with for such access; `tail` says what each does about it. */ +function inheritedAccessNote(slug: string, repo: string, source: string, tail: string): string { + return `teams[${slug}]: access to ${repo} is granted at the ${source} level, not on the repository${tail}`; +} /** The probe plan() and snapshot() share, under the media type LiveTeamRepo describes. */ async function probeTeamRole( @@ -75,19 +105,20 @@ function personalAccountNote(owner: string): string { export const teamsSection = { key: "teams", - undeclaredDefault: "untouched", + undeclaredDefault: "keep", permission, // Teams exist only under an organization owner; the org probe below implements the no-op this declares. ownerSensitivity: "org", endpoints: ENDPOINTS, - shape: loosen(TeamsConfig), + shape: loosen(knobbed(TeamConfig)), // The grant PUT accepts exactly one setting ("permission"), so an extra key is always a typo. closedSurface: { known: { name: true, permission: true }, describe: (t) => t.name, consequence: `a misspelled "permission" key would silently grant the default "${DEFAULT_ROLE}" role instead of the intended one`, }, - async plan(ctx, desired) { + async plan(ctx, declared) { + const { policy, entries: desired } = undeclaredPolicy(declared, defaultUndeclaredPolicy(this)); rejectDuplicates( this, desired, @@ -103,6 +134,10 @@ export const teamsSection = { ); return plan; } + // 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. + const live = parseLive(this, ENDPOINTS.list, z.array(LiveTeam), await ctx.read.list.listAll()); + const declaredSlugs = new Set(desired.map((team) => team.name.toLowerCase())); for (const team of desired) { const role = team.permission ?? DEFAULT_ROLE; const params = { org: ctx.repo.owner, team_slug: team.name }; @@ -132,14 +167,59 @@ export const teamsSection = { change: `granted team "${team.name}" ${role}`, }); } + + for (const team of live) { + if (declaredSlugs.has(team.slug.toLowerCase())) { + continue; + } + const inherited = inheritedAccess(team); + if (inherited !== undefined) { + // Only a revocation would act on it, so only the policy that would revoke is told it cannot. + if (policy === "delete") { + plan.notes.push( + inheritedAccessNote( + team.slug, + ctx.repo.slug, + inherited, + ', so "_undeclared: delete" cannot revoke it; left untouched', + ), + ); + } + continue; + } + if (policy === "keep") { + plan.notes.push( + undeclaredNote({ + subject: `team "${team.slug}"`, + state: "has access but is not declared", + manage: "its access", + action: "REVOKE its access", + }), + ); + continue; + } + plan.ops.push({ + role: "revoke", + params: { org: ctx.repo.owner, team_slug: team.slug }, + drift: [ + undeclaredDrift(defaultUndeclaredPolicy(this), { + label: `teams[${team.slug}]`, + action: "REVOKE its access", + keep: "its access", + }), + ], + change: `REVOKED undeclared team "${team.slug}"`, + }); + } return plan; }, /** * The role comes from the probe, not the listing's `permission`: * the listing reports a custom role as its base role, role_name names it. - * Omitted with a note, each a no-op since the section never removes an undeclared team: - * non-direct access (declaring it would grant direct access), a probe 404 (no access, or a - * concealed denial), an unreadable role, a role no declaration plans as. + * Omitted with a note, each a no-op under the keep default: non-direct access (declaring it + * would grant direct access; plan() never revokes it either), a probe 404 (no access, or a + * concealed denial), an unreadable role, a role no declaration plans as (a direct team plan() + * still revokes under `_undeclared: delete`, so the note names what the file would have to declare). */ async snapshot(ctx) { const orgProbe = await ctx.read.org.probeAbsent({ params: { org: ctx.repo.owner } }); @@ -151,9 +231,15 @@ export const teamsSection = { const entries: TeamConfig[] = []; for (const team of teams) { const label = `teams[${team.slug}]`; - if (team.access_source !== undefined && team.access_source !== "direct") { + const inherited = inheritedAccess(team); + if (inherited !== undefined) { notes.push( - `${label}: access to ${ctx.repo.slug} is granted at the ${team.access_source} level, not on the repository; not declared, since declaring it would grant direct access`, + inheritedAccessNote( + team.slug, + ctx.repo.slug, + inherited, + "; not declared, since declaring it would grant direct access", + ), ); continue; } @@ -182,6 +268,6 @@ export const teamsSection = { } entries.push({ name: team.slug, permission }); } - return { value: entries.length === 0 ? undefined : entries, notes }; + return { value: entries.length === 0 ? undefined : knobbedSnapshot(this, entries), notes }; }, } satisfies SectionModule<"teams", typeof ENDPOINTS>; diff --git a/src/sections/teams/mock.test.ts b/src/sections/teams/mock.test.ts index ac31c8fdc..9be5dacc6 100644 --- a/src/sections/teams/mock.test.ts +++ b/src/sections/teams/mock.test.ts @@ -46,7 +46,7 @@ describe("teams.probe answers by Accept media type, like GitHub", () => { drift: [], }); expect(await teamsSection.snapshot(snapshotContext(teamsSection, api, REPO, "fail"))).toEqual({ - value: [{ name: "platform", permission: "push" }], + value: { _undeclared: "keep", entries: [{ name: "platform", permission: "push" }] }, notes: [], }); expect(api.writes).toEqual([]); diff --git a/src/sections/teams/mock.ts b/src/sections/teams/mock.ts index ce448e5a5..54ac56ece 100644 --- a/src/sections/teams/mock.ts +++ b/src/sections/teams/mock.ts @@ -92,4 +92,8 @@ export const teamsMockHandlers: SectionRestHandlers<"teams"> = { state.teams[slug] = teamRepoFromPut(asObject(body)); return noContent(); }, + "teams.revoke": ({ state, param }) => { + state.teams[param("team_slug")] = null; + return noContent(); + }, }; diff --git a/src/sections/teams/scenarios/teams-snapshot-roundtrip.yml b/src/sections/teams/scenarios/teams-snapshot-roundtrip.yml index 605505087..2940c51de 100644 --- a/src/sections/teams/scenarios/teams-snapshot-roundtrip.yml +++ b/src/sections/teams/scenarios/teams-snapshot-roundtrip.yml @@ -23,12 +23,14 @@ expect: teams: snapshot snapshot: teams: - - name: platform - permission: push - - name: auditors - permission: security-auditor - - name: readers - permission: pull + _undeclared: keep + entries: + - name: platform + permission: push + - name: auditors + permission: security-auditor + - name: readers + permission: pull summary_contains: - "teams[pushy]: the live role \"push\" has no declaration that plans as itself (\"push\" in a settings file means the \"write\" role); not declared" never: diff --git a/src/sections/teams/scenarios/teams-undeclared-delete.yml b/src/sections/teams/scenarios/teams-undeclared-delete.yml new file mode 100644 index 000000000..34f0eaaa9 --- /dev/null +++ b/src/sections/teams/scenarios/teams-undeclared-delete.yml @@ -0,0 +1,32 @@ +name: teams-undeclared-delete +description: >- + Under "_undeclared: delete" an organization repo's team the settings file does + not name loses its direct access: apply DELETEs that team's repository grant, + leaves the declared team's converged access alone, and a check rerun over the + mutated state converges. +settings: + teams: + _undeclared: delete + entries: + - name: platform + permission: push +owner_kind: org +live_state: + teams: + platform: + role_name: write + legacy: + role_name: read +expect: + exit_code: 0 + result: applied + outcomes: + teams: applied + mutations: + - "DELETE /orgs/e2e-owner/teams/legacy/repos/{repo}" + never: + - "PUT /orgs/e2e-owner/teams/platform/repos/{repo}" + - "DELETE /orgs/e2e-owner/teams/platform/repos/{repo}" + stdout_contains: + - "REVOKED undeclared team \"legacy\"" + fixpoint: converges diff --git a/src/sections/teams/scenarios/teams-undeclared-keep-note.yml b/src/sections/teams/scenarios/teams-undeclared-keep-note.yml new file mode 100644 index 000000000..c211c0d97 --- /dev/null +++ b/src/sections/teams/scenarios/teams-undeclared-keep-note.yml @@ -0,0 +1,29 @@ +name: teams-undeclared-keep-note +description: >- + The section's keep default: a team with direct access the settings file does + not name is KEPT and surfaced as a note naming the "_undeclared: delete" knob + that would revoke it. Apply grants nothing (the declared team is converged), + stays green, and a check rerun converges, since the kept team is a note, not + drift. +settings: + teams: + - name: platform + permission: push +owner_kind: org +live_state: + teams: + platform: + role_name: write + legacy: + role_name: read +expect: + exit_code: 0 + result: applied + outcomes: + teams: applied + never: + - "DELETE /orgs/e2e-owner/teams/" + - "PUT /orgs/e2e-owner/teams/" + stdout_contains: + - "team \"legacy\" has access but is not declared in the settings file; kept under \"_undeclared: keep\"" + fixpoint: converges diff --git a/src/sections/teams/schema.ts b/src/sections/teams/schema.ts index 6c744bdda..815896ed2 100644 --- a/src/sections/teams/schema.ts +++ b/src/sections/teams/schema.ts @@ -2,12 +2,10 @@ import { z } from "zod"; -const TeamConfig = z +export const TeamConfig = z .object({ name: z.string(), permission: z.string().optional(), }) .meta({ id: "TeamConfig" }); export type TeamConfig = z.infer; - -export const TeamsConfig = z.array(TeamConfig); diff --git a/src/sections/teams/teams.docs.yml b/src/sections/teams/teams.docs.yml index d2a70f65d..4a0581fa9 100644 --- a/src/sections/teams/teams.docs.yml +++ b/src/sections/teams/teams.docs.yml @@ -1,22 +1,34 @@ sections_table: endpoints: org team repo permissions + repo team list - notes: org repos only, skipped with a notice on personal accounts + notes: >- + org repos only, skipped with a notice on personal accounts; undeclared teams kept by default + (`_undeclared: delete` revokes their direct access) coverage: - area: >- [Team repository permissions](https://docs.github.com/en/rest/teams/teams) (org repos) notes: >- - PUT /orgs/{org}/teams/{slug}/repos/{owner}/{repo}; probes GET /orgs/{owner} and no-ops with a - note on personal accounts (404 only; 403/5xx still fail). Both modes probe each 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. - Snapshot lists the teams with access (GET /repos/{owner}/{repo}/teams) and reads each team's - role through the same probe, so a custom role reads back by name; noted, not declared: access - granted at the organization or enterprise level, a probe 404 (no access, or a fine-grained - token missing the grant; the note names both), a role the probe does not report, and a role - no declaration plans as (a custom role named "push" or "pull"). A personal account snapshots - nothing. + PUT/DELETE /orgs/{org}/teams/{slug}/repos/{owner}/{repo}; 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. 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. + Snapshot lists the same teams and reads each team's role through the same probe, so a custom + role reads back by name, and writes the wrapped form with the keep default spelled out; + noted, not declared: access granted at the organization or enterprise level, a probe 404 (no + access, or a fine-grained token missing the grant; the note names both), a role the probe + does not report, and a role no declaration plans as (a custom role named "push" or "pull"). + A personal account snapshots nothing. schema: - SettingsFile.teams: Org team access to the repo; skipped on personal accounts. + SettingsFile.teams: >- + Org team access to the repo; skipped on personal accounts. Undeclared teams are kept by default + and noted; the wrapped form can set `_undeclared: delete` to revoke their direct access (a grant + made at the organization level is never touched). A declared child team can hold its access + through an undeclared parent's grant, which `delete` revokes: declare the parent too, or keep. TeamConfig: >- One org team's access to the repository, matched by team slug. Keys other than name and permission are rejected (a misspelled "permission" would otherwise silently grant the default diff --git a/src/sections/teams/teams.test.ts b/src/sections/teams/teams.test.ts index 9c3258014..38cb7b5f8 100644 --- a/src/sections/teams/teams.test.ts +++ b/src/sections/teams/teams.test.ts @@ -32,6 +32,7 @@ describe("teams", () => { test("plans a grant per team without access or at a divergent role, and nothing for a converged one", async () => { const api = new MockApi({ [ORG]: { data: { login: "o" } }, + [LIST]: { data: [] }, [probeOf("platform")]: { data: { role_name: "read" } }, [probeOf("ops")]: { data: { role_name: "write" } }, }); @@ -66,15 +67,74 @@ describe("teams", () => { }); expect(api.calls.map((c) => `${c.method} ${c.path}`)).toEqual([ ORG, + LIST, probeOf("platform"), probeOf("ops"), probeOf("new"), ]); }); + test("an undeclared team with direct access is kept with a note by default; access granted above the repository passes silently", async () => { + const api = new MockApi({ + [ORG]: { data: { login: "o" } }, + [LIST]: { + data: [ + { slug: "platform", access_source: "direct" }, + { slug: "legacy", access_source: "direct" }, + { slug: "everyone", access_source: "organization" }, + ], + }, + [probeOf("platform")]: { data: { role_name: "write" } }, + }); + const result = await plan(api, [{ name: "platform", permission: "push" }]); + expect(result).toEqual({ + ops: [], + notes: [ + 'team "legacy" has access but is not declared in the settings file; kept under "_undeclared: keep" - add it to the settings file to manage its access, or set "_undeclared: delete" to have apply REVOKE its access', + ], + drift: [], + }); + expect(api.calls.map((c) => `${c.method} ${c.path}`)).toEqual([ORG, LIST, probeOf("platform")]); + }); + + test('under "_undeclared: delete" access granted above the repository is noted as beyond the repository\'s reach', async () => { + const api = new MockApi({ + [ORG]: { data: { login: "o" } }, + [LIST]: { data: [{ slug: "everyone", access_source: "organization" }] }, + }); + const result = await plan(api, { _undeclared: "delete", entries: [] }); + expect(result).toEqual({ + ops: [], + notes: [ + 'teams[everyone]: access to o/r is granted at the organization level, not on the repository, so "_undeclared: delete" cannot revoke it; left untouched', + ], + drift: [], + }); + }); + + test('under "_undeclared: delete" an undeclared direct team is revoked, and the re-plan is empty', async () => { + const api = fragmentFake(teamsSection, teamsMockHandlers, { + teams: { platform: { role_name: "write" }, legacy: { role_name: "read" } }, + }); + const { first, second, changes } = await provePlanIdempotent(teamsSection, api, { + _undeclared: "delete", + entries: [{ name: "platform", permission: "push" }], + }); + expect(first.ops.map((op) => op.drift)).toEqual([ + [ + 'teams[legacy]: undeclared - not in the settings file and "_undeclared: delete" is set, so apply will REVOKE its access; add it to the settings file to keep its access', + ], + ]); + expect(changes).toEqual(['REVOKED undeclared team "legacy"']); + expect(api.writes).toEqual(["DELETE /orgs/o/teams/legacy/repos/o/r"]); + expect(second).toEqual({ ops: [], notes: [], drift: [] }); + expect(api.state.teams).toEqual({ platform: { role_name: "write" }, legacy: null }); + }); + test("a bare 204 probe body reads as access without a role, so the declared role is drift", async () => { const api = new MockApi({ [ORG]: { data: { login: "o" } }, + [LIST]: { data: [] }, [probeOf("platform")]: { data: null }, }); const result = await plan(api, [{ name: "platform", permission: "pull" }]); @@ -121,15 +181,18 @@ describe("teams", () => { }); }); - test("the read port exposes the org probe in its absent posture, the list, and the team probe, never the grant", () => { + test("the read port exposes the public org probe, the list in its denied posture, and the team probe, never a write", () => { const ctx = planContext(teamsSection, new MockApi({}), REPO); expect(Object.keys(ctx.read)).toEqual(["org", "list", "probe"]); // @ts-expect-error a write role is not a read: the port has no `grant` ctx.read.grant; - // @ts-expect-error an "absent" primary read offers no throwing helper - ctx.read.org.call; - // @ts-expect-error nor a list - ctx.read.org.listAll; + // @ts-expect-error nor a `revoke` + ctx.read.revoke; + // @ts-expect-error a "denied" primary read offers no absent probe: its 404 is a denial + ctx.read.list.probeAbsent; + // @ts-expect-error nor a tolerant call + ctx.read.list.tryCall; + expect(typeof ctx.read.org.probeAbsent).toBe("function"); expect(typeof ctx.read.probe.probeAbsent).toBe("function"); expect(typeof ctx.read.list.listAll).toBe("function"); }); @@ -157,11 +220,14 @@ describe("teams", () => { [probeOf("pushy")]: { data: { role_name: "push" } }, }); expect(await snapshot(api)).toEqual({ - value: [ - { name: "platform", permission: "push" }, - { name: "auditors", permission: "security-auditor" }, - { name: "legacy", permission: "pull" }, - ], + value: { + _undeclared: "keep", + entries: [ + { name: "platform", permission: "push" }, + { name: "auditors", permission: "security-auditor" }, + { name: "legacy", permission: "pull" }, + ], + }, notes: [ "teams[everyone]: access to o/r is granted at the organization level, not on the repository; not declared, since declaring it would grant direct access", "teams[gone]: listed with access to o/r, but the access probe answered 404, read here as no access; not declared. " + diff --git a/test/action/run.test.ts b/test/action/run.test.ts index d688389b2..8b8e3fdb8 100644 --- a/test/action/run.test.ts +++ b/test/action/run.test.ts @@ -748,7 +748,7 @@ describe("run in mode: merge", () => { expect(await run({ api: new MockApi({}), io: testIo })).toBe(1); expect(existsSync(mergedFile)).toBe(false); expect(captured).toEqual([ - `error: unknown top-level section(s) in ${top}: future (known: ${SECTION_KEYS.join(", ")}). Fix the typo, or prefix private keys with "_", or set the "sections" input to limit processing`, + `error: unknown top-level section(s) in ${top}: future (known: ${SECTION_KEYS.join(", ")}). Fix the typo, or set the "sections" input to limit processing`, "result: failed", ]); }), @@ -777,10 +777,10 @@ describe("run in mode: merge", () => { test("a cyclic layer (a YAML anchor that includes itself) is refused by the fold, naming the layer", () => withTempDir("merge-mode-", async (dir) => { - // A self-referencing anchor under a private key parses to a cyclic object; standalone validation ignores the key, so the engine's boundary is - // what refuses the layer. + // A self-referencing anchor under an open section parses to a cyclic object; the standalone validation passes the unknown nested key through, so the + // engine's boundary is what refuses the layer. const top = join(dir, "top.yml"); - writeFileSync(top, ["_notes: &loop", " self: *loop", "labels: []", ""].join("\n")); + writeFileSync(top, ["repository: &loop", " self: *loop", "labels: []", ""].join("\n")); const mergedFile = setMergeEnv(dir, [layer("fleet.yml"), top]); expect(await run({ api: new MockApi({}), io: testIo })).toBe(1); expect(existsSync(mergedFile)).toBe(false); diff --git a/test/e2e/foundation.test.ts b/test/e2e/foundation.test.ts index ec75b502d..46b1657f1 100644 --- a/test/e2e/foundation.test.ts +++ b/test/e2e/foundation.test.ts @@ -1,7 +1,8 @@ import { describe, expect, test } from "bun:test"; -import { chmodSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { chmodSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { basename, join } from "node:path"; +import { parse as parseYaml } from "yaml"; import { MARKER_LABEL, MARKER_LABEL_CONFIG } from "../../src/report/issue-report.js"; import { SECTION_KEYS } from "../../src/schema.js"; import { ROOT } from "../root.js"; @@ -323,6 +324,21 @@ describe("scenario schema", () => { }); describe("scenario corpus loader (collectYmlFiles)", () => { + test("every scenario file name is dashed lowercase and names its scenario, so a section key's underscore never leaks into the corpus and --scenario selects the file", () => { + const files = scenarioRoots().flatMap((root) => collectYmlFiles(root)); + expect(files.length).toBeGreaterThan(0); + const offenders = files.filter( + (path) => !/^[a-z0-9]+(?:-[a-z0-9]+)*\.yml$/.test(basename(path)), + ); + expect(offenders).toEqual([]); + const misnamed = files.filter( + (path) => + (parseYaml(readFileSync(path, "utf8")) as { name?: unknown }).name !== + basename(path, ".yml"), + ); + expect(misnamed).toEqual([]); + }); + function withTempRoot(body: (root: string) => void): void { const root = mkdtempSync(join(tmpdir(), "e2e-corpus-")); try { diff --git a/test/e2e/generators.ts b/test/e2e/generators.ts index 17f52452e..3e71f6736 100644 --- a/test/e2e/generators.ts +++ b/test/e2e/generators.ts @@ -311,7 +311,7 @@ export function genLiveWitness( * assert failures the contract does not promise, so they stay out. * offendingToken -> must appear in the rejection error: a section path ("labels[2].name"), an unknown key, or a wording fragment * stays out -> unknown nested keys under a loose shape, un-modeled enums, arbitrary types on loose keys, - * `pages: null`, underscore-prefixed keys + * `pages: null` */ export interface InvalidSettingsCase { doc: Json; @@ -400,7 +400,6 @@ export const INVALID_SETTINGS_CASES: ReadonlyArray<{ { name: "unknown-top-level-key", build: (rng) => { - // None underscore-prefixed: those are accepted as private keys by design. const typo = rng.pick(["labelz", "label", "milestone", "repositories", "branch"]); return { doc: { labels: genSettings(rng.fork("labels"), "labels") as Json, [typo]: [] }, @@ -408,6 +407,17 @@ export const INVALID_SETTINGS_CASES: ReadonlyArray<{ }; }, }, + { + name: "unknown-underscore-key", + build: (rng) => { + // The underscore is the two directives' and nothing else's: a note or a misspelled directive is rejected, never dropped. + const key = rng.pick(["_notes", "_owner", "_layerin", "_undeclared"]); + return { + doc: { labels: genSettings(rng.fork("labels"), "labels") as Json, [key]: "x" }, + offendingToken: key, + }; + }, + }, { name: "array-section-wrong-type", build: (rng) => { @@ -950,7 +960,7 @@ export interface MultiScenarioMeta { selfSlug: string; /** * The GLOBAL token mask (scenario token_permissions), varied only on org_members. The idempotence eligibility - * predicate reads it: a globally denied org gate makes a declared teams section a denied-path section even when + * predicate reads it: a globally denied org gate answers a declared teams section no access and denies its grants even when * every per-target mask is empty. */ globalMask: Partial>; @@ -1007,7 +1017,7 @@ export function genMultiScenario( const selfSlug = ADMIN_SLUG; // Varied ONLY on org_members: the mock grades org routes against the global mask and repo routes against the per-slug // overlay (mock/routes.ts), so any other global entry would have mock and oracle grading different masks. The - // idempotence force clears it: a globally denied teams section under fail policy would preflight-abort the fixpoint proof. + // idempotence force clears it: a globally denied org gate leaves a declared teams section granting on every apply, which is no fixpoint. const globalMaskRng = rng.fork("global-mask"); const globalMask: Partial> = {}; if (globalMaskRng.bool(0.3) && force !== "idempotence-eligible") { diff --git a/test/e2e/oracle.test.ts b/test/e2e/oracle.test.ts index a297e9617..deed20913 100644 --- a/test/e2e/oracle.test.ts +++ b/test/e2e/oracle.test.ts @@ -15,6 +15,7 @@ import { judgePreflightAbort, KEYED_MERGE_SECTIONS, NO_READ_SECTIONS, + orgGateDenied, type PreflightAbort, predictDiscovery, predictMerge, @@ -84,23 +85,72 @@ describe("sectionGrade", () => { ).toBe("read"); }); - test("teams: org_members is a read-gate, not a grade cap", () => { - expect(sectionGrade("teams", { administration: "write", org_members: "none" })).toBe("none"); - expect(sectionGrade("teams", { administration: "write", org_members: "read" })).toBe("write"); + test("teams: org_members shuts the org gate, never the repository grade", () => { + expect(sectionGrade("teams", { administration: "write", org_members: "none" })).toBe("write"); expect(sectionGrade("teams", { administration: "read", org_members: "write" })).toBe("read"); - expect(sectionGrade("teams", { administration: "write", org_members: "write" })).toBe("write"); + expect(sectionGrade("teams", { administration: "none", org_members: "write" })).toBe("none"); + expect(orgGateDenied("teams", { org_members: "none" })).toBe(true); + expect(orgGateDenied("teams", { org_members: "read" })).toBe(false); + expect(orgGateDenied("teams", {})).toBe(false); + // A section without an org permission has no org gate to shut. + expect(orgGateDenied("labels", { org_members: "none" })).toBe(false); }); test("teams: the org gate reads org_members from orgMask, not the per-slug mask", () => { - // Nightly seed 28401742: the mock takes org_members for teams' org-scoped grant endpoint from the - // GLOBAL mask, so a per-slug org_members:none must NOT gate teams when the global mask (empty, so write) grants it. - expect(sectionGrade("teams", { administration: "write", org_members: "none" }, {})).toBe( - "write", - ); - // The orgMask's own org_members:none does gate it, as on the single-repo path where orgMask === mask. - expect(sectionGrade("teams", { administration: "write" }, { org_members: "none" })).toBe( - "none", - ); + // Nightly seed 28401742: the mock takes org_members for teams' org-scoped endpoints from the GLOBAL mask, so a + // per-slug org_members:none must NOT shut the gate when the global mask (empty, so write) grants it. + const teams = meta({ + sections: ["teams"], + mask: { administration: "write", org_members: "none" }, + mode: "check", + }); + expect(predictSection("teams", { ...teams, orgMask: {} }).grades).toEqual(["write"]); + // The orgMask's own org_members:none does shut it, as on the single-repo path where orgMask === mask: the + // repository reads pass and the probes answer 404, so the section runs at none under the absent posture. + const shut = predictSection("teams", { + ...meta({ sections: ["teams"], mask: { administration: "write" }, mode: "check" }), + orgMask: { org_members: "none" }, + }); + expect(shut.grades).toEqual(["none"]); + expect([...shut.allowed].sort()).toEqual(["clean", "drift"]); + // With no repository grant the team list is denied first, under the section's own "denied" posture. + const noAdmin = predictSection("teams", { + ...meta({ sections: ["teams"], mask: { administration: "none" }, mode: "check" }), + orgMask: { org_members: "none" }, + }); + expect([...noAdmin.allowed]).toEqual(["failed"]); + }); + + test("teams: a shut org gate never arms the preflight barrier; a missing repository grant does", () => { + // The list read passes and the probes' 404 reads as no access, so preflight passes and the PUT is what fails: + // the section row is failed with preflightDenied empty, never a preflight abort. + const gateShut: ScenarioMeta = { + ...meta({ + sections: ["teams"], + mask: { administration: "write" }, + mode: "apply", + policy: "fail", + }), + orgMask: { org_members: "none" }, + }; + const shut = predictSection("teams", gateShut); + expect(shut.posture).toBe("absent"); + expect(preflightDeniable(shut, gateShut)).toBe("no"); + expect(predictOutcomes(gateShut).preflightAborts).toBe("no"); + // Under the 403 style the probe's denial is a plain denial the engine sees, so preflight does abort. + expect(preflightDeniable(shut, { ...gateShut, denialStyle: 403 })).toBe("yes"); + const noAdmin: ScenarioMeta = { + ...meta({ + sections: ["teams"], + mask: { administration: "none" }, + mode: "apply", + policy: "fail", + }), + orgMask: { org_members: "none" }, + }; + const denied = predictSection("teams", noAdmin); + expect(denied.posture).toBe("denied"); + expect(preflightDeniable(denied, noAdmin)).toBe("yes"); }); }); @@ -458,6 +508,7 @@ describe("predictSection rules", () => { key: "labels", grades: [], allowed: new Set(["excluded"]), + posture: "denied", mayWrite: false, }); const unrestricted = predictSection( @@ -468,6 +519,7 @@ describe("predictSection rules", () => { key: "labels", grades: ["none"], allowed: new Set(["failed"]), + posture: "denied", mayWrite: false, }); }); @@ -486,6 +538,7 @@ describe("predictSection rules", () => { key: "check_suite_preferences", grades: [], allowed: new Set(["excluded"]), + posture: "absent", mayWrite: false, }); }); @@ -501,8 +554,20 @@ describe("predictSection rules", () => { }); expect(predictOutcomes(excludedDenied)).toEqual({ sections: [ - { key: "labels", grades: [], allowed: new Set(["excluded"]), mayWrite: false }, - { key: "pages", grades: ["write"], allowed: new Set(["applied"]), mayWrite: true }, + { + key: "labels", + grades: [], + allowed: new Set(["excluded"]), + posture: "denied", + mayWrite: false, + }, + { + key: "pages", + grades: ["write"], + allowed: new Set(["applied"]), + posture: "absent", + mayWrite: true, + }, ], allowedExitCodes: new Set([0]), noWritesInCheck: false, @@ -514,8 +579,20 @@ describe("predictSection rules", () => { // The control: the same meta with labels ACTIVE reaches the denied read, and the barrier aborts. expect(predictOutcomes({ ...excludedDenied, onlySections: undefined })).toEqual({ sections: [ - { key: "labels", grades: ["none"], allowed: new Set(["failed"]), mayWrite: false }, - { key: "pages", grades: ["write"], allowed: new Set(["applied"]), mayWrite: true }, + { + key: "labels", + grades: ["none"], + allowed: new Set(["failed"]), + posture: "denied", + mayWrite: false, + }, + { + key: "pages", + grades: ["write"], + allowed: new Set(["applied"]), + posture: "absent", + mayWrite: true, + }, ], allowedExitCodes: new Set([1]), noWritesInCheck: false, @@ -557,6 +634,7 @@ describe("predictSection rules", () => { key: "teams", grades: ["write"], allowed: new Set(["applied"]), + posture: "denied", mayWrite: false, }); @@ -573,6 +651,7 @@ describe("predictSection rules", () => { key: "teams", grades: ["write"], allowed: new Set(["clean"]), + posture: "denied", mayWrite: false, }); }); @@ -592,6 +671,7 @@ describe("predictSection rules", () => { key: "teams", grades: [], allowed: new Set(["excluded"]), + posture: "denied", mayWrite: false, }); }); @@ -611,8 +691,20 @@ describe("predictOutcomes run level", () => { }); expect(predictOutcomes(personal)).toEqual({ sections: [ - { key: "teams", grades: ["write"], allowed: new Set(["applied"]), mayWrite: false }, - { key: "labels", grades: ["write"], allowed: new Set(["applied"]), mayWrite: true }, + { + key: "teams", + grades: ["write"], + allowed: new Set(["applied"]), + posture: "denied", + mayWrite: false, + }, + { + key: "labels", + grades: ["write"], + allowed: new Set(["applied"]), + posture: "denied", + mayWrite: true, + }, ], allowedExitCodes: new Set([0]), noWritesInCheck: false, @@ -625,8 +717,20 @@ describe("predictOutcomes run level", () => { // probe, and the barrier aborts the run. expect(predictOutcomes({ ...personal, ownerKind: "org" })).toEqual({ sections: [ - { key: "teams", grades: ["none"], allowed: new Set(["failed"]), mayWrite: false }, - { key: "labels", grades: ["write"], allowed: new Set(["applied"]), mayWrite: true }, + { + key: "teams", + grades: ["none"], + allowed: new Set(["failed"]), + posture: "absent", + mayWrite: false, + }, + { + key: "labels", + grades: ["write"], + allowed: new Set(["applied"]), + posture: "denied", + mayWrite: true, + }, ], allowedExitCodes: new Set([1]), noWritesInCheck: false, diff --git a/test/e2e/oracle.ts b/test/e2e/oracle.ts index d51d2b681..6f67a1cb0 100644 --- a/test/e2e/oracle.ts +++ b/test/e2e/oracle.ts @@ -78,11 +78,10 @@ function repoMaskKeys(permission: SectionPermission): MaskKey[] { return [...permission.repo]; } -/** The grade a mask GRANTS a section, before its read gating folds it (effectiveGrades). */ +/** The grade a mask GRANTS a section's repository reads and writes, before its read gating folds it (effectiveGrades). */ export function sectionGrade( key: SectionKey, mask: Partial>, - orgMask: Partial> = mask, ): MaskGrade { const permission = PERMISSION_BY_KEY[key]; let repoGrade: MaskGrade = "none"; @@ -92,16 +91,23 @@ export function sectionGrade( repoGrade = grade; } } - if (permission.org !== "members") { - return repoGrade; - } - // org_members gates only the organization PROBE (a read); team writes go through administration, - // so capping by org_members would wrongly downgrade an administration-write + members-read token. - // The mock takes org_members for teams' /orgs/ endpoints from the GLOBAL mask, not the per-slug - // overlay (the rest of the mask stays per slug), so the gate reads orgMask (equal to mask outside - // multi-repo mode). - const orgGrade = orgMask.org_members ?? "write"; - return orgGrade === "none" ? "none" : repoGrade; + return repoGrade; +} + +/** + * Whether the org gate denies a section's org-scoped requests: org_members gates teams' per-team access probes and + * its grants, never the repository's team list (GitHub grades that at repository Administration alone, and the + * section declares it so). A denied probe answers 404 under fine_grained, which the section reads as "no access", + * so the reads go through and only the writes are denied: the ABSENT posture at grade none, whatever the + * section's own posture says. The mock takes org_members for teams' /orgs/ endpoints from the GLOBAL mask, not + * the per-slug overlay (the rest of the mask stays per slug), so the gate reads orgMask (equal to mask outside + * multi-repo mode). + */ +export function orgGateDenied( + key: SectionKey, + orgMask: Partial>, +): boolean { + return PERMISSION_BY_KEY[key].org === "members" && (orgMask.org_members ?? "write") === "none"; } /** @@ -124,6 +130,12 @@ export interface SectionPrediction { * writeGranted) is vacuous for it and no consumer recognizes "excluded" by hand. */ grades: readonly MaskGrade[]; + /** + * What a fine-grained 404 on the first read denied at these grades means: the section's own + * posture, or "absent" when only the org gate is shut (the repository read passes and the + * org-gated probe's 404 reads as a missing grant). The preflight fold reads it. + */ + posture: DenialPosture; /** The outcomes the section is allowed to report; the runner must see one. */ allowed: Set; /** False folds the section into writeDeniedSections, whose writes the fuzz asserts never mutate state. */ @@ -151,7 +163,13 @@ export function predictSectionAt( meta.onlySections.length > 0 && !meta.onlySections.includes(key) ) { - return { key, grades: [], allowed: new Set(["excluded"]), mayWrite: false }; + return { + key, + grades: [], + posture: DENIAL_POSTURE[key], + allowed: new Set(["excluded"]), + mayWrite: false, + }; } // The org probe is declared permission "none", so no mask key gates it and its 404 is met before // any gated read; by sectionGrade's convention an ungated resource grades write. @@ -159,21 +177,30 @@ export function predictSectionAt( return { key, grades: ["write"], + posture: DENIAL_POSTURE[key], allowed: new Set([meta.mode === "check" ? "clean" : "applied"]), mayWrite: false, }; } - const grant = sectionGrade(key, meta.mask, meta.orgMask ?? meta.mask); + const grant = sectionGrade(key, meta.mask); const grades = effectiveGrades(grant, gating); const deniedAtGatedRead = gating === "mixed" && grant === "read"; - const arms = grades.flatMap((grade) => - grade === "none" && deniedAtGatedRead - ? DENIAL_POSTURES.map((posture) => predictAtGrade(key, meta, grade, posture)) - : [predictAtGrade(key, meta, grade, DENIAL_POSTURE[key])], - ); + // The repository grant holds but the org gate is shut: the repository reads pass and the org-gated probes are + // denied, so the section runs at grade none under the absent posture (orgGateDenied). With no repository grant + // the gated repository read is denied first, under the section's own posture. + const orgDenied = orgGateDenied(key, meta.orgMask ?? meta.mask) && grant !== "none"; + const posture: DenialPosture = orgDenied ? "absent" : DENIAL_POSTURE[key]; + const arms = orgDenied + ? [predictAtGrade(key, meta, "none", posture)] + : grades.flatMap((grade) => + grade === "none" && deniedAtGatedRead + ? DENIAL_POSTURES.map((candidate) => predictAtGrade(key, meta, grade, candidate)) + : [predictAtGrade(key, meta, grade, posture)], + ); return { key, - grades, + grades: orgDenied ? ["none"] : grades, + posture, allowed: new Set(arms.flatMap((arm) => [...arm.allowed])), mayWrite: arms.some((arm) => arm.mayWrite), }; @@ -356,8 +383,7 @@ export function preflightDeniable(section: SectionPrediction, meta: ScenarioMeta if (section.grades.length > 1) { return "possible"; } - const posture = DENIAL_POSTURE[section.key]; - return meta.denialStyle === 403 || posture === "denied" ? "yes" : "no"; + return meta.denialStyle === 403 || section.posture === "denied" ? "yes" : "no"; } export function predictOutcomes(meta: ScenarioMeta): RunPrediction { diff --git a/test/e2e/runner.ts b/test/e2e/runner.ts index c099bafd5..dbd648a63 100644 --- a/test/e2e/runner.ts +++ b/test/e2e/runner.ts @@ -426,7 +426,7 @@ function childEnv( GITHUB_STEP_SUMMARY: join(dir, SUMMARY_FILE), RUNNER_DEBUG: "1", // A test knob: millisecond plugin units and the immediate scheduler, so retry scenarios run in milliseconds instead of seconds. - RETRY_BASE_MS: "1", + GSAC_RETRY_BASE_MS: "1", }; // settings-file is a single-repo input the action rejects beside the multi-repo inputs and in // snapshot mode. A merge lists every layer file below settings.yml, lowest first; a snapshot names diff --git a/test/e2e/schema.ts b/test/e2e/schema.ts index 8612d5955..12b450ad2 100644 --- a/test/e2e/schema.ts +++ b/test/e2e/schema.ts @@ -310,7 +310,7 @@ const DiscoverySchema = z * failures the permission and handler layers cannot: * * rate_limit_403 -> 403 with "rate limit" in the body; the client must read it as throttling, not a denial - * 429_then_200 -> the secondary-rate-limit shape; the throttling plugin honors its Retry-After (in RETRY_BASE_MS units under the runner) + * 429_then_200 -> the secondary-rate-limit shape; the throttling plugin honors its Retry-After (in GSAC_RETRY_BASE_MS units under the runner) * server_error -> 5xx rotating 500/502/503 per firing; times 1 recovers, times >= 3 (1 + MAX_RETRIES) fails * connection_drop -> the socket dies before any response, a network failure surfaced after the retries * echo_422 -> a validation rejection quoting the request body verbatim; a secret-carrying request must surface none of it @@ -327,7 +327,7 @@ const FaultSchema = z * Names a scenario's `env` map may not set: the runner builds the child environment from scratch * and these are its own controls. The prefixes are the SAME set secret references refuse. */ -const RESERVED_ENV_NAMES = new Set(["PATH", "HOME", "RETRY_BASE_MS"]); +const RESERVED_ENV_NAMES = new Set(["PATH", "HOME", "GSAC_RETRY_BASE_MS"]); function reservedEnvKey(name: string): boolean { return RESERVED_ENV_NAMES.has(name) || RESERVED_REF_PREFIXES.some((p) => name.startsWith(p)); diff --git a/test/engine/check-purity.test.ts b/test/engine/check-purity.test.ts index 1303a1aee..d20f5c48d 100644 --- a/test/engine/check-purity.test.ts +++ b/test/engine/check-purity.test.ts @@ -131,6 +131,8 @@ const ROUTES = { data: [{ id: 7, invitee: { login: "carol" }, permissions: "read", expired: false }], }, "GET /orgs/o": { data: { login: "o" } }, + // No team holds access, so the declared one is existence drift and the undeclared walk has nothing to note. + "GET /repos/o/r/teams?per_page=100&page=1": { data: [] }, "GET /repos/o/r/milestones?state=all&per_page=100&page=1": { data: [{ number: 1, title: "old", description: null, state: "open" }], }, diff --git a/test/engine/layers.test.ts b/test/engine/layers.test.ts index ee3d49270..760343921 100644 --- a/test/engine/layers.test.ts +++ b/test/engine/layers.test.ts @@ -915,7 +915,7 @@ describe("stripNulls", () => { ["before", (shared: unknown) => ({ _template: shared, rulesets: shared })], ["after", (shared: unknown) => ({ rulesets: shared, _template: shared })], ])( - "a wrapper aliased under a private key %s the section is stripped by the position it sits in, not the one first met", + "a wrapper aliased under a non-section key %s the section is stripped by the position it sits in, not the one first met", (_order, compose) => { const shared = { entries: [{ name: "main", bypass_actors: null }] }; expect(stripNulls(deepFreeze(compose(shared)))).toEqual({ diff --git a/test/engine/orchestrate.test.ts b/test/engine/orchestrate.test.ts index f6134d7ba..e6500b738 100644 --- a/test/engine/orchestrate.test.ts +++ b/test/engine/orchestrate.test.ts @@ -340,6 +340,23 @@ describe("validateSettingsDoc", () => { ); }); + test("an unknown underscore key is a problem under every allowlist, and it outranks the unknown sections; the document directive passes", () => { + const { io, annotations } = captureIo(); + const doc = { _notes: "private", _layerin: "replace", labls: [], repository: {} }; + const refused = err({ + code: "settings-unknown-directives" as const, + source: "f.yml", + unknown: ["_notes", "_layerin"], + }); + expect(validateSettingsDoc(doc, "f.yml", new Set(), io)).toEqual(refused); + // Outside a `sections` allowlist an unknown SECTION only warns; the underscore rule has no such downgrade. + expect(validateSettingsDoc(doc, "f.yml", new Set(["repository"]), io)).toEqual(refused); + expect(annotations).toEqual([]); + expect( + validateSettingsDoc({ _layering: "replace", repository: {} }, "f.yml", new Set(), io).isOk(), + ).toBe(true); + }); + test.each<[what: string, doc: unknown, shape: TopLevelShape]>([ ["a list", [], "list"], ["null", null, "null"], diff --git a/test/engine/validate.test.ts b/test/engine/validate.test.ts index 4d30200ef..02f3b8ad9 100644 --- a/test/engine/validate.test.ts +++ b/test/engine/validate.test.ts @@ -65,7 +65,7 @@ describe("section shape validation", () => { test("only the declared known sections make up the parsed document", () => { const verdict = validateSectionShapes( - { _notes: "private", pages: { source: { branch: "main" } } }, + { _layering: "replace", pages: { source: { branch: "main" } } }, "f.yml", ); expect(verdict).toEqual(ok({ pages: { source: { branch: "main" } } })); @@ -171,6 +171,30 @@ describe("the wrapped undeclared-policy form", () => { ]); }); + test("an unknown underscore key on a wrapper names the two directives, on a top-level and a nested wrapper alike", () => { + const line = + 'the wrapper\'s directives are "_undeclared" and, on a top-level section, "_layering", and nothing else - ' + + "there are no private-note keys. Remove the key, or keep the note as a YAML comment"; + expect(issuesOf({ labels: { _notes: "private", entries: [{ name: "bug" }] } })).toEqual([ + `labels: Unrecognized key: "_notes"; ${line}`, + ]); + expect( + issuesOf({ + environments: [{ name: "prod", variables: { _layering: "merge", entries: [] } }], + }), + ).toEqual([`environments[0].variables: Unrecognized key: "_layering"; ${line}`]); + // Beside a plain typo the clause names the underscore key it is about; the typo stays on zod's own line. + expect(issuesOf({ labels: { _notes: "x", entires: [], entries: [] } })).toEqual([ + `labels: Unrecognized keys: "_notes", "entires"; "_notes": ${line}`, + ]); + // Beside the pre-v3 policy key both clauses appear, so one run names every fix. + expect(issuesOf({ labels: { undeclared: "keep", _owner: "note", entries: [] } })).toEqual([ + 'labels: Unrecognized keys: "undeclared", "_owner"; the wrapper\'s policy key "undeclared" was renamed ' + + 'to "_undeclared" in v3 (a directive, like _layering) - write _undeclared: keep or ' + + `_undeclared: delete; "_owner": ${line}`, + ]); + }); + test("entry paths keep their precision inside the wrapper", () => { expect( issuesOf({ diff --git a/test/github/api.test.ts b/test/github/api.test.ts index 19f28e711..549310928 100644 --- a/test/github/api.test.ts +++ b/test/github/api.test.ts @@ -120,13 +120,13 @@ describe("retry and throttling", () => { }, 20_000); // Two fixed-backoff retries (~1s + ~4s) before the final failure. }); -describe("RETRY_BASE_MS: millisecond units and the immediate scheduler, same plugin topology", () => { - const saved = process.env.RETRY_BASE_MS; +describe("GSAC_RETRY_BASE_MS: millisecond units and the immediate scheduler, same plugin topology", () => { + const saved = process.env.GSAC_RETRY_BASE_MS; afterEach(() => { if (saved === undefined) { - delete process.env.RETRY_BASE_MS; + delete process.env.GSAC_RETRY_BASE_MS; } else { - process.env.RETRY_BASE_MS = saved; + process.env.GSAC_RETRY_BASE_MS = saved; } }); @@ -168,7 +168,7 @@ describe("RETRY_BASE_MS: millisecond units and the immediate scheduler, same plu new GitHubApi({ token: "t", io, baseUrl: "https://api.test", apiVersion: "2022-11-28" }); test("many writes complete without the write limiter's ~1s spacing", async () => { - process.env.RETRY_BASE_MS = "1"; + process.env.GSAC_RETRY_BASE_MS = "1"; stubFetch([() => new Response(null, { status: 204 })]); const client = envKnobClient(traceIo().io); const started = Date.now(); @@ -190,7 +190,7 @@ describe("RETRY_BASE_MS: millisecond units and the immediate scheduler, same plu // The retry plugin never sees a 429 (doNotRetry) and would ignore Retry-After if it did; only the throttling plugin's callback writes this // trace line, so the line pins which plugin owned the recovery. No Bottleneck group serving a request pins that the knob selected the // immediate scheduler, with no clock involved (the retry plugin schedules through a bare Bottleneck of its own, never a group). - process.env.RETRY_BASE_MS = "50"; + process.env.GSAC_RETRY_BASE_MS = "50"; const state = stubFetch([secondaryLimit("60"), okJson]); const trace = traceIo(); const timers = spyOn(TIMERS_SCHEDULER.Group.prototype, "key"); @@ -268,7 +268,7 @@ describe("RETRY_BASE_MS: millisecond units and the immediate scheduler, same plu }); test("a 429 whose Retry-After exceeds the wait cap fails at once instead of being retried blind", async () => { - process.env.RETRY_BASE_MS = "1"; + process.env.GSAC_RETRY_BASE_MS = "1"; const state = stubFetch([secondaryLimit(String(MAX_RETRY_WAIT_S + 1)), okJson]); const result = await envKnobClient(traceIo().io).tryRequest("GET", "/rl"); expect(state.calls).toBe(1); diff --git a/test/problem.test.ts b/test/problem.test.ts index bd4aa4ae0..afdbe3d1f 100644 --- a/test/problem.test.ts +++ b/test/problem.test.ts @@ -178,7 +178,14 @@ const SPECIMENS = { ], "settings-unknown-sections": [ { code: "settings-unknown-sections", source: "f.yml", unknown: ["labls"], known: SECTION_KEYS }, - `unknown top-level section(s) in f.yml: labls (known: ${KNOWN}). Fix the typo, or prefix private keys with "_", or set the "sections" input to limit processing`, + `unknown top-level section(s) in f.yml: labls (known: ${KNOWN}). Fix the typo, or set the "sections" input to limit processing`, + ], + "settings-unknown-directives": [ + { code: "settings-unknown-directives", source: "f.yml", unknown: ["_notes", "_layerin"] }, + "unknown underscore key(s) in f.yml: _notes, _layerin. The underscore marks this action's " + + "directives, \"_layering\" (a file's top level or a list section's {entries} wrapper) and " + + '"_undeclared" (a wrapper), and nothing else; there are no private-note keys. Remove the key, ' + + "or keep the note as a YAML comment", ], "settings-malformed-sections": [ { @@ -426,6 +433,7 @@ describe("SettingsProblem", () => { "settings-not-mapping", "settings-not-plain-mapping", "settings-unknown-sections", + "settings-unknown-directives", "settings-malformed-sections", ]; // @ts-expect-error a read failure shares the prefix but is not a validation problem diff --git a/test/published-schema.test.ts b/test/published-schema.test.ts index 083206058..55954b688 100644 --- a/test/published-schema.test.ts +++ b/test/published-schema.test.ts @@ -60,6 +60,7 @@ describe("published schema wrapper strictness", () => { "UndeclaredPolicyList", "UndeclaredPolicyList", "UndeclaredPolicyList", + "UndeclaredPolicyList", "UndeclaredPolicyList", ]); for (const name of wrapperNames) { diff --git a/test/schema-corpus.test.ts b/test/schema-corpus.test.ts index 232d1116c..c39d6ed11 100644 --- a/test/schema-corpus.test.ts +++ b/test/schema-corpus.test.ts @@ -66,7 +66,7 @@ function scenarioDocs(): CorpusDoc[] { } } // The corpus size is pinned exactly so a loader that silently drops a root, a file, or a document kind cannot pass. - expect(docs.length).toBe(332); + expect(docs.length).toBe(334); return docs; } diff --git a/test/schema-slices.test.ts b/test/schema-slices.test.ts index 7ca9bf687..313786d09 100644 --- a/test/schema-slices.test.ts +++ b/test/schema-slices.test.ts @@ -29,7 +29,7 @@ import { PagesConfig } from "../src/sections/pages/schema.js"; import { RepositoryConfig } from "../src/sections/repository/schema.js"; import { RulesetConfig } from "../src/sections/rulesets/schema.js"; import { SecretScanningPatternConfig } from "../src/sections/secret_scanning_custom_patterns/schema.js"; -import { TeamsConfig } from "../src/sections/teams/schema.js"; +import { TeamConfig } from "../src/sections/teams/schema.js"; import { WebhookConfig } from "../src/sections/webhooks/schema.js"; import { WorkflowsConfig } from "../src/sections/workflows/schema.js"; @@ -68,7 +68,7 @@ const EXPECTED: Record< code_scanning_default_setup: { kind: "slice", slice: CodeScanningDefaultSetupConfig }, code_quality_setup: { kind: "slice", slice: CodeQualitySetupConfig }, collaborators: { kind: "knob", entry: CollaboratorConfig }, - teams: { kind: "slice", slice: TeamsConfig }, + teams: { kind: "knob", entry: TeamConfig }, milestones: { kind: "knob", entry: MilestoneConfig }, interaction_limits: { kind: "slice", slice: InteractionLimitsConfig }, actions_variables: { kind: "knob", entry: ActionsVariableConfig }, diff --git a/test/scripts/changed-sections.test.ts b/test/scripts/changed-sections.test.ts index 2f2947376..01ba415d9 100644 --- a/test/scripts/changed-sections.test.ts +++ b/test/scripts/changed-sections.test.ts @@ -112,7 +112,7 @@ describe("changed-sections derived fan-out", () => { "repo-variables.ts": inKeyOrder("actions_variables", "agents_variables"), "setup-section.ts": inKeyOrder("code_scanning_default_setup", "code_quality_setup"), // The snapshot projection and knobbed wrapper: every section with a snapshot() except - // workflows and teams, whose two-field entries are spelled by hand. + // workflows, whose two-field entries are spelled by hand. "snapshot-helpers.ts": inKeyOrder( "repository", "labels", @@ -128,6 +128,7 @@ describe("changed-sections derived fan-out", () => { "code_scanning_default_setup", "code_quality_setup", "collaborators", + "teams", "milestones", "interaction_limits", "actions_variables", diff --git a/test/sections/registry.test.ts b/test/sections/registry.test.ts index 99061fb73..978d9a202 100644 --- a/test/sections/registry.test.ts +++ b/test/sections/registry.test.ts @@ -135,6 +135,7 @@ describe("section permissions", () => { codespaces_secrets: "keep", agents_secrets: "keep", collaborators: "delete", + teams: "keep", milestones: "keep", actions_variables: "delete", agents_variables: "delete", @@ -168,7 +169,10 @@ describe("section permissions", () => { }).toEqual({ topLevel: Object.fromEntries(UNDECLARED_POLICY_SECTIONS.map((key) => [key, true])), nested: Object.keys(nested) - .map((list) => [`0.${list}`, 'Unrecognized key: "_layering"']) + .map((list) => [ + `0.${list}`, + 'Unrecognized key: "_layering"; the wrapper\'s directives are "_undeclared" and, on a top-level section, "_layering", and nothing else - there are no private-note keys. Remove the key, or keep the note as a YAML comment', + ]) .sort(), }); }); @@ -303,9 +307,10 @@ describe("section permissions", () => { ) : value; for (const [key, endpoint] of Object.entries(allEndpoints())) { - // The WHOLE declaration is compared, so a later EndpointDecl field cannot diverge uncovered. + // The WHOLE wire declaration is compared, so a later EndpointDecl field cannot diverge uncovered; only the + // 404 posture stays out, since it is the SECTION's (denialPosture reads it per section, never per route). // The effective permission rides along: two declarations can both omit an override while inheriting different section permissions. - const { route: _route, section, role: _role, ...rest } = endpoint; + const { route: _route, section, role: _role, primaryRead: _posture, ...rest } = endpoint; const projected = { ...rest, effective: rest.permission ?? sectionPermission.get(section) }; const contract = JSON.stringify(canonical(projected)); const group = byRoute.get(endpoint.route) ?? []; @@ -469,6 +474,7 @@ describe("section endpoints", () => { "environments.listProtectionRules", "environments.removePolicy", "environments.removeProtectionRule", + "teams.list", "teams.org", ]); }); @@ -752,6 +758,7 @@ describe("allEndpoints", () => { "teams.list", "teams.org", "teams.probe", + "teams.revoke", "webhooks.create", "webhooks.list", "webhooks.remove", diff --git a/test/sections/snapshot-rows/teams.ts b/test/sections/snapshot-rows/teams.ts index c8c7756e2..b95039fad 100644 --- a/test/sections/snapshot-rows/teams.ts +++ b/test/sections/snapshot-rows/teams.ts @@ -2,7 +2,7 @@ import { teamsSection } from "../../../src/sections/teams/index.js"; import type { Row } from "../snapshot-roundtrip.js"; // A null access entry is a team without access, which the listing omits; a custom role named -// "push" has no declaration that plans as itself, and the section never removes an undeclared team. +// "push" has no declaration that plans as itself. The wrapper spells the section's keep default out. export const row: Row = { section: teamsSection, live: { @@ -15,11 +15,14 @@ export const row: Row = { }, }, expected: { - value: [ - { name: "platform", permission: "push" }, - { name: "auditors", permission: "security-auditor" }, - { name: "readers", permission: "pull" }, - ], + value: { + _undeclared: "keep", + entries: [ + { name: "platform", permission: "push" }, + { name: "auditors", permission: "security-auditor" }, + { name: "readers", permission: "pull" }, + ], + }, notes: [ 'teams[pushy]: the live role "push" has no declaration that plans as itself ("push" in a settings file means the "write" role); not declared', ],