Skip to content

Commit 9ee0f7f

Browse files
authored
[policy] feat(maintainers): add risk-based PR evidence gate (#33)
* [policy] feat(maintainers): add risk-based PR evidence gate Change-Id: I6fca63510c1953053a0feddd77b4bdff29c64191 * fix(ci): accept spaced PR evidence sections Change-Id: I8a21b384a5d0a18f8069becce1464cefaa1250ae
1 parent 660a187 commit 9ee0f7f

7 files changed

Lines changed: 168 additions & 3 deletions

File tree

.github/pull_request_template.md

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
## What
1+
## Summary
22

33
<!-- One-sentence summary of the change -->
44

@@ -33,6 +33,12 @@
3333
- [ ] New provider follows the six-file structure, if applicable
3434
- [ ] No new runtime dependencies added without justification
3535

36+
## Behavior / risk
37+
38+
<!-- Required only when changing provider contracts, config/schema parsing, release/package files, or GitHub workflows.
39+
State the user-visible behavior or compatibility/security risk. “No user-visible behavior change” is valid for an internal refactor. -->
40+
3641
## Validation
3742

38-
<!-- How did you verify this change works? -->
43+
<!-- Commands, automated test cases, or manual reproduction used to verify this change.
44+
For high-risk changes, this must be non-empty. -->

.github/workflows/ci.yml

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,8 @@ jobs:
7878
scripts/lint-changed.sh
7979
scripts/verify.ts
8080
scripts/verify.test.ts
81+
scripts/pr-evidence.ts
82+
scripts/pr-evidence.test.ts
8183
scripts/release/
8284
scripts/setup-githooks.sh
8385
'
@@ -115,6 +117,23 @@ jobs:
115117
echo "Add '[policy]' to a commit message to acknowledge changing the verification harness itself."
116118
exit 1
117119
120+
maintainer-evidence:
121+
name: Maintainer evidence
122+
runs-on: ubuntu-latest
123+
steps:
124+
- uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6
125+
with:
126+
fetch-depth: 0
127+
- uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2
128+
with:
129+
bun-version: "1.3.5"
130+
- name: Require evidence only for high-risk changes
131+
env:
132+
BASE_SHA: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || github.event.before }}
133+
HEAD_SHA: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }}
134+
PR_BODY: ${{ github.event.pull_request.body }}
135+
run: bun scripts/pr-evidence.ts
136+
118137
package-compatibility:
119138
name: Package compatibility / Node ${{ matrix.node }}
120139
runs-on: ubuntu-latest
@@ -138,14 +157,15 @@ jobs:
138157
gate:
139158
name: Gate
140159
runs-on: ubuntu-latest
141-
needs: [prepare-verification, verify, policy-surface, package-compatibility]
160+
needs: [prepare-verification, verify, policy-surface, maintainer-evidence, package-compatibility]
142161
if: always()
143162
steps:
144163
- name: Check results
145164
run: |
146165
if [ "${{ needs.prepare-verification.result }}" != "success" ] || \
147166
[ "${{ needs.verify.result }}" != "success" ] || \
148167
[ "${{ needs.policy-surface.result }}" != "success" ] || \
168+
[ "${{ needs.maintainer-evidence.result }}" != "success" ] || \
149169
[ "${{ needs.package-compatibility.result }}" != "success" ]; then
150170
echo "One or more required checks failed."
151171
exit 1

CONTRIBUTING.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,10 @@ Every PR must satisfy:
3939
2. At least one maintainer approval
4040
3. No unresolved review comments
4141

42+
We keep the contribution path open: documentation, examples, and ordinary small changes do not need a design document or a special PR label. Changes to provider contracts, configuration/schema parsing, release/package files, or GitHub workflows have a small additional requirement: fill in the **Behavior / risk** and **Validation** sections of the PR description. This lets maintainers review the contract and its evidence before reading an implementation diff.
43+
44+
Maintainers should enable the repository settings that make the same policy effective at merge time: require the `Gate` and `Analyze TypeScript` checks, require one approving review, dismiss stale approvals, require approval of the latest push, and require resolved conversations. CODEOWNERS review and a merge queue are intentionally not required for routine contributions at the current project stage.
45+
4246
## Reporting bugs and requesting features
4347

4448
Open an issue using the templates under [.github/ISSUE_TEMPLATE](./.github/ISSUE_TEMPLATE). For anything security-related, do **not** open a public issue — follow [SECURITY.md](./SECURITY.md) instead.

docs/README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ For external contributors.
6464
| Doc | What's inside |
6565
|-----|---------------|
6666
| [Contributing overview](../CONTRIBUTING.md) | Dev setup, merge requirements, code style. |
67+
| [Maintainer review](./contributing/maintainer-review.md) | Evidence-based PR review and merge gates. |
6768
| [Development](./contributing/development.md) | Clone, install, run, test, package boundaries. |
6869
| [Provider development](./contributing/provider-development.md) | The six files a new provider must implement. |
6970
| [Release](./contributing/release.md) | npm publishing workflow and Trusted Publishing. |
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
# Maintainer review
2+
3+
OpenAgentPack welcomes exploratory and AI-assisted contributions. The merge bar is based on evidence, not on who wrote the code or how it was produced.
4+
5+
## Review in this order
6+
7+
1. Read **Summary**, then **Behavior / risk** and **Validation** when present.
8+
2. Confirm that CI covers the claimed behavior. A passing test is useful only if it would fail when the stated contract regresses.
9+
3. Read the diff around provider, configuration, credentials, release, and workflow boundaries. These paths have the greatest blast radius.
10+
4. Resolve every conversation, then approve only the latest commit.
11+
12+
## Gate levels
13+
14+
| Change | Automated gate | Human review |
15+
| --- | --- | --- |
16+
| Documentation, examples, small isolated fixes | `Gate` | Scope and user-facing accuracy |
17+
| CLI or SDK behavior | `Gate` | Contract plus a regression test or reproduction |
18+
| Provider contracts, config/schema, release/package files, workflows | `Gate` plus CodeQL | Behavior/risk and validation evidence; inspect compatibility and security impact |
19+
20+
The PR evidence check deliberately applies only to the third row. It does not require a design document, a linked Issue, or an AI-use declaration.
21+
22+
## GitHub settings
23+
24+
Configure `main` with these required checks: `Gate` and `Analyze TypeScript`. Require one approving review, dismiss stale approvals, require approval of the most recent push, require conversations to be resolved, and require branches to be up to date. Keep linear history enabled. Do not require CODEOWNERS review or a merge queue until review volume makes them worthwhile.
25+
26+
## Fast disposition
27+
28+
If a PR does not yet provide enough evidence, ask for one focused thing: a failing regression test, a runnable reproduction, or a clear statement of the intended behavior. If the author cannot provide it, keep the PR open as a Draft or close it with an invitation to reopen when the evidence is available.

scripts/pr-evidence.test.ts

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
import { describe, expect, test } from "bun:test";
2+
import { requiresMaintainerEvidence, validateMaintainerEvidence } from "./pr-evidence.ts";
3+
4+
describe("pull-request maintainer evidence", () => {
5+
test("does not add a merge barrier for ordinary contribution paths", () => {
6+
expect(requiresMaintainerEvidence(["README.md", "packages/cli/src/program.ts"])).toBe(false);
7+
expect(validateMaintainerEvidence("", ["README.md"])).toBeUndefined();
8+
});
9+
10+
test("asks for behavior and validation evidence on high-risk paths", () => {
11+
const files = ["packages/sdk/src/internal/providers/interface.ts"];
12+
expect(requiresMaintainerEvidence(files)).toBe(true);
13+
expect(validateMaintainerEvidence("## Summary\nA change", files)).toContain("## Behavior / risk");
14+
});
15+
16+
test("accepts concrete evidence for high-risk changes", () => {
17+
const body = `## Summary
18+
Add a provider capability.
19+
20+
## Behavior / risk
21+
22+
Existing provider configuration remains unchanged; the new capability is opt-in.
23+
24+
## Validation
25+
26+
\`bun test packages/sdk/tests/unit/provider-conformance.test.ts\``;
27+
expect(validateMaintainerEvidence(body, [".github/workflows/release.yml"])).toBeUndefined();
28+
});
29+
30+
test("does not treat HTML comments as evidence", () => {
31+
const body = `## Behavior / risk
32+
<!--
33+
34+
## Validation
35+
<!--`;
36+
expect(validateMaintainerEvidence(body, [".github/workflows/release.yml"])).toContain("## Behavior / risk");
37+
});
38+
});

scripts/pr-evidence.ts

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
import { existsSync } from "node:fs";
2+
3+
const highRiskPathPatterns = [
4+
/^\.github\/workflows\//,
5+
/^scripts\/release\//,
6+
/^(?:package\.json|bun\.lock)$/,
7+
/^packages\/sdk\/src\/internal\/(?:parser\/|types\/(?:config|session)\.ts|providers\/(?:interface|capabilities|registry|base-client)\.ts)/,
8+
];
9+
10+
export function requiresMaintainerEvidence(files: readonly string[]): boolean {
11+
return files.some((file) => highRiskPathPatterns.some((pattern) => pattern.test(file)));
12+
}
13+
14+
function sectionHasContent(body: string, heading: string): boolean {
15+
const escapedHeading = heading.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
16+
const match = body.match(new RegExp(`^## ${escapedHeading}[ \\t]*\\r?\\n([\\s\\S]*?)(?=^## |(?![\\s\\S]))`, "mi"));
17+
if (!match) return false;
18+
return (
19+
match[1]
20+
.replace(/<!--[\s\S]*?(?:-->|$)/g, "")
21+
.replace(/- \[ \] .*/g, "")
22+
.trim().length > 0
23+
);
24+
}
25+
26+
export function validateMaintainerEvidence(body: string, files: readonly string[]): string | undefined {
27+
if (!requiresMaintainerEvidence(files)) return undefined;
28+
const missing = ["Behavior / risk", "Validation"].filter((heading) => !sectionHasContent(body, heading));
29+
if (missing.length === 0) return undefined;
30+
return `High-risk changes require a non-empty ${missing.map((heading) => `## ${heading}`).join(" and ")} section.`;
31+
}
32+
33+
function changedFiles(base: string, head: string): string[] {
34+
const result = Bun.spawnSync(["git", "diff", "--name-only", "--diff-filter=ACMR", base, head], {
35+
stdout: "pipe",
36+
stderr: "pipe",
37+
});
38+
if (result.exitCode !== 0) throw new Error(result.stderr.toString().trim() || "Could not read changed files.");
39+
return result.stdout.toString().split("\n").filter(Boolean);
40+
}
41+
42+
function main(): number {
43+
if (process.env.GITHUB_EVENT_NAME !== "pull_request") {
44+
console.log("Not a pull request; maintainer-evidence check skipped.");
45+
return 0;
46+
}
47+
const base = process.env.BASE_SHA;
48+
const head = process.env.HEAD_SHA;
49+
if (!base || !head || !existsSync(".git"))
50+
throw new Error("BASE_SHA and HEAD_SHA are required for pull-request evidence checks.");
51+
const files = changedFiles(base, head);
52+
const error = validateMaintainerEvidence(process.env.PR_BODY ?? "", files);
53+
if (!error) {
54+
console.log(
55+
requiresMaintainerEvidence(files)
56+
? "Maintainer evidence supplied."
57+
: "Low-risk change; maintainer evidence is optional.",
58+
);
59+
return 0;
60+
}
61+
console.error(`::error::${error}`);
62+
console.error(
63+
"Describe the user-visible behavior or compatibility risk, then give the test command, test case, or manual reproduction used to validate it.",
64+
);
65+
return 1;
66+
}
67+
68+
if (import.meta.main) process.exit(main());

0 commit comments

Comments
 (0)