Repository navigation
Release 0.6.0: keep a concept's untouched bytes when editing it - #70
Merged
Merged
Conversation
leoafarias
marked this pull request as ready for review
October 22, 2026 19:05
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
A host that previews a change, such as a write pipeline that shows the diff before committing it, has to know how okf will spell a value: whether 2026-08-18, true or a: b is quoted. That rule lived in a private method, so a host could only copy it and drift. OkfYamlEmitter.scalar returns the text emit writes for a scalar value and for a mapping key, and its documentation states when a string stays plain and that every other string is written as its JSON string literal. It throws OkfYamlEncodeException for a collection. Tests pin the plain and the quoted spellings, read each back through package:yaml, and check that emit writes key: plus the same text.
Update, link and deprecate changes re-emitted the whole concept through the canonical emitter, so a one-key update turned flow collections into block ones, moved keys such as tags into the preferred order, dropped comments and rewrote CRLF files with LF. A host that shows the diff of a change, such as a write pipeline that previews it, saw every line move. OkfDocument now keeps the text it was parsed from as sourceText, and rewriteSource edits that text to a requested mapping and body, finding each entry by package:yaml's spans. An unchanged key keeps its bytes. A changed one-line scalar or flow collection is replaced by its span, so a comment after it stays; a changed block mapping is edited entry by entry, and a block sequence that grows at its end gets the new items after its last one, in that item's style; any other value is written after its key by the emitter. A removed key removes its own lines, and a new key goes directly after the last entry of its mapping. New lines take the document's line break. An unchanged mapping and body return the text byte for byte. Every result is parsed again and must read back as the requested mapping and body. If it does not, the edit is made again replacing each changed top-level value whole, and then the whole mapping is re-emitted between the existing delimiters. That re-emission is used outright for a flow-style mapping and for frontmatter with aliases, explicit keys or collection keys, whose spans cannot locate an entry. A value that cannot be written so that it reads back throws OkfYamlEncodeException, which the change set reports as an OkfBundleChangeException. The overlay edits existing concepts through rewriteSource and treats an edit that leaves the text as it is as no change. UTF-8 decoding drops a byte order mark, so the commit writes it back for a concept file whose source began with one. Tests cover flow collections, key order, quoting and multi-line strings, comments, CRLF and a byte order mark, nested mappings and sequences, removed and added keys, each fallback and a no-op; every fixture written in okf's own and hand-written styles keeps its untouched lines under every single-key edit; the applier tests check the bytes on disk; and the portable entry's JavaScript run calls rewriteSource.
rewriteSource returned the text of a document without frontmatter unchecked when the requested mapping was empty, and a document without source text skipped the read-back the same way. A replacement body whose first line is `---` therefore came back as text that parses with a frontmatter block, or that does not parse at all, and the change set's parse of it threw OkfDocumentException out of prepare instead of an OkfBundleChangeException. Both paths now parse their result again and require it to read back without frontmatter and with the expected body. Otherwise rewriteSource throws OkfYamlEncodeException, which the change set reports as a tool error naming the concept. Adding a frontmatter block in front of a body-only document that begins with a byte order mark left the mark after the new block. The mark now stays at the start of the text, before the opening delimiter, when the document keeps its body; a replacement body is written as given, since a body-only document's body holds the mark. Tests cover a body that would read back as frontmatter, from parsed and constructed documents and through prepare, and the byte order mark.
emitOkfYamlBlockLines was a public top-level function in yaml_emitter.dart that only the frontmatter rewriter called, kept out of the API by a hide on okf.dart's export. Any other entry point exporting the library had to repeat the hide, and package:okf/src/ still reached it. yaml_emitter.dart also imported document.dart only so its dartdoc could link OkfDocument.rewriteSource, which made the two libraries import each other. The rewriter now gets a collection's block lines from the public emit: it emits the value under a single key and moves the lines after that key to the indentation it needs, which every emitted line starts with since emit writes each scalar on one line. The helper and the hide are gone, and the dartdoc link comes from a @DocImport.
A changed flow collection was replaced whole by the emitter's flow text,
so changing one entry respelled the rest: updating verified.at in
`verified: {by: "human:r", at: "2026"}` dropped the quotes on the
untouched `by`, and `{ by: x }` lost its spacing. A host previewing the
change saw noise on exactly the line it cared about.
A flow mapping that keeps every key now has each changed value replaced
by its span and each new entry inserted after the last value as
`, key: value`; a flow sequence that keeps its length or grows has each
changed item replaced and new items inserted after the last one. Nested
flow collections are edited the same way, and anything else in the
collection, spacing, line breaks and comments included, stays. A
collection that loses an entry, or whose value to replace is omitted or
spans lines, is still replaced whole, now always in flow style: the flow
text nests collections instead of giving up on them.
package:yaml's span of a plain scalar inside a flow collection runs on
over the whitespace and line breaks before the next `,` or bracket, so a
plain scalar's text is taken to end where its span's text stops before
trailing whitespace; one whose text spans lines is not located that way.
Tests cover changed, added and nested entries, spacing and a trailing
comment, a multi-line flow sequence, and the whole rewrite on removal
and for an omitted value; the applier's update test now keeps the flow
mapping's spacing.
A changed `description: |` block was rewritten after its key as one JSON string, `description: "one\nthree\n"`, so editing a multi-line field turned the whole block into a line a reviewer cannot read: the style change O2 exists to avoid. The in-place attempt now writes a changed literal or folded block scalar whose new value is a string with a line break as a literal block at the old block's indentation, keeping any comment on its header line. Its final line breaks pick `|-`, `|` or `|+`. A string that such a block does not hold exactly as written, one whose first line is empty or starts with whitespace, that has a line reading as a `---` delimiter or that holds a control character, U+2028, U+2029, U+FEFF or an unpaired surrogate, is written as before, and so is a string without a line break. The read-back check still guards the result; if it fails, the attempt that replaces whole top-level values writes the string as OkfYamlEmitter.scalar does, as it did before. Tests cover literal and folded blocks with each chomping, a header comment and a block scalar sequence item, and the strings that keep the scalar spelling.
The fixture test normalized line endings before checking that untouched lines survive, so it could not see an LF line leaking into the CRLF fixture under an add, remove or append. Only the in-place attempt's _NotInPlace route to the top-level fallback was tested, not a nested edit that is made but does not read back. And no applier test deprecated a concept without a status key, which is where a new key's position matters. The fixture test now requires every rewrite of a CRLF fixture to hold no bare LF. A new test removes the last key of a nested mapping after a keeping block scalar, whose nested edit reads back a line break short, and checks that the whole changed value is replaced while other keys and comments stay. An applier test deprecates a concept without status and expects the key directly after the last entry.
The rewriteSource dartdoc said the only in-place give-up was removing the first key of a `- key: value` item and named only explicit `?` keys among the cases that re-emit the whole mapping; the README omitted collection keys, and said the last fallback re-emits the mapping "in its key order", while it uses the emitter's preferred order and so moves keys such as tags. The README example's output also left out the body the call returns. The dartdoc now covers flow collections edited by span, literal blocks, the byte order mark of a body-only document and the refused `---` body, and lists the three attempts with every condition that skips one: a removal on a compact item's `-` line, a tagged or anchored block sequence, overlapping edits, a flow root, aliases, collection keys and explicit `?` keys. A tag or anchor before a compact item's key, as in `- !tag key: value`, is not among them: package:yaml starts the key's span at the tag, so the rewriter's check never sees it, which the check now says. sourceText notes that a parsed document keeps its text besides its body. The emitter's dartdoc says where rewriteSource departs from scalar. The README example updates a flow-style verified mapping and shows the whole result, the fallback paragraph names every give-up, and the CHANGELOG entry covers the new behaviour. ADR-0010 records the policy ADR-0003's "lossless round trip" left open: span edits, the three-step ladder checked by read-back, new keys after the last entry of their mapping rather than in the preferred order, and the no-op rule that compares mappings without regard to key order.
In-place concept edits change the bytes every update, link and deprecation writes, and they reverse the README's promise that formatting is semantic rather than byte-preserving, so they ship as a minor release that ^0.5.1 dependents opt into, not in 0.5.1. The pubspec, okfVersion and the README's action reference move to 0.6.0, and the CHANGELOG entries for the edits, sourceText, scalar and rewriteSource move out of 0.5.1 into a new 0.6.0 section. okfIndexGeneratorRevision stays 0.5.0: the generator's output is unchanged, as its golden test shows.
OkfDocument.rewriteSource and OkfYamlEmitter.scalar were public, which froze okf's layout rules and quoting as a contract that no consumer uses: Wayfinder and Stargate read the text a change writes from its candidate rather than predict it. rewriteSource moves to OkfDocumentSourceRewrite, an extension in the rewriter's library, which okf.dart does not export, and the scalar spelling becomes okfYamlScalar, which okf.dart hides the way okf_io.dart hides OkfBundleWriteTransaction. OkfDocument.sourceText stays public. Tests that the library exports neither, and the portable entry checks sourceText instead of the rewriter. The README describes the in-place edit rules under change sets, with a deprecation example, instead of as a rewriteSource API, and it, the CHANGELOG and ADR-0010 no longer say hosts can predict how a value is written.
Before in-place edits, every change-set write was canonical. With them, a new key went after the last entry of its mapping even in frontmatter okf format had written, so deprecating a concept put status after sources, and okf mcp's deprecate-concept, link-concepts and update-concept could make a bundle that passed okf format --check fail it. A filled tags: [] also stayed in flow style. When the frontmatter block holds exactly the lines the emitter writes for its values, line breaks aside, the rewriter now writes the block of a result that reads back in that form too. That keeps every unchanged line and nested key order, moves a new top-level key to its place in the preferred order and writes a filled empty collection as a block. Hand-written blocks keep the in-place rules. Tests cover exact canonical edits, a CRLF canonical block, a block one respelled line takes out of canonical form, every formatted fixture under each edit, and a bundle that stays clean under okf format --check through a deprecation, an update and a link. The README, CHANGELOG, ADR-0010 and OkfUpdateConceptChange state the rule.
In-place edits left the README describing them as one more change-set behaviour, while its compatibility principles still said formatting is semantic rather than byte-preserving, which now holds only for okf format and new concepts. The library section now names both policies before the edit rules: canonical output for okf format, serialize and new concepts, and in-place edits for update, link and deprecate changes and their MCP verbs. The compatibility principle says which writes it covers and that in-place edits keep a canonical concept canonical.
A concept file that began with a UTF-8 byte order mark kept it when an edit rewrote it, but an index.md or log.md lost it, whether the change set regenerated it or a projection replaced it: preparation retained the source bytes, where it finds the mark, only for concepts. Preparation now retains the bytes of the bundle's reserved files too, so one rule covers every file a commit rewrites: a file that began with the mark keeps it, and one that did not gains none. Candidate and projected texts stay decoded text without the mark. A test commits a creation with and without a projection over a marked index and log and reads the bytes and the reloaded texts back; prepare, the projection typedef, the README and the CHANGELOG state the rule.
An update, link or deprecation whose input held an unpaired surrogate failed wherever the rewrite first met it: the emitter's refusal for a frontmatter value, preparation's UTF-8 check of the finished text for a body, and, before the emitter refused such strings, the rewriter's read-back check, whose message named neither the surrogate nor the field. The overlay now checks every string an edit supplies, keys and nested values included, and the replacement body before the rewrite runs, and throws OkfBundleChangeException naming the field, a key or the body. A test sends the input through each rewrite path (a span, a literal block, a flow mapping, a block list, a new key, the body and a link's sources item) via prepare and apply, and checks the message and that nothing is written.
Every rewrite is parsed again and must read back as the whole requested mapping, which is what makes each layout rule and fallback safe. The tests checked read-back with equals, which takes 1 for 1.0, over a handful of values, and the only test of the final refusal now failed earlier, in the emitter, since the emitter refuses unpaired surrogates. A sameYamlData helper compares as the rewriter does: mappings in any key order, integers apart from doubles, NaN equal to NaN. The rewriter tests now apply over 4,000 edits, each fixture's keys, nested values and list items set to strings that read as other types or YAML syntax, line breaks, control characters, numbers and collections, and require each result to read back exactly. A mapping keyed by NaN, which no text reads back as, shows that every path throws rather than return such a text, and an applier test checks that an edit's candidate and the committed file read back as the requested values.
The boundary review asked whether package:yaml_edit could shrink the rewriter. A comparison over the rewriter's fixtures, with every scalar written in okf's spelling, matched on 2,379 of 2,460 edits. The rest replace flow collections and literal blocks with other styles, drop the comment and blank lines after a removed key, leave blank lines behind nested replacements, insert keys alphabetically into sorted mappings, and fail on aliases with an internal assertion error, while the document layer, the fallback ladder, the read-back check, the canonical rule and the no-op comparison would all stay. ADR-0010 now records the evaluation and why the rewriter stays.
Inserting the canonical rule and the surrogate sentence left the entry's lines ragged; this only reflows it.
The 0.6.0 entry says an edit whose input holds an unpaired surrogate is refused naming the input that holds it. A deprecation note goes only to the log, so its surrogate was still refused by the check on prepared texts, as "Bundle path log.md holds an unpaired surrogate". The input check now covers the note too and names it, and the CHANGELOG says so. An already deprecated concept stays a no-op, which writes nothing and ignores the note.
Loading drops one byte order mark that begins a file. The commit wrote a mark back only for a file that began with one, so a text that itself began with U+FEFF loaded as another text than the one validated: over a file without a mark, the U+FEFF was written as the mark and dropped on the next load. The docs also said projected texts carry no mark, which nothing enforced. The commit now writes a mark before every text that begins with U+FEFF too, which is then content after it, so every committed file loads as exactly the text validated. A projection that returns such a text over a file that began with a mark gets both, and a concept file that begins with two marks keeps both through an edit. The prepare and projection docs, the README and the CHANGELOG state the rule, and tests cover a projected text with and without the file's own mark.
The applier's class and commit docs said a commit writes exactly the bytes captured during preparation, and the candidate's docs called its concept texts the exact serialized bytes. Since a commit writes a byte order mark before a text where loading needs one to read it back, the candidate holds texts and the committed bytes are those texts in UTF-8, after that mark when the file takes one. The docs and the README now say so.
ADR-0003's header names ADR-0009 and ADR-0010 as amendments, and the index marks every amended ADR as its header does, but its 0003 row named only 0009. ADR-0010's Decision also said without condition that a new key goes after the last entry of its mapping, which the next bullet, canonical in, canonical out, contradicts for canonical frontmatter; the bullet now says it applies to frontmatter that is not canonical.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ec438e714
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+201
to
+206
| final mark = newBody == null && source.startsWith(_byteOrderMark) | ||
| ? _byteOrderMark | ||
| : ''; | ||
| final rest = newBody == null | ||
| ? source.substring(mark.length) | ||
| : layout.withLineEndings(newBody); |
There was a problem hiding this comment.
When a body-only document's decoded body genuinely starts with U+FEFF (for example, a file containing a UTF-8 BOM followed by another U+FEFF as content), an update that adds frontmatter without replacing the body treats that content character as a file mark, removes it from rest, and explicitly validates against expectedBody.substring(mark.length). The committed document therefore has a different body even though the update did not request a body change; retain this character in rest and validate against the complete original body.
Useful? React with 👍 / 👎.
Comment on lines
+133
to
+136
| retainedTextPaths: <String>{ | ||
| ...loaded.documents.keys, | ||
| ...loaded.indexes.keys, | ||
| ...loaded.logs.keys, |
There was a problem hiding this comment.
When a BOM-prefixed Markdown file fails parsing or UTF-8 decoding, the loader omits it from documents, indexes, and logs; consequently this snapshot does not retain its bytes. A create change can repair a malformed concept at the same path, and a projection can repair a malformed reserved file, but startsWithByteOrderMark(path) will then be false and the replacement silently loses the original BOM. Include replaced malformed paths when retaining source bytes so repairs honor the new BOM-preservation behavior.
Useful? React with 👍 / 👎.
Comment on lines
+416
to
+422
| return <Object?, Object?>{ | ||
| for (final entry in value.entries) | ||
| normalized(entry.key, depth + 1): normalized( | ||
| entry.value, | ||
| depth + 1, | ||
| ), | ||
| }; |
There was a problem hiding this comment.
When a requested nested mapping contains both a DateTime key and the equal ISO-8601 string key, these are distinct supported Dart keys, but this map comprehension normalizes both to the same string and silently overwrites one before the read-back invariant runs. An update can therefore commit a mapping missing one requested entry instead of refusing the unrepresentable input; detect duplicate normalized keys and throw OkfYamlEncodeException rather than constructing a lossy map.
Useful? React with 👍 / 👎.
kauandotnet
force-pushed
the
feat/o2-preserve-frontmatter
branch
from
October 22, 2026 19:28
9ec438e to
3c1b115
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
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
okf 0.6.0: editing a concept keeps the bytes of everything the edit does not touch. Today an update, link or deprecation re-emits the whole frontmatter, so flow-style lists turn into block lists, keys move and quoting changes, and a one-field edit shows up as a rewrite of the file. ADR-0010 records the decision.
What changed
index.mdandlog.md.okf formatwrites it), the edit keeps it canonical: a new key goes to its place in the preferred key order. An okf write never makes a canonical bundle failokf format --check.format, in-place edits for update, link and deprecate.OkfDocument.sourceTextis public. The rewriter and the scalar encoder stay package-internal.okfVersion, pubspec, README action reference) with its own CHANGELOG section.okfIndexGeneratorRevisionstays 0.5.0; the index generator is unchanged and its golden passes.Breaking changes
None in the API. Behaviour: edits no longer canonicalize a concept's frontmatter. Callers that relied on update/link/deprecate re-formatting a file should run
okf format.Tests and checks
dart test: 310 pass, including the portable entry compiled to JavaScript under node; analyze clean; publish dry run 0 warnings; changed files formatted.package:yaml_editwas evaluated as a way to shrink the rewriter and not adopted; ADR-0010 says why.Notes