Repository navigation
Conversation
loadKuboConfigFile looked in .ipfs-bitsocial-cli while kubo runs in .bitsocial-cli.ipfs or $IPFS_PATH, so the read always came back empty and the daemon fell back to env vars and defaults. Both now use resolveKuboRepoPath. That makes the multiaddr parsers reachable for the first time, so they now handle IPv6 hosts, list-valued addresses and unix sockets.
computeKuboConfig(kuboConfig, reachabilityConfig, enabled) is the one place that decides the kubo edits. planKuboConfig also returns a record of what changed, which tells our entries from identical operator ones and lets disable restore the touched fields byte for byte.
An explicit KUBO_RPC_URL or IPFS_GATEWAY_URL can name a kubo other than the one in this repo, so it still wins over the kubo config. Connectivity checks now strip the brackets a URL keeps around an IPv6 host.
reachability/config.json holds the operator's choices and refuses to load a newer version rather than drop its fields. The daemon's check results go in a separate file so they cannot overwrite a setup in progress.
…e status Caddy keeps certificates for old addresses and other CAs until they expire, so status now filters to the configured CA and current names. A damaged runtime status file reads as no checks yet, and the text output prints domain and Caddy checks.
…le tests Caddy is downloaded on first use and checked against a sha256 pinned in source. The generated Caddyfile keeps Caddy off UDP 443 and port 80. The integration test issues a certificate for 127.0.0.1 from Pebble and completes a js-libp2p handshake with kubo through Caddy.
Discovers public addresses from interfaces, kubo and the router, obtains an IP certificate, applies the kubo config and restarts kubo. It checks what kubo really listens on, because kubo stays silent when it cannot bind UDP 443.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 34 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
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: 654557cb81
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!exitedEarly) { | ||
| await stopCaddy(spec.adminPort).catch(() => caddy.kill()); | ||
| await exited; |
There was a problem hiding this comment.
On the normal first-run path, setup starts Caddy, obtains the certificate, and then unconditionally stops Caddy before returning. No daemon code imports or starts Caddy, so the newly announced /tls/ws address has no TLS proxy listening as soon as bitsocial reachability setup exits, making browser WSS dials fail despite setup reporting success.
Useful? React with 👍 / 👎.
| export async function discoverNatRouter(timeoutMs = 5000): Promise<NatRouter | undefined> { | ||
| const { upnpNat } = await import("@achingbrain/nat-port-mapper"); | ||
| const client = upnpNat({ description: "bitsocial reachability", autoRefresh: false, ttl: MAPPING_TTL_MS }); |
There was a problem hiding this comment.
For nodes behind a UPnP router, these mappings are created with a one-hour TTL and automatic refresh disabled. The only mapPorts call in src occurs during setup, so after an hour the advertised TCP 443 and UDP 443 endpoints become unreachable while the persisted configuration still reports reachability as enabled.
Useful? React with 👍 / 👎.
| export default class Setup extends Command { | ||
| static override description = | ||
| "Make this node dialable from browsers: discover public addresses, obtain a certificate, configure kubo and verify. Safe to rerun"; | ||
|
|
||
| static override flags = { | ||
| "pkcOptions.dataPath": Flags.directory({ description: "Data path of the node", required: false }), | ||
| staging: Flags.boolean({ description: "Use the Let's Encrypt staging CA. Its certificates are not trusted by browsers", default: false }), |
There was a problem hiding this comment.
The commit adds reachability setup and reachability status, but the generated README contains no entries for either command; its only change is the daemon's Kubo path. This leaves the new user-facing commands and flags absent from the repository's command reference, contrary to the requirement to update documentation when commands or flags change.
AGENTS.md reference: AGENTS.md:L70-L81
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const fromInterfaces = deps.publicIpsFromInterfaces(); | ||
| const identity = await kuboIdentity(options.kuboRpcUrl); | ||
| const fromKubo = identity ? publicIpsFromMultiaddrs(identity.addresses) : []; | ||
| const ips = new Set([...fromInterfaces, ...fromKubo]); |
There was a problem hiding this comment.
Bug: a rerun after the public IP changes keeps announcing the old IP.
fromKubo comes from kubo's /id Addresses, which include the AppendAnnounce entries a previous setup wrote (e.g. /ip4/OLD_IP/tcp/443/tls/ws). Discovery never excludes them, so the old IP A is treated as a current one. waitForCertificates (src/reachability/caddy.ts, ~L205-209) counts any unexpired cert on disk as issued, and short-lived IP certs last about 6 days.
Scenario: setup runs on IP A, the ISP moves the host to B, and the operator reruns setup within ~6 days. issuedIps becomes [A, B], and both land in the Caddyfile, publicIps and kubo's AppendAnnounce, so the dead A stays announced. After A's cert expires, every rerun sends a doomed ACME validation for A and waits the full 90 s.
Suggested fix: drop hosts from previous.announce / previous.kuboEdits.addedAppendAnnounce out of fromKubo, or use only observed addresses. Alternatively, require certs issued after setup started.
| const wait = await Promise.race([ | ||
| waitForCertificates(options.dataPath, spec.acmeDirectory, names, timeoutMs), | ||
| exited.then(() => undefined) | ||
| ]); |
There was a problem hiding this comment.
Bug: the CLI hangs for up to ~90 s when Caddy exits early.
When the exited promise wins the Promise.race, the losing waitForCertificates call is abandoned but keeps polling on 500 ms setTimeouts until its 90 s deadline. It takes no AbortSignal, and the timers are not unref'd. The success path in src/cli/commands/reachability/setup.ts does not call process.exit.
Scenario: Caddy dies at startup (config error, or a non-root bind denial the user declines to fix), and setup continues to the "unreachable" warning and returns. The process then sits idle for most of the 90 s before Node exits. In the setcap retry path, the first attempt's poller also keeps running next to the second.
Suggested fix: pass an AbortSignal to waitForCertificates and abort it when Caddy exits, or unref the poll timer.
|
Consolidated into #208. |
Part of #198 (step 5). Based on #204 and #201, so their commits show here until those merge.
Summary
Gap in the issue, handled here. Linux
setcapon Caddy does not help kubo, which also has to bind UDP 443. kubo starts normally, logs nothing and just doesn't listen, so "WebTransport, always" would report success with nothing listening. Setup now checksswarm/addrs/listenafter the restart and prints the fix: theip_unprivileged_port_startsysctl, which covers both binaries, orsetcapon kubo, which is lost whenever kubo updates. Verified by running kubo as an unprivileged user.Not done from the issue:
pmpNatneeds the router's address, and only UPnP discovery is implemented. libp2p's ownupnp-natis also UPnP only.Guard: setup refuses the production Let's Encrypt directory when
CIis set.Evidence
bitsocial reachability setupdid not exist.After:
npm run test:clipasses.No real Let's Encrypt issuance has run yet. The manual staging test from #198 is still needed before anyone uses this for real.
Merge Danger
Door: two-way
disable(step 8) reverses the kubo edits, but until it lands reverting means editing the kubo config by hand.Blast Radius: opt-in
Nothing changes unless an operator runs
setup. A run writes the kubo config and restarts kubo, and until step 7 the daemon does not spawn Caddy, so this PR should not ship without step 7.