Up to standards ✅

_{NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.}

Code Review

Overview

This PR fixes a real data-corruption bug in Simple8bCodec.encode(): zigzagEncode(Long.MAX_VALUE) returns -2 and zigzagEncode(Long.MIN_VALUE) returns -1, both negative in signed arithmetic. The old guard encoded > MAX_ZIGZAG_VALUE was always false for these inputs (large positive vs. negative), silently bypassing validation and masking away the top 4 bits. The one-line fix is correct and the root cause analysis is well documented.

Fix Correctness - Looks Good

The Long.compareUnsigned approach is correct. An equivalent alternative worth noting is:

if (encoded < 0 || encoded > MAX_ZIGZAG_VALUE)

This also works because valid zigzag-encoded values (for inputs within the supported range) are always non-negative — a negative encoded value is itself proof that the original value was out of range. That said, Long.compareUnsigned is equally correct, arguably more explicit about the bit-pattern semantics, and consistent with the variable name. No objection either way.

Tests - Good Coverage, Minor Wording Nit

Three focused regression tests are added and they cover the right cases. One minor comment:

The Javadoc on nearLongMaxValueThrows() says "values just inside the extreme" which is ambiguous - a reader could parse "just inside the extreme" as "just inside the valid range" (i.e., a boundary value that should be accepted). The intent is "just below Long.MAX_VALUE". A small rewording like "values just below Long.MAX_VALUE" would be clearer.

There is also a minor inconsistency between the PR description's Changes section (which names the test extremeNegativeBoundaryStillThrows) and the actual test method name (nearLongMaxValueThrows). Not a code issue, but worth correcting in the PR body for traceability.

Production Impact - Worth Calling Out Explicitly

The PR body mentions this in the impact analysis section, but it is worth reiterating: nanosecond timestamps in the range (2^59, Long.MAX_VALUE] (i.e., > ~576 years from epoch) were silently corrupted before this fix and will now throw IllegalArgumentException. This is the correct behavior (fail-fast beats silent corruption), but any caller that passes raw nanosecond timestamps without delta-encoding should be aware that this guard may now surface in practice if deltas are large. If there is a higher-level codec layer (e.g. DeltaOfDeltaCodec) that feeds into Simple8bCodec, confirming that its output deltas stay within [-(2^59), (2^59)-1] would be a good follow-up.

No Issues

• Single-line change, minimal blast radius.
• Long.compareUnsigned is available since Java 8, well within the Java 21 baseline.
• The docs/ pattern (per-issue markdown files) matches other files already in that directory.
• No new dependencies introduced.
• Existing 16 tests still pass per the PR description.

Summary: The fix is correct and the test coverage is solid. The only actionable items are the wording nit in the nearLongMaxValueThrows comment and the test-name mismatch in the PR body - both minor. Good work tracking down the unsigned/signed comparison trap.

Code Review

Overall Assessment

This is a correct, minimal, and well-reasoned bug fix. The root cause analysis is accurate and the solution is the right tool for the job. A few points worth discussing.

The Fix (Simple8bCodec.java) - Approved

The one-line change is correct. The arithmetic proof in the PR description is solid:

• zigzagEncode(Long.MAX_VALUE) = 0xFFFFFFFFFFFFFFFE (negative in signed, huge in unsigned)
• zigzagEncode(Long.MIN_VALUE) = 0xFFFFFFFFFFFFFFFF (negative in signed, huge in unsigned)

Both bypass the old signed guard, and Long.compareUnsigned correctly catches them.

Tests - Good coverage with a minor style note

The three new regression tests (longMaxValueThrows, longMinValueThrows, nearLongMaxValueThrows) are well-targeted and verify the exact failure modes described in the issue.

Minor: The multi-line Javadoc comments in the test methods repeat information already captured in the method names and the PR description. One short line per method would be sufficient per project conventions. Not blocking.

Suggestion: Consider adding Long.MIN_VALUE + 1 as a fourth test case. Its zigzag encoding (0xFFFFFFFFFFFFFFFD) also falls in the unsigned-overflow zone and was equally silently corrupted before this fix. It would complete the boundary sweep.

Documentation file (docs/4336-simple8b-zigzag-validation.md) - Recommend dropping

A per-issue markdown file in docs/ tends to:

1. Accumulate over time without a clear ownership or cleanup path.
2. Go stale faster than the code it describes.
3. Duplicate information that already lives in the PR description, commit message, and test Javadoc.

The most durable place for this explanation is as a comment on the comparison line in encode() itself:

// Unsigned comparison required: zigzagEncode(Long.MAX/MIN_VALUE) returns negative signed values
// that look smaller than MAX_ZIGZAG_VALUE in signed arithmetic but exceed it unsigned.
if (Long.compareUnsigned(encoded, MAX_ZIGZAG_VALUE) > 0)

That makes the non-obvious invariant visible to any future reader right at the call site, which is more useful than a detached markdown file.

Potential follow-up (not blocking this PR)

Any data already written to disk before this fix with Long.MAX_VALUE or Long.MIN_VALUE (silently corrupted to a masked value) will decode to a wrong result silently. This PR correctly changes the write path to fail-fast going forward, but there is no migration or detection path for pre-existing corrupt data. Worth a note in the issue or release notes so operators are aware.

Summary

The core fix is correct and the tests are solid. I would recommend dropping the documentation file (or folding its key insight into a single inline comment on the comparison), and optionally adding Long.MIN_VALUE + 1 to the test suite. Everything else is good to go.

Code Review

Overview

This PR fixes a real data-corruption bug in Simple8bCodec.encode(). The root cause is a signed vs unsigned comparison issue: zigzagEncode(Long.MAX_VALUE) returns -2 and zigzagEncode(Long.MIN_VALUE) returns -1, both of which are negative in signed arithmetic, so the old guard encoded > MAX_ZIGZAG_VALUE (where MAX_ZIGZAG_VALUE is a large positive) was always false for these inputs. The fix replacing it with Long.compareUnsigned(encoded, MAX_ZIGZAG_VALUE) > 0 is mathematically correct and is the minimal change needed.

The Fix (Simple8bCodec.java)

The one-line change is correct. Manual verification:

• zigzagEncode(Long.MAX_VALUE) = 0xFFFFFFFFFFFFFFFE - unsigned value far exceeds MAX_ZIGZAG_VALUE = 0x0FFFFFFFFFFFFFFF - correctly rejected
• zigzagEncode(Long.MIN_VALUE) = 0xFFFFFFFFFFFFFFFF - same, correctly rejected
• Boundary -(2^59) still zigzag-encodes to exactly MAX_ZIGZAG_VALUE - compareUnsigned returns 0, still accepted

No issues with the production code change.

Test Coverage (Simple8bCodecTest.java)

The three new tests are sound and consistent with the existing test style (assertThatThrownBy + isInstanceOf + hasMessageContaining). Good.

One minor style note from the project's CLAUDE.md:

"Never write multi-paragraph docstrings or multi-line comment blocks - one short line max."

The three new test methods each carry a 3-4 line Javadoc block. These should be trimmed to a single-line comment, e.g.:

// Regression: zigzagEncode(Long.MAX_VALUE) = -2 (negative signed) bypassed the old > guard.
@Test
void longMaxValueThrows() { ... }

Same for longMinValueThrows and nearLongMaxValueThrows.

Documentation File (docs/4336-simple8b-zigzag-validation.md) - Needs Removal

This file should not be committed to the repository. It contains:

• Internal review-cycle notes ("Review Cycles" / "Cycle 1", "Cycle 2")
• A self-referencing PR link
• Bot review feedback log

This is working-document content that belongs in the PR description (where it already lives verbatim) and not in the tracked codebase. Please drop this file before merging.

Summary

The core fix is solid. Please remove the docs file and trim the multi-line Javadoc blocks to single lines, then this is ready to merge.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.36%. Comparing base (f2a41f8) to head (8c31704).

@@             Coverage Diff              @@
##               main    #4361      +/-   ##
============================================
- Coverage     64.65%   64.36%   -0.30%     
- Complexity      423      455      +32     
============================================
  Files          1647     1647              
  Lines        127753   127753              
  Branches      27382    27382              
============================================
- Hits          82599    82224     -375     
- Misses        33507    33958     +451     
+ Partials      11647    11571      -76     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.