From 8d9a95bd5b7472c622d0e7ef61eb6904afe5495d Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 20:48:47 -0700 Subject: [PATCH 1/2] Converge copilot-instructions.md and remaining intent files with hub Rewrite .github/copilot-instructions.md's four declared sections to the current hub canonical (the review runbook shrank substantially, its mechanics now delegated to scripts/pr_review.py). Move this file's nine undeclared, repo-specific sections (architecture, patterns, processing pipeline, per-command implementation detail, test layout) into a new ARCHITECTURE.md, the destination spec/section-model.md declares for this content; drop the 'Command Line Usage' section outright as a duplicate of README.md's own Usage section. Preserve the (currently empty) Disproved Claims ledger. Fix a round of real staleness bugs review caught along the way, several pre-dating this resync: nine in the newly-extracted ARCHITECTURE.md (dead file/test references, an incomplete extensions list, wrong video conversion tiers, a wrong JSON context name, a stale trim pattern, a broken relative link, hard-coded test counts already drifted twice), plus WORKFLOW.md's D1.1/D1.4/D1.5/D4.1 clauses and their S1/S2/S3/S5/S6 scenario rows, which described a paths-filter/plan-job shape this repo had already migrated away from (verified directly against the current test-pull-request.yml and publish-release.yml), and AGENTS.md's own preamble, which still routed architecture content at .github/copilot-instructions.md after this same change moved it. Re-vendor CODESTYLE.md (repoint two dead GOVERNANCE.md 'Running the Linters Locally' links at OPERATIONS.md 'Local Verification', which now holds that content since that section moved hub-only; adopt the hub's stronger pre-commit-gate wording; keep the repo's own 'The Full Post-Change Set' subsection) and cspell.json's fleet word list (opencode, Qodo added; nugetlibrary, pypilibrary dropped, matching the WORKFLOW.md artifact rename). Rebuilt as a single commit on the current develop tip after the earlier PR's multi-commit history (originally stacked on the instruction-set PR) stopped applying once that PR's squash-merge rewrote its commits; the diff is unchanged, verified against the prior branch tip. Audit run 2026-08-29T01:09:26Z | hub b823bc4 --- .github/copilot-instructions.md | 668 ++------------------------------ AGENTS.md | 2 +- ARCHITECTURE.md | 231 +++++++++++ CODESTYLE.md | 8 +- GOVERNANCE.md | 2 +- WORKFLOW.md | 68 ++-- cspell.json | 4 +- 7 files changed, 309 insertions(+), 674 deletions(-) create mode 100644 ARCHITECTURE.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 0a5912c..a96ef3e 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,324 +1,56 @@ # Copilot Instructions -Repository conventions for GitHub Copilot (and any other AI agent reading this file). +Repository-wide instructions for GitHub Copilot. -Read [AGENTS.md](../AGENTS.md) first. When performing code review, load and follow the -`code-review` skill in `.github/skills/code-review/SKILL.md`, then load every language, -documentation, or workflow skill it selects for the changed files. GitHub Copilot reads these -files from the pull request's head branch, so review the instructions in that tree. +Read [AGENTS.md](../AGENTS.md) first. It routes every standing repository rule to its canonical +document. When performing code review, load and follow the `code-review` skill in +`.github/skills/code-review/SKILL.md`, then load every language, documentation, or workflow skill +that it selects for the changed files. GitHub Copilot reads these files from the pull request's +head branch, so review the instructions in that tree. -The **canonical guide is [AGENTS.md](../AGENTS.md)** at the repo root. Read it first, then the [PR Review Etiquette](../GOVERNANCE.md#pr-review-etiquette) review-loop contract this file's runbook implements. This file is intentionally narrow: commit/PR-title conventions (summarized inline so VS Code's commit-message and PR-title generators have them), guidance for reviewing carried fleet content, plus the GitHub Copilot Review Runbook. - -For code-style rules, see [`CODESTYLE.md`](../CODESTYLE.md) at the repo root, one guide with a General section plus a section per language the repo uses. - -Do not duplicate language-specific rules here. **Project-specific conventions and API/behavioral contracts also belong in [GOVERNANCE.md](../GOVERNANCE.md), not here.** This file is intentionally limited to the inline commit/PR-title summary, the guidance for reviewing carried fleet content, and the GitHub Copilot Review Runbook. Non-Copilot agents (Claude Code, Codex, Cursor, ...) are not directed to this file and don't read it by default, so any rule a reviewer must honor has to live in `GOVERNANCE.md`, routed to from `AGENTS.md`, to be provider-independent. +Do not duplicate rules from `AGENTS.md`, `GOVERNANCE.md`, `CODESTYLE.md`, or `WORKFLOW.md` here. +This file contains only Copilot-specific bootstrap and output requirements. This project's +architecture, processing pipeline, and per-command implementation details live in +[`ARCHITECTURE.md`](../ARCHITECTURE.md). ## Commit Messages and Pull Request Titles -Summarized for VS Code's generators. The full rules, rationale, and examples are in [GOVERNANCE.md "Pull Request Title and Commit Message Conventions"](../GOVERNANCE.md#pull-request-title-and-commit-message-conventions). - -- Imperative subject, <= 72 characters, no trailing period, with an optional blank-line-separated body for the non-obvious *why*. -- US English, title case with lowercase short bind words. No vague titles, no `Co-Authored-By:` unless asked, no release-bump magnitude (NBGV handles versioning). Dependabot's `Bump X from Y to Z` titles are fine. -- develop PRs squash-merge (`gh pr merge --squash`), main PRs merge-commit (`--merge`). A mismatched flag is rejected by branch protection. +Use an imperative subject of at most 72 characters with no trailing period. Use US English and +title case with lowercase short bind words. Do not add `Co-Authored-By:` unless requested. Do not +put a release-bump magnitude in the title. The full contract is in +[GOVERNANCE.md "Pull Request Title and Commit Message Conventions"](../GOVERNANCE.md#pull-request-title-and-commit-message-conventions). ## 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 `GOVERNANCE.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. `AGENTS.md` is the thin router and carries three byte-locked sections of its own, with no repository-specific ones. - -Three 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, but note that the fix lands at the template and re-vendors to every repository, rather than proposing a local edit the audit would reject. -- **A reference to a hub script is a pointer to follow, not a broken local path.** The fleet's gates live in one place and a repository runs them from a checkout of that place rather than holding a copy, so `scripts/prose_lint.py` (prose the CI linters pass on), `scripts/repo_gate.py` (repository settings and action pins), `scripts/pr_review.py` (the review digest, and reply plus resolve without a hand-typed id), and `spec/audit.py` (the conformance audit) resolve there and in none of the repositories they measure. [GOVERNANCE.md "Documentation Style Conventions"](../GOVERNANCE.md#documentation-style-conventions) carries the exception that permits such a pointer inside carried text, and [GOVERNANCE.md "Hub-Hosted Tooling"](../GOVERNANCE.md#hub-hosted-tooling) states how one is reached and what to report when it cannot be. Reach for them before writing a check of your own, since a reconstructed gate encodes its author's reading of a rule rather than the rule, and agrees with no other repository. +Follow the fidelity declared for the file. A byte-locked reference to shared infrastructure that +this repository does not carry is intentional, not a broken link. Raise substantive defects in +canonical content, but locate the fix at its canonical source instead of proposing a local edit +that its fidelity rejects. ## GitHub Copilot Review Runbook -> This runbook implements the [GOVERNANCE.md "PR Review Etiquette"](../GOVERNANCE.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, fill the `` / `` / `` placeholders. - -Use this section for provider-specific mechanics. The expected review loop *contract* (request review on every push, verify head-SHA coverage, triage findings, reply + resolve, escalate when stuck) is defined in [GOVERNANCE.md -> PR Review Etiquette](../GOVERNANCE.md#pr-review-etiquette). This section only describes how to make GitHub Copilot reliably execute it. - -### Triggering and Polling - -Auto-review on push is configured (via the branch ruleset's `copilot_code_review` rule with `review_on_push: true`) but fires inconsistently in practice, so treat it as best-effort, not guaranteed. After every push, **re-request a review programmatically** via the GraphQL `requestReviews` mutation, passing the Copilot reviewer's bot node id in `botIds`. This drives the loop end-to-end without a UI hand-off. - -**A review with no inline comments is still a completed review, not a failure, and not a reason to ask the maintainer to re-trigger.** Copilot very often posts a single formal review (GraphQL `state: COMMENTED`) whose body ends with "...reviewed N of N changed files ... and generated no comments" and adds **zero** inline threads. That review carries the head `commit.oid` and fully satisfies the loop, and it is the clean-pass success case. Never read "no inline comments" as "the review didn't run," and never re-request or escalate to the maintainer because comments are absent. - -**The one exception is a review that says it did not review, and it is delivered in exactly that shape.** Copilot answers a pull request it will not take on with a formal review, `state: COMMENTED`, carrying the correct `commit.oid` and **zero** inline threads, whose whole body is a refusal: "Copilot wasn't able to review this pull request because it exceeds the maximum number of files (300). Try reducing the number of changed files and requesting a review from Copilot again." Every coverage check passes, the rule above says an empty review is the clean pass, and the two together read a round that never happened as a round that found nothing. Observed on a pull request of 301 changed files, one over the limit, which was one command from merging on it. **The limit is 300 changed files and the remedy is to split the pull request**, since re-requesting the same head repeats the refusal: the file count is what it declined on and re-requesting does not change it. A repository committing binary or generated data alongside code crosses that line easily. Match the refusal on the body's **opening line** rather than anywhere in it, because a review discussing the wording is not one carrying it, and one line rather than two, because a review's first line is its heading and its second is the overview prose where such a description sits. Match an alternation for the same reason the suppressed heading takes one: - -```sh -# A review whose opening line declines the round. That line is the unit, since a refusal is -# the whole body and a match further down is a review quoting the wording rather than refusing. -# The dot spans both spellings of the apostrophe, the typographic one Copilot writes and the -# ASCII one, and it also keeps this filter usable inside single quotes, which neither survives. -gh api repos///pulls//reviews --jq \ - '.[] | select([(.body // "") | split("\n")[] | select(. != "")][0] // "" - | test("wasn.t able to review|was not able to review|unable to review")) | {commit_id, body}' -``` - -**Read the low-confidence findings, which are not inline threads.** A review body can carry a collapsed `
` block of findings Copilot withheld from the inline threads, and those findings appear nowhere in `reviewThreads`, so a loop that polls threads alone never sees them and reports a clean pass. **Match the block on more than one phrasing.** Its heading has appeared both as `Suppressed comments (N)` and as "Comments suppressed due to low confidence", so a filter keyed on either one alone silently reports zero suppressed findings on a review that has them, the same false clean this rule exists to prevent, one level up in the detection. **The section moves as well as it is worded, so match the heading wherever it sits.** It has appeared as its own `
` wrapper with a matching ``, as a bare heading in the body, and as a Markdown heading nested inside the `Review details` wrapper, whose `` names the wrapper and not the section. A filter reading a wrapper's `` reports zero on the nested shape, and the count it needs is the heading's own `(N)` rather than the wrapper's. They have been right repeatedly, including a rule stated more broadly than its check enforced and a check that skipped fenced blocks in every rule but one. Read the body of every review, investigate each suppressed finding on the same footing as an inline one, and answer it in the PR conversation, since a suppressed finding has no thread to reply on or resolve. - -```sh -# `test` with an alternation, not `contains` on one phrasing: the heading wording has changed. -gh api repos///pulls//reviews --jq \ - '.[] | select(.body | test("Suppressed comments|low confidence")) | .body' - -# Read every round, not only the head. A suppressed finding has no resolved state, so a push -# does not retire it: it simply stops appearing in a head-scoped query while still unanswered. -# Head-scoping this query is how four rounds went unanswered across three pull requests in a day. -gh api repos///pulls//reviews --jq \ - '[.[] | select(.body | test("Suppressed comments|low confidence"))] | length' - -# Mark which round each came from, since a finding on an older round may since be moot. -PR_HEAD=$(gh pr view --json headRefOid --jq '.headRefOid') -gh api repos///pulls//reviews --jq \ - "[.[] | select(.body | test(\"Suppressed comments|low confidence\")) - | {round: (if .commit_id == \"$PR_HEAD\" then \"head\" else \"earlier\" end), id}]" -``` - -**Round 1 is normally auto-seeded, so poll for it before trying to self-trigger.** Auto-review-on-open supplies the first review with no `botIds` call needed, but it can lag one to three minutes, and on some pull requests it never fires at all. After opening a PR (or the first push), **poll** for a Copilot review on the head SHA (see [Verify Review Covered Current Head](#verify-review-covered-current-head)) before concluding none ran. Where it never lands, drive round 1 with the same `requestReviews` mutation every later round uses, which needs nothing this PR has to produce first. A round 1 carrying no review therefore means "wait, then request it yourself," **not** "ask the maintainer to kick it off." - -> **The reviewer login differs by API, in three forms rather than two.** In **GraphQL** (`gh api graphql` and `gh pr view --json reviews`, which is GraphQL-backed) the `Bot.login` is `copilot-pull-request-reviewer`, with **no `[bot]` suffix**. In the **REST** API (`gh api repos/.../issues|pulls/...`) the same account's `user.login` is `copilot-pull-request-reviewer[bot]`, **with** the suffix. In a REST **timeline** `review_requested` event the `requested_reviewer` is a third spelling again, login `Copilot` with `type` `Bot`, so a filter written against either of the other two selects nothing there and reports a pull request with requests as having none. Match on the type plus a loose login test rather than on any one spelling, and each query below uses the correct form for its API. - -```sh -# 1. PR node id, plus the reviewer bot's node id read across the repo's recent PRs. -# The bot id is the reviewer account's own, so every PR in the repo carries the same one. -# The reviewer login is `copilot-pull-request-reviewer` in GraphQL. -PR_NODE=$(gh pr view --json id --jq '.id') -BOT_ID=$(gh api graphql -f query=' -{ - repository(owner: "", name: "") { - pullRequests(first: 20, orderBy: { field: CREATED_AT, direction: DESC }) { - nodes { reviews(first: 20) { nodes { author { __typename login ... on Bot { id } } } } } - } - } -}' --jq '[.data.repository.pullRequests.nodes[].reviews.nodes[] - | select(.author.login == "copilot-pull-request-reviewer") - | .author.id] | first // empty') -if [ -z "$BOT_ID" ]; then - echo "no Copilot review in the 20 most recent PRs, so widen the window" >&2 - return 1 2>/dev/null || exit 1 # Stop. Do NOT call requestReviews with an empty id. -fi - -# 2. Re-request a Copilot review on the current head. -gh api graphql -f query=' -mutation($pr: ID!, $bot: ID!) { - requestReviews(input: { pullRequestId: $pr, botIds: [$bot], union: true }) { - pullRequest { id } - } -}' -F pr="$PR_NODE" -F bot="$BOT_ID" -``` - -**The bot node id belongs to the reviewer account, not to a pull request**, and it is the same id on **every PR in the repo**, so nothing has to land on this PR before step 1 can read it. A PR opened a minute ago, with no review and no comment of its own, needs no UI seeding to bootstrap the id and no prior review to source it from: any Copilot review anywhere in the repo carries it. Query the **most recent** PRs, since a plain `last: 20` returns the *oldest* ones, which may predate Copilot on the repo. **Guard for an empty result**, because an empty `$BOT_ID` says only that none of the PRs sampled carry a Copilot review, so widen the window (raise the count or paginate) before concluding the repo has never had one. Never pass an empty id to the mutation. - -A read scoped to this PR (`pullRequest(number: ) { reviews }`) returns the same id once a review has landed here, and it buys nothing over the repo-wide read while failing on exactly the round the repo-wide read handles. Where the repo's only Copilot artifact is an issue comment rather than a formal review, read the id from that comment's author instead (`pullRequest.comments` -> author `... on Bot { id }`). Manual UI seeding is the last resort, needed only for a repo that has **never** had a Copilot review, so no prior id exists anywhere to read. - -**Do NOT post `@Copilot review` as a PR comment.** That comment triggers the Copilot *coding agent* (`copilot-swe-agent[bot]`), which makes code changes rather than posting a review. - -Known non-working request paths (don't rely on them, and use the `requestReviews` mutation above instead): - -- `POST /requested_reviewers` with `reviewers=[Copilot]` can return 200 but no-op. -- `copilot-pull-request-reviewer` as a requested reviewer slug returns 422. -- `requestReviews` with the reviewer's bot node id in **`userIds`** fails with `Could not resolve to User node`, because the Copilot reviewer is a **Bot**, so its node id goes in **`botIds`** (as in the mutation above), never `userIds`. -- `suggestedActors(capabilities: [CAN_BE_ASSIGNED])` lists `copilot-swe-agent` (the coding agent), not `copilot-pull-request-reviewer`, so do not source the reviewer's bot node id there. Read it from an existing review per step 1 above. -- There is no `removePullRequestFromReviewRequest` mutation, but removal is not therefore impossible: `requestReviews` **replaces** the reviewer set when `union` is false (the schema describes `union` as "add users to the set rather than replace"), so an empty `botIds` with `union: false` removes the pending request. Reach for it only in the stuck case below, since `union: true` re-fires a review on the current head without it. -- `gh pr view --json reviewRequests` **omits a Bot reviewer entirely**, reporting an empty set while Copilot sits in it. Read the pending set through GraphQL `reviewRequests`, which returns the `Bot` node, because the REST-backed projection makes a pending request read as no request at all. - -### Verify Review Covered Current Head - -Before merging, confirm Copilot reviewed the current PR head SHA. Copilot may respond as either a formal review (carries an exact commit SHA) or an issue comment (no SHA, so use the most recent Copilot comment for manual confirmation). Check both. - -**Count matches and compare numerically, so an empty result cannot read as success.** A poll that captures a `gh api --jq` result and exits on `[ "$found" != "0" ]` treats an **empty** string as a landed review, and an empty string is exactly what a mis-written filter returns. Pipe the matches through `wc -l` and test `-gt 0`, so a query that finds nothing and a query that ran wrong both read as "not yet". A `gh` call that fails to run reaches the test the same way, because it writes its message to stderr and prints nothing to stdout, so the `$(...)` around it still yields the empty string. A mistyped or unsupported flag is the usual cause, and `gh` reports one as `accepts 1 arg(s), received 4` rather than as anything resembling a review verdict. - -**Check head coverage before reading merge-state, never the reverse.** A push makes the required checks go green before Copilot re-reviews the new head, so `mergeStateStatus` can read `CLEAN` in the window before any formal review covers the head. A poll that exits on `CLEAN` merges into that gap. Gate on a formal review whose `commit.oid` equals the current head SHA first, then on zero unresolved threads, and only then read merge-state. - -```sh -PR_HEAD=$(gh pr view --json headRefOid --jq '.headRefOid') - -# 1. Formal review - exact SHA match. -gh pr view --json reviews --jq \ - '.reviews[] | select(.author.login=="copilot-pull-request-reviewer") | .commit.oid' \ - | grep -q "$PR_HEAD" && echo "covered via formal review" - -# 2. Issue comment - show the most recent Copilot comment for manual -# confirmation. This is the REST API, so the login carries the `[bot]` suffix. -gh api repos///issues//comments --jq \ - '[.[] | select(.user.login=="copilot-pull-request-reviewer[bot]")] | last | {created_at, body: .body[:200]}' -``` - -Coverage is confirmed when (1) exits 0, and **a formal review with no inline comments still satisfies path (1)**, because coverage is about the head SHA, not the comment count. The exception is the refusal above, which is a formal review on the head with no inline comments and covers nothing, so path (1) exits 0 over a round that never ran. Read the body of the review the SHA matched, not only the SHA. For issue comments (path 2), body content is the only reliable signal, and `created_at` is not: `git log -1 --format=%cI` is the **commit** timestamp, not the push timestamp, so amended or rebased commits can have an earlier timestamp and an older Copilot comment could satisfy a time check even though Copilot never saw the current head. Treat path (2) as confirmed only when the comment body explicitly refers to the current changes. - -**Coverage of the head is not coverage of the diff, and the second one is stated in a line nothing above reads.** A review body says how many of the pull request's changed files it read, and a round that read fewer than the pull request changed is byte for byte the clean pass in everything else: the same `commit.oid`, the same absent threads, the same "generated no comments". Measured over 332 Copilot review bodies on this repository, five rounds across three pull requests reported reading fewer files than were changed, and all three merged. One of them changed three files, left one unread across **both** its rounds, and reported no comments each time. This is the third instance of the shape the refusal above and the suppressed block below are the first two, so read it the same way: **fail closed on a wording you do not recognize**, since a gate that allows whatever it does not recognize stops gating as the wording drifts, and both of those wordings have drifted once already. - -Two spellings carry the count, and both are current rather than one superseding the other. Each opens its own line, which is what separates the round stating its coverage from prose mentioning changed files, that prose being what a review of a change to this rule looks like: - -```text -Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments. -- **Files reviewed:** 2/3 changed files -``` +For every review: -The sentence tail after the first spelling reports how many comments the round raised and appears in four wordings. It is not coverage, so it is not part of what has to be recognized, and the counts are. Read them into three verdicts and two exemptions: +1. Read the full pull request diff and count its changed files. +2. Follow `.github/skills/code-review/SKILL.md` and every skill it selects. +3. Publish every supported finding. Never suppress a finding or place it in a low-confidence or + hidden findings block. +4. Use an inline comment when a changed line can anchor the finding. Use the review body only when + no valid inline anchor exists. +5. End the review body with the exact machine-readable marker required by the `code-review` skill. -- **Counts equal** - the round read the whole diff. This is the clean pass. -- **Counts unequal** - files in the diff have no review at all. Do **not** treat a re-request as the remedy: measured over four pull requests and seven rounds on this repository, every partial round stayed partial at the identical ratio and no round ever recovered, so re-requesting spends a round and changes nothing. Splitting works where it applies and does not apply to a promotion, whose head is `develop`. **The file table in the body does not tell you which file went unread**, and it looks as though it should, which is why it is written down here: measured over 348 review bodies on this repository and 121 on another in this fleet, that table names the whole changed set on partial and fully covered rounds alike, so a table naming every changed file is what a full round carries too and contradicts nothing. One round of the seven is the exception, stating 16 of 17 and naming 16, omitting `GOVERNANCE.md`, and `status` names an omitted file only in that shape, where the table is short by exactly what the counts leave unread and names nothing outside the diff. Treat that as a lead to check rather than a verdict, one round here naming `GOVENANCE.md`, a path no diff carries. Report the state and hand the merge decision to the maintainer. -- **Coverage-shaped and unreadable** - the remedy is to fix the reader, not to read past it. The vetted spellings live in `scripts/pr_review.py` and here, and they stay in step because a case reads them out of this file. -- **Exempt: a body stating no coverage at all.** 28 of those 332 bodies are an overview and a change list and nothing more. That shape is current, interleaves with the counted one throughout, and one pull request carries both across its two rounds, so treating it as a failure cries wolf on about one review in twelve and teaches an agent to work around the gate. It reads as `coverage=unstated`, never as a pass and never as a failure. -- **Exempt: a refusal.** It carries no coverage line by design, and the refusal rule above has already classified it. Read it here as well and every refusal grows a spurious second failure on top of the one that names its remedy. +The review automation is `scripts/pr_review.py`, run from a hub checkout. Use its `status`, `wait`, +`comment`, and `reply --resolve` commands instead of reconstructing GraphQL queries or copying +review identifiers by hand. Use `comment` for a suppressed-finding answer in the pull request +conversation. Its status gate verifies the current head, diff coverage, output shape, inline +threads, body-only findings, and required checks. -`scripts/pr_review.py status --repo /` reports this as `coverage=full`, `coverage=PARTIAL`, `coverage=UNVETTED` or `coverage=unstated`, and exits `42` on a partial round. An unreadable wording exits `43` instead, as one of the unrecognized shapes below rather than as a case of its own, since both say the reader is what needs fixing. Read it by hand as: +A formal review with no findings is complete only when it covers the current head and states full +diff coverage. A refusal, partial or absent coverage statement, unrecognized output shape, +unresolved thread, or body-only finding blocks the review loop. Re-run the loop after every fix +push. Never infer review completion from `mergeStateStatus: CLEAN`. -```sh -gh pr view --json reviews --jq \ - '.reviews[] | select(.author.login=="copilot-pull-request-reviewer") | .body - | split("\n")[] | select(test("^(Copilot|[-*] \\*\\*Files reviewed:).*changed files?"))' -``` - -### A Shape Nothing Recognizes Blocks the Loop and Earns an Issue - -**Every rule above keys on a marker in what Copilot sent, so a marker that changes spelling is a section the reader stops finding and reports as absent.** That is not a hypothetical: all three failures on record here have exactly that shape. The suppressed heading was reworded and the count went to zero. The suppressed section moved inside another wrapper and the count went to zero again. The coverage line was never read at all. Each one reported a clean pass over a review it had misread, and each was caught by the maintainer after it had already landed, rather than by the gate. - -**So an unrecognized shape is a blocking outcome, and its remedy is an issue rather than a judgment call.** When any reader here meets a heading, a collapsed section, a metadata line, a coverage wording or a reviewer login it has no vetted spelling for, the review loop **does not close**, whatever else the digest says. Do not read past it, do not infer what the new wording probably means, and do not treat a body that looks clean as a clean review, because "looks clean" is precisely what a misread review looks like. Two things follow, in this order: - -1. **File an issue on the hub**, the repository `AGENTS.md` "Fleet Bootstrap" names, which hosts `scripts/pr_review.py` and holds the vetted inventory. Name each unrecognized shape and quote the review body it came from, so the fix is made against the real wording rather than a paraphrase. The issue is filed even when the shape turns out to be cosmetic, since "cosmetic" is a conclusion drawn after reading the body and not before. -2. **The merge decision is the maintainer's**, not the agent's and not the script's. An unrecognized shape does not mean the pull request is bad, it means nothing here can vouch for the review of it. Report the state, hand it over, and stop. - -`scripts/pr_review.py status --repo /` reports this as `shapes=UNRECOGNIZED`, lists each shape under a marker naming the remedy, and exits `43`. `wait` carries the same code, so a wait cannot end on a clean zero over output nothing read. The vetted inventory lives in that script and is small on purpose: measured over 332 Copilot review bodies on this repository, with fenced blocks dropped and text reduced to ASCII, the whole corpus is seven headings, six `` texts and three metadata labels, and every body carries at least one of them. A body carrying none is itself the unrecognized shape, which is what catches a rewrite that changes everything at once, the refusal wording drifting among it. - -### Bounded Retry Workflow - -This path is only for a **genuinely missing** review, meaning no Copilot review (formal *or* issue comment) covers the current head SHA after polling. A review that covered the head but produced no comments is a clean pass, not a missing review, so do not enter this retry path for it. - -**A slow review is pending, not missing, so poll with backoff and never escalate on a timeout alone.** Copilot can lag far beyond the usual one-to-three minutes when it has been re-requested many times in quick succession, because it throttles under load, and a re-review landing tens of minutes after the request is normal. A poll that times out is therefore evidence only that the review has not landed *yet*, not that Copilot is done or unresponsive. Report the status as "review still pending" and keep polling on a widening interval (for example 20s steps, then a few minutes) rather than stopping. Enter the escalation step below only when the `requestReviews` mutation itself no-ops or errors, or after a genuinely long wait with the request confirmed accepted, never merely because one fixed poll window elapsed. - -**Bound each wait, and read what Copilot actually posted before opening another one.** A poll that widens forever is indistinguishable from a poll that has stopped, and "still pending" is the honest report for exactly as long as evidence supports it. Two readings decide whether waiting again is warranted. Compare the request's timestamp against the newest Copilot activity of **any** kind on the pull request, since a reviewer that has already answered on a later head, or that posted an issue comment instead of a formal review, is not a reviewer running late, and a wait that keeps reporting "pending" against a landed review is a broken wait rather than a slow reviewer. Then read that newest response, because a Copilot answer naming a quota or a rate limit is a **terminal** outcome rather than a pending one: no formal review will land, so path (1) never matches the head and path (2) is correctly never confirmed, both paths behave exactly as specified, and the agent waits for something that is not coming. The fix is account-side and re-requesting does not change it, so report it to the maintainer and stop waiting. Where the newest response is neither a review nor a refusal you recognize, that too goes to the maintainer with its text, rather than being waited through. - -**A pending request nothing picked up is a third state, and it is the one that looks most like patience.** Copilot raises a `copilot_work_started` timeline event within about half a minute of accepting a request, and submits its review a few minutes later. A request that never draws one is not a slow review, it is a request nothing is acting on, and it stays that way indefinitely: one sat for thirteen and a half hours while the pull request read as waiting on the reviewer. Elapsed time cannot tell the two apart, since a genuinely slow round also shows no review, so read the event rather than the clock. `copilot_work_started` appears in the REST timeline only, and no GraphQL timeline item carries it: - -```sh -# The pending set (GraphQL, since the `gh pr view` projection cannot see a Bot reviewer). -gh api graphql -f query=' -{ repository(owner:"",name:""){ pullRequest(number:){ - reviewRequests(first:10){ totalCount - nodes{ requestedReviewer{ __typename ... on Bot{login} ... on User{login} } } } } } }' - -# The request and pickup events, newest last. A `review_requested` with no later -# `copilot_work_started` is the stuck state. Requests are filtered to the reviewer's own, -# since a human requested afterwards is a different request and reading it as this one -# reports a picked-up review as never picked up. `per_page` is the pagination cost. -gh api --paginate 'repos///issues//timeline?per_page=100' \ - --jq '.[] | select(.event == "copilot_work_started" or (.event == "review_requested" - and .requested_reviewer.type == "Bot" - and ((.requested_reviewer.login // "") | ascii_downcase | test("copilot")))) - | "\(.event) \(.created_at)"' -``` - -**Recover it by clearing the request and requesting again**, because the pull request UI offers no re-request control while a request is pending, and `requestReviews` with `union: true` adds a reviewer already in the set, which changes nothing. Read the pending set first, since `union: false` replaces the whole set and would drop a human reviewer requested alongside the bot. Where the clear-and-request does not draw a `copilot_work_started` within a minute or so, push a commit instead, since a new head raises a fresh request rather than poking a stale one. - -```sh -PR_NODE=$(gh pr view --json id --jq '.id') -# 1. Clear. `union: false` replaces the set, so an empty botIds removes the pending request. -gh api graphql -f query=' -mutation($pr: ID!) { - requestReviews(input: { pullRequestId: $pr, botIds: [], union: false }) { - pullRequest { reviewRequests(first: 10) { totalCount } } } -}' -F pr="$PR_NODE" -# 2. Request again, against a now-empty set, with $BOT_ID read as in "Triggering and Polling". -gh api graphql -f query=' -mutation($pr: ID!, $bot: ID!) { - requestReviews(input: { pullRequestId: $pr, botIds: [$bot], union: true }) { - pullRequest { reviewRequests(first: 10) { totalCount } } } -}' -F pr="$PR_NODE" -F bot="$BOT_ID" -``` - -If a review did not run on the current head, retry: - -1. Wait briefly and check head-SHA coverage (see above). -1. Re-request the review via the `requestReviews` mutation (see "Triggering and Polling"), falling back to the GitHub PR UI only if the mutation no-ops. -1. Retry up to two more times (three total). -1. If still missing, mark review as blocked and escalate to the user/maintainer with what was attempted. - -### Reply and Thread Resolution Workflow - -Every id below is captured from a live query into a variable and passed from there, never hand-typed, guessed, or pasted as a `PRRT_...` literal. A node id resolves globally, so a fabricated or stale id does not fail, it writes to a real thread on an unrelated repository. This runbook implements [GOVERNANCE.md "Repository Boundaries and Write Safety"](../GOVERNANCE.md#repository-boundaries-and-write-safety): write only to this repo, capture every id from a live query, and never suppress a mutation's output. - -**Use the hub's helper, which has nowhere to type an id.** `scripts/pr_review.py reply --repo / --match "" --body "" --resolve` queries the thread id itself and passes it straight to the mutation. That rule is known and read by the agents that break it anyway, three times so far, so the shape is what changes rather than the wording. It selects on the finding's own words rather than a line number, since a fix push moves the line; it refuses on no match and on more than one rather than picking; and it does not resolve a thread whose reply came back without a `url`. Cross-owner targets it refuses outright, which is where the hand-run form below applies, and there the `gh-write-guard` hook is what reads the maintainer's grant. It is hub-hosted per [GOVERNANCE.md "Hub-Hosted Tooling"](../GOVERNANCE.md#hub-hosted-tooling), so it is invoked from a hub checkout and never rebuilt locally. - -The hand-run form is below, for a cross-owner target and for the case where the hub cannot be reached and the work cannot wait. - -List unresolved threads. Use `first: 100` with cursor-based pagination, and where `hasNextPage` is true, re-run with `after: ""` to retrieve the next page: - -```sh -gh api graphql -f query=' -{ - repository(owner: "", name: "") { - pullRequest(number: ) { - reviewThreads(first: 100) { - nodes { - id isResolved path - comments(first: 1) { nodes { author { login } body } } - } - pageInfo { hasNextPage endCursor } - } - } - } -}' | jq ' - .data.repository.pullRequest.reviewThreads | - (.pageInfo | "hasNextPage=\(.hasNextPage) endCursor=\(.endCursor)"), - (.nodes[] | select(.isResolved == false)) -' -``` - -Reply on a thread, then resolve it. Capture the target thread's id into `$TID` from the listing query above, filtering to the thread being answered by its `path`, and guard for an empty result so a mutation never runs on a guessed id. When a file carries more than one unresolved thread, `path` alone is ambiguous and `head -n 1` would pick the wrong one, so narrow by first-comment body (the query already fetches `comments(first: 1)` for this) by adding `and (.comments.nodes[0].body | contains(""))` to the `select`: - -```sh -TID=$(gh api graphql -f query=' -{ - repository(owner: "", name: "") { - pullRequest(number: ) { - reviewThreads(first: 100) { - nodes { id isResolved path comments(first: 1) { nodes { body } } } - } - } - } -}' --jq '.data.repository.pullRequest.reviewThreads.nodes[] - | select(.isResolved == false and .path == "") - | .id' | head -n 1) -[ -n "$TID" ] || { echo "no matching unresolved thread on - do not guess an id" >&2; return 1 2>/dev/null || exit 1; } - -# Show the mutation's output. Never append an output-discard or force-success tail -# (>/dev/null, 2>/dev/null, &>/dev/null, || true, || :, || echo) to a write. -gh api graphql -f query=' -mutation($threadId: ID!, $body: String!) { - addPullRequestReviewThreadReply(input: { pullRequestReviewThreadId: $threadId, body: $body }) { - comment { id url } - } -}' -F threadId="$TID" -F body="Fixed in : ." - -# Confirm isResolved: true in this response before treating the thread as closed - a write that -# appears to fail may have taken on the server. -gh api graphql -f query=' -mutation($threadId: ID!) { - resolveReviewThread(input: { threadId: $threadId }) { thread { id isResolved } } -}' -F threadId="$TID" -``` - -Issue-level Copilot comments (those in `issues//comments`) have no resolution action, since GitHub provides no API or UI to resolve them. Reply if the finding warrants it, but no resolution step is needed or possible. - -### PR Edits and Merge-State Gotchas - -- **`gh pr edit --title/--body` is broken on `gh` 2.45.x and 2.46.x, and works from 2.47 up.** Those releases touch the deprecated Projects-classic `projectCards` GraphQL field and **exit non-zero without applying the change** (a stale PR description then survives review rounds), which the GitHub CLI maintainers name as broken by deprecated APIs. A distribution package is where that version comes from, so check `gh --version` before concluding the command is unusable, and install from the official repository rather than working around it. Where a host is genuinely stuck on one, edit via the API and verify it took: GraphQL `updatePullRequest(input: { pullRequestId, title, body })`, or REST `gh api -X PATCH repos///pulls/ -F body=@body.md` (the `@` reads the body from a file, so name it explicitly, not the literal `file`). The same version range carries no `--json` flag on `gh pr checks`, so a watcher built on it prints nothing and a quiet result reads as a passing one. -- **`main`/`develop` use rulesets, not classic branch protection.** The classic protection REST endpoint (`repos/.../branches//protection`) 404s, so read the ruleset instead. A `mergeStateStatus` of `BLOCKED` on a green PR is most often just **unresolved review threads** (the ruleset requires thread resolution), and resolving them moves it to `CLEAN`. (`BLOCKED` is a `mergeStateStatus` value, so don't confuse it with the separate `mergeable` field's `MERGEABLE`/`CONFLICTING`, which reports merge conflicts, not review gates.) -- **`BLOCKED` never says which gate, so never infer one.** The same word covers a red check, a required check nothing is running, an unresolved thread, and a missing approval, and the bullet above says "most often" rather than "always" for that reason. Read the checks instead of guessing: `pr_review.py status` prints `checks=N/M` beside the merge word and names a stuck one, and it exits `44` from `wait` where the merge reads `BLOCKED`, the review loop closed, and a check is starved, expected and never posted, running far past what the job costs, or failed. A **queued check with no runner** is the case that reads exactly like patience: a run here polled `BLOCKED` for twenty-five minutes on a pull request whose only unfinished check was an aggregator job GitHub dispatched and never assigned a runner, and the cause came from the maintainer rather than from any field. Nothing agent-side starts that job, because the pool is GitHub-hosted, so the remedy is a re-run of the workflow or waiting on that capacity, and it is **not** a re-request, a rebase, or an empty commit. A job held behind a `needs:` dependency does not enter the rollup until that dependency finishes, so a queued check is never a dependency waiting its turn. -- **Push -> head-SHA read race.** A `headRefOid` read taken immediately after a push can return the **old** head, so re-read after the push registers, or a coverage poll evaluates the stale SHA. -- **Copilot is sometimes factually wrong** (e.g. it claimed `actionlint -color` "requires a value" when it is a boolean flag). Verify a finding before fixing, and decline with evidence when it is wrong, which is distinct from dismissing a still-present finding as stale. The evidence goes under [Disproved Claims](#disproved-claims) as well as in the thread, because the thread closes with the pull request and the next round starts without it. - -Reply-body conventions: - -- Accepted bug/style fix: include fixing commit SHA and a one-line summary. -- Declined style comment: cite the rule (GOVERNANCE.md or the CODESTYLE.md language section) and the existing-tree precedent. -- Declined architecture proposal: one-sentence rationale. -- Declined false positive on carried fleet content (a broken-link or dead-cross-reference flag inside byte-locked rule text): cite the "Reviewing Carried Fleet Content" section, since the reference is intentional and the text cannot be edited locally. - -After the final push, sweep-resolve stale older threads for removed code paths. +Review effort is user-controlled. The automation observes `Lite`, `Balanced`, or `Max`, including an inherited `Default ()`, and never selects or changes the setting. Effort does not determine coverage or completion. A request can complete without a `copilot_work_started` event, so absence of that event is not a stalled-review verdict. When `wait` returns `PENDING` with `requested=yes`, report the state and rerun `wait` for another bounded interval by default. Do not clear the request automatically because it may be active. If the maintainer directs a retry, remove Copilot in the pull request UI, add it again, and rerun `wait`. This recovery replaces only the review request and never changes the effort setting. ### Disproved Claims @@ -332,329 +64,5 @@ After the final push, sweep-resolve stale older threads for removed code paths. ## When in Doubt -Read [AGENTS.md](../AGENTS.md) to find the section that governs your change, and [GOVERNANCE.md](../GOVERNANCE.md) for the rule text itself. For code-style rules, [`CODESTYLE.md`](../CODESTYLE.md) (its General section plus the relevant language section) is authoritative. Don't restate any of these files' rules in commit bodies or PR descriptions, and keep those focused on the change itself. - -If you find a gap in the governance itself (this file, AGENTS.md, or GOVERNANCE.md is out of date, a rule is missing, something bit this repo and would bite the next), fix it in the governance docs as part of your change rather than only working around it locally. - -## Project Overview - -PhotoCleaner is a .NET 10 console application that processes media files in preparation for import into photo management systems (Lightroom, Immich, PhotoPrims). It analyzes and transforms media files through validation, modification, and verification phases. - -## Architecture & Data Flow - -### Project Structure - -- **Docker/**: Docker configuration - - `Dockerfile`: Two-stage build (SDK Alpine build -> runtime Alpine final) that installs `exiftool` and `ffmpeg` in the final stage -- **PhotoCleaner/**: Main console application - - `Program.cs`: Entry point with logger setup (Main only) - - `CommandLine.cs`: System.CommandLine implementation for CLI parsing (`process`, `undo`, `import`, `index`, `trash`, and `verify` subcommands) - - `MediaUtilities.cs`: Shared static utilities, `SupportedExtensions` (FrozenSet), `GetUniqueFileName`, `GetExifToolJsonAsync`, `SetCreateDateAsync`, video/duration constants - - `CommandRunner.cs`: Thin wrapper for command start/complete/error logging, taking `Func>` and returning the command's exit code - - `ExitCode.cs`: The shared exit-code contract, being `Success` (0), `Error` (1, command could not run), and `Failed` (2, ran to completion with per-file failures) - - `DatabaseScope.cs`: Generic async DB lifecycle helper (create, init, dispose) - - `TrashDatabaseScope.cs`: Same lifecycle helper pattern for `TrashDatabase` - - `FileEnumerator.cs`: Parallel file enumeration returning `(IReadOnlyList, int)` - - `DirectoryCleaner.cs`: Static helper that deletes empty subdirectories under a root (deepest-first, and the root itself is never deleted), used by `import` and `process` when `--deleteempty` is set - - `ProcessCommand.cs`: Process command orchestration, case conflict resolution, reprocessing loop, result reporting - - `ImportCommand.cs`: Import command orchestration (formerly `OrganizeCommand`) - - `IndexCommand.cs`: Index command orchestration - - `TrashCommand.cs`: Trash command orchestration, fetches trashed asset checksums from Immich API, stores SHA-1 hashes in a `TrashDatabase` - - `UndoCommand.cs`: Undo command orchestration - - `VerifyCommand.cs`: Verify command orchestration, which enumerates, runs `VerifyTask`, and reports counts - - `ProcessTask.cs`: Core file processing pipeline (validation, conversion, metadata) - - `UndoTask.cs`: Undo logic, two-pass algorithm that restores `.bak` files - - `ImportTask.cs`: Import logic, copies (default) or moves supported media files from source into date-based subdirectories under `--outpath`. Inserts a row keyed by SOURCE path into Import.db. Optional SQLite deduplication via `Database`. (Formerly `OrganizeTask`.) - - `VerifyTask.cs`: Verification logic, a decode pass that runs Immich's own `MediaRepository` inside `ghcr.io/immich-app/immich-server:release` via `docker run`, batching paths over stdin. Preflights the image before judging any file, so an infrastructure failure exits `Error` rather than marking files invalid - - `ImmichVerifyScript.cs`: The Node script run inside the Immich image, as const strings. Calls Immich's own compiled `MediaRepository`, `defaults`, and `ThumbnailConfig` rather than reimplementing the preview pipeline, so behavior tracks Immich across releases - - `VerifyResult.cs`: The AOT-compatible `ImmichVerifyLine` JSON model and its `ImmichVerifyJsonContext` source-generated context, together forming the container's output protocol - - `IndexTask.cs`: Common DB upsert logic used by `process` and `index` commands. `IndexFileAsync` (single-file) returns `(IndexStatus, sha256, sha1, wasProcessed)`. `ExecuteAsync` (batch parallel) returns `(inserted, updated, unchanged, ignored, failed)`. When `options.MarkProcessed` is true, newly inserted rows are marked `is_processed=1` (used by `index --processed` to seed Process.db). - - `Database.cs`: SQLite wrapper with a single `files` table (`path` PRIMARY KEY, `sha256`, `sha1`, `file_size`, `mtime_ticks`, `is_processed`), indexes on both hash columns, and size/mtime caching via `ResolveHashesAsync` to skip rehashing unchanged files. Every write computes both sha256 and sha1 in a single read pass. Both columns are non-null. - - `TrashDatabase.cs`: Simple SQLite wrapper for Immich trash hashes with a single `trash_hashes` table (`sha1` PRIMARY KEY), used by `trash`, `import`, and `process` commands - - `ImmichApiModels.cs`: AOT-compatible JSON models for Immich API (`ImmichSearchRequest`, `ImmichSearchResponse`, `ImmichAssetDto`) with `ImmichJsonContext` source generation - - `DateFromPath.cs`: Static utility class for date inference from filenames/paths - - `ExifToolJson.cs`: JSON model for ExifTool metadata, including the `ExifTool:Validate` verdict and `ParseValidate` which splits it into error and warning counts - - `SkippedExtensionTracker.cs`: Thread-safe tracker for unknown file extensions skipped during processing; used by all commands that filter by `MediaUtilities.SupportedExtensions` (`process`, `import`, `index`) - - `HttpClientFactory.cs`: Polly resilience pipeline (retry, circuit breaker) and `SocketsHttpHandler` connection pooling - - `AssemblyInfo.cs`: Assembly metadata (app name, version) used by `HttpClientFactory` for User-Agent header - - `Extensions.cs`: Extension methods for logging and error handling -- **PhotoCleanerTests/**: Comprehensive test project - - `DateInferenceTests.cs`: Core date inference functionality tests (33 tests) - - `DateInferenceEdgeCasesTests.cs`: Edge cases and comprehensive scenarios (19 tests) - - `CommandLineTests.cs`: Command line parsing and validation tests (15 tests) - - `ProcessTaskTests.cs`: Process task tests (61 tests) - - `UndoTaskTests.cs`: Undo task tests (13 tests) - - `ExifToolJsonTests.cs`: ExifToolJson unit tests (includes GetDate, IsDngVersionNewer) (33 tests) - - `ImportTaskTests.cs`: Import task tests (24 tests) - - `DatabaseTests.cs`: Database tests (15 tests) - - `IndexTaskTests.cs`: IndexTask tests (7 tests) - - `TrashDatabaseTests.cs`: TrashDatabase tests (8 tests) - - `TrashCommandTests.cs`: TrashCommand tests with mock HTTP handler (6 tests) - - `DirectoryCleanerTests.cs`: DirectoryCleaner static helper tests (6 tests) - - `VerifyTaskTests.cs`: Verify protocol parsing and script-contract tests (10 tests) - -### Core Processing Pipeline - -The application uses a sequential validation pipeline where each method returns `bool` - -`false` stops processing the current file: - -```csharp -if (!RenameMismatchedMimeExtensions() - || !RenameMixedCaseExtensions() - || !await DeleteLivePhotosAsync() - || !await ConvertVideoAsync() - || !WarnDngVersion()) -``` - -Before that chain runs, `CheckExifToolValidation` acts on the `ExifTool:Validate` verdict that -rides along with the metadata read. Only an error count fails the file (`ProcessResult.Invalid`). -Warnings are logged at debug level, because roughly three quarters of healthy files in a real -collection carry at least one. - -### State Management Pattern - -- **Primary Constructor Parameters**: Command and task classes use C# 12 primary constructors. All task classes take `CommandLine.Options options` as their first parameter, plus any non-option runtime params (e.g., `Database`, shared collections). Command classes take `(CommandLine.Options options, CancellationToken cancellationToken)` and pass `options` directly to task constructors. -- **Command/Task Separation**: Command classes (e.g., `ProcessCommand`) handle orchestration (file enumeration, DB lifecycle, result logging), while task classes (e.g., `ProcessTask`) handle per-file business logic -- **Composable Infrastructure**: `CommandRunner`, `DatabaseScope`, and `FileEnumerator` are static helpers freely composed by command classes, no inheritance hierarchy -- **Shared Collections**: `ConcurrentBag` for file names, `ConcurrentDictionary` for unknown extensions with case-insensitive comparison -- **Parallel Processing**: Files processed using `Parallel.ForEachAsync` with `MaxDegreeOfParallelism` -- **External Tool Integration**: Uses `CliWrap` for all external command execution (exiftool, ffmpeg, ffprobe) -- **FrozenSet Collections**: All static readonly extension collections use `FrozenSet` with `StringComparer.OrdinalIgnoreCase` for O(1) lookups - -## Key Patterns & Conventions - -### External Tool Execution Pattern - -```csharp -BufferedCommandResult result = await Cli.Wrap("exiftool") - .WithArguments(["-groupNames", "-json", "-validate", "-all", _fileInfo.FullName]) - .ExecuteBufferedAsync(); -``` - -- Always use array syntax for arguments: `["-arg1", "value"]` -- Use `BufferedCommandResult` for output capture, `CommandResult` for fire-and-forget -- JSON trimming pattern: `result.StandardOutput.Trim(' ', '\n', '\r', ' ', '[', ']')` - -### Media File Processing Conventions - -- **FrozenSet Extensions**: Define supported extensions as `FrozenSet` with `StringComparer.OrdinalIgnoreCase` (e.g., `s_remuxExtensions`, `s_jpegExtensions`) -- **Case-Insensitive Matching**: Use FrozenSet `.Contains()` directly without `.ToLower()`, comparer handles case-insensitivity -- **File Type Categorization**: Group operations by file type requirements (remux vs re-encode vs audio-only) -- **Single-Pass Optimizations**: Prefer single-loop iterations with early exit over multiple LINQ passes -- **Skipped Extension Tracking**: Commands that filter files by `MediaUtilities.SupportedExtensions` pass a shared `SkippedExtensionTracker` instance to their task classes. The tracker collects unknown extensions (thread-safe via `Track()`), and the command calls `LogWarnings()` after processing to log them sorted. Used by `process`, `import`, and `index` commands. - -### EXIF/Metadata Handling - -- Uses `ExifToolJson` class with `JsonPropertyName` attributes for precise metadata field mapping -- Date validation prioritizes `EXIF:DateTimeOriginal` over `QuickTime:CreateDate` -- Custom `IsDateSet()` and `GetDateString()` methods handle metadata extraction logic -- `ContentIdentifier` property maps both `QuickTime:ContentIdentifier` and `Keys:ContentIdentifier` - group names (both occur in the wild for ISOBMFF files) returning whichever is set - -### Date Inference System (DateFromPath.cs) - -- **Static Internal Methods**: All methods are `internal static` for testability with `InternalsVisibleTo` -- **DateFromPath.InferCreatedDate()**: Main entry point, tries filename first, then path fallback -- **DateFromPath.ExtractDateFromFilename()**: Supports multiple filename patterns: - - `YYYYMMDD_HHMMSS` format (e.g., `20210502_200152957_iOS-1747.jpg`) - - `YYYYMMDD` format (e.g., `EX_20030219_3378.jpg`) - - `YYYY-MM-DD-HH-MM-SS` format (e.g., `PHOTO-2024-06-22-07-56-41.jpg`) - - `YYYY MM DD` format with spaces (e.g., `EV 2014 07 03_0003.tif`) -- **DateFromPath.ExtractDateFromPath()**: Extracts from directory structures and year-only fallback -- **DateFromPath.IsDateValid()**: Validates dates within 1900-current year range - -### Command Line Interface (CommandLine.cs) - -- **System.CommandLine Integration**: Uses modern .NET command line parsing -- **Six subcommands**: `process`, `undo`, `import`, `index`, `trash`, `verify`, each with their own option set -- **Required `--path` Parameter**: Single directory path using `Option`. Validated with `AcceptExistingOnly()` -- **Optional `--dryrun` Flag**: Non-destructive preview mode (process, undo, import, not index) -- **Optional `--threads` Parameter**: Controls parallel processing degree with `DefaultValueFactory = _ => Math.Min(Environment.ProcessorCount, 4)`. Validated to be > 0 and <= Environment.ProcessorCount using `Validators.Add()` (process, import, index) -- **Optional `--skipbackup` Flag** (process only): Skips all `.bak` file creation, originals are deleted/overwritten in-place. Logs a warning at startup. Disables undo. -- **Optional `--deleteempty` Flag** (process, import): After the command completes, deletes empty child subdirectories from the target directory (deepest first, while the target root is never deleted). For `process` the target is `--path` (operated on in-place), and for `import` it is `--outpath`. Implemented by `DirectoryCleaner.DeleteEmptyDirectories(root, dryRun)`. -- **`import` subcommand** (formerly `organize`): Copies (default) or moves supported media files from `--path` sources into `--outpath/date/filename` directory structure. Date comes from EXIF metadata (falls back to `DateTime.MinValue` -> `"0001/01/01"` bucket when absent). `--format` (default `"yyyy/MM/dd"`) controls subdirectory naming and is validated as a date-only format (no time components). Uses `GetUniqueFileName` for collision handling (`foo_1.jpg` etc.). Parallel via `--threads` (same as `process`). `--deleteempty` (default `false`) deletes empty child subdirectories from `--outpath` after all files are imported. `--move` (default `false`) moves files instead of copying. `--tagpath` (default `false`) splits the source sub-directory path into tokens and writes each token as an `XMP:Subject` tag on the destination file using exiftool. It is filtered by `s_exiftoolWriteExtensions` (`.3gp`, `.arw`, `.cr2`, `.dng`, `.gif`, `.heic`, `.heif`, `.jpeg`, `.jpg`, `.mov`, `.mp4`, `.nef`, `.orf`, `.png`, `.psd`, `.rw2`, `.tif`, `.tiff`) checked via `meta.FileTypeExtension`, and uses `-XMP:Subject-= / -XMP:Subject+=` to prevent duplicates while preserving existing tags. `--tags ` (optional) applies explicit comma-separated `XMP:Subject` tags to every imported file. `--datepath` (default `false`) infers the EXIF creation date from the source file path when no date is already embedded, then applies the date to the destination file before restoring mtime. **`--db ` (Import.db) is the source-side dedup DB**: rows are keyed by `path = source_path` (NOT dest path) and hold the source file's hash/size/mtime. On each source file, import calls `GetByPathAsync(source_path)` for source-side hash caching, then `Sha256ExistsAsync(source_hash)` to skip already-imported sources. New imports insert a row at the source path. **No command outside `import` writes to source-keyed rows**, so dedup cannot be clobbered by later runs of `process`/`index`. `--trashdb ` skips files whose **source-file** SHA-1 is in Trash.db. When import rewrites the destination via `--tags`/`--tagpath`/`--datepath`, its SHA-1 differs from the source SHA-1. Immich stored the destination SHA-1 from a prior upload, so the trash match is missed here and caught later by `process --trashdb`. `--skipdb ` skips files whose SHA-256 matches a reference DB (read-only). Cross-collection dedup is typically implemented by pointing `--skipdb` at another collection's Import.db. `--rehash` forces recomputation of all hashes ignoring the size/mtime cache. -- **`index` subcommand**: Iterates all files in `--path`, upserts each into the `files` DB table via `IndexTask.ExecuteAsync` (insert new, update if hash changed, skip unchanged). `--db ` is **required**. No `--dryrun` (always writes to DB). Supports `--threads` and `--rehash`. `--processed` (optional) marks newly-INSERTED rows with `is_processed = 1`, which is useful when seeding a Process.db from existing files so `process` treats them as already-done. The flag does not flip the flag on existing rows. Reports `inserted`/`updated`/`unchanged`/`ignored`/`failed` counts. -- **`trash` subcommand**: Syncs trashed asset checksums from an Immich server into a local SQLite trash database. `--url` (Immich server URL, required), `--trashdb ` (trash database, required), and the API key supplied by exactly one of `--apikey` (inline) or `--apikey-file` (path to a file whose trimmed contents are the key). The two API-key options are mutually exclusive and exactly one must be provided; `--apikey-file` must reference an existing, non-empty, readable file (existence enforced by an option validator, non-empty/readable by a command-level validator; read failures are translated to validation errors, never thrown). The key is resolved at parse time by `CommandLine.ResolveApiKey`/`ReadApiKeyFile` (file contents preferred and `.Trim()`-med) and flows into `Options.ImmichApiKey`. Uses `POST /api/search/metadata` with `trashedAfter` to fetch all trashed assets, converts Base64 SHA-1 checksums to hex, and inserts them via `INSERT OR IGNORE`. Full sync (idempotent, append-only). No `--dryrun`. -- **`--trashdb` Flag** (import, process): SQLite database file with Immich trash hashes (synced by `trash`). In `import`, files matching the trash DB are skipped, preventing re-import of photos the user trashed in Immich. The check is against the **source-file** SHA-1, so files whose destination SHA-1 was mutated by `import` itself (`--tags`/`--tagpath`/`--datepath`) will not match here even though Immich stored the mutated SHA-1. The `process --trashdb` command catches those on the next pass. In `process`, matching files are **deleted from disk and from Process.db** before the per-file processing pipeline runs (cleanup of files trashed in Immich after upload, and the safety net for the import source-vs-dest SHA-1 drift). The Trash.db check is the durable safety net beyond Immich's ~30-day trash retention. -- **Optional `--skipdb` Flag** (import only): SQLite database of files to skip (read-only SHA-256 check). Files whose SHA-256 matches a record in this DB are skipped without being recorded. Use this to skip files already present in another collection. -- **Optional `--rehash` Flag** (process, import, index): Forces SHA-256 recomputation for every file, ignoring the size/mtime cache. SHA-1 is also recomputed when `--trashdb` is in use. Useful after filesystem operations that preserve mtime but change content. -- **Optional `--duration` Flag** (process only): Overrides `ShortVideoDuration` (default `1.0`s). Videos in a live-photo-compatible format whose duration is <= this value are always deleted. Must be `> 0`. Stored in `CommandLine.Options.ShortVideoDuration` and read by `DeleteLivePhotosAsync`. -- **Optional `--reprocess` Flag** (process only): When set, ignores `is_processed` in the DB and forces every file to be processed again. Stored in `CommandLine.Options.Reprocess`, it disables the `IndexStatus.Unchanged && wasProcessed` early-return in `ExecuteAsync`. -- **Command Construction**: `CommandLine.SetAction` handlers create the appropriate command class (e.g., `ProcessCommand`, `ImportCommand`) with `CommandLine.Options` and `CancellationToken` -- **Built-in Help System**: Automatic help generation and validation - -## Development Workflow - -See [`CODESTYLE.md`](../CODESTYLE.md) for build requirements, formatting commands, and tooling. - -### Dependencies - -- **CliWrap**: External process execution -- **System.CommandLine**: Modern CLI argument parsing and validation -- **System.Text.Json**: High-performance JSON with source generation -- **Microsoft.Data.Sqlite**: SQLite database access for source file deduplication -- **Serilog**: Structured logging with console output -- **Native AOT**: Project configured for `PublishAot=true` with `InvariantGlobalization=true` -- **xUnit**: Testing framework for PhotoCleanerTests project - -### Test Architecture - -- **PhotoCleanerTests Project**: 300 comprehensive tests covering all functionality -- **InternalsVisibleTo**: Enables direct testing of internal methods without reflection -- **Test Categories**: - - `DateInferenceTests.cs`: Core date inference functionality (33 tests) - - `DateInferenceEdgeCasesTests.cs`: Date inference edge cases and integration (19 tests) - - `CommandLineTests.cs`: Command line parsing and validation (24 tests) - - `ProcessTaskTests.cs`: Process task tests (61 tests) -- **Coverage Areas**: Date inference (filename patterns, path structures, validation), command line interface (parsing, validation, error handling, multiple paths, thread configuration and boundary validation), integration scenarios, process task execution, live photo detection (ContentIdentifier matching, `_hevc` suffix naming, mismatch/missing tag scenarios), metadata preservation through conversion - -## Critical Implementation Details - -### Video Conversion Logic - -- **Three-tier approach**: Remux (.mts, .m2ts, .mkv) -> Re-encode (.wmv, .avi, .3gp, .gif) -> Audio-only (.mov/.mp4 with PCM) -- **Backup Strategy**: Original files renamed to `.bak` extension after successful conversion, and `BackupFile()` returns the backup path. A `{backup}.out` companion file (e.g. `img.gif.bak.out`) is written alongside the backup containing the full output path, this is needed when `GetUniqueFileName` appended a counter suffix (e.g. `img_1.mp4`) because the canonical name was already taken. When `options.SkipBackup` is true, no `.bak` or `.bak.out` files are created, and the original is deleted after conversion. -- **Metadata Preservation**: After every ffmpeg conversion, `exiftool -TagsFromFile -all:all -overwrite_original` copies all source metadata to the output file. `ffmpeg -map_metadata` is not used, it is unreliable for Apple QuickTime-specific tags (e.g. `ContentIdentifier` in the `mdta`/`keys` atom). `TagsFromFile` handles cross-format date mapping, so no separate date-setting step is needed after conversion. -- **Re-queue Pattern**: Converted files are added back to processing queue for validation - -### Live Photo Detection - -- **Short videos** (duration <= `options.ShortVideoDuration`, default `1.0s`, overridable via `--duration`): always deleted regardless of companion file -- **Companion file search** (`FindCompanionImagePath()`): looks for a HEIC/JPG/JPEG file by: - 1. Direct basename match (`IMG_1234.mov` -> `IMG_1234.heic`) - 2. Basename minus `_hevc` suffix (`IMG_1234_HEVC.mov` -> `IMG_1234.heic`), the newer iPhone naming -- **ContentIdentifier confirmation**: a candidate pair is only deleted when both files expose a `ContentIdentifier` tag that matches exactly. If either file lacks the tag, or the tags differ, the video is kept. There is no fallback to name-only deletion. -- **Long videos** (>= `LiveVideoDuration` = 4.0s): always kept even with a matching companion, and a warning is logged - -### Undo Architecture (UndoTask.cs) - -- **Backup naming**: `X.bak` (first), `X.bak1`, `X.bak2`, ... (subsequent runs of `process`) -- **`FileEnumerator.Enumerate()`** enumerates all files including `.bak*` files before calling `Execute()` -- **Two-pass algorithm** in `UndoTask.Execute()`: - - *Pass 1 - Identify derived bases*: - - **Rule 1**: any numbered backup (`.bak1`, `.bak2`, ...) present -> base is derived - - **Rule 2**: `.mp4` base with same-stem non-`.mp4` primary backup in same dir -> base is derived - - *Pass 2 - Act*: - - Derived base: delete current file + all its backups - - Non-derived base: delete current file if present, restore `X.bak` -> `X`; then locate the derived conversion output: if `X.bak.out` companion exists read the explicit output path from it and delete that file (handles uniquified names like `img_1.mp4`); otherwise fall back to checking whether `stem.mp4` exists and has no backup (legacy single-run heuristic) -- **Internal static helpers** (testable via `InternalsVisibleTo`): - - `IsBackupFile(path)`: matches `.bak\d*$` - - `IsNumberedBackup(path)`: matches `.bak\d+$` - - `GetBackupBase(path)`: strips the `.bak\d*` suffix -- **Dry run**: logs all intended operations but performs no file I/O -- **Known limitation**: extension renames to a previously non-existent filename create no backup and cannot be undone - -### Error Handling Strategy - -- Console output uses structured prefixes: `WARNING:`, `INFORMATION:` -- External command failures throw `CommandExecutionException` -- Methods return `false` to skip file processing rather than throwing exceptions - -## File Processing Extensions - -Supported: `.3gp`, `.arw`, `.avi`, `.cr2`, `.dng`, `.gif`, `.heic`, `.heif`, `.jpeg`, `.jpg`, `.m2ts`, `.mkv`, `.mov`, `.mp4`, `.mts`, `.nef`, `.orf`, `.png`, `.rw2`, `.tif`, `.tiff`, `.wmv` - -## Command Line Usage - -```bash -# Basic usage -PhotoCleaner process --path /photos - -# Dry run mode -PhotoCleaner process --path /photos --dryrun - -# Custom thread count -PhotoCleaner process --path /photos --threads 8 - -# Skip backup files (no .bak created, undo not possible) -PhotoCleaner process --path /photos --skipbackup - -# Process and remove empty subdirectories from --path afterward -PhotoCleaner process --path /photos --deleteempty - -# Undo last process run -PhotoCleaner undo --path /photos -PhotoCleaner undo --path /photos --dryrun - -# Import: copy media files from /Originals into date-based subdirectories under /Processed -PhotoCleaner import --path /photos --outpath /organized -PhotoCleaner import --path /photos --outpath /organized --format "yyyy/MM/yyyy-MM-dd" -PhotoCleaner import --path /photos --outpath /organized --dryrun - -# Import with move (removes source files) -PhotoCleaner import --path /photos --outpath /organized --move - -# Import with path-based tags (adds sub-directory tokens as XMP:Subject) -PhotoCleaner import --path /photos --outpath /organized --tagpath - -# Import with explicit tags applied to every file -PhotoCleaner import --path /photos --outpath /organized --tags "vacation,family" - -# Import with date inference from path (sets EXIF date when missing) -PhotoCleaner import --path /photos --outpath /organized --datepath - -# Import with deduplication DB (skip sources already imported) -PhotoCleaner import --path /icloud/originals --outpath /intermediate --db /data/Import.db - -# Stage-specific DBs: import tracks source identity, process tracks dest state -PhotoCleaner import --path /icloud/originals --outpath /processed --db /processed/Import.db --trashdb /data/Trash.db -PhotoCleaner process --path /processed --db /processed/Process.db --trashdb /data/Trash.db -# subsequent runs: only new sources are imported; only new dest files are processed - -# Index: hash a tree into a DB. Use to seed Import.db (no flag) or Process.db (--processed). -PhotoCleaner index --path /icloud/originals --db /processed/Import.db -PhotoCleaner index --path /processed --db /processed/Process.db --processed - -# Cross-collection dedup: point a secondary import's --skipdb at the primary collection's Import.db -PhotoCleaner import --path /Originals/Pictures --outpath /Processed/Pictures --db /Processed/Pictures/Import.db \ - --skipdb /Processed/iCloud/Import.db - -# Trash: sync Immich trash hashes into a local DB -PhotoCleaner trash --url http://immich:2283 --apikey YOUR_API_KEY --trashdb /data/Trash.db - -# Trash: supply the API key from a file instead of inline (keeps the secret out of shell history/process args) -PhotoCleaner trash --url http://immich:2283 --apikey-file /secrets/immich_api_key.txt --trashdb /data/Trash.db - -# Import with trash skip (prevents re-importing files trashed in Immich, even after Immich purges trash) -PhotoCleaner import --path /photos --outpath /organized --db /data/Import.db --trashdb /data/Trash.db - -# Process with trash delete (cleans up files trashed in Immich after upload, before re-uploading) -PhotoCleaner process --path /organized --db /data/Process.db --trashdb /data/Trash.db - -# Import with skip DB (skip files already in another collection) -PhotoCleaner import --path /photos --outpath /organized --skipdb /data/existing-collection.db - -# Full workflow with trash integration and per-stage DBs -PhotoCleaner trash --url http://immich:2283 --apikey $IMMICH_KEY --trashdb /data/Trash.db -PhotoCleaner import --path /icloud --outpath /processed --db /processed/Import.db --trashdb /data/Trash.db -PhotoCleaner process --path /processed --db /processed/Process.db --trashdb /data/Trash.db - -# Help -PhotoCleaner --help -PhotoCleaner process --help -PhotoCleaner import --help -PhotoCleaner index --help -PhotoCleaner trash --help -``` - -## JSON Source Generation - -Uses `SourceGenerationContext` for AOT-compatible JSON serialization of `ExifToolJson` metadata. -Uses `ImmichJsonContext` for AOT-compatible JSON serialization of Immich API models. - -## Testing Strategy - -- **Direct Method Testing**: Uses `InternalsVisibleTo` for compile-time safe method calls -- **Comprehensive Coverage**: Tests all filename patterns, path structures, date validation, and CLI parsing -- **Integration Testing**: Validates end-to-end date inference and command line interface logic -- **No Reflection**: All tests use direct method calls for better performance and maintainability - -### Command Line Testing Patterns - -- **CreateTestCommand() Helper**: Uses `CommandLine.CreateRootCommand()` directly for single source of truth -- **Type-based Option Extraction**: Identifies options by type (`Option>`, `Option`, `Option`) using 4-tuple destructuring -- **Real Directory Testing**: Uses `Directory.GetCurrentDirectory()` for path validation tests -- **Parse Result Validation**: Tests both success/error states and extracted argument values, including list counts for multiple paths and thread values -- **Comprehensive Scenarios**: Single path, multiple paths, thread configuration, option properties, argument parsing, validation errors, edge cases, default values -- **Multiple Path Testing**: Validates 2-path and 3-path scenarios, mixed valid/invalid paths, and proper list indexing -- **Thread Option Testing**: Validates thread count parsing, default value calculation, short option, and combined option scenarios +Stop and report the uncertainty. Do not guess at an instruction, suppress a possible finding, or +claim coverage that the review did not perform. diff --git a/AGENTS.md b/AGENTS.md index c8d3bf1..4e98785 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,7 +2,7 @@ **PhotoCleaner** is a .NET 10 console application that prepares media files (photos and videos) for import into photo management systems such as Lightroom, Immich, and PhotoPrism. It analyzes and transforms files through validation, conversion, and verification phases, orchestrating external tools (ExifTool, FFmpeg) through CLI wrappers, with SQLite-backed deduplication and Immich trash integration. The repo also contains an xUnit test project (`PhotoCleanerTests/`) and a benchmark project (`PhotoCleanerBenchmarks/`). -This file is the entry point every coding agent reads first, and it holds only two things: the rules for managing context and delegation, which apply to every task, and a map of where every other rule lives. The rule text itself is in [`GOVERNANCE.md`](./GOVERNANCE.md), one section per topic. Code style lives in [`CODESTYLE.md`](./CODESTYLE.md), the CI/CD workflow contract in [`WORKFLOW.md`](./WORKFLOW.md), and this project's architecture, processing pipeline, per-command details, and test layout in [`.github/copilot-instructions.md`](./.github/copilot-instructions.md). +This file is the entry point every coding agent reads first, and it holds only two things: the rules for managing context and delegation, which apply to every task, and a map of where every other rule lives. The rule text itself is in [`GOVERNANCE.md`](./GOVERNANCE.md), one section per topic. Code style lives in [`CODESTYLE.md`](./CODESTYLE.md), the CI/CD workflow contract in [`WORKFLOW.md`](./WORKFLOW.md), and this project's architecture, processing pipeline, per-command details, and test layout in [`ARCHITECTURE.md`](./ARCHITECTURE.md). Treat this file and `GOVERNANCE.md` as authoritative for cross-cutting rules, and do not restate their rules elsewhere. This project's own conventions and behavioral contracts live in [`CODESTYLE.md`](./CODESTYLE.md), **not** in [`.github/copilot-instructions.md`](./.github/copilot-instructions.md), because that file targets GitHub Copilot / VS Code specifically, while this file and `GOVERNANCE.md` are the agent-agnostic ones every coding agent is directed to read, so any rule a reviewer must honor has to live in one of those two files to be provider-independent. diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md new file mode 100644 index 0000000..88d9a51 --- /dev/null +++ b/ARCHITECTURE.md @@ -0,0 +1,231 @@ +# Architecture + +PhotoCleaner is a .NET 10 console application that processes media files in preparation for import into photo management systems (Lightroom, Immich, PhotoPrism). It analyzes and transforms media files through validation, modification, and verification phases. + +## Project Structure + +- **Docker/**: Docker configuration + - `Dockerfile`: Two-stage build (SDK Alpine build -> runtime Alpine final) that installs `exiftool` and `ffmpeg` in the final stage +- **PhotoCleaner/**: Main console application + - `Program.cs`: Entry point with logger setup (Main only) + - `CommandLine.cs`: System.CommandLine implementation for CLI parsing (`process`, `undo`, `import`, `index`, `trash`, and `verify` subcommands) + - `MediaUtilities.cs`: Shared static utilities, `SupportedExtensions` (FrozenSet), `GetUniqueFileName`, `GetExifToolJsonAsync`, `SetCreateDateAsync`, video/duration constants + - `CommandRunner.cs`: Thin wrapper for command start/complete/error logging, taking `Func>` and returning the command's exit code + - `ExitCode.cs`: The shared exit-code contract, being `Success` (0), `Error` (1, command could not run), and `Failed` (2, ran to completion with per-file failures) + - `DatabaseScope.cs`: Generic async DB lifecycle helper (create, init, dispose) + - `TrashDatabaseScope.cs`: Same lifecycle helper pattern for `TrashDatabase` + - `FileEnumerator.cs`: Parallel file enumeration returning `(IReadOnlyList, int)` + - `DirectoryCleaner.cs`: Static helper that deletes empty subdirectories under a root (deepest-first, and the root itself is never deleted), used by `import` and `process` when `--deleteempty` is set + - `ProcessCommand.cs`: Process command orchestration, case conflict resolution, reprocessing loop, result reporting + - `ImportCommand.cs`: Import command orchestration (formerly `OrganizeCommand`) + - `IndexCommand.cs`: Index command orchestration + - `TrashCommand.cs`: Trash command orchestration, fetches trashed asset checksums from Immich API, stores SHA-1 hashes in a `TrashDatabase` + - `UndoCommand.cs`: Undo command orchestration + - `VerifyCommand.cs`: Verify command orchestration, which enumerates, runs `VerifyTask`, and reports counts + - `ProcessTask.cs`: Core file processing pipeline (validation, conversion, metadata) + - `UndoTask.cs`: Undo logic, two-pass algorithm that restores `.bak` files + - `ImportTask.cs`: Import logic, copies (default) or moves supported media files from source into date-based subdirectories under `--outpath`. Inserts a row keyed by SOURCE path into Import.db. Optional SQLite deduplication via `Database`. (Formerly `OrganizeTask`.) + - `VerifyTask.cs`: Verification logic, a decode pass that runs Immich's own `MediaRepository` inside `ghcr.io/immich-app/immich-server:release` via `docker run`, batching paths over stdin. Preflights the image before judging any file, so an infrastructure failure exits `Error` rather than marking files invalid + - `ImmichVerifyScript.cs`: The Node script run inside the Immich image, as const strings. Calls Immich's own compiled `MediaRepository`, `defaults`, and `ThumbnailConfig` rather than reimplementing the preview pipeline, so behavior tracks Immich across releases + - `VerifyResult.cs`: The AOT-compatible `ImmichVerifyLine` JSON model and its `ImmichVerifyJsonContext` source-generated context, together forming the container's output protocol + - `IndexTask.cs`: Common DB upsert logic used by `process` and `index` commands. `IndexFileAsync` (single-file) returns `(IndexStatus, sha256, sha1, wasProcessed)`. `ExecuteAsync` (batch parallel) returns `(inserted, updated, unchanged, ignored, failed)`. When `options.MarkProcessed` is true, newly inserted rows are marked `is_processed=1` (used by `index --processed` to seed Process.db). + - `Database.cs`: SQLite wrapper with a single `files` table (`path` PRIMARY KEY, `sha256`, `sha1`, `file_size`, `mtime_ticks`, `is_processed`), indexes on both hash columns, and size/mtime caching via `ResolveHashesAsync` to skip rehashing unchanged files. Every write computes both sha256 and sha1 in a single read pass. Both columns are non-null. + - `TrashDatabase.cs`: Simple SQLite wrapper for Immich trash hashes with a single `trash_hashes` table (`sha1` PRIMARY KEY), used by `trash`, `import`, and `process` commands + - `ImmichApiModels.cs`: AOT-compatible JSON models for Immich API (`ImmichSearchRequest`, `ImmichSearchResponse`, `ImmichAssetDto`) with `ImmichJsonContext` source generation + - `DateFromPath.cs`: Static utility class for date inference from filenames/paths + - `ExifToolJson.cs`: JSON model for ExifTool metadata, including the `ExifTool:Validate` verdict and `ParseValidate` which splits it into error and warning counts + - `SkippedExtensionTracker.cs`: Thread-safe tracker for unknown file extensions skipped during processing; used by all commands that filter by `MediaUtilities.SupportedExtensions` (`process`, `import`, `index`) + - `LoggerFactory.cs`: Serilog logger factory, reusing an already-configured `Log.Logger` when one exists rather than always creating a new one + - `Extensions.cs`: Extension methods for logging and error handling +- **PhotoCleanerTests/**: Comprehensive test project (`TempDirectoryFixture.cs` is the shared fixture, not a test class) + - `DateInferenceTests.cs`: Core date inference functionality tests + - `CommandLineTests.cs`: Command line parsing and validation tests + - `ProcessTaskTests.cs`: Process task tests + - `UndoTaskTests.cs`: Undo task tests + - `ExifToolJsonTests.cs`: ExifToolJson unit tests (includes GetDate, IsDngVersionNewer) + - `ImportTaskTests.cs`: Import task tests + - `DatabaseTests.cs`: Database tests + - `IndexTaskTests.cs`: IndexTask tests + - `TrashDatabaseTests.cs`: TrashDatabase tests + - `TrashCommandTests.cs`: TrashCommand tests with mock HTTP handler + - `DirectoryCleanerTests.cs`: DirectoryCleaner static helper tests + - `VerifyTaskTests.cs`: Verify protocol parsing and script-contract tests + +## Core Processing Pipeline + +The application uses a sequential validation pipeline where each method returns `bool` - +`false` stops processing the current file: + +```csharp +if (!RenameMismatchedMimeExtensions() + || !RenameMixedCaseExtensions() + || !await DeleteLivePhotosAsync() + || !await ConvertVideoAsync() + || !WarnDngVersion()) +``` + +Before that chain runs, `CheckExifToolValidation` acts on the `ExifTool:Validate` verdict that +rides along with the metadata read. Only an error count fails the file (`ProcessResult.Invalid`). +Warnings are logged at debug level, because roughly three quarters of healthy files in a real +collection carry at least one. + +## State Management Pattern + +- **Primary Constructor Parameters**: Command and task classes use C# 12 primary constructors. All task classes take `CommandLine.Options options` as their first parameter, plus any non-option runtime params (e.g., `Database`, shared collections). Command classes take `(CommandLine.Options options, CancellationToken cancellationToken)` and pass `options` directly to task constructors. +- **Command/Task Separation**: Command classes (e.g., `ProcessCommand`) handle orchestration (file enumeration, DB lifecycle, result logging), while task classes (e.g., `ProcessTask`) handle per-file business logic +- **Composable Infrastructure**: `CommandRunner`, `DatabaseScope`, and `FileEnumerator` are static helpers freely composed by command classes, no inheritance hierarchy +- **Shared Collections**: `ConcurrentBag` for file names, `ConcurrentDictionary` for unknown extensions with case-insensitive comparison +- **Parallel Processing**: Files processed using `Parallel.ForEachAsync` with `MaxDegreeOfParallelism` +- **External Tool Integration**: Uses `CliWrap` for all external command execution (exiftool, ffmpeg, ffprobe) +- **FrozenSet Collections**: All static readonly extension collections use `FrozenSet` with `StringComparer.OrdinalIgnoreCase` for O(1) lookups + +## Key Patterns & Conventions + +### External Tool Execution Pattern + +```csharp +BufferedCommandResult result = await Cli.Wrap("exiftool") + .WithArguments(["-groupNames", "-json", "-validate", "-all", filePath]) + .ExecuteBufferedAsync(); +``` + +- Always use array syntax for arguments: `["-arg1", "value"]` +- Use `BufferedCommandResult` for output capture, `CommandResult` for fire-and-forget +- JSON trimming pattern: `result.StandardOutput.AsSpan().Trim([' ', '\n', '\r', '[', ']'])` + +### Media File Processing Conventions + +- **FrozenSet Extensions**: Define supported extensions as `FrozenSet` with `StringComparer.OrdinalIgnoreCase` (e.g., `s_remuxExtensions`, `s_jpegExtensions`) +- **Case-Insensitive Matching**: Use FrozenSet `.Contains()` directly without `.ToLower()`, comparer handles case-insensitivity +- **File Type Categorization**: Group operations by file type requirements (remux vs re-encode vs audio-only) +- **Single-Pass Optimizations**: Prefer single-loop iterations with early exit over multiple LINQ passes +- **Skipped Extension Tracking**: Commands that filter files by `MediaUtilities.SupportedExtensions` pass a shared `SkippedExtensionTracker` instance to their task classes. The tracker collects unknown extensions (thread-safe via `Track()`), and the command calls `LogWarnings()` after processing to log them sorted. Used by `process`, `import`, and `index` commands. + +### EXIF/Metadata Handling + +- Uses `ExifToolJson` class with `JsonPropertyName` attributes for precise metadata field mapping +- Date validation prioritizes `EXIF:DateTimeOriginal` over `QuickTime:CreateDate` +- Custom `IsDateSet()` and `GetDateString()` methods handle metadata extraction logic +- `ContentIdentifier` property maps both `QuickTime:ContentIdentifier` and `Keys:ContentIdentifier` + group names (both occur in the wild for ISOBMFF files) returning whichever is set + +### Date Inference System (DateFromPath.cs) + +- **Static Internal Methods**: All methods are `internal static` for testability with `InternalsVisibleTo` +- **DateFromPath.InferCreatedDate()**: Main entry point, tries filename first, then path fallback +- **DateFromPath.ExtractDateFromFilename()**: Supports multiple filename patterns: + - `YYYYMMDD_HHMMSS` format (e.g., `20210502_200152957_iOS-1747.jpg`) + - `YYYYMMDD` format (e.g., `EX_20030219_3378.jpg`) + - `YYYY-MM-DD-HH-MM-SS` format (e.g., `PHOTO-2024-06-22-07-56-41.jpg`) + - `YYYY MM DD` format with spaces (e.g., `EV 2014 07 03_0003.tif`) +- **DateFromPath.ExtractDateFromPath()**: Extracts from directory structures and year-only fallback +- **DateFromPath.IsDateValid()**: Validates dates within 1900-current year range + +### Command Line Interface (CommandLine.cs) + +- **System.CommandLine Integration**: Uses modern .NET command line parsing +- **Six subcommands**: `process`, `undo`, `import`, `index`, `trash`, `verify`, each with their own option set +- **Required `--path` Parameter**: Single directory path using `Option`. Validated with `AcceptExistingOnly()` +- **Optional `--dryrun` Flag**: Non-destructive preview mode (process, undo, import, not index) +- **Optional `--threads` Parameter**: Controls parallel processing degree with `DefaultValueFactory = _ => Math.Min(Environment.ProcessorCount, 4)`. Validated to be > 0 and <= Environment.ProcessorCount using `Validators.Add()` (process, import, index) +- **Optional `--skipbackup` Flag** (process only): Skips all `.bak` file creation, originals are deleted/overwritten in-place. Logs a warning at startup. Disables undo. +- **Optional `--deleteempty` Flag** (process, import): After the command completes, deletes empty child subdirectories from the target directory (deepest first, while the target root is never deleted). For `process` the target is `--path` (operated on in-place), and for `import` it is `--outpath`. Implemented by `DirectoryCleaner.DeleteEmptyDirectories(root, dryRun)`. +- **`import` subcommand** (formerly `organize`): Copies (default) or moves supported media files from `--path` sources into `--outpath/date/filename` directory structure. Date comes from EXIF metadata (falls back to `DateTime.MinValue` -> `"0001/01/01"` bucket when absent). `--format` (default `"yyyy/MM/dd"`) controls subdirectory naming and is validated as a date-only format (no time components). Uses `GetUniqueFileName` for collision handling (`foo_1.jpg` etc.). Parallel via `--threads` (same as `process`). `--deleteempty` (default `false`) deletes empty child subdirectories from `--outpath` after all files are imported. `--move` (default `false`) moves files instead of copying. `--tagpath` (default `false`) splits the source sub-directory path into tokens and writes each token as an `XMP:Subject` tag on the destination file using exiftool. It is filtered by `s_exiftoolWriteExtensions` (`.3gp`, `.arw`, `.cr2`, `.dng`, `.gif`, `.heic`, `.heif`, `.jpeg`, `.jpg`, `.mov`, `.mp4`, `.nef`, `.orf`, `.png`, `.psd`, `.rw2`, `.tif`, `.tiff`) checked via `meta.FileTypeExtension`, and uses `-XMP:Subject-= / -XMP:Subject+=` to prevent duplicates while preserving existing tags. `--tags ` (optional) applies explicit comma-separated `XMP:Subject` tags to every imported file. `--datepath` (default `false`) infers the EXIF creation date from the source file path when no date is already embedded, then applies the date to the destination file before restoring mtime. **`--db ` (Import.db) is the source-side dedup DB**: rows are keyed by `path = source_path` (NOT dest path) and hold the source file's hash/size/mtime. On each source file, import calls `GetByPathAsync(source_path)` for source-side hash caching, then `Sha256ExistsAsync(source_hash)` to skip already-imported sources. New imports insert a row at the source path. **No command outside `import` writes to source-keyed rows**, so dedup cannot be clobbered by later runs of `process`/`index`. `--trashdb ` skips files whose **source-file** SHA-1 is in Trash.db. When import rewrites the destination via `--tags`/`--tagpath`/`--datepath`, its SHA-1 differs from the source SHA-1. Immich stored the destination SHA-1 from a prior upload, so the trash match is missed here and caught later by `process --trashdb`. `--skipdb ` skips files whose SHA-256 matches a reference DB (read-only). Cross-collection dedup is typically implemented by pointing `--skipdb` at another collection's Import.db. `--rehash` forces recomputation of all hashes ignoring the size/mtime cache. +- **`index` subcommand**: Iterates all files in `--path`, upserts each into the `files` DB table via `IndexTask.ExecuteAsync` (insert new, update if hash changed, skip unchanged). `--db ` is **required**. No `--dryrun` (always writes to DB). Supports `--threads` and `--rehash`. `--processed` (optional) marks newly-INSERTED rows with `is_processed = 1`, which is useful when seeding a Process.db from existing files so `process` treats them as already-done. The flag does not flip the flag on existing rows. Reports `inserted`/`updated`/`unchanged`/`ignored`/`failed` counts. +- **`trash` subcommand**: Syncs trashed asset checksums from an Immich server into a local SQLite trash database. `--url` (Immich server URL, required), `--trashdb ` (trash database, required), and the API key supplied by exactly one of `--apikey` (inline) or `--apikey-file` (path to a file whose trimmed contents are the key). The two API-key options are mutually exclusive and exactly one must be provided; `--apikey-file` must reference an existing, non-empty, readable file (existence enforced by an option validator, non-empty/readable by a command-level validator; read failures are translated to validation errors, never thrown). The key is resolved at parse time by `CommandLine.ResolveApiKey`/`ReadApiKeyFile` (file contents preferred and `.Trim()`-med) and flows into `Options.ImmichApiKey`. Uses `POST /api/search/metadata` with `trashedAfter` to fetch all trashed assets, converts Base64 SHA-1 checksums to hex, and inserts them via `INSERT OR IGNORE`. Full sync (idempotent, append-only). No `--dryrun`. +- **`--trashdb` Flag** (import, process): SQLite database file with Immich trash hashes (synced by `trash`). In `import`, files matching the trash DB are skipped, preventing re-import of photos the user trashed in Immich. The check is against the **source-file** SHA-1, so files whose destination SHA-1 was mutated by `import` itself (`--tags`/`--tagpath`/`--datepath`) will not match here even though Immich stored the mutated SHA-1. The `process --trashdb` command catches those on the next pass. In `process`, matching files are **deleted from disk and from Process.db** before the per-file processing pipeline runs (cleanup of files trashed in Immich after upload, and the safety net for the import source-vs-dest SHA-1 drift). The Trash.db check is the durable safety net beyond Immich's ~30-day trash retention. +- **Optional `--skipdb` Flag** (import only): SQLite database of files to skip (read-only SHA-256 check). Files whose SHA-256 matches a record in this DB are skipped without being recorded. Use this to skip files already present in another collection. +- **Optional `--rehash` Flag** (process, import, index): Forces SHA-256 recomputation for every file, ignoring the size/mtime cache. SHA-1 is also recomputed when `--trashdb` is in use. Useful after filesystem operations that preserve mtime but change content. +- **Optional `--duration` Flag** (process only): Overrides `ShortVideoDuration` (default `1.0`s). Videos in a live-photo-compatible format whose duration is <= this value are always deleted. Must be `> 0`. Stored in `CommandLine.Options.ShortVideoDuration` and read by `DeleteLivePhotosAsync`. +- **Optional `--reprocess` Flag** (process only): When set, ignores `is_processed` in the DB and forces every file to be processed again. Stored in `CommandLine.Options.Reprocess`, it disables the `IndexStatus.Unchanged && wasProcessed` early-return in `ExecuteAsync`. +- **Command Construction**: `CommandLine.SetAction` handlers create the appropriate command class (e.g., `ProcessCommand`, `ImportCommand`) with `CommandLine.Options` and `CancellationToken` +- **Built-in Help System**: Automatic help generation and validation + +## Development Workflow + +See [`CODESTYLE.md`](./CODESTYLE.md) for build requirements, formatting commands, and tooling. + +### Dependencies + +- **CliWrap**: External process execution +- **System.CommandLine**: Modern CLI argument parsing and validation +- **System.Text.Json**: High-performance JSON with source generation +- **Microsoft.Data.Sqlite**: SQLite database access for source file deduplication +- **Serilog**: Structured logging with console output +- **Native AOT**: Project configured for `PublishAot=true` with `InvariantGlobalization=true` +- **xUnit**: Testing framework for PhotoCleanerTests project + +### Test Architecture + +- **PhotoCleanerTests Project**: comprehensive test coverage across all commands, see the file list under Project Structure above rather than a count here, which drifts every time a test is added +- **InternalsVisibleTo**: Enables direct testing of internal methods without reflection +- **Coverage Areas**: Date inference (filename patterns, path structures, validation), command line interface (parsing, validation, error handling, multiple paths, thread configuration and boundary validation), integration scenarios, process task execution, live photo detection (ContentIdentifier matching, `_hevc` suffix naming, mismatch/missing tag scenarios), metadata preservation through conversion + +## Critical Implementation Details + +### Video Conversion Logic + +- **Three-tier approach**: Remux (.m2t, .mkv) -> Re-encode (.asf, .wmv, .avi, .3gp, .gif) -> Audio-only re-encode (.mov/.mp4 with PCM audio) +- **Backup Strategy**: Original files renamed to `.bak` extension after successful conversion, and `BackupFile()` returns the backup path. A `{backup}.out` companion file (e.g. `img.gif.bak.out`) is written alongside the backup containing the full output path, this is needed when `GetUniqueFileName` appended a counter suffix (e.g. `img_1.mp4`) because the canonical name was already taken. When `options.SkipBackup` is true, no `.bak` or `.bak.out` files are created, and the original is deleted after conversion. +- **Metadata Preservation**: After every ffmpeg conversion, `exiftool -TagsFromFile -all:all -overwrite_original` copies all source metadata to the output file. `ffmpeg -map_metadata` is not used, it is unreliable for Apple QuickTime-specific tags (e.g. `ContentIdentifier` in the `mdta`/`keys` atom). `TagsFromFile` handles cross-format date mapping, so no separate date-setting step is needed after conversion. +- **Re-queue Pattern**: Converted files are added back to processing queue for validation + +### Live Photo Detection + +- **Short videos** (duration <= `options.ShortVideoDuration`, default `1.0s`, overridable via `--duration`): always deleted regardless of companion file +- **Companion file search** (`FindCompanionImagePath()`): looks for a HEIC/JPG/JPEG file by: + 1. Direct basename match (`IMG_1234.mov` -> `IMG_1234.heic`) + 2. Basename minus `_hevc` suffix (`IMG_1234_HEVC.mov` -> `IMG_1234.heic`), the newer iPhone naming +- **ContentIdentifier confirmation**: a candidate pair is only deleted when both files expose a `ContentIdentifier` tag that matches exactly. If either file lacks the tag, or the tags differ, the video is kept. There is no fallback to name-only deletion. +- **Long videos** (>= `LiveVideoDuration` = 4.0s): always kept even with a matching companion, and a warning is logged + +### Undo Architecture (UndoTask.cs) + +- **Backup naming**: `X.bak` (first), `X.bak1`, `X.bak2`, ... (subsequent runs of `process`) +- **`FileEnumerator.Enumerate()`** enumerates all files including `.bak*` files before calling `Execute()` +- **Two-pass algorithm** in `UndoTask.Execute()`: + - *Pass 1 - Identify derived bases*: + - **Rule 1**: any numbered backup (`.bak1`, `.bak2`, ...) present -> base is derived + - **Rule 2**: `.mp4` base with same-stem non-`.mp4` primary backup in same dir -> base is derived + - *Pass 2 - Act*: + - Derived base: delete current file + all its backups + - Non-derived base: delete current file if present, restore `X.bak` -> `X`; then locate the derived conversion output: if `X.bak.out` companion exists read the explicit output path from it and delete that file (handles uniquified names like `img_1.mp4`); otherwise fall back to checking whether `stem.mp4` exists and has no backup (legacy single-run heuristic) +- **Internal static helpers** (testable via `InternalsVisibleTo`): + - `IsBackupFile(path)`: matches `.bak\d*$` + - `IsNumberedBackup(path)`: matches `.bak\d+$` + - `GetBackupBase(path)`: strips the `.bak\d*` suffix +- **Dry run**: logs all intended operations but performs no file I/O +- **Known limitation**: extension renames to a previously non-existent filename create no backup and cannot be undone + +### Error Handling Strategy + +- Console output uses structured prefixes: `WARNING:`, `INFORMATION:` +- External command failures throw `CommandExecutionException` +- Methods return `false` to skip file processing rather than throwing exceptions + +## File Processing Extensions + +Supported: `.3gp`, `.arw`, `.asf`, `.avi`, `.cr2`, `.dng`, `.gif`, `.heic`, `.heif`, `.jpeg`, `.jpg`, `.m2t`, `.m2ts`, `.mkv`, `.mov`, `.mp4`, `.mts`, `.nef`, `.orf`, `.png`, `.psd`, `.rw2`, `.tif`, `.tiff`, `.wmv` + +## JSON Source Generation + +Uses `ExifToolJsonContext` for AOT-compatible JSON serialization of `ExifToolJson` metadata, +`ImmichJsonContext` for the Immich API models, and `ImmichVerifyJsonContext` for the +`ImmichVerifyLine` protocol the `verify` command reads from the containerized decoder. + +## Testing Strategy + +- **Direct Method Testing**: Uses `InternalsVisibleTo` for compile-time safe method calls +- **Comprehensive Coverage**: Tests all filename patterns, path structures, date validation, and CLI parsing +- **Integration Testing**: Validates end-to-end date inference and command line interface logic +- **No Reflection**: All tests use direct method calls for better performance and maintainability + +### Command Line Testing Patterns + +- **CreateTestCommand() Helper**: Uses `CommandLine.CreateRootCommand()` directly for single source of truth +- **Type-based Option Extraction**: Identifies options by type (`Option>`, `Option`, `Option`) using 4-tuple destructuring +- **Real Directory Testing**: Uses `Directory.GetCurrentDirectory()` for path validation tests +- **Parse Result Validation**: Tests both success/error states and extracted argument values, including list counts for multiple paths and thread values +- **Comprehensive Scenarios**: Single path, multiple paths, thread configuration, option properties, argument parsing, validation errors, edge cases, default values +- **Multiple Path Testing**: Validates 2-path and 3-path scenarios, mixed valid/invalid paths, and proper list indexing +- **Thread Option Testing**: Validates thread count parsing, default value calculation, short option, and combined option scenarios diff --git a/CODESTYLE.md b/CODESTYLE.md index 51bc666..b6c0016 100644 --- a/CODESTYLE.md +++ b/CODESTYLE.md @@ -16,9 +16,9 @@ Use each tool's official casing in task labels, docs, and prose, per the `commen Each language defines a **clean-compile** verification: the combination of build, formatter, linter, and code-analysis tools that must report clean before a commit. It is exposed as one or more **named** VS Code tasks (or, where a language ships no tasks, documented commands), and those definitions are the same across the fleet. The concrete names live in each language section below. -- **Run it after every code change, and it is not the whole gate.** The relevant language's clean-compile must pass before you commit. CI runs those same language checks as a backstop **plus everything else its validation workflow runs**, and all of it reports into the one required status, so a green clean-compile does not predict a green CI. That remainder is at least the doc-lint set (markdownlint, cspell, actionlint, `editorconfig-checker`) and whatever spec, config, and script gates the repo carries, so read the workflow for the full list rather than assuming this sentence enumerates it. What has to pass before a push is the repo's **whole** lint gate, per [GOVERNANCE.md "Verification Discipline"][governance-verification-discipline]. Each linter's known-working invocation is in [GOVERNANCE.md "Running the Linters Locally"][governance-running-the-linters-locally]. +- **Run it after every code change, and it is not the whole gate.** The relevant language's clean-compile must pass before you commit. CI runs those same language checks as a backstop **plus everything else its validation workflow runs**, and all of it reports into the one required status, so a green clean-compile does not predict a green CI. That remainder is at least the doc-lint set (markdownlint, cspell, actionlint, `editorconfig-checker`) and whatever spec, config, and script gates the repo carries, so read the workflow for the full list rather than assuming this sentence enumerates it. What has to pass before a push is the repo's **whole** lint gate, per [GOVERNANCE.md "Verification Discipline"][governance-verification-discipline]. Each linter's known-working invocation is in [OPERATIONS.md "Local Verification"][operations-local-verification]. - **The named task definition is the canonical spec** - its exact command sequence, arguments, and strictness. You may run it through the VS Code task **or** by invoking the equivalent native commands directly, and either is fine **only if the sequence, arguments, and strictness match exactly**. No shortcuts and no more-lenient options (for example, never drop `--verify-no-changes` or loosen a `--severity`). -- **A local commit/pre-commit gate is the repo's choice.** No single hook runner fits every language (a `dotnet`-tool runner like Husky.Net suits .NET but not Python), so none is mandated, but that is **not** a recommendation against commit gates. CI is the authoritative backstop regardless, and a local gate is an additive convenience a repo may wire and keep: Husky.Net (and `dotnet husky run` as a style step) for .NET, `pre-commit` for Python. Keeping a working gate is not drift. +- **A working local commit/pre-commit gate is strongly suggested, not the repo's free choice to skip.** No single hook runner fits every language (a `dotnet`-tool runner like Husky.Net suits .NET but not Python), so the *mechanism* stays the repo's choice: Husky.Net for .NET, `pre-commit` for Python, canonical configs for both in the hub's `catalog/snippets/` (not carried into this repo). What that gate must cover, and what its absence means for the audit, is [OPERATIONS.md "Local Verification"][operations-local-verification], not restated here. Keeping a working gate is not drift. ### Analyzer Diagnostics and Suppressions @@ -59,13 +59,13 @@ This is packaged as the `python-codestyle` Skill at `.agents/skills/python-codes ## Shell -Bash, and only where a program cannot be Python: a bootstrap that installs the interpreter cannot be written in it, and a host tool that must run before a development toolchain exists cannot depend on one. Everything else is Python, with a test under the scripts tree's `tests/` directory. The mandatory `set -Eeuo pipefail` header, the pipefail-versus-early-reader pitfall, self-locating scripts, `shellcheck` cleanliness, and the why-not-what comment rule are packaged as the `shell-codestyle` Skill at `.agents/skills/shell-codestyle/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo. Read the skill for the full rules. +Bash, and only where a program cannot be Python: a bootstrap that installs the interpreter cannot be written in it, and a host tool that must run before a development toolchain exists cannot depend on one. Everything else is Python, with a test under the scripts tree's `tests/` directory. The mandatory `set -Eeuo pipefail` header, the pipefail-versus-early-reader pitfall, self-locating scripts, the `shellcheck`-plus-`shfmt` clean-compile, and the why-not-what comment rule are packaged as the `shell-codestyle` Skill at `.agents/skills/shell-codestyle/SKILL.md` in the hub, not a repo-relative link since that path is hub-local and not carried into every fleet repo. Read the skill for the full rules. Run the clean-compile check itself per [OPERATIONS.md "Local Verification"][operations-local-verification], not by probing `command -v shellcheck`. [governance]: ./GOVERNANCE.md -[governance-running-the-linters-locally]: ./GOVERNANCE.md#running-the-linters-locally-known-working-invocations [governance-verification-discipline]: ./GOVERNANCE.md#verification-discipline +[operations-local-verification]: ./OPERATIONS.md#local-verification [readme]: ./README.md [root]: ./.editorconfig [vscode-tasks]: ./.vscode/tasks.json diff --git a/GOVERNANCE.md b/GOVERNANCE.md index a1dcc13..5585d6e 100644 --- a/GOVERNANCE.md +++ b/GOVERNANCE.md @@ -254,7 +254,7 @@ Every repo's GitHub repository details (the About panel) follow a fixed conventi - [`AGENTS.md`](./AGENTS.md): the agent entry point, carrying context and delegation rules plus the map to the sections above. - [`GOVERNANCE.md`](./GOVERNANCE.md), [`CODESTYLE.md`](./CODESTYLE.md), [`WORKFLOW.md`](./WORKFLOW.md), [`AUDIT.md`](./AUDIT.md): the governance and audit docs. This file is the cross-cutting-rules authority, `AUDIT.md` the self-audit of settings, rulesets, and secret names. -- [`.github/copilot-instructions.md`](./.github/copilot-instructions.md): the Copilot review runbook, plus this project's architecture, processing pipeline, per-command details, and test layout. +- [`.github/copilot-instructions.md`](./.github/copilot-instructions.md): the Copilot bootstrap and review runbook. [`ARCHITECTURE.md`](./ARCHITECTURE.md): this project's architecture, processing pipeline, per-command implementation details, and test layout. - [`PhotoCleaner/`](./PhotoCleaner/): the console application. [`PhotoCleanerTests/`](./PhotoCleanerTests/): the xUnit test project. [`PhotoCleanerBenchmarks/`](./PhotoCleanerBenchmarks/): the BenchmarkDotNet project. [`PhotoCleaner.slnx`](./PhotoCleaner.slnx) is the solution. - [`Docker/`](./Docker/): the Dockerfile and the Docker Hub overview the release pipeline publishes. - [`.github/workflows/`](./.github/workflows/): the pull request gate, the reusable build tasks, and the publisher. diff --git a/WORKFLOW.md b/WORKFLOW.md index e06aa6f..e7a75c7 100644 --- a/WORKFLOW.md +++ b/WORKFLOW.md @@ -1,6 +1,6 @@ # WORKFLOW.md -The guide for CI/CD **workflows** (GitHub Actions): a deliberate mixture of code style, architecture, a **behavioral contract** (expected inputs and outputs), and a **test methodology**. Code style lives in [`CODESTYLE.md`][codestyle]. This file is its sibling for everything under [`.github/workflows/`][workflows]. +The guide for CI/CD **workflows** (GitHub Actions): a deliberate mixture of code style, architecture, a **behavioral contract** (expected inputs and outputs), and a **test methodology**. Code style lives in [`CODESTYLE.md`][codestyle]. This file is its sibling for everything under `.github/workflows/`. Its defining principle: **it describes required outcomes, not a required implementation.** Two repos may implement the same guarantee with different YAML. A workflow is correct when it **satisfies the contract** in section 4 and is **defect-free against the expected inputs and outputs**, not when it matches a reference implementation byte for byte. The conventions in section 2 keep workflows legible. The contract in section 4 is what they must *do*. @@ -68,7 +68,7 @@ Their CI is lint/validation only (editorconfig/EOL plus domain linters such as H ### Two Layers: Orchestration vs Build -- **Orchestration** is generic and forms the standardization baseline **at the job level**: the single-branch publisher, the `get-version`, `validate-release`, and `github-release` jobs, the date-badge job, and the `changes -> smoke-build -> aggregator` shape of the PR workflow. These job *bodies* should not need per-repo edits. +- **Orchestration** is generic and forms the standardization baseline **at the job level**: the single-branch publisher, the `get-version`, `validate-release`, and `github-release` jobs, and the `changes -> smoke-build -> aggregator` shape of the PR workflow. These job *bodies* should not need per-repo edits. - **Build** is repo-owned: the `build--task.yml` leaf tasks. - **What the repo curates** (by design, not a leak): the *list* of targets. This is **not** a byte-for-byte file carry. Adding or dropping a target edits the orchestrator's surface: the `enable_` inputs and the `build-` job + its `github-release` `needs:` entry in the release task, **and** the `changes` paths-filter entry + output + the `smoke-build` enable-forward in the PR workflow. "Verbatim" applies to the `github-release` job and the version/publish-plan logic, not to the release task's job list or the paths-filter. Subsetting is symmetric: the same surface you trim to drop a target you extend to add a new one (e.g. a `release-asset--library` producer needs a new `enable_library` input, a `build-library` job, a `needs:` entry, and a `library` paths-filter). @@ -78,8 +78,8 @@ A target contributes a file to the GitHub release by uploading a workflow artifa ```mermaid flowchart LR - leafa[leaf: target A] -->|release-asset-branch-A| store[(run artifacts)] - leafb[leaf: target B] -->|release-asset-branch-B| store + dotnet[dotnet-publish] -->|release-asset--dotnet-publish| store[(run artifacts)] + nuget[build-nuget] -->|release-asset--nuget| store store -->|pattern + merge-multiple| rel["github-release job (D6)"] reg[registry leaf: nuget / pypi / docker] -->|push, no asset| registries[(registries)] ``` @@ -98,7 +98,7 @@ When a workflow's inputs carry a cross-input or input-versus-derived-state invar ### Resource Lifecycle -Workflow artifacts are an **intra-run handoff** only. Durable copies live on the release/registry. The rule: a transfer artifact handed **between jobs** is deleted by exact name/pattern **at its point of consumption**, the delete is **gated to the same condition as the consumer**, and it is **best-effort**. **Every** `upload-artifact` sets `retention-days: 1` as the universal failure-path backstop, so no terminal blanket-delete job is needed, and an intermediate consumed only within the same run (e.g. an executable's per-runtime outputs feeding an aggregation step) may rely on the retention backstop alone. The run is **never** blanket-deleted (`.artifacts[].id`). See D5. +Workflow artifacts are an **intra-run handoff** only. Durable copies live on the release/registry. The rule: a transfer artifact handed **between jobs** is deleted by exact name/pattern **at its point of consumption**, the delete is **gated to the same condition as the consumer**, and it is **best-effort**. **Every** `upload-artifact` sets `retention-days: 1` as the universal failure-path backstop, so no terminal blanket-delete job is needed. An intermediate consumed only within the same run may rely on the retention backstop alone. The run is **never** blanket-deleted (`.artifacts[].id`). See D5. ### Fast PR Feedback @@ -136,7 +136,7 @@ Pick each output's path by **where the artifact goes**: - **Package-registry push** (NuGet, PyPI): the leaf builds and publishes to its registry. NuGet pushes from the leaf *and* uploads a `release-asset-*`. PyPI is **split**: the leaf only builds + uploads its build artifact, a separate publish job does the OIDC upload (so `id-token: write` is granted at one entry point, behind an environment gate) and contributes **no** `release-asset-*`. - **Image-registry push** (Docker): the leaf pushes the default branch multi-arch (amd64+arm64) and any other branch `amd64`-only (arm64 emulation is reserved for the released image), and contributes no `release-asset-*`. - **Filesystem on a host the project owns** (a static site, a config tree): the leaf builds the tree, ships it to the host, and contributes no `release-asset-*`. The transport is the repo's own. What the contract fixes is that the deploy is a **separate `workflow_dispatch`** from the release, so a redeploy of an unchanged commit mints no tag and a host rebuild, a rollback, or proving a branch on a non-production environment costs nothing; that its credentials come from a **per-environment GitHub Environment** rather than the repository secret store; and that the deploy ends by asserting **what the host serves** rather than the transport's exit status (D4.6). Retention at the destination is bounded by a declared count with one side recorded as owning the prune, which is the deploy where its credential can observe the destination and the host where that credential is deliberately write-only (D5.6). -- **No file target via the release task** (Docker-only, PyPI-only): the release is tag + source zip + README + LICENSE. The repo's **caller MUST pass `expect_release_assets: false`** to the release task (the input is never set by a publisher that ships file targets, which keeps the default `true`). This is the one case where the otherwise-verbatim publisher is edited. With the default `true` and no assets, the release-create step fails on `fail_on_unmatched_files`. A **source-only** repo has no release task at all. Its standalone `publish-release.yml` inlines `action-gh-release`, so `expect_release_assets` does not apply (see Section 6). +- **No file target via the release task** (Docker-only, PyPI-only, source-only): the release is tag + source zip + README + LICENSE. The caller **MUST pass `expect_release_assets: false`** to the release task. A publisher with file targets retains the default `true`. This setting is caller-specific. The default `true` fails on `fail_on_unmatched_files` when no assets exist. A **source-only** repo also passes every `enable_*` input as false because it has no build leaf (see Section 6). ## 4. Behavioral Contract: Expected Outcomes @@ -144,12 +144,12 @@ The required behaviors, organized by domain. Each is a **MUST**, stated as input ### D1 - PR Fast-Feedback (Smoke) -- **D1.1 Only changed targets build.** Input: a PR touching some targets. Output: the paths-filter marks exactly those targets and only their smoke builds run. Unchanged targets skip. A repo's own targets MUST each have a filter entry (so a touched target is never silently skipped). *Prevents: rebuilding everything, and a changed target slipping through unbuilt.* **This repo runs no paths filter**, so every push builds both targets. That is stricter than the guarantee and prevents the same failure by never letting a changed target go unbuilt, at the cost of building an unchanged one. -- **D1.2 A validation job always runs.** Input: any PR. Output: a type-appropriate validation job runs unconditionally and the aggregator `needs:` it. In a .NET repo this is the `unit-test` job (format/style/test). A non-.NET repo **replaces** it (not deletes) with its own validator (lint, schema-check) and re-points **every** `needs:` on it to the replacement. *Prevents: a PR merging with no validation, or a dangling `needs:` that fails the whole workflow to load.* Here `validate` and `smoke-build` are siblings rather than a chain: both run unconditionally and the aggregator `needs:` both, so validation cannot be skipped and neither waits on the other. +- **D1.1 Only changed targets build.** Input: a PR touching some targets. Output: the paths-filter marks exactly those targets and only their smoke builds run. Unchanged targets skip. A repo's own targets MUST each have a filter entry (so a touched target is never silently skipped). *Prevents: rebuilding everything, and a changed target slipping through unbuilt.* This repo builds one combined release target (dotnet-publish plus Docker together, one `smoke-build` job), so there is no target selection to make: the `changes` job's single `release` filter entry exists to implement D1.4 below rather than to choose between targets. +- **D1.2 A validation job always runs.** Input: any PR. Output: a type-appropriate validation job runs unconditionally and the aggregator `needs:` it. In a .NET repo this is the `unit-test` job (format/style/test). A non-.NET repo **replaces** it (not deletes) with its own validator (lint, schema-check) and re-points **every** `needs:` on it to the replacement. *Prevents: a PR merging with no validation, or a dangling `needs:` that fails the whole workflow to load.* Here `validate` runs unconditionally and the aggregator `needs:` it directly, independent of `changes`/`smoke-build`. - **D1.3 Smoke never publishes and never uploads.** Input: `smoke: true`. Output: full compile/lint/test, but no registry/image push, no release, and **no** artifact uploads (every `upload-artifact`, including any aggregation job, is gated `!smoke`). *Prevents: a PR publishing, and orphaned artifacts churning the storage quota.* -- **D1.4 Workflow-file changes are not smoke-built.** Input: a PR changing only `.github/workflows/**`. Output: the paths-filter excludes workflow files, so smoke-build skips. *Implication: a workflow-only change is not smoke-built, but actionlint still validates it in CI.* **N/A here**, since this repo runs no paths filter: a workflow-only change is smoke-built like any other. `test-pull-request.yml` runs on push so the reusable tasks resolve from the pushed head, which means such a change tests its own copy rather than the base branch's. -- **D1.5 One required aggregator gates merge.** Input: any PR. Output: a single aggregator job must **succeed**, `needs:` the changes job and the validation job, treat a **skipped** smoke build as pass, and **block** on `failure`/`cancelled`. Its name is ruleset-bound: the job `name:` and the ruleset `context:` are the same string and MUST be renamed together, never independently. *Prevents: a paths-filter error letting a target-changing PR merge unbuilt.* Here the aggregator is `check-workflow-status`, named `Check pull request workflow status job`, and it `needs: [validate, smoke-build]`. There is no changes job to need, and it demands **success** from both rather than treating a skip as pass, since neither is ever skipped. -- **D1.6 Coverage is reported to Codecov (C# and Python).** Input: a C# or Python repo's validation/test job. Output: tests run with coverage collection (`dotnet test --collect:"XPlat Code Coverage"` or `pytest --cov-report=xml`) and a `codecov/codecov-action` step uploads it, **best-effort** (`continue-on-error` and/or `fail_ci_if_error: false`, so a Codecov outage or an absent token never reds the gate). `CODECOV_TOKEN` lives in the repo's **actions** secret store and reaches the reusable validator via `secrets: inherit`. Required for **every** C# and Python repo that has tests (see `spec/secrets.json` `typeMechanisms`). The repo also ships a **`codecov.yml`** that sets the project and patch statuses to **`informational: true`** so a coverage delta never gates a PR (a distinct knob from `fail_ci_if_error`, which only guards the upload step) and excludes intentionally-untested, non-shipped code (an example/demo or benchmark project) from the coverage denominator via `ignore`. A repo may override this to enforce a coverage threshold where its quality bar requires it. Coverage output is a build artifact, so `.gitignore` excludes it (e.g. `coverage/`, `*.cobertura.xml`). The `.gitignore` file is the full source of truth, so a blanket `git add -A` won't stage the untracked output. *Prevents: coverage silently going unreported, a stale unused token, a coverage regression blocking an unrelated PR, or a coverage artifact committed by a blanket add.* +- **D1.4 Workflow-file changes are not smoke-built.** Input: a PR changing only `.github/workflows/**`. Output: the paths-filter excludes workflow files, so smoke-build skips. *Implication: a workflow-only change is not smoke-built, but actionlint still validates it in CI.* This repo's `changes` job filter is exactly this exclusion (`'!.github/workflows/**'`), so a PR touching only workflow files skips `smoke-build`. Any other change, including a docs-only one, matches the filter and runs it. +- **D1.5 One required aggregator gates merge.** Input: any PR. Output: a single aggregator job must **succeed**, `needs:` the changes job and the validation job, treat a **skipped** smoke build as pass, and **block** on `failure`/`cancelled`. Its name is ruleset-bound: the job `name:` and the ruleset `context:` are the same string and MUST be renamed together, never independently. *Prevents: a paths-filter error letting a target-changing PR merge unbuilt.* Here the aggregator is `check-workflow-status`, named `Check pull request workflow status job`, and it `needs: [changes, validate, smoke-build]`, treating each `success` or `skipped` result as passing and failing on anything else. +- **D1.6 Coverage is reported to Codecov (C# and Python).** Input: a C# or Python repo's validation/test job. Output: tests run with coverage collection (`dotnet test --collect:"XPlat Code Coverage"` or `pytest --cov-report=xml`) and a `codecov/codecov-action` step uploads it, **best-effort** (`continue-on-error` and/or `fail_ci_if_error: false`, so a Codecov outage or an absent token never reds the gate). `CODECOV_TOKEN` lives in the repo's **actions** secret store and reaches the reusable validator via `secrets: inherit`. Required for **every** C# and Python repo that has tests. Where this guarantee does not apply (a `lint-only` profile for that type, per `registry/repos.json`), the hub's `spec/secrets.json` `typeMechanisms` mapping is not claimed for that repo, and the absence is not drift. The repo also ships a **`codecov.yml`** that sets the project and patch statuses to **`informational: true`** so a coverage delta never gates a PR (a distinct knob from `fail_ci_if_error`, which only guards the upload step), and excludes intentionally-untested, non-shipped code (an example/demo or benchmark project) from the coverage denominator via `ignore`. A repo may override this to enforce a coverage threshold where its quality bar requires it. Coverage output is a build artifact, so `.gitignore` excludes it (e.g. `coverage/` and `*.cobertura.xml`, with `.gitignore` the full source of truth) so a blanket `git add -A` won't stage the untracked output. *Prevents: coverage silently going unreported; a stale, unused token; a coverage regression blocking an unrelated PR; a coverage artifact committed by a blanket add.* ### D2 - Input/State Validation at Entry @@ -168,16 +168,16 @@ The required behaviors, organized by domain. Each is a **MUST**, stated as input ### D4 - Release / Publish -- **D4.1 Gated single-branch publish.** Output: PRs smoke-test and publish nothing. A **human merge never auto-publishes**. The fleet shape puts a first `plan` job (`publish-plan-task.yml`) in front and gates every job on it, covering a **code-affecting bot push to `main`**, a **dispatch** of `main`/`develop`, and a **main-only weekly schedule**. **This repo carries no plan job and no push trigger at all**: `publish-release.yml` runs on `workflow_dispatch` or the weekly `main`-only schedule, and its single job gates on `github.ref_name` being `main` or `develop`, so a dispatch from a feature branch is a no-op. Nothing publishes on a push, bot or human, and accumulated changes ship in the next scheduled or dispatched run. Each run builds one branch. +- **D4.1 Gated single-branch publish.** Output: PRs smoke-test and publish nothing. A **human merge never auto-publishes**. The fleet shape puts a first `plan` job (`publish-plan-task.yml`) in front and gates every job on it, covering a **code-affecting bot push to `main`**, a **dispatch** of `main`/`develop`, and a **main-only weekly schedule**. This repo carries that `plan` job and gates `validate`/`publish` on its `outputs.publish`, but `publish-release.yml` declares **no push trigger at all**, only `workflow_dispatch` and the weekly `main`-only `schedule`, so the bot-push leg of `plan`'s decision never has a push event to evaluate and never fires here. Nothing publishes on a merge, and accumulated changes ship in the next scheduled or dispatched run. Each run builds one branch. - **D4.2 Tag the built commit.** Output: the release `target_commitish` is the built commit's SHA (NBGV's `GitCommitId`), never a branch name or a separately re-resolved ref. *Prevents: the tag landing on the default branch instead of the built tree.* -- **D4.3 Release contents.** Output: every release is a tag on the built commit plus the auto source zip, README, and LICENSE; file-producing targets attach `release-asset-*`; `prerelease` equals `branch != default`. A no-file-target repo that uses the release task (Docker-only, PyPI-only) reaches the tag-only shape **only** with `expect_release_assets: false` set by the caller (which relaxes `fail_on_unmatched_files` and skips the asset download). With the default `true` and no assets the release-create step fails. A source-only repo reaches the same shape through its inlined `action-gh-release` instead, with no release task or `expect_release_assets`. +- **D4.3 Release contents.** Output: every release contains a tag on the built commit plus the auto source zip, README, and LICENSE. File targets attach `release-asset-*`. The `prerelease` value equals `branch != default`. A no-file-target caller sets `expect_release_assets: false` to reach the tag-only shape. This applies to Docker-only, PyPI-only, and source-only repos. The setting relaxes `fail_on_unmatched_files` and skips the asset download. The release-create step fails when no assets exist and the setting retains its default `true`. A source-only caller also sets every `enable_*` input false. - **D4.4 No-op republish.** Input: a re-run whose version is unchanged. Output: nothing is re-pushed, because the release-create step is skipped when the tag exists (refreshed only on `workflow_dispatch`), and the paired asset-delete is skipped with it. Registry pushes are no-ops. The NuGet/PyPI publish steps are **not** statically gated on existence. They run and the **server** dedupes (`dotnet nuget push --skip-duplicate` turns a 409 into success, and PyPI does the same under `skip-existing: true`). **Docker always re-pushes** the image (base-image refresh), independently of the release-create skip, within the same run. *Prevents: duplicate releases and wasted pushes.* -- **D4.5 A build failure blocks every publish target.** Input: a real publish where one enabled build fails. Output: nothing publishes. `github-release` needs every build, so a failed build skips it (no tag, no release), and the terminal registry pusher (Docker) needs every other build and guards its `if` with `!failure() && !cancelled()`, so a failed build skips docker too (no image push) while a disabled or unchanged target (skipped, not failed) still lets docker build on smoke. *Prevents: a partial publish, e.g. a Docker image pushed while the executable build failed and no release was cut.* A repo pushing two registry targets at once would need a build/publish split behind an all-builds gate, which none does today. +- **D4.5 A build failure blocks every publish target.** Input: a real publish where one enabled build fails. Output: nothing publishes. `github-release` needs every build, so a failed build skips it (no tag, no release), and the terminal registry pusher (Docker) needs every other build and guards its `if` with `!failure() && !cancelled()`, so a failed build skips docker too (no image push) while a disabled or unchanged target (skipped, not failed) still lets docker push. *Prevents: a partial publish, e.g. a Docker image pushed while .NET publish failed and no release was cut.* A repo pushing two registry targets at once would need a build/publish split behind an all-builds gate, which none does today. - **D4.6 Deploy verification names the release.** Input: a deploy to a filesystem on a host the project owns that completes without error. Output: a check against the running host asserts **which release is answering**, not merely that it answers. The artifact stamps its own version into the configuration it ships, and the check compares that against the version just installed, **waiting for convergence to a bounded timeout** rather than sampling once, because content goes live the instant a pointer moves while server rules wait on an asynchronous reload. The same check asserts **which environment** answered, since several environments serve a byte-identical artifact and a proxy rule aimed at the wrong one answers healthily under the right hostname. An unreachable host is reported distinctly from an HTTP status. *Prevents: a green deploy over a host still serving the previous release's configuration, a URL contract checked against the wrong environment, and a dead config watcher read as a routing fault.* ### D5 - Resource Cleanup -- **D5.1 Delete at the point of consumption.** Output: the job that downloads a **cross-job** transfer artifact deletes it (by exact name/pattern) right after consuming it. An intermediate consumed only within the same run (e.g. an executable's per-runtime outputs feeding an in-run aggregation) MAY instead rely on the `retention-days: 1` backstop. *Prevents: transfer artifacts accumulating against the storage quota.* +- **D5.1 Delete at the point of consumption.** Output: the job that downloads a **cross-job** transfer artifact deletes it (by exact name/pattern) right after consuming it. An intermediate consumed only within the same run MAY instead rely on the `retention-days: 1` backstop. *Prevents: transfer artifacts accumulating against the storage quota.* - **D5.2 Gate the delete to the consumer's condition.** Output: the delete runs under the **same** condition as its consuming step. Where the consumer is conditional (the GitHub release create), the delete is conditional too. Where the consumer always runs when its job runs (the PyPI publish step), the delete always runs, so on a no-op re-run the `release-asset-*` delete is **skipped** while the PyPI build-artifact delete still **runs** (its publish ran). *Prevents: deleting freshly built assets on a no-op re-run.* - **D5.3 Best-effort.** Output: cleanup is `continue-on-error`, tolerates a failed listing, and deletes **all** matching ids. *Prevents: a cleanup hiccup reddening a job whose publish succeeded.* - **D5.4 Retention backstop.** Output: **every** `upload-artifact` sets `retention-days: 1`. @@ -202,7 +202,7 @@ The required behaviors, organized by domain. Each is a **MUST**, stated as input - **D8.1 Merge-bot.** Output: enables auto-merge on `opened`/`reopened` for **every** Dependabot tier including semver-major (the required checks are the gate, not the bump magnitude); dispatches `--squash`/`--merge` by the PR's base ref; disables on a maintainer-pushed `synchronize`; concurrency keyed on the **PR number**, not `github.ref`. *Prevents: two PRs colliding in auto-merge.* - **D8.2 CodeGen and Dependabot.** Output: codegen runs as a matrix over both branches and is deterministic from an external source. Dependabot targets both branches, security PRs to default. -- **D8.3 Upstream-version tracker.** Output: a scheduled resolver prints a JSON `name -> version` object to a committed state file, opens a rolling per-branch bump PR naming only the moved keys, the merge-bot auto-merges it. The `main` pin push publishes via the release gate, while a `develop` pin does not auto-publish. It ships via a `develop` dispatch (prerelease) or the next promotion to `main`. The tracker's `bump-branch-prefix` + `branches` MUST match the merge-bot's hard-coded `-` head/base pairs, or auto-merge silently never fires. +- **D8.3 Upstream-version tracker.** Output: a scheduled resolver prints a JSON `name -> version` object to a committed state file, opens a rolling per-branch bump PR naming only the moved keys, the merge-bot auto-merges it. The `main` pin push publishes via the release gate, while a `develop` pin does not auto-publish. It ships via a `develop` dispatch (prerelease) or the next promotion to `main`. The tracker's `bump-branch-prefix` + `branches` MUST match a merge-bot rule, one of the built-in `-` head/base pairs or a `rules` entry the caller passes, or auto-merge silently never fires. - **D8.4 An identity allowlist used as a gate fails loud.** Where a gate compares `github.actor` (or a PR author) against hard-coded bot identities, the non-matching branch on an otherwise-legitimate trigger **emits a `::warning::`** rather than falling through silently. Output: a run that declines to act on an unrecognized identity is visibly annotated. *Prevents: the App being renamed, replaced, or reinstalled under a new slug, after which the comparison quietly evaluates false and the gate stops firing, a green and silent run that looks identical to a healthy one.* The masking matters most where a second path hides the loss: a weekly schedule keeps publishing, so the only symptom is release *timeliness*, easily missed for months. Where the failure is self-announcing instead (the merge-bot simply stops merging, so bot PRs visibly pile up) an annotation is optional. Resolving the identity at run time (mint an App token, read `GET /app`) removes the hard-coded string entirely and is the escalation if an allowlist proves fragile in practice. ### D9 - Style / Static (See Section 2) @@ -230,14 +230,14 @@ Read the workflow files plus `version.json` and assert the structural fact behin - **D5:** each cross-job transfer artifact has a delete step at its consumer, gated to the consumer's condition, `continue-on-error: true`, looping all ids; **every** upload sets `retention-days: 1`; **no** `.artifacts[].id` blanket delete exists anywhere. - **D6:** the release download uses `pattern:`/`merge-multiple:` (no `artifact-ids:`). Branch-derived config reads `inputs.branch` (a `github.ref_name` in such config is a finding). Artifact names are branch-suffixed. The target set is consistent across the release task and the paths-filter. - **D7:** the publisher concurrency group is ref-independent with `cancel-in-progress: false`. Reusable jobs declare permissions. Boolean `if:` uses both forms. -- **D8/D9:** merge-bot concurrency keys on PR number. The upstream tracker's branch prefix matches the merge-bot's head-ref pairs (wrapper repos). Actions are SHA-pinned. Names/shells/conditionals follow section 2. +- **D8/D9:** merge-bot concurrency keys on PR number. The upstream tracker's branch prefix matches a merge-bot rule (wrapper repos). Actions are SHA-pinned. Names/shells/conditionals follow section 2. **Per-type addenda (apply only the ones present):** -- **Console/executable:** the smoke runtime matrix is a strict non-empty subset of the full matrix. The per-runtime outputs (`publish--`) are aggregated by `pattern:` + `merge-multiple:` into one `release-asset--` and the aggregation job is gated `!smoke`. The per-runtime intermediates rely on the retention backstop (no explicit delete is required for an in-run intermediate). +- **.NET publish:** the smoke runtime set is a strict non-empty subset of the full runtime set. The selected set runs sequentially inside one composite-action job. A non-smoke run uploads one `release-asset--dotnet-publish` artifact, while a smoke run skips the archive and upload steps. - **NuGet:** the publish step is gated `if: inputs.push` only (not on an existence check) and uses `--skip-duplicate`. `*.nupkg` push also carries the paired `.snupkg` to the symbol server where symbols are enabled. The `release-asset` zip carries the package(s). - **PyPI:** `publish-pypi` declares `environment: { name: pypi }`. `id-token: write` appears only on that job (absent from the build/PR path). `skip-existing: true` is set on the publish action. The build artifact is deleted after publish. The `pypi` environment has a deployment-branch rule. -- **Docker:** a Docker-only repo's caller passes `expect_release_assets: false`. The leaf reads the external state file for the tag instead of `SemVer2` (wrapper repos only, since a plain Docker repo correctly tags off `SemVer2` and records this N/A). The readme/date-badge jobs are gated main-only. The docker-readme task validates `repositories` XOR `manifest`+`manifest-jq`. The buildcache follows D9.4. +- **Docker:** a Docker-only repo's caller passes `expect_release_assets: false`. The leaf reads the external state file for the tag instead of `SemVer2` (wrapper repos only, since a plain Docker repo correctly tags off `SemVer2` and records this N/A). The readme job is gated main-only, both by the caller's branch input and inside the hub-hosted `publish-docker-readme-task.yml` itself. The docker-readme task validates `repositories` XOR `manifest`+`manifest-jq`. The buildcache follows D9.4. - **Static site deployed to a host:** the generator is pinned by version **and** by a checksum verified before install, declared once across the workflows that install it. The deploy is a dispatch carrying an environment choice, with concurrency keyed on the **environment** and `cancel-in-progress: false`, and production gated to the default branch while any ref may reach a non-production environment. The reusable callee re-asserts the environment name in a job of its own. The upload targets a per-release directory and carries no delete flag at the environment root, and the pointer flip is a separate step. The terminal check asserts the golden-list length floors first, then the environment, then the release id, then the URL contract. Retention is bounded by a declared count and one side is recorded as owning the prune: the deploy asserts it where the credential can observe the destination, and the host owns it where the credential is confined write-only (D5.6). ### 5B. End-to-End Trace Scenarios (No Execution, Deterministic from the YAML) @@ -246,12 +246,12 @@ For each *applicable* scenario, evaluate every job's `if:`/`needs:` against the | # | Input | Expected output | Exercises | | --- | --- | --- | --- | -| S1 | PR touching a build target | validation runs; both smoke builds run; no push, **no uploads**; validate-release **skipped (smoke), succeeds**; release **skipped**; aggregator **success**; version = prerelease; no release; no dangling artifacts | D1, D2.2, D3 | -| S2 | PR changing only docs | validation and both smoke builds run (no filter to skip them), aggregator **success** | D1.1, D1.5 | -| S3 | PR changing only `.github/workflows/**` | validation and both smoke builds run against the pushed head, so the change tests its own copy, and the aggregator reports **success** | D1.4 | +| S1 | PR touching a build target | `changes` flags `release`; validation runs; the smoke build runs; no push, **no uploads**; aggregator **success**; no dangling artifacts | D1, D2.2, D3 | +| S2 | PR changing only docs | `changes` still flags `release` (the filter excludes only `.github/workflows/**`), so validation and the smoke build both run, aggregator **success** | D1.1, D1.4, D1.5 | +| S3 | PR changing only `.github/workflows/**` | `changes` reports `release=false`, smoke-build **skipped**, validation still runs, aggregator **success** | D1.4 | | S4 | PR base = default branch, carrying a build target | smoke versions as prerelease, validate-release **skipped (smoke)** so the default-branch arm does **not** fire, aggregator **success**, promotion not blocked | D1.3, D2.2 | -| S5 | bot push to `main` (e.g. an Actions bump) | `publish-release.yml` has no push trigger, so nothing publishes | D4.1 | -| S6 | code-affecting push to `main`, bot or human | still nothing publishes, and the change ships in the next scheduled or dispatched run | D3, D4 | +| S5 | bot push to `main` (e.g. an Actions bump) | `publish-release.yml` has no push trigger, so nothing publishes regardless of what `plan` would have decided | D4.1 | +| S6 | code-affecting push to `main`, bot or human | still nothing publishes (no push trigger exists to fire), and the change ships in the next scheduled or dispatched run | D3, D4 | | S7 | publish run (the weekly `main` schedule, or a dispatch) | builds the **one** trigger branch: `main` -> `X.Y.Z`, `prerelease=false`, registry stable, badge/readme run; `develop` -> `X.Y.Z-g`, `prerelease=true`, registry prerelease; `release-asset-*` consumed-then-deleted; PyPI build-artifact deleted after its publish; **no dangling artifacts** | D3, D4, D5, D6, D7 | | S8 | dispatch from a ref other than `main` or `develop` | **fails fast** | D2.3 | | S9 | re-run publish, version unchanged | release-create **skipped**, `release-asset-*` delete **skipped**; NuGet/PyPI pushes no-op (server dedupe); **PyPI build-artifact still deleted** (its publish ran); **Docker still re-pushes** the image; no duplicate release | D4.4, D5.2 | @@ -281,18 +281,14 @@ The workflow is **operational** iff every *applicable* 5A item passes and every Each type maps the *applicable* S-scenarios onto its targets. The differences are which leaf tasks exist and what each produces, which 5A addenda apply, and which scenarios are N/A. Walking these is the self-check that the contract holds for each shape. -- **Console / executable application.** Target produces `release-asset--executable` (a 7z archive, `Console.7z`) by building a per-runtime `dotnet publish` matrix, then an aggregation job downloads the per-runtime `publish--` intermediates (`pattern:` + `merge-multiple:`), zips them, and uploads the single asset. Smoke builds a strict subset of runtimes. The per-runtime upload **and** the aggregation job are both gated `!smoke`, so smoke uploads nothing. The per-runtime intermediates rely on `retention-days: 1` (no explicit delete). Test: S1 with a console change smoke-builds the subset and uploads nothing; S7 attaches the 7z, `prerelease=true` on the non-default leg and `prerelease=false` on the default leg (GitHub auto-marks the stable default release "Latest", and the workflow does not set it). -- **NuGet library.** The leaf both pushes (`dotnet nuget push *.nupkg --skip-duplicate`, gated `if: push` only) and uploads `release-asset--nugetlibrary`. Configuration is Release on the default branch, Debug otherwise. Where symbols are enabled (`snupkg`), the push auto-carries the paired `.snupkg` to NuGet.org's symbol server and the asset zip also contains it, a triple surface. NuGet.org derives `isPrerelease` from the SemVer2 `-g` suffix (the workflow sets no such flag). Test: S7 non-default leg publishes a prerelease package + asset, default a stable; S9 re-run is a server-side `--skip-duplicate` no-op. 5C: query NuGet.org for both versions and the symbol package. -- **PyPI library.** The leaf builds + uploads `pypilibrary-build-`. A **separate** `publish-pypi` job (with `environment: pypi`, `id-token: write`, `actions: write`) does the OIDC Trusted-Publishing upload with `skip-existing: true`, then **consume-then-deletes** the build artifact, **unconditionally on consume**, so on S9 it is deleted even though the `release-asset-*` delete is skipped. The version is `AssemblyFileVersion` with `.dev0` appended on `develop` only, and must stay `--pre`-selectable and sorted above the default release. PyPI contributes no `release-asset-*`. A PyPI-only repo sets `expect_release_assets: false` at the caller. Test: S7 default leg publishes a release, non-default a `.dev0`; S9 is a `skip-existing` no-op; 5C inspects the `dist/*` filenames and the compute-version log. -- **Docker image.** The leaf pushes the default branch multi-arch (amd64+arm64) and any other branch `amd64`-only, with a per-branch registry buildcache (`buildcache-`; a multi-image repo adds a per-image tag) (`cache-to` only the built branch and only on push, `cache-from` both branches); no `release-asset-*`, so a Docker-only repo's caller passes `expect_release_assets: false`; the readme (`peter-evans/dockerhub-description`, `DOCKER_HUB_ACCESS_TOKEN`) and date-badge jobs run **only** when the default branch publishes; the docker-readme task validates `repositories` XOR `manifest`+`manifest-jq` and a multi-image repo derives its publish matrix from the manifest. Docker **always re-pushes** the image, independently of a skipped release-create (S9). A **wrapper** repo tracks an upstream release: the upstream tracker writes a `name -> version` state file and the merge-bot auto-merges the bump PR (S11), and the leaf MUST read that file for the immutable tag instead of `SemVer2` (the tracker ships without this consumer wiring). Test: S7 default leg pushes `latest` + the version tag and updates readme/badge. Non-default pushes the develop tag (amd64 only). S9 still re-pushes. S11 ships the bumped upstream version next publish. 5C Docker probe needs `DOCKER_HUB_*` secrets and same-repo (not fork) runs. -- **Data / asset library.** A single new leaf: validate -> zip -> upload `release-asset--library` (`retention-days: 1`, upload gated `!smoke`, mirroring the nugetlibrary leaf's shape). Because no such leaf ships, you **add a target** (D6.4): a new `enable_library` input + `build-library` job + `github-release` `needs:` entry in the release task, and a `library` paths-filter entry + `changes` output + `smoke-build` enable-forward in the PR workflow (without it, D1.1 never smoke-builds the library). Keep `expect_release_assets: true` (it has a file target, unlike Docker). The .NET `unit-test` job is replaced by a type-appropriate validator with the aggregator **and** `smoke-build` both re-pointed to it (D1.2/D1.5). `version.json` + the NBGV `get-version` step are retained (they own the tag). Test: S1 smoke runs validate+zip and uploads nothing; S7 attaches the zip, prerelease on the non-default leg; S9 on a *scheduled* re-run release-create + asset-delete skip (the existing zip is untouched, no registry push), while a `workflow_dispatch` re-run **refreshes** the release and re-runs the asset-delete (the asset is re-uploaded then re-deleted). N/A: the nuget/pypi/docker/executable 5A addenda and their scenario clauses. -- **Source-only / no build.** There is no `build-release-task.yml` (its `appliesTo` excludes source-only) and no package/image leaf, so nothing is edited down. The release is a standalone dispatch-only `publish-release.yml` that inlines NBGV for the tag and `action-gh-release` for the release: tag + source zip + README + LICENSE, with no reusable release task and no asset download. With no target the paths-filter matches nothing, so `smoke-build` is **structurally always skipped**, and validation is carried solely by the (replaced, non-.NET) validation job that the aggregator and `smoke-build`'s own `needs:` must both point at (D1.2, or drop the never-running `smoke-build` job). NBGV and `version.json` are still retained (they own the tag). Its publish job gates on the repo's reusable validation task (`needs:` the same `workflow_call` job the PR workflow runs), so a dispatch cannot release a ref that fails validation. Applicable scenarios: S1 (validation only), S5/S6 (publish gating), S7 (tag-only release), S8 (dispatch guard), S9 (no-op republish), S10 (classification gate). N/A: S2-S4 (assume a smoke-built target), the artifact-lifecycle and registry clauses of S7/S9, the D5/D6 artifact items, and all per-type 5A addenda, all recorded N/A, not failed. -- **Static site deployed to a host the project owns.** Two independent surfaces, and keeping them apart is the point. The **release** is the source-only shape above, unchanged: a dispatch-only `publish-release.yml` where NBGV and `version.json` own the tag, producing tag + source zip + README + LICENSE. The **deploy** is its own `workflow_dispatch` carrying an `environment` choice input, so redeploying an unchanged commit mints no tag, which matters because redeploying is routine. It runs a ref gate **first**, before anything is installed or written (production from the default branch only, while any ref may reach a non-production environment, since proving a branch before it merges is what that environment is for), then the **same** reusable validation task the PR gate runs, so a dispatch cannot deploy a ref that fails validation, then calls `deploy-site-task.yml` with `secrets: inherit`. Concurrency is keyed on the environment with `cancel-in-progress: false`, because a cancelled deploy leaves a release uploaded and unflipped. The task pins the generator by version and checksum, and re-asserts the environment name in a job of its own, because the `environment:` binding resolves before any step runs and a `workflow_call` caller is not bound by the dispatch choice list a human sees. Its environment-bound job then: checks out full history (a shallow clone silently changes page metadata), installs the verified generator, derives the release id **once** and exports it (deriving it twice yields ids seconds apart, and the live check then asserts a version nothing installed), builds the tree, installs the deploy credential from the environment, uploads into a per-release directory hard-linked against the current release and carrying **no** delete flag (at an environment root a delete removes the rollback targets), flips the pointer as a separate atomic step so a failed transfer cannot half-publish, and finally checks the running host (D4.6). Retention (D5.6) is bounded by a declared count with one side recorded as owning it: a deploy whose credential can observe the destination prunes and asserts the count here, while a credential confined **write-only** can neither delete nor read back, so there the prune is a host-side timer and the repo's runbook records that ownership. Widening the credential to bring the prune in-pipeline would trade a real confinement boundary for a check, and is the wrong trade. What the guarantee rejects is neither side owning it. One thing the pipeline cannot assert and the server config must: a non-public environment serving a byte-identical copy must not be indexed, and that default belongs on the side that is harmless in production, since a non-public container missing the value is still behind its gate while a production container inheriting it deindexes the site silently. Applicable scenarios: S1 (validation), the source-only release set S7/S8/S9/S10, and S12/S13 (the deploy dispatch). N/A: S2-S4, every registry scenario, and D5.1-D5.4 (the pipeline uploads no workflow artifact at all, so D5.6 is what applies in their place), all recorded N/A, not failed. -- **Operational (workflow model, not a build target).** A `workflowModel: operational` repo layers a direct-commit `develop` onto the **source-only** release shape (above). Two workflows: (1) a **lint/validation** PR workflow feeding the required `Check pull request workflow status job`, built from the generic linters (editorconfig/EOL, markdownlint, cspell, actionlint) plus a domain validator (Home Assistant `hass --script check_config`, `esphome config`, a firmware build), with **no unit tests**; its triggers differ from the `release` model: `push` to `develop` (advisory feedback on the direct-commit path) plus `pull_request` to `branches: [ main, develop ]` (enforced on `main` as the promotion gate, reported but not required on `develop`) plus `workflow_dispatch`. (2) the standard **source-only publisher** on `workflow_dispatch` only (`releaseTrigger: dispatch-only`): NBGV + `version.json` own the tag, and a manual dispatch cuts a GitHub release (tag + source zip + README + LICENSE, via the standalone publisher's inlined `action-gh-release`). **The PR trigger names both branches, and naming `main` alone is a defect.** A set that omits `develop` matches nothing when a PR opens against `develop`, so the validation job never starts, the aggregator never reports, and the PR shows a clean mergeable state with an empty check list, which is the exact output D1.2 forbids. Naming both costs a duplicate run, since a PR-merged change validates once on the PR and again on the push when the merge lands, whatever merge method the repo allows (the operational `develop` ruleset prescribes none). The concurrency group is keyed on the workflow name plus `${{ github.ref }}` (Section 2), and a `pull_request` run's ref is `refs/pull//merge` where the push run's is `refs/heads/develop`, so the two occupy different groups and neither cancels the other. Pay it. On a lint-only gate that is a couple of runner-minutes, where the alternative, a condition that suppresses the push run, has to tell a merge commit from a direct commit and reintroduces exactly the reasoning the trigger set removes. Applicable scenarios: S1 (validation) on every PR, the promotion PR and a PR into `develop` alike, plus the source-only release set: S7 (tag-only release), S8 (dispatch guard), S9 (no-op republish), S10 (classification). N/A: the auto-publish paths (S5/S6 bot-push and schedule, neither of which an operational repo has) and every build/registry scenario. See the branch-model note in Section 3 and [GOVERNANCE.md "Branching Model"][governance-branching-model]. - - - -[workflows]: ./.github/workflows/ +- **.NET publish.** The target runs a sequential `dotnet publish` runtime loop inside one composite-action job. Configuration is Release on `main` and Debug otherwise. A non-smoke run builds the full runtime set, zips the combined output, and uploads `release-asset--dotnet-publish`. The archive is named from the project file stem unless `dotnet_publish_asset_name` overrides it. A smoke run builds a two-runtime subset and skips the zip and upload steps, so it uploads nothing. S1 smoke-builds that subset after a .NET project change. S7 attaches the 7z from a non-smoke run. The non-default leg sets `prerelease=true`, and the default leg sets `prerelease=false`. GitHub marks the stable default release "Latest" automatically. +- **NuGet.** The leaf both pushes (`dotnet nuget push *.nupkg --skip-duplicate`, gated `if: push` only) and uploads `release-asset--nuget`. Configuration is Release on the default branch, Debug otherwise. Where symbols are enabled (`snupkg`), the push auto-carries the paired `.snupkg` to NuGet.org's symbol server and the asset zip also contains it, a triple surface. NuGet.org derives `isPrerelease` from the SemVer2 `-g` suffix (the workflow sets no such flag). Test: S7 non-default leg publishes a prerelease package + asset, default a stable; S9 re-run is a server-side `--skip-duplicate` no-op. 5C: query NuGet.org for both versions and the symbol package. +- **PyPI.** The leaf builds and uploads `pypi-build-`. A **separate** `publish-pypi` job (with `environment: pypi`, `id-token: write`, `actions: write`) does the OIDC Trusted-Publishing upload with `skip-existing: true`, then **consume-then-deletes** the build artifact, **unconditionally on consume**, so on S9 it is deleted even though the `release-asset-*` delete is skipped. The version is `AssemblyFileVersion` with `.dev0` appended on `develop` only, and must stay `--pre`-selectable and sorted above the default release. PyPI contributes no `release-asset-*`. A PyPI-only repo sets `expect_release_assets: false` at the caller. Test: S7 default leg publishes a release, non-default a `.dev0`; S9 is a `skip-existing` no-op; 5C inspects the `dist/*` filenames and the compute-version log. +- **Docker image.** The leaf pushes the default branch multi-arch (amd64+arm64) and any other branch `amd64`-only, with a per-branch registry buildcache (`buildcache-`; a multi-image repo adds a per-image tag) (`cache-to` only the built branch and only on push, `cache-from` both branches); no `release-asset-*`, so a Docker-only repo's caller passes `expect_release_assets: false`; the readme job (`peter-evans/dockerhub-description`, `DOCKER_HUB_ACCESS_TOKEN`) runs **only** when the default branch publishes, whether called directly or reached through the hub-hosted `publish-docker-readme-task.yml`; the docker-readme task validates `repositories` XOR `manifest`+`manifest-jq` and a multi-image repo derives its publish matrix from the manifest. Docker **always re-pushes** the image, independently of a skipped release-create (S9). A **wrapper** repo tracks an upstream release: the upstream tracker writes a `name -> version` state file and the merge-bot auto-merges the bump PR (S11), and the leaf MUST read that file for the immutable tag instead of `SemVer2` (the tracker ships without this consumer wiring). Test: S7 default leg pushes `latest` + the version tag and updates the readme. Non-default pushes the develop tag (amd64 only). S9 still re-pushes. S11 ships the bumped upstream version next publish. 5C Docker probe needs `DOCKER_HUB_*` secrets and same-repo (not fork) runs. +- **Data / asset library.** A single new leaf: validate -> zip -> upload `release-asset--library` (`retention-days: 1`, upload gated `!smoke`, mirroring the NuGet leaf's shape). Because no such leaf ships, you **add a target** (D6.4): a new `enable_library` input + `build-library` job + `github-release` `needs:` entry in the release task, and a `library` paths-filter entry + `changes` output + `smoke-build` enable-forward in the PR workflow (without it, D1.1 never smoke-builds the library). Keep `expect_release_assets: true` (it has a file target, unlike Docker). The .NET `unit-test` job is replaced by a type-appropriate validator with the aggregator **and** `smoke-build` both re-pointed to it (D1.2/D1.5). `version.json` + the NBGV `get-version` step are retained (they own the tag). Test: S1 smoke runs validate+zip and uploads nothing; S7 attaches the zip, prerelease on the non-default leg; S9 on a *scheduled* re-run release-create + asset-delete skip (the existing zip is untouched, no registry push), while a `workflow_dispatch` re-run **refreshes** the release and re-runs the asset-delete (the asset is re-uploaded then re-deleted). N/A: the NuGet, PyPI, Docker, and .NET publish 5A addenda and their scenario clauses. +- **Source-only / no build.** There is no package/image build leaf. A repo may own the reusable release task or call its hub-hosted copy. The dispatch-only `publish-release.yml` reaches the reusable plan, validation, and release tasks. Its publish job passes `github: true`, every `enable_*` input as false, and `expect_release_assets: false`. This produces tag + source zip + README + LICENSE with no asset download. With no target, the paths-filter matches nothing. A retained `smoke-build` job is therefore **structurally always skipped**. The repo may instead drop that never-running job. Validation remains the replaced, non-.NET validation job. The aggregator and any retained `smoke-build` job must depend on it (D1.2). NBGV and `version.json` own the tag. The publish job depends on the same reusable validation task that the PR workflow runs. This prevents a dispatch from releasing a ref that fails validation. Applicable scenarios are S1 (validation only), S7, S8, S9, and S10. S7 covers the tag-only release, S8 the dispatch guard, S9 no-op republish, and S10 the classification gate. S2-S6, D5/D6 artifact items, and all per-type 5A addenda are N/A. The artifact-lifecycle and registry clauses of S7/S9 are also N/A, not failed. +- **Static site deployed to a host the project owns.** Two independent surfaces, and keeping them apart is the point. The **release** is the source-only shape above, unchanged: a dispatch-only `publish-release.yml` where NBGV and `version.json` own the tag, producing tag + source zip + README + LICENSE. The **deploy** is its own `workflow_dispatch` carrying an `environment` choice input, so redeploying an unchanged commit mints no tag, which matters because redeploying is routine. It runs a ref gate **first**, before anything is installed or written (production from the default branch only, while any ref may reach a non-production environment, since proving a branch before it merges is what that environment is for), then the **same** reusable validation task the PR gate runs, so a dispatch cannot deploy a ref that fails validation, then calls the hub-hosted `deploy-site-task.yml`, binding the same `environment:` on the caller's own job so the one crossing secret, `DEPLOY_SSH_PRIVATE_KEY`, resolves from the GitHub Environment store and can be mapped explicitly rather than through `secrets: inherit`, which a cross-repository reusable workflow cannot use. Concurrency is keyed on the environment with `cancel-in-progress: false`, because a cancelled deploy leaves a release uploaded and unflipped. The task re-asserts the environment name in a job of its own, because the `environment:` binding resolves before any step runs and a `workflow_call` caller is not bound by the dispatch choice list a human sees. Its environment-bound job then: checks out full history (a shallow clone silently changes page metadata), derives the release id **once** and exports it (deriving it twice yields ids seconds apart, and the live check then asserts a version nothing installed), runs a required deploy hook that builds the tree with whatever generator and precompression the site owns, installs the deploy credential from the environment, uploads into a per-release directory hard-linked against the current release and carrying **no** delete flag (at an environment root a delete removes the rollback targets), flips the pointer as a separate atomic step so a failed transfer cannot half-publish, then runs the same hook again to prune old releases and to check the running host (D4.6). Retention (D5.6) is bounded by a declared count with one side recorded as owning it: a deploy whose credential can observe the destination prunes and asserts the count here, while a credential confined **write-only** can neither delete nor read back, so there the prune is a host-side timer and the repo's runbook records that ownership. Widening the credential to bring the prune in-pipeline would trade a real confinement boundary for a check, and is the wrong trade. What the guarantee rejects is neither side owning it. One thing the pipeline cannot assert and the server config must: a non-public environment serving a byte-identical copy must not be indexed, and that default belongs on the side that is harmless in production, since a non-public container missing the value is still behind its gate while a production container inheriting it deindexes the site silently. Applicable scenarios: S1 (validation), the source-only release set S7/S8/S9/S10, and S12/S13 (the deploy dispatch). N/A: S2-S4, every registry scenario, and D5.1-D5.4 (the pipeline uploads no workflow artifact at all, so D5.6 is what applies in their place), all recorded N/A, not failed. +- **Operational (workflow model, not a build target).** A `workflowModel: operational` repo layers direct commits to `develop` onto the **source-only** release shape above. It has two workflows. The first is a **lint/validation** PR workflow that feeds the required `Check pull request workflow status job`. It uses the generic linters (editorconfig/EOL, markdownlint, cspell, actionlint) plus a domain validator, with **no unit tests**. Examples include Home Assistant `hass --script check_config`, `esphome config`, or a firmware build. Its triggers differ from the `release` model. It runs on pushes to `develop`, pull requests to `[ main, develop ]`, and `workflow_dispatch`. Push validation is advisory. Pull request validation is enforced on `main` and reported but not required on `develop`. The second workflow is the standard **source-only publisher** with `releaseTrigger: dispatch-only`. NBGV and `version.json` own the tag. The reusable release task creates tag + source zip + README + LICENSE. **The PR trigger names both branches, and naming `main` alone is a defect.** Omitting `develop` starts no validation when a PR opens against `develop`. The aggregator then never reports, and the PR appears clean with an empty check list. D1.2 forbids that output. Naming both causes a duplicate run after a PR merge. The change validates on the PR and again on the resulting push, regardless of merge method. The operational `develop` ruleset prescribes no merge method. The concurrency group uses the workflow name plus `${{ github.ref }}` (Section 2). A pull request uses `refs/pull//merge`, while its push uses `refs/heads/develop`. The runs occupy different groups and neither cancels the other. Pay that cost. The lint-only gate costs only a few runner-minutes. Suppressing the push requires distinguishing a merge commit from a direct commit, which restores the ambiguity the trigger set removes. S1 applies to every PR, including promotion and `develop` PRs. The source-only S7, S8, S9, and S10 scenarios also apply. Bot-push and schedule paths in S5/S6 are N/A, as are every build and registry scenario. See the branch-model note in Section 3 and [GOVERNANCE.md "Branching Model"][governance-branching-model]. diff --git a/cspell.json b/cspell.json index ce740aa..bde1a83 100644 --- a/cspell.json +++ b/cspell.json @@ -121,8 +121,8 @@ "nostdin", "NTFS", "nuget", - "nugetlibrary", "onCreateCommand", + "opencode", "opencontainers", "Optix", "othercommand", @@ -139,11 +139,11 @@ "pydantic", "pypa", "pypi", - "pypilibrary", "pyproject", "pyright", "pyrightconfig", "pytest", + "Qodo", "quicktime", "quoteoftheday", "reencode", From 42d5e814397cb4dfe67007619d56d2970fc95c76 Mon Sep 17 00:00:00 2001 From: Pieter Viljoen Date: Fri, 28 Aug 2026 20:56:45 -0700 Subject: [PATCH 2/2] Fix more ARCHITECTURE.md/AGENTS.md content bugs review caught - Metadata Preservation was flatly wrong: ProcessTask does call a separate SetCreateDateAsync after CopyMetadataAsync/TagsFromFile (confirmed at ProcessTask.cs ~line 610), contra the doc's 'no separate date-setting step is needed' claim. Also clarified the metadata source under --skipbackup (the still-live original, deleted only after the copy runs, not the .bak file the old text implied universally). - Command Line Testing Patterns described a test suite that does not exist: CommandLineTests.cs constructs CommandLine directly (no CreateTestCommand() helper), and --path is a single Option (confirmed at CommandLine.cs), not the Option> multi-path suite the old text described in detail. Rewrote to match the actual test file, and dropped the same stale 'multiple paths' claim from the Coverage Areas line above it. - AGENTS.md's CODESTYLE.md/ARCHITECTURE.md ownership statement read as contradictory once architecture content moved out of copilot-instructions.md: 'this project's own conventions... live in CODESTYLE.md' read as claiming code-style AND design conventions, while ARCHITECTURE.md's own 'Key Patterns & Conventions' section covers the latter. Narrowed the CODESTYLE.md sentence to code-style specifically and added an explicit ARCHITECTURE.md pointer for design conventions. --- AGENTS.md | 2 +- ARCHITECTURE.md | 14 +++++--------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4e98785..5d86c2a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,7 +4,7 @@ This file is the entry point every coding agent reads first, and it holds only two things: the rules for managing context and delegation, which apply to every task, and a map of where every other rule lives. The rule text itself is in [`GOVERNANCE.md`](./GOVERNANCE.md), one section per topic. Code style lives in [`CODESTYLE.md`](./CODESTYLE.md), the CI/CD workflow contract in [`WORKFLOW.md`](./WORKFLOW.md), and this project's architecture, processing pipeline, per-command details, and test layout in [`ARCHITECTURE.md`](./ARCHITECTURE.md). -Treat this file and `GOVERNANCE.md` as authoritative for cross-cutting rules, and do not restate their rules elsewhere. This project's own conventions and behavioral contracts live in [`CODESTYLE.md`](./CODESTYLE.md), **not** in [`.github/copilot-instructions.md`](./.github/copilot-instructions.md), because that file targets GitHub Copilot / VS Code specifically, while this file and `GOVERNANCE.md` are the agent-agnostic ones every coding agent is directed to read, so any rule a reviewer must honor has to live in one of those two files to be provider-independent. +Treat this file and `GOVERNANCE.md` as authoritative for cross-cutting rules, and do not restate their rules elsewhere. This project's own **code-style** conventions, the formatting and language rules a reviewer enforces, live in [`CODESTYLE.md`](./CODESTYLE.md), **not** in [`.github/copilot-instructions.md`](./.github/copilot-instructions.md), because that file targets GitHub Copilot / VS Code specifically, while this file and `GOVERNANCE.md` are the agent-agnostic ones every coding agent is directed to read, so any rule a reviewer must honor has to live in one of those two files to be provider-independent. This project's **design** conventions, its architecture, processing pipeline, and implementation patterns, are `ARCHITECTURE.md`'s own subject and are read there rather than restated here. ## Fleet Bootstrap diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 88d9a51..8b5e060 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -159,7 +159,7 @@ See [`CODESTYLE.md`](./CODESTYLE.md) for build requirements, formatting commands - **PhotoCleanerTests Project**: comprehensive test coverage across all commands, see the file list under Project Structure above rather than a count here, which drifts every time a test is added - **InternalsVisibleTo**: Enables direct testing of internal methods without reflection -- **Coverage Areas**: Date inference (filename patterns, path structures, validation), command line interface (parsing, validation, error handling, multiple paths, thread configuration and boundary validation), integration scenarios, process task execution, live photo detection (ContentIdentifier matching, `_hevc` suffix naming, mismatch/missing tag scenarios), metadata preservation through conversion +- **Coverage Areas**: Date inference (filename patterns, path structures, validation), command line interface (parsing, validation, error handling, thread configuration and boundary validation), integration scenarios, process task execution, live photo detection (ContentIdentifier matching, `_hevc` suffix naming, mismatch/missing tag scenarios), metadata preservation through conversion ## Critical Implementation Details @@ -167,7 +167,7 @@ See [`CODESTYLE.md`](./CODESTYLE.md) for build requirements, formatting commands - **Three-tier approach**: Remux (.m2t, .mkv) -> Re-encode (.asf, .wmv, .avi, .3gp, .gif) -> Audio-only re-encode (.mov/.mp4 with PCM audio) - **Backup Strategy**: Original files renamed to `.bak` extension after successful conversion, and `BackupFile()` returns the backup path. A `{backup}.out` companion file (e.g. `img.gif.bak.out`) is written alongside the backup containing the full output path, this is needed when `GetUniqueFileName` appended a counter suffix (e.g. `img_1.mp4`) because the canonical name was already taken. When `options.SkipBackup` is true, no `.bak` or `.bak.out` files are created, and the original is deleted after conversion. -- **Metadata Preservation**: After every ffmpeg conversion, `exiftool -TagsFromFile -all:all -overwrite_original` copies all source metadata to the output file. `ffmpeg -map_metadata` is not used, it is unreliable for Apple QuickTime-specific tags (e.g. `ContentIdentifier` in the `mdta`/`keys` atom). `TagsFromFile` handles cross-format date mapping, so no separate date-setting step is needed after conversion. +- **Metadata Preservation**: After every ffmpeg conversion, `exiftool -TagsFromFile -all:all -overwrite_original` copies all source metadata to the output file. The source is the `.bak` file in the normal case, or the still-live original file itself when `--skipbackup` is set (the original is deleted only after this copy runs). `ffmpeg -map_metadata` is not used, it is unreliable for Apple QuickTime-specific tags (e.g. `ContentIdentifier` in the `mdta`/`keys` atom). `TagsFromFile` does not reliably set the output's create date across every format pairing, so a separate `SetCreateDateAsync` call follows it, reading the date from the already-parsed `ExifToolJson` and writing it to the output explicitly. - **Re-queue Pattern**: Converted files are added back to processing queue for validation ### Live Photo Detection @@ -222,10 +222,6 @@ Uses `ExifToolJsonContext` for AOT-compatible JSON serialization of `ExifToolJso ### Command Line Testing Patterns -- **CreateTestCommand() Helper**: Uses `CommandLine.CreateRootCommand()` directly for single source of truth -- **Type-based Option Extraction**: Identifies options by type (`Option>`, `Option`, `Option`) using 4-tuple destructuring -- **Real Directory Testing**: Uses `Directory.GetCurrentDirectory()` for path validation tests -- **Parse Result Validation**: Tests both success/error states and extracted argument values, including list counts for multiple paths and thread values -- **Comprehensive Scenarios**: Single path, multiple paths, thread configuration, option properties, argument parsing, validation errors, edge cases, default values -- **Multiple Path Testing**: Validates 2-path and 3-path scenarios, mixed valid/invalid paths, and proper list indexing -- **Thread Option Testing**: Validates thread count parsing, default value calculation, short option, and combined option scenarios +- **Direct construction, no helper**: `new CommandLine([args])` parses directly, and the test reads `cli.Result.Errors` / `cli.Result.GetValue(...)` (or the equivalent per-subcommand options) rather than going through a shared builder. +- **Real Directory Testing**: Uses `Directory.GetCurrentDirectory()` as a path that is guaranteed to exist, for both success cases and `AcceptExistingOnly()` validator cases. +- **Comprehensive Scenarios**: Per-option default values, explicit values, flag parsing, validation errors (an unknown option, a missing required option, an out-of-range numeric value), and subcommand registration.