test(agent): pin LU-11 random-sampling prover backfill orchestration#743
Merged
Merged
Conversation
PR #716 audit cluster B.4 — `buildCiphertextChunkBackfill` at `dkg-agent.ts:11221-11299` is the curated-path glue between the Random Sampling prover and `fetchCiphertextChunkFromPeer`. The prover invokes the returned closure whenever its extractor reports a `CiphertextChunksMissingError` (the OT-RFC-39 late-join path). The closure's individual building blocks all had direct coverage: - `fetchCiphertextChunkFromPeer` (#742) - `handleGetCiphertextChunk` (#739) - `ingestSwmCiphertextChunkEnvelope` (#742) But the ORCHESTRATION layer — peer iteration, self-exclusion, denied/error classification, aggregation — had zero direct coverage. A refactor that flipped continue/break semantics or dropped the self-filter would silently break the late-join contract on every core that joined a CG after the curator's chunked publish rolled off the gossip mesh. New file `packages/agent/test/lu11-backfill-orchestration.test.ts` pins all 6 return shapes + 3 iteration policies, 9 tests total: Return shapes: - `{ fetched: N, failures: 0 }` happy path - `{ reason: 'cg-not-locally-registered' }` chain-event race window - `{ reason: 'no-peers' }` workspace topic empty after self-exclusion - mixed `fetched/failures` with NO aggregated reason - `{ reason: 'all-denied: <lastDenied>' }` everyone authoritatively no - `{ reason: 'no-responders' }` everyone transport-failed (no ACK) Iteration policies: - Self-exclusion: local peerId is filtered out even when the local node subscribes to its own workspace topic. Without this the prover would try to fetch its own missing chunks from itself, guaranteed failure mode + wasted candidate slot. - Per-chunk fall-through: a `denied` ACK from peer A advances to peer B for the SAME chunk; a successful B short-circuits the inner loop. Pins both halves of the loop semantics. - Zero missing indexes: fast short-circuit BEFORE topic resolution — neither `gossip.getSubscribers` nor the messenger is invoked. The orchestrator is correct today (verified by inspection); these pins ensure peer-iteration / aggregation semantics can't silently drift on a future refactor. Co-authored-by: Cursor <cursoragent@cursor.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
12 tasks
…ests
Three issues raised, all valid:
1. Do not replace `agent.node` with a bare `{peerId}` stub.
`DKGAgent.stop()` reaches into `this.node.stop()` during teardown
and the bare stub would silently break shutdown — masked by the
`afterEach` swallow-catch, leaking timers/libp2p state into later
tests. Override the agent's OWN `peerId` getter via
`Object.defineProperty` instead, shadowing the prototype getter
on the instance and leaving the real `node` intact.
2. `ReliableSendResult` union: `delivered: false` with `queued: false`
is only valid for the `inFlight: true` sender-side dedup variant.
Hard transport failures use `delivered: false, queued: true,
nextAttemptAtMs` (durable retry). Switched both occurrences to
the durable-retry shape — the realistic production failure mode
and the one the union actually admits.
3. The `all-denied` test accepted either peer's denial reason, so a
regression from "last-denial-wins" to "first-denial-wins" would
pass silently. The closure iterates the candidate set in
insertion order (Set-from-Array preserves it), so with
`[peerA, peerB]` subscribers the FINAL `lastDenied` is always
peerB's reason. Pinned the exact value.
Codex review on #743.
Co-authored-by: Cursor <cursoragent@cursor.com>
Codex review (round 2) on PR #743 raised two valid follow-ups: 1. The `afterEach` wrapped `agent.stop()` in `.catch(() => undefined)`, which would hide any teardown regression — for example a future reintroduction of the round-1 `node`-replacement bug would silently leak timers/libp2p state into later tests while the originating test stayed green. Removed the swallow: null the reference first (so the next test gets a fresh slot even if teardown throws), then await `stop()` without a catch. Teardown bugs now fail locally rather than in a downstream suite. 2. The messenger stub captured the protocol but never validated it, and ignored the payload entirely. A regression that called the wrong protocol id or sent the wrong (contextGraphId, batchId, chunkIndex) wire fields would slip through every test that used the helper. Extended the capture to include payload bytes, and added validation in the happy-path test: - protocol id === '/dkg/10.0.2/get-ciphertext-chunk' - decoded contextGraphId equals the local cg id - decoded batchId equals the requested batchId - decoded chunkIndexes match the missingIndexes in order Codex review on #743 (round 2). Co-authored-by: Cursor <cursoragent@cursor.com>
Codex review (round 3) on PR #743 raised three valid follow-ups: 1. The all-denied and all-errored tests used a single `missingIndexes: [0]` entry, so they exercised only the single-chunk path of `lastDenied`/`failures` aggregation. A regression that miscounted across chunks would slip through. Both tests now use `missingIndexes: [0, 1]` and assert the exact `failures: 2` count. The all-denied test's "last-denial- wins" assertion now holds across BOTH chunks (peerB's reason is still the final value because peerA precedes it in iteration order for each chunk). 2. The happy-path wire-fidelity check hardcoded the protocol string `/dkg/10.0.2/get-ciphertext-chunk`. Routine protocol- version bumps would otherwise break the test even when the orchestrator's behavior is still correct. Imported and used `PROTOCOL_GET_CIPHERTEXT_CHUNK` from `dkg-core` instead, so the assertion only fails on real protocol changes. 3. Inline comments were noisy with review provenance (`Codex review (round N) feedback: ...`). Stripped the tool / round markers while keeping the behavioral rationale — provenance is in the commit log and PR history, not the test body. Codex review on #743 (round 3). Co-authored-by: Cursor <cursoragent@cursor.com>
Codex review (round 4) on PR #743 raised two valid follow-ups on the all-denied test: 1. The test asserted only the final counters (fetched=0, failures=2). A regression that stopped after the first missing index and derived `failures` from `missingIndexes.length` would still pass — the counter would read 2 with only 2 wire requests instead of 4. Pinned the actual request pattern: 4 sends, peers cycle [A, B, A, B] across the two chunks, chunk indexes are [0, 0, 1, 1]. 2. The exact-suffix pin (`'all-denied: peer-rate-limited'`) was over-constraining the impl: `result.reason` is documented as operator-facing free-form, so a harmless refactor (peer sorting, parallel fetches) would break the test without changing the contract. Loosened to assert that: - reason starts with `all-denied: ` (proves the aggregator surfaced a denial-class result, not a transport error or empty string) - the suffix is one of the two reasons wired into the stub (proves the suffix isn't empty/garbled) Which specific peer's reason wins is now an iteration-order detail, not a behavioral contract. Resolves the round-2 / round-4 review contradiction: round 2 asked to pin the exact reason (to distinguish first-vs-last); round 4 said that's too brittle. The per-chunk request pattern already pins iteration order independently, so the reason assertion can be loose without losing first-vs-last protection. Codex review on #743 (round 4). Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PR #716 audit cluster B.4 — closes the last LU-11 coverage gap:
the
buildCiphertextChunkBackfillclosure that glues theRandom Sampling prover to
fetchCiphertextChunkFromPeer.Background
dkg-agent.ts:11221-11299. The closure is handed to the provervia
bindRandomSampling(dkg-agent.ts:2832-2870) and invokedwhenever the prover's extractor reports a
CiphertextChunksMissingError(OT-RFC-39 late-join path — thiscore missed the curator's chunked publish).
Per-building-block coverage existed:
But the orchestration layer — peer iteration, self-exclusion,
denied/error classification, aggregation — had zero direct
coverage. A refactor that flipped `continue`/`break` semantics or
dropped the self-filter would silently break the late-join
contract on every core that joined a CG after the curator's
chunked publish rolled off the gossip mesh.
What this PR adds
New file
`packages/agent/test/lu11-backfill-orchestration.test.ts` — 9
tests covering all 6 return shapes + 3 iteration policies.
Return shapes (6 tests)
Iteration policies (3 tests)
even when the local node subscribes to its own workspace topic.
Without this the prover would try to fetch its own missing
chunks from itself — guaranteed failure mode + wasted candidate
slot.
to peer B for the SAME chunk; a successful B short-circuits the
inner loop. Pins both halves of the loop semantics.
resolution; neither `gossip.getSubscribers` nor the messenger is
invoked.
The orchestrator is correct today (verified by inspection); these
pins make peer-iteration / aggregation drift loud at PR time.
Test plan
Related: #716, #715, #717, #727, #729, #742.
Made with Cursor