Repository navigation
Conversation
Signed-off-by: Tom Brow <tomb@block.xyz>
Signed-off-by: Tom Brow <tomb@block.xyz>
|
@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. |
![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: 06123e3866
ℹ️ 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".
Signed-off-by: Tom Brow <tomb@block.xyz>
Signed-off-by: Tom Brow <tomb@block.xyz>
brow
changed the title
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ccfaf39bc
ℹ️ 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".
Signed-off-by: Tom Brow <tomb@block.xyz>
* origin/main: fix(mobile): bump Flutter 3.47, resolving memory leak (#8419) fix(mobile): bound search result rendering work (#8418) Polish mobile message layout and navigation controls (#8427) feat(relay): start a channel with its members in one create (#8421) fix(desktop): bump zip to 9 for RUSTSEC-2026-0367/0368/0369 (#8412) fix(deps): bump negentropy to 0.5.2 for RUSTSEC-2026-0365 (#8409) chore(deny): exclude buzz-mesh-smoke from cargo-deny graph (#8413) fix(mobile): polish Inbox controls and system notices (#8423) fix(mobile perf): avoid eager emoji palette scans for prose (#8416) Signed-off-by: Tom Brow <tomb@block.xyz>
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! 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>
|
@buzz-security-review 3488514 |
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 | |
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: 819dd7924f1740b72b37a736393ae20509864e9e..3488514915b88e40916ed2e9afd1892223acd274 (live exact head rechecked before submission)
Risk: high — this changes authentication and async admission for the shared mobile media path used while rendering channel/thread history and is proposed for the conference stable release.
Blocking finding
- [P2] Temporary signing overload permanently strands media that remains mounted. The new 64-request admission cap rejects request 65 immediately. Cache eviction permits a later new resolution, but an already-mounted Flutter
Imagerebuilt with an equal(url, scale, auth)provider does not resolve a new stream. A production-seam widget probe at this exact head reproduced: queue-full error → fallback (auth=1,fetch=0) → capacity drain and rebuild of the same mounted image → still fallback (auth=1,fetch=0) → only unmount/remount retries and loads. The same shared provider is used by message images, avatars, reactions, and custom emoji. Posterless video similarly memoizes the failed loader future asnull, so it remains on fallback while mounted after capacity recovers. This turns the PR’s explicitly transient overload and “later image resolution can retry” contract into a sticky user-visible failure.
Author action: retain bounded admission, but make capacity rejection (and other deliberately recoverable worker failures) schedule at most one bounded retry for still-active consumers after capacity returns. Use a typed transient failure so queue pressure is not confused with network cooldown or permanent signing/configuration errors. Add production-seam regressions proving reject → drain/recover → the same mounted shared image consumer succeeds and that posterless video recovers without navigation/remount/provider rekey, while outstanding signing/retry work never exceeds the bound. One shared MediaImage regression can cover provider behavior inherited by avatars/reactions/custom emoji; those callers do not each need bespoke queue machinery.
Verification owner: author for fix and deterministic regressions; reviewer for exact-head rerun.
Contracts traced / validation
- Traced proof identity, relay-server tag, timestamp sampling, FIFO serialization, 64-work accounting, failure release, cross-origin/path fencing, provider cache identity, posterless-video error handling, and formatter-only scope.
- No credential/proof/server-scope break found. Proof time is sampled when an admitted job starts; failures release capacity without poisoning later jobs; auth remains absent for other origins and invalid paths.
- PASS at exact head: focused changed tests (18/18),
just mobile-check(757 files, analyzer clean), andgit diff --check; independent final trees were clean. - PASS: GitHub Actions run
39162291669is pinned to exact head and has mobile checks, all three mobile test shards, Android debug APK, native iOS Simulator build/tests, and unsigned iOS release build green. - Reviewer-local focused Flutter execution was attempted at the exact clean head but blocked before compilation by the host’s unaccepted Xcode license/native
objective_chook (xcrunfailure path). This tooling gap does not cause the blocking verdict; source tracing plus the production-seam widget reproduction establish it.
Residual risk
No independent native device/simulator UX run or clean performance cohort was completed. The cited 293 ms Debug/JIT trace supports removing observed signing from the UI isolate; it does not establish release-device speedup. Isolate restart behavior also lacks a real compute spawn-failure probe. Those are reviewer/tooling confidence gaps, not additional author defects.
| if (nsec == null || nsec.isEmpty || !isRelayMediaUrl(url)) { | ||
| return Future.value(const {}); | ||
| } | ||
| if (_pendingSignatures >= _maxPendingSignatures) { |
There was a problem hiding this comment.
[P2] Queue rejection has no recovery transition for already-mounted consumers. Request 65 fails here immediately, but eviction in MediaImageProvider only permits a future resolution; Flutter keeps the existing failed stream when a rebuild supplies an equal provider. At this exact head, a production-seam widget probe stayed on fallback after capacity drained and retried only after unmount/remount. Posterless video likewise memoizes the failed future as null. Keep the cap, but arrange one bounded capacity-available retry for active consumers and add a reject → drain → same mounted consumer succeeds regression (plus posterless-video recovery), while proving outstanding work remains bounded.
There was a problem hiding this comment.
🤖 Addressed in 93c1359. Typed queue saturation keeps each mounted image or posterless-video load pending until the existing queue snapshot drains, then permits one retry through the same 64-job admission limit. A second rejection propagates. Production-consumer regressions fail without recovery and pass without remounting; the burst regression verifies bounded admission and terminal rejection after one retry. Full mobile validation, native build, and Simulator navigation pass.
Signed-off-by: Tom Brow <tomb@block.xyz>
github-actions
Bot
removed
the
codex-security-review-current
|
@codex review |
|
@buzz-security-review 93c1359 |
github-actions
Bot
added
the
codex-security-review-current
|
Codex Review: Didn't find any major issues. What shall we delve into next? 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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93c1359b12
ℹ️ 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".
Signed-off-by: Tom Brow <tomb@block.xyz>
github-actions
Bot
removed
the
codex-security-review-current
|
@codex review |
|
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". |
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: REQUEST CHANGES
Reviewed: 819dd7924f1740b72b37a736393ae20509864e9e..aee8135424f8afae58568920ecc20141cbfea6b3 (live exact head rechecked immediately before submission)
Risk: high — this changes authentication and async admission for shared mobile media paths proposed for the conference stable release.
Blocking finding
- [P2] A transient signing-worker failure still permanently strands mounted media.
headersForAsyncWithRetryretries onlyMediaAuthQueueFull(mobile/lib/shared/relay/media_auth.dart:114-127). If the newly introducedcomputeworker has a one-time startup/runtime failure (media_auth.dart:90-103), that failure propagates immediately. Shared images fail before fetching (mobile/lib/shared/relay/media_image.dart:102-104), and an equal provider does not cause the existing mountedImagestream to resolve again. Posterless video catches the failure asnull(mobile/lib/features/channels/message_content/video_preview.dart:33-42) and memoizes that completed future for the mounted loader/poster/URL tuple (:61-67). The result is sticky fallback until remount/rekey for avatars, message images, reactions, custom emoji, and posterless video. A production-seam mounted-image probe at this exact head injected one transient first-worker failure and observed one auth attempt rather than the required second attempt (Expected: 2,Actual: 1). Queue saturation now recovers, but the same user-visible failure class remains for a transient isolate failure introduced by moving signing tocompute.
Author action: distinguish deliberately recoverable worker/startup failures from permanent signing/configuration failures and give the transient class at most one retry through the same bounded retry-admission path. Add production-seam mounted-image and posterless-video regressions proving transient worker failure → bounded retry → success without remount/provider rekey. Keep permanent failures terminal, and prove a second rejection/failure cannot loop.
Verification owner: author for the bounded implementation and regressions; reviewer for the next exact-head rerun.
What the follow-up fixed
- Queue-full recovery is closed for the same mounted shared-image and posterless-video consumers: they remain pending through drain and retry once without remounting.
- Signing jobs and retry operations are independently capped at 64. Excess waiters fail before incrementing; second admission rejection escapes the single catch.
- Signing and retry permits release in
finallyacross success and failure. The queue tail follows a separate release future, so a failed worker cannot poison successors or create a lost wakeup. - Retry admission mints a fresh, server-scoped proof after drain; no cross-origin/path credential leak was found.
- Disposal can leave bounded stale work (at most the retry cap), but it settles and releases its permit. This is residual efficiency/fairness risk, not a second blocker.
Exact-head validation
- PASS — focused Flutter coverage across
media_queue_recovery_test.dart,message_video_auth_test.dart, andmedia_image_test.dart: 23/23. - PASS —
just mobile-check: 758 files formatted, 0 changed; analyzer clean. - PASS —
git diff --check 3488514915b88e40916ed2e9afd1892223acd274..aee8135424f8afae58568920ecc20141cbfea6b3; exact-head worktrees clean. - PASS — GitHub Actions run
39168175881: all three mobile test shards, mobile checks, Android build, native iOS Simulator lane, and iOS release lane green; required Mobile and DCO checks green. - FAIL — temporary production-seam mounted-image probe for one transient worker failure: expected two auth attempts, observed one; the shim was removed after reproduction.
- LOCAL BLOCKED — an independent unshimmed Flutter/native replay could not compile because this host has not accepted the Xcode license (
xcrunexit 69). Hosted exact-head execution supplies build/test evidence; this tooling gap is not the blocking reason.
Residual risk
No independent native device/simulator visual recovery run or clean release-performance cohort was completed. The Debug/JIT trace supports moving the captured signing work off the UI isolate, but it does not establish a release-device speedup. Any head change expires this verdict.
Signed-off-by: Tom Brow <tomb@block.xyz>
github-actions
Bot
removed
the
codex-security-review-current
|
@codex review |
|
@buzz-security-review 6c5102a |
|
🤖 Addressed the transient worker-startup finding in 6c5102a. IsolateSpawnException now gets one retry through the same 64-consumer retry admission and 64-job signing queue. Signing/configuration errors and RemoteError remain terminal because they do not establish a transient startup failure. A second failure never loops. Both mounted-image and production posterless-video regressions fail before this fix and recover without remounting afterward. All 28 focused tests, the full 3,539-test mobile suite, analysis, native build, and Simulator channel/thread navigation pass. The PR is draft while current-head CI and renewed review run. |
github-actions
Bot
added
the
codex-security-review-current
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: 819dd7924f1740b72b37a736393ae20509864e9e..6c5102ac40a5f5135affdf177e7eeb09922a8935 (exact live head 6c5102ac40a5f5135affdf177e7eeb09922a8935)
Risk: high — shared mobile media authentication concurrency, transient failure recovery, and visible image/video behavior.
Findings: the prior mounted-consumer defects are resolved; no unresolved author-actionable defect remains.
Behavior/contracts traced
mobile/lib/shared/relay/media_auth.dart:117-140retries only queue saturation andIsolateSpawnException, once, through the same normal signing admission path. Permanent signing/configuration errors and ambiguousRemoteErrorremain terminal.- Signing jobs and retry operations remain independently bounded at 64. Success, rejection, and worker failure release permits in
finally; failure cannot poison the queue tail or recurse through the wrapper. - The retried job samples proof time when normal signing begins and preserves the original server/account scope.
- Shared images and posterless video use the bounded retry API. Production-seam widget tests keep the same mounted consumers through transient failure → recovery → authenticated load, without navigation, remount, or provider rekey. Second failures fall back without an unauthenticated request.
- Disposal/navigation remains fenced; late video-frame results are disposed.
Author action: none.
Verification owner: reviewer/release QA for optional physical-device visual recovery observation.
Validation at matching head
- PASS: clean exact-head checkout and
git diff --check. - PASS from the product/adversarial lane: 28/28 focused tests across
media_queue_recovery_test.dart,message_video_auth_test.dart, andmedia_image_test.dart. - PASS: exact-head CI has no failed or pending checks. All three Mobile test shards, format/analyze, Android debug, iOS simulator/native tests, and unsigned iOS release build are green.
- This review host could not independently compile the Flutter tests because the Objective-C native-asset hook cannot discover the local Apple SDK. That is reviewer/tooling debt, not author work; the independent focused run and exact-head CI own the executable evidence.
Manual/native evidence: no physical-device visual replay was completed.
Residual risk: up to 64 disposed/offscreen consumers may finish one bounded retry before settling, and native UX/performance was not independently observed. Both are bounded confidence gaps; neither requires author action.
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 (systems/performance/lifecycle), Mantis (correctness/security/concurrency), and Groot (product/accessibility) reviews.
Reviewed head 6c5102ac40a5f5135affdf177e7eeb09922a8935 against base/merge-base 819dd7924f1740b72b37a736393ae20509864e9e.
No actionable introduced defects found; no changes requested. The change supports VISION_MOBILE’s responsive-UI and bounded-background-work goals. Traced serial signing admission, both 64-operation limits, failure release, one-shot retry, server/identity scoping, image loading and posterless-video success/error/disposal paths. The added recovery tests bind the production retry wrapper and consumers; the carousel-test change is formatting only. Source assessment: minimalism, elegance and correctness each 9/10.
Residual limitations: a second rejection or exhausted retry capacity remains terminal for the mounted consumer, as documented. Already-admitted work is not cancelled on disposal or auth-scope replacement. No new disclosure concern found in the public PR description or changed source; the description contains no attached images.
Validation: source inspection only. We did not execute tests, builds, the app, or PR code; we did not independently verify CI, native isolate-startup behavior, device accessibility, or release-performance claims. This COMMENT review is not approval or merge authorization.
This branch was successfully deployed
3 participants
Opening message history can synchronously sign image and video-preview authentication proofs on the UI isolate. Move that signing to a background isolate while preserving fresh, server-scoped proofs. Serialize signing and cap outstanding work at 64 requests so a media burst cannot create an unbounded backlog; mounted image and posterless-video loads retry queue saturation or isolate-startup failure once after the existing queue drains, through the same admission limit. Signing errors and ambiguous worker-runtime failures remain terminal. At most 64 consumers may wait or retry in addition to the 64 signing jobs. Excess waiters and second rejections fail explicitly.
A Debug/JIT iOS Simulator capture attributed 99 of 155 samples in one 293 ms navigation frame to media-auth signing. A follow-up capture of the combined candidate had no signing samples on the UI isolate. Other long frames remained, so this is evidence of removing the identified blocking work, not a release-device speedup claim.
Stable release recommendation
Include in
mobile-release/conference-2026-10after validation and approval. This addresses the strongest captured blocking cost with a narrow change to media loading. Under an extreme burst, the bounded queue can produce transient media failures.Validation
All 28 focused media tests pass, including fresh proofs, credential scoping, queue overflow and recovery, and waiting for asynchronous auth before fetching. New production-loader widget regressions reproduce unhandled posterless-video errors for queue rejection and isolate startup failure before the fix, and verify the existing preview fallback after it. Mounted image and posterless-video regressions fail without recovery and pass after queue saturation or a one-time isolate-startup failure without remounting. Permanent errors and a second startup failure remain terminal. Burst tests verify signing and retry-waiter limits, permit release on failure, and terminal rejection after one retry; removing the waiter cap fails the regression. The media-only build also passes the full mobile suite, analysis, and native iOS Simulator build. Channel history with loaded images, avatars, and reaction images, plus thread navigation, were exercised on iOS 26.5 Simulator.
The full local repository gate previously encountered an unrelated, unchanged desktop subprocess timing test failure; that test passed in isolation. CI also exposed a pre-existing carousel-test formatting failure after the main merge; this PR includes the formatter-only correction. Full mobile validation, native Simulator navigation, and current-head PR CI pass for the worker-startup recovery follow-up. Codex re-review found no major issues.