From 47b474f1aa766f0b930a7168bb618843ff668281 Mon Sep 17 00:00:00 2001 From: Geeven Date: Mon, 7 Sep 2026 11:27:41 -0700 Subject: [PATCH 1/4] Make versioning rules post-1.0 and define "breaking" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AGENTS.md §12 and CHANGELOG.md's preamble both still described the project as pre-1.0, claiming breaking changes ride in minor bumps until v1.0.0. The project has been past that since the v1.0.0 tag and is now at v1.9.0, so both statements actively misled readers. Separately, "breaking" was never defined. For a library it self- evidently means a changed signature; DiffViewer ships a WPF desktop app with no published package and no external API consumers, so the rule as written gave no usable guidance. That ambiguity is what prompted the question of whether a large feature justifies a major bump. Rewrite the bump rules as plain post-1.0 SemVer and add a "What counts as 'breaking'" subsection built on a definition a client app can actually apply: a breaking change is an upgrade that can disturb an existing install. Enumerate the five compatibility surfaces that definition covers — the CLI argv contract, the settings.json / recents.json on-disk formats, the user-PATH install footprint, the keyboard-shortcut bindings, and the Velopack update channel — each cited to the type or file that owns it. Settle the previously-unaddressed question of non-compatibility major bumps: major is reserved exclusively for compatibility breaks. Size is not a trigger. A major that only means "this release is big" teaches users that majors are safe to take, which inverts the signal on the one occasion it matters. Two adjacent corrections this change forced: - The lead paragraph claimed "there is no auto-update channel", which is false — VelopackUpdateService.cs exists, and release.yml packs a Setup.exe plus RELEASES metadata for the updater to consume. Left as-is it would have contradicted compatibility surface #5 two paragraphs below. - The new subsection is a sibling ### of "When to release" rather than a nested ####, so the existing skip-releases, group-commits, and agent-must-not-tag paragraphs stay under the heading they belong to. Bump semantics are otherwise unchanged: feature is still minor, bug-fix batch is still patch, and a Copilot CLI session still must not push a release tag on its own judgment. The CHANGELOG.md edit is confined to the preamble to avoid conflicting with PR #24, which touches [Unreleased]. Docs only; no .cs, .xaml, or .csproj files touched. AI-Local-Session: ada363aa-6c26-4bcf-b059-b369422de5a7 AI-Cloud-Session: 747f79eb-14cd-4937-9ae0-90a063986dd6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 80 +++++++++++++++++++++++++++++++++++++++++++--------- CHANGELOG.md | 5 ++-- 2 files changed, 70 insertions(+), 15 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 766b55b..ab55920 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -444,19 +444,30 @@ to touch `CHANGELOG.md`. ### When to release -Each release is a user-visible Windows `.exe` artifact downloaded from -GitHub Releases. There is no auto-update channel and no nightly feed -— releases *are* the distribution. Tag deliberately, not on a fixed -cadence and not per commit. - -**Bump rules (pre-1.0 SemVer):** - -- User-facing feature added → **minor** bump (e.g., `v0.1.0` → - `v0.2.0`). -- Bug-fix-only batch → **patch** bump (e.g., `v0.1.0` → `v0.1.1`). -- Pre-1.0 breaking changes ride in minor bumps (standard SemVer - carve-out for `0.y.z`). Once the project hits `1.0.0`, breaking - changes require a major bump. +Each release is a user-visible Windows artifact downloaded from GitHub +Releases — and, for installs that came through the Velopack `Setup.exe`, +the payload that in-field copies upgrade *themselves* to. There is no +nightly feed; releases *are* the distribution. Tag deliberately, not on +a fixed cadence and not per commit. + +**Bump rules (SemVer):** + +- User-facing feature added → **minor** bump (e.g., `v1.9.0` → + `v1.10.0`). +- Bug-fix-only batch → **patch** bump (e.g., `v1.9.0` → `v1.9.1`). +- Breaking change → **major** bump (e.g., `v1.9.0` → `v2.0.0`). See + "What counts as 'breaking'" below. + +The project is past `1.0.0`, so the pre-1.0 carve-out that let +breaking changes ride in minor bumps no longer applies. + +**Major bumps are reserved for compatibility breaks.** Size is not a +trigger. A large feature, a rewritten pane, or a redesigned UI is +still a **minor** bump so long as it doesn't disturb an existing +install. Say "this one is big" in the `CHANGELOG.md` section and the +release title, not in the version number — a major that only means +"big" teaches users that majors are safe to take, which is precisely +backwards on the one occasion the signal matters. **Skip releases for** doc-only commits, build hygiene, test-only changes, and pure refactors that don't change shipped behavior. These @@ -479,6 +490,49 @@ to tag. Recommending a release in conversation is fine and encouraged when a meaningful delta has accumulated; pushing the tag without an explicit command is not. +### What counts as "breaking" + +DiffViewer publishes no library and has no external API consumers, so +the usual "a public signature changed" test says nothing useful here. +Use this instead: + +> A breaking change is an upgrade that can disturb an existing +> install. + +Five surfaces carry that risk. A change that violates any of them is +breaking no matter how small the diff is: + +1. **The CLI argv contract.** Both the positional form + (`DiffViewer.exe `) and the flag form + (`--repo` / `--left` / `--right` / `--file`) — see + `Services/CommandLineParser.cs` and the "Command-line launch" + section of `README.md`. Users wire these into shell aliases, `git` + aliases, and editor integrations that live outside this repo, so + changing what a positional slot means, or dropping a flag, breaks + setups we can neither see nor migrate. +2. **On-disk state formats.** `settings.json` and `recents.json` under + `%APPDATA%\DiffViewer`. Both are already explicit versioned + contracts: `Services/SettingsMigrations.cs` chains v(N) → v(N+1) + migrations, and `Services/RecentsJsonSerializer.cs` documents the + downgrade-safety rule that an unknown future version loads as empty + rather than throwing. Breaking here means a user loses their + configuration on upgrade, or corrupts it on downgrade. +3. **Install footprint.** `Services/UserPathRegistrar.cs` and + `Services/WindowsUserPathStore.cs` put the install directory on the + per-user `PATH` from the Velopack install/update hooks. Renaming the + executable or changing how that entry is registered orphans it — + leaving a stale `PATH` entry and a name that no longer resolves. +4. **Keyboard shortcuts.** `Models/KeyboardShortcutCatalog.cs`. Adding + a binding is a feature; repointing an existing binding at a + different action is a silent behavioural break — muscle memory + starts doing something the user never asked for, with no error to + read. +5. **The update channel.** `Services/VelopackUpdateService.cs` and the + `vpk pack --packId DiffViewer` identity in `release.yml`. A change + that strands in-field installs so they can no longer self-update is + maximally breaking: it removes the very mechanism by which a + subsequent fix would have reached them. + ## 13. Origin DiffViewer was extracted from the [geevensingh/DevTools][devtools] diff --git a/CHANGELOG.md b/CHANGELOG.md index 95dffd1..9cd9919 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,8 +3,9 @@ All notable changes to DiffViewer are documented in this file. The format is based on [Keep a Changelog 1.1.0](https://keepachangelog.com/en/1.1.0/), -and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html) -(with the pre-1.0 carve-out: breaking changes ride in minor bumps until v1.0.0). +and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +Major bumps are reserved for changes that can disturb an existing install; +see "What counts as 'breaking'" in `AGENTS.md` §12 for the specifics. `.github/workflows/release.yml` reads the section matching the pushed tag (e.g. `## [0.2.0]` for `v0.2.0`) verbatim and uses it as the GitHub Release From f38566a3433df17f7bc311ce22f463495eaed599 Mon Sep 17 00:00:00 2001 From: Geeven Date: Mon, 7 Sep 2026 12:25:15 -0700 Subject: [PATCH 2/4] Correct three "breaking" surface claims against implementation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Surfaces 2, 3, and 5 of the new "What counts as 'breaking'" section described intent taken from nearby XML doc comments rather than what the code actually does. Re-verified all five against implementation. Surface 2 was outright wrong. It claimed RecentsJsonSerializer loads an unknown future version as empty. Deserialize actually discards the version outright (`_ = TryInt(obj, "version")`), keeps every row that parses, and re-stamps at RecentsDoc.CurrentVersion on the next write — the comment directly above that line records that Phase 7 of the PR-review feature superseded the "unknown version = empty" policy. The stale class-level XML doc on RecentsJsonSerializer still asserts the old rule and is what misled the original text; it is production code and is deliberately left alone here. Re-verifying also turned up something better than the original claim: settings.json and recents.json use opposite strategies, not a shared one. settings.json migrates forward through registered v(N) -> v(N+1) steps and refuses to read a schemaVersion newer than the binary, backing the file up and starting from defaults. recents.json is forgiving in exactly the way described above. Both are now documented as the distinct contracts they are. Surface 3 conflated two different failures. The PATH entry holds AppContext.BaseDirectory (the Velopack `current` directory), not the executable, so renaming the exe leaves the entry valid and only breaks the command name; stranding the entry requires changing which directory gets registered. Separate them, name App.xaml.cs as the place the hooks are wired, and note the hooks only fire for the Velopack-installed copy. Surface 5 named a file where it should have named an identity. What must stay stable is the pairing of the GithubSource that TryCreateForInstalled resolves against with the `vpk pack --packId` value stamped by release.yml. Surfaces 1 and 4 re-verified as accurate: CommandLineParser really does accept 0-3 positional args and reject any unrecognised leading-dash flag in both grammars, and KeyboardShortcutCatalog really does hold the KeyChord bindings mirroring MainWindow.xaml. Docs only; no .cs, .xaml, or .csproj files touched. AI-Local-Session: ada363aa-6c26-4bcf-b059-b369422de5a7 AI-Cloud-Session: 747f79eb-14cd-4937-9ae0-90a063986dd6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 50 ++++++++++++++++++++++++++++++++++---------------- 1 file changed, 34 insertions(+), 16 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ab55920..fd044e1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -511,27 +511,45 @@ breaking no matter how small the diff is: changing what a positional slot means, or dropping a flag, breaks setups we can neither see nor migrate. 2. **On-disk state formats.** `settings.json` and `recents.json` under - `%APPDATA%\DiffViewer`. Both are already explicit versioned - contracts: `Services/SettingsMigrations.cs` chains v(N) → v(N+1) - migrations, and `Services/RecentsJsonSerializer.cs` documents the - downgrade-safety rule that an unknown future version loads as empty - rather than throwing. Breaking here means a user loses their - configuration on upgrade, or corrupts it on downgrade. -3. **Install footprint.** `Services/UserPathRegistrar.cs` and - `Services/WindowsUserPathStore.cs` put the install directory on the - per-user `PATH` from the Velopack install/update hooks. Renaming the - executable or changing how that entry is registered orphans it — - leaving a stale `PATH` entry and a name that no longer resolves. + `%APPDATA%\DiffViewer`. Both are versioned contracts, and they use + deliberately *opposite* strategies — a change here must not silently + break either: + - `settings.json` migrates forward and refuses to go backward. + `SettingsMigrations.MigrateUpTo` chains registered v(N) → v(N+1) + steps and throws if one is missing. When the file's + `schemaVersion` is newer than the binary, `SettingsService` + declines to read it, copies it to `settings.json.bak.`, + and starts from defaults. + - `recents.json` is forgiving instead. `RecentsJsonSerializer` + reads the stored version and discards it, keeps every row that + parses, ignores unknown sibling fields, and re-stamps the file at + `RecentsDoc.CurrentVersion` on the next write — so version drift + heals itself. + + Break either and a user loses their setup on upgrade, or has it + quietly reset on downgrade. +3. **Install footprint.** `App.xaml.cs` wires `UserPathRegistrar` into + Velopack's after-install / after-update / before-uninstall + callbacks, putting `AppContext.BaseDirectory` — the stable Velopack + `current` directory — onto the per-user `PATH` through + `WindowsUserPathStore`, so `diffviewer` resolves from any new + terminal. Two distinct breaks live here: renaming the executable + leaves the `PATH` entry valid but stops the old command name from + resolving, while changing which directory gets registered strands a + stale entry pointing at nothing. These hooks only fire for the + Velopack-installed copy — portable and dev launches never register. 4. **Keyboard shortcuts.** `Models/KeyboardShortcutCatalog.cs`. Adding a binding is a feature; repointing an existing binding at a different action is a silent behavioural break — muscle memory starts doing something the user never asked for, with no error to read. -5. **The update channel.** `Services/VelopackUpdateService.cs` and the - `vpk pack --packId DiffViewer` identity in `release.yml`. A change - that strands in-field installs so they can no longer self-update is - maximally breaking: it removes the very mechanism by which a - subsequent fix would have reached them. +5. **The update channel.** An install finds its upgrades through the + identity `VelopackUpdateService.TryCreateForInstalled` resolves + against — a `GithubSource` pointed at this repo's Releases — paired + with the `vpk pack --packId DiffViewer` that stamps `release.yml`'s + output. Change either side and in-field installs stop matching the + feed. That is maximally breaking: it removes the very mechanism by + which a subsequent fix would have reached them. ## 13. Origin From 28ad7da5143f63308782a30614e2e078b781dcc4 Mon Sep 17 00:00:00 2001 From: Geeven Date: Mon, 7 Sep 2026 12:29:18 -0700 Subject: [PATCH 3/4] Add platform floor as a breaking surface; fix backup wording Both from Copilot review on PR #25; each verified against source before acting. The five-surface list was presented as exhaustive but omitted the platform contract. README.md promises Windows 10 or later on x64, and the Release config pins RuntimeIdentifier win-x64 with SelfContained on top of net8.0-windows. Raising the OS floor or switching architecture plainly "disturbs an existing install" yet fell outside all five surfaces, so the rule as written would have misclassified it as a minor bump. Added as surface 6 rather than hedging the list as non-exhaustive. The point of this section is that the major-bump call is mechanically decidable; "among others" would put the judgment call straight back in. The new entry also names the interaction the reviewer did not: because surface 5 installs updates on its own, a platform drop can push an in-field copy into a build it can no longer run without its owner choosing to. Separately, surface 2 said SettingsService "copies" a future-versioned settings file to settings.json.bak.. BackupAndUseDefaults calls File.Move (SettingsService.cs:151), so the original does not survive at its path. Reworded to "moves it aside to", which is what a reader needs in order to reason about the downgrade case. Docs only; no .cs, .xaml, or .csproj files touched. AI-Local-Session: ada363aa-6c26-4bcf-b059-b369422de5a7 AI-Cloud-Session: 747f79eb-14cd-4937-9ae0-90a063986dd6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index fd044e1..b88a849 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -499,7 +499,7 @@ Use this instead: > A breaking change is an upgrade that can disturb an existing > install. -Five surfaces carry that risk. A change that violates any of them is +Six surfaces carry that risk. A change that violates any of them is breaking no matter how small the diff is: 1. **The CLI argv contract.** Both the positional form @@ -518,8 +518,8 @@ breaking no matter how small the diff is: `SettingsMigrations.MigrateUpTo` chains registered v(N) → v(N+1) steps and throws if one is missing. When the file's `schemaVersion` is newer than the binary, `SettingsService` - declines to read it, copies it to `settings.json.bak.`, - and starts from defaults. + declines to read it, moves it aside to + `settings.json.bak.`, and starts from defaults. - `recents.json` is forgiving instead. `RecentsJsonSerializer` reads the stored version and discards it, keeps every row that parses, ignores unknown sibling fields, and re-stamps the file at @@ -550,6 +550,13 @@ breaking no matter how small the diff is: output. Change either side and in-field installs stop matching the feed. That is maximally breaking: it removes the very mechanism by which a subsequent fix would have reached them. +6. **The platform floor.** `README.md` promises Windows 10 or later on + **x64**, and the Release config pins `RuntimeIdentifier win-x64` + with `SelfContained` on top of `net8.0-windows`. Raising the OS + floor or changing the architecture strands machines that run the app + today. This one compounds with surface 5: because updates install + themselves, an in-field copy can be upgraded into a build it can no + longer run without its owner ever choosing to. ## 13. Origin From 6eabd65f5428b3a9c531ab475243c3b15bd3d8b4 Mon Sep 17 00:00:00 2001 From: Geeven Date: Mon, 7 Sep 2026 12:38:51 -0700 Subject: [PATCH 4/4] Add the PR-URL launch form to the CLI compatibility surface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From Copilot review on PR #25, verified against source before acting. Surface 1 enumerated only the positional and flag forms, and led with "Both" — a closed list of two. DiffViewer accepts a third documented form: a lone GitHub pull-request URL. README.md:15-17 advertises `DiffViewer.exe https://github.com/owner/repo/pull/123`, and CommandLineParser.ParseLaunch (lines 37-40) matches it via PullRequestRef.TryParse and returns CommandLineLaunchPlan.FromPullRequest before positional parsing runs. That omission matters for the same reason the missing platform floor did: the list is deliberately exhaustive so the major-bump call stays mechanically decidable. Leaving a documented launch form out means removing or narrowing it would be misclassified as non-breaking, when it would in fact break any alias, script, or handler a user has wired to open a PR in DiffViewer. Rewrote the entry to name all three forms and to cover narrowing which URLs parse, not just positional-slot and flag changes. Docs only; no .cs, .xaml, or .csproj files touched. AI-Local-Session: ada363aa-6c26-4bcf-b059-b369422de5a7 AI-Cloud-Session: 747f79eb-14cd-4937-9ae0-90a063986dd6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- AGENTS.md | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b88a849..1bf5591 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -502,14 +502,16 @@ Use this instead: Six surfaces carry that risk. A change that violates any of them is breaking no matter how small the diff is: -1. **The CLI argv contract.** Both the positional form - (`DiffViewer.exe `) and the flag form - (`--repo` / `--left` / `--right` / `--file`) — see - `Services/CommandLineParser.cs` and the "Command-line launch" - section of `README.md`. Users wire these into shell aliases, `git` - aliases, and editor integrations that live outside this repo, so - changing what a positional slot means, or dropping a flag, breaks - setups we can neither see nor migrate. +1. **The CLI argv contract.** Three launch forms, all documented in + `README.md` and dispatched by `Services/CommandLineParser.cs`: the + positional form (`DiffViewer.exe `), the flag + form (`--repo` / `--left` / `--right` / `--file`), and a lone GitHub + pull-request URL, which `ParseLaunch` routes to the PR resolver + before positional parsing ever runs. Users wire these into shell + aliases, `git` aliases, and editor integrations that live outside + this repo, so changing what a positional slot means, dropping a + flag, or narrowing which URLs parse breaks setups we can neither see + nor migrate. 2. **On-disk state formats.** `settings.json` and `recents.json` under `%APPDATA%\DiffViewer`. Both are versioned contracts, and they use deliberately *opposite* strategies — a change here must not silently