refactor(sections)!: environments plan through the secrets and variables engines; the sealing key is read at apply

The secrets engine reads the scope's sealing key from the first sealed PUT's thunk (once per scope), so the four repo families declare the key endpoint execution-phase and environments' nested secrets share the engine; the variables engine serves environments' nested variables too. Both engines take the scope's home noun and change-line suffix and render through the shared prose helpers; the variables engine proves every payload plain. The per-list duplicate rejecters in environments give way to rejectDuplicates with a resource noun.

BREAKING CHANGE: check mode no longer reads the sealing key (a malformed key fails the first PUT at apply); the environment secret and variable lines use the engines' wording. docs/upgrading/v2-to-v3.md sections 10-11.

Author: Vivswan

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).
+Seventeen 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,8 @@ 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) |
+| 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 16](#16-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 17](#17-environment-secrets-and-variables-plan-through-the-shared-engines) |
 
 ## 1. The defaults-file fallback
 
@@ -241,6 +243,27 @@ 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. 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`.
+
+## 17. Environment secrets and variables plan through the shared engines
+
+Their lines are the engines' lines now:
+
+| Line | v2 | v3 |
+|---|---|---|
+| A kept undeclared variable | `variable "X" exists on environment "prod" but is not declared ...` | `variable "X" exists on the environment but is not declared ...` |
+| 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, 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.

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<typeof actionsSecretsSection.plan>[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([]);
   });
 });
 

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, {

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 {

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 @@ function livePolicyName(policy: LiveBranchPolicy, envName: string): string {
  * 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<string>();
-  const duplicates = new Set<string>();
-  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(

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([
@@ -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([]);
   });
@@ -308,7 +308,7 @@ describe("environments variables undeclared policy", () => {
       { name: "prod", variables: { _undeclared: "keep", entries: [] } },
     ]);
     expect(result.notes).toEqual([
-      'variable "LEGACY" exists on environment "prod" 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',
+      'variable "LEGACY" 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',
     ]);
     expect(api.mutations()).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([]);
   });
@@ -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",
     ]);
   });
 });