Restructure policy checks into a single tool [GH #164]

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: sdarwin

.git-hooks/pre-push.py

@@ -32,20 +32,15 @@
 # pushing anything. Information about why the push is rejected may be sent to
 # the user by writing to standard error.
 #
-# The policy this hook applies is defined in the edgpolicy library, which the
-# policy CI workflow also uses.  This hook exists to give fast local feedback;
-# CI is what actually enforces the policy.
+# All of the policy itself lives in edg-check-policy, which CI and users at a
+# shell run too.  This hook only arranges for a usable terminal and then asks
+# that tool to review everything that has not been pushed yet.
 #
 
 import platform
+import shutil
 import subprocess
 import sys
-import tempfile
-
-import edgpolicy
-import edgutil
-
-from pathlib import Path
 
 if platform.system() == 'Windows':
   # On Windows we need to use the kernel API to detect if this script is
@@ -68,172 +63,24 @@
       creationflags = subprocess.CREATE_NEW_CONSOLE
     )
     sys.exit(delegate_proc.returncode)
-else:
-  # On Unix-like systems we can regain access to terminal input using /dev/tty.
-  sys.stdin = open('/dev/tty')
-
-#
-# How each kind of finding reported by edgpolicy is introduced to the user.
-#
-FINDING_KIND_HEADINGS = {
-  'misspelling': 'New misspelled words:',
-  'typo': 'New typos:',
-  'coding-error': 'Coding errors:',
-  'overlong-line': 'Lines too long:'
-}
-
-def print_indented(text, prefix):
-  '''Print each line of text with the given prefix.'''
-  if not text:
-    return
-
-  for line in text.splitlines():
-    print(f"{prefix}{line}")
-
-class PrePushChecker:
-  '''Runs EDG pre-push policy checks against new commits and changed files.'''
-
-  def __init__(self):
-    self.errors = 0
-    self.repository = edgpolicy.GitRepository(edgutil.find_mono_repo_or_exit())
-
-  def confirm_okay(self):
-    '''Ask the user whether a policy warning is acceptable.
-
-    When stdout is a terminal, prompt on /dev/tty for y/n/q. Otherwise record
-    an error without prompting (e.g. non-interactive pushes).
-    '''
-    if not sys.stdout.isatty():
-      self.errors = 1
-      return
-
-    tries = 0
-    while True:
-      response = input('    Okay? [ynq] ').lower()
-
-      if response == 'y':
-        return
-      if response == 'n':
-        self.errors = 1
-        return
-      if response == 'q':
-        self.errors = 1
-        sys.exit(1)
-
-      print(
-        "Please answer 'y' (to accept), 'n' (to continue check) or "
-        "'q' (to quit)."
-      )
-      tries += 1
-      if tries >= 10:
-        print('Too many attempts.')
-        sys.exit(1)
-
-  def check_commits(self):
-    '''Walk commits until one already on a remote branch is found.
-
-    As commits are walked, check to make sure they match formatting guidance.
-
-    Returns the oldest new commit hash, or None if there are no new commits.
-    A commit subject that fails the policy aborts the push outright when
-    edgpolicy says the rule is binding, and is otherwise offered for
-    confirmation like any other finding.
-    '''
-    first_new_commit = None
-
-    commits = self.repository.lines(['log', '--pretty=%H'])
-    for commit in commits:
-      # Break if the remote already has this commit.
-      contains = self.repository.run(
-        ['branch', '-r', '--contains', commit], check = False
-      ).stdout.strip()
-      if contains:
-        break
 
-      print('New commit:')
-      print('')
-      show_output = self.repository.output(['show', '--no-patch', commit])
-      print_indented(show_output, '  ')
-      print('')
+TOOL_PATH_CHECK_POLICY = shutil.which('edg-check-policy')
 
-      subject_error = edgpolicy.commit_subject_error(
-        self.repository.subject_of(commit)
-      )
-      if subject_error is not None:
-        print(f"  [POLICY] {subject_error}")
-        print_indented(edgpolicy.SUBJECT_POLICY_DESCRIPTION, '  ')
-        if edgpolicy.ENFORCE_COMMIT_SUBJECT_POLICY:
-          sys.exit(1)
-        self.confirm_okay()
-
-      first_new_commit = commit
-
-    return first_new_commit
-
-  def report_findings(self, findings):
-    '''Print findings grouped by kind, confirming once per group.'''
-    for kind, heading in FINDING_KIND_HEADINGS.items():
-      messages = [
-        message for finding_kind, message in findings
-        if finding_kind == kind
-      ]
-      if not messages:
-        continue
-
-      print(f"    {heading}")
-      for message in messages:
-        print(f"    - {message}")
-      self.confirm_okay()
-
-  def check_changed_files(self, first_new_commit):
-    '''Run per-file policy checks for files changed since
-    first_new_commit~1.'''
-    base_rev = f"{first_new_commit}~1"
-    print('Checking changed files:')
-
-    containing_tmp_dir = self.repository.repo_dir / '.tmp'
-    containing_tmp_dir.mkdir(exist_ok = True)
-
-    with tempfile.TemporaryDirectory(
-      dir = str(containing_tmp_dir)
-    ) as tmp_dir_str:
-      tmp_dir = Path(tmp_dir_str)
-
-      change_spec = edgpolicy.collect_change_spec(
-        self.repository, base_rev, 'HEAD', tmp_dir
-      )
-
-      use_docker = edgpolicy.should_use_docker(self.repository.repo_dir)
-      results = edgpolicy.run_file_checks(
-        self.repository, change_spec, tmp_dir, use_docker = use_docker
-      )
-      if results is None:
-        sys.exit(1)
-
-      for file_path_str in sorted(results):
-        print(f"  Checking file: {file_path_str}")
-
-        findings = edgpolicy.summarize_file_result(results[file_path_str])
-        if any(kind == 'spelling-unavailable' for kind, _ in findings):
-          print('    Spell check failed:')
-          if not use_docker:
-            print('      docker not used.')
-          print('      aspell not found or missing dictionary.')
-
-        self.report_findings(findings)
-
-  def run(self) -> bool:
-    '''Execute the full pre-push check sequence.'''
-    first_new_commit = self.check_commits()
-
-    # Skip per file checks if we didn't actually detect a new commit.
-    if first_new_commit is not None:
-      self.check_changed_files(first_new_commit)
+def main():
+  if TOOL_PATH_CHECK_POLICY is None:
+    print(
+      'edg-check-policy tool is required, but was not found.\n'
+      'Has dev-init.py been run, and is dev_tools/bin on PATH?',
+      file = sys.stderr
+    )
+    sys.exit(1)
 
-    return self.errors == 0
+  completed_process = subprocess.run([
+    TOOL_PATH_CHECK_POLICY,
+    '--unpushed',
+    '--interactive'
+  ])
 
-def main():
-  checker = PrePushChecker()
-  sys.exit(0 if checker.run() else 1)
+  sys.exit(completed_process.returncode)
 
 main()

CONTRIBUTING.md

@@ -81,33 +81,22 @@ existing issues and discussion (it's possible someone is already working on
 the item or that there has been significant discussion about the desired
 implementation characeristics).
 
-### What CI Enforces
+### Automated Checks
 
-Some of the items below are checked automatically on every pull request by
-the `Policy Check` workflow, which runs the same rules as the `pre-push`
-hook installed by `dev-init.py`.
-
-These **fail** the check:
-
-- Changed files must stay within 79 columns, must comment their `#endif`
-  and `#else` directives, and must not contain repeated-word typos.
-
-These are **reported but do not fail** the check:
-
-- Commit subjects should carry a bracketed tag naming at least one GitHub
-  issue or legacy PR, such as `[GH #213]` or `[EDGcpfe/29032]`.  Whether
-  commits that belong to no issue must still carry an empty `[]` is an
-  open question, so this is advisory for now.
-- Spelling, because there is no project dictionary and a compiler code
-  base trips `aspell` constantly.  Please still read the spelling
-  annotations; they catch real mistakes.
-
-You can run the same check locally before opening a pull request:
+Some of the checklist below is checked automatically when you open a pull
+request.  `edg-check-policy` is the tool that does it, and you can run it
+yourself at any point to see what a pull request would report:
 
 ```sh
 edg-check-policy --base-rev origin/main
 ```
 
+Commit subjects must name the work they belong to, and changed files must
+follow the project's coding style; both will fail the check.  Spelling is
+reported but does not fail, because there is no project dictionary and a
+compiler code base trips `aspell` constantly.  Please still read what it
+says; it catches real mistakes.
+
 ### Checklist for Success
 
 > [!NOTE]

HACKING.md

@@ -12,26 +12,6 @@ As a result it is *highly recommended* to use the Docker based development
 process as this will typically provide the best experience (even if directly
 running a Linux-based operation system).
 
-## Project Policy Checks
-
-`dev-init.py` installs a `pre-push` hook that checks new commits against the
-project's commit message and coding style policies.  The hook exists to tell
-you about problems before you push; it is not what enforces them.  It is only
-present if you ran `dev-init.py`, it is skipped by `git push --no-verify`, and
-it never runs for work done through the GitHub web interface.
-
-Enforcement happens in the `Policy Check` workflow, which applies the same
-rules from the same `edgpolicy` library when a pull request is opened.  To
-run those checks yourself at any point:
-
-```sh
-edg-check-policy --base-rev origin/main
-```
-
-Use `--strict` to also fail on the spelling findings that the workflow only
-reports as warnings.  See `.github/rulesets/README.md` for how the checks are
-tied to branch protection.
-
 ## Prerequisites
 
 The first step (regardless of Docker use) is to make EDG development tools