test: adopt the hermetic negentropy + agent-eval fixes onto main - #78
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.
Adopts the work from PR #64 onto current main so it can go into 0.7.0-rc.1. Verified against main first: its four agent-eval defects are all still present, though its stated trigger for the negentropy change -- relay.ohstr.com returning a Cloudflare 404 -- has since resolved and that test passes again. The change is still right, because a test against a public relay cannot tell an outage from a regression. Its ports needed moving. It was written before the integration stacks moved below Linux's ephemeral floor (32768), so it added remote2 on 45521. The merge combined that with main's 21520 and produced a stack straddling both ranges, which would have quietly reintroduced the random "address already in use" failure the move removed -- on that one new service. remote2 is now 21521, and every reference in compose.yaml, relay.yaml, the READMEs, the justfile and the test moved with it. Checked afterwards: 48 published ports across all stacks, all unique, none at or above 32768. Conflicts otherwise resolved toward main: the 0.7.0-rc.1 release notes are untouched, since this is test and eval infrastructure and earns no entry, and the port-range list keeps the ephemeral-floor note while widening sync to 21520-21521. What it brings: a 100-event fixture so unit tests stop fetching from public relays, two local relay instances covering negentropy propagation instead of one public relay, deletion of the last two public-relay tests, and fixes for agent-eval rounds reading stale data, .env pinning one vault password across runs, a discarded error path, four probes sharing fixed /tmp paths, and a world-writable report directory.
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.
Supersedes #64, which was based on the tree before the integration ports moved. Same commits, rebased onto current main with the port range fixed, so it can go into 0.7.0-rc.1.
Why it still matters
Verified against main before adopting: the agent-eval defects are all still present. Its stated trigger for the negentropy change ??? relay.ohstr.com returning a Cloudflare 404 ??? has since resolved, and
TestNegSync_Integrationpasses again. The change is still right: a test against a public relay cannot tell an outage from a regression.The port problem this fixes
#64 predates #68, so it added
remote2on 45521. Merging it naively produced a stack straddling both ranges ???remoteon 21520 andremote2on 45521 ??? which would have quietly reintroduced the randomaddress already in usefailure #68 removed, on that one new service.remote2is now 21521, with every reference moved:compose.yaml,relay.yaml, both READMEs, the justfile and the test.Checked after: 48 published ports across all stacks, all unique, none at or above 32768.
Conflicts resolved toward main
sync 21520-21521.Test plan
go build ./...,go vet ./...,gofmt -l .clean;go mod tidya no-opgolangci-lint run --max-same-issues=0 --max-issues-per-linter=0: 0 issuesgovulncheck ./...: 0 affectinggo test -short -race ./...: zero failuresbash -nclient/sync_integration_test.gois byte-identical to test(sync): hermetic negentropy coverage + agent-eval fixes #64's apart from the port moveintegration/README.md's documented caveat), so theintegrationsjob is the authoritative run of the newNegentropyPropagatesBetweenRelayInstancessubtest