From 1d911d0e0b7894da0ccd8be2e74b6cdf2df7a618 Mon Sep 17 00:00:00 2001 From: Codex Date: Thu, 5 Nov 2026 23:53:19 +0800 Subject: [PATCH] ci: Remove owner label requirements from source merge gate --- .github/workflows/risk-gate.yml | 23 +++-------- AGENTS.md | 9 +++-- PROPOSAL.md | 70 ++++++++++++++++----------------- README.md | 2 +- README.zh-CN.md | 2 +- docs/STYLE_GUIDE.md | 16 +++++--- scripts/test-risk-gate.cjs | 52 ++++++++++++++++-------- 7 files changed, 93 insertions(+), 81 deletions(-) diff --git a/.github/workflows/risk-gate.yml b/.github/workflows/risk-gate.yml index c5436be..1afba11 100644 --- a/.github/workflows/risk-gate.yml +++ b/.github/workflows/risk-gate.yml @@ -89,9 +89,9 @@ jobs: const hash = value => typeof value === 'string' && /^[0-9a-f]{40}$/.test(value); const target = `${context.serverUrl}/${repo.owner}/${repo.repo}/actions/runs/${context.runId}`; const number = Number(process.env.PR_NUMBER); - // Explicit owner-only globs: match normalized lowercase snake_case paths. + // Sensitive-path globs: match normalized lowercase snake_case paths. // * never crosses /; ** does. No docs/tests exemption applies to this list. - const ownerOnlyGlobs = [ + const sensitiveGlobs = [ '**/*secret*', '**/*secret*/**', '**/*credential*', '**/*credential*/**', '**/.env*', '**/*.pem', '**/*.key', '**/*.p12', '**/*.pfx', '**/*iam*', '**/*iam*/**', '**/*service_account*', '**/*service_account*/**', @@ -120,7 +120,7 @@ jobs: } return new RegExp(pattern + '$').test(normalize(path)); } - const ownerOnly = path => ownerOnlyGlobs.some(glob => matchesGlob(path, glob)); + const sensitivePath = path => sensitiveGlobs.some(glob => matchesGlob(path, glob)); function highRisk(path) { const p = path.replace(/([a-z0-9])([A-Z])/g, '$1_$2').toLowerCase(); if (p.startsWith('.github/workflows/') || /(^|\/)codeowners$/.test(p)) return true; @@ -241,23 +241,11 @@ jobs: }; const files = await readFiles(); const paths = files.flatMap(file => [file.filename, file.previous_filename].filter(Boolean)); - const protectedPath = paths.some(ownerOnly); + const protectedPath = paths.some(sensitivePath); const risky = protectedPath || paths.some(highRisk); let approved = false; let before, after; - if (protectedPath) { - const readEvents = async () => github.paginate(github.rest.issues.listEvents, - {...repo, issue_number: number, per_page: 100}); - before = await readEvents(); - if (pr.labels.some(label => label.name === 'owner-approved')) { - const events = before; - const latest = events.filter(event => ['labeled', 'unlabeled'].includes(event.event) - && event.label?.name === 'owner-approved').sort((a, b) => b.id - a.id)[0]; - // A shared GitHub identity cannot distinguish the owner from their AI agents. - approved = latest?.event === 'labeled' && latest.actor?.login === 'Pigbibi'; - } - after = await readEvents(); - } else if (risky) { + if (risky) { const {data: content} = await github.rest.repos.getContent({...repo, path: '.github/governance/policy.json', ref: pr.base.sha}); if (content.encoding !== 'base64' || typeof content.content !== 'string') fail(); @@ -281,7 +269,6 @@ jobs: || JSON.stringify(before) !== JSON.stringify(after)) fail(); const allowed = !risky || approved; await status('risk-gate', allowed ? 'success' : 'failure', !risky ? 'Low-risk paths: no owner approval needed' - : protectedPath ? (approved ? 'Owner-only paths: existing owner label verified' : 'Owner-only paths: owner approval required') : approved ? 'Exact-head clean review and required checks verified' : 'Automatic approval blocked: review or required checks incomplete'); if (!allowed || !templateOK) core.setFailed('Required PR governance checks did not pass'); } catch { diff --git a/AGENTS.md b/AGENTS.md index d06bab8..7c72f62 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -89,10 +89,11 @@ Organization documentation rules are checked by `python scripts/check_style.py`, - Commit and PR titles use professional English, imperative Conventional Commits (`feat`, `fix`, `docs`, `refactor`, `test`, `chore`, `perf`, `ci`) and at most 72 characters. Sentence case is allowed; commitlint ignores merge commits. - PR descriptions explain the problem, changes, verification and risk. AI-authored PRs identify the tool and the model when available. Labels and approval or risk summaries use professional English. -- Use `risk:low` for documentation, tests and ordinary checks; use `risk:high` and `needs-owner-approval` for protected paths. Classification labels describe risk; the path-based `risk-gate` determines whether approval is required. -- AI-only development uses required automated checks, template validation and independent AI review when available, with zero required human or code-owner approvals. Ordinary high-risk paths need no manual label only when the trusted Codex App's latest `codex-review-state:v1` marker is completed for the exact current head with `has_unresolved_findings=false`, and every required check is green. Missing, stale, malformed, unreadable or changing evidence blocks automatic approval. -- The owner-only globs in [PROPOSAL.md](PROPOSAL.md#owner-only-paths) retain the existing owner-applied `owner-approved` requirement. They cover secrets, IAM, deployments, trading/execution and broker credentials, plus Kelly, stop-loss and this gate's trust configuration. Renames check both paths; docs and tests cannot exempt these globs. Agents must never add or remove approval labels. -- Automated path approval satisfies only `risk-gate`; it grants no merge, deployment, trading or configuration authority. The reusable governance shadow observer remains report-only. This proposal affects this repository only until separately adopted elsewhere. +- Use `risk:low` for documentation, tests and ordinary checks; use `risk:high` for sensitive paths. Classification labels describe risk and never grant authority. +- Exact-head-clean merges need no owner approval label, including high-risk paths, the gate itself and trust configuration. The trusted Codex App's latest `codex-review-state:v1` marker must be completed for the exact current head with `has_unresolved_findings=false`, and every required check must be green. Missing, stale, malformed, unreadable or changing evidence blocks automatic approval. Low-risk path checks retain their existing behavior; merge still requires the same review and checks. +- Agents must never add or remove approval labels, force-push or print secrets. The gate reads its trust policy from the immutable base revision and never executes candidate code. +- Source merge is not deployment authority. Live-trading deployments, Secrets/IAM changes and deletions require explicit owner authorization in chat, recorded in `_ops/approvals.md` by the coordinator. Live trading, Kelly sizing and every stop-loss switch stay OFF unless the owner explicitly authorizes them. +- Automated path approval satisfies only `risk-gate`; the reusable governance shadow observer remains report-only. This policy affects this repository only until separately adopted elsewhere. - Preserve required checks and branch protection; merge only through normal checks. Use `/` PR branches; never push directly to the default branch or force-push a reviewed branch. Commit, push, merge and production actions require authorization; existing authorization remains valid within its scope. ## Scope and authorization diff --git a/PROPOSAL.md b/PROPOSAL.md index 19ba252..707849c 100644 --- a/PROPOSAL.md +++ b/PROPOSAL.md @@ -1,21 +1,20 @@ -# Automated approval for ordinary high-risk paths +# Automated approval without owner labels -Status: proposal for owner decision. Opening this Draft PR does not adopt it. +Status: owner-authorized policy update; effective when merged. ## Problem and rationale -The current path gate requires an owner-applied label for every high-risk -change, including ordinary strategy, risk and runtime code. It ignores an -already completed, clean Codex review and passing required checks. The proposed -policy removes that repeated manual step while keeping owner control over -sensitive operations and the gate's trust configuration. +The previous gate retained an owner-applied label for sensitive paths even with +a clean exact-head review and passing required checks. All high-risk paths now +use the same automated evidence rule, including the gate and trust configuration. +Source review and merge do not grant operational authority. ## Decision rule Low-risk paths retain their existing behavior. For other paths, inspect both current and previous filenames before any docs or tests exemption. -An ordinary high-risk change passes `risk-gate` only when all of these hold: +A high-risk change passes `risk-gate` only when all of these hold: 1. The newest marker from a trusted Codex App is exactly one well-formed `codex-review-state:v1` comment marker. The App IDs come from the existing @@ -29,9 +28,8 @@ An ordinary high-risk change passes `risk-gate` only when all of these hold: missing results, skipped/neutral checks, wrong App bindings and API errors block automatic approval. Legacy statuses cannot prove an App-bound identity and therefore cannot satisfy such a requirement. -4. No changed or previous path matches the owner-only list below. An old owner - label cannot substitute for missing automated evidence on ordinary high-risk - paths. +4. No owner label is required for any path class. An existing label cannot + substitute for missing review or check evidence. `risk-gate` is the output of this decision, so it cannot be its own prerequisite. The locally validated `pr-template` result is published before the required-check @@ -39,12 +37,12 @@ read. These are the only two self-produced contexts; their expected App binding is checked. Every other required job stays mandatory. Native branch protection and the required-check list stay unchanged. -## Owner-only paths +## Sensitive paths These are the exact implemented globs. Paths are converted from camelCase to snake_case and lowercased before matching. `*` stays within one path segment; `**/` matches zero or more directory segments. Broad credential, trading and -infrastructure families intentionally keep ambiguous files under owner control. +infrastructure families intentionally require automated review for ambiguous files. There is no docs, tests, examples or fixtures exemption for this list. ```text @@ -100,16 +98,20 @@ There is no docs, tests, examples or fixtures exemption for this list. **/codeowners ``` -The list includes the gate itself, its trust policy and CODEOWNERS because a PR -must not grant itself authority by modifying those files. Broker, order and -execution paths stay owner-only even when a filename does not say live. Kelly -and stop-loss paths stay protected; this proposal enables no trading feature. -Other workflow changes still require the automated high-risk rule. +The list includes the gate itself, its trust policy and CODEOWNERS. These paths +require the same exact-head clean review and green required checks, never an +owner label. Trust App IDs are read from the immutable base revision, so a PR +cannot grant itself trust by changing its candidate policy. Renames check both +paths, and docs/tests cannot exempt sensitive paths. -Owner-only paths retain the existing verified owner-label event predicate. -This proposal neither strengthens that legacy human-identity proof nor allows -an agent to add/remove a label. A label is not operational authorization to -execute a deployment or trade. +## Operational authorization + +Exact-head-clean merges need no owner label. Agents must never add or remove +approval labels, force-push or print secrets. Source merge is not deployment +authority. Live-trading deployments, Secrets/IAM changes and deletions require +explicit owner authorization in chat, recorded in `_ops/approvals.md` by the +coordinator. Live trading, Kelly sizing and every stop-loss switch stay OFF +unless the owner explicitly authorizes them. ## Execution and evidence freshness @@ -128,7 +130,7 @@ A read-only discovery job resolves every event to PR numbers. All event types then share one concurrency group per PR without cancellation. Replaced pending events evaluate the same PR; unrelated PRs never replace each other in the queue. Each evaluation reconstructs evidence after restart; no persisted approval is -reused. It rereads PR head/base/body/labels, changed paths, owner events or +reused. It rereads PR head/base/body/labels, changed paths and review/check/protection evidence before publishing. A mismatch or read failure blocks the decision. API exception text and user bodies are never logged. @@ -147,10 +149,10 @@ unit. The root filename `PROPOSAL.md` is explicitly requested for this decision; the naming checker permits only this root path, while nested uppercase proposal filenames remain invalid. No security or identifier rule is relaxed. -This PR stays Draft for the owner's decision and the established review bot. -The current base-branch risk gate still governs this PR. Opening it does not -satisfy its own approval gate and does not authorize merge, undraft, labels, -repository/organization settings, deployment or live activation. +The current base-branch gate governs this PR until merge. The owner's bounded +bootstrap authorization may satisfy that legacy gate for this exact PR/head; +it does not become an ongoing label requirement or authorize other operations. +Normal required checks and exact-head review remain mandatory. ## Validation @@ -164,11 +166,9 @@ checker before publication. The PR description records the actual results. ## Rollback -Through a separately authorized reviewed change, restore the previous -`risk-gate.yml` and the corresponding AGENTS approval wording, remove the added -risk tests from the CI command and revert this proposal's document/name-only -changes. Keep every existing native required check, the owner-only controls and -branch protection enabled. Reevaluate affected open PRs under the restored gate -using its existing workflow-dispatch input; do not reuse successful statuses -from the proposed policy as proof of legacy owner approval. No deployment, -settings edit, history rewrite or label change is part of this Draft PR. +Through a separately authorized reviewed change, restore the previous gate and +its corresponding rules, documentation and tests together. Keep every native +required check and branch protection enabled. Reevaluate affected open PRs under +the restored gate using its existing workflow-dispatch input; do not reuse old +successful statuses as proof of a current decision. No deployment, settings edit, +history rewrite or label change is part of this policy update. diff --git a/README.md b/README.md index 33263c9..8e615e0 100644 --- a/README.md +++ b/README.md @@ -16,7 +16,7 @@ Organization-wide files for QuantStrategyLab repositories. | [Secret scanning](docs/secret-scanning.md) | Publication guard, PR check and weekly all-branch scan templates. | | `profile/README.md` | Text shown on the organization's GitHub profile page. | | `PULL_REQUEST_TEMPLATE.md` | Default pull request template for repositories without their own. | -| [Automated risk approval proposal](PROPOSAL.md) | Approval conditions, owner-only paths and rollback. | +| [Automated risk approval proposal](PROPOSAL.md) | Exact-head review, label-free merges, operational authorization and rollback. | | `AGENTS.md` | AI contribution and maintenance rules for this repository. | ## Contributing diff --git a/README.zh-CN.md b/README.zh-CN.md index 6bbfc3a..bbf22b3 100644 --- a/README.zh-CN.md +++ b/README.zh-CN.md @@ -16,7 +16,7 @@ QuantStrategyLab 各仓库共用的组织级文件。 | [密钥扫描](docs/secret-scanning.md) | 发布前扫描、PR 检查和每周全分支扫描模板。 | | `profile/README.md` | 组织 GitHub 主页上显示的介绍。 | | `PULL_REQUEST_TEMPLATE.md` | 没有自带模板的仓库默认使用的 pull request 模板。 | -| [风险门禁自动批准提案](PROPOSAL.md) | 自动批准条件、保留 owner 审批的路径和回退方式。 | +| [风险门禁自动批准提案](PROPOSAL.md) | 精确 head 审查、无需标签的合并、操作授权和回退方式。 | | `AGENTS.md` | 本仓库的 AI 参与与维护规则。 | ## 参与贡献 diff --git a/docs/STYLE_GUIDE.md b/docs/STYLE_GUIDE.md index f15afee..43513ab 100644 --- a/docs/STYLE_GUIDE.md +++ b/docs/STYLE_GUIDE.md @@ -282,14 +282,20 @@ Sentence-case subjects are allowed; commitlint ignores merge commits. AI-authored PRs identify the tool and the model when available. Labels and risk summaries use professional English. Use `risk:low` for documentation, tests and -ordinary checks; use `risk:high` and `needs-owner-approval` for protected paths. -The path-based `risk-gate` determines whether owner approval is required; -classification labels cannot override it. +ordinary checks; use `risk:high` for sensitive paths. Classification labels +describe risk and do not grant authority. In this repository, `risk-gate` uses +exact-head review and required checks for all high-risk paths without an owner +label. Other repositories adopt that policy only through separate reviewed changes. Development uses required automated checks, template validation and independent AI review when available, with zero required human or code-owner approvals. -High-risk paths wait for the owner's `owner-approved` label. AI agents must never -add or remove that label. Existing required checks and protections still apply. +Exact-head-clean merges in this repository need no owner approval label, including +gate and trust configuration changes. Agents must never add or remove approval +labels, force-push or print secrets. Existing required checks and protections +still apply. Source merge does not authorize operations: live-trading deployments, +Secrets/IAM changes and deletions require explicit owner authorization in chat, +recorded by the coordinator in `_ops/approvals.md`. Live trading, Kelly sizing and +every stop-loss switch stay OFF unless explicitly authorized by the owner. ## 10. Code comments and docstrings diff --git a/scripts/test-risk-gate.cjs b/scripts/test-risk-gate.cjs index 0d6c09c..94cc696 100644 --- a/scripts/test-risk-gate.cjs +++ b/scripts/test-risk-gate.cjs @@ -4,10 +4,7 @@ const fs = require('node:fs'); const path = require('node:path'); const workflow = fs.readFileSync(process.env.RISK_WORKFLOW || path.join(__dirname, '../.github/workflows/risk-gate.yml'), 'utf8'); const script = workflow.split(' script: |\n').at(-1).split('\n').map(x => x.slice(12)).join('\n'); -// Substitute only the legacy identity literal so fixtures contain no real owner. -const testScript = script.replace(/(latest\.actor\?\.login === ')[^']+(')/, '$1-owner$2'); -assert.notEqual(testScript, script, 'Legacy owner predicate must remain present'); -const execute = new (Object.getPrototypeOf(async function(){}).constructor)('github', 'context', 'core', 'process', testScript); +const execute = new (Object.getPrototypeOf(async function(){}).constructor)('github', 'context', 'core', 'process', script); const discoveryScript = workflow.split(' script: |\n')[1].split('\n\n evaluate:')[0].split('\n').map(x => x.slice(12)).join('\n'); const discover = new (Object.getPrototypeOf(async function(){}).constructor)('github', 'context', 'core', 'process', discoveryScript); const H = 'a'.repeat(40), B = 'b'.repeat(40); @@ -39,7 +36,7 @@ async function run(f = fixture()) { issues: {listComments: 'comments', listEvents: 'events'}, checks: {listForRef: 'checks'}, repos: {listCommitStatusesForRef: 'statuses', getBranchRules: 'rules', listPullRequestsAssociatedWithCommit: 'associated', - getContent: async () => ({data: {encoding: 'base64', content: Buffer.from(JSON.stringify(read('policy'))).toString('base64')}}), + getContent: async params => {f.policyRefs?.push(params.ref); return {data: {encoding: 'base64', content: Buffer.from(JSON.stringify(read('policy'))).toString('base64')}};}, createCommitStatus: async params => {if (f.apiError === 'publish') throw new Error('private diagnostic '); out.statuses.push(params);}}}, paginate: async (method, params) => { if (method === 'statuses') out.statusRefs.push(params.ref); @@ -86,13 +83,17 @@ for (const filename of ['docs/secrets.md', 'tests/deploy/test.js', 'src/iam/poli 'src/broker_credentials.json', '.env.local', 'infra/main.tf', '.github/workflows/deploy.yml', 'src/liveTrading.js', 'src/stop_loss.js', 'src/kelly.py', 'src/orders/run.js', '.github/governance/policy.json', '.github/workflows/risk-gate.yml']) { - test(`Owner-only path cannot use automatic approval: ${filename}`, async () => { + test(`Sensitive path passes without an owner label: ${filename}`, async () => { const f = fixture(); f.files[0].filename = filename; + assert.equal((await run(f)).risk, 'success'); + f.comments = []; assert.notEqual((await run(f)).risk, 'success'); }); } -test('Renaming an owner-only file to ordinary code keeps owner approval', async () => { +test('Renaming a sensitive file still requires exact-head review', async () => { const f = fixture(); f.files[0].status = 'renamed'; f.files[0].previous_filename = 'src/secrets.js'; + assert.equal((await run(f)).risk, 'success'); + f.comments = []; assert.notEqual((await run(f)).risk, 'success'); }); for (const mutation of ['failure', 'skipped', 'neutral', 'pending', 'missing', 'wrong-app', 'stale', 'newer-failure', 'status-pending']) { @@ -188,7 +189,7 @@ test('Status publication errors never expose API diagnostics', async () => { assert.notEqual(out.risk, 'success'); assert.ok(!JSON.stringify(out).includes('private diagnostic')); assert.ok(out.failures.length); }); -test('Owner globs escape literal dots', async () => { +test('Sensitive path globs escape literal dots', async () => { const f = fixture(); f.files[0].filename = 'src/notXkey'; assert.equal((await run(f)).risk, 'success'); }); @@ -221,19 +222,36 @@ test('A ruleset without classic protection can authorize when every check is gre required_status_checks: [{context: 'Unit tests', integration_id: 23456}]}}]; assert.equal((await run(f)).risk, 'success'); }); -test('Owner-only paths retain the existing owner-label event predicate', async () => { - const f = fixture(); f.files[0].filename = 'src/secrets.js'; f.pr.labels = [{name: 'owner-approved'}]; - f.events = [{id: 1, event: 'labeled', label: {name: 'owner-approved'}, actor: {login: '-owner'}}]; +for (const filename of ['.github/workflows/risk-gate.yml', '.github/governance/policy.json', 'src/secrets.js']) { + test(`Labels cannot bypass exact-head evidence on sensitive paths: ${filename}`, async () => { + const f = fixture(); f.files[0].filename = filename; + f.pr.labels = [{name: 'owner-approved'}]; + assert.equal((await run(f)).risk, 'success'); + f.comments = []; + assert.notEqual((await run(f)).risk, 'success'); + f.comments = fixture().comments; f.checks[0].conclusion = 'failure'; + assert.notEqual((await run(f)).risk, 'success'); + }); +} +test('Gate trust comes from the immutable base revision', async () => { + const f = fixture(); f.files[0].filename = '.github/governance/policy.json'; + f.policyRefs = []; assert.equal((await run(f)).risk, 'success'); - f.events.push({id: 2, event: 'unlabeled', label: {name: 'owner-approved'}, actor: {login: '-owner'}}); - assert.notEqual((await run(f)).risk, 'success'); + assert.deepEqual(f.policyRefs, [B]); }); -test('Owner-label event revocation during evaluation blocks the old approval', async () => { - const f = fixture(); f.files[0].filename = 'src/secrets.js'; f.pr.labels = [{name: 'owner-approved'}]; - f.events = [{id: 1, event: 'labeled', label: {name: 'owner-approved'}, actor: {login: '-owner'}}]; - f.changed = {events: [...f.events, {id: 2, event: 'unlabeled', label: {name: 'owner-approved'}}]}; +test('Sensitive paths fail closed across concurrent review changes and restart', async () => { + const f = fixture(); f.files[0].filename = '.github/workflows/risk-gate.yml'; + f.changed = {comments: []}; + assert.notEqual((await run(f)).risk, 'success'); + f.comments = []; delete f.changed; assert.notEqual((await run(f)).risk, 'success'); }); +test('Sensitive path API errors remain redacted', async () => { + const f = fixture(); f.files[0].filename = 'src/secrets.js'; f.apiError = 'comments'; + const out = await run(f); + assert.notEqual(out.risk, 'success'); + assert.ok(!JSON.stringify(out).includes('private diagnostic')); +}); test('Ordinary high-risk paths cannot use a legacy owner label to bypass review', async () => { const f = fixture(); f.pr.labels = [{name: 'owner-approved'}]; f.comments = []; f.events = [{id: 1, event: 'labeled', label: {name: 'owner-approved'}, actor: {login: '-owner'}}];