fix(common): return error instead of panicking on truncated record bytes

[shikhar]

Summary

• Replace get_uint with try_get_uint when parsing metered size in TryFrom<Bytes> for Metered<Record>, returning InternalRecordError::Truncated("MeteredSize") instead of panicking.
• Consistent with the safe parsing pattern already used in envelope.rs and decode_if_command_record.

Closes #355

Test plan

• Verify cargo check passes
• Confirm truncated input (valid magic byte but missing metered size bytes) returns Err instead of panicking

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

This PR fixes a potential panic in TryFrom<Bytes> for Metered<Record> by replacing the infallible buf.get_uint(...) call (which panics on insufficient bytes) with buf.try_get_uint(...).map_err(|_| InternalRecordError::Truncated("MeteredSize")), bringing this code path in line with the safe parsing patterns already used throughout envelope.rs and decode_if_command_record.

• Root cause: a valid magic byte followed by fewer bytes than metered_size_varlen dictated would previously cause a runtime panic; it now returns a typed Err.
• Pattern consistency: the change mirrors try_get_uint calls in EnvelopeRecord::try_from (e.g. NumHeaders, HeaderNameLen, HeaderValueLen) and the length check in decode_if_command_record.
• Scope: single-line functional change with no API surface impact — InternalRecordError::Truncated was already an existing variant and callers already handle Err.
• Minor gap: no unit test is added that specifically drives Metered::<Record>::try_from with a valid magic byte but a truncated metered-size field, whereas envelope.rs has a dedicated truncated_returns_error test for analogous paths.

Confidence Score: 5/5

Safe to merge — the fix is minimal, correct, and consistent with existing patterns; the only remaining feedback is a P2 suggestion to add a regression test.

The change is a one-line targeted replacement of a known panic path with the safe try_get_uint API, matching patterns used everywhere else in the same module. No API changes, no logic regressions, and no P0/P1 issues were found. The only open item is a missing unit test for the newly safe code path, which is a P2 suggestion and does not block merge.

No files require special attention; the single changed file (common/src/record/mod.rs) is straightforward.

Important Files Changed

Filename	Overview
common/src/record/mod.rs	Replaces panicking buf.get_uint(...) with the safe buf.try_get_uint(...).map_err(...) in TryFrom<Bytes> for Metered<Record>, returning InternalRecordError::Truncated("MeteredSize") on short input; the rest of the file is unchanged

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["TryFrom<Bytes> for Metered<Record>"] --> B{buf is empty?}
    B -- yes --> C["Err(Truncated('MagicByte'))"]
    B -- no --> D["buf.get_u8() → MagicByte::try_from(...)"]
    D -- invalid --> E["Err(InvalidValue('MagicByte', msg))"]
    D -- ok --> F["buf.try_get_uint(metered_size_varlen)"]
    F -- "Err (insufficient bytes) — BEFORE: panic / AFTER: error" --> G["Err(Truncated('MeteredSize'))"]
    F -- ok --> H{record_type?}
    H -- Command --> I["CommandRecord::try_from(buf)"]
    H -- Envelope --> J["EnvelopeRecord::try_from(buf)"]
    I --> K["Ok(Metered<Record>)"]
    J --> K

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: common/src/record/mod.rs
Line: 316-318

Comment:
**Missing test for the fixed panic path**

The PR test plan calls out "Confirm truncated input (valid magic byte but missing metered size bytes) returns `Err` instead of panicking", but no corresponding unit test is added to the test module. `envelope.rs` has a dedicated `truncated_returns_error` test that verifies every truncation point returns `Err`; the same pattern would be valuable here to guard against regressions.

Consider adding something like:

```rust
#[test]
fn metered_record_truncated_after_magic_byte_returns_error() {
    // Magic byte: Envelope (0b0000_0010), metered_size_varlen = 1 → expects 1 more byte.
    let truncated = Bytes::from_static(&[0b0000_0010]);
    assert_eq!(
        Metered::<Record>::try_from(truncated),
        Err(InternalRecordError::Truncated("MeteredSize"))
    );
}
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix(common): return error instead of pan..." | Re-trigger Greptile}