test(shim): add C4:ParseXml

[McGlothlin-Inc]

The shim had no C4:ParseXml, so nothing that parses XML under test could be exercised: a thermostatV2 driver receives its preset list as XML whose per-preset field values ride as escaped XML inside an attribute, and a test could only stub around it.

The shim now carries a small parser that returns what Director's does, a node with Name, Attributes (name -> value) and ordered ChildNodes. Attribute values are entity-unescaped so nested XML carried in an attribute is re-parsable; because markup inside an attribute is always escaped, scanning to the first '>' is safe. Prolog and comments are dropped. Same-name nesting closes at the matching depth. Both C4:ParseXml(xml) and C4.ParseXml(C4, xml) are accepted. No mixed content, CDATA or namespaces.

Fourteen assertions cover the preset-list shape, attribute quoting and entities, nesting, the two calling styles and the nil cases.

[svc-finitelabs]

Reviewed at 7349c17 with all 7 checks green. This is a comment, not an approval: the verdict and the merge are Derek's.

The port itself is clean. I diffed the 86-line implementation against the copy in finitelabs/control4-esphome#99 at 7b297f2 and they are byte-identical. The +89/-0 there versus +86 here is entirely the three-line "LOCAL PATCH, not yet in the template" banner, correctly dropped on the way over. template/test/c4_shim.lua is not in _skip_if_exists, so the template copy becomes authoritative on the next copier update. The suite runs 207 passed, 0 failed under luajit, and the 14 new assertions are as described.

Four things worth a look before this lands.

1. .Value is never set, and the repo's main consumer of C4:ParseXml reads it

template/vendor/drivers-common-public/global/handlers.lua:847 is ReceivedFromProxy, the proxy-command entry point Director calls on every driver that vendors this library:

local parsedArgs = C4:ParseXml(tParams.ARGS)
for _, v in pairs(parsedArgs.ChildNodes) do
  args[v.Attributes.name] = v.Value
end

CreateXML at lib.lua:581 confirms Value belongs to the shape. Its comment is "Create XML from Lua table formatted like the result of C4:ParseXml", and it branches on item.Value right beside Name, Attributes and ChildNodes.

The shim never assigns Value, so that loop yields nothing. Reproducing the consumer verbatim:

ARGS = <c4soap><param name="LEVEL">42</param><param name="MODE">HEAT</param></c4soap>

child .Name            = param
child .Attributes.name = LEVEL
child .Value           = nil
args populated         = 0 entries   (Director gives 2)

The failure mode concerns me more than the gap. On main today the same path raises attempt to call method 'ParseXml' (a nil value), which is loud and unmissable. With this PR it returns a well-formed node and every argument silently becomes nil, so a test driving a proxy command moves from "obviously unimplemented" to "passes with empty args". "No mixed content" is an accurate caveat, but this is the one consumer already vendored in the repo, so capturing text content for the element-free case looks better than documenting around it.

2. The "both calling styles" guard is dead, and the test covering it is vacuous

function C4:ParseXml(xml, ...)
  if type(xml) == "table" and xml == C4 then
    xml = select(1, ...)
  end

function C4:ParseXml(xml, ...) already desugars to (self, xml, ...), so the two styles named in the comment are the same call:

C4:ParseXml('<x/>')     -> node<x>
C4.ParseXml(C4,'<x/>')  -> node<x>
C4.ParseXml('<x/>')     -> nil      <- the style that actually breaks
C4:ParseXml(C4,'<x/>')  -> node<x>  <- the only style the guard catches

Deleting the three guard lines leaves 207 passed, 0 failed, including the assertion "the C4.ParseXml(C4, xml) calling style works". The guard's only effect is on C4:ParseXml(C4, xml), which passes C4 twice and which nothing writes. The dot call without a receiver, meanwhile, returns nil silently. Either drop the guard and its comment, or make it test self so it catches the case that actually fails.

3. > is legal unescaped inside an attribute value

The header justifies scanning to the first > "because markup inside an attribute is always escaped". That holds for < and &, which XML requires escaping. It does not hold for >.

<rule cond="a > b" other="z"/>    -> Name=rule  cond=nil  other=nil   (every attribute lost, node still returned)
<rule cond="a &gt; b" other="z"/> -> cond="a > b"  other=z

In practice this repo is safe, since XMLEncode at lib.lua:462 escapes > too, so anything from CreateXML or XMLTag round-trips fine. The exposure is XML originating outside that encoder, and the silent part is that the node still comes back, just stripped of all attributes. Worth restating that line as a constraint on accepted input rather than a proof of safety.

4. Numeric character references are decoded as bytes

string.char(code) is byte-oriented, so a code point is not a character:

é  -> a single 0xE9 byte (Latin-1), where XML means U+00E9 and Director returns UTF-8
’ -> ERROR: bad argument #1 to 'char' (invalid value)
'  -> left literal (hex refs unhandled, degrades cleanly)

Anything above U+00FF raises out of C4:ParseXml rather than degrading. Given how deliberately forgiving the rest of the parser is, a range check falling back to the literal would match its own behaviour.

None of this is reachable from the existing suite, which stays green. Items 1 and 2 are the ones I would want closed before merge; 3 and 4 are fine as documented constraints if you would rather keep the parser small.

One sequencing note for whoever merges: consumers pin the template by tag, and control4-esphome is on _commit: v0.9.24, which is exactly this PR's merge base. Merging to main therefore reaches nothing until a v0.9.25 tag is cut, so esphome#99 keeps its local copy until then.

[svc-finitelabs]

CI on 4cdc3a9 is not failing, it is waiting on a maintainer. The check suite came back action_required with latest_check_runs_count: 0, so no job ever started: this is a fork PR and GitHub is holding the run behind "Approve and run workflows" (run 34778896927). The first push, 7349c17, ran and went green, so approval is being asked per push rather than once for the contributor.

I have not clicked it. Approving a workflow run executes an outside contributor's code on the repo's runners, which is a maintainer gate, so it stays Derek's. Everything below was run locally instead, at 4cdc3a9, so the information is available without spending that approval.

The four review items are all closed

I re-ran the exact reproductions from my earlier comment against both 7349c17 and 4cdc3a9. The old head reproduced all four failures verbatim, which is what makes the new results meaningful rather than just green.

1. .Value, driven through the real consumer. Reproducing handlers.lua:847 (ReceivedFromProxy) verbatim rather than asserting on the node:

ARGS = <c4soap><param name="LEVEL">42</param><param name="MODE">HEAT</param></c4soap>
  7349c17 -> args populated = 0 entries
  4cdc3a9 -> args[LEVEL] = 42, args[MODE] = HEAT   (2 entries, matching Director)

The guard is scoped the way I would want: Value stays nil for a node with children, for <a/>, and for whitespace-only content, so it is not invented where Director has no text. Entity-unescaping applies to text as well, <a>4 &lt; 5</a> gives 4 < 5.

2. The dot call. The dead guard is gone and the replacement tests self, which is what actually breaks:

                          7349c17     4cdc3a9
C4:ParseXml('<x/>')       node<x>     node<x>
C4.ParseXml(C4,'<x/>')    node<x>     node<x>
C4.ParseXml('<x/>')       nil         node<x>   <- the style that was broken
C4:ParseXml(C4,'<x/>')    node<x>     nil       <- see below

3. Unescaped > in an attribute. Fixed for both quote styles, and for paired tags, not just self-closing ones:

<rule cond="a > b" other="z"/>   7349c17: cond=nil other=nil   ->  4cdc3a9: cond="a > b" other=z
<rule cond='a > b' other='z'/>   7349c17: cond=nil other=nil   ->  4cdc3a9: cond="a > b" other=z

The header comment was also corrected, it now describes the scanner instead of claiming the old scan was safe.

4. Numeric character references. Byte counts, since this is the whole point of the item:

é       7349c17: E9 (Latin-1)   ->  4cdc3a9: C3 A9        (U+00E9)
’      7349c17: ERROR          ->  4cdc3a9: E2 80 99     (U+2019)
'       7349c17: left literal   ->  4cdc3a9: 27
😀    7349c17: ERROR          ->  4cdc3a9: F0 9F 98 80  (astral plane)
� � �            ->  4cdc3a9: left literal, no error

Out of range, zero and surrogate halves degrade to the literal rather than raising, which is the forgiving behaviour the rest of the parser has.

The new assertions are not vacuous

Item 2 last time was a test that passed with the code deleted, so I mutation-tested the new ones rather than trusting the count. Each mutant reverts exactly one fix:

mutation	result
drop the dot-call guard	suite errors out at test_c4_shim.lua:670
never set .Value	214 passed, 9 failed
make the tag scanner quote-blind	222 passed, 1 failed
revert to byte-oriented string.char	221 passed, 2 failed

All four are caught. Baseline is 223 passed, 0 failed under luajit, up from 207. stylua --check with the CI flags is clean.

Scope of what I ran: the suite in place under luajit, not through a copier render, which is what CI actually exercises. Those files carry no Jinja, so I expect no difference, but the render is genuinely unverified until the workflow is approved.

Three small things, none blocking

• C4:ParseXml(C4, xml) now returns nil where it used to return a node. That is the trade I asked for and nothing in the repo writes that style, but it is a behaviour change rather than a pure addition, so it should be a deliberate yes.
• An empty element gives Value = nil, not "". <param name="X"></param> leaves the key absent from args rather than present and empty, so a consumer writing if args.X then branches differently than one writing if args.X ~= nil. I cannot check what Director returns here without hardware, so this is the one remaining unknown against real behaviour.
• goto is a 5.2 feature. Fine as written, since make test pins luajit in CI and in rendered driver repos, and the shim never runs on Director. Flagging only because the rest of this repo is careful about 5.1.

Comments, CDATA, the prolog, same-name nesting and quotes nested inside the other quote style all behave sanely, including the cases the rewrite could plausibly have regressed. An unterminated attribute quote returns nil for the whole document, which is loud enough.

Sequencing

My earlier note that this file is byte-identical to the copy in finitelabs/control4-esphome#99 is now out of date. That copy (blob 6c4ca28) is still exactly 7349c17 plus its three-line local-patch banner, so it carries all four defects; this branch has moved 121 lines ahead of it. Consumers pin the template by tag and control4-esphome is on _commit: v0.9.24, so merging to main still reaches nothing until a v0.9.25 tag is cut.

This is a comment, not an approval. The verdict and the merge are Derek's, and the workflow approval is his too.