Allow receiving data from other vehicles on the network - #2938
Allow receiving data from other vehicles on the network#2938rafaellehmkuhl wants to merge 8 commits into
Conversation
rafaellehmkuhl
changed the title
Automated PR Review — round 1Warning Adds a settings panel where the user lists extra vehicles by websocket address. Cockpit opens a read-only link to each at startup and copies everything those links carry into the shared pool of telemetry values that widgets and map markers read from. The links never join the connection manager, so none of those vehicles can be commanded or become the one being piloted — that part holds up. What does not hold up is the naming of the copied values: they are filed under the vehicle's own system number, which by factory default is the same number the piloted vehicle uses, so the two overwrite each other. What still needs attention
Change map — what was established before judgingClaims (from the PR body — recorded, then checked against the code)
Failure site — n/a, this PR fixes no bug. Entry points
No changed or added function traced to Invariants
1. Correctness & Implementation Bugs — 3 findings (2 major, 1 minor)1.1 — Secondary telemetry is filed under the same ids as the piloted vehicle, and the mitigation is a line of text in a settings panel
|
| Key | Backend | What happened |
|---|---|---|
cockpit-secondary-mavlink2rest-uris |
machine-local — useStorage → localStorage (src/composables/secondaryVehicles.ts:21; key declared at src/libs/vehicle/mavlink/secondary-connections.ts:202) |
added: string[] of normalized ws:///wss:// addresses |
Nothing else is added, reshaped or removed, and the PR contains no migration.
Judgement
- Backend is the right one, and for the reason AGENTS.md gives rather than by accident. These are addresses on the topside computer's own network, and
main.ts:105auto-connects to every stored entry at boot — an automatic action on a synced value is exactly what the rule forbids, so machine-local is required here, not merely preferred.useStoragefor machine-localcockpit-*keys matches existing practice (cockpit-vehicle-address,src/stores/mainVehicle.ts:85;cockpit-ignored-update-versions,src/components/UpdateNotification.vue:53). - Key starts with
cockpit-as AGENTS.md requires. - Shape is sound: a flat array of the normalized URI, no
idfield duplicating the key, no nested state that could go stale.secondaryVehicleUrisre-reads it defensively withArray.isArray(secondaryVehicles.ts:24), so a hand-corrupted value degrades to an empty list rather than throwing at boot. - No default or behaviour change strands existing users: the key is new and empty, so nothing runs for anyone who does not opt in.
- Worth recording, not a finding: the sibling feature in the same panel persists its list through the vehicle-synced
settingsManagerinstead (cockpit-generic-websocket-connections,src/libs/generic-websocket.ts:32,70,93). This PR's choice is the safer of the two, but it is one more axis on which the two near-identical panels diverge — see 7.1. - Data-lake variables created by a removed link are deliberately left behind (
secondary-connections.ts:296-298); those live in memory only, so nothing is orphaned on disk.
5. Performance — 1 finding (minor)
5.1 — Secondary sockets take the built-in 4 s watchdog, not the one the same settings page exposes, and the status window is 5 s minor
Consequence: a mistyped address reconnects forever in the background, and the status light never settles on anything the user can act on.
secondary-connections.ts:322 constructs the connection with no options:
const connection = new WebSocketConnection(parsedUri, Protocol.Type.MAVLink)so _getWatchdogTimeoutMs falls back to DEFAULT_WATCHDOG_TIMEOUT_MS = 4_000 (src/libs/connection/websocket-connection.ts:18,67). Every other WebSocketConnection in the tree passes the user's value — ConfigurationGeneralView.vue:679 and src/stores/mainVehicle.ts:617 both pass getWatchdogTimeoutMs: () => vehicleConnectionWatchdogTimeoutMs. That value is edited by the "Watchdog timeout" field on this very page (ConfigurationGeneralView.vue:487-495), so a user who raises it for a slow link will find it silently ignored by the panel two sections above.
The two timeouts also disagree with each other. receivingTimeoutMs = 5000 (secondary-connections.ts:208) defines the 'connecting' → 'connected' boundary in secondaryConnectionStatus, but _runWatchdog tears the socket down and reopens it after 4 s of silence (websocket-connection.ts:324-334). An open-but-silent link is therefore always recycled before the 5 s state can be reached, so an address pointing at a non-MAVLink endpoint — or a right host with the wrong path — produces an endless 4-second reconnect cycle with a console.warn per iteration, while the row flickers between disconnected and connected, waiting for data instead of settling on something the user can act on.
Pass { getWatchdogTimeoutMs } like the main connection does, and derive receivingTimeoutMs from the same source (or set it above the watchdog) so the two cannot contradict each other.
Checked and clean elsewhere in this section: useIntervalFn self-disposes via the component scope, so the 1 Hz poll stops on unmount; syncSecondaryVehicleConnections calls disconnect() and clears all three module maps on removal (secondary-connections.ts:305-312), and WebSocketConnection.disconnect() stops both the watchdog interval and the pending reconnect timer; the per-message injection cost carries a ponytail: comment naming its ceiling and upgrade path (secondary-connections.ts:252-253), which is the marking AGENTS.md asks for — the part of that cost the comment does not cover is filed as 1.2.
6. UI / UX — 1 finding (minor)
6.1 — The "Other vehicles" panel gives the user nothing to type and nothing to read minor
Consequence: the panel's only input is an unlabeled empty box, and the example address the code prepares is never shown anywhere.
Two breaches on one surface, grouped:
- The example address is bound to a slot that is switched off. The added
v-text-fieldcarries:hint="exampleSecondaryVehicleUri"andhide-details, so the hint area never renders andexampleSecondaryVehicleUri = 'ws://192.168.2.4/mavlink2rest/ws/mavlink'is dead. There is nolabeland noplaceholdereither, and the panel's#infoblock only showsexampleSecondaryVehicleCoordinate— somavlink2rest/ws/mavlink, the one part a user cannot guess, appears nowhere in the UI. The sibling panel survives the samehint+hide-detailscombination only because it seeds the model with the example (newGenericWebSocketUrl = ref(exampleGenericWebSocketUrl),ConfigurationGeneralView.vue:968), which this one does not (ref('')). Drophide-details, or useplaceholder, or add the address example beside the coordinate example in#info. - The remove button has no accessible name.
<v-btn icon="mdi-close" size="x-small" variant="text" @click="removeSecondaryVehicle(row.uri)" />carries neitherv-tooltipnoraria-label, so a screen reader announces nothing for the control that deletes a configured entry. The same view labels its other icon controls (v-tooltip.bottom="'Reset to default'",ConfigurationGeneralView.vue:461,498). Gradedminorbecause the glyph and the adjacent address convey the action to a sighted user.
Checked and clean on this surface: logUserAction fires on both add and remove in the owning handlers, in the past tense naming the target (secondaryVehicles.ts:81,97); both paths give visible feedback via openSnackbar, with no paired console.* of the same message; the panel is an ExpansiblePanel and so satisfies the space-economy rule; titles and labels are sentence case; the divider chain (no-top-divider) matches the panels on either side; the duplicate-ID warning avoids MAVLink jargon and names the concrete parameter the user must change.
7. Code Quality & Style — 1 finding (minor)
7.1 — The new panel duplicates the Generic WebSocket panel that sits ~100 lines below it in the same file minor
Consequence: the same list-of-connections screen now exists twice on one settings page, so every future fix has to be made twice.
The added markup is a near-verbatim copy of ConfigurationGeneralView.vue:376-415, class strings included: the same flex items-center justify-between py-2 px-3 mb-2 rounded bg-[#FFFFFF11] row, the same getLoadingStatusColor / getLoadingStatusIcon pair, the same truncated-address span, the same (status) span, the same v-btn icon="mdi-close" size="x-small" variant="text", the same empty-state line, and the same v-text-field + Add … button footer down to :class="interfaceStore.isOnSmallScreen ? 'ml-1' : 'ml-5'". That is how the hide-details defect in 6.1 was inherited.
The plumbing then diverges on all three axes, so the page now carries two of everything:
Generic WebSocket (src/libs/generic-websocket.ts) |
Secondary vehicles (this PR) | |
|---|---|---|
| Persistence | settingsManager (vehicle-synced), lines 32, 70, 93 |
useStorage (machine-local), secondaryVehicles.ts:21 |
| Status delivery | push listener, listenToGenericWebSocketConnections:292 |
1 Hz poll, useIntervalFn(refreshSecondaryVehicleStates, 1000) |
| Reconnect | hand-rolled scheduleReconnect, line 176 |
WebSocketConnection's watchdog + backoff |
AGENTS.md, "Reuse before reinventing": "If the same logic would live in two or more places, extract it once and reuse it." The two-copy threshold is now met. The smallest move is to lift the row list + add-row into one small child component under src/components/configuration/ and have both panels render it with their own callbacks; the persistence and status mechanisms can stay different if the divergence is deliberate, but the near-identical 40 lines of template should not.
Nothing else in this section produced a finding: ConfigurationGeneralView.vue grows 1017 → 1127 lines, well under the ~2000 threshold, and the domain logic correctly went to src/libs/ with the reactive orchestration in src/composables/ rather than into the <script setup>; no scoped CSS was added; no comment was deleted or reworded over unchanged code; the extraction into data-lake-injection.ts genuinely deletes 62 lines and collapses four copies of create-then-set into one helper.
8. Commit Hygiene — 1 finding (minor)
8.1 — The feature commit is ~377 added lines across four files minor
Consequence: reviewers and anyone bisecting later have to take the transport, the boot wiring and the UI as a single unit.
2bfaa0d vehicle: receive telemetry from other vehicles on the network bundles three steps that are each independently reviewable and revertable:
- the transport module,
src/libs/vehicle/mavlink/secondary-connections.ts(+151), - the composable and the boot wiring,
src/composables/secondaryVehicles.ts(+109) andsrc/main.ts(+4), - the settings panel,
src/views/ConfigurationGeneralView.vue(+110).
AGENTS.md: "Keep a commit reviewable in one sitting. Several hundred lines in a single commit is hard to follow even when it is nominally one thing, so look for the atomic steps inside it."
The rest of this section is clean: two commits, no wip / fixup! / address review noise, no commit reverting or reimplementing another on the branch, no commits replicated from a sibling PR, and no GitHub issue or PR reference in either message. Both prefixes describe their change and match the git log style on master (refactor: vehicle: and vehicle: alongside map:, widgets:, ci:). 0dc37cc correctly isolates the refactor from the feature ahead of it — the one behavioural slip it carries is filed as 1.3.
Sections with nothing to report (5)
3. AGENTS.md Adherence — ✅ (no dependency added — @vueuse/core 9.8.1 and vue already at package.json:49,92, so ordering is untouched; every exported symbol in the three new/changed modules traced to a call site inside this PR, so no groundwork; JSDoc present and non-empty on all six exported functions plus all three SecondaryConnectionState property signatures, satisfying the jsdoc/require-jsdoc contexts in .eslintrc.cjs:39; the per-message throughput corner-cut carries a ponytail: comment naming both the ceiling and the upgrade path at secondary-connections.ts:252; vehicle.ts shows no rename, import reorder or formatter reflow outside the extraction)
4. Security — ✅ (scanned pr.diff for zero-width, bidi-override and homoglyph codepoints — none; no encoded blob, eval, Function() or v-html; no new dependency, no env var, token or credential; no change under scripts/, .github/ or src/electron/, and no build, postinstall or workflow file touched; the only network activity is a WebSocket to an address the user types into the panel, and the diff contains nothing addressed to an automated reviewer)
9. Tests — ✅ (no file under tests/ or *.spec.* appears in the six changed paths, so nothing was removed, skipped or weakened; the extraction of injectMavlinkPackageIntoDataLake out of a private class method into a free function in a Vue-free module makes that logic testable where it previously was not)
10. Documentation — ✅ (no README.md Lite/Standalone table entry is owed: WebSocket, useStorage and the data lake behave identically in both builds and the diff touches no window.electronAPI, electron-* module or other Electron-only API, so the isElectron() guard rule does not apply; the module header at secondary-connections.ts:180-190 documents the read-only and ConnectionManager-bypass decisions where they are made)
11. Nitpicks / Optional — ✅ (reviewed the added code for naming, ordering and small refactors; simple-import-sort ordering holds in all four import blocks, func-style and explicit-function-return-type are satisfied on every added arrow function, and nothing remained that was worth raising separately from findings 1.3, 5.1, 6.1 and 7.1)
Generated by Claude. This is advisory; a human reviewer must still approve.
2bfaa0d to
060b76f
Compare
Review follow-up — round 1Done
Done differently
Panel copy and the PR body were updated to describe the new behaviour, since "they overwrite each other" is no longer what happens. |
|
/review |
Automated PR Review — round 2Warning Adds a settings panel where the user lists extra vehicles by websocket address. Cockpit opens a read-only link to each at startup and copies their telemetry into the shared pool of values that widgets and map markers read from. This round closes the naming problem: a link now only mirrors a vehicle that announced itself on that link, only the first link to claim a given vehicle number gets to write it, and anything using the piloted vehicle's number is thrown away — so another vehicle can no longer overwrite the pilot's own readouts once the piloted vehicle is known. The panel's screen was also factored out and is now shared with the near-identical panel below it. What is left is a block of placeholder characters committed inside a documentation comment in the new test file, which the project's own rules forbid and the lint step is set up to reject. What still needs attention
Since round 1 — 7 closed, 3 new, comparing 2bfaa0d → 060b76fRange —
Findings that changed status
New this round — 3.1 ( Discussion since round 1
Change map — what was established before judgingClaims (from the PR body — recorded, then checked against the code)
Failure site — n/a. The PR fixes no reported bug; the only defect it repairs is one this review raised at round 1, and it is repaired at the site the round-1 finding named ( Entry points
No changed or added function traced to Invariants
1. Correctness & Implementation Bugs — 1 finding (minor)1.4 — The piloted-vehicle guard reads a value that does not exist until the piloted vehicle connects
|
| Key | Backend | What happened |
|---|---|---|
cockpit-secondary-mavlink2rest-uris |
machine-local — useStorage → localStorage (src/composables/secondaryVehicles.ts:16; key declared at src/libs/vehicle/mavlink/secondary-connections.ts:24) |
added: string[] of normalized ws:///wss:// addresses |
Nothing else is added, reshaped or removed, and the PR contains no migration. Unchanged since round 1 — the round-2 push touched no persistence.
Judgement
- Backend is the right one, and for the reason AGENTS.md gives rather than by accident. These are addresses on the topside computer's own network, and
main.ts:105auto-connects to every stored entry at boot — an automatic action on a synced value is exactly what the rule forbids, so machine-local is required here, not merely preferred.useStoragefor machine-localcockpit-*keys matches existing practice (cockpit-vehicle-address,src/stores/mainVehicle.ts:85;cockpit-ignored-update-versions,src/components/UpdateNotification.vue:53). - Key starts with
cockpit-as AGENTS.md requires. - Shape is sound: a flat array of the normalized URI, no
idfield duplicating the key, no nested state that could go stale.secondaryVehicleUrisre-reads it defensively withArray.isArray(secondaryVehicles.ts:19), so a hand-corrupted value degrades to an empty list rather than throwing at boot. Noundefinedis ever written to it — removal writes a filtered array (:91). - No default or behaviour change strands existing users: the key is new and empty, so nothing runs for anyone who does not opt in.
- Worth recording, not a finding: the sibling feature in the same panel persists its list through the vehicle-synced
settingsManager(cockpit-generic-websocket-connections,src/libs/generic-websocket.ts:32,70,93). The two backends still differ, which is deliberate and is now the only axis on which the two panels diverge in the UI layer, since they shareConnectionsList. - Data-lake variables created by a removed link are deliberately left behind (
secondary-connections.ts:136-138); those live in memory only, so nothing is orphaned on disk.
3. AGENTS.md Adherence — 1 finding (major)
3.1 — Two placeholder JSDoc blocks committed in the new test file major
Consequence: a new test file carries a comment filled with repeated letters and another that is empty, which the project forbids outright and which the repo's own lint gate is configured to reject.
src/tests/libs/vehicle/mavlink/data-lake-injection.test.ts contains, at lines 30-33, a documentation block whose two content lines are a run of 44 c characters each (the second of them ending in a stray *), wrapped around the variables property of an inline cast:
const variables = (
dataLake as unknown as {
/**
cccc… *
cccc…
*/
variables: Map<string, string | number>
}
).variablesand at lines 18-20 a second block with no summary at all:
getDataLakeVariableInfo: (id: string): { /** * */ id: string } | undefined => …AGENTS.md, "Keep JSDocs updated": "Never write a JSDoc whose summary line is empty, whitespace-only, or filler (placeholder characters, repeated letters, lorem-ipsum). If you have nothing useful to say, omit the block entirely instead of leaving it blank." Repeated letters and an empty summary are the two named cases.
Both blocks exist only to satisfy jsdoc/require-jsdoc, which .eslintrc.cjs:39 extends to TSPropertySignature — so the inline anonymous object types are what force them. Two consequences beyond the rule itself:
plugin:jsdoc/recommended(.eslintrc.cjs:8) enablesjsdoc/check-alignmentandjsdoc/no-multi-asterisks. Lines beginning withcccc…rather than an aligned*, and one ending in*, are what those two rules exist to catch, andyarn lintruns with--max-warnings=0(package.json), so this very likely fails the lint gate outright rather than merely breaching the convention.- AGENTS.md requires
yarn lint:fixbefore finishing, and neither of these is auto-fixable, so both would have surfaced.
The fix removes the need for the blocks rather than filling them in: hoist the map with vi.hoisted and reference it directly instead of casting the mocked module to an inline object type, and drop the explicit return type on the mocked getDataLakeVariableInfo — @typescript-eslint/explicit-function-return-type is configured with allowExpressions: true (.eslintrc.cjs:84), so a function expression does not need one. With no inline TSPropertySignature left, jsdoc/require-jsdoc has nothing to demand.
Nothing else in this section produced a finding — see the clean-sections note for what was checked.
6. UI / UX — 1 finding (minor)
6.2 — The duplicate-ID warning states a rule that does not apply to the collision it most often reports minor
Consequence: when another vehicle uses the piloted vehicle's number the panel tells the user the first one to speak up wins, when in fact that vehicle is never received at all.
ConfigurationGeneralView.vue:281-283 renders one sentence for every duplicate:
More than one vehicle is using system ID {{ … }}, so only the first one to announce it is being received. Give each vehicle a different system ID.
But duplicatedSecondaryVehicleSystemIds (src/composables/secondaryVehicles.ts:25-30) deliberately folds two different collisions into one list — it pushes the main vehicle's ID into the set before looking for repeats (:28). For the main-vehicle collision the code does not apply "first to announce": secondary-connections.ts:82 drops the package unconditionally, on every message, whoever claimed the ID first. A user whose secondary vehicle announced before the piloted one connected reads that they are the first, and therefore that their vehicle is the one being received, while the opposite is true.
The panel's #info paragraph does state both rules correctly (:263-264, "otherwise only the first one to announce each ID is received, and one using the piloted vehicle's ID is ignored entirely"), which makes the warning the odd one out — and the warning is the line the user actually sees when something is wrong, since #info is behind the panel's info toggle.
Split the sentence on the case, e.g. keep the current wording for links colliding with each other and emit "A vehicle is using the same system ID as the vehicle you are piloting, so it is not being received. Change its system ID." when the duplicate is the main vehicle's ID — the computed already knows which one that is at :26.
Checked and clean on this surface: the extracted ConnectionsList gives both panels a labelled field, a placeholder carrying the example address, and a remove button with a tooltip and an aria-label (ConnectionsList.vue:18-19,35-36); logUserAction fires on add and remove in the owning handlers, in the past tense naming the target (secondaryVehicles.ts:69,91); both paths give visible feedback via openSnackbar with no paired console.* of the same message; the panel is an ExpansiblePanel with no-top-divider, matching the divider chain of the panels on either side; labels and titles are sentence case ("Other vehicles (telemetry only)", "Add vehicle", "Vehicle telemetry address"); the add button is a text button sitting beside the field it acts on rather than in a row of its own; no overlay-teleporting Vuetify control was added, so no theme="dark" is owed; no z-index, no hand-written backdropFilter, no nested glass layer; and the user-facing copy names SYSID_THISMAV — an autopilot parameter the user must change — rather than protocol jargon for its own sake.
Sections with nothing to report (7)
4. Security — ✅ (pr.diff contains no non-ASCII byte at all, so no zero-width, bidi-override or homoglyph codepoint; no encoded blob, eval, Function() or v-html; no new dependency, env var, token or credential; nothing under scripts/, .github/, src/electron/, no build, postinstall or workflow file touched; the only network activity is a WebSocket to an address the user types into the panel)
5. Performance — ✅ (the per-message path gained two Map lookups and one getDataLakeVariableData, which is a plain object index at data-lake.ts:116-118, and the two gates now reject most traffic before injectMavlinkPackageIntoDataLake runs, so the hot path is cheaper than at round 1; useIntervalFn self-disposes with the component scope; syncSecondaryVehicleConnections:145-153 disconnects and clears all four module maps on removal, and WebSocketConnection.disconnect() stops both the watchdog interval and the pending reconnect timer; the residual throughput ceiling carries its ponytail: comment at secondary-connections.ts:84)
7. Code Quality & Style — ✅ (ConfigurationGeneralView.vue goes 1017 → ~1079 lines, far short of the ~2000 threshold, and the round-2 push is net-negative on it — 44 lines of duplicated template out, the shared component in; domain logic sits in src/libs/, reactive orchestration in src/composables/, presentation in src/components/configuration/; no scoped CSS added; no comment deleted or reworded over unchanged code; simple-import-sort ordering holds in all five import blocks and the two dropped getLoadingStatus* imports have no remaining call site in that file; every added arrow function satisfies func-style and explicit-function-return-type; no stray any outside the two deliberate as unknown as casts in the test file)
8. Commit Hygiene — ✅ (five commits, largest +172; each prefix — refactor: vehicle:, refactor: configuration:, vehicle:, configuration: — describes its own change and matches the scope-prefixed style on master per git log; no wip/fixup!/address review noise, no commit reverting or reimplementing another on the branch, no commits replicated from a sibling PR, and no #N or closing keyword in any message, the issue reference living in the PR body where AGENTS.md wants it; the two behaviour changes folded into 0883f7c — the placeholder and the accessible name — are inherent to the extraction that commit performs and fall under the refactor exception)
9. Tests — ✅ (no existing test was removed, skipped or weakened; the one added file is new, and its three cases pin the extraction's contract including the exact undefined system-id regression this review raised at round 1 — the mock's shared module-level map is not reset between cases but no case depends on another's writes, so ordering does not make them brittle; the placeholder comments in that file are filed as 3.1, not as a test defect)
10. Documentation — ✅ (no README.md Lite/Standalone entry is owed: WebSocket, useStorage and the data lake behave identically in both builds and the diff touches no window.electronAPI, electron-* module or other Electron-only API, so the isElectron() guard rule does not apply; the module header at secondary-connections.ts:1-11 documents the read-only and ConnectionManager-bypass decisions where they are made, and the ownership rule is explained at :51-52 and :77-79)
11. Nitpicks / Optional — ✅ (reviewed the added code for naming, ordering and small refactors; the only candidates were the ConnectionRow interface being unexported so neither parent can type its computed against it, and the generic panel still seeding its field with the example address so the new placeholder never shows there — neither is worth a finding of its own)
Generated by Claude. This is advisory; a human reviewer must still approve.
060b76f to
42578d2
Compare
Review follow-up — round 2Done
Done differently
|
|
/review |
42578d2 to
ae7ca9f
Compare
Automated PR Review — round 3Tip ✅ READY TO MERGE — nothing open; 10 closed, 3 of them this round. Adds a settings panel where the user lists extra vehicles by websocket address. Cockpit opens a read-only link to each at startup and copies their telemetry into the shared pool of values that widgets and map markers read from. This round removes the placeholder text from the new test file by deleting the inline types that forced it, splits the panel's duplicate-ID warning so the message matches what actually happens to each kind of collision, and marks the one remaining startup window with the project's deliberate-corner-cut marker naming both the ceiling and the way out. What still needs attentionNothing is open. Every finding raised across the three rounds is closed by a code change; none was closed by argument or by maintainer decision. Since round 2 — 3 closed, 0 new, comparing 060b76f → 42578d2Range —
Findings that changed status
New this round — none. Sections 0 through 11 were re-run over the whole of Discussion since round 2
Change map — what was established before judgingClaims (from the PR body — recorded, then checked against the code)
Failure site — n/a. The PR fixes no reported bug. The only defect it repairs is the Entry points
No changed or added function traced to Invariants
2. Persistence & User Data — inventory, no findingsInventory
Nothing else is added, reshaped or removed, and the PR contains no migration. Unchanged since round 1 — neither the round-2 nor the round-3 push touched persistence. Judgement
Sections with nothing to report (10)1. Correctness & Implementation Bugs — ✅ (the extraction was diffed line for line against the 65 deleted lines of 3. AGENTS.md Adherence — ✅ (every added 4. Security — ✅ ( 5. Performance — ✅ (the per-message path costs one 6. UI / UX — ✅ (both warning sentences now match the code path they report — 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ (five commits — 9. Tests — ✅ (no existing test was removed, skipped or weakened; the one added file is new and its three cases still pin the extraction's contract, including the 10. Documentation — ✅ (no 11. Nitpicks / Optional — ✅ (reviewed the added code for naming, ordering and small refactors; the only candidates were Generated by Claude. This is advisory; a human reviewer must still approve. |
ae7ca9f to
9ebf2f1
Compare
|
Pushed a per-vehicle connection status, squashed into the mirroring commit. Each mirrored vehicle now gets How it works, in
The silence decision is now the pure It is deliberately a boolean, not the panel's three states: "disconnected" is a property of a link, not of a vehicle — a vehicle with no link is simply not being received. Not under |
|
/review |
Automated PR Review — round 4Note 📝 MINOR SUGGESTIONS — 4 open (3 minor, 1 nit); 10 closed, none of them this round. Adds a settings panel where the user lists extra vehicles by websocket address. Cockpit opens a read-only link to each at startup and copies their telemetry into the shared pool of values that widgets and map markers read from. This round adds a per-vehicle "is data arriving" flag to that pool, kept current by a once-a-second background check that only runs while at least one extra vehicle is configured, so a widget can tell a live reading from the last one received before the vehicle went quiet. The round also rebased the branch and replaced the existing test file with a smaller one. What still needs attention
Since round 3 — 0 status changes, 4 new, comparing 42578d2 → 9ebf2f1Range —
Findings that changed status — none. All ten findings from rounds 1 to 3 were already
New this round — four. Sections 0 through 11 were re-run over the whole of Discussion since round 3
Change map — what was established before judgingClaims (from the pull-request body and the author's round-3 follow-up — recorded, then checked against the code)
Failure site — n/a. The pull request fixes no reported bug. The only defect it repairs is the Entry points
No changed or added function traced to Invariants
1. Correctness & Implementation Bugs — 1 finding1.5 — Removing a link writes an
systemIdOwners.forEach((owner, systemId) => {
if (owner !== uri) return
systemIdOwners.delete(systemId)
setIsReceivingDataVariable(systemId, false)
lastMessageAtBySystemId.delete(systemId)
})Ownership is claimed on the heartbeat at The consequence is concrete because the colliding ID is, by construction, the piloted vehicle's — commonly This also breaks the invariant the code states one line above the tracking, at
systemIdOwners.delete(systemId)
if (lastMessageAtBySystemId.delete(systemId)) setIsReceivingDataVariable(systemId, false)That keeps the documented behaviour for every mirrored vehicle and drops the write for the ones that never were. 2. Persistence & User Data — inventory, no findingsInventory
Nothing else is added, reshaped or removed, and the pull request contains no migration. Unchanged since round 1 — none of the three pushes since has touched persistence. Judgement
8. Commit Hygiene — 1 finding8.2 — The mirroring commit now carries the per-vehicle status feature, and its message does not say so ( Read from the The problem is AGENTS.md asks that each commit be one logical change and that the subject describe the change it carries. A derived Cockpit-owned variable driven by a background timer is not the same logical change as mirroring MAVLink fields — it has its own namespace, its own lifecycle and its own test, and it is independently revertable. The practical cost is that the log gives no notice of an always-on 1 Hz timer: anyone reviewing commit by commit, or bisecting later, reads a commit about mirroring and does not expect one. Either split it into its own commit after the mirroring one, or, if it genuinely belongs there, extend 9. Tests — 1 finding9.1 — The test that pinned the round-1 regression was deleted in this round's rebase (
What went with it was the only guard on the defect this review found at round 1. Finding 1.3 was that The replacement is not equivalent in weight: This is not a request for new tests — it is the pull request removing a test it had. Restore 11. Nitpicks / Optional — 1 finding11.1 — "nothing polls in the background" is no longer true of this module (
Ninety lines further down, Narrowing it to the function is enough: "Pull-based: this state is not pushed, the UI reads it while it is on screen." The background-poll story is already told where it belongs, at Sections with nothing to report (6)3. AGENTS.md Adherence — ✅ (the round's added code declares no new interface or property signature, so 4. Security — ✅ ( 5. Performance — ✅ (the per-message path gains one 6. UI / UX — ✅ (no template, dialog, control or copy changed this round — 7. Code Quality & Style — ✅ ( 10. Documentation — ✅ (no Generated by Claude. This is advisory; a human reviewer must still approve. |
9ebf2f1 to
6328c96
Compare
Review follow-up — round 4Not from the review — direct author request. Done
The four open findings from round 3 (1.5, 8.2, 9.1, 11.1) are untouched here and still stand. |
|
/review |
Automated PR Review — round 5Note 📝 MINOR SUGGESTIONS — 4 open (3 minor, 1 nit); 10 closed, none of them this round. Adds a settings panel where the user lists extra vehicles by websocket address. Cockpit opens a read-only link to each at startup and copies their telemetry into the shared pool of values that widgets and map markers read from. Each extra vehicle also gets an "is data arriving" flag in that pool, kept current by a once-a-second background check that only runs while at least one extra vehicle is configured, so a widget can tell a live reading from the last one received before the vehicle went quiet. This round renamed that flag and rewrote three commit messages; it changed no behaviour. What still needs attention
Since round 4 — 0 status changes, 0 new findings, comparing 9ebf2f1 → 6328c96Range —
What actually moved this round. One line of code and three commit messages:
Findings that changed status — none. All four open findings were re-checked against the current head, and all four are
New this round — none. Sections 0 through 11 were re-run over the whole of The rename itself was checked rather than waved through. Discussion since round 4
Change map — what was established before judgingClaims (from the pull-request body and the author's round-4 follow-up — recorded, then checked against the code)
Failure site — n/a. The pull request fixes no reported bug. The only defect it repairs is the Entry points
No changed or added function traced to Invariants
1. Correctness & Implementation Bugs — 1 finding1.5 — Removing a link writes an
systemIdOwners.forEach((owner, systemId) => {
if (owner !== uri) return
systemIdOwners.delete(systemId)
setIsReceivingDataVariable(systemId, false)
lastMessageAtBySystemId.delete(systemId)
})Ownership is claimed on the heartbeat at The consequence is concrete because the colliding ID is, by construction, the piloted vehicle's — commonly This also breaks the invariant the code states one line above the tracking, at
systemIdOwners.delete(systemId)
if (lastMessageAtBySystemId.delete(systemId)) setIsReceivingDataVariable(systemId, false)That keeps the documented behaviour for every mirrored vehicle and drops the write for the ones that never were. 2. Persistence & User Data — inventory, no findingsInventory
Nothing else is added, reshaped or removed, and the pull request contains no migration. Unchanged since round 1 — none of the four pushes since has touched persistence. Judgement
8. Commit Hygiene — 1 finding8.2 — No commit message mentions the per-vehicle status feature the branch carries ( Read from the What is still missing is any mention of the second logical change the author added at round 4 and squashed in: the
None of the three names the status variable or the background timer, and no sixth commit was added to hold them. The rewrite is why this finding is retitled: at round 4 the feature was demonstrably in AGENTS.md asks that each commit be one logical change and that the subject describe the change it carries. A derived Cockpit-owned variable driven by a background timer is not the same logical change as mirroring MAVLink fields — it has its own namespace, its own lifecycle and its own test, and it is independently revertable. The practical cost is that the log gives no notice of an always-on 1 Hz timer: anyone reviewing commit by commit, or bisecting later, gets no signal that one was introduced. Either split it into its own commit after the mirroring one, or, if it genuinely belongs where it is, extend that commit's body to name the 9. Tests — 1 finding9.1 — The test that pinned the round-1 regression is still deleted (
What went with it was the only guard on the defect this review found at round 1. Finding 1.3 was that The replacement is not equivalent in weight: This is not a request for new tests — it is the pull request removing a test it had. Restore 11. Nitpicks / Optional — 1 finding11.1 — "nothing polls in the background" is no longer true of this module (
Ninety lines further down, Narrowing it to the function is enough: "Pull-based: this state is not pushed, the UI reads it while it is on screen." The background-poll story is already told where it belongs, at Sections with nothing to report (6)3. AGENTS.md Adherence — ✅ (the one line that changed adds no interface, property or function signature, so 4. Security — ✅ ( 5. Performance — ✅ (the changed line runs inside 6. UI / UX — ✅ (no template, dialog, control or copy changed this round — 7. Code Quality & Style — ✅ ( 10. Documentation — ✅ (no Generated by Claude. This is advisory; a human reviewer must still approve. |
Turning a MAVLink package into data-lake variables never needed the vehicle instance, only the system id that also gets the legacy unprefixed names, so it moves out of MAVLinkVehicle into a module any connection can call.
6328c96 to
165359e
Compare
Review follow-up — round 5Done
|
|
/review |
Automated PR Review — round 6Tip ✅ READY TO MERGE — 0 open; 14 closed, 4 of them this round. Adds a settings panel where the user lists extra vehicles by websocket address. Cockpit opens a read-only link to each at startup and copies their telemetry into the shared pool of values that widgets and map markers read from. Each extra vehicle also gets an "is data arriving" flag in that pool, kept current by a once-a-second background check that only runs while at least one extra vehicle is configured, so a widget can tell a live reading from the last one received before the vehicle went quiet. This round fixed the stale-flag bug on removal, restored a deleted test, gave the status feature its own commit, and narrowed one comment. What still needs attentionNothing is open. All fourteen findings raised over six rounds are closed. Since round 5 — 4 closed, 0 new findings, comparing 6328c96 → 165359eRange —
What actually moved this round. Three code edits and a commit split:
Findings that changed status — all four open findings closed, each against the code rather than against the author's account of it:
New this round — none. Sections 0 through 11 were re-run over the whole of Discussion since round 5
Change map — what was established before judgingClaims (from the pull-request body and the author's round-5 follow-up — recorded, then checked against the code)
Failure site — n/a. The pull request fixes no reported bug. The only defect it repairs is the Entry points
No changed or added function traced to Invariants
2. Persistence & User Data — inventory, no findingsInventory
Nothing else is added, reshaped or removed, and the pull request contains no migration. Unchanged since round 1 — none of the five pushes since has touched persistence. Judgement
Sections with nothing to report (10)1. Correctness & Implementation Bugs — ✅ (the third producer of the status variable is now gated at 3. AGENTS.md Adherence — ✅ (the restored test file carries no 4. Security — ✅ (a search for 5. Performance — ✅ (the changed line is on the removal path, reached per user action, and now does strictly less work than before; the per-message path is untouched, with 6. UI / UX — ✅ (no template, dialog, control or copy changed this round — 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ (read from 9. Tests — ✅ ( 10. Documentation — ✅ (no 11. Nitpicks / Optional — ✅ (the module comment that had gone stale now reads "Pull-based: this state is not pushed, the UI reads it while it is on screen." at Generated by Claude. This is advisory; a human reviewer must still approve. |
ArturoManzoli
left a comment
ArturoManzoli
left a comment
There was a problem hiding this comment.
Still testing the feature and code
For now, some UI changes are needed:
(1) Change this icon to mdi-delete. The X icon (mdi-close) is usually meant to 'close' not to delete an element. It does make this card look like a message, banner or snackbar, instead of an instance of a vehicle.
(2) How to change the System ID? This procedure could be quickly described on the (i) icon above.
|
How about adding a button 'Place vehicle marker on the map' or something like that. So users can have a POI with already the |
… component The generic websocket panel is the second place in the general settings that lists addresses with their status and takes a new one, so the rows and the add field become a component both panels render with their own callbacks. The address example moves from a hint the panel never rendered (it is combined with hide-details) to the field's placeholder, and the remove button gains the accessible name it was missing.
165359e to
6b458b5
Compare
|
/review |
Automated PR Review — round 8Warning
The PR lets the operator list extra vehicle addresses on the local network and mirrors everything those vehicles announce into Cockpit's shared value store, read-only, filed per vehicle system ID with a flag saying whether it is still being heard from. A button beside each address drops a map marker per vehicle found there, wired to that vehicle's live position, and now refuses until the vehicle has actually said where it is. New this round is a change to how Cockpit fills values into the small formulas it evaluates: values are now inserted as data rather than as raw text, which closes the code-execution hole from the last round and also, as a side effect, changes what some formulas users already saved will compute. What still needs attention
Since round 7 — 6 closed, 2 new findings, comparing 6b458b5 → b1bb6fcRange.
What actually moved. Three things. (1) The substitution used before evaluating an expression: Resolutions. Status changes.
Discussion since round 7.
No text in Change map — what was established before judgingClaims (PR body and the two new commit bodies):
Failure site. For the one bug-fix part of this PR, the misbehaving code is the default replacement in Entry points.
The multiplier the diff does not show: the substitution change is not scoped to this feature. Every transforming function in the app — the two per live POI, the joystick axis outputs ( Invariants.
2. Persistence & User Data — inventory, 1 findingInventory.
The addresses being machine-local is right and was settled in round 5. No key is reshaped, no migration is added, and no default changes. What did change is the interpretation of an existing key's contents, which is the finding below. 2.2 — Values are now substituted as literals, which silently changes what already-saved expressions compute, and nothing tells the user ( Consequence: an operator whose saved formula pastes a vehicle's text value into a sentence now sees quote marks in the result, or a comparison that stops matching, with no error and no notice anywhere.
Under the old raw substitution (
Neither throws, so nothing surfaces: the function keeps evaluating and keeps writing a wrong value into its variable. The expressions live in To be explicit: the security fix must not be reverted, and 4.1 stays closed under either route below.
6. UI / UX — 1 finding6.4 — The map button treats an address as one vehicle, so a vehicle waiting for a position is skipped in silence and reported as already placed ( Consequence: on an address that carries more than one vehicle, one of them can be left off the map with no mention, or the operator can be told it is already there when it never was. The button is per row, and a row is an address. Concretely, for an address announcing systems 3 and 4:
Reachability: it needs one address carrying more than one autopilot (a MAVLink router, or BlueOS bridging two), which the panel already treats as an expected shape — the row renders "System ID 3, 4" from Fix: collect the two skip reasons separately in the walk that already visits each id — placed, already placed, waiting for a position — and say which vehicles are in which state, wording the tooltip for the number of vehicles the address announced rather than for one. Sections with nothing to report (9)1. Correctness & Implementation Bugs — ✅ (traced the new substitution to its only caller and its three 3. AGENTS.md Adherence — ✅ (the one added export carries typed non-filler JSDoc with 4. Security — ✅ (re-walked the whole 5. Performance — ✅ (the substitution runs on a per-incoming-message path for every transforming function in the app, and 7. Code Quality & Style — ✅ (the new helper reuses 8. Commit Hygiene — ✅ (eight commits, each one logical change: the map feature is now its own ( 9. Tests — ✅ (no existing test is deleted, skipped or weakened; the three test files in the diff are additions, and the new one asserts the substitution's three cases including the payload from finding 4.1 — no new assertion is asked for here) 10. Documentation — ✅ (the new code path touches no 11. Nitpicks / Optional — ✅ (re-read every comment added or changed this round: the round-7 "coast of Africa" wording is gone, the JSDoc that replaced it states the rule the code enforces, and no comment whose code is unchanged was reworded) Generated by Claude. This is advisory; a human reviewer must still approve. |
b1bb6fc to
62ffd73
Compare
Review follow-up — round 8Done differently
|
|
/review |
Automated PR Review — round 9Warning Cockpit can be pointed at the addresses of other vehicles on the network and copies their telemetry into the shared pool of values that widgets and map markers read, without any of them becoming the vehicle being piloted. Each address gets a button that drops a marker following each vehicle it hears from, and the formula language those markers are built on now inserts a value as a value rather than pasting it into the code that gets run. This round rewrote that insertion so a value used inside a piece of text keeps working the way saved formulas expect, and made the map button report each vehicle's outcome separately. The rewrite closed both of the findings that were open, and the way it decides where a placeholder sits is what the two new findings are about. What still needs attention
Since round 8 — 2 closed, 2 new findings, comparing b1bb6fc → 62ffd73Range —
Findings that changed status
New this round — 2. Sections 0 through 11 were re-run over the whole of
Discussion since round 8
Change map — what was established before judgingClaims (from the PR body and the commit bodies — recorded, then checked against the code)
Failure site — the PR implements a feature request, so there is no misbehaving code it is fixing. The two defects repaired in it are ones this review raised: the Entry points
No changed or added function traces to Invariants
2. Persistence & User Data — inventory, 1 findingInventory
The addresses being machine-local is right and was settled in round 5. No key is reshaped, no migration is added, and no default changes. What changed for the second round running is the interpretation of an existing key's contents: round 8's 2.2 covered the placeholders that sit inside a string literal, which this round restored, and the finding below is the half of the same change that nothing has covered yet. 2.3 — A numeral held by a string-typed variable now concatenates instead of adding in already-saved expressions, and nothing tells the user ( Consequence: an operator whose saved formula adds up a value that arrives as text now sees the digits glued together instead of a sum, or a comparison that stops matching, with no error and no notice anywhere. Outside a string literal the substitution is
This is reachable through an in-tree design decision rather than by accident. Those expressions live in Raised now rather than folded into 2.2: 2.2 was scoped to placeholders inside string literals and the code has closed it on those terms. This half went unmentioned in round 8, when the same change introduced it. 4. Security — 1 finding4.2 — The string-literal scan misreads any unbalanced quote in code position, and an escaped-but-unquoted value is syntax again ( Consequence: a vehicle at one of the configured addresses can get Cockpit to run code of its choosing, whenever one of the operator's saved formulas has an apostrophe — in a comment, most likely — ahead of the value it reads.
The Reachability, in the editor this feature is actually used from:
To be explicit about the provenance: the escaped-unquoted form is the "complete route" that round 8's 2.2 prescribed, and that prescription did not say what to do when the scan is wrong. The correction is to make the scan's failure mode the safe substitution instead of the unsafe one:
AGENTS.md, "Do not be lazy about: … input validation at trust boundaries, error handling that prevents data loss, security" ( Sections with nothing to report (9)1. Correctness & Implementation Bugs — ✅ (confirmed 3. AGENTS.md Adherence — ✅ (both added exports carry typed, non-filler JSDoc with 5. Performance — ✅ (the added scan is one pass and one boolean array per evaluation, on the per-incoming-message path traced above, against the 6. UI / UX — ✅ (re-checked 6.4's fix at the three outcome sentences and the pluralised tooltip/ 7. Code Quality & Style — ✅ ( 8. Commit Hygiene — ✅ (eight commits, each one logical change; the two at the tip were amended rather than patched by follow-ups, so no 9. Tests — ✅ (no existing test is deleted, skipped or weakened; 10. Documentation — ✅ (no 11. Nitpicks / Optional — ✅ (re-read every comment added this round: the two on the outcome walk and the coordinate variable state why rather than what, and the only inaccurate one is the Generated by Claude. This is advisory; a human reviewer must still approve. |
62ffd73 to
8da9311
Compare
Review follow-up — round 9Done
|
|
/review |
Automated PR Review — round 10Warning Cockpit can be pointed at the addresses of other vehicles on the network and copies their telemetry into the shared pool of values that widgets and map markers read, without any of them becoming the vehicle being piloted. Each address gets a button that drops a marker following each vehicle it hears from, and the formula language those markers are built on now inserts a value as a value rather than pasting it into the code that gets run. This round taught the part that decides where a value is being inserted to skip comments, and made it fall back to the safe insertion whenever it loses track — which closes both findings that were open. What it still gets wrong is a formula that does arithmetic in the middle of a piece of text it is building: there the value goes back to being code. What still needs attention
Since round 9 — 2 closed, 1 new finding, comparing 62ffd73 → 8da9311Range —
Findings that changed status
New this round — 1. Sections 0 through 11 were re-run over the whole of
Discussion since round 9
Change map — what was established before judgingClaims (from the PR body and the commit bodies — recorded, then checked against the code)
Failure site — the PR implements a feature request, so there is no misbehaving code it is fixing. The defects repaired in it are ones this review raised: the Entry points
No changed or added function traces to Invariants
2. Persistence & User Data — inventory, no findingsInventory
The addresses being machine-local is right and was settled in round 5. No key is reshaped, no migration is added, no default changes, and nothing stored is dropped or rewritten. The one live persistence question on this PR was the re-interpretation of an existing key's contents, in two halves: placeholders inside a string literal (round 8's 2.2, closed in round 9 by restoring what they computed) and a string-typed numeral in arithmetic (2.3, closed this round by the disclosure AGENTS.md asks for when carrying users over is not available — the popup bullet at 4. Security — 1 finding4.3 — A placeholder inside a template literal's Consequence: a vehicle at one of the configured addresses can get Cockpit to run code of its choosing, whenever one of the operator's saved formulas builds a string with a backtick template and does anything to the value inside the
The reachable expression is an idiomatic one, and it is the reason someone reaches for
This is not the ceiling the Two ways to correct it, in the shape of the existing code:
AGENTS.md, "Do not be lazy about: … input validation at trust boundaries, error handling that prevents data loss, security" ( Sections with nothing to report (9)1. Correctness & Implementation Bugs — ✅ (walked the changed scan for the safe direction: the comment jump at 3. AGENTS.md Adherence — ✅ (the changed scan keeps its typed JSDoc, now stating the comment behaviour ( 5. Performance — ✅ ( 6. UI / UX — ✅ (the popup bullet reads as sentence case and states the consequence in the operator's terms rather than the implementation's; 6.4's three outcome sentences and the pluralised tooltip/ 7. Code Quality & Style — ✅ (no complexity report for this head, so no measured complexity or nesting figure is available and none is claimed; by inspection the scan gained one branch and no nesting level beyond the loop it already had, and it stays in the framework-agnostic 8. Commit Hygiene — ✅ (eight commits, each one logical change; the two at the tip were amended rather than patched by follow-ups, so no 9. Tests — ✅ (no existing test is deleted, skipped or weakened; 10. Documentation — ✅ (no 11. Nitpicks / Optional — ✅ (re-read every comment added or changed this round: the rewritten Generated by Claude. This is advisory; a human reviewer must still approve. |
8da9311 to
393b57e
Compare
Review follow-up — round 10Done
Won't change (with reasoning)
|
|
/review |
Automated PR Review — round 11Warning The PR lets the operator list extra vehicle addresses in the general settings. Cockpit opens a read-only websocket to each one, mirrors whatever telemetry arrives into the data lake under names derived from the sending vehicle's system ID, reports per-link and per-vehicle reception status, and offers a one-click button that drops a map marker following each vehicle's live position. Alongside that, it changes how a The round-10 finding is fixed: the scan now tracks template substitutions and reads a placeholder inside What still needs attention
Since round 10 — 1 closed, 1 new, comparing 8da9311 → 393b57e
4.3 — A placeholder inside a template literal's The finding asked for three things, and all three landed:
The residual the ponytail names — a quote or a brace inside a regex literal shifting what follows it — is the same marked ceiling round 10 accepted, and it is still only reachable by an expression the operator wrote themselves. New this round: 4.4 (major), below. It is not a regression from the round-10 fix; it is a second route into the same Discussion. rafaellehmkuhl's round-10 follow-up comment describes the stack approach, the two new tests, and the ponytail and JSDoc corrections; each claim was checked against the diff above and each holds. The comment also concedes the round-9 remark that "both fail without the change" was imprecise, since the comment case is caught by the open-literal backstop on its own — that concerns no open finding and changes nothing. The only other comment since round 10 is the bare Change map — what was established before judgingClaims.
Failure site. Not a bug fix. The one behaviour change that rides along, the literal substitution, has its site at Entry points.
Invariants.
2. Persistence & User Data — inventory, no findings
Judged: no key was reshaped and no automatic migration was added, so nothing rewrites existing user data. The meaning shift on the third row was raised as 2.2 and 2.3 in earlier rounds and closed by the note now in the expression info popup ( 4. Security — 1 finding4.4 — The announced system ID is used unvalidated, so a mirrored vehicle chooses both its own variable names and the text of a generated expression (
|
393b57e to
9ed697b
Compare
Review follow-up — round 11Done
Squashed into the two commits that own the code: the guard into |
|
/review |
Automated PR Review — round 12Warning The PR lets the operator list extra vehicle addresses in the general settings. Cockpit opens a read-only websocket to each one, mirrors whatever telemetry arrives into the data lake under names derived from the sending vehicle's system ID, reports per-link and per-vehicle reception status, and offers a one-click button that drops a map marker following each vehicle's live position. Alongside that, it changes how a The round-11 finding is fixed: the announced system and component IDs are now validated as integers at the one place every consumer is fed from. Re-running the whole diff surfaced the remaining half of the same hole — the numbers in a mirrored variable's name are now checked, but the words around them are still whatever the remote endpoint typed into its JSON, and the variable picker splices a variable's name straight into an expression that gets evaluated. What still needs attention
Since round 11 — 1 closed, 1 new, comparing 393b57e → 9ed697b
4.4 — The announced system ID is used unvalidated — ✅ Addressed. The finding asked for three things, and all three landed:
New this round: 4.5 (major), below. It is not a regression from the round-11 fix and not a re-litigation of it: the guard covers the header, and 4.5 is about the message body, which no guard has ever looked at. Twelve rounds, this one included, read the variable name as a fixed shape and only argued about the numbers inside it. Discussion. rafaellehmkuhl's round-11 follow-up (summarised rather than quoted, since it is wrapped in markup) states that the header IDs are checked at the single chokepoint, that both arms close as a result, and that the guard and the test were squashed into the two commits owning that code. The code claims were checked against the diff above and each holds; which commit each hunk ended up in is not visible from a squashed head diff, and nothing in the commit list contradicts it. The only other comment since round 11 is the bare Change map — what was established before judgingClaims.
Failure site. Not a bug fix. The one behaviour change that rides along, the literal substitution, has its site at Entry points.
Invariants.
2. Persistence & User Data — inventory, no findings
Judged: no key was reshaped and no automatic migration was added, so nothing rewrites existing user data. The meaning shift on the third row was raised as 2.2 and 2.3 in earlier rounds and closed by the note now in the expression info popup ( 4. Security — 1 finding4.5 — The message body of a mirrored vehicle is copied verbatim into data-lake ids, and the expression editor splices an id into code (
|
Opens a read-only MAVLink link per configured address and files everything the vehicles on it announce under /mavlink/<system id>/..., so widgets and live points of interest can read them. The links bypass the ConnectionManager, since adding a connection there replaces the main one and its onRead is what feeds the main vehicle and the vehicle factory. So none of these vehicles can be commanded, or become the vehicle being piloted. Only systems that announce themselves as a vehicle are mirrored, and only from the first link to claim each system ID, so a link can never write over the piloted vehicle's variables or over another link's.
A vehicle dropping mid-flight was invisible: its `/mavlink/<system id>/...` variables simply stopped updating, and widgets kept rendering the last value received, indistinguishable from live data. Each mirrored vehicle now gets `/vehicles/mavlink/<system id>/isReceivingData`, a boolean data-lake variable created on that vehicle's first message and kept up to date app-wide, with no UI mounted, by a module-level 1 Hz poll that only exists while at least one link does. A timer is needed because the receiving to quiet transition has no message to notice it by. Last-seen is tracked per system ID rather than per link, since one link carries every system on its network, and the silence threshold is the same connection watchdog timeout the per-link status uses, now a shared pure predicate so the two cannot contradict each other. Removing an address writes a final `false` for the vehicles it was mirroring, and leaves the variable in place, as the module does for every variable it injects.
…esses Boots the links at startup so the telemetry reaches the data lake with no UI mounted, and refuses an address that is malformed, already added, or the main vehicle's. The addresses are stored machine-local, since they are addresses on the local network.
Lists the configured addresses with their link status and the system IDs each one announced, and warns when two vehicles share an ID, since only the first to announce it is received.
The expression of a transforming function is evaluated as JavaScript with its
'{{ variable }}' inputs substituted raw, so a variable holding a string was read
as syntax rather than as a value, and whatever wrote it chose what ran. Writing
each value as a literal keeps a string a string, and makes comparisons against
one work where they used to throw.
An input inside a string literal is text being built rather than a value being
read, so there the value is escaped instead of quoted. Expressions already saved
under the old substitution, where quoting the input was the only way to use a
string variable, keep computing what they always did, and the value still cannot
close the literal it lands in.
Telling one position from the other is a scan over quotes, comments and template
substitutions, which skips a comment so an apostrophe in one cannot hand a code
position the unquoted form, reads a '${ }' as the code it is rather than as part
of the literal around it, and quotes every value when it ends with something
still open, since by then it can no longer tell code from text.
Each configured address gets a button that creates a point of interest per vehicle it announced, with the vehicle's position variables as its coordinates, so the marker follows the vehicle with nothing typed by hand. A vehicle can only be placed once it has reported a position, since the point of interest is stored with a fallback location for whoever has no live data for it, and it is synced to every operator of the piloted vehicle while the address it follows is not. Its coordinate expression is also what identifies the marker as that vehicle's, so a point of interest the user happened to name after it does not stop the button.
9ed697b to
f35329b
Compare
Review follow-up — round 12Done
Won't change (with reasoning)
|
|
/review |
Automated PR Review — round 13Note 📝 MINOR SUGGESTIONS — 1 open (1 minor, disputed) · 26 closed. The PR lets the operator list extra vehicle addresses in the general settings. Cockpit opens a read-only websocket to each one, mirrors whatever telemetry arrives into the data lake under names derived from the sending vehicle's system ID, reports per-link and per-vehicle reception status, and offers a one-click button that drops a map marker following each vehicle's live position. Alongside that, it changes how a Round 12's finding is half fixed, and the half that mattered is the one that landed: a mirrored vehicle can no longer choose a variable name that turns into code when the operator picks it out of the expression editor's list. What is left of 4.5 is the resource half — the same endpoint can still mint an unlimited number of perfectly well-formed names — and the author has argued against the guard round 12 proposed for it. That argument is partly right, which is why the finding is now graded What still needs attention
🙋 Decisions for a human4.5 — A mirrored vehicle can mint an unbounded number of data-lake variable ids
Ticking a box records the decision here; the finding itself closes only on Since round 12 — 1 partially addressed and disputed, comparing 9ed697b → f35329b
4.5 — The message body of a mirrored vehicle is copied verbatim into data-lake ids — :large_yellow_circle: Partially addressed, and the remainder 💬 Disputed. The finding asked for two guards. Restating each and what landed:
Discussion. rafaellehmkuhl's follow-up (summarised rather than quoted, since it is wrapped in markup) states the first guard as done and declines the second, on the grounds that the enum check does not bound the namespace it is offered for and would drop telemetry from a newer dialect. Both halves of that were checked against the code rather than taken as given, and both hold: a hostile endpoint can mint
Change map — what was established before judgingClaims.
Failure site. Not a bug fix. The one behaviour change that rides along, the literal substitution, has its site in Entry points.
Invariants.
2. Persistence & User Data — inventory, no findings
Judged: no key was reshaped and no automatic migration was added, so nothing rewrites existing user data. The meaning shift on the third row was raised as 2.2 and 2.3 in earlier rounds and closed by the note now in the expression info popup ( 4. Security — 1 finding (carried from round 12, narrowed)4.5 — A mirrored vehicle can mint an unbounded number of data-lake variable ids (
|
|
/resolve 4.5 - If we connected to the vehicle we trust it. |
|
Recorded: rafaellehmkuhl resolved 4.5. Comment |
3 participants
Adds "get" support for extra vehicles in the network by pointing their address in the General settings. Each one's telemetry is mirrored into the data lake under
/mavlink/<system id>/..., so widgets can read it and a point of interest can follow each vehicle on the map.Example below showing my vehicle in Brazil and Tony's vehicle in Hawaii:
These links are read-only and bypass the
ConnectionManager, since adding a connection there replaces the main one and itsonReadis what feeds the main vehicle and the vehicle factory. So none of the extra vehicles can be commanded, or become the vehicle being piloted.The first commit is a behavior-preserving refactor: turning a MAVLink package into data-lake variables never needed the vehicle instance, only the system id that also gets the legacy unprefixed names, so it moves out of
MAVLinkVehicleinto a module any connection can call.Only systems that announce themselves as a vehicle are mirrored, and only from the first link to claim each system ID, so neither two vehicles sharing an ID nor one using the piloted vehicle's can write over each other's variables. The panel warns when that happens, so the user knows why a vehicle is not showing up. The configured addresses are stored machine-local, since they are addresses on the local network, and no migration is involved.
Each mirrored vehicle also gets
/vehicles/mavlink/<system id>/isReceivingData, a boolean data-lake variable saying whether data from that vehicle is currently arriving. Without it, a vehicle dropping mid-flight is invisible: its variables just stop updating and widgets keep rendering the last value received. It is kept up to date app-wide with no UI mounted, uses the same silence threshold as the link status in the panel, and is writtenfalseonce when the address is removed.A behavior change rides along in its own commit: a
{{ variable }}in a transforming function's expression is now substituted as a value instead of being pasted into the code that gets evaluated, so no vehicle on the network can choose what an expression runs. Each value keeps its own type, so a saved expression that adds up a variable which arrives as text (a Generic WebSocket supplier can force that by quoting the value it sends) now concatenates the digits where it used to add them, and a strict comparison against a number stops matching. Placeholders inside a string literal are unaffected, and the expression info popup states that a variable enters with its own type.Fixes #1098.