fix(profiles): accept a limiter with no range, and refuse one with no value - #795
fix(profiles): accept a limiter with no range, and refuse one with no value#795ChampionDesigns wants to merge 5 commits into
Conversation
…e way on POST and PUT
Decal audit finding F-048, server half. A client arming a step limiter from
null sends `{"value": x}` with no range, and `StepLimiter.fromJson` read both
keys through `parseDouble`, which throws on null. POST answered 500 and PUT
answered 400 for the same bytes, because only `_handleUpdate` carried an
`on TypeError` arm.
Measured on the device, 29 Aug (captures kept verbatim as the two fixtures):
POST /api/v1/profiles -> 500 {"error":"Internal server error", ...}
PUT /api/v1/profiles/<id> -> 400 {"error":"Invalid request", ...}
both with the message `type 'Null' is not a subtype of type 'String'`, neither
naming the limiter.
`range` now defaults to 0.0 when absent or null, because range 0 is the DE1
wire's own encoding of "no falloff band" — every encoder writes
`data[2] = _encodeFlowPressure(limiter.range)` and the mock's `range <= 0`
branch already hard-clamps to `value` — so a rangeless limiter parses to
exactly what the user set, a hard cap, rather than to a band the server
invented.
`value` stays required and is now refused BY NAME, as is a garbage `range`:
defaulting a missing value to 0 would turn a malformed limiter into a silently
OFF one on the machine, which is the same silent degradation this file already
refuses for TransitionType.hold and ExitType.power. The new `on TypeError` arm
in `_handleCreate` is byte-identical to `_handleUpdate`'s and catches the
residual cases (a null transition, a non-string name), so the two routes now
answer identically for every malformed profile body.
`rest_v1.yml` moves `range` out of StepLimiter's required list and documents
the default. The request contract genuinely changed, and the spec is what the
next agent generates clients from; it stays required in practice on the way
out, because an omitted range is stored as 0 and every response carries it.
No profile id moves. The hash is computed on the parsed model, and the only
bodies whose parse changes are ones that previously threw and so never had an
id; `{value: x}` and `{value: x, range: 0}` now hash equal, which is correct
dedupe for identical machine behaviour. Two characterisation tests pin the
pre-fix hashes of a complete-limiter and a null-limiter profile and both still
pass unchanged.
Tests first, confirmed red: the fixtures reproduced the device's 500/400 split
before the fix. 3697 pass, 1 failure — `workflow_handler_test.dart`
"publishes each changed flow exactly once", the known wall-clock flake, which
passes 40/40 twice when run alone and shares no code with this change.
flutter analyze clean, dart format clean.
The sb-dev smoke was NOT run, deliberately: booting the app mounts
`device_discovery_view`, whose initState calls `connectionManager.connect()`,
and the desktop serial service probes USB ports on scan. Ben's machine is on
/dev/ttyACM0, so a smoke run would have opened the real hardware. The handler
tests cover the same surface with the real controller, handler and router.
Upstream carries the identical fault, so this is PR-able to Vid.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This lane is re-cut onto upstream/main so it can be a PR. Two of its tests carried the limiter on a ProfileStepPower, which is ben/profile-modes' own addition -- upstream's ProfileStep.fromJson refuses it with "Invalid step type. Must include either pressure or flow." The production change is in StepLimiter.fromJson and is independent of step type, so nothing is lost: - "a power step accepts one as a hard cap" duplicated the flow-step test directly above it, which stays. - "a power step still refuses a zero-value limiter" tested the POWER step's own mandatory-limiter rule, not the limiter parser. That behaviour belongs on ben/profile-modes and both tests should be re-added there. powerStepJson is removed with them; it had no other caller on this lane. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tadelv
left a comment
tadelv
left a comment
There was a problem hiding this comment.
Two required fixes before merge:
- The OpenAPI contract says an explicit
range: nullis accepted, but in OpenAPI 3.0.3 an optional numeric property is still non-nullable unlessnullable: trueis set. Please make the schema match the implemented/request-tested behavior. - Current
AGENTS.mdsays not to add explanatory comments to new/substantially rewritten code and to keep rationale/hardware/debug history in the matchingdoc/AI_*_NOTES.md. Please remove/move the new explanatory/debug-history comments (including the test-file header where applicable) while keeping the code/tests self-explanatory.
The behavioral direction otherwise looks sound: value-only limiters canonicalize to range 0, valueless limiters fail visibly, POST/PUT behavior is aligned, hash compatibility is covered, and CI is green.
| format: double | ||
| description: Acceptable range around the limiter value | ||
| default: 0 | ||
| description: >- |
There was a problem hiding this comment.
The description says explicit null is accepted, and the parser/tests do accept it, but this file is OpenAPI 3.0.3. Removing range from required only permits omission; it does not permit JSON null for a type: number property. Please add nullable: true (or stop documenting/accepting null) so the published contract matches runtime behavior.
| value: parseDouble(json["value"]), | ||
| range: parseDouble(json["range"]), | ||
| ); | ||
| // `value` is required: defaulting it would turn a garbage limiter into a |
There was a problem hiding this comment.
Current AGENTS.md explicitly says not to add explanatory comments to new/substantially rewritten code and to put rationale, hardware constraints, and debugging history in the matching doc/AI_*_NOTES.md. Please keep the validation readable through names/control flow and move/remove this rationale; the same guidance applies to the long explanatory block immediately below and the new test-file header.
Two review points. The published contract now matches the parser. In OpenAPI 3.0.3 dropping `range` from `required` permits omission only, so a `type: number` property still refuses an explicit null however the description reads. `nullable: true` says what the parser actually does. The explanatory comments this lane added to StepLimiter.fromJson, the test-file header and the handler-test group are gone, and the reasoning is in doc/AI_API_NOTES.md under "Profile Step Limiters" — where AGENTS.md puts rationale. The names and the control flow carry the meaning on their own. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tadelv
left a comment
tadelv
left a comment
There was a problem hiding this comment.
The two previous blockers are resolved: the OpenAPI schema now permits explicit null via nullable: true, and the implementation/test rationale was moved out of code into doc/AI_API_NOTES.md. Current CI is green.
One small documentation fix remains before merge: the new API note says POST /api/v1/profile, but the implemented endpoint is POST /api/v1/profiles. Please correct that path. I did not find any other merge-blocking issues in the updated diff.
| property from `required` permits omission only; without `nullable` the published | ||
| contract would still refuse the explicit null the parser accepts. | ||
|
|
||
| `POST /api/v1/profile` used to answer such a body with a 500 while `PUT` answered |
There was a problem hiding this comment.
This endpoint path is singular here, but the actual route is POST /api/v1/profiles. Please correct the note so the API guidance does not point readers at a nonexistent endpoint.
The endpoint is POST /api/v1/profiles, not /api/v1/profile, and the update path
it is compared against is PUT /api/v1/profiles/{id}. A note whose job is to stop
drift must not point at a route that does not exist.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-parsing # Conflicts: # doc/AI_API_NOTES.md
|
The path is corrected in Also brought current in As on the other branches I merged rather than rebased: this branch is published and reviewed, and my pre-push hook refuses a non-fast-forward. The three-dot diff is unchanged and still only this branch's own work. Verified after the merge: |
|
Reopening this because I think the remaining question needs to be answered explicitly on the Bengle/v2 firmware side rather than inferred from The PR currently treats a value-only limiter as Since Decaid serializes the limiter range into the extension frame, while Could we clarify the intended/actual semantics for v2 profiles on Bengle firmware and make those authoritative here? In particular:
Once that is established, I think we should make the encoder, I still agree with the useful part of this PR: a limiter containing |
2 participants
Summary
StepLimiter.fromJsondefaulted a missingvalue, which turns a garbage limiter into a silentOFF on the machine. It now refuses visibly — a
FormatException, which the REST boundary reportsas a 400.
rangebecomes genuinely optional. Absent or null means a hard cap atvalue, encoded asrange 0 — which is the DE1 wire's own encoding, and what a client arming a cap actually sends.
Base:
main. Independent.Design notes
valueis required andrangeis not, deliberately: the same policyTransitionType.holdandExitType.poweralready follow above it in the same file. A limiter with no value is not a limiter.A rangeless limiter hashes identically to one with an explicit
range: 0, so profile identity isunchanged for anything already stored.
Two tests were deliberately left out. They carried the limiter on a
ProfileStepPower, whichis added by the profile-modes PR and does not exist on
main. One duplicated the flow-step testbeside it; the other tested the power step's mandatory-limiter rule, not the limiter parser.
They should be re-added once both PRs land.
Linked Issue
N/A
Verification
flutter analyze— clean.flutter test— full suite 3914 passed / 1 skipped, run against currentmainon5 Sep 2026.
flutter test—step_limiter_test18 passed;test/modelsandtest/services/webserver506 passed.
dart format— clean on every changed file.Fixtures
test/fixtures/f048/e01_post_body.jsonande01_put_body.jsonpin the REST shape.Verified on hardware. This change ships in the Decaid-Canary build Ben runs on his own
machine, and has been exercised in normal use rather than only under test.
Impact
limiter.
assets/api/rest_v1.ymldocuments the rule.{"value": x}with no range now gets a hard cap, which iswhat it meant. A client sending a limiter with no
valuenow gets a 400 — that request never didwhat it looked like.
the limiter off.
Contributor Responsibility
AI-assisted development is allowed. The submitter remains responsible for the submitted work.