Skip to content

fix: harden CLI per code review; require Bash 4+ - #14

Merged
d6veteran merged 2 commits into
mainfrom
claude/add-team-personas-9jyqiz
Jul 27, 2026
Merged

fix: harden CLI per code review; require Bash 4+#14
d6veteran merged 2 commits into
mainfrom
claude/add-team-personas-9jyqiz

Conversation

@d6veteran

@d6veteran d6veteran commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Summary

Two commits: (1) fixes every High/Medium/Low finding from Robin's code-quality review of the shell codebase, and (2) raises the supported shell floor from Bash 3.2 to Bash 4+, dropping stock-macOS /bin/bash support.

No user-visible behavior changes except the fixed defects: a golden comparison of the old and new CLI produced byte-identical output across list, show, use, switch, status, coordinator on/prod/off, reset, launch --dry-run, help, and the full branch lifecycle.

Review fixes (commit 1)

High severity

  • Non-atomic CLAUDE.md writes (cmd_reset, coordinator off): the second filter stage redirected a pipeline straight onto ~/.claude/CLAUDE.md with || true, so an interrupt or full disk could truncate the user's global config. All block edits now go through block_install/block_remove helpers that write to a temp file and land with mv.
  • Branch-index lookups compiled names as regexes: five awk sites matched $3 ~ " " proj " ". Names like release/1.2.3 cross-matched similar rows, and unbalanced metacharacters crashed awk. All five now trim fields and compare with literal string equality via one index_active_field helper.

Medium severity

  • Triplicated marker-block logic: now single-sourced in the CLI helpers; install.sh delegates its coordinator step to claude-team coordinator on|prod, exercising the tested code path.
  • shellcheck configured but never enforced: added .github/workflows/ci.yml (shellcheck on all five scripts + tests on ubuntu and macOS). All five scripts pass shellcheck 0.9.0 clean.

Low severity

  • Block removal trims only the blank lines the removed block leaves at EOF (the old code stripped leading blanks and every line's trailing whitespace, mangling user content).
  • Dead cyan() deleted; unused SC1090 suppression removed.
  • Name — Role title parsing factored into first-em-dash-splitting helpers, mirrored in generate-agents.sh; regenerating all 16 agents produces a zero diff.
  • session list is one awk pass instead of three subshells per line.
  • Test-suite portability: POSIX ERE alternation, glob count instead of parsing ls, herestrings in assert helpers, quoted literal done.

Bash 4+ floor (commit 2)

Bash 3.2 is a 2006 release Apple froze at /bin/bash over GPLv3. Supporting it constrained the codebase for a twenty-year-old shell, so:

  • README badge and Requirements now state Bash 4+, with the brew install bash note for macOS.
  • bin/claude-team, install.sh, and tests/run.sh fail fast under older bash with a clear error and the brew hint, instead of misbehaving silently on stock macOS.
  • The macOS CI job installs Homebrew bash and runs the suite under it; the version guard makes the job self-verifying.
  • First use of the raised floor: lowercase()/capitalize() are ${var,,}/${var^} parameter expansions instead of tr/awk subshells.

Regression testing (103 → 123 assertions)

  • use+reset and coordinator on+off round-trip a fixture CLAUDE.md byte-for-byte.
  • Dotted and parenthesized project/branch names (my.app vs myXapp, release/1.2.3, app(1), feat/(v1.2.3)) register, resolve, and close without cross-matching or crashing, through both branch and session paths.
  • A multi-dash profile title keeps its full role in list.
  • install.sh runs end to end into an isolated HOME for both the "skip coordinator" and "prod" answers.
  • Each new test family was executed against the previous implementation and fails there; the Bash guard was verified by flipping its threshold and observing the failure path.

bash tests/run.sh: 123/123 passing. shellcheck on all five scripts: clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Nx3U4ZrUAmCpsCeSxvzRWX

claude added 2 commits July 27, 2026 03:03
… index matching)

Fixes all findings from the code-quality review, highest severity first:

- CLAUDE.md is never truncated on failure: reset and coordinator off now
  filter through a temp file and land with mv, like every other write
  path. The old second stage redirected a pipeline straight onto the
  file with a swallowed exit status, so an interrupt or full disk could
  empty the user's global config.
- Branch index lookups compare project and branch names as trimmed
  literal strings instead of compiling them into awk regexes. Names
  like release/1.2.3 no longer cross-match similar projects, and names
  with unbalanced metacharacters no longer crash the lookup. Fixed in
  all five sites (four in the CLI, one in the SessionStart hook).
- The replace-or-append marker-block logic now exists once, as
  block_install/block_remove helpers; install.sh delegates its
  coordinator step to the CLI instead of carrying a third copy.
- CI added (.github/workflows/ci.yml): shellcheck on all five scripts
  plus the test suite on ubuntu and on macOS under bash 3.2, so the
  existing .shellcheckrc is actually enforced.
- Block removal now trims only the blank lines the removed block leaves
  at end-of-file; user content round-trips byte-for-byte (the old code
  stripped leading blanks and every line's trailing whitespace, inverse
  to its own comment).
- Dead cyan() helper removed; unused SC1090 suppression removed.
- Title parsing ("Name — Role") factored into shared helpers that split
  at the first em dash, so roles containing one are not truncated;
  generate-agents.sh mirrors the idiom (regenerated agents: zero diff).
- session list gathers rows in one awk pass instead of three subshells
  per line.
- Test suite portability: ERE alternation instead of GNU-only BRE \|,
  glob count instead of parsing ls, herestrings in the assert helpers,
  and the literal "done" argument quoted for shellcheck.

New regression coverage (103 -> 123 assertions): use/reset and
coordinator on/off round-trip a fixture byte-for-byte; dotted and
parenthesized project/branch names register, resolve, and close without
cross-matching or crashing (verified to fail against the previous
implementation); a multi-dash title keeps its full role; install.sh runs
end to end into an isolated HOME for both the skip and prod answers.

Old and new CLIs produce byte-identical output across list, show, use,
switch, status, coordinator, reset, launch dry-run, help, and the branch
lifecycle (golden comparison; only intended fixes differ).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nx3U4ZrUAmCpsCeSxvzRWX
Bash 3.2 is a 2006 release that Apple froze at /bin/bash over GPLv3;
supporting it meant constraining the codebase for a twenty-year-old
shell. The floor is now Bash 4+:

- README badge and Requirements updated; macOS users install a current
  Bash with 'brew install bash'.
- bin/claude-team, install.sh, and tests/run.sh fail fast with a clear
  message (and the brew hint) when run under anything older, instead of
  misbehaving silently on stock macOS.
- The macOS CI job now installs Homebrew bash and runs the suite under
  it; the version guard in tests/run.sh makes the job self-verifying.
- First use of the raised floor: lowercase()/capitalize() are now
  ${var,,}/${var^} parameter expansions instead of tr/awk subshells.

Guard wiring verified by flipping the threshold and observing the
failure path; list output is unchanged after the helper swap; 123/123
tests pass and shellcheck is clean on all five scripts.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nx3U4ZrUAmCpsCeSxvzRWX
@d6veteran d6veteran changed the title fix: harden CLI file handling per code review (atomic writes, literal index matching) fix: harden CLI per code review; require Bash 4+ Jul 27, 2026
@d6veteran
d6veteran marked this pull request as ready for review July 27, 2026 03:52
@d6veteran
d6veteran merged commit 64c1428 into main Jul 27, 2026
2 of 3 checks passed
d6veteran pushed a commit that referenced this pull request Jul 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants