build: ask the SemVer gate about the pins, not about git history - #709
Conversation
![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: 2b34d73615
ℹ️ 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".
| OLD_PIN_JSON="$(git -C "$ROOT" show "${BASE}:${PIN}" 2>/dev/null || echo '')" | ||
|
|
||
| if [ -z "$OLD_PIN_JSON" ]; then | ||
| echo "no ${PIN} at ${BASE}; nothing to compare" | ||
| exit 0 |
There was a problem hiding this comment.
When BASE is misspelled, unavailable in a shallow clone, or otherwise unreadable, this command converts every git show failure into an empty value and exits successfully as though the pin were being introduced for the first time. Consequently, the advertised make driver-versions-across-pin gate can silently skip all validation; only a confirmed missing BUNDLED_SOURCE.json should be treated as the first-pin case, while revision and Git errors should propagate.
Useful? React with 👍 / 👎.
| REPOSITORY="$(printf '%s' "$NEW_PIN_JSON" | jq -r .repository)" | ||
| SOURCE_DIR="$(printf '%s' "$NEW_PIN_JSON" | jq -r .source_dir)" | ||
| NEW_COMMIT="$(printf '%s' "$NEW_PIN_JSON" | jq -r .commit)" | ||
| OLD_COMMIT="$(printf '%s' "$OLD_PIN_JSON" | jq -r .commit)" |
There was a problem hiding this comment.
If a pin update also changes repository or source_dir, both the old and new commits are fetched from the new pin's repository and read through its source path because only OLD_COMMIT is extracted from the old JSON. Such a valid repository/layout migration therefore either fails to materialize the old snapshot or compares unrelated bytes; extract the old repository and source directory too and materialize each commit using its corresponding pin.
Useful? React with 👍 / 👎.
| - name: Require version bumps across the pin | ||
| run: bash scripts/check-driver-versions.sh "${{ needs.changes.outputs.base_sha }}" |
There was a problem hiding this comment.
In .github/workflows/test.yml, the drivers job is selected only for Lua files, BUNDLED_SOURCE.json, sync-bundled-drivers.sh, Makefile, or workflow changes; the newly added scripts/check-driver-versions.sh is absent from that classifier. A later PR changing only this gate will therefore skip this step and all of its related checks while the final required job still succeeds with skipped suites, so this script path should select the drivers job.
Useful? React with 👍 / 👎.
|
Retargeted to That means this PR currently carries two commits: #708's and this one. The dependency is unchanged — #708 should still merge first, and when it does this diff shrinks to the SemVer commit on its own with no action needed. Review the second commit, |
The gate diffs changed .lua files between two Git revisions of this repository. That works only while the drivers are committed here, and it was never quite the right question: what reaches a gateway is decided by where drivers/BUNDLED_SOURCE.json points, not by what this history shows. check-versions gains -old-dir/-new-dir, comparing two materialised snapshots instead of two revisions. ValidateDriverVersionChange is untouched -- it always took bytes rather than revisions, so the rule being enforced is identical. scripts/check-driver-versions.sh reads the pin at a base revision and the pin now, and stays quiet when they match: a pull request that does not move the pin cannot have moved any driver's bytes. When it has moved, both snapshots are fetched and every bundled driver compared. The Git-based mode stays for now. It still works while the files are committed, and removing it belongs with the deletion. Tested where it counts -- the failure case. Six table-driven cases in main_test.go: changed bytes under an unchanged version fail and name the driver, a bump passes, untouched files pass, a newly bundled driver has nothing to compare, a dropped one is a coverage decision rather than a version one, and every offender is reported rather than just the first. Also run against the real pin move in #704 (1f67078 -> 3f62a00), where pixii, solaredge_legacy and sungrow all changed with bumps: verified across 37 bundled drivers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
2b34d73 to
e6d0789
Compare
Brings the whole absent-register campaign into the recovery snapshot: every bundled driver now gives up on a register the device does not answer instead of paying a failed read for it on every poll. Generated by scripts/sync-bundled-drivers.sh. The SemVer gate added in #709 verified the move across all 37 bundled drivers. HELD: go test ./internal/drivers/ fails on this snapshot. TestSungrowZeroBatteryCommandForcesIdle expects a battery command issued straight after load to write idle; device-drivers #40 made the driver refuse a battery command unless the model named itself a hybrid or a battery register has answered, and neither is true before the first poll. That disagreement is a control-path decision and is not mine to settle -- see the pull request for both positions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
Brings the whole absent-register campaign into the recovery snapshot: every bundled driver now gives up on a register the device does not answer instead of paying a failed read for it on every poll. 497 addresses across 49 drivers, plus the control-path fixes that landed alongside. Pin moves 1f67078 -> edb32ff. Generated by scripts/sync-bundled-drivers.sh; nothing edited by hand. The SemVer gate added in #709 verified the move across all 37 bundled drivers. go test ./... passes, including the Sungrow zero-power case that held this back, and the full-stack e2e is green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com>
Brings the whole absent-register campaign into the recovery snapshot: every bundled driver now gives up on a register the device does not answer instead of paying a failed read for it on every poll. 497 addresses across 49 drivers, plus the control-path fixes that landed alongside. Pin moves 1f67078 -> edb32ff. Generated by scripts/sync-bundled-drivers.sh; nothing edited by hand. The SemVer gate added in #709 verified the move across all 37 bundled drivers. go test ./... passes, including the Sungrow zero-power case that held this back, and the full-stack e2e is green. Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
drivers/*.lua leaves git and is gitignored. The files still ship -- offline recovery needs them in the image, the release tarballs carry them, the tests read them -- but they are fetched from the commit pinned in drivers/BUNDLED_SOURCE.json rather than committed here. The point is not the bytes. It is that there is no file to open a pull request against. Two contributors spent real effort, including thirty minutes on an SE17K and register-by-register probing on a Pixii, writing fixes against files that regenerate. A guard that fails the pull request recovers the codebase but not the afternoon. Absence teaches faster than a red check does. Everything that needed the files in git already stopped needing them: #708 made the fetch a build step and proved a build works from the pin alone, #709 moved the SemVer gate onto the pins. This is the small change those two were for. --check changes its question. It used to compare committed bytes against the pin; there are no committed bytes now, so it asks whether any driver is committed at all. Cheaper than a content diff, a stricter rule, and it needs no network. Every workflow that reads drivers/ fetches first: core, drivers and e2e in test.yml, the image build in beta.yml, and both the tarball and image jobs in release-assets.yml. e2e, release and driver-repository-validate depend on drivers-present locally, so a missing fetch fails with the remedy rather than a confusing file-not-found. Verified: rm -rf the drivers, make drivers restores 37 of 37, go test ./... passes uncached, and the full-stack e2e is green. The guard is proven to fire -- git add -f on one driver and --check reports it. Signed-off-by: Fredrik Ahlgren <fredrik@sourceful-labs.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
Second step toward FTW holding no driver source. Stacked on #708 — merge that first; this targets its branch so the diff stays readable.
The problem
check-versionsdiffs changed.luafiles between two Git revisions of this repository. That works only while the drivers are committed here. Once they are fetched instead, there is nothing in the history to diff and the gate silently has no subject.It was never quite the right question either. What reaches a gateway is decided by where
drivers/BUNDLED_SOURCE.jsonpoints, not by what this repository's history happens to show.The change
check-versionsgains-old-dir/-new-dir, comparing two materialised snapshots instead of two revisions.ValidateDriverVersionChangeis untouched — it always took bytes rather than revisions, so the rule being enforced is identical. Only where the bytes come from changed.scripts/check-driver-versions.shreads the pin at a base revision and the pin now:The Git-based mode stays for now. It still works while the files are committed, and removing it belongs with the deletion.
Tested where it counts
Six cases in
main_test.go, and the first is the one the gate exists for:Also run against the real pin move in #704 (
1f67078→3f62a00), where pixii, solaredge_legacy and sungrow all changed with bumps:And with the pin left alone:
Where this leaves the cutover
drivers/*.lua, gitignore them, keep the pin.luamay be committed here"The remaining step is genuinely small now: both things that needed the files in git — the build and this gate — no longer do.
go vet ./...clean,go test ./cmd/ftw-driver-repository/ ./internal/driverrepo/green.🤖 Generated with Claude Code