3406 Commits

This Branch

This Branch

All Branches

Author	SHA1	Message	Date
timothy	83cd36e0de	test(491): run the dedupe fixture against MySql in CI; correct the collation claim ... 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 #308 fix #491	2026-07-25 21:13:31 +02:00
timothy	48d41b9235	fix(491): make the MySql dedupe byte-exact, not just case-exact (PAD SPACE) ... 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 #308 fix #491	2026-07-25 21:13:31 +02:00
timothy	50eff83628	docs(491): reattach the case-exact lookup rationale to GetFolder ... 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.	2026-07-25 21:13:31 +02:00
timothy	ecb763ea58	fix(491): review polish — heal cannot abort a scan, ordinal settle unit-tested ... 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 #308 fix #491	2026-07-25 21:13:30 +02:00
timothy	14e9b03433	fix(491): wire the unique-violation classifier in the scanner; make folder lookup case-exact ... 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 #308 fix #491	2026-07-25 21:13:30 +02:00
timothy	9a4f3e832d	fix(491): unique index on LibraryFolder(LibraryPathId, PathHash) + tolerate concurrent insert ... 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 #308 fix #491	2026-07-25 21:13:22 +02:00
timothy	8e84191a03	Merge pull request 'feat(610): split the decision corpus into one YAML-frontmatter file per record' (#619) from feat/610-decisions-one-file-per-record into main	2026-07-25 18:36:08 +00:00
timothy	02c82b35ea	fix(610): make the frontmatter READ path dependency-free — CI has no PyYAML ... 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	2026-07-25 19:57:22 +02:00
timothy	52786a545c	feat(610): re-key the body-diff guard to key; enforce path<->key; pin the filesystem invariant ... 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	2026-07-25 19:53:08 +02:00
timothy	fba5233caf	feat(610): split the decision corpus into one YAML-frontmatter file per record ... 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	2026-07-25 19:45:09 +02:00
timothy	8578dc1ca7	fix(610): de-brittle the count assertion; stop conflating index lines with preserved prose ... 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.	2026-07-25 19:08:58 +02:00
timothy	64b65fd2db	feat(610): generated where-did-it-go index on each stripped file ... 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.	2026-07-25 19:08:58 +02:00
timothy	8ec1f527e4	fix(610): catalog path links only for the split layout, not every file under docs/decisions ... 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.	2026-07-25 19:08:58 +02:00
timothy	1af65b7bee	feat(610): budget counts PROSE, excluding YAML frontmatter ... 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.	2026-07-25 19:08:58 +02:00
timothy	ef67be924c	feat(610): teach the validator and catalog the split layout ... 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.	2026-07-25 19:08:58 +02:00
timothy	d52268f2ed	feat(610): migration script + field-level equivalence harness ... 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.	2026-07-25 19:08:58 +02:00
timothy	e063fd065f	feat(610): dual-format decision-record parser (backtick line + YAML frontmatter) ... 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.	2026-07-25 19:08:58 +02:00
timothy	798ecd1f53	Merge pull request 'fix(609): the decisions rationale-edit marker is a git trailer, not a bare substring' (#618) from fix/609-decisions-edit-token into main	2026-07-25 17:07:49 +00:00
timothyandClaude Opus 5	f49556b6ef	docs(609): qualify the two ci-cd.md mentions as non-merge ... 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>	2026-07-25 18:40:55 +02:00
timothyandClaude Opus 5	e9f4444fec	docs(609): qualify two comments as non-merge, matching the implementation ... 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>	2026-07-25 18:38:58 +02:00
timothyandClaude Opus 5	f00dfde0c5	docs(609): correct the --no-merges rationale — merges are discouraged, not blocked ... 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>	2026-07-25 18:36:07 +02:00
timothyandClaude Opus 5	cc9481f541	fix(609): review fixes — exclude merge commits, unfold folded values, correct the fail-posture doc ... 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>	2026-07-25 18:29:46 +02:00
timothyandClaude Opus 5	c597c49f02	fix(609): arm the decisions body-diff with a git trailer, not a bare substring ... 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>	2026-07-25 18:21:12 +02:00
timothy	54ed75624a	Merge pull request 'fix(496): per-library server identity for music videos — itemId diff + soft trash' (#607) from fix/496-musicvideo-identity into main	2026-07-25 16:08:12 +00:00
timothy	b5b6e7f636	test(496,484): record projection failures during enumeration, not eagerly [decisions-edit] ... 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.	2026-07-25 17:36:34 +02:00
timothy	4bdad4bf52	fix(496,484): thread #484's projection-failure guard through the new music-video scanner [decisions-edit] ... 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.	2026-07-25 17:27:50 +02:00
timothy	147166b053	fix(496): harden the legacy path sweep against MySQL collation (re-review Low) ... 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.	2026-07-25 17:20:53 +02:00
timothy	6bd1d954bd	fix(496): review fixes — thread the replaced local path, scope identity per library, sweep legacy rows [decisions-edit] ... 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	2026-07-25 17:20:53 +02:00
timothy	64decd492e	fix(496): give music videos a per-library server identity; itemId diff + soft trash ... 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	2026-07-25 17:20:53 +02:00
timothy	eb4de0cc2b	Merge pull request 'feat(484): suppress the media-server sweep on projection failures; reject the ratio threshold' (#612) from feat/484-scanner-guard-threshold into main	2026-07-25 15:08:07 +00:00
timothy	7cc12881de	docs(484): correct the counted-enumeration counts on the api client interfaces ... 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.	2026-07-25 16:36:35 +02:00
timothy	6f4497e1ce	fix(484): guard the nested TV season and episode sweeps against projection failures ... 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	2026-07-25 16:36:35 +02:00
timothy	e3645a2840	feat(484): suppress the media-server sweep on projection failures; reject the ratio threshold ... 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	2026-07-25 16:36:35 +02:00
timothy	fd70e6eb4f	Merge pull request 'fix(503): map WatermarkLocation.MiddleCenter to a centered overlay position' (#611) from fix/503-watermark-middlecenter into main	2026-07-25 14:35:27 +00:00
timothy	c3beee1629	Merge pull request 'feat(603): adopt OKF's optional stale-after and Sources decision-record metadata' (#605) from worktree-603-decisions-stale-after-sources into main	2026-07-25 14:15:59 +00:00
timothy	f04412a95b	docs(603): correct the record's own no-backfill claim, which the backfill commit falsifies ... 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.	2026-07-25 15:30:43 +02:00
timothy	8e1e15e4d7	docs(603): backfill stale-after + Sources onto the three ci.* outside-world records ... 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.	2026-07-25 15:22:50 +02:00
timothy	75139bbf31	fix(603): close four defects found in adversarial review of the stale-after fields ... 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).	2026-07-25 15:22:50 +02:00
timothy	0ad02db651	feat(603): adopt OKF's optional stale-after and Sources decision-record metadata ... 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	2026-07-25 15:22:50 +02:00
timothy	39873811da	test(503): guard WatermarkLocation exhaustiveness and cover source-content margins ... 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.	2026-07-25 15:19:23 +02:00
timothyandClaude Sonnet 5	97bc346539	fix(503): map WatermarkLocation.MiddleCenter to a centered overlay position ... 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>	2026-07-25 15:19:23 +02:00
timothy	abeef63f05	Merge pull request 'feat(445,533): headless Playwright UI-E2E flows + fix e2e-local readiness probe' (#591) from feat/445-533-ui-e2e into main	2026-07-25 12:52:32 +00:00
timothy	9798877631	ci(445): re-point the toolchain pin after the second rebase (1652fc5 -> 32747a0) ... 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	2026-07-25 14:35:02 +02:00
timothy	32747a067e	docs(445): note the ci-image-pin length guard in the rebase-trap warning ... 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	2026-07-25 14:29:27 +02:00
timothy	352aa70634	refactor(445): drop e2e-ui.sh's port pre-flight, now owned by e2e-local.sh (#586/#598) ... 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	2026-07-25 14:29:27 +02:00
timothy	e87285c33d	Merge pull request 'fix(460): write null LastScan on disable-sync instead of the MinValue sentinel' (#601) from fix/460-lastscan-null into main	2026-07-25 12:11:34 +00:00
timothy	e7bac06d1b	ci(445): re-point the toolchain pin after the rebase (e9fd26f -> 1652fc5) ... 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	2026-07-25 14:04:52 +02:00
timothy	5dd0b147e8	docs(445): record that rebasing invalidates the CI toolchain image pin ... 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	2026-07-25 14:04:52 +02:00
timothy	dbe72dbf03	docs(445): refresh web test count after rebase (983 -> 995) ... 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.	2026-07-25 14:04:52 +02:00
timothy	54919e4730	fix(445): pidfile-first cleanup ordering + clear BOOT_PID after wait ... 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	2026-07-25 14:04:52 +02:00