From 0b653c35a6eef1facaf035a729c3ac9ede1030bc Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Mon, 7 Sep 2026 17:02:01 +0200 Subject: [PATCH 1/2] PKG-13: the no-op half of the pre-deploy check has never run as documented MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ⚠️ THE INVOCATION ON PKG-09's CARD AND IN THE TOOL'S OWN DOCSTRING RUNS ONLY HALF THE CHECK. The container runs 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` returned non-zero, `last_version_change()` returned None, and the silent-no-op check - THE REASON PKG-09 EXISTS - reported SKIPPED. Measured, on the documented command against release/1.0: ⚠️ SKIPPED: could not ask git what changed since the last bump; the no-op check is INCONCLUSIVE, not clean NOT CLEARED and with `--user $(id -u):$(id -g)`, unchanged otherwise: no runtime change since the last bump - nothing to deploy CLEARED for deploy. ⚠️ THE TWO HALVES WERE MUTUALLY EXCLUSIVE ACROSS THE TWO DOCUMENTED ENVIRONMENTS. 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. The pre-0.9.0 clearance was sound only because it happened to be driven as two separate invocations. ⚠️ 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 ever needing git. FIXED IN `_git()` WITH `safe.directory`, NOT BY DOCUMENTING `--user`. The tool should be correct however it is invoked rather than requiring every future caller to remember a flag; the read is read-only, on a repository the operator is about to deploy from. ⚠️ WHY THE TESTS COULD NOT SEE IT, WHICH IS THE FINDING BEHIND THE FINDING. `test_version_gate.py` invokes the gate with `sys.executable` against fixtures it creates as its own uid, so no fixture could ever meet an ownership mismatch. MUTATION.md habit 4 in a fourth shape, inside the file written to prevent it - after the pickaxe bug, after the one-bump history, after the filter-only survivor. Two fixtures added: - a directory that is NOT a git repository, so the lookup genuinely fails: the gate must exit 2 saying INCONCLUSIVE and must NOT say CLEARED. - ⚠️ THE EXIT CODES PINNED BY VALUE, NOT BY TRUTHINESS. `returncode != 0` is satisfied by collapsing NOT-CLEARED into REFUSED, which review found the earlier draft could not tell apart. 1 says "this deploy is wrong"; 2 says "nobody checked". Driven: collapsing 2 into 1 now goes red on exactly that check. Suite: 26 files, all pass. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- goodwe_controller/test_version_gate.py | 38 +++++++++++++++++++++ goodwe_controller/tools/predeploy_check.py | 39 ++++++++++++++++++++-- 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/goodwe_controller/test_version_gate.py b/goodwe_controller/test_version_gate.py index e579336..0bd76e4 100644 --- a/goodwe_controller/test_version_gate.py +++ b/goodwe_controller/test_version_gate.py @@ -231,6 +231,44 @@ check("...and because config.yaml IS a runtime path, that cosmetic commit is " r8.returncode == 1 and "COSMETIC" in r8.stdout) +print() +print("⭐ A GIT LOOKUP THAT GENUINELY FAILS - THE SHAPE NO FIXTURE COULD REACH") +# ⚠️ EVERY FIXTURE ABOVE CREATES ITS OWN REPOSITORY AS ITS OWN UID, so none of +# them could ever meet the failure that actually happened: the documented +# invocation runs the container as uid 0 against a checkout owned by the +# operator, git refuses with "detected dubious ownership", `_git` returns +# non-zero, and the no-op half reports SKIPPED. It degraded honestly - exit 2, +# never a false clearance - but the check this file exists for had never once +# run in its documented form, and nothing here could see that. +# +# `_git` now sets `safe.directory`, so ownership no longer breaks it. This +# section grades the REMAINING failure mode: git answering not at all. +root5 = tempfile.mkdtemp(prefix="test_version_gate.nogit.") +tmpdirs.append(root5) +addon5 = os.path.join(root5, "goodwe_controller") +os.makedirs(addon5) +write(addon5, "config.yaml", 'name: t\nversion: "0.7.0"\n') # NOT a git repo + +check("a repository git cannot answer for reports no last-bump commit", + last_version_change(root5) is None) + +r9 = run_gate(addon5, root5, "0.7.0", opts_file(VALID)) +check("⭐ AND THE GATE DOES NOT CLEAR ON IT. An unanswerable git lookup is " + "INCONCLUSIVE, not clean - the one thing it must never do is call a " + "tree it could not inspect deployable", + r9.returncode != 0 and "CLEARED for deploy" not in r9.stdout) +check("...it says INCONCLUSIVE in those words", "INCONCLUSIVE" in r9.stdout) + +# ⚠️ AND THE EXIT CODES ARE PINNED BY VALUE, NOT BY TRUTHINESS. `returncode != 0` +# is satisfied by collapsing NOT-CLEARED into REFUSED, which review found the +# earlier draft could not tell apart. 1 and 2 mean different things to an +# operator: 1 says "this deploy is wrong", 2 says "nobody checked". +check("NOT CLEARED is exactly 2, not merely non-zero", r9.returncode == 2) +r10 = run_gate(addon3, root3, "0.7.0", opts_file(dict(VALID, slew_w=1000))) +check("REFUSED is exactly 1, not merely non-zero", + r10.returncode == 1 and "REFUSED" in r10.stdout) + + print("\nthe shipped tree's own version is readable") check("shipped_version() finds a version", bool(shipped_version(HERE))) diff --git a/goodwe_controller/tools/predeploy_check.py b/goodwe_controller/tools/predeploy_check.py index e5512bb..a75d5ca 100644 --- a/goodwe_controller/tools/predeploy_check.py +++ b/goodwe_controller/tools/predeploy_check.py @@ -84,8 +84,43 @@ def is_runtime(path): def _git(*args, repo=None): - return subprocess.run(["git", "-C", repo or os.path.dirname(ADDON), *args], - capture_output=True, text=True) + """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", "safe.directory=*", + "-C", where, *args], + capture_output=True, text=True) def _version_at(sha, repo=None, path=CONFIG_PATH): -- 2.54.0 From 1ba35f9c99c66523a851dc882e233d63d21fe69b Mon Sep 17 00:00:00 2001 From: glenn schrooyen Date: Mon, 7 Sep 2026 17:27:36 +0200 Subject: [PATCH 2/2] PKG-13: one safe.directory flag, not two, and the tighter of the pair 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 Claude-Session: https://claude.ai/code/session_01BwRLw7eKYj2byVdwB6kbjU --- goodwe_controller/tools/predeploy_check.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/goodwe_controller/tools/predeploy_check.py b/goodwe_controller/tools/predeploy_check.py index a75d5ca..cc718af 100644 --- a/goodwe_controller/tools/predeploy_check.py +++ b/goodwe_controller/tools/predeploy_check.py @@ -118,8 +118,7 @@ def _git(*args, repo=None): """ where = repo or os.path.dirname(ADDON) return subprocess.run( - ["git", "-c", f"safe.directory={where}", "-c", "safe.directory=*", - "-C", where, *args], + ["git", "-c", f"safe.directory={where}", "-C", where, *args], capture_output=True, text=True) -- 2.54.0