Repository navigation
perf(core): prevalidate bundled phone metadata at build time - #289
Conversation
|
Caution Review failedThe pull request is closed. Finishing Touches
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 |
|
@codex review Please review the cold-start patch and preservation of runtime validation for caller-provided metadata. Only build.rs and src/metadata/database.rs differ from the published crate's Rust sources. The XML is byte-identical; docs/measurements/phone-prevalidation.md has synthetic differential coverage, failure injection, raw samples and the explicit vendoring cost. No account data was accessed. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Additional validation on 30e3e05: |
|
@codex review Please review final commit 50f515d, especially the maintenance guard added after independent review. The core integration target compiles the actual build.rs visitor; 25 invalid-regex positions cover all 16 descriptors, three optional regexes, both format patterns and every leading-digit entry in both lists. Each must also fail the original checked runtime path. The complete XML passes both validators. Deliberately removing mobile from the visitor made the test fail; the mutation was restored. The report now distinguishes RegexBuilder(ignore_whitespace).build() from the old regex-syntax 0.6 Parser validation; they are not the same validator. Added 24 representative synthetic comparison cases and pinned formatted baseline outputs. Both corpora match base/head. The vendor update procedure records checksum, two-file Rust patch, required test/build files and the 1.2 MiB cost. No runtime parsing/formatting changes in this follow-up and no retained-heap gain claimed. Passed core tests + metadata integration tests + doctest, targeted representative regression, Clippy all targets/features, fmt, cargo-machete 0.9.2 and WASM integration. Only build.rs and src/metadata/database.rs differ from published Rust sources; XML/README/LICENSE remain byte-identical. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Final qualification update for 50f515d: Codex completed its requested review with no findings. An isolated copy of the trimmed vendor also passed its own library tests (86 passed, 68 ignored) and doctest (1 passed), confirming no omitted source/test/build file was required by those retained targets. Archive checksum and XML/README/LICENSE identity were checked. Current CI run 41000719222 still has all six jobs queued; draft status remains until required CI completes. No merge or upstream communication. |
|
Web CI triage: this is the same shared harness compile failure as #288, not a phone-vendor-specific error. Normalizing timestamps/ANSI yields identical diagnostics in jobs 123884222100 (#289, run 41000719222) and 123878204949 (#288, run 40998832174):
The daemon's media/avatar_web_tests.rs includes that GUI file using #[path]. Its decode_avatar helper references GUI-only symbols that do not exist in the daemon test crate. That helper was introduced by a65ec85 (#282), already in base 33108ce. The helper, harness, media mod and CI workflow are unchanged in #289. #288 does not contain this vendor and has the exact same errors. The phone backend compiles before the daemon lib-test failure; browser execution has not started. Native Check, Windows, macOS, MSRV and xtask are green for 50f515d. The #288 owner is doing primary triage of the common web issue; no duplicate generic fix is being published here. #289 remains draft until coordinated correction and required web CI pass. |
|
CI triage checkpoint: the shared web failure is a baseline issue from #282, reproduced on main 33108ce before any profiling patch. The daemon imports actual avatar/web.rs storage code, but its GUI decode helper references missing DecodePriority/gpui/crate::session types. Shared fix is isolated in #290, commit d21f091, with all storage code and browser tests preserved. Final local browser suites pass, 16 daemon + 32 session tests. The #288 native fuel timeout is a separate baseline flake. Main passes isolated in 8.47 s and fails under controlled CPU contention with the same timeout. Commit 0da555a in #290 adjusts only the infinite-loop fixture's fuel consumption efficiency; production budget, deadline and assertions remain unchanged. It passes 5/5 under identical contention; full plugin-host suite passes 135 tests, one ignored. Detailed reproduction/classification: docs/measurements/ci-288-289-triage.md in #290. Neither SQL optimization nor phone metadata changes are included there. #290 remains draft pending remote CI and requested Codex review; no merge performed. |
|
@codex review Integrated the common CI fix branch from #290 with a no-conflict merge, preserving all original commits through 71130e9. Final merge head dc1a994. No SQL #288 changes: queries.rs and lid.rs remain identical to main 33108ce. Please review the final integration, retaining the previous phone-vendor review context. The imported fixes separate the GUI decode adapter, keep fuel budgets/deadlines/assertions, and make avatar workflow instrumentation idempotent. Local integration checks and fresh CI are in progress; no PR merge. |
|
Codex Review: Didn't find any major issues. Chef's kiss. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Integration validation on dc1a994:
Fresh CI 41018950607 and supporting workflows are being monitored; keep draft until required jobs complete. No PR merge. |
The first legitimate phone label currently validates every bundled metadata regex at runtime. Validate the immutable data during the build and retain the original lazy regex cache, international rules and checked runtime path for caller-provided databases.
The build uses RegexBuilder(ignore_whitespace).build(); the previous CachedRegexBuilder check uses regex-syntax 0.6 Parser. They are different validators. The complete bundled XML is tested through both paths. A core integration target compiles the actual build.rs visitor and injects invalid regexes into 25 positions: all 16 descriptors, three optional regexes, national/international patterns and every leading-digit entry in both format lists. A deliberate mobile-visitor omission made this guard fail.
On client 33108ce / core af2e018c, 15 alternating fresh-process samples measured 94.86 ms → 8.01 ms for the cold call and 355,830 → 61,975 allocation/reallocation calls. Warmed calls and retained heap are unchanged. These are local microprobe results, not whole-app startup or guaranteed RSS gains. The CSV records runtime patch 30e3e05; follow-up 50f515d adds coverage and maintenance documentation without changing runtime parsing/formatting.
This carries about 1.2 MiB of vendored phonenumber 0.3.10+9.0.33. Only build.rs and src/metadata/database.rs differ from published Rust sources; XML, README and LICENSE are byte-identical. LOCAL_CHANGES.md records the archive SHA-256, required build/test files, two-file patch update procedure and removal condition. No version updates or new production dependencies; regex has an additional test-only edge.
Validation:
Full CSV, ranges and reproduction: docs/measurements/phone-prevalidation.md. No real account data used.
The shared CI fixes from #290 were integrated without conflict at merge head dc1a994. Original commits through 71130e9 are preserved. This imports the GUI decode/storage boundary fix, fuel-fixture fixes preserving budgets/deadlines/assertions, and idempotent avatar-workflow instrumentation. SQL #288 changes are not included; queries.rs and lid.rs remain identical to base main.
Qualified on integration head dc1a994. Codex reviewed this exact head without findings: #289 (comment). No unresolved review threads were present at qualification.
All final workflows passed:
Local core/plugin-host tests, Clippy all targets/features, fmt, cargo-machete and daemon/session WASM harness compilation also passed. Technical qualification is complete; the PR currently remains draft for the user to change its review status and merge manually. The tradeoff remains about 1.2 MiB of vendored source/metadata; retained heap and warm-call cost are unchanged. No PR merge or deployment performed.