Skip to content

Commit dcf02f4

Browse files
Branimir Rakiccursoragent
authored andcommitted
fix(network): warn once when ProtocolRouter.send() runs without peerResolver (Codex PR #497 round 5)
Codex review feedback: keeping peerResolver optional makes the cold- peer priming guarantee implicit. Any future `new ProtocolRouter(node)` on an outbound path would compile and silently skip priming, reintroducing PR-448's class of relay-route failures. Two mitigations are now in place: 1. CI grep gate (`scripts/audit-dial-protocol.mjs` from PR #499) bans raw `dialProtocol(peerId)` calls outside an allowlist, so any new outbound path is forced through ProtocolRouter. 2. (this commit) `send()` emits a one-time `console.warn` the first time it runs without a peerResolver, surfacing misconfiguration loudly at first cold dial rather than silently regressing. The full enforcement (`peerResolver` becoming required) is intentionally deferred — it would be a breaking change to test fixtures and edge callers that legitimately route over warm connections only. The warn+grep combo turns the silent-bypass risk into a build-time + first-call-time surface. Adds a test that asserts: (a) the warn fires, (b) only once across multiple sends, and (c) references RFC 07 / PR #448 in the message. Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 68f08cd commit dcf02f4

2 files changed

Lines changed: 76 additions & 6 deletions

File tree

packages/core/src/protocol-router.ts

Lines changed: 40 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -36,12 +36,24 @@ export interface ProtocolRouterOptions {
3636
* RFC 07 §3.2 — when present, `send()` consults the resolver before
3737
* dialing so the libp2p peerStore is primed with whatever multiaddrs
3838
* the resolution order finds (live-conn → DHT → RFC 04 registry →
39-
* agents-CG → bootstrap). Optional during the RFC 07 migration; the
40-
* resolver is wired in `dkg-agent.ts` for production daemons. Tests
41-
* that don't exercise the resolver may omit it. PR-4 of the RFC 07
42-
* rollout adds a CI grep gate that disallows raw `dialProtocol(peerId)`
43-
* outside `peer-resolver.ts`, by which point this becomes a structural
44-
* property of the codebase rather than a recommendation.
39+
* agents-CG). Required for any router that handles agent P2P
40+
* traffic in production; optional only for local/in-process tests
41+
* that exclusively talk over already-warmed connections.
42+
*
43+
* Codex review feedback on PR #497 round 5: keeping this optional
44+
* makes the cold-peer priming guarantee implicit. Two mitigations
45+
* are in place:
46+
* 1. CI grep gate (`scripts/audit-dial-protocol.mjs`, PR-4 of the
47+
* RFC 07 rollout) bans raw `dialProtocol(peerId)` calls
48+
* outside an allowlist, so any new outbound path is forced
49+
* through `ProtocolRouter`.
50+
* 2. `send()` emits a one-time `console.warn` the first time it
51+
* runs without a `peerResolver` configured (see implementation
52+
* below), so a misconfiguration is loud at the first cold dial
53+
* rather than silently regressing PR-448's class of failures.
54+
* Together these turn the "any future caller that omits the
55+
* resolver silently skips priming" risk into a build-time + first-
56+
* call-time surface.
4557
*/
4658
peerResolver?: PeerResolver;
4759
}
@@ -50,6 +62,13 @@ export class ProtocolRouter {
5062
private readonly node: DKGNode;
5163
private readonly peerResolver?: PeerResolver;
5264
private handlers = new Map<string, DKGStreamHandler>();
65+
/**
66+
* One-shot guard: we warn the first time `send()` runs without a
67+
* `peerResolver` so a misconfigured outbound router is loud, but
68+
* we don't spam the logs on every subsequent call. Codex PR #497
69+
* round 5 — see `ProtocolRouterOptions.peerResolver`.
70+
*/
71+
private warnedMissingResolver = false;
5372
readonly maxReadBytes: number;
5473

5574
constructor(node: DKGNode, options?: ProtocolRouterOptions) {
@@ -122,6 +141,21 @@ export class ProtocolRouter {
122141
await this.peerResolver
123142
.resolve(peerIdStr, { signal, perStepTimeoutMs: remaining })
124143
.catch(() => undefined);
144+
} else if (!this.warnedMissingResolver) {
145+
// Codex PR #497 round 5: structural enforcement (CI grep gate)
146+
// already prevents raw `dialProtocol(peerId)` calls outside an
147+
// allowlist, but a router constructed without a resolver still
148+
// silently skips priming. Surface it loudly the first time so a
149+
// misconfigured outbound path is caught at first cold dial
150+
// rather than reintroducing PR-448's relay-route failures.
151+
this.warnedMissingResolver = true;
152+
console.warn(
153+
'[ProtocolRouter] send() called without a peerResolver configured. ' +
154+
'Cold-peer dials will fall back to libp2p\'s identify-cached addresses ' +
155+
'and may fail to find relay routes (see RFC 07 §3.2 / PR #448). ' +
156+
'Pass `{ peerResolver }` to the ProtocolRouter constructor for any ' +
157+
'router that handles agent P2P traffic.',
158+
);
125159
}
126160

127161
// libp2p internally upgrades relay connections to direct during

packages/core/test/protocol-router-resolver.test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,4 +208,40 @@ describe('ProtocolRouter.send + PeerResolver', () => {
208208
);
209209
expect(dec.decode(response)).toBe('echo:hi');
210210
}, 15000);
211+
212+
// Codex review feedback on PR #497 round 5: keeping peerResolver
213+
// optional makes the priming guarantee implicit and a future
214+
// `new ProtocolRouter(node)` would silently skip priming. The
215+
// mitigation is a one-time loud warn at first cold dial.
216+
it('warns once on first send() when peerResolver is omitted (PR #497 round 5)', async () => {
217+
const a = spawn();
218+
const b = spawn();
219+
await a.start();
220+
await b.start();
221+
await a.libp2p.dial(multiaddr(b.multiaddrs[0]));
222+
await new Promise((r) => setTimeout(r, 300));
223+
224+
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {});
225+
226+
const routerA = new ProtocolRouter(a);
227+
const routerB = new ProtocolRouter(b);
228+
const enc = new TextEncoder();
229+
const dec = new TextDecoder();
230+
routerB.register('/test/warn-no-resolver/1.0.0', async (data) =>
231+
enc.encode(`echo:${dec.decode(data)}`),
232+
);
233+
234+
await routerA.send(b.peerId, '/test/warn-no-resolver/1.0.0', enc.encode('1'));
235+
await routerA.send(b.peerId, '/test/warn-no-resolver/1.0.0', enc.encode('2'));
236+
await routerA.send(b.peerId, '/test/warn-no-resolver/1.0.0', enc.encode('3'));
237+
238+
const matches = warnSpy.mock.calls.filter((c) => {
239+
const first = c[0];
240+
return typeof first === 'string' && first.includes('peerResolver');
241+
});
242+
expect(matches.length).toBe(1);
243+
expect(matches[0][0]).toMatch(/RFC 07/);
244+
245+
warnSpy.mockRestore();
246+
}, 15000);
211247
});

0 commit comments

Comments
 (0)