diff --git a/CLAUDE.md b/CLAUDE.md index 6753f7b..67f82a0 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -160,7 +160,11 @@ Presentation/ > `Web/Components/Pages/` — `Home · Search · Activity · Settings · Mappings · Login · Logout · Setup`. Each host is an **independently deployable microservice** from day one. **Adding a broadcaster** = -a new `Application/` slice + an Infrastructure HTTP client + a `Presentation/Agents/` host. +an Infrastructure HTTP client + an `IBroadcasterCrawler` adapter + a `Presentation/Agents/` +host — **no new Application slice**: `Application/Crawling` is shared and selects a crawler by provider +key. The four registrations that are easy to miss (slnx · AppHost · `Build.Publish` Services · +the Newznab host's on-demand block, without which search never reaches the new broadcaster) are in +[`docs/adding-a-broadcaster.md`](docs/adding-a-broadcaster.md). ### Enforced @@ -193,12 +197,15 @@ dotnet fallout Test # same thing via the tool, which is what CI invokes `develop` is the **default branch and integration trunk**; `main` is production and is the only branch tagged `v*`. Work goes `feat|fix|chore|docs/*` → PR into `develop`. A release is `develop` (or a -`release/*` window) rebase-merged into `main`, then tagged. `hotfix/*` is cut from `main` and **must** +`release/*` window) **fast-forwarded** into `main`, then tagged. `hotfix/*` is cut from `main` and **must** be ported back to `develop`. - Every non-docs push to `develop` republishes the images as `:edge` (`PushEdge`). - `GitHubRelease` **refuses** a tag that is not reachable from `main` or `support/*` — the trunk is never tagged for release. +- **Never merge a release PR with GitHub's button.** It rewrites the commits, which severs the + commit→PR link the generated release notes are built from (v0.3.0 lost two entries that way). + Advance `main` with `git merge --ff-only develop && git push origin main`. - Full model in [`docs/branching-and-release.md`](docs/branching-and-release.md), pipeline in [`docs/ci.md`](docs/ci.md), runbooks in [`docs/releasing.md`](docs/releasing.md). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000..c21993a --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,87 @@ +# Contributing + +Krautwatch is a self-hosted Newznab indexer and SABnzbd download client for German public +broadcasters. Contributions are welcome — especially **new broadcasters**, which have a documented +shape of their own: [docs/adding-a-broadcaster.md](docs/adding-a-broadcaster.md). + +## Getting set up + +```bash +dotnet tool restore # Fallout + the Aspire CLI are pinned local tools +./build.sh Test # restore + compile + unit/architecture tests +dotnet run --project src/Presentation/AppHost # the whole fleet, via Aspire +``` + +You need **.NET 10**, **Docker running** (the repository tests use a real Postgres container via +Testcontainers) and **ffmpeg on PATH** if you're working on downloads. + +`./build.sh TestLive` runs the tests that hit ARD and ZDF for real. They're excluded from the CI +gate because external APIs drift and rate-limit — run them yourself when you touch a crawler. + +## Where work lands + +The repo runs **GitFlow**: `develop` is the default branch and the integration trunk, `main` is what +is released. + +```bash +git switch develop && git pull --ff-only +git switch -c feat/my-change +./build.sh Test +gh pr create --base develop --label enhancement +``` + +The full model — where a fix belongs, how releases are cut, what CI does at each step — is in +[docs/branching-and-release.md](docs/branching-and-release.md), [docs/ci.md](docs/ci.md) and +[docs/releasing.md](docs/releasing.md). + +## Pull requests + +**A PR title is a changelog line.** It appears verbatim in the release notes, months later, out of +context — so write an imperative sentence: "Serve the SABnzbd surface on /api", not +`fix(api): sab endpoint`. No `feat(scope):` prefixes, no bare issue numbers. Full guidance in +[docs/agents/issue-and-pr-style.md](docs/agents/issue-and-pr-style.md). + +**Label the PR when you create it**, in the same `gh pr create --label …` call — the labels *are* +the changelog. One category from [`.github/release.yml`](.github/release.yml): `enhancement`, `bug`, +`breaking-change`, `security`, `documentation`, `dependencies`, or `skip-changelog` for +housekeeping. + +Note that `dependencies` and `skip-changelog` are both excluded from the notes, so a PR carrying +`security` **and** `dependencies` vanishes entirely — a CVE-fixing bump gets `security` alone. + +## What the reviewer will check + +- **`./build.sh Test` is green**, architecture tests included. Four ArchUnitNET rules enforce the + hexagon: Domain depends on nothing, Application only on Domain, Infrastructure never on + Presentation, and no slice depends on a sibling slice. +- **The layering is respected.** Ports live in `Domain/Interfaces`; adapters in `Infrastructure`; + use-cases as vertical slices in `Application` with the CQRS/A split marked by banner comments + inside each file. [`CLAUDE.md`](CLAUDE.md) is the working map of the layout, and + `docs/architecture/` holds the decision records — **DR-009, DR-010 and DR-011 are current**; read + them before a structural change. +- **Generated files aren't hand-edited.** `.github/workflows/*.yml` comes from the + `[GitHubActions]` attributes in `build/Build.CI.GitHubActions.cs`, and the compose file comes from + the Aspire AppHost. Editing either by hand is silently undone. +- **Anything user-visible is documented** in the README, and anything structural gets a plan in + `docs/plans/` first — the convention is `YYYY-MM-DD - .md`, written before implementation. + +## Adding a broadcaster + +The catalog is built entirely by per-broadcaster crawlers behind one port, `IBroadcasterCrawler`, so +adding ORF, SRF or arte is a bounded unit of work: an HTTP client, an adapter, an agent host, and +four registrations. The walkthrough, with the traps called out, is in +**[docs/adding-a-broadcaster.md](docs/adding-a-broadcaster.md)**. + +## Reporting issues + +Bugs and feature requests both belong in [GitHub issues](https://github.com/Chrison-dev/Krautwatch/issues). +For a crawl or download failure, the useful details are: the broadcaster, the show, what Sonarr +asked for, and the agent's log lines around the failure. For a geo-restricted asset, say whether +`Download:ProxyUrl` was configured — that's the difference between a bug and the documented +fail-fast. + +## Legal + +Krautwatch downloads freely available content from German public broadcasters' own official APIs for +personal, offline use, and circumvents no DRM. Contributions that add DRM circumvention, scrape +paywalled or commercial catalogs, or bypass a broadcaster's access controls will not be merged. diff --git a/Directory.Packages.props b/Directory.Packages.props index b7c67ad..f7b7421 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -60,9 +60,12 @@ </ItemGroup> <ItemGroup Label="TheTVDB — first-party client (Chrison-dev/TvdbApi), used rather than hand-rolled"> - <PackageVersion Include="TvdbClient" Version="4.7.12" /> - <PackageVersion Include="TvdbClient.Abstractions" Version="4.7.12" /> - <PackageVersion Include="TvdbClient.Models" Version="4.7.12" /> + <!-- 4.7.13 moves its Scrutor.Extensions.HttpClient dependency to 7.0.1, whose + Microsoft.Extensions.Http range is [9.0.0, 11.0.0). 4.7.12 carried the 5.0.1 range that + stopped below 10.0.0, which is what made every project here report NU1608. --> + <PackageVersion Include="TvdbClient" Version="4.7.13" /> + <PackageVersion Include="TvdbClient.Abstractions" Version="4.7.13" /> + <PackageVersion Include="TvdbClient.Models" Version="4.7.13" /> </ItemGroup> <ItemGroup Label="Compression"> @@ -78,7 +81,12 @@ <ItemGroup Label="Testing"> <PackageVersion Include="Microsoft.NET.Test.Sdk" Version="18.8.1" /> - <PackageVersion Include="Testcontainers.PostgreSql" Version="4.13.0" /> + <!-- 4.14.0 is the first release off SSH.NET 2026.0.0, which patches GHSA-q939-rpr3-3284 + (high: recursive ScpClient download writes arbitrary files from server-controlled + names). SSH.NET arrives here only as a Testcontainers transitive, so bumping the + parent is the fix — pinning the transitive would leave us guessing what Testcontainers + tested against. --> + <PackageVersion Include="Testcontainers.PostgreSql" Version="4.14.0" /> <PackageVersion Include="xunit.v3" Version="3.2.2" /> <PackageVersion Include="xunit.runner.visualstudio" Version="3.1.5" /> <PackageVersion Include="Shouldly" Version="4.3.0" /> diff --git a/README.md b/README.md index 0387514..b611d0e 100644 --- a/README.md +++ b/README.md @@ -364,8 +364,11 @@ Domain ← Application ← Infrastructure Wolverine is the mediator + bus + transactional outbox (**Postgres transport** by default — durable, no extra container; RabbitMQ opt-in for scale-out). -Each host is an independently deployable microservice. **Adding a broadcaster** = a new Application -slice + an Infrastructure HTTP client + a `Presentation/Agents/<Broadcaster>` host. +Each host is an independently deployable microservice. **Adding a broadcaster** = an Infrastructure +HTTP client + an `IBroadcasterCrawler` adapter + a `Presentation/Agents/<Broadcaster>` host. The +`Application/Crawling` slice is shared and broadcaster-agnostic — it selects a crawler by provider +key and never learns their names. Walkthrough: +[`docs/adding-a-broadcaster.md`](docs/adding-a-broadcaster.md). Decision records live in [`docs/architecture/`](docs/architecture/). The current ones are **[DR-009](docs/architecture/DR-009-architecture-reset.md)** (architecture reset), @@ -419,8 +422,9 @@ dotnet ef migrations add <Name> --project src/Infrastructure --context AppDbCont ## Contributing -The repo runs **GitFlow**: `develop` is the default branch and the integration trunk, `main` is what -is released, and every release is a `v*` tag on `main`. +Start with [`CONTRIBUTING.md`](CONTRIBUTING.md). The repo runs **GitFlow**: `develop` is the default +branch and the integration trunk, `main` is what is released, and every release is a `v*` tag on +`main`. ```bash git switch develop && git pull --ff-only @@ -429,6 +433,8 @@ git switch -c feat/my-change gh pr create --base develop --label enhancement # one category label — the labels are the changelog ``` +- [`docs/adding-a-broadcaster.md`](docs/adding-a-broadcaster.md) — the walkthrough for a new + Mediathek: HTTP client → `IBroadcasterCrawler` adapter → agent host, with the traps called out - [`docs/branching-and-release.md`](docs/branching-and-release.md) — the branch model, where a fix belongs, protection and merge methods - [`docs/ci.md`](docs/ci.md) — what CI runs and why there is no hand-written YAML here diff --git a/docs/adding-a-broadcaster.md b/docs/adding-a-broadcaster.md new file mode 100644 index 0000000..8107fad --- /dev/null +++ b/docs/adding-a-broadcaster.md @@ -0,0 +1,265 @@ +# Adding a broadcaster + +Krautwatch's catalog is built entirely by per-broadcaster crawlers behind one port, +`IBroadcasterCrawler`. Adding ORF, SRF, arte or any other Mediathek is therefore a **bounded, +reviewable unit of work**: one HTTP client, one adapter, one host, and a handful of registrations. + +This walkthrough follows ZDF, the simplest of the three that exist. Read +[DR-009](architecture/DR-009-architecture-reset.md) (layering) and +[DR-011](architecture/DR-011-search-driven-indexing.md) (why search is query-driven) if you want the +reasoning behind the shape. + +## The shape + +```mermaid +flowchart TD + subgraph Presentation + AG["Agents/<X><br/>CrawlSchedulerService"] + NZ["Api/NewznabIndexerApi<br/>OnDemandResolution"] + end + subgraph Application + CMD["CrawlShowCommand<br/>(ProviderKey, ShowQuery)"] + H["CrawlShowHandler<br/><i>broadcaster-agnostic</i>"] + end + subgraph Infrastructure + CR["<X>BroadcasterCrawler<br/><b>implements the port</b>"] + CL["<X>CatalogClient<br/>raw HTTP + JSON"] + MAP["EpisodeMapper"] + end + subgraph Domain + PORT(["IBroadcasterCrawler"]) + EP["Episode graph<br/>Channel → Show → Episode → Stream"] + end + + AG -->|durable bus| CMD --> H + NZ -->|"in-process (search path)"| CR + H -->|selects by ProviderKey| PORT + PORT -.implemented by.-> CR + CR --> CL + CR --> MAP --> EP + H --> DB[("Postgres<br/>UpsertMany")] + + style PORT fill:#1d4e6f,color:#fff + style CR fill:#2d6a4f,color:#fff +``` + +Two callers, one port. The **agent** crawls a standing list on a schedule (the RSS feed's input); +the **Newznab host** resolves an unseen query live when Sonarr searches for something nobody has +crawled. Both go through your adapter, which is why registering in only one of them is the classic +way to ship a broadcaster that half-works. + +`CrawlShowHandler` never learns your broadcaster's name — it picks the crawler whose `ProviderKey` +matches the command and hands you the query string. + +## The port + +```csharp +public interface IBroadcasterCrawler +{ + /// The catalog scope this crawler serves — matches Channel.ProviderKey. + string ProviderKey { get; } + + /// Crawl one show by (a substring of) its title. Empty list when it can't be found. + Task<IReadOnlyList<Episode>> CrawlShowAsync(string showQuery, CancellationToken ct = default); +} +``` + +`Domain/Interfaces/IBroadcasterCrawler.cs` — the whole contract. You return **fully-formed `Episode` +graphs** (Show + Channel + Streams attached) ready to upsert; nothing downstream does further +enrichment. + +## Step 1 — the catalog client + +`src/Infrastructure/Crawling/<X>/<X>CatalogClient.cs`. A typed `HttpClient` that speaks the +broadcaster's API and returns *broadcaster-shaped* records — not Domain entities. It is the only +place that knows about their JSON. + +Model it on `ZdfCatalogClient`, which does three things: + +1. **Search** — `SearchEpisodesAsync(query)` → the episodes matching a show title. +2. **Resolve a stream** — follow whatever indirection the broadcaster uses (ZDF: episode doc → + `ptmd-template` → PTMD `priorityList` → progressive MP4) and pick the **best progressive MP4**. + HLS is acceptable if that's all they publish: the Downloader dispatches on the URL — anything + containing `.m3u8` gets an ffmpeg remux (`-c copy`), everything else a raw byte copy — so no + flag is needed from you. (`EpisodeMapper` labels every stream `mp4` regardless; the label is + cosmetic, the URL is what routes.) Progressive is still preferred: it's a copy, not a remux. +3. **Fetch detail** — `FetchEpisodeDetailAsync(hit)` → an `EpisodeDetail`, the normalized shape + every broadcaster converges on: + +```csharp +public sealed record EpisodeDetail( + string Title, + string Show, + string Broadcaster, + DateTimeOffset? AirDate, + TimeSpan Duration, + string? Synopsis, + string? StreamUrl, // progressive MP4 (preferred) + string? SubtitleUrl, // webvtt, if available + bool GeoRestricted = false); // in-region-only per the broadcaster's own metadata +``` + +Two fields are easy to leave null and shouldn't be: + +- **`SubtitleUrl`** (#20) — if the broadcaster publishes a WebVTT track, carry it. The Downloader + saves it as `{video}.de.vtt` on a best-effort basis: a missing subtitle never fails the video. +- **`GeoRestricted`** (#45) — take it from *their* metadata (ARD's `isGeoBlocked`, ZDF's + `attributes.geoLocation` where anything but `"none"` counts), never from guesswork. It routes the + download through a German egress proxy, and a job flagged wrongly either fails fast for no reason + or tries a direct fetch that 403s. + +**API keys.** A static key the broadcaster ships in their own public player (like ZDF's `Api-Auth` +bearer) can live in the client as a `const` with a comment saying it rotates — see #13. Anything +user-specific belongs in configuration, never in source. + +## Step 2 — the crawler adapter + +`src/Infrastructure/Crawling/<X>/<X>BroadcasterCrawler.cs`. Thin: orchestrate the client's calls and +map through `EpisodeMapper`. + +```csharp +public sealed class ZdfBroadcasterCrawler(ZdfCatalogClient client) : IBroadcasterCrawler +{ + public string ProviderKey => "zdf"; + + public async Task<IReadOnlyList<Episode>> CrawlShowAsync(string showQuery, CancellationToken ct = default) + { + var hits = await client.SearchEpisodesAsync(showQuery, ct); + if (hits.Count == 0) return []; + + var channel = EpisodeMapper.Channel("zdf", "ZDF"); + var episodes = new List<Episode>(hits.Count); + + foreach (var hit in hits) + { + var detail = await client.FetchEpisodeDetailAsync(hit, ct); + if (detail?.StreamUrl is null) continue; // unplayable → not in the catalog + + var show = /* one Show instance per distinct title, reused across the batch */; + episodes.Add(EpisodeMapper.Episode("zdf", show, NativeId(hit.Canonical), detail)); + } + return episodes; + } +} +``` + +Four rules the existing crawlers all follow: + +| Rule | Why | +|---|---| +| **Only return streamable episodes** — skip anything with no resolved stream | An entry Sonarr can grab but not download is worse than no entry | +| **Reuse one `Channel` and one `Show` instance per batch** | The upsert walks the graph; separate instances of the same show fight each other | +| **Pass the broadcaster's *stable native id*** to `EpisodeMapper.Episode` | Ids are `{providerKey}:{nativeId}`, so re-crawls upsert in place instead of duplicating | +| **Return `[]` rather than throwing** when the show isn't found | "Not found" is a normal answer, and the handler logs it as such | + +`EpisodeMapper` (internal to Infrastructure) does the rest: deterministic ids +(`Channel = providerKey`, `Show = {providerKey}:{slug(title)}`, `Episode = {providerKey}:{nativeId}`), +synopsis truncation, and the season/episode parse that decides whether the show is `Standard` or +`Daily` for Sonarr. Don't hand-roll any of that. + +**One adapter can serve several scopes.** `ArdBroadcasterCrawler` takes `providerKey`, `scope` and +`channelName` as constructor arguments, so ARD and KiKA are two registrations of one class over one +client. Do that when a platform hosts several channels; otherwise hard-code the key like ZDF. + +## Step 3 — register the adapter + +In `src/Infrastructure/InfrastructureServiceExtensions.cs`, next to its siblings: + +```csharp +public static IServiceCollection AddXyzCrawler(this IServiceCollection services) +{ + services.AddHttpClient<XyzCatalogClient>(); + services.AddScoped<IBroadcasterCrawler>(sp => + new XyzBroadcasterCrawler(sp.GetRequiredService<XyzCatalogClient>())); + return services; +} +``` + +`IBroadcasterCrawler` is resolved as `IEnumerable<>`, so every registration simply adds itself to +the set the handler chooses from. + +## Step 4 — the agent host + +`src/Presentation/Agents/<X>/` — copy `Agents/Zdf/Program.cs` and change three lines: the +`AddXyzCrawler()` call, the seed crawl targets, and the comments. Everything else (Aspire service +defaults, the Postgres connection, durable Wolverine, the scheduler) is boilerplate that must stay +identical. + +```csharp +builder.Services.AddXyzCrawler(); +builder.Services.AddMessageDispatcher(); + +var crawlOptions = new CrawlOptions(); +builder.Configuration.GetSection(CrawlOptions.SectionName).Bind(crawlOptions); +if (crawlOptions.Targets.Count == 0) + crawlOptions.Targets = [new CrawlTarget("xyz", "Some Show")]; // fallback seed +``` + +The seed list is a **fallback for an unconfigured deployment**, not the search path — per DR-011 the +standing list feeds the RSS feed, while search resolves on demand. Seed it with one or two shows you +have actually verified, which is also what your live test will use. + +## Step 5 — make it deployable + +Four registrations, and missing any one of them produces a different partial failure: + +| Where | What | If you forget | +|---|---|---| +| `Krautwatch.slnx` | the project, under the `Agents` folder | It doesn't build in CI | +| `src/Presentation/AppHost/Program.cs` | `builder.AddProject<Projects.Krautwatch_Agents_Xyz>("agent-xyz")` with the db reference, `WaitForCompletion(migrator)` and `/health` | It never runs locally, and it is absent from the generated compose file | +| `build/Build.Publish.cs` → `Services` | `("agent-xyz", "<csproj>", "<dll>", false)` | No image is built or published, so the compose file references something that doesn't exist | +| `src/Presentation/Api/NewznabIndexerApi/Program.cs` | `builder.Services.AddXyzCrawler();` inside the `if (resolutionOptions.Enabled)` block | **The scheduled crawl works but search never reaches your broadcaster** — the easiest one to miss | + +The `Ffmpeg` flag in the `Services` tuple stays `false`: only the Downloader needs ffmpeg in its +image. + +## Step 6 — tests + +- **Live test** (required): add a case to `tests/Live.Tests/BroadcasterCrawlerLiveTests.cs`. It hits + the real API, so it is `[Trait("Category", "Live")]` and excluded from the CI gate; run it with + `./build.sh TestLive`. Assert the shape rather than the content — ids prefixed with your provider + key, a non-empty stream list, `mp4` format — because episode titles rotate weekly and an assertion + on one will fail next Tuesday. + +```csharp +[Fact] +public async Task Xyz_crawler_maps_a_known_show_to_domain_episodes_with_streams() +{ + var crawler = new XyzBroadcasterCrawler(new XyzCatalogClient(Http)); + + var episodes = await crawler.CrawlShowAsync("Some Show", TestContext.Current.CancellationToken); + + episodes.ShouldNotBeEmpty(); + episodes.ShouldAllBe(e => e.Id.StartsWith("xyz:")); + episodes[0].Streams.ShouldNotBeEmpty(); +} +``` + +- **Handler tests** need nothing: `CrawlShowHandlerTests` drives the port through a `FakeCrawler`, so + it already covers the selection-by-provider path for a broadcaster that doesn't exist yet. +- **Unit-test your parsing** if the API shape is gnarly — the client's JSON walk is where the bugs + live, and it is testable against a captured payload without the network. +- `./build.sh Test` must stay green, architecture tests included. They will fail you for reaching + across a slice or pointing Infrastructure at Presentation. + +## Checklist + +``` +[ ] Infrastructure/Crawling/<X>/<X>CatalogClient.cs — search · resolve stream · fetch detail +[ ] Infrastructure/Crawling/<X>/<X>BroadcasterCrawler.cs — implements IBroadcasterCrawler +[ ] EpisodeDetail carries SubtitleUrl and GeoRestricted where the broadcaster publishes them +[ ] InfrastructureServiceExtensions.Add<X>Crawler() +[ ] Presentation/Agents/<X>/ host + seed CrawlTarget +[ ] Krautwatch.slnx · AppHost · Build.Publish Services · NewznabIndexerApi on-demand block +[ ] Live test in tests/Live.Tests, passing with ./build.sh TestLive +[ ] ./build.sh Test green +[ ] README's broadcaster list updated +``` + +## What you don't have to do + +- **Touch the Newznab or SABnzbd surfaces.** They read the catalog; they don't care who filled it. +- **Write a Dockerfile.** Images come from `docker/service.Dockerfile` parameterised by project. +- **Edit `.github/workflows/*.yml` or the generated compose file.** Both are generated — see + [ci.md](ci.md) and DR-003. +- **Add a database migration.** Your episodes use the existing model. diff --git a/docs/branching-and-release.md b/docs/branching-and-release.md index 9ee6abb..d080fd1 100644 --- a/docs/branching-and-release.md +++ b/docs/branching-and-release.md @@ -149,32 +149,58 @@ release needs pushing through by hand. ## Merging -Linear history is enforced on every protected branch, so merge commits are out. Which of the two -remaining methods to use depends on the direction: +Linear history is enforced on every protected branch, so merge commits are out. Which method to use +depends on the direction: | Merging | Method | Why | |---|---|---| | `feat/*` → `develop` | **Squash** | Working branches accumulate WIP. One commit per landed change keeps the trunk readable. | -| `develop` → `main` | **Rebase** | Squashing would collapse an entire release into a single commit on the production branch, losing the per-change history — and the release notes are generated from the PRs that make it up. | -| `release/*` → `main` | **Rebase** | Same, and the individual stabilisation commits are what you cherry-pick back to `develop`. | -| `hotfix/*` → `main` | **Rebase** | Same — you need a real commit to port back. | +| `develop` → `main` | **Fast-forward**, pushed locally | Preserves the exact commits — see below. Squash would collapse a whole release into one commit, and GitHub's *Rebase and merge* rewrites them. | +| `release/*` → `main` | **Fast-forward** | Same, and the individual stabilisation commits are what you cherry-pick back to `develop`. | +| `hotfix/*` → `main` | **Squash** (PR) | `main` is the base, so nothing downstream depends on the SHA; the port-back to `develop` is a cherry-pick of it. | | anything → `develop` (port-back) | **Squash** | It's a working branch like any other. | -GitHub can't enforce a method per branch, so this is discipline rather than configuration. Both -methods stay enabled because both are correct somewhere. +GitHub can't enforce a method per branch, so this is discipline rather than configuration. -### Why rebase across two long-lived branches is safe +### Why `main` advances by fast-forward and not by the merge button -Rebase-merge rewrites commits, so `main` never becomes an ancestor of `develop` — and once a hotfix -has landed on `main` and been ported back, the merge base falls behind both. The obvious worry is -that the *next* release would try to replay commits already present on `main`. +**Release notes are generated from the PRs behind the commits in a release**, so anything that +rewrites those commits severs the link. GitHub's *Rebase and merge* rewrites unconditionally — even +when the merge is a pure fast-forward — and the rewritten commits on `main` then belong to the +release PR rather than to the PRs that did the work. -It doesn't. `git rebase` detects already-applied commits by patch-id and drops them, so a second -release replays only the genuinely new work. +That is not hypothetical: v0.3.0 shipped with two PRs missing from its notes for exactly this +reason, and they had to be added by hand. -The edge case to know: if a port-back was **conflict-resolved differently** from the original, its -patch no longer matches and rebase will try to apply it again. That surfaces as a conflict at -release time — visible and fixable, not silent. +A fast-forward keeps the SHAs, so every PR keeps its association and the notes come out right: + +```bash +git switch main && git pull --ff-only +git merge --ff-only develop +git push origin main # GitHub marks the release PR merged automatically +``` + +The release PR still exists — it is what runs CI and what you review. It is just merged by pushing +rather than by clicking. + +### Keeping the fast-forward available + +A fast-forward only works while `main` is a strict ancestor of `develop`. Two things preserve that: + +- **Never push to `main` outside a release.** It only ever moves forward onto commits that are + already on the trunk. +- **After a hotfix, `main` and `develop` diverge** — `main` has the squashed fix, `develop` has the + cherry-picked twin. `git rebase` drops the duplicate by patch-id, so `git rebase origin/main + develop` reconciles them, but it rewrites `develop`'s unreleased commits and costs those PRs + their note entries. + + The cheaper answer for a single-maintainer repo: hotfixes are rare, so **reconcile immediately + after the hotfix** — while `develop` has few or no unreleased commits of its own, the rewrite + costs nothing. Waiting until release time is what makes it expensive. + + The edge case to know either way: if the port-back was **conflict-resolved differently** from the + original, its patch no longer matches and the rebase tries to apply it again. That surfaces as a + conflict — visible and fixable, not silent. ### The double merge-back diff --git a/docs/plans/2026-08-16 - gitflow and ci-cd.md b/docs/plans/2026-08-16 - gitflow and ci-cd.md index 8acfb05..faa5606 100644 --- a/docs/plans/2026-08-16 - gitflow and ci-cd.md +++ b/docs/plans/2026-08-16 - gitflow and ci-cd.md @@ -84,6 +84,27 @@ currently imply PRs target `main`. ## Then: cut v0.3.0 -The first release through the new path. `develop` → `main` by rebase-merge, tag `v0.3.0` on `main`, +The first release through the new path. `develop` → `main`, tag `v0.3.0` on `main`, which fires `publish-ghcr` and `publish-release`. Minor bump because subtitle download (#20) is a new user-facing capability; the rest of the batch is fixes. + +## Outcome (2026-08-16) + +Done, and proven rather than assumed: + +- The trunk push published all six multi-arch images to `:edge` in **8m30s**; the docs-only push + that followed correctly published nothing, so the path filter works. +- **v0.3.0** went out with `:0.3.0` images (amd64 + arm64) and a release carrying + `docker-compose.yaml` + `env.example`. +- Rulesets applied to `develop`/`main`/`support/*` and to `v*` tags; default branch switched; merge + commits disabled. + +**One thing the first release taught us.** Its notes were missing #101 and #103. GitHub's *Rebase +and merge* rewrites commits even for a pure fast-forward, so `main`'s copies were associated with +the release PR (`skip-changelog`) instead of the PRs that did the work, and the generated notes +dropped them. Fixed by advancing `main` with a **fast-forward push** instead — documented in +[branching-and-release.md](../branching-and-release.md#merging) — after a one-off realignment of +`develop` onto `main`'s SHAs so the two share ancestry again. v0.3.0's notes were amended by hand. + +The lesson generalises: **anything that rewrites commits between `develop` and `main` costs the +release its changelog**, because the changelog is derived from the commit→PR link and nothing else. diff --git a/docs/releasing.md b/docs/releasing.md index 787aa05..5b0b38c 100644 --- a/docs/releasing.md +++ b/docs/releasing.md @@ -65,7 +65,7 @@ flowchart TD B -->|Yes| D["Cut release/X.Y.Z from develop"] D --> E["Fixes only, on the release branch"] E --> F["PR release/X.Y.Z → main"] - C --> G["Rebase-merge into main"] + C --> G["Fast-forward main"] F --> G G --> H["Tag main"] H --> I["publish-ghcr + publish-release fire"] @@ -77,8 +77,11 @@ flowchart TD ```bash git switch develop && git pull --ff-only gh pr create --base main --title "Release v0.3.0" --label skip-changelog -# merge with REBASE (see below), then: + +# once the gate is green, advance main by FAST-FORWARD — not the merge button: git switch main && git pull --ff-only +git merge --ff-only develop +git push origin main # this also marks the release PR merged git tag v0.3.0 && git push origin v0.3.0 ``` @@ -96,16 +99,19 @@ git switch -c release/0.3.0 develop git push -u origin release/0.3.0 # … fixes land here by PR; feature work continues on develop … gh pr create --base main -# merge, tag as above, then port the stabilisation commits back: +# fast-forward main onto release/0.3.0 and tag, as above, then port the fixes back: git switch -c chore/port-0.3.0 develop git cherry-pick <fix-sha>… gh pr create --base develop --label skip-changelog ``` -> **Merge with rebase, not squash, into `main`.** Squashing collapses a whole release into one -> commit, losing the per-change history on the production branch — and the release notes are -> generated from the PRs that make it up. Reasoning and the one edge case in -> [branching-and-release.md](branching-and-release.md#merging). +A `release/*` branch is cut from `develop`, so `main` fast-forwards onto it the same way. What does +*not* fast-forward is `develop` afterwards — hence the cherry-pick. + +> **Do not use GitHub's merge button on a release PR.** It rewrites the commits even when the merge +> is a pure fast-forward, and the release notes are built from the commit→PR link that rewriting +> severs — v0.3.0 lost two entries to exactly this. Push a fast-forward instead. Reasoning and the +> hotfix-divergence case in [branching-and-release.md](branching-and-release.md#merging). > **"Merge back to develop" is a cherry-pick or a second PR here**, not a literal merge. GitFlow > assumes merge commits; this repo enforces linear history. The effect is the same — the fix must @@ -170,7 +176,10 @@ Rehearse the whole thing without publishing anything: ## Release notes are the PR labels There is no `CHANGELOG.md`. The notes are generated from merged PRs grouped by label, so improving -them is a matter of labelling PRs rather than writing release prose twice. One category label per +them is a matter of labelling PRs rather than writing release prose twice. That generation walks the +commits in the release and asks GitHub which PR each came from — which is why `main` must +[fast-forward](branching-and-release.md#why-main-advances-by-fast-forward-and-not-by-the-merge-button) +rather than be rewritten. One category label per PR (`enhancement`, `bug`, `breaking-change`, `security`, `documentation`, `dependencies`), or `skip-changelog` for housekeeping.