From 3f50167e7b34b5696886b862f5cb97b95644178a Mon Sep 17 00:00:00 2001 From: Frank Lin Date: Wed, 16 Sep 2026 21:10:31 +1000 Subject: [PATCH 01/16] Preserve line endings and trailing newline in Helm values files ReplaceNodeContent rebuilt the file with StringBuilder.AppendLine, which emits Environment.NewLine, so a file whose convention differed from the agent's had every line rewritten and a one-line tag change became a whole-file diff. It now appends the line ending detected from the file itself. The trailing-newline check compared against Environment.NewLine too. On a Windows worker an LF file therefore looked as though it had no trailing newline and TrimEnd removed it, which together with the rewrite matches the reported symptom of CRLF endings and no trailing newline. It now tests for an actual line break. That second fix cannot be covered on Linux, where Environment.NewLine is already "\n" and the old comparison happens to work. Co-Authored-By: Claude Opus 5 (1M context) --- .../ArgoCD/Helm/HelmYamlParserTests.cs | 60 +++++++++++++++++++ source/Calamari/ArgoCD/Helm/HelmYamlParser.cs | 8 ++- 2 files changed, 65 insertions(+), 3 deletions(-) diff --git a/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs b/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs index fff879ad7d..88f110e0b6 100644 --- a/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs +++ b/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs @@ -143,6 +143,66 @@ public void UpdateNodeValue_RespectsTrailingWhitespaceFromInput() result.ReplaceLineEndings().Should().Be(expectedUpdate.ReplaceLineEndings()); } + [Test] + public void UpdateNodeValue_WithCrlfLineEndings_PreservesCrlfOnEveryLine() + { + const string yamlContent = "root:\r\n node1: 42\r\n node2: stable\r\n"; + + var sut = new HelmYamlParser(yamlContent); + + var result = sut.UpdateContentForPath("root.node1", "69"); + + result.Should().Be("root:\r\n node1: 69\r\n node2: stable\r\n"); + } + + [Test] + public void UpdateNodeValue_WithLfLineEndings_PreservesLfOnEveryLine() + { + const string yamlContent = "root:\n node1: 42\n node2: stable\n"; + + var sut = new HelmYamlParser(yamlContent); + + var result = sut.UpdateContentForPath("root.node1", "69"); + + result.Should().Be("root:\n node1: 69\n node2: stable\n"); + } + + [Test] + public void UpdateNodeValue_WithNoTrailingNewline_DoesNotAddOne() + { + const string yamlContent = "root:\n node1: 42"; + + var sut = new HelmYamlParser(yamlContent); + + var result = sut.UpdateContentForPath("root.node1", "69"); + + result.Should().Be("root:\n node1: 69"); + } + + [Test] + public void UpdateNodeValue_WithCrlfAndNoTrailingNewline_PreservesBoth() + { + const string yamlContent = "root:\r\n node1: 42\r\n node2: \"latest\""; + + var sut = new HelmYamlParser(yamlContent); + + var result = sut.UpdateContentForPath("root.node2", "stable"); + + result.Should().Be("root:\r\n node1: 42\r\n node2: \"stable\""); + } + + [Test] + public void UpdateNodeValue_WithUnchangedPath_ReturnsContentByteForByte() + { + const string yamlContent = "root:\r\n node1: 42\r\n"; + + var sut = new HelmYamlParser(yamlContent); + + var result = sut.UpdateContentForPath("root.missing", "69"); + + result.Should().Be(yamlContent); + } + [Test] public void CreateDotPathsForNodes_WithExistingDotNotationKeys_IgnoresThoseKeys() { diff --git a/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs b/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs index 8c1f2cd014..1d8061a30a 100644 --- a/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs +++ b/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs @@ -4,6 +4,7 @@ using System.IO; using System.Linq; using System.Text; +using Calamari.Common.Plumbing.Extensions; using YamlDotNet.Core; using YamlDotNet.RepresentationModel; using YamlDotNet.Serialization; @@ -22,7 +23,7 @@ public HelmYamlParser(string yamlContent) var reader = new StringReader(yamlString); yamlStream = new YamlStream(); yamlStream.Load(reader); - endsWithNewline = yamlString.EndsWith(Environment.NewLine); + endsWithNewline = yamlString.EndsWith("\n") || yamlString.EndsWith("\r"); } readonly string yamlString; @@ -87,6 +88,7 @@ string ReplaceNodeContent(YamlScalarNode node, string newValue) { var result = new StringBuilder(); using var reader = new StringReader(yamlString); + var newLine = yamlString.DetectLineEnding() ?? "\n"; var targetLine = (int)node.Start.Line; int startColumn; @@ -115,11 +117,11 @@ string ReplaceNodeContent(YamlScalarNode node, string newValue) // Replace in this line var before = line[..startColumn]; var after = line[endColumn..]; - result.AppendLine(before + newValue + after); + result.Append(before + newValue + after).Append(newLine); } else { - result.AppendLine(line); + result.Append(line).Append(newLine); } currentLine++; } From 9e18692862a9cbf5c69e677132d77c47af0d4074 Mon Sep 17 00:00:00 2001 From: Frank Lin Date: Tue, 15 Sep 2026 16:19:03 +1000 Subject: [PATCH 02/16] Route inline patch replacers through newline-aware serialization SerializeDocuments applied the detected line ending only to the separator between documents; each document body still came out of a plain StringWriter as LF. It also had no callers, while the three inline patch replacers each did their own Save(writer) plus TrimEnd, losing the file's line endings and trailing newline. SerializeDocuments now reapplies the line ending after emitting (the emitter writes '\n' directly and ignores TextWriter.NewLine) and restores the trailing newline when the original had one. The three replacers call it instead of serializing themselves. This removes the line-ending and trailing-newline noise only. These replacers still round-trip the document through YamlDotNet, so quoting, indentation and comments are still reflowed. Co-Authored-By: Claude Opus 5 (1M context) --- .../ArgoCD/YamlStreamLoaderTests.cs | 56 +++++++++++++++++++ .../InlineStrategicMergeImageReplacer.cs | 5 +- .../ArgoCD/InlineJsonPatchReplacer.cs | 4 +- .../ArgoCD/YamlJson6902PatchImageReplacer.cs | 11 +--- source/Calamari/ArgoCD/YamlStreamLoader.cs | 37 ++++++------ 5 files changed, 79 insertions(+), 34 deletions(-) create mode 100644 source/Calamari.Tests/ArgoCD/YamlStreamLoaderTests.cs diff --git a/source/Calamari.Tests/ArgoCD/YamlStreamLoaderTests.cs b/source/Calamari.Tests/ArgoCD/YamlStreamLoaderTests.cs new file mode 100644 index 0000000000..0937cac8c4 --- /dev/null +++ b/source/Calamari.Tests/ArgoCD/YamlStreamLoaderTests.cs @@ -0,0 +1,56 @@ +using Calamari.ArgoCD; +using FluentAssertions; +using NUnit.Framework; + +namespace Calamari.Tests.ArgoCD +{ + [TestFixture] + public class YamlStreamLoaderTests + { + [Test] + public void SerializeDocuments_WithCrlfOriginal_EmitsCrlfOnEveryLine() + { + const string original = "kind: Kustomization\r\nnamespace: dev\r\n"; + + var result = Serialize(original); + + result.Should().Be("kind: Kustomization\r\nnamespace: dev\r\n"); + } + + [Test] + public void SerializeDocuments_WithLfOriginal_EmitsLfOnEveryLine() + { + const string original = "kind: Kustomization\nnamespace: dev\n"; + + var result = Serialize(original); + + result.Should().Be("kind: Kustomization\nnamespace: dev\n"); + } + + [Test] + public void SerializeDocuments_WithOriginalMissingTrailingNewline_DoesNotAddOne() + { + const string original = "kind: Kustomization\nnamespace: dev"; + + var result = Serialize(original); + + result.Should().Be("kind: Kustomization\nnamespace: dev"); + } + + [Test] + public void SerializeDocuments_WithMultipleDocuments_SeparatesUsingTheOriginalLineEnding() + { + const string original = "kind: First\r\n---\r\nkind: Second\r\n"; + + var result = Serialize(original); + + result.Should().Be("kind: First\r\n---\r\nkind: Second\r\n"); + } + + static string Serialize(string original) + { + var stream = YamlStreamLoader.TryLoadSilent(original); + return YamlStreamLoader.SerializeDocuments(stream!.Documents, original); + } + } +} diff --git a/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs b/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs index 484e7c72ed..3e37462ecc 100644 --- a/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs +++ b/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs @@ -1,5 +1,4 @@ using System.Collections.Generic; -using System.IO; using System.Linq; using Calamari.ArgoCD.Models; using Calamari.Common.Plumbing.Logging; @@ -56,9 +55,7 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection(), new HashSet()); } - using var writer = new StringWriter(); - yamlStream.Save(writer, false); - var modifiedContent = writer.ToString().TrimEnd(); + var modifiedContent = YamlStreamLoader.SerializeDocuments(yamlStream.Documents, input); return new ImageReplacementResult(modifiedContent, allUpdatedImages, new HashSet()); } diff --git a/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs b/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs index e7a0ec585c..72484df8c4 100644 --- a/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs +++ b/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs @@ -91,9 +91,7 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection()); } diff --git a/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs b/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs index 0796ea902c..426c28e44e 100644 --- a/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs +++ b/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs @@ -1,7 +1,6 @@ #nullable enable using System; using System.Collections.Generic; -using System.IO; using System.Linq; using Calamari.ArgoCD.Conventions; using Calamari.ArgoCD.Models; @@ -72,15 +71,9 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection 0) - { - var singleDocStream = new YamlStream(stream.Documents[0]); - singleDocStream.Save(writer, false); - } - var modifiedContent = writer.ToString().TrimEnd(); + // Take just the first document to avoid unwanted document separators. + var modifiedContent = YamlStreamLoader.SerializeDocuments(stream.Documents.Take(1), yamlContent); return new ImageReplacementResult(modifiedContent, combinedResult.UpdatedImageReferences, combinedResult.AlreadyUpToDateImages); } diff --git a/source/Calamari/ArgoCD/YamlStreamLoader.cs b/source/Calamari/ArgoCD/YamlStreamLoader.cs index 83965b535a..c9a3ad5394 100644 --- a/source/Calamari/ArgoCD/YamlStreamLoader.cs +++ b/source/Calamari/ArgoCD/YamlStreamLoader.cs @@ -109,27 +109,28 @@ public static string SerializeDocuments(IEnumerable documents, str return string.Empty; var newLine = originalContent?.DetectLineEnding() ?? "\n"; - var serializedDocs = new List(); + var serializedDocs = documentList.Select(doc => SerializeDocument(doc, newLine)); - foreach (var doc in documentList) - { - using var writer = new StringWriter(); - var tempStream = new YamlStream(doc); - tempStream.Save(writer, false); - var serialized = writer.ToString(); - - serialized = serialized.TrimEnd(); - if (serialized.EndsWith("...")) - { - serialized = serialized.Substring(0, serialized.Length - 3).TrimEnd(); - } + var joined = string.Join($"{newLine}---{newLine}", serializedDocs); + return EndsWithNewLine(originalContent) ? joined + newLine : joined; + } - serializedDocs.Add(serialized); - } + static string SerializeDocument(YamlDocument document, string newLine) + { + // The emitter always writes '\n' regardless of the writer's NewLine, so the document's + // own line ending has to be reapplied afterwards. + using var writer = new StringWriter(); + new YamlStream(document).Save(writer, false); + + var serialized = writer.ToString().TrimEnd().ReplaceLineEndings(newLine); + return serialized.EndsWith("...") + ? serialized.Substring(0, serialized.Length - 3).TrimEnd() + : serialized; + } - return documentList.Count == 1 - ? serializedDocs[0] - : string.Join($"{newLine}---{newLine}", serializedDocs); + static bool EndsWithNewLine(string? content) + { + return content != null && (content.EndsWith("\n") || content.EndsWith("\r")); } } } \ No newline at end of file From 1743cd5c06e32a119f97a08b67ab782ae0015b0d Mon Sep 17 00:00:00 2001 From: Frank Lin Date: Thu, 17 Sep 2026 07:58:28 +1000 Subject: [PATCH 03/16] Extract HasTrailingNewLine alongside DetectLineEnding The trailing-newline check had two copies, one in each replacer path. It now sits next to DetectLineEnding in StringExtensions, which is where the line-ending handling already lives, and tolerates null so callers holding optional content do not need their own guard. Co-Authored-By: Claude Opus 5 (1M context) --- .../Plumbing/Extensions/StringExtensions.cs | 5 +++++ .../Fixtures/Util/StringExtensionsFixture.cs | 14 ++++++++++++++ source/Calamari/ArgoCD/Helm/HelmYamlParser.cs | 2 +- source/Calamari/ArgoCD/YamlStreamLoader.cs | 7 +------ 4 files changed, 21 insertions(+), 7 deletions(-) diff --git a/source/Calamari.Common/Plumbing/Extensions/StringExtensions.cs b/source/Calamari.Common/Plumbing/Extensions/StringExtensions.cs index 4ea21405bc..8d2dbb9739 100644 --- a/source/Calamari.Common/Plumbing/Extensions/StringExtensions.cs +++ b/source/Calamari.Common/Plumbing/Extensions/StringExtensions.cs @@ -99,6 +99,11 @@ public static string ToCamelCase(this string text) : null; } + public static bool HasTrailingNewLine(this string? input) + { + return input != null && (input.EndsWith("\n") || input.EndsWith("\r")); + } + public static string EnsureDoubleQuoteIfContainsSpaces(this string text) => EnsureDoubleQuote(text, t => t.Contains(" ")); public static string EnsureDoubleQuote(this string text) => EnsureDoubleQuote(text, t => !t.EndsWith("\"") && !t.StartsWith("\"")); public static string EnsureDoubleQuote(this string text, Predicate shouldQuote) => shouldQuote(text) ? $"\"{text}\"" : text; diff --git a/source/Calamari.Tests/Fixtures/Util/StringExtensionsFixture.cs b/source/Calamari.Tests/Fixtures/Util/StringExtensionsFixture.cs index ef72f9a9c1..9c48a6b7a4 100644 --- a/source/Calamari.Tests/Fixtures/Util/StringExtensionsFixture.cs +++ b/source/Calamari.Tests/Fixtures/Util/StringExtensionsFixture.cs @@ -44,6 +44,20 @@ public void AsRelativePathFrom(string source, string baseDirectory, string expec Assert.AreEqual(expected, source.AsRelativePathFrom(baseDirectory)); } + [TestCase("a\n", true)] + [TestCase("a\r\n", true)] + [TestCase("a\r", true)] + [TestCase("a\n\n", true)] + [TestCase("a", false)] + [TestCase("a ", false)] + [TestCase("", false)] + [TestCase(null, false)] + [Test] + public void HasTrailingNewLine_DetectsATrailingLineBreak(string input, bool expected) + { + input.HasTrailingNewLine().Should().Be(expected); + } + [TestCase("to_camel_case_function", "toCamelCaseFunction")] [TestCase("My S3 Bucket", "myS3Bucket")] [TestCase("-only-$AlphaNUMERIC-characters%^", "onlyAlphanumericCharacters")] diff --git a/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs b/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs index 1d8061a30a..27948ca441 100644 --- a/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs +++ b/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs @@ -23,7 +23,7 @@ public HelmYamlParser(string yamlContent) var reader = new StringReader(yamlString); yamlStream = new YamlStream(); yamlStream.Load(reader); - endsWithNewline = yamlString.EndsWith("\n") || yamlString.EndsWith("\r"); + endsWithNewline = yamlString.HasTrailingNewLine(); } readonly string yamlString; diff --git a/source/Calamari/ArgoCD/YamlStreamLoader.cs b/source/Calamari/ArgoCD/YamlStreamLoader.cs index c9a3ad5394..e7b5cc00d9 100644 --- a/source/Calamari/ArgoCD/YamlStreamLoader.cs +++ b/source/Calamari/ArgoCD/YamlStreamLoader.cs @@ -112,7 +112,7 @@ public static string SerializeDocuments(IEnumerable documents, str var serializedDocs = documentList.Select(doc => SerializeDocument(doc, newLine)); var joined = string.Join($"{newLine}---{newLine}", serializedDocs); - return EndsWithNewLine(originalContent) ? joined + newLine : joined; + return originalContent.HasTrailingNewLine() ? joined + newLine : joined; } static string SerializeDocument(YamlDocument document, string newLine) @@ -127,10 +127,5 @@ static string SerializeDocument(YamlDocument document, string newLine) ? serialized.Substring(0, serialized.Length - 3).TrimEnd() : serialized; } - - static bool EndsWithNewLine(string? content) - { - return content != null && (content.EndsWith("\n") || content.EndsWith("\r")); - } } } \ No newline at end of file From 9ccd29135663e311b5435bcd593f63eb9e656d83 Mon Sep 17 00:00:00 2001 From: Frank Lin Date: Thu, 17 Sep 2026 08:40:18 +1000 Subject: [PATCH 04/16] Assert line endings are preserved, not that they match the agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TwoImagesWithSameTag_OnlyUpdatesConfiguredPath asserted the output contained Environment.NewLine. Because .gitattributes checks .cs files out with native endings, the YAML literal's endings always equalled Environment.NewLine, so the assertion held on every platform and could not tell "preserved the file's endings" from "rewrote them to the agent's" — the two only differ for content that does not match the platform, which is exactly the reported case of an LF file on a Windows agent. It now spells the line endings out, runs for both LF and CRLF, and compares the whole result, so only the configured tag may change. A second case covers the reported symptom directly: CRLF with no trailing newline. Reverting the parser fix now fails both, where previously it failed neither. Co-Authored-By: Claude Opus 5 (1M context) --- ...elmValuesImageReplaceStepVariablesTests.cs | 39 +++++++++++++------ 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/source/Calamari.Tests/ArgoCD/Helm/HelmValuesImageReplaceStepVariablesTests.cs b/source/Calamari.Tests/ArgoCD/Helm/HelmValuesImageReplaceStepVariablesTests.cs index e68151e7b3..0d5242d0b2 100644 --- a/source/Calamari.Tests/ArgoCD/Helm/HelmValuesImageReplaceStepVariablesTests.cs +++ b/source/Calamari.Tests/ArgoCD/Helm/HelmValuesImageReplaceStepVariablesTests.cs @@ -1,4 +1,3 @@ -using System; using System.Collections.Generic; using Calamari.ArgoCD; using Calamari.ArgoCD.Conventions; @@ -119,16 +118,17 @@ public void StructuredValue_ImageOnNonDefaultRegistry_UpdatesFullRefAndTracksWit result.UpdatedContents.Should().Contain("name: us-docker.pkg.dev/shared-gke-dev-gqtrxy/argo-test/helloworld:v2"); } + // Line endings are spelled out rather than taken from a verbatim literal: .gitattributes checks + // .cs files out with native endings, so a literal's endings always match Environment.NewLine and + // an assertion against it cannot distinguish "preserved the file's endings" from "used the + // agent's". The customer's case was an LF file on a Windows agent, where those differ. [Test] - public void TwoImagesWithSameTag_OnlyUpdatesConfiguredPath() + [TestCase("\n")] + [TestCase("\r\n")] + public void TwoImagesWithSameTag_OnlyUpdatesConfiguredPath(string newLine) { - const string yaml = @" -nginx: - tag: 1.0 -redis: - tag: 1.0 -"; - + var yaml = string.Join(newLine, "", "nginx:", " tag: 1.0", "redis:", " tag: 1.0", ""); + var replacer = new HelmValuesImageReplaceStepVariables(yaml, DefaultRegistry, log); var images = new List { @@ -139,8 +139,25 @@ public void TwoImagesWithSameTag_OnlyUpdatesConfiguredPath() using var scope = new AssertionScope(); result.UpdatedImageReferences.Should().BeEquivalentTo(["nginx:1.27.1"]); - result.UpdatedContents.Should().Contain($"nginx:{Environment.NewLine} tag: 1.27.1"); - result.UpdatedContents.Should().Contain($"redis:{Environment.NewLine} tag: 1.0"); + result.UpdatedContents + .Should() + .Be(string.Join(newLine, "", "nginx:", " tag: 1.27.1", "redis:", " tag: 1.0", "")); + } + + [Test] + public void UpdatesTag_PreservingCrlfAndTheAbsenceOfATrailingNewline() + { + const string yaml = "image:\r\n tag: 1.0"; + + var replacer = new HelmValuesImageReplaceStepVariables(yaml, DefaultRegistry, log); + var images = new List + { + new(ContainerImageReference.FromReferenceString("nginx:1.27.1", DefaultRegistry), "image.tag") + }; + + var result = replacer.UpdateImages(images); + + result.UpdatedContents.Should().Be("image:\r\n tag: 1.27.1"); } [Test] From 8d01ff896560d47075c1d5042ce13424a7af7231 Mon Sep 17 00:00:00 2001 From: Frank Lin Date: Thu, 17 Sep 2026 08:54:51 +1000 Subject: [PATCH 05/16] Assert line endings exactly in the remaining Helm value tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five tests compared both sides through ReplaceLineEndings, so they passed whatever the replacer did to line endings. They now spell the endings out, run for both LF and CRLF, and compare the whole result, which keeps their original subjects — quote preservation and trailing whitespace — while also pinning the endings. UpdatesTag_PreservingCrlfAndTheAbsenceOfATrailingNewline now mirrors TwoImagesWithSameTag_OnlyUpdatesConfiguredPath without the trailing newline, so the pair differs in one variable rather than in shape, and covers both endings instead of only CRLF. Reverting the parser fix fails nine of these, where before this branch it failed none. Co-Authored-By: Claude Opus 5 (1M context) --- .../ArgoCD/Helm/HelmValuesEditorTests.cs | 43 +++++----- ...elmValuesImageReplaceStepVariablesTests.cs | 16 ++-- .../ArgoCD/Helm/HelmYamlParserTests.cs | 80 +++++-------------- 3 files changed, 53 insertions(+), 86 deletions(-) diff --git a/source/Calamari.Tests/ArgoCD/Helm/HelmValuesEditorTests.cs b/source/Calamari.Tests/ArgoCD/Helm/HelmValuesEditorTests.cs index de6d2736c0..443bd28da2 100644 --- a/source/Calamari.Tests/ArgoCD/Helm/HelmValuesEditorTests.cs +++ b/source/Calamari.Tests/ArgoCD/Helm/HelmValuesEditorTests.cs @@ -98,29 +98,32 @@ public void GenerateVariableDictionary_ReturnsDictionaryOfNodeValuesWithValues() result.Should().BeEquivalentTo(expected); } - [Test] - public void UpdateNodeValue_ReturnsModifiedYaml() + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdateNodeValue_ReturnsModifiedYaml(string newLine) { - const string yamlContent = @"root: - node1: ""node1value"" - node2: - node2Nest: - node2nestedValue: ""banana"" - node2Child1: ""node2child1value"" - node2Child2: 42 -"; + var yamlContent = string.Join(newLine, + "root:", + " node1: \"node1value\"", + " node2:", + " node2Nest:", + " node2nestedValue: \"banana\"", + " node2Child1: \"node2child1value\"", + " node2Child2: 42", + ""); + var result = HelmValuesEditor.UpdateNodeValue(yamlContent, "root.node1", "awesome new value"); - const string expected = @"root: - node1: ""awesome new value"" - node2: - node2Nest: - node2nestedValue: ""banana"" - node2Child1: ""node2child1value"" - node2Child2: 42 -"; - //ensure platform-agnostic multiline comparison - result.ReplaceLineEndings().Should().Be(expected.ReplaceLineEndings()); + result.Should() + .Be(string.Join(newLine, + "root:", + " node1: \"awesome new value\"", + " node2:", + " node2Nest:", + " node2nestedValue: \"banana\"", + " node2Child1: \"node2child1value\"", + " node2Child2: 42", + "")); } } } diff --git a/source/Calamari.Tests/ArgoCD/Helm/HelmValuesImageReplaceStepVariablesTests.cs b/source/Calamari.Tests/ArgoCD/Helm/HelmValuesImageReplaceStepVariablesTests.cs index 0d5242d0b2..c063f5e888 100644 --- a/source/Calamari.Tests/ArgoCD/Helm/HelmValuesImageReplaceStepVariablesTests.cs +++ b/source/Calamari.Tests/ArgoCD/Helm/HelmValuesImageReplaceStepVariablesTests.cs @@ -122,7 +122,6 @@ public void StructuredValue_ImageOnNonDefaultRegistry_UpdatesFullRefAndTracksWit // .cs files out with native endings, so a literal's endings always match Environment.NewLine and // an assertion against it cannot distinguish "preserved the file's endings" from "used the // agent's". The customer's case was an LF file on a Windows agent, where those differ. - [Test] [TestCase("\n")] [TestCase("\r\n")] public void TwoImagesWithSameTag_OnlyUpdatesConfiguredPath(string newLine) @@ -144,20 +143,25 @@ public void TwoImagesWithSameTag_OnlyUpdatesConfiguredPath(string newLine) .Be(string.Join(newLine, "", "nginx:", " tag: 1.27.1", "redis:", " tag: 1.0", "")); } - [Test] - public void UpdatesTag_PreservingCrlfAndTheAbsenceOfATrailingNewline() + [TestCase("\n")] + [TestCase("\r\n")] + public void TwoImagesWithSameTag_WithoutATrailingNewline_OnlyUpdatesConfiguredPath(string newLine) { - const string yaml = "image:\r\n tag: 1.0"; + var yaml = string.Join(newLine, "", "nginx:", " tag: 1.0", "redis:", " tag: 1.0"); var replacer = new HelmValuesImageReplaceStepVariables(yaml, DefaultRegistry, log); var images = new List { - new(ContainerImageReference.FromReferenceString("nginx:1.27.1", DefaultRegistry), "image.tag") + new(ContainerImageReference.FromReferenceString("nginx:1.27.1", DefaultRegistry), "nginx.tag") }; var result = replacer.UpdateImages(images); - result.UpdatedContents.Should().Be("image:\r\n tag: 1.27.1"); + using var scope = new AssertionScope(); + result.UpdatedImageReferences.Should().BeEquivalentTo(["nginx:1.27.1"]); + result.UpdatedContents + .Should() + .Be(string.Join(newLine, "", "nginx:", " tag: 1.27.1", "redis:", " tag: 1.0")); } [Test] diff --git a/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs b/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs index 88f110e0b6..5ba552469f 100644 --- a/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs +++ b/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs @@ -51,96 +51,56 @@ public void GetValueAtPath_ReturnsTheValueOfTheSpecifiedNode(string path, string result.Should().Be(expected); } - [Test] - public void UpdateNodeValue_WithNonDelimitedNodeValue_ReplacesValueInDocument() + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdateNodeValue_WithNonDelimitedNodeValue_ReplacesValueInDocument(string newLine) { - const string yamlContent = @" -root: - node1: 42 - node2: stable -"; + var yamlContent = string.Join(newLine, "", "root:", " node1: 42", " node2: stable", ""); var sut = new HelmYamlParser(yamlContent); - const string expectedUpdate = @" -root: - node1: 69 - node2: stable -"; - var result = sut.UpdateContentForPath("root.node1", "69"); - //ensure platform-agnostic multiline comparison - result.ReplaceLineEndings().Should().Be(expectedUpdate.ReplaceLineEndings()); + result.Should().Be(string.Join(newLine, "", "root:", " node1: 69", " node2: stable", "")); } - [Test] - public void UpdateNodeValue_WithDoubleQuoteDelimitedNodeValue_PreservesDelimitersWithNewValue() + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdateNodeValue_WithDoubleQuoteDelimitedNodeValue_PreservesDelimitersWithNewValue(string newLine) { - const string yamlContent = @" -root: - node1: 42 - node2: ""latest"" -"; + var yamlContent = string.Join(newLine, "", "root:", " node1: 42", " node2: \"latest\"", ""); var sut = new HelmYamlParser(yamlContent); - const string expectedUpdate = @" -root: - node1: 42 - node2: ""stable"" -"; - var result = sut.UpdateContentForPath("root.node2", "stable"); - //ensure platform-agnostic multiline comparison - result.ReplaceLineEndings().Should().Be(expectedUpdate.ReplaceLineEndings()); + result.Should().Be(string.Join(newLine, "", "root:", " node1: 42", " node2: \"stable\"", "")); } - [Test] - public void UpdateNodeValue_WithSingleQuoteDelimitedNodeValue_PreservesDelimitersWithNewValue() + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdateNodeValue_WithSingleQuoteDelimitedNodeValue_PreservesDelimitersWithNewValue(string newLine) { - const string yamlContent = @" -root: - node1: 42 - node2: 'latest' -"; + var yamlContent = string.Join(newLine, "", "root:", " node1: 42", " node2: 'latest'", ""); var sut = new HelmYamlParser(yamlContent); - const string expectedUpdate = @" -root: - node1: 42 - node2: 'stable' -"; - var result = sut.UpdateContentForPath("root.node2", "stable"); - //ensure platform-agnostic multiline comparison - result.ReplaceLineEndings().Should().Be(expectedUpdate.ReplaceLineEndings()); + result.Should().Be(string.Join(newLine, "", "root:", " node1: 42", " node2: 'stable'", "")); } - [Test] - public void UpdateNodeValue_RespectsTrailingWhitespaceFromInput() + [TestCase("\n")] + [TestCase("\r\n")] + public void UpdateNodeValue_RespectsTrailingWhitespaceFromInput(string newLine) { - const string yamlContent = @" -root: - node1: 42 - -"; + var yamlContent = string.Join(newLine, "", "root:", " node1: 42", " ", ""); var sut = new HelmYamlParser(yamlContent); - const string expectedUpdate = @" -root: - node1: 69 - -"; - var result = sut.UpdateContentForPath("root.node1", "69"); - //ensure platform-agnostic multiline comparison - result.ReplaceLineEndings().Should().Be(expectedUpdate.ReplaceLineEndings()); + result.Should().Be(string.Join(newLine, "", "root:", " node1: 69", " ", "")); } [Test] From bad625f6084984a2d53f270e8f0bc2297c057439 Mon Sep 17 00:00:00 2001 From: Nick Josevski Date: Tue, 8 Sep 2026 21:01:50 +1000 Subject: [PATCH 06/16] Run gzip directly in BuildDockerImages instead of through pwsh BuildDockerImages launched PowerShell for exactly one thing: running `gzip -k -9 -f` on the OCI tar. That made the step depend on whichever .NET runtime the agent's `pwsh` global tool was built against, and on main's build agent those no longer line up: App: /root/.dotnet/tools/pwsh Framework: 'Microsoft.NETCore.App', version '10.0.0' .NET location: .../.nuke/temp/dotnet-unix The following frameworks were found: 8.0.30 The agent's `pwsh` needs .NET 10, and the only runtime on offer is the .NET 8 SDK that build.sh bootstraps into .nuke/temp and puts on PATH. Calling gzip directly removes pwsh from the equation. This was the build's only use of PowerShellTasks, so nothing else in the build cares about the agent's pwsh now. Failures also surface properly. `pwsh -Command` exits 0 regardless of the native exit code, so a failed gzip used to show up later as a confusing missing-artifact error from PublishArtifacts. A Nuke Tool asserts a zero exit code, so it now fails at the gzip call with gzip's stderr attached. Verified with a throwaway target: resolves /usr/bin/gzip from PATH, arguments pass through intact, -k keeps the .tar alongside the .gz, and a deliberate failure raises `ProcessException: Process 'gzip' exited with code 1`. Not addressed here: reaching the SDK bootstrap at all means `dotnet --version` failed, so the agent no longer satisfies global.json's 8.0.419 pin. That costs every build a full SDK download and belongs with the .NET 10 work. Co-Authored-By: Claude Opus 5 (1M context) (cherry picked from commit edee954078116d951e06f51db3c767056765b1f1) --- build/Build.Docker.cs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/build/Build.Docker.cs b/build/Build.Docker.cs index c9bf2006f4..585b83b93a 100644 --- a/build/Build.Docker.cs +++ b/build/Build.Docker.cs @@ -1,12 +1,15 @@ using Calamari.Build.Utilities; using JetBrains.Annotations; +using Nuke.Common.Tooling; using Nuke.Common.Tools.Docker; -using Nuke.Common.Tools.PowerShell; namespace Calamari.Build; public partial class Build { + //Resolved from PATH so a missing gzip fails by name, rather than as a mystery exit code + static Tool Gzip => ToolResolver.GetPathTool("gzip"); + [PublicAPI] Target BuildDockerImages => d => @@ -82,10 +85,10 @@ public partial class Build return settings; }); - //compress with gzip - PowerShellTasks.PowerShell(_ => _ - .EnableNoProfile() - .SetCommand($"gzip -k -9 -f '{outputFile}'")); + //compress with gzip. Invoked directly rather than via pwsh, which only + //added a dependency on whatever .NET runtime the agent's `pwsh` global tool + //was built against - not the SDK this build pins. + Gzip($"-k -9 -f \"{outputFile}\""); //gzip always uses the .gz suffix var compressedZipPath = $"{outputFile}.gz"; From ecb3d797e44866b1164379c88d2b079683c05e37 Mon Sep 17 00:00:00 2001 From: Frank Lin Date: Tue, 15 Sep 2026 16:39:00 +1000 Subject: [PATCH 07/16] Splice inline patch image tags instead of re-emitting the document The three inline patch replacers parsed kustomization.yaml, mutated a node and re-emitted the whole document. That drops every comment and blank line, reflows indentation, quoting and folded scalars, and appends a "..." marker, so a one-line image tag change still arrived as an unreviewable whole-file diff even after the line endings were fixed. YamlScalarSplicer replaces a scalar's value in the original text and leaves every other byte alone. It handles plain and quoted scalars, keeping the existing quotes, and literal block scalars, reindenting the new content to the block's own indentation and preserving whether the block ended with a line break. Multiple edits are applied last-to-first so earlier offsets stay valid. HelmYamlParser now shares the splicer rather than carrying its own copy of the offset arithmetic, and the 6902 replacer no longer force-quotes image values, since that only existed to survive re-emission. SerializeDocuments goes with it: nothing calls it any more, and leaving it in place as dead code is what hid the original defect. Co-Authored-By: Claude Opus 5 (1M context) --- .../ArgoCD/InlineJsonPatchReplacerTests.cs | 28 ++++ .../ArgoCD/InlineStrategicMergeTest.cs | 31 ++++ .../YamlJson6902PatchImageReplacerTests.cs | 20 +++ .../ArgoCD/YamlScalarSplicerTests.cs | 88 +++++++++++ .../ArgoCD/YamlStreamLoaderTests.cs | 56 ------- .../InlineStrategicMergeImageReplacer.cs | 5 +- source/Calamari/ArgoCD/Helm/HelmYamlParser.cs | 52 +----- .../ArgoCD/InlineJsonPatchReplacer.cs | 13 +- .../ArgoCD/YamlJson6902PatchImageReplacer.cs | 47 +++--- source/Calamari/ArgoCD/YamlScalarSplicer.cs | 148 ++++++++++++++++++ source/Calamari/ArgoCD/YamlStreamLoader.cs | 38 ----- 11 files changed, 349 insertions(+), 177 deletions(-) create mode 100644 source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs delete mode 100644 source/Calamari.Tests/ArgoCD/YamlStreamLoaderTests.cs create mode 100644 source/Calamari/ArgoCD/YamlScalarSplicer.cs diff --git a/source/Calamari.Tests/ArgoCD/InlineJsonPatchReplacerTests.cs b/source/Calamari.Tests/ArgoCD/InlineJsonPatchReplacerTests.cs index 1037b720d0..a3a0d51b19 100644 --- a/source/Calamari.Tests/ArgoCD/InlineJsonPatchReplacerTests.cs +++ b/source/Calamari.Tests/ArgoCD/InlineJsonPatchReplacerTests.cs @@ -21,6 +21,34 @@ public class InlineJsonPatchReplacerTests ILog log = new InMemoryLog(); + [Test] + public void UpdateImages_ChangesOnlyTheImageReference_PreservingCommentsAndLineEndings() + { + const string inputYaml = "# managed by the platform team\r\n" + + "apiVersion: kustomize.config.k8s.io/v1beta1\r\n" + + "kind: Kustomization\r\n" + + "\r\n" + + "patches:\r\n" + + " - target:\r\n" + + " kind: Deployment # only the web tier\r\n" + + " patch: |-\r\n" + + " apiVersion: apps/v1\r\n" + + " kind: Deployment\r\n" + + " spec:\r\n" + + " template:\r\n" + + " spec:\r\n" + + " containers:\r\n" + + " - name: nginx\r\n" + + " image: nginx:1.21\r\n"; + + var replacer = new InlineJsonPatchReplacer(inputYaml, ArgoCDConstants.DefaultContainerRegistry, log); + + var result = replacer.UpdateImages(imagesToUpdate); + + result.UpdatedImageReferences.Should().ContainSingle().Which.Should().Be("nginx:1.25"); + result.UpdatedContents.Should().Be(inputYaml.Replace("nginx:1.21", "nginx:1.25")); + } + [Test] public void UpdateImages_WithInlinePatchContainerImage_UpdatesImageReference() { diff --git a/source/Calamari.Tests/ArgoCD/InlineStrategicMergeTest.cs b/source/Calamari.Tests/ArgoCD/InlineStrategicMergeTest.cs index 106a25198b..8ff131add4 100644 --- a/source/Calamari.Tests/ArgoCD/InlineStrategicMergeTest.cs +++ b/source/Calamari.Tests/ArgoCD/InlineStrategicMergeTest.cs @@ -14,6 +14,37 @@ namespace Calamari.Tests.ArgoCD; public class InlineStrategicMergeTest { readonly ILog log = new InMemoryLog(); + [Test] + public void UpdateImages_ChangesOnlyTheImageReference_PreservingCommentsAndLineEndings() + { + const string content = "# strategic merge patches\r\n" + + "apiVersion: kustomize.config.k8s.io/v1beta1\r\n" + + "kind: Kustomization\r\n" + + "\r\n" + + "patchesStrategicMerge:\r\n" + + " - |\r\n" + + " apiVersion: apps/v1\r\n" + + " kind: Deployment\r\n" + + " spec:\r\n" + + " template:\r\n" + + " spec:\r\n" + + " containers:\r\n" + + " - name: nginx # the web tier\r\n" + + " image: nginx:1.21\r\n"; + + var imagesToUpdate = new List + { + new(ContainerImageReference.FromReferenceString("nginx:1.25", "default-registry")) + }; + + var replacer = new InlineStrategicMergeImageReplacer(content, "default-registry", log); + + var result = replacer.UpdateImages(imagesToUpdate); + + result.UpdatedImageReferences.Should().ContainSingle().Which.Should().Be("nginx:1.25"); + result.UpdatedContents.Should().Be(content.Replace("nginx:1.21", "nginx:1.25")); + } + [Test] public void ProcessInlineStrategicMergePatches_WithInlinePatches_UpdatesImages() { diff --git a/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs b/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs index 1ace1e4aec..2372801389 100644 --- a/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs +++ b/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs @@ -196,6 +196,26 @@ public void UpdateImages_WithInvalidYaml_ReturnsNoChange() result.UpdatedContents.Should().Be(invalidYaml); } + [Test] + public void UpdateImages_ChangesOnlyTheImageReference_PreservingCommentsAndLineEndings() + { + const string yamlContent = "# rollout patch\r\n" + + "- op: replace\r\n" + + " path: /spec/template/spec/containers/0/image # web\r\n" + + " value: nginx:1.21\r\n" + + "\r\n" + + "- op: replace\r\n" + + " path: /spec/template/spec/initContainers/0/image\r\n" + + " value: \"nginx:1.21\"\r\n"; + + var replacer = new YamlJson6902PatchImageReplacer(yamlContent, ArgoCDConstants.DefaultContainerRegistry, log); + + var result = replacer.UpdateImages(imagesToUpdate); + + result.UpdatedImageReferences.Should().ContainSingle().Which.Should().Be("nginx:1.25"); + result.UpdatedContents.Should().Be(yamlContent.Replace("nginx:1.21", "nginx:1.25")); + } + [Test] public void UpdateImages_WithComplexPatch_UpdatesCorrectly() { diff --git a/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs b/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs new file mode 100644 index 0000000000..c2dab38e88 --- /dev/null +++ b/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs @@ -0,0 +1,88 @@ +using System.Linq; +using Calamari.ArgoCD; +using FluentAssertions; +using NUnit.Framework; +using YamlDotNet.RepresentationModel; + +namespace Calamari.Tests.ArgoCD +{ + [TestFixture] + public class YamlScalarSplicerTests + { + [Test] + public void ReplaceValue_OnPlainScalar_LeavesTheRestOfTheFileUntouched() + { + const string document = "# top comment\r\nimage: nginx:1.21 # pinned\r\n\r\nother: keep\r\n"; + + var result = Replace(document, "image", "nginx:1.25"); + + result.Should().Be("# top comment\r\nimage: nginx:1.25 # pinned\r\n\r\nother: keep\r\n"); + } + + [Test] + public void ReplaceValue_OnQuotedScalar_KeepsTheOriginalQuotes() + { + const string document = "image: \"nginx:1.21\"\nsingle: 'nginx:1.21'\n"; + + var result = Replace(document, "image", "nginx:1.25"); + + result.Should().Be("image: \"nginx:1.25\"\nsingle: 'nginx:1.21'\n"); + } + + [Test] + public void ReplaceValue_OnLiteralBlock_ReindentsAndKeepsSurroundingText() + { + const string document = "patch: |-\r\n kind: Deployment\r\n image: nginx:1.21\r\ntarget: x\r\n"; + + var result = Replace(document, "patch", "kind: Deployment\nimage: nginx:1.25"); + + result.Should().Be("patch: |-\r\n kind: Deployment\r\n image: nginx:1.25\r\ntarget: x\r\n"); + } + + [Test] + public void ReplaceValue_OnLiteralBlockWithBlankLine_DoesNotIndentTheBlankLine() + { + const string document = "patch: |\n one\n\n two\nafter: x\n"; + + var result = Replace(document, "patch", "one\n\ntwo\n"); + + result.Should().Be("patch: |\n one\n\n two\nafter: x\n"); + } + + [Test] + public void ReplaceValue_OnLiteralBlockAtEndOfFileWithoutTrailingNewline_DoesNotAddOne() + { + const string document = "patch: |-\n image: nginx:1.21"; + + var result = Replace(document, "patch", "image: nginx:1.25"); + + result.Should().Be("patch: |-\n image: nginx:1.25"); + } + + [Test] + public void ReplaceValues_WithSeveralEdits_AppliesThemAll() + { + const string document = "a: nginx:1.21\r\nb: nginx:1.21\r\nc: nginx:1.21\r\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var edits = new[] { "a", "b", "c" } + .Select(key => new YamlScalarEdit((YamlScalarNode)root.Children[new YamlScalarNode(key)], "nginx:1.25")); + + var result = YamlScalarSplicer.ReplaceValues(document, edits); + + result.Should().Be("a: nginx:1.25\r\nb: nginx:1.25\r\nc: nginx:1.25\r\n"); + } + + static string Replace(string document, string key, string newValue) + { + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var node = (YamlScalarNode)root.Children[new YamlScalarNode(key)]; + return YamlScalarSplicer.ReplaceValue(document, node, newValue); + } + + static YamlStream Load(string document) + { + return YamlStreamLoader.TryLoadSilent(document)!; + } + } +} diff --git a/source/Calamari.Tests/ArgoCD/YamlStreamLoaderTests.cs b/source/Calamari.Tests/ArgoCD/YamlStreamLoaderTests.cs deleted file mode 100644 index 0937cac8c4..0000000000 --- a/source/Calamari.Tests/ArgoCD/YamlStreamLoaderTests.cs +++ /dev/null @@ -1,56 +0,0 @@ -using Calamari.ArgoCD; -using FluentAssertions; -using NUnit.Framework; - -namespace Calamari.Tests.ArgoCD -{ - [TestFixture] - public class YamlStreamLoaderTests - { - [Test] - public void SerializeDocuments_WithCrlfOriginal_EmitsCrlfOnEveryLine() - { - const string original = "kind: Kustomization\r\nnamespace: dev\r\n"; - - var result = Serialize(original); - - result.Should().Be("kind: Kustomization\r\nnamespace: dev\r\n"); - } - - [Test] - public void SerializeDocuments_WithLfOriginal_EmitsLfOnEveryLine() - { - const string original = "kind: Kustomization\nnamespace: dev\n"; - - var result = Serialize(original); - - result.Should().Be("kind: Kustomization\nnamespace: dev\n"); - } - - [Test] - public void SerializeDocuments_WithOriginalMissingTrailingNewline_DoesNotAddOne() - { - const string original = "kind: Kustomization\nnamespace: dev"; - - var result = Serialize(original); - - result.Should().Be("kind: Kustomization\nnamespace: dev"); - } - - [Test] - public void SerializeDocuments_WithMultipleDocuments_SeparatesUsingTheOriginalLineEnding() - { - const string original = "kind: First\r\n---\r\nkind: Second\r\n"; - - var result = Serialize(original); - - result.Should().Be("kind: First\r\n---\r\nkind: Second\r\n"); - } - - static string Serialize(string original) - { - var stream = YamlStreamLoader.TryLoadSilent(original); - return YamlStreamLoader.SerializeDocuments(stream!.Documents, original); - } - } -} diff --git a/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs b/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs index 3e37462ecc..8abfacb669 100644 --- a/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs +++ b/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs @@ -34,6 +34,7 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection(); + var edits = new List(); foreach (var patchNode in patchSequence.Children) { if (patchNode is YamlScalarNode patchScalar && patchScalar.Style == ScalarStyle.Literal) @@ -44,7 +45,7 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection 0) { - patchScalar.Value = result.UpdatedContents; + edits.Add(new YamlScalarEdit(patchScalar, result.UpdatedContents)); allUpdatedImages.UnionWith(result.UpdatedImageReferences); } } @@ -55,7 +56,7 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection(), new HashSet()); } - var modifiedContent = YamlStreamLoader.SerializeDocuments(yamlStream.Documents, input); + var modifiedContent = YamlScalarSplicer.ReplaceValues(input, edits); return new ImageReplacementResult(modifiedContent, allUpdatedImages, new HashSet()); } diff --git a/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs b/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs index 27948ca441..2d2799a0ee 100644 --- a/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs +++ b/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs @@ -3,9 +3,6 @@ using System.Collections.Generic; using System.IO; using System.Linq; -using System.Text; -using Calamari.Common.Plumbing.Extensions; -using YamlDotNet.Core; using YamlDotNet.RepresentationModel; using YamlDotNet.Serialization; @@ -23,12 +20,10 @@ public HelmYamlParser(string yamlContent) var reader = new StringReader(yamlString); yamlStream = new YamlStream(); yamlStream.Load(reader); - endsWithNewline = yamlString.HasTrailingNewLine(); } readonly string yamlString; readonly YamlStream yamlStream; - readonly bool endsWithNewline; public string GetValueAtPath(string path) { @@ -78,57 +73,12 @@ public string UpdateContentForPath(string path, string newValue) var nodeAtPath = GetNodeAtPath(path); if (nodeAtPath != null) { - return ReplaceNodeContent(nodeAtPath, newValue); + return YamlScalarSplicer.ReplaceValue(yamlString, nodeAtPath, newValue); } return yamlString; } - string ReplaceNodeContent(YamlScalarNode node, string newValue) - { - var result = new StringBuilder(); - using var reader = new StringReader(yamlString); - var newLine = yamlString.DetectLineEnding() ?? "\n"; - - var targetLine = (int)node.Start.Line; - int startColumn; - int endColumn; - switch (node.Style) - { - case ScalarStyle.Literal: - case ScalarStyle.Plain: - startColumn = (int)node.Start.Column - 1; - endColumn = (int)node.End.Column - 1; - break; - case ScalarStyle.DoubleQuoted: - case ScalarStyle.SingleQuoted: - startColumn = (int)node.Start.Column; - endColumn = (int)node.End.Column - 2; - break; - default: - throw new NotSupportedException("Modifying Folded or Ambiguous Scar Values is not supported."); - } - int currentLine = 1; - - while (reader.ReadLine() is { } line) - { - if (currentLine == targetLine) - { - // Replace in this line - var before = line[..startColumn]; - var after = line[endColumn..]; - result.Append(before + newValue + after).Append(newLine); - } - else - { - result.Append(line).Append(newLine); - } - currentLine++; - } - - return endsWithNewline ? result.ToString() : result.ToString().TrimEnd(); - } - static void FlattenObject(object? obj, string currentPath, List paths) { switch (obj) diff --git a/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs b/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs index 72484df8c4..5a462a7de4 100644 --- a/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs +++ b/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs @@ -80,9 +80,10 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection(); + var edits = new List(); foreach (var patchNode in patchesSequence.OfType()) { - var changes = ProcessPatchNode(patchNode, imagesToUpdate); + var changes = ProcessPatchNode(patchNode, imagesToUpdate, edits); replacementsMade.UnionWith(changes); } @@ -91,11 +92,11 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection()); } - HashSet ProcessPatchNode(YamlMappingNode patchNode, IReadOnlyCollection imagesToUpdate) + HashSet ProcessPatchNode(YamlMappingNode patchNode, IReadOnlyCollection imagesToUpdate, List edits) { var changes = new HashSet(); @@ -106,7 +107,7 @@ HashSet ProcessPatchNode(YamlMappingNode patchNode, IReadOnlyCollection< { if (kvp.Key is YamlScalarNode scalar && scalar.Value == FieldNames.Patch && kvp.Value is YamlScalarNode patchContentScalar) { - var patchChanges = ProcessInlinePatchContent(patchContentScalar, imagesToUpdate); + var patchChanges = ProcessInlinePatchContent(patchContentScalar, imagesToUpdate, edits); changes.UnionWith(patchChanges); break; } @@ -116,7 +117,7 @@ HashSet ProcessPatchNode(YamlMappingNode patchNode, IReadOnlyCollection< return changes; } - HashSet ProcessInlinePatchContent(YamlScalarNode patchContentNode, IReadOnlyCollection imagesToUpdate) + HashSet ProcessInlinePatchContent(YamlScalarNode patchContentNode, IReadOnlyCollection imagesToUpdate, List edits) { var changes = new HashSet(); @@ -141,7 +142,7 @@ HashSet ProcessInlinePatchContent(YamlScalarNode patchContentNode, IRead if (result.UpdatedImageReferences.Count > 0) { - patchContentNode.Value = result.UpdatedContents; + edits.Add(new YamlScalarEdit(patchContentNode, result.UpdatedContents)); } } catch (Exception ex) diff --git a/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs b/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs index 426c28e44e..cc7587fdff 100644 --- a/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs +++ b/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs @@ -5,7 +5,6 @@ using Calamari.ArgoCD.Conventions; using Calamari.ArgoCD.Models; using Calamari.Common.Plumbing.Logging; -using YamlDotNet.Core; using YamlDotNet.RepresentationModel; namespace Calamari.ArgoCD @@ -50,6 +49,7 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection(); + var edits = new List(); // Process each document in the YAML stream foreach (var document in stream.Documents) @@ -59,7 +59,7 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection()) { - var operationResult = ProcessPatchOperation(operationNode, imagesToUpdate); + var operationResult = ProcessPatchOperation(operationNode, imagesToUpdate, edits); results.Add(operationResult); } } @@ -71,15 +71,14 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection imagesToUpdate) + IReadOnlyCollection imagesToUpdate, + List edits) { var opValue = operationNode.GetStringValue(FieldNames.Op); var pathValue = operationNode.GetStringValue(FieldNames.Path); @@ -91,20 +90,21 @@ ImageReplacementResult ProcessPatchOperation(YamlMappingNode operationNode, return opValue switch { - OpValues.Replace => ProcessReplaceOperation(operationNode, pathValue, imagesToUpdate), - OpValues.Add => ProcessAddOperation(operationNode, pathValue, imagesToUpdate), + OpValues.Replace => ProcessReplaceOperation(operationNode, pathValue, imagesToUpdate, edits), + OpValues.Add => ProcessAddOperation(operationNode, pathValue, imagesToUpdate, edits), _ => NoChangeResult }; } ImageReplacementResult ProcessReplaceOperation(YamlMappingNode operationNode, string path, - IReadOnlyCollection imagesToUpdate) + IReadOnlyCollection imagesToUpdate, + List edits) { if (IsImagePath(path) && operationNode.Children.TryGetValue(new YamlScalarNode(FieldNames.Value), out var valueNode)) { if (valueNode is YamlScalarNode imageScalar && !string.IsNullOrEmpty(imageScalar.Value)) { - return ProcessImageReference(imageScalar, imagesToUpdate); + return ProcessImageReference(imageScalar, imagesToUpdate, edits); } } @@ -112,17 +112,18 @@ ImageReplacementResult ProcessReplaceOperation(YamlMappingNode operationNode, st } ImageReplacementResult ProcessAddOperation(YamlMappingNode operationNode, string path, - IReadOnlyCollection imagesToUpdate) + IReadOnlyCollection imagesToUpdate, + List edits) { if (IsContainersPath(path) && operationNode.Children.TryGetValue(new YamlScalarNode(FieldNames.Value), out var valueNode)) { if (valueNode is YamlSequenceNode containersSequence) { - return ProcessContainersSequence(containersSequence, imagesToUpdate); + return ProcessContainersSequence(containersSequence, imagesToUpdate, edits); } else if (valueNode is YamlMappingNode singleContainer) { - return ProcessContainerMapping(singleContainer, imagesToUpdate); + return ProcessContainerMapping(singleContainer, imagesToUpdate, edits); } } @@ -130,13 +131,14 @@ ImageReplacementResult ProcessAddOperation(YamlMappingNode operationNode, string } ImageReplacementResult ProcessContainersSequence(YamlSequenceNode containersSequence, - IReadOnlyCollection imagesToUpdate) + IReadOnlyCollection imagesToUpdate, + List edits) { var results = new List(); foreach (var containerNode in containersSequence.Children.OfType()) { - var result = ProcessContainerMapping(containerNode, imagesToUpdate); + var result = ProcessContainerMapping(containerNode, imagesToUpdate, edits); results.Add(result); } @@ -144,19 +146,21 @@ ImageReplacementResult ProcessContainersSequence(YamlSequenceNode containersSequ } ImageReplacementResult ProcessContainerMapping(YamlMappingNode containerNode, - IReadOnlyCollection imagesToUpdate) + IReadOnlyCollection imagesToUpdate, + List edits) { if (containerNode.Children.TryGetValue(new YamlScalarNode(FieldNames.Image), out var imageNode) && imageNode is YamlScalarNode imageScalar) { - return ProcessImageReference(imageScalar, imagesToUpdate); + return ProcessImageReference(imageScalar, imagesToUpdate, edits); } return NoChangeResult; } ImageReplacementResult ProcessImageReference(YamlScalarNode imageScalar, - IReadOnlyCollection imagesToUpdate) + IReadOnlyCollection imagesToUpdate, + List edits) { if (string.IsNullOrEmpty(imageScalar.Value)) return NoChangeResult; @@ -169,12 +173,7 @@ ImageReplacementResult ProcessImageReference(YamlScalarNode imageScalar, if (matchedUpdate != null && !matchedUpdate.Comparison.TagMatch) { var newImageRef = currentImageRef.WithTag(matchedUpdate.Reference.Tag); - imageScalar.Value = newImageRef.FriendlyName(); - - if (imageScalar.Style != ScalarStyle.SingleQuoted && imageScalar.Style != ScalarStyle.DoubleQuoted) - { - imageScalar.Style = ScalarStyle.DoubleQuoted; - } + edits.Add(new YamlScalarEdit(imageScalar, newImageRef.FriendlyName())); log.Verbose($"Updated container image in YAML JSON 6902 patch: {newImageRef.FriendlyName()}"); diff --git a/source/Calamari/ArgoCD/YamlScalarSplicer.cs b/source/Calamari/ArgoCD/YamlScalarSplicer.cs new file mode 100644 index 0000000000..b83430dd61 --- /dev/null +++ b/source/Calamari/ArgoCD/YamlScalarSplicer.cs @@ -0,0 +1,148 @@ +#nullable enable +using System; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using Calamari.Common.Plumbing.Extensions; +using YamlDotNet.Core; +using YamlDotNet.RepresentationModel; + +namespace Calamari.ArgoCD +{ + public record YamlScalarEdit(YamlScalarNode Node, string NewValue); + + /// + /// Replaces scalar values by splicing into the original YAML text. Re-emitting a parsed document + /// drops comments and blank lines and reflows indentation, quoting and folded scalars, which turns + /// a one-line image tag change into an unreviewable whole-file diff. Splicing leaves every byte + /// outside the replaced value exactly as it was, line endings included. + /// + public static class YamlScalarSplicer + { + public static string ReplaceValue(string document, YamlScalarNode node, string newValue) + { + return ReplaceValues(document, new[] { new YamlScalarEdit(node, newValue) }); + } + + public static string ReplaceValues(string document, IEnumerable edits) + { + // Applying last-to-first keeps the offsets of the remaining, earlier edits valid. + var ordered = edits.OrderByDescending(e => e.Node.Start.Line) + .ThenByDescending(e => e.Node.Start.Column); + + return ordered.Aggregate(document, Splice); + } + + static string Splice(string document, YamlScalarEdit edit) + { + return edit.Node.Style == ScalarStyle.Literal + ? SpliceBlockScalar(document, edit.Node, edit.NewValue) + : SpliceInlineScalar(document, edit.Node, edit.NewValue); + } + + static string SpliceInlineScalar(string document, YamlScalarNode node, string newValue) + { + var (startColumn, endColumn) = InlineValueColumns(node); + var start = OffsetOfLine(document, (int)node.Start.Line) + startColumn; + var end = OffsetOfLine(document, (int)node.End.Line) + endColumn; + + return document[..start] + newValue + document[end..]; + } + + /// + /// Columns bounding the value itself, excluding any quotes so they are left in place. + /// + static (int startColumn, int endColumn) InlineValueColumns(YamlScalarNode node) + { + switch (node.Style) + { + case ScalarStyle.Plain: + return ((int)node.Start.Column - 1, (int)node.End.Column - 1); + case ScalarStyle.SingleQuoted: + case ScalarStyle.DoubleQuoted: + return ((int)node.Start.Column, (int)node.End.Column - 2); + default: + throw new NotSupportedException($"Replacing the value of a {node.Style} scalar is not supported."); + } + } + + /// + /// A block scalar starts at its indicator (| or |- …) and its content is the indented lines + /// that follow. End is column 1 of the line after the content, except when the block ends the + /// file without a trailing newline, where it is the end of the last content line instead. + /// + static string SpliceBlockScalar(string document, YamlScalarNode node, string newValue) + { + var contentStart = OffsetOfLine(document, (int)node.Start.Line + 1); + var contentEnd = OffsetOfLine(document, (int)node.End.Line) + (int)node.End.Column - 1; + var originalContent = document[contentStart..contentEnd]; + + var replacement = Reindent(newValue, BlockIndent(originalContent), document.DetectLineEnding() ?? "\n"); + if (!EndsWithLineBreak(originalContent)) + replacement = replacement.TrimEnd('\r', '\n'); + + return document[..contentStart] + replacement + document[contentEnd..]; + } + + static string Reindent(string value, string indent, string newLine) + { + var builder = new StringBuilder(); + foreach (var line in ContentLines(value)) + { + if (line.Length > 0) + builder.Append(indent); + builder.Append(line).Append(newLine); + } + + return builder.ToString(); + } + + /// + /// A clipped or kept block's value ends with a line break, which would otherwise yield a + /// trailing empty element that is not a content line of its own. + /// + static IEnumerable ContentLines(string value) + { + var lines = value.Split('\n'); + var count = lines.Length > 0 && lines[lines.Length - 1].Length == 0 + ? lines.Length - 1 + : lines.Length; + + return lines.Take(count).Select(line => line.TrimEnd('\r')); + } + + static string BlockIndent(string content) + { + var firstContentLine = content.Split('\n') + .FirstOrDefault(line => line.Trim('\r').Trim().Length > 0) + ?? ""; + + return firstContentLine[..(firstContentLine.Length - firstContentLine.TrimStart(' ', '\t').Length)]; + } + + static bool EndsWithLineBreak(string content) + { + return content.EndsWith("\n") || content.EndsWith("\r"); + } + + /// + /// Index of the first character of the given 1-based line, counting YAML line breaks + /// (\r\n, \n and \r). + /// + static int OffsetOfLine(string document, int line) + { + var currentLine = 1; + var index = 0; + while (currentLine < line && index < document.Length) + { + var character = document[index++]; + if (character == '\r' && index < document.Length && document[index] == '\n') + index++; + if (character == '\r' || character == '\n') + currentLine++; + } + + return index; + } + } +} diff --git a/source/Calamari/ArgoCD/YamlStreamLoader.cs b/source/Calamari/ArgoCD/YamlStreamLoader.cs index e7b5cc00d9..17b3835e1b 100644 --- a/source/Calamari/ArgoCD/YamlStreamLoader.cs +++ b/source/Calamari/ArgoCD/YamlStreamLoader.cs @@ -1,9 +1,6 @@ #nullable enable using System; -using System.Collections.Generic; using System.IO; -using System.Linq; -using Calamari.Common.Plumbing.Extensions; using Calamari.Common.Plumbing.Logging; using YamlDotNet.RepresentationModel; @@ -92,40 +89,5 @@ public static class YamlStreamLoader return stream.Documents[0].RootNode as YamlMappingNode; } - - /// - /// Serializes YAML documents back to string format, preserving line endings and handling document separators. - /// - /// The YAML documents to serialize - /// Original YAML content to detect line endings from (optional) - /// Serialized YAML string - public static string SerializeDocuments(IEnumerable documents, string? originalContent = null) - { - if (documents == null) - throw new ArgumentNullException(nameof(documents)); - - var documentList = documents.ToList(); - if (documentList.Count == 0) - return string.Empty; - - var newLine = originalContent?.DetectLineEnding() ?? "\n"; - var serializedDocs = documentList.Select(doc => SerializeDocument(doc, newLine)); - - var joined = string.Join($"{newLine}---{newLine}", serializedDocs); - return originalContent.HasTrailingNewLine() ? joined + newLine : joined; - } - - static string SerializeDocument(YamlDocument document, string newLine) - { - // The emitter always writes '\n' regardless of the writer's NewLine, so the document's - // own line ending has to be reapplied afterwards. - using var writer = new StringWriter(); - new YamlStream(document).Save(writer, false); - - var serialized = writer.ToString().TrimEnd().ReplaceLineEndings(newLine); - return serialized.EndsWith("...") - ? serialized.Substring(0, serialized.Length - 3).TrimEnd() - : serialized; - } } } \ No newline at end of file From b86ab308e212e87c97a107a80414aace0d5e4b49 Mon Sep 17 00:00:00 2001 From: Frank Lin Date: Tue, 15 Sep 2026 21:57:16 +1000 Subject: [PATCH 08/16] Refuse to splice scalars whose value cannot be located safely MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three defects in the splicing change, found by probing anchors, aliases and scalar styles: An alias resolves to the same node object, so a container referenced twice produced two edits for one scalar. The second splice used offsets the first had invalidated: a longer tag duplicated a fragment, and a shorter one truncated the rest of the file, writing broken YAML to the customer's repository. Edits are now deduplicated by node identity — by reference, since YamlScalarNode compares by value. A folded scalar threw NotSupportedException out of UpdateImages, turning a reformatted file into a failed deployment step. Callers now ask CanReplaceValue first and leave the file untouched with a warning. Start points at a scalar's anchor or tag rather than its value, so "image: &web nginx:1.21" spliced away the anchor and left the alias dangling. Node properties are now skipped, and the located region is verified against the value the parser reported before anything is replaced — so a position we cannot account for becomes a refusal instead of a corrupted file. Co-Authored-By: Claude Opus 5 (1M context) --- .../YamlJson6902PatchImageReplacerTests.cs | 39 +++++++ .../ArgoCD/YamlScalarSplicerTests.cs | 41 +++++++ .../InlineStrategicMergeImageReplacer.cs | 2 +- .../ArgoCD/InlineJsonPatchReplacer.cs | 8 ++ .../ArgoCD/YamlJson6902PatchImageReplacer.cs | 8 ++ source/Calamari/ArgoCD/YamlScalarSplicer.cs | 105 ++++++++++++++---- 6 files changed, 180 insertions(+), 23 deletions(-) diff --git a/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs b/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs index 2372801389..f97a2c5cd0 100644 --- a/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs +++ b/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs @@ -196,6 +196,45 @@ public void UpdateImages_WithInvalidYaml_ReturnsNoChange() result.UpdatedContents.Should().Be(invalidYaml); } + [Test] + public void UpdateImages_WithAnAliasedContainer_DoesNotCorruptTheDocument() + { + const string yamlContent = "- op: add\n" + + " path: /spec/template/spec/containers\n" + + " value:\n" + + " - &web\n" + + " name: a\n" + + " image: nginx:1.21\n" + + "- op: add\n" + + " path: /spec/template/spec/initContainers\n" + + " value:\n" + + " - *web\n"; + + var shorterTag = new List + { + new(ContainerImageReference.FromReferenceString("nginx:9", ArgoCDConstants.DefaultContainerRegistry)) + }; + + var replacer = new YamlJson6902PatchImageReplacer(yamlContent, ArgoCDConstants.DefaultContainerRegistry, log); + + var result = replacer.UpdateImages(shorterTag); + + result.UpdatedContents.Should().Be(yamlContent.Replace("nginx:1.21", "nginx:9")); + } + + [Test] + public void UpdateImages_WithAFoldedImageValue_LeavesTheFileAloneInsteadOfThrowing() + { + const string yamlContent = "- op: replace\n path: /spec/template/spec/containers/0/image\n value: >\n nginx:1.21\n"; + + var replacer = new YamlJson6902PatchImageReplacer(yamlContent, ArgoCDConstants.DefaultContainerRegistry, log); + + var result = replacer.UpdateImages(imagesToUpdate); + + result.UpdatedContents.Should().Be(yamlContent); + result.UpdatedImageReferences.Should().BeEmpty(); + } + [Test] public void UpdateImages_ChangesOnlyTheImageReference_PreservingCommentsAndLineEndings() { diff --git a/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs b/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs index c2dab38e88..b9cb959628 100644 --- a/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs +++ b/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs @@ -73,6 +73,47 @@ public void ReplaceValues_WithSeveralEdits_AppliesThemAll() result.Should().Be("a: nginx:1.25\r\nb: nginx:1.25\r\nc: nginx:1.25\r\n"); } + [Test] + public void ReplaceValues_WithTheSameNodeEditedTwice_AppliesItOnce() + { + // An alias makes YamlDotNet hand back the same node object, so callers can collect the + // same edit twice. Splicing it twice would corrupt the document. + const string document = "a: &x nginx:1.21\nb: *x\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var shared = (YamlScalarNode)root.Children[new YamlScalarNode("a")]; + var edits = new[] { new YamlScalarEdit(shared, "nginx:9"), new YamlScalarEdit(shared, "nginx:9") }; + + var result = YamlScalarSplicer.ReplaceValues(document, edits); + + result.Should().Be("a: &x nginx:9\nb: *x\n"); + } + + [Test] + public void CanReplaceValue_IsFalseForStylesThatCannotBeSpliced() + { + const string document = "a: >\n folded\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var node = (YamlScalarNode)root.Children[new YamlScalarNode("a")]; + + YamlScalarSplicer.CanReplaceValue(document, node).Should().BeFalse(); + } + + [Test] + public void CanReplaceValue_IsTrueForPlainQuotedAndLiteralScalars() + { + const string document = "a: plain\nb: \"quoted\"\nc: |\n block\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + + foreach (var key in new[] { "a", "b", "c" }) + { + var node = (YamlScalarNode)root.Children[new YamlScalarNode(key)]; + YamlScalarSplicer.CanReplaceValue(document, node).Should().BeTrue($"{key} should be spliceable"); + } + } + static string Replace(string document, string key, string newValue) { var root = (YamlMappingNode)Load(document).Documents[0].RootNode; diff --git a/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs b/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs index 8abfacb669..45f50e50dc 100644 --- a/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs +++ b/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs @@ -37,7 +37,7 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection(); foreach (var patchNode in patchSequence.Children) { - if (patchNode is YamlScalarNode patchScalar && patchScalar.Style == ScalarStyle.Literal) + if (patchNode is YamlScalarNode patchScalar && YamlScalarSplicer.CanReplaceValue(input, patchScalar)) { var patchContent = patchScalar.Value ?? ""; var replacer = new ContainerImageReplacer(patchContent, defaultRegistry); diff --git a/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs b/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs index 5a462a7de4..9d9acf7c93 100644 --- a/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs +++ b/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs @@ -127,6 +127,14 @@ HashSet ProcessInlinePatchContent(YamlScalarNode patchContentNode, IRead if (string.IsNullOrEmpty(patchContent)) return changes; + if (!YamlScalarSplicer.CanReplaceValue(yamlContent, patchContentNode)) + { + log.WarnFormat("Cannot safely update images in the inline patch at line {0} (a {1} scalar). Leaving it unchanged.", + patchContentNode.Start.Line, + patchContentNode.Style); + return changes; + } + IContainerImageReplacer patchImageReplacer; if (discovery.IsJson6902PatchContent(patchContent!)) { diff --git a/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs b/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs index cc7587fdff..dbe242bf0f 100644 --- a/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs +++ b/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs @@ -172,6 +172,14 @@ ImageReplacementResult ProcessImageReference(YamlScalarNode imageScalar, if (matchedUpdate != null && !matchedUpdate.Comparison.TagMatch) { + if (!YamlScalarSplicer.CanReplaceValue(yamlContent, imageScalar)) + { + log.WarnFormat("Cannot safely update the image reference at line {0} (a {1} scalar). Leaving it unchanged.", + imageScalar.Start.Line, + imageScalar.Style); + return NoChangeResult; + } + var newImageRef = currentImageRef.WithTag(matchedUpdate.Reference.Tag); edits.Add(new YamlScalarEdit(imageScalar, newImageRef.FriendlyName())); diff --git a/source/Calamari/ArgoCD/YamlScalarSplicer.cs b/source/Calamari/ArgoCD/YamlScalarSplicer.cs index b83430dd61..56ab03dd2b 100644 --- a/source/Calamari/ArgoCD/YamlScalarSplicer.cs +++ b/source/Calamari/ArgoCD/YamlScalarSplicer.cs @@ -24,15 +24,94 @@ public static string ReplaceValue(string document, YamlScalarNode node, string n return ReplaceValues(document, new[] { new YamlScalarEdit(node, newValue) }); } + /// + /// True when this scalar's value can be replaced by splicing. Callers MUST check before + /// reporting an image as updated: an unsupported style, or a position we cannot verify, + /// leaves the file untouched rather than corrupting it or failing the step. + /// + public static bool CanReplaceValue(string document, YamlScalarNode node) + { + if (node.Style == ScalarStyle.Literal) + return node.End.Line > node.Start.Line; + + return TryGetInlineValueRegion(document, node, out _, out _); + } + + /// + /// Locates the value inside an inline scalar and confirms the located text is exactly what the + /// parser reported as the value. Start can point at an anchor or tag rather than the value + /// itself (a: &x nginx:1.21), and quoted values can carry escapes, so the region is only + /// safe to replace once it has been checked against the value. + /// + static bool TryGetInlineValueRegion(string document, YamlScalarNode node, out int start, out int end) + { + start = 0; + end = 0; + + if (node.Start.Line != node.End.Line) + return false; + + var lineStart = OffsetOfLine(document, (int)node.Start.Line); + var scalarStart = lineStart + (int)node.Start.Column - 1; + + switch (node.Style) + { + case ScalarStyle.Plain: + start = SkipNodeProperties(document, scalarStart); + end = lineStart + (int)node.End.Column - 1; + break; + case ScalarStyle.SingleQuoted: + case ScalarStyle.DoubleQuoted: + start = SkipNodeProperties(document, scalarStart) + 1; + end = lineStart + (int)node.End.Column - 2; + break; + default: + return false; + } + + return start >= 0 + && end >= start + && end <= document.Length + && document[start..end] == node.Value; + } + + /// + /// Skips any anchor (&name) and tag (!tag) properties preceding the value. + /// + static int SkipNodeProperties(string document, int index) + { + while (index < document.Length && (document[index] == '&' || document[index] == '!')) + { + while (index < document.Length && !char.IsWhiteSpace(document[index])) + index++; + while (index < document.Length && (document[index] == ' ' || document[index] == '\t')) + index++; + } + + return index; + } + public static string ReplaceValues(string document, IEnumerable edits) { + // An alias resolves to the same node object, so the same edit can be collected more than + // once. Splicing it twice would apply the second edit at offsets the first has invalidated, + // so keep one edit per node — by reference, since YamlScalarNode compares by value. + var distinct = edits.Distinct(new NodeIdentityComparer()); + // Applying last-to-first keeps the offsets of the remaining, earlier edits valid. - var ordered = edits.OrderByDescending(e => e.Node.Start.Line) - .ThenByDescending(e => e.Node.Start.Column); + var ordered = distinct.OrderByDescending(e => e.Node.Start.Line) + .ThenByDescending(e => e.Node.Start.Column); return ordered.Aggregate(document, Splice); } + class NodeIdentityComparer : IEqualityComparer + { + public bool Equals(YamlScalarEdit? x, YamlScalarEdit? y) => ReferenceEquals(x?.Node, y?.Node); + + public int GetHashCode(YamlScalarEdit edit) => System.Runtime.CompilerServices.RuntimeHelpers.GetHashCode(edit.Node); + } + static string Splice(string document, YamlScalarEdit edit) { return edit.Node.Style == ScalarStyle.Literal @@ -42,30 +121,12 @@ static string Splice(string document, YamlScalarEdit edit) static string SpliceInlineScalar(string document, YamlScalarNode node, string newValue) { - var (startColumn, endColumn) = InlineValueColumns(node); - var start = OffsetOfLine(document, (int)node.Start.Line) + startColumn; - var end = OffsetOfLine(document, (int)node.End.Line) + endColumn; + if (!TryGetInlineValueRegion(document, node, out var start, out var end)) + throw new NotSupportedException($"Cannot locate the value of this {node.Style} scalar to replace it. Check CanReplaceValue before creating an edit."); return document[..start] + newValue + document[end..]; } - /// - /// Columns bounding the value itself, excluding any quotes so they are left in place. - /// - static (int startColumn, int endColumn) InlineValueColumns(YamlScalarNode node) - { - switch (node.Style) - { - case ScalarStyle.Plain: - return ((int)node.Start.Column - 1, (int)node.End.Column - 1); - case ScalarStyle.SingleQuoted: - case ScalarStyle.DoubleQuoted: - return ((int)node.Start.Column, (int)node.End.Column - 2); - default: - throw new NotSupportedException($"Replacing the value of a {node.Style} scalar is not supported."); - } - } - /// /// A block scalar starts at its indicator (| or |- …) and its content is the indented lines /// that follow. End is column 1 of the line after the content, except when the block ends the From 01ef6714385a3c3ad485e74a5d28648b090d9656 Mon Sep 17 00:00:00 2001 From: Frank Lin Date: Tue, 15 Sep 2026 22:08:06 +1000 Subject: [PATCH 09/16] Add formatting invariance tests for the image replacers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Generates 800 cases across the formatting a customer's file might use — LF and CRLF, with and without a trailing newline, leading and inline comments, blank lines inside block scalars, | and |- indicators, plain, single- and double-quoted values, anchors, and an alias that makes two operations share one scalar — and asserts the only thing a replacer may ever do is replace the image reference. Each case runs with tags shorter than, equal to and longer than the original, because an equal-length replacement masks a splice applied at stale offsets. Verified the suite actually fails by reverting each fix in turn: dropping the edit deduplication fails 8 cases, breaking the trailing-newline handling fails 288, and breaking the anchor handling fails 12. Breaking the anchor arithmetic while leaving the region verification in place fails nothing on the corruption assertions and everything on the must-actually-update assertions, which is the intended behaviour — the verification turns a position we cannot account for into a refusal rather than a corrupted file. Co-Authored-By: Claude Opus 5 (1M context) --- .../ArgoCD/ImageReplacerInvarianceTests.cs | 242 ++++++++++++++++++ 1 file changed, 242 insertions(+) create mode 100644 source/Calamari.Tests/ArgoCD/ImageReplacerInvarianceTests.cs diff --git a/source/Calamari.Tests/ArgoCD/ImageReplacerInvarianceTests.cs b/source/Calamari.Tests/ArgoCD/ImageReplacerInvarianceTests.cs new file mode 100644 index 0000000000..bbd96843e7 --- /dev/null +++ b/source/Calamari.Tests/ArgoCD/ImageReplacerInvarianceTests.cs @@ -0,0 +1,242 @@ +using System.Collections.Generic; +using System.Linq; +using Calamari.ArgoCD; +using Calamari.ArgoCD.Conventions; +using Calamari.ArgoCD.Conventions.UpdateImageTag; +using Calamari.ArgoCD.Models; +using Calamari.Common.Plumbing.Logging; +using Calamari.Testing.Helpers; +using FluentAssertions; +using NUnit.Framework; + +namespace Calamari.Tests.ArgoCD +{ + /// + /// Systematically varies the formatting a customer's file might use and asserts the only thing a + /// replacer may ever do is replace the image reference. Anything else — reformatting, a lost + /// anchor, a duplicated or truncated fragment — fails here, whatever the cause. + /// + [TestFixture] + public class ImageReplacerInvarianceTests + { + const string OldImage = "nginx:1.21"; + readonly ILog log = new InMemoryLog(); + + [TestCaseSource(nameof(KustomizationVariants))] + public void InlineJsonPatch_OnlyEverReplacesTheImageReference(Variant variant) + { + AssertOnlyTheImageMayChange(variant, + (content, images) => new InlineJsonPatchReplacer(content, ArgoCDConstants.DefaultContainerRegistry, log).UpdateImages(images)); + } + + [TestCaseSource(nameof(StrategicMergeVariants))] + public void InlineStrategicMerge_OnlyEverReplacesTheImageReference(Variant variant) + { + AssertOnlyTheImageMayChange(variant, + (content, images) => new InlineStrategicMergeImageReplacer(content, ArgoCDConstants.DefaultContainerRegistry, log).UpdateImages(images)); + } + + [TestCaseSource(nameof(Json6902Variants))] + public void Json6902Patch_OnlyEverReplacesTheImageReference(Variant variant) + { + AssertOnlyTheImageMayChange(variant, + (content, images) => new YamlJson6902PatchImageReplacer(content, ArgoCDConstants.DefaultContainerRegistry, log).UpdateImages(images)); + } + + [TestCaseSource(nameof(KustomizationVariants))] + public void InlineJsonPatch_WithNoMatchingImage_ReturnsTheFileUnchanged(Variant variant) + { + var noMatch = Images("redis:7.0"); + + var result = new InlineJsonPatchReplacer(variant.Content, ArgoCDConstants.DefaultContainerRegistry, log).UpdateImages(noMatch); + + result.UpdatedContents.Should().Be(variant.Content); + result.UpdatedImageReferences.Should().BeEmpty(); + } + + void AssertOnlyTheImageMayChange(Variant variant, System.Func, ImageReplacementResult> replace) + { + YamlStreamLoader.TryLoadSilent(variant.Content).Should().NotBeNull($"the generated variant {variant.Name} should be valid YAML"); + + foreach (var newTag in new[] { "nginx:9", "nginx:1.25", "nginx:1.25-alpine-with-a-long-suffix" }) + { + var result = replace(variant.Content, Images(newTag)); + + var unchanged = variant.Content; + var surgicallyChanged = variant.Content.Replace(OldImage, newTag); + + if (variant.ExpectUpdate) + { + // Asserting the exact result, not "unchanged or changed", so that a regression to + // silently declining every update is caught rather than passing as a safe no-op. + result.UpdatedContents.Should() + .Be(surgicallyChanged, $"{variant.Name} with {newTag} must change only the image reference"); + } + else + { + result.UpdatedContents.Should() + .BeOneOf(new[] { unchanged, surgicallyChanged }, + $"{variant.Name} with {newTag} must either decline to change the file or change only the image reference"); + } + + YamlStreamLoader.TryLoadSilent(result.UpdatedContents) + .Should() + .NotBeNull($"{variant.Name} with {newTag} must still be valid YAML"); + } + } + + static List Images(string image) + { + return new List + { + new(ContainerImageReference.FromReferenceString(image, ArgoCDConstants.DefaultContainerRegistry)) + }; + } + + public record Variant(string Name, string Content, bool ExpectUpdate) + { + public override string ToString() => Name; + } + + static IEnumerable KustomizationVariants() + { + foreach (var shape in Shapes()) + { + var body = new List + { + "apiVersion: kustomize.config.k8s.io/v1beta1", + "kind: Kustomization", + "patches:", + " - target:", + $" kind: Deployment{shape.InlineComment}", + $" patch: {shape.BlockIndicator}", + }; + body.AddRange(PatchLines(shape)); + + yield return Build($"kustomization[{shape.Name}]", shape, body); + } + } + + static IEnumerable StrategicMergeVariants() + { + foreach (var shape in Shapes()) + { + var body = new List + { + "apiVersion: kustomize.config.k8s.io/v1beta1", + "kind: Kustomization", + "patchesStrategicMerge:", + $" - {shape.BlockIndicator}", + }; + body.AddRange(PatchLines(shape, blockIndent: " ")); + + yield return Build($"strategicMerge[{shape.Name}]", shape, body); + } + } + + static IEnumerable Json6902Variants() + { + foreach (var quote in new[] { "", "\"", "'" }) + foreach (var anchor in new[] { "", "&img " }) + foreach (var newLine in new[] { "\n", "\r\n" }) + foreach (var trailing in new[] { true, false }) + { + var name = $"6902[quote={(quote == "" ? "none" : quote)},anchor={(anchor == "" ? "no" : "yes")},nl={(newLine == "\n" ? "LF" : "CRLF")},trailingNl={trailing}]"; + var body = new List + { + "# rollout patch", + "- op: replace", + " path: /spec/template/spec/containers/0/image", + $" value: {anchor}{quote}{OldImage}{quote}", + }; + + var content = string.Join(newLine, body) + (trailing ? newLine : ""); + yield return new Variant(name, content, ExpectUpdate: true); + } + + foreach (var aliased in AliasSharingVariants()) + yield return aliased; + } + + /// + /// An alias resolves to the same node object, so the same scalar gets visited — and edited — + /// more than once. Equal-length tags can hide a splice applied at stale offsets, so the + /// invariance assertions deliberately try tags shorter and longer than the original. + /// + static IEnumerable AliasSharingVariants() + { + foreach (var newLine in new[] { "\n", "\r\n" }) + foreach (var quote in new[] { "", "\"" }) + foreach (var trailing in new[] { true, false }) + { + var name = $"6902-alias[nl={(newLine == "\n" ? "LF" : "CRLF")},quote={(quote == "" ? "none" : quote)},trailingNl={trailing}]"; + var body = new List + { + "- op: add", + " path: /spec/template/spec/containers", + " value:", + " - &web", + " name: nginx", + $" image: {quote}{OldImage}{quote}", + "- op: add", + " path: /spec/template/spec/initContainers", + " value:", + " - *web", + }; + + var content = string.Join(newLine, body) + (trailing ? newLine : ""); + yield return new Variant(name, content, ExpectUpdate: true); + } + } + + static IEnumerable PatchLines(Shape shape, string blockIndent = " ") + { + yield return $"{blockIndent}apiVersion: apps/v1"; + yield return $"{blockIndent}kind: Deployment"; + if (shape.BlankLineInBlock) + yield return ""; + yield return $"{blockIndent}spec:"; + yield return $"{blockIndent} template:"; + yield return $"{blockIndent} spec:"; + yield return $"{blockIndent} containers:"; + yield return $"{blockIndent} - name: nginx{shape.InlineComment}"; + yield return $"{blockIndent} image: {shape.Quote}{OldImage}{shape.Quote}"; + } + + static Variant Build(string name, Shape shape, List body) + { + var lines = new List(); + if (shape.LeadingComment) + lines.Add("# managed by the platform team"); + lines.AddRange(body); + + var content = string.Join(shape.NewLine, lines) + (shape.TrailingNewLine ? shape.NewLine : ""); + return new Variant($"{name}", content, ExpectUpdate: true); + } + + record Shape(string Name, string NewLine, bool TrailingNewLine, bool LeadingComment, string InlineComment, bool BlankLineInBlock, string BlockIndicator, string Quote); + + static IEnumerable Shapes() + { + foreach (var newLine in new[] { "\n", "\r\n" }) + foreach (var trailing in new[] { true, false }) + foreach (var leadingComment in new[] { true, false }) + foreach (var inlineComment in new[] { "", " # the web tier" }) + foreach (var blankInBlock in new[] { true, false }) + foreach (var indicator in new[] { "|", "|-" }) + foreach (var quote in new[] { "", "\"", "'" }) + { + var name = string.Join(",", + newLine == "\n" ? "LF" : "CRLF", + $"trailingNl={trailing}", + $"leadComment={leadingComment}", + $"inlineComment={inlineComment != ""}", + $"blankInBlock={blankInBlock}", + $"block={indicator}", + $"quote={(quote == "" ? "none" : quote)}"); + + yield return new Shape(name, newLine, trailing, leadingComment, inlineComment, blankInBlock, indicator, quote); + } + } + } +} From a66e76ca76bd5d1f5e8e78f6821de070e33ca08b Mon Sep 17 00:00:00 2001 From: Frank Lin Date: Wed, 16 Sep 2026 07:07:44 +1000 Subject: [PATCH 10/16] Verify block scalar indentation before splicing it CanReplaceValue only checked that a block scalar spanned more than one line, so the refusal guarantee covered inline scalars alone. An explicit indent indicator makes part of the indentation content rather than structure, and BlockIndent reads the whole leading run, so reindenting doubled it: replacing a |2 block's value with itself turned six spaces into ten and silently corrupted the file. Locating a block's content now renders the parser's own value back and requires it to reproduce the original bytes. A block whose indentation we cannot describe is refused like any other unsupported scalar, and the splice reuses the same renderer, so what is verified is exactly what gets written. Co-Authored-By: Claude Opus 5 (1M context) --- .../ArgoCD/YamlScalarSplicerTests.cs | 50 ++++++++++++++++++ source/Calamari/ArgoCD/YamlScalarSplicer.cs | 52 +++++++++++++++---- 2 files changed, 92 insertions(+), 10 deletions(-) diff --git a/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs b/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs index b9cb959628..1569f0b806 100644 --- a/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs +++ b/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs @@ -114,6 +114,56 @@ public void CanReplaceValue_IsTrueForPlainQuotedAndLiteralScalars() } } + [Test] + public void CanReplaceValue_IsFalseWhenABlockIndentIndicatorMakesIndentationContent() + { + // |2 fixes the block indent at 2, so the remaining 4 spaces on each line are part of the + // value. Reindenting would prepend all 6 again and double the indentation. + const string document = "a: |2\n one\n two\nb: keep\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var node = (YamlScalarNode)root.Children[new YamlScalarNode("a")]; + + YamlScalarSplicer.CanReplaceValue(document, node).Should().BeFalse(); + } + + [Test] + public void CanReplaceValue_IsTrueForBlockIndicatorsWhoseIndentationWeCanAccountFor() + { + foreach (var indicator in new[] { "|", "|-", "|+", "|2" }) + { + const string indent = " "; + var document = $"a: {indicator}\n{indent}one\n{indent}two\nb: keep\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var node = (YamlScalarNode)root.Children[new YamlScalarNode("a")]; + + YamlScalarSplicer.CanReplaceValue(document, node) + .Should() + .BeTrue($"{indicator} with matching indentation should be spliceable"); + } + } + + [Test] + public void ReplaceValue_ReplacingABlockValueWithItself_IsAByteForByteNoOp() + { + foreach (var indicator in new[] { "|", "|-", "|+" }) + foreach (var newLine in new[] { "\n", "\r\n" }) + { + var document = $"a: {indicator}{newLine} one{newLine}{newLine} two{newLine}b: keep{newLine}"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var node = (YamlScalarNode)root.Children[new YamlScalarNode("a")]; + + if (!YamlScalarSplicer.CanReplaceValue(document, node)) + continue; + + YamlScalarSplicer.ReplaceValue(document, node, node.Value) + .Should() + .Be(document, $"{indicator} should round-trip unchanged"); + } + } + static string Replace(string document, string key, string newValue) { var root = (YamlMappingNode)Load(document).Documents[0].RootNode; diff --git a/source/Calamari/ArgoCD/YamlScalarSplicer.cs b/source/Calamari/ArgoCD/YamlScalarSplicer.cs index 56ab03dd2b..3a091e594a 100644 --- a/source/Calamari/ArgoCD/YamlScalarSplicer.cs +++ b/source/Calamari/ArgoCD/YamlScalarSplicer.cs @@ -31,10 +31,33 @@ public static string ReplaceValue(string document, YamlScalarNode node, string n /// public static bool CanReplaceValue(string document, YamlScalarNode node) { - if (node.Style == ScalarStyle.Literal) - return node.End.Line > node.Start.Line; + return node.Style == ScalarStyle.Literal + ? TryGetBlockContentRegion(document, node, out _, out _) + : TryGetInlineValueRegion(document, node, out _, out _); + } + + /// + /// Locates a block scalar's indented content and confirms that rendering the parser's own + /// value back reproduces the original bytes exactly. When it does not — an explicit indent + /// indicator (|2) makes part of the indentation content, so reindenting would double it — + /// this block is not one we can describe, and replacing it would corrupt the file. + /// + static bool TryGetBlockContentRegion(string document, YamlScalarNode node, out int start, out int end) + { + start = 0; + end = 0; + + if (node.End.Line <= node.Start.Line) + return false; - return TryGetInlineValueRegion(document, node, out _, out _); + start = OffsetOfLine(document, (int)node.Start.Line + 1); + end = OffsetOfLine(document, (int)node.End.Line) + (int)node.End.Column - 1; + + if (end < start || end > document.Length) + return false; + + var region = document[start..end]; + return RenderBlockContent(node.Value ?? "", region, document) == region; } /// @@ -134,15 +157,24 @@ static string SpliceInlineScalar(string document, YamlScalarNode node, string ne /// static string SpliceBlockScalar(string document, YamlScalarNode node, string newValue) { - var contentStart = OffsetOfLine(document, (int)node.Start.Line + 1); - var contentEnd = OffsetOfLine(document, (int)node.End.Line) + (int)node.End.Column - 1; - var originalContent = document[contentStart..contentEnd]; + if (!TryGetBlockContentRegion(document, node, out var start, out var end)) + throw new NotSupportedException("Cannot account for this block scalar's indentation to replace it. Check CanReplaceValue before creating an edit."); - var replacement = Reindent(newValue, BlockIndent(originalContent), document.DetectLineEnding() ?? "\n"); - if (!EndsWithLineBreak(originalContent)) - replacement = replacement.TrimEnd('\r', '\n'); + var replacement = RenderBlockContent(newValue, document[start..end], document); + + return document[..start] + replacement + document[end..]; + } + + /// + /// Renders a block scalar's value as it must appear in the file: indented to match the block, + /// using the document's line ending, and keeping the original region's trailing break or lack + /// of one. + /// + static string RenderBlockContent(string value, string originalRegion, string document) + { + var rendered = Reindent(value, BlockIndent(originalRegion), document.DetectLineEnding() ?? "\n"); - return document[..contentStart] + replacement + document[contentEnd..]; + return EndsWithLineBreak(originalRegion) ? rendered : rendered.TrimEnd('\r', '\n'); } static string Reindent(string value, string indent, string newLine) From 93877a55c0742458df56d9d4d13d725517821c35 Mon Sep 17 00:00:00 2001 From: Frank Lin Date: Wed, 16 Sep 2026 07:17:20 +1000 Subject: [PATCH 11/16] Harmonise a block scalar's line endings instead of refusing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A block whose own line endings differed from the rest of the file failed the round-trip check and was left unupdated. Comparing the round-trip with line endings normalised lets the block be rewritten with the document's ending instead, so the image is updated and only that block's endings change — the rest of the file stays byte-identical. Indentation differences are unaffected by the normalisation, so the |2 case this check exists for is still refused, including when the file's endings are mixed. Co-Authored-By: Claude Opus 5 (1M context) --- .../ArgoCD/YamlScalarSplicerTests.cs | 29 +++++++++++++++++++ source/Calamari/ArgoCD/YamlScalarSplicer.cs | 9 ++++-- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs b/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs index 1569f0b806..55ab8742ea 100644 --- a/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs +++ b/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs @@ -164,6 +164,35 @@ public void ReplaceValue_ReplacingABlockValueWithItself_IsAByteForByteNoOp() } } + [Test] + public void ReplaceValue_OnBlockWithLineEndingsDifferingFromTheFile_HarmonisesOnlyThatBlock() + { + // A CRLF file whose block content uses LF. The block is rewritten with the file's CRLF; + // everything outside it stays byte-identical. + const string document = "before: x\r\npatch: |-\r\n kind: Deployment\n image: nginx:1.21\r\nafter: y\r\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var node = (YamlScalarNode)root.Children[new YamlScalarNode("patch")]; + + YamlScalarSplicer.CanReplaceValue(document, node).Should().BeTrue(); + + var result = YamlScalarSplicer.ReplaceValue(document, node, "kind: Deployment\nimage: nginx:1.25"); + + result.Should().Be("before: x\r\npatch: |-\r\n kind: Deployment\r\n image: nginx:1.25\r\nafter: y\r\n"); + } + + [Test] + public void CanReplaceValue_StillRefusesIndentationItCannotAccountFor_WhenLineEndingsAreMixed() + { + // The |2 indentation problem must not be masked by ignoring line endings. + const string document = "a: |2\r\n one\n two\r\nb: keep\r\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var node = (YamlScalarNode)root.Children[new YamlScalarNode("a")]; + + YamlScalarSplicer.CanReplaceValue(document, node).Should().BeFalse(); + } + static string Replace(string document, string key, string newValue) { var root = (YamlMappingNode)Load(document).Documents[0].RootNode; diff --git a/source/Calamari/ArgoCD/YamlScalarSplicer.cs b/source/Calamari/ArgoCD/YamlScalarSplicer.cs index 3a091e594a..2a20052aa0 100644 --- a/source/Calamari/ArgoCD/YamlScalarSplicer.cs +++ b/source/Calamari/ArgoCD/YamlScalarSplicer.cs @@ -38,9 +38,12 @@ public static bool CanReplaceValue(string document, YamlScalarNode node) /// /// Locates a block scalar's indented content and confirms that rendering the parser's own - /// value back reproduces the original bytes exactly. When it does not — an explicit indent + /// value back reproduces the original content. When it does not — an explicit indent /// indicator (|2) makes part of the indentation content, so reindenting would double it — /// this block is not one we can describe, and replacing it would corrupt the file. + /// Line endings are excluded from the comparison: a block whose own endings differ from the + /// rest of the file is harmonised to the document's ending rather than refused, which still + /// catches every indentation difference the check exists for. /// static bool TryGetBlockContentRegion(string document, YamlScalarNode node, out int start, out int end) { @@ -57,7 +60,9 @@ static bool TryGetBlockContentRegion(string document, YamlScalarNode node, out i return false; var region = document[start..end]; - return RenderBlockContent(node.Value ?? "", region, document) == region; + var rendered = RenderBlockContent(node.Value ?? "", region, document); + + return rendered.ReplaceLineEndings("\n") == region.ReplaceLineEndings("\n"); } /// From a6e5ed055834a5c29f9bdd7412cad1d0a0ac57f5 Mon Sep 17 00:00:00 2001 From: Frank Lin Date: Wed, 16 Sep 2026 07:26:44 +1000 Subject: [PATCH 12/16] Derive a block scalar's structural indentation instead of guessing it BlockIndent treated a content line's whole leading whitespace as structure. That holds when YAML takes the indent from the first non-empty line, but an explicit indicator (|2) declares it and leaves any surplus as part of the string, so reindenting doubled the surplus. Such blocks were refused. The structural amount is now the difference between the raw line and the line the parser returned, which covers both cases without interpreting the indicator, so |2 blocks with surplus indentation are updated correctly rather than declined. The indent is derived from the original value and then applied to the new one, and the round-trip check still confirms the result before anything is written. Co-Authored-By: Claude Opus 5 (1M context) --- .../ArgoCD/YamlScalarSplicerTests.cs | 38 ++++++-- source/Calamari/ArgoCD/YamlScalarSplicer.cs | 93 ++++++++++++------- 2 files changed, 90 insertions(+), 41 deletions(-) diff --git a/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs b/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs index 55ab8742ea..1d0712f476 100644 --- a/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs +++ b/source/Calamari.Tests/ArgoCD/YamlScalarSplicerTests.cs @@ -115,16 +115,21 @@ public void CanReplaceValue_IsTrueForPlainQuotedAndLiteralScalars() } [Test] - public void CanReplaceValue_IsFalseWhenABlockIndentIndicatorMakesIndentationContent() + public void ReplaceValue_OnBlockWithAnIndentIndicatorAndSurplusIndentation_KeepsTheSurplus() { - // |2 fixes the block indent at 2, so the remaining 4 spaces on each line are part of the - // value. Reindenting would prepend all 6 again and double the indentation. + // |2 declares two spaces of structure, so the remaining four on each line are part of the + // value and must survive the replacement rather than being indented a second time. const string document = "a: |2\n one\n two\nb: keep\n"; var root = (YamlMappingNode)Load(document).Documents[0].RootNode; var node = (YamlScalarNode)root.Children[new YamlScalarNode("a")]; - YamlScalarSplicer.CanReplaceValue(document, node).Should().BeFalse(); + node.Value.Should().Be(" one\n two\n"); + YamlScalarSplicer.CanReplaceValue(document, node).Should().BeTrue(); + + var result = YamlScalarSplicer.ReplaceValue(document, node, " one\n three\n"); + + result.Should().Be("a: |2\n one\n three\nb: keep\n"); } [Test] @@ -182,15 +187,34 @@ public void ReplaceValue_OnBlockWithLineEndingsDifferingFromTheFile_HarmonisesOn } [Test] - public void CanReplaceValue_StillRefusesIndentationItCannotAccountFor_WhenLineEndingsAreMixed() + public void ReplaceValue_OnBlockWithAnIndentIndicatorAndMixedLineEndings_StillKeepsTheSurplus() { - // The |2 indentation problem must not be masked by ignoring line endings. const string document = "a: |2\r\n one\n two\r\nb: keep\r\n"; var root = (YamlMappingNode)Load(document).Documents[0].RootNode; var node = (YamlScalarNode)root.Children[new YamlScalarNode("a")]; - YamlScalarSplicer.CanReplaceValue(document, node).Should().BeFalse(); + YamlScalarSplicer.CanReplaceValue(document, node).Should().BeTrue(); + + var result = YamlScalarSplicer.ReplaceValue(document, node, node.Value); + + result.Should().Be("a: |2\r\n one\r\n two\r\nb: keep\r\n"); + } + + [Test] + public void ReplaceValue_ReplacingAnIndentIndicatorBlockWithItself_IsAByteForByteNoOp() + { + foreach (var surplus in new[] { "", " ", " " }) + { + var document = $"a: |2\n {surplus}one\n {surplus}two\nb: keep\n"; + + var root = (YamlMappingNode)Load(document).Documents[0].RootNode; + var node = (YamlScalarNode)root.Children[new YamlScalarNode("a")]; + + YamlScalarSplicer.ReplaceValue(document, node, node.Value) + .Should() + .Be(document, $"surplus of {surplus.Length} spaces should round-trip"); + } } static string Replace(string document, string key, string newValue) diff --git a/source/Calamari/ArgoCD/YamlScalarSplicer.cs b/source/Calamari/ArgoCD/YamlScalarSplicer.cs index 2a20052aa0..f624844ea5 100644 --- a/source/Calamari/ArgoCD/YamlScalarSplicer.cs +++ b/source/Calamari/ArgoCD/YamlScalarSplicer.cs @@ -32,39 +32,44 @@ public static string ReplaceValue(string document, YamlScalarNode node, string n public static bool CanReplaceValue(string document, YamlScalarNode node) { return node.Style == ScalarStyle.Literal - ? TryGetBlockContentRegion(document, node, out _, out _) + ? TryGetBlockRegion(document, node) != null : TryGetInlineValueRegion(document, node, out _, out _); } /// - /// Locates a block scalar's indented content and confirms that rendering the parser's own - /// value back reproduces the original content. When it does not — an explicit indent - /// indicator (|2) makes part of the indentation content, so reindenting would double it — - /// this block is not one we can describe, and replacing it would corrupt the file. - /// Line endings are excluded from the comparison: a block whose own endings differ from the - /// rest of the file is harmonised to the document's ending rather than refused, which still - /// catches every indentation difference the check exists for. + /// Locates a block scalar's indented content and works out how much of the leading whitespace + /// is structure. Rendering the parser's own value back must then reproduce the original + /// content, so a block we cannot describe is refused rather than corrupted. Line endings are + /// excluded from that comparison: a block whose own endings differ from the rest of the file is + /// harmonised to the document's ending, which still catches indentation differences. /// - static bool TryGetBlockContentRegion(string document, YamlScalarNode node, out int start, out int end) + static BlockRegion? TryGetBlockRegion(string document, YamlScalarNode node) { - start = 0; - end = 0; - if (node.End.Line <= node.Start.Line) - return false; + return null; - start = OffsetOfLine(document, (int)node.Start.Line + 1); - end = OffsetOfLine(document, (int)node.End.Line) + (int)node.End.Column - 1; + var start = OffsetOfLine(document, (int)node.Start.Line + 1); + var end = OffsetOfLine(document, (int)node.End.Line) + (int)node.End.Column - 1; if (end < start || end > document.Length) - return false; + return null; var region = document[start..end]; - var rendered = RenderBlockContent(node.Value ?? "", region, document); + var value = node.Value ?? ""; - return rendered.ReplaceLineEndings("\n") == region.ReplaceLineEndings("\n"); + var indent = StructuralIndent(region, value); + if (indent == null) + return null; + + var block = new BlockRegion(start, end, indent, EndsWithLineBreak(region)); + + return Render(value, block, document).ReplaceLineEndings("\n") == region.ReplaceLineEndings("\n") + ? block + : null; } + record BlockRegion(int Start, int End, string Indent, bool EndsWithBreak); + /// /// Locates the value inside an inline scalar and confirms the located text is exactly what the /// parser reported as the value. Start can point at an anchor or tag rather than the value @@ -162,24 +167,21 @@ static string SpliceInlineScalar(string document, YamlScalarNode node, string ne /// static string SpliceBlockScalar(string document, YamlScalarNode node, string newValue) { - if (!TryGetBlockContentRegion(document, node, out var start, out var end)) - throw new NotSupportedException("Cannot account for this block scalar's indentation to replace it. Check CanReplaceValue before creating an edit."); - - var replacement = RenderBlockContent(newValue, document[start..end], document); + var block = TryGetBlockRegion(document, node) + ?? throw new NotSupportedException("Cannot account for this block scalar's indentation to replace it. Check CanReplaceValue before creating an edit."); - return document[..start] + replacement + document[end..]; + return document[..block.Start] + Render(newValue, block, document) + document[block.End..]; } /// - /// Renders a block scalar's value as it must appear in the file: indented to match the block, - /// using the document's line ending, and keeping the original region's trailing break or lack - /// of one. + /// Renders a value as a block scalar's content: indented to match the block, using the + /// document's line ending, and keeping the original region's trailing break or lack of one. /// - static string RenderBlockContent(string value, string originalRegion, string document) + static string Render(string value, BlockRegion block, string document) { - var rendered = Reindent(value, BlockIndent(originalRegion), document.DetectLineEnding() ?? "\n"); + var rendered = Reindent(value, block.Indent, document.DetectLineEnding() ?? "\n"); - return EndsWithLineBreak(originalRegion) ? rendered : rendered.TrimEnd('\r', '\n'); + return block.EndsWithBreak ? rendered : rendered.TrimEnd('\r', '\n'); } static string Reindent(string value, string indent, string newLine) @@ -209,13 +211,36 @@ static IEnumerable ContentLines(string value) return lines.Take(count).Select(line => line.TrimEnd('\r')); } - static string BlockIndent(string content) + /// + /// The portion of a content line's leading whitespace that is structure rather than part of the + /// string. YAML normally takes it from the first non-empty line, but an explicit indicator + /// (|2) declares it, leaving any surplus as content. Deriving it from the difference between + /// the raw line and the parsed line covers both without having to interpret the indicator. + /// Returns null when the two do not correspond, so the caller refuses the block. + /// + static string? StructuralIndent(string region, string value) { - var firstContentLine = content.Split('\n') - .FirstOrDefault(line => line.Trim('\r').Trim().Length > 0) - ?? ""; + var regionLines = ContentLines(region).ToList(); + var valueLines = ContentLines(value).ToList(); + + for (var index = 0; index < valueLines.Count; index++) + { + if (valueLines[index].Length == 0) + continue; + + if (index >= regionLines.Count) + return null; + + var indentLength = regionLines[index].Length - valueLines[index].Length; + if (indentLength < 0) + return null; + + var indent = regionLines[index][..indentLength]; + + return indent.All(character => character == ' ' || character == '\t') ? indent : null; + } - return firstContentLine[..(firstContentLine.Length - firstContentLine.TrimStart(' ', '\t').Length)]; + return ""; } static bool EndsWithLineBreak(string content) From 4293600e3adad0570e5038cee76abcdebeb03dd1 Mon Sep 17 00:00:00 2001 From: Frank Lin Date: Wed, 16 Sep 2026 07:40:19 +1000 Subject: [PATCH 13/16] Warn about an unwritable patch only when it holds an image to update The inline patch replacer checked whether it could write a patch back before working out whether the patch contained anything to change, so any folded patch body produced a warning on every deployment even when the images being updated were nowhere near it. The check now runs once the inner replacer reports a change, so a patch we cannot write back but have no reason to touch stays silent. The updated image references are collected after the check too, so a skipped patch is no longer reported as updated. The other two paths were already correct: the 6902 replacer only examines scalars at image paths and checks inside the branch that found a tag to change, and the strategic merge replacer skips patches it cannot handle without logging, as it did before. Co-Authored-By: Claude Opus 5 (1M context) --- .../ArgoCD/InlineJsonPatchReplacerTests.cs | 40 +++++++++++++++++++ .../YamlJson6902PatchImageReplacerTests.cs | 21 ++++++++++ .../ArgoCD/InlineJsonPatchReplacer.cs | 23 ++++++----- 3 files changed, 73 insertions(+), 11 deletions(-) diff --git a/source/Calamari.Tests/ArgoCD/InlineJsonPatchReplacerTests.cs b/source/Calamari.Tests/ArgoCD/InlineJsonPatchReplacerTests.cs index a3a0d51b19..ddd52afc60 100644 --- a/source/Calamari.Tests/ArgoCD/InlineJsonPatchReplacerTests.cs +++ b/source/Calamari.Tests/ArgoCD/InlineJsonPatchReplacerTests.cs @@ -21,6 +21,46 @@ public class InlineJsonPatchReplacerTests ILog log = new InMemoryLog(); + [Test] + public void UpdateImages_WithAFoldedPatchThatHasNoRelevantImage_DoesNotWarn() + { + var inMemoryLog = new InMemoryLog(); + const string inputYaml = "patches:\n" + + " - patch: >\n" + + " some: folded prose that mentions no image at all\n" + + " - target:\n" + + " kind: Deployment\n" + + " patch: |-\n" + + " apiVersion: apps/v1\n" + + " kind: Deployment\n" + + " spec:\n" + + " template:\n" + + " spec:\n" + + " containers:\n" + + " - name: nginx\n" + + " image: nginx:1.21\n"; + + var result = new InlineJsonPatchReplacer(inputYaml, ArgoCDConstants.DefaultContainerRegistry, inMemoryLog).UpdateImages(imagesToUpdate); + + result.UpdatedContents.Should().Be(inputYaml.Replace("nginx:1.21", "nginx:1.25")); + inMemoryLog.MessagesWarnFormatted.Should().BeEmpty(); + } + + [Test] + public void UpdateImages_WithAFoldedPatchThatDoesHoldTheImage_WarnsAndLeavesItAlone() + { + var inMemoryLog = new InMemoryLog(); + const string inputYaml = "patches:\n" + + " - patch: >\n" + + " [{\"op\": \"replace\", \"path\": \"/spec/template/spec/containers/0/image\", \"value\": \"nginx:1.21\"}]\n"; + + var result = new InlineJsonPatchReplacer(inputYaml, ArgoCDConstants.DefaultContainerRegistry, inMemoryLog).UpdateImages(imagesToUpdate); + + result.UpdatedContents.Should().Be(inputYaml); + result.UpdatedImageReferences.Should().BeEmpty(); + inMemoryLog.MessagesWarnFormatted.Should().ContainMatch("*inline patch at line*Folded*"); + } + [Test] public void UpdateImages_ChangesOnlyTheImageReference_PreservingCommentsAndLineEndings() { diff --git a/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs b/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs index f97a2c5cd0..5532cac0a9 100644 --- a/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs +++ b/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs @@ -235,6 +235,27 @@ public void UpdateImages_WithAFoldedImageValue_LeavesTheFileAloneInsteadOfThrowi result.UpdatedImageReferences.Should().BeEmpty(); } + [Test] + public void UpdateImages_WithAFoldedScalarElsewhereInTheFile_DoesNotWarn() + { + var inMemoryLog = new InMemoryLog(); + const string yamlContent = "- op: add\n" + + " path: /metadata/annotations/notes\n" + + " value: >\n" + + " some folded prose\n" + + " spanning lines\n" + + "- op: replace\n" + + " path: /spec/template/spec/containers/0/image\n" + + " value: nginx:1.21\n"; + + var replacer = new YamlJson6902PatchImageReplacer(yamlContent, ArgoCDConstants.DefaultContainerRegistry, inMemoryLog); + + var result = replacer.UpdateImages(imagesToUpdate); + + result.UpdatedContents.Should().Be(yamlContent.Replace("nginx:1.21", "nginx:1.25")); + inMemoryLog.MessagesWarnFormatted.Should().BeEmpty(); + } + [Test] public void UpdateImages_ChangesOnlyTheImageReference_PreservingCommentsAndLineEndings() { diff --git a/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs b/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs index 9d9acf7c93..e816b1b2f9 100644 --- a/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs +++ b/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs @@ -127,14 +127,6 @@ HashSet ProcessInlinePatchContent(YamlScalarNode patchContentNode, IRead if (string.IsNullOrEmpty(patchContent)) return changes; - if (!YamlScalarSplicer.CanReplaceValue(yamlContent, patchContentNode)) - { - log.WarnFormat("Cannot safely update images in the inline patch at line {0} (a {1} scalar). Leaving it unchanged.", - patchContentNode.Start.Line, - patchContentNode.Style); - return changes; - } - IContainerImageReplacer patchImageReplacer; if (discovery.IsJson6902PatchContent(patchContent!)) { @@ -146,12 +138,21 @@ HashSet ProcessInlinePatchContent(YamlScalarNode patchContentNode, IRead } var result = patchImageReplacer.UpdateImages(imagesToUpdate); - changes.UnionWith(result.UpdatedImageReferences); + if (result.UpdatedImageReferences.Count == 0) + return changes; - if (result.UpdatedImageReferences.Count > 0) + // Checked only once there is something to write, so a patch we cannot write back but + // have no reason to touch stays silent. + if (!YamlScalarSplicer.CanReplaceValue(yamlContent, patchContentNode)) { - edits.Add(new YamlScalarEdit(patchContentNode, result.UpdatedContents)); + log.WarnFormat("Cannot safely update images in the inline patch at line {0} (a {1} scalar). Leaving it unchanged.", + patchContentNode.Start.Line, + patchContentNode.Style); + return changes; } + + changes.UnionWith(result.UpdatedImageReferences); + edits.Add(new YamlScalarEdit(patchContentNode, result.UpdatedContents)); } catch (Exception ex) { From 9dac037ed5071b7070aede17ae449eaacccfc6f9 Mon Sep 17 00:00:00 2001 From: Frank Lin Date: Wed, 16 Sep 2026 07:57:37 +1000 Subject: [PATCH 14/16] Fail loudly with an actionable message when a value cannot be updated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A value we can find an image in but cannot write back was handled three different ways: the Helm path threw an internal message telling the reader to "check CanReplaceValue", the inline patch and 6902 paths logged a warning and carried on, reporting success while the image stayed at the old tag. All four sites now raise a CommandException naming what could not be updated, the line it is on, why, and what to change. The shared explanation lives in DescribeUnsupportedValue so the wording stays consistent, and the inline patch replacer rethrows CommandException ahead of its own catch, which would otherwise have turned the failure back into a warning. Two fixes found while doing it. The 6902 message interpolated the scalar's value, which for a folded scalar carries the trailing line break that made it unwritable and would have split the message across lines. And the strategic merge replacer had been widened from a literal-block check to CanReplaceValue, which would have treated a plain entry — a path to a patch file — as inline patch content; it identifies inline patches by literal block again. Co-Authored-By: Claude Opus 5 (1M context) --- .../ArgoCD/Helm/HelmYamlParserTests.cs | 15 +++++++++++++++ .../ArgoCD/InlineJsonPatchReplacerTests.cs | 16 +++++++++------- .../YamlJson6902PatchImageReplacerTests.cs | 11 +++++++---- .../InlineStrategicMergeImageReplacer.cs | 8 +++++++- source/Calamari/ArgoCD/Helm/HelmYamlParser.cs | 12 +++++++----- .../Calamari/ArgoCD/InlineJsonPatchReplacer.cs | 12 ++++++------ .../ArgoCD/YamlJson6902PatchImageReplacer.cs | 10 ++++------ source/Calamari/ArgoCD/YamlScalarSplicer.cs | 11 +++++++++++ 8 files changed, 66 insertions(+), 29 deletions(-) diff --git a/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs b/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs index 5ba552469f..85a42b666a 100644 --- a/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs +++ b/source/Calamari.Tests/ArgoCD/Helm/HelmYamlParserTests.cs @@ -1,5 +1,6 @@ using System; using Calamari.ArgoCD.Helm; +using Calamari.Common.Commands; using FluentAssertions; using NUnit.Framework; @@ -163,6 +164,20 @@ public void UpdateNodeValue_WithUnchangedPath_ReturnsContentByteForByte() result.Should().Be(yamlContent); } + [Test] + public void UpdateNodeValue_WithAFoldedValue_FailsWithAMessageNamingThePathAndTheFix() + { + const string yamlContent = "image:\n tag: >\n 1.21\n"; + + var sut = new HelmYamlParser(yamlContent); + + var act = () => sut.UpdateContentForPath("image.tag", "1.25"); + + act.Should() + .Throw() + .WithMessage("*image.tag*line 2*folded block scalar (>)*literal block (|)*"); + } + [Test] public void CreateDotPathsForNodes_WithExistingDotNotationKeys_IgnoresThoseKeys() { diff --git a/source/Calamari.Tests/ArgoCD/InlineJsonPatchReplacerTests.cs b/source/Calamari.Tests/ArgoCD/InlineJsonPatchReplacerTests.cs index ddd52afc60..5e31a1a74d 100644 --- a/source/Calamari.Tests/ArgoCD/InlineJsonPatchReplacerTests.cs +++ b/source/Calamari.Tests/ArgoCD/InlineJsonPatchReplacerTests.cs @@ -3,6 +3,7 @@ using Calamari.ArgoCD; using Calamari.ArgoCD.Conventions; using Calamari.ArgoCD.Models; +using Calamari.Common.Commands; using Calamari.Common.Plumbing.Logging; using Calamari.Testing.Helpers; using FluentAssertions; @@ -22,7 +23,7 @@ public class InlineJsonPatchReplacerTests ILog log = new InMemoryLog(); [Test] - public void UpdateImages_WithAFoldedPatchThatHasNoRelevantImage_DoesNotWarn() + public void UpdateImages_WithAFoldedPatchThatHasNoRelevantImage_DoesNotFail() { var inMemoryLog = new InMemoryLog(); const string inputYaml = "patches:\n" @@ -47,18 +48,19 @@ public void UpdateImages_WithAFoldedPatchThatHasNoRelevantImage_DoesNotWarn() } [Test] - public void UpdateImages_WithAFoldedPatchThatDoesHoldTheImage_WarnsAndLeavesItAlone() + public void UpdateImages_WithAFoldedPatchThatDoesHoldTheImage_FailsWithAnActionableMessage() { - var inMemoryLog = new InMemoryLog(); const string inputYaml = "patches:\n" + " - patch: >\n" + " [{\"op\": \"replace\", \"path\": \"/spec/template/spec/containers/0/image\", \"value\": \"nginx:1.21\"}]\n"; - var result = new InlineJsonPatchReplacer(inputYaml, ArgoCDConstants.DefaultContainerRegistry, inMemoryLog).UpdateImages(imagesToUpdate); + var replacer = new InlineJsonPatchReplacer(inputYaml, ArgoCDConstants.DefaultContainerRegistry, log); - result.UpdatedContents.Should().Be(inputYaml); - result.UpdatedImageReferences.Should().BeEmpty(); - inMemoryLog.MessagesWarnFormatted.Should().ContainMatch("*inline patch at line*Folded*"); + var act = () => replacer.UpdateImages(imagesToUpdate); + + act.Should() + .Throw() + .WithMessage("*inline patch on line 2*folded block scalar (>)*literal block (|)*"); } [Test] diff --git a/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs b/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs index 5532cac0a9..710202c27c 100644 --- a/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs +++ b/source/Calamari.Tests/ArgoCD/YamlJson6902PatchImageReplacerTests.cs @@ -1,8 +1,10 @@ +using System; using System.Collections.Generic; using System.Linq; using Calamari.ArgoCD; using Calamari.ArgoCD.Conventions; using Calamari.ArgoCD.Models; +using Calamari.Common.Commands; using Calamari.Common.Plumbing.Logging; using Calamari.Testing.Helpers; using FluentAssertions; @@ -223,16 +225,17 @@ public void UpdateImages_WithAnAliasedContainer_DoesNotCorruptTheDocument() } [Test] - public void UpdateImages_WithAFoldedImageValue_LeavesTheFileAloneInsteadOfThrowing() + public void UpdateImages_WithAFoldedImageValue_FailsWithAMessageNamingTheImageAndTheFix() { const string yamlContent = "- op: replace\n path: /spec/template/spec/containers/0/image\n value: >\n nginx:1.21\n"; var replacer = new YamlJson6902PatchImageReplacer(yamlContent, ArgoCDConstants.DefaultContainerRegistry, log); - var result = replacer.UpdateImages(imagesToUpdate); + var act = () => replacer.UpdateImages(imagesToUpdate); - result.UpdatedContents.Should().Be(yamlContent); - result.UpdatedImageReferences.Should().BeEmpty(); + act.Should() + .Throw() + .WithMessage("*nginx:1.21*line 3*folded block scalar (>)*literal block (|)*"); } [Test] diff --git a/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs b/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs index 45f50e50dc..0f7aa19b9c 100644 --- a/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs +++ b/source/Calamari/ArgoCD/Conventions/UpdateImageTag/InlineStrategicMergeImageReplacer.cs @@ -1,6 +1,7 @@ using System.Collections.Generic; using System.Linq; using Calamari.ArgoCD.Models; +using Calamari.Common.Commands; using Calamari.Common.Plumbing.Logging; using YamlDotNet.Core; using YamlDotNet.RepresentationModel; @@ -37,7 +38,9 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection(); foreach (var patchNode in patchSequence.Children) { - if (patchNode is YamlScalarNode patchScalar && YamlScalarSplicer.CanReplaceValue(input, patchScalar)) + // A literal block is inline patch content; a plain entry is a path to a patch file, which + // is not ours to rewrite. + if (patchNode is YamlScalarNode patchScalar && patchScalar.Style == ScalarStyle.Literal) { var patchContent = patchScalar.Value ?? ""; var replacer = new ContainerImageReplacer(patchContent, defaultRegistry); @@ -45,6 +48,9 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection 0) { + if (!YamlScalarSplicer.CanReplaceValue(input, patchScalar)) + throw new CommandException($"Cannot update images in the strategic merge patch on line {patchScalar.Start.Line}: {YamlScalarSplicer.DescribeUnsupportedValue(patchScalar)}."); + edits.Add(new YamlScalarEdit(patchScalar, result.UpdatedContents)); allUpdatedImages.UnionWith(result.UpdatedImageReferences); } diff --git a/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs b/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs index 2d2799a0ee..833fae3291 100644 --- a/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs +++ b/source/Calamari/ArgoCD/Helm/HelmYamlParser.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.IO; using System.Linq; +using Calamari.Common.Commands; using YamlDotNet.RepresentationModel; using YamlDotNet.Serialization; @@ -71,12 +72,13 @@ public List CreateDotPathsForNodes() public string UpdateContentForPath(string path, string newValue) { var nodeAtPath = GetNodeAtPath(path); - if (nodeAtPath != null) - { - return YamlScalarSplicer.ReplaceValue(yamlString, nodeAtPath, newValue); - } + if (nodeAtPath == null) + return yamlString; + + if (!YamlScalarSplicer.CanReplaceValue(yamlString, nodeAtPath)) + throw new CommandException($"Cannot update the value at '{path}' on line {nodeAtPath.Start.Line}: {YamlScalarSplicer.DescribeUnsupportedValue(nodeAtPath)}."); - return yamlString; + return YamlScalarSplicer.ReplaceValue(yamlString, nodeAtPath, newValue); } static void FlattenObject(object? obj, string currentPath, List paths) diff --git a/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs b/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs index e816b1b2f9..2b5e20478b 100644 --- a/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs +++ b/source/Calamari/ArgoCD/InlineJsonPatchReplacer.cs @@ -6,6 +6,7 @@ using Calamari.ArgoCD.Conventions; using Calamari.ArgoCD.Conventions.UpdateImageTag; using Calamari.ArgoCD.Models; +using Calamari.Common.Commands; using Calamari.Common.Plumbing.Extensions; using Calamari.Common.Plumbing.Logging; using Calamari.Kubernetes; @@ -144,16 +145,15 @@ HashSet ProcessInlinePatchContent(YamlScalarNode patchContentNode, IRead // Checked only once there is something to write, so a patch we cannot write back but // have no reason to touch stays silent. if (!YamlScalarSplicer.CanReplaceValue(yamlContent, patchContentNode)) - { - log.WarnFormat("Cannot safely update images in the inline patch at line {0} (a {1} scalar). Leaving it unchanged.", - patchContentNode.Start.Line, - patchContentNode.Style); - return changes; - } + throw new CommandException($"Cannot update images in the inline patch on line {patchContentNode.Start.Line}: {YamlScalarSplicer.DescribeUnsupportedValue(patchContentNode)}."); changes.UnionWith(result.UpdatedImageReferences); edits.Add(new YamlScalarEdit(patchContentNode, result.UpdatedContents)); } + catch (CommandException) + { + throw; + } catch (Exception ex) { log.WarnFormat("Error processing inline patch content: {0}", ex.Message); diff --git a/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs b/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs index dbe242bf0f..9ecba1fb25 100644 --- a/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs +++ b/source/Calamari/ArgoCD/YamlJson6902PatchImageReplacer.cs @@ -4,6 +4,7 @@ using System.Linq; using Calamari.ArgoCD.Conventions; using Calamari.ArgoCD.Models; +using Calamari.Common.Commands; using Calamari.Common.Plumbing.Logging; using YamlDotNet.RepresentationModel; @@ -173,12 +174,9 @@ ImageReplacementResult ProcessImageReference(YamlScalarNode imageScalar, if (matchedUpdate != null && !matchedUpdate.Comparison.TagMatch) { if (!YamlScalarSplicer.CanReplaceValue(yamlContent, imageScalar)) - { - log.WarnFormat("Cannot safely update the image reference at line {0} (a {1} scalar). Leaving it unchanged.", - imageScalar.Start.Line, - imageScalar.Style); - return NoChangeResult; - } + // Trimmed because a folded value carries the line break that made it unwritable, + // which would otherwise break the message across lines. + throw new CommandException($"Cannot update the image reference '{imageScalar.Value?.Trim()}' on line {imageScalar.Start.Line}: {YamlScalarSplicer.DescribeUnsupportedValue(imageScalar)}."); var newImageRef = currentImageRef.WithTag(matchedUpdate.Reference.Tag); edits.Add(new YamlScalarEdit(imageScalar, newImageRef.FriendlyName())); diff --git a/source/Calamari/ArgoCD/YamlScalarSplicer.cs b/source/Calamari/ArgoCD/YamlScalarSplicer.cs index f624844ea5..535d4ff1ce 100644 --- a/source/Calamari/ArgoCD/YamlScalarSplicer.cs +++ b/source/Calamari/ArgoCD/YamlScalarSplicer.cs @@ -19,6 +19,17 @@ public record YamlScalarEdit(YamlScalarNode Node, string NewValue); /// public static class YamlScalarSplicer { + /// + /// Why a value cannot be replaced, phrased for the person whose file it is. Callers add their + /// own context (which image, which file) and raise it as a CommandException. + /// + public static string DescribeUnsupportedValue(YamlScalarNode node) + { + return node.Style == ScalarStyle.Folded + ? "it is written as a folded block scalar (>), which does not record where the original line breaks were. Updating it would collapse the block onto one line and reformat the file. Use a literal block (|) or a plain value instead" + : $"the layout of this {node.Style} scalar could not be interpreted, so updating it could corrupt the file"; + } + public static string ReplaceValue(string document, YamlScalarNode node, string newValue) { return ReplaceValues(document, new[] { new YamlScalarEdit(node, newValue) }); From 1c367756bc5ef86e259849992fab5ff188fbd6e0 Mon Sep 17 00:00:00 2001 From: Frank Lin Date: Wed, 16 Sep 2026 11:04:32 +1000 Subject: [PATCH 15/16] Let deliberate failures escape the JSON patch replacer Its catch-all turned every exception into a warning and returned no change, so a CommandException raised while updating an image would have been reported as a successful step that left the tag alone. It now rethrows CommandException ahead of that handler, matching the inline patch replacer. The remaining catch-all handlers in this area sit around file discovery and YAML parsing and call no replacer, so none of them can swallow a deliberate failure. Co-Authored-By: Claude Opus 5 (1M context) --- source/Calamari/ArgoCD/JsonPatchImageReplacer.cs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/source/Calamari/ArgoCD/JsonPatchImageReplacer.cs b/source/Calamari/ArgoCD/JsonPatchImageReplacer.cs index b9365eb3ab..2acc7740d9 100644 --- a/source/Calamari/ArgoCD/JsonPatchImageReplacer.cs +++ b/source/Calamari/ArgoCD/JsonPatchImageReplacer.cs @@ -6,6 +6,7 @@ using System.Text.Json.Nodes; using Calamari.ArgoCD.Conventions; using Calamari.ArgoCD.Models; +using Calamari.Common.Commands; using Calamari.Common.Plumbing.Logging; namespace Calamari.ArgoCD @@ -101,6 +102,12 @@ public ImageReplacementResult UpdateImages(IReadOnlyCollection()); } + catch (CommandException) + { + // A failure we raised deliberately, with an actionable message. Degrading it to a + // warning here would report success while leaving the image at its old tag. + throw; + } catch (Exception ex) { log.WarnFormat("Error processing JSON patch file: {0}", ex.Message); From 803d2f38408b93f907bcd32f3412311330dcba48 Mon Sep 17 00:00:00 2001 From: Frank Lin Date: Thu, 17 Sep 2026 08:13:46 +1000 Subject: [PATCH 16/16] Use HasTrailingNewLine in the splicer The splicer carried its own copy of the trailing-newline check, which would have left the extracted extension with no callers once the earlier line-ending handling was replaced by splicing. Co-Authored-By: Claude Opus 5 (1M context) --- source/Calamari/ArgoCD/YamlScalarSplicer.cs | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/source/Calamari/ArgoCD/YamlScalarSplicer.cs b/source/Calamari/ArgoCD/YamlScalarSplicer.cs index 535d4ff1ce..51f3fe1aa3 100644 --- a/source/Calamari/ArgoCD/YamlScalarSplicer.cs +++ b/source/Calamari/ArgoCD/YamlScalarSplicer.cs @@ -72,7 +72,7 @@ public static bool CanReplaceValue(string document, YamlScalarNode node) if (indent == null) return null; - var block = new BlockRegion(start, end, indent, EndsWithLineBreak(region)); + var block = new BlockRegion(start, end, indent, region.HasTrailingNewLine()); return Render(value, block, document).ReplaceLineEndings("\n") == region.ReplaceLineEndings("\n") ? block @@ -254,11 +254,6 @@ static IEnumerable ContentLines(string value) return ""; } - static bool EndsWithLineBreak(string content) - { - return content.EndsWith("\n") || content.EndsWith("\r"); - } - /// /// Index of the first character of the given 1-based line, counting YAML line breaks /// (\r\n, \n and \r).