Update Label B6 — full FANS 1/A ADS-C decode#422
Update Label B6 — full FANS 1/A ADS-C decode#422thepacket wants to merge 1 commit intoairframesio:masterfrom
Conversation
Extended to handle additional sub-formats and/or bug fixes.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 55 minutes and 7 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kevinelliott
requested review from
Copilot and
makrsmark
and removed request for
makrsmark
There was a problem hiding this comment.
Pull request overview
Replaces the previous stub Label B6 decoder with a structured FANS 1/A ADS-C “Provide ADS Report” envelope parser, extracting addressing metadata and preserving the ADS-C payload for downstream decoding.
Changes:
- Implemented ADS-C
.ADS.envelope parsing for multiple observed message forms (prefixed and direct). - Added tail/payload splitting logic using a registration-pattern cascade plus fallback heuristics.
- Surfaced key extracted fields (flight, ground, tail, contract request number, payload hex) into
rawand formatted items.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| decode(message: Message, options: Options = {}): DecodeResult { | ||
| const decodeResult = this.defaultResult(); | ||
| decodeResult.decoder.name = this.name; | ||
| decodeResult.formatted.description = 'CPDLC Message'; | ||
| decodeResult.message = message; | ||
| const decodeResult = this.initResult( | ||
| message, | ||
| 'FANS-1/A ADS-C Provide ADS Report', | ||
| ); |
There was a problem hiding this comment.
Maintainer reviewer agree — this is a Must Fix. 200+ lines of new envelope/regex/cascade logic without a single unit test is a regression-risk magnet. Please add Label_B6.test.ts covering at least: the three documented envelope forms; an HL-tail-with-all-hex case (the motivation for the cascade); an invalid envelope (decoded:false); and a short-payload case (verifies the ADSNOTE row). Without this, future changes to the regex or cascade will silently break decoding.
| return { | ||
| labels: ['B6'], | ||
| preambles: ['/'], | ||
| preambles: ['/', 'J', '1'], |
There was a problem hiding this comment.
Maintainer reviewer agree — Must Fix. The dispatcher routes by labels AND preambles, so any B6 envelope starting with a sublabel character not in ['/', 'J', '1'] is silently dropped. Recommend dropping preambles entirely and letting labels: ['B6'] plus the .ADS. envelope match drive selection. (If preambles must stay for performance reasons, broaden to all valid 2-char sublabel starts — though that approaches [A-Z0-9] which is effectively no filter.)
| const m = | ||
| text.match(envelopeRe(2)) || | ||
| text.match(envelopeRe(1)) || | ||
| text.match(envelopeRe(0)) || | ||
| text.match(directRe); |
There was a problem hiding this comment.
Maintainer reviewer agree — Should Fix. Pick one: either extend the matcher to try envelopeRe(3) first, or tighten the docblock to '0–2 chars' to match what's actually implemented. The current discrepancy will mislead future maintainers. If 3-char msgblocks are observed in the wild (the docblock implies they are), extending the matcher is the right call — the cascade ordering (try longest first) is already the correct strategy.
| const afterCand = rest.substring(cand.length); | ||
| if (/^[0-9A-F]*$/i.test(afterCand)) { | ||
| tail = cand; | ||
| payload = afterCand.toUpperCase(); |
There was a problem hiding this comment.
Maintainer reviewer agree — Should Fix. The cascade path uppercases via afterCand.toUpperCase(); the fallback path (payload = rest.substring(...)) preserves casing. Recommend normalizing to uppercase in both paths and updating the docblock to say 'payload normalized to uppercase hex' rather than 'preserved verbatim'. Uppercase is conventional for ARINC hex and matches what most downstream tools (libacars, dumpvdl2) emit.
| if (!tail) { | ||
| // Fallback: non-hex scan in the first 8 chars | ||
| const window = rest.substring(0, Math.min(8, rest.length)); | ||
| let lastNonHex = -1; | ||
| for (let i = 0; i < window.length; i++) { |
There was a problem hiding this comment.
Maintainer reviewer agree — Should Fix. The fallback path can produce non-hex payloads and still mark the message decoded: true, level: full. At minimum: after the fallback split, validate /^[0-9A-F]*$/i.test(payload) and call failUnknown (or setDecodeLevel(decodeResult, false)) if it fails. Better: also reject if payload.length is odd (hex bytes are always pairs). This also has implications for the ADS Contract Request Number extraction at line ~150 which assumes pure hex.
kevinelliott
left a comment
kevinelliott
left a comment
There was a problem hiding this comment.
Summary
Replaces the previous stub Label_B6_Forwardslash plugin (which only emitted 'CPDLC Message' and never set decoded = true) with a full FANS-1/A ADS-C "Provide ADS Report" envelope decoder. Handles three observed envelope forms (standard with flight prefix, 2-char msgblock, direct without sublabel/flight) and uses a registration-pattern cascade (HL, JA, B-, hyphenated, N-numbers) to split tail from binary payload — necessary because hex-only payload bytes can look identical to digit-bearing tails like HL8501.
Verdict
Comment-only review. The previous code was a stub — this is a real, well-documented decoder. Direction is excellent. The tail-splitting cascade is clever and handles the common-case ICAO patterns. Before merge, however, this PR needs unit tests and the qualifiers().preambles mismatch needs to be resolved (see Copilot review below).
Must Fix
-
No tests. This plugin previously had no test file (
Label_B6.test.tsdoes not exist), and adding 200+ lines of envelope/regex logic without tests is a regression-risk magnet. Please addLabel_B6.test.tscovering at least:- The three documented envelope forms in the PR body.
- An
HL(Korean) tail with all-hex digits — the case that motivates the cascade. - An invalid envelope — verify it returns
decoded: false. - A short-payload case — verify the
ADSNOTErow appears. - The
preamblesfilter (see #2 below).
-
qualifiers().preamblesis too narrow. You set['/', 'J', '1'], but documented envelopes start with14,J7, and/. The decoder regex accepts any 2-char[A-Z0-9]{2}sublabel, but messages are dispatched to plugins by matching againstpreambles— so0x...,2x...,Ax...envelopes never reach this plugin. Either (a) droppreamblesand let the engine route by label only, or (b) broaden to all valid sublabel starts. (Copilot raised this — I agree.)
Should Fix
- Casing inconsistency: in the cascade path you
payload = afterCand.toUpperCase(); in the fallback path you keep original casing. The docblock says payload is "preserved verbatim". Pick one — I'd suggest normalizing to uppercase in both paths and updating the comment to say "payload normalized to uppercase hex". - Fallback validates nothing. If the envelope matches but the registration cascade fails, the fallback grabs
rest.substring(0, 6)regardless of whether the result is hex. That setsdecoded: true, level: fullfor arbitrary garbage. At minimum, validatepayloadmatches/^[0-9A-F]*$/iafter the fallback split, andfailUnknownif not. (Copilot raised this — I agree.) - 3-char msgblock isn't tried. The docblock says "1–3 chars" but you only try lengths 2, 1, 0. Either tighten the docblock to "0–2 chars" or extend the matcher to try
envelopeRe(3)first. Pick whichever matches reality. (Copilot raised this — I agree.) - The ADS Contract Request Number is the first byte — you correctly extract
payload.substring(0, 2)as hex. Worth noting in the formatted row that this is byte 1 of the ARINC-745 "basic group" header, so consumers know what they're looking at. - The
payloadIsShortheuristic (≤ 8 hex chars = "control plane") is a reasonable guess, but FANS-1/A actually has well-defined message types — short payloads here are typicallyProvide ADS Reportwith a basic-group-only payload, contract requests have their own labels. The annotation as written is helpful for human readers; consider softening from "likely contract-setup" to "may be a short-form ADS report or contract control message — full FANS-1/A ASN.1 decoder needed for definitive type".
Nits
- The 3-form regex builder
envelopeRe(msgLen)is neat but expensive (constructs a new RegExp on every decode). Consider hoisting the three regexes (lengths 0/1/2) to module-level constants. (m.groups as any).sublabel as string | undefined— the named group types arestring | undefinedalready (since TS 4.5 withnoUncheckedIndexedAccessthey're correctly typed). Theas anycasts can probably go away.- The unshift of the
MSGTYProw + later push ofSUBLBL,MSGNUM,GND,ADSREQ,ADSPAYLD,ADSNOTEproduces a stable order, but it's worth committing to that order in tests so it doesn't drift.
Backward Compat / Regression Risk
- Previous behavior: stub returned
decoded: false, level: 'none', formatted description'CPDLC Message', no items. Anything matched here was effectively undecoded. - New behavior:
decoded: true, level: 'full', items list of 4–7 rows. - This is a strict upgrade for messages that match the envelope, and a non-change for messages that don't. Caveat: the old description was
'CPDLC Message'. The new description is'FANS-1/A ADS-C Provide ADS Report'. These are different message classes — the file/class nameLabel_B6_Forwardslashoriginally implied CPDLC, and ADS-C is functionally a different protocol. Make sure the project intent for label B6 is ADS-C, not CPDLC. The PR body asserts ADS-C; please confirm with the airframes team before merging.
Test Coverage
Currently zero. See Must Fix #1.
Outstanding Existing Comments
Five inline comments from Copilot. My stance, replied inline:
- #3141135599 (no tests): Agree. Must Fix.
- #3141135616 (preambles too narrow): Agree. Must Fix.
- #3141135624 (3-char msgblock not handled): Agree. Should Fix — at minimum align doc and code.
- #3141135634 (payload casing inconsistency): Agree. Should Fix.
- #3141135643 (fallback doesn't validate hex): Agree. Should Fix.
All five are actionable and uncontested. Please address before merge.
Thanks @thepacket! This is a substantial improvement over the stub and the docblock is excellent — please add tests, fix the preambles, and address the Copilot comments before merge.
Replaces the stub
Label_B6_Forwardslashplugin with a full FANS 1/A ADS-C Provide-ADS-Report decoder.Handles three envelope forms:
14ASQ0431/SKYSWSQ.ADS.9V-SMI07…— standard with flight prefixJ70AQR040X/MELCAYA.ADS.A7-ANT030BA821— 2-char msg block/RPHIAYA.ADS.HL8501070ED832A9374985A7941D0E…— direct (no flight prefix)Tail/payload split uses a registration-pattern cascade (Korean HL, Taiwan/China B-, European dashed, US N-numbers, Japanese JA) to correctly split all-hex tails.
Surfaces ground, tail, first-byte contract request number. Binary payload preserved as hex for downstream ARINC-745 decoding.
npm run buildpasses.