Skip to content

Commit 3676e24

Browse files
castrojoJorge CastroCopilot
authored
fix(bling): rearm bash-preexec DEBUG trap on each prompt (projectbluefin#943)
## Root cause — confirmed against bash-preexec 0.6.0 source Verified by reading the tagged 0.6.0 source directly, not from memory: - L74: the installer is deferred by appending a string to `PROMPT_COMMAND`: `__bp_trap_string="$(trap -p DEBUG)"; trap - DEBUG; __bp_install` - L289-293: `__bp_install()` **returns early** if `"${PROMPT_COMMAND[*]}"` already contains `__bp_precmd_invoke_cmd` - The self-removal code reads `existing_prompt_command="${PROMPT_COMMAND:-}"` and later assigns `PROMPT_COMMAND='__bp_precmd_invoke_cmd'` On Fedora, `PROMPT_COMMAND` is an **array** (bash ≥ 5.1). `${PROMPT_COMMAND}` and bare assignment address **element `[0]` only**. So once any other hook (mise, direnv, starship, vte, systemd, zoxide) pushes the installer out of element `[0]`, it is **never removed** — and runs on every prompt. Its first act is `trap - DEBUG`; `__bp_install` then returns early. From prompt 2 onward the DEBUG trap is permanently empty. That is exactly the reporter's `declare -p` output: `__bp_precmd_invoke_cmd` in `[0]`, installer stranded in `[2]`, atuin's hook absent. Reproduced empirically in a real prompt loop: `cycle1 trap: [trap -- '__bp_preexec_invoke_exec "$_"' DEBUG]`, `cycle2: []`, `cycle3: []`. With this fix, all three cycles retain the trap. Matches upstream rcaloras/bash-preexec#188 (fixed upstream but insufficient alone) and #186 (still open). ## Non-obvious discovery Bash does not inherit the DEBUG trap into functions without `functrace`: `trap -p DEBUG` **inside a function always returns empty**, and `trap - DEBUG` inside a function is a no-op — but `trap ... DEBUG` inside a function *does* set it globally. Two consequences: a detector-guard is impossible (hence the unconditional re-arm), and it invalidated the first simulation, which passed **vacuously**. Written up in `docs/skills/shell-scripts.md`. ## Changes - New `system_files/shared/.../bling/bash-preexec-rearm.sh` — re-arms the exact trap bash-preexec installs, once per prompt - `bling.sh` sources it **last**, under the bash guard, so the hook is the final `PROMPT_COMMAND` entry `bling.sh` stays POSIX (it is under an sh-dialect shellcheck gate); the bash-only logic lives in the sibling file. `BLING_DIR` allows bats to point at the repo tree. No vendored copy of bash-preexec. ## Tests — 14 cases, all pass Tests **1 and 4 are negative controls** that pin the bash-preexec stand-in to the real defect, so the positive cases cannot pass vacuously. Covered: array `PROMPT_COMMAND` trap survival, atuin-style hook still firing, idempotency on double-source, scalar `PROMPT_COMMAND` (no regression), bash-preexec absent, readonly `PROMPT_COMMAND`, missing helper, zsh source-safety. Also green: `bash -n` / `sh -n`, both CI shellcheck invocations, `just check`, `pre-commit run --all-files`. ## Confidence and limits High that the DEBUG trap now survives — the failure and its repair are both directly reproduced. Two things only real hardware can prove: 1. atuin actually writing records end-to-end (tests use a preexec stub, not atuin 18.18.1) 2. that no downstream hook on a real session appends its own `trap - DEBUG` **after** our entry Ordering is safe for everything bling itself initializes. ## Reviewer note The re-install is **unconditional** per prompt, forced by the functrace scoping above; it matches the reporter's validated workaround. A third party's own raw DEBUG trap set after bling would be overwritten each prompt — but such a trap already breaks bash-preexec today. ## Blast radius — highest in this batch `bling.sh` is sourced by **every interactive bash shell** on bluefin, bluefin-lts, and dakota. Recommend lab validation before merge. Closes projectbluefin#869 --- ## Branch state Updated onto `main` after projectbluefin#926 landed (via `gh pr update-branch`). No conflict — this PR's `docs/TESTING.md` row and `Justfile` entry merged cleanly. ## Overlap with the sibling PRs in this batch **projectbluefin#942** also edits `docs/skills/shell-scripts.md`, but adds different sections (this PR: `Bash DEBUG traps are invisible inside functions`, `POSIX-sh files cannot hold bash array code`; projectbluefin#942: `image-info.json is build-time state`). Verified non-conflicting with a real sequential merge. No file overlap with projectbluefin#941. ## Pre-merge checklist for the reviewer - [ ] Lab-validate on real hardware — the tests use a preexec stub, not atuin 18.18.1, so end-to-end recording is unproven - [ ] Confirm the unconditional per-prompt trap re-install is acceptable (see reviewer note above; a detector-guard is impossible due to bash's functrace scoping) Co-authored-by: Jorge Castro <jorge@projectbluefin.io> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 2281f4f commit 3676e24

7 files changed

Lines changed: 419 additions & 0 deletions

File tree

Justfile

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ test:
99
bats tests/test_privileged_setup.bats
1010
bats tests/test_bling.bats
1111
bats tests/test_bling_sh.bats
12+
bats tests/test_bling_preexec_rearm.bats
1213
bats tests/test_luks_tpm2.bats
1314
bats tests/test_rechunker_group_fix.bats
1415
bats tests/test_bling_fastfetch.bats

docs/TESTING.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ Do not add exemptions for scripts with branching logic.
8989
| `tests/test_setup_scripts.bats` | `ublue-system-setup`, `ublue-user-setup`, `hookrunner.sh` — shared hook dispatcher + thin-wrapper guard |
9090
| `tests/test_privileged_setup.bats` | `ublue-privileged-setup` — privileged hook runner logic |
9191
| `tests/test_bling.bats` | `ublue-bling` — shell config injection install/uninstall |
92+
| `tests/test_bling_preexec_rearm.bats` | `bling/bash-preexec-rearm.sh` — DEBUG trap re-arm with array/scalar `PROMPT_COMMAND`, idempotency, degradation when bash-preexec is absent |
9293
| `tests/test_luks_tpm2.bats` | `luks-tpm2-autounlock` — UUID parsing, device resolution, cryptenroll flag construction |
9394
| `tests/test_rechunker_group_fix.bats` | `rechunker-group-fix` — group/gshadow append, duplicate detection, format |
9495
| `tests/test_bling_fastfetch.bats` | `ublue-bling-fastfetch` — all 9 accent colors, dconf/gsettings fallback chain, FASTFETCH_FORCE_THEME override |

docs/skills/shell-scripts/references/bats-patterns.md

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,3 +103,29 @@ setup() {
103103
unset UWELCOME_SHOWN
104104
}
105105
```
106+
107+
## Bash DEBUG traps are invisible inside functions
108+
109+
Without `set -o functrace`, bash does **not** inherit the `DEBUG` trap into
110+
shell functions. Two consequences bite when testing or writing prompt hooks:
111+
112+
```bash
113+
f() { echo "[$(trap -p DEBUG)]"; } # always prints [] — even when a trap is set
114+
g() { trap - DEBUG; } # does NOT clear the caller's DEBUG trap
115+
h() { trap 'cmd' DEBUG; } # DOES set the caller's DEBUG trap
116+
```
117+
118+
So `trap -p DEBUG` is useless as a detector from inside a function, while
119+
`trap ... DEBUG` from inside a function is a reliable way to (re-)install one.
120+
121+
For bats: `PROMPT_COMMAND` entries execute at **top level** in a real shell.
122+
Simulate a prompt cycle with a top-level loop, never a helper function —
123+
wrapping the cycle in a function hides `trap - DEBUG` clobbers entirely and
124+
makes the test pass vacuously.
125+
126+
```bash
127+
CYCLE='for __e in "${PROMPT_COMMAND[@]}"; do eval "$__e"; done'
128+
```
129+
130+
See `tests/test_bling_preexec_rearm.bats` and
131+
[#869](https://github.com/projectbluefin/common/issues/869).

docs/skills/shell-scripts/references/shellcheck-examples.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,3 +43,12 @@ for script in "${HOOKS_DIR}"/* ; do
4343
done
4444
```
4545
A space in `HOOKS_DIR` will silently fail to find hooks if only `$script` is fixed.
46+
47+
## POSIX-`sh` files cannot hold bash array code
48+
49+
`system_files/**/*.sh` is shellchecked with the dialect from its shebang.
50+
`bling.sh` is `#!/usr/bin/env sh`, so bash arrays, `BASH_SOURCE`, and `+=(...)`
51+
trip SC3028/SC3030/SC3054 and fail CI. Put bash-only logic in a sibling
52+
`#!/usr/bin/env bash` file and source it from inside the existing
53+
`[ "${BLING_SHELL}" = "bash" ]` guard, with a `BLING_DIR` override so bats can
54+
point at the repo tree instead of `/usr/share/ublue-os/bling`.
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
#!/usr/bin/env bash
2+
# Re-arm the bash-preexec DEBUG trap on every prompt.
3+
#
4+
# Fedora (bash >= 5.1) exposes PROMPT_COMMAND as an *array*. bash-preexec 0.6.0
5+
# defers its own installation by appending a string to PROMPT_COMMAND:
6+
#
7+
# __bp_trap_string="$(trap -p DEBUG)"; trap - DEBUG; __bp_install
8+
#
9+
# __bp_install is supposed to delete that string again, but it only ever reads
10+
# and rewrites "${PROMPT_COMMAND}" — which expands to element [0] alone. Once
11+
# another hook (direnv, starship, mise, zoxide, vte, systemd) has pushed the
12+
# installer into a later array element, it is never removed and therefore runs
13+
# on *every* prompt. Its first act is `trap - DEBUG`, and __bp_install then
14+
# returns early because PROMPT_COMMAND already contains __bp_precmd_invoke_cmd.
15+
# From the second prompt onward the DEBUG trap is permanently empty, so every
16+
# preexec consumer silently stops firing — atuin loads and CTRL+R works, but no
17+
# command is ever recorded.
18+
#
19+
# Re-arming the trap at the end of each prompt cycle restores the invariant
20+
# bash-preexec assumes without patching or vendoring bash-preexec itself.
21+
#
22+
# See: https://github.com/projectbluefin/common/issues/869
23+
# https://github.com/rcaloras/bash-preexec/issues/188
24+
# https://github.com/rcaloras/bash-preexec/issues/186
25+
26+
# Only meaningful when bash-preexec is actually loaded, and only safe when we
27+
# are allowed to write PROMPT_COMMAND (bash-preexec bails out in that case too).
28+
if [[ "$(type -t __bp_preexec_invoke_exec)" == "function" ]] &&
29+
(unset PROMPT_COMMAND) 2>/dev/null; then
30+
31+
# Re-install the exact trap bash-preexec installs in __bp_install. A prior,
32+
# non-bash-preexec DEBUG trap is not lost: bash-preexec preserves it as
33+
# __bp_original_debug_trap inside preexec_functions.
34+
__bling_rearm_bp_debug_trap() {
35+
trap '__bp_preexec_invoke_exec "$_"' DEBUG
36+
}
37+
38+
# Idempotent — sourcing bling.sh twice must not queue the hook twice.
39+
if [[ "${PROMPT_COMMAND[*]-}" != *__bling_rearm_bp_debug_trap* ]]; then
40+
if ((BASH_VERSINFO[0] > 5 || (BASH_VERSINFO[0] == 5 && BASH_VERSINFO[1] >= 1))); then
41+
PROMPT_COMMAND+=('__bling_rearm_bp_debug_trap')
42+
elif [[ -n "${PROMPT_COMMAND:-}" ]]; then
43+
PROMPT_COMMAND="${PROMPT_COMMAND}"$'\n'"__bling_rearm_bp_debug_trap"
44+
else
45+
PROMPT_COMMAND="__bling_rearm_bp_debug_trap"
46+
fi
47+
fi
48+
fi

system_files/shared/usr/share/ublue-os/bling/bling.sh

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,3 +67,13 @@ if command -v mise >/dev/null 2>&1; then
6767
fi
6868
fi
6969
fi
70+
71+
# Keep bash-preexec's DEBUG trap alive. Must stay last so the re-arm hook is the
72+
# final PROMPT_COMMAND entry, after every hook above has queued its own.
73+
# See: https://github.com/projectbluefin/common/issues/869
74+
if [ "${BLING_SHELL}" = "bash" ]; then
75+
BLING_REARM="${BLING_DIR:-/usr/share/ublue-os/bling}/bash-preexec-rearm.sh"
76+
# shellcheck source=/dev/null
77+
[ -f "${BLING_REARM}" ] && . "${BLING_REARM}"
78+
unset BLING_REARM
79+
fi

0 commit comments

Comments
 (0)