diff --git a/COVERAGE.md b/COVERAGE.md index 835a12c..1545562 100644 --- a/COVERAGE.md +++ b/COVERAGE.md @@ -22,7 +22,7 @@ The tenet: this action can control everything about a repository and nothing abo | [Rulesets](https://docs.github.com/en/rest/repos/rules) (branch, tag, and push targets; all rule types, conditions, bypass_actors) | `rulesets` | GET/POST/PUT/DELETE /repos/{owner}/{repo}/rulesets: upsert-by-name, full-payload PUT, verbatim passthrough except ref-name prefixing (staging -> refs/heads/staging, ~DEFAULT_BRANCH passes through). New rule types/bypass fields/condition types GitHub ships work day one. Undeclared rulesets kept by default (notes only); the wrapped `undeclared: delete` form deletes them. Org-sourced rulesets filtered out via source_type. | | [Merge queue](https://docs.github.com/en/rest/repos/rules) | `rulesets` | Configured as the merge_queue rule type inside a branch ruleset; passes through verbatim like every other rule type. No dedicated endpoint exists; rulesets ARE the API for merge queue. | | [Tag protection (modern)](https://docs.github.com/en/rest/repos/rules) | `rulesets` | target: tag rulesets cover everything the retired legacy tag-protection API did (legacy API itself is out of scope, removed by GitHub). | -| [Classic branch protection](https://docs.github.com/en/rest/branches/branch-protection) (literal branches, wildcard patterns, force-push bypass actors, required deployments) | `branches` | PUT /repos/{owner}/{repo}/branches/{branch}/protection passthrough; the four required keys are null-filled; protection: null issues DELETE (deleteBranchProtectionRule for a wildcard entry). Check mode flattens the GET shape ({enabled} wrappers, actor objects -> login/slug strings, *_url dropped) to compare like with like (src/sections/branches/index.ts flattenProtection). Every write is drift-gated: the PUT is planned when a declared key diverges or when the replacing PUT would remove a live setting the file omits, and a converged branch plans nothing. Three routed keys never ride the PUT: required_signatures (the PUT silently drops it) goes through its own POST/DELETE .../protection/required_signatures sub-endpoint when it drifts and again after any planned PUT (declared true POSTs, false DELETEs, undeclared untouched; a GET that omits the field reads as false; GitHub does not document whether the PUT preserves an existing requirement, so declare the toggle on any branch that carries one), while force_push_bypassers and required_deployments are REST-invisible entirely and ride ONE updateBranchProtectionRule GraphQL mutation on the same terms. force_push_bypassers is a list of actor strings - a bare login is a user, "org/team-slug" a team, "app/slug" a GitHub App - resolved to node ids at plan time, so a misspelled actor fails check mode and apply alike before any write: users and teams through GraphQL (the BranchProtectionActorUser and BranchProtectionActorTeam lookups, new-format ids), Apps through the public GET /apps/{app_slug} REST lookup, whose node_id can still be the legacy format for old Apps (GitHub accepts it with a deprecation warning). required_deployments takes {environments: [...]}; declaring null turns the requirement off, an absent key leaves the live state untouched. CAVEAT (verified live): GitHub SILENTLY drops required-deployment environment names that do not exist while the mutation succeeds, so apply verifies the mutation's read-back and fails loudly naming any dropped name - the environments section runs before branches, so environments declared in the same file exist by then. WILDCARD entries (a name containing `*`, `?`, or `[`, e.g. release/*) are invisible to the REST endpoints (their docs point wildcard use at the GraphQL API), so they reconcile entirely through the createBranchProtectionRule/updateBranchProtectionRule/deleteBranchProtectionRule mutations (a create addresses the repository node id the BranchProtectionRepository query fetches) against the Repository.branchProtectionRules read; a wildcard protection accepts exactly the keys this section can round-trip through the GraphQL rule surface - enforce_admins (its isAdminEnforced twin live-verified bidirectionally against the REST view of the same rule), required_linear_history, allow_force_pushes, allow_deletions, block_creations, required_conversation_resolution, lock_branch, allow_fork_syncing, required_signatures, required_status_checks (strict, contexts), required_pull_request_reviews (required_approving_review_count, require_code_owner_reviews, dismiss_stale_reviews, require_last_push_approval), force_push_bypassers, required_deployments - and rejects anything else upfront (the actor-list controls have GraphQL fields but their REST vocabulary of database ids does not round-trip through node-id-based reads, so this section does not manage them on wildcard rules), pointing at rulesets as the recommended path for new configuration. The rules query fires only when an entry has a wildcard name or declares a GraphQL-routed key (a pure-REST declaration issues no GraphQL request), which also scopes the undeclared-rule NOTE: only a run whose declaration fires the query reports a live wildcard rule the file does not declare, as a note and never a deletion; wildcard updates have PATCH semantics (an omitted key keeps its live value, unlike the literal PUT's replace). Actor and environment names compare case-insensitively (GitHub canonicalizes them), and the routed lists reject duplicate names upfront. | +| [Classic branch protection](https://docs.github.com/en/rest/branches/branch-protection) (literal branches, wildcard patterns, force-push bypass actors, required deployments) | `branches` | PUT /repos/{owner}/{repo}/branches/{branch}/protection passthrough; the four required keys are null-filled; protection: null issues DELETE (deleteBranchProtectionRule for a wildcard entry). Check mode flattens the GET shape ({enabled} wrappers, actor objects -> login/slug strings, *_url dropped) to compare like with like (src/sections/branches/index.ts flattenProtection). Every write is drift-gated: the PUT is planned when a declared key diverges or when the replacing PUT would remove a live setting the file omits, and a converged branch plans nothing. Three routed keys never ride the PUT: required_signatures (the PUT silently drops it) goes through its own POST/DELETE .../protection/required_signatures sub-endpoint when it drifts and again after any planned PUT (declared true POSTs, false DELETEs, undeclared untouched; a GET that omits the field reads as false; GitHub does not document whether the PUT preserves an existing requirement, so declare the toggle on any branch that carries one), while force_push_bypassers and required_deployments are REST-invisible entirely and ride ONE updateBranchProtectionRule GraphQL mutation on the same terms. force_push_bypassers is a list of actor strings - a bare login is a user, "org/team-slug" a team, "app/slug" a GitHub App - resolved to node ids when apply executes, ahead of the section's first write, so a misspelled actor fails before any write lands (check mode issues no lookup and reports the drift): users and teams through GraphQL (the BranchProtectionActorUser and BranchProtectionActorTeam lookups, new-format ids), Apps through the public GET /apps/{app_slug} REST lookup, whose node_id can still be the legacy format for old Apps (GitHub accepts it with a deprecation warning). required_deployments takes {environments: [...]}; declaring null turns the requirement off, an absent key leaves the live state untouched. CAVEAT (verified live): GitHub SILENTLY drops required-deployment environment names that do not exist while the mutation succeeds, so apply verifies the mutation's read-back and fails loudly naming any dropped name - the environments section runs before branches, so environments declared in the same file exist by then. WILDCARD entries (a name containing `*`, `?`, or `[`, e.g. release/*) are invisible to the REST endpoints (their docs point wildcard use at the GraphQL API), so they reconcile entirely through the createBranchProtectionRule/updateBranchProtectionRule/deleteBranchProtectionRule mutations (a create addresses the repository node id the BranchProtectionRepository query fetches) against the Repository.branchProtectionRules read; a wildcard protection accepts exactly the keys this section can round-trip through the GraphQL rule surface - enforce_admins (its isAdminEnforced twin live-verified bidirectionally against the REST view of the same rule), required_linear_history, allow_force_pushes, allow_deletions, block_creations, required_conversation_resolution, lock_branch, allow_fork_syncing, required_signatures, required_status_checks (strict, contexts), required_pull_request_reviews (required_approving_review_count, require_code_owner_reviews, dismiss_stale_reviews, require_last_push_approval), force_push_bypassers, required_deployments - and rejects anything else upfront (the actor-list controls have GraphQL fields but their REST vocabulary of database ids does not round-trip through node-id-based reads, so this section does not manage them on wildcard rules), pointing at rulesets as the recommended path for new configuration. The rules query fires only when an entry has a wildcard name or declares a GraphQL-routed key (a pure-REST declaration issues no GraphQL request), which also scopes the undeclared-rule NOTE: only a run whose declaration fires the query reports a live wildcard rule the file does not declare, as a note and never a deletion; wildcard updates have PATCH semantics (an omitted key keeps its live value, unlike the literal PUT's replace). Actor and environment names compare case-insensitively (GitHub canonicalizes them), and the routed lists reject duplicate names upfront. | | [Environments](https://docs.github.com/en/rest/deployments/environments) (wait_timer, reviewers, prevent_self_review, deployment_branch_policy protected_branches/custom_branch_policies flags), their [Actions variables](https://docs.github.com/en/rest/actions/variables), their [Actions secrets](https://docs.github.com/en/rest/actions/secrets), their [deployment branch policies](https://docs.github.com/en/rest/deployments/branch-policies) (custom patterns), and their [custom deployment protection rules](https://docs.github.com/en/rest/deployments/protection-rules) (GitHub App gates) | `environments` | PUT /repos/{owner}/{repo}/environments/{name} passthrough; check mode flattens GET's protection_rules[] back into the PUT shape. A declared per-environment `variables` key reconciles that environment's Actions variables AFTER the PUT, through GET/POST /repos/{owner}/{repo}/environments/{name}/variables and PATCH/DELETE .../variables/{name}: create missing, update divergent values, and delete undeclared ones by default (the wrapped `undeclared: keep` form keeps them as notes); names match case-insensitively, values are plain text by design. A declared per-environment `secrets` key reconciles that environment's Actions secrets the same way the shipped secret sections do - GET .../environments/{name}/secrets (names + timestamps), GET .../secrets/public-key, sealed PUT/DELETE .../secrets/{secret_name} - one sealing scope per environment, so same-named secrets in sibling environments resolve independently; undeclared secrets within a declared key are KEPT by default (values unrecoverable; `undeclared: delete` opts in). A declared per-environment `deployment_branch_policies` key reconciles that environment's custom branch-policy patterns through GET/POST .../environments/{name}/deployment-branch-policies and DELETE .../deployment-branch-policies/{branch_policy_id}: create missing patterns, delete undeclared ones by default (`undeclared: keep` softens to notes), and replace a matching pattern whose type differs (type is immutable upstream, so the change is delete + recreate; the upstream PUT is deliberately unused - its body is the name alone, and the name is the pattern's identity, so it can never help reconciliation). Declaring the key requires the singular `deployment_branch_policy` sibling with custom_branch_policies: true (rejected upfront otherwise, since the pattern writes would 404 only after the environment PUT landed), and its endpoints sit outside the Environments PAT permission: the list read needs Actions read, the writes need Administration write. A declared per-environment `deployment_protection_rules` key reconciles that environment's custom deployment protection rules (GitHub App gates) through GET/POST .../environments/{name}/deployment_protection_rules (the list documents NO pagination, so it is fetched in one call; the POST body is {integration_id}) and DELETE .../deployment_protection_rules/{protection_rule_id}: enable/disable ONLY, since GitHub offers no update call. Rules are declared by App slug and resolved to the integration id at apply time via ONE GET .../deployment_protection_rules/apps fetch (made only when a declared rule is missing; a slug the listing does not carry is a hard error naming the available slugs). Undeclared rules within a declared key are KEPT by default - Apps can enable themselves as gates, and silently disabling a deployment gate is security-relevant - with `undeclared: delete` opting into disabling; these endpoints also sit outside the Environments PAT permission (the enabled-rules list under Actions read, the Apps read and both writes under Administration). In check mode against a missing environment the declared variables, secrets, patterns, and protection rules cannot be listed, so notes say they are unverifiable until it exists; against a live environment whose custom_branch_policies flag is off, the patterns earn the same note while the flag drift comes from the environment diff. A declared per-environment `pinned` key reconciles the repository's pinned deployments sidebar over POST /graphql, AFTER every environment PUT - each mutation addresses the node_id the environment PUT/GET bodies already carry, so no extra lookup is made. The live pins read back through the repository's pinnedEnvironments connection (the EnvironmentPins query), where the ordering is the 1-based `position` field ON THE PinnedEnvironment NODE (it does not live on the Environment object) - and, verified against live GitHub, those numbers may be NON-CONTIGUOUS: unpinning leaves a hole, a new pin appends at the tail via a monotonic counter, and only a reorder renormalizes the list. Positions are therefore consumed as a sort key only, and reconciliation compares RANK ORDER: the entries declaring pinned: true must LEAD the pinned list in settings-file declaration order. pinEnvironment ({environmentId, pinned}) pins a missing pin (tail append) and unpins a pinned: false entry, reorderEnvironment ({environmentId, position}) pulls a divergent pin left into its declared rank; unpins are issued before pins, so a swap can never transiently exceed GitHub's cap of 10 pins (more than 10 declared pinned: true entries are rejected upfront, and a final count that would overflow the cap - live pins nobody declared count toward it - fails BEFORE the first pin mutation, naming the cap and the way to make room; check mode surfaces the same overflow as a note). Pins with no pinned declaration - undeclared environments, or entries without the key - are never unpinned; one sitting among the leading ranks is moved after the declared block, surfaced as a note in BOTH modes so check and apply agree exactly. Everything reads back, so check mode reports exact pin membership and order drift (the order line names both sequences). Undeclared environments are left untouched. | | [Autolinks](https://docs.github.com/en/rest/repos/autolinks) | `autolinks` | GET/POST/DELETE /repos/{owner}/{repo}/autolinks; immutable upstream so changed entries are delete+recreate; undeclared autolinks DELETED by default, kept as notes under the wrapped `undeclared: keep` form. | | [Actions permissions](https://docs.github.com/en/rest/actions/permissions) (enabled, allowed_actions + any base-permission field GitHub adds; selected_actions policy; workflow token default permissions + can_approve_pull_request_reviews; workflows access_level; artifact/log retention; cache limits; OIDC subject claim customization; fork PR contributor approval and private-repo fork PR workflow policies) | `actions` | Key routing (src/sections/actions/index.ts): the two known workflow-token keys -> PUT .../actions/permissions/workflow, selected_actions -> PUT .../permissions/selected-actions, access_level -> PUT .../permissions/access (private repositories only), artifact_and_log_retention -> PUT .../permissions/artifact-and-log-retention (body {days}, verbatim), cache.max_cache_retention_days -> PUT .../actions/cache/retention-limit, cache.max_cache_size_gb -> PUT .../actions/cache/storage-limit (each limit is its own single-field endpoint, so unrecognized cache keys are rejected; a 403 on the cache endpoints can mean an org- or enterprise-managed policy rather than a missing grant), oidc_customization_sub -> GET/PUT .../actions/oidc/customization/sub (201 on write; needs the "Actions" PAT permission instead of Administration, and include_claim_keys is compared positionally because claim-key order defines the subject format), fork_pr_contributor_approval -> GET/PUT .../permissions/fork-pr-contributor-approval (the approval_policy object, verbatim), fork_pr_workflows_private_repos -> GET/PUT .../permissions/fork-pr-workflows-private-repos (all four toggles are required by this action - GitHub does not document whether the PUT preserves or resets an omitted one, so the file declares the complete policy; the pair is documented for private repositories, so a denial on it can also mean the repository is public), EVERYTHING else -> base PUT .../actions/permissions verbatim. Whenever any base-permissions key is present in that PUT body, an undeclared `enabled` is defaulted to `true` (declaring a permissions field implies Actions are on), and selected_actions with no allowed_actions infers allowed_actions: selected. RISK: a key GitHub adds that belongs on a NEW sub-endpoint gets routed to the base PUT where GitHub ignores it; audit the routing whenever GitHub adds a permissions sub-endpoint. | diff --git a/src/engine/execute.ts b/src/engine/execute.ts index 27b4c1e..3a86370 100644 --- a/src/engine/execute.ts +++ b/src/engine/execute.ts @@ -104,6 +104,7 @@ export async function executePlan( `BUG: ${section.key} planned an operation under role "${op.role}", which is a read endpoint (${endpoint.route}); only write roles are plannable`, ); } + await op.before?.(exec); const payload = typeof op.payload === "function" ? await op.payload(exec) : op.payload; const request = { params: op.params, query: op.query, payload, describe: op.describe }; if (op.tolerate === undefined) { @@ -135,6 +136,7 @@ export async function executePlan( `BUG: ${section.key} planned an operation under role "${op.role}", which is a GraphQL ${graphqlOp.kind} operation; only write roles are plannable`, ); } + await op.before?.(exec); const variables = typeof op.variables === "function" ? await op.variables(exec) : op.variables; response = await callGraphql(ctx, section, graphqlOp, variables ?? {}, { diff --git a/src/sections/branches/branches.test.ts b/src/sections/branches/branches.test.ts index 7503acf..5f22a71 100644 --- a/src/sections/branches/branches.test.ts +++ b/src/sections/branches/branches.test.ts @@ -666,19 +666,25 @@ describe("branches GraphQL-routed keys", () => { expect(result.ops).toHaveLength(1); expect(result.ops[0]).toMatchObject({ role: "updateRule", - variables: { - input: { - branchProtectionRuleId: "RULE:main", - bypassForcePushActorIds: ["U_2"], - requiresDeployments: true, - requiredDeploymentEnvironments: ["prod"], - }, - }, drift: [ "branches[main].protection.force_push_bypassers: the settings file declares [release-bot] but the live rule allows [octocat]; apply will replace the allowance list", "branches[main].protection.required_deployments: the settings file requires deployments to [prod] but the live rule requires [qa]; apply will set the declared list", ], }); + // The actor's node id is an execution-time input: the plan issues no + // lookup (check mode never does), the sealed variables carry the id. + expect(api.calls.filter((c) => c.path.startsWith("BranchProtectionActor"))).toHaveLength(0); + const variables = result.ops[0]?.variables; + expect(typeof variables).toBe("function"); + expect(await (variables as (exec: typeof NO_SECRETS) => unknown)(NO_SECRETS)).toEqual({ + input: { + branchProtectionRuleId: "RULE:main", + bypassForcePushActorIds: ["U_2"], + requiresDeployments: true, + requiredDeploymentEnvironments: ["prod"], + }, + }); + expect(api.calls.filter((c) => c.path === "BranchProtectionActorUser")).toHaveLength(1); // The line renders from the mutation's read-back, so it is a thunk here. expect(typeof result.ops[0]?.change).toBe("function"); expect(api.mutations()).toHaveLength(0); @@ -709,29 +715,48 @@ describe("branches GraphQL-routed keys", () => { expect(api.calls.filter((c) => c.path.startsWith("BranchProtectionActor"))).toHaveLength(0); }); - test("a planned PUT re-applies matching routed keys through the update, with the re-apply as its drift", async () => { - const api = new MockApi({ - [PROTECTION]: { data: { enforce_admins: { enabled: false } } }, - "GRAPHQL BranchProtectionRules": rulesData([ - ruleNode("main", {}, [{ actor: { __typename: "User", login: "octocat" } }]), - ]), - "GRAPHQL BranchProtectionActorUser": { - data: { repository: { id: "R_1" }, user: { id: "U_1" } }, + test("a planned PUT re-applies matching routed keys through the update, with the re-apply as its drift, and resolves the actors ahead of the PUT", async () => { + const api = new MockApi( + { + [PROTECTION]: { data: { enforce_admins: { enabled: false } } }, + "GRAPHQL BranchProtectionRules": rulesData([ + ruleNode("main", {}, [{ actor: { __typename: "User", login: "octocat" } }]), + ]), + "GRAPHQL BranchProtectionActorUser": { + data: { repository: { id: "R_1" }, user: { id: "U_1" } }, + }, }, - }); + { unroutedMutations: "succeed" }, + ); const result = await plan(api, [ { name: "main", protection: { enforce_admins: true, force_push_bypassers: ["octocat"] } }, ]); - expect(result.ops.map((op) => [op.role, op.drift, op.variables])).toEqual([ - ["putProtection", ["branches[main].protection.enforce_admins: true != false"], undefined], + expect(result.ops.map((op) => [op.role, op.drift])).toEqual([ + ["putProtection", ["branches[main].protection.enforce_admins: true != false"]], [ "updateRule", [ "branches[main].protection: force_push_bypassers re-applied after the protection PUT (GitHub does not document whether the PUT preserves them)", ], - { input: { branchProtectionRuleId: "RULE:main", bypassForcePushActorIds: ["U_1"] } }, ], ]); + // The PUT carries the actor resolution, so a bad actor fails before the + // live protection is replaced; the update's variables seal the ids. + expect(typeof result.ops[0]?.before).toBe("function"); + expect(typeof result.ops[1]?.variables).toBe("function"); + + const execution = await executePlan(result, branchesSection, api, REPO, NO_SECRETS); + expect(execution.status).toBe("applied"); + // One lookup, ahead of the PUT; the update finds the id in the per-run cache. + expect( + api.calls + .filter((c) => c.method === "PUT" || c.path.startsWith("BranchProtectionActor")) + .map((c) => (c.method === "PUT" ? "PUT" : c.path)), + ).toEqual(["BranchProtectionActorUser", "PUT"]); + expect(api.mutations().map((m) => m.payload)).toEqual([ + NULL_FILLED, + { input: { branchProtectionRuleId: "RULE:main", bypassForcePushActorIds: ["U_1"] } }, + ]); }); test("declared null turns a live requirement off through the update, verified by the read-back", async () => { @@ -793,25 +818,34 @@ describe("branches GraphQL-routed keys", () => { expect(api.mutations()).toHaveLength(0); }); - test("an unknown team is a named config error at plan time, not a node-id crash", async () => { - const api = new MockApi({ - [PROBE]: { data: { name: "main" } }, - "GRAPHQL BranchProtectionRules": rulesData([ruleNode("main")]), - "GRAPHQL BranchProtectionActorTeam": { - data: { repository: { id: "R_1" }, organization: { team: null } }, - }, - }); - await expect( - plan(api, [ - { - name: "main", - protection: { enforce_admins: true, force_push_bypassers: ["e2e-owner/ghost-team"] }, + test("an unknown team is a named config error ahead of the section's first write, not a node-id crash", async () => { + const api = new MockApi( + { + [PROBE]: { data: { name: "main" } }, + "GRAPHQL BranchProtectionRules": rulesData([ruleNode("main")]), + "GRAPHQL BranchProtectionActorTeam": { + data: { repository: { id: "R_1" }, organization: { team: null } }, }, - ]), - ).rejects.toThrow(/no team with slug "ghost-team"/); + }, + { unroutedMutations: "succeed" }, + ); + const result = await plan(api, [ + { + name: "main", + protection: { enforce_admins: true, force_push_bypassers: ["e2e-owner/ghost-team"] }, + }, + ]); + // The plan carries the PUT; the resolution it runs first names the error. + expect(result.ops.map((op) => op.role)).toEqual(["putProtection", "updateRule"]); + const execution = await executePlan(result, branchesSection, api, REPO, NO_SECRETS); + expect(execution.status).toBe("failed"); + expect(String((execution as { error: Error }).error.message)).toMatch( + /no team with slug "ghost-team"/, + ); + expect(api.mutations()).toHaveLength(0); }); - test("a misspelled actor fails the plan, so no write - the destructive PUT included - is ever issued", async () => { + test("a misspelled actor fails before the section's first write, so the destructive PUT is never issued", async () => { const api = new MockApi( { [PROBE]: { data: { name: "main" } }, @@ -820,11 +854,53 @@ describe("branches GraphQL-routed keys", () => { }, { unroutedMutations: "succeed" }, ); - await expect( - plan(api, [ - { name: "main", protection: { enforce_admins: true, force_push_bypassers: ["ghost"] } }, - ]), - ).rejects.toThrow(/GraphQL lookup succeeded but returned no node id/); + const result = await plan(api, [ + { name: "main", protection: { enforce_admins: true, force_push_bypassers: ["ghost"] } }, + ]); + // Planning issues no lookup: check mode reports the drift without one. + expect(api.calls.filter((c) => c.path.startsWith("BranchProtectionActor"))).toHaveLength(0); + const execution = await executePlan(result, branchesSection, api, REPO, NO_SECRETS); + expect(execution.status).toBe("failed"); + expect(String((execution as { error: Error }).error.message)).toMatch( + /GraphQL lookup succeeded but returned no node id/, + ); + expect(execution.landed).toBe(0); + expect(api.mutations()).toHaveLength(0); + }); + + test("a misspelled actor on a LATER entry fails before an EARLIER entry's write lands", async () => { + // main drifts on the REST half and carries no actors; dev declares the + // bad actor. The section's first operation (main's PUT) resolves every + // planned actor first, so nothing is written for either branch. + const api = new MockApi( + { + [PROTECTION]: { data: { enforce_admins: { enabled: false } } }, + "GET /repos/o/r/branches/dev": { data: { name: "dev" } }, + "GRAPHQL BranchProtectionRules": rulesData([ruleNode("main")]), + "GRAPHQL BranchProtectionActorUser": { data: { repository: { id: "R_1" }, user: null } }, + }, + { unroutedMutations: "succeed" }, + ); + const result = await plan(api, [ + { name: "main", protection: { enforce_admins: true } }, + { name: "dev", protection: { enforce_admins: true, force_push_bypassers: ["ghost"] } }, + ]); + expect(result.ops.map((op) => [op.role, typeof op.before])).toEqual([ + ["putProtection", "function"], + ["putProtection", "undefined"], + ["updateRule", "undefined"], + ]); + const execution = await executePlan(result, branchesSection, api, REPO, NO_SECRETS); + expect(execution).toEqual({ + status: "failed", + changes: [], + notes: [], + landed: 0, + error: expect.any(Error), + }); + expect(String((execution as { error: Error }).error.message)).toMatch( + /force_push_bypassers actor "ghost".*returned no node id/, + ); expect(api.mutations()).toHaveLength(0); }); @@ -882,46 +958,50 @@ describe("branches wildcard entries", () => { }, { name: "old/*", protection: null }, ]); - expect(result).toEqual({ - ops: [ - { - role: "createRule", - variables: { - input: { repositoryId: "R_1", pattern: "release/*", isAdminEnforced: true }, - }, - describe: 'creating the protection rule "release/*"', - drift: [ - "branches[release/*]: no live rule matches this pattern but the settings file declares protection; apply will create the rule", - ], - change: 'created protection rule "release/*"', - }, - { - role: "updateRule", - variables: { - input: { - branchProtectionRuleId: "RULE:hotfix/*", - requiresApprovingReviews: true, - requiredApprovingReviewCount: 2, - }, + const [create, update, remove] = result.ops; + expect([update, remove]).toEqual([ + { + role: "updateRule", + variables: { + input: { + branchProtectionRuleId: "RULE:hotfix/*", + requiresApprovingReviews: true, + requiredApprovingReviewCount: 2, }, - describe: 'updating the protection rule "hotfix/*"', - drift: [ - "branches[hotfix/*].protection.required_pull_request_reviews.required_approving_review_count: 2 != 1", - ], - change: 'updated protection rule "hotfix/*"', - }, - { - role: "deleteRule", - variables: { input: { branchProtectionRuleId: "RULE:old/*" } }, - describe: 'deleting the protection rule "old/*"', - drift: [ - "branches[old/*]: a live rule matches this pattern but the settings file declares protection: null; apply will delete the rule", - ], - change: 'deleted protection rule "old/*"', }, + describe: 'updating the protection rule "hotfix/*"', + drift: [ + "branches[hotfix/*].protection.required_pull_request_reviews.required_approving_review_count: 2 != 1", + ], + change: 'updated protection rule "hotfix/*"', + }, + { + role: "deleteRule", + variables: { input: { branchProtectionRuleId: "RULE:old/*" } }, + describe: 'deleting the protection rule "old/*"', + drift: [ + "branches[old/*]: a live rule matches this pattern but the settings file declares protection: null; apply will delete the rule", + ], + change: 'deleted protection rule "old/*"', + }, + ]); + expect(result.notes).toEqual([]); + expect(result.drift).toEqual([]); + // The create needs the repository's node id, an execution-time input: the + // plan issues no lookup for it, the sealed variables carry it. + expect(create).toMatchObject({ + role: "createRule", + describe: 'creating the protection rule "release/*"', + drift: [ + "branches[release/*]: no live rule matches this pattern but the settings file declares protection; apply will create the rule", ], - notes: [], - drift: [], + change: 'created protection rule "release/*"', + }); + expect(api.calls.map((c) => c.path)).toEqual(["BranchProtectionRules"]); + const variables = create?.variables; + expect(typeof variables).toBe("function"); + expect(await (variables as (exec: typeof NO_SECRETS) => unknown)(NO_SECRETS)).toEqual({ + input: { repositoryId: "R_1", pattern: "release/*", isAdminEnforced: true }, }); expect(api.calls.map((c) => c.path)).toEqual([ "BranchProtectionRules", diff --git a/src/sections/branches/docs.ts b/src/sections/branches/docs.ts index bf41aed..4832f08 100644 --- a/src/sections/branches/docs.ts +++ b/src/sections/branches/docs.ts @@ -11,7 +11,7 @@ export const docs: SectionDocs = { { area: "[Classic branch protection](https://docs.github.com/en/rest/branches/branch-protection) (literal branches, wildcard patterns, force-push bypass actors, required deployments)", notes: - 'PUT /repos/{owner}/{repo}/branches/{branch}/protection passthrough; the four required keys are null-filled; protection: null issues DELETE (deleteBranchProtectionRule for a wildcard entry). Check mode flattens the GET shape ({enabled} wrappers, actor objects -> login/slug strings, *_url dropped) to compare like with like (src/sections/branches/index.ts flattenProtection). Every write is drift-gated: the PUT is planned when a declared key diverges or when the replacing PUT would remove a live setting the file omits, and a converged branch plans nothing. Three routed keys never ride the PUT: required_signatures (the PUT silently drops it) goes through its own POST/DELETE .../protection/required_signatures sub-endpoint when it drifts and again after any planned PUT (declared true POSTs, false DELETEs, undeclared untouched; a GET that omits the field reads as false; GitHub does not document whether the PUT preserves an existing requirement, so declare the toggle on any branch that carries one), while force_push_bypassers and required_deployments are REST-invisible entirely and ride ONE updateBranchProtectionRule GraphQL mutation on the same terms. force_push_bypassers is a list of actor strings - a bare login is a user, "org/team-slug" a team, "app/slug" a GitHub App - resolved to node ids at plan time, so a misspelled actor fails check mode and apply alike before any write: users and teams through GraphQL (the BranchProtectionActorUser and BranchProtectionActorTeam lookups, new-format ids), Apps through the public GET /apps/{app_slug} REST lookup, whose node_id can still be the legacy format for old Apps (GitHub accepts it with a deprecation warning). required_deployments takes {environments: [...]}; declaring null turns the requirement off, an absent key leaves the live state untouched. CAVEAT (verified live): GitHub SILENTLY drops required-deployment environment names that do not exist while the mutation succeeds, so apply verifies the mutation\'s read-back and fails loudly naming any dropped name - the environments section runs before branches, so environments declared in the same file exist by then. WILDCARD entries (a name containing `*`, `?`, or `[`, e.g. release/*) are invisible to the REST endpoints (their docs point wildcard use at the GraphQL API), so they reconcile entirely through the createBranchProtectionRule/updateBranchProtectionRule/deleteBranchProtectionRule mutations (a create addresses the repository node id the BranchProtectionRepository query fetches) against the Repository.branchProtectionRules read; a wildcard protection accepts exactly the keys this section can round-trip through the GraphQL rule surface - enforce_admins (its isAdminEnforced twin live-verified bidirectionally against the REST view of the same rule), required_linear_history, allow_force_pushes, allow_deletions, block_creations, required_conversation_resolution, lock_branch, allow_fork_syncing, required_signatures, required_status_checks (strict, contexts), required_pull_request_reviews (required_approving_review_count, require_code_owner_reviews, dismiss_stale_reviews, require_last_push_approval), force_push_bypassers, required_deployments - and rejects anything else upfront (the actor-list controls have GraphQL fields but their REST vocabulary of database ids does not round-trip through node-id-based reads, so this section does not manage them on wildcard rules), pointing at rulesets as the recommended path for new configuration. The rules query fires only when an entry has a wildcard name or declares a GraphQL-routed key (a pure-REST declaration issues no GraphQL request), which also scopes the undeclared-rule NOTE: only a run whose declaration fires the query reports a live wildcard rule the file does not declare, as a note and never a deletion; wildcard updates have PATCH semantics (an omitted key keeps its live value, unlike the literal PUT\'s replace). Actor and environment names compare case-insensitively (GitHub canonicalizes them), and the routed lists reject duplicate names upfront.', + 'PUT /repos/{owner}/{repo}/branches/{branch}/protection passthrough; the four required keys are null-filled; protection: null issues DELETE (deleteBranchProtectionRule for a wildcard entry). Check mode flattens the GET shape ({enabled} wrappers, actor objects -> login/slug strings, *_url dropped) to compare like with like (src/sections/branches/index.ts flattenProtection). Every write is drift-gated: the PUT is planned when a declared key diverges or when the replacing PUT would remove a live setting the file omits, and a converged branch plans nothing. Three routed keys never ride the PUT: required_signatures (the PUT silently drops it) goes through its own POST/DELETE .../protection/required_signatures sub-endpoint when it drifts and again after any planned PUT (declared true POSTs, false DELETEs, undeclared untouched; a GET that omits the field reads as false; GitHub does not document whether the PUT preserves an existing requirement, so declare the toggle on any branch that carries one), while force_push_bypassers and required_deployments are REST-invisible entirely and ride ONE updateBranchProtectionRule GraphQL mutation on the same terms. force_push_bypassers is a list of actor strings - a bare login is a user, "org/team-slug" a team, "app/slug" a GitHub App - resolved to node ids when apply executes, ahead of the section\'s first write, so a misspelled actor fails before any write lands (check mode issues no lookup and reports the drift): users and teams through GraphQL (the BranchProtectionActorUser and BranchProtectionActorTeam lookups, new-format ids), Apps through the public GET /apps/{app_slug} REST lookup, whose node_id can still be the legacy format for old Apps (GitHub accepts it with a deprecation warning). required_deployments takes {environments: [...]}; declaring null turns the requirement off, an absent key leaves the live state untouched. CAVEAT (verified live): GitHub SILENTLY drops required-deployment environment names that do not exist while the mutation succeeds, so apply verifies the mutation\'s read-back and fails loudly naming any dropped name - the environments section runs before branches, so environments declared in the same file exist by then. WILDCARD entries (a name containing `*`, `?`, or `[`, e.g. release/*) are invisible to the REST endpoints (their docs point wildcard use at the GraphQL API), so they reconcile entirely through the createBranchProtectionRule/updateBranchProtectionRule/deleteBranchProtectionRule mutations (a create addresses the repository node id the BranchProtectionRepository query fetches) against the Repository.branchProtectionRules read; a wildcard protection accepts exactly the keys this section can round-trip through the GraphQL rule surface - enforce_admins (its isAdminEnforced twin live-verified bidirectionally against the REST view of the same rule), required_linear_history, allow_force_pushes, allow_deletions, block_creations, required_conversation_resolution, lock_branch, allow_fork_syncing, required_signatures, required_status_checks (strict, contexts), required_pull_request_reviews (required_approving_review_count, require_code_owner_reviews, dismiss_stale_reviews, require_last_push_approval), force_push_bypassers, required_deployments - and rejects anything else upfront (the actor-list controls have GraphQL fields but their REST vocabulary of database ids does not round-trip through node-id-based reads, so this section does not manage them on wildcard rules), pointing at rulesets as the recommended path for new configuration. The rules query fires only when an entry has a wildcard name or declares a GraphQL-routed key (a pure-REST declaration issues no GraphQL request), which also scopes the undeclared-rule NOTE: only a run whose declaration fires the query reports a live wildcard rule the file does not declare, as a note and never a deletion; wildcard updates have PATCH semantics (an omitted key keeps its live value, unlike the literal PUT\'s replace). Actor and environment names compare case-insensitively (GitHub canonicalizes them), and the routed lists reject duplicate names upfront.', }, ], }; diff --git a/src/sections/branches/index.ts b/src/sections/branches/index.ts index fcd4fa2..170353c 100644 --- a/src/sections/branches/index.ts +++ b/src/sections/branches/index.ts @@ -26,7 +26,14 @@ import { type GraphqlOpDecl, graphqlOp } from "../contract/graphql.js"; import { parseLive } from "../contract/live.js"; import { loosen, type SectionMeta, type SectionModule } from "../contract/module.js"; import type { SectionPermission } from "../contract/permissions.js"; -import { type PlanContext, type PlannedOp, plainData, type SectionPlan } from "../contract/plan.js"; +import { + type ExecTools, + type Late, + type PlanContext, + type PlannedOp, + plainData, + type SectionPlan, +} from "../contract/plan.js"; import { rejectDuplicates } from "../contract/requests.js"; import { type BranchConfig, @@ -222,6 +229,7 @@ const ENDPOINTS = { route: "GET /apps/{app_slug}", statuses: { 200: "the GitHub App", 404: "no App with this slug" }, permission: "none", + phase: "execution", }, } as const satisfies Record; @@ -297,10 +305,16 @@ const RULES_QUERY = graphqlOp<{ owner: string; repo: string }>()({ }`, }); -/** The repository's GraphQL node id, needed only to CREATE a wildcard rule. */ +/** + * The repository's GraphQL node id, needed only to CREATE a wildcard rule. + * Execution-phase, like the two actor lookups: a fine-grained denial answers + * NOT_FOUND, which none of the three tolerates, so they may only run where + * the section's posture puts the denial - at the first write. + */ const REPO_LOOKUP = graphqlOp<{ owner: string; repo: string }>()({ name: "BranchProtectionRepository", kind: "read", + phase: "execution", outcomes: { ok: "the repository's GraphQL node id" }, query: `query BranchProtectionRepository($owner: String!, $repo: String!) { repository(owner: $owner, name: $repo) { id } @@ -316,6 +330,7 @@ const REPO_LOOKUP = graphqlOp<{ owner: string; repo: string }>()({ const ACTOR_USER = graphqlOp<{ owner: string; repo: string; login: string }>()({ name: "BranchProtectionActorUser", kind: "read", + phase: "execution", outcomes: { ok: "the user's node id", NOT_FOUND: "no user with this login, or the token cannot see it", @@ -332,6 +347,7 @@ const ACTOR_USER = graphqlOp<{ owner: string; repo: string; login: string }>()({ const ACTOR_TEAM = graphqlOp<{ owner: string; repo: string; org: string; team: string }>()({ name: "BranchProtectionActorTeam", kind: "read", + phase: "execution", outcomes: { ok: "the team's node id", NOT_FOUND: "no organization with this login, or the token cannot see it", @@ -421,6 +437,12 @@ interface GraphqlRun { rules: LiveRules; repoId: string | null; actorIds: Map; + /** + * Every bypass actor a planned mutation resolves at execution, appended + * by ruleVariables() as it seals one; plan() resolves them all ahead of + * the FIRST write, whichever entry they belong to. + */ + lateActors: string[]; } /** The plan context over this section's literal dictionaries. */ @@ -747,6 +769,7 @@ function routedKeyDrift( */ async function resolveActorId( ctx: BranchesContext, + exec: ExecTools, graphqlRun: GraphqlRun, raw: string, ): Promise { @@ -762,6 +785,7 @@ async function resolveActorId( let id: unknown; if (actor.kind === "user") { const data = await ctx.read.actorUser.call( + exec, { ...repoVariables(ctx), login: actor.login }, { describe: `resolving force-push bypass user "${raw}"` }, ); @@ -769,6 +793,7 @@ async function resolveActorId( id = (data.user as Record | null)?.id; } else if (actor.kind === "team") { const data = await ctx.read.actorTeam.call( + exec, { ...repoVariables(ctx), org: actor.org, team: actor.team }, { describe: `resolving force-push bypass team "${raw}"` }, ); @@ -784,7 +809,7 @@ async function resolveActorId( } id = team.id; } else { - const result = await ctx.read.appLookup.tryCall({ + const result = await ctx.read.appLookup.tryCall(exec, { params: { app_slug: actor.slug }, describe: `resolving force-push bypass App "${raw}"`, }); @@ -833,40 +858,71 @@ async function lateRuleId(ctx: BranchesContext, pattern: string): Promise { const ids: string[] = []; for (const actor of actors) { - ids.push(await resolveActorId(ctx, graphqlRun, actor)); + ids.push(await resolveActorId(ctx, exec, graphqlRun, actor)); } return ids; } -/** The mutation input fields for a wildcard entry, routed keys resolved. */ -async function wildcardInputFields( - ctx: BranchesContext, - graphqlRun: GraphqlRun, - protection: BranchProtectionConfig, -): Promise> { +/** The mutation input fields for a wildcard entry the plan knows up front: every key but the actors. */ +function wildcardInput(protection: BranchProtectionConfig): Record { const input = translateWildcardProtection(protection); - if (protection.force_push_bypassers !== undefined) { - input.bypassForcePushActorIds = await resolveActorIds( - ctx, - graphqlRun, - protection.force_push_bypassers, - ); - } if (protection.required_deployments !== undefined) { Object.assign(input, deploymentInputFields(protection.required_deployments)); } return input; } +/** A rule mutation's variables: a value, or a thunk the executor seals right before the request. */ +type RuleVariables = { input: Record } | Late<{ input: Record }>; + +/** + * A rule mutation's variables: the plan-time `fields` plus what only the read + * port supplies at EXECUTION time - the bypass actors' node ids and any id + * `late` looks up (the repository's, a rule's the PUT ahead creates). Check + * mode must never issue those lookups: a fine-grained denial answers NOT_FOUND + * where the section's posture promises the denial surfaces at the first write. + * A value when nothing is late, so the idempotence proof compares it by field. + */ +function ruleVariables( + ctx: BranchesContext, + graphqlRun: GraphqlRun, + fields: Record, + actors: readonly string[] | undefined, + late?: (exec: ExecTools) => Promise>, +): RuleVariables { + if (actors === undefined && late === undefined) { + return { input: fields }; + } + if (actors !== undefined) { + graphqlRun.lateActors.push(...actors); + } + // The actors resolve first: their reads select the repository's node id + // too, which spares a create its dedicated lookup (see adoptRepoId). + return async (exec) => ({ + input: { + ...fields, + ...(actors === undefined + ? {} + : { bypassForcePushActorIds: await resolveActorIds(ctx, exec, graphqlRun, actors) }), + ...(late === undefined ? {} : await late(exec)), + }, + }); +} + /** The repository's node id: one an actor read already carried, else the dedicated lookup. */ -async function repositoryNodeId(ctx: BranchesContext, graphqlRun: GraphqlRun): Promise { +async function repositoryNodeId( + ctx: BranchesContext, + exec: ExecTools, + graphqlRun: GraphqlRun, +): Promise { if (graphqlRun.repoId === null) { - const data = await ctx.read.repoLookup.call(repoVariables(ctx), { + const data = await ctx.read.repoLookup.call(exec, repoVariables(ctx), { describe: "resolving the repository's GraphQL node id", }); const id = (data.repository as Record | null)?.id; @@ -994,12 +1050,10 @@ export const branchesSection = { const needsGraphql = (branch: BranchConfig): boolean => isWildcardPattern(branch.name) || hasRoutedGraphqlKeys(branch.protection); let entries: ClassifiedEntry[]; - if (desired.some(needsGraphql)) { - const graphqlRun: GraphqlRun = { - rules: await fetchRules(ctx), - repoId: null, - actorIds: new Map(), - }; + const graphqlRun: GraphqlRun | null = desired.some(needsGraphql) + ? { rules: await fetchRules(ctx), repoId: null, actorIds: new Map(), lateActors: [] } + : null; + if (graphqlRun !== null) { const declaredPatterns = new Set(desired.map((branch) => branch.name)); for (const pattern of [...(graphqlRun.rules?.keys() ?? [])].sort()) { if (isWildcardPattern(pattern) && !declaredPatterns.has(pattern)) { @@ -1028,6 +1082,22 @@ export const branchesSection = { } await planLiteralEntry(ctx, this, entry.routed, entry.branch, plan); } + // Every actor a planned mutation resolves at execution resolves ahead of + // the plan's FIRST write, whichever entry it belongs to: a misspelled + // actor fails while every branch's live protection is still untouched, + // and the mutations' thunks then find the ids cached. + const [lead, ...rest] = plan.ops; + if (graphqlRun !== null && graphqlRun.lateActors.length > 0 && lead !== undefined) { + plan.ops = [ + { + ...lead, + before: async (exec) => { + await resolveActorIds(ctx, exec, graphqlRun, graphqlRun.lateActors); + }, + }, + ...rest, + ]; + } return plan; }, } satisfies SectionModule<"branches", typeof ENDPOINTS, typeof GRAPHQL>; @@ -1187,20 +1257,41 @@ async function planLiteralEntry( ); } } - if (routed === null) { - return; + if (routed !== null) { + planRoutedUpdate(ctx, routed.graphqlRun, plan, { + name: branch.name, + protection: branch.protection, + prefix, + putPlanned, + }); } - const node = routed.graphqlRun.rules?.get(branch.name); +} + +/** + * Plan a literal entry's rule mutation for its routed keys. Its bypass + * actors, when declared, seal into the mutation's variables at execution + * (ruleVariables), and plan() resolves them ahead of the section's first write. + */ +function planRoutedUpdate( + ctx: BranchesContext, + graphqlRun: GraphqlRun, + plan: BranchesPlan, + entry: { + name: string; + protection: BranchProtectionConfig; + prefix: string; + putPlanned: boolean; + }, +): void { + const { name, protection, prefix, putPlanned } = entry; + const { force_push_bypassers: forcePushBypassers, required_deployments: requiredDeployments } = + protection; + const node = graphqlRun.rules?.get(name); const routedKeys = [ ...(forcePushBypassers === undefined ? [] : ["force_push_bypassers"]), ...(requiredDeployments === undefined ? [] : ["required_deployments"]), ].join(" and "); - const routedDrift = routedKeyDrift( - prefix, - branch.protection, - routed.graphqlRun.rules, - branch.name, - ); + const routedDrift = routedKeyDrift(prefix, protection, graphqlRun.rules, name); if (routedDrift.length === 0 && putPlanned) { routedDrift.push( `${prefix}: ${routedKeys} re-applied after the protection PUT (GitHub does not document whether the PUT preserves them)`, @@ -1210,37 +1301,28 @@ async function planLiteralEntry( if (drift === null) { return; } - // Actors resolve at plan time, so a misspelled actor fails the entry while - // the live protection is still untouched - never after the PUT replaced it. - const fields: Record = {}; - if (forcePushBypassers !== undefined) { - fields.bypassForcePushActorIds = await resolveActorIds( - ctx, - routed.graphqlRun, - forcePushBypassers, - ); - } - if (requiredDeployments !== undefined) { - Object.assign(fields, deploymentInputFields(requiredDeployments)); - } + const deploymentFields = + requiredDeployments === undefined ? {} : deploymentInputFields(requiredDeployments); plan.ops.push({ role: "updateRule", - describe: `setting the GraphQL-only protection fields of branch "${branch.name}"`, + describe: `setting the GraphQL-only protection fields of branch "${name}"`, // A rule the plan-time fetch did not carry (the PUT planned above // creates it) is looked up once that operation has run. variables: node !== undefined - ? { input: { branchProtectionRuleId: node.id, ...fields } } - : async () => ({ - input: { - branchProtectionRuleId: await lateRuleId(ctx, branch.name), - ...fields, - }, - }), + ? ruleVariables( + ctx, + graphqlRun, + { branchProtectionRuleId: node.id, ...deploymentFields }, + forcePushBypassers, + ) + : ruleVariables(ctx, graphqlRun, deploymentFields, forcePushBypassers, async () => ({ + branchProtectionRuleId: await lateRuleId(ctx, name), + })), drift, change: verifiedChange( - `set ${routedKeys} on "${branch.name}"`, - branch.name, + `set ${routedKeys} on "${name}"`, + name, requiredDeployments, "updateBranchProtectionRule", ), @@ -1273,12 +1355,14 @@ async function planWildcardEntry( return; } const deployments = branch.protection.required_deployments; + const actors = branch.protection.force_push_bypassers; + const fields = wildcardInput(branch.protection); if (node === undefined) { - const fields = await wildcardInputFields(ctx, graphqlRun, branch.protection); - const repositoryId = await repositoryNodeId(ctx, graphqlRun); plan.ops.push({ role: "createRule", - variables: { input: { repositoryId, pattern, ...fields } }, + variables: ruleVariables(ctx, graphqlRun, { pattern, ...fields }, actors, async (exec) => ({ + repositoryId: await repositoryNodeId(ctx, exec, graphqlRun), + })), describe: `creating the protection rule "${pattern}"`, drift: [ `branches[${pattern}]: no live rule matches this pattern but the settings file declares protection; apply will create the rule`, @@ -1302,10 +1386,14 @@ async function planWildcardEntry( if (drift === null) { return; } - const fields = await wildcardInputFields(ctx, graphqlRun, branch.protection); plan.ops.push({ role: "updateRule", - variables: { input: { branchProtectionRuleId: node.id, ...fields } }, + variables: ruleVariables( + ctx, + graphqlRun, + { branchProtectionRuleId: node.id, ...fields }, + actors, + ), describe: `updating the protection rule "${pattern}"`, drift, change: verifiedChange( diff --git a/src/sections/branches/scenarios/branches-administration-denied-check-drift.yml b/src/sections/branches/scenarios/branches-administration-denied-check-drift.yml new file mode 100644 index 0000000..7771b23 --- /dev/null +++ b/src/sections/branches/scenarios/branches-administration-denied-check-drift.yml @@ -0,0 +1,42 @@ +# A token denied Administration under fine-grained denial: the protection +# read 404s, which this section reads as "unprotected" (its declared posture), +# so check reports drift and the denial surfaces only at apply's first write. +# Both entries need GraphQL node ids for their writes - the wildcard the +# repository's, the bypass actor the team's - and those lookups seal at +# execution: check mode issues none of them, since the denied token answers +# each with NOT_FOUND, which would fail the section on a read. +name: branches-administration-denied-check-drift +settings: + branches: + - name: main + protection: + enforce_admins: true + force_push_bypassers: + - e2e-owner/platform + - name: release/* + protection: + enforce_admins: true +inputs: + mode: check +denial_style: fine_grained +live_state: + branches: ["main"] +token_permissions: + administration: none + contents: read +expect: + exit_code: 1 + result: drift + outcomes: + branches: drift + summary_contains: + - "branches[main]: unprotected live but the settings file declares protection; apply will protect it" + - "branches[main].protection.force_push_bypassers: the live rule cannot be read (the rules query answered not found); apply will set the declared value" + - "branches[release/*]: no live rule matches this pattern but the settings file declares protection; apply will create the rule" + requests_contain: + - "GRAPHQL BranchProtectionRules" + never: + - "GRAPHQL BranchProtectionRepository" + - "GRAPHQL BranchProtectionActorTeam" + - "GRAPHQL BranchProtectionActorUser" + - "PUT /repos/{repo}/branches" diff --git a/src/sections/contract/endpoints.ts b/src/sections/contract/endpoints.ts index a10c467..7edf677 100644 --- a/src/sections/contract/endpoints.ts +++ b/src/sections/contract/endpoints.ts @@ -41,6 +41,16 @@ export type EndpointDecl = readonly accessGrade?: never; readonly alwaysRewrite?: never; readonly unverifiable?: never; + /** + * When the READ is issued. Omitted: while planning, so check mode and preflight meet + * it. "execution": only while EXECUTING a plan (a node id a mutation input needs), + * through bound helpers that take the ExecTools token only a thunk receives (see + * ReadPort in ./plan.ts), so plan() cannot call it and check mode never issues it; + * the e2e mock treats one in check mode as a violation. Such a read carries no + * primaryRead posture (denialPosture() rejects the pair): no denied first read can + * ever be classified from it. + */ + readonly phase?: "execution"; }) | (EndpointDeclFields & Recurrence & { @@ -51,6 +61,7 @@ export type EndpointDecl = */ readonly permission?: SectionPermission | "none"; readonly accessGrade?: never; + readonly phase?: never; }) | GatedReadDecl; @@ -74,6 +85,7 @@ export interface GatedReadDecl extends EndpointDeclFields { readonly accessGrade: "write"; readonly alwaysRewrite?: never; readonly unverifiable?: never; + readonly phase?: never; } /** The fields both EndpointDecl arms share. */ diff --git a/src/sections/contract/graphql.ts b/src/sections/contract/graphql.ts index 565022a..f1c48aa 100644 --- a/src/sections/contract/graphql.ts +++ b/src/sections/contract/graphql.ts @@ -115,12 +115,15 @@ export type GraphqlOpDecl = Record | (GraphqlOpCommon & { readonly kind: "write"; readonly query: `mutation ${string}`; readonly connection?: never; + readonly phase?: never; }); /** @@ -136,6 +139,7 @@ export type GraphqlPaginatedReadDecl = Record< readonly kind: "read"; readonly query: `query ${string}$cursor${string}`; readonly connection: GraphqlConnectionDecl; + readonly phase?: "execution"; }; /** diff --git a/src/sections/contract/module.ts b/src/sections/contract/module.ts index a0e010f..167c522 100644 --- a/src/sections/contract/module.ts +++ b/src/sections/contract/module.ts @@ -155,13 +155,16 @@ export function endpointPermission( * the wire (a GET or a query vs a mutating method or a mutation), `grade` * the access level GitHub gates it at (endpointKind, so an accessGrade * override write-gates a wire read; a GraphQL operation's kind is both), - * and `permission` the effective permission (endpointPermission). + * and `permission` the effective permission (endpointPermission). `phase` is + * when the section issues it: "plan" (planning, so check mode and preflight + * meet it) or "execution" (a thunk, apply only; see EndpointDecl.phase). */ export interface SectionOperation { readonly role: string; readonly wire: "read" | "write"; readonly grade: "read" | "write"; readonly permission: SectionPermission | "none"; + readonly phase: "plan" | "execution"; } /** @@ -176,16 +179,26 @@ export function sectionOperations(section: SectionMeta): SectionOperation[] { wire: endpointMethod(endpoint.route) === "GET" ? ("read" as const) : ("write" as const), grade: endpointKind(endpoint), permission: endpointPermission(section, endpoint), + phase: endpoint.phase ?? ("plan" as const), })), ...Object.entries(section.graphql ?? {}).map(([role, op]) => ({ role, wire: op.kind, grade: op.kind, permission: endpointPermission(section, op), + phase: op.phase ?? ("plan" as const), })), ]; } +/** + * The reads a plan() body issues: every read but the execution-phase ones, + * which only a thunk reaches, so neither check mode nor preflight meets them. + */ +export function planningReads(section: SectionMeta): SectionOperation[] { + return sectionOperations(section).filter((op) => op.wire === "read" && op.phase === "plan"); +} + /** * How GitHub gates a section's reads under a read-only grant: "plain" reads all * (also no reads at all), "write-gated" is denied at the first read, "mixed" reads @@ -194,12 +207,12 @@ export function sectionOperations(section: SectionMeta): SectionOperation[] { export type ReadGating = "plain" | "write-gated" | "mixed"; export function readGating(section: SectionMeta): ReadGating { - const reads = sectionOperations(section).filter((op) => op.wire === "read").length; - const gated = writeGatedReads(section).length; + const reads = planningReads(section); + const gated = reads.filter((op) => op.grade === "write").length; if (gated === 0) { return "plain"; } - return gated === reads ? "write-gated" : "mixed"; + return gated === reads.length ? "write-gated" : "mixed"; } /** One read GitHub gates at write: its route and effective permission. */ @@ -231,18 +244,24 @@ export type DenialPosture = NonNullable["notFound"] */ export function denialPosture(section: SectionMeta): DenialPosture { const primaries = Object.values(section.endpoints).flatMap((endpoint) => - endpoint.primaryRead === undefined ? [] : [endpoint.primaryRead.notFound], + endpoint.primaryRead === undefined ? [] : [endpoint], ); if (primaries.length > 1) { throw new Error( `BUG: ${section.key} declares primaryRead on ${primaries.length} endpoints; at most one read carries the 404 posture`, ); } - const posture = primaries[0]; + const primary = primaries[0]; + if (primary !== undefined && primary.phase === "execution") { + throw new Error( + `BUG: ${section.key} declares primaryRead on the execution-phase read ${primary.route}; plan() never issues it, so no denied first read can be classified from it`, + ); + } + const posture = primary?.primaryRead?.notFound; if (posture !== undefined) { return posture; } - if (sectionOperations(section).some((op) => op.wire === "read")) { + if (planningReads(section).length > 0) { throw new Error( `BUG: ${section.key} reads but declares no primaryRead posture, so a denied first read cannot be classified`, ); @@ -276,8 +295,8 @@ type _OperationDictionariesFlattened = MustBeNever< >; /** - * The check-mode note of a WRITE-ONLY section: one that declares no read - * operation at all, so check mode can verify nothing (and issues no request) + * The check-mode note of a WRITE-ONLY section: one that issues no read while + * planning, so check mode can verify nothing (and issues no request) * while apply re-asserts the declared state on every run. Derived from the * section's own operation list rather than restated per section: a read * endpoint added later makes the note's claim false, so the helper throws @@ -290,7 +309,7 @@ export function writeOnlyCheckNote( section: SectionMeta, opts: { resource: string; reasserts: string }, ): string { - if (sectionOperations(section).some((op) => op.wire === "read")) { + if (planningReads(section).length > 0) { throw new Error( `BUG: ${section.key} declares a read operation, so it is not write-only and the cannot-verify note would be false; diff against the read instead`, ); diff --git a/src/sections/contract/plan.ts b/src/sections/contract/plan.ts index 58502c7..ff1e791 100644 --- a/src/sections/contract/plan.ts +++ b/src/sections/contract/plan.ts @@ -140,12 +140,36 @@ export function plainData(value: unknown): PlainData { /** * What a thunk may compute at EXECUTION time only: the plaintext behind a * `$NAME` reference (resolved and masked up front, so check mode never sees - * one). A thunk may also await the read-only port plan() closed over. + * one). A thunk may also await the read-only port plan() closed over, and it + * alone holds this token, which the port's execution-phase reads demand. */ export interface ExecTools { resolveSecret(reference: string): string; } +/** + * The bound helpers of an execution-phase read: each takes the ExecTools + * token first. A plan() body has no token, so the call does not compile + * there; a thunk passes the one it received. + */ +type Gated = { + readonly [K in keyof T]: T[K] extends (...args: infer A) => infer R + ? (exec: ExecTools, ...args: A) => R + : T[K]; +}; + +/** Gate a bound helper set at runtime: the runtime twin of Gated. */ +function gated(bound: T): Gated { + return Object.fromEntries( + Object.entries(bound).map(([name, helper]) => [ + name, + typeof helper === "function" + ? (_exec: ExecTools, ...args: unknown[]) => helper(...args) + : helper, + ]), + ) as Gated; +} + /** The roles of a REST dictionary whose route reads on the wire (a GET). */ type ReadRole = { [R in keyof E & string]: E[R]["route"] extends `GET ${string}` ? R : never; @@ -230,20 +254,31 @@ type BoundGraphqlRead = { * GraphQL alike. Write roles are absent from the type, so a plan() body that * reaches for `ctx.read.` does not compile - the reads a section * may issue are exactly its declared GETs and GraphQL queries. A REST role - * declaring a `primaryRead` posture exposes only the helpers that honor it. + * declaring a `primaryRead` posture exposes only the helpers that honor it; + * a role declaring `phase: "execution"` exposes them Gated. */ type BoundReads = { readonly [R in ReadRole]: ReadPort; } & { - readonly [R in GraphqlReadRole]: BoundGraphqlRead; + readonly [R in GraphqlReadRole]: GraphqlReadPort; }; +/** A GraphQL read's bound helpers, Gated when the declaration is execution-phase. */ +type GraphqlReadPort = O extends { readonly phase: "execution" } + ? Gated> + : BoundGraphqlRead; + /** * The helpers a read role exposes, narrowed by its declaration: an advisory * read (no failure may abort the section) offers only tryCall, a "denied" - * primary read only the throwing helpers, an "absent" one only the tolerant. + * primary read only the throwing helpers, an "absent" one only the tolerant; + * an execution-phase read offers its set Gated behind the ExecTools token. */ -type ReadPort = E extends { readonly advisory: true } +type ReadPort = E extends { readonly phase: "execution" } + ? Gated> + : PlanReadPort; + +type PlanReadPort = E extends { readonly advisory: true } ? Pick, "tryCall"> : E extends { readonly primaryRead: { notFound: "denied" } } ? Pick, "call" | "listAll" | "listAllEnveloped"> @@ -292,6 +327,14 @@ interface PlannedOpBase { * the hook stores it. It must not render; a throw fails the operation. */ readonly capture?: (response: unknown) => void; + /** + * Execution-time reads run before this operation's request is sealed and + * issued (bypass actors' node ids, pinned ahead of the first write so a + * bad input fails while live state is untouched). Never runs in check + * mode, like every Late facet. A throw fails the operation with its + * request never sent. + */ + readonly before?: Late; } /** @@ -316,7 +359,7 @@ export function driftOf(op: Pick): readonly string[] { * A request facet sealed at execution time, the ONLY place a plan may touch * a secret; async so it can read a value an earlier operation created. */ -type Late = (exec: ExecTools) => T | Promise; +export type Late = (exec: ExecTools) => T | Promise; /** * What a tolerated status means for the operation that met it (it did not @@ -491,7 +534,7 @@ function boundReads( // The helpers take a SectionContext; reads are the check arm's whole // capability, so that is the arm they get. const ctx: SectionContext = { api, repo, check: true }; - const port: Record | BoundGraphqlRead> = {}; + const port: Record = {}; for (const [role, declaration] of Object.entries(meta.endpoints)) { if (endpointMethod(declaration.route) !== "GET") { continue; @@ -505,7 +548,7 @@ function boundReads( listAllEnveloped: (envelopeKey, ...args) => listAllEnveloped(ctx, meta, endpoint, envelopeKey, ...args), }; - port[role] = bound; + port[role] = endpoint.phase === "execution" ? gated(bound) : bound; } for (const [role, declaration] of Object.entries(meta.graphql ?? {})) { if (declaration.kind !== "read") { @@ -522,7 +565,7 @@ function boundReads( listGraphqlConnection(ctx, meta, op, variables), }), }; - port[role] = bound; + port[role] = op.phase === "execution" ? gated(bound) : bound; } return Object.freeze(port) as BoundReads; } diff --git a/test/e2e/fuzz.ts b/test/e2e/fuzz.ts index 1f67302..cc633ca 100644 --- a/test/e2e/fuzz.ts +++ b/test/e2e/fuzz.ts @@ -66,6 +66,7 @@ import { import { Rng } from "./prng.js"; import { checkLeaks, + failureArtifacts, insertReplay, markReportTitle, parseReposResult, @@ -426,7 +427,7 @@ async function runPredicted( // every failure. return iterationResult(problems, { - artifactDir: report.artifactDir, + artifactDir: failureArtifacts(scenario, report, problems), sections: meta.sections, coverage: witnessCoverage(report.requests, meta, observed), faultClass, @@ -553,7 +554,7 @@ async function rejectionIteration(seed: number, spec: RejectionSpec): Promise> = { "repository.gToggles": G_READ, "repository.gProbe": G_READ_TOLERANT, + "repository.gNodeId": G_READ_EXECUTION, "repository.gUpdate": G_WRITE, }; @@ -79,6 +86,9 @@ const HANDLERS: Record = { data: { repository: { id: String(state.repo.node_id) } }, }), "repository.gProbe": () => ({ errors: [{ type: "NOT_FOUND", message: "feature off" }] }), + "repository.gNodeId": ({ state }) => ({ + data: { repository: { id: String(state.repo.node_id) } }, + }), "repository.gUpdate": ({ state, variables }) => { state.repo.has_wiki = variables.hasWiki === true; return { data: { updateRepository: { clientMutationId: null } } }; @@ -224,6 +234,19 @@ describe("GraphQL check-mode barrier", () => { expect(result.violation).toBeUndefined(); expect(result.response.status).toBe(200); }); + + test("an execution-phase read is a violation in check mode and passes in apply", () => { + const inCheck = dispatch( + G_READ_EXECUTION, + { owner: OWNER, repo: REPO }, + options(scenario({ inputs: { mode: "check" } })), + ); + expect(inCheck.violation).toBe("GraphQL execution-phase read in check mode (RepoNodeId)"); + expect(inCheck.response.status).toBe(400); + const inApply = dispatch(G_READ_EXECUTION, { owner: OWNER, repo: REPO }, options(scenario())); + expect(inApply.violation).toBeUndefined(); + expect(inApply.response.status).toBe(200); + }); }); describe("GraphQL denial styles", () => { @@ -427,7 +450,7 @@ describe("GraphQL response guard and chaos", () => { describe("assertGraphqlHandlerCompleteness", () => { test("both drift directions fail loudly", () => { expect(() => assertGraphqlHandlerCompleteness(OPS, {})).toThrow( - /GraphQL operations with no mock handler: \[repository\.gProbe \(add it in src\/sections\/repository\/mock\.ts/, + /GraphQL operations with no mock handler: \[repository\.gNodeId \(add it in src\/sections\/repository\/mock\.ts/, ); expect(() => assertGraphqlHandlerCompleteness({}, HANDLERS)).toThrow( /GraphQL handlers naming no declared operation/, diff --git a/test/e2e/mock/routes.ts b/test/e2e/mock/routes.ts index 64a7477..f4ff345 100644 --- a/test/e2e/mock/routes.ts +++ b/test/e2e/mock/routes.ts @@ -230,6 +230,9 @@ export function handleGraphqlRequest( if (options.checkMode && op.kind !== "read") { return violationFor(graphqlLog)(`GraphQL write in check mode (${op.name})`); } + if (options.checkMode && op.phase === "execution") { + return violationFor(graphqlLog)(`GraphQL execution-phase read in check mode (${op.name})`); + } // 4. Target/state resolution, before the fault barrier so a fault can // never mask an unknown-target violation. A MUTATION resolves its target @@ -555,6 +558,11 @@ export function runPipeline( if (options.checkMode && request.method !== "GET") { return violation(`write in check mode: ${request.method} ${pathname} (endpoint "${key}")`); } + // Its sibling for a read a plan may issue only while executing: check mode + // runs no thunk, so one arriving here means a plan() body called it. + if (options.checkMode && endpoint.phase === "execution") { + return violation(`execution-phase read in check mode: GET ${pathname} (endpoint "${key}")`); + } // Resolve the working state and permission mask for this request. In // single-repo mode both come from the one MockState and the scenario mask; in diff --git a/test/e2e/mock/server.test.ts b/test/e2e/mock/server.test.ts index ff67cfa..8ea947c 100644 --- a/test/e2e/mock/server.test.ts +++ b/test/e2e/mock/server.test.ts @@ -641,6 +641,22 @@ describe("check-mode barrier", () => { expect(h.violations.some((v) => v.startsWith("write in check mode"))).toBe(true); }); + test("an execution-phase GET in check mode is a violation, and passes in apply", async () => { + // The branches App lookup is declared execution-phase: only a thunk may + // issue it, and check mode runs no thunk. + const appPath = "/apps/deploy-gate"; + const inCheck = await start(scenario({ inputs: { mode: "check" } })); + const res = await call(inCheck, "GET", appPath); + expect(res.status).toBe(400); + expect((await json(res)).message).toContain("execution-phase read in check mode"); + expect(inCheck.violations).toEqual([ + 'execution-phase read in check mode: GET /apps/deploy-gate (endpoint "branches.appLookup")', + ]); + const inApply = await start(scenario()); + expect((await call(inApply, "GET", appPath)).status).toBe(200); + expect(inApply.violations).toHaveLength(0); + }); + test("a faulted write in check mode is STILL a check-mode violation (barrier runs before faults)", async () => { // The check-mode barrier runs before the fault barrier, so a synthetic fault // cannot mask the write the engine should never have sent in check mode. diff --git a/test/e2e/oracle.ts b/test/e2e/oracle.ts index 256eae4..0715a45 100644 --- a/test/e2e/oracle.ts +++ b/test/e2e/oracle.ts @@ -11,9 +11,9 @@ import type { SectionKey } from "../../src/schema.js"; import { type DenialPosture, denialPosture, + planningReads, type ReadGating, readGating, - sectionOperations, } from "../../src/sections/contract/module.js"; import type { SectionPermission } from "../../src/sections/contract/permissions.js"; import { SECTIONS } from "../../src/sections/registry.js"; @@ -67,9 +67,7 @@ const ORG_ONLY_SECTIONS: ReadonlySet = new Set( * set. */ export const NO_READ_SECTIONS: ReadonlySet = new Set( - SECTIONS.filter((section) => sectionOperations(section).every((op) => op.wire === "write")).map( - (section) => section.key, - ), + SECTIONS.filter((section) => planningReads(section).length === 0).map((section) => section.key), ); /** Map a section's repo resources to the mask keys they use (org is separate). */ diff --git a/test/e2e/runner.test.ts b/test/e2e/runner.test.ts index 370e62c..36ff168 100644 --- a/test/e2e/runner.test.ts +++ b/test/e2e/runner.test.ts @@ -9,15 +9,18 @@ import { checkLeaks, declaredBuildBundleScript, exitCodeFailure, + failureArtifacts, forbiddenPresent, insertReplay, isSubsequence, markReportTitle, parseGithubOutput, parseSummaryOutcomes, + type ScenarioReport, stripDebugLines, stripMaskLines, } from "./runner.js"; +import type { Scenario } from "./schema.js"; describe("bundle build parity (harness vs production)", () => { test("the declared build:bundle script matches what the harness builds", () => { @@ -328,3 +331,101 @@ describe("markReportTitle (counterfactual disambiguation)", () => { } }); }); + +describe("failureArtifacts (a verdict the runner did not reach)", () => { + const scenario: Scenario = { + name: "fuzz-oracle-42", + tiers: ["mock"], + settings: {}, + inputs: {}, + denial_style: "fine_grained", + owner_kind: "org", + expect: { exit_code: 0 }, + }; + const passed: ScenarioReport = { + scenario: scenario.name, + ok: true, + failures: [], + exitCode: 0, + outputs: {}, + summary: "", + stdout: "", + stderr: "", + requests: [], + faultsFired: {}, + reposResult: {}, + reruns: [], + }; + + test("a run the runner passed but a caller failed gets a report.md listing the caller's failures", () => { + const dir = failureArtifacts(scenario, passed, [ + 'branches: observed "failed" not in predicted {clean,drift}', + ]); + expect(dir).toBeDefined(); + try { + const lines = readFileSync(join(dir as string, "report.md"), "utf8").split("\n"); + expect(lines[0]).toBe("# fuzz-oracle-42"); + expect(lines).toContain('- branches: observed "failed" not in predicted {clean,drift}'); + // The same directory the runner's own dump writes, so the fuzz-issue + // action's upload step finds it. + expect(dir).toContain(join("test", "e2e", ".artifacts")); + } finally { + rmSync(dir as string, { recursive: true, force: true }); + } + }); + + test("no failures means no directory, and the runner's own dump is never duplicated", () => { + expect(failureArtifacts(scenario, passed, [])).toBeUndefined(); + const dumped = { ...passed, ok: false, artifactDir: "/already/dumped" }; + expect(failureArtifacts(scenario, dumped, [])).toBe("/already/dumped"); + }); + + test.each<[label: string, runnerFailures: string[], callerFailures: string[], listed: string[]]>([ + [ + "a caller failure on top of the runner's", + ["exit code 1 != expected 0"], + ["exit code 1 != expected 0", "labels: observed skipped, predicted failed"], + ["- exit code 1 != expected 0", "- labels: observed skipped, predicted failed"], + ], + [ + "a duplicated runner failure beside a new caller failure", + ["leak", "leak"], + ["oracle: drift predicted"], + ["- leak", "- oracle: drift predicted"], + ], + ])( + "%s is merged into the existing report.md, deduplicated", + (_label, runnerFailures, callerFailures, listed) => { + const dir = mkdtempSync(join(tmpdir(), "failure-artifacts-")); + try { + writeFileSync(join(dir, "report.md"), "# stale\n"); + const dumped: ScenarioReport = { + ...passed, + ok: false, + exitCode: 1, + failures: runnerFailures, + artifactDir: dir, + }; + expect(failureArtifacts(scenario, dumped, callerFailures)).toBe(dir); + const lines = readFileSync(join(dir, "report.md"), "utf8").split("\n"); + expect(lines[0]).toBe("# fuzz-oracle-42"); + expect(lines.filter((line) => line.startsWith("- "))).toEqual(listed); + expect(lines).toContain("Exit code: 1"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }, + ); + + test("the runner's own failures alone leave its report.md untouched", () => { + const dir = mkdtempSync(join(tmpdir(), "failure-artifacts-")); + try { + writeFileSync(join(dir, "report.md"), "# the runner's own\n"); + const dumped: ScenarioReport = { ...passed, ok: false, failures: ["leak"], artifactDir: dir }; + expect(failureArtifacts(scenario, dumped, ["leak"])).toBe(dir); + expect(readFileSync(join(dir, "report.md"), "utf8")).toBe("# the runner's own\n"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/test/e2e/runner.ts b/test/e2e/runner.ts index 06df9af..5cbe503 100644 --- a/test/e2e/runner.ts +++ b/test/e2e/runner.ts @@ -832,11 +832,14 @@ export function markReportTitle(artifactDir: string, marker: string): void { * Write a failing scenario's inputs and observed I/O for debugging, and * return the directory so the CLI can point the reader at it. Keyed by * scenario name and pid so parallel or repeated runs never collide. + * `failures` is what report.md lists: the runner's own by default, or a + * caller's judgment of a run the runner itself passed (the fuzz oracle's). */ function dumpArtifacts( scenario: Scenario, report: ScenarioReport, requests: LoggedRequest[], + failures: readonly string[] = report.failures, ): string { // Sanitize the scenario name to [a-z0-9-] so it cannot escape the .artifacts // root or collide via odd characters; a per-process counter disambiguates @@ -855,6 +858,17 @@ function dumpArtifacts( writeFileSync(join(dir, "stderr.txt"), report.stderr); writeFileSync(join(dir, "summary.md"), report.summary); writeFileSync(join(dir, "requests.json"), JSON.stringify(requests, null, 2)); + writeReport(dir, scenario, report, failures); + return dir; +} + +/** The report.md the fuzz-issue action reads: title, directory, failures, exit code. */ +function writeReport( + dir: string, + scenario: Scenario, + report: ScenarioReport, + failures: readonly string[], +): void { const md = [ `# ${scenario.name}`, "", @@ -862,10 +876,35 @@ function dumpArtifacts( "", "## Failures", "", - ...report.failures.map((f) => `- ${f.replace(/\n/g, "\n ")}`), + ...failures.map((f) => `- ${f.replace(/\n/g, "\n ")}`), "", `Exit code: ${report.exitCode}`, ].join("\n"); writeFileSync(join(dir, "report.md"), `${md}\n`); - return dir; +} + +/** + * The artifact directory a failing run reports, its report.md listing every + * failure judged against the run: the runner's own (which dumped the + * directory) and a caller's on top (the fuzz oracle's verdicts), or a fresh + * directory when only the caller failed a run the runner passed. The + * nightly's fuzz-issue action reads report.md, not the log, so a failure + * missing from it is a failure it cannot report. + */ +export function failureArtifacts( + scenario: Scenario, + report: ScenarioReport, + failures: readonly string[], +): string | undefined { + if (failures.length === 0) { + return report.artifactDir; + } + if (report.artifactDir === undefined) { + return dumpArtifacts(scenario, report, report.requests, failures); + } + const extra = failures.filter((failure) => !report.failures.includes(failure)); + if (extra.length > 0) { + writeReport(report.artifactDir, scenario, report, [...new Set(report.failures), ...extra]); + } + return report.artifactDir; } diff --git a/test/engine/execute.test.ts b/test/engine/execute.test.ts index eea8dc7..8c51a7e 100644 --- a/test/engine/execute.test.ts +++ b/test/engine/execute.test.ts @@ -422,6 +422,53 @@ describe("executePlan", () => { expect(captured).toBe(false); }); + test("a before hook runs ahead of the request, and its throw fails the operation with nothing sent", async () => { + // The hook reads through the plan's port (an input a later operation + // needs, pinned ahead of this write); its failure leaves the request unsent. + const api = new MockApi({ + "GET /repos/o/r/labels?per_page=100&page=1": { data: [{ name: "live" }] }, + }).allowMutations("POST /repos/o/r/labels", "GRAPHQL ExecutorWrite"); + const port = planContext(SECTION, api, REPO).read; + const seen: string[] = []; + const plan: SectionPlan = { + ops: [ + { + role: "create", + before: async () => { + seen.push(JSON.stringify(await port.list.listAll())); + }, + payload: { name: "bug" }, + drift: ["labels[bug]: missing"], + change: 'created label "bug"', + }, + { + role: "write", + before: () => { + throw new Error("the actor does not exist"); + }, + variables: {}, + drift: ["toggle off"], + change: "flipped the toggle", + }, + ], + notes: [], + drift: [], + }; + const execution = await executePlan(plan, SECTION, api, REPO, TOOLS); + expect(execution).toMatchObject({ + status: "failed", + changes: ['created label "bug"'], + landed: 1, + }); + expect(errorOf(execution)).toContain("the actor does not exist"); + expect(seen).toEqual(['[{"name":"live"}]']); + // The read ran before the first write; the second write never left. + expect(api.calls.map((c) => `${c.method} ${c.path}`)).toEqual([ + "GET /repos/o/r/labels?per_page=100&page=1", + "POST /repos/o/r/labels", + ]); + }); + test("a tolerated status renders the operation's own outcome, never throwFor's", async () => { // One tolerated 409 turns into a note (the plan goes on, no change line); // the next turns into the section's own failure advice. Neither reaches diff --git a/test/sections/contract.test.ts b/test/sections/contract.test.ts index ca57bc2..83c10f2 100644 --- a/test/sections/contract.test.ts +++ b/test/sections/contract.test.ts @@ -6,10 +6,11 @@ import { toleratedStatuses, } from "../../src/sections/contract/endpoints.js"; import { PermissionDenied, throwFor } from "../../src/sections/contract/errors.js"; -import type { GraphqlOpDecl } from "../../src/sections/contract/graphql.js"; +import { type GraphqlOpDecl, graphqlOp } from "../../src/sections/contract/graphql.js"; import { denialPosture, endpointPermission, + planningReads, readGating, type SectionMeta, sectionGrant, @@ -55,7 +56,13 @@ describe("sectionOperations", () => { undeclaredDefault: "untouched", }; expect(sectionOperations(graphqlOnly)).toEqual([ - { role: "read", wire: "read", grade: "read", permission: { repo: ["administration"] } }, + { + role: "read", + wire: "read", + grade: "read", + permission: { repo: ["administration"] }, + phase: "plan", + }, ]); }); @@ -69,18 +76,21 @@ describe("sectionOperations", () => { // endpoint overrides accessGrade, so wire and grade coincide here; the // override split is pinned by the overrides test below. expect(Object.keys(repositorySection.graphql ?? {}).length).toBeGreaterThan(0); + const phaseOf = (op: EndpointDecl | GraphqlOpDecl): "plan" | "execution" => op.phase ?? "plan"; expect(sectionOperations(repositorySection)).toEqual([ ...Object.entries(repositorySection.endpoints).map(([role, op]) => ({ role, wire: endpointKind(op), grade: endpointKind(op), permission: endpointPermission(repositorySection, op), + phase: phaseOf(op), })), ...Object.entries(repositorySection.graphql ?? {}).map(([role, op]) => ({ role, wire: op.kind, grade: op.kind, permission: endpointPermission(repositorySection, op), + phase: phaseOf(op), })), ]); }); @@ -100,9 +110,70 @@ describe("sectionOperations", () => { undeclaredDefault: "untouched", }; expect(sectionOperations(overridden)).toEqual([ - { role: "gatedList", wire: "read", grade: "write", permission: { repo: ["administration"] } }, - { role: "read", wire: "read", grade: "read", permission: "none" }, + { + role: "gatedList", + wire: "read", + grade: "write", + permission: { repo: ["administration"] }, + phase: "plan", + }, + { role: "read", wire: "read", grade: "read", permission: "none", phase: "plan" }, + ]); + }); + + test("an execution-phase read is not a planning read: a section with only that read plans read-free", () => { + // The shape a write-only section gains when a mutation input needs a node + // id: check mode and preflight never meet the lookup, so the gating, + // the posture (no primaryRead to declare), and the oracle's no-read set + // all read the section as one that issues no read while planning. + const writeWithLookup = { + key: "repository", + permission: { repo: ["administration"] }, + undeclaredDefault: "untouched", + endpoints: { + app: { + route: "GET /apps/{app_slug}", + statuses: { 200: "the App" }, + permission: "none", + phase: "execution", + }, + put: { + route: "PATCH /repos/{owner}/{repo}", + statuses: { 200: "updated" }, + }, + }, + graphql: { lookup: { ...readOp, phase: "execution" } }, + } as const satisfies SectionMeta; + expect(sectionOperations(writeWithLookup)).toEqual([ + { role: "app", wire: "read", grade: "read", permission: "none", phase: "execution" }, + { + role: "put", + wire: "write", + grade: "write", + permission: { repo: ["administration"] }, + phase: "plan", + }, + { + role: "lookup", + wire: "read", + grade: "read", + permission: { repo: ["administration"] }, + phase: "execution", + }, ]); + expect(planningReads(writeWithLookup)).toEqual([]); + expect(readGating(writeWithLookup)).toBe("plain"); + expect(denialPosture(writeWithLookup)).toBe("absent"); + // An execution-phase read can carry no posture: plan() never meets its denial. + const postured: SectionMeta = { + ...writeWithLookup, + endpoints: { + app: { ...writeWithLookup.endpoints.app, primaryRead: { notFound: "denied" } }, + }, + }; + expect(() => denialPosture(postured)).toThrow( + /BUG: repository declares primaryRead on the execution-phase read GET \/apps\/\{app_slug\}/, + ); }); }); @@ -673,6 +744,65 @@ describe("planContext read port", () => { ctx.read.probe.call; expect(typeof ctx.read.probe.tryCall).toBe("function"); }); + + test("an execution-phase read demands the ExecTools token only a thunk holds, REST and GraphQL alike", async () => { + const gated = { + key: "branches", + permission: { repo: ["administration"] }, + undeclaredDefault: "untouched", + endpoints: { + app: { + route: "GET /apps/{app_slug}", + statuses: { 200: "the App" }, + permission: "none", + phase: "execution", + }, + plain: { + route: "GET /repos/{owner}/{repo}/branches", + statuses: { 200: "the branches" }, + }, + }, + graphql: { + repo: graphqlOp<{ owner: string; repo: string }>()({ + name: "GateProbe", + kind: "read", + phase: "execution", + query: "query GateProbe($owner: String!, $repo: String!) { repository { id } }", + outcomes: { ok: "the repository" }, + }), + }, + } as const satisfies SectionMeta; + const api = new MockApi({ + "GET /apps/deploy-gate": { data: { node_id: "A_1" } }, + "GET /repos/o/r/branches": { data: [] }, + "GRAPHQL GateProbe": { data: { repository: { id: "R_1" } } }, + }); + const ctx = planContext(gated, api, REPO); + const exec = { + resolveSecret: (): string => { + throw new Error("no secrets here"); + }, + }; + // The first parameter is the token; a plan() body, holding none, cannot + // spell the call. The ungated read beside them is the control. + // @ts-expect-error a request options object is not the token + const forgedRest: Parameters[0] = { params: { app_slug: "x" } }; + // @ts-expect-error the variables are not the token either + const forgedGraphql: Parameters[0] = { owner: "o", repo: "r" }; + expect([forgedRest, forgedGraphql].length).toBe(2); + expect(await ctx.read.app.call(exec, { params: { app_slug: "deploy-gate" } })).toEqual({ + node_id: "A_1", + }); + expect(await ctx.read.repo.call(exec, { owner: "o", repo: "r" })).toEqual({ + repository: { id: "R_1" }, + }); + expect(await ctx.read.plain.call()).toEqual([]); + expect(api.calls.map((c) => c.path)).toEqual([ + "/apps/deploy-gate", + "GateProbe", + "/repos/o/r/branches", + ]); + }); }); describe("plainData", () => { diff --git a/test/sections/plan-idempotence.test.ts b/test/sections/plan-idempotence.test.ts index 6764ace..e0f3ff7 100644 --- a/test/sections/plan-idempotence.test.ts +++ b/test/sections/plan-idempotence.test.ts @@ -281,6 +281,7 @@ describe("identityOf", () => { payload: () => ({ encrypted_value: "x" }), change: () => "set A", capture: () => {}, + before: () => {}, tolerate: { statuses: [409], outcome: () => ({ note: "" }) }, }; const again: Op = { @@ -288,6 +289,7 @@ describe("identityOf", () => { payload: () => ({ encrypted_value: "y" }), change: () => "set B", capture: () => {}, + before: async () => {}, tolerate: { statuses: [409], outcome: () => ({ failure: "" }) }, }; expect(identityOf(rebuilt)).toEqual(identityOf(again)); @@ -305,6 +307,7 @@ describe("identityOf", () => { ["drift", { ...base, drift: ["stale"] }], ["a string change", { ...base, change: "set B" }], ["capture presence", { ...base, capture: () => {} }], + ["before presence", { ...base, before: () => {} }], [ "tolerated statuses", { ...base, tolerate: { statuses: [422], outcome: () => ({ note: "" }) } }, diff --git a/test/sections/plan-idempotence.ts b/test/sections/plan-idempotence.ts index b8066fd..9731bd8 100644 --- a/test/sections/plan-idempotence.ts +++ b/test/sections/plan-idempotence.ts @@ -38,7 +38,8 @@ const SEALED = Symbol("a thunk the plan builds afresh on every pass"); * a value comparison). A thunk's identity is that it exists - what it seals * is a secret the plan is not allowed to expose - so it folds to a marker, * as does a change thunk (it renders from a response the plan has not - * seen). A capture hook counts by presence, a tolerance by its statuses. + * seen). A capture hook counts by presence, as does a before hook (what it + * reads is execution-time state), a tolerance by its statuses. */ export function identityOf(op: SectionPlan["ops"][number]): unknown { const sealed = (value: unknown): unknown => (typeof value === "function" ? SEALED : value); @@ -52,6 +53,7 @@ export function identityOf(op: SectionPlan["ops"][number]): unknown { change: sealed(op.change), describe: op.describe, capture: op.capture !== undefined, + before: op.before !== undefined, tolerate: op.tolerate === undefined ? undefined : { statuses: op.tolerate.statuses }, }; } diff --git a/test/sections/registry.test.ts b/test/sections/registry.test.ts index a366be5..0f20bf5 100644 --- a/test/sections/registry.test.ts +++ b/test/sections/registry.test.ts @@ -17,11 +17,11 @@ import { defaultUndeclaredPolicy, denialPosture, endpointPermission, + planningReads, type SectionContext, type SectionMeta, type SectionModule, sectionGrant, - sectionOperations, } from "../../src/sections/contract/module.js"; import { grantFor, type SectionPermission } from "../../src/sections/contract/permissions.js"; import type { PlanContext, SectionPlan } from "../../src/sections/contract/plan.js"; @@ -856,7 +856,7 @@ describe("handler contracts", () => { const primaries = Object.entries(section.endpoints).filter( ([, endpoint]) => endpoint.primaryRead !== undefined, ); - const reads = sectionOperations(section).some((op) => op.wire === "read"); + const reads = planningReads(section).length > 0; const posture = denialPosture(section); expect(primaries.length, `${section.key} primaryRead declarations`).toBe(reads ? 1 : 0); if (!reads) { @@ -879,10 +879,12 @@ describe("handler contracts", () => { declaring.push(section.key); } } - // Every section with a REST read declares exactly one primaryRead. + // Every section with a REST read it issues while planning declares exactly one primaryRead. expect(declaring).toEqual( SECTIONS.filter((s) => - Object.values(s.endpoints).some((e) => endpointMethod(e.route) === "GET"), + Object.values(s.endpoints).some( + (e) => endpointMethod(e.route) === "GET" && e.phase !== "execution", + ), ).map((s) => s.key), ); });