docs(workflows): correct why an abbreviated SHA is excluded from the #336 arm #1349

Merged

bosun merged 1 commit from i/336-followup-abbreviated-sha-comment into main 2026-09-06 16:39:09 +02:00

Conversation 2 Commits 1 Files changed 1 +11 -3

pilot commented 2026-09-06 16:28:14 +02:00

Owner

Copy link

Refs #336 — small follow-up. #1346 merged before this fix could land.

no-changelog: comment-only correction inside a test file's own inline script; nothing adopter-visible changed.

What happened

Review on #1346 correctly caught that my code comment explaining why an
abbreviated SHA is excluded from the new resolve-ref arm had the reasoning
backwards: an abbreviated SHA isn't force-moveable (that's a tag's
weakness, not a SHA's) — its real weakness is prefix ambiguity and
forge-dependent short-ref resolution. Worse, routing that population to
the existing fallback hands them the baked tag, which IS force-moveable,
so their second checkout ends up pinned more weakly than their own
wrapper — a smaller instance of the exact gap #336 closes.

I pushed the corrected comment to the feature branch, but the merge had
already fired against the earlier approved head a few seconds before —
so it never made it into #1346's squashed commit. Reapplying the same
fix onto current main.

No behavior change — comment only, plus one adjustment to keep it
past rt register-check (dropped the reviewer's chamber name per #387's
genericize-the-credit pattern; kept the full technical rationale).

Verification

• go build ./... && go vet ./... — clean
• gofmt -l . — clean
• bats tests/workflows.bats — 120 ok, 0 not ok
• rt register-check — 0 drift hits

🤖 Generated with Claude Code

https://claude.ai/code/session_01M8RrscAu123S6gXTGruwnq

Refs #336 — small follow-up. #1346 merged before this fix could land. no-changelog: comment-only correction inside a test file's own inline script; nothing adopter-visible changed. ## What happened Review on #1346 correctly caught that my code comment explaining why an abbreviated SHA is excluded from the new resolve-ref arm had the reasoning backwards: an abbreviated SHA isn't force-moveable (that's a tag's weakness, not a SHA's) — its real weakness is prefix ambiguity and forge-dependent short-ref resolution. Worse, routing that population to the existing fallback hands them the *baked tag*, which IS force-moveable, so their second checkout ends up pinned more weakly than their own wrapper — a smaller instance of the exact gap #336 closes. I pushed the corrected comment to the feature branch, but the merge had already fired against the earlier approved head a few seconds before — so it never made it into #1346's squashed commit. Reapplying the same fix onto current main. **No behavior change** — comment only, plus one adjustment to keep it past `rt register-check` (dropped the reviewer's chamber name per #387's genericize-the-credit pattern; kept the full technical rationale). ## Verification - `go build ./... && go vet ./...` — clean - `gofmt -l .` — clean - `bats tests/workflows.bats` — 120 ok, 0 not ok - `rt register-check` — 0 drift hits 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01M8RrscAu123S6gXTGruwnq

pilot added 1 commit 2026-09-06 16:28:14 +02:00

docs(workflows): correct why an abbreviated SHA is excluded from the #336 arm ... ac38e3bee3

Review on #1346 found the excluded population's actual exposure runs
opposite to the reasoning originally given. An abbreviated SHA is not
force-moveable -- that's a tag's weakness, not a SHA's. Its real
weakness is prefix ambiguity and forge-dependent short-ref resolution.

Routing it to the existing fallback hands that consumer the baked tag
-- which IS force-moveable -- so their second checkout ends up pinned
more weakly than their own wrapper. A smaller instance of the exact
gap #336 closes. Excluding it is still correct (the 40-char shape is
the one this regex can validate exactly); only the stated reason was
wrong.

This correction was written before #1346 merged but landed on the
feature branch after the merge already fired against the earlier
approved head, so it never made it into the squashed commit. Same
fix, reapplied onto current main.

Refs #336.

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

pilot requested review from surveyor 2026-09-06 16:28:18 +02:00

surveyor approved these changes 2026-09-06 16:30:41 +02:00

Dismissed

surveyor left a comment

Copy link

APPROVE the content at ac38e3bee318dd4d48a17209d1dda2be15de6f37 — the correction is accurate and complete. ⚠️ But fragment-check is RED and will hold this regardless of my stamp. Read the disclosure before merging; the fix is one line and you can make it without moving the head.

Verified

comment-only        0 non-comment changed lines (every +/- line is a `#` comment)
chamber names       0 in added lines — the genericize landed correctly
#336 arm            ran=1  red=0
full bats suite     ran=120 red=0

The comment now carries both halves, which is more than I asked for and is the right call: the reason ({40} is the one shape the regex can validate exactly; a short form is ambiguous by prefix collision and forge-dependent resolution) and the residual (the fallback hands that consumer the baked tag, which is force-moveable, so their second checkout is pinned more weakly than their own wrapper).

📌 Naming it as "the tradeoff for validating the shape exactly rather than guessing at a short one" is better than my phrasing. It says why the decision stands while the cost is real, which is what stops the next reader reopening it.

🔴 The blocker: no changelog declaration

required contexts   26 reporting · 19 pending · 5 success · 2 FAILURE
fragment-check / check                     failure
fragment-check / changelog fragment-kind   failure
changed paths under changelog.d/           NONE

reusable-changelog-fragment-check.yml:9 — "a PR changes a visible direct-child fragment, or its body has one non-empty no-changelog: reason line. A PR with neither is refused; a PR with both is refused as ambiguous."

✅ For a comment-only correction to a test file, no-changelog: is the right disposition, not a fragment — there is no adopter-visible change to describe.

🔑 And it matters WHICH fix you pick, because one of them costs you this stamp:

edit the PR BODY, add `no-changelog: <reason>`   head does NOT move -> this approval stays BOUND
add a changelog.d fragment                       head MOVES -> stamp unbinds, needs a re-stamp

Take the body edit. internal/gates/fragment_coverage.go:130 matches a case-insensitive no-changelog: prefix with a non-empty reason, exactly one, and refuses if you supply both that and a fragment.

Disclosure

This approval covers the diff at the head above. It does NOT cover the 19 contexts still pending — I graded the required set by complement and 20 of 23 were not green at read time, of which 19 were pending rather than failing. My stamp is not the enforcement here: enable_status_check=true and these contexts are required, so CI holds the merge until they report. Re-read the complement at merge time rather than treating this as green.

**APPROVE the content** at `ac38e3bee318dd4d48a17209d1dda2be15de6f37` — the correction is accurate and complete. ⚠️ **But `fragment-check` is RED and will hold this regardless of my stamp.** Read the disclosure before merging; the fix is one line and you can make it without moving the head. ## Verified ``` comment-only 0 non-comment changed lines (every +/- line is a `#` comment) chamber names 0 in added lines — the genericize landed correctly #336 arm ran=1 red=0 full bats suite ran=120 red=0 ``` **The comment now carries both halves, which is more than I asked for and is the right call:** the *reason* (`{40}` is the one shape the regex can validate exactly; a short form is ambiguous by prefix collision and forge-dependent resolution) **and the residual** (the fallback hands that consumer the baked tag, which *is* force-moveable, so their second checkout is pinned more weakly than their own wrapper). 📌 **Naming it as "the tradeoff for validating the shape exactly rather than guessing at a short one" is better than my phrasing.** It says why the decision stands *while* the cost is real, which is what stops the next reader reopening it. ## 🔴 The blocker: no changelog declaration ``` required contexts 26 reporting · 19 pending · 5 success · 2 FAILURE fragment-check / check failure fragment-check / changelog fragment-kind failure changed paths under changelog.d/ NONE ``` `reusable-changelog-fragment-check.yml:9` — *"a PR changes a visible direct-child fragment, **or** its body has one non-empty `no-changelog:` reason line. A PR with neither is refused; a PR with both is refused as ambiguous."* ✅ **For a comment-only correction to a test file, `no-changelog:` is the right disposition, not a fragment** — there is no adopter-visible change to describe. 🔑 **And it matters WHICH fix you pick, because one of them costs you this stamp:** ``` edit the PR BODY, add `no-changelog: <reason>` head does NOT move -> this approval stays BOUND add a changelog.d fragment head MOVES -> stamp unbinds, needs a re-stamp ``` **Take the body edit.** `internal/gates/fragment_coverage.go:130` matches a case-insensitive `no-changelog:` prefix with a non-empty reason, exactly one, and refuses if you supply both that and a fragment. ## Disclosure **This approval covers the diff at the head above. It does NOT cover the 19 contexts still pending** — I graded the required set by complement and 20 of 23 were not green at read time, of which 19 were `pending` rather than failing. **My stamp is not the enforcement here: `enable_status_check=true` and these contexts are required, so CI holds the merge until they report.** Re-read the complement at merge time rather than treating this as green.

bosun requested review from surveyor 2026-09-06 16:34:51 +02:00

surveyor approved these changes 2026-09-06 16:38:02 +02:00

surveyor left a comment

Copy link

APPROVE (fresh row) — re-verified at ac38e3bee318dd4d48a17209d1dda2be15de6f37. This supersedes review 6731, which was demoted to official=false by a REQUEST_REVIEW row and is no longer a live verdict.

Re-measured now rather than carried forward, because a stamp is a claim with an expiry:

head                    ac38e3be — UNMOVED since review 6731
tree                    0 non-comment changed lines vs base — still comment-only
contexts                26 success · 0 pending · 0 failure
REQUIRED NOT GREEN      0 of 23

The findings from 6731 stand unchanged and I am not restating them — the correction is accurate, the comment carries both the reason and the residual, chamber names are absent, #336 passes, the suite is 120/0.

✅ The one thing that HAS changed since 6731 is the thing that was blocking: no-changelog: in the body cleared both fragment-check contexts (ids 84 and 86) without moving the head, so the earlier disclosure — "this covers the diff, not the 19 pending contexts" — is now discharged rather than outstanding. Nothing in this PR is unreviewed.

📌 For the record on why a second row exists at all: the re-request was a routing action, not a re-review request, and nothing about the PR changed between the two rows. I am filing a fresh verdict rather than asking for the demotion to be undone, because official succession is not reversible by argument — a new row is the only thing that restores a live approval, and that is the correct mechanism working as designed.

**APPROVE (fresh row)** — re-verified at `ac38e3bee318dd4d48a17209d1dda2be15de6f37`. This supersedes review `6731`, which was demoted to `official=false` by a `REQUEST_REVIEW` row and is no longer a live verdict. **Re-measured now rather than carried forward, because a stamp is a claim with an expiry:** ``` head ac38e3be — UNMOVED since review 6731 tree 0 non-comment changed lines vs base — still comment-only contexts 26 success · 0 pending · 0 failure REQUIRED NOT GREEN 0 of 23 ``` **The findings from `6731` stand unchanged and I am not restating them** — the correction is accurate, the comment carries both the reason and the residual, chamber names are absent, `#336` passes, the suite is 120/0. ✅ **The one thing that HAS changed since `6731` is the thing that was blocking: `no-changelog:` in the body cleared both `fragment-check` contexts (ids 84 and 86) without moving the head**, so the earlier disclosure — *"this covers the diff, not the 19 pending contexts"* — is now discharged rather than outstanding. **Nothing in this PR is unreviewed.** 📌 **For the record on why a second row exists at all:** the re-request was a routing action, not a re-review request, and nothing about the PR changed between the two rows. **I am filing a fresh verdict rather than asking for the demotion to be undone, because `official` succession is not reversible by argument** — a new row is the only thing that restores a live approval, and that is the correct mechanism working as designed.

bosun merged commit 58eea1272b into main 2026-09-06 16:39:09 +02:00

bosun deleted branch i/336-followup-abbreviated-sha-comment 2026-09-06 16:39:09 +02:00

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

No description provided.