Skip to content

docs(agents): say which runner the EXIT-trap failure mode needs - #3779

Open
myasnikovdaniil wants to merge 1 commit into
mainfrom
docs/e2e-s3-cozytest-subshell
Open

docs(agents): say which runner the EXIT-trap failure mode needs#3779
myasnikovdaniil wants to merge 1 commit into
mainfrom
docs/e2e-s3-cozytest-subshell

Conversation

@myasnikovdaniil

@myasnikovdaniil myasnikovdaniil commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

§3 bans test level EXIT traps because such trap replaces the handler bats binary installs, so a failing test prints no TAP line at all and grep for not ok reads a green suite. That is true, and it does not happen in CI.

hack/cozytest.sh runs each @test inside explicit subshell and reads the status out of band on the next line, so a trap in a test body fires when that subshell exits and takes nothing with it. Real bats binary is not called anywhere in this tree, make bats-unit-tests and every recipe in packages/core/testing/Makefile go through cozytest. Disappearing not ok needs someone running bats by hand on a file.

Worth writing down because the rule already cost four PRs and three successive mechanisms, and the missing note reads as unpaid debt, 68 declared traps that a reader assumes CI is one refactor away from needing. The bullet also records that two of the covered files resist a mechanical conversion, one body cd's into its scratch dir and never returns so an end of body cleanup would delete its own cwd, another has its last command deciding the verdict. Next person to try that should not find it halfway through.

Summary by CodeRabbit

  • Documentation
    • Clarified that the end-to-end test runner preserves failing test results when executing BATS tests in subshells.
    • Explained that disappearing failure results can occur only when invoking the BATS binary manually.
    • Expanded guidance on cleanup behavior, trap-count checks, sourced handlers, and runner-specific cases where removing exit traps may change results.

@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b49d1500-156a-4f33-b968-cd97eeb48066

📥 Commits

Reviewing files that changed from the base of the PR and between 04f4dcf and c3b80ea.

📒 Files selected for processing (1)
  • docs/agents/e2e-testing.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • docs/agents/e2e-testing.md

📝 Walkthrough

Walkthrough

The E2E testing guide clarifies hack/cozytest.sh subshell behavior, manual Bats verdict handling, EXIT-trap count coverage, sourced-handler exceptions, and cleanup effects.

Changes

BATS execution guidance

Layer / File(s) Summary
Runner behavior and trap guidance
docs/agents/e2e-testing.md
The guide distinguishes hack/cozytest.sh subshell execution from manual Bats execution. It documents trap-count scope, sourced-handler exceptions, and cleanup cases that can change test behavior.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to c3b80

This is a localized documentation clarification with no production behavior change; no actionable merge-blocking risk remains beyond normal checks and review.

Possibly related issues

Possibly related PRs

Suggested reviewers: lexfrei

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the documentation change and specifies that the EXIT-trap failure mode depends on the test runner.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch docs/e2e-s3-cozytest-subshell

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
docs/agents/e2e-testing.md (1)

48-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Narrow the repository-wide bats claim.

The repository contains #!/usr/bin/env bats shebangs and documents manual bats commands. State that the automated CI and Makefile paths do not invoke bats directly.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/agents/e2e-testing.md` at line 48, Update the documentation’s
repository-wide runner claim to acknowledge the existing `#!/usr/bin/env bats`
shebangs and documented manual `bats` commands, while explicitly stating that
automated CI and Makefile paths invoke `hack/cozytest.sh` rather than `bats`
directly. Keep the distinction between manual local execution and automated
paths clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@docs/agents/e2e-testing.md`:
- Line 48: Update the documentation’s repository-wide runner claim to
acknowledge the existing `#!/usr/bin/env bats` shebangs and documented manual
`bats` commands, while explicitly stating that automated CI and Makefile paths
invoke `hack/cozytest.sh` rather than `bats` directly. Keep the distinction
between manual local execution and automated paths clear.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8b438ba2-f6bd-41f3-b48b-bf7967288522

📥 Commits

Reviewing files that changed from the base of the PR and between e944619 and 04f4dcf.

📒 Files selected for processing (1)
  • docs/agents/e2e-testing.md

@github-actions github-actions Bot added area/ai Issues or PRs related to AI agent guides, AGENTS.md, docs/agents/ kind/documentation Categorizes issue or PR as related to documentation size/XS This PR changes 0-9 lines, ignoring generated files labels Aug 12, 2026
§3 bans test-level EXIT traps because such a trap displaces the handler
the bats binary installs, so a failing test prints no TAP line and a
grep for `not ok` reads a green suite. That is true, and it is
unreachable in CI.

hack/cozytest.sh runs each @test inside an explicit subshell and reads
the status out of band on the next line, so a trap in a test body fires
when that subshell exits and takes nothing with it. The real bats
binary is invoked nowhere in the tree -- make bats-unit-tests and every
recipe in packages/core/testing/Makefile go through cozytest.sh. The
disappearing `not ok` needs someone running bats by hand.

Worth writing down because the rule has cost four PRs and three
successive mechanisms, and the omission reads as an unpaid backlog: 68
declared traps that a reader assumes CI is one refactor away from
needing. It is a review aid over a hazard outside CI. The note also
records that two of the covered files resist a mechanical conversion,
so the next person to try does not discover it halfway.

No other line of the document changes.

Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ai Issues or PRs related to AI agent guides, AGENTS.md, docs/agents/ kind/documentation Categorizes issue or PR as related to documentation size/XS This PR changes 0-9 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant