From 266bd52dba31a961e0d8932905627706c626bd10 Mon Sep 17 00:00:00 2001 From: Olivier Cervello Date: Sat, 4 Jul 2026 18:35:32 +0200 Subject: [PATCH] fix(ai): warn 'missing shfmt/safecmd' (not 'ai addon') + ship shfmt via addon MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guardrail shell parser (_parse_subcommands) shells out to shfmt via safecmd. When either was absent it printed a misleading Error "Missing ai addon" (the ai addon = litellm, which can be present without the shell parser) on the ImportError path, and was SILENT on the shfmt-binary-missing path (FileNotFoundError swallowed by `except Exception`). Both left the user confused while the guardrail quietly fell back to whole-command approval. - Emit a clear one-shot Warning naming the actual missing piece — "Missing safecmd shell parser" (ImportError) or "Missing shfmt binary" (FileNotFoundError) — and fall back to the non-shfmt path (empty sub-command list -> caller asks for whole-command approval). Warn once per process, no per-command spam. - Make shfmt an explicit ai-addon dependency (`shfmt-py` in the `ai` extra) so `secator install addons ai` always ships the binary, not just the safecmd package. Also fixes the ADDONS_ENABLED['ai'] blind spot in practice: a litellm-only install no longer masquerades as a fully-working ai addon at the guardrail layer. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01NNjPggRSVZ2xnLb7ZxWP5H --- pyproject.toml | 6 +++- secator/ai/guardrails.py | 36 +++++++++++++++++++++-- tests/unit/test_ai_guardrails.py | 49 +++++++++++++++++++++++++++++++- 3 files changed, 87 insertions(+), 4 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index b9c23c7cb..ba46e700d 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -92,7 +92,11 @@ gcs = [ ] ai = [ 'litellm < 2', - 'safecmd' + 'safecmd', + # safecmd shells out to the `shfmt` binary (via shutil.which); pin shfmt-py + # explicitly so `secator install addons ai` always ships the binary, not just + # the safecmd Python package. Without it the guardrail shell parser degrades. + 'shfmt-py' ] [project.scripts] diff --git a/secator/ai/guardrails.py b/secator/ai/guardrails.py index ca85a7459..9f49c12e3 100644 --- a/secator/ai/guardrails.py +++ b/secator/ai/guardrails.py @@ -349,6 +349,34 @@ def _check_arg(arg: str): return targets +_SHELL_PARSER_WARNED = False + + +def _warn_shell_parser_unavailable(reason: str) -> None: + """Warn ONCE that the shfmt-based shell parser is unavailable, then let the + caller fall back to the non-shfmt path (whole-command approval). + + This is deliberately a Warning, not an Error, and it does NOT claim the ai + addon is missing: ``litellm`` (the ai addon) can be installed while the shell + parser — ``safecmd`` + the ``shfmt`` binary it shells out to — is not. Without + it the guardrail can't split a command into sub-commands, so + ``_check_action_type`` falls back to asking the user to approve the whole + command (safe, just coarser). Warn once so a long agent run isn't spammed on + every shell command. + """ + global _SHELL_PARSER_WARNED + if _SHELL_PARSER_WARNED: + return + _SHELL_PARSER_WARNED = True + from secator.rich import console + from secator.output_types import Warning + console.print(Warning( + message=f'{reason}: shell commands cannot be sub-parsed for guardrails — ' + 'falling back to whole-command approval. Run "secator install addons ai" ' + 'to enable precise per-subcommand parsing.' + )) + + def _parse_subcommands(command: str) -> List[List[str]]: """Parse a shell command into sub-command token lists via safecmd's parser. @@ -365,8 +393,8 @@ def _parse_subcommands(command: str) -> List[List[str]]: try: from safecmd.bashxtract import extract_commands except ImportError: - from secator.rich import console - console.print('[bold red][ERR][/] Missing ai addon: please run "secator install addons ai".') + # NOT a missing *ai* addon (litellm can be present without the shell parser). + _warn_shell_parser_unavailable('Missing safecmd shell parser') return [] try: # Normalize LLM-generated multiline commands: join lines where a pipe/operator @@ -374,6 +402,10 @@ def _parse_subcommands(command: str) -> List[List[str]]: command = re.sub(r'\s*\n\s*(\||\&\&|\|\|)', r' \1', command) cmds, ops, redirects = extract_commands(command) return [c for c in cmds if c] + except FileNotFoundError: + # safecmd is installed but the `shfmt` binary it shells out to isn't on PATH. + _warn_shell_parser_unavailable('Missing shfmt binary') + return [] except Exception: return [] diff --git a/tests/unit/test_ai_guardrails.py b/tests/unit/test_ai_guardrails.py index cda13aedc..bc082909b 100644 --- a/tests/unit/test_ai_guardrails.py +++ b/tests/unit/test_ai_guardrails.py @@ -10,7 +10,8 @@ from secator.ai.guardrails import ( parse_rule, match_rule, extract_command_targets, detect_paths, detect_paths_with_access, detect_sensitive_env_vars, classify_command, build_target_choices, PermissionEngine, - _is_file_path, _normalize_ip, _peel_wrapper, _exec_wrappers, EXEC_WRAPPERS + _is_file_path, _normalize_ip, _peel_wrapper, _exec_wrappers, EXEC_WRAPPERS, + _parse_subcommands, ) from secator.output_types import Warning, Error @@ -1283,5 +1284,51 @@ def test_firejail_scoped_rm_asks(self): self.assertEqual(self._decide(["firejail", "rm", "-rf", "/tmp/x"]), "ask") # not silent-allowed as firejail +@unittest.skipUnless(ADDONS_ENABLED['ai'], 'ai addon not installed') +class TestShellParserFallback(unittest.TestCase): + """When the shfmt-based shell parser (safecmd/shfmt) is unavailable, the + guardrail must Warn (NOT claim 'Missing ai addon') and fall back to + whole-command approval — an empty sub-command list makes the caller `ask`.""" + + def setUp(self): + import secator.ai.guardrails as g + g._SHELL_PARSER_WARNED = False # reset warn-once flag per test + + def _run_and_capture(self): + printed = [] + with patch('secator.rich.console.print', side_effect=lambda x, *a, **k: printed.append(x)): + result = _parse_subcommands('curl -s https://x.com | head -5') + return result, printed + + def test_missing_safecmd_warns_not_ai_addon(self): + # Simulate safecmd not installed -> ImportError on the in-function import. + with patch.dict('sys.modules', {'safecmd.bashxtract': None}): + result, printed = self._run_and_capture() + self.assertEqual(result, []) # unparseable -> caller falls back to ask + self.assertEqual(len(printed), 1) + item = printed[0] + self.assertIsInstance(item, Warning) # a Warning, not an Error + self.assertIn('safecmd', item.message) + self.assertNotIn('ai addon', item.message.lower()) + + def test_missing_shfmt_binary_warns(self): + # safecmd imports, but the shfmt binary it shells out to is not on PATH. + with patch('safecmd.bashxtract.extract_commands', side_effect=FileNotFoundError('shfmt')): + result, printed = self._run_and_capture() + self.assertEqual(result, []) + self.assertEqual(len(printed), 1) + self.assertIsInstance(printed[0], Warning) + self.assertIn('shfmt', printed[0].message) + self.assertNotIn('ai addon', printed[0].message.lower()) + + def test_warns_only_once_across_commands(self): + printed = [] + with patch('safecmd.bashxtract.extract_commands', side_effect=FileNotFoundError): + with patch('secator.rich.console.print', side_effect=lambda x, *a, **k: printed.append(x)): + _parse_subcommands('a | b') + _parse_subcommands('c | d') + self.assertEqual(len(printed), 1) # warn-once, no per-command spam + + if __name__ == '__main__': unittest.main()