From 9d5f9eac1853bcbe3d0f18382ab2822dd8a6d86e Mon Sep 17 00:00:00 2001 From: ArchonVII Date: Thu, 28 May 2026 19:01:39 -0500 Subject: [PATCH] feat(pr-policy): add role separation gate Add a pure role-policy helper with tests and wire pr-policy.yml to warn by default while allowing protected agent-managed PRs to opt into hard Release-Admiral enforcement. Document the new inputs in README and the example caller. Closes #26 --- .github/workflows/pr-policy.yml | 160 ++++++++++++++++++++++++++- README.md | 33 +++++- examples/pr-policy.yml | 17 +++ scripts/role-policy.mjs | 189 ++++++++++++++++++++++++++++++++ scripts/role-policy.test.mjs | 98 +++++++++++++++++ 5 files changed, 492 insertions(+), 5 deletions(-) create mode 100644 scripts/role-policy.mjs create mode 100644 scripts/role-policy.test.mjs diff --git a/.github/workflows/pr-policy.yml b/.github/workflows/pr-policy.yml index 22bc8a0..7eb56f6 100644 --- a/.github/workflows/pr-policy.yml +++ b/.github/workflows/pr-policy.yml @@ -57,13 +57,48 @@ on: workflow-library-ref: description: | Git ref (tag, branch, or SHA) of ArchonVII/github-workflows used to - source scripts/parse-evidence.mjs. MUST match the ref the caller + source policy helper scripts. MUST match the ref the caller pins this reusable workflow to (e.g. @v1). Defaulting to v1 keeps parser+workflow versioned together; callers using @main or a SHA should pass workflow-library-ref to match. Source: PR #19 review patch 2 (2026-05-20). type: string default: v1 + role-separation-warnings: + description: | + Emit warnings when PR metadata suggests the same account authored + and is preparing to close agent-managed work. This is warning-only + by default per ArchonVII/.github#14: same-account identity is not a + universal hard block in a solo-owner ecosystem. + type: boolean + default: true + enforce-role-separation: + description: | + Hard-fail agent-managed PRs that touch protected paths unless an + independent approval or non-author Release-Admiral marker is present. + Defaults false so consumers opt into the scoped hard block after + adopting the role policy. + type: boolean + default: false + role-protected-paths: + description: | + Newline-separated path patterns that require independent + Release-Admiral closure when enforce-role-separation is true. + Supports exact paths, trailing / prefixes, and trailing /** prefixes. + type: string + default: | + .github/** + .githooks/** + .agent/** + AGENTS.md + CLAUDE.md + GEMINI.md + package.json + package-lock.json + pnpm-lock.yaml + yarn.lock + src/** + scripts/** jobs: policy: @@ -83,8 +118,8 @@ jobs: contents: read steps: # Check out THIS reusable workflow's repo (ArchonVII/github-workflows) - # into a sibling path so the evidence-check step can dynamic-import - # scripts/parse-evidence.mjs. We deliberately don't check out the caller + # into a sibling path so policy steps can dynamic-import helper scripts + # from scripts/*.mjs. We deliberately don't check out the caller # repo here — the existing policy step uses the GitHub API and doesn't # need a working tree. # @@ -106,7 +141,7 @@ jobs: # "classify PR" step that exports outputs for both subsequent # steps to gate on. Source: PR #19 adversarial review # (2026-05-20), finding M1. - - name: Check out github-workflows for parser script + - name: Check out github-workflows for policy helper scripts if: github.event.pull_request.draft == false uses: actions/checkout@v4 with: @@ -176,6 +211,123 @@ jobs: core.setFailed(failures.join('\n')); } + # ---------------------------------------------------------------------- + # F7 role-separation policy (amendment 2026-05-28). + # + # Same-account author/merger identity is a soft warning by default, not a + # universal hard block. Callers may opt into scoped hard enforcement for + # agent-managed PRs touching protected paths via enforce-role-separation. + # + # Owner Maintenance Lane is direct-commit-only and never produces a PR, so + # it cannot reach this workflow. No PR-path exemption is implemented here + # on purpose. + # ---------------------------------------------------------------------- + - name: Role separation check + if: inputs.role-separation-warnings || inputs.enforce-role-separation + uses: actions/github-script@v7 + env: + ENFORCE_ROLE_SEPARATION: ${{ inputs.enforce-role-separation }} + ROLE_PROTECTED_PATHS: ${{ inputs.role-protected-paths }} + WORKFLOW_LIBRARY_REF: ${{ inputs.workflow-library-ref }} + with: + script: | + const pr = context.payload.pull_request; + const summary = core.summary; + + if (pr.draft) { + core.info('Draft PR; role-separation policy is advisory until draft is cleared.'); + await summary + .addHeading('Role separation check (advisory)', 3) + .addRaw('Draft PR — role-separation check skipped.') + .write(); + return; + } + + const path = require('path'); + const url = require('url'); + const helperPath = path.resolve( + process.env.GITHUB_WORKSPACE, + '__github-workflows__/scripts/role-policy.mjs', + ); + const workflowRef = process.env.WORKFLOW_LIBRARY_REF || '(unset)'; + let evaluateRolePolicy; + try { + ({ evaluateRolePolicy } = await import(url.pathToFileURL(helperPath).href)); + } catch (err) { + const msg = `Could not load role policy helper at ${helperPath} (workflow-library-ref=${workflowRef}): ${err && err.message ? err.message : err}. Role separation check skipped.`; + core.warning(msg); + await summary + .addHeading('Role separation check (skipped)', 3) + .addRaw(msg) + .write(); + return; + } + + const files = await github.paginate(github.rest.pulls.listFiles, { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pr.number, + per_page: 100, + }); + const commits = await github.paginate(github.rest.pulls.listCommits, { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pr.number, + per_page: 100, + }); + const reviews = await github.paginate(github.rest.pulls.listReviews, { + owner: context.repo.owner, + repo: context.repo.repo, + pull_number: pr.number, + per_page: 100, + }); + + const result = evaluateRolePolicy({ + prAuthor: pr.user && pr.user.login ? pr.user.login : '', + headRef: pr.head && pr.head.ref ? pr.head.ref : '', + files: files.map((f) => f.filename), + commitAuthors: commits.map((c) => + c.author && c.author.login + ? c.author.login + : c.commit && c.commit.author && c.commit.author.name + ? c.commit.author.name + : '', + ), + approvedReviewAuthors: reviews + .filter((r) => r.state === 'APPROVED') + .map((r) => (r.user && r.user.login ? r.user.login : '')), + body: pr.body || '', + labels: (pr.labels || []).map((l) => l.name), + enforceRoleSeparation: process.env.ENFORCE_ROLE_SEPARATION === 'true', + protectedPathPatterns: process.env.ROLE_PROTECTED_PATHS, + }); + + await summary.addHeading('Role separation check', 3).write(); + if (result.protectedPaths.length > 0) { + await summary + .addHeading('Protected paths touched', 4) + .addList(result.protectedPaths) + .write(); + } + if (result.warnings.length > 0) { + await summary + .addHeading('Warnings', 4) + .addList(result.warnings) + .write(); + for (const warning of result.warnings) core.warning(`role-policy: ${warning}`); + } + if (result.errors.length > 0) { + await summary + .addHeading('Errors', 4) + .addList(result.errors) + .write(); + core.setFailed(result.errors.join('\n')); + return; + } + if (result.warnings.length === 0 && result.errors.length === 0) { + await summary.addRaw('No role-separation concerns detected.').write(); + } + # ---------------------------------------------------------------------- # F2 + F10 evidence parser (amendment 2026-05-19). # diff --git a/README.md b/README.md index ebcedf8..dd80bab 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,7 @@ Companion repos: | Workflow | Purpose | Example | | -------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------- | -| [`pr-policy.yml`](.github/workflows/pr-policy.yml) | Enforce PR body has `## Verification` / `### Verification Notes` / a checked box / a linked issue. Doc-only PRs skip. Also runs `actionlint`. | [`examples/pr-policy.yml`](examples/pr-policy.yml) | +| [`pr-policy.yml`](.github/workflows/pr-policy.yml) | Enforce PR body has `## Verification` / `### Verification Notes` / a checked box / a linked issue. Doc-only PRs skip that ceremony. Also warns on role-separation concerns and can hard-gate protected agent PR paths. Also runs `actionlint`. | [`examples/pr-policy.yml`](examples/pr-policy.yml) | | [`pr-body-autoinject.yml`](.github/workflows/pr-body-autoinject.yml) | When a bot opens a non-doc PR with a freehand body, prepend a stub that satisfies `pr-policy`. Human PRs untouched. | [`examples/pr-body-autoinject.yml`](examples/pr-body-autoinject.yml) | | [`semantic-pr-title.yml`](.github/workflows/semantic-pr-title.yml) | Enforce Conventional Commits format on PR titles. Wraps `amannn/action-semantic-pull-request@v5`. | [`examples/semantic-pr-title.yml`](examples/semantic-pr-title.yml) | | [`branch-naming.yml`](.github/workflows/branch-naming.yml) | Enforce the `open`-skill branch convention (`agent//-` or `/`). Configurable regex. | [`examples/branch-naming.yml`](examples/branch-naming.yml) | @@ -81,8 +81,39 @@ permissions: jobs: policy: uses: ArchonVII/github-workflows/.github/workflows/pr-policy.yml@v1 + # Optional: fail agent-managed PRs touching protected paths unless an + # independent approval or non-author Release-Admiral marker is present. + # Defaults are warning-only per ArchonVII/.github#14. + # with: + # enforce-role-separation: true ``` +### PR role-separation policy + +`pr-policy.yml` includes the F7 role-separation check from +[`ArchonVII/.github#14`](https://github.com/ArchonVII/.github/issues/14). +By default, the workflow emits warnings when PR metadata suggests the same +account both authored and is preparing to close agent-managed work. This is not +a universal hard block because the ArchonVII solo-owner workflow permits Joseph +to be the human merging account for legitimate work. + +Consumers that want scoped hard enforcement can set: + +```yaml +with: + enforce-role-separation: true +``` + +When enabled, agent-managed PRs touching protected paths must have either an +independent approving review or a PR-body marker such as +`Release-Admiral: @reviewer-name`, where the marker names a non-author. The +default protected path set covers `.github/**`, `.githooks/**`, `.agent/**`, +agent authority docs, package manifests/locks, `src/**`, and `scripts/**`. Use +`role-protected-paths` to override that list. + +The Owner Maintenance Lane is direct-commit-only and intentionally has no PR +exemption here. Dependabot auto-merge is the explicit exception. + --- ## Per-repo setup script diff --git a/examples/pr-policy.yml b/examples/pr-policy.yml index d9d2cbf..e9742d9 100644 --- a/examples/pr-policy.yml +++ b/examples/pr-policy.yml @@ -34,3 +34,20 @@ jobs: # require-verification-section: true # require-checked-box: true # run-actionlint: true + # # Warning-only by default. Set true to hard-fail agent-managed PRs + # # touching protected paths unless an independent approval or + # # non-author `Release-Admiral: @name` marker is present. + # enforce-role-separation: false + # # Override only if this repo's protected agent-policy surface differs + # # from the default .github/.githooks/.agent/authority/code/script set. + # role-protected-paths: | + # .github/** + # .githooks/** + # .agent/** + # AGENTS.md + # CLAUDE.md + # GEMINI.md + # package.json + # package-lock.json + # src/** + # scripts/** diff --git a/scripts/role-policy.mjs b/scripts/role-policy.mjs new file mode 100644 index 0000000..46dab2c --- /dev/null +++ b/scripts/role-policy.mjs @@ -0,0 +1,189 @@ +const DEFAULT_PROTECTED_PATTERNS = [ + '.github/**', + '.githooks/**', + '.agent/**', + 'AGENTS.md', + 'CLAUDE.md', + 'GEMINI.md', + 'package.json', + 'package-lock.json', + 'pnpm-lock.yaml', + 'yarn.lock', + 'src/**', + 'scripts/**', +]; + +/** + * Evaluate ArchonVII role-separation policy for a PR. + * + * This helper is intentionally pure. The workflow gathers PR files, commits, + * reviews, labels, and body text via the GitHub API, then passes normalized + * values here for deterministic validation. + * + * @param {object} input + * @param {string} input.prAuthor + * @param {string} input.headRef + * @param {string[]} input.files + * @param {string[]} input.commitAuthors + * @param {string[]} input.approvedReviewAuthors + * @param {string[]} input.labels + * @param {string} input.body + * @param {boolean} input.enforceRoleSeparation + * @param {string|string[]} [input.protectedPathPatterns] + * @returns {{ + * ok: boolean, + * warnings: string[], + * errors: string[], + * protectedPaths: string[], + * selfAuthored: boolean, + * agentManaged: boolean, + * independentApproval: boolean, + * releaseAdmiralMarker: string, + * dependabotExempt: boolean, + * ownerMaintenancePrExempt: false, + * }} + */ +export function evaluateRolePolicy(input) { + const { + prAuthor = '', + headRef = '', + files = [], + commitAuthors = [], + approvedReviewAuthors = [], + labels = [], + body = '', + enforceRoleSeparation = false, + protectedPathPatterns = DEFAULT_PROTECTED_PATTERNS, + } = input || {}; + + const warnings = []; + const errors = []; + const authorKey = normalizeActor(prAuthor); + const commitAuthorKeys = new Set((commitAuthors || []).map(normalizeActor).filter(Boolean)); + const protectedPatterns = parsePatterns(protectedPathPatterns); + const protectedPaths = (files || []).filter((file) => + protectedPatterns.some((pattern) => matchPattern(file, pattern)), + ); + + const dependabotExempt = isDependabot(prAuthor, headRef); + if (dependabotExempt) { + return { + ok: true, + warnings, + errors, + protectedPaths, + selfAuthored: false, + agentManaged: false, + independentApproval: false, + releaseAdmiralMarker: '', + dependabotExempt: true, + ownerMaintenancePrExempt: false, + }; + } + + const selfAuthored = Boolean(authorKey && commitAuthorKeys.has(authorKey)); + const agentManaged = isAgentManaged({ headRef, labels, body }); + const independentApproval = hasIndependentApproval({ + approvedReviewAuthors, + prAuthor, + commitAuthors, + }); + const releaseAdmiralMarker = findReleaseAdmiralMarker(body); + const markerIndependent = Boolean( + releaseAdmiralMarker && + isIndependentActor(releaseAdmiralMarker, [prAuthor, ...commitAuthors]), + ); + + if (selfAuthored) { + warnings.push( + `Role separation: PR author "${prAuthor}" also authored commits. This is a same account signal; default policy warns but does not fail.`, + ); + } + + if (/owner maintenance lane/i.test(body || '')) { + warnings.push( + 'Owner Maintenance Lane is direct-commit-only; PRs receive no owner-maintenance exemption in pr-policy.', + ); + } + + if ( + enforceRoleSeparation && + agentManaged && + protectedPaths.length > 0 && + !independentApproval && + !markerIndependent + ) { + errors.push( + `Agent-managed PR touches protected path(s) (${protectedPaths.join(', ')}) and needs independent Release-Admiral approval or a non-author Release-Admiral marker.`, + ); + } + + return { + ok: errors.length === 0, + warnings, + errors, + protectedPaths, + selfAuthored, + agentManaged, + independentApproval, + releaseAdmiralMarker, + dependabotExempt: false, + ownerMaintenancePrExempt: false, + }; +} + +function isDependabot(prAuthor, headRef) { + const author = normalizeActor(prAuthor); + return author === 'dependabot[bot]' || String(headRef || '').startsWith('dependabot/'); +} + +function isAgentManaged({ headRef, labels, body }) { + const labelText = (labels || []).join(' '); + return ( + String(headRef || '').startsWith('agent/') || + /\bagent[- ]managed\b/i.test(labelText) || + /\bProject-Lieutenant\b/i.test(body || '') || + /\bLIEUTENANT_HANDOFF\b/i.test(body || '') + ); +} + +function hasIndependentApproval({ approvedReviewAuthors, prAuthor, commitAuthors }) { + return (approvedReviewAuthors || []).some((reviewer) => + isIndependentActor(reviewer, [prAuthor, ...(commitAuthors || [])]), + ); +} + +function isIndependentActor(actor, blockedActors) { + const actorKey = normalizeActor(actor); + if (!actorKey) return false; + const blocked = new Set((blockedActors || []).map(normalizeActor).filter(Boolean)); + return !blocked.has(actorKey); +} + +function findReleaseAdmiralMarker(body) { + const match = String(body || '').match(/Release[- ]Admiral\s*:\s*(@?[A-Za-z0-9_.-]+)/i); + return match ? match[1] : ''; +} + +function normalizeActor(actor) { + return String(actor || '').trim().replace(/^@/, '').toLowerCase(); +} + +function parsePatterns(raw) { + if (Array.isArray(raw)) return raw.map(String).map((s) => s.trim()).filter(Boolean); + if (!raw) return DEFAULT_PROTECTED_PATTERNS; + return String(raw) + .split(/\r?\n/) + .map((s) => s.replace(/#.*$/, '').trim()) + .filter(Boolean); +} + +function matchPattern(path, pattern) { + if (!pattern) return false; + if (pattern.endsWith('/**')) { + const prefix = pattern.slice(0, -2); + return path === prefix.slice(0, -1) || path.startsWith(prefix); + } + if (pattern.endsWith('/')) return path.startsWith(pattern); + return path === pattern; +} diff --git a/scripts/role-policy.test.mjs b/scripts/role-policy.test.mjs new file mode 100644 index 0000000..bb61707 --- /dev/null +++ b/scripts/role-policy.test.mjs @@ -0,0 +1,98 @@ +import { describe, it, expect } from 'vitest'; +import { evaluateRolePolicy } from './role-policy.mjs'; + +const input = (over = {}) => ({ + prAuthor: 'codex', + headRef: 'agent/codex/26-role-separation-pr-policy', + files: ['README.md'], + commitAuthors: ['codex'], + approvedReviewAuthors: [], + body: '', + labels: [], + enforceRoleSeparation: false, + ...over, +}); + +describe('evaluateRolePolicy - soft warning default', () => { + it('warns but does not fail when the PR author also authored commits', () => { + const result = evaluateRolePolicy(input()); + + expect(result.ok).toBe(true); + expect(result.errors).toEqual([]); + expect(result.warnings.some((w) => /same account/i.test(w))).toBe(true); + }); + + it('does not create a PR-path owner-maintenance exemption', () => { + const result = evaluateRolePolicy(input({ + files: ['docs/research/notes.md'], + body: 'Owner Maintenance Lane', + })); + + expect(result.ok).toBe(true); + expect(result.ownerMaintenancePrExempt).toBe(false); + expect(result.warnings.some((w) => /direct-commit-only/i.test(w))).toBe(true); + }); +}); + +describe('evaluateRolePolicy - protected hard block', () => { + it('fails agent-managed protected-path PRs without independent approval or marker', () => { + const result = evaluateRolePolicy(input({ + enforceRoleSeparation: true, + files: ['.github/workflows/pr-policy.yml'], + })); + + expect(result.ok).toBe(false); + expect(result.protectedPaths).toEqual(['.github/workflows/pr-policy.yml']); + expect(result.errors.some((e) => /Release-Admiral/i.test(e))).toBe(true); + }); + + it('passes protected-path PRs with an independent approval', () => { + const result = evaluateRolePolicy(input({ + enforceRoleSeparation: true, + files: ['scripts/setup-repo.mjs'], + approvedReviewAuthors: ['release-admiral'], + })); + + expect(result.ok).toBe(true); + expect(result.errors).toEqual([]); + expect(result.independentApproval).toBe(true); + }); + + it('passes protected-path PRs with a non-author Release-Admiral marker', () => { + const result = evaluateRolePolicy(input({ + enforceRoleSeparation: true, + files: ['AGENTS.md'], + body: 'Release-Admiral: @noether', + })); + + expect(result.ok).toBe(true); + expect(result.errors).toEqual([]); + expect(result.releaseAdmiralMarker).toBe('@noether'); + }); + + it('does not hard-fail unprotected docs-only paths', () => { + const result = evaluateRolePolicy(input({ + enforceRoleSeparation: true, + files: ['docs/usage.md'], + })); + + expect(result.ok).toBe(true); + expect(result.errors).toEqual([]); + expect(result.protectedPaths).toEqual([]); + }); + + it('exempts Dependabot from role-separation warnings and hard blocks', () => { + const result = evaluateRolePolicy(input({ + prAuthor: 'dependabot[bot]', + commitAuthors: ['dependabot[bot]'], + headRef: 'dependabot/npm_and_yarn/vitest-4.2.0', + enforceRoleSeparation: true, + files: ['package-lock.json'], + })); + + expect(result.ok).toBe(true); + expect(result.errors).toEqual([]); + expect(result.warnings).toEqual([]); + expect(result.dependabotExempt).toBe(true); + }); +});