glenn schrooyenandClaude Opus 5
1ba35f9c99
checks / unit (pull_request) Successful in 6m19s
Review found `-c safe.directory={where}` and `-c safe.directory=*` set on the
same invocation, the specific value redundant once the wildcard is present.
Dropped the WILDCARD rather than the specific path, which is the opposite of
the suggestion and the tighter end of it: `_git()` always runs `-C where`, so
the only repository it ever reads is the one it was pointed at, and naming that
one path grants exactly the trust the tool needs. `*` granted trust to every
repository on the machine for the lifetime of each subprocess - harmless for
read-only `log`/`show` with no tty, as review measured, but broader than the
job.
Verified after the change: the documented container invocation still runs BOTH
halves and reports `no runtime change since the last bump` / `CLEARED for
deploy`, so the fix this branch exists for is intact.
Suite: 26 files, all pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU
307 lines
14 KiB
Python
307 lines
14 KiB
Python
"""PKG-09 - refuse a deploy that would be a no-op, or that the add-on would reject.
|
|
|
|
python3 tools/predeploy_check.py --deployed-version 0.7.0 --options live-options.json
|
|
|
|
⚠️ RUN IT WHERE THE ADD-ON'S DEPENDENCIES EXIST. `app/validate.py` imports its
|
|
siblings and those need aiohttp, so on a bare host the validator half SKIPS -
|
|
loudly, and exit 2, never a silent pass. To get both halves:
|
|
|
|
docker run --rm -v <repo>:/w:ro goodwe-addon-checks:1 \
|
|
python3 /w/goodwe_controller/tools/predeploy_check.py \
|
|
--deployed-version 0.7.0 --options /w/live-options.json
|
|
|
|
EXIT CODES. 0 cleared; 1 REFUSED, do not deploy; 2 NOT CLEARED - a check did not
|
|
run, and that is not the same as passing.
|
|
|
|
TWO FAILURES THIS ANSWERS, BOTH OF WHICH HAVE HAPPENED.
|
|
|
|
1. THE SILENT NO-OP. The Supervisor keys the built image by `version:` in
|
|
config.yaml. `ha apps update` against an unchanged version does nothing and
|
|
REPORTS SUCCESS. PKG-06 recorded this on 2026-09-05 with SIX merges behind
|
|
an unbumped version - its own commit message says so, and this line said
|
|
"four" through two review rounds; PKG-07 recorded it again on 2026-09-06 with six. Both
|
|
times a human noticed. Nothing else was ever going to.
|
|
|
|
2. THE REFUSED CONFIG. A config the shipped validator rejects is
|
|
`SystemExit(78)` - the add-on does not start. ⚠️ AND THE INGRESS PANEL GOES
|
|
WITH IT, because the add-on serving it is not running, so recovery means the
|
|
Supervisor UI. SAFETY-39's rule 10 made this reachable by a config that
|
|
started fine on the previous version: it refuses an explicit `slew_w` above
|
|
SLEW_RAIL_FRAC x the site rail, and 0.7.0's own shipped default of 1000
|
|
exceeds the 555.6 W ceiling on a 5000 W rail.
|
|
|
|
⚠️ WHY THIS IS NOT A CI CHECK. The obvious version gate - "a pull request that
|
|
touches runtime code must bump the version" - is the WRONG shape for this
|
|
project, which deliberately batches many merges into one deploy. It would demand
|
|
a bump per pull request, put every branch in conflict over one line of
|
|
config.yaml, and still not prevent the actual failure, which happens at DEPLOY
|
|
and not at merge. The check belongs where the mistake is made.
|
|
|
|
⚠️ WHAT THIS DOES NOT DO. It does not read the live box. The options blob is
|
|
reachable only through the Supervisor API, which needs a supervisor token, so
|
|
the operator exports it and passes `--options`. Without that flag the validator
|
|
half is SKIPPED and says so - a skipped check must never look like a passed one.
|
|
"""
|
|
import argparse
|
|
import json
|
|
import os
|
|
import re
|
|
import subprocess
|
|
import sys
|
|
|
|
HERE = os.path.dirname(os.path.abspath(__file__))
|
|
ADDON = os.path.dirname(HERE)
|
|
sys.path.insert(0, ADDON)
|
|
|
|
# Files that end up in the built image and can change what it DOES. A change to
|
|
# any of these needs a new `version:` or the Supervisor will not rebuild.
|
|
# CHANGELOG.md, DOCS.md, MUTATION.md, DECISIONS/, test_*.py, tools/ and
|
|
# scenarios/ are deliberately absent: they ship in the repo but cannot change
|
|
# runtime behaviour, and demanding a bump for a typo fix trains people to bump
|
|
# without reading, which is the habit this file exists to prevent.
|
|
RUNTIME = (
|
|
"goodwe_controller/app/",
|
|
"goodwe_controller/translations/",
|
|
"goodwe_controller/config.yaml",
|
|
"goodwe_controller/Dockerfile",
|
|
"goodwe_controller/run.sh",
|
|
"goodwe_controller/requirements.txt",
|
|
"goodwe_controller/build.yaml",
|
|
)
|
|
|
|
CONFIG_PATH = "goodwe_controller/config.yaml"
|
|
_VER = re.compile(r'^version:\s*"([^"]+)"', re.M)
|
|
|
|
|
|
def shipped_version(tree=ADDON):
|
|
with open(os.path.join(tree, "config.yaml"), encoding="utf-8") as fh:
|
|
m = _VER.search(fh.read())
|
|
return m.group(1) if m else None
|
|
|
|
|
|
def is_runtime(path):
|
|
return any(path == p or path.startswith(p) for p in RUNTIME)
|
|
|
|
|
|
def _git(*args, repo=None):
|
|
"""git, run so it answers regardless of who owns the checkout.
|
|
|
|
⚠️ WITHOUT `safe.directory` THIS TOOL SILENTLY DID HALF ITS JOB. The
|
|
invocation this file's own docstring documents runs the container as uid 0
|
|
against a checkout owned by the operator, and git refuses that:
|
|
|
|
fatal: detected dubious ownership in repository at '/w/goodwe-addon'
|
|
|
|
`_git` then returned non-zero, `last_version_change()` returned None, and
|
|
the no-op check reported SKIPPED - so **the silent-no-op half, which is the
|
|
reason this file exists, had never once run in its documented form.**
|
|
|
|
It degraded honestly: exit 2, INCONCLUSIVE, never a false clearance. But a
|
|
check that cannot fire is not a check, and the two documented environments
|
|
were mutually exclusive - in the container the validator ran and git
|
|
skipped; on a bare host git ran and the validator skipped on aiohttp.
|
|
Neither documented command ran both halves.
|
|
|
|
⚠️ AND IT WAS INVISIBLE IN THE ONLY CASE ANYONE HAD RUN. `main()` consults
|
|
the git result only when `shipped == deployed`; every real run so far
|
|
compared 0.7.0 against 0.9.0 and printed its verdict without needing git.
|
|
|
|
`test_version_gate.py` could not see it either: it invokes the gate with
|
|
`sys.executable` against fixtures it creates as its own uid, so no fixture
|
|
ever met the ownership mismatch. MUTATION.md habit 4, in a fourth shape,
|
|
inside the file written to prevent it.
|
|
|
|
`safe.directory` is the right fix rather than `--user`: it makes the tool
|
|
correct however it is invoked, instead of making every future caller
|
|
remember a flag. The read is read-only, on a repository the operator is
|
|
about to deploy from.
|
|
"""
|
|
where = repo or os.path.dirname(ADDON)
|
|
return subprocess.run(
|
|
["git", "-c", f"safe.directory={where}", "-C", where, *args],
|
|
capture_output=True, text=True)
|
|
|
|
|
|
def _version_at(sha, repo=None, path=CONFIG_PATH):
|
|
"""`version:` as of one commit, or None if the file or the key is absent."""
|
|
r = _git("show", f"{sha}:{path}", repo=repo)
|
|
if r.returncode != 0:
|
|
return None
|
|
m = _VER.search(r.stdout)
|
|
return m.group(1) if m else None
|
|
|
|
|
|
def last_version_change(repo=None):
|
|
"""The newest commit whose `version:` VALUE differs from its parent's.
|
|
|
|
⚠️ THIS USED TO BE `git log -S 'version: "'` AND IT WAS WRONG ON EVERY REAL
|
|
REPOSITORY. `-S` is the pickaxe: it finds commits that change the COUNT of
|
|
a literal string. A bump edits the line in place - `version: "0.7.0"` ->
|
|
`version: "0.8.0"` - so the substring still occurs exactly once before and
|
|
after, and `-S` never sees it. It found only the commit that first
|
|
INTRODUCED the line.
|
|
|
|
Measured on this repository: `-S` returned `0a1e61d` (2026-08-23, the
|
|
original packaging commit) as "the last bump", so the script reported the
|
|
correctly-bumped tree as 87 commits behind an unbumped version - listing
|
|
PKG-06's and PKG-07's own bump commits among the offenders. A tool whose
|
|
entire purpose is to be trusted before a deploy cried wolf on every run.
|
|
|
|
⚠️ AND `-G` IS NOT THE FIX EITHER, ONLY A BETTER FILTER. `-G'^version: "'`
|
|
matches a commit whose diff touches a line with that shape - which includes
|
|
a reformat, a quote change, or a comment moved onto the line, none of which
|
|
are bumps. So `-G` narrows the candidates and then the VALUE is compared
|
|
against the parent's. The question is "did the version change", and that is
|
|
the only thing that answers it.
|
|
|
|
⚠️ TWO GIT SHAPES THIS STILL GETS WRONG, NEITHER OF WHICH HAS EVER OCCURRED
|
|
IN THIS REPOSITORY - every bump here has been a clean, linear, forward-only
|
|
edit. Both were constructed in review rather than found:
|
|
|
|
1. A BUMP RESOLVED INSIDE A MERGE COMMIT IS INVISIBLE. `git log -G -- path`
|
|
does not diff merge commits at all without `-m`, so a version line whose
|
|
final value is decided by a conflict resolution is never a candidate.
|
|
The lookup then names the last ordinary bump, and the merge itself counts
|
|
as a runtime change since it - a FALSE REFUSAL of a tree that is in fact
|
|
correctly bumped.
|
|
2. REVERTING A BUMP WHILE KEEPING THE WORK MASKS THE WORK. Bump, change
|
|
`app/`, then revert only `config.yaml`: the version is back where it
|
|
started, the runtime change is still in the tree, and this reports
|
|
"nothing to deploy" - a FALSE CLEARANCE, which is the dangerous
|
|
direction.
|
|
|
|
Card PKG-11 carries both. They are recorded here rather than fixed because
|
|
a lookup that handles them costs more than the failure they represent, and
|
|
a caveat a reader can check beats a guarantee that is not true.
|
|
"""
|
|
r = _git("log", "--format=%H", "-G", r'^version: "', "--", CONFIG_PATH,
|
|
repo=repo)
|
|
if r.returncode != 0:
|
|
return None
|
|
shas = r.stdout.split()
|
|
for sha in shas: # newest first
|
|
here = _version_at(sha, repo)
|
|
there = _version_at(sha + "^", repo)
|
|
if here is not None and here != there:
|
|
return sha
|
|
return shas[-1] if shas else None
|
|
|
|
|
|
def runtime_commits_since_version_change(repo=None):
|
|
"""Commits touching RUNTIME since `version:` last changed. ([], None) if unknown.
|
|
|
|
Returns (commits, last_bump_sha). An empty list with a sha means the tree is
|
|
clean; an empty list with None means git could not answer and the caller
|
|
must NOT read that as a pass.
|
|
"""
|
|
last = last_version_change(repo)
|
|
if not last:
|
|
return [], None
|
|
r = _git("log", "--format=%H %s", f"{last}..HEAD", repo=repo)
|
|
if r.returncode != 0:
|
|
return [], None
|
|
out = []
|
|
for line in r.stdout.splitlines():
|
|
sha, _, subject = line.partition(" ")
|
|
f = _git("show", "--name-only", "--format=", sha, repo=repo)
|
|
if any(is_runtime(p) for p in f.stdout.split()):
|
|
out.append((sha[:10], subject))
|
|
return out, last[:10]
|
|
|
|
|
|
def main():
|
|
ap = argparse.ArgumentParser()
|
|
ap.add_argument("--deployed-version", required=True,
|
|
help="the version currently running ON THE BOX")
|
|
ap.add_argument("--options", help="path to the live options blob, exported "
|
|
"from the Supervisor. Omitted = that half is SKIPPED.")
|
|
ap.add_argument("--tree", default=ADDON)
|
|
ap.add_argument("--repo", default=None)
|
|
a = ap.parse_args()
|
|
|
|
problems, skipped = [], []
|
|
shipped = shipped_version(a.tree)
|
|
print(f" deployed on the box : {a.deployed_version}")
|
|
print(f" shipped in this tree: {shipped}")
|
|
|
|
commits, last = runtime_commits_since_version_change(a.repo)
|
|
if shipped is None:
|
|
problems.append("could not read `version:` out of config.yaml")
|
|
elif shipped == a.deployed_version:
|
|
if last is None:
|
|
skipped.append("could not ask git what changed since the last bump; "
|
|
"the no-op check is INCONCLUSIVE, not clean")
|
|
if commits:
|
|
problems.append(
|
|
f"THE DEPLOY WOULD BE A NO-OP. Version is {shipped} on both sides, and "
|
|
f"{len(commits)} commit(s) touching runtime files have landed since the "
|
|
f"version last changed ({last}). `ha apps update` will report success "
|
|
f"and install NOTHING. Bump `version:` first.")
|
|
for sha, subj in commits[:12]:
|
|
print(f" {sha} {subj[:76]}")
|
|
elif last is not None:
|
|
print(" no runtime change since the last bump - nothing to deploy")
|
|
|
|
if a.options:
|
|
# ⚠️ A TOOL RUN IN A HURRY BEFORE A DEPLOY MUST NOT ANSWER WITH A
|
|
# TRACEBACK. A missing or malformed export is the likeliest operator
|
|
# error here, and it is a REFUSAL, not a skip - the config was not
|
|
# checked and the script must not be re-run past it.
|
|
opts = None
|
|
try:
|
|
with open(a.options, encoding="utf-8") as fh:
|
|
opts = json.load(fh)
|
|
except FileNotFoundError:
|
|
problems.append(f"--options file not found: {a.options}")
|
|
except (json.JSONDecodeError, OSError, UnicodeDecodeError) as exc:
|
|
problems.append(f"--options file could not be read as JSON "
|
|
f"({type(exc).__name__}: {exc}); the config was NOT checked")
|
|
if a.options and opts is not None:
|
|
try:
|
|
from app.validate import validate_startup_config
|
|
except Exception as exc: # pragma: no cover
|
|
skipped.append(
|
|
f"could not import the shipped validator ({exc}); the config check "
|
|
f"did NOT run. `app/validate.py` imports its siblings, and those need "
|
|
f"the add-on's runtime dependencies - so THIS SCRIPT ONLY CHECKS A "
|
|
f"CONFIG WHERE THOSE DEPENDENCIES EXIST: inside the add-on container, "
|
|
f"or in the CI image. Re-run it as:\n"
|
|
f" docker run --rm -v <repo>:/w:ro goodwe-addon-checks:1 \\\n"
|
|
f" python3 /w/goodwe_controller/tools/predeploy_check.py \\\n"
|
|
f" --deployed-version <ver> --options /w/<options.json>")
|
|
else:
|
|
found = validate_startup_config(opts)
|
|
if found:
|
|
problems.append(
|
|
f"THE LIVE CONFIG IS REFUSED BY THIS VERSION'S VALIDATOR "
|
|
f"({len(found)} problem(s)). Deploying it is SystemExit(78): the "
|
|
f"add-on does not start, and the ingress panel goes with it because "
|
|
f"the add-on serving it is not running. Recovery is the Supervisor UI.")
|
|
for p in found:
|
|
print(f" - {p}")
|
|
else:
|
|
print(" live options pass this version's validator")
|
|
else:
|
|
skipped.append("--options was not given, so the live config was NEVER CHECKED "
|
|
"against this version's validator. A config refused at startup is "
|
|
"exit 78. Export the options and re-run before deploying.")
|
|
|
|
print()
|
|
for s in skipped:
|
|
print(f" ⚠️ SKIPPED: {s}")
|
|
if problems:
|
|
for p in problems:
|
|
print(f" ❌ {p}")
|
|
print("\n REFUSED - do not deploy.")
|
|
return 1
|
|
if skipped:
|
|
print(" NOT CLEARED - checks above did not run. Deploying now is a decision "
|
|
"made without them.")
|
|
return 2
|
|
print(" CLEARED for deploy.")
|
|
return 0
|
|
|
|
|
|
if __name__ == "__main__":
|
|
sys.exit(main())
|