diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 9546e692..d391e86e 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -3,7 +3,16 @@ ## Summary -Closes # +## Issue references +Refs # + + ## Type - [ ] feat @@ -12,15 +21,31 @@ Closes # - [ ] chore / refactor / test / perf ## Checklist -- [ ] `python3 cc_peer.py list` works locally -- [ ] `python3 cc_peer.py list --host ` works over SSH -- [ ] `--dry-run` resolves a target without posting anything +- [ ] `python session_peer.py --version` / `--help` checked where relevant +- [ ] Generated standalone checked: `python tools/generate_session_peer.py --check` (or not run, with reason below) - [ ] Commits follow Conventional Commits -- [ ] Docs updated if behavior changed (README / SKILL.md / CLAUDE.md) +- [ ] Relevant [user documentation](https://github.com/abruption/session-peer/blob/main/docs/README.md), [CLI reference](https://github.com/abruption/session-peer/blob/main/docs/cli-reference.md) and translations updated, or marked not applicable - [ ] No secrets, tokens, or session keys in the diff +## Validation + + +| Area | Relevant check / evidence | +| --- | --- | +| Core | `python -m unittest discover -s tests/core -t . -v` | +| Adapters / Codex | `python -m unittest discover -s tests/adapters -t . -v`; `python -m unittest discover -s tests/codex -t . -v` | +| Distribution / docs | `python -m unittest discover -s tests/distribution -t . -v`; `python -m unittest discover -s tests/docs -t . -v` | +| Optional MCP / Relay | Install the relevant extra, then `python -m unittest discover -s tests/mcp_integration -t . -v` / `python -m unittest discover -s tests/relay -t . -v` | +| Optional Control | In `control/`: `npm ci`, `npm run typecheck`, `npm test`, `npm audit --omit=dev` | +| Control integration / built packages | Follow [test-suite prerequisites and artifact checks](https://github.com/abruption/session-peer/blob/main/tests/README.md) when relevant | + ## Verified against - + ## Notes diff --git a/.github/scripts/check_pr_references.py b/.github/scripts/check_pr_references.py new file mode 100644 index 00000000..0dd0b0a0 --- /dev/null +++ b/.github/scripts/check_pr_references.py @@ -0,0 +1,99 @@ +#!/usr/bin/env python3 +"""Warn about a small class of ambiguous closing phrases, without changing issues. + +This is a review aid, not GitHub's closing-reference parser. It does not decide +whether work is complete. It scans text (including quoted/code text), using a +short preceding-clause heuristic; false positives and missed cases are possible. +Manual sidebar links and commit-message references are outside this check; +merging into the default branch can close issues independently of this warning. +Only fixed messages are emitted: PR body content is never evaluated or reflected +into workflow commands or Markdown output. +""" + +import json +import os +from pathlib import Path +import re + + +MAX_EVENT_BYTES = 2 * 1024 * 1024 +CLOSING = re.compile( + r"\b(?:close[sd]?|fix(?:es|ed)?|resolve[sd]?)\b(?:\s*:\s*|\s+)" + r"(?:(?:[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+)?#[1-9][0-9]*\b|" + r"https://github\.com/[A-Za-z0-9_.-]+/[A-Za-z0-9_.-]+/issues/[1-9][0-9]*\b)", + re.IGNORECASE, +) +NEGATION = re.compile( + r"\b(?:not|never|without|no|cannot)\b|\b(?:ca|do|does|did|is|are|was|were|would|should|must|could)n['’]t\b|\bwon['’]t\b", + re.IGNORECASE, +) +PARTIAL = re.compile(r"\b(?:partial(?:ly)?|incomplete)\b", re.IGNORECASE) +MESSAGES = { + "negated": "A closing reference has nearby negation. GitHub may still close the issue; use a non-closing Refs reference for unfinished work.", + "partial": "A closing reference is described as partial or incomplete. Use Refs unless completing the issue is intentional.", + "unavailable": "PR reference analysis was unavailable. Review issue references manually; this warning does not determine GitHub closing behavior.", +} + + +def warnings_for_body(body): + """Return fixed warning keys; ordinary references/intentional closure pass.""" + found = set() + previous_end = 0 + for match in CLOSING.finditer(body): + # Reset after earlier references and sentence/paragraph boundaries. + # A single newline remains eligible for wrapped Markdown prose. + prefix = body[max(previous_end, match.start() - 160):match.start()] + prefix = re.split(r"[.!?;]|\n\s*\n", prefix)[-1] + if NEGATION.search(prefix): + found.add("negated") + if PARTIAL.search(prefix): + found.add("partial") + previous_end = match.end() + return tuple(key for key in ("negated", "partial") if key in found) + + +def event_warnings(path): + try: + with Path(path).open("rb") as stream: + raw = stream.read(MAX_EVENT_BYTES + 1) + if len(raw) > MAX_EVENT_BYTES: + return ("unavailable",) + event = json.loads(raw) + pull_request = event.get("pull_request") if isinstance(event, dict) else None + if not isinstance(pull_request, dict): + return ("unavailable",) + body = pull_request.get("body") + if body is None: + body = "" + if not isinstance(body, str): + return ("unavailable",) + return warnings_for_body(body) + except (OSError, ValueError, TypeError): + return ("unavailable",) + + +def main(): + warnings = event_warnings(os.environ.get("GITHUB_EVENT_PATH", "")) + for key in warnings: + print("::warning::" + MESSAGES[key]) + if not warnings: + print("No ambiguous closing phrase detected by this limited heuristic.") + summary = os.environ.get("GITHUB_STEP_SUMMARY") + if summary: + try: + with Path(summary).open("a", encoding="utf-8") as stream: + stream.write("## Issue-reference review aid\n\n") + if warnings: + for key in warnings: + stream.write("- " + MESSAGES[key] + "\n") + else: + stream.write("No ambiguous closing phrase detected. This is not proof of GitHub's complete closing behavior.\n") + stream.write("\nThis check does not edit descriptions, close issues or block intentional completion.\n") + except OSError: + print("::warning::" + MESSAGES["unavailable"]) + # The analyzer only warns; contributor/reviewer intent still controls closure. + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/.github/workflows/pr-reference-warning.yml b/.github/workflows/pr-reference-warning.yml new file mode 100644 index 00000000..6f96784d --- /dev/null +++ b/.github/workflows/pr-reference-warning.yml @@ -0,0 +1,24 @@ +name: PR reference warning + +on: + pull_request_target: + branches: [main] + types: [opened, edited, reopened, synchronize] + +permissions: + contents: read + +jobs: + references: + runs-on: ubuntu-latest + timeout-minutes: 5 + steps: + # Run code from the trusted default-branch workflow commit, never the PR head. + # A newly introduced checker first runs after it is merged into main. + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + ref: ${{ github.sha }} + persist-credentials: false + - name: Warn about ambiguous closing references + # The script reads the event JSON as data; no body text enters the shell. + run: python3 .github/scripts/check_pr_references.py diff --git a/tests/docs/test_pr_references.py b/tests/docs/test_pr_references.py new file mode 100644 index 00000000..b658ee79 --- /dev/null +++ b/tests/docs/test_pr_references.py @@ -0,0 +1,160 @@ +"""PR references remain review aids and never execute contributor text.""" + +import importlib.util +import json +import os +from pathlib import Path +import re +import subprocess +import sys +import tempfile +import unittest + + +ROOT = Path(__file__).resolve().parents[2] +SCRIPT = ROOT / ".github/scripts/check_pr_references.py" +WORKFLOW = ROOT / ".github/workflows/pr-reference-warning.yml" +spec = importlib.util.spec_from_file_location("check_pr_references", SCRIPT) +checker = importlib.util.module_from_spec(spec) +spec.loader.exec_module(checker) + + +class PrReferences(unittest.TestCase): + def check(self, body): + return checker.warnings_for_body(body) + + def test_nonclosing_references_and_intentional_completion_do_not_warn(self): + for body in ("Refs #12", "Related to #12 and #34", "Closes #12", + "Fixes owner/repository#34", "Resolves #12, #34", "", + "This does not change the protocol. Closes #12"): + with self.subTest(body=body): + self.assertEqual((), self.check(body)) + + def test_negated_closing_phrases_warn(self): + for body in ("Does not close #12", "This never fixes #12", "This can't close #12", + "Doesn't resolve owner/repository#12", "This won’t close #12", + "This is not intended to close #12", "Does not\nclose #12", + "This does NOT CLOSE: #12", "Does not resolve https://github.com/owner/repository/issues/12"): + with self.subTest(body=body): + self.assertEqual(("negated",), self.check(body)) + + def test_partial_closing_phrases_warn_without_disallowing_references(self): + self.assertEqual(("partial",), self.check("Partially fixes #12")) + self.assertEqual(("partial",), self.check("Incomplete work resolves #12")) + self.assertEqual((), self.check("Partial work. Refs #12")) + + def test_multiple_references_have_separate_preceding_context(self): + self.assertEqual(("negated",), self.check("Does not close #12, but closes #34")) + self.assertEqual(("negated", "partial"), self.check("Does not close #12. Partially fixes #34")) + self.assertEqual((), self.check("Refs #12; Closes #34")) + + def test_trailing_qualification_is_a_manual_review_limit(self): + # The bounded preceding-clause heuristic intentionally cannot certify + # intent stated after a closing reference. Review remains necessary. + for body in ("Closes #12, but this work is incomplete", "Fixes #12 (not yet)"): + self.assertEqual((), self.check(body)) + + def run_event(self, root, event): + path = root / "event.json" + path.write_text(json.dumps(event), encoding="utf-8") + summary = root / "summary.md" + result = subprocess.run( + [sys.executable, str(SCRIPT)], + env=dict(os.environ, GITHUB_EVENT_PATH=str(path), GITHUB_STEP_SUMMARY=str(summary)), + cwd=root, capture_output=True, text=True, timeout=5, + ) + self.assertEqual(0, result.returncode, result.stderr) + return result.stdout, summary.read_text(encoding="utf-8") + + def test_actual_event_script_handles_body_edits_without_changing_event(self): + with tempfile.TemporaryDirectory(prefix="codex-pr-reference-") as folder: + root = Path(folder) + for action, body, warning in (("opened", "Refs #12", False), + ("edited", "Does not close #12", True), + ("edited", "Closes #12", False)): + event = {"action": action, "pull_request": {"body": body}} + output, _ = self.run_event(root, event) + self.assertEqual(warning, "::warning::" in output) + self.assertEqual(event, json.loads((root / "event.json").read_text())) + + def test_body_is_data_and_never_workflow_command_or_summary_content(self): + with tempfile.TemporaryDirectory(prefix="codex-pr-reference-") as folder: + root = Path(folder) + marker = root / "executed" + body = "Does not close #12\n::error::INJECTED\n$(touch " + str(marker) + ")\n" + output, summary = self.run_event(root, {"pull_request": {"body": body}}) + self.assertIn("::warning::", output) + self.assertNotIn("INJECTED", output + summary) + self.assertFalse(marker.exists()) + + def test_missing_invalid_and_oversize_events_only_warn(self): + with tempfile.TemporaryDirectory(prefix="codex-pr-reference-") as folder: + root = Path(folder) + for event in ({}, [], {"pull_request": {"body": 123}}): + output, _ = self.run_event(root, event) + self.assertIn(checker.MESSAGES["unavailable"], output) + path = root / "oversize.json" + path.write_bytes(b" " * (checker.MAX_EVENT_BYTES + 1)) + self.assertEqual(("unavailable",), checker.event_warnings(path)) + self.assertEqual(("unavailable",), checker.event_warnings(root / "absent")) + + def test_long_whitespace_without_a_reference_is_not_a_regex_time_trap(self): + with tempfile.TemporaryDirectory(prefix="codex-pr-reference-") as folder: + output, _ = self.run_event(Path(folder), { + "pull_request": {"body": "Does not close" + " " * 65536 + "not-a-reference"}}) + self.assertNotIn("::warning::", output) + + def test_workflow_uses_only_trusted_base_code_and_read_only_permissions(self): + text = WORKFLOW.read_text() + self.assertIn("pull_request_target:", text) + self.assertIn("branches: [main]", text) + self.assertIn("types: [opened, edited, reopened, synchronize]", text) + self.assertIn("ref: ${{ github.sha }}", text) + self.assertIn("persist-credentials: false", text) + self.assertEqual(["contents: read"], re.findall(r"^ ([a-z-]+: (?:read|write))$", text, re.M)) + self.assertNotRegex(text, r"(?m)^\s+[a-z-]+:\s*write\b") + self.assertNotRegex(text, r"(?m)^\s*permissions:\s*write-all\b") + self.assertNotIn("secrets.", text) + self.assertNotIn("pull_request.head", text) + self.assertNotIn("pull_request.base.sha", text) + self.assertNotIn("pull_request.body", text) + self.assertNotIn("refs/pull", text) + self.assertIn("run: python3 .github/scripts/check_pr_references.py", text) + uses = re.findall(r"uses: ([^\s]+)", text) + self.assertTrue(all(re.fullmatch(r"[\w-]+/[\w-]+@[0-9a-f]{40}", ref) for ref in uses)) + + def test_template_defaults_to_nonclosing_references_and_current_checks(self): + text = (ROOT / ".github/pull_request_template.md").read_text() + self.assertIn("Refs #