diff --git a/.claude/rules/architecture-map.md b/.claude/rules/architecture-map.md index a5b12acf..6e9e6cdf 100644 --- a/.claude/rules/architecture-map.md +++ b/.claude/rules/architecture-map.md @@ -48,7 +48,7 @@ shell (or, in parallel, in per-test `.result` files aggregated at the end). | Module | Owns | |--------|------| | `bashunit` + `main.sh` | entry, subcommand routing, flag parsing, run lifecycle, exit codes, cleanup calls | -| `runner.sh` | aggregator only — sources the `src/runner/` module below | +| `runner/index.sh` | aggregator only — sources the `src/runner/` module below | | `runner/context.sh` | workdir restore, test identity/location exports, title interpolation, capability probes | | `runner/payload.sh` | the `_BASHUNIT_RUNNER_*_OUT` return slots; encode/decode of the per-test result payload | | `runner/diagnostics.sh` | runtime-error detection, kill-signal classification, profiling, verbose/file headers | @@ -68,7 +68,7 @@ shell (or, in parallel, in per-test `.result` files aggregated at the end). | `clock.sh` | time impl selection (EPOCHREALTIME > date > perl > …), return-slot reads | | `str.sh` / `math.sh` / `io.sh` / `globals.sh` | pure-bash utilities; `globals.sh` has `temp_file`/`temp_dir` (public test API) | | `test_doubles.sh` | spy/mock state via `_BASHUNIT_SPY_*` globals + files | -| `coverage.sh` | aggregator only — sources the `src/coverage/` modules below | +| `coverage/index.sh` | aggregator only — sources the `src/coverage/` modules below | | `coverage/config.sh` | data-file locations, tracked-file roots, engine selection (`init` resets state owned by several modules) | | `coverage/paths.sh` | `normalize_path`, `should_track` and the hot-path track/path caches | | `coverage/engine.sh` | DEBUG-trap and xtrace capture, buffering, `finalize`/`cleanup`, parallel merge; only active under `--coverage` | diff --git a/.claude/rules/bash-style.md b/.claude/rules/bash-style.md index 60fd8272..0944a97c 100644 --- a/.claude/rules/bash-style.md +++ b/.claude/rules/bash-style.md @@ -120,7 +120,7 @@ local thing=$_BASHUNIT_PKG_THING_OUT Examples in tree: `src/runner/payload.sh` (`_BASHUNIT_RUNNER_FIELD_OUT`, `_BASHUNIT_RUNNER_TOTAL_OUT`, `_BASHUNIT_RUNNER_TYPE_OUT`, `_BASHUNIT_RUNNER_OUTPUT_OUT`), -`src/coverage.sh` (`_BASHUNIT_BRANCH_ARMS_OUT`). +`src/coverage/branches.sh` (`_BASHUNIT_BRANCH_ARMS_OUT`). ### When the helper builds dynamic variable names (mock/spy state) @@ -158,10 +158,11 @@ function bashunit::pkg::do_thing() { ### Intentional dynamic-scope mutation is a separate pattern -The coverage branch helpers in `src/coverage.sh` (`_branch_push_if` and friends) deliberately -mutate caller locals (`if_decision_line`, `if_arms`, `if_depth`, `if_arm_start`). That is -documented inline at `src/coverage.sh:818-821` and is **not** the outvar pattern — the -helper has no `$1`-named outvar argument; the caller agrees to share state by convention. +The coverage branch helpers in `src/coverage/branches.sh` (`_branch_push_if` and friends) +deliberately mutate caller locals (`if_decision_line`, `if_arms`, `if_depth`, `if_arm_start`). +That is documented inline above the helpers in that file and is **not** the outvar pattern — +the helper has no `$1`-named outvar argument; the caller agrees to share state by convention. +It is also why those helpers and `extract_branches` must stay in one file (ADR-010). Don't introduce new instances of this pattern without an inline justification comment. ## ShellCheck diff --git a/CHANGELOG.md b/CHANGELOG.md index 476ef1cd..9c32d200 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,7 @@ ## Unreleased ### Changed -- Internal: `src/runner.sh` is split into a `src/runner/` module of ten single-responsibility files behind a `source`-only aggregator. A pure relocation, no behavior change; see [ADR-010](adrs/adr-010-src-module-directories.md) (#924) +- Internal: `src/runner.sh` and `src/coverage.sh` are split into `src/runner/` and `src/coverage/` modules of single-responsibility files, each behind a `source`-only `index.sh` aggregator. A pure relocation, no behavior change; see [ADR-010](adrs/adr-010-src-module-directories.md) (#924, #925) ### Fixed - `build.sh` dedupes embedded files by repo-relative path. The previous basename key compared the top-level loop's relative paths against the recursion's absolute ones, so a file reached from two places could be bundled twice in the released binary; it also collided for same-named files in different directories (#923) diff --git a/adrs/adr-010-src-module-directories.md b/adrs/adr-010-src-module-directories.md index d359b87f..76d70eb7 100644 --- a/adrs/adr-010-src-module-directories.md +++ b/adrs/adr-010-src-module-directories.md @@ -1,11 +1,17 @@ # Splitting large `src/` files into module directories -* Status: accepted +* Status: accepted, amended 2026-08-01 * Deciders: Chemaclass * Date: 2026-07-30 Technical Story: https://github.com/TypedDevs/bashunit/issues/924 +> **Amendment, 2026-08-01.** The aggregator moves *inside* the module directory +> as `index.sh`, replacing the original `src/.sh` beside it. The +> reasoning that first chose "beside" was wrong, and one of the two arguments +> that overturn it is a correctness bug rather than a preference — see +> [Aggregator placement](#aggregator-placement-amended-2026-08-01). + ## Context and Problem Statement `src/runner.sh` had grown to 2145 lines and 57 functions covering five unrelated @@ -36,24 +42,60 @@ directory without changing what the released single-file binary does? ## Decision Outcome -Chosen option: **`src/runner/` directory behind a thin aggregator beside it**. - -`src/runner.sh` keeps its single `source` line in the `bashunit` entrypoint and -becomes ten `source` lines plus comments. `build.sh` needs no per-module -knowledge: it already recurses into `source` lines, so the module children are -discovered through the aggregator. - -The aggregator sits **beside** the directory rather than inside it as an -`index.sh`. Both are identical to `build.sh` — see the next section, the build -follows the `source` graph and cannot tell the difference — so the choice is -about the source tree, not the artifact. Beside wins on precedent: it is exactly -how `src/assertions.sh` has aggregated the flat `src/assert_*.sh` files since -long before module directories existed, so there is one aggregator convention in -`src/` rather than two. Inside would make each module a self-contained directory -and keep `ls src/` free of aggregator/directory pairs, which is the real argument -for it; it was rejected as not worth renaming shipped modules and rewriting -entrypoint lines for a cosmetic gain. Revisit only with a deliberate amendment -here — not per module, or `src/` ends up with both conventions. +Chosen option: **`src/runner/` directory with the aggregator inside it as +`index.sh`** (originally "beside it"; amended 2026-08-01, below). + +The entrypoint keeps a single `source` line per module — +`src/runner/index.sh` — and that file is ten `source` lines plus comments. +`build.sh` needs no per-module knowledge: it already recurses into `source` +lines, so the module children are discovered through the aggregator. + +### Aggregator placement (amended 2026-08-01) + +The aggregator lives **inside** the module directory as `index.sh`: + +``` +src/runner/index.sh <- the module's entry point +src/runner/exec.sh +src/coverage/index.sh +src/coverage/engine.sh +``` + +Both placements are identical to `build.sh` — it follows the `source` graph and +cannot tell the difference (next section) — so this is a source-tree decision, +not an artifact one. + +The original decision was "beside" (`src/runner.sh` + `src/runner/*.sh`), on the +grounds that it matched the `src/assertions.sh` → `src/assert_*.sh` precedent and +kept one aggregator convention in `src/`. **That reasoning was wrong.** +`src/assertions.sh` aggregates *flat* files; there is no `src/assert/` directory. +It is not a module aggregator at all, it is unaffected by this choice, and it +would remain exactly as it is under either placement. There was never a +"two conventions" risk to avoid. + +Two arguments settle it for inside: + +* **A hand-maintained aggregator list drifts; a glob cannot.** The rule that + aggregators hold only `source` lines is enforced by + `test_module_aggregators_hold_only_source_lines_and_comments`, which named its + aggregators in a string: `"src/assertions.sh src/runner.sh"`. `src/coverage.sh` + was added in #928 and never appended, so the rule silently stopped covering it + within one module of being introduced. With the aggregator at a predictable + `src/*/index.sh`, the test discovers modules by glob and cannot drift. This is + the deciding argument: it is a correctness property, not a preference. +* **The cost only grows.** Converting two shipped modules costs two renames and + two entrypoint lines. #931 adds nine more; converting after it costs eleven. + +Alongside those, the ordinary case for treating a directory as the module: `mv` +or `rm -r` moves it as one unit, an orphaned `src/runner.sh` left behind by a +deleted `src/runner/` becomes impossible, and `ls src/` lists modules rather than +aggregator/directory pairs. The convention also matches what most ecosystems do — +`index.ts`, `__init__.py`, `mod.rs`. + +`index.sh` over `main.sh` or `init.sh`: both of those collide with existing +concepts here (`src/main.sh`, and `src/init.sh` implementing `bashunit init`). + +Apply this to every module. Mixing placements is the outcome worth avoiding. This required fixing `build.sh` first (#923). Its embed dedupe was keyed on a file's *basename*, which both hid a genuine double-embed (the top-level loop @@ -161,27 +203,35 @@ context · payload · diagnostics → parallel · hooks · result → provider grouping in a filename prefix rather than in the directory structure. * Bad, because it does not generalise — `src/coverage.sh` would add nine more. -### `src/runner/` directory behind an aggregator beside it +### `src/runner/` directory behind an aggregator beside it (rejected on amendment) -* Good, because it mirrors the existing `src/assertions.sh` → `src/assert_*.sh` - aggregator precedent, and `src/dev/debug.sh` already proved `src/` can nest. * Good, because `build.sh` discovers children through recursion, so adding a module file needs no build change. -* Good, because the entrypoint keeps one `source` line per module, unchanged in - shape from when the module was a single file. +* Good, because the entrypoint line keeps the shape it had when the module was a + single file. +* Bad, because the aggregator's path is unpredictable, so the test enforcing the + "only `source` lines" rule needs a hand-maintained list — which drifted the + first time a second module appeared (#928 added `src/coverage.sh` and never + appended it, silently un-enforcing the rule for that file). +* Bad, because `ls src/` shows an aggregator/directory pair per module, and the + pair can desync: deleting `src/runner/` leaves an orphaned `src/runner.sh`. * Bad, because it required fixing the build's dedupe key first (#923). -* Bad, because `ls src/` shows an aggregator/directory pair per module. - -### `src/runner/index.sh` inside the directory - -* Good, because a module becomes one self-contained directory — moving or - deleting it is a single `mv`/`rm -r`, and `ls src/` shows modules, not pairs. -* Good, because it scales more cleanly if `src/` ever reaches a dozen modules. +* ~~Good, because it mirrors the `src/assertions.sh` → `src/assert_*.sh` + precedent~~ — withdrawn. `src/assertions.sh` aggregates flat files and has no + directory, so it is not a module aggregator and is unaffected either way. + +### `src/runner/index.sh` inside the directory (chosen on amendment) + +* Good, because the aggregator sits at a predictable `src/*/index.sh`, so the + enforcement test discovers modules by glob and cannot drift. +* Good, because a module becomes one self-contained directory — `mv`/`rm -r` + moves it as a unit, no orphan is possible, and `ls src/` lists modules. +* Good, because it matches the convention most ecosystems already use + (`index.ts`, `__init__.py`, `mod.rs`), so the entry point is where a reader + looks for it. * Neutral on the build: identical to beside, which walks the `source` graph. -* Bad, because it would rename the two shipped modules and rewrite their - entrypoint lines for no behavioural gain. -* Bad, because `src/assertions.sh` would stay a beside-aggregator (it has no - directory), leaving `src/` with two aggregator conventions. +* Bad, because it cost renaming the two shipped modules and their entrypoint + lines — a cost that only grows, hence doing it before #931 adds nine more. ## Links diff --git a/bashunit b/bashunit index b4ff0f59..cd482436 100755 --- a/bashunit +++ b/bashunit @@ -73,7 +73,7 @@ source "$BASHUNIT_ROOT_DIR/src/io.sh" source "$BASHUNIT_ROOT_DIR/src/math.sh" source "$BASHUNIT_ROOT_DIR/src/parallel.sh" source "$BASHUNIT_ROOT_DIR/src/env.sh" -source "$BASHUNIT_ROOT_DIR/src/coverage.sh" +source "$BASHUNIT_ROOT_DIR/src/coverage/index.sh" source "$BASHUNIT_ROOT_DIR/src/clock.sh" source "$BASHUNIT_ROOT_DIR/src/state.sh" source "$BASHUNIT_ROOT_DIR/src/colors.sh" @@ -87,7 +87,7 @@ source "$BASHUNIT_ROOT_DIR/src/assertions.sh" source "$BASHUNIT_ROOT_DIR/src/doc.sh" source "$BASHUNIT_ROOT_DIR/src/reports.sh" source "$BASHUNIT_ROOT_DIR/src/rerun.sh" -source "$BASHUNIT_ROOT_DIR/src/runner.sh" +source "$BASHUNIT_ROOT_DIR/src/runner/index.sh" source "$BASHUNIT_ROOT_DIR/src/benchmark.sh" source "$BASHUNIT_ROOT_DIR/src/bashunit.sh" source "$BASHUNIT_ROOT_DIR/src/init.sh" diff --git a/src/coverage.sh b/src/coverage/index.sh similarity index 93% rename from src/coverage.sh rename to src/coverage/index.sh index 30a0bf6f..651e60ff 100644 --- a/src/coverage.sh +++ b/src/coverage/index.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash -# Aggregator for the src/coverage/ module: only `source` lines and comments +# Entry point for the src/coverage/ module: only `source` lines and comments # belong here. build.sh emits a file's body before recursing into its `source` # lines, so any statement here would run before its dependencies in the built # binary (adrs/adr-010-src-module-directories.md). diff --git a/src/runner.sh b/src/runner/index.sh similarity index 91% rename from src/runner.sh rename to src/runner/index.sh index 50d26da4..9c70aed2 100755 --- a/src/runner.sh +++ b/src/runner/index.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash -# Aggregator for the src/runner/ module: only `source` lines and comments belong +# Entry point for the src/runner/ module: only `source` lines and comments belong # here. build.sh emits a file's body before recursing into its `source` lines, so # any statement here would run before its dependencies in the built binary. # diff --git a/tests/unit/build_test.sh b/tests/unit/build_test.sh index 82b2fe7d..dc97650e 100644 --- a/tests/unit/build_test.sh +++ b/tests/unit/build_test.sh @@ -91,22 +91,44 @@ function test_build_embed_docs_fails_on_missing_markers() { # build::process_file emits a file's body and *then* recurses into its `source` # lines, so an aggregator holding anything else at top level would run that code -# before its dependencies in the built binary but after them in dev mode. Add any -# new aggregator here. -function test_module_aggregators_hold_only_source_lines_and_comments() { - local aggregators="src/assertions.sh src/runner.sh" +# before its dependencies in the built binary but after them in dev mode. +# +# Discovered by glob, never by a hand-maintained list: the previous list named +# src/assertions.sh and src/runner.sh, and src/coverage.sh was added in #928 +# without being appended, so the rule silently stopped covering it. A module's +# aggregator is src//index.sh (ADR-010); src/assertions.sh is the one +# flat-file aggregator, which has no directory of its own. +function build_aggregators() { + echo "src/assertions.sh" + local index + for index in "$ROOT_DIR"/src/*/index.sh; do + [ -f "$index" ] || continue + echo "${index#"$ROOT_DIR"/}" + done +} +function test_module_aggregators_hold_only_source_lines_and_comments() { local offenders="" local aggregator - for aggregator in $aggregators; do + while IFS= read -r aggregator; do if grep -qvE '^[[:space:]]*(#|source |$)' "$ROOT_DIR/$aggregator"; then offenders="$offenders $aggregator" fi - done + done < <(build_aggregators) assert_empty "$offenders" } +# The glob above is only a safety net if it actually finds the modules. +function test_module_aggregator_discovery_finds_every_module() { + local found + found=$(build_aggregators | tr '\n' ' ') + + assert_contains "src/runner/index.sh" "$found" + assert_contains "src/coverage/index.sh" "$found" + assert_contains "src/assertions.sh" "$found" +} + function test_build_process_file_embeds_a_file_only_once() { local dir dir=$(bashunit::temp_dir)