diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index c33f5e45..59762746 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -18,6 +18,15 @@ Summarized for VS Code's generators; the full rules, rationale, and examples are - 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. +## Reviewing Carried Fleet Content + +Several of this repository's governance files are carried from a shared template and kept in sync across a fleet of sibling repositories - among them `AGENTS.md`, `CODESTYLE.md`, `WORKFLOW.md`, this file, and the `repo-config/` rulesets. Most of `AGENTS.md` is universal fleet law - every section that states a rule, as opposed to the two that describe this repository's own directory tree and devcontainer, is byte-locked and verified by an automated byte-for-byte match against the template canonical, not by line-by-line review. + +Two constraints follow when reviewing that content. + +- **A reference inside byte-locked text to a path or section this repository does not carry is intentional, not a broken link.** Universal rule text names shared infrastructure - a fleet registry, a reusable config snippet, the other workflow model's ruleset payload - that a given repository legitimately may not contain. Editing the text to "fix" such a reference would break the fleet audit that governs it, so the reference is correct as written. Do not report it as a dead link, a missing file, or a broken cross-reference. +- **A genuine substantive defect is still worth raising.** Byte-locked is not unreviewable. A self-contradiction, a factual error, or a real typo in the canonical prose is a valid finding - note that the fix lands at the template and re-vendors to every repository, rather than proposing a local edit the audit would reject. + ## GitHub Copilot Review Runbook > 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. @@ -153,4 +162,4 @@ After the final push, sweep-resolve stale older threads for removed code paths. 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) 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). +**Shared conventions:** if you find a discrepancy that looks like it should be fixed in the shared conventions rather than only locally (this file or AGENTS.md is out of date, a rule is missing, something bit this repo and would bite the next), raise it with the maintainer rather than only patching it here. diff --git a/.github/workflows/build-docker-task.yml b/.github/workflows/build-docker-task.yml index 87333117..58d4a074 100644 --- a/.github/workflows/build-docker-task.yml +++ b/.github/workflows/build-docker-task.yml @@ -53,7 +53,7 @@ jobs: steps: - name: Checkout step - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: ref: ${{ inputs.ref }} @@ -72,7 +72,7 @@ jobs: # Always login (even on smoke) for higher pull/cache-read rate limits; the credentials are in both the # Actions and Dependabot secret stores so a Dependabot push CI run can log in too. Forks cannot push here. - name: Login to Docker Hub step - uses: docker/login-action@af1e73f918a031802d376d3c8bbc3fe56130a9b0 # v4.4.0 + uses: docker/login-action@06fb636fac595d6fb4b28a5dfcb21a6f5091859c # v4.5.0 with: username: ${{ secrets.DOCKER_HUB_USERNAME }} password: ${{ secrets.DOCKER_HUB_ACCESS_TOKEN }} diff --git a/.github/workflows/build-executable-task.yml b/.github/workflows/build-executable-task.yml index 562f3812..f4604fc8 100644 --- a/.github/workflows/build-executable-task.yml +++ b/.github/workflows/build-executable-task.yml @@ -51,12 +51,13 @@ jobs: dotnet-version: 10.x - name: Checkout code step - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: ref: ${{ inputs.ref }} - name: Build executable project step run: | + set -Eeuo pipefail dotnet publish ./PlexCleaner/PlexCleaner.csproj \ --runtime ${{ matrix.runtime }} \ -property:PublishDir=${{ runner.temp }}/publish/${{ matrix.runtime }}/ \ diff --git a/.github/workflows/build-release-task.yml b/.github/workflows/build-release-task.yml index b4b6edf3..e51af5ea 100644 --- a/.github/workflows/build-release-task.yml +++ b/.github/workflows/build-release-task.yml @@ -1,7 +1,8 @@ name: Build project release task -# Orchestrate one branch's release: version once (get-version), build the executable 7z and the Docker image, then -# create the GitHub release. github/dockerhub gate the two publish targets; smoke builds everything, publishes nothing. +# Orchestrate one branch's release: version once (get-version), gate on branch<->version consistency +# (validate-release), build the executable 7z and the Docker image, then create the GitHub release. +# github/dockerhub gate the two publish targets; smoke builds everything, publishes nothing. on: workflow_call: inputs: @@ -27,6 +28,12 @@ on: required: false type: boolean default: false + # Set false for a repo that produces no release-asset-* files (e.g. Docker-only): the release is then just the + # tag + source zip + README + LICENSE; the artifact download is skipped and the unmatched-files guard relaxes. + expect_release_assets: + required: false + type: boolean + default: true jobs: @@ -48,11 +55,43 @@ jobs: with: ref: ${{ inputs.ref }} + # Entry gate: validate branch<->version consistency once, before the build jobs, so an NBGV mis-classification fails + # fast instead of after building and publishing. main must be a public release (no prerelease '-'); every other branch + # must carry a prerelease '-' (guards a develop leg being classified public and published as stable). Strip + # '+buildmetadata' first; a '-' there is legitimate, only a '-' in the core/prerelease segment marks a prerelease. + validate-release: + name: Validate release version job + needs: [get-version] + runs-on: ubuntu-latest + steps: + - name: Validate branch and version consistency step + env: + SEMVER2: ${{ needs.get-version.outputs.SemVer2 }} + BRANCH: ${{ inputs.branch }} + SMOKE: ${{ inputs.smoke }} + run: | + set -Eeuo pipefail + # Smoke builds never publish and always version as prerelease (detached PR HEAD), which would trip the main arm. + if [[ "$SMOKE" == "true" ]]; then + echo "Smoke build; skipping release version validation." + exit 0 + fi + CORE_AND_PRE="${SEMVER2%%+*}" + if [[ "$BRANCH" == "main" ]]; then + if [[ "$CORE_AND_PRE" == *-* ]]; then + echo "::error::Public (main) release version '$SEMVER2' carries a prerelease suffix; refusing to publish." + exit 1 + fi + elif [[ "$CORE_AND_PRE" != *-* ]]; then + echo "::error::Prerelease ($BRANCH) version '$SEMVER2' has no prerelease suffix (NBGV classified it public); refusing to publish." + exit 1 + fi + # Build only when validation passed (success) or was skipped (smoke); never when it failed. build-executable: name: Build executable job - needs: [get-version, validate] - if: ${{ !cancelled() && needs.get-version.result == 'success' && (needs.validate.result == 'success' || needs.validate.result == 'skipped') }} + needs: [get-version, validate, validate-release] + if: ${{ !cancelled() && needs.get-version.result == 'success' && needs.validate-release.result == 'success' && (needs.validate.result == 'success' || needs.validate.result == 'skipped') }} uses: ./.github/workflows/build-executable-task.yml secrets: inherit with: @@ -65,10 +104,15 @@ jobs: assembly_file_version: ${{ needs.get-version.outputs.AssemblyFileVersion }} assembly_informational_version: ${{ needs.get-version.outputs.AssemblyInformationalVersion }} + # Docker is the terminal registry push, so it must never push on a partial run. It needs every other build and + # guards with `!failure() && !cancelled()`: a *failed* build skips docker (no build, no push), while a *skipped* + # build - validate on a smoke run - does not, so docker still builds on smoke. Plain `needs` cannot express this + # (a skipped need skips the dependent). The github-release job reaches the same intent more simply because it + # only runs on a publish (`!inputs.smoke`), where nothing is skipped. build-docker: name: Build Docker job - needs: [get-version, validate] - if: ${{ !cancelled() && needs.get-version.result == 'success' && (needs.validate.result == 'success' || needs.validate.result == 'skipped') }} + needs: [get-version, validate, validate-release, build-executable] + if: ${{ !failure() && !cancelled() }} uses: ./.github/workflows/build-docker-task.yml secrets: inherit with: @@ -85,49 +129,42 @@ jobs: github-release: name: Publish GitHub release job - # !smoke enforces "smoke never publishes" even if a smoke caller set github: true. + # `!inputs.smoke` enforces "smoke never publishes" at the job level, so a smoke caller that also set + # `github: true` still can't create a release. if: ${{ inputs.github && !inputs.smoke }} runs-on: ubuntu-latest - needs: [get-version, build-executable, build-docker] + needs: [get-version, validate-release, build-executable, build-docker] steps: # Check out the exact built commit so the uploaded release files match the tag even if the branch advances mid-run. - name: Checkout code step - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: ref: ${{ needs.get-version.outputs.GitCommitId }} - # Backstop (main only): refuse to publish if the public version carries a prerelease '-' (NBGV mis-versioning). - # Strip '+buildmetadata' first; only a '-' in the core segment marks a prerelease. - - name: Verify public release version step - if: ${{ inputs.branch == 'main' }} - env: - SEMVER2: ${{ needs.get-version.outputs.SemVer2 }} - run: | - set -euo pipefail - CORE_AND_PRE="${SEMVER2%%+*}" # drop +buildmetadata; a '-' here is the genuine prerelease separator - if [[ "$CORE_AND_PRE" == *-* ]]; then - echo "::error::Public (main) release version '$SEMVER2' carries a prerelease suffix; refusing to publish." - exit 1 - fi - - # Collect the executable build's release-asset--* artifacts (the PlexCleaner.7z) by pattern. + # Collect assets by the `release-asset--*` pattern so this step is target-agnostic: subset releases by + # deleting the target, not `enable_*: false` (a skipped `needs` job would skip this release job too). The release + # step guards `fail_on_unmatched_files: true`, so at least one `release-asset-*` must match; a repo that drops + # every file-producing target (e.g. a Docker-only repo, whose release carries only source zip + README + LICENSE) + # relaxes that guard. - name: Download release asset artifacts step + if: ${{ inputs.expect_release_assets }} uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: pattern: release-asset-${{ inputs.branch }}-* merge-multiple: true path: ./Publish - # Weekly re-runs may hit an already-released version; skip release-create when the tag exists (no-op republish). + # The weekly publisher re-runs even with no new commits, so the version may already be released. Skip the release + # step when a release for this tag already exists to avoid a no-op republish. - name: Check for existing release step id: release-exists env: GH_TOKEN: ${{ github.token }} TAG: ${{ needs.get-version.outputs.SemVer2 }} run: | - set -euo pipefail + set -Eeuo pipefail if gh release view "$TAG" --repo "$GITHUB_REPOSITORY" >/dev/null 2>&1; then echo "exists=true" >> "$GITHUB_OUTPUT" if [[ "${{ github.event_name }}" == "workflow_dispatch" ]]; then @@ -139,8 +176,13 @@ jobs: echo "exists=false" >> "$GITHUB_OUTPUT" fi - # target_commitish pins the tag to the exact built commit (GitCommitId), not the moving branch ref. - # fail_on_unmatched_files catches a missing or misnamed PlexCleaner.7z. + # `target_commitish` must be set explicitly: otherwise GitHub's REST API tags the release on the default branch. + # Pin it to `GitCommitId` so the tag is on the exact built commit, consistent with the SemVer2 tag and artifacts. + # Skip when the release already exists, but always let a manual `workflow_dispatch` through to refresh it. + # Every release (any branch, any target) is a tag on the built commit plus the auto-attached source zip, README, + # and LICENSE; targets amend it by uploading `release-asset-*` files (binaries/packages) or pushing elsewhere + # (image/registry). `fail_on_unmatched_files: true` fails loudly if a promised `release-asset-*` is missing or + # misnamed; a no-file-target repo relaxes it (see download step). - name: Create GitHub release step if: ${{ steps.release-exists.outputs.exists == 'false' || github.event_name == 'workflow_dispatch' }} uses: softprops/action-gh-release@3d0d9888cb7fd7b750713d6e236d1fcb99157228 # v3.0.2 @@ -149,8 +191,33 @@ jobs: tag_name: ${{ needs.get-version.outputs.SemVer2 }} target_commitish: ${{ needs.get-version.outputs.GitCommitId }} prerelease: ${{ inputs.branch != 'main' }} - fail_on_unmatched_files: true + fail_on_unmatched_files: ${{ inputs.expect_release_assets }} files: | LICENSE README.md ./Publish/* + + # Surgical cleanup at the point of consumption: the release-asset--* transfer artifacts now have durable + # copies on the release, so delete them by exact pattern to free the storage quota - scoped to this branch's + # assets, leaving diagnostics and any other artifacts. Gated to the same condition as the create step so it only + # deletes when a release was actually created/refreshed this run; on a skipped create (existing tag, no new + # commits) the fresh artifacts stay for the run, reaped by the retention-days: 1 backstop. Needs the caller to + # grant `actions: write` (publish-release's publish job does). + - name: Delete consumed release asset artifacts step + if: ${{ inputs.expect_release_assets && (steps.release-exists.outputs.exists == 'false' || github.event_name == 'workflow_dispatch') }} + # Best-effort: the release is already published, so a listing/delete hiccup must never red the job; the + # retention-days: 1 backstop reaps anything missed. Deletes every matching id (a rerun can upload duplicates). + continue-on-error: true + env: + GH_TOKEN: ${{ github.token }} + run: | + set -Eeuo pipefail + if ! ids=$(gh api "repos/$GITHUB_REPOSITORY/actions/runs/${{ github.run_id }}/artifacts" --paginate \ + --jq ".artifacts[] | select(.name | startswith(\"release-asset-${{ inputs.branch }}-\")) | .id"); then + echo "::warning::Could not list run artifacts; retention-days backstop will reap them." + ids="" + fi + for id in $ids; do + gh api --method DELETE "repos/$GITHUB_REPOSITORY/actions/artifacts/$id" \ + || echo "::warning::Failed to delete artifact $id; retention-days backstop will reap it." + done diff --git a/.github/workflows/get-version-task.yml b/.github/workflows/get-version-task.yml index 3ab8f141..4a1e7afc 100644 --- a/.github/workflows/get-version-task.yml +++ b/.github/workflows/get-version-task.yml @@ -1,8 +1,8 @@ name: Get version information task -# Run NBGV once and expose the version outputs. The publisher passes its trigger branch as ref so the run -# versions the branch it publishes; github.ref matches that branch (one branch per run), so NBGV classifies it -# correctly without overriding github.ref. +# Run NBGV once and expose the version outputs. The publisher passes the exact commit it publishes as ref, and +# github.ref still names the branch that commit belongs to (one branch per run), so NBGV classifies it correctly +# without overriding github.ref. on: workflow_call: inputs: @@ -44,7 +44,7 @@ jobs: dotnet-version: 10.x - name: Checkout code step - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: ref: ${{ inputs.ref }} fetch-depth: 0 diff --git a/.github/workflows/merge-bot-pull-request.yml b/.github/workflows/merge-bot-pull-request.yml index 69eb76c2..5d02979c 100644 --- a/.github/workflows/merge-bot-pull-request.yml +++ b/.github/workflows/merge-bot-pull-request.yml @@ -43,7 +43,7 @@ jobs: # Auto-merge every tier, semver-major included: the required checks are the gate, not the bump magnitude. - name: Merge pull request step run: | - set -euo pipefail + set -Eeuo pipefail case "${{ github.event.pull_request.base.ref }}" in develop) method=--squash ;; main) method=--merge ;; diff --git a/.github/workflows/publish-release.yml b/.github/workflows/publish-release.yml index e5e36a02..ab0ef89f 100644 --- a/.github/workflows/publish-release.yml +++ b/.github/workflows/publish-release.yml @@ -35,8 +35,11 @@ jobs: secrets: inherit permissions: contents: write + # The release job deletes the release-asset-* transfer artifacts once they are attached to the release. + actions: write with: - ref: ${{ github.ref_name }} + # Pin the exact dispatch/schedule-time commit: a push landing mid-run must not be released unvalidated. + ref: ${{ github.sha }} branch: ${{ github.ref_name }} smoke: false github: true diff --git a/.github/workflows/test-pull-request.yml b/.github/workflows/test-pull-request.yml index 636dea28..5f6e90c1 100644 --- a/.github/workflows/test-pull-request.yml +++ b/.github/workflows/test-pull-request.yml @@ -54,7 +54,7 @@ jobs: steps: - name: Check workflow results step run: | - set -euo pipefail + set -Eeuo pipefail for result in "validate:${{ needs.validate.result }}" "smoke-build:${{ needs.smoke-build.result }}"; do name="${result%%:*}" value="${result#*:}" diff --git a/.github/workflows/validate-task.yml b/.github/workflows/validate-task.yml index d4096628..d1cfddb9 100644 --- a/.github/workflows/validate-task.yml +++ b/.github/workflows/validate-task.yml @@ -26,7 +26,7 @@ jobs: dotnet-version: 10.x - name: Checkout code step - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: ref: ${{ inputs.ref }} @@ -57,13 +57,13 @@ jobs: dotnet-version: 10.x - name: Checkout code step - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 with: ref: ${{ inputs.ref }} - name: Check C# formatting step run: | - set -euo pipefail + set -Eeuo pipefail dotnet tool restore dotnet csharpier check . @@ -71,7 +71,7 @@ jobs: run: dotnet format style --verify-no-changes --severity=info --verbosity=detailed - name: Lint markdown step - uses: DavidAnson/markdownlint-cli2-action@8de2aa07cae85fd17c0b35642db70cf5495f1d25 # v24.0.0 + uses: DavidAnson/markdownlint-cli2-action@6bf21b07787794f89a243495939cd651942aeabe # v24.1.0 with: globs: '**/*.md' @@ -89,12 +89,12 @@ jobs: # the VSCode tasks run the latest tools, so local may differ slightly by design. Config lives # in RegressionTests/pyproject.toml, so run from that directory (mypy resolves config from CWD). - name: Setup uv step - uses: astral-sh/setup-uv@11f9893b081a58869d3b5fccaea48c9e9e46f990 # v8.3.2 + uses: astral-sh/setup-uv@c771a70e6277c0a99b617c7a806ffedaca235ff9 # v9.0.0 - name: Lint Python step working-directory: RegressionTests run: | - set -euo pipefail + set -Eeuo pipefail uvx ruff@0.15.22 check . uvx ruff@0.15.22 format --check . uvx mypy@2.3.0 . diff --git a/.markdownlint-cli2.jsonc b/.markdownlint-cli2.jsonc index 4afb100e..e570ccef 100644 --- a/.markdownlint-cli2.jsonc +++ b/.markdownlint-cli2.jsonc @@ -1,10 +1,9 @@ { "config": { - // Prose paragraphs and data-heavy tables/URLs are intentionally long; - // reflowing at 80 cols hurts readability and churns diffs. + // Prose paragraphs and data-heavy tables/URLs are intentionally long. + // Reflowing at 80 cols hurts readability and churns diffs. "MD013": false, - // Inline HTML is used for reference-link section dividers. - "MD033": false, + // MD033 (inline HTML) stays enabled: HTML comments (reference-link dividers) pass it, and elements are flagged so native markdown wins. // Require fenced code blocks over the legacy 4-space-indented style. "MD046": { "style": "fenced" }, // MD060 (table column style) is not enforced - allow both compact diff --git a/AGENTS.md b/AGENTS.md index 9e1cee39..ff1ef654 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -6,44 +6,97 @@ This file is the canonical reference for cross-cutting AI-agent rules. The CI/CD **Where rules live.** A durable project, code, or style rule belongs in this file (or `WORKFLOW.md` / `CODESTYLE.md` as appropriate), so it is versioned and read by every session and every agent. An agent's own session memory or scratch state is private and lost on restart, so it is never the system of record for a rule: when you learn or are corrected on a rule, write it into the right doc in the same change. Memory may also note it, but the committed docs are the source of truth. +## Foundational Principles + +The specific rules in this file implement a few governing principles. Read these first: they are the reason the branching, release, and versioning rules are shaped the way they are, and every rule below serves one of them. + +- **Distribution respects the user: pull by default, push only where the channel forces it.** Docker images, GitHub Releases, and NuGet/PyPI packages are **pull** - the user decides when to consume them. A few channels are **push**: HACS surfaces a new release to every installed user as a pending update they did not go looking for, and a consumer that vendors from `main` picks up its current state. Because a release can reach users who did not ask for it, releasing is a deliberate act that marks a real functional change - never mechanical churn. This is why a **human merge never auto-publishes** - a release is a deliberate `workflow_dispatch`, or a conditional auto-release when the App merges a code-affecting Dependabot/codegen PR to `main` (Docker also refreshes on a weekly schedule) - together with the no-op republish guarantee and maintainer-gated version bumps: a needless release spends the user's attention and, on a push channel, acts on their machine. +- **Both branches stay in sync, so a promotion never needs a back-merge.** Dependabot and codegen target `develop` and `main` in parallel, so neither branch drifts and a `develop -> main` promotion stays a clean forward merge by default. That is exactly what lets the model be **signed, linear, and free of back-merges** - forward sync removes any need to merge `main` back into `develop`, which the rules forbid. If sync is ever broken (a change lands on one branch only, or normalizes a file on one side), restore it forward-only, never back-merge. See "Branching Model". (These auto-publish rules describe `release` repos. **Operational** repos differ - direct-to-`develop`, dispatch-only release - see "Operational Repositories".) +- **Two version numbers, two jobs.** The 2-digit `major.minor` in `version.json` carries human meaning - the maintainer raises it only for a functional change (feature, behavior or API change, breaking change), at their discretion - while NBGV owns the patch position and always increments with git height, so every build is uniquely versioned with no edit. Human-facing docs name the 2-digit line; the toolchain guarantees monotonic builds. See "Release Model". +- **Contracts state what, not how, and favor reuse.** [`WORKFLOW.md`](./WORKFLOW.md) fixes required outcomes, not a required implementation - two repos may satisfy a guarantee with different YAML. Within that freedom, apply good engineering practice: minimize duplication and maximize reuse, which is why the pipeline splits a carried, generic orchestration layer from a repo-owned build layer. + +## Durable Knowledge and Self-Improvement + +- **Durable knowledge lives in the committed docs, not in agent memory.** Anything a future agent must honor - a rule, a contract, a hard-won gotcha, a pattern worth repeating or one to avoid - belongs in a committed governance file (`AGENTS.md`, `CODESTYLE.md`, `WORKFLOW.md`, or a committed backlog such as a `README.md` TODO section). Agent memory does not survive a new session, a new machine, or a new environment, so it holds only environment-specific nuance and in-flight session state - never anything whose loss on reset would matter. A durable lesson left only in memory is lost to the next agent. +- **Keep the governance current as you work.** When work surfaces something durable - a rule worth enforcing, a recurring gotcha, a positive pattern to repeat, a negative one to design out - record it in the governance docs as part of that change, rather than leaving it in a local note or routing around it with a one-off workaround. Where the governing doc is carried from a template this repo cannot edit directly, propose the change upstream instead of only fixing it locally. Governance is not static: it improves by agents folding good patterns in and designing bad ones out. + +## Repository Boundaries and Write Safety + +A state-changing GitHub call is the highest-blast-radius thing an agent does here: it runs under the maintainer's identity, so one wrong target writes to another owner's repository as the maintainer - an outward-facing, hard-to-reverse act. These rules bound every write - a git push, an API mutation, a comment, a label, a merge - on any platform. Reads are unrestricted. The bounds below are on writes. + +- **Write only to the current project's own repository.** Every state-changing call targets this project's `origin` and nothing else. A broad or logged-in identity is capability, not permission - a token that *can* reach another repository does not authorize writing to it. Writing to any other repository needs explicit, per-session human permission for that specific repository, and a "harmless test" write is still a write, so there is no probe exception. Reads from anywhere are fine. +- **Never fabricate, guess, or reuse an identifier passed to a write.** Every id a state-changing call consumes - a node id, a numeric id, a thread or comment id - is captured from a live query in the **same** session into a variable and passed from there. Do not hand-type an id, guess it, recall it from memory or an earlier session, or copy it from documentation or an example. Ids commonly resolve **globally**, so a wrong-but-valid id does not fail - it writes to the wrong target, in someone else's repository. If a query returns no id, stop rather than invent one to proceed. +- **A write is never a probe, and a write's output is never suppressed.** Never fire a state-changing call to see whether it works: decide it should happen, make it happen, and read the result. Never append output-discarding redirection or a force-success tail to a mutation (for example `>/dev/null`, `2>/dev/null`, `&>/dev/null`, `|| true`, `|| :`, `|| echo`) - the write's output is exactly what must be read. A write that appears to fail is **verified, not assumed harmless** - the operation may have succeeded on the server while the client reported an error - so confirm the actual state before retrying or moving on. The ban targets hiding a *failure*. An ad-hoc call's response is the only signal you get, so `>/dev/null 2>&1`, `|| true`, or `|| echo` - which swallow the error stream or force success - are never acceptable on one. A committed script under `set -e` is a narrow exception: it may send a write's *stdout* to `/dev/null` to drop the success-response noise, because stderr stays visible and a failed write still aborts loudly (`repo-config/configure.sh` does exactly this). The exception is stdout-only suppression inside a reviewed, fail-loud script, never `2>&1` or a force-success tail, and never an ad-hoc command. + ## 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. +- **Check the working tree for the maintainer's own uncommitted edits before committing.** The maintainer hand-edits files live (often `README.md`/`HISTORY.md`, sometimes with the editor's LF->CRLF flip on top). Review `git status` first. If there are changes you did not make, ask whether to include them rather than bundling half-finished work or stranding it in an unrelated commit. +- **All commits must be cryptographically signed (SSH or GPG).** Branch protection enforces this on both branches, and 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. - **Commit under the committing account's own GitHub `noreply` identity - never a private, personal, or invented address.** The `author` and `committer` on every agent-authored commit are the GitHub `noreply` address of the account whose key signs the commit (above) - GitHub issues these in a `username@users.noreply.github.com` or `ID+username@users.noreply.github.com` form, and for this single-maintainer fleet it is the owner's `ptr727@users.noreply.github.com`. Do not set `user.name`/`user.email` to a fabricated persona, bot name, or product name, and do not commit under whatever identity the environment happens to carry: verify `git config --get user.email` is that GitHub `noreply` address before committing, and fix it if not. A wrong identity is not cosmetic - a private email trips GitHub's email-privacy push protection (GH007), and an unrecognized or invented author pollutes history. Identity is separate from signing: a wrong author does not by itself fail the signature rule, but the ad-hoc identities that produce it are typically also unsigned, which the signing rule above then rejects on push. - **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. +- **A history rewrite includes only the commits that must change, and re-identifies any commit it rewrites that is not yours.** Filtering history (`git filter-repo` / `filter-branch`, e.g. to strip PII) rewrites the touched commits and you re-sign them with your key, while the tooling preserves each commit's original `author` and `committer` unless told otherwise. GitHub verifies a signature against the commit's `committer` identity, so a signature from your key over a commit still committed by a bot (`dependabot[bot]`, `github-actions[bot]`) or by GitHub's web-flow does not match its committer and is marked `unknown_key`/unverified, which a require-signed-commits ruleset then rejects. Two gates keep committer and signature aligned. **First, scope the rewrite to only the commits that must be modified** - by default those are your own, whose committer is already your identity, and a commit that does not need changing is kept out of the rewrite so its identity and signature are never touched. **Second, if a commit that must change is not yours, set its `committer` to the signing identity before re-signing** (and its `author` too, since a rewrite that alters the content should not keep attributing it to the bot), so the committer GitHub verifies matches your key - the original bot attribution is deliberately given up as the cost of having to rewrite it. Never leave your signature over a commit committed by another identity. Verify after the rewrite that every rewritten commit is signed and committed under your identity (`git log --show-signature`). - **Never run destructive git commands** (`git reset --hard`, `git checkout .`, `git restore .`, `git clean -f`) without explicit developer instruction. -### Git and Commit Rules - Repo-Specific Notes - -- **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 +- **Two workflow models, set per repo by the registry `workflowModel` field.** Most repos are `release`: they ship versioned units of delivery through the feature -> `develop` -> `main` flow this section describes. **Operational** repos instead track a live service's running state and differ substantially - direct-to-`develop`, advisory CI, dispatch-only release - see "Operational Repositories". The rest of this section is the `release` model unless noted. The promotion mechanics (never delete `develop`, EOL-conflict resolution) apply to both. - `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 to the weekly publish - see "Release Model" below). Branch protection enforces this: the develop ruleset allows only `squash`, the main ruleset allows only `merge`. +- `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 (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. Every tier auto-merges, semver-major included - the required checks are the gate, not the version bump. -- **Maintainer-pushed commits on a bot PR auto-disable auto-merge.** The merge-bot's `merge-dependabot` job only fires on `opened` / `reopened` events (auto-merge is enabled exactly once per PR, for Dependabot-authored PRs that originate from this repository, not forks). When a maintainer pushes commits to the bot's branch (a `synchronize` event with a non-bot actor), the `disable-auto-merge-on-maintainer-push` job fires and calls `gh pr merge --disable-auto`; the maintainer's commits stay in the PR but won't auto-merge with the bot's content. Re-enable manually (`gh pr merge --auto `) when ready. The merge-bot is on `pull_request_target` with per-PR concurrency; it carries only `merge-dependabot` + `disable-auto-merge-on-maintainer-push`. -- **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. -- **Mirror to `develop` any change that lands on `main` outside the feature -> develop -> main flow.** "Mirror" means landing the same fix directly on `develop` via a follow-up PR targeting `develop` - never a `main -> develop` back-merge, which the forward-only rule forbids. A reconciliation-branch fix made to resolve a `develop -> main` promotion conflict, or a security PR that merges only to `main`, leaves `develop` behind on that content - and forward-only `develop` never back-merges to catch up (the same parallel-target principle as the bots). Before basing new work on `develop`, or diagnosing a defect from it, compare content and not commit history: run `git diff origin/main origin/develop` and inspect its `-` lines - the `main`-side of each difference, to check for staleness. A `-`/`+` pair within one hunk is usually just `develop` modifying that code as normal unpromoted work (occasionally `develop` is reworking a `main`-side fix differently - worth a glance). The stronger staleness signal is a deletion-only hunk (`-` lines, no `+` lines): content on `main` that `develop` lacks entirely, i.e. a `main`-only fix `develop` never received, so the defect may already be fixed on `main`. Prefer this over a commit-log check like `git log origin/develop..origin/main`, which is noisy here because it also lists routine promotion merges and the `main`-direct bot commits whose content `develop` already carries via its own parallel bot PRs. -- **Put issue-closing keywords (`Closes #N`) where they fire on merge to the default branch (`main`).** GitHub closes an issue from a *PR description* only when that PR merges to `main`, so a `Closes #N` in a PR that targets `develop` never fires - put it in the `develop -> main` promotion PR instead. A closing keyword in a *commit message* does close the issue once that commit reaches `main` via promotion, but that is fragile across squash-merges, so prefer the promotion PR's description or close the issue manually once the fix lands on `main`. +- **`develop` is forward-only - no `main -> develop` back-merges.** The develop ruleset's squash-only setting physically blocks merge commits on develop. Any historical back-merge commits in `git log` predate this rule and must not be repeated. +- **Executing a `develop -> main` promotion safely - two traps, both learned the hard way:** + - **Never delete `develop`.** A promotion PR's head *is* `develop`, so `gh pr merge --delete-branch` (and the repo's "Automatically delete head branches" toggle, which is why that toggle is [kept off](./repo-config/settings.json)) deletes `develop` itself. Merge a promotion with a plain `gh pr merge --merge`, no `--delete-branch`. If `develop` is ever lost this way, restore it to the merged PR's head SHA - the SHA is still reachable as the merge commit's second parent: `gh api -X POST "repos///git/refs" -f ref=refs/heads/develop -f sha="$(gh pr view --json headRefOid --jq .headRefOid)"`. + - **Spurious EOL-only conflicts resolve by taking `develop`.** When develop declared workflow YAML as LF while main is still CRLF, `develop -> main` conflicts *whole-file* on those paths. develop's `required_linear_history` + PR rulesets forbid resolving on `develop` (no merge commit, no force-push), so resolve on a throwaway branch off `main`: `git checkout -b promote/develop-to-main origin/main && git merge origin/develop`, take develop's side for the EOL-conflicted files (`git checkout --theirs `) **after confirming each is content-identical modulo EOL or that develop is a strict superset** (`diff <(git show :2:f|tr -d '\r') <(git show :3:f|tr -d '\r')`), then open that branch -> `main`. Verify no genuine main-only content is dropped (build/test where the repo supports it). +- **Both rulesets intentionally omit "Require branches to be up to date before merging".** The flag is off on `main` and on `develop`, for related but 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. Other technical workarounds - rebasing develop onto main, or rewriting develop's history - exist but contradict the squash-only develop ruleset and the linearity invariant. + - *Develop:* the check stalls bot auto-merge when two bot PRs against develop land within the same window. As soon as the first merges, the second flips to `mergeStateStatus: BEHIND` and GitHub's auto-merge will not fire while strict is on. The merge-bot only *enables* auto-merge on `opened`/`reopened` (see below) and never auto-updates bot branches, and Dependabot's rebase isn't real-time, so the second PR sits OPEN with all checks green indefinitely. Squash mechanics still rebase the diff onto develop's tip on merge, `required_linear_history` still enforces linearity, textual conflicts still block `mergeable: CONFLICTING`, and the required `Check pull request workflow status job` still gates merges - the only thing lost is pre-merge detection of *semantic-but-not-textual* conflicts, which the post-merge develop CI run catches anyway. + - See [`repo-config/README.md`](./repo-config/README.md) "Rulesets" for the configured state. +- **Configuring branch protection on a fleet repo: don't hand-build the rules.** Reconstructing the rules by hand is error-prone and has gone wrong on past ports. First delete **all** legacy classic branch-protection rules and any stray rulesets (rulesets are the *only* mechanism used), then create **exactly two rulesets named `develop` and `main`** by importing the committed `repo-config/*.json` ruleset payloads via `gh api -X POST "repos///rulesets"` (`gh ruleset` is read-only). The names are load-bearing - this file and the workflows reference them. Operational repos import `repo-config/operational/develop.json` as their `develop` ruleset (the `main` ruleset is shared); [`configure.sh`](./repo-config/configure.sh) selects the right develop payload from the registry `workflowModel` automatically. **Brownfield repos** (pre-existing history) need an extra step: `Require signed commits` rejects legacy unsigned commits and the admin bypass does not cover `git push --force`, so re-signing requires temporarily disabling the ruleset. +- **Bots (Dependabot and codegen) target both `main` and `develop` in parallel.** [`.github/dependabot.yml`](./.github/dependabot.yml) duplicates every ecosystem entry (one per branch) and the codegen workflow runs as a matrix over both branches with branch names `codegen-main` and `codegen-develop`. 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). 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. The merge-bot auto-merges **every** Dependabot tier including semver-major (no ecosystem or update-type guard): the required CI checks are the gate, not the bump magnitude, so a major that breaks the build fails its checks and never merges. +- **Maintainer-pushed commits on a bot PR auto-disable auto-merge.** The merge-bot's `merge-dependabot` and `merge-codegen` jobs only fire on `opened` / `reopened` events (auto-merge is enabled exactly once per PR). When a maintainer pushes commits to a bot's branch (a `synchronize` event with an actor that isn't the same bot), the merge-bot's `disable-auto-merge-on-maintainer-push` job fires and calls `gh pr merge --disable-auto`. The maintainer's commits stay in the PR but won't auto-merge with the bot's content; re-enable auto-merge manually (`gh pr merge --auto ` or the GitHub UI) when ready. +- **Why parallel dual-target rather than develop-only with eventual flow-through:** push-distribution channels (HACS for Home Assistant integrations, Linux distros that vendor from `main`, etc.) consume `main` directly. A develop-only model would leave `main` running stale code during long-running develop features. Codegen content can also be production-critical (live API-derived data, language lists, build catalogs) rather than just sample/demo content, so both branches need fresh codegen on their own cadence. +- **Codegen regenerates committed files; its output must be deterministic from its inputs, never per-run state.** The codegen workflow is a mechanism to refresh files that are checked into the repo: it runs a matrix over `main` and `develop`, each leg regenerating against its own checkout and opening its own PR (`codegen-main -> main`, `codegen-develop -> develop`). For the two legs not to conflict on `develop -> main`, the generated output must depend only on its inputs - never on per-invocation state (timestamps, GUIDs, build IDs), which would diverge every run and conflict on every release. **What** a repo regenerates (data files, source, or both; code changes or pure data) and **how** (download and process an external source, transform local inputs, whatever) is entirely its own concern - the constraint is only that the output be input-deterministic, not how it is produced. + - *Reference:* a repo adopting codegen supplies its own input-deterministic generator and wires the codegen reference workflow (`run-codegen-pull-request-task.yml` and its scheduler). +- **App-token workflows use Client ID, not App ID.** `actions/create-github-app-token` deprecated the numeric `app-id` input in v3.0.0; use `client-id: ${{ secrets.CODEGEN_APP_CLIENT_ID }}`. When adding new App-token call sites, use the same form - do not reintroduce `app-id` / `CODEGEN_APP_ID`. See [`repo-config/README.md`](./repo-config/README.md) "Secrets" for which secrets each mechanism needs. ## Release Model -The publish behavior - the **scheduled + on-demand** publisher (one branch per run: the weekly schedule rebuilds `main`, and a dispatch publishes the branch it is started from - native binaries + multi-arch Docker image + a GitHub release that anchors the version), branch-scoped versioning (`main` = stable / `latest`, `develop` = prerelease / `develop`), and the rule that **merges do not publish** (changes accumulate and ship in the next scheduled run, which also refreshes the Docker base image; release `develop` by dispatching from `develop`) - is specified in [`WORKFLOW.md`](./WORKFLOW.md), the canonical CI/CD guide. Do not duplicate those rules here. - -Versioning is the one release rule that is a **human process**, not a workflow outcome, so it lives here: - -- The `version` (major.minor) in [`version.json`](./version.json) is the version floor; NBGV appends the git height as the SemVer patch. `main` (the public release ref, `publicReleaseRefSpec = ^refs/heads/main$`) builds a stable `X.Y.`; `develop` builds a prerelease `X.Y.-g`. The maintainer edits `version.json`; *routine* dependency bumps, CI/workflow fixes, and doc edits leave it untouched. -- **Bump `version.json` only by maintainer instruction**, for a functional change (a new feature, a behavior change, a breaking change) or a significant one-time overhaul of the build/release process (such as a CI/CD migration), in the PR that introduces it (typically on `develop`). Do not bump on a cadence, for routine CI/workflow or dependency or doc edits, or mechanically after a release. -- **No post-release bump; no develop-ahead requirement.** NBGV advances the patch 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. -- **`dotnet/nbgv` is consumed via `@master`, never SHA-pinned.** Its tag stream lags `master` such that Dependabot tag-tracking would only propose downgrades to stale tags; this is the sole WORKFLOW.md D9.1 exception (rationale inline in the workflow). Do not SHA-pin it. +The **two-phase model is the default**: PRs build fast, publishing is batched. See [README "Release Distribution Model"](./WORKFLOW.md) for the full rationale. The load-bearing rules follow. The auto-publish paths (bot push, schedule) apply to `release` repos. **Operational** repos differ - dispatch-only release, no auto-publish - see "Operational Repositories". + +- **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** build of only the changed targets (Docker `linux/amd64` only, executable on a representative runtime 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 (a later code PR's smoke build, or the scheduled/publish run). Workflow YAML is still linted in CI by the lint job's `actionlint` step; also run `actionlint` locally before pushing to catch issues early. +- **A human merge never auto-publishes.** [`publish-release.yml`](./.github/workflows/publish-release.yml) is the sole publisher; each run builds the **single trigger branch** (`main` a release, `develop` a prerelease). A first `plan` job decides once whether the run publishes and every other job gates on its output. It publishes on a **`workflow_dispatch`** of `main`/`develop` (the human-initiated release), a **code-affecting bot push to `main`** (the codegen App merges every Dependabot/codegen PR, so `github.actor` is the gate - a human merge/promotion to `main` skips), or a **weekly `schedule`** (Docker only, to refresh the base image). The `push` is main-only and paths-filtered, so a develop bot merge and an Actions-only bump publish nothing. A source-only repo publishes on dispatch only. +- **Required check.** The `changes` job is in the `Check pull request workflow status job` 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.** Every `build-*-task.yml` and `build-release-task.yml` takes `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`** - each run builds one branch; the top-level publisher passes `branch: ${{ github.ref_name }}`, which the tasks forward and read as `inputs.branch` (not `github.ref_name`) for config/tags/prerelease. `get-version-task.yml` takes a `ref` so NBGV versions the right branch. +- **Per-target subsetting.** `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`, its path-filter entry in `test-pull-request.yml`, and (for PyPI) the `publish-pypi` job in `publish-release.yml`. CodeGen, versioning, badge, merge-bot, and Dependabot are target-agnostic. +- **Orchestration vs. build - the override seam.** The pipeline splits into two layers. The **orchestration** layer is generic and is the standardization baseline: [`publish-release.yml`](./.github/workflows/publish-release.yml) (single-branch publish plan), the `get-version` + `github-release` jobs inside `build-release-task.yml`, `get-version-task.yml`, `build-datebadge-task.yml`, and the aggregator shape of [`test-pull-request.yml`](./.github/workflows/test-pull-request.yml). Within `test-pull-request.yml`, only the `changes -> smoke-build -> check-workflow-status` aggregator wiring and the ruleset-bound job name are verbatim orchestration; the `unit-test` job and the `dorny/paths-filter` entries are owned/per-target. The **build** layer - the `build--task.yml` leaf tasks - is what a derived project owns and replaces. The contract that keeps the seam clean: **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 - its `download-artifact` step uses `pattern:`/`merge-multiple:`, **never an `artifact-ids:` that names a build job's output** (the producing build jobs still appear in `needs` for sequencing) - so it (the tag-the-commit + create-the-release + attach-the-assets logic) is reusable **as-is** across repos. **This name-pattern handoff is canonical for every repo, single-target included** - name your one asset `release-asset--` and the verbatim `github-release` globs it; do not switch a single-target repo to an `artifact-id` output plus `download-artifact` `artifact-ids:`, which looks tidier for 1:1 but forks the `github-release` download (`pattern:`/`merge-multiple:`) and breaks its verbatim carry. + - **What a repo still curates** (this is by design, not a leak): the *list* of leaf jobs in `build-release-task.yml`. Per **Per-target subsetting** above, you delete the target jobs you don't ship and add the one(s) you do - `build-release-task.yml`'s `github-release` job is untouched, but the file is not byte-identical because its `needs`/job list reflects your targets. Making that list itself target-agnostic is a larger "factor build from orchestration" refactor that is intentionally **not** done. + - **Map your outputs to the right seam** - pick by where each artifact *goes*, not by language: + - *Files attached to the GitHub Release* (zips, binaries, packaged libraries): one leaf task per output, each uploading `release-asset--`. A data-only repo (e.g. a symbol library) has exactly one such task: validate -> `zip` -> upload `release-asset--library`; it deletes the nuget/pypi/executable/docker jobs and the `publish-pypi` job, keeps `github-release` as-is. This is also where the .NET `build-executable-task` lives - it is *not* a generic file step, it is specifically `dotnet publish` of the console app; replace it wholesale, don't adapt it. + - *Package-registry pushes* (NuGet.org, PyPI): the leaf task both builds **and** publishes to its registry. NuGet pushes from inside `build-nugetlibrary-task` (`dotnet nuget push --skip-duplicate`) *and* also uploads a `release-asset-*` (.7z) for the GitHub release. PyPI is split: `build-pypilibrary-task` only builds + uploads the `pypilibrary-build-` artifact, and the separate `publish-pypi` job in `publish-release.yml` does the OIDC Trusted-Publishing upload (so `id-token: write` is granted only at that one entry point) - PyPI contributes **no** `release-asset-*`. + - *Image-registry pushes* (Docker Hub): `build-docker-task` pushes multi-arch tags directly; contributes **no** `release-asset-*`. The image tag is build-layer-owned - drive it from whatever version source fits (NBGV `SemVer2`, an upstream-release pin, or a per-image matrix). To publish the Docker Hub repository overview, `publish-docker-readme-task.yml` pushes `Docker/README.md` via `peter-evans/dockerhub-description` (single-repo by default; matrix per image for multi-image repos), wired into `publish-release.yml` and gated to `main`. + - *Source-only / no build* (validate + tag + release): this seam does not apply. A source-only repo carries **no** `build-release-task.yml` (its `appliesTo` excludes it), so there are no leaf tasks and no `get-version`/`github-release`/`date-badge` jobs to curate. Its whole release is the standalone [`publish-release.yml`](./.github/workflows/publish-release.yml) on `workflow_dispatch`: a `validate` job (the repo's reusable validation task) gates a publish job that **inlines** NBGV for the tag and `action-gh-release` for the release (tag + auto source archive + README + LICENSE). + - `get-version-task.yml` installs the .NET SDK only because NBGV needs the runtime to compute the version/tag - heavyweight but expected even for a non-.NET repo; acceptable as-is. +- **No-op republish guarantee.** A weekly/dispatch publish where NBGV `SemVer2` is **unchanged** (no new commit since the last publish) re-pushes **nothing** to GitHub Releases (the `github-release` job's `release-exists` check skips the create step), NuGet (`dotnet nuget push --skip-duplicate`), or PyPI (`gh-action-pypi-publish` `skip-existing: true`) - all three key on the version string. **Docker always re-pushes** by design: it picks up upstream base-image refreshes (e.g. `ubuntu:rolling`) that aren't visible in the repo. Boundary: `version.json` has **no `pathFilters`**, so *any* commit - including a CI/workflow-only or docs-only change - advances the NBGV git height and therefore `SemVer2`, and the next publish *does* create a fresh release for it even when the shipped binary is byte-identical. This is accepted NBGV behavior; `pathFilters` are intentionally not added. +- **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`. The maintainer edits `version.json`; dependency bumps, CI/workflow fixes, and doc edits 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 or API 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 carries the unchanged `version.json` and `main` advances only its NBGV height. +- **Docs reference the 2-digit `major.minor` line, never a 3-digit build.** `README.md`, `HISTORY.md`, and release notes name the version as `Version 1.0` (the `version.json` floor); NBGV owns the patch/build position, so a concrete three-part number in a doc is both wrong (the real build height differs) and a maintenance trap. "Correcting" `1.0` to `1.0.0` is a defect, not a fix - it has blocked a release. +- **Issue-closing keywords (`Closes #N`, `Fixes #N`) go in the `develop -> main` promotion PR, not the feature -> develop PR.** GitHub auto-closes an issue only when the closing keyword merges into the **default branch** (`main`); a feature/develop PR merges into `develop`, so the keyword never fires there. Reference the issue in the develop PR body if useful, but put the actual closing keyword on the promotion PR. +- **Wrapper repos that track an upstream release.** A repo wrapping an upstream release uses `check-upstream-version-task.yml`: a resolver command prints the upstream version(s) as a **JSON object of `name -> version`**, written to a committed state file at the **repo root beside `version.json`** (default `upstream-version.json` - it is a build-input version source, not GitHub-platform config, so it does not belong under `.github/`), and opens a rolling App-signed bump PR per branch that the merge-bot auto-merges (`merge-upstream-version`). The object carries one key for the common single-version case (`{"version":"X"}`) or N keys for a wrapper that pins several upstream components (e.g. an image plus a companion tool), and the build reads each component by key; the bump PR's title/body name only the keys that actually moved. Call it from a scheduled entry-point workflow and matrix only the branches that ship the version (a CI-only version uses `["develop"]`). A merged bump ships on the **next publish**, not immediately - the two-phase latency tradeoff. + +## Operational Repositories + +The registry `workflowModel` field is `release` (the default) or `operational`. This section is the operational delta - every other rule in this file is the `release` model unless it says otherwise. + +**Operational** repos track a live service's running state rather than shipping versioned units of delivery - live-service config such as Home Assistant, ESPHome, Vantage, and home automation. + +- **Commit configuration directly to `develop`.** There is no feature branch - the maintainer commits straight to `develop` and *occasionally* opens a `develop -> main` PR to bless a known-good snapshot. The `develop` ruleset drops the PR and status-check gate, so direct signed pushes are allowed (force-push, deletion, and unsigned commits are still blocked) and CI runs on the push as **advisory** feedback that never rejects a commit. +- **The `main` promotion gate is unchanged.** The [`main` ruleset](./repo-config/main.json) is shared with `release` repos, so the `develop -> main` PR still **enforces** the required `Check pull request workflow status job`. For an operational repo that check is lint/validation only - editorconfig/EOL plus domain linters (a Home Assistant or ESPHome config validation, a firmware build), never unit tests - so `develop` stays the live surface and a broken config can never reach `main`. +- **Release only by manual dispatch.** Operational repos carry `releaseTrigger: dispatch-only` and run no codegen or auto-publish bots, so they publish **only** on a manual `workflow_dispatch` - the source-only release the publisher already supports (tag + source zip + README + LICENSE, NBGV-versioned), never automatically. The `develop -> main` promotion just blesses a known-good snapshot, and a release is a separate, deliberate dispatch. +- **Fleet sync still applies.** Dependabot's dual-target sync and the App-signed merge-bot run on **every** tier, operational included, so both branches stay in sync and a promotion stays a clean forward merge. + +Line-ending governance for an operational repo is in [Line Endings](#line-endings) - its `[*]` default follows the consuming app's native platform per the registry `lineEndings` field, not the fleet CRLF default. ## Pull Request Title and Commit Message Conventions @@ -62,34 +115,53 @@ Versioning is the one release rule that is a **human process**, not a workflow o ### Examples ```text -Add Direct Play seek-index verification +Add structured logging extensions to library Pin softprops/action-gh-release to commit SHA -Remove embedded closed captions during remux +Drop net8.0 multi-targeting from console project Bump xunit.v3 from 3.2.2 to 3.3.0 -Clarify HandBrake custom-options usage in README +Clarify devcontainer setup steps in README ``` ## Documentation Style Conventions +- **Carried files carry no coordination references.** In the files the fleet carries - `AGENTS.md`, `CODESTYLE.md`, `WORKFLOW.md`, `.github/copilot-instructions.md`, the `repo-config/` and `spec/` files, the carried `AUDIT.md` - two things are banned. **Any reference to the template repo**, in prose or in a link: it is private, so a link 404s for a downstream repo's users, and the coordination flow is machinery a consumer of that repo should never have to see. Where a carried file must express a template-level behavior - "report a rule discrepancy upstream" - state the behavior rather than the destination. The maintainer supplies the destination out of band. And **a sibling fleet repo named as an illustrative example** ("repo X does it this way", "see repo Y's adoption"), which couples the repos and rots as they diverge. To point at a current good example, name it in the onboarding or conformance issue, never in a carried doc. **A contextually relevant link to a related project is not a coordination reference, and is expected.** Where another repo is part of this repo's subject matter - the image that consumes this config, the builder that generates this hardware, a library this depends on - link it normally. The test is whether the link serves a reader of *this* repo's content, not whether the target happens to be in the fleet. This rule governs carried template content. A repo's own `README.md` and topical docs are its own content, not carried verbatim, and it does not reach them. This pairs with the present-tense rule below: state the current shape, not a history of which repo it came from. + ### 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. +- **Reference-style links in human-facing docs.** Every markdown file **except** the agent-instruction files (`AGENTS.md` and `.github/copilot-instructions.md`, which optimize for agents and keep inline links) uses reference-style links only: every URI - internal path, anchor, external URL, or shield image - is defined at the **bottom of the file**, split into groups by type under an HTML-comment header (e.g. ``, ``, ``, ``) with each group's definitions alphabetized by reference name. **Reference names are contextual and encode the target and its group** - `foo-shield` for a shield image, `foo-link` for an external URL, and a bare `foo` for a local path or anchor (e.g. `[license-shield]`, `[releases-link]`, `[repo-config]`) - never numeric (`[1]`) or opaque. No inline `[text](uri)` targets in prose. **A URL inside a fenced code block stays inline** - reference links do not resolve in code blocks, so do not extract it, and exclude fenced code from any link-integrity check (bracket literals like `["a", "b"]` otherwise read as undefined references). **Removing a link also removes its reference definition** - an orphaned definition fails the no-unused-defs rule. The one exception is the Table of Contents, whose entries stay inline anchor links (see Table of Contents below). +- **Table of Contents.** Generate it with the Markdown All in One extension, which fills and auto-updates the list on save - leave the `## Table of Contents` heading for the extension to populate and never hand-author or hand-edit the entries. Exclude a heading with an inline `` marker on it (the badge/build header block and the `## Table of Contents` heading itself carry it); the workspace sets which heading levels appear. - 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. +- **Write in the present tense, describing only the current state.** The reader has no knowledge beyond what they are reading, so state what *is* - what to know, do, follow, or avoid - never a change from a prior state. Write "X does Y", never "X *now* does Y", "X *no longer* does Z", "X *still* does W", or "changed/switched/restored to Y". This applies to docs and code/workflow comments alike; before/after framing belongs in changelogs, commit messages, and PR descriptions - where the prior state is the point - not in `README.md`, `AGENTS.md`, or other living docs. +- **When you change a behavior, search for prose that asserts the old one.** Updating the guarantee or rule you are consciously editing is not enough: comments, diagram labels, reusable-workflow input descriptions, and audit statements elsewhere may still describe the prior behavior, and each was accurate when written. Grep for the old behavior's distinctive phrasing and fix every instance. No linter catches this - markdownlint, cspell, actionlint, and editorconfig-checker all pass on a claim that is merely untrue - so the sweep is the only mechanism that will. This is the maintenance counterpart to the present-tense rule above: that one governs how to phrase a doc, this one how to keep it true when the behavior underneath it moves. ### Comments Applies to code and workflow (`#`) comments alike. -- Comment only when the code is non-obvious or important. Self-evident code needs no comment. -- Judge "obvious" in context, not line by line. A note that reads as redundant on its own line can be essential in the larger flow - a comment marking a workflow step's exit condition, for example, even though the line itself plainly does a `return` or `exit`. -- State the non-obvious *why*, not what the code already shows. 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. -- **One line if it fits in ~120 columns.** Do not wrap a comment at 75-80 columns; a short two-line comment that would fit on one line looks sloppy - collapse it. Go multi-line only when the content genuinely exceeds ~120, filling each line rather than narrow-wrapping. For a multi-point comment, prefer short structured lines or `-` bullets over one prose paragraph. -- **Workflows: prefer one short summary description at the top of the file** over scattering rationale across steps; comment an individual step only when its purpose is non-obvious. -- **Do not accumulate comments.** When you change code or a comment, rewrite the whole comment fresh; never bolt a new comment onto an existing one or layer explanations across edits. Comment volume should stay flat or shrink over time, not grow. -- **Leave human-authored comments and emojis exactly as written** - do not reword, trim, reflow, or "clean" them, even if they seem to bend a rule. Revise only agent-authored comments, and match the surrounding voice when you do. +- 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 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. +- **Keep it short.** One line is the default. A comment earns a second line only by carrying a constraint the code cannot. Most comments are one sentence, and never restate *what* the code does - a well-named symbol already says it. +- **Structured, not prose: one sentence per line, and never wrap a sentence across lines.** No block paragraphs and no multi-sentence run-ons. A comment that genuinely needs several sentences is several lines, each a single sentence. A sentence too long for one sensible line is too long - split the thought. +- **A multi-line comment shows whether it is a continuation or a list.** A continuation of the same topic stays unindented, one sentence per line. Mark a sub-topic with a `-` after the comment marker (`# -`, `// -`), and only for genuine sub-topics - parallel items hanging off a lead line, never a continuation of one thought. +- **No class-, type-, or file-header summary comment blocks.** A type or file gets a comment only for a specific non-obvious point, kept terse - never a block summarizing what the file contains or what the class is for. A summary restates the declaration below it, goes stale as the file grows, and is the file-scope form of the design narrative and verbosity creep this section already bans. A license or provenance header a tool or policy requires is not a summary and is unaffected. +- **Do not grow a comment across edits.** When you touch code near an existing comment, the comment must come out **same length or shorter** - never append "one more clause" of rationale. If a block comment has crept to multiple sentences of prose, cut it back to its single load-bearing point as part of your change. Verbosity creep is the specific regression to prevent: every iteration that adds a clause is a regression, not an improvement. + +A continuation stays unindented, one sentence per line: + +```text +# Change gate for the compile tests. +# An esp-idf build costs minutes, so gate on what each test covers. +# A diff that cannot be computed runs everything. +``` + +Sub-topics take a `-` after the comment marker, each elaborating a distinct item named in the lead: + +```text +# Source lint plus change-gated compile tests. +# - compile-test builds the external component. +# - template-compile-test builds one example device per template. +``` ### Character Set @@ -98,23 +170,45 @@ Applies to code and workflow (`#`) comments alike. - 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) -> `...` +- **No semicolon joining two independent clauses in agent-authored prose** - documentation, comments, commit messages, and PR descriptions. Recast as a comma or as two sentences: "the check runs on push; it gates the merge" becomes "the check runs on push and gates the merge", or two sentences. A semicolon separating items in a list that already contains commas keeps its standard use, and a statement terminator in **code** is untouched by this rule. This bans the semicolon splice only - a colon introducing an explanation, elaboration, or list keeps its standard use and is not a splice. Existing prose is corrected as each file is next edited, not swept. - **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) is the single source of truth for line endings: CRLF for `.md`, `.cs`, XML/`.csproj`/`.props`, non-workflow `.yml`/`.yaml`, `.json`, `.cmd`/`.bat`/`.ps1`; LF for `.sh`, Dockerfiles, and workflow YAML (`.github/workflows/*.{yml,yaml}`). Workflow YAML is pinned LF because Dependabot and Actions rewrite it with LF, so declaring LF keeps it consistent instead of mixed; git still leaves endings alone (`* -text`) and CI's `editorconfig-checker` enforces it. The `[*.cs]`/ReSharper style block applies because this repo ships .NET. -- **Always honor the `.editorconfig` ending.** Create a file with its spec ending; when editing a file, bring the whole file to spec (a file-wide EOL fix alongside the content change is expected, not a violation); if you come across a file with the wrong ending, fix it. [`.gitattributes`](./.gitattributes) (`* -text`) governs git's own normalization - it is not a license to leave a file on the wrong ending. Verify with `file ` after writing. -- **Python (`.py`) and `.toml` are CRLF.** They have no `[*.py]`/`[*.toml]` override, so they inherit the `[*]` CRLF default (matching the audited convention that keeps Python on the repo default rather than pinning LF). Only the `.sh` harness is LF. +- **[`.editorconfig`](./.editorconfig) sets the line ending:** `[*] end_of_line = crlf` is the **default** - every file type is CRLF unless pinned otherwise - with **LF** pinned for the execution-sensitive exceptions - `*.sh`, Dockerfiles, and any individual `.py` executed directly via its shebang (pinned **by path**, e.g. `spec/validate.py`; vanilla `.py` stays CRLF, since Python's universal newlines accept it and it is commonly edited on Windows). Only the LF exceptions are declared; the redundant per-type CRLF rules are intentionally omitted. `.gitattributes` mirrors it: `* -text` (git stores the exact bytes you commit and will **not** normalize) plus the matching LF pins. +- **Choosing an ending for a new file type:** CRLF is the **default** - cross-platform editors on Windows produce it, and it is harmless on Linux for everything except shell. Use LF only when the type **requires** it or CRLF **breaks how it is consumed**: executable scripts/shebangs (`*.sh`, s6, husky), Dockerfiles (CRLF breaks `RUN` heredocs/continuations), and tool-owned formats with a native LF ending (KiCad). **Non-workflow YAML stays CRLF** - GitHub Actions' parser tolerates it (a repo that also runs yamllint sets `new-lines: disable` to defer to `.editorconfig`). **Workflow YAML (`.github/workflows/*.{yml,yaml}`) is pinned LF** in `.editorconfig` - Dependabot and Actions rewrite it with LF, so declaring LF keeps it consistent instead of mixed on every bump. This is an LF class **not** backed by a `.gitattributes` pin: git keeps `* -text` (no normalization), and CI's `editorconfig-checker` (EOL-only) catches a mismatch instead. Distinguish where a file is *consumed* from where it is *edited*: consumption on Linux alone does not force LF. A config or pattern file consumed by a Linux tool stays CRLF when the tool tolerates a trailing CR: `.dockerignore` and `.gitignore` are CRLF (their parsers strip the CR), and only a *Dockerfile* - interpreted, where a CR breaks `RUN` heredocs and line continuations - is LF. +- **Operational (config) repos: the global default follows the consuming application's native platform, not the fleet CRLF default.** A config repo (registry `workflowModel: operational`) is a *view into an application's configuration directory* - often the exact tree mounted into that app's container - so its files must use the ending the app itself reads and writes, and forcing the fleet CRLF default would fight the app. Set the `[*] end_of_line` default to the app's native ending and record it in the registry `lineEndings` field (`lf` | `crlf`): **LF** for a Linux-native app whose config lives in a Linux container - ESPHome, Home Assistant, a devcontainer-only or HACS config - and **CRLF** for a Windows-native editor - e.g. Vantage InFusion config edited by Design Center on Windows. The execution-sensitive LF pins (`*.sh`, Dockerfiles, workflow YAML) still apply on top, and `.gitattributes` still mirrors the chosen default. This override is for operational repos only; `release` repos keep the `[*] end_of_line = crlf` fleet default above. Do **not** re-normalize such a repo to the fleet default - that is exactly the over-normalization these per-repo endings prevent. + - **Mixed-consumer config: prefer to split by platform into single-platform repos, not one mixed repo.** When a config repo would be consumed on two platforms (a Linux app plus a Windows-edited subtree), the clean answer is a repo per consumer, each single-platform with its own `lineEndings` - e.g. a controller config edited by a Windows-native editor (CRLF) lives in its own repo, **not** as a subtree inside a Linux-`lf` config repo. That keeps each repo's default, CI, and checkout matched to one platform and avoids per-path EOL machinery entirely. **Fallback only if a subtree genuinely cannot be split out:** keep the global default at the primary consumer and pin the odd subtree with an `.editorconfig` path override (e.g. `[/**] end_of_line = crlf`) matching its consumer, treated like any tool-owned format; the global `* -text` in `.gitattributes` already preserves those bytes, so no extra git pin is needed. +- **Scripts and extensionless executables must be LF - and pinned in `.gitattributes`, not just configured.** A CRLF shebang (`#!/usr/bin/env bash\r`) breaks execution. `.editorconfig` sets `[*.sh] = lf`, but that extension-based rule does not match **extensionless** executables (s6 service scripts `run`/`up`/`finish`, husky/git hook scripts like `.husky/pre-commit`), and `* -text` enforces nothing - so a broad normalization pass or an editor can silently flip them to CRLF (it has). `.gitattributes` is the enforcement layer: it carries `*.sh text eol=lf`, and any repo whose tooling ships extensionless scripts **adds the matching path pin** - e.g. `Docker/s6-overlay/** text eol=lf` for s6 init, `.husky/pre-commit text eol=lf` for husky hooks - so git holds them at LF on checkout and `--renormalize`. This pin is mandatory for any repo that overrides s6 init, uses husky/git hooks, or otherwise ships executable scripts. The same explicit-pin rule extends to **tool-owned file formats the base config doesn't key on**: pin them to whatever ending the tool reads and writes so a normalization sweep can't churn them - e.g. KiCad project/footprint/3D files (`*.kicad_mod`, `*.kicad_sym`, `*.step`), which KiCad writes LF (`*.kicad_mod text eol=lf`, ...). The principle is general: a file class the `.editorconfig` extension rules and `* -text` don't cover needs an explicit `.gitattributes` pin matching its tool's native ending. +- **Pair each such pin with a matching `.editorconfig` override - the git pin alone is not enough.** `.gitattributes` governs **git** (checkout, commit, `--renormalize`); the **editor** follows `.editorconfig`, where the `[*] end_of_line = crlf` default still applies to any file no extension rule covers. So even with the git pin, the editor writes a CRLF shebang into an extensionless hook (breaking it when run from the working tree) or re-ends/trims a byte-sensitive data file. Give every extensionless **executable** an editorconfig LF override beside its `.gitattributes` pin (`[.husky/pre-commit] end_of_line = lf`); and for a **byte-preserve data directory** (downloaded or opaque source whose exact bytes the consumer may depend on) disable *all* editor normalization, not just EOL - `[/*]` with `charset = unset`, `end_of_line = unset`, `insert_final_newline = false`, `trim_trailing_whitespace = false` (`unset` is EditorConfig's spec-defined special value that removes an inherited property, so the editor enforces neither the global `charset` nor `end_of_line` on that path). Keep these overrides with the line-ending governance (above any `.NET-only` divider), not in the language-style section. +- **New files:** create them with the `.editorconfig`-mandated ending. +- **Editing an existing file:** **preserve the file's current line endings** - do not reflow them as a side effect of a content change, even if the file is already non-compliant. A tool that rewrites a file in text mode (a script, a bulk find/replace) can silently flip CRLF to LF and turn a one-line change into a whole-file diff. After any programmatic edit, verify before staging: `git diff --stat` should touch only the lines you changed, and a byte check should confirm the expected ending (`file` is unreliable here - see Auditing below). If a diff balloons to the whole file, you flipped the endings - restore them and re-stage. +- **Fixing a non-compliant file:** bring it to its `.editorconfig` ending as a **deliberate** change, and prefer to isolate it in its own EOL-only commit so the churn is reviewable. When a broader maintenance change has to normalize endings alongside content edits (a repo-wide cleanup sometimes does), call it out explicitly in the commit/PR description and verify the content separately with `git diff --ignore-cr-at-eol`. +- **Auditing line endings - don't trust `file` or naive `git ls-files --eol`.** The authoritative check is a **byte scan** that classifies by which endings are present: **CRLF-only** (every `\n` is preceded by `\r`), **LF-only** (no `\r`), or **mixed** (both forms present) - flag mixed explicitly rather than lumping it in with CRLF; skip binaries via a NUL-byte check. `file` mislabels some types (it reports a CRLF `.json`/`.code-workspace` as plain "JSON text data" with no CRLF note), and `git ls-files --eol`'s `attr/` column holds multiple tokens that shift naive field-splitting into false positives. Scope a repo-wide audit to `git ls-files` plus `git ls-files --others --exclude-standard` - never a raw `find`, which sweeps self-ignoring caches (`.mypy_cache`, `.artifacts`). Idempotent normalize: `b.replace(b"\r\n", b"\n").replace(b"\n", b"\r\n")`. A single within-line string replace is EOL-safe, but an agent tool that inserts **multiple lines** or writes a **new file** into a CRLF file must emit `\r\n` - a naive `\n` insert creates mixed endings. `.code-workspace` is JSONC (it has `//` comments); strip them before JSON-parsing it. +- **Both `.editorconfig` and `.gitattributes` are required.** [`.editorconfig`](./.editorconfig) **and** [`.gitattributes`](./.gitattributes) together govern line endings. A repo missing either file, or one whose `.editorconfig` sets no global `end_of_line` default (e.g. declares it only under `[*.md]`), will accumulate files mixed between LF and CRLF - the exact failure these two files prevent. The canonical form is a `[*] end_of_line = crlf` default plus the LF exception pins, mirroring `.gitattributes`. Carry both files **whole** (the `[*.cs]` block is inert without `.cs` files), including the `*.sh text eol=lf` pin and any extensionless-script path pins. Adopting `.gitattributes` for the first time requires a one-time normalization pass. ### 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. +## Verification Discipline + +The checks that separate work actually done from work that merely reports success. Their unifying property: **every failure below is green.** A skipped job and a passing job are indistinguishable in the aggregated required check, a pattern that matches less still exits zero, and a gate that stops gating still reports success. No linter, status check, or review layer catches any of them. + +- **A test must assert the mechanism it names.** Label each case by the behavior it proves, and satisfy yourself it would fail if that mechanism broke. A case that passes for an incidental reason - the right answer reached by the wrong path - is worse than no case, because it is later cited as evidence. +- **Gates, filters, and gate-like watchers fail loud, never narrow quietly.** A pattern that silently matches less, an allowlist that silently stops matching, or a gate that silently stops gating all report success while doing nothing. When a construct exists to notice something, make the not-noticing case produce an error or an annotation. An identity allowlist used as a gate, for one, must raise an error when its list stops matching, not silently pass everything through. +- **Run the repo's whole lint gate before every push, not the parts that look relevant.** CI runs all of them, so a partial local run only defers the failure - and the tool most likely to catch a given change is often the one it seems least about (an edit that manipulates line endings is exactly when `editorconfig-checker` matters). The repo documents each linter's known-working invocation - this rule is that **all** of them run. +- **Editing CRLF files programmatically: `.` matches `\r` in a regex**, so a captured line keeps its carriage return and rejoining with `\r\n` yields `CRCRLF`. A text-mode rewrite has the mirror failure, silently flattening CRLF to LF. Prefer line-based edits (`splitlines(keepends=True)`) or literal replacement over regex reassembly. This is the mechanism behind the Line Endings warning above, and it is worth naming because the corruption is invisible in a rendered diff. +- **Never edit an active `.code-workspace` file.** A workspace file rewritten on disk can make VS Code reload the window, and a reload destroys the running agent session's context - the work in flight is lost with nothing to catch it, and the trigger is not fully characterized (an agent's edit has caused the reload where a human's identical edit did not). Surface the needed change for the maintainer to apply by hand. +- **A green check is not evidence the work happened.** A skipped job and a passing job are indistinguishable in the aggregated required check. When a job exists to exercise something, confirm from its log that it ran and produced the output it promises. +- **A workflow change is only fully exercised by CI.** Extracting a `run:` block and executing it locally validates the script and nothing else - `secrets: inherit`, `permissions:`, `needs:` wiring, and reusable-workflow inputs resolve only in a real run. +- **A review flags an instance - fix the class.** When a reviewer cites one stale claim, one silent-narrowing pattern, or one mis-worded contract, sweep for its siblings before replying. Reviewers sample - they do not enumerate. + ## PR Review Etiquette -> This "PR Review Etiquette" section is the provider-agnostic review-loop *contract*; the [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) "GitHub Copilot Review Runbook" implements its mechanics. Without both in-repo, an agent has no pointer to the reliable Copilot mechanics and falls back to ad-hoc (and known-broken) behavior. +> This "PR Review Etiquette" section is the provider-agnostic review-loop *contract* every fleet repo follows, alongside the [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) "GitHub Copilot Review Runbook" that implements it. Without both in-repo, an agent has no pointer to the reliable Copilot mechanics and falls back to ad-hoc (and known-broken) behavior. 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. @@ -171,50 +265,98 @@ Bring the user in when: Anti-pattern: don't keep flipping the code on the same style point. Flip the rule once and stick to the rule. -## Shared Configuration and Tooling - -- **Config files.** [`.editorconfig`](./.editorconfig) (per-file-type EOL plus the C# / ReSharper style block), [`.gitattributes`](./.gitattributes) (`* -text`, with the `.husky/pre-commit` LF pin), [`.markdownlint-cli2.jsonc`](./.markdownlint-cli2.jsonc), and [`CODESTYLE.md`](./CODESTYLE.md) hold the repo's formatting, linting, and code-style rules. Keep [`.github/copilot-instructions.md`](./.github/copilot-instructions.md) narrow (Copilot / VS Code review mechanics plus the commit/PR-title summary); project-specific conventions live in this file and the architecture deep-dive in [`ARCHITECTURE.md`](./ARCHITECTURE.md). -- **Clean-compile tasks.** [`.vscode/tasks.json`](./.vscode/tasks.json) defines the canonical `.NET Build`, `CSharpier Format`, and `.NET Format` tasks (the last chains the first two then `dotnet format style --verify-no-changes`); their names are owned by the `CODESTYLE.md` ".NET" section - do not loosen them. Husky.Net runs the same checks as a local pre-commit hook, and CI's `lint` job is the authoritative backstop. -- **`dotnet format style` gates at info severity, and its auto-fixes bite.** The pre-commit `.NET Format` task runs `dotnet format style --verify-no-changes --severity=info`, so info-level IDE analyzers gate the commit; CSharpier alone is not enough. Two to watch: `IDE0072` populates an enum `switch` expression with `throw new NotImplementedException()` for unlisted members - a `_ =>` discard arm does **not** satisfy it, so map values with explicit arms or a ternary chain instead of relying on the fixer; and `IDE0046` rejects `if (!cond) return false; return expr;` - write the combined `return cond && expr;` form. Run the `.NET Format` task before committing to surface these, and review any file it rewrote rather than staging it blind. -- **Brownfield analyzer relaxations.** `Directory.Build.props` sets strict analysis; because this is a pre-existing console app, a specific set of analyzer rules are relaxed to suggestion in [`.editorconfig`](./.editorconfig), each documented inline. Prefer fixing new violations over adding relaxations. -- **Spell check.** The cspell word list and path exclusions live in [`cspell.json`](./cspell.json), the single source shared by the editor and CI. Do not keep a parallel word list in the `.code-workspace` file. -- **Run CI CLI tooling via Docker.** The linters CI uses (actionlint, markdownlint-cli2, shellcheck, cspell, etc.) need not be installed on the host - run them from their official images (e.g. `docker run --rm -v "$PWD:/repo" -w /repo rhysd/actionlint`) to reproduce a CI check locally before pushing. -- **Release notes.** Keep a short summary in [`README.md`](./README.md) and the full history in [`HISTORY.md`](./HISTORY.md); update both when cutting a release. `README.md` carries the summary for the **current version only** - when bumping the version, replace the previous version's summary rather than appending; prior versions live in `HISTORY.md`. - ## Communicating with the User -- **Reference every pull request as a clickable link.** When you mention a PR - in chat, a summary, or a report - render it as a markdown link to the PR (`[#123](https://github.com///pull/123)`), never a bare `#123`. The same applies to issues and commits. +- **Reference every pull request as a clickable link.** When you mention a PR - in chat, a summary, or a report - render it as a markdown link to the PR (`[#123](https://github.com/OWNER/REPO/pull/123)`), never a bare `#123`. The same applies to issues and commits. - **Ask for input as a numbered list.** When you need the user to decide or answer, present the questions - and any options - as a numbered list so they can reply per number. A single inline question is fine; two or more are always numbered. ## Workflow YAML Conventions -The conventions for everything under [`.github/workflows/`](./.github/workflows/) - action pinning, file/workflow/job/step naming, concurrency, shells, conditionals, boolean inputs, permissions, artifact handling, Docker layer cache, and release tagging - are specified in [`WORKFLOW.md`](./WORKFLOW.md), the canonical CI/CD guide. New and modified workflows must respect it; do not duplicate those rules here. +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. Sweep PRs that apply a rule everywhere are welcome when a rule changes. + +- **Action pinning**: pin **every** action - first-party (`actions/*`) and third-party - to a commit SHA with a trailing `# vX.Y.Z` comment, so Renovate / 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. Documented exception (no SHA pin at all): `dotnet/nbgv` is consumed via `@master` because the upstream tag stream lags `master` substantially and Dependabot's tag-tracking would propose a downgrade. **This applies to repo-owned build-layer leaves too** - a leaf owning its build specifics is not a reason to use floating tags; Dependabot still bumps SHA pins (updating the SHA + version comment). +- **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 PyPI library 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"** - including the PR-gate aggregator, whose `name:` is a required-status-check `context:` in a branch ruleset (`Check pull request workflow status job` in `test-pull-request.yml`). A ruleset-bound job's `name:` and its ruleset `context:` are the **same string**: rename them **together** - update the live ruleset and `repo-config/{develop,main}.json` in lockstep with the job `name:`, never one without the other, or required-status-check enforcement silently breaks. There is no un-suffixed exception. +- **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 exceptions** (both record the rationale inline in their header comment): (1) [`merge-bot-pull-request.yml`](./.github/workflows/merge-bot-pull-request.yml) uses `cancel-in-progress: false` because its three-job model (enable-auto-merge on opened, disable-auto-merge on maintainer-pushed synchronize, with method dispatched by base) requires each event to run to completion in arrival order - cancellation would leave auto-merge in an inconsistent state. (2) [`publish-release.yml`](./.github/workflows/publish-release.yml) uses both a **global, ref-independent group** (`group: ${{ github.workflow }}`, dropping the usual `-${{ github.ref }}`) and `cancel-in-progress: false`. It publishes shared ref-independent artifacts (both branches' Docker tags/caches and GitHub releases) on schedule/dispatch 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-push; and cancelling a publish mid-flight can leave a partially pushed tag set or a half-created release. The global group + queueing serializes every publish run to completion. +- **Shells**: every bash surface - a multi-line `run:` block and every committed `.sh` script alike - starts with `set -Eeuo pipefail`: fail fast, fail on undefined vars, fail on a failed pipe segment, and let an `ERR` trap inherit into functions, subshells, and command substitutions (`-E`). The `-E` is defense in depth: the fleet ships no `ERR` trap today, so a script that later adds one inherits the behavior instead of silently losing it. +- **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' }}`. +- **Validate input/state consistency at entry, fail fast**: when a workflow's inputs must satisfy a cross-input or input-versus-derived-state invariant (e.g. the release branch must match the computed version's prerelease status, or two inputs are mutually exclusive), assert it **once** in a dedicated entry validation step/job that the downstream jobs `needs:`, before any expensive build or publish work - not as partial checks scattered deep in later jobs. One gate that fails fast with a clear `::error::` beats a late or one-directional check. Examples: `build-release-task.yml`'s `validate-release` job (branch-versus-prerelease, both directions) and `publish-docker-readme-task.yml`'s "Validate inputs step". +- **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**: workflow artifacts are an intra-run handoff only - durable copies live on the GitHub release, not in workflow artifacts - so they must not survive the run and accumulate against the small account-wide artifact-storage quota. **Clean up each transfer artifact surgically at its point of consumption**: the job that downloads it deletes it by exact name/pattern right after consuming it (the `github-release` job deletes `release-asset--*` after attaching them to the release; `publish-release.yml`'s `publish-pypi` deletes `pypilibrary-build-` after publishing). Deletion needs `actions: write` granted on that job - for a reusable callee (e.g. `github-release` inside `build-release-task.yml`) the **caller** grants it (`publish-release.yml`'s `publish` job does). **Never blanket-delete the run's artifacts** (`gh api .../artifacts --jq '.artifacts[].id'`) - that also destroys diagnostic/log artifacts and the build-records actions emit automatically (`docker/build-push-action`'s `.dockerbuild`), which are exactly what you need to debug a failed run. Set `retention-days: 1` on **every** explicit `upload-artifact`: it is the failure-path backstop - a job that dies before its consumer runs leaves its artifact, reaped within a day - so no separate terminal cleanup job is needed. A repo customizing these jobs must preserve the consume-then-delete shape. +- **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. A **multi-image** repo uses a **per-image** buildcache tag (`:buildcache-` for each image, plus the base image's own tag and inline cache); it does not fall back to `type=gha` for the extra images. +- **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` (which may differ from the exact commit NBGV versioned) and not a branch name (a moving ref that a mid-run commit could advance past the built tree). + +### Running the Linters Locally (Known-Working Invocations) + +CI runs the full lint set, but run the linters locally before pushing to catch issues early, so an agent must know how to invoke them. Their non-Docker install paths (curl-pipe installers, global npm) are frequently blocked in sandboxes or fail on WSL, so **prefer the Docker invocations below, the known-working path that needs no local toolchain.** These tools auto-discover their targets from the working directory. + +**Each surface runs the lint with the tool that fits it, all from the same config files** (`.markdownlint-cli2.jsonc`, `cspell.json`, `.editorconfig`): + +- **CI (authoritative)** runs **markdownlint-cli2**, **cspell**, and **actionlint** as pinned action wrappers (Dependabot bumps them), plus **editorconfig-checker** via Docker `:latest` (its action only installs the CLI, so the Docker one-liner is what actually runs the check). markdownlint covers all `**/*.md`; **cspell is scoped to `README.md` + `HISTORY.md`** (see [CODESTYLE.md](./CODESTYLE.md) "Markdown and Spelling" for why), matching the cspell one-liner below. +- **The `.husky/pre-commit` hook** runs **language formatting only** - CSharpier + `dotnet format` (or ruff) via native tooling, no Docker and no doc linters, so it stays fast. +- **The VS Code Lint tasks** run the full doc-lint set via Docker `:latest` on demand, the local surface for Markdown, spelling, workflow, and EditorConfig checks. + +The Docker invocations below are the same ones the VS Code tasks use, for ad-hoc or headless (agent) runs. + +- **editorconfig-checker** (line endings + charset across the tree): + + ```sh + docker run --rm --pull=always -v "$PWD":/check --workdir /check mstruebing/editorconfig-checker:latest + ``` + +- **actionlint** (GitHub Actions workflow YAML - run after any `.github/workflows/` edit, since workflow-only changes are not smoke-built): + + ```sh + docker run --rm --pull=always -v "$PWD":/repo --workdir /repo rhysd/actionlint:latest -color + ``` + + The `rhysd/actionlint` image bundles `shellcheck`, so it also validates `run:` shell blocks. The direct-binary/curl-installer path is often sandbox-blocked - use Docker. + +- **markdownlint-cli2** (Markdown - mirrors the davidanson VS Code extension via the shared [`.markdownlint-cli2.jsonc`](./.markdownlint-cli2.jsonc), so the CLI and IDE agree): + + ```sh + docker run --rm --pull=always -v "$PWD":/workdir --workdir /workdir davidanson/markdownlint-cli2:latest "**/*.md" + ``` + +- **cspell** (spelling in user-facing docs; word list + exclusions in [`cspell.json`](./cspell.json)): + + ```sh + docker run --rm --pull=always -v "$PWD":/workdir --workdir /workdir ghcr.io/streetsidesoftware/cspell:latest --no-progress README.md HISTORY.md + ``` + + In a configured editor the davidanson extension is enough; use the Docker CLI when there's no IDE (agent/headless) or to confirm a clean run before pushing. + +When pulling a public image fails on a Docker-Desktop/WSL credential-helper error (`docker-credential-desktop.exe: exec format error`), retry with an empty Docker config: `DOCKER_CONFIG=$(mktemp -d) docker run ...` after writing `{}` to `$DOCKER_CONFIG/config.json`. + +## Supported Development Platforms -## Logging Conventions +- **Cross-platform by default - Windows + macOS + Linux.** Linux runs natively (a Linux desktop, or SSH/remote into a Linux host), through a devcontainer on Windows or macOS, or through WSL2 on Windows - the devcontainer and WSL routes carry their own nuances (mounts, path translation, SSH-agent forwarding) but deliver the same toolchain. Editing is cross-platform through the GUI regardless of where code runs. Assume this default. +- **A repo's platform ceiling is set by its dependencies, not tooling effort; decide it per repo before writing dev tooling.** Narrow below the default only for a hard runtime ceiling - the code can only execute or test on one platform (e.g. a Home Assistant integration is Linux-only: HA Core has POSIX-only dependencies and will not run natively on Windows, so even maximal tooling yields only lint-only there). The narrowing axis is where code *executes* for dev and testing - native, SSH-remote, container, or CI - never where editing happens. +- **Record a narrowed platform and its reason in the repo** (README/AGENTS) so the restriction reads as a deliberate dependency ceiling, not an omission. -Serilog log levels describe the **nature** of an event, applied uniformly across the whole app - never "which command am I in". When adding or reviewing a log call, pick the level from what the event *is*, and keep the pipeline reading as a coherent story: *inspect -> decide to act -> do the work -> call the tool -> succeed or fail*. +## Devcontainer -- **Error** - an operation failed and could not complete (tool returned non-zero, IO/parse/verify failure, a step that aborts the file). Every early-exit failure path. -- **Warning** - the **trigger**: the orchestration layer inspected the file, interpreted the result, and has **decided to modify the media** (or detected a noteworthy non-fatal condition - unknown codec, cover art, language fallback, non-convergent repair, an interruption). Emitted **once**, at the decision point, *before* the modification. This is the event that elevates the per-file log from Warning to Information (see `PerFileLogLevel`), so `--loglevel Warning` shows every file that gets changed and why. - - A "modification" is a write to the **media file**, including in-place metadata edits (MkvPropEdit flags/language/title) and container remuxes/renames. Sidecar cache writes and the results file are bookkeeping, not media modifications - they are Debug/Information, not Warnings. - - **The media-manipulation code itself does not emit Warning.** Doing a remux or re-encode is that code's job, not a warning. Only the decision to run it is the Warning. Do not sprinkle Warnings through `Convert`, the media-tool wrappers, or the worker methods. -- **Information** - the high-level narrative of what the app is doing, readable end to end at the default level with no low-level mechanics: startup (banner, settings, tool versions), discovery (`Discovered N files`), batch lifecycle (`Starting {Command}, processing N files`, progress, `Completed`, the run summary), the per-file entry, read-only outcomes of note (skips), a worker **doing its job** (e.g. `Convert.ReMux` logging `Remux using MkvMerge`), and the intended output of read-only commands (`getmediainfo` / `getsidecarinfo` / `gettagmap` dumps). -- **Debug** - troubleshooting detail; *how* the work is done: raw tool invocations and command lines (`Executing MkvMerge : GetMediaPropsJson : args`, which carry the operation so a per-method "doing X" line is not needed), read/probe mechanics (`Reading media info from sidecar`, temp files, packet probes), per-track structural dumps during normal processing, inspection sub-steps (verify, bitrate, idet counting), and sidecar cache bookkeeping. -- **Verbose** - very granular: filesystem-watcher events, per-packet/byte-level progress. +This repo ships no committed devcontainer. Development is native on the .NET 10 SDK plus the external media tools PlexCleaner orchestrates (FFmpeg, HandBrake, MkvToolNix, MediaInfo, 7-Zip). The multi-arch [Docker image](./Docker/) bundles those tools and doubles as a ready-made environment. The toolchain and the clean-compile tasks are documented in [`CODESTYLE.md`](./CODESTYLE.md). -The elevation trigger (Warning) must be preserved: keep exactly one decision-Warning per media modification, with the action at Information and the underlying tool at Debug. +## Editor and Tasks -### Tool execution and failure logging +- **VS Code is the primary IDE, and the experience favors it.** Prefer VS Code tasks and launch configurations for building, running, and testing over ad-hoc shell scripts; a script is the fallback, not the default. +- The `.code-workspace` file carries the shared editor settings and the recommended-extension set. **All VS Code settings and extension recommendations live only here, never in a standalone `.vscode/settings.json` or `.vscode/extensions.json`** (`.vscode/` holds only `tasks.json` and `launch.json`). A **standard set** of extensions applies to every repo (markdownlint, cspell, editorconfig, markdown-all-in-one, better-todo-tree, github-actions, actionlint, shellcheck, claude-code); **language-specific** extensions are added per project (.NET: csdevkit, csharpier; Python: python, pylance, ruff, mypy; Docker: the Docker extension). +- The Table of Contents is maintained by the Markdown All in One extension; `markdown.extension.toc.levels` in the workspace sets which heading levels it includes (see the Markdown rules for the authoring convention and the `` exclusion marker). +- **Agents: editing the active `.code-workspace` can reload the VS Code window and drop the agent's session.** Commit all state first, prefer opening the folder rather than the workspace while editing it, or leave workspace edits to the maintainer (a maintainer edit does not reload). -- **Always consume a tool's output.** A subprocess whose stdout/stderr is not read can deadlock once it fills the pipe buffer, so never run a tool without consuming its pipes: `MediaTool.Execute` buffers them (summarize when the output is huge), and `ExecuteStreamStdErr` streams stderr line by line for the unbounded `-f null` verify pass. `Execute`, its cancellation path, and `LogFailedResult` record the **operation** (the calling method, captured via `[CallerMemberName]`, rendered with `:l`) so a command line ties to its purpose in a parallel log without correlating separate lines. -- **Tools write errors to different streams.** ffmpeg, ffprobe, HandBrake, and 7-Zip use **stderr**; the mkvtoolnix tools (mkvmerge, mkvpropedit) write everything including errors to **stdout** (confirmed from the mkvtoolnix source - all output goes through the one stdout object) and override `GetErrorOutput` to it. MediaInfo also emits to stdout but keeps the stderr default; its errors are caught by the `LogFailedResult` fallback, which reads the other captured stream when the tool's declared stream is empty, so an error is never lost. -- **Do not add a per-method debug line that just restates the command about to run** (e.g. `Getting media info`); the `Executing {Tool} : {operation} : args` line from `Execute` already covers it. +## Repository Details -### Failure-handling philosophy +Every repo's GitHub repository details (the About panel) follow a fixed convention so the fleet stays consistent and self-describing. -An **expected, recoverable** failure escalates through the standard repair tiers (detect -> surgical -> remux -> re-encode -> fail); an **unexpected or logic** failure (e.g. tool output that will not parse) aborts the file and stays a hard error, so the bug surfaces and gets fixed rather than being masked by a fallback that silently mis-processes at scale. +- **Description** matches the README's first non-empty line after the `#` H1 heading, as plain text - strip markdown links (`[text](url)` and `[text][ref]` become `text`) since a description is not rendered. The README is the source of truth: set the description from it (`gh api -X PATCH repos// -f description=...`), never the reverse. When the current description is *more specific* than the README (a chip revision or variant the README omits), surface the drift to the maintainer rather than silently discarding the detail - the fix is to sharpen the README so the description follows it. Keep the line at most **100 characters** - Docker Hub's short-description cap, the tightest surface it feeds. For a repo that publishes a Docker image, the **Docker Hub short description** mirrors the same README intro line, so one canonical sentence carries to the README, the About panel, and Docker Hub alike. +- **Topics** are optional; any that are present match the repo's actual content. Do not invent topics to fill the field. +- **Include in the home page**: Releases on, Deployments off, Packages off. These toggles are UI-only - the REST and GraphQL APIs neither read nor write them - so they are set by hand and cannot be audited through `gh`. -## Project Structure +## Repository Layout - **PlexCleaner** (`PlexCleaner/PlexCleaner.csproj`) - The CLI application - orchestrates FFmpeg, HandBrake, MkvToolNix, MediaInfo, and 7-Zip to optimize media for Direct Play. diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 49451373..3ffb93e8 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -75,6 +75,14 @@ All external process execution uses [CliWrap](https://github.com/Tyrrrz/CliWrap) - See `MediaTool.cs` for base execution patterns - All tool execution supports cancellation via `Program.CancelToken()` +### Runtime Metrics + +`Metrics.cs` owns a single `System.Diagnostics.Metrics.Meter` (`PlexCleaner.Process`) published for the whole process and read externally with `dotnet-counters` (no config flag, and instruments are inert until observed). + +- Hooks: `ProcessDriver.ProcessFiles` (the choke point every command and monitor cycle funnels through) drives the file/byte/in-flight instruments and the operation-weighted `progress.ratio`. `Process.ProcessFiles` records the per-`SidecarFile.StatesType` outcomes. `MediaTool` execution paths record `tool.duration`. +- Progress is operation-weighted, not file count: each heavy full-file operation counts the file's size as work to do when it starts and as work done when it finishes, so the total grows as operations are discovered rather than being summed up front. +- Run-scoped gauges (totals, in-flight, progress, ETA) reset per `ProcessFiles` call, while the counters stay cumulative for the process. All writers use `Interlocked`, so the parallel loop needs no lock, and observable-gauge callbacks only read. Tags are bounded (`state`, `tool`) - no filename tags. + ### Sidecar File System Critical performance feature - DO NOT break compatibility: @@ -230,6 +238,30 @@ For formatter, EditorConfig, pre-commit hooks, line endings, and charset details - Lock-based synchronization: `Lock` instances for collection access - Cancellation: Global `CancellationTokenSource` accessed via `Program.CancelToken()` +## Logging Conventions + +Serilog log levels describe the **nature** of an event, applied uniformly across the whole app - never "which command am I in". When adding or reviewing a log call, pick the level from what the event *is*, and keep the pipeline reading as a coherent story: *inspect -> decide to act -> do the work -> call the tool -> succeed or fail*. + +- **Error** - an operation failed and could not complete (tool returned non-zero, IO/parse/verify failure, a step that aborts the file). Every early-exit failure path. +- **Warning** - the **trigger**: the orchestration layer inspected the file, interpreted the result, and has **decided to modify the media** (or detected a noteworthy non-fatal condition - unknown codec, cover art, language fallback, non-convergent repair, an interruption). Emitted **once**, at the decision point, *before* the modification. This is the event that elevates the per-file log from Warning to Information (see `PerFileLogLevel`), so `--loglevel Warning` shows every file that gets changed and why. + - A "modification" is a write to the **media file**, including in-place metadata edits (MkvPropEdit flags/language/title) and container remuxes/renames. Sidecar cache writes and the results file are bookkeeping, not media modifications - they are Debug/Information, not Warnings. + - **The media-manipulation code itself does not emit Warning.** Doing a remux or re-encode is that code's job, not a warning. Only the decision to run it is the Warning. Do not sprinkle Warnings through `Convert`, the media-tool wrappers, or the worker methods. +- **Information** - the high-level narrative of what the app is doing, readable end to end at the default level with no low-level mechanics: startup (banner, settings, tool versions), discovery (`Discovered N files`), batch lifecycle (`Starting {Command}, processing N files`, progress, `Completed`, the run summary), the per-file entry, read-only outcomes of note (skips), a worker **doing its job** (e.g. `Convert.ReMux` logging `Remux using MkvMerge`), and the intended output of read-only commands (`getmediainfo` / `getsidecarinfo` / `gettagmap` dumps). +- **Debug** - troubleshooting detail; *how* the work is done: raw tool invocations and command lines (`Executing MkvMerge : GetMediaPropsJson : args`, which carry the operation so a per-method "doing X" line is not needed), read/probe mechanics (`Reading media info from sidecar`, temp files, packet probes), per-track structural dumps during normal processing, inspection sub-steps (verify, bitrate, idet counting), and sidecar cache bookkeeping. +- **Verbose** - very granular: filesystem-watcher events, per-packet/byte-level progress. + +The elevation trigger (Warning) must be preserved: keep exactly one decision-Warning per media modification, with the action at Information and the underlying tool at Debug. + +### Tool execution and failure logging + +- **Always consume a tool's output.** A subprocess whose stdout/stderr is not read can deadlock once it fills the pipe buffer, so never run a tool without consuming its pipes: `MediaTool.Execute` buffers them (summarize when the output is huge), and `ExecuteStreamStdErr` streams stderr line by line for the unbounded `-f null` verify pass. `Execute`, its cancellation path, and `LogFailedResult` record the **operation** (the calling method, captured via `[CallerMemberName]`, rendered with `:l`) so a command line ties to its purpose in a parallel log without correlating separate lines. +- **Tools write errors to different streams.** ffmpeg, ffprobe, HandBrake, and 7-Zip use **stderr**; the MkvToolNix tools (mkvmerge, mkvpropedit) write everything including errors to **stdout** (confirmed from the MkvToolNix source - all output goes through the one stdout object) and override `GetErrorOutput` to it. MediaInfo also emits to stdout but keeps the stderr default; its errors are caught by the `LogFailedResult` fallback, which reads the other captured stream when the tool's declared stream is empty, so an error is never lost. +- **Do not add a per-method debug line that just restates the command about to run** (e.g. `Getting media info`); the `Executing {Tool} : {operation} : args` line from `Execute` already covers it. + +### Failure-handling philosophy + +An **expected, recoverable** failure escalates through the standard repair tiers (detect -> surgical -> remux -> re-encode -> fail); an **unexpected or logic** failure (e.g. tool output that will not parse) aborts the file and stays a hard error, so the bug surfaces and gets fixed rather than being masked by a fallback that silently mis-processes at scale. + ## Common Patterns ### Command-Line Parsing @@ -352,6 +384,7 @@ Two-phase model - reusable `*-task.yml` workflows orchestrated by two entry poin - 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`. Most thread a required `branch` input (config keys off it, never `github.ref_name`) plus `ref`/`smoke`. Exception: `build-datebadge-task.yml` takes no `branch` input - it's caller-gated (the publisher invokes it only when `main` is published), since the badge tracks the last `main` build and has no per-branch context. - 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`). +- Release notes: keep a short current-version summary in `README.md` and the full history in `HISTORY.md`, updating both when cutting a release. `README.md` carries only the current version's summary - when bumping the version, replace the previous summary rather than appending, and prior versions live in `HISTORY.md`. ### Docker diff --git a/CODESTYLE.md b/CODESTYLE.md index 2c443293..f7bf29f8 100644 --- a/CODESTYLE.md +++ b/CODESTYLE.md @@ -28,14 +28,15 @@ Each language defines a **clean-compile** verification - the combination of buil 2. The **owning project's local config** when the exception is project-wide for one project (e.g. a test project's own `.editorconfig` / `pyproject.toml`). 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 mechanics (which attribute, which config key) are in the .NET section. +- **`dotnet format style --verify-no-changes --severity=info` gates the commit, and two IDE fixers bite.** CSharpier alone is not enough - info-level IDE analyzers run in the `.NET Format` verify. `IDE0072` wants every enum member listed in a `switch` expression, and a `_ =>` discard arm does **not** satisfy it, so map values with explicit arms or a ternary chain rather than leaning on the fixer's `throw new NotImplementedException()`. `IDE0046` rejects `if (!cond) return false; return expr;` - write the combined `return cond && expr;`. Run the `.NET Format` task before committing to surface these, and review any file it rewrote rather than staging it blind. ### 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. +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, `MD060` table pipe spacing) are **intentional** - do not "fix" them. `MD033` (inline HTML) stays enabled: HTML comments such as the reference-link group dividers pass it, and elements are flagged so native markdown wins. 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. -3. **Spelling CI scope**: The enforced CI spell-check gate covers **`README.md` and `HISTORY.md` only** - these are the files every repo visitor sees, so they must be clean. It is deliberately **not** all `**/*.md`: repos carry many markdown files full of technical terms, and gating every one of them would mean endlessly padding `cspell.json` just to keep CI green. Broad, live spell-checking across any file (source, markdown, text) is the **cspell editor extension's** job, so typos still surface to whoever is editing. A repo owner **may** widen their own CI file list, but the template ships README + HISTORY as the default; keep every surface that runs cspell - the CI workflow and any local VS Code task or one-liner the repo has - on the same file list. The list is explicit (not a glob), so a repo that ships no `HISTORY.md` (e.g. one with no changelog) must drop it from all three surfaces and gate on `README.md` alone - cspell errors on a listed file that does not exist. Markdown *linting* (item 1) stays repo-wide `**/*.md` - it does not choke on technical terms. +3. **Spelling CI scope**: The enforced CI spell-check gate covers **`README.md` and `HISTORY.md` only** - these are the files every repo visitor sees, so they must be clean. It is deliberately **not** all `**/*.md`: repos carry many markdown files full of technical terms, and gating every one of them would mean endlessly padding `cspell.json` just to keep CI green. Broad, live spell-checking across any file (source, markdown, text) is the **cspell editor extension's** job, so typos still surface to whoever is editing. A repo owner **may** widen their own CI file list, but README + HISTORY are the default; keep every surface that runs cspell - the CI workflow and any local VS Code task or one-liner the repo has - on the same file list. The list is explicit (not a glob), so a repo that ships no `HISTORY.md` (e.g. one with no changelog) must drop it from all three surfaces and gate on `README.md` alone - cspell errors on a listed file that does not exist. Markdown *linting* (item 1) stays repo-wide `**/*.md` - it does not choke on technical terms. ## .NET diff --git a/Directory.Packages.props b/Directory.Packages.props index 4a6153cd..0e4c7bcf 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -1,13 +1,13 @@ - + - - + + diff --git a/Docker/Dockerfile b/Docker/Dockerfile index c37a6ed6..84ff1adc 100644 --- a/Docker/Dockerfile +++ b/Docker/Dockerfile @@ -37,7 +37,7 @@ ARG TARGETPLATFORM \ BUILDPLATFORM # PlexCleaner build attribute configuration -ARG BUILD_CONFIGURATION="Debug" \ +ARG BUILD_CONFIGURATION="Release" \ BUILD_VERSION="1.0.0.0" \ BUILD_FILE_VERSION="1.0.0.0" \ BUILD_ASSEMBLY_VERSION="1.0.0.0" \ @@ -134,6 +134,9 @@ COPY --chmod=ug=rwx,o=rx ./Docker/InstallDebugTools.sh ./ RUN ./InstallDebugTools.sh \ && rm -rf ./InstallDebugTools.sh +# Wrapper to read the PlexCleaner.Process metrics: "docker exec counters" +COPY --chmod=ug=rwx,o=rx ./Docker/counters.sh /usr/local/bin/counters + # Copy version script COPY --chmod=ug=rwx,o=rx ./Docker/Version.sh /PlexCleaner/ diff --git a/Docker/README.md b/Docker/README.md index 980e25d6..d9321d64 100644 --- a/Docker/README.md +++ b/Docker/README.md @@ -4,21 +4,32 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. ## Documentation -Refer to the [project page](https://github.com/ptr727/PlexCleaner) for complete usage and configuration. +Refer to the [project page][github-link] for complete usage and configuration. -- **Source Code**: [GitHub](https://github.com/ptr727/PlexCleaner) - source code, issues, and CI/CD pipelines. -- **Binary Releases**: [GitHub Releases](https://github.com/ptr727/PlexCleaner/releases) - pre-compiled executables for Windows, Linux, and macOS. -- **Docker Images**: [Docker Hub](https://hub.docker.com/r/ptr727/plexcleaner) - container images with all tools pre-installed. +- **Source Code**: [GitHub][github-link] - source code, issues, and CI/CD pipelines. +- **Binary Releases**: [GitHub Releases][releases-link] - pre-compiled executables for Windows, Linux, and macOS. +- **Docker Images**: [Docker Hub][docker-link] - container images with all tools pre-installed. ## Docker Tags Images are rebuilt weekly to pick up upstream base-image and tool updates. -- `latest`: built from the release [main branch](https://github.com/ptr727/PlexCleaner/tree/main). Multi-architecture (`linux/amd64`, `linux/arm64`) on the `ubuntu:rolling` base. -- `develop`: built from the pre-release [develop branch](https://github.com/ptr727/PlexCleaner/tree/develop). +- `latest`: built from the release [main branch][main-branch-link]. Multi-architecture (`linux/amd64`, `linux/arm64`) on the `ubuntu:rolling` base. +- `develop`: built from the pre-release [develop branch][develop-branch-link]. - `X.Y.Z`: a specific released version (SemVer2 tag). ## License -Licensed under the [MIT License](https://github.com/ptr727/PlexCleaner/blob/main/LICENSE).\ -![GitHub License](https://img.shields.io/github/license/ptr727/PlexCleaner) +Licensed under the [MIT License][license-link].\ +![GitHub License][license-shield] + + +[license-shield]: https://img.shields.io/github/license/ptr727/PlexCleaner + + +[develop-branch-link]: https://github.com/ptr727/PlexCleaner/tree/develop +[docker-link]: https://hub.docker.com/r/ptr727/plexcleaner +[github-link]: https://github.com/ptr727/PlexCleaner +[license-link]: https://github.com/ptr727/PlexCleaner/blob/main/LICENSE +[main-branch-link]: https://github.com/ptr727/PlexCleaner/tree/main +[releases-link]: https://github.com/ptr727/PlexCleaner/releases diff --git a/Docker/counters.sh b/Docker/counters.sh new file mode 100644 index 00000000..d21ade22 --- /dev/null +++ b/Docker/counters.sh @@ -0,0 +1,17 @@ +#!/bin/bash + +# Run dotnet-counters against the in-container PlexCleaner process. +# The single-file tool needs a writable extract dir, and the app is PID 1 in the container. +# Usage from the host: +# docker exec counters # live monitor of the PlexCleaner.Process meter +# docker exec counters collect ... # any other dotnet-counters verb/args, passed through + +set -Eeuo pipefail + +export DOTNET_BUNDLE_EXTRACT_BASE_DIR="${DOTNET_BUNDLE_EXTRACT_BASE_DIR:-/tmp}" + +if [[ $# -eq 0 ]]; then + set -- monitor -p 1 --counters PlexCleaner.Process +fi + +exec /dotnet-tools/dotnet-counters "$@" diff --git a/HISTORY.md b/HISTORY.md index 2d4f954a..07b7925d 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -4,12 +4,19 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. ## Release History +- Version 3.22: + - Added always-on runtime metrics published via `System.Diagnostics.Metrics` under the `PlexCleaner.Process` meter, readable with `dotnet-counters` with no extra infrastructure. + - Overall progress is operation-weighted: each heavy full-file operation (the closed-caption and interlace scans, bitrate analysis, re-encode, deinterlace, and verify) counts the file's size as work to do when it starts and as work done when it finishes, so a run mixing tiny and huge files reports the actual work completed, and the total grows as the non-deterministic per-file path is discovered. + - Instruments include the run file total and input byte total, in-flight and active-thread counts, the `work.total`/`work.completed` byte gauges and the `progress.ratio` and `eta.seconds` derived from them, cumulative per-outcome counters (completed, modified, errors, verify-failed, and a per-`State`-flag tally), and the `file.duration` and per-tool `tool.duration` histograms. Metrics are aggregate only, with bounded `state` and `tool` tags and no filename tags. + - The run-scoped gauges reset at the start of every processing pass, so monitor mode and back-to-back commands each report their own run, while the counters stay cumulative for rate display. + - Instruments are inert until a listener observes them, so the feature is always on with no configuration flag and negligible idle overhead. + - The meter is OpenTelemetry and `dotnet-monitor` compatible, and the Docker image ships a `counters` wrapper, so reading the metrics is `docker exec counters`. - Version 3.21: - Repair non-monotonic DTS muxer warnings losslessly instead of failing repair permanently. - `ffmpeg -f null` can exit `0` yet emit `Application provided invalid, non monotonically increasing dts to muxer` for files that may decode and play correctly. - The previous "any stderr means failure" rule promoted this muxer-interleaving artifact to a hard `VerifyFailed`/`RepairFailed`, and a re-encode could not fix it because Matroska stores no DTS and ffmpeg re-derives a non-monotonic timeline on read. - Verify now classifies the decode diagnostics deterministically as clean, a timestamp-only failure, or a decode error; a timestamp-only failure is a repairable failure and everything else fails (fail-closed, so an unrecognized diagnostic fails as a decode error). - - The classification streams the output line by line, so memory stays bounded even when a file emits a warning per packet ([#827](https://github.com/ptr727/PlexCleaner/issues/827)). + - The classification streams the output line by line, so memory stays bounded even when a file emits a warning per packet ([#827][issue-827-link]). - Added a lossless timestamp repair as the first repair tier, escalating to remux and re-encode when it cannot apply. - When verification detects a demux-visible non-monotonic DTS on an audio stream, the audio packet timestamps are rewritten to be strictly monotonic using the `setts` bitstream filter with a stream copy (no re-encode), then re-verified. - A regression gate compares the per-stream coded payload hash and the per-stream start and duration before and after, discarding the result unless every stream is byte-identical and no stream shifted beyond the A/V-sync tolerance, so the lossless repair can neither alter the media nor drift the audio out of sync. @@ -38,7 +45,7 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. - Log a warning when a repair or cleanup condition is detected (redundant `Default` flags, invalid language tags, interlaced video, tracks needing re-encode, etc.) so `--loglevel Warning` surfaces every file that is modified; enable `--logelevate` to also see the subsequent cleanup steps for those files. - Always log the end-of-run summary regardless of the configured log level, so the modified, error, and verify-failed counts are recorded for every processing run. - Fixed a defect in `idet` interlace detection, beyond the logging changes: ffmpeg can emit its idet statistics more than once (an early empty pass before the final cumulative counts), which could cause the counts to parse incorrectly and the interlace detection operation to fail; the parser now matches every emitted statistics block and uses the final cumulative counts. - - Improved interlace detection reporting: `FindInterlacedTracks` is now a pure predicate that surfaces how interlacing was detected (idet scan versus container metadata flag), the interlaced verdict and its human-readable justification are encapsulated in a self-describing reason string, and dead idet reporting code that never fed the decision was removed ([#809](https://github.com/ptr727/PlexCleaner/issues/809), [#810](https://github.com/ptr727/PlexCleaner/issues/810)). + - Improved interlace detection reporting: `FindInterlacedTracks` is now a pure predicate that surfaces how interlacing was detected (idet scan versus container metadata flag), the interlaced verdict and its human-readable justification are encapsulated in a self-describing reason string, and dead idet reporting code that never fed the decision was removed ([#809][issue-809-link], [#810][issue-810-link]). - Handle the `SIGINT`, `SIGTERM`, and `SIGQUIT` termination signals (`docker stop`, `Ctrl+C`) so processing is interrupted gracefully and the summary and exit code are logged before exit. The custom `Ctrl+Q`/`Ctrl+Z` exit keys are removed in favor of the standard signals. - Normalize `Default` track flags instead of only warning about them: clear the flag on a lone track of a type, keep the preferred audio track as the single default when multiple are flagged, and clear all default flags on subtitle tracks. - Added a `custom` command that loads a user-provided plugin assembly implementing `IProcessPlugin` and runs it over the media files, reusing the file iteration and processing API for bespoke re-processing or repair. Includes the `MatroskaHeaderCleanup` example plugin. Not available in AOT builds. @@ -47,14 +54,14 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. - Reworked the CI/CD pipeline to a branch-scoped self-publishing model: a weekly scheduled run (and manual dispatch) publishes both `main` (stable, Docker `latest`) and `develop` (prerelease, Docker `develop`) - native executables, the multi-arch Docker image, and the GitHub release - while merges accumulate until the next run. No application changes. - Added `WORKFLOW.md` (the canonical CI/CD specification) and `repo-config/` (rulesets and repository settings as code). - Version 3.18: - - Fixed an infinite remux loop in `monitor` mode on files with unmappable IETF / BCP-47 language tags ([#747](https://github.com/ptr727/PlexCleaner/issues/747)). + - Fixed an infinite remux loop in `monitor` mode on files with unmappable IETF / BCP-47 language tags ([#747][issue-747-link]). - Invalid IETF language tags (e.g. `language_ietf` set to a value that cannot be resolved to ISO 639) are now set in place to a valid tag (the ISO 639 equivalent if known, else `und`) so the repair converges instead of remuxing the same file every cycle. - Added a non-convergence guard: if a repair (invalid language tags, metadata remux, or the Matroska structure check) does not resolve the detected errors, the file is marked `VerifyFailed` and is no longer re-processed, breaking the loop for any unfixable condition. - - Added a deterministic Direct Play verification check ([#746](https://github.com/ptr727/PlexCleaner/issues/746)). + - Added a deterministic Direct Play verification check ([#746][issue-746-link]). - Some Matroska files parse cleanly with FfProbe / MkvMerge / MediaInfo yet fail Direct Play in Jellyfin / Emby / Shield because the player cannot use the file's seek index (e.g. the Cues are positioned before the Tracks, which a forward-only reader cannot reach). - `Verify` now validates the Matroska seek index (the SeekHead and Cues a player needs for keyframe seeking) using the NEbml library, and remuxes only files whose index is missing or unusable, leaving valid files untouched. - - Quickscan accuracy: under `--quickscan` the limited sample is not representative, so interlace detection now skips the unreliable `idet` frame analysis (relying on container interlace flags only) and bitrate verification is skipped, avoiding false-positive deinterlacing of progressive content and unreliable bitrate-exceeded reports ([#749](https://github.com/ptr727/PlexCleaner/issues/749)). - - Interlace detection is more conservative to avoid deinterlacing progressive content: it uses idet's more reliable MultiFrame pass and the dominant field order, and only treats content as interlaced when interlaced frames outnumber progressive frames ([#749](https://github.com/ptr727/PlexCleaner/issues/749)). + - Quickscan accuracy: under `--quickscan` the limited sample is not representative, so interlace detection now skips the unreliable `idet` frame analysis (relying on container interlace flags only) and bitrate verification is skipped, avoiding false-positive deinterlacing of progressive content and unreliable bitrate-exceeded reports ([#749][issue-749-link]). + - Interlace detection is more conservative to avoid deinterlacing progressive content: it uses idet's more reliable MultiFrame pass and the dominant field order, and only treats content as interlaced when interlaced frames outnumber progressive frames ([#749][issue-749-link]). - Version 3.16: - Structural changes only, no functional changes. - Consolidated project structure, build configuration, CI/CD workflows, and Docker configuration across projects. @@ -66,48 +73,48 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. - Version 3.15: - This is primarily a code refactoring release. - Updated from .NET 9 to .NET 10. - - Added [Nullable types](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/nullable-value-types) support. - - Added [Native AOT](https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot) support. - - Replaced `JsonSchemaBuilder.FromType()` with `GetJsonSchemaAsNode()` as `FromType()` is [not AOT compatible](https://github.com/json-everything/json-everything/issues/975). - - Replaced `JsonSerializer.Deserialize()` with `JsonSerializer.Deserialize(JsonSerializerContext)` for generating [AOT compatible](https://learn.microsoft.com/en-us/dotnet/api/system.text.json.serialization.jsonserializercontext) JSON serialization code. + - Added [Nullable types][nullable-value-types-link] support. + - Added [Native AOT][native-aot-link] support. + - Replaced `JsonSchemaBuilder.FromType()` with `GetJsonSchemaAsNode()` as `FromType()` is [not AOT compatible][json-everything-issue-link]. + - Replaced `JsonSerializer.Deserialize()` with `JsonSerializer.Deserialize(JsonSerializerContext)` for generating [AOT compatible][jsonserializercontext-link] JSON serialization code. - Replaced `MethodBase.GetCurrentMethod()?.Name` with `[System.Runtime.CompilerServices.CallerMemberName]` to generate the caller function name during compilation. - - AOT cross compilation is [not supported](https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot/cross-compile) by the CI/CD pipeline and single file native AOT binaries can be [manually built](./README.md#aot) if needed. + - AOT cross compilation is [not supported][native-aot-cross-compile-link] by the CI/CD pipeline and single file native AOT binaries can be [manually built](./README.md#aot) if needed. - Changed MediaInfo output from `--Output=XML` using XML to `--Output=JSON` using JSON. - - Attempts to use `Microsoft.XmlSerializer.Generator` and generate AOT compatible XML parsing was [unsuccessful](https://stackoverflow.com/questions/79858800/statically-generated-xml-parsing-code-using-microsoft-xmlserializer-generator), while JSON `JsonSerializerContext` is AOT compatible. + - Attempts to use `Microsoft.XmlSerializer.Generator` and generate AOT compatible XML parsing was [unsuccessful][stackoverflow-xmlserializer-link], while JSON `JsonSerializerContext` is AOT compatible. - Parsing the existing XML schema is done with custom AOT compatible XML parser created for the MediaInfo XML content. - SidecarFile schema changed from v4 to v5 to account for XML to JSON content change. - Schema will automatically be upgraded and convert XML to JSON equivalent on reading. - - Using [`ArrayPool.Shared.Rent()`](https://learn.microsoft.com/en-us/dotnet/api/system.buffers.arraypool-1) vs. `new byte[]` to improve memory pressure during sidecar hash calculations. + - Using [`ArrayPool.Shared.Rent()`][arraypool-link] vs. `new byte[]` to improve memory pressure during sidecar hash calculations. - Removed `MonitorOptions` from the config file schema, default values do not need to be changed. - - ⚠️ Standardized on only using the Ubuntu [rolling](https://releases.ubuntu.com/) docker base image. + - ⚠️ Standardized on only using the Ubuntu [rolling][ubuntu-releases-link] docker base image. - No longer publishing Debian or Alpine based docker images, or images supporting `linux/arm/v7`. - The media tool versions published with the rolling release are typically current, and matches the versions available on Windows, offering a consistent experience, and requires less testing due to changes in behavior between versions. - Version 3.14: - Switch to using [CliWrap][cliwrap-link] for commandline tool process execution. - - Remove dependency on [deprecated](https://github.com/dotnet/command-line-api/issues/2576) `System.CommandLine.NamingConventionBinder` by directly using commandline options binding. + - Remove dependency on [deprecated][command-line-api-issue-2576-link] `System.CommandLine.NamingConventionBinder` by directly using commandline options binding. - Converted media tool commandline creation to using fluent builder pattern. - Converted FFprobe JSON packet parsing to using streaming per-packet processing using [Utf8JsonAsyncStreamReader][utf8jsonasync-link] vs. read everything into memory and then process. - Switched editorconfig `charset` from `utf-8-bom` to `utf-8` as some tools and PR merge in GitHub always write files without the BOM. - Improved closed caption detection in MediaInfo, e.g. discrete detection of separate `SCTE 128` tracks vs. `A/53` embedded video tracks. - Improved media tool parsing resiliency when parsing non-Matroska containers, i.e. added `testmediainfo` command to attempt parsing media files. - - Add [Husky.Net](https://alirezanet.github.io/Husky.Net) for pre-commit hook code style validation. + - Add [Husky.Net][husky-link] for pre-commit hook code style validation. - General refactoring. - Version 3.13: - - Escape additional filename characters for use with `ffprobe movie=filename[out0+subcc]` command. Fixes [#524](https://github.com/ptr727/PlexCleaner/issues/524). + - Escape additional filename characters for use with `ffprobe movie=filename[out0+subcc]` command. Fixes [#524][issue-524-link]. - Version 3:12: - Update to .NET 9.0. - ⚠️ Dropping Ubuntu docker `arm/v7` support as .NET for ARM32 is no longer published in the Ubuntu repository. - Switching Debian docker builds to install .NET using install script as the Microsoft repository now only supports x64 builds. (Ubuntu and Alpine still installing .NET using the distribution repository.) - Updated code style [`.editorconfig`](./.editorconfig) to closely follow the Visual Studio and .NET Runtime defaults. - - Set [CSharpier](https://csharpier.com/) as default C# code formatter. + - Set [CSharpier][csharpier-link] as default C# code formatter. - ⚠️ Removed docker [`UbuntuDevel.Dockerfile`](./Docker/Ubuntu.Devel.Dockerfile), [`AlpineEdge.Dockerfile`](./Docker/Alpine.Edge.Dockerfile), and [`DebianTesting.Dockerfile`](./Docker/Debian.Testing.Dockerfile) builds from CI as theses OS pre-release / Beta builds were prone to intermittent build failures. If "bleeding edge" media tools are required local builds can be done using the Dockerfile. - Updated 7-Zip version number parsing to account for newly [observed](./PlexCleanerTests/VersionParsingTests.cs) variants. - - EIA-608 and CTA-708 closed caption detection was reworked due to FFmpeg [removing](https://code.ffmpeg.org/FFmpeg/FFmpeg/commit/19c95ecbff84eebca254d200c941ce07868ee707) easy detection using FFprobe. + - EIA-608 and CTA-708 closed caption detection was reworked due to FFmpeg [removing][ffmpeg-commit-link] easy detection using FFprobe. - See the [EIA-608 and CTA-708 Closed Captions](./README.md#eia-608-and-cta-708-closed-captions) section for details. - Refactored the logic used to determine if a video stream should be considered to contain closed captions. - Detection may have been broken since the release of FFmpeg v7, it is possible that media files may be in the `Verified` state with closed captions being undetected, run the `removeclosedcaptions` command to re-detect and remove closed captions. - Interlace and Telecine detection is complicated and this implementation using track flags and `idet` is naive and may not be reliable, changed `DeInterlace` to default to `false`. - - Re-added `parallel` and `threadcount` option to `monitor` command, fixes [#498](https://github.com/ptr727/PlexCleaner/issues/498). + - Re-added `parallel` and `threadcount` option to `monitor` command, fixes [#498][issue-498-link]. - Added conditional checks for `ReMux` to warn when disabled and media must be modified for processing logic to work as intended, e.g. removing extra video streams, removing cover art, etc. - Added `quickscan` option to limit the scan duration and improve performance, at the potential cost of accuracy. - When `parallel` is enabled and `threadcount` is not specified, cap the default of 1/2 CPU cores to max 4, and cap set value to CPU count, prevents CPU starvation. @@ -126,19 +133,19 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. - If "bleeding edge" media tools are required consider using `ubuntu-devel` (based on `ubuntu:devel`), `alpine-edge` (based on `alpine:edge`) or `debian-testing` (based on `debian:testing-slim`) tags. - If you are currently using the `ptr727/plexcleaner:savoury` docker tag, please switch to `ptr727/plexcleaner:ubuntu`. - Version 3.9: - - Re-enabling Alpine Stable builds now that Alpine 3.20 has been [released](https://alpinelinux.org/posts/Alpine-3.20.0-released.html). - - No longer pre-installing VS Debug Tools in docker builds, replaced with [`DebugTools.sh`](./Docker//DebugTools.sh) script that can be used to install [VS Debug Tools](https://learn.microsoft.com/en-us/visualstudio/debugger/remote-debugging-dotnet-core-linux-with-ssh) and [.NET Diagnostic Tools](https://learn.microsoft.com/en-us/dotnet/core/diagnostics/tools-overview) if required. + - Re-enabling Alpine Stable builds now that Alpine 3.20 has been [released][alpine-release-link]. + - No longer pre-installing VS Debug Tools in docker builds, replaced with [`DebugTools.sh`](./Docker//DebugTools.sh) script that can be used to install [VS Debug Tools][remote-debugging-link] and [.NET Diagnostic Tools][dotnet-diagnostics-link] if required. - Version 3.8: - Added Alpine Stable and Edge, Debian Stable and Testing, and Ubuntu Rolling and Devel docker builds. - ⚠️ Removed ArchLinux docker build, only supported x64 and media tool versions were often lagging. - No longer using MCR base images with .NET pre-installed, support for new linux distribution versions were often lagging. - - Alpine Stable builds are still [disabled](https://github.com/ptr727/PlexCleaner/issues/344), waiting for Alpine 3.20 to be released, ETA 1 June 2024. + - Alpine Stable builds are still [disabled][issue-344-link], waiting for Alpine 3.20 to be released, ETA 1 June 2024. - Rob Savoury [announced][savoury-link] that due to a lack of funding Ubuntu Noble 24.04 LTS will not get PPA support. - Pinning `savoury` docker builds to Jammy 22.04 LTS. - Switching `latest` docker tag from `savoury` to an alias for `ubuntu` builds, i.e. the latest released version of Ubuntu, currently Noble 24.04 LTS. - Updated `savoury` docker builds to FfMpeg v7, currently the only docker build supporting FfMpeg v7. - Version 3.7: - - Added `ProcessOptions:FileIgnoreMasks` to support skipping (not deleting) sample files per [discussions request](https://github.com/ptr727/PlexCleaner/discussions/341). + - Added `ProcessOptions:FileIgnoreMasks` to support skipping (not deleting) sample files per [discussions request][discussion-341-link]. - Wildcard characters `*` and `?` are supported, e.g. `*.sample` or `*.sample.*`. - Wildcard support now also allows excluding temporary UnRaid FuseFS files, e.g. `*.fuse_hidden*`. - Settings JSON schema changed from v3 to v4. @@ -147,23 +154,23 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. - `ConvertOptions:FfMpegOptions:Output` has been deprecated, no need for user configurable values. - `ConvertOptions:FfMpegOptions:Global` no longer requires defaults values and will only be used during encoding, only add custom values for e.g. hardware acceleration, existing values will be converted. - E.g. `-analyzeduration 2147483647 -probesize 2147483647 -hwaccel cuda -hwaccel_output_format cuda` will be converted to `-hwaccel cuda -hwaccel_output_format cuda`. - - Changed JSON serialization from `Newtonsoft.Json` [to](https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/migrate-from-newtonsoft) .NET native `Text.Json`. + - Changed JSON serialization from `Newtonsoft.Json` [to][migrate-from-newtonsoft-link] .NET native `Text.Json`. - Changed JSON schema generation from `Newtonsoft.Json.Schema` [to][jsonschema-link] `JsonSchema.Net.Generation`. - Fixed issue with old settings schemas not upgrading as expected, and updated associated unit tests to help catch this next time. - - ⚠️ Disabling Alpine Edge builds, Handbrake is [failing](https://gitlab.alpinelinux.org/alpine/aports/-/issues/15979) to install, again. + - ⚠️ Disabling Alpine Edge builds, Handbrake is [failing][alpine-issue-15979-link] to install, again. - Will re-enable Alpine builds if Alpine 3.20 and Handbrake is stable. - Version 3.6: - Disabling Alpine 3.19 release builds and switching to Alpine Edge. - - Handbrake is only available on Edge, and mixing released and Edge versions cause too many [issues](https://gitlab.alpinelinux.org/alpine/aports/-/issues/15949). + - Handbrake is only available on Edge, and mixing released and Edge versions cause too many [issues][alpine-issue-15949-link]. - Alpine stable release builds will no longer be built, or not until Handbrake is supported on stable releases (v3.20 May 2024). - Alpine Edge builds will be tagged as `alpine-edge`. - Version 3.5: - - Download 7-Zip builds from [GitHub](https://github.com/ip7z/7zip/releases), fixes [#324](https://github.com/ptr727/PlexCleaner/issues/324). + - Download 7-Zip builds from [GitHub][sevenzip-releases-link], fixes [#324][issue-324-link]. - Update Alpine Docker image to 3.19. - Version 3.4: - - Updated to [.NET 8.0](https://learn.microsoft.com/en-us/dotnet/core/whats-new/dotnet-8). + - Updated to [.NET 8.0][dotnet-8-link]. - Updated Debian Docker image to Bookworm. - - Warn when a newer [GitHub Release](https://github.com/ptr727/PlexCleaner/releases/latest) version is available. + - Warn when a newer [GitHub Release][releases-latest-link] version is available. - Only tests for new release availability if `ToolsOptions:AutoUpdate` is enabled. - Updating the tool itself is still a manual process. - Alternatively subscribe to GitHub [Release Notifications][github-release-notification]. @@ -171,8 +178,8 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. - Only media stream validation is performed, track-, bitrate-, and HDR verification is only performed as part of the `process` command. - The `verify` command is useful when testing or selecting from multiple available media sources. - Version 3.3: - - Download Windows FfMpeg builds from [GyanD FfMpeg GitHub mirror](https://github.com/GyanD/codexffmpeg), may help with [#214](https://github.com/ptr727/PlexCleaner/issues/214). - - Install Alpine media tools from `latest-stable` to match the v3.18 base image version, resolves [MediaInfo segfault](https://github.com/ptr727/PlexCleaner/issues/208). + - Download Windows FfMpeg builds from [GyanD FfMpeg GitHub mirror][codexffmpeg-link], may help with [#214][issue-214-link]. + - Install Alpine media tools from `latest-stable` to match the v3.18 base image version, resolves [MediaInfo segfault][issue-208-link]. - Add "legacy" `osx.13-arm64` build. - Make Rider 2023.2.1 happy with current C# linter rules. - Version 3.2: @@ -192,22 +199,22 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. - Removed the `ConvertOptions:EnableH265Encoder`, `ConvertOptions:VideoEncodeQuality` and `ConvertOptions:AudioEncodeCodec` options. - Replaced with `ConvertOptions:FfMpegOptions` and `ConvertOptions:HandBrakeOptions` options. - On v3 schema upgrade old `ConvertOptions` settings will be upgrade to equivalent settings. - - Added support for [IETF / RFC 5646 / BCP 47](https://en.wikipedia.org/wiki/IETF_language_tag) language tag formats. + - Added support for [IETF / RFC 5646 / BCP 47][ietf-language-tag-link] language tag formats. - See the [Language Matching](./README.md#language-matching) section usage for details. - - IETF language tags allows for greater flexibility in Matroska player [language matching](https://codeberg.org/mbunkus/mkvtoolnix/wiki/Languages-in-Matroska-and-MKVToolNix). + - IETF language tags allows for greater flexibility in Matroska player [language matching][mkvtoolnix-languages-link]. - E.g. `pt-BR` for Brazilian Portuguese vs. `por` for Portuguese. - E.g. `zh-Hans` for simplified Chinese vs. `chi` for Chinese. - Update `ProcessOptions:DefaultLanguage` and `ProcessOptions:KeepLanguages` from ISO 639-2B to RFC 5646 format, e.g. `eng` to `en`. - On v3 schema upgrade old ISO 639-2B 3 letter tags will be replaced with generic RFC 5646 tags. - Added `ProcessOptions.SetIetfLanguageTags` to conditionally remux files using MkvMerge to apply IETF language tags when not set. - When enabled all files without IETF tags will be remuxed in order to set IETF language tags, this could be time consuming on large collections of older media that lack the now common IETF tags. - - [FFmpeg](https://github.com/ptr727/PlexCleaner/issues/148) and [HandBrake](https://github.com/ptr727/PlexCleaner/issues/149) removes IETF language tags. + - [FFmpeg][issue-148-link] and [HandBrake][issue-149-link] removes IETF language tags. - Files are remuxed using MkvMerge, and IETF tags are restored using MkvPropEdit, after any FFmpeg or HandBrake operation. - If you care and can, please do communicate the need for IETF language support to the FFmpeg and HandBrake development teams. - Added warnings and attempt to repair when the Language and LanguageIetf are set and are invalid or do not match. - `MkvMerge --identify` added the `--normalize-language-ietf extlang` option to report e.g. `zh-cmn-Hant` vs. `cmn-Hant`. - Existing sidecar metadata can be updated using the `updatesidecar` command. - - Added `ProcessOptions:KeepOriginalLanguage` to keep tracks marked as [original language](https://www.ietf.org/archive/id/draft-ietf-cellar-matroska-15.html#name-original-flag). + - Added `ProcessOptions:KeepOriginalLanguage` to keep tracks marked as [original language][matroska-original-flag-link]. - Added `ProcessOptions:RemoveClosedCaptions` to conditionally vs. always remove closed captions. - Added `ProcessOptions:SetTrackFlags` to set track flags based on track title keywords, e.g. `SDH` -> `HearingImpaired`. - Added `createschema` command to create the settings JSON schema file, no longer need to use `Sandbox` project to create the schema file. @@ -221,7 +228,7 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. - Updated cover art detection and removal logic to not be dependent on `RemoveTags` setting. - Updated `DeleteInvalidFiles` logic to delete any file that fails processing, not just files that fail verification. - Updated `RemoveDuplicateLanguages` logic to use MkvMerge IETF language tags. - - Updated `RemoveDuplicateTracks` logic to account for Matroska [track flags](https://www.ietf.org/archive/id/draft-ietf-cellar-matroska-15.html#name-track-flags). + - Updated `RemoveDuplicateTracks` logic to account for Matroska [track flags][matroska-track-flags-link]. - Refactored JSON schema versioning logic to use `record` instead of `class` allowing for derived classes to inherited attributes vs. needing to duplicate all attributes. - Refactored track selection logic to simplify containment and use with lambda filters. - Refactored verify and repair logic, became too complicated. @@ -233,7 +240,7 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. - Older settings schemas will automatically be upgraded with compatible settings to v3 on first run. - ⚠️ Removed the `reprocess` commandline option, logic was very complex with limited value, use `reverify` instead. - ⚠️ Refactored commandline arguments to only add relevant options to commands that use them vs. adding global options to all commands. - - Maintaining commandline backwards compatibility was [complicated](https://github.com/dotnet/command-line-api/issues/2023), and the change is unfortunately a breaking change. + - Maintaining commandline backwards compatibility was [complicated][command-line-api-issue-2023-link], and the change is unfortunately a breaking change. - The following global options have been removed and added to their respective commands: - `--settingsfile` used by several commands. - `--parallel` used by the `process` command. @@ -250,9 +257,9 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. - As with the `--reprocess` option, this option is useful when the tooling changed, and may now be better equipped to verify or repair broken media. - Version 2.9: - Added remote docker container debug support. - - `develop` tagged docker builds use the `Debug` build target, and will now install the .NET SDK and the [VsDbg](https://aka.ms/getvsdbgsh) .NET Debugger. + - `develop` tagged docker builds use the `Debug` build target, and will now install the .NET SDK and the [VsDbg][vsdbg-link] .NET Debugger. - Added a `--debug` command line option that will wait for a debugger to be attached on launch. - - Remote debugging in docker over SSH can be done using [VSCode](https://github.com/OmniSharp/omnisharp-vscode/wiki/Attaching-to-remote-processes) or [Visual Studio](https://docs.microsoft.com/en-us/visualstudio/debugger/attach-to-process-running-in-docker-container?view=vs-2022). + - Remote debugging in docker over SSH can be done using [VSCode][omnisharp-remote-link] or [Visual Studio][vs-docker-debug-link]. - Updated Dockerfile with latest Linux install steps for MediaInfo and MKVToolNix. - Updated System.CommandLine usage to accommodate Beta 4 breaking changes. - Version 2.8: @@ -272,7 +279,7 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. - Fixed verify and repair logic when `VerifyOptions:AutoRepair` is enabled and file is in `VerifyFailed` state but not `RepairFailed`, could happen when processing is interrupted. - Silenced the noisy `tool version mismatch` warnings when `ProcessOptions:SidecarUpdateOnToolChange` is disabled. - Replaced `FileEx.IsFileReadWriteable()` with `!FileInfo.IsReadOnly` to optimize for speed over accuracy, testing for attributes vs. opening for write access. - - Pinned docker base image to `ubuntu:focal` vs. `ubuntu:latest` until Handbrake PPA ads support for Jammy, tracked as [#98](https://github.com/ptr727/PlexCleaner/issues/98). + - Pinned docker base image to `ubuntu:focal` vs. `ubuntu:latest` until Handbrake PPA ads support for Jammy, tracked as [#98][issue-98-link]. - Version 2.6: - Fixed `SidecarFile.Update()` bug that would not update the sidecar when only the `State` changed, and kept re-verifying the same verified files. - Added a `--reprocess` option to the `process` command, `process --reprocess [0 (default), 1, 2]` @@ -281,14 +288,14 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. - 1: Re-process low cost operations, e.g. tag detection, closed caption detection, etc. - 2: Re-process all operations including expensive operations, e.g. deinterlace detection, bitrate calculation, stream verification, etc. - Whenever processing logic is updated or improved (e.g. this release), it is recommended to run with `--reprocess 1` at least once. - - Added workaround for HandBrake that [force converts](https://github.com/HandBrake/HandBrake/issues/160) closed captions and subtitle tracks to `ASS` format. + - Added workaround for HandBrake that [force converts][handbrake-issue-link] closed captions and subtitle tracks to `ASS` format. - After HandBrake deinterlacing, the original subtitles are added to the output file, bypassing HandBrake subtle logic. - Subtitle track formats and attributes are preserved, and closed captions embedded are not converted to subtitle tracks. - - The HandBrake issue tracked as [#95](https://github.com/ptr727/PlexCleaner/issues/95). - - Added the removal of [EIA-608](https://en.wikipedia.org/wiki/EIA-608) Closed Captions from video streams. + - The HandBrake issue tracked as [#95][issue-95-link]. + - Added the removal of [EIA-608][eia-608-link] Closed Captions from video streams. - Closed Caption subtitles in video streams are undesired as they cannot be managed, all subtitles should be in discrete tracks. - - FFprobe [fails](https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg126211.html) to set the `closed_captions` JSON attribute in JSON output mode, but does detect and print `Closed Captions` in normal output mode. - - FFprobe issue tracked as [#94](https://github.com/ptr727/PlexCleaner/issues/94). + - FFprobe [fails][ffmpeg-devel-link] to set the `closed_captions` JSON attribute in JSON output mode, but does detect and print `Closed Captions` in normal output mode. + - FFprobe issue tracked as [#94][issue-94-link]. - Added the ability to bootstrap 7-Zip downloads on Windows, manually downloading `7za.exe` is no longer required. - Getting started is now easier, just run: - `PlexCleaner.exe --settingsfile PlexCleaner.json defaultsettings` @@ -309,7 +316,7 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. - Use `process --reprocess 2` instead. - Minor code cleanup and improvements. - Version 2.5: - - Changed the config file JSON schema to simplify authoring of multi-value settings, resolves [#85](https://github.com/ptr727/PlexCleaner/issues/85) + - Changed the config file JSON schema to simplify authoring of multi-value settings, resolves [#85][issue-85-link] - Older file schemas will automatically be upgraded without requiring user input. - Comma separated lists in string format converted to array of strings. - Old: `"ReMuxExtensions": ".avi,.m2ts,.ts,.vob,.mp4,.m4v,.asf,.wmv,.dv",` @@ -320,10 +327,10 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. - `"ReEncodeVideoCodecs": "*,dx50,div3,mp42,*,*,*,*,*,*"` - `"ReEncodeVideoProfiles": "*,*,*,*,*,Constrained Baseline@30,*,*,*,*"` - New: `"ReEncodeVideo": [ { "Format": "mpeg2video" }, { "Format": "mpeg4", "Codec": "dx50" }, ... ]` - - Replaced [GitVersion](https://github.com/GitTools/GitVersion) with [Nerdbank.GitVersioning](https://github.com/dotnet/Nerdbank.GitVersioning) as versioning tool, resolves [#16](https://github.com/ptr727/PlexCleaner/issues/16). + - Replaced [GitVersion][gitversion-link] with [Nerdbank.GitVersioning][nerdbank-gitversioning-link] as versioning tool, resolves [#16][issue-16-link]. - Main branch will now build using `Release` configuration, other branches will continue building with `Debug` configuration. - Prerelease builds are now posted to GitHub releases tagged as `pre-release`, Docker builds continue to be tagged as `develop`. - - Docker builds are now also pushed to [GitHub Container Registry](https://github.com/ptr727/PlexCleaner/pkgs/container/plexcleaner). + - Docker builds are now also pushed to [GitHub Container Registry][ghcr-link]. - Builds will continue to push to Docker Hub while it remains free to use. - Added a xUnit unit test project. - Currently the only tests are for config and sidecar JSON schema backwards compatibility. @@ -384,9 +391,68 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. - File logging and console output is now done using structured Serilog logging. - Basic console and file logging options are used, configuration from JSON is not currently supported. -[cliwrap-link]: https://github.com/Tyrrrz/CliWrap + +[discussion-341-link]: https://github.com/ptr727/PlexCleaner/discussions/341 [docker-link]: https://hub.docker.com/r/ptr727/plexcleaner +[ghcr-link]: https://github.com/ptr727/PlexCleaner/pkgs/container/plexcleaner +[issue-148-link]: https://github.com/ptr727/PlexCleaner/issues/148 +[issue-149-link]: https://github.com/ptr727/PlexCleaner/issues/149 +[issue-16-link]: https://github.com/ptr727/PlexCleaner/issues/16 +[issue-208-link]: https://github.com/ptr727/PlexCleaner/issues/208 +[issue-214-link]: https://github.com/ptr727/PlexCleaner/issues/214 +[issue-324-link]: https://github.com/ptr727/PlexCleaner/issues/324 +[issue-344-link]: https://github.com/ptr727/PlexCleaner/issues/344 +[issue-498-link]: https://github.com/ptr727/PlexCleaner/issues/498 +[issue-524-link]: https://github.com/ptr727/PlexCleaner/issues/524 +[issue-746-link]: https://github.com/ptr727/PlexCleaner/issues/746 +[issue-747-link]: https://github.com/ptr727/PlexCleaner/issues/747 +[issue-749-link]: https://github.com/ptr727/PlexCleaner/issues/749 +[issue-809-link]: https://github.com/ptr727/PlexCleaner/issues/809 +[issue-810-link]: https://github.com/ptr727/PlexCleaner/issues/810 +[issue-827-link]: https://github.com/ptr727/PlexCleaner/issues/827 +[issue-85-link]: https://github.com/ptr727/PlexCleaner/issues/85 +[issue-94-link]: https://github.com/ptr727/PlexCleaner/issues/94 +[issue-95-link]: https://github.com/ptr727/PlexCleaner/issues/95 +[issue-98-link]: https://github.com/ptr727/PlexCleaner/issues/98 +[releases-latest-link]: https://github.com/ptr727/PlexCleaner/releases/latest + + +[alpine-issue-15949-link]: https://gitlab.alpinelinux.org/alpine/aports/-/issues/15949 +[alpine-issue-15979-link]: https://gitlab.alpinelinux.org/alpine/aports/-/issues/15979 +[alpine-release-link]: https://alpinelinux.org/posts/Alpine-3.20.0-released.html +[arraypool-link]: https://learn.microsoft.com/en-us/dotnet/api/system.buffers.arraypool-1 +[cliwrap-link]: https://github.com/Tyrrrz/CliWrap +[codexffmpeg-link]: https://github.com/GyanD/codexffmpeg +[command-line-api-issue-2023-link]: https://github.com/dotnet/command-line-api/issues/2023 +[command-line-api-issue-2576-link]: https://github.com/dotnet/command-line-api/issues/2576 +[csharpier-link]: https://csharpier.com/ +[dotnet-8-link]: https://learn.microsoft.com/en-us/dotnet/core/whats-new/dotnet-8 +[dotnet-diagnostics-link]: https://learn.microsoft.com/en-us/dotnet/core/diagnostics/tools-overview +[eia-608-link]: https://en.wikipedia.org/wiki/EIA-608 +[ffmpeg-commit-link]: https://code.ffmpeg.org/FFmpeg/FFmpeg/commit/19c95ecbff84eebca254d200c941ce07868ee707 +[ffmpeg-devel-link]: https://www.mail-archive.com/ffmpeg-devel@ffmpeg.org/msg126211.html [github-release-notification]: https://docs.github.com/en/account-and-profile/managing-subscriptions-and-notifications-on-github/managing-subscriptions-for-activity-on-github/viewing-your-subscriptions +[gitversion-link]: https://github.com/GitTools/GitVersion +[handbrake-issue-link]: https://github.com/HandBrake/HandBrake/issues/160 +[husky-link]: https://alirezanet.github.io/Husky.Net +[ietf-language-tag-link]: https://en.wikipedia.org/wiki/IETF_language_tag +[json-everything-issue-link]: https://github.com/json-everything/json-everything/issues/975 [jsonschema-link]: https://json-everything.net/json-schema/ +[jsonserializercontext-link]: https://learn.microsoft.com/en-us/dotnet/api/system.text.json.serialization.jsonserializercontext +[matroska-original-flag-link]: https://www.ietf.org/archive/id/draft-ietf-cellar-matroska-15.html#name-original-flag +[matroska-track-flags-link]: https://www.ietf.org/archive/id/draft-ietf-cellar-matroska-15.html#name-track-flags +[migrate-from-newtonsoft-link]: https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/migrate-from-newtonsoft +[mkvtoolnix-languages-link]: https://codeberg.org/mbunkus/mkvtoolnix/wiki/Languages-in-Matroska-and-MKVToolNix +[native-aot-cross-compile-link]: https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot/cross-compile +[native-aot-link]: https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot +[nerdbank-gitversioning-link]: https://github.com/dotnet/Nerdbank.GitVersioning +[nullable-value-types-link]: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/nullable-value-types +[omnisharp-remote-link]: https://github.com/OmniSharp/omnisharp-vscode/wiki/Attaching-to-remote-processes +[remote-debugging-link]: https://learn.microsoft.com/en-us/visualstudio/debugger/remote-debugging-dotnet-core-linux-with-ssh [savoury-link]: https://launchpad.net/~savoury1 +[sevenzip-releases-link]: https://github.com/ip7z/7zip/releases +[stackoverflow-xmlserializer-link]: https://stackoverflow.com/questions/79858800/statically-generated-xml-parsing-code-using-microsoft-xmlserializer-generator +[ubuntu-releases-link]: https://releases.ubuntu.com/ [utf8jsonasync-link]: https://github.com/gragra33/Utf8JsonAsyncStreamReader +[vs-docker-debug-link]: https://docs.microsoft.com/en-us/visualstudio/debugger/attach-to-process-running-in-docker-container?view=vs-2022 +[vsdbg-link]: https://aka.ms/getvsdbgsh diff --git a/PlexCleaner/FfMpegBuilder.cs b/PlexCleaner/FfMpegBuilder.cs index d421a932..8108c421 100644 --- a/PlexCleaner/FfMpegBuilder.cs +++ b/PlexCleaner/FfMpegBuilder.cs @@ -29,6 +29,8 @@ public GlobalOptions Default() => public GlobalOptions NoStats() => Add("-nostats"); + public GlobalOptions Progress() => Add("-progress").Add("pipe:1"); + public GlobalOptions ExitOnError() => Add("-xerror"); public GlobalOptions AbortOn() => Add("-abort_on"); diff --git a/PlexCleaner/FfMpegTool.cs b/PlexCleaner/FfMpegTool.cs index e626c4a3..52f51044 100644 --- a/PlexCleaner/FfMpegTool.cs +++ b/PlexCleaner/FfMpegTool.cs @@ -164,11 +164,15 @@ public VerifyResult VerifyMedia(string fileName) // Execute command: ffmpeg can exit 0 yet report stream errors on stderr // Classify stderr line by line as it streams to keep memory bounded, e.g. non-monotonic-DTS file emits a warning per packet VerifyClassifier.Accumulator classifier = new(); - if (!ExecuteStreamStdErr(command, classifier.Add, out int exitCode)) + Metrics.OpStarted(); + bool executed = ExecuteStreamStdErr(command, classifier.Add, out int exitCode); + if (!executed) { // Process could not run + Metrics.OpAborted(); return VerifyResult.DecodeError; } + Metrics.OpCompleted(); // A non-zero exit is always a failure, fail closed even if stderr shows only the timestamp warning VerifyResult verifyResult = classifier.Result; @@ -289,6 +293,27 @@ out string outputMap outputMap = outputMap.Trim(); } + // Parse an ffmpeg -progress line to a fraction, or null. out_time_us and out_time_ms are microseconds. + internal static double? ParseProgressFraction(string line, long durationUs) + { + int separator = line.IndexOf('='); + if (separator <= 0) + { + return null; + } + string value = line[(separator + 1)..]; + return line[..separator] switch + { + "progress" when value == "end" => 1.0, + "out_time_us" + or "out_time_ms" + when durationUs > 0 + && long.TryParse(value, CultureInfo.InvariantCulture, out long microseconds) + && microseconds > 0 => (double)microseconds / durationUs, + _ => null, + }; + } + public bool ConvertToMkv( string inputName, SelectMediaProps? selectMediaProps, @@ -326,8 +351,15 @@ string outputName .Build(); // Execute command - return Execute(command, true, true, out BufferedCommandResult result) - && (result.ExitCode == 0 || LogFailedResult(result, inputName)); + Metrics.OpStarted(); + bool executed = Execute(command, true, true, out BufferedCommandResult result); + if (!executed) + { + Metrics.OpAborted(); + return false; + } + Metrics.OpCompleted(); + return result.ExitCode == 0 || LogFailedResult(result, inputName); } public bool ConvertToMkv(string inputName, string outputName) @@ -354,8 +386,15 @@ public bool ConvertToMkv(string inputName, string outputName) .Build(); // Execute command - return Execute(command, true, true, out BufferedCommandResult result) - && (result.ExitCode == 0 || LogFailedResult(result, inputName)); + Metrics.OpStarted(); + bool executed = Execute(command, true, true, out BufferedCommandResult result); + if (!executed) + { + Metrics.OpAborted(); + return false; + } + Metrics.OpCompleted(); + return result.ExitCode == 0 || LogFailedResult(result, inputName); } public bool SetTimestamps(string inputName, string outputName) @@ -482,10 +521,14 @@ public bool GetIdetText(string fileName, out string text) .Build(); // Execute command - if (!Execute(command, true, true, out BufferedCommandResult result)) + Metrics.OpStarted(); + bool executed = Execute(command, true, true, out BufferedCommandResult result); + if (!executed) { + Metrics.OpAborted(); return false; } + Metrics.OpCompleted(); text = result.StandardError.Trim(); return result.ExitCode == 0 || LogFailedResult(result, fileName); } diff --git a/PlexCleaner/FfProbeTool.cs b/PlexCleaner/FfProbeTool.cs index c30a59b8..9ac959bd 100644 --- a/PlexCleaner/FfProbeTool.cs +++ b/PlexCleaner/FfProbeTool.cs @@ -1,3 +1,4 @@ +using System.Diagnostics; using System.Runtime.CompilerServices; using System.Text; using System.Text.Json; @@ -79,6 +80,7 @@ public bool GetPackets( ) { int processId = -1; + long startTimestamp = Stopwatch.GetTimestamp(); try { // Pipe target to deserialize JSON packets @@ -194,6 +196,13 @@ public bool GetPackets( { return (false, string.Empty); } + finally + { + Metrics.RecordToolDuration( + GetToolType(), + Stopwatch.GetElapsedTime(startTimestamp).TotalMilliseconds + ); + } } public bool GetClosedCaptions(string fileName, out bool hasClosedCaptions) @@ -217,10 +226,14 @@ public bool GetClosedCaptions(string fileName, out bool hasClosedCaptions) .Build(); // Execute command - if (!Execute(command, false, true, out BufferedCommandResult result)) + Metrics.OpStarted(); + bool executed = Execute(command, false, true, out BufferedCommandResult result); + if (!executed) { + Metrics.OpAborted(); return false; } + Metrics.OpCompleted(); if (result.ExitCode != 0) { return LogFailedResult(result, fileName); @@ -304,7 +317,18 @@ bool quickScan .Build(); // Get packet list - if (!GetPackets(command, packetFunc, out string error)) + Metrics.OpStarted(); + bool got = GetPackets(command, packetFunc, out string error); + // GetPackets is false on a non-zero exit where the scan still ran, count completion when it ran (exit 0 or stderr output), not on cancellation or a failure to start + if (got || !string.IsNullOrEmpty(error)) + { + Metrics.OpCompleted(); + } + else + { + Metrics.OpAborted(); + } + if (!got) { Log.Error("Failed to get analysis packets : {FileName}", fileName); LogErrorOutput(error); diff --git a/PlexCleaner/HandBrakeBuilder.cs b/PlexCleaner/HandBrakeBuilder.cs index 30612adc..46e3659d 100644 --- a/PlexCleaner/HandBrakeBuilder.cs +++ b/PlexCleaner/HandBrakeBuilder.cs @@ -10,6 +10,8 @@ public class GlobalOptions(ArgumentsBuilder argumentsBuilder) // TODO: Consolidate public GlobalOptions Default() => this; + public GlobalOptions Json() => Add("--json"); + public GlobalOptions Add(string option) => Add(option, false); public GlobalOptions Add(string option, bool escape) diff --git a/PlexCleaner/HandBrakeTool.cs b/PlexCleaner/HandBrakeTool.cs index a22c2d53..90acd4a1 100644 --- a/PlexCleaner/HandBrakeTool.cs +++ b/PlexCleaner/HandBrakeTool.cs @@ -91,6 +91,24 @@ protected override bool GetLatestVersionWindows(out MediaToolInfo mediaToolInfo) return true; } + // Parse a HandBrake --json line to a fraction, or null. Progress is 0..1 across scan, work, and mux. + internal static double? ParseProgressFraction(string line) + { + Match match = ProgressRegex().Match(line); + return + match.Success + && double.TryParse( + match.Groups[1].Value, + System.Globalization.CultureInfo.InvariantCulture, + out double progress + ) + ? progress + : null; + } + + [GeneratedRegex("\"Progress\":\\s*([0-9.]+)")] + private static partial Regex ProgressRegex(); + public bool ConvertToMkv( string inputName, string outputName, @@ -122,8 +140,15 @@ bool deInterlace .Build(); // Execute command - return Execute(command, true, true, out BufferedCommandResult result) - && (result.ExitCode == 0 || LogFailedResult(result, inputName)); + Metrics.OpStarted(); + bool executed = Execute(command, true, true, out BufferedCommandResult result); + if (!executed) + { + Metrics.OpAborted(); + return false; + } + Metrics.OpCompleted(); + return result.ExitCode == 0 || LogFailedResult(result, inputName); } [GeneratedRegex( diff --git a/PlexCleaner/MediaTool.cs b/PlexCleaner/MediaTool.cs index 0eb9c7a3..12df4b60 100644 --- a/PlexCleaner/MediaTool.cs +++ b/PlexCleaner/MediaTool.cs @@ -1,3 +1,4 @@ +using System.Diagnostics; using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Text; @@ -147,6 +148,39 @@ protected bool LogFailedResult( return false; } + // Overload for the streaming exec paths, which return the captured stderr directly. + protected bool LogFailedResult( + int exitCode, + string errorOutput, + string fileName, + [CallerMemberName] string operation = "" + ) + { + string summary = CleanForLog(Summarize(errorOutput.Trim())); + if (string.IsNullOrEmpty(summary)) + { + Log.Error( + "Failed execution of {ToolType} : {Operation:l} : ExitCode: {ExitCode} : {FileName}", + GetToolType(), + operation, + exitCode, + fileName + ); + } + else + { + Log.Error( + "Failed execution of {ToolType} : {Operation:l} : ExitCode: {ExitCode} : {Error} : {FileName}", + GetToolType(), + operation, + exitCode, + summary, + fileName + ); + } + return false; + } + // Join lines with " | " and drop other control characters so multi-line tool output stays a single structured log value; printable Unicode (e.g. media titles) is preserved protected static string CleanForLog(string text) { @@ -173,6 +207,7 @@ public bool Execute( { bufferedCommandResult = null!; int processId = -1; + long startTimestamp = Stopwatch.GetTimestamp(); try { StringBuilder stdOutBuilder = new(); @@ -223,6 +258,13 @@ public bool Execute( { return false; } + finally + { + Metrics.RecordToolDuration( + GetToolType(), + Stopwatch.GetElapsedTime(startTimestamp).TotalMilliseconds + ); + } } public bool ExecuteStreamStdErr( @@ -234,6 +276,7 @@ public bool ExecuteStreamStdErr( { exitCode = -1; int processId = -1; + long startTimestamp = Stopwatch.GetTimestamp(); try { // Stream stderr line by line to the caller instead of buffering it @@ -285,6 +328,88 @@ public bool ExecuteStreamStdErr( { return false; } + finally + { + Metrics.RecordToolDuration( + GetToolType(), + Stopwatch.GetElapsedTime(startTimestamp).TotalMilliseconds + ); + } + } + + public bool ExecuteStreamStdOut( + Command command, + Action lineAction, + out int exitCode, + out string standardError, + [CallerMemberName] string operation = "" + ) + { + exitCode = -1; + standardError = string.Empty; + int processId = -1; + long startTimestamp = Stopwatch.GetTimestamp(); + try + { + // Stream stdout line by line to the caller (progress output), summarize stderr for logging + PipeTarget stdOutTarget = PipeTarget.Create( + async (stream, cancellationToken) => + { + using StreamReader reader = new(stream, Encoding.Default, false, 1024, true); + while (await reader.ReadLineAsync(cancellationToken) is { } line) + { + if (cancellationToken.IsCancellationRequested) + { + return; + } + lineAction(line); + } + } + ); + StringBuilder stdErrBuilder = new(); + PipeTarget stdErrTarget = ToStringSummary(stdErrBuilder); + + CommandTask task = command + .WithStandardOutputPipe(stdOutTarget) + .WithStandardErrorPipe(stdErrTarget) + .WithValidation(CommandResultValidation.None) + .ExecuteAsync(CancellationToken.None, Program.CancelToken()); + processId = task.ProcessId; + Log.Debug( + "Executing {ToolType} : {Operation:l} : ProcessId: {ProcessId}, Arguments: {Arguments}", + GetToolType(), + operation, + processId, + command.Arguments + ); + + CommandResult commandResult = task.Task.GetAwaiter().GetResult(); + exitCode = commandResult.ExitCode; + standardError = stdErrBuilder.ToString(); + return task.Task.IsCompletedSuccessfully; + } + catch (OperationCanceledException) + { + Log.Error( + "Cancelled execution of {ToolType} : {Operation:l} : ProcessId: {ProcessId}, Arguments: {Arguments}", + GetToolType(), + operation, + processId, + command.Arguments + ); + return false; + } + catch (Exception e) when (Log.Logger.LogAndHandle(e)) + { + return false; + } + finally + { + Metrics.RecordToolDuration( + GetToolType(), + Stopwatch.GetElapsedTime(startTimestamp).TotalMilliseconds + ); + } } public static PipeTarget ToStringBuilder(StringBuilder stringBuilder) => diff --git a/PlexCleaner/Metrics.cs b/PlexCleaner/Metrics.cs new file mode 100644 index 00000000..409ab0c1 --- /dev/null +++ b/PlexCleaner/Metrics.cs @@ -0,0 +1,233 @@ +using System.Diagnostics; +using System.Diagnostics.Metrics; + +namespace PlexCleaner; + +internal static class Metrics +{ + private static readonly Meter s_meter = new("PlexCleaner.Process"); + + // Cumulative for the process lifetime, not reset between runs. + private static readonly Counter s_filesCompleted = s_meter.CreateCounter( + "plexcleaner.files.completed", + description: "Files finished, any outcome" + ); + private static readonly Counter s_filesModified = s_meter.CreateCounter( + "plexcleaner.files.modified", + description: "Files whose media was changed" + ); + private static readonly Counter s_filesErrors = s_meter.CreateCounter( + "plexcleaner.files.errors", + description: "Files that errored" + ); + private static readonly Counter s_filesVerifyFailed = s_meter.CreateCounter( + "plexcleaner.files.verifyfailed", + description: "Files that failed verification" + ); + private static readonly Counter s_filesProcessed = s_meter.CreateCounter( + "plexcleaner.files.processed", + description: "Per-outcome tally, tagged by each State flag set" + ); + + private static readonly Histogram s_fileDuration = s_meter.CreateHistogram( + "plexcleaner.file.duration", + unit: "ms", + description: "Per-file wall-clock time" + ); + private static readonly Histogram s_toolDuration = s_meter.CreateHistogram( + "plexcleaner.tool.duration", + unit: "ms", + description: "Per media-tool invocation time, tagged by tool" + ); + + // Run-scoped state, reset by BeginRun, read by the observable gauges. + // All access is via Interlocked so the parallel loop needs no lock. + private static long s_runFilesTotal; + private static long s_runBytesTotal; + + // Operation-weighted progress: each heavy full-file operation adds the file size to the work total when it starts and the completed total when it ends. + private static long s_runWorkTotal; + private static long s_runWorkCompleted; + private static long s_runInflight; + private static long s_runStartTimestamp; + + // The current file's size, set on the worker thread so OpStarted and OpCompleted can weight by it. + private static readonly ThreadLocal s_currentFileSize = new(); + + // Each State flag (minus None) with its tag, pre-built once so RecordStates allocates nothing per file. + private static readonly ( + SidecarFile.StatesType Flag, + KeyValuePair Tag + )[] s_stateTags = + [ + .. Enum.GetValues() + .Where(flag => flag != SidecarFile.StatesType.None) + .Select(flag => (flag, new KeyValuePair("state", flag.ToString()))), + ]; + + // Each tool with its pre-built tag, so RecordToolDuration allocates nothing per invocation. + private static readonly Dictionary< + MediaTool.ToolType, + KeyValuePair + > s_toolTags = Enum.GetValues() + .ToDictionary( + tool => tool, + tool => new KeyValuePair("tool", tool.ToString()) + ); + + static Metrics() + { + _ = s_meter.CreateObservableGauge( + "plexcleaner.files.total", + () => Interlocked.Read(ref s_runFilesTotal), + description: "Files in the current run" + ); + _ = s_meter.CreateObservableGauge( + "plexcleaner.files.inflight", + () => Interlocked.Read(ref s_runInflight), + description: "Files currently processing" + ); + _ = s_meter.CreateObservableGauge( + "plexcleaner.threads.active", + () => (long)(Program.Options?.ThreadCount ?? 0), + description: "Configured worker threads" + ); + _ = s_meter.CreateObservableGauge( + "plexcleaner.bytes.total", + () => Interlocked.Read(ref s_runBytesTotal), + unit: "By", + description: "Sum of input sizes in the current run" + ); + _ = s_meter.CreateObservableGauge( + "plexcleaner.work.total", + () => Interlocked.Read(ref s_runWorkTotal), + unit: "By", + description: "Operation work discovered, file size added per heavy operation, grows as the path unfolds" + ); + _ = s_meter.CreateObservableGauge( + "plexcleaner.work.completed", + () => Interlocked.Read(ref s_runWorkCompleted), + unit: "By", + description: "Operation work finished, file size added per completed heavy operation" + ); + _ = s_meter.CreateObservableGauge( + "plexcleaner.progress.ratio", + ComputeProgress, + description: "Operation-weighted overall progress [0..1]" + ); + _ = s_meter.CreateObservableGauge( + "plexcleaner.eta.seconds", + ComputeEtaSeconds, + unit: "s", + description: "Estimated time remaining" + ); + } + + // Reset the run-scoped gauges and ETA clock, called once per ProcessFiles run. + internal static void BeginRun(long totalFiles, long totalBytes) + { + _ = Interlocked.Exchange(ref s_runFilesTotal, totalFiles); + _ = Interlocked.Exchange(ref s_runBytesTotal, totalBytes); + _ = Interlocked.Exchange(ref s_runWorkTotal, 0); + _ = Interlocked.Exchange(ref s_runWorkCompleted, 0); + _ = Interlocked.Exchange(ref s_runInflight, 0); + _ = Interlocked.Exchange(ref s_runStartTimestamp, Stopwatch.GetTimestamp()); + } + + internal static void FileStarted(long sizeBytes) + { + s_currentFileSize.Value = sizeBytes; + _ = Interlocked.Increment(ref s_runInflight); + } + + internal static void FileInflightDone() + { + s_currentFileSize.Value = 0; + _ = Interlocked.Decrement(ref s_runInflight); + } + + // A heavy full-file operation started, count the current file's size as work to do. + internal static void OpStarted() => + Interlocked.Add(ref s_runWorkTotal, s_currentFileSize.Value); + + // The heavy operation finished, count the same size as work done. + internal static void OpCompleted() => + Interlocked.Add(ref s_runWorkCompleted, s_currentFileSize.Value); + + // The heavy operation never ran, roll its size back out of the total so progress can still converge. + internal static void OpAborted() => + Interlocked.Add(ref s_runWorkTotal, -s_currentFileSize.Value); + + internal static void FileCompleted(TimeSpan wall) + { + s_filesCompleted.Add(1); + s_fileDuration.Record(wall.TotalMilliseconds); + } + + internal static void FileErrored() => s_filesErrors.Add(1); + + internal static void RecordModified() => s_filesModified.Add(1); + + internal static void RecordVerifyFailed() => s_filesVerifyFailed.Add(1); + + internal static void RecordStates(SidecarFile.StatesType state) + { + foreach ((SidecarFile.StatesType flag, KeyValuePair tag) in s_stateTags) + { + if ((state & flag) == flag) + { + s_filesProcessed.Add(1, tag); + } + } + } + + internal static void RecordToolDuration(MediaTool.ToolType tool, double milliseconds) => + s_toolDuration.Record(milliseconds, s_toolTags[tool]); + + internal static void Dispose() + { + s_currentFileSize.Dispose(); + s_meter.Dispose(); + } + + // Completed operation work over discovered operation work, guarding a zero total. + internal static double ComputeProgress() + { + long total = Interlocked.Read(ref s_runWorkTotal); + if (total <= 0) + { + return 0.0; + } + double completed = Interlocked.Read(ref s_runWorkCompleted); + return Math.Clamp(completed / total, 0.0, 1.0); + } + + // Linear extrapolation from weighted progress and elapsed time. + // Returns 0 before any progress and never a non-finite value. + internal static double ComputeEtaSeconds() + { + double ratio = ComputeProgress(); + if (ratio <= 0.0) + { + return 0.0; + } + double elapsed = Stopwatch + .GetElapsedTime(Interlocked.Read(ref s_runStartTimestamp)) + .TotalSeconds; + double eta = elapsed * (1.0 - ratio) / ratio; + return double.IsFinite(eta) ? eta : 0.0; + } + + internal static IEnumerable EnumerateSetStates( + SidecarFile.StatesType state + ) + { + foreach ((SidecarFile.StatesType flag, _) in s_stateTags) + { + if ((state & flag) == flag) + { + yield return flag; + } + } + } +} diff --git a/PlexCleaner/PlexCleaner.csproj b/PlexCleaner/PlexCleaner.csproj index 7fe9ee9f..e72a9f29 100644 --- a/PlexCleaner/PlexCleaner.csproj +++ b/PlexCleaner/PlexCleaner.csproj @@ -35,6 +35,10 @@ $(DefineConstants);PLUGINS + + + true + True diff --git a/PlexCleaner/Process.cs b/PlexCleaner/Process.cs index e35919a7..873022b6 100644 --- a/PlexCleaner/Process.cs +++ b/PlexCleaner/Process.cs @@ -506,6 +506,17 @@ out string? failedOperation return processResult; } + // Per-outcome metrics: this is the only place the State flags are known + Metrics.RecordStates(state); + if (modified) + { + Metrics.RecordModified(); + } + if ((state & SidecarFile.StatesType.VerifyFailed) != 0) + { + Metrics.RecordVerifyFailed(); + } + // Save result lock (resultLock) { diff --git a/PlexCleaner/ProcessDriver.cs b/PlexCleaner/ProcessDriver.cs index 24c0f6fe..5c030bfc 100644 --- a/PlexCleaner/ProcessDriver.cs +++ b/PlexCleaner/ProcessDriver.cs @@ -117,6 +117,31 @@ Func taskFunc // Process all files in parallel int totalCount = fileList.Count; + + // Sum input sizes up front for bytes.total and to weight each file's operations, a missing file counts as zero. + Dictionary fileSizes = new(totalCount, StringComparer.Ordinal); + long totalBytes = 0; + foreach (string file in fileList) + { + // Exclude non-MKV files from the totals when mkvFilesOnly, since they are skipped not processed. + if (mkvFilesOnly && !SidecarFile.IsMkvFile(file)) + { + continue; + } + long length = 0; + try + { + length = new FileInfo(file).Length; + } + catch (Exception e) when (Log.Logger.LogAndHandle(e)) + { + // Length unavailable: weight this file as zero + } + fileSizes[file] = length; + totalBytes += length; + } + Metrics.BeginRun(fileSizes.Count, totalBytes); + int processedCount = 0; int errorCount = 0; bool error = false; @@ -177,9 +202,20 @@ Func taskFunc fileName ); - // Perform the task, timing this file's work + // Perform the task, timing this file's work. + // Decrement the in-flight count in a finally so a cancellation cannot leak it. + long fileSize = fileSizes.GetValueOrDefault(fileName); + Metrics.FileStarted(fileSize); long startTimestamp = Stopwatch.GetTimestamp(); - bool taskResult = taskFunc(fileName); + bool taskResult; + try + { + taskResult = taskFunc(fileName); + } + finally + { + Metrics.FileInflightDone(); + } TimeSpan taskElapsed = Stopwatch.GetElapsedTime(startTimestamp); // Handle cancel request @@ -191,6 +227,7 @@ Func taskFunc // Error Log.Error("{TaskName} Error : {FileName}", taskName, fileName); _ = Interlocked.Increment(ref errorCount); + Metrics.FileErrored(); } // Log completion % after task completes @@ -198,6 +235,7 @@ Func taskFunc Interlocked.Increment(ref processedCount), totalCount ); + Metrics.FileCompleted(taskElapsed); Log.Information( "{TaskName} ({Processed:F2}%) Elapsed : {Elapsed:l} : After : {FileName}", taskName, diff --git a/PlexCleaner/ProcessFile.cs b/PlexCleaner/ProcessFile.cs index 83b51913..13418545 100644 --- a/PlexCleaner/ProcessFile.cs +++ b/PlexCleaner/ProcessFile.cs @@ -2550,6 +2550,7 @@ out DtsInfo? dtsInfo Program.Config.VerifyOptions.MaximumBitrate / 8 ); DtsInfo packetDts = new(); + if ( !Tools.FfProbe.GetAnalysisPackets( FileInfo.FullName, diff --git a/PlexCleaner/Program.cs b/PlexCleaner/Program.cs index e22b31fa..298f2318 100644 --- a/PlexCleaner/Program.cs +++ b/PlexCleaner/Program.cs @@ -114,6 +114,7 @@ private static int Main(string[] args) Log.Logger.LogOverrideContext().Information("Exit Code : {ExitCode}", exitCode); Log.CloseAndFlush(); s_libraryLoggerFactory?.Dispose(); + Metrics.Dispose(); return exitCode; } diff --git a/PlexCleanerTests/MetricsTests.cs b/PlexCleanerTests/MetricsTests.cs new file mode 100644 index 00000000..3e74231a --- /dev/null +++ b/PlexCleanerTests/MetricsTests.cs @@ -0,0 +1,192 @@ +using System.Diagnostics.Metrics; +using AwesomeAssertions; +using PlexCleaner; +using Xunit; + +namespace PlexCleanerTests; + +// Metrics is process-static, so run these in the non-parallel collection and reset run state with +// BeginRun at the start of every test. +[Collection("Sequential")] +public class MetricsTests +{ + [Fact] + public void ComputeProgress_ZeroTotal_IsZero() + { + Metrics.BeginRun(0, 0); + + _ = Metrics.ComputeProgress().Should().Be(0.0); + } + + [Fact] + public void ComputeEtaSeconds_NoProgress_IsZero() + { + Metrics.BeginRun(2, 1000); + + _ = Metrics.ComputeEtaSeconds().Should().Be(0.0); + } + + [Fact] + public void ComputeEtaSeconds_PartialProgress_IsFiniteAndNonNegative() + { + // Discover two operations and finish one, so progress is partial and ETA is finite + Metrics.BeginRun(2, 1000); + Metrics.FileStarted(500); + Metrics.OpStarted(); + Metrics.OpCompleted(); + Metrics.OpStarted(); + + double eta = Metrics.ComputeEtaSeconds(); + + _ = double.IsFinite(eta).Should().BeTrue(); + _ = eta.Should().BeGreaterThanOrEqualTo(0.0); + + Metrics.OpCompleted(); + Metrics.FileInflightDone(); + } + + [Fact] + public void EnumerateSetStates_ReturnsEachSetFlag() + { + SidecarFile.StatesType state = + SidecarFile.StatesType.ReMuxed + | SidecarFile.StatesType.Verified + | SidecarFile.StatesType.ClearedTags; + + List flags = [.. Metrics.EnumerateSetStates(state)]; + + _ = flags + .Should() + .BeEquivalentTo([ + SidecarFile.StatesType.ReMuxed, + SidecarFile.StatesType.Verified, + SidecarFile.StatesType.ClearedTags, + ]); + } + + [Fact] + public void EnumerateSetStates_None_IsEmpty() => + _ = Metrics.EnumerateSetStates(SidecarFile.StatesType.None).Should().BeEmpty(); + + [Fact] + public void Instruments_AreObservableViaMeterListener() + { + List<(string Name, long Value, KeyValuePair[] Tags)> longs = []; + List<(string Name, double Value)> doubles = []; + + using MeterListener listener = new(); + listener.InstrumentPublished = (instrument, meterListener) => + { + if (instrument.Meter.Name == "PlexCleaner.Process") + { + meterListener.EnableMeasurementEvents(instrument); + } + }; + listener.SetMeasurementEventCallback( + (instrument, measurement, tags, _) => + longs.Add((instrument.Name, measurement, tags.ToArray())) + ); + listener.SetMeasurementEventCallback( + (instrument, measurement, _, _) => doubles.Add((instrument.Name, measurement)) + ); + listener.Start(); + + // File 1 runs and completes one op, file 2 starts an op and stays in flight, so operation-weighted progress is 400 of 1000 bytes + Metrics.BeginRun(2, 1000); + Metrics.FileStarted(400); + Metrics.OpStarted(); + Metrics.OpCompleted(); + Metrics.FileInflightDone(); + Metrics.FileCompleted(TimeSpan.Zero); + Metrics.RecordStates(SidecarFile.StatesType.ReMuxed | SidecarFile.StatesType.Verified); + Metrics.FileStarted(600); + Metrics.OpStarted(); + listener.RecordObservableInstruments(); + + // The counter fired one measurement per set flag with the state tag + List states = + [ + .. longs + .Where(m => m.Name == "plexcleaner.files.processed") + .Select(m => m.Tags.Single(t => t.Key == "state").Value?.ToString()), + ]; + _ = states.Should().BeEquivalentTo(["ReMuxed", "Verified"]); + + // One of two started files is still in flight, and progress is operation-weighted + _ = longs + .Should() + .ContainSingle(m => m.Name == "plexcleaner.files.inflight") + .Which.Value.Should() + .Be(1); + _ = doubles + .Should() + .ContainSingle(m => m.Name == "plexcleaner.progress.ratio") + .Which.Value.Should() + .BeApproximately(0.4, 1e-9); + + // Clear the in-flight file + Metrics.OpCompleted(); + Metrics.FileInflightDone(); + } + + [Fact] + public void ComputeProgress_IsCompletedOverDiscoveredWork() + { + // Progress is completed operation work over discovered operation work, each op weighted by file size + Metrics.BeginRun(1, 100_000); + Metrics.FileStarted(100_000); + + // No operations yet, guard the zero total + _ = Metrics.ComputeProgress().Should().Be(0.0); + + // First op started but not done, counted in the total only + Metrics.OpStarted(); + _ = Metrics.ComputeProgress().Should().Be(0.0); + + // First op done, all discovered work is complete + Metrics.OpCompleted(); + _ = Metrics.ComputeProgress().Should().Be(1.0); + + // A second op is discovered, the total grows and the ratio dips + Metrics.OpStarted(); + _ = Metrics.ComputeProgress().Should().BeApproximately(0.5, 1e-9); + + Metrics.OpCompleted(); + _ = Metrics.ComputeProgress().Should().Be(1.0); + + Metrics.FileInflightDone(); + } + + [Fact] + public void OpAborted_RollsBackTheStartedWork() + { + // An operation that never ran rolls its size back out of the total so progress still converges + Metrics.BeginRun(1, 1000); + Metrics.FileStarted(400); + + Metrics.OpStarted(); + Metrics.OpCompleted(); + _ = Metrics.ComputeProgress().Should().Be(1.0); + + // A second operation starts then aborts, the total returns to the completed work + Metrics.OpStarted(); + _ = Metrics.ComputeProgress().Should().BeApproximately(0.5, 1e-9); + Metrics.OpAborted(); + _ = Metrics.ComputeProgress().Should().Be(1.0); + + Metrics.FileInflightDone(); + } + + [Fact] + public void OpAfterInflightDone_CountsNothing() + { + // FileInflightDone clears the current file size, so a stray late op adds no work + Metrics.BeginRun(1, 1000); + Metrics.FileStarted(400); + Metrics.FileInflightDone(); + + Metrics.OpStarted(); + Metrics.OpCompleted(); + _ = Metrics.ComputeProgress().Should().Be(0.0); + } +} diff --git a/PlexCleanerTests/ToolProgressParsingTests.cs b/PlexCleanerTests/ToolProgressParsingTests.cs new file mode 100644 index 00000000..dd7d13d9 --- /dev/null +++ b/PlexCleanerTests/ToolProgressParsingTests.cs @@ -0,0 +1,46 @@ +using AwesomeAssertions; +using PlexCleaner; +using Xunit; + +namespace PlexCleanerTests; + +public class ToolProgressParsingTests +{ + [Theory] + [InlineData("out_time_us=5000000", 10000000L, 0.5)] + [InlineData("out_time_ms=2500000", 10000000L, 0.25)] + [InlineData("progress=end", 10000000L, 1.0)] + public void FfMpegProgress_ParsesFraction(string line, long durationUs, double expected) + { + double? fraction = FfMpeg.Tool.ParseProgressFraction(line, durationUs); + + _ = fraction.Should().NotBeNull(); + _ = fraction.Value.Should().BeApproximately(expected, 1e-9); + } + + [Theory] + [InlineData("frame=100", 10000000L)] // not a position line + [InlineData("out_time_us=0", 10000000L)] // zero position + [InlineData("out_time_us=5000000", 0L)] // no duration + [InlineData("progress=continue", 10000000L)] // continue is not a terminal + [InlineData("garbage", 10000000L)] + public void FfMpegProgress_ReturnsNullWithoutPosition(string line, long durationUs) => + _ = FfMpeg.Tool.ParseProgressFraction(line, durationUs).Should().BeNull(); + + [Theory] + [InlineData(" \"Progress\": 0.42,", 0.42)] + [InlineData("\"Progress\":1.0", 1.0)] + public void HandBrakeProgress_ParsesFraction(string line, double expected) + { + double? fraction = HandBrake.Tool.ParseProgressFraction(line); + + _ = fraction.Should().NotBeNull(); + _ = fraction.Value.Should().BeApproximately(expected, 1e-9); + } + + [Theory] + [InlineData("\"State\": \"WORKING\"")] + [InlineData("random line")] + public void HandBrakeProgress_ReturnsNullWithoutProgress(string line) => + _ = HandBrake.Tool.ParseProgressFraction(line).Should().BeNull(); +} diff --git a/README.md b/README.md index 159c6e78..6d2f77e9 100644 --- a/README.md +++ b/README.md @@ -23,13 +23,12 @@ Utility to optimize media files for Direct Play in Plex, Emby, Jellyfin, etc. ### Release Notes -**Version: 3.21**: +**Version: 3.22**: **Summary:** -- Treat a non-monotonic DTS as a verify failure, and attempt to repair it losslessly with the `setts` bitstream filter. -- Switched closed caption detection to `ffprobe -analyze_frames`, and consolidated the bitrate and DTS packet analyses into a single packet pass. -- Added the `DtsTimestampRepair` example plugin that attempts non-monotonic DTS repairs on `RepairFailed` files. +- Added always-on runtime metrics with operation-weighted progress, published via `System.Diagnostics.Metrics` and readable with `dotnet-counters`. +- Bundled a `counters` wrapper in the Docker image for reading the metrics with a single command. See [Release History](./HISTORY.md) for complete release notes and older versions. @@ -86,6 +85,7 @@ See [Installation](#installation) for detailed setup instructions and other plat - [Process Command](#process-command) - [Monitor Command](#monitor-command) - [Other Commands](#other-commands) +- [Runtime Metrics](#runtime-metrics) - [Custom Plugins](#custom-plugins) - [Testing](#testing) - [Unit Testing](#unit-testing) @@ -118,7 +118,7 @@ See [Installation](#installation) for detailed setup instructions and other plat ## Use Cases -> **ℹ️ TL;DR**: *Direct Play* means your media server (Plex/Emby/Jellyfin) sends the file directly to your player without transcoding on the server or the client. This saves server CPU, reduces power consumption, preserves quality, and enables playback on low-power devices. The **objective of PlexCleaner** is to *modify media content* such that it will always Direct Play in [Plex](https://support.plex.tv/articles/200250387-streaming-media-direct-play-and-direct-stream/), [Emby](https://support.emby.media/support/solutions/articles/44001920144-direct-play-vs-direct-streaming-vs-transcoding), [Jellyfin](https://jellyfin.org/docs/plugin-api/MediaBrowser.Model.Session.PlayMethod.html), etc. +> **ℹ️ TL;DR**: *Direct Play* means your media server (Plex/Emby/Jellyfin) sends the file directly to your player without transcoding on the server or the client. This saves server CPU, reduces power consumption, preserves quality, and enables playback on low-power devices. The **objective of PlexCleaner** is to *modify media content* such that it will always Direct Play in [Plex][plex-directplay-link], [Emby][emby-directplay-link], [Jellyfin][jellyfin-playmethod-link], etc. Common examples of issues resolved by the `process` command: @@ -161,7 +161,7 @@ PlexCleaner is optimized for processing large media libraries efficiently. Key p > - **Large libraries**: Use `--parallel` to process multiple files concurrently. > - **Testing**: Combine `--testsnippets` and `--quickscan` for faster test iterations. > - **Network storage**: Process files locally when possible to avoid network bottlenecks. -> - **Docker logging**: Configure [log rotation](https://docs.docker.com/config/containers/logging/configure/) to prevent large log files. +> - **Docker logging**: Configure [log rotation][docker-logging-link] to prevent large log files. > - **Thread count**: Default is half of CPU cores (max 4); adjust with `--threadcount` if needed. **Sidecar Files:** @@ -178,7 +178,7 @@ PlexCleaner is optimized for processing large media libraries efficiently. Key p **Docker Considerations:** -- Processing very large media collections on docker may result in a very large docker log file, set appropriate [docker logging](https://docs.docker.com/config/containers/logging/configure/) options. +- Processing very large media collections on docker may result in a very large docker log file, set appropriate [docker logging][docker-logging-link] options. ## Installation @@ -300,13 +300,13 @@ For one-time processing, see the [Getting Started](#getting-started) example or **Prerequisites:** -- For pre-compiled binaries: Install [.NET Runtime](https://docs.microsoft.com/en-us/dotnet/core/install/windows) (smaller, runtime only). -- For compiling from source: Install [.NET SDK](https://dotnet.microsoft.com/download) (includes build tools). +- For pre-compiled binaries: Install [.NET Runtime][dotnet-install-windows-link] (smaller, runtime only). +- For compiling from source: Install [.NET SDK][dotnet-download-link] (includes build tools). **Installation Steps:** -1. Download [PlexCleaner](https://github.com/ptr727/PlexCleaner/releases/latest) and extract the pre-compiled binaries. - - Or compile from [code](https://github.com/ptr727/PlexCleaner.git) using [Visual Studio](https://visualstudio.microsoft.com/downloads/) or [VSCode](https://code.visualstudio.com/download) with the .NET SDK. +1. Download [PlexCleaner][releases-latest-link] and extract the pre-compiled binaries. + - Or compile from [code][clone-link] using [Visual Studio][visualstudio-link] or [VSCode][vscode-link] with the .NET SDK. 2. Create a default JSON settings file using the `defaultsettings` command: - `PlexCleaner defaultsettings --settingsfile PlexCleaner.json` @@ -321,7 +321,7 @@ For one-time processing, see the [Getting Started](#getting-started) example or - Keep the 3rd party tools updated by periodically running the `checkfornewtools` command, or update tools on every run by setting `ToolsOptions:AutoUpdate` to `true`. **Option B: System-wide installation via winget** - - Run from an elevated shell e.g. using [`gsudo`](https://github.com/gerardog/gsudo), else [symlinks will not be created](https://github.com/microsoft/winget-cli/issues/3437). + - Run from an elevated shell e.g. using [`gsudo`][gsudo-link], else [symlinks will not be created][winget-cli-issue-link]. - `winget install --id=Gyan.FFmpeg --exact` - `winget install --id=MediaArea.MediaInfo --exact` - `winget install --id=HandBrake.HandBrake.CLI --exact` @@ -329,31 +329,31 @@ For one-time processing, see the [Getting Started](#getting-started) example or - Set `ToolsOptions:UseSystem` to `true` and `ToolsOptions:AutoUpdate` to `false`. **Option C: Manual download** - - [FfMpeg Full](https://github.com/GyanD/codexffmpeg/releases), e.g. `ffmpeg-6.0-full.7z`: `\Tools\FfMpeg` - - [HandBrake CLI x64](https://github.com/HandBrake/HandBrake/releases), e.g. `HandBrakeCLI-1.6.1-win-x86_64.zip`: `\Tools\HandBrake` - - [MediaInfo CLI x64](https://mediaarea.net/en/MediaInfo/Download/Windows), e.g. `MediaInfo_CLI_23.07_Windows_x64.zip`: `\Tools\MediaInfo` - - [MkvToolNix Portable x64](https://mkvtoolnix.download/downloads.html#windows), e.g. `mkvtoolnix-64-bit-79.0.7z`: `\Tools\MkvToolNix` - - [7-Zip Extra](https://www.7-zip.org/download.html), e.g. `7z2301-extra.7z`: `\Tools\SevenZip` + - [FfMpeg Full][codexffmpeg-releases-link], e.g. `ffmpeg-6.0-full.7z`: `\Tools\FfMpeg` + - [HandBrake CLI x64][handbrake-releases-link], e.g. `HandBrakeCLI-1.6.1-win-x86_64.zip`: `\Tools\HandBrake` + - [MediaInfo CLI x64][mediainfo-download-link], e.g. `MediaInfo_CLI_23.07_Windows_x64.zip`: `\Tools\MediaInfo` + - [MkvToolNix Portable x64][mkvtoolnix-download-link], e.g. `mkvtoolnix-64-bit-79.0.7z`: `\Tools\MkvToolNix` + - [7-Zip Extra][sevenzip-download-link], e.g. `7z2301-extra.7z`: `\Tools\SevenZip` - Set `ToolsOptions:UseSystem` to `false` and `ToolsOptions:AutoUpdate` to `false`. ### Linux - Automatic downloading of Linux 3rd party tools are not supported, consider using the [Docker](#docker) build instead. - Manually install the 3rd party tools, e.g. following steps similar to the [Docker](./Docker) file commands. -- Download [PlexCleaner](https://github.com/ptr727/PlexCleaner/releases/latest) and extract the pre-compiled binaries matching your platform. -- Or compile from [code](https://github.com/ptr727/PlexCleaner.git) using the [.NET SDK](https://docs.microsoft.com/en-us/dotnet/core/install/linux). +- Download [PlexCleaner][releases-latest-link] and extract the pre-compiled binaries matching your platform. +- Or compile from [code][clone-link] using the [.NET SDK][dotnet-install-linux-link]. - Create a default JSON settings file using the `defaultsettings` command: - `./PlexCleaner defaultsettings --settingsfile PlexCleaner.json` - Modify the settings to suit your needs. ### macOS -- macOS x64 and Arm64 binaries are built as part of [Releases](https://github.com/ptr727/PlexCleaner/releases/latest), but are not tested during CI. +- macOS x64 and Arm64 binaries are built as part of [Releases][releases-latest-link], but are not tested during CI. ### AOT Ahead-of-time compiled self-contained binaries do not require any .NET runtime components to be installed.\ -AOT builds are [platform specific](https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot), require a platform native compiler, and are created using [`dotnet publish`](https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-publish). +AOT builds are [platform specific][native-aot-link], require a platform native compiler, and are created using [`dotnet publish`][dotnet-publish-link]. > **ℹ️ Note**: AOT binaries are not published in CI/CD due to being platform specific, and cross compilation of AOT binaries are not supported. @@ -492,7 +492,7 @@ Quick configuration examples for common use cases. Edit your `PlexCleaner.json` ### IETF Language Matching -> **ℹ️ TL;DR**: Language tag matching supports [IETF / RFC 5646 / BCP 47](https://en.wikipedia.org/wiki/IETF_language_tag) tag formats as implemented by [MkvMerge](https://codeberg.org/mbunkus/mkvtoolnix/wiki/Languages-in-Matroska-and-MKVToolNix). +> **ℹ️ TL;DR**: Language tag matching supports [IETF / RFC 5646 / BCP 47][ietf-language-tag-link] tag formats as implemented by [MkvMerge][mkvtoolnix-languages-link]. **Common Use Cases:** @@ -766,7 +766,7 @@ Options: The `monitor` command will watch the specified folders for file changes, and periodically run the `process` command on the changed folders: - All the referenced directories will be watched for changes, and any changes will be added to a queue to be periodically processed. -- The [FileSystemWatcher](https://docs.microsoft.com/en-us/dotnet/api/system.io.filesystemwatcher) used to monitor for changes may not always work as expected when changes are made via virtual or network filesystem, e.g. NFS or SMB backed volumes may not detect changes made directly to the underlying ZFS filesystem, while running directly on ZFS will work fine. +- The [FileSystemWatcher][filesystemwatcher-link] used to monitor for changes may not always work as expected when changes are made via virtual or network filesystem, e.g. NFS or SMB backed volumes may not detect changes made directly to the underlying ZFS filesystem, while running directly on ZFS will work fine. Options: @@ -834,6 +834,17 @@ Additional commands for specific tasks, organized by category: - `getmediainfo`: - Print media file information and track details. +## Runtime Metrics + +PlexCleaner publishes always-on runtime metrics via `System.Diagnostics.Metrics` under the `PlexCleaner.Process` meter, so a long-running `process` or `monitor` pass can be watched live with no extra setup. Overall progress is operation-weighted: each heavy full-file operation on a file (closed-caption and interlace scans, bitrate analysis, re-encode, deinterlace, verify) counts the file's size as work to do when it starts and as work done when it finishes. A run mixing small and large files therefore reflects the actual work completed, and because the per-file path is not fixed the total grows as operations are discovered. The meter publishes `progress.ratio` and `eta.seconds`, the `work.total`/`work.completed` byte gauges behind them, `bytes.total` (input size), `files.total`/`files.inflight`/`files.completed`, and per-tool timing histograms. + +Read the meter with [`dotnet-counters`][dotnet-counters-link]: + +- Local: `dotnet-counters monitor -p --counters PlexCleaner.Process` +- Docker: `docker exec counters` (a bundled wrapper that runs the same command against the in-container process) + +The meter is also OpenTelemetry and [`dotnet-monitor`][dotnet-monitor-link] compatible if you want an HTTP or Prometheus surface. + ## Custom Plugins The `custom` command runs a user-provided plugin assembly over the media files, reusing PlexCleaner's file iteration and processing. This is useful for bespoke, targeted re-processing that the built-in commands do not cover, for example re-running a newly added or fixed verification check on a library where files are already marked as verified, without paying for a full re-verification. @@ -924,7 +935,7 @@ The [`Test.sh`](./Docker/Test.sh) script validates basic container functionality The [`Test.sh`](./Docker/Test.sh) test script is included in the docker build and can be used to test basic functionality from inside the container. -If an external media path is not specified the test will download and use the [Matroska test files](https://github.com/ietf-wg-cellar/matroska-test-files/archive/refs/heads/master.zip). +If an external media path is not specified the test will download and use the [Matroska test files][matroska-test-files-zip-link]. ```shell docker run \ @@ -972,63 +983,121 @@ Some ideas being considered: ## 3rd Party Tools -- [7-Zip](https://www.7-zip.org/) -- [AwesomeAssertions](https://awesomeassertions.org/) +- [7-Zip][sevenzip-link] +- [AwesomeAssertions][awesomeassertions-link] - [CliWrap][cliwrap-link] -- [Docker Hub Description](https://github.com/marketplace/actions/docker-hub-description) -- [Docker Run Action](https://github.com/marketplace/actions/docker-run-action) -- [dotnet-outdated](https://github.com/dotnet-outdated/dotnet-outdated) -- [FFmpeg](https://www.ffmpeg.org/) -- [Git Auto Commit](https://github.com/marketplace/actions/git-auto-commit) -- [GitHub Actions](https://github.com/actions) -- [GitHub Dependabot](https://github.com/dependabot) -- [HandBrake](https://handbrake.fr/) -- [Husky.Net](https://alirezanet.github.io/Husky.Net/) -- [ISO 639-2 language tags](https://www.loc.gov/standards/iso639-2/langhome.html) -- [ISO 639-3 language tags](https://iso639-3.sil.org/) +- [Docker Hub Description][docker-hub-description-action-link] +- [Docker Run Action][docker-run-action-link] +- [dotnet-outdated][dotnet-outdated-link] +- [FFmpeg][ffmpeg-link] +- [Git Auto Commit][git-auto-commit-action-link] +- [GitHub Actions][github-actions-link] +- [GitHub Dependabot][dependabot-link] +- [HandBrake][handbrake-link] +- [Husky.Net][husky-link] +- [ISO 639-2 language tags][iso639-2-link] +- [ISO 639-3 language tags][iso639-3-link] - [JSON2CSharp][json2csharp-link] -- [MediaInfo](https://mediaarea.net/en-us/MediaInfo/) -- [MKVToolNix](https://mkvtoolnix.download/) -- [NEbml](https://github.com/OlegZee/NEbml) -- [Nerdbank.GitVersioning](https://github.com/marketplace/actions/nerdbank-gitversioning) -- [regex101.com](https://regex101.com/) -- [RFC 5646 language tags](https://www.rfc-editor.org/rfc/rfc5646.html) -- [Serilog](https://serilog.net/) +- [MediaInfo][mediainfo-link] +- [MKVToolNix][mkvtoolnix-link] +- [NEbml][nebml-link] +- [Nerdbank.GitVersioning][nerdbank-gitversioning-action-link] +- [regex101.com][regex101-link] +- [RFC 5646 language tags][rfc5646-link] +- [Serilog][serilog-link] - [Utf8JsonAsyncStreamReader][utf8jsonasync-link] -- [Xml2CSharp](http://xmltocsharp.azurewebsites.net/) -- [xUnit.Net](https://xunit.net/) +- [Xml2CSharp][xmltocsharp-link] +- [xUnit.Net][xunit-link] ## Sample Media Files -- [DemoWorld](https://www.demo-world.eu/2d-demo-trailers-hd/) -- [JellyFish](http://jell.yfish.us/) -- [Kodi](https://kodi.wiki/view/Samples) -- [Matroska](https://github.com/ietf-wg-cellar/matroska-test-files) -- [MPlayer](https://samples.mplayerhq.hu/) +- [DemoWorld][demo-world-link] +- [JellyFish][jellyfish-link] +- [Kodi][kodi-samples-link] +- [Matroska][matroska-test-files-link] +- [MPlayer][mplayer-samples-link] ## License Licensed under the [MIT License][license-link]\ ![GitHub License][license-shield] -[actions-link]: https://github.com/ptr727/PlexCleaner/actions -[cliwrap-link]: https://github.com/Tyrrrz/CliWrap -[commit-link]: https://github.com/ptr727/PlexCleaner/commits/main -[discussions-link]: https://github.com/ptr727/PlexCleaner/discussions + [docker-develop-version-shield]: https://img.shields.io/docker/v/ptr727/plexcleaner/develop?label=Docker%20Develop&logo=docker&color=orange [docker-latest-version-shield]: https://img.shields.io/docker/v/ptr727/plexcleaner/latest?label=Docker%20Latest&logo=docker -[docker-link]: https://hub.docker.com/r/ptr727/plexcleaner [docker-status-shield]: https://img.shields.io/github/actions/workflow/status/ptr727/PlexCleaner/publish-release.yml?event=schedule&logo=github&label=Docker%20Build -[github-link]: https://github.com/ptr727/PlexCleaner -[plexcleaner-hub-link]: https://hub.docker.com/r/ptr727/plexcleaner -[issues-link]: https://github.com/ptr727/PlexCleaner/issues -[json2csharp-link]: https://json2csharp.com [last-commit-shield]: https://img.shields.io/github/last-commit/ptr727/PlexCleaner?logo=github&label=Last%20Commit -[license-link]: ./LICENSE [license-shield]: https://img.shields.io/github/license/ptr727/PlexCleaner?label=License [pre-release-version-shield]: https://img.shields.io/github/v/release/ptr727/PlexCleaner?include_prereleases&label=GitHub%20Pre-Release&logo=github [release-status-shield]: https://img.shields.io/github/actions/workflow/status/ptr727/PlexCleaner/publish-release.yml?event=schedule&logo=github&label=Releases%20Build [release-version-shield]: https://img.shields.io/github/v/release/ptr727/PlexCleaner?logo=github&label=GitHub%20Release + + +[actions-link]: https://github.com/ptr727/PlexCleaner/actions +[clone-link]: https://github.com/ptr727/PlexCleaner.git +[commit-link]: https://github.com/ptr727/PlexCleaner/commits/main +[discussions-link]: https://github.com/ptr727/PlexCleaner/discussions +[docker-link]: https://hub.docker.com/r/ptr727/plexcleaner +[github-link]: https://github.com/ptr727/PlexCleaner +[issues-link]: https://github.com/ptr727/PlexCleaner/issues +[license-link]: ./LICENSE +[plexcleaner-hub-link]: https://hub.docker.com/r/ptr727/plexcleaner +[releases-latest-link]: https://github.com/ptr727/PlexCleaner/releases/latest [releases-link]: https://github.com/ptr727/PlexCleaner/releases + + +[awesomeassertions-link]: https://awesomeassertions.org/ +[cliwrap-link]: https://github.com/Tyrrrz/CliWrap +[codexffmpeg-releases-link]: https://github.com/GyanD/codexffmpeg/releases +[demo-world-link]: https://www.demo-world.eu/2d-demo-trailers-hd/ +[dependabot-link]: https://github.com/dependabot +[docker-hub-description-action-link]: https://github.com/marketplace/actions/docker-hub-description +[docker-logging-link]: https://docs.docker.com/config/containers/logging/configure/ +[docker-run-action-link]: https://github.com/marketplace/actions/docker-run-action +[dotnet-counters-link]: https://learn.microsoft.com/dotnet/core/diagnostics/dotnet-counters +[dotnet-download-link]: https://dotnet.microsoft.com/download +[dotnet-install-linux-link]: https://docs.microsoft.com/en-us/dotnet/core/install/linux +[dotnet-install-windows-link]: https://docs.microsoft.com/en-us/dotnet/core/install/windows +[dotnet-monitor-link]: https://learn.microsoft.com/dotnet/core/diagnostics/dotnet-monitor +[dotnet-outdated-link]: https://github.com/dotnet-outdated/dotnet-outdated +[dotnet-publish-link]: https://learn.microsoft.com/en-us/dotnet/core/tools/dotnet-publish +[emby-directplay-link]: https://support.emby.media/support/solutions/articles/44001920144-direct-play-vs-direct-streaming-vs-transcoding +[ffmpeg-link]: https://www.ffmpeg.org/ +[filesystemwatcher-link]: https://docs.microsoft.com/en-us/dotnet/api/system.io.filesystemwatcher +[git-auto-commit-action-link]: https://github.com/marketplace/actions/git-auto-commit +[github-actions-link]: https://github.com/actions +[gsudo-link]: https://github.com/gerardog/gsudo +[handbrake-link]: https://handbrake.fr/ +[handbrake-releases-link]: https://github.com/HandBrake/HandBrake/releases +[husky-link]: https://alirezanet.github.io/Husky.Net/ +[ietf-language-tag-link]: https://en.wikipedia.org/wiki/IETF_language_tag +[iso639-2-link]: https://www.loc.gov/standards/iso639-2/langhome.html +[iso639-3-link]: https://iso639-3.sil.org/ +[jellyfin-playmethod-link]: https://jellyfin.org/docs/plugin-api/MediaBrowser.Model.Session.PlayMethod.html +[jellyfish-link]: http://jell.yfish.us/ +[json2csharp-link]: https://json2csharp.com +[kodi-samples-link]: https://kodi.wiki/view/Samples +[matroska-test-files-link]: https://github.com/ietf-wg-cellar/matroska-test-files +[matroska-test-files-zip-link]: https://github.com/ietf-wg-cellar/matroska-test-files/archive/refs/heads/master.zip +[mediainfo-download-link]: https://mediaarea.net/en/MediaInfo/Download/Windows +[mediainfo-link]: https://mediaarea.net/en-us/MediaInfo/ +[mkvtoolnix-download-link]: https://mkvtoolnix.download/downloads.html#windows +[mkvtoolnix-languages-link]: https://codeberg.org/mbunkus/mkvtoolnix/wiki/Languages-in-Matroska-and-MKVToolNix +[mkvtoolnix-link]: https://mkvtoolnix.download/ +[mplayer-samples-link]: https://samples.mplayerhq.hu/ +[native-aot-link]: https://learn.microsoft.com/en-us/dotnet/core/deploying/native-aot +[nebml-link]: https://github.com/OlegZee/NEbml +[nerdbank-gitversioning-action-link]: https://github.com/marketplace/actions/nerdbank-gitversioning +[plex-directplay-link]: https://support.plex.tv/articles/200250387-streaming-media-direct-play-and-direct-stream/ +[regex101-link]: https://regex101.com/ +[rfc5646-link]: https://www.rfc-editor.org/rfc/rfc5646.html +[serilog-link]: https://serilog.net/ +[sevenzip-download-link]: https://www.7-zip.org/download.html +[sevenzip-link]: https://www.7-zip.org/ [ubuntu-hub-link]: https://hub.docker.com/_/ubuntu [utf8jsonasync-link]: https://github.com/gragra33/Utf8JsonAsyncStreamReader +[visualstudio-link]: https://visualstudio.microsoft.com/downloads/ +[vscode-link]: https://code.visualstudio.com/download +[winget-cli-issue-link]: https://github.com/microsoft/winget-cli/issues/3437 +[xmltocsharp-link]: http://xmltocsharp.azurewebsites.net/ +[xunit-link]: https://xunit.net/ diff --git a/WORKFLOW.md b/WORKFLOW.md index 1af2aa6e..916755dd 100644 --- a/WORKFLOW.md +++ b/WORKFLOW.md @@ -86,7 +86,7 @@ Legibility rules. Necessary but not sufficient: a perfectly styled workflow can a manual dispatch, or back-to-back dispatches against the shared Docker tags) and none is cancelled mid-release. The merge-bot also overrides it: it keys on the PR number with `cancel-in-progress: false` so each PR's events run to completion in order. -- **Shells.** Every multi-line bash `run:` starts with `set -euo pipefail`. +- **Shells.** Every multi-line bash `run:` starts with `set -Eeuo pipefail`. - **Conditionals.** Multi-line `if:` uses the folded scalar `if: >-`. - **Boolean inputs.** A boolean used by both `workflow_call` and `workflow_dispatch` is declared in both trigger blocks and compared against `true` and `'true'`. @@ -94,7 +94,9 @@ Legibility rules. Necessary but not sufficient: a perfectly styled workflow can job needs valid permissions. Grant least privilege; a callee's extra scope is granted by the caller. - **Allowlist `success` and `skipped` explicitly** across an optional dependency: use `(needs.X.result == 'success' || needs.X.result == 'skipped')`, not `!= 'failure'`. -- **Line endings.** Workflow YAML follows [`.editorconfig`](./.editorconfig) (CRLF). Preserve on every edit. +- **Line endings.** Workflow YAML follows [`.editorconfig`](./.editorconfig), which pins + `.github/workflows/*.{yml,yaml}` to **LF** (Dependabot and Actions rewrite these files with LF). Preserve on + every edit. ## 3. Architecture @@ -110,9 +112,10 @@ from racing a publish on the same ref.* A publish builds exactly **one** branch - the run's trigger ref. The **schedule** always runs on the default branch, so it rebuilds `main`; a **dispatch** runs on the branch it is started from (`main` or `develop`). The -single `publish` job passes `github.ref_name` as both `ref` and `branch`, so the branch built, versioned, and -tagged is always the run's own ref. *No matrix and no cross-branch ref mixing - `github.ref` is the branch -being published.* The job is guarded to the long-lived branches (`main` / `develop`); a stray dispatch from a +single `publish` job passes `github.sha` as `ref` and `github.ref_name` as `branch`, so the branch built, +versioned, and tagged is always the run's own ref, pinned to the exact commit the run started from - a push +landing mid-run is never released unvalidated. *No matrix and no cross-branch ref mixing - `github.ref` is the +branch being published.* The job is guarded to the long-lived branches (`main` / `develop`); a stray dispatch from a feature branch is a no-op. To release `develop`, dispatch the workflow from `develop`. Because the run's ref **is** the built branch, GitHub resolves the local `uses: ./...` reusable workflows from @@ -131,12 +134,13 @@ NBGV classifies `publicReleaseRefSpec` from the `GITHUB_REF` environment variabl the **trigger ref** (one branch per run), `GITHUB_REF` already equals the branch being versioned - a schedule or `main` dispatch classifies as public (clean `X.Y.`), a `develop` dispatch as prerelease (`X.Y.-g`) - so no `GITHUB_REF` override is needed. (`IGNORE_GITHUB_REF` is only for matrix -publishers that build a non-trigger branch.) The main-version backstop (D2.2) catches any misclassification. +publishers that build a non-trigger branch.) The release-version gate (D2.2) catches any misclassification. ### Validate at entry -A run that carries a cross-input invariant (e.g. `main` must not carry a prerelease suffix) asserts it once -with `::error::` before any publish. Downstream jobs `needs:` it. +A run that carries a cross-input invariant asserts it once with `::error::` before any build, not after one. +The `validate-release` job is that gate: `main` must not carry a prerelease suffix, and every other branch +must. Downstream jobs `needs:` it. ### Fast CI feedback, head-resolved @@ -211,9 +215,9 @@ flowchart TD ``` **Publish - `publish-release.yml` -> `build-release-task.yml`.** A weekly schedule (rebuilds `main`) or a -dispatch on the started branch validates, versions once with NBGV, builds the per-RID executable 7z and the -multi-arch Docker image, then cuts the GitHub release and pushes to Docker Hub (D2, D3, D4). Both output -sinks are shown. +dispatch on the started branch validates, versions once with NBGV, gates on the branch matching that version's +classification, builds the per-RID executable 7z and the multi-arch Docker image, then cuts the GitHub release +and pushes to Docker Hub (D2, D3, D4). Both output sinks are shown. ```mermaid flowchart TD @@ -226,20 +230,20 @@ flowchart TD VAL["Validate job
(validate-task.yml, branch ref)"] --> VG{"validate succeeded
or skipped?"}:::gate VG -- "failed" --> VFAIL(["build + release skipped"]):::stop GV["Get version job
NBGV @master, runs once
SemVer2 + GitCommitId"] + GV --> VR{"Validate release job
branch vs version classification
(skipped on smoke)"}:::gate + VR -- "mismatch" --> VRX(["fail ::error::
refuse to publish"]):::stop VG -- "ok" --> BE - VG -- "ok" --> BD - GV --> BE - GV --> BD + VR -- "ok" --> BE BE["Build executable job
RID matrix: win-x64, linux-x64,
linux-musl-x64, linux-arm, linux-arm64,
osx-x64, osx-arm64 -> PlexCleaner.7z
release-asset-<branch>-executable"] - BD["Build Docker job
linux/amd64 + linux/arm64
tags: latest|develop + :SemVer2"] + BE --> BD + BD["Build Docker job
linux/amd64 + linux/arm64
tags: latest|develop + :SemVer2
skipped if any earlier build failed"] BD --> DH[("Docker Hub push
ptr727/plexcleaner
latest|develop + :SemVer2 (multi-arch)
+ overview on main")]:::pub BE --> GR BD --> GR - GR{"github-release job
main version clean?
(strip +meta, no '-')"}:::gate - GR -- "main carries prerelease '-'" --> GRX(["fail ::error::
refuse to publish"]):::stop - GR -- "ok" --> EX{"tag exists
and not dispatch?"}:::gate + GR["github-release job"] --> EX{"tag exists
and not dispatch?"}:::gate EX -- "exists, schedule" --> NOP(["skip release-create
artifact reclaimed by backstop"]):::stop EX -- "create or dispatch refresh" --> REL[("GitHub release
tag = SemVer2 at GitCommitId
PlexCleaner.7z + README + LICENSE
prerelease = branch != main")]:::pub + REL --> CLN(["delete release-asset-* artifacts
best-effort, gated to the create"]) end classDef trig fill:#dbeafe,stroke:#2563eb,color:#1e3a8a classDef gate fill:#fef9c3,stroke:#ca8a04,color:#713f12 @@ -277,9 +281,10 @@ Each is a **MUST**, stated as input -> output plus the failure it prevents. - **D0.1 CI is one run, one branch.** Input: any push. Output: `test-pull-request` builds/validates exactly `github.ref_name` and publishes nothing. *Prevents cross-branch ref mixing in CI.* -- **D0.2 The publisher builds one branch: the trigger ref.** Output: the `publish` job passes - `github.ref_name` as `ref` and `branch`, so it checks out, versions, and tags exactly the run's own branch - (the schedule's default branch, or a dispatch's branch). No matrix; the job is guarded to `main`/`develop`. +- **D0.2 The publisher builds one branch: the trigger ref.** Output: the `publish` job passes `github.sha` as + `ref` and `github.ref_name` as `branch`, so it checks out, versions, and tags exactly the commit the run + started from on the run's own branch (the schedule's default branch, or a dispatch's branch). No matrix; the + job is guarded to `main`/`develop`. *Prevents cross-branch ref mixing - `github.ref` is the branch being published.* - **D0.3 One version, threaded.** Output: NBGV runs once, every consumer reads it via `needs:` outputs; no consumer recomputes it. *Allowed:* checking out a specific commit to compile it, and @@ -310,9 +315,11 @@ Each is a **MUST**, stated as input -> output plus the failure it prevents. - **D2.1 Validate before publishing.** Output: a dedicated job/step asserts each cross-input invariant and fails fast with `::error::` before a publish; downstream jobs `needs:` it. -- **D2.2 Main matches version classification.** Input: a real (non-smoke) publish run for `main`. Output: the - release fails loudly if `main` carries a prerelease suffix. It strips `+buildmetadata` before testing for - the prerelease `-`. Skipped on smoke. *Prevents a develop build published as the stable `latest`.* +- **D2.2 Branch matches version classification.** Input: a real (non-smoke) publish run. Output: the dedicated + `validate-release` entry job fails loudly if `main` carries a prerelease suffix **or** a non-`main` branch + carries none. It strips `+buildmetadata` before testing for the prerelease `-` (only a core/prerelease `-` + counts), and it is skipped on smoke (a detached PR head always versions as prerelease). *Prevents a develop + build published as the stable `latest`, a develop leg classified public, and a build-metadata false positive.* ### D3 - Versioning and classification @@ -337,8 +344,9 @@ Each is a **MUST**, stated as input -> output plus the failure it prevents. targets and creates the GitHub release for `github.ref_name` - the schedule rebuilds `main` (stable / `latest`); a dispatch publishes its own branch (`main` stable / `latest`, `develop` prerelease / `develop`). *Prevents a half-published release set and cross-branch ref mixing.* -- **D4.3 Tag the built commit.** Output: the release `target_commitish` is the run's `GitCommitId` (the tip of - `github.ref_name`), never `github.sha`. *Prevents the tag landing on the wrong commit.* +- **D4.3 Tag the built commit.** Output: the release `target_commitish` is the run's `GitCommitId` - the commit + NBGV versioned - never a branch name or a separately re-resolved ref. *Prevents the tag landing on the wrong + commit.* - **D4.4 Release contents and flag.** Output: each release is a tag on the built commit plus the auto source zip, README, and LICENSE, with the multi-runtime `PlexCleaner.7z` attached via the `release-asset-*` seam. The GitHub-release `prerelease` boolean is `inputs.branch != 'main'`. The Docker target attaches no asset; @@ -368,6 +376,14 @@ Each is a **MUST**, stated as input -> output plus the failure it prevents. - **D5.2 Transfer artifacts consumed by pattern.** The `github-release` job collects `release-asset--*` by glob, so adding/removing a file-producing target needs no release-job edit. - **D5.3 Never blanket-delete.** Cleanup MUST NOT enumerate and delete the run's whole artifact set. +- **D5.4 Delete at the point of consumption, gated to the consumer, best-effort.** The `github-release` job + deletes the `release-asset--*` artifacts by exact pattern once they are attached to the release, + under the **same** condition as the release-create step - so a no-op re-run that skips the create also skips + the delete, and the `retention-days: 1` backstop reclaims those artifacts instead. The step is + `continue-on-error`, tolerates a failed listing, and deletes every matching id. It needs `actions: write`, + granted by the caller (`publish-release.yml`'s publish job). *Prevents transfer artifacts accumulating + against the storage quota, freshly built assets being deleted on a no-op re-run, and a cleanup hiccup + reddening a job whose publish succeeded.* ### D6 - Self-testing workflows @@ -427,24 +443,27 @@ Each is a **MUST**, stated as input -> output plus the failure it prevents. Read the workflow files plus `version.json` and assert the fact behind each applicable guarantee with a `file:line` citation: -- **D0:** CI has no branch matrix; the publisher's single `publish` job passes `github.ref_name` as - `ref`/`branch` and is guarded to `main`/`develop`; NBGV invoked once, every other consumer reads it via - `needs:`; the run builds the trigger ref so `GITHUB_REF` matches the versioned branch. +- **D0:** CI has no branch matrix; the publisher's single `publish` job passes `github.sha` as `ref` and + `github.ref_name` as `branch` and is guarded to `main`/`develop`; NBGV invoked once, every other consumer + reads it via `needs:`; the run builds the trigger ref so `GITHUB_REF` matches the versioned branch. - **D1:** CI runs on `push` with no paths filter; `validate` + `smoke-build` (both targets, `smoke: true`) run; every build `upload-artifact` is gated `!smoke`; `lint` runs CSharpier, `dotnet format style`, markdownlint, cspell on README/HISTORY, actionlint; the aggregator `needs:` both and blocks on non-success. -- **D2:** the main release backstop checks the prerelease `-`, strips `+buildmetadata`, self-skips on smoke. -- **D3:** `main` appears in the backstop and the `prerelease` expression; `publicReleaseRefSpec` is +- **D2:** a dedicated `validate-release` job runs before the build jobs and they `needs:` it; it checks both + arms (main without a prerelease `-`, every other branch with one), strips `+buildmetadata`, self-skips on + smoke. +- **D3:** `main` appears in the release-version gate and the `prerelease` expression; `publicReleaseRefSpec` is `^refs/heads/main$`. - **D4:** `publish-release` triggers are `schedule` + `workflow_dispatch` only (no `push`, no `PUBLISH_ON_MERGE`); the single `publish` job is guarded to `github.ref_name` in (`main`, `develop`) and - passes it as `ref`/`branch`; `target_commitish` is `GitCommitId`; the + passes `github.sha` as `ref` and `github.ref_name` as `branch`; `target_commitish` is `GitCommitId`; the `prerelease` boolean `== (inputs.branch != 'main')`; the executable attaches `PlexCleaner.7z` via `release-asset-*`; the Docker job logs in with `DOCKER_HUB_*` and pushes `latest`/`develop` + `:SemVer2`; release-create gated `exists == false || workflow_dispatch`; Docker buildcache is branch-scoped and write-gated on push; the Docker Hub overview push is gated to `main`. - **D5:** every upload sets `retention-days: 1`; the release job collects `release-asset--*` by - pattern; no blanket artifact delete. + pattern and deletes those same artifacts by pattern under the release-create condition, `continue-on-error`; + the caller grants `actions: write`; no blanket artifact delete. - **D6:** CI is `push` on every branch; the aggregator context has exactly one producer; no `pull_request`-triggered fallback. - **D7:** the publisher group is ref-independent with `cancel-in-progress: false`; the merge-bot keys on PR @@ -458,12 +477,12 @@ Read the workflow files plus `version.json` and assert the fact behind each appl | # | Input | Expected output | Exercises | | --- | --- | --- | --- | -| S1 | push touching `PlexCleaner/**` | `validate` + `smoke-build` run; both targets compile/pack, **no push, no uploads, no release**; aggregator success; no dangling artifacts | D0.1, D1 | +| S1 | push touching `PlexCleaner/**` | `validate` + `smoke-build` run; both targets compile/pack, **no push, no uploads, no release**; `validate-release` self-skips (smoke); aggregator success; no dangling artifacts | D0.1, D1, D2.2 | | S2 | push changing only docs | `validate` (lint checks markdown) + `smoke-build` run; nothing publishes | D1, D1.5 | | S3 | push changing only `.github/workflows/**` | `smoke-build` exercises the changed reusable workflow head-resolved; `lint` runs actionlint; aggregator success | D1.1, D6.1 | | S4 | weekly `schedule` | builds + publishes `main` only: stable release + refreshed `latest` (multi-arch) + `PlexCleaner.7z`; `target_commitish` = main's SHA; develop is not touched; no dangling artifacts | D4.1, D4.2, D4.4 | | S5 | `workflow_dispatch` from `develop` | builds + publishes `develop`: prerelease `X.Y.-g` + `develop` image + `PlexCleaner.7z`; `github.ref` is develop, so NBGV classifies it non-public | D4.1, D4.2, D3.2 | -| S6 | `workflow_dispatch` re-run, no new commits | release-create refreshed on dispatch (or skipped if the tag exists on schedule); Docker re-pushed (base refresh); no duplicate release | D4.5 | +| S6 | `workflow_dispatch` re-run, no new commits | release-create refreshed on dispatch (or skipped if the tag exists on schedule); the `release-asset-*` delete is gated to the create, so a skipped create leaves them to the retention backstop; Docker re-pushed (base refresh); no duplicate release | D4.5, D5.4 | | S7 | `workflow_dispatch` from a feature branch | the `publish` job's `github.ref_name in (main, develop)` guard skips it -> no publish | D4.1 | | S8 | merged dependency bump (any) | `Directory.Packages.props` is not a shipped input and merges don't publish -> **no release**; ships in the next weekly run | D4.1, D8.2 | | S9 | merged GitHub-Actions bump | not a shipped input, merges don't publish -> **no release** | D4.1 | @@ -471,6 +490,7 @@ Read the workflow files plus `version.json` and assert the fact behind each appl | S11 | `version.json` floor bump merged | merges don't publish -> no immediate release; the new floor ships in the next weekly publish | D3.3, D4.1 | | S12 | Dependabot semver-major bump (any ecosystem) | auto-merges on green like every other tier; the required checks are the gate | D8.2 | | S13 | `develop` -> `main` promotion (merge commit) | the merge itself does not publish; `main`'s accumulated changes ship in the next weekly run | D4.1, D8.1 | +| S14 | branch and version classification disagree (NBGV mis-classifies) | `validate-release` **fails loud** with `::error::`; the build and publish jobs skip, so nothing is built or pushed | D2.2 | ### 5C. Live probe (where warranted, never publishing) diff --git a/repo-config/README.md b/repo-config/README.md index 658fdd07..288228be 100644 --- a/repo-config/README.md +++ b/repo-config/README.md @@ -4,7 +4,7 @@ Repository and branch configuration held as committed files, kept out of `.githu - `main.json` plus one `develop` variant - the branch rulesets as the writable API subset (`name`, `target`, `enforcement`, `bypass_actors`, `conditions`, `rules`). The `develop` payload is `develop.json` (`release` repos) or `operational/develop.json` (`operational` repos); the hub keeps both, a carried copy only its own model's (see "Downstream Carry"). These are the canonical expected payloads that the audit (the hub's fleet-wide `AUDIT.md`, or a carried repo-scoped adaptation - see "Downstream Carry") diffs the live rulesets against. - `operational/develop.json` - the `develop` ruleset for **operational** repos (registry `workflowModel: operational`): direct signed pushes, no PR gate. Present at the hub and in operational carries only - a carried `release` repo does not have it. See "Rulesets" below. -- `configure.sh` - applies the rulesets to a repository via the GitHub API (create or full-payload update, idempotent). Run `repo-config/configure.sh [owner/repo] [release|operational]`; the model defaults to the registry `workflowModel` lookup. +- `configure.sh` - two modes over the GitHub API. `configure.sh apply [owner/repo] [release|operational]` creates-or-updates the settings, the Dependabot security features, and the rulesets idempotently (a full-payload update). `configure.sh check [owner/repo] [release|operational]` is the read-only inverse and exits non-zero on any drift, with the ruleset and settings assertions driven by the committed payloads so they stay repo-agnostic (rule presence, merge methods, and required checks, not a byte diff - so a GitHub-normalized stored ruleset does not false-positive). The command defaults to `apply`, the repo to the current one, and the model to the registry `workflowModel` lookup (or, absent a registry, inference from the carried `develop` payload - an ambiguous layout aborts rather than guesses). The model may be passed as the sole positional (`configure.sh check operational`). ## Downstream Carry @@ -47,7 +47,7 @@ Publish credentials required per mechanism are enumerated in `spec/secrets.json` ## Repo Settings -The fleet-standard general settings live in [`settings.json`][settings-json] and are applied idempotently by `configure.sh` alongside the rulesets (`gh api PATCH /repos/{owner}/{repo}`). The two settings that depend on per-repo state - `has_discussions` (visibility) and `default_branch` (main-must-exist) - are computed by the script, not stored in the file. +The fleet-standard general settings live in [`settings.json`][settings-json] and are applied idempotently by `configure.sh apply` alongside the rulesets (`gh api PATCH /repos/{owner}/{repo}`). The two settings that depend on per-repo state - `has_discussions` (visibility) and `default_branch` (main-must-exist) - are computed by the script, not stored in the file. `configure.sh apply` also enables Dependabot vulnerability alerts and automated security updates - fleet policy applied via the API, not a `settings.json` key. `configure.sh check` validates all of these and exits non-zero on drift. - **Default branch `main`** (the script sets it only when a `main` branch exists, never pointing the default at a missing branch). - **Merge methods**: `Allow merge commits` and `Allow squash merging` on, **rebase off** - each branch ruleset then picks its method (merge on `main`, squash on `develop`). diff --git a/repo-config/configure.sh b/repo-config/configure.sh index 6e2b228c..e42a5141 100755 --- a/repo-config/configure.sh +++ b/repo-config/configure.sh @@ -1,39 +1,60 @@ #!/usr/bin/env bash -# Apply the committed fleet configuration in this directory to the repository via the GitHub API: -# 1. General repository settings from settings.json (PATCH /repos/{owner}/{repo}), plus the two settings that depend on -# per-repo state - has_discussions (public repos only) and default_branch (main, only if it exists). -# 2. The branch rulesets. main.json is shared by both workflow models; the develop ruleset is model-specific - -# release repos use develop.json (PR-gated), operational repos use operational/develop.json (direct signed -# pushes). The model is read from ../registry/repos.json (per-repo workflowModel, else defaults.workflowModel, -# else release) and can be overridden with the second argument. Each .json holds the writable ruleset -# subset {name, target, enforcement, bypass_actors, conditions, rules}. An existing ruleset (matched by name) -# is updated with a full-payload PUT (partial PUTs 422); a missing one is created with POST. -# Rerunning is idempotent. +# Configure or validate a repository against the committed fleet config in this directory, via the GitHub API. # -# Usage: repo-config/configure.sh [owner/repo] [release|operational] (repo defaults to the current repo via gh; -# model defaults to the registry lookup) -set -euo pipefail +# repo-config/configure.sh apply [owner/repo] [release|operational] # create-or-update settings + rulesets (writes) +# repo-config/configure.sh check [owner/repo] [release|operational] # validate an existing repo, non-zero on drift (reads) +# +# Both modes need admin on the repo (the rulesets endpoints require it). The command defaults to apply, the repo +# to the current gh repo, and the model to the registry lookup (else inferred from the carried develop payload). +# The model may be passed as the sole positional (e.g. `configure.sh check operational`), and the command may be +# omitted for the apply default (`configure.sh owner/repo` still applies). +# +# apply: (1) settings.json via PATCH, plus has_discussions (public repos only) and default_branch (main, only if +# it exists). (2) Dependabot vulnerability alerts + automated security updates. (3) the branch rulesets - +# main.json (shared) and the model-specific develop ruleset (develop.json PR-gated, or operational/ +# develop.json direct signed pushes), create-or-update by name. Idempotent. +# check: the read-only inverse - every applied ruleset, setting, and security feature must match. The ruleset and +# settings assertions are driven by the committed payloads, so they stay repo-agnostic and survive the +# GitHub API normalizing a stored ruleset (rule presence + merge methods + required checks, not a byte +# diff). Secrets are per-repo (see spec/secrets.json) and not checkable from a standalone carry, so they +# are a manual-verify note. +set -Eeuo pipefail -repo="${1:-$(gh repo view --json nameWithOwner --jq '.nameWithOwner')}" +# ----- Command + target + model ----- +cmd=apply +case "${1:-}" in apply|check) cmd="$1"; shift ;; esac +repo_arg="${1:-}" +model="${2:-}" +# Allow the model as the sole positional (`configure.sh check operational`): a model name is not a repo. +case "$repo_arg" in release|operational) model="$repo_arg"; repo_arg="" ;; esac +repo="${repo_arg:-$(gh repo view --json nameWithOwner --jq '.nameWithOwner')}" script_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -# ----- Resolve the workflow model (selects the develop ruleset) ----- +# ----- Resolve the workflow model (selects the develop ruleset), shared by apply and check ----- registry="$script_dir/../registry/repos.json" name="${repo##*/}" -model="${2:-}" if [ -z "$model" ]; then if [ -f "$registry" ]; then - # Fail fast on a jq/parse error (malformed registry) instead of silently applying the release default - # to a repo whose lookup actually broke. A repo simply absent from the registry is not an error: the + # Fail fast on a jq/parse error (malformed registry) instead of silently applying the release default to + # a repo whose lookup actually broke. A repo simply absent from the registry is not an error: the # expression falls back through defaults.workflowModel to "release", so jq still exits 0 with a value. if ! model="$(jq -r --arg n "$name" '(.repos[] | select(.name==$n) | .workflowModel) // .defaults.workflowModel // "release"' "$registry")"; then - echo "Failed to read workflowModel from $registry (invalid JSON?). Pass the model explicitly as arg 2." >&2 + echo "Failed to read workflowModel from $registry (invalid JSON?). Pass the model explicitly (release|operational)." >&2 exit 1 fi else - # No registry to consult (e.g. running the script standalone) - default, but say so. - echo "Registry $registry not found; defaulting workflow model to release." >&2 - model="release" + # No registry to consult (a downstream carry): infer the model from which develop payload is carried - a + # carry holds exactly its own model's payload. Ambiguous layouts (both or neither, e.g. a partial copy) + # abort rather than guess - a wrong guess would apply or check the wrong develop ruleset. + if [ -f "$script_dir/develop.json" ] && [ ! -f "$script_dir/operational/develop.json" ]; then + model="release" + elif [ -f "$script_dir/operational/develop.json" ] && [ ! -f "$script_dir/develop.json" ]; then + model="operational" + else + echo "Registry $registry not found and the carried develop payloads are ambiguous (expected exactly one of develop.json or operational/develop.json). Pass the model explicitly (release|operational)." >&2 + exit 1 + fi + echo "Registry $registry not found. Inferred workflow model '$model' from the carried develop payload." >&2 fi fi case "$model" in @@ -41,12 +62,76 @@ case "$model" in operational) develop_ruleset="$script_dir/operational/develop.json" ;; *) echo "Unknown workflow model '$model' (expected release or operational)." >&2; exit 1 ;; esac -echo "Workflow model for $repo: $model" - -# ----- General repository settings ----- +main_ruleset="$script_dir/main.json" settings_file="$script_dir/settings.json" -if [ -e "$settings_file" ]; then - # has_discussions: enabled on public repos only (fleet policy); never on private. + +# ----- Ruleset id lookup (shared by apply and check) ----- +ruleset_id() { # ruleset-name -> id of the first match (empty if none). Warns on duplicates. Aborts on an API error or at the per_page cap (the single-fetch lookup would be unreliable). + local out ids count + # per_page=100 returns every ruleset in one array (a repo has only a handful), so the response is a single + # JSON document - a paginated fetch would concatenate multiple arrays and break the single-array jq below. + # Let gh print its own error on stderr. Add a context line and return non-zero so the caller stops rather + # than treat an API failure as "not found". + if ! out="$(gh api "repos/$repo/rulesets?per_page=100")"; then + echo "Failed to list rulesets for $repo (check auth and repo access)." >&2 + return 1 + fi + # Fail loud rather than silently narrow: a full page means the single-fetch assumption no longer holds, and + # a missed lookup would make apply create a duplicate ruleset by name. Abort so the caller stops (it treats a + # non-zero return as "stop", never as "not found"). + if [ "$(jq 'length' <<<"$out")" -eq 100 ]; then + echo "Failed for $repo: 100 rulesets returned (the per_page cap), so the single-fetch lookup is unreliable. Reduce rulesets or add pagination before applying." >&2 + return 1 + fi + # shellcheck disable=SC2016 # $n is a jq --arg variable, not a shell expansion + ids="$(jq -r --arg n "$1" '.[] | select(.name==$n) | .id' <<<"$out")" + if [ -z "$ids" ]; then return 0; fi + # Pre-existing drift can leave more than one ruleset with the same name. Use the first and warn so the + # duplicates are resolved rather than silently operating on the wrong one. grep -c and sed both read all + # input (no early pipe close), so neither SIGPIPEs jq under pipefail. + count="$(printf '%s\n' "$ids" | grep -c .)" + if [ "$count" -gt 1 ]; then + echo "Warning: $count rulesets named '$1' on $repo. Using the first (resolve the duplicates)." >&2 + fi + printf '%s\n' "$ids" | sed -n '1p' +} + +# =============================== apply =============================== +apply_ruleset() { # payload-file - create-or-update the ruleset by name + local file="$1" rname id + if [ ! -e "$file" ]; then + echo "Ruleset payload $file not found. Aborting to avoid a partially-applied configuration." >&2 + exit 1 + fi + rname="$(jq -r '.name // empty' "$file")" + if [ -z "$rname" ]; then + echo "Ruleset payload $file has no name. Aborting to avoid a partially-applied configuration." >&2 + exit 1 + fi + id="$(ruleset_id "$rname")" + if [ -n "$id" ]; then + echo "Updating ruleset '$rname' (id $id) on $repo" + gh api --method PUT "repos/$repo/rulesets/$id" --input "$file" >/dev/null + else + echo "Creating ruleset '$rname' on $repo" + gh api --method POST "repos/$repo/rulesets" --input "$file" >/dev/null + fi +} + +cmd_apply() { + local f private disc payload + # Pre-flight every required payload before any write, so a partial carry aborts before it half-applies. + for f in "$settings_file" "$develop_ruleset" "$main_ruleset"; do + if [ ! -e "$f" ]; then + echo "Required payload $f not found. Aborting to avoid a partially-applied configuration." >&2 + exit 1 + fi + done + echo "Applying configuration to $repo (model: $model)" + # The writes below silence stdout only (the success-response JSON is noise). They still fail loud - + # gh errors go to stderr and a failed write aborts the script (these writes run unguarded under `set -e`). + # ----- General repository settings ----- + # has_discussions: enabled on public repos only (fleet policy), never on private. private="$(gh api "repos/$repo" --jq '.private')" disc=false; [ "$private" = "false" ] && disc=true # default_branch main, but only point it at main when main exists - never set the default to a missing @@ -55,48 +140,114 @@ if [ -e "$settings_file" ]; then payload="$(jq --argjson d "$disc" '. + {has_discussions: $d, default_branch: "main"}' "$settings_file")" else payload="$(jq --argjson d "$disc" '. + {has_discussions: $d}' "$settings_file")" - echo "Warning: $repo has no 'main' branch; leaving default_branch unchanged." >&2 + echo "Warning: $repo has no 'main' branch. Leaving default_branch unchanged." >&2 fi - echo "Applying general settings to $repo (has_discussions=$disc)" + echo "Applying general settings (has_discussions=$disc)" printf '%s' "$payload" | gh api --method PATCH "repos/$repo" --input - >/dev/null -fi + # ----- Dependabot alerts + automated security updates ----- + gh api --method PUT "repos/$repo/vulnerability-alerts" >/dev/null + gh api --method PUT "repos/$repo/automated-security-fixes" >/dev/null + echo "Enabled Dependabot vulnerability alerts + automated security updates" + # ----- Branch rulesets (main shared, develop selected by workflow model) ----- + apply_ruleset "$develop_ruleset" + apply_ruleset "$main_ruleset" + echo "Configuration applied to $repo. Run '$0 check${repo_arg:+ $repo}' to validate." +} -# ----- Branch rulesets ----- -# main.json is shared; the develop ruleset was selected by workflow model above. A missing or nameless -# payload aborts - silently skipping it would report success on a partially-applied configuration. -for file in "$develop_ruleset" "$script_dir/main.json"; do - if [ ! -e "$file" ]; then - echo "Ruleset payload $file not found; aborting to avoid a partially-applied configuration." >&2 - exit 1 +# =============================== check =============================== +FAILED=0 +note() { printf ' %s\n' "$*"; } +pass() { printf ' ok %s\n' "$*"; } +fail() { printf ' FAIL %s\n' "$*"; FAILED=1; } + +# assert MESSAGE TEST... - run the test command, pass on success, fail on non-zero (a proper if/else, not the +# `A && B || C` footgun). Do not redirect the assert call's own stdout - that would swallow the pass/fail line; +# a command that prints (jq) uses jq_has, which silences only itself. +assert() { local msg="$1"; shift; if "$@"; then pass "$msg"; else fail "$msg"; fi; } + +# jq_has FILTER... - true iff the filter selects a truthy value. jq's output is discarded, not the caller's. +# Reads JSON from stdin. +jq_has() { jq -e "$@" >/dev/null 2>&1; } + +# gh_ok ENDPOINT... - true iff the gh api call succeeds (2xx, including 204). Output and errors discarded, so it +# is safe to pass to assert (e.g. vulnerability-alerts returns 204 enabled / 404 disabled). +gh_ok() { gh api "$@" >/dev/null 2>&1; } + +check_ruleset() { # payload-file - the live ruleset must match the committed policy, driven by the payload + local file="$1" rname id live t want got wantc gotc want_enf + rname="$(jq -r '.name // empty' "$file")" + if [ -z "$rname" ]; then fail "ruleset payload $file has no name"; return; fi + if ! id="$(ruleset_id "$rname")"; then fail "ruleset '$rname' - could not resolve id"; return; fi + if [ -z "$id" ]; then fail "ruleset '$rname' missing"; return; fi + if ! live="$(gh api "repos/$repo/rulesets/$id")"; then fail "ruleset '$rname' - could not read live state"; return; fi + want_enf="$(jq -r '.enforcement' "$file")" + assert "ruleset '$rname' enforcement = $want_enf" test "$(jq -r '.enforcement' <<<"$live")" = "$want_enf" + # Every rule type the committed payload declares must be present live (payload-driven, so repo-agnostic). + while IFS= read -r t; do + # shellcheck disable=SC2016 # $t is a jq --arg variable, not a shell expansion + assert "'$rname' enforces rule '$t'" jq_has --arg t "$t" '.rules[] | select(.type==$t)' <<<"$live" + done < <(jq -r '.rules[].type' "$file") + # pull_request: the live merge methods must match the payload (the develop=squash / main=merge policy). + if jq_has '.rules[] | select(.type=="pull_request")' "$file"; then + want="$(jq -c '[.rules[]|select(.type=="pull_request").parameters.allowed_merge_methods[]]|sort' "$file")" + got="$(jq -c '[.rules[]|select(.type=="pull_request").parameters.allowed_merge_methods[]]|sort' <<<"$live")" + assert "'$rname' merge methods = $want" test "$got" = "$want" fi - ruleset_name="$(jq -r '.name // empty' "$file")" - if [ -z "$ruleset_name" ]; then - echo "Ruleset payload $file has no name; aborting to avoid a partially-applied configuration." >&2 - exit 1 + # required_status_checks: the live required contexts must match the payload. + if jq_has '.rules[] | select(.type=="required_status_checks")' "$file"; then + wantc="$(jq -c '[.rules[]|select(.type=="required_status_checks").parameters.required_status_checks[].context]|sort' "$file")" + gotc="$(jq -c '[.rules[]|select(.type=="required_status_checks").parameters.required_status_checks[].context]|sort' <<<"$live")" + assert "'$rname' required checks = $wantc" test "$gotc" = "$wantc" fi - # Paginate so a name match on a later page is never missed (which would create a duplicate ruleset), and - # fail loudly if the API call itself fails (auth/404/network) rather than treating it as "not found". - if ! ids="$(gh api --paginate "repos/$repo/rulesets" --jq ".[] | select(.name==\"$ruleset_name\") | .id")"; then - echo "Failed to list rulesets for $repo (check auth and repo access)." >&2 - exit 1 - fi - # Pre-existing drift can leave more than one ruleset with the same name; update the first and warn. Guard - # on non-empty so `grep -c` (which exits non-zero on empty input under `set -e`) can't abort the create path. - id="" - if [ -n "$ids" ]; then - count="$(printf '%s\n' "$ids" | grep -c .)" - if [ "$count" -gt 1 ]; then - echo "Warning: $count rulesets named '$ruleset_name' on $repo; updating the first (resolve the duplicates)." >&2 - fi - id="$(printf '%s\n' "$ids" | sed -n '1p')" +} + +check_settings() { + local live key want got private wantdisc + if [ ! -e "$settings_file" ]; then fail "settings payload $settings_file missing"; return; fi + if ! live="$(gh api "repos/$repo")"; then fail "could not read repository settings"; return; fi + # Static settings, driven from settings.json so the check never drifts from the file - add a key there and + # it is audited here automatically. + while IFS=$'\t' read -r key want; do + # shellcheck disable=SC2016 # $k is a jq --arg variable, not a shell expansion + got="$(jq -r --arg k "$key" '.[$k]' <<<"$live")" + assert "setting $key = $want" test "$got" = "$want" + done < <(jq -r 'to_entries[] | "\(.key)\t\(.value)"' "$settings_file") + # Dynamic settings apply sets: has_discussions (public repos only), default_branch (main, if it exists). + private="$(jq -r '.private' <<<"$live")" + wantdisc=true; [ "$private" = "true" ] && wantdisc=false + assert "has_discussions = $wantdisc" test "$(jq -r '.has_discussions' <<<"$live")" = "$wantdisc" + if gh api "repos/$repo/branches/main" --jq '.name' >/dev/null 2>&1; then + assert "default_branch = main" test "$(jq -r '.default_branch' <<<"$live")" = main fi - if [ -n "$id" ]; then - echo "Updating ruleset '$ruleset_name' (id $id) on $repo" - gh api --method PUT "repos/$repo/rulesets/$id" --input "$file" >/dev/null +} + +check_security() { + local sec + # vulnerability-alerts: 204 enabled / 404 disabled, so probe with gh_ok. automated-security-fixes returns + # a JSON body { enabled, paused }, captured under an explicit failure guard so a read error is a clean + # FAIL rather than a set -e abort. + assert "Dependabot vulnerability alerts enabled" gh_ok "repos/$repo/vulnerability-alerts" + if sec="$(gh api "repos/$repo/automated-security-fixes" 2>/dev/null)"; then + assert "Dependabot automated security updates enabled" jq_has '.enabled == true' <<<"$sec" else - echo "Creating ruleset '$ruleset_name' on $repo" - gh api --method POST "repos/$repo/rulesets" --input "$file" >/dev/null + fail "Dependabot automated security updates - could not read the setting" fi -done +} -echo "Configuration applied to $repo" +cmd_check() { + echo "Validating configuration for $repo (model: $model)" + check_ruleset "$develop_ruleset" + check_ruleset "$main_ruleset" + check_settings + check_security + # Secrets are per-repo (spec/secrets.json) and not readable by value. A standalone carry has no registry to + # derive the required set from, so they are verified by hand rather than asserted here. + note "verify manually: the repo's required secrets (see spec/secrets.json) are present with valid values" + if [ "$FAILED" -ne 0 ]; then echo "Configuration drift detected on $repo."; exit 1; fi + echo "Configuration matches on $repo." +} + +case "$cmd" in + apply) cmd_apply ;; + check) cmd_check ;; +esac diff --git a/version.json b/version.json index 8c723482..329e7634 100644 --- a/version.json +++ b/version.json @@ -1,6 +1,6 @@ { "$schema": "https://raw.githubusercontent.com/dotnet/Nerdbank.GitVersioning/master/src/NerdBank.GitVersioning/version.schema.json", - "version": "3.21", + "version": "3.22", "publicReleaseRefSpec": [ "^refs/heads/main$" ],