18 sites in the natmap_docker suite have a failure branch that cannot fail

[FAZuH]

Found while closing #56. This is the same defect, 18 more times, and it is the most serious thing the audit has found about the test suite.

The mechanism

tests/natmap_docker.rs uses this shape:

grep -q '<pattern>' && (echo 'FAIL: ...' >&2 && exit 1) || echo 'PASS'

The exit 1 sits inside a subshell — the parentheses create one. exit 1 therefore kills the subshell, not the script. Control returns to the ||, echo 'PASS' runs, and the script exits 0. The harness sees PASS.

Proven:

$ sh -c 'false && (echo "FAIL-branch" >&2; exit 1) || echo PASS; echo "final=$?"'
PASS
final=0

Every test using this shape reports PASS when the condition it checks is violated. The &&/|| chain looks like a guard and behaves like a no-op.

Sites

18 in tests/natmap_docker.rs: lines 201, 216, 273, 293, 313, 333, 352, 371, 382, 405, 407, 462, 479, 496, 515, 537, 739, 821.

Line 739 is docker_mapping_remove_cleans_up_loopback_masquerade, the third hairpin test, so it is both target-blind and vacuous on its removal check.

The second, quieter half

Even with the subshell unwrapped, a bare grep -q absence check is satisfiable by a broken environment: if iptables is missing, the input is empty, grep exits 1, and the rule reads as absent. The test then passes for the wrong reason. #56 fixed this for the loopback negative test with a control grep: assert the thing that should be there is visible, so a broken command fails before the absence check can succeed.

What to build

Per site, both halves:

1. Unwrap the subshell: if <condition>; then echo 'FAIL' >&2; <diagnostics>; exit 1; fi
2. Add a control assertion where the check is an absence — something that must be present, so a missing binary or a failed command cannot masquerade as "the rule is gone"
3. Where the check is a presence, assert the jump target, not just the match fields. That is the The loopback hairpin Docker test greps for the rule but never checks the MASQUERADE target #56 lesson: a rule whose target changed still matches its match fields

Acceptance criteria

• rg -c 'exit 1\) \|\| echo' tests/natmap_docker.rs is 0
• Every one of the 18 sites is listed individually with before and after, so a site cannot be quietly skipped
• A demonstration that each rewritten assertion can fail: inject the violating condition and show the test goes red. Do it for a representative sample covering all three shapes present in the file (presence-by-grep, absence-by-grep, and the compound forms), and say which lines each sample covers
• The && (…) || shape does not survive anywhere else in tests/, checked across tests/auto_discover/ and tests/natmap_docker.rs
• cargo test -p lab-ops --all-features --test natmap_docker passes, currently 35
• No assertion is weakened to make a site pass

Why this was missed

#44 removed a tautological assert_pass helper from tests/auto_discover/, where the tautology was in Rust code. The same tautology exists here in shell strings, where no Rust-side audit reaches. rg for the pattern is the whole check.

[FAZuH]

Landed on v0.2 as d8dc1a6, merged 83218dc. Closed by hand: a closing keyword only fires from commits reaching the default branch.

The mechanism, proven

$ sh -c 'false && (echo "FAIL-branch" >&2; exit 1) || echo PASS; echo "final=$?"'
PASS
final=0

The exit 1 is inside a subshell. It kills the subshell, control returns to the ||, echo 'PASS' runs, the script exits 0.

All 18 reported PASS when the condition they checked was violated. This is the worst defect the audit found about the suite: every other measurement in it is taken against a baseline these 18 were quietly corrupting.

All 18 now use a top-level if … then …; exit 1; fi. rg -c 'exit 1\) \|\| echo' tests/ is 0 across the whole tree.

Four latent bugs the vacuous guards were hiding

1. The trailing || echo 'PASS' swallowed every earlier failure, not just the site's own guard — the whole && chain. clear_removes_dnat printed FAIL: DNAT rule not installed and the trailing || reset the status to 0. That is why the file looked green while printing FAIL.
2. Three sites greped the wrong chain. Static dnat/hairpin install into nat PREROUTING (iptables.rs:257); only docker mappings use nat NATMAP (iptables.rs:113). The controls asserted on NATMAP and could never match.
3. stop_job("%1", …) never killed anything. %1 is bash job-control syntax; the container's sh is dash, where it resolves to nothing. The kill failed, was swallowed, and kill -0 %1 also failed so the liveness check passed. All three callers silently no-op'd, including clear_resets_state, which therefore never tested a restart.
4. SIGTERM leaves the socket file behind, so WAIT_SOCKET returned instantly on the dead daemon's socket and the flush had not run. Confirmed by hand that the product flushes correctly; the test was reading the ruleset too early.

Also: docker rm --name portainer-ce --port 32771 was not a valid invocation at all, and policy_route_install_remove was missing the ip route add … scope link its sibling has, so its route add failed with "Nexthop has invalid gateway".

The second half, which is easy to miss

Unwrapping the subshell is half of it. A bare grep -q absence check passes when iptables is missing: empty input, grep exits 1, the rule reads as "absent". Every absence site therefore also gained a control assertion — something that must be present, checked first.

Six injections, one per shape, all red

Re-adding a rule after flush (8 flush sites) · re-dnat after clear (5 sites) · re-adding the loopback MASQUERADE (L800) · breaking the iptables read feeding the control (L403) · skipping the install a control requires · re-adding an ip rule after --delete. Sample 4 is the quieter half: with the read broken the old grep reported "absent" and passed.

A consequence worth stating plainly

These tests can now fail where they previously could not. The old shape swallowed socket-timeout and unreadable-chain failures along with everything else, so a green natmap_docker was partly an artifact. One socket-timeout failure was observed at loadavg 5.34 during this work; the same tests on baseline HEAD were 0/20, as they are after. That is not a regression — it is the point, and the target's green is worth more than it was.

A doc comment this change made false

stop_job's doc still told the reader to pass %1, the exact construct that silently no-op'd for three callers. It now says pass $DAEMON_PID and names the dash trap. Fixed in the same change rather than left behind, which is the AGENTS.md lesson from earlier in this batch.

Verification

natmap_docker: 35 passed, 0 failed, exit 0. ./dev.sh test: 427 passed, 0 failed, 10 targets, exit 0, at loadavg 1.50/2.21 before and 2.81/2.49 after.

Shapes: exit 1) || echo 18 → 0; top-level if … fi 2 → 20; the 4 && echo 'PASS' sites were already sound and untouched. crates/ and src/ byte-identical to HEAD.

Not closed here

The two -d 127.0.0.1/32 rules for a loopback mapping — NATMAP DNAT and OUTPUT DNAT — still have no coverage. #56's argument holds: the only delta for a loopback mapping is the extra MASQUERADE, which L800 now scopes by comment.