Repository navigation
ci: run every discovered bats file and skip macOS-only ones - #441
Conversation
|
@codex review |
Codex Review Summary: ❌ FailedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| - name: Run Bats tests | ||
| run: uv run bats --print-output-on-failure tests/esa/zsh.bats tests/moshi/update_moshi_hook.bats tests/codex/update_codex.bats | ||
| # Discovery keeps new files in CI; macOS-only files skip themselves with a reason. | ||
| run: uv run bats --recursive --print-output-on-failure tests |
There was a problem hiding this comment.
P1: Discovery now pulls in several files that build aarch64-darwin derivations, but only tests/nix/configuration.bats skips on Linux. The flake's system is aarch64-darwin, and codex, terminal_browser and friends set platforms = [ "aarch64-darwin" ]. On ubuntu-latest, these will fail because no darwin builder exists:
tests/nix/agent_hook.bats:nix build ...#homeConfigurations."kaito@mbp".activationPackagetests/nix/codex_notification.bats:setup_filerunsnix buildon the darwin codex drv, which fails every test in the filetests/herdr/herdr_command.bats: buildsactivationPackageand ahome.filesourcetests/zsh/startup.bats:276: buildsactivationPackagetests/terminal-browser/terminal_browser.bats:36: runsnix build "$_package_drv^*"on a darwin-only package
So the comment "macOS-only files skip themselves" doesn't hold yet, and this change will likely turn bats-tests red. Add the same Darwin guard to these files, at the file level or per test where only some tests build. nix eval-only tests like ssh-agent/load_identity.bats should still be fine.
Review summary1 blocking finding (P1)
The 🤖 Generated with Claude Code |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Automated review: I found no P0–P3 issues. What I checked:
Note: as the PR description says, |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| - name: Run Bats tests | ||
| run: uv run bats --print-output-on-failure tests/esa/zsh.bats tests/moshi/update_moshi_hook.bats tests/codex/update_codex.bats | ||
| # Discovery keeps new files in CI; macOS-only files skip themselves with a reason. | ||
| run: uv run bats --recursive --print-output-on-failure tests |
There was a problem hiding this comment.
P1 — Discovery now picks up tests/esa/format.bats, which on this branch (and on current origin/main) still calls prettierd. prettierd isn't in the dev shell, so bats-tests fails on every run until #438 lands. If this merges before #438, main's CI goes red.
The PR description says this is intentional. Still, the merge order isn't enforced anywhere. Either hold this PR until #438 is merged and rebased, or skip format.bats when prettierd is missing so the job stays green in between.
Review summary1 blocking finding (P1)
Checked, no issues found
Not verified: I couldn't run the suite (no nix, bats or nvim on the review host) or see this PR's CI results. I haven't confirmed that the files marked Linux, which run 🤖 Generated with Claude Code |
1bf4c8f to
dcd5d56
Compare
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
|
||
| @test "home activation restricts the SSH directory to its owner" { | ||
| load ../test-helper/macos | ||
| require_macos "the assertion uses BSD stat -f, which GNU stat lacks" |
There was a problem hiding this comment.
P3 — This test is skipped on Linux only because it reads the mode with BSD stat -f. The activation script it tests (sshDirectoryPermissions) is plain shell, and the test already runs it under /bin/bash. So the only thing that ties it to macOS is the assertion. You could keep this security check (~/.ssh is owner-only) running in CI with a portable mode check, e.g. stat -f %Lp "$dir" 2>/dev/null || stat -c %a "$dir", or a find "$dir" -maxdepth 0 -perm 700 test. Then you wouldn't need require_macos here. This is optional: the test was not running in CI before this PR, so nothing regresses.
Review summaryBlocking findings (P0/P1): 0 I read through the change and checked:
Non-blocking
I couldn't run the suite here because nix and nvim aren't available, so please confirm the 🤖 Generated with Claude Code |
The specs relied on ~/.config/nvim pointing at the repository, so put the repository nvim directory on runtimepath, and add neovim to the dev shell. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
CI discovers tests recursively and excludes files tagged macos, so a new Bats file can no longer drop out silently. macOS-only specs get a file tag and a reason; mixed files are split into *_macos.bats, and the SSH directory mode check now uses a portable find -perm. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
dcd5d56 to
1a9fe2e
Compare
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| ESA_EDIT_FILE="$ARTICLE_FILE" \ | ||
| XDG_DATA_HOME="$NVIM_DATA_HOME" \ | ||
| nvim --headless -u "$REPO_ROOT/nvim/init.lua" \ | ||
| nvim --headless --cmd "set runtimepath^=$REPO_ROOT/nvim" -u "$REPO_ROOT/nvim/init.lua" \ |
There was a problem hiding this comment.
P2 nvim/init.lua:34 calls require("lazy"). On the Mac, lazy.nvim resolves through the Home Manager neovim wrapper (programs.neovim.plugins = [ lazy-nvim ]). The dev shell's plain shellPkgs.neovim has no plugins. The stub that setup_nvim_data_stub writes to $NVIM_DATA_HOME/nvim/lazy/lazy.nvim/lua/lazy.lua is never on runtimepath, because neither init.lua nor the test adds it.
On Linux CI, init.lua therefore aborts at line 34 with module 'lazy' not found. The test passes only because require("esa").setup() ran before the error and headless nvim still exits 0 after an init error. Everything after line 34 is never run. The spec says "the Neovim configuration saves…", but it doesn't load the configuration cleanly. A later change that moves esa setup after the lazy call would fail confusingly, and this test would not point at the cause.
Suggested fix: put the stub on the runtimepath (both invocations):
nvim --headless --cmd "set runtimepath^=$REPO_ROOT/nvim,$NVIM_DATA_HOME/nvim/lazy/lazy.nvim" -u "$REPO_ROOT/nvim/init.lua" \
Optionally, also assert that stderr has no Error detected while processing.
Review summarySwitching to discovery with Findings:
No blocking findings. When I reviewed, 🤖 Generated with Claude Code |
dprint prints plugin compile messages on a cold cache, which broke the exact output checks on CI. Separate stderr in the format specs. Put the lazy.nvim stub on runtimepath so init.lua loads fully in the Neovim specs. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
| source "$REPO_ROOT/zsh/.zshrc" | ||
| print -r -- "${SSH_AUTH_SOCK:-unset}" | ||
| ' | ||
| load test-helper/ssh_agent |
There was a problem hiding this comment.
P3 These remaining tests pass trivially on Linux CI. zsh/.zshrc only looks at SSH_AUTH_SOCK inside if [[ "$OSTYPE" == darwin* ... ]], so on ubuntu the whole socket-validation block is skipped. The "refuses a regular file", "refuses a symlink", "leaves no agent set", "keeps the inherited agent" and "preserves a forwarded agent" cases all pass whatever the -S / -O / ! -L checks do. If someone deleted the symlink check, CI would stay green. This isn't a regression, since the file wasn't in CI before. But the split says these are Linux-meaningful, and they aren't. You could move them to ssh_agent_macos.bats as well, or note that Linux CI only checks that nothing changes on non-darwin hosts. (Small detail: the PR body says 4 tests stay here, but there are 6.)
Review summaryNo blocking findings (0 × P0/P1).
What I checked: discovery with Caveat: this was a static review. bats, nvim and nix weren't available here, so I didn't run the newly discovered Linux suite. The CI run on this PR is what shows that every untagged file really passes on ubuntu. 🤖 Generated with Claude Code |
.zshrc gates the 1Password agent logic behind OSTYPE=darwin, so the cases that passed on Linux were vacuous. Keep them in one tagged file instead of splitting. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Automated review summary: no findings. Checked:
I reviewed the code but did not run the suite, so I'm relying on the CI run to confirm the newly added Linux files pass. |
概要
CI の
bats-testsジョブが bats ファイルを 3 件だけ列挙していたため，残りが CI で走っていなかった．探索ベースに変更し，新しい bats ファイルが黙って漏れないようにする．設計
bats --recursive --filter-tags '!macos' testsbats --recursive testsで全件# macOS only: <理由>と# bats file_tags=macos（bats 1.12 のファイルタグ）を置く．混在ファイルは<name>_macos.batsに分割する（tests/zsh/startup.batsの 1 件をstartup_macos.batsへ）require_macosヘルパーは採用しない（印が散らばるため）変更
.github/workflows/ci.yml:bats-testsを探索実行 +--filter-tags '!macos'に変更tests/zsh/startup.batsから macOS 専用の 1 件をstartup_macos.batsに分割．tests/zsh/ssh_agent.batsは.zshrcがOSTYPE=darwinでのみ動くため，全件が Linux では空振りになるのでファイル全体を macos タグにしたtests/ssh-agent/load_identity.bats: BSD 専用のstat -fをfind -perm 0700に置き換え，Linux でも実行tests/esa/nvim.bats: リポジトリのnvimをruntimepathに追加（~/.config/nvimが無い環境でも動く）．dev shell にneovimを追加macosタグの意味と CI での除外を追記#438 はマージ済みで，
tests/esa/format.batsは dprint で Linux 上で実行される．分類（19 ファイル + 分割 1 件）
checks.aarch64-darwinをビルドfind -permに置き換えて Linux 対応OSTYPE=darwin限定のロジックで，Linux では全件が空振りになるローカル（
bats --count）: 全 90 件，--filter-tags '!macos'で 72 件，macOS 専用 18 件．Closes #440
🤖 Generated with Claude Code