feat(appproto): ops to set the car's charge level and PV-only - #1069
Conversation
Two command operations the app can send where the passthrough refuses the
HTTP routes: loadpoint.soc.set re-anchors the car's current state of charge
through Manager.SetCurrentSoC and replans synchronously with reason
loadpoint_soc_corrected; loadpoint.surplus_only.set flips the flag through
Manager.SetSurplusOnly and, when it turns off, forces the same synchronous
surplus_only_disabled replan the target route does. Both read back what the
box now holds. Neither sits behind the dispatch gate, for the mode's reason.
POST /api/loadpoints/{id}/soc and POST /api/loadpoints/{id}/target now
carry Via(op), so the passthrough's E_USE_CMD names the command.
contract/registry.yaml is the byte-identical copy of the app's; contract_gen.go
is regenerated from it.
Refs srcfl/ftw-webapp#59
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31a90a00b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| soc, ok := argNum(cmd.Args, "soc") | ||
| if !ok || soc < 0 || soc > 1 { | ||
| return h.rejectArg(cmd, "soc", cmd.Args["soc"]) |
There was a problem hiding this comment.
When a CBOR client sends soc: NaN, both range comparisons evaluate false, so the command reaches the manager instead of being rejected. The real manager's ClampFraction converts that NaN to zero, and the subsequent math.Abs(observed-soc) > socTolerance comparison also evaluates false, causing the box to report the command as applied after resetting the car's estimated charge to empty and replanning from it. Validate with units.ValidFraction or an explicit finite check before accepting the command.
Useful? React with 👍 / 👎.
| } else { | ||
| // Any other edit gets the HTTP route's background nudge. | ||
| go a.mpc.ReplanWithReason(context.Background(), "loadpoint_target_changed") |
There was a problem hiding this comment.
When surplus_only changes from false to true, the replan runs in a goroutine, but loadpointSurplusOnlySet immediately calls settleAndReport, which sends Plans.Latest() before that replan normally finishes. There is no plan-completion push elsewhere—plans are sent only here or in response to plan.get—so the app can receive and retain the pre-toggle plan even though the result says the setting was applied. Complete the replan synchronously or arrange to push the newly completed plan.
Useful? React with 👍 / 👎.
| func (a *appLoadpoints) SetSurplusOnly(id string, v bool) (bool, bool) { | ||
| prev, ok := a.mgr.SetSurplusOnly(id, v) |
There was a problem hiding this comment.
When either new operation changes manager state, the revision in subsequent snapshots does not move: appSite.Snapshot derives ControlRev exclusively from revision.Observe(a.ctrl), while these writes touch only the loadpoint manager. Consequently, after one client changes SoC or PV-only state, another client can submit a command using the pre-change expect.rev and pass the conflict check, defeating the protocol's optimistic-concurrency guard for these operations. Incorporate the relevant loadpoint state into the shared revision or explicitly advance it on these mutations.
Useful? React with 👍 / 👎.
miravoss26
left a comment
miravoss26
left a comment
There was a problem hiding this comment.
Reviewed the box side of the contract-pair (paired with ftw-webapp#61, ftw-app#1).
What it does: adds loadpoint.soc.set and loadpoint.surplus_only.set as cmd ops so the app can correct a car's charge level and toggle PV-only charging, the way the box's own UI already does through loadpoint.Manager. Both route through the existing HTTP handlers' logic (re-anchor + replan), and the corresponding POST /soc / POST /target routes now carry Via(...) so passthrough refusals name the op.
Correctness:
- SoC read-back tolerance (half a permille) and the unplugged→
E_UNAVAILABLE/reason:unpluggedpath both mirror the HTTP handler; good test coverage for both the happy path and the "rival writer" superseded case. surplus_onlyoff → synchronous replan (car may now import), any other edit → async — matches the existing HTTP-route asymmetry, and it's called out clearly in the comment.- Both ops deliberately skip the dispatch gate while still requiring
ftw.dispatch.writescope — reasoned in-line (state the box holds, not a plan-executing action) and consistent with howhold/boostare scoped.TestAStaleMeterDoesNotBlockALoadpointSettingbacks this up directly. - Test coverage is thorough: expired lease, unknown loadpoint, viewer scope, malformed args (out-of-range soc, wrong types), no-loadpoint box, dispatch-blocked — all extended to the two new ops via the existing table-driven tests rather than copy-pasted.
Security screen: no secrets, no new deps, no new network destinations, scope matches sibling ops (ftw.dispatch.write), authz unchanged (viewer still refused pre-manager-touch per the test).
Nothing to flag — safe to merge from my read. Merge-order note in the body (webapp → this → ftw-app) is for whoever merges, not a review concern.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 736e277. Configure here.
| CmdID: cmd.CmdID, | ||
| State: CmdSuperseded, | ||
| Observed: &Observed{Value: observed, Src: ObservedSrcCore, UptimeMs: readAtMs}, | ||
| } |
There was a problem hiding this comment.
Clamped SoC read-back marked superseded
Medium Severity
After a successful SetSoC, the result compares the requested fraction to ObservedSoC and treats any gap above half a permille as a rival writer. The manager itself can create a much larger gap: it clamps a negative plugin anchor to 0, so the stored level becomes session delivered energy over capacity rather than the value just written.
Reviewed by Cursor Bugbot for commit 736e277. Configure here.
miravoss26
left a comment
miravoss26
left a comment
There was a problem hiding this comment.
Follow-up to my review above: Bugbot's finding (ev.go#L364-L372, "clamped SoC read-back marked superseded") is real, I traced it through loadpoint.go.
reanchorSoCLocked does sessionPluginSoC = ClampFraction(soc - delivered). When the requested soc is below the fraction of capacity already delivered this session, that clamps to 0, and the resulting currentSoC becomes delivered/capacity, not the requested soc — a gap that can be far bigger than the half-permille tolerance loadpointSoCSet checks.
The existing HTTP route (handleLoadpointSoC) never surfaces this because it just returns {"ok": true} with no read-back. This cmd path is the first caller that reads back and compares, and it's the wrong label for what happened: CmdSuperseded reads as "a rival writer moved it," but a clamped self-correction isn't that — nothing else touched it. Observed does carry the real value, so the app isn't lied to about the number, but the state name misdiagnoses the cause.
Narrow trigger (correcting SoC down to below the energy already delivered this session), not a regression from main (the clamp is pre-existing, silent-only), and doesn't block a functioning correction — but worth a human call on whether the CmdSuperseded label should be scoped to "the read-back moved after our own write settled" rather than "differs from what we asked for," so a legitimate clamped correction doesn't read as a race. Not walking back "safe to merge" for the PR as a whole; flagging so it's a deliberate ship, not a missed catch.
2 participants
Contract-pair: srcfl/ftw-webapp@59-ops-soc-surplus
Refs srcfl/ftw-webapp#59.
What changed
Two command operations the app can send where the passthrough refuses the HTTP routes. Both are on
ftw.dispatch.write, likeloadpoint.holdandloadpoint.boost.loadpoint.soc.set—{ id, soc },soca 0–1 fraction. Callsloadpoint.Manager.SetCurrentSoCthrough theLoadpointsport. Not plugged in is refused after the ack withE_UNAVAILABLEandargs.reason = "unplugged", the session's answer for the route's 409. The adapter replans synchronously with reasonloadpoint_soc_corrected, ashandleLoadpointSoCdoes since feat(ev): show the plan at plug-in, set the car's charge level without a button #1062, so the plan pushed after the result is already drawn from the corrected level. The read-back is judged within half a permille, because the manager re-anchors through the session's delivered energy and rounding is not a rival writer.loadpoint.surplus_only.set—{ id, surplus_only }. Callsloadpoint.Manager.SetSurplusOnly. When the flag turns off the adapter forces the same synchronoussurplus_only_disabledreplanhandleLoadpointTargetdoes; any other edit gets that route's backgroundloadpoint_target_changednudge. The result reads back the flag the box now holds: 1 on, 0 off.dispatchWrite: false), for the mode's reason: each is state the box holds, and the plan it reshapes still meets the gate before anything moves. Refusing a corrected level while a meter is sick would keep the plan wrong for exactly as long as the box cannot act on it. Both still carry the dispatch scope, so a viewer is refused before the manager is touched.POST /api/loadpoints/{id}/soccarriesVia(appproto.OpLoadpointSoCSet)andPOST /api/loadpoints/{id}/targetcarriesVia(appproto.OpLoadpointSurplusOnlySet), so the passthrough'sE_USE_CMDnames the command. The routes stay as they were on the LAN. The target route's mark is honest about its limit: the op covers thesurplus_onlyfield only; the target level and its deadline still have no command.contract/registry.yamlis the byte-identical copy of the app's;go/internal/appproto/contract_gen.gois regenerated from it withgo generate."ftw": patch).Why
The box's own page has both controls; the remote client has neither. The passthrough refuses
Actuateroutes by design — an HTTP request has no expiry and must never move energy — and until now these two routes had nocmdop to point at, so the app gotE_USE_CMDwith noopand drew nothing. This is the contract and the box side only; the client draws the controls in a later PR.Verification
cd go && go test ./internal/appproto/ ./internal/api/ ./cmd/ftw/ -count=1: all pass.go vet ./...: clean.make verify: vet, tests, build, compose migration and container-boundary checks all clean.go/internal/appproto/ev_test.go: ack-then-read-back for both ops, the unplugged refusal, the half-permille read-back rule, a read-back that disagrees is superseded, both ops applied while dispatch is blocked, and both ops added to the shared tables (expired, unknown id, viewer, no-loadpoint box, malformed arguments, plan push after applied).go/internal/api/api_passthrough_test.go: the two routes are refused withE_USE_CMDnaming their op.go/cmd/ftw/app_link_test.go: the adapter against a realloadpoint.Manager— unplugged refused, plugged-in re-anchored and read back within tolerance, the flag flipped both ways with the previous value returned. The replan calls are not observable there (anmpc.Servicehas no test constructor); they mirror the HTTP handlers line for line.cmpagainst the app's and the native app's copies of the registry: identical. The app'sscripts/check-contract-drift.mjsrun against this branch's copy: byte for byte the same.Paired with
Merge order
Both contract jobs read the
Contract-pair:line at the top of the PR body and compare against that branch instead of the default branch, so both CIs are green while all three PRs are open. Merge the srcfl/ftw-webapp PR first, then this one, then srcfl/ftw-app. If the app's branch is deleted when its PR merges, drop theContract-pair:line from this body (or repoint it tomain) before the contract job reruns;maincarries the change by then.🤖 Generated with Claude Code
Note
Medium Risk
Changes EV charging state and planner inputs over the remote command path; mistakes could mis-report SoC or grid/PV charging intent, though behavior mirrors existing HTTP handlers and is heavily tested.
Overview
Adds
loadpoint.soc.setandloadpoint.surplus_only.setso the mobile app can do what the box UI already does: re-anchor the car’s charge level and toggle surplus-only (PV-only) charging over thecmdchannel instead of blocked HTTP actuation.The
Loadpointsport andappLoadpointsadapter call the sameloadpoint.Managerpaths asPOST /api/loadpoints/{id}/socand thesurplus_onlyfield onPOST /api/loadpoints/{id}/target, including MPC replans (loadpoint_soc_corrected, synchronoussurplus_only_disabledwhen PV-only turns off). Handlers ack, read back observed state (SoC within half a permille; surplus as 0/1), and map unplugged SoC correction toE_UNAVAILABLEwithreason: unplugged. Both ops useftw.dispatch.writebut skip the dispatch gate so stale meters do not block correcting state/plan inputs.contract/registry.yamland generated registry constants are updated; those HTTP routes getVia(...)so passthroughE_USE_CMDnames the op.Reviewed by Cursor Bugbot for commit 736e277. Bugbot is set up for automated code reviews on this repo. Configure here.