Repository navigation
Slice 2: CI for the test suite - #4
Conversation
Adds a GitHub Actions workflow on ubuntu-latest (the suite spawns real PIDs and reads /proc, so it needs a Linux runner). Fixes the shellcheck findings the new gate surfaced: unquoted expansion inside a pattern, redundant $ on arithmetic array indices, a name collision between the associative target_indices local and the top-level index array, an unquoted $repo_dir in the tests, and a duplicated shebang line. Closes #2 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The first CI run failed on the pretty-table assertion: `sort` collates by locale, so a listener bound to both `*:3002` and `127.0.0.1:3002` came out in one order under en_US.UTF-8 and the other under the runner's C locale. BIND ordering should not depend on the environment's locale, so pin the sort to LC_ALL=C and assert the C order in the suite. Also limit the push trigger to main so PR branches get one run, not two. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98e16726ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| run: shellcheck $SCRIPTS | ||
|
|
||
| - name: CLI tests | ||
| run: bash tests/test-cli.sh |
There was a problem hiding this comment.
On the Ubuntu runner's C.UTF-8 locale, the suite fails at tests/test-cli.sh:118: listener discovery sorts the mocked endpoints before rendering them, placing *:3002 before 127.0.0.1:3002, while the assertion requires the reverse order. Running this exact step produces FAIL: second pretty row lacks process metadata or bindings, so the newly added CI job will fail consistently unless the assertion accepts the sorted order (or otherwise ignores endpoint ordering).
Useful? React with 👍 / 👎.
|
@codex review the latest fixes on commit 3dbdbce: the P1 binding-order finding is fixed at the source rather than in the assertion. Listener discovery now sorts with |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Closes #2.
Adds
.github/workflows/ci.yml, running onpushandpull_request:bash -nonkill-dev-servers.sh,install.sh,tests/test-cli.sh(one file per invocation —bash -nonly reads its first file argument)shellcheckon the same three files (preinstalled onubuntu-latest)bash tests/test-cli.shubuntu-latestbecause the suite spawns real background PIDs and the script reads/proc.Turning shellcheck on surfaced findings that had to be fixed to make the gate green:
kill-dev-servers.sh:279— unquoted${BASH_REMATCH[1]}inside a${var#pattern}pattern (SC2295)kill-dev-servers.sh:364-367— redundant$on arithmetic array indices (SC2004)kill-dev-servers.sh— the associativetarget_indiceslocal incollect_candidatesshared a name with the top-level positional index array; the top-level one is nowselected_indices(SC2190)tests/test-cli.sh— unquoted$repo_dirin command substitutions (SC2086)kill-dev-servers.sh:1— duplicated shebang lineAll behavior-preserving;
bash tests/test-cli.shpasses andshellcheckis clean locally.Not included: the bash 4 matrix the issue floats as optional. No runner or distro image gives a bash 4 with procps
psand/proc(thebash:4.ximages are BusyBox/Alpine, already unsupported per the README), so it would mean building bash from source in CI. The README's "Bash 4 or newer" claim stays untested. Installer coverage is likewise left out, as the issue notes it is a separate question.🤖 Generated with Claude Code