Correct the 0.2.0 readback and the tag recovery table - #23
Conversation
There was a problem hiding this comment.
Pull request overview
Corrects release-control history and replaces unsafe tag-recovery guidance.
Changes:
- Records the corrected environment readback.
- Splits recovery steps by remote-tag state.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Two defects in RELEASING.md, both load-bearing under failure. The 0.2.0 control readback records the unused `copilot` environment as deleted, and the environment readback as listing only three environments. Neither survives: the environment is recreated on demand by GitHub's Copilot code review, which the active `Copilot Reviews` ruleset (`copilot_code_review`, scoped `~ALL`) runs on every pull request. The evidence is unambiguous. Its `created_at` of 2026-08-17T21:56:44Z is one second after PR #11 was opened, in the burst that opened #9-#13. Deleting it on 2026-08-22 reproduced the mechanism: it came back with a new id ten seconds after the next pull request was opened. So the readback was almost certainly accurate when taken, and an unrelated pull request recreated the environment seconds later. The defect is that a transient deletion was written down as a settled control, and that "the readback lists only three environments" cannot stay true in a repository that receives pull requests. Release authority was never affected: across both incarnations the environment had no protection rules, no branch policy, no secrets, no variables, no deployments and no workflow reference. The pre-tag gate therefore asserts protection values rather than an environment count -- an equality check on the name list would fail the release spuriously after any pull request. Recorded as a dated correction with the original paragraph intact. Amending in place would erase the reason this readback could not be reproduced. The recovery table separately told an operator to delete an unpublished tag by having an admin temporarily lift `release-tags-immutable`. That contradicts every other rule in this runbook, and conflicting emergency instructions are the ones followed under pressure. The row also conflated "unpublished" with "deletable". The real discriminator is whether the tag reached the remote, then whether a corrective commit is needed: `rake tag` creates the local tag before pushing and pushes `main` and the tag separately, so a failed run routinely leaves a local tag with no remote counterpart. That case needs `git tag -d` on a purely local ref and no ruleset change at all. Anything still appearing to require lifting the ruleset is now a stop.
46ad4af to
e0113bc
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46ad4affca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Four review findings on the recovery guidance, all valid. `git ls-remote` cannot establish canonical remote state. A URL spelled out in full on the command line is still rewritten by `url.<base>.insteadOf`, so the query can inspect a different repository and report the canonical tag absent. The previous text claimed the opposite -- that spelling the URL out defeats the rewrite -- which is exactly backwards, and a rewriting rule has already been configured on this maintainer's machine once. Recovery now asks GitHub over `git/matching-refs`, which touches no Git configuration and returns 200 with `[]` for a genuinely absent tag, so a failed query is distinguishable from absence rather than indistinguishable from it. A present remote tag is not necessarily your tag. A concurrent attempt can win the name, and the workflow only checks that its tag is well formed and points into `main` -- not that it matches your checkout or your rehearsal. Routing straight to `gh run rerun` could therefore have an approver authorize someone else's run. Recovery now splits present-and-identical from present-and-different, and the latter stops. The transient-retry row promised a re-run would work whenever no corrective commit was needed. It does not if remote `main` advanced meanwhile: the Rake task fetches `main` and aborts because the tagged HEAD no longer equals it, and fast-forwarding while keeping the tag then fails the peel check. That state has its own row, and its remedy is the local-only deletion plus a fresh rehearsal on the new head. Finally, the tag step said "on an up-to-date `main` checkout", which invites a `git pull` that silently moves the checkout off the rehearsed commit onto one whose bytes nobody has built -- and the Rake guard, being HEAD == fetched `main`, is satisfied by construction after a pull. The step now binds to the rehearsed SHA explicitly and says not to pull. The remaining race is safe in one direction: if `main` advances after the assertion, `rake tag` re-fetches and refuses. It cannot tag the wrong commit.
Review round addressed — all four threads fixed and resolved (48ae43c)The doc fixes are in the commit; the operator-script fixes are outside the PR, under
|
| Finding | Fix |
|---|---|
ls-remote on a literal URL is still rewritten by insteadOf — and the doc claimed the opposite |
Canonical state now comes from gh api .../git/matching-refs/tags/vX.Y.Z, which consults no Git config, and returns 200 + [] for a genuinely absent tag so absence is distinguishable from a failed query |
| Remote tag present ≠ remote tag is yours | Split into present-and-identical (gh run rerun) vs present-and-different (stop; do not approve that run's environments), with the object comparison spelled out |
Retry row false once remote main advanced |
Dedicated row: prove absent → delete local ref only → fast-forward → re-rehearse on the new head → tag that |
| — (found while fixing the above) | Step 3 said "up-to-date main checkout", inviting a git pull that moves off the rehearsed commit onto unbuilt bytes — and the Rake guard HEAD == fetched main is satisfied by construction after a pull. Now binds to the rehearsed SHA and says not to pull |
Operator scripts
Both now set -Eeuo pipefail, write to fresh attempt-scoped evidence directories, and contain no variable-derived rm -rf — a path that already exists is refused rather than cleared.
The central fix is that a failed producer can no longer read as a passing value. Demonstrated:
old style: $(false | wc -l) -> "0" == want "0" => FALSE GO
new chk(): NO-GO tag absent: producer failed (exit 1)
Three real bugs surfaced because the scripts now fail closed, none of which the previous happy-path testing could have caught:
jq -eexits 1 on a literalfalse, so everycan_admins_bypass: falseassertion was failing as a "producer error" and masking the real value. Booleans now compare viatostring.- The
miseshim prints a banner to stderr on everyghcall; merging it with2>&1corrupted every compared value. stderr is now captured separately, for diagnostics only. grepexits 1 on no match, whichpipefailsurfaced as a producer failure on the "named by no workflow" check.
I also had release-recovery's expected branch policy wrong (branch:main); RELEASING.md:363-368 documents both main and v*, which is what the API returns. The doc was right, my assertion was wrong.
Snapshot assertions completed as requested: required-status-check integration id 15368, dismiss_stale_reviews_on_push, release-recovery branch policy, the tag-creation creation rule, reviewer numeric id 199 alongside the login, plus local tag absence, clean tree, and on-main.
Negative control now checks the reason
It previously accepted any non-zero exit. It now requires the specific SourceRepositoryRef mismatch, and I verified that discriminates — a wrong --signer-workflow fails with verifying with issuer "sigstore.dev" and no SourceRepositoryRef, so it is correctly rejected as an invalid control rather than counted as a passing one.
A live instance of the same bug class, in a different tool
While testing, gh hit a secondary rate limit and gh release view reported an existing release as release not found — a 403 rendered as absence. verify-release.sh now looks the release up over the REST API first and distinguishes "rate limited → UNKNOWN" from "absent", rather than trusting that message.
Current state
pretag-snapshot.sh passes every control against live state. The only NO-GO rows are the two rehearsal checks, which correctly report producer failed (no rehearsal file yet) rather than silently passing — the rehearsal runs after this merges, on the merged commit.
No merge, no RubyGems MFA readback, no tag. Returning for review on the final head.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48ae43c6ad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
`git/matching-refs/tags/vX.Y.Z` matches by PREFIX, not exactly. Confirmed against this repository: querying `tags/v0.1` returns v0.1.0, v0.1.1, v0.1.2 and v0.1.3, none of which is a tag named `v0.1`. So when `vX.Y.Z` is absent but some `vX.Y.Z...` tag exists -- a `-rc1`, or `v1.2.30` against a query for `v1.2.3` -- the raw array is populated even though the tag being looked for is not there. The unfiltered length test then reported an absent tag as present and routed the operator into the present-tag rows, where the exact selector yields no SHA and the guidance lands on the present-but-different stop case. The absent-tag recovery, which is the correct path, was unreachable in exactly the situation a release-candidate tag makes likely. The query now filters to the exact ref before testing emptiness, and the paragraph explains why the filter is load-bearing rather than tidiness.
Round 3 addressed — thread resolved, four script items done (4d3338b)
|
| Query | Unfiltered | Exact-filtered |
|---|---|---|
tags/v0.1 (no such tag) |
4 → "present" ✗ | 0 → absent ✓ |
tags/v0.1.3 (real tag) |
1 | 1 ✓ |
Both call sites now filter to the exact ref before testing emptiness. Worth noting the two directions differ, which is why both needed it: in the snapshot an unfiltered result is a false NO-GO (blocks a release); in the recovery table it is a false stop (blocks a recovery) — the correct absent-tag path becomes unreachable exactly when a prefix-sharing tag exists, which an -rc tag makes likely.
Script items
eval over API-derived names removed. Environment names came from the API and were interpolated into an eval'd string. Inside single quotes a $(...) payload cannot fire — but a single-quote breakout can, which I confirmed rather than assumed:
E="x'; touch /tmp/PWNED-DEMO; echo 'y"
eval "printf '%s' 'repos/x/environments/$E'" -> INJECTED
probe(){ printf '%s' "repos/x/environments/$1"; }; probe "$E" -> safe
All environment-name checks now go through a new chk_cmd that passes argv and never re-parses the name; grep -F and -- stop a hostile name being read as a pattern or an option.
Atomic fresh directories. mkdir -p on a second-resolution stamp silently reuses a directory when two runs land in the same second — precisely the stale-evidence case the scheme exists to prevent. Both scripts now use mktemp -d: 3 same-second creations → 3 distinct directories. This also removed the last [ -e ] && exit / mkdir -p race.
Negative control tightened. Was grep -qi 'SourceRepositoryRef' anywhere in the output; now matches the full assertion including both refs — expected SourceRepositoryRef to be <bogus>, got refs/tags/v<version>.
Compact-index check added, bounded. index.rubygems.org's checksum: field is the sha256 of the .gem archive — verified equal to v0.1.3's digest, so it is a genuine fifth authority rather than a restatement. Bounded poll (20 × 15s, overridable via INDEX_TRIES/INDEX_SLEEP), hard failure if the version never appears, and only a checksum match counts as confirmation — presence of the version does not.
Re-tested against live v0.1.3
=== all five authorities agree: c0911960… ===, with the negative control still firing. pretag-snapshot.sh passes every control; the only NO-GO rows remain the two rehearsal checks, correctly reporting producer failed because no rehearsal exists yet.
One more grep-exit-1-on-no-match bug surfaced inside the new argv helper and was fixed — the third instance of that pattern in this work, which is itself the argument for the fail-closed design.
No merge, no MFA readback, no tag.
Pre-release correction to
RELEASING.md. Two defects, both load-bearing under failure.1. A readback that cannot stay true
The "0.2.0 control readback (2026-08-17)" section records the unused
copilotenvironment as "verified to have no protection rules and deleted; readback lists onlygithub-release,release-recovery, andrelease-rubygems."The environment was present again on 2026-08-22. The tempting inference — that it was never deleted — is wrong. It is recreated on demand by GitHub's Copilot code review, which the active
Copilot Reviewsruleset (copilot_code_review, scoped~ALL) runs on every pull request:created_at2026-08-17T21:56:44Z2026-08-17T21:56:43Z— one second earlier, in the burst that opened #9–#132026-08-22T19:13:34Z203984024462026-08-22T19:13:44Z— ten seconds laterSo the 2026-08-17 readback was almost certainly accurate the moment it was taken, and an unrelated pull request recreated the environment seconds afterwards.
The defect is therefore not a false record. It is that a transient deletion was written down as a settled control, and that "the readback lists only three environments" is an assertion which cannot hold in a repository that receives pull requests.
No release authority was ever affected. Across both incarnations: no protection rules, no deployment branch policy, no secrets, no variables, no deployments, and no reference from any workflow —
release.ymlandrelease-recovery.ymlname onlyrelease-rubygems,github-releaseandrelease-recovery. It can neither gate nor bypass any release job.The consequence for the gate matters more than the record: the pre-tag snapshot asserts protection values, not an environment count — the three release environments carry their required-reviewer and
can_admins_bypasssettings, and any environment outside that set is inert. An equality check on the name list would fail every release that follows a pull request.Recorded as a dated correction with the original paragraph intact, so the reason this readback could not be reproduced stays visible.
2. Contradictory break-glass instructions
The recovery table's first row told an operator that, for a pre-publish failure, an admin should "temporarily lift the
release-tags-immutableruleset, delete, and re-enable it." Every other rule in this runbook says never to touch a pushed tag, and conflicting emergency instructions are precisely the ones followed under pressure.The row also conflated "unpublished" with "deletable". The actual discriminator is whether the tag reached the remote, and then whether a corrective commit is needed.
rake tagcreates the local tag before pushing, and pushesmainand the tag as two separate operations — so a failed run routinely leaves a local tag with no remote counterpart. That case is resolved bygit tag -don a purely local ref: it touches no remote ref and no ruleset, and is not the immutability case at all.The table now splits on remote-tag presence, spells out the
ls-remotethat establishes it, and ends with a catch-all: anything still appearing to require lifting the ruleset is a stop, pending a separately reviewed break-glass runbook.The
ls-remoteis written against the literal canonical URL rather thanorigin, matching whatrake tagitself validates — local remote andinsteadOfconfig is exactly what you cannot trust while establishing remote state. The two existing published-tag rows are unchanged.Cost
This commit moves
main, voiding the earlier rehearsal and its digest33448ee9…a09acb. A fresh rehearsal runs against the merged commit before tagging.check_iana_driftis clean against live registries on all three sources, so there is no clock on this.