feat(v1.100 PR-22): uninstall lifecycle scaffold — detect + dry-run plan only#480
Merged
feat(v1.100 PR-22): uninstall lifecycle scaffold — detect + dry-run plan only#480
Conversation
Authorization basis: V1100_LIFECYCLE_COMPLETION_CONTRACT.md §13 frozen
2026-04-19 (all 9 open questions answered, §4.4 purge table + §10.2
verification matrix tightened per the answers).
Pinned sentence:
"PR-22 is detect + dry-run plan only. It must not flush any chain,
remove any table, disable any service, delete any file, touch any
.conf.local, or re-enable any external firewall. The dry-run output
reads like a release contract preview, not a debug dump, and
explicitly states that no phase beyond Plan is implemented yet."
Claim surface (strict PR-22 scope):
- state machine: +StateUninstallPlanning
- new package internal/installer/uninstall/ with:
- authority.go (4-state read-only classifier)
- prior.go (3-state read-only prior-authority record detector)
- plan.go (Plan struct + BuildPlan + Render)
- unit tests — no real-host mutation
- cmd/nftban-installer: --mode=uninstall dispatch → runUninstallDryRun
mirroring runUpdateDryRun shape
- flag surface: --mode=uninstall, --purge, --force-delete-operator-config,
--restore-prior-authority (all plan-only, no mutation code yet)
Authority classification — 4 states (read-only):
AuthorityNFTBan — ip nftban table + nftband active
AuthorityExternal — external firewall appears authoritative
AuthorityNone — no authoritative firewall detectable
AuthorityAmbiguous — both present / inconclusive
Prior-authority record — 3 states (read-only):
NoRecord — no artifact on disk
RecordUsable — parseable + complete
RecordIncomplete — exists but unusable (missing fields, malformed)
PR-22 reports; does NOT enforce. Enforcement of "refuse --restore when
record incomplete" is PR-24 scope.
Plan output reads like a release contract preview:
requested mode / artifact policy / current authority / target
authority / restore requested / restore authorized / prior record
state / detected external firewall / phases that would mutate
(with "NOT IMPLEMENTED in PR-22" markers pointing at PR-23..26) /
explicit scope-boundary block.
The scope-boundary block is MANDATORY — it prevents the PR-21 class
confusion where operators assume broader authorization than the
contract actually provides.
Explicit non-goals (PR-22 strict):
no kernel mutation, no service lifecycle, no filesystem writes
under /etc/nftban or /usr/lib/nftban, no .conf.local touch, no
external firewall touch, no prior-authority record WRITE, no
user/group deletion, no package-manager transactions, no
maintenance subsystem coupling.
CI gates:
G3-UN-PLAN-RENDERS structural check on contract-language output
G3-UN-NO-MUTATION structural grep on uninstall/*.go rejecting
mutation-flavored calls
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
Strict PR-22 scope: detect + dry-run plan only. No mutation.
Authorization: V1100_LIFECYCLE_COMPLETION_CONTRACT.md §13 frozen
2026-04-19 (Q1..Q9 answered; §4.4 purge table + §10.2 verification
matrix tightened).
State machine:
internal/installer/state/machine.go
+StateUninstallPlanning — the only uninstall state PR-22 ships.
Mutation-carrying states land in PR-23/25 so this doc stays
literally accurate: no phase beyond Planning exists yet.
New package internal/installer/uninstall/:
- authority.go — Classify() returns 4-state CurrentAuthority:
AuthorityNFTBan / AuthorityExternal / AuthorityNone /
AuthorityAmbiguous. READ-ONLY. Detects external firewall type
(ufw / firewalld / iptables / csf) via service + iptables-save
probes; no nft add/flush/delete, no systemctl mutation.
- prior.go — Probe() returns 3-state PriorRecordState:
PriorNoRecord / PriorRecordUsable / PriorRecordIncomplete. Reads
/var/lib/nftban/state/prior_authority.json (read-only; writer
lives in install-side PR per V1100 Q9). Schema version check +
required-field check + known-firewall-type check. Mismatched
schema → Incomplete (Q8=A: no migration unless demonstrated break).
- plan.go — Plan struct + BuildPlan (pure function) + Render (io.
Writer; contract-language output with MANDATORY scope-boundary
block per PR-22 contract) + JSON (machine consumers). Modes:
ModeRemove / ModePurge / ModePurgeForceDOC (§4.4 rows).
- uninstall_test.go — 19 tests covering:
* 4-state classifier (NFTBan / External ufw / External firewalld /
None / Ambiguous / partial-nftban note)
* 3-state probe (NoRecord / Usable / Incomplete malformed /
missing field / unknown type / schema mismatch)
* BuildPlan per mode (Remove defaults / Purge preserves
.conf.local policy language / PurgeForceDOC describes
.conf.local delete)
* Restore logic (Q4=B): requires usable record + flag; warns
on no-record or incomplete-record; §10.2 B-UN-restore-norecord
branch covered
* Render mandatory fields + scope-boundary block (all modes)
* JSON round-trip key fields
Installer dispatcher:
cmd/nftban-installer/
+flags.go — --purge, --force-delete-operator-config,
--restore-prior-authority (all plan-only in PR-22; no mutation
code consumes them). --mode=uninstall accepted; forces --dry-run
on with explicit operator-visible warning ("PR-22 ships detect +
dry-run plan only").
+main.go — --mode=uninstall dispatch to runUninstallDryRun.
+uninstall_dryrun.go — orchestrator mirroring runUpdateDryRun
shape (Detect phase via Classify+Probe → Plan phase via
BuildPlan → Render stdout + persist plan JSON to state-dir →
transition to StateUninstallPlanning → exit 0).
CI gate (new, blocking):
.github/workflows/ci-uninstall-canonization.yml
Matrix Ubuntu 24.04 + AlmaLinux 9.
G3-UN-NO-MUTATION: structural grep on internal/installer/uninstall/
+ cmd/nftban-installer/uninstall_dryrun.go rejecting 20 mutation-
flavored Go patterns (nft add/flush/delete, systemctl 8 verbs,
ufw/iptables/csf, apt-get/dnf remove, WriteFileAtomic, os.Remove,
.conf.local touch). Runs BEFORE unit tests for fast reviewer
signal.
G3-UN-PLAN-RENDERS: end-to-end --dry-run produces all 11 mandatory
contract-language elements; run WITHOUT --dry-run must still
not mutate files outside state-dir.
Explicit non-goals (all honored):
no kernel mutation, no service lifecycle, no filesystem writes
under /etc/nftban or /usr/lib/nftban, no .conf.local touch (read
or write), no external firewall touch, no prior-authority record
WRITE, no user/group deletion, no package-manager transactions,
no maintenance subsystem coupling.
Deferred to later PRs:
PR-23: authority release (Switch phase)
PR-24: CSF/external firewall restoration enforcement
PR-25: purge vs remove artifact execution
PR-26: G3-UN-VERIFY (point-of-no-return proof gate)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the five concerns flagged in the contract-level audit of the
PR-22 scaffold:
Audit C — auto-elevation to dry-run could teach bad mental model
Strengthened the warning: stderr banner now uses a 70-column box
that is impossible to skim-past. Message explicitly warns that PR-23
will change this behaviour and tells operators NOT to build habits
around the PR-22 safety-by-default.
Added a regression note in internal/installer/uninstall/contract.md
documenting the PR-23 acceptance criterion: either delete the
auto-elevation, OR replace with explicit refusal requiring
--dry-run vs --confirm-mutation choice.
Audit D.1 — classifier must prefer ambiguous over false certainty
Found + fixed a real bug: partial nftban (table without daemon, or
daemon without table) + external firewall = AuthorityAmbiguous, not
AuthorityExternal. Previous logic would have returned External
silently, giving later PR-24 mutation decisions a poisoned input.
New test TestClassify_PartialNFTBan_WithExternal_IsAmbiguous locks
the behaviour.
Audit D.5 — text/JSON renderer drift
Added two first-class fields to the Plan struct:
ScopeBoundary string
NoMutationPerformed bool
Both are populated by BuildPlan and appear in BOTH Render() human
output AND JSON serialization. Machine consumers now see the same
"no mutation performed" guarantee the human reader sees.
Audit F — critical semantic edge case proof
New test TestBuildPlan_Restore_IncompleteRecord_SemanticEdge_Audit_F
proves that with --restore-prior-authority AND PriorRecordIncomplete,
the plan satisfies ALL THREE properties:
1. does NOT promise restoration (TargetAuthority is not prior fw)
2. does NOT silently downgrade (RestoreRequested=true + warning
both surface the operator's intent)
3. explicitly reports restore requested but not authorized
Rendered output also tested — operator audit at read-time catches
the edge without reading code.
PR-22 scope is still detect + plan only. No mutation added.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Policy Gates "Suppression comment audit" step rejects //nolint: directives. Repo convention is //lint:ignore <CHECK> <reason>. One-line fix in uninstall_dryrun.go plan-persist site. No behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…pass
Issue 1 — TestRender_HasMandatoryFieldsAndScopeBoundary expected a
cross-line substring ("later PRs in the v1.100 track must land first").
Render wraps the sentence across two lines, so strings.Contains cannot
match. Changed the needle to a single-line fragment that's present
verbatim in the rendered output.
Fixes: Build & Test + Go Security Analysis (both failed on the same
unit-test regression).
Issue 2 — G3-UN-NO-MUTATION structural grep flagged the string
'.conf.local' in PR-22's OWN contract-language plan renderer. The
grep pattern was too aggressive: plan.go and contract.md legitimately
mention .conf.local when describing the §4.4 artifact policy. The
mutation danger is WRITE/DELETE operations, which the existing
os.Remove + os.RemoveAll + WriteFileAtomic patterns already block
regardless of which file they target. Removed the raw 'conf\.local'
pattern with an inline comment explaining why.
Fixes: Uninstall Canonization (both distros).
No behaviour change in PR-22 scope. Just unblocking CI so the audit
response can be evaluated.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
itcmsgr
added a commit
that referenced
this pull request
Apr 19, 2026
Independent audit of merged PR #480 found three concrete violations of the PR-22 contract Stop Condition ("no writes under /var/lib/nftban/"): 1. os.WriteFile(<stateDir>/uninstall_plan.json, …) on every dry-run 2. sf.Transition(StateUninstallPlanning, …) persisted install_state 3. unconditional writeHistory() recorded a successful plan preview as "install_fail" in update-history.json — silently poisoning every operator dashboard that alerts on install_fail Plus one classifier false-negative: 4. partial nftban (table OR daemon, not both) WITHOUT external firewall fell through to AuthorityNone — PR-23 release logic would skip kernel cleanup of the orphan table Plus CI blind spot: 5. G3-UN-NO-MUTATION grep missed os.WriteFile/Create/MkdirAll/Rename; snapshot step did not cover /var/lib/nftban/ at all This PR is BOUNDARY REPAIR ONLY. It does not add uninstall mutation, does not change purge/remove semantics, does not expand prior-authority logic, does not begin PR-23. Scope locked to R1-R5 from the repair contract seed. R1 (uninstall_dryrun.go): remove os.WriteFile + sf.Transition. Plan renders to stdout only. Option B — no installer-state persistence during dry-run. R2 (main.go): guard writeHistory with cfg.mode != "uninstall" so a successful plan preview is not recorded as install_fail. R3 (authority.go): explicit case nftbanPartial && !extPresent → AuthorityAmbiguous with diagnostic note; tighten the AuthorityNone case with !nftbanPartial. Regression test added (TestClassify_PartialNFTBan_NoExternal_IsAmbiguous) plus symmetric daemon-up-no-table case. R4 (uninstall_dryrun_test.go): new falsifiable purity test calls runUninstallDryRun with MockExecutor + real tempdir and asserts: - zero executor writes (WriteFileAtomic) - zero directory creates - zero mutation commands (nft add/flush/delete, systemctl lifecycle, ufw/firewall-cmd, package-manager removal) - zero files on disk under temp stateDir Two tests: no-authority host and ambiguous host. R5 (ci-uninstall-canonization.yml): - extend grep patterns: os.WriteFile/Create/MkdirAll/Mkdir/Rename, sf.Transition, nft create, apt-get purge, dnf erase - widen E2E snapshot to include /var/lib/nftban/ (was only /etc/nftban + /usr/lib/nftban + /usr/sbin/nftban*) - new G3-UN-HISTORY-PURITY gate: seeds realistic update-history.json, runs dry-run twice (explicit + implicit), asserts byte-identical hash — catches the exact class that escaped PR #480 - run the new orchestrator purity test in CI Acceptance criteria (all pass by construction): - uninstall dry-run no longer writes misleading plan/history artifacts under /var/lib/nftban/ - successful uninstall dry-run does not create install_fail - partial nftban without external returns AuthorityAmbiguous - purity test exists and would fail on regression - CI would fail on the exact write-pattern class that escaped in PR #480 (os.WriteFile + history pollution + snapshot blind spot) - no uninstall mutation capability added - scope remains boundary repair only Depends on: 547aa08 (PR #480) Refs: V1100_LIFECYCLE_COMPLETION_CONTRACT.md §13 (frozen 2026-04-19) Refs: internal/installer/uninstall/contract.md Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merged
13 tasks
itcmsgr
added a commit
that referenced
this pull request
Apr 19, 2026
… history + classifier fix (#481) Boundary-repair PR for PR #480 (PR-22 uninstall scaffold). Closes three contract violations found by independent audit: direct filesystem writes under /var/lib/nftban/ during dry-run, install_state persistence during dry-run, and unconditional history writes that recorded successful plan previews as install_fail. Plus classifier false-negative: partial nftban (table OR daemon, not both) WITHOUT external firewall fell through to AuthorityNone, hiding orphan kernel state from later release logic. Plus CI blind spot: grep missed direct os.* writers; snapshot step did not cover /var/lib/nftban/. Scope-locked repair — no uninstall mutation added, no purge/remove semantics changed, no PR-23 work begun. Extended grep catches the exact class that escaped. New history-purity gate proves update-history.json untouched. Filesystem snapshot extended to /var/lib/nftban/. New orchestrator-level purity test asserts zero writes / zero mutation commands. Per merge approval: auto-elevation is still temporary PR-22 compatibility and must be re-decided before PR-23 mutation lands (documented in internal/installer/uninstall/contract.md). Refs: V1100_LIFECYCLE_COMPLETION_CONTRACT.md §13 (frozen 2026-04-19)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First v1.100 PR. Detect + dry-run plan only. No mutation.
Authorization
V1100_LIFECYCLE_COMPLETION_CONTRACT.md§13 frozen 2026-04-19 — all 9 open questions answered, §4.4 purge table + §10.2 verification matrix tightened per the answers.Pinned sentence
Scope locks (per reviewer directive)
Current state
Seed commit only (
003ea26a) — the contract doc lands first, zero behaviour change. Implementation commits land next (state constants + uninstall package + dispatcher + tests + CI gates).Explicit non-goals
/etc/nftban/,/usr/lib/nftban/,/usr/sbin/nftban*.conf.localtouch (read-or-write)Deferred to later PRs
Implementation plan (commit-by-commit)
internal/installer/uninstall/contract.md(this commit)StateUninstallPlanningtointernal/installer/state/machine.gorunUninstallDryRunmirroringrunUpdateDryRunshape; no-mutation call-path auditStatus: Draft — un-drafts after CI green and scope-boundary language confirmed.
🤖 Generated with Claude Code