From 08b0ed56e92c0551d06bd8fb0a8cfaf51c40d71d Mon Sep 17 00:00:00 2001 From: Polichinl Date: Wed, 12 Aug 2026 23:02:45 +0200 Subject: [PATCH] =?UTF-8?q?fix(tests):=20#247=20=E2=80=94=20one=20conditio?= =?UTF-8?q?n,=20one=20diagnosis;=20and=20the=20scratch=20repos=20stop=20ha?= =?UTF-8?q?nging?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ONE CONDITION WAS PRODUCING TWO DIAGNOSES. A ref this clone cannot see and a ref that is not a frozen commit shared a message naming neither cause, which never said `git fetch`. Three branches now, with different remedies: a blank pin is a pin defect (an empty ref reads the index); an object this clone has never seen is a fetch problem (shallow, --single-branch, or older than the pin); a ref that resolves to a non-commit is a pin defect. The bogus-sha case is deliberately the second, because that is the truth — the reader cannot tell a bad pin from a missing fetch, and now says so instead of guessing. ROWS() REFUSES A SCALAR ROW BY NAME. `[test_environment]` on the live registry is top-level strings. Classifying such a table CONSUMED — which the partition check's own remediation message invites — used to return an AttributeError from a dict comprehension, in the module whose stated justification is failing legibly. THE SCRATCH REPOSITORIES ARE HERMETIC. All three run git with `-c commit.gpgsign=false -c core.hooksPath=/dev/null` and a timeout. Verified rather than assumed: ran under a HOME whose .gitconfig sets commit.gpgsign=true and points core.hooksPath at a nonexistent directory — four tests pass where they would have failed with a bare CalledProcessError, or with a passphrase-protected key blocked on pinentry and hung the run. The refusal proof's expectations moved with the split, which is the point of having it: two of its five cases now assert different messages because the messages became more precise. C-91 resolved and moved to Resolved Concerns; 22 open -> 21. Suite 421 passed / 1 skipped / 40 xfailed, ruff clean. Closes #247. Epic #241 complete. Co-Authored-By: Claude Opus 5 (1M context) --- reports/technical_risk_register.md | 59 ++++++++++++++++++------------ tests/seam_registry.py | 52 ++++++++++++++++++++++---- tests/test_env_declaration.py | 39 ++++++++++++++++++-- 3 files changed, 115 insertions(+), 35 deletions(-) diff --git a/reports/technical_risk_register.md b/reports/technical_risk_register.md index 7068e2c..e331c63 100644 --- a/reports/technical_risk_register.md +++ b/reports/technical_risk_register.md @@ -6,8 +6,8 @@ | Owner | Dylan Pinheiro / PRIO MD&D Team | | Last Updated | 2026-08-12 | | Total Concerns | 97 | -| Open Concerns | 22 | -| Resolved Concerns | 75 | +| Open Concerns | 21 | +| Resolved Concerns | 76 | --- @@ -313,27 +313,6 @@ Cross-refs: **C-57** (the scan's own entry and its history), **C-93** (why the a --- -### C-91: The git plumbing this arc added turns ordinary developer states into hard errors, bare tracebacks, and one possible hang - -| Field | Value | -|-------|-------| -| ID | C-91 | -| Tier | 3 — no wrong data and nothing silent. It taxes every contributor who does not already have the exact sibling checkout this repository assumes, and it does so with diagnoses that point at the wrong cause. | -| Source | `/code-review max` on PR #239 post-merge, 2026-08-11 | -| Trigger | Any of: a contributor clones views-appwrite shallow, single-branch, or before the pinned commit; a table classified `CONSUMED` upstream is written as flat keys rather than sub-tables; a contributor has `commit.gpgsign` or a global `core.hooksPath` set. | -| Owner | This repository. | -| Location | `tests/seam_registry.py:71` (refusal diagnoses), `:150` (`rows`), `tests/test_env_declaration.py:820` (the scratch repo). | - -**A stale clone produces four errors carrying the wrong explanation.** The reader the extraction replaced read the file off disk, so an older checkout simply read an older file. Now a clone that predates the pinned commit — or is shallow, or was made `--single-branch`, which matters because views-appwrite's default branch is not `main` — raises *"does not resolve to a commit … an empty ref reads the index and a branch reads a moving tip"*. That names neither cause and does not say `git fetch`. Meanwhile `test_the_pinned_commit_is_reachable_from_the_contract_repos_main` detects the identical root cause and *skips* with the right remedy. One condition, one skip, four errors, three explanations. - -**`rows()` raises a bare `AttributeError` on a shape the live registry already has.** It guards a null section and not a scalar row. Verified: views-appwrite's `[test_environment]` holds `status` and `fact` as top-level strings. That table is `IGNORED`, so nothing breaks today — but when the partition check fires on a new upstream table, its own message instructs the maintainer to classify it `CONSUMED` or `MIRRORED`, and doing so for a table written that way returns a traceback pointing into a dict comprehension. From the module whose docstring says a helper justified by failing legibly must not hand back a bare traceback. - -**The scratch repo inherits the developer's global git config and has no timeout.** `test_the_pinned_reader_refuses_every_way_a_baseline_can_be_wrong` sets `user.name` and `user.email` and stops. With `commit.gpgsign = true` it fails with a bare `CalledProcessError` — `capture_output=True` swallows git's explanation. With a passphrase-protected key it blocks on pinentry with no `timeout`, hanging the whole run; `conftest.git_output`, which this helper bypasses, caps at 30 seconds. The leak was anticipated for identity and not for the setting that blocks. - -Cross-refs: **C-90** (the same module's untested core), **C-88** (why the module exists outside `conftest.py`), ADR-008 (explicit failure), issue #196. - ---- - ### C-92: Nothing here checks that a consumer SELECTS by the delivery label — for either partner | Field | Value | @@ -1026,6 +1005,40 @@ See also C-40 (the inheritance/representation coupling this migration unwinds), ## Resolved Concerns +### C-91: The git plumbing this arc added turns ordinary developer states into hard errors, bare tracebacks, and one possible hang — RESOLVED + +| Field | Value | +|-------|-------| +| ID | C-91 | +| Tier | 3 — no wrong data and nothing silent. It taxes every contributor who does not already have the exact sibling checkout this repository assumes, and it does so with diagnoses that point at the wrong cause. | +| Source | `/code-review max` on PR #239 post-merge, 2026-08-11 | +| Trigger | Any of: a contributor clones views-appwrite shallow, single-branch, or before the pinned commit; a table classified `CONSUMED` upstream is written as flat keys rather than sub-tables; a contributor has `commit.gpgsign` or a global `core.hooksPath` set. | +| Owner | This repository. | +| Location | `tests/seam_registry.py:71` (refusal diagnoses), `:150` (`rows`), `tests/test_env_declaration.py:820` (the scratch repo). | + +**A stale clone produces four errors carrying the wrong explanation.** The reader the extraction replaced read the file off disk, so an older checkout simply read an older file. Now a clone that predates the pinned commit — or is shallow, or was made `--single-branch`, which matters because views-appwrite's default branch is not `main` — raises *"does not resolve to a commit … an empty ref reads the index and a branch reads a moving tip"*. That names neither cause and does not say `git fetch`. Meanwhile `test_the_pinned_commit_is_reachable_from_the_contract_repos_main` detects the identical root cause and *skips* with the right remedy. One condition, one skip, four errors, three explanations. + +**`rows()` raises a bare `AttributeError` on a shape the live registry already has.** It guards a null section and not a scalar row. Verified: views-appwrite's `[test_environment]` holds `status` and `fact` as top-level strings. That table is `IGNORED`, so nothing breaks today — but when the partition check fires on a new upstream table, its own message instructs the maintainer to classify it `CONSUMED` or `MIRRORED`, and doing so for a table written that way returns a traceback pointing into a dict comprehension. From the module whose docstring says a helper justified by failing legibly must not hand back a bare traceback. + +**The scratch repo inherits the developer's global git config and has no timeout.** `test_the_pinned_reader_refuses_every_way_a_baseline_can_be_wrong` sets `user.name` and `user.email` and stops. With `commit.gpgsign = true` it fails with a bare `CalledProcessError` — `capture_output=True` swallows git's explanation. With a passphrase-protected key it blocks on pinentry with no `timeout`, hanging the whole run; `conftest.git_output`, which this helper bypasses, caps at 30 seconds. The leak was anticipated for identity and not for the setting that blocks. + +**RESOLVED 2026-08-12 (#247) — all three.** + +**One condition, one diagnosis.** A ref this clone cannot see and a ref that is not a frozen commit used to share a message that named neither cause and never said `git fetch`. They are now separate branches with separate remedies, and a third — an empty pin — is called what it is: a defect in the pin, not the checkout. The bogus-sha case is deliberately classified as *"this clone cannot see it"*, because that is the truth: the reader cannot tell a bad pin from a missing fetch, and the message says so rather than guessing. + +**`rows()` refuses a scalar row by name.** `[test_environment]` on the live registry is top-level strings; classifying such a table CONSUMED — which the partition check's own remediation message invites — used to return an `AttributeError` from a dict comprehension, in the module whose justification is failing legibly. + +**The scratch repositories are hermetic.** All three now run git with `-c commit.gpgsign=false -c core.hooksPath=/dev/null` and an explicit timeout. Verified by running the suite under a `HOME` whose `.gitconfig` sets `commit.gpgsign = true` and points `core.hooksPath` at a nonexistent directory: four tests pass where they would previously have failed opaquely or blocked on pinentry with no timeout. + +Mutation-proven three ways, each reverted: removing the scalar-row refusal, the missing-object branch, and the empty-pin branch. + +Cross-refs: **C-90** (the same module's untested core), **C-88** (why the module exists outside `conftest.py`), ADR-008 (explicit failure), issue #196. + +--- + +--- + + ### C-90: A mutation proof that cannot fail, and the untested function a module was extracted to create — RESOLVED | Field | Value | diff --git a/tests/seam_registry.py b/tests/seam_registry.py index d6a4f90..beb870b 100644 --- a/tests/seam_registry.py +++ b/tests/seam_registry.py @@ -65,10 +65,34 @@ def registry_at(repo: Path, ref: str) -> dict: capture_output=True, text=True, check=False, timeout=30, ) if resolved.returncode != 0 or not resolved.stdout.strip(): + # Two conditions used to share this message, and they have different remedies. + # A ref this clone has never heard of is an environment problem — a shallow or + # `--single-branch` clone, or one made before the pin. A ref that resolves to + # something that is not a commit is a pin defect. Only the second is this + # repository's fault; only the first is fixed by fetching (register C-91). + if not ref.strip(): + raise RegistryReadError( + "the pin is empty, and an empty ref does not resolve to a commit — " + "`git show ':'` reads the INDEX, so a blanked pin would compare " + "the registry to itself and report green against every future edition. " + "This is a defect in the pin, not in the checkout." + ) + known = subprocess.run( + ["git", "-C", str(repo), "cat-file", "-e", f"{ref}^{{object}}"], + capture_output=True, text=True, check=False, timeout=30, + ).returncode == 0 + if not known: + raise RegistryReadError( + f"{repo} has no object {ref!r}. This is almost always a clone that is " + "shallow, `--single-branch`, or simply older than the pin — run " + f"`git -C {repo} fetch --tags origin` and try again. It is not a defect " + "in the pin: nothing here can tell whether that commit is good until " + "this checkout can see it." + ) raise RegistryReadError( - f"{ref!r} does not resolve to a commit in {repo}. A pin must name a frozen " - "commit: an empty ref reads the index and a branch reads a moving tip, and " - "either would make the comparison compare the registry to itself." + f"{ref!r} exists in {repo} but does not resolve to a commit. A pin must name " + "a frozen commit: an empty ref reads the index and a branch reads a moving " + "tip, and either would make the comparison compare the registry to itself." ) if not resolved.stdout.strip().startswith(ref): raise RegistryReadError( @@ -146,8 +170,20 @@ def rows(registry: dict, sections: tuple) -> dict[str, tuple]: The one shared projection. Two hand-copied versions had already diverged on null handling — one raised ``AttributeError`` on a null section, the other did not. """ - return { - name: (section, body.get("class"), body.get("value", ABSENT)) - for section in sections - for name, body in (registry.get(section) or {}).items() - } + out = {} + for section in sections: + for name, body in (registry.get(section) or {}).items(): + if not isinstance(body, dict): + # `[test_environment]` on the live registry is exactly this — top-level + # strings, not sub-tables. Reaching it means someone classified such a + # table CONSUMED, which the partition check's own remediation message + # invites. Refusing by name beats an AttributeError from a comprehension + # in the module whose justification is failing legibly (register C-91). + raise RegistryReadError( + f"[{section}].{name} is a bare {type(body).__name__}, not a table. " + "This section's rows are scalars, so it carries no class or value to " + "read — it cannot be CONSUMED. Classify it IGNORED with a reason, or " + "read it with something other than `rows()`." + ) + out[name] = (section, body.get("class"), body.get("value", ABSENT)) + return out diff --git a/tests/test_env_declaration.py b/tests/test_env_declaration.py index d5836aa..2f36f7d 100644 --- a/tests/test_env_declaration.py +++ b/tests/test_env_declaration.py @@ -43,6 +43,7 @@ registry_at as _registry_at, registry_current, registry_current as _registry_current, + rows, rows as _rows, ) from tests.conftest import ( @@ -882,7 +883,15 @@ def test_the_pinned_reader_refuses_every_way_a_baseline_can_be_wrong(tmp_path): import subprocess as sp def git(*args): - return sp.run(["git", "-C", str(tmp_path), *args], capture_output=True, text=True, check=True) + # `-c`, and a timeout. A contributor's global `commit.gpgsign` makes `git commit` + # fail with a bare CalledProcessError here — capture_output swallows git's + # explanation — and with a passphrase-protected key it blocks on pinentry with + # stdin inherited and no timeout, hanging the whole run (register C-91). + return sp.run( + ["git", "-C", str(tmp_path), "-c", "commit.gpgsign=false", + "-c", "core.hooksPath=/dev/null", *args], + capture_output=True, text=True, check=True, timeout=30, + ) git("init", "-q") git("config", "user.email", "t@t") @@ -905,7 +914,11 @@ def commit(text: str, message: str) -> str: "the reader must accept a well-formed registry, or the refusals below prove nothing" ) - with pytest.raises(_RegistryReadError, match="does not resolve to a commit"): + # The five refusals, and note that two of them now say DIFFERENT things. A ref this + # clone cannot see and a ref that is not a commit used to share one message; they have + # different remedies — `git fetch` versus fix the pin — and conflating them sent + # contributors to the wrong one (register C-91). + with pytest.raises(_RegistryReadError, match="the pin is empty"): _registry_at(tmp_path, "") # a blanked pin reads the INDEX with pytest.raises(_RegistryReadError, match="annotated TAG|does not start with it"): _registry_at(tmp_path, "v1") # a tag object: git peels it, the URL 404s @@ -913,8 +926,9 @@ def commit(text: str, message: str) -> str: _registry_at(tmp_path, empty) # exits 0, stdout empty with pytest.raises(_RegistryReadError, match="no meta.version or no"): _registry_at(tmp_path, anchorless) # parses, but is not the registry - with pytest.raises(_RegistryReadError, match="does not resolve to a commit"): - _registry_at(tmp_path, "0" * 40) # a pin that names nothing + with pytest.raises(_RegistryReadError, match="has no object"): + _registry_at(tmp_path, "0" * 40) # this clone cannot see it; it cannot tell + # a bad pin from a missing fetch, and says so def test_the_role_vocabulary_is_closed(): @@ -1470,3 +1484,20 @@ def git(*args): registry_at(tmp_path, absent) with pytest.raises(RegistryReadError, match="did not parse as TOML"): registry_at(tmp_path, garbage) + + +def test_rows_refuses_a_section_whose_entries_are_not_tables(): + """`[test_environment]` on the live registry is scalars, not sub-tables. + + Nothing breaks today because that table is IGNORED — but the partition check's own + remediation message tells a maintainer to classify a new table CONSUMED, and doing + that for one written this way used to return an `AttributeError` from a dict + comprehension. Register C-91. + """ + scalars = {"test_environment": {"status": "none", "fact": "a sentence"}} + with pytest.raises(RegistryReadError, match=r"\[test_environment\]\.(status|fact) is a bare str"): + rows(scalars, ("test_environment",)) + + # and the ordinary shape still works, or the refusal above proves nothing + tables = {"target": {"APPWRITE_X": {"class": "target", "value": "v"}}} + assert rows(tables, ("target",)) == {"APPWRITE_X": ("target", "target", "v")}