Enforce commit and file policy in CI rather than only at push time

[sdarwin]

Today the project's commit and coding-style policies live entirely in a
client-side pre-push hook. That hook is good at telling you about problems
early, but it cannot enforce anything: it only exists if you ran
dev-init.py, git push --no-verify skips it, and it never runs for work
done through the GitHub web UI. Neither of the two working copies used while
preparing this PR had it installed.

This PR moves the enforcement of those rules to a GitHub Actions job, while
keeping the hook as fast local feedback. Both now read the same rules from
one shared library, so they cannot drift apart.

Important

Merging this PR enforces nothing on its own. It makes a check exist
and go green or red. A repository admin has to mark that check as required
afterwards. See What a maintainer has to do by hand.

---

Background: why not a server-side hook?

The request that prompted this was for "server-side scripts" to enforce
commit restrictions, and the objection was that those need GitHub
Enterprise. That objection is half right, and the conclusion drawn from it
was wrong.

True server-side pre-receive hooks are GitHub Enterprise Server only.
But they are not the mechanism this project needs. A required status
check gives the same guarantee — it runs code the project controls, on
GitHub's infrastructure, and a contributor cannot skip it — and rulesets,
required status checks, required reviews and CODEOWNERS are all free on
public repositories.

The one capability genuinely out of reach on a standard account is push
rulesets (restricting file paths, file extensions, or file sizes at push
time), which need an organization on GitHub Team or Enterprise Cloud. If
anyone specifically wants "block pushes containing file X", that has to be a
CI check instead.

What was actually in place before this PR

Layer	State
Client-side hooks	One: .git-hooks/pre-push.py. No pre-commit, no commit-msg.
Ruleset	One, Default Branch Protection (id 24309424), containing only deletion and non_fast_forward.
Required reviews	None.
Required status checks	None.
Required pull request	None — direct pushes to main were allowed.
CODEOWNERS	Absent.

Four workflows already ran on pull_request, but because no check was
required, a red X blocked nothing.

So yes: opening a regular pull request bypassed every policy check. It was
in fact worse than that, because a direct push to main bypassed them too.

---

The three categories, and where the overlap bites

It is worth being precise about this, because the middle case is the one
that causes confusion.

1. Things that are purely files in the repo. Workflows, the hook, the
checker code. Fully version controlled, and this PR delivers them outright.

2. Things that only exist in GitHub's settings. Rulesets and branch
protection. GitHub does not read these from the repository. An admin has
to apply them. This is the unavoidable click-click part.

3. The overlap — and it cuts both ways.

• A workflow file is version controlled and runs on every PR, but it blocks
  nothing until its job name is listed as a required status check in a
  ruleset (category 2).
• CODEOWNERS is the mirror image: the file is version controlled, but it
  gates nothing until a ruleset turns on require_code_owner_review
  (category 2).

To get as close to file-based management as GitHub allows, this PR commits
the intended ruleset as JSON at
.github/rulesets/default-branch-protection.json, with the exact
gh api --input command to apply it.

Note

That JSON file is documentation plus a payload, not configuration GitHub
reads. Editing it changes nothing until someone applies it. Its value is
that a change to project enforcement gets proposed, reviewed and recorded
like a code change, instead of appearing silently in a settings page with
no history and no discussion.

---

What changed

File
dev_tools/pylibs/edgpolicy/__init__.py	New. Single source of truth for the policy.
dev_tools/bin/edg-check-policy	New. Non-interactive CLI over a revision range.
.github/workflows/policy-check.yml	New. The job that runs it on every PR.
.github/rulesets/default-branch-protection.json	New. Intended branch protection.
.github/rulesets/README.md	New. What enforces what, and how to apply it.
.github/CODEOWNERS	New, fully commented out (see below).
.git-hooks/pre-push.py	Now a thin consumer of edgpolicy.
dev_tools/typecheck-python.sh	Registers the new code for strict mypy.
CONTRIBUTING.md, HACKING.md	Document what is enforced and how to run it locally.

edgpolicy is almost entirely a move, not a rewrite. The excluded path
prefixes come verbatim from the hook, the 79-column limit and all the file
checks come from edg-check-file-changes, and the commit subject rule is the
hook's regex. There are exactly three deliberate behavioural changes, all
called out below: the subject rule no longer accepts an empty [], deleted
files are no longer checked, and binary files are skipped.

CODEOWNERS ships entirely commented out because the @edgcpp/* teams do
not exist yet and GitHub reports an invalid CODEOWNERS file if they are
referenced. Uncomment as the teams are created.

Running it yourself

edg-check-policy --base-rev origin/main

Flags: --head-rev, --skip-commits, --skip-files, --strict,
--no-docker.

---

Design decision: what blocks, and what only warns

CI has no human to answer a prompt, so every check the hook treated as
"warn and ask y/n" had to be classified as blocking or advisory. Rather
than guess, I measured the ten most recent commits:

Commit	Files	Overlong lines	Spelling
White space lost in pragma text [GH #213]	2	0	1
Implement CWG 2355 [GH #197]	3	0	1
Accept bracketed GH #XYZ []	1	0	3
Improve coding style documentation []	1	0	1
Add GitHub workflow support for docs []	10	1	27
Add automatic build configuration []	9	3	52
dev-init: fix direnv discovery messages []	1	0	0
Better stack address cleanup []	2	0	9

#endif / #else / closing-comment / repeated-word-typo findings were zero
across all ten.

The mechanical checks fire 0–3 times on a normal commit and are enforceable.
aspell fires 1–52 times on every commit, because there is no project
dictionary and it flags ordinary identifiers. Dogfooding this PR produced
142 spelling warnings (aspell, bool, cwd, stdout, tempfile, …) and
zero real violations.

Blocking: lines over 79 columns; #endif / #else without a comment;
closing-comment errors; repeated-word typos.

Advisory (reported, does not fail): new misspellings; spell-check
unavailable; commit subject, see below.

--strict promotes everything to blocking for anyone who wants it locally.
A project aspell dictionary would be the natural follow-up that lets
spelling become blocking.

Design decision: the commit subject rule is advisory for now

The old regex was:

^.*\[(GH #[0-9]+|(EDG[cfjp]+fe/([0-9]+))|,)*\].*$

The inner group can repeat zero times, so a literal empty [] satisfied
it. That is not theoretical: a third of recent subjects end in [], and
commit 5ecc08269c ("Accept bracketed GH #XYZ as a valid commit message
[]") made it deliberate. The requirement that commits reference an issue
was, in practice, unenforced.

The new rule requires at least one real reference:

Subject	Result
Fix thing [GH #213]	pass
Fix thing [EDGcpfe/29032]	pass
Fix bug [EDGcpfe/29055,EDGcpfe/28999]	pass
Fix bug [GH #1, GH #2]	pass
Mixed [nope] and [GH #7]	pass
...valid commit message []	empty tag
No tag at all	no bracketed tag
Bad ref [GH 213]	not a valid reference list
Comma junk [,,]	not a valid reference list

Note

Picking up the maintainer comment that "we could also drop the requirement
that non-issue linked commits need an empty [] attached to them" — this
check is wired up as a warning, not an error, pending that discussion.

Re-enabling it is a one-line change, and both the hook and the CI job read
that single flag:

# dev_tools/pylibs/edgpolicy/__init__.py
ENFORCE_COMMIT_SUBJECT_POLICY = False   # set True to make it binding

With the flag False the CI job reports the subject as a warning and the
hook asks for confirmation; with it True the CI job fails and the hook
aborts the push. Verified both ways.

Open question for reviewers: if the [] requirement is formally
dropped, should a subject with no tag at all stay a warning, or should the
check only complain about malformed references such as [GH 213]? The
awkward case is [WIP], which is indistinguishable from a typo'd reference
without heuristics, which is why this PR keeps one severity for the whole
check rather than splitting it.

---

Edge cases found and handled

Binary files crashed the checker. edg-check-file-changes decodes
aspell output as UTF-8, so handing it an image raises UnicodeDecodeError
part way through. The exception is caught and the file is dropped from the
results, so the run still reports success — a silent skip, with an alarming
Python traceback in the log. This is not hypothetical: the repo tracks four
binary files outside tests/ (doc/source/_static/doc-logo-{dark,light}.png,
dev_tools/services/acknowledg/client.png, and client/public/favicon.ico),
so updating the doc logo would have triggered it.

collect_change_spec now skips blobs that contain a NUL byte or do not
decode as UTF-8. Verified: adding a 20 KB binary alongside a markdown file
now checks the markdown, skips the binary, and emits no traceback.

Silent skips are now visible. Because the checker drops a file from its
output if checking it raised, a missing result meant "unchecked", not
"passed". The CLI now diffs the requested files against the returned ones
and warns about any difference.

Deleted files are no longer checked. --diff-filter=d was added;
previously a deletion was checked as an empty file.

Rebases and stale base branches. The CLI resolves the merge base of
--base-rev and --head-rev rather than diffing against the base tip, so a
branch that has fallen behind main, or has merged main in, is not blamed
for other people's commits.

Annotation safety. Findings can quote a line from the diff, so all line
breaks (\n and \r) are flattened before being written as a workflow
command.

Testing

• Subject rule checked against the nine cases in the table above.
• Clean range → exit 0. Range with real violations → exit 1, correctly
  flagging three overlong lines in cmake/wrapper.cmake.
• Dogfooded on this PR: caught a genuine 79-column violation in the new
  edgpolicy module (fixed); now passes with 142 spelling warnings.
• Binary-file regression tested before and after the fix.
• ENFORCE_COMMIT_SUBJECT_POLICY verified in both positions, for both
  the CLI and the hook.
• Refactored pre-push hook driven through a pty: exits 0, checks 10
  files, groups prompts per finding kind per file as before.
• GITHUB_ACTIONS=1 produces well-formed ::error / ::warning
  annotations.
• Strict mypy clean on modern mypy, and on the 3.6 floor replicated in
  python:3.6-slim with mypy==0.971.
• All five dev_tools C++ helpers compile with g++ -std=c++14.
• Ruleset JSON and workflow YAML parse; the job name
  Commit and file policy matches the required_status_checks context.

Warning

The workflow has never run on GitHub, because the branch did not exist
upstream. This PR is its own first smoke test. The step most likely to
need a tweak is the cmake build of the dev_tools helpers on
ubuntu-latest.

Why the job runs on stock ubuntu-latest rather than edgcpp/ci-runner:
the file checks need aspell, which the CI image does not carry
(ci-runner is build-base plus nodejs; aspell-en is only in the
dev-env image).

---

What a maintainer has to do by hand

Nothing here changes repository settings, and merging this alone enforces
nothing new.

1. Merge this PR and let Policy Check run once. GitHub only offers a
   status check by name after it has been seen at least once.

2. Apply the ruleset, as a repository admin:

   gh api repos/edgcpp/compiler/rulesets --jq '.[] | "\(.id)\t\(.name)"'
   
   gh api --method PUT repos/edgcpp/compiler/rulesets/24309424 \
     --input .github/rulesets/default-branch-protection.json

   Or in the UI, under Settings → Rules → Rulesets → Default Branch
   Protection, keeping Deletion and Non-fast-forward ticked and adding:

   • Require a pull request before merging
     • Required approvals: 1
     • Dismiss stale approvals when new commits are pushed
     • Require approval of the most recent reviewable push
     • Allowed merge methods: squash and rebase only (no merge commits)
   • Require status checks to pass
     • Require branches to be up to date before merging
     • Commit and file policy
     • typecheck-python (3.6)
     • typecheck-python (3.14)

3. Also untick Settings → General → Pull Requests → Allow merge commits.
   allowed_merge_methods governs the merge button; unticking the
   repository option closes the gap properly. A required_linear_history
   rule is the stricter alternative.

4. Decide on bypass actors. The JSON ships with bypass_actors: [], so
   the rules apply to admins too. If maintainers need an escape hatch, add
   them there rather than disabling the rule, so the exception is visible.

5. (Optional) Settings → Actions → General → Fork pull request
   workflows. The default, "Require approval for first-time
   contributors", is the right one; just be aware it means the policy check
   does not report on a brand new contributor's PR until a maintainer
   approves the run.

6. (Optional, later) Create the @edgcpp/* teams, uncomment the matching
   lines in .github/CODEOWNERS, and enable code owner review.

Checks deliberately not required

Build & Test CI and Strict Build Check run on every PR but are not in
the required list. The test legs carry a 360 minute timeout and the Windows
build pulls an external SoftFloat checkout, so making them blocking is a
scheduling and flakiness decision for the maintainers rather than a policy
one. They can be added later by job name, for example Test edg_x86_64 or
Linux GCC debug.

---

Pre-existing issues found but deliberately left alone

These are all visible from the code this PR touches, but fixing them would
change what is checked, which does not belong in a PR about where it is
checked. Happy to file them as issues.

1. edg-check-file-changes, _file_check_config(): elif suffix == 'txt': is missing a dot. No suffix is ever 'txt', so .txt files fall
   through to the else branch and do get line-length checked, which looks
   like the opposite of the intent.

2. CRLF costs a column. The 79-column check does
   len(line.rstrip('\n').expandtabs()), so a line committed with CRLF
   counts its carriage return. A Windows-authored file with exactly 79
   columns would be reported at 80. rstrip('\r\n') would fix it. No
   tracked file currently hits this.

3. The hook ignores its stdin. pre-push is handed <local ref> <local sha> <remote ref> <remote sha> lines on stdin, but
   check_commits() walks git log from HEAD, so pushing a branch that
   is not the current checkout is not inspected correctly.

4. confirm_okay() is inverted for non-interactive use. When stdout is
   not a TTY it records an error instead of prompting, which makes scripted
   pushes stricter than interactive ones. This also means an advisory
   finding can block a non-interactive push.

[wchilders-nvidia]

First thanks for making progress on this; I opened #164 but haven't had time to make progress on it.

The comments attached I would use as possible agent feedback (possibly including some AGENTS.md and related updates?) or guiding principles to consider (I believe this is primarily LLM authored?).

I think what I'd actually like to see here after sitting with PR for a bit is to take a Unix-like approach to abstraction. Let's just move most of the pre-push.py logic to edg-check-policy and then have pre-push.py be a wrapper script that invokes edg-check-policy with the correct commit range and flags asking for an interactive review.

In that situation we don't need to try and wrangle effectively two different command line tools into having a similar textual-interface with common abstraction in a library ... we just have one command line tool, with one interface, and sometimes the hooks use it, sometimes CI uses it, and sometimes the user (human or agent) might use it directly. edg-check-file-changes still needs to exist as a lower level command-line tool in this model ... but that's not all that different from what we've done with other tools (having a higher level tool -- e.g., edg-docker-shell -- and a lower level tool -- e.g., edg-exec).

[wchilders-nvidia]

I'd prefer a name that implies some decision is being made vs a concrete action being taken.

Like check_for_subject_errors and then maybe return a List[str] just to future proof a bit?

[wchilders-nvidia]

I think I'd side with ... let's keep the policy and remove codifying indecision. We can always relax the rules later.

[wchilders-nvidia]

I'm not sure we want this note here. Someone reading HACKING.md wouldn't even know what dev-init.py is yet (at this point in the file), wouldn't have git push --no-verify access to the main repository (I hope), and probably wouldn't care about the minutia of policy enforcement.

[wchilders-nvidia]

This seems like a good section to have, but we probably need to make it more general (not just to the new policy check workflow). I can help write up some copy once we've decided precisely what we're enforcing.

[wchilders-nvidia]

This is kind of an odd function; it's basically repacking the dict and it loses some of the decisions made in the existing pre-push.py for presentation.

For instance I think the overlong lines reporting (while now more general) is a bit of a regression. The previous code:

        overlong_lines = file_results['overlong_lines']
        if len(overlong_lines) != 0:
          for line_no, (line_start, lines) in overlong_lines.items():
            print(f"    Line {line_no} too long:")
            line_end = line_start + (len(lines) - 1)
            print(f" {line_start} -> {line_end} ".center(79, '='))
            for line in lines:
              print(line, end = '')
            print('=' * 79)

would actually print context in a window with = added to visually show what was too long, e.g.:

/*snip*/ =====
/*snip*/ this is too long
/*snip*/ =====

[sdarwin]

Claude writes:
Thanks — the "one tool, several callers" framing is clearly the right shape, and
the edg-docker-shell / edg-exec precedent makes the layering obvious. I'll
restructure along these lines:

• edg-check-file-changes stays as the low-level tool, unchanged.
• edg-check-policy becomes the one high-level tool and holds the logic that
  currently lives in pre-push.py, including its presentation.
• pre-push.py becomes a wrapper: the Windows standalone-console handling,
  then an invocation of edg-check-policy with the range and an
  interactive-review flag.
• dev_tools/pylibs/edgpolicy/ goes away entirely. With one tool there is
  nothing left for it to abstract over.

I'll also fold in the specific points: rename to check_for_subject_errors
returning List[str], drop summarize_file_result so each category keeps its
own presentation (including restoring the =-delimited context window for
overlong lines — agreed that flattening it was a regression), remove the
ENFORCE_COMMIT_SUBJECT_POLICY flag, and pull the policy note back out of
HACKING.md.

Four things I'd like to confirm before I push the restructure, because each
one changes the result and they're all cheap to settle now.

1. Does [] still satisfy the subject policy?

This is the one I'm least sure I've read correctly, and it's load-bearing.

Earlier you noted we could "drop the requirement that non-issue linked commits
need an empty [] attached to them", which implies that today [] is required
and therefore valid. Then on the flag you said "let's keep the policy and
remove codifying indecision".

Those point in different directions for one specific case, so to be explicit
about the two readings:

• (a) Keep the policy as this PR implements it. A tag must name at least
  one real reference; bare [] is rejected. This is stricter than the old
  regex, which accepted [] because its inner group could repeat zero times.
• (b) Keep the policy as it stands today. Every subject must carry a
  bracketed tag, and [] is how a non-issue commit satisfies that. Equivalent
  to the old regex's intent, just actually enforced and without the [,,]
  loophole.

I've implemented (a) on the reading that "the policy" meant the one the
flag was gating, and that "we can always relax the rules later" implies the
stricter starting point. Say the word and (b) is a two-line change.

Worth knowing either way: under (a), roughly a third of recent subjects would
not pass. That only affects new commits — the check runs over a pull request's
own range, so existing history is never re-examined.

2. In interactive mode, should the enforced checks still be promptable?

Today the hook treats nearly everything as Okay? [ynq], so a developer can
accept an overlong line and push it. Once CI enforces that same rule, accepting
it locally just moves the failure later.

• Preserve today's behaviour: --interactive makes every finding
  promptable. The hook behaves exactly as it does now.
• Make the hook predictive: prompt only for advisory findings (spelling),
  and fail on the enforced ones, so the hook tells you what CI will reject.

I've gone with preserving today's behaviour, since this PR is meant to change
where things are enforced rather than how the hook feels, but the second option
is arguably more useful now that the checks are binding.

3. Should the wrapper use the ref list on stdin?

"Invokes edg-check-policy with the correct commit range" raises a question
about what correct means here. The current hook ignores the
<local ref> <local sha> <remote ref> <remote sha> lines git feeds it and
instead walks git log from HEAD until it finds a commit already on a remote
branch, which means pushing a branch that isn't the current checkout isn't
inspected properly.

I've kept the existing range computation so this PR doesn't change behaviour,
exposed as a flag on edg-check-policy so the wrapper stays thin. Happy to
make the wrapper parse stdin properly instead, either here or as a follow-up.

4. CONTRIBUTING.md

Agreed it should be general rather than a note about one workflow. I've trimmed
my version back to a short factual statement of what is checked on a pull
request, and I'd gladly take your copy for the surrounding section once the
questions above are settled.

---

Two unrelated things that came out of testing, for the record:

Binary files were silently skipped. edg-check-file-changes decodes aspell
output as UTF-8, so handing it an image raises UnicodeDecodeError. That's
caught per-file and the file is dropped from the results, so the run still
reports success while leaving a traceback in the log. The repo tracks four
binary files outside tests/ (both doc/source/_static/doc-logo-*.png,
dev_tools/services/acknowledg/client.png, and the client favicon.ico), so
updating a logo would hit it. The policy tool now skips blobs containing a NUL
byte or failing UTF-8 decode, and warns when the checker returns no result for
a file it was asked about.

A few pre-existing items I noticed but deliberately haven't touched, since
they change what is checked rather than where:

1. _file_check_config() has elif suffix == 'txt': without the dot, so no
   suffix ever matches and .txt files fall through to the else branch and do
   get line-length checked.
2. The 79-column check does len(line.rstrip('\n').expandtabs()), so a line
   committed with CRLF counts its carriage return and a 79-column line reports
   as 80. No tracked file currently hits this.
3. confirm_okay() records an error instead of prompting when stdout isn't a
   TTY, which makes scripted pushes stricter than interactive ones.

Happy to file any of those as issues if useful.

[wchilders-nvidia]

Quick comment, not a full pass review, noticed the sys.stdin update was removed, we probably need to make sure we're opening /dev/tty for the stdin of the childprocess during process launch.

[sdarwin]

Good catch, and you're right about the fix even though the update survived:
it moved into edg-check-policy rather than being dropped. The restructure
carried it over as a _reopen_terminal_input() helper that ran when
--interactive was passed, so interactive pushes did still prompt.

Opening the terminal at process launch is the better answer regardless, for
two reasons:

1. Rebinding sys.stdin only fixed the Python-level object. File
   descriptor 0 still pointed at git's ref list for the whole child process
   tree. input() worked because it falls back to sys.stdin.readline()
   once sys.stdin isn't the original console object, but anything reading
   descriptor 0 directly would have got the refs. That's a sharp edge to
   leave lying around.
2. It put hook knowledge in the general tool. A function in
   edg-check-policy whose docstring has to explain how git invokes hooks
   is exactly the layering violation the rest of your review is about. A
   developer running edg-check-policy --interactive at a shell already has
   a terminal on stdin and shouldn't have it reopened underneath them.

So the hook now opens /dev/tty and hands it to the child as stdin at
launch, and the tool no longer refers to sys.stdin or platform at all.
--interactive now just means "prompt on stdin when stdin is a terminal",
which reads the same way for both callers.

This also closed a latent crash that predates the restructure. Opening
/dev/tty raises OSError: [Errno 6] No such device or address when there
is no controlling terminal, and that was unhandled in both the original hook
and my version of it, so such a push died with a traceback. The hook now
falls back to an empty stdin, and what happens next is the subject of the
last section below.

Verified by feeding the hook a real ref list on stdin under a pty, the way
git does it. Passing the refs straight through to the child, which is what
the wrapper did before this change, gives:

Failed: 6 policy finding(s) not accepted.

because the first ref line is consumed as an answer and the rest of the
prompts hit EOF — a push blocked despite the developer answering y to
everything. With the terminal passed explicitly, the same run answers all
six prompts and reports Passed: no policy violations. Both typecheck legs
still pass, including mypy==0.971 --python-version 3.6.

Fixing the crash forced a decision about what a terminal-less push should
actually do, so rather than leave it to chance I've made the checker fall
back to the accounting it already uses when nobody is watching — the same
one CI uses. Advisory findings are reported without blocking, enforced ones
still fail, so a push from a GUI client or a script now reaches the same
verdict as the pull request it turns into.

The reasoning, in case you'd have drawn the line elsewhere:

• There was no working behaviour to preserve. With no controlling
  terminal the old hook crashed, and with its output redirected it counted
  an error without trying to prompt. Both refused the push. So this isn't
  overturning a deliberate choice, it's finishing one that was never made.
• Refusing on advisories is the wrong failure. confirm_okay() counted
  an unanswerable prompt as an error, which on a branch like this one means
  a push rejected over spelling suggestions nobody was there to accept —
  and the pull request would then have gone green anyway.
• A pre-push hook should predict CI. Making the degraded path be the
  CI path means there are two behaviours to reason about rather than three,
  and the hook can't disagree with the merge outcome.

The test is sys.stdin.isatty() and sys.stdout.isatty(), which composes
with the change above: the hook opening /dev/tty is what makes stdin a
terminal, and if it couldn't, stdin is empty and isn't one. That let me drop
the separate stdout guard inside confirm_okay(), so this is a small net
simplification, and it incidentally fixes the redirected-output case (item 3
in my earlier list).

Verified across the four cases: a push from a terminal still prompts and
passes; one with no terminal issues no prompts and passes with the spelling
advisories reported; one with no terminal but a real violation still fails
(Failed: 3 policy finding(s) not accepted for overlong lines, with seven
groups of spelling advisories present and correctly not counted); and CI is
untouched at 0 errors and 129 warnings.

Happy to reverse this if you'd rather a terminal-less push be refused
outright — it's a two-line change and the comment says where.