Conversation
Independent discovery readiness evidence — 2026-09-07Verified current stacked range:
|
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — request changes on exact head 5dcc5deaf80626aea9f2ff62141792eaa91ec3bb (base 882f69cbaf0f851717f071fba366b3b3a4a877d8).
Required changes
-
Handle kind-5 coordinate deletions in the live directory refresh path.
mobile/lib/shared/mentions/agent_discovery.dart:79-87subscribes to kinds0, 10100, 30177, 39002, but managed-agent removal is a NIP-09 kind-5 tombstone witha=30177:<owner>:<d_tag>(desktop/src-tauri/src/managed_agents/agent_events.rs:147-158). An already-open mobile process therefore misses an archive/delete performed elsewhere and continues presenting the revoked agent as “managed by you” and actionable until another watched event, reconnect, config change, or restart.A production-provider probe reproduced this: starting with a candidate, removing its policy, and delivering the real tombstone only through the shipped subscription filter left the candidate present (
Expected: empty,Actual: [AgentDirectoryEntry]). Adding kind 5 to that filter made the probe pass. The existing discovery test calls the callback directly with a 30177 event, so it bypasses the production filter and does not cover deletion.Author action: subscribe to the actual removal signal (scoped as tightly as the protocol allows), invalidate/refetch, and add a production-seam regression proving a kind-5 coordinate tombstone removes the candidate without reconnect/relaunch. Cover the already-open picker and reconnect/replay behavior if those paths differ.
-
Recover when refresh-subscription establishment fails. The catch at
agent_discovery.dart:94-96only logs. IfsubscribeWithStatusfails while the session remainsconnected, nothing retries or invalidates the provider; the directory can indefinitely omit newly valid agents or retain revoked ones while the UI appears authoritative.Author action: add generation-fenced recovery (for example, bounded backoff or a session-owned retry/rebuild trigger) and a deterministic fail-once → recover → later policy-event refresh regression. Avoid an unbounded refresh loop.
Evidence and residual validation
- The identity/tenant boundary otherwise looks sound: the active-community identity supplies the viewer, seed discovery scopes 30177 authors to that viewer, and exposure re-resolves signed profile plus exact current owner policy.
- Exact-head Mobile CI is green. Systems-focused tests passed 37/37; an independent
just mobile-checkpassed, andjust mobile-testreached 2099/all passed, though its post-run SHA receipt was lost with shared worktree metadata and is not treated as exact-head attribution. - Author-supplied widget evidence clearly distinguishes “agent,” “managed by you,” and “not in channel” at the shown light-theme viewport.
- Native iOS was not run. After the defects are fixed, the native/product verification owner should exercise no-runtime discovery, open-picker add/delete, selection/send, reconnect/relaunch, dark theme, narrow layout, large text, and VoiceOver. This is a confidence gap, not separate author rework.
Authenticated reviewer jedwards27 is not PR author loganj. Any new head requires delta review.
Signed-off-by: Logan Johnson <loganj@squareup.com>
Signed-off-by: Logan Johnson <loganj@squareup.com>
5dcc5de to
1eabb04
Compare
|
Addressed both lifecycle callouts in 1eabb04 (base df61d54). Subscription includes kind 5 and only accepts verified 30177:: coordinate deletion triggers. Establishment retries at most three times per generation, cancels retired timers, and exposes exhausted failure to directory readers rather than catch-log-only success. Filter-faithful tests cover fail-once recovery while connected, live policy deny, unrelated deletion ignored, real coordinate deletion/replay, exhaustion, and disposal. Full mobile suite at exact published HEAD: 2,102 passed; flutter analyze clean. No native-device or repository-wide just ci claim. Range 442 additions + 9 deletions. This does not claim the separate profile-cache/provenance or loading/error picker callouts fixed. |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent — REQUEST CHANGES
Reviewed: df61d54cb2e4f534a19ce8782a7fd6ccd165b467..1eabb049c6f36f7a5296dc82f0895ce4326bb6dd.
The changed head fixes the two prior findings: kind-5 coordinate deletions are subscribed/validated, and establishment failures receive bounded generation-owned retry with error propagation. The product lane’s production-provider mutations and exact-head Mobile 2,102-test run passed.
[P1] Terminal subscription closure permanently drops directory freshness
_AgentDirectoryUpdates.onClosed only calls changed() (agent_discovery.dart:96). The real relay session then removes a terminally closed subscription (relay_session.dart:741-747). The directory owner neither records terminal loss nor re-establishes the subscription, so later profile/policy/kind-5 events are missed until unrelated rebuild/reconnect/relaunch while the picker can remain authoritative.
Author action: on terminal CLOSED, expose failure and/or boundedly rebuild under the current generation. Add a production-seam establish → terminal close → recover/visible error → later event regression; fence late callbacks and close retired subscriptions.
Test reliability defect
A clean exact-head full just mobile-test failed the new fail-once regression because its fixed 180 ms transient-error assertion raced successful recovery (2,101 passed / 1 failed); the file alone then passed 7/7. This PR introduces that test and timing policy.
Author action: replace wall-clock transient-state timing with deterministic completers/fake time; assert eventual recovery and bounded attempts.
Verification owner: author for both fixes; reviewer for exact-head lifecycle and full-suite rerun.
Validation: separate exact-head full Mobile run passed 2,102 and Mobile CI passed; focused discovery 7/7 plus kind-5/predicate/retry mutations passed. The contradictory full-suite outcome is exactly why the nondeterministic oracle is blocking, not why the product fixes are discarded. No native open-picker/VoiceOver journey was run; reviewer/tooling confidence gap only.
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: REQUEST CHANGES
Reviewed: df61d54cb2e4f534a19ce8782a7fd6ccd165b467..1eabb049c6f36f7a5296dc82f0895ce4326bb6dd (exact live head)
Risk: high — relay subscription lifecycle determines whether an open mobile agent picker remains authoritative after failures and ownership deletion.
The two previously reported paths are materially improved: kind-5 coordinate deletions now invalidate the directory, and subscription-establishment failures receive bounded same-session retries. One terminal-closure path and one regression-test reliability defect remain.
Blocking findings
-
Terminal relay
CLOSEDpermanently ends directory freshness without surfacing failure or rebuilding the subscription.mobile/lib/shared/mentions/agent_discovery.dart:96handlesonClosedonly by invalidating once. For a terminal closure,mobile/lib/shared/relay/relay_session.dart:741-747invokes that callback and then removes the live subscription. The directory can refresh once from stored events and continue presenting data as authoritative, but subsequent profile, policy, or kind-5 changes are no longer observed until an unrelated provider rebuild, reconnect, or relaunch. Retryable/rate-limited closures take a different path and retain retry/status handling atrelay_session.dart:749-754.Author action: on terminal
onClosed, either expose an authoritative failure state or boundedly establish a replacement directory subscription under the current generation. Add a production-seam regression covering establish → terminalCLOSED→ recovery (or persistent visible error) → a later valid 30177/kind-5 event changes the directory without reconnect/relaunch. Fence late callbacks and close every retired subscription. -
The new establishment-recovery regression is timing-sensitive and failed the full package suite.
mobile/test/shared/mentions/agent_discovery_test.dart:172-180waits a fixed 180 ms and expects a transient error, but the 250 ms retry may establish and refresh before that assertion executes under suite load. A clean exact-headjust mobile-testrun failed 1 of 2102 tests withExpected throws StateError; Actual emitted [AgentDirectoryEntry]; the focused file subsequently passed 7/7. This test therefore does not deterministically protect the claimed failure→recovery transition.Author action: replace wall-clock observation of the transient state with deterministic completers/fake time, and assert eventual recovery plus bounded attempts.
Verification owner: reviewer for both fixes on the next exact head.
Verified behavior
- The live filter includes kind 5 and validates signed
30177:<signer>:<agent>coordinates before invalidation (agent_discovery.dart:86-100,133-143). - Establishment failure is propagated, retries are bounded to three, retry timers are cancelled on retirement, and ready status clears failure (
agent_discovery.dart:54-125;agent_identity_provider.dart:73-80). - The open picker consumes directory state reactively; loading/error states do not contribute owned-agent rows (
mention_candidates_provider.dart:62-107). Existing selection/send logic binds the selected pubkey and prepares a non-member agent before publishing (compose_bar_widget.dart:357-403,459-531).
Validation at exact head
- Independent clean full suite run:
just mobile-test— FAIL, 2101 passed / 1 failed, timing-sensitive recovery test above. - Independent focused discovery suite: PASS, 7/7.
- Separate exact-head lane:
just mobile-check— PASS;just mobile-test— PASS, 2102 tests; focused discovery — PASS, 7/7; mutation checks for kind-5 filtering, coordinate admission, and retry establishment failed causally as expected before restoring a clean tree. - GitHub identity checked: reviewer
jedwards27; PR authorloganj. - Live PR head rechecked as
1eabb049c6f36f7a5296dc82f0895ce4326bb6ddbefore submission.
Manual/native evidence: no native iOS/open-picker relay journey was run. Real relay delivery while the picker remains open, VoiceOver/large-text/theme behavior, and end-to-end select/add/send remain confidence gaps, not additional author defects. Verification owner: reviewer/integration tooling.
Residual risk: exact-head Mobile CI passed, while unrelated Desktop jobs were still pending during review. CI success does not cover the terminal CLOSED lifecycle described above.
Signed-off-by: Logan Johnson <loganj@squareup.com>
|
Addressed reviews 5157621455 and 5157627230 in b829496, against unchanged #7393
Exact final HEAD: full mobile tests 2,103 passed, mobile check/analyzer and file-size gate passed. Reverting CLOSED handling or suppressing the live 39002 callback fails the new mounted-widget test. Full repository #7530 is mechanically carried to |
jedwards27
left a comment
There was a problem hiding this comment.
:bot: Jude’s code review agent
Verdict: APPROVE
Reviewed: df61d54cb2e4f534a19ce8782a7fd6ccd165b467..b829496e0bf363939afd6b54fc67b4b08716b35b (exact live head)
Risk: high — relay subscription lifecycle controls whether an open mobile agent picker remains truthful after ownership changes and terminal failures.
No author-actionable defect remains. The changed head resolves both prior blockers with bounded lifecycle recovery and deterministic, production-shaped regression coverage.
Findings and traced contracts
- Terminal
CLOSEDrecovery is now generation-safe._AgentDirectoryUpdatesretires the exact subscription attempt, exposes failure, closes an installed handle, and spends the same bounded three-attempt generation budget before leaving exhaustion visibly errored (mobile/lib/shared/mentions/agent_discovery.dart:82-133). Attempt-localactivefencing prevents events, status, or duplicate-close callbacks from a retired attempt affecting its replacement. This matches the relay contract where terminalCLOSEDinvokesonClosedand removes the live subscription (mobile/lib/shared/relay/relay_session.dart:718-747). - The production lifecycle is exercised through the open picker. The new mounted widget regression drives production
RelaySessionNotifierthrough initial EOSE → terminal CLOSED → visible error/row removal → replacement EOSE → candidate restoration → later signed roster removal, proving the same mounted editor drops the candidate (mobile/test/features/channels/discovery_lifecycle_tests.dart:4-87). Mutating productiononClosedback to invalidate-only made this test fail causally; restored code passed with a clean tree. - Recovery tests are deterministic. Changed tests use Flutter fake time (
tester.pump) and assert exact attempts 2/3, no fourth retry after exhaustion, late-callback fencing, and closure of retired handles (mobile/test/shared/mentions/agent_discovery_test.dart:147-277). - Kind-5 coordinate invalidation, reactive picker state, selected-pubkey binding, and non-member agent preparation remain intact. No new keyboard/accessibility defect was found in the changed seam.
Author action: none.
Verification owner: reviewer/tooling for native acceptance; CI infrastructure for Linux rerun.
Exact-head validation
just mobile-check— PASS: 554 files formatted with no changes; analyzer found no issues.just mobile-test— PASS: 2,103/2,103 tests.- Focused discovery plus full compose-bar files — PASS: 121/121 tests; five repeated focused lifecycle runs also passed.
git diff --check— PASS; fresh PR head, declared merge base, and clean trees verified.- Causal mutation of terminal-close handling — expected regression failure, then PASS after restoration.
- Exact-head Mobile and aggregate Clients checks — PASS. DCO, Semgrep, zizmor, macOS Desktop build, and Windows Desktop build also pass.
- Authenticated reviewer
jedwards27; PR authorloganj.
Manual/native evidence: no native iOS/network journey was run. Real-device open-picker relay delivery, select/send, reconnect/relaunch, large text, dark theme, and VoiceOver remain reviewer/tooling-owned residual risk, not author rework.
Residual risk: Linux Desktop jobs currently fail before PR code/tests during browser dependency installation with Google apt Hash Sum mismatch; sampled failures are CI-infrastructure-owned, while macOS and Windows Desktop builds pass. Rollout must still honor the PR’s stated #7394/#7390 integration condition.
🤖
Summary
Mobile's mention picker could only suggest agents that advertise a running instance in a channel you share — an agent you own that isn't currently running (or isn't in any shared channel) never appeared, so you couldn't mention your own agents from another device. This PR widens discovery to owned agents: it finds owner-published agent coordinates, verifies the latest signed agent profile and the owner's current policy, and surfaces the agent in the picker with its verified-owner provenance — no runtime advertisement or shared channel needed.
Do not enable widened discovery until #7394 (safe publication) and #7390 (explicit invitations) are integrated. That is a rollout condition, not a code dependency — this PR's declared base is #7393.
Related issue
commands/agent_discovery/relay_directory.rs(owner-coordinate seed + authenticated exact-policy resolution).Testing
b829496e0:just mobile-checkand thefile-sizegate pass and the full mobile suite passes (2,103 tests) — receipts in the corrected-head evidence comment. A bounded 90-second localjust ciattempt was terminated during workspace clippy, so repository-wide CI completion is not claimed at this head. (Historical only, at prior head5dcc5dea: a full localjust cipass and a green remote CI run — prior evidence comment.)To see it: with an owned agent that isn't running, open the mention picker — it now appears, with its verified-owner provenance.
Screenshots
Flutter production-widget test renders — not native-device screenshots or acceptance captures.
Capture provenance
Rendered by the Flutter widget engine in a
flutter testrun (production widgets, production theme; no device or simulator). Before: this PR's declared base882f69cbaf0f851717f071fba366b3b3a4a877d8. After: its head5dcc5deaf80626aea9f2ff62141792eaa91ec3bb.