fix(649): close the test-isolation gaps cold review found, and make the job's Gitea config authoritative
Four findings acted on; two more are real but pre-existing and are being filed rather
than fixed here (see below).
**The job's Gitea config was not authoritative.** `pr-changed-files.sh` resolves
`ETV_GITEA_URL` BEFORE `GITEA_BASE_URL` (and `ETV_GITEA_TOKEN` before `GITEA_TOKEN`),
because its other caller is a developer Mac using the ETV_* convention. Setting only the
GITEA_* names meant a runner exporting a stale ETV_GITEA_URL would enumerate a DIFFERENT
Gitea instance and this job would post a verdict here from a diff read there. Both names
are now set to the same value, so precedence cannot matter.
**Three guards passed their tests for the wrong reason.** Each was confirmed by deleting
the clause and watching the suite stay green — the reviewer asserted it, mutation proved
it:
- The explicit empty-response clause was uncovered on jq 1.8, because jq 1.8 rejects
empty input by itself. jq 1.6 does not, and THE RUNNER SHIPS 1.6 — so the one
environment where the clause is load-bearing had no coverage. That is the #643/#647
failure class reproduced inside the suite meant to prevent it. Now covered by importing
the existing jq-1.6 shim (imported, not copied — a second quirk emulator is the same
drift problem one level down), with a verify-the-verifier test and a positive control.
- `type == "array"` needed a body whose VALUES are valid rows. Two earlier attempts
failed for a third reason: `jq`'s `all(.[]; …)` iterates an object's values, so
`{"message":"…"}` and a single flat row are both rejected by `.filename` erroring on a
string. Only `{"0": {…valid row…}}` reaches the fail-open, where a non-array body
enumerates as a complete docs-only list.
- `.filename | ok` is now isolated by a row carrying a valid `.status` and no filename,
removing the closed-allow-list as a second reason to reject.
**Two assertions proved less than their names claimed.** `"jq-preflight.sh" in code` also
matched the `[ -x … ]` presence guard, so deleting the invocation left it green; it now
requires an invoking line. `_run_classify` accepted every POST, so a status aimed at the
wrong endpoint or sha would not have been noticed; it now asserts the POST lands on
`/statuses/<full head sha>`.
**One test name overclaimed** and is narrowed rather than left implying coverage it does
not have: the head-movement test proves "final head != expected sha", not movement
*during* enumeration.
Deferred, both pre-existing and neither introduced here — filed as follow-ups:
- A commit status is repo-global, so a `review-verdict/h10=success` obtained for head H
on one PR is inherited by any other PR with the same head, including one opened against
a different base. Same class as #632, reached by a third route.
- The A->B->A force-push race: paging is several round-trips and the head is re-read once
at the end, so a restore to the original sha passes the binding while the pages came
from two states. Inherent to enumerating a mutable list over an API with no
commit-pinned files endpoint.
Refs #649
This commit is contained in:
@@ -106,6 +106,15 @@ jobs:
|
||||
# repo, or a wrong host that answers, rather than erroring. A value already ending in
|
||||
# /api/v1 is used as-is by the script.
|
||||
GITEA_BASE_URL: ${{ github.server_url }}/api/v1
|
||||
# BOTH names, same value, on purpose. The script's precedence is
|
||||
# `ETV_GITEA_URL` > `GITEA_BASE_URL` > a hardcoded LAN default (and `ETV_GITEA_TOKEN` >
|
||||
# `GITEA_TOKEN`), because its other caller is a developer Mac using the ETV_* convention.
|
||||
# Setting only the GITEA_* names would leave this job's explicit configuration NON-
|
||||
# authoritative: a runner that happened to export a stale ETV_GITEA_URL would silently
|
||||
# enumerate a different Gitea instance and post the verdict here from a diff read there.
|
||||
# Cheap to make deterministic; leave both set even though only one is read.
|
||||
ETV_GITEA_URL: ${{ github.server_url }}/api/v1
|
||||
ETV_GITEA_TOKEN: ${{ secrets.GITEA_TOKEN }}
|
||||
REPO: ${{ github.repository }}
|
||||
PR: ${{ github.event.pull_request.number }}
|
||||
SHA: ${{ github.event.pull_request.head.sha }}
|
||||
|
||||
@@ -158,12 +158,41 @@ def test_non_array_body_fails_closed(enumerate_files):
|
||||
assert enumerate_files.fails_closed()
|
||||
|
||||
|
||||
def test_an_OBJECT_OF_VALID_ROWS_isolates_the_top_level_array_check(enumerate_files):
|
||||
"""Cold review found `test_non_array_body_fails_closed` passing for the wrong reason, and the
|
||||
first attempt to fix it failed for a THIRD reason — worth recording, because both near-misses
|
||||
look like coverage.
|
||||
|
||||
`jq`'s `all(.[]; …)` iterates an object's VALUES, so the top-level `type == "array"` check is
|
||||
only load-bearing when those values would themselves validate:
|
||||
|
||||
* `{"message":"internal error"}` — values are strings, `.filename` on a string errors. Rejected
|
||||
with the clause deleted, so it never isolated it.
|
||||
* `{"filename":"docs/a.md","status":"modified"}` — a single row, but its values are still
|
||||
strings. Same non-isolation, one level less obvious.
|
||||
* `{"0": {"filename":"docs/a.md","status":"modified"}}` — values ARE valid rows. With the clause
|
||||
deleted this validates, `length` is 1, the path is collected, and the next page ends the
|
||||
enumeration cleanly: a non-array body enumerated as a complete docs-only list. That is the
|
||||
fail-open, and only this shape exposes it.
|
||||
"""
|
||||
enumerate_files.set_pages({"0": {"filename": "docs/a.md", "status": "modified"}})
|
||||
assert enumerate_files.fails_closed()
|
||||
|
||||
|
||||
def test_rows_without_filename_fail_closed(enumerate_files):
|
||||
"""`[{}]` is a well-formed array that yields no paths — a partial list wearing a valid shape."""
|
||||
enumerate_files.set_pages([{}, {}])
|
||||
assert enumerate_files.fails_closed()
|
||||
|
||||
|
||||
def test_a_row_with_a_VALID_status_but_no_filename_isolates_the_filename_check(enumerate_files):
|
||||
"""Same wrong-reason problem: `[{}]` is also rejected by the closed `.status` allow-list, since
|
||||
an absent status is not in it. A row carrying a legitimate `status` and no `filename` removes
|
||||
that second reason, leaving only the guard under test."""
|
||||
enumerate_files.set_pages([{"status": "modified"}])
|
||||
assert enumerate_files.fails_closed()
|
||||
|
||||
|
||||
def test_array_of_scalars_fails_closed(enumerate_files):
|
||||
enumerate_files.set_pages(["docs/a.md", "docs/b.md"])
|
||||
assert enumerate_files.fails_closed()
|
||||
@@ -205,9 +234,17 @@ def test_renamed_row_without_previous_filename_fails_closed(enumerate_files):
|
||||
assert enumerate_files.fails_closed()
|
||||
|
||||
|
||||
def test_head_moving_during_enumeration_fails_closed(enumerate_files):
|
||||
"""A force-push between round-trips means page 1 came from head A and page 2 from head B, so
|
||||
the assembled list belongs to no single commit."""
|
||||
def test_head_differing_from_the_expected_sha_fails_closed(enumerate_files):
|
||||
"""Narrowed to what this actually proves, per cold review.
|
||||
|
||||
The stub serves the alternate sha from the FIRST PR-object read, and the script reads that
|
||||
object only once, after paging — so this exercises "the final head does not equal the expected
|
||||
sha", not movement *during* enumeration. The distinction matters: an A->B->A force-push round
|
||||
trip would restore the expected sha and pass this check while the pages came from two different
|
||||
states. That race is inherent to enumerating a mutable list over several round-trips against an
|
||||
API with no commit-pinned files endpoint, and is tracked separately rather than papered over
|
||||
with a test name that implies it is covered.
|
||||
"""
|
||||
enumerate_files.set_pages(_rows(["docs/a.md"]))
|
||||
enumerate_files.head_moves_to(OTHER_SHA)
|
||||
assert enumerate_files.fails_closed()
|
||||
@@ -237,6 +274,55 @@ def test_a_SHORT_page_does_not_end_the_enumeration(enumerate_files):
|
||||
assert "ErsatzTV/Program.cs" in enumerate_files.paths()
|
||||
|
||||
|
||||
# --- The same guard, under the runner's jq 1.6 ------------------------------------------------
|
||||
#
|
||||
# `test_transport_failure_mid_pagination_fails_closed` above does NOT isolate the explicit
|
||||
# `if [ -z "${raw//[[:space:]]/}" ]` clause — cold review claimed this and mutation confirmed it:
|
||||
# deleting that clause leaves the whole suite green on a developer Mac, because jq 1.8 rejects empty
|
||||
# input on its own. jq 1.6 does not, and the runner ships 1.6 — so the one environment where the
|
||||
# clause is load-bearing was the one environment with no coverage. That is the #643/#647 failure
|
||||
# class exactly, reproduced in the test suite meant to prevent it.
|
||||
#
|
||||
# The shim is IMPORTED, not copied. A second copy of a version-quirk emulator is the same
|
||||
# two-implementations-drift problem this whole issue is about, one level down.
|
||||
from scripts.tests.test_merge_consent_exemption import _JQ16_SHIM # noqa: E402
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def enumerate_files_jq16(enumerate_files, tmp_path):
|
||||
"""The same harness, plus a jq shim reproducing jq 1.6's empty-input `-e` exit status."""
|
||||
jq = tmp_path / "bin" / "jq"
|
||||
jq.write_text(_JQ16_SHIM)
|
||||
jq.chmod(0o755)
|
||||
return enumerate_files
|
||||
|
||||
|
||||
def test_jq16_shim_actually_reproduces_the_quirk(enumerate_files_jq16, tmp_path):
|
||||
"""Verify the verifier. A shim that failed to install would make the test below pass vacuously,
|
||||
reporting the guard safe on jq 1.6 without ever exercising the quirk."""
|
||||
assert enumerate_files_jq16 is not None # the fixture is what installs the shim
|
||||
jq = str(tmp_path / "bin" / "jq")
|
||||
empty = subprocess.run([jq, "-e", "."], input="", capture_output=True, text=True)
|
||||
assert empty.returncode == 0, "the shim does not reproduce jq 1.6's empty-input exit 0"
|
||||
real = subprocess.run([jq, "-e", ".a"], input='{"a":1}', capture_output=True, text=True)
|
||||
assert real.returncode == 0 and real.stdout.strip() == "1", "the shim broke ordinary jq"
|
||||
false = subprocess.run([jq, "-e", ".a"], input='{"a":false}', capture_output=True, text=True)
|
||||
assert false.returncode == 1, "the shim broke jq's real -e semantics for a false result"
|
||||
|
||||
|
||||
def test_transport_failure_mid_pagination_fails_closed_on_jq16(enumerate_files_jq16):
|
||||
"""The property, asserted on the interpreter that actually runs it in CI."""
|
||||
enumerate_files_jq16.set_pages(_rows([f"docs/f{i}.md" for i in range(50)]), "ERROR",
|
||||
_rows(["docs/tail.md"]))
|
||||
assert enumerate_files_jq16.fails_closed()
|
||||
|
||||
|
||||
def test_a_docs_only_pr_still_enumerates_cleanly_on_jq16(enumerate_files_jq16):
|
||||
"""Positive control: the shim must not make everything fail, or the test above proves nothing."""
|
||||
enumerate_files_jq16.set_pages(_rows(["docs/a.md", "docs/b.md"]))
|
||||
assert enumerate_files_jq16.paths() == ["docs/a.md", "docs/b.md"]
|
||||
|
||||
|
||||
# --- The CALLER contract: a non-zero exit must withhold the exemption, on its own -------------
|
||||
#
|
||||
# This is the single line the whole extraction rests on, and it was the one guard nothing pinned. A
|
||||
@@ -437,12 +523,21 @@ def test_the_workflow_runs_the_jq_preflight_in_FLOOR_mode_only():
|
||||
# CODE only, for the reason `_code_lines` documents: the first draft of this assertion read the
|
||||
# raw text and went red on the workflow's own comment explaining why `--expect` is banned here.
|
||||
code = _code_lines(WORKFLOW)
|
||||
assert "jq-preflight.sh" in code, (
|
||||
# A bare `"jq-preflight.sh" in code` is NOT enough, and cold review was right to say so: the
|
||||
# path also appears in the `if [ -x ./scripts/jq-preflight.sh ]` presence guard, so deleting the
|
||||
# actual invocation would leave that substring behind and the assertion green. Require a line
|
||||
# that INVOKES it.
|
||||
steps = [s for s in _workflow_steps() if "jq-preflight.sh" in (s.get("run") or "")]
|
||||
assert steps, (
|
||||
"review-verdict.yml no longer runs the jq preflight, so the version its shell gates run "
|
||||
"under is unobservable again (ersatztv#648)")
|
||||
invocations = [ln.strip() for ln in steps[0]["run"].splitlines()
|
||||
if re.match(r"^\s*(\./)?scripts/jq-preflight\.sh(\s|$)", ln)]
|
||||
assert invocations, "the jq preflight is referenced but never actually invoked"
|
||||
assert "--expect" not in code, (
|
||||
"review-verdict.yml must run jq-preflight.sh in floor-only mode; --expect here deadlocks "
|
||||
"every merge on `main` the day the runner's jq changes")
|
||||
assert all("--expect" not in ln for ln in invocations)
|
||||
|
||||
|
||||
# --- The ENFORCED caller's contract, EXECUTED ---------------------------------------------------
|
||||
@@ -467,6 +562,7 @@ out = pathlib.Path(os.environ["STUB_DIR"])
|
||||
if "-X" in args and args[args.index("-X") + 1] == "POST":
|
||||
payload = args[args.index("-d") + 1]
|
||||
(out / "posted.json").write_text(payload)
|
||||
(out / "posted_url.txt").write_text(url)
|
||||
print("{}")
|
||||
sys.exit(0)
|
||||
|
||||
@@ -512,6 +608,14 @@ def _run_classify(tmp_path, enum_stub: str | None, author: str = "timothy"):
|
||||
r = subprocess.run(["bash", str(script)], cwd=tmp_path, env=env,
|
||||
capture_output=True, text=True)
|
||||
posted = tmp_path / "posted.json"
|
||||
url_file = tmp_path / "posted_url.txt"
|
||||
if url_file.exists():
|
||||
# Assert the WIRING, not only the classification. Cold review's point: the stub accepts every
|
||||
# POST, so a status aimed at the wrong endpoint or the wrong sha would leave these tests green
|
||||
# while the real required check was never written.
|
||||
url = url_file.read_text().strip()
|
||||
assert url.endswith(f"/statuses/{SHA}"), (
|
||||
f"the status was POSTed to {url!r}, not to /statuses/<full head sha>")
|
||||
return (json.loads(posted.read_text()) if posted.exists() else None), r
|
||||
|
||||
|
||||
|
||||
Reference in New Issue
Block a user