heiko/gogogo
1
0
Fork 0
Code Issues 17 Pull requests 1 Projects Releases 42 Packages Wiki Activity Actions

publish: revision tracking, --replace semantics, signed receipt tags #63

Merged
heiko merged 9 commits from dev into master 2026-05-20 14:24:01 +02:00
Conversation 3 Commits 9 Files changed 31 +1552 -267
heiko commented 2026-05-19 23:38:21 +02:00
Owner
Copy link

Implements end-to-end Debian packaging-revision support and reshapes the publish flow's UX around it. Closes #61 fully (the working tree had quietly re-introduced the per-dist staging that caused the original bug).

Summary

Foundation (f51727c):

  • Receipt-tag model deb/v<X>-<N> written at the packaged commit; next publish auto-increments via build.HighestDebRevision.
  • pack.DebVersion accepts a revision (NoRevision = -1 keeps native semantics); source pkg switches to 3.0 (quilt) with .orig.tar.gz when revisioned.
  • dupload .changes file space-joins all configured distributions in a single upload (re-fixes the regression).
  • orig.tar.gz is byte-identical across rebuilds of the same commit: CommitTime threaded as gzip/tar ModTime; uid/gid/uname/gname zeroed; .git/ excluded alongside debian/.
  • DefaultDebDistribution const extracted; FD leak in createOrigTarball fixed.

Publish UX (f51727c):

  • --force renamed to --replace, split per transport. Forgejo: delete release + tag, recreate. Debian: pool cannot replace, revision auto-bumps with an explicit warning.
  • Graceful 409 on PostRelease: existing release is reused, conflicting asset names are warned and skipped (use --replace to overwrite).
  • Auto-prune: obsolete revision .deb/.dsc/.debian.tar.gz are removed from the existing release on each successful publish.
  • Deb receipt tags are now annotated, GPG-signed (message: gogogo: debian packaging revision N for v).
  • README documents the new model end-to-end under Re-publishing and Debian revisions.

Simplification (8aa37da):

  • pruneOldDebRevisions converted to method on *Release (5 params dropped, all derivable from receiver/ri).
  • Latent footgun fix: kept := ri.Assets[:0] aliasing the caller's backing array → fresh make() allocation.
  • Redundant bumpedDebRevision field removed (derivable from r.debRevision > 1).
  • Nested errors.As chain in PostRelease flattened into one type-assert + switch.
  • CreateSignedTag pre-checks tag existence so real signing failures aren't swallowed by the idempotency branch.
  • Shared build.DebPackagingTag helper keeps the writer and ParseDebPackagingTag in lockstep on the format.
  • Unreachable 409 fallback now wraps the underlying GET error or names the race-condition case explicitly.

Process

Two /simplify passes ran during development — one mid-session on the foundation work (rolled into f51727c), one on the new code in this PR (now its own commit 8aa37da). All findings are applied; behavior is unchanged across the simplify commit.

Test plan

  • go build ./... clean
  • go test ./... — all 27 packages green (both before and after the simplify commit)
  • Live publish against the apt server: bookworm trixie joined .changes accepted by reprepro into both distributions atomically
  • Live publish against the Forgejo release: graceful conflict warnings emitted, per-blob skip working, no error on existing release
  • Signed receipt tag verified: git cat-file -t deb/v0.0.13-2 returns tag, git tag -v shows Good signature

Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com

Implements end-to-end Debian packaging-revision support and reshapes the publish flow's UX around it. Closes #61 fully (the working tree had quietly re-introduced the per-dist staging that caused the original bug). ## Summary **Foundation (f51727c):** - Receipt-tag model `deb/v<X>-<N>` written at the packaged commit; next publish auto-increments via `build.HighestDebRevision`. - `pack.DebVersion` accepts a revision (`NoRevision = -1` keeps native semantics); source pkg switches to `3.0 (quilt)` with `.orig.tar.gz` when revisioned. - `dupload` `.changes` file space-joins all configured distributions in a single upload (re-fixes the regression). - `orig.tar.gz` is byte-identical across rebuilds of the same commit: `CommitTime` threaded as gzip/tar `ModTime`; uid/gid/uname/gname zeroed; `.git/` excluded alongside `debian/`. - `DefaultDebDistribution` const extracted; FD leak in `createOrigTarball` fixed. **Publish UX (f51727c):** - `--force` renamed to `--replace`, split per transport. Forgejo: delete release + tag, recreate. Debian: pool cannot replace, revision auto-bumps with an explicit warning. - Graceful 409 on `PostRelease`: existing release is reused, conflicting asset names are warned and skipped (use `--replace` to overwrite). - Auto-prune: obsolete revision `.deb`/`.dsc`/`.debian.tar.gz` are removed from the existing release on each successful publish. - Deb receipt tags are now annotated, GPG-signed (message: *gogogo: debian packaging revision N for v<X>*). - README documents the new model end-to-end under *Re-publishing and Debian revisions*. **Simplification (8aa37da):** - `pruneOldDebRevisions` converted to method on `*Release` (5 params dropped, all derivable from receiver/ri). - Latent footgun fix: `kept := ri.Assets[:0]` aliasing the caller's backing array → fresh `make()` allocation. - Redundant `bumpedDebRevision` field removed (derivable from `r.debRevision > 1`). - Nested `errors.As` chain in `PostRelease` flattened into one type-assert + switch. - `CreateSignedTag` pre-checks tag existence so real signing failures aren't swallowed by the idempotency branch. - Shared `build.DebPackagingTag` helper keeps the writer and `ParseDebPackagingTag` in lockstep on the format. - Unreachable 409 fallback now wraps the underlying GET error or names the race-condition case explicitly. ## Process Two `/simplify` passes ran during development — one mid-session on the foundation work (rolled into f51727c), one on the new code in this PR (now its own commit 8aa37da). All findings are applied; behavior is unchanged across the simplify commit. ## Test plan - [x] `go build ./...` clean - [x] `go test ./...` — all 27 packages green (both before and after the simplify commit) - [x] Live publish against the apt server: `bookworm trixie` joined `.changes` accepted by reprepro into both distributions atomically - [x] Live publish against the Forgejo release: graceful conflict warnings emitted, per-blob skip working, no error on existing release - [x] Signed receipt tag verified: `git cat-file -t deb/v0.0.13-2` returns *tag*, `git tag -v` shows *Good signature* Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`distributions: [bookworm, trixie]` now produces `Distribution: bookworm trixie`
in the .changes file (and the changelog entry), so dupload uploads to both targets.

Closes #61
Implements end-to-end Debian packaging-revision support and reshapes
the publish flow's UX around it. The work has two halves, separated
by a /simplify pass; both halves share these commits to keep
overlapping files coherent.

Foundation (post-/simplify):
* receipt-tag model deb/v<X>-<N>: stored at the packaged commit,
  next publish auto-increments via build.HighestDebRevision
* pack: DebVersion accepts a revision (NoRevision = -1 keeps native
  semantics); source pkg switches to 3.0 (quilt) with .orig.tar.gz
  when revisioned
* dupload .changes file space-joins all configured distributions in
  a single upload (regression in working tree had reintroduced #61)
* orig.tar.gz is byte-identical across rebuilds of the same commit:
  CommitTime threaded as gzip/tar ModTime, uid/gid/uname/gname
  zeroed, .git/ excluded alongside debian/
* DefaultDebDistribution const extracted to internal/config; FD
  leak in createOrigTarball fixed (defer in WalkDir callback)

Publish UX:
* --force renamed to --replace and split per transport:
  forgejo = delete release + tag and recreate; debian = pool cannot
  replace, revision auto-bumps with explicit warning
* graceful 409 on PostRelease: existing release is reused, conflicting
  asset names are warned and skipped (use --replace to overwrite)
* auto-prune: obsolete revision .deb/.dsc/.debian.tar.gz are removed
  from the existing release on each successful publish
* deb receipt tag is now an annotated, GPG-signed tag carrying
  the message *gogogo: debian packaging revision N for v<X>*
* README documents the new model end-to-end under
  *Re-publishing and Debian revisions*

All 27 packages green; live-validated against the apt server and a
Forgejo release with multiple revisions.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Findings from /simplify on the post-checkpoint scope (--replace rename,
graceful 409, auto-prune, signed tags). Behavior is unchanged.

* `pruneOldDebRevisions` is now a method on `*Release` — drops 5
  parameters that were all derivable from `r.cf`, `ri`, and
  `r.debRevision`.
* Fix latent footgun in forgejo PostRelease 409 branch: `kept :=
  ri.Assets[:0]` was aliasing the caller's backing array; switched to
  a fresh `make(publish.Assets, 0, len(...))` so any future caller
  reading `ri.Assets` post-call sees the unmodified slice.
* `bumpedDebRevision` field on `*Release` was redundant — derivable
  from `r.debRevision > 1`. Removed; the post-publish *had no effect*
  warning now reads the revision directly.
* Flatten the nested errors.As chain in PostRelease into a single
  type-assert + switch — three `errors.As` calls collapsed to one.
* Refine the unreachable fallback in the 409 branch: a GET error is
  wrapped with `%w`; a nil result (race condition) gets its own
  explicit message instead of the misleading *use --replace* error.
* `CreateSignedTag` now pre-checks tag existence before invoking
  `git tag -a -s`, so a real signing failure (missing key, expired
  cert) is propagated cleanly instead of being silently swallowed by
  the after-the-fact idempotency check.
* New helper `build.DebPackagingTag(upstream, rev)` lives next to
  `ParseDebPackagingTag` so writer and parser share one format.

All 27 packages green; live publish on xr-invoiced still emits the
expected warnings and writes the signed receipt tag.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
heiko commented 2026-05-20 08:23:57 +02:00
Author
Owner
Copy link

ai:codex review findings

I found three issues worth fixing before merge:

  1. internal/cmd/release/dupload.go:68: stageDebian always copies upload assets, even when uploadDirOverride points at the already-populated out/<version>/deb directory. In pack --out with one dupload destination, this can copy a file onto itself and truncate .deb/.dsc artifacts.

  2. internal/cmd/release/release.go:172: publish --dry-run now runs the publish pipeline instead of just printDryRun, contradicting README/help text that says no builds or uploads happen.

  3. internal/cmd/release/release.go:1487: obsolete Debian revision pruning only matches path.Base(module), so multi-package artifacts with configured package names will not be pruned.

Verification: go test ./... passes outside the sandbox with GOCACHE=/tmp/gogogo-go-build CCACHE_DIR=/tmp/gogogo-ccache. The sandboxed run failed only because local sockets/network integration tests are blocked there.

ai:codex review findings I found three issues worth fixing before merge: 1. `internal/cmd/release/dupload.go:68`: `stageDebian` always copies upload assets, even when `uploadDirOverride` points at the already-populated `out/<version>/deb` directory. In `pack --out` with one dupload destination, this can copy a file onto itself and truncate `.deb`/`.dsc` artifacts. 2. `internal/cmd/release/release.go:172`: `publish --dry-run` now runs the publish pipeline instead of just `printDryRun`, contradicting README/help text that says no builds or uploads happen. 3. `internal/cmd/release/release.go:1487`: obsolete Debian revision pruning only matches `path.Base(module)`, so multi-package artifacts with configured package names will not be pruned. Verification: `go test ./...` passes outside the sandbox with `GOCACHE=/tmp/gogogo-go-build CCACHE_DIR=/tmp/gogogo-ccache`. The sandboxed run failed only because local sockets/network integration tests are blocked there.
`copyFilePreserve` opened the destination with O_TRUNC before reading from the
source. When `pack --out <dir>` combined with a single dupload destination, the
override pointed at the dir that already held the artifacts, so src and dst
were the same file — O_TRUNC truncated the .deb/.dsc to zero bytes before the
copy could read it.

Guard via os.SameFile: if dst already exists and shares an inode with src, the
copy is a no-op. Regression test covers the src==dst path.

Reviewer-found (codex ai:codex).
The dry-run gate was conditioned on `reqStage != StagePublish`, so
`gogogo publish --dry-run` fell through to r.execute and actually ran the
build pipeline (cloning, go build, etc.) — contradicting README and --help
text that promise "no builds, no uploads".

Drop the stage exception so printDryRun handles every stage uniformly. The
existing --verify path inside printDryRun still does the module-proxy check
when explicitly requested.

Reviewer-found (codex ai:codex).
pruneOldDebRevisions filtered candidate assets by path.Base(r.cf.Module), so
explicit Package.Name entries (artifacts.packages: [{name: foo-server}, ...])
were never matched and stale revisions of those packages accumulated forever
on the Forgejo release.

Refactor parseRevisionedDebAsset to recover the package name from the asset
filename (split at the first "_<upstreamVer>-" boundary) and return it as a
third value. The prune loop now matches every <pkg>_<upstream>-<N>.<suffix>
asset regardless of which configured package it belongs to.

Tests cover the explicit-name, dash-and-underscore-name, and tilde-pre-release
cases that previously slipped through.

Reviewer-found (codex ai:codex).
strings.HasPrefix(rest[i:], ".dsc") && rest[i:] == ".dsc" is the equality
alone — the prefix check is subsumed.

Behavior unchanged.
dupload.go copyFilePreserve, pack/source/source.go copyFile, and
pack/source/deb.go copyDebFile all did the same thing: open, OpenFile with
O_TRUNC, io.Copy, close. Only the dupload one had the os.SameFile guard
added in bd35ff0; the other two were silently vulnerable to the same
truncate-to-zero bug whenever a future caller let src and dst overlap.

Move the canonical implementation to a new internal/fsutil package:

* preserves src's permission bits (info.Mode().Perm()) — matters for
  debian/rules and similar files where the exec bit must survive the copy.
* checks os.SameFile before opening dst with O_TRUNC.
* returns bare errors; callers wrap with their own context.

All three local helpers are removed. moveFile in source.go now calls
fsutil.Copy directly. The dupload self-copy regression test moved into
internal/fsutil/copy_test.go where it belongs, with an additional
permission-preservation test.

Net diff: -6 lines, one new exported helper, three latent footguns closed.
heiko commented 2026-05-20 14:09:18 +02:00
Author
Owner
Copy link

Re-review note after HEAD 995fa1e: the self-copy/truncation finding looks fixed. The duplicate copy helpers now route through internal/fsutil.Copy, and TestCopy_SameFile covers the no-op case. One remaining thing to tighten: fsutil.Copy currently documents/tests global permission preservation, but normal filesystem copies should remain umask-respecting under the project policy. Suggested fix: make fsutil.Copy's contract explicit as umask-respecting and remove or adjust TestCopy_PreservesPermissions. For packaged data paths that require stable modes, set modes at the packaging boundary instead: keep explicit chmod for generated/rendered debian/rules, and if overlay/source-tar/deb/rpm staging needs exact source modes, use a packaging-specific copy helper or post-copy Chmod there only. That keeps dupload/general staging umask-respecting while making deb/rpm/source artifacts deterministic.

Re-review note after HEAD 995fa1e: the self-copy/truncation finding looks fixed. The duplicate copy helpers now route through internal/fsutil.Copy, and TestCopy_SameFile covers the no-op case. One remaining thing to tighten: fsutil.Copy currently documents/tests global permission preservation, but normal filesystem copies should remain umask-respecting under the project policy. Suggested fix: make fsutil.Copy's contract explicit as umask-respecting and remove or adjust TestCopy_PreservesPermissions. For packaged data paths that require stable modes, set modes at the packaging boundary instead: keep explicit chmod for generated/rendered debian/rules, and if overlay/source-tar/deb/rpm staging needs exact source modes, use a packaging-specific copy helper or post-copy Chmod there only. That keeps dupload/general staging umask-respecting while making deb/rpm/source artifacts deterministic.
A review on PR #63 suggested making fsutil.Copy umask-respecting (the
standard `cp` default), with explicit chmod fixups at packaging boundaries
that need exact modes. The codebase pushes back on that for two reasons:

* All four callers feed packaging paths where the source mode is intentional
  — orig.tar.gz contents, debian/ helper scripts, finished tarballs, and
  pre-generated dupload assets. Preservation matters; umask doesn't.

* The deterministic orig.tar.gz work already on this branch deliberately
  strips environment dependence from the archive. A umask-respecting copy
  would re-introduce it for mode bits.

The behaviour is now spelled out in the Copy doc-comment and in the
TestCopy_PreservesPermissions header so future readers (and future
reviewers) can see the deliberate choice rather than rediscover it.
heiko merged commit 033fdc7401 into master 2026-05-20 14:24:01 +02:00
heiko commented 2026-05-20 14:28:27 +02:00
Author
Owner
Copy link

Thanks for the re-review. The umask-respecting variant is the canonical Unix policy for general-purpose copies, and we considered switching — but for this codebase the four callers of fsutil.Copy all feed packaging paths where source mode is intentional (debian/rules and helper scripts need the exec bit; orig.tar.gz mode bits need to be deterministic across rebuilds).

A umask-respecting copy would force chmod fixups at every packaging site and re-introduce environment dependence into the orig.tar.gz that the earlier reproducibility work on this branch deliberately stripped out.

To make sure this choice doesn't get rediscovered the hard way, 9c53a2b adds the rationale to the fsutil.Copy doc-comment and the TestCopy_PreservesPermissions header, with a back-reference to this PR discussion.

Merged as 033fdc7 on master; #61 autoclosed.

(co)authored by ai (claude opus 4.7)

Thanks for the re-review. The umask-respecting variant is the canonical Unix policy for general-purpose copies, and we considered switching — but for this codebase the four callers of `fsutil.Copy` all feed packaging paths where source mode is intentional (debian/rules and helper scripts need the exec bit; orig.tar.gz mode bits need to be deterministic across rebuilds). A umask-respecting copy would force `chmod` fixups at every packaging site and re-introduce environment dependence into the orig.tar.gz that the earlier reproducibility work on this branch deliberately stripped out. To make sure this choice doesn't get rediscovered the hard way, 9c53a2b adds the rationale to the `fsutil.Copy` doc-comment and the `TestCopy_PreservesPermissions` header, with a back-reference to this PR discussion. Merged as 033fdc7 on master; #61 autoclosed. (co)authored by ai (claude opus 4.7)
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
Compat/Breaking
Breaking change that won't be backward compatible

  
Kind/Bug
Something is not working

  
Kind/Documentation
Documentation changes

  
Kind/Enhancement
Improve existing functionality

  
Kind/Feature
New functionality

  
Kind/Security
This is security issue

  
Kind/Testing
Issue or pull request related to testing

  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Reviewed
Confirmed
 Issue has been confirmed

  
Reviewed
Duplicate
 This issue or pull request already exists

  
Reviewed
Invalid
 Invalid issue

  
Reviewed
Won't Fix
 This issue won't be fixed

  
Status
Abandoned
 Somebody has started to work on this but abandoned work

  
Status
Blocked
 Something is blocking this issue or pull request

  
Status
Need More Info
 Feedback is required to reproduce issue or to continue work

  
nagonag
Automated dependency scan finding

  
nagonag/ignore
Ignore this nagonag finding

No labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
nagonag
nagonag/ignore
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
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
heiko/gogogo!63
No description provided.