testsAndMisc/docs/shell-split-verification.md
Krzysztof kuhy Rudnicki 76e73830f6 shell: split clean_audio, enforce_vbox_hosts and migrate_hosts_guard
clean_audio.sh          419 -> 230 + 204
  enforce_vbox_hosts.sh   443 -> 220 + 236
  migrate_hosts_guard.sh  444 -> 167 + 162 + 141

migrate_hosts_guard needed two libraries: with all ten work functions moved
the single lib was 295 lines, so migrate and rollback were separated.

The extractor's brace matcher required index > start and so silently skipped
one-line `warn() { ...; }` definitions. Fixed, and regression-checked against
the committed fresh-install/main.sh split byte-for-byte.

The harness earned its keep on a fourth file: adding `export` to libre_translate
to satisfy SC2034 changed two function bodies, and verify_shell_split.sh caught
what lint had accepted. Those exports were reverted, and libre_translate.sh is
parked - what remains after its clean seams both assigns and reads the same
globals, so getting it under the cap needs parse_args restructured rather than
relocated, which is a refactor rather than a verbatim move.

That cost is now written down: a lib must contain the readers of every global
it assigns. The precedent at install_pacman_wrapper.sh:29 is about
definitions-only libs, not about assignments as such, and the pre-commit hook
runs shellcheck with no -x, so a source= directive will not make it follow the
link. This is the shell analogue of the monkeypatch-target trap.

Verified: all 8, 13 and 19 functions verbatim; all four libraries source with
no side effects; shellcheck passes standalone on every file with no
suppression; bash -n passes on all seven. None of the three was executed -
they drive ffmpeg over real media, mutate VirtualBox guest disks, and rewrite
systemd units.

Over-cap files 29 -> 26.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011w2Pj4Ek3ZNMQVYcFHgXDY
2026-08-17 14:27:57 +02:00

167 lines
8.6 KiB
Markdown

# Shell split verification: how a green split is still broken
Split out of `docs/shell-split-recipes.md` when that file hit the same 250-line
cap it exists to serve. That file covers **how to make** a split; this one
covers **how to find out it is wrong**. Read both before the next
`linux_configuration` or `phone_focus_mode` split.
Every section below cost a failed run, a revert, or — once — a live system
mutation.
### A `--skip-install` flag is not evidence the script is safe to run
`shell_check.sh` gates only `install_if_missing`'s pacman branch on
`SKIP_INSTALL`. The AUR branch inside `install_linters` runs regardless, so a
backgrounded "baseline" run installed four packages before it was killed
mid-`makepkg`. Decision 6 was violated by a flag that looked like permission.
Before running any script to capture a baseline, grep the flag's variable and
confirm it gates **every** mutation path, not just the first one:
```bash
grep -n 'SKIP_INSTALL\|DRY_RUN' <script> # then read each hit
```
`--list-only` was the genuinely safe path here, because it returns before
`install_linters` is reached at all.
### Check whether the target is DEPLOYED as a single file — the trace cannot
Before designing seams, find out whether some other script copies the target
somewhere and runs it from there. An entry+`lib/` shape ships an entry whose
deployed `SCRIPT_DIR` has no `lib/`, so it dies on its first `source` under
`set -e` — while your trace stays green, because the trace runs the repo copy
where `lib/` exists.
```bash
grep -rn '<target-basename>' linux_configuration/ --include='*.sh' \
| grep -v '<the target itself>:'
```
Confirmed blocked on this, not theoretically: `install_leechblock.sh` (the
pacman installer `cp`s it to `/usr/local/share/digital_wellbeing/` and
`pacman_wrapper.sh:831` prefers that copy on **every pacman invocation**) and
`block_compulsive_opening.sh` (`install_all` copies itself to
`/usr/local/bin`). A target invoked as `$CONFIG_DIR/scripts/...` is fine.
### A stubbed run cannot reach a bug in a seam that passes state
`meta/scripts/verify_shell_split.sh` and the hand-rolled `declare -F` probe
both **source the libs and never call the functions**. That is enough to prove
every `source` line resolves, and nothing more. The `analyze_repo.sh` split
passed `bash -n`, `shellcheck`, a function-set diff, a line-set diff and the
stubbed probe — and still shipped a script that aborted immediately after
language detection. Two separate bugs, neither reachable without executing:
- **`set -e` and the function tail.** Eight `((LANG_FILES[x] > 0)) && HAS_Y=true`
lines were top-level statements, where a false `&&` merely leaves `$?`
non-zero. Wrapped into a function they become its _return value_, and the
last one (java, false for most repos) killed the script. Any time you wrap
former top-level code in a function, check what its **last statement** is;
append `|| true` if it is a bare conditional.
- **A self-referencing nameref.** `local -n LANG_CODE_FILES="$1"` pointing at a
global of the same name still moves data on bash 5.3, but warns
`circular name reference` on _every access_ — straight into the script's
output. Name the local something else (`_code_files`).
So: when a seam passes state, run the thing. Stub only the parts that mutate
the system, and diff the generated artifacts against a detached worktree at the
pre-split commit. That comparison is what proved the fix — same file set, every
text artifact byte-identical.
## The harness and the fixtures
Everything about _running_ the check — `trace_shell_split.sh`, the `--prefix`
mode for scripts whose job is placing files, the regression fixtures, the
entry-point and `shfmt` traps — moved to
**`docs/shell-split-harness.md`** when this file hit the same 250-line cap it
exists to serve. This file stays the _why_; that one is the _how_.
### Prove the move was verbatim: `meta/scripts/verify_shell_split.sh`
The Python splits in this repo are verified with a throwaway `ast` identity
check. There is no `ast` for bash, so the shell equivalent normalises through
`shfmt -mn` (minify: strips comments and indentation, keeps logic) and compares
a hash per top-level function:
```bash
bash meta/scripts/verify_shell_split.sh HEAD <old-path> <new-path>...
# IDENTICAL: all N top-level functions moved verbatim
```
It reports which function changed, so a diff points at the seam rather than the
whole file. Run it after every split, and again after `shfmt`/pre-commit
autofixes — reordering an import or reflowing a line is exactly what it is
designed to see through.
**Verify the harness before trusting it.** A normalizer that silently passes
everything is worse than none. The three checks it must survive:
| input | expected |
| ------------------------------------------------ | --------------------------------------- |
| unchanged copy of the file | `IDENTICAL` |
| comments/blank lines/indentation changed | `IDENTICAL` |
| one character of logic changed (`"$1"` → `"$2"`) | `DIFFERENCE FOUND`, naming the function |
The first attempt at that mutation test used `sed 's/exit 1/exit 2/'` on a file
containing no `exit 1`. It reported `IDENTICAL` — not because the harness was
broken, but because the mutation never applied. Confirm the mutation actually
changed the file (`diff` it) before concluding anything about the harness.
### Deployment triage of the over-cap scripts (2026-08-17)
Run before designing any seam, per the section above. `deploy.sh` copies a
**hardcoded per-file list** into `/data/local/tmp/focus_mode` and never deploys
`lib/`:
- **`phone_focus_mode/*` (8 over-cap scripts)** — deployed individually.
A new sibling must be added to `deploy.sh`'s copy list **in the same commit**,
or focus mode dies on the phone at its first `source`. These scripts source
only `config.sh`, which is on that list, so a sibling-file seam is workable —
an entry+`lib/` seam is not.
- **`install_leechblock.sh`, `block_compulsive_opening.sh`** — copied to
`/usr/local/…` and preferred there by `pacman_wrapper.sh:831` on every pacman
invocation. Teach the installer to deploy the directory first, in its own
commit. Highest blast radius; do these last.
- **`hosts/install.sh`, `setup_hosts_guard.sh`, `lib/monitor.sh`** — install
helper scripts to `/usr/local/sbin` or `/usr/local/bin`; check whether the
file being split is itself the deployed one.
- **The other 16** (`fresh-install/main.sh`, `nvidia_troubleshoot.sh`,
`setup_passwordless_system.sh`, `clean_audio.sh`, `enforce_vbox_hosts.sh`,
`migrate_hosts_guard_to_guard_lib.sh`, `libre_translate.sh`,
`diagnose_pacman_hook_stall.sh`, `install_plagiarism_tools.sh`,
`steam_compatibility.sh`, `deploy.sh`, `setup_night_lockdown.sh`,
`pacman_wrapper.sh`, `generate_study_materials.sh`,
`check_and_enable_services.sh`, `setup_midnight_shutdown.sh`) — no deployed
copy found, so an entry+`lib/` shape is safe.
### A lib must contain the readers of every global it assigns
`install_pacman_wrapper.sh:29` records the constraint: a **definitions-only**
lib "assigns without referencing, which is SC2034, and the repo forbids
suppressions." The rule that follows from it is not "assignments stay in the
entry script" — it is that a file must not assign a variable it never reads.
A lib holding both the writer and its readers is clean.
Getting this inverted is easy and costs several revert cycles.
`libre_translate.sh` was split with `parse_args` (the writer) left in the entry
and `health_check` / `sample_request` / `start_container_ephemeral` (the
readers) moved to the lib — the worst of both, and SC2034 fired on nine
globals. The fix is to colocate: move the writer **and** its consumers into the
same file.
The pre-commit hook is `shellcheck` with **no `-x`**, so each file is analysed
standalone and a `# shellcheck source=` directive will not make the checker
follow the link. Verify by running `shellcheck <lib>` on its own before
committing; the hook will not be any more forgiving.
This is the shell analogue of the `monkeypatch.setattr` target trap: the
question is always which file's namespace owns the name.
**`libre_translate.sh` (488) is parked.** What remains after its clean seams
are taken is `parse_args` (111 lines), `write_env_file`, `detect_container_user`
and `main`, which between them assign and read the same configuration globals.
Getting the entry under the cap needs `parse_args` restructured rather than
relocated — a refactor, not a verbatim move, so it is out of scope for this
method. Attempt it with a test harness, or accept an entry that stays over.