diff --git a/.github/workflows/linux-lua.yml b/.github/workflows/linux-lua.yml index feea014..8205936 100644 --- a/.github/workflows/linux-lua.yml +++ b/.github/workflows/linux-lua.yml @@ -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 diff --git a/.github/workflows/pr-size.yml b/.github/workflows/pr-size.yml index 3ad7282..c209452 100644 --- a/.github/workflows/pr-size.yml +++ b/.github/workflows/pr-size.yml @@ -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 = ""; 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.`); diff --git a/architecture/notes/scripts/ci/2026-09-23-prohibited-name-range-check.md b/architecture/notes/scripts/ci/2026-09-23-prohibited-name-range-check.md index 0f1a507..836923b 100644 --- a/architecture/notes/scripts/ci/2026-09-23-prohibited-name-range-check.md +++ b/architecture/notes/scripts/ci/2026-09-23-prohibited-name-range-check.md @@ -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 diff --git a/architecture/notes/scripts/ci/2026-09-24-fork-size-check.md b/architecture/notes/scripts/ci/2026-09-24-fork-size-check.md new file mode 100644 index 0000000..056ebda --- /dev/null +++ b/architecture/notes/scripts/ci/2026-09-24-fork-size-check.md @@ -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. diff --git a/scripts/check_prohibited_names.py b/scripts/check_prohibited_names.py index 5382954..a9366b5 100644 --- a/scripts/check_prohibited_names.py +++ b/scripts/check_prohibited_names.py @@ -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) diff --git a/scripts/tests/test_ci_native_tests.py b/scripts/tests/test_ci_native_tests.py index 5a771fe..0055e55 100644 --- a/scripts/tests/test_ci_native_tests.py +++ b/scripts/tests/test_ci_native_tests.py @@ -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] diff --git a/scripts/tests/test_pr_size_workflow.py b/scripts/tests/test_pr_size_workflow.py new file mode 100644 index 0000000..afefd06 --- /dev/null +++ b/scripts/tests/test_pr_size_workflow.py @@ -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() diff --git a/scripts/tests/test_prohibited_names.py b/scripts/tests/test_prohibited_names.py index e051dfc..451d740 100644 --- a/scripts/tests/test_prohibited_names.py +++ b/scripts/tests/test_prohibited_names.py @@ -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"))