feat(ai): unified AI branch (chat + resiliency + bash-runner + steer + hardening + restore + metering)

[ocervell]

Unified AI branch

Consolidates all in-flight core AI branches onto main (0.40.0) as one coherent, prod-ready line instead of parallel, drifting branches. Supersedes #1215, #1217, #1241, #1282, #1283 (now draft + superseded).

Reliability / robustness / security hardening (all of #1241)

Full remediation of the 2026-06-30 AI-task review (docs/superpowers/specs/2026-06-30-ai-task-reliability-review.md) — 27 findings fixed, one focused PR each.

🔴 Critical

• fix(ai): sanitize LLM-supplied subagent opts to prevent guardrail bypass (C1) #1239 — sanitize LLM-supplied subagent opts (deny control keys + force dangerous=False)
• fix(ai): make shell guardrail wrapper-aware and fix destructive-command deny (C2, H6) #1238 — wrapper-aware shell guardrail (stop exec-wrapper command laundering)
• fix(ai): idempotency marker stops Celery redelivery replay (C3) #1254 — celery_id idempotency marker; redelivery no longer replays / re-bills a turn

🟠 High

• fix(ai): bound retries on persistent rate-limit to prevent infinite loop (H1) #1235 — bound retries on persistent 429 (no infinite loop)
• fix(ai): make history trim/compact tool-pair-aware to avoid orphan tool messages (H2) #1244 — tool-pair-aware trim/compact (no orphan-tool 400s)
• fix(ai): cap subagent recursion depth & per-turn fan-out (H4) #1248 — cap subagent recursion depth + per-turn fan-out
• fix(ai): cover plain-chat & max-iter remote turns with pending docs (H5) #1249 — persist pending docs for plain-chat & max-iter remote turns
• fix(ai): make shell guardrail wrapper-aware and fix destructive-command deny (C2, H6) #1238 — fix rm -rf /*-class destructive-command deny
• fix(ai): correlate permission prompts by prompt_uuid to stop stale auto-approval (H7) #1237 — correlate permission prompts by prompt_uuid (no stale auto-approve)
• fix(ai): make "allow this command" a true one-shot, not a session rule (H9) #1243 — "allow this command" is a true one-shot, not a session rule
• fix(ai): fail closed when guardrail prompts are exhausted (H10) #1242 — fail closed when guardrail ask-prompts are exhausted

🟡 Medium

• fix(ai): cap unbounded shell output into history (M1) #1256 — cap unbounded shell output into history
• fix(ai): surface/prevent persistence-less subagent spawns (M2) #1252 — don't silently spawn persistence-less subagents on empty drivers
• fix(ai): model-window-aware history trim budget (M3) #1255 — model-window-aware history trim budget
• fix(ai): 400s fail fast except orphan-tool repair (M4) #1251 — 400s fail fast (keep orphan-tool repair)
• fix(ai): token-count fallback when LLM usage missing (M5) #1247 — token-count fallback when LLM usage is absent
• fix(ai): default-safe target/path guardrail dimensions (M6) #1246 — default-safe target/path guardrail dimensions (no fail-open on missing catch-all)
• fix(ai): gate privileged add_finding types at the guardrail (M7) #1260 — gate privileged add_finding types (block injected trusted-target minting)
• fix(ai): normalize encoded IPs before SSRF/metadata deny (M8) #1253 — normalize encoded IPs before SSRF/metadata deny
• fix(ai): treat output-flag destinations as writes (M9) #1257 — treat curl -o / wget -O output paths as writes
• fix(ai): close the answer/timeout race and stop stale pending docs accumulating (M10) #1245 — channel hygiene: answer/timeout race + stale-pending GC
• fix(ai): extensible exec-wrapper peeling closes the C2 laundering class (M11) #1264 — extensible exec-wrapper peeling (closes the laundering class)
• fix(ai): give allow_all real session-wide scope vs one-shot allow (M12) #1258 — allow_all grants real session-wide scope vs a one-shot allow

🔵 Pertinence / cleanup

• fix(ai): fix unsubstituted template vars + phantom tool in prompts (D1) #1259 — substitute prompt template vars; drop phantom run_query example
• fix(ai): stop discarding exploit mode; wire + document it (D2) #1263 — stop discarding exploit mode; wire + document it
• chore(ai): remove vestigial code (D3) #1262 — remove vestigial code (ACTION_TOOL_MAP, orphan _maybe_encrypt, dead render branch)
• refactor(ai): cheap fast-path for mode detection, LLM only when ambiguous (D4) #1261 — cheap fast-path for mode detection (LLM only when ambiguous)

---

Features layered on the hardening

• AI chat over the Mongo channel (feat: headless Mongo session restore for remote AI chat #1215) — live transcript, proposed actions, follow-ups.
• Mid-flight steering — redirect a running turn via a steer doc without killing it.
• Unified, tool-call-complete session restore (feat(ai): unified, tool-call-complete session restore #1283) — one code path for local (JSON) + remote (Mongo), rebuilding the full transcript incl. tool calls. Fixes the 2 restore tests that were blocking feat: headless Mongo session restore for remote AI chat #1215.
• Bash commands as runners (feat(ai): bash commands as runners (generic command task) #1282) — the AI's shell commands are first-class command tasks (persisted, parented, queryable).
• Allowed-targets / workspace scope (feat(ai): constrain ai task to allowed_targets scope (AI-hardening round 1) #1217) — constrain the AI task to platform-set target allow/deny lists.
• Per-run token/cost metering — context.ai_tokens / ai_cost for platform billing.
• Prod-ready cleanup — actions.py destructured (pure helpers → utils.py), duplicated blocks deduped, verbose # comments trimmed, internal round tags (M#/H#/C#/D#) stripped, full docstrings kept.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NNjPggRSVZ2xnLb7ZxWP5H

Summary by CodeRabbit

• New Features

  • Added remote AI prompt steering with prompt-scoped polling and improved follow-up handling.
  • Added a command task runner to execute shell commands and restore completed runs.
  • Improved AI output/support for steering turns and more consistent UI-visible runner items.

• Bug Fixes

  • Hardened AI guardrails/approval flow and shell command authorization to fail closed.
  • Improved AI resilience for tool calls, queries, and batch/action execution.
  • Capped/sanitized persisted chat history, enhanced token/cost accounting, and made Markdown exporting more robust.
  • Blocked cleartext remote HTTP requests in the API hook.

• Chores

  • Canary publish runs no longer auto-cancel in-progress runs for the same ref.
  • Updated AI extra dependencies to include shfmt-py.

[coderabbitai]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0dad6c98-35da-4c86-84bf-b2ca3e0fa7cc

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

Walkthrough

This PR hardens AI guardrails, subagent dispatch, shell execution, history/session handling, and remote prompt polling; adds a verbatim command task plus worker-eviction shutdown signaling; and updates HTTP transport checks, output validation, deduplication, exporter behavior, CI concurrency, and AI extras dependencies.

Changes

AI Agent Runtime Hardening

Layer / File(s)	Summary
Guardrail permission engine hardening secator/ai/guardrails.py, tests/unit/test_ai_guardrails.py	Adds IP normalization, exec-wrapper peeling, output-flag write classification, platform target mandates, privileged finding-type gating, and stricter shell/target enforcement.
Safe action dispatch and subagent spawn control secator/ai/actions.py, tests/unit/test_ai_actions.py	Adds safe_dispatch_action, fail-closed prompt handling with prompt_uuid, child-hook reconstruction/denial, and subagent recursion/fan-out caps with opt sanitization.
Shell dispatch via verbatim command task secator/tasks/command.py, secator/ai/actions.py, tests/unit/test_command_task.py, tests/unit/test_ai_actions.py	Replaces subprocess.run in _handle_shell with the new command task run through the standard runner lifecycle, sanitized env, and output truncation.
Query, tool-argument, and finding coercion secator/ai/actions.py, secator/ai/tools.py, tests/unit/test_ai_actions.py, tests/unit/test_ai_tools.py	Coerces stringified limit/query/tool arguments and dataclass field types before validation, and unions live findings for local queries.
History trimming and LLM retry/billing secator/ai/history.py, secator/ai/utils.py, tests/unit/test_ai_history.py, tests/unit/test_ai_utils.py	Adds message capping, window-aware trim budgets, orphan-tool repair/dedup, usage estimation, and 400-error repair/retry logic.
Remote prompt scoping and steer draining secator/ai/interactivity.py, tests/unit/test_ai_interactivity.py	Reworks RemoteBackend pending-prompt persistence/polling around prompt_uuid and adds a steer-drain mechanism for mid-flight interjections.
Session restore, turn idempotency, and AI task orchestration secator/ai/session.py, secator/tasks/ai.py, secator/ai/prompts.py, secator/output_types/ai.py, tests	Adds restore_history_from_db, remote resume with turn-completion markers, token/cost accounting, mode fast-path detection, and Ai output/system-prompt updates.

Command Runner and Worker Eviction

Layer / File(s)	Summary
Runner env passthrough and worker shutdown flag secator/runners/_base.py, secator/runners/command.py, secator/celery_signals.py, tests/unit/test_eviction.py	Excludes env from resolved opts, honors run_opts['env'], and adds a persistent shutdown flag polled by the monitor thread for early task termination.

Platform Safety, Validation, and Config Updates

Layer / File(s)	Summary
Cleartext HTTP transport guard secator/hooks/api.py, tests/unit/test_api_hook_transport.py	Blocks non-loopback HTTP requests before sending, allowing HTTPS or loopback HTTP only.
Output validation, dedup, and exporter fixes secator/output_types/_base.py, secator/hooks/_dedup.py, secator/exporters/markdown.py	Adds field_types() resolution, treats 'NEW' status as unset for copy-forward, and defensively extracts content in markdown export.
CI concurrency and dependency updates .github/workflows/publish-canary.yml, pyproject.toml	Removes canary workflow concurrency cancellation and adds shfmt-py to AI extras.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

• freelabz/secator#940: Follow-up hardening of the same AI task components, history, prompts, and interactivity flows.
• freelabz/secator#949: Overlapping refactors to actions.py, guardrails.py, and interactivity.py.
• freelabz/secator#727: Both PRs extend secator/runners/command.py monitor-thread behavior.

Suggested labels: ai, status-needs-testing

Poem

A rabbit hops through guarded code,
Peeling wrappers off the road,
Steering turns and capping bytes,
While shutdown flags flip off the lights,
🐇 Hop, dispatch, and restore with cheer —
This burrow's safe another year!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main AI consolidation themes covered by the PR and matches the implemented changes.
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

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/ai-unified

---

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.}

[coderabbitai]

Actionable comments posted: 13

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

secator/tasks/ai.py (1)

1142-1176: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

_runner_id lookup returns the first result's (possibly empty) id instead of the first non-empty one.

_runner_id = next((r.get("_context", {}).get("task_id")
or r.get("_context", {}).get("workflow_id")
or r.get("_context", {}).get("scan_id")
for r in group_results if isinstance(r, dict)), "")

next() returns the generator's first produced value regardless of truthiness — the if isinstance(r, dict) clause only filters by type, not by whether an id was found. If the first grouped result lacks task_id/workflow_id/scan_id but a later one in the same group has one (e.g. batch execution), the computed _runner_id is None instead of the real id, breaking the UI's runner link for that tool_result.

🐛 Proposed fix

-			_runner_id = next((r.get("_context", {}).get("task_id")
-			                   or r.get("_context", {}).get("workflow_id")
-			                   or r.get("_context", {}).get("scan_id")
-			                   for r in group_results if isinstance(r, dict)), "")
+			_runner_id = next(
+				(rid for r in group_results if isinstance(r, dict)
+				 for rid in [r.get("_context", {}).get("task_id")
+				             or r.get("_context", {}).get("workflow_id")
+				             or r.get("_context", {}).get("scan_id")]
+				 if rid),
+				"",
+			)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/ai.py` around lines 1142 - 1176, The _runner_id selection in
the grouped tool result flow is taking the first dict’s id value even when it is
empty, so later non-empty ids in the same group are ignored. Update the lookup
in the result-grouping block (around group_results, _runner_id, and
self.history.add_tool_result) to skip empty task_id/workflow_id/scan_id values
and return the first non-empty one, with a fallback of empty string if none
exist.

🟡 Minor comments (9)

tests/unit/test_eviction.py-71-78 (1)

71-78: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Update this test so it doesn’t lock in monitor-start clearing.

This assertion preserves the behavior that can erase a real shutdown flag. After moving stale cleanup out of _monitor_process(), replace this with a regression that verifies a flag raised after process start is still observed.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/test_eviction.py` around lines 71 - 78, The current test for
_monitor_process() still asserts startup clearing via clear_shutdown_flag, which
locks in the old behavior and can hide a real shutdown flag. Update
test_monitor_clears_stale_flag_at_start in test_eviction.py to stop
mocking/asserting clear_shutdown_flag and instead add a regression that starts
the monitor with a live process, raises the shutdown flag after the process
begins, and verifies _monitor_process() observes it and aborts as expected.

tests/unit/test_ai_history.py-271-272 (1)

271-272: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Use _ for intentionally unused loop variables.

Ruff flags i as unused in these setup loops.

Suggested fix

-        for i in range(40):
+        for _ in range(40):
             history.add_user("x" * 4000)  # long history, well over 8k tokens
@@
-        for i in range(40):
+        for _ in range(40):
             history.add_user("x" * 4000)

Also applies to: 289-290

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/test_ai_history.py` around lines 271 - 272, The setup loops in the
AI history tests use an unused loop variable, so update the loop in the relevant
test block(s) to use the conventional placeholder instead of a named variable.
Adjust the affected loops in the test file around the long-history setup and the
other matching setup block so Ruff no longer flags the unused iterator.

Source: Linters/SAST tools

secator/ai/history.py-256-272 (1)

256-272: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Alias Warning to avoid builtin shadowing.

Ruff flags this import; aliasing keeps the local warning output type explicit.

Suggested fix

-        from secator.output_types import Warning
+        from secator.output_types import Warning as SecatorWarning
@@
-            console.print(Warning(
+            console.print(SecatorWarning(
                 message=f'Chat history trim failed ({type(e).__name__}: {e}); using untrimmed history.'
             ))

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/ai/history.py` around lines 256 - 272, The local import named Warning
in history.py is shadowing the builtin warning name and is flagged by Ruff.
Update the import in the trim_messages / console.print path to alias
secator.output_types.Warning to a more explicit name, and use that aliased
symbol at the console.print call so the warning output type remains clear
without shadowing.

Source: Linters/SAST tools

secator/tasks/command.py-22-23 (1)

22-23: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Annotate input_types and output_types as ClassVar.
They’re class-level task metadata, so ClassVar makes that intent explicit without changing runtime behavior.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/command.py` around lines 22 - 23, Annotate the task metadata on
the Command class so it’s treated as class-level state: update the input_types
and output_types declarations in Command to use ClassVar, keeping them as shared
task metadata rather than instance fields. Make the change where these
attributes are defined so the intent is explicit without affecting runtime
behavior.

Source: Linters/SAST tools

tests/unit/test_ai_actions.py-658-659 (1)

658-659: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Split semicolon-compressed statements to satisfy lint.

Ruff/flake8 flag these new lines as E702, and E702 is not in the configured ignore list. As per coding guidelines, **/*.py: Use flake8 linting with max-line-length=120, ignoring W191, E101, E128, E265, W605 as configured in .flake8.

Suggested style

-		mock_runner = MagicMock(); mock_runner.id = 'r1'; mock_runner.reports_folder = None
-		mock_runner.__iter__.return_value = iter([]); mock_task_cls.return_value = mock_runner
+		mock_runner = MagicMock()
+		mock_runner.id = 'r1'
+		mock_runner.reports_folder = None
+		mock_runner.__iter__.return_value = iter([])
+		mock_task_cls.return_value = mock_runner

Also applies to: 679-680, 698-699

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/test_ai_actions.py` around lines 658 - 659, The test setup in
test_ai_actions.py uses semicolon-compressed statements that trigger E702. Split
the combined assignments and method setup into separate lines in the affected
test blocks, including the mock_runner/mock_task_cls initialization and the
other similar occurrences, so the code conforms to the repo’s flake8/Ruff style
rules.

Sources: Coding guidelines, Linters/SAST tools

secator/ai/actions.py-1059-1066 (1)

1059-1066: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Only coerce boolean integers when they are 0 or 1.

Line 1060 converts any integer to bool, so verified=2 silently becomes True instead of failing validation. Keep unrecognized integers untouched so the schema error remains visible.

Suggested fix

-			if isinstance(value, int):
+			if isinstance(value, int) and value in (0, 1):
 				data[key] = bool(value)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/ai/actions.py` around lines 1059 - 1066, The boolean coercion logic
in the value-normalization block is too broad because the branch that handles
ints in the data conversion path turns every integer into a boolean. Update the
conditional in the function that iterates over data fields so only 0 and 1 are
coerced to False/True, and leave all other integers unchanged so downstream
schema validation can fail as intended. Use the existing int/str handling around
the `data[key]` assignments as the place to narrow this behavior.

tests/unit/test_ai_guardrails.py-1283-1291 (1)

1283-1291: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Skip this safecmd-patched suite when the AI addon is unavailable.

Line 1289 imports safecmd.bashxtract through patch(...); if safecmd is not installed, this class fails during test setup. Adjacent parser-dependent suites are already gated with ADDONS_ENABLED['ai'].

Suggested fix

+@unittest.skipUnless(ADDONS_ENABLED['ai'], 'ai addon not installed')
 class TestOutputFlagWrites(unittest.TestCase):

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/test_ai_guardrails.py` around lines 1283 - 1291, The
TestOutputFlagWrites suite still imports safecmd.bashxtract via patch in _paths,
so it should be skipped when the AI addon is unavailable. Add the same
ADDONS_ENABLED['ai']-based gating used by the adjacent parser-dependent test
classes so TestOutputFlagWrites does not run or set up unless the addon is
present, and keep the patch call inside that guarded path.

tests/unit/test_ai_interactivity.py-258-271 (1)

258-271: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Test doesn't actually exercise the "noop flip → re-read" path it claims to cover.

searches = [[], [], [{"answer": "raced in", ...}]] has only 3 entries. Tracing _poll_for_answer with timeout=5, poll_interval=5: the loop body consumes entry #1 (answer-resolve, empty) and #2 (poll_steers, empty), then sleeps and exits the loop; the "one final search before giving up" (_resolve_answer(answered_query) right after the loop) consumes entry #3 and already returns "raced in" — so the function returns before ever calling self.query_engine.update(...). mock_engine.update.return_value = 0 is therefore never exercised, and the intended M10 scenario (flip modifies 0 rows → re-read) isn't actually being tested, even though the assertion happens to pass.

To genuinely test the "noop flip re-reads" path, the pre-flip final search also needs to come back empty:

🔧 Proposed fix

-		searches = [[], [], [{"answer": "raced in", "_timestamp": 1.0}]]
+		searches = [[], [], [], [{"answer": "raced in", "_timestamp": 1.0}]]

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/test_ai_interactivity.py` around lines 258 - 271, The test for
RemoteBackend._poll_for_answer is not exercising the intended noop-flip path
because the final pre-update search already returns the answer. Adjust the
mocked search sequence in test_timeout_noop_flip_rereads_answer so the loop and
the immediate post-loop resolve both return empty results, forcing
_poll_for_answer to call query_engine.update and then re-read after a 0-row
update. Keep the assertion on the returned answer, but make sure the mocked side
effects actually drive the _resolve_answer and update flow in RemoteBackend.

secator/tasks/ai.py-46-61 (1)

46-61: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Substring cue matching in fast_detect_mode risks false positives.

has_attack = any(cue in text for cue in _ATTACK_CUES)
has_chat = any(cue in text for cue in _CHAT_CUES)
if has_attack and not has_chat:
return "attack"
if has_chat and not has_attack:
return "chat"

cue in text is a raw substring check, not word-bounded. E.g. "hack" matches inside "hackathon", "recon" matches inside "reconsider"/"reconnect", "scan" is fine but similar cues could misfire — an unambiguous-looking chat prompt could get fast-pathed into attack mode (wrong system prompt/tool schema) without ever reaching the LLM classifier fallback. Consider \b-bounded regex matching for the cue lists.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/ai.py` around lines 46 - 61, `fast_detect_mode` is using raw
substring checks for `_ATTACK_CUES` and `_CHAT_CUES`, which can misclassify
prompts when a cue appears inside a larger word. Update the cue detection in
`fast_detect_mode` to use word-bounded matching, such as regex-based checks with
boundaries, so only standalone cue terms trigger a fast-path decision. Keep the
existing fallback behavior to return None when the prompt is ambiguous or
matches both cue sets.

🧹 Nitpick comments (8)

secator/output_types/_base.py (1)

150-179: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Consider caching field_types().

keys() on this same class is @lru_cache-decorated with a comment noting it's a hot path (deduplicate/serialization). field_types() recomputes the same per-field annotation resolution (with string parsing/splitting) on every call, including on every validate_fields() invocation, without caching.

♻️ Proposed fix

 	`@classmethod`
+	`@lru_cache`(maxsize=None)
 	def field_types(cls) -> dict:

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/output_types/_base.py` around lines 150 - 179, Cache the computed
mapping in field_types() because it performs the same annotation resolution work
repeatedly on a hot path, similar to keys(). Add an lru_cache decoration to the
Base/OutputType field_types classmethod and keep its behavior the same so
validate_fields() and other callers reuse the resolved field type map instead of
rebuilding it each time.

secator/hooks/_dedup.py (1)

4-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Field-name-specific special-casing lives in a generic dedup helper.

_is_unset hardcodes field == 'status' and the magic string 'NEW' inside a backend-agnostic dedup module, coupling it to Vulnerability's specific default value. This works and is covered by tests, but if another OutputType field later needs similar "truthy default = unset" semantics, this helper will need another hardcoded branch.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/hooks/_dedup.py` around lines 4 - 17, The generic dedup helper
`_is_unset` currently hardcodes special handling for `status` and the `'NEW'`
default, which ties it to one output type. Refactor `_is_unset` in the dedup
hook to use a configurable per-field unset rule or default-unset mapping, and
update the copy-forward logic to pass that metadata instead of branching on
`field == 'status'`. Keep the special-case behavior for `Vulnerability.status`,
but make the helper extensible for future fields without adding more hardcoded
branches.

pyproject.toml (1)

93-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider pinning shfmt-py version for reproducibility.

Every other dependency in this file uses an upper-bound pin (e.g. litellm < 2, pymongo < 5), but shfmt-py is added unpinned. Since shfmt-py is versioned independently from the underlying shfmt binary it bundles, an unpinned install could silently pull in a new major/minor release with a different bundled binary version, undermining the reproducibility this new dependency is meant to guarantee for the guardrail shell parser.

♻️ Suggested pin

 ai = [
 	'litellm < 2',
 	'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'
+	'shfmt-py < 5'
 ]

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pyproject.toml` around lines 93 - 100, The ai dependency group adds shfmt-py
without a version bound, which breaks the repo’s existing pinning pattern and
can change the bundled shfmt binary unexpectedly. Update the shfmt-py entry in
the ai extras list to use an upper-bound pin consistent with the other
dependencies in pyproject.toml, keeping the guardrail shell parser install
reproducible.

secator/ai/prompts/constraints/common.txt (1)

75-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Potential ambiguity between file_io guidance and the truncated_output examples.

file_io now says "Do NOT read local report files (e.g. via cat/jq) to look up findings," but the truncated_output section (unchanged, lines 36-67) explicitly instructs run_shell(command="jq '[...]' <OUTPUT_JSON_PATH>") to inspect a task's own truncated output. These aren't strictly contradictory (one is "look up past workspace findings via query_workspace", the other is "post-process this call's own truncated output"), but an LLM reading both sections back-to-back could plausibly conflate them. Consider adding a short clarifying clause distinguishing "your own truncated tool output" from "workspace findings lookups."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/ai/prompts/constraints/common.txt` around lines 75 - 78, Clarify the
guidance in common.txt by distinguishing the allowed use of `run_shell`/`jq` for
inspecting this call’s own truncated output from the prohibited use of local
report files for workspace findings lookup. Update the `file_io` section to
explicitly say that `query_workspace` is the source of truth for
findings/results, while the `truncated_output` examples remain valid only for
post-processing the current tool’s output. Keep the wording aligned with the
existing `file_io` and `truncated_output` headings so the two instructions
cannot be read as conflicting.

tests/unit/test_ai_loop.py (1)

334-400: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Unused unpacked variables flagged by ruff (yielded at 380/395, follow_up at 395).

Cosmetic only; per static analysis, these tuple-unpacked values aren't read afterward in either test.

🔧 Proposed fix

-		yielded, persisted, follow_up = self._run_dispatch(backend)
+		_yielded, persisted, follow_up = self._run_dispatch(backend)
 ...
-		yielded, persisted, follow_up = self._run_dispatch(backend)
+		_yielded, persisted, _follow_up = self._run_dispatch(backend)

Also relates to the `output_types/ai.py` session_id-field-removal concern flagged above: this test's `doc.session_id` assertion (line 388) exercises the dynamically-set, non-schema attribute discussed there.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/test_ai_loop.py` around lines 334 - 400, The two tests in
test_ai_loop.py are unpacking values from _run_dispatch that are never read,
which triggers ruff’s unused-variable warning. Update
test_remote_follow_up_persisted_pending_with_choices and
test_local_follow_up_not_stamped_pending to avoid binding the unused tuple
elements, while keeping the assertions that use the persisted follow_up
document; if you keep the session_id check, ensure it still reflects the
intended dynamic attribute behavior in _dispatch_and_collect and Ai.

Source: Linters/SAST tools

secator/ai/session.py (1)

139-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Silent per-item exception swallow with no logging.

for item_data, cls in all_items:
try:
console.print(cls.load(item_data), highlight=False)
except Exception:
continue

Flagged by static analysis (S112/BLE001): a failed item load is silently dropped with no trace, making it hard to diagnose why a resumed session's console replay is missing entries. Consider logging via debug() before continuing.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/ai/session.py` around lines 139 - 174, The per-item exception
handling in print_session_results is swallowing load/print failures silently, so
missing replay entries are impossible to diagnose. Update the exception block
around cls.load(item_data) and console.print(...) to emit a debug() message with
the item’s type/class and the exception details before continuing, while keeping
the best-effort behavior and avoiding any raise.

Source: Linters/SAST tools

secator/tasks/ai.py (2)

1276-1346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Flake8 E127 (continuation line over-indented) at Line 1344-1345.

E127 is not in the project's flake8 ignore list (W191, E101, E128, E265, W605), so this could fail CI lint on the new items.append(...) call.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/ai.py` around lines 1276 - 1346, The new Ai append call in
_prompt_and_redetect triggers a Flake8 E127 continuation-line indentation issue,
so adjust the multi-line items.append(Ai(...)) formatting to use a compliant
hanging indent. Keep the change localized to the prompt result construction in
_prompt_and_redetect, and make sure the Ai invocation remains readable without
violating the project’s lint rules.

Sources: Coding guidelines, Linters/SAST tools

---

995-1055: 📐 Maintainability & Code Quality | 🔵 Trivial

Repeated error-emission pattern across three sites.

The "encrypt error → add_tool_result → yield Ai(ai_type="tool_result")" sequence is duplicated near-verbatim at malformed-JSON (Line 995-1000), unknown-tool (Line 1025-1030), and guardrail-denial (Line 1050-1055). Consider extracting a small helper, e.g. _reject_tool_call(name, tc_id, error_msg), to build the encrypted content, append to history, and return the Ai item.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/ai.py` around lines 995 - 1055, The rejection flow in Ai
tool-call handling repeats the same encrypt → history.add_tool_result → yield
Ai(ai_type="tool_result") pattern in multiple branches. Extract that shared
logic into a small helper on Ai, such as _reject_tool_call(name, tc_id,
error_msg), and use it from the malformed-arguments, unknown-tool, and denial
paths to build the encrypted payload, record it in history, and return the
tool_result Ai consistently.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@secator/ai/actions.py`:
- Around line 611-615: The LLM options handling in `secator/ai/actions.py` is
copying the parent `ctx.api_key` into `opts` and then persisting those same
`opts` in the `Ai` action item, which can leak the parent key and let a
model-supplied `api_base` point to an attacker-controlled endpoint. In the
`opts.setdefault(...)` block and the `Ai` construction/persistence path, stop
propagating inherited secrets into subagent opts, strip or ignore `api_key`
before saving `opts`, and ensure only non-sensitive config is stored while any
parent credentials are passed separately and transiently. Use the `ctx.model`,
`ctx.api_key`, `ctx.api_base`, and `Ai` initialization sites to locate the full
flow.
- Around line 76-78: The environment filtering comprehension that iterates over
os.environ.items() is matching secrets case-sensitively, so lowercase names like
openai_api_key or aws_secret_access_key can slip through. Normalize each
environment key with k.upper() before applying the SENSITIVE_ENV_PREFIXES checks
and the KEY/SECRET/TOKEN/PASSWORD substring checks, while keeping the existing
filtering logic otherwise unchanged.

In `@secator/ai/guardrails.py`:
- Around line 54-62: The wrapper grammar in _WRAPPER_ARG_GRAMMAR is missing
value-taking options for xargs and env, so wrapper peeling can stop before
reaching the inner command. Update the grammar used by the peeling logic in this
module to include the relevant xargs/env options (for example, operand-taking
flags like -I and -u) with the correct arity so wrapper parsing continues into
the wrapped command instead of downgrading to an unknown-command ask.
- Around line 986-992: The privileged add_finding branch currently returns
PermissionResult(decision="ask") without any promptable payload, so
check_guardrails cannot surface it for approval and will eventually recheck/deny
it. Update the add_finding path in guardrails.py to either return a resolvable
prompt payload (populate the field(s) that downstream prompting understands,
such as shell_command/targets/paths or an equivalent permission payload) or
change the decision to deny if approval should not be requested. Keep the
_is_privileged_finding_type gating and ensure the returned PermissionResult is
compatible with the existing check_guardrails prompt flow.
- Around line 819-830: The target-matching helpers in guardrails.py are too
permissive because _matches_allowed_targets and _matches_denied_targets fall
back to prefix matching with rx.match(value) after fullmatch. Remove the
prefix-match fallback so mandate targets only pass on exact/full regex matches,
and keep the logic localized to these two methods to preserve the platform scope
boundary.
- Around line 579-603: The output-flag path detection in the argument parser is
being skipped when the command is wrapped, so write destinations can fall back
to the wrapper’s default access. Update the logic in the command-argument
handling block around write_flags/_add_path so wrapper peeling happens before
classifying output-flag destinations, ensuring cases like timeout wrapping curl
or wget still resolve the actual cmd_name before checking OUTPUT_FLAG_COMMANDS.
Keep the existing write destination handling for arg forms like -o FILE,
--output=FILE, and attached short flags, but apply it after unwrapping the
command token.

In `@secator/ai/interactivity.py`:
- Around line 134-153: The permission-handling path in ask_user currently
converts every non-allow response into a deny, which causes steer text returned
by _poll_for_answer() to be lost. Update ask_user in the permission branch so
steer responses are handled separately from true deny responses, either by
returning the steer payload or by preventing permission waits from using the
steer-break behavior in _poll_for_answer(); keep the allow/allow_all logic in
ask_user and preserve the existing permission rule updates via
_add_permission_rules.
- Around line 154-199: The issue is that poll_steers() fetches a batch of
pending steers but then marks them consumed using only the broad session/status
filter, which can over-update or inconsistently update across backends. Fix it
by changing the consumption step in poll_steers() to target the exact fetched
documents from results, using a stable per-doc selector (for example each doc’s
unique identifier or another immutable claim field) or an atomic claim/update
path so only the returned steers are flipped to consumed exactly once. Ensure
the logic in poll_steers() still preserves oldest-first ordering and remains
backend-safe.

In `@secator/ai/session.py`:
- Around line 202-299: The restore logic in restore_history_from_db currently
splits persisted docs into message-bearing and legacy buckets, which causes
ai_type="steer" entries to be appended after later message docs instead of
staying in timestamp order. Update the session rebuild flow to merge both record
shapes into a single _timestamp-sorted sequence before populating ChatHistory,
while keeping the existing verbatim handling for message dicts and the legacy
maybe_encrypt fallback for text-only docs.

In `@secator/ai/utils.py`:
- Around line 343-356: The retry loop in the LLM call path can exit after the
orphan tool_use repair branch without ever setting response, so update the logic
in the completion retry block to avoid falling through after the final repair
retry. In the function that wraps litellm.completion, ensure the BadRequestError
handling either returns immediately after a successful repair retry or re-raises
the original error when no further retries remain, and only breaks the loop when
response is actually assigned. Use the existing symbols litellm.BadRequestError,
_repair_orphan_tool_uses, and response to locate and adjust the control flow.

In `@secator/celery_signals.py`:
- Around line 30-33: Make clear_shutdown_flag race-safe by removing the separate
SHUTDOWN_FLAG.exists() check and using a single atomic unlink attempt with
FileNotFoundError handling. Update the clear_shutdown_flag function in
celery_signals so concurrent workers or cleanup paths can safely clear the
shared flag without crashing if another process deletes it first.

In `@secator/runners/command.py`:
- Around line 751-756: Do not clear the shutdown flag inside `_monitor_process`
in `secator/runners/command.py`; that monitor should only observe shutdown state
via `is_worker_shutting_down()`. Move the stale-flag cleanup to worker setup or
another pre-subprocess path, keeping `worker_shutting_down_handler()`
responsible for setting the flag and ensuring `Popen()` cannot be affected by a
late clear.

In `@secator/tasks/command.py`:
- Around line 76-102: The imported timestamps are being overwritten by
mark_started() and mark_completed() before the on_start/on_end persistence hooks
run, so update_runner persists the auto-generated times instead of the
caller-supplied ones. Fix Command.from_result() by ensuring start_time and
end_time are set on the runner before calling mark_started()/mark_completed(),
or by introducing a timestamp-aware path in those methods so the hooks see the
restored values.

---

Outside diff comments:
In `@secator/tasks/ai.py`:
- Around line 1142-1176: The _runner_id selection in the grouped tool result
flow is taking the first dict’s id value even when it is empty, so later
non-empty ids in the same group are ignored. Update the lookup in the
result-grouping block (around group_results, _runner_id, and
self.history.add_tool_result) to skip empty task_id/workflow_id/scan_id values
and return the first non-empty one, with a fallback of empty string if none
exist.

---

Minor comments:
In `@secator/ai/actions.py`:
- Around line 1059-1066: The boolean coercion logic in the value-normalization
block is too broad because the branch that handles ints in the data conversion
path turns every integer into a boolean. Update the conditional in the function
that iterates over data fields so only 0 and 1 are coerced to False/True, and
leave all other integers unchanged so downstream schema validation can fail as
intended. Use the existing int/str handling around the `data[key]` assignments
as the place to narrow this behavior.

In `@secator/ai/history.py`:
- Around line 256-272: The local import named Warning in history.py is shadowing
the builtin warning name and is flagged by Ruff. Update the import in the
trim_messages / console.print path to alias secator.output_types.Warning to a
more explicit name, and use that aliased symbol at the console.print call so the
warning output type remains clear without shadowing.

In `@secator/tasks/ai.py`:
- Around line 46-61: `fast_detect_mode` is using raw substring checks for
`_ATTACK_CUES` and `_CHAT_CUES`, which can misclassify prompts when a cue
appears inside a larger word. Update the cue detection in `fast_detect_mode` to
use word-bounded matching, such as regex-based checks with boundaries, so only
standalone cue terms trigger a fast-path decision. Keep the existing fallback
behavior to return None when the prompt is ambiguous or matches both cue sets.

In `@secator/tasks/command.py`:
- Around line 22-23: Annotate the task metadata on the Command class so it’s
treated as class-level state: update the input_types and output_types
declarations in Command to use ClassVar, keeping them as shared task metadata
rather than instance fields. Make the change where these attributes are defined
so the intent is explicit without affecting runtime behavior.

In `@tests/unit/test_ai_actions.py`:
- Around line 658-659: The test setup in test_ai_actions.py uses
semicolon-compressed statements that trigger E702. Split the combined
assignments and method setup into separate lines in the affected test blocks,
including the mock_runner/mock_task_cls initialization and the other similar
occurrences, so the code conforms to the repo’s flake8/Ruff style rules.

In `@tests/unit/test_ai_guardrails.py`:
- Around line 1283-1291: The TestOutputFlagWrites suite still imports
safecmd.bashxtract via patch in _paths, so it should be skipped when the AI
addon is unavailable. Add the same ADDONS_ENABLED['ai']-based gating used by the
adjacent parser-dependent test classes so TestOutputFlagWrites does not run or
set up unless the addon is present, and keep the patch call inside that guarded
path.

In `@tests/unit/test_ai_history.py`:
- Around line 271-272: The setup loops in the AI history tests use an unused
loop variable, so update the loop in the relevant test block(s) to use the
conventional placeholder instead of a named variable. Adjust the affected loops
in the test file around the long-history setup and the other matching setup
block so Ruff no longer flags the unused iterator.

In `@tests/unit/test_ai_interactivity.py`:
- Around line 258-271: The test for RemoteBackend._poll_for_answer is not
exercising the intended noop-flip path because the final pre-update search
already returns the answer. Adjust the mocked search sequence in
test_timeout_noop_flip_rereads_answer so the loop and the immediate post-loop
resolve both return empty results, forcing _poll_for_answer to call
query_engine.update and then re-read after a 0-row update. Keep the assertion on
the returned answer, but make sure the mocked side effects actually drive the
_resolve_answer and update flow in RemoteBackend.

In `@tests/unit/test_eviction.py`:
- Around line 71-78: The current test for _monitor_process() still asserts
startup clearing via clear_shutdown_flag, which locks in the old behavior and
can hide a real shutdown flag. Update test_monitor_clears_stale_flag_at_start in
test_eviction.py to stop mocking/asserting clear_shutdown_flag and instead add a
regression that starts the monitor with a live process, raises the shutdown flag
after the process begins, and verifies _monitor_process() observes it and aborts
as expected.

---

Nitpick comments:
In `@pyproject.toml`:
- Around line 93-100: The ai dependency group adds shfmt-py without a version
bound, which breaks the repo’s existing pinning pattern and can change the
bundled shfmt binary unexpectedly. Update the shfmt-py entry in the ai extras
list to use an upper-bound pin consistent with the other dependencies in
pyproject.toml, keeping the guardrail shell parser install reproducible.

In `@secator/ai/prompts/constraints/common.txt`:
- Around line 75-78: Clarify the guidance in common.txt by distinguishing the
allowed use of `run_shell`/`jq` for inspecting this call’s own truncated output
from the prohibited use of local report files for workspace findings lookup.
Update the `file_io` section to explicitly say that `query_workspace` is the
source of truth for findings/results, while the `truncated_output` examples
remain valid only for post-processing the current tool’s output. Keep the
wording aligned with the existing `file_io` and `truncated_output` headings so
the two instructions cannot be read as conflicting.

In `@secator/ai/session.py`:
- Around line 139-174: The per-item exception handling in print_session_results
is swallowing load/print failures silently, so missing replay entries are
impossible to diagnose. Update the exception block around cls.load(item_data)
and console.print(...) to emit a debug() message with the item’s type/class and
the exception details before continuing, while keeping the best-effort behavior
and avoiding any raise.

In `@secator/hooks/_dedup.py`:
- Around line 4-17: The generic dedup helper `_is_unset` currently hardcodes
special handling for `status` and the `'NEW'` default, which ties it to one
output type. Refactor `_is_unset` in the dedup hook to use a configurable
per-field unset rule or default-unset mapping, and update the copy-forward logic
to pass that metadata instead of branching on `field == 'status'`. Keep the
special-case behavior for `Vulnerability.status`, but make the helper extensible
for future fields without adding more hardcoded branches.

In `@secator/output_types/_base.py`:
- Around line 150-179: Cache the computed mapping in field_types() because it
performs the same annotation resolution work repeatedly on a hot path, similar
to keys(). Add an lru_cache decoration to the Base/OutputType field_types
classmethod and keep its behavior the same so validate_fields() and other
callers reuse the resolved field type map instead of rebuilding it each time.

In `@secator/tasks/ai.py`:
- Around line 1276-1346: The new Ai append call in _prompt_and_redetect triggers
a Flake8 E127 continuation-line indentation issue, so adjust the multi-line
items.append(Ai(...)) formatting to use a compliant hanging indent. Keep the
change localized to the prompt result construction in _prompt_and_redetect, and
make sure the Ai invocation remains readable without violating the project’s
lint rules.
- Around line 995-1055: The rejection flow in Ai tool-call handling repeats the
same encrypt → history.add_tool_result → yield Ai(ai_type="tool_result") pattern
in multiple branches. Extract that shared logic into a small helper on Ai, such
as _reject_tool_call(name, tc_id, error_msg), and use it from the
malformed-arguments, unknown-tool, and denial paths to build the encrypted
payload, record it in history, and return the tool_result Ai consistently.

In `@tests/unit/test_ai_loop.py`:
- Around line 334-400: The two tests in test_ai_loop.py are unpacking values
from _run_dispatch that are never read, which triggers ruff’s unused-variable
warning. Update test_remote_follow_up_persisted_pending_with_choices and
test_local_follow_up_not_stamped_pending to avoid binding the unused tuple
elements, while keeping the assertions that use the persisted follow_up
document; if you keep the session_id check, ensure it still reflects the
intended dynamic attribute behavior in _dispatch_and_collect and Ai.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: a0552ae3-5e80-452a-bf9a-68fa62076d71

📥 Commits

Reviewing files that changed from the base of the PR and between 9b11716 and 0fe24d7.

📒 Files selected for processing (36)

• .github/workflows/publish-canary.yml
• pyproject.toml
• secator/ai/actions.py
• secator/ai/guardrails.py
• secator/ai/history.py
• secator/ai/interactivity.py
• secator/ai/prompts.py
• secator/ai/prompts/constraints/common.txt
• secator/ai/session.py
• secator/ai/tools.py
• secator/ai/utils.py
• secator/celery_signals.py
• secator/exporters/markdown.py
• secator/hooks/_dedup.py
• secator/hooks/api.py
• secator/output_types/_base.py
• secator/output_types/ai.py
• secator/runners/_base.py
• secator/runners/command.py
• secator/tasks/ai.py
• secator/tasks/command.py
• tests/unit/test_ai_actions.py
• tests/unit/test_ai_guardrails.py
• tests/unit/test_ai_history.py
• tests/unit/test_ai_interactivity.py
• tests/unit/test_ai_loop.py
• tests/unit/test_ai_prompts.py
• tests/unit/test_ai_resilience.py
• tests/unit/test_ai_session.py
• tests/unit/test_ai_task_opts.py
• tests/unit/test_ai_tokens.py
• tests/unit/test_ai_tools.py
• tests/unit/test_ai_utils.py
• tests/unit/test_api_hook_transport.py
• tests/unit/test_command_task.py
• tests/unit/test_eviction.py

💤 Files with no reviewable changes (1)

• .github/workflows/publish-canary.yml

[coderabbitai]

🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win

Do not let LLM opts redirect or persist the parent API key.

Because api_base is not stripped, a model-supplied subagent opt can set api_base to an attacker-controlled endpoint while Line 612 injects the parent api_key. Line 703 then also persists opts, including the inherited key, into the Ai action item.

Suggested direction

 		opts.setdefault("model", ctx.model)
+		# Never let tool-provided opts override transport credentials.
+		opts.pop("api_key", None)
+		opts.pop("api_base", None)
 		if ctx.api_key:
-			opts.setdefault("api_key", ctx.api_key)
+			opts["api_key"] = ctx.api_key
 		if ctx.api_base:
-			opts.setdefault("api_base", ctx.api_base)
+			opts["api_base"] = ctx.api_base
...
+	safe_display_opts = {k: v for k, v in opts.items() if k != "api_key"}
 	yield Ai(
...
-			"opts": opts,
+			"opts": safe_display_opts,

Also applies to: 701-704

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/ai/actions.py` around lines 611 - 615, The LLM options handling in
`secator/ai/actions.py` is copying the parent `ctx.api_key` into `opts` and then
persisting those same `opts` in the `Ai` action item, which can leak the parent
key and let a model-supplied `api_base` point to an attacker-controlled
endpoint. In the `opts.setdefault(...)` block and the `Ai`
construction/persistence path, stop propagating inherited secrets into subagent
opts, strip or ignore `api_key` before saving `opts`, and ensure only
non-sensitive config is stored while any parent credentials are passed
separately and transiently. Use the `ctx.model`, `ctx.api_key`, `ctx.api_base`,
and `Ai` initialization sites to locate the full flow.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Teach wrapper peeling about value-taking xargs/env options.

Line 443 falls back to an empty grammar for wrappers not listed here, so forms like xargs -I {} rm -rf / or env -u TOKEN rm -rf / can stop peeling at the option operand instead of the inner command. That downgrades inner-command denies into an unknown-command ask.

Suggested direction

 _WRAPPER_ARG_GRAMMAR = {
+	"env":          (frozenset({"-u", "--unset", "-C", "--chdir", "-S", "--split-string"}), 0, _EMPTY),
+	"xargs":        (frozenset({"-I", "--replace", "-E", "--eof", "-n", "--max-args", "-P", "--max-procs", "-s", "--max-chars"}), 0, _EMPTY),
 	"flock":        (frozenset({"-w", "--timeout", "-E", "--conflict-exit-code"}), 1, frozenset({"-c", "--command"})),

Also applies to: 427-467

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/ai/guardrails.py` around lines 54 - 62, The wrapper grammar in
_WRAPPER_ARG_GRAMMAR is missing value-taking options for xargs and env, so
wrapper peeling can stop before reaching the inner command. Update the grammar
used by the peeling logic in this module to include the relevant xargs/env
options (for example, operand-taking flags like -I and -u) with the correct
arity so wrapper parsing continues into the wrapped command instead of
downgrading to an unknown-command ask.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Do not prefix-match mandate targets.

Line 822 and Line 829 use rx.match(value) after fullmatch, so an allowed mandate like example.com can match example.com.evil.tld. This weakens the platform scope boundary; mandate checks should be exact/full regex matches only.

Suggested fix

 	def _matches_allowed_targets(self, value: str) -> bool:
 		"""Check if a target value matches any platform-supplied allowed_targets regex."""
 		for rx in self.allowed_targets:
-			if rx.fullmatch(value) or rx.match(value):
+			if rx.fullmatch(value):
 				return True
 		return False
 
 	def _matches_denied_targets(self, value: str) -> bool:
 		"""Check if a target value matches any platform-supplied denied_targets regex."""
 		for rx in self.denied_targets:
-			if rx.fullmatch(value) or rx.match(value):
+			if rx.fullmatch(value):
 				return True
 		return False

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	def _matches_allowed_targets(self, value: str) -> bool:
	"""Check if a target value matches any platform-supplied allowed_targets regex."""
	for rx in self.allowed_targets:
	if rx.fullmatch(value) or rx.match(value):
	return True
	return False
	def _matches_denied_targets(self, value: str) -> bool:
	"""Check if a target value matches any platform-supplied denied_targets regex."""
	for rx in self.denied_targets:
	if rx.fullmatch(value) or rx.match(value):
	return True
	def _matches_allowed_targets(self, value: str) -> bool:
	"""Check if a target value matches any platform-supplied allowed_targets regex."""
	for rx in self.allowed_targets:
	if rx.fullmatch(value):
	return True
	return False
	def _matches_denied_targets(self, value: str) -> bool:
	"""Check if a target value matches any platform-supplied denied_targets regex."""
	for rx in self.denied_targets:
	if rx.fullmatch(value):
	return True
	return False

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/ai/guardrails.py` around lines 819 - 830, The target-matching helpers
in guardrails.py are too permissive because _matches_allowed_targets and
_matches_denied_targets fall back to prefix matching with rx.match(value) after
fullmatch. Remove the prefix-match fallback so mandate targets only pass on
exact/full regex matches, and keep the logic localized to these two methods to
preserve the platform scope boundary.

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Make flag clearing race-safe.

exists() followed by unlink() can raise FileNotFoundError when another process clears the shared flag between the two calls. Use an atomic unlink attempt so setup/monitor cleanup cannot crash on concurrent workers.

Proposed fix

 def clear_shutdown_flag():
 	"""Remove the eviction flag (called on worker boot to drop any stale flag)."""
-	if SHUTDOWN_FLAG.exists():
+	try:
 		SHUTDOWN_FLAG.unlink()
+	except FileNotFoundError:
+		pass

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	def clear_shutdown_flag():
	"""Remove the eviction flag (called on worker boot to drop any stale flag)."""
	if SHUTDOWN_FLAG.exists():
	SHUTDOWN_FLAG.unlink()
	def clear_shutdown_flag():
	"""Remove the eviction flag (called on worker boot to drop any stale flag)."""
	try:
	SHUTDOWN_FLAG.unlink()
	except FileNotFoundError:
	pass

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/celery_signals.py` around lines 30 - 33, Make clear_shutdown_flag
race-safe by removing the separate SHUTDOWN_FLAG.exists() check and using a
single atomic unlink attempt with FileNotFoundError handling. Update the
clear_shutdown_flag function in celery_signals so concurrent workers or cleanup
paths can safely clear the shared flag without crashing if another process
deletes it first.

[coderabbitai]

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Don’t clear the shutdown flag inside the monitor thread.

A real eviction can be lost if worker_shutting_down_handler() writes the flag after Popen() starts but before _monitor_process() reaches Line 756. The monitor should only read shutdown state; stale cleanup should happen during worker setup or before the subprocess can be affected.

Proposed direction

 def _monitor_process(self):
 	"""Monitor thread that checks process health and kills if necessary."""
-	from secator.celery_signals import clear_shutdown_flag, is_worker_shutting_down
-	# Only honour a shutdown raised *during* this run: clear any stale flag left by a previous
-	# worker/task sharing this machine's state dir. In prod each task gets a fresh pod (and /tmp),
-	# but tests and a long-lived `secator worker` reuse it, so a leftover flag would otherwise
-	# wrongly stop every later task.
-	clear_shutdown_flag()
+	from secator.celery_signals import is_worker_shutting_down
 	last_stats_time = 0

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	from secator.celery_signals import clear_shutdown_flag, is_worker_shutting_down
	# Only honour a shutdown raised *during* this run: clear any stale flag left by a previous
	# worker/task sharing this machine's state dir. In prod each task gets a fresh pod (and /tmp),
	# but tests and a long-lived `secator worker` reuse it, so a leftover flag would otherwise
	# wrongly stop every later task.
	clear_shutdown_flag()
	from secator.celery_signals import is_worker_shutting_down
	last_stats_time = 0

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/runners/command.py` around lines 751 - 756, Do not clear the shutdown
flag inside `_monitor_process` in `secator/runners/command.py`; that monitor
should only observe shutdown state via `is_worker_shutting_down()`. Move the
stale-flag cleanup to worker setup or another pre-subprocess path, keeping
`worker_shutting_down_handler()` responsible for setting the flag and ensuring
`Popen()` cannot be affected by a late clear.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

ast-grep outline secator/tasks/command.py --view expanded

Repository: freelabz/secator

Length of output: 494

---

🏁 Script executed:

sed -n '1,220p' secator/tasks/command.py && printf '\n----\n' && ast-grep outline secator/runners/_base.py --view expanded

Repository: freelabz/secator

Length of output: 8278

---

🏁 Script executed:

sed -n '1140,1178p' secator/runners/_base.py && printf '\n----\n' && sed -n '1023,1068p' secator/runners/_base.py

Repository: freelabz/secator

Length of output: 3389

---

🏁 Script executed:

rg -n "update_runner|on_end|on_start|persist" secator -g '*.py'

Repository: freelabz/secator

Length of output: 12416

---

🏁 Script executed:

sed -n '1,140p' secator/hooks/update_runner.py

Repository: freelabz/secator

Length of output: 228

---

🏁 Script executed:

rg -n "from_result\\(" secator -g '*.py'

Repository: freelabz/secator

Length of output: 301

---

Persist imported timestamps before hook persistence. mark_started() / mark_completed() run on_start / on_end before from_result() restores start_time / end_time, so update_runner saves the auto-stamped times instead of the caller-supplied ones. Move those assignments ahead of the hooks or add a timestamp-aware mark path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/command.py` around lines 76 - 102, The imported timestamps are
being overwritten by mark_started() and mark_completed() before the
on_start/on_end persistence hooks run, so update_runner persists the
auto-generated times instead of the caller-supplied ones. Fix
Command.from_result() by ensuring start_time and end_time are set on the runner
before calling mark_started()/mark_completed(), or by introducing a
timestamp-aware path in those methods so the hooks see the restored values.

[ocervell]

@CodeRabbit do a full review, and also generate the whole Ai task graph.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -e
echo "=== ai.py outline ==="
ast-grep outline secator/tasks/ai.py --items all --type function,class --match 'ai|yielder|_run_loop|_dispatch|_maybe_resume|_drain_steers|_prompt_and_redetect|_mark_turn|_turn_uuid|_save_history|_get_query_engine|fast_detect_mode|_init_options'

echo "=== actions.py outline ==="
ast-grep outline secator/ai/actions.py --items all --type function --match 'dispatch_action|safe_dispatch_action|_handle_shell|_run_runner|_sanitize_child_opts|query|add_finding'

echo "=== guardrails.py outline ==="
ast-grep outline secator/ai/guardrails.py --items all --type function --match 'check_action|_check_action_type|_check_value|match_rule|prompt_shell|_parse_subcommands'

echo "=== interactivity.py outline ==="
ast-grep outline secator/ai/interactivity.py --items all --type function,class

echo "=== session.py outline ==="
ast-grep outline secator/ai/session.py --items all --type function

Length of output: 3941

---

@ocervell I'll kick off a full review of the PR now. In the meantime, here's the AI task graph based on the consolidated flow across secator/tasks/ai.py, secator/ai/actions.py, secator/ai/guardrails.py, secator/ai/interactivity.py, and secator/ai/session.py:

flowchart TD
    Start([ai task invoked]) --> InitOpts[_init_options<br/>stamp session_id, model, allowed/denied targets]
    InitOpts --> DetectMode{Mode specified?}
    DetectMode -- no --> FastDetect[fast_detect_mode<br/>deterministic cue matching]
    FastDetect --> LLMDetect{Ambiguous?}
    LLMDetect -- yes --> DetectModeLLM[_detect_mode<br/>LLM classification]
    LLMDetect -- no --> ModeResolved
    DetectModeLLM --> ModeResolved[mode resolved: chat/attack/exploit]
    DetectMode -- yes --> ModeResolved

    ModeResolved --> Yielder[yielder]
    Yielder --> ResumeCheck{Remote channel + session_id?}
    ResumeCheck -- yes --> MaybeResumeRemote[_maybe_resume_remote]
    MaybeResumeRemote --> RestoreDB[session.restore_history_from_db<br/>rebuild ChatHistory from Mongo docs]
    ResumeCheck -- no --> LocalResumeCheck{Local resume requested?}
    LocalResumeCheck -- yes --> ReplaySession[session.replay_session /<br/>print_session_results]
    LocalResumeCheck -- no --> FreshHistory[Fresh ChatHistory + system prompt]

    RestoreDB --> RunLoop
    ReplaySession --> RunLoop
    FreshHistory --> RunLoop

    RunLoop[_run_loop] --> TurnMarkerCheck{Turn already completed?<br/>_turn_completed_marker}
    TurnMarkerCheck -- yes --> SkipTurn[Skip replay: return cached result]
    TurnMarkerCheck -- no --> DrainSteers[_drain_steers<br/>poll_steers -> inject interjected user msgs]

    DrainSteers --> CallLLM[call_llm via secator/ai/utils.py<br/>retry + orphan tool-use repair]
    CallLLM --> AccountUsage[_account_usage<br/>tokens/cost -> context.ai_tokens]
    AccountUsage --> HasToolCalls{Tool calls in response?}

    HasToolCalls -- no --> AddAssistant[_add_assistant_to_history]
    AddAssistant --> FollowUp{Needs follow-up / ask_user?}
    FollowUp -- yes --> PromptRedetect[_prompt_and_redetect<br/>prompt_uuid correlation]
    PromptRedetect --> Backend[interactivity.create_backend]
    Backend --> CLIBackend[CLIBackend.ask_user]
    Backend --> RemoteBackend[RemoteBackend.ask_user<br/>build_pending_prompt / poll]
    Backend --> AutoBackend[AutoBackend.ask_user]
    RemoteBackend --> RunLoop
    CLIBackend --> RunLoop
    AutoBackend --> RunLoop
    FollowUp -- no --> MarkComplete

    HasToolCalls -- yes --> ProcessToolCalls[_process_tool_calls<br/>coerce_stringified_args, tool_call_to_action]
    ProcessToolCalls --> Guardrails[guardrails.check_action]
    Guardrails --> CheckType[_check_action_type]
    CheckType --> CheckValue[_check_value / _check_values]
    CheckValue --> MatchRule[match_rule<br/>IP normalization, deny/allow patterns]
    Guardrails --> Decision{allow / ask / deny}

    Decision -- deny --> ToolResultDenied[Yield Ai tool_result: denied]
    Decision -- ask --> PromptShell[prompt_shell / ask_user<br/>interactive or remote pending prompt]
    PromptShell --> Decision
    Decision -- allow --> DispatchCollect[_dispatch_and_collect]

    DispatchCollect --> SafeDispatch[safe_dispatch_action]
    SafeDispatch --> DispatchAction[dispatch_action]
    DispatchAction --> HandleShell[_handle_shell<br/>-> secator/tasks/command.py command task]
    DispatchAction --> HandleQuery[_handle_query<br/>query_workspace, union live findings]
    DispatchAction --> HandleFinding[_handle_add_finding<br/>field coercion + validate_fields]
    DispatchAction --> RunRunner[_run_runner<br/>spawn task/workflow/ai subagent]

    RunRunner --> SanitizeOpts[_sanitize_child_opts<br/>strip dangerous/interactive/hooks keys]
    SanitizeOpts --> DepthCaps{Depth/fan-out caps ok?}
    DepthCaps -- no --> DenyChild[Warning: deny spawn]
    DepthCaps -- yes --> RebuildHooks[Rebuild persistence hooks from parent]
    RebuildHooks --> RunChild[Run child runner<br/>inherit LLM config if ai]

    HandleShell --> ToolResultOk[Yield Ai shell / shell_output]
    HandleQuery --> ToolResultOk
    HandleFinding --> ToolResultOk
    RunChild --> ToolResultOk
    DenyChild --> ToolResultOk

    ToolResultOk --> AddAssistant2[Append tool_result to history]
    ToolResultDenied --> AddAssistant2
    AddAssistant2 --> MarkComplete[_mark_turn_completed<br/>persist turn_uuid marker]

    MarkComplete --> DrainHistUsage[_drain_history_usage<br/>compaction/summarization tokens]
    DrainHistUsage --> Compact{History over budget?}
    Compact -- yes --> ChatHistoryCompact[ChatHistory.compact / trim<br/>strip leading orphan tool msgs]
    Compact -- no --> SaveHistory
    ChatHistoryCompact --> SaveHistory[_save_history]

    SaveHistory --> LoopExit{Max iterations / done / abort?}
    LoopExit -- no --> RunLoop
    LoopExit -- yes --> End([Return final result])
    SkipTurn --> End

Loading

Key notes reflected in the graph:

• Remote resume now goes through _maybe_resume_remote → restore_history_from_db, splitting message-carrying vs. legacy docs, distinct from the local replay_session path.
• Turn idempotency: _turn_completed_marker / _mark_turn_completed guard against Celery acks_late redelivery replaying tool actions/billing.
• Steering (_drain_steers / RemoteBackend.poll_steers) is polled each loop iteration, not just at follow-up time.
• Guardrails are fail-closed (_is_approved normalizes only exact "allow"), with match_rule now normalizing encoded IPs and wrapper-peeling (_peel_wrapper) so timeout/xargs/sudo wrappers can't launder denied commands.
• Shell execution is no longer subprocess.run — it's dispatched as a first-class command task through the normal runner lifecycle (hooks, persistence, env sanitization).
• Subagent spawning (_run_runner) enforces depth/fan-out caps and refuses to spawn children that would lose persistence hooks.

I'll follow up with the full review results shortly.

✅ Action performed

Full review finished.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
fd . -t f -a | grep -i "secator/tasks/ai.py"
ast-grep outline secator/tasks/ai.py --items all

Length of output: 2007

---

🏁 Script executed:

#!/bin/bash
ast-grep outline secator/tasks/ai.py --items all --json=stream | python3 -c "
import json,sys
for line in sys.stdin:
    d=json.loads(line)
    print(d)
" 2>/dev/null | head -100

echo '---'
sed -n '1,80p' secator/tasks/ai.py

Length of output: 18566

---

🏁 Script executed:

#!/bin/bash
sed -n '176,300p' secator/tasks/ai.py
echo '=== _run_loop ==='
sed -n '451,668p' secator/tasks/ai.py

Length of output: 13391

---

🏁 Script executed:

#!/bin/bash
sed -n '948,1140p' secator/tasks/ai.py

Length of output: 8580

---

@ocervell Kicking off a full review of the PR now. In the meantime, here's the full execution flow ("task graph") for the ai runner in secator/tasks/ai.py, based on yielder, _run_loop, _process_tool_calls, _dispatch_and_collect, and the related helpers in secator/ai/actions.py, secator/ai/interactivity.py, and secator/ai/session.py:

flowchart TD
    A[yielder start] --> B{inputs == ['setup']?}
    B -- yes --> B1[setup_ai / Error if addon missing] --> Z[end]
    B -- no --> C{ai addon enabled?}
    C -- no --> C1[Error: missing addon] --> Z
    C -- yes --> D[_init_options]
    D --> E{show_prompt opt?}
    E -- yes --> E1[print system prompt] --> Z
    E -- no --> F[_verify_model]
    F --> G{model resolved?}
    G -- no --> Z
    G -- yes --> H{interactive == 'remote'?}
    H -- yes --> H1[_maybe_resume_remote]
    H1 --> H2{restored?}
    H2 -- yes --> Z
    H2 -- no --> I
    H -- no --> I{resume flag and not subagent?}
    I -- yes --> I1[show_session_picker]
    I1 --> I2{session picked?}
    I2 -- no --> Z
    I2 -- yes --> I3{session has session_id?}
    I3 -- yes --> I4[adopt session_id, _rebuild_prompt_and_tools, restore_history_from_db, print_session_results]
    I3 -- no --> I5[replay_session legacy]
    I4 --> I6[_prompt_and_redetect]
    I5 --> I6
    I6 --> I7{result None?}
    I7 -- yes --> I8[_save_history] --> Z
    I7 -- no --> J[_run_loop] --> Z
    I -- no --> K[get / prompt user via FullScreenPrompt if empty]
    K --> L[setup session metadata + encrypt session_name]
    L --> M[_detect_mode: fast_detect_mode then LLM fallback]
    M --> N[get_system_prompt + history.set_system + add_user]
    N --> O[yield Ai prompt + Info model/mode]
    O --> P[_run_loop]
    P --> P1[_mark_turn_completed]
    P1 --> Z

    subgraph RunLoop [_run_loop iteration]
      direction TB
      RL0[init_llm, build ActionContext] --> RL1{iteration < max_iterations?}
      RL1 -- no --> RLEND[_save_history] --> Z
      RL1 -- yes --> RL2[_drain_steers]
      RL2 --> RL3[_summarize_auto / _summarize_user]
      RL3 --> RL4[_drain_history_usage]
      RL4 --> RL5[history.to_messages + call_llm]
      RL5 --> RL6[_account_usage]
      RL6 --> RL7{empty content & no tool_calls?}
      RL7 -- yes,streak>=3 --> RLEND
      RL7 -- yes,retry --> RL2
      RL7 -- no --> RL8[_add_assistant_to_history + yield Ai response]
      RL8 --> RL9{tool_calls present?}
      RL9 -- yes --> RL10[_process_tool_calls guardrails]
      RL10 --> RL11{actions empty?}
      RL11 -- yes --> RL12[retry continue msg] --> RL2
      RL11 -- no --> RL13[_dispatch_and_collect]
      RL13 --> RL14{stop_reason?}
      RL14 -- yes --> RLEND
      RL14 -- no --> RL15{follow_up_choices or no tool_calls or last iter?}
      RL15 -- yes --> RL16[_prompt_and_redetect] --> RL17{result None?}
      RL17 -- yes --> RLEND
      RL17 -- no --> RL2
      RL15 -- no --> RL18[add continue_msg to history] --> RL2
      RL9 -- no --> RL15
      RL5 -.exceptions.-> RLEXC[RateLimitError/AuthError/APIConnectionError/other] --> RLEND
    end

    subgraph ProcessToolCalls [_process_tool_calls]
      direction TB
      PT0[for each tool_call] --> PT1{JSON args parse ok?}
      PT1 -- no --> PT2[_reject_tool_call: malformed args]
      PT1 -- yes --> PT3[coerce_stringified_args + decrypt]
      PT3 --> PT4[tool_call_to_action]
      PT4 --> PT5{action valid?}
      PT5 -- no --> PT6[_reject_tool_call: unknown/empty]
      PT5 -- yes --> PT7{self.dangerous?}
      PT7 -- yes --> PT9[append action]
      PT7 -- no --> PT8[check_guardrails ask/allow/deny]
      PT8 --> PT8a{denied?}
      PT8a -- yes --> PT8b[_reject_tool_call: denied]
      PT8a -- no --> PT9
    end

    subgraph DispatchCollect [_dispatch_and_collect]
      direction TB
      DC0{len actions > 1?}
      DC0 -- yes --> DC1[_run_batch: parallel safe_dispatch_action]
      DC0 -- no --> DC2[safe_dispatch_action single]
      DC1 --> DC3[iterate results]
      DC2 --> DC3
      DC3 --> DC4{Ai follow_up?}
      DC4 -- yes --> DC5[stamp pending + prompt_uuid if RemoteBackend]
      DC4 -- no --> DC6{Ai stopped?}
      DC6 -- yes --> DC7[set stop_reason]
      DC3 --> DC8[collect non-query results]
      DC8 --> DC9[group by tool_call_id]
      DC9 --> DC10[format_tool_result + truncate + encrypt]
      DC10 --> DC11[history.add_tool_result + yield Ai tool_result]
    end

    RL13 -.-> DispatchCollect
    RL10 -.-> ProcessToolCalls

Loading

Key sub-flows worth noting from this consolidation PR:

• Remote resume (_maybe_resume_remote → restore_history_from_db) rebuilds ChatHistory from Mongo _type:"ai" docs, with idempotency guarded by _turn_completed_marker/_mark_turn_completed so Celery redelivery can't replay tool actions or double-bill tokens.
• Steering (_drain_steers) injects mid-flight user messages polled via RemoteBackend.poll_steers on every loop iteration.
• Guardrails (check_guardrails in secator/ai/actions.py) gate every parsed tool call before dispatch, fail-closed unless explicitly allow.
• Dispatch resiliency (safe_dispatch_action/_run_batch) converts per-action exceptions into Error tool results instead of aborting the loop.
• Shell commands now dispatch through the new first-class command task runner (secator/tasks/command.py) rather than subprocess.run, so they get the standard runner lifecycle/hooks.

✅ Action performed

Full review finished.

[coderabbitai]

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

secator/ai/guardrails.py (1)

1211-1216: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

One-shot shell approval cannot resolve the current prompt.

Line 1212 intentionally adds no runtime rule, but the prompt loop re-runs check_action() after approval; the same shell ask is returned again until max rounds denies it. Preserve one-shot semantics across future calls, but mark the current invocation as resolved before rechecking target/path layers.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/ai/guardrails.py` around lines 1211 - 1216, The one-shot branch in
the approval flow is returning "allow" without marking the current shell prompt
as resolved, so the next check_action() call re-prompts indefinitely. Update the
shell approval handling in the guardrails logic (around the idx == 0 path in the
approval result handling) to preserve one-shot semantics for future invocations
while also recording the current request as satisfied before the target/path
recheck runs. Keep the runtime-allow behavior unchanged for the idx == 1 path
and make sure the resolution state is tied to the current prompt_cmd invocation.

♻️ Duplicate comments (1)

secator/tasks/command.py (1)

70-97: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Imported timestamps are overwritten only after hooks already persisted the wrong values.

mark_started()/mark_completed() fire on_start/on_end (the persistence hooks) before start_time/end_time are corrected to the caller-supplied values on the next line. Any update_runner-style hook passed via hooks= will therefore persist auto-stamped now() timestamps instead of the actual imported ones.

🔧 Proposed fix: seed timestamps before firing hooks

 		runner = cls(inputs=[command_line], context=context or {}, hooks=hooks or {})
 
-		# mark_started() fires the on_start hook. It also stamps start_time = now(), so
-		# apply the caller-supplied start_time right after (mark_started() unconditionally
-		# overwrites it, there's no way to seed it beforehand).
-		runner.mark_started()
-		runner.start_time = start_time or datetime.fromtimestamp(time(), timezone.utc)
+		runner.started = True
+		runner.start_time = start_time or datetime.fromtimestamp(time(), timezone.utc)
+		runner.run_hooks('on_start', sub='start')
 
 		# Populate the captured result.
 		runner.output = output
 		runner.return_code = return_code
 		if return_code != 0:
 			runner.add_result(
 				Error(message=f'Command exited with return code {return_code}'),
 				print=False,
 				output=False,
 			)
 
-		# mark_completed() fires the on_end hook (the persistence path). Same caveat as
-		# start_time: it unconditionally stamps end_time = now(), so apply the
-		# caller-supplied end_time right after.
-		runner.mark_completed()
-		runner.end_time = end_time or datetime.fromtimestamp(time(), timezone.utc)
+		runner.done = True
+		runner.progress = 100
+		runner.end_time = end_time or datetime.fromtimestamp(time(), timezone.utc)
+		runner.mark_duplicates()
+		runner.run_hooks('on_end', sub='end')

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/command.py` around lines 70 - 97, The imported timestamps are
being overwritten only after the persistence hooks already run, so
`mark_started()` and `mark_completed()` in `Command.from_*` need to use the
caller-supplied times before firing `on_start`/`on_end`. Move the `start_time`
and `end_time` assignment ahead of `runner.mark_started()` and
`runner.mark_completed()`, or otherwise seed those values before hooks execute,
so any `hooks=` implementation such as `update_runner` persists the intended
imported timestamps. Refer to `Command`, `mark_started()`, and
`mark_completed()` when making the change.

🧹 Nitpick comments (2)

pyproject.toml (1)

93-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Pin safecmd and shfmt-py versions for consistency.

Every other dependency in this file (including litellm < 2 right above) carries an upper-bound version pin, but safecmd and shfmt-py are added unpinned. Since these two packages directly drive the shell-guardrail parser (security-relevant), an unexpected breaking release could silently degrade or break the parser without any signal at install time.

♻️ Suggested pin

 ai = [
 	'litellm < 2',
-	'safecmd',
+	'safecmd < 2',
 	# 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'
+	'shfmt-py < 5'
 ]

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pyproject.toml` around lines 93 - 100, The ai dependency list currently adds
safecmd and shfmt-py without version bounds, unlike the other entries in this
group. Update the ai extras in pyproject.toml to pin both safecmd and shfmt-py
to specific compatible versions (with an upper bound if appropriate) so the
shell-guardrail path remains stable; keep the change localized to the ai array
alongside litellm < 2.

secator/ai/actions.py (1)

675-679: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Clamp AI-provided query limits.

int(limit) accepts very large or negative values, which can drive expensive workspace searches and oversized tool results. Clamp to a bounded range before calling engine.search().

Suggested fix

 	limit = action.get("limit", 100)
 	try:
 		limit = int(limit)
 	except (TypeError, ValueError):
 		limit = 100
+	limit = max(1, min(limit, 1000))

Also applies to: 714-720

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/ai/actions.py` around lines 675 - 679, Clamp the AI-supplied limit
before it reaches engine.search() in the action parsing flow: after converting
action.get("limit", 100) to an int, bound it to a safe minimum and maximum
instead of only falling back on parse errors. Apply the same normalization in
both limit-handling blocks referenced by action parsing so negative or
excessively large values cannot produce expensive searches or oversized results.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@secator/ai/actions.py`:
- Around line 241-245: In the shell approval flow inside actions.py, the
one-shot “allow this invocation” response is not persisted, so the later recheck
in the same execution re-prompts and fails closed. Update the approval handling
around the shell `ask`/recheck logic so a current-invocation approval marker is
recorded or the already-approved shell layer is skipped on the second pass,
while still enforcing target/path checks in the same control path.

In `@secator/ai/guardrails.py`:
- Around line 1020-1033: Normalize each target value before evaluating platform
mandate regexes in the target-check path so encoded or alternate IP forms cannot
bypass scope rules. Update the logic around the denied_targets and
allowed_targets checks to canonicalize values the same way match_rule() does via
_normalize_ip() before calling _matches_denied_targets() and
_matches_allowed_targets(), while preserving the existing deny-wins ordering in
guardrails.py.

In `@secator/ai/interactivity.py`:
- Around line 285-292: The timeout update in the AI interactivity flow is
matching all pending AI documents for a session, including steer interjections,
which can mark them timed out before _drain_steers() processes them. Update the
query in the affected update call to target only prompt-like documents by adding
the appropriate ai_type filter used elsewhere in this flow, and keep the status
transition to timed_out limited to those prompt records.

In `@secator/ai/utils.py`:
- Around line 36-38: The environment-variable filter currently only excludes
uppercase secret names, so lowercase or mixed-case names like openai_api_key and
database_password still leak through. Update the filtering logic in the
environment comprehension in utils.py to compare names case-insensitively by
normalizing each key before checking against SENSITIVE_ENV_PREFIXES and the
KEY/SECRET/TOKEN/PASSWORD substring rules. Keep the existing behavior of
returning a sanitized dict, but ensure all secret-like env var names are blocked
regardless of case.

In `@secator/tasks/ai.py`:
- Around line 361-363: The prompt-loading logic in ai.py is expanding any
path-like prompt in the task execution flow, which can incorrectly read
worker-local files for remote continuations. Update the prompt handling around
the prompt assignment and Path(...).is_file() check so file-based prompt
expansion happens only in the local CLI path, not in the remote/task execution
path; keep self.prompt as the raw string on remote runs and only resolve/read
prompt files in the CLI-specific entrypoint or guarded local-only branch.
- Around line 555-568: The persisted Ai response is storing the wrong model
identifier, since this branch generates output from self.model but records
self.intent_model instead. Update the Ai creation in this response path to
persist the actual generation model (the self.model value) while keeping the
rest of the metadata unchanged, and use the surrounding Ai yield block to locate
the fix.

In `@tests/unit/test_ai_actions.py`:
- Around line 656-708: The new tests use semicolon-joined statements, which
triggers flake8 E702 and will fail lint. Update the affected test methods in
test_run_runner_structures_subagent_prompt,
test_run_runner_subagent_inherits_parent_llm_config, and
test_run_runner_subagent_explicit_model_wins by splitting the MagicMock setup
and related assignments into separate lines, keeping the same test behavior
while matching the repo’s style rules.

In `@tests/unit/test_ai_loop.py`:
- Around line 386-388: The test is still asserting a removed top-level
session_id on the Ai document, so update the check to target the persisted
context value instead. In test_ai_loop.py, use the document’s _context/session
metadata after the fake persistence path or runner add_result stamping, and
reference the Ai document shape rather than doc.session_id so the assertion
matches what serialization actually stores.

---

Outside diff comments:
In `@secator/ai/guardrails.py`:
- Around line 1211-1216: The one-shot branch in the approval flow is returning
"allow" without marking the current shell prompt as resolved, so the next
check_action() call re-prompts indefinitely. Update the shell approval handling
in the guardrails logic (around the idx == 0 path in the approval result
handling) to preserve one-shot semantics for future invocations while also
recording the current request as satisfied before the target/path recheck runs.
Keep the runtime-allow behavior unchanged for the idx == 1 path and make sure
the resolution state is tied to the current prompt_cmd invocation.

---

Duplicate comments:
In `@secator/tasks/command.py`:
- Around line 70-97: The imported timestamps are being overwritten only after
the persistence hooks already run, so `mark_started()` and `mark_completed()` in
`Command.from_*` need to use the caller-supplied times before firing
`on_start`/`on_end`. Move the `start_time` and `end_time` assignment ahead of
`runner.mark_started()` and `runner.mark_completed()`, or otherwise seed those
values before hooks execute, so any `hooks=` implementation such as
`update_runner` persists the intended imported timestamps. Refer to `Command`,
`mark_started()`, and `mark_completed()` when making the change.

---

Nitpick comments:
In `@pyproject.toml`:
- Around line 93-100: The ai dependency list currently adds safecmd and shfmt-py
without version bounds, unlike the other entries in this group. Update the ai
extras in pyproject.toml to pin both safecmd and shfmt-py to specific compatible
versions (with an upper bound if appropriate) so the shell-guardrail path
remains stable; keep the change localized to the ai array alongside litellm < 2.

In `@secator/ai/actions.py`:
- Around line 675-679: Clamp the AI-supplied limit before it reaches
engine.search() in the action parsing flow: after converting action.get("limit",
100) to an int, bound it to a safe minimum and maximum instead of only falling
back on parse errors. Apply the same normalization in both limit-handling blocks
referenced by action parsing so negative or excessively large values cannot
produce expensive searches or oversized results.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: bae0b534-c4c7-4b0b-80fb-3ae4299dfb84

📥 Commits

Reviewing files that changed from the base of the PR and between 9b11716 and 0336da3.

📒 Files selected for processing (36)

• .github/workflows/publish-canary.yml
• pyproject.toml
• secator/ai/actions.py
• secator/ai/guardrails.py
• secator/ai/history.py
• secator/ai/interactivity.py
• secator/ai/prompts.py
• secator/ai/prompts/constraints/common.txt
• secator/ai/session.py
• secator/ai/tools.py
• secator/ai/utils.py
• secator/celery_signals.py
• secator/exporters/markdown.py
• secator/hooks/_dedup.py
• secator/hooks/api.py
• secator/output_types/_base.py
• secator/output_types/ai.py
• secator/runners/_base.py
• secator/runners/command.py
• secator/tasks/ai.py
• secator/tasks/command.py
• tests/unit/test_ai_actions.py
• tests/unit/test_ai_guardrails.py
• tests/unit/test_ai_history.py
• tests/unit/test_ai_interactivity.py
• tests/unit/test_ai_loop.py
• tests/unit/test_ai_prompts.py
• tests/unit/test_ai_resilience.py
• tests/unit/test_ai_session.py
• tests/unit/test_ai_task_opts.py
• tests/unit/test_ai_tokens.py
• tests/unit/test_ai_tools.py
• tests/unit/test_ai_utils.py
• tests/unit/test_api_hook_transport.py
• tests/unit/test_command_task.py
• tests/unit/test_eviction.py

💤 Files with no reviewable changes (1)

• .github/workflows/publish-canary.yml

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Normalize target values before mandate regex checks.

Config rules use _normalize_ip() via match_rule(), but allowed_targets/denied_targets mandates compare the raw target string. Encoded IP forms can therefore bypass platform deny/allow scopes even though normal rules canonicalize them.

Suggested direction

 		if rule_type == "target" and self.denied_targets:
 			for v in values_to_check:
-				if self._matches_denied_targets(v):
+				norm = _normalize_ip(v)
+				candidates = [v] + ([str(norm)] if norm is not None else [])
+				if any(self._matches_denied_targets(candidate) for candidate in candidates):
 					return PermissionResult(decision="deny", reason=f"Denied by mandate: target({v})")
 
 		# Platform-supplied allowed_targets (regex) allow-list — checked after deny
 		# (deny still wins) but before config/runtime allow rules.
 		if rule_type == "target" and self.allowed_targets:
 			for v in values_to_check:
-				if self._matches_allowed_targets(v):
+				norm = _normalize_ip(v)
+				candidates = [v] + ([str(norm)] if norm is not None else [])
+				if any(self._matches_allowed_targets(candidate) for candidate in candidates):
 					return PermissionResult(decision="allow", reason=f"Allowed by mandate: target({v})")

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	# Platform-supplied denied_targets (regex) deny-list — checked before the
	# allowed_targets allow-list so DENY WINS: a target matching both an allow
	# and a deny mandate scope is denied (mirrors the mandate scope matcher).
	if rule_type == "target" and self.denied_targets:
	for v in values_to_check:
	if self._matches_denied_targets(v):
	return PermissionResult(decision="deny", reason=f"Denied by mandate: target({v})")
	# Platform-supplied allowed_targets (regex) allow-list — checked after deny
	# (deny still wins) but before config/runtime allow rules.
	if rule_type == "target" and self.allowed_targets:
	for v in values_to_check:
	if self._matches_allowed_targets(v):
	return PermissionResult(decision="allow", reason=f"Allowed by mandate: target({v})")
	# Platform-supplied denied_targets (regex) deny-list — checked before the
	# allowed_targets allow-list so DENY WINS: a target matching both an allow
	# and a deny mandate scope is denied (mirrors the mandate scope matcher).
	if rule_type == "target" and self.denied_targets:
	for v in values_to_check:
	norm = _normalize_ip(v)
	candidates = [v] + ([str(norm)] if norm is not None else [])
	if any(self._matches_denied_targets(candidate) for candidate in candidates):
	return PermissionResult(decision="deny", reason=f"Denied by mandate: target({v})")
	# Platform-supplied allowed_targets (regex) allow-list — checked after deny
	# (deny still wins) but before config/runtime allow rules.
	if rule_type == "target" and self.allowed_targets:
	for v in values_to_check:
	norm = _normalize_ip(v)
	candidates = [v] + ([str(norm)] if norm is not None else [])
	if any(self._matches_allowed_targets(candidate) for candidate in candidates):
	return PermissionResult(decision="allow", reason=f"Allowed by mandate: target({v})")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/ai/guardrails.py` around lines 1020 - 1033, Normalize each target
value before evaluating platform mandate regexes in the target-check path so
encoded or alternate IP forms cannot bypass scope rules. Update the logic around
the denied_targets and allowed_targets checks to canonicalize values the same
way match_rule() does via _normalize_ip() before calling
_matches_denied_targets() and _matches_allowed_targets(), while preserving the
existing deny-wins ordering in guardrails.py.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Do not expire pending steers as stale prompts.

This update matches every pending AI doc for the session, including ai_type="steer", so creating a follow-up/permission prompt can time out user interjections before _drain_steers() sees them. Restrict this to prompt-like docs only.

Proposed fix

-		self.query_engine.update(
-			{
-				"_type": "ai",
-				"_context.session_id": session_id,
-				"status": "pending",
-			},
-			{"$set": {"status": "timed_out"}},
-		)
+		for ai_type in ("follow_up", "permission"):
+			self.query_engine.update(
+				{
+					"_type": "ai",
+					"ai_type": ai_type,
+					"_context.session_id": session_id,
+					"status": "pending",
+				},
+				{"$set": {"status": "timed_out"}},
+			)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	self.query_engine.update(
	{
	"_type": "ai",
	"_context.session_id": session_id,
	"status": "pending",
	},
	{"$set": {"status": "timed_out"}},
	)
	for ai_type in ("follow_up", "permission"):
	self.query_engine.update(
	{
	"_type": "ai",
	"ai_type": ai_type,
	"_context.session_id": session_id,
	"status": "pending",
	},
	{"$set": {"status": "timed_out"}},
	)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/ai/interactivity.py` around lines 285 - 292, The timeout update in
the AI interactivity flow is matching all pending AI documents for a session,
including steer interjections, which can mark them timed out before
_drain_steers() processes them. Update the query in the affected update call to
target only prompt-like documents by adding the appropriate ai_type filter used
elsewhere in this flow, and keep the status transition to timed_out limited to
those prompt records.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Make secret-name filtering case-insensitive.

Lowercase or mixed-case env vars like openai_api_key or database_password currently pass through to AI shell commands.

Proposed fix

-	return {k: v for k, v in os.environ.items()
-			if not any(k.startswith(p) for p in SENSITIVE_ENV_PREFIXES)
-			and "KEY" not in k and "SECRET" not in k and "TOKEN" not in k and "PASSWORD" not in k}
+	return {
+		k: v for k, v in os.environ.items()
+		if not any(k.upper().startswith(p) for p in SENSITIVE_ENV_PREFIXES)
+		and "KEY" not in k.upper()
+		and "SECRET" not in k.upper()
+		and "TOKEN" not in k.upper()
+		and "PASSWORD" not in k.upper()
+	}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	return {k: v for k, v in os.environ.items()
	if not any(k.startswith(p) for p in SENSITIVE_ENV_PREFIXES)
	and "KEY" not in k and "SECRET" not in k and "TOKEN" not in k and "PASSWORD" not in k}
	return {
	k: v for k, v in os.environ.items()
	if not any(k.upper().startswith(p) for p in SENSITIVE_ENV_PREFIXES)
	and "KEY" not in k.upper()
	and "SECRET" not in k.upper()
	and "TOKEN" not in k.upper()
	and "PASSWORD" not in k.upper()
	}

🧰 Tools

🪛 Flake8 (7.3.0)

[error] 37-37: continuation line under-indented for visual indent

(E128)

---

[error] 38-38: continuation line under-indented for visual indent

(E128)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/ai/utils.py` around lines 36 - 38, The environment-variable filter
currently only excludes uppercase secret names, so lowercase or mixed-case names
like openai_api_key and database_password still leak through. Update the
filtering logic in the environment comprehension in utils.py to compare names
case-insensitively by normalizing each key before checking against
SENSITIVE_ENV_PREFIXES and the KEY/SECRET/TOKEN/PASSWORD substring rules. Keep
the existing behavior of returning a sanitized dict, but ensure all secret-like
env var names are blocked regardless of case.

[coderabbitai]

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Do not load prompt paths on the remote path.

A remote continuation prompt that happens to be a worker-local path is read and sent into the AI transcript. Keep file-prompt expansion local/CLI-only.

Proposed fix

-		if self.prompt and Path(self.prompt).is_file():
+		if self.interactive != "remote" and self.prompt and Path(self.prompt).is_file():
 			self.prompt = Path(self.prompt).read_text().strip()

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	self.prompt = self.run_opts.get("prompt", "")
	if self.prompt and Path(self.prompt).is_file():
	self.prompt = Path(self.prompt).read_text().strip()
	self.prompt = self.run_opts.get("prompt", "")
	if self.interactive != "remote" and self.prompt and Path(self.prompt).is_file():
	self.prompt = Path(self.prompt).read_text().strip()

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/ai.py` around lines 361 - 363, The prompt-loading logic in
ai.py is expanding any path-like prompt in the task execution flow, which can
incorrectly read worker-local files for remote continuations. Update the prompt
handling around the prompt assignment and Path(...).is_file() check so
file-based prompt expansion happens only in the local CLI path, not in the
remote/task execution path; keep self.prompt as the raw string on remote runs
and only resolve/read prompt files in the CLI-specific entrypoint or guarded
local-only branch.

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Persist the actual response model.

This response is generated by self.model, but the persisted Ai item records self.intent_model, which corrupts response metadata and downstream reporting.

Proposed fix

-					model=self.intent_model,
+					model=self.model,

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	yield Ai(
	content=display_content,
	ai_type="response",
	mode=self.mode,
	model=self.intent_model,
	summary=not tool_calls,
	message=cap_message(assistant_msg),
	extra_data={
	"iteration": iteration,
	"max_iterations": self.max_iterations,
	"tokens": usage.get("tokens") if usage else None,
	"cost": usage.get("cost") if usage else None,
	},
	)
	yield Ai(
	content=display_content,
	ai_type="response",
	mode=self.mode,
	model=self.model,
	summary=not tool_calls,
	message=cap_message(assistant_msg),
	extra_data={
	"iteration": iteration,
	"max_iterations": self.max_iterations,
	"tokens": usage.get("tokens") if usage else None,
	"cost": usage.get("cost") if usage else None,
	},
	)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/ai.py` around lines 555 - 568, The persisted Ai response is
storing the wrong model identifier, since this branch generates output from
self.model but records self.intent_model instead. Update the Ai creation in this
response path to persist the actual generation model (the self.model value)
while keeping the rest of the metadata unchanged, and use the surrounding Ai
yield block to locate the fix.

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert _context.session_id, not removed top-level session_id.

Ai no longer declares a persisted top-level session_id; this assertion can fail or validate a dynamic attribute that serialization will not persist. Check doc._context["session_id"] once the fake persistence path stamps context, or update the harness to simulate runner add_result stamping before asserting.

Suggested test direction

-		self.assertEqual(doc.session_id, "sess-123")
+		self.assertEqual(doc._context.get("session_id"), "sess-123")

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	self.assertEqual(doc.status, "pending")
	self.assertEqual(doc.choices, ["Fuzz parameters", "Run nuclei", "Deep crawl"])
	self.assertEqual(doc.session_id, "sess-123")
	self.assertEqual(doc.status, "pending")
	self.assertEqual(doc.choices, ["Fuzz parameters", "Run nuclei", "Deep crawl"])
	self.assertEqual(doc._context.get("session_id"), "sess-123")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/unit/test_ai_loop.py` around lines 386 - 388, The test is still
asserting a removed top-level session_id on the Ai document, so update the check
to target the persisted context value instead. In test_ai_loop.py, use the
document’s _context/session metadata after the fake persistence path or runner
add_result stamping, and reference the Ai document shape rather than
doc.session_id so the assertion matches what serialization actually stores.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

secator/tasks/ai.py (2)

793-833: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Initialize tool_schemas for empty-prompt resumes. _maybe_resume_remote() can reach _run_loop() with self.prompt == "", but _detect_mode() returns early in that case without rebuilding tools. That leaves self.tool_schemas unset and call_llm(..., tools=self.tool_schemas) fails on the resumed turn.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/ai.py` around lines 793 - 833, The empty-prompt resume path in
_detect_mode leaves tool_schemas uninitialized because the early return for not
self.prompt skips the rebuild logic. Update _detect_mode in secator/tasks/ai.py
so that when it exits early for an empty prompt it still ensures
self.tool_schemas is initialized, either by calling _rebuild_prompt_and_tools()
or by explicitly building the schemas before returning, and keep the existing
rebuild behavior consistent with the old_mode/tool_schemas checks used later in
the method.

---

256-259: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win

Block file-path fallback in remote mode.
Fresh interactive=="remote" runs still fall through here after _maybe_resume_remote() returns False, so a user-supplied remote prompt can be treated as a local path and read from disk. Gate this branch off interactive=="remote" or share the same helper with _maybe_resume_remote().

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/ai.py` around lines 256 - 259, The prompt loading fallback in
the task setup can wrongly treat a remote user prompt as a local file path after
_maybe_resume_remote() declines to resume. Update the prompt handling in the AI
task flow around self.prompt in the task class so that the
Path(self.prompt).is_file() / read_text() branch is skipped when
interactive=="remote", or centralize this check with the existing
_maybe_resume_remote() logic to keep remote mode from reading local disk paths.

♻️ Duplicate comments (2)

secator/tasks/ai.py (2)

360-364: 🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win

Remote continuation still reads worker-local files from untrusted prompt text.

A remote (web) user's message is used directly as a filesystem path and read if it happens to match an existing file on the worker (Path(self.prompt).is_file() → read_text()). A remote user can trivially craft a message equal to a sensitive path (e.g. /etc/passwd, /proc/self/environ, mounted secrets) to have its contents read and injected into the AI transcript — this is an arbitrary local file read (LFI) via untrusted input, exactly the concern raised in a prior review that remains unfixed. File-prompt expansion should be local/CLI-only.

🔒 Proposed fix

-		self.prompt = self.run_opts.get("prompt", "")
-		if self.prompt and Path(self.prompt).is_file():
-			self.prompt = Path(self.prompt).read_text().strip()
+		# Remote prompts originate from an untrusted web user; never expand them
+		# as worker-local filesystem paths (file-prompt expansion is CLI-only).
+		self.prompt = self.run_opts.get("prompt", "")

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/ai.py` around lines 360 - 364, The prompt handling in ai.py
still treats untrusted remote message text as a local file path inside the
respawn prompt resolution logic. Update the prompt-loading flow in the code
around self.run_opts.get("prompt", "") so file expansion only happens for
trusted local/CLI inputs, and never for web/remote continuation messages. Use
the prompt resolution path in the respawn handling to gate
Path(self.prompt).is_file() and read_text() behind a trusted-source check or
remove file dereferencing from remote inputs entirely.

---

555-568: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Persisted response still records intent_model, not the generating model.

This response is generated by self.model but the persisted Ai item stores self.intent_model, corrupting response metadata/reporting. This exact issue was flagged in a previous review and remains unfixed.

🐛 Proposed fix

-					model=self.intent_model,
+					model=self.model,

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/ai.py` around lines 555 - 568, The persisted Ai response is
storing the wrong model metadata because this response is generated by
self.model but the yielded Ai item uses self.intent_model. Update the Ai
construction in the response path so the model field reflects the generating
model used for the assistant message, and keep the intent model only where it is
actually intended for routing/selection; use the surrounding yield Ai block and
self.model/self.intent_model references to locate and correct it.

🧹 Nitpick comments (3)

secator/celery_signals.py (1)

36-43: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider a higher log level for the flag-write failure.

If writing SHUTDOWN_FLAG fails, eviction detection is silently disabled for this worker — per the module's own comments, an in-flight task could then hang until the broker visibility timeout (potentially hours). Logging this at Info risks it going unnoticed; Warning better reflects the operational impact.

♻️ Proposed fix

 	except OSError as e:
-		console.print(Info(message=f'Failed to raise worker shutdown flag: {e}'))
+		console.print(Warning(message=f'Failed to raise worker shutdown flag: {e}'))

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/celery_signals.py` around lines 36 - 43, The shutdown-flag write
failure in worker_shutting_down_handler is logged at too low a severity for an
issue that disables eviction detection and can leave tasks running until broker
timeout. Update the console.print call in worker_shutting_down_handler to use a
Warning-level message instead of Info, keeping the existing error details so the
operational impact is more visible.

secator/tasks/ai.py (2)

1060-1080: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win

Dead follow_up_ai.session_id assignment — Ai no longer has that field.

Ai dropped its top-level session_id field in favor of _context.session_id (see secator/output_types/ai.py). Setting follow_up_ai.session_id = self.session_id here just creates a stray instance attribute that is never serialized/persisted (confirmed: plain, non-frozen/non-slotted dataclasses silently accept arbitrary extra attributes but toDict()/field-based serialization won't see them). It's harmless today only because correlation actually happens via _context.session_id (stamped by add_result), but it's misleading dead code and the reason tests/unit/test_ai_loop.py's doc.session_id assertion passes despite the field removal.

♻️ Proposed cleanup

 					if isinstance(self.backend, RemoteBackend):
 						follow_up_ai.status = "pending"
-						follow_up_ai.session_id = self.session_id
 						if not follow_up_ai.choices and follow_up_choices:

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/ai.py` around lines 1060 - 1080, The follow-up branch in Ai
result handling contains a dead assignment because Ai no longer exposes a
top-level session_id field. Remove the direct follow_up_ai.session_id =
self.session_id write in the result-processing block around result.ai_type ==
"follow_up", and rely on the existing _context.session_id stamping done by
add_result for persistence and correlation. Keep the prompt_uuid/choices/pending
status logic intact and make sure no code path still assumes a serialized
top-level session_id on Ai.

---

1195-1216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

_add_assistant_to_history reimplements tool-call formatting instead of reusing ChatHistory's helper.

ChatHistory.add_assistant_with_tool_calls (used elsewhere, e.g. in tests) appears to already build the litellm tool-call message shape. This method duplicates that transformation manually (constructing litellm_tool_calls, encrypting, appending to self.history.messages directly) presumably because the shared helper doesn't return the built message. Consider extending add_assistant_with_tool_calls to return the appended message and calling it here, to avoid the two implementations drifting.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@secator/tasks/ai.py` around lines 1195 - 1216, _add_assistant_to_history is
duplicating the litellm tool-call message निर्माण logic instead of reusing
ChatHistory.add_assistant_with_tool_calls, which can cause the two paths to
drift. Update ChatHistory.add_assistant_with_tool_calls to return the appended
assistant message, then have _add_assistant_to_history call that shared helper
for the tool_calls branch and keep encryption/content handling consistent there.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@secator/tasks/ai.py`:
- Around line 793-833: The empty-prompt resume path in _detect_mode leaves
tool_schemas uninitialized because the early return for not self.prompt skips
the rebuild logic. Update _detect_mode in secator/tasks/ai.py so that when it
exits early for an empty prompt it still ensures self.tool_schemas is
initialized, either by calling _rebuild_prompt_and_tools() or by explicitly
building the schemas before returning, and keep the existing rebuild behavior
consistent with the old_mode/tool_schemas checks used later in the method.
- Around line 256-259: The prompt loading fallback in the task setup can wrongly
treat a remote user prompt as a local file path after _maybe_resume_remote()
declines to resume. Update the prompt handling in the AI task flow around
self.prompt in the task class so that the Path(self.prompt).is_file() /
read_text() branch is skipped when interactive=="remote", or centralize this
check with the existing _maybe_resume_remote() logic to keep remote mode from
reading local disk paths.

---

Duplicate comments:
In `@secator/tasks/ai.py`:
- Around line 360-364: The prompt handling in ai.py still treats untrusted
remote message text as a local file path inside the respawn prompt resolution
logic. Update the prompt-loading flow in the code around
self.run_opts.get("prompt", "") so file expansion only happens for trusted
local/CLI inputs, and never for web/remote continuation messages. Use the prompt
resolution path in the respawn handling to gate Path(self.prompt).is_file() and
read_text() behind a trusted-source check or remove file dereferencing from
remote inputs entirely.
- Around line 555-568: The persisted Ai response is storing the wrong model
metadata because this response is generated by self.model but the yielded Ai
item uses self.intent_model. Update the Ai construction in the response path so
the model field reflects the generating model used for the assistant message,
and keep the intent model only where it is actually intended for
routing/selection; use the surrounding yield Ai block and
self.model/self.intent_model references to locate and correct it.

---

Nitpick comments:
In `@secator/celery_signals.py`:
- Around line 36-43: The shutdown-flag write failure in
worker_shutting_down_handler is logged at too low a severity for an issue that
disables eviction detection and can leave tasks running until broker timeout.
Update the console.print call in worker_shutting_down_handler to use a
Warning-level message instead of Info, keeping the existing error details so the
operational impact is more visible.

In `@secator/tasks/ai.py`:
- Around line 1060-1080: The follow-up branch in Ai result handling contains a
dead assignment because Ai no longer exposes a top-level session_id field.
Remove the direct follow_up_ai.session_id = self.session_id write in the
result-processing block around result.ai_type == "follow_up", and rely on the
existing _context.session_id stamping done by add_result for persistence and
correlation. Keep the prompt_uuid/choices/pending status logic intact and make
sure no code path still assumes a serialized top-level session_id on Ai.
- Around line 1195-1216: _add_assistant_to_history is duplicating the litellm
tool-call message निर्माण logic instead of reusing
ChatHistory.add_assistant_with_tool_calls, which can cause the two paths to
drift. Update ChatHistory.add_assistant_with_tool_calls to return the appended
assistant message, then have _add_assistant_to_history call that shared helper
for the tool_calls branch and keep encryption/content handling consistent there.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2b25acc7-9fb8-41d3-9628-fe152a3b39ed

📥 Commits

Reviewing files that changed from the base of the PR and between 9b11716 and 0336da3.

📒 Files selected for processing (36)

• .github/workflows/publish-canary.yml
• pyproject.toml
• secator/ai/actions.py
• secator/ai/guardrails.py
• secator/ai/history.py
• secator/ai/interactivity.py
• secator/ai/prompts.py
• secator/ai/prompts/constraints/common.txt
• secator/ai/session.py
• secator/ai/tools.py
• secator/ai/utils.py
• secator/celery_signals.py
• secator/exporters/markdown.py
• secator/hooks/_dedup.py
• secator/hooks/api.py
• secator/output_types/_base.py
• secator/output_types/ai.py
• secator/runners/_base.py
• secator/runners/command.py
• secator/tasks/ai.py
• secator/tasks/command.py
• tests/unit/test_ai_actions.py
• tests/unit/test_ai_guardrails.py
• tests/unit/test_ai_history.py
• tests/unit/test_ai_interactivity.py
• tests/unit/test_ai_loop.py
• tests/unit/test_ai_prompts.py
• tests/unit/test_ai_resilience.py
• tests/unit/test_ai_session.py
• tests/unit/test_ai_task_opts.py
• tests/unit/test_ai_tokens.py
• tests/unit/test_ai_tools.py
• tests/unit/test_ai_utils.py
• tests/unit/test_api_hook_transport.py
• tests/unit/test_command_task.py
• tests/unit/test_eviction.py

💤 Files with no reviewable changes (1)

• .github/workflows/publish-canary.yml