diff --git a/.editorconfig b/.editorconfig index d1399cd0..cc5416c4 100644 --- a/.editorconfig +++ b/.editorconfig @@ -36,7 +36,7 @@ indent_size = 2 end_of_line = crlf indent_size = 2 -# Json files +# Json and JsonC files [*.{json,jsonc}] end_of_line = crlf @@ -48,9 +48,15 @@ end_of_line = lf [*.{cmd,bat,ps1}] end_of_line = crlf +# --- .NET-only below: C# and ReSharper style. Everything above is the line-ending +# governance every derived repo carries; a non-.NET repo may drop from here down. --- + # C# files [*.cs] end_of_line = crlf +# Suppressions follow CODESTYLE.md "Analyzer Diagnostics and Suppressions": prefer a +# [SuppressMessage] attribute or the owning project's .editorconfig; relax a rule +# repo-wide here only when it applies to every project (never a brownfield batch). dotnet_diagnostic.IDE0055.severity = none dotnet_analyzer_diagnostic.severity = suggestion diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 7d796e75..058968c7 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,487 +1,38 @@ -# PlexCleaner AI Coding Instructions - -## Project Overview - -PlexCleaner is a .NET 10.0 CLI utility that optimizes media files for Direct Play in Plex/Emby/Jellyfin by: - -- Converting containers to MKV format -- Re-encoding incompatible video/audio codecs -- Managing tracks (language tags, duplicates, subtitles) -- Verifying and repairing media integrity -- Removing closed captions and unwanted content -- Monitoring folders for changes and automatically processing new/modified files - -The tool orchestrates external media processing tools (FFmpeg, HandBrake, MkvToolNix, MediaInfo, 7-Zip) via CLI wrappers. - -## Branching, Releases, and Bot Behavior - -For full rationale see [`AGENTS.md`](../AGENTS.md). Quick rules: - -- `feature → develop → main`. PRs only. -- Develop accepts **squash merges only**; main accepts **merge commits only**. Don't suggest rebase-merge — it's disabled at the repo level. -- **Two-phase publishing.** PRs only **smoke-build** changed targets (Docker `linux/amd64`, a 2-runtime executable subset, no push). `publish-release.yml` is the sole publisher: its **weekly schedule + manual dispatch** build/publish **both** branches (develop ⇒ NBGV prereleases `X.Y.Z-g{sha}` tagged `develop`; main ⇒ stable `X.Y.Z` tagged `latest`). Routine merges do **not** publish unless the `PUBLISH_ON_MERGE` repo variable is `true`. -- Dependabot targets **both** `main` and `develop` with the same ecosystems; major NuGet bumps gate on human review, everything else auto-merges via App-token-driven merge-bot. -- Every third-party GitHub Action is pinned to a full commit SHA with a `# vX.Y.Z` comment. Don't introduce `@v6` / `@main` / `@master` floating refs. -- Never merge a PR without a fresh "no issues found" review from `copilot-pull-request-reviewer[bot]` (shown as "Copilot" in the UI) on the latest commit. `mergeStateStatus: CLEAN` is necessary but not sufficient — Copilot's re-review of the latest push is required. Re-request the review **programmatically** after every push via the `requestReviews` GraphQL mutation (don't wait on flaky auto-review-on-push) — see the [GitHub Copilot Review Runbook](#github-copilot-review-runbook) below and [`AGENTS.md`](../AGENTS.md#merging-a-pr). -- After a develop → main merge lands and main's publish workflows complete, bump the minor in `version.json` on develop (e.g. `3.16` → `3.17`) via an isolated `bump-version-X.Y` PR. Without it, develop's next prerelease version numbers fall below main's just-shipped stable. -- A maintenance develop -> main promotion (dependency bumps, CI/doc fixes, template re-syncs - not a release) holds main's version: `git checkout main -- version.json` on the promotion branch, so main advances only a patch and develop keeps its lead. -- Don't recommend `git push --force` or `--force-with-lease`; both rulesets enforce `non_fast_forward`. -- `version.json`'s `publicReleaseRefSpec` is `^refs/heads/main$` — bumping the base `version` field is the only manual versioning action. - -## Documentation - -User-facing documentation is organized as follows: - -- **[README.md](../README.md)**: Main project documentation, quick start, installation, usage, and FAQ. -- **[Docs/LanguageMatching.md](../Docs/LanguageMatching.md)**: Technical details on IETF/RFC 5646 language tag matching and configuration. -- **[Docs/CustomOptions.md](../Docs/CustomOptions.md)**: FFmpeg and HandBrake custom encoding parameters, hardware acceleration setup, and encoder options. -- **[Docs/ClosedCaptions.md](../Docs/ClosedCaptions.md)**: Detailed technical analysis of EIA-608/CTA-708 closed caption detection methods and tools. -- **[HISTORY.md](../HISTORY.md)**: Release notes and version history. - -## Architecture - -### Command Structure - -PlexCleaner provides multiple commands: - -- **process**: Batch process media files in specified folders -- **monitor**: Watch folders for changes and automatically process modified files -- **verify**: Verify media files using FFmpeg -- **remux**: Re-multiplex media files to MKV -- **reencode**: Re-encode media tracks using HandBrake or FFmpeg -- **deinterlace**: De-interlace media files -- **createsidecar**: Create sidecar files for existing media -- **gettoolinfo**: Display tool version information -- **gettagmap**: Analyze language tags across media files -- **getmediainfo**: Extract and display media properties -- **checkfornewtools**: Check for and download tool updates (Windows only) -- **defaultsettings**: Create default configuration file -- **createschema**: Generate JSON schema for configuration validation -- **removesubtitles**: Remove all subtitle tracks -- **removeclosedcaptions**: Remove embedded EIA-608/CTA-708 closed captions from video streams -- **updatesidecar**: Create or update sidecar files to current schema/tool info -- **getsidecarinfo**: Display sidecar file information -- **testmediainfo**: Test parsing media tool information for non-Matroska containers -- **getversioninfo**: Print application and media tool version information - -### Fluent Builder Pattern for Media Tools - -All media tool command-line construction uses fluent builders (`*Builder.cs`). Never concatenate strings: - -```csharp -// Correct - fluent builder pattern -var command = new FfMpeg.GlobalOptions(args) - .Default() - .Add(customOption); - -// Wrong - string concatenation -string args = "-hide_banner " + option; -``` - -### Process Execution with CliWrap - -All external process execution uses [CliWrap](https://github.com/Tyrrrz/CliWrap) (v3.x): - -- Builders create `ArgumentsBuilder` instances -- Execute via `Cli.Wrap(toolPath).WithArguments(builder)` -- Use `BufferedCommandResult` for output capture -- See `MediaTool.cs` for base execution patterns -- All tool execution supports cancellation via `Program.CancelToken()` - -### Sidecar File System - -Critical performance feature - DO NOT break compatibility: - -- Each `.mkv` gets a `.PlexCleaner` sidecar JSON file -- Contains: processing state, tool versions, media properties, file hash -- Hash: First 64KB + last 64KB of file (not timestamp-based) -- Schema versioned (`SchemaVersion: 5` in `SidecarFileJsonSchema5`, global alias in `GlobalUsing.cs`) -- Processing skips verified files unless sidecar invalidated -- State flags are bitwise: `StatesType` enum with `[Flags]` attribute -- Sidecar operations: `Create()`, `Read()`, `Update()`, `Delete()` - -### Media Tool Abstraction - -- `MediaTool` base class defines tool lifecycle -- Each tool family has: Tool class, Builder class, Info schema -- Tool version info retrieved from CLI output, cached in `Tools.json` -- Windows supports auto-download via `GitHubRelease.cs`; Linux uses system tools -- Tool paths: `ToolsOptions.UseSystem` or `RootPath + ToolFamily/SubFolder/ToolName` -- Tool execution: Base `Execute()` method with cancellation, logging, and error handling -- Version checking: `GetInstalledVersion()`, `GetLatestVersion()` (Windows only) - -### Media Properties and Track Management - -**MediaProps hierarchy:** - -- `MediaProps`: Container for all media information (video, audio, subtitle tracks) -- `TrackProps`: Base class for all track types - - `VideoProps`: Video track properties (format, resolution, codec, HDR, interlacing) - - `AudioProps`: Audio track properties (format, channels, sample rate, codec) - - `SubtitleProps`: Subtitle track properties (format, codec, closed captions) - -**Track properties:** - -- Language tags: ISO 639-2B (`Language`) and RFC 5646/BCP 47 (`LanguageIetf`) -- Flags: Default, Forced, HearingImpaired, VisualImpaired, Descriptions, Original, Commentary -- State: Keep, Remove, ReMux, ReEncode, DeInterlace, SetFlags, SetLanguage, Unsupported -- Title, Format, Codec, Id, Number, Uid - -**Track selection (`SelectMediaProps.cs`):** - -- Separates tracks into Selected/NotSelected categories -- Used for language filtering, duplicate removal, codec selection -- Move operations: `Move(track, toSelected)`, `Move(trackList, toSelected)` -- State assignment: `SetState(selectedState, notSelectedState)` - -### Language Tag Management - -**IETF/RFC 5646 Support:** - -- Uses external package `ptr727.LanguageTags` for language tag parsing and matching -- Tag format: `language-extlang-script-region-variant-extension-privateuse` -- Matching: Left-to-right prefix matching via `LanguageLookup.IsMatch()` -- Conversion: ISO 639-2B ↔ RFC 5646 via `GetIsoFromIetf()`, `GetIetfFromIso()` -- Special tags: `und` (undefined), `zxx` (no linguistic content), `en` (English) - -**Language processing:** - -- MediaInfo reports both ISO 639-2B and IETF tags (if set) -- MkvMerge normalizes to IETF tags when `SetIetfLanguageTags` enabled -- FFprobe uses tag metadata which may differ from track metadata -- Track validation: Checks ISO/IETF consistency, sets error states for mismatches - -### Monitor Mode - -**File system watching:** - -- Uses `FileSystemWatcher` to monitor specified folders -- Monitors: Size, CreationTime, LastWrite, FileName, DirectoryName -- Handles: Changed, Created, Deleted, Renamed events -- Queue-based: Changes added to watch queue with timestamps - -**Processing logic:** - -- Files must "settle" (no changes for `MonitorWaitTime` seconds) before processing -- Files must be readable (not being written) before processing -- Retry logic: `FileRetryCount` attempts with `FileRetryWaitTime` delays -- Cleanup: Deletes empty folders after file removal -- Pre-process: Optional initial scan of all monitored folders on startup - -**Concurrency:** - -- Lock-based queue management (`_watchLock`) -- Periodic processing (1-second poll interval) -- Supports parallel processing when `--parallel` enabled - -### XML and JSON Parsing - -AOT-safe parsers in `MediaInfoXmlParser.cs`: - -- **MediaInfoFromXml()**: Parses specific MediaInfo XML elements into `MediaInfoToolXmlSchema.MediaInfo` - - Manually parses only known elements needed by PlexCleaner (id, format, language, etc.) - - Used by sidecar file system to parse XML output when JSON unavailable - - Avoids XmlSerializer (not AOT-compatible) -- **GenericXmlToJson()**: Converts any XML file to JSON format - - Preserves all elements and attributes (unlike MediaInfoFromXml's selective parsing) - - Handles attributes: prefix with `@` for elements with children, no prefix for leaf elements - - Detects arrays: elements appearing multiple times become JSON arrays - - Two-pass algorithm: collect children to detect arrays, then write JSON - - Uses `XmlReader` and `Utf8JsonWriter` for streaming efficiency - - Special handling for MediaInfo's mixed attribute/text content format (creatingLibrary) -- **MediaInfoXmlToJson()**: Converts parsed MediaInfo XML to MediaInfo JSON schema - - Bridges between XML and JSON schema types - - Maps only known MediaInfo track properties - -Parser design patterns: - -- Forward-only `XmlReader` with depth tracking for streaming -- Recursive `ElementData` tree for generic XML-to-JSON conversion -- Namespace filtering (skip `xmlns`, `xsi` attributes) -- Special handling for MediaInfo's mixed attribute/text content format - -### Extensions Pattern +# Copilot Instructions -**Modern C# 13 extension syntax:** +Repository conventions for GitHub Copilot (and any other AI agent reading this file). -- Uses implicit class extensions: `extension(ILogger logger)` -- Provides context-aware helper methods -- Examples: - - `LogAndPropagate()`: Log exception and return false (propagates error) - - `LogAndHandle()`: Log exception and return true (handles error for catch clauses) - - `LogOverrideContext()`: Create scoped logger with LogOverride context +The **canonical guide is [AGENTS.md](../AGENTS.md)** at the repo root - read it first, including the [PR Review Etiquette](../AGENTS.md#pr-review-etiquette) review-loop contract this file's runbook implements. This file is intentionally narrow: commit/PR-title conventions (summarized inline so VS Code's commit-message and PR-title generators have them) plus the GitHub Copilot Review Runbook. -## Code Conventions +For code-style rules, see [`CODESTYLE.md`](../CODESTYLE.md) at the repo root - one guide with a General section plus the .NET language section. -### Formatting Standards +For PlexCleaner's architecture, processing pipeline, and design patterns, see [../ARCHITECTURE.md](../ARCHITECTURE.md). -- **Code formatter**: CSharpier (`.csharpier.json`) - primary formatter -- **EditorConfig**: `.editorconfig` follows .NET Runtime style guide -- **Pre-commit hooks**: Husky.Net validates style (`dotnet husky run`) -- Line endings: CRLF for Windows files (`.cs`, `.json`, `.yml`), LF for shell scripts -- Charset: UTF-8 without BOM +Do not duplicate language-specific rules here. **Project-specific conventions and API/behavioral contracts also belong in [AGENTS.md](../AGENTS.md), not here** - this file is intentionally limited to the inline commit/PR-title summary and the GitHub Copilot Review Runbook. Non-Copilot agents (Claude Code, Codex, Cursor, ...) are not directed to this file and don't read it by default, so any rule a reviewer must honor has to live in `AGENTS.md` to be provider-independent. -### Code Style +## Commit Messages and Pull Request Titles -- Target: .NET 10.0 (`net10.0`) -- AOT compilation enabled: `true` in executable projects -- Use C# modern features (records, pattern matching, collection expressions, implicit class extensions) -- Prefer `Debug.Assert()` for internal invariants -- Logging: Serilog with thread IDs (`Log.Information/Warning/Error`) -- Exception handling: Currently uses broad `catch(Exception)` - TODO to specialize -- Global usings: `GlobalUsing.cs` defines project-wide type aliases (`ConfigFileJsonSchema`, `SidecarFileJsonSchema`) -- `Directory.Build.props`: Common MSBuild properties (`TargetFramework`, `Nullable`, `ImplicitUsings`, - `AnalysisLevel`, etc.) shared across all projects live here at the solution root. Do not duplicate - these in individual `.csproj` files -- only add a property to a `.csproj` when it is project-specific - or overrides the shared default. -- `Directory.Packages.props`: All NuGet package versions are centralised here via `PackageVersion` items. - `PackageReference` elements in `.csproj` files must not include a `Version` attribute. Asset metadata - (`PrivateAssets`, `IncludeAssets`) stays in the `.csproj` `PackageReference` element. +Summarized for VS Code's generators; the full rules, rationale, and examples are in [AGENTS.md "Pull Request Title and Commit Message Conventions"](../AGENTS.md#pull-request-title-and-commit-message-conventions). -### Naming and Structure +- Imperative subject, <= 72 characters, no trailing period; optional blank-line-separated body for the non-obvious *why*. +- US English, title case with lowercase short bind words; no vague titles, no `Co-Authored-By:` unless asked, no release-bump magnitude (NBGV handles versioning). Dependabot's `Bump X from Y to Z` titles are fine. +- develop PRs squash-merge (`gh pr merge --squash`), main PRs merge-commit (`--merge`); a mismatched flag is rejected by branch protection. -- JSON schemas: Generated via `JsonSchema.Net`, suffixed with version (e.g., `SidecarFileJsonSchema5`) -- Builder methods: Return `this` for chaining -- Media props: `*Props.cs` classes (VideoProps, AudioProps, SubtitleProps, TrackProps) -- Options classes: `*Options.cs` for command categories (ProcessOptions, VerifyOptions, ConvertOptions, ToolsOptions) -- Partial classes: Tool families use partial class structure (`*Tool.cs`, `*Builder.cs`) - -### Async and Concurrency - -- Main loop: Uses `WaitForCancel()` polling pattern instead of async/await -- Tool execution: Synchronous wrappers around CliWrap async operations -- Parallel processing: PLINQ with `AsParallel()`, `WithDegreeOfParallelism()` -- Lock-based synchronization: `Lock` instances for collection access -- Cancellation: Global `CancellationTokenSource` accessed via `Program.CancelToken()` - -## Testing - -### Test Framework - -- xUnit v3.x with `AwesomeAssertions` -- Test project: `PlexCleanerTests/` -- Fixture: `PlexCleanerFixture` (assembly-level, sets up defaults and logging) -- Sample media: `Samples/PlexCleaner/` (relative path `../../../../Samples/PlexCleaner`) - -### Test Coverage - -- Command-line parsing: `CommandLineTests.cs` -- Configuration validation: `ConfigFileTests.cs` -- FFmpeg parsing: `FfMpegIdetParsingTests.cs` -- Sidecar functionality: `SidecarFileTests.cs` -- Version parsing: `VersionParsingTests.cs` -- Wildcards: `WildcardTests.cs` -- Filename escaping for filters: `FileNameEscapingTests.cs` - -### Test Execution - -- Task: `"dotnet: .Net Build"` for builds -- Unit tests: `dotnet test` or VS Code test explorer -- Docker tests: Download Matroska test files from GitHub -- CI: Separate workflows for build tests and Docker tests - -## Build and Release - -### Local Development - -```bash -# Build -dotnet build - -# Format code -dotnet csharpier . - -# Verify formatting -dotnet format style --verify-no-changes --severity=info --verbosity=detailed - -# Run tests -dotnet test - -# Pre-commit validation (automatic via Husky) -dotnet husky run -``` - -### GitHub Actions - -Two-phase model — reusable `*-task.yml` workflows orchestrated by two entry points: - -- **test-pull-request.yml**: PR validation. `changes` (dorny/paths-filter) → always-on `unit-test` (Husky) + path-gated `smoke-build` (reduced, no-push) → `Check pull request workflow status` aggregator (ruleset-bound name; requires `changes` succeeded). -- **publish-release.yml**: the **sole publisher** (`push` + weekly `schedule` + `workflow_dispatch`). A `setup` job computes the branch list + publish gate; the `publish` matrix builds both branches via `build-release-task.yml` (executable 7-RID matrix + multi-arch Docker `linux/amd64,linux/arm64` + GitHub release), then `tool-versions`, `docker-readme` (main only), `date-badge` (main only). -- Reusable tasks: `build-release-task.yml`, `build-executable-task.yml`, `build-docker-task.yml`, `build-toolversions-task.yml`, `build-dockerreadme-task.yml`, `build-datebadge-task.yml`, `get-version-task.yml`. All thread a required `branch` input (config keys off it, never `github.ref_name`) plus `ref`/`smoke`. -- Version info: `version.json` with Nerdbank.GitVersioning format. `get-version-task.yml` surfaces `SemVer2`, the assembly versions, and `GitCommitId` (used to pin the release `target_commitish`). -- Branches: `main` (stable releases, `latest`), `develop` (pre-releases, `develop`). - -### Docker - -- Multi-stage builds in `Docker/Dockerfile` -- Base image: `ubuntu:rolling` only (no longer publishing Alpine or Debian variants) -- Supported architectures: `linux/amd64`, `linux/arm64` (no longer supporting `linux/arm/v7`) -- Tool installation: Ubuntu package manager (apt) -- Media tool versions match Windows versions for consistent behavior -- Test script: `Docker/Test.sh` validates all commands -- Version extraction: `Docker/Version.sh` captures tool versions for README -- User: Runs as `nonroot` user in containers -- Volumes: `/media` for media files and configuration - -## Common Patterns - -### Command-Line Parsing - -Uses `System.CommandLine` (v2.x): - -- Options defined in `CommandLineOptions.cs` -- Binding via `CommandLineParser.Bind()` -- No `System.CommandLine.NamingConventionBinder` (deprecated) -- Recursive options: Available to all subcommands (`--logfile`, `--logwarning`, `--debug`) -- Command routing: Each command maps to static method in `Program.cs` - -### Parallel Processing - -- `--parallel` flag enables concurrent file processing -- Uses `ProcessDriver.cs` with `AsParallel()` and `WithDegreeOfParallelism()` -- Default thread count: min(CPU/2, 4), configurable via `--threadcount` -- Lock-based collection updates in parallel contexts -- File grouping: Groups by path (excluding extension) to prevent concurrent access to same file - -### File Processing States - -```csharp -[Flags] -enum StatesType { - None, SetLanguage, ReMuxed, ReEncoded, DeInterlaced, - Repaired, RepairFailed, Verified, VerifyFailed, - BitrateExceeded, ClearedTags, FileReNamed, FileDeleted, - FileModified, ClearedCaptions, RemovedAttachments, - SetFlags, RemovedCoverArt -} -``` +## GitHub Copilot Review Runbook -Check states with `HasFlag()`, combine with `|=` +> This runbook implements the [AGENTS.md "PR Review Etiquette"](../AGENTS.md#pr-review-etiquette) review-loop contract for GitHub Copilot. Without it in-repo, an agent has no pointer to the reliable Copilot mechanics and falls back to known-broken paths (the no-op `POST /requested_reviewers`, the wrong bot-login filter). In the API snippets below, `` is the PR number. -### Configuration Schema - -- Settings: `PlexCleaner.defaults.json` with inline JSONC comments -- Schema: `PlexCleaner.schema.json` (auto-generated via JsonSchema.Net) -- Validation: JSON Schema.Net with source-generated context -- URL schema reference: `https://raw.githubusercontent.com/ptr727/PlexCleaner/main/PlexCleaner.schema.json` -- Versioned: ConfigFile schemas numbered (ConfigFileJsonSchema4, etc.) -- Defaults: `SetDefaults()` method in each options class -- Verification: `VerifyValues()` method validates configuration - -### Keep-Awake Pattern - -- Prevents system sleep during long operations -- Uses `KeepAwake.cs` with Windows API calls -- Timer-based: Refreshes every 30 seconds -- Cross-platform: No-op on non-Windows systems - -### Cancellation Handling - -- Global token source: `Program.s_cancelSource` -- Console handlers: Ctrl+C, Ctrl+Z, Ctrl+Q -- Keyboard monitoring: Separate task for key press handling -- Tool execution: All CliWrap calls use `Program.CancelToken()` -- Graceful cleanup: Logs cancellation messages, disables file watchers - -## Critical Details - -### DO NOT - -- Break sidecar file compatibility (versioned schema migrations only) -- Use string concatenation for command-line arguments (use builders) -- Modify file timestamps unless `RestoreFileTimestamp` enabled -- Execute media tools without CliWrap abstractions -- Add synchronous operations in parallel processing paths -- Use `XmlSerializer` for AOT compilation (not compatible) -- Break language tag matching logic (IETF/ISO conversion) - -### DO - -- Add tests for media tool parsing changes (see `FfMpegIdetParsingTests.cs`) -- Update `HISTORY.md` for notable changes -- Use `Program.CancelToken()` for cancellation support -- Log with context: filenames, state transitions, tool versions -- Handle cross-platform paths (`Path.Combine`, forward slashes in Docker) -- Use modern C# features (collection expressions, pattern matching, extensions) -- Version schemas when making breaking changes -- Update global using aliases in `GlobalUsing.cs` when changing schema versions - -### Performance Considerations - -- Sidecar files enable fast re-processing (skip verified files) -- `--parallel` most effective with I/O-bound operations (re-mux) -- `--quickscan` limits scan to 3 minutes (trades accuracy for speed) -- `--testsnippets` creates 30s clips for testing -- Docker logging can grow large - configure rotation externally -- Monitor mode: Settle time prevents excessive re-processing - -### Special Cases - -**Closed Captions:** - -- EIA-608/EIA-708 tracks handled specially in `SubtitleProps.HandleClosedCaptions()` -- Parsed as subtitle tracks but removed during processing -- Track IDs formatted as `{VideoId}-CC{Number}` (e.g., `256-CC1`) - -**VOBSUB Subtitles:** - -- Require `MuxingMode` to be set for Plex compatibility -- Missing `MuxingMode` triggers error and removal recommendation - -**Duplicate Tracks:** - -- Language-based grouping with flag preservation -- Preferred audio codec selection via `FindPreferredAudio()` -- Keeps one flagged track per flag type, one non-flagged track - -**Language Mismatches:** - -- ISO 639-2B vs IETF tag validation in `TrackProps.SetLanguage()` -- Tag metadata vs track metadata differences (FFprobe specific) -- Automatic fallback: At least one track kept even if language doesn't match - -## Key Files Reference - -- **Program.cs**: Entry point, command routing, global state, cancellation handling -- **ProcessDriver.cs**: File enumeration, parallel processing orchestration -- **ProcessFile.cs**: Single-file processing logic, track selection algorithms -- **Process.cs**: High-level processing workflow, empty folder deletion -- **SidecarFile.cs**: Sidecar creation, validation, state management, hashing -- **MediaTool.cs**: Base class for tool abstractions, execution patterns -- **MediaProps.cs**: Media container, track aggregation -- **TrackProps.cs**: Base track properties, language handling, flag management -- **VideoProps.cs / AudioProps.cs / SubtitleProps.cs**: Track-specific properties -- **MediaInfoXmlParser.cs**: AOT-safe XML/JSON parsing (MediaInfo output) -- **Monitor.cs**: File system watching, change queue management -- **Convert.cs**: Re-encoding and re-muxing orchestration -- **MkvProcess.cs**: MKV-specific operations (attachment removal, flag setting) -- **Tools.cs**: Tool instances, version verification, update checking -- **Language.cs**: IETF tag matching, language list extraction -- **SelectMediaProps.cs**: Track filtering and selection logic -- **CommandLineOptions.cs**: CLI parsing, option definitions -- **Extensions.cs**: Logger extensions, implicit class extensions -- **GlobalUsing.cs**: Global type aliases for schema versions -- **KeepAwake.cs**: System sleep prevention -- **PlexCleaner.defaults.json**: Canonical configuration reference -- **.editorconfig** / **.csharpier.json**: Code style definitions - -## Git and Commit Rules - -- **Default to staging, not committing.** Stage changes with `git add` and leave `git commit` to the developer unless explicitly authorized to commit for the current ask ("commit this", "open a PR"). Authorization is scope-bound to that task. -- **All commits must be cryptographically signed (SSH/GPG)** — branch protection rejects unsigned commits. Signing depends on environment config (`commit.gpgsign`, a `user.signingkey`, a loaded agent). If signing isn't configured, **do not commit** — stop at `git add` and surface it. Verify first: `git config --get commit.gpgsign && ssh-add -L`. -- **Never force push.** Do not run `git push --force` or `git push --force-with-lease`. Force pushing rewrites shared branch history and is blocked by branch protection rules. -- **Never run destructive git commands** (`git reset --hard`, `git checkout .`, `git restore .`, `git clean -f`) without explicit developer instruction. -- **The `develop → main` release merge is maintainer-only.** Drive `feature → develop` PRs end-to-end when authorized (commit, push, Copilot review loop, squash-merge), but never self-merge a release to `main`. +Use this section for provider-specific mechanics. The expected review loop *contract* (request review on every push, verify head-SHA coverage, triage findings, reply + resolve, escalate when stuck) is defined in [AGENTS.md -> PR Review Etiquette](../AGENTS.md#pr-review-etiquette). This section only describes how to make GitHub Copilot reliably execute it. -## GitHub Copilot Review Runbook +### Triggering and Polling -Provider-specific mechanics for driving GitHub Copilot reviews entirely via `gh`/GraphQL — no manual UI clicks. The review-loop *contract* (re-request on every push, verify head-SHA coverage, triage, reply + resolve, escalate when stuck) is in [AGENTS.md → Merging a PR](../AGENTS.md#merging-a-pr); this section is how to make Copilot reliably execute it. +Auto-review on push is configured (via the branch ruleset's `copilot_code_review` rule with `review_on_push: true`) but fires inconsistently in practice - treat it as best-effort, not guaranteed. After every push, **re-request a review programmatically** via the GraphQL `requestReviews` mutation, passing the Copilot reviewer's bot node id in `botIds`. This drives the loop end-to-end without a UI hand-off. -### Triggering and Polling +**A review with no inline comments is still a completed review - not a failure, and not a reason to ask the maintainer to re-trigger.** Copilot very often posts a single formal review (GraphQL `state: COMMENTED`) whose body ends with "...reviewed N of N changed files ... and generated no comments" and adds **zero** inline threads. That review carries the head `commit.oid` and fully satisfies the loop - it is the clean-pass success case. Never read "no inline comments" as "the review didn't run," and never re-request or escalate to the maintainer because comments are absent. -Auto-review on push is configured (the branch ruleset's `copilot_code_review` rule with `review_on_push: true`) but fires inconsistently — treat it as best-effort. After every push, **re-request a review programmatically** via the GraphQL `requestReviews` mutation, passing the Copilot reviewer's bot node id in `botIds`. This drives the loop end-to-end without a maintainer clicking "re-request review" in the UI. +**Round 1 is normally auto-seeded - poll for it before trying to self-trigger.** Auto-review-on-open supplies the first review with no `botIds` call needed, but it can lag one to three minutes. After opening a PR (or the first push), **poll** for a Copilot review on the head SHA (see [Verify Review Covered Current Head](#verify-review-covered-current-head)) before concluding none ran. The `requestReviews` mutation below is for **re-requesting on later pushes** (a new head SHA); by then a prior review exists, so its bot node id is readable. A missing bot node id on round 1 therefore means "the auto-review has not landed yet - wait and poll," **not** "ask the maintainer to kick it off." -> **The reviewer login differs by API — this is intentional, not a typo.** In **GraphQL** (`gh api graphql` and `gh pr view --json reviews`, which is GraphQL-backed) the `Bot.login` is `copilot-pull-request-reviewer` — **no `[bot]` suffix**. In the **REST** API (`gh api repos/.../issues|pulls/...`) the same account's `user.login` is `copilot-pull-request-reviewer[bot]` — **with** the suffix. Each query below uses the correct form for its API; match the API, not a single spelling, when adapting them. (The prose elsewhere referring to `copilot-pull-request-reviewer[bot]` is describing the REST/display login.) +> **The reviewer login differs by API.** In **GraphQL** (`gh api graphql` and `gh pr view --json reviews`, which is GraphQL-backed) the `Bot.login` is `copilot-pull-request-reviewer` - **no `[bot]` suffix**. In the **REST** API (`gh api repos/.../issues|pulls/...`) the same account's `user.login` is `copilot-pull-request-reviewer[bot]` - **with** the suffix. Each query below uses the correct form for its API; match the API, not a single spelling, when adapting them. ```sh # 1. PR node id + the Copilot reviewer's bot node id (read from any existing @@ -507,47 +58,49 @@ mutation($pr: ID!, $bot: ID!) { }' -F pr="$PR_NODE" -F bot="$BOT_ID" ``` -The bot node id is read from an existing Copilot review, so step 1 needs at least one prior review on the PR — auto-review-on-open normally supplies the first. If none exists yet and auto-review didn't fire, request `Copilot` once through the GitHub PR UI to seed it, then use the mutation for every subsequent re-request. The Copilot reviewer bot's global node id is `BOT_kgDOCnlnWA` (login `copilot-pull-request-reviewer`) if you need to skip discovery. +The bot node id is read from an existing Copilot **formal** review (`pullRequest.reviews`), so step 1 needs at least one prior formal review on the PR - the auto-review-on-open normally supplies the first one (it may have **no inline comments**; that still counts, and its bot node id is still readable). Poll for it (give auto-review-on-open a few minutes) before deciding it is missing. The Copilot reviewer bot's global node id is `BOT_kgDOCnlnWA` (login `copilot-pull-request-reviewer`) if you need to skip discovery. If Copilot posted **only an issue comment** and no formal review, the head is covered but `reviews` yields no bot node id - read the id from the Copilot issue comment's author by querying the PR's issue comments in GraphQL (`pullRequest.comments` -> author `... on Bot { id }`), or request `Copilot` once through the GitHub PR UI to produce a formal review. Manual UI seeding is the fallback specifically when no formal review exists to read the id from; then use the mutation for every subsequent re-request. -**Do NOT post `@Copilot review` as a PR comment.** That triggers the Copilot *coding agent* (`copilot-swe-agent[bot]`), which makes code changes rather than posting a review. +**Do NOT post `@Copilot review` as a PR comment.** That comment triggers the Copilot *coding agent* (`copilot-swe-agent[bot]`), which makes code changes rather than posting a review. -Known non-working request paths (use the `requestReviews` mutation instead): +Known non-working request paths (don't rely on them - use the `requestReviews` mutation above instead): - `POST /requested_reviewers` with `reviewers=[Copilot]` can return 200 but no-op. - `copilot-pull-request-reviewer` as a requested reviewer slug returns 422. ### Verify Review Covered Current Head -Before merging, confirm Copilot reviewed the current PR head SHA. Copilot may respond as a formal review (carries an exact commit SHA) or an issue comment (no SHA). Check both. +Before merging, confirm Copilot reviewed the current PR head SHA. Copilot may respond as either a formal review (carries an exact commit SHA) or an issue comment (no SHA - use the most recent Copilot comment for manual confirmation). Check both. ```sh PR_HEAD=$(gh pr view --json headRefOid --jq '.headRefOid') -# 1. Formal review — exact SHA match. +# 1. Formal review - exact SHA match. gh pr view --json reviews --jq \ '.reviews[] | select(.author.login=="copilot-pull-request-reviewer") | .commit.oid' \ | grep -q "$PR_HEAD" && echo "covered via formal review" -# 2. Issue comment — show the most recent Copilot comment for manual +# 2. Issue comment - show the most recent Copilot comment for manual # confirmation. This is the REST API, so the login carries the `[bot]` suffix. gh api repos/ptr727/PlexCleaner/issues//comments --jq \ '[.[] | select(.user.login=="copilot-pull-request-reviewer[bot]")] | last | {created_at, body: .body[:200]}' ``` -Coverage is confirmed when (1) exits 0. For issue comments (path 2), body content is the only reliable signal — `created_at` is not (commit timestamps can predate the push). Treat path (2) as confirmed only when the comment body explicitly refers to the current changes. +Coverage is confirmed when (1) exits 0 - **a formal review with no inline comments still satisfies path (1)**, because coverage is about the head SHA, not the comment count. For issue comments (path 2), body content is the only reliable signal - `created_at` is not: `git log -1 --format=%cI` is the **commit** timestamp, not the push timestamp, so amended or rebased commits can have an earlier timestamp and an older Copilot comment could satisfy a time check even though Copilot never saw the current head. Treat path (2) as confirmed only when the comment body explicitly refers to the current changes. ### Bounded Retry Workflow -If a review did not run on the current head: +This path is only for a **genuinely missing** review - no Copilot review (formal *or* issue comment) covers the current head SHA after polling. A review that covered the head but produced no comments is a clean pass, not a missing review; do not enter this retry path for it. -1. Wait briefly and check head-SHA coverage (above). -1. Re-request via the `requestReviews` mutation; fall back to the GitHub PR UI only if the mutation no-ops. +If a review did not run on the current head, retry: + +1. Wait briefly and check head-SHA coverage (see above). +1. Re-request the review via the `requestReviews` mutation (see "Triggering and Polling"); fall back to the GitHub PR UI only if the mutation no-ops. 1. Retry up to two more times (three total). -1. If still missing, mark the review blocked and escalate to the maintainer with what was attempted. +1. If still missing, mark review as blocked and escalate to the user/maintainer with what was attempted. ### Reply and Thread Resolution Workflow -List unresolved threads (`first: 100` + cursor pagination; if `hasNextPage`, re-run with `after: ""`): +List unresolved threads. Use `first: 100` with cursor-based pagination; if `hasNextPage` is true, re-run with `after: ""` to retrieve the next page: ```sh gh api graphql -f query=' @@ -586,12 +139,18 @@ mutation($threadId: ID!) { }' -F threadId="PRRT_..." ``` -Issue-level Copilot comments (those in `issues//comments`) have no resolution action — reply if the finding warrants it; no resolution step is possible. +Issue-level Copilot comments (those in `issues//comments`) have no resolution action - GitHub provides no API or UI to resolve them. Reply if the finding warrants it; no resolution step is needed or possible. Reply-body conventions: - Accepted bug/style fix: include fixing commit SHA and a one-line summary. -- Declined style comment: cite the rule (AGENTS.md or CODESTYLE) and the existing-tree precedent. +- Declined style comment: cite the rule (AGENTS.md or the CODESTYLE.md language section) and the existing-tree precedent. - Declined architecture proposal: one-sentence rationale. -A PR is mergeable when `mergeStateStatus == CLEAN` and there are 0 unresolved threads on the current head. After the final push, sweep-resolve stale older threads for removed code paths. +After the final push, sweep-resolve stale older threads for removed code paths. + +## When in Doubt + +Read [AGENTS.md](../AGENTS.md) for this repo's conventions and [../ARCHITECTURE.md](../ARCHITECTURE.md) for PlexCleaner's architecture, processing pipeline, and design patterns. For code-style rules, [`CODESTYLE.md`](../CODESTYLE.md) (its General section plus the relevant language section) is authoritative. Don't restate any of these files' rules in commit bodies or PR descriptions - keep those focused on the change itself. + +**In a derived repo:** if you find a discrepancy that should be fixed in the template itself (this file or AGENTS.md is out of date, a rule is missing, something bit this repo and would bite the next), open an issue upstream in [`ptr727/ProjectTemplate`](https://github.com/ptr727/ProjectTemplate) rather than only fixing it locally - see the template's [AGENTS.md "Staying in Sync and Reporting Drift Upstream"](https://github.com/ptr727/ProjectTemplate/blob/main/AGENTS.md#staying-in-sync-and-reporting-drift-upstream). diff --git a/.github/workflows/build-executable-task.yml b/.github/workflows/build-executable-task.yml index a528e29e..fa4fd42c 100644 --- a/.github/workflows/build-executable-task.yml +++ b/.github/workflows/build-executable-task.yml @@ -21,10 +21,6 @@ on: required: false type: boolean default: false - outputs: - # Output of the uploaded artifact id - artifact-id: - value: ${{ jobs.upload-build-artifacts.outputs.artifact-id }} jobs: @@ -73,16 +69,15 @@ jobs: with: name: publish-${{ inputs.branch }}-${{ matrix.runtime }} path: ${{ runner.temp }}/publish + retention-days: 1 # Smoke builds only need the per-runtime compile to succeed (fast PR - # feedback) — the zipped, downloadable artifact is a release concern, so - # skip the aggregation entirely on smoke. The `artifact-id` output is then - # empty, which is fine because the GitHub release job never runs on smoke. + # feedback) - the zipped, downloadable artifact is a release concern, so + # skip the aggregation entirely on smoke. The release job collects the + # release-asset--* artifacts by pattern, so no artifact-id is needed. upload-build-artifacts: name: Upload matrix build artifacts job if: ${{ !inputs.smoke }} - outputs: - artifact-id: ${{ steps.artifact-upload-step.outputs.artifact-id }} runs-on: ubuntu-latest needs: [ build-executable-matrix ] @@ -99,8 +94,8 @@ jobs: run: 7z a -t7z ${{ runner.temp }}/PlexCleaner.7z ${{ runner.temp }}/publish/* - name: Upload build artifacts step - id: artifact-upload-step uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: - name: executable-build-${{ inputs.branch }} + name: release-asset-${{ inputs.branch }}-executable path: ${{ runner.temp }}/PlexCleaner.7z + retention-days: 1 diff --git a/.github/workflows/build-release-task.yml b/.github/workflows/build-release-task.yml index 3ae6cdec..a7c6b876 100644 --- a/.github/workflows/build-release-task.yml +++ b/.github/workflows/build-release-task.yml @@ -103,10 +103,13 @@ jobs: with: ref: ${{ needs.get-version.outputs.GitCommitId }} - - name: Download executable build artifacts step + # Collect every release-asset--* artifact by pattern, so the + # release job never names a build job and stays reusable as targets change. + - name: Download release asset artifacts step uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: - artifact-ids: ${{ needs.build-executable.outputs.artifact-id }} + pattern: release-asset-${{ inputs.branch }}-* + merge-multiple: true path: ./Publish # The weekly publisher re-runs even when a branch has no new commits, so diff --git a/.github/workflows/build-toolversions-task.yml b/.github/workflows/build-toolversions-task.yml index 091039c7..c841b682 100644 --- a/.github/workflows/build-toolversions-task.yml +++ b/.github/workflows/build-toolversions-task.yml @@ -69,6 +69,7 @@ jobs: with: # Branch-suffixed so both matrix legs coexist; the inner filename # stays `latest.ver` for main (the m4 `include({{latest.ver}})` - # token), consumed by build-dockerreadme-task.yml. + # token), consumed by publish-docker-readme-task.yml. name: versions-${{ inputs.branch }} path: ${{ runner.temp }}/versions/${{ inputs.branch == 'main' && 'latest.ver' || 'develop.ver' }} + retention-days: 1 diff --git a/.github/workflows/build-dockerreadme-task.yml b/.github/workflows/publish-docker-readme-task.yml similarity index 96% rename from .github/workflows/build-dockerreadme-task.yml rename to .github/workflows/publish-docker-readme-task.yml index 0061269f..242d5bb9 100644 --- a/.github/workflows/build-dockerreadme-task.yml +++ b/.github/workflows/publish-docker-readme-task.yml @@ -1,4 +1,4 @@ -name: Create Docker README.md task +name: Publish Docker Hub readme task on: workflow_call: @@ -15,7 +15,7 @@ on: jobs: docker-readme: - name: Create Docker README.md job + name: Publish Docker Hub readme job # Render only for main: there is a single Docker Hub README, and the m4 # template includes the `latest` image's tool versions. if: ${{ inputs.branch == 'main' }} diff --git a/.github/workflows/publish-release.yml b/.github/workflows/publish-release.yml index 2e559e30..3a0e70e3 100644 --- a/.github/workflows/publish-release.yml +++ b/.github/workflows/publish-release.yml @@ -95,13 +95,13 @@ jobs: branch: ${{ matrix.branch }} docker-readme: - name: Create Docker README.md job + name: Publish Docker Hub readme job needs: [setup, tool-versions] if: ${{ needs.setup.outputs.publish == 'true' }} strategy: matrix: branch: ${{ fromJSON(needs.setup.outputs.branches) }} - uses: ./.github/workflows/build-dockerreadme-task.yml + uses: ./.github/workflows/publish-docker-readme-task.yml secrets: inherit with: # The task self-gates to `main`; the develop leg is a no-op. diff --git a/.markdownlint-cli2.jsonc b/.markdownlint-cli2.jsonc index c6a57141..4afb100e 100644 --- a/.markdownlint-cli2.jsonc +++ b/.markdownlint-cli2.jsonc @@ -7,7 +7,8 @@ "MD033": false, // Require fenced code blocks over the legacy 4-space-indented style. "MD046": { "style": "fenced" }, - // Wide tables are intentional where wrapping cells breaks GitHub rendering. + // MD060 (table column style) is not enforced - allow both compact + // (`|a|b|`) and padded (`| a | b |`) table pipe spacing. "MD060": false }, "gitignore": true diff --git a/.vscode/tasks.json b/.vscode/tasks.json index 1cc35902..00b69b74 100644 --- a/.vscode/tasks.json +++ b/.vscode/tasks.json @@ -37,6 +37,7 @@ "showReuseMessage": false, "clear": false }, + "dependsOrder": "sequence", "dependsOn": [ "CSharpier Format", ".Net Build" diff --git a/AGENTS.md b/AGENTS.md index 30311d63..37b39ad7 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,134 +1,187 @@ -# Instructions for AI Coding Agents - -**PlexCleaner** is a C# .NET utility that optimizes media files for Direct Play in Plex, Emby, Jellyfin, etc. - -For comprehensive coding standards and detailed conventions, refer to [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) and [`CODESTYLE.md`](./CODESTYLE.md). - -## Git and Commit Rules - -- **Default to staging, not committing.** Stage changes with `git add` and leave `git commit` to the developer unless the developer has explicitly authorized the agent to commit for the current ask ("commit this", "open a PR", etc.). Authorization is scope-bound — it covers the commits needed for that specific task, not a blanket commit license. -- **All commits must be cryptographically signed (SSH or GPG).** Branch protection enforces this on both branches; unsigned commits are rejected on push. Signing depends on environment configuration (`git config commit.gpgsign true`, a configured `user.signingkey`, and a loaded signing agent). If signing is not configured in the environment, **do not commit** — surface the missing config to the developer and stop at `git add`. Verify before any agent-authored commit (`git config --get commit.gpgsign && ssh-add -L`, or the GPG equivalent). -- **Never force push.** Do not run `git push --force` or `git push --force-with-lease` under any circumstances. Force pushing rewrites shared history and can cause data loss. -- **Never run destructive git commands** (`git reset --hard`, `git checkout .`, `git restore .`, `git clean -f`) without explicit developer instruction. -- **The `develop → main` release merge is maintainer-only.** An agent may drive `feature → develop` PRs end-to-end (commit, push, review loop, squash-merge) when authorized, but never self-merges a release to `main` — prepare it and hand it off. - -## Branches and merging - -- Pipeline is `feature → develop → main`. Both branches are protected by branch rulesets; everything lands via PR. -- **Feature → develop PRs squash-merge** (single commit on develop, PR title becomes the commit message; never rebase-merge). -- **Develop → main PRs merge-commit** (one merge commit on main per release, develop's tip becomes a second parent and stays in main's ancestry — see [Develop → Main Promotion](#develop--main-promotion)). -- Open feature PRs against `develop`. `develop → main` is how stable releases are cut. - -Repo settings reflect this: `allow_merge_commit=true`, `allow_squash_merge=true`, `allow_rebase_merge=false`, `allow_auto_merge=true`. The `develop` ruleset enforces `allowed_merge_methods=["squash"]` and `required_linear_history`. The `main` ruleset enforces `allowed_merge_methods=["merge"]` and intentionally omits linear-history (the develop → main merge commit is non-linear by design). - -## Merging a PR - -**Never merge a PR without `copilot-pull-request-reviewer[bot]` (shown as "Copilot" in the GitHub UI; the `[bot]` suffix is its actual login) having posted a clean re-review on the latest commit** — defined as a review whose `commit_id` (or GraphQL `commit.oid`) equals the PR's `headRefOid`, with no new unresolved inline threads (Copilot in this repo posts `COMMENTED` reviews, not `APPROVED`, so a clean COMMENTED review with zero open threads is the "no issues found" outcome). `mergeStateStatus: CLEAN` only confirms ruleset gates (thread resolution, status checks, signatures); it does not confirm Copilot has re-evaluated the latest changes. - -After resolving Copilot's threads or pushing fixes: - -1. **Re-request a Copilot review programmatically** on the current head via the GraphQL `requestReviews` mutation — auto-review-on-push fires inconsistently, so don't wait on it. The full mechanics (bot node-id discovery, the mutation, head-SHA coverage check, thread reply/resolve, bounded retry) are in the [GitHub Copilot Review Runbook](./.github/copilot-instructions.md#github-copilot-review-runbook). The agent can drive this loop end-to-end without a maintainer clicking "re-request review" in the UI. -2. Verify Copilot's most recent review targets the current head — compare its `commit.oid`/`commit_id` to `headRefOid`, not timestamps (multiple reviews and authors clutter the list, and timestamp drift is unreliable). -3. If the fresh review is `COMMENTED` with zero unresolved inline threads (or `APPROVED`), the PR is good to merge. -4. If the fresh review introduces new concerns (inline threads or body-level objections), address them and loop. -5. **If Copilot does not re-review within a reasonable window (~5 min) after re-requesting**, retry per the runbook's bounded-retry workflow (up to three total); if still missing, mark the review blocked and escalate to the maintainer. Silence is not approval. - -This applies to every human-authored PR (feature → develop, develop → main). The merge-bot workflow's auto-merge of dependabot bumps is the only exception and is governed separately by the `update-type` filter. - -## Develop → Main Promotion - -Use the **"Create a merge commit"** option on develop → main PRs. Repo rulesets are split: PRs into `develop` are squash-only (linear history); PRs into `main` are merge-commit only. Clicking "Create a merge commit" on a develop → main PR produces a merge commit on main whose second parent is develop's tip — so develop becomes a real ancestor of main, and the *next* develop → main PR has a clean merge base (no recurring conflicts, no behind-base churn). - -Under any squash-only setup this would be a recurring pain point: each develop → main squash drops develop's ancestry and forces a per-cycle admin-bypass merge commit on develop to resync. With merge-commit on main, that resync is unnecessary — main's history shows one merge commit per release (a feature, not a defect: each promotion is visible as a single auditable node), and develop stays linear. - -**Immediately after a develop → main merge lands and main's publish workflows complete, bump the minor version in [version.json](version.json) on develop.** Open a small isolated feature PR `bump-version-X.Y` (e.g. `"version": "3.16"` → `"version": "3.17"`), squash into develop, and continue feature work from there. Without this bump, develop's next NBGV-computed prerelease (`3.16.-g{sha}`) is *numerically lower* than the stable that just shipped (`3.16.`), which is visibly confusing in HISTORY.md, `--version` output, and consumer update prompts. Bumping ensures every develop prerelease is `3.17.-g{sha}` — visibly newer than main's `3.16.`. Don't bundle the bump with other work; keep the PR isolated so the version change is unambiguous in git blame. - -A **maintenance** develop -> main promotion - dependency bumps, CI/doc fixes, template re-syncs, not a release - holds main's version: run `git checkout main -- version.json` on the promotion branch before opening the PR, so main advances only its git height (a patch), not its minor, and develop keeps its lead. Only a release promotion carries develop's bumped version to main. - -## Release flow - -PlexCleaner is a "pull" project: consumers (`docker pull ptr727/plexcleaner:latest`, `docker pull ptr727/plexcleaner:develop`, GitHub Releases) track both branches. It uses a **two-phase model** that decouples merging from publishing: - -- **PRs smoke-test only.** [test-pull-request.yml](.github/workflows/test-pull-request.yml) always runs unit tests, then a [`dorny/paths-filter`](.github/workflows/test-pull-request.yml) `changes` job gates a **reduced** build of only the changed targets (Docker `linux/amd64` only, executable on a `linux-x64` + `win-x64` subset), never pushing. Build-workflow files are intentionally not in the path filters — a filter can't tell a logic change from an action-version bump — so a workflow-only change isn't smoke-built; the reusable workflows are exercised by the next run that uses them. There is no CI workflow-lint job; lint workflow edits with `actionlint` locally before pushing. The `changes` job is in the `Check pull request workflow status` aggregator's `needs` and **must succeed** (not just "not fail") — a paths-filter error must never let a target-changing PR merge with its smoke build silently skipped. -- **Merges don't publish by default.** [publish-release.yml](.github/workflows/publish-release.yml) is the **sole publisher**: its **weekly schedule** (Mondays 02:00 UTC) and **manual `workflow_dispatch`** always do the full multi-arch build/publish of **both** `main` and `develop` (a branch matrix in one run). Its `push` trigger publishes only when the **`PUBLISH_ON_MERGE` repository variable** is `true` (opt-in legacy continuous-release). Unset/`false` = two-phase: routine merges to `develop`/`main` only smoke-build, and `:latest`/`:develop` Docker tags + GitHub releases refresh on the weekly run instead of on every merge. - -A `setup` job computes the plan: `push` ⇒ the pushed branch with `publish = (vars.PUBLISH_ON_MERGE == 'true')`; `schedule`/`dispatch` ⇒ both branches with `publish = true`. The `publish` job is a `matrix.branch` fan-out over [build-release-task.yml](.github/workflows/build-release-task.yml); `tool-versions`, `docker-readme` (main only), and `date-badge` (main only) run after it. - -Branch-aware config keys off the **`branch` input** threaded through every reusable task — **never `github.ref_name`** (the publisher builds `develop` from a run whose `github.ref_name` is `main`, so a fallback would mislabel it). `main` ⇒ Release / `latest` / stable release; anything else ⇒ Debug / `develop` / prerelease. The GitHub release's `target_commitish` is pinned to NBGV's `GitCommitId` (the exact built commit) — not `github.sha` (wrong on the develop leg) and not a branch name (a moving ref); `get-version-task.yml` surfaces `GitCommitId` as an output. The release step is skipped when a release for the computed `SemVer2` tag already exists (no-op weekly republish), except on `workflow_dispatch` (which can refresh a partial release). - -**Per-target subsetting (derived projects).** `build-release-task.yml` has per-target `enable_*` gates and self-contained leaf tasks, so a project that drops a target deletes: its `build--task.yml`, the matching job + `github-release` `needs` entry in `build-release-task.yml`, and its path-filter entry in `test-pull-request.yml`. Versioning, badge, tool-versions, Docker README, merge-bot, and Dependabot are target-agnostic. PlexCleaner's targets are the **Docker image** and the **console executable** only (no NuGet/PyPI). - -Bot-merged PRs (Dependabot) still trigger `publish-release.yml` because the merge-bot uses an App token (see the merge-bot section) — under the default two-phase model that push run is a no-op publish unless `PUBLISH_ON_MERGE` is set. - -## Dependabot - -[.github/dependabot.yml](.github/dependabot.yml) targets **both `main` and `develop`** with two ecosystems each (`nuget`, `github-actions`), grouped per ecosystem, daily. The duplication is intentional: both branches ship from the weekly publisher (and on every merge when `PUBLISH_ON_MERGE` is set), so develop must not drift from main's dependency baseline. A NuGet major bump landing on develop should land on main on the next promotion cycle, not weeks later. - -Major NuGet bumps are not auto-merged by [merge-bot-pull-request.yml](.github/workflows/merge-bot-pull-request.yml) — they require human review. Major GitHub Actions bumps are auto-merged because the workflow execution itself is the validation surface. - -## GitHub Actions pinning - -Every third-party action in `.github/workflows/*.yml` is pinned to a full commit SHA with a trailing comment matching the upstream release tag, e.g. `uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2`. The comment is whatever tag the action's repo actually publishes — typically `# vX.Y.Z`, but use `# v3` if upstream only publishes major-only tags (e.g. `addnab/docker-run-action`) and `# master` if the action ships only a moving branch (rare). Floating refs without a SHA (`@v6`, `@main`, `@master`) are never used. Local reusable workflows (`./.github/workflows/*.yml`) are referenced by path and don't need pinning. - -**Why:** Floating tags can be silently re-pointed by the action's owner (or by a compromised account) to malicious code; a SHA pin is immutable. Matching the comment to upstream's actual release tag (rather than fabricating one) lets dependabot rewrite both the SHA and the comment together when bumping. - -When adding a new `uses:` line, resolve the latest release's commit SHA (`gh api repos///releases/latest`) and copy its `tag_name` into the comment verbatim. Don't ship a floating tag and "pin it later". - -## Merge bot - -[merge-bot-pull-request.yml](.github/workflows/merge-bot-pull-request.yml) auto-merges Dependabot PRs. Two key design choices: - -- **Branch-aware merge method**: the script picks `--squash` for PRs targeting develop and `--merge` for PRs targeting main, matching each ruleset's `allowed_merge_methods`. An unknown base branch is a hard error. -- **App token, not GITHUB_TOKEN**: the merge step uses a token minted by `actions/create-github-app-token` from `CODEGEN_APP_CLIENT_ID` / `CODEGEN_APP_PRIVATE_KEY` secrets. Pushes authored by `GITHUB_TOKEN` are blocked from triggering downstream workflows by GitHub's recursion guard; without the App token, a Dependabot merge would silently skip `publish-release.yml` on the merge commit. Under the default two-phase model that push is a no-op publish (it only republishes when `PUBLISH_ON_MERGE` is `true`), but the App token keeps that opt-in path — and any future push-triggered workflow — working. - -The App secrets (`CODEGEN_APP_CLIENT_ID`, `CODEGEN_APP_PRIVATE_KEY`) must exist in **both** secret namespaces: Settings → Secrets and variables → **Actions**, and Settings → Secrets and variables → **Dependabot**. Since Sept 2021, GitHub injects only the Dependabot-namespace secrets when a Dependabot-authored `pull_request` event fires; the regular Actions namespace is not visible to that run. Without the Dependabot duplicate the App-token step gets empty inputs and merge-bot silently fails to auto-merge. (The trigger remains `pull_request`, not `pull_request_target` — the merge-bot doesn't check out PR code, but `pull_request` plus duplicated secrets is the simpler, less-permissive setup.) - -## Key Requirements for All Projects Derived from This Template - -### Build & Quality Standards - -- **Zero Warnings Policy**: All builds must complete without errors or warnings - - Use `CSharpier Format`, `.Net Format`, and `Husky.Net Run` tasks - -- **Code Analysis**: Enable all .NET analyzers - - `true` - - `latest-all` - -### Project Configuration - -- Common MSBuild properties (`TargetFramework`, `Nullable`, `ImplicitUsings`, `AnalysisLevel`, etc.) - live in `Directory.Build.props` at the solution root. Do not duplicate these in individual `.csproj` - files — only add a property to a `.csproj` when it is project-specific or overrides the shared default. -- All NuGet package versions are centralised in `Directory.Packages.props`. `PackageReference` elements - in `.csproj` files must not include a `Version` attribute. Asset metadata (`PrivateAssets`, - `IncludeAssets`) stays in the `.csproj` `PackageReference` element. - -### Development Environment - -- Target latest .NET SDK (currently .NET 10 with C# 14) -- Support Visual Studio Code (`.code-workspace`) and Visual Studio Community (`.slnx`) -- Support Linux, Windows, and macOS with correct line endings and permissions -- Use `.editorconfig` for style enforcement - -### Project Structure - -- **PlexCleaner**: CLI application -- **PlexCleanerTests**: Unit tests using xUnit and AwesomeAssertions -- **Sandbox**: Sandbox/testing utility project -- **Docker**: Multi-platform Linux containers - -### Testing - -- Use xUnit v3 and AwesomeAssertions -- Organize tests logically in separate files -- Follow Arrange-Act-Assert pattern -- Test naming: `MethodName_Scenario_ExpectedBehavior()` - -## Authoritative References - -For detailed specifications, see: - -- [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) - Complete coding conventions and style guide -- [`CODESTYLE.md`](./CODESTYLE.md) - Code style and formatting rules -- [`.editorconfig`](./.editorconfig) - Automated style enforcement -- Project task definitions - `CSharpier Format`, `.Net Build`, `.Net Format`, `.Net Outdated Upgrade`, `Husky.Net Run` +# Instructions for AI Coding Agents + +**PlexCleaner** is a .NET 10 CLI utility that optimizes media files for Direct Play in Plex/Emby/Jellyfin (converting containers to MKV, re-encoding incompatible codecs, managing tracks and language tags, verifying and repairing media, and monitoring folders for changes). It orchestrates external media tools - FFmpeg, HandBrake, MkvToolNix, MediaInfo, and 7-Zip - through CLI wrappers. It ships two release targets: a multi-arch Docker image (Docker Hub `ptr727/plexcleaner`) and standalone executables attached to GitHub Releases; consumers pull from Docker Hub or the GitHub releases on their own cadence. The repo also contains an xUnit test project (`PlexCleanerTests/`). + +This file is the canonical reference for cross-cutting AI-agent and workflow rules. C# code-style conventions live in [`CODESTYLE.md`](./CODESTYLE.md). Copilot review *mechanics* are owned by [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) - this file delegates them there explicitly (see "PR Review Etiquette" below). PlexCleaner's architecture, processing pipeline, and design patterns live in [`ARCHITECTURE.md`](./ARCHITECTURE.md). High-level summaries in other docs (e.g. README's Contributing section) are allowed when they link back here; don't duplicate the rules themselves. The app's **project-specific conventions** also live here and in `ARCHITECTURE.md`, **not** in `.github/copilot-instructions.md` - that file targets GitHub Copilot / VS Code specifically, while this file is the agent-agnostic one every coding agent reads, so any rule a reviewer must honor has to live here to be provider-independent. + +## Git and Commit Rules + +- **Default to staging, not committing.** Stage changes with `git add` and leave `git commit` to the developer unless the developer has explicitly authorized the agent to commit for the current ask ("commit this", "open a PR", etc.). Authorization is scope-bound - it covers the commits needed for that specific task, not a blanket commit license for the rest of the session. +- **All commits must be cryptographically signed (SSH or GPG).** Branch protection enforces this on both branches; unsigned commits are rejected on push. Signing depends on environment configuration - `git config commit.gpgsign true`, a configured `user.signingkey`, and a working signing agent (loaded `ssh-agent` for SSH, or `gpg-agent` for GPG). If signing is not configured in the environment, **do not commit** - surface the missing config to the developer and stop at `git add`. Verify before any agent-authored commit (`git config --get commit.gpgsign && ssh-add -L` or the GPG equivalent). **Signing must be live before the *first* commit, not retrofitted.** Turning on `Require signed commits` against a branch that already has unsigned commits forces a rewrite of that entire history to re-sign it - changing every commit SHA and making whoever does the rewrite the committer and signer of every commit (a rebase preserves the `author` field but not the original signatures; you cannot sign another contributor's commits for them). During new-repo setup, never create commits until signing is verified. +- **Never force push.** Do not run `git push --force` or `git push --force-with-lease` under any circumstances. Force pushing rewrites shared history and can cause data loss. +- **Never run destructive git commands** (`git reset --hard`, `git checkout .`, `git restore .`, `git clean -f`) without explicit developer instruction. +- **The `develop -> main` release merge is maintainer-only.** Drive `feature -> develop` PRs end-to-end when authorized (commit, push, Copilot review loop, squash-merge), but never self-merge a release to `main`. + +## Branching Model + +- `develop` is the integration branch. Feature branches -> `develop` is **squash-only**; develop is kept linear. +- `develop` -> `main` is **merge-commit only** (no squash, no rebase). Merge commits preserve develop's commit list as a real second-parent reference on main, which lets the release model attribute releases to the develop commits that produced them (relevant both for the weekly publish and the opt-in `PUBLISH_ON_MERGE` mode - see "Release Model" below). Branch protection enforces this: the develop ruleset allows only `squash`, the main ruleset allows only `merge`. +- All commits on both branches must be cryptographically signed (SSH or GPG). Squash and merge commits created via the GitHub UI are signed by GitHub's web-flow key. +- **`develop` is forward-only - no `main -> develop` back-merges.** The develop ruleset's squash-only setting physically blocks merge commits on develop. Historical back-merge commits visible in `git log` predate this rule and must not be repeated. +- **Both rulesets intentionally omit "Require branches to be up to date before merging" (`strict_required_status_checks_policy: false`), for two distinct reasons:** + - *Main* - the check is graph-based; it asks whether main's tip commit is reachable from develop, not whether the two branches have the same content. After any develop -> main release, main's tip is a brand-new merge commit that develop's history doesn't contain. Forward-only develop never adds it (no back-merge of main into develop), so the check would fail on every subsequent release. + - *Develop* - bot auto-merge incompatibility. When two bot PRs against develop land in the same minute (e.g. two grouped Dependabot PRs from the same daily run), the first to merge pushes the second into `mergeStateStatus: BEHIND`. GitHub's auto-merge will not fire while the strict flag is on, and nothing in the workflow set auto-updates a bot branch in that window - the merge-bot enables auto-merge via `gh pr merge --auto` but never rebases a stalled branch onto base (see [`merge-bot-pull-request.yml`](./.github/workflows/merge-bot-pull-request.yml)). Real file-level conflicts are still caught textually (`mergeable: CONFLICTING` blocks merge regardless); semantic-but-not-textual conflicts that combine cleanly are caught by the post-merge develop CI run rather than pre-merge. Do not reintroduce the strict flag on develop thinking it's hygiene - it breaks bot auto-merge. +- **Dependabot targets both `main` and `develop` in parallel.** [`.github/dependabot.yml`](./.github/dependabot.yml) duplicates every ecosystem entry (one per branch). Each branch absorbs its own bot PRs independently, so neither falls behind, and the forward-only rule still holds (nothing is back-merged from main to develop - both branches receive their updates directly). Parallel auto-merge across same-batch bot PRs is race-proof only because both rulesets have the strict "up to date" flag off (see bullet above). The merge-bot ([`.github/workflows/merge-bot-pull-request.yml`](./.github/workflows/merge-bot-pull-request.yml)) dispatches `--squash` or `--merge` from each PR's base ref via a `case` statement so the form matches the ruleset on either base. Dependabot **security** PRs (CVE-driven) always open against the repo default branch (`main`) regardless of `target-branch` - the same `case` statement covers them. Semver-major NuGet bumps gate on human review; everything else auto-merges. +- **Maintainer repair commits on a Dependabot PR still auto-merge.** The merge-bot's `merge-dependabot` job gates on the PR *author* (`dependabot[bot]`), not the event actor, and fires on `opened` / `reopened` / `synchronize`, so a maintainer's fix-up commits pushed onto a Dependabot branch still auto-merge once CI passes (auto-merge is re-asserted idempotently via `gh pr merge --auto`). It runs only for Dependabot-authored PRs that originate from this repository, not forks. +- **App-token workflows use Client ID, not App ID.** `actions/create-github-app-token` deprecated the numeric `app-id` input in v3.0.0; the merge-bot uses `client-id: ${{ secrets.CODEGEN_APP_CLIENT_ID }}` (with `private-key: ${{ secrets.CODEGEN_APP_PRIVATE_KEY }}`). The App token - not `GITHUB_TOKEN` - is required so the merge push is committed by the App and fires downstream workflows (`GITHUB_TOKEN` pushes are blocked from triggering further runs by GitHub's recursion guard). When adding new App-token call sites, use the same form - do not reintroduce `app-id`. +- **Why parallel dual-target rather than develop-only with eventual flow-through:** consumers pull the Docker image and the release executables from `main` directly. A develop-only model would leave `main` running stale code during long-running develop features, so both branches receive their own bot updates on their own cadence and each stays current. + +## Release Model + +This repo uses a **two-phase model by default**: PRs build fast, publishing is batched weekly. The load-bearing rules: + +- **PRs smoke-test only.** [`test-pull-request.yml`](./.github/workflows/test-pull-request.yml) always runs unit tests, then a `dorny/paths-filter` `changes` job gates a **reduced, never-published** build of only the changed targets (Docker `linux/amd64` only, plus the executable on a representative runtime subset), with no push. Build-workflow files are intentionally not in the path filters - a filter can't tell a logic change from an action-version bump - so a workflow-only change isn't smoke-built; the reusable workflows are exercised by the next run that uses them. There is no CI workflow-lint job; lint workflow edits with `actionlint` locally before pushing. +- **Merges don't publish by default.** [`publish-release.yml`](./.github/workflows/publish-release.yml) is the sole publisher: its **weekly schedule** (Mondays 02:00 UTC) and **manual `workflow_dispatch`** always do the full build/publish of **both** `main` and `develop` (a branch matrix). Its `push` trigger publishes only when the **`PUBLISH_ON_MERGE` repository variable** is `true` (opt-in legacy continuous-release). Unset/`false` = two-phase. +- **Required check.** The `changes` job is in the `Check pull request workflow status` aggregator's `needs` and **must succeed** (not just "not fail") - a paths-filter error must never let a target-changing PR merge with its smoke build silently skipped. Skipped smoke jobs (no matching change) pass; `failure`/`cancelled` blocks. +- **Reusable-task parameter contract.** [`build-release-task.yml`](./.github/workflows/build-release-task.yml) and the leaf `build-*-task.yml` workflows take `ref` (git ref to check out/version), `branch` (logical branch driving config/tags/prerelease - `main` => Release/`latest`/non-prerelease, else Debug/`develop`/prerelease), and where relevant `smoke`. **Branch-derived config keys off `inputs.branch`, never `github.ref_name`** - the publisher's matrix builds `develop` from a run whose `github.ref_name` is `main`, so `ref_name` would be wrong. Artifact names are branch-suffixed so both matrix legs coexist in one run. [`get-version-task.yml`](./.github/workflows/get-version-task.yml) takes a `ref` so NBGV versions the right branch, and exposes `GitCommitId` so the release tag and built artifacts pin to the exact built commit. +- **The release-asset seam.** A target contributes files to the GitHub release by uploading a workflow artifact named `release-asset--`. The `github-release` job collects every `release-asset--*` artifact by pattern and **never names a build job**, so the tag-the-commit + create-the-release + attach-the-assets logic is reusable **verbatim**. PlexCleaner's executable target ([`build-executable-task.yml`](./.github/workflows/build-executable-task.yml)) uses this seam - it `dotnet publish`es the standalone executables and uploads them as `release-asset--*`. The Docker target ([`build-docker-task.yml`](./.github/workflows/build-docker-task.yml)) pushes multi-arch tags directly to Docker Hub (`latest` for main, `develop` for develop) and contributes **no** `release-asset-*`. The Docker Hub repository overview is pushed separately by [`publish-docker-readme-task.yml`](./.github/workflows/publish-docker-readme-task.yml), gated to `main`. +- **Versioning is semantic and maintainer-controlled.** The `version` (major.minor) in [`version.json`](./version.json) is the version floor; NBGV appends the git height (the SemVer patch position) for the build version. `main` (the public release ref) builds a stable `X.Y.`; `develop` builds a prerelease `X.Y.-g`. `version.json`'s `publicReleaseRefSpec` is `^refs/heads/main$`. The maintainer edits `version.json`; dependency bumps, CI/workflow fixes, doc edits, and template re-syncs leave it untouched. + - **Bump `version.json` only for functional changes, by maintainer instruction.** Raise the major/minor when the work being introduced warrants a new semantic version - a new feature, a behavior change, a breaking change - and do it in the PR that introduces that work (typically on `develop`). Do **not** bump on a fixed cadence or mechanically after a release. NBGV advances the patch (git height) on every commit automatically, so a release always gets a fresh build version without any `version.json` edit. + - **No post-release bump; no develop-ahead requirement.** NBGV advances the patch (git height) on every commit, so a release always gets a fresh build version with no `version.json` edit and there is no `bump-version-X.Y` PR after a release. A `develop -> main` promotion carries whatever `version.json` is current: a promotion with a functional bump releases that new version on `main`; a maintenance-only promotion (dependency bumps, CI/doc fixes, template re-syncs) carries the unchanged `version.json` and `main` advances only its NBGV height. + +## Pull Request Title and Commit Message Conventions + +### Format + +- Imperative subject summarizing the change, <=72 characters, no trailing period. ("Add Direct Play seek-index verification", not "Added X" or "Adds X".) +- Optional body, blank-line separated, explaining *why* the change is being made when that's non-obvious. The diff shows *what*. + +### Rules + +- Don't write `update stuff`, `wip`, or other vague titles. (Dependabot's default `Bump X from Y to Z` titles are fine - keep them.) +- Don't add `Co-Authored-By:` lines unless the developer explicitly asks. +- Don't put release-bump magnitude in the title - no "minor", "patch", "release v0.2.0", etc. Nerdbank.GitVersioning computes the next release version from `version.json` + git history. Dependency versions in dependency-bump titles are fine and expected. +- Use US English spelling and match the existing heading style of the file you're editing: title case with lowercase short bind words (a, an, the, and, but, or, of, in, on, at, to, by, for, from); hyphenated compounds capitalize both parts unless the second is a short preposition (*Built-in*, *Direct-Play*, *24-Hour*). + +### Examples + +```text +Add Direct Play seek-index verification +Pin softprops/action-gh-release to commit SHA +Remove embedded closed captions during remux +Bump xunit.v3 from 3.2.2 to 3.3.0 +Clarify HandBrake custom-options usage in README +``` + +## Documentation Style Conventions + +### Markdown + +- Use reference-style links for any URL referenced more than once or appearing in lists; alphabetize the reference definitions block. +- Inline single-use relative links (e.g. `[CODESTYLE.md](./CODESTYLE.md)`) are fine. +- One logical paragraph per line; no hard-wrap line-length limit. For an intentional hard line break within a block - stacked badges, status, or license lines - end the line with a trailing backslash (`\`); this explicit form is preferred over trailing whitespace and is not treated as a paragraph split. +- Headings follow the title-case-with-short-bind-words rule from the PR-title section. +- **Write docs in the current state, not as a change from a prior one.** The reader has no memory of the previous behavior, so describe what *is*: "X does Y", never "X *now* does Y", "X *no longer* does Z", or "changed/switched/restored to Y". Before/after framing belongs in changelogs, commit messages, and PR descriptions - not in `README.md` or other living docs. + +### Comments + +Applies to code and workflow (`#`) comments alike. + +- Comment only when the code does not explain itself or the logic is genuinely complex. Self-evident code needs no comment. +- Write for the human reading *this* project's code now: state what the code does and only the non-obvious *why*. No cross-project references (do not name other repos), no historic or design narrative, no rule citations - governance lives in this file, not echoed inline. +- Match the surrounding code's line length (typically ~120), not an 80-column wrap. + +### Character Set + +- **Write ASCII in all agent-authored text** - documentation, code, comments, commit messages, and PR descriptions. The agent does not introduce non-ASCII characters. Replace typographic Unicode with its ASCII equivalent on sight: + - em dash (U+2014) and en dash (U+2013) -> hyphen `-` (use a spaced ` - ` for an em-dash-style clause break) + - right arrow (U+2192) -> `->`; double arrow (U+21D2) -> `=>` + - less-than-or-equal (U+2264) -> `<=`; greater-than-or-equal (U+2265) -> `>=` + - curly quotes (U+2018/U+2019/U+201C/U+201D) -> straight `'` and `"`; ellipsis (U+2026) -> `...` +- **Allowed non-ASCII (two narrow exceptions):** + - **Scientific or technical symbols with no clean ASCII equivalent** - e.g. ohm, micro, degree, pi. Keep the symbol; do not approximate it away. + - **Unicode the developer deliberately typed** - emoji used for emphasis or as callout markers (for example the warning/info markers a maintainer placed in `README.md`). Preserve it; never strip the developer's own characters. This carve-out is for developer-authored text, not a license for the agent to add emoji. + +### Line Endings + +- [`.editorconfig`](./.editorconfig) defines the correct ending per file type (CRLF for `.md`, `.cs`, XML/`.csproj`/`.props`, `.yml`/`.yaml`, `.json`, `.cmd`/`.bat`/`.ps1`; LF for `.sh`), and [`.gitattributes`](./.gitattributes) (`* -text`) stops git from normalizing. The defaults + per-extension EOL block is always-verbatim from the template; the `[*.cs]`/ReSharper style block is .NET-only and is carried because this repo ships .NET. +- **Editing an existing file: preserve its current line endings** - do not reflow them as a side effect of a content change, even if the file is already non-compliant. After any programmatic edit, verify with `git diff --stat` (only changed lines) and `file ` (expected ending). Bring a non-compliant file to its `.editorconfig` ending only as a deliberate, isolated EOL-only change. + +### Quantitative Claims + +- Any quantitative claim in `README.md` (counts, sizes, version floors, supported platforms) must be verified against current code. If a doc number is derived from a code constant, mark the dependency in a source-code comment so the next editor knows to update both. + +## PR Review Etiquette + +The repo runs a review loop on every PR: local agent iteration plus remote automated review (GitHub Copilot is the configured reviewer). Treat this as a contract regardless of which local agent authored the changes. + +### Expected Review Loop + +1. Push changes to the PR branch. +2. Re-request a review for the **current head SHA**. Auto-trigger is unreliable, so request it explicitly via the `requestReviews` GraphQL mutation (now reliable end-to-end - see the runbook); the UI is only a fallback. +3. Wait for review activity on that head. A completed review that raises **no findings** is a valid terminal outcome for that head - proceed; do not re-trigger it or treat the absence of comments as a missing review. +4. Triage findings. +5. Apply fixes or write a rationale for declines. +6. Reply to each thread and resolve what was addressed. +7. Re-run the loop after every fix push until no actionable findings remain. + +`mergeStateStatus: CLEAN` only checks required statuses; it does not block on bot review comments. Drive the loop to green - review confirmed on the latest head SHA and every actionable finding closed - and then **wait for the maintainer's explicit permission to merge**. The agent does not merge on its own (consistent with "default to staging"; merging is maintainer-authorized). + +For provider-specific mechanics (how to request review, query review state, post replies, resolve threads), see the **GitHub Copilot Review Runbook** in [.github/copilot-instructions.md](./.github/copilot-instructions.md). This file owns the contract; that file owns the mechanics. + +### Triaging Review Comments + +For each comment, classify before responding: + +- **Bug** - wrong behavior, missing test coverage, or a real divergence between code and docs. Fix it. Reply with the fixing commit SHA when done. +- **Style/convention** - the comment cites a rule from this file or a language-specific style guide. Two cases: + - The cited rule matches what the existing codebase already does -> fix the offending code. + - The cited rule contradicts what's in the tree, or industry norm -> **update the rule instead of the code**. The rule is wrong, not the code. Bouncing the same code across rounds is the symptom of a wrong rule. Heuristic: three rounds on the same style category means the rule needs adjusting and the user should authorize the rule change. +- **Architectural opinion** - the comment proposes a different design ("constrain this to disabled-by-default", "move it elsewhere", "add a runtime guardrail"). This is judgment, not a bug. Surface it to the user with a recommendation; don't apply unilaterally. + +### Responding and Resolution Expectations + +Reply inline with either the fixing commit SHA (for accepted issues) or a concise rationale (for declines). Resolve review threads when addressed or intentionally declined with rationale. Issue-level comments (those at `repos/.../issues//comments` rather than tied to a specific line) have no resolution action - acknowledge with a reply if needed and move on. + +After the final push on a PR, sweep older threads from earlier rounds whose code paths no longer exist; otherwise stale unresolved markers remain in the review UI. + +### Escalating to the User + +Bring the user in when: + +- **Genuine design trade-off** surfaces (fail-open vs fail-closed, narrow vs broad refactor scope, "should we add a guardrail or trust the docstring"). Triage, recommend, ask. +- **Repeated friction** across rounds without convergence - that's the rule-needs-updating signal. Stop, summarize the pattern, and let the user authorize the rule change. +- **Architectural redesign** is requested rather than a bug fix. Surface with a recommendation; never apply unilaterally. + +Anti-pattern: don't keep flipping the code on the same style point. Flip the rule once and stick to the rule. + +## Staying in Sync with the Template + +This repo is derived from [`ptr727/ProjectTemplate`](https://github.com/ptr727/ProjectTemplate) and re-syncs against it periodically, not just at creation. + +- **Verbatim carries.** Pull the current template version of each shared artifact and re-apply it, adapting only this repo's placeholders: [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) (the Copilot review runbook - change only the ``/``/`` values in its API snippets), [`.markdownlint-cli2.jsonc`](./.markdownlint-cli2.jsonc), [`.editorconfig`](./.editorconfig), [`.gitattributes`](./.gitattributes), and this file's [PR Review Etiquette](#pr-review-etiquette) section. The `.editorconfig` EOL/per-extension block is always-verbatim; its `[*.cs]`/ReSharper block is .NET-only and is carried here. Keep `copilot-instructions.md` **narrow** (provider mechanics plus the commit/PR-title summary); project-specific conventions live in this file and the architecture deep-dive lives in [`ARCHITECTURE.md`](./ARCHITECTURE.md), not there - non-Copilot agents are not directed to that file. +- **CODESTYLE.md.** Re-sync the whole file from the template, then keep the **General** section plus the **.NET** language section and drop the language sections this repo doesn't ship (this repo is .NET-only). Repo-root placement is load-bearing - `AGENTS.md` and `.github/copilot-instructions.md` link it by relative path. Adapt the in-section repo-specific bits: the .NET project-folder list, the `InternalsVisibleTo` project names, and the VS Code task labels. Replacing the file wholesale and dropping whole sections is simpler to keep current than hand-editing per-language snippets. +- **.vscode/tasks.json.** Carry the named **clean-compile** task definitions verbatim - `.Net Build`, `CSharpier Format`, and `.Net Format` (which chains the first two then `dotnet format style --verify-no-changes`). Their names are owned by the `CODESTYLE.md` ".NET" section and their command sequence + arguments are the canonical clean-compile spec; don't loosen them. Convenience tasks are the adapt zone. +- **Release notes.** Keep a short release-notes summary in [`README.md`](./README.md) and the full history in [`HISTORY.md`](./HISTORY.md); update both when cutting a release. +- **Report drift upstream.** When a re-sync surfaces a template gap, an outdated instruction, or something that bit this repo and would bite the next derived repo, open an issue in [`ptr727/ProjectTemplate`](https://github.com/ptr727/ProjectTemplate) rather than only patching locally - the template is the single source of truth, and this upstream-issue rule is this repo's only cross-repo obligation. Do not maintain or reference a "known downstream" registry, and do not name sibling repositories in docs, comments, or workflows - that registry and the maintainer fan-out duty live in the template hub only. + +## Workflow YAML Conventions + +These conventions describe the target state. New and modified workflows must respect them; the rest of the repo is expected to be brought up to the same standard. + +- **Action pinning**: pin **every** action - first-party (`actions/*`) and third-party - to a commit SHA with a trailing `# vX.Y.Z` comment, so Dependabot can still bump it but a tag swap can't change the executed code. Use `# vX` (major-only) only when the upstream's floating major tag doesn't correspond to a specific patch/minor release SHA - pinning to the floating-tag SHA still gives the SHA guarantee, the version comment just records the major line. Every action in this repo, including [`dotnet/nbgv`](./.github/workflows/get-version-task.yml), is SHA-pinned with no exceptions. +- **Filename**: reusable workflows (those with `on: workflow_call`) end in `-task.yml`. Entry-point workflows (`on: push` / `pull_request` / `schedule` / `workflow_dispatch`) do NOT use the `-task` suffix; they end with what they do - `-pull-request.yml`, `-release.yml`, etc. The suffix carries semantic meaning: a `-task.yml` file is meant to be `uses:`-d, never triggered directly. +- **Workflow `name:`** (the top-level `name:` field): reusable workflow names end in **"task"** (e.g. `Build executable task`); entry-point workflow names end in **"action"** (e.g. `Publish project release action`, `Test pull request action`). The displayed action name in the GitHub Actions UI tells you at a glance whether you're looking at an orchestrator or a callee. +- **Job and step `name:` suffixes**: every job's `name:` ends in **"job"**; every step's `name:` ends in **"step"**. **Exception**: a job whose `name:` is also referenced as a required-status-check `context:` in a branch ruleset (currently `Check pull request workflow status` in `test-pull-request.yml`) keeps the ruleset-bound name verbatim - renaming would silently break required-status-check enforcement. Do not "fix" that name; if a future job becomes ruleset-bound, mark it the same way. +- **Concurrency**: top-level workflows declare `concurrency: { group: '${{ github.workflow }}-${{ github.ref }}', cancel-in-progress: true }` so a fresh push supersedes an in-flight run on the same ref. **Documented exception** (records the rationale inline in its header comment): [`publish-release.yml`](./.github/workflows/publish-release.yml) uses both a **global, ref-independent group** for real publishes (`group: ${{ github.workflow }}`, dropping the usual `-${{ github.ref }}`) and `cancel-in-progress: false`. Its schedule/dispatch runs publish both branches regardless of the triggering ref, so a ref-scoped group would let a scheduled run (ref `main`) and a manual dispatch (ref `develop`) run concurrently and double-publish; and cancelling a publish mid-flight can leave a half-created GitHub release or a partially pushed Docker tag set. Non-publishing (two-phase default) `push` runs get a unique per-run group so they never queue behind a real publish. +- **Shells**: multi-line `run:` blocks with bash start with `set -euo pipefail` - fail fast, fail on undefined vars, fail on a failed pipe segment. +- **Conditionals**: multi-line `if:` uses folded scalar `if: >-` so YAML preserves whitespace correctly. Literal block (`if: |`) is wrong because it embeds newlines inside the boolean expression. +- **Boolean inputs**: workflows triggered both via `workflow_call` and `workflow_dispatch` must declare each boolean input in *both* trigger blocks - one definition does not propagate to the other. `workflow_call` delivers booleans as actual booleans; `workflow_dispatch` delivers them as the *strings* `"true"`/`"false"`. Any `if:` consuming a boolean input must compare against both forms - `if: ${{ inputs.foo == true || inputs.foo == 'true' }}`. +- **Reusable workflows**: job-level `permissions:` are validated *before* the `if:` evaluates, so even a skipped job needs valid permissions declared. A `release` job with `permissions: contents: write` and `if: ${{ inputs.publish }}` will still cause `startup_failure` on a caller that doesn't grant `contents: write`. Either declare permissions at the call site, or omit the inner block and inherit. +- **Allowlist `success` and `skipped` explicitly** when chaining jobs across optional dependencies - `!= 'failure'` lets `cancelled` through (timeout, runner failure, manual cancel). Use `(needs.X.result == 'success' || needs.X.result == 'skipped')`. +- **Artifact retention**: intermediate build artifacts (`actions/upload-artifact`) are consumed by a later job in the same run, so set `retention-days: 1` - the default 90-day retention otherwise piles up against the account-wide artifact-storage quota. The durable copies live on the GitHub release, not in workflow artifacts. +- **Docker layer cache**: cache to/from a registry tag (`type=registry`, e.g. `buildcache-` on Docker Hub), not the GitHub Actions cache (`type=gha`), to keep large image layers off the 10 GB Actions cache. +- **Tag pinning on releases**: when using `softprops/action-gh-release` (or any tag-creating action), pass `target_commitish` explicitly - without it, GitHub's REST API defaults the new tag to the repository's default branch instead of the commit that built the artifact. Pin it to the **exact built commit's SHA** (the publisher uses NBGV's `GitCommitId` output), not `github.sha` (wrong branch in the publisher's branch matrix - a `develop` leg runs with `github.sha` = main's tip) and not a branch name (a moving ref that a mid-run commit could advance past the built tree). + +## Project Structure + +- **PlexCleaner** (`PlexCleaner/PlexCleaner.csproj`) + - The CLI application - orchestrates FFmpeg, HandBrake, MkvToolNix, MediaInfo, and 7-Zip to optimize media for Direct Play. + - Target framework: .NET 10.0, AOT compiled (`true`). Internals are exposed to the test project via `InternalsVisibleTo`. +- **PlexCleanerTests** (`PlexCleanerTests/PlexCleanerTests.csproj`) + - xUnit v3 test suite. Assertions via AwesomeAssertions. +- **`Docker/`** - multi-arch Linux container build (`ubuntu:rolling`, `linux/amd64` + `linux/arm64`); runs as a `nonroot` user, mounts media under `/media`. +- **Build configuration**: + - Common MSBuild properties (`TargetFramework`, `Nullable`, `ImplicitUsings`, `AnalysisLevel`, etc.) live in `Directory.Build.props` at the solution root. Do not duplicate these in individual `.csproj` files - only add a property to a `.csproj` when it is project-specific or overrides the shared default. + - All NuGet package versions are centralised in `Directory.Packages.props`. `PackageReference` elements in `.csproj` files must not include a `Version` attribute. Asset metadata (`PrivateAssets`, `IncludeAssets`) stays in the `.csproj` `PackageReference` element. +- **Style guide / further reading**: [`CODESTYLE.md`](./CODESTYLE.md) for C# code conventions; [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) for the Copilot review runbook; and [`ARCHITECTURE.md`](./ARCHITECTURE.md) for the architecture, processing pipeline, and design patterns - read it before changing processing, sidecar, media-tool, or language-tag code. diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md new file mode 100644 index 00000000..083170ea --- /dev/null +++ b/ARCHITECTURE.md @@ -0,0 +1,444 @@ +# PlexCleaner Architecture + +This document describes the architecture, processing pipeline, and design patterns of PlexCleaner for contributors and agents working on the codebase. Cross-cutting agent and workflow governance lives in [AGENTS.md](./AGENTS.md), C# code style in [CODESTYLE.md](./CODESTYLE.md), and GitHub Copilot review mechanics in [.github/copilot-instructions.md](./.github/copilot-instructions.md). + +## Project Overview + +PlexCleaner is a .NET 10.0 CLI utility that optimizes media files for Direct Play in Plex/Emby/Jellyfin by: + +- Converting containers to MKV format +- Re-encoding incompatible video/audio codecs +- Managing tracks (language tags, duplicates, subtitles) +- Verifying and repairing media integrity +- Removing closed captions and unwanted content +- Monitoring folders for changes and automatically processing new/modified files + +The tool orchestrates external media processing tools (FFmpeg, HandBrake, MkvToolNix, MediaInfo, 7-Zip) via CLI wrappers. + +## Documentation + +User-facing documentation is organized as follows: + +- **[README.md](./README.md)**: Main project documentation, quick start, installation, usage, and FAQ. +- **[Docs/LanguageMatching.md](./Docs/LanguageMatching.md)**: Technical details on IETF/RFC 5646 language tag matching and configuration. +- **[Docs/CustomOptions.md](./Docs/CustomOptions.md)**: FFmpeg and HandBrake custom encoding parameters, hardware acceleration setup, and encoder options. +- **[Docs/ClosedCaptions.md](./Docs/ClosedCaptions.md)**: Detailed technical analysis of EIA-608/CTA-708 closed caption detection methods and tools. +- **[HISTORY.md](./HISTORY.md)**: Release notes and version history. + +## Architecture + +### Command Structure + +PlexCleaner provides multiple commands: + +- **process**: Batch process media files in specified folders +- **monitor**: Watch folders for changes and automatically process modified files +- **verify**: Verify media files using FFmpeg +- **remux**: Re-multiplex media files to MKV +- **reencode**: Re-encode media tracks using HandBrake or FFmpeg +- **deinterlace**: De-interlace media files +- **createsidecar**: Create sidecar files for existing media +- **gettoolinfo**: Display tool version information +- **gettagmap**: Analyze language tags across media files +- **getmediainfo**: Extract and display media properties +- **checkfornewtools**: Check for and download tool updates (Windows only) +- **defaultsettings**: Create default configuration file +- **createschema**: Generate JSON schema for configuration validation +- **removesubtitles**: Remove all subtitle tracks +- **removeclosedcaptions**: Remove embedded EIA-608/CTA-708 closed captions from video streams +- **updatesidecar**: Create or update sidecar files to current schema/tool info +- **getsidecarinfo**: Display sidecar file information +- **testmediainfo**: Test parsing media tool information for non-Matroska containers +- **getversioninfo**: Print application and media tool version information + +### Fluent Builder Pattern for Media Tools + +All media tool command-line construction uses fluent builders (`*Builder.cs`). Never concatenate strings: + +```csharp +// Correct - fluent builder pattern +FfMpeg.GlobalOptions command = new FfMpeg.GlobalOptions(args) + .Default() + .Add(customOption); + +// Wrong - string concatenation +string rawArguments = "-hide_banner " + option; +``` + +### Process Execution with CliWrap + +All external process execution uses [CliWrap](https://github.com/Tyrrrz/CliWrap) (v3.x): + +- Builders create `ArgumentsBuilder` instances +- Execute via `Cli.Wrap(toolPath).WithArguments(builder)` +- Use `BufferedCommandResult` for output capture +- See `MediaTool.cs` for base execution patterns +- All tool execution supports cancellation via `Program.CancelToken()` + +### Sidecar File System + +Critical performance feature - DO NOT break compatibility: + +- Each `.mkv` gets a `.PlexCleaner` sidecar JSON file +- Contains: processing state, tool versions, media properties, file hash +- Hash: First 64KB + last 64KB of file (not timestamp-based) +- Schema versioned (`SchemaVersion: 5` in `SidecarFileJsonSchema5`, global alias in `GlobalUsing.cs`) +- Processing skips verified files unless sidecar invalidated +- State flags are bitwise: `StatesType` enum with `[Flags]` attribute +- Sidecar operations: `Create()`, `Read()`, `Update()`, `Delete()` + +### Media Tool Abstraction + +- `MediaTool` base class defines tool lifecycle +- Each tool family has: Tool class, Builder class, Info schema +- Tool version info retrieved from CLI output, cached in `Tools.json` +- Windows supports auto-download via `GitHubRelease.cs`; Linux uses system tools +- Tool paths: `ToolsOptions.UseSystem` or `RootPath + ToolFamily/SubFolder/ToolName` +- Tool execution: Base `Execute()` method with cancellation, logging, and error handling +- Version checking: `GetInstalledVersion()`, `GetLatestVersion()` (Windows only) + +### Media Properties and Track Management + +**MediaProps hierarchy:** + +- `MediaProps`: Container for all media information (video, audio, subtitle tracks) +- `TrackProps`: Base class for all track types + - `VideoProps`: Video track properties (format, resolution, codec, HDR, interlacing) + - `AudioProps`: Audio track properties (format, channels, sample rate, codec) + - `SubtitleProps`: Subtitle track properties (format, codec, closed captions) + +**Track properties:** + +- Language tags: ISO 639-2B (`Language`) and RFC 5646/BCP 47 (`LanguageIetf`) +- Flags: Default, Forced, HearingImpaired, VisualImpaired, Descriptions, Original, Commentary +- State: Keep, Remove, ReMux, ReEncode, DeInterlace, SetFlags, SetLanguage, Unsupported +- Title, Format, Codec, Id, Number, Uid + +**Track selection (`SelectMediaProps.cs`):** + +- Separates tracks into Selected/NotSelected categories +- Used for language filtering, duplicate removal, codec selection +- Move operations: `Move(track, toSelected)`, `Move(trackList, toSelected)` +- State assignment: `SetState(selectedState, notSelectedState)` + +### Language Tag Management + +**IETF/RFC 5646 Support:** + +- Uses external package `ptr727.LanguageTags` for language tag parsing and matching +- Tag format: `language-extlang-script-region-variant-extension-privateuse` +- Matching: Left-to-right prefix matching via `LanguageLookup.IsMatch()` +- Conversion: ISO 639-2B <-> RFC 5646 via `GetIsoFromIetf()`, `GetIetfFromIso()` +- Special tags: `und` (undefined), `zxx` (no linguistic content), `en` (English) + +**Language processing:** + +- MediaInfo reports both ISO 639-2B and IETF tags (if set) +- MkvMerge normalizes to IETF tags when `SetIetfLanguageTags` enabled +- FFprobe uses tag metadata which may differ from track metadata +- Track validation: Checks ISO/IETF consistency, sets error states for mismatches + +### Monitor Mode + +**File system watching:** + +- Uses `FileSystemWatcher` to monitor specified folders +- Monitors: Size, CreationTime, LastWrite, FileName, DirectoryName +- Handles: Changed, Created, Deleted, Renamed events +- Queue-based: Changes added to watch queue with timestamps + +**Processing logic:** + +- Files must "settle" (no changes for `MonitorWaitTime` seconds) before processing +- Files must be readable (not being written) before processing +- Retry logic: `FileRetryCount` attempts with `FileRetryWaitTime` delays +- Cleanup: Deletes empty folders after file removal +- Pre-process: Optional initial scan of all monitored folders on startup + +**Concurrency:** + +- Lock-based queue management (`_watchLock`) +- Periodic processing (1-second poll interval) +- Supports parallel processing when `--parallel` enabled + +### XML and JSON Parsing + +AOT-safe parsers in `MediaInfoXmlParser.cs`: + +- **MediaInfoFromXml()**: Parses specific MediaInfo XML elements into `MediaInfoToolXmlSchema.MediaInfo` + - Manually parses only known elements needed by PlexCleaner (id, format, language, etc.) + - Used by sidecar file system to parse XML output when JSON unavailable + - Avoids XmlSerializer (not AOT-compatible) +- **GenericXmlToJson()**: Converts any XML file to JSON format + - Preserves all elements and attributes (unlike MediaInfoFromXml's selective parsing) + - Handles attributes: prefix with `@` for elements with children, no prefix for leaf elements + - Detects arrays: elements appearing multiple times become JSON arrays + - Two-pass algorithm: collect children to detect arrays, then write JSON + - Uses `XmlReader` and `Utf8JsonWriter` for streaming efficiency + - Special handling for MediaInfo's mixed attribute/text content format (creatingLibrary) +- **MediaInfoXmlToJson()**: Converts parsed MediaInfo XML to MediaInfo JSON schema + - Bridges between XML and JSON schema types + - Maps only known MediaInfo track properties + +Parser design patterns: + +- Forward-only `XmlReader` with depth tracking for streaming +- Recursive `ElementData` tree for generic XML-to-JSON conversion +- Namespace filtering (skip `xmlns`, `xsi` attributes) +- Special handling for MediaInfo's mixed attribute/text content format + +### Extensions Pattern + +**Modern C# 13 extension syntax:** + +- Uses implicit class extensions: `extension(ILogger logger)` +- Provides context-aware helper methods +- Examples: + - `LogAndPropagate()`: Log exception and return false (propagates error) + - `LogAndHandle()`: Log exception and return true (handles error for catch clauses) + - `LogOverrideContext()`: Create scoped logger with LogOverride context + +## Code Conventions + +For formatter, EditorConfig, pre-commit hooks, line endings, and charset details, see [CODESTYLE.md](./CODESTYLE.md). + +### Code Style + +- Target: .NET 10.0 (`net10.0`) +- AOT compilation enabled: `true` in executable projects +- Use C# modern features (records, pattern matching, collection expressions, implicit class extensions) +- Prefer `Debug.Assert()` for internal invariants +- Logging: Serilog with thread IDs (`Log.Information/Warning/Error`) +- Exception handling: uses broad `catch(Exception)` blocks at boundary points +- Global usings: `GlobalUsing.cs` defines project-wide type aliases (`ConfigFileJsonSchema`, `SidecarFileJsonSchema`) +- `Directory.Build.props`: Common MSBuild properties (`TargetFramework`, `Nullable`, `ImplicitUsings`, `AnalysisLevel`, etc.) shared across all projects live here at the solution root. Do not duplicate these in individual `.csproj` files -- only add a property to a `.csproj` when it is project-specific or overrides the shared default. +- `Directory.Packages.props`: All NuGet package versions are centralised here via `PackageVersion` items. `PackageReference` elements in `.csproj` files must not include a `Version` attribute. Asset metadata (`PrivateAssets`, `IncludeAssets`) stays in the `.csproj` `PackageReference` element. + +### Naming and Structure + +- JSON schemas: Generated via `JsonSchema.Net`, suffixed with version (e.g., `SidecarFileJsonSchema5`) +- Builder methods: Return `this` for chaining +- Media props: `*Props.cs` classes (VideoProps, AudioProps, SubtitleProps, TrackProps) +- Options classes: `*Options.cs` for command categories (ProcessOptions, VerifyOptions, ConvertOptions, ToolsOptions) +- Partial classes: Tool families use partial class structure (`*Tool.cs`, `*Builder.cs`) + +### Async and Concurrency + +- Main loop: Uses `WaitForCancel()` polling pattern instead of async/await +- Tool execution: Synchronous wrappers around CliWrap async operations +- Parallel processing: PLINQ with `AsParallel()`, `WithDegreeOfParallelism()` +- Lock-based synchronization: `Lock` instances for collection access +- Cancellation: Global `CancellationTokenSource` accessed via `Program.CancelToken()` + +## Common Patterns + +### Command-Line Parsing + +Uses `System.CommandLine` (v2.x): + +- Options defined in `CommandLineOptions.cs` +- Binding via `CommandLineParser.Bind()` +- No `System.CommandLine.NamingConventionBinder` (deprecated) +- Recursive options: Available to all subcommands (`--logfile`, `--logwarning`, `--debug`) +- Command routing: Each command maps to static method in `Program.cs` + +### Parallel Processing + +- `--parallel` flag enables concurrent file processing +- Uses `ProcessDriver.cs` with `AsParallel()` and `WithDegreeOfParallelism()` +- Default thread count: min(CPU/2, 4), configurable via `--threadcount` +- Lock-based collection updates in parallel contexts +- File grouping: Groups by path (excluding extension) to prevent concurrent access to same file + +### File Processing States + +```csharp +[Flags] +enum StatesType { + None, SetLanguage, ReMuxed, ReEncoded, DeInterlaced, + Repaired, RepairFailed, Verified, VerifyFailed, + BitrateExceeded, ClearedTags, FileReNamed, FileDeleted, + FileModified, ClearedCaptions, RemovedAttachments, + SetFlags, RemovedCoverArt +} +``` + +Check states with `HasFlag()`, combine with `|=` + +### Configuration Schema + +- Settings: `PlexCleaner.defaults.json` with inline JSONC comments +- Schema: `PlexCleaner.schema.json` (auto-generated via JsonSchema.Net) +- Validation: JSON Schema.Net with source-generated context +- URL schema reference: `https://raw.githubusercontent.com/ptr727/PlexCleaner/main/PlexCleaner.schema.json` +- Versioned: ConfigFile schemas numbered (ConfigFileJsonSchema4, etc.) +- Defaults: `SetDefaults()` method in each options class +- Verification: `VerifyValues()` method validates configuration + +### Keep-Awake Pattern + +- Prevents system sleep during long operations +- Uses `KeepAwake.cs` with Windows API calls +- Timer-based: Refreshes every 30 seconds +- Cross-platform: No-op on non-Windows systems + +### Cancellation Handling + +- Global token source: `Program.s_cancelSource` +- Console handlers: Ctrl+C, Ctrl+Z, Ctrl+Q +- Keyboard monitoring: Separate task for key press handling +- Tool execution: All CliWrap calls use `Program.CancelToken()` +- Graceful cleanup: Logs cancellation messages, disables file watchers + +## Testing + +### Test Framework + +- xUnit v3.x with `AwesomeAssertions` +- Test project: `PlexCleanerTests/` +- Fixture: `PlexCleanerFixture` (assembly-level, sets up defaults and logging) +- Sample media: `Samples/PlexCleaner/` (relative path `../../../../Samples/PlexCleaner`) + +### Test Coverage + +- Command-line parsing: `CommandLineTests.cs` +- Configuration validation: `ConfigFileTests.cs` +- FFmpeg parsing: `FfMpegIdetParsingTests.cs` +- Sidecar functionality: `SidecarFileTests.cs` +- Version parsing: `VersionParsingTests.cs` +- Wildcards: `WildcardTests.cs` +- Filename escaping for filters: `FileNameEscapingTests.cs` + +### Test Execution + +- Task: `.Net Build` (VS Code task) for builds +- Unit tests: `dotnet test` or VS Code test explorer +- Docker tests: Download Matroska test files from GitHub +- CI: Separate workflows for build tests and Docker tests + +## Build and Release + +The authoritative release and workflow governance is in [AGENTS.md](./AGENTS.md). This section is a short architectural summary. + +### Local Development + +```bash +# Build +dotnet build + +# Format code (canonical CSharpier Format task invocation) +dotnet csharpier format --log-level=debug . + +# Verify formatting +dotnet format style --verify-no-changes --severity=info --verbosity=detailed + +# Run tests +dotnet test + +# Pre-commit validation (automatic via Husky) +dotnet husky run +``` + +### GitHub Actions + +Two-phase model - reusable `*-task.yml` workflows orchestrated by two entry points: + +- **test-pull-request.yml**: PR validation. `changes` (dorny/paths-filter) -> always-on `unit-test` (Husky) + path-gated `smoke-build` (reduced, no-push) -> `Check pull request workflow status` aggregator (ruleset-bound name; requires `changes` succeeded). +- **publish-release.yml**: the **sole publisher** (`push` + weekly `schedule` + `workflow_dispatch`). A `setup` job computes the branch list + publish gate; the `publish` matrix builds both branches via `build-release-task.yml` (executable 7-RID matrix + multi-arch Docker `linux/amd64,linux/arm64` + GitHub release), then `tool-versions`, `docker-readme` (main only), `date-badge` (main only). +- Reusable tasks: `build-release-task.yml`, `build-executable-task.yml`, `build-docker-task.yml`, `build-toolversions-task.yml`, `publish-docker-readme-task.yml`, `build-datebadge-task.yml`, `get-version-task.yml`. All thread a required `branch` input (config keys off it, never `github.ref_name`) plus `ref`/`smoke`. +- Version info: `version.json` with Nerdbank.GitVersioning format. `get-version-task.yml` surfaces `SemVer2`, the assembly versions, and `GitCommitId` (used to pin the release `target_commitish`). +- Branches: `main` (stable releases, `latest`), `develop` (pre-releases, `develop`). + +### Docker + +- Multi-stage builds in `Docker/Dockerfile` +- Base image: `ubuntu:rolling` only (no longer publishing Alpine or Debian variants) +- Supported architectures: `linux/amd64`, `linux/arm64` (no longer supporting `linux/arm/v7`) +- Tool installation: Ubuntu package manager (apt) +- Media tool versions match Windows versions for consistent behavior +- Test script: `Docker/Test.sh` validates all commands +- Version extraction: `Docker/Version.sh` captures tool versions for README +- User: Runs as `nonroot` user in containers +- Volumes: `/media` for media files and configuration + +## Critical Details + +### DO NOT + +- Break sidecar file compatibility (versioned schema migrations only) +- Use string concatenation for command-line arguments (use builders) +- Modify file timestamps unless `RestoreFileTimestamp` enabled +- Execute media tools without CliWrap abstractions +- Add synchronous operations in parallel processing paths +- Use `XmlSerializer` for AOT compilation (not compatible) +- Break language tag matching logic (IETF/ISO conversion) + +### DO + +- Add tests for media tool parsing changes (see `FfMpegIdetParsingTests.cs`) +- Update `HISTORY.md` for notable changes +- Use `Program.CancelToken()` for cancellation support +- Log with context: filenames, state transitions, tool versions +- Handle cross-platform paths (`Path.Combine`, forward slashes in Docker) +- Use modern C# features (collection expressions, pattern matching, extensions) +- Version schemas when making breaking changes +- Update global using aliases in `GlobalUsing.cs` when changing schema versions + +### Performance Considerations + +- Sidecar files enable fast re-processing (skip verified files) +- `--parallel` most effective with I/O-bound operations (re-mux) +- `--quickscan` limits scan to 3 minutes (trades accuracy for speed) +- `--testsnippets` creates 30s clips for testing +- Docker logging can grow large - configure rotation externally +- Monitor mode: Settle time prevents excessive re-processing + +### Special Cases + +**Closed Captions:** + +- EIA-608/EIA-708 tracks handled specially in `SubtitleProps.HandleClosedCaptions()` +- Parsed as subtitle tracks but removed during processing +- Track IDs formatted as `{VideoId}-CC{Number}` (e.g., `256-CC1`) + +**VOBSUB Subtitles:** + +- Require `MuxingMode` to be set for Plex compatibility +- Missing `MuxingMode` triggers error and removal recommendation + +**Duplicate Tracks:** + +- Language-based grouping with flag preservation +- Preferred audio codec selection via `FindPreferredAudio()` +- Keeps one flagged track per flag type, one non-flagged track + +**Language Mismatches:** + +- ISO 639-2B vs IETF tag validation in `TrackProps.SetLanguage()` +- Tag metadata vs track metadata differences (FFprobe specific) +- Automatic fallback: At least one track kept even if language doesn't match + +## Key Files Reference + +- **Program.cs**: Entry point, command routing, global state, cancellation handling +- **ProcessDriver.cs**: File enumeration, parallel processing orchestration +- **ProcessFile.cs**: Single-file processing logic, track selection algorithms +- **Process.cs**: High-level processing workflow, empty folder deletion +- **SidecarFile.cs**: Sidecar creation, validation, state management, hashing +- **MediaTool.cs**: Base class for tool abstractions, execution patterns +- **MediaProps.cs**: Media container, track aggregation +- **TrackProps.cs**: Base track properties, language handling, flag management +- **VideoProps.cs / AudioProps.cs / SubtitleProps.cs**: Track-specific properties +- **MediaInfoXmlParser.cs**: AOT-safe XML/JSON parsing (MediaInfo output) +- **Monitor.cs**: File system watching, change queue management +- **Convert.cs**: Re-encoding and re-muxing orchestration +- **MkvProcess.cs**: MKV-specific operations (attachment removal, flag setting) +- **Tools.cs**: Tool instances, version verification, update checking +- **Language.cs**: IETF tag matching, language list extraction +- **SelectMediaProps.cs**: Track filtering and selection logic +- **CommandLineOptions.cs**: CLI parsing, option definitions +- **Extensions.cs**: Logger extensions, implicit class extensions +- **GlobalUsing.cs**: Global type aliases for schema versions +- **KeepAwake.cs**: System sleep prevention +- **PlexCleaner.defaults.json**: Canonical configuration reference +- **.editorconfig** / **.csharpier.json**: Code style definitions diff --git a/CODESTYLE.md b/CODESTYLE.md index 96cd1da8..7385c8e1 100644 --- a/CODESTYLE.md +++ b/CODESTYLE.md @@ -1,314 +1,353 @@ -# Code Style and Formatting Rules - -## Build Requirements - -### Zero Warnings Policy - -**CRITICAL**: All builds must complete without warnings. The project enforces this through: - -1. **VS Code tasks** - - `CSharpier Format` → `.Net Build` → `.Net Format` - - `.Net Format` must pass with `--verify-no-changes` before commit - - Command: `dotnet format style --verify-no-changes --severity=info --verbosity=detailed` - -2. **Analyzer configuration** - - `latest-all` - - `true` - - Analyzer severity is `suggestion`, but all warnings must be addressed - -3. **Husky.Net pre-commit hooks** - - Automated checks run before commits - -### Build Tasks - -Available VS Code tasks (use via `run_task` tool): - -- `.Net Build`: Build with diagnostic verbosity -- `.Net Format`: Verify formatting and style (must pass) -- `CSharpier Format`: Auto-format code with CSharpier -- `.Net Tool Update`: Update dotnet tools -- `.Net Outdated Upgrade`: Upgrade outdated NuGet dependencies (interactive prompt) -- `Husky.Net Run`: Run pre-commit hooks manually - -## Tooling and Editor - -### Code Formatting and Tooling - -1. **CSharpier**: Primary code formatter - - Run before committing: `dotnet csharpier format --log-level=debug .` - -2. **dotnet format**: Style verification - - Verify no changes: `dotnet format style --verify-no-changes --severity=info --verbosity=detailed` - -3. **Husky.Net**: Git hooks for automated checks - - Installed as a local dotnet tool (via `dotnet tool restore`) - - Install Git hooks locally with `dotnet husky install` - - Pre-commit hooks run formatting and style checks - -4. **Other tools** - - `dotnet-outdated-tool`: Dependency update checks - - Nerdbank.GitVersioning: Version management - -### Editor Baseline - -1. **Required VS Code extensions**: CSharpier, markdownlint, CSpell -2. **VS Code settings**: Use the workspace settings without overrides - -### Markdown Files - -1. **Linting**: All `.md` files must be linted with the VS Code `markdownlint` extension (local only; no CI) -2. **Zero warnings**: Markdown linting must be error and warning free - -### Spelling - -1. **CSpell**: All spelling checks must be error free using the CSpell VS Code integration -2. **Accepted spellings**: Words must be correctly spelled in US or UK English -3. **Allowed exceptions**: Project-specific terms must be added to the workspace CSpell config - -## Coding Standards and Conventions - -Note: Code snippets are illustrative examples only. Replace namespaces/types to match your project. - -### C# Language Features - -1. **File-scoped namespaces** - - ```csharp - namespace PlexCleaner; - ``` - -2. **Nullable reference types**: Enabled (`enable`) - - Use nullable annotations appropriately - - Use `required` for mandatory properties - -3. **Modern C# features**: Prefer modern language constructs - - Primary constructors when appropriate - - Top-level statements for console apps - - Pattern matching over traditional checks - - Collection expressions when types loosely match - - Extension methods using `extension()` syntax - - Implicit object creation when type is apparent - - Range and index operators - -4. **Expression-bodied members**: Use for applicable members - - Methods, properties, accessors, operators, lambdas, local functions - -5. **`var` keyword**: Do NOT use `var` (always use explicit types) - - ```csharp - // Correct - int count = 42; - string name = "test"; - - // Incorrect - var count = 42; - var name = "test"; - ``` - -### Naming Conventions - -1. **Private fields**: underscore prefix with camelCase - - ```csharp - private readonly HttpClient _httpClient; - private int _counter; - ``` - -2. **Static fields**: `s_` prefix with camelCase - - ```csharp - private static int s_instanceCount; - ``` - -3. **Constants**: PascalCase - - ```csharp - private const int MaxRetries = 3; - ``` - -### Code Structure - -1. **Global usings**: Use `GlobalUsings.cs` for common namespaces - - ```csharp - global using System; - global using System.Net.Http; - global using System.Threading.Tasks; - global using Serilog; - ``` - -2. **Usings placement**: Outside namespace, sorted with `System` directives first - - ```csharp - using System.CommandLine; - using System.Runtime.CompilerServices; - using PlexCleaner; - - namespace PlexCleaner; - ``` - -3. **Braces**: Allman style - - ```csharp - public void Method() - { - if (condition) - { - // code - } - } - ``` - -4. **Indentation** - - C# files: 4 spaces - - XML/csproj files: 2 spaces - - YAML files: 2 spaces - - JSON files: 4 spaces - -5. **Line endings** - - C#, XML, YAML, JSON, Windows scripts: CRLF - - Linux scripts (`.sh`): LF - -6. **`#region`**: Do not use regions. Prefer logical file/folder/namespace organization. -7. **Member ordering (StyleCop SA1201)**: const → static readonly → static fields → instance readonly fields → instance fields → constructors → public (events → properties → indexers → methods → operators) → non-public in same order → nested types - -### Comments and Documentation - -1. **XML documentation** - - `true` - - Missing XML comments for public APIs are suppressed (`.editorconfig`) - - Must document all public surfaces. - - Single-line summaries, additional details in remarks, document input parameters, returns values, exceptions, and add crefs - - ```csharp - /// - /// Example of a single line summary. - /// - /// - /// Additional important details about usage. - /// Multiple lines if needed. - /// - /// - /// The quote category to request - /// - /// - /// A that can be used to cancel the request. - /// - /// - /// A containing the quote text. - /// - /// - /// Thrown when is not a supported value. - /// - public async Task GetQuoteOfTheDayAsync(string category, CancellationToken cancellationToken) {} - ``` - -2. **Code analysis suppressions** - - Do not use `#pragma` sections to disable analyzers - - For one-off cases, use suppression attributes with justifications - - For project-wide suppressions, add rules to `.editorconfig` - - ```csharp - [System.Diagnostics.CodeAnalysis.SuppressMessage( - "Design", - "CA1034:Nested types should not be visible", - Justification = "https://github.com/dotnet/sdk/issues/51681" - )] - ``` - -### Error Handling and Logging - -1. **Serilog logging**: Use structured logging - - ```csharp - logger.Error(exception, "{Function}", function); - ``` - -2. **Library log configuration**: Libraries must expose logging configuration - - Provide options or settings to supply an `ILoggerFactory` and/or `ILogger` - - Offer a global fallback logger for static usage when needed - -3. **CallerMemberName**: Use for automatic function name tracking - - ```csharp - public bool LogAndPropagate( - Exception exception, - [CallerMemberName] string function = "unknown" - ) - ``` - -4. **Logger extensions**: Use `Extensions.cs` for logger and other extension methods - - ```csharp - extension(ILogger logger) - { - public bool LogAndPropagate(Exception exception, ...) { } - } - ``` - -5. **Exceptions**: Do not swallow exceptions; log and rethrow or translate to a domain-specific exception - -### Code Patterns - -1. **Guard clauses**: Prefer early returns for validation and error handling -2. **Async all the way**: Avoid blocking calls (`.Result`, `.Wait()`); use `async`/`await` -3. **Cancellation tokens**: Accept `CancellationToken` as the last parameter and pass it through -4. **ConfigureAwait**: In library code, use `ConfigureAwait(false)` unless context is required - - Do not call `ConfigureAwait(false)` in xUnit tests (see xUnit1030) -5. **Disposables**: Use `await using` for async disposables; prefer `using` declarations -6. **LINQ vs loops**: Use LINQ for clarity, loops for hot paths or allocations -7. **HTTP**: Reuse `HttpClient` via factory; avoid per-request instantiation -8. **Collections**: Prefer `IReadOnlyList`/`IReadOnlyCollection` for public APIs -9. **Immutability**: Prefer immutable records; use init-only setters when records are not suitable; prefer immutable or frozen collections for read-only data -10. **Exceptions as control flow**: Avoid using exceptions for expected flow -11. **Sealing classes**: Seal classes that are not designed for inheritance -12. **Read-only data**: Use immutable or frozen collections for read-only data sets -13. **Lazy initialization**: Use `Lazy` for static, thread-safe instantiation (e.g., logger factory, HTTP factory) - -### Testing Conventions - -1. **Framework**: xUnit with AwesomeAssertions - - ```csharp - [Fact] - public void MethodName_Scenario_ExpectedBehavior() - { - // Arrange - int expected = 42; - - // Act - int actual = GetValue(); - - // Assert - actual.Should().Be(expected); - } - ``` - -2. **Organization**: Arrange-Act-Assert pattern -3. **Naming**: Descriptive names with underscores -4. **Theory tests**: Use `[Theory]` with `[InlineData]` - -## Project Configuration - -1. **Target framework**: .NET 10.0 (`net10.0`) - -2. **AOT compatibility** - - `true` - - `true` - -3. **Assembly information** - - Use semantic versioning - - Include SourceLink: `true` - - Embed untracked sources: `true` - -4. **Internal visibility**: Use `InternalsVisibleTo` for test access - - ```xml - - - - ``` - -## Best Practices - -1. **Code reviews**: All changes go through pull requests +# Code Style and Formatting Rules + +This is the single code-style guide for the repo. The **General** section applies to every language and is always carried. Each **language section** (.NET) is self-contained and **droppable**: a repo with no .NET side drops the .NET section - the same per-language model as [`.editorconfig`](./.editorconfig), whose `[*.cs]` block a non-.NET repo drops. + +Cross-cutting *process* rules (PR titles, branching, US English, markdown style, comments philosophy, workflow YAML, PR review etiquette) live in [AGENTS.md](./AGENTS.md) and are not repeated here. + +This file lives at the repo root. [AGENTS.md](./AGENTS.md) links it as `./CODESTYLE.md`; [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) links it as `../CODESTYLE.md`. + +## General + +These rules apply to every language in the repo. + +### Tooling Names and Casing + +Use each tool's official casing in docs and prose - `.NET`, `CSharpier`, `Husky.Net`. Don't invent personal variants. (Note: this repo's VS Code task labels use `.Net` casing for historical reasons - the labels are referenced verbatim below, but prose should use `.NET`.) + +### Clean-Compile Verification + +Each language defines a **clean-compile** verification - the combination of build, formatter, linter, and code-analysis tools that must report clean before a commit. It is exposed as one or more **named** VS Code tasks (or, where a language ships no tasks, documented commands), and those definitions are **carried verbatim** across derived repos. The concrete names live in each language section below. + +- **Run it after every code change.** The relevant language's clean-compile must pass before you commit; CI runs the same checks as a backstop. +- **The named task definition is the canonical spec** - its exact command sequence, arguments, and strictness. You may run it through the VS Code task **or** by invoking the equivalent native commands directly; either is fine **only if the sequence, arguments, and strictness match exactly**. No shortcuts and no more-lenient options (for example, never drop `--verify-no-changes` or loosen a `--severity`). + +### Analyzer Diagnostics and Suppressions + +- **A new port is not a license to silence diagnostics.** Brownfield / just-ported status never justifies relaxing analyzer or linter severities or muting newly surfaced warnings - fix them. +- **Suppress only genuine false-positives or deliberate, documented exceptions**, always at the **narrowest scope that fits**, in this order of preference: + 1. An **in-code annotation on the specific symbol**, with a justification - the language's attribute/comment form, never a blanket pragma spanning a region. + 2. The **owning project's local config** when the exception is project-wide for one project (e.g. a test project's own `.editorconfig`). + 3. The **root / shared config** only when the suppression is genuinely applicable to **every** project in the repo. +- **Never blanket-relax a batch of rules project-wide** to get a port to build. The per-language mechanics (which attribute, which config key) are in each language section. + +### Markdown and Spelling + +These apply repo-wide, in every directory: + +1. **Markdown linting**: All `.md` files must be lint-clean (error and warning free) via the VS Code `markdownlint` extension. [`.markdownlint-cli2.jsonc`](./.markdownlint-cli2.jsonc) at the repo root is the single source of truth - the davidanson `markdownlint` extension and a command-line `markdownlint-cli2` run both read it, so the IDE and CLI stay in lock-step. Rules it deliberately disables (e.g. `MD013` line-length, `MD033` inline HTML) are **intentional** - do not "fix" them. Fix violations at the source rather than disabling rules. +2. **Spelling**: All spelling must be clean via the CSpell VS Code integration; words must be correctly spelled in **US English** (the repo-wide convention - see [AGENTS.md](./AGENTS.md)). Project-specific terms go in the workspace CSpell config. + +## .NET + +*This section applies only to the .NET side. A repo with no .NET projects drops the whole section - see [Adopting Without .NET](#adopting-without-net) at its end.* + +This is the style guide for the **.NET projects** in this repo. This repo ships [`PlexCleaner/`](./PlexCleaner/) (the CLI application, .NET 10, AOT) and [`PlexCleanerTests/`](./PlexCleanerTests/) (xUnit v3 + AwesomeAssertions). There is also a [`Docker/`](./Docker/) folder for the container build. + +Shared MSBuild properties and central package versions are centralized at the repo root: [`Directory.Build.props`](./Directory.Build.props) holds the shared MSBuild properties, and [`Directory.Packages.props`](./Directory.Packages.props) holds central package versions (so a `PackageReference` is declared without a `Version` attribute). + +### Build Requirements + +#### Zero Warnings Policy + +**CRITICAL**: All builds must complete without warnings. The project enforces this through: + +1. **The `.Net Format` clean-compile task** (see [Clean-Compile Verification](#clean-compile-verification)) + - The .NET clean-compile is the **`.Net Format`** VS Code task, which chains `CSharpier Format` -> `.Net Build` -> `dotnet format style --verify-no-changes`. These three task definitions are carried verbatim in [`.vscode/tasks.json`](./.vscode/tasks.json). + - After any code change it must pass before commit. Run the `.Net Format` task. To run it natively instead, reproduce that task chain from [`.vscode/tasks.json`](./.vscode/tasks.json) exactly - `CSharpier Format`, then `.Net Build`, then the `dotnet format style --verify-no-changes --severity=info ...` verify - without dropping or loosening any argument (tasks.json is the canonical command spec). Bare `dotnet format` alone, skipping CSharpier or the build, is not sufficient. + +2. **Analyzer configuration** + - `latest-all` + - `true` + - Analyzer severity is `suggestion`, but all warnings must be addressed - see [Analyzer Diagnostics and Suppressions](#analyzer-diagnostics-and-suppressions); do not relax rules to dodge them. + +3. **Husky.Net pre-commit hook** + - The pre-commit hook (`dotnet husky run`) runs the CSharpier and style checks before a commit lands. CI runs the same checks as a backstop. + +#### Build Tasks + +Available VS Code tasks (run them from VS Code's task runner - **Terminal -> Run Task** - or an agent's task-running tool). The first three are the clean-compile set, carried verbatim; the rest are convenience tasks: + +- `.Net Build`: Build with diagnostic verbosity *(clean-compile)* +- `CSharpier Format`: Auto-format code with CSharpier *(clean-compile)* +- `.Net Format`: Run CSharpier and build, then verify formatting and style with `--verify-no-changes` *(clean-compile; the task to run after edits)* +- `.Net Tool Update`: Update dotnet tools *(convenience)* +- `.Net Outdated Upgrade`: Upgrade outdated NuGet dependencies, interactive prompt *(convenience)* +- `Husky.Net Run`: Run the pre-commit hooks manually *(convenience)* + +### Tooling and Editor + +#### Code Formatting and Tooling + +1. **CSharpier**: Primary code formatter + - Invoked by the `CSharpier Format` task / `dotnet csharpier format --log-level=debug .` +2. **dotnet format**: Style verification + - Verify no changes: `dotnet format style --verify-no-changes --severity=info --verbosity=detailed` +3. **Husky.Net**: Git hooks for automated checks + - Installed as a local dotnet tool (via `dotnet tool restore`) + - Install Git hooks locally with `dotnet husky install` + - Pre-commit hooks run formatting and style checks (`dotnet husky run`) +4. **Other tools** + - `dotnet-outdated-tool`: Dependency update checks + - Nerdbank.GitVersioning: Version management + +#### Editor Baseline + +1. **Required VS Code extensions**: CSharpier, markdownlint, CSpell +2. **VS Code settings**: Use the workspace settings without overrides + +### Coding Standards and Conventions + +Note: Code snippets are illustrative examples only. Replace namespaces/types to match your project. + +#### C# Language Features + +1. **File-scoped namespaces** + + ```csharp + namespace PlexCleaner; + ``` + +2. **Nullable reference types**: Enabled (`enable`) + - Use nullable annotations appropriately + - Use `required` for mandatory properties + +3. **Modern C# features**: Prefer modern language constructs + - Primary constructors when appropriate + - Top-level statements for console apps + - Pattern matching over traditional checks + - Collection expressions when types loosely match + - Extension methods using `extension()` syntax + - Implicit object creation when type is apparent + - Range and index operators + +4. **Expression-bodied members**: Use for applicable members + - Methods, properties, accessors, operators, lambdas, local functions + +5. **`var` keyword**: Do NOT use `var` (always use explicit types) + + ```csharp + // Correct + int count = 42; + string name = "test"; + + // Incorrect + var count = 42; + var name = "test"; + ``` + +#### Naming Conventions + +1. **Private fields**: underscore prefix with camelCase + + ```csharp + private readonly HttpClient _httpClient; + private int _counter; + ``` + +2. **Static fields**: `s_` prefix with camelCase + + ```csharp + private static int s_instanceCount; + ``` + +3. **Constants**: PascalCase + + ```csharp + private const int MaxRetries = 3; + ``` + +#### Code Structure + +1. **Global usings**: Use `GlobalUsing.cs` for common namespaces + + ```csharp + global using System; + global using System.Net.Http; + global using System.Threading.Tasks; + global using Serilog; + ``` + +2. **Usings placement**: Outside namespace, sorted with `System` directives first + + ```csharp + using System.CommandLine; + using System.Runtime.CompilerServices; + using PlexCleaner; + + namespace PlexCleaner; + ``` + +3. **Braces**: Allman style + + ```csharp + public void Method() + { + if (condition) + { + // code + } + } + ``` + +4. **Indentation** + - C# files: 4 spaces + - XML/csproj files: 2 spaces + - YAML files: 2 spaces + - JSON files: 4 spaces + +5. **Line endings** + - C#, XML, YAML, JSON, Windows scripts: CRLF + - Linux scripts (`.sh`): LF + +6. **`#region`**: Do not use regions. Prefer logical file/folder/namespace organization. +7. **Member ordering (StyleCop SA1201)**: const -> static readonly -> static fields -> instance readonly fields -> instance fields -> constructors -> public (events -> properties -> indexers -> methods -> operators) -> non-public in same order -> nested types + +#### Comments and Documentation + +1. **XML documentation** + - `true` + - Missing XML comments for public APIs are suppressed (`.editorconfig`) + - Must document all public surfaces. + - Single-line summaries, additional details in remarks, document input parameters, returns values, exceptions, and add crefs + + ```csharp + /// + /// Example of a single line summary. + /// + /// + /// Additional important details about usage. + /// Multiple lines if needed. + /// + /// + /// The quote category to request + /// + /// + /// A that can be used to cancel the request. + /// + /// + /// A containing the quote text. + /// + /// + /// Thrown when is not a supported value. + /// + public async Task GetQuoteOfTheDayAsync(string category, CancellationToken cancellationToken) {} + ``` + +#### Analyzer Suppressions (.NET) + +Follow the scope hierarchy in [Analyzer Diagnostics and Suppressions](#analyzer-diagnostics-and-suppressions). .NET mechanics, narrowest first: + +- **Never use `#pragma warning disable`** to silence an analyzer. +- **Symbol-scoped**: a `[System.Diagnostics.CodeAnalysis.SuppressMessage(...)]` attribute with a `Justification`, on the specific member or type: + + ```csharp + [System.Diagnostics.CodeAnalysis.SuppressMessage( + "Design", + "CA1034:Nested types should not be visible", + Justification = "https://github.com/dotnet/sdk/issues/51681" + )] + ``` + +- **Project-scoped** (e.g. a test project): a `dotnet_diagnostic..severity` entry in *that project's own* `.editorconfig`, with a comment explaining why. +- **Repo-wide**: a `dotnet_diagnostic..severity` entry in the root `.editorconfig`, only when the rule is genuinely not applicable to any project. Relaxing a batch of `CA*` rules (or `dotnet_analyzer_diagnostic.severity`) to push a brownfield port through the build is exactly what this forbids. + +#### Error Handling and Logging + +1. **Serilog logging**: Use structured logging + + ```csharp + logger.Error(exception, "{Function}", function); + ``` + +2. **Library log configuration**: Libraries must expose logging configuration + - Provide options or settings to supply an `ILoggerFactory` and/or `ILogger` + - Offer a global fallback logger for static usage when needed + +3. **CallerMemberName**: Use for automatic function name tracking + + ```csharp + public bool LogAndPropagate( + Exception exception, + [CallerMemberName] string function = "unknown" + ) + ``` + +4. **Logger extensions**: Use `Extensions.cs` for logger and other extension methods + + ```csharp + extension(ILogger logger) + { + public bool LogAndPropagate(Exception exception, ...) { } + } + ``` + +5. **Exceptions**: Do not swallow exceptions; log and rethrow or translate to a domain-specific exception + +#### Code Patterns + +1. **Guard clauses**: Prefer early returns for validation and error handling +2. **Async all the way**: Avoid blocking calls (`.Result`, `.Wait()`); use `async`/`await` +3. **Cancellation tokens**: Accept `CancellationToken` as the last parameter and pass it through +4. **ConfigureAwait**: In library code, use `ConfigureAwait(false)` unless context is required + - Do not call `ConfigureAwait(false)` in xUnit tests (see xUnit1030) +5. **Disposables**: Use `await using` for async disposables; prefer `using` declarations +6. **LINQ vs loops**: Use LINQ for clarity, loops for hot paths or allocations +7. **HTTP**: Reuse `HttpClient` via factory; avoid per-request instantiation +8. **Collections**: Prefer `IReadOnlyList`/`IReadOnlyCollection` for public APIs +9. **Immutability**: Prefer immutable records; use init-only setters when records are not suitable; prefer immutable or frozen collections for read-only data +10. **Exceptions as control flow**: Avoid using exceptions for expected flow +11. **Sealing classes**: Seal classes that are not designed for inheritance +12. **Read-only data**: Use immutable or frozen collections for read-only data sets +13. **Lazy initialization**: Use `Lazy` for static, thread-safe instantiation (e.g., logger factory, HTTP factory) + +#### Testing Conventions + +1. **Framework**: xUnit with AwesomeAssertions + + ```csharp + [Fact] + public void MethodName_Scenario_ExpectedBehavior() + { + // Arrange + int expected = 42; + + // Act + int actual = GetValue(); + + // Assert + actual.Should().Be(expected); + } + ``` + +2. **Organization**: Arrange-Act-Assert pattern +3. **Naming**: Descriptive names with underscores +4. **Theory tests**: Use `[Theory]` with `[InlineData]` + +### Project Configuration + +1. **Target framework**: .NET 10.0 (`net10.0`) + +2. **AOT compatibility** + - `true` + - `true` + +3. **Assembly information** + - Use semantic versioning + - Include SourceLink: `true` + - Embed untracked sources: `true` + +4. **Internal visibility**: `PlexCleaner.csproj` exposes its internals to the `PlexCleanerTests` project only + + ```xml + + + + ``` + +### Best Practices + +1. **Code reviews**: All changes go through pull requests + +### Adopting Without .NET + +If a derived project has no .NET side, drop this entire `.NET` section and delete the .NET projects and their build/release wiring: the `[*.cs]` / ReSharper block in `.editorconfig`, the `.Net` task group in `.vscode/tasks.json`, and the `nuget` entries in `.github/dependabot.yml`.