Skip to content

chore(shell): drop the pipe into early-closing readers (backend#2264) - #544

Merged
LukasWodka merged 2 commits into
developfrom
chore/2264-pipefail-early-close
Aug 21, 2026
Merged

chore(shell): drop the pipe into early-closing readers (backend#2264)#544
LukasWodka merged 2 commits into
developfrom
chore/2264-pipefail-early-close

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Part of tracebloc/backend#2264.

What

Seven pipelines fed a reader that closes before EOF — | grep -q, | head -1. Under set -euo pipefail that shape can return 141: the reader closes, the producer takes SIGPIPE, and pipefail surfaces the signal as the pipeline's status.

filesites
scripts/coverage-floor.sh2
scripts/file-budget.sh1
scripts/sync-backend-fixtures.sh2
scripts/sync-schema.sh2

Not a bug fix, and I measured rather than assumed

None of the seven is reachable. Every producer here is a bash builtin printf emitting a single short token — $min, $max, $line, $BACKEND_REF. Two independent reasons it cannot fire: the payload is far below the ~64KB pipe buffer, and a single-line producer cannot SIGPIPE a grep -q in the first place, because grep has to read to end-of-line before it can report a match, so -q drains the input regardless of where the needle sits. I measured that second point specifically (one 200KB line into grep -q → exit 0; 40k short lines → exit 141) because it is the difference between "hazardous shape" and "latent bug", and the ticket's offender list only knows about shape.

So: a consistency change that lets the shared gate be armed with this repo already green — not a defect being repaired.

Verification — both directions, on the converted conditions

The risk in a change like this is turning a guard into a no-op, so each converted condition was exercised for what it must reject and what it must still accept:

Ref validators (sync-schema.sh, sync-backend-fixtures.sh)

  • reject with exit 2: ../../etc/passwd, a..b, -badstart, x;rm -rf /, a b
  • accept and proceed: main, abc123/def-1.2_3, a 40-char SHA

Malformed-entry guards (FLOORS, BUDGETS)

  • caught: pkg (no colon), pkg:abc, pkg:12x, pkg:-5
  • well-formed still works: ok: internal/cli/data.go 248 <= 500

Coverage percentage extraction — the sed … | head -1 that became sed … <<< plus a bash slice — still reads a real number off go test -cover: ok: ./internal/cli/ 85.0% >= 1%

Every fixture asserted that it actually applied before the run, so an inert mutation cannot be mistaken for coverage.

make file-budget and make check-style pass. shellcheck -S warning clean across scripts/.

One note for the parent ticket

Converting these surfaced a false positive in the scanner backend#2264 is built on: it flags a || grep -q x <<<"$y", because its regex matches the second | of || as a pipe. That is the recommended form. I'm fixing the scanner in client separately; it does not affect the correctness of this diff, but the gate can't be armed fleet-wide until it's fixed.

🤖 Generated with Claude Code


Note

Low Risk
Mechanical shell-syntax change in CI helper scripts; validation logic is equivalent and not on a production runtime path.

Overview
Rewrites seven printf | grep / sed | head pipelines in CI ratchet and sync scripts to here-strings (grep … <<< "$var"), so set -o pipefail cannot surface SIGPIPE (exit 141) when a reader closes early.

Touched: integer-floor guards in coverage-floor.sh and file-budget.sh, coverage percent extraction (sed + bash slice instead of | head -1), and git-ref validators in sync-schema.sh / sync-backend-fixtures.sh. Behavior of the guards is unchanged; this is a consistency cleanup for a fleet-wide pipefail gate, not a live bug.

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

Seven pipelines fed a reader that closes before EOF (`| grep -q`, `| head -1`).
Under `set -euo pipefail` that shape can return 141: the reader closes, the
producer takes SIGPIPE, pipefail surfaces the signal as the pipeline status.
MEASURED, none of the seven is reachable. Every producer here is a bash
builtin `printf` emitting a single short token ("$min", "$max", "$line",
"$BACKEND_REF"), and a single-line producer cannot SIGPIPE a `grep -q`
anyway -- grep must read to end-of-line before it can report the match, so
-q drains the input regardless. This is a shape conversion so the shared
gate can be armed with the repo already green, NOT a bug fix.
scripts/coverage-floor.sh 2 sites
scripts/file-budget.sh 1 site
scripts/sync-backend-fixtures.sh 2 sites
scripts/sync-schema.sh 2 sites
Verified behaviourally, both directions, on the converted conditions:
ref validators REJECT (exit 2) ../../etc/passwd, a..b, -badstart,
'x;rm -rf /', 'a b'
ACCEPT main, abc123/def-1.2_3, a 40-char SHA
malformed guards catch 'pkg' (no colon), 'pkg:abc', 'pkg:12x', 'pkg:-5'
for BUDGETS and FLOORS alike; a well-formed entry still
reports "ok: internal/cli/data.go 248 <= 500"
pct extraction still reads a real number: "ok: ./internal/cli/ 85.0% >= 1%"
Each fixture was asserted to actually apply before its run, so an inert
mutation cannot be mistaken for coverage.
`make file-budget` and `make check-style` pass; shellcheck -S warning clean
across scripts/.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodkaLukasWodka self-assigned this Aug 20, 2026
Comment threadinternal/api/testdata/edge_device_create.json Outdated
Comment threadinternal/schema/ingest.v1.json Outdated
…nd#2264)
Self-inflicted, and worth naming precisely. To check that the converted ref
validators still ACCEPT valid refs, I ran
DATA_INGESTORS_REF=main bash scripts/sync-schema.sh
BACKEND_REF=develop bash scripts/sync-backend-fixtures.sh
These scripts are not predicates. On a valid ref they do the sync -- they
downloaded from unpinned refs and rewrote four tracked files, and `git add -A`
swept them into the conversion commit:
internal/schema/ingest.v1.json (+79 lines from main)
internal/api/testdata/edge_device_adopt.json ("status": 0 -> 2)
internal/api/testdata/edge_device_create.json
internal/api/testdata/edge_device_patch_cluster_id.json
That is a silent supply-chain change to PINNED content, which is exactly what
`.data-ingestors-ref` and `.backend-ref` exist to prevent, and it is what
reddened Schema drift, Backend fixtures drift, Test and Integration (kind).
The drift checks did their job.
Restored all four from origin/develop. The PR is back to the four script
changes it claims to be.
The lesson is the harness, not the scripts: a verification step that INVOKES a
mutating command is not a read-only check, and `git add -A` after running one
cannot tell the difference. The rejection cases (exit 2 before any network
call) were safe; the acceptance cases were not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Correction — I contaminated my own PR, and the drift gates caught it

The first push reddened Schema drift, Backend fixtures drift, Test and Integration (kind). Not a flake, and not the conversion — my verification harness did it.

To check that the converted ref validators still accept valid refs, I ran:

DATA_INGESTORS_REF=main bash scripts/sync-schema.sh
BACKEND_REF=develop bash scripts/sync-backend-fixtures.sh

These scripts are not predicates. Given a valid ref they do the sync — they downloaded from unpinned refs and rewrote four tracked files, and git add -A swept them into the conversion commit:

filedamage
internal/schema/ingest.v1.json+79 lines pulled from main
internal/api/testdata/edge_device_adopt.json"status": 02
internal/api/testdata/edge_device_create.jsonsame class
internal/api/testdata/edge_device_patch_cluster_id.jsonsame class

That is a silent change to pinned content — precisely what .data-ingestors-ref and .backend-ref exist to prevent. Restored all four from origin/develop in the follow-up commit; the PR is back to the four script changes it claims to be.

Two things worth keeping:

  1. The drift gates worked exactly as designed. They are the reason this is a PR comment and not a merged supply-chain change. Whoever built them: they earned their keep today.
  2. The lesson is the harness, not the scripts. A verification step that invokes a mutating command is not a read-only check, and git add -A afterwards cannot tell the difference. The rejection cases were safe (they exit 2 before any network call); the acceptance cases were not. I should have asserted on --check or a dry run, or committed with an explicit path list.

Re-verified after the restore: the four script changes are unchanged and the behavioural evidence in the PR body still stands — that testing was done before the sync runs, on the validators themselves.

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

bugbot run

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 79fc0b2. Configure here.

@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. Twenty-six checks green including Bugbot, zero open threads, MERGEABLE.

I checked all five conversions for equivalence rather than eyeballing the shape, including the edge case that usually bites here — the empty value. printf '%s' "" emits no lines at all, while <<<"" emits one empty line; both fail an anchored ^[0-9]+$ or ^[A-Za-z0-9]…$ match, so the branch is taken either way. Multi-line values behave identically too, since grep -q succeeds on any matching line in both forms.

The detail that tells me this was done attentively rather than mechanically is coverage-floor.sh:

pct="$(sed -nE 's/.*coverage: ([0-9]+(\.[0-9]+)?)% of statements.*/\1/p'<<<"$line")"
pct="${pct%%$'\n'*}"

No || true — and that's correct, where the same conversion on release-train#103 needed one. There the pipeline began with grep -F, which exits 1 on no match, so dropping head -1 (always 0) turned a documented miss into an errexit abort. Here the producer is sed -n, which exits 0 whether or not it matched, so the assignment can't fail. A mechanical sweep would have gone wrong in one of two directions: omitting the || true that #103 needed, or adding one here that would then mask a genuine sed failure. Neither happened.

One observation, pre-existing rather than introduced, and worth taking while you're in these files. The ref guards in sync-backend-fixtures.sh and sync-schema.sh are line-oriented, so they can't express what they mean — "the whole value is a valid ref". grep -qE '^[A-Za-z0-9][A-Za-z0-9._/-]*$' succeeds if any line matches, so BACKEND_REF=$'develop\nmain' passes the charset check on line 1 and then lands in the URL built at :126 ("${API_BASE}/${name}?ref=${BACKEND_REF}"). BACKEND_REF comes from the environment, so that's reachable input rather than a fixed file.

Practically it's mostly covered: the separate grep -q '\.\.' fires on any line, so the traversal case — the one that actually matters — is caught regardless of where in the value it sits. The gap is narrower than it first looks.

Still, the exact form costs less than what's there and fits this sweep's own theme better, since it drops two more subprocesses rather than converting them:

case"$BACKEND_REF"in""|*[!A-Za-z0-9._/-]*) invalid=1 ;; # any char outside the set — newline included*..*) invalid=1 ;;
esac

case matches against the whole string, so a newline fails on the character class itself and there is no line-splitting to reason about. Entirely your call whether that belongs in a sweep PR or a follow-up — the conversions as they stand are correct and I'm not holding this for it.

@LukasWodka
LukasWodka merged commit af22d8f into developAug 21, 2026
26 checks passed
@LukasWodka
LukasWodka deleted the chore/2264-pipefail-early-close 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