next #136
Reference in New Issue
Block a user
Delete Branch "next"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Long-lived integration branch. One commit per work unit lands here; this PR accumulates them until it is merged to
main.Landed units
Run all linting in Docker via
Dockerfile.lint+script/lint— #134golangci-lint is no longer installed or run on the host. New root
Dockerfile.lintCOPYs the repo into the digest-pinnedgolangci/golangci-lint:v2.12.2image and lints as a build step, so a successful build IS a clean lint;script/lintis reduced to a thin wrapper that builds it. This also works where the docker daemon is remote and bind mounts are impossible.Pinned digest and how it was verified.
golangci/golangci-lint:v2.12.2@sha256:5cceeef04e53efe1470638d4b4b4f5ceefd574955ab3941b2d9a68a8c9ad5240, exactly as quoted in the issue. It resolves, and it is genuinely v2.12.2:The tag's index digest is the quoted digest, and the binary inside reports commit
c0d3ddc9, matching the org's canonical pinc0d3ddc9cf3faa61a4e378e879ece580256d76e5.Forcing the linter to actually run.
Dockerfile.lintis split into adepsstage (base image +go mod download) and alintstage (source copy + linter run).script/lintruns:Caching is explicitly waived for linting, and a cached build lints nothing, so the
lintstage is invalidated on every invocation. The invalidation is scoped: thedepsstage stays cached and no global cache wipe is performed.--progress=plainkeeps the linter's own output visible.golangci-lint config verify: deliberately NOT included. It fetches its JSON schema over a live, unpinned HTTPS call, which would make linting network-dependent and defeat hash-pinning. Omitted for that reason, and the reason is recorded in a comment at the top ofDockerfile.lint.script/bootstrapno longer installs golangci-lint (and its pinned ref is gone); it warns non-fatally whendockeris absent instead. Thegoimportsinstall stays, becausescript/fmtandscript/fmt-checkstill run on the host. Header comment updated accordingly.Root
Dockerfile— required consequence, not scope creep. Its builder stage ranmake check, which now callsscript/lint, which shells out todocker build; there is no docker daemon inside a docker build, soscript/cibuildandscript/dockerwould have broken. It gains its own lint stage on the same pinned image (linter invoked directly, with a comment explaining why notmake lint), with the builder stage depending on it viaCOPY --from=lint /src/go.sum /dev/nulland runningmake fmt-check,make test,make build. The now-unneeded golangci-lint install is gone from the builder stage.README
EntrypointsandBuildingsections now describe linting as a docker-only operation.TODO.mdupdated in the same commit.Verification
All runs via
make/script/entrypoints only.Two consecutive
make lintruns on an unchanged tree, both executing the linter:A third run shows the cache scoping is working as intended —
depsserved from cache,lintre-executed:Negative control. A deliberate violation (an unused function containing an ineffectual assignment) was added to
internal/config/config.go:make lintexited non-zero naming both findings and their exact lines. After reverting the file,make lintwas clean again (0 issues.).make checkgreen end to end (test, lint, fmt-check), exit 0.script/cibuildgreen, confirming theDockerfilerestructure does not recurse: the lint stage ran (#16 12.56 0 issues.), then#22 [builder 8/9] RUN make testwithPASSlines, then#23 [builder 9/9] RUN make build.Notes for the owner
This supersedes two PRs you still have queued for merge, both of which tune host linting that no longer exists after this change: #128 (isolates the host golangci-lint cache and lock) and #131 (always installs the pinned lint tools in
script/bootstrap). Neither was merged or incorporated here.Made moot by this change: #121 and #130.
Review outcome
Reviewed at #136 (comment) — PASS. Three non-blocking comment-accuracy findings were recorded there for the next touch of those files; the reviewer independently confirmed the lint gate is live by negative control from a warm cache.
Live DNS tests made robust instead of gated; test caps moved to the org-wide 60s/20s/90s values — #93
The resolver's live-DNS tests failed nondeterministically, a different subset each run. Fixed by engineering the nondeterminism out, not by routing around the network. Nothing is mocked, faked, stubbed, recorded or replayed; there is no
-shortflag, no build tag, no skip, and no environment-tolerance for restricted egress. Production resolver behaviour is unchanged.Root causes, all test-side
internal/resolvercallst.Parallel()and the build hosts have many cores (48 here), so all ~35 iterative resolutions started within milliseconds of each other, and becausequeryServerswalksrootServerList()in fixed order they all aimed their first query at198.41.0.4. Root servers rate-limit that, which fits the reported symptom of a different arbitrary subset failing each run.TestQueryAllNameservers_AllReturnOKand_NXDomainFromAllNSrequired every nameserver of a domain to answer — four independent chances to fail per run, with no tolerance for one being slow.What was built
New
internal/resolver/livedns_test.goholds all the live-DNS machinery, soresolver_test.goitself takes only call-site edits:liveConcurrency = 6) caps how many live resolutions are in flight at once. Tests keept.Parallel(); only their network work is throttled. This is the direct fix for cause 1, and the 60s budget is what makes it affordable.ok/timeout/errorfor the all-OK test,nxdomain/timeout/errorfor the NXDOMAIN test. A nameserver that stays silent is tolerated; one that answers wrongly is not, at any count. The allowlist is the load-bearing part — see the rework note below for why a blocklist was not enough.New
internal/resolver/livedns_harness_test.gotests that machinery directly — quorum arithmetic, status counting, the allowlist, the gate's concurrency bound, per-attempt deadlines, and recovery from a transient failure. It performs no DNS resolution of any kind, so it neither mocks DNS nor depends on it.Rework after review — the quorum could not fail on a wrong answer
The review at #136 (comment) returned FAIL on
9cb2c2b, correctly. Fixed in87bce43.The defect. The claim above was, as first written, false for
resolver.StatusNoData. Each test banned exactly one wrong status —_AllReturnOKbanned onlynxdomain,_NXDomainFromAllNSbanned onlyok— andnodatais neither. It is a wrong answer, not silence:answeredCountcounted it as answered, so it did not even trigger a retry, and with a quorum of 3-of-4 a single wrong nameserver slid through undetected. The pre-change unanimity assertions would have caught it. That is robustness work quietly becoming assertion-loosening, which is exactly what this repo cannot afford.The fix. Tolerance is now an allowlist, not a blocklist of one status. New
unsanctionedStatuses()returns every per-nameserver result whose status the caller did not explicitly sanction, and each test asserts that list is empty in addition to its quorum. A blocklist bans the one wrong answer its author thought of and silently admits everything else, including any status added to the resolver later; an allowlist fails on anything nobody sanctioned.answeredCountwas reframed the same way — it now counts the closed setok/nxdomain/nodata, so an unfamiliar status is treated as silence and can only ever cause a retry and then a loud failure, never a quiet pass.Evidence — the reviewer's exact probe, re-run.
queryEachNSininternal/resolver/iterative.gowas patched to force one ofgoogle.com's four nameservers to returnStatusNoDatawith empty records.make testnow goes red, naming the offending nameserver and status:That is the same input that returned
exit=0with both tests passing under the old assertions. Probe reverted, tree clean, suite green again:Two harness tests lock the regression in without any probe: three OK plus one
nodata(quorum satisfied, no NXDOMAIN present — the exact input that used to pass) is reported as unsanctioned, and an unknown status is neither counted as answered nor tolerated.Also fixed from the review: the per-attempt deadline assertion in
livedns_harness_test.gohad no lower bound, so it passed for a deadline far shorter than intended. It now asserts the remaining time exceedsliveAttemptTimeout/2as well.Deliberately not done in this rework, per the review and the owner: no
-count=1inscript/test(the test-cache issue is real but pre-existing and repo-wide, filed separately); the remaining non-DNS mocks stay for #97;queryServersroot-ordering stays untouched under #138.The
-timeoutbackstop value: 90sPer the ruling at sneak/prompts#41 (comment) the cap is org-wide with two tiers: 60s hard cap for CI green, 20s target, and anything between the two must be filed as an improvement bug. The backstop is
90s, matching sneak/prompts#42 and preserving the 1.5x backstop-to-cap ratio the old 20s/30s pair had. It must strictly exceed the 60s cap or the cap is unreachable — the old-timeout 30swould have killed a 60s-capped suite at half its allowance. Applied toscript/test; nothing else in the repo carried the old30s.Worst case for one live operation is 3 attempts x 8s plus ~1.5s of backoff, about 26s — comfortably inside the 90s backstop even if several operations exhaust their attempts at once.
REPO_POLICIES.mdis re-vendored, not hand-editedThe file is org-canonical, so it was copied byte-for-byte from
prompts/REPO_POLICIES.mdonsneak/promptsbranchorg-wide-60s-test-cap(commit52b5192) rather than reworded to approximately the same thing. Verified:What that byte-identity does and does not certify. The source branch
org-wide-60s-test-capis an unmerged proposal — sneak/prompts#42 — notpromptsmain. So, precisely:90sbackstop is our own proposed number and is NOT ratified (sneak/prompts#41 (comment)).Known mismatch with this repo's actual state, recorded not papered over. Re-vendoring also picked up the paragraph at
REPO_POLICIES.md:266-271mandating that canonical golangci-lint be installed commit-pinned viago install ...@c0d3ddc9.... This repo does not comply with that mechanism:cc86473in this same PR made linting Docker-only, andscript/bootstrapnow installs golangci-lint nowhere. The version and commit match (v2.12.2/c0d3ddc9); the installation mechanism does not. The vendored file is org-canonical and must not be edited downstream, so this is being raised upstream for the org text to accommodate Docker-only linting rather than patched here.TESTING.md's stale "within the 30-second target" follows to 60. That edit is deliberately a single line so it merges cleanly when #97 lands.Verification
All runs through
make/script/entrypoints only; lint runs in Docker.Ten consecutive
make checkruns, all green, none served from cache. Go's test cache will happily reportok pkg (cached)without executing anything, which proves nothing about nondeterminism, so every run was forced to actually execute and each log was checked for zero(cached)lines:After the rework commit
87bce43,make checkgreen again end to end, zero(cached)test lines, Docker lint stage demonstrably executed rather than served from cache:The Docker lint stage was confirmed to execute rather than cache on each run:
make testwall time: 3.6-4.1s across three timed uncached runs (4093ms,3649ms,3729ms).internal/resolverwent from 2.0s to ~2.9s — the concurrency gate's cost. That is inside the 20s target, so no improvement bug is owed under the new two-tier rule.Honest note on what these runs do and do not prove. Live DNS was healthy throughout: no live-DNS retry fired even once, and no flake was observed either before or after the change (six pre-change baseline runs were also clean). So these runs demonstrate the change is not itself flaky and does not slow the suite; they do not demonstrate recovery from a real DNS failure, because no real DNS failure occurred. The original flakiness is not reproduced rather than shown fixed. The retry path is instead proven by
TestRetryLiveRecoversFromTransientFailure, the only source of the singleretrying in 500msline in each log:Observation, not acted on
The single most effective remaining lever against root-server rate limiting would be to stop
queryServersalways tryingrootServerList()in the same order, so that load spreads across all thirteen roots instead of concentrating ona.root-servers.net. That is production code and this issue scopes the work as test-side, so it was left alone rather than changed quietly. It is now tracked for the owner's decision at #138.Interaction with #97
TESTING.mdandinternal/resolver/resolver_test.goauto-merge — that PR touchesresolver_test.goonly at the import block and the final timeout-test section, while this change touches the body of the file and adds two new files, and it leaves the mock-DNSClienttimeout test at the tail ofresolver_test.goentirely alone since removing it is that PR's job.TODO.mddoes conflict; that PR is already labelledneeds-rebase, so this adds nothing material to its rebase.Go's test cache disabled, so every
make testactually queries live DNS — #139script/testdid not pass-count=1, so on an unchanged tree Go served the whole suite from cache: exit 0 in ~0.2s, every package marked(cached), and not one DNS query made. This repo's suite exists to exercise live resolution on every run (TESTING.md), so that green asserted nothing — and it is exactly the green used as evidence that a flakiness fix works, since "run it a few times" stops being runs after the first. It had already misled two agents, each of whom forced uncached runs by hand.-count=1now disables caching on every invocation.The conditional verbose rerun was missing and is added here.
REPO_POLICIES.mdmandates it; the primary run had been unconditionally-v, which is the failure mode the policy exists to prevent (unreadable CI anddocker buildlogs on success). Tests now run quiet, and only a failure triggers the-vrerun. Two properties matter and both are covered: the rerun carries-count=1too, so it cannot replay a cached copy of the failure it is meant to diagnose; and its exit status is discarded in favour of a forced1, so a flake that passes the second time cannot turn the build green — the first failure already proved the suite broken.-timeout 90suntouched. It is a deliberate backstop that must strictly exceed the 60s hard cap.No special-casing for the Docker build, which reaches the same script via
RUN make test: a fresh container's test cache is empty, so-count=1is a no-op there, and carving out an exception would only create a second code path that could drift.Verification
Proven from a warm cache, not a cold one. The suite was run first to populate the cache, and the pre-change state confirmed:
With the change applied to that same warm cache, three back-to-back runs on an unchanged tree, zero
(cached)markers in all three:Measured uncached wall time: 4.0-4.5s (was ~0.2s served from cache). Inside the 20s target, so no improvement bug is owed under the two-tier rule at sneak/prompts#41 (comment).
-count=1composes with-raceand-cover: both still present in the primary run, and the per-package coverage percentages above are identical to the pre-change values.The failure path was exercised, not assumed. A purpose-built flaky test that fails on its first run and passes every run after (marker file kept outside the module, so the tree stays byte-identical and a cached result would be served if caching were on) was run through the script:
Quiet failure, verbose rerun that genuinely re-executed (it passed, so it did not replay the cached
FAIL), and exit1regardless of the rerun passing. Scratch module removed afterwards.make checkgreen, exit 0, with the Docker lint stage demonstrably executed rather than served from cache:README.mdandTESTING.mdrecord why the cache is waived, andTODO.mdis updated in the same commit.Question for the owner, not filed as a defect
The Docker lint run emits
The linter 'gomodguard' is deprecated (since v2.12.0) due to: new major version. Replaced by gomodguard_v2.It is pre-existing and out of this issue's scope. It is not filed as an issue here because.golangci.ymltracks the org-canonical config, so switching togomodguard_v2looks like an upstreamsneak/promptsdecision rather than a per-repo fix. Say the word and it gets filed in whichever place you consider canonical.MIT
LICENSEadded; README states the licence — #102The repo had no licence file at all, so publicly readable code was all-rights-reserved by default and nobody could legally use it.
LICENSEwas also the last file missing fromREPO_POLICIES.md's required minimum.MIT, by standing org policy rather than a per-repo call: any public repo lacking a licence gets MIT, and a private one with no licence is already all-rights-reserved.
sneak/dnswatcheris public (private: falseon the Gitea repo record).README.md's first line now names the licence, per the Description requirement, and the License section states MIT and points at the file instead of recording the decision as pending.TODO.mdupdated in the same commit.Verification
LICENSEis the canonical MIT text byte-for-byte with only the copyright line filled in (Copyright (c) 2026 sneak) — no clauses added, removed, reworded, or reflowed. It was not typed from memory: the file was copied from an existing verbatim MIT template on disk and only the copyright line edited (diffagainst that template shows that one line and nothing else), then the result was word-diffed against SPDXMIT.txtfetched fromspdx/license-list-data, ignoring only line wrapping and the placeholder — identical.make fmtdid not touchLICENSE, and cannot:script/fmtrunsgofmt -s -w .andgoimports -w .only, with no prettier or markdown step in the repo, so no exclusion was needed.make checkgreen, exit 0. Tests executed rather than replayed (zero(cached)lines,internal/resolver 3.098s), and the Docker lint stage ran rather than cached:Comment-only corrections in
script/bootstrap,script/cibuild, andDockerfile.lint— #137Follow-up to this PR's own review. Nothing executable changed: the diff touches comment lines, one warning string, and
TODO.md.script/bootstrap's header justified the pinnedgoimportsinstall by claimingscript/fmt-checkruns it on the host. Verified against the script:script/fmt-checkrunsgofmt -l .and nothing else. The header now creditsscript/fmtalone. Thatfmt-checkdoes not verify goimports at all is #119 and was deliberately left alone.script/cibuild's header still said theDockerfilerunsmake check. It now describes the current file: lint stage runsmake fmt-checkandgolangci-lint, builder stage runsmake testandmake build.docker-missing warning was three fragments, each re-prefixed withbootstrap:mid-clause. Now one sentence:bootstrap: WARNING: docker not found; install it to run make lint and make docker.Dockerfile.lint's comment explained whygolangci-lint config verifyis omitted but read as though the omission were free. It now states the residual risk: unknown top-level keys in.golangci.ymlare silently ignored, so a mistyped or wrong-schema key lints clean while applying nothing.config verifywas not added — the network-dependence reasoning stands.Verification
make checkgreen, exit 0. Tests executed rather than replayed (zero(cached)lines,internal/resolver 2.820s), Docker lint stage executed rather than cached:Comment-only confirmed by reading the whole diff: no statement, flag, or command changed anywhere.
Adversarial review —
cc86473(#134)Reviewed in an independent clone at
cc86473. All runs viamake/script/entrypoints. No global cache invalidation performed.Primary target: is the
Dockerfilelint gate live or inert?Live. Stated plainly because the raw evidence looks exactly like a false green:
script/cibuildon an unchanged tree from a warm cache returns exit 0 in 0.63s with all 18 layersCACHED, lint stage included — the linter does not re-run. That is not the defect tracked at #115 and #124, because the sourceCOPY . .is inside the lint stage's cache key. Proved by negative control from that same warm cache: appended an unused func with an ineffectual assignment tointernal/config/config.go, re-ranscript/cibuild— failed in 16.6s, exit 1, at#16 [lint 7/7] RUN golangci-lint run, naminginternal/config/config.go:219:2: ineffectual assignment to x (ineffassign)and:218:6: func ... is unused (unused). Reverted; clean.COPY --from=lint /src/go.sum /dev/null— anomaly that passes. Tested rather than reasoned about, with a throwaway probe Dockerfile: BuildKit bind-mounts a real/devfor everyRUN, so/dev/nullstats ascharacter special file size=0both before and after the COPY; writes discard, reads return empty,catfrom it yields nothing. The go.sum bytes land in the builder layer's own filesystem but are masked at build-RUN time and at container runtime, andbuilderis not the final image. No hazard. It is also verbatim the patternREPO_POLICIES.mdmandates.Verified and passing
script/lintruns on an untouched tree both executed the linter (0 issues.at 16.1s and 22.6s of real linter work);--no-cache-filter=lintscoping confirmed —depsstagesCACHEDin every run, nothing else invalidated.Dockerfile.lintpath too:make lintexit 2 naming both findings at exact lines; reverted clean.internal/config/config_test.goand confirming it was reported (config_test.go:265:2), so test files are linted..dockerignoreexcludes only.git/,bin/,*.md,LICENSE,.editorconfig,.gitignore;.golangci.ymland all Go source reach the container.Dockerfile,.gitea/workflows/check.yml, README — only docker-mediated references.goimportsinstall correctly retained inscript/bootstrap.docker buildx imagetools inspect golangci/golangci-lint:v2.12.2reports index digestsha256:5cceeef0…ad5240; the binary at that digest reports2.12.2 … from c0d3ddc9.make checkgreen end to end, exit 0, zero(cached)markers — every test package actually executed.make fmtleaves the tree clean. Mergeable:nextismain+ 1 commit, fast-forwardable. CI statussuccessoncc86473.(closes #134); no attribution trailers; no Claude/Anthropic reference anywhere in tree, commit message, or PR body. Naming/idiom consistent; no non-inclusive terminology.Dockerfilerestructure is in scope (the builder'smake checkwould have recursed intodocker buildwith no daemon) and matches the canonical shape inREPO_POLICIES.md. Invoking the linter directly instead ofmake lintin that stage is the correct deviation and is commented.Findings (non-blocking, fix on next touch)
script/bootstrapline 7-8 — new comment states a falsehood. "goimports is installed … because script/fmt and script/fmt-check run it on the host."script/fmt-checkruns onlygofmt -l .; it never invokes goimports. Onlyscript/fmtdoes. Same wrong claim is repeated in the commit message and PR body. The decision to keep goimports is correct; the stated reason is half wrong. Acceptable: name onlyscript/fmt.script/cibuildline 2-3 — comment left stale by this change. It still says "The Dockerfile runs make check, so a successful build implies all checks pass." After this commit theDockerfilerunsmake fmt-check+golangci-lintin the lint stage andmake test+make buildin the builder;make checkappears nowhere in it. The implication still holds in substance, but the stated mechanism is now false. Every other doc describing the old arrangement was updated (README,TODO.md,script/bootstrapheader); this one was missed. Acceptable: reword to name the stages the build actually runs.script/bootstrapdocker warning is wrapped mid-clause, emittingbootstrap: WARNING: docker not found; make lint and/bootstrap: make docker require it. Install docker to/bootstrap: run the linter.Thebootstrap:prefix repeated inside a broken sentence reads badly. Cosmetic.Declared deviation —
golangci-lint config verifyomittedThe stated reason is factually correct and the omission is defensible;
.golangci.ymlis a frozen org-standard file that agents must not modify (REPO_POLICIES.mdline 261), so drift risk is low. But something real is lost, and the PR write-up does not acknowledge it. Probed empirically: appending an unknown top-level key (bogus_top_level_key_r136:) to.golangci.ymland runningmake lintgives exit 0,0 issues.—golangci-lint runsilently ignores it. So config typos and obsolete keys now pass unnoticed. This repo has already been bitten by exactly that class of bug: the comment at.golangci.ymllines 3-5 exists because v1-schemalinters-settingswas being silently ignored under v2. Recommend recording the residual risk in theDockerfile.lintcomment (it currently reads as though nothing is given up) and, if wanted, a follow-up issue for a pinned/offline schema. Not a blocker — the issue explicitly delegated this decision and it was made deliberately and documented.Judgement calls and things I could not verify — disclosed
cc86473(HTTP 403:user should be the owner of the repo), so I could not confirm the CI runner's lint stage executed rather than being served from the runner's cache. I have only the commit status (success, 1m17s). Mitigated by my own warm-cache negative control through theDockerfilepath failing correctly; I did not independently establish CI-side execution.script/lintwaives caching, but theDockerfilelint stage does not.script/cibuild/script/dockerwill therefore reuse a previous lint result for byte-identical source. Since.dockerignoreexcludes*.md, a markdown-only change yields acibuildgreen whose lint stage never ran — harmless, as markdown is not linted, and CI runners are cold.script/precommitcallsscript/checkcallsscript/lint, so every commit now requires a docker daemon and costs ~35s of deliberately uncached linting. That is the direct consequence of the owner's iron rule, not a flaw in this change.script/bootstrapnon-fatal docker warning: I judge it correct. Bootstrap cannot sensibly install a daemon, and everything exceptmake lint/make dockerworks without one; the later failure is a plaindocker: command not found, not a confusing one.make fmtdoes not cover markdown in this repo (no prettier target or config), so items 13's formatting check was done by inspection: the README andTODO.mdadditions wrap consistently with surrounding prose and put verbatim identifiers in backticks.cc86473.Verdict: PASS
Adversarial review — unit
9cb2c2bonly ("test: make live DNS tests robust instead of gated (closes #93)", #93).cc86473was reviewed separately and is not re-reviewed here.Verdict: FAIL —
needs-reworkOne defect, demonstrated empirically. Everything else passes; the fix is one assertion.
Finding 1 (defect) — a minority of nameservers may answer WRONGLY and the suite stays green
internal/resolver/resolver_test.go:310-335(TestQueryAllNameservers_AllReturnOK),:337-364(TestQueryAllNameservers_NXDomainFromAllNS),internal/resolver/livedns_test.go:177-183(answeredCount).The commit message, the PR body and the code comments all state the invariant as: "A nameserver that stays silent is tolerated; one that answers wrongly is not." The code does not implement that. Each test bans exactly one wrong status and ignores the rest:
_AllReturnOKrequires a quorum ofokand assertscountStatus(..., StatusNXDomain) == 0._NXDomainFromAllNSrequires a quorum ofnxdomainand assertscountStatus(..., StatusOK) == 0.resolver.StatusNoDatais neither. It is a genuine wrong answer forgoogle.com(the server answered, with no records), it is not silence, andansweredCountcounts it as answered — so it does not even trigger a retry. With 4 nameservers the quorum is 3, so onenodataserver passes both assertions.Demonstrated. I patched
queryEachNSininternal/resolver/iterative.goto force exactly one of the fourgoogle.comnameservers to returnStatusNoDatawith empty records, then ranmake test(forced uncached):Whole suite green. Before this commit both tests required unanimity and would have gone red. That is precisely the "robustness became assertion-loosening" failure mode, and it is a regression in detection power beyond what the quorum change requires. The probe edit was reverted; the tree is clean.
Acceptable looks like: tolerate only non-answers, not wrong answers. E.g. in
_AllReturnOKassert that every result isok,timeoutorerror(countStatus(ok) + countStatus(timeout) + countStatus(error) == len(results)) in addition to the OK quorum, and symmetrically in_NXDomainFromAllNS(nxdomain/timeout/erroronly). One assertion each.Anomalies (not blocking, but must be resolved or recorded)
2. The freshly re-vendored
REPO_POLICIES.mdnow contradicts this repo's actual state. Re-vendoring pulled in the new sentence atREPO_POLICIES.md:266-271stating that canonical golangci-lint is "installed commit-pinned viago install ...@c0d3ddc9...". As ofcc86473in this same PR, this repo installs golangci-lint nowhere —script/bootstrap:8-10says so explicitly and linting is Docker-only. The PR body's claim that this repo "already complies with [that paragraph] in practice since #96" is therefore inaccurate: the version and commit match (v2.12.2/c0d3ddc9), the installation mechanism does not. The vendored file must not be hand-edited, so this belongs upstream — please have the org text accommodate Docker-only linting, or record the divergence.3. The vendored text comes from an UNMERGED branch and is described as canonical. Byte-identity independently verified:
prompts/REPO_POLICIES.mdatsneak/prompts52b5192(branchorg-wide-60s-test-cap) has sha256bcf11c312a1bee18a0e937eb412b51914411c1ab23308b8362409f3f88379ff7, identical to this repo's copy; the repo-rootREPO_POLICIES.mdthere is a symlink to it, so there is a single lineage. But that branch is only proposed at sneak/prompts#42 and is not onpromptsmain. The 60s/20s tiers are ruled by sneak; the90sbackstop is explicitly the proposer's own unratified suggestion (sneak/prompts#41 (comment)). The PR body's "This is not a divergence — it is the new canonical text" overstates that. If sneak/prompts#42 lands with a different number, this repo silently carries non-canonical text asserted as canonical.4.
make testis served from Go's test cache on an unchanged tree — pre-existing, not introduced here, but it undercuts this unit's central guarantee.script/testhas no-count=1, so my first three back-to-backmake testruns came back exit 0 withok .../internal/resolver (cached)and 8 cached packages, executing no DNS at all. For a suite whose entire value is live resolution, a repeatmake checkproves nothing. Worth a separate issue.5. Mocks still exist in the repo after this "closes #93" commit.
internal/resolver/resolver_test.go:517-556still carriestimeoutClient(a fakeDNSClientinjected viaresolver.NewFromLoggerWithClient), andinternal/watcher/watcher_test.gostill carriesmockResolver. This commit correctly leaves them alone — removing them is the job of #97 — but closing #93 does not leave the repo free of DNS mocks, and item 5 of that issue's DoD reads as though it should. Flagging so it is not assumed done.6. Nit —
internal/resolver/livedns_harness_test.go:103-115asserts only that the per-attempt deadline is<= liveAttemptTimeout. That passes for any deadline, including an absurdly short one. A lower bound (e.g.> liveAttemptTimeout/2) would make it meaningful.7. The claimed clean interaction with #97 holds for
TESTING.mdandinternal/resolver/resolver_test.go(both auto-merge), butTODO.mdconflicts. That PR is alreadyneeds-rebase, so this adds nothing material — noting only that the PR body says "no collision found".Verified and passing
livedns_harness_test.gois not a disguised DNS mock. It builds no DNS message, starts no server, implements noDNSClient, and invokes no resolver method; the syntheticNameserverResponsemap at:43-60is passed only to the test-local pure counters (countStatus,answeredCount,describeStatuses). Nothing is substituted for DNS in any test that exercises DNS. Called out honestly as borderline: those same counters do consume real answers in the live tests, so the harness does synthesise values that normally come from live DNS — but it stops short of a fake DNS layer, because no resolution path is fed. Legitimate.classifyResponseininternal/resolver/iterative.goso NXDOMAIN classified asStatusOK;make testwent red in 3s withTestQueryNameserver_NXDomain(1.45s) andTestQueryAllNameservers_NXDomainFromAllNS(1.12s) failing, and zero live retries logged. A wrong answer fails on attempt 1 and is never retried into a green. Reverted.-short, no build tags, no skips, no env opt-outs, no egress tolerance anywhere in the diff.script/testonly.-raceretained inscript/test; all tests keept.Parallel(); the semaphore of 6 did not mask anything (race detector clean).make checkrun by me: exit 0 in 32s, Docker lint stage demonstrably executed (#12 [lint 2/2] RUN golangci-lint ... 0 issues. DONE 16.9s,depsCACHED,lintnot), zero(cached)test lines.make testforced uncached, 3 runs: 4s/6s/4s wall,internal/resolver2.88s/4.14s/4.47s. Inside the 20s target — no improvement bug owed. Confirms the reported 3.6-4.1s.-timeout 90sinscript/test; no stale30stimeout anywhere in the tree (Makefile, workflow,Dockerfile, docs all clean).TESTING.mdedit is the single line claimed.9cb2c2b;nextis fast-forwardable ontomainwith no conflicts.(closes #93); no attribution trailers; no unwanted vendor/product references anywhere in the tree.make fmt-checkclean; inclusive terminology clean; naming and idiom consistent, no stutter.Disclosed limits of this review
retrying in 500msline per run comes fromTestRetryLiveRecoversFromTransientFailure). Recovery from a real DNS failure is therefore evidenced only synthetically, and the original flakiness has been not reproduced rather than shown fixed. The burst-fan-out root-cause analysis is plausible and the gate is a sound mitigation, but this PR cannot claim to have demonstrated the cure. Not held against it.make testwithGOFLAGS=-count=1in the environment (see anomaly 4). No repo file was changed for that. The two deliberate-break probes editedinternal/resolver/iterative.goand were reverted;git statusis clean andnextis untouched.Rework of the #93 unit against the review at #136 (comment), pushed as
87bce43onnext. Point by point.Finding 1 (defect) — a minority may answer WRONGLY and the suite stays green. Accepted in full; fixed.
The finding is correct and the claim in the commit message, the PR body and the code comments was false for
resolver.StatusNoData.nodatais a wrong answer, not silence;answeredCountcounted it as answered so it did not even trigger a retry; and with a 3-of-4 quorum one wrong nameserver passed both tests. Detection power was lost beyond what the quorum change required.Fixed as an allowlist, not by adding
nodatato the two blocklists — a blocklist bans the one status its author thought of and silently admits every status added later, which is the same defect wearing a different hat:unsanctionedStatuses(results, allowed...)ininternal/resolver/livedns_test.goreturnsnameserver=statusfor every result whose status the caller did not explicitly sanction.TestQueryAllNameservers_AllReturnOKnow asserts that list is empty for the sanctioned setok/timeout/error, in addition to its OK quorum.TestQueryAllNameservers_NXDomainFromAllNSdoes the same fornxdomain/timeout/error.answeredCountwas reframed the same way: it counts the closed setok/nxdomain/nodatainstead of subtractingerror+timeout. An unfamiliar status now counts as silence, so it can only ever cause a retry and then a loud failure, never a quiet pass.Silence (
timeout,error) remains the only thing quorum tolerates. A wrong answer fails at any count.Your probe, re-run against the reworked tree.
queryEachNSininternal/resolver/iterative.gopatched to force one ofgoogle.com's four nameservers to returnStatusNoDatawith empty records,make testforced uncached:Same input, same probe, that previously gave
exit=0with both tests passing. Probe reverted, tree clean, green again:zero
(cached)lines in that run. Two new harness tests pin the regression without needing any probe:TestUnsanctionedStatusesRejectsWrongAnswersfeeds exactly the passing input you identified (threeokplus onenodata, quorum satisfied, no NXDOMAIN present) and asserts it is reported as unsanctioned;TestUnsanctionedStatusesToleratesSilenceOnlyasserts an unknown status is neither counted as answered nor tolerated.Anomaly 2 — vendored
REPO_POLICIES.mdcontradicts this repo's state. Accepted; recorded, not papered over.You are right that the version and commit match while the installation mechanism does not:
cc86473in this PR made linting Docker-only andscript/bootstrapinstalls golangci-lint nowhere, so "already complies in practice" was inaccurate. The claim is gone from the PR body and replaced with a plain statement of the mismatch. The vendored file is org-canonical and has not been hand-edited; it is being raised upstream so the org text can accommodate Docker-only linting.Anomaly 3 — vendored text described as canonical when its source branch is unmerged. Accepted; PR body reworded.
"This is not a divergence — it is the new canonical text" overstated it and is removed. The body now says plainly: vendored byte-for-byte from the proposed canonical branch
org-wide-60s-test-cap(sneak/prompts#42, unmerged); the 60s cap and 20s improvement-bug tier ARE the owner's ruling; the90sbackstop is our own proposed number and is not ratified; if that PR lands with different values this file gets re-vendored, not hand-edited. The byte-identity claim stands, and you confirmed it independently.Anomaly 4 —
make testserved from Go's test cache. Agreed, and deliberately not touched here.Real and it does undercut repeat-green evidence, but pre-existing and repo-wide, so it is being filed separately rather than fixed drive-by in this unit. No
-count=1was added toscript/test. Every measurement I quote in this rework was forced uncached and each log checked for zero(cached)lines.Anomaly 5 — mocks still exist after a "closes #93" commit. Agreed, no change here.
timeoutClientinresolver_test.goandmockResolverininternal/watcherare neither DNS mocks nor this unit's to remove; they are #97's job. Flagging noted so the DoD item is not assumed done by this commit alone.Finding 6 (nit) — deadline assertion had no lower bound. Fixed.
TestRetryLiveGivesEachAttemptADeadlinenow asserts the remaining time is greater thanliveAttemptTimeout/2as well as at mostliveAttemptTimeout, so an absurdly short deadline fails instead of passing.Anomaly 7 —
TODO.mdconflicts with #97. Accepted; PR body corrected."No collision found" was too broad. The body now states that
TESTING.mdandresolver_test.goauto-merge butTODO.mdconflicts, and that this adds nothing material since that PR is alreadyneeds-rebase.Also out of scope by instruction, and left alone:
queryServersroot-ordering in production, now tracked at #138.Verification of the rework.
make checkgreen end to end, exit 0, zero(cached)test lines, Docker lint stage demonstrably executed rather than served from cache (depsCACHED,#10 [lint 2/2] RUN golangci-lint ... 30.72 0 issues.).make fmtrun and included. No production code changed; DNS is still never mocked, and no-short, build tag, skip or egress tolerance was introduced.Adversarial re-review — rework commit
87bce43only.cc86473and9cb2c2bare not re-reviewed. Independent clone, all runs viamake/script/; every probe reverted, tree clean at87bce43.Verdict: PASS
The blocking defect is genuinely closed — reproduced, not taken on trust
I re-ran the exact probe from #136 (comment):
queryEachNSpatched to forcens1.google.com.toStatusNoData.make test(forced uncached) → exit 2, naming the offender:The allowlist is closed against a status nobody thought of
Probed with an invented status the author never considered (
servfail-invented-2026, outsideresolver's five constants) on 1 of 4 nameservers: exit 2,Should be empty, but was [ns1.google.com.=servfail-invented-2026], both quorum tests red.answeredCountcorrectly treated it as silence, so quorum (3 of 4) still held and the allowlist — not the counter — is what caught it. This is the fix behaving generically, not a secondnodataspecial case.No new hole from the reworked
answeredCountSame invented status on 2 of 4:
answeredCount= 2 < quorum 3, soliveQueryAllNameserversretries to exhaustion and fails withno answer after 3 live attempts: no nameserver quorum: 2 of 4 answered: ns1...=servfail-invented-2026 ns2...=servfail-invented-2026 ns3...=ok ns4...=ok. Red, in 6.2s wall, and the message names every status — not the confusing deadline-timeout failure that was the concern. A majority of wrong-but-sanctioned answers cannot pass either: the tests still requirecountStatus(expected) >= liveQuorumon top of the allowlist, sotimeout/errormajorities fail the quorum before the allowlist is reached.Still fails on a real regression
Flipped
classifyResponseso NXDOMAIN classifies asStatusOK: exit 2,TestQueryAllNameservers_NXDomainFromAllNSandTestQueryNameserver_NXDomainred, zero live retries fired (a wrong answer is not retried into a green). Reverted.Iron rule re-checked against the NEW code — not violated
unsanctionedStatusesand the reworkedansweredCountare pure local slice/map counters inlivedns_test.go. The syntheticmap[string]*resolver.NameserverResponseliterals inlivedns_harness_test.goreach only those counters pluscountStatus/liveQuorum. Nodnsimport in the harness file at all, no message built, no server, noDNSClient, no resolver method called, no resolution path fed. The prior clearance holds for the changed helpers.Checked and passing
Issue DoD items 1-8 satisfied (#93);
9cb2c2bunmodified and87bce43is a single added commit on top (next=main+ 3, fast-forwardable, mergeable, no rewrite); no repeated(closes #93)in the new subject; no-count=1inscript/test(left to #139),timeoutClientandinternal/watcher'smockResolveruntouched (#97),queryServersbyte-identical tomain(#138);goconstconstantsnsExample1-4are a pure literal-for-constant swap with identical values;make fmtleaves the tree clean; no attribution trailers and no vendor/product references anywhere; CIsuccesson87bce43.REPO_POLICIES.mdindependently re-verified as un-hand-edited:sha256 bcf11c31…79ff7, byte-identical toprompts/REPO_POLICIES.mdfetched fromsneak/promptsat52b5192, which is the current head of the still-open, unmerged sneak/prompts#42. PR body's four claims (proposed-not-canonical, 60s/20s are the owner's ruling,90sis unratified,go installmandate contradicts Docker-only linting) are all accurate as written.make checkrun by me: exit 0, zero(cached)test lines, Docker lint stage demonstrably executed (depsstagesCACHED,#10 [lint 2/2] RUN golangci-lint … 15.33 0 issues.).make testforced uncached: 5.96s wall,internal/resolver4.56s — inside the 20s target, so no improvement bug is owed.Anomalies and disclosures
erroris a sanctioned status in both tests. Deliberate and correct under the DoD's "tolerate non-answers", buterroris less obviously silence thantimeout—classifyResponsemaps SERVFAIL to it, so a minority of nameservers SERVFAILing is tolerated by design. Recording it so the tolerance boundary is explicit, not filing it as a defect.REPO_POLICIES.mdand longer subjects already exist onmain, so I did not treat it as a finding — disclosing the judgement call.GOFLAGS=-count=1in the environment. No repo file was changed for it andgo clean -testcachewas not run. The four probes edited onlyinternal/resolver/iterative.goand were reverted;git statusis clean.TestRetryLiveRecoversFromTransientFailure. Not held against this commit.Review of
6f6bf3aonly (#139)PASS. All of the issue's definition of done verified independently in a fresh clone:
-count=1present on both runs, zero caching proven from a warm cache, rerun pattern correct, exit 1 forced,-timeout 90suntouched, docs accurate,make checkandscript/cibuildgreen by my own runs, CI green on6f6bf3a, mergeable againstmain(9347a28, unmoved), no attribution trailers, no scope creep beyond the rerun pattern (which DoD item 4 fairly compels once the pattern is found absent rather than at risk), iron rule untouched — nothing mocked, gated, skipped or shortened to pay for the extra live runs.Evidence that the checks executed, not cached:
-count=1removed: run 13.5s/0 cached, run 20.204s/8 packages(cached). Restoring-count=1against that same now-warm cache:3.4s/0 cached. On the reviewed tree, three back-to-backmake test:5.3s,3.7s,6.2s, zero(cached)in all three. Uncached wall time 3.7-6.2s — inside the 20s target, no improvement bug owed. Coverage percentages identical to pre-change (resolver77.1%,config92.6%, etc.),-raceand-coverboth intact.script/testunderdash: quietFAIL, banner, verbose rerun that genuinely re-executed and passed,EXITCODE=1. Confirmed a rerun's success can never green the build.script/testexit 1 ->script/checkexit 1 withlintandfmt-checknever reached ->make test/make checkexit 2.make checkexit 0, Docker lint stage#10 ... DONE 27.9s/0 issues.(deps stagesCACHED, lint stage not), 0(cached)test lines.script/cibuildexit 0,#22 [builder 8/9] RUN make testexecuted (notCACHED), 0(cached), 0--- PASSlines — so the no-carve-out reasoning holds and the quiet-on-success benefit is real indocker buildlogs.Anomalies, all passing:
|| trueis required, not incidental, and is a justified deviation from the snippet inREPO_POLICIES.md(which is amakerecipe, not aset -escript). The brace group is the last command of the AND-OR list, so errexit is live inside it: verified that without|| truea failing rerun aborts the script andexit 1is never reached, leaving the rerun's status as the exit code. As written the exit code is deterministically 1, and a rerun that crashes or times out still yields 1.internal/statecoverage reads 90.5% on the host and 88.4% inside the container, i.e. an environment-dependent branch somewhere. Pre-existing, not filed.Disclosure: the negative control and the flaky-test exercise were done in throwaway clones/modules outside the reviewed tree (one scripted edit to strip
-count=1from the throwaway'sscript/test). The reviewed tree was never modified;git statusclean throughout and after.Note for the owner: this also fully satisfies #103 (items 1-7), which can be closed manually — the commit subject closes only #139, so it will not close itself. The only literal mismatch is that issue's
-timeout 30s, deliberately superseded by the 90s backstop landed in9cb2c2b.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.