Harden house-rules markers and main ancestry checks

.github/workflows/house-rules.yml

@@ -1,7 +1,7 @@
 name: House rules
 
 # Fails unless AGENTS.md copies the shared rules from craft-contrib unchanged, at a commit that is
-# on main. An intact older copy warns and does not fail. Callers run it beside their gate:
+# on main. An intact older copy warns only when its normalized standards file changed on main, and does not fail. Callers run it beside their gate:
 #
 #   house-rules:
 #     uses: storytold/craft-common-actions/.github/workflows/house-rules.yml@main

README.md

@@ -662,7 +662,8 @@ Fails unless `AGENTS.md` carries the shared house rules from
 the commit it was copied from and the SHA-256 of the bytes inside it. The check loads
 `standards/house-rules-core.md` (and `standards/house-rules-rust.md` when `rust` is true) at that
 commit, requires the commit to be on `main`, and compares the normalized bytes. An edited block, or
-a commit that is not on `main`, fails. An intact older commit warns. A source that cannot be read
+a commit that is not on `main`, fails. An intact older commit warns only when its normalized standards file changed on `main`.
+If the file is unchanged, there is no warning. A source that cannot be read
 fails as "could not check". `rust` defaults to true; a repository that is not Rust passes `false`.
 
 The caller is only the workflow call. The reusable workflow checks the app out and checks
@@ -703,7 +704,7 @@ Individual actions are also available for composing custom pipelines:
 - `actions/setup-windows-signing`: Exports the shared Authenticode signing script (`CRAFT_SIGN_SCRIPT`) for the app's packaging step (see "Windows code signing").
 - `actions/ast-grep`: Installs a pinned ast-grep, runs the caller's rule tests and scans with its `sgconfig.yml` (see `ast-grep.yml`).
 - `actions/check-git-sources`: The check `git-sources.yml` runs, for a job that has already checked out the app; `allowed-prefix` changes where git dependencies may come from (default `git+https://github.com/storytold/`).
-- `actions/house-rules`: The check `house-rules.yml` runs. `source` is a checkout of craft-contrib with `main` history; `require-rust` asks for the Rust block as well as the core block.
+- `actions/house-rules`: The check `house-rules.yml` runs. `source` is a checkout of craft-contrib with `main` history (refresh `origin/main` before checking; the local `main` ref is used only when `origin/main` is absent, never `HEAD`); `require-rust` asks for the Rust block as well as the core block.
 
 ## Storytold Organization Hosting
 

actions/house-rules/action.yml

@@ -1,7 +1,7 @@
 name: Check house rules
 description: >-
   Fail unless the AGENTS.md house-rules blocks match craft-contrib at the commit they name,
-  and that commit is on main. An intact older commit warns.
+  and that commit is on main. An intact older commit warns only when the normalized standards file changed on main.
 
 inputs:
   working-directory:

actions/house-rules/house_rules.py

@@ -4,7 +4,7 @@
 The bytes inside a block are the normalized standards file. The recorded sha256
 must match those bytes, and those bytes must match the file at the recorded
 commit on craft-contrib main. A digest rewritten to suit an edit does not pass.
-An intact older commit warns. A missing source is "could not check".
+An intact older commit warns only when the normalized standards file changed on main. A missing source is "could not check".
 """
 
 from __future__ import annotations
@@ -25,6 +25,7 @@ START = re.compile(
     r"<!-- craft-house-rules:start ([a-z]+) source=([0-9a-f]{40}) sha256=([0-9a-f]{64}) -->(?:\r\n|\r|\n)"
 )
 END = "<!-- craft-house-rules:end -->"
+MARKER = re.compile(r"<!--\s*craft-house-rules:")
 
 
 def normalize(text: str) -> str:
@@ -44,12 +45,18 @@ def parse_blocks(text: str):
     spans = []
     position = 0
     while True:
-        match = START.search(text, position)
-        if match is None:
+        marker = MARKER.search(text, position)
+        if marker is None:
             break
-        end_at = text.find(END, match.end())
-        if end_at < 0:
+        match = START.match(text, marker.start())
+        if match is None:
+            raise ValueError("house rules: malformed or unexpected block marker")
+        closing = MARKER.search(text, match.end())
+        if closing is None:
             raise ValueError(f"house rules: {match.group(1)} block is not closed")
+        end_at = closing.start()
+        if not text.startswith(END, end_at):
+            raise ValueError("house rules: nested or malformed block marker")
         body = text[match.end() : end_at]
         blocks.append(
             {
@@ -94,11 +101,24 @@ def commit_sha(raw: str) -> str | None:
 
 
 def git(source: str, args: list[str]) -> subprocess.CompletedProcess[str]:
-    return subprocess.run(
-        ["git", "-C", source, *args],
-        capture_output=True,
-        text=True,
-    )
+    command = ["git", "-C", source, *args]
+    try:
+        return subprocess.run(command, capture_output=True, text=True, encoding="utf-8")
+    except (OSError, UnicodeError) as error:
+        return subprocess.CompletedProcess(command, 128, "", str(error))
+
+
+def main_sha(source: str) -> str | None:
+    """Prefer fetched origin/main, falling back to local main; never trust HEAD."""
+    for ref in ("refs/remotes/origin/main", "refs/heads/main"):
+        exists = git(source, ["show-ref", "--verify", "--quiet", ref])
+        if exists.returncode == 1:
+            continue
+        if exists.returncode != 0:
+            return None
+        result = git(source, ["rev-parse", "--verify", f"{ref}^{{commit}}"])
+        return commit_sha(result.stdout) if result.returncode == 0 else None
+    return None
 
 
 def revision_state(source: str, sha: str) -> str:
@@ -106,28 +126,22 @@ def revision_state(source: str, sha: str) -> str:
     recorded = commit_sha(sha)
     if recorded is None:
         return "missing"
-    head = git(source, ["rev-parse", "HEAD"])
-    if head.returncode != 0:
-        return "missing"
-    head_sha_value = commit_sha(head.stdout)
-    if head_sha_value is None:
+    main = main_sha(source)
+    if main is None:
         return "missing"
     if git(source, ["cat-file", "-e", f"{recorded}^{{commit}}"]).returncode != 0:
         return "missing"
-    if head_sha_value == recorded:
+    if main == recorded:
         return "identical"
-    if git(source, ["merge-base", "--is-ancestor", recorded, "HEAD"]).returncode == 0:
+    ancestry = git(source, ["merge-base", "--is-ancestor", recorded, main])
+    if ancestry.returncode == 0:
         return "older"
-    return "not-on-main"
+    return "not-on-main" if ancestry.returncode == 1 else "missing"
 
 
-def file_at_head(source: str, path: str) -> str | None:
-    if path not in SOURCES.values():
-        return None
-    shown = git(source, ["show", f"HEAD:{path}"])
-    if shown.returncode != 0:
-        return None
-    return shown.stdout
+def file_at_main(source: str, path: str) -> str | None:
+    main = main_sha(source)
+    return file_at(source, main, path) if main is not None else None
 
 
 def file_at(source: str, sha: str, path: str) -> str | None:
@@ -160,7 +174,7 @@ def judge_block(source: str, block: dict) -> tuple[str | None, str | None]:
         return f"house rules: {block['name']} block does not match {path} at {block['source']}", None
     if state != "older":
         return None, None
-    current = file_at_head(source, path)
+    current = file_at_main(source, path)
     if current is None:
         return f"house rules: could not check {block['name']} at main", None
     if normalize(current) != body:
@@ -195,9 +209,8 @@ def check_file(agents: str, source: Path, require_rust: bool) -> tuple[list[str]
     return failures, warnings
 
 
-def head_sha(source: str) -> str:
-    result = git(source, ["rev-parse", "HEAD"])
-    recorded = commit_sha(result.stdout) if result.returncode == 0 else None
+def source_sha(source: str) -> str:
+    recorded = main_sha(source)
     if recorded is None:
         raise RuntimeError("house rules: could not read the source revision")
     return recorded
@@ -244,7 +257,7 @@ def write_agents(directory: Path, source: Path, require_rust: bool) -> None:
     checkout = checkout_path(str(source))
     if checkout is None:
         raise RuntimeError("house rules: could not read the craft-contrib checkout")
-    sha = head_sha(checkout)
+    sha = source_sha(checkout)
     names = ["core", "rust"] if require_rust else ["core"]
     rendered = []
     for name in names:

tests/test_house_rules.py

@@ -6,6 +6,7 @@ import os
 import subprocess
 import tempfile
 import unittest
+from unittest import mock
 
 import yaml
 
@@ -179,8 +180,10 @@ class HouseRulesTests(unittest.TestCase):
         git(self.source, "checkout", "--orphan", "side")
         self.commit_standards("Side core.\n", "Side rust.\n")
         side = git(self.source, "rev-parse", "HEAD")
-        house_rules.write_agents(self.app, self.source, True)
-        git(self.source, "checkout", "main")
+        (self.app / "AGENTS.md").write_text(
+            house_rules.render("core", side, "Side core.\n") +
+            house_rules.render("rust", side, "Side rust.\n"))
+        # Even with HEAD still on the unmerged side branch, it must not pass.
         failures, _ = self.check()
         self.assertTrue(any("is not on main" in item for item in failures), failures)
         self.assertIn(side, " ".join(failures))
@@ -200,6 +203,113 @@ class HouseRulesTests(unittest.TestCase):
         self.assertIn("house rules: the rust block is missing", failures)
 
 
+    def test_malformed_optional_rust_marker_fails_without_writing(self):
+        core = house_rules.render("core", self.second, "Core two.\n")
+        rust = house_rules.render("rust", self.second, "Rust two.\n")
+        for broken in (rust.replace("sha256=", "sha256=bad"),
+                       rust.replace("source=", "source=bad"),
+                       rust.replace("start rust", "start Rust"),
+                       rust.replace("<!-- craft", "<!--craft", 1)):
+            with self.subTest(broken=broken):
+                target = self.app / "AGENTS.md"
+                original = (core + broken).encode()
+                target.write_bytes(original)
+                failures, _ = self.check(require_rust=False)
+                self.assertTrue(any("marker" in item for item in failures), failures)
+                with self.assertRaises(ValueError):
+                    self.write(require_rust=False)
+                self.assertEqual(target.read_bytes(), original)
+
+    def test_nested_or_orphan_markers_fail_without_writing(self):
+        core = house_rules.render("core", self.second, "Core two.\n")
+        rust = house_rules.render("rust", self.second, "Rust two.\n")
+        for text in (house_rules.END + "\n" + core, core + house_rules.END,
+                     core.replace(house_rules.END, rust + house_rules.END),
+                     core.replace(house_rules.END, "<!-- craft-house-rules:end bad -->")):
+            with self.subTest(text=text):
+                target = self.app / "AGENTS.md"
+                target.write_bytes(text.encode())
+                self.assertTrue(self.check(False)[0])
+                with self.assertRaises(ValueError):
+                    self.write(False)
+                self.assertEqual(target.read_bytes(), text.encode())
+
+    def test_duplicate_and_unknown_blocks_fail_without_writing(self):
+        core = house_rules.render("core", self.second, "Core two.\n")
+        for text in (core + core, core + house_rules.render("other", self.second, "Other.\n")):
+            with self.subTest(text=text):
+                target = self.app / "AGENTS.md"
+                target.write_bytes(text.encode())
+                self.assertTrue(self.check(False)[0])
+                with self.assertRaises(ValueError):
+                    self.write(False)
+                self.assertEqual(target.read_bytes(), text.encode())
+
+    def test_writer_uses_main_even_with_unmerged_head(self):
+        git(self.source, "checkout", "-b", "side")
+        self.commit_standards("Side core.\n", "Side rust.\n")
+        self.write()
+        text = (self.app / "AGENTS.md").read_text()
+        self.assertIn(self.second, text)
+        self.assertIn("Core two.", text)
+        self.assertNotIn("Side core.", text)
+        self.assertEqual(self.check(), ([], []))
+
+    def test_origin_main_takes_priority_over_local_main(self):
+        git(self.source, "update-ref", "refs/remotes/origin/main", self.first)
+        self.write()
+        text = (self.app / "AGENTS.md").read_text()
+        self.assertIn(self.first, text)
+        self.assertIn("Core one.", text)
+        self.assertEqual(self.check(), ([], []))
+        self.assertEqual(house_rules.revision_state(str(self.source), self.second), "not-on-main")
+
+    def test_missing_main_fails_closed_even_with_valid_head(self):
+        self.write()
+        target = self.app / "AGENTS.md"
+        original = target.read_bytes()
+        git(self.source, "branch", "-m", "side")
+        self.assertTrue(any("could not check" in item for item in self.check()[0]))
+        with self.assertRaises(RuntimeError):
+            self.write()
+        self.assertEqual(target.read_bytes(), original)
+
+    def test_detached_head_uses_named_main(self):
+        git(self.source, "checkout", "--detach", self.first)
+        self.write()
+        self.assertIn(self.second, (self.app / "AGENTS.md").read_text())
+        self.assertEqual(self.check(), ([], []))
+
+    def test_ancestry_errors_are_not_reported_as_non_ancestry(self):
+        real_git = house_rules.git
+        def git_error(source, args):
+            if args[0] == "merge-base":
+                return subprocess.CompletedProcess(args, 128, "", "missing intermediate commit")
+            return real_git(source, args)
+        with mock.patch.object(house_rules, "git", side_effect=git_error):
+            self.assertEqual(house_rules.revision_state(str(self.source), self.first), "missing")
+            failures, _ = house_rules.check_file(
+                house_rules.render("core", self.first, "Core one.\n"), self.source, False)
+            self.assertTrue(any("could not check" in item for item in failures))
+
+    def test_git_uses_explicit_utf8_and_reports_invalid_output(self):
+        with mock.patch.object(house_rules.subprocess, "run") as run:
+            house_rules.git(str(self.source), ["status"])
+            self.assertEqual(run.call_args.kwargs["encoding"], "utf-8")
+        error = UnicodeDecodeError("utf-8", b"\xff", 0, 1, "invalid")
+        with mock.patch.object(house_rules.subprocess, "run", side_effect=error):
+            self.assertNotEqual(house_rules.git(str(self.source), ["status"]).returncode, 0)
+
+    def test_non_ascii_standards_round_trip(self):
+        standards = self.source / "standards"
+        (standards / "house-rules-core.md").write_bytes("Core café.\n".encode("utf-8"))
+        git(self.source, "add", "standards")
+        git(self.source, "commit", "-m", "Unicode standards")
+        self.write()
+        self.assertIn("café".encode("utf-8"), (self.app / "AGENTS.md").read_bytes())
+        self.assertEqual(self.check(), ([], []))
+
+
 class HouseRulesWiringTests(unittest.TestCase):
     def test_the_action_passes_its_inputs_through_the_environment(self):
         action = yaml.load((ROOT / "actions/house-rules/action.yml").read_text(), Loader=yaml.BaseLoader)