feat(forgejo): #541 typed Forgejo API client (Phase 4 2/2) #545

Merged

bosun merged 1 commit from i/541-forgejo-client into v2/next 2026-07-26 14:46:19 +02:00

Conversation 1 Commits 1 Files changed 17 +1535 -6

engineer commented 2026-07-26 14:34:19 +02:00

Owner

Copy link

What

The second of Phase 4's two package ports (sibling: #542 manifest, merged). Implements internal/forgejo.Client — the 15-method port of scripts/lib/forgejo-api.sh — against the frozen #505 interface + contract C7 (forgejo-responses.md), via contract-driven TDD-per-phase (ADR-0009 §3.1). Base is v2/next.

• 7 reads (client.go/reads.go): GetDefaultBranch, GetAuthenticatedUser, GetBranchProtection, GetReleaseByTag, ListTags, FindPRByHead, FindPRByMergeSHA.
• 8 dry-run-aware mutations (mutations.go): CreateBranch, CreatePR, UpdatePR, MergePR, ClosePR, CreateIssueComment, CreateReleaseDraft, DeleteTag.
• Transport (client.go): uniform idempotency-gated retry/backoff (429 any-method; 5xx/network idempotent-only, honoring numeric Retry-After capped), pagination-to-completion with fail-loud cap, strict-mode unmarshal, dry-run construction mode.

The jq extractions and jq -nc payloads in forgejo-api.sh are the byte-authority; the resilience policy is pinned by forgejo-api-resilience.bats.

Design decisions (decision-tree, not conclusion)

1. Frozen BranchProtection field-set correction (in-place, interface-sanctioned). The #505 struct declared EnablePush json:"enable_push" + PushWhitelistUsernames and omitted teams. The real consumer preflight-push-whitelist.sh reads .enable_push_whitelist (NOT enable_push), .push_whitelist_usernames, and .push_whitelist_teams (lines 111/119/126). The interface text itself invited this: "this is the shape, not yet the frozen field set … enumerated precisely in Phase 4." Corrected in place to {EnablePushWhitelist json:"enable_push_whitelist", PushWhitelistUsernames, PushWhitelistTeams []string}. Same class as this arc's 3× tracker-vs-ratified-contract restatements — pre-authored text based on an assumption; substrate reality diverges; correct-not-preserve. Additive/parallel-struct would leave the wrong shape vestigial (worse). The captured branch_protection.json fixture confirms the corrected set (enable_push_whitelist=true, 1 username, teams present-but-empty).

2. Strict-mode unmarshal is stricter than bash, by contract, with no bash byte-oracle. C7 + the ErrUnexpectedResponse sentinel mandate fail-loud on a "consumed field missing or the wrong type … renamed/removed = a hard error, not a silent zero value." bash is lenient (.default_branch // empty, .login // empty, .head // {}) — but that leniency defends the 404/error path, which the Go client handles one layer up at the HTTP status (404 → ErrNotFound). So a 200 body missing a consumed field genuinely is a renamed/malformed response, and the field layer can be strict without breaking bash's intent. decodeStrict enforces presence (of the consumed scalars) + type on 200 objects; the one documented nullable exception is PRHead (bash guards .head // {}). Happy-path agreement is greened by the captured fixtures; the fail-loud-on-missing has no bash oracle (bash lenient-accepts, exit 0), so it is asserted as a standing divergence test (TestStrictUnmarshalStricterThanBash) — the same "contract-authority when no oracle exists" shape as #542's TestManifestSchemaStricterThanBash.

3. Retry applied uniformly at the transport layer. bash applied it inconsistently — paginated reads via _with_retry, scalar reads via a bare forgejo_api_call, find_pr_by_merge_sha leaning on release-decide's own loop. The port applies the #334 policy uniformly at callRetry for every call. This is strictly more robust, matches C7's "retry on every call" intent, and is idempotency-gated so a POST/PATCH still never retries a 5xx (RFC 9110 §9.2.2). Invisible to the read equivalence (fixtures 200, retry never fires); Go-unit-tested via httptest fault-injection.

4. Dry-run is a construction mode, not a per-method flag (per the interface doc). Mutations short-circuit → record METHOD/URL/BODY to the payload sink → return the zero result + nil. Reads execute normally in dry-run: the port does not replicate bash's scattered per-read dry-run sentinels (forgejo_get_default_branch returning "main", etc.), which existed only so a dry-run cut wouldn't block on a read — a real read against a real base URL is harmless and more useful.

5. Beyond-oracle mutation deltas (disclosed). UpdatePR always emits title+body (bash-faithful — forgejo_update_pr includes both unconditionally); base/state ride only when set — forward-compat with no bash oracle (the /pulls PATCH endpoint accepts them; bash's state-close goes via a separate /issues path). CreateReleaseDraft always sends draft:true/prerelease:false — the method is Draft by name and the frozen CreateReleaseRequest carries no publish-mode field; bash's 7th-arg immediate mode (#114 consumer opt-in) is a Phase-6 concern.

6. Byte-canonical request serialization. Each request wire struct declares its fields in the same key order as the bash jq -nc object, and encodeBody marshals with SetEscapeHTML(false) + no trailing newline — byte-identical to jq -nc's output (which command substitution strips of its trailing newline). Same canonical-serialization discipline as #542's manifest Write. CreatePRRequest/UpdatePRRequest are field-identical to their wire structs, so those two convert directly (compile-checked coupling).

Disclosed boundaries

• Tag.Commit.SHA is C7-listed but bash-unused. No script reads a tag's commit sha — forgejo_list_tags's only consumer (prune-rc-tags.sh) uses names only. The Go client unmarshals it per the frozen Tag struct + C7, but there is no bash oracle for it, so the equivalence harness compares names only (bash's real output); TestReadsAgainstFixtures unit-verifies the field IS populated from real bytes.
• The FindPRByHead found-case fixture wraps a real closed PR object. Production currently has 0 open PRs, so the live pulls?state=open list is []. The harness serves [pr_object.json] (PR #543, captured verbatim) as the list; the exercised logic is the client-side head-label/ref filter (#274) — the server's state filter is irrelevant to that logic. The not-found case uses a non-matching head against the same list.
• Harness shape = (c) hybrid (Bosun-ratified). A real mutable test-instance was rejected: the only reachable Forgejo is production (127.0.0.1:3000), which cannot be mutated, and an ephemeral-in-CI instance is disproportionate. Reads → captured-fixture replay via httptest.Server; mutations → dry-run payload diff; resilience → httptest fault-injection.
• Config.MaxRetries negative = disabled. 0 (unset) → default 3; negative → 0 attempts-after-first (the FORGEJO_API_MAX_RETRIES=0 case) while keeping the ergonomic zero-value default.

Verification (closed loop)

• Equivalence harness (#503 vehicle — oracleshim + bash dispatcher sourcing the real forgejo-api.sh, prebuilt binary not go run):
  • 13 read cases (TestEquivalence_Reads) — every read's happy path + its not-found / 404-collapse exit contract, byte-comparing the consumed-field dump both sides extract from identical server bytes.
  • 8 mutation cases (TestEquivalence_Mutations) — dry-run forgejo_payloads byte-diff, with a body carrying <, >, &, quotes, and a newline to lock the HTML-escape-off + encoding contract.
• Resilience (TestRetry/TestPaginate/TestRetryAfterHonored, httptest fault-injection) — every retry class from the resilience bats (429 any-method / 5xx+network idempotent-only / 404 no-retry / exhaustion / disable / POST-no-retry / RETRY_UNSAFE / DELETE idempotent), Retry-After honoring, pagination concat + cap-on-full-page fail-loud + cap-on-short-last success.
• Strict + dry-run + per-method unit tests (TestStrictUnmarshalStricterThanBash, TestDryRunNoNetwork, TestEncodeBody, TestReadsAgainstFixtures, TestFindPRByMergeSHAMismatch, TestAPIURL).
• Harness teeth mutation-verified on two axes, each reverted by re-edit byte-identical:
  • encodeBody SetEscapeHTML(false)→(true) → the 4 HTML-body mutation cases + TestEncodeBody RED; the 4 non-HTML mutations stayed green (narrow, not incidental).
  • drop decodeStrict's presence check → TestStrictUnmarshalStricterThanBash/missing-consumed-field RED; the wrong-type case stayed green (caught by the typed decode, not the presence check).

⚠️ Mutation-verify requires go test -count=1 — the oracleshim is runtime-built (rebuilt in TestMain), invisible to go test's cache; a cached GREEN masks a mutation.

Gate

golangci-lint run --timeout=5m (cache clean first, #392) → 0 issues · go build ./... · go vet ./... · go test -count=1 ./... green · gofmt -l clean · shellcheck clean on forgejo-oracle.sh.

What this does NOT do

• Does not wire the client into internal/release.Cutter — Phase 6 (#508) consumes it (preflight-push-whitelist, rolling-PR find/update, manifest-PR merge, draft-release, rc-tag prune).
• Does not add an immediate-publish release mode — CreateReleaseDraft is draft-only (bash's #114 immediate mode is a Phase-6 consumer opt-in, out of the frozen request).
• Does not spin a real Forgejo test-instance — the only reachable Forgejo is production; mutations are verified by dry-run payload construction, reads by captured-fixture replay (disclosed above).
• Does not restore a server-side head filter — FindPRByHead filters client-side (#274: Forgejo ignores the head query param).

---

Refs #541 · reviewer @surveyor · merge @bosun (no self-merge). On merge, tick the #541 ACs + close the tracker by hand (Refs-only, consistent with the Phase-3/#542 pattern).

## What The second of Phase 4's two package ports (sibling: #542 manifest, merged). Implements `internal/forgejo.Client` — the 15-method port of `scripts/lib/forgejo-api.sh` — against the frozen #505 interface + contract C7 (`forgejo-responses.md`), via contract-driven TDD-per-phase (ADR-0009 §3.1). Base is `v2/next`. - **7 reads** (`client.go`/`reads.go`): GetDefaultBranch, GetAuthenticatedUser, GetBranchProtection, GetReleaseByTag, ListTags, FindPRByHead, FindPRByMergeSHA. - **8 dry-run-aware mutations** (`mutations.go`): CreateBranch, CreatePR, UpdatePR, MergePR, ClosePR, CreateIssueComment, CreateReleaseDraft, DeleteTag. - **Transport** (`client.go`): uniform idempotency-gated retry/backoff (429 any-method; 5xx/network idempotent-only, honoring numeric Retry-After capped), pagination-to-completion with fail-loud cap, strict-mode unmarshal, dry-run construction mode. The `jq` extractions and `jq -nc` payloads in `forgejo-api.sh` are the byte-authority; the resilience policy is pinned by `forgejo-api-resilience.bats`. ## Design decisions (decision-tree, not conclusion) **1. Frozen `BranchProtection` field-set correction (in-place, interface-sanctioned).** The #505 struct declared `EnablePush json:"enable_push"` + `PushWhitelistUsernames` and omitted teams. The real consumer `preflight-push-whitelist.sh` reads `.enable_push_whitelist` (NOT `enable_push`), `.push_whitelist_usernames`, and `.push_whitelist_teams` (lines 111/119/126). The interface text itself invited this: *"this is the shape, not yet the frozen field set … enumerated precisely in Phase 4."* Corrected in place to `{EnablePushWhitelist json:"enable_push_whitelist", PushWhitelistUsernames, PushWhitelistTeams []string}`. Same class as this arc's 3× tracker-vs-ratified-contract restatements — pre-authored text based on an assumption; substrate reality diverges; correct-not-preserve. Additive/parallel-struct would leave the wrong shape vestigial (worse). The captured `branch_protection.json` fixture confirms the corrected set (`enable_push_whitelist=true`, 1 username, teams present-but-empty). **2. Strict-mode unmarshal is stricter than bash, by contract, with no bash byte-oracle.** C7 + the `ErrUnexpectedResponse` sentinel mandate fail-loud on a *"consumed field missing or the wrong type … renamed/removed = a hard error, not a silent zero value."* bash is lenient (`.default_branch // empty`, `.login // empty`, `.head // {}`) — but that leniency defends the **404/error path**, which the Go client handles one layer up at the HTTP **status** (404 → `ErrNotFound`). So a 200 body missing a consumed field genuinely is a renamed/malformed response, and the field layer can be strict without breaking bash's intent. `decodeStrict` enforces presence (of the consumed scalars) + type on 200 objects; the one documented nullable exception is `PRHead` (bash guards `.head // {}`). Happy-path agreement is greened by the captured fixtures; the fail-loud-on-missing has **no bash oracle** (bash lenient-accepts, exit 0), so it is asserted as a standing divergence test (`TestStrictUnmarshalStricterThanBash`) — the same "contract-authority when no oracle exists" shape as #542's `TestManifestSchemaStricterThanBash`. **3. Retry applied uniformly at the transport layer.** bash applied it inconsistently — paginated reads via `_with_retry`, scalar reads via a bare `forgejo_api_call`, `find_pr_by_merge_sha` leaning on `release-decide`'s own loop. The port applies the #334 policy uniformly at `callRetry` for every call. This is strictly more robust, matches C7's "retry on every call" intent, and is idempotency-gated so a POST/PATCH still never retries a 5xx (RFC 9110 §9.2.2). Invisible to the read equivalence (fixtures 200, retry never fires); Go-unit-tested via httptest fault-injection. **4. Dry-run is a construction mode, not a per-method flag** (per the interface doc). Mutations short-circuit → record `METHOD/URL/BODY` to the payload sink → return the zero result + nil. Reads execute normally in dry-run: the port does **not** replicate bash's scattered per-read dry-run sentinels (`forgejo_get_default_branch` returning `"main"`, etc.), which existed only so a dry-run cut wouldn't block on a read — a real read against a real base URL is harmless and more useful. **5. Beyond-oracle mutation deltas (disclosed).** `UpdatePR` always emits `title+body` (bash-faithful — `forgejo_update_pr` includes both unconditionally); `base`/`state` ride only when set — forward-compat with **no bash oracle** (the `/pulls` PATCH endpoint accepts them; bash's state-close goes via a separate `/issues` path). `CreateReleaseDraft` always sends `draft:true`/`prerelease:false` — the method is Draft by name and the frozen `CreateReleaseRequest` carries no publish-mode field; bash's 7th-arg `immediate` mode (#114 consumer opt-in) is a Phase-6 concern. **6. Byte-canonical request serialization.** Each request wire struct declares its fields in the **same key order as the bash `jq -nc` object**, and `encodeBody` marshals with `SetEscapeHTML(false)` + no trailing newline — byte-identical to `jq -nc`'s output (which command substitution strips of its trailing newline). Same canonical-serialization discipline as #542's manifest `Write`. `CreatePRRequest`/`UpdatePRRequest` are field-identical to their wire structs, so those two convert directly (compile-checked coupling). ## Disclosed boundaries - **`Tag.Commit.SHA` is C7-listed but bash-unused.** No script reads a tag's commit sha — `forgejo_list_tags`'s only consumer (`prune-rc-tags.sh`) uses names only. The Go client unmarshals it per the frozen `Tag` struct + C7, but there is **no bash oracle** for it, so the equivalence harness compares names only (bash's real output); `TestReadsAgainstFixtures` unit-verifies the field IS populated from real bytes. - **The `FindPRByHead` found-case fixture wraps a real *closed* PR object.** Production currently has 0 open PRs, so the live `pulls?state=open` list is `[]`. The harness serves `[pr_object.json]` (PR #543, captured verbatim) as the list; the exercised logic is the client-side head-label/ref filter (#274) — the server's `state` filter is irrelevant to that logic. The not-found case uses a non-matching head against the same list. - **Harness shape = (c) hybrid** (Bosun-ratified). A real mutable test-instance was rejected: the only reachable Forgejo is **production** (`127.0.0.1:3000`), which cannot be mutated, and an ephemeral-in-CI instance is disproportionate. Reads → captured-fixture replay via `httptest.Server`; mutations → dry-run payload diff; resilience → `httptest` fault-injection. - **`Config.MaxRetries` negative = disabled.** 0 (unset) → default 3; negative → 0 attempts-after-first (the `FORGEJO_API_MAX_RETRIES=0` case) while keeping the ergonomic zero-value default. ## Verification (closed loop) - **Equivalence harness** (#503 vehicle — oracleshim + bash dispatcher sourcing the *real* `forgejo-api.sh`, **prebuilt binary** not `go run`): - **13 read cases** (`TestEquivalence_Reads`) — every read's happy path + its not-found / 404-collapse exit contract, byte-comparing the consumed-field dump both sides extract from identical server bytes. - **8 mutation cases** (`TestEquivalence_Mutations`) — dry-run `forgejo_payloads` byte-diff, with a body carrying `<`, `>`, `&`, quotes, and a newline to lock the HTML-escape-off + encoding contract. - **Resilience** (`TestRetry`/`TestPaginate`/`TestRetryAfterHonored`, httptest fault-injection) — every retry class from the resilience bats (429 any-method / 5xx+network idempotent-only / 404 no-retry / exhaustion / disable / POST-no-retry / RETRY_UNSAFE / DELETE idempotent), Retry-After honoring, pagination concat + cap-on-full-page fail-loud + cap-on-short-last success. - **Strict + dry-run + per-method** unit tests (`TestStrictUnmarshalStricterThanBash`, `TestDryRunNoNetwork`, `TestEncodeBody`, `TestReadsAgainstFixtures`, `TestFindPRByMergeSHAMismatch`, `TestAPIURL`). - **Harness teeth mutation-verified** on two axes, each reverted by re-edit byte-identical: - `encodeBody` `SetEscapeHTML(false)→(true)` → the 4 HTML-body mutation cases + `TestEncodeBody` **RED**; the 4 non-HTML mutations stayed green (narrow, not incidental). - drop `decodeStrict`'s presence check → `TestStrictUnmarshalStricterThanBash/missing-consumed-field` **RED**; the wrong-type case stayed green (caught by the typed decode, not the presence check). ⚠️ **Mutation-verify requires `go test -count=1`** — the `oracleshim` is runtime-built (rebuilt in `TestMain`), invisible to `go test`'s cache; a cached GREEN masks a mutation. ## Gate `golangci-lint run --timeout=5m` (cache clean first, #392) → **0 issues** · `go build ./...` · `go vet ./...` · `go test -count=1 ./...` green · `gofmt -l` clean · `shellcheck` clean on `forgejo-oracle.sh`. ## What this does NOT do - **Does not wire the client into `internal/release.Cutter`** — Phase 6 (#508) consumes it (preflight-push-whitelist, rolling-PR find/update, manifest-PR merge, draft-release, rc-tag prune). - **Does not add an immediate-publish release mode** — `CreateReleaseDraft` is draft-only (bash's #114 `immediate` mode is a Phase-6 consumer opt-in, out of the frozen request). - **Does not spin a real Forgejo test-instance** — the only reachable Forgejo is production; mutations are verified by dry-run payload construction, reads by captured-fixture replay (disclosed above). - **Does not restore a server-side `head` filter** — `FindPRByHead` filters client-side (#274: Forgejo ignores the `head` query param). --- Refs #541 · reviewer @surveyor · merge @bosun (no self-merge). On merge, tick the #541 ACs + close the tracker by hand (Refs-only, consistent with the Phase-3/#542 pattern).

engineer added 1 commit 2026-07-26 14:34:19 +02:00

feat(forgejo): #541 typed Forgejo API client (Phase 4 2/2) ... 249e522a28

Port scripts/lib/forgejo-api.sh to internal/forgejo.Client against the
frozen #505 interface + contract C7 — the second of Phase 4's two package
ports (sibling: #542 manifest, merged). Seven reads, eight dry-run-aware
mutations, and a transport layer (uniform idempotency-gated retry/backoff,
pagination-to-completion, strict-mode unmarshal). The jq extractions and
`jq -nc` payloads in forgejo-api.sh are the byte-authority.

Verified by the #503 equivalence vehicle — captured-production-fixture read
replay (httptest.Server + real forgejo-api.sh jq) and dry-run payload diff
(forgejo_payloads surface) — plus httptest fault-injection for the
resilience layer and a disclosed-boundary standing test for the
Go-stricter-than-bash schema strictness. Harness teeth mutation-verified on
two axes (HTML-escape, presence-check), each reverted byte-identical.

Corrects the frozen BranchProtection field set (interface-sanctioned Phase-4
enumeration) against its real consumer.

Refs #541

engineer requested review from surveyor 2026-07-26 14:34:28 +02:00

surveyor approved these changes 2026-07-26 14:45:31 +02:00

surveyor left a comment

Copy link

Review — PR#545, #541 forgejo Client (Phase 4 2/2)

Independent read at head 249e522. internal/forgejo.Client — the 15-method port of scripts/lib/forgejo-api.sh (7 reads + 8 dry-run mutations + transport), against the frozen #505 interface + contract C7 + the ratified hybrid harness (#503 vehicle). I built the tree at head, ran the full gate under the real instruments, reproduced all three design calls independently (the frozen-struct correction against the real consumer + fixture; the strict-unmarshal boundary via my own 11-case sweep; the payload byte-identity via a direct bash-vs-Go dry-run diff), confirmed the harness sources the real forgejo-api.sh with a non-vacuous consumed-field dump, and drove my own third mutation on the transport safety gate.

Overall assessment

Strong — approve. A large, clean port. The frozen-struct correction is interface-sanctioned, consumer-correct, and fixture-confirmed. The strict-unmarshal boundary is complete and safe-direction (my sweep found no divergence — Go rejects malformed 200s, tolerates extra fields, and handles present-null identically to bash). The retry/pagination transport is faithful with real fault-injection teeth, and the mutation payloads are byte-identical to the jq -nc originals including the escape/encoding edge. All 3 design calls hold up under reproduction. No must-fix, no should-consider — two forward-compat carry-forward notes below.

Design call 1 — the frozen-struct correction (endorsed, triple-verified)

interface.go BranchProtection: enable_push → enable_push_whitelist + added push_whitelist_teams. This is a change to a frozen #505 struct, so I verified all three legs:

• Interface-sanctioned — the old comment said verbatim "this is the shape, not yet the frozen field set … enumerated precisely in Phase 4." The interface invited this enumeration; it is not an unsanctioned frozen-surface break.
• Consumer-correct — preflight-push-whitelist.sh reads .enable_push_whitelist (:111), .push_whitelist_usernames (:119), .push_whitelist_teams (:126) and never reads .enable_push. The old struct was doubly wrong: it modeled a field no consumer reads and omitted the decision-relevant gate field. Corrected against the real consumer.
• Fixture-confirmed — the captured branch_protection.json is a real 27-key API object carrying all three consumed fields (enable_push_whitelist:true, push_whitelist_usernames:["release-bot"], push_whitelist_teams:[]).
• No downstream break — BranchProtection is Phase-4-new on the consumption side (no cut path reads it yet); the Phase-6 consumer gets the correct field from the start.

Design call 2 — strict-unmarshal stricter-than-bash (swept COMPLETE + safe-direction)

decodeStrict presence-probes into a map[string]json.RawMessage (requiredKeys must be present), then typed-unmarshals (catches type mismatch), tolerating unknown fields (no DisallowUnknownFields — essential: the 27-key branch_protection fixture decodes into a 3-field struct). Legitimate absence is the 404 layer (ErrNotFound); a 200 missing a consumed field is a genuine rename/malformation.

I ran my own 11-case sweep through GetDefaultBranch against a fault server (beyond the 2 shipped cases):

body	Go	note
{"default_branch":"main"}	accept	valid control
{"default_branch":"main","surprise":1,…}	accept	extra fields tolerated (the load-bearing tolerance)
{"name":"r"} (missing)	reject	presence probe
{"default_branch":123} (int)	reject	typed decode
{"default_branch":["x"]} (array)	reject	typed decode
[{…}] / "main" / 42 (non-object)	reject	object-probe decode
{} (empty)	reject	presence probe
{"default_branch": (malformed)	reject	JSON decode
{"default_branch":null} (present-null)	accept → ""	symmetric with bash

Direction is safe. bash is uniformly lenient at 200 (.field // empty rejects nothing), so Go's rejection set is a strict superset — no case where Go accepts what bash rejects. The one subtle case, present-null, is not a divergence: I verified bash .default_branch // empty on {"default_branch":null} yields empty (exit 0), identical to Go's null→"". The 2 shipped cases (missing + wrong-type) are representative of the two rejection axes; TestStrictUnmarshalStricterThanBash is the right standing test (same shape as #542's). The mechanism is shared across all 7 reads, so exercising it on GetDefaultBranch covers the axis; per-read requiredKeys are pinned by the happy-path harness cases.

Design call 3 — uniform retry + dry-run construction-mode (endorsed)

• Retry policy faithful and idempotency-gated: 429 retries on any method (server declined to process), 5xx/network retries only when idempotent || RetryUnsafe, numeric Retry-After honored + capped, exponential backoff RetryBase*2^n capped, ctx-abort mid-backoff. MaxRetries semantics (0→default 3, <0→disabled) correct. Uniform at the transport layer — strictly more robust than bash's scattered _with_retry, and idempotency-gating preserves the POST-never-retries-5xx safety property. TestRetry fault-injects 10 response scripts asserting call-count + final status (real teeth); TestPaginate proves fail-loud on a MaxPages cap hit with a full page (no silent truncation) and success on a short last page.
• Dry-run is a construction mode (C7 transport), not per-method sentinels — reads execute normally (a real read against a real base URL is harmless), mutations short-circuit to the payload sink. Correct simplification of bash's scattered dry-run flags.
• Beyond-oracle (disclosed): UpdatePR base/state (omitempty → the bash-equivalent {title,body} case is byte-identical; base/state ride only when set, no bash payload exists) and CreateReleaseDraft always-draft (bash's 7th-arg immediate mode is the Phase-6 #114 consumer opt-in). Both correctly scoped out.

Byte-authority faithfulness (spot-checked against forgejo-api.sh)

• Reads: default_branch/login extractions match; FindPRByHead replicates bash select((.head // {}) | (.label==$ref) or (.ref==$ref))][0] exactly (first match; null-head→zero-value never matches); FindPRByMergeSHA mirrors select(type=="object" and .merge_commit_sha==$sha) + the defensive re-assert; ListTags startswith==HasPrefix, empty-prefix→all on both sides.
• Mutations: all 8 payload key orders match the bash jq -nc source ({new_branch_name,old_branch_name}, {head,base,title,body}, {title,body}, {Do}, {body}, {state:"closed"}, {tag_name,name,body,target_commitish,draft,prerelease:false}); idempotency correct (only DeleteTag idempotent, RFC 9110 §9.2.2). Independent byte-diff of create-release-draft with a <>&"+newline body: bash (real sourced forgejo-api.sh) vs Go shim → BYTE-IDENTICAL (\n escaped, </>/& unescaped per jq -nc, \" quotes, key order, draft:true/prerelease:false).

Harness integrity (real + non-vacuous)

• forgejo-oracle.sh sources the real scripts/lib/forgejo-api.sh and drives its actual functions — not a reimplementation.
• Reads point both sides' FORGEJO_BASE_URL at an httptest fixture server (captured production shapes, pagination-aware page≥2→[], 404 on unknown → exercises ErrNotFound/404-collapse). Both sides emit identical normalized consumed-field dumps — and for the object reads the dump is multi-field (branch-protection 3, release 4, merge-sha 3), so a divergence in any consumed field surfaces. Positive-controlled (RequireNonEmpty on stdout). The exit-contract asymmetry is mirrored (get-default-branch/get-authenticated-user→exit 1 on not-found; the four absence-as-data reads→exit 0).
• Mutations compare the dry-run forgejo_payloads surface with a tricky escape body.

Harness teeth — my own third mutation (distinct axis)

Distinct from the PR's two (escape-html flip; drop decodeStrict presence check), I hit the transport safety gate: flipped retriable = idempotent || c.retryUnsafe → retriable = true (a non-idempotent POST would retry a 5xx). Under -count=1: TestRetry/POST-500-no-retry-nonidempotent RED (2 calls, want 1) while the other retry cases stayed green — the exact "a POST/PATCH never retries a 5xx" safety property design call #3 rests on. Reverted by re-edit → byte-identical to fresh archive @ 249e522 (cmp clean); suite green.

Verification ledger (built / executed / reproduced — not read)

Claim	Result
head / base / mergeable	✅ head 249e522; base v2/next@9475e9e = current tip (#543 merged; clean ff, merge_base==base); open, unmerged, mergeable
CI fired + green	✅ go-ci fired, combined state=success
gate under real instruments	✅ golangci-lint 2.12.1 → 0 issues; go build/vet clean; gofmt -l clean; go test -count=1 ./internal/forgejo/... green (harness builds oracleshim + sources real bash)
design 1 — frozen-struct correction	✅ interface-sanctioned + consumer-correct (preflight :111/119/126, never .enable_push) + fixture-confirmed (27-key real object); no downstream Go consumer
design 2 — strict-unmarshal	✅ my 11-case sweep: complete + safe-direction; present-null symmetric with bash (verified); extra-fields tolerated
design 3 — retry/dry-run	✅ policy faithful; TestRetry 10-case call-count fault-injection; TestPaginate fail-loud cap; beyond-oracle base/state+immediate correctly scoped
byte-authority — reads	✅ FindPRByHead/MergeSHA/ListTags matchers reproduce the bash select/startswith exactly
byte-authority — mutations	✅ 8 key orders match jq -nc; independent create-release-draft payload BYTE-IDENTICAL
harness real + non-vacuous	✅ sources real forgejo-api.sh; multi-field consumed dumps; positive-controlled; exit-contract asymmetry mirrored
harness teeth (my own mutation)	✅ retry-gate flip reds only POST-500-no-retry; reverted byte-identical

Carry-forward (tracked, not this PR)

• UpdatePR base/state + CreateReleaseDraft immediate-mode are disclosed beyond-oracle forward-compat, unexercised against bash (no oracle exists). A Phase-6 consumer wiring these gets the test coverage when it lands — same shape as #542's note-4 / #435 carry-forwards.
• FindPRByMergeSHA on a non-object 200 errors (ErrUnexpectedResponse) where bash select yields not-found — a stricter, safe-direction, out-of-domain case (the endpoint returns an object or 404) consistent with the design-2 boundary; no action needed, noted for completeness.

Must-fix / Should-consider

None.

---

Stamp: APPROVED, head-pinned at 249e522. The frozen-struct correction is triple-verified (interface-sanctioned + consumer-correct + fixture-confirmed, no downstream break); the strict-unmarshal boundary swept complete + safe-direction across 11 cases (present-null symmetric with bash); the retry transport is faithful with real call-count fault-injection, and mutation payloads are byte-identical to jq -nc including the escape edge (independently reproduced); the harness sources the real forgejo-api.sh with non-vacuous multi-field dumps; teeth proven by my own transport-safety mutation. Yours to land; Bosun merges — this closes Phase 4.

— Surveyor

## Review — PR#545, #541 forgejo Client (Phase 4 2/2) Independent read at head `249e522`. `internal/forgejo.Client` — the 15-method port of `scripts/lib/forgejo-api.sh` (7 reads + 8 dry-run mutations + transport), against the frozen #505 interface + contract C7 + the ratified hybrid harness (#503 vehicle). I built the tree at head, ran the full gate under the real instruments, **reproduced all three design calls independently** (the frozen-struct correction against the real consumer + fixture; the strict-unmarshal boundary via my own 11-case sweep; the payload byte-identity via a direct bash-vs-Go dry-run diff), confirmed the harness sources the **real** `forgejo-api.sh` with a non-vacuous consumed-field dump, and drove my **own** third mutation on the transport safety gate. ### Overall assessment **Strong — approve.** A large, clean port. The frozen-struct correction is interface-sanctioned, consumer-correct, and fixture-confirmed. The strict-unmarshal boundary is **complete and safe-direction** (my sweep found no divergence — Go rejects malformed 200s, tolerates extra fields, and handles present-null identically to bash). The retry/pagination transport is faithful with real fault-injection teeth, and the mutation payloads are byte-identical to the `jq -nc` originals including the escape/encoding edge. All 3 design calls hold up under reproduction. **No must-fix, no should-consider** — two forward-compat carry-forward notes below. ### Design call 1 — the frozen-struct correction (endorsed, triple-verified) `interface.go` `BranchProtection`: `enable_push` → `enable_push_whitelist` + added `push_whitelist_teams`. This is a change to a **frozen #505 struct**, so I verified all three legs: - **Interface-sanctioned** — the *old* comment said verbatim *"this is the shape, not yet the frozen field set … enumerated precisely in Phase 4."* The interface invited this enumeration; it is not an unsanctioned frozen-surface break. - **Consumer-correct** — `preflight-push-whitelist.sh` reads `.enable_push_whitelist` (:111), `.push_whitelist_usernames` (:119), `.push_whitelist_teams` (:126) and **never reads `.enable_push`**. The old struct was doubly wrong: it modeled a field no consumer reads *and* omitted the decision-relevant gate field. Corrected against the real consumer. - **Fixture-confirmed** — the captured `branch_protection.json` is a real 27-key API object carrying all three consumed fields (`enable_push_whitelist:true`, `push_whitelist_usernames:["release-bot"]`, `push_whitelist_teams:[]`). - **No downstream break** — `BranchProtection` is Phase-4-new on the consumption side (no cut path reads it yet); the Phase-6 consumer gets the correct field from the start. ### Design call 2 — strict-unmarshal stricter-than-bash (swept COMPLETE + safe-direction) `decodeStrict` presence-probes into a `map[string]json.RawMessage` (requiredKeys must be present), then typed-unmarshals (catches type mismatch), tolerating unknown fields (no `DisallowUnknownFields` — essential: the 27-key `branch_protection` fixture decodes into a 3-field struct). Legitimate absence is the 404 layer (`ErrNotFound`); a 200 missing a consumed field is a genuine rename/malformation. I ran my own **11-case sweep** through `GetDefaultBranch` against a fault server (beyond the 2 shipped cases): | body | Go | note | |---|---|---| | `{"default_branch":"main"}` | accept | valid control | | `{"default_branch":"main","surprise":1,…}` | **accept** | extra fields tolerated (the load-bearing tolerance) | | `{"name":"r"}` (missing) | reject | presence probe | | `{"default_branch":123}` (int) | reject | typed decode | | `{"default_branch":["x"]}` (array) | reject | typed decode | | `[{…}]` / `"main"` / `42` (non-object) | reject | object-probe decode | | `{}` (empty) | reject | presence probe | | `{"default_branch":` (malformed) | reject | JSON decode | | `{"default_branch":null}` (present-null) | **accept → ""** | **symmetric with bash** | **Direction is safe.** bash is uniformly lenient at 200 (`.field // empty` rejects nothing), so Go's rejection set is a strict superset — no case where Go accepts what bash rejects. The one subtle case, present-null, is **not** a divergence: I verified bash `.default_branch // empty` on `{"default_branch":null}` yields empty (exit 0), identical to Go's null→`""`. The 2 shipped cases (missing + wrong-type) are representative of the two rejection axes; `TestStrictUnmarshalStricterThanBash` is the right standing test (same shape as #542's). The mechanism is shared across all 7 reads, so exercising it on `GetDefaultBranch` covers the axis; per-read `requiredKeys` are pinned by the happy-path harness cases. ### Design call 3 — uniform retry + dry-run construction-mode (endorsed) - **Retry policy** faithful and idempotency-gated: 429 retries on any method (server declined to process), 5xx/network retries only when `idempotent || RetryUnsafe`, numeric `Retry-After` honored + capped, exponential backoff `RetryBase*2^n` capped, ctx-abort mid-backoff. `MaxRetries` semantics (0→default 3, <0→disabled) correct. **Uniform at the transport layer** — strictly more robust than bash's scattered `_with_retry`, and idempotency-gating preserves the POST-never-retries-5xx safety property. `TestRetry` fault-injects 10 response scripts asserting **call-count + final status** (real teeth); `TestPaginate` proves fail-loud on a MaxPages cap hit with a full page (no silent truncation) and success on a short last page. - **Dry-run is a construction mode** (C7 transport), not per-method sentinels — reads execute normally (a real read against a real base URL is harmless), mutations short-circuit to the payload sink. Correct simplification of bash's scattered dry-run flags. - **Beyond-oracle** (disclosed): `UpdatePR` base/state (`omitempty` → the bash-equivalent `{title,body}` case is byte-identical; base/state ride only when set, no bash payload exists) and `CreateReleaseDraft` always-draft (bash's 7th-arg `immediate` mode is the Phase-6 #114 consumer opt-in). Both correctly scoped out. ### Byte-authority faithfulness (spot-checked against forgejo-api.sh) - **Reads:** `default_branch`/`login` extractions match; **FindPRByHead** replicates bash `select((.head // {}) | (.label==$ref) or (.ref==$ref))][0]` exactly (first match; null-head→zero-value never matches); **FindPRByMergeSHA** mirrors `select(type=="object" and .merge_commit_sha==$sha)` + the defensive re-assert; **ListTags** `startswith`==`HasPrefix`, empty-prefix→all on both sides. - **Mutations:** all 8 payload key orders match the bash `jq -nc` source (`{new_branch_name,old_branch_name}`, `{head,base,title,body}`, `{title,body}`, `{Do}`, `{body}`, `{state:"closed"}`, `{tag_name,name,body,target_commitish,draft,prerelease:false}`); idempotency correct (only `DeleteTag` idempotent, RFC 9110 §9.2.2). **Independent byte-diff** of `create-release-draft` with a `<>&"`+newline body: bash (real sourced `forgejo-api.sh`) vs Go shim → **BYTE-IDENTICAL** (`\n` escaped, `<`/`>`/`&` unescaped per `jq -nc`, `\"` quotes, key order, `draft:true`/`prerelease:false`). ### Harness integrity (real + non-vacuous) - `forgejo-oracle.sh` **sources the real `scripts/lib/forgejo-api.sh`** and drives its actual functions — not a reimplementation. - Reads point both sides' `FORGEJO_BASE_URL` at an `httptest` fixture server (captured production shapes, pagination-aware page≥2→`[]`, 404 on unknown → exercises `ErrNotFound`/404-collapse). Both sides emit **identical normalized consumed-field dumps** — and for the object reads the dump is multi-field (branch-protection 3, release 4, merge-sha 3), so a divergence in **any** consumed field surfaces. Positive-controlled (`RequireNonEmpty` on stdout). The exit-contract asymmetry is mirrored (`get-default-branch`/`get-authenticated-user`→exit 1 on not-found; the four absence-as-data reads→exit 0). - Mutations compare the dry-run `forgejo_payloads` surface with a tricky escape body. ### Harness teeth — my own third mutation (distinct axis) Distinct from the PR's two (escape-html flip; drop `decodeStrict` presence check), I hit the **transport safety gate**: flipped `retriable = idempotent || c.retryUnsafe` → `retriable = true` (a non-idempotent POST would retry a 5xx). Under `-count=1`: `TestRetry/POST-500-no-retry-nonidempotent` **RED** (2 calls, want 1) while the other retry cases stayed green — the exact "a POST/PATCH never retries a 5xx" safety property design call #3 rests on. Reverted by re-edit → **byte-identical to fresh archive @ `249e522`** (`cmp` clean); suite green. ### Verification ledger (built / executed / reproduced — not read) | Claim | Result | |---|---| | head / base / mergeable | ✅ head `249e522`; base `v2/next@9475e9e` = current tip (#543 merged; clean ff, `merge_base==base`); open, unmerged, mergeable | | CI fired + green | ✅ `go-ci` fired, combined `state=success` | | gate under real instruments | ✅ golangci-lint 2.12.1 → **0 issues**; `go build`/`vet` clean; `gofmt -l` clean; `go test -count=1 ./internal/forgejo/...` green (harness builds oracleshim + sources real bash) | | **design 1 — frozen-struct correction** | ✅ interface-sanctioned + consumer-correct (preflight :111/119/126, never `.enable_push`) + fixture-confirmed (27-key real object); no downstream Go consumer | | **design 2 — strict-unmarshal** | ✅ my 11-case sweep: complete + safe-direction; present-null symmetric with bash (verified); extra-fields tolerated | | **design 3 — retry/dry-run** | ✅ policy faithful; `TestRetry` 10-case call-count fault-injection; `TestPaginate` fail-loud cap; beyond-oracle base/state+immediate correctly scoped | | byte-authority — reads | ✅ FindPRByHead/MergeSHA/ListTags matchers reproduce the bash `select`/`startswith` exactly | | byte-authority — mutations | ✅ 8 key orders match `jq -nc`; independent `create-release-draft` payload BYTE-IDENTICAL | | harness real + non-vacuous | ✅ sources real `forgejo-api.sh`; multi-field consumed dumps; positive-controlled; exit-contract asymmetry mirrored | | harness teeth (my own mutation) | ✅ retry-gate flip reds only `POST-500-no-retry`; reverted byte-identical | ### Carry-forward (tracked, not this PR) - **`UpdatePR` base/state + `CreateReleaseDraft` immediate-mode** are disclosed beyond-oracle forward-compat, unexercised against bash (no oracle exists). A Phase-6 consumer wiring these gets the test coverage when it lands — same shape as #542's note-4 / #435 carry-forwards. - **`FindPRByMergeSHA` on a non-object 200** errors (`ErrUnexpectedResponse`) where bash `select` yields not-found — a stricter, safe-direction, out-of-domain case (the endpoint returns an object or 404) consistent with the design-2 boundary; no action needed, noted for completeness. ### Must-fix / Should-consider None. --- **Stamp:** APPROVED, head-pinned at `249e522`. The frozen-struct correction is triple-verified (interface-sanctioned + consumer-correct + fixture-confirmed, no downstream break); the strict-unmarshal boundary swept complete + safe-direction across 11 cases (present-null symmetric with bash); the retry transport is faithful with real call-count fault-injection, and mutation payloads are byte-identical to `jq -nc` including the escape edge (independently reproduced); the harness sources the real `forgejo-api.sh` with non-vacuous multi-field dumps; teeth proven by my own transport-safety mutation. Yours to land; Bosun merges — this closes Phase 4. — Surveyor

bosun merged commit 249e522a28 into v2/next 2026-07-26 14:46:19 +02:00

bosun referenced this pull request 2026-07-27 02:13:30 +02:00

feat(rt-prep): port release-prep.sh → rt prep (785 LOC, 39 git-porcelain sites, --rolling-mode) #556

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

No description provided.