From d2855b97d0b01f83d932adf02470acb9046b3ac1 Mon Sep 17 00:00:00 2001 From: FredApps <153173854+FredApps@users.noreply.github.com> Date: Fri, 6 Nov 2026 12:56:48 +0100 Subject: [PATCH 1/3] fix: preserve local text when updating house rules --- actions/house-rules/house_rules.py | 60 ++++++++++++++++++------------ tests/test_house_rules.py | 49 ++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 23 deletions(-) diff --git a/actions/house-rules/house_rules.py b/actions/house-rules/house_rules.py index a707365..2e5c1b1 100644 --- a/actions/house-rules/house_rules.py +++ b/actions/house-rules/house_rules.py @@ -22,7 +22,7 @@ SOURCES = { "rust": "standards/house-rules-rust.md", } START = re.compile( - r"\n" + r"(?:\r\n|\r|\n)" ) END = "" @@ -40,7 +40,6 @@ def digest(text: str) -> str: def parse_blocks(text: str): """Return (blocks, spans). A span is the half-open range of one block in `text`.""" - text = normalize(text) blocks = [] spans = [] position = 0 @@ -60,8 +59,13 @@ def parse_blocks(text: str): "body": body, } ) - spans.append((match.start(), end_at + len(END))) position = end_at + len(END) + # The marker's line ending belongs to the generated block, not local text. + if text.startswith("\r\n", position): + position += 2 + elif text[position:position + 1] in ("\r", "\n"): + position += 1 + spans.append((match.start(), position)) return blocks, spans @@ -200,24 +204,29 @@ def head_sha(source: str) -> str: def apply_blocks(agents: str, rendered: list[str]) -> str: - text = normalize(agents) if agents else "" - try: - _, spans = parse_blocks(text) if text else ([], []) - except ValueError: - spans = [] - insertion = "".join(rendered) + """Replace only generated spans; keep all surrounding local text verbatim.""" + blocks, spans = parse_blocks(agents) if agents else ([], []) + replacements = {} + for text in rendered: + generated, _ = parse_blocks(text) + replacements[generated[0]["name"]] = text if not spans: - rest = text - if rest and not rest.startswith("\n"): - rest = "\n" + rest - return insertion + rest - start = spans[0][0] - end = spans[-1][1] - # Drop a single trailing newline that belonged to the last rendered block. - suffix = text[end:] - if suffix.startswith("\n"): - suffix = suffix[1:] - return text[:start] + insertion + suffix + separator = "\n" if agents and not agents.startswith(("\n", "\r")) else "" + return "".join(rendered) + separator + agents + names = [block["name"] for block in blocks] + if len(set(names)) != len(names): + raise ValueError("house rules: a block name is repeated") + if any(name not in SOURCES for name in names): + raise ValueError("house rules: refusing to replace an unknown block") + pieces = [] + position = 0 + for block, (start, end) in zip(blocks, spans): + pieces.append(agents[position:start]) + pieces.append(replacements.pop(block["name"], "")) + position = end + pieces.extend(replacements.values()) + pieces.append(agents[position:]) + return "".join(pieces) def agents_file(raw: str) -> Path: @@ -245,8 +254,13 @@ def write_agents(directory: Path, source: Path, require_rust: bool) -> None: rendered.append(render(name, sha, content)) path = agents_file(str(directory)) # The existing file is preserved around the blocks. Its text is not a path. - current = path.read_text(encoding="utf-8") if path.is_file() else "" - path.write_text(apply_blocks(current, rendered), encoding="utf-8") # NOSONAR + current = "" + if path.is_file(): + with path.open(encoding="utf-8", newline="") as handle: + current = handle.read() + updated = apply_blocks(current, rendered) + with path.open("w", encoding="utf-8", newline="") as handle: + handle.write(updated) # NOSONAR def main(argv: list[str] | None = None) -> int: @@ -261,7 +275,7 @@ def main(argv: list[str] | None = None) -> int: if args.mode == "write": try: write_agents(directory, source, args.require_rust) - except RuntimeError as error: + except (RuntimeError, ValueError) as error: print(error) return 2 return 0 diff --git a/tests/test_house_rules.py b/tests/test_house_rules.py index 161b49c..dcb1b2a 100644 --- a/tests/test_house_rules.py +++ b/tests/test_house_rules.py @@ -86,6 +86,55 @@ class HouseRulesTests(unittest.TestCase): again = (self.app / "AGENTS.md").read_text() self.assertEqual(text, again) + def test_local_text_between_blocks_is_kept(self): + core = house_rules.render("core", self.first, "Core one.\n") + rust = house_rules.render("rust", self.first, "Rust one.\n") + local = "\n## Local guard\nNever remove this guard.\n\n" + target = self.app / "AGENTS.md" + target.write_text("Before.\n" + core + local + rust + "After.\n") + self.write() + self.assertEqual(target.read_text(), "Before.\n" + + house_rules.render("core", self.second, "Core two.\n") + local + + house_rules.render("rust", self.second, "Rust two.\n") + "After.\n") + + def test_writer_preserves_outside_bytes_and_is_idempotent(self): + core = house_rules.render("core", self.first, "Core one.\n").replace("\n", "\r\n") + rust = house_rules.render("rust", self.first, "Rust one.\n").replace("\n", "\r\n") + target = self.app / "AGENTS.md" + local = "\r\nLocal guard.\r\n\r\n" + target.write_bytes(("Before.\r\n" + core + local + rust + "After.\r\n\r\n").encode()) + self.write() + expected = ("Before.\r\n" + + house_rules.render("core", self.second, "Core two.\n") + local + + house_rules.render("rust", self.second, "Rust two.\n") + "After.\r\n\r\n").encode() + self.assertEqual(target.read_bytes(), expected) + self.write() + self.assertEqual(target.read_bytes(), expected) + self.assertEqual(self.check(), ([], [])) + + def test_initial_write_keeps_local_line_endings_and_no_final_newline(self): + target = self.app / "AGENTS.md" + original = b"# App\r\nLocal rule without final newline." + target.write_bytes(original) + self.write() + self.assertTrue(target.read_bytes().endswith(original)) + + def test_removing_rust_does_not_remove_intervening_local_rule(self): + target = self.app / "AGENTS.md" + target.write_text(house_rules.render("core", self.first, "Core one.\n") + + "Local rule.\n" + house_rules.render("rust", self.first, "Rust one.\n")) + self.write(require_rust=False) + self.assertEqual(target.read_text(), + house_rules.render("core", self.second, "Core two.\n") + "Local rule.\n") + + def test_unclosed_block_is_not_overwritten(self): + target = self.app / "AGENTS.md" + original = house_rules.render("core", self.first, "Core one.\n").replace(house_rules.END, "Local guard") + target.write_text(original) + with self.assertRaises(ValueError): + self.write() + self.assertEqual(target.read_text(), original) + def test_an_edited_block_fails_even_when_its_digest_is_rewritten(self): self.write() text = (self.app / "AGENTS.md").read_text() From be28458a304d4023fc8cec4e8bc37c179907f048 Mon Sep 17 00:00:00 2001 From: FredApps Date: Fri, 6 Nov 2026 19:16:25 +0100 Subject: [PATCH 2/3] Harden house-rules markers and main ancestry checks --- .github/workflows/house-rules.yml | 2 +- README.md | 5 +- actions/house-rules/action.yml | 2 +- actions/house-rules/house_rules.py | 73 ++++++++++-------- tests/test_house_rules.py | 114 ++++++++++++++++++++++++++++- 5 files changed, 160 insertions(+), 36 deletions(-) diff --git a/.github/workflows/house-rules.yml b/.github/workflows/house-rules.yml index 667f275..c80a9fe 100644 --- a/.github/workflows/house-rules.yml +++ b/.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 diff --git a/README.md b/README.md index 7d2a810..498b0a9 100644 --- a/README.md +++ b/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 diff --git a/actions/house-rules/action.yml b/actions/house-rules/action.yml index 1da65e8..9ccf6aa 100644 --- a/actions/house-rules/action.yml +++ b/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: diff --git a/actions/house-rules/house_rules.py b/actions/house-rules/house_rules.py index 2e5c1b1..af430f8 100644 --- a/actions/house-rules/house_rules.py +++ b/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"(?:\r\n|\r|\n)" ) END = "" +MARKER = re.compile(r"")): + 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) From 13ad478f3b74a05f670efae4f9db8f481ec31d3e Mon Sep 17 00:00:00 2001 From: FredApps Date: Fri, 13 Nov 2026 21:18:10 +0100 Subject: [PATCH 3/3] fix(house-rules): preserve BOM and fenced marker examples --- actions/house-rules/house_rules.py | 32 ++++++++++++++--- tests/test_house_rules.py | 58 +++++++++++++++++++++--------- 2 files changed, 70 insertions(+), 20 deletions(-) diff --git a/actions/house-rules/house_rules.py b/actions/house-rules/house_rules.py index af430f8..6d6d123 100644 --- a/actions/house-rules/house_rules.py +++ b/actions/house-rules/house_rules.py @@ -39,19 +39,41 @@ def digest(text: str) -> str: return hashlib.sha256(normalize(text).encode("utf-8")).hexdigest() +def without_fenced_examples(text: str) -> str: + """Mask Markdown fences without changing offsets used for byte-preserving writes.""" + result = [] + fence = None + for line in text.splitlines(keepends=True): + if fence is None: + opening = re.match(r" {0,3}(`{3,}|~{3,})([^\r\n]*)", line) + if opening and not (opening[1][0] == "`" and "`" in opening[2]): + fence = opening[1] + else: + result.append(line) + continue + else: + closing = re.fullmatch(r" {0,3}" + re.escape(fence[0]) + + r"{" + str(len(fence)) + r",}[ \t]*(?:\r\n|\r|\n)?", line) + if closing: + fence = None + result.append("".join(c if c in "\r\n" else " " for c in line)) + return "".join(result) + + def parse_blocks(text: str): """Return (blocks, spans). A span is the half-open range of one block in `text`.""" + markers = without_fenced_examples(text) blocks = [] spans = [] position = 0 while True: - marker = MARKER.search(text, position) + marker = MARKER.search(markers, position) if marker is None: break 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()) + closing = MARKER.search(markers, match.end()) if closing is None: raise ValueError(f"house rules: {match.group(1)} block is not closed") end_at = closing.start() @@ -224,8 +246,10 @@ def apply_blocks(agents: str, rendered: list[str]) -> str: generated, _ = parse_blocks(text) replacements[generated[0]["name"]] = text if not spans: - separator = "\n" if agents and not agents.startswith(("\n", "\r")) else "" - return "".join(rendered) + separator + agents + bom = "\ufeff" if agents.startswith("\ufeff") else "" + local = agents[len(bom):] + separator = "\n" if local and not local.startswith(("\n", "\r")) else "" + return bom + "".join(rendered) + separator + local names = [block["name"] for block in blocks] if len(set(names)) != len(names): raise ValueError("house rules: a block name is repeated") diff --git a/tests/test_house_rules.py b/tests/test_house_rules.py index 9bcd0b7..f453d1f 100644 --- a/tests/test_house_rules.py +++ b/tests/test_house_rules.py @@ -61,9 +61,35 @@ class HouseRulesTests(unittest.TestCase): house_rules.write_agents(self.app, self.source, require_rust) def check(self, require_rust: bool = True): - text = (self.app / "AGENTS.md").read_text() + text = (self.app / "AGENTS.md").read_text(encoding="utf-8") return house_rules.check_file(text, self.source, require_rust) + def test_initial_write_keeps_bom_at_start(self): + target = self.app / "AGENTS.md" + target.write_bytes("\ufeff# Local\r\nKeep me.".encode("utf-8")) + self.write() + output = target.read_bytes() + self.assertTrue(output.startswith("\ufeff\n", 1)[1].split("", 1)[0] text = text.replace(house_rules.digest("Core two.\n"), house_rules.digest(body), 1) @@ -148,7 +174,7 @@ class HouseRulesTests(unittest.TestCase): def test_a_digest_that_does_not_match_the_bytes_fails(self): self.write() - text = (self.app / "AGENTS.md").read_text() + text = (self.app / "AGENTS.md").read_text(encoding="utf-8") text = text.replace(house_rules.digest("Core two.\n"), "0" * 64, 1) (self.app / "AGENTS.md").write_text(text) failures, _ = self.check() @@ -157,7 +183,7 @@ class HouseRulesTests(unittest.TestCase): def test_an_intact_older_commit_warns(self): house_rules.write_agents(self.app, self.source, True) # Move the recorded revision back to the first commit without changing the checker's view of HEAD. - text = (self.app / "AGENTS.md").read_text() + text = (self.app / "AGENTS.md").read_text(encoding="utf-8") old_core = house_rules.render("core", self.first, "Core one.\n") old_rust = house_rules.render("rust", self.first, "Rust one.\n") (self.app / "AGENTS.md").write_text(old_core + old_rust) @@ -190,7 +216,7 @@ class HouseRulesTests(unittest.TestCase): def test_a_missing_revision_could_not_be_checked(self): self.write() - text = (self.app / "AGENTS.md").read_text().replace(self.second, "a" * 40) + text = (self.app / "AGENTS.md").read_text(encoding="utf-8").replace(self.second, "a" * 40) (self.app / "AGENTS.md").write_text(text) failures, _ = self.check() self.assertTrue(any(item.startswith("house rules: could not check") for item in failures)) @@ -249,7 +275,7 @@ class HouseRulesTests(unittest.TestCase): git(self.source, "checkout", "-b", "side") self.commit_standards("Side core.\n", "Side rust.\n") self.write() - text = (self.app / "AGENTS.md").read_text() + text = (self.app / "AGENTS.md").read_text(encoding="utf-8") self.assertIn(self.second, text) self.assertIn("Core two.", text) self.assertNotIn("Side core.", text) @@ -258,7 +284,7 @@ class HouseRulesTests(unittest.TestCase): 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() + text = (self.app / "AGENTS.md").read_text(encoding="utf-8") self.assertIn(self.first, text) self.assertIn("Core one.", text) self.assertEqual(self.check(), ([], [])) @@ -277,7 +303,7 @@ class HouseRulesTests(unittest.TestCase): 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.assertIn(self.second, (self.app / "AGENTS.md").read_text(encoding="utf-8")) self.assertEqual(self.check(), ([], [])) def test_ancestry_errors_are_not_reported_as_non_ancestry(self): @@ -312,7 +338,7 @@ class HouseRulesTests(unittest.TestCase): 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) + action = yaml.load((ROOT / "actions/house-rules/action.yml").read_text(encoding="utf-8"), Loader=yaml.BaseLoader) step = action["runs"]["steps"][0] self.assertNotIn("${{", step["run"]) self.assertEqual(step["env"]["WORKSPACE_DIR"], "${{ inputs.working-directory }}") @@ -321,7 +347,7 @@ class HouseRulesWiringTests(unittest.TestCase): self.assertEqual(action["inputs"]["require-rust"]["default"], "true") def test_the_workflow_checks_out_craft_contrib_and_runs_the_action(self): - workflow = yaml.load((ROOT / ".github/workflows/house-rules.yml").read_text(), Loader=yaml.BaseLoader) + workflow = yaml.load((ROOT / ".github/workflows/house-rules.yml").read_text(encoding="utf-8"), Loader=yaml.BaseLoader) job = workflow["jobs"]["house-rules"] self.assertEqual(workflow["permissions"], {"contents": "read"}) self.assertIn("timeout-minutes", job)