feat: port utils from go-multiaddr-dns

[asabya]

Closes #101

[seetadev]

@asabya : This is fantastic. Thank you so much for your contribution on this PR.

Ran the CI/Cd pipeline and reviewing it.

CCing @acul71, @yashksaini-coder and @lla-dane on this PR for their review and feedback too.

[seetadev]

@asabya : CI/CD tests are failing. Please make corrections.

[asabya]

@seetadev fixed

[yashksaini-coder]

What's has changed

@asabya has add 6 new utility functions to make DNS resolution easier:

• is_fqdn() - Check if domain ends with a dot
• fqdn() - Add trailing dot if missing
• addr_len() - Count protocols in a multiaddr
• offset_addr() - Skip first N protocols
• matches() - Check if multiaddr has DNS in it
• resolve_all() - Resolve all DNS parts (handles recursion)

These are pulled from go-multiaddr-dns so Python users can access them instead of reimplementing.

---

What's Working Well ✅

• Great test coverage: 55 new tests, all passing. Tests cover edge cases (recursion limits, empty results, etc.)
• No regressions: All 362 existing tests still pass
• Clean code: Utilities are focused and single-responsibility. FQDN validation handles escaped dots correctly
• Good refactoring: Extracted DNSADDR_TXT_PREFIX constant reduces duplication
• Exports are clear: All functions properly documented in __all__

---

Security Review ✅

Status Good: No security vulnerabilities found.

• FQDN validation handles escaped characters correctly
• resolve_all() has recursion limits (max 32 iterations) to prevent DoS
• No unsafe type conversions
• DNS logic uses existing, well-tested DNSResolver

---

Test Results

Check	Result	Details
Linting (ruff)	✅ PASS	No errors or warnings
Type Checking (mypy)	✅ PASS	No errors
Tests (pytest)	✅ PASS	362 passed, 1 skipped
Docs Build	✅ PASS	no errors

Pyupgrade pre-commit hook fails due to Python 3.14 compatibility issue, must be fixed before merge.

Tests passed: All 362 tests pass with 0 failures. Great! But type checking must be fixed.

---

Pre-commit Hook Failure ⚠️

CRITICAL: Running make pr fails on pyupgrade pre-commit hook.

Error Output:

pyupgrade................................................................Failed
- hook id: pyupgrade
- exit code: 1

TypeError: cannot use a bytes pattern on a string-like object
  File "/home/yks/.cache/pre-commit/repotbcu8wk7/py_env-python3.14/lib/python3.14/site-packages/pyupgrade/_main.py", line 297, in _fix_tokens
    tokenize.cookie_re.match(token.src)
    ~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^
TypeError: cannot use a bytes pattern on a string-like object

What this means: The pyupgrade tool crashes when processing files in this PR. This is a Python 3.14 compatibility issue with the pyupgrade library. The tool runs 5+ times and fails each time with the same error.

Affected: make pr command cannot complete (this is the pre-merge check)

How to fix:

1. Check if any files have encoding issues or invalid Python syntax markers
2. Or: Temporarily skip pyupgrade in .pre-commit-config.yaml and re-run
3. Or: File an issue with pyupgrade library (Python 3.14 bug in tokenize.cookie_re)

Status: Must be resolved before PR is ready. This is a blocker.

---

What You Need to Do 📝

Must Fix (To Merge)

• Fix pyupgrade pre-commit hook failure - Check files for encoding issues or skip pyupgrade hook
• Create newsfragment file at newsfragments/NNN.feature.rst
• Add -> None to all test function signatures in tests/test_resolvers.py
• Run make pr again to verify all checks pass

Should Fix (Before Approval)

• Improve docstrings with parameter descriptions, return values, and examples
• Add comment explaining test case changes on lines 70, 144-145 in tests/test_multiaddr.py (these were modified to fix pyupgrade issues)

---

Summary

Code quality: Excellent ✅ (well-designed, thoroughly tested, no regressions)

Blockers: 2 issues ❌

• No linked newsfragment (project requirement)
• Sparse docstrings (needs examples)

Bottom line: Code is great, but need to fix pre-commit issues, follow process (issue + newsfragment + type annotations) before merge.

[acul71]

Thank you for this contribution, @asabya . Great work porting the resolver utility helpers from go-multiaddr-dns and backing them with strong test coverage. This improves parity across implementations and makes DNS resolution workflows easier for Python users.

Before merge, a few small fixes are still needed:

• Add a required newsfragment file using the linked issue number: newsfragments/101.feature.rst. (read https://github.com/multiformats/py-multiaddr/blob/master/newsfragments/README.md )
• Add a proper PR body/description and link the issue with Fixes #101.
• Add lightweight user-facing docs/example coverage for the new public helpers (especially resolve_all() and matches()).
• (Recommended) Guard offset_addr() against negative offsets (n < 0) and add one test for that edge case.

Thanks again for the thoughtful PR and follow-up fixes.

Full review here:

AI PR Review: #102

1. Summary of Changes

• Type of change: Feature + tests + minor internal refactor.
• Affected modules:
  • multiaddr/resolvers/ (util.py added, dns.py updated, resolver exports expanded)
  • tests/ (tests/test_resolvers.py adds substantial Go-parity coverage)
  • tests/test_multiaddr.py (certhash validity expectations updated)
  • multiaddr/codecs/certhash.py (typing cast fix)
• PR intent: Port utility helpers from go-multiaddr-dns (is_fqdn, fqdn, addr_len, offset_addr, matches, resolve_all) and increase test parity.
• Linked issues/discussions: Issue #101 exists for this work, but it is not currently referenced in the PR body/metadata (recommend adding Fixes #101).
• Breaking changes/deprecations: None identified.

---

2. Branch Sync Status and Merge Conflicts

• Branch status vs origin/master: ahead by 2, behind by 0 (0 2 from git rev-list --left-right --count origin/master...HEAD).
• Conflict status: No conflicts detected.

✅ No merge conflicts detected. The PR branch can be merged cleanly into origin/master.

---

3. Strengths

• Utility extraction from Go implementation is clean and focused; each helper has a single clear purpose.
• Test coverage is significantly improved, especially for resolver behavior parity and recursive DNS resolution paths.
• DNS TXT parsing in DNSResolver avoids hardcoded magic slices by introducing DNSADDR_TXT_PREFIX.
• Validation checks are green in this environment:
  • make lint: pass
  • make typecheck: pass
  • make test: pass (362 passed, 1 skipped)
  • make docs-ci: pass
• resolve_all() includes a recursion-limit guard and raises RecursionLimitError on unresolved recursive expansion.

---

4. Issues Found

Critical

• None found.

Major

• File: newsfragments/
• Line(s): N/A (missing file)
• Issue: No PR newsfragment was added. Project process requires a newsfragment for merge readiness.
• Suggestion: Add newsfragments/101.feature.rst with user-visible summary.

Minor

• File: multiaddr/resolvers/util.py

• Line(s): 50-55

• Issue: offset_addr() accepts negative n values and returns unintuitive results due to Python negative indexing (parts[n]), instead of rejecting invalid input.

• Suggestion: Validate n >= 0 and raise ValueError for negatives; add tests for n < 0.

• File: multiaddr/resolvers/util.py, docs/ (no corresponding docs edits)

• Line(s): N/A

• Issue: New public resolver utility APIs are exported from multiaddr.resolvers but have no user-facing docs/examples update.

• Suggestion: Add a short docs section (or README note) describing when to use these helpers, especially resolve_all().

---

5. Security Review

• Risk: Recursive DNS expansion can be abused for unbounded chaining.

• Impact: Low (mitigated).

• Mitigation: resolve_all() caps iterations (max_iterations=32) and raises RecursionLimitError.

• Risk: Parsing DNS TXT records from external input.

• Impact: Low.

• Mitigation: Existing resolver parsing remains constrained to dnsaddr= records; no unsafe subprocess/filesystem behavior introduced.

• Risk: Sensitive data leakage.

• Impact: Low.

• Mitigation: No new secret-handling paths introduced; logging usage did not expand in a way that exposes credentials.

Overall security impact: Low / no new high-risk findings.

---

6. Documentation and Examples

• make docs-ci passes successfully.
• Public API surface changed (multiaddr.resolvers now exports six new utilities + DNSADDR_TXT_PREFIX), but no explicit docs/examples were added for user discoverability.
• Recommendation: add lightweight narrative docs (not a full tutorial), at minimum:
  • one concrete example flow using matches() + resolve_all() on a chained DNS multiaddr
  • clear resolve_all() semantics (iterative behavior + recursion limit)
  • intended audience split: practical helpers (resolve_all, matches) vs lower-level primitives (addr_len, offset_addr)

---

7. Newsfragment Requirement

• Status: ❌ Missing.
• Required naming format: <NUMBER>.<TYPE>.rst.
• For this change, use the linked issue number: 101.feature.rst.
• Ensure file ends with newline and contains user-facing reStructuredText.

---

8. Tests and Validation

• Tests added/updated for changed behavior: Yes (substantial additions in resolver tests, including edge cases and recursion limits).
• Success + failure/edge coverage: Good.
• Lint/type/tests/docs:
  • make lint: pass
  • make typecheck: pass
  • make test: pass (362 passed, 1 skipped)
  • make docs-ci: pass
• Logs reviewed for warnings/errors: no actionable warnings or failures in captured runs.

---

9. Recommendations for Improvement

• Add required newsfragment before merge (newsfragments/101.feature.rst).
• Add negative-argument guard in offset_addr() and test coverage for invalid n.
• Add minimal user-facing docs for newly exported resolver utilities (is_fqdn, fqdn, addr_len, offset_addr, matches, resolve_all), including one small runnable example in docs/examples.rst or examples/.
• Expand utility docstrings with concise argument/return details and edge-case notes (offset_addr bounds, resolve_all recursion-limit behavior).

---

10. Questions for the Author

• Should offset_addr() explicitly reject negative offsets to avoid surprising Python-index behavior?
• Do you want resolve_all() documented as a supported public API in README/docs, or kept as an advanced/internal helper despite export?
• Please link the existing issue in the PR body with Fixes #101 to align tracking and release-note metadata.

---

11. Overall Assessment

• Quality Rating: Good
• Security Impact: Low
• Merge Readiness: Needs fixes (missing newsfragment)
• Confidence: High

[acul71]

Pyupgrade pre-commit hook fails due to Python 3.14 compatibility issue, must be fixed before merge.

Hi @yashksaini-coder, python 3.14 is not currently supported in Repo setup , see pyproject.toml and tox.yml

[acul71]

@asabya Can you fix the lint errors?

AI PR Review: #102 (CI/CD Failure Focus)

Critical

• None.

Major

• File: multiaddr/resolvers/util.py (docstring for resolve_all)
• Issue: Sphinx/docutils raises ERROR: Unexpected indentation when rendering this docstring.
• Evidence: failing checks tox (3.10, docs) and all failing windows matrix jobs show:
  • ...multiaddr/resolvers/__init__.py:docstring of multiaddr.resolvers.util.resolve_all:10: ERROR: Unexpected indentation. [docutils]
• Why CI/CD fails: windows jobs execute tox environments that also run docs; once docs fails, the whole windows job fails.
• Suggestion: normalize the resolve_all() docstring indentation (especially the Example:: block), then rerun docs and matrix checks.

Minor

• Node 20 deprecation warnings are present in check annotations (actions/checkout@v4, actions/setup-python@v5) but are non-blocking for this PR.

6. Documentation and Examples

• Docs/examples were added, but the doc build currently fails due to docstring formatting in resolver utility docs.

7. Newsfragment Requirement

• newsfragments/101.feature.rst is present in this PR (good).

8. Tests and Validation

• core tests pass in CI matrix.
• lint tooling (including pyupgrade) passes in current failing windows logs.
• Failing checks are docs-driven, not pyupgrade-driven.

9. Recommendations for Improvement

• Fix resolve_all docstring indentation, then rerun:
  • tox -e docs (or make docs-ci)
  • full CI matrix
• Keep this change minimal and avoid touching resolver logic unless needed.

10. Questions for the Author

• Was the resolve_all docstring recently edited with mixed indentation (tabs/spaces or uneven example indentation)?

11. Overall Assessment

• Quality Rating: Good
• Security Impact: Low
• Merge Readiness: Needs fixes
• Confidence: High (root cause confirmed from failed job logs and check runs)

[yashksaini-coder]

Pyupgrade pre-commit hook fails due to Python 3.14 compatibility issue, must be fixed before merge.

Hi @yashksaini-coder, python 3.14 is not currently supported in Repo setup , see pyproject.toml and tox.yml

Aah apologies for this, I had 3.14 set on main uv environment so the review draft included that as well.

[acul71]

Aah apologies for this, I had 3.14 set on main uv environment so the review draft included that as well.

It happened also to me. install venv with uv venv venv --python 3.13

[acul71]

LGTM!