Template consistency pass: close the require-hygiene class, tighten the shim boundary, drop lib_modules - #37
Template consistency pass: close the require-hygiene class, tighten the shim boundary, drop lib_modules#37derek-miller wants to merge 3 commits into
Conversation
…teeth DRV-97 declared the drivers-common-public globals every src/lib module calls, but two instances survived and the guard that should catch them could not see either. logging.lua calls tostring_return_period (global/lib.lua) with no requires at all -- the same defect DRV-97 fixed elsewhere, missed because the function is defined indented inside a `do` block and the guard's owner-extraction regex was anchored to column 0 (\nfunction), so it never entered the owner map. Six modules -- bindings, conditionals, events, http, persist, values -- call utils.lua globals (IsEmpty, InRange, toboolean, tointeger, TableDeepCopy) without requiring lib.utils. Same class one level in: src/lib depending on a sibling's globals via the driver's own driver.lua having required it. The guard only mapped drivers-common-public ownership, so intra-lib gaps were invisible. Fixes: - logging.lua requires global.lib; the six modules require lib.utils. - The guard's owner regex allows leading whitespace, so indented globals are seen; `local function` stays excluded. - The guard also maps src/lib-owned globals, so an intra-lib missing require is caught the same way. Proven by negative control: before the requires, the extended guard flags all six with the production nil-call message. - test_http_redact.lua stops hand-building a C4 surface and restubbing IsEmpty (wrong arity), InRange and tostring_return_period. It requires the shim and lib.http, which pull those in through their own requires -- the exact thing this class is about. Its InRange stub returned its argument unclamped, so the test was asserting against behaviour the real function does not have. Full suite green; guard 50/0.
Two boundary violations. The shim was paving over library code, and tests were supplying SDK the shim should own. - The shim defined the bare PersistGetValue/SetValue/DeleteValue globals, which belong to global/lib.lua. Its wrappers delegate to the C4:Persist* methods when they exist and otherwise route to their own store, so the shim's globals were overwritten and its store went dead the moment any module required global.lib. The shim now stubs the C4:Persist* methods; global.lib owns the globals, backed by them. - C4:GetDevices and C4:GetDeviceDisplayName were missing from the shim, so test_device_id_list and test_watched_variable each hand-rolled a project world. The shim now owns both, plus GetDeviceVariables, over a device registry a test fills with ShimSetDevices. GetDeviceDisplayName returns no value (arity 0) for an unlisted id, which is the DRV-95 case a driver must tolerate; the two tests inject their scenarios instead of defining the methods. - sleep/processEventLoop/runEventLoop were harness helpers without the Shim prefix, indistinguishable from controller APIs. Renamed ShimSleep / ShimProcessEventLoop / ShimRunEventLoop. No external callers. Left as-is, reviewed: test_websocket mutes C4:DebugLog (a no-op silence, not a reimplementation) and test_github_updater_alias spies on C4:FileSetDir by recording then delegating to the real shim impl. Neither reimplements SDK. Full suite green, DRV-84 and DRV-95 regressions included.
…ced it lib_modules gated which src/lib modules a repo rendered, but the gating was the entire source of the require-hygiene defect class: a module absent from a repo whose sibling needed its globals. gen-squishy bundles only what a driver requires -- a rendered-but-unrequired module is zero bytes in the .c4z -- so the question bought nothing at runtime and cost the whole defect class. Every one of the ten repos also answered it identically. Dropped. All five modules render unconditionally, alongside logging and utils which already did. github-updater stays gated on oss, its real condition. That removes every module-to-module gate from copier.yml; what remains is distributions and the optional vendor extras. Ripple, all in this commit: - CONTRIBUTING.md.jinja listed the libs conditionally on lib_modules, so an undefined variable would now break every render. Made unconditional. Its vendor list was also stale: json/deferred/drivers-common-public/xml became mandatory in DRV-97 but were still gated on tokens that no longer exist, so they silently dropped from the doc. Made unconditional too. - The CI matrix keyed three legs on lib_modules; those collapse without it. Reworked around the real remaining axes -- distributions and vendor_modules: default, oss-only, drivercentral-only, version-token, crypto-bundles. - CLAUDE.md and README.md described lib_modules as a question and carried the pre-DRV-97 vendor defaults. Also here, unrelated to the drop but same consistency pass: - vendor_modules help now lists the `version` token; it gated vendor/version.lua but was undocumented, so a non-oss driver needing it could not know to ask. - run_test.sh was an ESPHome integration-test runner (ESPHOME_TEST_* env, device flags, references to test files the template does not ship). Made it a generic single-test runner. Verified: five-leg render matrix, every module loads, make test and all three format checks green in each.
There was a problem hiding this comment.
Reviewed all four commits against a fresh clone at 58d7124; render matrix is green on every leg. Verified independently rather than trusting the descriptions.
1. require-hygiene (d2469f3). Ownership is unambiguous: the six globals (IsEmpty, InRange, toboolean, tointeger, TableDeepCopy) are defined only in src/lib/utils.lua, and tostring_return_period only in vendor/.../global/lib.lua at column 2 (indented in a do block), so comm finds zero name collisions between the two owner sets. That confirms the \n%s*function regex widening is both necessary (the col-2 def) and safe, since no dcp global gets its ownership stolen by a lib sibling. The six requires and logging's global.lib require are correct.
2. shim boundary (437bd05). Confirmed global/lib.lua routes PersistSetValue/GetValue/DeleteValue through if C4.PersistSetValue then C4:PersistSetValue(...), so stubbing the C4:Persist* methods and letting the library own the globals is the right layering; the old bare-global stubs really would have gone dead once any module pulled in global.lib. Device-registry stubs guard nil filters cleanly. The Shim-prefix rename is internal.
3. lib_modules drop (76f8085). The gate was the source of the whole defect class and every repo answered it identically, so dropping it is correct. copier.yml now has no module-to-module gating left, only distributions and vendor_modules. CONTRIBUTING.md.jinja lists all ten shipped src/lib files unconditionally (checked the rendered section against ls src/lib), the mandatory vendor entries are no longer gated on dead tokens, the version token is documented, and run_test.sh is cleanly de-ESPHome'd.
4. comment cuts (58d7124). Purely comment/blank churn; a diff filter finds zero non-comment code lines changed.
No defects found. Approving to clear the review gate. Auto-merge is not armed, so the merge is yours to trigger.
Closes DRV-97.
58d7124 to
76f8085
Compare
There was a problem hiding this comment.
Re-approving at 76f8085.
This push dropped the top commit 58d7124 (the comment-cull: comment-only, no behaviour change) and kept the three commits I verified earlier tonight:
- require-hygiene guard plus the intra-lib
require("lib.utils")fixes and the owner-map covering lib.utils globals. - test shim owning
C4:Persist*/GetDevicesinstead of stubbing the bare globals. - the
lib_modulescopier question drop, with CONTRIBUTING listing all tensrc/libfiles unconditionally.
The net diff is now a strict subset of what I approved (18 files, +188/-236 vs 22 files, +192/-291), so the earlier per-commit verification still holds. All five render legs are green at head.
Auto-merge is not armed, so this restores the gate to APPROVED without merging. The merge button is yours.
|
Approved gate, still unmergeable, and it is not a stale-checks artifact. Flagging the cause because it outlives this PR.
That list matches Two consequences worth separating:
The ruleset is repo settings rather than a tracked file, so nothing in the diff could have caught this and I cannot edit it with the token I have. Worth treating renamed CI job names as a ruleset change from here on, since the failure mode is a merge button that stays grey with a fully green matrix above it. |
A pre-rollout consistency pass over the whole template, from a two-agent audit (comments + consistency) verified by hand. Three commits, each its own concern; the render matrix is green on every leg at HEAD.
1.
fix(lib): close the require-hygiene defect class, and give the guard teethDRV-97 declared the
drivers-common-publicglobals eachsrc/libmodule calls, but two instances survived and the guard test couldn't see either:logging.luacallstostring_return_periodwith no requires at all — missed because it's defined indented inglobal/lib.luaand the guard's owner-extraction regex was anchored to column 0.bindings,conditionals,events,http,persist,values) callutils.luaglobals (IsEmpty,InRange,toboolean,tointeger,TableDeepCopy) withoutrequire("lib.utils")— the same class one level in, which the guard never checked because it only mappeddrivers-common-publicownership.Fixed both, plus the guard: its regex now catches indented defs, and it also maps
src/lib-owned globals so an intra-lib gap fails the same way. Proven by negative control — before the requires, the extended guard flags all six with the production nil-call message.test_http_redactstopped hand-building a C4 surface and restubbing library globals (itsInRangestub was even wrong-arity).2.
fix(test): keep the shim to the SDK, and stop tests reimplementing itPersist*globals, which belong toglobal/lib.lua; its wrappers overwrote them and the shim's store went dead wheneverglobal.libloaded. The shim now stubs theC4:Persist*methods; the library owns the globals, backed by them.C4:GetDevices/C4:GetDeviceDisplayNamewere missing, so two tests hand-rolled a project world. The shim now owns them plusGetDeviceVariablesover a device registry a test fills withShimSetDevices.GetDeviceDisplayNamereturns arity-0 for an unlisted id — the DRV-95 case — so the regression keeps its fidelity.sleep/processEventLoop/runEventLoop→ShimSleep/ShimProcessEventLoop/ShimRunEventLoop.3.
chore(template): drop thelib_modulesquestiongen-squishybundles only what a driver requires, so a rendered-but-unrequired module is zero bytes in the.c4z— the question bought nothing at runtime and was the entire source of the gating-defect class. Dropped; all five modules render unconditionally,github-updaterstays gated onoss. No module-to-module gating remains incopier.yml. Ripple synced in the same commit:CONTRIBUTING.md.jinja(would have broken on the undefined variable; its vendor list was also stale post-#32), the CI matrix (reworked arounddistributions+vendor_modules),CLAUDE.md,README.md. Also documented the undocumentedversionvendor token and de-ESPHome'drun_test.sh.Verification
Five-leg render matrix (
default,oss-only,drivercentral-only,version-token,crypto-bundles), each: everysrc/libmodule loads,make testgreen (308 assertions, 287 on non-oss legs which exclude the oss-gated alias test), stylua + black + mdformat clean. DRV-84 and DRV-95 regressions preserved.Closes DRV-97.