fix(drivers): make version switching work, and reversible#675
Conversation
Four faults, each hidden behind the one in front of it. The signed channel could not install anything at all. parseCatalogEntry only matched a DRIVER table written inline, but tools/ftw_repository.py assigns it from a local -- twice, so a driver cannot overwrite the identity it was signed under. Every signed artifact therefore parsed as empty metadata and validateLuaArtifact refused it with "driver metadata id/version @, want ferroamp@1.1.1". Update failed the same way; nobody noticed because nothing reached that far. The Versions panel from #672 rendered zero rows. GET /versions answers with VersionCandidate -- {repository_id, driver:{version}, installed?} -- and the picker read .version off the outer object, so the guard dropped every row. Its install call also omitted repository_id, which the endpoint requires. The test that was meant to cover this matched the source with a regex, so it passed against an empty panel. Rolling back the first managed install was refused: PreviousInstalledPath is empty, and the bundled driver is not an install to step back to. That is the common case -- installing over the bundled driver is the first thing anyone does when trying a new one. Rollback now removes the managed entry, which is what makes core resolve the bundled copy again, and the restart points config at the bundled path rather than the symlink it just deleted. The row itself said BUNDLED · V1.0.0 beside three buttons, where Rollback was a subset of Versions and bundled/managed/local is our vocabulary. Every driver runs locally; what matters is which version runs and what you can switch to. So the row now reads "v1.0.0 · official, shipped with this build · verified on hardware", the catalog's verification_status finally appears where updates are chosen, and one Versions list replaces Rollback. After a switch, Undo puts the old version back -- activating it when it is retained, rolling back when it is the bundled copy -- and re-arms the switch so trying again is one click. The web tests now drive the real code with the payload the API really returns. 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 the whole diff. Four faults, each fix lands where the fault is, and the tests now drive the real code instead of regex-matching the source (that the old web suite stayed green against an empty panel is the tell it was worth rewriting).
Correctness looks right:
extractDriverBlockkeeps the inline path, then followsDRIVER = <alias>to thelocal <alias> = {…}it names, and a dangling alias (DRIVER = missing_table) stays empty rather than borrowing a nearby table. The three catalog tests cover exactly those cases.- Rollback-from-first-install deactivates and points
targetPathat the bundledDriverDirwhenInstalledPath == "", consistent acrossmanager.goandapi_device_repository.go, with a test on the fall-back-to-bundled case. That's the case that actually matters (installing over the bundled driver is the first thing anyone does).
Security screen, clean:
- No secrets or keys in the diff.
- The dynamic regex is
regexp.QuoteMeta'd, so a driver-controlled alias name can't inject a pattern. - The web picker renders manifest fields via
createElement/textContent, neverinnerHTML, and there's a test asserting exactly that, so remote (signed but still remote) manifest input stays text. - No new runtime deps (the ed25519/httptest imports are test-only) and no new network destinations.
Not blocking, just noting: extractDriverBlock reads the first DRIVER = <alias> while the signed artifact assigns it twice (before and after source load). Same local both times today, and metadata is validated against the signed manifest anyway, so a first-vs-last mismatch just fails the install rather than smuggling an identity past validation. Worth knowing if the generator ever assigns different tables.
Safe to merge from my read. CI is green across go test+vet, web, and the driver contract.
![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: 26937e0e79
ℹ️ 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".
| if err := m.Deactivate(logicalPath); err != nil { | ||
| return state.DriverRepoInstall{}, err | ||
| } | ||
| return state.DriverRepoInstall{LogicalPath: logicalPath, DriverID: current.DriverID}, nil |
There was a problem hiding this comment.
When the first managed install is rolled back and restarting or validating the bundled driver fails, this deactivation leaves no active install for handleDeviceRepositoryRollback's recovery path: that handler calls Rollback again to roll forward, but the second call now fails at ActiveDriverRepoInstall. Because Registry.Restart has already removed the managed driver before attempting the failed bundled add, the request returns an error with the driver stopped instead of restoring the previously working managed artifact.
Useful? React with 👍 / 👎.
| function push(row) { | ||
| if (!row.version || seen[row.version]) return; | ||
| seen[row.version] = true; |
There was a problem hiding this comment.
When two enabled repositories publish different artifacts with the same version, deduplicating solely by row.version hides all but the first candidate. This can even present an uninstalled artifact as selectable while suppressing the active same-version artifact from a later repository, causing the button to install the wrong candidate; the backend explicitly distinguishes these cases by repository and SHA and requires the SHA when activating ambiguous versions, so the row key must retain that identity.
Useful? React with 👍 / 👎.
…ent undo An external review found that removing the Rollback button left no lasting way back. /versions lists signed and retained artifacts; the bundled copy is neither, so once a channel version was installed the only path back lived in an Undo button that vanished when the panel closed. Worse, the rollback-to-bundled path added in the previous commit could take a driver offline. Not every driver the channel offers is bundled, and pointing the restart at a file that is not there makes Registry.Restart remove the working instance before Add fails. The recovery path then called Rollback a second time, which had already deactivated the only active row and had nothing left to restore -- the "sql: no rows in result set" seen while testing. So going back to the bundled copy is now its own operation rather than a step inside Rollback. UseBundled checks the bundled file exists before deactivating anything, and returns the artifact it replaced so the caller can put it back if the bundled driver fails to start. Rollback returns to refusing a first install, which keeps its own recovery path intact. The version list carries a standing "the copy shipped with this build" row, so the way back survives closing the panel. Also from the review: the dedupe key was the version alone, which dropped one of two repositories publishing the same version with different hashes -- the hash is the artifact's identity, so it is the key. A row fetched from the channel now records that it is on disk, so a second attempt activates it instead of downloading again and failing offline. Undo picks its route from what was running rather than from a version number, since a managed artifact of the same version can sit on disk and differ in whether it may control anything. The summary line above the panel is rebuilt from the catalog after a switch rather than edited in place. Which version runs, its provenance, how well tested it is and whether it may control anything are all properties of the artifact now running; the telemetry-only badge in particular differs between versions of one driver, and it was stale in both directions. The DOM stub in the tests answered null from querySelector, which let a test pass while the code it covered did nothing. It resolves properly now, verified by breaking markRunning and watching the test fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Closes #674.
Four faults, each hidden behind the one in front of it. The first one meant nothing from the signed channel could be installed at all.
Nothing signed could be installed
parseCatalogEntrymatched only aDRIVERtable written inline. Signed artifacts do not write one:tools/ftw_repository.pybuilds the table as a local and assigns it twice, before the source loads and again after, so a driver cannot overwrite the identity it was signed under. That leftDRIVER = __sourceful_ftw_metadata, which the pattern misses.Every signed artifact therefore parsed as empty metadata, and
validateLuaArtifactrefused it:Update failed identically. Nobody had noticed, because nothing reached that far.
The Versions panel rendered nothing
GET /versionsanswers withVersionCandidate:{repository_id, driver:{version,…}, installed?}. The picker read.versionoff the outer object, soif (!version) returndropped every row. Its install call also omittedrepository_id, which the endpoint requires with a 400.The test meant to cover this matched the source with a regex —
assert.match(render, /body && body\.available/)— so it stayed green against an empty panel. The suite now drives the real code with the payload the API really returns.No way back from the first install
Installing a channel version over a bundled driver leaves
PreviousInstalledPathempty, and the bundled copy is not an install to activate by version. Rollback refused outright. That is the case that matters most: installing over the bundled driver is the first thing anyone does when trying a new one.Rollback now removes the managed entry, which is what makes core resolve the bundled copy again, and the restart points config at the bundled path rather than the symlink it just deleted.
The row spoke our vocabulary, not the operator's
It read
BUNDLED · V1.0.0beside Update, Rollback and Versions — where Rollback is a subset of Versions, andbundled/managed/localdescribes our storage, not their situation.Every driver runs locally. What matters is which version runs and what you can switch to. The row now reads:
verification_statushas been in the catalog all along and this view never showed it, so you could update onto an untested driver without being told. Rollback is gone; stepping back is picking an older row. After a switch, Undo puts the old version back — activating it when it is retained, rolling back when it is the bundled copy — and re-arms the switch so the next attempt is one click.An override still gets the list, since seeing what else exists is the point when you are testing your own driver, but its buttons say Download rather than Use this: a local file keeps winning either way.
Verified
Ran the full cycle against the real channel on a clean state: bundled v1.0.0 → fetch and switch to v1.1.1 → undo back to v1.0.0 → switch again, confirming telemetry each time. Both Go fixes have tests that fail without them.
make verifyclean; 125 web tests, full Go suite.🤖 Generated with Claude Code