fix(tests): freeze the EXIT-trap count for multus-install-cni-plugins.bats - #3584

…ostics_test.bats

#3548 added hack/run-kubernetes-talos-diagnostics_test.bats carrying 8 EXIT-trap
cleanups but did not register it in the frozen set of the cozyreport.bats
EXIT-trap guard, so the guard's found set no longer matches frozen and the
'Unit & controller tests' job fails on main and on every branch rebased onto it.
#3584 fixed the same class for multus-install-cni-plugins.bats but not this file.
Add the count, exactly as that guard's own comment prescribes for a new file
that arrives carrying traps. Value 8 confirmed by the guard's own fold_source +
grep and by the failing CI run.

Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>

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, and
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
failed, and it never worked even once. The inventory landed in #3567 at
15:25:20; #3195 had landed multus-install-cni-plugins.bats carrying
twelve traps forty-three seconds earlier, so replaying the old guard
against the tree at the commit that introduced it already gives
found != frozen. Main stayed red for about twenty-two and three quarter
hours. The first repair, #3584, landed already red the next morning
because #3548 had brought in run-kubernetes-talos-diagnostics_test.bats
with eight traps two minutes ahead of it, so that fix bought no green
time at all; green came only with the second repair. Neither pair of
PRs shared a line, and each was green against its own base.

A change adding 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 landed.
A file arriving with traps hit the same wall from the other side -- the
inventory did notice it, since the string it compared was built by
scanning the directory, but absorbing it meant an edit in a file nothing
in the author's diff pointed at.

Replace both lists with a declaration each file makes about itself: one
"# EXIT-TRAP DEBT: N" comment, exact rather than a ceiling. A file
carrying none must install no EXIT trap, which covers the converted
files, the files that never had one, and every file added later.
Growing or shedding a trap now fails in the file the change already
edits, so two changes that disagree about a count collide textually
instead of silently.

That collision is a steady-state property, and this change's own arrival
is the exception. A branch forked before the declaration existed has no
line to disagree with: it converts traps in its own file, merges clean,
and the count goes wrong only once both sides are on main -- verified
against a sibling branch that takes select-e2e_test.bats from fifteen
traps to zero. What the move buys even then is that the red names a file
that 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 inventory.

The declaration is read from the leading comment block, not from
anywhere in the file. 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. Reading
it there as a statement about the whole file would let an unrelated
fixture excuse a real trap, and would do it silently, since nothing in
that test's own diff looks 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 bats binary's own, so a test failing inside
`( ... )` still prints its `not ok`. hack/e2e-test-openapi.bats kills a
backgrounded kubectl proxy that way, and moving the kill to the end of
the body would leak a process holding a fixed port. Its declaration
records that rather than scheduling a conversion, and because the
ratchet is exact, removing the trap fails too -- the count protects the
construct. What the count cannot do is tell the two apart: substituting
a test-level trap for the subshell one keeps the total at 1 and stays
green, which the header states rather than leaves to be found.

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 are real handlers that scored zero, and
the left boundary matters most, since the inventory being replaced had
none and caught the semicolon form. A bare word boundary is not enough
either way: `bootstrap ` ends in `trap `, and it must keep scoring
nothing. A quoted signal counts for the same reason as the rest.

Two handlers sharing one line are reported rather than counted, since
the count is a count of lines
and the second would otherwise arrive without moving the total;
splitting the line properly needs a shell parser, a semicolon inside a
handler's own action not being a separator, and guessing wrong
undercounts -- the one direction a ratchet cannot afford.

The scan recurses, so hack/e2e-apps/*.bats is covered rather than
sitting one directory below the guard that claims the tree. It reads
.bats and nothing else, so a handler arriving through a sourced .sh
stays outside it -- hack/e2e-chainsaw/_lib/run-kubernetes.sh installs
two, each benign for its own reason rather than by design: one sits in
a function declared with `(` and so runs in a subshell, the other in a
brace function no @test calls. That boundary is stated in the header
rather than papered over.

The guard moves to hack/bats-no-exit-trap.bats: its subject is every
unit suite under hack/, not the report collector it grew up in. Its
fixture helpers assemble the trap keyword and the signal from separate
arguments, because the guard scans its own source and a fixture written
as a literal would be counted as a real trap in it.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Aleksei Sviridkin <f@lex.la>

…#3622)

<!-- Thank you for making a contribution! Here are some tips for you:
- Use Conventional Commits for the PR title: `type(scope): description`
- Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore
- Scopes are not an exhaustive list — pick the most specific scope for
the change and extend the list when a genuinely new area appears.
Examples:
- System components: dashboard, platform, operator, cilium, kube-ovn,
linstor, fluxcd, cluster-api
- Managed apps: postgres, mariadb, redis, kafka, clickhouse,
virtual-machine, kubernetes
- Development and maintenance: api, hack, tests, ci, docs, maintenance
- Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or
add a `BREAKING CHANGE:` footer
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->

## 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

<!-- REQUIRED for UI changes: attach screenshots or screen recordings
demonstrating
the visual impact of your changes. PRs with UI changes without
screenshots will not be merged. -->

### Downstream repositories

<!--
Cozystack is upstream for repositories that are NOT kept in sync with it
automatically, and nothing in CI compares the two sides, so a change
here can break them silently.

Which change forces what, and which file to touch, is the trigger map in
docs/agents/contributing.md under "Downstream Repositories". Walk it
before you tick anything.

IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM
TO FILL IN:

1. Walk the trigger map against the actual diff of this PR, file by
file. Do not decide from the PR title or from your memory of what you
changed.
2. For every repository you tick, open the follow-up PR there and link
it on the same line. A ticked box with no link claims work that does not
exist, which is worse than an empty box. Search that repository for an
open PR or issue covering it first, and link that instead of filing a
duplicate.
3. When the follow-up is out of scope, or needs a decision that is not
yours to make, open an issue in that repository instead, link that, and
say so in the PR body. Do not push a speculative PR into another
repository just to fill a line here.
4. Do not tick "No downstream repository is affected" to make the
checklist go away. If you are unsure, leave every box empty and say so
in the PR body, so a human decides.
-->

- [x] No downstream repository is affected by this change
- [ ] [cozystack/website](https://github.com/cozystack/website) -
follow-up:
- [ ]
[cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack)
- follow-up:
- [ ]
[cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack)
- follow-up:
- [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up:
- [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up:
- [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) -
follow-up:
- [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) -
follow-up:
- [ ]
[cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server)
- follow-up:
- [ ]
[cozystack/external-apps-example](https://github.com/cozystack/external-apps-example)
- follow-up:
- [ ] [cozystack/examples](https://github.com/cozystack/examples) -
follow-up:

### Release note

<!--  Write a release note:
- Explain what has changed internally and for users.
- Start with the same `type(scope):` prefix as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->

```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
```


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## 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.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->