fix(632): validate status ROWS, not just the top-level array — the same swallow one level down
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 11s
PR Gates / Docs update reminder (pull_request) Successful in 19s
Review verdict / Set review-verdict status (pull_request) Successful in 6s
PR Gates / decisions lifecycle (pull_request) Successful in 28s
PR Gates / Script tests (pytest) (pull_request) Successful in 49s
review-verdict/h10 Review-verdict: MERGEABLE @ ed8de77 (base: main)
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 9m14s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 6m1s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 9s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 8s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 16m46s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
PR Gates / CI image pin matches docker/ci (pull_request) Successful in 11s
PR Gates / Docs update reminder (pull_request) Successful in 19s
Review verdict / Set review-verdict status (pull_request) Successful in 6s
PR Gates / decisions lifecycle (pull_request) Successful in 28s
PR Gates / Script tests (pytest) (pull_request) Successful in 49s
review-verdict/h10 Review-verdict: MERGEABLE @ ed8de77 (base: main)
Build ErsatzTV Image / Build & test (.NET) (pull_request) Successful in 9m14s
Build ErsatzTV Image / Functional E2E (curl + UI contracts) (pull_request) Successful in 6m1s
Build ErsatzTV Image / API docs in sync (OpenAPI + endpoint index) (pull_request) Successful in 9s
Build ErsatzTV Image / Formatting (changed .cs conform to .editorconfig) (pull_request) Successful in 8s
Build ErsatzTV Image / EF migration integrity (SQLite + MySql) (pull_request) Successful in 16m46s
Build ErsatzTV Image / Build & push image (amd64) (pull_request) Has been skipped
Re-review caught my previous fix claiming more than it delivered. "Every unreadable input
asks" was false: validating only that `.statuses` is an array left `{"statuses":[1]}`
passing the guard, after which `.context` on a number errors and the `|| true` on the
extraction turned that error into an empty description — straight back onto the
graceful-adoption path the guard exists to distinguish from. The identical
swallow-the-error shape I had just fixed a few lines up, surviving one level deeper.
The validation domain now matches the CONSUMPTION domain: every row must be an object
with a string `.context` and a `.description` that is absent or a string. The extraction
drops its `|| true` and asks explicitly instead, since a swallowed error there is
indistinguishable from a benign "no base recorded".
Both guards are load-bearing, for DIFFERENT shapes — established by mutating them
together and separately rather than assuming the pair was redundant:
- a non-string `.description` is caught ONLY by the row validation (jq -r renders the
object as JSON, the sed finds no `(base: …)`, and it silently reads as a legacy verdict);
- a scalar row is caught by EITHER, so with the validation weakened the extraction guard
is what still asks.
Also noted rather than changed: this is the third read of the same status endpoint in a
worst-case hook run. Sharing one snapshot would close a narrow same-run disagreement
window, but the other two branches derive different decisions from a failed read, so
threading a shared response through them changes pre-existing logic rather than #632's.
Recorded in place so it is not rediscovered as an oversight — every `decide` exits
immediately, so the reads cannot produce one self-contradictory message.
Refs #632
This commit is contained in:
@@ -147,15 +147,39 @@ if [ -z "$live_base" ]; then
|
||||
decide ask "H10 merge gate: PR #$pr reports no base branch (.base.ref), so the verdict cannot be checked against the branch it was formed for (ersatztv#632). Confirm the PR still targets the branch it was reviewed against before merging."
|
||||
fi
|
||||
if [ -n "$sha" ]; then
|
||||
# This is the THIRD read of this endpoint in a worst-case hook run (the ordinary-CI branch and the
|
||||
# scheduled-auto-merge branch each do their own). Sharing one snapshot would close a narrow
|
||||
# same-run window where two reads disagree, but the later branches derive different decisions from
|
||||
# a failed read than this one does, so threading a shared response through them is a change to
|
||||
# pre-existing logic rather than to ersatztv#632's. Left deliberately, noted so it is not
|
||||
# rediscovered as an oversight: every `decide` exits immediately, so the reads cannot produce a
|
||||
# single self-contradictory message — only a later decision made on a fresher snapshot.
|
||||
vjson_base=$(gq "repos/$owner/$repo/commits/$sha/status?limit=100")
|
||||
# Same jq-1.6 rule as everywhere else in this file: check emptiness in SHELL first, never via
|
||||
# `jq -e`'s exit status over empty input.
|
||||
if [ -z "${vjson_base//[[:space:]]/}" ] || ! printf '%s' "$vjson_base" | jq -e '.statuses | type == "array"' >/dev/null 2>&1; then
|
||||
# VALIDATE EVERY FIELD THE EXTRACTION CONSUMES, on EVERY row — the same rule the file-enumeration
|
||||
# guard learned the hard way. Checking only that `.statuses` is an array left a hole one level
|
||||
# down: `{"statuses":[1]}` passes a top-level type check, then `.context` on a number errors, and
|
||||
# a `|| true` on the extraction turned that error into an empty `vdesc` — i.e. straight back onto
|
||||
# the graceful-adoption path this block exists to distinguish from. That is the identical
|
||||
# swallow-the-error shape fixed a few lines up, surviving one level deeper.
|
||||
if [ -z "${vjson_base//[[:space:]]/}" ] \
|
||||
|| ! printf '%s' "$vjson_base" \
|
||||
| jq -e '.statuses | type == "array"
|
||||
and all(.[]; type == "object"
|
||||
and (.context | type == "string")
|
||||
and (.description == null or (.description | type == "string")))' \
|
||||
>/dev/null 2>&1; then
|
||||
decide ask "H10 merge gate: could not read the commit statuses for PR #$pr head ${sha:0:7}, so the verdict could not be checked against the PR's base branch (ersatztv#632). Confirm the review covered the branch this PR currently targets ('$live_base') before merging."
|
||||
fi
|
||||
vdesc=$(printf '%s' "$vjson_base" \
|
||||
| jq -r '[.statuses[] | select(.context == "review-verdict/h10")] | first | .description // ""' \
|
||||
2>/dev/null || true)
|
||||
# No `|| true` here. The validation above makes an error unreachable, but a swallowed error would
|
||||
# be indistinguishable from "no base recorded" — the exact confusion this block removes — so the
|
||||
# failure is handled explicitly rather than left to a fallback that reads as a benign result.
|
||||
if ! vdesc=$(printf '%s' "$vjson_base" \
|
||||
| jq -r '[.statuses[] | select(.context == "review-verdict/h10")] | first | .description // ""' \
|
||||
2>/dev/null); then
|
||||
decide ask "H10 merge gate: the commit statuses for PR #$pr head ${sha:0:7} could not be parsed to find the review verdict, so it could not be checked against the PR's base branch (ersatztv#632). Confirm the review covered the branch this PR currently targets ('$live_base') before merging."
|
||||
fi
|
||||
# The field is written by scripts/post-review-verdict.sh as a trailing `(base: <ref>)`. Its
|
||||
# ABSENCE is the one benign case: a verdict posted before ersatztv#632 could not have carried it,
|
||||
# and denying those would block every in-flight PR the day this lands. The window closes on its
|
||||
|
||||
@@ -55,6 +55,12 @@ if "/status" in url:
|
||||
sys.exit(22)
|
||||
if desc == "GARBAGE":
|
||||
print('{"message":"internal error"}'); sys.exit(0)
|
||||
if desc == "SCALAR-ROW":
|
||||
print('{"state":"success","statuses":[1]}'); sys.exit(0)
|
||||
if desc == "NONSTRING-DESC":
|
||||
print(json.dumps({"state": "success", "statuses": [
|
||||
{"context": "review-verdict/h10", "status": "success", "description": {"x": 1}}]}))
|
||||
sys.exit(0)
|
||||
rows = [] if desc == "NONE" else [
|
||||
{"context": "review-verdict/h10", "status": "success", "description": desc}]
|
||||
print(json.dumps({"state": "success", "statuses": rows}))
|
||||
@@ -148,6 +154,23 @@ def test_a_verdict_with_no_recorded_base_gets_no_opinion(hook, desc):
|
||||
"a pre-#632 verdict drew a base-related decision for a field it could not have carried")
|
||||
|
||||
|
||||
@pytest.mark.parametrize("failure", ["SCALAR-ROW", "NONSTRING-DESC"])
|
||||
def test_a_malformed_status_MEMBER_asks_too(hook, failure):
|
||||
"""One level below the previous fix, and it survived it.
|
||||
|
||||
Validating only that `.statuses` is an array left `{"statuses":[1]}` passing the guard, after
|
||||
which `.context` on a number errors and a `|| true` on the extraction turned that error into an
|
||||
empty description — straight back onto the graceful-adoption path, which is precisely the
|
||||
outcome the guard exists to distinguish from. Same swallow-the-error shape as the bug one level
|
||||
up, which is why the validation domain must match the CONSUMPTION domain rather than stopping at
|
||||
the top-level type.
|
||||
"""
|
||||
hook.set_live_base("release/26.4")
|
||||
hook.set_verdict_description(failure)
|
||||
reason = hook.reason()
|
||||
assert "ask" in reason and "base" in reason
|
||||
|
||||
|
||||
@pytest.mark.parametrize("failure", ["TRANSPORT-ERROR", "GARBAGE"])
|
||||
def test_an_UNREADABLE_status_response_asks_rather_than_skipping_the_check(hook, failure):
|
||||
""""Could not check" is a third outcome, not a quiet synonym for "no base recorded".
|
||||
|
||||
Reference in New Issue
Block a user