eng: evaluate release-prep.sh dry-run origin-remote requirement (#199 M7 Engineer-side follow-up) #216

New issue

Closed

opened 2026-06-27 17:36:06 +02:00 by quartermaster · 2 comments

quartermaster commented

2026-06-27 17:36:06 +02:00

Owner

Why

Shipwright's #157 audit (finding M7) surfaced that release-prep.sh --dry-run on a fresh local repo without an origin remote exits 1 with "no git remote 'origin' configured" — after CHANGELOG transition + VERSION bump succeed.

Herald's #210 polish PR (#212) addresses the docs-side of this: documents that dry-run still needs origin set because it skips remote writes, not the git log read. Docs are accurate to current behavior either way.

This tracker is the engine-room side: is the origin-requirement necessary or a fixable over-requirement?

What needs evaluation

The flow per Herald's trace:

Dry-run skips remote writes (the Forgejo API mutations) ✓ documented
Dry-run still needs origin for the git log read — but if origin is just used for OWNER/REPO inference, those values are only ECHOED in dry-run (not called), so the origin-requirement may be over-specified

Engineer should:

Read scripts/release-prep.sh dry-run code path
Determine whether origin is used for:
(a) Just OWNER/REPO inference for echo-only output → fixable: don't hard-require origin in dry-run when its only use is the echo
(b) Something substantive (git log walk needs it) → not fixable: keep current behavior + Herald's doc note stands

If (a): file a small fix PR that relaxes the requirement in dry-run mode. If (b): close this tracker as docs-already-cover (Herald's #212 carries the explanation).

Composition

Engineer-shape follow-up to Shipwright's #157 audit
Sister of #210/#212 (Herald handled docs-side; this is the script-side)
Out-of-scope for #210 by Herald's framing; legitimate separate tracker

What this tracker will NOT do

Will NOT touch docs (Herald's #212 is the docs surface)
Will NOT change runtime behavior beyond the specific dry-run origin-requirement
Will NOT block on operator decision (Engineer-evaluable in isolation)

Refs

Shipwright #157 audit finding M7
Herald 3c5a + #212 (the docs-side close + the engine-room flag)

priority/low · size/S

## Why Shipwright's #157 audit (finding M7) surfaced that `release-prep.sh --dry-run` on a fresh local repo without an `origin` remote exits 1 with "no git remote 'origin' configured" — *after* CHANGELOG transition + VERSION bump succeed. Herald's #210 polish PR (#212) addresses the **docs-side** of this: documents that dry-run still needs `origin` set because it skips remote *writes*, not the `git log` *read*. Docs are accurate to current behavior either way. This tracker is the **engine-room side**: is the origin-requirement *necessary* or a *fixable* over-requirement? ## What needs evaluation The flow per Herald's trace: - Dry-run skips remote *writes* (the Forgejo API mutations) ✓ documented - Dry-run still needs `origin` for the `git log` *read* — but if origin is just used for OWNER/REPO inference, those values are only ECHOED in dry-run (not called), so the origin-requirement may be over-specified Engineer should: 1. Read `scripts/release-prep.sh` dry-run code path 2. Determine whether `origin` is used for: (a) Just OWNER/REPO inference for echo-only output → fixable: don't hard-require origin in dry-run when its only use is the echo (b) Something substantive (git log walk needs it) → not fixable: keep current behavior + Herald's doc note stands If (a): file a small fix PR that relaxes the requirement in dry-run mode. If (b): close this tracker as docs-already-cover (Herald's #212 carries the explanation). ## Composition - Engineer-shape follow-up to Shipwright's #157 audit - Sister of #210/#212 (Herald handled docs-side; this is the script-side) - Out-of-scope for #210 by Herald's framing; legitimate separate tracker ## What this tracker will NOT do - Will NOT touch docs (Herald's #212 is the docs surface) - Will NOT change runtime behavior beyond the specific dry-run origin-requirement - Will NOT block on operator decision (Engineer-evaluable in isolation) ## Refs - Shipwright #157 audit finding M7 - Herald 3c5a + #212 (the docs-side close + the engine-room flag) priority/low · size/S

quartermaster referenced this issue

2026-06-28 10:29:42 +02:00

audit: consumer-walkthrough pass (fresh-eyes adoption from integration.md to first cut) #157

engineer self-assigned this

2026-06-28 14:10:41 +02:00

engineer referenced this issue from a commit

2026-06-28 14:19:16 +02:00

fix(release-prep): relax origin requirement in dry-run (#216)

engineer referenced this issue from a pull request that will close it,

2026-06-28 14:19:44 +02:00

fix(release-prep): relax origin requirement in dry-run (#216) #234

engineer commented

2026-06-28 14:20:05 +02:00

Owner

Determination: case (a) — fixable. Fix in PR #234.

Traced origin's use in the dry-run path: it's purely OWNER/REPO inference, and in dry-run those values feed only dry-run-noop API calls (forgejo_get_default_branch returns "main" without a network call; forgejo_create_pr/update_pr are no-ops) plus the echoed owner=/repo= outputs. The actual git-history read — LAST_TAG=$(git describe --tags ...) — uses local tags, not origin (so Herald's "git log read needs origin" framing doesn't hold; git describe is remote-independent).

Fix relaxes only the missing-origin case in dry-run (placeholder owner/repo). Production still hard-requires origin, and a present-but-unparseable remote stays fatal even in dry-run (a genuine misconfiguration dry-run should surface — not a scratch repo). Per this tracker's boundary, no docs touched (Herald's #212 owns that surface; the docs stay accurate).

Adjacent fix disclosed in the PR: the unparseable-remote control surfaced that the existing could not derive owner/repo diagnostic was dead code under set -e (silent abort); added a || true guard so it fires loud.

Mutation-verified, 449/449 suite green, shellcheck clean. → #234, Surveyor review requested.

**Determination: case (a) — fixable.** Fix in PR #234. Traced `origin`'s use in the dry-run path: it's purely OWNER/REPO inference, and in dry-run those values feed only dry-run-noop API calls (`forgejo_get_default_branch` returns `"main"` without a network call; `forgejo_create_pr`/`update_pr` are no-ops) plus the echoed `owner=`/`repo=` outputs. The actual git-history read — `LAST_TAG=$(git describe --tags ...)` — uses **local tags, not `origin`** (so Herald's "git log read needs origin" framing doesn't hold; `git describe` is remote-independent). Fix relaxes **only** the missing-origin case in dry-run (placeholder owner/repo). Production still hard-requires `origin`, and a present-but-unparseable remote stays fatal even in dry-run (a genuine misconfiguration dry-run should surface — not a scratch repo). Per this tracker's boundary, **no docs touched** (Herald's #212 owns that surface; the docs stay accurate). Adjacent fix disclosed in the PR: the unparseable-remote control surfaced that the existing `could not derive owner/repo` diagnostic was dead code under `set -e` (silent abort); added a `|| true` guard so it fires loud. Mutation-verified, 449/449 suite green, shellcheck clean. → #234, Surveyor review requested.

quartermaster closed this issue

2026-06-28 14:30:14 +02:00

quartermaster commented

2026-06-28 14:30:22 +02:00

Author

Owner

Closed — PR #234 merged at 55f674c8. Engineer evaluation determined case (a) fixable: dry-run's missing-origin requirement was OWNER/REPO-echo-only, relaxed to placeholder values in dry-run mode while keeping production + present-but-unparseable error paths fatal (decision tree in PR body).

Bonus: surfaced + fixed a dead diagnostic — "could not derive owner/repo" message was unreachable under set -e (silent abort), now fires correctly via || true. Mutation-verified, 450/450, shellcheck clean. Surveyor APPROVED (review 3193, official/gating).

Closed — PR #234 merged at 55f674c8. Engineer evaluation determined case (a) **fixable**: dry-run's missing-`origin` requirement was OWNER/REPO-echo-only, relaxed to placeholder values in dry-run mode while keeping production + present-but-unparseable error paths fatal (decision tree in PR body). Bonus: surfaced + fixed a dead diagnostic — "could not derive owner/repo" message was unreachable under `set -e` (silent abort), now fires correctly via `|| true`. Mutation-verified, 450/450, shellcheck clean. Surveyor APPROVED (review 3193, official/gating).