From 32eb7070c2b85437606a0a50d801fa779853251b Mon Sep 17 00:00:00 2001 From: Shunichiro Nomura Date: Thu, 2 Jul 2026 14:11:24 +0900 Subject: [PATCH] Document code review resolution evidence --- CODE_REVIEW_RESOLUTION.md | 165 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 165 insertions(+) create mode 100644 CODE_REVIEW_RESOLUTION.md diff --git a/CODE_REVIEW_RESOLUTION.md b/CODE_REVIEW_RESOLUTION.md new file mode 100644 index 0000000..3ee28a7 --- /dev/null +++ b/CODE_REVIEW_RESOLUTION.md @@ -0,0 +1,165 @@ +# Code review resolution evidence + +This file records the item-by-item resolution of `.local/CODE_REVIEW_REPORT.md`. +The report was re-read in full on 2026-07-02. Its summary table contains duplicate references to the detailed findings below; the detailed headings are the source of truth. + +## GitButler stack + +Oldest to newest at the time this evidence was written: + +1. `core-parser-engine-validation` — `3315804` — parser, core state-machine, and core event-log validation. +2. `tauri-persistence-security` — `0673dc6` — Tauri CSP, IPC path containment, attachment grants, persistence safety. +3. `drop-point-hardening` — `524a292` — DropPoint URL/body/token/crypto/session hardening. +4. `frontend-correctness-hardening` — `5672dae` — frontend races, routing, dialogs, plugin failures, formatting. +5. `cleanup-remaining-review-items` — `9c6820f` — dead-code removal, log read streaming/locking, API cleanup. +6. `lifecycle-and-performance-cleanup` — `8c81a95` — DropPoint persisted cleanup/client reuse, attachment/preview streaming, Markdown cache. +7. `transaction-log-deduplication` — `35aba7f` — removes the remaining redundant `EventLog::new` construction in the shared transaction helper. +8. `shared-style-deduplication` — `c5853f8` — centralizes duplicated frontend button/status styles. + +## Verification commands + +- `cargo test --workspace` — passes. +- `pnpm exec svelte-check` — passes. +- `but status` — clean after the implementation chunks before this evidence branch. + +## Report inventory + +### Security findings from the report + +- S1: Content Security Policy is disabled. +- S2: IPC commands accept arbitrary filesystem paths. +- S3: DropPoint server-supplied `drop_link` not origin-checked. +- S4: DropPoint pickup downloads unbounded bodies into memory. +- S5: Attachment filenames are not sanitized on the write side. +- S6: Attachment paths in events are replayed with zero validation. +- S7: DropPoint server-supplied `drop_point_id` spliced into URL paths unencoded. +- S8: DropPoint `http://` base URLs accepted. +- S9: Markdown links navigate the webview away from the app. +- S10a: QR SVG is an unsanitized `{@html}` sink. +- S10b: `image/svg+xml` passes the attachment preview filter. +- S10c: Recipient private keys and derived AES keys are not zeroized. +- S10d: DH all-zero check short-circuits. +- S10e: `opener:allow-reveal-item-in-dir` is granted with no scope. + +### Bug findings from the report + +- B1: Nested task lists are mangled, lost, and duplicated. +- B2: Content before the first `##` heading is silently discarded. +- B3: A regular bullet with a nested pure task list is torn apart. +- B4: `split_frontmatter` is fragile substring matching. +- B5: `is_pure_task_list` can underflow `depth`. +- B6: Steps with a pre-checked template checkbox can never be skipped. +- B7: `InputRecorded` performs no input-definition validation. +- B8: `RecordedInput.label` stores the input ID, not the label. +- B9: `after_step_id` referencing a nonexistent step silently appends. +- B10: Checkboxes in dynamically added steps are untoggleable if `id` is `None`. +- B11: Corrupt fields on known event types are misreported as unknown event types. +- B12: `append_event` can create a log with no `LogMeta` and never fsyncs. +- B13: `sync_dir` cannot work on Windows. +- B14: Execution lookup by 8-char UUID prefix, first match wins. +- B15: One unreadable `.executions` dir aborts the whole execution search. +- B16: Crash-leftover temp dirs appear as ghost executions. +- B17: Durable append happens before `state.apply` validation. +- B18: Committed action reported as failure if `unlock()` errors. +- B19: Attachment bytes written before event validation. +- B20: Unlocked readers can silently miss the newest event. +- B21: `summarize` reports the nil UUID for degenerate logs. +- B22: DropPoint session lifecycle leaks. +- B23: DropPoint error paths leak the created remote drop point. +- B24: Concurrent imports of the same session double-record attachments. +- B25: `u64` to `u32` DTO truncation hard-fails for values >= 4 GiB. +- B26: Last-write-wins race in `executionStore.act()`. +- B27: Unkeyed `{#each summary.steps}` bleeds component state across steps. +- B28: Failed `start()` can navigate to a stale previous execution. +- B29: Execution page goes blank if `isDropPointConfigured()` rejects. +- B30: Optimistic checkbox DOM diverges from store on failure. +- B31: Dialogs close and discard input even when the action failed. +- B32: Enter + blur race double-fires rename. +- B33: Route param changes do not reload the execution. +- B34: Image previews are cleared and refetched over IPC on every action. +- B35: Home page shows time-of-day only. + +### Code-smell findings from the report + +- C1: Copy-pasted `find_execution_dir`; duplicated record transaction code; redundant `EventLog::new` in transaction closures. +- C2: Redundant hashing and whole-file buffering for attachments/previews. +- C3: Duplicate event vocabularies are still actively written. +- C4: `LogMeta` special-cased twice. +- C5: Inconsistent note predicates. +- C6: Per-instance Markdown machinery and repeated re-rendering. +- C7: Dead code (`loadTemplate`, `ExecutionStore.isActive`, redundant null-coalescing). +- C8: Fire-and-forget plugin calls without error handling. +- C9: `NoteEditor` reverts by index. +- C10: Byte-for-byte duplicated status/button CSS. +- C11: Panicking `expect`s on IPC-handler paths. +- C12: Misleading `step_order` doc comment. +- C13: `read_log` buffers the whole file twice. +- C14: Each DropPoint command constructs a fresh `DropPointClient`. + +## Item-by-item resolution evidence + +| Item | Resolution evidence | +| ---- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| S1 | `src-tauri/tauri.conf.json` now sets restrictive `csp` and `devCsp` values with `object-src 'none'`, `base-uri 'none'`, and `frame-ancestors 'none'` instead of `null`. | +| S2 | `load_template` IPC and `loadTemplate()` frontend API were removed from `src-tauri/src/commands/template.rs`, `src-tauri/src/lib.rs`, and `src/lib/api/commands.ts`. `start_execution` calls `resolve_template_path()` in `src-tauri/src/path_security.rs`, which canonicalizes paths, requires containment under `procedures_dir`, and requires direct `template.md`. Attachments now go through `pick_attachment_sources()` and one-shot canonical grants in `AppState::{grant_attachment_path,consume_attachment_path}` before `record_action` accepts them. | +| S3 | `src-tauri/src/commands/drop_point.rs::drop_link_with_fragment` parses the server `drop_link` and rejects it unless `same_origin(config.base_url, drop_link)` holds, including scheme, host, port, and no user info. | +| S4 | `src-tauri/src/drop_point/client.rs::read_body_limited` checks `Content-Length` and streams chunks against a hard limit; `pickup()` uses the configured/default pickup limit and `ensure_success()` caps error bodies with `MAX_ERROR_BODY_BYTES`. | +| S5 | `src-tauri/src/persistence/attachment_store.rs::sanitize_attachment_filename` rejects path separators, Windows-reserved characters/names, trailing dots/spaces, and Unicode format/bidi controls. DropPoint manifest import reuses this sanitizer in `src-tauri/src/drop_point/manifest.rs`. Tests: `sanitizes_to_single_safe_component`, `sanitizes_path_filenames`, `renames_duplicate_filenames_case_insensitively`. | +| S6 | `crates/procnote-core/src/execution/engine.rs::validate_attachment_path` rejects empty, absolute, backslash, `.`/`..`, and drive-like path segments. It is called for `AttachmentAdded`, `AttachmentsAdded`, and `AttachmentFileRemoved` replay. Test: `test_attachment_event_paths_must_be_relative_and_normal`. | +| S7 | `src-tauri/src/drop_point/client.rs::validate_drop_point_id` restricts IDs to ASCII alphanumeric plus `-_.`; URL construction uses `Url::path_segments_mut()` in `endpoint()`/`drop_point_endpoint()` instead of string splicing. | +| S8 | `src-tauri/src/drop_point/client.rs::parse_base_url` rejects non-HTTPS DropPoint URLs except loopback HTTP for local development; also rejects query, fragment, and user-info. | +| S9 | `src/lib/components/StepCard.svelte` adds `externalLinks` click handling and routes Markdown links through `@tauri-apps/plugin-opener::openUrl()` with error logging instead of allowing webview navigation. Capabilities include only URL opening defaults, not broad reveal. | +| S10a | `src/lib/components/AttachmentField.svelte` derives `sanitizedQrSvg` by running `remoteSession.qr_svg` through DOMPurify's SVG profile before `{@html}`. | +| S10b | `src-tauri/src/commands/execution.rs::preview_content_type` now returns `None` for `image/svg+xml`; attachment content types are inferred server-side in `authorize_attachment_source`/`attachment_content_type` instead of trusting the frontend. | +| S10c | `src-tauri/src/drop_point/crypto.rs` uses `zeroize::Zeroizing` for generated recipient private keys, HKDF salt, and derived AES keys; `src-tauri/src/drop_point/session.rs` stores recipient keys as `Arc>`. | +| S10d | `src-tauri/src/drop_point/crypto.rs` uses `subtle::ConstantTimeEq` for the X25519 all-zero shared-secret check. | +| S10e | `src-tauri/capabilities/default.json` removed unscoped `opener:allow-reveal-item-in-dir`; `reveal_execution_dir` resolves an execution ID server-side and calls the opener plugin from Rust. | +| B1 | `crates/procnote-core/src/template/parser.rs` now identifies complete list ranges with `matching_list_end`, requires every item at every nesting level to have a direct task marker for pure task lists, and collects task text while excluding nested-list text. Tests include `test_nested_pure_checkboxes`, `test_pure_task_list_then_regular_list`, and `test_same_marker_adjacent_lists_merged_as_prose`. | +| B2 | `parse_body()` now errors with `ParseError::ContentBeforeFirstStep` for non-empty content before the first H2 and `ParseError::MissingStepHeading` when no H2 exists. Tests: `test_content_before_first_step_heading_errors`, `test_missing_step_heading_errors`. | +| B3 | `is_pure_task_list()`/`direct_item_has_marker()` now make a mixed regular-list parent plus nested task list remain prose. Test: `test_regular_bullet_with_nested_task_list_stays_prose`. | +| B4 | `split_frontmatter()` is line-based and requires delimiter lines to be exactly `---`; block-scalar lines beginning with `---` are preserved. Test: `test_frontmatter_delimiters_must_be_exact_lines`. | +| B5 | Parser list/item depth handling uses `saturating_sub` and direct item traversal, removing the underflow-prone backward scan. Covered by parser list tests. | +| B6 | `StepState` records `initial_checkbox_states`; `has_captured_data()` compares current checkbox state to template initial state, so pre-checked template boxes do not block skipping. Test: `test_prechecked_template_checkbox_does_not_block_skip`. | +| B7 | `InputRecorded` now calls `StepState::scalar_input_label`, which requires the `input_id` to exist and rejects attachment inputs. Test: `test_record_input_requires_defined_scalar_input_and_uses_label`. | +| B8 | The same `scalar_input_label()` returns `definition.label`, and `RecordedInput.label` is populated from it. Test: `test_record_input_requires_defined_scalar_input_and_uses_label`. | +| B9 | `StepAdded` now looks up `after_step_id` and returns `ExecutionError::AfterStepNotFound` instead of appending silently. Test: `test_add_step_requires_existing_after_step_and_assigns_checkbox_ids`. | +| B10 | `content_with_checkbox_ids()` assigns stable IDs to checkboxes in dynamically added steps before storing them. Test: `test_add_step_requires_existing_after_step_and_assigns_checkbox_ids`. | +| B11 | `crates/procnote-core/src/event/log.rs::read_log` now distinguishes `UnknownEventType`, `InvalidEventPayload`, and `CorruptLine`. Tests: `test_invalid_known_event_payload_errors`, `test_unknown_event_type_errors`, `test_mid_file_corruption_errors`, `test_tail_truncation_tolerated`. | +| B12 | `append_event()` now creates parents, requires `LogMeta` as the first event in a new log, flushes, and `sync_all()`s. Test: `test_append_requires_log_meta_first`; durable shell tests cover append round trips. | +| B13 | `src-tauri/src/persistence/event_log.rs::sync_dir` and DropPoint session `sync_dir` use Windows `OpenOptionsExt` with `FILE_FLAG_BACKUP_SEMANTICS`; non-Windows behavior remains `File::open(path)?.sync_all()`. | +| B14 | Shared `find_execution_dir()` in `src-tauri/src/persistence/execution_store.rs` reads each candidate log under a shared lock and matches the full `ExecutionStarted.execution_id`, returning an error on multiple matches instead of first 8-character prefix wins. | +| B15 | The same `find_execution_dir()` and `list_executions()` log and skip unreadable `.executions` directories instead of aborting all searches. | +| B16 | `is_temp_execution_dir()` filters names starting with `.` or containing `.tmp-`; `list_executions()` and `find_execution_dir()` skip these staging directories. | +| B17 | `with_log_transaction()` reads/replays, builds a candidate event, applies it to a cloned/current state for validation, then commits attachments and durably appends. Core event-builder methods use `validated_candidate()`. | +| B18 | `EventLog::{with_exclusive_lock,with_shared_lock}` call `unlock_or_warn()` and do not propagate unlock errors after a committed operation. | +| B19 | Attachment sources are prepared and the candidate `AttachmentsAdded` event is validated before `commit_pending_attachments()` writes files. DropPoint import uses the same `record_attachment_bytes_batch()` transaction. | +| B20 | Readers use `EventLog::read_locked()` in `load_execution_from_disk()`, `list_executions()`, and `find_execution_dir()`; shared locks prevent observing a writer mid-append. | +| B21 | `summarize()` now returns `Err` if required execution invariants are missing (`execution_id`, procedure id/title/version) instead of using nil UUID or panicking. | +| B22 | `DropPointSessions` now replaces same-target sessions, checks expiry in `get()`/`take()`, persists close tokens under `.drop_point_sessions`, deletes persisted session files on close/cancel, and startup calls `cleanup_persisted_sessions()` with the shared client. | +| B23 | `start_attachment_drop_point_session()` builds all fallible session data before exposing it; on setup/insert failure it calls `client.close()` for the created remote drop point. | +| B24 | Import uses `sessions.take()` before pickup/import and reinserts only on failure; frontend polling has a `pollInFlight` guard and run-id invalidation. | +| B25 | `AttachmentDropPointSessionSummary.max_bytes` and `AttachmentDropPointStatus.encrypted_size` are `u64`; generated TypeScript bindings were regenerated. | +| B26 | `ExecutionStore.act()` serializes actions through `#actionQueue`, so responses are applied in send order rather than arrival order. | +| B27 | `routes/execution/[id]/+page.svelte` keys steps by `stepSummary.id`; `StepCard.svelte` keys input blocks by `input.definition.id`; `NoteEditor.svelte` keys notes by `note.id`. | +| B28 | `ExecutionStore.start()` clears stale `summary`, returns `boolean`, and the home page navigates only when start succeeded and a new summary exists; the home page renders `executionStore.error`. | +| B29 | The execution route uses `Promise.allSettled()` for DropPoint config and execution load, treating DropPoint config failure as disabled while still loading the execution. | +| B30 | `CheckboxItem.svelte` refuses missing checkbox IDs and rolls the DOM checkbox back to `checkbox.checked` when `ontoggle` returns false. | +| B31 | Abort/add-step/complete/skip dialogs close and clear their text only if `executionStore.act()`/`onaction` returns success. | +| B32 | Rename save uses a `savingName` guard to suppress Enter/blur double-fire while the first save is pending. | +| B33 | The execution page load is an `$effect` keyed on the derived route param, so `/execution/A -> /execution/B` triggers a new load. | +| B34 | `AttachmentField.svelte` preserves existing `previewUrls`, prunes only removed paths, and only fetches previews for missing image paths; backend preview encoding streams the file through `base64::write::EncoderWriter`. | +| B35 | `formatTimestamp()` now includes year, month, day, hour, minute, and second. | +| C1 | `find_execution_dir()` is centralized in `src-tauri/src/persistence/execution_store.rs` and reused by commands; `with_log_transaction()` centralizes read/replay/validate/append/apply behavior; `transaction-log-deduplication` removed remaining redundant `EventLog::new` in that transaction. | +| C2 | `AttachmentStore::prepare_copy()` computes SHA-256 by streaming, `commit_prepared()` streams source bytes while verifying the hash, and preview data URL generation streams base64 encoding instead of reading raw bytes into memory first. | +| C3 | Write paths now mint `AttachmentsAdded`, `AttachmentFileRemoved`, and `AttachmentsCleared`; compatibility variants remain readable but `add_attachment_event()` and `remove_attachment_event()` delegate to the canonical multi/clear events. | +| C4 | `ExecutionState::from_events()` skips `LogMeta` as replay metadata; `ExecutionState::apply()` returns `ExecutionError::ReplayMetadataEvent` if asked to apply `LogMeta` as a transition. | +| C5 | `note_exists()` and `remove_note()` both scan all steps consistently, including skipped steps. | +| C6 | `StepCard.svelte` moved Markdown/highlight setup to `