From 9d4ac66547d4be0b39396a94f81b1b04adabb219 Mon Sep 17 00:00:00 2001 From: Pigbibi <20649888+Pigbibi@users.noreply.github.com> Date: Sat, 7 Nov 2026 21:19:15 +0800 Subject: [PATCH] fix(governance): require review at R0 and split label recording --- pr-governance/action.yml | 5 ++ pr-governance/scripts/gate.cjs | 10 ++-- pr-governance/scripts/main.cjs | 27 +++++++-- pr-governance/scripts/secret_scan.py | 2 +- pr-governance/scripts/test-codex.cjs | 6 +- pr-governance/scripts/test-gate.cjs | 16 ++--- pr-governance/scripts/test-main.cjs | 73 +++++++++++++++++++++++ pr-governance/scripts/test_secret_scan.py | 4 ++ pr-governance/templates/risk-gate.yml | 2 + 9 files changed, 122 insertions(+), 23 deletions(-) diff --git a/pr-governance/action.yml b/pr-governance/action.yml index 9a532c9..9232e55 100644 --- a/pr-governance/action.yml +++ b/pr-governance/action.yml @@ -12,6 +12,10 @@ inputs: description: "Artifact name for the aggregate-only history summary" required: false default: "secret-scan-summary" + record-label: + description: "Record the head a clearing label was added to instead of evaluating" + required: false + default: "false" outputs: {} runs: using: composite @@ -21,6 +25,7 @@ runs: uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # v8 env: GOVERNANCE_ACTION_PATH: ${{ github.action_path }} + GOVERNANCE_RECORD_LABEL: ${{ inputs.record-label }} with: script: | const path = require('node:path'); diff --git a/pr-governance/scripts/gate.cjs b/pr-governance/scripts/gate.cjs index 3468f63..04cc704 100644 --- a/pr-governance/scripts/gate.cjs +++ b/pr-governance/scripts/gate.cjs @@ -310,7 +310,6 @@ function markerPayloads(body, name) { const carriesMarker = body => MARKER_NAMES.some(name => markerPayloads(body, name).length > 0); const REVIEW_TEXT = { - review_not_required: 'Low-risk prose: reviewer evidence not required', review_complete: 'Reviewer evidence covers this head', review_pending: 'Waiting for reviewer evidence on this head', review_running: 'Reviewer review is still running on this head', @@ -471,10 +470,9 @@ function evaluate(snapshot, policy, policySha, now = Math.floor(Date.now() / 100 const template = templateVerdict(snapshot, policy, dependabot); const labels = labelEvidence(snapshot, policy); const threads = unresolvedReviewerThreads(snapshot, policy); - let review = risk.tier === 'R0' - ? { provider: policy.reviewer.provider, state: 'success', reason: 'review_not_required', comment_id: null, - format: null, commit_sha: null, coverage: null, unresolved: null } - : reviewerEvidence(snapshot, policy, now); + // Reviewer evidence is required at every tier: a prose-only change can still + // introduce a misleading claim, so R0 only lowers the label requirement. + let review = reviewerEvidence(snapshot, policy, now); // Thread state comes from GitHub, the marker from the provider. The stricter // reading wins, and a resolved-or-missing thread never proves a clean review. if (threads.length && review.state !== 'failure') review = { ...review, state: 'failure', reason: 'review_threads' }; @@ -496,7 +494,7 @@ function evaluate(snapshot, policy, policySha, now = Math.floor(Date.now() / 100 const cleared = labels.applied === 'owner' ? 'cleared by owner-approved' : labels.applied === 'agent' ? 'cleared by agent-approved, NOT confirmed by the owner' : null; const gateDescription = gateState === 'success' - ? `${risk.tier}: ${cleared || (review.reason === 'review_not_required' ? 'low-risk prose' : 'review verified')}` + ? `${risk.tier}: ${cleared || 'review verified'}` : gateState === 'pending' ? `${risk.tier}: waiting for reviewer` : `${risk.tier}: ${GATE_TEXT[reason] || REVIEW_TEXT[reason] || reason}`; const evidenceSha = digest({ diff --git a/pr-governance/scripts/main.cjs b/pr-governance/scripts/main.cjs index 564bd66..22c3e1b 100644 --- a/pr-governance/scripts/main.cjs +++ b/pr-governance/scripts/main.cjs @@ -53,14 +53,29 @@ async function loadPolicy(github, context, branch) { } async function runGate({ github, context, core }) { - // Label evidence must not ride on an evaluation a later event can supersede, - // so its own job records it and stops. The evaluate job depends on this one. - if (context.eventName === 'pull_request_target' && context.payload?.action === 'labeled') { + // Recording and evaluating are separate dispatch paths so the evaluate job + // actually runs: a label record is bound to its own delivery, while an + // evaluation may be superseded by any later event. + if (process.env.GOVERNANCE_RECORD_LABEL === 'true') { return recordLabel({ github, context, core }); } - const branch = await defaultBranch(github, context); - const { policy, policyCommit, policySha } = await loadPolicy(github, context, branch); - return run({ github, context, core, policy, policySha, policyCommit, defaultBranch: branch }); + const fallback = context.payload?.repository?.default_branch + || context.payload?.pull_request?.base?.ref || ''; + let branch = fallback; + let loaded = null; + try { + branch = await defaultBranch(github, context); + loaded = await loadPolicy(github, context, branch); + } catch (error) { + // A trusted policy that cannot be read must replace stale successes with + // blocking statuses instead of leaving the pull request looking green. + core?.warning?.(`Governance policy unavailable: ${error?.code || error?.name || 'Error'}`); + } + if (!loaded) { + return run({ github, context, core, policy: null, policySha: null, policyCommit: null, + defaultBranch: branch || fallback }); + } + return run({ github, context, core, ...loaded, defaultBranch: branch }); } module.exports = { runGate, loadPolicy, defaultBranch }; diff --git a/pr-governance/scripts/secret_scan.py b/pr-governance/scripts/secret_scan.py index 92bec15..67ff099 100644 --- a/pr-governance/scripts/secret_scan.py +++ b/pr-governance/scripts/secret_scan.py @@ -168,7 +168,7 @@ def range_scan(binary, base, head, root=None, config=None): verify_range(root, base, head) with tempfile.TemporaryDirectory(prefix='gitleaks-', dir=os.environ.get('RUNNER_TEMP')) as directory: findings = scan(binary, 'git', root, Path(directory) / 'range.json', - config=config, log_opts=f'{base}..{head}') + config=config, log_opts=f'--diff-merges=first-parent {base}..{head}') if findings: print(f'Range scan: {len(findings)} finding(s) in added commits') else: diff --git a/pr-governance/scripts/test-codex.cjs b/pr-governance/scripts/test-codex.cjs index 7c10fdb..f321115 100644 --- a/pr-governance/scripts/test-codex.cjs +++ b/pr-governance/scripts/test-codex.cjs @@ -181,12 +181,12 @@ test('a thread author id of another account does not block', () => { assert.equal(evaluate(f).allowed, true); }); -test('R0 prose reports no review requirement even with a stale marker', () => { +test('R0 prose with a stale marker stays pending', () => { const f = receiptMarker(fixture({ path: 'README.md' }), { commit_sha: 'f'.repeat(40) }); const result = evaluate(f); assert.equal(result.tier, 'R0'); - assert.equal(result.review.reason, 'review_not_required'); - assert.equal(result.allowed, true); + assert.equal(result.review.reason, 'review_stale'); + assert.equal(result.allowed, false); }); test('an unresolved thread still blocks R0 prose', () => { diff --git a/pr-governance/scripts/test-gate.cjs b/pr-governance/scripts/test-gate.cjs index 7b9f5f0..766d269 100644 --- a/pr-governance/scripts/test-gate.cjs +++ b/pr-governance/scripts/test-gate.cjs @@ -189,13 +189,15 @@ test('a clean receipt passes R1 and publishes three success statuses', () => { assert.match(result.evidence_sha256, /^[a-f0-9]{64}$/); }); -test('R0 prose needs no reviewer evidence but still reports a verdict', () => { - const result = evaluate(fixture({ path: 'README.md' })); - assert.equal(result.tier, 'R0'); - assert.equal(result.allowed, true); - assert.equal(result.reason, 'review_not_required'); - assert.equal(result.review.state, 'success'); - assert.deepEqual(result.statuses.map(entry => entry.context), ['pr-template', 'ai-review', 'risk-gate']); +test('R0 prose still needs the current-head reviewer receipt', () => { + const missing = evaluate(fixture({ path: 'README.md' })); + assert.equal(missing.tier, 'R0'); + assert.equal(missing.allowed, false); + assert.equal(missing.reason, 'ai_review_pending'); + const reviewed = evaluate(receiptMarker(fixture({ path: 'README.md' }))); + assert.equal(reviewed.allowed, true); + assert.equal(reviewed.review.state, 'success'); + assert.deepEqual(reviewed.statuses.map(entry => entry.context), ['pr-template', 'ai-review', 'risk-gate']); }); test('R1 without any reviewer marker stays pending', () => { diff --git a/pr-governance/scripts/test-main.cjs b/pr-governance/scripts/test-main.cjs index 1a465f7..ef16177 100644 --- a/pr-governance/scripts/test-main.cjs +++ b/pr-governance/scripts/test-main.cjs @@ -62,3 +62,76 @@ test('an incomplete owner configuration fails closed', async () => { await assert.rejects(load(POLICY, { GOVERNANCE_OWNER_ID: '0', GOVERNANCE_OWNER_LOGIN: 'owner' }), /owner configuration/); }); + +async function withEnvironment(environment, action) { + const saved = { ...process.env }; + delete process.env.GOVERNANCE_RECORD_LABEL; + Object.assign(process.env, environment); + try { + return await action(); + } finally { + delete process.env.GOVERNANCE_RECORD_LABEL; + Object.assign(process.env, saved); + } +} + +test('recording a label is a separate dispatch path', async () => { + const written = []; + const labeled = { + action: 'labeled', + label: { name: 'agent-approved' }, + pull_request: { number: 7, state: 'open', updated_at: '2026-01-01T00:00:00Z', + head: { sha: 'b'.repeat(40) }, base: { ref: 'main' } }, + }; + const github = { + paginate: async () => [{ id: 11, event: 'labeled', created_at: '2026-01-01T00:00:00Z', + label: { name: 'agent-approved' }, actor: { id: 42, login: 'agent' } }], + rest: { + issues: { listEvents: async () => ({ data: [] }) }, + repos: { + // Evaluation would need this; the record path must never get here. + get: async () => { throw new Error('recording must not read the repository'); }, + createCommitStatus: async params => { written.push(params); return { data: params }; }, + }, + }, + }; + const result = await withEnvironment({ GOVERNANCE_RECORD_LABEL: 'true' }, () => main.runGate({ + github, core: { info() {} }, + context: { eventName: 'pull_request_target', repo: { owner: 'example-org', repo: 'example-repo' }, + payload: labeled, serverUrl: 'https://github.example', runId: 9 }, + })); + assert.equal(result.event_id, 11); + assert.deepEqual(written.map(item => [item.context, item.state]), [['clearing-label', 'success']]); +}); + +test('an unreadable policy publishes blocking statuses instead of stale successes', async () => { + const written = []; + const failed = []; + const github = { + paginate: async (method, params) => (await method(params)).data, + rest: { + repos: { + get: async () => { throw new Error('network'); }, + listCommitStatusesForRef: async () => ({ data: [] }), + createCommitStatus: async params => { written.push(params); return { data: params }; }, + }, + pulls: { + get: async () => ({ data: { number: 7, state: 'open', head: { sha: 'b'.repeat(40) }, + base: { ref: 'main' } } }), + list: async () => ({ data: [] }), + }, + }, + }; + const result = await withEnvironment({ GOVERNANCE_RECORD_LABEL: 'false' }, () => main.runGate({ + github, core: { warning() {}, setFailed: message => failed.push(message) }, + context: { eventName: 'pull_request_target', repo: { owner: 'example-org', repo: 'example-repo' }, + payload: { action: 'labeled', label: { name: 'agent-approved' }, + pull_request: { number: 7, state: 'open', head: { sha: 'b'.repeat(40) }, base: { ref: 'main' } } }, + serverUrl: 'https://github.example', runId: 9 }, + })); + assert.deepEqual(result.failures, [{ number: 7, message: 'policy unavailable' }]); + assert.deepEqual(written.map(item => [item.context, item.state]), + [['pr-template', 'error'], ['ai-review', 'error'], ['risk-gate', 'error']]); + assert.equal(failed.length, 1); +}); + diff --git a/pr-governance/scripts/test_secret_scan.py b/pr-governance/scripts/test_secret_scan.py index c537cf5..f5e2667 100644 --- a/pr-governance/scripts/test_secret_scan.py +++ b/pr-governance/scripts/test_secret_scan.py @@ -327,6 +327,7 @@ def commit(message, text): git('commit', '--quiet', '-m', message) commit('test: base', 'setting=base\n') + base = git('rev-parse', 'HEAD') git('checkout', '--quiet', '-b', 'feature') commit('test: feature', 'setting=feature\n') git('checkout', '--quiet', 'main') @@ -337,6 +338,9 @@ def commit(message, text): # merge commit, not in either parent. commit('test: merge resolution', 'setting=main\npassword=' + fake_value() + '\n') with patch.object(SCANNER, 'ROOT', repository): + # The pull request range walks the same merge commit, so a credential + # that exists only in a conflict resolution must block the scan. + self.assertEqual(SCANNER.range_scan(BINARY, base, git('rev-parse', 'HEAD'), root=repository), 1) findings = SCANNER.scan(BINARY, 'git', repository, self.output) self.assertIn('configuration-secret-value', {row['RuleID'] for row in findings}) diff --git a/pr-governance/templates/risk-gate.yml b/pr-governance/templates/risk-gate.yml index d12bbc9..1c68191 100644 --- a/pr-governance/templates/risk-gate.yml +++ b/pr-governance/templates/risk-gate.yml @@ -57,6 +57,7 @@ jobs: GOVERNANCE_OWNER_LOGIN: ${{ vars.RISK_GATE_OWNER }} with: mode: gate + record-label: 'true' evaluate: name: Evaluate PR governance @@ -89,3 +90,4 @@ jobs: GOVERNANCE_OWNER_LOGIN: ${{ vars.RISK_GATE_OWNER }} with: mode: gate + record-label: 'false'