Repository navigation
refactor(coverage): split src/coverage.sh into a src/coverage/ module - #928
Merged
Merged
Conversation
src/coverage.sh was 2548 lines and 59 functions, the largest file in src/. Apply the same treatment as #927: twelve cohesive modules behind a thin aggregator, layered leaves-first and acyclic. config · paths · lines · functions → engine · stats · branches → report_text · report_lcov · report_html → html_index · html_file The namespace is unchanged, no function is renamed, and bashunit:76 keeps its single source line. The responsibility map was posted on #925 and agreed before any code moved. This is a relocation. Verified by comparing the non-blank line multiset of the twelve new files against the original: the only differences are the six lines reflowed below and one stale directive. Function count is 59 before and after, and the aggregator holds zero statements that are not `source` or comment. branches.sh is deliberately one file: the _branch_* helpers mutate extract_branches's locals through dynamic scoping, so they cannot be separated. The justification comment travels with them. .editorconfig: the old whole-file `max_line_length = unset` is replaced by the same rule scoped to html_index.sh and html_file.sh, which hold 50 of the 56 lines over 120 chars. Every other coverage module now honours the global limit, so this is a net tightening rather than a blanket exemption. The remaining six lines were reflowed by hand; the two that build a shared string (_NONEXEC_PATTERN, _XTRACE_PS4) were checked to evaluate byte-identically. Two fixes that the move made unavoidable: - .gitignore had an unanchored `coverage/`, which matched the new src/coverage/ source directory and silently excluded all twelve modules from git. Anchored to /coverage/, which is where the generated report actually lives. - The file-wide `# shellcheck disable=SC2094` was dropped. It is stale: shellcheck does not raise SC2094 on the original file either, and `make sa` passes without it. Not fixed, and pre-existing: `--coverage` reports an arithmetic error from the lcov function-record loop when a function span is empty. It reproduces identically on main, so it stays out of a pure-move PR. Verified: sequential, --parallel, --parallel --simple --strict, --coverage --parallel, make sa, make lint, fork-budget acceptance tests, and `bash build.sh bin -v` printing "Build verified". Closes #925
CI runs ShellCheck per file without -x, so it cannot follow `source`. In the 2548-line monolith it saw `line_hits` used in compute_file_coverage and stayed quiet about the identically named local in report_lcov; once the two functions landed in different files it correctly reported SC2034. The variable is genuinely dead: report_lcov declares it in its `local` list and never reads or writes it. Removing the declaration rather than suppressing the warning -- a `# shellcheck disable` here would only preserve dead code. Verified with CI's exact invocation (per file, no -x, same SHELLCHECK_OPTS) across every tracked shell file.
This was referenced Jul 31, 2026
Chemaclass
added a commit
that referenced
this pull request
Jul 31, 2026
Four blocks a session can reconstruct on its own, two of which had already drifted out of date: - the architecture tree, which is `ls` output and still showed a flat `src/*.sh` after #927 and #928 added src/runner/, src/coverage/ and src/dev/ - the skills table, which duplicates the skill listing already injected into every session and had gone stale at 9 of 11 entries (missing /review and /gh-issues, the most-used skill in the repo) - Common Commands, whose contents are in the Makefile and whose one non-obvious note -- that `make lint` is the formatting authority -- is already stated under Quality Standards - the Path-Scoped Guidelines bullet lists, which restate bash-style.md and testing.md; the line explaining how the rules auto-load stays Everything not derivable is kept: the Bash 3.0 prohibited-features list, the shfmt/.editorconfig gotcha, the test-pattern pointers, Guardrails, Definition of Done, Commit Message Format and Prohibited Actions. 6638 -> 4290 chars, roughly 590 fewer resident tokens in every session.
Chemaclass
added a commit
that referenced
this pull request
Jul 31, 2026
ADR-010 was written from the runner split (#924) and merged before the coverage split (#925/#928) landed, so it is missing both an option and the constraints that second application uncovered. Considered Options gains the one it never weighed: an `index.sh` inside the module directory, rather than an aggregator beside it. Decision stays "beside", now with the reason stated -- `src/assertions.sh` has aggregated the flat `assert_*.sh` files that way since before module directories existed, so beside keeps one aggregator convention in src/ instead of two. The case for inside is recorded rather than dismissed, with a note to revisit it only by amending this ADR, never per module. The build section now states a measured fact instead of an assertion: `build::process_file` is a depth-first walk of `source` statements, not of directories, so nesting depth and aggregator placement are invisible to it. Verified with a throwaway three-level module -- all bodies embedded in DFS order, no `source` lines left in the artifact. Adds the five traps a split has to respect, four of them found the hard way in #928: order-dependent file-scope initialisers, an unanchored .gitignore pattern that silently excluded twelve new files from git, CI's per-file ShellCheck surfacing what a monolith hid, per-file .editorconfig rules lost by the split, and inseparable dynamic-scope helper groups. Plus the line-multiset check that proves a split is a pure relocation.
This was referenced Jul 31, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤔 Background
Related #925
src/coverage.shwas 2548 lines and 59 functions — the largest file insrc/. Same treatment as #927. The responsibility map was posted on #925 and agreed before any code moved.💡 Changes
config · paths · lines · functions → engine · stats · branches → report_text · report_lcov · report_html → html_index · html_file. Namespace unchanged, no function renamed,bashunit:76keeps its singlesource..editorconfigdecision (narrow, deliberate): the old whole-filemax_line_length = unsetis replaced by the same rule scoped tohtml_index.sh+html_file.sh, which hold 50 of the 56 over-120 lines. Every other coverage module now honours the global 120 — a net tightening. The remaining 6 lines were reflowed by hand, and the two building shared strings (_NONEXEC_PATTERN,_XTRACE_PS4) were verified to evaluate byte-identically..gitignorefix, required by the move:coverage/was unanchored, so it matched the newsrc/coverage/source directory and silently excluded all twelve modules from git. Anchored to/coverage/.# shellcheck disable=SC2094— shellcheck does not raise it on the original file either.branches.shstays one file on purpose: the_branch_*helpers mutateextract_branches's locals via dynamic scoping.✅ Verification
Relocation proven by comparing the non-blank line multiset before/after — only differences are the 6 reflowed lines and the stale directive. 59 functions before and after; aggregator holds zero non-
sourcestatements.Green: sequential ·
--parallel·--parallel --simple --strict·--coverage --parallel·make sa·make lint· fork-budget acceptance tests ·bash build.sh bin -v→✅ Build verified ✅.Pre-existing and not fixed here:
--coveragereports an arithmetic error from the lcov function-record loop on an empty function span. Reproduces identically onmain, so it stays out of a pure-move PR.No CHANGELOG entry — internal, no user-visible behaviour change.