Repository navigation
fix(utils): the send wrappers no longer hand the controller a nil it refuses - #202
Conversation
…refuses
SendSecretToDevice passed a nil allowEmptyValues ahead of its false logCommand, and the controller refuses
any nil it is handed as a flag ("4th parameter must be a boolean value"), so every command it sent failed.
The wrappers now pass false, or "COMMAND" for a proxy message's type, when a later argument follows, and the
test shim refuses each argument the controller refuses, with its message.
![svc-finitelabs[bot]](https://avatars.githubusercontent.com/in/3141848?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes for one finding: the controller's nil-argument rule is written out in three files. The wrapper fixes and the stricter shim are right as they are.
Beyond CI, I ran test_secret_sends_trace.lua with main's utils.lua, which fails with the controller's 4th-parameter refusal, and against main's lax shim, which passes. Dropping the SendToDevice fill, dropping the SendSecretToProxy fill, or always filling the 4th argument each fails a named check.
| -- Measured on dev, against ids that do not exist: the controller checks each argument it is handed by its place, | ||
| -- so a nil before the last one, or a trailing nil passed on, is refused like a value of the wrong type. |
There was a problem hiding this comment.
This restates the rule c4_shim.lua:190-191 already gives the shim (each argument checked by place, a nil 4th or 5th refused), and test_secret_sends_trace.lua says it again at 87 and in the second sentence at 20-21. Keep it in the shim and drop the three test copies, as Home Connect #114 did at 92de7b5. That also keeps the test files closer to the hotfix for its next copier update.
There was a problem hiding this comment.
Done in e8a7d14. I dropped the rule from test_c4_shim.lua and from both places in test_secret_sends_trace.lua, so it's stated once, in the shim. I kept the spies' order (record the call, then pass it to the shim): a refused call still shows its arguments in the checks after it, where calling the shim first would leave sent empty and stop the run.
There was a problem hiding this comment.
The rule is now stated once, at the shim's own check, and e8a7d14 removes only the three repeated comment blocks: no code or assertion in either test file changes, so the mutation results from the first review still hold. Keeping the spies' record-then-forward order makes sense for the reason you gave.
When a caller left
allowEmptyValuesout,SendSecretToDevicehanded the controller(id, command, params, nil, false). The controller refuses a nil handed in place of a flag ("4th parameter must be a boolean value"), so Home Connect's arm and disarm commands from Apple Home failed in the field. The test passed anyway, because the shim'sC4:SendToDeviceaccepted anything.What I changed:
SendSecretToDevicepassesallowEmptyValues == true, andSendToDevicedoes the same whenlogCommandis given.SendToProxyandSendSecretToProxypass"COMMAND"as the message type whenallowEmptyValuesis given without one. A call with nothing after a left-out argument reaches the controller as it did before.C4:SendToDeviceandC4:SendToProxynow check each argument they're handed, by place, with the controller's own messages, as measured on a 4.3.0 controller. A device command needs a table 3rd and boolean 4th and 5th. A proxy message needs a string or table 3rd, a string 4th and a boolean 5th.test_secret_sends_trace.luaexpects a boolean 4th argument and runs each wrapper's left-out-argument shape through the shim.test_c4_shim.luapins each measured case.Home Connect is the only driver that calls
SendSecretToDevice. Its hotfix carries the same wrapper and shim lines, so its nextcopier updateleaves both files unchanged. The other drivers pick this up on the next rollout.