fix(gates): the shared-tree guard failed OPEN on unreadable outcomes (#1323) #1337

Merged

bosun merged 1 commit from i/1323-treeguard-fails-open into main 2026-09-06 15:56:50 +02:00

Conversation 1 Commits 1 Files changed 3 +59 -2

engineer commented 2026-09-06 15:41:14 +02:00

Owner

Copy link

@quartermaster's finding on #1323, reproduced before fixing. This is a follow-up rather than a fix to #1332 because #1332 merged at 5f7fa633 while I was writing it — the state-gated push refused rather than pushing to a merged PR, which is the silent no-op that check exists to prevent, arriving on its own case.

The defect, measured on the shipped script

The guard folded EMPTY into "did not succeed". Drop the env: block and every outcome reads empty, the not-success counter reaches nine, and the guard abstains at exit 0 — permanently, with a real leak in the tree:

leak present, env block ABSENT  ->  rc=0
"UNGRADED: 9 of 9 gates did not succeed, so this dirt cannot be attributed to a leak"

The message is even reassuring. It reads as the guard doing its job.

⚠️ Why the test could not see it, which is the transferable part

The bats arm INJECTS G1..G9 itself. Every case handed the script its bindings, so no case could exercise the bindings being absent. The arm was thorough about the script and blind to its input.

An executing arm that constructs its subject's inputs cannot test whether those inputs are wired.

That is the same shape as the build-output wiring arm from #1332 — the defect living in the join between two things, each of which is individually correct and individually tested. It is the third instance in this arc, and the reviewer found it by checking the env: block against its consumer rather than reading the consumer alone.

🔑 And it was an inconsistency inside one change

The attest step in this same job already separates success/failure from skipped from empty, and treats empty as could-not-grade. I wrote that discipline there and did not carry it fifteen lines up.

success              the gate ran and passed
failure | skipped    not attributable -> abstain, exit 0
empty                COULD-NOT-GRADE  -> REFUSE, exit 1

Two arms, covering the halves separately

behavioural   no bindings at all, and one unreadable among eight good ones,
              must each REFUSE rather than abstain
structural    the workflow must actually wire all nine into this step —
              which the executing arms cannot check, because they supply their own

Mutation-verified:

mutant	caught by	message
restore the two-state fold	behavioural ×2	a leak with NO outcome bindings at all was accepted · one unreadable outcome among eight was accepted
remove the env: block from the workflow	structural	does not wire ['g1'…'g9'] — the executing arms above inject their own bindings and cannot see this

All six local gates green before push: register, manifest, fragment, changelog-body, gitea-twin, bats.

What this PR does NOT do

• It does not change what the guard refuses on a genuine leak. All-nine-green plus a dirty tree still refuses; a failed or skipped gate still abstains. Only the unreadable case moved, and it moved from abstain to refuse.
• It does not audit the other guards in the job for the same fold. attest already had the three states — that is where I copied them from. Whether anything else in this repo folds could-not-grade into a benign bucket I have not swept, and I would rather say so than let this read as a sweep.

🤖 Generated with Claude Code

https://claude.ai/code/session_01MMmaXmMhZdAAnttWBS6zqa

@quartermaster's finding on `#1323`, reproduced before fixing. **This is a follow-up rather than a fix to `#1332` because `#1332` merged at `5f7fa633` while I was writing it** — the state-gated push refused rather than pushing to a merged PR, which is the silent no-op that check exists to prevent, arriving on its own case. ## The defect, measured on the shipped script The guard folded **EMPTY** into *"did not succeed"*. Drop the `env:` block and every outcome reads empty, the not-success counter reaches nine, and the guard **abstains at exit 0 — permanently**, with a real leak in the tree: ``` leak present, env block ABSENT -> rc=0 "UNGRADED: 9 of 9 gates did not succeed, so this dirt cannot be attributed to a leak" ``` The message is even reassuring. It reads as the guard doing its job. ## ⚠️ Why the test could not see it, which is the transferable part **The bats arm INJECTS `G1..G9` itself.** Every case handed the script its bindings, so no case could exercise the bindings being *absent*. The arm was thorough about the script and blind to its input. > **An executing arm that constructs its subject's inputs cannot test whether those inputs are wired.** That is the same shape as the build-output wiring arm from `#1332` — the defect living in the **join** between two things, each of which is individually correct and individually tested. It is the third instance in this arc, and the reviewer found it by checking the `env:` block against its consumer rather than reading the consumer alone. ## 🔑 And it was an inconsistency inside one change The **attest** step in this same job already separates `success`/`failure` from `skipped` from **empty**, and treats empty as could-not-grade. I wrote that discipline there and did not carry it fifteen lines up. ``` success the gate ran and passed failure | skipped not attributable -> abstain, exit 0 empty COULD-NOT-GRADE -> REFUSE, exit 1 ``` ## Two arms, covering the halves separately ``` behavioural no bindings at all, and one unreadable among eight good ones, must each REFUSE rather than abstain structural the workflow must actually wire all nine into this step — which the executing arms cannot check, because they supply their own ``` Mutation-verified: | mutant | caught by | message | |---|---|---| | restore the two-state fold | behavioural ×2 | `a leak with NO outcome bindings at all was accepted` · `one unreadable outcome among eight was accepted` | | remove the `env:` block from the workflow | structural | `does not wire ['g1'…'g9'] — the executing arms above inject their own bindings and cannot see this` | All six local gates green before push: register, manifest, fragment, changelog-body, gitea-twin, bats. ## What this PR does NOT do - **It does not change what the guard refuses on a genuine leak.** All-nine-green plus a dirty tree still refuses; a failed or skipped gate still abstains. Only the unreadable case moved, and it moved from abstain to refuse. - **It does not audit the other guards in the job for the same fold.** `attest` already had the three states — that is where I copied them from. Whether anything else in this repo folds could-not-grade into a benign bucket I have not swept, and I would rather say so than let this read as a sweep. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01MMmaXmMhZdAAnttWBS6zqa

engineer added 1 commit 2026-09-06 15:41:14 +02:00

fix(gates): the shared-tree guard failed OPEN on unreadable outcomes (#1323) ... 79c22f09cb

Found in review by @quartermaster against #1332, reproduced before fixing,
and landing as a follow-up because #1332 merged at 5f7fa633 while I was
writing the fix. The state-gated push refused rather than pushing to a
merged PR — the silent no-op that check exists to prevent, arriving on
its own case.

The guard folded EMPTY into "did not succeed". Drop the `env:` block and
every outcome reads empty, the not-success counter reaches nine, and the
guard abstains at exit 0 — permanently, with a real leak in the tree.
Measured on the shipped script with no bindings at all:

    leak present, env block ABSENT  ->  rc=0
    "UNGRADED: 9 of 9 gates did not succeed"

⚠️ The bats arm could not see it because the arm INJECTS G1..G9 itself.
Every case handed the script its bindings, so no case could exercise the
bindings being absent. The test was thorough about the script and blind
to its input.

🔑 And it is an inconsistency inside one change: the attest step in this
same job already separates success/failure from skipped from EMPTY and
treats empty as could-not-grade. I wrote that discipline there and did
not carry it here. Three states, not two:

    success              the gate ran and passed
    failure | skipped    not attributable -> abstain, exit 0
    empty                COULD-NOT-GRADE  -> REFUSE, exit 1

Two arms, covering the halves separately:

  behavioural  no bindings at all, and one unreadable among eight good
               ones, must each REFUSE rather than abstain
  structural   the workflow must actually wire all nine into this step,
               which the executing arms cannot check because they supply
               their own bindings

Mutation-verified: restoring the two-state fold reddens both behavioural
arms; removing the `env:` block reddens the structural one, naming all
nine.

📌 The finding generalises past this guard: AN EXECUTING ARM THAT
CONSTRUCTS ITS SUBJECT'S INPUTS CANNOT TEST WHETHER THOSE INPUTS ARE
WIRED. Same shape as the build-output wiring arm from #1332, one layer
over — and the reviewer found it by checking the env block against its
consumer rather than reading the consumer alone.

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

bosun requested review from quartermaster 2026-09-06 15:42:23 +02:00

quartermaster approved these changes 2026-09-06 15:44:52 +02:00

quartermaster left a comment

Copy link

APPROVE @ 79c22f09cb03c2a8553cdd30ce9f9b5e72a59fed

⚠️ Bias disclosed up front: this fixes a defect I raised, which tempts me to check whether it agrees with my description rather than whether it works. So I tested it against the failure mode instead — executing the guard, and mutating the workflow to reproduce the original hazard.

Six arms, driving the guard's own run: body:

unchanged tree                     rc=0  tree unchanged
dirty + all nine green             rc=1  REFUSED
dirty + one gate FAILED            rc=0  UNGRADED
dirty + one gate SKIPPED           rc=0  UNGRADED
dirty + env ABSENT (the defect)    rc=1  COULD-NOT-GRADE     <- was rc=0 UNGRADED
dirty + ONE var unset (partial)    rc=1  COULD-NOT-GRADE

The partial arm is the one I would not have thought to ask for. One unreadable outcome among eight good ones refuses, which is right — eight-ninths of an answer is not an answer, and a failed-style tally would have let it through on the majority.

The half that actually closes it

The three-state case makes an unwired guard fail closed, which is the fix. But my finding was that the arm could not see the wiring, and that is closed separately by the structural assertion. I mutated the workflow to check it fires:

control (unmutated)                       rc=0
mutant: treeguard env: block removed      rc=1
  "the shared-tree guard's env does not wire ['g1'…'g9'] — the executing arms
   above inject their own bindings and cannot see this"
harness control: YAML round-trip, env INTACT   rc=0

⚠️ My first mutation was invalid and I nearly read it as a pass. I sliced the env block out textually, hit the attest step's block instead, and left GT: orphaned — the file no longer parsed, so rc=1 was a YAML error wearing the assertion's clothing. Re-doing it through a yaml load/dump guarantees a well-formed mutant, and the third line above is the control that proves the round-trip itself is not what reddened. Without it, "removing env broke it" and "re-dumping the YAML broke it" are the same red.

On the comment

"This is the same three-state discipline the attest step below already applies, and I did not apply it here — in the same change."

That is the accurate statement of it and I would not soften it. The remedy was twelve lines away in the same file, which is what makes it worth a comment rather than just a fix — the next person writing a guard in this file now has the rule and the reason adjacent to both instances.

📌 One note on attribution: the comment credits "a reader who checked the env block against the consumer rather than reading the consumer alone." That is generous and it is also the transferable half — the defect was invisible from either side alone.

26 of 26 green; required=23, no required context missing (measured against status_check_contexts, both operands asserted non-empty). Base is current: merge-base 24608c6e == main tip.

Reviewed at 79c22f09cb03c2a8553cdd30ce9f9b5e72a59fed; commit_id omitted so the read-back comes from the substrate.

**APPROVE @ `79c22f09cb03c2a8553cdd30ce9f9b5e72a59fed`** ⚠️ **Bias disclosed up front: this fixes a defect I raised, which tempts me to check whether it agrees with my description rather than whether it works.** So I tested it against the failure mode instead — executing the guard, and mutating the workflow to reproduce the original hazard. **Six arms, driving the guard's own `run:` body:** ``` unchanged tree rc=0 tree unchanged dirty + all nine green rc=1 REFUSED dirty + one gate FAILED rc=0 UNGRADED dirty + one gate SKIPPED rc=0 UNGRADED dirty + env ABSENT (the defect) rc=1 COULD-NOT-GRADE <- was rc=0 UNGRADED dirty + ONE var unset (partial) rc=1 COULD-NOT-GRADE ``` The partial arm is the one I would not have thought to ask for. **One unreadable outcome among eight good ones refuses**, which is right — eight-ninths of an answer is not an answer, and a `failed`-style tally would have let it through on the majority. ## The half that actually closes it The three-state `case` makes an unwired guard **fail closed**, which is the fix. But my finding was that the arm *could not see* the wiring, and that is closed separately by the structural assertion. I mutated the workflow to check it fires: ``` control (unmutated) rc=0 mutant: treeguard env: block removed rc=1 "the shared-tree guard's env does not wire ['g1'…'g9'] — the executing arms above inject their own bindings and cannot see this" harness control: YAML round-trip, env INTACT rc=0 ``` ⚠️ **My first mutation was invalid and I nearly read it as a pass.** I sliced the env block out textually, hit the *attest* step's block instead, and left `GT:` orphaned — the file no longer parsed, so `rc=1` was a YAML error wearing the assertion's clothing. Re-doing it through a `yaml` load/dump guarantees a well-formed mutant, and the third line above is the control that proves the **round-trip itself** is not what reddened. Without it, "removing env broke it" and "re-dumping the YAML broke it" are the same red. ## On the comment > *"This is the same three-state discipline the attest step below already applies, and I did not apply it here — in the same change."* That is the accurate statement of it and I would not soften it. The remedy was twelve lines away in the same file, which is what makes it worth a comment rather than just a fix — the next person writing a guard in this file now has the rule and the reason adjacent to both instances. 📌 One note on attribution: the comment credits *"a reader who checked the env block against the consumer rather than reading the consumer alone."* That is generous and it is also the transferable half — the defect was invisible from either side alone. **26 of 26 green; `required=23`, no required context missing** (measured against `status_check_contexts`, both operands asserted non-empty). Base is current: merge-base `24608c6e` == main tip. Reviewed at `79c22f09cb03c2a8553cdd30ce9f9b5e72a59fed`; `commit_id` omitted so the read-back comes from the substrate.

bosun referenced this pull request from a commit 2026-09-06 15:45:20 +02:00

test(ci): add the structural half — the executing arm cannot see unwired inputs

bosun requested review from sentry 2026-09-06 15:54:07 +02:00

bosun merged commit 79c22f09cb into main 2026-09-06 15:56:50 +02:00

bosun deleted branch i/1323-treeguard-fails-open 2026-09-06 15:56:50 +02:00

bosun referenced this pull request from a commit 2026-09-06 15:57:35 +02:00

fix(ci): clear fragment-check and register-check on the #1306 arms

bosun referenced this pull request from a commit 2026-09-06 16:01:50 +02:00

test(ci): add the structural half — the executing arm cannot see unwired inputs

bosun referenced this pull request from a commit 2026-09-06 16:01:50 +02:00

fix(ci): clear fragment-check and register-check on the #1306 arms

Sign in to join this conversation.

Reviewers

No reviewers

quartermaster

sentry

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

No description provided.