timothy/ersatztv
1
0
Fork 0
Code Issues 54 Pull Requests 6 Actions Packages Projects Releases 9 Wiki Activity

Merge-gate branch-protection read: a malformed rule states a cause that did not happen, and the endpoint is now fetched twice #859

New Issue
Closed
opened 2026-08-27 22:01:24 +02:00 by timothy · 2 comments
timothy commented 2026-08-27 22:01:24 +02:00
Owner
Copy Link
Copy Source

Two residuals found by cold review during #787, both deliberately left out of that PR: the first is
pre-existing behaviour in code #787 only moved, and the second is cost rather than unsafety.

1. A rule missing both name fields becomes a valid empty name

scripts/lib/branch-rule-classifier.jq (extracted verbatim from the hook in #787 — byte-identical,
so this predates it):

(.branch_name // .rule_name // "")

An unreadable rule therefore becomes the empty string, a valid name that simply matches nothing.
Measured: a payload whose rule carries neither field classifies as none, so the scheduled path
DENIES while claiming "the full rule list was read and none matches" — about a list it did not
understand. That is the same states-a-cause-that-did-not-happen defect the surrounding comments were
written to fix, one field deeper.

Direction matters: on #787's caller this surfaces as nomatch → ask, which is safe. On the
pre-existing caller it is a deny with a false stated cause.

2. branch_protections is fetched twice on the scheduled path

#787 added a second read for the guard-scope freshness arm. On a scheduled auto-merge the endpoint is
now hit twice (measured: the test stub recorded 2 URLs) — an extra round trip inside the hook's
15s budget, plus a window between the reads. Both fail conservatively, so this is cost, not unsafety.
Reusing the first read's payload is the obvious fix; the reason it was not done in #787 is that the
two reads ask different questions (one about $base_ref, one about main), so sharing them needs
care rather than a variable rename.

Done-when

  • A rule lacking a usable name classifies as unreadable, never as a name that matches nothing
  • The deny arm's reason cannot claim a list was read and understood when it was not
  • A decision on the double fetch: reuse the payload, or record why two reads are correct
  • Tests cover both — a malformed-name payload, and whichever fetch shape is chosen
  • Adversarial review passed
Two residuals found by cold review during #787, both deliberately left out of that PR: the first is pre-existing behaviour in code #787 only *moved*, and the second is cost rather than unsafety. ## 1. A rule missing both name fields becomes a valid empty name `scripts/lib/branch-rule-classifier.jq` (extracted verbatim from the hook in #787 — byte-identical, so this predates it): ```jq (.branch_name // .rule_name // "") ``` An unreadable rule therefore becomes the empty string, a *valid* name that simply matches nothing. Measured: a payload whose rule carries neither field classifies as `none`, so the scheduled path DENIES while claiming "the full rule list was read and none matches" — about a list it did not understand. That is the same states-a-cause-that-did-not-happen defect the surrounding comments were written to fix, one field deeper. Direction matters: on #787's caller this surfaces as `nomatch` → **ask**, which is safe. On the pre-existing caller it is a **deny** with a false stated cause. ## 2. `branch_protections` is fetched twice on the scheduled path #787 added a second read for the guard-scope freshness arm. On a scheduled auto-merge the endpoint is now hit twice (measured: the test stub recorded 2 URLs) — an extra round trip inside the hook's 15s budget, plus a window between the reads. Both fail conservatively, so this is cost, not unsafety. Reusing the first read's payload is the obvious fix; the reason it was not done in #787 is that the two reads ask different questions (one about `$base_ref`, one about `main`), so sharing them needs care rather than a variable rename. ## Done-when - [x] A rule lacking a usable name classifies as `unreadable`, never as a name that matches nothing - [x] The deny arm's reason cannot claim a list was read and understood when it was not - [x] A decision on the double fetch: reuse the payload, or record why two reads are correct - [x] Tests cover both — a malformed-name payload, and whichever fetch shape is chosen - [x] Adversarial review passed
timothy added the ci-cdpriority: medium labels 2026-08-27 22:01:39 +02:00
timothy commented 2026-08-30 09:52:05 +02:00
Author
Owner
Copy Link
Copy Source

Claiming as a bundle with #858 — Claude Code session, worktree ~/orca/workspaces/ersatztv/main-3, branch fix/858-859-merge-consent-hook-residuals off origin/main @ 58681b3a7.

Same rationale as posted on #858: both issues are #787 cold-review residuals in the merge-consent hook and its classifier, so they are one session's work, not two.

Parallel-session note: I yielded #887 to an earlier claim this morning (three sessions collided on it); the 3-way split is recorded on #887.

Pre-claim checks clear: no remote branch matching *858*/*859*/*787*, no open PR referencing either, no prior comments, git fetch origin main → 58681b3a7.

Claiming as a **bundle with #858** — Claude Code session, worktree `~/orca/workspaces/ersatztv/main-3`, branch `fix/858-859-merge-consent-hook-residuals` off `origin/main` @ `58681b3a7`. Same rationale as posted on #858: both issues are #787 cold-review residuals in the merge-consent hook and its classifier, so they are one session's work, not two. Parallel-session note: I yielded #887 to an earlier claim this morning (three sessions collided on it); the 3-way split is recorded on #887. Pre-claim checks clear: no remote branch matching `*858*`/`*859*`/`*787*`, no open PR referencing either, no prior comments, `git fetch origin main` → `58681b3a7`.
timothy added the in-progress label 2026-08-30 09:52:13 +02:00
timothy commented 2026-08-30 13:34:46 +02:00
Author
Owner
Copy Link
Copy Source

Closing record

Outcome: Both items shipped in PR #897 (squash cf5f42edf). This issue was filed as a wrong stated cause and turned out to be masking a live false-open in the merge gate — that is the headline, not the wording fix.

Root cause: (.branch_name // .rule_name // ""). jq's // fires on null and false but not on "". Measured 2026-08-30 on a scratch repo against Gitea 1.27.1 (one rule of each kind, read back):

{"branch_name":"",     "rule_name":"release/*"}   <- GLOB rule
{"branch_name":"main", "rule_name":"main"}        <- plain rule

So a glob rule reached the classifier as the empty string — a name carrying no metacharacters — and the glob test, which is the whole basis of the undecidable-first ordering, never saw it. Measured on the predecessor:

payload (base main) old new
glob m* (no h10) + plain main (h10) exact → AUTO-GRANT undecidable → ask
glob m* alone none → DENY "none matches" undecidable → ask

The first row is #622's hole reached through the ordering written to close it: the gate reads the plain rule's contexts and arms a scheduled merge while Gitea, ordering by Priority then plain-name-ness, may be applying m*. The issue's own described defect (both name fields absent → "" → none → a deny stating a finding nobody established) is the same mechanism with a milder payload.

Item 2's root cause is simpler: #787 added a second consumer of the same endpoint and gave it its own GET.

Decisions/conventions changed: added process.hook-resolves-inputs-from-repo-root (from #858, same PR). No sched.*/api.* keys touched. docs/remote-state-inventory.md's row for the second read now describes one shared read.

Reusable knowledge:

  • Gitea reports a glob branch-protection rule with an empty branch_name. Any code reading that endpoint must take rule_name as canonical. jq's // will not fall through an empty string for you.
  • An arbitrary sample gives false negatives. My first "measurement" probed this repo, which has exactly one plain rule, and I generalised it to all rule shapes and wrote "not known to be reachable" into the code. Cold review refuted it from Gitea's source; constructing the positive case on a scratch repo settled it. Construct the positive case.
  • jq binds as eagerly. An arm placed first in an if chain does not run first if a binding above it already touched the bad value — the guard must be structural (nested in the else), not merely first.
  • An arm that sets the same sentinel as the catch-all is a no-op. Measured: deleting it left the whole suite green. Give it a distinct sentinel and a distinct cause, or do not write it.
  • grep on the dev Mac is ugrep: a pattern containing ${...} matches nothing and exits 0. Pass -F; cross-check any population count against a second derivation.

Verification: 1377 passed / 2 skipped; 11 declared mutants, 11 detected, disjoint reddened sets, including reverting each fix to its real predecessor; classifier executed over a 27-payload matrix on jq 1.8.2 and jq 1.6 (the CI floor) with identical results; live Gitea probed on a scratch repo, deleted afterwards. CI green first run (14 green, 1 skipped, 0 failures). Four cold review rounds + a bounded prose check.

Deferred: #891 (the other 12 hooks' sourced fire-log path — priority: high + security); #895 ("all N tests green" claims, 4 instances, candidate detector). Neither is a residual of this fix; both are classes this work surfaced.

Docs updated: docs/remote-state-inventory.md, docs/decisions/records/process/hook-resolves-inputs-from-repo-root.md (new), regenerated docs/decisions/README.md.

## Closing record **Outcome:** Both items shipped in PR #897 (squash `cf5f42edf`). **This issue was filed as a wrong stated cause and turned out to be masking a live false-open in the merge gate** — that is the headline, not the wording fix. **Root cause:** `(.branch_name // .rule_name // "")`. jq's `//` fires on `null` and `false` but **not** on `""`. Measured 2026-08-30 on a scratch repo against Gitea 1.27.1 (one rule of each kind, read back): ``` {"branch_name":"", "rule_name":"release/*"} <- GLOB rule {"branch_name":"main", "rule_name":"main"} <- plain rule ``` So a glob rule reached the classifier as the empty string — a name carrying no metacharacters — and the glob test, which is the whole basis of the undecidable-first ordering, never saw it. Measured on the predecessor: | payload (base `main`) | old | new | |---|---|---| | glob `m*` (no h10) + plain `main` (h10) | **`exact` → AUTO-GRANT** | `undecidable` → ask | | glob `m*` alone | `none` → DENY "none matches" | `undecidable` → ask | The first row is #622's hole reached through the ordering written to close it: the gate reads the plain rule's contexts and arms a scheduled merge while Gitea, ordering by Priority then plain-name-ness, may be applying `m*`. The issue's own described defect (both name fields absent → `""` → `none` → a deny stating a finding nobody established) is the same mechanism with a milder payload. Item 2's root cause is simpler: #787 added a second consumer of the same endpoint and gave it its own GET. **Decisions/conventions changed:** added `process.hook-resolves-inputs-from-repo-root` (from #858, same PR). No `sched.*`/`api.*` keys touched. `docs/remote-state-inventory.md`'s row for the second read now describes one shared read. **Reusable knowledge:** - **Gitea reports a glob branch-protection rule with an empty `branch_name`.** Any code reading that endpoint must take `rule_name` as canonical. jq's `//` will not fall through an empty string for you. - **An arbitrary sample gives false negatives.** My first "measurement" probed *this* repo, which has exactly one plain rule, and I generalised it to all rule shapes and wrote "not known to be reachable" into the code. Cold review refuted it from Gitea's source; constructing the positive case on a scratch repo settled it. Construct the positive case. - **jq binds `as` eagerly.** An arm placed first in an `if` chain does not run first if a binding above it already touched the bad value — the guard must be *structural* (nested in the `else`), not merely first. - **An arm that sets the same sentinel as the catch-all is a no-op.** Measured: deleting it left the whole suite green. Give it a distinct sentinel and a distinct cause, or do not write it. - **`grep` on the dev Mac is ugrep**: a pattern containing `${...}` matches nothing and exits 0. Pass `-F`; cross-check any population count against a second derivation. **Verification:** 1377 passed / 2 skipped; **11 declared mutants, 11 detected**, disjoint reddened sets, including reverting each fix to its real predecessor; classifier executed over a 27-payload matrix on **jq 1.8.2 and jq 1.6** (the CI floor) with identical results; live Gitea probed on a scratch repo, deleted afterwards. CI green first run (14 green, 1 skipped, 0 failures). Four cold review rounds + a bounded prose check. **Deferred:** #891 (the other 12 hooks' sourced fire-log path — `priority: high` + `security`); #895 ("all N tests green" claims, 4 instances, candidate detector). Neither is a residual of this fix; both are classes this work surfaced. **Docs updated:** `docs/remote-state-inventory.md`, `docs/decisions/records/process/hook-resolves-inputs-from-repo-root.md` (new), regenerated `docs/decisions/README.md`.
timothy removed the in-progress label 2026-08-30 13:35:23 +02:00
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
ad-hoc
One-off / ad-hoc work not tracked by a dedicated issue
 api
REST API / HTTP endpoints
 bug
Something isn't working
 ci-cd
Build, test, deploy pipeline
 content
Channel content / schedules / playlists
 dependencies
Dependency updates (Renovate)
 enhancement
New feature or improvement
 frontend
ChicoryTV React SPA frontend
 in-progress
Claimed by an active session — do not pick up
 jellyfin
Jellyfin tuner / IPTV integration
 parked
Excluded from automatic queue pickup; work only when explicitly selected
 priority: high
priority: low
priority: medium
review
Adversarial review finding
 security
Security / vulnerability fix
No labels ci-cd priority: medium
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
renovate timothy (Timothy)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: timothy/ersatztv#859