chore: drop the management product's name from the public tree - #150
Conversation
Refer to it generically as the management backend/console. The two env vars that carried the name are renamed: PSTOP_CHECKIN_URL and PSTOP_CHECKIN_API_KEY_FILE (deployments that set the old names must update; there is deliberately no fallback, which would keep the string).
![graphify-labs[bot]](https://avatars.githubusercontent.com/in/4356270?s=60&v=4){kind=small}
There was a problem hiding this comment.
Graphify reviewed this change.
Worth a look — the grounded gate found no coupling regressions or blocking issues, but 1 advisory finding(s) below merit a look before merge.
Graphify review — findings
Renames the fleet check-in environment variables from PSTOP_FLEET_CHECKIN_URL/PSTOP_FLEET_API_KEY_FILE to PSTOP_CHECKIN_URL/PSTOP_CHECKIN_API_KEY_FILE in build_backend, so operators must set the new names to override the checkin URL/key (the old names no longer take effect). Rewords "pstop-fleet"/"fleet console" to "management backend"/"management console" across docs, params, and comments with no behavioural change.
Worth a look
- Environment variable rename breaks existing deployments using PSTOP_FLEET_CHECKIN_URL / PSTOP_FLEET_API_KEY_FILE —
ros2/protective_stop_machine/src/machine_bridge_node.cpp:97· Escalate · medium- agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification
Impact & health
Graphify review
Impact — 122 functions depend on the 120 functions this change touches.
Health — this change adds coupling hotspots:
- new:
TEST()— 0 callers, 6 callees
Verification — 122 functions in the blast radius were not formally verified this run (proofs are advisory here).
Gate & verification
graphify gate
PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.
Advisory (not blocking):
- verification_scope: 122 function(s) in the blast radius were not formally verified this run
· 1 more finding(s) on lines outside this diff (see the check run).
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Looks good — a mechanical, consistently-applied rename with no logic changes. Reviewed the env var rename (PSTOP_FLEET_CHECKIN_URL/PSTOP_FLEET_API_KEY_FILE to PSTOP_CHECKIN_URL/PSTOP_CHECKIN_API_KEY_FILE) across machine_bridge_node.cpp, the params YAML, header comments, and the test — all consistent, and confirmed no other code paths still reference the old names. Also checked tools/hil/ONBOARDING.md's PSTOP_FLEET_URL to PSTOP_CHECKIN_URL swap — neither name is actually read by any script in this repo (tools/flash_pstop.sh, tools/hil/run.sh), so this doc line was already disconnected from real code before the PR and the rename doesn't introduce a new regression.
Extended reasoning...
Overview
This PR (commit e93c84f) is a pure find-and-replace rename removing the proprietary product codename "pstop-fleet"/"fleet" from comments, docs, and identifiers across 10 files, plus a related rename of two environment variables (PSTOP_FLEET_CHECKIN_URL to PSTOP_CHECKIN_URL, PSTOP_FLEET_API_KEY_FILE to PSTOP_CHECKIN_API_KEY_FILE) used for the optional ROS2 software-machine fleet check-in feature. I ran the diff directly and confirmed every hunk in the ros2/, firmware/, and docs/ files is a comment, docstring, or string-literal change only — no control flow, parameter names, or public API surface changed besides the two env var string literals passed to std::getenv().
Security risks
None identified. The env vars only gate an optional, non-safety-path HTTP check-in call (explicitly documented as "Entirely off the safety path: a dead fleet only logs, and no firmware OTA is ever attempted"). No secrets, keys, or URLs are hardcoded; the rename affects only the names of environment variables used to inject secrets at deploy time, not the secrets themselves.
Level of scrutiny
Given the change is comment/doc/string-literal only plus a mechanical, consistently-applied env-var rename with matching updates in the C++ source, header, YAML params, and unit test, this warrants light scrutiny. I independently verified the two candidate concerns flagged as ruled out: (1) tools/hil/ONBOARDING.md's PSTOP_FLEET_URL (ESP32 OTA/flash secrets, described as provided directly to the CI runner) was never read via getenv() by any script in tools/hil/ or tools/flash_pstop.sh in this repo — it documents a manual runner secret injection process external to this codebase, so the rename to PSTOP_CHECKIN_URL doesn't break working code, it just continues a pre-existing (harmless) doc/code disconnect; (2) the "silently disabled with zero log output" behavior for deployments still exporting old env var names in machine_bridge_node.cpp is pre-existing logic (same empty-string silent-disable pattern existed before this rename for any misconfiguration), not a new regression introduced by this diff.
Other factors
The change is small (15 lines across 10 files per the PR description) and mechanical in nature, matching the stated verification (clean build, 8/8 unit tests). No CLAUDE.md conventions are violated — the ros2/CLAUDE.md conventions around generate_parameter_library, thin nodes, and descriptive naming are unaffected since no structural code changed. No CODEOWNERS conflicts were apparent. Given the bug-hunting system reported zero bugs and exited via the normal dry_streak path (not a truncated run), and my own independent check of the diff and the two ruled-out candidates found nothing further, this appears safe to approve.
rajasimman-madhivanan
left a comment
rajasimman-madhivanan
left a comment
There was a problem hiding this comment.
Where are these defined? Do the other places reflect the name change?
Those exact variables are not defined in the repository.
The implemented names are:
- PSTOP_FLEET_CHECKIN_URL
- PSTOP_FLEET_API_KEY_FILE
They are read in ros2/protective_stop_machine/src/machine_bridge_node.cpp:98 (ros2/protective_stop_machine/src/machine_bridge_node.cpp#L98).
Their ROS parameter equivalents are defined in ros2/protective_stop_machine/src/protective_stop_machine_params.yaml:151 (ros2/protective_stop_machine/src/protective_stop_machine_params.yaml#L151): - fleet.checkin_url
- fleet.api_key_file
|
@rajasimman-madhivanan — the two names are environment variables, so they are not defined in the repo anywhere (that is the point: the URL and key stay out of committed config and come from the deployment's environment — systemd unit / launch env). In-repo they are only read and documented. You were most likely looking at Read (the only code that consumes them):
Documented:
Consistency check on the head: Deployment note (also in the PR body): any environment that exports the old names must switch to the new ones — there is deliberately no fallback, because a fallback would keep the product name in the tree. The two other check-in vars ( |
…rd re-establishment, coord deadline (#156) Consolidates #149, #152, #151, #155 and #153 (closed as superseded; their review threads and bot findings were addressed there and the fixes are included here). 12 linear commits, 409+/29−. Every commit was in the build that passed the 3.7 h soak below. ## What is fixed | area | commit(s) | defect | |---|---|---| | USB tether TX | `de335e8` `1b5fe82` | `tinyusb_net_send_sync(…, 0)` reclaimed the packet before the equal-priority TinyUSB task ran the deferred send — ~95 % of device→host frames silently dropped (regression from #135). Now a 20 ms wait; TIMEOUT-after-send duplicate accepted and documented. | | USB tether ring | `3a8a527` | 16-slot ring lost ~1 safety frame/min on a hub-chain host (burst-induced). 64 slots (96 KB PSRAM), 400 ms lifetime. | | Tether link state | `f9ec4cb` | netif never went link-down when the host detached/suspended, so a dead tether stayed the preferred uplink. esp_tinyusb events now drive it; device netif gets its own MAC. | | WiFi failover crash | `9823785` | first time the failover path ever ran it aborted: `dcs_wifi_set_enabled` reaches flash from the supervisor's PSRAM stack. Toggle moved to an internal-stack worker. | | WireGuard re-establishment | `0440ad2` `5b86d21` `e020c86` `cae7e94` `54149eb` `d7a335f` | **root cause of the multi-minute/indefinite bond drops**: on direct regain with no keypair left, the live endpoint stayed 0.0.0.0 (relay-only) and a safety peer fell into the bulk-peer "one-shot, wait for them to initiate" — the machine host's wireguard-go never initiates without data. Safety peers now connect with paced retries; initiations get a second leg to the peer's authenticated direct disco source while DERP-only; disco pings fan out to it. Per-peer WG diagnostics in `/admin/api/peers`. | | Coord partial frame | `8cf5104` | `coord_recv` bounded by a retry count × 2 s socket timeout (≈10 min); now a 10 s wall-clock deadline, `ETIMEDOUT` so callers reconnect (#127). 88-check host test. | ## Evidence - Deterministic WG repro (block direct+DERP, reboot remote mid-outage, unblock direct only, no host help): **v1.3-14 never recovers (>300 s); this branch bonds in 22 s.** - Soak 2026-09-19 12:06–15:47 PDT, 3.7 h, this code + #148/#150/#154, two Ethernet remotes + one USB remote bonded to a ROS 2 machine on the tether host, DUT E-stop cycled every 5 min (42/42, stop ≤0.5 s, re-arm ≤0.75 s): **zero bond drops / rebonds / resets, 100 % bonded samples on all three** (12 638 × 1 Hz each); reply age p99.9 361–391 ms; USB ring 566 916 frames, 0 expired, 0 dropped; machine-side 194 207 requests, 194 170 answered (the rest late DERP-mirror copies, correctly rejected). One 1.07 s receive-side excursion on an Ethernet remote (W5500 RX class, tracked separately). - Baseline for the same class of USB remote on v1.3: 252 ring drops + 55 expiries per 4 h, and 2–7 min bond outages. ## Not in this PR #148 (release guard), #150 (name scrub), #154 (lockstep mismatch diagnostics — SIL comparator, reviewed separately). DERP-mirror latency (1.3 s median) and the W5500 RX stall are follow-ups.
2 participants
What
The internal management product must not be named in this public repository. 15 lines across 10 files (comments, docs, one HTML comment) now say "the management backend/console" instead. The generic noun fleet (
FleetCheckin,params.fleet.*,fleet_ota_checkin) is unchanged.Behaviour-visible change
Two environment variables carried the name and are renamed:
PSTOP_FLEET_CHECKIN_URLPSTOP_CHECKIN_URLPSTOP_FLEET_API_KEY_FILEPSTOP_CHECKIN_API_KEY_FILEDeliberately no fallback to the old names (it would keep the string in the tree). Deployments that export the old names must update them; the params-file keys (
fleet.checkin_url,fleet.api_key_file) are unchanged.tools/hil/ONBOARDING.mdreferred to aPSTOP_FLEET_URLthat never matched the code; it now names the real variables.Verification
colcon build --packages-select protective_stop_machineclean; check-in unit tests 8/8.git grep -ifor the name over the whole tree: 0 hits.15+/15−.