Feat/import data integrity tests#9
Merged
Merged
Conversation
Why: User reported observing duplication after browser-history
imports and asked for a robust test system before more browser
adapters land. The audit confirms cross-browser "visual duplication"
is the deliberate per-source-profile schema contract, not a bug —
but uncovers six real bugs the test harness must encode as failing
tests before fixes ship:
B1 urls upsert silently regresses visit_count / title / typed_count
when an older snapshot re-imports (writes.rs:123-138 has an
unconditional overwrite where only last_visit_ms is guarded).
B2 Firefox + Safari incremental re-import drop long-tail revisits
because their URL stream queries lack the OR fallback Chromium
added (chromium/mod.rs:74-90 has it; firefox/mod.rs:22-33 and
safari/mod.rs:42-56 do not).
B3 Takeout source_visit_id is path-bound: hash("{path}:{ord}:{url}")
means renaming or re-downloading the JSON produces a full
duplicate set (takeout/browser_history.rs:339).
B4 Takeout × local Chrome same-period overlap always double-counts
because Takeout hardcodes app_id="takeout" and transition=None,
so the fingerprint fallback can never match a real Chrome visit
of the same instant (takeout/browser_history.rs:381-386).
B5 takeout stable_key_i64 is a degenerate polynomial hash with
wrapping_mul(31)+abs() over hex bytes; collisions become likely
well below the AGENTS.md 14.4M-record ceiling
(takeout/browser_history.rs:442).
B6 Takeout time_usec unit ambiguity: the function name says Unix
microseconds but Google's Takeout dump historically uses Chrome
epoch — needs a fixture-pinned contract assertion to resolve.
What:
- docs/plan/program/import-dedup-audit.md (337 lines): per-table
dedup keys, the six bugs with file:line evidence, per-family
behavior summary, gaps the schema cannot cover.
- docs/plan/program/import-test-harness-spec.md (449 lines): crate
layout, Scenario DSL sketch, fixture-generator API, assertions API,
scenario library prioritized into 6 tiers, real-data redaction
policy, acceptance criteria.
- BACKLOG.md: new 2026-05-25 planning note + WORK-IMPORT-TEST-HARNESS-A
block at the top of the queue (unblocked, awaiting STATUS promotion).
The view-layer cross-browser aggregation (the user-visible fix for
the duplication UX) is decided in the planning conversation but
belongs to a separate work block; this audit deliberately stays in
storage-layer truth.
…um writer Why: WORK-IMPORT-TEST-HARNESS-A needs a foundation that emits real-format browser-history files from declarative records so dedup scenarios exercise the production parser and ingest pipeline rather than mocked record streams. Without a self-validating generator a failing scenario could just be the generator silently disagreeing with the parser — false positives on a correctness harness are worse than no tests at all. This commit lands the smallest verifiable slice: the Chromium History writer with a parser round-trip self test. What: - New crate src-tauri/crates/browser-history-fixtures/ added to the Cargo workspace. Only workspace dependencies (chrono, rusqlite, tempfile, plus browser-history-parser for the round-trip test) — no new third-party deps requiring supply-chain review. - src/time.rs: Unix-ms ↔ Chrome-microseconds-since-1601 helpers, with saturating arithmetic that mirrors vault-core's production helper. Three unit tests pin the round-trip, the zero-point offset, and saturation behavior. - src/chromium/mod.rs: ChromiumHistoryFixture builder + a SQLite writer that materializes the urls/visits column shape the production chromium parser reads in INGEST_URLS_FULL_SQL / INGEST_VISITS_SQL. Schema deliberately omits favicon_id and the sync/segment columns that the parser doesn't project, to avoid fixture drift; downloads/favicons/keyword-search-terms wait for their own writers when scenarios call for them. - tests/chromium_roundtrip.rs: constructs a 2-URL / 3-visit fixture with revisit + referrer + sync state, writes it, parses it back through browser_history_parser::chromium::parse_history, and asserts every emitted field exact-match. Time helper pinning asserts Chrome ms for 2026-05-02T12:00:00Z is exactly 13_422_283_200_000_000. Drive-by observation captured inline (no code change): the production ParsedVisit.visit_duration_ms field name claims milliseconds but the Chromium parser passes Chrome's native microsecond value through unchanged, and the canonical archive visit_duration_ms column stores microseconds too. The round-trip test pins the current behavior with a doc comment linking back to import-dedup-audit.md so future readers understand why the fixture writes microseconds. Next slices (same work block): Firefox places.sqlite writer, Safari History.db writer, Takeout JSON/JSONL writer (each with their own parser round-trip self-test), then the Scenario DSL, the vault-core test-helper for driving ingest end-to-end, and the Priority 1 scenarios C1/C2/C3/T1/T2/X1. Verification: - cargo test -p browser-history-fixtures → 5 passed, 0 failed - cargo check --workspace → clean across all six crates
Why: The Chromium-only scaffold from the previous commit can only exercise
one family of dedup scenarios. Completing the four supported source
formats (Chromium, Firefox, Safari, Takeout) unlocks the cross-source
scenarios from the spec — including X1 (Edge imports Chrome history then
diverges), X3 (mixed family time conversions), T1–T4 (Takeout failure
modes), and the per-family long-tail revisit failing tests for B2.
What:
- src/firefox/mod.rs: `FirefoxPlacesFixture` writes the
`moz_places` / `moz_historyvisits` shape the production Firefox
parser reads, with Unix-ms ↔ Firefox-μs conversion handled inside
the writer.
- src/safari/mod.rs: `SafariHistoryFixture` writes the
`history_items` / `history_visits` shape, selectable between the
Minimal historical schema and the Current macOS schema (with
`load_successful`, `synthesized`, `redirect_*`, `origin`,
`generation`, `attributes`, `score`). CFAbsoluteTime conversion
rounded to ms to match the parser's `(_ + offset) * 1000`.round
semantics.
- src/takeout/mod.rs: `TakeoutBrowserHistoryFixture` writes the three
on-disk layouts the Takeout source classifier accepts —
`{ "Browser History": [...] }` (standard), `{ "BrowserHistory":
[...] }` (no-space alternate), and JSONL one-record-per-line. The
writer emits Google's real field names (`page_transition`, `title`,
`url`, `time_usec`, `client_id`, `favicon_url`) so the parser's
record-extraction path is exercised end-to-end.
- src/lib.rs: re-exports for the three new writer surfaces.
- tests/{firefox,safari,takeout}_roundtrip.rs: each writes a small
fixture, parses it back through the real PathKeep parser, and
asserts every emitted field matches. Safari covers both schema
variants; Takeout covers all three formats. Time-helper pinning
asserts each family's epoch offset.
Verification:
- cargo test -p browser-history-fixtures → 15 passed, 0 failed
- cargo check --workspace → clean
Next slice (same work block): vault-core test-helper that lets
integration tests drive `process_profile_snapshot` end-to-end, then
Priority 1 scenarios C1/C2/C3/T1/T2/X1 wired up using these fixtures.
Why: The fixture writers from the previous slices proved the parser round-trips synthetic SQLite faithfully, but a writer that only round- trips through the parser cannot defend the rest of the ingest pipeline — watermark advancement, source-profile upsert, INSERT OR IGNORE dedup, the long-tail revisit OR fallback. This commit lands the first three Priority 1 contract scenarios that drive the full `process_profile_snapshot` path against the new fixtures, so future ingest refactors break a named test rather than silently changing truth on disk. What: - vault-core dev-dep on browser-history-fixtures (workspace path, no third-party deps added). - archive/ingest/dedup_scenarios.rs (new sibling test module) holds the scenarios. The module lives in-tree rather than under tests/ because process_profile_snapshot is pub(super) to the archive module; an in-module placement keeps the scenarios end-to-end without widening the public surface for testability alone. - ScenarioEnv helper wraps the TempDir + ProjectPaths + AppConfig setup that every scenario shares; run_one_ingest drives one full process_profile_snapshot pass and commits, so subsequent passes observe a stable archive. Scenarios: - C1 chromium_baseline_import: one profile, one pass, asserts summary.new_urls=2 / summary.new_visits=3 and that source_visit_id values flow through unmodified. - C2 chromium_incremental_no_new_data: re-runs the same fixture with use_watermark=true and asserts new_urls=0, new_visits=0 and archive counts unchanged. - C3 chromium_incremental_revisit_of_old_url: adversarial pass-2 fixture where the visit cursor moves past 10 but the URL's last_visit_time is deliberately left at the old value. Without the OR fallback in INGEST_URLS_SQL (chromium/mod.rs:85-90) the new visit would be silently dropped by the url_id_map lookup in ArchiveChunkConsumer::visits. The test pins that the fix stays intact across future refactors. - docs/plan/program/import-dedup-audit.md grows Section 6 "Scenarios Now Backed By Tests" — a living index of contract scenarios that pass today and bug scenarios that still need to be written. Each entry links to its scenario function. Verification: - cargo test -p vault-core --lib dedup_scenarios → 3 passed - cargo test -p vault-core --lib → 572 passed, 0 failed (no regression in the 569 pre-existing tests) Next slice (same work block): the Firefox + Safari + Takeout end-to-end scenarios (F1, S1, T1) plus the first failing tests for B2 (F2 / S2 long-tail revisit), B3 (T2 path-bound source_visit_id), B4 (T3 Takeout × local Chrome double-count). Those will need the takeout ingest path entrypoint and the multi-profile orchestration helper.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Why: Completes Priority 1 scenarios C1-C3-X1-T1-T2 from the harness spec and uncovers a real audit error along the way. The first cut of B3 (path-bound Takeout source_visit_id) overstated the bug as "renaming the file produces a full duplicate set"; the T2 scenario proved that in the all-fingerprint-inputs-identical case the `(source_profile_id, event_fingerprint)` partial unique index catches the duplicates even though every source_visit_id changes. This is exactly the kind of analytical overreach the harness is supposed to surface — landing the test, watching it pass against the "wrong" expected value, and forcing the audit to be corrected. Updated B3's actual blast radius: rename-only re-import is safe; rename + any fingerprint-input drift (e.g. title changed between two Takeout exports) reproduces the full duplicate set. T2b pins that narrower case with `#[should_panic]` until the fix lands. What: Four new scenarios in archive/ingest/dedup_scenarios.rs: - T1 t1_takeout_baseline_import: end-to-end through `crate::takeout::import_takeout`, ingests a synthetic BrowserHistory.json into profile_key="takeout::browser-history" with app_id="takeout" on every visit. - T2 t2_takeout_rename_file_reimport_dedups_via_fingerprint_partial_index: refutes the original B3 framing — fingerprint partial index catches the rename-only duplicate set. - T2b t2b_takeout_rename_with_title_change_demonstrates_b3_when_fingerprint_diverges: `#[should_panic]` failing test for the actual B3 surface — same records but Google captured a new title in the intervening export window. Both unique indexes miss, all 3 records duplicate. - X1 x1_edge_imports_chrome_then_both_diverge: per-source-profile contract — Chrome and Edge each get independent rows for the shared URL, total archive holds 5 urls / 6 visits (3 chrome + 2 edge URLs, 3 chrome + 3 edge visits), and Edge's browser_product stays "Microsoft Edge" rather than collapsing to "Google Chrome" (browser-support-and-adapter-playbook.md §107). Audit doc updates: - B3 description rewritten to reflect the narrow real-world case the harness actually demonstrates, with cross-links to T2 (the contract scenario) and T2b (the failing-test scenario). The design concern stays — path-bound source_visit_id provides zero useful dedup signal — but the practical impact is now correctly scoped to fingerprint-drift cases. - Section 6 grew the contract-scenario table to 6 rows (C1, C2, C3, T1, T2, X1) and the bugs-with-failing-tests table now points B3 at T2b. Verification: - cargo test -p vault-core --lib dedup_scenarios → 7 passed (4 new + 3 from previous commit; T2b correctly should_panics) - cargo test -p vault-core --lib → 576 passed, 0 failed (was 572)
Why: Finishes the bug-coverage spread the spec called for and surfaces two more audit corrections the harness alone could find. C4 pins B1 (URL upsert visit_count regression) as a `#[should_panic]`. F2 pins B2 for Firefox after building the right multi-URL setup that pushes the URL watermark past the long-tail target. Most importantly, S2 demonstrated that Safari does NOT have B2 — its URL query computes `MAX(history_visits.visit_time)` on the fly from the visits table and has no cached `last_visit_time` column on `history_items` to lag behind, so the OR fallback is unnecessary by construction. S2 stays as a contract test that catches a regression if anyone introduces such a cache. T3 reframes B4 from a "bug" to a design contract: per-source-profile storage truly keeps Chrome and Takeout independent, and the fingerprint inputs (app_id, transition) diverge enough that any future cross-source dedup proposal must normalize them first. T5 pins B6's current Unix-microsecond interpretation end-to-end and catches sign-flip regressions on the parser side. What: Five new scenarios + two audit corrections: - C4 c4_chromium_reimport_older_snapshot_regresses_visit_count_demonstrates_b1 `#[should_panic]`: imports a URL with visit_count=10, then a "same URL but stale snapshot" with visit_count=5. The unconditional overwrite in writes.rs:123-138 rolls the archive count back to 5 even though last_visit_ms is unchanged. Flip to plain `#[test]` after each affected field gets gated on `excluded.last_visit_ms >= urls.last_visit_ms`. - F2 f2_firefox_incremental_revisit_of_old_url_drops_visit_demonstrates_b2 `#[should_panic]`: long-tail Firefox URL + anchor URL setup so the URL watermark advances past the target. Pass 2 adds a new visit on the long-tail URL; the Firefox URL query at firefox/mod.rs:22-33 filters the URL out and ArchiveChunkConsumer::visits silently drops the visit (skipped_visits += 1). Flip to plain `#[test]` after the Firefox URL stream grows the same `OR id IN (SELECT DISTINCT url FROM visits WHERE id > ?2)` fallback the Chromium parser added at chromium/mod.rs:85-90. - S2 s2_safari_long_tail_revisit_captured_without_or_fallback Contract scenario, not a failing test. The same long-tail setup works correctly on Safari because safari/mod.rs:42-56 computes `MAX(history_visits.visit_time)` per item on the fly. The audit reframing in this commit's doc updates corrects the B2 entry to explicitly exclude Safari. - T3 t3_takeout_and_local_chrome_same_period_b4_contract Contract scenario for B4. Imports the same Chrome data via both the chromium adapter and the Takeout flow; asserts the per-source split (3 chrome visits + 3 takeout visits = 6) plus the app_id / transition divergence that prevents any naive cross-source fingerprint dedup. - T5 t5_takeout_time_usec_pinned_as_unix_microseconds_b6_contract Contract scenario for B6. Writes a Unix-microsecond `time_usec` field, imports through the Takeout flow, asserts the resulting visit_time_ms and ISO match the input. Catches any future flip to Chrome epoch immediately. The "what does Google really ship" open question stays documented in the audit until a real-world sample arrives. Audit updates in docs/plan/program/import-dedup-audit.md: - B2 entry split: Firefox is exposed; Safari is not. Cross-linked to F2 (failing test) and S2 (contract test). - Section 6 contract table grew to 7 entries; bugs-with-failing-tests table grew to 7 entries with B1/B2-Firefox/B3-narrow as `#[should_panic]` and B4/B6 as contract tests. B5 explicitly deferred to a dedicated scale-test slice. Drive-by note caught by T5's first failure: the 1_777_680_000_000 Unix ms constant used across these scenarios is actually 2026-05-02T00:00:00Z, not 2026-05-01 as some inline comments claimed. Test assertions adjusted; misleading comments stay flagged for a separate cleanup if they cause confusion later. Verification: - cargo test -p vault-core --lib dedup_scenarios → 12 passed (5 contract tests + 4 `#[should_panic]` failing tests + 3 from prior commit's still-relevant scenarios) - cargo test -p vault-core --lib → 581 passed, 0 failed (was 576)
Why: `bun run check` was failing at `check:base` due to accumulated drift across JS/TS and Rust files — prettier format, eslint `require-await` and `no-unnecessary-type-assertion` violations, TypeScript `.disabled` access on `HTMLElement`, Rust 1.94.0 `derivable_impls` and `unnecessary_cast` clippy lints, and `rustfmt` across the workspace. What: - JS/TS: prettier reformat (12 files), remove async from sync act/test callbacks in use-route-history-nav.test, eslint-disable for legitimate setState-in-effect, switch .disabled property access to toBeDisabled() matcher in link-previews + paper-form-primitives tests. - Rust: rustfmt --all across workspace, derive Default for OgImageFetchMode enum (clippy::derivable_impls), remove unnecessary i64 cast in og_images test helper (clippy::unnecessary_cast). Note: check:coverage (JS branch 97.96% < 98% threshold) was already failing at the committed state before these changes — tracked by WORK-V03-COVERAGE-RESIDUAL in BACKLOG.md.
Why: The import test harness work block is complete — 12 e2e scenarios covering the full ingest pipeline across all 4 browser families. The user flagged the sub-millisecond Chrome visit collision concern for follow-up. What: - BACKLOG: flip WORK-IMPORT-TEST-HARNESS-A from [ ] to [x] with closeout note; add WORK-IMPORT-SCALE-TEST-A for B5 follow-up. - CHANGELOG: append full closeout entry — audit, fixture crate, 12 scenarios (9 contract + 3 should_panic bug repros), TODO markers. - Audit doc: add TODO for sub-millisecond Chrome visit collision (C_SUB_MS) in §4 Time precision; fix markdown table formatting (|| in SQL was parsed as column separator). - dedup_scenarios.rs: add TODO comment stub for C_SUB_MS scenario. - import-test-harness-spec.md: prettier formatting.
Why: JS branch coverage sat at 97.96% and Rust had 12 uncovered source lines across og_images_fetch, archive_flows, and worker_bridge. Both gates now pass at 100% Rust (33,390 lines, 1,594 functions) and 99%+ JS (98.02% branches), ready for the mutation testing sweep. What: - JS: explorer-preferences (view mode + clock format localStorage round- trips, error/skip branches), paper-preferences (event dispatch, persist- and-return), appearance-section (PAPER_PREFERENCES_EVENT sync, missing- detail falsy branch), paper-settings-header (tabindex-already-set skip). - Rust fixtures: write-overwrite tests for Chromium, Firefox, Safari, and Takeout writers; Takeout default + JSON escape edge cases. - Rust og_images_synth: Bilibili BV prefix-mismatch and AV non-digit parse failure branches. - Rust og_images_fetch: YouTube synth→finish_image_fetch pipeline test; Bilibili API→finish_image_fetch via mockito (added bilibili_api_base parameter to fetch_og_image_for_pipeline for test injection). - Rust archive_flows: fixed FK constraint violation by seeding a runs row before the urls insert in the prefetch integration test. - Rust worker_bridge: exercised prefetch_og_images_impl via the existing og-image integration test with zero-budget short-circuit.
Why: The import-dedup audit identified three data-integrity bugs that cause silent data loss or corruption on re-import: - B1: URL upsert unconditionally overwrites visit_count/typed_count with incoming values, so re-importing an older snapshot rolls counts backward. - B2: Firefox URL query lacks the long-tail revisit OR fallback that Chromium has, so revisiting a URL whose last_visit_date is below the watermark silently drops the new visit. - B3: Takeout source_visit_id is derived from the on-disk file path, so renaming the export file + any fingerprint-input drift produces a full duplicate set. What: - writes.rs: gate title/hidden on excluded.last_visit_ms >= existing; use MAX(existing, incoming) for visit_count and typed_count. - firefox/mod.rs: add OR fallback subquery to URLS_SQL mirroring Chromium's INGEST_URLS_SQL pattern; pass after_visit_id param. - takeout/browser_history.rs: derive source_visit_id from (url, visit_time_micros) instead of (path, ordinal, url). - dedup_scenarios.rs: flip three #[should_panic] tests to plain #[test] now that the fixes are in place.
…dings Why: The comprehensive test audit identified 22 gaps (3 CRITICAL, 5 HIGH, 8 MEDIUM, 6 LOW) in the import dedup test harness. Several round-trip tests asserted only row counts without verifying field values, Firefox and Safari lacked baseline happy-path scenarios, and the fingerprint partial-index dedup path was untested for Chromium. These gaps would allow mutations in field-reading and dedup logic to survive undetected. What: - Round-trip hardening: Safari extra-column assertions (typed_evidence for load_successful/synthesized/redirect/score), Firefox full-field assertions (typed_count, visit_duration_ms, is_known_to_sync, etc.), Takeout client_id/favicon_url/page_transition context evidence assertions, alternate-key and JSONL format field-level assertions - New baseline scenarios: F1 (Firefox) and S1 (Safari) happy-path imports with URL/visit count, timestamp, and title verification - Chromium fingerprint dedup: re-import with different source_visit_ids, assert event_fingerprint partial index catches duplicates - Edge cases: CJK URL/title round-trip, Safari pre-1970 timestamp clamping, Firefox NULL visit_count/last_visit_date defaults - C4 expansion: third import pass with strictly older last_visit_ms to verify title/hidden don't regress (tests CASE WHEN guard) - Fingerprint contract test + url_bounds no-change test in writes.rs - Audit doc updated: B1/B2/B3 marked FIXED, new scenarios added to table Context: Implements findings from the SQLite-level test audit dispatched in the WORK-IMPORT-TEST-HARNESS-A work block. Prepares the harness for mutation testing by ensuring every production code branch is exercised and asserted against.
Why: The import test harness follow-up work (B1/B2/B3 fixes + 22-finding audit hardening) needs to be recorded in the project tracking docs. dedup_scenarios.rs at 1278 lines exceeds the 1200-line threshold per AGENTS.md and needs a BACKLOG maintainability entry. What: - CHANGELOG: append audit hardening closeout with fix details, coverage numbers, and deferred items list. - BACKLOG: add WORK-IMPORT-TEST-REMAINING-A for dedup_scenarios.rs maintainability review + remaining MEDIUM audit items (Takeout ptoken, visitedAt ISO, E6 URL canonicalization, C_SUB_MS).
Why: The import test harness assessment rated spec coverage at 40% (12/30
scenarios). The biggest blindspots were R-series (error/corruption),
E-series (boundary/canonicalization), and Firefox/Safari incremental
symmetry with Chromium C2. Without these tests, fingerprint collision
behavior, empty-DB resilience, corrupt-file handling, and URL storage
contracts are undocumented and could regress silently.
What:
- dedup_scenarios_edge_cases.rs (NEW, 564 lines):
• C_SUB_MS (E5): pins sub-millisecond fingerprint collision as known
limitation — two visits at same ms to same URL collapse to one
• E6: URL canonicalization contract — trailing slash, fragment, mixed
case all stored verbatim with no normalization
• Empty DB × 3 families: Chromium/Firefox/Safari zero-row fixtures
import without error, summary reports 0/0
• R1a: random bytes file → Err, not panic
• R1b: valid SQLite missing browser tables → Err, not panic
- dedup_scenarios_baselines.rs (+160 lines):
• F_C2: Firefox incremental no-new-data (watermark prevents re-import)
• S_C2: Safari incremental no-new-data (same pattern)
- Registered new module in mod.rs
- Replaced C_SUB_MS TODO in dedup_scenarios.rs with cross-reference
All 598 vault-core tests pass. Rust coverage: 100% (34,423 lines,
1,611 functions).
…ansion Why: The audit doc §4 and §6 need to reflect the 9 newly implemented scenarios, and the CHANGELOG needs the work-block closeout entry so future agents know what was done and what remains. What: - import-dedup-audit.md §4: sub-ms TODO → implemented cross-reference; URL canonicalization section → E6 test reference - import-dedup-audit.md §6: added F_C2, S_C2, C_SUB_MS, E6, Empty DB×3, R1a, R1b to contract scenarios table - CHANGELOG: appended WORK-IMPORT-TEST-HARNESS-B closeout entry with detailed test list, remaining gaps, and verification state
Why: WORK-IMPORT-TEST-REMAINING-A had E6, C_SUB_MS, empty DB, R1, and F_C2/S_C2 listed as todos. These are now implemented — the BACKLOG entry needs to reflect what's done vs. what remains so the next agent picks up the right work. What: Split WORK-IMPORT-TEST-REMAINING-A into completed items (with commit reference) and remaining items (maintainability review, Takeout ptoken/visitedAt, R2/R3, E1-E4).
…dAt coverage Why: The import-dedup audit identified several untested contract boundaries: - E1-E4 time edge cases (epoch 0, year-2038, far-future, negative→clamped) were theoretical assumptions with no test pins - Takeout `ptoken` field was silently dropped by fixtures, breaking evidence round-trip assertions - Takeout `visitedAt` ISO-8601 fallback path was completely untested because the fixture writer always emits `time_usec` - Records without any time field had no test proving silent-skip behavior What: - vault-core/dedup_scenarios_edge_cases: +4 tests (E1-E4) verifying timestamp storage boundaries including negative-timestamp clamping to 0 - browser-history-fixtures/takeout: added `ptoken: Option<String>` field with serialization support and unit test - browser-history-fixtures/takeout_roundtrip: ptoken evidence assertion in standard roundtrip, new `visitedAt` ISO parse test with hand-crafted JSON, new missing-time-field silent-skip test - vault-core/dedup_scenarios: fix compilation — add `ptoken: None` to `takeout_record` helper after fixture API change
… + Takeout coverage Why: The test additions from 30febca need corresponding doc traceability so future readers can map scenarios to code and track remaining gaps. What: - import-dedup-audit.md §6: added 7 contract scenario rows (E1-E4, Takeout ptoken evidence, visitedAt ISO fallback, missing-time-field skip) - CHANGELOG: appended WORK-IMPORT-TEST-REMAINING-A partial closeout with test inventory, remaining gaps, and verification stats - BACKLOG: updated WORK-IMPORT-TEST-REMAINING-A completed/remaining lists — only dedup_scenarios.rs refactor execution and blocked infra items remain
Why: dedup_scenarios.rs was 1274 lines (above the 1200-line maintainability review threshold in AGENTS.md). The review phase (completed in the prior session) documented a split proposal; this commit executes it with zero behavior change. What: - Extract T1/T2/T2b/T3/T5 → new `dedup_scenarios_takeout.rs` (561 lines) - Move F2/S2 + their Firefox/Safari snapshot/visit helpers → `dedup_scenarios_baselines.rs` (806 → 980 lines) - Main file shrinks from 1274 → 641 lines (Chromium-only: C1-C4, X1) - Removed 8 now-unused fixture imports from main file - Updated module doc to list the four companion modules - Registered `dedup_scenarios_takeout` in mod.rs How: Behavior-preserving move — each satellite module duplicates the shared ScenarioEnv / run_one_ingest / count_* helpers per the established pattern (test-only #[cfg(test)] modules cannot share private helpers). All 28 dedup scenarios pass across the 4 modules; 602 vault-core tests total.
…inks Why: The dedup_scenarios.rs split moved tests to new modules; the audit doc and BACKLOG need updated links and completion status. What: - BACKLOG: mark WORK-IMPORT-TEST-REMAINING-A as [x] complete with closeout note; remaining R2/R3 and B5 tracked as individually blocked - CHANGELOG: append maintainability refactor closeout entry with file size summary table - import-dedup-audit.md §6: update 9 scenario links (S2→baselines, T1/T2/T3/T5→takeout, F2→baselines, T2b/B3→takeout, B4→takeout, B6→takeout)
…cenarios Why: The audit doc §5 lists "ChatGPT Atlas / Perplexity Comet keep their product identity" as a provenance contract and "re-import after appending new rows" as a primary dedup scenario. Both were unpinned by tests — provenance for Atlas/Comet relied entirely on X1 (which only covers Edge), and incremental import was only covered by C2 (zero new data) and C3 (new visit on OLD URL). What: - X2 — `x2_chromium_family_products_preserve_browser_product_identity`: imports 3 Chromium-family profiles (Atlas, Comet, Chrome), asserts each `source_profiles.browser_product` matches its source `browser_name` verbatim and that `browser_kind` (derived from profile_id prefix) distinguishes them. Pins playbook §156-161 contracts. - C5 — `c5_chromium_incremental_append_new_urls_and_visits`: re-import where second pass adds 2 new URLs + 2 new visits (no overlap with first pass). Asserts watermark lets only the new rows land, originals stay deduplicated, summary reports the exact counts, and new visit timestamps round-trip correctly. - audit doc §6: 2 new scenario rows added to the contract table. How: Both scenarios use the existing `chromium_profile` helper and `ChromiumHistoryFixture`. Zero new helper functions, zero new dependencies. 604 vault-core tests pass (was 602 → 604). Main file: 641 → 868 lines (still under 1200 threshold).
Why: Chrome's `History` schema grows over time — real Chrome adds columns like `favicon_id` on `urls` and `segment_id` / `opener_visit` / `originator_cache_guid` on `visits` across releases. The parser uses explicit column lists in its SELECTs (INGEST_URLS_SQL, INGEST_VISITS_SQL) specifically so these extras are silently tolerated, but no test pinned that contract — a future refactor switching to `SELECT *` would break on real-world Chrome upgrades without any test catching it. What: - New `c6_chromium_extra_columns_on_source_db_do_not_break_ingest`: writes a normal fixture, then ALTER TABLEs to add 4 real Chrome columns with synthetic non-null values, then ingests and verifies row counts + data project correctly. The synthetic non-null values prove the parser truly ignores the extras (not just tolerates trailing NULLs). - audit doc §6: C6 row added to contract scenarios table. How: Pins the §5.1 "re-import after schema migration in source DB" contract. 605 vault-core tests pass (was 604 → 605).
Why: Real users almost always have multiple Chrome profiles (Default,
Profile 1, etc.). The dedup contract requires per-profile isolation on
three axes: (1) distinct source_profiles rows under same browser_kind,
(2) per-profile fingerprint scope so identical visits across profiles
don't dedup, (3) per-profile watermark isolation so one profile's import
doesn't affect another's incremental state. No test pinned these
contracts — a future refactor that keyed watermarks by browser_kind
instead of source_profile_id would silently break multi-profile users.
What:
- New `x3_multiple_profiles_within_same_browser_stay_independent`:
- Pass 1: imports same URL+visit under chrome:Default with source_visit_id=10
- Pass 2: imports same URL+visit under chrome:Profile 1 with source_visit_id=99
→ must NOT dedup (per-profile fingerprint scope)
- Pass 3: incremental re-import of Profile 1 with 2 new URLs+visits
→ Profile 1's own watermark advances; Default stays untouched
- Asserts final counts, archive totals, browser_kind / browser_product /
profile_name metadata round-trip
- audit doc §6: X3 row added to contract scenarios table.
How: 606 vault-core tests pass (was 605 → 606). dedup_scenarios.rs is now
1170 lines (approaching the 1200 review threshold — subsequent Chromium
scenarios should go to satellite modules or a new file).
Why: The four new contract scenarios added this session (X2 Atlas/Comet provenance, C5 append-new-rows, C6 schema tolerance, X3 multi-profile) close all remaining unblocked §5 audit contracts. Closeout entry maps each test to the audit gap it fills and notes file size + remaining infrastructure-blocked items. What: CHANGELOG appended with batch summary covering 4 commits (ec95f4f/325d4dc4/cd6b65d5/this), new test inventory, audit doc updates, file size impact warning, verification stats, and final contract coverage status.
Why: Real-world browsing data routinely has (1) URLs with NULL titles (pages that never finished loading, binary downloads) and (2) Unicode content (CJK titles, percent-encoded paths, emoji, em-dashes). Neither contract was pinned — a future refactor could silently start storing empty strings for NULL titles or applying NFC/NFD normalization to Unicode without any test catching it. What: - E7 — `e7_null_title_imports_with_null_archive_title`: imports two URLs (one with NULL title, one with non-NULL); asserts NULL projects as NULL in archive (not empty string) and non-NULL round-trips normally. - E8 — `e8_unicode_urls_and_titles_round_trip_byte_identical`: imports three Unicode shapes — CJK Traditional Chinese title with em-dash, percent-encoded path containing %E6%B8%AC%E8%A9%A6, and emoji 🚀 in title. Asserts byte-identical round-trip with no NFC/NFD normalization or case folding. Pins percent-encoded path stays VERBATIM (not decoded). - Module doc + audit doc §6 updated with E7/E8 references. How: 608 vault-core tests pass (was 606 → 608). edge_cases.rs grows modestly with two focused tests; no helper changes needed.
Why: This session brought the dedup test harness to a stable, near-final state covering all unblocked audit contracts. The summary captures the final scenario count, file distribution, and remaining infrastructure- blocked items for the next agent. What: Appended E7/E8 closeout entry to the prior session block, plus a "Final test harness state" summary table mapping the 34 scenarios across the 4 dedup modules, with line counts and audit traceability.
Why: Prior commits added rows to markdown tables that didn't pass prettier's table-column alignment check. `bun run check:base` failed on format gate. What: Re-aligned column widths in the §6 contract scenarios table and the CHANGELOG closeout entries. Pure whitespace; no semantic changes.
Why: Real Chrome marks redirect intermediates, certain extension URLs, and explicitly-hidden items with `hidden = 1`. No test pinned that the parser preserves this flag verbatim on first-time import — C-series only exercises `hidden: false`, and C4 (B1 fix) only tests `hidden: true` in the regression-prevention context. What: E9 in dedup_scenarios_edge_cases.rs imports two URLs (one hidden=false visible page, one hidden=true redirect intermediate) and asserts: archive `hidden = 0` for the visible URL, `hidden != 0` for the hidden URL (proves preservation, not silent drop or default). How: Sibling pattern to E7 (NULL title) and E8 (Unicode round-trip) in the edge_cases module. Audit doc §6 updated with E9 row.
…t test harness
Why: User confirmed this PR scope is complete ("這個 PR 就先這樣") and asked
for follow-up work to be written up in detail for later execution. The
import test harness now covers all unblocked §5 audit contracts; the
remaining valuable additions need explicit BACKLOG entries so the next
agent can pick them up cold.
What: BACKLOG additions (4 new blocks):
1. WORK-IMPORT-FIXTURE-SIDECARS-A — extend ChromiumHistoryFixture to
write downloads / keyword_search_terms / favicons / favicon_bitmaps /
icon_mapping tables; add T6-T9 end-to-end scenarios. CHANGELOG had
flagged this as a known untested area.
2. WORK-IMPORT-TEST-MINOR-A — 5 narrow contract pins as E10-E14:
visit_count edges, from_visit referential integrity, visit_duration
round-trip, Safari synthesized flag, Firefox visit_type enum.
3. WORK-IMPORT-TEST-PARSER-ORDERING-A — unit test the silent-skip
behavior in ArchiveChunkConsumer::visits when url_id_map misses
(audit §4 contract).
4. WORK-IMPORT-TEST-CONCURRENCY-A — audit + integration test for
same-profile concurrent ingest safety (audit §4 watermark race).
CHANGELOG: appended final session entry covering E9 (commit 8bc8b5c),
final 35-scenario state, and the four future-work BACKLOG entries.
Also notes the one unrelated E2E flake observed in `bun run check`
(`desktop-bridge.spec.ts:223` socket-hangup — network-level failure
with no connection to Rust-only test additions).
audit doc: §6 contract table received the E9 row.
Each new BACKLOG block follows the established pattern: 讀先 list with
absolute paths, 觀察 noting the audit gap, 目標 with specific named
test functions, 契約 enforcing safety/quality invariants, 驗收 with
green-gate requirements.
…newer Why: Code review against feat/v0.3-redesign-2 surfaced that the B1 fix (6884c10) protected the backup-pipeline URL upsert in writes.rs but left the same bug live in two takeout code paths, and that the original fix used `>=` on last_visit_ms for title/hidden — which silently overwrites a captured non-NULL title with NULL at equal timestamps (common case: Firefox bookmark-only URLs whose last_visit_date is 0, re-imported on every sync). What: - writes.rs upsert_url: change CASE WHEN gate from `>=` to `>` for title / hidden / url. Wrap payload_hash and recorded_at in the same strict-newer gate (were previously unconditional overwrites that defeated the older-snapshot guard for audit-trail fields). - vault-core/takeout/browser_history.rs upsert: mirror the writes.rs guards on title / visit_count (MAX) / typed_count (MAX) / hidden / url / payload_hash / recorded_at. The previous code unconditionally overwrote these with `excluded.*`. - vault-core/takeout/payload_import.rs upsert: same fix, plus add visit_count and typed_count to the UPDATE clause (they were missing entirely — INSERT VALUES hardcoded 1, 0 and UPDATE never touched them, so Takeout URLs stayed frozen at the first import's count regardless of how many later visits were observed). - ingest/mod.rs ArchiveChunkConsumer::visits: gate track_url_visit_bounds on `inserted > 0`. Previously the call ran unconditionally, so when INSERT OR IGNORE silently dropped a visit (clock-corrected timestamp re-using a source_visit_id) the URL's first_visit_ms / last_visit_ms widened from a visit row that was never stored, leaving the canonical urls table claiming bounds with no matching visit row. How: 3 new regression scenarios added — C7 (tied last_visit_ms tie-break preserves captured state), T6 (Takeout payload_import older-snapshot re-import doesn't regress), and the existing C4 / F2 / S2 / dedup scenarios continue to pass. 613 vault-core tests pass.
…en stable_key_i64 Why: Code review surfaced two B3-area defects in the Takeout parser: 1. The fix in 6884c10 changed source_visit_id from `{path}:{ordinal}:{url}` to `{url}:{visit_time_micros}` for cross-path stability. That gained dedup correctness across renames but lost per-record uniqueness — Google Takeout legitimately emits multiple records for the same URL within the same microsecond (sync replay, redirect within 1µs, multiple devices syncing the same event). All such records collided on the same source_visit_id and silently dropped via INSERT OR IGNORE. 2. `stable_key_i64` returned `acc.abs()` on a wrapping-add accumulator. For the input that hashes to `i64::MIN`, `.abs()` returns `i64::MIN` in release builds and panics in debug builds. Either way the non-negative-key contract this `.abs()` was meant to enforce silently breaks. What: - parse_browser_record: source_visit_id now hashes `{url}:{visit_time_micros}:{ordinal}` so same-URL-same-microsecond records get distinct keys. `ordinal` is the record's position in the source file — stable across re-imports of the same file (Google's Takeout JSON is a deterministic export), so the cross-path stability the original B3 fix sought is preserved. - stable_key_i64: explicit corner-case branch maps `i64::MIN` to `i64::MAX` instead of returning a negative value or panicking. All other inputs preserve the previous hash output (existing data's source_visit_ids stay stable). - Added stable_key_tests module with smoke test for non-negativity across assorted inputs. - Added T7 scenario in vault-core: same-URL same-microsecond records must produce distinct visit rows, and re-importing the same file (same ordinals) must still dedup. How: 46 browser-history-parser tests pass; 614 vault-core tests pass (+T7 + the stable_key tests). t2 / t2b dedup contract preserved.
…s Chromium) Why: The B2 fix added an OR-subquery to Firefox URLS_SQL to catch long-tail revisits below the URL watermark, but unlike the Chromium parser (which branches between INGEST_URLS_SQL and INGEST_URLS_FULL_SQL on `last_visit_time == 0 && last_visit_id == 0`), the Firefox path always ran the OR variant. On a first import with both watermarks at 0, the predicate `last_visit_date >= 0` already matches every place; the OR subquery `SELECT DISTINCT place_id FROM moz_historyvisits WHERE id > 0` is pure waste — SQLite still materializes an ephemeral B-tree of every distinct place_id across all visits before the outer filter runs. On the AGENTS.md design ceiling (14.4M-visit Firefox profile, target machine 4-core / 8GB RAM) this is a multi-GB transient and multi-minute stall on every cold-start import — exactly the regression the Chromium codepath explicitly guards against. What: - Add URLS_FULL_SQL (no OR clause, no bound params) alongside URLS_SQL. - stream_history branches on `first_import = after_visit_id == 0 && after_url_last_visit_ms == 0`, picking the simpler SQL when true. - Match Chromium's chromium/mod.rs:100,383-384 pattern exactly. How: All 7 Firefox parser tests pass; the F2 / F_C2 / F1 scenarios in vault-core continue to exercise the OR-fallback path on subsequent imports.
… MITM DoS) Why: `resolve_image_url_via_api_with_base` called `response.bytes().ok()?` and only checked the size AFTER fully buffering the body. The comment above said "Cap the body before the JSON parse so a misbehaving endpoint can't blow memory" but the code did the opposite: a hostile or MITM'd api.bilibili.com returning a multi-GB JSON response would fully materialize before the 64 KiB cap fired, OOM-killing the og:image worker on the AGENTS.md target machine (4-core / 8GB RAM). The override path is real — dev / test environments can point BILIBILI_API_BASE elsewhere. What: - Hoist the 64 KiB cap into a named constant `BILIBILI_API_BODY_CAP_BYTES`. - Add a Content-Length fast-path: if the server declares Content-Length above the cap, short-circuit before reading any body bytes. - Add a `read_response_with_cap` helper that stream-reads the body through a fixed-size buffer and aborts as soon as the running total exceeds the cap. Defends against servers that lie about Content-Length or omit it entirely while streaming gigabytes. - New unit test pins the streaming-cap contract via a fake Read impl that records how much was drained — proves the helper doesn't drain far beyond the cap. How: 41 og_images_synth tests pass (40 existing + new streaming-cap test). The existing `body_exceeds_cap` mockito test still passes via the Content-Length fast-path. Privacy posture unchanged (still no Referer, no cookies, same UA).
Why: The Pop branch of the route-history-nav effect only decremented stackIndex without touching forwardAvailable. The in-app `goBack` callback set forwardAvailable=true before calling navigate(-1), so clicking the topbar back button worked. But clicking the BROWSER back arrow also fires a Pop event and bypasses the in-app callback — the forward chevron stayed greyed out even though forward navigation was now actually available, stranding the user mid-history. What: - Add `expectingForwardPopRef` to distinguish goForward-initiated Pops from external Pops. `goForward` sets the tag before navigate(1); the Pop effect consumes the tag and skips the forwardAvailable update. - All other Pops (browser back, history.go(-N), in-app goBack) set forwardAvailable=true. The user just stepped backward, so forward is now reachable — the topbar chevron mirrors the browser's actual state. - New regression test simulates browser-back via navigate(-1) directly (bypassing goBack callback) and asserts canGoForward becomes true, then asserts goForward consumes it back to false. How: 15/15 use-route-history-nav tests pass, including the new browser-back regression and the existing "goBack arms forward / goForward clears it" and "Ctrl+] fires goForward after back step" tests that were initially broken by a naive first-pass fix.
… doc) Three independent low-risk cleanups surfaced by review: 1. **Drop unused chrono dep from browser-history-fixtures** — declared in Cargo.toml but never used in src/ (grep confirms zero `chrono::` or `use chrono` matches). Fixture time helpers in time.rs use plain integer arithmetic. Removes a transitive supply-chain surface plus compile time for downstream consumers; satisfies AGENTS.md dep discipline. lib.rs doc comment updated to reflect the actual approach. 2. **Align fixture chrome_time_to_unix_ms with production's `.max(0)` clamp** — the fixture's inverse helper was documented as "The inverse of unix_ms_to_chrome_time" but diverged from production for negative inputs (pre-1970 chrome timestamps): production clamps to 0, fixture returned negative. New test pins the symmetric clamping behavior so fixture-side verification helpers stay aligned with archived state. 3. **Update F2 stale doc comment in dedup_scenarios_baselines.rs** — the doc text still claimed Firefox parser "lacks that fallback" and that the test is "should_panic today; flip to plain #[test] after Firefox grows the OR fallback". The fix has been in place since 6884c10 and the test is already plain #[test]. Rewrite the comment to describe what the test actually pins (the existing OR fallback against regression) so future debuggers chasing a failure aren't sent looking for code that already exists.
…r branches are unit-testable Why: The previous `read_response_with_cap(response: Response, ...)` took `reqwest::blocking::Response` directly, which made the cap-exceeded and read-error branches uncoverable in unit tests without standing up a streaming mockito server with chunked encoding. The Rust coverage gate flagged the two `return None` lines as uncovered after the og:image memory-DoS fix landed. What: - Rename `read_response_with_cap` to `read_with_cap` and switch the parameter from `reqwest::blocking::Response` to `R: Read`. Production call site passes Response (which implements Read), so no behavior change. - Replace the previous inline reader-simulation test with three focused tests that drive the helper directly via a plain `&[u8]` slice and a fake `ErrorReader`: under-cap success, cap-exceeded branch, and Read-error branch. - Use `std::io::Error::other(...)` (stable in Rust 1.74) for the error reader. How: 3 read_with_cap tests pass (+612 other tests). The `body_exceeds_cap` mockito test continues to exercise the Content-Length fast-path. Full Rust coverage gate clean.
…at/v0.3-redesign-2) Why: User requested all valid review findings be fixed before merging this branch into feat/v0.3-redesign-2. 10 of 15 findings were implemented with trade-off worthwhile; this CHANGELOG entry traces each fix back to its review finding, names the new regression tests, and documents the trade-offs that were intentionally left as-is. What: Append closeout block to CHANGELOG covering: (1) correctness fixes for B1-still-live, tie-break asymmetry, bounds-on-IGNORE, B3 ordinal collision, stable_key_i64 overflow, og:image memory DoS, browser-back forward-state, (2) Firefox first-import perf, (3) test + doc hygiene including watermark assertions and misleading-comment corrections, (4) list of 7 new regression tests added across vault-core and frontend, and (5) the one finding (MAX visit_count) explicitly deferred as a B1 audit design trade-off.
…pendent of fixes Why: `bun run check` after the code-review batch landed reproduced the same `apiRequestContext.post: socket hang up` failure on `tests/e2e/desktop-bridge.spec.ts:223` that the prior session also hit and documented. To rule out whether any of the new Rust changes caused it, I reverted just writes.rs + ingest/mod.rs to b249ea7 (the pre-fix state) and re-ran the same e2e — same socket hang-up. So the failure is purely environmental / pre-existing and not gated on this branch. What: CHANGELOG closeout note updated to: - Reference the specific test + symptom - State the revert experiment that proved independence - Restate that the Rust changes only touch SQL inside the backup transaction + per-visit bookkeeping, neither of which can affect the dev-IPC HTTP server's connection lifecycle
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this exists
The user reported observing apparent duplication after importing browser
history into PathKeep and asked for a robust test infrastructure before more
browser adapters land. An audit of
archive/ingest/*and the four familyparsers (Chromium / Firefox / Safari / Google Takeout) shows two distinct
things tangled together in that report:
Cross-browser "visual duplication" is the deliberate schema contract,
not a bug. Every dedup key in the canonical archive is scoped by
source_profile_id(seemigrations/002_archive_runtime_foundation.sql).A URL visited in both Chrome and Edge intentionally produces two
urlsrows — provenance is preserved per browser, and the view layer is where
cross-browser aggregation belongs. The view-layer work is a separate work
block; this PR is storage-layer truth only.
Six real ingest bugs that the harness must encode as failing tests
before any fix lands. Listed below with audit IDs.
The work block is
WORK-IMPORT-TEST-HARNESS-A, queued inBACKLOG.md.Full audit and harness spec are in:
docs/plan/program/import-dedup-audit.mddocs/plan/program/import-test-harness-spec.mdWhat's landed in this draft
Planning artifacts
docs/plan/program/import-dedup-audit.md(~360 lines): per-table dedupkeys, the six bugs with
file:lineevidence, per-family behavior summary,schema invariants that cannot be enforced by SQLite alone, and a living
"scenarios backed by tests" index.
docs/plan/program/import-test-harness-spec.md(~450 lines): cratelayout, Scenario DSL sketch, fixture-generator API, assertion-helper API,
scenario library prioritized into six tiers, real-data redaction policy,
acceptance criteria for the work block.
BACKLOG.mdentry:WORK-IMPORT-TEST-HARNESS-Ablock + 2026-05-25planning note.
New crate:
browser-history-fixturesPure Rust fixture generators that emit real-format browser history files
from declarative records. No third-party dependencies added — only
workspace deps (
chrono,rusqlite,tempfile, plusbrowser-history-parseras a dev-dep for round-trip self-validation).src/time.rs— Unix-ms ↔ Chrome microseconds-since-1601 helpers withsaturating arithmetic, numerically pinned to
vault_core::utils.src/chromium/mod.rs—ChromiumHistoryFixturebuilder writes theurls+visitsshape the production Chromium parser reads.src/firefox/mod.rs—FirefoxPlacesFixturewritesmoz_places+moz_historyvisits(Unix-μs timestamps).src/safari/mod.rs—SafariHistoryFixturewriteshistory_items+history_visitswith selectableMinimalvsCurrentmacOS schemavariants (the latter includes
load_successful,synthesized,redirect_*,origin,generation,attributes,score).src/takeout/mod.rs—TakeoutBrowserHistoryFixtureemits all threeon-disk layouts the Takeout source classifier accepts: standard
{ "Browser History": [...] }, the alternate no-space{ "BrowserHistory": [...] }, and JSONL one-record-per-line. RealGoogle field names (
page_transition,title,url,time_usec,client_id,favicon_url).Self-validation: parser round-trip per family
Every writer is gated by a
tests/<family>_roundtrip.rsthat constructs asmall fixture, parses it back through the production PathKeep parser, and
asserts every emitted field matches. Without this gate, scenario
failures could be silent generator/parser disagreement rather than real
bugs in product code — a false-positive harness is worse than no harness.
End-to-end ingest scenarios
Lives at
src-tauri/crates/vault-core/src/archive/ingest/dedup_scenarios.rsbecause
process_profile_snapshotispub(super)to thearchivemodule— in-tree placement avoids widening the public surface for testability
alone. Drives the full ingest pipeline:
chromium_baseline_import— one profile, one pass, exact fieldflow-through,
source_visit_idpreserved.chromium_incremental_no_new_data— re-importing withuse_watermark = trueproducesnew_urls = 0,new_visits = 0,archive counts unchanged.
chromium_incremental_revisit_of_old_url— adversarial pass-2fixture where the visit cursor moves past 10 but the URL's
last_visit_timeis left at the old value. Pins theOR id IN (SELECT DISTINCT url FROM visits WHERE id > ?2)fallback inINGEST_URLS_SQL(
chromium/mod.rs:85-90) against future regression — without it, thenew visit's
url_id_maplookup would silently drop the visit.What's still coming in this PR (don't merge yet)
Same work block, future commits on this branch:
needs research into the Takeout-specific ingest command path, which is
separate from
process_profile_snapshot.#[should_panic]with a doc comment pointing at the audit ID. They will flip to plain
#[test]in the fix PRs that follow:source_visit_idstable_key_i64collision at scalediverge; pins the per-profile contract under refactor pressure.
bun run checkdry run to confirm coverage / mutation gates staygreen.
The audit doc's Section 6 "Scenarios Now Backed By Tests" is a living
index — each new scenario lands a row there.
The six bugs the audit uncovered
archive/ingest/writes.rs:123-138visit_count/title/typed_count; onlylast_visit_mshas a "keep newer" guard. Re-importing an older snapshot regresses counts.firefox/mod.rs:31,safari/mod.rs:52>= ?1and lack the long-tail revisitOR id IN (...)fallback Chromium has. Visits to URLs whoselast_visit_timefalls before the watermark get silently dropped viaskipped_visits++.takeout/browser_history.rs:339source_visit_id = hash("{source_path}:{ordinal}:{url}"). Renaming or re-downloading the same Takeout JSON to a different path produces a full duplicate set.takeout/browser_history.rs:381-386app_id = "takeout"andtransition = None, so theevent_fingerprintof a Takeout visit can never match a local-Chrome visit of the same instant. Users running both ingest paths see every visit doubled.takeout/browser_history.rs:442stable_key_i64is a degeneratewrapping_mul(31).wrapping_add(byte).abs()polynomial hash over hex bytes — collisions become likely well below the design's 14.4M-record ceiling.takeout/browser_history.rs:432micros_to_unix_msasserts Unix-microsecond input, but Google'stime_usecis historically Chrome epoch (since 1601). Needs a fixture-pinned contract to resolve which the runtime actually receives.Important non-bugs (architectural contracts being pinned)
CodeRabbit should treat these as expected behavior, not findings:
migrations/002_archive_runtime_foundation.sqllines 16-51. Twobrowsers visiting the same URL store two rows by design.
Cross-browser aggregation is the view layer's responsibility, addressed
in a separate work block.
https://example.comvshttps://example.com/, fragment vs no fragment, IDN vs punycode areall separate canonical rows. The scenario plan documents the current
behavior with passing contract tests rather than treating it as a bug
to fix.
dev-dependenciesarrow intobrowser-history-fixturesfromvault-core. Intentional: lets the fixture crate stay test-infrawithout leaking into the production graph, while scenarios still have
full
pub(super)access toprocess_profile_snapshot.ParsedVisit.visit_duration_msactually carries microseconds forChromium. Existing naming inconsistency in production parser; the
round-trip test pins current behavior with an inline comment linking
to the audit. Renaming is out of scope for this work block (will be a
separate cleanup commit when somebody owns the renames in
vault-core too).
Test plan
cargo build -p browser-history-fixturescleancargo check --workspacecleancargo test -p browser-history-fixtures— 5 unit tests + 10 round-trip integration tests, 15/15 passcargo test -p vault-core --lib dedup_scenarios— C1/C2/C3 all passcargo test -p vault-core --lib— full vault-core suite 572 passed, no regressions in pre-existing 569 tests#[should_panic]with audit-ID doc comments)bun run checkfull gate (100% JS/Rust coverage + mutation + e2e + desktop bridge truth)Files of interest for reviewers
docs/plan/program/import-dedup-audit.md§1–§3 explainthe dedup invariants and the six bugs.
docs/plan/program/import-test-harness-spec.md§3 (fixture generator API) and §5 (scenario library tiers).
src-tauri/crates/vault-core/src/archive/ingest/dedup_scenarios.rsis where future bug-targeted scenarios will land — naming follows the
audit IDs (
c4_…,f2_…,t3_…).src-tauri/crates/browser-history-fixtures/src/safari/mod.rshas the most schema surface to review since it supports both
Minimal and Current macOS Safari shapes.
🤖 Generated with Claude Code