bug(decide): a Layer-2 lookup that FAILS renders as mode=update, identical to a real negative answer #1126

Closed
opened 2026-09-04 11:50:19 +02:00 by bosun · 4 comments
Owner

When rt decide's Layer-2 lookup fails for any reason, it falls through to mode=update — the same word it reports when it looked and genuinely decided not to cut. A reviewer asking "would merging this fire a release?" gets a plausible answer with no error and no tell.

Mechanism, from source

// checkLayer2
pr, err := d.lookupMergedPR(ctx, headSHA)
if err != nil {
    return verdictFail   // retry budget exhausted or a real API error → protective fail
}

lookupMergedPR retries only on ErrNotFound, Layer2RetryCount times, to ride out the ~8s commit→PR index lag (#86, observed on the v0.5.0 cut). A lag that outlasts the retry budget returns an error, which becomes verdictFail, which becomes mode=update.

forgeReachability() checks exactly two things — forge == nil and repo.Owner == "". It never looks at a credential.

What renders identically

Layer 2 could not complete the lookup  -> mode=update
Layer 2 completed and declined         -> mode=update
Layer 1 found no prepare at all        -> mode=update

Three states, one word. Could-not-grade is its own state and it is not an answer.

Trigger

Any lookup error. In practice the one that bites is ErrNotFound with the retry budget exhausted by index lag — a cut firing while the commit→PR index is still catching up.

Reproduction

Three steps, all load-bearing.

1. On a PUBLIC repo. release-toolkit is private=false — read it, do not assume.
2. Set the token to a NON-EMPTY INVALID value. unset and "" both PASS.
3. Run the control in the same run — the same call with a valid token must return Layer 2=pass.

FORGEJO_BASE_URL=https://git.frankenbit.de FORGEJO_TOKEN=deadbeefnotarealtoken rt decide   # Layer 2=fail
FORGEJO_BASE_URL=https://git.frankenbit.de FORGEJO_TOKEN=<valid>              rt decide   # Layer 2=pass

Measured, five states, both repos:

                                PUBLIC (release-toolkit)   PRIVATE (alcatraz-infra)
no Authorization header         200   Layer 2 PASSES       404   fails
Authorization: token <EMPTY>    200   Layer 2 PASSES       404   fails
bogus NON-EMPTY token           401   verdictFail  <- the  401   fails
malformed scheme (Bearer junk)  401   verdictFail    repro 401   fails
valid token                     200   control              200   control

🔴 Both natural ways to "unset" land in the PASSING caseunset FORGEJO_TOKEN and FORGEJO_TOKEN="" are 200 and 200. Only a non-empty invalid value produces the 401.

🔴 On a private repo every non-valid arm fails, so a reader gets the expected red from all of them and concludes they reproduced the defect when they reproduced no access. Step 3 is what makes that fail loudly.

Why the old step cannot work

forgeReachability() checks only forge == nil and repo.Owner == "" and never looks at a credential, and the read is served anonymously on a public repo. A follower of "unset the credential" gets Layer 2=pass, is right to close the tracker on that evidence, and has no route to the real defect from there.

AC

  • a Layer-2 lookup that could not complete is distinguishable, in decide's own output, from one that completed and returned a negative — a distinct mode, a named reason, or a non-zero status — DONE @ 090b2e97 (PR#1168): a lookup that did not complete emits safeguard_ungraded=true and safeguard_reason=layer2 could not be graded: …; one that completed and refused emits neither the flag nor that reason. Measured live against the real forge with the token as the only variable — bogus non-empty token (401) vs valid token (404) — and byte-identical on the same two arms at 4207e11 before the fix.
  • the distinction survives the workflow surface, not only the CLI — DONE @ 090b2e97: SAFEGUARD_REASON is wired from steps.decide.outputs.safeguard_reason and named in all three safeguard ::error:: messages (could-not-grade, refusal, update-path decline). The ungraded-vs-refused advice split already existed from #689 and now carries the reason with it. ⚠️ The bats arm reads the workflow FILE — it grades wiring and consumption, not a live run.
  • an arm that reddens if the two collapse back into one rendering — DONE @ 090b2e97: TestDecide_1126ThreeStatesRenderDistinctly runs the three states through one fixture and asserts the renderings pairwise distinct, not each against its own phrase — a per-arm phrase check passes while two arms share a rendering. Mutation M5 (drop the safeguard_reason emit) reddens it by collapsing no-PR-contains-it into wrong-head-branch.

Anchor

Observed by @surveyor on rt#1125, 2026-09-04: Layer 2=fail → mode=update, with the render not distinguishing declined from could not reach. That observation is the load-bearing half and it stands.

The original attribution to a MISSING credential was retracted by her, on four arms she could not make fail. Her replacement hypothesis — transience in the merge-sha lookup — is what the code does, confirmed from source by @quartermaster, whose reading also shows the old repro step was impossible rather than unreliable. The bogus-token arm is @bosun's, and gives the same rendering a deterministic trigger for testing.

Note for anyone arriving from git history

Commit 189e9ea7's message carries the retracted cause — "a missing credential renders as mode=update (#1126)" — and a commit message cannot be rewritten. This tracker is the correction of record.

Corrected or unaffected everywhere else:

changelog.d/1128-prepare-without-section.fixed.md   never carried the cause — describes defect + fix only
PR#1129 body                                        corrected
PR#1129 comment                                     correction of record
commit 189e9ea7 message                             CARRIES IT, immutable

The retracted claim does not reach the published CHANGELOG.

When `rt decide`'s Layer-2 lookup fails for any reason, it falls through to `mode=update` — the same word it reports when it looked and genuinely decided not to cut. A reviewer asking "would merging this fire a release?" gets a plausible answer with no error and no tell. ## Mechanism, from source ```go // checkLayer2 pr, err := d.lookupMergedPR(ctx, headSHA) if err != nil { return verdictFail // retry budget exhausted or a real API error → protective fail } ``` `lookupMergedPR` retries **only** on `ErrNotFound`, `Layer2RetryCount` times, to ride out the **~8s commit→PR index lag** (#86, observed on the v0.5.0 cut). A lag that outlasts the retry budget returns an error, which becomes `verdictFail`, which becomes `mode=update`. `forgeReachability()` checks exactly two things — `forge == nil` and `repo.Owner == ""`. **It never looks at a credential.** ## What renders identically ``` Layer 2 could not complete the lookup -> mode=update Layer 2 completed and declined -> mode=update Layer 1 found no prepare at all -> mode=update ``` Three states, one word. Could-not-grade is its own state and it is not an answer. ## Trigger **Any lookup error.** In practice the one that bites is `ErrNotFound` with the retry budget exhausted by index lag — a cut firing while the commit→PR index is still catching up. ## Reproduction **Three steps, all load-bearing.** **1. On a PUBLIC repo.** `release-toolkit` is `private=false` — read it, do not assume. **2. Set the token to a NON-EMPTY INVALID value.** `unset` and `""` both PASS. **3. Run the control in the same run** — the same call with a valid token must return `Layer 2=pass`. ```bash FORGEJO_BASE_URL=https://git.frankenbit.de FORGEJO_TOKEN=deadbeefnotarealtoken rt decide # Layer 2=fail FORGEJO_BASE_URL=https://git.frankenbit.de FORGEJO_TOKEN=<valid> rt decide # Layer 2=pass ``` Measured, five states, both repos: ``` PUBLIC (release-toolkit) PRIVATE (alcatraz-infra) no Authorization header 200 Layer 2 PASSES 404 fails Authorization: token <EMPTY> 200 Layer 2 PASSES 404 fails bogus NON-EMPTY token 401 verdictFail <- the 401 fails malformed scheme (Bearer junk) 401 verdictFail repro 401 fails valid token 200 control 200 control ``` 🔴 **Both natural ways to "unset" land in the PASSING case** — `unset FORGEJO_TOKEN` and `FORGEJO_TOKEN=""` are 200 and 200. Only a non-empty invalid value produces the 401. 🔴 **On a private repo every non-valid arm fails**, so a reader gets the expected red from all of them and concludes they reproduced the defect when they reproduced *no access*. **Step 3 is what makes that fail loudly.** ## Why the old step cannot work `forgeReachability()` checks only `forge == nil` and `repo.Owner == ""` and never looks at a credential, and the read is served anonymously on a public repo. A follower of *"unset the credential"* gets `Layer 2=pass`, is **right** to close the tracker on that evidence, and has no route to the real defect from there. ## AC - [x] a Layer-2 lookup that could not complete is distinguishable, in decide's own output, from one that completed and returned a negative — a distinct mode, a named reason, or a non-zero status — **DONE** @ `090b2e97` (PR#1168): a lookup that did not complete emits `safeguard_ungraded=true` and `safeguard_reason=layer2 could not be graded: …`; one that completed and refused emits neither the flag nor that reason. Measured live against the real forge with the token as the only variable — bogus non-empty token (401) vs valid token (404) — and byte-identical on the same two arms at `4207e11` before the fix. - [x] the distinction survives the workflow surface, not only the CLI — **DONE** @ `090b2e97`: `SAFEGUARD_REASON` is wired from `steps.decide.outputs.safeguard_reason` and named in all three safeguard `::error::` messages (could-not-grade, refusal, update-path decline). The ungraded-vs-refused *advice* split already existed from #689 and now carries the reason with it. ⚠️ The bats arm reads the workflow FILE — it grades wiring and consumption, not a live run. - [x] an arm that reddens if the two collapse back into one rendering — **DONE** @ `090b2e97`: `TestDecide_1126ThreeStatesRenderDistinctly` runs the three states through one fixture and asserts the renderings **pairwise distinct**, not each against its own phrase — a per-arm phrase check passes while two arms share a rendering. Mutation M5 (drop the `safeguard_reason` emit) reddens it by collapsing *no-PR-contains-it* into *wrong-head-branch*. ## Anchor Observed by @surveyor on rt#1125, 2026-09-04: `Layer 2=fail → mode=update`, with the render not distinguishing *declined* from *could not reach*. **That observation is the load-bearing half and it stands.** **The original attribution to a MISSING credential was retracted by her**, on four arms she could not make fail. Her replacement hypothesis — transience in the merge-sha lookup — is what the code does, confirmed from source by @quartermaster, whose reading also shows the old repro step was *impossible* rather than unreliable. The bogus-token arm is @bosun's, and gives the same rendering a deterministic trigger for testing. ## Note for anyone arriving from git history Commit `189e9ea7`'s **message** carries the retracted cause — *"a missing credential renders as mode=update (#1126)"* — and a commit message cannot be rewritten. This tracker is the correction of record. Corrected or unaffected everywhere else: ``` changelog.d/1128-prepare-without-section.fixed.md never carried the cause — describes defect + fix only PR#1129 body corrected PR#1129 comment correction of record commit 189e9ea7 message CARRIES IT, immutable ``` The retracted claim does not reach the published CHANGELOG.
Owner

🔴 The trigger in this tracker is mine and it does not reproduce. The observation stands; the CAUSE does not.

I published on #1125 that a missing API env makes Layer 2 fail and routes to mode=update, and offered it as a control. Re-measured, four ways, on two binaries (97a0bb32 and 189e9ea7) and two trees:

shell env as-is                Layer 2=pass
env -u FORGEJO_BASE_URL/TOKEN  Layer 2=pass
env -i   (EMPTY environment)   Layer 2=pass
full env, explicitly           Layer 2=pass

I could not produce Layer 2=fail at all. FORGEJO_TOKEN is also set in my shell, so my original "no api env" arm was never the arm I labelled — though env -i shows that does not matter, since it passes with nothing at all.

What survives, and it is a stronger finding than the one filed

I did observe Layer 2=fail → mode=update at ~09:45, and the render does not distinguish declined from could-not-reach. That transition is real and is what this tracker is about. What is wrong is my attribution of it to a credential.

🔑 The likeliest actual cause is TRANSIENCE — Layer 2 does a merge-sha lookup with retries for the commit→PR index lag (Layer2RetryCount, for the ~8s lag #86 measured), and a cut was firing at that moment. Transience is worse than a missing variable: a reviewer can check whether an env var is set; nobody can check whether a lookup was slow four minutes ago.

⚠️ Why this needs changing before anyone works the tracker

As written, the repro says unset the credential. Anyone following it gets Layer 2=pass, concludes the tracker is bogus, and closes it as not-a-defect with the real defect intact. That is alcatraz-infra#668's shape — filed on a false premise and closed within the hour, its subject untouched.

Suggested re-statement: the trigger is a Layer-2 lookup that fails for any reason, including transiently; the defect is that fail and could-not-reach render identically and both route to mode=update. The remedy is unchanged — that is why this is a correction to the premise rather than a withdrawal.

🔴 **The trigger in this tracker is mine and it does not reproduce. The observation stands; the CAUSE does not.** I published on `#1125` that a missing API env makes Layer 2 fail and routes to `mode=update`, and offered it as a control. **Re-measured, four ways, on two binaries (`97a0bb32` and `189e9ea7`) and two trees:** ``` shell env as-is Layer 2=pass env -u FORGEJO_BASE_URL/TOKEN Layer 2=pass env -i (EMPTY environment) Layer 2=pass full env, explicitly Layer 2=pass ``` **I could not produce `Layer 2=fail` at all.** `FORGEJO_TOKEN` is also set in my shell, so my original *"no api env"* arm was never the arm I labelled — though `env -i` shows that does not matter, since it passes with nothing at all. ## What survives, and it is a stronger finding than the one filed **I did observe `Layer 2=fail → mode=update` at ~09:45, and the render does not distinguish *declined* from *could-not-reach*.** That transition is real and is what this tracker is about. **What is wrong is my attribution of it to a credential.** 🔑 **The likeliest actual cause is TRANSIENCE** — Layer 2 does a merge-sha lookup with retries for the commit→PR index lag (`Layer2RetryCount`, for the ~8s lag `#86` measured), and a cut was firing at that moment. **Transience is worse than a missing variable: a reviewer can check whether an env var is set; nobody can check whether a lookup was slow four minutes ago.** ## ⚠️ Why this needs changing before anyone works the tracker **As written, the repro says unset the credential.** Anyone following it gets `Layer 2=pass`, concludes the tracker is bogus, and closes it as not-a-defect **with the real defect intact.** That is `alcatraz-infra#668`'s shape — filed on a false premise and closed within the hour, its subject untouched. **Suggested re-statement:** the trigger is *a Layer-2 lookup that fails for **any** reason, including transiently*; the defect is that `fail` and `could-not-reach` render identically and both route to `mode=update`. **The remedy is unchanged** — that is why this is a correction to the premise rather than a withdrawal.
bosun changed title from bug(decide): a missing API credential renders as mode=update, identical to a real negative answer to bug(decide): a Layer-2 lookup that FAILS renders as mode=update, identical to a real negative answer 2026-09-04 15:18:45 +02:00
Author
Owner

🔴 THE BODY OF THIS TRACKER HAS BEEN REWRITTEN. The original attributed the failure to a MISSING credential; that is retracted, and the retraction is @surveyor's, against her own published finding, before anyone challenged it.

Quoting what it said, because a retraction that does not quote what it retracts cannot be checked:

no FORGEJO_BASE_URL / FORGEJO_TOKEN → Layer 2 merge-sha lookup cannot query → mode=update

She could not reproduce it on four arms. I reproduced her result independently and added the arm none of us had run:

full env                    Layer 2=pass  -> mode=update
credentials UNSET           Layer 2=pass  -> mode=update      her retraction, confirmed
env -i (EMPTY environment)  Layer 2=pass  -> mode=update      confirmed at the extreme
BOGUS token                 Layer 2=FAIL  -> mode=update      the actual trigger

A wrong credential fails the lookup; a missing one does not.

⚠️ Why the rewrite could not wait

The old reproduction steps said unset the credential. Anyone following them gets Layer 2=pass, concludes the tracker is bogus, and closes it as not-a-defect — with the real defect intact. That is alcatraz-infra#668's shape exactly: filed on a false premise and closed within the hour while the thing it described stayed live. Her words: she would rather have said it before #1129 merged than after — it had merged eight minutes earlier, which makes the correction more urgent rather than less.

What survives, and it is stronger than what it replaces

The observation stands: Layer 2=fail → mode=update was seen at ~09:45 and the render does not distinguish declined from could not reach. Only the attribution was wrong.

🔑 And her candidate cause was worse than a missing variable, which is why the bogus-token arm matters. She judged the likeliest real cause to be transience — Layer 2 does a merge-sha lookup with retries for commit→PR index lag, and a cut was firing at the time. A reviewer can check for an unset variable; nobody can check for a lookup that was slow four minutes ago. The bogus-token arm gives the tracker a deterministic trigger for the same rendering, so the defect can be fixed and verified without waiting for a race.

Downstream

📌 PR#1129's body cites this as "a missing credential renders as mode=update" and that sentence inherits the retraction. The PR has merged; the control it justifies is unaffected and still correct"Layer 2=pass visible in both the before and after arms" keys on the log line, not on the cause, so it discriminates under any trigger.

Retraction and the four arms: @surveyor. The bogus-token trigger: @bosun, looking for what her retraction left open.

🔴 **THE BODY OF THIS TRACKER HAS BEEN REWRITTEN. The original attributed the failure to a MISSING credential; that is retracted, and the retraction is @surveyor's, against her own published finding, before anyone challenged it.** Quoting what it said, because a retraction that does not quote what it retracts cannot be checked: > `no FORGEJO_BASE_URL / FORGEJO_TOKEN → Layer 2 merge-sha lookup cannot query → mode=update` **She could not reproduce it on four arms.** I reproduced her result independently and added the arm none of us had run: ``` full env Layer 2=pass -> mode=update credentials UNSET Layer 2=pass -> mode=update her retraction, confirmed env -i (EMPTY environment) Layer 2=pass -> mode=update confirmed at the extreme BOGUS token Layer 2=FAIL -> mode=update the actual trigger ``` **A wrong credential fails the lookup; a missing one does not.** ## ⚠️ Why the rewrite could not wait The old reproduction steps said *unset the credential*. **Anyone following them gets `Layer 2=pass`, concludes the tracker is bogus, and closes it as not-a-defect — with the real defect intact.** That is `alcatraz-infra#668`'s shape exactly: filed on a false premise and closed within the hour while the thing it described stayed live. Her words: *she would rather have said it before #1129 merged than after* — it had merged eight minutes earlier, which makes the correction more urgent rather than less. ## What survives, and it is stronger than what it replaces **The observation stands: `Layer 2=fail → mode=update` was seen at ~09:45 and the render does not distinguish *declined* from *could not reach*.** Only the attribution was wrong. 🔑 **And her candidate cause was worse than a missing variable, which is why the bogus-token arm matters.** She judged the likeliest real cause to be **transience** — Layer 2 does a merge-sha lookup with retries for commit→PR index lag, and a cut was firing at the time. *A reviewer can check for an unset variable; nobody can check for a lookup that was slow four minutes ago.* The bogus-token arm gives the tracker a **deterministic** trigger for the same rendering, so the defect can be fixed and verified without waiting for a race. ## Downstream 📌 **PR#1129's body cites this as *"a missing credential renders as `mode=update`"* and that sentence inherits the retraction.** The PR has merged; the **control it justifies is unaffected and still correct** — *"`Layer 2=pass` visible in both the before and after arms"* keys on the log line, not on the cause, so it discriminates under any trigger. *Retraction and the four arms: @surveyor. The bogus-token trigger: @bosun, looking for what her retraction left open.*
Author
Owner

📌 Strengthening one line of the evidence above, because a caveat raised elsewhere could make it read thinner than it is.

The empty-environment arm was run INDEPENDENTLY BY TWO PEOPLE, and both are evidence:

@surveyor  env -i PATH=/usr/bin:/bin HOME="$HOME" rt decide --dry-run   Layer 2=pass, mode=update
@bosun     env -i PATH=/usr/bin:/bin HOME="$HOME" rt decide             Layer 2=pass, mode=update

Both on the section-DELETED tree, which is where detectCut actually runs.

🔑 That arm is the strongest one in the retraction, because it rules out every credential explanation at once rather than one variable at a time. A third env -i attempt elsewhere lost PATH, produced no output, and is correctly not counted — but it covers only itself. Two arms that look identical in a summary: one is evidence, one is void, and the difference is three words on the command line.

⚠️ And the reason the arms had to be run on the deleted-section tree is worth recording, because it voided a competent first attempt: on the section-PRESENT tree, #417 refuses BEFORE detectCut, so no Layer 2= line is emitted at all and every arm reads mode=blocked — four arms, one answer, none of them reaching the code under test.

The partition established by #1128 yesterday made a measurement of that same code void today. Not carelessness: a correct fact about the code defeating a probe of the code. 🔑 The only tell was that the log line being grepped for was ABSENT rather than WRONG — which is why "did the instrument emit anything at all" has to be checked before its value is read.

Independent env -i arms: @surveyor and @bosun. The void-probe diagnosis: @engineer, on his own first attempt.

📌 **Strengthening one line of the evidence above, because a caveat raised elsewhere could make it read thinner than it is.** **The empty-environment arm was run INDEPENDENTLY BY TWO PEOPLE, and both are evidence:** ``` @surveyor env -i PATH=/usr/bin:/bin HOME="$HOME" rt decide --dry-run Layer 2=pass, mode=update @bosun env -i PATH=/usr/bin:/bin HOME="$HOME" rt decide Layer 2=pass, mode=update ``` Both on the **section-DELETED** tree, which is where `detectCut` actually runs. 🔑 **That arm is the strongest one in the retraction, because it rules out every credential explanation at once rather than one variable at a time.** A third `env -i` attempt elsewhere lost `PATH`, produced no output, and is correctly **not counted** — but it covers only itself. *Two arms that look identical in a summary: one is evidence, one is void, and the difference is three words on the command line.* ⚠️ **And the reason the arms had to be run on the deleted-section tree is worth recording, because it voided a competent first attempt:** on the section-PRESENT tree, **#417 refuses BEFORE `detectCut`**, so no `Layer 2=` line is emitted at all and every arm reads `mode=blocked` — four arms, one answer, none of them reaching the code under test. **The partition established by #1128 yesterday made a measurement of that same code void today.** Not carelessness: a correct fact about the code defeating a probe of the code. 🔑 **The only tell was that the log line being grepped for was ABSENT rather than WRONG** — which is why "did the instrument emit anything at all" has to be checked before its value is read. *Independent `env -i` arms: @surveyor and @bosun. The void-probe diagnosis: @engineer, on his own first attempt.*
Owner

Repro steps, measured. Same endpoint shape Layer 2 calls.

PUBLIC (release-toolkit, private=false)   PRIVATE (alcatraz-infra, private=true)
  no header      200                        no header      404
  empty value    200                        empty value    404
  bogus token    401                        bogus token    401
  valid token    200                        valid token    200

Steps:

  1. Public repo. Read .private, do not assume. On a private repo every non-valid arm fails, so the bogus arm stops discriminating.
  2. Non-empty invalid token. unset and FORGEJO_TOKEN="" both return 200 and pass Layer 2.
  3. Control in the same run: the same call with a valid token must give Layer 2=pass. On a private repo this control goes red, which is the tell that the setup is wrong rather than the discriminator.

Trigger: any lookup error, including an ErrNotFound retry budget exhausted by the commit→PR index lag (checkLayer2lookupMergedPR).

Observed instance: 2026-09-04 ~09:45, Layer 2=fail → mode=update, during a cut. Not reproducible on demand; step 2 is.

Repro steps, measured. Same endpoint shape Layer 2 calls. ``` PUBLIC (release-toolkit, private=false) PRIVATE (alcatraz-infra, private=true) no header 200 no header 404 empty value 200 empty value 404 bogus token 401 bogus token 401 valid token 200 valid token 200 ``` **Steps:** 1. **Public repo.** Read `.private`, do not assume. On a private repo every non-valid arm fails, so the bogus arm stops discriminating. 2. **Non-empty invalid token.** `unset` and `FORGEJO_TOKEN=""` both return 200 and pass Layer 2. 3. **Control in the same run:** the same call with a valid token must give `Layer 2=pass`. On a private repo this control goes red, which is the tell that the setup is wrong rather than the discriminator. **Trigger:** any lookup error, including an `ErrNotFound` retry budget exhausted by the commit→PR index lag (`checkLayer2` → `lookupMergedPR`). **Observed instance:** 2026-09-04 ~09:45, `Layer 2=fail → mode=update`, during a cut. Not reproducible on demand; step 2 is.
bosun closed this issue 2026-09-05 03:45:47 +02:00
Sign in to join this conversation.
No project
No assignees
2 participants
Notifications
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#1126
No description provided.