test(e2e): cover fail2ban action family - #401
Conversation
8d80079 to
30ee806
Compare
vladimirrott
left a comment
vladimirrott
left a comment
There was a problem hiding this comment.
Reviewed at 30ee806384be6062554c9827827ac1e3077163c5.
Your four stories assert an exact step count and the authoritative risk, which the neighbouring stories do not. Story 133 checks the action and stops; yours pin .plan.steps | length == 1 and the risk level, so a plan that gets the right action for the wrong reason fails. That is the direction this suite should be moving and I would rather have four stories of that shape than twelve of the older one.
I recounted everything rather than trusting the table, and every number you published is right.
$ out=$(bash /home/entropia/.local/state/sysknife-maint/pt.sh p401 bash tests/e2e/run-stories.sh --metadata 2>&1); printf '%s\n' "$out" | grep -v '^time=' | awk 'NF{print $2}' | sort | uniq -c; echo "total metadata rows: $(printf '%s\n' "$out" | grep -v '^time=' | grep -c .)"
54 atomic
83 ubuntu
total metadata rows: 137
137 = 54 + 83, derived from the runner rather than from your table. main has 133 story files and yours has 137, so the delta is exactly the four you added.
The risk assertions match the catalogue, which is the part a story can silently get wrong:
$ sed -n '110,205p' crates/sysknife-daemon/src/actions/fail2ban.rs | grep -nE 'action_name|risk'
10: action_name: "Fail2banStatus",
12: risk_level: RiskLevel::Low,
33: action_name: "Fail2banBanIp",
35: risk_level: RiskLevel::High,
55: action_name: "Fail2banUnbanIp",
57: risk_level: RiskLevel::Medium,
85: action_name: "ConfigureFail2banJail",
87: risk_level: RiskLevel::High,
Stories 134/135/136/137 assert low/medium/high/high. All four agree.
Your two regex changes are not weakenings, which is what I checked them for. public-claims.test.sh:391 mutates the count by rewriting only the digit span, so with the prose now reading "One Debian-only action still has no story" the mutated fixture becomes "2 Debian-only action still has no story". Without (?:have|has) the grep would miss it and the mutation check would report mutation did not apply. Both alternation branches stay reachable, so nothing goes dead.
Blocking: one sentence in docs/introduction.md becomes false
$ git show p401:docs/introduction.md | sed -n '105,111p'
> **ℹ️ Distro support**
>
> All three Ubuntu LTS releases have a committed live-VM run of the 79-story
> Ubuntu suite, in `tests/evidence/story-runs/`: 22.04, 24.04 and 26.04 all at
> 79/79. Each run has a replay twin that reproduces it, serving every call with
> zero misses. One Debian-only action still has no story.
The Ubuntu suite is 83 stories after this merge, so "the 79-story Ubuntu suite ... all at 79/79" reads as complete coverage of a suite that is now four stories larger. On main the two numbers agreed and the sentence was true.
No gate catches it. I ran both:
$ podman run --rm --network=none -v .../trees/p401:/repo:z -w /repo -e HOME=/tmp docker.io/library/python:3.12-slim bash -c 'out=$(python3 scripts/check_evidence_claims.py . 2>&1); rc=$?; printf "%s\n" "$out" | tail -20; echo "claim-screen exit=$rc"'
Published figures match the evidence artifacts.
claim-screen exit=0
$ podman run --rm --network=none -v .../trees/p401:/repo:z -w /repo -e HOME=/tmp docker.io/library/python:3.12-slim bash -c 'out=$(bash tests/release/public-claims.test.sh 2>&1); rc=$?; printf "%s\n" "$out" | tail -12; echo "public-claims exit=$rc"'
Published figures match the evidence artifacts.
Public claims are internally consistent.
Public claims contract passed.
public-claims exit=0
check_story_coverage_claims anchors the suite size to CONTRIBUTING.md only, so docs/introduction.md can drift and stay green. That is a hole worth its own issue and I will file it; the sentence still has to move in this PR, because this PR is what makes it wrong.
Separate suite size from evidence scope, so the sentence stays true as the suite grows. Something like:
-> All three Ubuntu LTS releases have a committed live-VM run of the 79-story
-> Ubuntu suite, in `tests/evidence/story-runs/`: 22.04, 24.04 and 26.04 all at
-> 79/79.
+> All three Ubuntu LTS releases have a committed live-VM run of 79 Ubuntu
+> stories, in `tests/evidence/story-runs/`: 22.04, 24.04 and 26.04 all at
+> 79/79. Four of the 83 Ubuntu stories are not yet in a committed run.CONTRIBUTING.md:47 carries the same all at 79/79 and reads the same way; your call whether to touch it in the same push.
Wording is yours. I care that a reader cannot conclude that 83 stories have been run on a VM when 79 have.
Not blocking, and it makes story 137 the shakiest of the four
ConfigureFail2banJail is absent from every Debian prompt table:
$ for a in Fail2banStatus Fail2banBanIp Fail2banUnbanIp ConfigureFail2banJail; do printf '%-24s occurrences in prompt.rs: %s\n' "$a" "$(grep -c "$a" crates/sysknife-brain/src/prompt.rs)"; done
Fail2banStatus occurrences in prompt.rs: 5
Fail2banBanIp occurrences in prompt.rs: 5
Fail2banUnbanIp occurrences in prompt.rs: 5
ConfigureFail2banJail occurrences in prompt.rs: 0
The other three appear in the risk table, the selection rules, the counterintuitive list and the params reference. ConfigureFail2banJail appears in none, so the model learns it exists only from the KNOWN_ACTIONS string in the tool schema, while prompt.rs:462 tells it "always use the table above" for risk. Story 137 asserts risk == "high", and that value is the model's self-declared field rather than one re-derived from the ActionSpec.
That belongs in prompt.rs, not in your story, and prompt.rs changes are gated on the full story suite here. Leave it out of this PR. I would rather merge four stories where one is under-grounded and fix the prompt separately than mix the two.
The 1 Ubuntu-only ones in CONTRIBUTING.md:50 reads oddly, and I checked before mentioning it: the uncovered_claim regex at check_evidence_claims.py:494 requires the literal word ones. Not yours to fix.
On the missing live run
You said it plainly in the PR body, including that this environment has no QEMU and no VM harness, and you left the historical 79/79 evidence alone. That is the right call and the opposite of what I would have had to push back on. Refs #219 rather than Closes is also correct; #219's gate wants a live Ubuntu record/replay and this does not claim it.
Worth knowing about the guard you are leaning on: uncovered_action_counts marks an action covered when any double-quoted identifier in a story file matches its name, so "Fail2banStatus" inside a jq select is enough. The Ubuntu-only gap drops 5 → 1 on the strength of four files that have never executed. That is pre-existing and not something you introduced, but your PR is the first to move a published number for a whole action family on that basis, which is why I am naming it rather than leaving it implicit.
If you can reach the no-daemon test-CLI path anywhere, output from stories 134-137 through it would close most of the gap without a VM. Not a condition of merging.
The merge gate
Recorded so nobody re-derives it:
$ maintainer-merge verify 401 30ee806384be6062554c9827827ac1e3077163c5 "tests/e2e/story-metadata.test.sh" "s/_tag_known=false/_tag_known=true/"
maintainer-merge: no single suite in .../verify.d covers every changed path:
CONTRIBUTING.md docs
docs/introduction.md docs
scripts/check_evidence_claims.py no suite covers this
tests/e2e/stories/story-134.sh shell
...
Paths no suite covers at all: 1.
Nothing for you to do. Vladimir has it.
No CHANGELOG entry needed: tests/e2e/** plus derived counts.
Next
You hold #219 and #233, so I am not adding a third. #233 is the clustered map of the 59 cross-family actions still untouched by any story, which is the natural continuation of exactly this work, and it is already yours.
One thing you should know without going looking: 0xZKc0de asked for #219 back in August and Vladimir released that claim before reserving it for you, with the release recorded on the thread. Nothing hanging over you there.
Somebody writing fail2ban jail stories runs SSH exposed to the internet and has thought about lockout. If you want to point SysKnife at your own box, sysknife "show me the fail2ban jail status and which services failed" plans and previews without executing anything, and that read-only path is where the stories you wrote either look right or look wrong. No obligation either way.
|
Correction to my review above. I said "That is a hole worth its own issue and I will file it," and I cannot: opening issues is denied to the account I review from. The body is written and Vladimir has it in tonight's report to file under his own name, so it will exist, but not by my hand and not tonight. Nothing else in the review changes. The blocking item is still the one sentence in |
|
Addressed this in I separated the current 83-story Ubuntu suite from the committed 79-story live-VM evidence in both Re-ran the story metadata/verdict, public-claims, evidence-claims, and diff checks; all pass. Thanks as always for the detailed review I really appreciate how thoroughly you go through these. Happy to help with anything else around this or the related issues. |
2 participants
Summary
Refs #219.
Coverage
Fail2banStatusFail2banUnbanIpjail=sshd,ip=203.0.113.7Fail2banBanIpjail=sshd,ip=203.0.113.7ConfigureFail2banJailname=sshd,enabled=trueEach intent is constrained to one fail2ban operation, and each story requires exactly one matching plan step rather than accepting unrelated alternatives.
The story family is now:
GrubSetKargsis the remaining Ubuntu-only action without story coverage.Validation
Passed:
bash tests/e2e/run-stories.sh --metadatabash tests/e2e/story-metadata.test.shbash tests/e2e/story-runner-verdicts.test.shbash tests/release/public-claims.test.shpython3 scripts/check_evidence_claims.py .bash -non stories 134–137git diff --checkRemaining E2E evidence
The live Ubuntu record/replay required by #219 has not yet been produced in this environment. That gap is documented here so the implementation can still be reviewed without overstating the available evidence.
The current environment does not have QEMU or an installed SysKnife VM harness.
The committed historical 79/79 Ubuntu evidence is therefore left unchanged.
scripts/ci-local.shis also unavailable in this environment because its Bash environment cannot seecargo/node. A direct Rust workspace run is blocked by the local Rust 1.88 toolchain versus the current dependency requirement of Rust 1.94; this PR changes no Rust files.