test(sync): hermetic negentropy coverage + agent-eval fixes - #64
Merged
Merged
Conversation
TestNegSync_Integration reconciled against wss://relay.ohstr.com, so an outage there was indistinguishable from a regression -- that relay now returns a Cloudflare 404 and the test fails on a bad websocket handshake. The sync stack runs its relay config twice instead (remote 45520, remote2 45521). NegentropyPropagatesBetweenRelayInstances seeds the first, reconciles it down into a temp local store, then pushes that store up into the empty second, asserting every seeded ID arrives. It checks the second relay is empty first, so the assertion can't pass on pre-existing data. Relays only answer NEG-OPEN and never initiate reconciliation with each other, so the module runs twice against one local store rather than the relays talking directly. TestNegSync_Integration is deleted and dropped from just test-integration.
Rounds write self-reports to a flat path under report/ and only copy them into the timestamped run dir afterwards, so nothing cleared them first and a round could read the previous run's file. Clear them before each round, from the round loop rather than run_round() -- run_r6 backgrounds run_round and polls for r6-bunker-uri.txt, so clearing inside the background job would race that poll onto a stale URI. Also: - .env was written only when missing and never removed, pinning one vault password across every run despite being documented as per-run. - prepare_r6 discarded its own output, so a failed identity step surfaced only as a generic "daemon did not come up" warning 15s later. - r8-error-contract.sh's four probes shared fixed /tmp paths that two concurrent runs would clobber and nothing cleaned up; its `set +e` was dead, since only `set -uo pipefail` is ever set. - report/ was made world-writable when the container user just needs to own it. R2 queried wss://relay.ohstr.com; it now queries the stack's own relay, seeded by prepare_r2 with kind:1 events via ncli id sign + ncli publish. That removes the harness's last public-relay dependency.
CONTRIBUTING no longer lists TestNegSync_Integration among the live-relay tests, points at the hermetic replacement, and notes that agent-eval is manual and billed. The local-verify skill claimed there's no `ncli publish` command -- there is (cli/ncli/publish.go), so it now shows the sign-and-publish sequence for seeding a relay. It also claimed relay.ohstr.com is reachable from this sandbox; it returns a Cloudflare 404 today, so the guidance is to probe before trusting a failure and prefer a relay you seed yourself.
runSyncLeg loaded sync.yaml's fixture filter verbatim, and that fixture is `since: 0`. By the time this subtest runs the earlier ones have left several hundred events on relay A, so the down leg pulled all of them into the temp store and the up leg pushed all of them into B -- work the test never intended, growing again with every subtest added above it, against a fixed 90s completion budget. Scoping both legs to the seeded kind/author since a timestamp taken just before seeding drops the subtest from 1.32s to 0.31s. Also: - Register t.Cleanup(sm.Close). This was the only SyncModule in the file without it, so a waitForSyncComplete timeout skipped the explicit Close and leaked the connection and store handle. Both Close paths are sync.Once-guarded, so closing twice is safe. - Drop the 200ms sleep after sm.Close(). EventStore.Close waits on its workers and closes the db inline, so the handle is already gone -- the sleep was copied from the subtests that defer Close to t.Cleanup, where it is load-bearing, and its comment claimed an async close that does not exist. - Drop `spec.From = rf`. SyncSpec.From is read only by UnmarshalJSON, to populate the private remote pointer; nothing in neg_sync.go reads it. compose.yaml picks up the x-relay anchor idiom the stream stress stack already uses, so remote2 no longer repeats remote's build block for the same image tag.
`chown 10001:10001 report` replaced `chmod 777 report`, but report/ is
tracked (report/.gitkeep) and exists in every clone owned by the checkout
user: the chown needs root, is never reverted, and leaves the developer's
own directory owned by a foreign uid, so their next non-root `git clean`
or the next run's `mkdir -p report/${RUN_ID}` fails with EACCES. It also
sits above the HOST_CREDS check, so a non-root run aborted on `chown:
Operation not permitted` instead of the actionable "log into Claude Code
first" message. Back to chmod, with the reasoning written down.
cleanup() deleting .env left every post-mortem `docker compose
logs/ps/exec` in that directory running with a blank NCLI_VAULT_PASSWORD,
which surfaces as an auth error that reads like a product bug. Only the
per-run regeneration was needed to stop the password going stale, so the
deletion is dropped.
Also: prepare_r2 wrote fixed container paths, the same defect this branch
fixes in r8-error-contract.sh -- switched to mktemp with a trap. The R2
verifier now asserts all 8 seeded events came back rather than ">= 1",
matching on their content marker, since the harness controls this corpus
and a half-failed seed should not pass on one survivor. Its seed-failure
message now names r0-bootstrap as where ncli gets installed.
- integration/sync/relay.yaml still said it served "compose.yaml's single remote service" and that there was "no multi-service collision to worry about". Both services mount it now, so it documents why that's safe the way integration/inspect/relay.yaml does, including that the two instances share one relay identity and nothing depends on them differing. - The `just sync` recipe still described one container; integration/ README.md's port registry still read "sync 45520", which would let the next stack allocate 45521 and collide at run time. - local-verify's heading still promised "a live public relay" while its body now says not to assume one is up, and the rewrite had swallowed the blank line that separated that advice from the targets/filters rule. Its new seeding snippet also called bare `ncli`, i.e. whatever release is on PATH, defeating the skill's whole point of driving ./bin/ncli.
testdata/events.json holds 100 real, signed kind:1 events dumped from wss://nos.lol, kept verbatim as `ncli dump` wrote them so the command in testdata/README.md reproduces the same shape. Validated before committing: all 100 ids hash to their own content, all field lengths are well-formed, there are no duplicates, and every signature verifies -- confirmed independently by publishing the set to a local `ncli relay`, which rejects bad signatures (100/100 accepted). client/fixtures_test.go re-checks that on every run via nip01 Event.Verify (format, schnorr signature, id-to-content binding) in 0.04s with no network, so a corrupted or hand-edited fixture fails there rather than surfacing as a confusing failure in whatever test consumes it. None of the events carry a nonce tag, so Verify's NIP-13 branch never applies and the default (PoW-checking) call is safe.
TestMultiRelaySync streamed from relay.primal.net and nos.lol into a local relay. TestStreamIntegration already covers multi-source fan-in against real relay containers, so the live version added no coverage -- only flakiness, and it had grown a classifier whose whole job was deciding which of its own failures to excuse as "an environment condition". It's gone, and `just test-integration` now runs only the cli/bunker live suite, which skips itself when its relay is unreachable. That leaves no Go test fetching from a public relay. testdata/events.json grows from 100 kind:1 events to 339 across 10 kinds: metadata, notes, contacts, reposts, reactions, reports, zap receipts, relay lists, blossom servers and long-form articles -- chosen so `profile`, relay zap/search indexing and the query paths all have realistic input. It's slightly smaller than the single-kind version it replaces: contact lists average ~16KB each, so only five are included. The whole set round-trips through ncli relay -- 339/339 accepted and 339/339 read back -- so seeding a relay from it loses nothing. Replaceable kinds are unique per author (per d-tag for 30023), so none collapse on ingest. client/fixtures_test.go now asserts the kind mix as well as verifying every signature, so a regenerated fixture that silently drops a kind fails loudly. Only 24 of 60 sampled zap receipts were storable: ncli's NIP-57 validation rejects an `lnurl` tag holding a lightning address rather than bech32 LNURL, which real clients commonly send. The fixture keeps the storable ones; testdata/README.md records the rejection and why it may be an interop gap rather than bad data.
integration/README.md pointed at client/multi_relay_test.go for the skip-gating convention; that file is gone, so the rule is stated directly. The changelog's .env entry still said the file was "never removed", which described the bug before the fix rather than after -- .env is now deliberately left in place so a post-mortem `docker compose` in that directory keeps working, and only the per-run regeneration changed.
…al error Running the harness caught both of these. prepare_r2 never seeded anything: `ncli` rejects an --events path whose extension isn't .json/.jsonp/.yaml/.yml, and plain mktemp produces /tmp/tmp.XXXXXXXXXX with none. R2's verifier duly reported "expected 8 seeded event(s), got 0". The earlier fixed paths happened to end in .json, so switching them to mktemp for the concurrency fix broke seeding -- mktemp --suffix=.json keeps both properties. prepare_r6's exit-status check couldn't see the failure it exists for: the `script -qec ... || true` on the next line means a failed `ncli bunker` still exits 0, so only `ncli id` failures were ever detectable. The real error -- an "invalid MAC" vault decryption failure -- went only to the TTY log and nothing printed it. The readiness-timeout path now tails that log.
8 tasks
Contributor
Author
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.
Why
TestNegSync_Integrationreconciled againstwss://relay.ohstr.com. Thathost now returns a Cloudflare 404, so the websocket upgrade fails and the
test can't tell an outage apart from a real regression. Egress is fine —
nos.lolandrelay.primal.netboth still serve events.While in there, reviewed
integration/agent-evaland fixed the confirmeddefects, including its own public-relay dependency.
Negentropy, hermetically
The sync stack runs the same relay config twice (
remote45520,remote245521). The new
NegentropyPropagatesBetweenRelayInstancessubtest seedsthe first instance, reconciles it down into a temp local store, then pushes
that store up into the empty second one, asserting every seeded ID arrives.
It checks the second relay is empty first, so the assertion can't pass on
pre-existing data.
Relays only answer NEG-OPEN — they never initiate reconciliation with
each other — so the sync module runs twice against one local store rather
than the two relays talking directly.
TestNegSync_Integrationis deleted and dropped fromjust test-integration.agent-eval
to a flat path under
report/and only copied into the run dirafterwards; nothing cleared them first. Now cleared before each round,
from the round loop rather than
run_round()—run_r6backgroundsrun_roundand polls forr6-bunker-uri.txt, so clearing inside thebackground job would race that poll onto a stale URI.
.envwas written only when missing and never removed, pinning one vaultpassword across every run despite being documented as per-run.
prepare_r6discarded its own output, so a failed identity step surfacedonly as a generic "daemon did not come up" warning 15s later.
r8-error-contract.sh's four probes shared fixed/tmppaths; itsset +ewas dead, since onlyset -uo pipefailis ever set.report/was made world-writable when the container user just needs to own it.relay.ohstr.com— it queries the stack's ownrelay, seeded by a new
prepare_r2viancli id sign+ncli publish.No CI job or justfile recipe was added for agent-eval: every round is a
real, billed Claude Code session, so it stays manual by design.
Verification
just check— vet + unit suite, green.just test-integrations— stream + inspect + sync, 12 subtests, exit 0,including the new one.
(6/6 published,
findreturns them,pingreports reachable).bash -n); agent-eval was not runend-to-end, since that bills real LLM calls.
Also fixes two stale docs claims found on the way: the
local-verifyskillsaid there's no
ncli publishcommand (there is), and thatrelay.ohstr.comis reachable from this sandbox (it 404s).