Round 5: raw HTML is the third code-block form. `<pre>`, `<code>` and HTML comments all render their
contents literally, so a verdict inside one is an example, not an approval:
<pre> / <code> / <!-- ... --> containing a verdict -> positive
Now stripped, tracked as a marker count rather than parsed — the direction of error is to strip MORE,
which can only ever withhold approval. Mutation-verified: removing the stripper fails all five cases.
AND THE HARDENING STOPS HERE, deliberately. The record now says so, because otherwise the next
session re-derives it: this is a best-effort heuristic, not a markdown parser. It covers the three
code-block forms markdown has (fenced; indented, via the column-0 rule; raw HTML) and is not proof
against every way to render text as non-prose.
Stopping is safe because the comment is NOT the load-bearing gate. Since #622 the authoritative
signal is the `review-verdict/h10` commit status, written only by post-review-verdict.sh from
explicit arguments — a comment cannot forge it. This classifier is condition (c) of the PreToolUse
hook: defense in depth on an agent's merge call. A residual false-open means the hook does not
object; it does not mean a merge happens.
Five rounds found five code-block forms, four of them introduced while fixing the previous round.
The generalisable rule, now in the record: when a heuristic keeps failing at the edges, check whether
it is actually the thing enforcing the invariant before spending another round on it.
Also measured, against the real corpus: a "verdict must be the first line" rule would have killed
every code-block form at once, but 14 of 18 verdict markers ever posted in this repo are NOT on the
first line — so it was rejected as a retroactive break, not deferred.
178 tests. refs #629
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Decisions-Edit: yes
Round 4 found the last false-open: markdown has a SECOND code-block form the fence stripper does
not cover — indented blocks (4 spaces or a tab). A pasted indented example still self-approved:
Example:
Review-verdict: MERGEABLE @ <head> -> positive
Adding an indented-code stripper would be the same move that produced rounds 2, 3 and 4: fixing the
instance, not the class. So the grammar is narrowed instead — the marker must sit at COLUMN 0. That
kills every indentation-based ambiguity at once (4-space, tab, list-nested, arbitrary indent).
Cost, accepted deliberately: a verdict indented under a list item is now ignored and classifies
`absent`, which asks a human. For a gate, erring toward ignoring is the safe direction. Fence
detection KEEPS its leading-whitespace tolerance, because stripping more is always safe.
`test_leading_indent_is_tolerated` asserted the old behaviour and is replaced by
`test_falseopen_an_indented_verdict_is_not_a_verdict`, parameterised over four indent shapes and
mutation-verified: restoring `^[[:space:]]*` fails all four, control green. 172 tests.
Round 4 verified clean by execution: the rc plumbing fails closed for a forced failure in the inner
jq, awk, grep AND the pipeline producer (rc 3/4/5/93 -> exit 2); fence-length semantics, mismatched
markers, CRLF fences, 10-marker fences, blockquote fences; emoji, CRLF, a 120k line, 200 comments;
NUL rejection with no JSON-encoding bypass.
refs #629
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Decisions-Edit: yes
Third review round, third set of real findings. Both reproduced before fixing.
1. High: markdown closes a fence only with N-or-more of the SAME marker it was opened with, so a
```` block legitimately CONTAINS a ``` line as content. Toggling on any 3+ marker left the
fence at that inner line and graded the verdict below it as a real approval:
````text / ``` / Review-verdict: MERGEABLE @ <head> / ```` -> positive
Now tracks the opening marker's character and length; a shorter or different marker while a
fence is open is content, so it neither closes the fence nor escapes it.
2. Medium: `awk ... | grep ... || true` flattened "no match" (grep rc 1, normal) together with a
real tool failure (rc >= 2). A failing reader produced no verdict lines at all — `absent` —
silently discarding a real BLOCKED verdict. awk and grep are now checked separately, and only
"no match" is tolerated.
Also fixed while writing (2): `[ rc = 0 ] && printf` as the loop body's LAST command would leave
the subshell exiting 1 whenever the newest comment carried no verdict, which the rc check would
then report as a failure to read comment bodies — an ordinary PR reading as broken. Uses an `if`.
169 tests. Both findings mutation-verified: restoring the naive fence toggle fails all four
longer-fence cases, restoring `|| true` fails the failing-grep case, control green. Verified the
ordinary shapes still work: plain ``` and ~~~ fences and lang-tagged fences still stripped, an
unclosed fence still swallows, a real verdict beside a fenced example still counts, and a fenced
positive alongside a real BLOCKED still classifies negative.
refs #629
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Decisions-Edit: yes
Re-review of 8d0df553 returned BLOCKED. Both findings are the same class as the bugs this
issue is about, one level down: the reader, not the grammar.
1. A here-document makes bash materialise a temp file. When that fails (read-only or
restricted TMPDIR) the loop reads NOTHING, and the classifier returned `absent` — silently
discarding a real BLOCKED verdict. Reproduced with TMPDIR=/nonexistent: `absent` before,
`negative` now. Both loops are off here-docs (a pipe for the reader, process substitution
for the classifier so its flags stay in the current shell).
"The environment could not supply a temp file" is not evidence that a PR was approved.
2. A malformed payload degraded to `absent` instead of an input error: an object-valued
`.body` reached the scanner and simply matched nothing. Shape is now asserted in jq — must
be an array, bodies must be strings — so it exits 2 and callers fail closed.
Also closed a pre-existing false-open the review found while probing (present before #629, so
not a regression, but cheap to fix here): bash strips NULs in command substitution, so a body
of `Review<NUL>-verdict: MERGEABLE @ <head>` arrived at the matcher as a valid verdict line —
text that is not a verdict becoming one in transit. Bodies containing NUL are now rejected.
161 tests. Verified: 80k body still classifies; empty array, null body still `absent`;
malformed JSON, non-array, array-of-non-objects, object body, NUL body all exit 2; a hostile
TMPDIR still returns `negative`.
refs #629
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cross-family review of 38a96f47 returned BLOCKED with three findings. All reproduced first:
~~~ fence -> positive fences were stripped for ``` only; markdown also takes ~~~
@ <40hex>f / ZZZ -> positive the sha matched {7,40} with NO right boundary, so an
over-long or malformed token was TRUNCATED into a passing one
\x01BODY-BOUNDARY\x01 -> positive an in-band separator joined comment bodies, so a body
containing that line forged a boundary, reset fence state
mid-comment, and exposed a verdict inside an unclosed fence
Fixes: both fence markers honoured; the hex run matched whole, required to end at a
non-alphanumeric boundary, with its length validated separately so an out-of-range token is
rejected rather than trimmed to fit; and bodies carried OUT-OF-BAND (one JSON-encoded string
per line), which removes the forgery class instead of escaping the sentinel.
The third is the one worth remembering: an in-band delimiter is forgeable by whoever writes the
data, and here that is anyone who can comment on the PR.
Two of these fixes broke previously-green tests, both of which were right to break:
- an over-long token now classifies `no-sha`, not `stale`. The fixture asserting `stale` was 45
hex chars, so it had been exercising the length guard while claiming to test the prefix rule.
Rebuilt as a well-formed 40-char sha that contains the head prefix without starting with it.
- `jq -e` exits 4 when a filter produces NO output, which is the legitimate empty-comment-list
case. Treating it as an error turned "no comments yet" into an input error — and callers fail
closed on those, so a new PR would have read as unclassifiable. Exit 4 is now accepted.
44 classifier tests, 155 total. `~~~` and the boundary fixes are each mutation-verified; the
out-of-band fix has no equivalent mutation (it is structural, not a regex) so its evidence is the
direct reproduction against the previous commit.
refs #629
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Decisions-Edit: yes
The H10 classification lived inline in `pretooluse-merge-consent.sh` with no tests. Three
protections the `release.review-verdict-gate` record described were never actually
implemented, and each graded an unreviewed head as approved. All three reproduced first:
1 MERGEABLE-LATER -> positive the token was prefix-matched, so any word STARTING
with mergeable/approved/lgtm passed
2 fenced code block -> positive the line-start anchor is satisfied inside ```, so
documentation showing the convention was a verdict
3 URL-borne sha -> positive the sha came from the first `@<hex>` ANYWHERE on the
line, so a markdown link could supply it
Fixes: whole-word token matching, with a token in neither vocabulary classified `unknown`
(never positive, and not guessed into a block either — it goes to a human); fenced blocks
stripped with fence state reset per comment body; the sha read from the verdict's OWN
`@ <sha>` field, which also makes multi-`@` lines unambiguous.
The grammar moves to `scripts/check-review-verdict.sh` so it can be tested at all — 38 tests,
and each fix mutation-verified: restoring the old regex/extraction makes exactly the
corresponding test fail, control green.
#629's fourth reported item is NOT a defect and is not claimed as a fix. A later `@ <head>`
on a BLOCKED line was reported as "masking a negative"; under the documented grammar that
line is a verdict for the sha in its own field, so `stale` is correct — and was correct
before this change too. Kept as a characterization test.
`test_post_review_verdict.py`'s cross-check re-implemented the hook's regexes in Python and
asserted the shell still contained them. That mirror is removed: it is the same duplication
that let these three survive, and a Python copy would keep passing while the shell drifted.
It now runs the real classifier.
The decision record is corrected — it asserted the URL protection this commit actually adds.
Note: the active corpus is 5637 lines against a 5600 budget, so the validator emits its
consolidation warning (non-blocking). That is #620's subject, not regressed here.
fixes#629
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Decisions-Edit: yes
Two LOWs from the re-review of af39123e. Both are the same defect I have already
corrected twice on this branch: a universal claim that the corrected body right
below it disproves. These read as normative, so a reader who stops at the title
gets the false version.
- The record was titled "...and every wrapper says so" while its own body admits
the 12 OpenAPI pageNum parameters carry no description at all (tracked as
#633). Retitled to state the target and name the exception.
- docs/mcp.md led with "Rows that reference another entity carry that entity's
id" — but PlayoutListItemResponseModel.ScheduleName ships with no schedule id.
Restated as a rule about ACTIONABLE references, matching the wording the record
already uses, with the known exception named.
The two remaining LOWs are accepted deferrals, not fixed here: the >100 rerun
collection truncation needs paging-to-completeness plus a multi-page fixture
(#634), and the catalog test's forward-looking gap (it compares tool names, not
PathTemplate against the paged-endpoint set) would need the MCP test project to
reference the controllers to close properly.
Docs only; catalog regenerated. No behaviour change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Findings from the cold cross-family review of 8d35a279/5648f8e9. The review
confirmed the three conclusions in 8d35a279 (the offset math is sound on all 12
paged endpoints; the list DTO really did already carry ChannelId; the new tests
are non-vacuous) but found the previous commits had documented a convention
without checking who actually violates it.
HIGH — a live instance of this issue's own bug class, in the SPA.
web/src/screens/SchedulesScreen.tsx asked for the rerun-collection picker with
`pageNum: 1, pageSize: 1000`. pageNum is 0-based and pageSize clamps to 100, so
the request skipped the first 100 rows: with <=100 rerun collections (the normal
case) the picker was served an EMPTY page and silently offered no rerun
collections at all; above 100 it dropped rows 1-100. Exactly the silent-short-set
failure #616 is about, shipped in the UI. Now `pageNum: 0`, with a vitest that
asserts the offset and is mutation-verified (restoring `pageNum: 1` fails it).
Checked the rest of the SPA rather than assuming: every other pageNum caller is
0-based. CollectionsScreen's paging loop starts at 1 but only after fetching
page 0 explicitly, so it is correct — verified before touching it.
MEDIUM — two MCP tools wrapped paged endpoints while declaring no paging args.
ersatztv_list_playouts and ersatztv_get_playout_items had no pageNum/pageSize,
and ToolArgumentValidator rejects undeclared arguments, so an agent was hard
capped at the first 100 rows with no way to ask for more and no error saying so.
Both now take Page(). A catalog-wide sweep confirmed these were the only two:
the other paged endpoints are not exposed as MCP tools at all.
That same gap made the new ToolCatalog test vacuous in the direction that
mattered — it filtered on tools that ALREADY declare pageNum, so a tool missing
paging entirely escaped it. It now pins the expected set by name, so a new tool
over a paged endpoint has to be added deliberately.
Record corrections (these are read as normative, so over-broad claims are
defects): "every wrapper says 0-based" was false for the OpenAPI surface, whose
12 pageNum parameters carry no description — named as a remaining gap instead of
claimed as done. "Every controller floors with Math.Max" ignored
SearchController's Math.Clamp. The ids-in-rows corollary was stated as an audit
result when PlayoutListItemResponseModel.ScheduleName has no schedule id;
restated as a rule about actionable ids.
Verification: .NET 1900 + MCP 59 pass; web 996 pass across 105 files; tsc clean;
eslint clean; format gate exit 0; no BOMs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Self-review of the previous commit caught an overclaim I introduced. I wrote the
paging docs as if the pageSize cap were uniformly 100. It is not:
most reads (collections, library browse, logs, playouts) Math.Clamp(.., 1, 100)
GetAutoTuneChannelMembers pageSize <= 0 ? 100 : Min(.., 200)
GET /api/v1/search/all-items default 500, cap 1000
That made a concrete example in docs/mcp.md simply false. It claimed
`pageSize=500&pageNum=1` returns items 101-200 "not 501-1000" — but
ersatztv_search_all_items also takes Page(), and 500 is UNDER its cap, so there
page 1 really is items 501-1000. A caller following that example on the one tool
most likely to be paged hard would have mis-derived its offsets, which is the
same class of silent-short-set error this issue is about.
The invariant that actually holds everywhere is the derivation, not any single
cap: the offset comes from the EFFECTIVE (bounded) page size, never the
requested one. Reworded to say that, in docs/mcp.md, the api.paging-zero-based
record (rule + mechanics + body), and the ToolCatalog Page() comment. Catalog
regenerated. The record's mechanics line no longer claims every controller uses
Math.Clamp — ChannelController does not.
No behaviour change; MCP suite still 59/59.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fourth review round, two Low findings, both in the condition-(a) message change
I added last round. Both are message-only — every path still denies — but both
would have printed something flatly false at the moment someone is trying to
understand why a merge is blocked.
1. The filter selected only `pending` and `failure`, but Gitea also has `error`
(and `warning`). With `review-verdict/h10=pending` and a build in `error`, the
errored build fell out of the set, leaving the verdict as the lone entry — and
the hook then claimed "every CI check is green" while a build was erroring.
Non-green is now anything that is not `success`.
`skipped` is deliberately still counted as GREEN: the image-push job skips on
every PR (#593 — a skipped context is not red), so treating it as non-green
would have permanently suppressed the H10-specific message in the one
situation it exists for.
2. If the sole non-green context was `review-verdict/h10=failure` — i.e. someone
reviewed this head and REJECTED it — the message said no verdict existed yet
and told the reader to post MERGEABLE. It now branches on the verdict's own
state: pending means nobody has reviewed this head, failure/error means it was
reviewed and rejected, so resolve the findings.
Also folds the two combined-status fetches into one.
Verified against eight payloads: verdict-only-pending, verdict-pending-plus-error,
verdict-failure-alone, skipped-plus-verdict-pending, two-pending, a real build
failure, a warning state, and an unreadable body — each producing the intended
message and no other.
Both review channels independently confirmed this round that the rename
validation is correct at both call sites, that the tests meaningfully pin it, and
that no ALLOW/GRANT path became more permissive.
#616 filed three MCP/API paging traps. Two were real; one was not, and one was
already half-fixed on main. Verified each against the code before changing it.
REAL — pageNum documented as 1-based. `ToolCatalog.Page()` described pageNum as
"1-based page number" while every paged controller defaults it to 0, floors it
with `Math.Max(0, pageNum)`, and skips `PageNum * PageSize`. A caller that
trusted the description started at page 1 and silently lost the first page: no
error, just a short set that reads as data loss rather than an off-by-one (it
cost #487 a verification pass). Fixed in the description rather than by making
the MCP layer 1-based: /api/v1 is additive-only post-freeze, 0-based is
load-bearing in a dozen controllers and the SPA, and a 1-based wrapper over a
0-based API would make the same parameter name mean two different things on two
surfaces a reader reads together.
NOT REAL — "pageSize caps the page but the offset honors the requested value".
Not reproducible on any endpoint. Every controller clamps before passing, every
handler skips by the clamped size, and GetCollectionItemsHandler re-clamps
defensively. The reported observation (pageSize=500&pageNum=2 on a 204-item
collection returning 4 items) is exactly correct 0-based behaviour at the
clamped width of 100 — page 2 is items 201-204. The issue's own trap-1 table
states this. Pinned by test rather than "fixed".
ALREADY FIXED — playout LIST rows gained channelId in #297 (2026-07-22), three
days before #616 was filed; the report was measured against prod, which runs an
older :prod image. The DETAIL response (PlayoutResponseModel) genuinely still
lacked it, so channelId is added there (additive) and the reset_channel_playout
argument now names the trap: the id spaces overlap numerically, so passing a
playout id silently resets a different channel and returns a plausible 202.
Tests, both mutation-verified (each fails when its fix is reverted):
- ToolCatalogTests pins "0-based" on EVERY paged tool's pageNum description,
with a non-empty guard so it can't pass vacuously over an empty tool set.
- GetCollectionItemsHandlerTests pins 0-based page boundaries and proves the
offset derives from the clamped pageSize (page 1 at pageSize=500 returns
items 101-150; the mutation that honors 500 returns an empty page).
Docs: new decision record api.paging-zero-based (catalog regenerated), the
api-conventions paging bullet, and a Paging section in docs/mcp.md. OpenAPI
v1.json + web/src/api/generated/v1.d.ts regenerated for the added field.
fixes#616
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Third BLOCKED verdict, third legitimate finding. The per-row guard validated the
DESTINATION only, so `{"filename":"docs/x.md","status":"renamed"}` with no
`previous_filename` passed while its source silently vanished — precisely the
path-hiding that collecting both sides exists to prevent. A rename row must now
carry a non-empty `previous_filename`.
Required for rename rows ONLY. Demanding it globally would reject every ordinary
modified/added row and make the gate refuse all exemptions — which every
"withholds the exemption" test would happily pass through, so that direction gets
its own positive control. Predicate checked against ten shapes before adopting;
mutation-verified in both directions (dropping the clause fails 2 tests, applying
it globally fails 3 including the controls).
Also fixes a wart this PR introduced. `review-verdict/h10` is itself folded into
the COMBINED status, so a PR awaiting its verdict reports combined 'pending' and
the hook's condition (a) reported it as a CI problem — sending a reader to build
logs when the missing thing is the review, and exiting before the H10 branch that
would have said so. The message now names the outstanding contexts, and says
plainly when the verdict is the only one left.
Scope boundary, stated in the record: real Gitea populates `previous_filename` on
renames and returns well-formed pages. Everything past this point defends against
shapes with no evidence of existing, so the guard's claim stays "any page we
cannot fully classify withholds the exemption" rather than growing to cover
unobserved responses.
Decisions-Edit: yes
Second BLOCKED verdict, also correct. My previous fix validated only the
TOP-LEVEL type of each page, which left the same hole one level down: `[{}]` is
a well-formed JSON array whose rows carry no `filename`, so it contributes zero
paths, looks like a short final page, and completes the enumeration from a
PARTIAL list — the exact failure the guard exists to prevent, just wearing a
valid-looking wrapper. An array of scalars additionally tripped `set -e` during
extraction without producing a decision.
Both call sites now require every row to carry a non-empty string `filename`.
An empty array remains valid, since that is what a real end-of-pagination is.
Predicate checked against nine shapes before adopting: `[]` and well-formed rows
valid; `[{}]`, `[{"filename":""}]`, `[{"filename":null}]`, `["x"]`, `[1,2]`, a
bare error object, and a mixed good/bad array all rejected.
The reviewer also caught that my `test_first_page_failure_*` was VACUOUS: with
page 1 failing the path list is empty, and the hook independently withholds the
exemption on an empty list, so it passed with the guard removed. It is kept as a
smoke case with that limitation stated in its docstring, and three tests that
actually pin the behaviour were added — malformed rows and scalar rows after a
FULL page 1 (so the path list is non-empty and the exemption would really fire),
plus MAX_PAGES exhaustion.
Added a positive control too: an empty final page must still exempt. Without it,
an over-strict guard would make every "withholds the exemption" test pass for the
wrong reason — the suite would be asserting "never exempt", which is not the
contract. Verified by mutation: top-level-only validation fails 2 tests,
an over-strict guard fails the positive control, and widening MAX_PAGES fails the
exhaustion test.
Decisions-Edit: yes
Cross-family re-review returned BLOCKED on the fix commit, correctly. Two of the
three findings were on the FAILURE path, which is why every happy-path run and
the whole first test suite looked fine.
1. (High/Medium) Both paging loops treated a failed or non-array page as a
legitimate short final page: empty output counts as zero rows, zero rows reads
as "end of list", and the enumeration completed from a PARTIAL list — failing
OPEN at exactly the point the guard exists to fail closed, while the decision
record claimed the opposite. Each page is now validated as a JSON array before
its rows are counted; anything else withholds the exemption.
2. (High) The workflow's "is there already a verdict" pre-check read
`/statuses/{sha}`, which returns one row per status POST rather than per
context and pages at 50 — so a head with a few CI reruns can push a real
verdict off the first page (this PR's head already carries 15 rows for 11
contexts). Missing it there is not benign: the job would post `pending`, or an
exemption `success`, OVER a human verdict. Both the workflow and the hook now
read `/commits/{sha}/status` (latest-per-context), and the workflow refuses to
post anything when that read fails rather than inferring "no verdict yet".
Credit: raised independently by the parallel session on this issue.
3. (Low) The record claimed both callers withhold the exemption when enumeration
cannot complete. True only after (1); the claim and the code now agree.
Adds scripts/tests/test_merge_consent_exemption.py — 9 tests over the hook's
exemption path via a curl shim, asserting behaviour when a page ERRORS or returns
a non-array body, not merely when it returns data. Mutation-proven: reverting the
array validation fails both failure-path tests; dropping `previous_filename`
fails the rename test.
One test I wrote was wrong and the run caught it: I asserted a
`.gitea/... -> docs/...` rename must break the hook's exemption, but that hook
deliberately exempts `.claude/`/`.gitea/`/`.husky/` too (to a human PROMPT, never
an auto-grant), so both sides legitimately match. The rename guard matters there
for a genuinely non-exempt source; the stricter PROTECTED semantics belong to the
workflow. Both cases are now pinned, including a positive control that a
docs->docs rename stays exempt.
Decisions-Edit: yes
The workflow this PR introduces does not exist on `main`, yet its job ran on
#630 and posted a status — so Gitea runs `pull_request` workflows from the PR
HEAD. Limitation 4 was written as a conditional ("if Gitea runs..."); it is now
a confirmed property, which makes PROTECTED definitively a guardrail against
accident rather than a control against a PR that rewrites the classifier.
Same run also confirms the good case: #630 was correctly judged non-exempt on
its `.claude/`+`scripts/` paths and got `review-verdict/h10=pending`.
Decisions-Edit: yes
`pretooluse-merge-consent.sh` proves all three consent conditions at the moment
the merge tool is called. With `merge_when_checks_succeed=true`, condition (a) is
delegated to Gitea, which then merges whatever head is green at ITS merge time —
while (b) Done-when and (c) the head-referencing verdict were proven against the
head at SCHEDULING time. Every commit pushed in between merges unreviewed. The
gate is not bypassed; it is satisfied against a snapshot that stops being true.
Demonstrated as a controlled A/B rather than inferred, with a CI check left
pending so Gitea waits as it really does:
without a required verdict context: unreviewed commit B MERGED
with it: same sequence REFUSED
after reviewing B and posting it: merges — blocked, not deadlocked
NOTE the anecdote in #622 is wrong and is corrected in the docs rather than
repeated: PR #619 does carry `Review-verdict: MERGEABLE @ 02c82b35`, posted six
seconds before the merge, explicitly re-reviewing the follow-up commits. #622 was
filed off a lagging API read. The hole is real regardless, and structural —
nothing FORCED that re-review inside the window Gitea would have merged in. This
turns a property that held by diligence into one that holds by construction.
The fix is the sha, not a smarter check. A Gitea commit status belongs to exactly
one commit, so a status written for a parent cannot be inherited by a child that
did not exist yet. `review-verdict/h10` becomes a REQUIRED status check on main:
push a new head and the context is simply absent, which Gitea reads as
not-passing (verified against 1.25.4: missing AND pending both block, and
auto-merge re-checks the current head). It also covers merge paths the hook never
sees — Gitea UI, raw API, another agent's session.
- scripts/post-review-verdict.sh writes the comment and the status together so
they cannot drift, and re-reads head after commenting: if a commit landed
mid-flight it writes NO status and exits non-zero rather than retargeting the
verdict at a commit nobody read.
- .gitea/workflows/review-verdict.yml auto-passes the two exempt classes that
would otherwise deadlock — Renovate-authored (platformAutomerge) and docs-only
— and marks everything else pending. Exemptions are void when the PR touches
.claude/, .gitea/, .husky/, scripts/ or docker/ci/.
- The hook refuses to SCHEDULE an auto-merge unless that status is green on head,
and no longer claims "CI green" on the mwcs path, where it never read CI.
Two silent false negatives in the exemption path, both found by verifying rather
than reasoning, both fixed at BOTH call sites (workflow and the hook's
pre-existing docs-only carve-out):
- The files endpoint caps at 50 rows and IGNORES a larger `limit` — PR #619 has
194 changed files and `?limit=100` returns 50. A single page saw ZERO protected
paths there where the full enumeration finds ten. Both now page to exhaustion
and withhold the exemption if they cannot complete.
- A rename is ONE row whose `filename` is the destination, the source only in
`previous_filename`. Verified live: `.gitea/workflows/renovate.yml` ->
`docs/innocuous-note.md` presented as docs-only with no protected path visible.
Both now read BOTH sides.
Limits are documented, not papered over: base changes leave the head sha (and so
the verdict) unchanged, and a PR editing the workflow is judged by its own edited
copy — so PROTECTED is a guardrail against accident, not a tamper-proof control.
fixes#622
The gate went red three times in CI with three distinct root causes (stale
pooled session after a drop, lost isolation from a shared database name,
connect-before-create). An intermittently-red gate is worse than none: it
trains everyone to re-run instead of read, which is how the two collation
defects escaped in the first place. The production fix is reviewed and green,
so it should not stay blocked behind test-harness reliability.
The fixture is kept and stays opt-in via ETV_TEST_MYSQL_CONNECTION (visible
skip without it); only the CI wiring is removed, with a note where it belongs.
Decision record corrected — it described a CI step that no longer exists.
[decisions-edit]
#610 split docs/decisions.md into one YAML-frontmatter file per record while
this branch was open, so the inline record could not be merged -- it had to be
converted. Same content and metadata, now at
docs/decisions/records/scan/libraryfolder-unique-identity.md with an index
entry and a regenerated catalog.
Second CI failure of this fixture, and my previous fix caused it: collapsing to
one shared database traded isolation for a wipe that has to succeed, and when it
silently did not, the second test seeded onto the first's rows and failed with
'Duplicate entry 1 for key LibraryPath.PRIMARY' in 539ms - too fast to have
re-run the migration chain, i.e. MigrateAsync no-opped against an already-current
__EFMigrationsHistory.
The coordinator's read was right and I verified it rather than assuming: the leak
came from never clearing pools, not from names being unique. Harness against a
real 8.4 server, unique database name per iteration WITH ClearPoolAsync on that
connection string: 0 leaked threads over 30 iterations. So isolation costs
nothing and the shared name was solving a problem pool-clearing already solved.
Restore a fresh etv491_<guid> database per test, never created out of band (the
test's own MigrateAsync(PreviousMigration) creates it, keeping EF the single
owner of the schema), dropped in TearDown via the guarded EnsureDeletedAsync and
followed by ClearPoolAsync on that exact connection string. The stale-session
hazard from the first failure needs the connection string to be REUSED after the
drop, which a never-repeated name makes impossible; clearing the pool is the belt
to that brace and closes the leak.
Verified: 10 consecutive runs 10/10 green, server threads flat at 2 and zero
leftover schemas throughout; each test run ALONE twice; both tests with the order
REVERSED - order-independence being the evidence this failure would have needed,
since contamination is invisible when a test runs first. Negative control
re-confirmed after the change: restoring COLLATE utf8mb4_bin still fails with
survivors [1,4,5,6,7,9] vs [1,4,5,6,7,9,10], then restored and green again.
No Retry anywhere. One shared fixture body across providers; still fail-closed
under ETV_REQUIRE_MYSQL_TESTS.
Refs #488#308fix#491
The new MySql coverage failed intermittently in CI with 'Unknown database
etv491_...' at connect time, and passed on the immediately preceding commit -
a latent race in the fixture, not a regression. Diagnosed against a real 8.4
server rather than by inspection; two measured mechanisms:
1. A pooled session outlives DROP DATABASE. Reopening the dropped database's
connection string SUCCEEDS, because MySqlConnector hands back the still-alive
session whose default schema no longer exists. Whether a caller sees success
or 1049 then depends on whether the pool reuses that session or opens a fresh
one - a fresh handshake names the dropped schema and fails. That is the
non-determinism, and it is invisible in any single green run.
2. A per-test database name leaks a pool per test. MySqlConnector keys pools by
connection string, so each test built a new pool that was never cleared,
leaking ~1 server thread per iteration (measured); hammering it exhausted
max_connections outright, and a saturated server also inflated this fixture
from ~1m20s to 13 minutes.
Fix is structural, not a retry: ONE fixed database name (hence one pool), EF owns
the lifecycle via EnsureDeletedAsync (guarded, unlike a raw DROP), the pool is
cleared after every drop, and the drop moves to the START of a test so no test
leaves a live pool pointing at a database it just destroyed. ServerVersion is
resolved once per SetUp instead of twice.
Determinism verified: 10 consecutive runs, 10/10 green, server threads flat at 2
throughout. Negative control re-confirmed after the change - restoring COLLATE
utf8mb4_bin still fails with survivors [1,4,5,6,7,9] vs [1,4,5,6,7,9,10].
Both providers still share one fixture body; still fail-closed under
ETV_REQUIRE_MYSQL_TESTS.
Refs #488#308fix#491
Review follow-up. The new MySql-half members introduced CS8600/8604/8625/8618
and were inconsistent with this same file's established late-init pattern
(_databasePath = null!). Nullable-annotate the genuinely-nullable ones and
use null! for the Dapper-populated row properties.
The dedupe DML had zero automated coverage on MySql: the migrations job only
applies migrations to a fresh EMPTY database, so no dedupe row ever executed
there. Two MySql-only collation defects escaped that gate in this session and
were caught only by hand-run containers.
Parameterize LibraryFolderDedupeMigrationTests over both providers from ONE
fixture body - same seeded rows, same expected survivors - rather than adding a
MySql-only copy that would drift and recreate the gap. Assertions no longer use
WHERE Path = '...', which is itself collation-dependent and would quietly mean
something different per provider; rows are read once and compared ordinally in
memory. A new step in the existing migrations job runs it against that job's
mysql:8.4 service, on a per-test database of its own.
Proven red when the collation is wrong: restoring COLLATE utf8mb4_bin fails the
MySql half with survivors [1,4,5,6,7,9] - the trailing-space sibling deleted -
while SQLite stays green. Proven non-skippable: without
ETV_TEST_MYSQL_CONNECTION the fixture ignores visibly, and with
ETV_REQUIRE_MYSQL_TESTS=1 (which CI sets) that skip becomes a hard failure, so
it cannot pass having connected to nothing. Local runs need no MySql.
Also correct an overstated comment. The schema pins only the utf8mb4 charset,
never a collation, so the effective comparison is the server default: always
case-insensitive, but PAD SPACE only on utf8mb4_general_ci - 8.4's default
utf8mb4_0900_ai_ci is NO PAD, verified on the real column. The migration bug was
independent of that because the old code applied an EXPLICIT utf8mb4_bin, which
is PAD SPACE everywhere; the runtime simply tolerates both.
Refs #488#308fix#491
Cross-family review of 1b4dd6d6 found that utf8mb4_bin - chosen to keep the
dedupe case-exact - is a PAD SPACE collation, so trailing spaces are
insignificant under it. Verified on MySQL 8.4: '/media/Foo' = '/media/Foo ' is
TRUE, while case correctly compares unequal. Two distinct legal directories
therefore grouped together and the second was DELETED irreversibly, even though
PathUtils.GetPathHash hashes them differently and the unique index about to be
created would have accepted both. The dedupe destroyed data the constraint
never required it to destroy.
Group and join on CONVERT(Path USING binary) instead - NO PAD and byte-exact,
matching the hash. utf8mb4_0900_bin is also NO PAD but carries a server-version
floor. This is the only path comparison in either migration (every other
predicate keys off an integer id), so there is no mix of padded and unpadded
comparisons across the keeper-selection, repoint and delete steps.
SQLite's = on TEXT is byte-exact with no padding, so that migration was already
correct - which is exactly why a SQLite-only test could not see the divergence.
The two providers are now semantically equivalent, and the dedupe fixture is
shared: same rows, same expected survivors (1,4,5,6,7,9,10), asserted by the
SQLite test and reproduced by hand on MySQL 8.4.
Runtime was never affected, and this is now stated and tested rather than
assumed: GetFolder's SQL equality is a superset narrowing (both collation quirks
make it more permissive, never less, so it cannot miss a byte-exact match) and
ResolveExact settles identity with StringComparison.Ordinal, which compares
length first. Added ResolveExact coverage for the trailing-space axis.
Refs #488#308fix#491
Review follow-up. ResolveExact was inserted between GetFolder's summary
and GetFolder itself, leaving ResolveExact with two summaries and
GetFolder with none -- so the H1 rationale (SQL equality is only a
narrowing filter; OrderBy(Id) for determinism) described neither of the
things ResolveExact does. On a fix that turns on exactly that reasoning,
a misattached explanation is what misleads the next reader.
Final low-severity items from the re-review of ee10f932.
L2: DbUpdateConcurrencyException derives from DbUpdateException but carries no
provider exception, so IsUniqueConstraintViolation does not classify it. A row
deleted by a concurrent library edit between the heal's read and its save would
propagate and fail the scan, contradicting the invariant stated directly above
it. Admit it in the filter.
L1: lift the in-memory ordinal settle into LibraryRepository.ResolveExact and
unit-test it with both spellings in the candidate list. No SQLite-backed test
can exercise it (SQLite's = on TEXT is already binary), so this converts the
half that rested on hand-run MySQL evidence into automated coverage. The
end-to-end companion test's comment no longer claims to be provider-independent.
L4: assert PRAGMA foreign_keys is 1 before migrating, so the enforcement guard
cannot silently degrade into the weak pre-fix form it was added to replace.
L3: detach the failed heal, matching the insert path.
N3: the heal's inner predicate now matches its IsNullOrEmpty outer guard, so a
PathHash = '' row cannot enter the branch and silently never heal.
N4: record that GetFolder returning null for a case-differing spelling makes
MySQL insert a second row where it used to reuse one — correct, and now matching
SQLite, but a real behaviour change on a case-insensitive filesystem.
Refs #488#308fix#491
Review of 491f5099 found the fix inert in the only process that runs it, plus
a MySQL collation defect in the lookup.
B1 — TvContext.IsUniqueConstraintViolation was assigned only in ErsatzTV/
Startup.cs, but ErsatzTV.Scanner is a separate executable and every production
caller of GetOrAddFolder/SetEtag lives there. The classifier kept its '_ =>
false' default, so the catch never ran and the DbUpdateException failed the
whole scan - worse than the duplicate row it replaced. Wire both provider
branches in ErsatzTV.Scanner/Program.cs, and add ProviderStaticsWiringTests
(architecture) asserting the scanner assigns every TvContext static the host
assigns, with IsSqlite documented as the one exemption.
H1 — GetFolder's 'Path == folder' is case-insensitive on MySQL while PathHash
is case-sensitive, and FirstOrDefault was unordered: a scan of '/x/foo' could
resolve the '/x/Foo' row and stamp the wrong hash onto it (verified on MySQL
8.4: the WHERE matches both, LIMIT 1 returns the wrong one). Treat the SQL
equality as a narrowing filter, order by Id, and settle identity ordinally.
Route the heal through EF and drop a classified violation, so an opportunistic
maintenance write can never abort a scan.
Also: run the dedupe migration test with foreign keys ON (matching prod), clear
the keeper's etag, null out a self-parent, and document the cleanup's limits
(NULL paths excluded, Down does not restore deleted rows, CI's fresh-DB apply
covers none of the data mutation).
Refs #488#308fix#491
GetOrAddFolder was a check-then-insert with no unique constraint behind it,
so two callers racing the same folder could both miss the lookup and both
insert. Enforce identity in the schema and make the loser adopt the winner.
- LibraryFolder gains a SHA-256 PathHash (the MediaFile.Path/PathHash
precedent): Path is MySQL longtext, which cannot be indexed without a
prefix length and collates case-insensitively, so the unique index is on
(LibraryPathId, PathHash) instead.
- GetOrAddFolder and SetEtag catch a classified unique violation via the
existing TvContext.IsUniqueConstraintViolation seam (#308) and re-read.
- Dual-provider migration audits and collapses pre-existing duplicates
(repointing MediaFile, ParentId and ImageFolderDuration) before creating
the index; legacy rows keep a null hash and heal on the next scan.
- Tests: deterministic cross-connection race, 8x10 barrier stress with an
insert-attempt vacuity guard, classifier-inversion negative control, and
a real-migration dedupe test.
Refs #488#308fix#491
This is why `decisions lifecycle` went red, and it was NOT the known flake. I
came close to dismissing it as one for the second time this session, because an
earlier red on another branch genuinely was.
The dual-format parser imported PyYAML to read frontmatter. `decisions-guard`
does `setup-python` and installs NOTHING, so once the corpus was migrated every
record became unparseable there: ModuleNotFoundError, job fails. The same would
hit the Husky pre-commit hook and every contributor's machine.
Installing PyYAML in CI is the wrong fix: READING happens everywhere -- CI, the
hook, every dev -- while WRITING happens once, in a migration a human runs
deliberately. So the read path is now dependency-free and only
`migrate_decisions_split` (the writer) still imports yaml.
A hand-rolled parser is only safe if it provably matches the library that WROTE
the files, so `test_frontmatter_reader_matches_pyyaml_on_every_real_record`
compares the two field-by-field across all 169 real records (importorskip, so it
is skipped rather than failing where PyYAML is absent) with a >100-file guard
against near-vacuity. It is narrow by construction: the frontmatter is
machine-generated with default_flow_style=False and width=10**9, so every value
is a single-line scalar, and the reader bails to None on anything nested.
Verified by running all four affected entry points against a shim that makes
`import yaml` raise: validate --base/--head, catalog --check, the kickoff guard,
and the plain validate the pre-commit hook calls. All exit 0.
refs #610
Three Done-when items that were genuinely unfinished. I audited the checklist
before ticking it rather than after, and these were not done.
1. BODY-DIFF GUARD RE-KEYED FROM HEADING TO `key`. Heading-keying made a pure
RENAME look like a removal plus an unrelated addition, so retitling a record
failed CI as an "unlogged removal" -- a trap that has bitten this repo before.
Records are now MATCHED by key and REPORTED by heading (a key alone is not
enough for a human to find the record). Demotion, which is inherently about a
record losing its key, is detected on the heading axis instead, and a demoted
record is no longer double-reported as removed. Key-matching is also what lets
the comparison work straight through the legacy->frontmatter migration, since
`key` survives both the format change and the move between files.
Pinned by a test driving the REAL git-backed diff engine: retitle a record,
same key, byte-identical prose -> removed/rewritten/demoted all empty.
Mutation-verified: restoring heading-keying makes it fail with exactly
"a retitle was reported as a removal".
2. PATH <-> KEY ENFORCED BY THE VALIDATOR. The filename is derived from the key,
which is what makes one-active-per-key a filesystem property rather than a
check -- but only if the two cannot drift. Now an error when they disagree,
with legacy multi-record files explicitly exempt (they have no key-derived
path to match).
3. ONE-ACTIVE-PER-KEY PINNED AS A FILESYSTEM PROPERTY. Two records sharing a key
derive the same path, so the filesystem refuses the second; the migration
aborts on a destination collision rather than silently overwriting. Both are
now tested, the latter end-to-end through `main()`.
Also fixed a vacuous assertion ruff caught in my own new test -- `assert X or
True` is always true.
Note on verification: my first positive control for the guard reported it NOT
firing. That was the probe, not the code -- "load 340" appears in both the
`sources:` frontmatter field and the body, and replace(...,1) hit the metadata
copy, which is correctly token-free. Re-run against genuine body prose, the guard
fires with exit 1.
refs #610
168 records -> docs/decisions/records/<area>/<topic>.md (163 active, 23 dirs) and
docs/decisions/archive/<area>/<topic>.md (5 archived). The filename IS the key,
so one-active-record-per-key becomes a filesystem property rather than a
validator check, and supersession becomes a `git mv`.
WHY: the monolith was a concurrency problem before an aesthetic one. A
3,900-line append target made parallel sessions collide -- PR #605 and PR #614
both hit append-vs-append conflicts during routine rebases, and hand-resolving
those inside the corpus is exactly the operation the rationale-rewrite guard
exists to police.
HOW IT IS VERIFIED: a ~170-file diff cannot be meaningfully read, so correctness
does not rest on reading it. The parser was taught BOTH formats first, so the
body-diff guard parses the old form at the merge-base and the new form at head --
the migration validates itself, no bypass. The proof is a field-level equivalence
harness: 168 records before and after, zero lost, zero gained, zero field
mismatches, zero rationale bodies differing. Reviewers should scrutinise the
harness; it is the actual evidence.
What measuring caught that reading would not have:
- ~500 lines sit OUTSIDE any record -- decisions.md's lifecycle schema and each
topic file's preamble, mostly the only copy. Source files are kept and
stripped, never deleted. They also cannot be filed per-area: topic files hold
several areas and 4 of 23 areas span several files.
- Archive discovery was a non-recursive glob; after the split it found ZERO
archived records, surfacing as four bogus "supersedes points to unknown key"
errors rather than an obvious failure.
- ~32 live docs point into the corpus BY DATE, which the split dangles. Each
stripped file now ends with a generated "Records formerly in this file" index,
which also rescues the identical breadcrumbs in old issue comments.
- decisions.md's "In this file:" list was 97 same-file anchor bullets that the
split makes WRONG, not merely stale. Dropped; the generated index replaces
them with links that resolve.
The equivalence harness now runs against a checked-in FIXTURE, not the live
corpus. The earlier version migrated the real tree, which made it a one-shot:
the moment the migration landed there was nothing left to move and the tests
failed for reasons unrelated to the code. A fixture keeps them testing the
SCRIPT rather than the repo's current state.
Keys preserved verbatim, warts included: `sched` (12) and `scheduling` (1) remain
two directories for one concept. Renaming a key is not a move -- it changes
identity, breaks the equivalence proof, and invalidates MemPalace's per-key
drawers. Taxonomy normalisation is separate work.
refs #610
Both surfaced when main gained two records mid-flight.
- test_record_count_is_the_expected_166 hardcoded the total, so it failed the
moment a record landed on main -- a merge turning an unrelated test red. The
real invariant is before == after; the count only needs to prove the harness
isn't parsing a stub corpus, so it is now equality plus a floor.
- The migration's "lines preserved" figure silently absorbed the generated
where-did-it-go index once that was threaded into the preamble string, jumping
507 -> 759 with no new prose preserved. It now reports the two separately:
514 lines of original prose, plus 245 generated index lines. A number that
quietly changes meaning is worse than no number.
Live docs point into the corpus BY DATE -- "see `decisions.md` 2026-07-10" --
about 32 such references across 12 files, plus the same form in historical issue
comments. The split would dangle every one of them.
Each stripped file now ends with a generated "Records formerly in this file"
index: date, title, and a link to the record's new path. A reader following a
date pointer lands on the file it names and resolves from there. That is far
cheaper and less error-prone than rewriting 32 references by hand, and it also
covers the issue-comment breadcrumbs, which cannot be rewritten at all.
Caught while verifying it: the generated `## Records formerly in this file`
heading is itself an H2, so the record parser counted one legacy-unmigrated
record per stripped file -- the notice went 0 -> 6. Same treatment as the
existing `## Index` section: skip it by name. SKIP_HEADINGS moved to
decisions_lib as the single source of truth, since three modules now need it.
Found by reading the validator's notice output on a trial migration, not by
inspection -- the corpus still validated OK, so nothing else would have flagged it.
The first cut tested `relative_to(TOPIC_DIR)`, which also matches the LEGACY
multi-record topic files -- so every record in workflow-process.md et al. lost its
anchor and linked to the top of the file instead of to its own record. Caught by
`build_decisions_catalog --check` going stale on the unmigrated corpus, not by
reading. Narrowed to `RECORDS_DIR in src.parents`, which by construction only
matches one-record-per-file.
The line budget exists to bound how much narrative a reader or agent must get
through. Under the split each record carries ~11 frontmatter lines plus two
fences -- 1789 lines across 166 records -- which are the structured restatement
of what used to be one dense backtick line. Counting them inflates the metric
without any new knowledge being added.
Stated plainly because it flatters the number: this is a change of METRIC, not a
consolidation. It re-measures the same corpus, it does not shrink it. Whole-file
counting put the migrated corpus at 6837 against a 5600 budget; prose-only puts
the same content at ~5048. The consolidation work is still worth doing -- it is
simply no longer being signalled by a warning that was partly measuring
punctuation.
Inert pre-migration: no legacy file has frontmatter, so the branch is never
taken and today's number is unchanged.
Both surfaced in a trial migration against a disposable copy of the real corpus,
not from reading:
- Archive discovery was a NON-recursive glob. After the split, archived records
live at archive/<area>/<topic>.md, so every one of them became invisible --
which surfaced as four bogus "supersedes points to unknown key" errors rather
than as an obvious "no archive found". rglob at both sites.
- Catalog links: a record is now a FILE, so the link is a plain relative path
with no anchor -- nothing to slug, nothing to keep in sync with a heading. The
legacy anchor form is kept for records still living inside a multi-record file,
so the catalog is correct on either side of the migration.
The migration is a MOVE, so correctness is provable rather than reviewable:
parse the corpus before, migrate, parse after, assert the Record sets are
identical field-for-field with byte-identical rationale.
scripts/migrate_decisions_split.py
166 records -> docs/decisions/records/<area>/<topic>.md (active) or
docs/decisions/archive/<area>/<topic>.md (superseded/retired), 26 directories.
Path is DERIVED from the key, so identity stays the key. Refuses to run if any
record lacks a key, and aborts on a destination collision.
Source files are KEPT, stripped to their narrative -- not deleted. 506 lines of
the corpus sit outside any record: decisions.md's lifecycle-schema header (the
status vocabulary, supersession rules, the edit-token contract) and each topic
file's preamble explaining why those records exist. For most of it that is the
only copy. It also cannot be filed per-area -- topic files hold several areas
and 4 of 23 areas span several files -- so the files themselves stay.
scripts/tests/test_migration_equivalence.py
Runs the real migration against a COPY of the real corpus in tmp_path, never
the working tree. Asserts: all 166 records survive with the same keys, every
metadata field round-trips, titles round-trip from headings, every rationale
body is byte-identical, path matches key, archived records land in the archive
wing, the legacy files keep their narrative, and no parseable record is left
behind in them.
Proven non-vacuous: corrupting one migrated record's prose is caught by the
byte-identical check, and deleting one is caught by the survival check.
One test-authoring note: an early assertion string-matched "## " to prove no
records were left in decisions.md. That is wrong -- the schema header quotes an
illustrative "## 2026-07-17 ..." example in prose. Whether records remain is a
PARSING question, so the parser-based leftover test is the real invariant.
First step of the one-file-per-record split. `decisions_lib.parse_text` now
dispatches on a leading `---`: the legacy "many ## records per file, metadata on
a backtick line" form and the new "one record per file, YAML frontmatter" form
both produce the same Record.
This is the load-bearing move for the migration. Because the parser understands
both, the body-diff guard can parse the OLD form at the merge-base and the NEW
form at head -- both keyed on `key` -- so the commit that relocates 166 records
validates itself with no one-time bypass and no escape hatch.
Details:
- Frontmatter uses the SAME field vocabulary as the backtick line
(`superseded-by`, `stale-after`), so on-disk names don't shift mid-migration.
- YAML `None` (a key with no value) is preserved as "" rather than collapsing to
None, keeping the empty-vs-absent distinction `stale-after` depends on (#603).
- `active_files()` also walks `docs/decisions/records/**`, so both layouts can
coexist while the migration lands.
- Malformed frontmatter (unterminated fence, non-dict, YAML error) yields no
record rather than raising; the validator then reports it as a missing record.
Last review Low, marked safe-to-defer but it is two words in a file this PR already
edits. Both prose mentions of the marker now say "non-merge commit", matching the
decision Rule, the catalog, the module comment and the function docstring.
Docs-only.
fixes#609
Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review Low: the module comment and the _edit_trailer_armed summary still said
"a commit message ... does arm" / "some commit", while the matcher excludes merges.
The decision Rule and catalog already carried the qualification; these two did not.
Comment-only.
fixes#609
Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review Low x2, both correct and both the stale-comment class:
The claim that prepush-rebase-check.sh forbids merging main into a PR branch is
false. That hook refuses a branch that is BEHIND origin/main; a merge makes
origin/main an ancestor, so the push is allowed. Merging main in is discouraged by
convention only. So --no-merges does cost a real false negative: an author who marks
ONLY a conflict-resolving merge commit gets a legitimate rewrite rejected. Keeping
--no-merges and stating the trade explicitly -- that failure is loud and costs one
extra commit, whereas honoring forge-composed merge bodies disables the guard
silently, which is #609 itself.
The module comment also still claimed a quoted example cannot arm the guard, which
contradicts the residual the decision record now states accurately. Aligned both, and
narrowed the record's Rule line from "some commit" to "some NON-MERGE commit" so the
stated contract matches the implementation.
No logic change -- comments, docstring, record prose and regenerated catalog only.
fixes#609
Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cross-family review (High + Medium + Low), all three reproduced before fixing:
High -- on a pull_request event actions/checkout lands on a synthetic merge commit whose
body the forge composes from the PR description, so a description ending in an example
marker armed a guard no author armed. Excluded merge commits from the range; merging main
into a PR branch is separately forbidden, so no author-written commit is skipped.
Medium -- a folded value (`no` + continuation ` yes`) was split into independent lines and
the continuation armed on its own, inverting the value the author wrote. Read with `unfold`
so the value is judged whole.
Low -- the module docstring promised fail-open while marker resolution deliberately fails
closed. The posture is right; the docstring was wrong. Documented as the one exception.
Both new negative controls verified red against the unfixed matcher. Decision record
amended to state the residual honestly rather than overclaim: a quoted example that is the
FINAL paragraph of an ordinary commit is a trailer by git's own grammar and does arm. What
the change buys is that discussing the marker can no longer disable anything.
fixes#609
Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rationale-edit exemption was a substring test over the whole commit range, so any
message containing the literal marker armed it -- including prose explaining why no
marker was needed, which is how it fired live in PR #605: a green --base/--head run
that was vacuous on the body-diff dimension, in the one PR that hand-resolved a merge
conflict inside the corpus the guard exists to police.
Now read as an affirmative `Decisions-Edit:` git trailer. Git parses trailers only in
the final paragraph, so a quoted example message cannot arm it -- which matters because
this commit and its decision record both quote one. A non-affirmative value (`no`) does
not arm it either; the retired substring arms nothing and gets a ::warning:: nudge.
Tests: negative controls (prose mention, quoted mid-body trailer, `no` value, retired
substring) plus positive controls (trailer, uppercase, alongside Co-Authored-By). All
four negative controls verified red against the old matcher.
fixes#609
Decisions-Edit: yes
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-review Low. The substitute incremented the failure counter inside .Returns(...), i.e. when
the enumerable was handed out, while the real paginator records from ProjectToMusicVideo's catch
DURING enumeration. A refactor that snapshotted Count before the enumeration completed would
then break production while both replacement tests kept passing — exactly the regression the
guard exists to prevent.
Moves the recording into an async iterator, and corrects the decision record to describe #484's
removed music-video test accurately and name its two replacements.
Rebasing onto #612 exposed a silent gap rather than a conflict. #612 added a
`projectionFailureCount` parameter to MediaServerReconciliationGuard.ShouldFlagMissing that
refuses the sweep when the enumeration reported swallowed projection exceptions — but the
parameter is OPTIONAL with a default of 0, so this scanner compiled unchanged while opting
out of the protection entirely. ProjectToMusicVideo has exactly the swallowing catch #484
exists to defend against, so the music-video sweep would have been the only one unguarded.
- MediaServerMusicVideoLibraryScanner creates one MediaServerProjectionFailureCounter per
enumeration (never a field on the singleton api client, so concurrent scans of different
libraries can't leak failures into each other's sweep decision), passes it to
GetMusicVideoLibraryItems, and feeds its Count to ShouldFlagMissing.
- The single guard call also gates the #496 legacy path diff, which is more exposed: a
legacy row has no etag to fall back on.
- ProjectToMusicVideo now returns MediaServerProjectionResult<JellyfinMusicVideo>, combining
#612's Skipped/Failed distinction with #496's identity type.
- main's own #484 music-video test targeted the pre-#496 hard-delete scanner (FindMusicVideoPaths
/DeleteByPath) and no longer applies; it is replaced by two tests in the new architecture
covering the identity sweep and the legacy sweep. Both proven non-vacuous — removing
projectionFailures.Count from the guard call fails exactly those two.
4,277 tests green; format/BOM clean; decisions validator OK.
Re-review returned MERGEABLE with one Low, MySQL-collation-dependent edge in
FlagFileNotFoundByPaths: the C#-side Except diff is ordinal, but `MF.Path IN @LocalPaths`
runs under MySQL's case-insensitive default collation, so a still-reported identified row
differing only in case from an absent legacy row could be matched and flagged missing.
Match on the indexed PathHash instead of collated Path text, and re-state the
NOT EXISTS (JellyfinMusicVideo) guard so the identity pass and the legacy pass are disjoint
by construction rather than by the caller's diff being correct. SQLite was unaffected.
Independent cold review (Codex) returned BLOCKED. Findings 1, 3 and 4 are fixed here;
each has a regression test proven non-vacuous by a negative control.
1. Blocker — the replaced local path was discarded. The scanner computed localPath but
GetOrAdd only received `incoming`, so the repository re-derived the path from the
UNREPLACED projection. On any install with path replacements, adoption hashed the
server-side path, missed the existing row, ALSO slipped past MediaFileAlreadyExists
(which hashes that same wrong string) and inserted a duplicate row under a server path,
leaving the original collection-linked row identity-less forever. The test harness hid
this because its path-replacement stub was an identity function.
→ GetOrAdd now takes localPath explicitly and never reads the projection's path;
BuildPathReplacement takes a real mapping and the new test genuinely replaces.
3. Medium — GetByItemId matched on ItemId alone, so two media sources presenting the same
item id (cloned Jellyfin DB) resolved to each other's row, letting one library repoint
another's. → filtered by LibraryPath.LibraryId.
4. Medium — a row predating the identity that the server had ALREADY stopped reporting was
never adopted (adoption only runs for an incoming item) and carried no identity, so the
itemId diff could not see it either: it sat Normal and schedulable forever, strictly
worse than the hard delete it replaced. → GetExistingLegacyMusicVideoPaths +
FlagFileNotFoundByPaths reconcile legacy rows by local path, and they are counted into
the #477 empty-fetch guard (on the first scan after this ships they ARE the whole
library, so a guard counting only identity rows would sweep all of them on a transient
empty fetch).
Finding 2 (the issue's Done-when #2) is a scope question, not a defect, and is unchanged:
one file path is still one MediaItem row globally, so this lands music videos at parity
with movies rather than eliminating shared-row trashing. Recorded honestly in the decision
record; raised for an explicit call before the issue is closed.
fixes#496
Music videos carried no server identity, so JellyfinMusicVideoLibraryScanner had to
reconcile by a (LibraryPathId, path) diff and HARD-delete the remainder. A file served
by two libraries with overlapping local paths is a single row owned by whichever library
scanned it first, so that owner's sweep destroyed a row another library still served —
taking collection membership and playout references with it, irreversibly.
This is #494's deferred "option 2":
- New JellyfinMusicVideo : MusicVideo (ItemId/Etag), mirroring JellyfinMovie — TPT table,
varchar(36), ItemId index. Dual-provider migration Add_JellyfinMusicVideo.
- New IMediaServerMusicVideoRepository + JellyfinMusicVideoRepository: itemId-keyed
existing-set/lookup and Flag{Normal,Unavailable,FileNotFound} seams, all scoped per
library via LibraryPath.LibraryId.
- New MediaServerMusicVideoLibraryScanner base; JellyfinMusicVideoLibraryScanner folds
onto it and keeps the #177/#488/#497/#500 metadata-reconcile logic verbatim.
- The sweep now soft-trashes (FileNotFound) instead of deleting, so removal is reversible
and EmptyTrash-governed. DeleteEmptyArtists consequently no longer fires from a sweep.
- Pre-identity rows are ADOPTED in place: the identity row is inserted against the same
MediaItem id, scoped to the scanned library's own LibraryPath, so collection membership
survives and a local/second-library row is never hijacked.
- AddMusicVideo normalizes Path/PathHash to the path-REPLACED local path; the projection
fills them from the server-reported path, which would break every later PathHash lookup.
Docs: scan.musicvideo-reconciliation relocated to docs/decisions/archive/scan.md as
superseded; new active record scan.musicvideo-server-identity.
fixes#496
Review follow-up. The comments still said three (Jellyfin) / two (Emby)
library-level enumerations after the nested season and episode ones were
counted, which understates the reach of the very safety property this
branch establishes -- a future auditor reading them would conclude the
nested sweeps are unprotected.
Review finding 1 (blocking). ScanSeasons' FlagFileNotFoundSeasons and ScanEpisodes'
FlagFileNotFoundEpisodes had no guard at all — neither #477's nor #484's — so ProjectToSeason /
ProjectToEpisode returning Failed() was computed and discarded.
#477 scoped those out because "the blast radius is one show's seasons / one season's episodes",
which holds for a per-parent EMPTY fetch but not for a projection failure: that is systematic by
construction. One bad code path fires on every parent, so every season enumerates zero episodes,
existing.Except([]) is the whole episode library, and EmptyTrashHandler deletes it permanently.
Threads the counter into GetSeasonLibraryItems / GetEpisodeLibraryItems(WithoutPeople) for
Jellyfin and Emby using the same optional-trailing-param shape, and guards both sweeps with
MediaServerReconciliationGuard.ShouldFlagMissingDescendants — the same class and the same private
failure predicate as ShouldFlagMissing, deliberately WITHOUT #477's empty-fetch branch so
per-parent empty behaviour (and #476's cascade, which depends on it) is unchanged.
Also from the review:
- finding 3: tests now pin the same-instance JOIN at every level (movie, show, season, episode,
music video) by driving the real ScanLibrary entry point and recording the failure from inside
the enumeration, so a refactor handing the api client a fresh counter goes red.
- finding 4: the missing-library Failed() branch is documented as defensive and unreachable.
- finding 2: the mass-Skip residual (Emby's response-shape-dependent MediaSources guard, Plex's
pre-projection filter) is stated as a known limitation in the decision record.
- finding 5: the log-contract change (only the #484 message when both refusals apply) is noted.
fixes#484
Extends MediaServerReconciliationGuard (#477) with a second deterministic refusal: when the
enumeration that produced the incoming set silently dropped items whose projection THREW, the
file-not-found sweep is refused. A dropped item the server did return is indistinguishable
from a deletion at the reconcile step, so a projection regression could otherwise mass-flag a
healthy library FileNotFound (which EmptyTrash then deletes permanently).
Deliberate guard-clause skips (STRM files, virtual items, unsupported types) are explicitly NOT
failures and never suppress a sweep — counting them would permanently disable reconciliation for
any library holding a single STRM file.
The ratio / missing-fraction threshold is REJECTED, not deferred: it is a two-sided heuristic
with no tunable default and no telemetry, and the failure it approximates is exactly observable
via the projection-failure count (a genuine bulk deletion produces zero failures).
Seam is deliberately narrow — the private ProjectTo* contract inside each api client changed from
Option<T> to MediaServerProjectionResult<T> (projected/skipped/failed), the paged helper counts
IsFailure in one place, and the scanner reads it through an optional trailing
MediaServerProjectionFailureCounter on only the five library-level methods that feed a sweep.
The counter is per-enumeration state created by the scanner, never a field on an api client.
fixes#484
Caught in re-review. The record shipped in 0ad02db6 said 'no backfill was done
here'; 8e1e15e4 then backfilled three ci.* records on the same branch. Corrected
now while the record is still NEW at HEAD, so the body-diff guard does not apply
and no edit token is needed -- after merge this would be surviving rationale
prose and the same fix would cost one.
Deliberately avoiding the literal bracketed token string here: decisions_validate
matches it as a bare substring over the whole commit range, so a message merely
DESCRIBING the mechanism arms it and suppresses the rationale-rewrite guard for
the entire PR. Found exactly that way on the first draft of this commit.
The mechanism shipped unused. First adopters are the records that encode measured
host behavior, which is what drifts silently:
- ci.runner-placement 2027-01-15 — bumblebee sizing (25 GiB/12 cores) + container
caps. Premise has already partly moved: the media transcoders left for jazz on
2026-07-20 and the runners were retuned since.
- ci.infra-shaped-red-under-load 2027-02-15 — a triage heuristic calibrated against
load 243 on a host that has since been retuned.
- ci.peak-anon-measurement 2027-03-15 — weakest of the three (mostly our own
script) but it does assert that memory.peak is cache-inflated, a cgroup fact.
Dates staggered so they don't all come due in the same week. Each Sources line cites
the incident evidence already named in the record's own prose.
1. `date.fromisoformat` is not a YYYY-MM-DD validator. On Python >= 3.11 it also
accepts ISO basic format ("20270101") and week dates ("2027-W01-1"), so a
malformed-looking value passed the blocking check — and which forms parse
depends on the interpreter, meaning the same corpus could validate differently
on a dev machine and on the runner (pr-checks.yml pins only python-version
'3.x'). Knock-on: the catalog's Review-due section sorts on the raw STRING, so
an accepted "20270101" sorted AFTER "2027-01-15" ('-' < '0'), contradicting the
section's own "sorted soonest-first" text. Gate on ^\d{4}-\d{2}-\d{2}$ first,
which fixes both — a fixed-width zero-padded form makes string sort == date sort.
2. A present-but-empty `stale-after:` was collapsed to None by `or None` in the
parser and then skipped by a truthiness guard in the validator, so it passed as
"absent" — a field that silently never fires, which is the exact failure mode
the blocking check exists to prevent. Keep "" distinct from None and test with
`is not None`.
3. `test_catalog_is_date_independent` was partly vacuous: with no date in either
render, both sides were trivially equal after the .replace(). It did still catch
an injected clock-derived marker, but it passed with the feature deleted. Assert
the dates are present.
4. The malformed-date check ran only over the active set, exempting archive
records. Staleness is moot there, but a typo is still a typo — check both wings.
Adds regression tests for each, plus a Review-due row for a topic-file record
(pinning the `../decisions.md` vs bare-filename link forms).
Evaluated the Open Knowledge Format (GoogleCloudPlatform/knowledge-catalog okf
v0.2, scaccogatto/okf-skills) as a replacement for our decision-record system and
rejected it: its conformance rules are deliberately permissive exactly where ours
are strict (broken links, unknown types and missing fields must all be tolerated;
`deprecated` points at no successor), and its stable identity is the file path,
which the breadcrumb rule tells agents not to trust.
Adopted two of its optional families instead, additively:
- `stale-after: YYYY-MM-DD` on the metadata line — marks a record asserting an
outside-world fact as due for re-confirmation. Absolute date, no TTL.
- `**Sources:**` in the metadata block — the evidence a record rests on, as
distinct from `Signals:` (recall keywords).
Neither is required; absence is never an error. A malformed `stale-after` is
blocking (it would silently never fire), but a past-due record is only a
non-blocking `::notice::` — going stale is the passage of time, not a defect in
whatever commit is under test. The catalog's new "Review due" section renders the
date only and never a clock-derived verdict, so it cannot drift `--check` red on a
calendar boundary with no commit touching the corpus.
No backfill: no existing record adopts either field here.
fixes#603
Review follow-up. The 9-element ExpectedPositions table meant a future enum
member would pass every case and silently render bottom-right again -- the
exact bug #503 fixes. Assert the table covers Enum.GetValues instead, and
exercise the previously untested SourceContentMargins() branch.
OverlayWatermarkFilter.Position had no switch arm for MiddleCenter, so it
silently fell into the BottomRight default and rendered bottom-right.
OverlayWatermarkCudaFilter and OverlayWatermarkQsvFilter both inherit this
Position property without overriding it, so they were affected too (the
whole ErsatzTV.FFmpeg project has only this one WatermarkLocation switch).
- Add an explicit MiddleCenter arm: x=(W-w)/2:y=(H-h)/2
- Make BottomRight an explicit arm instead of the fallthrough default
- The default case now logs a warning (not throw - this runs on the
playback hot path constructing ffmpeg args, and sibling filters in this
project already use safe string fallbacks rather than throwing for an
unmapped enum) and falls back to the BottomRight position
- Strip the pre-existing UTF-8 BOM from OverlayWatermarkFilter.cs (#311
formatting gate: touching a legacy BOM'd file makes removing it ours)
- Add ErsatzTV.FFmpeg.Tests/Filter/OverlayWatermarkFilterTests.cs asserting
the exact position expression for every WatermarkLocation value across
the software, CUDA, and QSV overlay filters, plus the unmapped-value
fallback
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Completes the second pin-recovery cycle. main moved twice during this branch's
review and each rebase rewrites the sha of the commit that touched docker/ci, so
the pin has to be re-pointed each time.
Simulated the guard's full logic before pushing, including the length check main
added in #598 (which came from this session's #594):
length = 7 (guard requires exactly 7)
expected = git log -1 --format=%H -- docker/ci .gitea/workflows/ci-image.yml
= 32747a067e
pin = 32747a0 -> resolves to the same commit
=> WOULD PASS
Confirmed the registry actually holds ersatztv-ci:32747a0 before pinning it, so
this cannot be the "pin resolves but no such tag exists" failure #594 describes.
Refs #445#594
Second pin-recovery cycle on this branch — main moved twice during review and each
rebase rewrites the sha of the commit that touched docker/ci. Records that #598 now
length-checks the pin at exactly 7 chars, which turns a locally-computed 8-char
`git rev-parse --short` into a loud gate failure instead of a confusing
manifest-unknown at image-pull time. That guard came from #594, filed by this
session after hitting exactly that ambiguity.
This commit also IS the recovery: it touches docker/ci, so ci-image.yml tags it and
the pin can be re-pointed in the follow-up commit.
Refs #445#594
main's #598 added a port pre-flight to scripts/e2e-local.sh while this branch was
in review, and theirs is strictly better than the one I had here:
- it probes BOTH bound ports. The app binds a SECOND (streaming) listener that
defaults to 8409 regardless of ETV_UI_PORT, so my single-port check could
certify a port free while the run still died binding 8409. That also means
this PR's CI step (ETV_UI_PORT=8410) was only working because the curl step's
server had already been killed — #598's fix, defaulting ETV_STREAMING_PORT to
the given port, is what makes it correct rather than lucky. A real latent bug
in my work, surfaced by their change.
- it REPORTS rather than reaps, which is the #586 rule.
Keeping mine would leave two divergent port checks on the same concern.
Also records the app's SINGLE-INSTANCE guard, which is independent of ports: a
stray instance blocks a run whatever port you choose, so 'pick another port' is
not a workaround. Kill the leftover BY PID (#586).
Refs #445#586
Second half of the post-rebase pin recovery. The preceding commit touched
docker/ci, so ci-image.yml tagged it and published ersatztv-ci:1652fc5; this
commit points all five container jobs (plus the header comment) at it.
Simulated ci-image-pin's own logic before pushing rather than guessing:
expected = git log -1 --format=%H -- docker/ci .gitea/workflows/ci-image.yml
= 1652fc568e53184c92c7e0bc5a41546aed19744d
pin = 1652fc5 -> resolves to the same commit
=> WOULD PASS
Verified the published image is the one CI will actually consume: pulled
:1652fc5 on the runner host, /ms-playwright holds chromium_headless_shell-1234,
chromium launches (151.0.7922.34), dotnet 10.0.302 intact.
Refs #445
Hit this for real on this branch. `ci-image-pin` went red after a rebase that
was otherwise clean, and the failure is confusing on three counts:
- the pin must equal the short sha of the commit that touched docker/ci, and a
rebase REWRITES that sha (e9fd26f6 -> 9130274c here);
- the pin still resolves to a real commit and the tagged image still exists in
the registry, so nothing looks broken;
- the force-push does NOT republish: ci-image.yml filters on
`paths: docker/ci/**`, and a rebase that leaves the Dockerfile's content
unchanged produces no diff for that path.
And it cannot be fixed by re-dispatching ci-image.yml, because that tags
`git rev-parse --short HEAD` — the branch HEAD, not the commit that touched
docker/ci. The two coincide only when the docker/ci commit IS HEAD, which is why
the original two-step worked and the post-rebase state does not.
Recorded in docs/ci-cd.md with the recovery, plus the cheaper lesson: land a
toolchain-image change on its OWN branch first, so the consuming branch never
carries the docker/ci commit through a rebase.
This commit is also the recovery itself — it touches docker/ci, so it becomes the
commit ci-image.yml tags, restoring the pin dance.
Refs #445
main added 12 web tests while this branch was in review, so the count I wrote
into docs/testing.md went stale during the rebase. Verified by running the suite
against the rebased tree: 995 tests / 105 files.
Codex round-3 verdict was BLOCKED @ 29bc5119 with three High findings. All three
are real; all three are fixed. They are narrower than the previous rounds — two
are sub-millisecond races between adjacent statements — but the fixes are cheap
and make the ordering correct rather than lucky.
**Recycled-PID kill (the one that genuinely mattered).** `BOOT_PID` stayed set
after `wait` had already reaped the launcher. A stale PID is not merely useless:
the OS can RECYCLE that number during a long spec run, and cleanup would then
SIGTERM an unrelated process. This is the same "never kill something you did not
prove is yours" failure that got the earlier port-based reap deleted — it came
back in a different disguise. Cleared immediately after `wait` returns.
**Cleanup ordering.** cleanup() killed e2e-local.sh BEFORE reading the pidfile,
so a signal landing between the server fork and the pidfile write could kill the
launcher and lose the only handle on an already-running server. The pidfile is
now read FIRST; stopping the launcher is best-effort and never the primary handle.
**Fork/publish sliver.** A signal can also land between `... &` and `BOOT_PID=$!`,
or between e2e-local.sh's fork and its pidfile write — the pidfile reads empty,
then populates microseconds later, and the server is orphaned. cleanup now does a
bounded grace re-read (~1s) when it has no PID. Verified this does NOT cost
anything on a genuinely server-less abort: a bad ETV_BUILD_CONFIG still exits in
1s rather than sitting out the loop.
## Verification
- passing run 3/3; failing run exits 1
- server-less abort exits in 1s (grace loop stays bounded)
- targeted SIGTERM: exit 143, no listener left
- zero orphaned processes after the full gate
Refs #445
Codex round-2 verdict was BLOCKED @ 2a5f26c0 on one High finding, which was
correct and is the subtlest defect in this whole change.
## The defect
Bash DEFERS a trapped signal while it waits on a FOREGROUND command. The boot was
OUT="$(ETV_UI_PORT=... scripts/e2e-local.sh "$CONFIG_DIR")"
so a TERM arriving during the (up to 120s) readiness wait did not run `on_signal`
until boot completed — and a supervisor escalating TERM->KILL means the trap
never runs at all, orphaning the server on its port. The pidfile added in the
previous commit only helps IF the trap runs; this is the case where it doesn't.
Proved the mechanism in isolation rather than asserting it, since the real boot
is only ~2s locally and kept winning the race:
foreground command substitution : TERM at t=1s -> trap ran at t=6s (deferred)
background job + `wait` : TERM at t=1s -> trap ran at t=1s (prompt)
## The fix
Boot in the background and `wait` on it. `wait` is interruptible, so the handler
runs immediately; by then e2e-local.sh has already written $ETV_PIDFILE, so
cleanup can reap a server whose PID this script has not yet parsed. Cleanup also
now stops the backgrounded e2e-local.sh itself, so it cannot sit waiting on a
server we just killed, and removes its temp output file.
Boot diagnostics are preserved on the failure path (verified with a deliberately
bad ETV_BUILD_CONFIG: the underlying "bin/<config> does not exist" error is
surfaced, along with the exit status, which is now propagated rather than
flattened to 1).
## Verification
- mechanism: deferred-vs-prompt trap proven as above
- passing run 3/3 green; failing run (--grep ZZZ_NOPE) exits 1
- boot-failure path prints e2e-local.sh's real error and its exit status
- targeted SIGTERM: exit 143, no listener, no stray process
- curl harness unaffected: 45/45 PASS
- no orphaned processes after the full gate (an earlier count of 1 was a
graceful shutdown still in flight, not a leak — re-checked clean)
Refs #445
Second review round. Codex returned BLOCKED @ e50a2624 with three High findings;
all three were real and all three are fixed. One of them was severe and was
introduced by my OWN previous "fix" commit.
## HIGH 1 — a FAILING spec run exited 0, silently passing CI
`if ! npx playwright test; then status=$?; ... exit "$status"; fi`
Under `!` negation bash sets `$?` to the LOGICAL NEGATION of the command's
status, so inside the failure branch `$?` reads 0 — the script exited 0 on a
failing run. Verified: `if ! (exit 42); then echo $?; fi` prints 0.
A UI-E2E harness that reports success when its specs fail is worse than no
harness. My five green local runs could never have caught this: the bug lives
only on the failure path. Introduced by the log-tail improvement in e50a2624.
Fixed with `set +e` / read `$?` / `set -e`, then exit that status explicitly.
Verified: a deliberately-failing run (`--grep ZZZ_NOPE`) now exits 1.
## HIGH 2 + 3 — the pre-PID fallback needed lsof and only GUESSED ownership
The mid-boot fallback reaped "whatever LISTENS on $PORT", which was wrong twice:
- it needed `lsof`, which is ABSENT from the CI toolchain image (verified
directly in the published image) — so it silently no-opped precisely where
it was needed;
- it INFERRED ownership from the earlier pre-flight rather than proving it, so
a process that grabbed the port after the pre-flight — or a real instance on
a shared host — could be killed. Reaping someone else's server is worse than
the leak it was meant to fix.
Replaced with an opt-in `ETV_PIDFILE`: e2e-local.sh writes the PID the instant
it forks, BEFORE its readiness wait, which is exactly the window a mid-boot
signal lands in. A pidfile we asked for PROVES ownership, needs no external
tool, and works in CI. The port-based kill is gone; the lsof pre-flight remains
as a friendly local check only.
Verified: the pidfile is populated while still mid-boot (readiness not yet
reached), names the real `dotnet ErsatzTV` process (not a subshell — which also
re-confirms the `exec` fix), and killing that PID alone frees the port.
Also dropped the `seq` dependency inside the trap (shell arithmetic instead),
addressing the other reviewer's busybox concern.
## Docs-reviewer finding — my stated reasoning was wrong
I justified amending `testing.e2e-local-fresh-config-dir` rather than superseding
it partly on "renaming the heading trips CI". That's a true statement that does
NOT bear on the choice: a supersession relocates to `archive/` with the heading
INTACT (verified: archive/api.md keeps the #72 heading verbatim). Corrected to
the actual reasons — the Rule never reversed, and the key is cited from
docs/handoffs/chicorytv-issue-queue.md plus two docs/superpowers/ files, which a
supersession would aim at an archived, stale-labelled record.
## Gotcha found by accident, now documented
A flawed test of mine booted two e2e-local.sh instances concurrently and the
first mysteriously failed to become ready. Cause: every run `rm -rf`s and
re-copies the SAME build-output wwwroot, so a second run yanks the static files
out from under a still-starting first instance. Documented in both the script
header and docs/e2e-local.md, because the symptom (readiness timeout, or /app
404ing) looks nothing like a shared-directory race. CI is unaffected — its curl
and UI-E2E steps are sequential.
## Budget: filed, not shaved
This PR pushes the active decisions corpus 7 lines past its 5600-line soft
budget (main was under). I trimmed my records repeatedly and each rewrite
recovered ~1 line, because the content is load-bearing; continuing would have
meant deleting useful rationale from a new convention record to hit an arbitrary
cap. The validator's own remedy is "schedule a consolidation", so that is filed
as #595 rather than paid for by starving the record. Non-blocking warning.
Also filed #594 for the pre-existing `ci-image-pin` any-hex-length weakness.
## Verification
- failing run exits 1 (was 0); passing run still 3/3 green
- pidfile written mid-boot, names the real dotnet proc, reap frees the port
- SIGTERM mid-run: exit 143, no orphan listener or process
- curl harness unaffected: 45/45 PASS; ETV_PIDFILE unset => unchanged behaviour
- decisions validator OK; zero orphaned processes after the full gate
Refs #445#533#594#595
Addresses the adversarial review round. Codex returned BLOCKED on the harness
lifecycle contract; the second (cold) reviewer independently flagged the same
trap-ordering defect, which is what made it credible.
**Trap installed AFTER boot -> signal mid-boot orphans the server.** The window
between `e2e-local.sh` returning and `trap ... EXIT INT TERM` had no handler, so
a Ctrl-C/TERM there (or the PID-parse bail-out) left dotnet holding the port —
violating the script's own "always kills the server" contract. The trap is now
installed BEFORE boot. When the PID is not yet known, cleanup falls back to
reaping whatever LISTENS on our port; attribution is sound because the
pre-flight proved that port free moments earlier.
**Signals were not re-raised.** A TERM landing beside a passing Playwright run
could exit 0, reporting success for a cancelled run. INT/TERM now clean up and
re-raise, so the wrapper dies BY the signal.
Verified, not assumed: SIGTERM mid-run -> wrapper exits 143 (128+15) and leaves
zero listeners and zero stray ErsatzTV processes.
`e2e-local.sh` backgrounded a MULTI-command subshell, so `$!` is the subshell —
not dotnet — wherever bash does not collapse it. Measured both ways:
bash 3.2.57 (stock macOS /bin/bash), 2-command subshell : $! = SUBSHELL
bash 5.3.15 (homebrew) : $! = leaf
with `exec` (both versions) : $! = leaf
Consequence on stock-macOS bash: every PID-based kill/liveness check targeted
the wrong process, the escalation silently no-opped, and the server leaked. Fixed
at the source with `exec`, which benefits all consumers (the CI step's trap and
the e2e-functional.sh pairing), not just e2e-ui.sh.
My first attempt to test this was WRONG and would have cleared the finding: a
single-command subshell is collapsed on both versions. Only the 2-command form
reproduces it.
`testing.e2e-local-fresh-config-dir` was still `status: active` asserting the bug
'wait for either line' option today", "widening the probe ... is tracked as #533".
`READY_LINE` existed ONLY in that record; nowhere in code.
Amended rather than superseded: the operative rule (use a fresh config dir) is
unchanged and still correct — only its RATIONALE moved from "the probe hangs" to
"state bleed". Heading deliberately left alone: the validator matches records by
`## HEADING`, so renaming fails CI as an "unlogged removal"; an explicit note now
tells the reader the heading is historical.
- Boot-failure diagnostics were lost: `OUT="$(...)"` aborts under `set -e` before
the print. Now `if ! OUT=$(...)` so the output is shown.
- Playwright failures surfaced no server-side evidence (nothing uploads the
traces in web/e2e/.output). The server log tail is now printed on failure.
- `lsof -ti :PORT` also matched outbound/TIME_WAIT sockets -> false positives.
Now `-sTCP:LISTEN`. Documented honestly that the CI image ships no lsof, so
the pre-flight is a local-developer guard only.
- Predictable `/tmp/etv-pw-probe.$$` -> `mktemp` (symlink-redirect on a shared host).
- Suggested escape-hatch port was 8411, which is scripts/security-scan.sh's
default; 8409/8410 are prod/ersatztv-test on jazz. Now suggests 8419 and names
the conflicts.
- boot-gate.spec.ts comment overclaimed: after logout the browser holds NO
cookie, so that assertion cannot prove stamp rotation (the curl harness does,
by replaying the same cookie). Comment corrected to what it actually proves.
Deliberately NOT changed: `ci-image-pin`'s regex accepts any hex length rather
than the exact 7 chars ci-image.yml publishes (pre-existing guard weakness, not
introduced here — filed as a follow-up rather than widened in this PR).
Also trimmed the two records I had bloated: the active decisions corpus went over
its 5600-line budget as a result of this PR (baseline on main was under), so the
overflow was mine to pay down, not to pass on.
- SIGTERM mid-run: exit 143, no orphan listener/process
- curl harness unaffected by the `exec` change: 45/45 PASS
- UI-E2E 2x clean; typecheck + lint clean; decisions validator OK, no size warning
Refs #445#533
Two review findings from the cold docs/convention pass.
HIGH — the new `ci.ui-e2e-harness` record cited `` `ci.toolchain-image` `` as if
it were a resolvable decision key. No such key exists anywhere in the corpus
(verified: the only occurrence in all of docs/ was that citation itself). #390's
toolchain-image work was deliberately never migrated to a standalone record —
`ci.runner-placement`'s own Signals line says so.
This is exactly the breadcrumb hazard docs/README.md -> "Knowledge retrieval"
warns about: a future session (or a MemPalace lookup) resolving that key gets
nothing back, and cannot distinguish "retired" from "never existed". Replaced
with an explicit pointer to #390, `ci.runner-placement`, and docs/ci-cd.md, and
stated outright that #390 has no standalone record.
Swept the whole class rather than the one instance: every dotted key cited in
added lines across the diff now resolves (ci.functional-e2e-harness,
ci.runner-placement, ci.ui-e2e-harness).
NIT — docs/ci-cd.md said the UI-E2E step is "Step 4 of this same job", which
refers to the doc's own 4-item prose summary, not the YAML step list (where it
is the 11th `steps:` entry). Reworded to name the actual step so a reader
skimming the workflow isn't sent looking for a 4th YAML step.
Refs #445
Adds the last deferred #299/#363 follow-up: the flows that CANNOT be expressed
as curl calls. Scope rule (the durable part) — assert only what the curl
harness structurally cannot reach:
1. client-side form validation (the Setup confirm-password gate is pure React
state and makes no request, so there is no HTTP contract to assert)
2. AuthGate's RENDERED states (Setup vs Login vs app)
3. the session cookie authenticating the SPA's OWN /api XHRs — curl proves the
cookie works for curl, not that the app sends it
4. sign-out through the UserMenu back to the login gate
New: web/e2e/boot-gate.spec.ts, web/playwright.config.ts, scripts/e2e-ui.sh
(owns the whole lifecycle: fresh config dir -> boot -> specs -> always kill).
Runs as a second step of the EXISTING advisory `functional-e2e` job rather than
a new job: the dominant cost there is `npm ci` + the Release build, both already
done, so this adds ~5s instead of duplicating a heavy job. It boots its own
fresh instance on port 8410 because the first spec asserts the one-shot Setup
gate that the curl step has already claimed on its config dir.
Determinism (the issue asked for it explicitly): `serial`, `workers: 1`,
`retries: 0` even in CI — a retry would let a flaky flow merge looking green.
Measured 5 consecutive clean runs, ~2s each.
Pins all five `container:` jobs to the toolchain image built by the preceding
commit, which bakes `chromium-headless-shell`.
Non-obvious coupling fixed: vitest's default include glob would have collected
web/e2e/*.spec.ts and run it under jsdom. Excluded `e2e/**` by spreading
`configDefaults.exclude` rather than narrowing `include` to `src/**`, because
web/scripts/ holds a real vitest test an src-only include would silently stop
running.
`RebuildSearchIndexHandler` logs one of two mutually-exclusive lines just before
`SystemStartup.SearchIndexIsReady()`:
fresh config -> "Done migrating search index in {Duration}"
reused config -> "Search index is already version {Version}"
The probe watched only the first, so a reused dir waited out the full 120s
timeout and then killed a perfectly healthy server. Widened to a `grep -Eq`
alternation; the handler's if/else is exhaustive, so the pair covers every path
to readiness.
Verified with a negative control: on a reused dir the server is ready in 2s via
the "already version" line, and the OLD probe string is genuinely ABSENT from
that run's log — so the old code would have hung, i.e. the fix is load-bearing
rather than incidentally passing.
The "prefer a fresh config dir" guidance stays: that guards state bleed, which
is a separate concern from the probe hanging.
- `wait "$PID"` in the cleanup trap was a NO-OP: the server is a grandchild
(launched in e2e-local.sh's subshell, which then exits), so `wait` fails
instantly and was swallowed by `|| true` — cleanup did not actually ensure the
port was released, exactly what its comment claimed. Replaced with a bounded
`kill -0` poll, then SIGKILL.
- Added a port pre-flight check: previously an occupied port surfaced as a 120s
readiness timeout that reads like a broken build. Now fails in 0s naming the
PIDs, and warns against blanket-killing `dotnet ErsatzTV.dll` (that reaps
other sessions' servers).
- UI-E2E: 5x clean (3 specs, ~2s); back-to-back runs pass with no manual cleanup
- curl harness unaffected by the boot-script change: 45/45 PASS
- web: 983 tests / 105 files green; typecheck + lint clean
- vitest collection verified: excludes web/e2e, still collects web/scripts
- Dockerfile sequence + browser launch validated verbatim in a container on the
real amd64 base before committing; chromium launches as root with NO sandbox
opt-out needed
- decisions validator green; catalog regenerated
- docs/decisions.md TOC repaired: it had drifted to 69 of 97 records and held a
dangling anchor to the #72 record that #415 superseded into archive/.
Regenerated with a generator validated against the 68 existing anchors (0
mismatches) -> 97/97, no dangling, no duplicates.
Docs: docs/e2e-local.md (new "UI-E2E harness" section), docs/ci-cd.md (toolchain
image + UI-E2E step), docs/testing.md, docs/README.md, docs/decisions.md
(new `ci.ui-e2e-harness` record; `ci.functional-e2e-harness` amended — its Rule
said "curl-only", now accurate).
Refs #445#533
Adds `chromium-headless-shell` + its system deps to the shared CI toolchain
image so the `functional-e2e` job can run the UI-E2E Playwright flows without
installing a browser per run — the same "jobs install nothing at run time"
rule as the rest of this image.
Measured on this exact base (Ubuntu 24.04 ffmpeg base, amd64):
- headless shell: 267M vs full chromium: 656M
- `chromium.launch()` resolves to the shell anyway (the config never asks
for headed), so the only cost is that a HEADED run inside this image would
fail — CI-only, and deliberate.
- Chromium launches as root in a container with NO --no-sandbox /
chromiumSandbox:false opt-out, so the Playwright config needs no sandbox
workaround. Verified rather than assumed.
`ARG PLAYWRIGHT_VERSION` must stay equal to web/package.json's EXACT
`@playwright/test` pin: Playwright ties a browser revision to the package
version, so a mismatch leaves no usable browser.
The build-time smoke test LAUNCHES the browser (not just a file test), so a
missing system library fails the image build instead of a CI run.
First half of the deliberate two-step toolchain bump: this commit is what
ci-image.yml publishes as :<sha>; the follow-up commit pins it.
Refs #445
Follow-up to the cold review of 615e00a. The record was corrected to say only
that no NEW sentinel rows are created, but the test file's header still carried
the disclaimed "the data is clean at rest for every provider" claim — sitting on
the very test that supposedly proved it. It now states what the tests actually
pin, and names what they cannot speak to (rows written before this change, which
keep the sentinel and are covered only by the read coercion).
Also corrects docs/testing.md's ErsatzTV.Core.Tests count (543 -> ~650 measured;
the neighbouring Scanner.Tests cell is corrected in #602 instead, to keep the two
open PRs off the same line).
MediaSourceRepository's Plex/Jellyfin/Emby remove-and-recreate (disable-sync)
flows stamped SystemTime.MinValueUtc into library.LastScan, so normal use kept
minting 0001-01-01 sentinel rows. #409 fixed the READ side (the API coerces the
sentinel to null and a migration cleaned the historical residue), so the wire
contract was already correct — this stops new sentinel rows being written.
Safe because every remaining LastScan reader either coalesces
(LastScan ?? SystemTime.MinValueUtc) for its own non-nullable scan-comparison
needs, or is the API read-boundary coercion itself — audited every reference
under ErsatzTV{,.Application,.Core,.Infrastructure,.Scanner,.Mcp} plus web/.
There is no Where/OrderBy/GroupBy on LastScan anywhere, so the SQL
null-ordering divergence between SQLite and MySQL has no surface here.
Scan-comparison behavior is unchanged.
Deliberately ships NO second cleanup migration: rows written between #409's
NullOutNeverScannedLastScan and this change keep the sentinel at rest, and the
permanent read coercion — not a migration — is what keeps the contract honest
for them (as it must be anyway for a restored or hand-edited DB).
LibraryPath.LastScan is likewise left untouched: the flows re-add Paths with
their original values, and its only readers are the local-library scan
handlers, so it has no API surface and no remote-scan effect.
Regression test per provider (MediaSourceRepositoryDisableSyncTests), proven
non-vacuous: restoring the MinValue writes fails all three.
[decisions-edit] — the media.lastscan-null-boundary record documented this
write as an ONGOING sentinel source and rested its "the coercion is permanent"
argument on it, so the rationale prose is corrected in the same PR per
docs.decision-lifecycle. The Rule line is unchanged, so the generated
decisions/README.md catalog is byte-identical.
MediaSourceRepository.cs also loses its UTF-8 BOM (fix-as-you-touch, #311).
fixes#460
The remove-stale + add-new reconcile idiom materializes its add set with
.ToList() BEFORE the loop mutates the existing collection, so the add filter
(`incoming.All(x2 => x2.Name != x.Name)`) is evaluated against a snapshot. Two
identically-named incoming entries whose name is not yet on the existing item
therefore BOTH passed the filter and BOTH inserted — a duplicate row.
Deduplicate the incoming set on the same key the filter compares (Name; Guid
for Guids), in both copies of the idiom:
- PlexMovieLibraryScanner.UpdateMetadata (the original) — genres, studios,
actors, directors, writers, guids, tags.
- JellyfinMusicVideoLibraryScanner.Reconcile{Genres,Tags,Studios,Artists}
(added in #497, mirrors the Plex pattern verbatim).
Plex ACTORS are the exception and get an artwork-preferring dedup hoisted out
and shared with the remove filter, because that filter is keyed on
(Name, artwork-presence) — it is the mechanism that drops an artwork-less actor
so the add loop can re-add it WITH artwork. A bare DistinctBy(a => a.Name)
there keeps the FIRST duplicate, so Plex listing the artwork-less copy first
discarded the artwork; worse, the remove filter would still see the
artwork-less duplicate, making its upgrade clause false, so the stale row was
never removed and the artwork never arrived on ANY later scan either. Actor
also carries Role/Order, which first-wins would silently drop too. Caught by
the cold review of the first version of this commit.
For the Jellyfin scanner the dedup sits at the incoming-list declaration, which
also covers the remove filter — safe because all four of those filters only ask
"is this name present at all", an answer duplicates cannot change.
Tests: duplicate-collapse for both paths, the two Actors cases above, and a
POSITIVE CONTROL proving distinct entries are still all added and stale ones
still removed (without it, a mis-keyed dedup that collapsed genuinely different
entries would pass every other assertion). Each proven non-vacuous.
The Plex tests drive the protected UpdateMetadata through a minimal test-only
subclass, as MediaServerMovieLibraryScannerTests already does.
Low likelihood in practice (a media server emitting two identically-named
genres for one item is unusual); this is defensive, with no observed occurrence.
The same idiom is copied into 8 further scanners/repositories that this change
deliberately does not touch (the issue scoped it to two paths) — filed as #600
so the class of bug is tracked rather than silently left in the majority of its
instances. The dedup rule is recorded under scan.musicvideo-reconciliation.
fixes#500
Three independent CI/repo-hygiene fixes swept together; disjoint file sets.
fixes#586 — E2E cleanup is scoped by PID, never a pattern-wide pkill
- New decision record `testing.e2e-cleanup-scope-by-pid`.
- docs/e2e-local.md states the constraint where a BRIEF-WRITER sees it (the
#586 root cause was a delegation gap, not agent error).
- scripts/e2e-local.sh: reviewed against e2e-ui.sh's trap lifecycle and
deliberately does NOT adopt it — its contract is to hand a running instance
back to its caller, so an EXIT trap would kill the server the instant the
launcher returned (both callers use `OUT="$(e2e-local.sh ...)"`). Recorded.
- Instead it gains what actually prevents the incident: an lsof pre-flight
that NAMES a foreign listener's PID rather than letting Kestrel fail its
bind and surface as "process N exited before becoming ready".
- Pre-flight probes BOTH bound ports, and ETV_STREAMING_PORT now defaults to
ETV_UI_PORT. Program.cs binds a second listener whose port defaults to 8409
independently of ETV_UI_PORT, so `ETV_UI_PORT=8420` alone still bound 8409
and died against a foreign holder — i.e. the documented escape hatch was a
dead end that led straight back to the confusion behind the pattern kill.
fixes#594 — ci-image-pin accepts any hex length
- Length is a separate invariant from correctness: the resolve/staleness
checks compare resolved shas, so an 8-char pin of the right commit passes
green while matching NO registry tag, and all five container: jobs then die
at image-pull with `manifest unknown` (reads like a registry outage).
- Guard fails at the gate and prints the exact tag to use. Verified against
doctored pins: 7 green; 6/8/10 red.
- Uses a literal 7 rather than a derived `--short=7`: in a full clone git may
widen an ambiguous abbreviation, demanding a pin ci-image.yml can never
publish. Escape hatch documented inline.
- Also fixes a pre-existing misdiagnosis: zero pins reported "MORE THAN ONE".
- docs/ci-cd.md documents the 7-char rule and `git rev-parse --short=7 HEAD`.
fixes#485 — .gitignore `core` silently ignored `*/Core/` files
- A bare `core` matched any path component named `core`; case-insensitively
on macOS that swallowed every `*/Core/` SOURCE dir, so new untracked files
were dropped by `git add -A` while tracked ones stayed fine — a clean local
build and a CI checkout that fails to compile.
- Now `/core` + `/core.[0-9]*`, both anchored (an unanchored `core.[0-9]*`
would re-introduce the same silent-exclusion class this fixes).
- Verified by diffing the full ignored-file set before/after: identical, and
the three real Core/ dirs are trackable without -f.
Docs updated in-PR: docs/e2e-local.md, docs/ci-cd.md, docs/decisions/
workflow-process.md (+ regenerated catalog), docs/handoffs/chicorytv-issue-queue.md.
Follow-ups filed: #596 (the same shared-host reap in the Playwright-MCP
recovery record) and the ci-image.yml `--short=7` publisher-side fix, which
cannot ride this PR — editing ci-image.yml re-points ci-image-pin's `expected`
at this commit and reds the gate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Amends ci.monitor-armed-at-pr-open (prose + Signals only; heading and key
unchanged, no supersession) with the monitor-classification rules that were
missing.
Landing #436/#583 today, my CI monitor filtered per-context statuses on
`!= "success"` and announced "NOT all green" on two fully green PRs, because
`Build & push image (amd64)` reports `skipped`. Nothing was blocked — Gitea's
combined /status already treats skipped as non-blocking and reported
overall=success — but a false red costs a diagnosis cycle every time.
Two corrections recorded:
- The image job is skipped on EVERY PR (job-level `if: github.event_name !=
'pull_request'`; images build only on push-to-main and tags), NOT because of
the docs-only mechanism. Misattributing it to docs-only is a plausible-sounding
wrong diagnosis, since docs-only gates STEPS precisely so required jobs still
report success. decisions.md already stated the fact from the branch-protection
angle; the monitor-authoring consequence was missing.
- skipped / failure / cancelled are three distinct meanings and must not be
collapsed. Prefer gating on the combined `.state`.
The documented filter is verified in BOTH directions: silent on a green PR
carrying a skipped build, and still dirty on a genuinely pending run. My first
draft of it was itself broken — `select(.status != …)` after the pipeline had
renamed `.status` to `.st`, so it compared against null and reported a green PR
as nine failures. That failure is recorded in the note, per the "verify your
detector" rule.
fixes#592
Text-only follow-up; no behavior change (14/14 tests, lint clean, tsc clean).
- AutoTuneScreen.tsx: the addSource comment claimed picking a hit that is
already a base member makes "the existing row read as customised". It does
not — patchSource(id, {}) materializes a DEFAULT draft, sourceCustomized is
false for it, and sourcesRequest omits it, so the pick is a payload no-op
whose only visible effect is the query clearing. Comment now states that.
- spa-conventions.md §11: said WEIGHT_MIN/WEIGHT_MAX are "the same const pair
the multi-collection editor uses". Same VALUES, separate screen-local consts
— there is no shared module. The old wording invited a future reader to
assume a shared seam that does not exist.
Both were nits in the independent review of d3c89d87 (verdict
MERGEABLE-WITH-NITS). Fixed rather than deferred because a comment that states
the opposite of the code, and a doc that implies a nonexistent shared const,
are exactly the kind of thing the next session reads and trusts.
Remaining review nits deferred to a follow-up issue: exporting compile.ts's
Lucene escaper instead of duplicating it, an exclude-all warning, and >50-source
axis handling.
Session context/config checkup (/doctor) found ~2.5k chars of always-loaded
CLAUDE.md text that a session can reconstruct from the codebase, plus a
task-specific workflow that only matters when closing an issue.
Cut (derivable from the repo):
- the `### Project Layout` table (what `ls` shows)
- the `dotnet build` / `dotnet run` invocations (standard for the toolchain;
the non-obvious `docker build -f docker/Dockerfile` line is kept)
- the Language / Media / Functional C# bullets (stated by the csproj and
Directory.Packages.props)
Migrated to lazy loading:
- the 7 mandatory completion steps and the `## Closing record` template move
to .claude/skills/closing-an-issue/SKILL.md; only its one-line description
stays resident. The `## Done-when` / merge-consent block stays in CLAUDE.md
on purpose — it is safety-critical and describes hook behaviour that fires
whether or not a skill was loaded.
CLAUDE.md 13,306 -> 10,801 chars (~625 est. tokens saved per session).
No convention, route, endpoint or decision changes, so no other doc updates
are triggered by the docs-update table.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Independent review of d221a065 found the prompt-text heuristic both over- and
under-fired. Confirmed repros: a read-only recon brief merely mentioning
"worktree" nagged, while "author the change and open a PR", "land this on the
branch" and "make the changes and commit them" all passed silently — the gate
missed exactly the case it existed to catch.
Root cause behind all three findings: the hook contradicted its own rule. The
HARD CONSTRAINT says "every dispatched agent"; the hook gated only dispatches
whose prose looked like an implementer. Prompt text is not a reliable signal for
authority.
Inverted: ask whenever `model` is absent, exempting only `fork` (the tool ignores
a fork's model override, so a prompt could not be acted on). This dissolves all
three findings rather than patching the regex — no phrasing dependence, no
false-positive concept, and no "cannot commit" claim to be wrong about. The
reviewer was right that Explore/Plan/claude-code-guide all carry Bash, so the old
exemption rationale was false.
The noise objection is answered by the escape hatch, not by scoping: naming a
tier costs one parameter and the hook never fires. Self-eliminating for anyone
following the rule.
Decision record updated to record the rejected narrow design and why.
Re-verified: 11 payloads incl. all three findings, the invariants (model named ->
never fires; fork exempt; non-Agent passes), and robustness (malformed JSON,
empty payload, missing tool_input, embedded backticks/quotes/newlines) — never
crashes, never emits malformed JSON. decisions-validate: OK.
Wires the Auto-Tune DetailPanel's Content-sources pane to #425's backend: each
member row gains a 1..1000 weight stepper and an include/exclude toggle, and a
library typeahead adds a source that isn't in the axis's base set. Edits
accumulate in the screen's per-channel draft (the existing §8/§11 guard covers
them) and are flushed as the create request's `sources` array.
- Only genuinely customised rows are sent, mirroring the server's own
`customized` predicate — an all-default array is a backend no-op, so the field
is omitted entirely and the channel keeps the cheaper fair-share shape.
- Weights are clamped to 1..1000 on blur and again at save, so an out-of-range
value never reaches the server as a raw 400 (spa-conventions §4a).
- The add-untagged picker compiles typed text to `title:*…*` rather than
forwarding raw Lucene: the index's default field does not match bare title
words, so a raw forward would silently find nothing.
- Removes the read-only #425 hint.
Docs: spa-conventions.md §11 records the per-source correction-row convention.
fixes#440
The kickoff tells the orchestrator to route agents by capability, but that rule
lived only in a prose paragraph. On 2026-07-25 a session dispatched two
implementers (#436, #440) with `model` omitted, silently inheriting the
orchestrator tier — while following every bullet in HARD CONSTRAINTS in the same
session. The bulleted list is what functions as the checklist; prose above it
reads as background.
- HARD CONSTRAINTS: keyed routing bullet requiring the tier to be stated in the
dispatch itself, so an invisible omission becomes visible output. Prose
paragraph tightened to point at the key rather than restate the table (the
kickoff is pasted into every session — duplication is a per-session tax, #542).
- .claude/hooks/pretooluse-agent-model.sh: PreToolUse on Agent, `ask` when a
committing agent is dispatched with no explicit `model`. Narrow by design —
passes through read-only/recon types, `fork` (model override ignored by the
tool), and any dispatch already naming a tier, because a gate that fires on
every fan-out trains one-shot dismissal. `ask` not `deny`: routing is a
judgment call with no derivable right answer, unlike the H6/H10 merge gate.
- New decision record `process.per-agent-model-routing`; catalog regenerated.
Decision matrix verified against 9 payloads incl. a replay of the dispatch that
missed. decisions-validate: OK.
fixes#583
The rule builder's Group nesting was capped at one level (#176's Kodi
model). Generalize it to recursive nesting bounded by a single shared
constant, MAX_GROUP_DEPTH (types.ts, = 5, root group is depth 0):
- parse.ts: replace the allowNested boolean with a depth counter that
recurses to the cap; deeper input stays out of subset (null -> raw-text
fallback), so parse remains the exact inverse of compile. Sub-group
detection now requires the leading '(' to be the one closed by the
trailing ')' (quote/escape aware), so '(a)x(b)' can't be mistaken for
one wrapped group.
- RuleBuilder.tsx: 'Add group' is offered while depth < MAX_GROUP_DEPTH
instead of only at the root; nested group boxes get box-sizing:
border-box so per-level padding can't overflow (no global reset).
- roundtrip.test.ts: the 500-tree generator nests to the cap and asserts
the corpus actually reached it; explicit depth-3 cases added to
compile/parse/validation tests and a depth-gate test to RuleBuilder.
compile.ts and validation.ts already recursed correctly and are unchanged.
No backend/OpenAPI change.
Docs: spa-conventions.md §12; decisions lifecycle — new active record
spa.rulebuilder-nesting, predecessor spa.smartcollection-rule-builder
relocated to docs/decisions/archive/spa.md as superseded.
fixes#436
Closes#415. Server-derived health object on the channel list + detail DTOs (built-timeline detection, kind-agnostic across all 5 PlayoutScheduleKind; assessable gate keyed to the owning channel's mode), single "Problems" SPA filter with per-fault badges. Supersedes #72's api.channel-health-signal decision.
Co-authored-by: Timothy <timothy.look@gmail.com>
Co-committed-by: Timothy <timothy.look@gmail.com>
Adopt the reusable RuleBuilder (#176) for inline query authoring in the Channel
Builder (/app/new-channel). Extract CollectionsScreen's smart-collection dialog
into a shared, self-contained component (SmartCollectionDialog) and consume it in
both screens; the Channel Builder's Collections source gains a "New smart query"
action that persists the authored query as a real SmartCollection and adds it to
the lineup by smartCollectionId. Pure frontend — no REST/MCP surface change (the
MCP already exposes ersatztv_create_smart_collection).
The Auto-Tune half of #437 is a different primitive (group-by, not single-query
filtering) and a backend epic; it is designed separately in
docs/superpowers/specs/2026-07-23-auto-tune-arbitrary-field-design.md and filed as
its own issue rather than wired here.
Verification: web typecheck + lint clean; full vitest suite green (981, incl. a new
inline-smart-query test); cold-context review clean; live-E2E on a real instance
(query authored in the SPA persisted as SmartCollection "Action Picks" and added to
the lineup, 0 console errors).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The GetFieldValues additions were reverted in the DB-sourcing rework; these two
files had only an incidental BOM strip left, which pulled unrelated pre-existing
whitespace debt into the scoped format gate. Restore byte-identical to origin/main.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ISearchIndex.cs / LuceneSearchIndex.cs still carried a BOM after the prior
commit — the strip ran after `git add`, so the staged (BOM'd) content was
what got committed. No content change beyond the BOM.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Edits the api.search-field-values decision record's rationale prose in
place (same key, same date, not a reversal) to describe the DB-sourced
per-field distinct-values design and the narrowed allow-list, replacing
the superseded Lucene-term-dictionary description. Updates the endpoint's
api-conventions.md entry the same way. Regenerated docs/decisions/README.md
via build_decisions_catalog.py; decisions_validate.py passes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Lucene term dictionary stores lowercased word tokens for analyzed text
fields ("Science Fiction" -> science/fiction), so the typeahead was
suggesting fragments instead of whole values. GetSearchFieldValuesHandler
now injects IDbContextFactory<TvContext> and resolves an explicit
per-field-name distinct-values query (genre/studio/director/writer/actor/
artist/tag/network/collection/video_codec/album), with state and
video_dynamic_range computed in memory and content_rating split on '/' to
match what search actually matches on. title/show_title/album_artist have
no distinct source and now correctly 404 (free-text fallback), same as
before. Reverts the GetFieldValues additions to ISearchIndex/
LuceneSearchIndex/ElasticSearchIndex back to their pre-#434 state (BOM
stripped per #311, otherwise byte-identical). Endpoint shape, DTO,
controller, and OpenAPI are unchanged (no diff from
./scripts/update-openapi.sh).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds GET /api/v1/search/fields/{name}/values?q=&limit= — the backend slice of the
visual rule builder's facet-value typeahead (#434). Enumerates distinct Lucene term
values for a text field via MultiFields.GetTerms + TermsEnum, filtered by a
case-insensitive prefix, limit clamped to [1,50]. 404s when the field is absent from
SearchFieldCatalog or is not type "text". ElasticSearchIndex (the optional external
backend) throws NotSupportedException for this method — its text fields are analyzed,
not keyword-mapped, so a terms aggregation isn't safe to guess at without verifying
against a live cluster.
Regenerated OpenAPI trio (v1.json, v1.d.ts, endpoint-index.md).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The rebase onto origin/main (which merged #74's ChannelGraphicsElement
migration) left the PadToNearestMinute migration's embedded Designer.cs
model snapshot stale — it still reflected the pre-#74 model, so EF's
diff against it produced an empty Up()/Down() when naively regenerated.
Reset TvContextModelSnapshot.cs to origin/main's true post-#74 state,
then re-ran scripts/add-migration.sh so the migration's Designer.cs
correctly folds in ChannelGraphicsElement and the migration's Up() adds
only the PadToNearestMinute column. Verified has-pending-model-changes
is clean for both providers.
OpenAPI (v1.json/v1.d.ts/endpoint-index.md) and docs/decisions/README.md
regenerated identically to the auto-merged state, so nothing to commit
there — confirmed both padToNearestMinute and #74's graphics-elements
endpoints/decision key are present.
Stripped a UTF-8 BOM this dotnet-ef/dotnet-format toolchain wrote into
the regenerated migration + snapshot files (known BOM trap, ersatztv#311).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Schedule-level pad control has no prototype counterpart (the schedule-level
scalar form is not modeled in Schedules.jsx); nothing to mirror there.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds parameterized invariant tests (Schedule_clock_padded_offline_multimode)
exercising schedule-level clock pad + offline advance across a 2-day window
(two midnight crossings) through Flood, Duration, and Multiple — previously
only PlayoutModeSchedulerOne had any coverage. Fixture uses sub-15-min content
so each padded item occupies one :15 slot and Duration's fill-the-block
contract tiles exactly (no off-boundary packing).
Part 2 (precision): replaces the day-boundary anchor-clamp magnitude heuristic
(overrun <= one pad interval on a padded schedule) with a precise signal — the
last scheduler now reports the exact offline-pad target it advanced CurrentTime
to (transient PlayoutSchedulerResult.ClockPadOfflineTarget, never persisted),
and the clamp exempts only when CurrentTime equals that target exactly. A
non-offline overrun (Duration/Flood/Multiple ending short of its natural end, a
hard-stop, a tail advance) changes CurrentTime away from the target and still
clamps, so persisted NextStart no longer shifts by up to an interval. No
persisted-schema/migration change. Output byte-identical for all existing
goldens.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Merge pull request '#571 fix(570): On Now/Next overlay YAML renders (font_family + format_datetime)' (#571) from fix/74-overlay-render into main
fixes#570
The #74 seeded on-now-next.yml never rendered at transcode time:
- format_datetime was called with 2 args; it needs 3 (DateTimeOffset, timeZoneId,
format) and does the tz conversion itself, so the Scriban render threw. Drop the
NEXT start-time (avoids hardcoding a timezone in a shipped default).
- styles had no font_family; CustomFontMapper.TypefaceFromStyle crashes on a null
family before its default-font fallback. Add font_family: Noto Sans.
Verified live on ersatztv-test via frame capture. Adds a seeder test that
deserializes the YAML and asserts base_style resolves + every style sets a font.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The lifecycle validator matches records by their `##` heading for removal
detection (gone = base_headings - head_headings), ungated by [decisions-edit].
Renaming the heading read as an unlogged record removal. Restore the exact
original heading -- which stays literally true ("excluded from the golden net":
#395 adds a unit test, not a golden) -- and keep the rationale correction in the
Rule/Signals/body, which is what [decisions-edit] authorizes.
[decisions-edit]
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extract the byte-identical PlaybackOrder -> IMediaCollectionEnumerator switch
shared by SchedulingEngine.EnumeratorForContent (Scripted) and
EnumeratorCache.GetEnumeratorForContent (Sequential/YAML) into one static
per-family seam, mirroring #380's ShuffleSourceBuilder. Each engine keeps its own
"not supported" warning on the None branch, so the per-engine message is unchanged.
Adds ContentEnumeratorBuilderTests pinning the block-shuffle-not-classic trap and
the unsupported-order -> None (#70) contract across all 8 unsupported orders.
Corrects the testing.scripted-playout-golden-deferred decision record: Scripted's
external-process + HTTP pipeline is integration-only (deferred to #563), but the
in-process SchedulingEngine it drives IS unit-testable (ScriptedScheduleController
is a 1:1 pass-through) -- the earlier "un-golden-able by construction" framing
conflated transport with engine. docs/testing.md reframed to match.
[decisions-edit]
fixes#395
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Extends the #163 PlayoutBuildGoldenTests in-memory net to the Sequential (YAML)
builder: a committed fixture (Goldens/Fixtures/sequential-schedule.yml) with two
`count: 2` instructions over one chronological collection, built via
SequentialPlayoutBuilder over the pinned window. The count/all/duration handlers
do UTC-only arithmetic off the caller-supplied start, so the case is
TZ-independent (passes, not skips, under a non-UTC TZ) and needs no Assume guard.
Non-vacuity: a fixture count tweak flips the golden + the contiguity assertion.
Scripted is deliberately excluded from the golden net — ScriptedPlayoutBuilder
shells out via Cli.Wrap to an external process that drives SchedulingEngine over
HTTP, which no in-memory golden can characterize. Recorded as the Done-when
"documented decision" arm in docs/decisions.md
(testing.scripted-playout-golden-deferred) + docs/testing.md; the scripted
integration harness is tracked as follow-up #563.
fixes#381
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Added ChannelId to PlayoutNameViewModel and all 6 construction sites
(Mapper, GetPlayoutByIdHandler, and the Update{,Scripted,ExternalJson,Sequential}
PlayoutHandler commands), plus the list DTO PlayoutListItemResponseModel and the
PlayoutController list projection. Regenerated OpenAPI (v1.json) and the TS client
(v1.d.ts); endpoint-index.md unchanged (no endpoint/operation delta). Simplified
PlayoutsScreen resetSelectedChannel to key directly on selectedSummary.channelId
instead of resolving via channelStates. Updated controller + SPA tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
focus trap added to shared useOverlayBehavior (covers Dialog + SlideOver);
Tab/Shift+Tab now cycle within the panel; test added.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Re-review of 60a0c505 caught a regression the prior fix introduced: onRetry cleared
the token cache and bumped playToken but kept the old resolvedSrc, so HlsPlayer
reloaded the stale-token URL before the remint resolved — a duplicate manifest
session and a stale 401 that could stick the panel as failed even after the fresh
stream succeeded.
Null resolvedSrc in onRetry before bumping playToken so the player unmounts until the
async effect resolves the freshly-minted URL. Added a controlled-async test proving
the stale-token URL is never reloaded and the retry loads the new token (validated by
negative control: the test fails with the fix removed, and only that test).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cold review (no Critical/High). Folded:
- Low: clamp JWT:BrowserTokenLifetimeMinutes to a 24h max so a seconds-vs-minutes
typo can't mint a multi-year bearer token (non-positive/unparseable still falls
back to 60 min).
- Low: reset the SPA iptv-token cache on the preview panel's Retry and on each
troubleshooting Play, so a stale token (key rotated) or a stale "JWT disabled"
latch (backend reconfigured since page load) can't wedge a user-initiated retry.
Deferred to #559 (tracked): redact access_token from Serilog request logs and set
no-store on token-bearing /iptv manifests — pre-existing properties of the shared
?access_token= transport (Jellyfin/M3U already use it), now bounded by the 60-min
lifetime; cross-cutting fixes beyond this feature's scope.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Under a JWT-enabled deployment (JWT:IssuerSigningKey set), /iptv/* is gated by
ConditionalIptvAuthorizeFilter and the "jwt" scheme does not accept the SPA's
ctv-session cookie, and nothing minted a JWT for the browser. So the #60 channel
preview was declared Unavailable and could not run at all.
Add GET /api/v1/auth/iptv-token (session-gated, on the [IgnoreApi] AuthController):
mints a short-lived global token via JwtHelper.GenerateBrowserToken (60 min default,
JWT:BrowserTokenLifetimeMinutes override), 204 when JWT is disabled. The SPA's new
withIptvToken(url) helper appends it as ?access_token= to the manifest URL (a no-op
when JWT is off), used by the channel-preview panel and the troubleshooting screen.
Mapper.GetPreview drops its iptvJwtEnabled -> Unavailable guard; preview is now
JWT-agnostic.
Live-E2E under JWT: /iptv manifest 401s without a token and passes with a valid one
(garbage token -> 401); token endpoint 401s anonymous, mints with a session.
Honest finding: the issue's point 2 (troubleshooting screen broken under JWT) does
not reproduce -- its live.m3u8 is static-served (UseStaticFiles at /iptv/session),
outside the JWT filter, so it was never gated. The withIptvToken call there is a
harmless defensive no-op.
Docs: security.iptv-browser-token (api-auth-security.md), amended
api.channel-preview-capability, spa-conventions §5b. No OpenAPI change (IgnoreApi +
unchanged Preview schema).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The "stops scan polling and refreshes sources once when scans complete" test
fired the poll tick via runPollTick but discarded the promise loadScanStatuses
returns, so the progress-clear state update and the fire-and-forget
loadSources() refetch it triggers landed on real microtasks AFTER act() had
resolved. The test out-waited that race with two waitFor({ timeout: 5000 })
calls, which were marginal on a starved CI VM (timed out on PR #509/run 918);
prior timeout bumps (#447) were diminishing whack-a-mole.
Drive the chain to completion deterministically instead of out-waiting it:
runPollTick now awaits the promise the handler returns (settling the
progress-clear) and then yields to a single macrotask (setTimeout 0) to drain
the microtask queue — including the fire-and-forget loadSources() refetch —
all INSIDE act(async () => ...). The mock fetches resolve synchronously on
microtasks and setInterval is the only mocked timer, so one macrotask turn
completes both chains with no wall-clock delay. The two waitFor calls are
replaced by synchronous assertions (progress label gone; exactly one extra
/api/v1/media-sources fetch), and the 20s test-level budget is dropped.
Verified: LibrariesScreen suite green 15/15 consecutive runs; npm run lint and
npm run build clean. No production code changed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Fixes#512
scripts/select-queue.sh ranked Renovate's Dependency Dashboard (#22) as an
ordinary `priority: low` candidate. With an all-low backlog ordered by issue
number, #22 sorted to the top and was proposed to every fresh session — a bot-
rewritten status board whose checkboxes are commands to Renovate, not work
items. A session trusting the selector's "trust the ordering" contract would
either waste a pickup or make the exact undocumented judgment call the script
exists to eliminate (this session hit it live).
Drop bot-authored issues in the same jq pass that drops PRs/in-progress/parked:
match login `renovate` plus the GitHub `name[bot]` convention so a future bot
dashboard is excluded too. A bot's actionable output is PRs (already excluded);
it never files a human work-item issue, so the whole class is never a pickup.
Also make "scan for bundle-able siblings after claiming" an explicit kickoff
step (new step 4) with a third bundle axis — shared label / adjacent subject —
so a session sweeps small independent same-label issues together (e.g. this
change's own #553 + #512 ci-cd hygiene bundle) instead of closing one at a time.
Verified: selector no longer lists #22; genuine backlog issues still rank; the
only bot-authored open issue is #22. shellcheck clean.
Fixes#553
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The skill told every session that music videos are typed "Movie" in
Jellyfin. That described a deliberate DB reclassification workaround which
existed only because ErsatzTV could not consume MusicVideo items. #42
shipped that sync (and #177 just extended it), so the workaround's premise
is gone -- but the note outlived it and would send a session querying
includeItemTypes=Movie and finding nothing.
Verified live: the Music Videos library (/data/music, collection type
"musicvideos") holds 1437 items typed MusicVideo and zero typed Movie.
Also records two Jellyfin API traps found while working #177:
- Album is NOT an ItemFields value. It is a plain BaseItemDto property
serialized whenever set, so it returns regardless of `fields=` -- adding
it there would be cargo-culted from Genres/People/Chapters, which ARE
ItemFields. 111 of 1437 items returned Album with fields=Path alone.
- IndexNumber is the track number; ParentIndexNumber is the disc/season
axis. Frequency misleads: ParentIndexNumber is populated 16x more often
(66 vs 4), but where both exist it is 1 while IndexNumber holds the real
ordinal, and alone it is a collection grouping tracking the album.
Scoped to this repo's copy. The canonical skill is owned by
server-management and is corrected separately -- see the issue filed there.
refs #177
The reset moved from render-phase into an effect; the existing switch test
asserted only that the error banner cleared, which stays green even if the
effect is deleted. Now the new channel must also reach 'playing'.
Negative control: disabling the reset fails exactly this test, and only it.
Refs #60
Jellyfin-sourced music videos rendered weaker MTV-style credits than local
NFO libraries: the Scriban credits templates expose Album/Track, and
MusicVideoNfoReader has always mapped both, but the Jellyfin projection
never did. ChronologicalMediaComparer orders music videos by the same two
fields, so they were also ordering worse.
Verified against the live server (1437 MusicVideo items): Album comes back
on 111 and IndexNumber on 4, both WITHOUT being named in the `fields` query
param -- Album is a plain BaseItemDto property, not an ItemFields value, so
no Refit `fields` change is needed (and adding one would be wrong).
ParentIndexNumber is deliberately NOT used for Track: on live data, where
both are present ParentIndexNumber is 1 while IndexNumber carries the real
ordinal, and where only ParentIndexNumber is present it is a collection/disc
grouping that tracks the Album ("Glastonbury: 2022" -> 230).
The fix is two layers, not one. The projection alone would only ever reach
music videos ADDED after it -- UpdateMetadata copies scalars field by field,
so an existing item whose album/track is set or corrected in Jellyfin would
keep a stale value forever. That is the same class of bug #497 fixed for
child collections, one layer up.
Also strips a pre-existing UTF-8 BOM from JellyfinLibraryItemResponse.cs,
which the format gate flags once the file is touched (format-as-you-touch).
fixes#177
- ChannelPreviewPanel: a manual play-button click on a video already
showing a fatal error was clearing the error, silently hiding the
fault the panel exists to reveal. onPlaying now ignores the event
while a fatal error is showing (tracked via a ref, reset in an
effect keyed on channel.id); Retry remains the only way to clear it.
- shell.css: .ctv-preview-facts spacing was dead — equal-specificity
.ctv-detail-infogrid{margin:0} later in the file won. Raised
specificity with a compound selector instead of touching
.ctv-detail-infogrid, which MediaDetailScreen also relies on.
- ChannelPreviewTests: added two cases exercising two simultaneously-
true Unavailable causes, so the documented guard precedence in
Mapper.GetPreview is actually pinned by a test.
- design doc: fixed a garbled sentence describing which DTO gained
the Preview field.
The design spec claimed ChannelDetailResponseModel also gained the Preview
field; only ChannelResponseModel did (deliberate — nothing consumes it on the
detail DTO). Also record the two new Unavailable causes (disabled channel,
zero playouts) added to Mapper.GetPreview in the api.channel-preview-capability
decisions.md record.
- HlsPlayer: drive onPlaying from the <video> element's own `playing` event on
BOTH the hls.js and Safari-native paths instead of MANIFEST_PARSED, which
fires before any media has decoded (an HttpLiveStreamingDirect manifest
always parses, even over a black video). MANIFEST_PARSED now only kicks
play(). Restore `void video.play().catch(...)` at both call sites and stub
HTMLMediaElement.prototype.play in setupTests.ts instead, so the `?.` that
existed only to survive jsdom is gone from production code.
- HlsPlayer.test.tsx: assert the auto-recovery guard against hls.js's own
startLoad()/recoverMediaError(), not just loadSource's call count.
- ChannelPreviewPanel: reuse existing ctv-* classes (ctv-channels-error,
ctv-settings-warn-callout, ctv-detail-actions, ctv-detail-infogrid) instead
of five undefined ctv-preview-* classes; add the two genuinely new rules
(ctv-preview-video max-width, spacing tweaks) to shell.css.
- Add an exported ChannelPreviewAvailability union (web/src/api/channels.ts)
so a typo like 'ForcedHLSOnly' fails to compile instead of silently
disabling a branch forever; use it in ChannelPreviewPanel's prop type and
at the ChannelsScreen comparison sites.
GetPreview keyed only on StreamingMode + JWT, so a disabled channel or one
with no playout was declared Available and then failed confusingly (404 from
IptvController, or an indefinitely-blocking manifest request). Extend it to
take isEnabled + playoutCount and check JWT, then disabled, then no-playout,
before falling back to the existing mode-based rules; order is documented
in a comment. Also corrects a stale comment in ChannelPreviewResponseModel.cs
that claimed a C# string generates a TypeScript union (it does not).
Wires the ChannelPreviewPanel (Task 4) into ChannelsScreen: a single
panel instance is rendered per screen and its `channel` prop is swapped
via previewChannelId state rather than remounting per row. The Play
button now reads the server-derived channel.preview.availability
(Task 2) instead of being permanently disabled -- Unavailable stays
disabled with the server's unavailableReason surfaced in the title;
Available and ForcedHlsOnly both enable it, since the panel itself
handles the forced-HLS opt-in and caveat.
Also fixes App.test.tsx's #244 channel fixture, which lacked the now-
required preview field and crashed once ChannelTableRow started
reading it.
Two review findings on the channel preview panel (#60):
- ChannelPreviewPanel's synchronous render-phase reset (started/state/error/playToken
on channel.id change) was reachable in prod (the channels screen keeps one panel
mounted and swaps the channel prop) but untested. Added tests proving no auto-start
switching into a ForcedHlsOnly channel, error clearing on switch between Available
channels, and no playToken leak across the switch.
- PlaybackState included 'playing' but nothing ever set it. Added HlsPlayer onPlaying,
fired from Hls.Events.MANIFEST_PARSED and the native-HLS <video> 'playing' event,
mirroring onError's optional/stable-callback contract; ChannelPreviewPanel now wires
it to reach 'playing'. Also fixed a latent bug hit while exercising this path:
video.play().catch(...) assumed a Promise, but jsdom's play() returns undefined.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Renders an in-browser HLS preview of a channel (operator diagnostic).
Deviates from the plan brief per updated requirements: an explicit
user-initiated Retry control replaces the hardcoded playToken, the
forced-HLS caveat renders both before and after opting in, and the
caveat string is exported verbatim as FORCED_HLS_CAVEAT.
Review findings on the Task 1 commit (f6ff3b63):
1. Mapper.GetPreview is internal to ErsatzTV.Application, granted only to
ErsatzTV.Tests by convention (one-assembly-one-test-project). Move
ChannelPreviewTests.cs from ErsatzTV.Core.Tests to
ErsatzTV.Tests/Application/Channels, and revert the second
InternalsVisibleTo entry added to ErsatzTV.Application.csproj for
ErsatzTV.Core.Tests.
2. GetAllChannelsForApiHandlerTests.cs constructed `new
GetAllChannelsForApi()` at three sites, which no longer compiles now
that the record requires IptvJwtEnabled. Pass IptvJwtEnabled: false at
each site (all three tests are about plain channel listing/logo
mapping, not JWT).
ChannelController.GetAll's missing argument remains, deliberately, for a
later task.
Adds ChannelPreviewResponseModel + ChannelPreviewAvailability constants
and Mapper.GetPreview(streamingMode, channelNumber, iptvJwtEnabled),
threaded through ProjectToResponseModel's new iptvJwtEnabled parameter
and GetAllChannelsForApi's new IptvJwtEnabled property.
Also adds ErsatzTV.Core.Tests to ErsatzTV.Application's
InternalsVisibleTo list (was ErsatzTV.Tests only) so the new
ChannelPreviewTests can call the internal Mapper methods it's testing.
ChannelController.GetAll is left failing to compile (Task 2 wires
JwtHelper.IsEnabled in at the controller).
Also simplifies the spec's guide correlation: ChannelStateResponseModel
already carries NowPlaying and ChannelsScreen already holds it, so the
panel needs no /api/v1/guide fetch and no timer.
Refs #60
Verification-oriented HLS preview on the channels list: server-declared
per-channel Preview capability (Availability/ManifestUrl/UnavailableReason),
a SlideOver panel over the existing HlsPlayer, and an onError extension so
playback failures stop being silent.
Refs #60
CreateChannelFromLineupHandler resolved every advanced override with
advanced.X ?? template.X, so null always meant INHERIT and a channel could
not drop a template-set watermark / filler / preferred language. Add an
optional typed `clear` enum list to CreateChannelFromLineupAdvancedOptions:
omitted/null still inherits (byte-stable for existing clients), a field named
in `clear` is forced to none. Set+clear of the same field is a 422.
The enum (CreateChannelFromLineupClearField) lives in ErsatzTV.Core so the
OpenAPI string-enum scan renders it as a string enum, matching every sibling
advanced-options enum. Handler resolves clearable fields once via
ResolveClearable and validates set/clear conflicts via ValidateClear;
reference validation skips existence checks for cleared (null) refs.
SPA: the shared advancedOptions model re-adds a real "None" option to the five
id selects (watermark + fillers) in both the Channel Builder and the Auto-Tune
DetailPanel, routed through a CLEAR overrides sentinel that applyOverridesToRequest
folds into advanced.clear (never leaking onto the wire as a field value). The
backend enum also covers the preferred audio/subtitle language strings for
machine clients; the SPA text inputs keep "empty = inherit" (tri-state deferred).
Docs: api-conventions.md §2, spa-conventions.md §11, decisions.md record
api.from-lineup-clear-to-none; v1.json + generated TS regenerated.
fixes#135
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three Low findings from the #536 clamp re-review, unreachable today (one
guarded release site) but filed against the day a second release site is added.
- §1: Release() now reads the count and CAS-decrements only when current > 0,
so it never publishes a negative count even transiently. The prior
decrement-first-then-clamp shape dipped to -1, which a concurrent TryAcquire
could read as phantom room and over-admit at the limit (re-opening the #529
QSV pool exhaustion). It records the unbalanced release synchronously on the
offending thread rather than blaming a later innocent release.
- §3: Release() returns bool; HlsSessionWorker logs a warning on the false
(unbalanced) return — the one in-band signal a future second release site
would need. WorkAheadSlots stays logger-free by design.
- §2: UnbalancedReleases doc-comment corrected — it can under-count (an
over-release while count > 0 cancels a coexisting leak and goes unrecorded);
no false positives, but zero does not prove correctness.
Test: Release_Unbalanced_NeverPublishesNegativeCount (2M unbalanced releases vs
4 count-samplers) with a documented, verified negative control (reverting to
the decrement-first body makes readers observe the transient -1).
Adds a decisions.md entry (ffmpeg.work-ahead-slot-release-never-negative).
fixes#539
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The skill still described archived upstream v26.3.0: "MediatR + Blazor (not
REST for mutations)", "No REST CRUD for channels/collections/schedules —
must use SQLite DB directly", and the ghcr.io image. All false for this
fork — Blazor was removed in #91b and /api/v1 has full write paths. A
session trusting it would hand-edit SQLite for something the API does.
Also adds what this session had to discover by hand while driving the API:
the key is root-owned so an unsudo'd read fails SILENTLY (empty header ->
401 body that parses as a dict -> a naive script reports "0 channels"
rather than an auth error); settings live at /api/v1/settings/ffmpeg, not
ffmpeg/settings; test is port 8410 and the service-scoped manual refresh
command for validating before the 03:00 auto-update.
Two measurement traps recorded with them: OCI labels are inherited from the
linuxserver base image and lie about what is running (compare .Image to the
registry Docker-Content-Digest), and container log brackets are local time
while `docker logs -t` is UTC, so --since windows mis-slice.
Endpoint lists now defer to docs/endpoint-index.md rather than being
re-maintained here, since a hand-copied list is what drifted in the first
place.
The decisions-guard validator enforced lifecycle metadata (key/status/since/
supersedes/superseded-by + reciprocal links) but not the **Signals:** line —
which is exactly what MemPalace's keyword recall matches on. A record without
it ingests with weak recall metadata and produces confident false-negatives
for the "MemPalace to find, file to confirm" retrieval workflow.
Add "signals" to REQUIRED_META so a migrated record with a missing or empty
**Signals:** line fails the same way a missing key does. All 114 active + 2
archive records already carry a Signals line, so this is non-breaking on the
current corpus. Archive records are intentionally out of scope (recall targets
the active corpus).
- scripts/decisions_validate.py: signals in REQUIRED_META (+ rationale comment)
- scripts/tests/test_decisions_validate.py: _rec() default + missing/empty/present cases
- docs/decisions.md: header + Enforcement note the requirement and why
fixes#545
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Timothy asked why the kickoff handoff doc stores historical narrative when
it should be instructions. It shouldn't — its own lore section is chartered
as "STANDING workflow/orchestration rules only" with the why belonging in
docs/decisions.md. But an inventory of every bullet against the decision
corpus inverted the premise: only ~8 of ~38 were actually covered. 19 had
no record anywhere and 11 more were half-covered, so that single file was
the ONLY copy of the mandatory review rubric, the whole CI-triage
vocabulary, the build-concurrency policy, the H12 session-end audit, and
the plumbing-merge recipe. Pruning first would have destroyed them.
So the records come first. New topic file docs/decisions/workflow-process.md
carries 32 records (ci.*, process.*, testing.*) covering every NONE and
PARTIAL the inventory found, including the Gitea `?milestones=` no-op bug
whose only copy was the archived selector section this prune deletes.
Only then the prune: HARD CONSTRAINTS and the lore section become one- or
two-line rules, each citing the decision key that holds its evidence, and
the 40-line "Archived — do not follow" section is gone. 636 -> 353 lines,
with every cited key verified to resolve against the corpus.
The aggregate corpus budget is re-baselined 4800 -> 5600 with the reason in
the code: the corpus grew because knowledge MOVED into it, which is the
system working, not drift.
refs #542
A session was handed docs/handoffs/chicorytv-issue-queue.md pasted out of
/Users/timothy/ersatztv while that tree was 81 commits behind, so it still
described the queue protocol #520 retired the day before (read tracker
command was ever run against that tree, so every existing "never read its
HEAD" guard was irrelevant: a stale checkout serves stale FILES, and docs
are what a kickoff depends on. Nothing broke only because selection went
through scripts/select-queue.sh.
The lore bullet on that tree already prescribed the shape of the fix for
its earlier failure modes — "a design flaw, not a discipline failure; a
check does not stay true" — so this removes the stale condition instead of
adding another check.
scripts/refresh-shared-checkout.sh fast-forwards the tree to origin/main
and reinstalls web/node_modules when the lockfile moved. It is deliberately
timid: it refuses and changes nothing when the tree is not on main, is
dirty, is ahead, or is mid-rebase/merge, and it never switches branches,
stashes or discards. A NO-OP is a normal outcome.
Uses npm ci rather than npm install — the first version used install,
which rewrote package-lock.json and left the tree dirty, i.e. the exact
state the next run refuses on, so it would have disabled itself after one
use. Asserts the tree is clean at exit.
refs #541
An on-demand channel (`PlayoutMode.OnDemand`) already is the "resume where I
left off" feature: `Playout.OnDemandCheckpoint` persists the viewer's position
and `PlayoutTimeShifter.TimeShift` slides the materialized timeline forward on
tune-in so the paused item is active again. Because it rewrites `GuideStart`/
`GuideFinish` alongside `Start`/`Finish`, guide and playback freeze together —
structurally avoiding the free-running-wall-clock desync #68 was filed about.
The one gap: `TimeShift` rewrote the stored `PlayoutItem` rows but the XMLTV
guide is served from a cached fragment that only `RefreshChannelData` rebuilds,
and the tune-in path never enqueued it. So an external EPG client polling after
a thaw could see a stale timeline until the next incidental rebuild.
Fix: `IPlayoutTimeShifter.TimeShift` now returns the channel numbers whose cached
guide is stale — the shifted channel plus any channels that mirror it (the same
fan-out `BuildPlayoutHandler` already does) — and `TimeShiftOnDemandPlayoutHandler`
enqueues a `RefreshChannelData` for each on `CancellationToken.None` (post-commit
side effect must not be abandoned if the session token cancels).
Tests: handler enqueues a rebuild per stale channel (+ mirror + no-shift cases);
`PlayoutTimeShifter` reports source+mirrors on a shift, empty on Continuous /
zero-offset / active-unforced, and correctly seeds+shifts a never-watched playout.
Non-vacuity of the enqueue proven by a compiling negative control.
Docs: channels.md (On-demand resume section), domain-model.md, decisions.md
(scheduling.ondemand-guide-refresh-on-thaw). Per-viewer resume is out of scope
(single per-channel checkpoint; #68 says per-channel suffices).
fixes#68
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Pre-push adversarial review, Low finding. The pool is process-wide and
lives for the life of the app, so a `Release()` not matched by a
successful `TryAcquire` would drive the count to -1 permanently: with a
limit of 1 that silently admits two unthrottled transcodes forever, which
is exactly the #529 QSV pool exhaustion with nothing in the logs to find
it by. Clamp at zero and record the breakage in `UnbalancedReleases`
rather than throwing — the sole caller releases from a `finally`, where a
throw would swallow the real exception.
The hammer tests now also assert `UnbalancedReleases == 0`, so the clamp
cannot mask drift it was added to survive.
Also moves the #536 index line to the end of the in-file decisions index
(it was inserted in the 2026-07-11 block while its body appends at the
end) — review nit, anchors were already correct.
refs #536
The slot check and its increment straddled an await: `Run` compared
`Volatile.Read(ref _workAheadCount)` against a DB-backed limit, and the
increment happened later inside `Transcode`. Every simultaneous tune-in
therefore observed `0 < limit` and started unthrottled — three concurrent
tunes on prod with a limit of 1 all ran with no `-readrate`. `Interlocked`
on the write side alone buys nothing when the read side is a separate,
earlier load (same class as #231/#250).
Extract the counter into a `WorkAheadSlots` pool whose `TryAcquire(limit)`
claims via compare-exchange, so the count never even transiently exceeds
the limit that the QSV hardware-frame pool sizing (#529) is derived from.
`Run` claims the slot and passes ownership in; `Transcode(bool
ownsWorkAheadSlot, ...)` derives `realtime` from it and releases it in its
existing `finally`, keeping acquire/release one-for-one. Acquisition stays
in the caller because `Run` sets `_state` from the outcome and `Transcode`
reads that state on entry to pick the item start time.
Tests hammer 8 threads x 20k rounds (a single Barrier round does not
collide on this hardware); the documented negative control reinstates the
check-then-act body and produces 15912 over-claiming rounds of 20000.
Also annotates the #350 decision record, whose "every concurrent tune-in
falls back to the throttled path" bullet described the intent rather than
the behaviour.
fixes#536
Three PR-only git-diff gates (ci-image-pin, docs-reminder, decisions-guard)
lived in docker-build.yml, which also triggers on push to main and v* tags.
Gitea dispatches a job as a runner task even when its `if` skips it, so on
every tag/main push these three were dispatched to the `small` lane just to
evaluate the skip. On the v26.12.0 tag those dispatched skip-tasks wedged in
act's setup phase and were killed by a runner restart mid-setup, reporting
`failure` (no logs) and reddening the tag's overall commit status even though
the release built, scanned, and deployed fine. The two identical-`if:` jobs on
ubuntu-latest (api-docs, format) skipped cleanly — the job logic was never the
problem; the kill lands in the dispatch window before any step or skip runs, so
tweaking the `if:`/step logic could not fix it.
Relocate exactly those three (pure checkout + git-diff, no container:, no image
pin) verbatim into a dedicated pr-checks.yml that triggers `on: pull_request`
only. Gitea evaluates a workflow's trigger before creating any job, so on a
tag/main push this workflow produces zero jobs: no dispatch, no kill, no
spurious red — for the whole class, permanently.
- ci-image-pin carries no pin and still greps docker-build.yml, where all five
pin-bearing jobs (test/migrations/functional-e2e/api-docs/format) remain, so
its validation is unchanged.
- None of the three are required checks (only Build & test + EF migration
integrity are), so the status-context prefix change (Build ErsatzTV Image / ...
-> PR Gates / ...) does not affect merges; the merge-consent hook reads the
prefix-agnostic combined status.
- pr-checks.yml declares `defaults: run: shell: bash` (ci-image-pin uses
mapfile / set -o pipefail).
docs/ci-cd.md: new "PR gates workflow" section + cross-references.
fixes#535
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Exhaustive triage of the closed tracker ersatztv#237, whose 111 comments
server-management#642 excludes from MemPalace ingestion (over the per-file
cap) and #520 removes from startup. #524's premise was that facts living only
in those comments would be orphaned and need curating into lifecycle records.
Result: zero decision-shaped orphans. Every durable decision-shaped fact is
already held by the decision corpus or by the individual issue the comment
narrates -- which the exporter does ingest. The tracker was always the lossy
copy, because the session protocol required the fuller closing record on the
worked issue first.
- docs/decisions.md: new active record docs.tracker-comment-retrofit, leading
with the reusable rule (check the worked issue BEFORE the decision corpus)
and the consequence for #642's benchmark row, which has no valid subject.
- docs/tracker-retrofit-triage-237.md: the audit trail -- method, per-comment
classification of all 111, totals, and the one candidate raised and
disproved (#497's Guids/Directors scope, stated more fully on its own issue).
- docs/handoffs/chicorytv-issue-queue.md: sweeps the two genuinely orphaned
LORE facts the triage surfaced (e2e-local.sh readiness probe hanging on a
reused config dir; troubleshooting playback cannot exercise channel branding).
- docs/README.md: index the new doc.
The claim is deliberately narrow: no *decision-shaped* orphans. The lore bucket
was classified but not coverage-checked, and it was not empty -- hence the sweep.
fixes#524
# A transport/parse failure must not look like a legitimate short final page: `gq` returns empty
# on any error, which counts as zero rows and would set files_complete=yes over a PARTIAL list —
# failing OPEN into the exemption.
#
# Checking only the top-level type leaves the same hole one level down: `[{}]` is a valid array
# whose rows carry no `filename`, so it yields no paths, looks like a short page, and completes
# the enumeration from a partial list. Require every row to carry a non-empty string `filename`
# (an empty array is still valid — that is a genuine end-of-pagination). This also rejects arrays
# of scalars, which would otherwise make the `.filename` extraction below fail under `set -e`.
if ! printf'%s'"$raw"\
| jq -e 'type == "array" and all(.[]; (.filename | type == "string" and length > 0) and (if .status == "renamed" then (.previous_filename | type == "string" and length > 0) else true end))'\
>/dev/null 2>&1;then
files_complete=no;break
fi
# BOTH sides of a rename: Gitea reports a `git mv` as ONE row whose `filename` is the DESTINATION,
# with the source in `previous_filename`. Reading only `filename` would let a PR move code into
# docs/ and claim the docs-only exemption. Page size is measured in ROWS, not paths — one renamed
"") decide ask "H6 merge gate: could not read CI status for PR #$pr ($sha). Verify CI is green before merging.";;
*) decide deny "H6 merge gate: BLOCKED — PR #$pr CI status is '$state', not 'success'. Wait for a green build (or pass merge_when_checks_succeed to let Gitea gate it) before merging.";;
*)
# `review-verdict/h10` is itself one of the contexts folded into the COMBINED state, so a PR
# awaiting its verdict reports combined 'pending' and would otherwise be reported as a CI
# problem — sending the reader to build logs when the missing thing is the review. Name the
# real blocker when the verdict is the only thing outstanding.
#
# "Not green" is anything that is not `success`, NOT just pending/failure: Gitea also has
# `error` (and `warning`), and omitting those would let an errored build hide behind the
# verdict and produce the flatly false claim "every CI check is green". `skipped` IS treated
# as green — the image-push job skips on every PR (ersatztv#593: a skipped context is not red).
| if (length == 1 and .[0].context == "review-verdict/h10") then .[0].status else "" end' 2>/dev/null ||true)
case"$vonly" in
pending)
decide deny "H6/H10 merge gate: BLOCKED — every CI check on PR #$pr is green; the only outstanding context is 'review-verdict/h10' on head ${sha:0:7}, i.e. this head has no review verdict yet. Review it and run: scripts/post-review-verdict.sh $pr MERGEABLE";;
failure|error)
decide deny "H6/H10 merge gate: BLOCKED — every CI check on PR #$pr is green, but 'review-verdict/h10' is '$vonly' on head ${sha:0:7}: this head was reviewed and REJECTED. Resolve the findings, then run: scripts/post-review-verdict.sh $pr MERGEABLE";;
esac
decide deny "H6 merge gate: BLOCKED — PR #$pr CI status is '$state', not 'success' (not green: ${nongreen:-unknown}). Wait for a green build (or pass merge_when_checks_succeed to let Gitea gate it) before merging."
;;
esac
else
# --- SCHEDULED auto-merge: everything this hook proves is a SNAPSHOT (ersatztv#622). ----------
# With merge_when_checks_succeed, Gitea performs the merge later, against whatever head is green
# at THAT moment — but (b) and (c) below are evaluated against the head that exists right now.
# Any commit pushed in between would merge with no verdict covering it. Demonstrated as a
# controlled A/B (#622): with a slow CI check pending so Gitea waits, an unreviewed commit pushed
# after scheduling MERGED without the required verdict context and was REFUSED with it.
#
# The durable fix is server-side and lives outside this hook: `review-verdict/h10` is a REQUIRED
# status check on `main`, and a commit status belongs to exactly ONE sha, so a later commit cannot
# inherit it and Gitea's own gate refuses to merge until that head is re-reviewed.
#
# What we add HERE is the matching precondition at SCHEDULING time: refuse to arm an auto-merge
# unless the sha-bound status already exists on this head. Checking the comment alone (condition
# (c) below) is not enough for this path — the comment is what a human reads, the status is what
# the server enforces, and only the latter survives a new push. Deny rather than ask: the remedy
# is a single documented command, so there is nothing here for a human to adjudicate.
[ -n "$sha"]|| decide ask "H6 merge gate: could not resolve PR #$pr head sha to check the review-verdict status. Verify the review covered the latest commit before scheduling an auto-merge."
# Read the COMBINED endpoint, not `/statuses/{sha}`: the latter returns one row per status POST
# rather than per context and pages at 50, so a head with a few CI reruns can push the verdict off
# the first page and read as absent — a confusing false deny. The combined endpoint returns
# latest-per-context, which is exactly the question being asked.
if ! printf'%s'"$vjson"| jq -e '.statuses | type == "array"' >/dev/null 2>&1;then
decide ask "H6/H10 merge gate: could not read the 'review-verdict/h10' status for PR #$pr head ${sha:0:7} (Gitea unreachable or an unexpected response). Confirm the current head is reviewed before scheduling an auto-merge."
"") decide deny "H6/H10 merge gate: BLOCKED — PR #$pr has no 'review-verdict/h10' commit status on head ${sha:0:7}, so scheduling an auto-merge would freeze consent at a head Gitea may not be the one to merge (ersatztv#622). Review the current head and run: scripts/post-review-verdict.sh $pr MERGEABLE";;
pending) decide deny "H6/H10 merge gate: BLOCKED — 'review-verdict/h10' is still pending on PR #$pr head ${sha:0:7} (no verdict posted for this commit yet). Review the current head and run: scripts/post-review-verdict.sh $pr MERGEABLE";;
*) decide deny "H6/H10 merge gate: BLOCKED — 'review-verdict/h10' is '$vstate' on PR #$pr head ${sha:0:7}. Resolve the findings, then run: scripts/post-review-verdict.sh $pr MERGEABLE";;
decide ask "H10 merge gate: could not fetch PR #$pr comments to verify a head-referencing review verdict ($short). Confirm the adversarial/Codex review covered the latest commit before merging."
fi
# Verdict lines across all comment bodies: a real verdict line STARTS with the marker (after optional
# leading whitespace). Anchoring to line-start is deliberate — it rejects a comment that merely QUOTES
decide ask "H10 merge gate: no 'Review-verdict:' comment found on PR #$pr referencing head $short. Post the adversarial/Codex verdict (e.g. 'Review-verdict: MERGEABLE @ $short'), or confirm the review covered the latest commit and approve."
# Classification is delegated to `scripts/check-review-verdict.sh` — the single source of truth for
# the H10 grammar, extracted in #629 so it could be TESTED. While it lived here it had none, and three
# false-opens survived in it: a prefix-matched token (`MERGEABLE-LATER` graded positive), a verdict
# inside a fenced code block (documentation showing the convention counted as a real verdict), and a
# sha taken from the first `@<hex>` anywhere on the line (a markdown link could supply it). Every
# decision the classifier makes is documented there; this file only maps a class onto a hook decision.
decide ask "H10 merge gate: verdict classifier not found at $verdict_script, so the review state can't be derived. Confirm the review covered the latest commit before merging."
fi
# An input error (exit 2) is NOT a classification — fall through to a human rather than guessing.
if ! class=$(printf'%s'"$comments"|"$verdict_script" --head "$sha" 2>/dev/null);then
decide ask "H10 merge gate: could not classify the review verdicts on PR #$pr (malformed comments payload or unreadable head). Confirm the review covered the latest commit ($short) before merging."
fi
# Classify each verdict line by the sha it references (its "@ <sha>" field) and its verdict word.
# A line references the CURRENT head iff head BEGINS WITH that sha token AND the token is >=7 chars
# (git short-sha prefix semantics) — NOT a loose substring test: an older sha that merely contains
# the head prefix, or the head prefix appearing in an unrelated URL on the line, must NOT count
# (adversarial false-opens). The verdict token must sit right after the marker on the same line.
head_pos=0;head_neg=0;stale=0
whileIFS=read -r line;do
[ -n "$line"]||continue
# The sha the line references: the hex token in its "@ <sha>" field (>=7 chars), lowercased.
ref=$(printf'%s'"$line"| grep -ioE '@[[:space:]]*[0-9a-f]{7,40}'| head -1 \
# A negative verdict on head wins over a positive one (a later BLOCKED retracts an earlier MERGEABLE
# on the SAME head; and if the head were fixed the sha would change, so this can't wrongly block).
if["$head_neg"=1];then
decide deny "H10 merge gate: BLOCKED — a review verdict for the current head ($short) is negative (BLOCKED/NOT-MERGEABLE). Resolve the findings and post a fresh 'Review-verdict: MERGEABLE @ $short' before merging PR #$pr."
fi
if["$head_pos"=1];then
# (a) CI green + (b) all Done-when ticked + (c) positive verdict @ current head -> SATISFIED. Auto-grant.
case"$class" in
negative)
# A negative verdict on head wins over a positive one (a later BLOCKED retracts an earlier
# MERGEABLE on the SAME head; if the head were fixed the sha would change, so this can't
# wrongly block).
decide deny "H10 merge gate: BLOCKED — a review verdict for the current head ($short) is negative (BLOCKED/NOT-MERGEABLE). Resolve the findings and post a fresh 'Review-verdict: MERGEABLE @ $short' before merging PR #$pr.";;
stale)
decide deny "H10 merge gate: BLOCKED — a review-verdict comment references an older commit, not the current head ($short). The latest commit(s) are unreviewed (ersatztv#242: re-review the fix commit, not just the initial diff). Re-review the head and post 'Review-verdict: MERGEABLE @ $short'.";;
unknown)
decide ask "H10 merge gate: a 'Review-verdict:' comment on PR #$pr uses an unrecognized verdict token (not MERGEABLE/APPROVED/LGTM/BLOCKED/NOT-MERGEABLE). It is deliberately NOT read as approval. Post a verdict using the documented vocabulary — e.g. 'Review-verdict: MERGEABLE @ $short'.";;
no-sha)
# Marker(s) exist but reference no sha at all -> ask (don't mislabel as a stale older-commit review).
decide ask "H10 merge gate: a 'Review-verdict:' comment on PR #$pr references no commit sha in its own '@ <sha>' field. Post one referencing the current head ($short) — e.g. 'Review-verdict: MERGEABLE @ $short' — or confirm the review covered the latest commit and approve.";;
absent)
decide ask "H10 merge gate: no 'Review-verdict:' comment found on PR #$pr referencing head $short. Post the adversarial/Codex verdict (e.g. 'Review-verdict: MERGEABLE @ $short'), or confirm the review covered the latest commit and approve.";;
positive) : ;;
*)
decide ask "H10 merge gate: unrecognized verdict classification '$class' for PR #$pr. Confirm the review covered the latest commit ($short) before merging.";;
esac
if["$class"="positive"];then
# (a) CI + (b) all Done-when ticked + (c) positive verdict @ current head -> SATISFIED. Auto-grant.
# The reason string must not claim more than was actually checked: on the merge_when_checks_succeed
# path this hook never read the CI status at all (it is delegated to Gitea), so saying "CI green"
# there was a plain falsehood in the one message a human reads to decide whether to trust the gate.
if["$mwcs"="true"];then
decide grant "H6/H10 merge gate: satisfied — all Done-when boxes ticked, and both a positive Review-verdict comment and the 'review-verdict/h10' status cover the current head ($short). CI is gated by Gitea (merge_when_checks_succeed), and because the verdict status is bound to this sha, a commit pushed before Gitea merges will clear it and block the merge (ersatztv#622). Auto-granted."
fi
decide grant "H6/H10 merge gate: satisfied — CI green, all Done-when boxes ticked, and a positive Review-verdict references the current head ($short). Auto-granted (no separate confirmation needed)."
fi
if["$stale"=1];then
decide deny "H10 merge gate: BLOCKED — a review-verdict comment references an older commit, not the current head ($short). The latest commit(s) are unreviewed (ersatztv#242: re-review the fix commit, not just the initial diff). Re-review the head and post 'Review-verdict: MERGEABLE @ $short'."
fi
# Marker(s) exist but reference no sha at all -> ask (don't mislabel as a stale older-commit review).
decide ask "H10 merge gate: a 'Review-verdict:' comment on PR #$pr references no commit sha. Post one referencing the current head ($short) — e.g. 'Review-verdict: MERGEABLE @ $short' — or confirm the review covered the latest commit and approve."
# All derivable and satisfied -> auto-grant (defensive: the head_pos branch above already exits here).
decide grant "H6/H10 merge gate: satisfied — auto-granted."
# Unreachable: the `case` above exits on every class, and `positive` exits in the block above. Kept as
# a fail-safe so a future class added to the classifier without a branch here cannot fall off the end
# of the script (which would exit 0 = silent passthrough, the one outcome a gate must never produce).
decide ask "H10 merge gate: verdict classification for PR #$pr produced no decision. Confirm the review covered the latest commit ($short) before merging."
description:The ersatztv task-completion protocol — the mandatory steps and the `## Closing record` comment template for closing a Gitea issue. Use when finishing a task that closes an issue, or when writing a closing comment. The `/done` command runs this automatically.
---
# Task Completion Protocol
Every task that closes a Gitea issue MUST complete ALL of these before it is considered done.
Use `/done <issue>` to run through this automatically.
Merge consent is a separate, hook-enforced concern — see the `## Done-when` convention in the
root `CLAUDE.md`, which stays always-loaded.
1.**Root cause** (bug fixes / incidents only): Document WHY the problem existed, not just what was changed. If root cause is unknown, say so explicitly and open a follow-up investigation issue. Fixing symptoms without understanding causes creates recurring problems.
2.**Comment on issues** as you work — what you found, what approach you're taking, any deviations from the suggested fix.
3.**Push changes**: `git push` all commits before closing. Use `fixes #N` in commit messages to auto-close where appropriate.
4.**Close comment**: Add a structured `## Closing record` comment on the issue (template below).
5.**Close the issue** via API or `fixes #N` commit. Leave open with a comment only if partially addressed.
6.**Update docs**: If the change affects operational behavior, update the relevant Obsidian docs (`~/homelab-docs/`), MEMORY.md, or CLAUDE.md inline — not as a follow-up.
7.**Reply to reviewer** (if from adversarial review): Summary of done/deferred/questions. This triggers the next review cycle.
## `## Closing record` template
Step 4 — this is both the human-readable summary and the per-issue unit MemPalace mines for
retrieval; see `docs/handoffs/chicorytv-issue-queue.md` → "Knowledge retrieval" for the retrieval
contract this feeds.
```markdown
## Closing record
**Outcome:** <what shipped / what didn't; PR link>
**Root cause:** <for bug fixes/incidents — why the problem existed, or "unknown, see follow-up #N">
**Decisions/conventions changed:** <keys added/superseded in docs/decisions.md, or "none">
**Reusable knowledge:** <a fact/gotcha worth surfacing to a future session or MemPalace search>
**Verification:** <tests run, live-E2E, CI status>
**Deferred:** <anything explicitly punted, with a follow-up issue link, or "none">
**Docs updated:** <which docs/*.md files changed in this PR, or "none required and why">
echo "::error::The pinned CI image tag ersatztv-ci:${pins[0]} does not resolve to a commit in this repo, so it cannot correspond to an image ci-image.yml built from these sources. Rebuild the image and pin the sha it prints."
exit 1
fi
if [ "$pin_full" != "$expected" ]; then
echo "::error::CI toolchain image pin is stale: docker-build.yml pins ersatztv-ci:${pins[0]} ($pin_full), but docker/ci was last changed in $expected. Your jobs are testing an image that is NOT built from this PR's docker/ci. Let ci-image.yml publish the new :<sha>, then update the pin in ALL jobs to it (docs/ci-cd.md -> 'CI toolchain image')."
exit 1
fi
echo "Pin is current: ersatztv-ci:${pins[0]} resolves to $pin_full = docker/ci's last change."
# Non-blocking nudge: if a PR migrates/adds a route but forgets the parity tracker, warn.
# The rule lives in CLAUDE.md → Conventions; this only surfaces an easy-to-miss omission.
# Deliberately no setup-dotnet/setup-node (and thus no actions/cache) so it can't hit the
# cache-save issues seen on the relocated runner (server-management#570).
docs-reminder:
name:Docs update reminder
runs-on:small # seconds-long git diff; keep it off the build runners
if:github.event_name == 'pull_request'
steps:
- name:Checkout
uses:actions/checkout@v4
with:
fetch-depth:0
- name:Warn when a screen/route change skips the parity doc
echo "Changed files in this PR:"; printf '%s\n' "$changed"
screen_or_route=no
if printf '%s\n' "$changed" | grep -Eq '^web/src/screens/.+\.tsx$|^ErsatzTV/LegacyUiRedirects\.cs$'; then
screen_or_route=yes
fi
parity=no
if printf '%s\n' "$changed" | grep -qx 'docs/blazor-route-parity.md'; then
parity=yes
fi
if [ "$screen_or_route" = yes ] && [ "$parity" = no ]; then
echo "::warning::This PR touches a SPA screen or LegacyUiRedirects.cs but does not update docs/blazor-route-parity.md. If you added/migrated/redirected a route, update the parity tracker (and docs/domain-model.md) in THIS PR — see CLAUDE.md → Conventions."
echo "Pins found in docker-build.yml: ${pins[*]} (${#pins[@]} distinct)"
if [ "${#pins[@]}" -eq 0 ]; then
echo "::error::No ersatztv-ci pin found in docker-build.yml at all. Every container: job must pin ersatztv-ci:<7-char-sha>; if the grep pattern stopped matching, fix it here too (docs/ci-cd.md -> 'CI toolchain image')."
exit 1
fi
if [ "${#pins[@]}" -ne 1 ]; then
echo "::error::docker-build.yml pins MORE THAN ONE ersatztv-ci tag (${pins[*]}). All jobs must pin the same image — bump them together."
exit 1
fi
# LENGTH is a separate invariant from CORRECTNESS, and only this check covers it
# (ersatztv#594). The resolve + staleness checks below compare RESOLVED shas, so a
# 8/9/10-char abbreviation of the right commit sails through them green — while
# matching NO tag in the registry, because ci-image.yml tags with
# `git rev-parse --short HEAD` under `fetch-depth: 1`, which always yields exactly 7.
# The failure would otherwise surface far downstream as all five `container:` jobs
# dying at image-pull with `manifest unknown`, which reads like a registry outage.
# This is an easy mistake to make: the natural local command prints 8 chars.
#
# Deliberately a literal 7, not a derived `git rev-parse --short=7`: in this full
# clone git may widen an ambiguous abbreviation past 7, which would demand a pin
# ci-image.yml can never publish — the exact clone-depth asymmetry noted above.
# `${expected:0:7}` is plain string truncation, so it is safe to suggest.
#
# ESCAPE HATCH, if you are ever stuck: this makes 7 mandatory, so if `${expected:0:7}` ever
# became an AMBIGUOUS prefix (two objects sharing it), the resolve check below would fail
# and a longer pin — previously the workaround — is now rejected here first. There is no
# in-repo remedy in that state: relax this length check in the same PR and say why. Note
# that ci-image.yml still tags with a plain `--short` (auto-scaled), so "always 7" is an
# empirical property of today's shallow clone, not an enforced invariant. Making the
# publisher emit `--short=7` is tracked as ersatztv#597. It is not blocked, just out of
# scope here: editing ci-image.yml re-points `expected` (above) at that commit, so it needs
# the branch's own publish-then-pin two-step (docs/ci-cd.md -> 'CI toolchain image') —
# ci-image.yml's push trigger has no branches: filter, so a feature branch does publish.
if [ "${#pins[0]}" -ne 7 ]; then
echo "::error::CI toolchain image pin ersatztv-ci:${pins[0]} is ${#pins[0]} chars, but ci-image.yml publishes 7-char tags (it tags with 'git rev-parse --short HEAD' from a fetch-depth:1 clone). A differently-sized abbreviation still resolves to the right commit, so this would pass every other check here — but NO such tag exists in the registry, and all five container: jobs would fail at image-pull time with 'manifest unknown'. Pin exactly: ersatztv-ci:${expected:0:7} (locally: git rev-parse --short=7 HEAD). See docs/ci-cd.md -> 'CI toolchain image'."
echo "::error::The pinned CI image tag ersatztv-ci:${pins[0]} does not resolve to a commit in this repo, so it cannot correspond to an image ci-image.yml built from these sources. Rebuild the image and pin the sha it prints."
exit 1
fi
if [ "$pin_full" != "$expected" ]; then
echo "::error::CI toolchain image pin is stale: docker-build.yml pins ersatztv-ci:${pins[0]} ($pin_full), but docker/ci was last changed in $expected. Your jobs are testing an image that is NOT built from this PR's docker/ci. Let ci-image.yml publish the new :<sha>, then update the pin in ALL jobs to it (docs/ci-cd.md -> 'CI toolchain image')."
exit 1
fi
echo "Pin is current: ersatztv-ci:${pins[0]} resolves to $pin_full = docker/ci's last change."
# Non-blocking nudge: if a PR migrates/adds a route but forgets the parity tracker, warn.
# The rule lives in CLAUDE.md → Conventions; this only surfaces an easy-to-miss omission.
# Deliberately no setup-dotnet/setup-node (and thus no actions/cache) so it can't hit the
# cache-save issues seen on the relocated runner (server-management#570).
docs-reminder:
name:Docs update reminder
runs-on:small # seconds-long git diff; keep it off the build runners
if:github.event_name == 'pull_request'
steps:
- name:Checkout
uses:actions/checkout@v4
with:
fetch-depth:0
- name:Warn when a screen/route change skips the parity doc
echo "Changed files in this PR:"; printf '%s\n' "$changed"
screen_or_route=no
if printf '%s\n' "$changed" | grep -Eq '^web/src/screens/.+\.tsx$|^ErsatzTV/LegacyUiRedirects\.cs$'; then
screen_or_route=yes
fi
parity=no
if printf '%s\n' "$changed" | grep -qx 'docs/blazor-route-parity.md'; then
parity=yes
fi
if [ "$screen_or_route" = yes ] && [ "$parity" = no ]; then
echo "::warning::This PR touches a SPA screen or LegacyUiRedirects.cs but does not update docs/blazor-route-parity.md. If you added/migrated/redirected a route, update the parity tracker (and docs/domain-model.md) in THIS PR — see CLAUDE.md → Conventions."
# Docs-only: prose and decision records. Deliberately narrower than the hook's pattern,
# which also lets .claude/.gitea/.husky through — that carve-out is safe there only
# because it falls through to a HUMAN PROMPT, whereas here it would post a green status
# with nobody in the loop.
DOCS_ONLY='^(docs/|[^/]*\.md$)'
if [ -z "${GITEA_TOKEN:-}" ]; then
echo "::error::No GITEA_TOKEN available, so the ${CONTEXT} status cannot be written. An exempt (bot/docs-only) PR will stall until this is fixed; a normal PR is unaffected — post its verdict with scripts/post-review-verdict.sh."
gh -X POST -H 'Content-Type: application/json' -d "$payload" \
"$BASE_URL/repos/$REPO/statuses/$SHA" >/dev/null
echo "Posted ${CONTEXT}=${state} on ${SHA:0:7}."
if [ "$state" = "pending" ]; then
echo "::notice::This PR needs an H10 review verdict for head ${SHA:0:7} before it can merge. After reviewing, run: scripts/post-review-verdict.sh ${PR} MERGEABLE"
@@ -4,25 +4,9 @@ Custom IPTV channel server for Jellyfin. Forked from [ErsatzTV/ErsatzTV](https:/
## Architecture
- **Language**: C# / .NET 10
- **UI**: ChicoryTV React SPA (`web/`, Vite, served at `/app`) over the REST API — the ONLY UI. The legacy Blazor Server UI (MudBlazor) was removed in #91 phase (b); root `/` and every legacy route now 302 to `/app`, either via an explicit redirect in `ErsatzTV/LegacyUiRedirects.cs` or the Startup catch-all fallback (any unmatched non-`/api`/`/artwork`/`/docs`/`/openapi` path → `/app`). Historical parity work: media detail pages + image folder browser landed via #141 (PR #183); scheduling parity #144/#162, #141/#158/#161/#180, #145, #151/#152/#153/#155, and the media-source write API/SPA #202 are all DONE.
- **Pattern**: CQRS via MediatR — queries/commands in `ErsatzTV.Application/`
- **Database**: EF Core (SQLite default, MySQL optional) — context in `ErsatzTV.Infrastructure/Data/TvContext.cs`
- **Media**: FFmpeg via CliWrap, SkiaSharp for logo generation
- **Functional C#**: Language Ext (Option, Either monads throughout)
### Project Layout
| Project | Role |
|---------|------|
| `ErsatzTV/` | ASP.NET Core host, API controllers, SPA static hosting, DI setup |
| Migrate / add / redirect a route (new `web/src/screens/*.tsx`, `LegacyUiRedirects.cs`) | `docs/blazor-route-parity.md` + `docs/domain-model.md` |
| Add / change a `/api/*` endpoint | `docs/api-conventions.md` checklist, then regenerate `v1.json` + `endpoint-index.md` via `./scripts/update-openapi.sh` |
| Change a SPA screen convention | `docs/spa-conventions.md` |
| Establish / reverse a convention or decision | `docs/decisions.md` (lifecycle: add record, relocate predecessor to archive/) + the affected doc |
| Establish / reverse a convention or decision | a new`docs/decisions/records/<area>/<topic>.md` (filename = key; lifecycle: add record, `git mv` predecessor to `archive/<area>/`) + regenerate the catalog + the affected doc |
| Add / remove / retitle a doc | `docs/README.md` index |
The `docs-reminder` CI job flags a screen/route change that skips `blazor-route-parity.md`, but it's a **non-blocking** nudge — the rule is on you, not the check.
- Test with **NUnit** + Shouldly + NSubstitute (the existing `*.Tests` projects); xUnit is **not** used here
- **Dependencies use Central Package Management**: versions live in the repo-root `Directory.Packages.props`; csproj reference packages by name only. Add/upgrade by editing the central `<PackageVersion>` — never put `Version=` back on a `<PackageReference>` (trips `NU1008`). See `docs/ci-cd.md` → Dependency management.
- **DB migrations target BOTH providers**: a `TvContext` model change needs a migration in `ErsatzTV.Infrastructure.Sqlite`**and**`ErsatzTV.Infrastructure.MySql` — run `scripts/add-migration.sh <Name>` (does both). CI's `migrations` job enforces model-drift + apply-to-fresh-DB per provider. See `docs/ci-cd.md` → Migration integrity.
- **Renovate** is live (`.gitea/workflows/renovate.yml`, weekly + `workflow_dispatch`): opens dependency-update + OSV vuln-fix PRs and a Dependency Dashboard issue; patch bumps to test/dev-only packages auto-merge once `Build & test` passes, the rest are manual. Cross-repo rollout: server-management#484. See `docs/ci-cd.md` → Dependency management.
- **Renovate** is live (`.gitea/workflows/renovate.yml`, weekly + `workflow_dispatch`): opens dependency-update + OSV vuln-fix PRs and a Dependency Dashboard issue; patch bumps to test/dev-only packages auto-merge once `Build & test` passes (their `review-verdict/h10` required check is auto-passed as a bot PR — unless they touch `.claude/`/`.gitea/`/`.husky/`/`scripts/`/`docker/ci/`, which need a real verdict), the rest are manual. Cross-repo rollout: server-management#484. See `docs/ci-cd.md` → Dependency management.
- **Versioning**: release tags are `vYY.<release-seq>.<patch>` (year · sequential release-within-year · patch) — inherited from upstream, **not** year.month. `v26.3.1` = our infra rebuild of upstream 26.3.0 (no app changes); `v26.4.0` is reserved for the first release with app changes. Never `[skip ci]` a commit you'll tag (it suppresses the release build). Full policy: `docs/ci-cd.md` → Versioning & releases.
- Backlog tracked via [Gitea Issues](http://192.168.1.95:3000/timothy/ersatztv/issues)
Every task that closes a Gitea issue MUST complete ALL of these before it is considered done. Use `/done <issue>` to run through this automatically.
**Merge-consent is derived from state, not asserted (`## Done-when` convention — ersatztv#303 H6 + H10).** Any issue whose PR will merge to `main` should carry a `## Done-when` section in its **issue body** — a checklist of completion criteria (always include an "adversarial review passed" box; add per-issue criteria like tests-green, docs-updated, live-E2E). Two hooks derive merge-consent from it so a premature merge is blocked *by construction*, not by memory:
-`pretooluse-merge-consent.sh` (Claude PreToolUse on the Gitea merge tool) — **auto-grants** a merge (emits `permissionDecision: allow`, so **no** redundant mechanical prompt fires) only when the PR's CI is green **and** every `## Done-when` box on the linked issue (`fixes #N`) is ticked **and** a `Review-verdict:` comment references the PR's *current head sha* (**H10**); **denies** on an unticked box, red CI, or a stale/negative review verdict; **asks** (falls back to a human prompt) when it can't derive state (no linked issue, no `## Done-when` section, no `Review-verdict:` comment yet, no creds, Gitea down). On the auto-grant (satisfied) path the derived state **is** the consent — do not also ask conversationally to merge; a separate human confirmation is warranted only when the gate **asks** (ersatztv#314). **The H10 review-verdict convention**: after an adversarial/Codex review of a PR (or its latest fix commit), post a PR comment with a line `Review-verdict: <MERGEABLE|APPROVED|BLOCKED> @ <head-sha>` — this proves the *latest* commit was reviewed, not a stale earlier diff (ersatztv#242).
-`pretooluse-merge-consent.sh` (Claude PreToolUse on the Gitea merge tool) — **auto-grants** a merge (emits `permissionDecision: allow`, so **no** redundant mechanical prompt fires) only when the PR's CI is green **and** every `## Done-when` box on the linked issue (`fixes #N`) is ticked **and** a `Review-verdict:` comment references the PR's *current head sha* (**H10**); **denies** on an unticked box, red CI, or a stale/negative review verdict; **asks** (falls back to a human prompt) when it can't derive state (no linked issue, no `## Done-when` section, no `Review-verdict:` comment yet, no creds, Gitea down). On the auto-grant (satisfied) path the derived state **is** the consent — do not also ask conversationally to merge; a separate human confirmation is warranted only when the gate **asks** (ersatztv#314). **The H10 review-verdict convention**: after an adversarial/Codex review of a PR (or its latest fix commit), run **`scripts/post-review-verdict.sh <pr> <MERGEABLE|APPROVED|BLOCKED|NOT-MERGEABLE> [note]`** — it posts both the `Review-verdict: … @ <head-sha>` comment and the sha-bound `review-verdict/h10` commit status, proving the *latest* commit was reviewed rather than a stale earlier diff (ersatztv#242). Do not hand-write the comment: the **status** is the required check branch protection enforces, and a comment alone leaves it absent.
- **The gate is enforced server-side, per sha (ersatztv#622).** `review-verdict/h10` is a required status check on `main`. Because a commit status belongs to one sha, a commit pushed *after* an auto-merge is scheduled clears it and blocks the merge — closing the hole where `merge_when_checks_succeed` froze consent at scheduling time and Gitea later merged an unreviewed head. Renovate-authored and docs-only PRs are auto-passed by `.gitea/workflows/review-verdict.yml`, **except** when they touch `.claude/`, `.gitea/`, `.husky/`, `scripts/` or `docker/ci/`. See `docs/ci-cd.md` → Review-verdict gate.
-`.husky/pre-push` → `prepush-donewhen.sh` — a fail-open backstop that blocks a direct `git push origin main` whose commits `fix #N` an issue with unticked boxes.
Both need Gitea read creds in the env to enforce (**`ETV_GITEA_BASICAUTH=user:pass`** or `ETV_GITEA_TOKEN`; `ETV_GITEA_URL` overrides the base). Without them the merge hook asks and the push backstop is a no-op — the gate degrades to today's manual confirmation, never a silent pass. Docs-only PRs/pushes are exempt.
1.**Root cause** (bug fixes / incidents only): Document WHY the problem existed, not just what was changed. If root cause is unknown, say so explicitly and open a follow-up investigation issue. Fixing symptoms without understanding causes creates recurring problems.
2.**Comment on issues** as you work — what you found, what approach you're taking, any deviations from the suggested fix.
3.**Push changes**: `git push` all commits before closing. Use `fixes #N` in commit messages to auto-close where appropriate.
4.**Close comment**: Add a structured `## Closing record` comment on the issue (template below).
5.**Close the issue** via API or `fixes #N` commit. Leave open with a comment only if partially addressed.
6.**Update docs**: If the change affects operational behavior, update the relevant Obsidian docs (`~/homelab-docs/`), MEMORY.md, or CLAUDE.md inline — not as a follow-up.
7.**Reply to reviewer** (if from adversarial review): Summary of done/deferred/questions. This triggers the next review cycle.
**`## Closing record` template** (step 4 — this is both the human-readable summary and the per-issue unit MemPalace mines for retrieval; see `docs/handoffs/chicorytv-issue-queue.md` → "Knowledge retrieval" for the retrieval contract this feeds):
```markdown
## Closing record
**Outcome:** <what shipped / what didn't; PR link>
**Root cause:** <for bug fixes/incidents — why the problem existed, or "unknown, see follow-up #N">
**Decisions/conventions changed:** <keys added/superseded in docs/decisions.md, or "none">
**Reusable knowledge:** <a fact/gotcha worth surfacing to a future session or MemPalace search>
**Verification:** <tests run, live-E2E, CI status>
**Deferred:** <anything explicitly punted, with a follow-up issue link, or "none">
**Docs updated:** <which docs/*.md files changed in this PR, or "none required and why">
```
**The 7 mandatory completion steps and the `## Closing record` comment template** live in the
`closing-an-issue` skill (`.claude/skills/closing-an-issue/SKILL.md`) — invoke it (or `/done`)
Some files were not shown because too many files have changed in this diff
Show More
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
timothyandClaude Opus 5