From 12e9f5916f317132920ad36a193be56097c43b69 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Fri, 21 Aug 2026 16:11:29 +0200 Subject: [PATCH] extract_shell_functions: recognise the `function name()` spelling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The detection regex matched only `name() {`. bash also accepts `function name() {` and `function name {`, and pacman_wrapper.sh uses the keyword form for 17 of its 30 functions — so a split of that file would have left every one of them behind in the entry script while the tool reported success. Silent, and exactly the kind of half-done split the verifier is meant to catch afterwards rather than the extractor causing. Parentheses stay mandatory in the non-keyword branch. Making them optional everywhere also matches brace groups and control constructs, which would sweep non-function code into the library — the opposite failure, and worse. Nine negative tests pin that boundary. The function name now comes from a named capture group rather than splitting on "(", which returned "function name" for the keyword form and would have broken both --functions selection and the verifier's name matching. Verified against the already-split files first: cco_state.sh still yields 13 functions and leechblock_fetch.sh 3, so the completed splits are unaffected. pacman_wrapper.sh goes from 13 detected to 31. First tests for this script: 20 cases covering every accepted spelling, brace matching across nested bodies, and single-line functions. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01F3PzGc75gBcdxP3PkHXGcV --- ...ell-functions-keyword-form-2026-08-21.json | 36 +++++ meta/scripts/extract_shell_functions.py | 22 ++- .../tests/test_extract_shell_functions.py | 140 ++++++++++++++++++ 3 files changed, 195 insertions(+), 3 deletions(-) create mode 100644 docs/superpowers/evidence/extract-shell-functions-keyword-form-2026-08-21.json create mode 100644 python_pkg/tests/test_extract_shell_functions.py diff --git a/docs/superpowers/evidence/extract-shell-functions-keyword-form-2026-08-21.json b/docs/superpowers/evidence/extract-shell-functions-keyword-form-2026-08-21.json new file mode 100644 index 00000000..39ef7034 --- /dev/null +++ b/docs/superpowers/evidence/extract-shell-functions-keyword-form-2026-08-21.json @@ -0,0 +1,36 @@ +{ + "intent": "Teach extract_shell_functions.py bash's `function name() {` spelling. It matched only `name() {`, so a file using the keyword form had those functions silently left behind while the tool still reported success.", + "scope": [ + "meta/scripts/extract_shell_functions.py: the function-detection regex and how the name is derived", + "python_pkg/tests/test_extract_shell_functions.py: first tests for this script", + "Non-goal: any change to how blocks are brace-matched or written out" + ], + "changes": [ + "The regex now accepts `name() {`, `function name() {` and `function name {`. Parentheses stay MANDATORY in the non-keyword branch: making them optional everywhere would also match brace groups and control constructs, which would sweep non-function code into the library — the opposite failure and a worse one.", + "The name comes from a named capture group instead of lines[start].split('(')[0], which yielded 'function name' for the keyword form and would have produced a corrupt --functions match and a wrong verify_shell_split.sh name." + ], + "verification": [ + { + "command": "python3 -m pytest python_pkg/tests/test_extract_shell_functions.py -q", + "result": "pass", + "evidence": "20 passed. Covers all five spellings bash accepts, nine negative cases (indented definitions, `while true; do {`, `if x {`, bare braces, a commented-out definition, a bare call), brace matching across a nested if/case body, single-line functions, and a file mixing all three forms." + }, + { + "command": "find_function_blocks on the real files", + "result": "pass", + "evidence": "pacman_wrapper.sh now yields 31 functions where the old pattern found 13 — the 17 `function`-keyword definitions plus one the count had missed. cco_state.sh (13) and leechblock_fetch.sh (3) are unchanged, so the already-completed splits would extract identically." + }, + { + "command": "pre-commit run --files ", + "result": "pass", + "evidence": "ruff (2 import-order fixes applied), ruff format, mypy, pylint, bandit, pytest-with-coverage and codespell all pass. No suppression added." + } + ], + "risks": [ + "A file that legitimately contains a top-level brace group opening at column 0 would now be misread if it also began with an identifier. The mandatory-parens rule plus the nine negative tests are what bound this; the alternative (missing real functions) is the failure that actually occurred.", + "This changes the tool every remaining split depends on. It was verified against the three already-split files first, so a regression would have shown up as a different function count there." + ], + "rollback": [ + "git revert the commit; the extractor returns to matching only `name() {` and the new tests fail, which is the intended signal rather than a silent regression." + ] +} diff --git a/meta/scripts/extract_shell_functions.py b/meta/scripts/extract_shell_functions.py index d43f8621..e7675363 100755 --- a/meta/scripts/extract_shell_functions.py +++ b/meta/scripts/extract_shell_functions.py @@ -18,7 +18,19 @@ from pathlib import Path import re import sys -_FUNC_RE = re.compile(r"^[a-zA-Z_][a-zA-Z0-9_]*\(\)\s*\{") +# Both bash spellings: `name() {` and `function name() {`. Accepting only the +# first silently skipped 17 of the 30 functions in pacman_wrapper.sh, leaving +# them in the entry script while reporting success -- a split that looks done +# and is not. +# +# `()` is required unless the `function` keyword is present. Making the parens +# optional in both branches would match plain brace groups and control blocks +# (`while read -r x; do ... done {`-style constructs and `if ... {` in other +# dialects), sweeping non-function code into the library. +_FUNC_RE = re.compile( + r"^(?:function\s+(?P[a-zA-Z_][a-zA-Z0-9_]*)\s*(?:\(\))?" + r"|(?P[a-zA-Z_][a-zA-Z0-9_]*)\s*\(\))\s*\{" +) def find_function_blocks(lines: list[str]) -> list[tuple[int, int, str]]: @@ -26,7 +38,11 @@ def find_function_blocks(lines: list[str]) -> list[tuple[int, int, str]]: blocks: list[tuple[int, int, str]] = [] index = 0 while index < len(lines): - if _FUNC_RE.match(lines[index]): + match = _FUNC_RE.match(lines[index]) + if match: + # Take the name from the regex, not from splitting on "(": the + # `function name()` form would otherwise yield "function name". + name = match.group("kwname") or match.group("name") start, depth = index, 0 while index < len(lines): depth += lines[index].count("{") - lines[index].count("}") @@ -35,7 +51,7 @@ def find_function_blocks(lines: list[str]) -> list[tuple[int, int, str]]: if depth <= 0: break index += 1 - blocks.append((start, index, lines[start].split("(")[0])) + blocks.append((start, index, name)) index += 1 return blocks diff --git a/python_pkg/tests/test_extract_shell_functions.py b/python_pkg/tests/test_extract_shell_functions.py new file mode 100644 index 00000000..4878e737 --- /dev/null +++ b/python_pkg/tests/test_extract_shell_functions.py @@ -0,0 +1,140 @@ +"""Tests for meta/scripts/extract_shell_functions.py. + +The extractor is what every 250-line-cap split runs through, and its failure +mode is silent: a function form it does not recognise is simply left behind in +the entry script while the tool still reports success. That is how 17 of the +30 functions in pacman_wrapper.sh were nearly missed — the original pattern +matched only `name() {` and not bash's `function name() {` spelling. +""" + +from __future__ import annotations + +import importlib.util +from pathlib import Path +from typing import TYPE_CHECKING + +import pytest + +if TYPE_CHECKING: + from types import ModuleType + +_ROOT = Path(__file__).resolve().parents[2] +_SCRIPT = _ROOT / "meta" / "scripts" / "extract_shell_functions.py" + + +def _load() -> ModuleType: + """Import the script by path; it lives outside any importable package.""" + spec = importlib.util.spec_from_file_location("extract_shell_functions", _SCRIPT) + assert spec is not None + assert spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +@pytest.fixture(name="esf", scope="module") +def _esf() -> ModuleType: + return _load() + + +@pytest.mark.parametrize( + ("line", "expected"), + [ + # Both spellings bash accepts, with and without whitespace variants. + ("foo() {", "foo"), + ("foo(){", "foo"), + ("foo () {", "foo"), + ("function foo() {", "foo"), + ("function foo(){", "foo"), + # The keyword form may omit the parentheses entirely. + ("function foo {", "foo"), + ("_leading_underscore() {", "_leading_underscore"), + ("name2() {", "name2"), + ], +) +def test_recognises_every_function_spelling( + esf: ModuleType, line: str, expected: str +) -> None: + """Every form bash accepts must be detected, and named correctly.""" + match = esf._FUNC_RE.match(line) + assert match is not None, f"{line!r} was not recognised as a function" + name = match.group("kwname") or match.group("name") + assert name == expected + + +@pytest.mark.parametrize( + "line", + [ + # Indented definitions are not top level. + " indented() {", + "\tindented() {", + # Brace groups and control flow must never be swept into a library: + # without the mandatory (), these would match and the extractor would + # move non-function code. + "while true; do {", + "if x {", + "{", + "}", + # A commented-out definition is not a definition. + "# foo() {", + # A call is not a definition. + "foo()", + "", + ], +) +def test_ignores_everything_that_is_not_a_top_level_function( + esf: ModuleType, line: str +) -> None: + assert esf._FUNC_RE.match(line) is None, ( + f"{line!r} was wrongly treated as a function" + ) + + +def test_brace_matching_spans_the_whole_body(esf: ModuleType) -> None: + """A function block must end at its own closing brace, not the first one.""" + lines = [ + "function outer() {", + "\tif true; then", + "\t\techo nested", + "\tfi", + "\tcase $x in", + "\ta) echo a ;;", + "\tesac", + "}", + "", + "after() {", + "\techo after", + "}", + ] + blocks = esf.find_function_blocks(lines) + assert [name for _, _, name in blocks] == ["outer", "after"] + start, end, _ = blocks[0] + assert (start, end) == (0, 7) + + +def test_single_line_function_closes_on_its_own_line(esf: ModuleType) -> None: + """`name() { ...; }` opens and closes on one line.""" + blocks = esf.find_function_blocks(["logg() { printf '%s\\n' \"$*\"; }", "echo hi"]) + assert [name for _, _, name in blocks] == ["logg"] + start, end, _ = blocks[0] + assert start == end == 0 + + +def test_detects_both_spellings_in_one_file(esf: ModuleType) -> None: + """The regression that motivated these tests: a file mixing both forms.""" + lines = [ + "#!/bin/bash", + "plain() {", + "\techo plain", + "}", + "", + "function keyword() {", + "\techo keyword", + "}", + "", + "function no_parens {", + "\techo no_parens", + "}", + ] + names = [name for _, _, name in esf.find_function_blocks(lines)] + assert names == ["plain", "keyword", "no_parens"]