test(e2e): cover fail2ban action family - #401
vladimirrott merged 7 commits into
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. |
vladimirrott
left a comment
vladimirrott
left a comment
There was a problem hiding this comment.
Reviewed at 39eee0818f139bdb28c2dd361a78c5ef5322b0d2.
Four stories, four fail2ban actions, and the one-word production change that makes the prose gate keep working:
- r"(?:that\s+)?still\s+have\s+no(?:ne|\s+story)\b"
+ r"(?:that\s+)?still\s+(?:have|has)\s+no(?:ne|\s+story)\b"That is the part I want to call out, because it is the part that is easy to miss. Covering four of the five uncovered Debian-only actions takes the remainder from plural to singular, and the claim checker only recognised the plural form. Without the has branch the sentence "one Debian-only action still has no story" stops being a recognised gap claim and the checker reports that it cannot derive the prose at all. You spotted a coupling between a test fixture and an English verb, which is not an obvious place to look.
I recounted the numbers rather than taking them from the body:
$ git ls-tree origin/main --name-only tests/e2e/stories/ | grep -c 'story-.*\.sh'
133
$ git ls-tree p401 --name-only tests/e2e/stories/ | grep -c 'story-.*\.sh'
137
133 + 4 = 137, and 54 atomic + 83 Ubuntu = 137 as CONTRIBUTING.md now states. I also checked that 134 through 137 do not collide with anything on main: nothing matches story-13[4-7].sh there. The runner picks them up through the story-*.sh glob and every tag you used is in the closed vocabulary, so all four land in the Ubuntu set rather than the curated atomic default.
One honest limitation, which you already stated in the body and which I want on the record rather than buried: these four stories have never been executed. They need a live VM and a model, CI runs neither, and the suite size is derived from files on disk. So merging this moves four published counts on the strength of files that have not run. I am comfortable with that because you said so plainly instead of implying a run, and because the alternative is that the fail2ban family stays uncovered indefinitely. It does mean the first live VM pass after this lands is where we find out.
One optional note. Story 134 asserts [[ "$PARAMS" != "{}" ]], which is the strictest of the four. Fail2banStatus takes an optional jail, so a model that emits null rather than {} fails on shape instead of behavior. The neighbouring stories use the more forgiving jq -r '.params.x // ""' form. Not worth changing before merge, but it is the assertion most likely to produce a confusing first failure on a real VM.
Nothing blocking. Approving.
If security-audit goes red on your branch, that is RUSTSEC-2026-0285 against rustls, published today and already affecting main at 61b3a878. Mine to clear, not yours.
|
Thanks Vladimir really appreciate the review. I enjoyed working through this one properly, especially the little edge cases around the coverage claims. I’ll keep moving through #233 cluster by cluster once this lands. |
vladimirrott
left a comment
vladimirrott
left a comment
There was a problem hiding this comment.
Reviewed at b1d0f877.
Your branch moved because I updated it against main, not because you pushed. The merge carried no conflict resolution and no edit of mine:
$ git merge-tree --write-tree main 39eee081
e1dcad325699477fca0d900203a0b5ab8dd90328
$ git rev-parse b1d0f877^{tree}
e1dcad325699477fca0d900203a0b5ab8dd90328
Identical trees, so b1d0f877 is your 39eee081 with main merged in and nothing else. Everything I approved at 39eee081 still stands, and the recount holds at the new head:
$ git ls-tree -r --name-only main tests/e2e/stories | grep -c 'story-[0-9]*\.sh$'
133
$ git ls-tree -r --name-only p401 tests/e2e/stories | grep -c 'story-[0-9]*\.sh$'
137
Approving at b1d0f877. Nothing blocking, and nothing for you to do.
The CONTRIBUTING row collides with #413, and that resolution is mine
#413 edits the same table row you do:
$ git merge-tree --write-tree p401 p413
5214ac87559d94325aa343b9755b45d14ec334dc
100644 4f109c0dd927166c9476a2afd91d77b2d99eb0e6 1 CONTRIBUTING.md
100644 f36c0e9c014f28b32adf01042ed10bf6538cbf87 2 CONTRIBUTING.md
100644 cad4d8d2eb1e291b9606beedcec67c29df7ee629 3 CONTRIBUTING.md
Auto-merging CONTRIBUTING.md
CONFLICT (content): Merge conflict in CONTRIBUTING.md
Auto-merging docs/introduction.md
The two edits are independent. You take the suite from 133 stories to 137 and the uncovered Ubuntu-only count from 5 to 1. #413 takes the untouched cross-family count from 59 to 61, because it adds two firewall actions no story reaches. The resolved row keeps both:
| E2E story coverage | Real prompts, real LLM, real daemon. The suite is 137 stories: 54 atomic + 83 Ubuntu. What is left is the cross-family middle: of the action names available on both families, 61 are still untouched by any story, plus 10 Fedora-only and 1 Ubuntu-only ones. See #233 for the clustered map. | medium |
Whoever lands second resolves it, and that is me in either order. docs/introduction.md merges on its own; the collision is one row in one file. check_evidence_claims.py derives every figure in that row from the tree (scripts/check_evidence_claims.py:467-498), so a wrong resolution turns docs-and-hygiene red instead of publishing a wrong number.
What is holding the merge
One red check, and it is mine:
$ gh pr checks 401
security-audit fail 11s https://github.com/lacs-project/sysknife/actions/runs/34923133936/job/104235389870
RUSTSEC-2026-0285 against rustls, published on the 14th and sitting on main at 61b3a878. Four other open pull requests carry the lockfile bump that clears it. Once one of those lands and your branch picks it up, this board goes green and I take it. I am not putting another issue in front of you while #233 and #219 are still yours.
Four stories about banning hosts that fail SSH auth is not something you pick up from documentation. If you run Ubuntu anywhere that matters, a server you patch or a box that takes real traffic, I would like to know how SysKnife reads there. npx sysknife-setup gets it running, and the read-only side (sysknife doctor, sysknife history, sysknife audit verify) touches nothing on the host.
|
Merged as The part I want to single out is the regex. It is also the reason this pull request could not be proved by the merge gate until tonight. Its only production change lives in a Reverting your Your branch was behind twice while I worked through the queue, and both updates were mine, not yours. I checked each one carried nothing of yours: #219 stays open, and your That recording is the natural next step and it is still yours if you want it: One last thing, since your diff says you run Ubuntu with fail2ban in front of sshd. The read-only surface is worth pointing at your own box rather than a fixture: |
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.