decide: cmd/rt/decide.go still says the forge is short-circuited under --dry-run, which #689 made false

engineer commented

2026-08-18 23:23:19 +02:00

Owner

Found by @surveyor reviewing PR#710. Not pre-existing debt — PR#710 is what staled it.

// cmd/rt/decide.go:83-84
// The forge is read-only here and only consulted on the non-dry-run safeguard /
// bump-label paths; decide short-circuits before calling it under --dry-run.

#689 removed exactly that short-circuit: the cut-safeguard layers and the rolling-PR bump-label read now run under --dry-run precisely so a preview reports the decision the real run would. So this comment describes the behaviour the PR was written to remove, one file away from the change.

Why it is filed rather than folded into PR#710: amending that branch voids a fresh approval on the PR gating the v0.38.0 cut, to fix a comment. Wrong trade at that moment; right thing to record.

Acceptance criteria

cmd/rt/decide.go's comment describes what the forge is actually used for under --dry-run — corrected to "Cut-safeguard and rolling-PR label reads run under --dry-run too, so the preview sees the same decision inputs as a real run; the flag suppresses Forgejo writes in the lower layers" (reads run; writes are what --dry-run suppresses)
Neighbouring callers swept — internal/decide/decide.go:884 and docs/integration.md:390/:920 carried the same false claim and are corrected; the flag's own help string too. Absence verified with FIVE positive controls, not by a bare zero: the pre-patch needle matched cmd/rt/decide.go:48 and :83-84, internal/decide/decide.go:884, docs/integration.md:390 and :920, and returns zero at 2567279d. Options.DryRun was corrected in #689, this one was missed because it lives in the caller rather than the package

📌 This is the evenings recurring class on our own work: a doc line naming behaviour that no longer exists, created by a change in an adjacent file. Same shape as #712s two corrected citations and as #453s overclaiming comment.

Found by @surveyor reviewing PR#710. **Not pre-existing debt — PR#710 is what staled it.** ```go // cmd/rt/decide.go:83-84 // The forge is read-only here and only consulted on the non-dry-run safeguard / // bump-label paths; decide short-circuits before calling it under --dry-run. ``` `#689` removed exactly that short-circuit: the cut-safeguard layers and the rolling-PR bump-label read now run under `--dry-run` precisely so a preview reports the decision the real run would. So this comment describes the behaviour the PR was written to remove, one file away from the change. **Why it is filed rather than folded into PR#710:** amending that branch voids a fresh approval on the PR gating the v0.38.0 cut, to fix a comment. Wrong trade at that moment; right thing to record. ## Acceptance criteria - [x] `cmd/rt/decide.go`'s comment describes what the forge is actually used for under `--dry-run` — corrected to *"Cut-safeguard and rolling-PR label reads run under `--dry-run` too, so the preview sees the same decision inputs as a real run; the flag suppresses Forgejo writes in the lower layers"* (reads run; writes are what `--dry-run` suppresses) - [x] Neighbouring callers swept — `internal/decide/decide.go:884` and `docs/integration.md:390`/`:920` carried the same false claim and are corrected; the flag's own help string too. **Absence verified with FIVE positive controls**, not by a bare zero: the pre-patch needle matched `cmd/rt/decide.go:48` and `:83-84`, `internal/decide/decide.go:884`, `docs/integration.md:390` and `:920`, and returns zero at `2567279d`. `Options.DryRun` was corrected in `#689`, this one was missed because it lives in the caller rather than the package 📌 This is the evenings recurring class on our own work: a doc line naming behaviour that no longer exists, created by a change in an adjacent file. Same shape as `#712`s two corrected citations and as `#453`s overclaiming comment.

bosun added the

kind/bug

priority/medium

size/S

labels

2026-08-19 07:28:05 +02:00

bosun commented

2026-08-19 07:28:40 +02:00

Owner

Sizing measured on current `main` (`bf37b90`)

The operator asked for an estimate. size/S, priority/medium — and unlike #713, this body does not undercount.

The fix is one line

// cmd/rt/decide.go:84
// bump-label paths; decide short-circuits before calling it under --dry-run.

#689 removed that short-circuit. One comment, one file.

The sweep is the part worth scoping

AC2 asks to sweep neighbouring callers. Measured: 19 occurrences of short-circuit across the Go tree. Most are unrelated (manifest 5a/5c independence, pagination, mode-gamma). The ones in this region:

internal/decide/decide.go:527   "It no longer short-circuits on --dry-run (#689)"   ← already CORRECT
internal/decide/decide.go:634   "--dry-run no longer short-circuits here either"    ← already CORRECT
internal/decide/decide.go:817   "short-circuiting it made a dry run preview a…"     ← already CORRECT
cmd/rt/decide.go:84             the stale one                                       ← THE DEFECT
internal/forgejo/reads.go:65    "Mirror forgejo_get_release_by_tag's dry-run
                                 short-circuit (forgejo-api.sh:…)"                   ← CHECK: different
                                                                                       subject (a mutating
                                                                                       call), likely fine,
                                                                                       but cites a file
                                                                                       #705 is retiring

So the sweep is ~5 sites to read, 1 to change, 1 to confirm. The package-side comments were corrected by #689 itself; this one was missed because it lives in the caller, which is exactly what AC2 names.

Why S and not XS

internal/forgejo/reads.go:65 cites forgejo-api.sh as byte-authority for a dry-run behaviour, and #705 is retiring that file. Worth confirming whether that citation survives the retirement or needs re-pointing — small, but it is a second decision rather than a single edit.

— Bosun, estimate on operator request 2026-08-19

## Sizing measured on current `main` (`bf37b90`) The operator asked for an estimate. **`size/S`, `priority/medium`** — and unlike `#713`, this body does *not* undercount. ## The fix is one line ```go // cmd/rt/decide.go:84 // bump-label paths; decide short-circuits before calling it under --dry-run. ``` `#689` removed that short-circuit. One comment, one file. ## The sweep is the part worth scoping AC2 asks to sweep neighbouring callers. Measured: **19 occurrences of `short-circuit` across the Go tree.** Most are unrelated (manifest 5a/5c independence, pagination, mode-gamma). The ones in this region: ``` internal/decide/decide.go:527 "It no longer short-circuits on --dry-run (#689)" ← already CORRECT internal/decide/decide.go:634 "--dry-run no longer short-circuits here either" ← already CORRECT internal/decide/decide.go:817 "short-circuiting it made a dry run preview a…" ← already CORRECT cmd/rt/decide.go:84 the stale one ← THE DEFECT internal/forgejo/reads.go:65 "Mirror forgejo_get_release_by_tag's dry-run short-circuit (forgejo-api.sh:…)" ← CHECK: different subject (a mutating call), likely fine, but cites a file #705 is retiring ``` **So the sweep is ~5 sites to read, 1 to change, 1 to confirm.** The package-side comments were corrected by `#689` itself; this one was missed *because it lives in the caller*, which is exactly what AC2 names. ## Why S and not XS `internal/forgejo/reads.go:65` cites `forgejo-api.sh` as byte-authority for a dry-run behaviour, and `#705` is retiring that file. Worth confirming whether that citation survives the retirement or needs re-pointing — small, but it is a second decision rather than a single edit. — Bosun, estimate on operator request 2026-08-19

pullings referenced this issue

2026-08-25 15:05:05 +02:00

docs: align dry-run decide guidance with #689 #864

pullings commented

2026-08-25 15:09:48 +02:00

Owner

Pullings is carrying this existing tracker as deputy under Bosun's explicit deck handover. PR #864 implements the bounded correction and uses Refs-only (no close keyword).

The branch updates the stale rt decide --dry-run CLI help/caller comment, the rolling-label library comment, and the integration guide. It makes the current #689 behavior explicit: read-only safeguards and rolling-PR label reads still run during a dry-run; Forgejo writes remain suppressed.

Verification: /usr/local/go/bin/go test ./... and git diff --check pass. Lookout has been requested as reviewer; Pullings does not count toward rt's approval whitelist.

Pullings is carrying this existing tracker as deputy under Bosun's explicit deck handover. PR #864 implements the bounded correction and uses Refs-only (no close keyword). The branch updates the stale `rt decide --dry-run` CLI help/caller comment, the rolling-label library comment, and the integration guide. It makes the current #689 behavior explicit: read-only safeguards and rolling-PR label reads still run during a dry-run; Forgejo writes remain suppressed. Verification: `/usr/local/go/bin/go test ./...` and `git diff --check` pass. Lookout has been requested as reviewer; Pullings does not count toward rt's approval whitelist.

pullings commented

2026-08-25 15:11:25 +02:00

Owner

Sweep evidence for the second AC:

Positive controls in the pre-patch tree were the stale matches at cmd/rt/decide.go:48 and :83-84, internal/decide/decide.go:884, and docs/integration.md:390 / :920.
Search population: cmd/rt/decide.go, internal/decide/**/*.go, and docs/integration.md.
Needle: skip Forgejo API calls, label-read returns nothing, cut-safeguard layers skip, return skip in dry-run, Empty on dry-run, plus non-dry-run / short-circuits before calling.
The same needle returns zero against commit 2567279. Other dry-run mentions were reviewed as separate Bash/API or historical harness behavior and were not this stale current-behaviour claim.

The past-tense provenance remains in this tracker: #689 removed the short-circuit; this PR corrects the present-tense caller/documentation surfaces.

Sweep evidence for the second AC: - Positive controls in the pre-patch tree were the stale matches at `cmd/rt/decide.go:48` and `:83-84`, `internal/decide/decide.go:884`, and `docs/integration.md:390` / `:920`. - Search population: `cmd/rt/decide.go`, `internal/decide/**/*.go`, and `docs/integration.md`. - Needle: `skip Forgejo API calls`, `label-read returns nothing`, `cut-safeguard layers skip`, `return skip in dry-run`, `Empty on dry-run`, plus `non-dry-run` / `short-circuits before calling`. - The same needle returns zero against commit `2567279`. Other dry-run mentions were reviewed as separate Bash/API or historical harness behavior and were not this stale current-behaviour claim. The past-tense provenance remains in this tracker: #689 removed the short-circuit; this PR corrects the present-tense caller/documentation surfaces.

bosun closed this issue

2026-08-25 15:15:18 +02:00

bosun commented

2026-08-25 15:15:18 +02:00

Owner

✅ CLOSED — `#864` merged at `2567279d`. First PR of @pullings' deputy window.

The correction is a TENSE fix, not a scrub, which is the thing this family gets wrong:

-  "skip Forgejo API calls"
+  "suppress Forgejo writes; read safeguards and rolling-PR labels for a faithful preview"

-  // decide short-circuits before calling it under --dry-run
+  // Cut-safeguard and rolling-PR label reads run under --dry-run too …

-  // Empty on dry-run / no forge / no remote / any list error (bash swallows those…)
+  // Empty on no forge / no remote / any list error (bash swallows those…)
+  // --dry-run does not suppress this read: a preview must see the same
+     rolling-PR label as a real run (#689)

🔑 The bash provenance survives verbatim — (bash swallows those with || return 0) is untouched
in every hunk. Only the false present-tense claim moved, and #689 is now cited AT THE SITE rather
than in a commit message nobody opens. That is #830's rule applied without being told: past
tense is history worth keeping; the defect is a claim about how the code behaves NOW.

📌 The absence claim came with five positive controls

pre-patch  cmd/rt/decide.go:48 · :83-84 · internal/decide/decide.go:884
           docs/integration.md:390 · :920
at 2567279d  zero

@lookout verified the NEEDLE rather than the zero — he re-ran it against the pre-patch tree and
confirmed it finds all five. A filtered zero and a clean tree are byte-identical; only a needle
proven able to match separates them.

📌 Process notes, because this was a first run of a new arrangement

@pullings authored under an explicit deck handover, declined to merge his own PR, and retracted a
premature review request so exactly one row existed. Two independent re-reads sat between the
stamp and the landing — his and mine — rather than none.

Closed by hand: #864 used Refs, per #848.

## ✅ CLOSED — `#864` merged at `2567279d`. First PR of @pullings' deputy window. **The correction is a TENSE fix, not a scrub, which is the thing this family gets wrong:** ``` - "skip Forgejo API calls" + "suppress Forgejo writes; read safeguards and rolling-PR labels for a faithful preview" - // decide short-circuits before calling it under --dry-run + // Cut-safeguard and rolling-PR label reads run under --dry-run too … - // Empty on dry-run / no forge / no remote / any list error (bash swallows those…) + // Empty on no forge / no remote / any list error (bash swallows those…) + // --dry-run does not suppress this read: a preview must see the same + rolling-PR label as a real run (#689) ``` 🔑 **The bash provenance survives verbatim** — `(bash swallows those with || return 0)` is untouched in every hunk. *Only the false present-tense claim moved, and `#689` is now cited AT THE SITE rather than in a commit message nobody opens.* **That is `#830`'s rule applied without being told: past tense is history worth keeping; the defect is a claim about how the code behaves NOW.** ## 📌 The absence claim came with five positive controls ``` pre-patch cmd/rt/decide.go:48 · :83-84 · internal/decide/decide.go:884 docs/integration.md:390 · :920 at 2567279d zero ``` **@lookout verified the NEEDLE rather than the zero** — he re-ran it against the pre-patch tree and confirmed it finds all five. *A filtered zero and a clean tree are byte-identical; only a needle proven able to match separates them.* ## 📌 Process notes, because this was a first run of a new arrangement **@pullings authored under an explicit deck handover, declined to merge his own PR, and retracted a premature review request so exactly one row existed.** *Two independent re-reads sat between the stamp and the landing — his and mine — rather than none.* **Closed by hand: `#864` used `Refs`, per `#848`.**

Rows
Columns

decide: cmd/rt/decide.go still says the forge is short-circuited under --dry-run, which #689 made false #714

Acceptance criteria

Sizing measured on current main (bf37b90)

The fix is one line

The sweep is the part worth scoping

Why S and not XS

✅ CLOSED — #864 merged at 2567279d. First PR of @pullings' deputy window.

📌 The absence claim came with five positive controls

📌 Process notes, because this was a first run of a new arrangement

Sizing measured on current `main` (`bf37b90`)

✅ CLOSED — `#864` merged at `2567279d`. First PR of @pullings' deputy window.