ci: fail early on platform budgets and validate forks without writes
This commit is contained in:
1 parent
02e13c1f97
commit
51d1bdf95f
8 files changed
+261
-17
No files matched your search
@@ -60,7 +60,11 @@ jobs:
|
||||
scripts.tests.test_ci_plan \
|
||||
scripts.tests.test_ci_native_tests \
|
||||
scripts.tests.test_stable_release \
|
||||
scripts.tests.test_ci_cache -v
|
||||
scripts.tests.test_ci_cache \
|
||||
scripts.tests.test_platform_cfg \
|
||||
scripts.tests.test_pr_size_workflow -v
|
||||
- name: Check platform cfg budget before native jobs
|
||||
run: python3 scripts/check_platform_cfg.py
|
||||
- name: Plan native validation
|
||||
id: matrix
|
||||
shell: bash
|
||||
|
||||
@@ -49,6 +49,20 @@ jobs:
|
||||
const size = counted <= 100 ? "S" : counted <= 500 ? "M" : counted <= LIMIT ? "L" : "XL";
|
||||
const labels = new Set(pr.labels.map((l) => l.name));
|
||||
const wanted = `size/${size}`;
|
||||
const oversized = size === "XL" && !labels.has("size-exempt");
|
||||
core.info(`counted ${counted} lines (${ignored} ignored) in ${files.length} files -> ${wanted}`);
|
||||
await core.summary
|
||||
.addRaw(`PR size: **${size}**, ${counted} counted lines (${ignored} ignored), limit ${LIMIT}.`)
|
||||
.write();
|
||||
if (oversized) {
|
||||
core.setFailed(`${counted} changed source lines exceed the ${LIMIT}-line limit; split the pull request or add size-exempt.`);
|
||||
}
|
||||
// Fork tokens are read-only. The size decision above is still enforced;
|
||||
// optional labels and comments must not turn a valid PR into a 403 failure.
|
||||
if (pr.head.repo?.full_name !== `${context.repo.owner}/${context.repo.repo}`) {
|
||||
core.info("Fork PR: size is reported in the summary; no label/comment writes.");
|
||||
return;
|
||||
}
|
||||
for (const l of labels) {
|
||||
if (l.startsWith("size/") && l !== wanted) {
|
||||
await github.rest.issues.removeLabel({ ...context.repo, issue_number: pr.number, name: l }).catch(() => {});
|
||||
@@ -58,8 +72,7 @@ jobs:
|
||||
await github.rest.issues.addLabels({ ...context.repo, issue_number: pr.number, labels: [wanted] });
|
||||
}
|
||||
|
||||
core.info(`counted ${counted} lines (${ignored} ignored) in ${files.length} files -> ${wanted}`);
|
||||
if (size !== "XL" || labels.has("size-exempt")) return;
|
||||
if (!oversized) return;
|
||||
|
||||
const marker = "<!-- pr-size-guard -->";
|
||||
const comments = await github.paginate(github.rest.issues.listComments, {
|
||||
@@ -78,4 +91,3 @@ jobs:
|
||||
].join("\n"),
|
||||
});
|
||||
}
|
||||
core.setFailed(`${counted} changed source lines exceed the ${LIMIT}-line limit; split the pull request or add size-exempt.`);
|
||||
@@ -17,7 +17,7 @@ The repository already used an ignored `scripts/check_prohibited_names.py` in st
|
||||
## Decision
|
||||
|
||||
- Publish the existing checker as the single policy implementation and add a fail-closed range mode.
|
||||
- Resolve both revisions, compute their `merge-base`, scan ordinary commits against their first parent and merge commits with a combined diff, and scan every commit message in the resulting range. Missing, zero, unreadable, or unrelated revisions return failure.
|
||||
- Resolve both revisions, compute their `merge-base`, scan ordinary commits against their first parent and merge commits with a combined diff, and scan every commit message in the resulting range. Missing, zero, unreadable, or unrelated revisions return failure. Invalid UTF-8 in paths, patches or messages fails explicitly rather than substituting another path or text.
|
||||
- Keep only the checker, its contract tests and the intentional negative comparison fixture as exact path exceptions. Strip only reviewed inline attribution, Cargo git/source references, and protocol compatibility identifiers before applying the existing patterns.
|
||||
- Run the checker as a step inside `architecture-contracts` and run its unit tests with the other guardrail tests. No new required check or server-side ruleset change is introduced.
|
||||
|
||||
@@ -34,7 +34,7 @@ New PR/merge-group/main-push text and commit subjects are checked by the existin
|
||||
|
||||
## Validation
|
||||
|
||||
`scripts/tests/test_prohibited_names.py` covers ordinary comparison failure, inline attribution positive/negative paths, protocol identifiers, Cargo dependency references, GitHub CLI text, range text, range commit messages, diverged related bases, unrelated/missing bases, old-history exclusion, added-then-deleted text, and merge-resolution additions. The checker also passes `py_compile` before CI execution.
|
||||
`scripts/tests/test_prohibited_names.py` covers ordinary comparison failure, inline attribution positive/negative paths, protocol identifiers, Cargo dependency references, GitHub CLI text, range text, range commit messages, diverged related bases, unrelated/missing bases, old-history exclusion, added-then-deleted text, and merge-resolution additions. The checker also covers malformed UTF-8 paths/text and normal Unicode/control-character paths; the raw-byte path fixture runs on POSIX, where that filename can exist.
|
||||
|
||||
## Supersedes
|
||||
|
||||
|
||||
@@ -0,0 +1,58 @@
|
||||
# Keep the size decision independent of optional PR writes
|
||||
|
||||
## Status
|
||||
|
||||
Implemented for review.
|
||||
|
||||
## Context
|
||||
|
||||
Fork pull requests have a read-only token. The required size check counted files,
|
||||
then tried to add a size label before reporting its decision. An ordinary small
|
||||
contribution therefore failed with HTTP 403 instead of reporting its actual size.
|
||||
|
||||
## Evidence
|
||||
|
||||
PR 274 at `98fad00b01da9dd089581e4d4f66a251bfc1404d` changes 127 Rust source lines.
|
||||
Run 35893377849 had PullRequests-read permission and failed at POST issue labels
|
||||
while trying to add the ordinary `size/M` classification. Its native tests passed.
|
||||
|
||||
## Decision
|
||||
|
||||
Compute and publish the size result before optional writes. The existing 1500-line
|
||||
limit, ignored documentation/assets and maintainer exemption semantics remain the
|
||||
same. An oversized fork fails immediately. Forks report their result in the job
|
||||
summary and do not attempt labels or comments with a read-only token. Same-repository
|
||||
pull requests retain their existing labels and oversized-change guidance.
|
||||
|
||||
Tests execute the actual inline workflow script with fake APIs, so there is one
|
||||
implementation of the counting and fork boundary. Node is already available on
|
||||
the supported hosted images; the tests require it and use no npm dependencies.
|
||||
|
||||
## Rejected alternatives
|
||||
|
||||
Broadening fork token access or executing fork code in a privileged PR event is
|
||||
unnecessary. Suppressing the entire size check on forks would waive its invariant.
|
||||
A separate privileged labeling service adds another trust boundary and is not
|
||||
needed to validate source size.
|
||||
|
||||
## Consequences
|
||||
|
||||
Fork contributors receive a genuine size-check result without write access.
|
||||
Automatic size labels remain available for same-repository contributions;
|
||||
maintainers may apply the ordinary size label to a fork. Such a label is not a
|
||||
budget exemption, and the source count is still recomputed by the required check.
|
||||
|
||||
## Validation
|
||||
|
||||
The workflow-script tests cover a small fork, oversized fork, exact limit,
|
||||
existing maintainer exemption, deleted-fork metadata and same-repository guidance.
|
||||
No test calls the real GitHub write APIs. Actionlint validates the workflow.
|
||||
|
||||
## Supersedes
|
||||
|
||||
None.
|
||||
|
||||
## Revisit when
|
||||
|
||||
A metadata-only labeling service is justified, or the source-size policy changes
|
||||
through a separately reviewed rule update.
|
||||
@@ -100,7 +100,7 @@ PROTOCOL_COMPATIBILITY_PATTERNS = (
|
||||
|
||||
|
||||
def git(*args: str) -> bytes:
|
||||
return subprocess.check_output(["git", *args], stderr=subprocess.DEVNULL)
|
||||
return subprocess.check_output(["git", "-c", "i18n.logOutputEncoding=utf-8", *args], stderr=subprocess.DEVNULL)
|
||||
|
||||
|
||||
def prohibited(text: str) -> bool:
|
||||
@@ -126,12 +126,12 @@ def prohibited_source_line(path: str, text: str) -> bool:
|
||||
|
||||
def changed_paths(*revision_args: str) -> list[str]:
|
||||
raw = git("diff", *revision_args, "--name-only", "-z", "--diff-filter=ACMR", "--", ".")
|
||||
return [path.decode("utf-8", "replace") for path in raw.split(b"\0") if path]
|
||||
return [path.decode("utf-8") for path in raw.split(b"\0") if path]
|
||||
|
||||
|
||||
def added_lines(path: str, *revision_args: str) -> Iterable[tuple[int, str]]:
|
||||
patch = git("diff", *revision_args, "--no-ext-diff", "--unified=0", "--", path)
|
||||
for line_no, raw_line in enumerate(patch.decode("utf-8", "replace").splitlines(), 1):
|
||||
for line_no, raw_line in enumerate(patch.decode("utf-8").splitlines(), 1):
|
||||
if raw_line.startswith("+") and not raw_line.startswith("+++"):
|
||||
yield line_no, raw_line[1:]
|
||||
|
||||
@@ -149,7 +149,7 @@ def scan_added_lines(*revision_args: str) -> list[str]:
|
||||
|
||||
|
||||
def added_patch_lines(patch: bytes, merge_parent_count: int | None) -> Iterable[tuple[int, str]]:
|
||||
lines = patch.decode("utf-8", "replace").splitlines()
|
||||
lines = patch.decode("utf-8").splitlines()
|
||||
if merge_parent_count is None:
|
||||
for line_no, raw_line in enumerate(lines, 1):
|
||||
if raw_line.startswith("+") and not raw_line.startswith("+++"):
|
||||
@@ -169,7 +169,7 @@ def added_patch_lines(patch: bytes, merge_parent_count: int | None) -> Iterable[
|
||||
|
||||
def scan_pending_commits(revision_range: str) -> list[str]:
|
||||
hits: list[str] = []
|
||||
commits = git("rev-list", "--reverse", revision_range).decode("ascii", "replace").splitlines()
|
||||
commits = git("rev-list", "--reverse", revision_range).decode("ascii").splitlines()
|
||||
for commit in commits:
|
||||
parents = git("rev-list", "--parents", "-n", "1", commit).decode("ascii").split()
|
||||
parent = parents[1] if len(parents) > 1 else None
|
||||
@@ -188,7 +188,7 @@ def scan_pending_commits(revision_range: str) -> list[str]:
|
||||
for raw_path in paths.split(b"\0"):
|
||||
if not raw_path:
|
||||
continue
|
||||
path = raw_path.decode("utf-8", "replace")
|
||||
path = raw_path.decode("utf-8")
|
||||
normalized = path.replace("\\", "/")
|
||||
if normalized in EXEMPT_PATHS:
|
||||
continue
|
||||
@@ -209,7 +209,7 @@ def scan_pending_commits(revision_range: str) -> list[str]:
|
||||
def scan_message(path: Path) -> list[str]:
|
||||
return [
|
||||
f"{line_no}:{line}"
|
||||
for line_no, line in enumerate(path.read_text(encoding="utf-8", errors="replace").splitlines(), 1)
|
||||
for line_no, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1)
|
||||
if prohibited(line)
|
||||
]
|
||||
|
||||
@@ -239,7 +239,7 @@ def scan_commit_messages(revision_range: str) -> list[str]:
|
||||
hits: list[str] = []
|
||||
commits = git("rev-list", "--reverse", revision_range).decode("ascii").splitlines()
|
||||
for commit in commits:
|
||||
message = git("show", "-s", "--format=%B", commit).decode("utf-8", "replace")
|
||||
message = git("show", "-s", "--format=%B", commit).decode("utf-8")
|
||||
for line_no, line in enumerate(message.splitlines(), 1):
|
||||
if prohibited(line):
|
||||
hits.append(f"commit-message:{commit}:{line_no}:{line}")
|
||||
@@ -263,7 +263,7 @@ def pending_push_range() -> str | None:
|
||||
stderr=subprocess.DEVNULL,
|
||||
check=False,
|
||||
)
|
||||
upstream = result.stdout.decode("utf-8", "replace").strip()
|
||||
upstream = result.stdout.decode("utf-8").strip()
|
||||
return f"{upstream}..HEAD" if result.returncode == 0 and upstream else None
|
||||
|
||||
|
||||
@@ -296,7 +296,7 @@ def main(argv: list[str]) -> int:
|
||||
if revision_range is None:
|
||||
print("No upstream branch; skipping pending-push name check")
|
||||
return 0
|
||||
message_text = git("log", "--format=%H:%s%n%b", revision_range).decode("utf-8", "replace")
|
||||
message_text = git("log", "--format=%H:%s%n%b", revision_range).decode("utf-8")
|
||||
hits = [
|
||||
f"commit-message:{line_no}:{line}"
|
||||
for line_no, line in enumerate(message_text.splitlines(), 1)
|
||||
@@ -321,4 +321,8 @@ def main(argv: list[str]) -> int:
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
raise SystemExit(main(sys.argv))
|
||||
try:
|
||||
raise SystemExit(main(sys.argv))
|
||||
except (OSError, UnicodeError, subprocess.CalledProcessError) as exc:
|
||||
print(f"ERROR: cannot read naming-check input: {exc}", file=sys.stderr)
|
||||
raise SystemExit(1)
|
||||
@@ -22,6 +22,8 @@ class NativeSuiteTests(unittest.TestCase):
|
||||
"scripts.tests.test_ci_native_tests",
|
||||
"scripts.tests.test_stable_release",
|
||||
"scripts.tests.test_ci_cache",
|
||||
"scripts.tests.test_platform_cfg",
|
||||
"scripts.tests.test_pr_size_workflow",
|
||||
):
|
||||
self.assertIn(contract, lint)
|
||||
self.assertIn("python scripts/ci_plan.py", lint)
|
||||
@@ -34,6 +36,13 @@ class NativeSuiteTests(unittest.TestCase):
|
||||
lint.index("Test CI contracts before matrix planning"),
|
||||
lint.index("python scripts/ci_plan.py"),
|
||||
)
|
||||
self.assertLess(
|
||||
lint.index("python3 scripts/check_platform_cfg.py"),
|
||||
lint.index("python scripts/ci_plan.py"),
|
||||
)
|
||||
platform_step = lint.split("Check platform cfg budget before native jobs", 1)[1].split(" - name:", 1)[0]
|
||||
self.assertNotIn("continue-on-error", platform_step)
|
||||
self.assertNotIn("--update", platform_step)
|
||||
for job, output in (("native-tests", "native_matrix"),
|
||||
("macos-release-check", "release_matrix")):
|
||||
body = workflow.split(f"\n {job}:\n", 1)[1]
|
||||
|
||||
@@ -0,0 +1,103 @@
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
from pathlib import Path
|
||||
import shutil
|
||||
import subprocess
|
||||
import textwrap
|
||||
import unittest
|
||||
|
||||
ROOT = Path(__file__).resolve().parents[2]
|
||||
RUNNER = r'''
|
||||
const fs = require('node:fs');
|
||||
const input = JSON.parse(fs.readFileSync(0, 'utf8'));
|
||||
const writes = [], failures = [], summaries = [];
|
||||
const listFiles = () => {}, listComments = () => {};
|
||||
const github = {
|
||||
rest: {
|
||||
pulls: {listFiles},
|
||||
issues: {
|
||||
listComments,
|
||||
addLabels: async (value) => writes.push(['add', value]),
|
||||
removeLabel: async (value) => writes.push(['remove', value]),
|
||||
createComment: async (value) => writes.push(['comment', value]),
|
||||
},
|
||||
},
|
||||
paginate: async (method) => method === listFiles ? input.files : [],
|
||||
};
|
||||
const summary = {addRaw(value) {summaries.push(value); return this;}, async write() {}};
|
||||
const core = {info() {}, setFailed(value) {failures.push(value);}, summary};
|
||||
const context = {
|
||||
repo: {owner: 'base', repo: 'project'},
|
||||
payload: {pull_request: {
|
||||
number: 1, labels: input.labels,
|
||||
head: {repo: input.deleted ? null : {full_name: input.fork ? 'fork/project' : 'base/project'}},
|
||||
}},
|
||||
};
|
||||
const AsyncFunction = Object.getPrototypeOf(async function () {}).constructor;
|
||||
new AsyncFunction('github', 'context', 'core', input.script)(github, context, core)
|
||||
.then(() => process.stdout.write(JSON.stringify({writes, failures, summaries})))
|
||||
.catch((error) => {console.error(error); process.exit(1);});
|
||||
'''
|
||||
|
||||
|
||||
class PrSizeWorkflowTests(unittest.TestCase):
|
||||
@classmethod
|
||||
def setUpClass(cls):
|
||||
cls.node = shutil.which('node')
|
||||
if cls.node is None:
|
||||
raise RuntimeError('Node.js is required for the PR size workflow contract')
|
||||
workflow = (ROOT / '.github/workflows/pr-size.yml').read_text(encoding='utf-8')
|
||||
# Execute the actual inline action, without duplicating its policy.
|
||||
cls.script = textwrap.dedent(workflow.split(' script: |\n', 1)[1])
|
||||
|
||||
def run_size(self, lines, *, fork=True, exempt=False, deleted=False):
|
||||
data = {
|
||||
'script': self.script, 'fork': fork, 'deleted': deleted,
|
||||
'labels': [{'name': 'size-exempt'}] if exempt else [],
|
||||
'files': [
|
||||
{'filename': 'src/example.rs', 'additions': lines, 'deletions': 0},
|
||||
{'filename': 'Cargo.lock', 'additions': 9000, 'deletions': 500},
|
||||
{'filename': 'docs/guide.md', 'additions': 9000, 'deletions': 500},
|
||||
],
|
||||
}
|
||||
result = subprocess.run(
|
||||
[self.node, '-e', RUNNER], input=json.dumps(data), text=True,
|
||||
encoding='utf-8', capture_output=True, check=False,
|
||||
)
|
||||
self.assertEqual(result.returncode, 0, result.stderr)
|
||||
return json.loads(result.stdout)
|
||||
|
||||
def test_small_fork_passes_without_write_permissions(self):
|
||||
result = self.run_size(127)
|
||||
self.assertEqual(result['failures'], [])
|
||||
self.assertEqual(result['writes'], [])
|
||||
self.assertIn('127 counted lines', result['summaries'][0])
|
||||
|
||||
def test_large_fork_is_rejected_before_optional_writes(self):
|
||||
result = self.run_size(1501)
|
||||
self.assertEqual(len(result['failures']), 1)
|
||||
self.assertIn('1501', result['failures'][0])
|
||||
self.assertEqual(result['writes'], [])
|
||||
|
||||
def test_exact_limit_and_existing_maintainer_exemption(self):
|
||||
self.assertEqual(self.run_size(1500)['failures'], [])
|
||||
self.assertEqual(self.run_size(1501, exempt=True)['failures'], [])
|
||||
|
||||
def test_same_repository_keeps_labels_and_oversize_guidance(self):
|
||||
small = self.run_size(127, fork=False)
|
||||
self.assertEqual(small['failures'], [])
|
||||
self.assertEqual(small['writes'][0][1]['labels'], ['size/M'])
|
||||
large = self.run_size(1501, fork=False)
|
||||
self.assertEqual(len(large['failures']), 1)
|
||||
self.assertEqual(large['writes'][0][1]['labels'], ['size/XL'])
|
||||
self.assertEqual(large['writes'][1][0], 'comment')
|
||||
|
||||
def test_deleted_fork_can_still_be_counted_without_writes(self):
|
||||
result = self.run_size(127, deleted=True)
|
||||
self.assertEqual(result['failures'], [])
|
||||
self.assertEqual(result['writes'], [])
|
||||
|
||||
|
||||
if __name__ == '__main__':
|
||||
unittest.main()
|
||||
@@ -20,6 +20,60 @@ def run_git(repository: Path, *args: str) -> None:
|
||||
|
||||
|
||||
class ProhibitedNamesTests(unittest.TestCase):
|
||||
def check_staged(self, repository: Path):
|
||||
return subprocess.run(
|
||||
[sys.executable, str(CHECKER), "staged"], cwd=repository,
|
||||
text=True, encoding="utf-8", capture_output=True,
|
||||
env={**os.environ, "PYTHONUTF8": "1"},
|
||||
)
|
||||
|
||||
@unittest.skipIf(os.name == "nt", "Windows filenames cannot contain raw invalid UTF-8 bytes")
|
||||
def test_invalid_utf8_path_cannot_turn_into_an_empty_diff(self) -> None:
|
||||
with tempfile.TemporaryDirectory() as directory:
|
||||
repository = Path(directory)
|
||||
run_git(repository, "init", "-q")
|
||||
with open(os.fsencode(repository) + b"/bad-\xff.rs", "wb") as output:
|
||||
output.write(b"Ghostty\n")
|
||||
run_git(repository, "add", "--all")
|
||||
result = self.check_staged(repository)
|
||||
self.assertNotEqual(result.returncode, 0)
|
||||
self.assertIn("utf-8", result.stderr.lower())
|
||||
|
||||
def test_invalid_utf8_text_and_message_are_rejected(self) -> None:
|
||||
with tempfile.TemporaryDirectory() as directory:
|
||||
repository = Path(directory)
|
||||
run_git(repository, "init", "-q")
|
||||
path = repository / "source.rs"
|
||||
path.write_bytes(b"safe \xff\n")
|
||||
run_git(repository, "add", "source.rs")
|
||||
staged = self.check_staged(repository)
|
||||
message = subprocess.run(
|
||||
[sys.executable, str(CHECKER), "message", str(path)], cwd=repository,
|
||||
text=True, encoding="utf-8", capture_output=True,
|
||||
env={**os.environ, "PYTHONUTF8": "1"},
|
||||
)
|
||||
for result in (staged, message):
|
||||
self.assertNotEqual(result.returncode, 0)
|
||||
self.assertIn("utf-8", result.stderr.lower())
|
||||
|
||||
def test_valid_unicode_and_control_character_paths_are_scanned(self) -> None:
|
||||
names = ["路径.rs"]
|
||||
if os.name != "nt":
|
||||
names.append("control\tline\n.rs")
|
||||
for name in names:
|
||||
with self.subTest(name=name), tempfile.TemporaryDirectory() as directory:
|
||||
repository = Path(directory)
|
||||
run_git(repository, "init", "-q")
|
||||
path = repository / name
|
||||
path.write_text("safe\n", encoding="utf-8")
|
||||
run_git(repository, "add", "--", name)
|
||||
self.assertEqual(self.check_staged(repository).returncode, 0)
|
||||
path.write_text("Ghostty\n", encoding="utf-8")
|
||||
run_git(repository, "add", "--", name)
|
||||
result = self.check_staged(repository)
|
||||
self.assertEqual(result.returncode, 1)
|
||||
self.assertIn("Ghostty", result.stderr)
|
||||
|
||||
def test_names_do_not_match_inside_unrelated_words(self) -> None:
|
||||
self.assertFalse(check_prohibited_names.prohibited("spotty network; tty72; netcatty_adapter"))
|
||||
self.assertTrue(check_prohibited_names.prohibited("Use Otty or TTY-7"))
|
||||
|
||||
Reference in new issue
Block a user