Skip to content

fix(gates): composite shell -e silently killed two gates - #32

Merged
hyperpolymath merged 2 commits into
mainfrom
fix/composite-shell-e-kills-valid-repos
Sep 22, 2026
Merged

hyperpolymath merged 2 commits into
mainfrom
fix/composite-shell-e-kills-valid-repos

Conversation

@hyperpolymath

Copy link
Copy Markdown
Owner

The defect

GitHub runs every composite shell: bash step as:

bash --noprofile --norc -e -o pipefail {0}

That -e comes from the harness, not from the script, and a script's own
set -uo pipefail does not clear it — it never mentions e. So any
x=$(grep ... ) is a silent kill site whenever grep legitimately matches nothing.

Two gates were dying on it, each in the exact case it exists to handle.

1. required-files-check — killed the repos it was written to accept

functional_lines=$(grep -vE '^\s*(#|//|;|$)' "$f" | wc -l)
if [ "$functional_lines" -eq 0 ]; then
  echo "  CODEOWNERS is comment-only (valid for solo-maintained repos per Rule 1)"

A comment-only CODEOWNERS makes grep -v exit 1 → pipefail → -e kills the
step one line above the message declaring that file valid.

Measured on pons-asinorum run 35782895038. The failure is invisible twice
over: no ::error:: is ever reached, so the log simply stops after the last
successful echo, and the check-run annotation carries only
Process completed with exit code 1.

Because this is step 3 of the estate audit, its red skips 25 downstream
gates
. One unguarded pipeline has been blanking the entire audit for every
solo-maintained repo in the estate.

2. spdx-license-check — its own warning branch was unreachable

has_spdx=$(git grep -i "SPDX-License-Identifier" | wc -l)
if [ "$has_spdx" -eq 0 ]; then
   echo "::warning::No SPDX-License-Identifier lines found in the repository."
   # exit 1 (disabled strictly to prevent total CI blockage until adoption)

git grep exits 1 on zero matches, so a repo with no SPDX headers — the only
case this check exists to detect — hard-failed the step, doing the exact
opposite of what the comment beside it says.

The fix

  • required-files-check: set +e, restoring the script's own contract. It
    accumulates fail=1 so it can report every defect and exit once at the end;
    -e means "die on the first", which is the opposite contract. Both count sites
    are additionally guarded with || true, so the intent survives someone
    re-adding -e later.
  • spdx-license-check: guard the git grep so the zero-SPDX branch is
    reachable at all.
  • Swept every composite for the same shape. formatting-check and
    metrics-check already used || true / || echo 0 at every such site — the
    authors knew about this hazard and missed these two files. No other instances.

Non-vacuity

tests/composite-shell-contract.sh runs both gates under the exact CI shell
against a fixture repo that is valid under every rule those gates state
(comment-only CODEOWNERS, zero SPDX headers), and kills a mutant:

  • reinstates the unguarded pipeline and removes set +e;
  • asserts both mutations actually applied — $RF is yq-extracted, so its
    indentation is not the .yml's, and a no-op sed here produces a fake green
    (this happened while writing the file);
  • asserts the mutant parses — a mutant that fails bash -n produces a fake red;
  • requires the mutant to die silently, with no ::error:: — the CI signature.
    Without that last check the red could come from fixture drift and would prove
    nothing about -e (this also happened while writing the file: a 4-line
    MAINTAINERS stub made the mutant "die" for the wrong reason).
PASS=7 FAIL=0
  ok   mutant dies SILENTLY (rc=1), no ::error:: — the -e kill is reproduced

Wired into code-hygiene-self-test.yml with the matching paths: filters.

Verification

Reproduced locally against pons-asinorum at the same composite version CI ran
(@main = 6ff6057):

shell result
bash reqfiles.sh rc=0 — this is what made the bug invisible
bash -e -o pipefail reqfiles.sh rc=1, silent, identical to CI
after this patch, -e -o pipefail rc=0, prints the Rule 1 acceptance and the success line

Refs: #787

🤖 Generated with Claude Code

https://claude.ai/code/session_01WRvDivYwLSeVCJUrfjic3f

GitHub runs a composite `shell: bash` step as
`bash --noprofile --norc -e -o pipefail {0}`. The `-e` comes from the
harness, and a script's own `set -uo pipefail` does not clear it. Any
`x=$(grep ... )` is therefore a silent kill site whenever grep
legitimately matches nothing.

Two gates were dying on it, both in the case they exist to handle:

required-files-check
  A comment-only CODEOWNERS made `grep -v ... | wc -l` exit 1, killing
  the step ONE LINE ABOVE the message declaring that exact file valid
  ("valid for solo-maintained repos per Rule 1"). The gate killed
  precisely the repos it was written to accept. Measured on
  pons-asinorum run 35782895038: the log stops after the last
  successful echo, with no `::error::` anywhere, and the check-run
  annotation carries only "Process completed with exit code 1".
  Because this is step 3 of the estate audit, its red SKIPS 25
  downstream gates — one unguarded pipeline blanked the whole audit
  for every solo-maintained repo in the estate.

spdx-license-check
  `git grep -i "SPDX-License-Identifier" | wc -l` exits 1 on a repo
  with zero SPDX lines, so the step hard-failed and the `::warning::`
  branch beneath it was UNREACHABLE — inverting the intent stated in
  the comment beside it ("disabled strictly to prevent total CI
  blockage until adoption").

Fixes:
- required-files-check restores its own contract with `set +e`. The
  script accumulates `fail=1` to report EVERY defect and exit once at
  the end; `-e` means "die on the first", which is the opposite. Both
  count sites are additionally guarded with `|| true` so the intent
  survives someone re-adding `-e`.
- spdx-license-check guards its `git grep` so the zero-SPDX branch can
  be reached at all.
- formatting-check and metrics-check already used `|| true`/`|| echo 0`
  at every such site and are untouched; the sweep found no others.

tests/composite-shell-contract.sh runs both gates under the exact CI
shell against a fixture repo that is valid under every rule they state,
and KILLS A MUTANT: it reinstates the unguarded pipeline, asserts both
mutations actually applied, asserts the mutant parses, and requires it
to die SILENTLY with no `::error::` — the CI signature. Without that
last check the red could come from fixture drift and prove nothing
about `-e`. Wired into code-hygiene-self-test.yml. PASS=7 FAIL=0.

Refs: #787

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

coderabbitai Bot commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

Next included review available in 27 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Advanced

Run ID: d1eb2b31-f3a0-4e7b-a004-157daaabc5cd

📥 Commits

Reviewing files that changed from the base of the PR and between 6ff6057 and 6614b5f.

📒 Files selected for processing (4)
  • .github/workflows/code-hygiene-self-test.yml
  • actions/required-files-check/action.yml
  • actions/spdx-license-check/action.yml
  • tests/composite-shell-contract.sh

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

The `test` job invokes composite actions, which drag transitive `uses:`
edges into actions.lock. One of those pins is currently stale — the lock
records `hyperpolymath/cicd-suite@main` at 9adb390 while main is at
6ff6057, and `$/actions/manifest-check` resolves to the head SHA, which
has no lock entry at all — so the job dies in `prepare` with "lockfile
missing pin for hyperpolymath/deed-ecosystem@f9d999b6", before a single
step runs.

That is pre-existing (this change adds no `uses:`), and it is the same
shape as the defect this suite exists to cover: a failure that happens
before the thing being measured is ever reached. A suite that can be
masked that way is not a measurement.

So the contract suite gets its own job, touching nothing but checkout.
Its square now always reflects the suite. The lock staleness is left for
its own change rather than hand-edited here — the file is machine
generated, and `gh actions-lock` rewrite mode has a recorded history of
de-pinning SHAs and inventing invalid local refs.

Refs: #787

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

Copy link
Copy Markdown

@hyperpolymath
hyperpolymath merged commit fa71ac2 into main Sep 22, 2026
7 of 8 checks passed
@hyperpolymath
hyperpolymath deleted the fix/composite-shell-e-kills-valid-repos branch September 22, 2026 21:21
hyperpolymath added a commit that referenced this pull request Sep 22, 2026
…nsumer (#35)

## The cure was green here and inert everywhere else

`fa71ac2` (#32) cured the composite `-e` kill. `0c1bc9f` (#34) restored
the
lockfile pins #28 dropped. Both merged green. **Neither reached a single
consumer**, and nothing was red to say so.

pons-asinorum was repointed at `0c1bc9f` (pons#26) and its estate audit
*still*
died at `Required Files Gate` — 82 ms, silent, exit 1, 25 downstream
gates
skipped. Identical to before the cure.

### Why

The reusable invokes its gates by **branch** ref:

```yaml
uses: hyperpolymath/cicd-suite/actions/required-files-check@main
```

A branch ref is *trusted from the lockfile*, and the runner executes
**the commit
the lockfile names** — not the branch tip. This lock pinned
`cicd-suite@main` at
`9adb3908`, four commits back:

| commit | PR | carried |
|---|---|---|
| `0c1bc9f` | #34 | lock pins restored |
| `fa71ac2` | #32 | the `-e` cure (3 + 1 guards) |
| `6ff6057` | #31 | linguist-check |
| `3b4afaf` | #28 | (the regression) |

`9adb3908` has **0** of those guards. So no SHA a consumer picks for the
*reusable* can reach a cured *composite*. The lock's `@main` entry is
the real
gate, and re-pinning it is the actual delivery step — the one #31, #32
and #34
all skipped.

## What this does

1. Bumps `dependencies['hyperpolymath/cicd-suite@main'].commit` to
`0c1bc9f`.
Ancestor-clean fast-forward (`9adb3908` is an ancestor of `0c1bc9f`);
the
transitive `uses:` list is unchanged, so `dependencies:` needs no other
churn.
2. **Makes it impossible to forget again.**
`tests/lock-transitive-closure.sh`
gains a third assertion: for any self-referencing branch pin, the
`actions/`
tree at the locked commit must equal `HEAD`'s. On drift it names the
files.
3. `shell-contract` gains `fetch-depth: 0` so the locked commit is
present to
   compare against.

The house pattern for this is a follow-up *"pin cicd-suite lock at
`<sha>`"*
commit — `f8c8f4a`, `4f9a7a4`, `373714a` all do exactly this. It has
been
carried by memory, and memory dropped it three times running. Now it is
a test.

## Non-vacuity — twice

**The assertion was written before the bump and caught the live
defect**, naming
all three drifted composites:

```
FAIL hyperpolymath/cicd-suite@main pins 9adb390..., whose actions/ tree differs from HEAD
       actions/linguist-check/action.yml
       actions/required-files-check/action.yml
       actions/spdx-license-check/action.yml
```

And its first draft printed its header while checking *nothing* — an ERE
`sed`
has no lazy quantifiers, so `.git` stayed on the repo name and no
dependency key
ever matched. A header is not a check, so the block now carries its own
non-vacuity counter, separate from block 1's.

## Local

```
tests/lock-transitive-closure.sh   PASS=9 FAIL=0
tests/composite-shell-contract.sh  PASS=7 FAIL=0   (mutant dies silently)
```

## Acceptance

`Composite shell contract` green with the new assertion passing — and
then
pons#26's audit re-measured. Per the standing owner ruling, the gates
that have
been *skipped rather than passing* may now surface new findings; those
become
issues with acceptance criteria, not blockers on this PR.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01WRvDivYwLSeVCJUrfjic3f

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant