Fix (b)run_fastsurfer on macOS, add python fs_time replacement, and tests for Mac in CI

[m-reuter]

Makes brun_fastsurfer.sh usable on macOS, with a smaller fix to run_fastsurfer.sh and new tests
to keep both that way. Also port fs_time to python to be portable across OSs, enabling timing in macOS.

Apple ships bash 3.2.57 as /bin/bash (frozen at the last GPLv2 release), and every shipped
script selects that interpreter through its own #!/bin/bash, whatever the user's login shell is.
Three separate GNU/bash-4 dependencies meant --subject_list, subjects on stdin and --parallel <n>
could not work there. The run_fastsurfer.sh change is one guard removal, restoring the
"Potentially Overwriting" warning that macOS users never saw.
Subject lists are also parsed correctly on macOS now: paths containing spaces can be quoted or escaped as in the shell, trailing whitespace and blank lines are stripped rather than a trailing  s , and a list file without a final newline keeps its last subject.

Found while packaging FastSurfer for macOS #859 ; none of it is macOS-packaging specific.

1. mapfile is bash 4+, so three features were refused outright

brun_fastsurfer.sh guarded its four mapfile calls with fail_bash_version_lt4, which exited with
"requires at minimum bash version 4" for --subject_list, subjects via stdin, and --parallel <n>.
On macOS that guard always fires, so those three features were simply unavailable.

Replaced with while IFS= read -r, which bash 3.2 has, so the features work rather than being
refused. The two duplicated job-counting sites collapse into one get_running_jobs helper, and
fail_bash_version_lt4 and its five call sites are gone.

run_fastsurfer.sh had the same guard around its "Potentially Overwriting" warning, meaning macOS
users never saw it when a subject directory already contained files. That block is now unconditional.

2. cut --output-delimiter is GNU-only, and silently dropped the t1 path

With the refusal lifted, the next failure appeared: cut -d= -f2-1000 --output-delimiter="=" in
process_by_token. BSD cut rejects the option, so image_parameters came out empty and the
image path in every <subject_id>=<path> spec was lost.

The option was redundant even on GNU, since cut -f already joins with the input delimiter, which is
why stools.sh never had it. Removed.

3. expr with a GNU BRE parsed nothing at all

The subject-spec tokenizer used expr "$s" : "$regex" with a BRE containing \| and \+, both GNU
extensions. BSD expr returns an empty string for every input, including a bare word, so no
subject parameters parsed on macOS at all.

Now matched with bash's own =~ (an ERE), which drops the expr dependency entirely. The pattern is
built from single-quoted pieces, because a single quote cannot appear inside a single-quoted string,
and it is verified against escaped spaces, both quote styles, and escapes nested inside double quotes.

Tests

test/shell/test_bash_compat.py, 19 tests:

• 16 static lint tests, one per script, flagging constructs bash 3.2 lacks (mapfile,
  readarray, declare -A, declare -n, coproc, wait -n, &>>, ${x^^}/${x,,}) and
  reporting the file, line and a replacement. Platform independent, so it runs on every PR via
  the existing ubuntu matrix (tests: ["image", "config", "shell"]). Covers the pipeline entry
  points, all five recon_surf/*.sh, the macOS build, the install-time scripts, and the
  .sh.template files rendered into the installed package.
• 3 functional tests for the paths that were refused, which need a real bash 3.2 and so run in a
  new macos-shell-test job on macos-15. That job also fails deliberately if /bin/bash stops
  being 3.x, so the guard cannot rot unnoticed the way it did before.

The lint alone would not have caught issues 2 and 3: both are GNU-only options to external tools,
and only executing the code on a BSD userland finds those. That is what the mac job buys, and the
same class of risk (sed -i, date -d, stat -c, grep -P) remains open elsewhere.

Verification

brun_fastsurfer.sh on both interpreters, invoked explicitly so its own shebang could not substitute
3.2, with a stub via --run_fastsurfer so nothing is actually processed:

	bash 3.2	bash 5.3
--parallel 2, 4 subjects	4 started, peak concurrency 2	identical
--parallel max, 4 subjects	4 started, peak 4	identical

Asserting the peak is the point: an always-empty running_jobs would still start all four, so
counting subjects would not test the job accounting. End to end the stub receives
--t1 <path> --sd <dir> --device auto --viewagg_device auto --sid subjX, with per-subject flags such
as --seg_only passed through. The lint was confirmed to fail on an injected mapfile rather than
being decorative.

run_fastsurfer.sh --seg_only was run on a real subject on macOS (bash 3.2), against a copy of an
existing output directory, and produced the warning that used to be skipped there:

WARNING: Found 27 files in subject directory $SUBJECTS_DIR/OAS1_0011_MR1:
Potentially Overwriting: ./surf/callosum.surf ./surf/callosum.thickness.w ...
That path is not in CI, since it needs real input data; the lint covers it against regressions.
The Linux skip path for the three functional tests is correct by construction but will first be
confirmed by CI.

Also in this PR: fs_time no longer needs GNU time

recon_surf/fs_time wrapped /usr/bin/time -f, a GNU extension, so functions.sh probed it, found
it broken and silently disabled per-command timings on every macOS run. It now measures with
os.wait4(), so there is no external tool to be missing. Verified against real GNU time on Linux
(identical field layout, maxrss identical for a 500 MB command) and on macOS by a full seg-only
run, which produced 8 timed commands where it previously produced none.

The rewrite also fixes two things the bash version got wrong:  @#@FSLOADPOST  now carries the load averages its own help documents, and an empty  FSTIME_LOAD  counts as "on" rather than "off".

Two known limits, both documented in the file: M cannot read below the launcher's own footprint on
Linux, which only affects commands too small for the figure to be interesting, and macOS reports no
block IO or swap counts, so I, O and W read . there.

Log output clearer

For example, HypVINN used to report its own runtime as "whole pipeline"; that is fixed here.

[Copilot]

🟡 Changes recommended

Subject parsing drops unterminated final lines and passes quoted or escaped paths with their syntax characters intact.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Improves macOS Bash 3.2 compatibility and adds CI regression coverage.

Changes:

• Replaces Bash 4/GNU-only shell constructs.
• Restores overwrite warnings on macOS.
• Adds static and functional shell compatibility tests.

File summaries

File	Description
brun_fastsurfer.sh	Reworks subject parsing and job tracking.
run_fastsurfer.sh	Enables overwrite detection under Bash 3.2.
test/shell/test_bash_compat.py	Adds compatibility tests.
.github/workflows/unittest.yaml	Runs shell tests on Linux and macOS.

Review details

Suppressed comments (1)

brun_fastsurfer.sh:293

• The stdin variant has the same final-line regression: if a producer closes stdin without a trailing newline, read fills subject_line but the loop body is skipped, so the final subject is never processed. Preserve a nonempty unterminated line as mapfile did.

  while IFS= read -r subject_line ; do subjects+=("$subject_line") ; done \
    < <(sed "$SED_CLEANUP_SUBJECTS")

• Files reviewed: 4/4 changed files
• Comments generated: 3
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The unresolved moderate timing-wrapper and concurrency-test issues must be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

• Files reviewed: 9/9 changed files
• Comments generated: 5
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Five unresolved moderate issues affect parsing correctness, safe execution, lint coverage, and functional test reliability.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

brun_fastsurfer.sh:562

• Double-quoted backslashes do not always escape the next character in the shell: before an ordinary character they remain literal. This branch removes every backslash, so a valid subject value such as "a\ b" is passed as a b instead of a\ b, contrary to the documented shell-style parsing. Only consume the backslash before $, backtick, ", or \; otherwise append it literally.

          if [[ "${rest:0:1}" == "\\" ]] && [[ -n "${rest:1:1}" ]]
          then out="$out${rest:1:1}" ; rest="${rest:2}"

• Files reviewed: 9/9 changed files
• Comments generated: 4
• Review effort level: Balanced

[Copilot]

🔵 Needs a closer look

Two moderate test-coverage gaps must be addressed before approval.

Review details

Suppressed comments (4)

Previously missed (2) — in code that hasn't changed since the last review.

test/shell/test_bash4_lint.py:62

• These patterns only match a standalone -A/-n, so valid combined or preceding flags such as declare -rA values and local -r -n ref bypass the Bash-4 guard. Match option groups containing the relevant flag so the lint covers all declaration spellings.
  test/shell/test_brun_bash32.py:87
• The compatibility tests all expect brun_fastsurfer.sh to succeed, but this helper never checks its exit status. A regression that starts every stub correctly and then exits nonzero would therefore pass all the log-based assertions. Assert success here so every caller verifies the CLI contract as well as its side effects.

brun_fastsurfer.sh:68

• This rule is quoting-dependent: unquote preserves backslashes inside single quotes and before non-special characters inside double quotes, so doubling a literal backslash there produces two backslashes. Please qualify that doubling is required outside quotes.

backslash likewise has to be written \\\\.

recon_surf/fs_time:125

• This documented sample omits the literal L that format_load() prefixes to every load sample, so it is not an example of the line the rewritten command actually emits. Update the public output contract to match the implementation and the load-line test.

The 3 numbers are the system load averages for the past 1, 5, and 15 minutes.

• Files reviewed: 10/10 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[Copilot]

🟡 Changes recommended

Unresolved lint failures, signal handling, and test coverage gaps must be addressed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (7)

Previously missed (2) — in code that hasn't changed since the last review.

recon_surf/fs_time:284

• A Ctrl-C delivered to the foreground process group interrupts this wait4 with KeyboardInterrupt, so the wrapper emits a Python traceback and exits before writing the timing record or applying the child’s 128+signal status mapping. The self-signalling child test does not exercise this because only the child receives SIGTERM. Handle wrapper signals while still reaping the child and producing the resource line (and forward signals when only the wrapper was targeted).
  test/shell/test_bash4_lint.py:112
• Skipping a missing hard-coded script makes an accidental rename or stale SHIPPED_SCRIPTS entry pass while silently removing that script from the compatibility scan. Treat this as a test failure so the coverage list cannot shrink unnoticed.

recon_surf/fs_time:136

• UP037 is enabled for this newly linted executable, so this unnecessary quoted annotation is reported by Ruff.

def shorten_command(cmd: list[str]) -> "tuple[str, int]":

recon_surf/fs_time:166

• Because UP037 is selected, Ruff reports this quoted annotation now that fs_time is included in the lint scope.

def make_line(key: str, command: str, nargs: int, extra: str = "", when: "datetime | None" = None) -> str:

recon_surf/fs_time:179

• This non-forward quoted union violates the enabled Ruff UP037 rule and will fail the code-style job.

def field(name: str, value: "int | str") -> str:

test/shell/test_fs_time.py:40

• Ruff's enabled UP037 rule rejects this quoted built-in annotation.

def parse_fields(line: str) -> "dict[str, str]":

test/shell/test_fs_time.py:175

• This quoted union is also reported by the enabled Ruff UP037 rule.

def test_fstime_load_default(value: "str | None", load_expected: bool):

• Files reviewed: 10/10 changed files
• Comments generated: 3
• Review effort level: Balanced