diff --git a/.github/workflows/pr-governance-tests.yml b/.github/workflows/pr-governance-tests.yml index 09b1e33..8d07c9c 100644 --- a/.github/workflows/pr-governance-tests.yml +++ b/.github/workflows/pr-governance-tests.yml @@ -24,7 +24,8 @@ jobs: run: >- node --test pr-governance/scripts/test-gate.cjs pr-governance/scripts/test-labels.cjs pr-governance/scripts/test-codex.cjs - pr-governance/scripts/test-runner.cjs pr-governance/scripts/test-nudge.cjs + pr-governance/scripts/test-main.cjs pr-governance/scripts/test-runner.cjs + pr-governance/scripts/test-nudge.cjs - name: Install the pinned scanner run: bash pr-governance/scripts/install-gitleaks.sh - name: Test the scanner diff --git a/pr-governance/README.md b/pr-governance/README.md index 34cae99..b21cae3 100644 --- a/pr-governance/README.md +++ b/pr-governance/README.md @@ -61,7 +61,10 @@ organization community-health files. ## Wiring a repository 1. Copy `.github/governance/policy.json` from `policy.default.json` and set - `owner`, `reviewer`, and repository-specific `r2_paths`. + `reviewer` and repository-specific `r2_paths`. Leave `owner` out: account + identities belong in protected configuration, so the action reads + `RISK_GATE_OWNER_ID` and `RISK_GATE_OWNER` from repository or organization + variables and replaces `owner` when they are set. 2. Copy `templates/risk-gate.yml`, `templates/codex-nudge.yml`, `templates/gitleaks.yml` and `templates/weekly-secret-scan.yml` into `.github/workflows/`, replacing `__GOVERNANCE_REPO__` with the @@ -77,8 +80,9 @@ organization community-health files. 4. Add the `SECRET_SCAN_BASELINE_B64` secret before enabling the weekly scan. The scan refuses to run without it. -`RISK_GATE_OWNER` is not read by this action; the owner is part of the policy -file, which is read from the default branch head. +The owner identity comes from the `RISK_GATE_OWNER_ID` and `RISK_GATE_OWNER` +variables. A policy that still carries `owner` keeps working until the +variables are set, but no account identity is required in the file. ## Trust model diff --git a/pr-governance/policy.default.json b/pr-governance/policy.default.json index 0caa54e..abb4451 100644 --- a/pr-governance/policy.default.json +++ b/pr-governance/policy.default.json @@ -7,10 +7,6 @@ "user_id": 199175422, "login": "chatgpt-codex-connector[bot]" }, - "owner": { - "user_id": 20649888, - "login": "Pigbibi" - }, "r0_paths": [ "README.md", "CHANGELOG.md", diff --git a/pr-governance/policy.schema.json b/pr-governance/policy.schema.json index 2263013..b9a80b3 100644 --- a/pr-governance/policy.schema.json +++ b/pr-governance/policy.schema.json @@ -9,51 +9,91 @@ "version", "mode", "reviewer", - "owner", "r0_paths", "r2_paths", "dependabot_template_exemption", "max_evidence_age_hours" ], "properties": { - "version": { "const": "pr-governance/v3" }, + "version": { + "const": "pr-governance/v3" + }, "mode": { "description": "enforce publishes and fails; report-only publishes advisory statuses and never fails the run; local publishes nothing.", - "enum": ["enforce", "local", "report-only"] + "enum": [ + "enforce", + "local", + "report-only" + ] }, "reviewer": { "description": "The only fixed identity in the gate. Reviewer-authored evidence is the sole review source.", "type": "object", "additionalProperties": false, - "required": ["provider", "app_id", "user_id", "login"], + "required": [ + "provider", + "app_id", + "user_id", + "login" + ], "properties": { - "provider": { "enum": ["codex"] }, - "app_id": { "type": "integer", "minimum": 1 }, - "user_id": { "type": "integer", "minimum": 1 }, - "login": { "type": "string", "pattern": "^[A-Za-z0-9][A-Za-z0-9-]*\\[bot\\]$" } + "provider": { + "enum": [ + "codex" + ] + }, + "app_id": { + "type": "integer", + "minimum": 1 + }, + "user_id": { + "type": "integer", + "minimum": 1 + }, + "login": { + "type": "string", + "pattern": "^[A-Za-z0-9][A-Za-z0-9-]*\\[bot\\]$" + } } }, "owner": { - "description": "The only actor whose timeline label event can claim owner-approved.", + "description": "Optional. The only actor whose label event can claim owner-approved. Prefer setting RISK_GATE_OWNER_ID and RISK_GATE_OWNER as repository or organization variables; the action then replaces this object and the policy carries no account identity.", "type": "object", "additionalProperties": false, - "required": ["user_id", "login"], + "required": [ + "user_id", + "login" + ], "properties": { - "user_id": { "type": "integer", "minimum": 1 }, - "login": { "type": "string", "minLength": 1 } + "user_id": { + "type": "integer", + "minimum": 1 + }, + "login": { + "type": "string", + "minLength": 1 + } } }, "r0_paths": { "description": "Prose paths that may be classified R0 when the change has no sensitive keyword. Trailing slash matches a prefix.", "type": "array", "uniqueItems": true, - "items": { "type": "string", "pattern": "^[^/\\\\][^\\\\]*$", "minLength": 1 } + "items": { + "type": "string", + "pattern": "^[^/\\\\][^\\\\]*$", + "minLength": 1 + } }, "r2_paths": { "description": "Repository-specific high-risk paths; added to the canonical heuristics.", "type": "array", "uniqueItems": true, - "items": { "type": "string", "pattern": "^[^/\\\\][^\\\\]*$", "minLength": 1 } + "items": { + "type": "string", + "pattern": "^[^/\\\\][^\\\\]*$", + "minLength": 1 + } }, "dependabot_template_exemption": { "description": "Skip the pull request template check for verified Dependabot heads.", diff --git a/pr-governance/scripts/fixtures.cjs b/pr-governance/scripts/fixtures.cjs index 881c4ec..4401f57 100644 --- a/pr-governance/scripts/fixtures.cjs +++ b/pr-governance/scripts/fixtures.cjs @@ -18,6 +18,9 @@ const at = (offsetSeconds) => new Date((NOW + offsetSeconds) * 1000).toISOString function fixture(options = {}) { const clone = structuredClone(policy); + // The shipped policy leaves the owner to protected variables, so fixtures + // inject a synthetic one for the label and digest assertions. + clone.owner ??= { user_id: 101, login: 'synthetic-owner' }; if (options.policy) options.policy(clone); const files = options.files || [{ filename: options.path || 'src/view.js', diff --git a/pr-governance/scripts/gate.cjs b/pr-governance/scripts/gate.cjs index fc4a5e5..3468f63 100644 --- a/pr-governance/scripts/gate.cjs +++ b/pr-governance/scripts/gate.cjs @@ -8,8 +8,9 @@ const SCHEMA = 'pr-governance/v3'; const MODES = ['enforce', 'local', 'report-only']; -const POLICY_FIELDS = ['version', 'mode', 'reviewer', 'owner', 'r0_paths', 'r2_paths', +const POLICY_FIELDS = ['version', 'mode', 'reviewer', 'r0_paths', 'r2_paths', 'dependabot_template_exemption', 'max_evidence_age_hours']; +const POLICY_OPTIONAL_FIELDS = ['owner']; const REVIEWER_FIELDS = ['provider', 'app_id', 'user_id', 'login']; const OWNER_FIELDS = ['user_id', 'login']; const PROVIDERS = ['codex']; @@ -105,17 +106,19 @@ const normalizeLogin = login => String(login || '').replace(/\[bot\]$/i, '').toL // ----------------------------------------------------------------- policy --- -function sameKeys(object, expected, label) { +function sameKeys(object, expected, label, optional = []) { assert(object && typeof object === 'object' && !Array.isArray(object), `Invalid ${label}`); - const actual = Object.keys(object).sort(); - assert(actual.length === expected.length && actual.every((key, index) => key === [...expected].sort()[index]), + const allowed = new Set([...expected, ...optional]); + const actual = Object.keys(object); + assert(actual.every(key => allowed.has(key)) + && expected.every(key => Object.prototype.hasOwnProperty.call(object, key)), `Invalid ${label} fields`); } const isPositiveId = value => Number.isSafeInteger(value) && value > 0; function validatePolicy(policy) { - sameKeys(policy, POLICY_FIELDS, 'policy'); + sameKeys(policy, POLICY_FIELDS, 'policy', POLICY_OPTIONAL_FIELDS); assert(policy.version === SCHEMA, `Unsupported policy version: ${String(policy.version)}`); assert(MODES.includes(policy.mode), `Unsupported policy mode: ${String(policy.mode)}`); sameKeys(policy.reviewer, REVIEWER_FIELDS, 'reviewer'); @@ -124,9 +127,13 @@ function validatePolicy(policy) { assert(isPositiveId(policy.reviewer.user_id), 'Invalid reviewer user id'); assert(typeof policy.reviewer.login === 'string' && /^[A-Za-z0-9][A-Za-z0-9-]*\[bot\]$/.test(policy.reviewer.login), 'Invalid reviewer login'); - sameKeys(policy.owner, OWNER_FIELDS, 'owner'); - assert(isPositiveId(policy.owner.user_id), 'Invalid owner user id'); - assert(typeof policy.owner.login === 'string' && policy.owner.login.length > 0, 'Invalid owner login'); + // The owner comes from protected repository variables, so the committed + // policy may leave it out. When it is present, it must be well formed. + if (policy.owner !== undefined) { + sameKeys(policy.owner, OWNER_FIELDS, 'owner'); + assert(isPositiveId(policy.owner.user_id), 'Invalid owner user id'); + assert(typeof policy.owner.login === 'string' && policy.owner.login.length > 0, 'Invalid owner login'); + } for (const field of ['r0_paths', 'r2_paths']) { const rules = policy[field]; assert(Array.isArray(rules), `Invalid ${field}`); @@ -418,7 +425,7 @@ function labelEvidence(snapshot, policy) { // The owner label claims a human confirmation; only the configured owner id // can carry that claim. Any actor may claim agent-approved, and the actor is // recorded in the digest because the label API already requires write access. - if (name === 'owner-approved' && latest.actor?.id !== policy.owner.user_id) continue; + if (name === 'owner-approved' && latest.actor?.id !== policy.owner?.user_id) continue; if (!Number.isSafeInteger(latest.id) || latest.id < 1 || !Number.isSafeInteger(latest.actor?.id)) continue; const expected = `${name} for PR ${snapshot.pr.number} at event ${latest.id} on this head`; const bound = (snapshot.statuses || []).some(status => status.context === 'clearing-label' diff --git a/pr-governance/scripts/main.cjs b/pr-governance/scripts/main.cjs index d2f8e63..564bd66 100644 --- a/pr-governance/scripts/main.cjs +++ b/pr-governance/scripts/main.cjs @@ -5,6 +5,20 @@ const { run, recordLabel } = require('./runner.cjs'); const POLICY_PATH = '.github/governance/policy.json'; +// Account identities belong in protected configuration, not in committed +// policy files, so the owner is resolved from repository or organization +// variables and only falls back to the policy when none is configured. +function configuredOwner() { + const raw = process.env.GOVERNANCE_OWNER_ID; + const login = process.env.GOVERNANCE_OWNER_LOGIN; + if (!raw && !login) return null; + const id = Number(raw); + if (!Number.isSafeInteger(id) || id < 1 || typeof login !== 'string' || !login) { + throw new Error('Incomplete owner configuration'); + } + return { user_id: id, login }; +} + async function defaultBranch(github, context) { const known = context.payload?.repository?.default_branch || context.payload?.pull_request?.base?.ref; @@ -29,7 +43,12 @@ async function loadPolicy(github, context, branch) { } catch { throw new Error('Trusted policy is not valid JSON'); } + const owner = configuredOwner(); + if (owner) policy.owner = owner; gate.validatePolicy(policy); + // Without an owner nobody can ever satisfy owner-approved, so a missing + // configuration must fail the check instead of silently downgrading it. + if (!policy.owner) throw new Error('Owner configuration is required'); return { policy, policyCommit, policySha: gate.sha256Hex(gate.canonical(policy)) }; } diff --git a/pr-governance/scripts/test-gate.cjs b/pr-governance/scripts/test-gate.cjs index a425234..7b9f5f0 100644 --- a/pr-governance/scripts/test-gate.cjs +++ b/pr-governance/scripts/test-gate.cjs @@ -33,14 +33,21 @@ test('the shipped default policy validates', () => { assert.equal(shipped.reviewer.provider, 'codex'); }); +test('the owner is optional and resolved from protected variables', () => { + const shipped = JSON.parse(fs.readFileSync(path.join(__dirname, '..', 'policy.default.json'), 'utf8')); + assert.equal(shipped.owner, undefined); + const withOwner = { ...shipped, owner: { user_id: 101, login: 'synthetic-owner' } }; + assert.deepEqual(gate.validatePolicy(withOwner), withOwner); +}); + test('the schema file lists exactly the fields the evaluator enforces', () => { const schema = JSON.parse(fs.readFileSync(path.join(__dirname, '..', 'policy.schema.json'), 'utf8')); assert.equal(schema.$id, 'pr-governance/v3'); assert.equal(schema.additionalProperties, false); const declared = Object.keys(schema.properties).sort(); - const validated = ['version', 'mode', 'reviewer', 'owner', 'r0_paths', 'r2_paths', + const validated = ['version', 'mode', 'reviewer', 'r0_paths', 'r2_paths', 'dependabot_template_exemption', 'max_evidence_age_hours'].sort(); - assert.deepEqual(declared, validated); + assert.deepEqual(declared, [...validated, 'owner'].sort()); assert.deepEqual([...schema.required].sort(), validated); assert.deepEqual([...schema.properties.mode.enum].sort(), ['enforce', 'local', 'report-only']); assert.deepEqual(schema.properties.reviewer.required.sort(), ['app_id', 'login', 'provider', 'user_id']); @@ -54,7 +61,9 @@ for (const [name, mutate] of [ ['extra reviewer field', policy => { policy.reviewer.app_slug = 'synthetic'; }], ['unknown top level field', policy => { policy.repository = 'example-org/example-repo'; }], ['non bot reviewer login', policy => { policy.reviewer.login = 'some-user'; }], - ['string owner id', policy => { policy.owner.user_id = '20649888'; }], + ['string owner id', policy => { policy.owner.user_id = '101'; }], + ['unknown owner field', policy => { policy.owner.team = 'synthetic'; }], + ['empty owner login', policy => { policy.owner.login = ''; }], ['unsupported provider', policy => { policy.reviewer.provider = 'coderabbit'; }], ['duplicate r2 rule', policy => { policy.r2_paths.push(policy.r2_paths[0]); }], ['absolute r0 rule', policy => { policy.r0_paths.push('/etc/passwd'); }], diff --git a/pr-governance/scripts/test-main.cjs b/pr-governance/scripts/test-main.cjs new file mode 100644 index 0000000..1a465f7 --- /dev/null +++ b/pr-governance/scripts/test-main.cjs @@ -0,0 +1,64 @@ +'use strict'; + +const test = require('node:test'); +const assert = require('node:assert/strict'); +const main = require('./main.cjs'); + +const POLICY = { + version: 'pr-governance/v3', + mode: 'enforce', + reviewer: { provider: 'codex', app_id: 1144995, user_id: 199175422, + login: 'chatgpt-codex-connector[bot]' }, + r0_paths: [], + r2_paths: [], + dependabot_template_exemption: true, + max_evidence_age_hours: 168, +}; + +const githubFor = policy => ({ + rest: { + repos: { + get: async () => ({ data: { default_branch: 'main' } }), + getBranch: async () => ({ data: { commit: { sha: 'a'.repeat(40) } } }), + getContent: async () => ({ data: { type: 'file', encoding: 'base64', + content: Buffer.from(JSON.stringify(policy)).toString('base64') } }), + }, + }, +}); + +const context = { repo: { owner: 'example-org', repo: 'example-repo' }, payload: {} }; + +async function load(policy, environment) { + const saved = { ...process.env }; + Object.assign(process.env, environment); + try { + return await main.loadPolicy(githubFor(policy), context, 'main'); + } finally { + delete process.env.GOVERNANCE_OWNER_ID; + delete process.env.GOVERNANCE_OWNER_LOGIN; + Object.assign(process.env, saved); + } +} + +test('owner configuration replaces a committed owner', async () => { + const { policy } = await load({ ...POLICY, owner: { user_id: 1, login: 'stale' } }, + { GOVERNANCE_OWNER_ID: '42', GOVERNANCE_OWNER_LOGIN: 'owner' }); + assert.deepEqual(policy.owner, { user_id: 42, login: 'owner' }); +}); + +test('a policy without owner is valid when the variables are set', async () => { + const { policy, policySha } = await load(POLICY, + { GOVERNANCE_OWNER_ID: '42', GOVERNANCE_OWNER_LOGIN: 'owner' }); + assert.deepEqual(policy.owner, { user_id: 42, login: 'owner' }); + assert.match(policySha, /^[0-9a-f]{64}$/); +}); + +test('a policy without owner and without variables fails closed', async () => { + await assert.rejects(load(POLICY, {})); +}); + +test('an incomplete owner configuration fails closed', async () => { + await assert.rejects(load(POLICY, { GOVERNANCE_OWNER_ID: '42' }), /owner configuration/); + await assert.rejects(load(POLICY, + { GOVERNANCE_OWNER_ID: '0', GOVERNANCE_OWNER_LOGIN: 'owner' }), /owner configuration/); +}); diff --git a/pr-governance/templates/risk-gate.yml b/pr-governance/templates/risk-gate.yml index 7874788..d12bbc9 100644 --- a/pr-governance/templates/risk-gate.yml +++ b/pr-governance/templates/risk-gate.yml @@ -52,6 +52,9 @@ jobs: steps: - name: Record label head uses: __GOVERNANCE_REPO__/pr-governance@__GOVERNANCE_SHA__ + env: + GOVERNANCE_OWNER_ID: ${{ vars.RISK_GATE_OWNER_ID }} + GOVERNANCE_OWNER_LOGIN: ${{ vars.RISK_GATE_OWNER }} with: mode: gate @@ -81,5 +84,8 @@ jobs: steps: - name: Evaluate uses: __GOVERNANCE_REPO__/pr-governance@__GOVERNANCE_SHA__ + env: + GOVERNANCE_OWNER_ID: ${{ vars.RISK_GATE_OWNER_ID }} + GOVERNANCE_OWNER_LOGIN: ${{ vars.RISK_GATE_OWNER }} with: mode: gate