Repository navigation
fix(reconcile): rewrite pins in place, not by re-serializing the document - #9
Merged
Merged
Conversation
…ment `charly box reconcile` is what the workflow rule names as the remedy for a `resolved to multiple versions` warning. Running it on a 160KB charly.yml changed the 4 pins it reported -- and 405 lines in total. Pass 2 mutated the parsed scalar and then yaml.Marshal'd the whole document, which round-trips every OTHER node through the emitter. yaml.v3 does not reproduce a FOLDED block scalar (`>-`): it re-emits the text as one long line and drops the blank lines around it. So every authored `description:` in the file was reflowed, across entities the reconcile had no reason to touch. A literal scalar (`|-`) survives, which is why the damage looks selective and went unnoticed. The effect is that the prescribed remedy is unusable: the diff it produces is itself a review blocker, and it silently reformats prose someone wrote deliberately. A pin is always a single-line scalar, so the rewrite is now line-local -- the original bytes are retained from pass 1 and only the lines carrying a changed ref are touched. Every other byte is preserved exactly. Where the node position and the bytes disagree it returns an error instead of replacing the value somewhere else; a blind fallback would corrupt the file it was asked to edit. Measured on the same file that exposed it, same 4 pins: before 2 files changed, 170 insertions(+), 235 deletions(-) after 2 files changed, 4 insertions(+), 4 deletions(-) Three guards. The fixture uses a FOLDED scalar because a literal one round-trips byte-exactly -- an earlier revision of this test used a literal, so its negative control SKIPPED and the suite proved nothing while passing. Claude-Session: https://claude.ai/code/session_016fnPmYiC6ZkqXSi6D8PAP6
Review — PASSHead SHA: Security screen: PASS — no scope mismatch, no secrets/exfiltration, no weakened guardrails, no supply-chain changes; the change does exactly what the description claims (line-local pin rewrite replacing whole-document re-serialization). Change class: plugin behaviour (Go code in Checklist: All applicable rules PASS or NA.
No comments in the thread to adjudicate. Verdict: PASS ollama/deepseek-v4-flash — pi-review-action. |
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
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.
Summary
The remedy the workflow rule prescribes for a version-mismatch warning is currently unusable.
charly box reconcilealigns cross-repo@githubpins so a repo resolves to one version — thegit-workflow rule names it as the fix for a
resolved to multiple versionswarning. Run on a160KB
charly.yml, it changed the 4 pins it reported and 405 lines in total.Pass 2 mutated the parsed scalar and then
yaml.Marshal'd the whole document, round-trippingevery other node through the emitter. yaml.v3 does not reproduce a folded block scalar
(
>-): it re-emits the body as one long line and drops the blank lines around it. Everyauthored
description:in the file was reflowed, across entities the reconcile had no reason totouch:
A literal scalar (
|-) round-trips byte-exactly, which is why the damage looks selective andwent unnoticed: files with only literal prose are unaffected.
The consequence is circular — the diff the remedy generates is itself a review blocker, and it
silently reformats prose someone wrote deliberately. So the warning it exists to clear cannot be
cleared with it.
What changed
A pin is always a single-line scalar, so the rewrite is now line-local: the original bytes
are retained from pass 1, and only the lines carrying a changed ref are touched. Every other byte
is preserved exactly.
Where the node position and the bytes disagree,
applyPinEditsreturns an error rather thanreplacing the value somewhere else — a blind fallback would corrupt the file it was asked to
edit.
Generated from
git diff --stat origin/main...HEAD.How tested
Executed live, on the file that exposed it. charly was built against this branch
(
go mod edit -replace …/plugin-box/candy/plugin-box, thentask build:binary; the binarycarries the new path —
strings bin/charly | grep 'reported at line .* outside the'→ 1 match),and
charly box reconcilewas run on the same 160KB project. It reported the same 4 pins asbefore the change:
and the after-diff is exactly the four pin rewrites, nothing else:
Three guards:
count is unchanged, so a dropped blank line fails it.
exists. It logs the damage (
647 bytes -> 610 bytes) and would skip itself if yaml.v3 everround-tripped faithfully, so it cannot silently become vacuous.
line all return an error and no output.
The fixture uses a FOLDED scalar deliberately. An earlier revision of this test used a
literal one; yaml.v3 round-tripped it byte-exactly, so the negative control SKIPPED and the
suite passed while proving nothing. That is recorded here because the skip was invisible in
go testoutput without-v— the failure mode this repo should not repeat.Rulebook compliance
and diffing, not by guessing; the literal-vs-folded distinction explains why the damage looked
selective.
rewrites) is unchanged.is removed. No retries, no formatting cleanup pass.
yaml.Marshalwrite path and its now-unusedfileChangedflag areremoved in this PR, not deprecated.
go vet+ full package tests on the committed tree, plus thechanged code compiled into charly and run against the 160KB file with the resulting diff
pasted above.
Change classification
reconcilewrites its result. The set of pins itselects, and its output, are unchanged.
go vet+go test ./...exit 0, three guards (one a negativecontrol), and a live
charly box reconcileon a real 160KB project reduced from 405 changedlines to 8.
fully tested and validated— the changed path executed inside a builtcharly against the file that exposed the defect, the before/after diffs are recorded, and the
guards fail without the change.
Assisted-by: Claude Code Anthropic Opus 5 (fully tested and validated)