Skip to content

chore(tests): mark the one early-close finding as the false positive it is (backend#2264) - #296

Merged
LukasWodka merged 1 commit into
developfrom
chore/2264-pipefail-guard-marker
Aug 21, 2026
Merged

chore(tests): mark the one early-close finding as the false positive it is (backend#2264)#296
LukasWodka merged 1 commit into
developfrom
chore/2264-pipefail-guard-marker

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Part of tracebloc/backend#2264.

The one finding here is a false positive, and the marker says so

The fleet sweep reported exactly one offender in this repo:

scripts/tests/house-rules-selftest.sh:474
NF=$(printf '%s' "$OUT" | grep -oE 'across [0-9]+ file' | grep -oE '[0-9]+' | head -1)

It carries no hazard:

  • this file runs under set -uo pipefail (line 37) — no errexit, so a 141 could not abort anything
  • and two grep -oE filters over one line of output could not SIGPIPE regardless

Why the scanner reports it: this suite passes whole scripts as multi-line quoted arguments to expect, and each fixture opens with set -euo pipefail. The scanner reads those as this file's options and concludes errexit is live.

expect "a bare curl fires both rules" \
'#!/bin/bashset -euo pipefailcurl -fsSL "$url" -o out' curl-timeout,curl-tls

Why a marker rather than a scanner fix

I tried the scanner fix and rejected it. Shell has no escape inside '…', so counting quotes is exact in principle — but apostrophes in prose ("the file's options") desynchronise the count, and measured, it is already out of phase by line 37 of this very file, before any fixture appears. A desynchronised count hides real offenders, which is strictly worse for a gate than reporting a false one.

So the limitation is documented in the scanner header and pinned by a test in tracebloc/client#777, and the affected line here carries # pipefail-guard: allow with the reason written out — a marker that explains, not a silencer.

This repo now reads clean for when the shared gate is armed.

Verification

  • the selftest suite still passes: 41 passed, 0 failed
  • shellcheck -S warning and bash -n clean
  • the scanner reports zero for the repo

🤖 Generated with Claude Code


Note

Low Risk
Comment-and-marker only on a test script; no matcher, CI, or runtime behavior change.

Overview
Marks the | head -1 pipeline in house-rules-selftest.sh with # pipefail-guard: allow so the shared scanner no longer flags it.

The pipeline is not a real early-close hazard: the suite uses set -uo pipefail without errexit, and the scanner only fires because it treats set -euo pipefail inside quoted fixture scripts as this file’s own options. A comment documents that limitation instead of changing the matcher.

Reviewed by Cursor Bugbot for commit 77cc2f0. Bugbot is set up for automated code reviews on this repo. Configure here.

…it is (backend#2264)
The #2264 fleet sweep reported exactly one offender in this repo:
house-rules-selftest.sh:474
NF=$(printf '%s' "$OUT" | grep -oE 'across [0-9]+ file' | ... | head -1)
It is not a hazard. This file runs under `set -uo pipefail` (line 37) with NO
errexit, so a 141 could not abort anything -- and two `grep -oE` filters over
one line of output could not SIGPIPE in the first place.
The scanner reports it because it reads the `set -euo pipefail` lines inside
this suite's QUOTED FIXTURE SCRIPTS -- whole scripts passed as multi-line
arguments to `expect` -- as this file's own options. That limitation is
documented and pinned by a test in tracebloc/client (client#777); tracking
quoted regions by counting apostrophes was tried and rejected, because prose
apostrophes desynchronise the count and a desynchronised count HIDES real
offenders.
So the marker carries the reason, not just a silencer -- and this repo now
reads clean for when the shared gate is armed.
Verified: the selftest suite still passes (41 passed, 0 failed); shellcheck
and bash -n clean; the scanner reports zero for the repo.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodkaLukasWodka self-assigned this Aug 20, 2026

@saadqbalsaadqbal left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. All twelve checks green including Bugbot, MERGEABLE. I verified all four claims rather than taking the false-positive call on trust, since a PR that declares a gate finding invalid is exactly the one to check.

1. errexit really is not live.set -uo pipefail at line 37 is this file's only real option statement. There are 27 further set -euo pipefail lines sitting at column 1 — which is precisely the ambiguity you describe — but expect() does f=$(fixture "t$$.sh" "$body") then run_on "$f", so fixture text is written to disk and scanned, never executed or sourced in this shell. No eval, and the three source hits in the file are comments discussing the concept. So a 141 at :474 sets $? and aborts nothing.

2. The producer cannot SIGPIPE anyway. Two grep -oE filters over one line of --all summary output, so the second claim holds independently of the first. Two reasons, either sufficient — worth having both stated.

3. The scanner's misreading is real, and 27 occurrences is enough that no amount of tightening the option-detection heuristic gets there.

4. "Already out of phase by line 37 of this very file" is exactly right, and I measured it too. Five apostrophes in prose comments before line 37 — house-rules.sh's matcher (:3), the release train's (:9), #1788's own argument (:28), The ticket's evidence (:30), THIS REPO'S OWN (:33) — odd parity, so a quote-counting scanner believes it is inside a single-quoted string from line 3 onward.

Declining the scanner fix is the right call and the reasoning is the part worth keeping."A desynchronised count hides real offenders, which is strictly worse for a gate than reporting a false one" — that's the correct ordering for a gate, and it's the kind of judgement that reads as laziness if it isn't argued. You argued it, and then did the two things that make a marker legitimate rather than a silencer: documented the limitation in the scanner header and pinned it with a test in tracebloc/client#777, so the false positive is a known property of the tool rather than a mystery someone re-investigates next quarter.

The marker itself is the right shape — # pipefail-guard: allow with eight lines of reason above the line, and the code deliberately unchanged because there is nothing to fix. A marker that explains beats one that suppresses, and this one tells the next reader both why the finding fires and why it does not matter here.

One thing worth carrying into the sweep's other repos: the scanner's blind spot is "quoted script bodies inside a test harness", so any repo whose suites embed fixture scripts the same way will produce the same false positive. If more than one or two turn up, the honest fix might be for the scanner to skip files that declare themselves harnesses rather than to count quotes — but that is a decision for the sweep, not for this PR.

@LukasWodka
LukasWodka merged commit 09da430 into developAug 21, 2026
12 checks passed
@LukasWodka
LukasWodka deleted the chore/2264-pipefail-guard-marker branch August 21, 2026 06:39
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

/fr-pass

Sign up for freeto 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.

2 participants

@LukasWodka@saadqbal