🐛 fix(ci): make pipefail dead-fallback handlers actually reachable#599
Conversation
The step resolves the highest dev/vX.Y on origin and falls back to the default branch when there is none, so translations always have somewhere to land in the window between a GA and the next dev branch being cut. That fallback was dead code. Under `set -euo pipefail` a no-match grep exits 1, which fails the whole pipeline, which fails the assignment, which aborts the step before the `if [ -z ]` check ever runs. It fired for real on 2026-07-24: merging sync PR #596 briefly deleted dev/v1.6 via auto-delete-head-branches, the push-to-main sync run found no dev/v*, and the job died with a bare exit 1 instead of retargeting main. Guard just the grep rather than the whole substitution, so a failing ls-remote (network, auth) still propagates and fails loudly instead of silently retargeting translations at main.
Executes the real `id: base` run block extracted from i18n-crowdin.yml against a stubbed git, rather than asserting on the workflow text, so the test tracks the actual logic instead of a copy that can drift. Three cases: no dev/vX.Y on origin falls back to the default branch and exits 0 (this is the one that fails against the pre-fix snippet, with `expected 1 to be +0`); several dev branches picks dev/v1.10 over v1.9, which a lexical sort would get wrong; and a failing ls-remote still exits non-zero without silently retargeting main — that last one guards against "fixing" the bug by swallowing every error, which would be worse than the bug.
Same pipefail dead-fallback class as the Crowdin bug, found by sweeping
the rest of the workflows for it.
The load-test steps resolve their newest report with
report="$(find artifacts/load-test/<mode> ... | sort -rn | head -n1 ...)"
find exits 0 on a directory that exists but is empty, which is the case
the downstream scripts handle. It exits 1 when the directory is missing
entirely, and GitHub runs these steps under the default `bash -eo
pipefail`, so that aborts the step. summarize-load-test-report.sh and
check-load-test-correctness.sh both open with an `if [ -z "${REPORT}" ]`
block written for exactly this situation, and neither ever gets to run.
The missing-directory case is reachable, not theoretical: these steps are
`if: always()`, and run-load-test.sh only does `mkdir -p` on the artifact
dir at line 168, after the docker build, compose up, and the health-check
wait that exits 1 at line 136. So a load test that fails the way load
tests usually fail leaves the directory absent, and the step that was
supposed to explain that dies with a bare exit 1 instead.
Guards all 8 sites. The accompanying test discovers them by walking the
parsed workflow rather than pinning line numbers, so it also catches a
new unguarded find added later, and carries a sanity assertion so it
can't pass vacuously if the shape ever stops matching.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughCI load-test jobs guard Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/tests/ci-verify-find-pipefail-guard.test.ts:
- Around line 86-89: Update findFindAssignmentSites validation and the test
using GUARDED_FIND_PREFIX so pipelines cannot pass when later find stages remain
unguarded under pipefail. Require exactly one find command per assignment, or
inspect every find stage and ensure each has the missing-directory guard, while
preserving acceptance of valid guarded assignments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b1f003ac-4f10-41e2-8ca7-3f9482ebb748
📒 Files selected for processing (4)
.github/tests/ci-verify-find-pipefail-guard.test.ts.github/tests/crowdin-base-branch-resolver.test.ts.github/workflows/ci-verify.yml.github/workflows/i18n-crowdin.yml
Per CodeRabbit on #599. The guard regex is anchored, so it only vouches for the first stage — `{ find a || true; } | find b ...` passed while its second find could still abort under pipefail. Requires exactly one find per assignment rather than trying to parse each stage. Every current site has one, and a second should fail this test so someone has to look at it. Confirmed the double-find case passes before this change and fails after.
|
Good catch on the anchored regex, that was a real hole. Fixed in 3d651b1. Went with requiring exactly one |
The sweep matched one physical line at a time, so folding a site across a backslash continuation dropped it from the results entirely. The sanity check only asserted `> 0`, so going from 8 sites to 7 still passed. A guard that reports green while covering less than it did is worse than no guard. Folds continuations into logical lines before matching, so a reformatted site is still found, and pins the expected count so a change in either direction has to be made deliberately. Verified: a folded-but-guarded site still counts 8 and passes, and deleting a site now fails with `expected 7 to be 8` instead of passing.
Same pipefail dead-fallback as the workflows. cpu-bench.sh sets `set -euo pipefail` at line 13, so a container with no matching samples made the grep exit 1 and killed the whole script, before awk could print the N/A it has a branch for. Only a manual dev tool, nothing in CI calls it, but it's the same bug and the same one-line fix.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/tests/ci-verify-find-pipefail-guard.test.ts:
- Around line 51-63: The joinContinuations function incorrectly treats
backslashes followed by whitespace as shell continuations and inserts a space
where shell continuation removes the newline. Restrict continuation detection to
a single backslash that is the final character of rawLine, remove only that
backslash, and preserve the existing logical-line flushing behavior for
non-continued lines and trailing pending content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 704e9844-b73b-4f8a-bd31-24790faf40fb
📒 Files selected for processing (2)
.github/tests/ci-verify-find-pipefail-guard.test.tstest/cpu-bench.sh
| // Reconstructs logical shell lines from a `run:` block by folding backslash | ||
| // continuations, so a site reformatted across `\` line breaks is still | ||
| // discovered by the single-line FIND_ASSIGNMENT_LINE regex below. A line | ||
| // ending in a trailing `\` (optionally followed by trailing whitespace) | ||
| // continues onto the next physical line. | ||
| function joinContinuations(run: string): string[] { | ||
| const logicalLines: string[] = []; | ||
| let pending = ''; | ||
|
|
||
| for (const rawLine of run.split('\n')) { | ||
| const continued = /\\\s*$/.test(rawLine); | ||
| const chunk = continued ? rawLine.replace(/\\\s*$/, '') : rawLine; | ||
| pending += (pending ? ' ' : '') + chunk.trim(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file and inspect relevant sections"
fd -a 'ci-verify-find-pipefail-guard.test.ts' . || true
if [ -f .github/tests/ci-verify-find-pipefail-guard.test.ts ]; then
wc -l .github/tests/ci-verify-find-pipefail-guard.test.ts
sed -n '1,180p' .github/tests/ci-verify-find-pipefail-guard.test.ts | cat -n
fi
echo
echo "Search related function/usages"
rg -n "joinContinuations|FIND_ASSIGNMENT_LINE|pipefail|continuation" .github . --glob '!node_modules' --glob '!dist' --glob '!build' || trueRepository: CodesWhat/drydock
Length of output: 15733
🌐 Web query:
POSIX shell command line continuation backslash immediately before newline trailing whitespace behavior
💡 Result:
In the POSIX shell command language, when a backslash immediately precedes a newline character, it is interpreted as line continuation [1][2][3]. The shell removes both the backslash and the newline character from the input before the tokenization process begins [1][4]. Regarding trailing whitespace, the POSIX standard explicitly states that the escaped newline is removed entirely and is not replaced by any whitespace [1][2][5]. Consequently, if there is trailing whitespace (such as spaces or tabs) on the same line after the backslash but before the newline, that whitespace remains in the input stream and is not part of the line continuation sequence. Because the backslash only acts as a line continuation character when it is immediately followed by a newline, any intervening characters—including whitespace—prevent the backslash from being treated as a line continuation [1][2]. In such cases, the backslash is treated as a literal character (unless quoted), and the newline remains a newline character, effectively ending the command line at that point [1][4].
Citations:
- 1: https://pubs.opengroup.org/onlinepubs/9799919799/utilities/V3_chap02.html
- 2: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
- 3: https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/utilities/V3_chap02.html
- 4: https://pubs.opengroup.org/onlinepubs/007908775/xcu/chap2.html
- 5: https://pubs.opengroup.org/onlinepubs/9699919799.2016edition/xrat/V4_xcu_chap02.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import re
def current(join_):
logical_lines = []
pending = ''
for line in join_.split('\n'):
continued = bool(re.search(r'\\\s*$', line))
chunk = re.sub(r'\\\s*$', '', line) if continued else line
pending += (' ' if pending else '') + chunk.strip()
if not continued:
logical_lines.append(pending)
pending = ''
if pending:
logical_lines.append(pending)
return logical_lines
def posix_continuation(join_):
logical_lines = []
pending = ''
for line in join_.split('\n'):
continued = line.endswith('\\')
chunk = line[:-1] if continued else line
pending += (' ' if pending else '') + chunk.strip()
if not continued:
logical_lines.append(pending)
pending = ''
if pending:
logical_lines.append(pending)
return logical_lines
cases = {
'immediate_backslash_newline': 'foo\\\nbar',
'backslash_whitespace_before_newline': 'foo \\ \nbar',
'quoted_backslash_before_newline': "'foo\\\nbar'",
'single_line_no_continuation': 'report="$(find . | sort)"',
'realistic_spacing_without_real_continuation': 'report="$(find \\\n".")"',
}
for name, case in cases.items():
print(f'{name}:')
print(' current:', current(case))
print(' current_regex:', bool(re.search(r'\\\s*$', case.split("\n")[0])))
print(' line_reconstructed_for_scan:', current(case)[0]) if current(case) else None
print(' shell_continues:', case.split('\n')[0].endswith('\\'))
print(' shell_logical_line:', posix_continuation(case)[0])
PYRepository: CodesWhat/drydock
Length of output: 1118
🌐 Web query:
Bash quoted backslash-newline continuation behavior bash manual
💡 Result:
In Bash, the behavior of a backslash-newline pair depends on whether the backslash is quoted. 1. Unquoted backslash-newline: When a backslash is not quoted, a backslash-newline pair is treated as a line continuation [1][2][3]. This causes the backslash and the newline character to be removed from the input stream and effectively ignored, allowing a single logical line to span multiple physical lines [1][2][3]. 2. Double-quoted backslash-newline: Within double quotes, the backslash retains its special meaning only when followed by specific characters, one of which is a newline [1][3]. Consequently, a backslash followed by a newline inside double quotes is also treated as a line continuation and removed [1][3]. 3. Single-quoted backslash-newline: Inside single quotes, all characters—including backslashes and newlines—are treated literally [1]. Therefore, a backslash-newline pair inside single quotes does not function as a line continuation; the backslash is preserved and the newline remains as part of the string [1]. 4. Dollar-single-quoted ($'string') backslash-newline: Within this quoting mechanism, backslash escape sequences are interpreted according to ANSI C standards [1][3]. A backslash followed by a newline in this context is typically used to represent escape sequences or literal characters, but it does not function as a standard shell line continuation mechanism [1][3].
Citations:
- 1: https://man7.org/linux/man-pages/man1/bash.1.html
- 2: https://www.gnu.org.cach3.com/software/bash/manual/html_node/Escape-Character.html
- 3: https://ss64.com/bash/syntax-quoting.html
🌐 Web query:
POSIX shell double quoted backslash newline continuation inside double quotes
💡 Result:
According to the POSIX Shell Command Language standard, the behavior of a backslash followed by a newline character (a "line continuation") within double quotes depends on whether that backslash is acting as an escape character [1][2]. Within double quotes, the shell interprets a backslash as an escape character only when it is followed by one of the following special characters: $, `, ", , or a newline [3][4]. When a backslash is immediately followed by a newline character inside double quotes: 1. The shell interprets this sequence as a line continuation [1][2]. 2. The backslash and the newline character are removed entirely from the input [1][2]. 3. Because the characters are removed and not replaced by whitespace, the sequence does not act as a token separator [1][2]. This mechanism effectively allows a single logical line to be split across multiple physical lines in the source code while being treated as a continuous string by the shell [1][2]. It is important to note that if you wish to include a literal backslash followed by a newline in your output (i.e., you do not want the shell to consume them), you must escape the backslash itself (e.g., \ followed by a newline). In this case, the first backslash escapes the second, preventing it from being interpreted as an escape character for the newline, thereby preserving both the backslash and the newline in the resulting string [1][2].
Citations:
- 1: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html
- 2: https://pubs.opengroup.org/onlinepubs/9699919799.2018edition/utilities/V3_chap02.html
- 3: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3%5Fchap02.html
- 4: https://pubs.opengroup.org/onlinepubs/9799919799/utilities/V3%5Fchap02.html
Preserve real pipeline boundary semantics.
Backslash-newline is a single valid continuation; trailing whitespace after the backslash is not handled by the shell, and shell continuation removes the line break, it does not replace it with a space. This static scan should only join the actual shell-continued command so it does not create fake find pipelines.
function joinContinuations(run: string): string[] {
const logicalLines: string[] = [];
let pending = '';
for (const rawLine of run.split('\n')) {
const continued = rawLine.endsWith('\\') && !rawLine.endsWith('\\\\');
const chunk = continued ? rawLine.slice(0, -1) : rawLine;
pending += (pending ? ' ' : '') + chunk.trim();
if (!continued) {
logicalLines.push(pending);
pending = '';
}
}
if (pending) logicalLines.push(pending);
return logicalLines;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/tests/ci-verify-find-pipefail-guard.test.ts around lines 51 - 63,
The joinContinuations function incorrectly treats backslashes followed by
whitespace as shell continuations and inserts a space where shell continuation
removes the newline. Restrict continuation detection to a single backslash that
is the final character of rawLine, remove only that backslash, and preserve the
existing logical-line flushing behavior for non-continued lines and trailing
pending content.
Routine dev → main sync, per the branch model: main only advances by merging from `dev/v1.6`, never by taking independent commits. Opened from a throwaway `sync/main-20260726` branch rather than from `dev/v1.6` directly. Last time (#596) the head was `dev/v1.6` itself, and auto-delete-head-branches deleted it on merge — which is what broke the Crowdin sync, since its base resolver then found no `dev/v*` on origin. ## What this carries 48 files. Two things worth calling out: **Clears the CVE failing main's scheduled scan.** `brace-expansion` goes 5.0.7 → 5.0.8 in `app/`, `ui/`, and `e2e/` lockfiles, fixing CVE-2026-14257 / GHSA-mh99-v99m-4gvg (medium, DoS via unbounded expansion). 5.0.8 is the fixed version per OSV. This is the sole cause of the "🔬 CI Verify — main" failures on the twice-weekly schedule; the `--all` qlty gate flags it in all three lockfiles. No code change was needed, dev already had the bump. **The pipefail dead-fallback fixes (#599).** The Crowdin base-branch resolver and 8 load-test `find` pipelines in `ci-verify.yml`, where a command exiting non-zero on an empty result aborted the step before its explicit fallback could run. Both covered by new tests in `.github/tests/`. Plus the #587 maturity-cleared notification feature and its store/trigger plumbing. ## Notes - `renovate.json` still targets `dev/v1.6`, which matches the active dev branch, so the drift guard's bot-target assertion passes. - Commit-count comparisons between these branches are meaningless here — the repo is squash-merge-only, so dev's commits never become ancestors of main. Tree equality is the real invariant, and that's what `release-cut.yml` asserts. - No release tag is being cut by this PR. This is the routine sync that keeps main current. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ✨ Added - Maturity gate sweep scheduler with configurable cron execution. - `maturityGatePendingSince` lifecycle tracking across containers and persistence. - `maturity-cleared` events, audit entries, notification rules, templates, deduplication, UI support, and documentation. - Extensive unit and workflow tests. 🔧 Changed - Controller and watcher startup/shutdown now manage the maturity scheduler. - Container processing checks for cleared maturity gates before emitting reports. - Crowdin branch resolution and load-test report discovery now tolerate empty `find`/branch results. - `brace-expansion` upgraded to `5.0.8` across lockfiles. 🔒 Security - Dependency update addresses CVE-2026-14257 / GHSA-mh99-v99m-4gvg. - Verify the synchronized tree matches `dev/v1.6` exactly. - Confirm `renovate.json` continues targeting `dev/v1.6`. - Ensure no release tag is created by this synchronization. - Validate maturity-cleared notification behavior across watch cycles, scheduled sweeps, deduplication, and failed deliveries. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Crowdin Bot <support+bot@crowdin.com>
1 participant
The Crowdin sync failed on main on 2026-07-24 with a bare
exit 1and no message. Root cause turned out to be a bug class rather than a one-off, so this fixes both instances of it and adds coverage.The pattern
Inside a
set -e/pipefailregion, a command that exits non-zero on an empty result aborts the step before an explicitly written fallback can run. The fallback is dead code. The tell: someone wrote handling for the empty case, and the empty case can't reach it.1. Crowdin base-branch resolution
i18n-crowdin.ymlresolves the highestdev/vX.Yon origin and falls back to the default branch when there is none, so translations always have somewhere to land between a GA and the next dev branch being cut. Underset -euo pipefaila no-matchgrepexits 1, which fails the pipeline, which fails the assignment, which aborts the step. Theif [ -z ]fallback below it could never run.It fired for real: merging sync PR #596 briefly deleted
dev/v1.6via auto-delete-head-branches, the push-to-main sync run found nodev/v*, and the job died.Guards the grep only, so a failing
ls-remote(network, auth) still propagates and fails loudly instead of silently retargeting translations at main. That distinction matters more than the fix itself.2. Load-test report resolution
Sweeping the other workflows for the same pattern turned up 8 more sites in
ci-verify.yml:findexits 0 on a directory that exists but is empty — the case the downstream scripts handle. It exits 1 when the directory is missing, and GitHub runs these steps under the defaultbash -eo pipefail, so that aborts the step.summarize-load-test-report.shandcheck-load-test-correctness.shboth open with anif [ -z "${REPORT}" ]block written for exactly this, and neither gets to run.Reachable, not theoretical: these steps are
if: always(), andrun-load-test.shonly doesmkdir -pon the artifact dir at line 168 — after the docker build, compose up, and the health-check wait that exits 1 at line 136. A load test that fails the way load tests usually fail leaves the directory absent, and the step meant to explain that dies with a bare exit 1.Coverage
crowdin-base-branch-resolver.test.tsexecutes the realid: baserun block extracted from the workflow against a stubbed git, rather than asserting on workflow text, so it tracks the actual logic instead of a copy that can drift. Three cases: no dev branches falls back and exits 0; several picksdev/v1.10overv1.9(lexical sort gets this wrong); failingls-remotestill exits non-zero. That last one guards against "fixing" the bug by swallowing all errors, which would be worse than the bug.Verified it fails against the pre-fix snippet with
expected 1 to be +0, and that the other two still pass — the bug only affects the one path.ci-verify-find-pipefail-guard.test.tsdiscovers the find sites by walking the parsed workflow rather than pinning line numbers, so it also catches a new unguardedfindadded later. Carries a sanity assertion so it can't pass vacuously if the shape stops matching. Verified it catches a revert by naming the exact offending step.Executing the load-test steps for real would need docker plus Artillery artifacts, so that one is a static assertion by design — noted in a comment in the file.
Notes
mainis currently failing its scheduled CI Verify onbrace-expansion@5.0.7(CVE-2026-14257). That is already fixed ondev/v1.6at 5.0.8 — it just needs the next dev → main sync, no code change.l10n_crowdin; it was cut before 🔧 chore(i18n): sync translations from Crowdin #545 landed and had gone conflicting. It regenerates from the dev tip on the next push here.Changelog
DEFAULT_BRANCHunderset -euo pipefail, whilegit ls-remotefailures still propagate.find artifacts/load-test/<site>/...*.json ...command-substitution pipeline so missing artifact directories don’t abort later empty-report handling underpipefail.test/cpu-bench.shto guard the per-containergrep "^$c "used to compute stats so missing samples don’t stop the script underset -euo pipefail.dev/vX.Yselection, andls-remoteerror propagation.findguard (including later pipeline stages).Concerns
find-into-variable command-substitution assignments and doesn’t miss new sites.