Repository navigation
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #347 +/- ##
==========================================
+ Coverage 92.12% 92.18% +0.05%
==========================================
Files 96 97 +1
Lines 23534 23970 +436
==========================================
+ Hits 21680 22096 +416
- Misses 1854 1874 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
AFI/SAFI scope and missing-NLRI escalation currently produce incorrect RFC 7606 outcomes.
Review effort: Balanced
Findings: 1
Open (3)
What changed in this PR
Adds opt-in RFC 7606 error classification and handling across BGP parsing, element/route iteration, filtering, CLI output, and public APIs.
Changes:
- Classifies validation findings as discard, treat-as-withdraw, AFI/SAFI disable, or session reset.
- Adds
RESETelements, handling metadata, parser/CLI controls, and filtering. - Expands RFC 7606 tests and documentation.
| File | Description |
|---|---|
tests/test_wasm_type_fixtures.rs |
Updates element fixtures. |
tests/test_rfc7606_error_handling.rs |
Adds end-to-end RFC 7606 tests. |
src/wasm/js/index.d.ts |
Extends element TypeScript types. |
src/wasm/js/generated/ErrorHandlingApproach.ts |
Adds generated approach type. |
src/parser/text_dump.rs |
Initializes handling metadata. |
src/parser/rislive/mod.rs |
Initializes RIS Live metadata. |
src/parser/mrt/mrt_elem.rs |
Applies approaches during element conversion. |
src/parser/mod.rs |
Adds parser configuration API. |
src/parser/iters/update.rs |
Propagates handling configuration. |
src/parser/iters/route.rs |
Adds selective RFC 7606 route handling. |
src/parser/iters/recovery.rs |
Configures recovery iterators. |
src/parser/iters/raw.rs |
Configures filtered raw iteration. |
src/parser/iters/mod.rs |
Documents and propagates mode. |
src/parser/iters/fallible.rs |
Configures fallible iterators. |
src/parser/iters/diagnostic.rs |
Updates warning expectations. |
src/parser/iters/default.rs |
Configures default iterators. |
src/parser/filter.rs |
Adds reset filtering. |
src/parser/bgp/attributes/README.md |
Documents RFC handling rules. |
src/parser/bgp/attributes/mod.rs |
Expands attribute validation. |
src/models/bgp/mod.rs |
Exports handling models. |
src/models/bgp/error_handling.rs |
Implements classification and discard plans. |
src/models/bgp/elem.rs |
Adds reset type and handling field. |
src/models/bgp/attributes/mod.rs |
Introduces attribute-code set. |
src/lib.rs |
Documents the public feature. |
src/error.rs |
Clarifies malformed-NLRI handling. |
src/bin/README.md |
Documents CLI options. |
src/bin/main.rs |
Adds --rfc7606. |
README.md |
Synchronizes generated documentation. |
examples/update_messages_iter.rs |
Handles non-exhaustive element types. |
examples/treat_as_withdrawal.rs |
Demonstrates the new APIs. |
examples/README.md |
Updates example description. |
CHANGELOG.md |
Records API and behavior changes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+830
to
+834
| announced: msg.announced_prefixes.into_iter().chain(nlri_announced), | ||
| withdrawn: msg.withdrawn_prefixes.into_iter().chain(nlri_withdrawn), | ||
| in_withdrawn_phase: false, | ||
| announced_as: approach.announced_elem_type(), | ||
| error_handling: Some(approach), |
Comment on lines
+358
to
+363
| /// Whether any attribute other than MP_UNREACH_NLRI is present. | ||
| pub(crate) fn has_attrs_other_than_mp_unreach(&self) -> bool { | ||
| self.inner | ||
| .iter() | ||
| .any(|attribute| attribute.value.attr_type() != AttrType::MP_UNREACHABLE_NLRI) | ||
| } |
Comment on lines
+373
to
+374
| let has_reachable_nlri = !self.announced_prefixes.is_empty() | ||
| || self.attributes.has_attr(AttrType::MP_REACHABLE_NLRI); |
Map every path attribute to the error-handling approach its RFC prescribes when it is malformed (attribute discard, treat-as-withdraw, AFI/SAFI disable, session reset), classify each validation finding with it, and combine an UPDATE's findings with the strongest-action rule (RFC 7606 §3(h)) and the missing-NLRI escalation (§5.2). MRT data does not record the session type, so the classification assumes eBGP. Close detection gaps: attribute-list overruns and trailing bytes, the remaining RFC length rules, unrecognized well-known attributes, empty AS path segments and AS 0 (RFC 7607). Malformed ORIGIN, AS_PATH, NEXT_HOP and MP attributes report specific findings. Opt in with BgpkitParser::enable_rfc7606_error_handling(), Elementor::with_error_handling or --rfc7606: treat-as-withdraw announcements become WITHDRAW elems, session-reset-class ones the new ElemType::RESET, and discarded attributes are dropped. Elems carry the approach in BgpElem::error_handling. The route iterator judges UPDATEs as a minimal speaker on its selective parser. Assisted-by: Claude Code
…erator Under RFC 7606 error handling the route iterator treated every attribute outside its parsed set as unrecognized, so a known attribute such as COMMUNITIES with the optional bit clear became a session reset (RESET) where the elem iterator reports a flags error (WITHDRAW). Run the shared header checks (flags, lengths, duplicates, unrecognized well-known codes) on every attribute instead, and skip only the value parsing of attributes the route iterator does not use. The checks are allocation-free, and the route verdict is now a subset of the elem verdict: it can miss value errors in skipped attributes, never invent one. Assisted-by: Claude Code
- parser/rislive: stamp the new BgpElem.error_handling field (None) at both RIS Live elem constructors; only compiled with the rislive feature. - attributes: pass the attribute value to observe_parse_error by re-slicing the section handle, matching main's zero-copy raw retention. - wasm type-check: ExactKeys now requires only the declared type's required keys; serde skips None fields, so optional keys cannot appear in fixtures.
digizeph
force-pushed
the
feat/rfc7606-treat-as-withdraw
branch
from
October 8, 2026 18:23
67ead72 to
bf3b2c7
Compare
Member
|
Rebased onto current Conflict-resolution notes (the parts worth reviewing):
CI fix:
Verified locally before pushing: The branch history was rewritten in the push (original tip |
ties
changed the title
ties
marked this pull request as draft
October 11, 2026 14:52
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A pass at implementing rfc7606 behaviour for treat-as-withdraw that is fully AI driven (Claude).
Choices:
Not sure I like the API changes (additional enum variety) and I am also not sure if I want rfc7606 behaviour to be a feature flag. Furthermore the comments in the code also need to be more succinct.