Support full-width MAVLink source and destination system IDs

[tridge]

Preserve full-width source and destination system IDs when forwarding MAVLink frames, including signed frames and unknown payload extension bytes. Widen log replies, reboot filters, status identities and configuration without changing the database layout. Ordinary 8-bit targets remain in the message payload; wider targets use the TARGET32 header extension.

Build against the merged ArduPilot MAVLink/pymavlink revision. Exercise the default pymavlink tlog reader, include previously omitted regression tests in the standard runner, and continue through every test phase while retaining a failing exit status. Document extension-aware peer requirements and configuration precautions for rollback to older binaries.

Stabilize parallel integration tests by waiting for listeners and completed publisher handshakes, cleaning up test process groups, and restricting backend-process checks to the relevant proxy. Orphan checks still track captured backend PIDs after their parent exits.

Validation: Docker and Test CI checks pass on 8503e6a. Local validation includes 59 connection, 13 authentication, 301 robustness and 305 webadmin tests, plus all 90 tests in the two affected fixture files with 16 workers. Claude reviewed the dependency, runner and fixture changes with no remaining findings.

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-09-25)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Full report: https://uav.tridgell.net/DevCallReviews/2026_09_25_AIReview/devcall_pr_reviews.html#prSupportProxy-46

Reviewed at head e447756dbb. Verdict: COMMENT — no blockers.

The relay change is correct, and I checked that by mutation rather than by reading. Five separate reversions each fail the new tests: turning off the MAVLINK_IFLAG_TARGETTED branch (6 of 9 test_sysid32 items), reverting the finalize length from msg.len back to max_len (same 6, on No matching FILE_TRANSFER_PROTOCOL received — the 255th extension byte really is lost without it), narrowing fc_sysid_filter_ to uint8_t (3 of 3 binlog items), narrowing the binlog target_system to uint8_t (3 of 3), and narrowing the webadmin validator back to 255 (6 of 14 form items). Built at -O2 with the project's own -Werror set; tests/test_sysid32.py + tests/test_keydb_log.py give 39 passed and tests/webadmin/test_wide_sysid_forms.py 14 passed against the pinned pymavlink 2.4.50.

Three things worth a look, none of them a defect in this repo's own code.

1. Every relayed frame now advertises MAVLINK_CFLAG_SYSID32 on the original sender's behalf. mavlink.cpp:246 → the pinned finalizer sets msg->compat_flags = mavlink1?0:MAVLINK_CFLAG_SYSID32 unconditionally (mavlink_helpers.h:345); the revision this PR replaces set compat_flags = 0. A compiled probe mirroring send_message() on a frame with compat_flags=0 emits compat_flags=1 (wire byte 3 = 1), and git show 7bdb2dc...'s pymavlink confirms the old value. The relay is the one component that cannot honestly answer that flag — it is speaking for an endpoint whose capability it does not know, and the realistic case is an existing MissionPlanner user whose frames now reach the far side claiming a capability they do not have.

Severity stated honestly: nothing in the pinned stack currently reads CFLAG_SYSID32 — every reference in mavlink_helpers.h, mavgen_python.py and mavgen_javascript.py is a write, and all senders set it unconditionally. So this is latent, not currently breaking anything, which is why it is not a blocker. But it is the only place this PR changes wire behaviour for traffic that has nothing to do with wide IDs, so it seems worth a deliberate decision. The fix shape is upstream in ArduPilot/mavlink: a finalize variant that preserves the caller's compat_flags, which a relay would use.

2. Wide-ID .tlog files are silently truncated by the default pymavlink reader. mavutil.mavlink_connection(<file>.tlog) returns mavmmaplog, whose indexer reads the MAVLink2 message ID at fixed offsets ofs+15..17 and advances by a fixed mlen += 12 (modules/mavlink/pymavlink/mavutil.py:1543 and :1546). It accounts for the signature block via incompat_flags but not for the two new header extensions, so a SYSID32 (+3) or TARGETTED (+4/+5) frame is read at the wrong offset and skipped. Reproduced: a tlog of five HEARTBEATs with sources 42 / 0xFEDCBA98 / 43 / 0x80000001 / 44 yields only the three narrow ones through mavlink_connection, and all five through mavutil.mavlogfile. That is presumably why the new test bypasses the default reader with the comment "The mmap indexer still assumes fixed-size MAVLink2 headers" (tests/test_sysid32.py:168).

The consequence is operational: mavlogdump.py, MAVExplorer and anything else built on mavlink_connection will quietly lose exactly the frames this feature exists for. The .bin binlog path is unaffected. Worth fixing in the pinned indexer before support logs depend on it, or at least noting in the description that wide-ID tlogs need mavlogfile today.

3. This PR's own webadmin test never ran in CI. scripts/run_tests.py:53 — the runner executes phases with subprocess.check_call, so the first failing phase aborts it. The Robustness phase failed, and the Webadmin phase after it never started: the job log has === Running Connection Tests ===, === Running Authentication Tests === and === Running Robustness Tests === and no Webadmin line, and zero occurrences of test_wide_sysid_forms. So the 14 parametrisations that are the only coverage of the widened form validators did not execute. I ran them here (14 passed, and 6 fail if the validator goes back to 255), so the file has teeth — it just never fired. Letting the runner continue through the remaining phases and report a combined failure would stop a flake in one phase hiding the others.

Smaller notes:

• scripts/run_tests.py:31 — while you are editing PHASES, seven other files are in no phase at all and so never run in CI: test_binlog_capture.py (26 test functions), test_log_cleanup.py (6), test_tlog_capture.py (4), test_kill_drop.py (2), test_engineer_preauth_pool.py, test_engineer_udp_churn.py, test_setup_signing_guard.py (1 each). Zero <name>.py:: hits for all seven in the job log. The biggest one covers exactly the binlog ACK / target_system path this PR widens, and test_sysid32.py already imports its helpers — adding it and test_tlog_capture.py would cost nothing.
• mavlink.cpp:204 — the MAVLINK_IFLAG_SIGNED strip/set on msg2 is dead: the finalizer zeroes incompat_flags and re-derives SIGNED from status->signing. Pre-existing and harmless either way, but this PR is reworking exactly that block, so it is the natural moment to drop it or note that signing is decided elsewhere.
• supportproxy.cpp:279 — the record layout is genuinely unchanged (fc_sysid was already uint32_t at offset 104), but a rollback is asymmetric: an older binary still applies uint8_t(k.fc_sysid), so a configured 256 truncates to 0 = "match any" rather than failing visibly. Worth one line in the release note.
• CI is red only on tests/test_video_rtsp.py::TestRtmpIngest::test_no_orphan_backend_after_the_rtmp_publisher_leaves; nothing here touches video, and the same file flaked on pr-video-5-slots (run 34109326089). The proxy log also shows [18233] Failed to open TCP port 18233 - Address already in use, but I am not claiming that as the cause — the failing assertion waits for publisher gone and the video port bound fine on 18234.

Also checked and clean: MAVLINK_MAX_PACKET_LEN is 288 = 255 + 18 + 2 + 13, exactly the largest frame the relay can emit, so buf[MAVLINK_MAX_PACKET_LEN] is correct and exactly sized; the generated remote_log_block_status and statustext packers both take uint32_t, so binlog.cpp:406/:519 and the widened last_sysid are not narrowed on the way out (verified for 0, 256, 0x80000001, 0xFFFFFFFF); last_sysid/last_compid are still initialised in init(); MAVLINK_STATUS_FLAG_OUT_MAVLINK1 is never set anywhere, so the force_target-plus-MAVLink1 path where send_message() could newly drop a frame is unreachable; $(OBJECTS): $(MAVLINK_DIR)/protocol.h is the right dependency edge; regen_headers.sh's ${PYTHONPATH:+...} is correctly guarded against set -u; both generator/mavgen.py and tools/mavgen.py exist in the pinned pymavlink so the Makefile prerequisite and the Dockerfile's test -f both resolve; and .dockerignore excludes .git but not modules/, which is what makes the host-side submodule requirement work.

Not covered: no hardware, all loopback. The interop claim in point 1 is reasoned from the flag's definition plus the fact that nothing reads it yet — I did not test against a genuinely legacy peer, so "a peer that acts on it will mis-send" is a prediction. docker-test was not reproduced locally. Signing was exercised only via the new test's bad-signature and unsigned-warning cases; the replay/timestamp paths were not re-audited. The submodule bump was reviewed only where SupportProxy depends on it.

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-09-25)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Re-reviewed at head 38ef67c64f (previously e447756dbb). Full report, including everything checked and cleared: https://uav.tridgell.net/DevCallReviews/2026_09_25_AIReview/devcall_pr_reviews.html#prSupportProxy-46

Verdict: COMMENT — there is no defect in this repo's own code at this head: no memory-safety problem, and no possibility of cross-tenant routing or log leakage. It is not a clean APPROVE only because a separate cold review pass found a real behavioural asymmetry on a path our primary review had explicitly cleared. Everything below is a NOTE.

We built and tested this head locally (-O2 with the project's own -Werror set, libtdb-dev unpacked into scratch without sudo, venv with the pinned pymavlink 2.4.50) and proved each of the delta's behaviours by mutation.

Routing and cross-tenant isolation

Stating this unambiguously because it is the most serious thing that could be wrong in a relay: there is no sysid-keyed routing in SupportProxy, so a sysid above 255 cannot collide with, alias, or leak into another peer's traffic, and there is no (sysid << 8) | compid composite key anywhere in this codebase. Routing is by configured port pair plus the socket the byte arrived on, decided before any MAVLink parsing: the parent forks one child per port pair, each child holds mav1 and a conn2[] array for engineers on that one port2, and all four forwarding paths (supportproxy.cpp:698-716, :790-800, :896-910, :1026-1032) are an unconditional broadcast over that pair's slots with no sysid test. There is no learning path and no eviction, because no table maps a sysid to a connection. Both log writers place files under the configured port2 and session naming does not use a sysid. Isolation is process- and socket-level, not identity-level, so widening the sysid cannot reach it. The two identity-keyed structures that could have aliased are already 32-bit: signing replay state is keyed on {link_id, uint32_t sysid, compid}, and fc_sysid_filter_/target_system are the two fields this PR widens.

Two corrections to our own wording, from the Codex pass: our grep claim was too absolute (mavlink.h and keydb.h also match), and there is sysid-dependent filtering — at binlog.cpp:551, for binlog reboot detection — but it compares full-width values, so it does not alias.

Findings

NOTE — relaying a TARGET32 frame whose target is at or below 255 drops the header target. At mavlink.cpp:240-245 the relay re-finalises the frame, and mavlink_finalize_message_buffer_target() sets TARGET32 only when target_sysid > 255 — we confirmed that directly in the pinned mavlink_helpers.h (if (target_sysid > 255) { msg->incompat_flags |= MAVLINK_IFLAG_TARGET32; ... }). So a small header target is not re-emitted and the payload target byte governs instead. Measured through the actual compiled receive/send methods at -O2 with send() intercepted:

Incoming header target	Payload target	Forwarded effective target
7	0	0 — becomes broadcast
7	99	99 — changes recipient
0	99	99 — loses explicit broadcast
256	99	256 — preserved

The cold pass called this a BUG and returned REQUEST CHANGES; we have reduced it to a NOTE. A conforming sender cannot produce the frame: TARGET32 is set on the sending side only when the target exceeds 255 (the same condition, in both _mav_finalize and _mav_header_pack), set_target() was removed from pymavlink in this very series, and you have recorded that targeting without a payload target field is unsupported. So the only frames affected are non-conforming or hostile ones, and normalising those to payload targeting is defensible relay behaviour. Worth knowing that the finding-validation pass, testing the same code with a wide target (0x11223344), found preservation and reported no target-rewriting bug — the two agents disagreed only because they probed different values.

It is still worth raising for two reasons. It corrects our own primary review, which cleared this path as "forwarded verbatim" on the strength of a wide-target probe alone (Codex separately noted "verbatim" overstates it, since the relay re-finalises and re-signs). And the PR description opens with "Re-finalizing forwarded frames discarded explicit header targets ... Preserve targets" — so this is precisely the behaviour the PR sets out to fix; it is fixed for every value a conforming sender can emit, and not for the rest. Either a line in the description scoping the claim, or preserving target presence independently of its value. Note the delta also dropped the only regression test for a header/payload target disagreement (tests/test_sysid32.py:115-120, which used the now-removed set_target), so nothing pins this either way — a raw-write case with mismatched targets is about six lines.

NOTE — wide-ID tlogs are silently truncated by the default pymavlink reader. mavutil.py:1545 reads the MAVLink 2 msgid at fixed offsets ofs+15..17 and :1546 advances mlen += 12, adjusting only for MAVLINK_IFLAG_SIGNED — there is no SYSID32/TARGET32 header-extra accounting (grep for those names over the pinned mavutil.py returns zero hits). Reproduced by both passes: a 151-byte tlog of five HEARTBEATs from 42 / 0xFEDCBA98 / 43 / 0x80000001 / 44 yields [42, 43, 44] through mavutil.mavlink_connection (which returns mavmmaplog) and all five through mavutil.mavlogfile. The files retain the frames; the default reader skips exactly the wide ones this feature exists to produce, and that reader is what mavlogdump.py and MAVExplorer use on support logs. The PR's own test documents the limitation (tests/test_sysid32.py:169) rather than fixing it. This belongs in pymavlink#1229, not here — both passes agree — but it would be good to land there before the series merges, or to note in the description that wide-ID tlogs need mavlogfile today.

NOTE — the TARGET32 branch at mavlink.cpp:240-247 is redundant. mavlink_finalize_message_buffer is ..._target(..., 0), and the parser guarantees msg.target_sysid == 0 whenever TARGET32 is clear — it clears the field in GOT_LENGTH and again on the MAVLink 1 path in GOT_STX. We mutation-proved the equivalence: replacing the whole block with a single unconditional mavlink_finalize_message_buffer_target(..., msg.target_sysid) rebuilds clean and gives 9/9 passed. Codex independently confirmed the parser clearing at mavlink_helpers.h:768 and :784-788 with reused-buffer probes, so no accepted frame can carry a stale non-zero target. Five lines shorter if you want it — optional cleanup, no defect.

NOTE — the description does not link the companion PRs. We checked it directly. The pointer chain itself is correct and coherent: modules/mavlink → 7108ebf, the head of open ArduPilot/mavlink#515, whose pymavlink → 450e1db, the head of open ArduPilot/pymavlink#1229; and ardupilot#33734 at 99d4cef2a5 points at the same 7108ebf. An unmerged submodule pointer is normal practice and not a defect — the only expectation is the links, and for comparison ardupilot#33734's description lists all five. Two lines to add.

NOTE — a legacy engineer GCS loses the proxy's own diagnostics after a wide heartbeat. The proxy neither translates nor truncates a wide sysid: it forwards verbatim and a pre-2.1 peer drops the frame silently, because the old MAVLINK_IFLAG_MASK was 0x01. That is the right choice — truncating would alias two vehicles into one. The residual is that mavlink.cpp:215 remembers the full vehicle sysid and both mav_printf STATUSTEXT packs (:479, :495) use it, so the proxy's own warnings inherit the wide source and become unreadable to a legacy operator too. Codex confirmed with a compiled-method probe (a warning from 0xFEDCBA98 carries incompat flags 2, which the legacy mask rejects). It is correct by construction that a >255 sysid implies a 2.1 vehicle and that the pairing is broken end to end regardless — but the proxy is the one component positioned to notice and say so. Low priority; raised because it is the series' interop seam.

NOTE — seven test files are in no phase of the runner at all. Parsing PHASES for 'tests/*.py' literals gives 20 files; walking tests/ (excluding webadmin/) gives 27 plus the test_config.py helper. Not in any phase: test_binlog_capture.py, test_engineer_preauth_pool.py, test_engineer_udp_churn.py, test_kill_drop.py, test_log_cleanup.py, test_setup_signing_guard.py, test_tlog_capture.py — and all seven have zero <name>.py:: hits in this run's green CI log (Codex reproduced both the phase parse and the log absence). We ran the three most relevant: test_binlog_capture.py + test_tlog_capture.py + test_setup_signing_guard.py = 31 passed in 329 s. So they pass at this head, they would cost about 5.5 minutes, and test_binlog_capture.py (26 tests) covers exactly the binlog ACK / target_system path this PR widens — test_sysid32.py already imports its helpers. Worth adding to a phase. Structurally, scripts/run_tests.py:58 still uses subprocess.check_call, so the first failing phase aborts the rest.

NOTE — rollback asymmetry. supportproxy.cpp:279 now passes k.fc_sysid through untruncated, but an older binary reading the same configuration applies uint8_t(k.fc_sysid), so a configured 256 becomes 0 — and binlog.cpp:551 treats 0 as "match any" rather than failing visibly. Codex confirmed uint8_t(256)==0 by probe and scoped it correctly: this affects binlog reboot detection after a rollback, not cross-account forwarding. Release-note item.

Previous round

• RESOLVED — "every relayed frame now advertises MAVLINK_CFLAG_SYSID32", fixed by the submodule bump: the new pinned finalizer sets msg->compat_flags = 0 unconditionally and CFLAG appears nowhere in the pinned pymavlink's C headers or generators. Corroborating: the header extension shrank from 5 bytes to 4, which is why the test's expected frame sizes move 288/275 → 287/274.
• RESOLVED / confirmed flake — the red test check at the previous head was tests/test_video_rtsp.py::TestRtmpIngest::test_no_orphan_backend_after_the_rtmp_publisher_leaves. That exact test passes at this head, nothing here touches video, and the same file had flaked before on an unrelated branch. Not caused by this PR.
• RESOLVED as observed — our "this PR's own webadmin test never ran in CI" claim is no longer true: the test job log now shows === Running Webadmin Tests === with 28 test_wide_sysid_forms hits and 255 passed / 1 skipped. The structural cause (a single failing phase aborting the rest) is unchanged, but that claim was only true last round because the Robustness phase had flaked.

Checked and cleared

Five mutations, each rebuilt in the foreground with the binary's mtime verified before measuring — because a failed build leaves the old binary in place: dropping the TARGET32 branch → 6/9 fail; msg.len→max_len in both finalize calls → 6/9 fail (the 255th extension byte is genuinely lost); binlog.h:233 target_system→uint8_t → 3/3 binlog items fail; binlog.h:305 fc_sysid_filter_→uint8_t → 3/3 fail; unconditional ..._target → 9/9 pass. Codex corrected our explanation of the last two: narrowing fc_sysid_filter_ causes the initial wide-ID sample to be ignored so the subsequent matching reboot is missed — the low-byte sample does not trigger rotation in that sequence.

Frame sizing: 10 core + 3 SYSID32 + 4 TARGET32 + 255 payload + 2 CRC + 13 signature = 287, and a compiled probe confirms MAVLINK_MAX_PACKET_LEN=287, MAVLINK_MAX_HEADER_LEN=17, with the largest serialisation writing through index 286 — so uint8_t buf[MAVLINK_MAX_PACKET_LEN] at mavlink.cpp:191 fits exactly, with no off-by-one.

Untrusted input: Codex ran ASan/UBSan over the full serialisation matrix (2048 payload-length/flag combinations), 274 truncated frame prefixes with recovery padding, and one million pseudorandom parser bytes — no overflow. We separately drove hand-built hostile frames into the real binary over UDP: a truncated frame claiming len=255 with TARGET32, and a SYSID32|TARGET32 header cut off mid-target-field, cause no crash and no wedge, and a following wide frame forwards correctly. MAVLINK_IFLAG_MASK is 0x07 so unknown incompat flags are rejected in GOT_LENGTH; payload writes are bounded by rxmsg->len <= 255 against a 264-byte payload64[]; the GOT_PAYLOAD zero-fill is clamped.

Also cleared: the finalized_len == 0 guard is unreachable (MAVLINK_STATUS_FLAG_OUT_MAVLINK1 is never set in this codebase and mavlink_set_proto_version has no caller outside the generated tree), so MAVLink 1 input is still upgraded to MAVLink 2 output and no frame is newly dropped; keydb.h:157 fc_sysid was already uint32_t with a static_assert on offset 104, so the TDB layout is unchanged; both webadmin forms use NumberRange(min=0, max=0xFFFFFFFF) and the 14 new parametrisations pin the -1 and 0x100000000 rejections. The delta's test rewrite exercises wide sysids and wide targets through the real binary over all three transports × signed and bidi-signed, with the expected <= recorded tlog check keyed on unique per-case tags — not a "field is wide" assertion.

CI at this head: docker-test pass (1m23s), test pass (3m59s) — confirmed against the raw check-runs REST API (both completed/success, one check suite, no cancelled rows). There is no failing check at this head.

Not covered: no hardware, all loopback; docker build was not reproduced locally (we read the CI log); the signing replay/timestamp windows were not re-audited; the legacy-peer interop claim is reasoned from the old IFLAG_MASK value rather than measured against a real old GCS; and Codex could not run the socket-based tests at all (socket creation denied in its sandbox), so the live pytest counts above are ours.

Codex cross-check: the finding-validation pass returned 5 confirmed, 2 adjusted, 1 refuted, 0 new and judged the approval defensible; the cold pass, given only the PR, independently confirmed no cross-tenant aliasing and clean parser bounds but found the small-header-target normalisation above — which is exactly what a cold spot-check exists for, since it lands on a path the primary review had cleared rather than on a finding it had made.

[AP-Review]

Deprecated — see below for the updated review.

Previous review (2026-09-25)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.
Verdict: COMMENT

Re-reviewed at head 929fc906f4. Full report: https://uav.tridgell.net/DevCallReviews/2026_09_26/devcall_pr_reviews.html#prSupportProxy-46

Verdict: COMMENT — no defect established in this repo's own code at this head. The 20-line delta is an upstream convention bump plus the matching test assertion, and no SupportProxy C++ changed: modules/mavlink moves 7108ebf → a755fae, which is itself only a pymavlink gitlink bump 450e1db → c29f315, and the substantive change is that a target above 255 now leaves the non-broadcast sentinel 255 in the legacy uint8 payload byte instead of 0. tests/test_sysid32.py:121 changes bytes([0, 250]) to bytes([255, 250]) to match. The pointer chain was verified live via git ls-remote and the pulls API, not assumed: a755fae is the head of the open ArduPilot/mavlink#515 and c29f315 the head of the open ArduPilot/pymavlink#1229.

Two notes

1. README.md:14 — the title, body and README still promise "header targets", which the new pin explicitly no longer does for small targets. The README says "Preserves 32-bit source system IDs and explicit header targets across forwarding, signing and telemetry logs", and the PR body opens "Re-finalizing forwarded frames discarded explicit header targets … Preserve targets". The pinned upstream commit says the opposite for the small case — "Small targets remain in the payload; no capability flag or targetless-message targeting is added" — and your own commit title already changed from "header targets" to "payload targets" without the PR title, body or README following. Confirmed in the pinned header: mavlink_helpers.h:326 clears msg->target_sysid and restores it only under if (target_sysid > 255) at :334. Confirmed again by a live probe through the running binary over UDP at this head: hdr=7/pay=0 → effective 0; hdr=7/pay=99 → 99; hdr=0/pay=99 → 99; hdr=256/pay=99 → 256 preserved.

Note what this closes: the previous round raised the behaviour as a finding, and you have now answered it on the merits by redesign — with mavlink_msg_target_field() a conforming sender can never emit TARGET32 with a target ≤ 255, so there is nothing to preserve. Only the wording mismatch survives. Suggest "targets above 255" (or "full-width targets") in README.md:14, the title and the body — and while editing, the description still links none of the four companion PRs.

2. scripts/run_tests.py:58 — this PR's only webadmin coverage did not run in CI at this head, for the second time. run() uses subprocess.check_call and the loop at :242-244 does not catch failures, so the first failing phase aborts the run. In the failing job the Robustness phase died on an unrelated video flake, and the log then contains only Connection, Authentication and Robustness — no === Running Webadmin Tests === line and zero hits for test_wide_sysid_forms. That file is the only coverage of the widened uint32 form validators (webadmin/forms.py:303, :357); run locally at this head it is 14 passed in 0.76 s, so the file is fine — it simply did not execute. Last round this looked resolved; it was only masked by a green run. Collecting phase results and failing at the end would fix it and also stop one flake hiding every later phase.

Still open from earlier rounds, restated so they aren't lost

Wide-ID tlogs are still silently truncated by the default pymavlink reader, and this belongs in pymavlink#1229 rather than here: at the new pin, mavutil.py has zero references to SYSID32/TARGET32 and :1545 still reads the msgid at a fixed offset with mlen += 12, adjusting only for the signing flag, so mavlink_connection() skips exactly the wide frames. Your own test documents this at tests/test_sysid32.py:169 and uses mavutil.mavlogfile instead. A legacy engineer GCS loses the proxy's own diagnostics after a wide heartbeat — mavlink.cpp:215 latches last_sysid = msg.sysid full-width and :479/:495 pack STATUSTEXT with it; your test at :159 asserts the warning carries 0xFEDCBA98 and it passes, which demonstrates it directly. Still the right trade — truncating would alias vehicles — but it is the series' interop seam. Seven test files are in no phase of the runner (run_tests.py:31-52 lists 20): test_binlog_capture, test_engineer_preauth_pool, test_engineer_udp_churn, test_kill_drop, test_log_cleanup, test_setup_signing_guard, test_tlog_capture. Rollback asymmetry — supportproxy.cpp:279 still passes k.fc_sysid untruncated into upsert_port(), so an older binary applying uint8_t() turns a configured 256 into 0 = "match any" at binlog.cpp:551; a release-note item. The redundant TARGET32 branch at mavlink.cpp:240-247 is byte-for-byte unchanged; I did not re-run last round's equivalence mutation this time, so that one is a restatement of the code state, not a re-proof.

What was checked and cleared

Sentinel audit — the one place this delta could have introduced a bug here. The payload target byte for a wide target is now 255, not 0, so anything reading a decoded payload target could misread it as a real system 255. There are exactly three _decode() call sites in the tree: binlog.cpp:178 (remote_log_data_block, reads only seqno/data), binlog.cpp:555 (system_time, reads time_boot_ms) and mavlink.cpp:527 (setup_signing, reads secret_key/initial_timestamp). A tree-wide search for .target_system / ->target_system / get_target_sys across all *.cpp/*.h finds no read of a decoded payload target anywhere, and binlog.cpp:291-296 explicitly selects the source header identity instead. Both reviewers ran that search independently. The relay never rewrites the payload, and outgoing packs go through the generated _pack_chan with the _target suffix, so the full target rides the header.

Mutations actually run (each rebuilt in the foreground with the binary's mtime checked first, because a failed build leaves the old binary in place). Most usefully: installing the old pinned pymavlink (450e1db, which packs 0) as the sender against the unmodified new binary gives 6 failed / 3 passed, failing exactly at tests/test_sysid32.py:121 with assert bytearray(b'\x00\xfa') == b'\xff\xfa' — so the one changed assertion is non-vacuous, and line 120 passed throughout, meaning it catches something the payload compare cannot. Also: mavlink.cpp:242,246 msg.len → max_len fails 6 (the 255th extension byte really is lost without it); dropping the TARGET32 branch entirely fails 6 at :110; binlog.h:233 target_system uint32_t→uint8_t fails 3 at :191; binlog.h:305 fc_sysid_filter_ uint32_t→uint8_t fails 3 at :201. Tree restored and rebuilt clean afterwards. Built at this head with the project's own -O2 -Werror -Wextra set, with libtdb unpacked into scratch (no sudo). Baseline 39 passed on test_sysid32 + test_keydb_log, 14 on the webadmin form tests.

Routing and isolation: no sysid-keyed forwarding, no (sysid << 8) | compid composite key; user traffic fans out through conn2[], engineer traffic returns through mav1, and reboot filtering at binlog.cpp:551 compares full-width identities. Widening the sysid cannot reach tenant isolation. The frame-size arithmetic (287 signed / 274 unsigned, tests/test_sysid32.py:130) still holds at the new pin, and the earlier MAVLINK_CFLAG_SYSID32 advertise-on-behalf issue remains fixed.

What the suite does not catch, plainly: there is still no test for a frame whose header and payload targets disagree, so the small-header-target normalisation is unpinned in either direction — those frames had to be hand-built for the probe above. A raw-write case is about six lines.

CI

docker-test passes, so the submodule pointer is fetchable and this is not a push-ordering race. test fails on tests/test_video_rtsp.py::TestRtspIngest::test_no_orphan_backend_after_the_publisher_leaves ("backend outlived the publisher"), totals 1 failed / 254 passed / 1 skipped. Not this PR's fault: it touches no video file, all 9 test_sysid32 items passed in that same run, and this file's orphan-backend checks have flaked on unrelated branches and on this PR's own earlier head. The real cost of that failure is note 2 above — it aborted the Webadmin phase.

[AP-Review]

Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting.
Verdict: ACCEPT

Re-reviewed at head 8503e6addc (you were last told 929fc906f4); my earlier comment above is superseded. Full report: https://firmware.ardupilot.org/Tools/APReview/DevCallReviews/followups/2026_09_28_1802/devcall_pr_reviews.html#prSupportProxy-46

My previous comment had 8 items. 6 are resolved; the other 2 were optional and still stand (a simplification and a test gap). Nothing here blocks.

#	Item	Status
1	README/title/body promised "header targets"	Resolved. README.md:14 and the title now say "source and destination system IDs". The body explains that 8-bit targets stay in the payload.
2	First failing phase aborted the run, so Webadmin never ran	Resolved. scripts/run_tests.py:253-264 runs every phase and returns 1. Verified 3 ways: a local end-to-end run with failing phases still ran all four and exited 1; reverting the try/except makes the new tests/test_run_tests.py fail 2/4; the CI log now shows the Webadmin phase ran (305 passed).
3	Default pymavlink reader dropped wide-ID tlog frames	Resolved upstream by the pin. tests/test_sysid32.py:169 now uses mavlink_connection(). With the old pin's mavutil.py swapped in, 6/9 fail at :176, missing exactly the wide frames.
4	Legacy GCS loses proxy diagnostics after a wide heartbeat	Resolved (documented at README.md:23-25)
5	Seven test files were in no runner phase	Resolved. Every tests/test_*.py is now in a phase except test_config.py, which has no tests.
6	Rollback: older binary turns fc_sysid 256 into 0 = match-any	Resolved (documented at README.md:25-27)
7	"Redundant" TARGET32 branch, mavlink.cpp:240-247	Still stands, optional simplification — not a defect. The branch could be replaced by an unconditional mavlink_finalize_message_buffer_target(..., msg.target_sysid): the parser zeroes target_sysid whenever TARGET32 is absent (mavlink_helpers.h:766,785), and the plain finalizer is just the target variant with 0 (:367-370). (Replacing it with the plain finalizer is a different change and does break test_sysid32, which a first pass at this re-review mistook for a refutation.) Keeping the explicit branch is equally fine.
8	No test where the header and payload targets disagree	Still open, optional. test_sysid32.py still has no hand-built mismatched frame.

Submodule: modules/mavlink → a23652e is exactly current ArduPilot/mavlink master (the merged mavlink#515). Compared with the old pin, the only change is pymavlink c29f315 → 19880422 (merged pymavlink#1229). Message definitions are identical. On the C side, the bump only reorders one #define. The real change is the mavutil mmap indexer fix behind item 3.

Test harness: the group kill via start_new_session + killpg left no supportproxy processes after a full -j 16 run. _terminate() is only called from cleanup paths, so it doesn't mask any asserted parent behaviour. Two optional notes:

• tests/test_video_rtsp.py:377-380: the publisher-leaves check now watches only the PIDs captured while publishing. It does catch a real orphan (I made stop() skip the kill and the test fails), but a respawned backend with a new PID would slip through. Adding assert not _ffmpeg_children(s.proxy) would cover that and is already per-proxy.
• tests/test_video_rtsp.py:384-394: test_backend_dies_with_the_proxy stops the publisher first, so ffmpeg exits on its own. With both the stop() kill and PR_SET_PDEATHSIG removed it still passes. This was already the case before this PR; mentioning it for your information.

CI is green (2/2) and all four phases ran.