[sumanjeet0012]

Description

Closes #46.

This pull request addresses issue, where get_prefix was returning an incomplete/incorrect prefix for hash functions utilizing multi-byte varint codes (e.g., hash functions with codes > 0x7F like sha2-224, blake2b-*, etc.).

The original code assumed a hardcoded 2-byte prefix size, expecting exactly one byte for the code varint and exactly one byte for the length varint.

Proposed Changes

• Updates get_prefix to properly decode both the hash code varint and the digest length varint from the input stream using BytesIO, guaranteeing accuracy regardless of varint length.
• Adds test cases (test_get_prefix_multi_byte_code & test_get_prefix_type_error) ensuring accuracy against multi-byte codes (using sha2_224 as an example).

[acul71]

Maintainer review (acul71)

Verdict: Request changes

The get_prefix fix looks correct (decode both varints via BytesIO / varint.decode_stream) and addresses #46. Local lint/typecheck/test (230 passed)/docs-ci are green, and there are no merge conflicts with master.

Blocker

• Missing newsfragment: please add newsfragments/46.bugfix.rst (user-facing description, trailing newline).

Optional

• Add a second multi-byte test (e.g. blake2b-256, 3-byte code varint) as cited in #46
• Align TypeError message with decode to include the offending type

Please push the newsfragment and we can re-review for approval.

[acul71]

Thanks @sumanjeet0012 for this fix!

Brief summary

• Correctly decodes multi-byte varints in get_prefix() instead of a hardcoded 2-byte slice (Closes #46).
• Prior blocker fixed: valid newsfragments/46.bugfix.rst present; targeted tests added.
• Lint/typecheck/tests/docs green; no merge conflicts (branch ~23 behind — rebase recommended).
• Verdict: Approve / Ready (Excellent).

Full AI PR review (62-1)

AI Pull Request Review: PR #62

Review Date: 2026-10-25
PR Title: Fix: decode multi-byte varints correctly in get_prefix
Author: sumanjeet0012 (Sumanjeet)
Reviewer (GitHub): acul71 (Changes requested — prior review; newsfragment since added)
Status: OPEN
URL: #62
Review version: 1 (follows AI-PR-REVIEW-62-0.md)

---

1. Summary of Changes

This PR is a bug fix for get_prefix(), which previously returned a hardcoded multihash[:2] slice. That assumed both the hash-code and digest-length varints were single bytes, which is wrong for hash codes above 0x7F (multi-byte varints).

Issues Addressed

• Closes #46 — get_prefix() hardcodes a 2-byte prefix and returns incomplete/incorrect prefixes for multi-byte varint codes (e.g. sha2-224 at 0x1013, blake2b-256 at 0xB220, ripemd-160, etc.).

What Changed

• multihash/multihash.py (get_prefix): Validate bytes (raise TypeError), validate via is_valid (raise ValueError), then decode both code and length varints with BytesIO + varint.decode_stream, and return multihash[:buffer.tell()]. Docstring updated with :raises TypeError.
• tests/test_multihash.py: Adds test_get_prefix_multi_byte_code (sha2_224, 2-byte code + 1-byte length → 3-byte prefix) and test_get_prefix_type_error.
• newsfragments/46.bugfix.rst: User-facing bugfix note (added in commit 04573a0 after the prior review’s blocker).

Modules / Files Affected

• multihash/multihash.py
• tests/test_multihash.py
• newsfragments/46.bugfix.rst

Breaking Changes / Deprecations

• No intentional API break. For single-byte codes (≤ 0x7F) behavior remains equivalent. For multi-byte codes, callers that relied on the incorrect 2-byte slice will now receive the correct longer prefix — that is a correctness fix, not a deprecation.

---

2. Branch Sync Status and Merge Conflicts

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

Branch Sync Status

• Status: ⚠️ Diverged from origin/master
• Details: branch_sync_status.txt reports 23 2
  • Left (23): commits on origin/master not in HEAD — branch is 23 commits behind
  • Right (2): commits on HEAD not in origin/master — branch is 2 commits ahead
    • fba7332 Fix get_prefix logic for multi-byte varints
    • 04573a0 Add newsfragment for #46
• Behind commits include unrelated features/deps (#59, #64–#79, etc.). Notably, #59 fixed decode()’s broken TypeError format string on master; this PR branch still has the older decode() message shape, which is relevant only for optional consistency of get_prefix’s TypeError (see Minor issues).

Merge Conflict Analysis

• Conflicts Detected: ✅ No conflicts
• Dry-run git merge --no-commit --no-ff origin/master succeeded (MERGE_EXIT_CODE=0).
• Log notes auto-merge of multihash/multihash.py and tests/test_multihash.py without conflicts; merge was aborted to restore a clean worktree.

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

Note: Being 23 commits behind is not a merge blocker given a clean test merge, but rebasing onto current master before merge is recommended so CI and local checks match the tip of the default branch.

---

3. Strengths

1. Correct fix aligned with #46 — decodes both varints and slices by stream position instead of a fixed 2-byte assumption.
2. Reuses established patterns — BytesIO + varint.decode_stream already used in decode() / related paths.
3. Clear error contract — explicit TypeError for non-bytes and ValueError for invalid multihashes; docstring documents both.
4. Targeted regression test — sha2_224 covers the multi-byte code case called out in the issue.
5. Valid newsfragment — 46.bugfix.rst is present, user-focused, correctly typed, and ends with a newline (addresses the prior review’s CRITICAL blocker and maintainer request).
6. Local quality gates green — lint, typecheck, 230 tests, docs-ci (SPHINXOPTS=-W) all pass in this review environment.

---

4. Issues Found

Critical

None. The previous CRITICAL (missing newsfragment) is resolved.

Major

None.

Minor

- **File:** tests/test_multihash.py
- **Line(s):** test_get_prefix_multi_byte_code
- **Issue:** Only covers a 2-byte code varint (`sha2_224` → 3-byte prefix). Issue #46 also cites `blake2b-256` (3-byte code varint, 4-byte prefix). Maintainer review listed this as optional.
- **Suggestion:** Add a second case with `Func.blake2b_256` asserting a 4-byte prefix (`b'\xa0\xe4\x02\x20'` pattern / `encode()[:4]`), to lock in the longer varint path.

- **File:** multihash/multihash.py
- **Line(s):** get_prefix TypeError (~831–832)
- **Issue:** Message is `"multihash should be bytes"` without the offending type. On current `master`, `decode()` was fixed in #59 to include the type; aligning `get_prefix` would improve consistency. Maintainer listed this as optional.
- **Suggestion:** Prefer `raise TypeError(f"multihash should be bytes, not {type(multihash)}")` to match the fixed `decode()` style after rebase onto `master`.

- **File:** (branch sync)
- **Issue:** Branch is 23 commits behind `origin/master`.
- **Suggestion:** Rebase or merge `origin/master` before final merge so CI runs against current tip; not a functional conflict today.

---

5. Security Review

No security vulnerabilities identified.

• Input is validated (isinstance(..., bytes) then is_valid).
• Parsing uses the same varint stream decoding as the rest of the library; no new subprocess, filesystem, or crypto surface.
• Exceptions do not expose sensitive data.

- **Risk:** None specific to this change
- **Impact:** None
- **Mitigation:** N/A — correctness-oriented fix with tighter type checking

---

6. Documentation and Examples

• Docstring for get_prefix updated with :raises TypeError and existing :raises ValueError.
• No README / usage-guide changes required; behavior for the documented “prefix” concept is corrected rather than newly introduced as a public API surface.
• Newsfragment provides the user-facing changelog entry for towncrier.
• Docs build succeeds under -W (warnings as errors).

No documentation gaps that block approval.

---

7. Newsfragment Requirement

✅ Satisfied.

Check	Result
Linked issue	Closes #46
File present	newsfragments/46.bugfix.rst
Naming	<ISSUE_NUMBER>.<TYPE>.rst → 46.bugfix.rst
Type	bugfix (correct for this fix)
Content	User-facing ReST describing correct prefixes for codes > 0x7F and new TypeError
Trailing newline	Present (\n verified)

Prior review (62-0): Missing newsfragment was a CRITICAL BLOCKER.
This review (62-1): Blocker cleared by commit 04573a0.

Only one issue is referenced; one newsfragment is appropriate.

---

8. Tests and Validation

Commands run from worktree
/home/luca/.../py-multihash/.worktrees/pr-62
with source .venv/bin/activate, logs teed to
downloads/AI-PR-REVIEWS/62/.

New / updated tests

• GetPrefixTestCase.test_get_prefix_multi_byte_code — multi-byte code path (sha2_224).
• GetPrefixTestCase.test_get_prefix_type_error — non-bytes → TypeError.
• Existing valid/invalid prefix tests retained.
• Gap (minor): no 3-byte-code (blake2b-256) case yet.

Linting (make lint) — exit 0

• pre-commit: check yaml/toml, end-of-file, trailing whitespace, pyupgrade, ruff, ruff format, mypy-local — all Passed
• No errors, warnings, or skipped hooks in lint_output.log

Type Checking (make typecheck) — exit 0

• pre-commit run mypy-local --all-files — Passed
• No type errors or warnings in typecheck_output.log

Test Execution (make test) — exit 0

• 230 passed in ~0.16s
• No failures, errors, skipped tests, or collection issues
• Suites: test_funcs.py, test_multihash.py, test_new_features.py
• No deprecation/resource warnings reported in the captured summary

Documentation Build (make docs-ci) — exit 0

• sphinx-apidoc + make -C docs html SPHINXOPTS="-W"
• Sphinx 9.1.0 — build succeeded with warnings-as-errors
• No build errors or warnings in docs_output.log

---

9. Recommendations for Improvement

1. Optional: Add a blake2b-256 (or other 3-byte code) get_prefix test as in #46 / maintainer feedback.
2. Optional: After rebase onto master, align get_prefix’s TypeError message with the #59-fixed decode() message (include type(multihash)).
3. Suggested hygiene: Rebase/merge origin/master (23 commits behind) before merge so CI matches current default branch.
4. No further newsfragment or issue-linking work required.

---

10. Questions for the Author

1. Will you add a second multi-byte test (e.g. blake2b-256) before merge, or leave that for a follow-up?
2. After rebasing onto master (post-#59), do you want get_prefix’s TypeError to include the offending type for consistency with decode?
3. Any known callers that depended on the incorrect 2-byte slice for multi-byte codes that should be called out in release notes beyond the newsfragment?

---

11. Overall Assessment

Dimension	Rating
Quality Rating	Excellent
Security Impact	None
Merge Readiness	Ready (optional polish only; rebase onto master recommended)
Confidence	High

Delta vs review 62-0

Item	62-0	62-1
Newsfragment	CRITICAL BLOCKER	✅ Fixed
Merge readiness	Needs fixes	Ready
Maintainer request	Request changes (newsfragment)	Newsfragment present; remaining items were optional
Behind master	9 behind / 1 ahead	23 behind / 2 ahead; still no conflicts

Verdict: Approve once optional items are accepted as follow-ups (or landed). The #46 fix and mandatory newsfragment are in place; local lint/typecheck/test/docs-ci are green; no merge conflicts with origin/master.

---

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.

Superseded: approval was incorrect while branch is behind master. Replacing with a single request-changes review.

[acul71]

Thanks @sumanjeet0012 for this fix!

Brief summary

• Correctly decodes multi-byte varints in get_prefix() instead of a hardcoded 2-byte slice (Closes #46).
• Prior newsfragment blocker fixed: valid newsfragments/46.bugfix.rst present; targeted tests added.
• Lint/typecheck/tests/docs were green on the PR tip, and there are no text merge conflicts with master.

Not merge-ready yet (blocker)

The branch must be rebased onto current master and CI must re-run green on that tip before merge. Being ~23+ commits behind is a merge gate — a clean auto-merge does not guarantee the combined tree is correct.

We cannot rebase this branch for you because “Allow edits from maintainers” is disabled (maintainerCanModify: false). Please either:

1. Rebase onto master yourself and push, or
2. Enable “Allow edits and access to secrets by maintainers” so we can rebase/help,

then we will re-review for approval/merge.

Full AI PR review (62-1)

AI Pull Request Review: PR #62

Review Date: 2026-10-25
PR Title: Fix: decode multi-byte varints correctly in get_prefix
Author: sumanjeet0012 (Sumanjeet)
Reviewer (GitHub): acul71 (Changes requested — prior review; newsfragment since added)
Status: OPEN
URL: #62
Review version: 1 (follows AI-PR-REVIEW-62-0.md)

---

1. Summary of Changes

This PR is a bug fix for get_prefix(), which previously returned a hardcoded multihash[:2] slice. That assumed both the hash-code and digest-length varints were single bytes, which is wrong for hash codes above 0x7F (multi-byte varints).

Issues Addressed

• Closes #46 — get_prefix() hardcodes a 2-byte prefix and returns incomplete/incorrect prefixes for multi-byte varint codes (e.g. sha2-224 at 0x1013, blake2b-256 at 0xB220, ripemd-160, etc.).

What Changed

• multihash/multihash.py (get_prefix): Validate bytes (raise TypeError), validate via is_valid (raise ValueError), then decode both code and length varints with BytesIO + varint.decode_stream, and return multihash[:buffer.tell()]. Docstring updated with :raises TypeError.
• tests/test_multihash.py: Adds test_get_prefix_multi_byte_code (sha2_224, 2-byte code + 1-byte length → 3-byte prefix) and test_get_prefix_type_error.
• newsfragments/46.bugfix.rst: User-facing bugfix note (added in commit 04573a0 after the prior review’s blocker).

Modules / Files Affected

• multihash/multihash.py
• tests/test_multihash.py
• newsfragments/46.bugfix.rst

Breaking Changes / Deprecations

• No intentional API break. For single-byte codes (≤ 0x7F) behavior remains equivalent. For multi-byte codes, callers that relied on the incorrect 2-byte slice will now receive the correct longer prefix — that is a correctness fix, not a deprecation.

---

2. Branch Sync Status and Merge Conflicts

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

Branch Sync Status

• Status: ⚠️ Diverged from origin/master
• Details: branch_sync_status.txt reports 23 2
  • Left (23): commits on origin/master not in HEAD — branch is 23 commits behind
  • Right (2): commits on HEAD not in origin/master — branch is 2 commits ahead
    • fba7332 Fix get_prefix logic for multi-byte varints
    • 04573a0 Add newsfragment for #46
• Behind commits include unrelated features/deps (#59, #64–#79, etc.). Notably, #59 fixed decode()’s broken TypeError format string on master; this PR branch still has the older decode() message shape, which is relevant only for optional consistency of get_prefix’s TypeError (see Minor issues).

Merge Conflict Analysis

• Conflicts Detected: ✅ No conflicts
• Dry-run git merge --no-commit --no-ff origin/master succeeded (MERGE_EXIT_CODE=0).
• Log notes auto-merge of multihash/multihash.py and tests/test_multihash.py without conflicts; merge was aborted to restore a clean worktree.

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

Note: Being 23 commits behind is not a merge blocker given a clean test merge, but rebasing onto current master before merge is recommended so CI and local checks match the tip of the default branch.

---

3. Strengths

1. Correct fix aligned with #46 — decodes both varints and slices by stream position instead of a fixed 2-byte assumption.
2. Reuses established patterns — BytesIO + varint.decode_stream already used in decode() / related paths.
3. Clear error contract — explicit TypeError for non-bytes and ValueError for invalid multihashes; docstring documents both.
4. Targeted regression test — sha2_224 covers the multi-byte code case called out in the issue.
5. Valid newsfragment — 46.bugfix.rst is present, user-focused, correctly typed, and ends with a newline (addresses the prior review’s CRITICAL blocker and maintainer request).
6. Local quality gates green — lint, typecheck, 230 tests, docs-ci (SPHINXOPTS=-W) all pass in this review environment.

---

4. Issues Found

Critical

None. The previous CRITICAL (missing newsfragment) is resolved.

Major

None.

Minor

- **File:** tests/test_multihash.py
- **Line(s):** test_get_prefix_multi_byte_code
- **Issue:** Only covers a 2-byte code varint (`sha2_224` → 3-byte prefix). Issue #46 also cites `blake2b-256` (3-byte code varint, 4-byte prefix). Maintainer review listed this as optional.
- **Suggestion:** Add a second case with `Func.blake2b_256` asserting a 4-byte prefix (`b'\xa0\xe4\x02\x20'` pattern / `encode()[:4]`), to lock in the longer varint path.

- **File:** multihash/multihash.py
- **Line(s):** get_prefix TypeError (~831–832)
- **Issue:** Message is `"multihash should be bytes"` without the offending type. On current `master`, `decode()` was fixed in #59 to include the type; aligning `get_prefix` would improve consistency. Maintainer listed this as optional.
- **Suggestion:** Prefer `raise TypeError(f"multihash should be bytes, not {type(multihash)}")` to match the fixed `decode()` style after rebase onto `master`.

- **File:** (branch sync)
- **Issue:** Branch is 23 commits behind `origin/master`.
- **Suggestion:** Rebase or merge `origin/master` before final merge so CI runs against current tip; not a functional conflict today.

---

5. Security Review

No security vulnerabilities identified.

• Input is validated (isinstance(..., bytes) then is_valid).
• Parsing uses the same varint stream decoding as the rest of the library; no new subprocess, filesystem, or crypto surface.
• Exceptions do not expose sensitive data.

- **Risk:** None specific to this change
- **Impact:** None
- **Mitigation:** N/A — correctness-oriented fix with tighter type checking

---

6. Documentation and Examples

• Docstring for get_prefix updated with :raises TypeError and existing :raises ValueError.
• No README / usage-guide changes required; behavior for the documented “prefix” concept is corrected rather than newly introduced as a public API surface.
• Newsfragment provides the user-facing changelog entry for towncrier.
• Docs build succeeds under -W (warnings as errors).

No documentation gaps that block approval.

---

7. Newsfragment Requirement

✅ Satisfied.

Check	Result
Linked issue	Closes #46
File present	newsfragments/46.bugfix.rst
Naming	<ISSUE_NUMBER>.<TYPE>.rst → 46.bugfix.rst
Type	bugfix (correct for this fix)
Content	User-facing ReST describing correct prefixes for codes > 0x7F and new TypeError
Trailing newline	Present (\n verified)

Prior review (62-0): Missing newsfragment was a CRITICAL BLOCKER.
This review (62-1): Blocker cleared by commit 04573a0.

Only one issue is referenced; one newsfragment is appropriate.

---

8. Tests and Validation

Commands run from worktree
/home/luca/.../py-multihash/.worktrees/pr-62
with source .venv/bin/activate, logs teed to
downloads/AI-PR-REVIEWS/62/.

New / updated tests

• GetPrefixTestCase.test_get_prefix_multi_byte_code — multi-byte code path (sha2_224).
• GetPrefixTestCase.test_get_prefix_type_error — non-bytes → TypeError.
• Existing valid/invalid prefix tests retained.
• Gap (minor): no 3-byte-code (blake2b-256) case yet.

Linting (make lint) — exit 0

• pre-commit: check yaml/toml, end-of-file, trailing whitespace, pyupgrade, ruff, ruff format, mypy-local — all Passed
• No errors, warnings, or skipped hooks in lint_output.log

Type Checking (make typecheck) — exit 0

• pre-commit run mypy-local --all-files — Passed
• No type errors or warnings in typecheck_output.log

Test Execution (make test) — exit 0

• 230 passed in ~0.16s
• No failures, errors, skipped tests, or collection issues
• Suites: test_funcs.py, test_multihash.py, test_new_features.py
• No deprecation/resource warnings reported in the captured summary

Documentation Build (make docs-ci) — exit 0

• sphinx-apidoc + make -C docs html SPHINXOPTS="-W"
• Sphinx 9.1.0 — build succeeded with warnings-as-errors
• No build errors or warnings in docs_output.log

---

9. Recommendations for Improvement

1. Optional: Add a blake2b-256 (or other 3-byte code) get_prefix test as in #46 / maintainer feedback.
2. Optional: After rebase onto master, align get_prefix’s TypeError message with the #59-fixed decode() message (include type(multihash)).
3. Suggested hygiene: Rebase/merge origin/master (23 commits behind) before merge so CI matches current default branch.
4. No further newsfragment or issue-linking work required.

---

10. Questions for the Author

1. Will you add a second multi-byte test (e.g. blake2b-256) before merge, or leave that for a follow-up?
2. After rebasing onto master (post-#59), do you want get_prefix’s TypeError to include the offending type for consistency with decode?
3. Any known callers that depended on the incorrect 2-byte slice for multi-byte codes that should be called out in release notes beyond the newsfragment?

---

11. Overall Assessment

Dimension	Rating
Quality Rating	Excellent
Security Impact	None
Merge Readiness	Ready (optional polish only; rebase onto master recommended)
Confidence	High

Delta vs review 62-0

Item	62-0	62-1
Newsfragment	CRITICAL BLOCKER	✅ Fixed
Merge readiness	Needs fixes	Ready
Maintainer request	Request changes (newsfragment)	Newsfragment present; remaining items were optional
Behind master	9 behind / 1 ahead	23 behind / 2 ahead; still no conflicts

Verdict: Approve once optional items are accepted as follow-ups (or landed). The #46 fix and mandatory newsfragment are in place; local lint/typecheck/test/docs-ci are green; no merge conflicts with origin/master.