Fail fast on invalid catalog data

[bsstahl]

Summary

• Add typed domain exceptions for malformed YAML, duplicate Talk IDs, and duplicate Title + PresentationFamily.Variant keys
• Fail fast during catalog load instead of skipping invalid records
• Document the validation policy in ADR-015 and close the strict-mode follow-up issue

Validation

• dotnet test src\TalkFolio.Tests\TalkFolio.Tests.csproj

[Copilot]

🟡 Changes recommended

There are compile-breaking namespace import issues and likely analyzer-breaking public exception patterns that need to be corrected before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This pull request introduces an initial file-backed YAML catalog loader for TalkFolio that enforces fail-fast validation via typed domain exceptions, and updates the documentation (including ADR-015) to codify the strict-load policy and PresentationFamily modeling.

Changes:

• Added FileSystemTalkCatalogRepository to load talk YAML files, enforce duplicate-key invariants, and throw typed load exceptions on invalid catalog data.
• Added a minimal TalkFolio.Api endpoint (GET /talks) plus tests covering successful loads and expected failure modes.
• Updated schema and architecture documentation to reflect PresentationFamily.Name semantics and the new validation and logging ADRs.

File summaries

File	Description
TalkFolio.slnx	Adds solution structure for the new src projects
Directory.Build.props	Enables latest analyzers and treats warnings as errors
src/TalkFolio/TalkFolio.csproj	Introduces core library project and YAML/logging dependencies
src/TalkFolio/TalkCatalogLoadException.cs	Adds typed domain exceptions for strict catalog load failures
src/TalkFolio/FileSystemTalkCatalogRepository.cs	Implements YAML-backed repository with fail-fast validation and logging
src/TalkFolio.Api/TalkFolio.Api.csproj	Adds the minimal web project referencing the core library
src/TalkFolio.Api/Program.cs	Wires repository options and exposes a GET /talks endpoint
src/TalkFolio.Tests/TalkFolio.Tests.csproj	Adds xUnit test project plus web-testing dependencies
src/TalkFolio.Tests/FileSystemTalkCatalogRepository_LoadAsync_Should.cs	Adds repository behavior tests for success, duplicates, and malformed YAML
src/TalkFolio.Tests/TalksEndpoint_GetTalks_Should.cs	Adds endpoint test validating canonical talk payload
docs/TalkSchema.md	Updates schema docs for PresentationFamily.Name and invariants
docs/Integration-Strategy.md	Aligns integration rules with PresentationFamily.Name lookups
docs/ADRs.md	Adds ADR-014 logging rule and ADR-015 fail-fast validation policy
CONTRIBUTING.md	Links logging instructions and documents test data conventions
ARCHITECTURE.md	Documents boundary-level observability expectations
.github/instructions/test-methodology.instructions.md	Adds coverage guidance to the test methodology docs
.github/instructions/logging.instructions.md	Adds repository logging requirements for C# files
.vscode/settings.json	Expands terminal auto-approve entries for common commands

Review details

Suppressed comments (2)

src/TalkFolio/TalkCatalogLoadException.cs:61

• DuplicateTalkIdException is a public exception type but it is missing the standard exception constructors, which can trip analyzer rules (for example CA1032) when warnings are treated as errors.

public sealed class DuplicateTalkIdException(Guid talkId, string firstFilePath, string duplicateFilePath)
    : TalkCatalogLoadException(
        $"Talk ID '{talkId}' is defined more than once. First file: '{firstFilePath}'. Duplicate file: '{duplicateFilePath}'.")
{
    /// <summary>

src/TalkFolio/TalkCatalogLoadException.cs:88

• DuplicateTalkTitleVariantException is a public exception type but it is missing the standard exception constructors, which can trip analyzer rules (for example CA1032) when warnings are treated as errors.

public sealed class DuplicateTalkTitleVariantException(string title, string variant, string firstFilePath, string duplicateFilePath)
    : TalkCatalogLoadException(
        $"Talk title '{title}' with PresentationFamily.Variant '{variant}' is defined more than once. First file: '{firstFilePath}'. Duplicate file: '{duplicateFilePath}'.")
{
    /// <summary>

• Files reviewed: 18/18 changed files
• Comments generated: 7
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[bsstahl]

Analyzer cleanup is complete:

• Explicit repo-wide analyzer enforcement is now enabled in Directory.Build.props
• Build is clean with 0 warnings / 0 errors
• Full test suite passes
• The repo and API logging paths were updated to satisfy the analyzer rules without changing behavior

[Copilot]

🟡 Changes recommended

There is at least one confirmed analyzer-related build-break risk (unused parameter under warnings-as-errors) and a misleading error message path for deserialization failures that should be corrected before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/TalkFolio/FileSystemTalkCatalogRepository.cs:201

• In the InvalidOperationException catch path, MalformedTalkYamlException.ForFilePath(...) produces an exception message that specifically advises quoting scalar values containing ':' (malformed YAML), but this catch is intended for deserialization/shape problems. This makes the thrown error message misleading and inconsistent with the TalkFileCouldNotBeDeserialized log message.

        catch (InvalidOperationException ex)
        {
            var malformedTalkYamlException = MalformedTalkYamlException.ForFilePath(filePath, ex);
            FileSystemTalkCatalogRepositoryLog.TalkFileCouldNotBeDeserialized(_logger, malformedTalkYamlException, filePath);
            throw malformedTalkYamlException;

• Files reviewed: 20/20 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The PR introduces a brittle test that verifies logger mock call details and a domain exception constructor that produces an empty top-level message, both of which should be corrected before approval.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/TalkFolio/TalkCatalogLoadException.cs:47

• The parameterless constructor builds the exception with an empty outer message (it passes string.Empty to the base), which makes the thrown/logged error hard to diagnose unless callers inspect InnerException. Prefer a non-empty default message on the exception itself.

    public MalformedTalkYamlException()
        : this(string.Empty, new InvalidOperationException("Talk file content could not be parsed."))
    {

src/TalkFolio.Tests/FileSystemTalkCatalogRepository_LoadAsync_Should.cs:265

• This test asserts emitted log levels by inspecting calls on a substituted ILogger, which makes the test brittle and tightly coupled to implementation details (and conflicts with the repo's guidance to avoid verifying logger mocks). Prefer asserting repository behavior (for example, that the catalog loads) and rely on code review/ADR for logging conformance, or use a dedicated log-capture harness instead of mock-call counting.

        var levels = logger.ReceivedCalls()
            .Select(static call => call.GetArguments())
            .Where(static arguments => arguments.Length > 0 && arguments[0] is LogLevel)
            .Select(static arguments => (LogLevel)arguments[0]!)
            .ToList();

• Files reviewed: 20/20 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟡 Changes recommended

The current implementation leaks mutable collections from the YAML model, includes an exception constructor that can produce empty messages, and adds a brittle logging test that relies on mock call inspection.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/TalkFolio/FileSystemTalkCatalogRepository.cs:123

• MapTalk returns YAML-deserialized List instances directly for AlternateTitles/Tags/TargetAudience/SlideDeckIds. Because these are mutable collections, callers can cast the IReadOnlyList back to List and mutate the catalog, which undermines the intent of a canonical read model and is inconsistent with the defensive copies used for ProposalCopyItems/PublicPresentationReferences/RelatedContent.

        return new TalkRecord(
            Id: source.Id,
            Title: source.Title,
            AlternateTitles: source.AlternateTitles ?? [],
            Category: source.Category ?? string.Empty,

src/TalkFolio.Tests/FileSystemTalkCatalogRepository_LoadAsync_Should.cs:261

• This test verifies logging by inspecting NSubstitute logger calls. That is brittle (depends on implementation details like number of logs) and conflicts with the repo test guidance to avoid verifying logger mocks; it will likely create churn as logging evolves.

        // Act
        _ = await target.LoadAsync(CancellationToken.None);

        // Assert
        var levels = logger.ReceivedCalls()

• Files reviewed: 36/36 changed files
• Comments generated: 1
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

There are fixable issues in the new code/tests (brittle logger-mock verification and an exception constructor that yields a blank message) that should be addressed before approval.

Review details

Suppressed comments (2)

src/TalkFolio/MalformedTalkYamlException.cs:13

• The parameterless constructor passes an empty message into the base Exception, which results in an unhelpful (blank) Message if this overload is ever thrown. Give it a meaningful default message.

    public MalformedTalkYamlException()
        : this("Talk file content could not be parsed.", new InvalidOperationException("Talk file content could not be parsed."))
    {

src/TalkFolio.Tests/FileSystemTalkCatalogRepository_LoadAsync_Should.cs:264

• This test asserts directly on a substituted ILogger's received calls/levels, which conflicts with the repo's test guidance (logger mocks should not be verified) and makes the test brittle to harmless logging changes.

        // Assert
        var levels = logger.ReceivedCalls()
            .Select(static call => call.GetArguments())
            .Where(static arguments => arguments.Length > 0 && arguments[0] is LogLevel)
            .Select(static arguments => (LogLevel)arguments[0]!)

• Files reviewed: 36/36 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

One of the new domain error messages is overly specific and can be misleading for non-quoting YAML/deserialization failures, and there is minor cleanup needed in the new test file (trailing blank lines).

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

src/TalkFolio.Tests/FileSystemTalkCatalogRepository_LoadAsync_Should.cs:337

• There are multiple trailing blank lines at the end of this test file, which adds noise and can cause unnecessary diffs in future edits.
  src/TalkFolio/MalformedTalkYamlException.cs:56
• MalformedTalkYamlException.BuildMessage currently hard-codes a specific "colon must be quoted" root cause, but this exception is also used for other load failures (for example deserialization issues). The message should be accurate in the general case while still providing the quoting tip as a common fix.

    private static string BuildMessage(string filePath)
    {
        return $"Talk file '{filePath}' contains malformed YAML. Scalar values containing ':' must be quoted, for example: - \"Workshop Edition: TP for Teams\".";
    }

• Files reviewed: 36/36 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

The current exception wrapping/message path for deserialization failures can produce misleading “malformed YAML” guidance and should be corrected before approval.

Review details

Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/TalkFolio/MalformedTalkYamlException.cs:23

• MalformedTalkYamlException() and MalformedTalkYamlException(string) manufacture new InvalidOperationException instances as the inner exception. This loses the original stack trace/cause when these constructors are used, and it adds a misleading inner exception that didn't actually occur. These overloads can call the base message-only constructor instead and reserve the (message, innerException) overload for real parser exceptions.

    public MalformedTalkYamlException()
        : this("Talk file content could not be parsed.", new InvalidOperationException("Talk file content could not be parsed."))
    {
    }

    /// <summary>
    /// Initializes a new instance of the <see cref="MalformedTalkYamlException"/> class.
    /// </summary>
    /// <param name="message">The error message.</param>
    public MalformedTalkYamlException(string message)
        : this(message, new InvalidOperationException(message))
    {

src/TalkFolio/FileSystemTalkCatalogRepository.cs:175

• DeserializeTalk catches InvalidOperationException (deserialization/shape issues) but wraps it in MalformedTalkYamlException.ForFilePath, whose message claims the YAML is malformed and gives ':' quoting advice. That can produce misleading errors for cases like missing/invalid fields or type conversion errors. Consider a dedicated exception (for example TalkYamlDeserializationException) or a separate factory that builds a "could not be deserialized" message while still capturing FilePath.

        catch (InvalidOperationException ex)
        {
            var malformedTalkYamlException = MalformedTalkYamlException.ForFilePath(filePath, ex);
            FileSystemTalkCatalogRepositoryLog.TalkFileCouldNotBeDeserialized(_logger, malformedTalkYamlException, filePath);
            throw malformedTalkYamlException;

src/TalkFolio/FileSystemTalkCatalogRepository.Logging.cs:8

• The initial Information log for loading the catalog is "Loading TalkFolio catalog." without the configured DataRoot. The repo logging guidance expects repository boundaries to include the data root/source path (and the examples use "Loading TalkFolio catalog from {DataRoot}"). Consider changing LoadingCatalog to accept a dataRoot parameter and updating the call site so operators can correlate loads to the configured root.

    [LoggerMessage(EventId = 1, Level = LogLevel.Information, Message = "Loading TalkFolio catalog.")]
    public static partial void LoadingCatalog(ILogger logger);

• Files reviewed: 36/36 changed files
• Comments generated: 0 new
• Review effort level: Lite