test(tests): let each bats file declare the EXIT traps it still holds - #3622

What this PR does

The ban on EXIT-trap cleanup in hack/*.bats was enforced by two lists inside hack/cozyreport.bats: one naming the files known to be clean, one freezing an exact trap count for each file that was not. Both had to be edited from whatever change moved them, and that is where the guard kept failing.

It is worth being exact about how badly, because the history is sharper than "it went stale a few times". The inventory was never correct on main for a single commit. It landed in #3567 at 15:25:20. #3195 had landed multus-install-cni-plugins.bats carrying twelve traps at 15:24:37, forty-three seconds earlier. Replaying the old guard's own logic against the tree at the very commit that introduced it already gives found != frozen. Main then stayed red for roughly twenty-two and three quarter hours. The first repair, #3584, landed already red the next morning at 10:42:43, because #3548 had brought in run-kubernetes-talos-diagnostics_test.bats with eight traps at 10:40:03, under three minutes ahead of it. That fix was correct and bought zero green time. Green arrived only with the second repair, #3602.

Neither pair of PRs shared a line, and each was green against its own base. That is the whole mechanism, and it is why a third one-line repair is not the answer.

The mechanism is structural rather than careless. A change that adds a trap to its own file had to edit a string in a suite it otherwise never touches, so two changes sharing no line still invalidated each other: each stayed green against its own base, git merged both cleanly, and the guard went red only once the second one landed. A file arriving with traps was worse still, because nothing in its author's diff pointed at that string at all. That is exactly how run-kubernetes-talos-diagnostics_test.bats got in, twice.

That contention is not in the past tense. Two open PRs are editing that one line right now, and they disagree about what it should say: #3575 adds run-kubernetes-talos-diagnostics_test.bats=8 to it, repeating a repair that has already landed, and #3441 removes select-e2e_test.bats=15 from it, because it converts that file. Neither PR is about EXIT traps. Both have to touch that string anyway, and whichever lands second is wrong until someone edits it again.

So this PR is not fixing a red main. It removes the thing that keeps making main red, which is why it is worth more than the one-line fix that is now the established habit.

One honest caveat about its own landing. The textual-conflict property is steady-state: a branch forked before the declaration exists has no line to disagree with, so it converts traps in its own file, merges clean, and the count only goes wrong once both sides are on main. I checked this against #3441, which takes select-e2e_test.bats from fifteen traps to zero. Merged after this, that file would declare fifteen and hold none. What the move buys even in that case is that the red names a file the branch already edited, the repair is one line inside it, and rebasing before merge catches it on the branch's own CI. None of those three held against the central inventory. Whoever merges this should expect one such adjustment on the conversion branches still in flight.

So this replaces both lists with a declaration each file makes about itself: one # EXIT-TRAP DEBT: N comment in its leading comment block. A file carrying no declaration must install no EXIT trap. Growing or shedding a trap now fails in the file the change already edits, so two changes that disagree about a count get a real textual conflict instead of silently invalidating each other, and a change that leaves the traps alone edits nothing.

The include list is redundant, not lost. It named the files proven clean, so that a trap reappearing in one of them would fail. Under the new rule those files carry no declaration, and a file with no declaration must hold zero traps, so a trap reappearing in any of them fails on its own, with no list to be on. Coverage widens rather than narrows: the two lists named twenty files between them, and the rule covers all fifty bats files under hack/, subdirectories included, plus the ones added tomorrow.

Rebasing this branch onto current main is the property working. Main has since gained hack/kubernetes-pre-delete-hook.bats and hack/tenant-pre-delete-hook.bats, and hack/cozyreport.bats grew by some eight hundred lines. Neither new file installs an EXIT trap, so neither needed a declaration and neither needed an edit here; the rebase took no conflict at all. Under the inventory, each arriving file was a coin toss on whether somebody had remembered the string.

To be precise about what the inventory could and could not do, since it is easy to overstate: it did notice a new file carrying traps. The string it compared was built by scanning the directory, so an arriving file appended a token and failed the comparison, which is exactly how main went red. What it could not do is let that file arrive without an edit in a foreign suite. Being seen and being absorbable are different properties, and only the second one decides whether two changes can land independently.

The declaration is pinned in both directions. Declaring N while holding N+1 fails, obviously; declaring N while holding N−1 fails too. Without that second half the number becomes a ceiling and rots upward: somebody converts half a file, the declaration stays, and the guard quietly licenses traps that were removed long ago.

It is read only from the leading comment block, under the shebang and above the first line of code. A .bats file is shell that writes shell, so the same line turns up inside a heredoc, a fixture writer or an expected-output string, where it is data belonging to one test; honouring it there would let an unrelated fixture excuse a real trap, silently, with nothing in that test's own diff looking like a declaration. A comment block is the region with no interior; stopping instead at the first @test would still read a line out of a helper's heredoc.

Not every counted handler is debt. A trap inside an explicit subshell does not replace the one the bats binary installs, so a test failing inside ( … ) still prints its not ok, checked against a test-level trap in the same file, where the TAP line vanishes. hack/e2e-test-openapi.bats kills a backgrounded kubectl proxy exactly that way, and "convert it like the others" would leak a process holding a fixed port and wedge the next run. Its declaration now records the carve-out instead of scheduling a conversion, and because the ratchet is exact in both directions, removing that trap fails too, so the count protects the construct rather than marking it for deletion. docs/agents/e2e-testing.md previously scoped this exception to Chainsaw script steps only; it now names the BATS subshell case as well.

The counting bounds the keyword and the signal the same way, at any character that cannot be part of an identifier, and matches the signal in either case. Whitespace on the right missed trap … EXIT; cd "$tmp"; whitespace on the left missed tmp=$(mktemp -d);trap … EXIT and (trap … EXIT; true); upper case missed trap … exit, which bash and dash both install. All of those are real handlers that scored zero. The left boundary is the one worth dwelling on, because the inventory being replaced had none at all and did catch the semicolon form. Getting it wrong here would have narrowed coverage while the commit claimed to widen it. A plain word boundary is not enough either: bootstrap ends in trap , and it has to keep scoring nothing, or the documented answer to a red guard (add a debt line) would buy a file a permanent licence for one real trap to silence a line that has none.

Two handlers sharing one line are reported rather than counted, because the count is a count of lines and the second would otherwise arrive free. Splitting such a line properly needs a shell parser, since a semicolon inside a handler's own quoted action is not a separator, and guessing wrong undercounts, the one direction a ratchet cannot afford.

What this does not fix. The declaration is still a loophole: a new file can write # EXIT-TRAP DEBT: 8 instead of cleaning up, and nothing here makes that impossible. What changes is that the admission is local and visible. It sits at the top of the file it excuses, in front of whoever reviews that file, instead of being a number in a neighbouring suite nobody in that review is reading. Today's loophole is the same size and invisible.

Three more limits, all stated in the guard's own header rather than left to be discovered. The scan is lexical, so a signal computed at runtime and a quoted action spanning physical lines without a backslash are both invisible. An exact count catches addition and removal but never substitution: swap the openapi file's subshell trap for a test-level one and the total stays 1. And the scan reads .bats only, so a handler arriving through a sourced .sh is outside it. hack/e2e-chainsaw/_lib/run-kubernetes.sh installs two right now, and each is benign for its own reason rather than by design: the one in cozy_capture_tenant_talos because that function is declared with ( and so runs in a subshell, the one in run_kubernetes_test because no @test calls it despite being declared with {. Three hack/*.bats source that library, and two tests in the converted file call cozy_capture_tenant_talos, so flipping a single ( to { reinstates a test-level handler in both of them with the guard green. Widening the scan to .sh would mean counting handlers that are correct in a script and wrong only in a test body, so the honest answer is that this is where the instrument stops.

Three further boundaries, recorded here so they land as known edges rather than as surprises. The old include list also failed when a file named on it disappeared from the tree; the new rule can only judge a file that is present, so a deleted converted file goes unnoticed. That is a genuinely smaller check, though its absence shows up in the diff that deletes the file. The guard's own failure messages are code lines, so they are scanned by the pattern they belong to: they pass today only because no bare EXIT or 0 happens to follow the keyword in any of them, and a rewording that introduced one would make the file demand a debt of itself. It fails loudly rather than quietly, and the fixture writers and test titles already split the keyword from the signal for this reason, but the messages do not. Finally, docs/agents/e2e-testing.md bans test-level EXIT and RETURN traps, while every mechanical guard this repo has had, the one being deleted included, matches only EXIT and 0. hack/ holds no RETURN trap today, so nothing regresses here, but half of that documented rule has never had an executor.

The guard moves out of hack/cozyreport.bats into hack/bats-no-exit-trap.bats, because its subject is every unit suite under hack/ and not the report collector it grew up in. Living inside cozyreport.bats is precisely why unrelated PRs kept converging on one line. hack/md-no-hardwrap.bats is the neighbouring precedent.

Separately, the eight EXIT traps in hack/run-kubernetes-talos-diagnostics_test.bats are converted to end-of-body cleanup rather than given a declaration. That file was written six weeks after the ban was documented, so "it predates the rule" is not available to it, and handing it a debt is literally the move that failed the last two times. The remaining files keep their declarations, since a mass conversion would collide with the branches that own them.

One note on where the prose sits. The guard's own header carries only what a reader has to keep true: why the ban exists, the two carve-outs, where the declaration lives and why there, that the count is exact in both directions, why the marker is anchored at column zero, and where the lexical scan stops. Everything above about how the inventory failed (the timestamps, the PR numbers, the arithmetic) is deliberately not in the file. It is true today and will rot on its own; a comment nobody is obliged to maintain is how a file grows a paragraph that is quietly false.

Screenshots

Downstream repositories

• No downstream repository is affected by this change
• cozystack/website - follow-up:
• cozystack/terraform-provider-cozystack - follow-up:
• cozystack/ansible-cozystack - follow-up:
• cozystack/ccp - follow-up:
• cozystack/talm - follow-up:
• cozystack/cozyhr - follow-up:
• cozystack/cozy-proxy - follow-up:
• cozystack/cozystack-telemetry-server - follow-up:
• cozystack/external-apps-example - follow-up:
• cozystack/examples - follow-up:

Release note

test(tests): each hack/*.bats file now declares its own remaining EXIT-trap debt in a `# EXIT-TRAP DEBT: N` header comment, checked by hack/bats-no-exit-trap.bats, replacing the central inventory in hack/cozyreport.bats

Summary by CodeRabbit

• Testing

  • Added automated auditing for EXIT traps across Bats end-to-end tests, including validation of tracking declarations and edge cases.
  • Improved diagnostics tests by replacing trap-based temporary-directory cleanup with explicit cleanup steps.
  • Added tracking annotations for remaining trap-related cleanup work.

• Documentation

  • Clarified when traps are permitted inside self-contained subshells and how remaining cleanup debt is reported.
  • Updated review guidance for consistent end-to-end test maintenance.