From 0128a51c8077f329d324c526da2f9ecafcaba1fd Mon Sep 17 00:00:00 2001 From: Barnabas Jovanovics Date: Mon, 9 Mar 2026 10:15:32 +0100 Subject: [PATCH] fix: --check false positive after disk round-trip due to eof_newline mismatch Rewrite.Source.write/2 applies eof_newline/1 (String.trim_trailing + "\n") before File.write/2, but the generated content may not end with "\n". On subsequent syncs, the 1-byte trailing newline difference causes --check to report changes when nothing actually changed. Fix by comparing managed sections (trimmed) for SKILL.md files and trimmed content for reference files, so trailing whitespace differences from the disk round-trip are ignored. --- .gitignore | 3 + lib/mix/tasks/usage_rules.sync.ex | 63 ++++++-- test/mix/tasks/usage_rules.sync_test.exs | 185 +++++++++++++++++++++++ 3 files changed, 242 insertions(+), 9 deletions(-) diff --git a/.gitignore b/.gitignore index 03954e0..671845f 100644 --- a/.gitignore +++ b/.gitignore @@ -25,3 +25,6 @@ usage_rules-*.tar # Temporary files, for example, from tests. /tmp/ + +# Scratch test file +test/scratch_test.exs diff --git a/lib/mix/tasks/usage_rules.sync.ex b/lib/mix/tasks/usage_rules.sync.ex index 4f1e312..34ccd29 100644 --- a/lib/mix/tasks/usage_rules.sync.ex +++ b/lib/mix/tasks/usage_rules.sync.ex @@ -853,7 +853,7 @@ if Code.ensure_loaded?(Igniter) do acc, ref_path, content, - fn source -> Rewrite.Source.update(source, :content, content) end + fn source -> update_source_content(source, content) end ) end @@ -868,7 +868,7 @@ if Code.ensure_loaded?(Igniter) do inner_acc, ref_path, content, - fn source -> Rewrite.Source.update(source, :content, content) end + fn source -> update_source_content(source, content) end ) end) end) @@ -1038,7 +1038,7 @@ if Code.ensure_loaded?(Igniter) do acc2, dst_path, content, - fn source -> Rewrite.Source.update(source, :content, content) end + fn source -> update_source_content(source, content) end ) end) end) @@ -1346,20 +1346,50 @@ if Code.ensure_loaded?(Igniter) do if String.contains?(current_content, "") do custom = extract_skill_custom_content(current_content) - if custom != "" do - [new_frontmatter, new_managed] = - String.split(new_skill_md, "\n\n", parts: 2) + new_content = + if custom != "" do + [frontmatter, managed_body] = + String.split(new_skill_md, "\n\n", parts: 2) - new_frontmatter <> - "\n\n" <> custom <> "\n\n" <> new_managed + frontmatter <> + "\n\n" <> custom <> "\n\n" <> managed_body + else + new_skill_md + end + + # Only return new content if the managed section actually changed. + # Rewrite.Source.write/2 applies eof_newline/1 which appends "\n" + # to disk content (see rewrite/lib/rewrite/source.ex, write/3 and + # eof_newline/1). The generated content may not end with "\n", so + # subsequent reads see a trailing-whitespace diff that isn't a real + # change. Comparing just the managed section avoids this. + if managed_section_changed?(current_content, new_content) do + new_content else - new_skill_md + current_content end else new_skill_md end end + defp managed_section_changed?(current, new) do + extract_managed_section(current) != extract_managed_section(new) + end + + defp extract_managed_section(content) do + case String.split(content, "", parts: 2) do + [_, rest] -> + case String.split(rest, "", parts: 2) do + [managed, _] -> String.trim(managed) + _ -> String.trim(rest) + end + + _ -> + nil + end + end + defp strip_managed_skill_content(content) do # Remove managed-by metadata from frontmatter content = String.replace(content, ~r/metadata:\n\s+managed-by: usage-rules\n/, "") @@ -1385,6 +1415,21 @@ if Code.ensure_loaded?(Igniter) do String.replace(content, ~r/\A\s*\s*\n*/s, "") end + # Updates a reference file's content, ignoring trailing whitespace differences. + # Reference files have no managed-section markers — the entire file is synced + # content. Rewrite.Source.write/2 applies eof_newline/1 (see + # rewrite/lib/rewrite/source.ex) which appends "\n" when writing to disk, + # but dep content may not end with one, causing false diffs on subsequent syncs. + defp update_source_content(source, new_content) do + current = Rewrite.Source.get(source, :content) + + if String.trim_trailing(current) == String.trim_trailing(new_content) do + source + else + Rewrite.Source.update(source, :content, new_content) + end + end + defp format_yaml_string(str) do str = String.trim(str) diff --git a/test/mix/tasks/usage_rules.sync_test.exs b/test/mix/tasks/usage_rules.sync_test.exs index d670dcf..bf66cb8 100644 --- a/test/mix/tasks/usage_rules.sync_test.exs +++ b/test/mix/tasks/usage_rules.sync_test.exs @@ -1455,4 +1455,189 @@ defmodule Mix.Tasks.UsageRules.SyncTest do |> assert_creates(".claude/skills/foo-built/SKILL.md") end end + + describe "idempotency (--check)" do + # Rewrite.Source.write/2 calls eof_newline/1 before File.write/2 + # (see rewrite/lib/rewrite/source.ex line ~297 and ~970): + # + # defp write(%Source{path: path, content: content}, ...) do + # file_write(path, eof_newline(content)) + # end + # defp eof_newline(string), do: String.trim_trailing(string) <> "\n" + # + # In test mode, simulate_write stores raw content without this + # normalization, so tests don't reproduce the trailing-newline mismatch + # that causes --check failures in real usage. This helper applies the + # same eof_newline transform to simulate a real disk round-trip. + defp simulate_disk_roundtrip(igniter) do + test_files = + Enum.reduce(igniter.assigns[:test_files], igniter.assigns[:test_files], fn + {"deps/" <> _, _content}, acc -> + acc + + {_path, ""}, acc -> + acc + + {path, content}, acc -> + Map.put(acc, path, String.trim_trailing(content) <> "\n") + end) + + igniter + |> Map.put(:rewrite, Rewrite.new()) + |> Map.put(:assigns, %{ + test_mode?: true, + test_files: test_files, + igniter_exs: igniter.assigns[:igniter_exs] + }) + |> Igniter.include_glob("**/*.*") + end + + test "second sync reports no changes for skills.build" do + config = [ + skills: [ + location: ".claude/skills", + build: [ + "use-foo": [ + description: "Foo skill", + usage_rules: [:foo, :bar] + ] + ] + ] + ] + + igniter = + project_with_deps(%{ + "deps/foo/usage-rules.md" => "# Foo Rules\n\nUse foo wisely.", + "deps/bar/usage-rules.md" => "# Bar Rules\n\nUse bar wisely." + }) + |> sync(config) + |> assert_creates(".claude/skills/use-foo/SKILL.md") + |> assert_creates(".claude/skills/use-foo/references/foo.md") + |> assert_creates(".claude/skills/use-foo/references/bar.md") + |> apply_igniter!() + |> simulate_disk_roundtrip() + + igniter + |> sync(config) + |> assert_unchanged() + end + + test "second sync reports no changes for skills.deps" do + config = [ + skills: [ + location: ".claude/skills", + deps: [:foo] + ] + ] + + igniter = + project_with_deps(%{ + "deps/foo/usage-rules.md" => "# Foo Rules\n\nUse foo wisely." + }) + |> sync(config) + |> assert_creates(".claude/skills/use-foo/SKILL.md") + |> apply_igniter!() + |> simulate_disk_roundtrip() + + igniter + |> sync(config) + |> assert_unchanged() + end + + test "second sync reports no changes for AGENTS.md" do + config = [ + file: "AGENTS.md", + usage_rules: [:foo] + ] + + igniter = + project_with_deps(%{ + "deps/foo/usage-rules.md" => "# Foo Rules\n\nUse foo wisely." + }) + |> sync(config) + |> assert_creates("AGENTS.md") + |> apply_igniter!() + |> simulate_disk_roundtrip() + + igniter + |> sync(config) + |> assert_unchanged() + end + + test "second sync reports no changes with sub-rules" do + config = [ + skills: [ + location: ".claude/skills", + build: [ + "use-foo": [usage_rules: [:foo]] + ] + ] + ] + + igniter = + project_with_deps(%{ + "deps/foo/usage-rules.md" => "# Foo Rules", + "deps/foo/usage-rules/testing.md" => "# Testing Guide" + }) + |> sync(config) + |> assert_creates(".claude/skills/use-foo/SKILL.md") + |> assert_creates(".claude/skills/use-foo/references/foo.md") + |> assert_creates(".claude/skills/use-foo/references/testing.md") + |> apply_igniter!() + |> simulate_disk_roundtrip() + + igniter + |> sync(config) + |> assert_unchanged() + end + + test "second sync reports no changes with custom content in SKILL.md" do + config = [ + skills: [ + location: ".claude/skills", + build: [ + "use-foo": [ + description: "Foo skill", + usage_rules: [:foo] + ] + ] + ] + ] + + # First sync creates the skill + igniter = + project_with_deps(%{ + "deps/foo/usage-rules.md" => "# Foo Rules\n\nUse foo wisely." + }) + |> sync(config) + |> assert_creates(".claude/skills/use-foo/SKILL.md") + |> assert_creates(".claude/skills/use-foo/references/foo.md") + |> apply_igniter!() + + # Inject custom content between frontmatter and managed section + skill_content = igniter.assigns[:test_files][".claude/skills/use-foo/SKILL.md"] + + [frontmatter, managed] = + String.split(skill_content, "\n\n", parts: 2) + + custom_skill = + frontmatter <> + "\n\nMy custom instructions go here.\n\n" <> + managed + + test_files = + Map.put(igniter.assigns[:test_files], ".claude/skills/use-foo/SKILL.md", custom_skill) + + igniter = put_in(igniter.assigns[:test_files], test_files) + + igniter = + igniter + |> simulate_disk_roundtrip() + + # Second sync should preserve custom content and report no changes + igniter + |> sync(config) + |> assert_unchanged() + end + end end