Repository navigation
fix(tesla-ble): apply must/should items from multi-agent review of #211 - #213
Merged
Merged
Conversation
Multi-agent review of the tesla-ble-driver branch surfaced a punch list
of correctness, security, and UX items. This commit applies the
must-fix and should-fix subsets in a single coherent change:
control/dispatch (must)
- Republish FuseEVMaxW after forceFuseDischarge runs. The reactive
fuse-saver may swing battery from charge→discharge between when the
joint allocator computes the EV cap and when targets are dispatched;
the published cap was stale by the time the loadpoint controller
read it. Now reflects post-saver battery totals so the EV gets the
fuse headroom it actually has.
- Document that the joint allocator's H computation uses rawGridW
directly, so its math is independent of BatteryCoversEV (which only
affects PI's gridW). Regression test pins the design contract.
telemetry (must)
- New telemetry.PickBestVehicle helper centralises the multi-vehicle
picker that was inline-duplicated in main.go and api.go. SoC and
charge_limit are bounded to [0,100] and readings older than
VehicleMaxAge (5 min) are skipped — defends the loadpoint and MPC
against a misbehaving BLE proxy publishing 200% SoC or stalling.
- VehicleConnectedRank moved into the same package as the single
source of truth for the rank table.
api/loadpoints (must)
- decorateLoadpointsWithVehicle no longer overwrites CurrentSoCPct
with the BMS reading. The controller's plan was computed from the
inferred value the previous tick; mirroring BMS truth onto
CurrentSoCPct made the UI lie about which value the controller
used. VehicleSoCPct already exposes the BMS value separately.
api/tesla-verify (should + sec)
- Per-VIN rate limit (30 s) on /api/drivers/verify_tesla — same-origin
XSS or a tab the operator left open can no longer pump verifies as
a "keep car awake" loop.
- wakeup=true cooldown (5 min) — calls inside the window omit the
flag and the proxy serves cached data, sparing the 12V battery.
- Detect CheckRedirect rejection wrapped in *url.Error so the upstream
Location header doesn't leak in the error body.
- splitHostPort uses net.SplitHostPort under the hood; explicit ":0",
out-of-range, and non-numeric ports fall back rather than silently
rewriting the operator's intent.
drivers/lua (should + sec)
- HTTP allowlist is now port-aware: bare entries ("192.168.1.50")
retain the old host-only behaviour; "host:port" entries restrict to
exactly that port. Drivers granted access to one proxy can no longer
probe SSH/22, Redis/6379, etc. on the same host.
- Schemes other than http/https are rejected — no file://, ftp://, etc.
drivers/tesla_vehicle.lua (should)
- Don't force BLE wakeup on the first poll. A crash-loop would
otherwise hammer the radio at multiple wakeups per minute. The
30-min cadence still anchors at startup so the first scheduled
wake fires WAKEUP_INTERVAL_MS after init.
- driver_cleanup nils every persistent piece of state, not just
last.soc, so a hot-reload re-anchors the cadence cleanly.
cmd/main.go (should)
- Startup replan goroutine observes ctx.Done() during its 2 s warm-up
sleep, so SIGTERM during startup doesn't keep the goroutine alive.
web (must)
- ftw-energy-flow.js aggregateGroups now propagates chargeLimit /
socStale / socSource so a Tesla-only setup doesn't lose those
fields in the default aggregated view.
- EV-bubble SoC text drops the chargeLimit suffix when the planet
radius shrinks below 50 px (4+ planets clustered in one corner) —
prevents the "~24/50%★" overflow.
- "★" stale marker → "⚠ " prefix with stat-warn fill colour.
"★" universally reads as "favourite", the opposite of "degraded".
tests
- api_tesla_verify_test.go: SSRF gate (RFC1918 only, port allowlist,
loopback/link-local rejected), VIN regex (path/query smuggling),
rate-limit + wakeup cooldown helpers, malformed input variants.
- lua_http_test.go: end-to-end allowlist enforcement against a real
httptest server, capability-not-granted block, splitHostPortLower
IPv6/edge-case table.
- vehicle_test.go: PickBestVehicle bounds, freshness, rank ordering,
multi-vehicle tiebreak.
- control_test.go: post-republish FuseEVMaxW regression, BatteryCoversEV
mode-independence regression.
All packages pass; full e2e green (25.7 s).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 of 3 tasks
…ner-default Two related fallback gaps surfaced when an operator set per-battery power limits only under the `batteries:` config section: - driverLimitsFrom() only read drivers[].max_charge_w/max_discharge_w. When operators put their 9 kW limit under batteries.pixii (where the MPC reads it from), the dispatch's per-driver clamp silently fell back to MaxCommandW = 5000 W. The planner happily scheduled 9 kW discharge against the configured battery, but every dispatch tick capped at -5 kW. Now driverLimitsFrom takes the batteries map and uses it as the fallback for any driver whose drivers-section entry doesn't override. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…id sign-flip The energy-allocation dispatch path tracks slotDelivered as a running integral of commanded battery power across the slot, then derives the instantaneous setpoint from (BatteryEnergyWh − slotDelivered) / remaining_s. This works inside a single plan. But MPC issues reactive replans on PV/load divergence: a slot in flight can have its BatteryEnergyWh shrunk mid-slot (e.g. -2250 → -800 Wh) while the accumulator already passed the new target. When that happens the next tick computes remainingWh = -800 − (-1500) = +700 Wh and the dispatch flips sign — telling the inverter to charge during a discharge slot. The plan/exec sign-mismatch floor (bab8ffa) catches this and idles instead, preserving SoC, but the underlying behaviour is wrong on its own merits: charging against a shrunk-but-still-discharge directive "buys back" the energy the planner just sold at peak prices. Cap slotDelivered to the new BatteryEnergyWh in the same direction when the directive shrinks under it. remainingWh becomes 0 and the rest of the slot idles — the conservative behaviour the sign-floor was approximating after the fact. Also keep state.currentDirective.BatteryEnergyWh / SlotEnd fresh on non-rollover ticks so the formula reads the live directive, not the one from the slot rollover. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Applies the must-fix and should-fix subset from the multi-agent code review of #211 (security / UX / UI / code-review / tests / implementation perspectives, run in parallel).
What changed
Correctness (must)
FuseEVMaxWafterforceFuseDischargeruns so the EV cap reflects post-saver battery totals (no more 1-tick stale throttle).telemetry.PickBestVehiclehelper bounds SoC + chargeLimit to[0,100], skips wallclock-stale readings (>5 min), and consolidates the multi-vehicle picker (was inline-duplicated inmain.goandapi.go).decorateLoadpointsWithVehicleno longer overwritesCurrentSoCPctwith BMS truth — was making the UI lie about which value the controller actually used.VehicleSoCPctalready exposes the BMS value separately.Security
wakeup=truecooldown (5 min) on/api/drivers/verify_tesla— same-origin XSS or a kept-open tab can no longer pump verifies as a "keep car awake" loop.host:portentries restrict to the specified port only. Drivers granted access to one proxy can no longer probe SSH/22, Redis/6379, etc.http/httpsare rejected from the Lua HTTP capability.Locationheader in the error body.splitHostPortusesnet.SplitHostPort; explicit:0/ out-of-range / non-numeric ports fall back instead of silently rewriting operator intent.Reliability (should)
tesla_vehicle.luano longer forces a BLE wake on the first poll — a crash-loop would otherwise hammer the radio at multiple wakeups per minute. The 30-min cadence still anchors at startup so the first scheduled wake firesWAKEUP_INTERVAL_MSafter init.driver_cleanupnils every persistent piece of state (was onlylast.soc), so a hot-reload re-anchors the cadence cleanly.ctx.Done()during its 2 s warm-up sleep.UX / UI
aggregateGroupspropagateschargeLimit / socStale / socSourceso a Tesla-only setup doesn't lose those fields in the default aggregated view.chargeLimitsuffix when the planet radius shrinks below 50 px (4+ planets clustered) — fixes the~24/50%★overflow.Tests
api_tesla_verify_test.go: SSRF gate (RFC1918 only, port allowlist, loopback/link-local rejected), VIN regex (path/query smuggling), rate-limit + wakeup cooldown helpers, malformed input table.lua_http_test.go: end-to-end allowlist enforcement against anhttptestserver, capability-not-granted block,splitHostPortLowerIPv6/edge-case table.vehicle_test.go:PickBestVehiclebounds, freshness, rank ordering, multi-vehicle tiebreak.control_test.go: post-republishFuseEVMaxWregression, BatteryCoversEV mode-independence regression. Updated existingTestJointFuseAllocatorScalesBothBatteryAndEVexpectation to reflect the new (looser, correct) post-republish cap.Not in scope (deferred)
slotDeliveredovershoot amplified by PR feat(control): reactive fuse-saver — battery always discharges to protect fuse #208's reactive fuse-saver. Diagnostic plan documented in the review summary; fix is a separate one-line bound indispatch.go:549after operator confirmation via telemetry.mpc.TriggerSetfrom main.go (~188 lines of mixed state), moveapi_tesla_verify.go(267 lines, mini-driver) intointernal/teslable, and various polish items (CSS tokens, mobile breakpoint onfield-row).Test plan
go test ./...— all packages pass (verified locally; e2e 25.7 s)/api/drivers/verify_teslatwice in 5 s → second returns 429allowed_hosts: ["192.168.1.50:8080"]→host.http_get("http://192.168.1.50:22/")returns "host (port 22) not in allowed_hosts"🤖 Generated with Claude Code