kayg/calternal
Watch
1
0
Fork
You've already forked calternal
 0
Code Issues 190 Pull requests 61 Projects Releases Packages Wiki Activity Actions 26

notes: nested map edits in the property writer (calendar step 2) #781

Closed
kayg wants to merge 7 commits from feat/calendar-props-maps into dev
pull from: feat/calendar-props-maps
merge into: kayg:dev
Conversation 8 Commits 7 Files changed 6 +737 -14
kayg commented 2026-10-31 05:24:09 +00:00
Owner
Copy link

simplification for final review: accept only a local splice of simple target lines, with YAML reparse verification; refuse unusual target syntax rather than preserving it heuristically. corpus tests independently check untouched bytes. linear in frontmatter size, with no new persistent state.

Failure → effect → detection → recovery → test: unsafe input → explicit refusal with no note writes → boundary validation → edit the note or enable the completed UI contract later → awkward YAML / hidden syntax / stored replay / cross-folder transfer regressions. Gates: fmt, four-crate clippy with -D warnings, events/notes/tasks/calternal tests on dev-01.

Plan (ADR 0067 build order, step 2)

Add safe nested map edits to the existing property writer (crates/notes/src/props.rs). Steps 3 (event parser) and 4 (identity/promotion) stack on this branch. Needed later by events (journal map: marker -> fields) and source (connected resource map).

API

  • Change gains parents: Vec<String> and Change::under(&["events", id]): Change::text("start", v).under(&["events", id]) is events.<id>.start. Top-level changes are unchanged (parents empty), so rooms (EditKind::Props), tasks and the merge path need no other change.
  • Reads: props::get_at(text, path) and props::keys_at(text, path).
  • New code is props/map.rs (about 330 lines); props.rs only got a scan refactor (one scanner for any indent; entries calls it with indent 0, same behaviour) and render_in (list indent relative to a nesting depth).

Sizes and big-O

  • Frontmatter F bytes, depth d <= ~4 (we write 3), s sibling keys per level.
  • One change: validate (existing, one serde_yaml parse + entry scan) O(F); path walk O(F) (each level scans only its own span, so total <= dF worst case, F in practice); alias refusal scan O(F); verify = 2 more serde_yaml parses O(F). Total O(dF), no allocation beyond the block copy. F is KBs; no per-note-count term, nothing touches the index.
  • Memory: a few copies of the frontmatter block.

Failure table

Failure Effect Detection Recovery Test
Duplicate key on the path (any level) ambiguous target pick finds two; also validate (serde_yaml) rejects any duplicate in the block Broken{line}, nothing written, person fixes in source mode duplicate_keys_refuse
Parent is text, list, [..], {..}, block scalar would clobber a value inline not empty / child is - refuse, nothing written malformed_maps_never_write
Ragged indent, tab, non-key line, unclosed quote inside a map cannot place the edit scan errors refuse malformed_maps_never_write
Anchors, aliases, << merge in the block an edit could change another value through an alias refuse_aliases (ignores quoted text, comments, block scalars) refuse malformed_maps_never_write, aliases_in_text_and_block_scalars_are_fine
Scanner bug writes wrong bytes silent corruption verify: new block parsed by serde_yaml must equal the old parse with only this change made Broken, nothing written nested_edits_match_serde_yaml (5000 random trees)
Unrelated bytes rewritten lost comments/spacing exact-bytes goldens; property: replace keeps prefix/suffix, insert keeps all old lines, remove keeps the rest n/a adds_last_and_keeps_every_other_byte, property test
Removing last pair leaves empty events: junk key collapse test map removed unless a comment sits on it removes_a_pair_and_an_emptied_map
Same value written again pointless edit/history same_value returns no edits adds_last...
Crash mid-write n/a here edits are pure; they go through the room/ydoc path like any property change existing room durability existing

Gate tests

unrelated YAML bytes survive: adds_last_and_keeps_every_other_byte, nested_edits_match_serde_yaml. duplicate keys refuse safely: duplicate_keys_refuse. malformed maps never write: malformed_maps_never_write.

Decisions (no questions asked)

  • Maps are written in block style, 2 spaces per level (kayg, round 3); an existing map keeps its own indent for new pairs.
  • Removing the last pair removes the map (ADR 0068: unset optional properties are omitted), except when the map line carries a comment or a comment is inside it.
  • A flow map ({}), and sequences of maps (log: list of outcome maps) are not edited by path in this step: refused, nothing written. Replacing/removing such a key whole still works through the old top-level path. (Not needed by steps 3-4.)
  • New is not extended with a map variant; maps are built pair by pair, so the writer never needs to serialise whole maps.
  • CRLF frontmatter is unreadable to the existing writer (block needs ---\n); unchanged.

Results

All run on calternal-dev-01-compute02 inside the infra/nix-devshell flake (flake files not committed):

  • cargo test -p calternal-notes --lib props : 24 passed (9 new in props::map_tests).
  • cargo clippy -p calternal-notes -p calternal-tasks --all-targets -j4 -- -D warnings : clean. cargo fmt applied.
  • cargo test -p calternal-notes -p calternal-tasks --no-fail-fast -j4 : notes 435 passed, tasks 28 passed. One earlier run had a single failure in room::save::...::independent_652_real_syncclient_joining_snapshot_is_not_saved (EOF while parsing, unrelated to props, VM shared with other jobs); it passed on rerun. Not investigated further.

Plan was thought through before coding; the PR text was written once the gate was green.

Resume verification (2026-10-31)

Head remains cd8e5cbdc2688c255b6cdbdbfa31a53fb5187e6d; no implementation change was needed. Fresh checks on dev-01 in the Nix shell, with every heavy command through ~/bin/slot:

  • cargo fmt --all -- --check: passed.
  • cargo clippy -p calternal-notes -p calternal-tasks --all-targets -j4 -- -D warnings: passed.
  • cargo test -p calternal-notes -p calternal-tasks --no-fail-fast -j4: notes 435 passed / 6 ignored, tasks 28 passed. All nested-map gates passed.

Raw log: calternal-dev-01-compute02:~/cal-resume-s2.log. No PRs merged.

> simplification for final review: accept only a local splice of simple target lines, with YAML reparse verification; refuse unusual target syntax rather than preserving it heuristically. corpus tests independently check untouched bytes. linear in frontmatter size, with no new persistent state. Failure → effect → detection → recovery → test: unsafe input → explicit refusal with no note writes → boundary validation → edit the note or enable the completed UI contract later → awkward YAML / hidden syntax / stored replay / cross-folder transfer regressions. Gates: fmt, four-crate clippy with -D warnings, events/notes/tasks/calternal tests on dev-01. # Plan (ADR 0067 build order, step 2) Add safe nested map edits to the existing property writer (`crates/notes/src/props.rs`). Steps 3 (event parser) and 4 (identity/promotion) stack on this branch. Needed later by `events` (journal map: marker -> fields) and `source` (connected resource map). ## API - `Change` gains `parents: Vec<String>` and `Change::under(&["events", id])`: `Change::text("start", v).under(&["events", id])` is `events.<id>.start`. Top-level changes are unchanged (`parents` empty), so rooms (`EditKind::Props`), tasks and the merge path need no other change. - Reads: `props::get_at(text, path)` and `props::keys_at(text, path)`. - New code is `props/map.rs` (about 330 lines); `props.rs` only got a `scan` refactor (one scanner for any indent; `entries` calls it with indent 0, same behaviour) and `render_in` (list indent relative to a nesting depth). ## Sizes and big-O - Frontmatter F bytes, depth d <= ~4 (we write 3), s sibling keys per level. - One change: validate (existing, one serde_yaml parse + entry scan) O(F); path walk O(F) (each level scans only its own span, so total <= d*F worst case, F in practice); alias refusal scan O(F); verify = 2 more serde_yaml parses O(F). Total O(d*F), no allocation beyond the block copy. F is KBs; no per-note-count term, nothing touches the index. - Memory: a few copies of the frontmatter block. ## Failure table | Failure | Effect | Detection | Recovery | Test | |---|---|---|---|---| | Duplicate key on the path (any level) | ambiguous target | `pick` finds two; also `validate` (serde_yaml) rejects any duplicate in the block | `Broken{line}`, nothing written, person fixes in source mode | `duplicate_keys_refuse` | | Parent is text, list, `[..]`, `{..}`, block scalar | would clobber a value | inline not empty / child is `- ` | refuse, nothing written | `malformed_maps_never_write` | | Ragged indent, tab, non-key line, unclosed quote inside a map | cannot place the edit | `scan` errors | refuse | `malformed_maps_never_write` | | Anchors, aliases, `<<` merge in the block | an edit could change another value through an alias | `refuse_aliases` (ignores quoted text, comments, block scalars) | refuse | `malformed_maps_never_write`, `aliases_in_text_and_block_scalars_are_fine` | | Scanner bug writes wrong bytes | silent corruption | `verify`: new block parsed by serde_yaml must equal the old parse with only this change made | `Broken`, nothing written | `nested_edits_match_serde_yaml` (5000 random trees) | | Unrelated bytes rewritten | lost comments/spacing | exact-bytes goldens; property: replace keeps prefix/suffix, insert keeps all old lines, remove keeps the rest | n/a | `adds_last_and_keeps_every_other_byte`, property test | | Removing last pair leaves empty `events:` | junk key | collapse test | map removed unless a comment sits on it | `removes_a_pair_and_an_emptied_map` | | Same value written again | pointless edit/history | `same_value` | returns no edits | `adds_last...` | | Crash mid-write | n/a here | edits are pure; they go through the room/ydoc path like any property change | existing room durability | existing | ## Gate tests `unrelated YAML bytes survive`: `adds_last_and_keeps_every_other_byte`, `nested_edits_match_serde_yaml`. `duplicate keys refuse safely`: `duplicate_keys_refuse`. `malformed maps never write`: `malformed_maps_never_write`. ## Decisions (no questions asked) - Maps are written in block style, 2 spaces per level (kayg, round 3); an existing map keeps its own indent for new pairs. - Removing the last pair removes the map (ADR 0068: unset optional properties are omitted), except when the map line carries a comment or a comment is inside it. - A flow map (`{}`), and sequences of maps (`log:` list of outcome maps) are not edited by path in this step: refused, nothing written. Replacing/removing such a key whole still works through the old top-level path. (Not needed by steps 3-4.) - `New` is not extended with a map variant; maps are built pair by pair, so the writer never needs to serialise whole maps. - CRLF frontmatter is unreadable to the existing writer (`block` needs `---\n`); unchanged. # Results All run on calternal-dev-01-compute02 inside the `infra/nix-devshell` flake (flake files not committed): - `cargo test -p calternal-notes --lib props` : 24 passed (9 new in `props::map_tests`). - `cargo clippy -p calternal-notes -p calternal-tasks --all-targets -j4 -- -D warnings` : clean. `cargo fmt` applied. - `cargo test -p calternal-notes -p calternal-tasks --no-fail-fast -j4` : notes 435 passed, tasks 28 passed. One earlier run had a single failure in `room::save::...::independent_652_real_syncclient_joining_snapshot_is_not_saved` (EOF while parsing, unrelated to props, VM shared with other jobs); it passed on rerun. Not investigated further. Plan was thought through before coding; the PR text was written once the gate was green. ## Resume verification (2026-10-31) Head remains `cd8e5cbdc2688c255b6cdbdbfa31a53fb5187e6d`; no implementation change was needed. Fresh checks on dev-01 in the Nix shell, with every heavy command through `~/bin/slot`: - `cargo fmt --all -- --check`: passed. - `cargo clippy -p calternal-notes -p calternal-tasks --all-targets -j4 -- -D warnings`: passed. - `cargo test -p calternal-notes -p calternal-tasks --no-fail-fast -j4`: notes 435 passed / 6 ignored, tasks 28 passed. All nested-map gates passed. Raw log: `calternal-dev-01-compute02:~/cal-resume-s2.log`. No PRs merged.
notes: clippy
Some checks failed
CI / server (pull_request) Has been cancelled
Details
CI / rust (pull_request) Has been cancelled
Details
CI / web (pull_request) Has been cancelled
Details
cd8e5cbdc2
kayg commented 2026-10-31 16:20:05 +00:00
Author
Owner
Copy link

Independent review, round 1

Verdict: fix first. Reviewed cd8e5cbdc2688c255b6cdbdbfa31a53fb5187e6d, ADRs 0067/0068/0056/0066, the implementation and the cited tests. Severity follows the requested rule: silent note-content loss is P0. Findings below are source-level traces; I have not rerun the full gate suites.

  1. P0 — CRLF/BOM frontmatter is treated as absent, allowing a second frontmatter block. crates/notes/src/props/map.rs:36, through crates/notes/src/props.rs:365 and :394. For ---\r\ntitle: Keep\r\n---\r\nbody, changed(text, &[Change::text("uid", "new").under(&["source"])]) accepts validation, sees old_len == 0, and prepends a fresh LF block. The original properties become body text. A UTF-8 BOM before an otherwise valid block takes the same path; even malformed CRLF/BOM YAML passes the absent-block check. The PR's statement that CRLF is unreadable does not mean writes are refused. Fix: distinguish absent frontmatter from unsupported/malformed frontmatter before building edits; preserve the complete existing encoding/line endings or return Broken, with no edit. Add CRLF and BOM golden/refusal cases, including malformed and duplicate-key variants.

  2. P0 — Replacing a leaf deletes an untouched indented comment. crates/notes/src/props/map.rs:118 and the new scanner in crates/notes/src/props.rs:250. For ---\nm:\n a: old\n # keep this comment\n b: other\n---\nbody, setting m.a to new replaces the whole entry span, including the comment. The YAML equality verifier accepts this because comments do not affect parsed values. Fix: preserve comment lines independently of the replaced value, or refuse a replacement whose continuation lines cannot be retained safely. Test this exact case for replace/remove and comments between nested maps.

Failure-table audit (coverage inspected, not a new passing-test claim):

Claimed failure row What the tests establish / gap
Duplicate key on path duplicate_keys_refuse covers top, inner and unrelated duplicates for LF blocks. CRLF/BOM bypass remains finding 1.
Parent is scalar/list/flow/block scalar Explicit refusal examples exist in malformed_maps_never_write.
Ragged indent/tab/non-key/unclosed quote Explicit LF refusal examples exist; unsupported envelope variants are absent.
Anchors/aliases/merge Block/sequence examples and quoted/block-scalar exceptions exist. Flow-map anchors such as other: {x: &a yes} are not covered; starts_alias does not recognize a value after a flow-map colon. Expand refusals if the contract is “any anchor anywhere means no write.”
Scanner changes wrong semantic value serde_yaml verification exists. The property test also uses production map::expected, so it is not an independent oracle for errors in that helper.
Unrelated bytes rewritten Existing golden/property cases cover generated LF trees, ordering and some spacing. They do not establish CRLF/BOM, arbitrary quoting or comment preservation; findings 1–2 contradict the broad claim.
Last pair leaves empty map Removal/collapse plus comments on/above a map are covered. This does not cover finding 2.
Same value again No-op golden and property checks exist.
Crash mid-write This API returns edits; room durability is inherited, not newly exercised here.

Mergeability: Forgejo currently reports mergeable: false, but on dev-01 git merge-tree --write-tree origin/dev cd8e5cbd exits 0 against dev=2f304154054ec68b7855bb1f2bd24b1fb15a386d (tree 40626d4e458dbcf9c10386842a66339b40c86765). No Git content conflict was reproduced. #783/#785 stack on this PR; stacking explains cumulative diffs, not this false server flag. The precise Forgejo-side cause is unproven. No code pushed or merged.

Independent reproduction addendum: a standalone Rust probe on dev-01, inside the Nix shell and compiled through ~/bin/slot, reproduced both P0 scenarios. CRLF and BOM inputs returned Ok with a fresh LF frontmatter prepended; replacing m.a returned Ok with # keep this comment deleted. Probe: /tmp/cal-independent-r1/repro.rs, binary /tmp/cal-independent-r1/repro. It linked the VM's existing cached Notes library (no fresh full-suite build); the relevant props/map implementation is unchanged between that library's branch source and the reviewed stack. The parser modules in the probe were copied directly from the reviewed source. This is a focused reproduction, not a fresh full gate run.

# Independent review, round 1 **Verdict: fix first.** Reviewed `cd8e5cbdc2688c255b6cdbdbfa31a53fb5187e6d`, ADRs 0067/0068/0056/0066, the implementation and the cited tests. Severity follows the requested rule: silent note-content loss is P0. Findings below are source-level traces; I have not rerun the full gate suites. 1. **P0 — CRLF/BOM frontmatter is treated as absent, allowing a second frontmatter block.** `crates/notes/src/props/map.rs:36`, through `crates/notes/src/props.rs:365` and `:394`. For `---\r\ntitle: Keep\r\n---\r\nbody`, `changed(text, &[Change::text("uid", "new").under(&["source"])])` accepts validation, sees `old_len == 0`, and prepends a fresh LF block. The original properties become body text. A UTF-8 BOM before an otherwise valid block takes the same path; even malformed CRLF/BOM YAML passes the absent-block check. The PR's statement that CRLF is unreadable does not mean writes are refused. **Fix:** distinguish absent frontmatter from unsupported/malformed frontmatter before building edits; preserve the complete existing encoding/line endings or return `Broken`, with no edit. Add CRLF and BOM golden/refusal cases, including malformed and duplicate-key variants. 2. **P0 — Replacing a leaf deletes an untouched indented comment.** `crates/notes/src/props/map.rs:118` and the new scanner in `crates/notes/src/props.rs:250`. For `---\nm:\n a: old\n # keep this comment\n b: other\n---\nbody`, setting `m.a` to `new` replaces the whole entry span, including the comment. The YAML equality verifier accepts this because comments do not affect parsed values. **Fix:** preserve comment lines independently of the replaced value, or refuse a replacement whose continuation lines cannot be retained safely. Test this exact case for replace/remove and comments between nested maps. Failure-table audit (coverage inspected, not a new passing-test claim): | Claimed failure row | What the tests establish / gap | |---|---| | Duplicate key on path | `duplicate_keys_refuse` covers top, inner and unrelated duplicates for LF blocks. CRLF/BOM bypass remains finding 1. | | Parent is scalar/list/flow/block scalar | Explicit refusal examples exist in `malformed_maps_never_write`. | | Ragged indent/tab/non-key/unclosed quote | Explicit LF refusal examples exist; unsupported envelope variants are absent. | | Anchors/aliases/merge | Block/sequence examples and quoted/block-scalar exceptions exist. Flow-map anchors such as `other: {x: &a yes}` are not covered; `starts_alias` does not recognize a value after a flow-map colon. Expand refusals if the contract is “any anchor anywhere means no write.” | | Scanner changes wrong semantic value | serde_yaml verification exists. The property test also uses production `map::expected`, so it is not an independent oracle for errors in that helper. | | Unrelated bytes rewritten | Existing golden/property cases cover generated LF trees, ordering and some spacing. They do not establish CRLF/BOM, arbitrary quoting or comment preservation; findings 1–2 contradict the broad claim. | | Last pair leaves empty map | Removal/collapse plus comments on/above a map are covered. This does not cover finding 2. | | Same value again | No-op golden and property checks exist. | | Crash mid-write | This API returns edits; room durability is inherited, not newly exercised here. | Mergeability: Forgejo currently reports `mergeable: false`, but on dev-01 `git merge-tree --write-tree origin/dev cd8e5cbd` exits 0 against `dev=2f304154054ec68b7855bb1f2bd24b1fb15a386d` (tree `40626d4e458dbcf9c10386842a66339b40c86765`). No Git content conflict was reproduced. #783/#785 stack on this PR; stacking explains cumulative diffs, not this false server flag. The precise Forgejo-side cause is unproven. No code pushed or merged. Independent reproduction addendum: a standalone Rust probe on dev-01, inside the Nix shell and compiled through `~/bin/slot`, reproduced both P0 scenarios. CRLF and BOM inputs returned `Ok` with a fresh LF frontmatter prepended; replacing `m.a` returned `Ok` with `# keep this comment` deleted. Probe: `/tmp/cal-independent-r1/repro.rs`, binary `/tmp/cal-independent-r1/repro`. It linked the VM's existing cached Notes library (no fresh full-suite build); the relevant props/map implementation is unchanged between that library's branch source and the reviewed stack. The parser modules in the probe were copied directly from the reviewed source. This is a focused reproduction, not a fresh full gate run.
kayg commented 2026-10-31 16:34:20 +00:00
Author
Owner
Copy link

Round 1 implementation fixes

Head f92341a6. No merge. #783 is being rebased onto this head, followed by #785.

Finding Status / regression
P0 CRLF/BOM demote existing properties Fixed by refusing these unsupported envelopes before any edit. review_envelopes_refuse_before_any_edit covers valid, malformed and duplicate-key input, both nested and top-level writes.
P0 leaf replacement/removal deletes continuation comments Fixed by refusing replacements/removals with comment-bearing continuation spans; review_leaf_comments_are_never_deleted uses the exact reproduction plus a nested-map case.
Flow-map anchor gap Fixed: alias/anchor guard recognizes value starts after flow-map colons. review_flow_aliases_refuse covers both alias-consumer and standalone-anchor forms.
Property-test oracle shares expected Remaining test limitation: semantic property checking still shares that helper. Exact byte/refusal regression assertions are independent.

All three new regressions failed on the original implementation in ~/cal-r1-s2-red.log. The first full run exposed the old property generator's assumption that every valid tree can be edited, including commented subtrees; its refusal oracle now checks that exact additional unsupported case. No permissive production fallback was added.

Decision: retain the existing line-level splicer; no whole-YAML serialization. CRLF/BOM editing remains unsupported and safely refuses. Unrelated bytes are never demoted to body text. Comment-bearing subtree replacement is unsupported until comment ownership can be retained unambiguously. Work remains O(frontmatter bytes) with no Home scan.

Fresh dev-01 Nix gates, heavy commands through ~/bin/slot, log ~/cal-r1-s2-final.log:

  • cargo fmt --all && cargo fmt --all -- --check
  • cargo clippy -p calternal-notes -p calternal-tasks -p calternal --all-targets -j4 -- -D warnings
  • cargo test -p calternal-notes -p calternal-tasks -p calternal --no-fail-fast -j4

All passed, including notes 438 passed / 6 ignored and tasks 28 passed. The events crate does not exist at this stack layer; its gates run on #783/#785. Existing ignored tests were not enabled. No further independent review round is claimed.

## Round 1 implementation fixes Head `f92341a6`. No merge. #783 is being rebased onto this head, followed by #785. | Finding | Status / regression | |---|---| | P0 CRLF/BOM demote existing properties | Fixed by refusing these unsupported envelopes before any edit. `review_envelopes_refuse_before_any_edit` covers valid, malformed and duplicate-key input, both nested and top-level writes. | | P0 leaf replacement/removal deletes continuation comments | Fixed by refusing replacements/removals with comment-bearing continuation spans; `review_leaf_comments_are_never_deleted` uses the exact reproduction plus a nested-map case. | | Flow-map anchor gap | Fixed: alias/anchor guard recognizes value starts after flow-map colons. `review_flow_aliases_refuse` covers both alias-consumer and standalone-anchor forms. | | Property-test oracle shares `expected` | Remaining test limitation: semantic property checking still shares that helper. Exact byte/refusal regression assertions are independent. | All three new regressions failed on the original implementation in `~/cal-r1-s2-red.log`. The first full run exposed the old property generator's assumption that every valid tree can be edited, including commented subtrees; its refusal oracle now checks that exact additional unsupported case. No permissive production fallback was added. Decision: retain the existing line-level splicer; no whole-YAML serialization. CRLF/BOM editing remains unsupported and safely refuses. Unrelated bytes are never demoted to body text. Comment-bearing subtree replacement is unsupported until comment ownership can be retained unambiguously. Work remains O(frontmatter bytes) with no Home scan. Fresh dev-01 Nix gates, heavy commands through `~/bin/slot`, log `~/cal-r1-s2-final.log`: - `cargo fmt --all && cargo fmt --all -- --check` - `cargo clippy -p calternal-notes -p calternal-tasks -p calternal --all-targets -j4 -- -D warnings` - `cargo test -p calternal-notes -p calternal-tasks -p calternal --no-fail-fast -j4` All passed, including notes 438 passed / 6 ignored and tasks 28 passed. The events crate does not exist at this stack layer; its gates run on #783/#785. Existing ignored tests were not enabled. No further independent review round is claimed.
kayg commented 2026-10-31 17:10:31 +00:00
Author
Owner
Copy link

Independent review, round 2

Verdict: fix first. Reviewed f92341a658fb0fa892aeb19e3cb9037d82f47569, the round-1 review/fix status, and ADRs 0067/0068/0056/0066. No prior round-2 comment existed when checked. No code pushed or merged.

Round-1 finding / audit item Round-2 status Does the test prove it?
P0 CRLF/BOM frontmatter demotion Verified: validation refuses before either top-level or nested edits. review_envelopes_refuse_before_any_edit directly checks valid, malformed and duplicate-key variants for the unsupported envelopes and both write routes. It establishes refusal, not editing support.
P0 continuation comments lost on replacement/removal Not fixed for the full comment-preservation requirement; the exact standalone-comment reproductions are fixed. review_leaf_comments_are_never_deleted proves the cited standalone-comment cases refuse. It does not cover inline comments on retained sequence items or child fields; new finding below.
Flow-map anchor hole Verified for the cited colon/value cases. review_flow_aliases_refuse checks standalone flow anchors and a consumer. It is not an exhaustive YAML-token oracle.
Shared property-test expected oracle Not fixed, explicitly acknowledged. Generated semantic checks still share production expected; byte/refusal goldens are independent but narrower.

New P0 — Inline continuation comments still disappear silently. crates/notes/src/props.rs:292, crates/notes/src/props/map.rs:112, and crates/notes/src/props.rs:501. The new guard recognizes only lines whose trimmed text starts with #. Starting with:

m:
  a:
    - old # keep this comment
  b: other

Change::list("a", vec!["old".into(), "new".into()]).under(&["m"]) returns success with old and new, but deletes # keep this comment from the unchanged old item. Replacing a subtree containing x: old # keep this comment also deletes the comment. YAML semantic verification cannot detect either loss. This is a remaining hole exposed by round 2, not a claim that the fix newly created the old loss. Fix: preserve inline comments with their retained source items/fields, or conservatively refuse affected replacements/removals using YAML-aware comment detection that distinguishes quoted/scalar # text. Add independent exact-byte/refusal cases for list append, subtree replacement and removal.

Verification: on dev-01, Nix shell, all heavy commands through ~/bin/slot, at stack head 771eb102 (these props functions are unchanged from this PR): cargo test -p calternal-notes review_ -j4 passed 24 tests, including all three new props regressions. A standalone Rust probe linked the current cached stack libraries and observed the successful comment deletion above; /tmp/cal-r2-probe.rs, /tmp/cal-r2-probe.out. This is focused evidence, not a fresh full-suite, perf or exhaustive YAML claim. The test pass does not waive the P0.

# Independent review, round 2 **Verdict: fix first.** Reviewed `f92341a658fb0fa892aeb19e3cb9037d82f47569`, the round-1 review/fix status, and ADRs 0067/0068/0056/0066. No prior round-2 comment existed when checked. No code pushed or merged. | Round-1 finding / audit item | Round-2 status | Does the test prove it? | |---|---|---| | P0 CRLF/BOM frontmatter demotion | **Verified**: validation refuses before either top-level or nested edits. | `review_envelopes_refuse_before_any_edit` directly checks valid, malformed and duplicate-key variants for the unsupported envelopes and both write routes. It establishes refusal, not editing support. | | P0 continuation comments lost on replacement/removal | **Not fixed for the full comment-preservation requirement**; the exact standalone-comment reproductions are fixed. | `review_leaf_comments_are_never_deleted` proves the cited standalone-comment cases refuse. It does not cover inline comments on retained sequence items or child fields; new finding below. | | Flow-map anchor hole | **Verified** for the cited colon/value cases. | `review_flow_aliases_refuse` checks standalone flow anchors and a consumer. It is not an exhaustive YAML-token oracle. | | Shared property-test `expected` oracle | **Not fixed**, explicitly acknowledged. | Generated semantic checks still share production `expected`; byte/refusal goldens are independent but narrower. | **New P0 — Inline continuation comments still disappear silently.** `crates/notes/src/props.rs:292`, `crates/notes/src/props/map.rs:112`, and `crates/notes/src/props.rs:501`. The new guard recognizes only lines whose trimmed text starts with `#`. Starting with: ```yaml m: a: - old # keep this comment b: other ``` `Change::list("a", vec!["old".into(), "new".into()]).under(&["m"])` returns success with `old` and `new`, but deletes `# keep this comment` from the unchanged old item. Replacing a subtree containing `x: old # keep this comment` also deletes the comment. YAML semantic verification cannot detect either loss. This is a remaining hole exposed by round 2, not a claim that the fix newly created the old loss. **Fix:** preserve inline comments with their retained source items/fields, or conservatively refuse affected replacements/removals using YAML-aware comment detection that distinguishes quoted/scalar `#` text. Add independent exact-byte/refusal cases for list append, subtree replacement and removal. Verification: on dev-01, Nix shell, all heavy commands through `~/bin/slot`, at stack head `771eb102` (these props functions are unchanged from this PR): `cargo test -p calternal-notes review_ -j4` passed 24 tests, including all three new props regressions. A standalone Rust probe linked the current cached stack libraries and **observed** the successful comment deletion above; `/tmp/cal-r2-probe.rs`, `/tmp/cal-r2-probe.out`. This is focused evidence, not a fresh full-suite, perf or exhaustive YAML claim. The test pass does not waive the P0.
kayg commented 2026-10-31 17:30:39 +00:00
Author
Owner
Copy link

round 2 fixes for final review

Head 94807faa. Simplification: replace only a simple scalar or block-list target's own lines. Comments inside the target, flow values, anchors/aliases, multiline scalars, subtree replacement and unsupported syntax refuse with “edit this in the note in source mode”. A true no-op keeps the original bytes. Deleting the last leaf keeps the parent lines. No merge.

Finding Status
#70965 P0 inline continuation comments lost Fixed by refusal for list append, subtree replacement and removal; quoted # text remains editable.
Semantic verification cannot prove byte preservation Every write re-parses through serde_yaml. review_round2_awkward_corpus_only_target_lines_or_refusal independently checks exact prefix/suffix bytes and the parsed target for a corpus of comments, flow values, anchors, aliases, block scalars and multiline quotes. It does not use the production expected-value helper.
Scope of deletion Only target lines are removed; empty parent lines are retained. Existing task update tests now use editable block lists.

Passed on dev-01 in Nix, log ~/cal-r2-s2-final3.log:

cargo fmt --all -- --check
~/bin/slot cargo clippy -p calternal-notes -p calternal-tasks -p calternal --all-targets -j4 -- -D warnings
~/bin/slot cargo test -p calternal-notes -p calternal-tasks -p calternal --no-fail-fast -j4

Notes: 439 passed / 6 existing ignored; tasks: 28 passed; calternal unit/integration suites passed with existing ignored tests. The events crate is introduced by the next stack layer, so it is absent from this layer's command. No build/test ran on the control host; Nix scaffolding was not committed.

Decision: support fewer write forms, keeping lenient reads. Source editing is required for refused forms; this is intentional. The existing generated tests still use their old shared semantic oracle, supplemented by the independent byte/refusal corpus. Final review round 3 is pending; this comment does not claim review approval.

## round 2 fixes for final review Head `94807faa`. Simplification: replace only a simple scalar or block-list target's own lines. Comments inside the target, flow values, anchors/aliases, multiline scalars, subtree replacement and unsupported syntax refuse with “edit this in the note in source mode”. A true no-op keeps the original bytes. Deleting the last leaf keeps the parent lines. No merge. | Finding | Status | |---|---| | #70965 P0 inline continuation comments lost | Fixed by refusal for list append, subtree replacement and removal; quoted `#` text remains editable. | | Semantic verification cannot prove byte preservation | Every write re-parses through serde_yaml. `review_round2_awkward_corpus_only_target_lines_or_refusal` independently checks exact prefix/suffix bytes and the parsed target for a corpus of comments, flow values, anchors, aliases, block scalars and multiline quotes. It does not use the production expected-value helper. | | Scope of deletion | Only target lines are removed; empty parent lines are retained. Existing task update tests now use editable block lists. | Passed on dev-01 in Nix, log `~/cal-r2-s2-final3.log`: ```sh cargo fmt --all -- --check ~/bin/slot cargo clippy -p calternal-notes -p calternal-tasks -p calternal --all-targets -j4 -- -D warnings ~/bin/slot cargo test -p calternal-notes -p calternal-tasks -p calternal --no-fail-fast -j4 ``` Notes: 439 passed / 6 existing ignored; tasks: 28 passed; calternal unit/integration suites passed with existing ignored tests. The events crate is introduced by the next stack layer, so it is absent from this layer's command. No build/test ran on the control host; Nix scaffolding was not committed. Decision: support fewer write forms, keeping lenient reads. Source editing is required for refused forms; this is intentional. The existing generated tests still use their old shared semantic oracle, supplemented by the independent byte/refusal corpus. Final review round 3 is pending; this comment does not claim review approval.
kayg commented 2026-10-31 17:43:47 +00:00
Author
Owner
Copy link

Independent review, round 3 (final)

Verdict: stop and simplify. Reviewed 94807faabbdc6335ba719e5fa4987a5f032a7ab6, round-2 review #70965, fix status #71020, simplification notes and ADRs 0067/0068. No code pushed or merged. Round 3 still finds a new P0 reproduction, so this is the final review round under kayg's rule.

Round-2 item Final status / evidence
Inline list/subtree comments disappear Reported fixtures now refuse; the full comment-preservation requirement is not fixed, see new P0 below.
CRLF/BOM, flow aliases, standalone continuation comments Existing refusal regressions pass; no regression found in those cases.
Exact splices / empty parents Implementation replaces the selected span or inserts at a map boundary; parent lines remain. Independent awkward corpus passes and checks exact prefix/suffix plus parsed target. It establishes locality for its accepted cases, not preservation of comments inside the selected span.
Property-style oracle nested_edits_match_serde_yaml passes its default 5,000 cases. Replacement checks exact prefix/suffix, but insertion/deletion checks only line subsequences, and semantic expectations still call production map::expected. The alphabetic generated leaves and 14-value independent corpus miss plain scalars containing interior quotes plus comments. These tests do not prove every accepted edit preserves authored comments.

New P0: an interior quote in a YAML plain scalar bypasses the comment-loss refusal. crates/notes/src/props.rs:291, :670. Valid YAML:

---
other: untouched
m:
  a: old 'quoted # keep
  b: stay # neighbor
---
body

Change::text("a", "new").under(&["m"]) succeeds and replaces the target with a: new, silently deleting # keep. Top-level replacement does the same. a: Bob said "hello # keep also reproduces it. In YAML, a quote embedded in an already-started plain scalar does not open a quoted scalar; # keep is a real comment. strip_comment opens quote state after a space, misses the hash, and refuse_comment_loss accepts it. The serde_yaml postcondition passes because it cannot see comments. Independent VM probes observed success and exact comment loss; the ordinary old # keep control refuses. This is a new input hole in the surviving comment-loss class, not a regression claim about the earlier exact fixtures.

Concrete simpler design / fix: make the editable syntax a strict lexical allowlist independent of the lenient reader: initially refuse any existing target scalar/list item containing #, and refuse interior quotes in plain scalars. Permit only complete single-line scalars and simple block-list items whose whole lexical form is proven safe; keep all complex/commented targets for source editing. Keep exact splices and serde_yaml as a postcondition, not a comment oracle. Add independent replacement/removal/list tests for both quote types and hashes, asserting exact unchanged input on refusal; generate punctuation-bearing plain scalars rather than only alphabetic leaves. Do not start a fourth patch/review cycle around another quote heuristic.

Fresh verification on dev-01, Nix shell, stack head eb7ffd2d (the implicated writer/guard is unchanged from this PR), heavy commands via ~/bin/slot:

  • cargo test -p calternal-events -p calternal-notes -p calternal-tasks review_ -j4: events 21, notes 25, tasks 7 passed; includes independent awkward corpus.
  • cargo test -p calternal-notes nested_edits_match_serde_yaml -j4: 1 passed, default 5,000 generated cases.
  • Standalone cached-library probe compiled with ~/bin/slot rustc --edition=2024 /tmp/cal-r3-props-probe.rs -L dependency=target/debug/deps --extern calternal_notes="$notes_lib" -o /tmp/cal-r3-props-probe; executed separately. Source/output: /tmp/cal-r3-props-probe.rs, /tmp/cal-r3-props-probe.out; test log /tmp/cal-r3-review-tests.log, property log /tmp/cal-r3-property-test.log on dev-01. $notes_lib was the newest built libcalternal_notes-*.rlib in that worktree.

No fresh full-suite, clippy, performance or browser result is claimed. No additional P1–P3 finding beyond the previously acknowledged oracle limitation.

# Independent review, round 3 (final) **Verdict: stop and simplify.** Reviewed `94807faabbdc6335ba719e5fa4987a5f032a7ab6`, round-2 review #70965, fix status #71020, simplification notes and ADRs 0067/0068. No code pushed or merged. Round 3 still finds a new P0 reproduction, so this is the final review round under kayg's rule. | Round-2 item | Final status / evidence | |---|---| | Inline list/subtree comments disappear | Reported fixtures now refuse; the full comment-preservation requirement is **not fixed**, see new P0 below. | | CRLF/BOM, flow aliases, standalone continuation comments | Existing refusal regressions pass; no regression found in those cases. | | Exact splices / empty parents | Implementation replaces the selected span or inserts at a map boundary; parent lines remain. Independent awkward corpus passes and checks exact prefix/suffix plus parsed target. It establishes locality for its accepted cases, not preservation of comments inside the selected span. | | Property-style oracle | `nested_edits_match_serde_yaml` passes its default 5,000 cases. Replacement checks exact prefix/suffix, but insertion/deletion checks only line subsequences, and semantic expectations still call production `map::expected`. The alphabetic generated leaves and 14-value independent corpus miss plain scalars containing interior quotes plus comments. These tests do **not** prove every accepted edit preserves authored comments. | **New P0: an interior quote in a YAML plain scalar bypasses the comment-loss refusal.** `crates/notes/src/props.rs:291`, `:670`. Valid YAML: ```yaml --- other: untouched m: a: old 'quoted # keep b: stay # neighbor --- body ``` `Change::text("a", "new").under(&["m"])` succeeds and replaces the target with `a: new`, silently deleting `# keep`. Top-level replacement does the same. `a: Bob said "hello # keep` also reproduces it. In YAML, a quote embedded in an already-started plain scalar does not open a quoted scalar; `# keep` is a real comment. `strip_comment` opens quote state after a space, misses the hash, and `refuse_comment_loss` accepts it. The serde_yaml postcondition passes because it cannot see comments. Independent VM probes observed success and exact comment loss; the ordinary `old # keep` control refuses. This is a new input hole in the surviving comment-loss class, not a regression claim about the earlier exact fixtures. **Concrete simpler design / fix:** make the editable syntax a strict lexical allowlist independent of the lenient reader: initially refuse any existing target scalar/list item containing `#`, and refuse interior quotes in plain scalars. Permit only complete single-line scalars and simple block-list items whose whole lexical form is proven safe; keep all complex/commented targets for source editing. Keep exact splices and serde_yaml as a postcondition, not a comment oracle. Add independent replacement/removal/list tests for both quote types and hashes, asserting exact unchanged input on refusal; generate punctuation-bearing plain scalars rather than only alphabetic leaves. Do not start a fourth patch/review cycle around another quote heuristic. Fresh verification on dev-01, Nix shell, stack head `eb7ffd2d` (the implicated writer/guard is unchanged from this PR), heavy commands via `~/bin/slot`: - `cargo test -p calternal-events -p calternal-notes -p calternal-tasks review_ -j4`: events 21, notes 25, tasks 7 passed; includes independent awkward corpus. - `cargo test -p calternal-notes nested_edits_match_serde_yaml -j4`: 1 passed, default 5,000 generated cases. - Standalone cached-library probe compiled with `~/bin/slot rustc --edition=2024 /tmp/cal-r3-props-probe.rs -L dependency=target/debug/deps --extern calternal_notes="$notes_lib" -o /tmp/cal-r3-props-probe`; executed separately. Source/output: `/tmp/cal-r3-props-probe.rs`, `/tmp/cal-r3-props-probe.out`; test log `/tmp/cal-r3-review-tests.log`, property log `/tmp/cal-r3-property-test.log` on dev-01. `$notes_lib` was the newest built `libcalternal_notes-*.rlib` in that worktree. No fresh full-suite, clippy, performance or browser result is claimed. No additional P1–P3 finding beyond the previously acknowledged oracle limitation.
kayg commented 2026-10-31 17:50:34 +00:00
Author
Owner
Copy link

On hold under the three-round stop-and-simplify rule. The replacement plan is PR #802: #802

Plan: https://git.kayg.org/kayg/calternal/src/branch/docs/calendar-backend-v2-plan/docs/plans/0067-calendar-backend-v2.md

No further patches or fourth review round on this stack. The plan chooses a strict whole-frontmatter write allowlist, the editor's Lezer parser with an explicit backend bridge feasibility gate, and centralized marker writes disabled until full UI hiding. It lists salvage, independent tests and smaller replacement PRs. This comment changes no implementation code and requests no merge.

On hold under the three-round stop-and-simplify rule. The replacement plan is PR #802: https://git.kayg.org/kayg/calternal/pulls/802 Plan: https://git.kayg.org/kayg/calternal/src/branch/docs/calendar-backend-v2-plan/docs/plans/0067-calendar-backend-v2.md No further patches or fourth review round on this stack. The plan chooses a strict whole-frontmatter write allowlist, the editor's Lezer parser with an explicit backend bridge feasibility gate, and centralized marker writes disabled until full UI hiding. It lists salvage, independent tests and smaller replacement PRs. This comment changes no implementation code and requests no merge.
kayg commented 2026-11-07 19:10:06 +00:00
Author
Owner
Copy link

housekeeping #1009: needs-decision.

Calendar v1 writer slice was held after three review rounds; #802 chooses the shared note executor approach. Preserve old writer regressions instead of replaying the abandoned mutation path.

original head: 94807faabbdc6335ba719e5fa4987a5f032a7ab6. against dev cd7f84432febfc63b788fbb9b9f9950452982d74, git cherry reports 11 unmatched and 0 equivalent patches. the landing audit lists each candidate missing commit and stable patch match in pr-781.md; unmatched patches can include superseded behavior.

left open, source branch unchanged. no rebase or new test claim for this decision lane.

housekeeping #1009: needs-decision. Calendar v1 writer slice was held after three review rounds; #802 chooses the shared note executor approach. Preserve old writer regressions instead of replaying the abandoned mutation path. original head: `94807faabbdc6335ba719e5fa4987a5f032a7ab6`. against dev `cd7f84432febfc63b788fbb9b9f9950452982d74`, git cherry reports 11 unmatched and 0 equivalent patches. the [landing audit](https://git.kayg.org/attachments/fe3c09dc-dea3-47a0-92e2-b44a777083e5) lists each candidate missing commit and stable patch match in `pr-781.md`; unmatched patches can include superseded behavior. left open, source branch unchanged. no rebase or new test claim for this decision lane.
kayg commented 2026-11-07 21:21:18 +00:00
Author
Owner
Copy link

Superseded by calendar backend v2 (#802). The branch and original tips are archived under archive/ (see #1009), so nothing is lost; useful regression vectors are noted there for the active lanes. Closing.

Superseded by calendar backend v2 (#802). The branch and original tips are archived under `archive/` (see #1009), so nothing is lost; useful regression vectors are noted there for the active lanes. Closing.
kayg closed this pull request 2026-11-07 21:21:19 +00:00
Some checks failed
CI / server (pull_request) Has been cancelled
Details
CI / rust (pull_request) Has been cancelled
Details
CI / web (pull_request) Has been cancelled
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
backend
Rust server

  
design
Mockups, UI and TASTE

  
docs
ADRs, CONTEXT, TASTE

  
idea
Parked idea, never worked until promoted

  
infra
Deploy, CI, VMs

  
perf
Performance and budgets

  
reliability
Data integrity, corruption traps

  
web
Web client

No labels
backend
design
docs
idea
infra
perf
reliability
web
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
kayg/calternal!781
No description provided.