Repository navigation
Conversation
|
@coderabbitai please review |
|
✅ Action performedReview finished.
|
|
Warning Review limit reached
Next review available in: 25 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis change establishes the first public release of an iterative DNSSEC resolver. It adds DNSSEC validation, hardened resolution, typed caching, security controls, CLI updates, comprehensive tests, CI, release automation, documentation, and contribution workflows. ChangesResolver release
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CLI
participant RecursiveResolver
participant DNSCache
participant DNSSECValidator
participant DNSAuthority
Client->>CLI: submit query and resolver options
CLI->>RecursiveResolver: resolve_answer or trace_answer
RecursiveResolver->>DNSCache: check answer or delegation cache
RecursiveResolver->>DNSAuthority: perform iterative DNS queries
RecursiveResolver->>DNSSECValidator: validate RRset or denial proof
DNSSECValidator-->>RecursiveResolver: validation state
RecursiveResolver-->>CLI: Answer or resolver error
CLI-->>Client: records, trace, and DNSSEC status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (20)
scripts/collect_domains_diverse.py-218-220 (1)
218-220: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject negative
--limitvalues.A negative limit is truthy at Line 256. Python then removes rows from the end of the corpus. Reject negative values before slicing.
Proposed fix
args = parser.parse_args() + if args.limit < 0: + parser.error("--limit must be zero or greater") rng = random.Random(args.seed) @@ - if args.limit: + if args.limit > 0: rows = rows[: args.limit]Also applies to: 254-257
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/collect_domains_diverse.py` around lines 218 - 220, Validate the parsed limit argument in the collection flow before the slicing logic around the main corpus handling at lines 254–257, rejecting any negative --limit value while continuing to treat 0 as unlimited. Ensure invalid input exits with a clear argument error rather than allowing Python’s negative slicing behavior.scripts/diff_harness.py-286-302 (1)
286-302: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate required execution inputs.
ThreadPoolExecutorrejectsmax_workersof 0 or negative values at runtime. An empty--reference-serversvalue leavesserversempty, so no reference is queried andrun_one()hits thebest is not Noneassertion. Reject these inputs during argument validation.Proposed fix
args = parser.parse_args() + if args.workers < 1: + parser.error("--workers must be at least 1") + @@ servers = [s.strip() for s in args.reference_servers.split(",") if s.strip()] + if not servers: + parser.error("--reference-servers must contain at least one server") + corpus = load_corpus(args.csv, args.sample, args.seed)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/diff_harness.py` around lines 286 - 302, Update the argument validation near parser.parse_args() to reject non-positive args.workers and an empty servers list derived from --reference-servers, reporting each as an invalid input and returning 2 before execution begins. Preserve the existing validation flow for reference, types, and configs.README.md-111-114 (1)
111-114: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winLimit the root-query warning to cache misses.
With
cache_answers=True, an answer or negative-cache hit can finish resolution without a root query, even when delegation caching is disabled. Replace “every single resolution” with “every upstream cache miss” to describe the actual load.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 111 - 114, Update the warning text in the zone-cut caching documentation to limit the root-query claim to upstream cache misses, replacing “every single resolution” accordingly while preserving the existing production warning and cache configuration context..github/ISSUE_TEMPLATE/bug_report.yml-30-31 (1)
30-31: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse an explicit reference resolver in the reproduction command.
The form requests
dig +dnssec <domain> <type>through the system stub. CONTRIBUTING.md Lines 100-106 says that the system stub can synthesize aCNAMEand that reference queries must use an explicit IP. Requestdig +dnssec @<server> <domain> <type>, or require reporters to record the resolver address.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/ISSUE_TEMPLATE/bug_report.yml around lines 30 - 31, Update the “Expected versus actual” description in the bug report template to require an explicit reference resolver IP in the dig command, using the form `dig +dnssec @<server> <domain> <type>` or requiring the reporter to record the resolver address.README.md-30-42 (1)
30-42: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep live DNS responses out of normative output examples.
Lines 35-41 and 167-178 hard-code A records and current CDN addresses. These values can change without a library release. Use placeholders or label the output as a dated snapshot.
Also applies to: 166-179
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 30 - 42, Update the README examples around RecursiveResolver.resolve and resolve_answer, including the repeated example near the later referenced section, so they do not present live DNS addresses as normative output. Replace changing A-record values with stable placeholders or explicitly label the output as a dated snapshot while preserving the API usage and DNSSEC result demonstration.README.md-50-52 (1)
50-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not claim that an RSA-2048 DKIM key is always split into two chunks.
DNS character strings can be split at arbitrary boundaries. A DKIM record can contain more than two chunks. The invariant is concatenation without a separator.
Suggested wording
-An RSA-2048 DKIM key is always split in two, so this is the difference between a key that verifies and one that never does. +An RSA-2048 DKIM key requires multiple chunks, but publishers may split it into two or more chunks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 50 - 52, Update the DKIM guidance in the README to remove the claim that RSA-2048 keys are always split into two chunks. State that DNS character strings may be split at arbitrary boundaries or into multiple chunks, and that the correct invariant is concatenating all chunks without separators.README.md-93-94 (1)
93-94: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake DNSSEC requirements consistent with
pyproject.toml.
pyproject.tomlonly depends ondnspython[dnssec,idna], which does not installcryptography, butREADME.md:93-94sayspip install recursive-resolverincludes it andRecursiveResolver()enables DNSSEC by default. Either addcryptography>=42todependenciesor change the documented default/quick-start to require thednssecextra. Also updateSECURITY.mdso it does not implycryptographycomes from thednssecextra whenrecursive-resolvercan document it as a default runtime dependency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 93 - 94, Add cryptography>=42 to the dependencies in pyproject.toml so the documented default RecursiveResolver() DNSSEC behavior matches installation. Keep the README quick-start/default guidance consistent with that dependency, and update SECURITY.md to state that cryptography is a direct runtime dependency rather than implying it is provided by the dnspython dnssec extra.scripts/release.sh-87-88 (1)
87-88: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe pre-flight changelog check and the release-notes extraction use different patterns.
Line 87 accepts
[${VERSION}]anywhere inCHANGELOG.md. Line 437 requires a heading that starts with## [${VERSION}]. A version referenced only in a link definition at the bottom of the file passes pre-flight and then yields empty release notes after the irreversible PyPI upload. Thegreppattern also treats.as a wildcard.Match the heading form in pre-flight, and fail if the extracted notes are empty.
🛠️ Proposed change
-grep -q "\[${VERSION}\]" "${REPO_DIR}/CHANGELOG.md" \ - || die "CHANGELOG.md has no entry for ${VERSION}" +grep -qF "## [${VERSION}]" "${REPO_DIR}/CHANGELOG.md" \ + || die "CHANGELOG.md has no '## [${VERSION}]' section"' "${REPO_DIR}/CHANGELOG.md" > "${NOTES_FILE}" +[[ -s "${NOTES_FILE}" ]] || die "extracted release notes for ${VERSION} are empty" printf '\n---\n\nInstall: `pip install %s==%s`\n' "${PKG_NAME}" "${VERSION}" >> "${NOTES_FILE}"Also applies to: 436-440
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/release.sh` around lines 87 - 88, Update the pre-flight check near the changelog validation to require a line beginning with the exact heading form “## [${VERSION}]”, escaping the version for literal matching rather than allowing regex wildcards. In the release-notes extraction around the existing heading logic, validate that the extracted notes are non-empty and call the established failure path before any upload proceeds..github/workflows/ci.yml-84-87 (1)
84-87: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winConfirm that
pythonis on PATH in thebuildjob.This job never runs
actions/setup-python, anduv python installdoes not guarantee apythonexecutable on PATH. If the runner image stops shipping a barepython, this step fails.uv run --no-project pythonremoves the assumption.Also confirm that exactly one wheel matches
dist/*.whl, becausepython -m zipfile -laccepts a single archive argument.🛠️ Proposed change
- name: Verify the wheel ships py.typed run: | - python -m zipfile -l dist/*.whl | grep -q 'recursive_resolver/py.typed' \ + uv run --no-project python -m zipfile -l dist/*.whl | grep -q 'recursive_resolver/py.typed' \ || { echo "py.typed missing from the wheel"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 84 - 87, Update the “Verify the wheel ships py.typed” step to invoke Python through “uv run --no-project python” instead of assuming a bare python executable is on PATH, and ensure the wheel check first resolves exactly one file from dist/*.whl before passing it to zipfile.scripts/check_authors.py-120-120 (1)
120-120: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRead the commit message file as UTF-8.
Path.read_text()uses the platform default encoding. Git writes commit messages as UTF-8. On a system with a non-UTF-8 default, this raisesUnicodeDecodeErrorfor an accented name in a trailer.🛠️ Proposed change
- problems = check_message(Path(argv[1]).read_text(), "commit message") + problems = check_message(Path(argv[1]).read_text(encoding="utf-8"), "commit message")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check_authors.py` at line 120, Update the commit-message read in the check_authors flow to explicitly decode Path(argv[1]) as UTF-8 before passing it to check_message, preserving the existing "commit message" label and validation behavior.scripts/check_authors.py-60-61 (1)
60-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
\[bot\]rejects every GitHub App author, not only AI tools.The pattern matches
dependabot[bot],renovate[bot], andgithub-actions[bot]. TheauthorsCI job runs on every pull request, so a dependency-bump PR fails the check for a reason the policy does not intend. Restrict the pattern to the AI app names, or allowlist the known non-AI bots.🛠️ Proposed change
- # The GitHub App convention, e.g. "copilot[bot]" or "openai-codex[bot]". - r"\[bot\]", + # The GitHub App convention, e.g. "copilot[bot]" or "openai-codex[bot]". + # Scoped to AI apps: dependabot[bot] and renovate[bot] are not authors of + # this kind and must not trip the check. + r"\b(?:copilot|codex|claude|devin|cursor|gemini|chatgpt)[\w-]*\[bot\]",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check_authors.py` around lines 60 - 61, Update the author-matching regex in the authors validation logic to reject only the intended AI GitHub App names, such as copilot[bot] and openai-codex[bot], rather than every author ending in [bot]. Preserve acceptance of known non-AI bots including dependabot[bot], renovate[bot], and github-actions[bot].scripts/release.sh-452-454 (1)
452-454: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe final message prints the PyPI URL, not the GitHub release URL.
This block confirms the GitHub release. Line 454 repeats the PyPI link from line 411.
🛠️ Proposed change
echo ok "GitHub release ${TAG} created" -echo " https://pypi.org/project/${PKG_NAME}/${VERSION}/" +echo " https://github.com/${REPO_SLUG}/releases/tag/${TAG}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/release.sh` around lines 452 - 454, Update the final confirmation block after “GitHub release ${TAG} created” to print the GitHub release URL instead of the PyPI URL, reusing the script’s existing GitHub repository and TAG variables or URL format.pyproject.toml-10-11 (1)
10-11: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winConstrain Hatchling for PEP 639 license metadata.
requires = ["hatchling"]allows old Hatchling releases that either fail onlicense-filesor emit incompatible metadata; lock a sufficiently recent bound, such as Hatchling with final PEP 639/metadata 2.4 support, soscripts/release.shreceivesLicense-Expressionand included license files as promised.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` around lines 10 - 11, Constrain the Hatchling build requirement in the project configuration to a sufficiently recent version that supports PEP 639 and metadata 2.4, while retaining the existing license and license-files declarations so release.sh produces License-Expression metadata and includes the specified license files.tests/test_cli.py-80-87 (1)
80-87: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winThree CLI tests build a real
RecursiveResolverand so depend on thednssecextra. These tests omit thepatch("recursive_resolver.cli.RecursiveResolver")context used by every other test in the file.maintherefore constructs a real resolver with the CLI defaultdnssec=True, andRecursiveResolver.__init__raisesDNSSECUnavailableErrorwhencryptography_available()is false. In such an environmentmainreturns 2 from the constructor handler atsrc/recursive_resolver/cli.pylines 114-116, and each assertion fails for a reason unrelated to what it tests.
tests/test_cli.py#L80-L87:test_unknown_rdtype_is_reported_cleanlyandtest_invalid_name_is_reported_cleanlyboth assert== 1; add"--no-dnssec"to each argv list so the constructor never reaches the cryptography check.tests/test_cli.py#L180-L182:test_an_unknown_cache_depth_is_reported_cleanlyasserts"unknown cache depth"on stderr, but the DNSSEC check precedes cache construction in__init__, so the message becomesDNSSECUnavailableError; add"--no-dnssec"to the argv list here as well.If every CI matrix job installs the
dnssecextra, no change is needed. Confirm with the script in the comment at lines 80-87.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli.py` around lines 80 - 87, Update tests/test_cli.py lines 80-87 in test_unknown_rdtype_is_reported_cleanly and test_invalid_name_is_reported_cleanly to pass --no-dnssec in each main argument list; also update tests/test_cli.py lines 180-182 in test_an_unknown_cache_depth_is_reported_cleanly with --no-dnssec, ensuring these tests exercise their intended validation paths without requiring the DNSSEC extra.src/recursive_resolver/cli.py-118-127 (1)
118-127: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHandle
BrokenPipeErrorso piped output does not print a traceback.The plain output path is documented as pipeable. If the reader closes early, for example
recursive-resolver example.com | head -1, the interpreter raisesBrokenPipeErroron flush and prints a traceback at exit. Catch it and exit quietly.🛡️ Proposed fix
try: if args.trace: return _run_trace(resolver, args) return _run_query(resolver, args) + except BrokenPipeError: + # The reader (head, grep -q, ...) closed the pipe. Exit quietly. + os.dup2(os.open(os.devnull, os.O_WRONLY), sys.stdout.fileno()) + return 0 except ResolverError as exc:Add
import osat the top of the module.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/recursive_resolver/cli.py` around lines 118 - 127, Update the CLI execution flow around the existing ResolverError handling to catch BrokenPipeError from piped output and exit quietly with no traceback; add the required os import and use the appropriate quiet-exit behavior for the process.src/recursive_resolver/cli.py-178-188 (1)
178-188: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
--textis ignored in trace mode.
_run_queryhonours--textand prints concatenated character-strings._run_tracealways printsanswer.records, in both JSON and plain mode. A user who runs--trace --text example.com TXTreceives presentation format with the" "seam, which is the exact DKIM/SPF trap thatAnswer.text_values()exists to avoid. The flag combination is accepted, so the silent difference is surprising.Either apply the same conversion, or reject
--textwith--trace.🐛 Proposed fix: share the value conversion
+def _values_for(answer: Answer, args: argparse.Namespace) -> list[str] | None: + """Records honouring --text, or None if --text does not apply to the type.""" + if not args.text: + return answer.records + try: + return answer.text_values() + except TypeError as exc: + print(f"--text: {exc}", file=sys.stderr) + return NoneThen in
_run_trace:answer, trace = resolver.trace_answer(args.domain, args.rdtype) + values = _values_for(answer, args) if answer is not None else None + if answer is not None and values is None: + return 2 if args.json_output: @@ - "records": answer.records if answer else None, + "records": values, @@ if answer is not None: _note_dnssec(answer, args) - for record in answer.records: + for record in values or []: print(record)
_run_querycan then call_values_foras well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/recursive_resolver/cli.py` around lines 178 - 188, Update _run_trace to honor --text by using the shared value-conversion helper, such as _values_for, when rendering answer records instead of printing answer.records directly. Reuse the same conversion in _run_query so normal and trace modes produce identical text values, while preserving existing JSON and plain output behavior when --text is not set.tests/test_cli.py-14-22 (1)
14-22: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winImport
dns.rdatatypeanddns.nameexplicitly.
_answerusesdns.rdatatype.from_textanddns.name.from_text, but_answeronly importsdns.rdata,dns.rdataclass, anddns.rrset. Add explicit imports for the modules this helper uses so the test does not depend on dnspython’s internal import side effects.🛡️ Proposed fix
def _answer(records: str = "1.2.3.4", rdtype: str = "A", dnssec=ValidationState.SECURE) -> Answer: + import dns.name import dns.rdata import dns.rdataclass + import dns.rdatatype import dns.rrset🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cli.py` around lines 14 - 22, Update the test helper _answer to explicitly import dns.rdatatype and dns.name before using dns.rdatatype.from_text and dns.name.from_text, preserving the existing RRset construction behavior.src/recursive_resolver/resolver.py-854-857 (1)
854-857: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winUse a cryptographically secure source for the two anti-attacker randomisations.
Both call sites justify randomisation as a defence against an attacker choosing the order. Line 855 states that a deterministic slice is attacker-orderable.
randomis the Mersenne Twister, which is seeded predictably and whose state is recoverable from enough observed outputs. Using it for a control whose stated purpose is to deny an attacker control over selection is inconsistent.The change is local and has no measurable cost at this call rate.
🔒️ Proposed fix
-import random +import secrets+_RANDOM = secrets.SystemRandom()- ns_names = random.sample(ns_names, self.limits.max_ns_per_referral) + ns_names = _RANDOM.sample(ns_names, self.limits.max_ns_per_referral)def _order_servers(self, nameservers: list[str]) -> list[str]: servers = self.address_filter.filter(nameservers) - random.shuffle(servers) + _RANDOM.shuffle(servers) return serversAlso applies to: 1265-1268
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/recursive_resolver/resolver.py` around lines 854 - 857, Replace the standard `random` module calls at both anti-attacker randomisation sites—including the `ns_names` sampling near the referral limit check and the corresponding call near the second site—with a cryptographically secure randomness source. Preserve the existing sample sizes and selection behavior while ensuring both selections use the secure source consistently.Source: Linters/SAST tools
src/recursive_resolver/resolver.py-244-246 (1)
244-246: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAn empty
trust_anchorstuple silently falls back to the IANA anchors.Line 246 tests truthiness.
RecursiveResolver(trust_anchors=())therefore constructsDNSSECValidator()with the built-in IANA anchors, not with an empty anchor set. The declared type istuple[str, ...] | None, andNonealready means "use the defaults". A caller who passes()expects the opposite of what happens.Test against
Noneso the two cases stay distinct. If an empty tuple is invalid input, reject it explicitly.🐛 Proposed fix
self._validator: DNSSECValidator | None = None if dnssec: - self._validator = DNSSECValidator(trust_anchors=trust_anchors) if trust_anchors else DNSSECValidator() + self._validator = ( + DNSSECValidator(trust_anchors=trust_anchors) if trust_anchors is not None else DNSSECValidator() + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/recursive_resolver/resolver.py` around lines 244 - 246, Update the validator initialization in RecursiveResolver to distinguish trust_anchors=None from an explicitly empty tuple: only use DNSSECValidator() for None, and pass any provided tuple, including (), as trust_anchors. If empty tuples are intended to be invalid, validate and reject them explicitly instead.src/recursive_resolver/dnssec.py-512-523 (1)
512-523: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winApply the NSEC3 iteration cap in
prove_wildcard.
prove_no_ds,prove_nxdomain, andprove_nodataall reject NSEC3 records aboveself.max_nsec3_iterationsbefore hashing.prove_wildcarddoes not. A hostile zone can therefore supply a wildcard proof with a very high iteration count, and each_nsec3_coverscall performs that many hash rounds. The query budget limits the number of hashes, not the cost of one hash.🛡️ Proposed fix
params = nsec3s[0][0] + if params.iterations > self.max_nsec3_iterations: + logger.debug("NSEC3 iteration count %d exceeds cap", params.iterations) + return False return any(self._nsec3_covers(next_closer, n, params, budget) for n in nsec3s)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/recursive_resolver/dnssec.py` around lines 512 - 523, Update prove_wildcard around the validated NSEC3 records and before any _nsec3_covers call to reject records whose iteration count exceeds self.max_nsec3_iterations, matching the guards in prove_no_ds, prove_nxdomain, and prove_nodata; preserve the existing false-return behavior for disallowed proofs and ensure over-limit records are not hashed.
🧹 Nitpick comments (16)
scripts/release.sh (1)
400-408: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReconsider
--skip-existingon the real PyPI upload.On TestPyPI the flag is useful for retries. On PyPI it converts "this version is already published" into a silent success. The script then tags and creates a GitHub release for artifacts that PyPI never accepted. Dropping the flag makes the conflict visible.
🛠️ Proposed change
- python -m twine upload --skip-existing dist/* + python -m twine upload dist/*🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/release.sh` around lines 400 - 408, Remove the --skip-existing option from the python -m twine upload command in the real PyPI release block, while leaving the TestPyPI upload behavior unchanged so duplicate-version conflicts fail visibly before tagging or creating the GitHub release..github/workflows/ci.yml (2)
64-72: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd
timeout-minutesto the jobs, in particularintegration.The
integrationjob queries live DNS. If resolution hangs, the job runs to the 6-hour default limit and consumes the runner. A short explicit timeout bounds the failure.⏱️ Proposed change
integration: runs-on: ubuntu-latest + timeout-minutes: 20 steps:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 64 - 72, Add an explicit short timeout-minutes setting to the integration job, and apply the same job-level timeout to the other CI jobs as appropriate. Keep the existing integration steps and test command unchanged.
27-27: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon the checkout steps.No job pushes to the repository. The default checkout stores the
GITHUB_TOKENin.git/config, which widens the blast radius of any compromised step.zizmorflags all four checkouts.🔒 Proposed change for each checkout step
- uses: actions/checkout@v4 + with: + persist-credentials: falseFor the
authorsjob, add it next to the existingfetch-depth:- uses: actions/checkout@v4 with: # The whole history, so the base commit of the PR is present. fetch-depth: 0 + persist-credentials: falseAlso applies to: 51-54, 67-67, 77-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml at line 27, Update all four actions/checkout@v4 steps in the workflow, including those in the authors and other referenced jobs, to set persist-credentials to false alongside the existing checkout options. Preserve the current checkout behavior and other configuration.Source: Linters/SAST tools
Makefile (1)
64-75: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAnchor the version regexes, and consider reusing one extractor.
re.search(r'version = "(.+?)"', ...)is not anchored, so it returns the first match anywhere inpyproject.toml, not necessarily the[project]version.scripts/release.shanchors the same lookup withawk -F'"' '/^version = /', so the two paths can disagree. Line 75 also interpolates the version into agreppattern, where.matches any character.♻️ Proposed change
-VERSION := $(shell python3 -c "import re; print(re.search(r'version = \"(.+?)\"', open('pyproject.toml').read()).group(1))") +VERSION := $(shell python3 -c "import re; print(re.search(r'^version = \"(.+?)\"', open('pyproject.toml').read(), re.M).group(1))") release: ## Interactive release to PyPI and GitHub (see CONTRIBUTING.md) ./scripts/release.sh release-check: check-all ## Everything the release script checks, without publishing `@echo` "Verifying version consistency..." - `@PY_VER`=$$(python3 -c "import re; print(re.search(r'__version__ = \"(.+?)\"', open('src/recursive_resolver/__init__.py').read()).group(1))"); \ + `@PY_VER`=$$(python3 -c "import re; print(re.search(r'^__version__ = \"(.+?)\"', open('src/recursive_resolver/__init__.py').read(), re.M).group(1))"); \ if [ "$$PY_VER" != "$(VERSION)" ]; then \ echo "ERROR: Version mismatch: __init__.py=$$PY_VER vs pyproject.toml=$(VERSION)"; exit 1; \ fi - `@grep` -q "\[$(VERSION)\]" CHANGELOG.md || { echo "ERROR: no CHANGELOG.md entry for $(VERSION)"; exit 1; } + `@grep` -qF "[$(VERSION)]" CHANGELOG.md || { echo "ERROR: no CHANGELOG.md entry for $(VERSION)"; exit 1; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 64 - 75, Update the Makefile VERSION extraction and release-check version lookup to match the anchored top-level version assignment used by scripts/release.sh, preferably by reusing a single extractor. Also make the CHANGELOG validation treat $(VERSION) literally rather than as an unescaped grep regular expression, while preserving the existing consistency checks.Dockerfile (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the
uvimage tag.
ghcr.io/astral-sh/uv:latestmakes the image non-reproducible and pulls unrevieweduvreleases into the build. Pin a version tag, or a digest for full reproducibility.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile` at line 8, Update the Dockerfile COPY source to replace the floating ghcr.io/astral-sh/uv:latest tag with a specific reviewed uv version tag or immutable digest, keeping the existing /uv and /uvx binaries copied into /usr/local/bin/.src/recursive_resolver/cli.py (1)
80-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider rejecting
--no-dnssec --require-dnssecat parse time.The two flags are contradictory. With both set, the resolver is built with
dnssec=False, require_dnssec=True, and every lookup fails later withDNSSECInsecureErrorinstead of a usage message. A mutually exclusive group reports the conflict immediately with exit code 2.♻️ Proposed refactor
- parser.add_argument("--no-dnssec", action="store_true", help="Disable DNSSEC validation") - parser.add_argument("--require-dnssec", action="store_true", help="Fail unless the answer is DNSSEC-authenticated") + dnssec_group = parser.add_mutually_exclusive_group() + dnssec_group.add_argument("--no-dnssec", action="store_true", help="Disable DNSSEC validation") + dnssec_group.add_argument( + "--require-dnssec", action="store_true", help="Fail unless the answer is DNSSEC-authenticated" + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/recursive_resolver/cli.py` around lines 80 - 81, Update the CLI argument definitions around the DNSSEC options to place --no-dnssec and --require-dnssec in a mutually exclusive argparse group, so conflicting flags are rejected during parsing with the standard usage error and exit code 2.tests/test_resolver.py (1)
11-11: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueNo change needed for the sibling
conftestimport.
pytestdoes not overrideimportmode, andtests/__init__.pyis empty, sofrom conftest import ...resolves with the current test execution setup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_resolver.py` at line 11, No code changes are required for the from conftest import in tests/test_resolver.py; retain the existing import because it resolves correctly under the current pytest configuration.src/recursive_resolver/budget.py (1)
113-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the two DNSSEC fields in the
QueryBudgetdocstring.The
Argsblock listsmax_queries,max_nx_targets,max_referralsanddeadline. It omitsmax_signature_validationsandmax_nsec3_hashes, which are real fields with their own spend methods.Limitsdocuments both.♻️ Proposed docstring addition
max_nx_targets: Total NS-hostname resolutions allowed to fail. max_referrals: Total referrals allowed to be followed. + max_signature_validations: Total RRSIG verifications allowed (KeyTrap). + max_nsec3_hashes: Total NSEC3 hash computations allowed (KeyTrap). deadline: Absolute ``time.monotonic()`` value after which the resolution is abandoned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/recursive_resolver/budget.py` around lines 113 - 136, Update the QueryBudget docstring’s Args section to document max_signature_validations and max_nsec3_hashes alongside the other budget fields, matching the corresponding limit descriptions and existing spend-method behavior.src/recursive_resolver/cache.py (2)
317-345: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDelegation lookups return the live cached object, unlike answer lookups.
get_answercopies the RRset through_isolateso the cache and the caller never share mutable state.get_delegationandclosest_delegationreturn the storedDelegationdirectly.Delegation.addressesandDelegation.ns_namesare mutable lists held by the cache and shared across threads.The current resolver does not mutate them:
_starting_pointbuilds a new list throughaddress_filter.filter. The hazard is a future caller that appends todelegation.addressesand silently rewrites the cached zone cut for every thread.♻️ Proposed isolation for delegation reads
def get_delegation(self, zone: dns.name.Name) -> Delegation | None: """Look up the cached delegation for exactly this zone.""" with self._lock: entry = self._get((_DELEGATION, zone, 0, 0)) if entry is None: return None delegation: Delegation = entry.rrset - return delegation + return replace(delegation, addresses=list(delegation.addresses), ns_names=list(delegation.ns_names))Apply the same copy in
closest_delegation, and copy the lists input_delegationso the producer cannot mutate the stored entry either.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/recursive_resolver/cache.py` around lines 317 - 345, Isolate mutable delegation data at cache boundaries to prevent callers and producers from sharing cached lists. Update get_delegation and closest_delegation to return copied Delegation objects, and update put_delegation to copy addresses and ns_names before storing; preserve the existing lookup and filtering behavior.
247-303: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
statscounts only answer lookups.
get_answerupdatesstats.hitsandstats.misses.get_nxdomain,get_nodata,get_delegationandclosest_delegationdo not. A reportedhit_ratetherefore ignores the negative and delegation traffic, which is the majority of lookups in an iterative resolver. If that is deliberate, state it in theCacheStatsdocstring so an operator does not read the value as a whole-cache hit rate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/recursive_resolver/cache.py` around lines 247 - 303, Update CacheStats documentation to explicitly state that stats.hits, stats.misses, and hit_rate cover only get_answer lookups, excluding get_nxdomain, get_nodata, get_delegation, and closest_delegation traffic. Do not change the lookup methods’ accounting unless the intended contract is to make hit_rate represent all cache lookups.tests/test_security.py (2)
39-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_resolveris duplicated across two test modules.
tests/test_internals.pylines 44-47 defines the same helper with the same two defaults. Both modules already import fromconftest. Move the helper intotests/conftest.pyso the DNSSEC and cache defaults for unit tests are set in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_security.py` around lines 39 - 42, Move the duplicated _resolver helper into tests/conftest.py, preserving its dnssec=False and cache_enabled=False defaults and RecursiveResolver construction. Remove the local definitions from both test_security.py and test_internals.py, allowing both modules to reuse the shared conftest helper.
504-521: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the suppressed exception type.
contextlib.suppress(Exception)hides every failure inside_verify_denial, including anAttributeErrororTypeErrorintroduced by a later refactor. The two assertions that follow would still pass, becauseattemptedis populated by the first_send_querycall.The comment states the intent: the call may fail because the stub serves no real signatures. That failure is a
ResolverError. Suppress only that type so a genuine breakage still surfaces.♻️ Proposed change
- with patch.object(resolver, "_send_query", side_effect=send), contextlib.suppress(Exception): + with patch.object(resolver, "_send_query", side_effect=send), contextlib.suppress(ResolverError):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_security.py` around lines 504 - 521, In the _verify_denial test invocation, replace the broad contextlib.suppress(Exception) with suppression of only ResolverError, preserving the existing _send_query patch and assertions. Ensure unexpected AttributeError, TypeError, or other failures propagate normally.src/recursive_resolver/singleflight.py (1)
62-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWaiters re-raise one shared exception instance.
Every waiter for the same key executes
raise call.erroron the same exception object. Eachraisewrites__traceback__on that shared instance, concurrently, from several threads. Two effects follow: the traceback grows on each re-raise, and a caller inspecting__traceback__sees frames from threads it never ran.The behaviour is correct in the sense that the right exception type and message reach every caller, so this is a diagnostics concern rather than a resolution bug.
♻️ Proposed change to give each waiter its own traceback
if call.error is not None: - raise call.error + raise call.error.with_traceback(None)Alternatively, record the exception and re-raise a new instance of the same class per waiter, if preserving the leader's traceback is not required.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/recursive_resolver/singleflight.py` around lines 62 - 68, Update the waiter error path in the singleflight resolution method around call.error so each waiter raises its own exception instance instead of re-raising the shared exception object. Preserve the original exception type and message while avoiding mutation of the shared exception’s traceback; leave the timeout fallback and successful call.value path unchanged.src/recursive_resolver/resolver.py (2)
890-893: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd parentheses to the glue rdtype test.
The condition relies on
andbinding tighter thanor. It evaluates asA or (AAAA and not self.ipv4_only), which is the intended behaviour, and the tests confirm it. It reads as(A or AAAA) and not self.ipv4_only, which would reject all IPv4 glue in the defaultipv4_only=Truemode.This is the address-selection path for nameservers. A later edit made on the misread meaning breaks glue selection silently. Make the grouping explicit.
♻️ Proposed clarification
- if rrset.rdtype == dns.rdatatype.A or rrset.rdtype == dns.rdatatype.AAAA and not self.ipv4_only: - pass - else: + if rrset.rdtype == dns.rdatatype.A: + pass + elif rrset.rdtype == dns.rdatatype.AAAA and not self.ipv4_only: + pass + else: continue🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/recursive_resolver/resolver.py` around lines 890 - 893, In the glue rdtype condition, add explicit parentheses around the AAAA branch so it remains equivalent to A or (AAAA and not self.ipv4_only). Preserve IPv4 glue selection when ipv4_only is true and keep the existing continue behavior for unsupported records.
479-479: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe loop bound makes
max_referralsunreachable.The delegation loop runs at most
self.max_depthiterations, default 20. Every referral, every NXDOMAIN sibling retry and every lame-server retry consumes one iteration.Limits.max_referralsdefaults to 130, sonote_referralcan never raise: the loop raisesMaxDepthErrorfirst, at iteration 21.Two consequences.
max_referralsis dead configuration at the default settings, and a caller who raises it to bound a hostile zone gets no effect.budget.pydocuments it as Unbound'sMAX_REFERRAL_COUNTcontrol.Either derive the loop bound from
self.limits.max_referralsas well, or state in theLimitsdocstring thatmax_depthis the binding limit for a single_resolve_iterativeframe and thatmax_referralsbounds the whole call tree.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/recursive_resolver/resolver.py` at line 479, The delegation loop in _resolve_iterative currently caps iterations at self.max_depth, making Limits.max_referrals ineffective. Update the loop bound to account for both limits so referral, NXDOMAIN sibling, and lame-server retries cannot prevent note_referral from enforcing max_referrals; preserve MaxDepthError behavior when the depth limit is the binding constraint.tests/test_dnssec_signed.py (1)
410-463: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
_wildcard_chain(proof=False)instead of rebuilding the chain.
_wildcard_chainalready acceptsproofand builds the same root, leaf, forged answer, delegation, DNSKEY table, andsendfunction. Theproof=Falsebranch is never used, and this test duplicates about 50 lines of that setup. Callself._wildcard_chain(proof=False)here so both wildcard tests share one fixture.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_dnssec_signed.py` around lines 410 - 463, Replace the duplicated setup in test_the_resolver_refuses_an_unproven_wildcard_answer with the existing self._wildcard_chain(proof=False) fixture, reusing its returned resolver/send context and preserving the DNSSECValidationError assertion for the wildcard failure. Remove the locally constructed zones, responses, table, state, and send function so both wildcard tests share the same chain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e22bb8d4-4ec0-4982-b92b-7c38e395c191
📒 Files selected for processing (49)
.github/ISSUE_TEMPLATE/bug_report.yml.github/ISSUE_TEMPLATE/config.yml.github/ISSUE_TEMPLATE/feature_request.yml.github/pull_request_template.md.github/workflows/ci.ymlCHANGELOG.mdCONTRIBUTING.mdDockerfileLICENSEMakefileREADME.mdSECURITY.mdTHIRD-PARTY.mdpyproject.tomlscripts/check_authors.pyscripts/collect_domains_1_tranco.pyscripts/collect_domains_2_ct.pyscripts/collect_domains_3_zone.pyscripts/collect_domains_diverse.pyscripts/diff_harness.pyscripts/prepare_test_domains.pyscripts/release.shsrc/recursive_resolver/__init__.pysrc/recursive_resolver/__main__.pysrc/recursive_resolver/addresses.pysrc/recursive_resolver/budget.pysrc/recursive_resolver/cache.pysrc/recursive_resolver/cli.pysrc/recursive_resolver/dnssec.pysrc/recursive_resolver/exceptions.pysrc/recursive_resolver/py.typedsrc/recursive_resolver/resolver.pysrc/recursive_resolver/roots.pysrc/recursive_resolver/singleflight.pytests/conftest.pytests/signed_zone.pytests/test_cache.pytests/test_check_authors.pytests/test_cli.pytests/test_csv.pytests/test_dnssec.pytests/test_dnssec_chain.pytests/test_dnssec_signed.pytests/test_integration.pytests/test_internals.pytests/test_resolver.pytests/test_security.pytests/test_singleflight.pytests/test_transport.py
| import dns.rdatatype | ||
| import dns.rrset | ||
| import pytest | ||
| from conftest import make_response |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
from conftest import make_response breaks collection in CI.
CI reports ModuleNotFoundError: No module named 'conftest' at this line for every matrix job. The tests directory is not on sys.path during collection. Fix the pytest configuration rather than each import.
🧰 Tools
🪛 GitHub Actions: CI / 0_test (3.14).txt
[error] 17-17: pytest collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 1_test (3.13).txt
[error] 17-17: uv run pytest -m "not integration" -q --no-cov failed during test collection: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 2_test (3.10).txt
[error] 17-17: pytest collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 3_test (3.11).txt
[error] 17-17: Test collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 4_test (3.12).txt
[error] 17-17: Test collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 6_integration.txt
[error] 17-17: uv run pytest -q --cov failed during collection: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / integration
[error] 17-17: Test collection failed because the module 'conftest' could not be imported.
🪛 GitHub Actions: CI / test (3.10)
[error] 17-17: Pytest collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / test (3.11)
[error] 17-17: Test collection failed during 'uv run pytest -m "not integration" -q --no-cov': ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / test (3.12)
[error] 17-17: Test collection failed under uv run pytest -m "not integration" -q --no-cov: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / test (3.13)
[error] 17-17: pytest collection failed during 'uv run pytest -m "not integration" -q --no-cov': ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / test (3.14)
[error] 17-17: Pytest collection failed: ModuleNotFoundError: No module named 'conftest'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_dnssec_chain.py` at line 17, Update the pytest configuration so
the tests directory is added to the import path during collection, allowing
tests such as test_dnssec_chain.py to resolve make_response from conftest. Fix
the shared pytest configuration rather than changing individual imports.
| from conftest import make_response | ||
| from signed_zone import SignedZone, rrset_of |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
from conftest import ... and from signed_zone import ... break collection in CI.
CI reports ModuleNotFoundError: No module named 'conftest' at line 18 for every matrix job. signed_zone resolves the same way and will fail next. Both imports need the tests directory on sys.path.
🧰 Tools
🪛 GitHub Actions: CI / 0_test (3.14).txt
[error] 18-18: pytest collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 1_test (3.13).txt
[error] 18-18: uv run pytest -m "not integration" -q --no-cov failed during test collection: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 2_test (3.10).txt
[error] 18-18: pytest collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 3_test (3.11).txt
[error] 18-18: Test collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 4_test (3.12).txt
[error] 18-18: Test collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 6_integration.txt
[error] 18-18: uv run pytest -q --cov failed during collection: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / integration
[error] 18-18: Test collection failed because the module 'conftest' could not be imported.
🪛 GitHub Actions: CI / test (3.10)
[error] 18-18: Pytest collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / test (3.11)
[error] 18-18: Test collection failed during 'uv run pytest -m "not integration" -q --no-cov': ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / test (3.12)
[error] 18-18: Test collection failed under uv run pytest -m "not integration" -q --no-cov: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / test (3.13)
[error] 18-18: pytest collection failed during 'uv run pytest -m "not integration" -q --no-cov': ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / test (3.14)
[error] 18-18: Pytest collection failed: ModuleNotFoundError: No module named 'conftest'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_dnssec_signed.py` around lines 18 - 19, Update the imports in
tests/test_dnssec_signed.py to resolve conftest and signed_zone through the
tests package/path, ensuring both modules are discoverable during CI test
collection without relying on the tests directory being implicitly added to
sys.path.
| from unittest.mock import patch | ||
|
|
||
| import pytest | ||
| from conftest import make_response, referral, root_to_com |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
from conftest import ... breaks collection in CI.
CI reports ModuleNotFoundError: No module named 'conftest' at this line for every matrix job.
🧰 Tools
🪛 GitHub Actions: CI / 0_test (3.14).txt
[error] 10-10: pytest collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 1_test (3.13).txt
[error] 10-10: uv run pytest -m "not integration" -q --no-cov failed during test collection: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 2_test (3.10).txt
[error] 10-10: pytest collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 3_test (3.11).txt
[error] 10-10: Test collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 4_test (3.12).txt
[error] 10-10: Test collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 6_integration.txt
[error] 10-10: uv run pytest -q --cov failed during collection: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / integration
[error] 10-10: Test collection failed because the module 'conftest' could not be imported.
🪛 GitHub Actions: CI / test (3.10)
[error] 10-10: Pytest collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / test (3.11)
[error] 10-10: Test collection failed during 'uv run pytest -m "not integration" -q --no-cov': ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / test (3.12)
[error] 10-10: Test collection failed under uv run pytest -m "not integration" -q --no-cov: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / test (3.13)
[error] 10-10: pytest collection failed during 'uv run pytest -m "not integration" -q --no-cov': ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / test (3.14)
[error] 10-10: Pytest collection failed: ModuleNotFoundError: No module named 'conftest'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_singleflight.py` at line 10, Update the imports in
tests/test_singleflight.py so the test module resolves the shared fixtures
without relying on a top-level conftest module import. Preserve the existing
make_response, referral, and root_to_com references while using the repository’s
supported test import pattern.
| import dns.query | ||
| import dns.rdatatype | ||
| import pytest | ||
| from conftest import make_response |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
from conftest import make_response breaks collection in CI.
CI reports ModuleNotFoundError: No module named 'conftest' at this line for every matrix job.
🧰 Tools
🪛 GitHub Actions: CI / 0_test (3.14).txt
[error] 18-18: pytest collection failed: ModuleNotFoundError: No module named 'conftest'. Eight test modules failed during collection; the command exited with code 2.
🪛 GitHub Actions: CI / 1_test (3.13).txt
[error] 18-18: uv run pytest -m "not integration" -q --no-cov failed during test collection: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 2_test (3.10).txt
[error] 18-18: pytest collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 3_test (3.11).txt
[error] 18-18: Test collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 4_test (3.12).txt
[error] 18-18: Test collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / 6_integration.txt
[error] 18-18: uv run pytest -q --cov failed during collection: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / integration
[error] 18-18: Test collection failed because the module 'conftest' could not be imported.
🪛 GitHub Actions: CI / test (3.10)
[error] 18-18: Pytest collection failed: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / test (3.11)
[error] 18-18: Test collection failed during 'uv run pytest -m "not integration" -q --no-cov': ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / test (3.12)
[error] 18-18: Test collection failed under uv run pytest -m "not integration" -q --no-cov: ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / test (3.13)
[error] 18-18: pytest collection failed during 'uv run pytest -m "not integration" -q --no-cov': ModuleNotFoundError: No module named 'conftest'.
🪛 GitHub Actions: CI / test (3.14)
[error] 18-18: Pytest collection failed: ModuleNotFoundError: No module named 'conftest'.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_transport.py` at line 18, Update the import in the affected test
module so it resolves the shared make_response fixture/helper through the
project’s supported test-package import path rather than importing conftest as a
top-level module. Preserve use of make_response while ensuring pytest collection
works from CI’s execution context.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/test_cache.py (1)
163-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest reader isolation for
ns_names.
Delegation.addressesandDelegation.ns_namesare mutable. The reader-isolation check mutates onlyaddresses. A regression that sharesns_namesbetween readers would pass.Proposed test addition
first.addresses.append("7.7.7.7") + first.ns_names.append("evil.example.com.") second = cache.closest_delegation(dns.name.from_text("www.example.com.")) assert second is not None assert second.addresses == ["1.2.3.4"] + assert second.ns_names == ["ns1.example.com."]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_cache.py` around lines 163 - 188, The test test_a_cached_delegation_is_isolated_from_its_producer_and_its_readers must also verify reader isolation for Delegation.ns_names. Mutate first.ns_names after the initial read, then obtain the next delegation and assert its ns_names remains the original value, alongside the existing addresses assertion.Makefile (1)
72-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the
release-checkrecipe ifcheckmakeis enforced.
checkmakereports seven recipe lines forrelease-check, above itsmaxbodylengthlimit of five. Move the version and changelog checks into helper targets or a script, then keeprelease-checkas a short dependency list.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` around lines 72 - 79, Refactor the release-check target so its recipe has no more than five lines for checkmake compliance. Move the Python version-consistency and CHANGELOG validation into helper targets or a script, then make release-check depend on check-all and those helpers while preserving the existing validations and success output.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Makefile`:
- Line 78: Update the changelog validation command in the Makefile to require
the fixed version marker at the beginning of a line, ensuring only a column-1
“## [$(VERSION)]” section heading satisfies the check while preserving
fixed-string matching.
In `@src/recursive_resolver/cli.py`:
- Around line 208-209: Update _run_trace so a missing answer produces a defined
JSON failure shape instead of emitting null records and dnssec alongside
plain-text stderr; handle answer is None before the success JSON path or include
an explicit error field, preserve status 1, and add a regression test for
--trace --json with (None, []).
- Around line 129-134: Update the BrokenPipeError handler in main to store the
descriptor returned by os.open(os.devnull, os.O_WRONLY), duplicate it onto
stdout, and close it in a finally block when it differs from
sys.stdout.fileno().
In `@src/recursive_resolver/dnssec.py`:
- Around line 199-200: Update DNSSECValidator.__init__’s empty trust_anchors
error guidance to match the constructor’s actual API: retain rejection of an
empty tuple, but instruct callers to omit trust_anchors, or update
initialization to accept and normalize None before validation.
- Around line 531-532: Update prove_wildcard and its next-closer coverage loop
to reject every NSEC3 record whose iterations exceed max_nsec3_iterations, not
just the first RRset. Before adopting first-RRset parameters, validate that
salt, algorithm, and iteration values are consistent across all records; add a
multi-record wildcard NSEC3 regression test containing an over-limit later row.
---
Nitpick comments:
In `@Makefile`:
- Around line 72-79: Refactor the release-check target so its recipe has no more
than five lines for checkmake compliance. Move the Python version-consistency
and CHANGELOG validation into helper targets or a script, then make
release-check depend on check-all and those helpers while preserving the
existing validations and success output.
In `@tests/test_cache.py`:
- Around line 163-188: The test
test_a_cached_delegation_is_isolated_from_its_producer_and_its_readers must also
verify reader isolation for Delegation.ns_names. Mutate first.ns_names after the
initial read, then obtain the next delegation and assert its ns_names remains
the original value, alongside the existing addresses assertion.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5a6d6661-56a2-408c-8041-910cf1214346
📒 Files selected for processing (25)
.github/ISSUE_TEMPLATE/bug_report.yml.github/workflows/ci.ymlCONTRIBUTING.mdDockerfileMakefileREADME.mdpyproject.tomlscripts/check_authors.pyscripts/collect_domains_diverse.pyscripts/diff_harness.pyscripts/release.shsrc/recursive_resolver/budget.pysrc/recursive_resolver/cache.pysrc/recursive_resolver/cli.pysrc/recursive_resolver/dnssec.pysrc/recursive_resolver/resolver.pysrc/recursive_resolver/singleflight.pytests/conftest.pytests/test_cache.pytests/test_cli.pytests/test_dnssec.pytests/test_dnssec_signed.pytests/test_internals.pytests/test_security.pytests/test_transport.py
🚧 Files skipped from review as they are similar to previous changes (20)
- Dockerfile
- .github/ISSUE_TEMPLATE/bug_report.yml
- tests/test_dnssec_signed.py
- .github/workflows/ci.yml
- src/recursive_resolver/singleflight.py
- scripts/check_authors.py
- pyproject.toml
- src/recursive_resolver/budget.py
- CONTRIBUTING.md
- tests/test_internals.py
- scripts/diff_harness.py
- tests/test_dnssec.py
- tests/test_transport.py
- tests/test_security.py
- README.md
- tests/test_cli.py
- scripts/collect_domains_diverse.py
- scripts/release.sh
- src/recursive_resolver/cache.py
- src/recursive_resolver/resolver.py
| if not trust_anchors: | ||
| raise ValueError("trust_anchors must not be empty; pass None to use the IANA root anchors") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the empty-anchor error guidance.
DNSSECValidator.__init__ rejects None, but Line [200] tells direct callers to pass None to use the defaults. Keep the empty-tuple rejection, but say to omit trust_anchors or accept and normalize None.
Proposed fix
- raise ValueError("trust_anchors must not be empty; pass None to use the IANA root anchors")
+ raise ValueError("trust_anchors must not be empty; omit trust_anchors to use the IANA root anchors")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not trust_anchors: | |
| raise ValueError("trust_anchors must not be empty; pass None to use the IANA root anchors") | |
| if not trust_anchors: | |
| raise ValueError("trust_anchors must not be empty; omit trust_anchors to use the IANA root anchors") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/recursive_resolver/dnssec.py` around lines 199 - 200, Update
DNSSECValidator.__init__’s empty trust_anchors error guidance to match the
constructor’s actual API: retain rejection of an empty tuple, but instruct
callers to omit trust_anchors, or update initialization to accept and normalize
None before validation.
| if params.iterations > self.max_nsec3_iterations: | ||
| return False |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 10 \
'def prove_wildcard|nsec3s\[0\]\[0\]|_nsec3_covers|test_prove_wildcard_cap|nsec3s=\[' \
src/recursive_resolver/dnssec.py testsRepository: sylvinus/recursive-resolver
Length of output: 37447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== dnssec.py section =="
sed -n '360,545p' src/recursive_resolver/dnssec.py
echo
echo "== tests around wildcard NSEC3 cap =="
sed -n '740,935p' tests/test_dnssec.py
echo
echo "== search all NSEC3 caps in prove functions =="
rg -n "nsec3s\[0\]\[0\]|iterations > self.max_nsec3_iterations|_nsec3_covers|_nsec3_owner" src/recursive_resolver/dnssec.py tests/test_dnssec.pyRepository: sylvinus/recursive-resolver
Length of output: 20025
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
text = Path("src/recursive_resolver/dnssec.py").read_text()
idx = text.index("def prove_wildcard")
func = text[idx:text.index("\n def ", idx + 1)]
print("proven_wildcard_source:")
print(func)
checks = {
"checks_first_iteration_cap": "if params.iterations > self.max_nsec3_iterations",
"uses_first_params": "params = nsec3s[0][0]",
"coverage_scans_each_nsec3": "any(self._nsec3_covers(next_closer, n, params, budget) for n in nsec3s)",
"no_second_iteration_cap": "if self._nsec3_covers(next_closer, n, nsec3[0], nsec3, budget)",
}
for label, needle in checks.items():
print(f"{label}: {needle in func}")
PYRepository: sylvinus/recursive-resolver
Length of output: 2760
Reject over-limit NSEC3 rows in wildcard proofs.
prove_wildcard caps only the first NSEC3 RRset, but the next-closer coverage check iterates over all rows. A safe covering first RRset can make the proof return True even when a later NSEC3 row has too many iterations. Apply the cap to each NSEC3 record used in coverage checks, and check for inconsistent salt/algorithm/iteration values before adopting the first RRset parameters. Add a multi-record wildcard NSEC3 regression test with one over-limit row.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/recursive_resolver/dnssec.py` around lines 531 - 532, Update
prove_wildcard and its next-closer coverage loop to reject every NSEC3 record
whose iterations exceed max_nsec3_iterations, not just the first RRset. Before
adopting first-RRset parameters, validate that salt, algorithm, and iteration
values are consistent across all records; add a multi-record wildcard NSEC3
regression test containing an over-limit later row.
There was a problem hiding this comment.
Pull request overview
This PR is a broad pre-0.1.0 hardening and release-readiness pass for recursive-resolver, adding DNSSEC validation, stronger security controls (SSRF/address filtering, work budgets, downgrade resistance), improved CLI/packaging, and significantly expanded unit + integration coverage plus CI/release tooling.
Changes:
- Added core hardening primitives: per-resolution work budgets, address filtering to prevent DNS-driven SSRF, delegation/negative caching improvements, and concurrent single-flight deduplication.
- Added DNSSEC chain-of-trust support (including root trust anchors) and expanded public API/errors to make validation states and failures explicit.
- Added substantial test coverage (unit + live-DNS integration), plus CI workflows, contribution/security docs, and a hermetic release script.
Reviewed changes
Copilot reviewed 48 out of 49 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| THIRD-PARTY.md | Adds third-party attribution and derived-work notes. |
| tests/test_transport.py | New tests for transport-layer exception translation and retry logic. |
| tests/test_singleflight.py | New tests for concurrent de-duplication and resolver-level herd collapse. |
| tests/test_integration.py | Expanded live-DNS integration coverage including DNSSEC, DKIM, cache, and trace behavior. |
| tests/test_dnssec_chain.py | New detailed unit tests for DNSSEC chain plumbing and validation branches. |
| tests/test_csv.py | Improves bulk differential harness documentation and behavior (DNSSEC disabled for fair comparison). |
| tests/test_cli.py | Reworks CLI tests to match new structured output, DNSSEC notes, trace, and --text behavior. |
| tests/test_check_authors.py | Adds tests for commit-author AI-tool detection script. |
| tests/test_cache.py | Expands cache tests to cover answers, negative caching, delegation caching, and resolver integration. |
| tests/signed_zone.py | Adds an in-memory signed-zone fixture for deterministic offline DNSSEC tests. |
| tests/conftest.py | Adds shared DNS message builders and pytest options used across the expanded suite. |
| src/recursive_resolver/singleflight.py | Implements single-flight deduplication for concurrent identical resolutions. |
| src/recursive_resolver/roots.py | Adds root trust anchors alongside root hints. |
| src/recursive_resolver/py.typed | Marks the package as typed for downstream type checkers. |
| src/recursive_resolver/exceptions.py | Expands and documents the public exception hierarchy (input, DNSSEC, budget, etc.). |
| src/recursive_resolver/cli.py | Major CLI expansion: DNSSEC flags, cache controls, EDNS payload, JSON output, trace, and TXT joining. |
| src/recursive_resolver/cache.py | Reworks cache to use dns.name.Name keys, adds negative + delegation caching, and shared LRU. |
| src/recursive_resolver/budget.py | Adds shared per-resolution query/crypto work budgets (NXNSAttack/KeyTrap defenses). |
| src/recursive_resolver/addresses.py | Adds nameserver IP filtering to prevent DNS-driven SSRF via glue/NS resolution. |
| src/recursive_resolver/main.py | Fixes module entrypoint guard for python -m recursive_resolver. |
| src/recursive_resolver/init.py | Exports new public API surface (Answer, ValidationState, Limits, cache/address helpers, new errors). |
| SECURITY.md | Adds security policy + explicit threat model and mitigations. |
| scripts/release.sh | Adds an interactive hermetic Docker-based release script with artifact verification and smoke tests. |
| scripts/prepare_test_domains.py | Minor robustness/cleanup improvements when building corpora. |
| scripts/diff_harness.py | Adds standalone differential-testing harness against dig/stub resolvers across configs. |
| scripts/collect_domains_diverse.py | Adds corpus builder for diverse pathological DNS cases. |
| scripts/collect_domains_3_zone.py | Refines corpus collection strategy and data sources. |
| scripts/collect_domains_2_ct.py | Formatting/clarity changes for CT-based corpus collection. |
| scripts/collect_domains_1_tranco.py | Minor docstring punctuation/clarity update. |
| scripts/check_authors.py | Adds CI check to reject AI tools in author/co-author fields. |
| pyproject.toml | Updates packaging metadata, dependency floors/extras, sdist include list, and strict test/coverage settings. |
| Makefile | Updates dev targets, adds stricter check/check-all flows, and wires in the release script. |
| LICENSE | Updates copyright years. |
| Dockerfile | Pins uv image tag and syncs extras for reproducible dev container builds. |
| CONTRIBUTING.md | Adds detailed dev workflow, licensing guidance, AI contribution policy, and release process. |
| CHANGELOG.md | Adds initial changelog skeleton for 0.1.0. |
| .github/workflows/ci.yml | Adds CI matrix, authorship check, integration+coverage gate, and build verification. |
| .github/pull_request_template.md | Adds PR checklist aligned with security/testing/release expectations. |
| .github/ISSUE_TEMPLATE/feature_request.yml | Adds feature request template. |
| .github/ISSUE_TEMPLATE/config.yml | Configures issue templates and security contact link. |
| .github/ISSUE_TEMPLATE/bug_report.yml | Adds bug report template with trace and explicit-resolver guidance. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if call.error is not None: | ||
| # Every waiter for this key re-raises the *same* exception object, | ||
| # and each `raise` writes __traceback__ on it. Without this, several | ||
| # threads concurrently append to one traceback and a caller ends up | ||
| # inspecting frames from threads it never ran. Dropping it gives | ||
| # each waiter a traceback rooted at its own call site. | ||
| raise call.error.with_traceback(None) |
| @staticmethod | ||
| def _name(name: dns.name.Name | str) -> dns.name.Name: | ||
| if isinstance(name, dns.name.Name): | ||
| return name | ||
| return dns.name.from_text(name) | ||
|
|
| def _clamp(self, ttl: int, negative: bool = False) -> int: | ||
| ceiling = self.max_negative_ttl if negative else self.max_ttl | ||
| return max(self.min_ttl, min(int(ttl), ceiling)) | ||
|
|
Summary by CodeRabbit
New Features
Security
Documentation
Quality