diff --git a/docs/upgrading/v2-to-v3.md b/docs/upgrading/v2-to-v3.md index 6b4e778f..84bd1936 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 -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). +Twenty 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 | |---|---|---|---| @@ -26,6 +26,8 @@ Eighteen breaks (the ninth is for library consumers). One is silent (the fallbac | 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) | +| The sealing key is read at apply time | Check mode read `GET .../secrets/public-key` and failed on a malformed key | The first sealed PUT reads it at apply | Check mode issues one request fewer per secret family; a malformed key fails at apply; [section 19](#19-the-sealing-key-is-read-at-apply-time) | +| Environment secrets and variables plan through the shared engines | Their own wording | The engines' wording | Only a grep over the output notices; [section 20](#20-environment-secrets-and-variables-plan-through-the-shared-engines) | ## 1. The defaults-file fallback @@ -277,6 +279,27 @@ The plain array form keeps working, and the [undeclared policy](../reference/und 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. +## 19. The sealing key is read at apply time + +Every secret family (`actions_secrets`, `dependabot_secrets`, `codespaces_secrets`, `agents_secrets`) now reads `GET .../secrets/public-key` from the first sealed PUT, as environment secrets always did. + +- Check mode issues one request fewer per family and never touches the key endpoint. +- A malformed key fails the first PUT at apply, before its request leaves, where v2 failed the plan in check mode too. +- The cannot-verify note carries the section label and the shared template: `actions_secrets: Actions secret values cannot be read back from GitHub, so check mode cannot verify them, only that each declared secret exists; apply re-seals and rewrites every declared value on every run`. + +## 20. Environment secrets and variables plan through the shared engines + +Their lines are the engines' lines now: + +| Line | v2 | v3 | +|---|---|---| +| A missing declared variable | `environments[prod].variables[X]: missing - declared in the settings file but not on the environment; ...` | `... but not on environment "prod"; ...` | +| A variable delete | `DELETED undeclared variable "X" from environment "prod"` | `DELETED undeclared variable "X" in environment "prod"` | +| Two entries naming one variable or secret | `environments: the "prod" entry declares variables that GitHub treats as the same variable ...` | `environments: the settings file declares entries that name the same variable of the "prod" environment: "a" and "A". Keep exactly one entry per resource` (branch policies and protection rules spell theirs the same way) | +| The secrets cannot-verify note | once per environment that exists | once per environment with declared secrets, the missing environment included, under the `environments[prod].secrets` label | + +Repository variable operations (`actions_variables`, `agents_variables`) gain a describe line in failure prose (`creating Actions variable "X"`); nothing else moves. + ## Order of operations 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. diff --git a/src/sections/actions_secrets/actions_secrets.test.ts b/src/sections/actions_secrets/actions_secrets.test.ts index 13ff5d57..87a345ee 100644 --- a/src/sections/actions_secrets/actions_secrets.test.ts +++ b/src/sections/actions_secrets/actions_secrets.test.ts @@ -20,7 +20,7 @@ const LIST = "GET /repos/o/r/actions/secrets?per_page=100&page=1"; const PUBLIC_KEY = "GET /repos/o/r/actions/secrets/public-key"; const KEY_ROUTE = { data: { key_id: "test-key-id", key: MOCK_SECRETS_PUBLIC_KEY } }; const CANNOT_VERIFY = - "Actions secret values cannot be read back from GitHub, so check mode verifies only that each declared secret exists; apply re-seals and rewrites every declared value on each run"; + "actions_secrets: Actions secret values cannot be read back from GitHub, so check mode cannot verify them, only that each declared secret exists; apply re-seals and rewrites every declared value on every run"; type Declared = Parameters[1]; @@ -141,9 +141,9 @@ describe("actions_secrets planning", () => { notes: [CANNOT_VERIFY], drift: [], }); + // The sealing key is an execution-time read, so check mode issues the list alone. expect(api.calls.map((c) => `${c.method} ${c.path}`)).toEqual([ "GET /repos/o/r/actions/secrets?per_page=100&page=1", - "GET /repos/o/r/actions/secrets/public-key", ]); }); @@ -156,9 +156,15 @@ describe("actions_secrets planning", () => { ]); // Nothing at plan time could have resolved a reference: the resolver is first consulted when a thunk runs. const exec = tools({ $A: "value-a", $B: "value-b" }); - const payloads = result.ops.map((op) => - typeof op.payload === "function" ? sealedPayload({ payload: op.payload(exec) }) : null, + const payloads = await Promise.all( + result.ops.map(async (op) => + typeof op.payload === "function" + ? sealedPayload({ payload: await op.payload(exec) }) + : null, + ), ); + // Both thunks share one key read, issued when the first ran. + expect(api.calls.filter((c) => c.path.endsWith("/public-key")).length).toBe(1); expect(exec.lookups).toEqual(["$A", "$B"]); expect(payloads.map((p) => p?.key_id)).toEqual(["test-key-id", "test-key-id"]); expect(payloads.map((p) => unsealSecretValue(p?.encrypted_value ?? ""))).toEqual([ @@ -413,20 +419,24 @@ describe("actions_secrets execution", () => { }); describe("actions_secrets sealing key", () => { - test("an unusable public key fails the plan loudly, so no operation exists to execute", async () => { + test("an unusable public key fails the first PUT before its request leaves; check mode never reads the key", async () => { const api = new MockApi({ [LIST]: listOf(), [PUBLIC_KEY]: { data: { key: 42 } } }); - await expect(plan(api, [{ name: "X", value: "$V" }])).rejects.toThrow( + const planned = await plan(api, [{ name: "X", value: "$V" }]); + expect(planned.ops.map((op) => op.role)).toEqual(["put"]); + await expect(apply(api, [{ name: "X", value: "$V" }], tools({ $V: "v" }))).rejects.toThrow( /no usable \{key_id, key\} pair \(key_id is missing\)/, ); + expect(api.mutations()).toEqual([]); // GitHub requires key_id in the PUT body to route the ciphertext, so an empty one is as unusable as a missing one. const emptyId = new MockApi({ [LIST]: listOf(), [PUBLIC_KEY]: { data: { key_id: "", key: MOCK_SECRETS_PUBLIC_KEY } }, }); - await expect(plan(emptyId, [{ name: "X", value: "$V" }])).rejects.toThrow( + await expect(apply(emptyId, [{ name: "X", value: "$V" }], tools({ $V: "v" }))).rejects.toThrow( /no usable \{key_id, key\} pair \(key_id is empty\)/, ); + expect(emptyId.mutations()).toEqual([]); }); }); diff --git a/src/sections/actions_secrets/scenarios/actions-secrets-check-drift.yml b/src/sections/actions_secrets/scenarios/actions-secrets-check-drift.yml index 0d2fd53a..dbaf02eb 100644 --- a/src/sections/actions_secrets/scenarios/actions-secrets-check-drift.yml +++ b/src/sections/actions_secrets/scenarios/actions-secrets-check-drift.yml @@ -3,8 +3,8 @@ description: > Check mode reconciles secrets by existence alone: a declared-but-missing secret is drift, the declared values earn one cannot-verify note, an undeclared live secret stays a keep-note under the default policy, and no - reference is resolved (this scenario deliberately defines no env), so only the list and the sealing key are read - the - key is a read the plan closes over for its sealed writes, in every mode. + reference is resolved (this scenario deliberately defines no env), so only the list is read; the sealing + key is a read the plan closes over for its sealed writes, issued at apply. settings: actions_secrets: - name: DEPLOY_TOKEN diff --git a/src/sections/actions_variables/actions_variables.test.ts b/src/sections/actions_variables/actions_variables.test.ts index 772574d3..945f5759 100644 --- a/src/sections/actions_variables/actions_variables.test.ts +++ b/src/sections/actions_variables/actions_variables.test.ts @@ -99,6 +99,7 @@ describe("actions_variables", () => { 'actions_variables[DEPLOY_REGION].value: declared "eu-west-1" != live "us-east-1"; apply will set the declared value', ], change: 'updated Actions variable "DEPLOY_REGION"', + describe: 'updating Actions variable "DEPLOY_REGION"', }, { role: "create", @@ -107,6 +108,7 @@ describe("actions_variables", () => { "actions_variables[BUILD_MODE]: missing - declared in the settings file but not on the repo; apply will create it", ], change: 'created Actions variable "BUILD_MODE"', + describe: 'creating Actions variable "BUILD_MODE"', }, { role: "remove", @@ -115,6 +117,7 @@ describe("actions_variables", () => { "actions_variables[RETIRED_FLAG]: undeclared - not in the settings file, so apply will DELETE it; add it to the settings file to keep it", ], change: 'DELETED undeclared Actions variable "RETIRED_FLAG"', + describe: 'deleting undeclared Actions variable "RETIRED_FLAG"', }, ], notes: [], @@ -168,6 +171,22 @@ describe("actions_variables", () => { expect(api.calls).toHaveLength(0); }); + test("a passthrough value JSON cannot carry fails the plan at the payload, on a create and on an update alike", async () => { + // The loose shape admits any extra key; the engine proves the body plain before it is planned, so + // NaN (which JSON would turn into null) never reaches the wire as a silent change of value. + const api = new MockApi(listRoute([{ name: "PRESENT", value: "x" }])); + const odd = { name: "NEW", value: "v", extra: Number.NaN }; + await expect(plan(api, [odd as never])).rejects.toThrow( + /a planned payload carries a value JSON cannot carry at extra/, + ); + await expect(plan(api, [{ ...odd, name: "PRESENT" } as never])).rejects.toThrow( + /a planned payload carries a value JSON cannot carry at extra/, + ); + // The control: a plain extra key rides through. + const plain = await plan(api, [{ name: "NEW", value: "v", extra: 42 } as never]); + expect(plain.ops.map((op) => op.payload)).toEqual([{ name: "NEW", value: "v", extra: 42 }]); + }); + test("wrapped _undeclared:keep leaves the undeclared variable as a note, never a DELETE", async () => { const api = new MockApi(listRoute(liveVariables)); const result = await plan(api, { diff --git a/src/sections/agents_secrets/scenarios/agents-secrets-check-drift.yml b/src/sections/agents_secrets/scenarios/agents-secrets-check-drift.yml index 09d5876a..90dc6097 100644 --- a/src/sections/agents_secrets/scenarios/agents-secrets-check-drift.yml +++ b/src/sections/agents_secrets/scenarios/agents-secrets-check-drift.yml @@ -4,8 +4,8 @@ description: > declared-but-missing secret is drift, the declared values earn one cannot-verify note, an undeclared live secret stays a keep-note under the default policy, and no reference is resolved (this scenario deliberately - defines no env), so only the list and the sealing key are read - the - key is a read the plan closes over for its sealed writes, in every mode. + defines no env), so only the list is read; the sealing + key is a read the plan closes over for its sealed writes, issued at apply. settings: agents_secrets: - name: AGENT_TOKEN diff --git a/src/sections/codespaces_secrets/scenarios/codespaces-secrets-check-drift.yml b/src/sections/codespaces_secrets/scenarios/codespaces-secrets-check-drift.yml index 2504eb62..5c853bbb 100644 --- a/src/sections/codespaces_secrets/scenarios/codespaces-secrets-check-drift.yml +++ b/src/sections/codespaces_secrets/scenarios/codespaces-secrets-check-drift.yml @@ -4,8 +4,8 @@ description: > declared-but-missing secret is drift, the declared values earn one cannot-verify note, an undeclared live secret stays a keep-note under the default policy, and no reference is resolved (this scenario deliberately - defines no env), so only the list and the sealing key are read - the - key is a read the plan closes over for its sealed writes, in every mode. + defines no env), so only the list is read; the sealing + key is a read the plan closes over for its sealed writes, issued at apply. settings: codespaces_secrets: - name: DOTFILES_PAT diff --git a/src/sections/contract/endpoints.ts b/src/sections/contract/endpoints.ts index 9cbcca5f..b352b382 100644 --- a/src/sections/contract/endpoints.ts +++ b/src/sections/contract/endpoints.ts @@ -84,7 +84,8 @@ export interface GatedReadDecl extends EndpointDeclFields { readonly accessGrade: "write"; readonly alwaysRewrite?: never; readonly unverifiable?: never; - readonly phase?: never; + /** As on a plain GET: "execution" gates the read behind a thunk's token (the Codespaces sealing key). */ + readonly phase?: "execution"; } interface EndpointDeclFields { diff --git a/src/sections/contract/errors.ts b/src/sections/contract/errors.ts index 325cff73..6634ada7 100644 --- a/src/sections/contract/errors.ts +++ b/src/sections/contract/errors.ts @@ -60,8 +60,11 @@ export function throwFor( ): never { // The operation label says WHAT was being done in settings-file terms; the raw method/path keeps the request identifiable. // creating ruleset "x" failed - POST /repos/...: 422 ... - const operation = context?.operation ? `${context.operation} failed - ` : ""; - const cause = `${operation}${method} ${path}: ${error.status} ${error.message}`; + // the token was denied POST /repos/... (creating ruleset "x"): 403 ... + const request = `${method} ${path}`; + const outcome = `${error.status} ${error.message}`; + const cause = `${context?.operation ? `${context.operation} failed - ` : ""}${request}: ${outcome}`; + const denied = `${request}${context?.operation ? ` (${context.operation})` : ""}: ${outcome}`; if (isRateLimitError(error)) { // Secondary rate limits arrive as 403 and must not read as missing permissions. throw new Error( @@ -85,7 +88,7 @@ export function throwFor( : sectionGrant(section); throw new PermissionDenied( section.key, - `the token was denied ${cause}${alsoMissing}. To fix, ${grant}${denialHint}`, + `the token was denied ${denied}${alsoMissing}. To fix, ${grant}${denialHint}`, error.status, ); } diff --git a/src/sections/dependabot_secrets/scenarios/dependabot-secrets-check-drift.yml b/src/sections/dependabot_secrets/scenarios/dependabot-secrets-check-drift.yml index 2e0b7570..0e60849d 100644 --- a/src/sections/dependabot_secrets/scenarios/dependabot-secrets-check-drift.yml +++ b/src/sections/dependabot_secrets/scenarios/dependabot-secrets-check-drift.yml @@ -4,8 +4,8 @@ description: > declared-but-missing secret is drift, the declared values earn one cannot-verify note, an undeclared live secret stays a keep-note under the default policy, and no reference is resolved (this scenario deliberately - defines no env), so only the list and the sealing key are read - the - key is a read the plan closes over for its sealed writes, in every mode. + defines no env), so only the list is read; the sealing + key is a read the plan closes over for its sealed writes, issued at apply. settings: dependabot_secrets: - name: REGISTRY_TOKEN diff --git a/src/sections/environments/branch-policies.ts b/src/sections/environments/branch-policies.ts index 7db3156b..4e8ea038 100644 --- a/src/sections/environments/branch-policies.ts +++ b/src/sections/environments/branch-policies.ts @@ -9,6 +9,7 @@ import type { UndeclaredPolicy } from "../../types.js"; import { liveByIdentity, parseLive } from "../contract/live.js"; import { type SectionMeta, undeclaredDrift, undeclaredNote } from "../contract/module.js"; import { hasDrift, plainData } from "../contract/plan.js"; +import { rejectDuplicates } from "../contract/requests.js"; import { ENDPOINTS, type EnvironmentRestOp, type EnvironmentsRestContext } from "./endpoints.js"; import type { NestedPlan } from "./nested.js"; import type { DeploymentBranchPolicyConfig, EnvironmentConfig } from "./schema.js"; @@ -59,23 +60,17 @@ export function livePolicyName(policy: LiveBranchPolicy, envName: string): strin * the zod shape (schema.ts), not here, so it fails before any section writes. */ export function validateBranchPolicies( + section: SectionMeta, env: EnvironmentConfig, entries: readonly DeploymentBranchPolicyConfig[], ): void { - const seen = new Set(); - const duplicates = new Set(); - for (const pattern of entries) { - if (seen.has(pattern.name)) { - duplicates.add(pattern.name); - } - seen.add(pattern.name); - } - if (duplicates.size > 0) { - throw new Error( - `environments: the "${env.name}" entry declares deployment branch polic${duplicates.size === 1 ? "y" : "ies"} ` + - `${[...duplicates].map((name) => `"${name}"`).join(", ")} more than once. Keep exactly one entry per pattern`, - ); - } + rejectDuplicates( + section, + entries, + (pattern) => pattern.name, + (pattern) => pattern.name, + `deployment branch policy of the "${env.name}" environment`, + ); } function createPolicyOp( diff --git a/src/sections/environments/environments.test.ts b/src/sections/environments/environments.test.ts index 636f698c..bff28c50 100644 --- a/src/sections/environments/environments.test.ts +++ b/src/sections/environments/environments.test.ts @@ -99,7 +99,7 @@ describe("environments plan", () => { 'applied environment "prod"', 'created variable "NEW" in environment "prod"', 'updated variable "UPD" in environment "prod"', - 'DELETED undeclared variable "GONE" from environment "prod"', + 'DELETED undeclared variable "GONE" in environment "prod"', ]); // The strip builds a fresh object, so the duplicate pre-pass (which reads env.variables across all entries) never sees a mutated declaration. expect(declared[0]?.variables).toEqual([ @@ -251,7 +251,7 @@ describe("environments variables check mode", () => { }); const result = await check(api, [{ name: "prod", variables: [{ name: "A", value: "1" }] }]); expect(result.drift).toEqual([ - "environments[prod].variables[A]: missing - declared in the settings file but not on the environment; apply will create it", + 'environments[prod].variables[A]: missing - declared in the settings file but not on environment "prod"; apply will create it', ]); }); }); @@ -292,7 +292,7 @@ describe("environments variables case-insensitive matching", () => { }, ]), ).rejects.toThrow( - 'environments: the "prod" entry declares variables that GitHub treats as the same variable (names are case-insensitive): "Region" and "REGION". Keep exactly one entry per variable', + 'environments: the settings file declares entries that name the same variable of the "prod" environment: "Region" and "REGION". Keep exactly one entry per resource', ); expect(api.calls).toEqual([]); }); @@ -319,9 +319,7 @@ describe("environments variables undeclared policy", () => { [VARIABLES_LIST]: variablesBody([{ name: "LEGACY", value: "x" }]), }).allowMutations("DELETE /repos/o/r/environments/prod/variables/LEGACY"); const result = await apply(api, [{ name: "prod", variables: [] }]); - expect(result.changes).toEqual([ - 'DELETED undeclared variable "LEGACY" from environment "prod"', - ]); + expect(result.changes).toEqual(['DELETED undeclared variable "LEGACY" in environment "prod"']); }); test("check mode under _undeclared:keep converges (note, not drift)", async () => { @@ -585,7 +583,7 @@ describe("environments nested secrets validation and shape", () => { ], }, ]), - ).rejects.toThrow(/"prod" entry declares secrets .*"token" and "TOKEN"/); + ).rejects.toThrow(/the same secret of the "prod" environment: "token" and "TOKEN"/); expect(api.calls).toEqual([]); }); @@ -811,7 +809,7 @@ describe("environments deployment branch policies validation and shape", () => { await expect( plan(api, [envWithPolicies([{ name: "release/*" }, { name: "release/*", type: "tag" }])]), ).rejects.toThrow( - 'environments: the "prod" entry declares deployment branch policy "release/*" more than once. Keep exactly one entry per pattern', + 'environments: the settings file declares entries that name the same deployment branch policy of the "prod" environment: "release/*" and "release/*". Keep exactly one entry per resource', ); expect(api.calls).toEqual([]); }); @@ -1112,7 +1110,7 @@ describe("environments missing-environment planning across the nested families", { name: "prod", variables: [{ name: "A", value: "1" }] }, "environments[prod].variables: not verifiable while the environment is missing; apply will create the environment and reconcile the declared variables", "/variables", - "environments[prod].variables[A]: missing - declared in the settings file but not on the environment; apply will create it", + 'environments[prod].variables[A]: missing - declared in the settings file but not on environment "prod"; apply will create it', ], [ "secrets", @@ -1137,14 +1135,22 @@ describe("environments missing-environment planning across the nested families", ], ] as Array<[string, EnvironmentConfig, string, string, string]>)( "%s: the sub-resource read is skipped, the note says it is unverifiable, and the create is planned", - async (_key, entry, expectedNote, subResourcePath, createDrift) => { + async (key, entry, expectedNote, subResourcePath, createDrift) => { const api = new MockApi({}); const result = await check(api, [entry]); expect(result.drift).toEqual([ "environments[prod]: missing - declared in the settings file but not on the repo; apply will create it", createDrift, ]); - expect(result.notes).toEqual([expectedNote]); + // The secrets engine states once per scope that values never read back, beside the missing-environment note. + expect(result.notes).toEqual( + key === "secrets" + ? [ + expectedNote, + "environments[prod].secrets: prod environment secret values cannot be read back from GitHub, so check mode cannot verify them, only that each declared secret exists; apply re-seals and rewrites every declared value on every run", + ] + : [expectedNote], + ); expect(api.calls.filter((c) => c.path.includes(subResourcePath))).toEqual([]); }, ); @@ -1161,7 +1167,7 @@ describe("environments deployment protection rules validation and shape", () => }, ]), ).rejects.toThrow( - 'environments: the "prod" entry declares the deployment protection rule App "deploy-gate" more than once. Keep exactly one entry per App', + 'environments: the settings file declares entries that name the same deployment protection rule App of the "prod" environment: "deploy-gate" and "deploy-gate". Keep exactly one entry per resource', ); expect(api.calls).toEqual([]); }); @@ -1358,7 +1364,7 @@ describe("environments convergence", () => { 'applied environment "prod"', 'created variable "DEPLOY_REGION" in environment "prod"', 'updated variable "log_level" in environment "prod"', - 'DELETED undeclared variable "GONE" from environment "prod"', + 'DELETED undeclared variable "GONE" in environment "prod"', 'updated secret "DEPLOY_TOKEN" in environment "prod"', 'created deployment branch policy "release/*" in environment "prod"', 'deleted deployment branch policy "v*" in environment "prod" to change its immutable type (branch -> tag)', @@ -1389,7 +1395,7 @@ describe("environments convergence", () => { "PUT /repos/o/r/environments/prod/secrets/DEPLOY_TOKEN", ]); expect(first.notes).toEqual([ - "prod environment secret values cannot be read back from GitHub, so check mode verifies only that each declared secret exists; apply re-seals and rewrites every declared value on each run", + "environments[prod].secrets: prod environment secret values cannot be read back from GitHub, so check mode cannot verify them, only that each declared secret exists; apply re-seals and rewrites every declared value on every run", 'prod environment secret "KEPT" exists on the environment but is not declared in the ' + 'settings file; kept under "_undeclared: keep" - add it to the settings file to manage ' + 'it, or set "_undeclared: delete" to have apply DELETE it (a deleted secret\'s value is ' + @@ -1471,7 +1477,7 @@ describe("environments convergence", () => { [{ role: "putSecret", drift: [], change: 'updated secret "NEW" in environment "staging"' }], ); expect(second.notes).toEqual([ - "staging environment secret values cannot be read back from GitHub, so check mode verifies only that each declared secret exists; apply re-seals and rewrites every declared value on each run", + "environments[staging].secrets: staging environment secret values cannot be read back from GitHub, so check mode cannot verify them, only that each declared secret exists; apply re-seals and rewrites every declared value on every run", ]); }); }); diff --git a/src/sections/environments/index.ts b/src/sections/environments/index.ts index e7eb1a10..8b91b05c 100644 --- a/src/sections/environments/index.ts +++ b/src/sections/environments/index.ts @@ -72,7 +72,7 @@ export const environmentsSection = { // one entry invalidates. for (const env of desired) { for (const key of NESTED_KEYS) { - validateNested(key, env); + validateNested(this, key, env); } } const plan: EnvironmentsPlan = { ops: [], notes: [], drift: [] }; diff --git a/src/sections/environments/nested.ts b/src/sections/environments/nested.ts index 995b835e..b112e1ea 100644 --- a/src/sections/environments/nested.ts +++ b/src/sections/environments/nested.ts @@ -1,22 +1,19 @@ import { z } from "zod"; -import { phantomKeys, phantomNote, subsetDiff } from "../../engine/diff.js"; import type { MustBeNever, UndeclaredPolicy, UndeclaredPolicyList } from "../../types.js"; -import { liveByIdentity, parseLive } from "../contract/live.js"; -import { - type EntryOf, - type SectionMeta, - undeclaredDrift, - undeclaredNote, - undeclaredPolicy, -} from "../contract/module.js"; -import { type ExecTools, hasDrift, plainData } from "../contract/plan.js"; +import { parseLive } from "../contract/live.js"; +import { type EntryOf, type SectionMeta, undeclaredPolicy } from "../contract/module.js"; import { LIVE_SECRET_NAMES, - parseSealingKey, - type SealingKey, - secretKey, + planSecrets, + rejectDuplicateSecretNames, + type SecretsPlanScope, } from "../shared/secrets-engine.js"; -import { LiveVariable, variableKey } from "../shared/variables-engine.js"; +import { + LiveVariable, + planVariables, + rejectDuplicateVariableNames, + type VariablesPlanScope, +} from "../shared/variables-engine.js"; import { BRANCH_POLICIES_DEFAULT_POLICY, planBranchPolicies, @@ -87,7 +84,11 @@ interface NestedPlanner { */ missingNote: (envName: string) => string; /** Rejects misdeclared entries for every environment before anything is read or written. */ - validate?: (env: EnvironmentConfig, entries: readonly NestedEntry[]) => void; + validate?: ( + section: SectionMeta, + env: EnvironmentConfig, + entries: readonly NestedEntry[], + ) => void; plan: ( ctx: EnvironmentsRestContext, section: SectionMeta, @@ -102,6 +103,11 @@ interface NestedPlanner { ) => Promise; } +/** The resource noun a nested duplicate is reported under: `variable of the "prod" environment`. */ +function nestedWhat(noun: string, env: EnvironmentConfig): string { + return `${noun} of the "${env.name}" environment`; +} + const NESTED_PLANNERS: { [K in NestedKey]: NestedPlanner } = { variables: { // "delete" like the top-level actions_variables default: variables are @@ -109,8 +115,9 @@ const NESTED_PLANNERS: { [K in NestedKey]: NestedPlanner } = { defaultPolicy: "delete", missingNote: (envName) => `environments[${envName}].variables: not verifiable while the environment is missing; apply will create the environment and reconcile the declared variables`, - validate: (env, entries) => rejectDuplicateVariables(env.name, entries), - plan: planVariables, + validate: (section, env, entries) => + rejectDuplicateVariableNames(section, entries, nestedWhat("variable", env)), + plan: planEnvironmentVariables, }, secrets: { // "keep" like the top-level secret families: a deleted secret's value is @@ -118,7 +125,8 @@ const NESTED_PLANNERS: { [K in NestedKey]: NestedPlanner } = { defaultPolicy: "keep", missingNote: (envName) => `environments[${envName}].secrets: not verifiable while the environment is missing; apply will create the environment and reconcile the declared secrets`, - validate: (env, entries) => rejectDuplicateSecrets(env.name, entries), + validate: (section, env, entries) => + rejectDuplicateSecretNames(section, entries, nestedWhat("secret", env)), plan: planEnvironmentSecrets, }, deployment_branch_policies: { @@ -158,10 +166,14 @@ export function nestedDefaultPolicy(key: NestedKey): UndeclaredPolicy { return NESTED_PLANNERS[key].defaultPolicy; } -export function validateNested(key: K, env: EnvironmentConfig): void { +export function validateNested( + section: SectionMeta, + key: K, + env: EnvironmentConfig, +): void { const declared = env[key]; if (declared !== undefined) { - NESTED_PLANNERS[key].validate?.(env, unwrapNested(key, declared).entries); + NESTED_PLANNERS[key].validate?.(section, env, unwrapNested(key, declared).entries); } } @@ -225,29 +237,6 @@ export function splitEntry(env: EnvironmentConfig): { return { settings: settings as Record, nested, routed }; } -/** Two spellings of one case-insensitive name would fight each other on every run. */ -function rejectDuplicateVariables( - envName: string, - entries: readonly EnvironmentVariableConfig[], -): void { - const seen = new Map(); - const collisions: string[] = []; - for (const variable of entries) { - const key = variableKey(variable.name); - const first = seen.get(key); - if (first !== undefined) { - collisions.push(`"${first}" and "${variable.name}"`); - continue; - } - seen.set(key, variable.name); - } - if (collisions.length > 0) { - throw new Error( - `environments: the "${envName}" entry declares variables that GitHub treats as the same variable (names are case-insensitive): ${collisions.join("; ")}. Keep exactly one entry per variable`, - ); - } -} - /** One environment's live Actions variables. */ export async function listEnvironmentVariables( ctx: EnvironmentsRestContext, @@ -265,7 +254,24 @@ export async function listEnvironmentVariables( ); } -async function planVariables( +/** + * The words the two engines render one environment's nested lists with. A variable's kept note names + * the environment (two environments holding the same undeclared name would otherwise emit one note + * twice); a secret's noun already carries it. + */ +function nestedProse(envName: string, key: "variables" | "secrets", noun: string) { + return { + label: `environments[${envName}].${key}`, + noun, + where: key === "variables" ? `environment "${envName}"` : "the environment", + suffix: ` in environment "${envName}"`, + }; +} + +type Op = Extract; + +/** The variables engine over one environment; an environment the plan creates lists nothing and plans every entry as a create. */ +async function planEnvironmentVariables( ctx: EnvironmentsRestContext, section: SectionMeta, envName: string, @@ -274,113 +280,44 @@ async function planVariables( liveEnv: Record | undefined, ): Promise { const params = { environment_name: envName }; - const label = `environments[${envName}].variables`; - const live = liveEnv === undefined ? [] : await listEnvironmentVariables(ctx, section, envName); - const liveByKey = liveByIdentity( - section, - "variable", - live, - (variable) => variableKey(variable.name), - (variable) => variable.name, - ); - const declaredKeys = new Set(entries.map((variable) => variableKey(variable.name))); - const planned: NestedPlan = { ops: [], notes: [] }; - - for (const variable of entries) { - const entryLabel = `${label}[${variable.name}]`; - const existing = liveByKey.get(variableKey(variable.name)); - const { name: _name, value: _value, ...extraKeys } = variable; - if (!existing) { - planned.ops.push({ - role: "createVariable", - params, - payload: plainData({ name: variable.name, value: variable.value, ...extraKeys }), - drift: [ - `${entryLabel}: missing - declared in the settings file but not on the environment; apply will create it`, - ], - change: `created variable "${variable.name}" in environment "${envName}"`, - describe: `creating variable "${variable.name}" in environment "${envName}"`, - }); - continue; - } - // GitHub stores the name uppercased whatever casing the file uses, so the name never drifts; - // only the value and the declared passthrough fields can. - const drift = [ - ...(existing.value === variable.value - ? [] - : [ - `${entryLabel}.value: declared ${JSON.stringify(variable.value)} != live ${JSON.stringify(existing.value)}; apply will set the declared value`, - ]), - ...subsetDiff(extraKeys, existing, entryLabel), - ]; - if (!hasDrift(drift)) { - continue; - } - const phantom = phantomKeys(extraKeys, existing); - if (phantom.length > 0) { - planned.notes.push(phantomNote(entryLabel, phantom, "variable", "this update will re-run")); - } - planned.ops.push({ + const scope: VariablesPlanScope< + Op<"createVariable">, + Op<"updateVariable">, + Op<"removeVariable"> + > = { + ...nestedProse(envName, "variables", "variable"), + list: async () => + liveEnv === undefined ? [] : await listEnvironmentVariables(ctx, section, envName), + create: (write) => ({ + role: "createVariable", + params, + payload: write.payload, + drift: write.drift, + change: write.change, + describe: write.describe, + }), + update: (write) => ({ role: "updateVariable", - params: { ...params, name: existing.name }, - payload: plainData({ value: variable.value, ...extraKeys }), - drift, - change: `updated variable "${variable.name}" in environment "${envName}"`, - describe: `updating variable "${variable.name}" in environment "${envName}"`, - }); - } - - for (const variable of liveByKey.values()) { - if (declaredKeys.has(variableKey(variable.name))) { - continue; - } - if (policy === "keep") { - planned.notes.push( - undeclaredNote({ - subject: `variable "${variable.name}"`, - state: `exists on environment "${envName}" but is not declared`, - action: "DELETE it", - }), - ); - continue; - } - planned.ops.push({ + params: { ...params, name: write.liveName }, + payload: write.payload, + drift: write.drift, + change: write.change, + describe: write.describe, + }), + remove: (deletion) => ({ role: "removeVariable", - params: { ...params, name: variable.name }, - drift: [ - undeclaredDrift(NESTED_PLANNERS.variables.defaultPolicy, { - label: `${label}[${variable.name}]`, - action: "DELETE it", - }), - ], - change: `DELETED undeclared variable "${variable.name}" from environment "${envName}"`, - describe: `deleting undeclared variable "${variable.name}" from environment "${envName}"`, - }); - } - return planned; -} - -/** Two spellings of one case-insensitive name (GitHub stores secret names uppercase) would fight each other on every run. */ -function rejectDuplicateSecrets( - envName: string, - entries: readonly EnvironmentSecretConfig[], -): void { - const seen = new Map(); - const collisions: string[] = []; - for (const secret of entries) { - const key = secretKey(secret.name); - const first = seen.get(key); - if (first !== undefined) { - collisions.push(`"${first}" and "${secret.name}"`); - continue; - } - seen.set(key, secret.name); - } - if (collisions.length > 0) { - throw new Error( - `environments: the "${envName}" entry declares secrets that GitHub treats as the same secret (names are case-insensitive): ${collisions.join("; ")}. Keep exactly one entry per secret`, - ); - } + params: { ...params, name: deletion.name }, + drift: deletion.drift, + change: deletion.change, + describe: deletion.describe, + }), + }; + const planned = await planVariables(section, scope, { + entries, + policy, + defaultPolicy: NESTED_PLANNERS.variables.defaultPolicy, + }); + return { ops: planned.ops, notes: planned.notes }; } /** One environment's live Actions secret names (GitHub never lists values). */ @@ -401,9 +338,9 @@ export async function listEnvironmentSecrets( } /** - * Existence is the only comparable state (values never read back), so every declared secret is a - * sealed PUT. The sealing key is an execution-phase read (endpoints.ts), issued once per environment - * from the first payload thunk that runs; the token it demands is the one the thunk received. + * The secrets engine over one environment. The sealing key is an execution-phase read (endpoints.ts): + * in apply the environment PUT may only just have created the environment the key belongs to, so the + * engine reads it from the first payload thunk that runs, with the token that thunk received. */ async function planEnvironmentSecrets( ctx: EnvironmentsRestContext, @@ -414,87 +351,32 @@ async function planEnvironmentSecrets( liveEnv: Record | undefined, ): Promise { const params = { environment_name: envName }; - const label = `environments[${envName}].secrets`; - const noun = `${envName} environment secret`; - const suffix = ` in environment "${envName}"`; - const live = liveEnv === undefined ? [] : await listEnvironmentSecrets(ctx, section, envName); - // Real GitHub lists names uppercase already; keying by secretKey keeps a differently-cased mock or proxy harmless. - const liveByKey = new Map( - [ - ...liveByIdentity( - section, - noun, - live, - (item) => secretKey(item.name), - (item) => item.name, - ), - ].map(([key, item]) => [key, item.name]), - ); - const declaredKeys = new Set(entries.map((entry) => secretKey(entry.name))); - const planned: NestedPlan = { ops: [], notes: [] }; - - let sealingKey: Promise | undefined; - const readSealingKey = (exec: ExecTools): Promise => { - sealingKey ??= ctx.read.secretsPublicKey - .call(exec, { params, describe: `reading the ${label} sealing key` }) - .then((body) => parseSealingKey(section, { label }, ENDPOINTS.secretsPublicKey, body)); - return sealingKey; - }; - for (const entry of entries) { - const name = secretKey(entry.name); - const exists = liveByKey.has(name); - planned.ops.push({ + const scope: SecretsPlanScope, Op<"removeSecret">> = { + ...nestedProse(envName, "secrets", `${envName} environment secret`), + list: async () => + liveEnv === undefined ? [] : await listEnvironmentSecrets(ctx, section, envName), + publicKey: (exec, describe) => ctx.read.secretsPublicKey.call(exec, { params, describe }), + publicKeyEndpoint: ENDPOINTS.secretsPublicKey, + put: (write) => ({ role: "putSecret", - params: { ...params, secret_name: name }, - payload: async (exec) => { - const plaintext = exec.resolveSecret(entry.value); - return (await readSealingKey(exec)).seal(plaintext); - }, - drift: exists - ? [] - : [ - `${label}[${name}]: missing - declared in the settings file but not on the environment; apply will create it`, - ], - // The listing decides the verb: the PUT's own 201/204 would say the same, but the executor - // deliberately does not surface statuses. - change: `${exists ? "updated" : "created"} secret "${name}"${suffix}`, - describe: `writing secret "${name}"${suffix}`, - }); - } - if (entries.length > 0 && liveEnv !== undefined) { - // One note per environment; an environment the plan creates already carries the - // missing-environment note, which covers its whole list. - planned.notes.push( - `${noun} values cannot be read back from GitHub, so check mode verifies only that each declared secret exists; apply re-seals and rewrites every declared value on each run`, - ); - } - - for (const [key, liveName] of liveByKey) { - if (declaredKeys.has(key)) { - continue; - } - if (policy === "keep") { - planned.notes.push( - undeclaredNote({ - subject: `${noun} "${liveName}"`, - state: "exists on the environment but is not declared", - action: "DELETE it (a deleted secret's value is unrecoverable)", - }), - ); - continue; - } - planned.ops.push({ + params: { ...params, secret_name: write.name }, + payload: write.payload, + drift: write.drift, + change: write.change, + describe: write.describe, + }), + remove: (deletion) => ({ role: "removeSecret", - params: { ...params, secret_name: liveName }, - drift: [ - undeclaredDrift(NESTED_PLANNERS.secrets.defaultPolicy, { - label: `${label}[${liveName}]`, - action: "DELETE it (the value is unrecoverable)", - }), - ], - change: `DELETED undeclared secret "${liveName}"${suffix}`, - describe: `deleting undeclared secret "${liveName}"${suffix}`, - }); - } - return planned; + params: { ...params, secret_name: deletion.name }, + drift: deletion.drift, + change: deletion.change, + describe: deletion.describe, + }), + }; + const planned = await planSecrets(section, scope, { + entries, + policy, + defaultPolicy: NESTED_PLANNERS.secrets.defaultPolicy, + }); + return { ops: planned.ops, notes: planned.notes }; } diff --git a/src/sections/environments/protection-rules.ts b/src/sections/environments/protection-rules.ts index 17de317f..a41f95a2 100644 --- a/src/sections/environments/protection-rules.ts +++ b/src/sections/environments/protection-rules.ts @@ -3,6 +3,7 @@ import type { UndeclaredPolicy } from "../../types.js"; import { liveByIdentity, parseLive } from "../contract/live.js"; import { type SectionMeta, undeclaredDrift, undeclaredNote } from "../contract/module.js"; import type { ExecTools } from "../contract/plan.js"; +import { rejectDuplicates } from "../contract/requests.js"; import { ENDPOINTS, type EnvironmentsRestContext } from "./endpoints.js"; import type { NestedPlan } from "./nested.js"; import type { DeploymentProtectionRuleConfig, EnvironmentConfig } from "./schema.js"; @@ -111,23 +112,17 @@ async function listProtectionRuleApps( } export function validateProtectionRules( + section: SectionMeta, env: EnvironmentConfig, entries: readonly DeploymentProtectionRuleConfig[], ): void { - const seen = new Set(); - const duplicates = new Set(); - for (const rule of entries) { - if (seen.has(rule.app)) { - duplicates.add(rule.app); - } - seen.add(rule.app); - } - if (duplicates.size > 0) { - throw new Error( - `environments: the "${env.name}" entry declares the deployment protection rule App${duplicates.size === 1 ? "" : "s"} ` + - `${[...duplicates].map((app) => `"${app}"`).join(", ")} more than once. Keep exactly one entry per App`, - ); - } + rejectDuplicates( + section, + entries, + (rule) => rule.app, + (rule) => rule.app, + `deployment protection rule App of the "${env.name}" environment`, + ); } /** Every missing slug resolves from one Apps read before the first POST leaves, so an unlisted slug fails before any rule is half-enabled. */ diff --git a/src/sections/shared/repo-secrets.ts b/src/sections/shared/repo-secrets.ts index e2d6e2be..902e9bb0 100644 --- a/src/sections/shared/repo-secrets.ts +++ b/src/sections/shared/repo-secrets.ts @@ -3,7 +3,7 @@ * same four endpoints under a different path segment and differ only in PAT resource, noun, and (Codespaces) * the grade GitHub gates the reads at, so each section module is ONE repoSecretsSection() call. * - * environments section -> consumes ./secrets-engine.ts directly, with nested scopes + * environments section -> plans its nested secrets through ./secrets-engine.ts too, one scope per environment * .github/scripts/changed-sections.ts -> derives this file's smoke fan-out from the import graph */ @@ -93,6 +93,7 @@ type RepoSecretsEndpoints

= { readonly route: `GET /repos/{owner}/{repo}/${P}/secrets/public-key`; readonly statuses: { readonly 200: string }; readonly accessGrade?: "write"; + readonly phase: "execution"; }; readonly put: { readonly route: `PUT /repos/{owner}/{repo}/${P}/secrets/{secret_name}`; @@ -205,10 +206,12 @@ export function repoSecretsSection(family: { // A fine-grained token conceals a denied list as 404; reading it as "no secrets" would be wrong, so it is a denial. primaryRead: { notFound: "denied" }, }, + // Read inside the first sealed PUT's payload thunk (the engine's rule), so check mode never issues it. publicKey: { route: `GET /repos/{owner}/{repo}/${pathSegment}/secrets/public-key`, statuses: { 200: "the sealing public key" }, ...readGrade, + phase: "execution", }, put: { route: `PUT /repos/{owner}/{repo}/${pathSegment}/secrets/{secret_name}`, @@ -239,7 +242,7 @@ export function repoSecretsSection(family: { LIVE_SECRET_NAMES, await ctx.read.list.listAllEnveloped("secrets"), ), - publicKey: (describe) => ctx.read.publicKey.call({ describe }), + publicKey: (exec, describe) => ctx.read.publicKey.call(exec, { describe }), publicKeyEndpoint: wide.publicKey, put: (write) => ({ role: "put", diff --git a/src/sections/shared/repo-variables.ts b/src/sections/shared/repo-variables.ts index cd31981a..9b53259f 100644 --- a/src/sections/shared/repo-variables.ts +++ b/src/sections/shared/repo-variables.ts @@ -3,7 +3,7 @@ * a different path segment and differ only in PAT resource and noun, so each section module is ONE * repoVariablesSection() call. * - * environments section -> consumes ./variables-engine.ts directly, with nested scopes + * environments section -> plans its nested variables through ./variables-engine.ts too, one scope per environment * .github/scripts/changed-sections.ts -> derives this file's smoke fan-out from the import graph */ @@ -28,12 +28,12 @@ import type { SectionPlan, SnapshotContext, } from "../contract/plan.js"; -import { rejectDuplicates } from "../contract/requests.js"; import { knobbed } from "./schema-helpers.js"; import { knobbedSnapshot, projectOntoSchema } from "./snapshot-helpers.js"; import { LiveVariable, planVariables, + rejectDuplicateVariableNames, type VariableEntry, type VariablesPlanScope, variableKey, @@ -186,13 +186,7 @@ export function repoVariablesSection(family: { const plan: SharedPlan = async (ctx, declared) => { const defaultPolicy = defaultUndeclaredPolicy(section); const { policy, entries } = undeclaredPolicy(declared, defaultPolicy); - // Variable names are case-insensitive on GitHub, so two entries differing only in case name one variable. - rejectDuplicates( - section, - entries, - (variable) => variableKey(variable.name), - (variable) => variable.name, - ); + rejectDuplicateVariableNames(section, entries); // Built where the routes are known, so params typecheck ({name} on update/remove). type Op = PlannedOp; const scope: VariablesPlanScope< @@ -214,6 +208,7 @@ export function repoVariablesSection(family: { payload: write.payload, drift: write.drift, change: write.change, + describe: write.describe, }), update: (write) => ({ role: "update", @@ -221,12 +216,14 @@ export function repoVariablesSection(family: { payload: write.payload, drift: write.drift, change: write.change, + describe: write.describe, }), remove: (deletion) => ({ role: "remove", params: { name: deletion.name }, drift: deletion.drift, change: deletion.change, + describe: deletion.describe, }), }; return planVariables(section, scope, { entries, policy, defaultPolicy }); diff --git a/src/sections/shared/secrets-engine.ts b/src/sections/shared/secrets-engine.ts index b7fefb28..ea5ae968 100644 --- a/src/sections/shared/secrets-engine.ts +++ b/src/sections/shared/secrets-engine.ts @@ -1,10 +1,15 @@ -/** Existence reconciliation over route-free scopes: values are never read back, and every declared secret is re-sealed on each apply. */ +/** + * Existence reconciliation over route-free scopes: values are never read back, and every declared secret + * is re-sealed on each apply. The four repo families (./repo-secrets.ts) and the environments section's + * nested secrets (../environments/nested.ts) plan through it. + */ import { z } from "zod"; import type { UndeclaredPolicy } from "../../types.js"; import { type EndpointDecl, endpointPath } from "../contract/endpoints.js"; import { liveByIdentity } from "../contract/live.js"; import { + cannotVerifyNote, type DeclaredSecretValue, type SectionMeta, secretValuesOf, @@ -36,11 +41,16 @@ export type SealedSecretPayload = { type AnyPlannedOp = SectionPlan["ops"][number]; -interface SecretsScopeProse { +/** The words a scope supplies; a nested scope names its home so the lines say where the secret lives. */ +export interface SecretsScopeProse { /** The drift-line prefix, e.g. "actions_secrets" or "environments[prod].secrets". */ label: string; /** The noun for notes ("Actions secret"; a nested scope says "prod environment secret"). */ noun: string; + /** Where the secrets live when "the repo" understates it ("the environment"). */ + where?: string; + /** Appended to change and describe lines (` in environment "prod"`). */ + suffix?: string; } /** The payload thunk resolves and seals only when executed, so the plan carries the `$NAME` reference and nothing derived from a value. */ @@ -49,8 +59,8 @@ interface SealedSecretWrite { readonly name: string; /** What the write is doing, in settings-file terms, for its error prose. */ readonly describe: string; - /** Seals the resolved plaintext against the scope's sealing key at execution time. */ - readonly payload: (exec: ExecTools) => SealedSecretPayload; + /** Reads the scope's sealing key (once per plan) and seals the resolved plaintext, at execution time. */ + readonly payload: (exec: ExecTools) => Promise; /** The missing-secret line, or empty when the name exists (the PUT recurs by declaration). */ readonly drift: readonly string[]; readonly change: string; @@ -70,8 +80,11 @@ export interface SecretsPlanScope Promise; - /** GET the {key_id, key} sealing key for this scope. */ - readonly publicKey: (describe: string) => Promise; + /** + * GET the {key_id, key} sealing key for this scope, at EXECUTION time: an environment's key exists + * only once the PUT that creates the environment has landed, so the read rides the first PUT's thunk. + */ + readonly publicKey: (exec: ExecTools, describe: string) => Promise; /** The declaration behind `publicKey`, named in the prose of a key the endpoint cannot supply. */ readonly publicKeyEndpoint: EndpointDecl; /** The planned sealed PUT; function-valued so a builder demanding an unsupplied facet fails. */ @@ -84,11 +97,7 @@ export function secretKey(name: string): string { return name.toUpperCase(); } -/** - * Each value is labelled with its entry's secret NAME so a validation error can point at it. DEFENSIVE by - * contract: a malformed container returns [] instead of throwing, so the actionable error always comes - * from shape validation, never a TypeError here. - */ +/** Each value is labelled with its entry's secret NAME so a validation error can point at it. */ export function listSecretValues(declared: unknown): DeclaredSecretValue[] { return secretValuesOf(declared, (entry) => { if (typeof entry.value !== "string") { @@ -102,16 +111,21 @@ export function listSecretValues(declared: unknown): DeclaredSecretValue[] { }); } -/** GitHub folds two names equal uppercased into one secret, so the last write would silently win on every run. */ +/** + * GitHub folds two names equal uppercased into one secret, so the last write would silently win on + * every run. `what` names the resource when "

entry" understates it (a nested scope's). + */ export function rejectDuplicateSecretNames( section: SectionMeta, entries: readonly SecretEntry[], + what?: string, ): void { rejectDuplicates( section, entries, (entry) => secretKey(entry.name), (entry) => entry.name, + what, ); } @@ -179,19 +193,19 @@ export function parseSealingKey( }; } -function missingSecretDrift(scope: SecretsScopeProse, name: string): string { - return `${scope.label}[${name}]: missing - declared in the settings file but not on the repo; apply will create it`; -} - /** ONE note per scope (the LFS precedent): values are unverifiable by design. */ -function cannotVerifyNote(scope: SecretsScopeProse): string { - return `${scope.noun} values cannot be read back from GitHub, so check mode verifies only that each declared secret exists; apply re-seals and rewrites every declared value on each run`; +function valuesUnverifiableNote(scope: SecretsScopeProse): string { + return cannotVerifyNote(scope.label, { + why: `${scope.noun} values cannot be read back from GitHub`, + what: "them, only that each declared secret exists", + reasserts: "re-seals and rewrites every declared value", + }); } function undeclaredSecretNote(scope: SecretsScopeProse, liveName: string): string { return undeclaredNote({ subject: `${scope.noun} "${liveName}"`, - state: "exists on the repo but is not declared", + state: `exists on ${scope.where ?? "the repo"} but is not declared`, action: "DELETE it (a deleted secret's value is unrecoverable)", }); } @@ -210,7 +224,7 @@ function undeclaredSecretDrift( /** Uppercase key -> the name as listed (normalizing keeps a differently-cased mock harmless). */ function liveSecretsByKey( section: SectionMeta, - scope: SecretsScopeProse, + scope: SecretsPlanScope, live: readonly LiveSecretName[], ): Map { const byKey = liveByIdentity( @@ -237,33 +251,43 @@ export async function planSecrets> { const { entries, policy, defaultPolicy } = opts; + const suffix = scope.suffix ?? ""; const plan: SectionPlan = { ops: [], notes: [], drift: [] }; const liveByKey = liveSecretsByKey(section, scope, await scope.list()); const declaredKeys = new Set(entries.map((entry) => secretKey(entry.name))); - if (entries.length > 0) { - const sealingKey = parseSealingKey( - section, - scope, - scope.publicKeyEndpoint, - await scope.publicKey(`reading the ${scope.label} sealing key`), + // Read once per scope, by the first payload thunk that runs; the token it demands is the one the thunk received. + let sealingKey: Promise | undefined; + const readSealingKey = (exec: ExecTools): Promise => { + sealingKey ??= scope + .publicKey(exec, `reading the ${scope.label} sealing key`) + .then((body) => parseSealingKey(section, scope, scope.publicKeyEndpoint, body)); + return sealingKey; + }; + for (const entry of entries) { + const name = secretKey(entry.name); + // The listing decides the verb; the executor does not surface the PUT's 201/204. + const exists = liveByKey.has(name); + plan.ops.push( + scope.put({ + name, + describe: `writing secret "${name}"${suffix}`, + payload: async (exec) => { + const plaintext = exec.resolveSecret(entry.value); + return (await readSealingKey(exec)).seal(plaintext); + }, + drift: exists + ? [] + : [ + `${scope.label}[${name}]: missing - declared in the settings file but not on ${scope.where ?? "the repo"}; apply will create it`, + ], + change: `${exists ? "updated" : "created"} secret "${name}"${suffix}`, + }), ); - for (const entry of entries) { - const name = secretKey(entry.name); - // The listing decides the verb; the executor does not surface the PUT's 201/204. - const exists = liveByKey.has(name); - plan.ops.push( - scope.put({ - name, - describe: `writing secret "${name}"`, - payload: (exec) => sealingKey.seal(exec.resolveSecret(entry.value)), - drift: exists ? [] : [missingSecretDrift(scope, name)], - change: `${exists ? "updated" : "created"} secret "${name}"`, - }), - ); - } - plan.notes.push(cannotVerifyNote(scope)); + } + if (entries.length > 0) { + plan.notes.push(valuesUnverifiableNote(scope)); } for (const [key, liveName] of liveByKey) { @@ -276,9 +300,9 @@ export async function planSecrets; type AnyPlannedOp = SectionPlan["ops"][number]; -type PlainPayload = Exclude, (exec: ExecTools) => unknown>; +type PlainPayload = PlainData; /** The index signature types the passthrough fields as plain data, so spreading them into a body needs no cast. */ export interface VariableEntry { @@ -26,17 +36,24 @@ export interface VariableEntry { readonly [key: string]: PlainPayload | undefined; } +/** The words a scope supplies; a nested scope names its home so the lines say where the variable lives. */ interface VariablesScopeProse { /** The drift-line prefix, e.g. "actions_variables" or "environments[prod].variables". */ label: string; /** The noun for notes and change lines ("Actions variable"). */ noun: string; + /** Where the variables live when "the repo" understates it ("the environment"). */ + where?: string; + /** Appended to change and describe lines (` in environment "prod"`). */ + suffix?: string; } interface VariableCreate { readonly payload: PlainPayload; readonly drift: readonly [string]; readonly change: string; + /** What the write is doing, in settings-file terms, for its error prose. */ + readonly describe: string; } interface VariableUpdate { @@ -45,6 +62,7 @@ interface VariableUpdate { readonly payload: PlainPayload; readonly drift: readonly [string, ...string[]]; readonly change: string; + readonly describe: string; } interface VariableDeletion { @@ -52,6 +70,7 @@ interface VariableDeletion { readonly name: string; readonly drift: readonly [string]; readonly change: string; + readonly describe: string; } /** The type parameters are the section's exact PlannedOp arms, so a wrong role fails to compile. */ @@ -68,18 +87,25 @@ export interface VariablesPlanScope< readonly remove: (deletion: VariableDeletion) => Remove; } -function missingVariableDrift(label: string): string { - return `${label}: missing - declared in the settings file but not on the repo; apply will create it`; -} - -function valueDriftLine(label: string, declared: string, live: string): string { - return `${label}.value: declared ${JSON.stringify(declared)} != live ${JSON.stringify(live)}; apply will set the declared value`; +/** Variable names are case-insensitive on GitHub, so two entries differing only in case name one variable. */ +export function rejectDuplicateVariableNames( + section: SectionMeta, + entries: readonly VariableEntry[], + what?: string, +): void { + rejectDuplicates( + section, + entries, + (variable) => variableKey(variable.name), + (variable) => variable.name, + what, + ); } function undeclaredVariableNote(scope: VariablesScopeProse, liveName: string): string { return undeclaredNote({ subject: `${scope.noun} "${liveName}"`, - state: "exists on the repo but is not declared", + state: `exists on ${scope.where ?? "the repo"} but is not declared`, action: "DELETE it", }); } @@ -113,6 +139,7 @@ export async function planVariables< }, ): Promise> { const { entries, policy, defaultPolicy } = opts; + const suffix = scope.suffix ?? ""; const plan: SectionPlan = { ops: [], notes: [], drift: [] }; const liveByKey = liveByIdentity( @@ -131,9 +158,12 @@ export async function planVariables< if (!existing) { plan.ops.push( scope.create({ - payload: { name: variable.name, value: variable.value, ...extraKeys }, - drift: [missingVariableDrift(label)], - change: `created ${scope.noun} "${variable.name}"`, + payload: plainData({ name: variable.name, value: variable.value, ...extraKeys }), + drift: [ + `${label}: missing - declared in the settings file but not on ${scope.where ?? "the repo"}; apply will create it`, + ], + change: `created ${scope.noun} "${variable.name}"${suffix}`, + describe: `creating ${scope.noun} "${variable.name}"${suffix}`, }), ); continue; @@ -142,7 +172,13 @@ export async function planVariables< // GitHub stores the name uppercased whatever casing the file uses, so the live name never drifts; only the value and passthrough fields can. const [first, ...rest] = [ ...(existing.value !== variable.value - ? [valueDriftLine(label, variable.value, existing.value)] + ? [ + valueDrift( + `${label}.value`, + JSON.stringify(variable.value), + JSON.stringify(existing.value), + ), + ] : []), ...subsetDiff(extraKeys, existing, label), ]; @@ -156,9 +192,10 @@ export async function planVariables< plan.ops.push( scope.update({ liveName: existing.name, - payload: { value: variable.value, ...extraKeys }, + payload: plainData({ value: variable.value, ...extraKeys }), drift: [first, ...rest], - change: `updated ${scope.noun} "${variable.name}"`, + change: `updated ${scope.noun} "${variable.name}"${suffix}`, + describe: `updating ${scope.noun} "${variable.name}"${suffix}`, }), ); } @@ -174,7 +211,8 @@ export async function planVariables< scope.remove({ name: variable.name, drift: [undeclaredVariableDrift(scope, defaultPolicy, variable.name)], - change: `DELETED undeclared ${scope.noun} "${variable.name}"`, + change: `DELETED undeclared ${scope.noun} "${variable.name}"${suffix}`, + describe: `deleting undeclared ${scope.noun} "${variable.name}"${suffix}`, }), ); } diff --git a/test/engine/orchestrate.test.ts b/test/engine/orchestrate.test.ts index e6500b73..45c3969c 100644 --- a/test/engine/orchestrate.test.ts +++ b/test/engine/orchestrate.test.ts @@ -72,7 +72,7 @@ describe("runForRepo", () => { 'branches: replacing protection for branch "main" failed - PUT /repos/o/r/branches/main/protection: 404 Branch not found. ' + "The declared branch does not exist on the repo, so its protection cannot be applied; create the branch, or remove it from the settings file"; const deniedPut = - 'the token was denied replacing protection for branch "main" failed - PUT /repos/o/r/branches/main/protection: 404 Not Found ' + + 'the token was denied PUT /repos/o/r/branches/main/protection (replacing protection for branch "main"): 404 Not Found ' + '(a 404 here can also mean the resource does not exist). To fix, grant "Administration" (read and write) under the PAT\'s Repository permissions'; test.each([ ["warn", "Branch not found", "failed", ["branches", "failed"], `error: ${missingBranch}`], diff --git a/test/sections/contract.test.ts b/test/sections/contract.test.ts index 4a544c13..cc14ac0a 100644 --- a/test/sections/contract.test.ts +++ b/test/sections/contract.test.ts @@ -431,7 +431,7 @@ describe("throwFor context enrichment", () => { } expect(thrown).toBeInstanceOf(PermissionDenied); expect((thrown as PermissionDenied).detail).toBe( - 'the token was denied creating ruleset "quality" failed - POST /repos/o/r/rulesets: 403 Resource not accessible. To fix, grant "Administration" (read and write) under the PAT\'s Repository permissions', + 'the token was denied POST /repos/o/r/rulesets (creating ruleset "quality"): 403 Resource not accessible. To fix, grant "Administration" (read and write) under the PAT\'s Repository permissions', ); }); @@ -449,7 +449,7 @@ describe("throwFor context enrichment", () => { { status: 404, message: "Not Found" }, { denied: - 'the token was denied protecting "x" failed - PUT /repos/o/r/branches/x/protection: 404 Not Found (a 404 here can also mean the resource does not exist). To fix, grant "Administration" (read and write) under the PAT\'s Repository permissions', + 'the token was denied PUT /repos/o/r/branches/x/protection (protecting "x"): 404 Not Found (a 404 here can also mean the resource does not exist). To fix, grant "Administration" (read and write) under the PAT\'s Repository permissions', }, ], [ @@ -457,7 +457,7 @@ describe("throwFor context enrichment", () => { { status: 404, message: "Branch not found on fork" }, { denied: - 'the token was denied protecting "x" failed - PUT /repos/o/r/branches/x/protection: 404 Branch not found on fork ' + + 'the token was denied PUT /repos/o/r/branches/x/protection (protecting "x"): 404 Branch not found on fork ' + '(a 404 here can also mean the resource does not exist). To fix, grant "Administration" (read and write) under the PAT\'s Repository permissions', }, ], @@ -466,7 +466,7 @@ describe("throwFor context enrichment", () => { { status: 403, message: "Branch not found" }, { denied: - 'the token was denied protecting "x" failed - PUT /repos/o/r/branches/x/protection: 403 Branch not found. To fix, grant "Administration" (read and write) under the PAT\'s Repository permissions', + 'the token was denied PUT /repos/o/r/branches/x/protection (protecting "x"): 403 Branch not found. To fix, grant "Administration" (read and write) under the PAT\'s Repository permissions', }, ], ] as const)( @@ -1222,7 +1222,7 @@ describe("a marked request's failure is rebuilt on the engine's side of the clie wire: "REST, the client answers a plain 403", run: () => rest(answering(403), true), thrown: - `actions: the token was denied arming the setup failed - PATCH /repos/o/r/code-quality/setup: 403 ${SECRET_RESPONSE_WITHHELD}. ` + + `actions: the token was denied PATCH /repos/o/r/code-quality/setup (arming the setup): 403 ${SECRET_RESPONSE_WITHHELD}. ` + `To fix, grant "Administration" (read and write) under the PAT's Repository permissions; the "oidc_customization_sub" key alone instead needs "Actions" (read and write)`, }, // The controls: unmarked, the same answers render as the client gave them. diff --git a/test/sections/graphql-contract.test.ts b/test/sections/graphql-contract.test.ts index c5f4d855..d30169a5 100644 --- a/test/sections/graphql-contract.test.ts +++ b/test/sections/graphql-contract.test.ts @@ -77,7 +77,7 @@ describe("callGraphql", () => { } expect(thrown).toBeInstanceOf(PermissionDenied); expect((thrown as PermissionDenied).detail).toBe( - 'the token was denied reading repository toggles failed - GRAPHQL RepoToggles: 403 Resource not accessible. To fix, grant "Administration" (read and write) under the PAT\'s Repository permissions', + 'the token was denied GRAPHQL RepoToggles (reading repository toggles): 403 Resource not accessible. To fix, grant "Administration" (read and write) under the PAT\'s Repository permissions', ); }); diff --git a/test/sections/registry.test.ts b/test/sections/registry.test.ts index 978d9a20..fe0db5b1 100644 --- a/test/sections/registry.test.ts +++ b/test/sections/registry.test.ts @@ -1274,7 +1274,7 @@ describe("handler contracts", () => { ); }); - test("the execution-phase reads are exactly the thunk-issued lookups: the branches ids and the environment sealing key", () => { + test("the execution-phase reads are exactly the thunk-issued lookups: the branches ids and every sealing key", () => { // Check mode never issues these (the e2e mock fails a check-mode arrival), so a read that moves in or out of this list changes what a // read-only token meets in check mode. const byLateReads = SECTIONS.flatMap((section) => { @@ -1286,6 +1286,10 @@ describe("handler contracts", () => { expect(Object.fromEntries(byLateReads)).toEqual({ branches: ["appLookup", "repoLookup", "actorUser", "actorTeam"], environments: ["secretsPublicKey", "listProtectionRuleApps"], + actions_secrets: ["publicKey"], + dependabot_secrets: ["publicKey"], + codespaces_secrets: ["publicKey"], + agents_secrets: ["publicKey"], }); }); }); diff --git a/test/sections/secrets-engine.test.ts b/test/sections/secrets-engine.test.ts index d70dbc63..33b25c98 100644 --- a/test/sections/secrets-engine.test.ts +++ b/test/sections/secrets-engine.test.ts @@ -55,7 +55,7 @@ function fabricatedPlanScope(live: string[], reads: string[]): SecretsPlanScope< return live.map((name) => ({ name })); }, publicKeyEndpoint: PUBLIC_KEY_ENDPOINT, - publicKey: async (describe) => { + publicKey: async (_exec, describe) => { reads.push(describe); return KEY_DATA; }, @@ -156,7 +156,7 @@ describe("secretKey and duplicates", () => { }); describe("planSecrets and the execution-time resolver", () => { - test("each PUT's thunk seals its OWN entry's resolved value, uppercasing the name; planning resolves nothing", async () => { + test("each PUT's thunk seals its OWN entry's resolved value, uppercasing the name; planning reads the list alone and the key once at execution", async () => { await mockSodiumReady(); const reads: string[] = []; const plan = await planSecrets(section, fabricatedPlanScope([], reads), { @@ -167,8 +167,8 @@ describe("planSecrets and the execution-time resolver", () => { policy: "keep", defaultPolicy: "keep", }); - // The sealing key is read once at plan time, after the list. - expect(reads).toEqual(["list", "reading the actions_secrets sealing key"]); + // The sealing key is an execution-time read: check mode never issues it. + expect(reads).toEqual(["list"]); expect(plan.ops.map((op) => op.params)).toEqual([ { secret_name: "FIRST" }, { secret_name: "SECOND" }, @@ -180,9 +180,15 @@ describe("planSecrets and the execution-time resolver", () => { return resolver({ $ONE: "plain-1", $TWO: "plain-2" })(reference); }, }; - const sealed = plan.ops.map((op) => - typeof op.payload === "function" ? (op.payload(exec) as SealedSecretPayload) : null, + const sealed = await Promise.all( + plan.ops.map((op) => + typeof op.payload === "function" + ? (op.payload(exec) as Promise) + : Promise.resolve(null), + ), ); + // Both thunks share ONE key read. + expect(reads).toEqual(["list", "reading the actions_secrets sealing key"]); expect(lookups).toEqual(["$ONE", "$TWO"]); expect(sealed.map((p) => p?.key_id)).toEqual(["key-1", "key-1"]); expect(sealed.map((p) => unsealSecretValue(p?.encrypted_value ?? ""))).toEqual([