Repository navigation
feat(j5): the peering dialog sets up poll mode for you - #406
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
github-actions
Bot
added
size:XXL
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (26)
Included review availability: This review used your included allowance. 1 included review remains after this review. Your included PR review attempts over the past 7 days set your current allowance at 3 reviews per hour. 📝 WalkthroughWalkthroughThe changes add peer address discovery and identity probing, polling-state tracking, and polling-based peering setup. The web dialog and CLI now present connection choices and peer status, and the runbook documents direct and polling setup. ChangesPeer reachability and polling
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant PeerIntroductionDialog
participant runPeeringCheck
participant peeringClient
participant PeerHttp
participant PeerRegistryService
PeerIntroductionDialog->>runPeeringCheck: check both peer directions
runPeeringCheck->>peeringClient: list addresses and probe candidate origins
peeringClient->>PeerHttp: call addresses and probe routes
PeerHttp->>PeerRegistryService: compute origins and probe identity
PeerRegistryService-->>PeerHttp: return origins or probe result
PeerHttp-->>peeringClient: return route responses
peeringClient-->>runPeeringCheck: return addresses and probe results
runPeeringCheck-->>PeerIntroductionDialog: return directional reachability
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change adds reachability checks and poll-mode setup to the peering dialog, and shows peer health in Settings and the CLI. No concrete merge-blocking defect remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 57.89% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 24 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
cfe2a74 to
e176268
Compare
e176268 to
d585848
Compare
d585848 to
369ed2a
Compare
369ed2a to
c393669
Compare
bryantderosier
left a comment
bryantderosier
left a comment
There was a problem hiding this comment.
Had GPT 6.1 Sol and Claude Opus 5.5 review the whole peer-poll stack (#400 → #408) together, so anything flagged here was checked against the top of the stack (ede1490c47a6) first. If a later PR fixes it, I say so instead of asking for a change.
The new probe and addresses endpoints need access:write, send no credential, don't follow redirects, and time out at 4s. So the probe doesn't open up anything meaningful beyond what add already does. Relay and tunnel are fine, because the client URL is only a hint and gets verified against the answering environment id. Mobile not having a peering UI matches cross-device.md:93. The ServerEnvironment.ts edit is recorded in FORK.md.
The things I'd fix are in the dialog:
- Peer again doesn't keep the existing direction or origin (medium). It reruns a fresh recommendation, so repairing from the other side of a two-desktop pair fails with
peer_link_mode_conflict. A changed origin fails withPeerOriginConflictError, which tells the user to pass--replace-origin, and they can't do that from the dialog. Both reviewers found this one. - "Too old" shows up before the other server is even connected (low).
- The check effect reruns on every environment refresh (low, perf).
The rest are suggestions and nits: probe error hygiene, IPv6 addresses offered on an IPv4-only bind, a test loop that checks nothing, and a misplaced doc comment.
| key={peer.environmentId} | ||
| peer={peer} | ||
| primaryEnvironmentId={primaryEnvironmentId} | ||
| onPeerAgain={() => openDialog(EnvironmentId.make(peer.environmentId))} |
There was a problem hiding this comment.
Needs a fix (medium): Peer again only passes the other environment's id, so the dialog reruns recommendPeering from scratch (PeerIntroductionDialog.tsx:115-116, :149-174, :203) and ignores what's recorded. Example: pair two desktop servers from A and take the default, so B polls A. Revoke the credential, then click Peer again on B's stopped row. Now B is local and A is remote, and the desktop-first tie-break (client-runtime/src/j5/peering.ts:317-336) picks A as the poller. It then tries to issue a store credential on B, which already records A as poll, and PeerHttp.ts:295-300 refuses with peer_link_mode_conflict. If the fresh check picks a different origin, you get PeerOriginConflictError asking for --replace-origin, which a dialog user can't pass. This is the UI's only repair path for a rejected credential.
Fix: when re-peering a recorded peer, start from its recorded link mode, direction and origin, and only rotate the credential. Leave mode changes to remove-and-repeer. Worth a test that starts the repair from each side of a two-desktop pair. Both reviewers found this.
There was a problem hiding this comment.
Fixed: Peer again, or Add peer on a pair that's already peered, now starts from each server's record of the other (recordedPeeringChoice). It keeps the link mode, direction and origin, shown read-only, and only issues new credentials. Edits can only fill an address neither record holds (repeeringChoice). Changing the mode is remove-and-repeer, and the dialog says so. Tests in client-runtime peering.test.ts: a two-desktop pair repaired from each side, plus edits that try to move a recorded origin. (40408cd)
| origin: otherOrigin.trim(), | ||
| }; | ||
| const remoteLabel = remote?.label ?? "the other server"; | ||
| : !bothSupportPoll |
There was a problem hiding this comment.
Needs a fix (low): too-old gets decided even when the remote has no descriptor yet (peeringCheck.ts:20-21), so supportsPoll is false. You get "X is running J5 , which is too old" right next to "Connect to X first", with only a Close button. I'd only decide too-old once a descriptor exists, for example otherReady or other.serverConfig !== null.
There was a problem hiding this comment.
Fixed: too-old is decided only once both descriptors exist. (40408cd)
| return () => { | ||
| cancelled = true; | ||
| }; | ||
| }, [checkKey, local, remote, primaryBaseUrl, otherBaseUrl]); |
There was a problem hiding this comment.
Needs a fix (low, perf): this effect depends on the local/remote objects, which get rebuilt on every presentationById change (state/environments.ts:46-52). So any environment's connection or config refresh reruns two address lists plus N probes on both servers. Stale results are dropped, but the work still happens. I'd depend on checkKey and the base URLs, or memoize on primitive fields.
There was a problem hiding this comment.
Fixed: the check effect is keyed on checkKey and the two base URLs only. (40408cd)
| reason: `answered HTTP ${String(response.status)}`, | ||
| }); | ||
| } | ||
| const identity = yield* response.json.pipe(Effect.flatMap(decodePublicIdentity)); |
There was a problem hiding this comment.
Suggestion (security): a failed probe returns reasonOf(cause) word for word, and decode errors can echo fragments of the probed body. response.json also reads a body of any size, and identity.label (:703) skips the reportedLabel bound. I'd map decode failures to a fixed phrase, apply reportedLabel, and cap the body.
There was a problem hiding this comment.
Fixed: the probe body is read with a 64 KiB cap. A body that isn't an identity, or is too large, gets a fixed phrase ("answered, but not as a J5 server") instead of the decode error, and the label goes through reportedLabel. Tested in PeerRegistryService.test.ts with hostile, echoing and huge answers. (40408cd)
| if (input.host !== undefined && !isWildcardHost(input.host)) return [origin(input.host)]; | ||
| return Object.values(input.interfaces) | ||
| .flatMap((entries) => entries ?? []) | ||
| .filter((entry) => !entry.internal && !entry.address.startsWith("fe80:")) |
There was a problem hiding this comment.
Nit: with a 0.0.0.0 (IPv4-only) bind this still offers IPv6 addresses, which can only fail and clutter the error list.
There was a problem hiding this comment.
Fixed: a 0.0.0.0 bind offers IPv4 addresses only. Tested in peerReachability.test.ts. (40408cd)
| const addresses = await admin.handler(get(J5_PEER_API_PATHS.addresses)); | ||
| assert.equal(addresses.status, 200); | ||
| const { origins } = (await addresses.json()) as { origins: ReadonlyArray<string> }; | ||
| for (const origin of origins) { |
There was a problem hiding this comment.
Nit: this "not a loopback" loop doesn't check anything. The test layer's host is undefined, so origins is []. The real coverage is in peerReachability.test.ts, so I'd either assert [] here or drop the loop.
There was a problem hiding this comment.
Fixed: it now asserts { origins: [] }. (40408cd)
|
|
||
| const errorText = (cause: unknown) => (cause instanceof Error ? cause.message : String(cause)); | ||
|
|
||
| /** One direction: `from` fetches `to`'s public identity at each address `to` might be reached at. */ |
There was a problem hiding this comment.
Nit: this doc comment is sitting above OfferedAddresses, but it's describing reach.
There was a problem hiding this comment.
Fixed: the comment now sits above reach. (40408cd)
c393669 to
40408cd
Compare
bryantderosier
left a comment
bryantderosier
left a comment
There was a problem hiding this comment.
Approving. Two dialog dead-ends worth a follow-up ticket:
- A record that only exists on the other server locks the mode, and there's no way to clear it from here. If only the remote still has a record of this server,
PeerIntroductionDialogtakes the mode from that record (recordedPeeringChoice), hides "Set up differently", and says "remove the peer, then peer again". But this server has no row to remove. Example: A polls B, the person removes B on A while this client isn't connected to B, so B keeps itsstorerecord of A. Later they want B to send to A directly, and Add peer on A forces "A polls B" with no manual form. The fix is on B (its own UI or the CLI), and the dialog never says so. I'd either name the server whose record is pinning the mode or offer to remove that remote record from the dialog. - Peer again opens an empty dialog when the peer isn't saved in this client.
PeerServersSettings.tsxpassespeer.environmentIdwhatever its connection state. If that server isn't one of this client's environments,otherresolves to null and you get "Choose a remote server" with Peer disabled and no "Connect to X first" or "Add another environment" hint, since that hint only shows when there are no candidates at all. I'd say "add or connect to this server first" in that case, or hide the button and say why on the row.
40408cd to
0216dbf
Compare
0216dbf to
f81eb0a
Compare
f81eb0a to
f019528
Compare
Peering two servers assumed each could reach the other, and asked the person to type each address and a name for each side. Now the dialog checks first and recommends. - Two admin routes support the check: GET /api/j5/a2a/peers/addresses lists the origins a server might be reached at (none for a loopback-only server), and POST /api/j5/a2a/peers/probe fetches another server's public identity at an origin within four seconds, returning who answered or the error verbatim. The descriptor now publishes j5PeerPoll. - client-runtime's peering module turns each server's descriptor (its version, j5PeerPoll and run mode) and the two probes into a recommendation: poll without asking when one direction cannot connect, send directly when both connect and run as services, and ask one question, storing by default, when a side runs in the desktop app, was started by hand, or could not be tested. A too-old server gets only "update J5 there". It also introduces a poll pairing in two steps. - The dialog shows the check, the question, how messages will travel in plain lines and only the address that will be used, with "Set up differently" for the manual form. A server that could not list its addresses says why, rather than a guess about its network. - Peer rows say how messages travel and show online or offline, the backlog and the last error, refreshed while Connections stays open. A poller whose credential was rejected gets "Peer again", which opens the dialog with that server chosen. A pair already peered is peered again the way it is set up, with new credentials only; changing how messages travel is removing the peer first. A protocol mismatch only names the server to update. - The probe fetches that one path and follows no redirect. - `j5 a2a peer list` prints the same health, from one online rule in contracts. The runbook covers the poll pairing and the peer list's health. Stack 6/8 for #399. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
f019528 to
de10d34
Compare
This branch was successfully deployed
2 participants
Stack 6/8. Depends on #405.
Problem
Peering asked the person to type both addresses and a name for each side, and assumed each server could reach the other. With poll mode in place (#404, #405), the dialog can check how the two servers actually reach each other and recommend a setup. Part of #399.
What changed
Reachability routes. Two admin routes support the check. Both need
access:write.GET /api/j5/a2a/peers/addresseslists the origins a server might be reached at. A server that listens on loopback only lists none.POST /api/j5/a2a/peers/probefetches another server's public identity at one origin, within 4 s. It follows no redirect, and returns who answered or the error verbatim.Descriptor. It now publishes
j5PeerPoll.Recommendation. client-runtime's peering module turns each server's descriptor (its version,
j5PeerPolland run mode) and the two probes into a setup:Dialog. It shows:
A server that couldn't list its addresses says why, instead of a guess about its network. When the check couldn't test a direction, it says so; only directions that were tried and failed are called unreachable. "How does this work?" links to
j5.codes/peering(feat(marketing): explain peering at j5.codes/peering #422).Peer rows in Settings → Connections.
Poller. Each time it stops (rejected credential, or a 403/409 refusal), and each time a protocol mismatch is retried, it records
Polling stopped: <reason>as the peer's last error. That mark is how the CLI and the rows know polling stopped. Failures it retries carry no mark. The mark stays until the poller's next successful poll or the peer is recorded again; other errors and successes don't erase it, and only a newer stop replaces it.CLI.
j5 a2a peer listprints the same health, using one online rule that now lives in contracts. A stopped poller printspolling stopped: <reason>, and a peer this server polls has noinbound:field, since it never holds a session here.Runbook. It covers the poll pairing and the peer list's health, and says to restart J5 after renaming a machine.
UI changes
The introduction dialog is rebuilt to the approved mockup (
plans/peering-onboarding-mockup.html), and the peer rows gain their status and Peer again.Captured from real pairs: the stack's build served twice on one host, Local on :7708 and Remote on :7808, peered through this dialog in poll mode. Both servers run on one machine, so both are named
digeng-j5code-01; on two machines each line names a different server.j5/mainb3a45dc047: the rows and the remove confirmation at8841511873, the dialog shots at743054e36f.Before (
j5/main): the manual form and a push peer row.After: the check found both directions reachable and Remote started by hand, so it asks one question, with storing as the default.
A pair that knows only loopback addresses for itself: the check says it couldn't test either direction.
The peer rows on both servers. Then Remote is stopped with a message waiting, and finally Local removes the peer and Remote finds its credential rejected.
Removing a peer:
j5 a2a peer liston Remote at that point:Upstream impact
apps/server/src/environment/ServerEnvironment.tsand its test change one line each, reportingj5PeerPoll: true. They are recorded in FORK.md case 48, and their file-table rows now read "34, 48".peerReachability.ts. That file imports upstream's host helpers fromstartupAccess.tswithout changing them.Checklist
peeringCheck: a failed address list is reported as its error; an untested direction is never called unreachable;inbound:for a poll record;peerPoll: the online rule, the stop mark, and credential rejection only from a marked stop.Built by Claude Opus 5.5 (1M context) in Claude Code, as the builder seat of a J5 crew. Reviewer-passed.
🤖 Generated with Claude Code
Summary by CodeRabbit