diff --git a/docs/adr/0001-forge-providers-not-unified.md b/docs/adr/0001-forge-providers-not-unified.md index f32b052..23da532 100644 --- a/docs/adr/0001-forge-providers-not-unified.md +++ b/docs/adr/0001-forge-providers-not-unified.md @@ -1,35 +1,13 @@ -# Forge providers stay separate; share only what is shared by standard - -**Decision:** `GitHubProvider` and `GitLabProvider` remain independent classes. They are **not** -collapsed into a single descriptor-driven `RestForgeProvider` engine parameterised by a per-forge -spec. The only shared mechanism extracted is the RFC 8288 Link-header pagination loop behind -`list_tags`, which lives once in `semvertag/providers/_rest.py`. - -An architecture review flagged roughly 70% line-level similarity between `providers/github.py` and -`providers/gitlab.py` — four methods, the tag pagination loop, the try/except-then-translate pattern -— and proposed a deep engine whose differing response shapes (`commit.message` nested vs flat -`message`, `sha` vs `id`) would be normalised through pydantic validation aliases into one uniform -attribute surface. Three options were on the table: keep the duplication and spend the effort on real -bugs; extract only the pagination driver; or go to the full descriptor engine. - -GitHub's v4 REST API and GitLab's v4 REST API are **independently versioned third-party contracts**. -Most of the line-level similarity is *coincidental* — both are REST CRUD — not *essential*. Unifying -them behind one engine couples two contracts that will drift, and turns the shared engine into a -magnet for `if forge == "github"` conditionals: the wrong abstraction, which is strictly worse than -two honest copies. The pydantic-alias normalisation is the tell, because it pretends two different -API shapes are one. - -The discriminating test adopted here, and reused by every later decision of this kind: **extract only -what is shared by *standard*, not by *coincidence*.** Link-header pagination is implemented the same -way by both forges because it is RFC 8288, not a coincidence — genuinely deep, stable, reused, so it -earns a shared home. Response shapes, URL paths, create-tag payloads, and conflict semantics (GitHub -422 vs GitLab 400) are where the two APIs are honestly independent and *will* diverge; those stay -separate by design. Keeping the duplication wholesale was defensible but left the one -genuinely-shared, standard mechanism copy-pasted. - -**Revisit trigger:** a third forge that also paginates via RFC 8288 Link headers is added *and* its -commit/tag/default-branch operations turn out to be expressible as pure per-forge **data** with -**zero** forge-conditionals in the shared code. Two such forges plus a clean data-only third would -mean the descriptor engine is a real seam rather than a forced unification, and it is worth -re-pricing. Conversely, the moment the shared pagination helper needs its first forge-conditional, -narrow it back toward two independent copies. +# Forge providers stay separate; only what is shared by standard is extracted + +`GitHubProvider` and `GitLabProvider` stay independent classes despite roughly 70% line-level +similarity, and the per-forge status ladders in `providers/_errors.py` stay as mirrored functions. +An architecture review proposed a descriptor-driven engine that would flatten `sha` against `id` and +nested against flat commit messages through pydantic validation aliases, and a generic translator +driven by a per-forge table of message strings; both were rejected. The two REST APIs are +independently versioned third-party contracts, so most of the similarity is coincidental rather than +essential, and a shared engine becomes a magnet for `if forge == ...` conditionals, which is worse +than two honest copies. What is shared by standard is extracted instead: RFC 8288 Link-header +pagination lives once in `_rest.collect_link_pages`, as does the transport translator, whose +messages differ only by a provider label. A third forge, or the first real drift between the two +ladders, would reprice this. diff --git a/docs/adr/0002-blank-settings-values-normalize-to-unset.md b/docs/adr/0002-blank-settings-values-normalize-to-unset.md deleted file mode 100644 index 1a21f07..0000000 --- a/docs/adr/0002-blank-settings-values-normalize-to-unset.md +++ /dev/null @@ -1,28 +0,0 @@ -# Blank optional settings values normalize to unset, not reject - -**Decision:** for an *optional* `Settings` field where blank means "no value, use the fallback", a -declared-but-empty or whitespace-only input normalizes to `None` through a field validator. Such a -field is **not** guarded with `min_length` or a hard `ValidationError`. - -`Settings.default_branch` first shipped a draft guarded by `pydantic.Field(default=None, -min_length=1)`, intending to reject the degenerate `--default-branch ""`. Review caught that this is -a regression: pydantic-settings materializes a declared-but-empty environment variable -(`SEMVERTAG_DEFAULT_BRANCH=`, a common CI idiom where a variable is exported with no value) as the -string `""`, so `min_length=1` raised `ValidationError` → `ConfigError` and aborted **every** -invocation. Before the field was wired up, that same empty variable was a harmless no-op. - -The distinction is what blank *means for this field*. When blank means "I am not setting this; fall -back to the default or derived value" — and `default_branch` falls back to the forge API — the -correct behavior is to normalize blank to `None`. Rejecting it turns a no-op into a crash and -punishes the CI idiom of declaring a variable without populating it. Stripping also lets a -stray-padded value (`" main "`) still resolve. Only when blank is *genuinely invalid* — there is no -fallback and an empty value cannot mean anything sensible — is a hard rejection correct. - -The canonical shape is a `field_validator` that strips and returns `stripped or None`, which keeps -`None` the single "unset" sentinel and leaves every downstream reader a dead-simple `is not None` -check. - -**Revisit trigger:** a specific field where blank input must be a **hard error** — the field has no -fallback and an empty value is a configuration mistake the user must see immediately, such as a -required token where `""` should fail loudly rather than silently behave as unset. Prefer explicit -rejection for that field; this decision governs only the blank-means-fallback case. diff --git a/docs/adr/0002-outcome-renderings-stay-split.md b/docs/adr/0002-outcome-renderings-stay-split.md new file mode 100644 index 0000000..2edc4aa --- /dev/null +++ b/docs/adr/0002-outcome-renderings-stay-split.md @@ -0,0 +1,11 @@ +# Outcome renderings stay split between wire and human + +`to_run_result` in `_outcome.py` and `_format_outcome` in `_output.py` are two exhaustive `match` +statements over the same closed `Outcome` sum, rather than two renderings hung on the variants +themselves. Only the match skeleton is shared, and no string is, by design: the wire arm builds a +frozen machine contract of fixed status tokens and reasons that `action.yml` parses with `jq`, while +the human arm builds a sentence that is free to be reworded. Co-locating them would trade +locality-of-concern for locality-of-variant and drag presentation phrasing into a module that today +depends only on `_types`. The drift a shared home would guard against is already type-enforced: +both matches end in `assert_never`, so a sixth variant is a type error in both arms until handled. +Unification earns its keep only once a variant's two renderings must be the identical string. diff --git a/docs/adr/0003-error-translators-not-tabled.md b/docs/adr/0003-error-translators-not-tabled.md deleted file mode 100644 index b85f320..0000000 --- a/docs/adr/0003-error-translators-not-tabled.md +++ /dev/null @@ -1,38 +0,0 @@ -# Error translators stay duplicated, not table-driven - -**Decision:** `semvertag/providers/_errors.py` keeps its two per-forge status ladders — and the -paired auth translators and create-tag specials — as explicit, mirrored functions. The shared -status-to-exception ladder is **not** extracted into one generic translator driven by a per-forge -message table. - -An architecture review flagged that the two functions share a ladder shape (401/403 → `AuthError`, -404 → `ConfigError`, 422 → `ConfigError`, 429 and 5xx → `ProviderAPIError`, else → -`ProviderAPIError`) and proposed collapsing them into one translator parameterised by a per-forge -table of message strings, mirroring how the transport translator is already shared. - -The split between what is shared and what varies kills the candidate. What is shared is small and -stable: only the ladder *skeleton* — which HTTP code maps to which domain exception — and that is -fixed HTTP semantics. It does not change, and the two ladders have not drifted; it is the part least -in need of a single source of truth. What varies is the bulk, and it cannot be deduped: every message -string is genuinely per-forge — the scope hints (`api`/`write_repository` versus -`contents: write`/`public_repo`), the identifier and its environment-variable hint (`project_id` + -`CI_PROJECT_ID` versus `repo` + `GITHUB_REPOSITORY`), and the tag-exists fragment (`already exists` -versus `already_exists`). That text stays per-forge data whether or not the ladder is extracted. - -So the table trades two linearly-readable ladders for a message-table struct of roughly seven fields -— some of them callables for the parameterised rungs — plus a generic function and the indirection -between them. Net-neutral on lines, worse on locality: one forge's error handling can no longer be -read top to bottom. It also risks exactly the failure mode -[ADR-0001](0001-forge-providers-not-unified.md) named: the first time one forge gives a status a new -meaning, the shared ladder grows an `if forge == …` conditional — the wrong abstraction *plus* -indirection. - -The transport translator is *correctly* shared because its messages are uniform, differing only by a -provider label. The status ladder is not like that, so the file already draws the line in the right -place: the type-mapping is standard, but the per-rung messages — the thing a table would have to -carry — are forge-specific content. - -**Revisit trigger:** a third forge is added, since three copies of the ladder shifts the balance -toward a table; or the two ladders **actually drift** — one forge starts mapping a status to a -different exception type, or grows a rung the other lacks. At that point the single-source-of-truth -value is real. Until then, duplication is cheaper than the abstraction. diff --git a/docs/adr/0003-semver-form-tags-only.md b/docs/adr/0003-semver-form-tags-only.md new file mode 100644 index 0000000..0bd568a --- /dev/null +++ b/docs/adr/0003-semver-form-tags-only.md @@ -0,0 +1,13 @@ +# The bump baseline is SemVer-form only, and a prerelease baseline finalizes + +`_select_latest_semver_tag` keeps only tags `semver.Version.parse` accepts, strips build metadata, +and takes the maximum by SemVer precedence; `_compute_new_version` then calls `Version.next_version` +rather than `bump_*`, so a prerelease baseline finalizes (`1.0.0-rc.1` plus a patch bump gives +`1.0.0`, not `1.0.1`). That is the correct release-ramp semantics and is identical to `bump_*` on +every stable baseline, so it changed no existing behaviour. PEP 440 prereleases such as `0.9.0rc1` +are deliberately skipped: python-semver cannot parse them, its `coerce` recipe discards the `rc1` and +makes a prerelease masquerade as final, and honest support means running `packaging` alongside +`semver` to consume a form a SemVer tagger should not have to. A leading `v` is skipped too, which is +a deferral rather than a rejection, since it is a one-line strip but would make semvertag consume a +convention it does not emit; the cost until then is that a repo whose history is entirely +`v`-prefixed reports `NoTags` and never bumps. diff --git a/docs/adr/0004-no-doctor-preflight-command.md b/docs/adr/0004-no-doctor-preflight-command.md new file mode 100644 index 0000000..30412dc --- /dev/null +++ b/docs/adr/0004-no-doctor-preflight-command.md @@ -0,0 +1,12 @@ +# No `doctor` preflight command + +semvertag ships one verb, `tag`; configuration and permission problems surface from the real run as +a typed `AuthError`, `ConfigError` or `ProviderAPIError` carrying its own exit code. A `doctor` +subsystem shipped and was removed pre-1.0. It checked token validity, token scopes, project access +and protected-tag permission, and chose its exit code by matching string fragments out of a check's +cause, which had to be kept in lockstep with the provider's wording. Every failure it could name +already surfaces from the ordinary run with the same exit code and a message that names the fix, so +the preflight bought only a few seconds of earliness from a second code path. It also taxed the +forge-neutral `Provider` protocol with four `check_*` operations every new forge would owe, and a +small protocol is what made the GitHub provider cheap. Wanting a faster failure is not a reason to +bring it back; a failure the real run genuinely cannot report would be. diff --git a/docs/adr/0004-outcome-renderings-stay-split.md b/docs/adr/0004-outcome-renderings-stay-split.md deleted file mode 100644 index 87d6cb9..0000000 --- a/docs/adr/0004-outcome-renderings-stay-split.md +++ /dev/null @@ -1,33 +0,0 @@ -# Outcome renderings stay split (wire vs human), not unified on the variant - -**Decision:** `to_run_result` in `semvertag/_outcome.py` (Outcome → JSON wire DTO) and -`_format_outcome` in `semvertag/_output.py` (Outcome → human sentence) remain two independent `match` -statements over the closed `Outcome` sum. Both renderings are **not** moved onto the variants -themselves to co-locate them. - -An architecture review flagged that two exhaustive matches walk the same five-variant sum, and that a -comment in `_outcome.py` instructs the maintainer that the two audiences are worded differently on -purpose and both must be edited together. The proposed deepening: give each variant both renderings, -so the two outputs ask the variant instead of re-matching it. - -The same test as [ADR-0001](0001-forge-providers-not-unified.md) and -[ADR-0003](0003-error-translators-not-tabled.md) kills it. What is shared is the `match` skeleton, -and it is coincidental — the only common structure is the sum's cardinality, not duplicated content. -What varies is the bulk and cannot be deduped: the wire arm builds a stable machine contract with -fixed status tokens and fixed reasons; the human arm builds presentation — a `No tag created — …` -sentence, a short commit, a tag interpolated into the already-tagged case. No string is shared -between them, by design. - -The drift the comment warns about is already type-enforced: both matches end in `assert_never`, so -adding a sixth variant is a type error in *both* arms until handled. There is no silent -forgot-to-update-the-other failure mode to prevent. - -And unifying mixes two concerns. The wire contract is stable and machine-facing; the sentence is -mutable and human-facing. Co-locating them trades locality-of-concern — all wire tokens in one place, -all phrasing in one place, both already true — for locality-of-variant, and drags presentation -phrasing into a module that today depends only on `_types`. - -**Revisit trigger:** the wire reason and the human sentence for a variant must become the -**identical** string — a genuine single fact rendered once — or a variant's two renderings must stay -byte-for-byte in lockstep by contract. At that point the single-source-of-truth value is real; until -then the structural similarity of the two matches is coincidental. diff --git a/docs/adr/0005-composite-action-does-not-check-out.md b/docs/adr/0005-composite-action-does-not-check-out.md new file mode 100644 index 0000000..f8e82d4 --- /dev/null +++ b/docs/adr/0005-composite-action-does-not-check-out.md @@ -0,0 +1,13 @@ +# The composite action does not check out the repository + +`action.yml` sets up `uv` and runs the CLI; it does not run `actions/checkout`, so +`uses: modern-python/semvertag@v0` is not self-contained and the caller owns the checkout step. +Folding one in was rejected because the caller has almost always already checked out, with options +the composite cannot guess: a specific `ref`, a submodule set, LFS objects, a sparse or monorepo +subpath, or a token other than `github.token`. A second checkout would either discard that setup or +fight it, and the failure would be confusing precisely because the step is invisible from the +calling workflow. The established actions in this niche, `mathieudutour/github-tag-action`, +`googleapis/release-please-action` and `cycjimmy/semantic-release-action`, uniformly skip it, so +callers already expect to own it. semvertag reads the head commit and the tag history over the +GitHub API and never touches the working tree, so a folded-in checkout would buy nothing and impose +a `fetch-depth` requirement the tool does not actually have. diff --git a/docs/adr/0005-semver-form-tags-only.md b/docs/adr/0005-semver-form-tags-only.md deleted file mode 100644 index ee08197..0000000 --- a/docs/adr/0005-semver-form-tags-only.md +++ /dev/null @@ -1,45 +0,0 @@ -# Bump baseline is SemVer-form only; prereleases finalize via `next_version` - -**Decision:** `_select_latest_semver_tag` picks the bump baseline from tags parseable by -`semver.Version.parse` — SemVer-form (`MAJOR.MINOR.PATCH`, optionally `-prerelease`/`+build`). Tags -that are not valid SemVer are skipped: **PEP 440 prereleases** (`0.9.0rc1`, `0.8.1a1`) and -**`v`-prefixed** tags (`v1.2.3`). When a SemVer-form *prerelease* (`1.0.0-rc.1`) is the selected -baseline, the new version is computed with `Version.next_version`, which **finalizes** it -(`1.0.0-rc.1` + patch → `1.0.0`), not with `bump_*`, which would jump to `1.0.1`. - -A review of the tag-selection chain found that its *composed* behavior had untested, emergent -semantics. `semver.Version.parse` is strict, so `v1.2.3` and PEP 440 prereleases are silently skipped -from selection; and the old `bump_*` arithmetic on a SemVer-form prerelease baseline jumped past -finalization instead of reaching it. Three options were considered for what the selector should -recognize: SemVer-form only with `next_version` to finalize (chosen); PEP 440 prereleases too -(rejected); `v`-prefixed tags too (deferred). - -semvertag is a **SemVer** tagger: it emits bare `X.Y.Z`, sorts by SemVer precedence, and SemVer-form -is the format it should expect in a repo it manages. - -`next_version` is feasible, dependency-free, and behavior-preserving for every tag that exists today. -On a stable baseline without build metadata it equals `bump_*` exactly; it differs only by finalizing -a SemVer-form prerelease baseline, which is the correct release-ramp semantics and the bug the review -found. The selector strips build metadata before carrying the `Version`, so a hand-pushed -`1.0.0+build` tag — SemVer-valid, precedence-irrelevant — is carried as `1.0.0` and bumps to -`1.0.1`; semvertag never emits build metadata, so stripping is safe. A prerelease baseline also -finalizes on major and minor alike (`1.0.0-rc.1` + major → `1.0.0`, because the lower parts are -already zero) — defensible release-ramp semantics, and dormant because semvertag never self-emits -prereleases. - -**PEP 440 recognition is rejected.** `python-semver` has no PEP 440 parser; its `coerce` recipe -extracts only `major.minor.patch` and *discards* the `rc1`, making a prerelease masquerade as final — -unusable. Real support needs the `packaging` library running *alongside* `semver`, two version models -in one selection path, to recognize a form a SemVer tool should not need to consume. PEP 440 is -semvertag's own PyPI-publishing quirk, not the form of the repos it manages. - -**`v`-prefix recognition is deferred, not rejected.** It is cheap — strip a leading `v`/`V` before -parse — but a distinct *policy* change: semvertag would then *consume* `v`-prefixed tags while still -*emitting* bare semver, a mixed convention worth deciding deliberately. It is a real adoption -footgun, since a repo with `v`-prefixed history sees `NoTags` and never bumps. - -**Revisit trigger:** for PEP 440, users need prerelease tags recognized as bump baselines in managed -repos — at which point adding `packaging` for selection, kept separate from the `semver` bump, is -worth pricing. For the `v` prefix, adoption against `v`-prefixed repos becomes a goal; the fix is a -leading-`v` strip before parse, plus a decision on whether semvertag should then also emit -`v`-prefixed tags. diff --git a/docs/adr/0006-no-doctor-preflight-command.md b/docs/adr/0006-no-doctor-preflight-command.md deleted file mode 100644 index 54fd8c4..0000000 --- a/docs/adr/0006-no-doctor-preflight-command.md +++ /dev/null @@ -1,33 +0,0 @@ -# No `doctor` preflight command - -**Decision:** semvertag ships one verb, `tag`. There is no `doctor` subcommand and no preflight -diagnostic mode. Configuration and permission problems surface from the real run, through the domain -error hierarchy and its exit codes. - -A `doctor` subsystem shipped and was removed pre-1.0. It ran four checks against the configured -forge — token validity, token scopes, project access, protected-tag permission — and mapped each to -a category exit code. It cost about 400 lines of source plus a comparable weight of tests, and it -picked its exit code by matching *string fragments* out of a check's `cause`, with a comment in the -code instructing the maintainer to keep those fragments in lockstep with the provider's wording. - -Three arguments retire it, and they are the reasons not to bring it back. - -The diagnostics were redundant. Every failure `doctor` could name — a rejected token, a missing -scope, an unreachable project, a refused tag creation — already surfaces from the real run as a -typed `AuthError` / `ConfigError` / `ProviderAPIError` with the same exit code and a message that -names the fix. The only thing the preflight added was reporting the failure a few seconds earlier, -and reporting it from a second code path that had to be kept in agreement with the first. - -It taxed the `Provider` protocol. Four `check_*` operations sat on the forge-neutral contract, so -every forge added — GitHub then, Bitbucket later — owed four implementations that existed purely to -re-ask questions the ordinary operations already answer. A smaller protocol is what made the GitHub -provider cheap. - -And the shape is rare for this kind of tool. Focused CLIs — `git`, `kubectl`, `gh`, `aws` — do not -carry one; `doctor` belongs to framework CLIs with many interacting config sources and a large -install surface (Flutter, Homebrew, Hugo). An auto-tagger reading two API endpoints is not that. - -**Revisit trigger:** a failure mode appears that the real run genuinely cannot report — one where the -run succeeds or fails misleadingly and only a dedicated check would have caught it — or the -`Provider` protocol grows operations whose only caller would be a preflight. Either would mean the -redundancy argument no longer holds. Wanting a faster failure is not that trigger. diff --git a/docs/adr/0007-composite-action-does-not-check-out.md b/docs/adr/0007-composite-action-does-not-check-out.md deleted file mode 100644 index 7425c41..0000000 --- a/docs/adr/0007-composite-action-does-not-check-out.md +++ /dev/null @@ -1,24 +0,0 @@ -# The composite action does not check out the repository - -**Decision:** `action.yml` sets up `uv` and runs the CLI. It does **not** run `actions/checkout`. -The caller checks out, and the caller owns `fetch-depth`. - -The tempting alternative is to fold a checkout step in so `uses: modern-python/semvertag@v0` works -on its own. It is rejected because a composite that checks out silently *re-*checks out: the caller -has almost always already done it, with options the composite cannot guess. Real workflows check out -a specific `ref`, a submodule set, LFS objects, a sparse or monorepo subpath, or use a token other -than `github.token`. A second checkout inside the action either discards that setup or fights it, -and the failure is confusing precisely because the step is invisible from the calling workflow. - -The established actions in this niche — `mathieudutour/github-tag-action`, -`googleapis/release-please-action`, `cycjimmy/semantic-release-action` — uniformly skip it, so -callers already expect to own the step. - -The cost is one documented footgun rather than a hidden one: `actions/checkout` defaults to -`fetch-depth: 1`, which misses the tag-relative history, so the README and -`docs/providers/github.md` both call out `fetch-depth: 0`. That is a line in the caller's workflow -they can see and fix, which is the better trade against a checkout they cannot see at all. - -**Revisit trigger:** GitHub gains a way for a composite action to *detect* that the workspace is -already checked out at the ref it needs, so a folded-in checkout could be conditional rather than -unconditional. At that point the convenience is free and the objection disappears. diff --git a/docs/agents/domain.md b/docs/agents/domain.md index ac69cb3..1d45ba2 100644 --- a/docs/agents/domain.md +++ b/docs/agents/domain.md @@ -18,7 +18,7 @@ Single-context repo: ├── CONTEXT.md ├── docs/adr/ │ ├── 0001-forge-providers-not-unified.md -│ └── 0002-blank-settings-values-normalize-to-unset.md +│ └── 0002-outcome-renderings-stay-split.md └── semvertag/ ```