fix(ci): make pytest tests/ fail-loud (remove || true)#28
Merged
Conversation
The `Run pytest` step swallowed all failures with `|| true`, so a failing test never failed the job. This hid a real regression (the adversarial Aristotle section, fixed in #27) and violates the testing-fail-loud-fail-fast rule. Remove the mask so test failures break CI. Co-Authored-By: Claude Opus 4.8 <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: 93d8d68253
ℹ️ 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: | | ||
| if [ -d "tests" ]; then | ||
| python -m pytest tests/ -v --tb=short || true | ||
| python -m pytest tests/ -v --tb=short |
There was a problem hiding this comment.
In this workflow the pytest tests/ step runs after only checkout and pip install, with no W2.2/layer setup. Several tests in tests/layers/test_layer_stack.py assert that ~/.ralph/layers/L0_identity.md and ~/.ralph/layers/L1_essential.md already exist, and a fresh GitHub-hosted runner will not have those home-directory files, so removing || true makes the CI test job fail for every PR before the later harness step. Please either create the required fixtures/setup in CI or fix/skip those environment-dependent tests before making this command gating.
Useful? React with 👍 / 👎.
…ug, test drift Phase 0-2 of making tests/ fail-loud in CI: - conftest.py: add isolated_home (redirects $HOME + Path.home() to a seeded temp dir), requires_tool (skip-if-tool-absent), and a shared git_repo fixture. - session-end-handoff.sh: REAL BUG fix — gc_cleanup used ((counter++)) under set -euo pipefail, which returns exit 1 when the prior value is 0, aborting the hook before its final JSON. Use counter=$((counter+1)) (always exit 0). Fixes 4 test_gc_cleanup failures AND a genuine SessionEnd defect. - Test drift (code is current, tests were stale): test_v3_agents accepts VERSION 3.0.x via regex (ralph-frontend is 3.0.1); test_worktree_utils and test_learning_pipeline accept the v2.95.1/v3.1.0 allow contract (clean exit 0 with empty stdout), per tests/HOOK_FORMAT_REFERENCE.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Redesign tests that depended on the developer's $HOME so they run against repo state under an isolated temp HOME, exercising real logic (no weakened assertions): - Repath hook invocations from ~/.claude/hooks to the repo .claude/hooks (test_hooks_functional, test_context_engine, test_aristotle_antirat_hooks, test_auto_007_hooks). - Use isolated_home + seeded settings.json so settings/state-coupled hooks emit JSON (test_hooks_optimization, test_v3_hooks, test_hooks_registration). - Build a temp Obsidian vault via scripts/setup-obsidian-vault.sh and inject HOME so vault hooks reach their approve path (test_v3_vault, test_karpathi_cycle). - Point ralph tests at the repo scripts/ralph; build L0/L1 in tmp_path (test_command_sync, test_layer_stack). - mkdir the rsync dest parent (GNU rsync exit 11 on Linux) + requires_tool guards (test_learning_pipeline). Several "approve"-format assertions were stale: hooks v3.1.1 emit the valid Stop/SessionEnd allow contract (clean exit 0, no stdout) per tests/HOOK_FORMAT_REFERENCE.md. Fixed the tests, not the hooks (verify-test-expectations). Skips used only for genuinely-absent repo resources (auto-mode-setter.sh, ~/.ralph/markers), documented. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two genuine production defects that only manifest on Linux (GNU coreutils),
masked by the CI || true and surfaced by making pytest tests/ fail-loud:
- worktree-utils.sh checkWorktreeTTL: tried `stat -f "%B"` (BSD birth time)
first, but on GNU `stat -f` means "filesystem mode" -> non-numeric multi-line
output -> arithmetic abort under set -u -> empty stdout (the JSONDecodeError).
Detect GNU vs BSD stat, fall back to mtime when birth time is 0
(ext4/overlayfs), guard against non-numeric values, clamp negative drift.
- vault-writeback.sh: the tr set '...:/-()...' contains '/-(' which GNU tr
parses as a reversed range (/=0x2F to (=0x28) and aborts; under set -euo
pipefail this killed the hook (rc=1) on Ubuntu. Move '-' to the end of the
set (literal dash on both BSD and GNU tr).
Fixes the last 4 CI failures (TestCheckWorktreeTTL x3, writeback draft wiki).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
.github/workflows/ci.ymlranpython -m pytest tests/ -v --tb=short || true. The|| trueswallowed every test failure, so the job passed even when tests failed. This masked a real regression (the adversarial Aristotle section — fixed in #27) and violates thetesting-fail-loud-fail-fastrule.git blame: introduced in
f19e2f70(2026-04-12).Change
Remove the
|| truesopytest tests/failures break CI.Expected
This will surface any genuinely-failing tests on Ubuntu/py3.12. Local runs (macOS/py3.14) show failures that appear environment-specific (hooks emitting empty output, local
~/.claude/settings.jsonpaths). The CI run on this PR is the arbiter of what actually fails and must be fixed.🤖 Generated with Claude Code