Repository navigation
Conversation
|
@codex review |
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. 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". |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 657c9e04bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 6c5102ac40a5f5135affdf177e7eeb09922a8935..657c9e04bd77ca945591df81fd5bf2de35c9d75d (exact head 657c9e04bd77ca945591df81fd5bf2de35c9d75d)
Risk: critical — every iOS event signature and its private key now cross a new Dart → C FFI and app-linking boundary.
Blocking finding
[P2] The required regression bypasses the production iOS Runner linking seam.
scripts/test-mobile-native-signing.sh:9-18 builds a standalone macOS dylib and supplies it through BUZZ_SIGNER_LIBRARY; mobile/test/shared/crypto/event_signer_test.dart:9-12,30-34 then opens that dylib directly. Meanwhile, the test’s ordinary signEvent case runs on macOS, so mobile/lib/shared/crypto/event_signer.dart:19-27 takes the Dart fallback. No checked-in CI command launches mobile/tool/native_signing_probe.dart inside the built Runner.
The suite therefore never exercises the shipped iOS lookup through DynamicLibrary.process() (mobile/lib/shared/crypto/event_signer.dart:39,45-64) or proves that the retention flags in mobile/ios/Flutter/Debug.xcconfig:31-32 and mobile/ios/Flutter/Release.xcconfig:21-22 keep all three symbols reachable. Removing or breaking a retention/export flag leaves the standalone-dylib tests and app build green, but the first signing call in the app fails at runtime. This violates the repository’s production-seam/falsifiability requirement in AGENTS.md:234-238 and TESTING.md:25-31.
Author action: add a CI regression that executes signEvent through DynamicLibrary.process() in the actual linked iOS Runner for both Debug and Release configurations. It should validate a canonical signed event and the media worker-isolate path. Mutation-prove the regression by removing or breaking each required linker-retention symbol/flag and showing that the linked-Runner journey fails causally.
Verification owner: author for the regression and mutation evidence; exact-head CI for execution; reviewer for the fresh-head delta and final immutable-head check.
Integrated trace
No additional concrete defect was found in the reviewed head:
- Dart/native declarations match the C pointer and status contracts; malformed or invalid 32-byte scalars fail explicitly.
- Canonical hashing remains in
nostr.Event.unsigned; native code signs that 32-byte ID with a 64-byte BIP340 signature, returns the x-only public key, and self-verifies. - The randomized process context is initialized once and then read-only. Each signing call owns its keypair; native keypair/auxiliary randomness and the Dart scratch arena are wiped. No account or host secret cache was added.
- Swift coverage exercises concurrent independent identities and recovery after per-call failure. All production event creation found under
mobile/libroutes throughsignEvent; the remainingnostr.Event.fromis the intentional non-iOS fallback. - The bounded media path remains serialized, capped at 64, and off the UI isolate. The changed UI signing callers do not add a new burst loop; typing remains throttled. Android deliberately retains the Dart signer.
Validation
At exact head 657c9e04bd77ca945591df81fd5bf2de35c9d75d with a clean detached checkout:
- PASS:
just mobile-install && just mobile-check— formatting clean; Flutter analysis reported no issues. - PASS:
just file-size-check. - PASS:
git diff --check. - PASS: terminal GitHub rollup: 56 successful, 23 skipped by workflow/matrix routing, 0 failed, 0 pending. Relevant green jobs include all Flutter shards, Android, Mobile Checks, Mobile Swift Native, Mobile Swift Release, Security, and aggregates.
- BLOCKED by reviewer environment, not PR-caused: local
just mobile-teststopped before test execution because the host’s Xcode 27.1 license is not accepted. - Not independently witnessed: a real linked-Runner journey and physical-device Release latency. The latter is a release/performance confidence gap, not an additional author defect; current latency claims are simulator/host measurements and preserve the existing worker queue.
|
@codex review |
Dismissed because the PR head advanced before submission completed. This review applies only to 657c9e0; a fresh review of the new head is required.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! 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". |
Signed-off-by: Tom Brow <tomb@block.xyz>
Signed-off-by: Tom Brow <tomb@block.xyz>
Signed-off-by: Tom Brow <tomb@block.xyz>
Signed-off-by: Tom Brow <tomb@block.xyz>
3a26907 to
58f54a2
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 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". |
github-actions
Bot
added
the
codex-security-review-current
🔐 Codex Security Review
Review SummaryOverall Risk: NONE
FindingsNo concrete security, correctness, or reliability findings were identified. Notes
Generated by Codex Security Review | |
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket, Mantis and Groot reviews.
Reviewed head 58f54a20b9af13226368fa12e027e4261057c4e2 against base/merge-base 0b53fc699dd649ec483c07d628f9c41197607a3c.
One P1 release-configuration finding inline. No additional actionable correctness, secret-handling, concurrency or caller-flow defects found. Canonical event hashing, per-call native keypair cleanup and the Android fallback remain consistent with existing behavior. The linked Runner probe addresses the earlier standalone-dylib test gap; archive postprocessing is a separate boundary.
Optional wording note: the description says “all mobile event creation,” but mobile/lib/shared/hosted/web_connect_binding.dart:54 still uses the existing Dart signer. Narrow that wording if desired; this unchanged one-shot caller is not a functional regression.
Validation: source inspection only; no PR code, tests, builds, archives or apps executed. Read-only CI snapshot: 78 successful and 29 skipped checks, including successful Mobile Swift Native and Release jobs. Those results do not verify a postprocessed archive/IPA, installed-device symbol lookup, physical-device performance, native error/retry focus or accessibility. Current signing calls and the probe remain synchronous on the UI isolate; worker offloading is separate. Public description inspected, no attached screenshots or actionable disclosure found; a historical review’s third-party severity badge was not rendered. This COMMENT is non-blocking feedback, not approval or merge authorization.
| #include? "AppOverrides.xcconfig" | ||
|
|
||
| // Keep the Dart FFI entry points visible in the app executable. | ||
| OTHER_LDFLAGS = $(inherited) -Wl,-u,_buzz_signer_create -Wl,-u,_buzz_signer_sign -Wl,-u,_buzz_signer_destroy |
There was a problem hiding this comment.
🤖 [P1] Preserve the FFI exports through archive stripping
These -u flags force the three functions into the linked executable, but do not preserve them through the subsequent archive/install strip step. Runner’s checked-in scheme archives Release, and its tracked configuration leaves STRIP_STYLE at Xcode’s default All Symbols. For a normal archive without an external override, the runtime-only buzz_signer_* exports can therefore be stripped even though the ordinary device build passes nm. The first DynamicLibrary.process() lookup in event_signer.dart then fails, breaking the migrated iOS authentication, media and send paths.
Set STRIP_STYLE = non-global for Runner Release (or explicitly preserve these exported symbols), as prescribed by Flutter’s iOS FFI stripping guidance, and check all three exports in the postprocessed archive/IPA executable. The current CI check at _ci-clients.yml:256–262 inspects flutter build ios --release --no-codesign, so it misses archive stripping. This is a source/configuration finding; I did not build an archive or observe an installed-app failure, and external release overrides were not inspected.
There was a problem hiding this comment.
🤖 Addressed in 7aa63ef. Runner Release now uses non-global stripping, and CI inspects the postprocessed archive executable. A local unsigned device Release archive succeeded; its log confirms strip -x and nm -gU confirms all three buzz_signer exports remain. This validates archive retention without claiming installed physical-device execution.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 0b53fc699dd649ec483c07d628f9c41197607a3c..58f54a20b9af13226368fa12e027e4261057c4e2 (exact head 58f54a20b9af13226368fa12e027e4261057c4e2)
Risk: critical — mobile identity secrets and every migrated iOS event signature cross a new Dart → C FFI and app-linking boundary.
Blocking finding
[P2] One live iOS event-creation path still bypasses the shared/native signer.
mobile/lib/shared/hosted/web_connect_binding.dart:54 still constructs its kind-24243 identity-binding event with nostr.Event.from(...). This is production behavior reached by hosted account binding and browser return at mobile/lib/shared/hosted/hosted_flow.dart:494 and :528. On iOS it therefore continues through the Dart secp256k1 implementation instead of this PR's signEvent / DynamicLibrary.process() route.
A source inventory at the reviewed head found only two direct nostr.Event.from calls under mobile/lib: the intentional non-iOS fallback at mobile/lib/shared/crypto/event_signer.dart:22, and this hosted binding call. The latter predates the PR, but omitting it leaves the PR's stated migration—“Routes all mobile event creation” and the new mobile/README.md:84 rule to use the shared signer—materially incomplete in a real identity flow.
Author action: route the hosted binding event through signEvent(..., pubkey: keys.public) and add regression coverage that prevents a production direct-constructor bypass from returning. Preserve the existing account/pubkey checks and hosted-binding behavior.
Verification owner: author for the patch and regression; exact-head CI for the linked iOS Runner/native-symbol gates; reviewer for the fresh caller inventory and hosted-flow trace.
Integrated trace
No second author-actionable defect was found:
- Dart/native declarations match the C pointer/status contracts. Canonical event hashing remains in
nostr.Event.unsigned; native code derives the x-only key, emits a 64-byte BIP340 signature, and self-verifies it. - The randomized process context is initialized once and then used read-only. Keypair handles and per-sign auxiliary randomness are per-call and wiped; no account/host secret cache was added. Concurrent independent keypairs have Swift coverage.
- The actual Debug Runner executes the signer through
DynamicLibrary.process(). The current probe validates ordinary events, synchronous media-auth signatures and foreign-host isolation, and NIP-98 query signatures. The device Release job checks all three exports in the actualBuzz.app/Buzzexecutable. - Current-tree scope matters:
mobile/tool/native_signing_probe.dart:63-65invokes synchronousMediaGetAuthService.headersFor, and this tree has noheadersForAsyncworker-isolate route. Earlier-head claims that the probe covers a media worker isolate do not apply. This is not an additional defect because the PR now explicitly preserves synchronous scheduling and treats offloading as separate work. - The revised performance claim is properly limited to complete synchronous Debug Simulator event construction, not rendered-frame or physical-device Release performance. No new media/navigation state transition was introduced.
Validation
At clean detached HEAD 58f54a20b9af13226368fa12e027e4261057c4e2:
- PASS:
git diff --check. - PASS:
bash -n scripts/test-mobile-linked-signing.sh. - SOURCE TRACE:
rg -n 'nostr\.Event\.from\(' mobile/lib -g '*.dart'found the intentional fallback and the hosted-binding bypass above. - PASS: exact-head GitHub rollup is terminal: 78 success, 29 workflow/matrix skips, 0 failed or pending.
- PASS: Mobile Swift Native ran the linked Debug Runner and logged
SIGNING_PROBE COMPLETE, valid media/host-isolation results, and valid query signatures. - PASS: Mobile Swift Release built
build/ios/iphoneos/Buzz.appand completed the exact-export checks forbuzz_signer_create,buzz_signer_sign, andbuzz_signer_destroy.
Manual/native evidence: exact-head CI executed the actual linked Debug Simulator Runner and inspected the actual device Release executable. I did not independently run a physical-device Release build or rendered-frame journey.
Residual risk: physical-device Release latency and user-visible frame behavior remain unwitnessed. Those are release/performance confidence gaps, not additional author rework; the checked-in claim is correctly narrower.
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: 0b53fc699dd649ec483c07d628f9c41197607a3c..58f54a20b9af13226368fa12e027e4261057c4e2 (exact head 58f54a20b9af13226368fa12e027e4261057c4e2)
Risk: high — this migration changes the private-key/signature boundary for iOS event creation, but one live identity-binding path still bypasses that boundary.
Blocking finding
[P1] The hosted web-connect identity-binding flow still signs in Dart instead of using the native iOS signer.
mobile/lib/shared/hosted/web_connect_binding.dart:54-73 creates the kind-24243 binding event with nostr.Event.from(...). Both live hosted-flow branches call this service: automatic identity binding at mobile/lib/shared/hosted/hosted_flow.dart:494-501, and an incoming NostrBindDeepLink immediately before browser return at mobile/lib/shared/hosted/hosted_flow.dart:528-539.
On iOS, these journeys therefore bypass signEvent and retain synchronous Dart secp256k1 signing on the UI isolate. That contradicts the PR’s stated contract to route all mobile/iOS event creation through the shared native signer, leaving a real account-binding event outside the migration. The existing mobile/test/shared/hosted/web_connect_binding_test.dart:89-123 validates the event shape, but passes while the direct-constructor bypass remains; it does not bind the hosted route to the production signer.
Author action: replace the direct constructor with signEvent(kind: 24243, ..., secretKey: keys.secret, pubkey: keys.public, verify: false). Add hosted-binding/return coverage bound to that production seam and a caller-inventory guard that rejects direct production Event.from signing outside the intentional non-iOS fallback. Extend the linked iOS Runner probe to exercise the hosted kind-24243 route causally, rather than only a synthetic event of that kind.
Verification owner: author for the patch and regressions; reviewer for fresh-head production-caller inventory and mutation bite; linked iOS CI for process-native execution of kind 24243.
Integrated review
No separate author-actionable defect was found in the reviewed ABI signatures, canonical hash/native-pubkey/BIP340 path, per-call key/handle cleanup, randomized shared read-only context, identity isolation, explicit missing-symbol failure, Debug linked-Runner lookup, Release executable symbol retention, or revised benchmark wording.
The rebased linked probe at mobile/tool/native_signing_probe.dart:63-65 calls synchronous MediaGetAuthService.headersFor; exact-tree search found no headersForAsync under mobile/lib, mobile/tool, or mobile/test. It therefore proves synchronous media-auth signature and host-isolation behavior, not worker-isolate behavior. That corrects earlier-head evidence; it is not a second defect because this tree exposes no worker-isolate media-signing route.
Validation
At clean detached HEAD 58f54a20b9af13226368fa12e027e4261057c4e2:
- PASS:
git diff --check 0b53fc699dd649ec483c07d628f9c41197607a3c...HEAD. - PASS: exact-head GitHub rollup is terminal: 78 passed, 29 skipped by workflow/matrix routing, 0 failed or pending. Relevant green jobs include all Flutter shards, Android, Mobile Checks, Mobile Swift Native linked-Runner execution, Mobile Swift Release symbol inspection, Security, and aggregates.
- PASS: production-caller inventory found the intentional non-iOS fallback at
mobile/lib/shared/crypto/event_signer.dart:22and the omitted hosted signer atmobile/lib/shared/hosted/web_connect_binding.dart:54. - Not independently witnessed: physical-device Release execution and rendered-frame latency. The PR correctly limits its benchmark to synchronous Debug Simulator event-construction timing; these remain release/performance confidence gaps, not additional author rework.
Signed-off-by: Tom Brow <tomb@block.xyz>
Signed-off-by: Tom Brow <tomb@block.xyz>
github-actions
Bot
removed
the
codex-security-review-current
|
🤖 Addressed the hosted-binding bypass in 4153655. The production service now uses signEvent with its cached public key. Focused tests and analysis passed; the actual iOS Runner executed the service and verified the kind-24243 signature before browser return. Restoring the direct constructor caused the new production inventory guard to fail on that exact file. Fresh CI and review are pending. |
blockers resolved
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! 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". |
github-actions
Bot
added
the
codex-security-review-current
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 0b53fc699dd649ec483c07d628f9c41197607a3c..7aa63ef33493e7db82a5068556e3d20929610979 (exact head 7aa63ef33493e7db82a5068556e3d20929610979)
The prior hosted web-connect bypass and Release archive-stripping concerns are resolved. No author-actionable defect remains.
Integrated review
- Dart’s three FFI declarations match the C header. Dart constructs the canonical unsigned event hash and passes exactly 32 ID bytes; native code produces and self-verifies the 64-byte BIP340 signature against the x-only public key. Invalid scalar and randomness failures are explicit, with no fallback.
pthread_oncepublishes one randomized read-only context. Each call owns its keypair and auxiliary randomness, wipes native key material plus the Dart arena, and cleans up on all exits. Swift coverage exercises concurrent independent keys and invalid-key recovery; no account or host secret cache was introduced.- Production inventory under
mobile/libfinds 14signEventcall sites and only one directEvent.from: the intentional non-iOS fallback. The formerly omitted hosted kind-24243 path now callssignEventwith the resolved identity pubkey while retaining cancellation, identity-match, server-verification, and browser-return ordering. - The inventory regression is causal: mutating hosted binding back to
Event.frommakes it fail with the exact production file, then pass after restoration. - Debug CI builds and launches the actual iOS Runner, exercising
DynamicLibrary.process(). The exact-head probe verifies message, media, NIP-98, foreign-host isolation, hosted kind-24243 binding, and verified browser return before completion. - Release CI archives the device app and checks all three required exports after archive stripping. The exact-head log reaches
ARCHIVE SUCCEEDEDand all post-strip symbol checks pass. - Android intentionally retains the existing Dart signer. iOS missing-symbol and native failures remain explicit. The documented performance result is correctly scoped to synchronous Debug Simulator event construction rather than rendered frames or physical-device Release behavior.
Validation
At exact head 7aa63ef33493e7db82a5068556e3d20929610979:
- PASS: focused shared-signer and hosted-binding tests: 7 passed, 4 expected native-library skips locally; native execution is covered by the linked macOS CI lanes.
- PASS:
git diff --check; clean detached worktree during review. - PASS: live GitHub rollup was terminal immediately before submission: 94 completed checks, no failed, cancelled, pending, or action-required checks. CI run
39473757621is successful at this head, including mobile shards, Android, linked Mobile Swift Native, archived Mobile Swift Release, Security, and aggregate jobs; other entries are workflow/matrix skips. - PASS: authenticated reviewer is
jedwards27, distinct from PR authorbrow; PR remained open, non-draft, mergeable, and pinned to the reviewed head immediately before submission.
Confidence gaps
No signed/TestFlight physical-device Release journey was observed. Current evidence proves linked Debug Runner behavior and post-strip Release export retention, not physical-device Release latency or distribution behavior. This is release confidence, not author remediation.
Author action: none.
Verification owner: Release/QA for an optional signed physical-device/TestFlight smoke across hosted binding and verified browser return, identity switching, message/auth/media signing, and perceived responsiveness. Any new head invalidates this approval and requires delta review.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
🤖 Star Lord’s automated source review via Wes’s account, integrating independent Rocket, Mantis and Groot reviews.
Reviewed head 7aa63ef33493e7db82a5068556e3d20929610979 against base/merge-base 0b53fc699dd649ec483c07d628f9c41197607a3c, including the five-file follow-up from previously reviewed head 58f54a20b9af13226368fa12e027e4261057c4e2.
No actionable findings. The earlier archive-stripping finding is addressed in source: Release now sets STRIP_STYLE = non-global, and CI checks the three FFI exports in the postprocessed archive executable. Hosted identity binding now uses the shared signer while retaining identity checks, server verification before browser return, and the existing cancellation/retry flow. The linked Runner probe includes that binding path.
Validation: source inspection only; no PR code, tests, builds, archives or apps executed by this review. A read-only current-head CI snapshot showed 62 successful and 32 skipped checks. We did not independently verify installed-device symbol lookup, physical-device performance, native error/retry focus, accessibility, or external release overrides. Signing remains synchronous; worker offloading is separate. Public PR description inspected with no attached screenshots or actionable disclosure found; a historical review’s third-party severity badge was not rendered. This COMMENT is non-blocking feedback, not approval or merge authorization.
Routes all mobile event creation through the existing CSecp256k1 library on iOS, replacing Dart signing. Android retains Dart signing and its cached-public-key optimization. Native keypairs are short-lived and wiped after signing; the randomized shared context retains no account secrets.
This PR targets main independently. It changes signing only, preserving existing call scheduling. Media/query worker offloading remains separate.
Validation
Same-process Debug iPhone 17 Pro Simulator comparison: identical kind-24242 inputs, cached public key supplied to both paths, ten warmup calls each, seven samples per burst, alternating order. Both paths run synchronously on the Dart UI isolate. These are complete event-construction elapsed times, not UIKit main-thread CPU time or rendered-frame measurements.
Native signing reduces the 64-event burst by 76% (4.2x faster). This is not a physical-device Release benchmark. The reproducible comparison is in
mobile/tool/compare_event_signers.dart.DynamicLibrary.process(); device Release CI checks all three exported symbols in the postprocessed archive. Earlier missing-export mutations each produced the expected lookup failure, then passed when restored. Current-head Codex review found no major issues. All current-head GitHub checks passed, including the linked Simulator probe and postprocessed Release archive export checks. Current-head Codex review found no major issues.just cistopped at the unchanged desktopreaps_descendant_on_timeouttest: its PID file was empty under a 300 ms deadline (3,421 passed, 1 failed, 19 ignored). The isolated rerun passed. No desktop code changed; this is not a full-suite pass.