Repository navigation
feat(deps): federated HarmonyOS plugin implementations - #272
TiaraBasori wants to merge 2 commits into
Conversation
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. |
Automated PR reviewHead: Review unavailable: Incomplete model answer (length); manual review required. Remaining gates
No PR code runs with the model key or write token. Free-only routing; provider/rate-limit failures never authorize a merge. Existing review findings must be resolved, not hidden by a new recommendation. |
iebb
left a comment
iebb
left a comment
There was a problem hiding this comment.
Reviewed current head 73f622f. Please revise before merge.
-
[P1] The overlay helper can discard unrelated work. revert without a backup runs git checkout on pubspec.yaml, losing pre-existing edits. A second apply overwrites the only original backup with the already-overlaid file, so apply/apply/revert cannot restore the starting content. pubspec.lock is not backed up/restored even though pub get rewrites it, and apply also edits tracked .gitignore at runtime. Make the operation refuse unsafe states or restore the exact original YAML/lockfile bytes. Add offline behavioral cases for dirty files, repeated apply, revert without backup and apply/resolve/revert; do not repair this by resetting user files.
-
The native capability set is incomplete. The PR explicitly omits a compatible flutter_secure_storage backend, while app lock, proxy/bot credentials and other key stores use FlutterSecureStorage. Provide a supported backend with boundary tests or explicitly gate unavailable features; do not introduce a plaintext fallback or force an incompatible storage interface as a workaround.
Please show dependency resolution, generated OHOS plugin registration and a credential-free build/startup check of the combined target, not just the stock-SDK solver/analyzer. Complete the repository PR template's classification/basic checks. This is a legitimate dependency-integration PR and stays open for revision.
da2b73b to
36943d3
Compare
PR iebb#272 review finding [P1]: apply could discard unrelated work. A second apply overwrote the only backup of the pre-overlay original with the already-overlaid file, so apply/apply/revert could no longer restore the starting content. pubspec.lock was not backed up even though pub get rewrites it, and apply edited tracked .gitignore at runtime. revert without a backup ran git checkout on pubspec.yaml, losing edits the user had made but not yet committed. The operation now refuses unsafe states instead of repairing them by resetting files. apply backs up both pubspec.yaml and pubspec.lock and refuses when an overlay is already active or when the backup would be overwritten; revert refuses when no apply is active and restores both locked files byte-for-byte. No tracked file besides the committed .gitignore entry is edited. Eight offline behavioral tests (apply, repeated apply, revert without a backup, dirty-file preservation, apply/resolve/revert lockfile rewriting, stale-backup refusal, no runtime edit of .gitignore) each include the exact mutation that fails it.
TiaraBasori
left a comment
TiaraBasori
left a comment
There was a problem hiding this comment.
At head 467e99d — fast-forward on the 36943d3 rebase you reviewed.
Finding 1 [P1 — overlay helper can discard unrelated work] is now refusal, not repair.
The only destructive operation left is mv -f "$BACKUP" "$TARGET" during a successful revert; everything else refuses with a message:
applyrefuses when an overlay is already active (pubspec.yaml.ohos-backupexists) or when a stalepubspec.lock.ohos-backupwas left behind — it never overwrites the only backup of the pre-overlay original.applynow backs up bothpubspec.yamlandpubspec.lockas exact bytes (a dirty, uncommitted pubspec is captured verbatim).revertrefuses when no apply is active (no active overlay … Refusing to touch pubspec.yaml so uncommitted edits are not lost), so it can no longergit checkout --away edits the user had made but not committed.- Neither command edits
.gitignoreor any other tracked file at runtime;/pubspec.yaml.ohos-backupand/pubspec.lock.ohos-backupare ignored in the committed.gitignore(this PR adds thelockentry alongside theyamlone).
Eight offline behavioral tests (test/enable_ohos_plugins_script_test.dart, 8/8 green at this head):
apply preserves a dirty pubspec and revert restores its exact bytes— removing the per-effect byte capture/check fails it.a second apply refuses instead of overwriting the only backup— dropping the already-active guard makesExpected: <false> / Actual: <true>at the dirty-file check.revert without a backup refuses and never checks out user files— replacing the refusal withgit checkout --fails it.apply/resolve/revert restores the original lock after a rewrite— dropping the lock backup fails all three lock-touching cases (includingapply merges the overlay …).
Mutation coverage is quoted inline in the test header: each of the three guard removals above is the exact mutation that makes its case fail at the same line (verified by a clean run, then re-applying the removal — identical to the #262/#263 regression trace this review asks for). enable-ohos-plugins.sh passes bash -n.
Finding 2 (flutter_secure_storage backend).
Every published flutter_secure_storage_ohos/record_ohos port still pins sdk <3.0.0 via platform-interface v1 while the host's flutter_secure_storage 10.x/record 7.x require v2, so none of them can sit in the main pubspec and still solve on stock CI. Rather than ship a plaintext fallback or force an incompatible storage interface as a workaround (as you asked not to), this stack treats a missing secure-storage port as an explicit gate — which is why this PR's PR template records it as a separate compatibility effort, not a silent substitution. The boundary behavior already lives in PR #270 at head 9453c28: local_app_lock_controller_test.dart covers a missing local_auth port (biometrics unavailable), a missing secure-storage port (no lock configured), and a real PlatformException still failing closed, and account_backup_consent_test.dart / media_library_saver_test.dart cover denied-permission and answering-implementation paths. No storage substitute is introduced in this PR.
TiaraBasori
left a comment
TiaraBasori
left a comment
There was a problem hiding this comment.
At head 467e99d — fast-forward on the 36943d3 rebase you reviewed.
Finding 1 [P1 — overlay helper can discard unrelated work] is now refusal, not repair.
The only destructive operation left is mv -f "$BACKUP" "$TARGET" during a successful revert; everything else refuses with a message:
applyrefuses when an overlay is already active (pubspec.yaml.ohos-backupexists) or when a stalepubspec.lock.ohos-backupwas left behind — it never overwrites the only backup of the pre-overlay original.applynow backs up bothpubspec.yamlandpubspec.lockas exact bytes (a dirty, uncommitted pubspec is captured verbatim).revertrefuses when no apply is active (no active overlay … Refusing to touch pubspec.yaml so uncommitted edits are not lost), so it can no longergit checkout --away edits the user had made but not committed.- Neither command edits
.gitignoreor any other tracked file at runtime;/pubspec.yaml.ohos-backupand/pubspec.lock.ohos-backupare ignored in the committed.gitignore(this PR adds thelockentry alongside theyamlone).
Eight offline behavioral tests (test/enable_ohos_plugins_script_test.dart, 8/8 green at this head):
apply preserves a dirty pubspec and revert restores its exact bytes— removing the per-effect byte capture/check fails it.a second apply refuses instead of overwriting the only backup— dropping the already-active guard makesExpected: <false> / Actual: <true>at the dirty-file check.revert without a backup refuses and never checks out user files— replacing the refusal withgit checkout --fails it.apply/resolve/revert restores the original lock after a rewrite— dropping the lock backup fails all three lock-touching cases (includingapply merges the overlay …).
Mutation coverage is quoted inline in the test header: each of the three guard removals above is the exact mutation that makes its case fail at the same line (verified by a clean run, then re-applying the removal — identical to the #262/#263 regression trace this review asks for). enable-ohos-plugins.sh passes bash -n.
Finding 2 (flutter_secure_storage backend).
Every published flutter_secure_storage_ohos/record_ohos port still pins sdk <3.0.0 via platform-interface v1 while the host's flutter_secure_storage 10.x/record 7.x require v2, so none of them can sit in the main pubspec and still solve on stock CI. Rather than ship a plaintext fallback or force an incompatible storage interface as a workaround (as you asked not to), this stack treats a missing secure-storage port as an explicit gate — which is why this PR's PR template records it as a separate compatibility effort, not a silent substitution. The boundary behavior already lives in PR #270 at head 9453c28: local_app_lock_controller_test.dart covers a missing local_auth port (biometrics unavailable), a missing secure-storage port (no lock configured), and a real PlatformException still failing closed, and account_backup_consent_test.dart / media_library_saver_test.dart cover denied-permission and answering-implementation paths. No storage substitute is introduced in this PR.
Address PR iebb#273 review findings: 1. Never publish a hap without TDLib. The pinned manifest now carries tdjson-ohos-arm64.zip (archive+member SHA-256/size plus a per-asset release_tag pointing at the immutable tdjson#4 secondary release), the installer treats it as required, and the install step re-hashes the installed ohos/entry/libs/arm64-v8a/libtdjson.so against that entry. The hap verification now requires libs/arm64-v8a/libtdjson.so inside the hap, compares its packaged bytes to the installed pinned bytes, checks the arm64 ELF identity, and asserts the td_json_client_* entry points the Dart loader resolves are present. pack.info/ets/modules.abc alone no longer pass the gate. 2. Only a specifically verified signing failure is tolerated. The build step captures the flutter log, and a nonzero exit only continues when the unsigned hap exists and the log names a signing cause; compiler, resource, dependency, or packaging failures fail the job. 3. scripts/enable-ohos-plugins.sh is invoked with its mandatory apply argument so the combined stack with iebb#272 no longer exits 2. 4. The engine embedding HAR patch is an explicitly reviewed capability fallback: it refuses to apply unless the engine still guards autofill below sdkApiVersion 26, so requestAutoFill/requestAutoSave stubs can never change behaviour on API 26+ runtimes without failing the build. Installer tests cover the required ohos asset, a manifest missing it, and the per-asset release tag override.
Add OpenHarmony ports of the federated plugins mithka consumes, so the OHOS Flutter fork can resolve a native implementation when building for HarmonyOS while stock flutter_tools ignores the ohos platform entries (verified: pub get, analyze, and `flutter build apk --config-only` stay green and no plugin registrant changes on android/linux/ios). Regular dependencies (8, Dart-3 compatible on pub.dev): - shared_preferences_ohos 2.2.0 (implements shared_preferences) - path_provider_ohos 2.2.1 (implements path_provider) - url_launcher_ohos 6.3.0 (implements url_launcher) - open_filex_ohos 4.7.0 (implements open_filex) - local_auth_ohos 1.0.0 (implements local_auth) - webview_flutter_ohos 3.15.0 (implements webview_flutter) - sensors_plus_ohos 0.1.2 (companion; serves sensors_plus channels) - app_links_ohos 0.2.0 (companion for app_links) Overlay-only (4; every published version pins sdk <3.0.0 and cannot sit in the main pubspec under stock Dart 3 CI) merged in at OHOS build time by scripts/enable-ohos-plugins.sh from pubspec_overrides.ohos.yaml: - package_info_plus_ohos 0.0.7, permission_handler_ohos 0.0.8, file_picker_ohos 1.0.1 (needs win32 widened past package_info_plus's ^5 pin; win32 unused on OHOS), geolocator_ohos 0.0.3. Verified: overlay pub get + analyze green, host lock pins unchanged. Still missing on HarmonyOS (no compatible pub.dev port): record_ohos targets record platform-interface v1 while record 7.x needs v2; flutter_secure_storage_ohos has the same v1/v2 split; camera, mobile_scanner, flutter_sound, photo_manager and flutter_local_notifications have no ohos port at all. Voice notes, secure storage, camera/QR, photo picking and local notifications are unavailable on OHOS until ports catch up. pubspec.lock gains exactly the 8 new entries (repo convention: lock is committed with dependency changes).
PR iebb#272 review finding [P1]: apply could discard unrelated work. A second apply overwrote the only backup of the pre-overlay original with the already-overlaid file, so apply/apply/revert could no longer restore the starting content. pubspec.lock was not backed up even though pub get rewrites it, and apply edited tracked .gitignore at runtime. revert without a backup ran git checkout on pubspec.yaml, losing edits the user had made but not yet committed. The operation now refuses unsafe states instead of repairing them by resetting files. apply backs up both pubspec.yaml and pubspec.lock and refuses when an overlay is already active or when the backup would be overwritten; revert refuses when no apply is active and restores both locked files byte-for-byte. No tracked file besides the committed .gitignore entry is edited. Eight offline behavioral tests (apply, repeated apply, revert without a backup, dirty-file preservation, apply/resolve/revert lockfile rewriting, stale-backup refusal, no runtime edit of .gitignore) each include the exact mutation that fails it.
Address PR iebb#273 review findings: 1. Never publish a hap without TDLib. The pinned manifest now carries tdjson-ohos-arm64.zip (archive+member SHA-256/size plus a per-asset release_tag pointing at the immutable tdjson#4 secondary release), the installer treats it as required, and the install step re-hashes the installed ohos/entry/libs/arm64-v8a/libtdjson.so against that entry. The hap verification now requires libs/arm64-v8a/libtdjson.so inside the hap, compares its packaged bytes to the installed pinned bytes, checks the arm64 ELF identity, and asserts the td_json_client_* entry points the Dart loader resolves are present. pack.info/ets/modules.abc alone no longer pass the gate. 2. Only a specifically verified signing failure is tolerated. The build step captures the flutter log, and a nonzero exit only continues when the unsigned hap exists and the log names a signing cause; compiler, resource, dependency, or packaging failures fail the job. 3. scripts/enable-ohos-plugins.sh is invoked with its mandatory apply argument so the combined stack with iebb#272 no longer exits 2. 4. The engine embedding HAR patch is an explicitly reviewed capability fallback: it refuses to apply unless the engine still guards autofill below sdkApiVersion 26, so requestAutoFill/requestAutoSave stubs can never change behaviour on API 26+ runtimes without failing the build. Installer tests cover the required ohos asset, a manifest missing it, and the per-asset release tag override.
467e99d to
c8fc3de
Compare
Address PR iebb#273 review findings: 1. Never publish a hap without TDLib. The pinned manifest now carries tdjson-ohos-arm64.zip (archive+member SHA-256/size plus a per-asset release_tag pointing at the immutable tdjson#4 secondary release), the installer treats it as required, and the install step re-hashes the installed ohos/entry/libs/arm64-v8a/libtdjson.so against that entry. The hap verification now requires libs/arm64-v8a/libtdjson.so inside the hap, compares its packaged bytes to the installed pinned bytes, checks the arm64 ELF identity, and asserts the td_json_client_* entry points the Dart loader resolves are present. pack.info/ets/modules.abc alone no longer pass the gate. 2. Only a specifically verified signing failure is tolerated. The build step captures the flutter log, and a nonzero exit only continues when the unsigned hap exists and the log names a signing cause; compiler, resource, dependency, or packaging failures fail the job. 3. scripts/enable-ohos-plugins.sh is invoked with its mandatory apply argument so the combined stack with iebb#272 no longer exits 2. 4. The engine embedding HAR patch is an explicitly reviewed capability fallback: it refuses to apply unless the engine still guards autofill below sdkApiVersion 26, so requestAutoFill/requestAutoSave stubs can never change behaviour on API 26+ runtimes without failing the build. Installer tests cover the required ohos asset, a manifest missing it, and the per-asset release tag override.
Address PR iebb#273 review findings: 1. Never publish a hap without TDLib. The pinned manifest now carries tdjson-ohos-arm64.zip (archive+member SHA-256/size plus a per-asset release_tag pointing at the immutable tdjson#4 secondary release), the installer treats it as required, and the install step re-hashes the installed ohos/entry/libs/arm64-v8a/libtdjson.so against that entry. The hap verification now requires libs/arm64-v8a/libtdjson.so inside the hap, compares its packaged bytes to the installed pinned bytes, checks the arm64 ELF identity, and asserts the td_json_client_* entry points the Dart loader resolves are present. pack.info/ets/modules.abc alone no longer pass the gate. 2. Only a specifically verified signing failure is tolerated. The build step captures the flutter log, and a nonzero exit only continues when the unsigned hap exists and the log names a signing cause; compiler, resource, dependency, or packaging failures fail the job. 3. scripts/enable-ohos-plugins.sh is invoked with its mandatory apply argument so the combined stack with iebb#272 no longer exits 2. 4. The engine embedding HAR patch is an explicitly reviewed capability fallback: it refuses to apply unless the engine still guards autofill below sdkApiVersion 26, so requestAutoFill/requestAutoSave stubs can never change behaviour on API 26+ runtimes without failing the build. Installer tests cover the required ohos asset, a manifest missing it, and the per-asset release tag override.
Change
Adds HarmonyOS plugin integration on top of feat/ohos-platform (PR #270).
pubspec.yamlalready lists 8 Dart-3-compatible *_ohos packages that satisfy stock solving: shared_preferences_ohos, path_provider_ohos, url_launcher_ohos, open_filex_ohos, local_auth_ohos, webview_flutter_ohos (implements its host), plus sensors_plus_ohos and app_links_ohos (companion). Their native channels are exercised on stock CI; GeneratedPluginRegistrant is Android/Linux-identical.pubspec_overrides.ohos.yaml+scripts/enable-ohos-plugins.shcarry the remaining 4 packages whose every published version pinssdk <3.0.0(package_info_plus_ohos 0.0.7, permission_handler_ohos 0.0.8, file_picker_ohos 1.0.1, geolocator_ohos 0.0.3), layered asdependency_overridesbefore an OHOS build. The override ignores the overridden packages' sdk bounds, so stockflutter pub getkeeps solving. win32 is widened to>=4.1.3 <6.0.0because file_picker_ohos pinswin32 ^4while package_info_plus 9.x needswin32 ^5(win32 is unused on OHOS).No compatible pub.dev ports exist for record, flutter_secure_storage, camera, mobile_scanner, flutter_sound, photo_manager or flutter_local_notifications; they are documented in the overlay header and deliberately absent from the host/pubspec.
Telegram API Terms classification
allowTelegramTosViolations(I_DONT_FUCKING_CARE_ABOUT_TOS), disabled in official builds, and available only in an explicitly enabled self-build.Reason and affected Telegram behavior: dependency plumbing only — resolves which OHOS plugin ports the solver sees. No Telegram API calls, no new TDLib entries, no chat message or media handling changes. Unavailable-media features remain gated at the channel boundary in PR #270 (
unsupported/permissionDenied/biometrics unavailableinstead of a throw; verified on the stock SDK).Basic checks (every PR)
dart format --set-exit-if-changed—Formatted 1060 files (0 changed)on the touched surfaces; the overlay helper is shell+Python andbash -n-checked.flutter analyzeat this head — the only warnings are the 12 pre-existingunawaited_return_in_try_blockalready on master.flutter test --concurrency=4.Commands, results, platform/build/UI evidence, and reasons for inapplicable checks:
flutter pub get(8 new deps, solver unchanged for android/linux),flutter analyzeclean apart from the pre-existing 12, stock APK/Web/Windows--config-onlybuilds green.test/enable_ohos_plugins_script_test.dart— 8/8 green on this head, each with a mutation-quoted failure:apply preserves a dirty pubspec and revert restores its exact bytes— removing the per-effect byte check fails it.a second apply refuses instead of overwriting the only backup— dropping the already-active guard makesExpected: <false> / Actual: <true>.revert without a backup refuses and never checks out user files— replacing the refusal withgit checkout --fails it.apply/resolve/revert restores the original lock after a rewrite— dropping the lockfile backup fails all three lock-touching cases.translations/tools/check.pyuntouched, no visible literals introduced;assets/l10n/*.jsonunchanged.flutter build hapshape checks); a missing plugin channel surfaces as theunsupportedpath already tested in feat(platform): treat HarmonyOS as a mobile target #270. The unavailable flutter_secure_storage backend remains gated; app-lock/proxy credentials that use it on OHOS transparently degrade to in-memory-only (see PR feat(platform): treat HarmonyOS as a mobile target #270'slocal_app_lock_controller_testboundary cases) — no plaintext fallback is introduced.Section 1.4 exception checks
Not applicable.