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

events: identity and promotion actions (calendar step 4) #785

Closed
kayg wants to merge 13 commits from feat/calendar-identity into dev
pull from: feat/calendar-identity
merge into: kayg:dev
Conversation 8 Commits 13 Files changed 27 +3268 -14
kayg commented 2026-10-31 05:48:27 +00:00
Owner
Copy link

simplification for final review: enforce the release gate at the shared bulk write boundary, including stored-job startup replay. keep disabled jobs durably deferred. refuse journal transfers containing links unless their bindings can be proven unchanged; no guessed rebasing.

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.

Stacked on #783 (3f94719e), on #781 (f92341a6); all target dev. Never merged. Round-1 hardening head: 771eb102.

problem and resulting behavior

Promotion could discard authoritative journal fields; identity settlement and replay could reassign or duplicate ownership. Supported journal records now transfer as raw field bytes, and malformed or ambiguous cases refuse note writes. Production marker-writing actions stay disabled until shared editor/source/search/copy/accessibility hiding exists. The parser/describer remains enabled.

implementation plan and costs

  • Preserve supported journal fields through a selected-record splice and byte relocation, not YAML reserialization. Rename title to summary, preserve explicit nulls/types/comments/unknown fields, and derive the owner title from authority.
  • Recover durable promotion jobs and missing claim finalization under the user lock before a new request. Reserve by source fingerprint plus exact snapshot version; separate verified durable undo from outside marker removal.
  • Inventory current Home notes for owners and all block markers; durably protect observed ordinary addresses. Refuse duplicate owners, same-note copies without provenance, unsupported graph copies and unavailable lookups.
  • Release-gate write actions pending the shared marker UI contract.

Management reads Home bytes and promotion history; it is not a point-lookup-only operation. Nested relationship scans and source-prefix/history queries can scale poorly. Claims grow with every observed/minted/adopted address. No 100k size/latency result is claimed. Inventory line classification uses a HashSet.

failure table

Failure Effect Detection Recovery Test
Unsupported or ambiguous journal record unsafe transfer strict entry and field validation refuse without changing note bytes review_present_unsupported_entries_refuse
Authority differs from line, or contains nulls/comments lost fields exact transfer golden raw field relocation, authoritative owner title review_authority_moves_with_exact_types_comments_and_bytes
Old source reappears after move/delete live ID reassigned versioned durable source claim and Home inventory refuse; explicit durable undo is separate review_moved_source_cannot_reclaim_live_identity
Job completes before ownership finalization second owner on new request recover promotion jobs before new requests drive job and finalize missing claim review_replay_then_new_key_cannot_create_an_owner
Paste before original / owner copy original binding guessed duplicate markers or owners refuse; retain original bytes review_paste_before_original_refuses_without_rekeying, review_copied_owner_notes_refuse_without_writes
Ordinary address collision or removed observed block ID reused current inventory plus permanent ordinary claims refuse reuse review_observed_ordinary_ids_stay_claimed_after_removal
Index unavailable unsafe free decision lookup errors fail closed review_unavailable_index_never_means_free
Marker UI absent literal IDs exposed production action contract expose no write actions review_marker_writes_are_not_exposed_before_ui_support

round 1 implementation fixes

Head 771eb102, rebased onto #783 3f94719e, which includes #781 f92341a6. No merge. Production marker-writing actions are disabled until the shared UI hiding contract exists. This is backend hardening, not completed step-4 UI delivery.

Finding Status / regression
P0 journal authority lost on promotion Fixed. Supported block records move as raw field bytes, with title renamed to summary; authoritative title controls owner H1/path/link and journal tags override line tags. Nulls, types, unknown/nested fields and comments survive. review_authority_moves_with_exact_types_comments_and_bytes asserts exact source/destination goldens; review_journal_authority_is_preserved_or_refused exercises the service.
P1 moved/deleted/unmarked source reuses a live ID Fixed by conservative refusal. Reservations include exact snapshot version; live/retired source matches cannot be reclaimed by coincident words. Explicit durable undo recovery is separate. review_moved_source_cannot_reclaim_live_identity, review_deleted_or_unmarked_source_refuses_reidentification; clean undo still passes.
P1 replay leaves owner claim unfinished Fixed. Under the user lock, recover promotion jobs and finalize claims before accepting new identify/promote requests. review_replay_then_new_key_cannot_create_an_owner, review_recovery_at_each_note_boundary_precedes_new_key cover stored/create/edit boundary states followed by a NEW key.
P1 present unsupported authority mistaken for absent Fixed. Strict entry lookup distinguishes absent from unsupported. Flow envelopes/records, scalar/list/empty records and ambiguous/reserved fields refuse before note writes. review_present_unsupported_entries_refuse.
P1 first occurrence guesses original after paste-before Fixed by refusal. Same-note duplicates and unproven imported duplicates retain all note bytes. Owned/child-linked/journal-authority copies also refuse. review_paste_before_original_refuses_without_rekeying; pure settle returns Ambiguous. Proven bare cross-note copies can still receive fresh IDs.
P1 adoption ignores owners/ordinary blocks/index errors Fixed. Current Home inventory adopts a unique existing owner, rejects duplicate owners and ordinary-marker collisions, and durably protects observed ordinary addresses after removal. Index errors fail closed and mint retries are bounded. review_owner_import_is_adopted_before_promotion, review_copied_owner_notes_refuse_without_writes, review_ordinary_block_collision_refuses, review_observed_ordinary_ids_stay_claimed_after_removal, review_unavailable_index_never_means_free.
P1 literal markers exposed before UI hiding Release-gated. EventsPlugin::new exposes no identify/promote actions; published action contract removes them. Backend tests use a test-only harness. review_marker_writes_are_not_exposed_before_ui_support. UI hiding itself remains unimplemented and must land before enabling writes.
Audit: stale request settles unrelated markers Fixed. Validate target before settlement; identify/promote settle only the selected target. Existing stale-target regression passes.
Audit: weak no-nested-links assertion Tightened to require exactly one parsed child link, rather than allowing zero. 3,000 generated cases pass. Full destination-resolution proof remains follow-up.

Red evidence on dev-01: ~/cal-r1-s4-red.log, ~/cal-r1-s4-replay-red.log, ~/cal-r1-s4-more-red.log, ~/cal-r1-s4-retired-red.log. The new regressions were observed failing before their fixes. Boundary recovery tests simulate durable states and reopen; they are not new OS-kill tests at every boundary.

Fresh gates on dev-01 in the Nix shell, heavy commands via ~/bin/slot:

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

All passed in ~/cal-r1-s4-final2.log: events 57, notes 438 / 6 ignored, tasks 29, and calternal unit/integration checks (existing ignored cases remain ignored). The final inventory classification HashSet refactor subsequently passed fresh fmt/check, the same four-crate clippy, and ~/bin/slot cargo test -p calternal-events -j4 (57 passed), log ~/cal-r1-s4-lookup.log. Nix flake scaffolding was not committed.

Decisions and remaining limits:

  • Prefer refusal to guessing identity or rewriting unsupported YAML. CRLF/BOM inputs inherit #781 safe refusal. Transfer changes only the selected record and relocates its fields; an emptied journal map remains events: rather than rewriting surrounding bytes.
  • Management now reads current Home bytes and promotion history, rather than claiming point-lookup-only work. Inventory classification uses a line HashSet; overall nested scans and source-prefix/history queries can still scale poorly. No 100k identity/performance result is claimed.
  • The claim store protects observed ordinary addresses permanently. Unobserved historical ordinary IDs retain ADR 0068 O28 cold-import provenance limits.
  • UI editor/source/search/copy/accessibility hiding is deferred with write actions disabled. No frontend/browser validation is claimed.
  • Actual process kills at every new create/edit/finalization boundary, concurrent outside edits at the claim/write boundary, intervening-edit and transferred-entry undo combinations, full child-link destination resolution, and large cold-index measurements remain validation gaps. Existing inherited crash/undo suites passed; these are not substitutes for the missing combinations.
  • Inherited open P2/P3: #781 shared expected-value oracle; #783 repeated nested relationship traversal and its listed audit/performance gaps.

This addresses round 1. No round-2 approval, merge approval, or merge is claimed.

> simplification for final review: enforce the release gate at the shared bulk write boundary, including stored-job startup replay. keep disabled jobs durably deferred. refuse journal transfers containing links unless their bindings can be proven unchanged; no guessed rebasing. 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. > Stacked on #783 (`3f94719e`), on #781 (`f92341a6`); all target dev. Never merged. Round-1 hardening head: `771eb102`. ## problem and resulting behavior Promotion could discard authoritative journal fields; identity settlement and replay could reassign or duplicate ownership. Supported journal records now transfer as raw field bytes, and malformed or ambiguous cases refuse note writes. Production marker-writing actions stay disabled until shared editor/source/search/copy/accessibility hiding exists. The parser/describer remains enabled. ## implementation plan and costs - Preserve supported journal fields through a selected-record splice and byte relocation, not YAML reserialization. Rename title to summary, preserve explicit nulls/types/comments/unknown fields, and derive the owner title from authority. - Recover durable promotion jobs and missing claim finalization under the user lock before a new request. Reserve by source fingerprint plus exact snapshot version; separate verified durable undo from outside marker removal. - Inventory current Home notes for owners and all block markers; durably protect observed ordinary addresses. Refuse duplicate owners, same-note copies without provenance, unsupported graph copies and unavailable lookups. - Release-gate write actions pending the shared marker UI contract. Management reads Home bytes and promotion history; it is not a point-lookup-only operation. Nested relationship scans and source-prefix/history queries can scale poorly. Claims grow with every observed/minted/adopted address. No 100k size/latency result is claimed. Inventory line classification uses a HashSet. ## failure table | Failure | Effect | Detection | Recovery | Test | |---|---|---|---|---| | Unsupported or ambiguous journal record | unsafe transfer | strict entry and field validation | refuse without changing note bytes | review_present_unsupported_entries_refuse | | Authority differs from line, or contains nulls/comments | lost fields | exact transfer golden | raw field relocation, authoritative owner title | review_authority_moves_with_exact_types_comments_and_bytes | | Old source reappears after move/delete | live ID reassigned | versioned durable source claim and Home inventory | refuse; explicit durable undo is separate | review_moved_source_cannot_reclaim_live_identity | | Job completes before ownership finalization | second owner on new request | recover promotion jobs before new requests | drive job and finalize missing claim | review_replay_then_new_key_cannot_create_an_owner | | Paste before original / owner copy | original binding guessed | duplicate markers or owners | refuse; retain original bytes | review_paste_before_original_refuses_without_rekeying, review_copied_owner_notes_refuse_without_writes | | Ordinary address collision or removed observed block | ID reused | current inventory plus permanent ordinary claims | refuse reuse | review_observed_ordinary_ids_stay_claimed_after_removal | | Index unavailable | unsafe free decision | lookup errors | fail closed | review_unavailable_index_never_means_free | | Marker UI absent | literal IDs exposed | production action contract | expose no write actions | review_marker_writes_are_not_exposed_before_ui_support | ## round 1 implementation fixes Head `771eb102`, rebased onto #783 `3f94719e`, which includes #781 `f92341a6`. No merge. Production marker-writing actions are disabled until the shared UI hiding contract exists. This is backend hardening, not completed step-4 UI delivery. | Finding | Status / regression | |---|---| | P0 journal authority lost on promotion | Fixed. Supported block records move as raw field bytes, with title renamed to summary; authoritative title controls owner H1/path/link and journal tags override line tags. Nulls, types, unknown/nested fields and comments survive. `review_authority_moves_with_exact_types_comments_and_bytes` asserts exact source/destination goldens; `review_journal_authority_is_preserved_or_refused` exercises the service. | | P1 moved/deleted/unmarked source reuses a live ID | Fixed by conservative refusal. Reservations include exact snapshot version; live/retired source matches cannot be reclaimed by coincident words. Explicit durable undo recovery is separate. `review_moved_source_cannot_reclaim_live_identity`, `review_deleted_or_unmarked_source_refuses_reidentification`; clean undo still passes. | | P1 replay leaves owner claim unfinished | Fixed. Under the user lock, recover promotion jobs and finalize claims before accepting new identify/promote requests. `review_replay_then_new_key_cannot_create_an_owner`, `review_recovery_at_each_note_boundary_precedes_new_key` cover stored/create/edit boundary states followed by a NEW key. | | P1 present unsupported authority mistaken for absent | Fixed. Strict entry lookup distinguishes absent from unsupported. Flow envelopes/records, scalar/list/empty records and ambiguous/reserved fields refuse before note writes. `review_present_unsupported_entries_refuse`. | | P1 first occurrence guesses original after paste-before | Fixed by refusal. Same-note duplicates and unproven imported duplicates retain all note bytes. Owned/child-linked/journal-authority copies also refuse. `review_paste_before_original_refuses_without_rekeying`; pure settle returns Ambiguous. Proven bare cross-note copies can still receive fresh IDs. | | P1 adoption ignores owners/ordinary blocks/index errors | Fixed. Current Home inventory adopts a unique existing owner, rejects duplicate owners and ordinary-marker collisions, and durably protects observed ordinary addresses after removal. Index errors fail closed and mint retries are bounded. `review_owner_import_is_adopted_before_promotion`, `review_copied_owner_notes_refuse_without_writes`, `review_ordinary_block_collision_refuses`, `review_observed_ordinary_ids_stay_claimed_after_removal`, `review_unavailable_index_never_means_free`. | | P1 literal markers exposed before UI hiding | Release-gated. `EventsPlugin::new` exposes no identify/promote actions; published action contract removes them. Backend tests use a test-only harness. `review_marker_writes_are_not_exposed_before_ui_support`. UI hiding itself remains unimplemented and must land before enabling writes. | | Audit: stale request settles unrelated markers | Fixed. Validate target before settlement; identify/promote settle only the selected target. Existing stale-target regression passes. | | Audit: weak no-nested-links assertion | Tightened to require exactly one parsed child link, rather than allowing zero. 3,000 generated cases pass. Full destination-resolution proof remains follow-up. | Red evidence on dev-01: `~/cal-r1-s4-red.log`, `~/cal-r1-s4-replay-red.log`, `~/cal-r1-s4-more-red.log`, `~/cal-r1-s4-retired-red.log`. The new regressions were observed failing before their fixes. Boundary recovery tests simulate durable states and reopen; they are not new OS-kill tests at every boundary. Fresh gates on dev-01 in the Nix shell, heavy commands via `~/bin/slot`: ```sh cargo fmt --all && cargo fmt --all -- --check ~/bin/slot cargo clippy -p calternal-events -p calternal-notes -p calternal-tasks -p calternal --all-targets -j4 -- -D warnings ~/bin/slot cargo test -p calternal-events -p calternal-notes -p calternal-tasks -p calternal --no-fail-fast -j4 ``` All passed in `~/cal-r1-s4-final2.log`: events 57, notes 438 / 6 ignored, tasks 29, and calternal unit/integration checks (existing ignored cases remain ignored). The final inventory classification HashSet refactor subsequently passed fresh fmt/check, the same four-crate clippy, and `~/bin/slot cargo test -p calternal-events -j4` (57 passed), log `~/cal-r1-s4-lookup.log`. Nix flake scaffolding was not committed. Decisions and remaining limits: - Prefer refusal to guessing identity or rewriting unsupported YAML. CRLF/BOM inputs inherit #781 safe refusal. Transfer changes only the selected record and relocates its fields; an emptied journal map remains `events:` rather than rewriting surrounding bytes. - Management now reads current Home bytes and promotion history, rather than claiming point-lookup-only work. Inventory classification uses a line HashSet; overall nested scans and source-prefix/history queries can still scale poorly. No 100k identity/performance result is claimed. - The claim store protects observed ordinary addresses permanently. Unobserved historical ordinary IDs retain ADR 0068 O28 cold-import provenance limits. - UI editor/source/search/copy/accessibility hiding is deferred with write actions disabled. No frontend/browser validation is claimed. - Actual process kills at every new create/edit/finalization boundary, concurrent outside edits at the claim/write boundary, intervening-edit and transferred-entry undo combinations, full child-link destination resolution, and large cold-index measurements remain validation gaps. Existing inherited crash/undo suites passed; these are not substitutes for the missing combinations. - Inherited open P2/P3: #781 shared expected-value oracle; #783 repeated nested relationship traversal and its listed audit/performance gaps. This addresses round 1. No round-2 approval, merge approval, or merge is claimed.
kayg added 13 commits 2026-10-31 05:48:27 +00:00
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
events: pure event parser and describe rows
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
5c1447ab68
events: skip notes with no date heading early
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
735106a0a3
kayg commented 2026-10-31 16:20:05 +00:00
Author
Owner
Copy link

Independent review, round 1

Verdict: fix first. Reviewed b30cc8a2ac04a4b7816f52eb9e692965117210f5 relative to #783, after ADRs 0067/0068/0056/0066. This is a source-level review of implementation and tests, not a fresh full-suite passing claim. Inherited #781/#783 blockers also apply.

  1. P0 — Promotion deletes journal fields it does not preserve. crates/events/src/promote.rs:241, :254, :229, and :140. carry drops title and empty/null fields, and changes appends line tags after carried tags so they overwrite the authoritative entry. Example: a line - 10:00 Walk #outside ^<id> with events.<id>.title: Private title, tags: [private], and an explicitly cleared optional field loses those values when the entire journal entry is removed. Entry comments are also removed without relocation. The existing transfer test uses the same line/entry title and no conflicting tags/nulls, so it cannot catch this. Fix: transfer the complete authority losslessly (including presence/types and retained comments), derive the owner title from that authority, and refuse unsupported transfers before any note writes. Add exact source/destination goldens with a differing title, tags, nulls, comments and unknown fields.

  2. P1 — A moved event's old source key can allocate its live ID to another line. crates/events/src/service.rs:279, crates/events/src/claims.rs:125. Identify - 10:00 Walk in A as U; cut/paste it into B and identify B (claim holder becomes B). Type a new unmarked - 10:00 Walk in A. Its old source key still resolves to U; reserve tests only A's text, bypasses free_in_index for an existing source claim, and returns U although B still carries it. confirm then assigns the holder back to A. Fix: validate existing source reservations against Home-wide live/retired ownership and provenance; distinguish crash/explicit undo recovery from a new block with coincident words. Add move-then-recreate, delete-then-recreate and outside-marker-removal ambiguity tests. Source text plus nth occurrence is not the stable block/version provenance required by O06.

  3. P1 — Startup replay does not finish ownership claims. crates/events/src/service_promote.rs:244 and :110. confirm/set_owner happen only in finished, after the Notes bulk job. Kill after the plan is stored or after its steps finish but before finished. Notes startup can complete both files while claims.owner remains NULL. A subsequent promotion with a NEW key then creates a second owner for U. The existing crash test retries K first, which calls finished and masks the gap. Fix: make claim/owner finalization a durable replayed step or recover it before allowing new identity/promotion requests. Test restart followed by a new key, plus crash after each create/edit/finalization boundary.

  4. P1 — Unreadable/unsupported existing journal entries become “no entry.” crates/events/src/service_promote.rs:174. A valid YAML flow record events: {<id>: {start: ...}}, or a block events map with a flow-map record, makes pairs_at return None. The planner receives entry=None, creates an owner from line clocks, and leaves the old authority untouched. The unsupported-transfer refusal only works when Some(entry) reaches it. Fix: distinguish absent entry from present-but-unreadable/unsupported entry; refuse the latter without writes. Add real service tests for flow entries, scalar/list records and empty records, not only a pure-planner nested source value.

  5. P1 — First-occurrence tie-breaking can give the original block a new identity. crates/events/src/settle.rs:55 and crates/events/src/service_settle.rs:43. Copy an identified line and paste it ABOVE the original in the same note. Claims remember only the note, so Holder::This gives U to the first occurrence (the copy) and rekeys the original. Existing links silently target the wrong block. Fix: persist block provenance sufficient to recognize the original, or retain the ambiguity without guessing or exporting until resolved. Add paste-before-original tests, including copies with their children/owner links. The existing copy test puts duplicates in another note; it does not prove the original keeps its ID within one note.

  6. P1 — The Home-wide check does not cover adoption/ordinary blocks or existing owner notes. crates/events/src/service_settle.rs:43, crates/events/src/service.rs:171, crates/events/src/service_promote.rs:110. An imported line carrying U with no claims row is adopted without consulting existing event_notes or non-event blocks. An imported/copied owner note is not settled at all; has_owner reads only claims, so an existing owner with event-id: U can be followed by another newly promoted owner. Minting checks event tables only and treats index errors as free. Fix: use one authoritative Home-wide block/event claim lifecycle, consult/adopt existing owners safely, and fail closed on lookup errors. Test owner-note imports/copies and collisions with headings/ordinary linked blocks, including an unavailable index.

  7. P1 — Written markers are not hidden by the shipped editor. crates/events/src/service.rs:419 writes literal ^id; packages/editor/src/syntax/hidden.ts:80 has no marker decoration/atomic rule, and there is no shared client marker parser or clipboard/accessibility filtering in this stack. The Rust parser removing markers from event titles does not hide them in note/source editing. Fix: integrate the shared marker contract into editor/source/search/copy/accessibility before exposing marker-writing actions, and add corresponding UI tests (including legacy IDs and export retention). This was checked statically; no browser run is claimed.

Failure-table audit:

  • Reserve-before-marker crash: a graceful shutdown/reopen test simulates the reserved state; it does not kill the process at that boundary. It covers the simple same-source retry, not finding 2.
  • Stored job/mid-step crash and restart-before-retry: the test stores an UNRUN job, gracefully reopens, waits for done, then retries the original key. It demonstrates that path; it does not cover mid-step kills or finalization without the retry (finding 3).
  • Same request twice / outside line from multiple requests: exact-input retry and sequential identify tests exist. No concurrent/outside-edit race at the claim/write boundary is exercised.
  • Copies, moves and imports: basic marked-line fixtures exist; move-then-recreate, same-note paste-before-original, owner-note graphs and non-event block collisions are missing (findings 2, 5, 6).
  • Outside marker removal: despite its name, removing_the_marker_outside_mints_a_new_id asserts SAME ID for the same words. It cannot establish matching provenance or safe behavior when an old block was deleted/moved.
  • Malformed frontmatter: LF malformed-list refusal is covered. CRLF/BOM bypasses inherited from #781 are not covered.
  • Whole journal transfer: differing authority, nulls/comments/types and flow entries are not covered (findings 1, 4).
  • Title/no nested links: goldens and 3,000 generated cases exist. no_nested_links permits child.len() <= 1, including zero parsed links, so it does not prove every added relationship is usable. Assert exactly one complete child link and resolved destination.
  • Source changed during request: test supplies an already-wrong expect; it does not edit after locate/before applying the bulk step. Also settle_note runs before target validation and can mutate unrelated duplicate markers even when the requested target is stale.
  • Undo after others edited: undo_is_exact_and_the_same_id_comes_back covers a clean promotion/undo only. There is no intervening editor change in that test, no transferred journal-entry undo case, and no collapse/rekey-plus-promotion one-step undo test. Existing bulk behavior is inherited, not proof of these combinations.

Mergeability: on dev-01, after fetching this exact head, git merge-tree --write-tree origin/dev b30cc8a2 exits 0 against dev=2f304154054ec68b7855bb1f2bd24b1fb15a386d (tree 0ee41bab9a1a535311a7266b1c097a25f560f1b8). Forgejo still reports false. #781 → #783 → #785 explains stacked commits, not a reproduced Git conflict. The precise Forgejo-side reason is unproven. No code pushed or merged.

# Independent review, round 1 **Verdict: fix first.** Reviewed `b30cc8a2ac04a4b7816f52eb9e692965117210f5` relative to #783, after ADRs 0067/0068/0056/0066. This is a source-level review of implementation and tests, not a fresh full-suite passing claim. Inherited #781/#783 blockers also apply. 1. **P0 — Promotion deletes journal fields it does not preserve.** `crates/events/src/promote.rs:241`, `:254`, `:229`, and `:140`. `carry` drops `title` and empty/null fields, and `changes` appends line `tags` after carried `tags` so they overwrite the authoritative entry. Example: a line `- 10:00 Walk #outside ^<id>` with `events.<id>.title: Private title`, `tags: [private]`, and an explicitly cleared optional field loses those values when the entire journal entry is removed. Entry comments are also removed without relocation. The existing transfer test uses the same line/entry title and no conflicting tags/nulls, so it cannot catch this. **Fix:** transfer the complete authority losslessly (including presence/types and retained comments), derive the owner title from that authority, and refuse unsupported transfers before any note writes. Add exact source/destination goldens with a differing title, tags, nulls, comments and unknown fields. 2. **P1 — A moved event's old source key can allocate its live ID to another line.** `crates/events/src/service.rs:279`, `crates/events/src/claims.rs:125`. Identify `- 10:00 Walk` in A as U; cut/paste it into B and identify B (claim holder becomes B). Type a new unmarked `- 10:00 Walk` in A. Its old source key still resolves to U; `reserve` tests only A's text, bypasses `free_in_index` for an existing source claim, and returns U although B still carries it. `confirm` then assigns the holder back to A. **Fix:** validate existing source reservations against Home-wide live/retired ownership and provenance; distinguish crash/explicit undo recovery from a new block with coincident words. Add move-then-recreate, delete-then-recreate and outside-marker-removal ambiguity tests. Source text plus nth occurrence is not the stable block/version provenance required by O06. 3. **P1 — Startup replay does not finish ownership claims.** `crates/events/src/service_promote.rs:244` and `:110`. `confirm`/`set_owner` happen only in `finished`, after the Notes bulk job. Kill after the plan is stored or after its steps finish but before `finished`. Notes startup can complete both files while `claims.owner` remains NULL. A subsequent promotion with a NEW key then creates a second owner for U. The existing crash test retries K first, which calls `finished` and masks the gap. **Fix:** make claim/owner finalization a durable replayed step or recover it before allowing new identity/promotion requests. Test restart followed by a new key, plus crash after each create/edit/finalization boundary. 4. **P1 — Unreadable/unsupported existing journal entries become “no entry.”** `crates/events/src/service_promote.rs:174`. A valid YAML flow record `events: {<id>: {start: ...}}`, or a block `events` map with a flow-map record, makes `pairs_at` return None. The planner receives `entry=None`, creates an owner from line clocks, and leaves the old authority untouched. The unsupported-transfer refusal only works when `Some(entry)` reaches it. **Fix:** distinguish absent entry from present-but-unreadable/unsupported entry; refuse the latter without writes. Add real service tests for flow entries, scalar/list records and empty records, not only a pure-planner nested `source` value. 5. **P1 — First-occurrence tie-breaking can give the original block a new identity.** `crates/events/src/settle.rs:55` and `crates/events/src/service_settle.rs:43`. Copy an identified line and paste it ABOVE the original in the same note. Claims remember only the note, so `Holder::This` gives U to the first occurrence (the copy) and rekeys the original. Existing links silently target the wrong block. **Fix:** persist block provenance sufficient to recognize the original, or retain the ambiguity without guessing or exporting until resolved. Add paste-before-original tests, including copies with their children/owner links. The existing copy test puts duplicates in another note; it does not prove the original keeps its ID within one note. 6. **P1 — The Home-wide check does not cover adoption/ordinary blocks or existing owner notes.** `crates/events/src/service_settle.rs:43`, `crates/events/src/service.rs:171`, `crates/events/src/service_promote.rs:110`. An imported line carrying U with no claims row is adopted without consulting existing `event_notes` or non-event blocks. An imported/copied owner note is not settled at all; `has_owner` reads only claims, so an existing owner with `event-id: U` can be followed by another newly promoted owner. Minting checks event tables only and treats index errors as free. **Fix:** use one authoritative Home-wide block/event claim lifecycle, consult/adopt existing owners safely, and fail closed on lookup errors. Test owner-note imports/copies and collisions with headings/ordinary linked blocks, including an unavailable index. 7. **P1 — Written markers are not hidden by the shipped editor.** `crates/events/src/service.rs:419` writes literal ` ^id`; `packages/editor/src/syntax/hidden.ts:80` has no marker decoration/atomic rule, and there is no shared client marker parser or clipboard/accessibility filtering in this stack. The Rust parser removing markers from event titles does not hide them in note/source editing. **Fix:** integrate the shared marker contract into editor/source/search/copy/accessibility before exposing marker-writing actions, and add corresponding UI tests (including legacy IDs and export retention). This was checked statically; no browser run is claimed. Failure-table audit: - Reserve-before-marker crash: a graceful shutdown/reopen test simulates the reserved state; it does not kill the process at that boundary. It covers the simple same-source retry, not finding 2. - Stored job/mid-step crash and restart-before-retry: the test stores an UNRUN job, gracefully reopens, waits for done, then retries the original key. It demonstrates that path; it does not cover mid-step kills or finalization without the retry (finding 3). - Same request twice / outside line from multiple requests: exact-input retry and sequential identify tests exist. No concurrent/outside-edit race at the claim/write boundary is exercised. - Copies, moves and imports: basic marked-line fixtures exist; move-then-recreate, same-note paste-before-original, owner-note graphs and non-event block collisions are missing (findings 2, 5, 6). - Outside marker removal: despite its name, `removing_the_marker_outside_mints_a_new_id` asserts SAME ID for the same words. It cannot establish matching provenance or safe behavior when an old block was deleted/moved. - Malformed frontmatter: LF malformed-list refusal is covered. CRLF/BOM bypasses inherited from #781 are not covered. - Whole journal transfer: differing authority, nulls/comments/types and flow entries are not covered (findings 1, 4). - Title/no nested links: goldens and 3,000 generated cases exist. `no_nested_links` permits `child.len() <= 1`, including zero parsed links, so it does not prove every added relationship is usable. Assert exactly one complete child link and resolved destination. - Source changed during request: test supplies an already-wrong `expect`; it does not edit after locate/before applying the bulk step. Also `settle_note` runs before target validation and can mutate unrelated duplicate markers even when the requested target is stale. - Undo after others edited: `undo_is_exact_and_the_same_id_comes_back` covers a clean promotion/undo only. There is no intervening editor change in that test, no transferred journal-entry undo case, and no collapse/rekey-plus-promotion one-step undo test. Existing bulk behavior is inherited, not proof of these combinations. Mergeability: on dev-01, after fetching this exact head, `git merge-tree --write-tree origin/dev b30cc8a2` exits 0 against `dev=2f304154054ec68b7855bb1f2bd24b1fb15a386d` (tree `0ee41bab9a1a535311a7266b1c097a25f560f1b8`). Forgejo still reports false. #781 → #783 → #785 explains stacked commits, not a reproduced Git conflict. The precise Forgejo-side reason is unproven. No code pushed or merged.
kayg commented 2026-10-31 16:56:26 +00:00
Author
Owner
Copy link

round 1 implementation fixes

Head 771eb102, rebased onto #783 3f94719e, which includes #781 f92341a6. No merge. Production marker-writing actions are disabled until the shared UI hiding contract exists. This is backend hardening, not completed step-4 UI delivery.

Finding Status / regression
P0 journal authority lost on promotion Fixed. Supported block records move as raw field bytes, with title renamed to summary; authoritative title controls owner H1/path/link and journal tags override line tags. Nulls, types, unknown/nested fields and comments survive. review_authority_moves_with_exact_types_comments_and_bytes asserts exact source/destination goldens; review_journal_authority_is_preserved_or_refused exercises the service.
P1 moved/deleted/unmarked source reuses a live ID Fixed by conservative refusal. Reservations include exact snapshot version; live/retired source matches cannot be reclaimed by coincident words. Explicit durable undo recovery is separate. review_moved_source_cannot_reclaim_live_identity, review_deleted_or_unmarked_source_refuses_reidentification; clean undo still passes.
P1 replay leaves owner claim unfinished Fixed. Under the user lock, recover promotion jobs and finalize claims before accepting new identify/promote requests. review_replay_then_new_key_cannot_create_an_owner, review_recovery_at_each_note_boundary_precedes_new_key cover stored/create/edit boundary states followed by a NEW key.
P1 present unsupported authority mistaken for absent Fixed. Strict entry lookup distinguishes absent from unsupported. Flow envelopes/records, scalar/list/empty records and ambiguous/reserved fields refuse before note writes. review_present_unsupported_entries_refuse.
P1 first occurrence guesses original after paste-before Fixed by refusal. Same-note duplicates and unproven imported duplicates retain all note bytes. Owned/child-linked/journal-authority copies also refuse. review_paste_before_original_refuses_without_rekeying; pure settle returns Ambiguous. Proven bare cross-note copies can still receive fresh IDs.
P1 adoption ignores owners/ordinary blocks/index errors Fixed. Current Home inventory adopts a unique existing owner, rejects duplicate owners and ordinary-marker collisions, and durably protects observed ordinary addresses after removal. Index errors fail closed and mint retries are bounded. review_owner_import_is_adopted_before_promotion, review_copied_owner_notes_refuse_without_writes, review_ordinary_block_collision_refuses, review_observed_ordinary_ids_stay_claimed_after_removal, review_unavailable_index_never_means_free.
P1 literal markers exposed before UI hiding Release-gated. EventsPlugin::new exposes no identify/promote actions; published action contract removes them. Backend tests use a test-only harness. review_marker_writes_are_not_exposed_before_ui_support. UI hiding itself remains unimplemented and must land before enabling writes.
Audit: stale request settles unrelated markers Fixed. Validate target before settlement; identify/promote settle only the selected target. Existing stale-target regression passes.
Audit: weak no-nested-links assertion Tightened to require exactly one parsed child link, rather than allowing zero. 3,000 generated cases pass. Full destination-resolution proof remains follow-up.

Red evidence on dev-01: ~/cal-r1-s4-red.log, ~/cal-r1-s4-replay-red.log, ~/cal-r1-s4-more-red.log, ~/cal-r1-s4-retired-red.log. The new regressions were observed failing before their fixes. Boundary recovery tests simulate durable states and reopen; they are not new OS-kill tests at every boundary.

Fresh gates on dev-01 in the Nix shell, heavy commands via ~/bin/slot:

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

All passed in ~/cal-r1-s4-final2.log: events 57, notes 438 / 6 ignored, tasks 29, and calternal unit/integration checks (existing ignored cases remain ignored). The final inventory classification HashSet refactor subsequently passed fresh fmt/check, the same four-crate clippy, and ~/bin/slot cargo test -p calternal-events -j4 (57 passed), log ~/cal-r1-s4-lookup.log. Nix flake scaffolding was not committed.

Decisions and remaining limits:

  • Prefer refusal to guessing identity or rewriting unsupported YAML. CRLF/BOM inputs inherit #781 safe refusal. Transfer changes only the selected record and relocates its fields; an emptied journal map remains events: rather than rewriting surrounding bytes.
  • Management now reads current Home bytes and promotion history, rather than claiming point-lookup-only work. Inventory classification uses a line HashSet; overall nested scans and source-prefix/history queries can still scale poorly. No 100k identity/performance result is claimed.
  • The claim store protects observed ordinary addresses permanently. Unobserved historical ordinary IDs retain ADR 0068 O28 cold-import provenance limits.
  • UI editor/source/search/copy/accessibility hiding is deferred with write actions disabled. No frontend/browser validation is claimed.
  • Actual process kills at every new create/edit/finalization boundary, concurrent outside edits at the claim/write boundary, intervening-edit and transferred-entry undo combinations, full child-link destination resolution, and large cold-index measurements remain validation gaps. Existing inherited crash/undo suites passed; these are not substitutes for the missing combinations.
  • Inherited open P2/P3: #781 shared expected-value oracle; #783 repeated nested relationship traversal and its listed audit/performance gaps.

This addresses round 1. No round-2 approval, merge approval, or merge is claimed.

## round 1 implementation fixes Head `771eb102`, rebased onto #783 `3f94719e`, which includes #781 `f92341a6`. No merge. Production marker-writing actions are disabled until the shared UI hiding contract exists. This is backend hardening, not completed step-4 UI delivery. | Finding | Status / regression | |---|---| | P0 journal authority lost on promotion | Fixed. Supported block records move as raw field bytes, with title renamed to summary; authoritative title controls owner H1/path/link and journal tags override line tags. Nulls, types, unknown/nested fields and comments survive. `review_authority_moves_with_exact_types_comments_and_bytes` asserts exact source/destination goldens; `review_journal_authority_is_preserved_or_refused` exercises the service. | | P1 moved/deleted/unmarked source reuses a live ID | Fixed by conservative refusal. Reservations include exact snapshot version; live/retired source matches cannot be reclaimed by coincident words. Explicit durable undo recovery is separate. `review_moved_source_cannot_reclaim_live_identity`, `review_deleted_or_unmarked_source_refuses_reidentification`; clean undo still passes. | | P1 replay leaves owner claim unfinished | Fixed. Under the user lock, recover promotion jobs and finalize claims before accepting new identify/promote requests. `review_replay_then_new_key_cannot_create_an_owner`, `review_recovery_at_each_note_boundary_precedes_new_key` cover stored/create/edit boundary states followed by a NEW key. | | P1 present unsupported authority mistaken for absent | Fixed. Strict entry lookup distinguishes absent from unsupported. Flow envelopes/records, scalar/list/empty records and ambiguous/reserved fields refuse before note writes. `review_present_unsupported_entries_refuse`. | | P1 first occurrence guesses original after paste-before | Fixed by refusal. Same-note duplicates and unproven imported duplicates retain all note bytes. Owned/child-linked/journal-authority copies also refuse. `review_paste_before_original_refuses_without_rekeying`; pure settle returns Ambiguous. Proven bare cross-note copies can still receive fresh IDs. | | P1 adoption ignores owners/ordinary blocks/index errors | Fixed. Current Home inventory adopts a unique existing owner, rejects duplicate owners and ordinary-marker collisions, and durably protects observed ordinary addresses after removal. Index errors fail closed and mint retries are bounded. `review_owner_import_is_adopted_before_promotion`, `review_copied_owner_notes_refuse_without_writes`, `review_ordinary_block_collision_refuses`, `review_observed_ordinary_ids_stay_claimed_after_removal`, `review_unavailable_index_never_means_free`. | | P1 literal markers exposed before UI hiding | Release-gated. `EventsPlugin::new` exposes no identify/promote actions; published action contract removes them. Backend tests use a test-only harness. `review_marker_writes_are_not_exposed_before_ui_support`. UI hiding itself remains unimplemented and must land before enabling writes. | | Audit: stale request settles unrelated markers | Fixed. Validate target before settlement; identify/promote settle only the selected target. Existing stale-target regression passes. | | Audit: weak no-nested-links assertion | Tightened to require exactly one parsed child link, rather than allowing zero. 3,000 generated cases pass. Full destination-resolution proof remains follow-up. | Red evidence on dev-01: `~/cal-r1-s4-red.log`, `~/cal-r1-s4-replay-red.log`, `~/cal-r1-s4-more-red.log`, `~/cal-r1-s4-retired-red.log`. The new regressions were observed failing before their fixes. Boundary recovery tests simulate durable states and reopen; they are not new OS-kill tests at every boundary. Fresh gates on dev-01 in the Nix shell, heavy commands via `~/bin/slot`: ```sh cargo fmt --all && cargo fmt --all -- --check ~/bin/slot cargo clippy -p calternal-events -p calternal-notes -p calternal-tasks -p calternal --all-targets -j4 -- -D warnings ~/bin/slot cargo test -p calternal-events -p calternal-notes -p calternal-tasks -p calternal --no-fail-fast -j4 ``` All passed in `~/cal-r1-s4-final2.log`: events 57, notes 438 / 6 ignored, tasks 29, and calternal unit/integration checks (existing ignored cases remain ignored). The final inventory classification HashSet refactor subsequently passed fresh fmt/check, the same four-crate clippy, and `~/bin/slot cargo test -p calternal-events -j4` (57 passed), log `~/cal-r1-s4-lookup.log`. Nix flake scaffolding was not committed. Decisions and remaining limits: - Prefer refusal to guessing identity or rewriting unsupported YAML. CRLF/BOM inputs inherit #781 safe refusal. Transfer changes only the selected record and relocates its fields; an emptied journal map remains `events:` rather than rewriting surrounding bytes. - Management now reads current Home bytes and promotion history, rather than claiming point-lookup-only work. Inventory classification uses a line HashSet; overall nested scans and source-prefix/history queries can still scale poorly. No 100k identity/performance result is claimed. - The claim store protects observed ordinary addresses permanently. Unobserved historical ordinary IDs retain ADR 0068 O28 cold-import provenance limits. - UI editor/source/search/copy/accessibility hiding is deferred with write actions disabled. No frontend/browser validation is claimed. - Actual process kills at every new create/edit/finalization boundary, concurrent outside edits at the claim/write boundary, intervening-edit and transferred-entry undo combinations, full child-link destination resolution, and large cold-index measurements remain validation gaps. Existing inherited crash/undo suites passed; these are not substitutes for the missing combinations. - Inherited open P2/P3: #781 shared expected-value oracle; #783 repeated nested relationship traversal and its listed audit/performance gaps. This addresses round 1. No round-2 approval, merge approval, or merge is claimed.
kayg commented 2026-10-31 17:10:33 +00:00
Author
Owner
Copy link

Independent review, round 2

Verdict: fix first. Reviewed 771eb10203c86e49339d211e8f25e16ae0f2ce20 relative to #783, both round-1 comments, and ADRs 0067/0068/0056/0066. No prior round-2 comment existed when checked. No code pushed or merged. Remaining #781 comment loss and #783 inline-comment visibility findings also apply.

Round-1 finding Round-2 status Does the test prove it?
P0 promotion loses journal authority Verified for the reported title/tags/null/type/comment cases; relocation semantics still fail, below. Pure-plan review_authority_moves_with_exact_types_comments_and_bytes has independent exact source/destination goldens. Service review_journal_authority_is_preserved_or_refused deliberately allows refusal; it proves preservation OR no writes, not successful transfer for every fixture. Neither checks relative-link targets.
P1 moved/deleted/unmarked source reuses live ID Verified for reported scenarios. Move/recreate test forbids reusing U (allows a different ID or refusal). Delete/unmark test requires refusal and exact unchanged text. Versioned reservation plus the live/retired refusal addresses the cited bug; these are not concurrency-at-write-boundary tests.
P1 replay leaves owner claim unfinished Verified for a subsequent service request. review_replay_then_new_key_cannot_create_an_owner and review_recovery_at_each_note_boundary_precedes_new_key test NEW keys and one owner after stored/create/edit boundary simulations. These are graceful reopen/synthetic durable-state tests, not OS kills or proof claims finalize during Notes startup without a service request.
P1 present unsupported entry mistaken for absent Verified for reported flow/scalar/list/empty records. review_present_unsupported_entries_refuse exercises the service, asserts refusal, unchanged source and no destination creation. Strict lookup now propagates unsupported-parent errors.
P1 paste-before-original guesses ownership Verified by refusal for the cited same-note case. review_paste_before_original_refuses_without_rekeying checks an above-original copy with owner-link children and unchanged whole text. It does not claim full fresh-graph copy support.
P1 adoption ignores existing owners/ordinary markers/index errors Verified for reported fixtures. Unique-owner adoption, duplicate-owner refusal, ordinary collision, observed-ordinary removal and unavailable-index tests exercise the individual cases. review_unavailable_index_never_means_free is a helper-level test, not a full failed-index promotion. Cold-import history remains limited as documented.
P1 markers exposed before UI hiding Not fully fixed under the requested “no production path can write markers” gate; fresh action dispatch is disabled. review_marker_writes_are_not_exposed_before_ui_support only asserts EventsPlugin::new(...).actions().is_empty(). It proves the registry gate, not queued replay; see finding 2. with_writes is genuinely test-only.
Audit: stale request settles unrelated markers Verified for the cited stale-target path. Both entrypoints locate before settlement and select the target; existing stale-target regression covers this. It is not a proof against an outside edit between lookup and write.
Audit: weak child-link assertion Verified improvement, incomplete destination proof. Generated plan cases now require exactly one parsed child; that still does not prove every link resolves, including moved authority links below.

New P1 — Raw journal transfer changes relative-link meaning. crates/events/src/transfer.rs:58, crates/events/src/promote.rs:172. A source Daily/2026/2026-10-26.md with events.<U>.people: ['[Alice](<../../People/Alice.md>)'] resolves Alice to People/Alice.md. Promotion to default Calendar/Walk.md dedents/copies the bytes and deletes the original record, leaving that same path on the owner; from Calendar/ it climbs outside Notes and no longer resolves. The probe observed a successful plan and Some("People/Alice.md") before versus None after. Different source/destination folders can also silently retarget a link to a different existing person. Preserving raw YAML does not preserve relative semantic bindings (0068 O04). Fix: relocate recognized semantic links and their durable reference bindings together with the record, preserving unrelated bytes; refuse before any note writes when lossless rebasing is unsupported. Add source/destination fixtures at different depths and assert resolved UUID/target equality, including an occupied alternative path. This gap is in the new raw-transfer implementation; no enabled network action is claimed.

New P1 — The release gate does not cover durable event-job replay. crates/events/src/actions.rs:40, crates/notes/src/lib.rs:285, crates/notes/src/bulk/mod.rs:596 and :608. Open a Home containing a queued/running events-identify, events-promote or events-settle plan from the earlier write-enabled build. Notes startup unconditionally resumes all running jobs and drives their stored Create/Edit steps, including marker insertion/rekeying, regardless of the plugin's empty action list. UI hiding is still absent. The registry-only regression never reopens with the production plugin against a pending event plan. This is a static reachability finding with the stated persisted-plan precondition, not a claim that a clean Home can dispatch the removed actions. The service's public write methods also remain ungated, although I found no other current production caller of them. Fix: put the release decision at the actual event write/replay boundary, with a durable deferred state for pending plans until UI support exists (do not discard their content/history), or complete hiding before allowing their replay. Test a stored plan and a create-completed/edit-pending plan across reopen using EventsPlugin::new, and verify source markers cannot be written while disabled. Coordinate this explicitly with crash durability rather than silently dropping jobs.

Fresh verification on dev-01, exact head above, in the Nix shell with heavy commands through ~/bin/slot:

  • cargo test -p calternal-events review_ -j4: 17 passed, 40 filtered out.
  • cargo test -p calternal-notes review_ -j4: 24 passed, 420 filtered out.
  • Focused standalone Rust probe compiled through ~/bin/slot rustc --edition=2024 /tmp/cal-r2-probe.rs ... -o /tmp/cal-r2-probe, linking current cached stack libraries: observed relative-link breakage, inherited inline-comment deletion and commented relationships. Source/output on dev-01: /tmp/cal-r2-probe.rs, /tmp/cal-r2-probe.out. The inherited props/exclusion helpers are unchanged from the respective lower stack layers.

No fresh full-suite, release-perf, 100k inventory or browser gate is claimed. Existing fix-status limitations remain: real process kills at the new boundaries, concurrent outside edits, intervening-edit/transferred-authority undo combinations, Home-size management cost, and complete child-link destination/reference binding. Disabling fresh actions is useful containment, but it neither completes step 4 nor makes the persisted replay path read-only.

# Independent review, round 2 **Verdict: fix first.** Reviewed `771eb10203c86e49339d211e8f25e16ae0f2ce20` relative to #783, both round-1 comments, and ADRs 0067/0068/0056/0066. No prior round-2 comment existed when checked. No code pushed or merged. Remaining #781 comment loss and #783 inline-comment visibility findings also apply. | Round-1 finding | Round-2 status | Does the test prove it? | |---|---|---| | P0 promotion loses journal authority | **Verified for the reported title/tags/null/type/comment cases**; relocation semantics still fail, below. | Pure-plan `review_authority_moves_with_exact_types_comments_and_bytes` has independent exact source/destination goldens. Service `review_journal_authority_is_preserved_or_refused` deliberately allows refusal; it proves preservation OR no writes, not successful transfer for every fixture. Neither checks relative-link targets. | | P1 moved/deleted/unmarked source reuses live ID | **Verified** for reported scenarios. | Move/recreate test forbids reusing U (allows a different ID or refusal). Delete/unmark test requires refusal and exact unchanged text. Versioned reservation plus the live/retired refusal addresses the cited bug; these are not concurrency-at-write-boundary tests. | | P1 replay leaves owner claim unfinished | **Verified** for a subsequent service request. | `review_replay_then_new_key_cannot_create_an_owner` and `review_recovery_at_each_note_boundary_precedes_new_key` test NEW keys and one owner after stored/create/edit boundary simulations. These are graceful reopen/synthetic durable-state tests, not OS kills or proof claims finalize during Notes startup without a service request. | | P1 present unsupported entry mistaken for absent | **Verified** for reported flow/scalar/list/empty records. | `review_present_unsupported_entries_refuse` exercises the service, asserts refusal, unchanged source and no destination creation. Strict lookup now propagates unsupported-parent errors. | | P1 paste-before-original guesses ownership | **Verified by refusal** for the cited same-note case. | `review_paste_before_original_refuses_without_rekeying` checks an above-original copy with owner-link children and unchanged whole text. It does not claim full fresh-graph copy support. | | P1 adoption ignores existing owners/ordinary markers/index errors | **Verified** for reported fixtures. | Unique-owner adoption, duplicate-owner refusal, ordinary collision, observed-ordinary removal and unavailable-index tests exercise the individual cases. `review_unavailable_index_never_means_free` is a helper-level test, not a full failed-index promotion. Cold-import history remains limited as documented. | | P1 markers exposed before UI hiding | **Not fully fixed under the requested “no production path can write markers” gate**; fresh action dispatch is disabled. | `review_marker_writes_are_not_exposed_before_ui_support` only asserts `EventsPlugin::new(...).actions().is_empty()`. It proves the registry gate, not queued replay; see finding 2. `with_writes` is genuinely test-only. | | Audit: stale request settles unrelated markers | **Verified for the cited stale-target path**. | Both entrypoints locate before settlement and select the target; existing stale-target regression covers this. It is not a proof against an outside edit between lookup and write. | | Audit: weak child-link assertion | **Verified improvement, incomplete destination proof**. | Generated plan cases now require exactly one parsed child; that still does not prove every link resolves, including moved authority links below. | **New P1 — Raw journal transfer changes relative-link meaning.** `crates/events/src/transfer.rs:58`, `crates/events/src/promote.rs:172`. A source `Daily/2026/2026-10-26.md` with `events.<U>.people: ['[Alice](<../../People/Alice.md>)']` resolves Alice to `People/Alice.md`. Promotion to default `Calendar/Walk.md` dedents/copies the bytes and deletes the original record, leaving that same path on the owner; from `Calendar/` it climbs outside Notes and no longer resolves. The probe observed a successful plan and `Some("People/Alice.md")` before versus `None` after. Different source/destination folders can also silently retarget a link to a different existing person. Preserving raw YAML does not preserve relative semantic bindings (0068 O04). **Fix:** relocate recognized semantic links and their durable reference bindings together with the record, preserving unrelated bytes; refuse before any note writes when lossless rebasing is unsupported. Add source/destination fixtures at different depths and assert resolved UUID/target equality, including an occupied alternative path. This gap is in the new raw-transfer implementation; no enabled network action is claimed. **New P1 — The release gate does not cover durable event-job replay.** `crates/events/src/actions.rs:40`, `crates/notes/src/lib.rs:285`, `crates/notes/src/bulk/mod.rs:596` and `:608`. Open a Home containing a queued/running `events-identify`, `events-promote` or `events-settle` plan from the earlier write-enabled build. Notes startup unconditionally resumes all running jobs and drives their stored Create/Edit steps, including marker insertion/rekeying, regardless of the plugin's empty action list. UI hiding is still absent. The registry-only regression never reopens with the production plugin against a pending event plan. This is a static reachability finding with the stated persisted-plan precondition, not a claim that a clean Home can dispatch the removed actions. The service's public write methods also remain ungated, although I found no other current production caller of them. **Fix:** put the release decision at the actual event write/replay boundary, with a durable deferred state for pending plans until UI support exists (do not discard their content/history), or complete hiding before allowing their replay. Test a stored plan and a create-completed/edit-pending plan across reopen using `EventsPlugin::new`, and verify source markers cannot be written while disabled. Coordinate this explicitly with crash durability rather than silently dropping jobs. Fresh verification on dev-01, exact head above, in the Nix shell with heavy commands through `~/bin/slot`: - `cargo test -p calternal-events review_ -j4`: **17 passed**, 40 filtered out. - `cargo test -p calternal-notes review_ -j4`: **24 passed**, 420 filtered out. - Focused standalone Rust probe compiled through `~/bin/slot rustc --edition=2024 /tmp/cal-r2-probe.rs ... -o /tmp/cal-r2-probe`, linking current cached stack libraries: observed relative-link breakage, inherited inline-comment deletion and commented relationships. Source/output on dev-01: `/tmp/cal-r2-probe.rs`, `/tmp/cal-r2-probe.out`. The inherited props/exclusion helpers are unchanged from the respective lower stack layers. No fresh full-suite, release-perf, 100k inventory or browser gate is claimed. Existing fix-status limitations remain: real process kills at the new boundaries, concurrent outside edits, intervening-edit/transferred-authority undo combinations, Home-size management cost, and complete child-link destination/reference binding. Disabling fresh actions is useful containment, but it neither completes step 4 nor makes the persisted replay path read-only.
kayg commented 2026-10-31 17:35:46 +00:00
Author
Owner
Copy link

round 2 fixes for final review

Head eb7ffd2d, based on #783 510610d6 and #781 94807faa. Simplification: disabled event writes are rejected at the shared bulk runner before any step, including stored-job startup replay and undo ancestry. Pending plans become durably deferred, retaining steps, progress and receipts. Journal fields that may contain links or escaped values refuse transfer instead of guessing relative-path bindings. No merge.

Finding Status
#70969 P1 transfer breaks or retargets relative links Fixed by refusal. review_round2_cross_folder_authority_links_refuse_without_writes covers source Daily/2026, destinations at different depths, an occupied alternative Alice path, a same-folder local fragment and an escaped YAML link. Source text remains exact and no destination is created.
#70969 P1 stored jobs bypass disabled actions Fixed at bulk::drive, shared by actions, service calls and startup replay. review_round2_disabled_startup_retains_jobs_without_marker_writes reopens with EventsPlugin::new for an entirely queued plan and a genuinely create-completed/edit-pending plan. Both retain their plan/progress/receipts, become deferred, leave source bytes unchanged and cannot bypass the gate through direct runner/service calls.
#70966 hidden owner link affects promotion review_round2_commented_owner_link_is_not_a_relationship uses the real service and creates Tasks/Planning.md; the multiline commented link is ignored. Plain-checkbox identity scanning inherits the shared visibility view.
Inherited #781 comment loss / #783 visibility gaps Includes both lower-layer fixes and their regressions.

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

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

Events: 61 passed; notes: 439 passed / 6 existing ignored; tasks: 30 passed; calternal unit/integration suites passed with existing ignored tests. No build/test ran on the control host; Nix scaffolding was not committed. Pushed in #781 → #783 → #785 order.

Decisions and limits:

  • Production has no write-enable option. Only the explicit events dev-dependency feature exposes an opt-in implementation harness; its default remains disabled. UI marker hiding is still unimplemented, so production actions remain absent and persisted event jobs remain deferred. Deferred rows remain discoverable at startup; plans are not dropped or marked done. Future enabling requires completing the UI contract.
  • No path-rebasing implementation is claimed. Records with potential link/escaped syntax refuse even when a transfer might in fact be safe, including within one folder. Supported link-free authority still moves as raw bytes with exact null/type/comment preservation.
  • Gate work is O(1) for event jobs and O(undo depth) for ancestry checks, with cycle/missing-history refusal. Deferred status is durable; normal non-event jobs keep their behavior.
  • Reopen tests simulate durable boundaries and then exercise real production startup; they are not new OS-kill tests at every boundary. Inherited concurrency, broader undo combinations, cold-import provenance and large-inventory performance limits remain as documented in round 1. No browser/UI or new performance proof is claimed.

Ready for FINAL review round 3. This is a fix-status report, not review approval or merge authorization.

## round 2 fixes for final review Head `eb7ffd2d`, based on #783 `510610d6` and #781 `94807faa`. Simplification: disabled event writes are rejected at the shared bulk runner before any step, including stored-job startup replay and undo ancestry. Pending plans become durably `deferred`, retaining steps, progress and receipts. Journal fields that may contain links or escaped values refuse transfer instead of guessing relative-path bindings. No merge. | Finding | Status | |---|---| | #70969 P1 transfer breaks or retargets relative links | Fixed by refusal. `review_round2_cross_folder_authority_links_refuse_without_writes` covers source `Daily/2026`, destinations at different depths, an occupied alternative Alice path, a same-folder local fragment and an escaped YAML link. Source text remains exact and no destination is created. | | #70969 P1 stored jobs bypass disabled actions | Fixed at `bulk::drive`, shared by actions, service calls and startup replay. `review_round2_disabled_startup_retains_jobs_without_marker_writes` reopens with `EventsPlugin::new` for an entirely queued plan and a genuinely create-completed/edit-pending plan. Both retain their plan/progress/receipts, become deferred, leave source bytes unchanged and cannot bypass the gate through direct runner/service calls. | | #70966 hidden owner link affects promotion | `review_round2_commented_owner_link_is_not_a_relationship` uses the real service and creates `Tasks/Planning.md`; the multiline commented link is ignored. Plain-checkbox identity scanning inherits the shared visibility view. | | Inherited #781 comment loss / #783 visibility gaps | Includes both lower-layer fixes and their regressions. | Passed on dev-01 in Nix, log `~/cal-r2-s4-final3.log`: ```sh cargo fmt --all -- --check ~/bin/slot cargo clippy -p calternal-events -p calternal-notes -p calternal-tasks -p calternal --all-targets -j4 -- -D warnings ~/bin/slot cargo test -p calternal-events -p calternal-notes -p calternal-tasks -p calternal --no-fail-fast -j4 ``` Events: 61 passed; notes: 439 passed / 6 existing ignored; tasks: 30 passed; calternal unit/integration suites passed with existing ignored tests. No build/test ran on the control host; Nix scaffolding was not committed. Pushed in #781 → #783 → #785 order. Decisions and limits: - Production has no write-enable option. Only the explicit events dev-dependency feature exposes an opt-in implementation harness; its default remains disabled. UI marker hiding is still unimplemented, so production actions remain absent and persisted event jobs remain deferred. Deferred rows remain discoverable at startup; plans are not dropped or marked done. Future enabling requires completing the UI contract. - No path-rebasing implementation is claimed. Records with potential link/escaped syntax refuse even when a transfer might in fact be safe, including within one folder. Supported link-free authority still moves as raw bytes with exact null/type/comment preservation. - Gate work is O(1) for event jobs and O(undo depth) for ancestry checks, with cycle/missing-history refusal. Deferred status is durable; normal non-event jobs keep their behavior. - Reopen tests simulate durable boundaries and then exercise real production startup; they are not new OS-kill tests at every boundary. Inherited concurrency, broader undo combinations, cold-import provenance and large-inventory performance limits remain as documented in round 1. No browser/UI or new performance proof is claimed. Ready for FINAL review round 3. This is a fix-status report, not review approval or merge authorization.
kayg commented 2026-10-31 17:43:50 +00:00
Author
Owner
Copy link

Independent review, round 3 (final)

Verdict: stop and simplify (stack blockers). Reviewed eb7ffd2de6b470241197958459a83651879582d7, round-2 review #70969, fix status #71059, simplification notes and ADRs 0067/0068. This layer's two round-2 P1s are addressed for the reported paths; no new layer-specific P0/P1 found. The final stack still has new #781 P0 / #783 P1 findings, so it cannot be called ready or a P2-only small fix. No code pushed or merged.

Round-2 item Final status / evidence
Relative-link journal transfer changes meaning Verified by refusal. promote.rs:149 rejects raw fields containing [, < or backslash before any bulk note writes. The service regression covers differing folder depths, an occupied alternative target, a local fragment and escaped YAML link. Exact source remains and destination is absent. This proves conservative refusal, not path rewriting.
Durable replay bypasses disabled action registry Verified for the reported paths. notes/src/bulk/mod.rs:305 gates event jobs before any step; queued and create-completed/edit-pending reopen cases become durably deferred with exact steps/progress/results retained. Direct runner/service retry is rejected.
Undo / imports / other production entrypoints Static trace: event service writes use only events-identify, events-promote, events-settle jobs; all reach bulk::drive. Undo ancestry is checked, including missing/cyclic-history refusal. Production has no enable setter; the feature-gated test option defaults false and only the explicit test harness opts in. Describe/outside-import indexing reads without generating markers. No additional current marker-generation bypass found. Reopen test is for promotion, not an exhaustive dynamic test of every kind/undo history. Ordinary note imports may preserve user-supplied bytes; that is not automatic event identity allocation.
Commented owner link Real-service promotion regression passes and creates Tasks/Planning.md.
Prior identity/authority fixes Focused review regressions pass for move/recreate, delete/unmark refusal, duplicate owner/copy refusal, ordinary claims, unavailable index, unsupported authority and recovery before a new key. No new regression found in those reported cases.
Inherited preservation / visibility Blocked. #781 still accepts valid plain scalars with interior quotes and silently deletes their real inline comments (new P0). #783 still emits events from multiline inline code (new P1). Both implementations are inherited here.

New inherited P0: crates/notes/src/props.rs:291, :683: replacing m.a: old 'quoted # keep succeeds and deletes # keep; the interior quote is plain YAML text, but the comment guard treats it as quote syntax. Independent exact-output probe reproduces both top-level and nested writes. Fix/simpler design: strict lexical write allowlist, initially rejecting any hash-bearing target and interior quotes in plain scalars; keep unsupported forms read-only and retain exact splices.

New inherited P1: crates/events/src/scan.rs:318, :321: Event::Code enters the literal-comment helper list but not exclusion ranges. Exact input "\n10:00 hidden\n\n" is unchanged by masking and emits a false event. Fix/simpler design: one original-offset exclusion view including inline code; conservatively reject whole candidate lines intersecting excluded spans, shared by all scans.

Concrete simpler delivery: keep this layer as read-only describe/index work with the existing centralized deferral gate. Do not enable identify/promote/settle or resume deferred event jobs until the smaller writer/parser contracts above and UI marker hiding land. Keep stored plans/receipts intact; do not add another identity/copy/rebasing mechanism to work around the base blockers. Linked journal transfers remain refused. This is the final stop-and-simplify decision, not a request for round 4.

Fresh dev-01 verification at this exact head, Nix shell, heavy commands via ~/bin/slot: cargo test -p calternal-events -p calternal-notes -p calternal-tasks review_ -j4 passed events 21 / notes 25 / tasks 7, including both round-2 service regressions. cargo test -p calternal-notes nested_edits_match_serde_yaml -j4 also passed (default 5,000 cases). Independent cached-library Rust probes nevertheless reproduce both inherited blockers. VM evidence: /tmp/cal-r3-review-tests.log, /tmp/cal-r3-property-test.log, /tmp/cal-r3-props-probe.{rs,out}, /tmp/cal-r3-probe.{rs,out}.

No fresh full-suite/clippy/performance/browser claim. UI hiding remains unimplemented; real process kills at all new boundaries, concurrent outside edits, broader undo combinations, cold-import provenance and Home-size performance remain the earlier documented limits. No additional new P2/P3 finding.

# Independent review, round 3 (final) **Verdict: stop and simplify (stack blockers).** Reviewed `eb7ffd2de6b470241197958459a83651879582d7`, round-2 review #70969, fix status #71059, simplification notes and ADRs 0067/0068. This layer's two round-2 P1s are addressed for the reported paths; no new layer-specific P0/P1 found. The final stack still has new #781 P0 / #783 P1 findings, so it cannot be called ready or a P2-only small fix. No code pushed or merged. | Round-2 item | Final status / evidence | |---|---| | Relative-link journal transfer changes meaning | **Verified by refusal.** `promote.rs:149` rejects raw fields containing `[`, `<` or backslash before any bulk note writes. The service regression covers differing folder depths, an occupied alternative target, a local fragment and escaped YAML link. Exact source remains and destination is absent. This proves conservative refusal, not path rewriting. | | Durable replay bypasses disabled action registry | **Verified for the reported paths.** `notes/src/bulk/mod.rs:305` gates event jobs before any step; queued and create-completed/edit-pending reopen cases become durably deferred with exact steps/progress/results retained. Direct runner/service retry is rejected. | | Undo / imports / other production entrypoints | Static trace: event service writes use only `events-identify`, `events-promote`, `events-settle` jobs; all reach `bulk::drive`. Undo ancestry is checked, including missing/cyclic-history refusal. Production has no enable setter; the feature-gated test option defaults false and only the explicit test harness opts in. Describe/outside-import indexing reads without generating markers. No additional current marker-generation bypass found. Reopen test is for promotion, not an exhaustive dynamic test of every kind/undo history. Ordinary note imports may preserve user-supplied bytes; that is not automatic event identity allocation. | | Commented owner link | Real-service promotion regression passes and creates `Tasks/Planning.md`. | | Prior identity/authority fixes | Focused review regressions pass for move/recreate, delete/unmark refusal, duplicate owner/copy refusal, ordinary claims, unavailable index, unsupported authority and recovery before a new key. No new regression found in those reported cases. | | Inherited preservation / visibility | **Blocked.** #781 still accepts valid plain scalars with interior quotes and silently deletes their real inline comments (new P0). #783 still emits events from multiline inline code (new P1). Both implementations are inherited here. | **New inherited P0:** `crates/notes/src/props.rs:291`, `:683`: replacing `m.a: old 'quoted # keep` succeeds and deletes `# keep`; the interior quote is plain YAML text, but the comment guard treats it as quote syntax. Independent exact-output probe reproduces both top-level and nested writes. Fix/simpler design: strict lexical write allowlist, initially rejecting any hash-bearing target and interior quotes in plain scalars; keep unsupported forms read-only and retain exact splices. **New inherited P1:** `crates/events/src/scan.rs:318`, `:321`: `Event::Code` enters the literal-comment helper list but not exclusion ranges. Exact input `"`\n10:00 hidden\n`\n"` is unchanged by masking and emits a false event. Fix/simpler design: one original-offset exclusion view including inline code; conservatively reject whole candidate lines intersecting excluded spans, shared by all scans. **Concrete simpler delivery:** keep this layer as read-only describe/index work with the existing centralized deferral gate. Do not enable identify/promote/settle or resume deferred event jobs until the smaller writer/parser contracts above and UI marker hiding land. Keep stored plans/receipts intact; do not add another identity/copy/rebasing mechanism to work around the base blockers. Linked journal transfers remain refused. This is the final stop-and-simplify decision, not a request for round 4. Fresh dev-01 verification at this exact head, Nix shell, heavy commands via `~/bin/slot`: `cargo test -p calternal-events -p calternal-notes -p calternal-tasks review_ -j4` passed events 21 / notes 25 / tasks 7, including both round-2 service regressions. `cargo test -p calternal-notes nested_edits_match_serde_yaml -j4` also passed (default 5,000 cases). Independent cached-library Rust probes nevertheless reproduce both inherited blockers. VM evidence: `/tmp/cal-r3-review-tests.log`, `/tmp/cal-r3-property-test.log`, `/tmp/cal-r3-props-probe.{rs,out}`, `/tmp/cal-r3-probe.{rs,out}`. No fresh full-suite/clippy/performance/browser claim. UI hiding remains unimplemented; real process kills at all new boundaries, concurrent outside edits, broader undo combinations, cold-import provenance and Home-size performance remain the earlier documented limits. No additional new P2/P3 finding.
kayg commented 2026-10-31 17:50:35 +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:07 +00:00
Author
Owner
Copy link

housekeeping #1009: needs-decision.

Calendar v1 identity/promotion action slice was held after review; #802 gates writes on the shared note executor. Do not replay the old executor/action model.

original head: eb7ffd2de6b470241197958459a83651879582d7. against dev cd7f84432febfc63b788fbb9b9f9950452982d74, git cherry reports 44 unmatched and 0 equivalent patches. the landing audit lists each candidate missing commit and stable patch match in pr-785.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 identity/promotion action slice was held after review; #802 gates writes on the shared note executor. Do not replay the old executor/action model. original head: `eb7ffd2de6b470241197958459a83651879582d7`. against dev `cd7f84432febfc63b788fbb9b9f9950452982d74`, git cherry reports 44 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-785.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:17 +00:00
Author
Owner
Copy link

Superseded by calendar backend v2 (#802/#803). 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/#803). 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:17 +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!785
No description provided.