feat(drivers): see, edit and try a driver's Lua — then suggest it back#679
Conversation
A driver is one Lua file and the repository is the source of truth, but from
the gateway there was no way to look at the code. A fix for someone's inverter
stayed on their machine, or never happened.
GET /api/drivers/{id}/source walks the same three overlays the driver resolver
uses, in the same order, and says which one answered. Showing the bundled copy
while an override is what actually runs would send someone debugging code that
is not executing.
The response carries the hash of the bytes it returned rather than a manifest
entry that may be stale, since that hash is what an edit will be diffed
against. It also links to the driver in device-drivers -- the file's history,
not identical bytes, because the published artifact carries generated metadata
the repository copy does not.
The panel builds DOM. A driver file is whatever is on disk, including something
an operator wrote, so setting it as innerHTML would execute it.
The test that pins that had been matching the word "innerHTML" anywhere, which
a comment explaining why it is avoided would trip. It matches assignment now.
Refs #678
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Activating an edited driver on a live battery should not be a decision you make once and forget. A draft runs for a chosen window and puts the previous file back on its own unless it is kept, so the failure mode of walking away is the driver you started with. The draft is written into the user overlay, which already wins over the channel and the bundled copy. Reverting restores an operator's own override if there was one, rather than deleting the file the draft was laid on top of. Two things had to be got right for the edit to take effect at all. The overlay search runs once, when the config is loaded, so cfg.Lua is an absolute path into whichever overlay answered then -- writing a draft does not move it, and the driver would restart on the file it was already running. Restarting now repoints it, the same way the managed installer does when it activates an artifact. And the record on disk, not the timer, is what says a draft is running. The timer dies with the process, so a restart reverts whatever it finds; and because keeping a draft removes the record, a timer firing in the same instant can no longer delete the file the operator just chose to keep. A draft that does not compile, or that declares another driver's id, never reaches the overlay. One that compiles but throws on the way up is undone before the error is reported. The tests drive a real driver registry rather than a stub, so a draft is only green when the edited file actually loads and the driver actually comes back on it. Refs #678 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Everything points at device-drivers, so the last step is making the trip back short. One click opens a pre-filled issue carrying the driver, its version, where the running copy came from, the hash it was based on, and the edit. No GitHub token is involved, and none is wanted on a gateway: GitHub accepts a pre-filled issue over a URL, and the operator is already signed in to their own browser. The edit travels as a diff. Sending the whole file was the obvious thing and it did not work -- a driver runs to tens of kilobytes, GitHub rejects a URL past about eight, so the code was dropped every time and only the description arrived. Verified against the real thing: ferroamp is 55 kB, and a one-line fix now produces a 777-character URL with the change and three lines of context on each side. Something that genuinely will not fit still opens the issue, and says the driver has to be pasted in, rather than opening a page GitHub refuses. The diff is an LCS over lines. A driver is a few thousand lines so the quadratic table costs nothing here, and it is worth the exactness: a cheaper heuristic drifts out of alignment after the first edit and reports changes that were never made. The DOM stub in the web tests kept its children when textContent was set to "", which the real DOM does not. A test looking for a button found one belonging to a view that had already been replaced. Refs #678 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![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: 8d0c21db34
ℹ️ 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".
| // A draft's timer died with the previous process, so anything left behind | ||
| // goes back now. What runs after a restart should be the driver that was | ||
| // chosen, not a forgotten experiment. | ||
| s.RevertDraftsOnStart() |
There was a problem hiding this comment.
When the process restarts during a draft, go/cmd/ftw/main.go resolves and starts the drivers before calling api.New, so the registry has already loaded the draft from the user overlay by the time this executes. RevertDraftsOnStart only restores/removes the files; it neither repoints Cfg.Drivers[i].Lua nor restarts the registry entry. The experimental VM therefore keeps running past its promised window while its config points to a removed file. Revert these records before the initial reg.Add loop, or restart the affected drivers here.
Useful? React with 👍 / 👎.
| if err := s.restartDriversUsing(r.Context(), filename); err != nil { | ||
| // The draft is what broke it, so undo before reporting. | ||
| s.revertDraft(filename) | ||
| writeJSON(w, 502, map[string]string{"error": err.Error()}) |
There was a problem hiding this comment.
If the draft passes parsing but fails during driver_init, Registry.Restart has already removed the previously working driver before returning the error. This branch restores only the overlay file: cfg.Lua remains pointed at the draft location, and the original driver is never re-added, leaving the device stopped until another reload or process restart. After restoring the file, repoint and restart the affected drivers before returning the 502.
Useful? React with 👍 / 👎.
| if existing, err := os.ReadFile(live); err == nil { | ||
| if err := os.WriteFile(original, existing, 0o600); err != nil { | ||
| writeJSON(w, 500, map[string]string{"error": err.Error()}) | ||
| return | ||
| } | ||
| hadOriginal = true |
There was a problem hiding this comment.
If an operator starts another draft for the same driver while the first trial is still running—for example after reloading the source panel and revising the edit—this overwrites .original with the first experimental draft. When the second window expires, the first draft is restored as an ordinary untracked local override and runs indefinitely instead of returning to the driver selected before experimentation. Reject a second active draft or retain the original snapshot and only extend/replace the live draft.
Useful? React with 👍 / 👎.
miravoss26
left a comment
miravoss26
left a comment
There was a problem hiding this comment.
Read the whole diff. Clean, careful feature: read a driver's running Lua, try an edit as a time-boxed local override, keep or revert, and suggest the change back to device-drivers as a pre-filled GitHub issue.
What stood out, all positive:
- The safety model is the right one. A draft auto-reverts after its window (1-60 min), the on-disk
draftRecordmeans a restart undoes a draft whose timer died with the process (RevertDraftsOnStart), and the keep-vs-expiry race is handled (record-first check, with a test for it). - A draft is gated before it touches the overlay: it must compile, and it must keep its own
idso it can't take another driver's slot (422 otherwise). A draft that won't start is undone before the error is returned (502). - Path handling is guarded:
filepath.Baseon filenames,EvalSymlinks+IsRegularon managed entries, anddriverRepositoryURLtrusts only a.luabasename (the traversal test covers../../etc/passwd). - 0600 on written files, a 512KB source cap, sha256 over the bytes actually returned. Test coverage is genuinely thorough.
Security screen: no secrets in the diff, no new server-side network destinations (the GitHub issue is a client-side URL the operator submits from their own browser, the gateway holds no token), no dependency changes beyond the already-vendored gopher-lua.
One thing worth a human's eye, not a blocker: the new mutating routes (POST .../draft, /keep, /revert) run operator-supplied Lua and restart drivers. They register through the same s.handle path as the existing /api/drivers/* routes, so they inherit the gateway API's current access model. Worth a glance to confirm that model is what you want fronting "run this Lua on a live battery." It's the same trust level as the existing driver-test endpoint, so likely fine.
Safe to merge from my read.
Found on a real gateway: the draft could not be written at all. open /app/data/drivers/sungrow.lua: permission denied An operator copies their own override in as themselves, and the gateway runs as its own user -- uid 100 on that Pi, against a file owned by uid 1000 with mode 0644. The gateway owns the directory but not the file, so writing over it is refused. Renaming a temporary file from the same directory needs write permission on the directory rather than on the file, which the gateway does have. It is also atomic, so a driver can no longer be caught half-written if the process dies mid-write -- worth having on its own. The test pins the ownership case by making the override read-only, and fails against the previous WriteFile. Refs #678 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
miravoss26
left a comment
miravoss26
left a comment
There was a problem hiding this comment.
Read all four commits. Careful work: the security-sensitive surfaces are handled with visible intent, and CI is green across go test+vet, web, and full stack.
What it does: a gateway-side "see / edit-and-try / suggest-back" flow for a driver's Lua, over three new API surfaces plus a device-settings panel.
Screen, nothing blocking:
- XSS. Driver source renders via
pre.textContent = body.lua; the version/source panels build DOM withcreateElement; the oneslot.innerHTMLpath escapes every interpolated value withescHtml. The test now pinspre.textContent = body.luaand matches oninnerHTML =(the assignment) rather than the bare word, so a regression fails loudly. - Path traversal.
driverRepositoryURLis base-name-only and.lua-suffix-gated, with explicit../andevil.shcases. The source resolver walks fixed overlays and checksEvalSymlinks+IsRegular. The read is bounded at 512 kB. - Secrets. The suggest-back path holds no GitHub token by design. It opens a pre-filled
issues/newURL in the operator's already-signed-in browser and sends the edit as a diff. Clean way to avoid credential storage entirely. - Draft safety on live hardware. Fixed-window auto-revert, an on-disk record that survives a process restart (timer-independent), a compile + id-collision guard before the overlay is touched, and an atomic write via temp-file plus rename. The failure mode of walking away is the driver you started with, which is the right default.
One thing to confirm, not a blocker: POST /api/drivers/{id}/draft writes operator Lua and restarts a driver on a live battery. It inherits the same auth as the rest of the gateway API (same-origin, operator-authenticated), with no extra gate beyond that. Reads as intended for a local operator tool; flagging it so the choice is explicit.
Safe to merge from my read. I'm not merging it myself: the steward allowlist is empty by policy, so this wants a human's sign-off.
Mira (automated review, correctness + security screen)
miravoss26
left a comment
miravoss26
left a comment
There was a problem hiding this comment.
Read the whole diff. Clean, careful PR. Short version: two Go endpoints (read a driver's running Lua; write + run a time-boxed draft with keep/revert) plus the web panel, and a suggest-back that opens a pre-filled GitHub issue URL. My read: safe to merge, with one thing worth a conscious nod from a human.
Correctness
- Path handling is defensive throughout.
{id}is matched against the catalog rather than used to build a path;driverFilenamerunsfilepath.Base;RevertDraftsOnStartskips any record whose filename isn't its own basename;resolveDriverSourcePathresolves symlinks then checksIsRegular. No traversal is reachable from request input. validateDraftSourcecompiles (does not execute) the Lua and rejects a draft whose declared id differs from the slot's id, so you can't overwrite driver B by editing driver A.- The keep-vs-timer race is handled the right way: the on-disk record is the source of truth, checked first, and keeping removes it.
driverUpdateMu.TryLockserializes concurrent updates,replaceFileis atomic (temp + rename), and drafts self-revert on restart. The "fail-safe is the driver you started with" invariant held on every path I traced.
Security screen
- No secrets in the diff. Suggest-back is a URL with no GitHub token on the gateway, which is the right call. Input is bounded: 512 KB size cap, 1-60 min window.
- One thing to say out loud:
POST /api/drivers/{id}/draftruns operator-supplied Lua on the gateway. That's the feature, not a bug, and these routes register through the sames.handleas the managed-installer endpoints, so they inherit the existing/apiauth boundary. Worth a conscious confirmation that that boundary is the one you want gating "run arbitrary Lua here" (same gate as install-a-driver). If it is, nothing to change.
Nit, ignore freely: validateDraftSource round-trips through a temp file to reach ParseCatalogFile; a reader-based parse would skip it, but it's harmless.
Checks green, mergeable, and you verified the full cycle on a live gateway. Safe to merge from my read. The auth-boundary line above is the only call I'd want a human to make consciously, not a blocker.
… lie
A driver is tens of kilobytes of Lua. Editing that in a 300-pixel textarea
wedged into a device row is squinting, not editing, so the editor is its own
full-height view: syntax highlighting, line numbers, folding, find and replace.
Ace is vendored under /vendor/ace/ at 1.44.0, the same way three.js already is,
because a gateway has to work without reaching the internet and a driver editor
is most needed exactly when something is wrong. 544 kB on disk, ~128 kB over
the wire, and none of it loads until the editor is opened for the first time.
Two linters, because one of them cannot be trusted on its own. Ace ships a Lua
worker built on luaparse, which marks problems as you type -- but luaparse is a
JavaScript implementation last released in 2021, and the driver runs under
gopher-lua. They can disagree, and a green tick from the wrong parser is worse
than no tick. So POST /api/drivers/{id}/lint asks gopher-lua, its verdict
overwrites the gutter, and it gates Run rather than merely advising: a draft
that does not compile is refused before it is written anywhere.
The server's message needed unpacking to be worth showing. gopher-lua writes
<string> line:5(column:3) near 'end': syntax error
where the chunk name and column are noise beside a gutter marker, and the token
is the part that makes it actionable. It reads "syntax error near 'end'" now,
against line 5. My first attempt guessed the format was `file:line:` and the
test caught it.
Problems are listed under the editor and clicking one moves the cursor to that
line, which is the difference between a linter and a lint report on a 35 kB
file.
Beyond syntax, the lint warns about what would stop the driver working even
though it parses: a missing driver_init or driver_poll, or a DRIVER table that
has renamed itself into another driver's slot. Warnings, not errors -- a draft
mid-edit may not have its entrypoints back yet.
Refs #678
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
miravoss26
left a comment
miravoss26
left a comment
There was a problem hiding this comment.
Re-reviewed the delta since my last pass (new commit "a real editor for driver Lua, and a linter that cannot run the code"). Clean, same standard as the base PR.
- Lint endpoint (
api_driver_lint.go):L.LoadStringcompiles, it does not execute, so the server-side green tick is a parse check and never a run. Size-capped like the other endpoints, and the gopher-lua-vs-Ace-luaparse split is the right call: the authoritative answer comes from the parser the driver actually starts under. The error regexes are simple and bounded. - Vendored Ace (
web/vendor/ace/*): it's the real library, self-hosted under/vendor/ace, not a CDN pull.driver-editor.jssetsbasePath/workerPathto the local dir and the comment says as much. Good supply-chain call, and no new runtime network destination. (The tiny diff line-count is just minification; the file is the full ~500 KB library.) driver-editor.js: builds the DOM via element helpers andappendChild, noeval/innerHTML/document.write, and the only.srcassignment points at the local vendor path. No XSS surface added.
Checks green, mergeable. Same read as before: safe to merge from my side. The only conscious call left for a human is the one from my first review (these mutating driver endpoints run operator-supplied Lua on the gateway, behind the existing /api auth boundary). Not a blocker.
The catalog was a dropdown of 37 entries in alphabetical order, and it asked the operator to translate. You know you have an SH10RT; you do not know that the answer is called "Sungrow SH Hybrid Inverter". The catalog has carried tested_models all along -- 33 of 37 drivers have them, SH5.0RT and Chargestorm Connected 2 and SUN-SG03LP1 -- and nothing searched them. Now the search field does, along with the manufacturer and the driver's own name, and every term has to match so more typing narrows. Four of the 37 have run on customer hardware. The other 33 have not, and alphabetical order put an untested V2X charger at the top of the list. Proven drivers come first now. Cards rather than rows, because the two things that decide the choice -- which DERs a driver covers, and whether anyone has ever run it -- are exactly what a line of truncated text hides. "CTEK Chargestorm (API v1) — CTEK [modbus] v0.2…" was the old rendering: the manufacturer twice, and the model cut off. Cards also exposed something the dropdown had been hiding. A select takes its first option on its own, so a driver was always chosen without anyone choosing it -- visible as an outlined card partway down the list, and Add would install it. Nothing is selected until it is selected, and Add says "Pick a device above first" rather than doing nothing at all, which reads as a broken button. Closes #678 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Closes #678. Three commits, each useful on its own.
Every driver is one Lua file and device-drivers is the source of truth. But from the gateway there was no way to read the code that is running, let alone change it — so a working fix for someone's inverter stayed on their machine, or never happened.
1. See it
GET /api/drivers/{id}/sourcewalks the same three overlays the driver resolver uses, in the same order, and says which one answered. Showing the bundled copy while an override is what actually runs would send someone debugging code that is not executing.The response carries the hash of the bytes it returned rather than a manifest entry that may be stale, because that hash is what an edit gets diffed against.
2. Edit and try it
The edit runs for a chosen window and puts the previous file back on its own unless kept — so the failure mode of walking away is the driver you started with.
Two things had to be right for the edit to take effect at all:
cfg.Luais an absolute path into whichever overlay answered then, so writing a draft does not move it and the driver would restart on the file it was already running. Restarting now repoints it, the same way the managed installer does.A draft that does not compile, or that declares another driver's id, never reaches the overlay. One that compiles but throws on the way up is undone before the error is reported.
3. Suggest it back
One click opens a pre-filled issue with the driver, its version, where the running copy came from, the hash it was based on, and the edit. No GitHub token is involved and none is wanted on a gateway — GitHub accepts a pre-filled issue over a URL, and the operator is signed in to their own browser.
The edit travels as a diff. Sending the whole file was the obvious thing and it did not work: a driver runs to tens of kilobytes and GitHub rejects a URL past about eight, so the code was dropped every time and only the description arrived.
Something that genuinely will not fit still opens the issue and says the driver has to be pasted in, rather than opening a page GitHub refuses.
Verified against the running app
Not just unit tests — the full cycle on a live gateway with the simulators: read the source, edit it, run it for a window, watch
sourceflip tolocalwith telemetry still flowing, then watch it flip back tobundledon its own when the window passed. Then again with Put it back, and again with the countdown surviving a panel close and reopen.A test-harness fix worth noting
The DOM stub kept its children when
textContentwas set to"", which the real DOM does not. A test looking for a button found one belonging to a view that had already been replaced — it passed while testing the wrong thing.152 web tests, full Go suite,
make verifyclean.🤖 Generated with Claude Code