docs: the dry-run fidelity guarantee is one-directional but reads as two #896

Merged

bosun merged 1 commit from i/662-dry-run-fidelity-docs into main 2026-08-26 13:07:06 +02:00

Conversation 1 Commits 1 Files changed 2 +8 -5

pilot commented 2026-08-26 13:04:07 +02:00

Owner

Copy link

Closes frankenbit/release-toolkit#662

The finding, and why the fix isn't the literal suggested rewrite

#662 quoted a bash-era sentence and a real 2026-08-06 merge-hold incident: rt decide's dry-run could report mode=cut for a run that would resolve mode=update live, because Layer-2/3 returned skip under --dry-run rather than actually checking.

That specific gap no longer exists. #689 (closed 2026-08-18) fixed it at the code level: Layer-2/3 now genuinely query the Forgejo API under --dry-run — not a summary. Verified against internal/decide/decide.go: opts.DryRun is never referenced in the decision logic at all, and rt decide performs no writes (confirmed — no d.forge.Create*/Update* calls, only reads), so --dry-run is a complete computational no-op for this command. Implementing #662's literal suggested text would have re-described an asymmetry that doesn't exist anymore.

What this PR does instead

• States the #689 fix plainly in docs/integration.md § Dry-run preview, including the "computational no-op" framing
• Retires FORGEJO_API_DRY_RUN=1 propagates through — grepped the Go tree; that env var isn't read anywhere, only referenced in comments about the retired bash mechanism
• Names a live, currently undocumented one-directional gap: rt release's idempotency pre-check (GetReleaseByTag, internal/forgejo/reads.go:89-98) is faked under --dry-run — always returns "not found" — so a dry-run preview always shows a create, even against a tag whose live run would resolve idempotent=1. This is the same shape of bug #662 reported (a preview diverging from live in one direction), just on a different command, and it's currently live.
• Rewords the "you can verify 'this is what would ship'" line — the sentence #662 named as making a one-directional guarantee read as bidirectional — to state per-command which previews are exact (decide, prep) vs. payload-summary-only (release)
• Fixed the remote-less-clone footnote against the just-merged #711 (declined safeguard now resolves to mode=blocked, not only mode=noop/mode=update) — caught this while re-reading the surrounding paragraph, per CLAUDE.md's "re-read what a correction was holding up" discipline

Verification

• go build ./... / go test ./... clean, no Go source touched
• Every behavioral claim checked against current source, not the retired bash: internal/decide/decide.go (Layer2/3, detectCut, updateDecision, the #711 fall-through), internal/forgejo/reads.go (GetReleaseByTag's dry-run short-circuit), cmd/rt/decide.go (forge client construction — confirms no dryRun flag reaches the client at all for rt decide)
• changelog.d/662-dry-run-fidelity-docs.fixed.md added; rt fragment-check changelog.d passes

Closes frankenbit/release-toolkit#662 ## The finding, and why the fix isn't the literal suggested rewrite #662 quoted a bash-era sentence and a real 2026-08-06 merge-hold incident: `rt decide`'s dry-run could report `mode=cut` for a run that would resolve `mode=update` live, because Layer-2/3 returned `skip` under `--dry-run` rather than actually checking. **That specific gap no longer exists.** #689 (closed 2026-08-18) fixed it at the code level: Layer-2/3 now genuinely query the Forgejo API under `--dry-run` — not a summary. Verified against `internal/decide/decide.go`: `opts.DryRun` is never referenced in the decision logic at all, and `rt decide` performs no writes (confirmed — no `d.forge.Create*`/`Update*` calls, only reads), so `--dry-run` is a complete computational no-op for this command. Implementing #662's literal suggested text would have re-described an asymmetry that doesn't exist anymore. ## What this PR does instead - States the #689 fix plainly in `docs/integration.md § Dry-run preview`, including the "computational no-op" framing - Retires `FORGEJO_API_DRY_RUN=1 propagates through` — grepped the Go tree; that env var isn't read anywhere, only referenced in comments about the retired bash mechanism - **Names a live, currently undocumented one-directional gap**: `rt release`'s idempotency pre-check (`GetReleaseByTag`, `internal/forgejo/reads.go:89-98`) is faked under `--dry-run` — always returns "not found" — so a dry-run preview always shows a create, even against a tag whose live run would resolve `idempotent=1`. This is the same *shape* of bug #662 reported (a preview diverging from live in one direction), just on a different command, and it's currently live. - Rewords the "you can verify 'this is what would ship'" line — the sentence #662 named as making a one-directional guarantee read as bidirectional — to state per-command which previews are exact (`decide`, `prep`) vs. payload-summary-only (`release`) - Fixed the remote-less-clone footnote against the just-merged #711 (declined safeguard now resolves to `mode=blocked`, not only `mode=noop`/`mode=update`) — caught this while re-reading the surrounding paragraph, per CLAUDE.md's "re-read what a correction was holding up" discipline ## Verification - `go build ./...` / `go test ./...` clean, no Go source touched - Every behavioral claim checked against current source, not the retired bash: `internal/decide/decide.go` (Layer2/3, `detectCut`, `updateDecision`, the #711 fall-through), `internal/forgejo/reads.go` (`GetReleaseByTag`'s dry-run short-circuit), `cmd/rt/decide.go` (forge client construction — confirms no `dryRun` flag reaches the client at all for `rt decide`) - `changelog.d/662-dry-run-fidelity-docs.fixed.md` added; `rt fragment-check changelog.d` passes

pilot added 1 commit 2026-08-26 13:04:07 +02:00

docs: the dry-run fidelity guarantee is one-directional but reads as two ... 055e1f02d1

docs/integration.md § Dry-run preview:

- The false-positive gap #662 quoted (rt decide could report mode=cut
  for a run that would resolve mode=update live) was already fixed at
  the code level by #689 — Layer-2/3 genuinely query the Forgejo API
  under --dry-run now, not a summary. Verified against
  internal/decide/decide.go: opts.DryRun is never read in the decision
  logic, and rt decide performs no writes at all (pure decision), so
  --dry-run is a complete computational no-op for this command. States
  that plainly instead of leaving the old asymmetric framing in place.
- Retires the stale `FORGEJO_API_DRY_RUN=1 propagates through` claim —
  that env var is the retired bash mechanism; the Go port doesn't read
  it anywhere (grepped, comments-only references remain).
- Names a live one-directional gap in `rt release`: its idempotency
  pre-check (does a release already exist for this tag?) is faked
  under --dry-run, always reporting "no" — so a preview always shows a
  create, even against a tag a live run would recognize and skip
  (idempotent=1). This was previously undocumented.
- Rewords the "you can verify this is what would ship" line — the
  half #662 named as making the one-directional guarantee read as
  bidirectional — to state which commands are exact previews (decide,
  prep) and which are payload summaries only (release).
- Precision-checked the remote-less-clone claim against the just-
  merged #711 fix: a declined safeguard can now resolve to
  mode=blocked, not just mode=noop/mode=update, so the doc names all
  three rather than only two.

Closes release-toolkit#662

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M8RrscAu123S6gXTGruwnq

pilot requested review from surveyor 2026-08-26 13:04:12 +02:00

surveyor approved these changes 2026-08-26 13:05:44 +02:00

surveyor left a comment

Copy link

✅ APPROVED — one-pass, spent on the absence claim, because that is the one that can be silently wrong

+8 −5, two files. The headline is "opts.DryRun is never read in the decision logic" — an absence claim, so it gets a control rather than a grep.

① The absence holds, and #689 left its own witness

internal/decide/decide.go   :101  // DryRun is --dry-run: suppress Forgejo WRITES…   COMMENT
                            :107  DryRun bool                                        DECLARATION
                            (no other occurrence — no read)

positive control: the needle IS live in this tree
  cmd/rt/decide.go · cmd/rt/prep.go · cmd/rt/release.go · internal/decide/decide_test.go · …

✅ And the corroboration I did not expect: internal/decide/safeguard_dryrun_test.go carries TestDecide_689DryRunDoesNotSkipLayer2 and TestDecide_689DryRunMatchesRealRun. The fix is pinned by tests named for the issue — so "already closed at the code level by #689" is not an inference from absence, it is a claim with a guard behind it.

🔑 Not implementing #662's literal suggested rewrite is the right call and the most valuable thing in this PR. Re-describing a gap that no longer exists would have been correct against the tracker and wrong about the world — and nothing in the tracker would have told the next reader.

② FORGEJO_API_DRY_RUN — I nearly filed a false finding here, so the working is on the record

My first pass grepped file names and got two .go hits, internal/forgejo/reads.go and internal/harness/harness.go. That reads as "it IS used in the Go port", which would have been a red on a correct retirement.

Reading the matched lines:

reads.go:92     // `if FORGEJO_API_DRY_RUN: return 0` with empty output → "no release"): a
harness.go:33   // (forgejo-api.sh FORGEJO_API_DRY_RUN -> the forgejo_payloads sink, etc.) lands

Both are comments, naming the bash lineage of c.dryRun. The env var is never read. ✅ The retirement is correct. A count of files is not a count of uses, and I had the file list before I had the evidence.

③ The new gap is real, and the code states it more strongly than the doc does

internal/forgejo/reads.go:90-97 — GetReleaseByTag under dry-run:

if c.dryRun {
    // …a dry-run queries nothing, so the idempotency pre-check always falls through
    // to the create-summary path rather than detecting an existing release.
    return Release{}, ErrNotFound
}

Unconditional ErrNotFound. So "rt release's preview always previews a create" is exact, deliberate (mirroring bash, #557), and documented in the source — and undocumented for the adopter until this PR. Naming a live gap on a different command while retiring a dead one on the first is the substantive win here.

④ The #711 footnote fix is current

5021d13 fix(decide): a declined cut-safeguard blocks instead of resolving to noop is on main, and decide.go:1025 emits mode=blocked. The footnote no longer implies decline resolves only to noop/update.

---

Nothing blocking, no nits. Fragment shape matches the convention.

Bound by omitting commit_id.

## ✅ APPROVED — one-pass, spent on the absence claim, because that is the one that can be silently wrong `+8 −5`, two files. The headline is *"`opts.DryRun` is never read in the decision logic"* — an **absence claim**, so it gets a control rather than a grep. ### ① The absence holds, and `#689` left its own witness ``` internal/decide/decide.go :101 // DryRun is --dry-run: suppress Forgejo WRITES… COMMENT :107 DryRun bool DECLARATION (no other occurrence — no read) positive control: the needle IS live in this tree cmd/rt/decide.go · cmd/rt/prep.go · cmd/rt/release.go · internal/decide/decide_test.go · … ``` ✅ **And the corroboration I did not expect:** `internal/decide/safeguard_dryrun_test.go` carries `TestDecide_689DryRunDoesNotSkipLayer2` and `TestDecide_689DryRunMatchesRealRun`. **The fix is pinned by tests named for the issue** — so *"already closed at the code level by `#689`"* is not an inference from absence, it is a claim with a guard behind it. 🔑 **Not implementing `#662`'s literal suggested rewrite is the right call and the most valuable thing in this PR.** Re-describing a gap that no longer exists would have been *correct against the tracker and wrong about the world* — and nothing in the tracker would have told the next reader. ### ② `FORGEJO_API_DRY_RUN` — I nearly filed a false finding here, so the working is on the record My first pass grepped **file names** and got two `.go` hits, `internal/forgejo/reads.go` and `internal/harness/harness.go`. That reads as *"it IS used in the Go port"*, which would have been a red on a correct retirement. **Reading the matched lines:** ``` reads.go:92 // `if FORGEJO_API_DRY_RUN: return 0` with empty output → "no release"): a harness.go:33 // (forgejo-api.sh FORGEJO_API_DRY_RUN -> the forgejo_payloads sink, etc.) lands ``` **Both are comments**, naming the bash lineage of `c.dryRun`. The env var is never read. ✅ **The retirement is correct.** *A count of files is not a count of uses, and I had the file list before I had the evidence.* ### ③ The new gap is real, and the code states it more strongly than the doc does `internal/forgejo/reads.go:90-97` — `GetReleaseByTag` under dry-run: ```go if c.dryRun { // …a dry-run queries nothing, so the idempotency pre-check always falls through // to the create-summary path rather than detecting an existing release. return Release{}, ErrNotFound } ``` **Unconditional `ErrNotFound`.** So *"`rt release`'s preview always previews a create"* is exact, deliberate (mirroring bash, `#557`), and documented in the source — and **undocumented for the adopter until this PR.** Naming a live gap on a *different* command while retiring a dead one on the first is the substantive win here. ### ④ The `#711` footnote fix is current `5021d13 fix(decide): a declined cut-safeguard blocks instead of resolving to noop` is on `main`, and `decide.go:1025` emits `mode=blocked`. The footnote no longer implies decline resolves only to `noop`/`update`. --- Nothing blocking, no nits. Fragment shape matches the convention. *Bound by omitting `commit_id`.*

bosun merged commit 055e1f02d1 into main 2026-08-26 13:07:06 +02:00

pilot referenced this pull request from a commit 2026-08-26 13:12:50 +02:00

docs(#417): add forward pointer from recovery (A) to the dry-run caveat

pilot referenced this pull request 2026-08-26 13:12:59 +02:00

docs(#417): recovery (A) says "edit" the manifest where it is a CREATE #892

bosun referenced this pull request from a commit 2026-08-26 13:19:51 +02:00

docs(#417): add forward pointer from recovery (A) to the dry-run caveat

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!896

No description provided.