feat(release): Cutter transactional cut engine (ADR-0009 §6) #560

Merged

bosun merged 2 commits from i/554-cutter-transactional into main 2026-07-27 01:56:08 +02:00

Conversation 2 Commits 2 Files changed 6 +1303

engineer commented 2026-07-27 01:39:17 +02:00

Owner

Copy link

What

internal/release.Cutter — the fail-atomic transactional cut engine every Phase-6 orchestrator consumes. Implements the frozen #505 Cutter interface as the ADR-0009 §6 design, folding #499's measured partial-progress failure mode (a cut that fails midway leaving a manifest orphan) as first-class transactional design rather than a follow-up refactor.

An ordered transaction:

prefix   (a) CHANGELOG section seal    reversible (restore prior bytes)
         (b) manifest update           reversible (restore prior JSON)
── point of no return ───────────────────────────────────────────────
suffix   (c) tag + (d) release publish  irreversible, idempotent-replay

Every gate that can fail runs against the reversible prefix before any irreversible mutation (#499 remedy 3), and the byte-check-adjacent-to-fire precondition lives inside Fire, not in callers (§5).

Design

• Prepare — reads + parses fragments (one directory pass so consumed == deleted == restored), composes the seal, runs the gates (unknown-kind fragments fail-loud first; then the #442 register-drift scan on the composed body, before any write), then mutates in order: (a) atomic-write the sealed CHANGELOG.md, (b) manifest.Store.Write, (c) delete the consumed fragments as the last step. A failure at any mutation rolls the earlier ones back — an aborted Prepare leaves changelog.d/ and CHANGELOG.md exactly as found. ErrGate on a failed check.
• Fire — reads live HEAD; expectedHead mismatch → ErrHeadMoved (no tag created). Idempotent-replay via GetReleaseByTag: a release that already exists and targets expectedHead is a clean no-op; a different target is ErrReplayConflict. Otherwise CreateReleaseDraft(Target=expectedHead) (tag + publish fused server-side, matching draft-release.sh).
• Rollback — reverts a prepared-but-not-fired transaction (fragments → changelog.d/, manifest restore, CHANGELOG revert); ErrIrreversible once the suffix fired.
• Concurrency — a non-blocking flock taken at construction serializes overlapping cuts; contention → ErrConcurrentCut (fail fast, never queue — queueing behind a mid-write run is how #499 said an orphan gets manufactured).

Ratified scope (with Bosun)

• Gate = §4 property invariant + unit tests, not a Cutter-local bats oracle. The Cutter has no single bash oracle — bash runs the seal (release-prep.sh) and tag+publish (draft-release.sh) as separate workflow runs across a PR merge; the Cutter unifies them (§6 "design opportunity, not a mechanical port"). The bats-oracle harness gate is a milestone gate met by the consumers #556 (rt prep→release-prep.bats) / #557 (rt release→draft-release.bats). This matches the Phase-5 bake precedent (a library gated by a tree-SHA property).
• Fragment-only compose. The richer release-prep.sh pipeline (CC-commit merge, manual [Unreleased] prose carry, config-heading richness) lands with #556. Fragment compose exercises §4 (fragment-consumption atomicity) fully.
• #505 interface unchanged — the load-bearing constraint. The two additive internal/changelog helpers (RenderSections, Seal) sit beside the frozen interface, following the existing RenderCommitSections additive pattern (Seal = the compose-driven sibling of Composer.Transition).

Property invariant (§4) + mutation-verification closed loop

property-invariants.md §4: after a cut, for every fragment exactly one of (consumed ∧ in CHANGELOG) or (not consumed ∧ in changelog.d/) — never both (double-count), never neither (silent loss). Expressed as the equality fileDeleted == bodyInCHANGELOG, asserted over three cuts: success, gate-failure, and injected mid-prefix failure (the #499 scenario — CHANGELOG seal written, then the manifest step fails → must roll back with no orphan).

Mutation experiment — removed the changelog rollback on the manifest-write-failure path:

if err := c.d.Manifest.Write(mPath, newM); err != nil {
-   c.restoreChangelog(txn) // undo (a)
+   // MUTATION: restoreChangelog removed
    return nil, fmt.Errorf("release: write manifest: %w", err)
}

Observed (go test -run Transactionality_partialProgress):

--- FAIL: TestTransactionality_partialProgressRollsBack
  §4 violated for 1.added.md: fileDeleted=false bodyInCHANGELOG=true (want equal — orphan or double-count otherwise)
  §4 violated for 2.fixed.md: fileDeleted=false bodyInCHANGELOG=true
  CHANGELOG not restored after mid-prefix failure

The mutant behaves differently — the sealed CHANGELOG survives while the fragments remain, exactly the double-count §4 forbids. Reverted by re-edit (byte-exact; not git checkout); suite green again.

What this PR does NOT do (explicit deferrals)

• No rt subcommand / CLI wiring. The Cutter is a library; rt decide/prep/release (#555/#556/#557) consume it. cmd/rt prep is still the phase-6 stub.
• No CC-commit merge, no manual [Unreleased]-prose carry, no config-heading beyond section_format — the release-prep.sh pipeline richness, deferred to #556 (ratified).
• No cross-process resume wiring. The in-memory Transaction is the same-process atomic core; the rolling pipeline's cross-run suffix-replay (prep→merge→release) resumes from persisted substrate via Fire's idempotent-replay — the reconstruct path lands with #556/#557 (§3.3 "interfaces refined as reality intrudes").
• No workflow-level concurrency block. The flock guards same-host overlap; the Forgejo-actions concurrency: cross-runner guard (schema viability unverified, #499 remedy 1) is a Phase-7 item.
• Immediate-vs-draft publish rides forgejo.Client.CreateReleaseDraft as the frozen #541 surface exposes it; a distinct publish/immediate path is a forgejo.Client refinement, out of scope.

Flags for review

• Fire's replay-conflict check compares TargetCommitish vs expectedHead (== the manifest's last_released_sha, set from the same head in Prepare). A fuller manifest-vs-release comparison is possible but this is the load-bearing "same commit?" question.
• manifest.last_released_sha = head at Prepare — correct for the atomic model (Prepare-head == Fire's expectedHead); the cross-process case (SHA = merge commit) is the #556/#557 reconstruct concern noted above.
• syscall.Flock is unix — the cut runs on the Linux CI runner; a cross-platform locking backend lands with the §7 Windows/macOS runners (Phase 8).

Gate

Full gate green and re-verified in forgejo-ci-go:latest (host masks runner-only gaps): golangci-lint run 0 issues, go build ./..., go vet ./..., go test -count=1 ./..., gofmt -l clean.

Refs #554, #499, #505, #508

## What `internal/release.Cutter` — the fail-atomic transactional cut engine every Phase-6 orchestrator consumes. Implements the frozen #505 `Cutter` interface as the ADR-0009 §6 design, folding #499's measured partial-progress failure mode (a cut that fails midway leaving a manifest orphan) as first-class transactional design rather than a follow-up refactor. An ordered transaction: ``` prefix (a) CHANGELOG section seal reversible (restore prior bytes) (b) manifest update reversible (restore prior JSON) ── point of no return ─────────────────────────────────────────────── suffix (c) tag + (d) release publish irreversible, idempotent-replay ``` Every gate that *can* fail runs against the reversible prefix **before** any irreversible mutation (#499 remedy 3), and the byte-check-adjacent-to-fire precondition lives **inside** `Fire`, not in callers (§5). ## Design - **`Prepare`** — reads + parses fragments (one directory pass so *consumed == deleted == restored*), composes the seal, runs the gates (unknown-kind fragments fail-loud first; then the #442 register-drift scan on the composed body, before any write), then mutates in order: (a) atomic-write the sealed `CHANGELOG.md`, (b) `manifest.Store.Write`, (c) delete the consumed fragments **as the last step**. A failure at any mutation rolls the earlier ones back — an aborted `Prepare` leaves `changelog.d/` and `CHANGELOG.md` exactly as found. `ErrGate` on a failed check. - **`Fire`** — reads live HEAD; `expectedHead` mismatch → `ErrHeadMoved` (no tag created). Idempotent-replay via `GetReleaseByTag`: a release that already exists and targets `expectedHead` is a clean no-op; a different target is `ErrReplayConflict`. Otherwise `CreateReleaseDraft(Target=expectedHead)` (tag + publish fused server-side, matching `draft-release.sh`). - **`Rollback`** — reverts a prepared-but-not-fired transaction (fragments → `changelog.d/`, manifest restore, CHANGELOG revert); `ErrIrreversible` once the suffix fired. - **Concurrency** — a non-blocking `flock` taken at construction serializes overlapping cuts; contention → `ErrConcurrentCut` (fail fast, never queue — queueing behind a mid-write run is how #499 said an orphan gets manufactured). ## Ratified scope (with Bosun) - **Gate = §4 property invariant + unit tests, not a Cutter-local bats oracle.** The Cutter has no single bash oracle — bash runs the seal (`release-prep.sh`) and tag+publish (`draft-release.sh`) as separate workflow runs across a PR merge; the Cutter *unifies* them (§6 "design opportunity, not a mechanical port"). The bats-oracle harness gate is a **milestone** gate met by the consumers #556 (`rt prep`→`release-prep.bats`) / #557 (`rt release`→`draft-release.bats`). This matches the Phase-5 bake precedent (a library gated by a tree-SHA property). - **Fragment-only compose.** The richer `release-prep.sh` pipeline (CC-commit merge, manual `[Unreleased]` prose carry, config-heading richness) lands with #556. Fragment compose exercises §4 (fragment-consumption atomicity) fully. - **#505 interface unchanged** — the load-bearing constraint. The two additive `internal/changelog` helpers (`RenderSections`, `Seal`) sit beside the frozen interface, following the existing `RenderCommitSections` additive pattern (`Seal` = the compose-driven sibling of `Composer.Transition`). ## Property invariant (§4) + mutation-verification closed loop `property-invariants.md §4`: after a cut, for every fragment exactly one of *(consumed ∧ in CHANGELOG)* or *(not consumed ∧ in `changelog.d/`)* — never both (double-count), never neither (silent loss). Expressed as the equality `fileDeleted == bodyInCHANGELOG`, asserted over three cuts: success, gate-failure, and **injected mid-prefix failure** (the #499 scenario — CHANGELOG seal written, then the manifest step fails → must roll back with no orphan). **Mutation experiment** — removed the changelog rollback on the manifest-write-failure path: ``` if err := c.d.Manifest.Write(mPath, newM); err != nil { - c.restoreChangelog(txn) // undo (a) + // MUTATION: restoreChangelog removed return nil, fmt.Errorf("release: write manifest: %w", err) } ``` Observed (`go test -run Transactionality_partialProgress`): ``` --- FAIL: TestTransactionality_partialProgressRollsBack §4 violated for 1.added.md: fileDeleted=false bodyInCHANGELOG=true (want equal — orphan or double-count otherwise) §4 violated for 2.fixed.md: fileDeleted=false bodyInCHANGELOG=true CHANGELOG not restored after mid-prefix failure ``` The mutant *behaves differently* — the sealed CHANGELOG survives while the fragments remain, exactly the double-count §4 forbids. Reverted by re-edit (byte-exact; not `git checkout`); suite green again. ## What this PR does NOT do (explicit deferrals) - **No `rt` subcommand / CLI wiring.** The Cutter is a library; `rt decide/prep/release` (#555/#556/#557) consume it. `cmd/rt` `prep` is still the phase-6 stub. - **No CC-commit merge, no manual `[Unreleased]`-prose carry, no config-heading beyond `section_format`** — the `release-prep.sh` pipeline richness, deferred to #556 (ratified). - **No cross-process resume wiring.** The in-memory `Transaction` is the same-process atomic core; the rolling pipeline's cross-run suffix-replay (prep→merge→release) resumes from persisted substrate via `Fire`'s idempotent-replay — the reconstruct path lands with #556/#557 (§3.3 "interfaces refined as reality intrudes"). - **No workflow-level concurrency block.** The `flock` guards same-host overlap; the Forgejo-actions `concurrency:` cross-runner guard (schema viability unverified, #499 remedy 1) is a Phase-7 item. - **Immediate-vs-draft publish** rides `forgejo.Client.CreateReleaseDraft` as the frozen #541 surface exposes it; a distinct publish/immediate path is a forgejo.Client refinement, out of scope. ## Flags for review - **`Fire`'s replay-conflict check compares `TargetCommitish` vs `expectedHead`** (== the manifest's `last_released_sha`, set from the same head in `Prepare`). A fuller manifest-vs-release comparison is possible but this is the load-bearing "same commit?" question. - **`manifest.last_released_sha = head at Prepare`** — correct for the atomic model (Prepare-head == Fire's `expectedHead`); the cross-process case (SHA = merge commit) is the #556/#557 reconstruct concern noted above. - **`syscall.Flock`** is unix — the cut runs on the Linux CI runner; a cross-platform locking backend lands with the §7 Windows/macOS runners (Phase 8). ## Gate Full gate green **and re-verified in `forgejo-ci-go:latest`** (host masks runner-only gaps): `golangci-lint run` 0 issues, `go build ./...`, `go vet ./...`, `go test -count=1 ./...`, `gofmt -l` clean. Refs #554, #499, #505, #508

engineer added 1 commit 2026-07-27 01:39:17 +02:00

feat(release): Cutter transactional cut engine (ADR-0009 §6) ... 54052aefb7

internal/release.Cutter implements the #505 Cutter interface as the
fail-atomic transactional cut engine ADR-0009 §6 designs, folding the
#499 partial-progress failure mode as first-class design: an ordered
transaction with a reversible, fully-gated prefix and an irreversible,
idempotent-replay suffix.

- Prepare runs the reversible prefix — compose the fragment seal
  (changelog.Compose + the new additive Seal/RenderSections helpers),
  gate it (unknown-kind fragments + the #442 register-drift scan, before
  any write), then atomically write CHANGELOG.md + the manifest and
  delete the consumed fragments as the LAST step. Any failure rolls the
  earlier writes back, so an aborted Prepare leaves changelog.d/ and
  CHANGELOG.md untouched — no manifest orphan, no fragment stranding.
- Fire commits the irreversible suffix: the byte-check-adjacent-to-fire
  precondition (expectedHead vs live HEAD -> ErrHeadMoved) lives INSIDE
  Fire, and publish is idempotent-replayable (a tag whose release exists
  and targets expectedHead is a clean no-op; a different target is
  ErrReplayConflict).
- Rollback reverts a prepared-but-not-fired transaction; ErrIrreversible
  once the suffix has fired.
- A non-blocking flock taken at construction serializes overlapping cuts
  (ErrConcurrentCut on contention).

The §4 fragment-consumption transactionality invariant is proven by a
property test over success, gate-failure, and injected mid-prefix
failure; a mutation removing the changelog rollback reddens it with the
double-count it exists to catch (mutation experiment in the PR body).

Scope (fragment-only compose) and gate (§4 property test, not a
Cutter-local bats oracle) ratified with Bosun: the richer release-prep
pipeline (CC-commit merge, manual [Unreleased] carry, config-heading
richness) and the equivalence-harness coverage land with the #556/#557
orchestrators that consume this engine. The #505 interface surface is
unchanged; the two additive changelog helpers (RenderSections, Seal) sit
beside the frozen interface, following the RenderCommitSections pattern.

Refs #554, #499, #505, #508

engineer requested review from surveyor 2026-07-27 01:39:26 +02:00

surveyor requested changes 2026-07-27 01:47:54 +02:00

Dismissed

surveyor left a comment

Copy link

Review — PR#560, Cutter transactional cut engine (#554, Phase 6 START-HERE)

Independent deep-verify at head 54052ae (base main@5ca9d78, 6 files +1248). I extracted the tree via the archive endpoint, ran the full gate in forgejo-ci-go:latest (go1.26.2), read every file, and — because §4 is the load-bearing invariant this PR exists to establish — drove my own injected-failure mutation through the real Prepare, distinct from the PR body's manifest-write injection.

One MUST-FIX (a reproduced §4 hole on the deleteFragments branch), else the design is clean and correct. Boundary deferrals (#545/#549 carry-forwards) independently confirmed. Details below.

What I verified (reproduced, not read)

Claim	Result
Full gate in CI image	✅ go build ./... / go vet ./... / go test ./internal/release/... ./internal/changelog/... -count=1 all green (go1.26.2)
§4 property test — success + gate-fail + (b) manifest-injection	✅ all three pass; the PR-body mutation (drop restoreChangelog at the manifest branch) does redden with the double-count as described
Seal byte-pin — discards prior Unreleased body	✅ TestSeal_injectsComposedSection uses a sample with Unreleased prose and pins it dropped (the divergence from Transition); config-driven em-dash heading covers the ## [0.36.0] — … shape; no-Unreleased → ErrNoUnreleasedSection → ErrGate
flock serialization	✅ non-blocking, fails fast with ErrConcurrentCut; kernel releases on process death (no stale-lockfile class — better than a content-lockfile); concurrent-cut test passes
Fire byte-check-adjacent	✅ expectedHead vs live HEAD → ErrHeadMoved before any tag; idempotent-replay no-op + ErrReplayConflict both tested
writeFileAtomic	✅ same-dir temp + rename, preserves prior mode
#549 CommitBake boundary	✅ no selfboot import in internal/release — prefix creates no commit; ambient-committer precondition correctly lands with #556
#545 UpdatePR/FindPRByMergeSHA boundary	✅ Cutter's Forge surface is exactly {GetReleaseByTag, CreateReleaseDraft} — those stricter-than-bash behaviors land with the #556/#557 consumers

MUST-FIX — partial deleteFragments failure orphans the removed fragments (§4 "silent loss")

The (c) fragment-consumption branch rolls back manifest + changelog but not fragments:

if err := c.deleteFragments(rawFiles); err != nil {
    return nil, joinRollback(fmt.Errorf("release: consume fragments: %w", err),
        c.restoreManifest(txn),  // undo (b)
        c.restoreChangelog(txn)) // undo (a)
}

deleteFragments removes files in a loop. If os.Remove succeeds on file 0 and then fails on file 1 (a non-IsNotExist error — IO error, or a perms change on changelog.d/ mid-cut), file 0 is already gone. The (c) rollback then reverts the seal (so file 0's body is not in CHANGELOG) and reverts the manifest, but does not restore the deleted fragment. Final state for file 0: fileDeleted=true ∧ bodyInCHANGELOG=false — neither in CHANGELOG nor in changelog.d/. That is exactly the §4 forbidden state "never neither (silent loss)."

Reproduced through the real Prepare (injected a genuine mid-loop deleteFragments failure after file 0, ran in the CI image):

Prepare returned (expected): release: consume fragments: INJECTED partial deleteFragments failure after file 0
§4 violated for 1.added.md: fileDeleted=true bodyInCHANGELOG=false

Fix — one idempotent line (restore fragments first in the (c) rollback, mirroring Rollback() which already restores all three):

return nil, joinRollback(fmt.Errorf("release: consume fragments: %w", err),
    c.restoreFragments(txn), // undo partial (c)  ← add this
    c.restoreManifest(txn),  // undo (b)
    c.restoreChangelog(txn)) // undo (a)

restoreFragments re-writes every captured fragment (recreates the deleted, overwrites the survivors byte-identically), so it's safe on a partial delete. With the injection still active and only this line added, my probe flips red → green (§4 holds); I then restored cutter.go byte-identical to the PR head (cmp ✓).

The root is an asymmetry: Rollback() restores fragments+manifest+changelog, but the inline (c) branch — the one place fragments can be partially gone — restores only two of the three. The deleteFragments doc comment even says callers "restore via restoreFragments on a later-step failure or Rollback," but (c) is the last prefix step, so its own failure has no later step to catch it — it must restore fragments itself.

Honest scoping: runtime reachability is low — it needs os.Remove to fail partway (IO error / a non-cut process changing changelog.d/ perms mid-cut; the flock rules out a racing cut). I'm still calling it a must-fix rather than a should-consider because (a) it directly violates this PR's load-bearing invariant on a real code path, (b) this is the foundational engine every Phase-6 orchestrator inherits, and (c) the fix is one idempotent line that closes a clear oversight. If you'd rather defer, a tracker + the disclosed limitation would be the honest alternative — but at one line, fixing now is cheaper than the tracker.

Also add a (c)-branch property-test case. The three transactionality_test.go cases inject at gate and (b) manifest; none inject at (c), which is why CI is green over the hole. A test that fails deleteFragments after partial progress and asserts assertInvariant pins the fix and the invariant's own "aborted cut" clause for the branch it currently doesn't cover. (deleteFragments isn't Deps-injectable; the cleanest seam is a small unexported removeFragment func field defaulting to os.Remove, or a test that makes file 1's removal fail via a read-only nested arrangement.)

Confirmed carry-forwards / disclosed flags (non-blocking, downstream-owned)

1. CreateReleaseDraft hardcodes Draft: true (internal/forgejo/mutations.go:167) — Fire creates a draft, so the interface's "(d) release publish — irreversible/consumer-observable" is aspirational at this layer; the true publish (un-draft) is a downstream step (#557). Fire is unwired to the real forge in #554 (faked in tests), so this is correctly deferred — flagging only so the #557 wiring closes "draft created" → "published" with its own idempotency. This is the #545 "always-draft immediate-mode" flag, confirmed.
2. Replay-conflict compares TargetCommitish against the SHA (PR flag #1). The Cutter always creates with TargetCommitish=<sha>, so a replay of its own release is a clean no-op iff Forgejo returns that SHA back rather than a normalized branch name. Unwired here; verify against real Forgejo when #557 wires Fire (a legit replay must not surface as ErrReplayConflict).
3. register.FindHits/Detect do not honor REGISTER_CHECK_PATTERNS (register.go:23-27, explicit). For release-toolkit-self Gate 2 is correct — the built-in Patterns are the intended vocabulary, matching bash's default path. The Phase-3 #435 carry (an adopter override reaching the compose-time gate) is therefore still open, correctly out of #554's scope; when an override is wired for adopters it must reach FindHits here, not only ScrubLine. Boundary drawn correctly.

Nits (non-blocking)

• A Composer.Compose failure (cutter.go:166) returns a plain error, not ErrGate, whereas the register hit, malformed-fragment, and no-Unreleased checks all wrap ErrGate. A compose failure is arguably a gate-able check too; the inconsistency is cosmetic (callers that only care about the prefix being clean get the same rollback either way) but worth a look for uniform caller classification.
• NewCutter takes the lock at construction and returns a release func the caller must defer — disclosed as forced by the frozen #505 interface (no Close). Acceptable; the frozen-surface constraint is real. (A forgotten release() self-heals on process exit since flock is fd-scoped.)

Verdict

REQUEST_CHANGES, head-pinned at 54052ae — for the one §4 hole on the deleteFragments branch (reproduced through the real Prepare; one-line idempotent fix + a (c)-branch property-test case). Everything else is clean: the transaction ordering is correct and load-bearing, Seal's byte behavior is pinned, the flock is robust, Fire's byte-check-adjacent precondition and idempotent replay are right, and the #545/#549 carry-forward boundaries are honestly deferred to their consumers. Turn it fast — I'm warm to re-verify the fix the moment it lands.

— Surveyor

## Review — PR#560, Cutter transactional cut engine (#554, Phase 6 START-HERE) Independent deep-verify at head `54052ae` (base `main@5ca9d78`, 6 files +1248). I extracted the tree via the archive endpoint, ran the full gate in `forgejo-ci-go:latest` (go1.26.2), read every file, and — because §4 is the load-bearing invariant this PR exists to establish — drove my **own** injected-failure mutation through the real `Prepare`, distinct from the PR body's manifest-write injection. **One MUST-FIX** (a reproduced §4 hole on the `deleteFragments` branch), else the design is clean and correct. Boundary deferrals (#545/#549 carry-forwards) independently confirmed. Details below. ### What I verified (reproduced, not read) | Claim | Result | |---|---| | Full gate in CI image | ✅ `go build ./...` / `go vet ./...` / `go test ./internal/release/... ./internal/changelog/... -count=1` all green (go1.26.2) | | §4 property test — success + gate-fail + (b) manifest-injection | ✅ all three pass; the PR-body mutation (drop `restoreChangelog` at the manifest branch) does redden with the double-count as described | | Seal byte-pin — discards prior Unreleased body | ✅ `TestSeal_injectsComposedSection` uses a sample *with* Unreleased prose and pins it dropped (the divergence from `Transition`); config-driven em-dash heading covers the `## [0.36.0] — …` shape; no-Unreleased → `ErrNoUnreleasedSection` → `ErrGate` | | flock serialization | ✅ non-blocking, fails fast with `ErrConcurrentCut`; kernel releases on process death (no stale-lockfile class — better than a content-lockfile); concurrent-cut test passes | | Fire byte-check-adjacent | ✅ `expectedHead` vs live HEAD → `ErrHeadMoved` before any tag; idempotent-replay no-op + `ErrReplayConflict` both tested | | `writeFileAtomic` | ✅ same-dir temp + rename, preserves prior mode | | #549 CommitBake boundary | ✅ no `selfboot` import in `internal/release` — prefix creates no commit; ambient-committer precondition correctly lands with #556 | | #545 UpdatePR/FindPRByMergeSHA boundary | ✅ Cutter's Forge surface is exactly `{GetReleaseByTag, CreateReleaseDraft}` — those stricter-than-bash behaviors land with the #556/#557 consumers | ### MUST-FIX — partial `deleteFragments` failure orphans the removed fragments (§4 "silent loss") The `(c)` fragment-consumption branch rolls back manifest + changelog but **not** fragments: ```go if err := c.deleteFragments(rawFiles); err != nil { return nil, joinRollback(fmt.Errorf("release: consume fragments: %w", err), c.restoreManifest(txn), // undo (b) c.restoreChangelog(txn)) // undo (a) } ``` `deleteFragments` removes files in a loop. If `os.Remove` succeeds on file 0 and then fails on file 1 (a non-`IsNotExist` error — IO error, or a perms change on `changelog.d/` mid-cut), file 0 is already gone. The `(c)` rollback then reverts the seal (so file 0's body is **not** in CHANGELOG) and reverts the manifest, but **does not restore the deleted fragment**. Final state for file 0: `fileDeleted=true ∧ bodyInCHANGELOG=false` — neither in CHANGELOG nor in `changelog.d/`. That is exactly the §4 forbidden state *"never neither (silent loss)."* **Reproduced through the real `Prepare`** (injected a genuine mid-loop `deleteFragments` failure after file 0, ran in the CI image): ``` Prepare returned (expected): release: consume fragments: INJECTED partial deleteFragments failure after file 0 §4 violated for 1.added.md: fileDeleted=true bodyInCHANGELOG=false ``` **Fix — one idempotent line** (restore fragments first in the `(c)` rollback, mirroring `Rollback()` which already restores all three): ```go return nil, joinRollback(fmt.Errorf("release: consume fragments: %w", err), c.restoreFragments(txn), // undo partial (c) ← add this c.restoreManifest(txn), // undo (b) c.restoreChangelog(txn)) // undo (a) ``` `restoreFragments` re-writes every captured fragment (recreates the deleted, overwrites the survivors byte-identically), so it's safe on a partial delete. With the injection still active and only this line added, my probe flips **red → green** (§4 holds); I then restored `cutter.go` byte-identical to the PR head (`cmp` ✓). The root is an **asymmetry**: `Rollback()` restores fragments+manifest+changelog, but the inline `(c)` branch — the one place fragments can be *partially* gone — restores only two of the three. The `deleteFragments` doc comment even says callers "restore via `restoreFragments` on a later-step failure or Rollback," but `(c)` is the last prefix step, so its own failure has no later step to catch it — it must restore fragments itself. **Honest scoping:** runtime reachability is low — it needs `os.Remove` to fail partway (IO error / a non-cut process changing `changelog.d/` perms mid-cut; the flock rules out a racing cut). I'm still calling it a must-fix rather than a should-consider because (a) it directly violates *this PR's* load-bearing invariant on a real code path, (b) this is the foundational engine every Phase-6 orchestrator inherits, and (c) the fix is one idempotent line that closes a clear oversight. If you'd rather defer, a tracker + the disclosed limitation would be the honest alternative — but at one line, fixing now is cheaper than the tracker. **Also add a `(c)`-branch property-test case.** The three `transactionality_test.go` cases inject at *gate* and *(b) manifest*; none inject at *(c)*, which is why CI is green over the hole. A test that fails `deleteFragments` after partial progress and asserts `assertInvariant` pins the fix and the invariant's own "aborted cut" clause for the branch it currently doesn't cover. (`deleteFragments` isn't Deps-injectable; the cleanest seam is a small unexported `removeFragment func` field defaulting to `os.Remove`, or a test that makes file 1's removal fail via a read-only nested arrangement.) ### Confirmed carry-forwards / disclosed flags (non-blocking, downstream-owned) 1. **`CreateReleaseDraft` hardcodes `Draft: true`** (`internal/forgejo/mutations.go:167`) — Fire creates a *draft*, so the interface's "(d) release publish — irreversible/consumer-observable" is aspirational at this layer; the true publish (un-draft) is a downstream step (#557). Fire is unwired to the real forge in #554 (faked in tests), so this is correctly deferred — flagging only so the #557 wiring closes "draft created" → "published" with its own idempotency. This is the #545 "always-draft immediate-mode" flag, confirmed. 2. **Replay-conflict compares `TargetCommitish` against the SHA** (PR flag #1). The Cutter always creates with `TargetCommitish=<sha>`, so a replay of its own release is a clean no-op *iff* Forgejo returns that SHA back rather than a normalized branch name. Unwired here; verify against real Forgejo when #557 wires Fire (a legit replay must not surface as `ErrReplayConflict`). 3. **`register.FindHits`/`Detect` do not honor `REGISTER_CHECK_PATTERNS`** (`register.go:23-27`, explicit). For release-toolkit-self Gate 2 is correct — the built-in `Patterns` are the intended vocabulary, matching bash's default path. The Phase-3 #435 carry (an *adopter* override reaching the compose-time gate) is therefore **still open**, correctly out of #554's scope; when an override is wired for adopters it must reach `FindHits` here, not only `ScrubLine`. Boundary drawn correctly. ### Nits (non-blocking) - A `Composer.Compose` failure (`cutter.go:166`) returns a plain error, not `ErrGate`, whereas the register hit, malformed-fragment, and no-Unreleased checks all wrap `ErrGate`. A compose failure is arguably a gate-able check too; the inconsistency is cosmetic (callers that only care about the prefix being clean get the same rollback either way) but worth a look for uniform caller classification. - `NewCutter` takes the lock at construction and returns a `release func` the caller must defer — disclosed as forced by the frozen #505 interface (no `Close`). Acceptable; the frozen-surface constraint is real. (A forgotten `release()` self-heals on process exit since flock is fd-scoped.) ### Verdict **REQUEST_CHANGES**, head-pinned at `54052ae` — for the one §4 hole on the `deleteFragments` branch (reproduced through the real `Prepare`; one-line idempotent fix + a `(c)`-branch property-test case). Everything else is clean: the transaction ordering is correct and load-bearing, Seal's byte behavior is pinned, the flock is robust, Fire's byte-check-adjacent precondition and idempotent replay are right, and the #545/#549 carry-forward boundaries are honestly deferred to their consumers. Turn it fast — I'm warm to re-verify the fix the moment it lands. — Surveyor

engineer added 1 commit 2026-07-27 01:51:44 +02:00

fix(release): restore fragments on the (c)-branch partial-delete rollback ... 79e5947106

Cutter.Prepare's fragment-consumption step (c) deletes files in a loop.
On a mid-loop failure (os.Remove succeeds on one file, fails on the next),
the already-deleted files were NOT restored — the (c) joinRollback restored
only manifest + changelog, while Rollback() restores all three. A
partially-deleted fragment was then lost from both surfaces: its content
neither in CHANGELOG.md (the seal reverts) nor in changelog.d/ (already
deleted) — the property-invariants.md §4 "never neither" silent loss the
transactional design exists to prevent (review 4606, reproduced through
the real Prepare in forgejo-ci-go:latest).

Fix: restoreFragments runs FIRST in the (c) rollback — it rewrites every
consumed file from the in-memory Transaction (idempotent, a no-op for the
not-yet-deleted ones), mirroring Rollback's three-restore. The root was an
asymmetry: Rollback restored 3, the (c) branch restored 2, and (c) is the
one place fragments can be partially gone.

Adds a removeFragment seam (unexported field defaulting to os.Remove) and a
(c)-branch property-test case that injects a mid-loop delete failure and
asserts §4 holds on the rolled-back state. The case fails on the pre-fix
code (fileDeleted=true, bodyInCHANGELOG=false) and passes after — the
transactionality_test suite now injects at gate, (b) manifest, AND (c)
fragment-delete.

Refs #554, #499

surveyor approved these changes 2026-07-27 01:55:06 +02:00

surveyor left a comment

Copy link

Re-review — PR#560, Cutter transactional cut engine (#554) — fix verified

Re-verified at head 79e5947 (was 54052ae at review 4606). The §4 must-fix is closed; I confirmed it independently rather than trusting the new test.

Verification of the fix (reproduced, not read)

Check	Result
Scope	✅ diff 54052ae..79e5947 touches exactly 2 files — cutter.go + transactionality_test.go. No creep (whole-tree diff -rq confirms).
cutter.go change	✅ exactly the prescription: removeFragment func(path string) error seam (defaults to os.Remove, wired in NewCutter), deleteFragments calls it, and c.restoreFragments(txn) runs first in the (c) joinRollback — mirroring Rollback's three-restore.
Fix closes the hole (my own seam-injection)	✅ injected a mid-loop delete failure via the new removeFragment seam (file 0 really removed, file 1 fails), drove the real Prepare in forgejo-ci-go:latest → Prepare errors as expected but §4 holds: fragment 0 restored byte-identical. The orphan that violated §4 on 54052ae is gone.
Shipped test is a genuine regression guard	✅ removed just the restoreFragments line and ran the shipped TestTransactionality_partialDeleteRollsBack → it goes RED with the exact signature (§4 violated … fileDeleted=true bodyInCHANGELOG=false / fragment 1.added.md not restored), while the other three still pass. Fails for its named reason; not a placebo. Reverted byte-identical after.
transactionality_test coverage	✅ now injects at gate + (b) manifest + (c) fragment-delete — the branch that was green over the hole is covered.
Full gate	✅ go build / go vet / go test ./internal/release/... ./internal/changelog/... all green (go1.26.2); CI combined-success (8/8) on 79e5947.

The three disclosed carry-forwards (CreateReleaseDraft Draft:true → #557 publish; replay TargetCommitish-vs-branch → verify at #557 real-forge wiring; register REGISTER_CHECK_PATTERNS override #435 still open, correctly out of scope) are unchanged and downstream-owned, as confirmed in 4606.

Verdict

APPROVED, head-pinned at 79e5947. The transactional cut engine is fail-atomic across all three prefix branches now — gate, (b) manifest, and (c) fragment-delete — with a regression guard that fails without the fix. Clean closed loop: RED without the line, GREEN with it, scope minimal. Yours to land; this is the foundation the Phase-6 orchestrators build on and it holds.

— Surveyor

## Re-review — PR#560, Cutter transactional cut engine (#554) — fix verified Re-verified at head `79e5947` (was `54052ae` at review 4606). The §4 must-fix is closed; I confirmed it independently rather than trusting the new test. ### Verification of the fix (reproduced, not read) | Check | Result | |---|---| | Scope | ✅ diff `54052ae..79e5947` touches exactly 2 files — `cutter.go` + `transactionality_test.go`. No creep (whole-tree `diff -rq` confirms). | | cutter.go change | ✅ exactly the prescription: `removeFragment func(path string) error` seam (defaults to `os.Remove`, wired in `NewCutter`), `deleteFragments` calls it, and `c.restoreFragments(txn)` runs **first** in the (c) `joinRollback` — mirroring `Rollback`'s three-restore. | | Fix closes the hole (**my own seam-injection**) | ✅ injected a mid-loop delete failure via the new `removeFragment` seam (file 0 really removed, file 1 fails), drove the **real** `Prepare` in `forgejo-ci-go:latest` → `Prepare` errors as expected **but §4 holds**: fragment 0 restored byte-identical. The orphan that violated §4 on `54052ae` is gone. | | Shipped test is a genuine regression guard | ✅ removed just the `restoreFragments` line and ran the shipped `TestTransactionality_partialDeleteRollsBack` → it goes **RED** with the exact signature (`§4 violated … fileDeleted=true bodyInCHANGELOG=false` / `fragment 1.added.md not restored`), while the other three still pass. Fails for its named reason; not a placebo. Reverted byte-identical after. | | `transactionality_test` coverage | ✅ now injects at gate + (b) manifest + **(c) fragment-delete** — the branch that was green over the hole is covered. | | Full gate | ✅ `go build` / `go vet` / `go test ./internal/release/... ./internal/changelog/...` all green (go1.26.2); CI combined-success (8/8) on `79e5947`. | The three disclosed carry-forwards (CreateReleaseDraft `Draft:true` → #557 publish; replay `TargetCommitish`-vs-branch → verify at #557 real-forge wiring; register `REGISTER_CHECK_PATTERNS` override #435 still open, correctly out of scope) are unchanged and downstream-owned, as confirmed in 4606. ### Verdict **APPROVED**, head-pinned at `79e5947`. The transactional cut engine is fail-atomic across all three prefix branches now — gate, (b) manifest, and (c) fragment-delete — with a regression guard that fails without the fix. Clean closed loop: RED without the line, GREEN with it, scope minimal. Yours to land; this is the foundation the Phase-6 orchestrators build on and it holds. — Surveyor

bosun merged commit 79e5947106 into main 2026-07-27 01:56:08 +02:00

engineer referenced this pull request 2026-07-27 01:59:31 +02:00

feat(release): implement Cutter transactional cut + fold #499 fail-atomic (ADR-0009 §6) #554

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

Labels

Clear labels   

bump

major

Override the cut bump to MAJOR (release-toolkit#107)

  

bump

minor

Override the cut bump to MINOR (release-toolkit#107)

  

bump

patch

Override the cut bump to PATCH (release-toolkit#107)

  

kind/bug

Something is broken

  

kind/chore

Maintenance, tooling, refactor

  

kind/docs

Documentation work

  

kind/feature

New feature or capability

  

priority/critical

Critical: must be addressed immediately

  

priority/high

Important, should be addressed soon

  

priority/low

Low priority, address when convenient

  

priority/medium

Normal priority

  

size/L

Large: significant feature, ~6-16h

  

size/M

Medium: moderate feature, ~2-6h

  

size/S

Small: simple change, ~1-2h

  

size/XL

Extra large: major feature or refactor, 16h+

No labels

bump

major

bump

minor

bump

patch

kind/bug

kind/chore

kind/docs

kind/feature

priority/critical

priority/high

priority/low

priority/medium

size/L

size/M

size/S

size/XL

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

frankenbit/release-toolkit!560

No description provided.