firewall: T9205: fix ruff findings in conf_mode/op_mode firewall.py

[ruben-herold]

Summary

• Split out of firewall: T9160: add apply-path for deriving group membership from config #5374 (T9160) at sever-sever's request: that PR bundled unrelated cleanup with the apply-path feature because it happened to touch the same files.
• Fixes bare except:, extraneous f-string prefixes on ConfigError()/text calls without placeholders in src/conf_mode/firewall.py, the same except: pattern in src/op_mode/firewall.py, and removes 4 unused-but-harmless locals in smoketest/scripts/cli/test_firewall.py (this is the ruff cleanup itself; firewall: T9160: add apply-path for deriving group membership from config #5374 separately re-added the same locals to undo the unrelated cleanup that had snuck into that branch).
• Narrowed the except Exception: in src/op_mode/firewall.py further to the specific exceptions each call site can actually raise (OSError from cmdl(), json.JSONDecodeError from json.loads()), per review feedback.
• Minor, intentional behavior change: the original bare except: blocks silently swallowed everything, including KeyboardInterrupt/SystemExit/unrelated bugs. They now only catch the documented failure modes of the calls inside the try, so unexpected errors propagate instead of being hidden.

Task: https://vyos.dev/T9205

Test plan

• ruff check src/conf_mode/firewall.py src/op_mode/firewall.py passes clean
• python3 -m py_compile on all three touched files

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 762dc59d-b973-4e31-8877-583223d6aa72

📥 Commits

Reviewing files that changed from the base of the PR and between 6c80398 and edc1a80.

📒 Files selected for processing (1)

• src/op_mode/firewall.py

🔗 Linked repositories identified

CodeRabbit considers these linked repositories for cross-repo context during reviews:

• ansible/ansible (manual)

🚧 Files skipped from review as they are similar to previous changes (1)

• src/op_mode/firewall.py

📜 Recent review details

⏰ Context from checks skipped due to timeout. (2)

• GitHub Check: Mergify Merge Protections
• GitHub Check: Summary

🧰 Additional context used

🔍 Remote MCP vyos.dev

Relevant review context

• T9205 — “firewall: fix pre-existing ruff findings in conf_mode/op_mode firewall.py” is In progress, normal priority, cosmetic, and compatible with VyOS 1.5.1. Its description requests narrowing bare exception handlers, removing unnecessary f-string prefixes, and deleting unused smoketest locals. It explicitly says “No behavior change.” The PR’s exception narrowing therefore warrants review because it intentionally changes handling of unexpected exceptions.
• T9205 was split from T9160 — “Config-path-derived group/prefix-list membership (analogous to Junos apply-path)”, an unrelated in-progress feature task.
• T9205’s task description says “3 unused locals”, while the PR context says four were removed. This count discrepancy should be verified against the actual diff.
• The task contains a comment linking the change to PR #5398.

---

📝 Walkthrough

Summary by CodeRabbit

• Refactor
  • Improved formatting and consistency of firewall-related error messages without changing their content or behavior.
  • Made command and JSON error handling more explicit while preserving existing fallback behavior.
  • Removed unused variables from firewall smoke tests.

Walkthrough

Changes

Firewall maintenance

Layer / File(s)	Summary
Explicit nftables exception handling src/op_mode/firewall.py	Four nftables helpers catch OSError, UnicodeDecodeError, and, where JSON parsing occurs, json.JSONDecodeError. Empty dictionary or list fallbacks remain unchanged at lines 77, 115, 138, and 166.
Configuration message formatting src/conf_mode/firewall.py	Three ConfigError constructions and one domain-resolver marker use equivalent formatting at lines 572–574, 610–612, 622–624, and 759.
Firewall smoketest local cleanup smoketest/scripts/cli/test_firewall.py	Unused local variables were removed from IPv4 and IPv6 firewall tests at lines 331, 416, and 644.

Mergeability Score: ⚪ Minimal · up to edc1a

This PR makes localized firewall cleanup changes and narrows exception handling to documented failure modes; no actionable merge-blocking risk remains after normal checks and review.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description accurately summarizes the Ruff cleanup, narrowed exception handling, removed unused locals, behavior change, and validation steps.
Title check	✅ Passed	The title clearly identifies the firewall Ruff cleanup and the primary affected modules.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

✨ Simplify code

• Create PR with simplified code

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Pull request overview

This PR targets ruff-driven cleanup in VyOS firewall conf-mode and op-mode scripts, primarily adjusting exception handling and string formatting, plus minor smoketest refactoring.

Changes:

• Replace bare except: blocks in src/op_mode/firewall.py with explicit exception handling.
• Remove unnecessary f-string prefixes / reformat some ConfigError raises in src/conf_mode/firewall.py.
• Remove unused local variables in smoketest/scripts/cli/test_firewall.py.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/op_mode/firewall.py	Reworks exception handling around nft queries and JSON parsing to satisfy linting.
src/conf_mode/firewall.py	Cleans up string formatting in ConfigError paths and removes an unnecessary f-string.
smoketest/scripts/cli/test_firewall.py	Removes unused locals in a few firewall smoketests.

Suppressed comments (5)

src/op_mode/firewall.py:116

• The PR description says "No behavior change", but changing a bare except: to except Exception: does change behavior (e.g., KeyboardInterrupt/SystemExit will no longer be swallowed and will now propagate). If this is intended (it usually is), please update the PR description to reflect that it’s a small behavior change in exception handling semantics.

    command = ['nft', 'list', 'chain', suffix, 'vyos_filter', f'VYOS_STATE_{name_suffix}']
    try:
        results = cmdl(command)
    except Exception:
        return {}

src/op_mode/firewall.py:139

• Catching Exception here can hide JSON parsing bugs or other unexpected errors. Since the expected failure modes are cmdl() failing (raises OSError) or invalid JSON (json.JSONDecodeError), catch those explicitly and let everything else raise.

    try:
        results_str = cmdl(['nft', '-j', 'list', 'set', prefix, table, name])
        results = json.loads(results_str)
    except Exception:
        return out

src/op_mode/firewall.py:167

• Same as above: except Exception is broader than needed and can mask unexpected errors. Consider catching the expected OSError/json.JSONDecodeError failure modes only.

    try:
        results_str = cmdl(['nft', '-j', 'list', 'set', prefix, table, name])
        results = json.loads(results_str)
    except Exception:
        return out

smoketest/scripts/cli/test_firewall.py:417

• The PR description mentions restoring unused-but-harmless locals, but this change removes an unused interface local in this test. Please update the PR description to match the code (or re-add it if that was the intent).

    def test_ipv4_mask(self):
        name = 'smoketest-mask'

        self.cli_set(['firewall', 'group', 'address-group', 'mask_group', 'address', '1.1.1.1'])

smoketest/scripts/cli/test_firewall.py:645

• The PR description mentions restoring unused-but-harmless locals, but this change removes an unused interface local in this test. Please update the PR description to match the code (or re-add it if that was the intent).

    def test_ipv6_mask(self):
        name = 'v6-smoketest-mask'

        self.cli_set(['firewall', 'group', 'ipv6-address-group', 'mask_group', 'address', '::beef'])

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[ruben-herold]

Thanks for the review, addressed:

• Narrowed except Exception: to the specific exceptions each call actually raises: OSError from cmdl() and json.JSONDecodeError from json.loads(). Also applied the same narrowing to a fourth, identical call site (get_nftables_details(), ~line 77) that wasn't flagged but has the same pattern. Pushed as a new commit.
• You're right that this isn't strictly "no behavior change" — bare except:/broad except Exception: swallowed KeyboardInterrupt/SystemExit/unrelated bugs, and now only the documented failure modes are caught. Updated the PR description to call that out explicitly.
• Also fixed the PR description's wording on the smoketest locals: this PR removes the 4 unused interface/name2 locals (that's the actual ruff cleanup); firewall: T9160: add apply-path for deriving group membership from config #5374 is the one that re-adds them to undo the unrelated cleanup that had leaked into that branch. The old description had it backwards.

[ruben-herold]

One more refinement while double-checking: cmdl() decodes subprocess output as utf-8 and can raise UnicodeDecodeError (not an OSError subclass) in addition to OSError on command failure. Extremely unlikely in practice since nft output is always well-formed text/JSON, but for completeness all 4 sites now also catch UnicodeDecodeError. Pushed as edc1a80.

[github-actions]

CI integration 👍 passed!

Details

CI logs

• CLI Smoketests 👍 passed
• CLI Smoketests (interfaces only) 👍 passed
• Config tests 👍 passed
• RAID1 tests 👍 passed
• CLI Smoketests VPP 👍 passed
• Config tests VPP 👍 passed
• TPM tests 👍 passed

[ruben-herold]

@c-po Gentle ping for review — CI is green, no outstanding feedback. Same category as the T9185/T9210 lint cleanups.