Conversation
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
flin-8
force-pushed
the
franklin/sf-3581-argocd-image-tag-line-endings
branch
from
September 16, 2026 22:13
8a0f3d1 to
25c1ed5
Compare
flin-8
changed the title
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
flin-8
force-pushed
the
franklin/sf-3581-argocd-image-tag-line-endings
branch
from
September 16, 2026 22:55
25c1ed5 to
cda9788
Compare
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) <noreply@anthropic.com>
(cherry picked from commit edee954)
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
flin-8
force-pushed
the
franklin/sf-3581-argocd-image-tag-line-endings
branch
from
September 16, 2026 23:43
cda9788 to
803d2f3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #2162 for SF-3581. #2162 is the minimal fix onto
release/2026.3; this targetsmainand contains it, plus the rest of the problem.#2162 stops line endings and the trailing newline being rewritten. It does not stop inline patches in
kustomization.yamlbeing re-emitted, which still drops comments and blank lines and reflows indentation and quoting.YamlScalarSplicerreplaces a scalar's value in the original text instead, so every byte outside it is unchanged. It handles plain, quoted and literal block scalars, deriving a block's structural indentation rather than assuming the whole leading run. A value whose position cannot be verified is refused rather than written.Also fixes three defects found while building it, all present in shipped code:
Startpoints at its anchor, soimage: &web nginx:1.21spliced the anchor away and left the alias dangling....document-end marker to every 6902 patch file it touched, and discarded all but the first document.A value we can find an image in but cannot write back now fails the step with a message naming the line and the fix, instead of the previous mix of an internal exception in the Helm path and silent warnings elsewhere.
800 generated cases assert the only thing a replacer may ever do is replace the image reference, across line endings, trailing newlines, comments, blank lines, block indicators, quoting, anchors and aliases. Each fix was checked by reverting it and confirming the suite fails.
🤖 Generated with Claude Code