Compare commits
12
Commits
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
e5a01dc841 | ||
|
|
3e2c07b42f | ||
|
|
7966e08164 | ||
|
|
ad31a02850 | ||
|
|
b5dee26202 | ||
|
|
78fc283627 | ||
|
|
8c72a5de9e | ||
|
|
d304c8573e | ||
|
|
30640bb780 | ||
|
|
0cf355e494 | ||
|
|
5ba442c11c | ||
|
|
29d7a06e65 |
@@ -114,6 +114,27 @@ env:
|
||||
MSBUILDDISABLENODEREUSE: "1" # MSBuild worker nodes exit with the build instead of lingering
|
||||
|
||||
jobs:
|
||||
# Answers "is the toolchain image still there?" in ONE place, so a deleted pin does not read as
|
||||
# five broken jobs and a broken diff (ersatztv#772). Deliberately container-free and deliberately
|
||||
# NOT a `needs:` of the jobs it diagnoses — see scripts/ci-toolchain-image-resolves.sh for both
|
||||
# decisions and for the cleanup-rule root cause it cannot fix from this repo.
|
||||
toolchain-preflight:
|
||||
name: CI toolchain image resolves
|
||||
runs-on: small
|
||||
steps:
|
||||
- name: Checkout
|
||||
uses: actions/checkout@v4
|
||||
- name: Resolve the pinned toolchain tag in the registry
|
||||
env:
|
||||
ETV_REGISTRY_AUTH: ${{ secrets.REGISTRY_USER }}:${{ secrets.REGISTRY_PASSWORD }}
|
||||
run: |
|
||||
"${GITHUB_WORKSPACE:-.}/scripts/ci-step-ran.sh" mark resolve
|
||||
scripts/ci-toolchain-image-resolves.sh
|
||||
- name: Assert every expected step executed (ersatztv#756)
|
||||
run: >-
|
||||
scripts/ci-step-ran.sh assert
|
||||
--always resolve
|
||||
|
||||
test:
|
||||
name: Build & test (.NET)
|
||||
runs-on: ubuntu-latest
|
||||
|
||||
@@ -25,9 +25,9 @@
|
||||
<PackageVersion Include="LanguageExt.Core" Version="4.4.9" />
|
||||
<PackageVersion Include="LanguageExt.Transformers" Version="4.4.8" />
|
||||
<PackageVersion Include="Lennox.NvEncSharp" Version="2.0.0" />
|
||||
<PackageVersion Include="Lucene.Net" Version="4.8.0-beta00017" />
|
||||
<PackageVersion Include="Lucene.Net.Analysis.Common" Version="4.8.0-beta00017" />
|
||||
<PackageVersion Include="Lucene.Net.QueryParser" Version="4.8.0-beta00017" />
|
||||
<PackageVersion Include="Lucene.Net" Version="4.8.0-beta00018" />
|
||||
<PackageVersion Include="Lucene.Net.Analysis.Common" Version="4.8.0-beta00018" />
|
||||
<PackageVersion Include="Lucene.Net.QueryParser" Version="4.8.0-beta00018" />
|
||||
<PackageVersion Include="MediatR" Version="[12.5.0]" />
|
||||
<PackageVersion Include="Meziantou.Analyzer" Version="3.0.129" />
|
||||
<PackageVersion Include="Microsoft.AspNetCore.Authentication.JwtBearer" Version="10.0.2" />
|
||||
|
||||
@@ -595,6 +595,13 @@ public class RefreshChannelDataHandler : IRequestHandler<RefreshChannelData>
|
||||
metadata.Genres ??= [];
|
||||
metadata.Studios ??= [];
|
||||
|
||||
// Artists/AlbumArtists are NULLABLE primitive collections, so they are guarded at the read site
|
||||
// rather than assigned back onto `metadata` like the navigations above (ersatztv#701/#691): they
|
||||
// are scalar JSON-array columns, so `??= []` on a tracked entity would persist `[]` over NULL.
|
||||
// The shipped `_song.sbntxt` only does `array.join`, but a user template is free to do anything.
|
||||
List<string> songArtists = Optional(metadata.Artists).Flatten().ToList();
|
||||
List<string> songAlbumArtists = Optional(metadata.AlbumArtists).Flatten().ToList();
|
||||
|
||||
string artworkPath = GetPrioritizedArtworkPath(metadata);
|
||||
|
||||
var data = new
|
||||
@@ -607,8 +614,8 @@ public class RefreshChannelDataHandler : IRequestHandler<RefreshChannelData>
|
||||
HasCustomTitle = hasCustomTitle,
|
||||
displayItem.CustomTitle,
|
||||
SongTitle = subtitle,
|
||||
SongArtists = metadata.Artists,
|
||||
SongAlbumArtists = metadata.AlbumArtists,
|
||||
SongArtists = songArtists,
|
||||
SongAlbumArtists = songAlbumArtists,
|
||||
SongHasYear = metadata.Year.HasValue,
|
||||
SongYear = metadata.Year,
|
||||
SongGenres = metadata.Genres.Map(g => g.Name).OrderBy(n => n),
|
||||
|
||||
@@ -440,64 +440,64 @@ public class ElasticSearchIndex : ISearchIndex
|
||||
Season season)
|
||||
{
|
||||
foreach (SeasonMetadata metadata in season.SeasonMetadata.HeadOrNone())
|
||||
foreach (ShowMetadata showMetadata in season.Show.ShowMetadata.HeadOrNone())
|
||||
{
|
||||
try
|
||||
foreach (ShowMetadata showMetadata in season.Show.ShowMetadata.HeadOrNone())
|
||||
{
|
||||
var seasonTitle = $"{showMetadata.Title} - S{season.SeasonNumber}";
|
||||
string sortTitle = $"{showMetadata.SortTitle}_{season.SeasonNumber:0000}"
|
||||
.ToLowerInvariant();
|
||||
string titleAndYear = $"{showMetadata.Title}_{showMetadata.Year}_{season.SeasonNumber}"
|
||||
.ToLowerInvariant();
|
||||
|
||||
var doc = new ElasticSearchItem
|
||||
try
|
||||
{
|
||||
Id = season.Id,
|
||||
Type = LuceneSearchIndex.SeasonType,
|
||||
Title = seasonTitle,
|
||||
SortTitle = sortTitle,
|
||||
LibraryName = season.LibraryPath.Library.Name,
|
||||
LibraryId = season.LibraryPath.Library.Id,
|
||||
TitleAndYear = titleAndYear,
|
||||
TitleAndYearSearch = LuceneSearchIndex.GetTitleAndYearSearch(metadata),
|
||||
JumpLetter = LuceneSearchIndex.GetJumpLetter(showMetadata),
|
||||
State = season.State.ToString(),
|
||||
SeasonNumber = season.SeasonNumber,
|
||||
ShowTitle = showMetadata.Title,
|
||||
ShowGenre = showMetadata.Genres.Map(g => g.Name).ToList(),
|
||||
ShowTag = showMetadata.Tags.Map(t => t.Name).ToList(),
|
||||
ShowStudio = showMetadata.Studios.Map(s => s.Name).ToList(),
|
||||
ShowContentRating = GetContentRatings(showMetadata.ContentRating),
|
||||
Language = GetLanguages(
|
||||
languageCodeService,
|
||||
await searchRepository.GetLanguagesForSeason(season)),
|
||||
LanguageTag = await searchRepository.GetLanguagesForSeason(season),
|
||||
SubLanguage = GetLanguages(
|
||||
languageCodeService,
|
||||
await searchRepository.GetSubLanguagesForSeason(season)),
|
||||
SubLanguageTag = await searchRepository.GetSubLanguagesForSeason(season),
|
||||
ContentRating = GetContentRatings(showMetadata.ContentRating),
|
||||
ReleaseDate = GetReleaseDate(metadata.ReleaseDate),
|
||||
AddedDate = GetAddedDate(metadata.DateAdded),
|
||||
TraktList = season.TraktListItems
|
||||
.Map(t => t.TraktList.TraktId.ToString(CultureInfo.InvariantCulture)).ToList(),
|
||||
Tag = metadata.Tags.Map(a => a.Name).ToList(),
|
||||
TagFull = metadata.Tags.Map(t => t.Name).ToList()
|
||||
};
|
||||
var seasonTitle = $"{showMetadata.Title} - S{season.SeasonNumber}";
|
||||
string sortTitle = $"{showMetadata.SortTitle}_{season.SeasonNumber:0000}"
|
||||
.ToLowerInvariant();
|
||||
string titleAndYear = $"{showMetadata.Title}_{showMetadata.Year}_{season.SeasonNumber}"
|
||||
.ToLowerInvariant();
|
||||
|
||||
foreach ((string key, List<string> value) in GetMetadataGuids(metadata))
|
||||
{
|
||||
doc.AdditionalProperties.Add(key, value);
|
||||
var doc = new ElasticSearchItem
|
||||
{
|
||||
Id = season.Id,
|
||||
Type = LuceneSearchIndex.SeasonType,
|
||||
Title = seasonTitle,
|
||||
SortTitle = sortTitle,
|
||||
LibraryName = season.LibraryPath.Library.Name,
|
||||
LibraryId = season.LibraryPath.Library.Id,
|
||||
TitleAndYear = titleAndYear,
|
||||
TitleAndYearSearch = LuceneSearchIndex.GetTitleAndYearSearch(metadata),
|
||||
JumpLetter = LuceneSearchIndex.GetJumpLetter(showMetadata),
|
||||
State = season.State.ToString(),
|
||||
SeasonNumber = season.SeasonNumber,
|
||||
ShowTitle = showMetadata.Title,
|
||||
ShowGenre = showMetadata.Genres.Map(g => g.Name).ToList(),
|
||||
ShowTag = showMetadata.Tags.Map(t => t.Name).ToList(),
|
||||
ShowStudio = showMetadata.Studios.Map(s => s.Name).ToList(),
|
||||
ShowContentRating = GetContentRatings(showMetadata.ContentRating),
|
||||
Language = GetLanguages(
|
||||
languageCodeService,
|
||||
await searchRepository.GetLanguagesForSeason(season)),
|
||||
LanguageTag = await searchRepository.GetLanguagesForSeason(season),
|
||||
SubLanguage = GetLanguages(
|
||||
languageCodeService,
|
||||
await searchRepository.GetSubLanguagesForSeason(season)),
|
||||
SubLanguageTag = await searchRepository.GetSubLanguagesForSeason(season),
|
||||
ContentRating = GetContentRatings(showMetadata.ContentRating),
|
||||
ReleaseDate = GetReleaseDate(metadata.ReleaseDate),
|
||||
AddedDate = GetAddedDate(metadata.DateAdded),
|
||||
TraktList = season.TraktListItems
|
||||
.Map(t => t.TraktList.TraktId.ToString(CultureInfo.InvariantCulture)).ToList(),
|
||||
Tag = metadata.Tags.Map(a => a.Name).ToList(),
|
||||
TagFull = metadata.Tags.Map(t => t.Name).ToList()
|
||||
};
|
||||
|
||||
foreach ((string key, List<string> value) in GetMetadataGuids(metadata))
|
||||
{
|
||||
doc.AdditionalProperties.Add(key, value);
|
||||
}
|
||||
|
||||
await _client.IndexAsync(doc, IndexName, ES.Id.From(doc));
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
metadata.Season = null;
|
||||
_logger.LogWarning(ex, "Error indexing season with metadata {@Metadata}", metadata);
|
||||
}
|
||||
|
||||
await _client.IndexAsync(doc, IndexName, ES.Id.From(doc));
|
||||
}
|
||||
catch (Exception ex)
|
||||
{
|
||||
metadata.Season = null;
|
||||
_logger.LogWarning(ex, "Error indexing season with metadata {@Metadata}", metadata);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private async Task UpdateArtist(
|
||||
@@ -763,8 +763,10 @@ public class ElasticSearchIndex : ISearchIndex
|
||||
{
|
||||
try
|
||||
{
|
||||
metadata.AlbumArtists ??= [];
|
||||
metadata.Artists ??= [];
|
||||
// Guard the two NULLABLE primitive collections at the READ SITE, never by assigning back onto
|
||||
// `metadata` (ersatztv#701) -- see the matching comment in LuceneSearchIndex.UpdateSong.
|
||||
List<string> artists = Optional(metadata.Artists).Flatten().ToList();
|
||||
List<string> albumArtists = Optional(metadata.AlbumArtists).Flatten().ToList();
|
||||
|
||||
var doc = new ElasticSearchItem
|
||||
{
|
||||
@@ -785,8 +787,8 @@ public class ElasticSearchIndex : ISearchIndex
|
||||
SubLanguageTag = GetSubLanguageTags(song.MediaVersions),
|
||||
AddedDate = GetAddedDate(metadata.DateAdded),
|
||||
Album = metadata.Album ?? string.Empty,
|
||||
Artist = metadata.Artists.ToList(),
|
||||
AlbumArtist = metadata.AlbumArtists.ToList(),
|
||||
Artist = artists,
|
||||
AlbumArtist = albumArtists,
|
||||
Genre = metadata.Genres.Map(g => g.Name).ToList(),
|
||||
Tag = metadata.Tags.Map(t => t.Name).ToList(),
|
||||
TagFull = metadata.Tags.Map(t => t.Name).ToList()
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
using System.Globalization;
|
||||
using System.Globalization;
|
||||
using ErsatzTV.Core;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Interfaces.Metadata;
|
||||
@@ -145,7 +145,7 @@ public sealed class LuceneSearchIndex : ISearchIndex
|
||||
_directory = FSDirectory.Open(FileSystemLayout.SearchIndexFolder);
|
||||
Analyzer analyzer = SearchQueryParser.AnalyzerWrapper();
|
||||
var indexConfig = new IndexWriterConfig(AppLuceneVersion, analyzer)
|
||||
{ OpenMode = OpenMode.CREATE_OR_APPEND };
|
||||
{ OpenMode = OpenMode.CREATE_OR_APPEND };
|
||||
_writer = new IndexWriter(_directory, indexConfig);
|
||||
_initialized = true;
|
||||
}
|
||||
@@ -328,7 +328,7 @@ public sealed class LuceneSearchIndex : ISearchIndex
|
||||
using (Analyzer analyzer = SearchQueryParser.AnalyzerWrapper())
|
||||
{
|
||||
var indexConfig = new IndexWriterConfig(AppLuceneVersion, analyzer)
|
||||
{ OpenMode = OpenMode.CREATE_OR_APPEND };
|
||||
{ OpenMode = OpenMode.CREATE_OR_APPEND };
|
||||
using (var w = new IndexWriter(d, indexConfig))
|
||||
{
|
||||
using (DirectoryReader _ = w.GetReader(true))
|
||||
@@ -1318,8 +1318,13 @@ public sealed class LuceneSearchIndex : ISearchIndex
|
||||
{
|
||||
try
|
||||
{
|
||||
metadata.AlbumArtists ??= [];
|
||||
metadata.Artists ??= [];
|
||||
// Guard the two NULLABLE primitive collections at the READ SITE, never by assigning back onto
|
||||
// `metadata` (ersatztv#701). The entity reaching here may be TRACKED, and Artists/AlbumArtists
|
||||
// are scalar JSON-array columns rather than navigations -- so `??= []` flips the entity to
|
||||
// Modified and the next SaveChanges writes `[]` over a NULL column. Same convention as
|
||||
// SongVideoGenerator and MediaCollectionRepository (ersatztv#691).
|
||||
List<string> artists = Optional(metadata.Artists).Flatten().ToList();
|
||||
List<string> albumArtists = Optional(metadata.AlbumArtists).Flatten().ToList();
|
||||
|
||||
var doc = new Document
|
||||
{
|
||||
@@ -1355,12 +1360,12 @@ public sealed class LuceneSearchIndex : ISearchIndex
|
||||
doc.Add(new TextField(AlbumField, metadata.Album, Field.Store.NO));
|
||||
}
|
||||
|
||||
foreach (string artist in metadata.Artists)
|
||||
foreach (string artist in artists)
|
||||
{
|
||||
doc.Add(new TextField(ArtistField, artist, Field.Store.NO));
|
||||
}
|
||||
|
||||
foreach (string albumArtist in metadata.AlbumArtists)
|
||||
foreach (string albumArtist in albumArtists)
|
||||
{
|
||||
doc.Add(new TextField(AlbumArtistField, albumArtist, Field.Store.NO));
|
||||
}
|
||||
|
||||
@@ -0,0 +1,230 @@
|
||||
using System.Reflection;
|
||||
using ErsatzTV.Core.Domain;
|
||||
using ErsatzTV.Core.Interfaces.Metadata;
|
||||
using ErsatzTV.Core.Interfaces.Repositories;
|
||||
using ErsatzTV.Core.Search;
|
||||
using ErsatzTV.Infrastructure.Data;
|
||||
using ErsatzTV.Infrastructure.Extensions;
|
||||
using ErsatzTV.Infrastructure.Search;
|
||||
using ErsatzTV.Tests.Support;
|
||||
using Lucene.Net.Analysis.Standard;
|
||||
using Lucene.Net.Index;
|
||||
using Lucene.Net.Store;
|
||||
using Lucene.Net.Util;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Microsoft.Extensions.Logging;
|
||||
using NSubstitute;
|
||||
using NUnit.Framework;
|
||||
using Shouldly;
|
||||
|
||||
namespace ErsatzTV.Tests.Integration;
|
||||
|
||||
/// <summary>
|
||||
/// ersatztv#701, EXECUTED against a real <see cref="TvContext" /> on SQLite.
|
||||
/// <para>
|
||||
/// <b>The defect.</b> <c>LuceneSearchIndex.UpdateSong</c> opened with
|
||||
/// <c>metadata.AlbumArtists ??= []; metadata.Artists ??= [];</c>. Unlike the navigation
|
||||
/// collections guarded the same way all around them, these two are SCALAR COLUMNS: the whole
|
||||
/// list lives in one column, so the property IS the column value. They are two of EIGHT such
|
||||
/// columns in the model — the population is derived from the MODEL CONFIGURATION (EF-native
|
||||
/// primitive collections plus the six <c>HasConversion<*CollectionValueConverter></c>
|
||||
/// columns on <c>ProgramScheduleAlternate</c>/<c>PlayoutTemplate</c>), NOT by grepping the
|
||||
/// domain classes for <c>IList<string></c>, which finds only two of the eight. See
|
||||
/// the decision record <c>media.nullable-primitive-collection-mutation</c>.
|
||||
/// Assigning one on a TRACKED entity flips it to <see cref="EntityState.Modified" />, and the next
|
||||
/// <c>SaveChanges</c> writes <c>[]</c> over what the database held as <c>NULL</c> — the exact
|
||||
/// mechanism an adversarial review demonstrated in ersatztv#691, which is why that issue's
|
||||
/// entity-level guard was reverted in favour of guarding at the READ SITE.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// <b>Why the fixture loads the song TRACKED even though production does not.</b> Both feeds into
|
||||
/// the indexer are <c>AsNoTracking()</c> today — <c>SearchRepository.GetItemToIndex</c> and
|
||||
/// <c>SearchRepository.GetAllSongs</c> — so no shipped caller loses data. That is a property of
|
||||
/// today's two callers, not of the indexer, and it is exactly what ersatztv#691 recorded as "a
|
||||
/// loaded gun". This fixture therefore pins the INDEXER's own contract: handed a tracked entity it
|
||||
/// must not mutate it. Run against the real pre-fix file, the FIRST of the numbered assertions
|
||||
/// below fails (<c>metadata.Artists should be null but was []</c>) and the run stops there;
|
||||
/// reaching the persistence half needs a probe variant with assertions 1 and 2 replaced by
|
||||
/// prints, which reports <c>Modified</c> and the column moving from <c>NULL</c> to <c>[]</c>.
|
||||
/// Each was separately shown discriminating. A future caller that drops <c>AsNoTracking</c>
|
||||
/// therefore cannot reintroduce the data loss silently.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// The Lucene <see cref="IndexWriter" /> is injected into the private field rather than obtained via
|
||||
/// <c>Initialize</c>, because <c>Initialize</c> writes to <c>FileSystemLayout.SearchIndexFolder</c> —
|
||||
/// a process-wide static resolved once from <c>ETV_CONFIG_FOLDER</c>, i.e. the developer's real
|
||||
/// application data folder. Letting the writer throw instead is NOT an option here: the
|
||||
/// <c>catch</c> in <c>UpdateSong</c> assigns <c>metadata.Song = null</c>, which would itself dirty
|
||||
/// the entity under test and make the probe report the wrong cause.
|
||||
/// </para>
|
||||
/// </summary>
|
||||
[TestFixture]
|
||||
public class SongIndexerMetadataMutationTests
|
||||
{
|
||||
[Test]
|
||||
public async Task UpdateSong_Must_Not_Mutate_Nullable_Artists_On_A_Tracked_Entity()
|
||||
{
|
||||
await using var harness = await InMemoryTvContext.CreateAsync();
|
||||
|
||||
int metadataId;
|
||||
await using (TvContext context = harness.CreateContext())
|
||||
{
|
||||
var library = new LocalLibrary { Name = "Music", MediaKind = LibraryMediaKind.Songs };
|
||||
context.Add(library);
|
||||
await context.SaveChangesAsync();
|
||||
|
||||
var libraryPath = new LibraryPath { Path = "/music", LibraryId = library.Id };
|
||||
context.Add(libraryPath);
|
||||
await context.SaveChangesAsync();
|
||||
|
||||
var song = new Song
|
||||
{
|
||||
LibraryPathId = libraryPath.Id,
|
||||
MediaVersions = [],
|
||||
SongMetadata =
|
||||
[
|
||||
new SongMetadata
|
||||
{
|
||||
MetadataKind = MetadataKind.Fallback,
|
||||
Title = "Untagged Track",
|
||||
SortTitle = "untagged track",
|
||||
DateAdded = new DateTime(2026, 1, 1, 0, 0, 0, DateTimeKind.Utc),
|
||||
|
||||
// The shape FallbackMetadataProvider.GetSongMetadata leaves behind: it never
|
||||
// assigns either primitive collection, so both columns persist as NULL.
|
||||
Artists = null!,
|
||||
AlbumArtists = null!,
|
||||
|
||||
Genres = [],
|
||||
Tags = [],
|
||||
Studios = [],
|
||||
Actors = [],
|
||||
Artwork = [],
|
||||
Guids = []
|
||||
}
|
||||
]
|
||||
};
|
||||
|
||||
context.Add(song);
|
||||
await context.SaveChangesAsync();
|
||||
metadataId = song.SongMetadata[0].Id;
|
||||
}
|
||||
|
||||
// The seed must actually have produced NULL columns, or every assertion below is vacuous.
|
||||
(await ReadRawArtists(harness, metadataId)).ShouldBeNull();
|
||||
|
||||
await using (TvContext context = harness.CreateContext())
|
||||
{
|
||||
// Deliberately TRACKED -- see the fixture docstring.
|
||||
Song tracked = await context.Songs
|
||||
.IncludeForSearch()
|
||||
.AsSplitQuery()
|
||||
.SingleAsync();
|
||||
|
||||
SongMetadata metadata = tracked.SongMetadata[0];
|
||||
metadata.Artists.ShouldBeNull("EF must materialize the NULL column as null, not as an empty list");
|
||||
|
||||
// UpdateSong wraps its whole body in a catch that logs a warning and assigns
|
||||
// `metadata.Song = null` -- which severs a required relationship and cascades the metadata to
|
||||
// Deleted. A silently-exercised catch would therefore make every assertion below report the
|
||||
// wrong cause, so the logger fails the test instead of swallowing.
|
||||
var logger = new ThrowOnWarningLogger<LuceneSearchIndex>();
|
||||
var index = new LuceneSearchIndex(
|
||||
new SearchQueryParser(
|
||||
Substitute.For<ISmartCollectionCache>(),
|
||||
Substitute.For<ILogger<SearchQueryParser>>()),
|
||||
logger);
|
||||
|
||||
using var directory = new RAMDirectory();
|
||||
using var writer = new IndexWriter(
|
||||
directory,
|
||||
new IndexWriterConfig(LuceneVersion.LUCENE_48, new StandardAnalyzer(LuceneVersion.LUCENE_48)));
|
||||
|
||||
typeof(LuceneSearchIndex)
|
||||
.GetField("_writer", BindingFlags.NonPublic | BindingFlags.Instance)!
|
||||
.SetValue(index, writer);
|
||||
|
||||
// A bare substitute returns null from GetAllLanguageCodes, which NPEs inside AddLanguages and
|
||||
// would divert the run into the catch above.
|
||||
var languageCodeService = Substitute.For<ILanguageCodeService>();
|
||||
languageCodeService.GetAllLanguageCodes(Arg.Any<List<string>>()).Returns([]);
|
||||
languageCodeService.GetAllLanguageCodes(Arg.Any<string>()).Returns([]);
|
||||
|
||||
await index.UpdateItems(
|
||||
Substitute.For<ISearchRepository>(),
|
||||
Substitute.For<IFallbackMetadataProvider>(),
|
||||
languageCodeService,
|
||||
[tracked]);
|
||||
|
||||
logger.Failure.ShouldBeNull("UpdateSong threw and its catch ran, so this probe measured the "
|
||||
+ "error path rather than the indexing path");
|
||||
|
||||
// POSITIVE CONTROL. Every assertion below asserts that something did NOT happen, so all of
|
||||
// them hold vacuously if UpdateSong never ran at all -- and it silently stops running if a
|
||||
// future refactor gates UpdateItems on `_initialized`, which this fixture deliberately
|
||||
// bypasses by injecting the writer. Verified BOTH ways by adding
|
||||
// `if (!_initialized) { return Unit.Default; }` to UpdateItems (a bare `return;` does not
|
||||
// compile there -- CS0126): with this line present it is the only failure, and with it
|
||||
// removed the whole test PASSES while the code under test is unreachable.
|
||||
// NumDocs == 1 proves the song-indexing path ran; it does NOT prove the artist loops
|
||||
// specifically ran, which would need a second seeded song asserting ArtistField.
|
||||
writer.NumDocs.ShouldBe(1, "UpdateSong did not index the song, so the assertions below "
|
||||
+ "would pass without exercising the code under test");
|
||||
|
||||
// 1. The indexer left the entity alone.
|
||||
metadata.Artists.ShouldBeNull();
|
||||
metadata.AlbumArtists.ShouldBeNull();
|
||||
|
||||
// 2. ...so EF has nothing to persist. This is the assertion that fails loudly the day the
|
||||
// mutation returns, even if a later refactor stopped the value from being observable above.
|
||||
context.Entry(metadata).State.ShouldBe(EntityState.Unchanged);
|
||||
|
||||
// 3. And the save that a real caller would go on to make does not rewrite the column.
|
||||
await context.SaveChangesAsync();
|
||||
}
|
||||
|
||||
(await ReadRawArtists(harness, metadataId)).ShouldBeNull();
|
||||
}
|
||||
|
||||
private sealed class ThrowOnWarningLogger<T> : ILogger<T>
|
||||
{
|
||||
public Exception? Failure { get; private set; }
|
||||
|
||||
public IDisposable? BeginScope<TState>(TState state) where TState : notnull => null;
|
||||
|
||||
public bool IsEnabled(LogLevel logLevel) => true;
|
||||
|
||||
public void Log<TState>(
|
||||
LogLevel logLevel,
|
||||
EventId eventId,
|
||||
TState state,
|
||||
Exception? exception,
|
||||
Func<TState, Exception?, string> formatter)
|
||||
{
|
||||
if (logLevel >= LogLevel.Warning)
|
||||
{
|
||||
Failure ??= exception ?? new InvalidOperationException(formatter(state, exception));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private static async Task<object?> ReadRawArtists(InMemoryTvContext harness, int metadataId)
|
||||
{
|
||||
await using TvContext context = harness.CreateContext();
|
||||
await using var command = context.Database.GetDbConnection().CreateCommand();
|
||||
command.CommandText = $"SELECT Artists FROM SongMetadata WHERE Id = {metadataId}";
|
||||
object? value = await command.ExecuteScalarAsync();
|
||||
|
||||
// ExecuteScalar returns CLR null both for "the column is NULL" and for "there is no such row",
|
||||
// and the second is reachable: UpdateSong's catch assigns metadata.Song = null, which severs a
|
||||
// required relationship and cascades the row to Deleted, so a SaveChanges on the error path
|
||||
// DELETES it and a plain null check would pass for the wrong reason.
|
||||
if (value is null)
|
||||
{
|
||||
Assert.Fail($"SongMetadata row {metadataId} no longer exists, so its Artists column cannot "
|
||||
+ "be read -- the probe measured a deleted row rather than a preserved NULL.");
|
||||
}
|
||||
|
||||
return value is DBNull ? null : value;
|
||||
}
|
||||
}
|
||||
+1
-1
@@ -26,7 +26,7 @@ doc below, or that changes which sections a task signal points to.**
|
||||
| CI / release pipeline work | `docs/ci-cd.md` + `docs/decisions/release-ci-governance.md` |
|
||||
| Proposing a new guard / CI check / regression test convention | `docs/defect-shapes-773.md` §4 (detector menu + the classes where no detector is plausible), then the three rules every guard must satisfy: `docs/decisions/records/testing/guard-derives-population-from-source.md`, `…/guard-ships-with-mutation-proof.md` and `…/mutation-claims-are-executed.md` (a `MUTATION` grade carries a DECLARED clause mutation that is re-run every suite) |
|
||||
| Testing a surface gated by config / an env var / a credential | `docs/decisions/records/testing/deny-path-at-production-config-value.md` — cover the setting absent, at its production value, and each opt-out, and assert the DENY branch |
|
||||
| Touching a full-replace write path or a hand-built request object | `docs/decisions/records/testing/full-replace-asserts-field-list.md` — derive the field list from the DTO and assert set equality; reconcile by id where child state exists |
|
||||
| Touching a full-replace write path or a hand-built request object | `docs/decisions/records/testing/full-replace-asserts-field-list.md` — derive the field list from the DTO and assert set equality; reconcile by id where child state exists. In the SPA the same rule is enforced by the type system: `docs/spa-conventions.md` §4b — build the body as `Complete<T>`, annotating BOTH the wrapper parameter and every construction site |
|
||||
| Writing or editing any doc, or answering a review finding in prose | `docs/decisions/records/docs/no-session-narrative.md` — the doc records the END STATE; the path to it goes in the commit message. Apply the who-benefits test, and read the carve-out before you cut (dated measurements, stated snapshot boundaries and tested-and-rejected results stay) |
|
||||
| Adding / changing / deleting a guard file | `docs/guard-inventory.md` — every guard's row is machine-checked by `scripts/tests/test_guard_inventory.py`, so a new guard must acquire a row before the suite goes green, and a row graded `MUTATION` must also acquire a declared clause in `scripts/tests/mutation_manifest.py` |
|
||||
| Writing code that reads live Gitea/remote state and then acts on it | `docs/decisions/records/process/check-and-use-pins-a-version.md`, then `docs/remote-state-inventory.md` — a new executable under `scripts/` (**excluding `scripts/tests/`**), `.claude/hooks/`, `.husky/` or `.gitea/workflows/` must acquire a row there before `scripts/tests/test_remote_state_inventory.py` goes green |
|
||||
|
||||
+135
-4
@@ -206,10 +206,14 @@ so a PR claiming many proofs costs proportionally more than the rest of the lane
|
||||
`script-tests`, which is a checkout plus a `pytest` run needing only
|
||||
`pytest` and `pyyaml` (ersatztv#631; it is NOT stdlib-only — that assumption is what turned the
|
||||
job red on its first CI run, see below) — plus **`scan`** (ersatztv#767), the same
|
||||
lightweight-Python shape. `scan` is the lane member to think hardest about before changing anything
|
||||
here: it is the only one that lives in `docker-build.yml` rather than `pr-checks.yml`, so the only
|
||||
one that runs on a **tag push**, and the only one whose failure does not merely redden a status but
|
||||
**skips `build`** — an OOM or a wedge there yields no release image at all.
|
||||
lightweight-Python shape, and **`toolchain-preflight`** (ersatztv#772), a checkout plus one `curl`.
|
||||
Those last two are the lane members that live in `docker-build.yml` rather than `pr-checks.yml`, so
|
||||
they are the ones that also run on a **tag push**. `scan` is still the one to think hardest about
|
||||
before changing anything here: its failure does not merely redden a status but **skips `build`** —
|
||||
an OOM or a wedge there yields no release image at all. `toolchain-preflight` is a `needs:` of nothing, by
|
||||
design — it does not gate the jobs it diagnoses. It is not consequence-free either: like any red
|
||||
job it lands in the PR's combined status, which the merge gate reads (see "When the pinned tag
|
||||
disappears").
|
||||
Nothing there runs a compiler or a `docker build`, which is why the lane
|
||||
can be capped at 1 GiB per job. The lightweight-Python jobs are the deliberate edge of the
|
||||
"git-only" rule, not an exception to it: `setup-python` + `pip install pytest` + a suite whose
|
||||
@@ -1576,6 +1580,133 @@ a follow-up commit. That is the same two-step below, just re-run after the rebas
|
||||
avoid it entirely is to **land a toolchain-image change on its own, before** the work that consumes
|
||||
it, so the consuming branch never carries the `docker/ci` commit through a rebase.
|
||||
|
||||
### When the pinned tag disappears
|
||||
|
||||
⚠️ **An immutable PIN is a promise about what we consume, not about what the registry keeps.** It
|
||||
means the jobs never follow a floating tag like `:latest`, so a fresh toolchain push cannot change
|
||||
what today's CI runs. It does not promise the tag will still EXIST — nor, strictly, that the tag's
|
||||
content is frozen: `ci-image.yml` tags `git rev-parse --short HEAD`, so a `workflow_dispatch` or a
|
||||
weekly `no-cache` run at the same HEAD republishes that same `:<sha>` from a rebuilt image. Those are
|
||||
three different claims, and existence is the one that is not ours to make: the registry belongs to
|
||||
server-management, and an owner-level Gitea *package cleanup rule* there (`keep_count` 15,
|
||||
`remove_days` 1, `remove_pattern` `.*`, and a `keep_pattern` that no 7-hex sha can match) deletes any
|
||||
sha tag once 15 newer versions of the package exist. `ci-image.yml` publishes a new `:<sha>` weekly
|
||||
and on every push touching `docker/ci/**` or the workflow file, while the pin only moves when a human
|
||||
bumps it — so a pin ages toward eviction on its own. That is what happened between 2026-08-11 and
|
||||
2026-08-13 (ersatztv#772): the tag vanished, and every `container:` job — **both required contexts
|
||||
included** — died after 1–2s with
|
||||
|
||||
```
|
||||
Error response from daemon: failed to resolve reference ".../ersatztv-ci:<pin>": not found
|
||||
```
|
||||
|
||||
buried in each job's log. Nothing said "your toolchain image is gone", so the natural first reading
|
||||
was "my diff broke the build", and that is where the review time went. The durable fix is
|
||||
registry-side and is tracked in **timothy/server-management#842**; until it lands, assume any pin
|
||||
older than a couple of weeks can evaporate.
|
||||
|
||||
**How firm that cause is, since it decides whether you go looking further.** The rule and its nightly
|
||||
execution are directly observed; the specific deletion is not, because Gitea hard-deletes package
|
||||
versions with no audit row. What ties them is the same rule's fingerprint on the sibling `ersatztv`
|
||||
package — every `:<sha>` older than the 15-slot window gone, every `keep_pattern` tag kept back to
|
||||
`26.3.1`. Reproduce both halves on the Gitea host (LXC 119, `192.168.1.95`):
|
||||
|
||||
```bash
|
||||
# on the Gitea host: the rule itself
|
||||
sqlite3 /var/lib/gitea/data/gitea.db 'select * from package_cleanup_rule;'
|
||||
# from anywhere: that the cleanup task is scheduled and has been running (schedule/prev/exec_times)
|
||||
curl -s -u user:pass 'http://192.168.1.95:3000/api/v1/admin/cron?limit=50' \
|
||||
| jq '.[] | select(.name == "cleanup_packages")'
|
||||
```
|
||||
|
||||
(Do **not** reach for `journalctl -u gitea | grep ExecuteCleanupRules` — that identifier reaches the
|
||||
log only via slow-query warnings, so an empty grep on a healthy host would read as "the rule never
|
||||
ran", which is the inverse of what it means.)
|
||||
|
||||
If a pin disappears again *after* #842 changes that rule, treat this cause as refuted rather than
|
||||
re-applying it — something else is deleting tags.
|
||||
|
||||
**Detection.** `docker-build.yml::toolchain-preflight` (`scripts/ci-toolchain-image-resolves.sh`)
|
||||
resolves every pin in `docker-build.yml` against the registry on every run and fails with a message
|
||||
that names the tag. It is container-free by necessity — a job consuming the missing image could not
|
||||
run to report it — and deliberately **not** a `needs:` of the five jobs it diagnoses: the container
|
||||
jobs already fail fast, so gating them would tax every green run to speed up a rare red one.
|
||||
|
||||
**Everything it cannot establish is a FAILURE, not a warning**, and the arms are worth knowing
|
||||
because they send you to different places:
|
||||
|
||||
| Answer | Job | Message says |
|
||||
|---|---|---|
|
||||
| HTTP 200 with a manifest body | green | resolves |
|
||||
| HTTP 404 | **red** | `IS GONE` — rebuild the tag (recovery above) |
|
||||
| HTTP 200, body is not a manifest | **red** | something is answering for the registry (proxy, login page) |
|
||||
| 401 / 403 | **red** | the credentials were rejected — fix the secrets |
|
||||
| anything else (5xx, unreachable, no `curl`) | **red** after `ETV_CI_ATTEMPTS` tries | `could NOT VERIFY` — check the registry's health, NOT the pin |
|
||||
| `ETV_REGISTRY_AUTH` unset, malformed, or either half empty | **red**, before any query | an absent secret interpolates to `":"`, which is not a credential |
|
||||
|
||||
The last two rows are the ones worth defending, because warning on them and exiting 0 is the natural
|
||||
way to write this check and it is wrong: a missing `curl`, a moved registry and a DNS change all land
|
||||
there, and a green-with-a-warning job is indistinguishable from a healthy pin forever after. The
|
||||
unknown arm retries first (`ETV_CI_ATTEMPTS`, default 3, `ETV_CI_RETRY_SECONDS` apart) so an ordinary
|
||||
registry blip does not redden a PR — that pause is what makes failing on unknown affordable, and
|
||||
shortening it silently trades this guard for flake.
|
||||
|
||||
**It is not a `needs:` of anything, but it is not consequence-free either.** The merge-consent hook
|
||||
reads the PR's **combined** status and denies on a non-`success` combined state (a `skipped` context
|
||||
counts as green, ersatztv#593; an advisory red does not, ersatztv#598), so a red preflight blocks the
|
||||
merge exactly like any other red job. "Advisory" would be the wrong word for
|
||||
it — what it does not do is *skip* the jobs it diagnoses.
|
||||
|
||||
**Recovery, without needing CI to be healthy.** The tag names a commit, and that commit still builds
|
||||
the same image, so the fastest fix is to republish the *same* tag by hand — no PR, no pin bump, no
|
||||
green CI required, and every open branch recovers at once. Run this on a host with docker and this
|
||||
registry in `insecure-registries` (bumblebee `192.168.1.99` or jazz `192.168.1.29`):
|
||||
|
||||
```bash
|
||||
repo=$(pwd) # keep the CURRENT checkout: the pin commit predates
|
||||
# the preflight script and the verify step below
|
||||
pin=$(grep -oE 'ersatztv-ci:[0-9a-f]+' .gitea/workflows/docker-build.yml | cut -d: -f2 | sort -u)
|
||||
git worktree add /tmp/etv-toolchain "$pin" # the pin IS the commit's short sha
|
||||
cd /tmp/etv-toolchain
|
||||
|
||||
# The registry is HTTP-only and BuildKit does NOT inherit the daemon's insecure-registries, so the
|
||||
# `docker-container` driver (anything created by `docker buildx create`) will try HTTPS and fail.
|
||||
# Either build on the default `docker` driver — `docker buildx use default` — or give the container
|
||||
# driver the same inline config ci-image.yml passes it:
|
||||
# [registry."192.168.1.95:3000"]
|
||||
# http = true
|
||||
prev_builder=$(docker buildx inspect 2>/dev/null | awk '/^Name:/{print $2; exit}')
|
||||
docker buildx use default # needs the containerd image store to --push;
|
||||
# both named hosts have it (checked 2026-08-22)
|
||||
docker login 192.168.1.95:3000 -u timothy
|
||||
docker buildx build --platform linux/amd64 --provenance=false \
|
||||
-f docker/ci/Dockerfile -t "192.168.1.95:3000/timothy/ersatztv-ci:$pin" --push .
|
||||
|
||||
cd "$repo" && git worktree remove /tmp/etv-toolchain
|
||||
[ -n "$prev_builder" ] && docker buildx use "$prev_builder" # leave the builder as you found it
|
||||
```
|
||||
|
||||
Then confirm the tag resolves before re-running anything — the preflight script does exactly this
|
||||
check and takes no arguments. Run it from the CURRENT checkout, not the pin worktree, which is why
|
||||
`$repo` is kept above:
|
||||
|
||||
```bash
|
||||
ETV_REGISTRY_AUTH=user:pass scripts/ci-toolchain-image-resolves.sh
|
||||
```
|
||||
|
||||
**What this rebuild does and does not restore.** It restores a *working* toolchain at that tag, built
|
||||
from that commit's `docker/ci` — not a bit-identical copy of what was deleted: the base image tags
|
||||
and the apt/NodeSource packages the Dockerfile pulls are mutable, so a rebuild picks up whatever they
|
||||
point at today. That is the same exposure the weekly `no-cache` cron has by design. Prefer this over
|
||||
the two-step above whenever the pin is *missing* rather than *stale*: the two-step exists to move the
|
||||
pin to a NEW image, and running it here would leave the repo pinning a different sha for no reason.
|
||||
|
||||
The push path was exercised against this registry on 2026-08-22 — a throwaway `docker push` of a
|
||||
13 MB image to `timothy/etv-772-recovery-probe:probe1` from bumblebee, `HEAD /v2/.../manifests/probe1`
|
||||
→ `200`, then `DELETE /api/v1/packages/timothy/container/etv-772-recovery-probe/probe1` → `204` and
|
||||
the manifest read back `404`. Re-run that shape against a scratch package name to re-establish it;
|
||||
what it establishes is the auth + HTTP-registry push path, not the toolchain build itself.
|
||||
|
||||
**Bumping the pin is enforced, not remembered.** The `ci-image-pin` job (blocking, PR-only; defined
|
||||
in `pr-checks.yml`, but it greps `docker-build.yml` where the pins live) fails if
|
||||
`docker-build.yml`'s pin isn't the short sha of the last commit to touch `docker/ci/**` or
|
||||
|
||||
@@ -102,6 +102,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `mcp.server-foundation` | `ErsatzTV.Mcp` is a fresh stdio JSON-RPC server wrapping frozen `/api/v1` with explicit narrow per-endpoint tools, read-only-by-default enforced at runtime (`ERSATZTV_ALLOW_WRITES`), machine-key auth, and opt-in `If-Match`. | 2026-07-20 | [link](records/mcp/server-foundation.md) |
|
||||
| `mcp.tool-schema-openapi-parity` | Every POST/PUT/PATCH tool in `ToolCatalog` declares exactly the request-body properties its endpoint accepts, each with a matching type, and EVERY tool (read and write) declares exactly its endpoint's query parameters, both asserted against the generated `ErsatzTV/wwwroot/openapi/v1.json` (linked into `ErsatzTV.Mcp.Tests`) by `Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields` and `Every_Tool_Should_Declare_Exactly_Its_OpenApi_Query_Parameters`. A field the endpoint accepts but the tool omits is a DEFECT, not a deferral: on the full-replace tools (channel update, schedule update, custom-order) the omission is silently applied as a clear. The write tools are NOT uniformly full-replace — add-collection-items is additive, and several leave an omitted field unchanged — so each tool description states its own semantics. An omitted query parameter is UNREACHABLE, not merely undocumented, because `ToolArgumentValidator` rejects undeclared arguments. | 2026-08-06 | [link](records/mcp/tool-schema-openapi-parity.md) |
|
||||
| `media.lastscan-null-boundary` | A never-scanned `LastScan` surfaces as `null` at the API/MCP boundary, not the `0001-01-01` MinValue sentinel — enforced by an ongoing read-boundary coercion plus a one-time data migration cleanup. | 2026-07-18 | [link](records/media/lastscan-null-boundary.md) |
|
||||
| `media.nullable-primitive-collection-mutation` | Code that reads a nullable EF PRIMITIVE COLLECTION guards it at the read site — `Optional(x).Flatten()` hoisted into a local — and NEVER writes the guard back onto the entity with `??= []`. Such a collection is stored WHOLE, in ONE COLUMN, so unlike the navigation collections that the same `??= []` idiom guards harmlessly all over this repo, the property IS the column value: assigning it on a TRACKED entity flips the entry to `Modified` and the next `SaveChanges` persists `[]` over what the database held as `NULL`. The population is derived from the MODEL, not from a grep of the domain classes, and is currently EIGHT columns: `SongMetadata.Artists`/`AlbumArtists` (EF-native primitive collections, no explicit configuration, stored as a JSON array) plus the six value-converted collections registered in `ErsatzTV.Infrastructure/Data/Configurations` — `ProgramScheduleAlternate` and `PlayoutTemplate` each carrying `DaysOfMonth`, `MonthsOfYear` (`IntCollectionValueConverter`, COMMA-SEPARATED text, not JSON) and `DaysOfWeek` (`EnumCollectionJsonValueConverter`, JSON). The shared property that matters is not the serialization format but that the collection is ONE SCALAR COLUMN, so do not reason from EF-native primitive-collection behaviour when standing in front of a converter. Only the `SongMetadata` pair is left NULL in practice, because `FallbackMetadataProvider` never assigns it. No site applies `??=` to any of the six (`grep -rn 'DaysOfMonth ??=\\|MonthsOfYear ??=\\|DaysOfWeek ??=' --include='*.cs' .` returns 0 at time of writing), so THIS defect has no instance there; whether a null can reach one of them at runtime is a SEPARATE question this record does not answer and does not assert — the API request records normalize with `?? []`, but `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity, so a non-API caller is UNVERIFIED (#823). A grep for `IList<string>` finds only two of the eight and an enum collection none of them — so a sweep follows the FIELD via the model configuration, not the file: the mutation and every read it was protecting must move together, or removing the assignment trades a silent write for a live throw. The read FORM decides which throw, and BOTH occur in this one PR: `foreach` over a null collection throws `NullReferenceException` (the two Lucene reads — measured), while `string.Join`/`Enumerable.ToList` on a null SOURCE throw `ArgumentNullException` (the two Elastic reads, and the `#671` mapper sites). Neither exception type alone characterises the class, so neither grep finds it — which is the actual reason a sweep must follow the FIELD rather than an exception name. Whether today's callers happen to be `AsNoTracking` is NOT the safety argument and must not be written down as one: it is a property of the current callers, not of the code holding the entity, and it is what #691 recorded as a loaded gun. | 2026-08-22 | [link](records/media/nullable-primitive-collection-mutation.md) |
|
||||
| `media.remote-stream-probe` | `ValidatePlayoutItemPath` probes the Plex/Jellyfin/Emby remote-stream URL via `IRemoteStreamProber` before returning it; only a redirected 404 fails closed (`PlayoutItemNotAvailableFromMediaServer`), everything else fails open, and there is no toggle. | 2026-07-19 | [link](records/media/remote-stream-probe.md) |
|
||||
| `media.remote-stream-probe-externaljson` | External-JSON playout channels' `StreamRemotely` now probes the remote-stream URL through the same `IRemoteStreamProber` seam as the generated-playout path, closing the #473 scope gap for a channel kind with no DB `PlayoutItem` rows. | 2026-07-20 | [link](records/media/remote-stream-probe-externaljson.md) |
|
||||
| `media.source-mgmt-write-api` | Media-source management (local/Plex/Jellyfin/Emby) is a REST write API + SPA under `/app/libraries/*`, wrapping existing MediatR commands 1:1 with no new commands or DB migration; connection GETs never leak a stored `apiKey`, and each PUT-replace family's identity contract is documented per-family (not assumed uniform). | 2026-07-11 | [link](records/media/source-mgmt-write-api.md) |
|
||||
@@ -136,6 +137,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `release.promotion-floating-prod` | Prod tracks the floating `:prod` image reference; a tag build's immutable `:<version>` image is scanned first, then promotion happens via a separate manual `DeployStack`, with daily auto-update only as a fallback — tag with enough runway before 03:00 to avoid an unscanned promotion. | 2026-07-13 | [link](records/release/promotion-floating-prod.md) |
|
||||
| `release.review-verdict-gate` | A PR may not merge until a `Review-verdict: <MERGEABLE\|APPROVED\|BLOCKED\|NOT-MERGEABLE> @ <head-sha>` comment references the PR's current head sha (short-sha prefix match against the verdict's OWN `@ <sha>` field, marker at COLUMN 0 (no indent, so indented code blocks cannot self-approve), whole-word verdict token, fenced code blocks stripped with markdown fence-length semantics, negative wins over positive on the same head); folds into the H6 merge-consent hook as condition (c). The grammar lives in ONE tested place, `scripts/check-review-verdict.sh` — #629 found three false-opens that survived because it was implemented inline and untested while this record described stricter behaviour than the code had. | 2026-07-12 | [link](records/release/review-verdict-gate.md) |
|
||||
| `release.verdict-status-check` | The H10 review verdict is written as a `review-verdict/h10` Gitea **commit status** on the exact reviewed sha by `scripts/post-review-verdict.sh`, and that context is a REQUIRED status check on `main`. Because a status belongs to one sha, a later commit cannot inherit it, so Gitea's own `merge_when_checks_succeed` refuses to merge a head no one reviewed. The PreToolUse hook additionally refuses to SCHEDULE an auto-merge unless that status is already green on head. A `pull_request_target` workflow auto-passes the two exempt classes (Renovate-authored, docs-only) unless the PR touches a protected path (`.claude/`, `.codex/`, `.gitea/`, `.husky/`, `scripts/`, `docker/ci/`). This extends — does not supersede — `release.review-verdict-gate` (#303 H10), whose comment convention remains the human-readable artifact and the hook's condition (c). | 2026-07-25 | [link](records/release/verdict-status-check.md) |
|
||||
| `release.verdict-writes-status-before-comment` | `scripts/post-review-verdict.sh` writes the sha-bound `review-verdict/h10` commit status FIRST and the human-readable `Review-verdict:` comment SECOND. Every refusal path still refuses (fail-closed, unchanged) and exits non-zero, and none of them may leave a verdict comment behind. An orphaned comment is therefore PREVENTED rather than tolerated. If the comment write fails after the status was written, that is an error too, but it degrades to an `ask` at the merge gate rather than to an apparent grant. | 2026-08-22 | [link](records/release/verdict-writes-status-before-comment.md) |
|
||||
| `rulebuilder.relative-date-macros` | The visual rule builder's `inLast`/`notInLast` date operators compile to/parse from the pre-existing `CustomMultiFieldQueryParser` macros `released_inthelast`/`released_notinthelast` and `added_inthelast`/`added_notinthelast`, value form `"<n> day\|week\|month\|year"`; there is no backend change. | 2026-07-23 | [link](records/rulebuilder/relative-date-macros.md) |
|
||||
| `scan.collections-scan-status` | `GET /api/v1/media-sources/collections-scan-status` reports a family-global (not per-source), boolean-only active-scan set read from `IEntityLocker`; the SPA reconciles authoritatively against it (with a grace-tick helper) instead of a fixed client-side timeout. | 2026-07-12 | [link](records/scan/collections-scan-status.md) |
|
||||
| `scan.getoraddfolder-db-lookup` | `ILibraryRepository.GetOrAddFolder` resolves the existing folder via a DB query on `(LibraryPathId, Path)`, not the caller's `LibraryPath.LibraryFolders` in-memory navigation, since that navigation is only eager-loaded on the local scan path and is null on remote (Jellyfin) callers. | 2026-07-20 | [link](records/scan/getoraddfolder-db-lookup.md) |
|
||||
@@ -199,7 +201,7 @@ the link for rationale. Superseded/retired history lives in `archive/`. Regenera
|
||||
| `testing.e2e-local-fresh-config-dir` | Always point `scripts/e2e-local.sh` at a fresh config dir — leftover channels/schedules/DB rows bleed state between runs and corrupt assertions. (The *readiness-probe hang* this record was originally written about was fixed in #533; the fresh-dir rule stands on state-bleed grounds alone.) | 2026-07-21 | [link](records/testing/e2e-local-fresh-config-dir.md) |
|
||||
| `testing.enumerating-guard-identity-not-position` | A guard that cross-checks a hand-reviewed registry against call sites discovered across the whole repo must key each entry on properties INTRINSIC to the site — file, kind, and the value source text — and never on its absolute line or column. A registry keyed on position is a function of every other file in the repo, so a branch that never touches the guard can invalidate it; and because each PR is green against its own base, that failure is structurally invisible pre-merge and lands on `main` after review and after the merge gate. Dropping the position keeps every mutation the guard exists for — a NEW site, a REMOVED site and a CHANGED value each still fail, since each changes the identity multiset — and costs exactly ONE case, which must be stated rather than implied: a SAME-IDENTITY SUBSTITUTION within one file (delete a registered site, add a different unreviewed one with the same kind and value token, net-zero count) now passes. A REPORTED failure still prints the discovered line:column, because identity and diagnostics need not share a format. Comparison stays a MULTISET count rather than set membership, so two sites in one file sharing an identity must be discovered exactly that many times and a third occurrence still fails. A SCANNER test that asserts real AST positions against FIXED inline fixtures is the opposite case and keeps its line/column identity — it has no churn, because its input does not move. | 2026-07-27 | [link](records/testing/enumerating-guard-identity-not-position.md) |
|
||||
| `testing.fix-ships-a-witnessed-red-test` | A commit claiming to fix something may carry a `Proves: <pytest selector>` trailer; when it does, `scripts/prove-fix.sh` must show that selector GREEN with the fix and RED with the code side reverted, and CI enforces it per-PR. The trailer is opt-in — an unproven commit is allowed — but a claimed proof that does not hold fails the build. | 2026-08-16 | [link](records/testing/fix-ships-a-witnessed-red-test.md) |
|
||||
| `testing.full-replace-asserts-field-list` | Any path that writes a WHOLE entity or a WHOLE child collection — a PUT-replace handler, a hand-built request object, a test comparer standing in for one — derives its field list from the authoritative type and asserts SET EQUALITY against it, rather than enumerating the fields by hand. A hand-written list is correct on the day it is written and structurally unable to report the day it stops being: the field that drifts is the one nobody wrote a line for, so no amount of care in the existing lines can reach it. The failure is silent by construction — a full replace with a field omitted returns HTTP 200 and destroys that field's value (#754 drifted from a 28-property DTO by one and cleared it; the symptom arrived hours later as missing pixels). SECOND CLAUSE, separable from the first: where a replaced child row carries state keyed to its identity — progression, ordering, an enumerator position — the handler RECONCILES BY ID rather than delete-and-reinsert, because reinsertion silently resets state a client never asked to touch (#252: a schedule PUT reset fill-group progression; #500: a dedup fix became permanent data loss because the add filter and the remove filter used different keys, so the two halves must agree on the key). Delete-and-reinsert is acceptable ONLY where no such state exists, and that emptiness is a fact about today's schema that a later feature can silently invalidate — so record it where the handler is, dated, rather than leaving it to be re-derived. The canonical worked example is `ToolCatalogTests.Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields`, which reads the accepted fields from the generated OpenAPI document and compares both directions. | 2026-08-21 | [link](records/testing/full-replace-asserts-field-list.md) |
|
||||
| `testing.full-replace-asserts-field-list` | Any path that writes a WHOLE entity or a WHOLE child collection — a PUT-replace handler, a hand-built request object, a test comparer standing in for one — derives its field list from the authoritative type and asserts SET EQUALITY against it, rather than enumerating the fields by hand. A hand-written list is correct on the day it is written and structurally unable to report the day it stops being: the field that drifts is the one nobody wrote a line for, so no amount of care in the existing lines can reach it. The failure is silent by construction — a full replace with a field omitted returns HTTP 200 and destroys that field's value (#754 drifted from a 28-property DTO by one and cleared it; the symptom arrived hours later as missing pixels). SECOND CLAUSE, separable from the first: where a replaced child row carries state keyed to its identity — progression, ordering, an enumerator position — the handler RECONCILES BY ID rather than delete-and-reinsert, because reinsertion silently resets state a client never asked to touch (#252: a schedule PUT reset fill-group progression; #500: a dedup fix became permanent data loss because the add filter and the remove filter used different keys, so the two halves must agree on the key). Delete-and-reinsert is acceptable ONLY where no such state exists, and that emptiness is a fact about today's schema that a later feature can silently invalidate — so record it where the handler is, dated, rather than leaving it to be re-derived. The canonical worked example is `ToolCatalogTests.Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields`, which reads the accepted fields from the generated OpenAPI document and compares both directions. ON THE SPA SIDE the same rule is enforced by the TYPE SYSTEM rather than by a test: a full-replace body is built as `Complete<T>` (`web/src/api/completeRequest.ts`), a mapped type that makes every member of a generated request type required, so a builder that omits one fails `npm run typecheck`. Annotate BOTH the API-wrapper parameter (so later callers inherit it) AND each construction site including every `.map` callback return type, because the excess-property check that catches a PHANTOM field fires only on a fresh literal in a contextually typed position and a generic `.map` callback is not one. Do not infer from "most builders already typecheck" that the gap is closed: a member is omittable exactly when it is absent from the schema `required` array in the ASP.NET-produced OpenAPI document, and two live cases (#807) sat unchecked inside a large majority of checked ones. | 2026-08-21 | [link](records/testing/full-replace-asserts-field-list.md) |
|
||||
| `testing.guard-derives-population-from-source` | A guard that asserts a COMPLETENESS property enumerates its population from a machine-readable authoritative source — the enum, the generated OpenAPI document, the parsed workflow YAML, the provider list — and asserts SET EQUALITY in BOTH directions against it. It may not narrow that population with a filter, a `Where`, a `grep` or an early `continue` before the assertion, because a filter cannot see the member that is MISSING: the member whose absence is the defect is precisely the one the predicate excludes. A hand-written literal list of members is the same defect in slower motion — a filter frozen at authoring time, correct on the day it was written and unable to report the day it stopped being. Two boundaries bound the rule rather than weaken it. FIRST, filtering to select the SUBJECT of a PER-MEMBER property is legitimate and is not this defect: the excluded members satisfy the property vacuously, so the filtered walk and the whole walk assert the same thing (`ToolCatalogTests.Every_Query_Parameter_Should_Be_A_Declared_Property` filters to tools that declare query parameters, and a tool declaring none has nothing to check). The defect is filtering the population before a COMPLETENESS claim, which is what makes an absent member unrepresentable (#757 filtered on `QueryParameters is {Count: > 0}` and so could not see a tool that should have declared one and did not). SECOND, a population of VALUES always has an external authoritative source and this rule applies directly; a population of SITES IN CODE has no such list, needs find-all-references tooling, and is tracked separately in #777 — do not stretch a set-equality assertion over it. Distinguish the guard SCOPE (which subsystems it covers — a reviewed policy choice, legitimately hand-written) from the guard POPULATION (the members inside that scope — always derived). When the scope itself MIRRORS an authoritative source, the mirror needs its own equality check or a dated staleness marker, or the guard is complete within a scope that has silently gone stale. The canonical worked example in this repo is `ToolCatalogTests.Every_Tool_Should_Declare_Exactly_Its_OpenApi_Query_Parameters`; the canonical residual gap is `MARKED_JOBS` in `scripts/tests/test_ci_dropped_step_guard.py`. WHEN THE POPULATION IS FILES (#806), the authoritative source is the GIT INDEX and never a filesystem walk. A walk is not merely a weaker enumerator, it answers a question about the MACHINE rather than about the repo: it reports build output, generated shims and editor droppings, and it differs between CI and every checkout, so the same guard asserts a different population in each place. Derive with `git ls-files`, take direct children only unless a nested population is stated and wanted, and assert existence rather than filtering on it, because filtering is what makes a missing member unrepresentable. This is an instantiation and not a blanket rewrite: the question per guard remains whether it makes a COMPLETENESS claim over TRACKED files, and a walk that assembles a fixture or selects the SUBJECT of a per-member property stays a walk with its reason written down. | 2026-08-13 | [link](records/testing/guard-derives-population-from-source.md) |
|
||||
| `testing.guard-ships-with-mutation-proof` | A guard is not considered tested because a test involving it passes. It ships with a MUTATION PROOF: remove or disarm THAT GUARD'S CLAUSE ALONE, and a NAMED test must go red. ONE NAMED EXCEPTION, with its limits, because the rule degenerates without it: where the guard IS a test (a checker enforcing a repo invariant, with no separate script behind it), disarming it makes it ABSENT rather than red, so the proof is the contrapositive — INTRODUCE THE DEFECT THE GUARD EXISTS TO CATCH into an isolated copy of the guarded artifact, and the named test must go red. That is a mutation of the guarded SYSTEM rather than of the assertion, and it is admissible ONLY for checker-guards and ONLY when the mutation was executed and witnessed. It is NOT a licence to grade an ordinary script-guard MUTATION for having a bad-input test: feeding a script an input its clause rejects is BEHAVIOUR-ONLY, which is what three rows were regraded for. A file-level grade under this exception covers the clause its cited case actually mutates, not every assertion that later lands in the same file. Three things this excludes, each of which has already shipped here as a green suite over a dead check. FIRST, a behavioural test — one that feeds the guard a good input and a bad input and checks it passes and fails — proves the guard REACTS, never that it is LOAD-BEARING; #685 had two guards on one condition where deleting either left the whole suite green while every behavioural test passed. SECOND, mutating the WHOLE FILE does not count (#510): a whole-file revert cannot show that a test reaches a particular clause, so the mutation must target the clause. THIRD, the guard being WIRED is not the guard RUNNING — #631's suite was invoked by no CI job, #751's step was dropped by the runner and the job reported success in 6s against a normal 14-17s, and #719's new logic was never connected to stdin. Every guard that DERIVES A POPULATION also carries an ANTI-VACUITY assertion, because the characteristic failure of a completeness check is reporting that it proved everything while its population was empty; a guard with no population has nothing for such an assertion to be about, and stating it universally reads as coverage the unproven rows do not have. Mechanical enforcement is possible for the BOOKKEEPING and not for the JUDGEMENT, and the split is the decision: `docs/guard-inventory.md` lists every guard file with its Kind, its Proof class (`MUTATION`/`BEHAVIOUR-ONLY`/`NONE`) and a `file::function` ref, and `scripts/tests/test_guard_inventory.py` derives the guard population from the GIT INDEX and the call sites (#806), asserts SET EQUALITY against the rows, and resolves every claimed ref to a real `def`. So a new guard cannot ship unclassified and a renamed test cannot leave a row silently claiming coverage. Whether a row claiming `MUTATION` is telling the truth is no longer left to review: `testing.mutation-claims-are-executed` (#790) requires each such row to carry a DECLARED clause mutation that is applied to an isolated copy of the repository on every run, with the row's own named test required to go red. | 2026-08-13 | [link](records/testing/guard-ships-with-mutation-proof.md) |
|
||||
| `testing.hook-reports-its-own-execution` | Every script in `.claude/hooks/` sources `scripts/hook-fire-log.sh` and calls `etv_hook_fire_begin <its-own-name> <label> <capture\|stream>` as its FIRST act, before anything reads stdin. Two records are appended per invocation — a `fire` record on entry and an `exit` record carrying the exit status and the decision — to a session-scoped JSONL log. THE DECISION IS READ FROM WHAT THE HOOK ACTUALLY EMITTED, never declared by the hook author: Claude Code hooks (`capture` mode) always exit 0 and communicate by PRINTING JSON, so their stdout is diverted and replayed, and the recorded decision is parsed from those bytes; git hooks (`stream` mode) decide by EXIT CODE and their stdout is live progress text a human is watching, so it is not diverted and the decision is the status. That split is not a tuning knob — capturing a slow pre-push hook's output would hold it back until the end and read as a hang, and inferring a git hook's decision from absent JSON would put the report back into the guessing business this record exists to end. The population is DERIVED from `.claude/hooks/*.sh` by `scripts/tests/test_hook_fire_log.py`, so a new hook is uninstrumented-and-red rather than silently unobserved, and the report lists every hook that EXISTS rather than every hook that appears in the log — a report built from the log alone can only show hooks that fired, which makes the never-fired hook, the one finding worth having, invisible. THE INSTRUMENTATION MUST BE INVISIBLE TO THE HARNESS, and this is the load-bearing half: it sits in the stdin and stdout path of the most authoritative guards in the repo, so a differential test drives EVERY hook with and without it over a payload matrix and demands byte-equal stdout and equal exit status. It fails OPEN in exactly one direction — if the log cannot be written the hook behaves exactly as before — because observability that breaks a guard is worse than the blindness it replaces. Two mechanical traps are pinned by tests rather than left to care: stdout must be replayed from the FILE, since `out=$(cat f)` strips trailing newlines and delivers a guard's JSON one byte short with no parser anywhere to complain; and stdin must never be slurped when it is a TTY, because an interactive `git commit` hands its hooks a terminal and `cat` would block forever, hanging the commit the instrumentation was added to observe. | 2026-08-14 | [link](records/testing/hook-reports-its-own-execution.md) |
|
||||
|
||||
@@ -7,7 +7,7 @@ supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'A step the runner declines to interpolate is DROPPED and the job still concludes `success` (`ci.workflow-run-body-no-expressions`). In `review-verdict.yml` that is fail-CLOSED — the required status is absent and the merge is blocked. In `docker-build.yml`''s `test` and `migrations` it is fail-OPEN: those are the other two required contexts on `main`, so the check reports green having done no work. So in those two jobs every `run:` step that is not `continue-on-error: true` calls `"$GITHUB_WORKSPACE/scripts/ci-step-ran.sh" mark <key>` as its FIRST act, and the job''s LAST step calls `ci-step-ran.sh assert --always <keys> --gated <keys>`, which fails the job when an expected key was never recorded. PER STEP, not per job: a marker written by the first step only proves the job started, while the drop that costs something is `Test` or the migration replay. The guard carries NO `if:` — the default `success()` is the wanted condition, because a genuine failure in an early step legitimately skips every later one and an `always()` guard would announce a false "these steps never executed" on every ordinary red build; the invariant that makes the omission safe is that the guard is skipped only when an earlier step FAILED, which already fails the job, so guard-skipped implies job-red and every path to a green job runs the guard. Separately and independently, no `${{` OPENER may appear in any `run:` body of those two jobs OR of `build` — the drop mechanism requires the opener, so banning it makes the class unreachable rather than merely caught, and an UNCLOSED opener triggers the same rewrite as a well-formed pair. Pass values in through the step''s `env:`, which is interpolated per value. The two halves have DIFFERENT scopes on purpose: markers cover the required pair, while the ban also covers `build`, whose `Smoke + IPTV E2E` step runs AFTER the image is pushed, so a drop there publishes a release candidate that was never booted and that `DeployStack jazz-media` then promotes. `functional-e2e` is delimiter-free but deliberately excluded (advisory by declaration), and `api-docs`/`format` keep one `github.base_ref` each and gate nothing that ships. The ban is enforced on the RELEASE PATH itself, not only in review (#767): a `scan` job runs the PyYAML-based ban test and `build` lists it in `needs:`, so a delimiter means `build` never runs and no image is published. A guard STEP inside `build` was tried first and is wrong — a step cannot protect the job it publishes from, and "my body has no opener so I cannot be dropped" is circular when only the PR-only test enforces that. The pytest in `script-tests` remains, but it is `on: pull_request` and not a required context, so it alone left the tag path unchecked.'
|
||||
signals: 'required check green but no work done, step never ran but job green, Build & test green in seconds, EF migration integrity green without replaying, missing Run Main step marker, Unable to interpolate expression format(, dropped step docker-build, ci-step-ran.sh, marker file, expression delimiter in a required job · paths: `.gitea/workflows/docker-build.yml`, `scripts/ci-step-ran.sh`, `scripts/tests/test_ci_dropped_step_guard.py`, `scripts/tests/test_ci_release_path_scan_job.py` · issues: #756, #751, #684, #767'
|
||||
mechanics: '`scripts/ci-step-ran.sh` owns the marker path so it exists ONCE and the write and the read cannot diverge. It is keyed on `GITHUB_JOB`/`GITHUB_RUN_ID` — REQUIRED, refusing rather than falling back to a reusable name — plus `GITHUB_RUN_ATTEMPT`. All three REFUSE rather than falling back to a reusable name. The third was warn-and-default until its presence was measured: grepping a log for the variable NAME proves nothing, and inferring it from the absence of a stderr warning proves nothing either (stderr capture was itself unestablished), so `assert` was made to print `Marker identity: job=… run=… attempt=… (from the runner)` on STDOUT and the answer was read off run 1916 for both required jobs. That line is retained as standing evidence. Do NOT justify the keying with #751''s "RUNNER_TEMP is /tmp, not a private per-job dir": that was measured on a job with no `container:` and does not transfer — these jobs get a fresh container, which is the primary protection, and the keying is defence in depth. Held by `scripts/tests/test_ci_dropped_step_guard.py`: static (marker set derived from the workflow equals the guard''s expectations, bucket matches each step''s `if:`, guard is last / has no `if:` / is not advisory / has no delimiter) and behavioural (the guard''s real command line executed against markers written by the steps'' real marker lines, dropping each key in turn). The release-path `scan` job (#767) runs the existing PyYAML-based ban test rather than a second implementation, so there is no drift surface; `scripts/tests/test_ci_release_path_scan_job.py` holds the WIRING instead — that `build` needs it AND that `build`''s own `if:` carries no `always()`/`!cancelled()`/`failure()` (which would downgrade the `needs:` edge to mere ordering), that `scan` carries no job-level `if:` (one excluding the tag push restores the hole, one skipping the job skips `build` too) and is not advisory at STEP or JOB level, and that its own run bodies are delimiter-free. Its load-bearing test is an EXECUTION PROBE, not a shape assertion: it runs the scan step''s real `run:` body with the full env the runner would give it (workflow, job AND step `env:` tiers) against a copy of the repo whose `Smoke` body carries an injected delimiter, and requires a non-zero exit, with a clean-tree negative control. Shape assertions were tried and lost repeatedly — from `echo`ing the command to `PYTEST_ADDOPTS` one env tier up — so do NOT replace the probe with cheaper checks about the command. Two tiers cannot be reached from inside pytest at all and are handled differently: a step writing to `$GITHUB_ENV` is BANNED by test, and repo-root pytest configuration (`pytest.ini` `addopts`, `pytest_collection_modifyitems`) can deselect any test including the guards, so the positive control is a SHELL step — `scripts/ci-prove-ban-detects.sh` poisons the checked-out workflow in the REAL checkout, re-runs the ban test, and vouches ONLY for the ban test''s `build` parametrisation failing — any other outcome (exit 5 from a total deselect, exit 2 from a collection error, an unrelated test failing) is a REFUSAL, not a pass, because each weaker reading was a live bug in an earlier draft and the deselection disarm it exists to catch exits 5 rather than 1. That script is itself positively controlled — `test_the_PROOF_SCRIPT_itself_refuses_when_the_ban_is_deselected` and `test_the_PROOF_SCRIPT_refuses_when_the_WRONG_test_fails` cover the two refusal branches a disarm actually lands on, each verified by making that branch alone unreachable — since it was for a while the one guard exercised only on the happy path. The third branch (pytest passing outright) has no control and does not need one: neutering it falls through to the exit-code branch, which still refuses. A copy-based proof is not equivalent: it does not inherit the repo-root config a disarm would live in. Its steps carry markers and a trailing assert of their own, verified by the same drop-each-key-in-turn behavioural pattern. CARVE-OUT: the "fresh container is the primary protection, keying is defence in depth" reasoning above does NOT cover `scan` — it has no `container:` and runs on `small`, where RUNNER_TEMP is the shared host /tmp, so for that job the run-id/attempt keying is the ONLY protection. Residual: a single-job re-run that does not increment GITHUB_RUN_ATTEMPT would find the prior attempt''s marker file and the assert would pass even with the pytest step dropped. Identity on that lane was measured, not assumed — run 1929 printed `Marker identity: job=scan run=1929 attempt=1 (from the runner)`.'
|
||||
mechanics: '`scripts/ci-step-ran.sh` owns the marker path so it exists ONCE and the write and the read cannot diverge. It is keyed on `GITHUB_JOB`/`GITHUB_RUN_ID` — REQUIRED, refusing rather than falling back to a reusable name — plus `GITHUB_RUN_ATTEMPT`. All three REFUSE rather than falling back to a reusable name. The third was warn-and-default until its presence was measured: grepping a log for the variable NAME proves nothing, and inferring it from the absence of a stderr warning proves nothing either (stderr capture was itself unestablished), so `assert` was made to print `Marker identity: job=… run=… attempt=… (from the runner)` on STDOUT and the answer was read off run 1916 for both required jobs. That line is retained as standing evidence. Do NOT justify the keying with #751''s "RUNNER_TEMP is /tmp, not a private per-job dir": that was measured on a job with no `container:` and does not transfer — these jobs get a fresh container, which is the primary protection, and the keying is defence in depth. Held by `scripts/tests/test_ci_dropped_step_guard.py`: static (marker set derived from the workflow equals the guard''s expectations, bucket matches each step''s `if:`, guard is last / has no `if:` / is not advisory / has no delimiter) and behavioural (the guard''s real command line executed against markers written by the steps'' real marker lines, dropping each key in turn). The release-path `scan` job (#767) runs the existing PyYAML-based ban test rather than a second implementation, so there is no drift surface; `scripts/tests/test_ci_release_path_scan_job.py` holds the WIRING instead — that `build` needs it AND that `build`''s own `if:` carries no `always()`/`!cancelled()`/`failure()` (which would downgrade the `needs:` edge to mere ordering), that `scan` carries no job-level `if:` (one excluding the tag push restores the hole, one skipping the job skips `build` too) and is not advisory at STEP or JOB level, and that its own run bodies are delimiter-free. Its load-bearing test is an EXECUTION PROBE, not a shape assertion: it runs the scan step''s real `run:` body with the full env the runner would give it (workflow, job AND step `env:` tiers) against a copy of the repo whose `Smoke` body carries an injected delimiter, and requires a non-zero exit, with a clean-tree negative control. Shape assertions were tried and lost repeatedly — from `echo`ing the command to `PYTEST_ADDOPTS` one env tier up — so do NOT replace the probe with cheaper checks about the command. Two tiers cannot be reached from inside pytest at all and are handled differently: a step writing to `$GITHUB_ENV` is BANNED by test, and repo-root pytest configuration (`pytest.ini` `addopts`, `pytest_collection_modifyitems`) can deselect any test including the guards, so the positive control is a SHELL step — `scripts/ci-prove-ban-detects.sh` poisons the checked-out workflow in the REAL checkout, re-runs the ban test, and vouches ONLY for the ban test''s `build` parametrisation failing — any other outcome (exit 5 from a total deselect, exit 2 from a collection error, an unrelated test failing) is a REFUSAL, not a pass, because each weaker reading was a live bug in an earlier draft and the deselection disarm it exists to catch exits 5 rather than 1. That script is itself positively controlled — `test_the_PROOF_SCRIPT_itself_refuses_when_the_ban_is_deselected` and `test_the_PROOF_SCRIPT_refuses_when_the_WRONG_test_fails` cover the two refusal branches a disarm actually lands on, each verified by making that branch alone unreachable — since it was for a while the one guard exercised only on the happy path. The third branch (pytest passing outright) has no control and does not need one: neutering it falls through to the exit-code branch, which still refuses. A copy-based proof is not equivalent: it does not inherit the repo-root config a disarm would live in. Its steps carry markers and a trailing assert of their own, verified by the same drop-each-key-in-turn behavioural pattern. CARVE-OUT: the "fresh container is the primary protection, keying is defence in depth" reasoning above does NOT cover the container-free jobs — `scan`, and `toolchain-preflight` since #772. Neither has a `container:` and both run on `small`, where RUNNER_TEMP is the shared host /tmp, so for those jobs the run-id/attempt keying is the ONLY protection. The MEMBERSHIP is the part that rots: a further container-free job added without joining this list silently inherits a reassurance nobody checked for it. Residual: a single-job re-run that does not increment GITHUB_RUN_ATTEMPT would find the prior attempt''s marker file and the assert would pass even with the pytest step dropped. Identity on that lane was measured, not assumed — run 1929 printed `Marker identity: job=scan run=1929 attempt=1 (from the runner)`.'
|
||||
---
|
||||
|
||||
**Why per step, when #756 proposed per job.** A job-start marker answers "did this job begin", which
|
||||
|
||||
@@ -0,0 +1,68 @@
|
||||
---
|
||||
key: media.nullable-primitive-collection-mutation
|
||||
title: '2026-08-22 — A nullable primitive collection is guarded at the READ SITE and never assigned back onto a possibly-tracked entity (#701)'
|
||||
status: active
|
||||
since: '2026-08-22'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'Code that reads a nullable EF PRIMITIVE COLLECTION guards it at the read site — `Optional(x).Flatten()` hoisted into a local — and NEVER writes the guard back onto the entity with `??= []`. Such a collection is stored WHOLE, in ONE COLUMN, so unlike the navigation collections that the same `??= []` idiom guards harmlessly all over this repo, the property IS the column value: assigning it on a TRACKED entity flips the entry to `Modified` and the next `SaveChanges` persists `[]` over what the database held as `NULL`. The population is derived from the MODEL, not from a grep of the domain classes, and is currently EIGHT columns: `SongMetadata.Artists`/`AlbumArtists` (EF-native primitive collections, no explicit configuration, stored as a JSON array) plus the six value-converted collections registered in `ErsatzTV.Infrastructure/Data/Configurations` — `ProgramScheduleAlternate` and `PlayoutTemplate` each carrying `DaysOfMonth`, `MonthsOfYear` (`IntCollectionValueConverter`, COMMA-SEPARATED text, not JSON) and `DaysOfWeek` (`EnumCollectionJsonValueConverter`, JSON). The shared property that matters is not the serialization format but that the collection is ONE SCALAR COLUMN, so do not reason from EF-native primitive-collection behaviour when standing in front of a converter. Only the `SongMetadata` pair is left NULL in practice, because `FallbackMetadataProvider` never assigns it. No site applies `??=` to any of the six (`grep -rn ''DaysOfMonth ??=\|MonthsOfYear ??=\|DaysOfWeek ??='' --include=''*.cs'' .` returns 0 at time of writing), so THIS defect has no instance there; whether a null can reach one of them at runtime is a SEPARATE question this record does not answer and does not assert — the API request records normalize with `?? []`, but `ReplacePlayoutAlternateScheduleItemsHandler` and `ReplacePlayoutTemplateItemsHandler` assign the command value straight onto the entity, so a non-API caller is UNVERIFIED (#823). A grep for `IList<string>` finds only two of the eight and an enum collection none of them — so a sweep follows the FIELD via the model configuration, not the file: the mutation and every read it was protecting must move together, or removing the assignment trades a silent write for a live throw. The read FORM decides which throw, and BOTH occur in this one PR: `foreach` over a null collection throws `NullReferenceException` (the two Lucene reads — measured), while `string.Join`/`Enumerable.ToList` on a null SOURCE throw `ArgumentNullException` (the two Elastic reads, and the `#671` mapper sites). Neither exception type alone characterises the class, so neither grep finds it — which is the actual reason a sweep must follow the FIELD rather than an exception name. Whether today''s callers happen to be `AsNoTracking` is NOT the safety argument and must not be written down as one: it is a property of the current callers, not of the code holding the entity, and it is what #691 recorded as a loaded gun.'
|
||||
signals: 'Artists ??= [] on a tracked entity · nullable primitive collection not a navigation · JSON array in one column · value converter is the same hazard as a primitive collection · IntCollectionValueConverter EnumCollectionJsonValueConverter · derive the collection-column population from the model configuration · SaveChanges writes empty array over NULL · entity flips to Modified on a read guard · AsNoTracking today is not a safety argument · untagged song loses its NULL artists · foreach over null throws NRE while string.Join throws ArgumentNullException · sweep by FIELD not by file · Optional Flatten hoisted local · paths: `ErsatzTV.Infrastructure/Search/LuceneSearchIndex.cs`, `ErsatzTV.Infrastructure/Search/ElasticSearchIndex.cs`, `ErsatzTV.Application/Channels/Commands/RefreshChannelDataHandler.cs`, `ErsatzTV.Tests/Integration/SongIndexerMetadataMutationTests.cs` · issues: #701, #691, #671, #823, #824'
|
||||
mechanics: 'Pinned by `SongIndexerMetadataMutationTests.UpdateSong_Must_Not_Mutate_Nullable_Artists_On_A_Tracked_Entity`, which drives the real `LuceneSearchIndex` against a real `TvContext` on SQLite with a deliberately TRACKED song. That fixture covers LUCENE ONLY — `ElasticSearchIndex` holds an independent copy of the same code and needs a stubbed transport, so an Elastic-only reintroduction stays green (#824). No repo-wide detector: the `??=` idiom is correct on navigations and appears 92 times across the app projects (`grep -rn ''??= '' --include=''*.cs'' ErsatzTV ErsatzTV.Core ErsatzTV.Application ErsatzTV.Infrastructure ErsatzTV.Scanner | grep -v ''/obj/\|/bin/'' | grep -c ''''`, 2026-08-22), so a grep for it would be noise — the eight-column population is small enough to sweep by field instead.'
|
||||
---
|
||||
|
||||
**Deriving this population from the domain classes gives the wrong answer.** "The `IList<string>`
|
||||
properties under `ErsatzTV.Core/Domain`" is the derivation that looks obviously right, and it is
|
||||
REJECTED: it returns two of the eight. It is blind to the six value-converted collections, which are
|
||||
declared as ordinary `ICollection<int>` / `ICollection<DayOfWeek>` and become single columns only in
|
||||
`ErsatzTV.Infrastructure/Data/Configurations`, and blind to enum collections entirely. The
|
||||
authoritative source is the model configuration — `HasConversion<*CollectionValueConverter, …>` plus
|
||||
EF's native primitive-collection mapping. This is `testing.guard-derives-population-from-source`
|
||||
applied to a sweep rather than to a guard.
|
||||
|
||||
**The idiom is right almost everywhere it appears, which is what makes this hard to see.** `??= []`
|
||||
on `metadata.Genres`, `metadata.Tags`, `metadata.Artwork` and their kin is harmless: those are
|
||||
navigation collections, and setting a null navigation to an empty list is not a scalar property
|
||||
change, so EF has nothing to persist. The reader who wrote `metadata.Artists ??= []` two lines below
|
||||
`metadata.Genres ??= []` was following the surrounding code correctly. The difference is invisible at
|
||||
the call site and lives in the model: `Artists` is a primitive collection — one column holding the
|
||||
whole list.
|
||||
|
||||
**Why the "it is `AsNoTracking` today" argument is banned rather than merely weak.** Both feeds into
|
||||
the search indexer — `SearchRepository.GetItemToIndex` and `SearchRepository.GetAllSongs` — are
|
||||
`AsNoTracking`, so no shipped caller loses data, and that was true when #691 looked at it too. It is
|
||||
a fact about two callers. Nothing in the indexer requires it, nothing tests for it, and a future
|
||||
caller that drops `AsNoTracking` to reuse an existing context reintroduces silent data loss with no
|
||||
diff anywhere near the indexer. Writing the observation down as a justification is what converts a
|
||||
latent bug into a checked decision that talks the next reader out of verifying.
|
||||
|
||||
**The measurement, so it is not re-argued.** Restoring only the `??= []` clause — the real predecessor
|
||||
lines, not a hand-written mutant — and re-running the fixture reports
|
||||
`metadata.Artists should be null but was []`, and stops there: the first assertion short-circuits.
|
||||
The persistence half needs a probe VARIANT with assertions 1 and 2 replaced by prints, which reports
|
||||
`STATE=Modified` and the raw column moving from `NULL` to `"[]"`. Both halves were executed. The
|
||||
recipe is spelled out to that level of detail because the short version is not runnable as stated:
|
||||
following it produces only the first failure, which reads as the record overstating itself.
|
||||
|
||||
**The fixture's two anti-vacuity guards.** A POSITIVE CONTROL (`writer.NumDocs.ShouldBe(1)`) is
|
||||
required because every other assertion says something did NOT happen, so all of them hold vacuously
|
||||
if `UpdateSong` never runs. Measured: gate `UpdateItems` on `_initialized` — which this fixture
|
||||
bypasses by injecting the writer, so it is a plausible refactor — and with the control removed the
|
||||
test PASSES with the code under test unreachable. Separately, the fixture fails loudly if
|
||||
`UpdateSong` throws, because that method wraps its whole body in a `catch` that assigns
|
||||
`metadata.Song = null` — which severs a required relationship and cascades the metadata to
|
||||
`Deleted`. Without that check a probe silently measures the error path and reports the wrong cause,
|
||||
and the raw-column helper likewise fails on a MISSING row, since `ExecuteScalar` returns CLR null
|
||||
both for a NULL column and for no such row.
|
||||
|
||||
**Removing the assignment is not sufficient on its own.** The `??= []` was load-bearing for the four
|
||||
reads below it (`foreach (string artist in metadata.Artists)`, `metadata.Artists.ToList()`). Deleting
|
||||
it alone converts a silent write into a live throw on every untagged song — measured, by deleting
|
||||
only those two lines from the real predecessor file: `NullReferenceException`, thrown at the
|
||||
`foreach`. (Cited by SYMBOL deliberately: a line number in a mutant that exists in no committed tree
|
||||
is unreproducible by construction.) The exception type follows the read FORM, not the field:
|
||||
`foreach` yields NRE, `string.Join`/`ToList` yield `ArgumentNullException`, and this PR contains two
|
||||
of each. That is the same trap #691 hit from the other direction, and it is why the rule pairs the
|
||||
removal with the read-site guard rather than stating them separately.
|
||||
|
||||
Related: `api.selection-projection-include-chain` (#671) records the read-site guard itself and the
|
||||
sweep-by-FIELD instruction; this record covers the write half it does not address.
|
||||
@@ -0,0 +1,48 @@
|
||||
---
|
||||
key: release.verdict-writes-status-before-comment
|
||||
title: '2026-08-22 — the verdict STATUS is written before the verdict COMMENT, so the only reachable half-state is the safe one (#792)'
|
||||
status: active
|
||||
since: '2026-08-22'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: '`scripts/post-review-verdict.sh` writes the sha-bound `review-verdict/h10` commit status FIRST and the human-readable `Review-verdict:` comment SECOND. Every refusal path still refuses (fail-closed, unchanged) and exits non-zero, and none of them may leave a verdict comment behind. An orphaned comment is therefore PREVENTED rather than tolerated. If the comment write fails after the status was written, that is an error too, but it degrades to an `ask` at the merge gate rather than to an apparent grant.'
|
||||
signals: 'verdict comment without a status, half-posted verdict, orphaned Review-verdict comment, exit code lies, post-review-verdict exits 0 · paths: `scripts/post-review-verdict.sh`, `scripts/tests/test_post_review_verdict.py`, `.claude/hooks/pretooluse-merge-consent.sh` · issues: #792, #622, #632, #778'
|
||||
mechanics: '`scripts/tests/test_post_review_verdict.py::test_the_status_is_written_BEFORE_the_comment`, `::test_no_refusal_leaves_a_VERDICT_COMMENT_standing_in_for_the_status` (every refusal mode), `::test_every_path_that_writes_NO_STATUS_exits_non_zero`, `::test_a_failed_COMMENT_after_a_written_status_is_still_an_error`'
|
||||
---
|
||||
|
||||
The script has two writes and they are not equal. The **status** is the gate — a required context on
|
||||
`main`, bound to one sha. The **comment** is the artifact a human reads, and the merge hook's
|
||||
condition (c). Writing the comment first meant that every refusal between the two writes left a PR
|
||||
carrying `Review-verdict: MERGEABLE @ <head>` with no status behind it: an artifact that reads as
|
||||
granted consent, produced by the very run that refused to grant it. The refusals are correct and are
|
||||
not what changed (`ci.verdict-write-retarget-fence` — the fence must keep refusing when it cannot
|
||||
bind safely); what changed is which write survives a partial failure.
|
||||
|
||||
Ordering settles it without a rollback, and rollback is the option not taken: deleting or annotating
|
||||
the orphaned comment needs a Gitea call, and the refusals it would compensate for are frequently
|
||||
*caused* by Gitea being unreachable, so the compensating write is unavailable exactly when it is
|
||||
needed. Ordering costs nothing and cannot fail to apply.
|
||||
|
||||
The two surviving half-states are asymmetric, and that asymmetry is the whole justification:
|
||||
|
||||
- comment, no status → the hook's condition (c) classifies a positive verdict, the operator sees
|
||||
consent, and only the required check stands between that and a merge. Fail-open in appearance.
|
||||
- status, no comment → the hook has no verdict for this head to classify, which it resolves as
|
||||
**ask**. Fail-closed, visible, and cured by re-running the command. The qualification, because
|
||||
the hook reads the whole comment history rather than this run's write: if a positive verdict for
|
||||
the SAME head already exists, condition (c) is satisfied by it and the hook may grant — which is
|
||||
correct, since that comment covers this exact sha. A stale or negative verdict yields deny. So
|
||||
`ask` is the outcome when the current head has no verdict comment, and nothing here can produce a
|
||||
grant over a head no one reviewed.
|
||||
|
||||
**#792's premise about the exit code was wrong, and is corrected rather than repeated.** The issue
|
||||
reported the script printing its refusal and exiting 0. Re-measured on the tree that carries #632's
|
||||
fence, every no-status path exits NON-ZERO (`die` exits 1, the usage path 2, a failing `jq` its own
|
||||
status, a signal 128+n) — eight refusal modes are now driven through the real entry point
|
||||
and asserted, and those assertions pass against the predecessor as well, which is how we know the
|
||||
defect was never in the script. The observed 0 came from the invocation around it (a pipeline reports
|
||||
its last command's status, not the script's). The exit-code contract is asserted anyway: it was true
|
||||
by convention, held by one shared `die` helper, and nothing had ever executed it.
|
||||
|
||||
Read with `release.verdict-status-check` (why the status, not the comment, is the gate) and
|
||||
`release.review-verdict-gate` (the comment convention itself).
|
||||
@@ -5,8 +5,8 @@ status: active
|
||||
since: '2026-08-21'
|
||||
supersedes: none
|
||||
superseded-by: none
|
||||
rule: 'Any path that writes a WHOLE entity or a WHOLE child collection — a PUT-replace handler, a hand-built request object, a test comparer standing in for one — derives its field list from the authoritative type and asserts SET EQUALITY against it, rather than enumerating the fields by hand. A hand-written list is correct on the day it is written and structurally unable to report the day it stops being: the field that drifts is the one nobody wrote a line for, so no amount of care in the existing lines can reach it. The failure is silent by construction — a full replace with a field omitted returns HTTP 200 and destroys that field''s value (#754 drifted from a 28-property DTO by one and cleared it; the symptom arrived hours later as missing pixels). SECOND CLAUSE, separable from the first: where a replaced child row carries state keyed to its identity — progression, ordering, an enumerator position — the handler RECONCILES BY ID rather than delete-and-reinsert, because reinsertion silently resets state a client never asked to touch (#252: a schedule PUT reset fill-group progression; #500: a dedup fix became permanent data loss because the add filter and the remove filter used different keys, so the two halves must agree on the key). Delete-and-reinsert is acceptable ONLY where no such state exists, and that emptiness is a fact about today''s schema that a later feature can silently invalidate — so record it where the handler is, dated, rather than leaving it to be re-derived. The canonical worked example is `ToolCatalogTests.Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields`, which reads the accepted fields from the generated OpenAPI document and compares both directions.'
|
||||
signals: 'full replace asserts its field list · hand-maintained mirror drifts by one field · reconcile by id not delete and reinsert · 200 and the field is gone · add filter and remove filter must share a key · fill-group progression reset by a PUT · derive the comparer from the DTO · a lossless round-trip test that is itself a hand-copied list · anti-vacuity PIN not a floor on a reflective walk · stale exemption must still name a real property · a complete comparer over a hand-written fixture · universal negative in a record is a trap · paths: `ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemResponseRoundTripTests.cs`, `ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`, `ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs` · issues: #779, #773, #757, #754, #500, #252'
|
||||
rule: 'Any path that writes a WHOLE entity or a WHOLE child collection — a PUT-replace handler, a hand-built request object, a test comparer standing in for one — derives its field list from the authoritative type and asserts SET EQUALITY against it, rather than enumerating the fields by hand. A hand-written list is correct on the day it is written and structurally unable to report the day it stops being: the field that drifts is the one nobody wrote a line for, so no amount of care in the existing lines can reach it. The failure is silent by construction — a full replace with a field omitted returns HTTP 200 and destroys that field''s value (#754 drifted from a 28-property DTO by one and cleared it; the symptom arrived hours later as missing pixels). SECOND CLAUSE, separable from the first: where a replaced child row carries state keyed to its identity — progression, ordering, an enumerator position — the handler RECONCILES BY ID rather than delete-and-reinsert, because reinsertion silently resets state a client never asked to touch (#252: a schedule PUT reset fill-group progression; #500: a dedup fix became permanent data loss because the add filter and the remove filter used different keys, so the two halves must agree on the key). Delete-and-reinsert is acceptable ONLY where no such state exists, and that emptiness is a fact about today''s schema that a later feature can silently invalidate — so record it where the handler is, dated, rather than leaving it to be re-derived. The canonical worked example is `ToolCatalogTests.Every_Write_Tool_Should_Declare_Exactly_Its_OpenApi_Request_Body_Fields`, which reads the accepted fields from the generated OpenAPI document and compares both directions. ON THE SPA SIDE the same rule is enforced by the TYPE SYSTEM rather than by a test: a full-replace body is built as `Complete<T>` (`web/src/api/completeRequest.ts`), a mapped type that makes every member of a generated request type required, so a builder that omits one fails `npm run typecheck`. Annotate BOTH the API-wrapper parameter (so later callers inherit it) AND each construction site including every `.map` callback return type, because the excess-property check that catches a PHANTOM field fires only on a fresh literal in a contextually typed position and a generic `.map` callback is not one. Do not infer from "most builders already typecheck" that the gap is closed: a member is omittable exactly when it is absent from the schema `required` array in the ASP.NET-produced OpenAPI document, and two live cases (#807) sat unchecked inside a large majority of checked ones.'
|
||||
signals: 'full replace asserts its field list · hand-maintained mirror drifts by one field · reconcile by id not delete and reinsert · 200 and the field is gone · add filter and remove filter must share a key · fill-group progression reset by a PUT · derive the comparer from the DTO · a lossless round-trip test that is itself a hand-copied list · anti-vacuity PIN not a floor on a reflective walk · stale exemption must still name a real property · a complete comparer over a hand-written fixture · universal negative in a record is a trap · the SPA builds a full-replace body as `Complete<T>` · a member is omittable only when it is absent from the OpenAPI `required` array · excess-property checking does not fire inside a generic `.map` callback · a comment warning about a silent reset is not a check · `weight` · `qsvPreferNativeDecoder` · paths: `ErsatzTV.Tests/Application/ProgramSchedules/ScheduleItemResponseRoundTripTests.cs`, `ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`, `ErsatzTV.Application/ProgramSchedules/Commands/ReplaceProgramScheduleItemsHandler.cs`, `web/src/api/completeRequest.ts`, `web/src/api/completeRequest.guard.test.ts` · issues: #807, #779, #773, #757, #754, #500, #252'
|
||||
mechanics: 'Detector G of `docs/defect-shapes-773.md` §4. Enforced per-site by a reflective comparison over the DTO plus a written-down count pin, not by a repo-wide check — see the record body for why a global one is not proposed. The exemption set is empty today; the machinery guarding exemptions remains for the first one that earns its place.'
|
||||
---
|
||||
|
||||
@@ -130,9 +130,128 @@ be a name matcher over handler classes ending `Replace…Handler`, which misses
|
||||
and flags the ones that do it correctly. Per-site derivation, applied when a full-replace path is
|
||||
touched, is the enforceable version.
|
||||
|
||||
**Residue, tracked rather than implied closed.** The SPA builds request objects field-by-field in six
|
||||
screens (`normalizeForSave` and its equivalents). A *required* field omitted there fails `tsc`; an
|
||||
*optional* field added to the DTO and not carried through compiles clean and drops silently — #754's
|
||||
mechanism narrowed to optional fields, with no equivalent of the MCP write-tool test on the SPA side.
|
||||
That is the highest-probability recurrence in the repo today and is filed separately rather than
|
||||
folded in here.
|
||||
**The SPA half.** The residue this record recorded was that the SPA builds request objects
|
||||
field-by-field in six screens (`normalizeForSave` and its equivalents), so an *optional* field added
|
||||
to a DTO and not carried through would compile clean and drop silently.
|
||||
|
||||
**The current statement.** A member is omittable exactly when it sits outside its schema's
|
||||
`required` array; nullable properties usually emit as required-and-nullable (`"name": null | string`),
|
||||
so most builders are checked and the gap reads as closed on inspection. It is not. Two full-replace
|
||||
PUTs carried a live optional member (verified by execution 2026-08-22, by deleting the field and
|
||||
watching `npm run typecheck` stay CLEAN):
|
||||
|
||||
| schema | optional member | wrapper | consequence of the drop |
|
||||
|---|---|---|---|
|
||||
| `MultiCollectionItemRequest` | `weight` | `updateMultiCollection` (PUT) | every weight resets to 1 on save |
|
||||
| `UpdateFFmpegProfileRequest` | `qsvPreferNativeDecoder` | `updateFFmpegProfile` (PUT) | the setting reverts to its default |
|
||||
|
||||
**The framing this replaced, recorded so it is not re-adopted**: that the mechanism was *latent* —
|
||||
true of the request schemas the six named builders target, which were the ones checked, and
|
||||
reported as a property of the SPA. (No count is given: the earlier drafts carried one, and no
|
||||
natural cut of "the schemas those builders target" reproduces it.) A
|
||||
conclusion verified across the cases examined and then stated about the whole is the shape this
|
||||
corpus keeps recording, and it read as checked. `MultiCollectionsScreen` carries a prose comment
|
||||
warning about precisely that weight reset; a comment is not a check, and neither is a boundary
|
||||
drawn around the sample.
|
||||
|
||||
**`Complete<T>`** (`web/src/api/completeRequest.ts`) maps a request type so every member is
|
||||
required, making an omission a hard error however the schema was modelled. THE RULE is to apply it
|
||||
in two places: at the full-replace API-wrapper boundary, so later callers inherit it without knowing
|
||||
it exists, and at each construction site, which is what keeps TypeScript's excess-property check
|
||||
(the *phantom* direction) alive.
|
||||
|
||||
That is the rule, not a claim about how much of the tree currently follows it. Nothing enforces the
|
||||
second half — #820 — so any sentence here asserting present coverage would be falsifiable by one
|
||||
`tsc` run and would go stale on the next screen anyone adds. Two wrappers annotated under #807
|
||||
initially shipped with unannotated construction sites for exactly that reason. To find out what is
|
||||
actually annotated, read the code; do not read a count here.
|
||||
|
||||
**Why the second half of the rule is the one that gets skipped.** The wrapper annotation catches a
|
||||
MISSING member anywhere. The PHANTOM direction — a field the schema does not accept — relies on
|
||||
TypeScript's excess-property check, and that fires only on a fresh object literal in a contextually
|
||||
typed position. A literal returned from a generic `.map` callback is not one, because `map<U>`
|
||||
infers `U` from the callback rather than from the target element type. Measured on the pre-#807 tree by injecting a
|
||||
phantom property at each construction site then present: some rejected it and some accepted it,
|
||||
and among those that accepted, three contained neither a spread nor an inferred local — so "it has
|
||||
no spread" is not a reason to think a site is checked. No denominator is given: what counts as a
|
||||
"construction site" is not derived from anything, the figure was already wrong once on this branch,
|
||||
and the transferable finding is the mechanism, not the tally.
|
||||
|
||||
Measured, not argued (2026-08-22): injecting an optional member into `ScheduleItemRequest` reddens
|
||||
`normalizeForSave` with `TS2741` under `Complete<T>` and compiles clean without it; deleting
|
||||
`weight` from `MultiCollectionsScreen.toItemRequest` now reddens and previously did not. The proof
|
||||
ships as `@ts-expect-error` cases in `web/src/api/completeRequest.guard.test.ts`, re-executed on
|
||||
every `npm run typecheck` (a marked CI step).
|
||||
|
||||
**The boundary is DERIVED, and the two attempts to write it by hand are why.** The rule this
|
||||
record states — derive the population, never enumerate it — took two rounds to apply to the record's
|
||||
own coverage boundary, each time failing the same way: a schema sorted on its NAME rather than on
|
||||
what its endpoint does.
|
||||
|
||||
| round | the hand-written form | what it missed |
|
||||
|---|---|---|
|
||||
| 1 | a prose sentence exempting "create/update" | `updateMultiCollection` and `updateFFmpegProfile` are full replaces — both were LIVE silent drops |
|
||||
| 2 | a table, written to replace that sentence | `ArtworkContentTypeModel`, because `…Model` reads as a response model. It is reachable from the full-replace `PUT /channels/{id}` |
|
||||
|
||||
Two misses from one mechanism, so the mechanism went instead of the list getting a third patch.
|
||||
`scripts/tests/test_optional_request_members.py` now DERIVES the population every run from
|
||||
`ErsatzTV/wwwroot/openapi/v1.json` — every schema carrying a property outside its `required` array
|
||||
that is **transitively** reachable from ANY operation's request body, plus request bodies declared
|
||||
inline rather than by `$ref` — and asserts set equality in both directions against a registry of
|
||||
per-schema dispositions. Its own reach is bounded by what its composition resolver handles, and the
|
||||
resolver's branches are pinned by constructed-schema tests rather than by the one shape today's
|
||||
document happens to contain; read that file for the current boundary rather than a summary here. Transitivity is load-bearing:
|
||||
`MultiCollectionItemRequest` and `ArtworkContentTypeModel` are both nested, so a check reading only
|
||||
top-level bodies would have reproduced both misses.
|
||||
|
||||
The split follows `testing.guard-derives-population-from-source`: the POPULATION is derived, the
|
||||
DISPOSITIONS are the SCOPE — a reviewed policy choice per schema, legitimately hand-written, and
|
||||
forced to exist by the equality assertion. A new optional member in a named component schema
|
||||
reachable from a request body, or in an inline request body, now fails that test until someone
|
||||
writes down what should happen about it. The dispositions themselves
|
||||
live in that file rather than here, so there is one copy.
|
||||
|
||||
**On this record's own pin-vs-floor argument, applied to that guard.** The `Id` discussion above
|
||||
argues that an anti-vacuity check must be a PIN and not a `>=` floor, because a floor lets members
|
||||
vanish silently. `test_optional_request_members.py` uses floors, and that is not a quiet exception:
|
||||
a pin on "how many schemas are reachable from a request body" would be a pin on the size of the
|
||||
whole API, red on every unrelated endpoint added. What replaces the pin's strength is the
|
||||
both-directions set equality — a schema that vanishes from the population reports as PHANTOM, which
|
||||
is exactly what the pin existed to catch — plus a planted-member test that is red whenever the walk
|
||||
stops seeing nested or `oneOf`-referenced schemas. Measured 2026-08-22: the floors alone do NOT
|
||||
catch a partially broken walk (deleting the transitive step leaves them satisfied); the planted
|
||||
test and the equality assertion do. The floors are the crude backstop against a parse that reached
|
||||
nothing at all, and the guard's docstring says so rather than letting them read as coverage.
|
||||
|
||||
**One disposition is worth stating here because it bounds this record's own rule.** `Complete<T>`
|
||||
must NOT be applied to a schema whose optional members are computed get-only properties:
|
||||
`ArtworkContentTypeModel`'s `IsExternalUrl` / `HasContentType` / `UrlWithContentType` are derived
|
||||
from `Path` and are never deserialized, so a client omitting them drops nothing — while annotating
|
||||
the site would force a caller to fabricate server-computed values in an outbound request. The test
|
||||
that decides the column is **does this write replace a whole entity or collection, and can the
|
||||
member actually carry a stored value**, not whether the wrapper is named `update…` or `replace…`.
|
||||
|
||||
**The wrapper-boundary rationale is a DIRECTION, not a claim about today's coverage.** Some
|
||||
full-replace PUT wrappers in `web/src/api/*.ts` carry `Complete<>` — the ones reachable from a
|
||||
schema that can drop a member, plus the six builders #807 set out to fix — and most do not. No
|
||||
figure is given here deliberately, and neither is a characterisation of WHICH ones: nothing
|
||||
derives either, and a hand-maintained description of a code population is the thing this record
|
||||
argues against. (An earlier draft said "the ones reachable from a schema that can drop a member,
|
||||
plus the six builders #807 set out to fix" — five annotated wrappers fall outside both sets. A set
|
||||
phrased in words rots exactly like a count.) No reproduction command is offered either: isolating
|
||||
"full-replace PUT wrappers" from a grep needs the judgement about intent this record already says a
|
||||
matcher cannot make. For the unannotated wrappers the protection is still the
|
||||
contingent kind this record complains about: it holds only while their properties stay inside
|
||||
`required`. What makes that survivable rather than a re-run of the same mistake is that the
|
||||
contingency is now MONITORED — the derived test above reddens the moment one of them acquires an
|
||||
optional member — instead of being assumed. Annotating the rest is cheap and should happen when
|
||||
each is next touched.
|
||||
|
||||
**Residue, named rather than implied closed.** `Complete<T>` proves its own semantics but NOT that
|
||||
it is applied: reverting one screen to its pre-#807 form leaves the guard green, because the
|
||||
population of construction sites is derived from nothing. The repo already owns the tool for that
|
||||
— `web/src/api/pageSizeScan.ts` (TypeScript compiler API) plus a registry cross-checked in both
|
||||
directions — and building it is tracked in #820. Until then this convention is enforced by review,
|
||||
which is the weaker thing this record exists to warn about. Separately: `Complete<T>` is shallow,
|
||||
so a new nested item request type needs its own annotation; and requiring a field to be *named* is
|
||||
not the same as requiring it to be *populated* correctly.
|
||||
|
||||
@@ -56,8 +56,11 @@ inventory shipped with were sitting in the gap:
|
||||
1. **Guards inline in workflow YAML** — most importantly `pr-checks.yml:ci-image-pin`. "Which jobs
|
||||
are guards" needs a judgement call per job the filesystem cannot supply. Two were audited under
|
||||
#774 and one fixed; extending the population is tracked in #786.
|
||||
2. **C# and TypeScript guards** — `ErsatzTV.Mcp.Tests/ToolCatalogTests.cs` and
|
||||
`web/src/api/pageSizeCallSites.guard.test.ts` are both structural guards and neither has a row.
|
||||
2. **C# and TypeScript guards** — `ErsatzTV.Mcp.Tests/ToolCatalogTests.cs`,
|
||||
`web/src/api/pageSizeCallSites.guard.test.ts` and `web/src/api/completeRequest.guard.test.ts`
|
||||
are all structural guards and none has a row. Note what that costs: this list is a HAND-WRITTEN
|
||||
mirror of a population nothing derives, so it goes stale silently and CI stays green — #807
|
||||
added the third entry, and only review caught that the second had become the only one named.
|
||||
3. **Mentions counted as call sites.** The `scripts/…` scrape matches any occurrence, including
|
||||
inside a comment or an `::error::` string. `scripts/update-openapi.sh` is named in a
|
||||
`pr-checks.yml` error message, so removing the step that runs it would leave its row intact.
|
||||
@@ -178,6 +181,7 @@ recorded as unexamined rather than as cleared.
|
||||
| `scripts/ci-peak-anon.sh` | nothing (samples container memory) | TOOLING | NONE | — |
|
||||
| `scripts/ci-prove-ban-detects.sh` | the release path, if the delimiter ban is disarmed | GUARD | NONE | — |
|
||||
| `scripts/ci-step-ran.sh` | the two required contexts, on a dropped step | GUARD | MUTATION | `test_ci_dropped_step_guard.py::test_dropping_ANY_single_step_FAILS_the_guard` |
|
||||
| `scripts/ci-toolchain-image-resolves.sh` | the `toolchain-preflight` job, when the pinned CI toolchain image has been deleted from the registry | GUARD | MUTATION | `test_ci_toolchain_image_resolves.py::test_MUTATION_a_deleted_tag_is_reported_as_a_failure` |
|
||||
| `scripts/decisions_validate.py` | the `decisions-guard` job, on a lifecycle fault | GUARD | MUTATION | `test_decisions_validate.py::test_main_actually_CALLS_the_wing_scan` |
|
||||
| `scripts/e2e-functional.sh` | the Functional E2E job, on a failed HTTP contract assertion | GUARD | NONE | — |
|
||||
| `scripts/e2e-local.sh` | nothing (boots a local instance) | TOOLING | NONE | — |
|
||||
@@ -196,6 +200,7 @@ recorded as unexamined rather than as cleared.
|
||||
| `scripts/tests/test_ci_dropped_step_guard.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_ci_image_pin_population.py` | the `script-tests` job, when a container job loses its pin | GUARD | MUTATION | `test_ci_image_pin_population.py::test_a_single_job_losing_its_pin_is_DETECTED` |
|
||||
| `scripts/tests/test_ci_release_path_scan_job.py` | the `script-tests` job, on a weakened release-path scan job | GUARD | NONE | — |
|
||||
| `scripts/tests/test_ci_toolchain_image_resolves.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_decisions_lib.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_decisions_validate.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_guard_inventory.py` | the `script-tests` job, on an unclassified guard or a stale proof ref | GUARD | MUTATION | `test_guard_inventory.py::test_the_inventory_covers_exactly_the_guards_that_exist` |
|
||||
@@ -211,6 +216,7 @@ recorded as unexamined rather than as cleared.
|
||||
| `scripts/tests/test_prepush_unsets_git_env.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_pr_changed_files.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_prepush_rebase_check_tag_exemption.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_optional_request_members.py` | the `script-tests` job, on an OpenAPI request schema that can silently drop a member with no stated disposition | GUARD | MUTATION | `test_optional_request_members.py::test_every_droppable_request_schema_has_a_stated_disposition` |
|
||||
| `scripts/tests/test_prove_fix.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
| `scripts/tests/test_remote_state_inventory.py` | the `script-tests` job, on an executable that talks to a remote service with no row in `docs/remote-state-inventory.md` | GUARD | MUTATION | `test_remote_state_inventory.py::test_every_in_scope_file_has_a_row_and_every_row_names_a_real_file` |
|
||||
| `scripts/tests/test_worktree_ownership_guard.py` | the `script-tests` job | PROOF | NONE | — |
|
||||
@@ -235,7 +241,7 @@ clause-provable and the entry is regraded.
|
||||
|
||||
## What the numbers say
|
||||
|
||||
36 guards, 6 tooling scripts, 19 proof files. **14 guards carry a mutation proof; 6 are
|
||||
38 guards, 6 tooling scripts, 20 proof files. **16 guards carry a mutation proof; 6 are
|
||||
behaviour-only; 16 have none.** These figures are asserted against the table by
|
||||
`test_the_summary_counts_match_the_table` — they were wrong in the first draft (28/4/6/3/19 against
|
||||
a table holding 27/5/6/3/18), because a hand-maintained summary of a table is a second copy of it,
|
||||
|
||||
@@ -95,7 +95,7 @@ classifications differ; otherwise the strictest applies and the Note names the e
|
||||
|
||||
| Site | Class | Note |
|
||||
|---|---|---|
|
||||
| `scripts/post-review-verdict.sh` — commit-status write | `PINNED` | Re-reads the PR and compares **both** `.head.sha` and `.base.ref` immediately before the POST, and `die`s (exit 1, no status written) on a mismatch **or on a field it cannot read**. That last clause is new: both comparisons were guarded by `[ -n "$x" ] &&`, so a well-formed 2xx body that merely omitted the field made the check a no-op and the status was posted having confirmed nothing — found by cold review on #778 and regression-tested against the real predecessor, since the redundant `-z` arm alone mutates green. Closes #706 and #632 for this path by read-compare-refuse, not by CAS: Gitea's status API offers no conditional write. Residual: the **comment** is posted *before* the re-read, so a head that moves in between leaves a verdict comment with no status — the comment is not the gate, but the mismatch is confusing and is tracked in **#792**. |
|
||||
| `scripts/post-review-verdict.sh` — commit-status write | `PINNED` | Re-reads the PR and compares **both** `.head.sha` and `.base.ref` immediately before the POST, and `die`s (exit 1, no status written) on a mismatch **or on a field it cannot read**. That last clause is new: both comparisons were guarded by `[ -n "$x" ] &&`, so a well-formed 2xx body that merely omitted the field made the check a no-op and the status was posted having confirmed nothing — found by cold review on #778 and regression-tested against the real predecessor, since the redundant `-z` arm alone mutates green. Closes #706 and #632 for this path by read-compare-refuse, not by CAS: Gitea's status API offers no conditional write. Residual closed on the write ORDER since #792: the status is written first and the comment second, so a refusal can no longer leave a verdict comment with no status behind it — the only reachable half-state is a status with no comment, which the merge hook resolves as `ask` (`release.verdict-writes-status-before-comment`). |
|
||||
| `scripts/pr-changed-files.sh` — paged file enumeration | `UNSAFE-KNOWN` | #707's fix, graded honestly after cold review: `.base.ref`, `.base.sha` and `.head.sha` are captured before paging and re-checked after, and any *observed* movement fails the whole enumeration closed rather than emitting a short list. But before-and-after equality is **ABA-vulnerable** — a `main → scratch → main` retarget during paging can return the same ref and, if nothing merged meanwhile, the same base sha, while the pages in between were diffed against the scratch base. The script's own comment says it narrows rather than erases; this row previously said "any movement fails", which was stronger than the code. Accepted here because the enumerator cannot close it alone, but be exact about what the caller-side fence does and does not cover: `ci.verdict-write-retarget-fence` counts `change_target_branch` events, so it catches the BASE alias and **nothing else**. A HEAD alias is not covered by anything — a force-push `H1 -> H2 -> H1` during pagination leaves the final `.head.sha` comparison equal while the middle pages were enumerated against `H2`, and no counter moves. That residual is real, unfenced, and stated here rather than papered over; closing it needs a monotonic head-mutation fence or enumeration bound to an immutable tree, neither of which exists today — tracked in **#803**, which also carries the three older contracts that still assert more than this row does. |
|
||||
| `scripts/select-queue.sh` — issue list, then per-issue `/dependencies` | `UNSAFE-KNOWN` | The open-issue list (labels, milestone, priority) is snapshotted once; per-candidate dependency reads happen seconds later and never re-read the issue's own labels, so an issue claimed `in-progress` in that gap still appears on the shortlist. Accepted: the script authorizes **no write**. The real gate is the four-way claim check in `process.parallel-session-claim`, which runs after selection and re-reads live state by construction. Tightening this would move a check that must be adversarial into a tool that is advisory. |
|
||||
| `scripts/ci-detect-already-validated.sh` — prior-head combined status | `UNSAFE-KNOWN` | Reads the PR head's status and emits `skip=true`, with nothing re-checking before the consuming job runs. Accepted and narrow: the skip elides only **re-running** test/migrations on a tree already validated; the `build` job still builds and pushes unconditionally, so no image ever ships from unvalidated source. |
|
||||
@@ -128,6 +128,7 @@ classifications differ; otherwise the strictest applies and the Note names the e
|
||||
| `scripts/ci-peak-anon.sh` | `N/A` | Reads no live remote state — samples the runner's local cgroup `memory.stat`/`memory.peak`. |
|
||||
| `scripts/ci-prove-ban-detects.sh` | `N/A` | Reads no live remote state — mutates a local workflow copy and runs pytest against the local checkout. |
|
||||
| `scripts/ci-step-ran.sh` | `N/A` | Reads no live remote state — reads runner-supplied env vars and local marker files it wrote itself. |
|
||||
| `scripts/ci-toolchain-image-resolves.sh` — registry manifest read for the pinned toolchain tag | `UNSAFE-KNOWN` | Reads a MUTABLE identifier (a registry tag) with nothing re-checking it before the `container:` jobs pull, so a tag deleted between the preflight and the pull is reported as present. Graded `UNSAFE-KNOWN` rather than `N/A` deliberately: nothing proceeds on the strength of the read — it can only turn its own job red, which is not nothing (the merge-consent hook denies on the COMBINED status, ersatztv#598) but is not authorization either — while a stale PASS is read by a human as "the image is fine", which is an assertion about remote state this file exists to grade. The residual is bounded by what it degrades to: a stale pass leaves exactly the pre-#772 behaviour (five jobs failing at pull), never anything that proceeds on the strength of the read. The opposite error is closed by the EXIT CODE rather than by wording: an unusable credential, an unverifiable answer (after retries) and an HTTP 200 whose body is not a manifest all FAIL the job. THE TRAP, since warning on those and exiting 0 is the natural way to write this check: a missing `curl`, a moved registry or a DNS change all land there, and a green-with-a-warning job is indistinguishable from a healthy pin forever after — "the check could not run" presenting as "the pin is fine", which is precisely what this row would then be asserting falsely. |
|
||||
| `scripts/set-provider.sh` | `N/A` | Reads no live remote state — sets local `dotnet user-secrets` values. |
|
||||
| `scripts/__init__.py` | `N/A` | Empty package marker — executes nothing. |
|
||||
| `scripts/scripted-schedules/entrypoint.py` — `ScriptedScheduleApi.get_context(build_id)`, then `define_content` / `reset_playout` / `build_playout` against the same live server | `UNSAFE-KNOWN` | A genuine read-then-act over live ErsatzTV state, and the row cold review found missing when the population was still non-recursive. The context is fetched, handed to user-supplied script functions that mutate the playout, and re-fetched after a reset with nothing pinning either read — a concurrent build or edit between them is invisible. Accepted because it runs inside a single scripted-schedule build the server itself serialises per playout, and because the API exposes no version or ETag on the context to compare against; the honest bound is that the blast radius is one playout's content, reversible by rebuilding. |
|
||||
|
||||
@@ -446,6 +446,52 @@ fresher edit:
|
||||
(authoritative) — never let an out-of-range value reach the server and surface a raw 400. Mirror the
|
||||
server's bound as a shared const (e.g. multi-collection weight `WEIGHT_MIN`/`WEIGHT_MAX` = 1..1000, #404).
|
||||
|
||||
## 4b. Full-replace request bodies are built as `Complete<T>` (#807)
|
||||
|
||||
A full-replace endpoint writes the WHOLE entity, so a field the builder never sets is not left
|
||||
alone — it is written as its default. Build every full-replace body against `Complete<T>`
|
||||
(`web/src/api/completeRequest.ts`), which maps a generated request type so **every** member is
|
||||
required:
|
||||
|
||||
```ts
|
||||
function toReplaceRequest(draft: Draft): Complete<ReplaceBlockRequest> { … }
|
||||
items.map((item): Complete<DecoTemplateItemRequest> => ({ … }))
|
||||
```
|
||||
|
||||
Two rules, and the second is the one that gets skipped:
|
||||
|
||||
- **Annotate the wrapper parameter** (`body: Complete<ReplaceBlockRequest>`) so every caller —
|
||||
including ones written later by someone who never read this — inherits the check.
|
||||
- **Annotate each construction site too, including every `.map` callback's return type.** The
|
||||
wrapper annotation catches a *missing* field anywhere. The *phantom* direction (a field the
|
||||
schema does not accept) relies on TypeScript's excess-property check, which fires only on a
|
||||
**fresh object literal in a contextually typed position** — and a literal returned from a generic
|
||||
`.map` callback is not one, because `map<U>` infers `U` from the callback's return rather than
|
||||
from the target element type. A spread or an inferred local loses it too, but the `.map` callback
|
||||
is the common case and the easy one to misread as safe: several construction sites accepted a
|
||||
phantom field before #807, and three of them contained neither a spread nor an inferred local.
|
||||
|
||||
An optional member may be written `field: undefined`. The point is not to forbid omitting a value,
|
||||
it is to forbid omitting the *decision*: an unmentioned field is an oversight, an explicit
|
||||
`undefined` is a choice a reviewer can see.
|
||||
|
||||
**Do NOT apply `Complete<T>` to a schema whose optional members are computed server-side.**
|
||||
`ArtworkContentTypeModel` (reachable from `PUT /channels/{id}` via `UpdateChannelRequest.logo`) has
|
||||
`isExternalUrl` / `hasContentType` / `urlWithContentType` as get-only properties derived from
|
||||
`path`. Nothing deserializes them, so omitting them drops nothing — and annotating the site would
|
||||
force you to invent server-computed values in an outbound request. Check the schema's disposition
|
||||
in `scripts/tests/test_optional_request_members.py` before annotating a new site.
|
||||
|
||||
Why this is needed even though most builders already typecheck: a member is omittable exactly when
|
||||
the generated type marks it `?:`, which comes from the `required` array of the schema in
|
||||
`ErsatzTV/wwwroot/openapi/v1.json` — the generator script only passes it through. Most nullable
|
||||
properties emit as required-and-nullable, so the gap looks closed on inspection while a minority
|
||||
sits unchecked inside it. Do not trust a list of which schemas those are: two hand-written ones
|
||||
were wrong (#807). `scripts/tests/test_optional_request_members.py` derives the set on every run
|
||||
and fails until each has a stated disposition. See
|
||||
`docs/decisions/records/testing/full-replace-asserts-field-list.md`, and
|
||||
`web/src/api/completeRequest.guard.test.ts` for the executed proof.
|
||||
|
||||
## 5. Artwork rendering
|
||||
|
||||
Render `item.artwork` / `item.poster` (or whatever the DTO field is named) **directly as an `<img
|
||||
|
||||
@@ -101,12 +101,14 @@ EOF
|
||||
# message naming the variable; that is loud, instantly diagnosable, and the correct direction for a
|
||||
# required check.
|
||||
#
|
||||
# EXCEPT ON A LANE WITH NO `container:` (ersatztv#767). The `scan` job runs on `small` with no
|
||||
# container, so RUNNER_TEMP is the shared host /tmp and the keying below is the ONLY thing separating
|
||||
# runs, not defence in depth on top of a fresh filesystem. There the rerun residual DOES still exist:
|
||||
# a single-job rerun that does not increment GITHUB_RUN_ATTEMPT would find the previous attempt's
|
||||
# marker file. See the carve-out in `ci.required-job-step-execution-markers`; do not read the
|
||||
# paragraph above as covering that job.
|
||||
# EXCEPT ON A LANE WITH NO `container:` (ersatztv#767, extended by #772). `scan` and
|
||||
# `toolchain-preflight` both run on `small` with no container, so RUNNER_TEMP is the shared host /tmp
|
||||
# and the keying below is the ONLY thing separating runs, not defence in depth on top of a fresh
|
||||
# filesystem. There the rerun residual DOES still exist: a single-job rerun that does not increment
|
||||
# GITHUB_RUN_ATTEMPT would find the previous attempt's marker file. See the carve-out in
|
||||
# `ci.required-job-step-execution-markers`; do not read the paragraph above as covering those jobs.
|
||||
# The list is the property to keep current — a THIRD container-free job added without appearing here
|
||||
# inherits a reassurance that was never checked for it.
|
||||
marker_path() {
|
||||
local missing=""
|
||||
[ -n "${GITHUB_JOB:-}" ] || missing="$missing GITHUB_JOB"
|
||||
|
||||
Executable
+149
@@ -0,0 +1,149 @@
|
||||
#!/usr/bin/env bash
|
||||
# Preflight: does the PINNED CI toolchain image still exist in the registry? (ersatztv#772)
|
||||
#
|
||||
# WHY THIS EXISTS. `docker-build.yml` pins its five `container:` jobs to an immutable
|
||||
# `ersatztv-ci:<sha>`. Between 2026-08-11 and 2026-08-13 that tag was deleted from the Gitea
|
||||
# registry and every one of those jobs — including BOTH required contexts — died after 1-2s with
|
||||
#
|
||||
# Error response from daemon: failed to resolve reference "…/ersatztv-ci:<the pinned sha>": not found
|
||||
#
|
||||
# buried in each job's log. Nothing said "your toolchain image is gone", so the natural first
|
||||
# reading was "my diff broke the build". This job says it in one line, in a job whose NAME says it.
|
||||
#
|
||||
# "Immutable" was taken to mean "will always exist", and those are different claims. The cause was
|
||||
# an owner-level Gitea package cleanup rule (keep_count 15, remove_days 1, remove_pattern `.*`, and
|
||||
# a keep_pattern no 7-hex sha can match), so a pinned tag is deleted once 15 newer versions of the
|
||||
# package exist. The rule lives in the registry's repo — the durable fix is
|
||||
# timothy/server-management#842 — and THIS script does not fix it. It converts a five-job pull
|
||||
# failure into one actionable message, which is all a consumer of someone else's registry can do.
|
||||
#
|
||||
# WHY IT DOES NOT GATE THE CONTAINER JOBS with `needs:`. Serialising five jobs behind a checkout +
|
||||
# one curl would tax every green run to speed up the rare red one, and the container jobs already
|
||||
# fail fast (1-2s) when the pull fails. This runs in PARALLEL: the diagnosis is present the moment
|
||||
# anyone looks, and the happy path pays nothing.
|
||||
#
|
||||
# UNKNOWN IS NOT A PASS, and this is where the first draft was wrong. It warned and exited 0 on
|
||||
# every answer that was not 200 or 404, which makes "curl is missing from this runner", "the
|
||||
# registry moved", and "DNS changed" all indistinguishable from a healthy pin — a job that is green
|
||||
# forever having checked nothing, in a file whose header claims the opposite. Unknown answers are
|
||||
# RETRIED (they are usually transient) and then FAIL. The message stays distinct from the deleted
|
||||
# case: "could not verify" and "IS GONE" send an operator to different places.
|
||||
#
|
||||
# Env (all optional except the credential; the defaults are the live values):
|
||||
# ETV_CI_REGISTRY registry host:port (default 192.168.1.95:3000)
|
||||
# ETV_CI_IMAGE_REPO package path inside the registry (default timothy/ersatztv-ci)
|
||||
# ETV_CI_WORKFLOW workflow file to read the pin from (default .gitea/workflows/docker-build.yml)
|
||||
# ETV_CI_ATTEMPTS tries per pin before an unknown becomes a failure (default 3)
|
||||
# ETV_CI_RETRY_SECONDS pause between those tries (default 5)
|
||||
# ETV_REGISTRY_AUTH user:pass — REQUIRED; the registry rejects anonymous reads with 401
|
||||
set -euo pipefail
|
||||
|
||||
registry="${ETV_CI_REGISTRY:-192.168.1.95:3000}"
|
||||
image_repo="${ETV_CI_IMAGE_REPO:-timothy/ersatztv-ci}"
|
||||
workflow="${ETV_CI_WORKFLOW:-.gitea/workflows/docker-build.yml}"
|
||||
|
||||
fail() { printf '::error::ci-toolchain-image-resolves: %s\n' "$*" >&2; exit 1; }
|
||||
|
||||
[ -f "$workflow" ] || fail "cannot read $workflow to find the toolchain pin"
|
||||
|
||||
# The same expression `pr-checks.yml::ci-image-pin` greps with, so the two cannot disagree about
|
||||
# what "the pin" is. Note it is written so THIS line cannot match itself: the character after the
|
||||
# colon here is `[`, which is not in [0-9a-f].
|
||||
pins=$(grep -oE 'ersatztv-ci:[0-9a-f]+' "$workflow" | cut -d: -f2 | sort -u || true)
|
||||
[ -n "$pins" ] || fail "no ersatztv-ci pin found in $workflow — if the grep pattern stopped matching, fix it here and in pr-checks.yml::ci-image-pin together"
|
||||
|
||||
# No credentials is NOT a pass. An unauthenticated read of this registry is a 401 for every tag,
|
||||
# present or deleted, so a run without them would report "cannot tell" for a live pin and for a
|
||||
# deleted one alike — the shape where a guard reports green having checked nothing.
|
||||
#
|
||||
# The EMPTY-halves check is the one that matters in CI and is easy to miss: an absent secret does
|
||||
# not arrive here as an unset variable. `ETV_REGISTRY_AUTH: ${{ secrets.REGISTRY_USER }}:${{ ... }}`
|
||||
# interpolates a missing secret to the empty string, so the job passes the non-empty string ":".
|
||||
# Testing only the unset case would leave the production shape uncovered.
|
||||
auth="${ETV_REGISTRY_AUTH:-}"
|
||||
[ -n "$auth" ] || fail "ETV_REGISTRY_AUTH (user:pass) is unset, so the registry cannot be queried — this check refuses to report a pass it did not establish"
|
||||
case "$auth" in
|
||||
*:*) ;;
|
||||
*) fail "ETV_REGISTRY_AUTH must be user:pass, got a value with no ':' — the registry cannot be queried and this check refuses to report a pass it did not establish" ;;
|
||||
esac
|
||||
[ -n "${auth%%:*}" ] && [ -n "${auth#*:}" ] \
|
||||
|| fail "ETV_REGISTRY_AUTH has an empty half (user or password) — this is what an ABSENT REGISTRY_USER/REGISTRY_PASSWORD secret interpolates to, not a credential. Fix the secrets rather than reading an unauthenticated 401 as could-not-tell."
|
||||
|
||||
accept='application/vnd.oci.image.manifest.v1+json,application/vnd.docker.distribution.manifest.v2+json,application/vnd.oci.image.index.v1+json,application/vnd.docker.distribution.manifest.list.v2+json'
|
||||
attempts="${ETV_CI_ATTEMPTS:-3}"
|
||||
retry_seconds="${ETV_CI_RETRY_SECONDS:-5}"
|
||||
rc=0
|
||||
|
||||
# One GET, returning "<code> <is-a-manifest>". The body is fetched rather than a HEAD sent, because
|
||||
# HTTP 200 alone does not mean "the manifest is there": a proxy, a captive login page or an error
|
||||
# document all answer 200 with a body that is not a manifest, and a check that reads only the status
|
||||
# line reports those as "resolves". A manifest always carries `schemaVersion`, so the body is matched
|
||||
# for it — with a shell `case`, so nothing depends on jq being installed and no pipeline can invert
|
||||
# the result on a large body.
|
||||
probe() {
|
||||
local url="$1" resp code body
|
||||
# `-u` puts the credential in argv, visible to `ps` for the length of the call — and this job has
|
||||
# no `container:`, so that is the shared host. Kept because it is the shape every other curl caller
|
||||
# in scripts/ already uses (`ci-detect-already-validated.sh`, `pr-changed-files.sh`,
|
||||
# `select-queue.sh`, `issue-qualification-audit.sh`): fixing one site would leave the class intact
|
||||
# and the codebase inconsistent. The class is tracked in ersatztv#821.
|
||||
resp=$(curl -s -w '\n%{http_code}' -u "$auth" -H "Accept: $accept" "$url") || resp=""
|
||||
[ -n "$resp" ] || { printf '000 no\n'; return 0; }
|
||||
code=${resp##*$'\n'}
|
||||
body=${resp%$'\n'*}
|
||||
case "$body" in
|
||||
*'"schemaVersion"'*) printf '%s yes\n' "$code" ;;
|
||||
*) printf '%s no\n' "$code" ;;
|
||||
esac
|
||||
}
|
||||
|
||||
for pin in $pins; do
|
||||
url="http://$registry/v2/$image_repo/manifests/$pin"
|
||||
attempt=1
|
||||
while : ; do
|
||||
read -r code is_manifest <<EOF
|
||||
$(probe "$url")
|
||||
EOF
|
||||
case "$code" in
|
||||
200|404|401|403) break ;;
|
||||
esac
|
||||
# Only the unknown answers are retried: 200/404 are answers, and an auth failure will not cure
|
||||
# itself. A transient registry is the common case for the rest, and absorbing it here is what
|
||||
# lets the unknown be a FAILURE at the end rather than a warning nobody reads.
|
||||
[ "$attempt" -lt "$attempts" ] || break
|
||||
attempt=$((attempt + 1))
|
||||
sleep "$retry_seconds"
|
||||
done
|
||||
|
||||
case "$code" in
|
||||
200)
|
||||
if [ "$is_manifest" = "yes" ]; then
|
||||
printf 'ci-toolchain-image-resolves: %s/%s:%s resolves (HTTP 200, manifest present)\n' "$registry" "$image_repo" "$pin"
|
||||
else
|
||||
printf '::error::ci-toolchain-image-resolves: %s/%s:%s answered HTTP 200 with a body that is not a manifest (no schemaVersion). Something is answering for the registry — a proxy, a login page, or an error document. The pin was NOT verified.\n' \
|
||||
"$registry" "$image_repo" "$pin" >&2
|
||||
rc=1
|
||||
fi
|
||||
;;
|
||||
404)
|
||||
# The one unambiguous answer, and the outage this exists for.
|
||||
printf '::error::ci-toolchain-image-resolves: the pinned CI toolchain image %s/%s:%s IS GONE from the registry (HTTP 404). Every container: job in docker-build.yml will fail at image pull, including both required contexts, and NO diff caused it. Recovery does not need CI: rebuild that exact tag from the commit it names and push it — see docs/ci-cd.md -> "CI toolchain image" -> "When the pinned tag disappears". Root cause + the durable fix: timothy/server-management#842.\n' \
|
||||
"$registry" "$image_repo" "$pin" >&2
|
||||
rc=1
|
||||
;;
|
||||
401|403)
|
||||
# `fail` rather than `rc=1`: unlike a 404, this says nothing about the pin, and it will say
|
||||
# the same thing about every remaining one. Abandoning the loop keeps the log to one cause.
|
||||
fail "the registry rejected these credentials (HTTP $code) for $registry/$image_repo:$pin, so the pin could not be checked. Fix REGISTRY_USER/REGISTRY_PASSWORD rather than reading this as a pass."
|
||||
;;
|
||||
*)
|
||||
# NOT gone, and NOT a pass either. Deliberately worded apart from the 404 message: this sends
|
||||
# an operator to the registry's health, not to a rebuild of a tag that may be sitting there.
|
||||
printf '::error::ci-toolchain-image-resolves: could NOT VERIFY %s/%s:%s after %s attempt(s) (last answer: HTTP %s). This is not evidence the image is gone — it is evidence the check could not run, which fails rather than passing so the preflight cannot quietly become a no-op.\n' \
|
||||
"$registry" "$image_repo" "$pin" "$attempts" "$code" >&2
|
||||
rc=1
|
||||
;;
|
||||
esac
|
||||
done
|
||||
|
||||
exit "$rc"
|
||||
@@ -119,21 +119,20 @@ short=${sha:0:7}
|
||||
base_ref=$(printf '%s' "$prjson" | jq -r '.base.ref // ""')
|
||||
[ -n "$base_ref" ] || die "PR #$pr has no resolvable base branch (.base.ref) — refusing to post a verdict that cannot record what it was formed against"
|
||||
|
||||
# --- The comment (human-readable artifact + the hook's condition-(c) input). --------------------
|
||||
# The verdict line MUST start the line: the hook anchors its parser to line-start precisely so a
|
||||
# comment that merely QUOTES the template mid-sentence cannot self-approve a merge.
|
||||
body="Review-verdict: $verdict @ $short"
|
||||
[ -n "$note" ] && body="$body"$'\n\n'"$note"
|
||||
comment_payload=$(jq -n --arg b "$body" '{body:$b}')
|
||||
api_post "repos/$owner/$repo/issues/$pr/comments" "$comment_payload" >/dev/null \
|
||||
|| die "failed to post the verdict comment on PR #$pr"
|
||||
printf 'posted comment: Review-verdict: %s @ %s\n' "$verdict" "$short"
|
||||
|
||||
# --- TOCTOU guard: refuse to green a head that stopped being head while we were posting. --------
|
||||
# --- TOCTOU guard: refuse to green a head that stopped being head since we read it. -------------
|
||||
# Without this, a commit pushed between the head read above and the status write below would inherit
|
||||
# a verdict written for its parent — reintroducing ersatztv#622 at a smaller time scale. We do NOT
|
||||
# retry against the new head: the new commit is genuinely unreviewed, and silently re-targeting the
|
||||
# verdict at it is exactly the failure this script exists to prevent.
|
||||
#
|
||||
# THE WINDOW THIS FENCES USED TO BE MUCH WIDER, and that is why the comment is now written AFTER the
|
||||
# status rather than before it (ersatztv#792). Posting the comment first meant every refusal below
|
||||
# left a PR carrying `Review-verdict: MERGEABLE @ <head>` with NO `review-verdict/h10` status — and
|
||||
# the comment is not the gate. The half-state was read by an operator as consent that had not been
|
||||
# granted. Ordering the two writes status-first makes the surviving half the SAFE half: a status
|
||||
# with no comment leaves the hook at condition (c) with nothing to classify, which is an `ask`, not
|
||||
# a grant. The refusals themselves are unchanged and must stay — see
|
||||
# `release.verdict-writes-status-before-comment`.
|
||||
# Fail CLOSED if the re-read itself fails. This used to be `sha_now=$(api_get ... | jq ...)`, where
|
||||
# `set -e` + `pipefail` aborted the script on a failed GET — implicitly, but before any status was
|
||||
# written. Folding the two reads into one variable with `|| true` would have swallowed that: both
|
||||
@@ -187,6 +186,17 @@ api_post "repos/$owner/$repo/statuses/$sha" "$status_payload" >/dev/null \
|
||||
|| die "failed to post the '$STATUS_CONTEXT' commit status on $short"
|
||||
printf 'posted status: %s = %s on %s\n' "$STATUS_CONTEXT" "$state" "$short"
|
||||
|
||||
# --- The comment (human-readable artifact + the hook's condition-(c) input). --------------------
|
||||
# Written LAST, after the gating status exists (ersatztv#792). The verdict line MUST start the line:
|
||||
# the hook anchors its parser to line-start precisely so a comment that merely QUOTES the template
|
||||
# mid-sentence cannot self-approve a merge.
|
||||
body="Review-verdict: $verdict @ $short"
|
||||
[ -n "$note" ] && body="$body"$'\n\n'"$note"
|
||||
comment_payload=$(jq -n --arg b "$body" '{body:$b}')
|
||||
api_post "repos/$owner/$repo/issues/$pr/comments" "$comment_payload" >/dev/null \
|
||||
|| die "the '$STATUS_CONTEXT' status was written on $short, but the verdict COMMENT could not be posted. The merge gate needs both: it reads the comment for condition (c) and will ASK rather than auto-grant until one exists. Re-run this command once Gitea is reachable."
|
||||
printf 'posted comment: Review-verdict: %s @ %s\n' "$verdict" "$short"
|
||||
|
||||
if [ "$state" = "failure" ]; then
|
||||
printf '\nPR #%s stays BLOCKED: %s is failing on head %s.\n' "$pr" "$STATUS_CONTEXT" "$short"
|
||||
else
|
||||
|
||||
@@ -146,6 +146,25 @@ MUTATIONS: tuple[Mutation, ...] = (
|
||||
"the index, which is what the proof does, separates them. That is why the proof has to "
|
||||
"remove EVERY member rather than sample one.",
|
||||
),
|
||||
Mutation(
|
||||
guard="scripts/tests/test_optional_request_members.py",
|
||||
target="scripts/tests/test_optional_request_members.py",
|
||||
clause='"ArtworkContentTypeModel": (',
|
||||
replacement='"ArtworkContentTypeModelRENAMED": (',
|
||||
proof="test_optional_request_members.py::test_every_droppable_request_schema_has_a_stated_disposition",
|
||||
granularity=CLAUSE,
|
||||
expect="no disposition written down",
|
||||
why="THE GUARD IS A TEST, so the mutation goes into the guarded ARTIFACT — here the "
|
||||
"DISPOSITIONS registry the checker maintains, the same shape as the deleted "
|
||||
"guard-inventory row below. Renaming the key rather than deleting the entry keeps the "
|
||||
"module importable, so the red is a real set-equality failure and not an ImportError "
|
||||
"reddening for the wrong reason. The rename fires BOTH directions — MISSING for the real "
|
||||
"schema and PHANTOM for the renamed key — which is the correct behaviour and worth stating, "
|
||||
"since `expect` names only the MISSING half. "
|
||||
"`ArtworkContentTypeModel` is the right key to name: it is the exact schema #807's "
|
||||
"hand-written table omitted, because `...Model` reads as a response model while it is in "
|
||||
"fact reachable from the full-replace PUT /channels/{id}.",
|
||||
),
|
||||
Mutation(
|
||||
guard="scripts/tests/test_guard_inventory.py",
|
||||
target="docs/guard-inventory.md",
|
||||
@@ -213,6 +232,20 @@ MUTATIONS: tuple[Mutation, ...] = (
|
||||
"`test_MUTATION_disarming_the_EXIT_STATUS_gate_accepts_a_run_that_NEVER_RAN_A_TEST`; the "
|
||||
"inventory holds one ref per row, so this entry names the stronger of the two.",
|
||||
),
|
||||
Mutation(
|
||||
guard="scripts/ci-toolchain-image-resolves.sh",
|
||||
target="scripts/ci-toolchain-image-resolves.sh",
|
||||
clause=" 404)",
|
||||
replacement=" 4040)",
|
||||
proof="test_ci_toolchain_image_resolves.py::test_MUTATION_a_deleted_tag_is_reported_as_a_failure",
|
||||
granularity=CLAUSE,
|
||||
expect="a deleted tag was not reported as GONE",
|
||||
why="404 is the ONE answer that establishes the pinned toolchain image is gone; every other "
|
||||
"code means the check could not run. Both fail the job, so the EXIT CODE does not separate "
|
||||
"them and the mutation is caught by the DIAGNOSTIC instead: retargeting the arm sends the "
|
||||
"real outage down the could-not-verify path, which sends an operator to the registry's "
|
||||
"health rather than to the rebuild that fixes it.",
|
||||
),
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -82,10 +82,12 @@ _DOC = yaml.safe_load(WORKFLOW.read_text())
|
||||
# reviewable act; a job silently losing its container block is not.
|
||||
TOOLCHAIN_JOBS = frozenset({"test", "migrations", "functional-e2e", "api-docs", "format"})
|
||||
|
||||
# `scan` and `build` deliberately run on the bare runner: `scan` is `runs-on: small` and needs only
|
||||
# python, and `build` drives docker/buildx on the host. Listed here so their ABSENCE above reads as
|
||||
# a decision rather than an oversight.
|
||||
BARE_RUNNER_JOBS = frozenset({"scan", "build"})
|
||||
# `scan`, `build` and `toolchain-preflight` deliberately run on the bare runner: `scan` is
|
||||
# `runs-on: small` and needs only python, `build` drives docker/buildx on the host, and
|
||||
# `toolchain-preflight` exists to report that the pinned toolchain image is GONE — a job that
|
||||
# consumed that image could not run to say so (ersatztv#772). Listed here so their ABSENCE above
|
||||
# reads as a decision rather than an oversight.
|
||||
BARE_RUNNER_JOBS = frozenset({"scan", "build", "toolchain-preflight"})
|
||||
|
||||
|
||||
def _jobs(doc) -> dict:
|
||||
|
||||
@@ -0,0 +1,303 @@
|
||||
"""Tests for `scripts/ci-toolchain-image-resolves.sh` (ersatztv#772).
|
||||
|
||||
The script answers one question — does the tag `docker-build.yml` pins still exist? — and the whole
|
||||
value is in *which answers it refuses to round off*. A registry read has three outcomes, not two:
|
||||
present, gone, and could-not-tell. Collapsing the third into either of the others is how a preflight
|
||||
becomes decoration, so each is driven here through the real entry point with a stubbed `curl`.
|
||||
|
||||
`test_MUTATION_a_deleted_tag_is_reported_as_a_failure` is the load-bearing one and is declared in
|
||||
`scripts/tests/mutation_manifest.py`. Note what it can and cannot turn on: since an unverifiable
|
||||
answer fails the job too, disarming the `404` arm still exits non-zero, so the EXIT CODE separates
|
||||
nothing. What the disarm destroys is the DIAGNOSTIC — the outage is reported as "could not verify",
|
||||
which sends an operator to the registry's health instead of to the rebuild that fixes it.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
import subprocess
|
||||
import time
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parents[2]
|
||||
SCRIPT = REPO_ROOT / "scripts" / "ci-toolchain-image-resolves.sh"
|
||||
|
||||
# Stands in for `curl -s -w '\n%{http_code}' -u <auth> -H Accept <url>`: prints a scripted body,
|
||||
# a newline and the HTTP code, and logs the call. It VALIDATES `-u` rather than ignoring it — a stub
|
||||
# that answers 200 whether or not the script authenticates would stay green if the real `-u` were
|
||||
# deleted, which is the fidelity gap that lets a test double certify a script the live registry
|
||||
# would reject on every request.
|
||||
CURL_SHIM = r"""#!/usr/bin/env python3
|
||||
import os, pathlib, sys
|
||||
|
||||
state = pathlib.Path(os.environ["STUB_DIR"])
|
||||
args = sys.argv[1:]
|
||||
url = [a for a in args if a.startswith("http")][-1]
|
||||
tag = url.rsplit("/", 1)[-1]
|
||||
auth = args[args.index("-u") + 1] if "-u" in args else ""
|
||||
with (state / "calls").open("a") as fh:
|
||||
fh.write(f"{url} auth={auth}\n")
|
||||
|
||||
# The live registry answers 401 to an anonymous read of ANY tag, present or deleted.
|
||||
user, _, password = auth.partition(":")
|
||||
if not user or not password:
|
||||
print("{}\n401", end="")
|
||||
sys.exit(0)
|
||||
|
||||
codes = dict(pair.split("=", 1) for pair in (state / "codes").read_text().split() if pair)
|
||||
code = codes.get(tag, codes.get("*", "200"))
|
||||
if code == "TRANSPORT":
|
||||
# Only the EXIT STATUS is observable: the script's `|| resp=""` discards whatever curl printed,
|
||||
# so what this reproduces is the non-zero exit, not the `\n000` real curl also emits.
|
||||
sys.exit(7)
|
||||
body = '{"schemaVersion": 2, "mediaType": "application/vnd.oci.image.manifest.v1+json"}'
|
||||
if code == "200-NOT-A-MANIFEST":
|
||||
code, body = "200", "<html><title>Sign in</title></html>"
|
||||
print(f"{body}\n{code}", end="")
|
||||
"""
|
||||
|
||||
WORKFLOW_TEMPLATE = """jobs:
|
||||
test:
|
||||
container:
|
||||
image: 192.168.1.95:3000/timothy/ersatztv-ci:{pin}
|
||||
"""
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def preflight(tmp_path):
|
||||
bindir = tmp_path / "bin"
|
||||
bindir.mkdir()
|
||||
shim = bindir / "curl"
|
||||
shim.write_text(CURL_SHIM)
|
||||
shim.chmod(0o755)
|
||||
|
||||
state = tmp_path / "state"
|
||||
state.mkdir()
|
||||
(state / "codes").write_text("*=200")
|
||||
|
||||
workflow = tmp_path / "docker-build.yml"
|
||||
workflow.write_text(WORKFLOW_TEMPLATE.format(pin="32747a0"))
|
||||
|
||||
env = dict(os.environ)
|
||||
env["PATH"] = f"{bindir}{os.pathsep}{env['PATH']}"
|
||||
env["STUB_DIR"] = str(state)
|
||||
env["ETV_CI_WORKFLOW"] = str(workflow)
|
||||
env["ETV_REGISTRY_AUTH"] = "stub-user:stub-pass"
|
||||
# The retry PAUSE is what makes failing on an unknown affordable in CI and unaffordable in a
|
||||
# test suite; the retry COUNT is behaviour, so it is kept and only the wait is removed.
|
||||
env["ETV_CI_ATTEMPTS"] = "2"
|
||||
env["ETV_CI_RETRY_SECONDS"] = "0"
|
||||
|
||||
class Handle:
|
||||
def __init__(self):
|
||||
self.env = env
|
||||
self.state = state
|
||||
self.workflow = workflow
|
||||
self.script = SCRIPT
|
||||
|
||||
def set_codes(self, mapping: dict[str, str]):
|
||||
(state / "codes").write_text(" ".join(f"{k}={v}" for k, v in mapping.items()))
|
||||
|
||||
def set_workflow_text(self, text: str):
|
||||
workflow.write_text(text)
|
||||
|
||||
def calls(self):
|
||||
log = state / "calls"
|
||||
return log.read_text().splitlines() if log.exists() else []
|
||||
|
||||
def run(self, script: Path | None = None):
|
||||
return subprocess.run(
|
||||
["bash", str(script or SCRIPT)],
|
||||
env=env,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
cwd=REPO_ROOT,
|
||||
)
|
||||
|
||||
return Handle()
|
||||
|
||||
|
||||
def test_a_pin_that_resolves_passes(preflight):
|
||||
preflight.set_codes({"*": "200"})
|
||||
result = preflight.run()
|
||||
assert result.returncode == 0, result.stderr
|
||||
assert "32747a0 resolves" in result.stdout
|
||||
assert preflight.calls(), "the registry was never queried, so nothing was established"
|
||||
|
||||
|
||||
def test_MUTATION_a_deleted_tag_is_reported_as_a_failure(preflight):
|
||||
"""The outage of 2026-08-11..13, in one assertion.
|
||||
|
||||
Declared in `mutation_manifest.py`: replacing the `404` arm sends a deleted tag down the
|
||||
could-not-verify path, which fails the job with the wrong story — a preflight that runs, reddens,
|
||||
and still misses the only thing it was built to name.
|
||||
"""
|
||||
preflight.set_codes({"32747a0": "404"})
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0, (
|
||||
"a deleted tag did not fail the preflight — the 404 arm is not load-bearing:\n"
|
||||
f"stdout={result.stdout}\nstderr={result.stderr}"
|
||||
)
|
||||
assert "IS GONE" in result.stderr, (
|
||||
"a deleted tag was not reported as GONE — the 404 arm is not load-bearing. Since an "
|
||||
"unverifiable answer now fails too, exiting non-zero no longer distinguishes 'the image is "
|
||||
"deleted' from 'the check could not run', and only this message does:\n"
|
||||
f"stderr={result.stderr}"
|
||||
)
|
||||
assert "32747a0" in result.stderr, "the message must name the tag the operator has to restore"
|
||||
assert "server-management#842" in result.stderr, "and where the durable fix lives"
|
||||
|
||||
|
||||
@pytest.mark.parametrize("code", ["TRANSPORT", "503"])
|
||||
def test_an_unknown_answer_FAILS_and_is_not_reported_as_gone(preflight, code):
|
||||
"""The first draft warned and exited 0 here, which is how a preflight becomes a no-op.
|
||||
|
||||
A missing `curl`, a moved registry or a DNS change all land in this arm, and each would have
|
||||
been green forever. It fails — but with its own wording, because "could not verify" and "IS
|
||||
GONE" send an operator to entirely different places.
|
||||
"""
|
||||
preflight.set_codes({"32747a0": code})
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0, "an unestablished check must not report success"
|
||||
assert "could NOT VERIFY" in result.stderr
|
||||
assert "IS GONE" not in result.stderr, "could-not-tell must never be reported as gone"
|
||||
|
||||
|
||||
def test_an_unknown_is_RETRIED_before_it_fails(preflight):
|
||||
"""Retries are what make failing on unknown affordable rather than flaky."""
|
||||
preflight.env["ETV_CI_ATTEMPTS"] = "3"
|
||||
preflight.set_codes({"32747a0": "503"})
|
||||
assert preflight.run().returncode != 0
|
||||
assert len(preflight.calls()) == 3, f"expected 3 attempts, got {preflight.calls()}"
|
||||
|
||||
|
||||
def test_an_ANSWER_is_not_retried(preflight):
|
||||
"""404 and 200 are answers; retrying them would only slow the job down."""
|
||||
preflight.set_codes({"32747a0": "404"})
|
||||
assert preflight.run().returncode != 0
|
||||
assert len(preflight.calls()) == 1, f"a 404 must not be retried, got {preflight.calls()}"
|
||||
|
||||
|
||||
def test_HTTP_200_with_a_body_that_is_not_a_manifest_is_not_a_pass(preflight):
|
||||
"""A proxy or a login page answers 200 too; the status line alone establishes nothing."""
|
||||
preflight.set_codes({"32747a0": "200-NOT-A-MANIFEST"})
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0
|
||||
assert "not a manifest" in result.stderr
|
||||
assert "IS GONE" not in result.stderr
|
||||
|
||||
|
||||
@pytest.mark.parametrize("code", ["401", "403"])
|
||||
def test_rejected_credentials_refuse_rather_than_pass(preflight, code):
|
||||
"""The failure mode that would otherwise make this job green forever.
|
||||
|
||||
An anonymous read of this registry is 401 for a live tag and a deleted one alike, so treating
|
||||
an auth failure as "could not tell, carry on" would turn a broken secret into a permanent,
|
||||
silent pass.
|
||||
"""
|
||||
preflight.set_codes({"32747a0": code})
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0
|
||||
assert "rejected these credentials" in result.stderr
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("value", "shape"),
|
||||
[
|
||||
(None, "unset"),
|
||||
(":", "both secrets absent — WHAT THE WORKFLOW ACTUALLY PASSES"),
|
||||
("user:", "password secret absent"),
|
||||
(":pass", "user secret absent"),
|
||||
("no-colon", "malformed"),
|
||||
],
|
||||
)
|
||||
def test_unusable_credentials_refuse_BEFORE_querying_anything(preflight, value, shape):
|
||||
"""The empty-halves cases are the ones that happen, and testing only `unset` misses them.
|
||||
|
||||
`ETV_REGISTRY_AUTH: ${{ secrets.REGISTRY_USER }}:${{ secrets.REGISTRY_PASSWORD }}` interpolates
|
||||
a missing secret to the empty string, so a job with no secrets configured passes the non-empty
|
||||
string ":" — which is a perfectly good non-empty value and a useless credential. The registry
|
||||
answers 401 to it for a live tag and a deleted one alike.
|
||||
"""
|
||||
if value is None:
|
||||
del preflight.env["ETV_REGISTRY_AUTH"]
|
||||
else:
|
||||
preflight.env["ETV_REGISTRY_AUTH"] = value
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0, f"{shape}: reported success on a credential it cannot use"
|
||||
assert "ETV_REGISTRY_AUTH" in result.stderr
|
||||
assert preflight.calls() == [], "it must not query the registry it cannot authenticate to"
|
||||
|
||||
|
||||
def test_the_credential_actually_REACHES_the_registry(preflight):
|
||||
"""Anti-vacuity for every test above: the stub 401s an unauthenticated read, as the live
|
||||
registry does, so a script that stopped passing `-u` would redden the whole file rather than
|
||||
sailing through on a stub that answers 200 regardless."""
|
||||
preflight.set_codes({"*": "200"})
|
||||
assert preflight.run().returncode == 0
|
||||
assert preflight.calls() == [
|
||||
"http://192.168.1.95:3000/v2/timothy/ersatztv-ci/manifests/32747a0 auth=stub-user:stub-pass"
|
||||
]
|
||||
|
||||
|
||||
def test_a_workflow_with_no_pin_at_all_is_a_failure(preflight):
|
||||
"""If the grep stops matching, the honest report is 'I found nothing', not 'all clear'."""
|
||||
preflight.set_workflow_text("jobs:\n test:\n runs-on: ubuntu-latest\n")
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0
|
||||
assert "no ersatztv-ci pin found" in result.stderr
|
||||
|
||||
|
||||
def test_every_distinct_pin_is_checked_and_one_gone_fails_the_job(preflight):
|
||||
"""`ci-image-pin` bans a second pin; this must not silently check only the first one anyway."""
|
||||
preflight.set_workflow_text(
|
||||
WORKFLOW_TEMPLATE.format(pin="32747a0") + " image: 192.168.1.95:3000/timothy/ersatztv-ci:15d2439\n"
|
||||
)
|
||||
preflight.set_codes({"32747a0": "200", "15d2439": "404"})
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0
|
||||
assert "15d2439" in result.stderr
|
||||
assert len(preflight.calls()) == 2, f"both pins must be queried, got {preflight.calls()}"
|
||||
|
||||
|
||||
def test_the_grep_line_cannot_match_ITSELF(preflight):
|
||||
"""The pin is found with the same expression `pr-checks.yml::ci-image-pin` uses.
|
||||
|
||||
That expression is written into this script's own source, so a careless pattern would find its
|
||||
own text and 'check' a pin nobody wrote — and the same hazard sits in `pr-checks.yml`, whose
|
||||
pin-count check greps the file this script's job now lives in. Feed the real script its own
|
||||
source as the workflow file: the answer must be 'no pin found', not a query for `[0-9a-f]+`.
|
||||
This also pins the second half of the property — the source carries no literal pin of its own,
|
||||
so the file cannot go stale against a pin bump it does not participate in.
|
||||
"""
|
||||
preflight.set_workflow_text(SCRIPT.read_text())
|
||||
result = preflight.run()
|
||||
assert result.returncode != 0
|
||||
assert "no ersatztv-ci pin found" in result.stderr
|
||||
assert preflight.calls() == []
|
||||
|
||||
|
||||
def test_the_PRODUCTION_retry_defaults_are_the_ones_that_run(preflight):
|
||||
"""Every other test overrides the retry knobs, so nothing evaluated `${VAR:-default}` itself.
|
||||
|
||||
That matters because the defaults are the argument: "unknown fails" is only affordable if an
|
||||
ordinary registry blip is absorbed first. Edited to 1 attempt / 0 seconds, this file would stay
|
||||
green while a single transient 503 reddened every PR. So this one drops both overrides and
|
||||
measures the real thing — three attempts, and a pause long enough to have actually happened.
|
||||
"""
|
||||
del preflight.env["ETV_CI_ATTEMPTS"]
|
||||
del preflight.env["ETV_CI_RETRY_SECONDS"]
|
||||
preflight.set_codes({"32747a0": "503"})
|
||||
|
||||
started = time.monotonic()
|
||||
result = preflight.run()
|
||||
elapsed = time.monotonic() - started
|
||||
|
||||
assert result.returncode != 0
|
||||
assert len(preflight.calls()) == 3, f"the default attempt count is not 3 — got {len(preflight.calls())} call(s)"
|
||||
assert elapsed >= 8, (
|
||||
f"two pauses at the default 5s should clear the 8s floor; took {elapsed:.1f}s, so the pause "
|
||||
"has been shortened out from under the 'a blip does not redden a PR' argument"
|
||||
)
|
||||
@@ -0,0 +1,821 @@
|
||||
"""#807 guard: every schema that can SILENTLY DROP a member on a SPA write has a stated disposition.
|
||||
|
||||
WHAT THIS BLOCKS. A request-body property that is absent from its schema's `required` array emits
|
||||
into `web/src/api/generated/v1.d.ts` as an OPTIONAL member (`"weight"?: number`). A SPA builder may
|
||||
then omit it, `tsc` says nothing — optional means omittable, by design — and on a FULL-REPLACE write
|
||||
the server stores the field's default. That is #754's mechanism and it is what #807 found live in
|
||||
`MultiCollectionItemRequest.weight` and `UpdateFFmpegProfileRequest.qsvPreferNativeDecoder`.
|
||||
|
||||
WHY IT IS A DERIVED GUARD AND NOT A TABLE IN A DOC. #807 shipped the disposition list by hand
|
||||
TWICE and got it wrong BOTH times, each time by sorting a schema on its NAME rather than on what its
|
||||
endpoint does:
|
||||
|
||||
round 1 a prose sentence exempted "create/update" — `updateMultiCollection` and
|
||||
`updateFFmpegProfile` are full replaces, and both were live silent drops.
|
||||
round 2 a hand-written table replaced that sentence and omitted `ArtworkContentTypeModel`,
|
||||
because `…Model` reads as a response model. It is reachable from `PUT /channels/{id}`.
|
||||
|
||||
Two misses from one mechanism, so the mechanism goes rather than the list getting a third patch.
|
||||
`testing.guard-derives-population-from-source` is explicit that a hand-written list is "a filter
|
||||
frozen at authoring time, correct on the day it was written and unable to report the day it stopped
|
||||
being" — and unlike #820's population (sites in code, which needs compiler-API tooling), THIS
|
||||
population has an authoritative machine-readable source: the OpenAPI document.
|
||||
|
||||
SCOPE vs POPULATION, per that same record. The POPULATION — which schemas can drop a member on a
|
||||
write — is DERIVED here, every run, from `ErsatzTV/wwwroot/openapi/v1.json`. The DISPOSITIONS below
|
||||
are the SCOPE: a reviewed policy choice per schema, legitimately hand-written, and each one is
|
||||
FORCED to exist by the set-equality assertion. A new optional member in a named component schema
|
||||
reachable from a request body, or in an inline request body, fails this test until someone writes
|
||||
down what should happen about it — that is the reach, bounded by what `_resolve`
|
||||
resolves. That resolver walks `allOf`, `oneOf`, `anyOf`, `if`/`then`/`else`, `dependentSchemas`,
|
||||
`items`/`prefixItems` and inline objects under `properties`, and deliberately contributes nothing
|
||||
for `additionalProperties`/`patternProperties` (which name no fixed members) — each pinned by a
|
||||
case in `test_composition_is_resolved_the_way_JSON_Schema_means_it`. It does NOT follow `$ref`;
|
||||
that is the component walk's job. An earlier draft said "anywhere in the request graph"; inline bodies were invisible at
|
||||
the time, so the universal was false the day it was written.
|
||||
|
||||
Set equality is asserted in BOTH directions and reported separately, because they are opposite
|
||||
defects: `missing` is a schema the API can drop and nobody has ruled on (the #807 defect), `phantom`
|
||||
is a disposition for a schema that no longer has an optional member reachable from a request body
|
||||
(the registry claiming coverage of something gone).
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import json
|
||||
import re
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
REPO_ROOT = Path(__file__).resolve().parents[2]
|
||||
|
||||
OPENAPI = REPO_ROOT / "ErsatzTV" / "wwwroot" / "openapi" / "v1.json"
|
||||
|
||||
# --- the closed disposition vocabulary -------------------------------------------------------
|
||||
#
|
||||
# COVERED the SPA builds this body and the builder is annotated `Complete<T>`, so omitting a
|
||||
# member is a typecheck error (`web/src/api/completeRequest.ts`).
|
||||
# CREATE a POST that creates a new entity, where an omitted member correctly means "use the
|
||||
# default". Annotating it would be a BUG, not coverage — see the from-lineup note.
|
||||
# TRIGGER the body parameterises an ACTION and replaces no stored entity, so there is nothing to
|
||||
# drop.
|
||||
# COMPUTED the optional members are get-only computed properties on the C# record. System.Text.Json
|
||||
# never deserializes them, so the client cannot drop a stored value by omitting them —
|
||||
# and `Complete<T>` must NOT be applied here, because it would force a caller to invent
|
||||
# server-computed values in an outbound request.
|
||||
COVERED = "COVERED"
|
||||
CREATE = "CREATE"
|
||||
TRIGGER = "TRIGGER"
|
||||
COMPUTED = "COMPUTED"
|
||||
|
||||
DISPOSITIONS: dict[str, tuple[str, str]] = {
|
||||
"UpdateFFmpegProfileRequest": (
|
||||
COVERED,
|
||||
"PUT /ffmpeg/profiles/{id} is a full replace. `qsvPreferNativeDecoder` is a defaulted ctor "
|
||||
"param so ASP.NET drops it from `required`. Was a LIVE silent drop before #807.",
|
||||
),
|
||||
"CreateFFmpegProfileRequest": (
|
||||
COVERED,
|
||||
"FFmpegProfilesScreen's `Draft` feeds BOTH the POST and the full-replace PUT, so the draft "
|
||||
"type itself is `Complete<…>` rather than only the update wrapper.",
|
||||
),
|
||||
"MultiCollectionItemRequest": (
|
||||
COVERED,
|
||||
"Nested in the full-replace PUT /multi-collections/{id}. `weight` is a defaulted ctor param "
|
||||
"(`CreateMultiCollectionRequest.cs`). Was a LIVE silent drop before #807: the screen carried "
|
||||
"a prose comment warning that dropping it resets every weight to 1, and a comment is not a "
|
||||
"check.",
|
||||
),
|
||||
"ArtworkContentTypeModel": (
|
||||
COMPUTED,
|
||||
"Reachable from the full-replace PUT /channels/{id} (via `UpdateChannelRequest.logo`), so "
|
||||
"the endpoint test alone would put it in COVERED. It is not: `IsExternalUrl`, "
|
||||
"`HasContentType` and `UrlWithContentType` are computed get-only properties on the record "
|
||||
"`ArtworkContentTypeModel(string Path, string ContentType)`, never deserialized, so a "
|
||||
"client omitting them drops nothing. Annotating the SPA site `Complete<…>` would force it "
|
||||
"to fabricate server-computed values. This row exists because #807's hand-written table "
|
||||
"omitted this schema on the strength of its `…Model` name.",
|
||||
),
|
||||
"AutoTunedChannelRequest": (
|
||||
CREATE,
|
||||
"POST /channels/auto-tune. `CreateChannelFromLineupHandler` does `Channels.Add(...)` and "
|
||||
"rejects a duplicate number; it never overwrites an existing channel.",
|
||||
),
|
||||
"CreateChannelFromLineupAdvancedOptionsRequest": (
|
||||
CREATE,
|
||||
"POST /channels/from-lineup. Omission is LOAD-BEARING here: `CreateChannelFromLineupClearField` "
|
||||
"documents that for the template-inheritable fields a null/omitted override means INHERIT the "
|
||||
"template value, with a separate explicit `clear` list to force NONE. `Complete<T>` would "
|
||||
"collapse that third state into explicit-null.",
|
||||
),
|
||||
"AutoTuneSourceWeightRequest": (
|
||||
CREATE,
|
||||
"Nested in the auto-tune POST body; same create-time semantics as its parent.",
|
||||
),
|
||||
"POST /api/v1/artwork/uploads (multipart/form-data inline body)": (
|
||||
TRIGGER,
|
||||
"A multipart upload. Its body is declared INLINE rather than as a named schema, which is why "
|
||||
"it needs a key of this shape at all. `file` and `target` sit outside `required`, but the "
|
||||
"endpoint stores an uploaded blob and replaces no entity, so there is no stored value for an "
|
||||
"omitted member to overwrite; `web/src/api/artwork.ts` builds it as hand-rolled `FormData` "
|
||||
"with no generated type involved.",
|
||||
),
|
||||
"ScanShowRequest": (
|
||||
TRIGGER,
|
||||
"POST /libraries/{id}/scan-show starts a scan. `deepScan` parameterises the action; no entity is replaced.",
|
||||
),
|
||||
}
|
||||
|
||||
# Anti-vacuity floors. A completeness check whose population came back empty must not report that it
|
||||
# proved everything (`testing.guard-ships-with-mutation-proof`). These are LOWER bounds on the
|
||||
# derived intermediates, deliberately well under today's values (142 reachable, 13 with optional
|
||||
# members) so ordinary schema churn does not trip them — they exist to catch a broken parse or a
|
||||
# `$ref` walk that reached nothing, not to pin the corpus.
|
||||
MIN_REQUEST_REACHABLE_SCHEMAS = 40
|
||||
MIN_SCHEMAS_WITH_OPTIONAL_MEMBERS = 5
|
||||
|
||||
|
||||
def _load() -> dict:
|
||||
if not OPENAPI.is_file():
|
||||
pytest.fail(f"{OPENAPI.relative_to(REPO_ROOT)} is missing — run scripts/update-openapi.sh")
|
||||
return json.loads(OPENAPI.read_text())
|
||||
|
||||
|
||||
def _schema_refs(node: object, out: set[str]) -> None:
|
||||
"""Collect every `#/components/schemas/X` name anywhere under `node`."""
|
||||
if isinstance(node, dict):
|
||||
ref = node.get("$ref")
|
||||
if isinstance(ref, str) and ref.startswith("#/components/schemas/"):
|
||||
out.add(ref.rsplit("/", 1)[1])
|
||||
for value in node.values():
|
||||
_schema_refs(value, out)
|
||||
elif isinstance(node, list):
|
||||
for value in node:
|
||||
_schema_refs(value, out)
|
||||
|
||||
|
||||
def _request_reachable(doc: dict) -> set[str]:
|
||||
"""Every schema reachable from ANY operation's request body, transitively.
|
||||
|
||||
Two deliberate non-restrictions, both because this population has now been drawn by hand wrongly
|
||||
twice and every hand-drawn edge is a place to be wrong again:
|
||||
|
||||
NO VERB ALLOW-LIST. An earlier draft scanned POST/PUT/PATCH, which reads as obviously right and
|
||||
already had an exception: `DELETE /api/v1/media-items` carries a request body. Rather than argue
|
||||
that a DELETE body cannot cause a full-replace drop — probably true, and exactly the kind of
|
||||
"probably" that produced this record's two live misses — every operation carrying a request body
|
||||
seeds the walk, and anything it surfaces must acquire a stated disposition.
|
||||
|
||||
TRANSITIVE, and that is load-bearing rather than thorough: `MultiCollectionItemRequest` and
|
||||
`ArtworkContentTypeModel` are both NESTED, so a check reading only top-level request bodies
|
||||
would have missed both of the schemas this guard exists because of. `_schema_refs` walks every
|
||||
value of every dict and every list element, so `oneOf` (23 occurrences today), `items`,
|
||||
`additionalProperties` and any future composition keyword are covered without naming them.
|
||||
"""
|
||||
schemas = doc["components"]["schemas"]
|
||||
seeds: set[str] = set()
|
||||
for operations in doc["paths"].values():
|
||||
for operation in operations.values():
|
||||
if isinstance(operation, dict) and operation.get("requestBody"):
|
||||
_schema_refs(operation["requestBody"], seeds)
|
||||
|
||||
seen: set[str] = set()
|
||||
stack = list(seeds)
|
||||
while stack:
|
||||
name = stack.pop()
|
||||
if name in seen or name not in schemas:
|
||||
continue
|
||||
seen.add(name)
|
||||
nested: set[str] = set()
|
||||
_schema_refs(schemas[name], nested)
|
||||
stack.extend(nested - seen)
|
||||
return seen
|
||||
|
||||
|
||||
def _optional_of(schema: object) -> set[str]:
|
||||
"""The members of `schema` a client may omit, nested inline objects included.
|
||||
|
||||
This is the function to call. `_resolve` below is its recursive half and returns three sets;
|
||||
the split matters and is the fix for a real defect, so it is stated here rather than in a
|
||||
comment further down.
|
||||
|
||||
A nested inline object lives in its OWN namespace. Qualifying its members as `parent.child`
|
||||
BEFORE subtracting `required` merges the two namespaces, and a literal member named
|
||||
`parent.child` then collides with the nested one — masking it entirely when the literal is
|
||||
required, so a droppable member vanishes from the population with nothing failing (measured
|
||||
2026-08-23). Subtracting inside each namespace first and qualifying only the survivors means
|
||||
the two sets are never mixed, so the collision cannot arise and there is no separator to
|
||||
defend. An earlier version instead ASSERTED that no property name contains a dot, which is a
|
||||
guard where a restructure was available.
|
||||
|
||||
Residual, stated because it is real: if a literal `a.b` and a nested `a` -> `b` are BOTH
|
||||
optional they conflate into one reported string. That is a diagnostic ambiguity, not a miss —
|
||||
the schema still enters the population and still forces a disposition.
|
||||
"""
|
||||
properties, required, nested_optional = _resolve(schema)
|
||||
return (properties - required) | nested_optional
|
||||
|
||||
|
||||
def _resolve(schema: object) -> tuple[set[str], set[str], set[str]]:
|
||||
"""(properties, required, already-resolved nested optional members) for one schema node.
|
||||
|
||||
The third set is carried through every composition site rather than merged into the first two,
|
||||
for the namespace reason in `_optional_of`. Do NOT collapse this back to a 2-tuple to spare the
|
||||
callers: folding `nested_optional` into `properties` re-creates the mask at the merge boundary
|
||||
and the whole suite stays green while it does.
|
||||
|
||||
NESTING. Handling `allOf` one level deep misses an `allOf` inside an `allOf` — a property there
|
||||
reported as no properties at all. Composition nests, so the resolution has to recurse.
|
||||
|
||||
`$ref` IS DELIBERATELY NOT RESOLVED HERE, and that is not a hole: an arm that is a `$ref` names
|
||||
a component schema, which `_request_reachable` already seeds on (`_schema_refs` finds a `$ref`
|
||||
anywhere, arms included) and `_optional_members` already walks, so its optional members surface
|
||||
under their OWN key rather than being merged into the inline one. Verified by construction in
|
||||
`test_a_ref_ARM_is_covered_by_the_component_walk_not_by_this_one`. Resolving refs here as well
|
||||
would report the same member twice under two keys, which is worse than either.
|
||||
|
||||
CONJUNCTION vs DISJUNCTION. `allOf` arms ALL apply, so their `required` sets UNION. `oneOf` and
|
||||
`anyOf` arms are ALTERNATIVES, so a member is only genuinely required when EVERY alternative
|
||||
requires it — the `required` sets INTERSECT. Unioning them instead (the first version) marks a
|
||||
member required because one arm requires it, hiding the arm that lets a client omit it. That is
|
||||
the drop this whole guard exists to catch, so getting it backwards is not a detail.
|
||||
|
||||
No cycle guard, deliberately. This resolver never follows `$ref`, so the only way to recurse
|
||||
forever is a schema that contains ITSELF by object identity — which `json.load` cannot produce,
|
||||
since JSON has no back-references. Verified 2026-08-23: a hand-built self-referential dict does
|
||||
raise `RecursionError`, a LOUD red rather than a silent wrong answer. The `$ref` cycle a real
|
||||
document CAN express is handled by `_request_reachable`'s `seen` set, also verified.
|
||||
"""
|
||||
if not isinstance(schema, dict):
|
||||
return set(), set(), set()
|
||||
|
||||
properties = set(schema.get("properties") or {})
|
||||
required = set(schema.get("required") or [])
|
||||
nested_optional: set[str] = set()
|
||||
|
||||
# A property may itself be an INLINE OBJECT rather than a `$ref`, and the generator recurses
|
||||
# into it (`objectTypeFromSchema` -> `typeFromSchema`), so a member outside that nested
|
||||
# `required` really does emit `?:` and really is droppable. Resolved in its own namespace and
|
||||
# qualified afterwards — see `_optional_of`.
|
||||
for name, value in (schema.get("properties") or {}).items():
|
||||
if not isinstance(value, dict) or "$ref" in value:
|
||||
continue
|
||||
nested_optional |= {f"{name}.{child}" for child in _optional_of(value)}
|
||||
|
||||
# CONJUNCTIVE: every `allOf` arm applies, so both sets union.
|
||||
for branch in schema.get("allOf") or []:
|
||||
branch_properties, branch_required, branch_nested = _resolve(branch)
|
||||
properties |= branch_properties
|
||||
required |= branch_required
|
||||
nested_optional |= branch_nested
|
||||
|
||||
# CONDITIONAL keywords — `if`, `then`, `else`, `dependentSchemas`. All four are treated the same
|
||||
# way: collect their properties (a client may send them) and DISCARD their `required` (it binds
|
||||
# only on a branch that may not be taken, so the member is omittable).
|
||||
#
|
||||
# An earlier version put `then`/`else` in the conjunctive list above, unioning their `required`.
|
||||
# That is the polarity error this function warns about above, committed in the same block:
|
||||
# `then` and `else` are MUTUALLY EXCLUSIVE, so a member required only under `then` is omittable
|
||||
# whenever `if` does not match, and the guard reported it as required — the silent-miss
|
||||
# direction, which is the one this whole file exists to catch. `if`'s `required` is discarded
|
||||
# for a different reason (it selects a branch rather than obliging anyone), and
|
||||
# `dependentSchemas` for a third (it binds only when its trigger key is present), but the
|
||||
# resulting rule is identical, so they share one loop rather than three arguments.
|
||||
conditional: list[object] = []
|
||||
for keyword in ("if", "then", "else"):
|
||||
value = schema.get(keyword)
|
||||
conditional.extend(value if isinstance(value, list) else ([value] if isinstance(value, dict) else []))
|
||||
conditional.extend((schema.get("dependentSchemas") or {}).values())
|
||||
for branch in conditional:
|
||||
branch_properties, _, branch_nested = _resolve(branch)
|
||||
properties |= branch_properties
|
||||
nested_optional |= branch_nested
|
||||
|
||||
# DISJUNCTIVE keywords, handled SEPARATELY rather than concatenated: `oneOf` and `anyOf` are
|
||||
# conjunctive WITH EACH OTHER (a body satisfying both must satisfy one arm of each), so the
|
||||
# correct required set is the intersection within each keyword, unioned across them. Merging
|
||||
# the two lists first intersects across keywords and under-reports required.
|
||||
for keyword in ("oneOf", "anyOf"):
|
||||
alternatives = schema.get(keyword) or []
|
||||
shared_required: set[str] | None = None
|
||||
for branch in alternatives:
|
||||
branch_properties, branch_required, branch_nested = _resolve(branch)
|
||||
properties |= branch_properties
|
||||
nested_optional |= branch_nested
|
||||
shared_required = branch_required if shared_required is None else (shared_required & branch_required)
|
||||
if shared_required:
|
||||
required |= shared_required
|
||||
|
||||
# An inline body may be an ARRAY of inline objects; the members live on `items`, and a member
|
||||
# droppable there is droppable in the request. `items` is a SCHEMA in OpenAPI 3.1 / JSON Schema
|
||||
# 2020-12 and may be a LIST in the 3.0 tuple form, so both shapes are walked — and `prefixItems`
|
||||
# is the 2020-12 spelling of that tuple.
|
||||
for keyword in ("items", "prefixItems"):
|
||||
value = schema.get(keyword)
|
||||
branches = value if isinstance(value, list) else ([value] if isinstance(value, dict) else [])
|
||||
for branch in branches:
|
||||
branch_properties, branch_required, branch_nested = _resolve(branch)
|
||||
properties |= branch_properties
|
||||
required |= branch_required
|
||||
nested_optional |= branch_nested
|
||||
|
||||
return properties, required, nested_optional
|
||||
|
||||
|
||||
def _optional_members(doc: dict) -> dict[str, list[str]]:
|
||||
"""Schema -> its properties that sit OUTSIDE `required`, i.e. the ones that emit `?:`."""
|
||||
out: dict[str, list[str]] = {}
|
||||
for name, schema in doc["components"]["schemas"].items():
|
||||
optional = sorted(_optional_of(schema))
|
||||
if optional:
|
||||
out[name] = optional
|
||||
return out
|
||||
|
||||
|
||||
def _inline_body_members(doc: dict) -> dict[str, list[str]]:
|
||||
"""Optional members of request bodies declared INLINE, i.e. with no `$ref` to a named schema.
|
||||
|
||||
`_optional_members` iterates `components.schemas`, and `_request_reachable` seeds from `$ref`s,
|
||||
so between them an inline body is invisible in BOTH directions. That was not hypothetical: the
|
||||
document declares one today (`POST /api/v1/artwork/uploads`, multipart), whose `file` and
|
||||
`target` sit outside any `required` array — a member already present and outside the guard's
|
||||
reach while its docstring claimed to cover the whole request graph.
|
||||
|
||||
Keyed by `"<VERB> <path> (<media type> inline body)"` rather than by a schema name, because there is no name
|
||||
to use — which is exactly why the component-schema walk cannot see it.
|
||||
|
||||
Composition is resolved by `_optional_of`/`_resolve`, which recurse and treat `allOf` as
|
||||
conjunction and `oneOf`/`anyOf` as alternatives — this body splits its properties across `allOf`
|
||||
arms and would otherwise report none.
|
||||
"""
|
||||
out: dict[str, list[str]] = {}
|
||||
for path, operations in doc["paths"].items():
|
||||
for verb, operation in operations.items():
|
||||
if not isinstance(operation, dict) or not operation.get("requestBody"):
|
||||
continue
|
||||
for content_type, media in (operation["requestBody"].get("content") or {}).items():
|
||||
schema = media.get("schema") or {}
|
||||
if "$ref" in schema:
|
||||
# Belt-and-braces, not load-bearing: `_optional_of` returns an empty
|
||||
# sets for a bare `$ref` node anyway, so deleting this line changes no result
|
||||
# today. It stays because a body that names a component schema is that schema's
|
||||
# business — `_optional_members` already covers it — and skipping it here keeps
|
||||
# that division explicit rather than accidental.
|
||||
continue
|
||||
optional = sorted(_optional_of(schema))
|
||||
if optional:
|
||||
# Keyed by MEDIA TYPE as well as verb and path. An operation may declare more
|
||||
# than one inline body (a second `[Consumes]` is all it takes), and keying on
|
||||
# verb+path alone made the later one overwrite the earlier — a droppable member
|
||||
# silently disappearing from the population rather than failing.
|
||||
out[f"{verb.upper()} {path} ({content_type} inline body)"] = optional
|
||||
return out
|
||||
|
||||
|
||||
def _droppable(doc: dict) -> dict[str, list[str]]:
|
||||
reachable = _request_reachable(doc)
|
||||
droppable = {n: m for n, m in _optional_members(doc).items() if n in reachable}
|
||||
droppable.update(_inline_body_members(doc))
|
||||
return droppable
|
||||
|
||||
|
||||
def test_the_derivation_reached_a_real_population() -> None:
|
||||
"""Anti-vacuity: a broken `$ref` walk or parse must not read as 'nothing to rule on'.
|
||||
|
||||
What this test does NOT do, measured 2026-08-22 rather than assumed: it does not catch a
|
||||
PARTIALLY broken walk. Deleting the transitive step from `_request_reachable` — so only the
|
||||
schemas named directly on a request body resolve — leaves both floors satisfied and this test
|
||||
GREEN. What reddens is `test_MUTATION_a_planted_optional_member_is_reported_as_MISSING`, whose
|
||||
planted schema is reached through a nested `$ref` precisely so that it can, plus the disposition
|
||||
test (the nested rows vanish and report as PHANTOM). So the floors below are the crude backstop
|
||||
against a parse that reached nothing at all; the planted-member test is what actually holds the
|
||||
walk honest, and it should be the one kept working if these two ever conflict.
|
||||
"""
|
||||
doc = _load()
|
||||
reachable = _request_reachable(doc)
|
||||
optional = _optional_members(doc)
|
||||
assert len(reachable) >= MIN_REQUEST_REACHABLE_SCHEMAS, (
|
||||
f"only {len(reachable)} schemas reachable from a request body — the $ref walk is broken, not the API"
|
||||
)
|
||||
assert len(optional) >= MIN_SCHEMAS_WITH_OPTIONAL_MEMBERS, (
|
||||
f"only {len(optional)} schemas have a property outside `required` — suspect the parse"
|
||||
)
|
||||
|
||||
|
||||
def test_the_walks_ASSUMPTIONS_about_the_document_still_hold() -> None:
|
||||
"""The derivation's scope mirrors two properties of the OpenAPI document. Check them.
|
||||
|
||||
`testing.guard-derives-population-from-source`: "When the scope itself MIRRORS an authoritative
|
||||
source, the mirror needs its own equality check or a dated staleness marker, or the guard is
|
||||
complete within a scope that has silently gone stale." Two such assumptions are baked into
|
||||
`_request_reachable`, and both are true of the document today (2026-08-22) rather than
|
||||
guaranteed by anything:
|
||||
|
||||
1. Request bodies are declared INLINE on the operation. If ASP.NET ever emits a
|
||||
`components.requestBodies` bucket and operations `$ref` into it, the seed walk still finds
|
||||
the `$ref` — but only because `_schema_refs` collects `#/components/schemas/...` names, so a
|
||||
body referencing `#/components/requestBodies/X` would seed NOTHING and the schemas under it
|
||||
would drop out of the population silently.
|
||||
2. Every `$ref` in the document points into `#/components/schemas/`. `_schema_refs` matches on
|
||||
that prefix, so a ref into any other bucket is invisible to it.
|
||||
|
||||
Both are cheap to assert and neither is asserted anywhere else, so a change in the emitter
|
||||
would otherwise shrink this guard's population without failing anything.
|
||||
|
||||
A THIRD assumption used to sit here unstated and was already violated: that every request body
|
||||
`$ref`s a named component schema. `POST /api/v1/artwork/uploads` declares its body inline, so
|
||||
both `_optional_members` (which iterates `components.schemas`) and `_request_reachable` (which
|
||||
seeds from `$ref`s) were blind to it. That one is not an assumption any more —
|
||||
`_inline_body_members` handles it — which is why it is described here rather than asserted.
|
||||
"""
|
||||
doc = _load()
|
||||
buckets = set(doc.get("components", {}))
|
||||
assert "requestBodies" not in buckets, (
|
||||
"the OpenAPI document now declares components.requestBodies — `_request_reachable` seeds "
|
||||
"only from inline operation bodies and `_schema_refs` only follows #/components/schemas/, "
|
||||
"so schemas behind a shared request body are now INVISIBLE to this guard. Teach the walk "
|
||||
"to resolve that bucket before deleting this assertion."
|
||||
)
|
||||
ref_buckets = set(re.findall(r'"#/components/([^/"]+)/', json.dumps(doc)))
|
||||
assert ref_buckets <= {"schemas"}, (
|
||||
f"$refs now point into {sorted(ref_buckets - {'schemas'})} as well as schemas; "
|
||||
"`_schema_refs` matches only the schemas prefix and silently ignores the rest"
|
||||
)
|
||||
|
||||
|
||||
def test_every_droppable_request_schema_has_a_stated_disposition() -> None:
|
||||
"""Set equality, both directions, accumulated into ONE message.
|
||||
|
||||
Failing fast on the first mismatch hands back one schema at a time and invites fixing them one
|
||||
at a time, which is how #754's twin stayed hidden.
|
||||
"""
|
||||
droppable = _droppable(_load())
|
||||
|
||||
unruled = sorted(set(droppable) - set(DISPOSITIONS))
|
||||
phantom = sorted(set(DISPOSITIONS) - set(droppable))
|
||||
|
||||
problems: list[str] = []
|
||||
if unruled:
|
||||
problems.append(
|
||||
"MISSING — reachable from a request body with a member outside `required`, and "
|
||||
"no disposition written down. Decide what happens to each and add a row:\n"
|
||||
+ "\n".join(f" {n}: optional members {droppable[n]}" for n in unruled)
|
||||
)
|
||||
if phantom:
|
||||
problems.append(
|
||||
"PHANTOM — a disposition for a schema that is no longer request-reachable with an "
|
||||
"optional member. Delete the row rather than leaving it claiming coverage:\n"
|
||||
+ "\n".join(f" {n}" for n in phantom)
|
||||
)
|
||||
assert not problems, "\n\n".join(problems)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("case", "schema", "expected"),
|
||||
[
|
||||
(
|
||||
"a flat schema reports the properties outside `required`",
|
||||
{"type": "object", "properties": {"a": {}, "b": {}}, "required": ["a"]},
|
||||
["b"],
|
||||
),
|
||||
(
|
||||
"allOf arms are conjunctive: each arm's `required` applies",
|
||||
{"allOf": [{"properties": {"a": {}}}, {"properties": {"b": {}}, "required": ["b"]}]},
|
||||
["a"],
|
||||
),
|
||||
(
|
||||
"allOf NESTED inside allOf is reached — a one-level walk reported nothing here",
|
||||
{"allOf": [{"allOf": [{"properties": {"deep": {}}}]}]},
|
||||
["deep"],
|
||||
),
|
||||
(
|
||||
"oneOf arms are ALTERNATIVES: required in one arm only means a client may omit it",
|
||||
{"oneOf": [{"properties": {"x": {}}, "required": ["x"]}, {"properties": {"x": {}}}]},
|
||||
["x"],
|
||||
),
|
||||
(
|
||||
"oneOf where EVERY arm requires it is genuinely required",
|
||||
{
|
||||
"oneOf": [
|
||||
{"properties": {"x": {}}, "required": ["x"]},
|
||||
{"properties": {"x": {}}, "required": ["x"]},
|
||||
]
|
||||
},
|
||||
[],
|
||||
),
|
||||
(
|
||||
"anyOf arms are alternatives too — required in one arm only means droppable",
|
||||
{"anyOf": [{"properties": {"y": {}}, "required": ["y"]}, {"properties": {"y": {}}}]},
|
||||
["y"],
|
||||
),
|
||||
(
|
||||
"anyOf where EVERY arm requires it is genuinely required",
|
||||
{
|
||||
"anyOf": [
|
||||
{"properties": {"y": {}}, "required": ["y"]},
|
||||
{"properties": {"y": {}}, "required": ["y"]},
|
||||
]
|
||||
},
|
||||
[],
|
||||
),
|
||||
(
|
||||
"oneOf and anyOf on ONE node are conjunctive with EACH OTHER, not one alternative list",
|
||||
{
|
||||
"oneOf": [{"properties": {"x": {}}, "required": ["x"]}, {"properties": {"x": {}}, "required": ["x"]}],
|
||||
"anyOf": [{"properties": {"y": {}}, "required": ["y"]}, {"properties": {"y": {}}, "required": ["y"]}],
|
||||
},
|
||||
[],
|
||||
),
|
||||
(
|
||||
"composition NESTED inside an alternative arm is reached",
|
||||
{"oneOf": [{"allOf": [{"properties": {"nestedInArm": {}}}]}]},
|
||||
["nestedInArm"],
|
||||
),
|
||||
(
|
||||
"properties in arms 2..n are collected, not just the first arm's",
|
||||
{"oneOf": [{"properties": {"first": {}}}, {"properties": {"second": {}}}]},
|
||||
["first", "second"],
|
||||
),
|
||||
(
|
||||
"a top-level `required` still applies when alternatives are present",
|
||||
{"properties": {"top": {}, "other": {}}, "required": ["top"], "oneOf": [{"properties": {"arm": {}}}]},
|
||||
["arm", "other"],
|
||||
),
|
||||
(
|
||||
"an inline body that is an ARRAY of objects exposes its item members",
|
||||
{"type": "array", "items": {"properties": {"itemReq": {}, "itemOpt": {}}, "required": ["itemReq"]}},
|
||||
["itemOpt"],
|
||||
),
|
||||
(
|
||||
"`items` in the 3.0 TUPLE form (a list) is walked, not just the schema form",
|
||||
{"type": "array", "items": [{"properties": {"tupleReq": {}, "tupleOpt": {}}, "required": ["tupleReq"]}]},
|
||||
["tupleOpt"],
|
||||
),
|
||||
(
|
||||
"`prefixItems`, the 2020-12 spelling of a tuple, is walked",
|
||||
{"type": "array", "prefixItems": [{"properties": {"prefixOpt": {}}}]},
|
||||
["prefixOpt"],
|
||||
),
|
||||
(
|
||||
"nested arrays are followed to the object at the bottom",
|
||||
{"type": "array", "items": {"type": "array", "items": {"properties": {"deepOpt": {}}}}},
|
||||
["deepOpt"],
|
||||
),
|
||||
(
|
||||
"`dependentSchemas` members are collectable and its conditional `required` is discarded",
|
||||
{"dependentSchemas": {"trigger": {"properties": {"depOpt": {}}, "required": ["depOpt"]}}},
|
||||
["depOpt"],
|
||||
),
|
||||
(
|
||||
"a `required` under `then` does NOT make a member required — the branch may not be taken",
|
||||
{
|
||||
"if": {"properties": {"k": {}}, "required": ["k"]},
|
||||
"then": {"properties": {"m": {}}, "required": ["m"]},
|
||||
},
|
||||
["k", "m"],
|
||||
),
|
||||
(
|
||||
"a `required` under `else` does not either, and `then`/`else` are mutually exclusive",
|
||||
{
|
||||
"if": {"properties": {"k": {}}},
|
||||
"then": {"properties": {"m": {}}, "required": ["m"]},
|
||||
"else": {"properties": {"m": {}}},
|
||||
},
|
||||
["k", "m"],
|
||||
),
|
||||
(
|
||||
"a `required` under `if` selects a branch, it does not oblige the client",
|
||||
{"if": {"properties": {"k": {}}, "required": ["k"]}, "then": {"properties": {"m": {}}}},
|
||||
["k", "m"],
|
||||
),
|
||||
(
|
||||
"an INLINE OBJECT under `properties` is recursed into, reported with a dotted path",
|
||||
{
|
||||
"properties": {
|
||||
"top": {},
|
||||
"nested": {"properties": {"a": {}, "b": {}}, "required": ["a"]},
|
||||
},
|
||||
"required": ["top", "nested"],
|
||||
},
|
||||
["nested.b"],
|
||||
),
|
||||
(
|
||||
"a REQUIRED literal `a.b` cannot mask a nested `a` -> `b` — the namespaces never merge",
|
||||
{"properties": {"a.b": {}, "a": {"properties": {"b": {}}}}, "required": ["a.b", "a"]},
|
||||
["a.b"],
|
||||
),
|
||||
(
|
||||
"`prefixItems` positions are conjunctive, so a `required` there really does bind",
|
||||
{
|
||||
"type": "array",
|
||||
"prefixItems": [{"properties": {"pReq": {}, "pOpt": {}}, "required": ["pReq"]}],
|
||||
},
|
||||
["pOpt"],
|
||||
),
|
||||
(
|
||||
"a dotted nested member cannot be confused with a top-level member of the same name",
|
||||
{
|
||||
"properties": {"b": {}, "nested": {"properties": {"b": {}}}},
|
||||
"required": ["b", "nested"],
|
||||
},
|
||||
["nested.b"],
|
||||
),
|
||||
(
|
||||
"`patternProperties` names no FIXED members, so it contributes none — same as additionalProperties",
|
||||
{"properties": {"named": {}}, "patternProperties": {"^x-": {"properties": {"notAMember": {}}}}},
|
||||
["named"],
|
||||
),
|
||||
(
|
||||
"`additionalProperties` names no members, so it contributes none",
|
||||
{"properties": {"named": {}}, "additionalProperties": {"properties": {"notAMember": {}}}},
|
||||
["named"],
|
||||
),
|
||||
(
|
||||
"`not` cannot make a member required",
|
||||
{"properties": {"a": {}}, "not": {"required": ["a"]}},
|
||||
["a"],
|
||||
),
|
||||
(
|
||||
"properties declared under `else` count for the same reason `then` does",
|
||||
{"if": {}, "else": {"properties": {"elseOpt": {}}}},
|
||||
["elseOpt"],
|
||||
),
|
||||
(
|
||||
"properties declared under `then` are reachable on some branch, so they count",
|
||||
{"if": {"properties": {"kind": {}}}, "then": {"properties": {"conditional": {}}}},
|
||||
["conditional", "kind"],
|
||||
),
|
||||
(
|
||||
"top-level properties and an allOf arm are merged, not either/or",
|
||||
{"properties": {"top": {}}, "required": ["top"], "allOf": [{"properties": {"inner": {}}}]},
|
||||
["inner"],
|
||||
),
|
||||
],
|
||||
)
|
||||
def test_composition_is_resolved_the_way_JSON_Schema_means_it(case: str, schema: dict, expected: list[str]) -> None:
|
||||
"""Pin `_optional_of`/`_resolve` against constructed schemas, not against today's document.
|
||||
|
||||
The document exercises exactly one shape (a two-arm `allOf` in the artwork upload body), so
|
||||
every other branch of this resolver would otherwise be unexercised prose. Both of the first
|
||||
version's defects are here as cases: the nested `allOf` it could not reach, and the `oneOf`
|
||||
whose `required` it unioned instead of intersecting — which marked a member required because
|
||||
ONE arm required it, hiding the arm that lets a client drop it.
|
||||
"""
|
||||
assert sorted(_optional_of(schema)) == expected, case
|
||||
|
||||
|
||||
_NESTED_PROBE = {
|
||||
"properties": {"outer": {"properties": {"req": {}, "opt": {}}, "required": ["req"]}},
|
||||
"required": ["outer"],
|
||||
}
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("site", "schema"),
|
||||
[
|
||||
("top level", _NESTED_PROBE),
|
||||
("allOf arm", {"allOf": [_NESTED_PROBE]}),
|
||||
("if", {"if": _NESTED_PROBE}),
|
||||
("then", {"then": _NESTED_PROBE}),
|
||||
("else", {"else": _NESTED_PROBE}),
|
||||
("dependentSchemas", {"dependentSchemas": {"trigger": _NESTED_PROBE}}),
|
||||
("oneOf arm", {"oneOf": [_NESTED_PROBE]}),
|
||||
("anyOf arm", {"anyOf": [_NESTED_PROBE]}),
|
||||
("items", {"type": "array", "items": _NESTED_PROBE}),
|
||||
("prefixItems", {"type": "array", "prefixItems": [_NESTED_PROBE]}),
|
||||
],
|
||||
)
|
||||
def test_a_nested_inline_object_survives_EVERY_composition_site(site: str, schema: dict) -> None:
|
||||
"""`_resolve` returns three sets, and the third has to be threaded through every branch.
|
||||
|
||||
A composition site that unions `properties` and `required` but forgets `nested_optional` loses
|
||||
the nested member silently — no error, just a smaller population — and each site is a separate
|
||||
opportunity to forget. Enumerating the sites here is what makes "threaded through every branch"
|
||||
a checked property instead of a claim in a docstring; a NEW composition keyword must be added
|
||||
to this list, and if it is not, the omission is at least visible in one place rather than
|
||||
spread across the resolver.
|
||||
"""
|
||||
assert "outer.opt" in _optional_of(schema), (
|
||||
f"a nested inline object under `{site}` lost its optional member — `nested_optional` is "
|
||||
"not threaded through that branch of `_resolve`"
|
||||
)
|
||||
|
||||
|
||||
def test_a_ref_ARM_is_covered_by_the_component_walk_not_by_this_one() -> None:
|
||||
"""The two walks COMPOSE; neither alone covers an inline body with a `$ref` arm.
|
||||
|
||||
`_resolve` does not resolve `$ref`, so an `allOf` arm that is a `$ref`
|
||||
contributes nothing to the inline key. That looks like a gap and is not: the referenced schema
|
||||
is a named component, so it is seeded by `_request_reachable` and walked by `_optional_members`,
|
||||
and its optional members surface under their own key. Pinned here because the obvious "fix" —
|
||||
resolving refs in this resolver too — would report the same member under two keys, and because
|
||||
a reader checking only one of the two walks would reasonably conclude the case is uncovered.
|
||||
"""
|
||||
doc = _load()
|
||||
doc["components"]["schemas"]["RefArmProbeSchema"] = {
|
||||
"type": "object",
|
||||
"properties": {"probeRequired": {"type": "string"}, "probeOptional": {"type": "string"}},
|
||||
"required": ["probeRequired"],
|
||||
}
|
||||
doc["paths"]["/probe-ref-arm"] = {
|
||||
"post": {
|
||||
"requestBody": {
|
||||
"content": {
|
||||
"application/json": {
|
||||
"schema": {
|
||||
"allOf": [
|
||||
{"$ref": "#/components/schemas/RefArmProbeSchema"},
|
||||
{"properties": {"probeInline": {}}},
|
||||
]
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
droppable = _droppable(doc)
|
||||
assert droppable.get("RefArmProbeSchema") == ["probeOptional"], (
|
||||
"a $ref arm's target must still surface via the component walk — if this is empty, an "
|
||||
"inline body can hide a droppable member behind a $ref"
|
||||
)
|
||||
assert droppable.get("POST /probe-ref-arm (application/json inline body)") == ["probeInline"]
|
||||
|
||||
|
||||
def test_every_disposition_uses_the_closed_vocabulary() -> None:
|
||||
"""A free-text disposition would let a row read as considered while saying nothing."""
|
||||
allowed = {COVERED, CREATE, TRIGGER, COMPUTED}
|
||||
for name, (disposition, why) in DISPOSITIONS.items():
|
||||
assert disposition in allowed, f"{name}: {disposition!r} is not one of {sorted(allowed)}"
|
||||
assert len(why.strip()) >= 40, f"{name}: the reason is too thin to be a decision"
|
||||
|
||||
|
||||
def test_MUTATION_a_planted_optional_member_is_reported_as_MISSING() -> None:
|
||||
"""The checker-guard mutation proof (`testing.guard-ships-with-mutation-proof`).
|
||||
|
||||
Disarming a checker makes it ABSENT rather than red, so the mutation goes into the guarded
|
||||
ARTIFACT: plant a request-reachable schema carrying a member outside `required` and require the
|
||||
derivation to surface it. Mutating the checker's own population instead would be the trap that
|
||||
record names — a shrunken population makes every real row report PHANTOM, so the proof would go
|
||||
red on a false positive while saying nothing about the MISSING detection this row claims.
|
||||
|
||||
The planted schema is reached through a real request body, so this also exercises the
|
||||
transitive `$ref` walk that the hand-written list twice failed to do by eye.
|
||||
"""
|
||||
doc = _load()
|
||||
schemas = doc["components"]["schemas"]
|
||||
|
||||
# Deterministic, because `set` iteration order over strings varies per process: picking the
|
||||
# host with `next(iter(...))` would silently vary WHICH walk depth the proof exercises from run
|
||||
# to run, so a green run would not mean the same thing twice.
|
||||
candidates = sorted(
|
||||
n for n in _request_reachable(doc) if isinstance(schemas.get(n), dict) and schemas[n].get("properties")
|
||||
)
|
||||
assert candidates, "no request-reachable schema with properties — the walk is broken"
|
||||
host = candidates[0]
|
||||
|
||||
schemas["PlantedDroppableRequest"] = {
|
||||
"type": "object",
|
||||
"properties": {"plantedRequired": {"type": "string"}, "plantedOptional": {"type": "string"}},
|
||||
"required": ["plantedRequired"],
|
||||
}
|
||||
# Planted through a `oneOf` LIST rather than a bare dict `$ref`, because `_schema_refs` has two
|
||||
# descent branches and only the dict one was exercised: deleting its list descent — which makes
|
||||
# every `oneOf` reference invisible, 23 of them in the document today — left the whole file
|
||||
# GREEN when measured 2026-08-22. A proof that cannot see half its own walk is the "green for
|
||||
# the wrong reason" shape this repo keeps recording.
|
||||
schemas[host]["properties"]["plantedLink"] = {
|
||||
"oneOf": [{"type": "null"}, {"$ref": "#/components/schemas/PlantedDroppableRequest"}]
|
||||
}
|
||||
|
||||
# Plant into the INLINE request body too, so `_inline_body_members` is exercised rather than
|
||||
# merely present. Without this the whole inline branch could be deleted and every test here
|
||||
# would stay green — the exact shape the `oneOf` list branch was in before it was planted
|
||||
# through.
|
||||
inline_host = next(
|
||||
(
|
||||
media["schema"]
|
||||
for operations in doc["paths"].values()
|
||||
for operation in operations.values()
|
||||
if isinstance(operation, dict) and operation.get("requestBody")
|
||||
for media in (operation["requestBody"].get("content") or {}).values()
|
||||
if isinstance(media.get("schema"), dict) and "$ref" not in media["schema"]
|
||||
),
|
||||
None,
|
||||
)
|
||||
assert inline_host is not None, "no inline request body in the document — re-target this plant"
|
||||
inline_host.setdefault("properties", {})["plantedInlineOptional"] = {"type": "string"}
|
||||
|
||||
droppable = _droppable(doc)
|
||||
planted_inline = [k for k, v in droppable.items() if "plantedInlineOptional" in v]
|
||||
assert planted_inline, (
|
||||
"the member planted in an INLINE request body was NOT surfaced — `_inline_body_members` is "
|
||||
"not reaching inline bodies, so a body declared without a $ref is invisible to this guard"
|
||||
)
|
||||
assert "PlantedDroppableRequest" in droppable, (
|
||||
"the planted schema was NOT surfaced — the derivation cannot see a droppable member, so a "
|
||||
"green run of this file proves nothing"
|
||||
)
|
||||
assert droppable["PlantedDroppableRequest"] == ["plantedOptional"]
|
||||
assert "PlantedDroppableRequest" not in DISPOSITIONS
|
||||
@@ -50,6 +50,11 @@ if "-d" in args:
|
||||
payload = args[args.index("-d") + 1]
|
||||
|
||||
if is_post:
|
||||
# A POST can be scripted to fail (`post_fail` holds a URL substring), so the two write paths
|
||||
# can be broken independently — the shape ersatztv#792 is about.
|
||||
fail_on = (state / "post_fail").read_text().strip() if (state / "post_fail").exists() else ""
|
||||
if fail_on and fail_on in url:
|
||||
sys.exit(22)
|
||||
with (state / "posts.jsonl").open("a") as fh:
|
||||
fh.write(json.dumps({"url": url, "payload": json.loads(payload)}) + "\n")
|
||||
print("{}")
|
||||
@@ -120,6 +125,10 @@ def gitea(tmp_path):
|
||||
def set_pr_state(self, value):
|
||||
(state / "pr_state").write_text(value)
|
||||
|
||||
def fail_posts_to(self, url_substring):
|
||||
"""Make POSTs whose URL contains this substring fail, as curl -f does on a 4xx/5xx."""
|
||||
(state / "post_fail").write_text(url_substring)
|
||||
|
||||
def set_base_sequence(self, *refs):
|
||||
"""Base branch per PR GET. 'MISSING' omits `.base` from the response entirely."""
|
||||
(state / "pr_bases").write_text(" ".join(refs))
|
||||
@@ -177,8 +186,10 @@ def test_refuses_when_head_moves_mid_flight(gitea):
|
||||
assert result.returncode != 0
|
||||
assert "UNREVIEWED" in result.stderr
|
||||
assert gitea.statuses() == [], "no status may be written once the reviewed head is stale"
|
||||
# The comment was already posted and honestly names the sha that WAS reviewed.
|
||||
assert SHA_A[:7] in gitea.comments()[0]["payload"]["body"]
|
||||
# And no comment either, since ersatztv#792. This assertion used to say the opposite — the
|
||||
# comment went first, so a refusal left `Review-verdict: MERGEABLE @ <sha>` on the PR with no
|
||||
# status behind it, which reads to an operator as consent that was never granted.
|
||||
assert gitea.comments() == [], "a refusal must leave no verdict comment standing in for a status"
|
||||
|
||||
|
||||
def test_never_retargets_the_verdict_at_the_new_head(gitea):
|
||||
@@ -412,3 +423,110 @@ def test_a_reread_that_LOSES_a_field_refuses_instead_of_posting(head_seq, base_s
|
||||
assert not gitea.statuses(), (
|
||||
f"a status was written despite the re-read carrying no {field} — this is the fail-open the -n conjunct created"
|
||||
)
|
||||
|
||||
|
||||
# --- ersatztv#792: no path may write no status and report success ------------------------------
|
||||
#
|
||||
# The issue was filed on an observed "printed the refusal AND exited 0". Re-measured on the tree
|
||||
# that fixed the re-read fence: every refusal already exits NON-ZERO, and the exit-0 came from the
|
||||
# caller's pipeline, not from the script. That is worth an executed contract rather than a second
|
||||
# reading of the source — `die` is one line away from being edited into a `return`, and this file is
|
||||
# where that would be caught. The parametrisation covers each refusal REASON, not one
|
||||
# representative, because those paths were added at four different times and only the shared helper
|
||||
# makes them agree today.
|
||||
#
|
||||
# SCOPE, since "every path" would overclaim: these are the eight refusal MODES reachable through the
|
||||
# real entry point. The source also exits non-zero for a usage error (2), a failing `jq` (its own
|
||||
# status, under `set -e` + `pipefail`), and a signal (128+n); none of those is a refusal DECISION,
|
||||
# and only the non-zero-ness is common to all of them.
|
||||
|
||||
|
||||
def _drive(gitea, mode):
|
||||
if mode == "head-moved":
|
||||
gitea.set_head_sequence(SHA_A, SHA_B)
|
||||
elif mode == "reread-failed":
|
||||
gitea.set_head_sequence(SHA_A, "GONE")
|
||||
elif mode == "reread-lost-head":
|
||||
gitea.set_head_sequence(SHA_A, "NOHEAD")
|
||||
elif mode == "base-retargeted":
|
||||
gitea.set_base_sequence("main", "some-feature-branch")
|
||||
elif mode == "reread-lost-base":
|
||||
gitea.set_base_sequence("main", "MISSING")
|
||||
elif mode == "first-read-failed":
|
||||
gitea.set_head_sequence("GONE")
|
||||
elif mode == "pr-closed":
|
||||
gitea.set_pr_state("closed")
|
||||
elif mode == "status-post-failed":
|
||||
gitea.fail_posts_to("/statuses/")
|
||||
else: # pragma: no cover - a typo in the parametrisation must not pass silently
|
||||
raise AssertionError(f"unknown mode {mode}")
|
||||
return gitea.run("42", "MERGEABLE")
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"mode",
|
||||
[
|
||||
"head-moved",
|
||||
"reread-failed",
|
||||
"reread-lost-head",
|
||||
"base-retargeted",
|
||||
"reread-lost-base",
|
||||
"first-read-failed",
|
||||
"pr-closed",
|
||||
"status-post-failed",
|
||||
],
|
||||
)
|
||||
def test_every_REFUSAL_MODE_that_writes_no_status_exits_non_zero(gitea, mode):
|
||||
result = _drive(gitea, mode)
|
||||
assert gitea.statuses() == [], f"{mode} wrote a status it had no business writing"
|
||||
assert result.returncode != 0, (
|
||||
f"{mode} wrote no status and reported SUCCESS — anything checking $? concludes the verdict "
|
||||
f"posted. stdout={result.stdout!r} stderr={result.stderr!r}"
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"mode",
|
||||
[
|
||||
"head-moved",
|
||||
"reread-failed",
|
||||
"reread-lost-head",
|
||||
"base-retargeted",
|
||||
"reread-lost-base",
|
||||
"status-post-failed",
|
||||
],
|
||||
)
|
||||
def test_no_refusal_leaves_a_VERDICT_COMMENT_standing_in_for_the_status(gitea, mode):
|
||||
"""The half-state, which is the part of #792 that was really broken.
|
||||
|
||||
The comment is not the gate — `review-verdict/h10` is — but `Review-verdict: MERGEABLE @ <head>`
|
||||
sitting on a PR reads exactly like consent. Every mode here is one where the status is refused
|
||||
after the head has been resolved, i.e. every mode that could once have left that comment behind.
|
||||
"""
|
||||
_drive(gitea, mode)
|
||||
assert gitea.comments() == [], f"{mode} left an orphaned verdict comment: {gitea.comments()}"
|
||||
|
||||
|
||||
def test_the_status_is_written_BEFORE_the_comment(gitea):
|
||||
"""Ordering is the mechanism, so it is asserted rather than described.
|
||||
|
||||
Status-then-comment makes the only reachable half-state the safe one: a status with no comment
|
||||
leaves the merge hook's condition (c) with nothing to classify, which is an `ask`. The reverse
|
||||
order manufactures the appearance of a granted verdict.
|
||||
"""
|
||||
assert gitea.run("42", "MERGEABLE").returncode == 0
|
||||
urls = [p["url"] for p in gitea.posts()]
|
||||
assert len(urls) == 2, urls
|
||||
assert "/statuses/" in urls[0], f"the status must be written first, got {urls}"
|
||||
assert "/comments" in urls[1], f"the comment must be written second, got {urls}"
|
||||
|
||||
|
||||
def test_a_failed_COMMENT_after_a_written_status_is_still_an_error(gitea):
|
||||
"""The surviving half-state is safe, not silent: the operator is told to re-run."""
|
||||
gitea.fail_posts_to("/comments")
|
||||
result = gitea.run("42", "MERGEABLE")
|
||||
assert result.returncode != 0
|
||||
assert len(gitea.statuses()) == 1, "the status was already written and must not be rolled back"
|
||||
assert gitea.comments() == []
|
||||
assert "COMMENT could not be posted" in result.stderr
|
||||
assert "ask" in result.stderr.lower(), "it must say what the gate will do, not just that a call failed"
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import type { Complete } from './completeRequest';
|
||||
import { ApiError, request, requestWithMeta, type ResponseWithMeta } from './client';
|
||||
import type { components } from './generated/v1';
|
||||
|
||||
@@ -60,7 +61,7 @@ export function getBlockItemsWithMeta(id: number): Promise<ResponseWithMeta<Bloc
|
||||
*/
|
||||
export function replaceBlock(
|
||||
id: number,
|
||||
body: ReplaceBlockRequest,
|
||||
body: Complete<ReplaceBlockRequest>,
|
||||
ifMatch?: string | null
|
||||
): Promise<ResponseWithMeta<BlockWithItems>> {
|
||||
return requestWithMeta<BlockWithItems>(`/api/v1/blocks/${id}`, {
|
||||
@@ -70,7 +71,7 @@ export function replaceBlock(
|
||||
});
|
||||
}
|
||||
|
||||
export function previewBlock(id: number, body: ReplaceBlockRequest): Promise<BlockPreviewItem[]> {
|
||||
export function previewBlock(id: number, body: Complete<ReplaceBlockRequest>): Promise<BlockPreviewItem[]> {
|
||||
return request<BlockPreviewItem[]>(`/api/v1/blocks/${id}/preview`, { body, method: 'POST' });
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,146 @@
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import type { Complete } from './completeRequest';
|
||||
import type { components } from './generated/v1';
|
||||
|
||||
/**
|
||||
* #807 guard: the mutation proof for `Complete<T>` (`./completeRequest.ts`).
|
||||
*
|
||||
* `Complete<T>` is enforced by the COMPILER, not at runtime, so a vitest assertion cannot prove it
|
||||
* works — a passing `expect` here would say nothing about whether the type still rejects an
|
||||
* incomplete request. The proof is therefore the `@ts-expect-error` directives below, and it is a
|
||||
* real one in the sense `testing.guard-ships-with-mutation-proof` requires: each directive
|
||||
* INTRODUCES THE DEFECT the guard exists to catch, and `tsc` fails the build if the defect is NOT
|
||||
* reported. A `@ts-expect-error` on a line that compiles cleanly is itself an error
|
||||
* (`TS2578: Unused '@ts-expect-error' directive`). That mutation runs on every
|
||||
* `npm run typecheck`, which is a marked CI step (`docker-build.yml` → `ci-step-ran.sh mark
|
||||
* typecheck`), rather than being executed once by hand and asserted in prose.
|
||||
*
|
||||
* Which mutation reddens which case, measured 2026-08-22 against THIS file and not generalised:
|
||||
*
|
||||
* | mutation of `Complete<T>` | cases that go red |
|
||||
* |---|---|
|
||||
* | `{ [K in keyof T]: T[K] }` (drop `Required` — the realistic weakening) | 2 and 4 |
|
||||
* | `T` (delete the mapped type) | 2 and 4 |
|
||||
* | `Partial<T>` | 2 and 4 |
|
||||
*
|
||||
* Cases 2 and 4 are the load-bearing pair — one synthetic, one a real generated schema. Cases 1, 3
|
||||
* and 5 stay green under all three BY DESIGN: 1 and 3 are controls that must stay green, and 5's
|
||||
* excess-property check does not depend on `Required`. So do not read "5 cases, 2 red" as thin
|
||||
* coverage, and equally do not restate this as "every case reddens under any weakening" — that is
|
||||
* false, and a proof that overstates its own reach stops being re-examined.
|
||||
*
|
||||
* Case 4 reddened under `Partial<T>` ONLY until it was re-pinned from a required member to a
|
||||
* genuinely optional one; see its own comment for why that distinction is the whole game.
|
||||
*
|
||||
* What this file does NOT prove is that `Complete<T>` is APPLIED at every site that needs it.
|
||||
* Reverting one screen to its pre-#807 form leaves this guard green — the population of
|
||||
* construction sites is not derived from anything. That residue is tracked, not implied closed.
|
||||
*
|
||||
* Both directions are covered, because a MISSING field and a PHANTOM field are opposite defects:
|
||||
* cases 2 and 4 are the missing direction (the #807 defect proper — a silently dropped field on a
|
||||
* full-replace write), case 5 is the phantom direction.
|
||||
*
|
||||
* Cases 1-3 use a SYNTHETIC type so the proof cannot be invalidated by an unrelated schema change,
|
||||
* and cases 4-5 use a REAL generated request type so the proof is demonstrably wired to
|
||||
* `./generated/v1.d.ts` rather than only to a local fixture. Both are needed: the synthetic case
|
||||
* alone would pass even if the generated types stopped being importable here, and the real case
|
||||
* alone would break for reasons that have nothing to do with `Complete<T>`.
|
||||
*
|
||||
* Which real schemas carry a genuinely optional member is DERIVED, not listed here or anywhere
|
||||
* else in prose: `scripts/tests/test_optional_request_members.py` computes it from the OpenAPI
|
||||
* document every run and fails until each has a stated disposition. Three hand-written versions of
|
||||
* that list were wrong (#807), so do not add a fourth to this comment.
|
||||
*
|
||||
* Note the deliberate shape of the synthetic type: `optionalMember?` is what a schema property
|
||||
* emits as when it is absent from its `required` array in the OpenAPI document. Most request
|
||||
* properties are not, which is why most builders already typecheck and the gap reads as closed on
|
||||
* inspection. Case 2 is the whole point of this guard: it is the case that compiles clean WITHOUT
|
||||
* `Complete<T>`, which is why case 3 asserts exactly that. See `./completeRequest.ts`.
|
||||
*/
|
||||
|
||||
type SyntheticRequest = {
|
||||
requiredMember: string;
|
||||
optionalMember?: number;
|
||||
nullableMember: string | null;
|
||||
};
|
||||
|
||||
// Case 1 — every member named, the optional one explicitly `undefined`: MUST compile.
|
||||
// This is not filler. If `Complete<T>` were written so that an optional member had to carry a
|
||||
// real value, every builder would be forced to invent one, and the guard would be abandoned.
|
||||
const case1: Complete<SyntheticRequest> = {
|
||||
requiredMember: 'x',
|
||||
optionalMember: undefined,
|
||||
nullableMember: null
|
||||
};
|
||||
|
||||
// Case 2 — the OPTIONAL member omitted: MUST be an error. This is #807's defect exactly.
|
||||
// @ts-expect-error omitting an optional member of a Complete<T> request must not compile
|
||||
const case2: Complete<SyntheticRequest> = {
|
||||
requiredMember: 'x',
|
||||
nullableMember: null
|
||||
};
|
||||
|
||||
// Case 3 — the SAME omission against the bare type: MUST compile.
|
||||
// This is the negative control, and without it cases 2 and 4 prove nothing: it demonstrates that
|
||||
// the error in case 2 comes from `Complete<T>` and not from some other property of the object.
|
||||
// It is the "compiles clean and is silently dropped" state #807 was filed about.
|
||||
const case3: SyntheticRequest = {
|
||||
requiredMember: 'x',
|
||||
nullableMember: null
|
||||
};
|
||||
|
||||
// Case 4 — a REAL generated request type with a genuinely OPTIONAL member omitted: MUST error.
|
||||
//
|
||||
// `MultiCollectionItemRequest.weight` sits outside its schema's `required` array (it is a defaulted
|
||||
// ctor param), so it emits as `weight?: number` and omitting it is legal against the bare type.
|
||||
// That makes this case DISCRIMINATING: it goes red under the realistic weakening
|
||||
// `{ [K in keyof T]: T[K] }`, not only under `Partial<T>`. It is also the exact field whose loss
|
||||
// was live before #807 — dropping it from `MultiCollectionsScreen.toItemRequest` typechecked clean
|
||||
// and would have reset every weight to 1 on the next full-replace save.
|
||||
//
|
||||
// An earlier version of this case pinned `ReplaceDecoTemplateRequest.name`, a REQUIRED member, and
|
||||
// then reasoned in a comment that no real request type could discriminate "because none has an
|
||||
// optional member". That was false when written — `scripts/tests/test_optional_request_members.py`
|
||||
// derives the schemas that do, and there are several — and it cost the proof its most valuable
|
||||
// case. No count is given here on purpose: that population is derived, and every hand-written
|
||||
// version of it on this issue has been wrong. Do not re-pin this to a required member: check
|
||||
// the schema's `required` array
|
||||
// first, and prefer a member that a builder could actually drop.
|
||||
//
|
||||
// If `weight` ever becomes required or disappears, this directive goes unused and tsc reports it.
|
||||
// That is the correct outcome — re-pin to another genuinely optional member rather than deleting
|
||||
// the case.
|
||||
// @ts-expect-error omitting the optional `weight` from a real
|
||||
// Complete<MultiCollectionItemRequest> must not compile
|
||||
const case4: Complete<components['schemas']['MultiCollectionItemRequest']> = {
|
||||
collectionId: 1,
|
||||
smartCollectionId: null,
|
||||
scheduleAsGroup: false,
|
||||
playbackOrder: 'Chronological'
|
||||
};
|
||||
|
||||
// Case 5 — the PHANTOM direction: a field the schema does not accept: MUST be an error.
|
||||
// The directive sits on the OFFENDING PROPERTY, not on the declaration: an excess-property error
|
||||
// (TS2353) is reported at the property, whereas a missing-property error (TS2741, case 4) is
|
||||
// reported at the declaration. Placing it on the declaration instead made tsc report the directive
|
||||
// as unused AND the excess property as an error — two reds, not a proof.
|
||||
const case5: Complete<components['schemas']['ReplaceDecoTemplateRequest']> = {
|
||||
name: 'x',
|
||||
items: [],
|
||||
// @ts-expect-error a field absent from the schema must not compile
|
||||
fieldTheSchemaDoesNotHave: 1
|
||||
};
|
||||
|
||||
describe('#807 Complete<T> request guard', () => {
|
||||
it('is proven by the @ts-expect-error directives above, which npm run typecheck executes', () => {
|
||||
// These runtime assertions exist only so the compile-time cases are REFERENCED. Without a use,
|
||||
// `noUnusedLocals`/lint could remove them and the proof would vanish silently — the exact
|
||||
// "a guard that never executed proves nothing" failure this repo has shipped before (#751).
|
||||
// They deliberately assert almost nothing about behaviour: the guard is the compiler.
|
||||
expect(case1.requiredMember).toBe('x');
|
||||
expect(case3.requiredMember).toBe('x');
|
||||
expect(case2).toBeDefined();
|
||||
expect(case4).toBeDefined();
|
||||
expect(case5).toBeDefined();
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,70 @@
|
||||
/**
|
||||
* `Complete<T>` — a request type with every member REQUIRED, so a builder that forgets one
|
||||
* fails `npm run typecheck` instead of silently dropping the field (issue #807).
|
||||
*
|
||||
* ## The defect this closes
|
||||
*
|
||||
* The SPA builds write-request bodies field-by-field as object literals against the generated
|
||||
* schema types in `./generated/v1.d.ts`. Those bodies are sent to full-replace endpoints, so a
|
||||
* field the builder never sets is not "left alone" — it is written as its default. That is #754's
|
||||
* mechanism: a hand-maintained mirror drifts from a DTO by one field, the write returns HTTP 200,
|
||||
* and the loss surfaces hours later. See `testing.full-replace-asserts-field-list`.
|
||||
*
|
||||
* This is not hypothetical. Measured 2026-08-22, before this change: deleting
|
||||
* `weight: clampWeight(item.weight)` from `MultiCollectionsScreen.toItemRequest` typechecked
|
||||
* CLEAN, and that PUT replaces the item list — so every weight would have reset to 1 on the next
|
||||
* save. The screen carries a prose comment warning about exactly that; a comment is not a check.
|
||||
* `FFmpegProfilesScreen` had the same exposure on `qsvPreferNativeDecoder`.
|
||||
*
|
||||
* ## What determines whether the compiler can see it
|
||||
*
|
||||
* A member is omittable exactly when the generated type marks it `?:`, and that comes from the
|
||||
* `required` array of the schema in `ErsatzTV/wwwroot/openapi/v1.json`.
|
||||
* `web/scripts/generate-openapi-types.mjs` is a pure pass-through — its whole contribution is
|
||||
* `const optional = required.has(name) ? '' : '?'` — so the determinant is the ASP.NET-produced
|
||||
* OpenAPI document, NOT the generator script. A property lands outside `required` because of how
|
||||
* its DTO is modelled — typically a defaulted constructor parameter, as `weight` and
|
||||
* `qsvPreferNativeDecoder` are. Do not maintain a count of such properties here: two hand-written
|
||||
* lists of them were wrong (#807), and `scripts/tests/test_optional_request_members.py` derives
|
||||
* the current set from the OpenAPI document on every run.
|
||||
*
|
||||
* NOT EVERY optional member is a droppable field. Where the optional members are computed get-only
|
||||
* properties on the C# record — `ArtworkContentTypeModel`'s `IsExternalUrl`, `HasContentType`,
|
||||
* `UrlWithContentType` — nothing deserializes them, so omitting them drops nothing, and applying
|
||||
* `Complete<T>` there would be a BUG: it would force a caller to fabricate server-computed values
|
||||
* in an outbound request. That test file records the disposition per schema.
|
||||
*
|
||||
* Note the direction of the trap: a nullable property usually emits as REQUIRED-and-nullable
|
||||
* (`"name": null | string`), so most builders are checked and the gap looks closed on inspection.
|
||||
* The unchecked ones are a small minority hiding inside a large majority of checked ones.
|
||||
*
|
||||
* ## Both directions
|
||||
*
|
||||
* - MISSING (a schema field the builder never sets) — `Complete<T>` makes it a hard error, and it
|
||||
* does so wherever the value is assigned, including through a spread or an inferred local.
|
||||
* - PHANTOM (a field the builder sets that the schema does not accept) — TypeScript's excess
|
||||
* property check already reports this, but ONLY for a "fresh" object literal in a typed
|
||||
* position. Returning a literal from a generic `.map` callback is NOT such a position: `map<U>`
|
||||
* infers `U` FROM the callback's return, so the target element type never contextually types the
|
||||
* literal, and the check does not fire — with or without a spread in it. Measured on the
|
||||
* pre-#807 tree: several construction sites accepted a phantom field, three of them with no
|
||||
* spread and no inferred local. Annotating each site's return type is what restores this
|
||||
* direction. (No tally: "construction site" is not a derived population, and the count in an
|
||||
* earlier draft was wrong.)
|
||||
*
|
||||
* ## Explicit `undefined` is still allowed, deliberately
|
||||
*
|
||||
* `T[K]` is preserved, so an optional member may be written as `field: undefined`. The point is
|
||||
* not to forbid omitting a value, it is to forbid omitting the DECISION — an unmentioned field is
|
||||
* an oversight, `field: undefined` is a choice a reviewer can see.
|
||||
*
|
||||
* ## Shallow, and what that costs
|
||||
*
|
||||
* `Complete<T>` does not recurse into `items: Array<ItemRequest>`; each nested item builder is
|
||||
* annotated `Complete<ItemRequest>` directly instead. This is a scope choice, not a claim that a
|
||||
* deep variant is infeasible — probed 2026-08-22, the shallow mapped form distributes correctly
|
||||
* over unions, leaves `number[]` an array, preserves `readonly`, and passes an index signature
|
||||
* through, so a deep variant is not obviously blocked. A NEW nested request type therefore needs
|
||||
* its own annotation and nothing forces that; see the residue noted on the decision record.
|
||||
*/
|
||||
export type Complete<T> = { [K in keyof Required<T>]: T[K] };
|
||||
@@ -1,3 +1,4 @@
|
||||
import type { Complete } from './completeRequest';
|
||||
import { ApiError, request, requestWithMeta, type ResponseWithMeta } from './client';
|
||||
import type { components } from './generated/v1';
|
||||
|
||||
@@ -57,7 +58,7 @@ export function getDecoTemplateItemsWithMeta(id: number): Promise<ResponseWithMe
|
||||
*/
|
||||
export function replaceDecoTemplate(
|
||||
id: number,
|
||||
body: ReplaceDecoTemplateRequest,
|
||||
body: Complete<ReplaceDecoTemplateRequest>,
|
||||
ifMatch?: string | null
|
||||
): Promise<ResponseWithMeta<DecoTemplateWithItems>> {
|
||||
return requestWithMeta<DecoTemplateWithItems>(`/api/v1/deco-templates/${id}`, {
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import type { Complete } from './completeRequest';
|
||||
import { ApiError, request } from './client';
|
||||
import type { components } from './generated/v1';
|
||||
|
||||
@@ -42,7 +43,7 @@ export function deleteDeco(id: number): Promise<void> {
|
||||
return request<void>(`/api/v1/decos/${id}`, { method: 'DELETE' });
|
||||
}
|
||||
|
||||
export function replaceDeco(id: number, body: ReplaceDecoRequest): Promise<Deco> {
|
||||
export function replaceDeco(id: number, body: Complete<ReplaceDecoRequest>): Promise<Deco> {
|
||||
return request<Deco>(`/api/v1/decos/${id}`, { body, method: 'PUT' });
|
||||
}
|
||||
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import type { Complete } from './completeRequest';
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest';
|
||||
import {
|
||||
createFFmpegProfile,
|
||||
@@ -19,7 +20,7 @@ function noContent(): Response {
|
||||
return new Response(null, { status: 204 });
|
||||
}
|
||||
|
||||
const sampleRequest: CreateFFmpegProfileRequest = {
|
||||
const sampleRequest: Complete<CreateFFmpegProfileRequest> = {
|
||||
allowBFrames: false,
|
||||
audioBitrate: 192,
|
||||
audioBufferSize: 384,
|
||||
@@ -37,6 +38,7 @@ const sampleRequest: CreateFFmpegProfileRequest = {
|
||||
normalizeVideo: true,
|
||||
padMode: 'Software',
|
||||
qsvExtraHardwareFrames: null,
|
||||
qsvPreferNativeDecoder: true,
|
||||
resolutionId: 1,
|
||||
scalingBehavior: 'ScaleAndPad',
|
||||
tonemapAlgorithm: 'Linear',
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import type { Complete } from './completeRequest';
|
||||
import { ApiError, request } from './client';
|
||||
import type { components } from './generated/v1';
|
||||
|
||||
@@ -14,11 +15,11 @@ export function getFFmpegProfile(id: number): Promise<FFmpegProfileDetail> {
|
||||
return request<FFmpegProfileDetail>(`/api/v1/ffmpeg/profiles/${id}`);
|
||||
}
|
||||
|
||||
export function createFFmpegProfile(body: CreateFFmpegProfileRequest): Promise<FFmpegProfileDetail> {
|
||||
export function createFFmpegProfile(body: Complete<CreateFFmpegProfileRequest>): Promise<FFmpegProfileDetail> {
|
||||
return request<FFmpegProfileDetail>('/api/v1/ffmpeg/profiles', { body, method: 'POST' });
|
||||
}
|
||||
|
||||
export function updateFFmpegProfile(id: number, body: UpdateFFmpegProfileRequest): Promise<FFmpegProfileDetail> {
|
||||
export function updateFFmpegProfile(id: number, body: Complete<UpdateFFmpegProfileRequest>): Promise<FFmpegProfileDetail> {
|
||||
return request<FFmpegProfileDetail>(`/api/v1/ffmpeg/profiles/${id}`, { body, method: 'PUT' });
|
||||
}
|
||||
|
||||
|
||||
@@ -6,6 +6,7 @@ export * from './channels';
|
||||
export * from './channelTemplates';
|
||||
export * from './client';
|
||||
export * from './collections';
|
||||
export * from './completeRequest';
|
||||
export * from './dashboard';
|
||||
export * from './decos';
|
||||
export * from './decoTemplates';
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import type { Complete } from './completeRequest';
|
||||
import { ApiError, request } from './client';
|
||||
import type { components } from './generated/v1';
|
||||
import type { RemoteFamily } from '../mediaSources/familyMeta';
|
||||
@@ -101,7 +102,7 @@ export function getRemoteLibraries(family: RemoteFamily, sourceId: number): Prom
|
||||
export function replaceRemoteLibraryPreferences(
|
||||
family: RemoteFamily,
|
||||
sourceId: number,
|
||||
body: ReplaceRemoteLibraryPreferencesRequest
|
||||
body: Complete<ReplaceRemoteLibraryPreferencesRequest>
|
||||
): Promise<RemoteLibrary[]> {
|
||||
return request<RemoteLibrary[]>(`/api/v1/media-sources/${family}/${sourceId}/libraries`, { body, method: 'PUT' });
|
||||
}
|
||||
@@ -113,7 +114,7 @@ export function getPathReplacements(family: RemoteFamily, sourceId: number): Pro
|
||||
export function replacePathReplacements(
|
||||
family: RemoteFamily,
|
||||
sourceId: number,
|
||||
body: ReplacePathReplacementsRequest
|
||||
body: Complete<ReplacePathReplacementsRequest>
|
||||
): Promise<PathReplacement[]> {
|
||||
return request<PathReplacement[]>(`/api/v1/media-sources/${family}/${sourceId}/path-replacements`, {
|
||||
body,
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import type { Complete } from './completeRequest';
|
||||
import { ApiError, request, requestWithMeta, type ResponseWithMeta } from './client';
|
||||
import type { components } from './generated/v1';
|
||||
|
||||
@@ -53,7 +54,7 @@ export function createMultiCollection(body: CreateMultiCollectionRequest): Promi
|
||||
*/
|
||||
export function updateMultiCollection(
|
||||
id: number,
|
||||
body: UpdateMultiCollectionRequest,
|
||||
body: Complete<UpdateMultiCollectionRequest>,
|
||||
ifMatch?: string | null
|
||||
): Promise<ResponseWithMeta<MultiCollection>> {
|
||||
return requestWithMeta<MultiCollection>(`/api/v1/multi-collections/${id}`, {
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import type { Complete } from './completeRequest';
|
||||
import { ApiError, request, requestWithMeta, type ResponseWithMeta } from './client';
|
||||
import type { components } from './generated/v1';
|
||||
|
||||
@@ -55,7 +56,7 @@ export function createPlaylist(body: CreatePlaylistRequest): Promise<Playlist> {
|
||||
// the new ETag for a subsequent save (issue #253).
|
||||
export function updatePlaylist(
|
||||
id: number,
|
||||
body: ReplacePlaylistRequest,
|
||||
body: Complete<ReplacePlaylistRequest>,
|
||||
ifMatch?: string | null
|
||||
): Promise<ResponseWithMeta<PlaylistItem[]>> {
|
||||
return requestWithMeta<PlaylistItem[]>(`/api/v1/playlists/${id}`, {
|
||||
@@ -69,7 +70,7 @@ export function deletePlaylist(id: number): Promise<void> {
|
||||
return request<void>(`/api/v1/playlists/${id}`, { method: 'DELETE' });
|
||||
}
|
||||
|
||||
export function previewPlaylist(body: ReplacePlaylistRequest): Promise<PlaylistPreviewItem[]> {
|
||||
export function previewPlaylist(body: Complete<ReplacePlaylistRequest>): Promise<PlaylistPreviewItem[]> {
|
||||
return request<PlaylistPreviewItem[]>('/api/v1/playlists/preview', { body, method: 'POST' });
|
||||
}
|
||||
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import type { Complete } from './completeRequest';
|
||||
import { useCallback, useEffect, useRef, useState } from 'react';
|
||||
import { ApiError, request, requestWithMeta, type ResponseWithMeta } from './client';
|
||||
import type { components } from './generated/v1';
|
||||
@@ -179,7 +180,7 @@ export function getAlternateSchedulesWithMeta(
|
||||
*/
|
||||
export function replaceAlternateSchedules(
|
||||
playoutId: number,
|
||||
body: ReplacePlayoutAlternateSchedulesRequest,
|
||||
body: Complete<ReplacePlayoutAlternateSchedulesRequest>,
|
||||
ifMatch?: string | null
|
||||
): Promise<ResponseWithMeta<PlayoutAlternateSchedule[]>> {
|
||||
return requestWithMeta<PlayoutAlternateSchedule[]>(`/api/v1/playouts/${playoutId}/alternate-schedules`, {
|
||||
@@ -204,7 +205,7 @@ export function getPlayoutTemplatesWithMeta(playoutId: number): Promise<Response
|
||||
*/
|
||||
export function replacePlayoutTemplates(
|
||||
playoutId: number,
|
||||
body: ReplacePlayoutTemplatesRequest,
|
||||
body: Complete<ReplacePlayoutTemplatesRequest>,
|
||||
ifMatch?: string | null
|
||||
): Promise<ResponseWithMeta<PlayoutTemplate[]>> {
|
||||
return requestWithMeta<PlayoutTemplate[]>(`/api/v1/playouts/${playoutId}/templates`, {
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import type { Complete } from './completeRequest';
|
||||
import { ApiError, request, requestWithMeta, type ResponseWithMeta } from './client';
|
||||
import type { components } from './generated/v1';
|
||||
// FillerPreset is exported by ./pickers — import (don't re-export) to avoid a duplicate `export *`
|
||||
@@ -67,7 +68,7 @@ export function addScheduleItem(scheduleId: number, body: ScheduleItemRequest):
|
||||
// response (see SchedulesScreen.tsx `save()`), not reuse the ids it submitted.
|
||||
export function replaceScheduleItems(
|
||||
scheduleId: number,
|
||||
body: ReplaceScheduleItemsRequest,
|
||||
body: Complete<ReplaceScheduleItemsRequest>,
|
||||
ifMatch?: string | null
|
||||
): Promise<ResponseWithMeta<ScheduleItem[]>> {
|
||||
return requestWithMeta<ScheduleItem[]>(`/api/v1/schedules/${scheduleId}/items`, {
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import type { Complete } from './completeRequest';
|
||||
import { ApiError, request, requestWithMeta, type ResponseWithMeta } from './client';
|
||||
import type { components } from './generated/v1';
|
||||
|
||||
@@ -58,7 +59,7 @@ export function getTemplateItemsWithMeta(id: number): Promise<ResponseWithMeta<T
|
||||
*/
|
||||
export function replaceTemplate(
|
||||
id: number,
|
||||
body: ReplaceTemplateRequest,
|
||||
body: Complete<ReplaceTemplateRequest>,
|
||||
ifMatch?: string | null
|
||||
): Promise<ResponseWithMeta<TemplateWithItems>> {
|
||||
return requestWithMeta<TemplateWithItems>(`/api/v1/templates/${id}`, {
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
// Pure, exhaustively-tested rules for the schedule-item editor. Encodes the Blazor
|
||||
// ProgramScheduleItemEditViewModel gating + forced-reset behavior (the parity standard for #207).
|
||||
// No React, no fetch — every function is a deterministic transform so itemRules.test.ts can pin it.
|
||||
import type { Complete } from '../api/completeRequest';
|
||||
import type { components } from '../api/generated/v1';
|
||||
|
||||
export type CollectionType = components['schemas']['CollectionType'];
|
||||
@@ -389,7 +390,7 @@ export function fromResponse(model: ScheduleItem): DraftItem {
|
||||
|
||||
// Projects a draft to the request body, applying Blazor's getter-gating: gated-off values are
|
||||
// nulled (or defaulted to the enum's None) so the payload matches what a non-editing Blazor VM emits.
|
||||
export function normalizeForSave(item: DraftItem): ScheduleItemRequest {
|
||||
export function normalizeForSave(item: DraftItem): Complete<ScheduleItemRequest> {
|
||||
const isFixed = item.startType === 'Fixed';
|
||||
const isMultipleCount = item.playoutMode === 'Multiple' && item.multipleMode === 'Count';
|
||||
const isDuration = item.playoutMode === 'Duration';
|
||||
|
||||
@@ -13,6 +13,7 @@ import {
|
||||
Trash2,
|
||||
TriangleAlert
|
||||
} from 'lucide-react';
|
||||
import type { Complete } from '../api/completeRequest';
|
||||
import { navigateToPath } from '../routing';
|
||||
import {
|
||||
Badge,
|
||||
@@ -206,7 +207,7 @@ function itemFromResponse(item: BlockItem): DraftItem {
|
||||
};
|
||||
}
|
||||
|
||||
function toRequestItem(item: DraftItem): BlockItemRequest {
|
||||
function toRequestItem(item: DraftItem): Complete<BlockItemRequest> {
|
||||
return {
|
||||
collectionType: item.collectionType,
|
||||
collectionId: item.collectionType === 'Collection' ? item.collectionId : null,
|
||||
@@ -269,7 +270,7 @@ function validate(draft: Draft): null | string {
|
||||
return null;
|
||||
}
|
||||
|
||||
function toReplaceRequest(draft: Draft): ReplaceBlockRequest {
|
||||
function toReplaceRequest(draft: Draft): Complete<ReplaceBlockRequest> {
|
||||
return {
|
||||
name: draft.name.trim(),
|
||||
minutes: draft.hours * 60 + draft.minutes,
|
||||
|
||||
@@ -20,8 +20,10 @@ import {
|
||||
type DecoListItem,
|
||||
type DecoTemplate,
|
||||
type DecoTemplateGroup,
|
||||
type DecoTemplateItem
|
||||
type DecoTemplateItem,
|
||||
type DecoTemplateItemRequest
|
||||
} from '../api';
|
||||
import type { Complete } from '../api/completeRequest';
|
||||
|
||||
const BASE_PATH = '/app/deco-templates';
|
||||
const MINUTES_PER_DAY = 24 * 60;
|
||||
@@ -604,7 +606,7 @@ function DecoTemplateEditor({ decoTemplateId }: { decoTemplateId: number }) {
|
||||
decoTemplateId,
|
||||
{
|
||||
name: draft.name.trim(),
|
||||
items: draft.items.map((item) => ({
|
||||
items: draft.items.map((item): Complete<DecoTemplateItemRequest> => ({
|
||||
decoId: item.decoId,
|
||||
startTime: item.startTime,
|
||||
endTime: item.endTime
|
||||
|
||||
@@ -36,6 +36,7 @@ import {
|
||||
type ReplaceDecoRequest,
|
||||
type SchedulingPickerOption
|
||||
} from '../api';
|
||||
import type { Complete } from '../api/completeRequest';
|
||||
import { getGraphicsElements, getWatermarks, type GraphicsElement, type Watermark } from '../api/pickers';
|
||||
import { getPlaylistGroups, getPlaylists, type Playlist, type PlaylistGroup } from '../api/playlists';
|
||||
|
||||
@@ -206,7 +207,7 @@ function fillerIdFields(type: CollectionType, id: number | null) {
|
||||
};
|
||||
}
|
||||
|
||||
function breakToRequest(item: BreakDraft): DecoBreakContentRequest {
|
||||
function breakToRequest(item: BreakDraft): Complete<DecoBreakContentRequest> {
|
||||
if (item.collectionType === 'Playlist') {
|
||||
return {
|
||||
id: item.id,
|
||||
@@ -233,7 +234,7 @@ function breakToRequest(item: BreakDraft): DecoBreakContentRequest {
|
||||
};
|
||||
}
|
||||
|
||||
function toReplaceRequest(draft: Draft): ReplaceDecoRequest {
|
||||
function toReplaceRequest(draft: Draft): Complete<ReplaceDecoRequest> {
|
||||
const defaultFiller = fillerIdFields(draft.defaultFillerCollectionType, draft.defaultFillerId);
|
||||
const deadAir = fillerIdFields(draft.deadAirFallbackCollectionType, draft.deadAirFallbackId);
|
||||
return {
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import type { Complete } from '../api/completeRequest';
|
||||
import { useCallback, useEffect, useRef, useState, type ReactNode } from 'react';
|
||||
import { ArrowLeft, Check, Copy, Plus, SlidersHorizontal, Trash2, TriangleAlert } from 'lucide-react';
|
||||
import { navigateToPath } from '../routing';
|
||||
@@ -24,7 +25,11 @@ const BASE_PATH = '/app/ffmpeg-profiles';
|
||||
// form must not offer one it would silently override (ersatztv#529)
|
||||
const MINIMUM_QSV_EXTRA_HARDWARE_FRAMES = 64;
|
||||
|
||||
type Draft = CreateFFmpegProfileRequest;
|
||||
// `Complete<…>` so the draft must name every request member: the edit path PUTs this whole
|
||||
// object to a full-replace endpoint, where an unset member is written as its default rather
|
||||
// than left alone. `qsvPreferNativeDecoder` is optional in the schema and both draft builders
|
||||
// happened to set it; nothing required them to (#807).
|
||||
type Draft = Complete<CreateFFmpegProfileRequest>;
|
||||
|
||||
/* ---------- enum option lists (mirror ErsatzTV/Pages/FFmpegEditor.razor) ---------- */
|
||||
|
||||
|
||||
@@ -16,8 +16,10 @@ import {
|
||||
type MediaCollection,
|
||||
type MultiCollection,
|
||||
type MultiCollectionItemRequest,
|
||||
type SmartCollection
|
||||
type SmartCollection,
|
||||
type UpdateMultiCollectionRequest
|
||||
} from '../api';
|
||||
import type { Complete } from '../api/completeRequest';
|
||||
|
||||
/* ---------- data hook ---------- */
|
||||
|
||||
@@ -137,7 +139,7 @@ function itemsFromMultiCollection(mc: MultiCollection): DraftItem[] {
|
||||
);
|
||||
}
|
||||
|
||||
function toItemRequest(item: DraftItem): MultiCollectionItemRequest {
|
||||
function toItemRequest(item: DraftItem): Complete<MultiCollectionItemRequest> {
|
||||
return {
|
||||
collectionId: item.kind === 'manual' ? item.id : null,
|
||||
playbackOrder: 'Chronological',
|
||||
@@ -277,7 +279,10 @@ function MultiCollectionEditor({
|
||||
setSaveError(null);
|
||||
|
||||
try {
|
||||
const body = { items: items.map(toItemRequest), name: trimmedName };
|
||||
const body: Complete<UpdateMultiCollectionRequest> = {
|
||||
items: items.map(toItemRequest),
|
||||
name: trimmedName
|
||||
};
|
||||
if (initial) {
|
||||
await updateMultiCollection(initial.id, body, etagRef.current);
|
||||
} else {
|
||||
|
||||
@@ -1,3 +1,5 @@
|
||||
import type { Complete } from '../api/completeRequest';
|
||||
import type { PathReplacementItemRequest } from '../api/mediaSources';
|
||||
import { useCallback, useEffect, useRef, useState } from 'react';
|
||||
import { ArrowLeft, Check, Plus, Trash2, TriangleAlert } from 'lucide-react';
|
||||
import { Button, Card, Input, Spinner } from '../components';
|
||||
@@ -172,7 +174,13 @@ export function PathReplacementsEditScreen({ family, sourceId }: { family: Remot
|
||||
setSaving(true);
|
||||
setSaveError(null);
|
||||
replacePathReplacements(family, sourceId, {
|
||||
items: draft.map((row) => ({ id: row.id, remotePath: row.remotePath.trim(), localPath: row.localPath.trim() }))
|
||||
items: draft.map(
|
||||
(row): Complete<PathReplacementItemRequest> => ({
|
||||
id: row.id,
|
||||
remotePath: row.remotePath.trim(),
|
||||
localPath: row.localPath.trim()
|
||||
})
|
||||
)
|
||||
})
|
||||
.then((rows) => {
|
||||
if (!activeRef.current) {
|
||||
|
||||
@@ -44,8 +44,10 @@ import {
|
||||
type PlaylistGroup,
|
||||
type PlaylistItem,
|
||||
type PlaylistItemRequest,
|
||||
type PlaylistPreviewItem
|
||||
type PlaylistPreviewItem,
|
||||
type ReplacePlaylistRequest
|
||||
} from '../api';
|
||||
import type { Complete } from '../api/completeRequest';
|
||||
import { SearchPicker } from '../schedules/pickers';
|
||||
|
||||
type CollectionType = PlaylistItemRequest['collectionType'];
|
||||
@@ -273,7 +275,7 @@ function draftFromItem(item: PlaylistItem): DraftItem {
|
||||
};
|
||||
}
|
||||
|
||||
function toItemRequest(item: DraftItem, index: number): PlaylistItemRequest {
|
||||
function toItemRequest(item: DraftItem, index: number): Complete<PlaylistItemRequest> {
|
||||
const source = configFor(item.collectionType)?.source ?? 'browse';
|
||||
const trimmedCount = item.count.trim();
|
||||
const parsedCount = trimmedCount === '' ? null : Number(trimmedCount);
|
||||
@@ -582,7 +584,10 @@ function PlaylistEditor({ playlistId, onBack, onSaved }: { playlistId: number; o
|
||||
});
|
||||
};
|
||||
|
||||
const buildRequest = () => ({ items: items.map(toItemRequest), name: name.trim() });
|
||||
const buildRequest = (): Complete<ReplacePlaylistRequest> => ({
|
||||
items: items.map(toItemRequest),
|
||||
name: name.trim()
|
||||
});
|
||||
|
||||
// Every item must carry a selection the API can bind. Without this the screen happily PUT an item
|
||||
// with a null id — the server 422s it (`ReplacePlaylistItemsHandler.CollectionTypeMustBeValid`),
|
||||
|
||||
@@ -20,8 +20,11 @@ import {
|
||||
type PlayoutAlternateSchedule,
|
||||
type PlayoutTemplate,
|
||||
type ProgramSchedule,
|
||||
type Template
|
||||
type Template,
|
||||
type PlayoutAlternateScheduleItemRequest,
|
||||
type PlayoutTemplateItemRequest
|
||||
} from '../api';
|
||||
import type { Complete } from '../api/completeRequest';
|
||||
|
||||
const PLAYOUTS_PATH = '/app/playouts';
|
||||
|
||||
@@ -526,11 +529,13 @@ export function PlayoutAlternateSchedulesScreen({ playoutId }: { playoutId: numb
|
||||
replaceAlternateSchedules(
|
||||
playoutId,
|
||||
{
|
||||
items: items.map((item) => ({
|
||||
id: item.id,
|
||||
programScheduleId: item.programScheduleId,
|
||||
...toRequestRecurrence(item)
|
||||
}))
|
||||
items: items.map(
|
||||
(item): Complete<PlayoutAlternateScheduleItemRequest> => ({
|
||||
id: item.id,
|
||||
programScheduleId: item.programScheduleId,
|
||||
...toRequestRecurrence(item)
|
||||
})
|
||||
)
|
||||
},
|
||||
etagRef.current
|
||||
)
|
||||
@@ -830,12 +835,14 @@ export function PlayoutTemplatesEditorScreen({ playoutId }: { playoutId: number
|
||||
replacePlayoutTemplates(
|
||||
playoutId,
|
||||
{
|
||||
items: items.map((item) => ({
|
||||
id: item.id,
|
||||
templateId: item.templateId,
|
||||
decoTemplateId: item.decoTemplateId,
|
||||
...toRequestRecurrence(item)
|
||||
}))
|
||||
items: items.map(
|
||||
(item): Complete<PlayoutTemplateItemRequest> => ({
|
||||
id: item.id,
|
||||
templateId: item.templateId,
|
||||
decoTemplateId: item.decoTemplateId,
|
||||
...toRequestRecurrence(item)
|
||||
})
|
||||
)
|
||||
},
|
||||
etagRef.current
|
||||
)
|
||||
|
||||
@@ -1,3 +1,5 @@
|
||||
import type { Complete } from '../api/completeRequest';
|
||||
import type { RemoteLibraryPreferenceRequest } from '../api/mediaSources';
|
||||
import { useCallback, useEffect, useRef, useState } from 'react';
|
||||
import { ArrowLeft, ArrowUpDown, Check, TriangleAlert } from 'lucide-react';
|
||||
import { Button, Card, Spinner, Switch } from '../components';
|
||||
@@ -171,7 +173,12 @@ export function RemoteLibrariesEditScreen({ family, sourceId }: { family: Remote
|
||||
setSaving(true);
|
||||
setSaveError(null);
|
||||
replaceRemoteLibraryPreferences(family, sourceId, {
|
||||
libraries: draft.map((library) => ({ id: library.id, shouldSyncItems: library.shouldSyncItems }))
|
||||
libraries: draft.map(
|
||||
(library): Complete<RemoteLibraryPreferenceRequest> => ({
|
||||
id: library.id,
|
||||
shouldSyncItems: library.shouldSyncItems
|
||||
})
|
||||
)
|
||||
})
|
||||
.then((libraries) => {
|
||||
if (!activeRef.current) {
|
||||
|
||||
@@ -21,8 +21,10 @@ import {
|
||||
type BlockGroup,
|
||||
type Template,
|
||||
type TemplateGroup,
|
||||
type TemplateItem
|
||||
type TemplateItem,
|
||||
type TemplateItemRequest
|
||||
} from '../api';
|
||||
import type { Complete } from '../api/completeRequest';
|
||||
|
||||
const BASE_PATH = '/app/templates';
|
||||
|
||||
@@ -678,7 +680,9 @@ function TemplateEditor({ templateId }: { templateId: number }) {
|
||||
templateId,
|
||||
{
|
||||
name: draft.name.trim(),
|
||||
items: draft.items.map((item) => ({ blockId: item.blockId, startTime: item.startTime }))
|
||||
items: draft.items.map(
|
||||
(item): Complete<TemplateItemRequest> => ({ blockId: item.blockId, startTime: item.startTime })
|
||||
)
|
||||
},
|
||||
etagRef.current
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user