[sumanjeet0012]

This PR resolves #52 by adding a spec validation test that verifies the multihash constants in constants.py and funcs.py match the authoritative upstream multicodec/table.csv.

Changes included:

• Added multicodec as a git submodule in spec/multicodec to track the authoritative upstream table.
• Created tests/test_spec.py with test_spec_table_completeness() to assert all multihash codes present in upstream table.csv are properly mapped in HASH_CODES and Func.
• Regenerated multihash/constants.py and multihash/funcs.py to sync with the latest upstream table, adding many missing hash codes and aligning exact naming strings.
• Ignored spec folder from ruff formatting, linting and mypy typechecks to preserve the upstream submodule.

[acul71]

Maintainer review (acul71)

Verdict: Request changes

Solid direction: multicodec submodule + CI submodules: true + tests/test_spec.py + synced constants.py/funcs.py. After git submodule update --init, local lint/typecheck/test (229 passed)/docs-ci are green and there are no merge conflicts with master.

Blockers

1. Missing newsfragment for #52. Please add release notes. Because this renames public hash strings (HASH_CODES["id"] → "identity", and Skein casing Skein* → skein*), use 52.breaking.rst (and optionally also 52.feature.rst for the compliance test / sync).

Major feedback

2. test_spec is one-directional — it checks CSV → local, but #52 also asks to catch extra local entries not in the CSV. Please add the reverse assertions.
3. MANIFEST.in recursive-include spec * packages the whole submodule into sdists. Runtime does not need it; consider keeping spec/ as a test/dev submodule only unless packaging is intentional.
4. Call out the breaking name renames clearly in the PR description / newsfragment. Optional: temporary id alias if you want a softer migration.

Minor

• Document git submodule update --init for contributors
• Overlaps with #61 on tests/test_funcs.py — prefer rebasing after that lands

Happy to re-review once the newsfragment and reverse spec checks are in.

[acul71]

Thanks @sumanjeet0012 for the upstream sync work!

Brief summary

• Syncs hash constants with multicodec table.csv, adds spec/multicodec submodule + test_spec (Resolves #52).
• 52.feature.rst added; lint/typecheck/tests/docs green; in sync with master.
• Still needs fixes before approve:
  1. Add newsfragments/52.breaking.rst for public string renames (id→identity, kangarootwelve→kt-128, Skein casing).
  2. Make test_spec bidirectional (catch local orphans such as murmur3 0x22/0x23).
• Verdict: Request changes (Good / Needs Work).

Full AI PR review (63-1)

AI Pull Request Review: PR #63

Review Date: 2026-10-25
Review Version: 1 (follows AI-PR-REVIEW-63-0.md)
PR Title: Fix #52: Sync hash constants with upstream and add spec compliance test
Author: sumanjeet0012 (Sumanjeet)
Status: OPEN (maintainer review: Changes requested by acul71)
URL: #63
Resolves: #52

---

1. Summary of Changes

This PR syncs local multihash hash name/code tables with the authoritative upstream multiformats/multicodec table.csv, adds that repo as a git submodule, and introduces a one-directional spec compliance test.

Changes Made

• Added: spec/multicodec git submodule (via .gitmodules)
• Added: tests/test_spec.py — asserts every CSV row with tag=multihash exists in HASH_CODES / Func with matching codes
• Added: newsfragments/52.feature.rst (new since review 0)
• Modified: multihash/constants.py — regenerated HASH_TABLE names/codes to match upstream
• Modified: multihash/funcs.py — expanded Func IntEnum to cover the full synced table (125 → 361 members)
• Modified: ruff.toml — exclude spec/ from ruff
• Modified: tests/test_funcs.py — HASH_CODES["id"] → HASH_CODES["identity"]

Issues Addressed

• #52 — No spec compliance test validating hash codes against multicodec/table.csv; manually maintained tables can drift (missing entries, wrong codes/names, extra local entries). Proposed solution: submodule + test_spec modeled on go-multihash TestSpec.

Modules / Files Affected

• multihash.constants, multihash.funcs, tests, tooling (ruff.toml), submodule metadata

Breaking Changes

Yes — public string name lookups change:

Code	Old name	New name
0x0	id	identity
0x1d01	kangarootwelve	kt-128
0xb301–0xb3e0 (224 entries)	Skein*	skein* (casing)

Also added kt-256 (0x1d02). Numeric codes and existing Func member names for previously-enumerated functions are largely stable; many formerly table-only hashes are newly exposed as Func members. Callers using old string keys with HASH_CODES[...], digest(..., "id"), "kangarootwelve", or "Skein256-256" will break.

Note vs review 0: CI submodules: recursive and MANIFEST.in graft spec are already on origin/master and are not part of this PR’s diff anymore; the new submodule still increases what graft spec packages.

---

2. Branch Sync Status and Merge Conflicts

⚠️ Checked from worktree against origin/master (default branch is master, not main).

Branch Sync Status

• Status: ℹ️ Ahead of origin/master (not behind)
• Details: branch_sync_status.txt = 0 2
  • Left (0): commits in origin/master not in HEAD → branch is 0 behind
  • Right (2): commits in HEAD not in origin/master → branch is 2 ahead (c6d9bbe, eceea1a)
• Compared to review 0: Previously diverged (9 3). Branch has been rebased/updated and is now cleanly ahead.

Merge Conflict Analysis

• Conflicts Detected: ✅ No conflicts
• Test merge of origin/master into HEAD: “Already up to date.” / === NO MERGE CONFLICTS DETECTED ===
• Overlap note: PR #61 (still OPEN) also touches tests/test_funcs.py; prefer coordinating/rebasing if that lands first.

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

---

3. Strengths

1. Addresses a real compliance gap described in #52 and aligned with go-multihash’s TestSpec approach.
2. Submodule wiring is sound: .gitmodules entry is correct; CI on master already uses actions/checkout with submodules: recursive, so test_spec can see table.csv in GitHub Actions.
3. Large, accurate name sync: After sync, all 359 tag=multihash CSV rows match local HASH_CODES names/codes (verified locally).
4. Func coverage expanded so enum members track the table, not only a subset of hashes.
5. Quality gates green locally and on CI: lint, mypy, tests, docs all pass; GitHub tox / Windows matrix checks are green.
6. Newsfragment added since review 0 (52.feature.rst), with trailing newline, and it does mention the id → identity rename.
7. Tooling exclude for spec/ in ruff.toml avoids linting upstream submodule content.

---

4. Issues Found

Critical

(None that fail local/CI builds. Process/completeness blockers remain under Major / newsfragment section.)

Major

• File: newsfragments/52.feature.rst (missing 52.breaking.rst)

• Line(s): entire file

• Issue: Release notes use .feature only. This PR renames public hash strings (id→identity, kangarootwelve→kt-128, 224× Skein*→skein*). Maintainer review explicitly requested 52.breaking.rst. The feature fragment also omits the kangarootwelve → kt-128 rename and the new kt-256 code, and frames Skein as “added” though those codes already existed (casing-only change).

• Suggestion: Add newsfragments/52.breaking.rst covering string-key renames; keep or trim 52.feature.rst for the compliance test / kt-256 / Func expansion. Update PR description to call out breaking renames clearly.

• File: tests/test_spec.py

• Line(s): 8–26

• Issue: Test is one-directional (CSV → local only). Issue #52 and go-multihash also require catching extra local entries not in the multihash-tagged CSV. Local verification shows orphans that a reverse check would fail on today:

  • 0x22 / murmur3-128 — CSV has tag=hash, name murmur3-x64-64 (not multihash, and different name)
  • 0x23 / murmur3-32 — CSV has tag=hash (not multihash)

• Suggestion: Add reverse assertions over HASH_CODES / Func (and optionally CODE_HASHES). Decide explicitly whether murmur3 stays as intentional non-spec locals (document + allowlist) or is renamed/retagged/removed to match multicodec.

• File: MANIFEST.in (pre-existing on master: graft spec) + new spec/multicodec

• Line(s): graft spec

• Issue: Packaging already grafts all of spec/. Adding multicodec (~100K) increases sdist contents; runtime does not need the CSV. Not introduced by this PR’s MANIFEST diff, but amplified by the new submodule (maintainer feedback still applies).

• Suggestion: Confirm intent. Prefer excluding spec/ from release artifacts and treating it as test/dev-only unless packaging is deliberate.

• File: PR description / user-facing docs

• Issue: Breaking renames are under-documented outside a partial newsfragment sentence; kangarootwelve→kt-128 is easy to miss.

• Suggestion: Document in PR body + breaking newsfragment; optionally temporary aliases (id, kangarootwelve, old Skein spellings) if a softer migration is desired.

Minor

• File: CONTRIBUTING.rst / README.rst

• Issue: No contributor note that git submodule update --init --recursive is required after clone for test_spec (and existing spec/multihash vectors).

• Suggestion: Add a short “Submodules” note under contributing / installation.

• File: tests/test_spec.py

• Line(s): 24–25

• Issue: values = [f.value for f in Func] is rebuilt inside the per-row loop (O(n×m)).

• Suggestion: Hoist outside the loop (or use a set).

• File: tests/test_spec.py

• Issue: Does not assert name consistency via CODE_HASHES[code] == name (only HASH_CODES[name] == code), and does not check Func member names, only code presence.

• Suggestion: Strengthen assertions if full go-parity is the goal.

• File: overlap with open PR #61

• Issue: Both touch tests/test_funcs.py; merge order may need a trivial rebase.

---

5. Security Review

No high-risk vulnerabilities introduced.

• Risk: Git submodule pulls third-party content under spec/multicodec.
  Impact: Low — not imported/executed at runtime; used for tests/reference data.
  Mitigation: Keep CI submodule checkout enabled (already on master); pin/review submodule updates in future PRs.

• Risk: Expanding Func / HASH_CODES increases the set of names/codes accepted by encode/decode APIs (including hashes the library may not implement as digesters).
  Impact: Low — expected for spec sync; invalid use should still fail where implementations are missing.
  Mitigation: Ensure registry/digest paths still reject unimplemented functions clearly (existing behavior).

• Risk: No unsafe subprocess, tempfile, or secret handling added.
  Impact: None.

Overall Security Impact: Low

---

6. Documentation and Examples

Area	Status
Docstrings / API reference	No dedicated docs updates for renamed string keys or new Func members
Newsfragment	Present as 52.feature.rst; incomplete for breaking renames (see §7)
CONTRIBUTING / README	Missing submodule init instructions
PR body	Does not clearly label breaking string renames (kangarootwelve, Skein casing)
Usage examples	Existing murmur3 / identity examples in funcs.py still use current names; no migration guide

Sphinx make docs-ci succeeds with -W. User-facing migration notes should live in the breaking newsfragment / changelog.

---

7. Newsfragment Requirement

Issue link: ✅ PR resolves #52 (mandatory issue reference satisfied).

Newsfragment present: ✅ newsfragments/52.feature.rst

• Ends with \n ✅
• User-oriented text ✅ (partially)
• Mentions id → identity ✅

Remaining blocker / process gap:

• Severity: MAJOR (maintainer-requested; release-note correctness) — treat as approval blocker per maintainer review
• Issue: Missing 52.breaking.rst for public string renames; feature fragment understates / misstates part of the user impact (kangarootwelve→kt-128 absent; Skein described as additions rather than renames).
• Impact: Towncrier would publish this primarily under Features, which is misleading for a renaming release.
• Suggestion:
  1. Add newsfragments/52.breaking.rst describing id→identity, kangarootwelve→kt-128, and Skein casing alignment.
  2. Optionally keep 52.feature.rst for the compliance test, kt-256, and Func table expansion.
• Action Required: Address before approval (aligns with acul71 “Request changes”).

---

8. Tests and Validation

Submodule initialized in worktree (git submodule update --init --recursive) before checks. Logs under downloads/AI-PR-REVIEWS/63/.

Linting (make lint)

• Exit code: 0
• Result: All pre-commit hooks passed (yaml, toml, EOF, trailing whitespace, pyupgrade, ruff, ruff format, mypy-local)
• Errors / warnings: None

Type Checking (make typecheck)

• Exit code: 0
• Result: pre-commit run mypy-local --all-files — Passed
• Errors / warnings: None

Test Execution (make test)

• Exit code: 0
• Summary: 506 passed, 10 deselected (benchmarks), 0 failed, in ~0.33s
• Includes tests/test_spec.py (1 test)
• Warnings: None reported
• Coverage: Not printed by default make test
• Gap: No reverse-direction / orphan-entry tests (see Major issues). If reverse checks were added naively, murmur3 0x22/0x23 would fail today.

Documentation Build (make docs-ci)

• Exit code: 0
• Result: sphinx-apidoc + Sphinx HTML with SPHINXOPTS="-W" succeeded
• Errors / warnings: None that failed the build (build succeeded)

CI (GitHub)

• All listed tox / Windows matrix checks for the PR: pass

---

9. Recommendations for Improvement

1. Add 52.breaking.rst (and refine 52.feature.rst) so release notes match user-visible renames — do this first.
2. Extend test_spec with reverse completeness (local → CSV), then either fix murmur3 drift or document an explicit allowlist.
3. Call out breaking renames in the PR description (id, kangarootwelve, Skein casing).
4. Document submodule init for contributors.
5. Revisit packaging of spec/ now that multicodec is included under graft spec.
6. Hoist Func value set construction out of the CSV loop in test_spec.py.
7. Coordinate with #61 on tests/test_funcs.py if that PR merges first.
8. Optional: temporary aliases for old string names to soften migration.

---

10. Questions for the Author

1. Will you add 52.breaking.rst as requested in the maintainer review, and mention kangarootwelve → kt-128?
2. How should murmur3 (0x22/0x23) be treated once reverse spec checks exist — remove, rename to CSV names, retag expectation, or allowlist as intentional extras?
3. Is shipping the full spec/multicodec tree in sdists via graft spec intentional?
4. Do you want temporary aliases for id / old Skein spellings / kangarootwelve?
5. Should test_spec also assert CODE_HASHES[code] == name and/or Func member naming conventions?

---

11. Overall Assessment

Dimension	Rating
Quality Rating	Good (still Needs Work on release notes + reverse spec coverage)
Security Impact	Low
Merge Readiness	Needs fixes
Confidence	High

Progress since review 0

• ✅ Newsfragment 52.feature.rst added
• ✅ Branch no longer behind origin/master (0 behind / 2 ahead)
• ✅ Local and CI quality gates remain green (506 tests)

Still blocking approval (aligned with maintainer)

1. 52.breaking.rst (or equivalent clear breaking release notes) for string renames
2. Reverse-direction test_spec assertions from #52 (and a decision on murmur3 orphans)
3. Prefer addressing spec packaging / contributor submodule docs

Verdict: REQUEST_CHANGES — solid technical direction and green CI, but maintainer feedback on breaking newsfragment and bidirectional spec checks remains unresolved.

---

Review artifacts

• branch_sync_status.txt, merge_conflict_check.log
• lint_output.log, typecheck_output.log, test_output.log, docs_output.log
• submodule_init.log, pr_info.txt, pr_comments.txt, issue_52.txt
• Diff base: git diff origin/master...HEAD (worktree)

---

Note on maintainer edits

This PR is from a fork with “Allow edits from maintainers” disabled (maintainerCanModify: false), so we cannot push commits or rebase on this branch for you.

For future PRs, please either:

1. Check “Allow edits and access to secrets by maintainers” when opening the PR, or
2. Open the PR from a branch on multiformats/py-multihash (if you have push access),

so maintainers can help with rebases and small follow-up fixes.