ci: add release machinery (version.txt, goreleaser, identity, reusable callers) by rianjs · Pull Request #4 · open-cli-collective/codereview-cli

rianjs · 2026-05-29T19:04:32Z

Adds release machinery to codereview-cli and wires it into the shared @v1 reusable workflows. First real caller of auto-release.yml + release.yml.

Changes

cmd/cr/main.go + internal/version — a minimal scaffold cr binary that reports its build version, so GoReleaser has a real package main to build (a dry-run --snapshot compiles). Replaces the now-redundant internal/placeholder. Not the command surface — that lands separately.
version.txt (0.1), .goreleaser.yml — pure-Go build (CGO off) for linux/darwin/windows × amd64/arm64, archives (cr_v{{.Version}}_…), checksums, github changelog, and release.replace_existing_artifacts: true (release-preflight requires it). Before-hook uses make tidy so a mutating tidy fails the release.
packaging/identity.yml — binary: cr, tag.prefix: v, version_scheme: major_minor_run_patch, archive template. No packages: section — chocolatey/winget are README stubs, not implemented channels; declaring them would force identity-check to require real manifests and run secretless channel jobs.
.github/workflows/auto-release.yml + release.yml (callers) — delegate to open-cli-collective/.github/.github/workflows/*@v1. Boolean-safe dry-run; workflow_dispatch for manual runs.
ci.yml — adds the identity-check job (manifest now exists).
Makefile — completes the §4 shipped-binary contract (snapshot/release/install/fmt/test-cover); CLAUDE.md/README/CONTRIBUTING document the release flow and the full required-check set.

Channel scope

GitHub-release archives only for now — no Homebrew/winget/chocolatey/linux, no CGO/Keychain. Those arrive when the real CLI implements the credential store and packaging.

Verification

make check green; goreleaser check + goreleaser release --snapshot --clean build all 6 OS/arch + checksums locally.
identity-check validates the manifest (all channels null → channel jobs skip); release-preflight passes.

Sequencing

Merged with a ci: title so the post-merge auto-release run cuts no tag (it gates on feat:/fix:) — also sidesteps the not-yet-configured RELEASE_TAG_TOKEN.
Branch protection gains identity-check after the job runs green here.
The live auto-release tag-minting path (needs RELEASE_TAG_TOKEN) is a residual follow-up; the tag→release path is proven by the local snapshot and the first real v0.1.0 tag.

Closes #2

@v1

…e callers) Wire codereview-cli into the shared @v1 release flow: - Add a minimal `cr` binary (cmd/cr + internal/version) so goreleaser has a real package main to build; replace the now-redundant internal/placeholder. - version.txt (0.1), .goreleaser.yml (pure-Go build for linux/darwin/windows on amd64/arm64, archives, checksums, replace_existing_artifacts), and packaging/identity.yml (binary, tag prefix v, archive template). - Caller workflows auto-release.yml (push to main) and release.yml (tag push) delegating to the reusable open-cli-collective/.github workflows at @v1. - Add the identity-check job to ci.yml now that a manifest exists. - Expand the Makefile to the shipped-binary target contract (snapshot/release/ install/fmt/test-cover) and document the release flow. No packaging channels are declared (chocolatey/winget are README stubs only), so the reusable release workflow ships GitHub-release archives only and its channel jobs skip. The CGO-darwin split and channels land when the real CLI implements the credential store and packaging. Closes #2

rianjs · 2026-05-29T19:06:00Z

Findings

Minor: `fmt` target does not satisfy the Makefile contract

Makefile uses gofmt -w .. With the current layout, that does not recursively format cmd/cr / internal/version the way a repo-wide fmt target should. Use go fmt ./... or a recursive gofmt invocation.

Minor: changelog grouping does not match the accepted commit grammar

.goreleaser.yml groups only ^feat: and ^fix:. The shared grammar accepts scoped/bang forms like feat(cli):, fix(api)!:. Those would fall into Other despite being release-gating commits. Use regexes aligned with the shared grammar, e.g. ^feat(\$[^)]+\$)?!?: and ^fix(\$[^)]+\$)?!?:.

Architecture Check

The three priming majors are honored:

No stub channels declared in packaging/identity.yml.
identity-check is added only after the manifest exists, with branch-protection sequencing documented.
First release on merge is controlled by the ci: PR title; live auto-release tag minting remains a deliberate follow-up after RELEASE_TAG_TOKEN.

The reusable @v1 callers look correctly wired: auto-release passes tag-token and boolean-safe dry-run; release passes no channel secrets because no channels are declared.

rianjs · 2026-05-29T19:06:59Z

TDD verdict: one genuine gap worth addressing; everything else in this PR is non-testable infrastructure.

What's covered

No tests were added in this PR, and none are needed for the bulk of the diff:

.github/workflows/auto-release.yml, release.yml, ci.yml additions — CI config, not testable in Go.
.goreleaser.yml, packaging/identity.yml, version.txt — build/packaging config, not testable in Go.
Makefile, .golangci.yml, CLAUDE.md, CONTRIBUTING.md, README.md — docs/tooling, not testable.
internal/version/version.go — three var declarations with string defaults; no logic to test.

Genuine gap

cmd/cr/main.go — dispatch logic is testable but has no tests.

The file has real branching logic across three functions:

Function	Testable behavior
`firstArg()`	Returns `""` when `os.Args` has no second element; returns `os.Args[1]` otherwise.
`usage(w)`	Writes the help string to the supplied `io.Writer`.
`main()` dispatch	Routes `--version`/`-v`/`version`, `""`/`--help`/`-h`/`help`, and unknown args to the right output/exit code.

firstArg and usage are already package-level functions and can be tested directly from a main_test.go in the same package. The main() dispatch itself is harder to unit-test (it calls os.Exit), but the common pattern is to extract the dispatch into a run(args []string, stdout, stderr io.Writer) int helper — main becomes a one-liner — and then run is fully testable.

What tests should assert:

firstArg with a single-element os.Args slice → "".
firstArg with ["cr", "version"] → "version".
usage writes the expected help string to a bytes.Buffer (non-empty, contains "cr version").
A run-style helper: known version args → version string on stdout, exit 0; help args → usage on stdout, exit 0; unknown arg → error line on stderr, usage on stderr, exit 2.

Proportionality note

This is a scaffold release-machinery PR — the CLI does nothing yet. Leaving main.go untested is a low-cost deferral now, but the run-helper refactor is easiest to do while the file is still 44 lines. Worth a follow-up issue rather than blocking this PR.

monit-reviewer

Automated PR Review

Reviewed commit: 7ca8d19

Summary

Reviewer	Findings
documentation:docs-reviewer	1
harness-engineering:harness-architecture-reviewer	2
harness-engineering:harness-enforcement-reviewer	3
harness-engineering:harness-knowledge-reviewer	2

documentation:docs-reviewer (1 findings)

💡 Suggestion - CLAUDE.md:69

CLAUDE.md, CONTRIBUTING.md, and README.md all list 'identity-check' as a current required status check, but the PR sequencing notes branch protection gains this check only after this PR merges green. The docs present the target state as current state, which could mislead a contributor setting up branch protection by following these docs before the check is actually enforced.

harness-engineering:harness-architecture-reviewer (2 findings)

💡 Suggestion - cmd/cr/main.go:29

'-v' is universally understood as --verbose in CLI conventions (grep, git, curl, go test). Mapping it to --version now means it cannot be repurposed for verbose/debug once the real command surface lands — removing it then would be a breaking change. '--version' and the 'version' subcommand are sufficient; drop '-v' now or explicitly reserve it for future verbosity use.

💡 Suggestion - .golangci.yml:24

The errcheck exclusion for fmt.Fprint/Fprintf/Fprintln is blanket project-wide. The stated rationale ('failed terminal write isn't actionable') applies only to os.Stdout/os.Stderr. As the codebase grows beyond a scaffold, this will silently suppress unchecked writes to files or network connections. Consider scoping the exclusion to cmd/cr/ via a path exclusion rather than suppressing it globally.

harness-engineering:harness-enforcement-reviewer (3 findings)

⚠️ Should Fix - .github/workflows/auto-release.yml:25

Boolean dry-run expression bug: GitHub Actions converts type: boolean workflow_dispatch inputs to the strings 'true'/'false', not actual booleans. The expression github.event_name == 'workflow_dispatch' && inputs.dry-run || false always evaluates truthy when dry-run is the string 'false', meaning dry-run mode cannot be disabled via workflow_dispatch — the workflow always behaves as if dry-run is true when manually triggered. Fix: github.event_name == 'workflow_dispatch' && inputs.dry-run == 'true' || false.

⚠️ Should Fix - .github/workflows/release.yml:25

Same boolean dry-run expression bug as auto-release.yml: inputs.dry-run is a string, not a boolean. A manual workflow_dispatch run can never disable dry-run mode, so a human can never trigger a real publish via workflow_dispatch. Fix: github.event_name == 'workflow_dispatch' && inputs.dry-run == 'true' || false.

💡 Suggestion - Makefile:41

make release runs goreleaser release --clean with no guard or confirmation step. If a developer has GORELEASER_* tokens or GITHUB_TOKEN set in their shell environment (common after local testing), running this target accidentally publishes a live release. Consider adding a guard or documenting it explicitly as CI-only.

harness-engineering:harness-knowledge-reviewer (2 findings)

⚠️ Should Fix - packaging/identity.yml:7

version_scheme is declared as 'major_minor_run_patch' (4 segments: X.Y.Z.W), but CLAUDE.md and the PR description both document the tag format as 'v{MAJOR}.{MINOR}.{run-number}' — 3 segments. If the shared identity-check and auto-release reusable workflows interpret this scheme name literally, released tags will be mis-versioned (e.g. v0.1.42.0 instead of v0.1.42). Confirm the scheme name accepted by open-cli-collective/.github@v1 for a 3-part tag and align identity.yml accordingly.

💡 Suggestion - .github/workflows/release.yml:11

workflow_dispatch has no ref-type constraint. If a maintainer manually dispatches from a branch ref with dry-run unchecked, GoReleaser will fail because it cannot resolve a semver tag. The dry-run default (true) mitigates accidental publishes but the failure mode is silent until GoReleaser errors. Consider adding an explicit guard checking that github.ref starts with 'refs/tags/'.

6 info-level observations excluded. Run with --verbose to include.

2 PR discussion threads considered.

Completed in 3m 03s | $0.67 | sonnet | daemon 0.2.121 | Glorfindel

Field	Value
Model	sonnet
Reviewers	hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine	claude · sonnet
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	3m 03s wall · 7m 01s compute (Reviewers: 1m 18s · Synthesis: 1m 41s)
Cost	$0.67
Tokens	115.4k in / 28.6k out
Turns	7

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	35.0k	6.9k	18.6k	16.4k (1h)	$0.18
documentation:docs-reviewer	haiku	6.1k	7.6k	0	6.1k (1h)	$0.05
harness-engineering:harness-architecture-reviewer	sonnet	16.4k	3.5k	2.1k	14.2k (1h)	$0.11
harness-engineering:harness-enforcement-reviewer	sonnet	16.4k	3.7k	2.1k	14.2k (1h)	$0.12
harness-engineering:harness-knowledge-reviewer	sonnet	16.4k	4.1k	2.1k	14.2k (1h)	$0.12
harness-engineering:harness-self-documenting-code-reviewer	sonnet	12.5k	1.3k	2.1k	10.4k (1h)	$0.06
security:security-code-auditor	haiku	12.8k	1.5k	0	12.8k (1h)	$0.03

_{Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.}

monit-reviewer · 2026-05-29T19:10:46Z

+  auto-release:
+    uses: open-cli-collective/.github/.github/workflows/auto-release.yml@v1
+    with:
+      # boolean on both events: workflow_dispatch honors the input; push is live.


🟡 Medium (harness-engineering:harness-enforcement-reviewer): Boolean dry-run expression bug: GitHub Actions converts type: boolean workflow_dispatch inputs to the strings 'true'/'false', not actual booleans. The expression github.event_name == 'workflow_dispatch' && inputs.dry-run || false always evaluates truthy when dry-run is the string 'false', meaning dry-run mode cannot be disabled via workflow_dispatch — the workflow always behaves as if dry-run is true when manually triggered. Fix: github.event_name == 'workflow_dispatch' && inputs.dry-run == 'true' || false.

Reply to this thread when addressed.

Fixed — both callers now use github.event_name == 'workflow_dispatch' && (inputs.dry-run == true || inputs.dry-run == 'true'). I compare against both true and 'true' rather than only == 'true': GitHub's coercion is ambiguous in both directions (if the input surfaces as a real boolean, true == 'true' coerces to a number compare and is false), so the dual comparison is robust either way and always yields a real boolean (push → false).

monit-reviewer · 2026-05-29T19:10:46Z

+jobs:
+  release:
+    uses: open-cli-collective/.github/.github/workflows/release.yml@v1
+    with:


🟡 Medium (harness-engineering:harness-enforcement-reviewer): Same boolean dry-run expression bug as auto-release.yml: inputs.dry-run is a string, not a boolean. A manual workflow_dispatch run can never disable dry-run mode, so a human can never trigger a real publish via workflow_dispatch. Fix: github.event_name == 'workflow_dispatch' && inputs.dry-run == 'true' || false.

Reply to this thread when addressed.

Fixed — same dual-comparison hardening applied here in release.yml.

monit-reviewer · 2026-05-29T19:10:46Z

+goreleaser_config: .goreleaser.yml
+tag:
+  prefix: v
+  version_scheme: major_minor_run_patch


🟡 Medium (harness-engineering:harness-knowledge-reviewer): version_scheme is declared as 'major_minor_run_patch' (4 segments: X.Y.Z.W), but CLAUDE.md and the PR description both document the tag format as 'v{MAJOR}.{MINOR}.{run-number}' — 3 segments. If the shared identity-check and auto-release reusable workflows interpret this scheme name literally, released tags will be mis-versioned (e.g. v0.1.42.0 instead of v0.1.42). Confirm the scheme name accepted by open-cli-collective/.github@v1 for a 3-part tag and align identity.yml accordingly.

Reply to this thread when addressed.

Not a bug — verified against open-cli-collective/.github@v1. The auto-release composite computes the tag as compute_tag = <prefix><MAJOR.MINOR>.<run> (gate.sh), i.e. v0.1.<run> — a 3-segment tag. version_scheme: major_minor_run_patch is descriptive metadata (the patch segment is the run number), not parsed literally; it's the family-standard value (the gro/nrq identity fixtures use it). version.txt holds MAJOR.MINOR and the run number fills patch. No 4-segment tag is produced.

monit-reviewer · 2026-05-29T19:10:46Z

+		arg = args[0]
+	}
+	switch arg {
+	case "--version", "-v", "version":


🔵 Low (harness-engineering:harness-architecture-reviewer): '-v' is universally understood as --verbose in CLI conventions (grep, git, curl, go test). Mapping it to --version now means it cannot be repurposed for verbose/debug once the real command surface lands — removing it then would be a breaking change. '--version' and the 'version' subcommand are sufficient; drop '-v' now or explicitly reserve it for future verbosity use.

Reply to this thread when addressed.

Fixed — dropped the -v alias (it conventionally means --verbose; reserving it). --version and the version subcommand remain, and a test now asserts -v is treated as an unknown command.

monit-reviewer · 2026-05-29T19:10:46Z

+    errcheck:
+      # writes to stdout/stderr are conventionally unchecked in a CLI (family
+      # precedent); a failed terminal write isn't actionable.
+      exclude-functions:


🔵 Low (harness-engineering:harness-architecture-reviewer): The errcheck exclusion for fmt.Fprint/Fprintf/Fprintln is blanket project-wide. The stated rationale ('failed terminal write isn't actionable') applies only to os.Stdout/os.Stderr. As the codebase grows beyond a scaffold, this will silently suppress unchecked writes to files or network connections. Consider scoping the exclusion to cmd/cr/ via a path exclusion rather than suppressing it globally.

Reply to this thread when addressed.

Keeping it: the exclusion is function-scoped to the three fmt.Fprint* writers (not a blanket errcheck-off), matches the family precedent (newrelic-cli), and the rationale is documented inline. golangci v2's only path-scoping tool (exclusions.rules with path:) disables all of errcheck for that path, which is broader than intended. When a non-terminal writer (file/network) appears we can tighten then.

monit-reviewer · 2026-05-29T19:10:46Z

+snapshot:
+	goreleaser release --snapshot --clean
+
+release:


🔵 Low (harness-engineering:harness-enforcement-reviewer): make release runs goreleaser release --clean with no guard or confirmation step. If a developer has GORELEASER_* tokens or GITHUB_TOKEN set in their shell environment (common after local testing), running this target accidentally publishes a live release. Consider adding a guard or documenting it explicitly as CI-only.

Reply to this thread when addressed.

Fixed — make release is now guarded: it refuses to run unless CONFIRM_RELEASE=1 is set, with a message that it's CI-only. A stray local run with tokens in the environment can no longer publish.

monit-reviewer · 2026-05-29T19:10:46Z

+  push:
+    tags:
+      - "v*"
+  workflow_dispatch:


🔵 Low (harness-engineering:harness-knowledge-reviewer): workflow_dispatch has no ref-type constraint. If a maintainer manually dispatches from a branch ref with dry-run unchecked, GoReleaser will fail because it cannot resolve a semver tag. The dry-run default (true) mitigates accidental publishes but the failure mode is silent until GoReleaser errors. Consider adding an explicit guard checking that github.ref starts with 'refs/tags/'.

Reply to this thread when addressed.

Mitigated by design rather than guarded: the dispatch dry-run defaults to true, and a non-dry-run dispatch from a non-tag ref fails fast with the reusable workflow's explicit tag '…' does not start with prefix 'v' error. A hard refs/tags/ guard in the caller adds surface for a rare manual misuse that already fails loudly; left as-is.

monit-reviewer · 2026-05-29T19:10:47Z

-only, delete-branch-on-merge. Release machinery (`version.txt`, `.goreleaser.yml`,
-`packaging/identity.yml`, the reusable release callers, and the `identity-check`
-required check) arrives in a follow-up.
+`build`/`test`/`lint`/`pr-title`/`identity-check`, signed commits, linear history,


🔵 Low (documentation:docs-reviewer): CLAUDE.md, CONTRIBUTING.md, and README.md all list 'identity-check' as a current required status check, but the PR sequencing notes branch protection gains this check only after this PR merges green. The docs present the target state as current state, which could mislead a contributor setting up branch protection by following these docs before the check is actually enforced.

Reply to this thread when addressed.

Acknowledged — the docs describe the post-merge steady state, which this PR establishes (branch protection gains identity-check immediately after it merges green, per the sequencing note). I'd rather the committed docs reflect the intended required-check set than carry a transient 'coming soon' caveat that's stale within the hour.

monit-reviewer

Automated PR Review

Reviewed commit: d221359 | Previous: 7ca8d19 (incremental)

Summary

No issues found. (2 info-level observations excluded)

10 PR discussion threads considered.

Completed in 1m 31s | $0.62 | sonnet | daemon 0.2.121 | Glorfindel

Field	Value
Model	sonnet
Mode	Re-review · Cycle 2 · Session resumed
Reviewers	hybrid-synthesis, documentation:docs-reviewer, harness-engineering:harness-architecture-reviewer, harness-engineering:harness-enforcement-reviewer, harness-engineering:harness-knowledge-reviewer, harness-engineering:harness-self-documenting-code-reviewer, security:security-code-auditor
Engine	claude · sonnet
Reviewed by	pr-review-daemon · monit-pr-reviewer
Duration	1m 31s wall · 1m 54s compute (Reviewers: 45s · Synthesis: 20s)
Cost	$0.62
Tokens	216.4k in / 8.4k out
Turns	7

Per-workstream usage

Workstream	Model	In	Out	Cache read	Cache create	Cost
hybrid-synthesis	sonnet	47.2k	1.1k	18.6k	28.6k (1h)	$0.13
documentation:docs-reviewer	haiku	16.1k	4.8k	6.1k	10.0k (1h)	$0.04
harness-engineering:harness-architecture-reviewer	sonnet	28.8k	74	2.8k	25.9k (1h)	$0.10
harness-engineering:harness-enforcement-reviewer	sonnet	29.0k	65	2.8k	26.1k (1h)	$0.10
harness-engineering:harness-knowledge-reviewer	sonnet	29.3k	82	2.8k	26.5k (1h)	$0.10
harness-engineering:harness-self-documenting-code-reviewer	sonnet	17.9k	309	2.8k	15.1k (1h)	$0.06
security:security-code-auditor	haiku	20.1k	711	0	20.0k (1h)	$0.03
discussion-summarizer	—	28.0k	1.3k	18.6k	9.4k (1h)	$0.06

_{Re-reviews only run when @monit-reviewer is re-requested as a reviewer — push as many commits as you need, then re-request when ready. PRs targeting branches other than main, master are skipped, even when @monit-reviewer is re-requested.}

rianjs · 2026-05-29T19:16:27Z

No architectural drift.

The post-daemon changes are aligned with the rollout:

Caller dry-run expressions now produce real booleans on both push and dispatch events.
make release is guarded; snapshot remains safe for local proof.
cmd/cr stays a minimal scaffold with tests, not a premature command surface.
Changelog grouping now matches the shared conventional-commit grammar.
make tidy in GoReleaser hooks preserves the “no mutating tidy during release” invariant.
Manifest still declares no stub channels; identity-check timing and controlled ci: merge remain correct.

ci: align changelog groups with full commit grammar and use go fmt

5a2a33f

test: make cr dispatch testable via run() and cover the exit paths

7ca8d19

monit-reviewer suggested changes May 29, 2026

View reviewed changes

ci: harden dry-run expression, guard make release, drop -v alias

d221359

monit-reviewer approved these changes May 29, 2026

View reviewed changes

rianjs merged commit db6e295 into main May 29, 2026
8 checks passed

rianjs deleted the feat/2-release-machinery branch May 29, 2026 19:16

This was referenced May 30, 2026

CR-14a: gate **decision kernel** (pure) #21

Open

CR-14b: gate **IO integration** #22

Open

CR-14c: gate **repair + retry-posts execution** #23

Open

CR-18: review-plan / **action planner** (shared by dry-run + live) #27

Open

Conversation

rianjs commented May 29, 2026

Changes

Channel scope

Verification

Sequencing

Uh oh!

rianjs commented May 29, 2026

Findings

Minor: fmt target does not satisfy the Makefile contract

Minor: changelog grouping does not match the accepted commit grammar

Architecture Check

Uh oh!

rianjs commented May 29, 2026

What's covered

Genuine gap

Proportionality note

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

monit-reviewer left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Uh oh!

rianjs commented May 29, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Minor: `fmt` target does not satisfy the Makefile contract