restructure: split core logic into gpgpublish/ library package ai:claude-sonnet-4-6 #7

heiko · 2026-05-22T00:26:41+02:00

heiko commented

2026-05-22 00:26:41 +02:00

Move all non-CLI source files to a new gpgpublish/ subdirectory that
exports package gpgpublish (importable as
go.schlittermann.de/heiko/gpg-publish/gpgpublish). main.go stays at
the repo root and becomes a thin CLI wrapper.

Changes:

gpgpublish/process.go: new Config struct, ProcessKey, GetKey,
parseKeyRing extracted from old main.go
gpgpublish/domain.go: domainList → DomainList (exported)
gpgpublish/ttl.go: parseTTL → ParseTTL (exported)
gpgpublish/check.go: exported constants get doc comments
main.go: rewritten as thin flag-parsing CLI
man.go: receives //go:generate directives from old main.go
gpgpublish/process_test.go (was main_test.go): tests migrated;
TestGetKeyFromStdin now t.Parallel-safe (no os.Stdin swap)
new TestProcessKeyBehaviour: structural test checking DNS labels and
parseable key data, immune to map-iteration non-determinism
testdata/ moved to gpgpublish/testdata/ alongside the tests
CLAUDE.md updated

(co)authored by ai:claude-sonnet-4-6

Move all non-CLI source files to a new gpgpublish/ subdirectory that exports package gpgpublish (importable as go.schlittermann.de/heiko/gpg-publish/gpgpublish). main.go stays at the repo root and becomes a thin CLI wrapper. Changes: - gpgpublish/process.go: new Config struct, ProcessKey, GetKey, parseKeyRing extracted from old main.go - gpgpublish/domain.go: domainList → DomainList (exported) - gpgpublish/ttl.go: parseTTL → ParseTTL (exported) - gpgpublish/check.go: exported constants get doc comments - main.go: rewritten as thin flag-parsing CLI - man.go: receives //go:generate directives from old main.go - gpgpublish/process_test.go (was main_test.go): tests migrated; TestGetKeyFromStdin now t.Parallel-safe (no os.Stdin swap) - new TestProcessKeyBehaviour: structural test checking DNS labels and parseable key data, immune to map-iteration non-determinism - testdata/ moved to gpgpublish/testdata/ alongside the tests - CLAUDE.md updated (co)authored by ai:claude-sonnet-4-6

heiko added 1 commit

2026-05-22 00:26:41 +02:00

restructure: split core logic into gpgpublish/ library package ai:claude-sonnet-4-6 9908a8f808

Move all non-CLI source files to a new gpgpublish/ subdirectory that
exports package gpgpublish (importable as
go.schlittermann.de/heiko/gpg-publish/gpgpublish).  main.go stays at
the repo root and becomes a thin CLI wrapper.

- gpgpublish/process.go: new Config struct, ProcessKey, GetKey,
  parseKeyRing extracted from old main.go
- gpgpublish/domain.go: domainList → DomainList (exported)
- gpgpublish/ttl.go: parseTTL → ParseTTL (exported)
- gpgpublish/check.go: StatusOK/DIFF/MISS constants get doc comments
- main.go: rewritten as thin flag-parsing CLI
- man.go: receives the //go:generate directives from old main.go
- gpgpublish/process_test.go (was main_test.go): all tests migrated,
  TestGetKeyFromStdin no longer swaps os.Stdin (now t.Parallel safe)
- new TestProcessKeyBehaviour: stable structural test verifying DNS
  labels and parseable key data, immune to map-iteration non-determinism
- testdata/ moved to gpgpublish/testdata/ alongside the tests
- CLAUDE.md updated to reflect new layout

heiko added 1 commit

2026-05-22 00:33:20 +02:00

simplify: fix nil-deref, hoist time.Now, clean up emailMatch ai:claude-sonnet-4-6 596c11d897

- check.go: guard err != nil before accessing r.Truncated (was a panic path)
- process.go: hoist time.Now() above the entity loop; consistent snapshot
- process.go: use ident.UserId.Name (pre-parsed) instead of manual " <" strip
- process.go: drop dead email field from emailMatch; remove what-comment
- process_test.go: collapse 4 format smoke-tests into one table test

heiko added 1 commit

2026-05-22 00:38:57 +02:00

simplify: guard unknown OutputFormat, move type out of loop, fix test name ai:claude-sonnet-4-6 a4ddddff72

- process.go: add default case to OutputFormat switch; unknown format now
  returns an error instead of silently emitting nothing
- process.go: move emailMatch type definition above the entity loop (was
  being redeclared on every iteration)
- process_test.go: add OutputFormat to TestProcessKeyEmptyDomains (was
  passing "" which the new guard catches)
- process_test.go: rename TestGetKeyFromFileNotExist to
  TestGetKeyFromArmoredFile (the .asc fixture exists; the old name implied
  it didn't)
- process_test.go: remove what-comment on RDATA trimming line

heiko commented

2026-05-22 00:44:44 +02:00

Code Review

What'''s Good

ProcessKey(w io.Writer, …) instead of hardcoded os.Stdout — testable and composable
GetKey(r io.Reader, …) making stdin injectable — TestGetKeyFromStdin is now t.Parallel()-safe
TestProcessKeyBehaviour is a solid structural test immune to map-iteration non-determinism (sorts output, checks labels + base64 validity + OpenPGP parseability)
emailMatch type hoisted above loop; time.Now() hoisted above entity loop — both clean-ups
default: return fmt.Errorf("unknown output format %q") prevents silent no-op for library callers

Issues

Bug — matchOrder has non-deterministic ordering (process.go:142)

e.Identities is a map[string]*openpgp.Identity. The order in which identities are added to matchOrder varies between runs. When two UIDs share the same email, the names slice inside emailMatch also varies. Output record order is therefore non-deterministic when one entity has multiple UIDs. TestProcessKeyBehaviour sorts output lines to dodge this, but repeated runs of the tool against the same key can produce records in different order. Probably harmless in practice, worth noting.

Footgun — GetKey(nil, "-") panics (process.go:53)

func GetKey(r io.Reader, target string) ([]byte, error) {
    if target == "-" {
        return io.ReadAll(r)  // panics if r == nil
    }

io.ReadAll(nil) panics. The doc says "when target is -, r is read" but does not warn about nil. A one-liner guard closes this:

if target == "-" {
    if r == nil {
        return nil, errors.New("stdin requested but no reader provided")
    }
    return io.ReadAll(r)
}

Library footgun — warnings always go to os.Stderr (warn.go)

warnf (and the stderrDim TTY check evaluated at init() time) write directly to os.Stderr. Library callers have no way to suppress or redirect them. Config has ShowWarnings bool for key-stripping warnings, but expiry and conflict warnings bypass it entirely. Consider an optional Stderr io.Writer field in Config.

DomainList mixes CLI and library concerns (domain.go)

DomainList implements flag.Value (String() and Set()). This drags the CLI flag abstraction into the library API. A plain []string for Config.Domains is sufficient; DomainList with its Set validation is only useful for flag.Var. Consider keeping DomainList in main.go and accepting []string in the library — or document that library callers should populate Config.Domains directly without going through Set.

Minor

process.go:230 and main.go:83 both list the four valid output formats. The default: guard in ProcessKey is now authoritative; the validFormats map in main.go is a second copy that must stay in sync manually.
StatusOK/DIFF/MISS as bare string constants means a case StatusDifF: typo compiles silently.
The check path (checkRecord) has no unit test with an injected exchanger — dnsExchanger is unexported, so mock-based testing is not possible from outside the package.
The //go:generate move from main.go to man.go is correct.

Verdict

Mechanically sound, tests pass, lint clean. The nil-panic in GetKey is the one item worth fixing before merge. warn.go and DomainList concerns can be addressed in a follow-up if the package gets external callers.

(co)authored by ai:claude-sonnet-4-6

## Code Review ### What'''s Good - `ProcessKey(w io.Writer, …)` instead of hardcoded `os.Stdout` — testable and composable - `GetKey(r io.Reader, …)` making stdin injectable — `TestGetKeyFromStdin` is now `t.Parallel()`-safe - `TestProcessKeyBehaviour` is a solid structural test immune to map-iteration non-determinism (sorts output, checks labels + base64 validity + OpenPGP parseability) - `emailMatch` type hoisted above loop; `time.Now()` hoisted above entity loop — both clean-ups - `default: return fmt.Errorf("unknown output format %q")` prevents silent no-op for library callers --- ### Issues **Bug — `matchOrder` has non-deterministic ordering** (`process.go:142`) `e.Identities` is a `map[string]*openpgp.Identity`. The order in which identities are added to `matchOrder` varies between runs. When two UIDs share the same email, the `names` slice inside `emailMatch` also varies. Output record order is therefore non-deterministic when one entity has multiple UIDs. `TestProcessKeyBehaviour` sorts output lines to dodge this, but repeated runs of the tool against the same key can produce records in different order. Probably harmless in practice, worth noting. **Footgun — `GetKey(nil, "-")` panics** (`process.go:53`) ```go func GetKey(r io.Reader, target string) ([]byte, error) { if target == "-" { return io.ReadAll(r) // panics if r == nil } ``` `io.ReadAll(nil)` panics. The doc says "when target is `-`, r is read" but does not warn about nil. A one-liner guard closes this: ```go if target == "-" { if r == nil { return nil, errors.New("stdin requested but no reader provided") } return io.ReadAll(r) } ``` **Library footgun — warnings always go to `os.Stderr`** (`warn.go`) `warnf` (and the `stderrDim` TTY check evaluated at `init()` time) write directly to `os.Stderr`. Library callers have no way to suppress or redirect them. `Config` has `ShowWarnings bool` for key-stripping warnings, but expiry and conflict warnings bypass it entirely. Consider an optional `Stderr io.Writer` field in `Config`. **`DomainList` mixes CLI and library concerns** (`domain.go`) `DomainList` implements `flag.Value` (`String()` and `Set()`). This drags the CLI flag abstraction into the library API. A plain `[]string` for `Config.Domains` is sufficient; `DomainList` with its `Set` validation is only useful for `flag.Var`. Consider keeping `DomainList` in `main.go` and accepting `[]string` in the library — or document that library callers should populate `Config.Domains` directly without going through `Set`. --- ### Minor - `process.go:230` and `main.go:83` both list the four valid output formats. The `default:` guard in `ProcessKey` is now authoritative; the `validFormats` map in `main.go` is a second copy that must stay in sync manually. - `StatusOK/DIFF/MISS` as bare `string` constants means a `case StatusDifF:` typo compiles silently. - The check path (`checkRecord`) has no unit test with an injected exchanger — `dnsExchanger` is unexported, so mock-based testing is not possible from outside the package. - The `//go:generate` move from `main.go` to `man.go` is correct. --- ### Verdict Mechanically sound, tests pass, lint clean. The nil-panic in `GetKey` is the one item worth fixing before merge. `warn.go` and `DomainList` concerns can be addressed in a follow-up if the package gets external callers. (co)authored by ai:claude-sonnet-4-6

heiko added 1 commit

2026-05-22 00:48:39 +02:00

fix: GetKey returns error instead of panicking when r is nil and target is "-" ai:claude-sonnet-4-6 980bced149

heiko commented

2026-05-22 00:48:45 +02:00

Fixed the GetKey(nil, "-") footgun in 980bced: the function now returns errors.New("stdin requested but no reader provided") instead of panicking. Test TestGetKeyFromStdinNilReader added.

Fixed the `GetKey(nil, "-")` footgun in 980bced: the function now returns `errors.New("stdin requested but no reader provided")` instead of panicking. Test `TestGetKeyFromStdinNilReader` added.

heiko reviewed

2026-05-22 00:59:16 +02:00

heiko left a comment

Review findings from Codex.

gpgpublish/process.go

					
				@ -0,0 +160,4 @@

							email := ident.UserId.Email

							if m, ok := matches[email]; ok {

								m.names = append(m.names, ident.UserId.Name)

This loses the OpenPGP User ID comment component. For a UID like Heiko Schlittermann (Dresden) <hs@...>, ident.UserId.Name is only Heiko Schlittermann; the previous full-identity behavior preserved (Dresden) / (HS12-RIPE) distinctions when comments were rendered for multiple UIDs sharing the same name/email.

This loses the OpenPGP User ID comment component. For a UID like `Heiko Schlittermann (Dresden) <hs@...>`, `ident.UserId.Name` is only `Heiko Schlittermann`; the previous full-identity behavior preserved `(Dresden)` / `(HS12-RIPE)` distinctions when comments were rendered for multiple UIDs sharing the same name/email.

gpgpublish/process.go

					
				@ -0,0 +241,4 @@

										snip = b64[:4] + "…" + b64[len(b64)-4:]

									}

									fmt.Fprintf(w, "%s %d IN OPENPGPKEY %s ; %s\n", label, cfg.TTL, snip, recordComment)

Now that ProcessKey accepts a caller-supplied io.Writer, this fmt.Fprintf error is silently dropped. A library caller writing to a full file or failing pipe can get nil after truncated output. The check-mode fmt.Fprintf above has the same problem, while the other formats already return w.Write errors, so these Fprintf calls should be checked.

Now that `ProcessKey` accepts a caller-supplied `io.Writer`, this `fmt.Fprintf` error is silently dropped. A library caller writing to a full file or failing pipe can get `nil` after truncated output. The check-mode `fmt.Fprintf` above has the same problem, while the other formats already return `w.Write` errors, so these `Fprintf` calls should be checked.

heiko commented

2026-05-22 11:36:46 +02:00

Opus follow-up review

Verifying Sonnet's findings against 980bced.

Fixed in this PR

GetKey(nil, "-") panic — fixed; TestGetKeyFromStdinNilReader added.

Confirmed open

matchOrder is non-deterministic (gpgpublish/process.go:142).
e.Identities is map[string]*openpgp.Identity. Iteration order varies, so matchOrder (and the names slice inside emailMatch) varies between runs whenever an entity has multiple UIDs sharing an email. Output record order changes accordingly. The structural test sorts lines and hides it; the binary's output to a pipe will not. Sorting matchOrder (e.g. by ident.UserId.Email, then by ident.UserId.Name inside emailMatch.names) removes the non-determinism for ~3 lines of code.

warn.go package init drags os.Stderr into the library (gpgpublish/warn.go:13).
var stderrDim = term.IsTerminal(int(os.Stderr.Fd())) runs at package init time on every import — even when callers never invoke warnf. A library package should not probe os.Stderr at init. A Config.Stderr io.Writer field, lazily defaulted to os.Stderr inside ProcessKey, lets callers redirect or silence warnings and avoids the init-time side effect.

DomainList mixes CLI and library (gpgpublish/domain.go).
String()/Set() make it a flag.Value. Library callers populating Config.Domains directly bypass Set's validation/lowercasing, so the type is a footgun there. Two clean options:

Move DomainList back to main.go; let the library take a plain []string and rely on matchDomain for runtime semantics.
Export Set-style validation as a free function NormalizeDomain so both CLI and library use the same path.

validFormats duplicated (main.go:83 vs the switch in gpgpublish/process.go). The CLI's map[string]bool and the library's switch must stay in sync manually. Hoisting the canonical set into the library (e.g. gpgpublish.ValidOutputFormats() []string) and having main.go consume it removes the drift risk.

Minor

StatusOK/DIFF/MISS as bare string constants — agree, a named type Status string prevents the silent-typo compile.

Verdict

Not blocking; all four open items are post-merge polish for the library boundary. Worth a single follow-up PR rather than churning this one.

(co)authored by ai:claude-opus-4-7

## Opus follow-up review Verifying Sonnet's findings against `980bced`. ### Fixed in this PR - `GetKey(nil, "-")` panic — fixed; `TestGetKeyFromStdinNilReader` added. ### Confirmed open **`matchOrder` is non-deterministic** (`gpgpublish/process.go:142`). `e.Identities` is `map[string]*openpgp.Identity`. Iteration order varies, so `matchOrder` (and the `names` slice inside `emailMatch`) varies between runs whenever an entity has multiple UIDs sharing an email. Output record order changes accordingly. The structural test sorts lines and hides it; the binary's output to a pipe will not. Sorting `matchOrder` (e.g. by `ident.UserId.Email`, then by `ident.UserId.Name` inside `emailMatch.names`) removes the non-determinism for ~3 lines of code. **`warn.go` package init drags `os.Stderr` into the library** (`gpgpublish/warn.go:13`). `var stderrDim = term.IsTerminal(int(os.Stderr.Fd()))` runs at package init time on every import — even when callers never invoke `warnf`. A library package should not probe `os.Stderr` at init. A `Config.Stderr io.Writer` field, lazily defaulted to `os.Stderr` inside `ProcessKey`, lets callers redirect or silence warnings and avoids the init-time side effect. **`DomainList` mixes CLI and library** (`gpgpublish/domain.go`). `String()`/`Set()` make it a `flag.Value`. Library callers populating `Config.Domains` directly bypass `Set`'s validation/lowercasing, so the type is a footgun there. Two clean options: 1. Move `DomainList` back to `main.go`; let the library take a plain `[]string` and rely on `matchDomain` for runtime semantics. 2. Export `Set`-style validation as a free function `NormalizeDomain` so both CLI and library use the same path. **`validFormats` duplicated** (`main.go:83` vs the `switch` in `gpgpublish/process.go`). The CLI's `map[string]bool` and the library's `switch` must stay in sync manually. Hoisting the canonical set into the library (e.g. `gpgpublish.ValidOutputFormats() []string`) and having `main.go` consume it removes the drift risk. ### Minor - `StatusOK/DIFF/MISS` as bare `string` constants — agree, a named `type Status string` prevents the silent-typo compile. ### Verdict Not blocking; all four open items are post-merge polish for the library boundary. Worth a single follow-up PR rather than churning this one. (co)authored by ai:claude-opus-4-7

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin restructure-library-package:restructure-library-package

git switch restructure-library-package

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git switch master

git merge --no-ff restructure-library-package

git switch restructure-library-package

git rebase master

git switch master

git merge --ff-only restructure-library-package

git switch restructure-library-package

git rebase master

git switch master

git merge --no-ff restructure-library-package

git switch master

git merge --squash restructure-library-package

git switch master

git merge --ff-only restructure-library-package

git switch master

git merge restructure-library-package

git push origin master

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project

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/gpg-publish!7

Rows
Columns