fix(encoding): add fallible try_encode() API, remove panics and silent truncation

[digizeph]

Summary

The encoding layer had three classes of bugs that let arbitrary input crash the process or silently corrupt wire output. This PR adds a proper EncodingError type and try_encode() methods to fix all three.

Closes #313.

Problems fixed

1. Hard panics: .expect() in encode_bgp_open_param_value (introduced in Fix BGP open encoder and remove redundant param_length #312) crashed on oversized capability values — a round-trip parse→encode of any OPEN with a >255-byte raw capability would panic.

2. Silent truncation: Unchecked as u8 / as u16 casts throughout encoding code silently discarded high bits, producing wrong length/count fields on the wire. Examples: AS_PATH segment with >255 ASes, attribute value >65535 bytes, BGP message total >65519 bytes.

3. Error swallowing: MP_REACH/MP_UNREACH NLRI encoding failures were silently replaced with empty bytes.

Changes

New API (additive, backwards-compatible):

• EncodingError enum in error.rs — exported as bgpkit_parser::error::EncodingError and bgpkit_parser::EncodingError
• try_encode() methods on BgpOpenMessage, BgpUpdateMessage, BgpMessage, Attribute, Attributes — return Result<Bytes, EncodingError>
• encode_as_path() now returns Result<Bytes, EncodingError>
• Existing encode() methods retained as panic-on-error wrappers

Internal fixes:

• All .expect() / debug_assert! in encoding replaced with checked u8::try_from / u16::try_from + ? propagation
• All as u8 / as u16 truncation casts in secondary attribute encoders (tunnel_encap, linkstate, bfd, prefix_sid, bier, sfp, flowspec) replaced with .min() clamps as a defensive boundary
• MRT encoder casts (peer_index_table, rib_afi_entries, table_dump, geo_peer_table) also clamped

Regression tests (5 new):

• test_fallible_encoding_as_path_overflow — >255 ASes in a segment
• test_fallible_encoding_open_raw_capability_oversize — >255-byte raw capability
• test_fallible_encoding_open_extended_param_oversize — extended params >65535 bytes
• test_fallible_encoding_update_attributes_oversize — total attributes >65535 bytes
• test_bgp_open_encoding_rejects_oversized_add_path_capability — updated from #[should_panic] to verify both try_encode() returns Err and encode() panics

Test Plan

• cargo fmt --check ✅
• cargo clippy --all-targets --all-features -- -D warnings ✅
• cargo test --all-features — 673 unit tests + 47 doc tests + 1 integration test, all passing ✅
• 5 new regression tests pass ✅

[codecov]

Codecov Report

❌ Patch coverage is 95.41189% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.48%. Comparing base (7d9268b) to head (25fbba6).

Files with missing lines	Patch %	Lines
src/parser/bgp/messages.rs	95.46%	18 Missing ⚠️
src/parser/bgp/attributes/attr_29_linkstate.rs	53.57%	13 Missing ⚠️
src/parser/mrt/messages/mod.rs	69.56%	7 Missing ⚠️
src/parser/mrt/messages/bgp4mp.rs	89.28%	3 Missing ⚠️
...arser/mrt/messages/table_dump_v2/geo_peer_table.rs	94.73%	1 Missing ⚠️
...ser/mrt/messages/table_dump_v2/peer_index_table.rs	98.46%	1 Missing ⚠️
...rser/mrt/messages/table_dump_v2/rib_afi_entries.rs	98.79%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #314      +/-   ##
==========================================
+ Coverage   90.31%   90.48%   +0.17%     
==========================================
  Files          91       92       +1     
  Lines       19306    19887     +581     
==========================================
+ Hits        17436    17995     +559     
- Misses       1870     1892      +22     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

This PR hardens the BGP/MRT encoding layer by introducing a fallible encoding API (try_encode) backed by a new EncodingError type, aiming to eliminate panics, silent integer truncation, and swallowed encoding failures when serializing potentially-untrusted parsed data back to wire format.

Changes:

• Added EncodingError and exposed it via bgpkit_parser::error::EncodingError and bgpkit_parser::EncodingError.
• Introduced try_encode() methods on core BGP message/attribute types and propagated checked length conversions (u8::try_from / u16::try_from) instead of panicking/truncating.
• Reworked encode_as_path() to return Result<Bytes, EncodingError> and added regression tests for oversized encodings.

Reviewed changes

Copilot reviewed 19 out of 19 changed files in this pull request and generated 12 comments.

Show a summary per file

File	Description
CHANGELOG.md	Documents the new fallible encoding API and the encoding hardening.
src/error.rs	Adds EncodingError enum + Display/Error impls.
src/parser/mod.rs	Re-exports EncodingError from the parser module API surface.
src/parser/bgp/messages.rs	Adds try_encode() for OPEN/UPDATE/BgpMessage and replaces panics/truncation with checked conversions + propagation.
src/parser/bgp/attributes/mod.rs	Adds try_encode() for Attribute/Attributes and propagates encoding errors (including AS_PATH).
src/parser/bgp/attributes/attr_02_17_as_path.rs	Makes AS_PATH encoding fallible and checks segment-count overflow.
src/parser/bgp/attributes/attr_23_tunnel_encap.rs	Changes length encoding to clamped writes (but currently still risks wire-length/value mismatches).
src/parser/bgp/attributes/attr_29_linkstate.rs	Changes TLV length encoding to clamped writes (but currently still risks wire-length/value mismatches).
src/parser/bgp/attributes/attr_37_sfp.rs	Clamps TLV length fields (but currently still risks wire-length/value mismatches).
src/parser/bgp/attributes/attr_38_bfd_discriminator.rs	Clamps TLV length fields (but currently still risks wire-length/value mismatches).
src/parser/bgp/attributes/attr_40_bgp_prefix_sid.rs	Clamps TLV length fields (but currently still risks wire-length/value mismatches).
src/parser/bgp/attributes/attr_41_bier.rs	Clamps TLV length fields (but currently still risks wire-length/value mismatches).
src/parser/mrt/messages/table_dump.rs	Clamps attribute-length field (but currently still risks length/payload mismatches).
src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs	Clamps attribute-length field (but currently still risks length/payload mismatches).
src/parser/mrt/messages/table_dump_v2/peer_index_table.rs	Clamps peer IDs/counts/lengths (but currently can corrupt the table and/or emit inconsistent wire format).
src/parser/mrt/messages/table_dump_v2/geo_peer_table.rs	Clamps view-name length and peer count (but currently can emit inconsistent wire format).
src/models/bgp/tunnel_encap.rs	Clamps Sub-TLV length() helper.
src/models/bgp/linkstate.rs	Clamps Link-State TLV length() helper.
src/models/bgp/flowspec/nlri.rs	Clamps FlowSpec NLRI length (but must respect RFC’s 12-bit length encoding semantics).

Comments suppressed due to low confidence (2)

src/parser/bgp/attributes/attr_23_tunnel_encap.rs:113

• The tunnel-length field is clamped, but the full sub_tlv_bytes block is still appended. If the sub-TLV block exceeds u16::MAX, the declared tunnel length won’t match the bytes written, making the attribute invalid.

If clamping is intended, also truncate the emitted sub-TLV bytes to the clamped tunnel length.

        // Encode tunnel length
        bytes.put_u16(sub_tlv_bytes.len().min(u16::MAX as usize) as u16);

        // Append sub-TLV data
        bytes.extend_from_slice(&sub_tlv_bytes);

src/parser/bgp/attributes/attr_29_linkstate.rs:460

• The unknown-attributes TLV encoder writes tlv.length() (now clamped) but still appends the full tlv.value. If tlv.value.len() > u16::MAX, the encoded TLV becomes invalid because the declared length won’t match the bytes written.

To keep the wire format self-consistent under the new clamping approach, truncate the emitted value bytes to the encoded length (or make the encoder fallible and return an error).

    // Encode prefix attributes
    for (attr_type, value) in &attr.prefix_attributes {
        let type_code = u16::from(*attr_type);
        bytes.put_u16(type_code);
        bytes.put_u16(value.len().min(u16::MAX as usize) as u16);
        bytes.extend_from_slice(value);
    }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[ties]

AI review:

[0] src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs:179 (CONFIRMED)

RibAfiEntries::encode() silently drops any RIB entry whose encoding fails: bytes.extend(entry.encode_for_rib_type(is_add_path)) passes a Result<Bytes, EncodingError> to BytesMut::extend, which consumes it via Result: IntoIterator and appends nothing on Err instead of propagating or panicking.
Failure scenario: Empirically confirmed: a RibAfiEntries containing one entry whose attributes exceed u16::MAX encodes to a 10-byte buffer that declares entry count = 1 but contains zero entry bytes. Any consumer parsing the emitted MRT TABLE_DUMP_V2 record hits a truncated/corrupt record (entry count mismatch) with no error ever surfaced to the writer — exactly the silent-corruption class this PR (issue #313) set out to eliminate, and worse than the wrapper methods which at least panic.
Verifier evidence: src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs:179 reads bytes.extend(entry.encode_for_rib_type(is_add_path)); while line 196 now declares fn encode_for_rib_type(&self, include_path_id: bool) -> Result<Bytes, EncodingError>. With bytes v1.9.0 (impl Extend for BytesMut) and Result's IntoIterator (0 items on Err), this compiles and appends nothing on failure. Reproduced at runtime: an entry with 20,000 communities returns Err(ValueTooLarge { field: "BGP attribute value length (non-extended)", actual: 80000, max: 255 }) from try_encode(), yet RibAfiEntries::encode() returns 10 bytes [00 00 00 01 | 18 0a 00 00 | 00 01] — entry_count field = 1 with zero entry bytes following — with no panic or error.

[1] src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs:179 (CONFIRMED)

RibAfiEntries::encode silently drops any RIB entry whose encoding fails: bytes.extend(entry.encode_for_rib_type(is_add_path)) feeds a Result<Bytes, EncodingError> into Extend<Bytes> (Result-as-iterator yields nothing on Err), so the error path added by this branch is swallowed instead of propagated; the old code (encode_for_rib_type returned plain Bytes) always wrote the entry.
Failure scenario: A RibAfiEntries containing an entry with attributes whose encoded length exceeds u16::MAX (the exact case the new EncodingError was added for): encode() writes entry_count including that entry, then omits its bytes entirely — producing a corrupt MRT TABLE_DUMP_V2 record that declares N entries but contains N-1. No error, no panic, no log; downstream parsers of the written MRT file fail or read garbage. Verified the branch compiles with this Result-into-extend, so the silent drop is real.
Verifier evidence: Same mechanism confirmed at src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs:179: bytes.extend(entry.encode_for_rib_type(is_add_path)); feeds Result<Bytes, EncodingError> into BytesMut's Extend (bytes v1.9.0), silently yielding nothing on Err. The candidate's claim about the old code is accurate — the diff shows the prior signature was fn encode_for_rib_type(&self, include_path_id: bool) -> Bytes, which always wrote the entry. Runtime test: a RibAfiEntries with one entry whose encoded attributes exceed the length limit produces a 10-byte record declaring entry_count = 1 but containing N-1 = 0 entries, with no error, panic, or log.

[2] src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs:179 (CONFIRMED)

RibAfiEntries::encode silently drops any RIB entry whose encoding fails: encode_for_rib_type now returns Result<Bytes, EncodingError>, and bytes.extend(entry.encode_for_rib_type(is_add_path)) still compiles because Result implements IntoIterator (0 or 1 items) and BytesMut implements Extend — an Err yields zero items, so the entry and the error both vanish.
Failure scenario: Verified at runtime on this branch: a RibAfiEntries with one entry whose attributes exceed u16::MAX encodes to a 10-byte record whose entry-count field says 1 but which contains zero entry bytes — no panic, no error. Via the production path MrtRibEncoder::export_bytes -> MrtMessage::encode -> RibAfiEntries::encode, a user exporting a RIB dump containing one oversized entry writes a corrupt MRT file; any downstream parser (including this library's parse_mrt_record) then reads the next record's header bytes as RIB-entry fields and fails or returns garbage routes. This is strictly worse than the pre-PR behavior (truncated length field) and defeats the PR's stated goal of never silently corrupting output.
Verifier evidence: Confirmed including the production path: src/encoder/rib_encoder.rs:119 builds MrtMessage::TableDumpV2Message(TableDumpV2Message::RibAfi(prefix_rib_entry)) inside export_bytes(), and src/parser/mrt/messages/mod.rs:14 dispatches TableDumpV2Message::RibAfi(r) => r.encode() — the buggy RibAfiEntries::encode at rib_afi_entries.rs:179 (bytes.extend(entry.encode_for_rib_type(is_add_path));). Verified at runtime on this branch: an oversize entry (try_encode() = Err(ValueTooLarge { actual: 80000, max: 255 })) encodes via RibAfiEntries::encode() to a 10-byte record [00 00 00 01 | 18 0a 00 00 | 00 01] whose entry-count field says 1 but which contains zero entry bytes — no panic, no error, so a written MRT file is corrupt and downstream parsers misalign on the next record.

[3] src/parser/bgp/attributes/mod.rs:547 (CONFIRMED)

Attribute::try_encode() swallows MP_REACH_NLRI/MP_UNREACH_NLRI encoding failures via unwrap_or_else(|e| { log::warn!(...); Bytes::new() }), returning Ok with a zero-length attribute value instead of an EncodingError, contradicting the new fallible API contract.
Failure scenario: A BgpUpdateMessage whose MpReachNlri fails encode_nlri (e.g. unsupported NLRI combination) passes try_encode() successfully but emits an MP_REACH_NLRI attribute with length 0 on the wire — an RFC-invalid attribute that BGP peers reject with a NOTIFICATION or that fails round-trip parsing; the caller who deliberately used try_encode() to catch encoding problems gets Ok plus corrupt output, with only a log line as evidence.
Verifier evidence: src/parser/bgp/attributes/mod.rs:547-550 inside the new try_encode(): encode_nlri(v, true, add_path).unwrap_or_else(|e| { log::warn!("Failed to encode MP_REACH_NLRI: {}", e); Bytes::new() }) — the error is swallowed and try_encode proceeds to write a length-0 attribute value (lines 576-596) and returns Ok(bytes.freeze()). The error path is reachable: encode_nlri (attr_14_15_nlri.rs:155) propagates encode_labeled_prefix errors (src/models/network/mpls.rs: if prefix.labels.is_empty() { return Err(LabeledNlriEncodeError::EmptyLabelStack); } and if prefix.path_id.is_some() && !add_path { return Err(LabeledNlriEncodeError::AddPathNotNegotiated); }). E.g. an MpReachNlri with SAFI MplsLabel and a labeled prefix whose label stack is empty yields Ok plus a zero-length MP_REACH_NLRI attribute on the wire, with only a log::warn as evidence — exactly as claimed. (Minor: the trigger is labeled-prefix encode failures, not generic 'unsupported NLRI combination', but the mechanism and wrong output are as stated.)

[4] src/parser/bgp/attributes/mod.rs:547 (CONFIRMED)

The new fallible Attribute::try_encode still swallows encode_nlri errors in the MpReachNlri/MpUnreachNlri arms (unwrap_or_else -> log::warn + Bytes::new()), so it returns Ok with a zero-length attribute value instead of Err, violating the contract the PR establishes for every other arm (LinkState, TunnelEncap, BFD, PrefixSid, Bier, Sfp all use ?).
Failure scenario: A caller who adopts the new error-safe API — e.g. BgpUpdateMessage::try_encode on an update whose MP_REACH_NLRI fails to encode (encode_nlri returns Err for unsupported AFI/SAFI combinations) — receives Ok and transmits/stores a BGP UPDATE containing an MP_REACH_NLRI attribute with length 0, which is malformed per RFC 4760; a receiving BGP speaker sends a NOTIFICATION and tears down the session, and re-parsing the stored bytes fails, even though try_encode reported success.
Verifier evidence: Same defect, accurately described. src/parser/bgp/attributes/mod.rs:552-558: AttributeValue::MpUnreachNlri(v) => { encode_nlri(v, false, false).unwrap_or_else(|e| { log::warn!("Failed to encode MP_UNREACH_NLRI: {}", e); Bytes::new() }) } — add_path is hardcoded false, so an MPLS-labeled withdrawal prefix with a path_id hits LabeledNlriEncodeError::AddPathNotNegotiated in encode_labeled_prefix (mpls.rs: if prefix.path_id.is_some() && !add_path), which encode_nlri propagates as Err and this arm swallows. All other fallible arms use ? (lines 559-564: encode_link_state_attribute(v)?, encode_tunnel_encapsulation_attribute(v)?, encode_bfd_discriminator(v)?, encode_bgp_prefix_sid(v)?, encode_bier(v)?, encode_sfp(v)?), confirming the contract inconsistency. BgpUpdateMessage::try_encode calls Attributes::try_encode (line 611: bytes.extend(attr.try_encode(asn_len)?)) so the caller receives Ok with a zero-length MP_UNREACH_NLRI attribute, malformed per RFC 4760, as claimed.

[5] src/parser/mrt/messages/table_dump_v2/peer_index_table.rs:74 (CONFIRMED)

PeerIndexTable::add_peer clamps the new peer id with .min(u16::MAX as usize), so once 65536 distinct peers exist every additional peer silently receives id 65535, overwriting the existing id_peer_map entry and aliasing multiple peers to one index.
Failure scenario: A writer building a PEER_INDEX_TABLE with more than 65536 peers gets id 65535 returned for every overflow peer; RIB entries recorded earlier against the original peer 65535 now resolve (get_peer_by_id / the encoded table) to a different peer — silent mis-association of routes to the wrong peer. The clamp merely swaps the old wrapping as u16 truncation for saturating truncation instead of returning an error, leaving the silent-corruption bug the PR claims to fix; try_encode only errors later once id_peer_map.len() exceeds 65535, after the in-memory map is already corrupted.
Verifier evidence: peer_index_table.rs:74 reads let peer_id = self.peer_ip_id_map.len().min(u16::MAX as usize) as u16; and line 76 self.id_peer_map.insert(peer_id, peer); overwrites. With 65536 distinct peers already assigned ids 0..=65535, every further peer gets id 65535, replacing the existing entry — routes recorded against the original peer 65535 then resolve via get_peer_by_id (line 91-93) to the wrong peer. The diff shows this replaced the old wrapping let peer_id = self.peer_ip_id_map.len() as u16;, i.e. saturating instead of wrapping truncation, still silent. try_encode only errors afterwards via the peer-count check at lines 163-168 (u16::try_from(self.id_peer_map.len())), by which time the map is already corrupted. All claims match the code.

[6] src/parser/mrt/messages/table_dump_v2/peer_index_table.rs:74 (CONFIRMED)

PeerIndexTable::add_peer replaces the old wrapping cast with a silent saturating clamp .min(u16::MAX as usize) as u16 instead of an error, so peer IDs silently collide at 65535 — the branch's goal of surfacing overflows as EncodingError is not re-established on this path (no fallible add_peer).
Failure scenario: MrtRibEncoder::process_elem (src/encoder/rib_encoder.rs:60) with more than 65536 unique peers: every peer past the 65536th gets ID 65535, silently overwriting the previous peer in id_peer_map and mis-attributing all its RIB entries to the wrong peer; entries_map.insert(peer_index, ...) then silently discards colliding entries. The corruption happens silently at add time; only later does export_bytes hit the peer-count check (as a panic via the infallible wrapper).
Verifier evidence: add_peer returns bare u16 (peer_index_table.rs:70, pub fn add_peer(&mut self, peer: Peer) -> u16) — no fallible variant exists, so the branch's EncodingError surfacing is absent on this path. The cited caller is real: src/encoder/rib_encoder.rs process_elem does let peer_index = self.index_table.add_peer(peer); (~line 61) and later entries_map.insert(peer_index, entry);, so two overflow peers colliding at index 65535 on the same prefix silently overwrite each other's RibEntry. export_bytes calls mrt_message.encode(subtype) which routes to p.encode() (src/parser/mrt/messages/mod.rs:13), the infallible wrapper that panics via .expect("PeerIndexTable encoding failed...") (peer_index_table.rs:203-205) once id_peer_map.len() > 65535.

[7] src/parser/mrt/messages/table_dump_v2/peer_index_table.rs:74 (CONFIRMED)

PeerIndexTable::add_peer's new .min(u16::MAX as usize) as u16 clamp silently assigns the duplicate peer id 65535 to every peer past the 65536th, overwriting id_peer_map entry 65535 and returning a peer_index that points at a different peer than the one just added.
Failure scenario: A user feeding more than 65536 unique peers through MrtRibEncoder::process_elem gets RIB entries whose peer_index (65535) refers to whichever peer was added last, misattributing routes across peers in the in-memory table; at export_bytes the id_peer_map has 65536 entries so PeerIndexTable::encode's new peer-count check trips and the infallible encode() wrapper panics via expect, aborting the process instead of returning an error — the silent clamp in add_peer gives the caller no earlier signal.
Verifier evidence: Line 74's clamp assigns id 65535 to every peer past the 65536th; line 76's id_peer_map.insert(peer_id, peer) overwrites, so peer_index 65535 in already-recorded RIB entries ends up referring to whichever overflow peer was added last — misattribution as claimed. At export: with 65536+ entries in id_peer_map, try_encode's check u16::try_from(self.id_peer_map.len()).map_err(|_| EncodingError::ValueTooLarge { field: "PeerIndexTable peer count", ... }) (lines 163-168) fails, and export_bytes reaches it through MrtMessage::encode → p.encode() (src/parser/mrt/messages/mod.rs:13) → self.try_encode().expect(...) (lines 203-206), aborting via panic rather than returning an error. One minor imprecision — at the moment of insertion the returned index points at the just-added peer, and only becomes stale when a later overflow peer overwrites it — but the described corruption and panic path are accurate.

[8] src/parser/bgp/messages.rs:471 (CONFIRMED)

BgpOpenMessage::try_encode writes param.param_type unconditionally, so a first optional parameter with param_type 255 emitted in non-extended framing is indistinguishable from the RFC 9072 extended-length marker and is mis-parsed on round trip.
Failure scenario: Construct BgpOpenMessage { extended_length: false, opt_params: vec![OptParam { param_type: 255, param_value: ParamValue::Raw(vec![0xAA, 0xBB]) }] }; encode() emits [len, 255, 2, 0xAA, 0xBB]. parse_bgp_open_message's detection triggers on first param_type == 255 alone (messages.rs:244), so it re-reads the value bytes as a 16-bit extended length (0x02AA = 682), sets extended_length = true, and either errors on truncation or returns a structurally different message — round-trip corruption the encoder neither rejects nor escapes.
Verifier evidence: src/parser/bgp/messages.rs:470-471: for (param_type, value) in encoded_params { buf.put_u8(param_type); — param_type is written unconditionally, and use_extended_length (lines 432-433: self.extended_length || non_extended_params_len > u8::MAX as usize) never considers param_type, so try_encode has no rejection or escaping of type 255 in non-extended framing. OptParam is pub param_type: u8 (src/models/bgp/mod.rs:99), so OptParam { param_type: 255, param_value: ParamValue::Raw(vec![0xAA, 0xBB]) } with extended_length=false is constructible; it encodes to params section [255, 2, 0xAA, 0xBB] with opt_params_len=4. On re-parse, line 244 if opt_params_len != 0 && param_type == 255 { triggers the RFC 9072 branch: opt_params_len = input.read_u16()? (line 267) consumes [2, 0xAA] as extended length 0x02AA=682, only a warn fires on the mismatch (lines 272-278), param_type = input.read_u8()? (line 280) reads 0xBB, and input.read_u16()? for param_len (line 287) then fails on the empty buffer — round-trip corruption/error that the encoder neither rejects (no EncodingError) nor escapes, exactly as claimed.

[9] src/parser/bgp/attributes/mod.rs:570 (CONFIRMED)

Attribute::try_encode returns Ok with an empty value for AttributeValue::AttrSet (encoding "not yet implemented"), silently emitting a zero-length ATTR_SET attribute instead of an EncodingError.
Failure scenario: Re-encoding a parsed UPDATE that carries an ATTR_SET (RFC 6368) attribute — e.g. rewriting an MRT file — succeeds via try_encode but writes flag/type/len=0 with the entire nested attribute set discarded; downstream parsers see an invalid empty ATTR_SET and the original data is lost with no error or warning, despite the new fallible API existing precisely to surface unencodable values.
Verifier evidence: src/parser/bgp/attributes/mod.rs:570-573 reads AttributeValue::AttrSet(_v) => { // ATTR_SET encoding not yet implemented - return empty bytes Bytes::new() }, and try_encode then returns Ok(bytes.freeze()) (line 597), so an Attribute holding a publicly-constructible AttributeValue::AttrSet (pub struct AttrSet with pub fields, src/models/bgp/attributes/mod.rs:627-632) deterministically encodes as Ok([flag, 128, len=0]) with the nested attribute set silently discarded — no EncodingError despite the new fallible API. One correction to the stated scenario: the parse-then-re-encode path is NOT affected, because the parser raw-retains ATTR_SET (is_raw_retained_attr includes AttrType::ATTR_SET at line 147; line 396-405 stores it as AttributeValue::Raw, which round-trips via v.bytes.clone() at line 566). The silent empty encode only triggers for user-constructed AttrSet values, but that trigger is concrete and the wrong output is deterministic.

[10] src/parser/bgp/messages.rs:720 (CONFIRMED)

The retained infallible encode() wrappers now panic via .expect() where the old code never panicked (it truncated), and higher-level unchanged APIs with no try_ alternative — Bgp4MpMessage::encode (bgp4mp.rs:182 calls self.bgp_message.encode), MrtMessage::encode, MrtRibEncoder::export_bytes — inherit the panic with no fallible escape hatch.
Failure scenario: A user encoding a Bgp4MpMessage or exporting via MrtRibEncoder::export_bytes whose data contains e.g. an AS_PATH segment with >255 ASes or oversized attributes: previous releases returned (corrupt but non-fatal) bytes; this branch aborts the whole process with a panic, and the caller cannot opt into Result handling because Bgp4MpMessage/MrtMessage/export_bytes were not given try_encode variants.
Verifier evidence: Behavior change is real and the panic is inherited with no fallible escape. (1) Old code truncated, never panicked, on these paths: main attr_02_17_as_path.rs:63 output.put_u8(asns.len() as u8); main attributes/mod.rs:576 bytes.put_u8(value_bytes.len() as u8); main rib_afi_entries.rs:200 bytes.put_u16(attr_bytes.len() as u16) — grep of main shows no non-test .expect()/panic! on any of these encode paths. (2) The branch replaces the casts with checked conversions returning EncodingError (attr_02_17_as_path.rs:67 u8::try_from(asns.len()).map_err(|_| EncodingError::ValueTooLarge {...})), and the retained wrappers panic: src/parser/bgp/messages.rs:720-722 pub fn encode(&self, asn_len: AsnLength) -> Bytes { self.try_encode(asn_len).expect("BGP message encoding failed; use try_encode() for fallible handling") } (same pattern at 495-497, 641-643, and rib_afi_entries.rs:191-193). (3) No fallible alternative at the named higher levels: src/parser/mrt/messages/bgp4mp.rs:174 pub fn encode(&self, asn_len: AsnLength) -> Bytes with line 182 bytes.extend(&self.bgp_message.encode(asn_len));; src/parser/mrt/messages/mod.rs:9 pub fn encode(&self, sub_type: u16) -> Bytes dispatches only to the panicking infallible wrappers (m.encode(), r.encode(), msg.encode(asn_len)); src/encoder/rib_encoder.rs:82 pub fn export_bytes(&mut self) -> Bytes calls mrt_message.encode(subtype) at lines 91 and 123. git grep try_encode in those files returns nothing. Trigger: a user-constructed or AS_PATH/AS4_PATH-merged update with a segment >255 ASes (or attributes totaling >65535 bytes) encoded via Bgp4MpMessage::encode/MrtMessage::encode/export_bytes previously produced corrupt-but-returned bytes; on this branch the .expect() aborts the process and the caller cannot opt into Result handling.

[11] src/parser/bgp/messages.rs:432 (CONFIRMED)

BgpOpenMessage::try_encode silently switches to RFC 9072 extended framing when extended_length is false but non-extended params exceed 255 bytes, instead of returning an EncodingError — a silent wire-format change replacing the old silent truncation rather than the fallible behavior the branch promises.
Failure scenario: A user builds an OPEN with extended_length=false (e.g. because the peer does not support RFC 9072) whose optional parameters total >255 bytes: try_encode returns Ok with an extended-format OPEN (opt-params-len byte 0xFF + type 0xFF + u16 length) that a non-RFC-9072 BGP speaker rejects or misparses as a malformed capability list, and a parse round-trip flips the model's extended_length field from false to true. The user gets no error signaling the format substitution.
Verifier evidence: src/parser/bgp/messages.rs:432-433: let use_extended_length = self.extended_length || non_extended_params_len > u8::MAX as usize; — when extended_length is false but params exceed 255 bytes, the encoder silently switches to RFC 9072 framing (lines 457-463 emit buf.put_u8(u8::MAX); buf.put_u16(agg_len);) and returns Ok; no EncodingError path exists for this case. The branch's own test test_bgp_open_automatically_uses_extended_parameter_encoding (lines 1240-1261) constructs exactly this input (extended_length: false, one 256-byte raw param) and asserts the round-trip flips the model flag: let parsed = parse_bgp_open_message(...).unwrap(); assert!(parsed.extended_length);. So the trigger, silent wire-format substitution, and false→true round-trip flip are all real and demonstrated. Caveat: the behavior is deliberate — a code comment says "we must switch to RFC 9072 extended framing" and a dedicated test locks it in — so this is an intentional design choice with a real interop consequence (a non-RFC-9072 peer sees opt-parm-len 0xFF and unknown param type 0xFF and will reject the OPEN), not an accidental bug.

[12] src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs:176 (CONFIRMED)

RibAfiEntries::encode still writes the entry count with an unchecked entry_count as u16 cast, one of the exact silent-truncation patterns this PR set out to eliminate, three lines above the newly fallible per-entry encoding.
Failure scenario: An add-path RIB with more than 65535 entries for one prefix (e.g. built programmatically or via MrtRibEncoder from many peer/path-id combinations) encodes a wrapped entry-count (65536 -> 0) followed by all 65536+ entry bodies; a parser reading the record back stops after the stated count and interprets the remaining entry bytes as the next MRT record header, producing parse failures or silently wrong data, with no error surfaced at encode time.
Verifier evidence: src/parser/mrt/messages/table_dump_v2/rib_afi_entries.rs:175-176: let entry_count = self.rib_entries.len(); bytes.put_u16(entry_count as u16); — an unchecked usize->u16 cast in RibAfiEntries::encode, which the diff did not touch (only RibEntry gained try_encode). There is no guard on rib_entries.len() anywhere, and RibAfiEntries has no fallible encode. With 65536+ entries (constructible: RibAfiEntries { rib_entries: vec![entry; 65536], .. }) the count wraps to 0 while the loop at lines 178-180 still writes all entry bodies; the parser at line 81 (for _i in 0..entry_count) then reads the wrapped count and treats the remaining entry bytes as subsequent record data — silent truncation with no encode-time error, exactly the pattern this PR eliminates elsewhere (cf. the checked u16::try_from(attr_bytes.len()) added at lines 206-211 in the same file).

[13] src/parser/mrt/messages/table_dump_v2/peer_index_table.rs:74 (CONFIRMED, cleanup)

add_peer still uses a silent .min(u16::MAX) clamp for the new peer id, so the 65537th distinct peer silently reuses id 65535 and overwrites the existing peer in id_peer_map
Failure scenario: Building a PeerIndexTable with more than 65536 distinct peer IPs makes every additional peer claim id 65535, replacing the prior peer in id_peer_map; RIB entries recorded with the old id now resolve to the wrong peer (silent data corruption) before try_encode later errors on peer count. The branch's stated goal (commit 203fed6) was to replace .min clamps with checked conversions, but this one remains.
Verifier evidence: Line 74: self.peer_ip_id_map.len().min(u16::MAX as usize) as u16. Peers 1..=65536 receive ids 0..=65535; the 65537th distinct peer sees len()==65536, clamps to 65535, and line 76 overwrites the prior peer in id_peer_map — silent corruption before try_encode's later peer-count error (lines 163-168). The stated-goal claim also checks out: commit 203fed6's message is "fix(encoding): replace .min() clamps with checked conversions for all Copilot comments ... The .min() clamp approach ... producing wire-inconsistent output", yet this .min clamp remains in add_peer (it was actually introduced/kept on this branch; main had a wrapping as u16 cast).

[14] src/models/bgp/flowspec/nlri.rs:97 (CONFIRMED, cleanup)

FlowSpec NLRI length check uses u16::MAX (65535) as the bound, but the wire length field is 12-bit (max 4095): encode_length does 0xF0 | (length >> 8) and parse_length masks with 0x0F, so lengths 4096..=65535 pass the new check yet still encode a corrupt length byte
Failure scenario: An NLRI whose component data totals e.g. 5000 bytes passes try_encode's ValueTooLarge check, but encode_length writes 0xF3 0x88 which decodes as length 904 — silently corrupt wire output that misframes the NLRI, the exact silent-truncation bug class issue #313 claims to fix. The bound should be 0x0FFF, matching parse_length's 12-bit mask.
Verifier evidence: src/models/bgp/flowspec/nlri.rs:94-98 guards only against > u16::MAX: let nlri_len = u16::try_from(data.len()).map_err(|_| EncodingError::ValueTooLarge { field: "FlowSpec NLRI total length", actual: data.len(), max: u16::MAX as usize })?;. But encode_length (lines 130-131) does data.push(0xF0 | ((length >> 8) as u8)); data.push(length as u8); and parse_length (line 121) masks ((first_byte & 0x0F) as u16) << 8 | second_byte as u16 — a 12-bit field per RFC 8955. Concrete trigger: NLRI component data totaling 5000 bytes (0x1388) passes the u16 check, encode_length emits 0xF3 0x88 (high nibble 0x1 ORed into the 0xF0 marker), and parse_length decodes it as 904 — silently corrupt, misframed wire output. Any length in 4096..=65535 hits this. The new oversize test (line 520, 40000 operators = 80001 bytes) only exercises > u16::MAX, so the 4096..=65535 gap is untested. Bound should be 0x0FFF.

[15] src/parser/bgp/messages.rs:470 (CONFIRMED, cleanup)

Non-extended OPEN encoding emits a user-supplied param_type of 255 as the first parameter byte, which this crate's own parser (and any RFC 9072 peer) interprets as the extended-format marker
Failure scenario: BgpOpenMessage { extended_length: false, opt_params: [OptParam { param_type: 255, param_value: Raw(..) }] } encodes to bytes where parse_bgp_open_message sees first param_type == 255 with nonzero opt_params_len and switches to RFC 9072 extended parsing — the round trip yields garbage parameters or a parse error instead of the original message. The encoder should force extended framing or return an EncodingError when the first parameter type is 255.
Verifier evidence: Encoder (src/parser/bgp/messages.rs, BgpOpenMessage::try_encode) writes any user-supplied type unchecked in the non-extended path: for (param_type, value) in encoded_params { buf.put_u8(param_type); ... buf.put_u8(value.len() as u8); } with no guard for param_type == 255, and use_extended_length = self.extended_length || non_extended_params_len > u8::MAX as usize stays false for a small param. OptParam's fields are public (src/models/bgp/mod.rs:99 pub param_type: u8), so BgpOpenMessage { extended_length: false, opt_params: [OptParam { param_type: 255, param_value: Raw(..) }] } is constructible. The crate's own parser (parse_bgp_open_message, same file) then misreads the output: if opt_params_len != 0 && param_type == 255 { // RFC 9072 ... extended_length = true; opt_params_len = input.read_u16()?; } — it treats the emitted 255 as the RFC 9072 marker and reinterprets the 1-octet param length plus value bytes as a 2-octet extended length, so the round trip yields garbage parameters or a ParserError instead of the original message. No EncodingError::ValueTooLarge (or any other) arm rejects param_type 255.

[16] src/parser/mrt/messages/table_dump.rs:151 (CONFIRMED, cleanup)

TableDumpMessage::try_encode hand-rolls a loop over self.attributes.inner that duplicates the existing Attributes::try_encode helper (which rib_afi_entries.rs already calls)
Failure scenario: Duplicated encoding logic: the loop plus intermediate BytesMut re-implements Attributes::try_encode(AsnLength::Bits16) added in this same branch; any future change to attribute-sequence encoding must be made in two places, and this file already diverges stylistically from the sibling RibEntry encoder that uses the helper.
Verifier evidence: table_dump.rs:150-154 hand-rolls the loop: let mut attr_bytes = BytesMut::new(); for attr in &self.attributes.inner { attr_bytes.extend(attr.try_encode(AsnLength::Bits16)?); } — this is byte-for-byte the same logic as the helper this branch adds at src/parser/bgp/attributes/mod.rs:608-614: pub fn try_encode(&self, asn_len: AsnLength) -> Result<Bytes, EncodingError> { let mut bytes = BytesMut::new(); for attr in &self.inner { bytes.extend(attr.try_encode(asn_len)?); } Ok(bytes.freeze()) }. The sibling encoder in table_dump_v2/rib_afi_entries.rs:205 already uses the helper (let attr_bytes = self.attributes.try_encode(AsnLength::Bits32)?;), so table_dump.rs could be let attr_bytes = self.attributes.try_encode(AsnLength::Bits16)?;. This is a reuse/simplification finding with no behavioral difference (both produce identical bytes and identical error propagation), but the duplication claim is factually accurate as stated.

[17] src/error.rs:56 (CONFIRMED, cleanup)

~22 near-identical X::try_from(len).map_err(|_| EncodingError::ValueTooLarge { field, actual, max })? blocks are copy-pasted across 12 files instead of a shared helper like EncodingError::check_u8/check_u16(field, len)
Failure scenario: About 100 lines of 6-line boilerplate repeated 22 times (30 ValueTooLarge construction sites total); each block varies only in field name and target type, so any change to the error shape or message requires touching every encoder file, and each new encoder invites another slightly-different copy (the wrong max bound in flowspec nlri.rs is exactly the kind of per-copy mistake this enables).
Verifier evidence: grep finds 24 copies of the identical pattern X::try_from(...).map_err(|_| EncodingError::ValueTooLarge { field, actual, max })? across 14 files (e.g. src/parser/bgp/messages.rs:461,474,619,629,709; table_dump.rs:157; geo_peer_table.rs:148,162; peer_index_table.rs:152,164; rib_afi_entries.rs:207; attr_37_sfp.rs:35; attr_23_tunnel_encap.rs:127; flowspec/nlri.rs:94), each varying only in field name and target type, while src/error.rs:49-74 defines only the enum + Display/Error impls with no shared check_u8/check_u16 helper. The candidate's cited per-copy mistake is real: src/models/bgp/flowspec/nlri.rs:94-98 uses max: u16::MAX as usize but encode_length (nlri.rs:130: data.push(0xF0 | ((length >> 8) as u8))) writes a 12-bit length capped at 4095, so lengths 4096..=65535 pass the check yet corrupt the wire encoding — exactly the per-copy bound error the duplication enables.

[18] src/models/bgp/linkstate.rs:191 (CONFIRMED, cleanup)

Tlv::length() (and SubTlv::length() in tunnel_encap.rs:132) now silently saturates at u16::MAX while the encode path bypasses it and recomputes u16::try_from(tlv.value.len()) — the method is dead on all production paths (only tests call it) and is a footgun for downstream users
Failure scenario: Two sources of truth for TLV length: a public method that returns a silently-wrong 65535 for oversized values (the silent-truncation behavior this branch removes elsewhere) and a separate checked conversion inside encode_link_state_attribute. A downstream caller using tlv.length() to size buffers or emit lengths gets corrupt output with no error; either length() should be the checked helper the encoder calls, or it should be removed.
Verifier evidence: Tlv::length() at src/models/bgp/linkstate.rs:191 is now self.value.len().min(u16::MAX as usize) as u16 (same pattern in SubTlv::length() at src/models/bgp/tunnel_encap.rs:132): for a Tlv whose value.len() > 65535 it silently returns 65535 — exactly the silent-truncation behavior this branch removes everywhere else. The encode path bypasses it: attr_29_linkstate.rs replaced the old bytes.put_u16(tlv.length()) with let len = u16::try_from(tlv.value.len()).map_err(|_| EncodingError::ValueTooLarge {...})? (encode_link_state_attribute, unknown-attributes loop), and encode_tunnel_encapsulation_attribute likewise uses u16::try_from(sub_tlv.value.len()) directly, never calling SubTlv::length(). Grep over src/ shows the only remaining callers are tests: src/models/bgp/linkstate.rs:615 assert_eq!(tlv.length(), 3) and src/models/bgp/tunnel_encap.rs:344 assert_eq!(sub_tlv.length(), 4). So the public method is dead on all production paths and returns a silently-wrong 65535 for oversized values; a downstream caller sizing buffers or emitting wire lengths from tlv.length() gets corrupt output with no error. Concrete trigger: Tlv { tlv_type: 0, value: vec![0u8; 70000] }.length() returns 65535 with no indication of overflow. The doc comment acknowledges the saturation but the two-sources-of-truth footgun (documented-saturating public API vs. checked private path) is real as claimed.

[19] src/parser/bgp/messages.rs:474 (CONFIRMED, cleanup)

The per-parameter u16::try_from(value.len()) error arm in the extended OPEN path is unreachable: the aggregate agg_len check at line ~430 already bounds encoded_params_len (= 3*count + values_len >= value.len() + 3) to u16::MAX
Failure scenario: Dead error-handling code (an EncodingError arm that can never fire) adds a second 6-line ValueTooLarge block and a misleading field name ("BGP OPEN extended optional parameter length") that no test can cover; a debug_assert or a comment referencing the aggregate check would do the same job with less code.
Verifier evidence: src/parser/bgp/messages.rs: the per-param check at lines 473-478 (u16::try_from(value.len()).map_err(|_| EncodingError::ValueTooLarge { field: "BGP OPEN extended optional parameter length", ... })?) only executes when use_extended_length is true (line 472), and the aggregate check at lines 460-465 (u16::try_from(encoded_params_len).map_err(... "BGP OPEN extended optional parameters total length" ...)?) runs first under the exact same flag (line 457). Since line 435 defines encoded_params_len = 3 * encoded_params.len() + values_len in the extended case and line 427 defines values_len as the sum of all value.len(), inside the loop value.len() <= values_len <= encoded_params_len - 3 <= u16::MAX - 3 once the aggregate check passes — so the per-param try_from can never fail; the arm is provably unreachable dead code. The sibling non-extended branch (lines 481-484) already handles the analogous guaranteed-fits case with a debug_assert!(value.len() <= u8::MAX as usize) plus comment, confirming that pattern would suffice here too. No test covers this arm (all try_encode error-path tests hit other fields).

[20] src/parser/bgp/messages.rs:491 (CONFIRMED, cleanup)

Doc comment on BgpOpenMessage::encode reads "Infinitely convenient infallible encoding wrapper." — a nonsense phrase (presumably meant "Infallible convenience wrapper")
Failure scenario: The garbled sentence ships in rustdoc for a public API method; users reading the generated docs for encode() see meaningless text where the panic contract of the wrapper should be introduced.
Verifier evidence: src/parser/bgp/messages.rs:491 reads exactly /// Infinitely convenient infallible encoding wrapper. — a garbled phrase ("Infinitely convenient" instead of something like "Infallible convenience wrapper") — and it is the first rustdoc line on the public method pub fn encode(&self) -> Bytes (line 495) of BgpOpenMessage, so it ships as the summary sentence in generated docs for this public API.

[21] src/parser/bgp/messages.rs:1346 (CONFIRMED, cleanup)

New tests construct Attributes via struct literal (inner/validation_warnings/attr_mask: [0; 4]) in four places instead of using the existing From<Vec> impl (models/bgp/attributes/mod.rs:400)
Failure scenario: Tests duplicate knowledge of Attributes' internal fields and build values with an attr_mask inconsistent with the attributes they contain; if a field is added or attr_mask semantics change, four test sites (messages.rs lines ~1346, ~2134, ~2161, plus the table-dump test) break instead of none, whereas Attributes::from(attrs) already produces a consistent value.
Verifier evidence: The diff adds struct-literal constructions of Attributes in new tests at src/parser/bgp/messages.rs:1343-1347 (attributes: Attributes { inner: attrs, validation_warnings: vec![], attr_mask: [0; 4] } in test_fallible_encoding_update_attributes_oversize), and identically at lines 2131-2135 (TableDumpMessage test) and 2158-2162 (RibEntry test) — three sites, not four as claimed (the 'table-dump test' at ~2134 is the same site the candidate counted separately). The existing impl From<Vec<Attribute>> for Attributes at src/models/bgp/attributes/mod.rs:400-408 computes the mask via compute_mask(&value), so Attributes::from(attrs) would produce a consistent value; the literals instead hardcode attr_mask: [0; 4], which is inconsistent with the contained code-200 Raw attributes (compute_mask at mod.rs:385 would set their bits). Fields are pub(crate) (mod.rs:142-146), so these in-crate tests compile but duplicate internal-field knowledge and would break if a field is added. Maintainability finding, no runtime bug: the encode path under test iterates inner and does not consult attr_mask.

[22] src/parser/mrt/messages/table_dump_v2/geo_peer_table.rs:229 (CONFIRMED, cleanup)

Test fixtures add pointless .min(u16::MAX as usize) clamps to the length of constant string literals ("test-view", "private-view") at lines 229, 316, and 453
Failure scenario: Noise with no effect: a 9-byte literal can never exceed u16::MAX, so the clamp only obscures the test fixture and falsely implies an overflow concern; view_name.len() as u16 was already exact for these constants.
Verifier evidence: All three clamps exist exactly as claimed in the diff and current file. Line 229: data.put_u16(view_name.len().min(u16::MAX as usize) as u16); with let view_name = "test-view"; (9 bytes). Line 316: same pattern with let view_name = "private-view"; (12 bytes). Line 453: expected.put_u16(view_name.len().min(u16::MAX as usize) as u16); with "test-view". Each operates on a constant string literal whose length (9 or 12) is provably below u16::MAX (65535), so .min(u16::MAX as usize) is a compile-time-obvious no-op; the pre-diff view_name.len() as u16 was already exact for these constants. This is dead defensive code in test fixtures with no observable effect other than obscuring the fixture — the candidate's claim is factually accurate.

[23] src/parser/bgp/attributes/attr_23_tunnel_encap.rs:100 (CONFIRMED, cleanup)

Both branches of the sub_tlv_type < 128 conditional start with the identical sub_tlv_bytes.put_u8(sub_tlv_type as u8) line (100 and 110) — the type write can be hoisted above the if, leaving only the u8-vs-u16 length write in the branches
Failure scenario: Copy-paste with slight variation kept by the rewrite: the duplicated type-byte write plus two near-identical ValueTooLarge blocks with the same field string make the function longer and let the two branches drift apart in future edits, when only the length-field width actually differs.
Verifier evidence: src/parser/bgp/attributes/attr_23_tunnel_encap.rs lines 100 and 110: both branches of if sub_tlv_type < 128 begin with the identical statement sub_tlv_bytes.put_u8(sub_tlv_type as u8);. Only the subsequent length write differs (line 108 sub_tlv_bytes.put_u8(len); vs line 118 sub_tlv_bytes.put_u16(len);), and the two ValueTooLarge blocks (101-107, 111-117) are near-identical, sharing the same field string "Tunnel Encap sub-TLV value length" and differing only in u8 vs u16 try_from/MAX. Hoisting the type-byte write above the conditional is behavior-preserving; this is a confirmed duplication/simplification finding with no runtime failure mode.

[digizeph]

closed, prefer #315 solution